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

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

On 10/1/2025 5:40 PM, Ville Syrjälä wrote:
> On Wed, Oct 01, 2025 at 04:04:37PM +0530, Nautiyal, Ankit K wrote:
>> 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.
> I think we could just use something like this:
>
> something_something_vblank_len()
> {
> 	if (crtc_state->vrr.enable)
> 		return crtc_state->vrr.guardband;
> 	else
> 		return adjuste_mode.crtc_vtotal -
> 			adjusted_mode.crtc_vblank_start;
> }
>
> That should work fine for for both values of always_use_vrr_tg().

Yep I had something similar in mind.


>
>> 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?
> Effectively we do, because of intel_vrr_extra_vblank_delay().
> The VRR guardband is always one line less than the fixed refresh
> rate guardband. And if we run into the max pipeline full limit
> then the difference would be even larger.

Oh ok, got it.


>
>> 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'm pretty sure I tested always_use_vrr_tg()==true briefly on
> TGL and ADL and it seemed to be working fine.
>
> But I'm a bit concerned becase we originally enabled the VRR timing
> generator earlier during the modeset (but not from the very start IIRC),
> and that wasn't working all that well on some systems. So later I
> changed things to keep running with the legacy TG for the modeset and
> only switch to the VRR TG at the very end. It might have been some MTL
> system where we had these problems actually...
>
> It's of course possible that we've since fixed whatever was the
> underlying cause there. Or maybe the problem really was that we
> started to use the VRR timings already during the modeset. With the
> current always_use_vrr_tg()==true code we will still use the fixed
> refresh rate timings for the modeset, and switch to the VRR timings
> only at the very end.

Hmm I think PSR was giving problem with fixed refresh rate mode for TGL 
when I had tried that, but it was long back.

Need to recheck that.

Thanks for clearing things up.

Regards,

Ankit

>