Re: [PATCH 08/24] drm/i915: fix hpd interrupt register locking

[Egbert Eich <eich@suse.com>]

Daniel Vetter writes:
 > Our interrupt handler (in hardird context) could race with the timer
                          Nitpick: s/d/q/

 > (in softirq context), hence we need to hold the spinlock around the
 > call to ->hdp_irq_setup in intel_hpd_irq_handler, too.
 > 
 > But as an optimization (and more so to clarify things) we don't need
 > to do the irqsave/restore dance in the hardirq context.
 > 
 > Note also that on ilk+ the race isn't just against the hotplug
 > reenable timer, but also against the fifo underrun reporting. That one
 > also modifies the SDEIMR register (again protected by the same
 > dev_priv->irq_lock).
 > 
 > To lock things down again sprinkle a assert_spin_locked. But exclude
 > the functions touching SDEIMR for now, I want to extract them all into
 > a new helper function (like we do already for pipestate, display
 > interrupts and all the various gt interrupts).
 > 
 > Cc: Egbert Eich <eich@suse.de>
 > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
 > ---
 >  drivers/gpu/drm/i915/i915_irq.c | 15 ++++++++++-----
 >  1 file changed, 10 insertions(+), 5 deletions(-)
 > 
 > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
 > index 1815891..95e15cd 100644
 > --- a/drivers/gpu/drm/i915/i915_irq.c
 > +++ b/drivers/gpu/drm/i915/i915_irq.c
 > @@ -880,15 +880,13 @@ static inline void intel_hpd_irq_handler(struct drm_device *dev,
 >  					 const u32 *hpd)
 >  {
 >  	drm_i915_private_t *dev_priv = dev->dev_private;
 > -	unsigned long irqflags;
 >  	int i;
 >  	bool storm_detected = false;
 >  
 >  	if (!hotplug_trigger)
 >  		return;
 >  
 > -	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 > -
 > +	spin_lock(&dev_priv->irq_lock);
 >  	for (i = 1; i < HPD_NUM_PINS; i++) {
 >  
 >  		if (!(hpd[i] & hotplug_trigger) ||
 > @@ -911,10 +909,9 @@ static inline void intel_hpd_irq_handler(struct drm_device *dev,
 >  		}
 >  	}
 >  
 > -	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 > -
 >  	if (storm_detected)
 >  		dev_priv->display.hpd_irq_setup(dev);
 > +	spin_unlock(&dev_priv->irq_lock);
 >  
 >  	queue_work(dev_priv->wq,
 >  		   &dev_priv->hotplug_work);
 > @@ -3327,6 +3324,8 @@ static void i915_hpd_irq_setup(struct drm_device *dev)
 >  	struct intel_encoder *intel_encoder;
 >  	u32 hotplug_en;
 >  
 > +	assert_spin_locked(&dev_priv->irq_lock);
 > +
 >  	if (I915_HAS_HOTPLUG(dev)) {
 >  		hotplug_en = I915_READ(PORT_HOTPLUG_EN);
 >  		hotplug_en &= ~HOTPLUG_INT_EN_MASK;


Didn't you want to do the same for ibx_hpd_irq_setup() ?

 > @@ -3610,6 +3609,7 @@ void intel_hpd_init(struct drm_device *dev)
 >  	struct drm_i915_private *dev_priv = dev->dev_private;
 >  	struct drm_mode_config *mode_config = &dev->mode_config;
 >  	struct drm_connector *connector;
 > +	unsigned long irqflags;
 >  	int i;
 >  
 >  	for (i = 1; i < HPD_NUM_PINS; i++) {
 > @@ -3622,6 +3622,11 @@ void intel_hpd_init(struct drm_device *dev)
 >  		if (!connector->polled && I915_HAS_HOTPLUG(dev) && intel_connector->encoder->hpd_pin > HPD_NONE)
 >  			connector->polled = DRM_CONNECTOR_POLL_HPD;
 >  	}
 > +
 > +	/* Interrup setup is already guaranteed to be single-threaded, this is

Nitpick -  missed a 't'.

 > +	 * just to make the assert_spin_locked checks happy. */
 > +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 >  	if (dev_priv->display.hpd_irq_setup)
 >  		dev_priv->display.hpd_irq_setup(dev);
 > +	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 >  }
 > -- 
 > 1.8.1.4