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

[Chris Wilson <chris@chris-wilson.co.uk>]

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