drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt, v2

An HPD interrupt may fire while we are in a function that changes
the PORT_HOTPLUG_EN register - especially when an HPD interrupt
storm occurs.
Since the interrupt handler changes the enabled HPD lines when it
detects such a storm the read-modify-write cycles may interfere.
To avoid this, shiled the rmw cycles with IRQ save spinlocks.

Changes since v1:
- Implement a function which takes care of accessing PORT_HOTPLUG_EN.

Signed-off-by: Egbert Eich <eich@suse.de>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4e8178d34a34256fdacee02822c42ae428bdd287..ba4596e9e7ebbf1ac232aeda46118abdc1a2e4d4 100644 (file)
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2742,6 +2742,9 @@ i915_disable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
 
 void valleyview_enable_display_irqs(struct drm_i915_private *dev_priv);
 void valleyview_disable_display_irqs(struct drm_i915_private *dev_priv);
+void i915_hotplug_interrupt_update(struct drm_i915_private *dev_priv,
+                                  uint32_t mask,
+                                  uint32_t bits);
 void
 ironlake_enable_display_irq(struct drm_i915_private *dev_priv, u32 mask);
 void

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index ca6d806a195ab50a0ea67236d5b218cf31bb840c..6ca6c16fe500638159a55115a2c74b879498daf6 100644 (file)
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -167,6 +167,44 @@ static const u32 hpd_bxt[HPD_NUM_PINS] = {
 
 static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir);
 
+/* For display hotplug interrupt */
+static inline void
+i915_hotplug_interrupt_update_locked(struct drm_i915_private *dev_priv,
+                                    uint32_t mask,
+                                    uint32_t bits)
+{
+       uint32_t val;
+
+       assert_spin_locked(&dev_priv->irq_lock);
+       WARN_ON(bits & ~mask);
+
+       val = I915_READ(PORT_HOTPLUG_EN);
+       val &= ~mask;
+       val |= bits;
+       I915_WRITE(PORT_HOTPLUG_EN, val);
+}
+
+/**
+ * i915_hotplug_interrupt_update - update hotplug interrupt enable
+ * @dev_priv: driver private
+ * @mask: bits to update
+ * @bits: bits to enable
+ * NOTE: the HPD enable bits are modified both inside and outside
+ * of an interrupt context. To avoid that read-modify-write cycles
+ * interfer, these bits are protected by a spinlock. Since this
+ * function is usually not called from a context where the lock is
+ * held already, this function acquires the lock itself. A non-locking
+ * version is also available.
+ */
+void i915_hotplug_interrupt_update(struct drm_i915_private *dev_priv,
+                                  uint32_t mask,
+                                  uint32_t bits)
+{
+       spin_lock_irq(&dev_priv->irq_lock);
+       i915_hotplug_interrupt_update_locked(dev_priv, mask, bits);
+       spin_unlock_irq(&dev_priv->irq_lock);
+}
+
 /**
  * ilk_update_display_irq - update DEIMR
  * @dev_priv: driver private
@@ -3050,7 +3088,7 @@ static void vlv_display_irq_reset(struct drm_i915_private *dev_priv)
 {
        enum pipe pipe;
 
-       I915_WRITE(PORT_HOTPLUG_EN, 0);
+       i915_hotplug_interrupt_update(dev_priv, 0xFFFFFFFF, 0);
        I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 
        for_each_pipe(dev_priv, pipe)
@@ -3466,7 +3504,7 @@ static void vlv_display_irq_postinstall(struct drm_i915_private *dev_priv)
 {
        dev_priv->irq_mask = ~0;
 
-       I915_WRITE(PORT_HOTPLUG_EN, 0);
+       i915_hotplug_interrupt_update(dev_priv, 0xffffffff, 0);
        POSTING_READ(PORT_HOTPLUG_EN);
 
        I915_WRITE(VLV_IIR, 0xffffffff);
@@ -3840,7 +3878,7 @@ static void i915_irq_preinstall(struct drm_device * dev)
        int pipe;
 
        if (I915_HAS_HOTPLUG(dev)) {
-               I915_WRITE(PORT_HOTPLUG_EN, 0);
+               i915_hotplug_interrupt_update(dev_priv, 0xffffffff, 0);
                I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
        }
 
@@ -3874,7 +3912,7 @@ static int i915_irq_postinstall(struct drm_device *dev)
                I915_USER_INTERRUPT;
 
        if (I915_HAS_HOTPLUG(dev)) {
-               I915_WRITE(PORT_HOTPLUG_EN, 0);
+               i915_hotplug_interrupt_update(dev_priv, 0xffffffff, 0);
                POSTING_READ(PORT_HOTPLUG_EN);
 
                /* Enable in IER... */
@@ -4036,7 +4074,7 @@ static void i915_irq_uninstall(struct drm_device * dev)
        int pipe;
 
        if (I915_HAS_HOTPLUG(dev)) {
-               I915_WRITE(PORT_HOTPLUG_EN, 0);
+               i915_hotplug_interrupt_update(dev_priv, 0xffffffff, 0);
                I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
        }
 
