drm/i915: protect force_wake_(get|put) with the gt_lock

Orabug: 15851255

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>
Signed-off-by: Guangyu Sun <guangyu.sun@oracle.com>

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index e36efdc67e1f1c368012475eb50326308964c6f0..5ddf455ea28de65e49a3822b5b544d5a8deb5da6 100644 (file)
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1229,9 +1229,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 ef164432ba6d8ddec3613646fe6a290a4dd3f9cf..fe89759db03116580645f741993e65ac3d88bf1f 100644 (file)
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -2039,6 +2039,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 72a6bf40c6c9ca27722890f43de50469d0100f61..5ffcaa2a675599288fc7e336301b5bf269bf552a 100644 (file)
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -335,11 +335,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)
@@ -359,10 +360,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)
@@ -599,6 +602,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)
@@ -617,8 +621,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 13ad55d57724b7875a8e6cc954c30dd272be9de0..8f33968ddcd55659a9c9c47f849aa6fb62041f3d 100644 (file)
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -278,7 +278,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;
@@ -729,8 +735,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 {