Attachment 122009 Details for Bug 94307 – Only start retire worker when idle

[patch] Only start retire worker when idle

0001-drm-i915-Only-start-retire-worker-when-idle.patch (text/plain), 9.38 KB, created by Chris Wilson on 2016-02-27 20:18:40 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Wilson

Created: 2016-02-27 20:18:40 UTC

Size: 9.38 KB

patch

obsolete

>From 2df781037133af4dfdb29c982873b315a03326d9 Mon Sep 17 00:00:00 2001
>From: Chris Wilson <chris@chris-wilson.co.uk>
>Date: Tue, 8 Dec 2015 20:51:32 +0000
>Subject: [PATCH] drm/i915: Only start retire worker when idle
>
>The retire worker is a low frequency task that makes sure we retire
>outstanding requests if userspace is being lax. We only need to start it
>once as it remains active until the GPU is idle, so do a cheap test
>before the more expensive queue_work(). A consequence of this is that we
>need correct locking in the worker to make the hot path of request
>submission cheap. To keep the symmetry and keep hangcheck strictly bound
>by the GPU's wakelock, we move the cancel_sync(hangcheck) to the idle
>worker before dropping the wakelock.
>
>v2: Guard against RCU fouling the breadcrumbs bottom-half whilst we kick
>the waiter.
>v3: Remove the wakeref assertion squelching (now we hold a wakeref for
>the hangcheck, any rpm error there is genuine).
>
>Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>References: https://bugs.freedesktop.org/show_bug.cgi?id=88437
>---
> drivers/gpu/drm/i915/i915_drv.c            |  2 -
> drivers/gpu/drm/i915/i915_drv.h            |  2 +-
> drivers/gpu/drm/i915/i915_gem.c            | 77 ++++++++++++++++++++----------
> drivers/gpu/drm/i915/i915_gem_execbuffer.c |  6 +++
> drivers/gpu/drm/i915/i915_irq.c            | 17 ++-----
> drivers/gpu/drm/i915/intel_display.c       | 29 -----------
> 6 files changed, 62 insertions(+), 71 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>index b545796..da08379 100644
>--- a/drivers/gpu/drm/i915/i915_drv.c
>+++ b/drivers/gpu/drm/i915/i915_drv.c
>@@ -1495,8 +1495,6 @@ static int intel_runtime_suspend(struct device *device)
> 	i915_gem_release_all_mmaps(dev_priv);
> 	mutex_unlock(&dev->struct_mutex);
> 
>-	cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
>-
> 	intel_guc_suspend(dev);
> 
> 	intel_suspend_gt_powersave(dev);
>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>index 8758f90..fe4b6f7 100644
>--- a/drivers/gpu/drm/i915/i915_drv.h
>+++ b/drivers/gpu/drm/i915/i915_drv.h
>@@ -3037,7 +3037,7 @@ int __must_check i915_gem_set_seqno(struct drm_device *dev, u32 seqno);
> struct drm_i915_gem_request *
> i915_gem_find_active_request(struct intel_engine_cs *ring);
> 
>-bool i915_gem_retire_requests(struct drm_device *dev);
>+void i915_gem_retire_requests(struct drm_device *dev);
> void i915_gem_retire_requests_ring(struct intel_engine_cs *ring);
> 
> static inline u32 i915_reset_counter(struct i915_gpu_error *error)
>diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>index 16bca71..d704cc2 100644
>--- a/drivers/gpu/drm/i915/i915_gem.c
>+++ b/drivers/gpu/drm/i915/i915_gem.c
>@@ -2480,6 +2480,24 @@ i915_gem_get_seqno(struct drm_device *dev, u32 *seqno)
> 	return 0;
> }
> 
>+static void i915_gem_mark_busy(struct drm_i915_private *dev_priv)
>+{
>+	if (dev_priv->mm.busy)
>+		return;
>+
>+	dev_priv->mm.busy = true;
>+
>+	intel_runtime_pm_get_noresume(dev_priv);
>+
>+	i915_update_gfx_val(dev_priv);
>+	if (INTEL_INFO(dev_priv)->gen >= 6)
>+		gen6_rps_busy(dev_priv);
>+
>+	queue_delayed_work(dev_priv->wq,
>+			   &dev_priv->mm.retire_work,
>+			   round_jiffies_up_relative(HZ));
>+}
>+
> /*
>  * NB: This function is not allowed to fail. Doing so would mean the the
>  * request is not being tracked for completion but the work itself is
>@@ -2560,10 +2578,7 @@ void __i915_add_request(struct drm_i915_gem_request *request,
> 
> 	trace_i915_gem_request_add(request);
> 
>-	queue_delayed_work(dev_priv->wq,
>-			   &dev_priv->mm.retire_work,
>-			   round_jiffies_up_relative(HZ));
>-	intel_mark_busy(dev_priv->dev);
>+	i915_gem_mark_busy(dev_priv);
> 
> 	/* Sanity check that the reserved size was large enough. */
> 	intel_ring_reserved_space_end(ringbuf);
>@@ -2917,7 +2932,7 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring)
> 	WARN_ON(i915_verify_lists(ring->dev));
> }
> 
>-bool
>+void
> i915_gem_retire_requests(struct drm_device *dev)
> {
> 	struct drm_i915_private *dev_priv = dev->dev_private;
>@@ -2925,6 +2940,9 @@ i915_gem_retire_requests(struct drm_device *dev)
> 	bool idle = true;
> 	int i;
> 
>+	if (!dev_priv->mm.busy)
>+		return;
>+
> 	for_each_ring(ring, dev_priv, i) {
> 		i915_gem_retire_requests_ring(ring);
> 		idle &= list_empty(&ring->request_list);
>@@ -2941,8 +2959,6 @@ i915_gem_retire_requests(struct drm_device *dev)
> 		mod_delayed_work(dev_priv->wq,
> 				 &dev_priv->mm.idle_work,
> 				 msecs_to_jiffies(100));
>-
>-	return idle;
> }
> 
> static void
>@@ -2951,17 +2967,21 @@ i915_gem_retire_work_handler(struct work_struct *work)
> 	struct drm_i915_private *dev_priv =
> 		container_of(work, typeof(*dev_priv), mm.retire_work.work);
> 	struct drm_device *dev = dev_priv->dev;
>-	bool idle;
> 
> 	/* Come back later if the device is busy... */
>-	idle = false;
> 	if (mutex_trylock(&dev->struct_mutex)) {
>-		idle = i915_gem_retire_requests(dev);
>+		i915_gem_retire_requests(dev);
> 		mutex_unlock(&dev->struct_mutex);
> 	}
>-	if (!idle) {
>+
>+	/* Keep the retire handler running until we are finally idle.
>+	 * We do not need to do this test under locking as in the worst-case
>+	 * we queue the retire worker once too often.
>+	 */
>+	if (READ_ONCE(dev_priv->mm.busy)) {
> 		i915_queue_hangcheck(dev_priv);
>-		queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work,
>+		queue_delayed_work(dev_priv->wq,
>+				   &dev_priv->mm.retire_work,
> 				   round_jiffies_up_relative(HZ));
> 	}
> }
>@@ -2975,25 +2995,30 @@ i915_gem_idle_work_handler(struct work_struct *work)
> 	struct intel_engine_cs *ring;
> 	int i;
> 
>-	for_each_ring(ring, dev_priv, i)
>-		if (!list_empty(&ring->request_list))
>-			return;
>+	if (!READ_ONCE(dev_priv->mm.busy))
>+		return;
> 
>-	/* we probably should sync with hangcheck here, using cancel_work_sync.
>-	 * Also locking seems to be fubar here, ring->request_list is protected
>-	 * by dev->struct_mutex. */
>+	if (!mutex_trylock(&dev->struct_mutex))
>+		return;
>+
>+	for_each_ring(ring, dev_priv, i) {
>+		if (!list_empty(&ring->request_list))
>+			goto out;
> 
>-	intel_mark_idle(dev);
>+		i915_gem_batch_pool_fini(&ring->batch_pool);
>+	}
> 
>-	if (mutex_trylock(&dev->struct_mutex)) {
>-		struct intel_engine_cs *ring;
>-		int i;
>+	dev_priv->mm.busy = false;
> 
>-		for_each_ring(ring, dev_priv, i)
>-			i915_gem_batch_pool_fini(&ring->batch_pool);
>+	if (INTEL_INFO(dev_priv)->gen >= 6)
>+		gen6_rps_idle(dev_priv);
>+	intel_runtime_pm_put(dev_priv);
>+out:
>+	mutex_unlock(&dev->struct_mutex);
> 
>-		mutex_unlock(&dev->struct_mutex);
>-	}
>+	if (!dev_priv->mm.busy &&
>+	    cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work))
>+		intel_kick_waiters(dev_priv);
> }
> 
> /**
>diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>index e6e8321..9763f30 100644
>--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>@@ -1481,6 +1481,12 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> 		dispatch_flags |= I915_DISPATCH_RS;
> 	}
> 
>+	/* Take a local wakeref for preparing to dispatch the execbuf as
>+	 * we expect to access the hardware fairly frequently in the
>+	 * process. Upon first dispatch, we acquire another prolonged
>+	 * wakeref that we hold until the GPU has been idle for at least
>+	 * 100ms.
>+	 */
> 	intel_runtime_pm_get(dev_priv);
> 
> 	ret = i915_mutex_lock_interruptible(dev);
>diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>index 78f6cd3..cc20213 100644
>--- a/drivers/gpu/drm/i915/i915_irq.c
>+++ b/drivers/gpu/drm/i915/i915_irq.c
>@@ -3065,12 +3065,8 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
> 	if (!i915.enable_hangcheck)
> 		return;
> 
>-	/*
>-	 * The hangcheck work is synced during runtime suspend, we don't
>-	 * require a wakeref. TODO: instead of disabling the asserts make
>-	 * sure that we hold a reference when this work is running.
>-	 */
>-	DISABLE_RPM_WAKEREF_ASSERTS(dev_priv);
>+	if (!READ_ONCE(dev_priv->mm.busy))
>+		return;
> 
> 	/* As enabling the GPU requires fairly extensive mmio access,
> 	 * periodically arm the mmio checker to see if we are triggering
>@@ -3188,17 +3184,12 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
> 		}
> 	}
> 
>-	if (rings_hung) {
>-		i915_handle_error(dev, true, "Ring hung");
>-		goto out;
>-	}
>+	if (rings_hung)
>+		return i915_handle_error(dev, true, "Ring hung");
> 
> 	/* Reset timer in case GPU hangs without another request being added */
> 	if (busy_count)
> 		i915_queue_hangcheck(dev_priv);
>-
>-out:
>-	ENABLE_RPM_WAKEREF_ASSERTS(dev_priv);
> }
> 
> static void ibx_irq_reset(struct drm_device *dev)
>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>index 7d46f02..7be39a66 100644
>--- a/drivers/gpu/drm/i915/intel_display.c
>+++ b/drivers/gpu/drm/i915/intel_display.c
>@@ -10790,35 +10790,6 @@ struct drm_display_mode *intel_crtc_mode_get(struct drm_device *dev,
> 	return mode;
> }
> 
>-void intel_mark_busy(struct drm_device *dev)
>-{
>-	struct drm_i915_private *dev_priv = dev->dev_private;
>-
>-	if (dev_priv->mm.busy)
>-		return;
>-
>-	intel_runtime_pm_get(dev_priv);
>-	i915_update_gfx_val(dev_priv);
>-	if (INTEL_INFO(dev)->gen >= 6)
>-		gen6_rps_busy(dev_priv);
>-	dev_priv->mm.busy = true;
>-}
>-
>-void intel_mark_idle(struct drm_device *dev)
>-{
>-	struct drm_i915_private *dev_priv = dev->dev_private;
>-
>-	if (!dev_priv->mm.busy)
>-		return;
>-
>-	dev_priv->mm.busy = false;
>-
>-	if (INTEL_INFO(dev)->gen >= 6)
>-		gen6_rps_idle(dev->dev_private);
>-
>-	intel_runtime_pm_put(dev_priv);
>-}
>-
> static void intel_crtc_destroy(struct drm_crtc *crtc)
> {
> 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>-- 
>1.9.1
>

Actions: View | Splinter Review

Attachments on bug 94307: 122009

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.