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

From: Daniel Vetter <daniel@ffwll.ch>
To: "Sharma, Shashank" <shashank.sharma@intel.com>
Cc: intel-gfx <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 0/2] Optimization on intel HDMI detect and get_modes
Date: Fri, 11 Apr 2014 16:22:32 +0200	[thread overview]
Message-ID: <20140411142232.GI9262@phenom.ffwll.local> (raw)
In-Reply-To: <FF3DDC77922A8A4BB08A3BC48A1EA8CB01692AC3@BGSMSX101.gar.corp.intel.com>

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.vetter@ffwll.ch>

Cheers, Daniel

> 
> Regards
> Shashank 
> -----Original Message-----
> From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@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.sharma@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.sharma@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.sharma@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-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

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