| Message ID | 20230613044239.691706-1-arun.r.murthy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCHv3] drm/i915/display/dp: On AUX xfer timeout restart freshly | expand |
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 <arun.r.murthy@intel.com> > --- > .../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. > + ret = intel_dp_aux_wait_done(intel_dp, &status); > + > + /* On timeout dont read the status bits as its not updated */ > + if (ret == -ETIMEDOUT) > + continue; > > /* Clear done status and any errors */ > intel_de_write(i915, ch_ctl, > -- > 2.25.1 >
> -----Original Message----- > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Arun R Murthy > Sent: Tuesday, June 13, 2023 7:43 AM > To: intel-gfx@lists.freedesktop.org > Cc: Nikula, Jani <jani.nikula@intel.com> > Subject: [Intel-gfx] [PATCHv3] drm/i915/display/dp: On AUX xfer timeout restart freshly > > 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 <arun.r.murthy@intel.com> > --- > .../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 | > - 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++) { It seems that we try 5 times here. Was this intention or keep tries in 3 as commit message/comment would suggest? -Mika- > + 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); > + ret = intel_dp_aux_wait_done(intel_dp, &status); > + > + /* On timeout dont read the status bits as its not updated */ > + if (ret == -ETIMEDOUT) > + continue; > > /* Clear done status and any errors */ > intel_de_write(i915, ch_ctl, > -- > 2.25.1
> -----Original Message----- > From: Kahola, Mika <mika.kahola@intel.com> > Sent: Tuesday, June 13, 2023 6:02 PM > To: Murthy, Arun R <arun.r.murthy@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: Nikula, Jani <jani.nikula@intel.com> > Subject: RE: [Intel-gfx] [PATCHv3] drm/i915/display/dp: On AUX xfer timeout > restart freshly > > > -----Original Message----- > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of > > Arun R Murthy > > Sent: Tuesday, June 13, 2023 7:43 AM > > To: intel-gfx@lists.freedesktop.org > > Cc: Nikula, Jani <jani.nikula@intel.com> > > Subject: [Intel-gfx] [PATCHv3] drm/i915/display/dp: On AUX xfer > > timeout restart freshly > > > > 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 <arun.r.murthy@intel.com> > > --- > > .../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 | > > - 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++) { > It seems that we try 5 times here. Was this intention or keep tries in 3 as > commit message/comment would suggest? > Will change the comment. As per the spec yes its 3 times, but with software overhead we have been trying this retry for 5 times. Thanks and Regards, Arun R Murthy -------------------- > -Mika- > > + 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); > > + ret = intel_dp_aux_wait_done(intel_dp, &status); > > + > > + /* On timeout dont read the status bits as its not > updated */ > > + if (ret == -ETIMEDOUT) > > + continue; > > > > /* Clear done status and any errors */ > > intel_de_write(i915, ch_ctl, > > -- > > 2.25.1
> -----Original Message----- > From: Deak, Imre <imre.deak@intel.com> > Sent: Tuesday, June 13, 2023 4:19 PM > To: Murthy, Arun R <arun.r.murthy@intel.com> > Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com> > 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 <arun.r.murthy@intel.com> > > --- > > .../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 --------------------
On Tue, Jun 20, 2023 at 09:26:28AM +0300, Murthy, Arun R wrote: > > -----Original Message----- > > From: Deak, Imre <imre.deak@intel.com> > > Sent: Tuesday, June 13, 2023 4:19 PM > > To: Murthy, Arun R <arun.r.murthy@intel.com> > > Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com> > > 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 <arun.r.murthy@intel.com> > > > --- > > > .../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. There is already a check in the current code for SEND_BUSY before the first transaction attempt. Any subsequent error handling, including checking for SEND_BUSY being still set, should be done at one place after waiting for the transaction to complete (along with TIME_OUT_ERROR and RECEIVE_ERROR checks). I don't see a reason to repeat the call to intel_dp_aux_wait_done() as in this patch, right after it has already timed out. > Will be floating v6 patch which takes care of this SEND_BUSY and acts > upon properly. > > Thanks and Regards, > Arun R Murthy > --------------------
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 | - 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); + ret = intel_dp_aux_wait_done(intel_dp, &status); + + /* On timeout dont read the status bits as its not updated */ + if (ret == -ETIMEDOUT) + continue; /* Clear done status and any errors */ intel_de_write(i915, ch_ctl,
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 <arun.r.murthy@intel.com> --- .../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(-)