[Intel-gfx] [PATCH] drm/i915: Don't WARN about unexpected hpd interrupts on gmch platforms

[Egbert Eich]

Daniel Vetter writes:
 > The status bits are unconditionally set, the control bits only enable
 > the actual interrupt generation. Which means if we get some random
 > other interrupts we'll bogusly complain about them.
 > 
 > So restrict the WARN to platforms with a sane hotplug interrupt
 > handling scheme.
 > 
 > This WARN has been introduced in
 > 
 > commit b8f102e8bf71cacf33326360fdf9dcfd1a63925b
 > Author: Egbert Eich <eich at suse.de>
 > Date:   Fri Jul 26 14:14:24 2013 +0200
 > 
 >     drm/i915: Add messages useful for HPD storm detection debugging (v2)
 > 
 > Cc: Egbert Eich <eich at suse.de>
 > Cc: bitlord <bitlord0xff at gmail.com>
 > Reported-by: bitlord <bitlord0xff at gmail.com>
 > Cc: stable at vger.kernel.org
 > Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
 > ---
 >  drivers/gpu/drm/i915/i915_irq.c | 18 ++++++++++++++----
 >  1 file changed, 14 insertions(+), 4 deletions(-)
 > 
 > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
 > index 7753249b3a95..f98ba4e6e70b 100644
 > --- a/drivers/gpu/drm/i915/i915_irq.c
 > +++ b/drivers/gpu/drm/i915/i915_irq.c
 > @@ -1362,10 +1362,20 @@ static inline void intel_hpd_irq_handler(struct drm_device *dev,
 >  	spin_lock(&dev_priv->irq_lock);
 >  	for (i = 1; i < HPD_NUM_PINS; i++) {
 >  
 > -		WARN_ONCE(hpd[i] & hotplug_trigger &&
 > -			  dev_priv->hpd_stats[i].hpd_mark == HPD_DISABLED,
 > -			  "Received HPD interrupt (0x%08x) on pin %d (0x%08x) although disabled\n",
 > -			  hotplug_trigger, i, hpd[i]);
 > +		if (hpd[i] & hotplug_trigger &&
 > +		    dev_priv->hpd_stats[i].hpd_mark == HPD_DISABLED) {
 > +			/*
 > +			 * On GMCH platforms the interrupt mask bits only
 > +			 * prevent irq generation, not the setting of the
 > +			 * hotplug bits itself. So only WARN about unexpected
 > +			 * interrupts on saner platforms.
 > +			 */
 > +			WARN_ONCE(INTEL_INFO(dev)->gen >= 5 && !IS_VALLEYVIEW(dev),
 > +				  "Received HPD interrupt (0x%08x) on pin %d (0x%08x) although disabled\n",
 > +				  hotplug_trigger, i, hpd[i]);

Personally I'd prefer the condition in the WARN..() macro to be the 
unexpected condition you want to warn about. This makes it easier for
anybody not up to speed with the details of hotplug handling to understand
the code. 
Of course the way you structure this avoids a lot of unnecessary tests.
But if this is a concern maybe the entire for loop should be restructured
with 

if (!(hpd[i] & hotplug_trigger))
   	     continue;

right at the beginning.

 > +
 > +			continue;
 > +		}
 >  
 >  		if (!(hpd[i] & hotplug_trigger) ||
 >  		    dev_priv->hpd_stats[i].hpd_mark != HPD_ENABLED)
 > -- 
 > 1.8.5.2

Cheers,
	Egbert.