[Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect and get_modes

Sharma, Shashank shashank.sharma at intel.com
Fri Apr 11 15:23:43 CEST 2014


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 at ffwll.ch [mailto:daniel.vetter at 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 at 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 at 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 at 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 at 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 at lists.freedesktop.org] On 
> Behalf Of Sharma, Shashank
> Sent: Wednesday, April 09, 2014 12:20 PM
> To: Wang, Quanxian; Daniel Vetter
> Cc: intel-gfx at 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 at 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 at lists.freedesktop.org
>>[mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf Of Sharma, 
>>Shashank
>>Sent: Tuesday, January 14, 2014 1:20 AM
>>To: Daniel Vetter
>>Cc: intel-gfx at 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 at ffwll.ch [mailto:daniel.vetter at ffwll.ch] On Behalf 
>>Of Daniel Vetter
>>Sent: Monday, January 13, 2014 6:57 PM
>>To: Sharma, Shashank
>>Cc: C, Ramalingam; intel-gfx at 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 at 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 at lists.freedesktop.org
>>http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at 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



More information about the Intel-gfx mailing list