Re: [PATCH 12/14] drm/i915/vrr: Use static guardband to support seamless LRR switching

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

On 9/10/2025 6:00 PM, Ville Syrjälä wrote:
> On Tue, Sep 09, 2025 at 08:32:11PM +0530, Nautiyal, Ankit K wrote:
>> On 9/8/2025 10:13 PM, Ville Syrjälä wrote:
>>> On Sun, Sep 07, 2025 at 01:02:39PM +0530, Ankit Nautiyal wrote:
>>>> In the current VRR implementation, vrr.vmin and vrr.guardband are set such
>>>> that they do not need to change when switching from fixed refresh rate to
>>>> variable refresh rate. Specifically, vrr.guardband is always set to match
>>>> the vblank length. This approach works for most cases, but not for LRR,
>>>> where the guardband would need to change while the VRR timing generator is
>>>> still active.
>>>>
>>>> With the VRR TG always active, live updates to guardband are unsafe and not
>>>> recommended. To ensure hardware safety, guardband was moved out of the
>>>> !fastset block, meaning any change now requires a full modeset.
>>>> This breaks seamless LRR switching, which was previously supported.
>>>>
>>>> Since the problem arises from guardband being matched to the vblank length,
>>>> solution is to use a minimal, sufficient static value, instead. So we use a
>>>> static guardband defined during mode-set that fits within the smallest
>>>> expected vblank and remains unchanged in case of features like LRR where
>>>> vtotal changes. To compute this minimum guardband we take into account
>>>> latencies/delays due to different features as mentioned in the Bspec.
>>>>
>>>> v2:
>>>> -Use helpers for dsc/scaler prefill latencies. (Mitul)
>>>> -Account for pkgc latency and take max of pkgc and sagv latencies.
>>>> v3: Use new helper for PSR2/Panel Replay latency.
>>>> v4: Avoid re-setting the Vmin/Flipline for optimized guardband.
>>>>
>>>> Bspec: 70151
>>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>>> Reviewed-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com> (#v3)
>>>> ---
>>>>    drivers/gpu/drm/i915/display/intel_display.c |   2 +-
>>>>    drivers/gpu/drm/i915/display/intel_vrr.c     | 127 ++++++++++++++++++-
>>>>    drivers/gpu/drm/i915/display/intel_vrr.h     |   3 +-
>>>>    3 files changed, 128 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>>>> index fb072275b1c7..3fa94675d5e1 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>>>> @@ -4902,7 +4902,6 @@ intel_modeset_pipe_config_late(struct intel_atomic_state *state,
>>>>    	struct drm_connector *connector;
>>>>    	int i;
>>>>    
>>>> -	intel_vrr_compute_config_late(crtc_state);
>>>>    
>>>>    	for_each_new_connector_in_state(&state->base, connector,
>>>>    					conn_state, i) {
>>>> @@ -4914,6 +4913,7 @@ intel_modeset_pipe_config_late(struct intel_atomic_state *state,
>>>>    		    !encoder->compute_config_late)
>>>>    			continue;
>>>>    
>>>> +		intel_vrr_compute_config_late(crtc_state, conn_state);
>>>>    		ret = encoder->compute_config_late(encoder, crtc_state,
>>>>    						   conn_state);
>>>>    		if (ret)
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
>>>> index 855974174afd..fff684eb2514 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
>>>> @@ -6,12 +6,15 @@
>>>>    
>>>>    #include <drm/drm_print.h>
>>>>    
>>>> +#include "intel_alpm.h"
>>>>    #include "intel_de.h"
>>>>    #include "intel_display_regs.h"
>>>>    #include "intel_display_types.h"
>>>>    #include "intel_dp.h"
>>>>    #include "intel_vrr.h"
>>>>    #include "intel_vrr_regs.h"
>>>> +#include "skl_scaler.h"
>>>> +#include "skl_watermark.h"
>>>>    
>>>>    #define FIXED_POINT_PRECISION		100
>>>>    #define CMRR_PRECISION_TOLERANCE	10
>>>> @@ -413,15 +416,135 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
>>>>    	}
>>>>    }
>>>>    
>>>> -void intel_vrr_compute_config_late(struct intel_crtc_state *crtc_state)
>>>> +static
>>>> +int scaler_prefill_latency(struct intel_crtc_state *crtc_state, int linetime_us)
>>>> +{
>>>> +	int chroma_downscaling_factor = skl_scaler_chroma_downscale_factor(crtc_state);
>>>> +	u64 hscale_k, vscale_k;
>>>> +	int cdclk_adjustment;
>>>> +	int num_scaler_users;
>>>> +
>>>> +	/*
>>>> +	 * Assuming:
>>>> +	 * Both scaler enabled.
>>>> +	 * scaler 1 downscaling factor as 2 x 2 (Horiz x Vert)
>>>> +	 * scaler 2 downscaling factor as 2 x 1 (Horiz x Vert)
>>>> +	 * Cdclk Adjustment : 1
>>>> +	 */
>>>> +	num_scaler_users = 2;
>>>> +	hscale_k = 2 * 1000;
>>>> +	vscale_k = 2 * 1000;
>>>> +	cdclk_adjustment = 1;
>>>> +
>>>> +	return intel_vrr_guardband_scaler_latency(num_scaler_users, hscale_k, vscale_k,
>>>> +						  chroma_downscaling_factor,
>>>> +						  cdclk_adjustment,
>>>> +						  linetime_us);
>>>> +}
>>>> +
>>>> +static
>>>> +int dsc_prefill_latency(struct intel_crtc_state *crtc_state, int linetime_us)
>>>> +{
>>>> +#define MAX_SCALERS 2
>>>> +	int chroma_downscaling_factor = skl_scaler_chroma_downscale_factor(crtc_state);
>>>> +	u64 hscale_k[MAX_SCALERS], vscale_k[MAX_SCALERS];
>>>> +	int cdclk_adjustment;
>>>> +	int num_scaler_users;
>>>> +
>>>> +	/*
>>>> +	 * Assuming:
>>>> +	 * Both scaler enabled.
>>>> +	 * scaler 1 downscaling factor as 2 x 2 (Horiz x Vert)
>>>> +	 * scaler 2 downscaling factor as 2 x 1 (Horiz x Vert)
>>>> +	 * Cdclk Adjustment : 1
>>>> +	 */
>>>> +	num_scaler_users = MAX_SCALERS;
>>>> +	hscale_k[0] = 2 * 1000;
>>>> +	vscale_k[0] = 2 * 1000;
>>>> +	hscale_k[1] = 2 * 1000;
>>>> +	vscale_k[1] = 1 * 1000;
>>>> +
>>>> +	cdclk_adjustment = 1;
>>>> +
>>>> +	return intel_vrr_guardband_dsc_latency(num_scaler_users, hscale_k, vscale_k,
>>>> +					       chroma_downscaling_factor,
>>>> +					       cdclk_adjustment,
>>>> +					       linetime_us);
>>>> +}
>>>> +
>>>> +static
>>>> +int intel_vrr_compute_guardband(struct intel_crtc_state *crtc_state,
>>>> +				struct intel_connector *connector)
>>>> +{
>>>> +	const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
>>>> +	struct intel_display *display = to_intel_display(crtc_state);
>>>> +	int dsc_prefill_time = 0;
>>>> +	int psr2_pr_latency = 0;
>>>> +	int scaler_prefill_time;
>>>> +	int wm0_prefill_time;
>>>> +	int pkgc_max_latency;
>>>> +	int sagv_latency;
>>>> +	int sdp_latency = 0;
>>>> +	int guardband_us;
>>>> +	int linetime_us;
>>>> +	int guardband;
>>>> +	int pm_delay;
>>>> +
>>>> +	linetime_us = DIV_ROUND_UP(adjusted_mode->crtc_htotal * 1000,
>>>> +				   adjusted_mode->crtc_clock);
>>>> +
>>>> +	pkgc_max_latency = skl_watermark_max_latency(display, 1);
>>>> +	sagv_latency = display->sagv.block_time_us;
>>>> +
>>>> +	/* Assuming max wm0 lines = 4 */
>>>> +	wm0_prefill_time = 4 * linetime_us + 20;
>>>> +
>>>> +	scaler_prefill_time = scaler_prefill_latency(crtc_state, linetime_us);
>>>> +
>>>> +	if (crtc_state->dsc.compression_enable)
>>>> +		dsc_prefill_time = dsc_prefill_latency(crtc_state, linetime_us);
>>>> +
>>>> +	pm_delay = crtc_state->framestart_delay +
>>>> +		   max(sagv_latency, pkgc_max_latency) +
>>>> +		   wm0_prefill_time +
>>>> +		   scaler_prefill_time +
>>>> +		   dsc_prefill_time;
>>>> +
>>>> +	switch (connector->base.connector_type) {
>>>> +	case DRM_MODE_CONNECTOR_eDP:
>>>> +	case DRM_MODE_CONNECTOR_DisplayPort:
>>>> +		psr2_pr_latency = intel_alpm_compute_max_link_wake_latency(crtc_state, true);
>>>> +		sdp_latency = intel_dp_compute_sdp_latency(crtc_state, true);
>>>> +		break;
>>>> +	default:
>>>> +		break;
>>>> +	}
>>>> +
>>>> +	guardband_us = max(sdp_latency, psr2_pr_latency);
>>>> +	guardband_us = max(guardband_us, pm_delay);
>>>> +
>>>> +	guardband = DIV_ROUND_UP(guardband_us, linetime_us);
>>>> +
>>>> +	/* guardband cannot be more than the Vmax vblank */
>>>> +	guardband = min(guardband, crtc_state->vrr.vmax - adjusted_mode->crtc_vblank_start);
>>>> +
>>>> +	return guardband;
>>>> +}
>>>> +
>>>> +void intel_vrr_compute_config_late(struct intel_crtc_state *crtc_state,
>>>> +				   struct drm_connector_state *conn_state)
>>>>    {
>>>>    	struct intel_display *display = to_intel_display(crtc_state);
>>>>    	const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
>>>> +	struct intel_connector *connector =
>>>> +		to_intel_connector(conn_state->connector);
>>>>    
>>>>    	if (!intel_vrr_possible(crtc_state))
>>>>    		return;
>>>>    
>>>> -	if (DISPLAY_VER(display) >= 13) {
>>>> +	if (intel_vrr_always_use_vrr_tg(display)) {
>>>> +		crtc_state->vrr.guardband = intel_vrr_compute_guardband(crtc_state, connector);
>>> This all looks to be in the wrong place. It needs to be done as part of
>>> intel_crtc_compute_vblank_delay() which then updates crtc_vblank_start
>>> to reflect reality instead of leaving some incorrect junk in there.
>>> Presumably that is why you also had to do that
>>> intel_dsb_wait_for_delayed_vblank() hack.
>> Hi Ville,
>>
>> Thanks for for the comments.
>>
>> Looks like intel_crtc_compute_vblank_delay() indeed is the place to add
>> the vblank_delay and get the crtc_vblank_start point to the delayed vblank.
>>
>> But I think, there are couple of things need to take care with this change:
>> 1) The SCL currently is derived from crtc_vblank_start - vdisplay, which
>> is ~1 with guradband = vblank length.
>> With modified crtc_vblank_start, this will become huge so perhaps we
>> need to set it to 1?
> When using the legacy timing generator SCL is what defines the
> position of the delayed vblank. So it should be exactly what it is.
>
> I don't quite recall how this stuff works with the VRR timing
> generator. I think it might have been that guardband more or less
> defines the location of the delayed vblank, and SCL then effectively
> defines the start of the safe window. So I think it should stay
> exactly the way it is. IIRC there is some way on new platforms to
> even move the SCL position into the vertical active, but I haven't
> though through what that would actually do for us.

Hmm yes for VRR timing generator case, guardband  defines the position 
of the delayed vblank.

Currently, I see that in most cases the vblank delay (crtc_vblank_start 
- crtc_vdisplay) evaluates to 0.

In few case its 1, I think based on intel_psr_min_vblank_delay().

This vblank delay is used to write in TRANS_SET_CONTEXT_LATENCY and to 
compute the vblank_start during readback.

With this series, since we have a shorter guardband now, the 
crtc_vblank_start is moved away by quite a bit.

Without modification this is resulting in TRANS_SET_CONTEXT_LATENCY to a 
bigger value and we are getting DSB POLL errors as it waits for safe window.

So with the updated crtc_vblank_start, for platforms where VRR TG is 
always on (and static guardband is used), I am now clamping the set 
context latency value between (0, 1).


There are couple of more points like with updated crtc_vblank_start for 
LRR, where it now changes since vtotal is changed.

Since guardband is static value the vblank_start moves due to change in 
vtotal, and prevents from fastset.

I have made some changes and will be posting patches after some more 
testing.


>
>> 2) The intel_dsb_wait_vblank_delay() (which inturn uses
>> intel_vrr_real_vblank_delay() for vrr case) also computes a fixed delay
>> based on (crtc_vblank_start - vdisplay) lines
>> Instead of this should we not use intel_dsb_wait_scanline_in() to wait
>> only till scanline reaches in range  [flipline decision boundary, vmax
>> decision boundary]?
> You can't use absolute line numbers with VRR since you don't know
> when the vblank will be terminated. It has to be all relative.

Oh ok. Then I will leave it as it is then.


Thanks & Regards,

Ankit

>
>> Let me know what you think, I can send a follow-up patch with the
>> suggested changes.
>>
>> Regards,
>> Ankit
>>
>>
>>>> +	} else if (DISPLAY_VER(display) >= 13) {
>>>>    		crtc_state->vrr.guardband =
>>>>    			crtc_state->vrr.vmin - adjusted_mode->crtc_vblank_start;
>>>>    	} else {
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.h b/drivers/gpu/drm/i915/display/intel_vrr.h
>>>> index 950041647e47..362638fd0d66 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_vrr.h
>>>> +++ b/drivers/gpu/drm/i915/display/intel_vrr.h
>>>> @@ -21,7 +21,8 @@ bool intel_vrr_possible(const struct intel_crtc_state *crtc_state);
>>>>    void intel_vrr_check_modeset(struct intel_atomic_state *state);
>>>>    void intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
>>>>    			      struct drm_connector_state *conn_state);
>>>> -void intel_vrr_compute_config_late(struct intel_crtc_state *crtc_state);
>>>> +void intel_vrr_compute_config_late(struct intel_crtc_state *crtc_state,
>>>> +				   struct drm_connector_state *conn_state);
>>>>    void intel_vrr_set_transcoder_timings(const struct intel_crtc_state *crtc_state);
>>>>    void intel_vrr_enable(const struct intel_crtc_state *crtc_state);
>>>>    void intel_vrr_send_push(struct intel_dsb *dsb,
>>>> -- 
>>>> 2.45.2