Re: [Intel-gfx] [PATCH] drm/i915/dp: wait on timeout before retry include sw delay

[Imre Deak <imre.deak@intel.com>]

On Thu, Nov 24, 2022 at 12:39:25PM +0530, Arun R Murthy wrote:
> AUX HW timeout is being set to max(4000ms), consider AUX SW timeout to
                                     ^ 4ms
> be 200ms more to avoid AUX boundary read//write.

The HSD mentions a 200us extension.

> HSDES: 1409498780
> 
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp_aux.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 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..6c1c9602518b 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c
> @@ -293,14 +293,13 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
>  					   DP_AUX_CH_CTL_RECEIVE_ERROR);
>  
>  			/*
> -			 * DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2
> -			 *   400us delay required for errors and timeouts
> -			 *   Timeout errors from the HW already meet this
> -			 *   requirement so skip to next iteration

I think keeping the above still makes sense to explain why the 400us
explicit delay described in the CTS is not needed.

> +			 * Once the hw timeouts, before next try
> +			 * need to add a sw timeout of 200usec(HSD: 1409498780).

The HSD is for ICL, I can't see the bspec entry for it at bspec/33450.
WAs should have a "Wa_<lineage>:<platform(s)>" tag.

>  			 */
> -			if (status & DP_AUX_CH_CTL_TIME_OUT_ERROR)
> +			if (status & DP_AUX_CH_CTL_TIME_OUT_ERROR) {
> +				usleep_range(200, 300);

The HSD ticket implies that the WA is to increase the timeout when
polling for the BUSY flag to clear from 600us to 800us for the non-LTTPR
case and from 4ms to 4.2ms in the LTTPR case (regardless of why the BUSY
flag is cleared). This seems to match what the Windows driver does now.
i915 waits for the same condition for 10ms, so to me it looks like the
waiting here already complies with the change described by the HSD.

One difference I see is that Windows just polls for the BUSY flag
without enabling the interrupt-on-done for this, not sure if this could
cause a problem.

>  				continue;
> -
> +			}
>  			if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) {
>  				usleep_range(400, 500);
>  				continue;
> -- 
> 2.25.1
>