Re: [Intel-gfx] [PATCH v2] drm/i915/display: Reset message bus after each read/write operation

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Oct 13, 2023 at 09:55:32AM +0300, Mika Kahola wrote:
> Every know and then we receive the following error when running
> for example IGT test kms_flip.
> 
> [drm] *ERROR* PHY G Read 0d80 failed after 3 retries.
> [drm] *ERROR* PHY G Write 0d81 failed after 3 retries.
> 
> Since the error is sporadic in nature, the patch proposes
> to reset the message bus after every successful or unsuccessful
> read or write operation. However, the testing revealed that this
> alone is not sufficient method and therefore an additional
> delay is introduced anything from 200us to 300us to let HW to
> settle down. This delay is experimental value and has no
> specification to back it up.
> 
> v2: Add FIXME's to indicate the experimental nature of
>     this workaround (Rodrigo)
> 
> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cx0_phy.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> index 6e6a1818071e..7c48ec5e54bd 100644
> --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> @@ -221,6 +221,14 @@ static u8 __intel_cx0_read(struct drm_i915_private *i915, enum port port,
>  	for (i = 0; i < 3; i++) {
>  		status = __intel_cx0_read_once(i915, port, lane, addr);
>  
> +		/*
> +		 * FIXME: Workaround to let HW to settle
> +		 * down and let the message bus to end up
> +		 * in a known state
> +		 */
> +		intel_cx0_bus_reset(i915, port, lane);
> +		usleep_range(200, 300);
> +
>  		if (status >= 0)
>  			return status;
>  	}
> @@ -300,6 +308,14 @@ static void __intel_cx0_write(struct drm_i915_private *i915, enum port port,
>  	for (i = 0; i < 3; i++) {
>  		status = __intel_cx0_write_once(i915, port, lane, addr, data, committed);
>  
> +		/*
> +		 * FIXME: Workaround to let HW to settle
> +		 * down and let the message bus to end up
> +		 * in a known state
> +		 */
> +		intel_cx0_bus_reset(i915, port, lane);
> +		usleep_range(200, 300);

what cases trigger these paths?
and how many calls in the modset case?
what about suspend/resume cylces?

if we do a single rmw we are introducing at least 400us of delay here.
have we measured the overall final impact of these extra sleeps on the resume and modeset latencies?

> +
>  		if (status == 0)
>  			return;
>  	}
> -- 
> 2.34.1
>