Re: [PATCH 11/15] drm/i915/display: Use vrr.guardband to derive vblank_start

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

On Wed, Sep 17, 2025 at 01:51:15PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 16, 2025 at 09:56:47PM +0300, Ville Syrjälä wrote:
> > On Tue, Sep 16, 2025 at 08:08:01PM +0530, Nautiyal, Ankit K wrote:
> > > 
> > > On 9/16/2025 8:00 PM, Nautiyal, Ankit K wrote:
> > > >
> > > > On 9/15/2025 6:02 PM, Ville Syrjälä wrote:
> > > >> On Sun, Sep 14, 2025 at 11:29:10AM +0530, Nautiyal, Ankit K wrote:
> > > >>> On 9/11/2025 7:55 PM, Ville Syrjälä wrote:
> > > >>>> On Thu, Sep 11, 2025 at 08:15:50AM +0530, Ankit Nautiyal wrote:
> > > >>>>> When VRR TG is always enabled and an optimized guardband is used, 
> > > >>>>> the pipe
> > > >>>>> vblank start is derived from the guardband.
> > > >>>>> Currently TRANS_SET_CONTEXT_LATENCY is programmed with 
> > > >>>>> crtc_vblank_start -
> > > >>>>> crtc_vdisplay, which is ~1 when guardband matches the vblank length.
> > > >>>>> With shorter guardband this become a large window.
> > > >>>>>
> > > >>>>> To avoid misprogramming TRANS_SET_CONTEXT_LATENCY, clamp the scl 
> > > >>>>> value to 1
> > > >>>>> when using optimized guardband.
> > > >>>>>
> > > >>>>> Also update the VRR get config logic to set crtc_vblank_start 
> > > >>>>> based on
> > > >>>>> vtotal - guardband, during readback.
> > > >>>>>
> > > >>>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> > > >>>>> ---
> > > >>>>>    drivers/gpu/drm/i915/display/intel_display.c | 36 
> > > >>>>> ++++++++++++++++----
> > > >>>>>    drivers/gpu/drm/i915/display/intel_vrr.c     |  9 ++++-
> > > >>>>>    2 files changed, 38 insertions(+), 7 deletions(-)
> > > >>>>>
> > > >>>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
> > > >>>>> b/drivers/gpu/drm/i915/display/intel_display.c
> > > >>>>> index 55bea1374dc4..73aec6d4686a 100644
> > > >>>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > >>>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > >>>>> @@ -2638,6 +2638,30 @@ transcoder_has_vrr(const struct 
> > > >>>>> intel_crtc_state *crtc_state)
> > > >>>>>        return HAS_VRR(display) && !transcoder_is_dsi(cpu_transcoder);
> > > >>>>>    }
> > > >>>>>    +static int intel_set_context_latency(const struct 
> > > >>>>> intel_crtc_state *crtc_state,
> > > >>>>> +                     int crtc_vblank_start,
> > > >>>>> +                     int crtc_vdisplay)
> > > >>>>> +{
> > > >>>>> +    struct intel_display *display = to_intel_display(crtc_state);
> > > >>>>> +
> > > >>>>> +    /*
> > > >>>>> +     * When VRR TG is always on and optimized guardband is used,
> > > >>>>> +     * the pipe vblank start is based on the guardband,
> > > >>>>> +     * TRANS_SET_CONTEXT_LATENCY cannot be used to configure it.
> > > >>>>> +     */
> > > >>>>> +    if (intel_vrr_always_use_vrr_tg(display))
> > > >>>>> +        return clamp(crtc_vblank_start - crtc_vdisplay, 0, 1);
> > > >>>> What are you trying to achieve with this? As in what problem are you
> > > >>>> seeing with the current SCL programming?
> > > >>> In VRR TG mode with optimized guardband, the guardband is shortened and
> > > >>> vblank start is moved to match the delayed vblank position.
> > > >>>
> > > >>> The SCL value which we are currently writing as difference between
> > > >>> delayed vblank and undelayed vblank becomes quite large.
> > > >>>
> > > >>> With this large SCL, the flipline decision boundary which is given by
> > > >>> delayed vblank start and SCL lines is same as the undelayed vblank.
> > > >> Everything should match the undelayed vblank.
> > > >>
> > > >>> It seems that intel_dsb_wait_vblank_delay() (in turn
> > > >>> intel_dsb_wait_usec()) does not behave correctly within the W2 window
> > > >>> (between flipdone decision boundary and delayed vblank start).
> > > >>>
> > > >>> It seems to return prematurely. Since the push bit hasn’t cleared yet,
> > > >>> this leads to DSB poll errors.
> > > >> That doesn't make any sense. That command is supposed to simply wait
> > > >> for the specifid number of microseconds. It should not care at all
> > > >> what is happening with the scanout. If that is not the case then we
> > > >> need to write a synthetic test to reproduce it, and report the
> > > >> problem to the hardware folks.
> > > >
> > > > You are right, on debugging further I noticed that 
> > > > intel_dsb_wait_usec() and intel_dsb_wait_vblank_delay() are working 
> > > > correctly.
> > > >
> > > > Due to large SCL, the the intel_dsb_wait_vblanks() is not waiting till 
> > > > the undelayed vblank but the safe window, apparently undelayed vblank 
> > > > - SCL lines.
> > > >
> > > > We are setting DSB_CHICKEN_REG bits 14-15 : which says: Wait for 
> > > > Vblank instruction will use only safe window signal from dptunit in 
> > > > DSB HW to complete the wait for vblank instruction.
> > > >
> > > > I am not exactly sure if its mentioned in Bspec that safe window start 
> > > > = undelayed vblank start - SCL lines.
> > > >
> > > > Observation:
> > > >
> > > > For example with eDP panel VRR range 40-60 and below mode:
> > > >
> > > > Mode: "2880x1800": 60 347710 2880 2928 2960 3040 1800 1803 1809 1906
> > > >
> > > > Before optimization:
> > > >
> > > > guardband = vblank length = 106; Undelayed vblank start =1800; Delayed 
> > > > vblank start = 1906 - 106 = 1800
> > > >
> > > > SCL = 1800 - 1800 = 0
> > > >
> > > > Flipline decision boundary is = 1800
> > > >
> > > > After optimization:
> > > >
> > > > vblank length = 106; guardband = 38; Undelayed Vblank start = 1800; 
> > > > Delayed Vblank start = 1868 (1906 - 38)
> > > >
> > > > SCL = 1868 - 1800 = 68
> > > >
> > > > Flipline decision boundary = 1868 - 68 = 1800
> > > >
> > > > Consider lines in  intel_atomic_dsb_finish() :
> > > >
> > > >  intel_dsb_wait_vblanks(new_crtc_state->dsb_commit, 1);         /* If 
> > > > flip is earlier than 1732 (1800 - 68) this waits till 1732.*/
> > > >
> > > >                 intel_vrr_send_push(new_crtc_state->dsb_commit, 
> > > > new_crtc_state);    /* Push happens immediately*/
> > > >                 intel_dsb_wait_vblank_delay(state, 
> > > > new_crtc_state->dsb_commit);        /* Waits for duration (delayed 
> > > > vblank start - undelayed vblank start) ie. 68 lines ie. till we reach 
> > > > 1732 + 68 = 1800*/
> > > > intel_vrr_check_push_sent(new_crtc_state->dsb_commit,     /* Push is 
> > > > not clear yet as delayed vblank start (1868) is not reach yet, we get 
> > > > DSB POLL error */
> > > >                                           new_crtc_state);
> > > > intel_dsb_interrupt(new_crtc_state->dsb_commit);       /* DSB 
> > > > interrupt is fired earlier */
> > > 
> > > 
> > > Sorry for the bad formatting, perhaps this will be more readable:
> > > 
> > > 
> > >   intel_dsb_wait_vblanks(new_crtc_state->dsb_commit, 1);
> > > 
> > > /* If flip is earlier than 1732 (1800 - 68) this waits till 1732.*/
> > 
> > That does not seem right, or at least it's not how it works on LNL.
> > I just hacked some DSB_STATUS stuff into intel_display_poller [1],
> > and when running that on LNL the safe window always starts at the
> > undelayed vblank.
> > 
> > So if we look at the vmax case then I think the diagram should look
> > like this:
> > 
> >     udelayed vblank 
> >     ^               vmax decision boudnary
> >     |               ^           delayed vblank
> >     |               |           ^                 vmax
> >     |               |           |                 ^
> >     | <- stretch -> | <- scl -> | <- guardband - >|
> >      _______________
> > ..._/               \______________________________... safe window
> > 
> > ... push affects curent frame ->|<- push affects next frame ...
> >                                 |
> > 				v
> > 				push send bit clears if set
> > 
> > And then for the maximum vrefresh case (defined by flipline instead
> > of vnax) the "stretch" part is something between 0 and 
> > delayed_vblank-undelayed_vblank, depending on how we configure SCL.
> > 
> > Additionally if a push is sent during the scl window just
> > after the vmax decisioun bondary, said push will still affect
> > the current frame (ie. such a frame will not have a full
> > scl/w2 window). Only a push sent after the delayed vblank
> > will in fact get deferred to the next frame. That particular
> > scenatio isn't really described in the bspec timing diagrams.
> > Though since we always precede the push with a "wait for safe
> > window" for us the push would get deferred to the next frame
> > anyway.
> 
> Hmm, now that I think about this I think we might have to go with your
> "minimize SCL" approach after all. The problem being that our vblank
> evasion code only evades the undelayed vblank (and a bit before it).
> But with a large SCL the safe window prior to the vmax decision boundary
> may have already ended long before we're even close to crashing into the
> undelayed vblank. Thus we will write all the double buffered registers,
> and they will latch at the vmax undelayed vblank, but the push will
> get deferred into the next frame due to the "wait for safe window".
> 
> I suppose we should really have our vblank evasion code extend the
> evasion scanline window backwards to also cover the SCL.
> 
> I think what we probably need to do is start tracking the scl
> explicitly in the crtc state. And then we'll have to set things up
> in slightly different ways depending on which hw is used:
>  pre-tgl: scl=0, vblank_delay=0
>  tgl: scl=0, vblank_delay configured via VBLANK_START
>  adl+ legacy tg: scl=vblank_delay
>  adl+ vrr tg: scl=whatever(0-vblank_delay), vblank_delay configured via guardband
> 
> Hmm, or maybe we need to also pretend that tgl has scl since after
> a push we need to wait for the scl window to pass, but since tgl
> doesn't have one maybe there we need to actually wait for the vblank
> delay. I think I'll need to poke at a tgl a bit more here to
> figure out exactly how the safe window works there...

OK, done poking TGL. And the conclusion is that it looks to
be close enough to ADL+ that we can treat it almost identically.

- TRANS_SET_CONTEXT_LATENCY doesn't exist (we knew that),
  but the VBLANK_START-VACTIVE diffence plays an identical
  role here, ie. it can be used to create the SCL window ahead
  of the undelayed vblank

- pipeline_full does appear to behave the same as the guardband,
  ie. make it shorter and the undelayed vblank moves forward
  (just sent some patches to hide the differences better)

- safe window starts at undelayed vblank, and ends at
  the start of the vmax SCL window, so same as on ADL+

- vmin/vmax/flipline need to be reduced by the SCL length
  (also sent patches to hide this annoyance better)

- I suppose the only difference we can't completely hide is
  the intel_vrr_extra_vblank_delay() off-by-one issue in the
  hardware

-- 
Ville Syrjälä
Intel