| Message ID | 20220627105939.657782-1-arun.r.murthy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dp: wait on timeout before retry | expand |
On Mon, 27 Jun 2022, Arun R Murthy <arun.r.murthy@intel.com> wrote: > On linktraining error/timeout before retry need to wait for 400usec as > per the DP CTS spec1.2 > The patch with commit id > 74ebf294a1dd816bdc248ac733009a8915d59eb5 > drm/i915: Add a delay in Displayport AUX transactions for > compliance testing Please reference other commits like this: commit 74ebf294a1dd ("drm/i915: Add a delay in Displayport AUX transactions for compliance testing") I've got this git alias in my .gitconfig: cite = "!f() { git log -1 '--pretty=format:%H (\"%s\")%n' $1 | sed -e 's/\\([0-f]\\{12\\}\\)[0-f]*/\\1/'; }; f" And you can use 'git cite <commit-ish>' to give you the right format. > 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. Please try to reflow commit messages to about 72 characters. I had a bit of a hard time parsing the above, but basically you're saying that the hardware handles and flags the timeout, but the DP spec says we need to wait 400 us *after* the error condition. Makes sense, though I didn't have the time to read the specs to verify. > > Signed-off-by: Arun R Murthy <arun.r.murthy@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..a1fef1645d6a 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)) { Please check the indentation here. The DP_AUX_CH_* should start in the same column. BR, Jani. > usleep_range(400, 500); > continue; > }
> On Mon, 27 Jun 2022, Arun R Murthy <arun.r.murthy@intel.com> wrote: > > On linktraining error/timeout before retry need to wait for 400usec as > > per the DP CTS spec1.2 The patch with commit id > > 74ebf294a1dd816bdc248ac733009a8915d59eb5 > > drm/i915: Add a delay in Displayport AUX transactions for compliance > > testing > > Please reference other commits like this: commit 74ebf294a1dd > ("drm/i915: Add a delay in Displayport AUX transactions for compliance > testing") > > I've got this git alias in my .gitconfig: > > cite = "!f() { git log -1 '--pretty=format:%H (\"%s\")%n' $1 | sed -e > 's/\\([0-f]\\{12\\}\\)[0-f]*/\\1/'; }; f" > > And you can use 'git cite <commit-ish>' to give you the right format. > Will correct this! > > 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. > > Please try to reflow commit messages to about 72 characters. > Commit msg is maintained to be 72 char, but the patch details added might be misleading. Will correct them! > I had a bit of a hard time parsing the above, but basically you're saying that > the hardware handles and flags the timeout, but the DP spec says we need to > wait 400 us *after* the error condition. Makes sense, though I didn't have > the time to read the specs to verify. > > > > > Signed-off-by: Arun R Murthy <arun.r.murthy@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..a1fef1645d6a 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)) { > > Please check the indentation here. The DP_AUX_CH_* should start in the > same column. > Done! 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..a1fef1645d6a 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 The patch with commit id 74ebf294a1dd816bdc248ac733009a8915d59eb5 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. Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> --- drivers/gpu/drm/i915/display/intel_dp_aux.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)