[Intel-gfx] [PATCH] drm/i915: Correct max delay for HDMI hotplug live status checking
Daniel Vetter
daniel at ffwll.ch
Fri Dec 11 10:42:54 PST 2015
On Fri, Dec 11, 2015 at 07:10:35AM +0000, Wang, Gary C wrote:
> I will upload new version of patch for review. Thanks!
>
> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf Of Wang, Gary C
> Sent: Friday, December 11, 2015 2:23 PM
> To: Jindal, Sonika <sonika.jindal at intel.com>; intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Correct max delay for HDMI hotplug live status checking
>
> No problem.
>
> Please refer following information.
> Lenovo Mode: L246 1xwA (4/30 failed, necessary hot-plug delay: 58/40/60/40ms) Philips Model : HH2AP (9/30 failed, necessary hot-plug delay: 80/50/50/60/46/40/58/58/39ms) BENQ Model:ET-0035-N (6/30 failed, necessary hot-plug delay: 60/50/50/80/80/40ms) DELL Model:U2713HM (2/30 failed, necessary hot-plug delay: 58/59ms) HP Model: HP-LP2475w (5/30 failed, necessary hot-plug delay: 70/50/40/60/40ms)
>
> Those test were based on following test-case (test continually with 30 test cycles), 1. HDMI display correctly 2. Un-plug/re-plug HDMI cable 3. Wait for one sec
>
> Thanks for your review!
>
> Gary
>
> -----Original Message-----
> From: Jindal, Sonika
> Sent: Friday, December 11, 2015 2:18 PM
> To: Wang, Gary C <gary.c.wang at intel.com>; intel-gfx at lists.freedesktop.org
> Subject: RE: [Intel-gfx] [PATCH] drm/i915: Correct max delay for HDMI hotplug live status checking
>
> Sure.
> Can you please also post your findings about different panels taking different time to settle the live status so that we know that this increase in retrials is helping you with multiple monitors?
>
> Regards,
> Sonika
>
>
> -----Original Message-----
> From: Wang, Gary C
> Sent: Friday, December 11, 2015 11:34 AM
> To: Jindal, Sonika <sonika.jindal at intel.com>; intel-gfx at lists.freedesktop.org
> Subject: RE: [Intel-gfx] [PATCH] drm/i915: Correct max delay for HDMI hotplug live status checking
>
> Hi Sonika,
>
> Thanks for your comment, and it's more readability on code.
>
> But it only delay 2 integration of each 10ms among checking hot-plug with 3 times. Could I change it to following snippet code to read hotplug_status 4 times at most for 3 integration 10ms delay?
>
> unsigned int retry = 3;
>
> do {
> live_status = intel_digital_port_connected(dev_priv,
> hdmi_to_dig_port(intel_hdmi));
> if (live_status || !retry)
> break;
> mdelay(10);
> } while (retry--);
>
> Thanks!
>
> Gary
>
> -----Original Message-----
> From: Jindal, Sonika
> Sent: Friday, December 11, 2015 1:05 PM
> To: Wang, Gary C <gary.c.wang at intel.com>; intel-gfx at lists.freedesktop.org
> Subject: RE: [Intel-gfx] [PATCH] drm/i915: Correct max delay for HDMI hotplug live status checking
>
> How about following instead of two levels of check in the while loop:
>
> unsigned int retry = 3;
>
> do {
> live_status = intel_digital_port_connected(dev_priv,
> hdmi_to_dig_port(intel_hdmi));
> if (live_status)
> break;
> mdelay(10);
> } while (--retry);
>
> Regards,
> Sonika
>
> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf Of Wang, Gary C
> Sent: Friday, December 11, 2015 7:39 AM
> To: intel-gfx at lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH] drm/i915: Correct max delay for HDMI hotplug live status checking
>
> The total delay of HDMI hotplug detecting with 30ms should have been split into a resolution of 3 retries of 10ms each, for the worst cases. But it still suffered from only waiting 10ms at most in intel_hdmi_detect(). This patch corrects it by reading hotplug status with 4 times at most for 30ms delay.
>
> Reviewed-by: Cooper Chiou <cooper.chiou at intel.com>
> Cc: Gavin Hindman <gavin.hindman at intel.com>
> Signed-off-by: Gary Wang <gary.c.wang at intel.com>
Can we please have less top-posting when reviewing patches? I can't really
read the above with my bare-knuckles console mail client I use for reading
patch mailing lists ;-)
> ---
> drivers/gpu/drm/i915/intel_hdmi.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-) mode change 100644 => 100755 drivers/gpu/drm/i915/intel_hdmi.c
>
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> old mode 100644
> new mode 100755
> index be7fab9..888401b
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1387,16 +1387,19 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
> struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
> struct drm_i915_private *dev_priv = to_i915(connector->dev);
> bool live_status = false;
> - unsigned int retry = 3;
> + // read hotplug status 4 times at most for 30ms delay (3 retries of 10ms each)
Please no C++ comments, also your code should be self-explanatory enough
that this is obvious.
> + unsigned int retry = 4;
>
> DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> connector->base.id, connector->name);
>
> intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
>
> - while (!live_status && --retry) {
> + while (!live_status && retry--) {
> live_status = intel_digital_port_connected(dev_priv,
> hdmi_to_dig_port(intel_hdmi));
> + if (live_status || !retry)
> + break;
> mdelay(10);
Whiel at it please review my patch to switch this one to an msleep - it's
not nice to kill the battery with a busy loop like this.
-Daniel
> }
>
> --
> 1.9.1
>
> _______________________________________________
> 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
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list