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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Sat, 12 Apr 2014, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> On Sat, Apr 12, 2014 at 8:12 PM, Egbert Eich <eich@freedesktop.org> wrote:
>>  > 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;
>>  > +            }
>
>
> We want to skip the hpd handling in any case if the interrupt is
> blocked, otherwise every time we have some unrelated interrupt on gmch
> platforms while a hpd storm is ongoing we'll fire off the hotplug
> work. Which we obviously don't want since that defeats the point of
> the storm handling code.

IMO this skipping the rest of hpd handling is actually the bigger (and
an actual functional) change here, but the commit message only talks
about limiting the WARN. Please amend the commit message.

BR,
Jani.


>
> But we only want to WARN on platforms where we can reliably stop the
> status bits too, hence why I've nested the checks like this.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center