| Message ID | 20221214032955.963806-1-arun.r.murthy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCHv4] drm/i915/dp: change aux_ctl reg read to polling read | expand |
> -----Original Message----- > From: Murthy, Arun R <arun.r.murthy@intel.com> > Sent: Wednesday, December 14, 2022 9:00 AM > To: intel-gfx@lists.freedesktop.org; ville.syrjala@linux.intel.com; Nikula, Jani > <jani.nikula@intel.com>; Deak, Imre <imre.deak@intel.com> > Cc: Murthy, Arun R <arun.r.murthy@intel.com> > Subject: [PATCHv4] drm/i915/dp: change aux_ctl reg read to polling read > > The busy timeout logic checks for the AUX BUSY, then waits for the timeout > period and then after timeout reads the register for BUSY or Success. > Instead replace interrupt with polling so as to read the AUX CTL register often > before the timeout period. Looks like there might be some issue with > interrupt-on-read. Hence changing the logic to polling read. > > v2: replace interrupt with polling read > v3: use usleep_rang instead of msleep, updated commit msg > v4: use intel_wait_for_regiter internal function(Jani N) > BAT is failing with using this intel_wait_for_register/intel_de_wait_for_register, this function does something more than required. Thanks and Regards, Arun R Murthy --------------------- > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp_aux.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c > b/drivers/gpu/drm/i915/display/intel_dp_aux.c > index 91c93c93e5fc..fe5ed432a66a 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c > @@ -41,20 +41,20 @@ intel_dp_aux_wait_done(struct intel_dp *intel_dp) > i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp); > const unsigned int timeout_ms = 10; > u32 status; > - bool done; > > -#define C (((status = intel_de_read_notrace(i915, ch_ctl)) & > DP_AUX_CH_CTL_SEND_BUSY) == 0) > - done = wait_event_timeout(i915->display.gmbus.wait_queue, C, > - msecs_to_jiffies_timeout(timeout_ms)); > + __intel_wait_for_register(&i915->uncore, ch_ctl, > + DP_AUX_CH_CTL_SEND_BUSY, > + DP_AUX_CH_CTL_SEND_BUSY, > + 500, timeout_ms, &status); > > - /* just trace the final value */ > - trace_i915_reg_rw(false, ch_ctl, status, sizeof(status), true); > - > - if (!done) > + status = intel_uncore_read_notrace(&i915->uncore, ch_ctl); > + if ((status & DP_AUX_CH_CTL_SEND_BUSY) != 0) > drm_err(&i915->drm, > "%s: did not complete or timeout within %ums > (status 0x%08x)\n", > intel_dp->aux.name, timeout_ms, status); -#undef C > + > + /* just trace the final value */ > + trace_i915_reg_rw(false, ch_ctl, status, sizeof(status), true); > > return status; > } > -- > 2.25.1
On Wed, 14 Dec 2022, Arun R Murthy <arun.r.murthy@intel.com> wrote: > The busy timeout logic checks for the AUX BUSY, then waits for the > timeout period and then after timeout reads the register for BUSY or > Success. > Instead replace interrupt with polling so as to read the AUX CTL > register often before the timeout period. Looks like there might be some > issue with interrupt-on-read. Hence changing the logic to polling read. > > v2: replace interrupt with polling read > v3: use usleep_rang instead of msleep, updated commit msg > v4: use intel_wait_for_regiter internal function > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp_aux.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c b/drivers/gpu/drm/i915/display/intel_dp_aux.c > index 91c93c93e5fc..fe5ed432a66a 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c > @@ -41,20 +41,20 @@ intel_dp_aux_wait_done(struct intel_dp *intel_dp) > i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp); > const unsigned int timeout_ms = 10; > u32 status; > - bool done; > > -#define C (((status = intel_de_read_notrace(i915, ch_ctl)) & DP_AUX_CH_CTL_SEND_BUSY) == 0) > - done = wait_event_timeout(i915->display.gmbus.wait_queue, C, > - msecs_to_jiffies_timeout(timeout_ms)); > + __intel_wait_for_register(&i915->uncore, ch_ctl, > + DP_AUX_CH_CTL_SEND_BUSY, > + DP_AUX_CH_CTL_SEND_BUSY, > + 500, timeout_ms, &status); This waits for DP_AUX_CH_CTL_SEND_BUSY to be set, not cleared. Please use intel_de_wait_for_register(). > > - /* just trace the final value */ > - trace_i915_reg_rw(false, ch_ctl, status, sizeof(status), true); > - > - if (!done) > + status = intel_uncore_read_notrace(&i915->uncore, ch_ctl); > + if ((status & DP_AUX_CH_CTL_SEND_BUSY) != 0) Please check intel_de_wait_for_register()'s return value here instead of reading the register once more. You might read the register the register again just to log the value, if intel_de_wait_for_register() returned != 0. But no need to trace log that again. > drm_err(&i915->drm, > "%s: did not complete or timeout within %ums (status 0x%08x)\n", > intel_dp->aux.name, timeout_ms, status); > -#undef C > + > + /* just trace the final value */ > + trace_i915_reg_rw(false, ch_ctl, status, sizeof(status), true); intel_de_wait_for_register() already does the tracing of the final value. > > return status; > }
diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c b/drivers/gpu/drm/i915/display/intel_dp_aux.c index 91c93c93e5fc..fe5ed432a66a 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c @@ -41,20 +41,20 @@ intel_dp_aux_wait_done(struct intel_dp *intel_dp) i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg(intel_dp); const unsigned int timeout_ms = 10; u32 status; - bool done; -#define C (((status = intel_de_read_notrace(i915, ch_ctl)) & DP_AUX_CH_CTL_SEND_BUSY) == 0) - done = wait_event_timeout(i915->display.gmbus.wait_queue, C, - msecs_to_jiffies_timeout(timeout_ms)); + __intel_wait_for_register(&i915->uncore, ch_ctl, + DP_AUX_CH_CTL_SEND_BUSY, + DP_AUX_CH_CTL_SEND_BUSY, + 500, timeout_ms, &status); - /* just trace the final value */ - trace_i915_reg_rw(false, ch_ctl, status, sizeof(status), true); - - if (!done) + status = intel_uncore_read_notrace(&i915->uncore, ch_ctl); + if ((status & DP_AUX_CH_CTL_SEND_BUSY) != 0) drm_err(&i915->drm, "%s: did not complete or timeout within %ums (status 0x%08x)\n", intel_dp->aux.name, timeout_ms, status); -#undef C + + /* just trace the final value */ + trace_i915_reg_rw(false, ch_ctl, status, sizeof(status), true); return status; }
The busy timeout logic checks for the AUX BUSY, then waits for the timeout period and then after timeout reads the register for BUSY or Success. Instead replace interrupt with polling so as to read the AUX CTL register often before the timeout period. Looks like there might be some issue with interrupt-on-read. Hence changing the logic to polling read. v2: replace interrupt with polling read v3: use usleep_rang instead of msleep, updated commit msg v4: use intel_wait_for_regiter internal function Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> --- drivers/gpu/drm/i915/display/intel_dp_aux.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)