| Message ID | 20221209083510.475948-1-arun.r.murthy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND,PATCHv3] drm/i915/dp: Change aux_ctl reg read to polling read | expand |
On Fri, 09 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 > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp_aux.c | 24 ++++++++++++--------- > 1 file changed, 14 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..230f27d75846 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c > @@ -41,21 +41,25 @@ 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; > + int try; > > -#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)); > + for (try = 0; try < 10; try++) { > + status = intel_uncore_read_notrace(&i915->uncore, ch_ctl); > + if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0) > + break; > + usleep_range(400, 500); > + } > + if (try == 3) { > + 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); > + } > > /* just trace the final value */ > trace_i915_reg_rw(false, ch_ctl, status, sizeof(status), true); Okay, so there are still a bunch of issues above. For example, try < 10 vs. try == 3, reverting back to intel_uncore_* functions after conflict resolution, having duplicated reads and conditions. Now, I should've taken a step back earlier and realized you should use the helper we already have for this: intel_de_wait_for_register(). All of the above shrinks to just a few lines: ret = intel_de_wait_for_register(i915, ch_ctl, DP_AUX_CH_CTL_SEND_BUSY, 0, timeout_ms); if (ret) drm_err(...); Sorry for missing this earlier. BR, Jani. > > - if (!done) > - 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: Friday, December 9, 2022 4:16 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: [RESEND PATCHv3] drm/i915/dp: Change aux_ctl reg read to > polling read > > On Fri, 09 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 > > > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_dp_aux.c | 24 > > ++++++++++++--------- > > 1 file changed, 14 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..230f27d75846 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c > > @@ -41,21 +41,25 @@ 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; > > + int try; > > > > -#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)); > > + for (try = 0; try < 10; try++) { > > + status = intel_uncore_read_notrace(&i915->uncore, ch_ctl); > > + if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0) > > + break; > > + usleep_range(400, 500); > > + } > > + if (try == 3) { > > + 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); > > + } > > > > /* just trace the final value */ > > trace_i915_reg_rw(false, ch_ctl, status, sizeof(status), true); > > Okay, so there are still a bunch of issues above. For example, try < 10 vs. try > == 3, reverting back to intel_uncore_* functions after conflict resolution, > having duplicated reads and conditions. The logic tries to read in a loop for 10 times with a sleep of 500usec between each reads. Finally after the 10th iteration for the last time the register is read again and the status is returned. > > Now, I should've taken a step back earlier and realized you should use the > helper we already have for this: intel_de_wait_for_register(). I checked this earlier. The reason for not opting this is this function is its interrupt-on-read, but we need a polling read with timeout. Thanks and Regards, Arun R Murthy -------------------- > > All of the above shrinks to just a few lines: > > ret = intel_de_wait_for_register(i915, ch_ctl, > DP_AUX_CH_CTL_SEND_BUSY, 0, timeout_ms); > if (ret) > drm_err(...); > > Sorry for missing this earlier. > > BR, > Jani. > > > > > - if (!done) > > - 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
On Tue, 13 Dec 2022, "Murthy, Arun R" <arun.r.murthy@intel.com> wrote: >> -----Original Message----- >> From: Nikula, Jani <jani.nikula@intel.com> >> Sent: Friday, December 9, 2022 4:16 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: [RESEND PATCHv3] drm/i915/dp: Change aux_ctl reg read to >> polling read >> >> On Fri, 09 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 >> > >> > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> >> > --- >> > drivers/gpu/drm/i915/display/intel_dp_aux.c | 24 >> > ++++++++++++--------- >> > 1 file changed, 14 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..230f27d75846 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c >> > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c >> > @@ -41,21 +41,25 @@ 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; >> > + int try; >> > >> > -#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)); >> > + for (try = 0; try < 10; try++) { >> > + status = intel_uncore_read_notrace(&i915->uncore, ch_ctl); >> > + if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0) >> > + break; >> > + usleep_range(400, 500); >> > + } >> > + if (try == 3) { >> > + 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); >> > + } >> > >> > /* just trace the final value */ >> > trace_i915_reg_rw(false, ch_ctl, status, sizeof(status), true); >> >> Okay, so there are still a bunch of issues above. For example, try < 10 vs. try >> == 3, reverting back to intel_uncore_* functions after conflict resolution, >> having duplicated reads and conditions. > The logic tries to read in a loop for 10 times with a sleep of 500usec between each reads. > Finally after the 10th iteration for the last time the register is read again and the status is returned. Yeah, but the condition is (try == 3). > >> >> Now, I should've taken a step back earlier and realized you should use the >> helper we already have for this: intel_de_wait_for_register(). > > I checked this earlier. The reason for not opting this is this function is its > interrupt-on-read, but we need a polling read with timeout. It *is* a polling read with a timeout, with a bunch of smarts. BR, Jani. > > Thanks and Regards, > Arun R Murthy > -------------------- >> >> All of the above shrinks to just a few lines: >> >> ret = intel_de_wait_for_register(i915, ch_ctl, >> DP_AUX_CH_CTL_SEND_BUSY, 0, timeout_ms); >> if (ret) >> drm_err(...); >> >> Sorry for missing this earlier. >> >> BR, >> Jani. >> >> > >> > - if (!done) >> > - 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..230f27d75846 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c @@ -41,21 +41,25 @@ 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; + int try; -#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)); + for (try = 0; try < 10; try++) { + status = intel_uncore_read_notrace(&i915->uncore, ch_ctl); + if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0) + break; + usleep_range(400, 500); + } + if (try == 3) { + 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); + } /* just trace the final value */ trace_i915_reg_rw(false, ch_ctl, status, sizeof(status), true); - if (!done) - 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 Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> --- drivers/gpu/drm/i915/display/intel_dp_aux.c | 24 ++++++++++++--------- 1 file changed, 14 insertions(+), 10 deletions(-)