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

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Apr 11, 2013 at 11:32 AM, Jani Nikula
<jani.nikula@linux.intel.com> wrote:
> On Tue, 09 Apr 2013, Egbert Eich <eich@freedesktop.org> wrote:
>> From: Egbert Eich <eich@suse.de>
>>
>> Add a hotplug IRQ storm detection (triggered when a hotplug interrupt
>> fires more than 5 times / sec).
>
> Okay, this is theoretical, but a display port sink could do more than
> that many hpd irq requests when connected. Which leads me to wonder in
> general if the storm detection should be different for hot plug
> vs. unplug and hpd irq events.

Yeah, I've considered what to do with short pulse hotplug events. But
otoh we don't differentiate that yet in our code and worst case we
need to increase the limits a bit maybe. I guess we just need to see
what happens in reality and then act accordingly.

>> Rationale:
>> Despite of the many attempts to fix the problem with noisy hotplug
>> interrupt lines we are still seeing systems which have issues:
>> Once cause of noise seems to be bad routing of the hotplug line
>> on the board: cross talk from other signals seems to cause erronous
>> hotplug interrupts. This has been documented as an erratum for the
>> the i945GM chipset and thus hotplug support was disabled for this
>> chipset model but others seem to have this problem, too.
>>
>> We have seen this issue on a G35 motherboard for example:
>> Even different motherboards of the same model seem to behave
>> differently: while some only see only around 10-100 interrupts/s
>> others seem to see 5k or more.
>> We've also observed a dependency on the selected video mode.
>>
>> Also on certain laptops interrupt noise seems to occur duing
>> battery charging when the battery is at a certain charge levels.
>
> Have you observed difference between hot plug/unplug?
>
> Has this been a problem on PCH split platforms, i.e. since ilk/gen5?

I've discussed this with Art (iirc) and apparently the BIOS/Windows
guys have some elaborate schemes for this on all platforms. So I guess
this could happen everywhere. Note that hpd storms could also be
caused in the sink, so imo we want this everywhere.

>> Thus we add a simple algorithm here that detects an 'interrupt storm'
>> condition.
>>
>> v2: Fixed comment.
>> v3: Reordered drm_i915_private: moved hpd state tracking to hotplug work stuff.
>> v4: Followed by Jesse Barnes to use a time_..() macro.
>>
>> Signed-off-by: Egbert Eich <eich@suse.de>
>> Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h |  9 ++++++
>>  drivers/gpu/drm/i915/i915_irq.c | 66 +++++++++++++++++++++++++++++++++--------
>>  2 files changed, 63 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 44fca0b..83fc1a6 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -929,6 +929,15 @@ typedef struct drm_i915_private {
>>
>>       struct work_struct hotplug_work;
>>       bool enable_hotplug_processing;
>> +     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.

Yeah, agreed. Though to avoid too much rebase hell I think it's better
to do that as a follow-up.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch