> -----Original Message-----
> From: Deak, Imre <imre.deak@xxxxxxxxx>
> Sent: Monday, June 12, 2023 5:07 PM
> To: Murthy, Arun R <arun.r.murthy@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCHv2] drm/i915/display/dp: On AUX xfer timeout
> restart freshly
>
> On Thu, Jun 08, 2023 at 03:02:18PM +0530, Arun R Murthy wrote:
> > At the begining of the aux transfer a check for aux control busy bit
> > is done. Then as per the spec on aux transfer timeout, need to retry
> > freshly for 3 times with a delay which is taken care by the control
> > register.
> > On each of these 3 trials a check for busy has to be done so as to
> > start freshly.
> >
> > v2: updated the commit message
> >
> > Signed-off-by: Arun R Murthy <arun.r.murthy@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_dp_aux.c | 50
> > +++++++++------------
> > 1 file changed, 22 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c
> > b/drivers/gpu/drm/i915/display/intel_dp_aux.c
> > index 197c6e81db14..25090542dd9f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c
> > @@ -273,30 +273,6 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
> > * it using the same AUX CH simultaneously
> > */
> >
> > - /* Try to wait for any previous AUX channel activity */
> > - for (try = 0; try < 3; try++) {
> > - status = intel_de_read_notrace(i915, ch_ctl);
> > - if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0)
> > - break;
> > - msleep(1);
> > - }
> > - /* just trace the final value */
> > - trace_i915_reg_rw(false, ch_ctl, status, sizeof(status), true);
> > -
> > - if (try == 3) {
> > - const u32 status = intel_de_read(i915, ch_ctl);
> > -
> > - if (status != intel_dp->aux_busy_last_status) {
> > - drm_WARN(&i915->drm, 1,
> > - "%s: not started (status 0x%08x)\n",
> > - intel_dp->aux.name, status);
> > - intel_dp->aux_busy_last_status = status;
> > - }
> > -
> > - ret = -EBUSY;
> > - goto out;
> > - }
> > -
> > /* Only 5 data registers! */
> > if (drm_WARN_ON(&i915->drm, send_bytes > 20 || recv_size > 20)) {
> > ret = -E2BIG;
> > @@ -304,14 +280,31 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
> > }
> >
> > while ((aux_clock_divider = intel_dp-
> >get_aux_clock_divider(intel_dp, clock++))) {
> > - u32 send_ctl = intel_dp->get_aux_send_ctl(intel_dp,
> > + /* Must try at least 3 times according to DP spec */
> > + for (try = 0; try < 5; try++) {
> > + u32 send_ctl = intel_dp->get_aux_send_ctl(intel_dp,
> > send_bytes,
> > aux_clock_divider);
> >
> > - send_ctl |= aux_send_ctl_flags;
> > + send_ctl |= aux_send_ctl_flags;
>
> Why is send_ctl recomputed in each iteration?
This can be moved outside the loop, since the value doesn't tend to change.
>
> > +
> > + /* Try to wait for any previous AUX channel activity
> */
> > + status = intel_dp_aux_wait_done(intel_dp);
>
> How does it help to wait here for the same condition that was already waited
> for at the end of the loop?
Here we are checking for busy bit so as to ensure that the previous transfer is complete and only then the new transfer is initiated.
In the latter case, we sent the data and then wait to get the acknowledgement(busy/error/timeout).
Check for busy is to be done before sending the data. Here it was done before this loop.
The spec limits the trials for sending data to 3 in case of failure and in each of this iteration it has to be started freshly. So we need to ensure that the previous transfer is completed before sending the new data.
Thanks and Regards,
Arun R Murthy
--------------------
>
> > + /* just trace the final value */
> > + trace_i915_reg_rw(false, ch_ctl, status, sizeof(status),
> true);
> > +
> > + if (status & DP_AUX_CH_CTL_SEND_BUSY) {
> > + drm_WARN(&i915->drm, 1,
> > + "%s: not started, previous Tx still in
> process (status 0x%08x)\n",
> > + intel_dp->aux.name, status);
> > + intel_dp->aux_busy_last_status = status;
> > + if (try > 3) {
> > + ret = -EBUSY;
> > + goto out;
> > + } else
> > + continue;
> > + }
> >
> > - /* Must try at least 3 times according to DP spec */
> > - for (try = 0; try < 5; try++) {
> > /* Load the send data into the aux channel data
> registers */
> > for (i = 0; i < send_bytes; i += 4)
> > intel_de_write(i915, ch_data[i >> 2], @@ -
> 321,6 +314,7 @@
> > intel_dp_aux_xfer(struct intel_dp *intel_dp,
> > /* Send the command and wait for it to complete */
> > intel_de_write(i915, ch_ctl, send_ctl);
> >
> > + /* TODO: if typeC then 4.2ms else 800us. For DG2
> add 1.5ms for
> > +both cases */
> > status = intel_dp_aux_wait_done(intel_dp);
> >
> > /* Clear done status and any errors */
> > --
> > 2.25.1
> >
