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

[Jani Nikula <jani.nikula@linux.intel.com>]

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