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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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


[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux