Re: [PATCH 13/16] drm/i915/display: Add vblank_start adjustment logic for always-on VRR TG

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

On Tue, Oct 07, 2025 at 12:00:34PM +0530, Nautiyal, Ankit K wrote:
> 
> On 10/7/2025 1:26 AM, Ville Syrjälä wrote:
> > On Mon, Oct 06, 2025 at 09:58:49AM +0530, Ankit Nautiyal wrote:
> >> As we move towards using a shorter, optimized guardband, we need to adjust
> >> how the delayed vblank start is computed.
> >>
> >> Introduce intel_crtc_compute_vrr_guardband() to handle guardband
> >> computation and apply vblank_start adjustment for platforms that always use
> >> the VRR timing generator.
> >>
> >> This function wraps the existing intel_vrr_compute_guardband() and adjusts
> >> crtc_vblank_start using (vblank_length - guardband) only when
> >> intel_vrr_always_use_vrr_tg() is true. Since the guardband is not yet
> >> optimized, the adjustment currently evaluates to zero, preserving existing
> >> behavior.
> >>
> >> This paves way for guardband optimization, by handling the movement of
> >> the crtc_vblank_start for platforms that have VRR TG always active.
> >>
> >> Also update allow_vblank_delay_fastset() to permit vblank delay adjustments
> >> during fastboot when VRR TG is always active, even without inherited state.
> >>
> >> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/display/intel_display.c | 33 ++++++++++++++++++--
> >>   1 file changed, 30 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> >> index b2d4e24fd7c6..1964e41b5704 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> >> @@ -2403,6 +2403,27 @@ static int intel_crtc_compute_set_context_latency(struct intel_atomic_state *sta
> >>   	return 0;
> >>   }
> >>   
> >> +static void intel_crtc_compute_vrr_guardband(struct intel_atomic_state *state,
> >> +					     struct intel_crtc *crtc)
> > Why this wrapper? You could just stick the adjustemnt into
> > intel_vrr_compute_guardband().
> 
> 
> The idea was to prepare for the optimized guardband which needs 
> connector also.

You shouldn't need the connector there. Looks like you were just using
it to figure out the output type when you could have just grabbed that
from the crtc state.

> 
> In subsequent patch I am getting the connector here to use the optimized 
> guardband only for platforms with always_use_vrr_tg=true.
> And at last I am making changes in intel_vrr_compute_guardband() itself.
> 
> As for this patch I can just avoid the wrapper and just use the adjustment.
> 
> >
> >> +{
> >> +	struct intel_display *display = to_intel_display(state);
> >> +	struct intel_crtc_state *crtc_state =
> >> +		intel_atomic_get_new_crtc_state(state, crtc);
> >> +	struct drm_display_mode *adjusted_mode =
> >> +		&crtc_state->hw.adjusted_mode;
> >> +
> >> +	intel_vrr_compute_guardband(crtc_state);
> >> +
> >> +	if (intel_vrr_always_use_vrr_tg(display)) {
> >> +		int vblank_length = adjusted_mode->crtc_vtotal -
> >> +				    (crtc_state->set_context_latency +
> >> +				     adjusted_mode->crtc_vdisplay);
> >> +
> >> +		adjusted_mode->crtc_vblank_start +=
> >> +			vblank_length - crtc_state->vrr.guardband;
> > Why aren't you using the same 'vblank_start = vtotal-guardband' here as
> > during readout?
> 
> Hmm I was thinking this more as change in the vblank_start. In 
> compute_set_context_latency we move the vblank_start by SCL lines. Here 
> we move further as much amount as the change in guardband.

The SCL adjustment is for the legacy timing generator timings.
This should just overwrite the whole thing with what the VRR
timing generator will actually do.

> 
> 
> But I guess that is not very intuitive, so I will just set 
> crtc_vblank_start as vtotal - guardband here.
> 
> 
> >
> >> +	}
> >> +}
> >> +
> >>   static int intel_crtc_compute_config(struct intel_atomic_state *state,
> >>   				     struct intel_crtc *crtc)
> >>   {
> >> @@ -2414,7 +2435,7 @@ static int intel_crtc_compute_config(struct intel_atomic_state *state,
> >>   	if (ret)
> >>   		return ret;
> >>   
> >> -	intel_vrr_compute_guardband(crtc_state);
> >> +	intel_crtc_compute_vrr_guardband(state, crtc);
> >>   
> >>   	ret = intel_dpll_crtc_compute_clock(state, crtc);
> >>   	if (ret)
> >> @@ -5105,9 +5126,15 @@ static bool allow_vblank_delay_fastset(const struct intel_crtc_state *old_crtc_s
> >>   	 * Allow fastboot to fix up vblank delay (handled via LRR
> >>   	 * codepaths), a bit dodgy as the registers aren't
> >>   	 * double buffered but seems to be working more or less...
> >> +	 *
> >> +	 * Also allow this when the VRR timing generator is always on,
> >> +	 * which implies optimized guardband is used. In such cases,
> >> +	 * vblank delay may vary even without inherited state, but it's
> >> +	 * still safe as VRR guardband is still same.
> >>   	 */
> >> -	return HAS_LRR(display) && old_crtc_state->inherited &&
> >> -		!intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DSI);
> >> +	return HAS_LRR(display) &&
> >> +	       (old_crtc_state->inherited || intel_vrr_always_use_vrr_tg(display)) &&
> >> +	       !intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DSI);
> > This part doesn't seem directly related to the making crtc_vblank_start
> > correct. We still use the non-optimzied guardband so crtc_vblank_start
> > should not be changing during normal runtime operation.
> 
> 
> Yes we do not need this at this time, but only when we really start 
> using optimized guardband.
> I can make it as a separate function.
> 
> Regards,
> 
> Ankit
> 
> 
> >>   }
> >>   
> >>   bool
> >> -- 
> >> 2.45.2

-- 
Ville Syrjälä
Intel