| Message ID | 20220701110309.1237002-1-arun.r.murthy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCHv3] drm/i915/dp: wait on timeout before retry | expand |
On Fri, 01 Jul 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 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 > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> Based on the description alone, Acked-by: Jani Nikula <jani.nikula@intel.com> but would be good if you could get someone to double check the details against bspec and CTS. > --- > 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; > }
> -----Original Message----- > From: Nikula, Jani <jani.nikula@intel.com> > Sent: Friday, July 1, 2022 4:45 PM > To: Murthy, Arun R <arun.r.murthy@intel.com>; 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>; Murthy, Arun R > <arun.r.murthy@intel.com> > Subject: Re: [PATCHv3] drm/i915/dp: wait on timeout before retry > > On Fri, 01 Jul 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 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. The source side as per bspec says how much time to wait for sink response before calling timeout: Reg: DDI_AUX_CTL, BitField: 27:26 Name: Time out timer value Description: Value Name 00b 400us (default) 01b 600us 10b 800us 11b 4000us So this should meet the CTS requirement. For any non-timeout error from sink, s/w should add explicit delay of minimum 400us before retry (which is being done) If 400us is not enough this needs to be checked separately or the timeout value in this register can be changed from default of 400us Regards, Uma Shankar > > > > v2: fixed checkpatch warning and error > > v3: used proper indentation > > > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > > Based on the description alone, > > Acked-by: Jani Nikula <jani.nikula@intel.com> > > but would be good if you could get someone to double check the details against > bspec and CTS. > > > --- > > 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; > > } > > -- > Jani Nikula, Intel Open Source Graphics Center
> > -----Original Message----- > > From: Nikula, Jani <jani.nikula@intel.com> > > Sent: Friday, July 1, 2022 4:45 PM > > To: Murthy, Arun R <arun.r.murthy@intel.com>; > > 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>; Murthy, Arun R <arun.r.murthy@intel.com> > > Subject: Re: [PATCHv3] drm/i915/dp: wait on timeout before retry > > > > On Fri, 01 Jul 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 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. > > The source side as per bspec says how much time to wait for sink response > before calling timeout: > Reg: DDI_AUX_CTL, BitField: 27:26 > Name: Time out timer value > Description: > ValueName > 00b400us (default) > 01b600us > 10b800us > 11b4000us > > So this should meet the CTS requirement. For any non-timeout error from > sink, s/w should add explicit delay of minimum 400us before retry (which is > being done) > > If 400us is not enough this needs to be checked separately or the timeout > value in this register can be changed from default of 400us > The delay specified in the register is the time to wait for triggering the timeout. This is being achieved in the register mentioned above. But what the CTS spec says is once the timeout has occurred, before retrying again we need to wait for 400ms. This is the delay that this patch has added. Thanks and Regards, Arun R Murthy --------------------
> -----Original Message----- > From: Murthy, Arun R <arun.r.murthy@intel.com> > Sent: Monday, July 4, 2022 11:07 AM > To: Shankar, Uma <uma.shankar@intel.com>; Nikula, Jani <jani.nikula@intel.com>; > intel-gfx@lists.freedesktop.org > Cc: ville.syrjala@linux.intel.com; Zanoni, Paulo R <paulo.r.zanoni@intel.com>; > tprevite@gmail.com > Subject: RE: [PATCHv3] drm/i915/dp: wait on timeout before retry > > > > -----Original Message----- > > > From: Nikula, Jani <jani.nikula@intel.com> > > > Sent: Friday, July 1, 2022 4:45 PM > > > To: Murthy, Arun R <arun.r.murthy@intel.com>; > > > 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>; Murthy, Arun R <arun.r.murthy@intel.com> > > > Subject: Re: [PATCHv3] drm/i915/dp: wait on timeout before retry > > > > > > On Fri, 01 Jul 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 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. > > > > The source side as per bspec says how much time to wait for sink > > response before calling timeout: > > Reg: DDI_AUX_CTL, BitField: 27:26 > > Name: Time out timer value > > Description: > > ValueName > > 00b400us (default) > > 01b600us > > 10b800us > > 11b4000us > > > > So this should meet the CTS requirement. For any non-timeout error > > from sink, s/w should add explicit delay of minimum 400us before retry > > (which is being done) > > > > If 400us is not enough this needs to be checked separately or the > > timeout value in this register can be changed from default of 400us > > > The delay specified in the register is the time to wait for triggering the timeout. This > is being achieved in the register mentioned above. > > But what the CTS spec says is once the timeout has occurred, before retrying again > we need to wait for 400ms. This is the delay that this patch has added. Thanks Arun for clarification. Seems there is some confusion with the hardware wait and this retry wait. I would suggest to add the CTS spec details clarifying this wait and we should be good. > > 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 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 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(-)