Ok, I will change the implementation. Regards Shashank -----Original Message----- From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel Vetter Sent: Friday, April 11, 2014 7:53 PM To: Sharma, Shashank Cc: Daniel Vetter; C, Ramalingam; Wang, Quanxian; intel-gfx Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect and get_modes
On Fri, Apr 11, 2014 at 01:23:43PM +0000, Sharma, Shashank wrote: > Thanks for the comments, > Actually, we are not using live_status at all. > > The check for < gen6 is only for EDID caching. So if the HW is >= gen6 > cache_edid. > Else do not cache EDID, so that we will not block any of the old HW, which > might not be HPD capable. Oh, I've thought that this is incremental on top of something you already have. > > Does it sound ok now :) ? No. HPD is _NOT_ I repeat _NOT_ reliably. Not even on gen6+. live status simply reflects the hpd pin, if either doesn't work, then neither does the other one. Nacked-with-prejudice-by: Daniel Vetter <daniel.vet...@ffwll.ch> Cheers, Daniel > > Regards > Shashank > -----Original Message----- > From: daniel.vet...@ffwll.ch [mailto:daniel.vet...@ffwll.ch] On Behalf > Of Daniel Vetter > Sent: Friday, April 11, 2014 6:28 PM > To: Sharma, Shashank > Cc: C, Ramalingam; Wang, Quanxian; intel-gfx > Subject: Re: [Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect > and get_modes > > Please don't drop the mailing list when discussing patches. > > And nope, conditioning this on gen6+ won't help at all, since I have a > gen6 and gen7 machine here which don't have reliable hdmi live status. > Afaik that is _really_ broken across the board and going with the fancy > invalidation scheme and delayed work items is the way forward. > > Cheers, Daniel > > On Fri, Apr 11, 2014 at 2:51 PM, Sharma, Shashank <shashank.sha...@intel.com> > wrote: > > Hi Daniel, > > > > First of all, sorry for the delay in coming up with the new patch set. I > > got stuck with some commercial projects :) . > > > > There were few review comments what you gave for the previous optimization > > patch, those were: > > 1. This might break the old HWs. Few of them might not even be HPD > > capable, so its required to handle them. > > 2. Do not rely on the live_status check, as that HW is not yet proven. > > And you recommended to have a WQ, which will invalidate the cached HDMI > > EDID after a timeout. > > > > I have written another patch, which is addressing both of the above > > comments, but doesn't use a WQ. > > > > Before sending this to ML, I wanted to have an opinion from you, if this > > qualifies the design you were expecting. > > I will be really glad, if you can spare some time, and just have a top > > level view on the patch, and give your opinion. > > > > If you think that this is still not the way, and making a WQ is a better > > approach, I would gladly create a patch which applies that. > > Or if you will prefer to have this reviewed in the mailing list only, > > please accept my apologies for direct contact, and just let me know so. I > > will publish this in ML. > > > > The patch is (It's also attached): > > > > From e7ec4e4827615c0ff3a4ddb020df35fdf1b0b82f Mon Sep 17 00:00:00 > > 2001 > > From: Shashank Sharma <shashank.sha...@intel.com> > > Date: Fri, 11 Apr 2014 18:02:50 +0530 > > Subject: [PATCH] drm/i915: Cache HDMI EDID for future reference > > > > This patch contains following changes: > > 1.Cache HDMI EDID and optimize detect() calls, which are > > frequently called from userspace, causing un-necessary > > EDID reads. > > 2.This cached EDID will be used for detection of the status of > > HDMI connector, for Non HPD detect() calls. HPD calls will update > > cached EDID. > > 3.This optimization is kept for new HW's (Gen6 and +), so that It > > will not break old HWs who may not be even HPD capable. > > > > Signed-off-by: Shashank Sharma <shashank.sha...@intel.com> > > --- > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > drivers/gpu/drm/i915/intel_hdmi.c | 28 ++++++++++++++++++++++++++-- > > 2 files changed, 27 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > b/drivers/gpu/drm/i915/intel_drv.h > > index 42762b7..b7911df 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -478,6 +478,7 @@ struct intel_hdmi { > > const void *frame, ssize_t len); > > void (*set_infoframes)(struct drm_encoder *encoder, > > struct drm_display_mode > > *adjusted_mode); > > + struct edid *edid; > > }; > > > > #define DP_MAX_DOWNSTREAM_PORTS 0x10 > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c > > b/drivers/gpu/drm/i915/intel_hdmi.c > > index b0413e1..33550ca 100644 > > --- a/drivers/gpu/drm/i915/intel_hdmi.c > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > > @@ -938,7 +938,7 @@ intel_hdmi_detect(struct drm_connector *connector, bool > > force) > > hdmi_to_dig_port(intel_hdmi); > > struct intel_encoder *intel_encoder = &intel_dig_port->base; > > struct drm_i915_private *dev_priv = dev->dev_private; > > - struct edid *edid; > > + struct edid *edid = NULL; > > enum intel_display_power_domain power_domain; > > enum drm_connector_status status = > > connector_status_disconnected; > > > > @@ -948,6 +948,21 @@ intel_hdmi_detect(struct drm_connector *connector, > > bool force) > > power_domain = intel_display_port_power_domain(intel_encoder); > > intel_display_power_get(dev_priv, power_domain); > > > > + /* > > + * Support EDID caching only for new architectures > > + * Let old architectures probe and force read EDID > > + */ > > + if (INTEL_INFO(dev)->gen >= 6) { > > + if (force && intel_hdmi->edid && > > + (connector->status != connector_status_unknown)) { > > + /* Optimize userspace query, read EDID only > > + in case of a real hot plug */ > > + DRM_DEBUG_KMS("HDMI %s", intel_hdmi->edid ? > > + "Connected" : "Disconnected"); > > + return connector->status; > > + } > > + } > > + > > intel_hdmi->has_hdmi_sink = false; > > intel_hdmi->has_audio = false; > > intel_hdmi->rgb_quant_range_selectable = false; @@ -964,10 > > +979,19 @@ intel_hdmi_detect(struct drm_connector *connector, bool > > +force) > > intel_hdmi->has_audio = > > drm_detect_monitor_audio(edid); > > intel_hdmi->rgb_quant_range_selectable = > > > > drm_rgb_quant_range_selectable(edid); > > + DRM_DEBUG_KMS("HDMI Connected\n"); > > } > > - kfree(edid); > > + } else { > > + /* > > + * Connector disconnected, free cached EDID > > + * kfree is NULL protected, so will work for < gen 6 also */ > > + kfree(intel_hdmi->edid); > > + DRM_DEBUG_KMS("HDMI Disconnected\n"); > > } > > > > + /* Save latest EDID for further queries */ > > + intel_hdmi->edid = edid; > > + > > if (status == connector_status_connected) { > > if (intel_hdmi->force_audio != HDMI_AUDIO_AUTO) > > intel_hdmi->has_audio = > > -- > > 1.7.10.4 > > > > > > -----Original Message----- > > From: Wang, Quanxian > > Sent: Thursday, April 10, 2014 4:12 PM > > 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 > > > > Tizen-IVI has one feature will require hdmi edid cache. It will be happy to > > find your patch pushed into upstream tree. > > > > Regards > > > > Thanks > > > > Quanxian Wang > > > > -----Original Message----- > > From: Intel-gfx [mailto:intel-gfx-boun...@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-boun...@lists.freedesktop.org > >>[mailto:intel-gfx-boun...@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.vet...@ffwll.ch [mailto:daniel.vet...@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.sha...@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 -- 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
