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@xxxxxxxxxxxxxxxxxxxxx] On Behalf Of Sharma, Shashank > Sent: Wednesday, April 09, 2014 12:20 PM > To: Wang, Quanxian; Daniel Vetter > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [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@xxxxxxxxxxxxxxxxxxxxx > Subject: RE: [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@xxxxxxxxxxxxxxxxxxxxx > >[mailto:intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx] On Behalf Of Sharma, > >Shashank > >Sent: Tuesday, January 14, 2014 1:20 AM > >To: Daniel Vetter > >Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > >Subject: Re: [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@xxxxxxxx [mailto:daniel.vetter@xxxxxxxx] On Behalf > >Of Daniel Vetter > >Sent: Monday, January 13, 2014 6:57 PM > >To: Sharma, Shashank > >Cc: C, Ramalingam; intel-gfx@xxxxxxxxxxxxxxxxxxxxx > >Subject: Re: [PATCH 0/2] Optimization on intel HDMI detect > >and get_modes > > > >On Mon, Jan 13, 2014 at 10:39 AM, Sharma, Shashank > ><shashank.sharma@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx > >http://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
