Re: [PATCH 12/13] drm/i915/vrr: Always use VRR timing generator for XELPD+

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Sep 02, 2024 at 01:36:33PM +0530, Ankit Nautiyal wrote:
> Currently VRR timing generator is used only when VRR is enabled by
> userspace. From XELPD+, gradually move away from older timing
> generator and use VRR timing generator for fixed refresh rate also.
> In such a case, Flipline VMin and VMax all are set to the Vtotal of the
> mode, which effectively makes the VRR timing generator work in
> fixed refresh rate mode.
> 
> v2: Use VRR Timing Generator from XELPD+ instead of MTL as it needs
> Wa_14015406119.
> v3: Set vrr.fixed during vrr_get_config (Mitul)
> v4: Avoid setting vrr.fixed when vrr.cmrr is enabled.
> 
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_vrr.c | 61 +++++++++++++++---------
>  1 file changed, 39 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> index e01d4b4b8fa7..625728aff5a2 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -172,41 +172,54 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
>  	if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
>  		return;
>  
> -	crtc_state->vrr.in_range =
> -		intel_vrr_is_in_range(connector, drm_mode_vrefresh(adjusted_mode));
> -	if (!crtc_state->vrr.in_range)
> -		return;
> -
>  	if (HAS_LRR(display))
>  		crtc_state->update_lrr = true;

We aren't supposed to do a LRR update unless the refresh rates are
within the VRR range. So this needs to be moved as well.

>  
> -	vmin = DIV_ROUND_UP(adjusted_mode->crtc_clock * 1000,
> -			    adjusted_mode->crtc_htotal * info->monitor_range.max_vfreq);
> -	vmax = adjusted_mode->crtc_clock * 1000 /
> -		(adjusted_mode->crtc_htotal * info->monitor_range.min_vfreq);
> +	if (!crtc_state->uapi.vrr_enabled && DISPLAY_VER(display) >= 20) {
> +		/*
> +		 * for XELPD+ always go for VRR timing generator even for
> +		 * fixed refresh rate.
> +		 */
> +		crtc_state->vrr.vmin = adjusted_mode->crtc_vtotal;
> +		crtc_state->vrr.vmax = adjusted_mode->crtc_vtotal;
> +		crtc_state->vrr.flipline = adjusted_mode->crtc_vtotal;

Has the "flipline can't be below vmin+1" issue been changed in the hardware?

> +		crtc_state->vrr.fixed_rr = true;
> +	} else {
> +		crtc_state->vrr.in_range =
> +			intel_vrr_is_in_range(connector, drm_mode_vrefresh(adjusted_mode));
>  
> -	vmin = max_t(int, vmin, adjusted_mode->crtc_vtotal);
> -	vmax = max_t(int, vmax, adjusted_mode->crtc_vtotal);
> +		if (!crtc_state->vrr.in_range)
> +			return;
>  
> -	if (vmin >= vmax)
> -		return;
> +		vmin = DIV_ROUND_UP(adjusted_mode->crtc_clock * 1000,
> +				    adjusted_mode->crtc_htotal * info->monitor_range.max_vfreq);
> +		vmax = adjusted_mode->crtc_clock * 1000 /
> +			(adjusted_mode->crtc_htotal * info->monitor_range.min_vfreq);
>  
> -	/*
> -	 * flipline determines the min vblank length the hardware will
> -	 * generate, and flipline>=vmin+1, hence we reduce vmin by one
> -	 * to make sure we can get the actual min vblank length.
> -	 */
> -	crtc_state->vrr.vmin = vmin - 1;
> -	crtc_state->vrr.vmax = vmax;
> +		vmin = max_t(int, vmin, adjusted_mode->crtc_vtotal);
> +		vmax = max_t(int, vmax, adjusted_mode->crtc_vtotal);
> +
> +		if (vmin >= vmax)
> +			return;
> +
> +		/*
> +		 * flipline determines the min vblank length the hardware will
> +		 * generate, and flipline>=vmin+1, hence we reduce vmin by one
> +		 * to make sure we can get the actual min vblank length.
> +		 */
> +		crtc_state->vrr.vmin = vmin - 1;
> +		crtc_state->vrr.vmax = vmax;
>  
> -	crtc_state->vrr.flipline = crtc_state->vrr.vmin + 1;
> +		crtc_state->vrr.flipline = crtc_state->vrr.vmin + 1;
> +		crtc_state->vrr.fixed_rr = false;
> +	}
>  
>  	/*
>  	 * When panel is VRR capable and userspace has
>  	 * not enabled adaptive sync mode then Fixed Average
>  	 * Vtotal mode should be enabled.
>  	 */
> -	if (crtc_state->uapi.vrr_enabled) {
> +	if (crtc_state->uapi.vrr_enabled || crtc_state->vrr.fixed_rr) {
>  		crtc_state->vrr.enable = true;
>  		crtc_state->mode_flags |= I915_MODE_FLAG_VRR;

Hmm. This is now a bit of a mess. We need to come up with a sane way to
deal with the vblank timestamping code. Dunno if we want to make it so
that we'd always use VRR timings or the non-VRR timings. Should be
identical from HW POV so technically might not matter, apart from the
software state tracking POV. And from that angle it seems to me that
for the dynamic fixed vs. variable swithcing we might want to keep the
code using the non-VRR timings for fixed refresh rate.

There seems to other stuff amiss still:
- We don't enable/disable the VRR timings generator early/late
  in the modeset sequence?
- How do we atomically switch between the fixed vs. variable
  timings since we can't temporarily disable the VRR timing generator?

>  	} else if (is_cmrr_frac_required(crtc_state) && is_edp) {
> @@ -421,6 +434,10 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state)
>  						     TRANS_VRR_VMAX(display, cpu_transcoder)) + 1;
>  		crtc_state->vrr.vmin = intel_de_read(display,
>  						     TRANS_VRR_VMIN(display, cpu_transcoder)) + 1;
> +		if (!crtc_state->cmrr.enable &&
> +		    crtc_state->vrr.vmax == crtc_state->vrr.flipline &&
> +		    crtc_state->vrr.vmin == crtc_state->vrr.flipline)
> +			crtc_state->vrr.fixed_rr = true;
>  	}
>  
>  	if (crtc_state->vrr.enable) {
> -- 
> 2.45.2

-- 
Ville Syrjälä
Intel