[PATCH] drm/i915: Increase the response time for slow SDVO devices

[PATCH] drm/i915: Increase the response time for slow SDVO devices

[Chris Wilson]

Some devices may respond very slowly and only flag that the reply is
pending within the first 15us response window. Be kind to such devices
and wait a further 15ms, before checking for the pending reply. This
moves the existing special case delay of 30ms down from the detection
routine into the common path and pretends to explain it...

Tested-by: bo.b.wang@intel.com
References: https://bugs.freedesktop.org/show_bug.cgi?id=36997
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_sdvo.c |   39 ++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index d85ebb0..f0a1a6f 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -522,19 +522,32 @@ static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo,
 	 * command to be complete.
 	 *
 	 * Check 5 times in case the hardware failed to read the docs.
+	 *
+	 * Also beware that the first response by many devices is to
+	 * reply PENDING and stall for time. TVs are notorious for
+	 * requiring longer than specified to complete their replies.
 	 */
 	if (!intel_sdvo_read_byte(intel_sdvo,
 				  SDVO_I2C_CMD_STATUS,
 				  &status))
 		goto log_fail;
 
-	while (status == SDVO_CMD_STATUS_PENDING && retry--) {
-		udelay(15);
-		if (!intel_sdvo_read_byte(intel_sdvo,
-					  SDVO_I2C_CMD_STATUS,
-					  &status))
-			goto log_fail;
-	}
+	do {
+		int quick = 5;
+
+		while (status == SDVO_CMD_STATUS_PENDING && quick--) {
+			udelay(15);
+			if (!intel_sdvo_read_byte(intel_sdvo,
+						  SDVO_I2C_CMD_STATUS,
+						  &status))
+				goto log_fail;
+		}
+
+		if (status != SDVO_CMD_STATUS_PENDING || --retry == 0)
+			break;
+
+		msleep(15);
+	} while (1);
 
 	if (status <= SDVO_CMD_STATUS_SCALING_NOT_SUPP)
 		DRM_LOG_KMS("(%s)", cmd_status_names[status]);
@@ -1535,15 +1548,9 @@ intel_sdvo_detect(struct drm_connector *connector, bool force)
 	struct intel_sdvo_connector *intel_sdvo_connector = to_intel_sdvo_connector(connector);
 	enum drm_connector_status ret;
 
