[PATCH 1/2] drm/i915: Avoid concurrent access when marking the device as idle/busy

[PATCH 1/2] drm/i915: Avoid concurrent access when marking the device as idle/busy

[Chris Wilson]

Whilst marking the device as active is protect by struct_mutex, we would
mark the individual components as not-busy without any locking, leading
to potential confusion and missed powersaving (or even performance
loss). Move the softirq accesses to an atomic and defer the actual
update until we acquire the struct_mutex in the worker.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_dma.c      |    2 +-
 drivers/gpu/drm/i915/i915_drv.h      |    3 +-
 drivers/gpu/drm/i915/intel_display.c |   86 +++++++++++++++-------------------
 drivers/gpu/drm/i915/intel_drv.h     |    1 -
 4 files changed, 42 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index ee25584..ee402c5 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1701,7 +1701,7 @@ bool i915_gpu_busy(void)
 		goto out_unlock;
 	dev_priv = i915_mch_dev;
 
-	ret = dev_priv->busy;
+	ret = !!(dev_priv->busy & (1 << 31));
 
 out_unlock:
 	spin_unlock(&mchdev_lock);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3815d9a..47e5722 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -778,7 +778,8 @@ typedef struct drm_i915_private {
 	int lvds_downclock;
 	struct work_struct idle_work;
 	struct timer_list idle_timer;
-	bool busy;
+	unsigned busy;
+	atomic_t idle;
 	u16 orig_clock;
 	int child_dev_num;
 	struct child_device_config *child_dev;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d5aa2d2..1428e6e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5519,14 +5519,7 @@ static void intel_gpu_idle_timer(unsigned long arg)
 	struct drm_device *dev = (struct drm_device *)arg;
 	drm_i915_private_t *dev_priv = dev->dev_private;
 
-	if (!list_empty(&dev_priv->mm.active_list)) {
-		/* Still processing requests, so just re-arm the timer. */
-		mod_timer(&dev_priv->idle_timer, jiffies +
-			  msecs_to_jiffies(GPU_IDLE_TIMEOUT));
-		return;
-	}
-
-	dev_priv->busy = false;
+	atomic_set_mask(1 << 31, &dev_priv->idle);
 	queue_work(dev_priv->wq, &dev_priv->idle_work);
 }
 
@@ -5535,19 +5528,9 @@ static void intel_gpu_idle_timer(unsigned long arg)
 static void intel_crtc_idle_timer(unsigned long arg)
 {
 	struct intel_crtc *intel_crtc = (struct intel_crtc *)arg;
-	struct drm_crtc *crtc = &intel_crtc->base;
-	drm_i915_private_t *dev_priv = crtc->dev->dev_private;
-	struct intel_framebuffer *intel_fb;
+	drm_i915_private_t *dev_priv = intel_crtc->base.dev->dev_private;
 
-	intel_fb = to_intel_framebuffer(crtc->fb);
-	if (intel_fb && intel_fb->obj->active) {
-		/* The framebuffer is still being accessed by the GPU. */
-		mod_timer(&intel_crtc->idle_timer, jiffies +
-			  msecs_to_jiffies(CRTC_IDLE_TIMEOUT));
-		return;
-	}
-
-	intel_crtc->busy = false;
+	atomic_set_mask(1 << intel_crtc->pipe, &dev_priv->idle);
 	queue_work(dev_priv->wq, &dev_priv->idle_work);
 }
 
@@ -5651,26 +5634,32 @@ static void intel_idle_update(struct work_struct *work)
 						    idle_work);
 	struct drm_device *dev = dev_priv->dev;
 	struct drm_crtc *crtc;
-	struct intel_crtc *intel_crtc;
-
-	if (!i915_powersave)
-		return;
+	unsigned idle;
 
 	mutex_lock(&dev->struct_mutex);
 
-	i915_update_gfx_val(dev_priv);
+	idle = atomic_xchg(&dev_priv->idle, 0);
+	if (idle & (1 << 31))
+		intel_sanitize_pm(dev);
 
+	if (!i915_powersave)
+		goto unlock;
+
+	i915_update_gfx_val(dev_priv);
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		struct intel_crtc *intel_crtc;
+
 		/* Skip inactive CRTCs */
 		if (!crtc->fb)
 			continue;
 
 		intel_crtc = to_intel_crtc(crtc);
