[Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect and get_modes

[Sharma, Shashank]

Thanks again for this explanation Daniel. 
We will work on your suggestions and come up with a new patch. 

Regards
Shashank  / Ramalingam
-----Original Message-----
From: daniel.vetter at ffwll.ch [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel Vetter
Sent: Monday, January 13, 2014 6:57 PM
To: Sharma, Shashank
Cc: C, Ramalingam; intel-gfx at lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect and get_modes

On Mon, Jan 13, 2014 at 10:39 AM, Sharma, Shashank <shashank.sharma at intel.com> wrote:
> Thanks a lot for your time, for reviewing the changes, and giving us some pointers.
> Both me and Ramalingam are designing this together, and we discussed about these changes and your suggestions.
> There are few things we would like to discuss about. Please correct us if some of our understanding is not proper.

First something I've forgotten in the original mail: Overall your patches look really nice and the commit messages and cover letter have been excellent. Unfortunately you've run into one of the nastier cases of "reality just wont agree with the spec" :(

> Those two patches provide two solution.
> 1. Support for soft HPD, and slow removal of HDMI (when the DDC channel can still get the EDID).
> 2. Try to reduce the EDID reads over DDC channel for get connector and fill mode calls, by caching EDID, and using it until next HPD comes.
>
> Patch 2: Reduce the EDID read over DDC channel We are caching the EDID 
> at every HPD up, on HDMI detect calls, and we are freeing it on subsequent HDMI disconnect calls.
>
> The design philosophy here is, to maintain a state machine of HDMI connector status, and differentiate between IOCTL detect calls and HPD detect calls.
> If there is a detect() or get_modes() call due to any of the IOCTL, which makes sure that input variable force=1, we just use the cached EDID, to serve this calls.
> But if the detect call is coming from HPD work function, due to a HPD plug-out, we remove/invalidate the old cached EDID, and cache the new EDID, on subsequent HDMI plug-in.
> From here, the same state machine follows.
>
> Can you please let us know, why do you think that we should invalidate the cached EDID after 1-2 seconds ?

Because there are machines out there where hpd never happens. So if you keep onto the cached value forever userspace will never notice a change in output configuration. Of course hotplug handling won't work, but at least users can still manually probe outputs. By unconditionally using the cached edid from ioctls you break this use case. Yes, such machines are broken, but we need to keep them working anyway.

Also in my experience all machines are affected, we have examples covering gm45, ilk, snb & ivb. We haven't seen a case for hsw/byt yet since we don't rely on the hpd bits any more (and so won't see bug reports any more).

Generally if you use the hpd stuff your code must be designed under the assumption that hpd is completely unreliably. We've seen anything from random noise, flat-out not-working at all, stuck bits and unstable hpd values that occasionally flip-flop. So you can't rely on it at all.

> Note: In this same patch, there is additional optimization, which you pointed out, where we check if the connector->status is same as live status.
> This can be removed independently, as you suggested.

Hm, where have I pointed this out? Some other mail on internal discussions?

> About patch 1:
> We have done some local experiments and we came to know that for VLV and HSW boards, we can rely on the live status, if we give it some time to settle (~300ms).
> Probably, we need to modify this patch, as you suggested, until it becomes handy to be used reliably. We are on it, and will send another patch soon.
>
> But if somehow we are able to get some consistent results from live status, do you think it would be worth accepting this change, so that it can handle soft HPDs and automation testing.
> Because I believe we will face this problem whenever we are trying to test something from automation, where the physical device is not removed, and DDC channel is up always.

It's very well possible that all the platforms you have, but experience says that some OEM will horrible screw this up. At least they've consistently botched this in the past on occasional machines.

Now the ghost hdmi detection on slow removal is obviously not great, but we can't use the hpd bits to fix this. One approach would be.
1. Upon hpd interrupt do an immediate probe of the connector. This way we'll have good userspace experience if the unplug happens quickly and the hw works.
2. Re-probe with a 1s delay to catch slow-uplugs. The current output probing helpers are clever enough already that if a state-change happens to be detected a uevent will be generate, irrespective of the source of the detect call (i.e. hpd, kernel poll or ioctl/sysfs).

Note that we already track the hpd interrupts on a per-source basis, so doing the re-poll shouldn't be costly. Maybe do the re-poll as part of the EDID invalidation to avoid stalling userspace.

But you can't rely upon the hpd pins unfortunately :(

This way we should be able to implement the 2 features you want, even on unreliable hw.

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