diff mbox

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

Message ID 874nkgh7f5.fsf@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jani Nikula Nov. 23, 2012, 11:42 a.m. UTC 
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:


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

Comments

Chris Wilson Nov. 23, 2012, 11:47 a.m. UTC | #1 
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
diff mbox

Patch

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))