[Intel-gfx] [PATCH 0/8] Detect and deal with Interrupt 'Storms' from noisy Hotplug Lines.

[Daniel Vetter]

On Tue, Jan 22, 2013 at 2:22 PM, Egbert Eich <eich at freedesktop.org> wrote:
> On Thu, Jan 17, 2013 at 03:45:26PM +0100, Daniel Vetter wrote:
>> On Thu, Jan 17, 2013 at 03:01:06PM +0100, Egbert Eich wrote:
>> > 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.
>> >
>
> So far I have not seen anywhere where there's hotplug support for PORT_A.
> PORT_A is marked as an internal connector without any HPD.

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.

>
>> > >
>> > > - 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.
>
> 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.

> 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?

>> 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.

> 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.

> 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.

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.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch