Re: [PATCH 13/16] drm/i915/display: Add vblank_start adjustment logic for always-on VRR TG

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

On 10/7/2025 1:26 AM, Ville Syrjälä wrote:
> On Mon, Oct 06, 2025 at 09:58:49AM +0530, Ankit Nautiyal wrote:
>> As we move towards using a shorter, optimized guardband, we need to adjust
>> how the delayed vblank start is computed.
>>
>> Introduce intel_crtc_compute_vrr_guardband() to handle guardband
>> computation and apply vblank_start adjustment for platforms that always use
>> the VRR timing generator.
>>
>> This function wraps the existing intel_vrr_compute_guardband() and adjusts
>> crtc_vblank_start using (vblank_length - guardband) only when
>> intel_vrr_always_use_vrr_tg() is true. Since the guardband is not yet
>> optimized, the adjustment currently evaluates to zero, preserving existing
>> behavior.
>>
>> This paves way for guardband optimization, by handling the movement of
>> the crtc_vblank_start for platforms that have VRR TG always active.
>>
>> Also update allow_vblank_delay_fastset() to permit vblank delay adjustments
>> during fastboot when VRR TG is always active, even without inherited state.
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_display.c | 33 ++++++++++++++++++--
>>   1 file changed, 30 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index b2d4e24fd7c6..1964e41b5704 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -2403,6 +2403,27 @@ static int intel_crtc_compute_set_context_latency(struct intel_atomic_state *sta
>>   	return 0;
>>   }
>>   
>> +static void intel_crtc_compute_vrr_guardband(struct intel_atomic_state *state,
>> +					     struct intel_crtc *crtc)
> Why this wrapper? You could just stick the adjustemnt into
> intel_vrr_compute_guardband().


The idea was to prepare for the optimized guardband which needs 
connector also.

In subsequent patch I am getting the connector here to use the optimized 
guardband only for platforms with always_use_vrr_tg=true.
And at last I am making changes in intel_vrr_compute_guardband() itself.

As for this patch I can just avoid the wrapper and just use the adjustment.

>
>> +{
>> +	struct intel_display *display = to_intel_display(state);
>> +	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;
>> +
>> +	intel_vrr_compute_guardband(crtc_state);
>> +
>> +	if (intel_vrr_always_use_vrr_tg(display)) {
>> +		int vblank_length = adjusted_mode->crtc_vtotal -
>> +				    (crtc_state->set_context_latency +
>> +				     adjusted_mode->crtc_vdisplay);
>> +
>> +		adjusted_mode->crtc_vblank_start +=
>> +			vblank_length - crtc_state->vrr.guardband;
> Why aren't you using the same 'vblank_start = vtotal-guardband' here as
> during readout?

Hmm I was thinking this more as change in the vblank_start. In 
compute_set_context_latency we move the vblank_start by SCL lines. Here 
we move further as much amount as the change in guardband.


But I guess that is not very intuitive, so I will just set 
crtc_vblank_start as vtotal - guardband here.


>
>> +	}
>> +}
>> +
>>   static int intel_crtc_compute_config(struct intel_atomic_state *state,
>>   				     struct intel_crtc *crtc)
>>   {
>> @@ -2414,7 +2435,7 @@ static int intel_crtc_compute_config(struct intel_atomic_state *state,
>>   	if (ret)
>>   		return ret;
>>   
>> -	intel_vrr_compute_guardband(crtc_state);
>> +	intel_crtc_compute_vrr_guardband(state, crtc);
>>   
>>   	ret = intel_dpll_crtc_compute_clock(state, crtc);
>>   	if (ret)
>> @@ -5105,9 +5126,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,
>> +	 * which implies 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);
> This part doesn't seem directly related to the making crtc_vblank_start
> correct. We still use the non-optimzied guardband so crtc_vblank_start
> should not be changing during normal runtime operation.


Yes we do not need this at this time, but only when we really start 
using optimized guardband.
I can make it as a separate function.

Regards,

Ankit


>>   }
>>   
>>   bool
>> -- 
>> 2.45.2