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