-		if (!intel_crtc->busy)
+		if (idle & (1 << intel_crtc->pipe))
 			intel_decrease_pllclock(crtc);
 	}
 
-
+unlock:
+	dev_priv->busy &= ~idle;
 	mutex_unlock(&dev->struct_mutex);
 }
 
@@ -5687,38 +5676,42 @@ static void intel_idle_update(struct work_struct *work)
 void intel_mark_busy(struct drm_device *dev, struct drm_i915_gem_object *obj)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
-	struct drm_crtc *crtc = NULL;
-	struct intel_framebuffer *intel_fb;
-	struct intel_crtc *intel_crtc;
-
-	if (!drm_core_check_feature(dev, DRIVER_MODESET))
-		return;
+	struct drm_crtc *crtc;
+	unsigned busy = 0;
 
-	if (!dev_priv->busy) {
+	if ((dev_priv->busy & (1 << 31)) == 0) {
 		intel_sanitize_pm(dev);
-		dev_priv->busy = true;
-	} else
-		mod_timer(&dev_priv->idle_timer, jiffies +
-			  msecs_to_jiffies(GPU_IDLE_TIMEOUT));
+		busy |= 1 << 31;
+	}
+
+	if (!i915_powersave)
+		goto out;
 
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		struct intel_crtc *intel_crtc;
+
 		if (!crtc->fb)
 			continue;
 
 		intel_crtc = to_intel_crtc(crtc);
-		intel_fb = to_intel_framebuffer(crtc->fb);
-		if (intel_fb->obj == obj) {
-			if (!intel_crtc->busy) {
+		if (to_intel_framebuffer(crtc->fb)->obj == obj) {
+			if ((dev_priv->busy & (1 << intel_crtc->pipe)) == 0) {
 				/* Non-busy -> busy, upclock */
 				intel_increase_pllclock(crtc);
-				intel_crtc->busy = true;
-			} else {
-				/* Busy -> busy, put off timer */
-				mod_timer(&intel_crtc->idle_timer, jiffies +
-					  msecs_to_jiffies(CRTC_IDLE_TIMEOUT));
+				busy |= 1 << intel_crtc->pipe;
 			}
+
+			mod_timer(&intel_crtc->idle_timer, jiffies +
+				  msecs_to_jiffies(CRTC_IDLE_TIMEOUT));
 		}
 	}
+
+out:
+	mod_timer(&dev_priv->idle_timer, jiffies +
+		  msecs_to_jiffies(GPU_IDLE_TIMEOUT));
+
+	atomic_clear_mask(busy, &dev_priv->idle);
+	dev_priv->busy |= busy;
 }
 
 static void intel_crtc_destroy(struct drm_crtc *crtc)
@@ -6311,7 +6304,6 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 
 	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
 
-	intel_crtc->busy = false;
 	setup_timer(&intel_crtc->idle_timer, intel_crtc_idle_timer,
 		    (unsigned long)intel_crtc);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0bd6d8a..64c2e7d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -165,7 +165,6 @@ struct intel_crtc {
 	u8 lut_r[256], lut_g[256], lut_b[256];
 	int dpms_mode;
 	bool active; /* is the crtc on? independent of the dpms mode */
-	bool busy; /* is scanout buffer being updated frequently? */
 	struct timer_list idle_timer;
 	bool lowfreq_avail;
 	struct intel_overlay *overlay;
-- 
1.7.10

[PATCH 2/2] drm/i915: Limit calling mark-busy only for potential scanouts

[Chris Wilson]

The principle of intel_mark_busy() is that we want to spot the
transition of when the display engine is being used in order to bump
powersaving modes and increase display clocks. As such it is only
important when the display is changing, i.e. when rendering to the
scanout or other sprite/plane, and these are characterised by being
pinned.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 451efa3..10bb075 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -979,7 +979,8 @@ i915_gem_execbuffer_move_to_active(struct list_head *objects,
 			obj->pending_gpu_write = true;
 			list_move_tail(&obj->gpu_write_list,
 				       &ring->gpu_write_list);
-			intel_mark_busy(ring->dev, obj);
+			if (obj->pin_count) /* check for potential scanout */
+				intel_mark_busy(ring->dev, obj);
 		}
 
 		trace_i915_gem_object_change_domain(obj, old_read, old_write);
-- 
1.7.10

Re: [PATCH 2/2] drm/i915: Limit calling mark-busy only for potential scanouts

[Daniel Vetter]

On Thu, May 03, 2012 at 12:35:21PM +0100, Chris Wilson wrote:
> The principle of intel_mark_busy() is that we want to spot the
> transition of when the display engine is being used in order to bump
> powersaving modes and increase display clocks. As such it is only
> important when the display is changing, i.e. when rendering to the
> scanout or other sprite/plane, and these are characterised by being
> pinned.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 451efa3..10bb075 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -979,7 +979,8 @@ i915_gem_execbuffer_move_to_active(struct list_head *objects,
>  			obj->pending_gpu_write = true;
>  			list_move_tail(&obj->gpu_write_list,
>  				       &ring->gpu_write_list);
> -			intel_mark_busy(ring->dev, obj);
> +			if (obj->pin_count) /* check for potential scanout */
> +				intel_mark_busy(ring->dev, obj);

intel_mark_busy also controls ips power sharing on ilk, so if we have
loads of batchbuffers not rendering to the a framebuffer, this might hurt
performance. Another thing I've noticed is that we fail to call
intel_mark_busy on pageflips, and I presume running the display at a low
refresh rate instead of the expected high when e.g. displaying videos
could decently confuse userspace.

Care to throw in that fix, too?
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH] drm/i915: Limit calling mark-busy only for potential scanouts

[Chris Wilson]

The principle of intel_mark_busy() is that we want to spot the
transition of when the display engine is being used in order to bump
powersaving modes and increase display clocks. As such it is only
important when the display is changing, i.e. when rendering to the
scanout or other sprite/plane, and these are characterised by being
pinned.

v2: Mark the whole device as busy on execbuffer and pageflips as well
and rebase against dinq for the minor bug fix to be immediately
applicable.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    5 ++++-
 drivers/gpu/drm/i915/intel_display.c       |    4 ++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index a46ed26..60d2d75 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -967,11 +967,14 @@ i915_gem_execbuffer_move_to_active(struct list_head *objects,
 			obj->pending_gpu_write = true;
 			list_move_tail(&obj->gpu_write_list,
 				       &ring->gpu_write_list);
-			intel_mark_busy(ring->dev, obj);
+			if (obj->pin_count) /* check for potential scanout */
+				intel_mark_busy(ring->dev, obj);
 		}
 
 		trace_i915_gem_object_change_domain(obj, old_read, old_write);
 	}
+
+	intel_mark_busy(ring->dev, NULL);
 }
 
 static void
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ddeb794..8a4cf37 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5537,6 +5537,9 @@ void intel_mark_busy(struct drm_device *dev, struct drm_i915_gem_object *obj)
 		mod_timer(&dev_priv->idle_timer, jiffies +
 			  msecs_to_jiffies(GPU_IDLE_TIMEOUT));
 
+	if (obj == NULL)
+		return;
+
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
 		if (!crtc->fb)
 			continue;
@@ -5980,6 +5983,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 		goto cleanup_pending;
 
 	intel_disable_fbc(dev);
+	intel_mark_busy(ring->dev, obj);
 	mutex_unlock(&dev->struct_mutex);
 
 	trace_i915_flip_request(intel_crtc->plane, obj);
-- 
1.7.10

Re: [PATCH] drm/i915: Limit calling mark-busy only for potential scanouts

[Daniel Vetter]

On Thu, May 03, 2012 at 03:47:57PM +0100, Chris Wilson wrote:
> The principle of intel_mark_busy() is that we want to spot the
> transition of when the display engine is being used in order to bump
> powersaving modes and increase display clocks. As such it is only
> important when the display is changing, i.e. when rendering to the
> scanout or other sprite/plane, and these are characterised by being
> pinned.
> 
> v2: Mark the whole device as busy on execbuffer and pageflips as well
> and rebase against dinq for the minor bug fix to be immediately
> applicable.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 1/2] drm/i915: Avoid concurrent access when marking the device as idle/busy

[Daniel Vetter]

On Thu, May 03, 2012 at 12:35:20PM +0100, Chris Wilson wrote:
> Whilst marking the device as active is protect by struct_mutex, we would
> mark the individual components as not-busy without any locking, leading
> to potential confusion and missed powersaving (or even performance
> loss). Move the softirq accesses to an atomic and defer the actual
> update until we acquire the struct_mutex in the worker.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

I'm still confused about the locking, i.e. I think dev_priv->busy and
dev_priv->idle deserve small comments to explain what they're for. The
other thing is that the gpu_idle_timer reinvents our perfectly useable
delayed request retiring code (and in doing so is rather racy). I think we
should kill that and update the gpu busy/idleness in retire_work_handler.
That way the gpu would only be marked idle when it truely is.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_dma.c      |    2 +-
>  drivers/gpu/drm/i915/i915_drv.h      |    3 +-
>  drivers/gpu/drm/i915/intel_display.c |   86 +++++++++++++++-------------------
>  drivers/gpu/drm/i915/intel_drv.h     |    1 -
>  4 files changed, 42 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index ee25584..ee402c5 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1701,7 +1701,7 @@ bool i915_gpu_busy(void)
>  		goto out_unlock;
>  	dev_priv = i915_mch_dev;
>  
> -	ret = dev_priv->busy;
> +	ret = !!(dev_priv->busy & (1 << 31));
>  
>  out_unlock:
>  	spin_unlock(&mchdev_lock);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 3815d9a..47e5722 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -778,7 +778,8 @@ typedef struct drm_i915_private {
>  	int lvds_downclock;
>  	struct work_struct idle_work;
>  	struct timer_list idle_timer;
> -	bool busy;
> +	unsigned busy;
> +	atomic_t idle;
>  	u16 orig_clock;
>  	int child_dev_num;
>  	struct child_device_config *child_dev;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d5aa2d2..1428e6e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5519,14 +5519,7 @@ static void intel_gpu_idle_timer(unsigned long arg)
>  	struct drm_device *dev = (struct drm_device *)arg;
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  
> -	if (!list_empty(&dev_priv->mm.active_list)) {
> -		/* Still processing requests, so just re-arm the timer. */
> -		mod_timer(&dev_priv->idle_timer, jiffies +
> -			  msecs_to_jiffies(GPU_IDLE_TIMEOUT));
> -		return;
> -	}
> -
> -	dev_priv->busy = false;
> +	atomic_set_mask(1 << 31, &dev_priv->idle);
>  	queue_work(dev_priv->wq, &dev_priv->idle_work);
>  }
>  
> @@ -5535,19 +5528,9 @@ static void intel_gpu_idle_timer(unsigned long arg)
>  static void intel_crtc_idle_timer(unsigned long arg)
>  {
>  	struct intel_crtc *intel_crtc = (struct intel_crtc *)arg;
> -	struct drm_crtc *crtc = &intel_crtc->base;
> -	drm_i915_private_t *dev_priv = crtc->dev->dev_private;
> -	struct intel_framebuffer *intel_fb;
> +	drm_i915_private_t *dev_priv = intel_crtc->base.dev->dev_private;
>  
> -	intel_fb = to_intel_framebuffer(crtc->fb);
> -	if (intel_fb && intel_fb->obj->active) {
> -		/* The framebuffer is still being accessed by the GPU. */
> -		mod_timer(&intel_crtc->idle_timer, jiffies +
> -			  msecs_to_jiffies(CRTC_IDLE_TIMEOUT));
> -		return;
> -	}
> -
> -	intel_crtc->busy = false;
> +	atomic_set_mask(1 << intel_crtc->pipe, &dev_priv->idle);
>  	queue_work(dev_priv->wq, &dev_priv->idle_work);
>  }
>  
> @@ -5651,26 +5634,32 @@ static void intel_idle_update(struct work_struct *work)
>  						    idle_work);
>  	struct drm_device *dev = dev_priv->dev;
>  	struct drm_crtc *crtc;
> -	struct intel_crtc *intel_crtc;
> -
> -	if (!i915_powersave)
> -		return;
> +	unsigned idle;
>  
>  	mutex_lock(&dev->struct_mutex);
>  
> -	i915_update_gfx_val(dev_priv);
> +	idle = atomic_xchg(&dev_priv->idle, 0);
> +	if (idle & (1 << 31))
> +		intel_sanitize_pm(dev);
>  
> +	if (!i915_powersave)
> +		goto unlock;
> +
> +	i915_update_gfx_val(dev_priv);
>  	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> +		struct intel_crtc *intel_crtc;
> +
>  		/* Skip inactive CRTCs */
>  		if (!crtc->fb)
>  			continue;
>  
>  		intel_crtc = to_intel_crtc(crtc);
> -		if (!intel_crtc->busy)
> +		if (idle & (1 << intel_crtc->pipe))
>  			intel_decrease_pllclock(crtc);
>  	}
>  
> -
> +unlock:
> +	dev_priv->busy &= ~idle;
>  	mutex_unlock(&dev->struct_mutex);
>  }
>  
> @@ -5687,38 +5676,42 @@ static void intel_idle_update(struct work_struct *work)
>  void intel_mark_busy(struct drm_device *dev, struct drm_i915_gem_object *obj)
>  {
>  	drm_i915_private_t *dev_priv = dev->dev_private;
> -	struct drm_crtc *crtc = NULL;
> -	struct intel_framebuffer *intel_fb;
> -	struct intel_crtc *intel_crtc;
> -
> -	if (!drm_core_check_feature(dev, DRIVER_MODESET))
> -		return;
> +	struct drm_crtc *crtc;
> +	unsigned busy = 0;
>  
> -	if (!dev_priv->busy) {
> +	if ((dev_priv->busy & (1 << 31)) == 0) {
>  		intel_sanitize_pm(dev);
> -		dev_priv->busy = true;
> -	} else
> -		mod_timer(&dev_priv->idle_timer, jiffies +
> -			  msecs_to_jiffies(GPU_IDLE_TIMEOUT));
> +		busy |= 1 << 31;
> +	}
> +
> +	if (!i915_powersave)
> +		goto out;
>  
>  	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> +		struct intel_crtc *intel_crtc;
> +
>  		if (!crtc->fb)
>  			continue;
>  
>  		intel_crtc = to_intel_crtc(crtc);
> -		intel_fb = to_intel_framebuffer(crtc->fb);
> -		if (intel_fb->obj == obj) {
> -			if (!intel_crtc->busy) {
> +		if (to_intel_framebuffer(crtc->fb)->obj == obj) {
> +			if ((dev_priv->busy & (1 << intel_crtc->pipe)) == 0) {
>  				/* Non-busy -> busy, upclock */
>  				intel_increase_pllclock(crtc);
> -				intel_crtc->busy = true;
> -			} else {
> -				/* Busy -> busy, put off timer */
> -				mod_timer(&intel_crtc->idle_timer, jiffies +
> -					  msecs_to_jiffies(CRTC_IDLE_TIMEOUT));
> +				busy |= 1 << intel_crtc->pipe;
>  			}
> +
> +			mod_timer(&intel_crtc->idle_timer, jiffies +
> +				  msecs_to_jiffies(CRTC_IDLE_TIMEOUT));
>  		}
>  	}
> +
> +out:
> +	mod_timer(&dev_priv->idle_timer, jiffies +
> +		  msecs_to_jiffies(GPU_IDLE_TIMEOUT));
> +
> +	atomic_clear_mask(busy, &dev_priv->idle);
> +	dev_priv->busy |= busy;
>  }
>  
>  static void intel_crtc_destroy(struct drm_crtc *crtc)
> @@ -6311,7 +6304,6 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  
>  	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
>  
> -	intel_crtc->busy = false;
>  	setup_timer(&intel_crtc->idle_timer, intel_crtc_idle_timer,
>  		    (unsigned long)intel_crtc);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0bd6d8a..64c2e7d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -165,7 +165,6 @@ struct intel_crtc {
>  	u8 lut_r[256], lut_g[256], lut_b[256];
>  	int dpms_mode;
>  	bool active; /* is the crtc on? independent of the dpms mode */
> -	bool busy; /* is scanout buffer being updated frequently? */
>  	struct timer_list idle_timer;
>  	bool lowfreq_avail;
>  	struct intel_overlay *overlay;
> -- 
> 1.7.10
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH] drm/i915: Avoid concurrent access when marking the device as idle/busy

[Chris Wilson]

As suggested by Daniel, rip out the independent timers for device and
crtc busyness and integrate the manual powermanagement of the display
engine into the GEM core and its request tracking. The benefits are that
the code is a lot smaller, fewer moving parts and should fit more neatly
into the overall activity tracking of the driver.

v2: Complete overhaul and removal of the racy timers and workers.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h            |    3 -
 drivers/gpu/drm/i915/i915_gem.c            |    9 +-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    4 +-
 drivers/gpu/drm/i915/intel_display.c       |  146 +++++-----------------------
 drivers/gpu/drm/i915/intel_drv.h           |    8 +-
 drivers/gpu/drm/i915/intel_pm.c            |    5 +-
 6 files changed, 39 insertions(+), 136 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e03a4f8..9bbaf5c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -770,9 +770,6 @@ typedef struct drm_i915_private {
 	bool lvds_downclock_avail;
 	/* indicates the reduced downclock for LVDS*/
 	int lvds_downclock;
-	struct work_struct idle_work;
-	struct timer_list idle_timer;
-	bool busy;
 	u16 orig_clock;
 	int child_dev_num;
 	struct child_device_config *child_dev;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 44a5f24..d972dbf 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1419,6 +1419,9 @@ i915_gem_object_move_off_active(struct drm_i915_gem_object *obj)
 	list_del_init(&obj->ring_list);
 	obj->last_rendering_seqno = 0;
 	obj->last_fenced_seqno = 0;
+
+	if (obj->pin_count) /* are we a framebuffer? */
+		intel_mark_fb_idle(obj);
 }
 
 static void
@@ -1579,9 +1582,11 @@ i915_add_request(struct intel_ring_buffer *ring,
 				  jiffies +
 				  msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
 		}
-		if (was_empty)
+		if (was_empty) {
 			queue_delayed_work(dev_priv->wq,
 					   &dev_priv->mm.retire_work, HZ);
+			intel_mark_busy(dev_priv->dev);
+		}
 	}
 	return 0;
 }
