Re: [Intel-gfx] [PATCHv2] drm/i915/dp: Change aux_ctl reg read to polling read
> -Original Message-
> From: Nikula, Jani
> Sent: Monday, December 5, 2022 4:08 PM
> To: Murthy, Arun R ; intel-
> g...@lists.freedesktop.org; ville.syrj...@linux.intel.com; Deak, Imre
>
> Cc: Murthy, Arun R
> Subject: Re: [PATCHv2] drm/i915/dp: Change aux_ctl reg read to polling read
>
> On Thu, 01 Dec 2022, Arun R Murthy 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 set
> > and fails.
>
> That's inaccurate. It checks for busy, waits for the gmbus irq, and on timeout
> or irq, whichever happens first, checks busy again.
>
> The commit message fails to explain what is wrong with that. The rationale is
> missing.
>
Sure will elaborate more windows found some issue with this interrupt on read
and hence
are using polling read with timeout. Hence we are trying to bring the same over
here.
> > Instead replace interrupt with polling so as to read the AUX CTL
> > register often before the timeout period.
>
> Again, what's the problem that this addresses? Are you not getting an irq,
> and often hitting the timeout? Or do you think there's too much delay
> between getting the irq and continuing? Or what?
>
As explained above windows also had some issue on interrupt on read and using
polling read.
> >
> > v2: replace interrupt with polling read
> >
> > Signed-off-by: Arun R Murthy
> > ---
> > 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 664bebdecea7..22c0a59850df 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c
> > @@ -40,20 +40,24 @@ 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_uncore_read_notrace(>uncore, 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));
> > + int try;
> >
> > + for (try = 0; try < 10; try++) {
> > + status = intel_uncore_read_notrace(>uncore, ch_ctl);
> > + if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0)
> > + break;
> > + msleep(1);
>
> I believe msleep(1), while used quite a bit in the kernel, is considered bad
> style. You don't really know how long it's going to sleep, it's not that
> accurate. It could be 10-20 ms I think. (IIRC I've seen 100+ ms sleeps on busy
> systems.) So if the condition never happens, you might be looping for 100-
> 200 ms here instead of the 10 ms.
Sure, I agree will change this to 500usleep range.
>
> And you might wait for 20+ ms before the 2nd read of the register.
>
> If your problem is the delay between the irq calling wake_up_all() and this
> actually waking up (which I believe is subject to the same latencies as
> msleep()) then this is all pretty random. You need to analyze and describe
> the problem better.
Yes agree, in the same function just above this where a check is being done for
any previous AUX channel activity, similar polling read is being done. So will
replace
both of these to usleep.
>
> > + }
> > /* just trace the final value */
> > trace_i915_reg_rw(false, ch_ctl, status, sizeof(status), true);
>
> This should continue to trace the final value. Now you read status once more
> after this.
Sorry my bad missed it. Will correct.
Thanks and Regards,
Arun R Murthy
>
>
> BR,
> Jani.
>
> >
> > - if (!done)
> > - drm_err(>drm,
> > - "%s: did not complete or timeout within %ums
> (status 0x%08x)\n",
> > - intel_dp->aux.name, timeout_ms, status);
> > -#undef C
> > + if (try == 3) {
> > + status = intel_uncore_read_notrace(>uncore, ch_ctl);
> > + if ((status & DP_AUX_CH_CTL_SEND_BUSY) != 0)
> > + drm_err(>drm,
> > + "%s: did not complete or timeout within
> %ums (status 0x%08x)\n",
> > + intel_dp->aux.name, timeout_ms, status);
> > + }
> >
> > return status;
> > }
>
> --
> Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [PATCHv2] drm/i915/dp: Change aux_ctl reg read to polling read
On Thu, 01 Dec 2022, Arun R Murthy 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 set
> and fails.
That's inaccurate. It checks for busy, waits for the gmbus irq, and on
timeout or irq, whichever happens first, checks busy again.
The commit message fails to explain what is wrong with that. The
rationale is missing.
> Instead replace interrupt with polling so as to read the AUX CTL
> register often before the timeout period.
Again, what's the problem that this addresses? Are you not getting an
irq, and often hitting the timeout? Or do you think there's too much
delay between getting the irq and continuing? Or what?
>
> v2: replace interrupt with polling read
>
> Signed-off-by: Arun R Murthy
> ---
> 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 664bebdecea7..22c0a59850df 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c
> @@ -40,20 +40,24 @@ 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_uncore_read_notrace(>uncore, 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));
> + int try;
>
> + for (try = 0; try < 10; try++) {
> + status = intel_uncore_read_notrace(>uncore, ch_ctl);
> + if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0)
> + break;
> + msleep(1);
I believe msleep(1), while used quite a bit in the kernel, is considered
bad style. You don't really know how long it's going to sleep, it's not
that accurate. It could be 10-20 ms I think. (IIRC I've seen 100+ ms
sleeps on busy systems.) So if the condition never happens, you might be
looping for 100-200 ms here instead of the 10 ms.
And you might wait for 20+ ms before the 2nd read of the register.
If your problem is the delay between the irq calling wake_up_all() and
this actually waking up (which I believe is subject to the same
latencies as msleep()) then this is all pretty random. You need to
analyze and describe the problem better.
> + }
> /* just trace the final value */
> trace_i915_reg_rw(false, ch_ctl, status, sizeof(status), true);
This should continue to trace the final value. Now you read status once
more after this.
BR,
Jani.
>
> - if (!done)
> - drm_err(>drm,
> - "%s: did not complete or timeout within %ums (status
> 0x%08x)\n",
> - intel_dp->aux.name, timeout_ms, status);
> -#undef C
> + if (try == 3) {
> + status = intel_uncore_read_notrace(>uncore, ch_ctl);
> + if ((status & DP_AUX_CH_CTL_SEND_BUSY) != 0)
> + drm_err(>drm,
> + "%s: did not complete or timeout within %ums
> (status 0x%08x)\n",
> + intel_dp->aux.name, timeout_ms, status);
> + }
>
> return status;
> }
--
Jani Nikula, Intel Open Source Graphics Center
[Intel-gfx] [PATCHv2] 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 set
and fails.
Instead replace interrupt with polling so as to read the AUX CTL
register often before the timeout period.
v2: replace interrupt with polling read
Signed-off-by: Arun R Murthy
---
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 664bebdecea7..22c0a59850df 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c
@@ -40,20 +40,24 @@ 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_uncore_read_notrace(>uncore, 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));
+ int try;
+ for (try = 0; try < 10; try++) {
+ status = intel_uncore_read_notrace(>uncore, 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 (!done)
- drm_err(>drm,
- "%s: did not complete or timeout within %ums (status
0x%08x)\n",
- intel_dp->aux.name, timeout_ms, status);
-#undef C
+ if (try == 3) {
+ status = intel_uncore_read_notrace(>uncore, ch_ctl);
+ if ((status & DP_AUX_CH_CTL_SEND_BUSY) != 0)
+ drm_err(>drm,
+ "%s: did not complete or timeout within %ums
(status 0x%08x)\n",
+ intel_dp->aux.name, timeout_ms, status);
+ }
return status;
}
--
2.25.1
