Re: [PATCH v3 1/7] drm/i915: Add HPD IRQ storm detection (v4)

[Egbert Eich <eich@freedesktop.org>]

Hi Jani,

I've rebased and regenerated all the patches now, as there 
were some other bikesheds i had not adressed. I've also
included all Reviewed-By: 
This should make it easier to integrate the patches.

Some comments below:


On Thu, Apr 11, 2013 at 12:32:29PM +0300, Jani Nikula wrote:
> > +	struct {
> > +		unsigned long hpd_last_jiffies;
> > +		int hpd_cnt;
> > +		enum {
> > +			HPD_ENABLED = 0,
> > +			HPD_DISABLED = 1,
> > +			HPD_MARK_DISABLED = 2
> > +		} hpd_mark;
> > +	} hpd_stats[HPD_NUM_PINS];
> 
> With all the hotplug stuff being added, I think it's getting time to
> group all the hotplug stuff under a struct.

I will postpone this until I address the issues that I have on my TODO.

> 
> >  	int num_pch_pll;
> >  	int num_plane;
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 4c5bdd0..32b5527 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -582,6 +582,37 @@ static void gen6_queue_rps_work(struct drm_i915_private *dev_priv,
> >  	queue_work(dev_priv->wq, &dev_priv->rps.work);
> >  }
> >  
> > +#define HPD_STORM_DETECT_PERIOD 1000
> > +
> > +static inline void hotplug_irq_storm_detect(struct drm_device *dev,
> > +					    u32 hotplug_trigger,
> > +					    const u32 *hpd)
> 
> I think you should just add the bool return status right here instead of
> postponing until the later patch that needs it.

I thought about this and left it as it is: Returning a bool status now
will raise other questions as the logic in this patch doesn't require it.
I'd rather have a bigger patch later which will however clearly explains
the reason for the change to the function.

> 
> > +{
> > +	drm_i915_private_t *dev_priv = dev->dev_private;
> > +	unsigned long irqflags;
> > +	int i;
> > +
> > +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> > +
> > +	for (i = 1; i < HPD_NUM_PINS; i++) {
> > +		if ((hpd[i] & hotplug_trigger) &&
> > +		    dev_priv->hpd_stats[i].hpd_mark == HPD_ENABLED) {
> 
> Bikeshed: You might get a nicer layout below by negating that if and
> adding continue.

Fixed.

> 
> > +			if (!time_in_range(jiffies, dev_priv->hpd_stats[i].hpd_last_jiffies,
> > +					  dev_priv->hpd_stats[i].hpd_last_jiffies
> > +					  + msecs_to_jiffies(HPD_STORM_DETECT_PERIOD))) {
> > +				dev_priv->hpd_stats[i].hpd_last_jiffies = jiffies;
> > +				dev_priv->hpd_stats[i].hpd_cnt = 0;
> > +			} else if (dev_priv->hpd_stats[i].hpd_cnt > 5) {
> > +				dev_priv->hpd_stats[i].hpd_mark = HPD_MARK_DISABLED;
> > +				DRM_DEBUG_KMS("HPD interrupt storm detected on  PIN %d\n", i);
> 
> Extra space before "PIN".

Fixed.
> 
> > +			} else
> > +				dev_priv->hpd_stats[i].hpd_cnt++;
> 
> If one branch requires braces, then all do.

Fixed.

Cheers,
	Egbert.