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 at suse.de>
> Signed-off-by: Daniel Vetter <daniel.vetter at 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
