[Intel-gfx] [PATCH 0/2] Optimization on intel HDMI detect and get_modes
Daniel Vetter
daniel at ffwll.ch
Fri Apr 11 14:58:08 CEST 2014
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