> -----Original Message-----
> From: Nikula, Jani <jani.nikula@xxxxxxxxx>
> Sent: Monday, December 5, 2022 4:08 PM
> To: Murthy, Arun R <arun.r.murthy@xxxxxxxxx>; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx; ville.syrjala@xxxxxxxxxxxxxxx; Deak, Imre
> <imre.deak@xxxxxxxxx>
> Cc: Murthy, Arun R <arun.r.murthy@xxxxxxxxx>
> Subject: Re: [PATCHv2] drm/i915/dp: Change aux_ctl reg read to polling read
>
> On Thu, 01 Dec 2022, Arun R Murthy <arun.r.murthy@xxxxxxxxx> 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 <arun.r.murthy@xxxxxxxxx>
> > ---
> > 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(&i915->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(&i915->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(&i915->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(&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);
> > + }
> >
> > return status;
> > }
>
> --
> Jani Nikula, Intel Open Source Graphics Center
