On Sun, 6 Nov 2011 13:35:22 +0100, Daniel Vetter <[email protected]> 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 <[email protected]> Reviewed-by: Chris Wilson <[email protected]>
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 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
