> -----Original Message----- > From: Deak, Imre <[email protected]> > Sent: Tuesday, June 13, 2023 4:19 PM > To: Murthy, Arun R <[email protected]> > Cc: [email protected]; Nikula, Jani <[email protected]> > Subject: Re: [PATCHv3] drm/i915/display/dp: On AUX xfer timeout restart > freshly > > On Tue, Jun 13, 2023 at 10:12:39AM +0530, Arun R Murthy wrote: > > On AUX transfer timeout, as per DP spec need to retry for 3 times and > > has to be restarted freshly. > > > > v3: handle timeout and dont rely on register value on timeout (Imre) > > > > Signed-off-by: Arun R Murthy <[email protected]> > > --- > > .../drm/i915/display/intel_display_types.h | 1 - > > drivers/gpu/drm/i915/display/intel_dp_aux.c | 72 +++++++++---------- > > 2 files changed, 34 insertions(+), 39 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > > b/drivers/gpu/drm/i915/display/intel_display_types.h > > index 2d8297f8d088..0942b109b4ca 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > @@ -1741,7 +1741,6 @@ struct intel_dp { > > /* sink or branch descriptor */ > > struct drm_dp_desc desc; > > struct drm_dp_aux aux; > > - u32 aux_busy_last_status; > > u8 train_set[4]; > > > > struct intel_pps pps; > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c > > b/drivers/gpu/drm/i915/display/intel_dp_aux.c > > index 0c27db8ae4f1..244b4d7d716d 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c > > @@ -36,25 +36,24 @@ static void intel_dp_aux_unpack(u32 src, u8 *dst, > int dst_bytes) > > dst[i] = src >> ((3 - i) * 8); > > } > > > > -static u32 > > -intel_dp_aux_wait_done(struct intel_dp *intel_dp) > > +static int > > +intel_dp_aux_wait_done(struct intel_dp *intel_dp, u32 *status) > > { > > struct drm_i915_private *i915 = dp_to_i915(intel_dp); > > i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp); > > const unsigned int timeout_ms = 10; > > - u32 status; > > int ret; > > > > ret = __intel_de_wait_for_register(i915, ch_ctl, > > DP_AUX_CH_CTL_SEND_BUSY, 0, > > - 2, timeout_ms, &status); > > + 50, timeout_ms, status); > > > > if (ret == -ETIMEDOUT) > > drm_err(&i915->drm, > > "%s: did not complete or timeout within %ums > (status 0x%08x)\n", > > - intel_dp->aux.name, timeout_ms, status); > > + intel_dp->aux.name, timeout_ms, *status); > > > > - return status; > > + return ret; > > } > > > > static u32 g4x_get_aux_clock_divider(struct intel_dp *intel_dp, int > > index) @@ -186,10 +185,7 @@ static u32 skl_get_aux_send_ctl(struct > intel_dp *intel_dp, > > */ > > ret = DP_AUX_CH_CTL_SEND_BUSY | > > DP_AUX_CH_CTL_DONE | > > - DP_AUX_CH_CTL_INTERRUPT | > > The above one-liner change, also applied in g4x_get_aux_send_ctl() should > be a separate patch. > > > - DP_AUX_CH_CTL_TIME_OUT_ERROR | > > DP_AUX_CH_CTL_TIME_OUT_MAX | > > - DP_AUX_CH_CTL_RECEIVE_ERROR | > > DP_AUX_CH_CTL_MESSAGE_SIZE(send_bytes) | > > > DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(intel_dp_aux_fw_sync_len()) > | > > > DP_AUX_CH_CTL_SYNC_PULSE_SKL(intel_dp_aux_sync_len()); > > @@ -273,30 +269,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 +276,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; > > + > > + /* Try to wait for any previous AUX channel activity > */ > > + /* TODO: if typeC then 4.2ms else 800us. For DG2 > add 1.5ms for both cases */ > > + ret = intel_dp_aux_wait_done(intel_dp, &status); > > + /* just trace the final value */ > > + trace_i915_reg_rw(false, ch_ctl, status, sizeof(status), > true); > > + > > + /* On timeout dont read the status bits as its not > updated */ > > + if (ret == -ETIMEDOUT) { > > + drm_WARN(&i915->drm, 1, > > + "%s: not started, previous Tx still in > process (status 0x%08x)\n", > > + intel_dp->aux.name, status); > > + if (try > 3) > > + 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], @@ - > 320,8 +309,15 @@ > > 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); > > + intel_de_rmw(i915, ch_ctl, > DP_AUX_CH_CTL_INTERRUPT | > > + DP_AUX_CH_CTL_TIME_OUT_ERROR | > > + DP_AUX_CH_CTL_RECEIVE_ERROR, 0); > > > > - status = intel_dp_aux_wait_done(intel_dp); > > The review comments were not addressed and these changes still don't > make sense to me. What I meant is - instead of all the above changes - > bailing out at this point if DP_AUX_CH_CTL_SEND_BUSY is still set in status. >
I think my 2st patch version had considered this SEND_BUSY bit. In the beginning of this loop there is a check for SEND_BUSY to ensure that the previous aux transaction is complete. Then we write the data and here we check for error(timeout or receive) only. If SEND_BUSY is checked over here again the code for aux transaction in progress check will be duplicated. Will be floating v6 patch which takes care of this SEND_BUSY and acts upon properly. Thanks and Regards, Arun R Murthy --------------------