@@ -4057,7 +4095,7 @@ static void i965_irq_preinstall(struct drm_device * dev)
        struct drm_i915_private *dev_priv = dev->dev_private;
        int pipe;
 
-       I915_WRITE(PORT_HOTPLUG_EN, 0);
+       i915_hotplug_interrupt_update(dev_priv, 0xffffffff, 0);
        I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 
        I915_WRITE(HWSTAM, 0xeffe);
@@ -4118,7 +4156,7 @@ static int i965_irq_postinstall(struct drm_device *dev)
        I915_WRITE(IER, enable_mask);
        POSTING_READ(IER);
 
-       I915_WRITE(PORT_HOTPLUG_EN, 0);
+       i915_hotplug_interrupt_update(dev_priv, 0xffffffff, 0);
        POSTING_READ(PORT_HOTPLUG_EN);
 
        i915_enable_asle_pipestat(dev);
@@ -4133,22 +4171,22 @@ static void i915_hpd_irq_setup(struct drm_device *dev)
 
        assert_spin_locked(&dev_priv->irq_lock);
 
-       hotplug_en = I915_READ(PORT_HOTPLUG_EN);
-       hotplug_en &= ~HOTPLUG_INT_EN_MASK;
        /* Note HDMI and DP share hotplug bits */
        /* enable bits are the same for all generations */
-       hotplug_en |= intel_hpd_enabled_irqs(dev, hpd_mask_i915);
+       hotplug_en = intel_hpd_enabled_irqs(dev, hpd_mask_i915);
        /* Programming the CRT detection parameters tends
           to generate a spurious hotplug event about three
           seconds later.  So just do it once.
        */
        if (IS_G4X(dev))
                hotplug_en |= CRT_HOTPLUG_ACTIVATION_PERIOD_64;
-       hotplug_en &= ~CRT_HOTPLUG_VOLTAGE_COMPARE_MASK;
        hotplug_en |= CRT_HOTPLUG_VOLTAGE_COMPARE_50;
 
        /* Ignore TV since it's buggy */
-       I915_WRITE(PORT_HOTPLUG_EN, hotplug_en);
+       i915_hotplug_interrupt_update_locked(dev_priv,
+                                     (HOTPLUG_INT_EN_MASK
+                                      | CRT_HOTPLUG_VOLTAGE_COMPARE_MASK),
+                                     hotplug_en);
 }
 
 static irqreturn_t i965_irq_handler(int irq, void *arg)
@@ -4261,7 +4299,7 @@ static void i965_irq_uninstall(struct drm_device * dev)
        if (!dev_priv)
                return;
 
-       I915_WRITE(PORT_HOTPLUG_EN, 0);
+       i915_hotplug_interrupt_update(dev_priv, 0xffffffff, 0);
        I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 
        I915_WRITE(HWSTAM, 0xffffffff);

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index af5e43bef4a41003437a7f1f2979992d065193ac..6ce38e3edf21a9b215e201b637ffb3a3bccdd417 100644 (file)
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -376,7 +376,7 @@ static bool intel_crt_detect_hotplug(struct drm_connector *connector)
 {
        struct drm_device *dev = connector->dev;
        struct drm_i915_private *dev_priv = dev->dev_private;
-       u32 hotplug_en, orig, stat;
+       u32 stat;
        bool ret = false;
        int i, tries = 0;
 
@@ -395,12 +395,12 @@ static bool intel_crt_detect_hotplug(struct drm_connector *connector)
                tries = 2;
        else
                tries = 1;
-       hotplug_en = orig = I915_READ(PORT_HOTPLUG_EN);
-       hotplug_en |= CRT_HOTPLUG_FORCE_DETECT;
 
        for (i = 0; i < tries ; i++) {
                /* turn on the FORCE_DETECT */
-               I915_WRITE(PORT_HOTPLUG_EN, hotplug_en);
+               i915_hotplug_interrupt_update(dev_priv,
+                                             CRT_HOTPLUG_FORCE_DETECT,
+                                             CRT_HOTPLUG_FORCE_DETECT);
                /* wait for FORCE_DETECT to go off */
                if (wait_for((I915_READ(PORT_HOTPLUG_EN) &
                              CRT_HOTPLUG_FORCE_DETECT) == 0,
@@ -415,8 +415,7 @@ static bool intel_crt_detect_hotplug(struct drm_connector *connector)
        /* clear the interrupt we just generated, if any */
        I915_WRITE(PORT_HOTPLUG_STAT, CRT_HOTPLUG_INT_STATUS);
 
-       /* and put the bits back */
-       I915_WRITE(PORT_HOTPLUG_EN, orig);
+       i915_hotplug_interrupt_update(dev_priv, CRT_HOTPLUG_FORCE_DETECT, 0);
 
        return ret;
 }