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>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
Signed-off-by: Keith Packard <keithp@keithp.com>

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index f8b8ed22b4dc42f2218b3c2780232e141bf1f131..a017b989b1ab744ed9ebe05096ffc8a44fec5bfb 100644 (file)
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1398,9 +1398,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 = %u\n", forcewake_count);
 
        return 0;
 }

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 5f4d5893e98356ff8b657a3a3048858d8ba15fab..ddfe3d902b2a3a5d908b0c7664a4348f94ce3988 100644 (file)
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -2045,6 +2045,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 46c36f5cafb14164af4dc52bde4634db5fc77bef..bdf6a1b3622353360e1ad59f5d53097c53104c8a 100644 (file)
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -368,11 +368,12 @@ void __gen6_gt_force_wake_mt_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)
                dev_priv->display.force_wake_get(dev_priv);
+       spin_unlock_irqrestore(&dev_priv->gt_lock, irqflags);
 }
 
 void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
@@ -392,10 +393,12 @@ void __gen6_gt_force_wake_mt_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)
                dev_priv->display.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)
@@ -626,6 +629,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)
@@ -644,8 +648,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)
                        dev_priv->display.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 602bc80baabb982814dcce3640e13b53a638f09e..9689ca38b2b333f26c75e95421e773b3331bd81c 100644 (file)
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -288,7 +288,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;
@@ -741,8 +747,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 {