intel-gfx.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drm/i915: Correct max delay for HDMI hotplug live status checking
@ 2015-12-11  2:08 Gary Wang
  2015-12-11  5:05 ` Jindal, Sonika
  0 siblings, 1 reply; 11+ messages in thread
From: Gary Wang @ 2015-12-11  2:08 UTC (permalink / raw)
  To: intel-gfx

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@intel.com>
Cc: Gavin Hindman <gavin.hindman@intel.com>
Signed-off-by: Gary Wang <gary.c.wang@intel.com>
---
 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)
+	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);
 	}
 
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/i915: Correct max delay for HDMI hotplug live status checking
  2015-12-11  2:08 [PATCH] drm/i915: Correct max delay for HDMI hotplug live status checking Gary Wang
@ 2015-12-11  5:05 ` Jindal, Sonika
  2015-12-11  6:04   ` Wang, Gary C
  2015-12-11 13:04   ` Ville Syrjälä
  0 siblings, 2 replies; 11+ messages in thread
From: Jindal, Sonika @ 2015-12-11  5:05 UTC (permalink / raw)
  To: Wang, Gary C, intel-gfx@lists.freedesktop.org

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@lists.freedesktop.org] On Behalf Of Wang, Gary C
Sent: Friday, December 11, 2015 7:39 AM
To: intel-gfx@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@intel.com>
Cc: Gavin Hindman <gavin.hindman@intel.com>
Signed-off-by: Gary Wang <gary.c.wang@intel.com>
---
 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)
+	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);
 	}
 
--
1.9.1

_______________________________________________
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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/i915: Correct max delay for HDMI hotplug live status checking
  2015-12-11  5:05 ` Jindal, Sonika
@ 2015-12-11  6:04   ` Wang, Gary C
  2015-12-11  6:17     ` Jindal, Sonika
  2015-12-11 13:04   ` Ville Syrjälä
  1 sibling, 1 reply; 11+ messages in thread
From: Wang, Gary C @ 2015-12-11  6:04 UTC (permalink / raw)
  To: Jindal, Sonika, intel-gfx@lists.freedesktop.org

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@intel.com>; intel-gfx@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@lists.freedesktop.org] On Behalf Of Wang, Gary C
Sent: Friday, December 11, 2015 7:39 AM
To: intel-gfx@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@intel.com>
Cc: Gavin Hindman <gavin.hindman@intel.com>
Signed-off-by: Gary Wang <gary.c.wang@intel.com>
---
 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)
+	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);
 	}
 
--
1.9.1

_______________________________________________
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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/i915: Correct max delay for HDMI hotplug live status checking
  2015-12-11  6:04   ` Wang, Gary C
@ 2015-12-11  6:17     ` Jindal, Sonika
  2015-12-11  6:23       ` Wang, Gary C
  0 siblings, 1 reply; 11+ messages in thread
From: Jindal, Sonika @ 2015-12-11  6:17 UTC (permalink / raw)
  To: Wang, Gary C, intel-gfx@lists.freedesktop.org

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@intel.com>; intel-gfx@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@intel.com>; intel-gfx@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@lists.freedesktop.org] On Behalf Of Wang, Gary C
Sent: Friday, December 11, 2015 7:39 AM
To: intel-gfx@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@intel.com>
Cc: Gavin Hindman <gavin.hindman@intel.com>
Signed-off-by: Gary Wang <gary.c.wang@intel.com>
---
 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)
+	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);
 	}
 
--
1.9.1

_______________________________________________
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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/i915: Correct max delay for HDMI hotplug live status checking
  2015-12-11  6:17     ` Jindal, Sonika
@ 2015-12-11  6:23       ` Wang, Gary C
  2015-12-11  7:10         ` Wang, Gary C
  0 siblings, 1 reply; 11+ messages in thread
From: Wang, Gary C @ 2015-12-11  6:23 UTC (permalink / raw)
  To: Jindal, Sonika, intel-gfx@lists.freedesktop.org

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@intel.com>; intel-gfx@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@intel.com>; intel-gfx@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@intel.com>; intel-gfx@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@lists.freedesktop.org] On Behalf Of Wang, Gary C
Sent: Friday, December 11, 2015 7:39 AM
To: intel-gfx@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@intel.com>
Cc: Gavin Hindman <gavin.hindman@intel.com>
Signed-off-by: Gary Wang <gary.c.wang@intel.com>
---
 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)
+	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);
 	}
 
--
1.9.1

