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

From: Daniel Vetter <daniel@ffwll.ch>
To: Egbert Eich <eich@suse.de>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 4/4] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt
Date: Wed, 2 Sep 2015 14:06:12 +0200	[thread overview]
Message-ID: <20150902120612.GD1367@phenom.ffwll.local> (raw)
In-Reply-To: <1441138895-23732-5-git-send-email-eich@suse.de>

On Tue, Sep 01, 2015 at 10:21:35PM +0200, Egbert Eich wrote:
> A HPD interrupt may fire during intel_crt_detect_hotplug() - especially
> when HPD interrupt storms occur.
> Since the interrupt handler changes the enabled interrupt lines when it
> detects a storm this races with intel_crt_detect_hotplug().
> To avoid this, shiled the rmw cycles with IRQ save spinlocks.
> 
> Signed-off-by: Egbert Eich <eich@suse.de>

I think this only reduces one source of such races, but fundamentally we
can't avoid them. E.g. if you're _very_ unlucky you might cause a real hpd
right when the strom code frobs around. Plugging the race with this known
interrupt source here doesn't fix the fundamental issue.

Also I think the actual interrupt deliver is fairly asynchronous, both in
the hardware and in the sw handling. E.g. spin_lock_irq does not
synchronize with the interrupt handler on SMP systems, it only guarantees
that it's not running concurrently on the same cpu (which would deadlock).

I think fundamentally this race is unfixable.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_crt.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index af5e43b..685f3de 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -376,9 +376,10 @@ 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 hotplug_en, stat;
>  	bool ret = false;
>  	int i, tries = 0;
> +	unsigned long irqflags;
>  
>  	if (HAS_PCH_SPLIT(dev))
>  		return intel_ironlake_crt_detect_hotplug(connector);
> @@ -395,12 +396,14 @@ 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);
> +		spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> +		hotplug_en = I915_READ(PORT_HOTPLUG_EN);
> +		I915_WRITE(PORT_HOTPLUG_EN, hotplug_en |
> +			   CRT_HOTPLUG_FORCE_DETECT);
> +		spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  		/* wait for FORCE_DETECT to go off */
>  		if (wait_for((I915_READ(PORT_HOTPLUG_EN) &
>  			      CRT_HOTPLUG_FORCE_DETECT) == 0,
> @@ -416,7 +419,11 @@ static bool intel_crt_detect_hotplug(struct drm_connector *connector)
>  	I915_WRITE(PORT_HOTPLUG_STAT, CRT_HOTPLUG_INT_STATUS);
>  
>  	/* and put the bits back */
> -	I915_WRITE(PORT_HOTPLUG_EN, orig);
> +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> +	hotplug_en = I915_READ(PORT_HOTPLUG_EN);
> +	hotplug_en &= ~CRT_HOTPLUG_FORCE_DETECT;
> +	I915_WRITE(PORT_HOTPLUG_EN, hotplug_en);
> +	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  
>  	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