@@ -1813,6 +1818,8 @@ i915_gem_retire_work_handler(struct work_struct *work)
 
 	if (!dev_priv->mm.suspended && !idle)
 		queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work, HZ);
+	if (idle)
+		intel_mark_idle(dev);
 
 	mutex_unlock(&dev->struct_mutex);
 }
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 21fc11d..989f547 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -968,13 +968,11 @@ i915_gem_execbuffer_move_to_active(struct list_head *objects,
 			list_move_tail(&obj->gpu_write_list,
 				       &ring->gpu_write_list);
 			if (obj->pin_count) /* check for potential scanout */
-				intel_mark_busy(ring->dev, obj);
+				intel_mark_fb_busy(obj);
 		}
 
 		trace_i915_gem_object_change_domain(obj, old_read, old_write);
 	}
-
-	intel_mark_busy(ring->dev, NULL);
 }
 
 static void
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 42b9e20..0a23605 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5384,46 +5384,6 @@ struct drm_display_mode *intel_crtc_mode_get(struct drm_device *dev,
 	return mode;
 }
 
-#define GPU_IDLE_TIMEOUT 500 /* ms */
-
-/* When this timer fires, we've been idle for awhile */
-static void intel_gpu_idle_timer(unsigned long arg)
-{
-	struct drm_device *dev = (struct drm_device *)arg;
-	drm_i915_private_t *dev_priv = dev->dev_private;
-
-	if (!list_empty(&dev_priv->mm.active_list)) {
-		/* Still processing requests, so just re-arm the timer. */
-		mod_timer(&dev_priv->idle_timer, jiffies +
-			  msecs_to_jiffies(GPU_IDLE_TIMEOUT));
-		return;
-	}
-
-	dev_priv->busy = false;
-	queue_work(dev_priv->wq, &dev_priv->idle_work);
-}
-
-#define CRTC_IDLE_TIMEOUT 1000 /* ms */
-
-static void intel_crtc_idle_timer(unsigned long arg)
-{
-	struct intel_crtc *intel_crtc = (struct intel_crtc *)arg;
-	struct drm_crtc *crtc = &intel_crtc->base;
-	drm_i915_private_t *dev_priv = crtc->dev->dev_private;
-	struct intel_framebuffer *intel_fb;
-
-	intel_fb = to_intel_framebuffer(crtc->fb);
-	if (intel_fb && intel_fb->obj->active) {
-		/* The framebuffer is still being accessed by the GPU. */
-		mod_timer(&intel_crtc->idle_timer, jiffies +
-			  msecs_to_jiffies(CRTC_IDLE_TIMEOUT));
-		return;
-	}
-
-	intel_crtc->busy = false;
-	queue_work(dev_priv->wq, &dev_priv->idle_work);
-}
-
 static void intel_increase_pllclock(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -5453,10 +5413,6 @@ static void intel_increase_pllclock(struct drm_crtc *crtc)
 		if (dpll & DISPLAY_RATE_SELECT_FPA1)
 			DRM_DEBUG_DRIVER("failed to upclock LVDS!\n");
 	}
