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. Does it sound ok now :) ? Regards Shashank -----Original Message----- From: daniel.vetter@xxxxxxxx [mailto:daniel.vetter@xxxxxxxx] 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: [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.sharma@xxxxxxxxx> 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.sharma@xxxxxxxxx> > 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.sharma@xxxxxxxxx> > --- > 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@xxxxxxxxxxxxxxxxxxxxx > Subject: RE: [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-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
