Re: [Intel-gfx] [PATCHv2] drm/i915/dp: Change aux_ctl reg read to polling read

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 01 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 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 <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 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.

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(&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