Re: [PATCH 10/10] drm/i915/display: Prepare for vblank_delay for LRR

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

On Wed, Oct 15, 2025 at 12:52:17PM +0530, Ankit Nautiyal wrote:
> Update allow_vblank_delay_fastset() to permit vblank delay adjustments
> during with LRR when VRR TG is always active.
> 
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index ceee5ae99c2c..65a7da694ef6 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -4958,9 +4958,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,
> +	 * and 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);

I suppose this won't actually do anything until we get the fixed
guardband size in place. But with that I suppose it is the right
thing to do.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

However I was pondering about the place where do timing generator
reprogramming. Currently that is done from within the vblank evasion
critical section. But that is actually wrong because the VRR registers
aren't double buffered. So in the worst case we'll evade the previous
vblank start, and then reprogram the timing generator which could
move the vblank start to be just ahead of the current scanline and
then the commit will end up straddling the start of vblank (which is
exactly what the vblank evasion tried to prevent).

So I think we'll have to move the timing generator update to happen
after we've done all the double buffered register programming. I suppose
that might still be technically wrong as then the position of the
delayed vblank might still move before the double buffered registers
have been latched. I don't think that shouldn't cause any underruns/etc.
but in the worst case the start of vblank moves backwards past the
current scanline, and then the registers don't actually latch until the
next frame.

I wonder if we should use the vblank worker here to do the timing
generator update right after the delayed vblank? That would guarantee
that the current delayed vblank stays in place until the register have
been latched.

Though we may still end up in at least two weird scenarios:
- delayed vblank moves forward, and we might get two delayed vblank
  events for the same frame
- vtotal gets reduced below the current delayed vblank start. Which
  I suppose means the vtotal for the frame will not necessarily be
  the old or new vtotal value, but something in between.

That's all assuming certain behaviours of the VRR timing generator
of course. I haven't actuall confirmed how the hardware behaves
in either case. We should probably do some more hw poking at some
point to really figure this stuff out...

-- 
Ville Syrjälä
Intel