-
-	/* Schedule downclock */
-	mod_timer(&intel_crtc->idle_timer, jiffies +
-		  msecs_to_jiffies(CRTC_IDLE_TIMEOUT));
 }
 
 static void intel_decrease_pllclock(struct drm_crtc *crtc)
@@ -5495,89 +5451,48 @@ static void intel_decrease_pllclock(struct drm_crtc *crtc)
 
 }
 
-/**
- * intel_idle_update - adjust clocks for idleness
- * @work: work struct
- *
- * Either the GPU or display (or both) went idle.  Check the busy status
- * here and adjust the CRTC and GPU clocks as necessary.
- */
-static void intel_idle_update(struct work_struct *work)
+void intel_mark_busy(struct drm_device *dev)
+{
+	intel_sanitize_pm(dev);
+	i915_update_gfx_val(dev->dev_private);
+}
+
+void intel_mark_idle(struct drm_device *dev)
 {
-	drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t,
-						    idle_work);
-	struct drm_device *dev = dev_priv->dev;
+	intel_sanitize_pm(dev);
+}
+
+void intel_mark_fb_busy(struct drm_i915_gem_object *obj)
+{
+	struct drm_device *dev = obj->base.dev;
 	struct drm_crtc *crtc;
-	struct intel_crtc *intel_crtc;
 
 	if (!i915_powersave)
 		return;
 
-	mutex_lock(&dev->struct_mutex);
-
-	i915_update_gfx_val(dev_priv);
-
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
-		/* Skip inactive CRTCs */
 		if (!crtc->fb)
 			continue;
 
-		intel_crtc = to_intel_crtc(crtc);
-		if (!intel_crtc->busy)
-			intel_decrease_pllclock(crtc);
+		if (to_intel_framebuffer(crtc->fb)->obj == obj)
+			intel_increase_pllclock(crtc);
 	}
