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

[Egbert Eich <eich@freedesktop.org>]

On Tue, Jan 22, 2013 at 02:48:29PM +0100, Daniel Vetter wrote:
> On Tue, Jan 22, 2013 at 2:22 PM, Egbert Eich <eich@freedesktop.org> wrote:
> 
> Hm, I've thought the hw supports short dp pulses on eDP port A in case
> the panel needs our attention, but maybe I've mixed that up with the
> dp aux irq for port A. In any case, that's easy to add if we need it.

Yes, of course.
> 
> >
> >
> > For SDVO it is definitely much simpler to track this in the encoder.
> > However in the IRQ handling code we always need to take the detour thru
> > the connector as the drm code expects any hotplog related information in
> > the connector.
> 
> Yeah, once we start filtering irq handling to only the ports affect,
> the current code will get messy. Which is why I think we should move
> the actual hpd handling into encoder->hotplug (and no longer call
> every ->detect callback). But I'm not too sure about it, and it can be
> done later on anyway.

Yeah, this looks easily doable.

> 
> > To get the connector we always have to walk thru the connector list and
> > obtain the associated encoder. Walking thru the encoder list isn't
> > sufficient as there is no easy way from encoder to connector.
> > I don't have a strong preference either way - in the code I'm currently
> > playing around with I keep this information in struct intel_encoder.
> 
> Can't we just loop over all connectors and before we update the
> hotplug state check whether the attached intel encoder is interested
> in the hpd irq we're processing? Or do I miss something here?

No, this is exactly how I've implemented it now.

> 
> >> with POLL and HDP connectors (sometimes on the same one) - SDVO is the
> >> prime example since polling seems to work, but not too reliably. Hence we
> >> need the polling as a backup. To correctly restore those flags I guess we
> >> need a saved_polled variable in intel_connector which we need to restore
> >> when enabling the the hpd line again.
> >
> > I don't see this in the code (drm-intel-testing pulled last Friday).
> > On any connector it is either the DRM_CONNECTOR_POLL_HPD or the
> > DRM_CONNECTOR_POLL_CONNECT (mostly with the DRM_CONNECTOR_POLL_DISCONNECT flag)
> > set but not both.
> > Of course it could be done like you suggest, ie. continue polling despite
> > waiting for interrupts, but this begs the question if we should not resort
> > to polling entirely: the only benefit of doing HDP would be that we would
> > get informed about an output change more quickly.
> 
> I mean that different connectors which use the same hpd line could use
> both polling and non-polling modes since there's only one hpd line per
> sdvo encoder. So we need to be a bit careful with which part of our
> detect code handles which pieces of hw and that we don't lose anything
> when switching modes.

Ah, ok. Got it, will think about it. Thanks!

> 
> > I had sent a patch for EDID caching on the DRM level to Dave last December.
> > I received some comments and suggestions from Ville from Intel which I had
> > worked in - however I have not seen any reaction from Dave, yet.
> 
> Hm, missed that, I'll take a look again.

I had sent it on dri-devel. If you want I can send this to you again.

> 
> > Maybe you want to take a look at this. It cannot cache in all situations
> > where caching would be useful and possible. It still should do a fairly good
> > job of caching EDID extension blocks. This is because it currently
> > lacks any driver interface and thus only can do as much as you can do
> > without a deeper knowledge of what's going on on the hardware level.
> 
> Atm I'm less sure about what we really should do wrt edid caching. One
> ugly problem is boot-up, where a bunch of userspace pieces all want to
> figure out the optimal configuration, but where it's pretty pointless
> to re-query to hw all the time. So I wonder whether we shouldn't have
> temporal caching of even unreliable outputs (e.g. a few seconds), with
> an option for drivers to invalidate the cache on e.g. a hpd interrupt.

Definitely. I don't think it is unreasonable to believe that once we have
a valid EDID that this will remain valid either until we receive an event
from the hardware telling us that something has changed (hotplug event)
or we reprobe and find out that something has definitely changed or
our reprobe timeout has expired.
If there's no valid EDID it is of course reasonable to reprobe at the
earliest convenience: the situation where there's no EDID at all can
be detected very quickly.
EDID readout is a slow undertaking and EDID extension make it even
take longer.
Thus in my code I compare the EDID base block and if this hasn't changed
I use data from the cache. This of course will only work if monitors
are not able to dynamically reconfigure their EDID blocks.

> 
> Another ugly issue is when people disconnect outputs slowly so that we
> get the hpd interrupt _before_ the cable is fully unplugged. We've
> tried to fix those by checking the relevant hpd line state, but on
> noisy lines that won't really work out well (and we already have the
> regression bugs to prove it's a bad idea). So imo that entire
> caching/re-probing topic needs more thought.

Grr. Would it help to simply delay the worker a few tenth of a second so
that this will happen less likely?

Cheers,
	Egbert.