Re: [PATCH 15/15] drm/i915/display: Use optimized guardband to set vblank start

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

On 9/29/2025 2:15 PM, Ville Syrjälä wrote:
> On Sun, Sep 28, 2025 at 12:35:40PM +0530, Ankit Nautiyal wrote:
>> +static void intel_crtc_vblank_delay(struct intel_atomic_state *state,
>> +				    struct intel_crtc *crtc)
>> +{
>> +	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;
>> +	int vblank_delay = 0;
>> +
>> +	vblank_delay = intel_crtc_min_guardband_delay(state, crtc);
>> +
>> +	adjusted_mode->crtc_vblank_start += vblank_delay;
> The situation with crtc_vblank_start is already kinda broken,
> and I think we need to fix that first somehow.
>
> Currently crtc_vblank_start is assumed to be the vblank_start
> for the fixed refresh rate case. That value can be different
> from the variable refresh rate case whenever
> always_use_vrr_tg()==false. On icl/tgl it's always different
> due to the extra vblank delay, and also on adl+ it could be
> different if we were to use an optimized guardband.
>
> I think there are a few options how we might solve this:
> 1. keep crtc_vblank_start as is, and make sure every user of it
>     gets adjusted to also deal with the vrr case correctly


Alright, so we avoid changing the vblank_start.
It means for platforms with always_use_vrr_tg()==true we directly set 
the value for guardband. (Currently I was getting it from vmin_vtotal - 
vblank_start)
For platforms ADL+ with always always_use_vrr_tg()== false for the fixed 
refresh rate case, guardband is full vblank_length for variable refresh 
rate set the guardband directly.

As you have mentioned need to check which all places we need vblank_start.

For ICL/TGL we do not use optimization for now, right?
The extra_vblank_delay quirk is already handled while filling the 
registers.


> 2. enable always_use_vrr_tg() whenever there might be switch
>     between vrr and fixed refresh rate, which I think would mean
>     crtc_state->vrr.in_range==true.
I think I didnt get this part:
Do you mean later at some point we move to option 2: 
always_use_vrr_tg()==true for all platforms.(Need to check if we can do 
it for ICL, TGL).



>
> I kinda like option 2 because then we'll be doing the vrr vs.
> fixed refresh rate always the same way. However we haven't really
> tested that mode of operation on the older platforms, so I'd
> rather not bet all your work on that working. If we later run
> into problems with that then we'd have to revert everything.
>
> So I think we should start with option 1, adjust all the
> crtc_vblank_start users approriately (I don't think there are
> too many of them), and adjust crtc_vblank_start to match
> the guardband only when always_use_vrr_tg()==true.

Sure will start with this for now.

Thanks & Regards,

Ankit


>
> After that I think we might still have some potential issues/race
> conditions around the actual vrr <-> fixed refresh rate switch.
> Those might require more work later, or if we decide at that point
> to try option 2 maybe we could sidestep some/all of them.
>