Re: [PATCH 3/5] drm/i915/vrr: Store guardband in crtc state even for icl/tgl

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

On 9/18/2025 2:04 AM, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> While ICL/TGL VRR hardware doesn't have a register for the guardband
> value, our lives will be simpler if we pretend that it does. Start
> by computing the guardband the same as on ADL+ and storing it in
> the state, and only then we convert it into the corresponding
> pipeline_full value that the hardware can consume. During readout we
> do the opposite.
>
> I was debating whether to completely remove pipeline_full from the
> crtc state, but decided to keep it for now. Mainly because we check
> it in vrr_params_changed() and simply checking the guardband instead
> isn't 100% equivalent; Theoretically, framestart_delay may have
> changed in the opposite direction to pipeline_full, keeping the
> derived guardband value unchaged. One solution would be to also check
> framestart_delay, but that feels a bit leaky abstraction wise.
>
> Also note that we don't currently handle the maximum limit of 255
> scanlines for the pipeline_full in a very nice way. The actual
> position of the delayed vblank will move because of that clamping,
> and so some of our code may get confused. But fixing this shall
> wait a for now.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_display.c |  1 +
>   drivers/gpu/drm/i915/display/intel_vrr.c     | 36 +++++++++++---------
>   2 files changed, 21 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index c7d85fd38890..f4124c79bc83 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -3891,6 +3891,7 @@ static bool hsw_get_pipe_config(struct intel_crtc *crtc,
>   	intel_joiner_get_config(pipe_config);
>   	intel_dsc_get_config(pipe_config);
>   
> +	/* intel_vrr_get_config() depends on .framestart_delay */
>   	if (!transcoder_is_dsi(pipe_config->cpu_transcoder)) {
>   		tmp = intel_de_read(display, CHICKEN_TRANS(display, pipe_config->cpu_transcoder));
>   
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> index 5fee85b0bc99..9cdcc2558ead 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -151,13 +151,7 @@ static int intel_vrr_pipeline_full_to_guardband(const struct intel_crtc_state *c
>    */
>   static int intel_vrr_vblank_exit_length(const struct intel_crtc_state *crtc_state)
>   {
> -	struct intel_display *display = to_intel_display(crtc_state);
> -
> -	if (DISPLAY_VER(display) >= 13)
> -		return crtc_state->vrr.guardband;
> -	else
> -		return intel_vrr_pipeline_full_to_guardband(crtc_state,
> -							    crtc_state->vrr.pipeline_full);
> +	return crtc_state->vrr.guardband;
>   }
>   
>   int intel_vrr_vmin_vtotal(const struct intel_crtc_state *crtc_state)
> @@ -431,18 +425,22 @@ void intel_vrr_compute_config_late(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;
> -	int guardband;
>   
>   	if (!intel_vrr_possible(crtc_state))
>   		return;
>   
> -	guardband = crtc_state->vrr.vmin - adjusted_mode->crtc_vblank_start;
> +	crtc_state->vrr.guardband =
> +		crtc_state->vrr.vmin - adjusted_mode->crtc_vblank_start;
> +
> +	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));
>   
> -	if (DISPLAY_VER(display) >= 13) {
> -		crtc_state->vrr.guardband = guardband;
> -	} else {
>   		crtc_state->vrr.pipeline_full =
> -			min(255, intel_vrr_guardband_to_pipeline_full(crtc_state, guardband));
> +			intel_vrr_guardband_to_pipeline_full(crtc_state,
> +							     crtc_state->vrr.guardband);


For removing the #FIXME to handle the limit what can be required:

Do we need to abstract it with intel_vrr_clamp_pipeline_full()  or else 
we need:

crtc_state->vrr.pipeline_full = min(255, 
intel_vrr_guardband_to_pipeline_full(crtc_state, 
crtc_state->vrr.guardband));

crtc_state->vrr.guardband = 
intel_vrr_guardband_to_pipeline_full(crtc_state, 
crtc_state->vrr.pipeline_full);

(Though this might be bit confusing since we use guardband to get 
pipline and again change guardband.)


Regards,

Ankit


>   
>   		/*
>   		 * vmin/vmax/flipline also need to be adjusted by
> @@ -734,14 +732,20 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state)
>   					     TRANS_CMRR_M_HI(display, cpu_transcoder));
>   	}
>   
> -	if (DISPLAY_VER(display) >= 13)
> +	if (DISPLAY_VER(display) >= 13) {
>   		crtc_state->vrr.guardband =
>   			REG_FIELD_GET(XELPD_VRR_CTL_VRR_GUARDBAND_MASK, trans_vrr_ctl);
> -	else
> -		if (trans_vrr_ctl & VRR_CTL_PIPELINE_FULL_OVERRIDE)
> +	} else {
> +		if (trans_vrr_ctl & VRR_CTL_PIPELINE_FULL_OVERRIDE) {
>   			crtc_state->vrr.pipeline_full =
>   				REG_FIELD_GET(VRR_CTL_PIPELINE_FULL_MASK, trans_vrr_ctl);
>   
> +			crtc_state->vrr.guardband =
> +				intel_vrr_pipeline_full_to_guardband(crtc_state,
> +								     crtc_state->vrr.pipeline_full);
> +		}
> +	}
> +
>   	if (trans_vrr_ctl & VRR_CTL_FLIP_LINE_EN) {
>   		crtc_state->vrr.flipline = intel_de_read(display,
>   							 TRANS_VRR_FLIPLINE(display, cpu_transcoder)) + 1;