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