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

From: Egbert Eich <eich@suse.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Egbert Eich <eich@suse.de>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: Avoid race of intel_crt_detect_hotplug() with HPD interrupt, v2
Date: Wed, 23 Sep 2015 17:43:55 +0200	[thread overview]
Message-ID: <22018.51387.40148.91302@linux-qknr.fritz.box> (raw)
In-Reply-To: daniel@ffwll.ch wrote on Wednesday, 23 September 2015 at 16:57:17 +0200

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 <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.

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, 0xffffffff, 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