_______________________________________________
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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/i915: Correct max delay for HDMI hotplug live status checking
  2015-12-11  6:23       ` Wang, Gary C
@ 2015-12-11  7:10         ` Wang, Gary C
  2015-12-11 18:42           ` Daniel Vetter
  0 siblings, 1 reply; 11+ messages in thread
From: Wang, Gary C @ 2015-12-11  7:10 UTC (permalink / raw)
  To: Wang, Gary C, Jindal, Sonika, intel-gfx@lists.freedesktop.org

I will upload new version of patch for review. Thanks!

-----Original Message-----
From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Wang, Gary C
Sent: Friday, December 11, 2015 2:23 PM
To: Jindal, Sonika <sonika.jindal@intel.com>; intel-gfx@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@intel.com>; intel-gfx@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@intel.com>; intel-gfx@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@intel.com>; intel-gfx@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@lists.freedesktop.org] On Behalf Of Wang, Gary C
Sent: Friday, December 11, 2015 7:39 AM
To: intel-gfx@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@intel.com>
Cc: Gavin Hindman <gavin.hindman@intel.com>
Signed-off-by: Gary Wang <gary.c.wang@intel.com>
---
 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)
+	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);
 	}
 
--
1.9.1

_______________________________________________
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH] drm/i915: Correct max delay for HDMI hotplug live status checking
@ 2015-12-11  7:12 Gary Wang
  2015-12-11 18:45 ` Daniel Vetter
  0 siblings, 1 reply; 11+ messages in thread
From: Gary Wang @ 2015-12-11  7:12 UTC (permalink / raw)
  To: intel-gfx

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@intel.com>
Tested-by: Gary Wang <gary.c.wang@intel.com>
Cc: Gavin Hindman <gavin.hindman@intel.com>
Cc: Sonika Jindal <sonika.jindal@intel.com>
Signed-off-by: Gary Wang <gary.c.wang@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c | 6 ++++--
 1 file changed, 4 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..ba042cf
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1394,11 +1394,13 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
 
 	intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
 
-	while (!live_status && --retry) {
+	do {
 		live_status = intel_digital_port_connected(dev_priv,
 				hdmi_to_dig_port(intel_hdmi));
+		if (live_status || !retry)
+			break;
 		mdelay(10);
-	}
+	} while (retry--);
 
 	if (!live_status)
 		DRM_DEBUG_KMS("Live status not up!");
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/i915: Correct max delay for HDMI hotplug live status checking
  2015-12-11  5:05 ` Jindal, Sonika
  2015-12-11  6:04   ` Wang, Gary C
@ 2015-12-11 13:04   ` Ville Syrjälä
  1 sibling, 0 replies; 11+ messages in thread
From: Ville Syrjälä @ 2015-12-11 13:04 UTC (permalink / raw)
  To: Jindal, Sonika; +Cc: intel-gfx@lists.freedesktop.org

On Fri, Dec 11, 2015 at 05:05:11AM +0000, Jindal, Sonika wrote:
> 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);

How about a straight up for loop instead?

> 
> Regards,
> Sonika
> 
> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Wang, Gary C
> Sent: Friday, December 11, 2015 7:39 AM
> To: intel-gfx@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@intel.com>
> Cc: Gavin Hindman <gavin.hindman@intel.com>
> Signed-off-by: Gary Wang <gary.c.wang@intel.com>
> ---
>  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)
> +	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);
>  	}
>  
> --
> 1.9.1
> 
> _______________________________________________
> 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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/i915: Correct max delay for HDMI hotplug live status checking
  2015-12-11  7:10         ` Wang, Gary C
@ 2015-12-11 18:42           ` Daniel Vetter
  2015-12-14  2:30             ` Wang, Gary C
  0 siblings, 1 reply; 11+ messages in thread
From: Daniel Vetter @ 2015-12-11 18:42 UTC (permalink / raw)
  To: Wang, Gary C; +Cc: intel-gfx@lists.freedesktop.org

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@lists.freedesktop.org] On Behalf Of Wang, Gary C
> Sent: Friday, December 11, 2015 2:23 PM
> To: Jindal, Sonika <sonika.jindal@intel.com>; intel-gfx@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@intel.com>; intel-gfx@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@intel.com>; intel-gfx@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@intel.com>; intel-gfx@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@lists.freedesktop.org] On Behalf Of Wang, Gary C
> Sent: Friday, December 11, 2015 7:39 AM
> To: intel-gfx@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@intel.com>
> Cc: Gavin Hindman <gavin.hindman@intel.com>
> Signed-off-by: Gary Wang <gary.c.wang@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@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
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/i915: Correct max delay for HDMI hotplug live status checking
  2015-12-11  7:12 Gary Wang
@ 2015-12-11 18:45 ` Daniel Vetter
  0 siblings, 0 replies; 11+ messages in thread
From: Daniel Vetter @ 2015-12-11 18:45 UTC (permalink / raw)
  To: Gary Wang; +Cc: intel-gfx

On Fri, Dec 11, 2015 at 03:12:05PM +0800, Gary Wang wrote:
> 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@intel.com>
> Tested-by: Gary Wang <gary.c.wang@intel.com>
> Cc: Gavin Hindman <gavin.hindman@intel.com>
> Cc: Sonika Jindal <sonika.jindal@intel.com>
> Signed-off-by: Gary Wang <gary.c.wang@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_hdmi.c | 6 ++++--
>  1 file changed, 4 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..ba042cf
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1394,11 +1394,13 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
>  
>  	intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
>  
> -	while (!live_status && --retry) {
> +	do {
>  		live_status = intel_digital_port_connected(dev_priv,
>  				hdmi_to_dig_port(intel_hdmi));
> +		if (live_status || !retry)
> +			break;
>  		mdelay(10);
> -	}
> +	} while (retry--);

I agree with Ville, let's just go with an obviously correct for loop here.
This is apparently too hard ;-)
-Daniel

>  
>  	if (!live_status)
>  		DRM_DEBUG_KMS("Live status not up!");
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] drm/i915: Correct max delay for HDMI hotplug live status checking
  2015-12-11 18:42           ` Daniel Vetter
@ 2015-12-14  2:30             ` Wang, Gary C
  0 siblings, 0 replies; 11+ messages in thread
From: Wang, Gary C @ 2015-12-14  2:30 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: intel-gfx@lists.freedesktop.org

I totally agree on Ville and Daniel's comments and am sorry for incorrect text format of some description.

Following code should be more clear. It only delays 3 times (30ms delay) at most for checking hot-plug status.

-       unsigned int retry = 3;
+       unsigned int retry;

        DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
                      connector->base.id, connector->name);

-       while (!live_status && --retry) {
+       for (retry = 3; retry >= 0; retry--) {
                live_status = intel_digital_port_connected(dev_priv,
                                hdmi_to_dig_port(intel_hdmi));
-               mdelay(10);
+               if (live_status || !retry)
+                               break;
+               msleep(10);
        }

        if (!live_status)


BTW, I list again for hot-plug delay test on my BSW platform. It seems to be not enough with 30ms delay for some old HDMI monitor,


Please refer following information for my BSW platform,
1. Lenovo Mode: L246 1xwA (4/30 failed, necessary hot-plug delay: 58/40/60/40ms) ,
2. Philips Model : HH2AP (9/30 failed, necessary hot-plug delay: 80/50/50/60/46/40/58/58/39ms) ,
3. BENQ Model:ET-0035-N (6/30 failed, necessary hot-plug delay: 60/50/50/80/80/40ms) ,
4. DELL  Model:U2713HM (2/30 failed, necessary hot-plug delay: 58/59ms) ,
5. 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!

Gary

-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
Sent: Saturday, December 12, 2015 2:43 AM
To: Wang, Gary C <gary.c.wang@intel.com>
Cc: Jindal, Sonika <sonika.jindal@intel.com>; intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Correct max delay for HDMI hotplug live status checking

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@lists.freedesktop.org] On 
> Behalf Of Wang, Gary C
> Sent: Friday, December 11, 2015 2:23 PM
> To: Jindal, Sonika <sonika.jindal@intel.com>; 
> intel-gfx@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@intel.com>; 
> intel-gfx@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@intel.com>; 
> intel-gfx@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@intel.com>; 
> intel-gfx@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@lists.freedesktop.org] On 
> Behalf Of Wang, Gary C
> Sent: Friday, December 11, 2015 7:39 AM
> To: intel-gfx@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@intel.com>
> Cc: Gavin Hindman <gavin.hindman@intel.com>
> Signed-off-by: Gary Wang <gary.c.wang@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@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
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2015-12-14  2:34 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-12-11  2:08 [PATCH] drm/i915: Correct max delay for HDMI hotplug live status checking Gary Wang
2015-12-11  5:05 ` Jindal, Sonika
2015-12-11  6:04   ` Wang, Gary C
2015-12-11  6:17     ` Jindal, Sonika
2015-12-11  6:23       ` Wang, Gary C
2015-12-11  7:10         ` Wang, Gary C
2015-12-11 18:42           ` Daniel Vetter
2015-12-14  2:30             ` Wang, Gary C
2015-12-11 13:04   ` Ville Syrjälä
  -- strict thread matches above, loose matches on Subject: below --
2015-12-11  7:12 Gary Wang
2015-12-11 18:45 ` Daniel Vetter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).