[Intel-gfx] [PATCH 3/9] drm/i915: Add a delay in Displayport AUX transactions for compliance testing
Todd Previte
tprevite at gmail.com
Mon Apr 6 19:07:29 PDT 2015
On 4/6/15 4:28 PM, Paulo Zanoni wrote:
> 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.
I see what you're saying. That was an oversight on my part. Thanks for
catching it. Updated patch will be here shortly.
>
>> 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.
Fair enough, although I'm not sure I'd agree that it's a somewhat
expected case. It's certainly recoverable though. I'll remove the
message to avoid possible log spam.
>
>> 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
>
>
More information about the Intel-gfx
mailing list