| Message ID | 20220704072352.1331682-1-arun.r.murthy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dp: wait on timeout before retry | expand |
> -----Original Message----- > From: Murthy, Arun R <arun.r.murthy@intel.com> > Sent: Monday, July 4, 2022 12:54 PM > To: intel-gfx@lists.freedesktop.org > Cc: ville.syrjala@linux.intel.com; Zanoni, Paulo R <paulo.r.zanoni@intel.com>; > tprevite@gmail.com; Shankar, Uma <uma.shankar@intel.com>; Nikula, Jani > <jani.nikula@intel.com>; Murthy, Arun R <arun.r.murthy@intel.com> > Subject: [PATCH] drm/i915/dp: wait on timeout before retry > > On linktraining error/timeout before retry need to wait for 400usec as per the DP > CTS spec1.2 Under section 2.7.2 AUX Transaction Response/Reply Time-outs AUX > Replier (the uPacket RX) must start sending the reply back to the AUX requester (the > uPacket TX) within the response period of 300μs. The timer for Response Time-out > starts ticking after the uPacket RX has finished receiving the AUX STOP condition > which ends the AUX Request transaction. > The timer is reset either when the Response Time-out period has elapsed or when > the uPacket RX has started to send the AUX Sync pattern (which follows > 10 to 16 active pre-charge pulses) for the Reply transaction. If the uPacket TX does > not receive a reply from the uPacket RX it must wait for a Reply Time-out period of > 400us before initiating the next AUX Request transaction. The timer for the Reply > Time-out starts ticking after the uPacket TX has finished sending the AUX STOP > condition. > > The patch with commit 74ebf294a1dd ("drm/i915: Add a delay in Displayport AUX > transactions for compliance testing") removes this delay mentioning the hardware > already meets this requirement, but as per the spec the delay mentioned in the spec > specifies how long to wait for the receiver response before timeout. So the delay > here to wait for timeout and not a delay after timeout. The DP spec specifies a delay > after timeout and hence adding this delay. > > v2: fixed checkpatch warning and error > v3: used proper indentation > v4: added DP CEA 1.2 spec details in patch commit msg Looks Good to me. Reviewed-by: Uma Shankar <uma.shankar@intel.com> > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > Acked-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp_aux.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c > b/drivers/gpu/drm/i915/display/intel_dp_aux.c > index 2bc119374555..722c9f210690 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c > @@ -286,13 +286,9 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp, > /* > * 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_TIME_OUT_ERROR) > - continue; > - > - if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) { > + if (status & (DP_AUX_CH_CTL_RECEIVE_ERROR | > + DP_AUX_CH_CTL_TIME_OUT_ERROR)) { > usleep_range(400, 500); > continue; > } > -- > 2.25.1
On Mon, Jul 04, 2022 at 12:53:52PM +0530, Arun R Murthy wrote: > On linktraining error/timeout before retry need to wait for 400usec as > per the DP CTS spec1.2 s/CTS// > Under section 2.7.2 AUX Transaction Response/Reply Time-outs > AUX Replier (the uPacket RX) must start sending the reply back to the AUX > requester (the uPacket TX) within the response period of 300μs. The timer > for Response Time-out starts ticking after the uPacket RX has finished > receiving the AUX STOP condition which ends the AUX Request transaction. > The timer is reset either when the Response Time-out period has elapsed or > when the uPacket RX has started to send the AUX Sync pattern (which follows > 10 to 16 active pre-charge pulses) for the Reply transaction. If the > uPacket TX does not receive a reply from the uPacket RX it must wait for a > Reply Time-out period of 400us before initiating the next AUX Request > transaction. The timer for the Reply Time-out starts ticking after the > uPacket TX has finished sending the AUX STOP condition. > > The patch with commit 74ebf294a1dd ("drm/i915: Add a delay in Displayport > AUX transactions for compliance testing") > removes this delay mentioning the hardware already meets this requirement, > but as per the spec the delay mentioned in the spec specifies how long to > wait for the receiver response before timeout. So the delay here to wait > for timeout and not a delay after timeout. The DP spec specifies a delay > after timeout and hence adding this delay. Not sure what you're saying here. The spec states the reply timeout should start counting once the TX has sent the AUX STOP, and gets reset when the reply AUX SYNC is detected. If that doesn't match what the hardware is doing then we really need to get bspec updated to say what is actually happening. Oh, and the reply timeout has been increased to 3.2ms in later revisions of the spec to deal with LTTPRs. We should adjust the code to match. > > v2: fixed checkpatch warning and error > v3: used proper indentation > v4: added DP CEA 1.2 spec details in patch commit msg > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > Acked-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp_aux.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c b/drivers/gpu/drm/i915/display/intel_dp_aux.c > index 2bc119374555..722c9f210690 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c > @@ -286,13 +286,9 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp, > /* > * 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_TIME_OUT_ERROR) > - continue; > - > - if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) { > + if (status & (DP_AUX_CH_CTL_RECEIVE_ERROR | > + DP_AUX_CH_CTL_TIME_OUT_ERROR)) { > usleep_range(400, 500); > continue; > } > -- > 2.25.1
> On Mon, Jul 04, 2022 at 12:53:52PM +0530, Arun R Murthy wrote: > > On linktraining error/timeout before retry need to wait for 400usec as > > per the DP CTS spec1.2 > > s/CTS// > > > Under section 2.7.2 AUX Transaction Response/Reply Time-outs AUX > > Replier (the uPacket RX) must start sending the reply back to the AUX > > requester (the uPacket TX) within the response period of 300μs. The > > timer for Response Time-out starts ticking after the uPacket RX has > > finished receiving the AUX STOP condition which ends the AUX Request > transaction. > > The timer is reset either when the Response Time-out period has > > elapsed or when the uPacket RX has started to send the AUX Sync > > pattern (which follows > > 10 to 16 active pre-charge pulses) for the Reply transaction. If the > > uPacket TX does not receive a reply from the uPacket RX it must wait > > for a Reply Time-out period of 400us before initiating the next AUX > > Request transaction. The timer for the Reply Time-out starts ticking > > after the uPacket TX has finished sending the AUX STOP condition. > > > > The patch with commit 74ebf294a1dd ("drm/i915: Add a delay in > > Displayport AUX transactions for compliance testing") removes this > > delay mentioning the hardware already meets this requirement, but as > > per the spec the delay mentioned in the spec specifies how long to > > wait for the receiver response before timeout. So the delay here to > > wait for timeout and not a delay after timeout. The DP spec specifies > > a delay after timeout and hence adding this delay. > > Not sure what you're saying here. The spec states the reply timeout should > start counting once the TX has sent the AUX STOP, and gets reset when the > reply AUX SYNC is detected. > Copying the above said spec for quick reference "AUX Replier (the uPacket RX) must start sending the reply back to the AUX requester (the uPacket TX) within the response period of 300μs. The timer for Response Time-out starts ticking after the uPacket RX has finished receiving the AUX STOP condition which ends the AUX Request transaction. The timer is reset either when the Response Time-out period has elapsed or when the uPacket RX has started to send the AUX Sync pattern" The timer that is reference in the 1st line above is 300usec. The line just above says this timer should be reset when the response time-out has elapsed, my understanding over here is this 300usec time should be reset on timeout. Now coming back to the last few lines of the spec referenced above, " If the uPacket TX does not receive a reply from the uPacket RX it must wait for a Reply Time-out period of 400us before initiating the next AUX Request transaction" My understanding on this is if the TX did not receive the reply, which means the timeout has occurred in that case before initiating a new request again, we should wait for 400usec. So in the above referenced part of the code, there are two timers involved. One is 300usec and the other is 400usec. The 300usec is the timer for hardware timeout to occur and the 400usec is the timer that the software has to wait on timeout/error before retry. Thanks and Regards, Arun R Murthy --------------------
> -----Original Message----- > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > Sent: Monday, July 4, 2022 7:41 PM > To: Murthy, Arun R <arun.r.murthy@intel.com> > Cc: intel-gfx@lists.freedesktop.org; Zanoni, Paulo R > <paulo.r.zanoni@intel.com>; tprevite@gmail.com; Shankar, Uma > <uma.shankar@intel.com>; Nikula, Jani <jani.nikula@intel.com> > Subject: Re: [PATCH] drm/i915/dp: wait on timeout before retry > > On Mon, Jul 04, 2022 at 12:53:52PM +0530, Arun R Murthy wrote: > > On linktraining error/timeout before retry need to wait for 400usec as > > per the DP CTS spec1.2 > > s/CTS// > > > Under section 2.7.2 AUX Transaction Response/Reply Time-outs AUX > > Replier (the uPacket RX) must start sending the reply back to the AUX > > requester (the uPacket TX) within the response period of 300μs. The > > timer for Response Time-out starts ticking after the uPacket RX has > > finished receiving the AUX STOP condition which ends the AUX Request > transaction. > > The timer is reset either when the Response Time-out period has > > elapsed or when the uPacket RX has started to send the AUX Sync > > pattern (which follows > > 10 to 16 active pre-charge pulses) for the Reply transaction. If the > > uPacket TX does not receive a reply from the uPacket RX it must wait > > for a Reply Time-out period of 400us before initiating the next AUX > > Request transaction. The timer for the Reply Time-out starts ticking > > after the uPacket TX has finished sending the AUX STOP condition. > > > > The patch with commit 74ebf294a1dd ("drm/i915: Add a delay in > > Displayport AUX transactions for compliance testing") removes this > > delay mentioning the hardware already meets this requirement, but as > > per the spec the delay mentioned in the spec specifies how long to > > wait for the receiver response before timeout. So the delay here to > > wait for timeout and not a delay after timeout. The DP spec specifies > > a delay after timeout and hence adding this delay. > > Not sure what you're saying here. The spec states the reply timeout should > start counting once the TX has sent the AUX STOP, and gets reset when the > reply AUX SYNC is detected. > > If that doesn't match what the hardware is doing then we really need to get > bspec updated to say what is actually happening. > > Oh, and the reply timeout has been increased to 3.2ms in later revisions of > the spec to deal with LTTPRs. We should adjust the code to match. > Will take this separately! Thanks and Regards, Arun R Murthy --------------------
> -----Original Message----- > From: Murthy, Arun R <arun.r.murthy@intel.com> > Sent: Monday, July 18, 2022 4:49 PM > To: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: intel-gfx@lists.freedesktop.org; Zanoni, Paulo R <paulo.r.zanoni@intel.com>; > tprevite@gmail.com; Shankar, Uma <uma.shankar@intel.com>; Nikula, Jani > <jani.nikula@intel.com> > Subject: RE: [PATCH] drm/i915/dp: wait on timeout before retry > > > -----Original Message----- > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Sent: Monday, July 4, 2022 7:41 PM > > To: Murthy, Arun R <arun.r.murthy@intel.com> > > Cc: intel-gfx@lists.freedesktop.org; Zanoni, Paulo R > > <paulo.r.zanoni@intel.com>; tprevite@gmail.com; Shankar, Uma > > <uma.shankar@intel.com>; Nikula, Jani <jani.nikula@intel.com> > > Subject: Re: [PATCH] drm/i915/dp: wait on timeout before retry > > > > On Mon, Jul 04, 2022 at 12:53:52PM +0530, Arun R Murthy wrote: > > > On linktraining error/timeout before retry need to wait for 400usec > > > as per the DP CTS spec1.2 > > > > s/CTS// > > > > > Under section 2.7.2 AUX Transaction Response/Reply Time-outs AUX > > > Replier (the uPacket RX) must start sending the reply back to the > > > AUX requester (the uPacket TX) within the response period of 300μs. > > > The timer for Response Time-out starts ticking after the uPacket RX > > > has finished receiving the AUX STOP condition which ends the AUX > > > Request > > transaction. > > > The timer is reset either when the Response Time-out period has > > > elapsed or when the uPacket RX has started to send the AUX Sync > > > pattern (which follows > > > 10 to 16 active pre-charge pulses) for the Reply transaction. If the > > > uPacket TX does not receive a reply from the uPacket RX it must wait > > > for a Reply Time-out period of 400us before initiating the next AUX > > > Request transaction. The timer for the Reply Time-out starts ticking > > > after the uPacket TX has finished sending the AUX STOP condition. > > > > > > The patch with commit 74ebf294a1dd ("drm/i915: Add a delay in > > > Displayport AUX transactions for compliance testing") removes this > > > delay mentioning the hardware already meets this requirement, but as > > > per the spec the delay mentioned in the spec specifies how long to > > > wait for the receiver response before timeout. So the delay here to > > > wait for timeout and not a delay after timeout. The DP spec > > > specifies a delay after timeout and hence adding this delay. > > > > Not sure what you're saying here. The spec states the reply timeout > > should start counting once the TX has sent the AUX STOP, and gets > > reset when the reply AUX SYNC is detected. > > > > If that doesn't match what the hardware is doing then we really need > > to get bspec updated to say what is actually happening. > > > > Oh, and the reply timeout has been increased to 3.2ms in later > > revisions of the spec to deal with LTTPRs. We should adjust the code to match. > > > Will take this separately! Hi Arun, I would suggest to create an issue to track the LTTPR work and validate. Also get the bspec updated to match the hardware behavior. With above done, we can take this patch to unblock multiple CI issues. @Ville Syrjälä Hope this is ok. Regards, Uma Shankar > Thanks and Regards, > Arun R Murthy > --------------------
On Mon, Jul 04, 2022 at 04:01:23PM +0000, Murthy, Arun R wrote: > > On Mon, Jul 04, 2022 at 12:53:52PM +0530, Arun R Murthy wrote: > > > On linktraining error/timeout before retry need to wait for 400usec as > > > per the DP CTS spec1.2 > > > > s/CTS// > > > > > Under section 2.7.2 AUX Transaction Response/Reply Time-outs AUX > > > Replier (the uPacket RX) must start sending the reply back to the AUX > > > requester (the uPacket TX) within the response period of 300μs. The > > > timer for Response Time-out starts ticking after the uPacket RX has > > > finished receiving the AUX STOP condition which ends the AUX Request > > transaction. > > > The timer is reset either when the Response Time-out period has > > > elapsed or when the uPacket RX has started to send the AUX Sync > > > pattern (which follows > > > 10 to 16 active pre-charge pulses) for the Reply transaction. If the > > > uPacket TX does not receive a reply from the uPacket RX it must wait > > > for a Reply Time-out period of 400us before initiating the next AUX > > > Request transaction. The timer for the Reply Time-out starts ticking > > > after the uPacket TX has finished sending the AUX STOP condition. > > > > > > The patch with commit 74ebf294a1dd ("drm/i915: Add a delay in > > > Displayport AUX transactions for compliance testing") removes this > > > delay mentioning the hardware already meets this requirement, but as > > > per the spec the delay mentioned in the spec specifies how long to > > > wait for the receiver response before timeout. So the delay here to > > > wait for timeout and not a delay after timeout. The DP spec specifies > > > a delay after timeout and hence adding this delay. > > > > Not sure what you're saying here. The spec states the reply timeout should > > start counting once the TX has sent the AUX STOP, and gets reset when the > > reply AUX SYNC is detected. > > > Copying the above said spec for quick reference "AUX Replier (the > uPacket RX) must start sending the reply back to the AUX requester > (the uPacket TX) within the response period of 300μs. The timer for > Response Time-out starts ticking after the uPacket RX has finished > receiving the AUX STOP condition which ends the AUX Request > transaction. The timer is reset either when the Response Time-out > period has elapsed or when the uPacket RX has started to send the AUX > Sync pattern" > > The timer that is reference in the 1st line above is 300usec. The line > just above says this timer should be reset when the response time-out > has elapsed, my understanding over here is this 300usec time should be > reset on timeout. > > Now coming back to the last few lines of the spec referenced above, " > If the uPacket TX does not receive a reply from the uPacket RX it must > wait for a Reply Time-out period of 400us before initiating the next > AUX Request transaction" My understanding on this is if the TX did not > receive the reply, which means the timeout has occurred in that case > before initiating a new request again, we should wait for 400usec. > > So in the above referenced part of the code, there are two timers > involved. One is 300usec and the other is 400usec. The 300usec is the > timer for hardware timeout to occur and the 400usec is the timer that > the software has to wait on timeout/error before retry. 300 usec is a timeout used by RX. After this expires RX should not transmit anything (rather it should switch to listen mode) to avoid a collision with TX. 400 usec is a timeout used by TX (starting from the last bit of the packet it transmitted) for the same reason. Based on the above not sure why an additional 400 usec delay would be needed after a timeout error (which took 400 usec at least). I suppose TX might incorrectly signal timeouts too early, this could be confirmed from the retransmit timestamps. > Thanks and Regards, > Arun R Murthy > --------------------
diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c b/drivers/gpu/drm/i915/display/intel_dp_aux.c index 2bc119374555..722c9f210690 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c @@ -286,13 +286,9 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp, /* * 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_TIME_OUT_ERROR) - continue; - - if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) { + if (status & (DP_AUX_CH_CTL_RECEIVE_ERROR | + DP_AUX_CH_CTL_TIME_OUT_ERROR)) { usleep_range(400, 500); continue; }
On linktraining error/timeout before retry need to wait for 400usec as per the DP CTS spec1.2 Under section 2.7.2 AUX Transaction Response/Reply Time-outs AUX Replier (the uPacket RX) must start sending the reply back to the AUX requester (the uPacket TX) within the response period of 300μs. The timer for Response Time-out starts ticking after the uPacket RX has finished receiving the AUX STOP condition which ends the AUX Request transaction. The timer is reset either when the Response Time-out period has elapsed or when the uPacket RX has started to send the AUX Sync pattern (which follows 10 to 16 active pre-charge pulses) for the Reply transaction. If the uPacket TX does not receive a reply from the uPacket RX it must wait for a Reply Time-out period of 400us before initiating the next AUX Request transaction. The timer for the Reply Time-out starts ticking after the uPacket TX has finished sending the AUX STOP condition. The patch with commit 74ebf294a1dd ("drm/i915: Add a delay in Displayport AUX transactions for compliance testing") removes this delay mentioning the hardware already meets this requirement, but as per the spec the delay mentioned in the spec specifies how long to wait for the receiver response before timeout. So the delay here to wait for timeout and not a delay after timeout. The DP spec specifies a delay after timeout and hence adding this delay. v2: fixed checkpatch warning and error v3: used proper indentation v4: added DP CEA 1.2 spec details in patch commit msg Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> Acked-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/i915/display/intel_dp_aux.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)