[Intel-gfx] [PATCH 3/9] drm/i915: Add a delay in Displayport AUX transactions for compliance testing

[Ville Syrjälä]

On Wed, Apr 01, 2015 at 11:55:44AM -0700, Todd Previte wrote:
> 
> 
> On 4/1/2015 11:23 AM, Ville Syrjälä wrote:
> > On Tue, Mar 31, 2015 at 10:15:00AM -0700, Todd Previte wrote:
> >> The Displayport Link Layer Compliance Testing Specification 1.2 rev 1.1
> >> specifies that repeated AUX transactions after a failure (no response /
> >> invalid response) must have a minimum delay of 400us before the resend can
> >> occur. Tests 4.2.1.1 and 4.2.1.2 are two tests that require this specifically.
> >>
> >> V2:
> >> - Changed udelay() to usleep_range()
> >> V3:
> >> - Removed extraneous check for timeout
> >> - Updated comment to reflect this change
> >>
> >> Signed-off-by: Todd Previte <tprevite at gmail.com>
> >> ---
> >>   drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++--
> >>   1 file changed, 8 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> index ed2f60c..dc87276 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> @@ -877,9 +877,15 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> >>   				   DP_AUX_CH_CTL_TIME_OUT_ERROR |
> >>   				   DP_AUX_CH_CTL_RECEIVE_ERROR);
> >>   
> >> -			if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR |
> >> -				      DP_AUX_CH_CTL_RECEIVE_ERROR))
> >> +			/* DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2
> >> +			     400us delay required for errors and timeouts
> >> +			     Timeout errors from the HW already meet this
> >> +			     requirement so skip to next iteration
> >> +			*/
> > Weird format for multi line comment.
> Yeah I had to squish it in there to keep it under 80 columns. Needs the 
> '*' on the left side too though. I'll fix that and repost.
> >
> >> +			if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) {
> >> +				usleep_range(400, 500);
> >>   				continue;
> >> +			}
> > Where did the DP_AUX_CH_CTL_TIME_OUT_ERROR handling go?
> As I recall, previous review feedback indicated that the timeout 
> condition there was already accounted for.
> 
> on 12/15, Paulo commented:
> 
> One thing to notice is that if we get a TIME_OUT_ERROR I guess it
> means we already waited our standard timeout (which is either 400, 600
> or 1600, depending on the platform), so shouldn't we just do the
> usleep() after the RECEIVE_ERROR error?
> 
> 
> When I checked the BSpec, that seemed to be the case so I removed the 
> TIME_OUT_ERROR. Without this
> code in place, we still pass the compliance tests for AUX transactions, 
> one of which is for a no-reply transaction.
> That case specifically should hit the TIME_OUT_ERROR if it was going to 
> occur, I would think.
> 
> If you can give me a case where that becomes an issue, it's a simple fix 
> to add it back in there.

Not doing the usleep for the timeout error does seem sane enough to me,
but I didn't actually read through the specs to confirm that.

However my concern is that you no longer check the timeout error bit
at all inside the loop and instead just check the done bit even when the
timeout error may have happened. Now, I'm not sure both bits can actually
be set at the same time, but if they can't the original error check was
entirely redundant. So it would seem safer to keep checking both error
bits before the done bit.

> 
> >
> >>   			if (status & DP_AUX_CH_CTL_DONE)
> >>   				break;
> >>   		}
> >> -- 
> >> 1.9.1
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx at lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC