Re: [PATCH 0/2] Optimization on intel HDMI detect and get_modes

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Apr 10, 2014 at 06:46:58AM +0000, Sharma, Shashank wrote:
> Hi Daniel / Quanxian / All, 
> 
> I have one question about the 'force' flag given to connector's detect() functions.
> To design new EDID caching solution, I was trying to re-use this flag.
> As you all know, detect() gets called from few places in DRM and I915
> layer, with this flag status:
> 
> drm_sysfs.c : status_show (sysfs status check inquire from USP)						force = 1
> drm_crtc_helper.c: drm_helper_probe_single_connector_modes (part of get_connector IOCTL) 		force = 1
> 
> drm_crtc_helper.c: output_poll_execute(scheduled from poll work)						force = 0
> drm_crtc_helper.c: drm_helper_hpd_irq_event (resume hotplug)						force = 0
> i915_irq.c: i915_hotplug_work_function (bottom half of hotplug IRQ)						force = 0
> 
> What I am seeing is, the places where it's really required to probe the
> device, like in IRQ handlers or while resuming the device,  the force
> flag is 0, whereas whenever there is a userspace interaction or query
> for status, the flag is 1.  Please correct me if my understating is not
> proper, 
> but I feel this should be the opposite way. 
> 
> Please let me know your opinion about this.

Force is a bit misnomer, a better name might be non-invasive. Without
force we don't do stuff like load-detect by default since at least when
doing this manually some old crt screens switch on. But this is really
only relevant for gen2/3 and tv-out, so not of any concern on modern
platforms. Essentially force means "userspace asked for this and it's ok
if the screen flickers a bit due to that".

Imo you can do the caching and use the cached version irrespective of
force.
-Daniel

> 
> Regards
> Shashank  
> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Sharma, Shashank
> Sent: Wednesday, April 09, 2014 12:20 PM
> To: Wang, Quanxian; Daniel Vetter
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect and get_modes
> 
> Hello Quanxian Wang
> 
> This patch is available and working on all MCG tree's (Main, R42B and R44B) We were trying to opensource this patch, but due to the dependency on live_status reg, we had to change the design. 
> 
> I was working on that, but couldn't finish the activity yet, Thanks for reminding me I will update soon. :)
> 
> Regards
> Shashank   
> -----Original Message-----
> From: Wang, Quanxian
> Sent: Wednesday, April 09, 2014 11:49 AM
> To: Sharma, Shashank; Daniel Vetter
> Cc: intel-gfx@lists.freedesktop.org
> Subject: RE: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect and get_modes
> 
> Hi, Sharma, Shashank
> 
> Is there any following patches to make it happen?
> 
> Thanks
> 
> Regards
> 
> Quanxian Wang
> 
> >-----Original Message-----
> >From: intel-gfx-bounces@lists.freedesktop.org
> >[mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Sharma, 
> >Shashank
> >Sent: Tuesday, January 14, 2014 1:20 AM
> >To: Daniel Vetter
> >Cc: intel-gfx@lists.freedesktop.org
> >Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect 
> >and get_modes
> >
> >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@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf 
> >Of Daniel Vetter
> >Sent: Monday, January 13, 2014 6:57 PM
> >To: Sharma, Shashank
> >Cc: C, Ramalingam; intel-gfx@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@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
> >_______________________________________________
> >Intel-gfx mailing list
> >Intel-gfx@lists.freedesktop.org
> >http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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