| Message ID | 20221215095533.1073528-1-arun.r.murthy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCHv6] drm/i915/dp: change aux_ctl reg read to polling read | expand |
On Thu, 15 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 > v5: use __intel_de_wait_for_register with 500us slow and 10ms fast timeout > v6: check return value of __intel_de_wait_for_register > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp_aux.c | 15 +++++---------- > 1 file changed, 5 insertions(+), 10 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..dec88f41380e 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c > @@ -40,21 +40,16 @@ intel_dp_aux_wait_done(struct intel_dp *intel_dp) > 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; > - 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)); > + u32 status, ret; > > - /* just trace the final value */ > - trace_i915_reg_rw(false, ch_ctl, status, sizeof(status), true); > + ret = __intel_de_wait_for_register(i915, ch_ctl, > + DP_AUX_CH_CTL_SEND_BUSY, 0, > + 500, timeout_ms, &status); > > - if (!done) > + if (ret == -ETIMEDOUT) What's wrong with this comparison? Although it probably does work by coincidence. BR, Jani. > 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 > > return status; > }
> -----Original Message----- > From: Nikula, Jani <jani.nikula@intel.com> > Sent: Thursday, December 15, 2022 4:00 PM > To: Murthy, Arun R <arun.r.murthy@intel.com>; intel- > gfx@lists.freedesktop.org; ville.syrjala@linux.intel.com; Deak, Imre > <imre.deak@intel.com> > Cc: Murthy, Arun R <arun.r.murthy@intel.com> > Subject: Re: [PATCHv6] drm/i915/dp: change aux_ctl reg read to polling read > > On Thu, 15 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 > > v5: use __intel_de_wait_for_register with 500us slow and 10ms fast > > timeout > > v6: check return value of __intel_de_wait_for_register > > > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_dp_aux.c | 15 +++++---------- > > 1 file changed, 5 insertions(+), 10 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..dec88f41380e 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c > > @@ -40,21 +40,16 @@ intel_dp_aux_wait_done(struct intel_dp *intel_dp) > > 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; > > - 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)); > > + u32 status, ret; > > > > - /* just trace the final value */ > > - trace_i915_reg_rw(false, ch_ctl, status, sizeof(status), true); > > + ret = __intel_de_wait_for_register(i915, ch_ctl, > > + DP_AUX_CH_CTL_SEND_BUSY, 0, > > + 500, timeout_ms, &status); > > > > - if (!done) > > + if (ret == -ETIMEDOUT) > > What's wrong with this comparison? Although it probably does work by > coincidence. > If (ret) dev_err(); Will also work, but for code readability using -ETIMEDOUT(sorry missed the declaration int ret;) Thanks and Regards, Arun R Murthy -------------------- > BR, > Jani. > > > > 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 > > > > return status; > > } > > -- > Jani Nikula, Intel Open Source Graphics Center
diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c b/drivers/gpu/drm/i915/display/intel_dp_aux.c index 91c93c93e5fc..dec88f41380e 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c @@ -40,21 +40,16 @@ intel_dp_aux_wait_done(struct intel_dp *intel_dp) 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; - 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)); + u32 status, ret; - /* just trace the final value */ - trace_i915_reg_rw(false, ch_ctl, status, sizeof(status), true); + ret = __intel_de_wait_for_register(i915, ch_ctl, + DP_AUX_CH_CTL_SEND_BUSY, 0, + 500, timeout_ms, &status); - if (!done) + 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); -#undef C 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 v5: use __intel_de_wait_for_register with 500us slow and 10ms fast timeout v6: check return value of __intel_de_wait_for_register Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> --- drivers/gpu/drm/i915/display/intel_dp_aux.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)