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
