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

[Todd Previte <tprevite@gmail.com>]

On 4/1/2015 12:22 PM, Ville Syrjälä wrote:
> 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@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.

While there's no harm in checking the timeout bit here, does it really 
make sense to do so unless we're
going to take action on it? As you said, it seems reasonable enough to 
not wait an addition 400-500us,
so is there something else to do? It may be worth logging the error just 
to make sure there's some
record of when it happens, even if we're not going to do anything else.

As for exclusion between the two bits, the BSpec makes no indication 
that they're exclusive of one another. So
it's entirely possible to have both set.

>>>>    			if (status & DP_AUX_CH_CTL_DONE)
>>>>    				break;
>>>>    		}
>>>> -- 
>>>> 1.9.1
>>>>
>>>> _______________________________________________
>>>> 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