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

[Daniel Vetter <daniel@ffwll.ch>]

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 <eich@suse.de>

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(&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
> @@ -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, 0xFFFFFFFF, 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, 0xffffffff, 0);
>  	POSTING_READ(PORT_HOTPLUG_EN);
>  
>  	I915_WRITE(VLV_IIR, 0xffffffff);
> @@ -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, 0xffffffff, 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, 0xffffffff, 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, 0xffffffff, 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;
>  
> -	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);
> @@ -4142,7 +4180,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);
> @@ -4157,22 +4195,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)
> @@ -4285,7 +4323,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 af5e43b..6ce38e3 100644
> --- 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;
>  }
> -- 
> 1.8.4.5
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx