Re: [PATCH] drm/i915: protect force_wake_(get|put) with the gt_lock

[Chris Wilson <chris@chris-wilson.co.uk>]

On Sun,  6 Nov 2011 13:35:22 +0100, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> The problem this patch solves is that the forcewake accounting
> necessary for register reads is protected by dev->struct_mutex. But the
> hangcheck and error_capture code need to access registers without
> grabbing this mutex because we hold it while waiting for the gpu.
> So a new lock is required. Because currently the error_state capture
> is called from the error irq handler and the hangcheck code runs from
> a timer, it needs to be an irqsafe spinlock (note that the registers
> used by the irq handler (neglecting the error handling part) only uses
> registers that don't need the forcewake dance).
> 
> We could tune this down to a normal spinlock when we rework the
> error_state capture and hangcheck code to run from a workqueue.  But
> we don't have any read in a fastpath that needs forcewake, so I've
> decided to not care much about overhead.
> 
> This prevents tests/gem_hangcheck_forcewake from i-g-t from killing my
> snb on recent kernels - something must have slightly changed the
> timings. On previous kernels it only trigger a WARN about the broken
> locking.
> 
> v2: Drop the previous patch for the register writes.
> 
> v3: Improve the commit message per Chris Wilson's suggestions.
> 
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

One minor oddity left ;-)

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |    8 ++++++--
>  drivers/gpu/drm/i915/i915_dma.c     |    1 +
>  drivers/gpu/drm/i915/i915_drv.c     |   19 +++++++++++++------
>  drivers/gpu/drm/i915/i915_drv.h     |   10 +++++++---
>  4 files changed, 27 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 5ba63ad..51b21eb 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1320,9 +1320,13 @@ static int i915_gen6_forcewake_count_info(struct seq_file *m, void *data)
>  	struct drm_info_node *node = (struct drm_info_node *) m->private;
>  	struct drm_device *dev = node->minor->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	unsigned forcewake_count;
>  
> -	seq_printf(m, "forcewake count = %d\n",
> -		   atomic_read(&dev_priv->forcewake_count));
> +	spin_lock_irq(&dev_priv->gt_lock);
> +	forcewake_count = dev_priv->forcewake_count;
> +	spin_unlock_irq(&dev_priv->gt_lock);
> +
> +	seq_printf(m, "forcewake count = %d\n", forcewake_count);

Is it signed or unsigned? Who cares? ;-)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre