Re: [PATCH 8/9] drm/i915/vrr: Clamp guardband as per hardware and timing constraints

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

On Wed, Sep 24, 2025 at 04:21:28PM +0530, Ankit Nautiyal wrote:
> The maximum guardband value is constrained by two factors:
> - The actual vblank length minus set context latency (SCL)
> - The hardware register field width:
>   - 8 bits for ICL/TGL (VRR_CTL_PIPELINE_FULL_MASK -> max 255)
>   - 16 bits for ADL+ (XELPD_VRR_CTL_VRR_GUARDBAND_MASK -> max 65535)
> 
> Remove the #FIXME and clamp the guardband to the maximum allowed value.
> 
> v2:
> - Use REG_FIELD_MAX(). (Ville)
> - Separate out functions for intel_vrr_max_guardband(),
>   intel_vrr_max_vblank_guardband(). (Ville)
> 
> v3:
> - Fix Typo: Add the missing adjusted_mode->crtc_vdisplay in guardband
>   computation. (Ville)
> - Refactor intel_vrr_max_hw_guardband() and use else for consistency.
>   (Ville)
> 
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_vrr.c | 49 ++++++++++++++++++------
>  1 file changed, 37 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> index 26c5c32a9a58..e29b4050a9df 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -409,6 +409,40 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
>  	}
>  }
>  
> +static int
> +intel_vrr_max_hw_guardband(const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_display *display = to_intel_display(crtc_state);
> +	int max_pipeline_full = REG_FIELD_MAX(VRR_CTL_PIPELINE_FULL_MASK);
> +	int max_guardband;
> +
> +	if (DISPLAY_VER(display) >= 13)
> +		max_guardband = REG_FIELD_MAX(XELPD_VRR_CTL_VRR_GUARDBAND_MASK);
> +	else
> +		max_guardband = intel_vrr_pipeline_full_to_guardband(crtc_state,
> +								     max_pipeline_full);
> +	return max_guardband;

The 'max_guardband' variable looks useless here, could just return
directly from both sides of the if-else.

'max_pipeline_full' is perhaps redundant too, but I suppose the
line would get pretty long without it. So maybe it makes sense to keep
it.

> +}
> +
> +static int
> +intel_vrr_max_vblank_guardband(const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_display *display = to_intel_display(crtc_state);
> +	const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
> +
> +	return crtc_state->vrr.vmin -
> +	       adjusted_mode->crtc_vdisplay -
> +	       crtc_state->set_context_latency -
> +	       intel_vrr_extra_vblank_delay(display);
> +}
> +
> +static int
> +intel_vrr_max_guardband(struct intel_crtc_state *crtc_state)
> +{
> +	return min(intel_vrr_max_hw_guardband(crtc_state),
> +		   intel_vrr_max_vblank_guardband(crtc_state));
> +}
> +
>  void intel_vrr_compute_config_late(struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_display *display = to_intel_display(crtc_state);
> @@ -417,22 +451,13 @@ void intel_vrr_compute_config_late(struct intel_crtc_state *crtc_state)
>  	if (!intel_vrr_possible(crtc_state))
>  		return;
>  
> -	crtc_state->vrr.guardband =
> -		crtc_state->vrr.vmin -
> -		adjusted_mode->crtc_vdisplay -
> -		crtc_state->set_context_latency -
> -		intel_vrr_extra_vblank_delay(display);
> -
> -	if (DISPLAY_VER(display) < 13) {
> -		/* FIXME handle the limit in a proper way */
> -		crtc_state->vrr.guardband =
> -			min(crtc_state->vrr.guardband,
> -			    intel_vrr_pipeline_full_to_guardband(crtc_state, 255));
> +	crtc_state->vrr.guardband = min(crtc_state->vrr.vmin - adjusted_mode->crtc_vdisplay,
> +					intel_vrr_max_guardband(crtc_state));
>  
> +	if (DISPLAY_VER(display) < 13)
>  		crtc_state->vrr.pipeline_full =
>  			intel_vrr_guardband_to_pipeline_full(crtc_state,
>  							     crtc_state->vrr.guardband);
> -	}
>  }
>  
>  static u32 trans_vrr_ctl(const struct intel_crtc_state *crtc_state)
> -- 
> 2.45.2

-- 
Ville Syrjälä
Intel