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

["Nicolas Kalkhof" <nkalkhof@web.de>]

Hi Daniel,

is there a specific git branch availble where I can test your patch? I'm also experiencing random forcewake hangs and would like to see if your patch helps.

Thanks
Nic


-----Ursprüngliche Nachricht-----
Von: "Daniel Vetter" <daniel.vetter@ffwll.ch>
Gesendet: Nov 6, 2011 1:35:22 PM
An: intel-gfx <intel-gfx@lists.freedesktop.org>
Betreff: [Intel-gfx] [PATCH] drm/i915: protect force_wake_(get|put) with the gt_lock

>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>
>---
> 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);
>
> return 0;
> }
>diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
>index 2eac955..ab3a3fd 100644
>--- a/drivers/gpu/drm/i915/i915_dma.c
>+++ b/drivers/gpu/drm/i915/i915_dma.c
>@@ -2031,6 +2031,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> if (!IS_I945G(dev) && !IS_I945GM(dev))
> pci_enable_msi(dev->pdev);
>
>+ spin_lock_init(&dev_priv->gt_lock);
> spin_lock_init(&dev_priv->irq_lock);
> spin_lock_init(&dev_priv->error_lock);
> spin_lock_init(&dev_priv->rps_lock);
>diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>index 548e04b..bab4e08 100644
>--- a/drivers/gpu/drm/i915/i915_drv.c
>+++ b/drivers/gpu/drm/i915/i915_drv.c
>@@ -351,11 +351,12 @@ static void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
> */
> void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
> {
>- WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
>+ unsigned long irqflags;
>
>- /* Forcewake is atomic in case we get in here without the lock */
>- if (atomic_add_return(1, &dev_priv->forcewake_count) == 1)
>+ spin_lock_irqsave(&dev_priv->gt_lock, irqflags);
>+ if (dev_priv->forcewake_count++ == 0)
> __gen6_gt_force_wake_get(dev_priv);
>+ spin_unlock_irqrestore(&dev_priv->gt_lock, irqflags);
> }
>
> static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
>@@ -369,10 +370,13 @@ static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
> */
> void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
> {
>- WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
>+ unsigned long irqflags;
>
>- if (atomic_dec_and_test(&dev_priv->forcewake_count))
>+ spin_lock_irqsave(&dev_priv->gt_lock, irqflags);
>+
>+ if (--dev_priv->forcewake_count == 0)
> __gen6_gt_force_wake_put(dev_priv);
>+ spin_unlock_irqrestore(&dev_priv->gt_lock, irqflags);
> }
>
> void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
>@@ -603,6 +607,7 @@ int i915_reset(struct drm_device *dev, u8 flags)
> * need to
> */
> bool need_display = true;
>+ unsigned long irqflags;
> int ret;
>
> if (!i915_try_reset)
>@@ -621,8 +626,10 @@ int i915_reset(struct drm_device *dev, u8 flags)
> case 6:
> ret = gen6_do_reset(dev, flags);
> /* If reset with a user forcewake, try to restore */
>- if (atomic_read(&dev_priv->forcewake_count))
>+ spin_lock_irqsave(&dev_priv->gt_lock, irqflags);
>+ if (dev_priv->forcewake_count)
> __gen6_gt_force_wake_get(dev_priv);
>+ spin_unlock_irqrestore(&dev_priv->gt_lock, irqflags);
> break;
> case 5:
> ret = ironlake_do_reset(dev, flags);
>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>index bd98fb3..25036f5 100644
>--- a/drivers/gpu/drm/i915/i915_drv.h
>+++ b/drivers/gpu/drm/i915/i915_drv.h
>@@ -276,7 +276,13 @@ typedef struct drm_i915_private {
> int relative_constants_mode;
>
> void __iomem *regs;
>- u32 gt_fifo_count;
>+ /** gt_fifo_count and the subsequent register write are synchronized
>+ * with dev->struct_mutex. */
>+ unsigned gt_fifo_count;
>+ /** forcewake_count is protected by gt_lock */
>+ unsigned forcewake_count;
>+ /** gt_lock is also taken in irq contexts. */
>+ struct spinlock gt_lock;
>
> struct intel_gmbus {
> struct i2c_adapter adapter;
>@@ -725,8 +731,6 @@ typedef struct drm_i915_private {
>
> struct drm_property *broadcast_rgb_property;
> struct drm_property *force_audio_property;
>-
>- atomic_t forcewake_count;
> } drm_i915_private_t;
>
> enum i915_cache_level {
>--
>1.7.6.4
>
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx@lists.freedesktop.org
>http://lists.freedesktop.org/mailman/listinfo/intel-gfx


___________________________________________________________
SMS schreiben mit WEB.DE FreeMail - einfach, schnell und
kostenguenstig. Jetzt gleich testen! http://f.web.de/?mc=021192
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx