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.
