subject:"\[Intel\-gfx\] \[PATCH\] drm\/i915\: Avoid race of intel_crt_detect

Re: [Intel-gfx] [PATCH] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt, v2

2015-09-23 Thread Daniel Vetter

On Wed, Sep 23, 2015 at 04:15:27PM +0200, Egbert Eich wrote:
> 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 

Looks pretty. Queued for -next, thanks for the patch (assuming that we
don't need this for -fixes since there's no bug report linked). Please
correct me so I can drop this and let Jani pick it up instead.
-Daniel
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  3 ++
>  drivers/gpu/drm/i915/i915_irq.c  | 64 
> 
>  drivers/gpu/drm/i915/intel_crt.c | 11 ---
>  3 files changed, 59 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index bf33d6e..a6b7576 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2737,6 +2737,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 a8aa797..ff85eae 100644
> --- 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(_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(_priv->irq_lock);
> + i915_hotplug_interrupt_update_locked(dev_priv, mask, bits);
> + spin_unlock_irq(_priv->irq_lock);
> +}
> +
>  /**
>   * ilk_update_display_irq - update DEIMR
>   * @dev_priv: driver private
> @@ -3074,7 +3112,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, 0x, 0);
>   I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
>  
>   for_each_pipe(dev_priv, pipe)
> @@ -3490,7 +3528,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, 0x, 0);
>   POSTING_READ(PORT_HOTPLUG_EN);
>  
>   I915_WRITE(VLV_IIR, 0x);
> @@ -3864,7 +3902,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, 0x, 0);
>   I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
>   }
>  
> @@ -3898,7 +3936,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, 0x, 0);
>   POSTING_READ(PORT_HOTPLUG_EN);
>  
>   /* Enable in IER... */
> @@ -4060,7 +4098,7 @@ static void i915_irq_uninstall(struct

[Intel-gfx] [PATCH] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt, v2

2015-09-23 Thread Egbert Eich

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 
---
 drivers/gpu/drm/i915/i915_drv.h  |  3 ++
 drivers/gpu/drm/i915/i915_irq.c  | 64 
 drivers/gpu/drm/i915/intel_crt.c | 11 ---
 3 files changed, 59 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bf33d6e..a6b7576 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2737,6 +2737,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 a8aa797..ff85eae 100644
--- 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(_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(_priv->irq_lock);
+   i915_hotplug_interrupt_update_locked(dev_priv, mask, bits);
+   spin_unlock_irq(_priv->irq_lock);
+}
+
 /**
  * ilk_update_display_irq - update DEIMR
  * @dev_priv: driver private
@@ -3074,7 +3112,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, 0x, 0);
I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
 
for_each_pipe(dev_priv, pipe)
@@ -3490,7 +3528,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, 0x, 0);
POSTING_READ(PORT_HOTPLUG_EN);
 
I915_WRITE(VLV_IIR, 0x);
@@ -3864,7 +3902,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, 0x, 0);
I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
}
 
@@ -3898,7 +3936,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, 0x, 0);
POSTING_READ(PORT_HOTPLUG_EN);
 
/* Enable in IER... */
@@ -4060,7 +4098,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, 0x, 0);
I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
}
 
@@ -4081,7 +4119,7 @@ static void i965_irq_preinstall(struct drm_device * dev)
struct drm_i915_private *dev_priv = dev->dev_private;
int pipe;
 
-

Re: [Intel-gfx] [PATCH] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt, v2

2015-09-23 Thread Egbert Eich

Daniel Vetter writes:
 > On Wed, Sep 23, 2015 at 04:15:27PM +0200, Egbert Eich wrote:
 > > 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 
 > 
 > Looks pretty. Queued for -next, thanks for the patch (assuming that we
 > don't need this for -fixes since there's no bug report linked). Please
 > correct me so I can drop this and let Jani pick it up instead.

I didn't bother to file a bug report. I know only one machine that's
affected. 
However the problem this fixes seems to be what caused spurious warnings
which we tried to get rid of with
WARN_ONCE(  INTEL_INFO(dev)->gen >= 5 && !IS_VALLEYVIEW(dev)  ,
  "Received HPD interrupt on pin %d although 
disabled\n", i);

as I did not see these warnings on my gen3 when I removed these tests.

BTW: Using 
 i915_hotplug_interrupt_update(dev_priv, 0x, 0) 
in the *_irq_pre/post/uninstall() functions does not help us much 
in terms of avoiding races. 
It can still happen that an interrupts or reenable worker gets fired 
and resets these values after the spinlock is released in
i915_hotplug_interrupt_update().

IHMO one must 
a. cancel the delayed worker, 
b. disable all interrupt pins and
c. call hpd_irq_setup() 
before calling intel_runtime_pm_disable_interrupts() to avoid this race.

Cheers,
Egbert.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt, v2

[Intel-gfx] [PATCH] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt, v2

Re: [Intel-gfx] [PATCH] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt, v2

3 matches

Site Navigation

Mail list logo

Footer information