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

[Paulo Zanoni]

2015-04-01 16:45 GMT-03:00 Todd Previte <tprevite at 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 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.
>

I agree with Ville here. See below.

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

Before this patch, we would check the bit and then run "continue",
regardless of the state of DP_AUX_CH_CTL_DONE. With your patch, we
don't check for the _TIME_OUT bit, which means that if both _TIME_OUT
and _CTL_DONE bits are set, we will "break" instead of the old
behavior. I think Ville's point is that we should probably continue
with the old behavior, especially since this patch is just about
adding a new sleep call, and not the specific interaction of _TIME_OUT
and _CTL_DONE.


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

I'm not sure adding a log message here is a good idea: we're probably
going to flood dmesg on a case that is somewhat expected and
recoverable. We already print an error message in case we finish the
loop without _CTL_DONE set, so I think we're covered regarding error
printing already: just run "continue" without logging anything since
we're going to retry anyway.


>
> 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 at lists.freedesktop.org
>>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni