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

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

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.*/


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, new_crtc_state);

/* Push is not clear yet as delayed vblank start (1868) is not reach 
yet, we get DSB POLL error */


intel_dsb_interrupt(new_crtc_state->dsb_commit);

/* DSB interrupt is fired earlier */



>
>
> This explains the observation that if we call 
> intel_dsb_wait_vblank_delay() twice, it reaches delayed vblank and works.
>
> So I think we dont need to set SCL as the difference between undelayed 
> vblank - delayed vblank and clamp it between 0,1.
>
> In any case the above lines should be modified to something like:
>
> intel_dsb_wait_vblanks()
>
> intel_vrr_send_push()
>
> wait for scanline in range [delayed vblank start, vmax decision 
> boundary] /* To ensure we have atelast past the delayed vblank start */
>
> wait for SCL lines /* Only for VRR case, to ensure send push is 
> cleared */
>
> intel_vrr_check_push_sent()
>
> intel_dsb_interrupt(new_crtc_state->dsb_commit);


intel_dsb_wait_vblanks()

intel_vrr_send_push()

wait for scanline in range [delayed vblank start, vmax decision boundary]

/* To ensure we have atelast past the delayed vblank start */

wait for SCL lines

/* Only for VRR case, to ensure send push is cleared */

intel_vrr_check_push_sent()

intel_dsb_interrupt(new_crtc_state->dsb_commit);


Regards,

Ankit

>
>
>>
>>> AFAIU we are not using the SCL (Set Context Latency) lines to write
>>> registers via DSB.
>>>
>>> The evasion logic ensures we write within a separate window, making the
>>> actual SCL value less critical for register programming.
>>>
>>> So I have clamped the SCL value to (0,1). With this after the push is
>>> sent the send push bit is cleared after (0,1) lines.
>>>
>>> But we still need to wait for the delayed vblank. For this we need
>>> either intel_dsb_wait_vblank_delay() or dsb_wait_for_scanline_in().
>>>
>>>
>>> Do you have any ideas, what could have been going wrong or if anything
>>> we might have been missing?
>> Was your crtc_vblank_start even correct (== undelayed vblank)
>> when you were testing that?
>
> I think the new crtc_vblank_start should be equal to the delayed vblank.
>
> So we get guardband = Vmin Vtotal (=Flipline Vtotal) - 
> crtc_vblank_start as mentioned in below lines which I am not changing 
> now.
>
>                 crtc_state->vrr.guardband =
>                         crtc_state->vrr.vmin - 
> adjusted_mode->crtc_vblank_start;
>
>
> Regards,
>
> Ankit
>
>>