-	if (!intel_sdvo_write_cmd(intel_sdvo,
-				  SDVO_CMD_GET_ATTACHED_DISPLAYS, NULL, 0))
-		return connector_status_unknown;
-
-	/* add 30ms delay when the output type might be TV */
-	if (intel_sdvo->caps.output_flags & SDVO_TV_MASK)
-		msleep(30);
-
-	if (!intel_sdvo_read_response(intel_sdvo, &response, 2))
+	if (!intel_sdvo_get_value(intel_sdvo,
+				  SDVO_CMD_GET_ATTACHED_DISPLAYS,
+				  &response, 2))
 		return connector_status_unknown;
 
 	DRM_DEBUG_KMS("SDVO response %d %d [%x]\n",
-- 
1.7.10.4

Re: [PATCH] drm/i915: Increase the response time for slow SDVO devices

[Jani Nikula]

On Fri, 23 Nov 2012, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Some devices may respond very slowly and only flag that the reply is
> pending within the first 15us response window. Be kind to such devices
> and wait a further 15ms, before checking for the pending reply. This
> moves the existing special case delay of 30ms down from the detection
> routine into the common path and pretends to explain it...
>
> Tested-by: bo.b.wang@intel.com
> References: https://bugs.freedesktop.org/show_bug.cgi?id=36997
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_sdvo.c |   39 ++++++++++++++++++++++---------------
>  1 file changed, 23 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index d85ebb0..f0a1a6f 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -522,19 +522,32 @@ static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo,
>  	 * command to be complete.
>  	 *
>  	 * Check 5 times in case the hardware failed to read the docs.
> +	 *
> +	 * Also beware that the first response by many devices is to
> +	 * reply PENDING and stall for time. TVs are notorious for
> +	 * requiring longer than specified to complete their replies.
>  	 */
>  	if (!intel_sdvo_read_byte(intel_sdvo,
>  				  SDVO_I2C_CMD_STATUS,
>  				  &status))
>  		goto log_fail;
>  
> -	while (status == SDVO_CMD_STATUS_PENDING && retry--) {
> -		udelay(15);
> -		if (!intel_sdvo_read_byte(intel_sdvo,
> -					  SDVO_I2C_CMD_STATUS,
> -					  &status))
> -			goto log_fail;
> -	}
> +	do {
> +		int quick = 5;
> +
> +		while (status == SDVO_CMD_STATUS_PENDING && quick--) {
> +			udelay(15);
> +			if (!intel_sdvo_read_byte(intel_sdvo,
> +						  SDVO_I2C_CMD_STATUS,
> +						  &status))
> +				goto log_fail;
> +		}
> +
> +		if (status != SDVO_CMD_STATUS_PENDING || --retry == 0)
> +			break;
> +
> +		msleep(15);
> +	} while (1);

Is your intention to have 5 quick retries nested in 5 slow retries,
resulting in 25 retries total? What do the quick retries buy you after
the first msleep(15)? In other words, why not just do something simple
like:

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 30f1752..3b2eddc 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -504,7 +504,7 @@ out:
 static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo,
 				     void *response, int response_len)
 {
-	u8 retry = 5;
+	u8 retry = 2*5;
 	u8 status;
 	int i;
 
@@ -524,7 +524,10 @@ static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo,
 		goto log_fail;
 
 	while (status == SDVO_CMD_STATUS_PENDING && retry--) {
-		udelay(15);
+		if (retry >= 5)
+			udelay(15);
+		else
+			msleep(15);
 		if (!intel_sdvo_read_byte(intel_sdvo,
 					  SDVO_I2C_CMD_STATUS,
 					  &status))

BR,
Jani.


>  
>  	if (status <= SDVO_CMD_STATUS_SCALING_NOT_SUPP)
>  		DRM_LOG_KMS("(%s)", cmd_status_names[status]);
> @@ -1535,15 +1548,9 @@ intel_sdvo_detect(struct drm_connector *connector, bool force)
>  	struct intel_sdvo_connector *intel_sdvo_connector = to_intel_sdvo_connector(connector);
>  	enum drm_connector_status ret;
>  
> -	if (!intel_sdvo_write_cmd(intel_sdvo,
> -				  SDVO_CMD_GET_ATTACHED_DISPLAYS, NULL, 0))
> -		return connector_status_unknown;
> -
> -	/* add 30ms delay when the output type might be TV */
> -	if (intel_sdvo->caps.output_flags & SDVO_TV_MASK)
> -		msleep(30);
> -
> -	if (!intel_sdvo_read_response(intel_sdvo, &response, 2))
> +	if (!intel_sdvo_get_value(intel_sdvo,
> +				  SDVO_CMD_GET_ATTACHED_DISPLAYS,
> +				  &response, 2))
>  		return connector_status_unknown;
>  
>  	DRM_DEBUG_KMS("SDVO response %d %d [%x]\n",
> -- 
> 1.7.10.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Increase the response time for slow SDVO devices

[Chris Wilson]

On Fri, 23 Nov 2012 13:42:38 +0200, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Fri, 23 Nov 2012, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > Some devices may respond very slowly and only flag that the reply is
> > pending within the first 15us response window. Be kind to such devices
> > and wait a further 15ms, before checking for the pending reply. This
> > moves the existing special case delay of 30ms down from the detection
> > routine into the common path and pretends to explain it...
> >
> > Tested-by: bo.b.wang@intel.com
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=36997
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/intel_sdvo.c |   39 ++++++++++++++++++++++---------------
> >  1 file changed, 23 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> > index d85ebb0..f0a1a6f 100644
> > --- a/drivers/gpu/drm/i915/intel_sdvo.c
> > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> > @@ -522,19 +522,32 @@ static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo,
> >  	 * command to be complete.
> >  	 *
> >  	 * Check 5 times in case the hardware failed to read the docs.
> > +	 *
> > +	 * Also beware that the first response by many devices is to
> > +	 * reply PENDING and stall for time. TVs are notorious for
> > +	 * requiring longer than specified to complete their replies.
> >  	 */
> >  	if (!intel_sdvo_read_byte(intel_sdvo,
> >  				  SDVO_I2C_CMD_STATUS,
> >  				  &status))
> >  		goto log_fail;
> >  
> > -	while (status == SDVO_CMD_STATUS_PENDING && retry--) {
> > -		udelay(15);
> > -		if (!intel_sdvo_read_byte(intel_sdvo,
> > -					  SDVO_I2C_CMD_STATUS,
> > -					  &status))
> > -			goto log_fail;
> > -	}
> > +	do {
> > +		int quick = 5;
> > +
> > +		while (status == SDVO_CMD_STATUS_PENDING && quick--) {
> > +			udelay(15);
> > +			if (!intel_sdvo_read_byte(intel_sdvo,
> > +						  SDVO_I2C_CMD_STATUS,
> > +						  &status))
> > +				goto log_fail;
> > +		}
> > +
> > +		if (status != SDVO_CMD_STATUS_PENDING || --retry == 0)
> > +			break;
> > +
> > +		msleep(15);
> > +	} while (1);
> 
> Is your intention to have 5 quick retries nested in 5 slow retries,
> resulting in 25 retries total? What do the quick retries buy you after
> the first msleep(15)? In other words, why not just do something simple
> like:

Sure, looks better.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: Increase the response time for slow SDVO devices

[Chris Wilson]

Some devices may respond very slowly and only flag that the reply is
pending within the first 15us response window. Be kind to such devices
and wait a further 15ms, before checking for the pending reply. This
moves the existing special case delay of 30ms down from the detection
routine into the common path and pretends to explain it...

v2: Simplify the loop constructs as suggested by Jani Nikula.

References: https://bugs.freedesktop.org/show_bug.cgi?id=36997
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_sdvo.c |   31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index d85ebb0..cff3c0b 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -509,7 +509,7 @@ out:
 static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo,
 				     void *response, int response_len)
 {
-	u8 retry = 5;
+	u8 retry = 15; /* 5 quick checks, followed by 10 long checks */
 	u8 status;
 	int i;
 
@@ -522,14 +522,27 @@ static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo,
 	 * command to be complete.
 	 *
 	 * Check 5 times in case the hardware failed to read the docs.
+	 *
+	 * Also beware that the first response by many devices is to
+	 * reply PENDING and stall for time. TVs are notorious for
+	 * requiring longer than specified to complete their replies.
+	 * Originally (in the DDX long ago), the delay was only ever 15ms
+	 * with an additional delay of 30ms applied for TVs added later after
+	 * many experiments. To accommodate both sets of delays, we do a
+	 * sequence of slow checks if the device is falling behind and fails
+	 * to reply within 5*15µs.
 	 */
 	if (!intel_sdvo_read_byte(intel_sdvo,
 				  SDVO_I2C_CMD_STATUS,
 				  &status))
 		goto log_fail;
 
-	while (status == SDVO_CMD_STATUS_PENDING && retry--) {
-		udelay(15);
+	while (status == SDVO_CMD_STATUS_PENDING && --retry) {
+		if (retry < 10)
+			msleep(15);
+		else
+			udelay(15);
+
 		if (!intel_sdvo_read_byte(intel_sdvo,
 					  SDVO_I2C_CMD_STATUS,
 					  &status))
@@ -1535,15 +1548,9 @@ intel_sdvo_detect(struct drm_connector *connector, bool force)
 	struct intel_sdvo_connector *intel_sdvo_connector = to_intel_sdvo_connector(connector);
 	enum drm_connector_status ret;
 
-	if (!intel_sdvo_write_cmd(intel_sdvo,
-				  SDVO_CMD_GET_ATTACHED_DISPLAYS, NULL, 0))
-		return connector_status_unknown;
-
-	/* add 30ms delay when the output type might be TV */
-	if (intel_sdvo->caps.output_flags & SDVO_TV_MASK)
-		msleep(30);
-
-	if (!intel_sdvo_read_response(intel_sdvo, &response, 2))
+	if (!intel_sdvo_get_value(intel_sdvo,
+				  SDVO_CMD_GET_ATTACHED_DISPLAYS,
+				  &response, 2))
 		return connector_status_unknown;
 
 	DRM_DEBUG_KMS("SDVO response %d %d [%x]\n",
-- 
1.7.10.4

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

Re: [PATCH] drm/i915: Increase the response time for slow SDVO devices

[Jani Nikula]

On Fri, 23 Nov 2012, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Some devices may respond very slowly and only flag that the reply is
> pending within the first 15us response window. Be kind to such devices
> and wait a further 15ms, before checking for the pending reply. This
> moves the existing special case delay of 30ms down from the detection
> routine into the common path and pretends to explain it...
>
> v2: Simplify the loop constructs as suggested by Jani Nikula.
>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=36997
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_sdvo.c |   31 +++++++++++++++++++------------
>  1 file changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index d85ebb0..cff3c0b 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -509,7 +509,7 @@ out:
>  static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo,
>  				     void *response, int response_len)
>  {
> -	u8 retry = 5;
> +	u8 retry = 15; /* 5 quick checks, followed by 10 long checks */
>  	u8 status;
>  	int i;
>  
> @@ -522,14 +522,27 @@ static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo,
>  	 * command to be complete.
>  	 *
>  	 * Check 5 times in case the hardware failed to read the docs.
> +	 *
> +	 * Also beware that the first response by many devices is to
> +	 * reply PENDING and stall for time. TVs are notorious for
> +	 * requiring longer than specified to complete their replies.
> +	 * Originally (in the DDX long ago), the delay was only ever 15ms
> +	 * with an additional delay of 30ms applied for TVs added later after
> +	 * many experiments. To accommodate both sets of delays, we do a
> +	 * sequence of slow checks if the device is falling behind and fails
> +	 * to reply within 5*15µs.
>  	 */
>  	if (!intel_sdvo_read_byte(intel_sdvo,
>  				  SDVO_I2C_CMD_STATUS,
>  				  &status))
>  		goto log_fail;
>  
> -	while (status == SDVO_CMD_STATUS_PENDING && retry--) {
> -		udelay(15);
> +	while (status == SDVO_CMD_STATUS_PENDING && --retry) {

Hey, why did you switch from post to pre decrement? It will now retry
only retry-1 times. Or is this about the semantics of retries vs. tries?
;)

Otherwise,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> +		if (retry < 10)
> +			msleep(15);
> +		else
> +			udelay(15);
> +
>  		if (!intel_sdvo_read_byte(intel_sdvo,
>  					  SDVO_I2C_CMD_STATUS,
>  					  &status))
> @@ -1535,15 +1548,9 @@ intel_sdvo_detect(struct drm_connector *connector, bool force)
>  	struct intel_sdvo_connector *intel_sdvo_connector = to_intel_sdvo_connector(connector);
>  	enum drm_connector_status ret;
>  
> -	if (!intel_sdvo_write_cmd(intel_sdvo,
> -				  SDVO_CMD_GET_ATTACHED_DISPLAYS, NULL, 0))
> -		return connector_status_unknown;
> -
> -	/* add 30ms delay when the output type might be TV */
> -	if (intel_sdvo->caps.output_flags & SDVO_TV_MASK)
> -		msleep(30);
> -
> -	if (!intel_sdvo_read_response(intel_sdvo, &response, 2))
> +	if (!intel_sdvo_get_value(intel_sdvo,
> +				  SDVO_CMD_GET_ATTACHED_DISPLAYS,
> +				  &response, 2))
>  		return connector_status_unknown;
>  
>  	DRM_DEBUG_KMS("SDVO response %d %d [%x]\n",
> -- 
> 1.7.10.4
>
> _______________________________________________
> 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

Re: [PATCH] drm/i915: Increase the response time for slow SDVO devices

[Chris Wilson]

[-- Attachment #1: Type: text/plain, Size: 2696 bytes --]

On Fri, 23 Nov 2012 14:21:58 +0200, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Fri, 23 Nov 2012, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > Some devices may respond very slowly and only flag that the reply is
> > pending within the first 15us response window. Be kind to such devices
> > and wait a further 15ms, before checking for the pending reply. This
> > moves the existing special case delay of 30ms down from the detection
> > routine into the common path and pretends to explain it...
> >
> > v2: Simplify the loop constructs as suggested by Jani Nikula.
> >
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=36997
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/intel_sdvo.c |   31 +++++++++++++++++++------------
> >  1 file changed, 19 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> > index d85ebb0..cff3c0b 100644
> > --- a/drivers/gpu/drm/i915/intel_sdvo.c
> > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> > @@ -509,7 +509,7 @@ out:
> >  static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo,
> >  				     void *response, int response_len)
> >  {
> > -	u8 retry = 5;
> > +	u8 retry = 15; /* 5 quick checks, followed by 10 long checks */
> >  	u8 status;
> >  	int i;
> >  
> > @@ -522,14 +522,27 @@ static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo,
> >  	 * command to be complete.
> >  	 *
> >  	 * Check 5 times in case the hardware failed to read the docs.
> > +	 *
> > +	 * Also beware that the first response by many devices is to
> > +	 * reply PENDING and stall for time. TVs are notorious for
> > +	 * requiring longer than specified to complete their replies.
> > +	 * Originally (in the DDX long ago), the delay was only ever 15ms
> > +	 * with an additional delay of 30ms applied for TVs added later after
> > +	 * many experiments. To accommodate both sets of delays, we do a
> > +	 * sequence of slow checks if the device is falling behind and fails
> > +	 * to reply within 5*15µs.
> >  	 */
> >  	if (!intel_sdvo_read_byte(intel_sdvo,
> >  				  SDVO_I2C_CMD_STATUS,
> >  				  &status))
> >  		goto log_fail;
> >  
> > -	while (status == SDVO_CMD_STATUS_PENDING && retry--) {
> > -		udelay(15);
> > +	while (status == SDVO_CMD_STATUS_PENDING && --retry) {
> 
> Hey, why did you switch from post to pre decrement? It will now retry
> only retry-1 times. Or is this about the semantics of retries vs. tries?
> ;)

Because on the last go through, inside the loop retry would be 255 and
we would not get the final 15ms sleep.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH] drm/i915: Increase the response time for slow SDVO devices

[Jani Nikula]

On Fri, 23 Nov 2012, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Fri, 23 Nov 2012 14:21:58 +0200, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>> On Fri, 23 Nov 2012, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > Some devices may respond very slowly and only flag that the reply is
>> > pending within the first 15us response window. Be kind to such devices
>> > and wait a further 15ms, before checking for the pending reply. This
>> > moves the existing special case delay of 30ms down from the detection
>> > routine into the common path and pretends to explain it...
>> >
>> > v2: Simplify the loop constructs as suggested by Jani Nikula.
>> >
>> > References: https://bugs.freedesktop.org/show_bug.cgi?id=36997
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > ---
>> >  drivers/gpu/drm/i915/intel_sdvo.c |   31 +++++++++++++++++++------------
>> >  1 file changed, 19 insertions(+), 12 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
>> > index d85ebb0..cff3c0b 100644
>> > --- a/drivers/gpu/drm/i915/intel_sdvo.c
>> > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
>> > @@ -509,7 +509,7 @@ out:
>> >  static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo,
>> >  				     void *response, int response_len)
>> >  {
>> > -	u8 retry = 5;
>> > +	u8 retry = 15; /* 5 quick checks, followed by 10 long checks */
>> >  	u8 status;
>> >  	int i;
>> >  
>> > @@ -522,14 +522,27 @@ static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo,
>> >  	 * command to be complete.
>> >  	 *
>> >  	 * Check 5 times in case the hardware failed to read the docs.
>> > +	 *
>> > +	 * Also beware that the first response by many devices is to
>> > +	 * reply PENDING and stall for time. TVs are notorious for
>> > +	 * requiring longer than specified to complete their replies.
>> > +	 * Originally (in the DDX long ago), the delay was only ever 15ms
>> > +	 * with an additional delay of 30ms applied for TVs added later after
>> > +	 * many experiments. To accommodate both sets of delays, we do a
>> > +	 * sequence of slow checks if the device is falling behind and fails
>> > +	 * to reply within 5*15µs.
>> >  	 */
>> >  	if (!intel_sdvo_read_byte(intel_sdvo,
>> >  				  SDVO_I2C_CMD_STATUS,
>> >  				  &status))
>> >  		goto log_fail;
>> >  
>> > -	while (status == SDVO_CMD_STATUS_PENDING && retry--) {
>> > -		udelay(15);
>> > +	while (status == SDVO_CMD_STATUS_PENDING && --retry) {
>> 
>> Hey, why did you switch from post to pre decrement? It will now retry
>> only retry-1 times. Or is this about the semantics of retries vs. tries?
>> ;)
>
> Because on the last go through, inside the loop retry would be 255 and
> we would not get the final 15ms sleep.

Right. The r-b applies.

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

Re: [PATCH] drm/i915: Increase the response time for slow SDVO devices

[Daniel Vetter]

On Fri, Nov 23, 2012 at 11:57:56AM +0000, Chris Wilson wrote:
> Some devices may respond very slowly and only flag that the reply is
> pending within the first 15us response window. Be kind to such devices
> and wait a further 15ms, before checking for the pending reply. This
> moves the existing special case delay of 30ms down from the detection
> routine into the common path and pretends to explain it...
> 
> v2: Simplify the loop constructs as suggested by Jani Nikula.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=36997
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch