Re: [PATCH 0/8] Detect and deal with Interrupt 'Storms' from noisy Hotplug Lines.

[Egbert Eich <eich@freedesktop.org>]

Hi Daniel,

On Fri, Jan 11, 2013 at 09:34:08PM +0100, Daniel Vetter wrote:
> 
> Nice work, and we know that we need this since quite a while. But
> unfortunately we've not yet come around to implement something. Some
> high-level comments on how I think this should best be handled:
> 
> - imo dv_priv->hotplug_supported_mask should die - it leaks platform
>   specific irq magic from i915_irq.c into every connector/encoder. And we
>   have had the bugs and confusions to prove that it's not a good idea. I
>   think it'd be better if we add a new HOTPLUG_PIN_FOO enum that encoders
>   register interest in, and the platform code in i915_irq.c then maps
>   from/to that. On a quick check we have hotplug pins for CRT, TV,
>   SDVO_B&C and PORT_A-D (for DP&HDMI).

I thought along the same lines, I just didn't want to go quite as far.
Therefore I added functions in i915_irq.c to set these depending on the
connector.

> 
>   Also note that on PCH_SPLIT platforms port A is not in the same
>   register, further platforms will make an even cuter mess of this ...

Ok, I will look into that.

> 
> - I think the the hpd pin should be track in the encoder, not in the
>   connector. The only encoders where there's not a 1:1 relationship (sdvo
>   and ddi on hsw) want it there. Also, we already have the ->hot_plug
>   callback in the encoder, which will be useful for later extensions.
> 
> - Since some encoders share the same hpd pin (HDMI&DP on pre-hsw) I think
>   we should keep the noise statistic data in the device's dev_priv
>   somewhere in an array, with one set for each hpd pin from the enum above.

This would also be an option. I did notice that these pins are shared, it
didn't cause any issues as always both connectors got flagged simultaniously.
On the other hand calling the same disable/enable twice when traversing the
connector list is sorta ugly.

> 
> - In 3.8 the drm hpd/polling helpers are much improved and don't randomly
>   poll everything any more. So if a hpd connector isn't marked as
>   OUTPUT_POLL, it wont ever get polled. Which means if you disable the hpd
>   irq for it, we need to have our own poll work to do that for us. The
>   long-term goal I have is to pimp the encoder->hot_plug callback also for
>   this case, to avoid re-running the connector detect code on unrelated
>   outputs (which can sometimes cause havoc).

I do change the state of the 'polled' member when I disable/reenable hotplug
interrupts already. This part therefore should work fine already.

> 
>   Eventually a want a hpd interrupt to only run the ->hot_plug callbacks
>   on encoders which are interested in that signal, hence this slight
>   overkill ... Ofc, that requires that we move a lot of the ->detect logic

This was exactly my question: we have all information at hand now to do this
and I can easily add this. The downside is that if the information about
the mapping is not accurate (ie if a vendor routes HPD lines differently)
this connector will never light up :(
As it is now since we poll everything when an interrupt happened we can
be sure that we catch all connectors even if the mapping in our tables
don't reflect what's wired on the board.
I didn't just go ahead and implement this yet as I've gotten too pessimistic.

>   into ->hot_plug, but that's the only way to do sane EDID cache and
>   similar things on outputs where hpd should work (DP/HDMI).

... But since you suggest this I will gladly add this :)

> 
> - The math buff in me would like hpd stroms to gracefully degrade into
>   polling at 10s or so. We could achieve that with irq source masking and
>   scheduling the work item to do the hotplug handling with an (increasing)
>   delay if there's too many interrupts from a given hpd pin. But that
>   requires that we can mask hotplug interrupts properly, which seems to be
>   impossible with the PORT_HOTPLUG regs on gmch/SoC platforms :( So I
>   think your logic is nice enough ;-)

What you suggest would be possible with some small changes to my code I
guess. I just fear if we do have an IRQ storm 10s would be too short - on
a completely idle system this might be the prime source of wakeups.

I believe I can spare some hours to think about and work in your 
suggestions.

Cheers,
	Egbert.