[PATCH v13 3/3] drm/i915/panelreplay: Panel replay workaround with VRR

[Cavitt, Jonathan]

-----Original Message-----
From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf Of Mitul Golani
Sent: Tuesday, October 1, 2024 6:47 AM
To: intel-gfx at lists.freedesktop.org
Cc: Nikula, Jani <jani.nikula at intel.com>; Syrjala, Ville <ville.syrjala at intel.com>; Nautiyal, Ankit K <ankit.k.nautiyal at intel.com>; Shankar, Uma <uma.shankar at intel.com>
Subject: [PATCH v13 3/3] drm/i915/panelreplay: Panel replay workaround with VRR
> 
> From: Animesh Manna <animesh.manna at intel.com>
> 
> Panel Replay VSC SDP not getting sent when VRR is enabled
> and W1 and W2 are 0. So Program Set Context Latency in
> TRANS_SET_CONTEXT_LATENCY register to at least a value of 1.
> The same is applicable for PSR1/PSR2 as well.
> 
> HSD: 14015406119
> 
> v1: Initial version.
> v2: Update timings stored in adjusted_mode struct. [Ville]
> v3: Add WA in compute_config(). [Ville]
> v4:
> - Add DISPLAY_VER() check and improve code comment. [Rodrigo]
> - Introduce centralized intel_crtc_vblank_delay(). [Ville]
> v5: Move to crtc_compute_config(). [Ville]
> v6: Restrict DISPLAY_VER till 14. [Mitul]
> v7:
> - Corrected code-comment. [Mitul]
> - dev_priv local variable removed. [Jani]
> v8: Introduce late_compute_config() which will take care late
> vblank-delay adjustment. [Ville]
> v9: Implementation simplified and split into multiple patches.
> v10:
> - Split vrr changes and use struct intel_display in DISPLAY_VER(). [Ankit]
> - Use for_each_new_intel_connector_in_state(). [Jani]
> v11: Remove loop and use flipline instead of vrr.enable flag. [Ville]
> v12:
> - Use intel_Vrr_possible helper.
> - Correct flag check for flipline.
> 
> Signed-off-by: Animesh Manna <animesh.manna at intel.com>
> Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani at intel.com>

I left some notes below, but nothing blocking, so:
Reviewed-by: Jonathan Cavitt <jonathan.cavitt at intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 21 ++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_display.h |  1 +
>  2 files changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index c59d7bffbf57..a8f846b654e9 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -2573,6 +2573,8 @@ static int intel_crtc_compute_config(struct intel_atomic_state *state,
>  		intel_atomic_get_new_crtc_state(state, crtc);
>  	int ret;
>  
> +	intel_crtc_adjust_vblank_delay(crtc_state);


If the purpose of adjusting the vblank delay is for workaround 14015401596,
we might want to consider refactoring this into two steps.  Specifically, we would
first separately check if the workaround is needed:

"""
/*
 * wa_14015401596 for display versions 13, 14.
 * Program Set Context Latency in TRANS_SET_CONTEXT_LATENCY register
 * to at least a value of 1 when PSR1/PSR2/Panel Replay is enabled with VRR.
 * Value for TRANS_SET_CONTEXT_LATENCY is calculated by subtracting
 * crtc_vdisplay from crtc_vblank_start, so this workaround is needed when
 * both are equal.
 */
static bool intel_crtc_needs_wa_14015401596(struct intel_crtc_state *crtc_state)
{
	struct intel_display *display = to_intel_display(crtc_state);
	struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;

	return (intel_vrr_possible(crtc_state) && crtc_state->has_psr &&
		adjusted_mode->crtc_vblank_start == adjusted_mode->crtc_vdisplay &&
		IS_DISPLAY_VER(display, 13, 14));
}
"""

Then, here, instead of calling intel_crtc_adjust_vblank_delay, we would do this:

"""
	if (intel_crtc_needs_wa_14015401596(crtc_state)) {
		struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
		adjusted_mode->crtc_vblank_start += 1;
	}
"""

I don't think this refactor is strictly necessary, though, so feel free to ignore this.


> +
>  	ret = intel_dpll_crtc_compute_clock(state, crtc);
>  	if (ret)
>  		return ret;
> @@ -3985,6 +3987,25 @@ bool intel_crtc_get_pipe_config(struct intel_crtc_state *crtc_state)
>  	return true;
>  }
>  
> +void intel_crtc_adjust_vblank_delay(struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_display *display = to_intel_display(crtc_state);
> +	struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
> +
> +	/*
> +	 * wa_14015401596 for display versions 13, 14.
> +	 * Program Set Context Latency in TRANS_SET_CONTEXT_LATENCY register
> +	 * to at least a value of 1 when PSR1/PSR2/Panel Replay is enabled with VRR.
> +	 * Value for TRANS_SET_CONTEXT_LATENCY is calculated by substracting


s/substracting/subtracting


> +	 * crtc_vdisplay from crtc_vblank_start, so incrementing crtc_vblank_start
> +	 * by 1 if both are equal.
> +	 */
> +	if (intel_vrr_possible(crtc_state) && crtc_state->has_psr &&
> +	    adjusted_mode->crtc_vblank_start == adjusted_mode->crtc_vdisplay &&
> +	    IS_DISPLAY_VER(display, 13, 14))
> +		adjusted_mode->crtc_vblank_start += 1;
> +}
> +
>  int intel_dotclock_calculate(int link_freq,
>  			     const struct intel_link_m_n *m_n)
>  {
> diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
> index 1f0fed5ea7bc..e6bd03ef104d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.h
> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> @@ -446,6 +446,7 @@ u8 _intel_modeset_primary_pipes(const struct intel_crtc_state *crtc_state);
>  u8 _intel_modeset_secondary_pipes(const struct intel_crtc_state *crtc_state);
>  struct intel_crtc *intel_primary_crtc(const struct intel_crtc_state *crtc_state);
>  bool intel_crtc_get_pipe_config(struct intel_crtc_state *crtc_state);
> +void intel_crtc_adjust_vblank_delay(struct intel_crtc_state *crtc_state);


You may want to make this a static function in intel_display.c.  Doing so would eliminate the need for
the added function declaration in intel_display.h.  The only caveat is that you would need to move the
current definition of this function in intel_display.c from where it is now to above
intel_crtc_compute_config if you wanted to go that route.
-Jonathan Cavitt


>  bool intel_pipe_config_compare(const struct intel_crtc_state *current_config,
>  			       const struct intel_crtc_state *pipe_config,
>  			       bool fastset);
> -- 
> 2.46.0
> 
>