| Message ID | 1427822106-29617-4-git-send-email-tprevite@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > + 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? > 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
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. > >> 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
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. > > > > >> 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
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
2015-04-01 16:45 GMT-03:00 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. > 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@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
On 4/6/15 4:28 PM, Paulo Zanoni wrote: > 2015-04-01 16:45 GMT-03:00 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. > 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@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 > >
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 + */ + if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) { + usleep_range(400, 500); continue; + } if (status & DP_AUX_CH_CTL_DONE) break; }
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(-)