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

[Sharma, Shashank]

Hello Daniel

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. 

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 ?

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.   

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. 

Regards
Shashank 

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

On Mon, Jan 13, 2014 at 12:21:52PM +0530, Ramalingam C wrote:
> On slow HDMI hotplug out, because of the physical interface design, 
> DDC remains active for short duration even when HPD live status is 
> indicating the disconnect state. Because of this on VLV and HSW, slow 
> hotplug out events are not captured.
> 
> Hence this change uses the HPD pins live status to identify the HDMI 
> connector status.
> 
> Multiple forced detect and read modes calls come from user space for 
> different connectors, which can be handled with connector status and 
> previous detect event's cached EDID.
> With this approach for hot pluggable displays, EDID retrieval is 
> required only when there is a real hot-plug events.
> 
> This change also includes:
> 1. A logic to optimize those multiple calls, by re-using
>    cached data from previous detect and get_mode calls.
> 2. Store HDMI EDID, and re-use it in read modes.
> 3. Read live status reg to suppress spurious interrupts
> 
> These two changes will optimize the access to the DDC and also the CPU 
> cycles burned by intel_hdmi_detect and intel_hdmi_get_modes.
> 
> Ramalingam C (1):
>   drm/i915: HDMI detection based on HPD pin live status
> 
> Shashank Sharma (1):
>   drm/i915: Optimize EDID retrival on detect and get_modes
> 
>  drivers/gpu/drm/i915/intel_drv.h  |   12 +++
>  drivers/gpu/drm/i915/intel_hdmi.c |  149 
> ++++++++++++++++++++++++++++++++-----
>  2 files changed, 144 insertions(+), 17 deletions(-)

Nak.

We've had code to use the hpd live status register on all platforms, but had to take it out again on all platforms due to broken hardware: The live status simply doesn't work sometimes, resulting in angry users reporting black screens. So as-is patch 1 can't go into upstream. I know that it'd be really useful if we could rely on the hpd live status but, but thus far I couldn't come up with a good idea to make it work. Git history should have all the details.

Also note that machines with noisy interrupt lines (see the interrupt storm handling code in i915_irq.c) complicate this further.

This means that patch 2 is also a no-go since we really can't rely upon the hpd stuff for hdmi detection. But we can save EDID caching if we automatically invalidate the cached edid after 1-2 seconds or the next hpd interrupt (whichever is first). 1-2 seconds should be long enough for userspace to read the EDID a few times and change the output configuration, but not long enough for users to get pissed. Note that the caching interval should be shorter than the polling interval, which we use as a fallback if the hpd lines are noise and atm is at 10 seconds.

Also this EDID caching and invalidation code should imo be a generic helper with data structures (for the invalidation work and cached edid) in struct intel_output. That way we can also wire it up for e.g. VGA or any other output that'll benefit from EDID caching.

Aside: On DP the hpd pins seem to be reliable thus far, but I'm not sure whether that's just lack of test coverage (since DP screens are less
common) or because it actually works ...

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