-
-
-	mutex_unlock(&dev->struct_mutex);
 }
 
-/**
- * intel_mark_busy - mark the GPU and possibly the display busy
- * @dev: drm device
- * @obj: object we're operating on
- *
- * Callers can use this function to indicate that the GPU is busy processing
- * commands.  If @obj matches one of the CRTC objects (i.e. it's a scanout
- * buffer), we'll also mark the display as busy, so we know to increase its
- * clock frequency.
- */
-void intel_mark_busy(struct drm_device *dev, struct drm_i915_gem_object *obj)
+void intel_mark_fb_idle(struct drm_i915_gem_object *obj)
 {
-	drm_i915_private_t *dev_priv = dev->dev_private;
-	struct drm_crtc *crtc = NULL;
-	struct intel_framebuffer *intel_fb;
-	struct intel_crtc *intel_crtc;
-
-	if (!drm_core_check_feature(dev, DRIVER_MODESET))
-		return;
-
-	if (!dev_priv->busy) {
-		intel_sanitize_pm(dev);
-		dev_priv->busy = true;
-	} else
-		mod_timer(&dev_priv->idle_timer, jiffies +
-			  msecs_to_jiffies(GPU_IDLE_TIMEOUT));
+	struct drm_device *dev = obj->base.dev;
+	struct drm_crtc *crtc;
 
-	if (obj == NULL)
+	if (!i915_powersave)
 		return;
 
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
 		if (!crtc->fb)
 			continue;
 
-		intel_crtc = to_intel_crtc(crtc);
-		intel_fb = to_intel_framebuffer(crtc->fb);
-		if (intel_fb->obj == obj) {
-			if (!intel_crtc->busy) {
-				/* Non-busy -> busy, upclock */
-				intel_increase_pllclock(crtc);
-				intel_crtc->busy = true;
-			} else {
-				/* Busy -> busy, put off timer */
-				mod_timer(&intel_crtc->idle_timer, jiffies +
-					  msecs_to_jiffies(CRTC_IDLE_TIMEOUT));
-			}
-		}
+		if (to_intel_framebuffer(crtc->fb)->obj == obj)
+			intel_decrease_pllclock(crtc);
 	}
 }
 
@@ -6010,7 +5925,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 		goto cleanup_pending;
 
 	intel_disable_fbc(dev);
-	intel_mark_busy(dev, obj);
+	intel_mark_fb_busy(obj);
 	mutex_unlock(&dev->struct_mutex);
 
 	trace_i915_flip_request(intel_crtc->plane, obj);
@@ -6174,11 +6089,6 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	}
 
 	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
-
-	intel_crtc->busy = false;
-
-	setup_timer(&intel_crtc->idle_timer, intel_crtc_idle_timer,
-		    (unsigned long)intel_crtc);
 }
 
 int intel_get_pipe_from_crtc_id(struct drm_device *dev, void *data,
@@ -6719,10 +6629,6 @@ void intel_modeset_init(struct drm_device *dev)
 	/* Just disable it once at startup */
 	i915_disable_vga(dev);
 	intel_setup_outputs(dev);
-
-	INIT_WORK(&dev_priv->idle_work, intel_idle_update);
-	setup_timer(&dev_priv->idle_timer, intel_gpu_idle_timer,
-		    (unsigned long)dev);
 }
 
 void intel_modeset_gem_init(struct drm_device *dev)
@@ -6777,14 +6683,6 @@ void intel_modeset_cleanup(struct drm_device *dev)
 	/* flush any delayed tasks or pending work */
 	flush_scheduled_work();
 
-	/* Shut off idle work before the crtcs get freed. */
-	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
-		intel_crtc = to_intel_crtc(crtc);
-		del_timer_sync(&intel_crtc->idle_timer);
-	}
-	del_timer_sync(&dev_priv->idle_timer);
-	cancel_work_sync(&dev_priv->idle_work);
-
 	drm_mode_config_cleanup(dev);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e5ee166..beb22c7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -169,8 +169,6 @@ struct intel_crtc {
 	u8 lut_r[256], lut_g[256], lut_b[256];
 	int dpms_mode;
 	bool active; /* is the crtc on? independent of the dpms mode */
-	bool busy; /* is scanout buffer being updated frequently? */
-	struct timer_list idle_timer;
 	bool lowfreq_avail;
 	struct intel_overlay *overlay;
 	struct intel_unpin_work *unpin_work;
@@ -334,8 +332,10 @@ extern bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg,
 			    bool is_sdvob);
 extern void intel_dvo_init(struct drm_device *dev);
 extern void intel_tv_init(struct drm_device *dev);
-extern void intel_mark_busy(struct drm_device *dev,
-			    struct drm_i915_gem_object *obj);
+extern void intel_mark_busy(struct drm_device *dev);
+extern void intel_mark_idle(struct drm_device *dev);
+extern void intel_mark_fb_busy(struct drm_i915_gem_object *obj);
+extern void intel_mark_fb_idle(struct drm_i915_gem_object *obj);
 extern bool intel_lvds_init(struct drm_device *dev);
 extern void intel_dp_init(struct drm_device *dev, int dp_reg);
 void
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 4389234..8bbfb75 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3014,13 +3014,16 @@ bool i915_gpu_busy(void)
 {
 	struct drm_i915_private *dev_priv;
 	bool ret = false;
+	int n;
 
 	spin_lock(&mchdev_lock);
 	if (!i915_mch_dev)
 		goto out_unlock;
 	dev_priv = i915_mch_dev;
 
-	ret = dev_priv->busy;
+	for (n = 0; n < I915_NUM_RINGS; n++)
+		if (dev_priv->ring[n].obj)
+			ret |= !list_empty(&dev_priv->ring[n].request_list);
 
 out_unlock:
 	spin_unlock(&mchdev_lock);
-- 
1.7.10