Re: [PATCH 5/5] drm/i915/alpm: Allow LOBF only if window1 > alpm check_entry lines

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

On 10/30/2025 4:05 PM, Ville Syrjälä wrote:
> On Thu, Oct 30, 2025 at 10:47:55AM +0530, Ankit Nautiyal wrote:
>> LOBF must be disabled if the number of lines within Window 1 is not greater
>> than ALPM_CTL[ALPM Entry Check]
>>
>> Bspec:71041
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_alpm.c | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c b/drivers/gpu/drm/i915/display/intel_alpm.c
>> index 1c240dd8d668..58cd0d2a4395 100644
>> --- a/drivers/gpu/drm/i915/display/intel_alpm.c
>> +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
>> @@ -255,6 +255,7 @@ void intel_alpm_lobf_compute_config_late(struct intel_dp *intel_dp,
>>   	struct intel_display *display = to_intel_display(intel_dp);
>>   	struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
>>   	int waketime_in_lines, first_sdp_position;
>> +	int window1;
>>   
>>   	if (intel_dp->alpm.lobf_disable_debug) {
>>   		drm_dbg_kms(display->drm, "LOBF is disabled by debug flag\n");
>> @@ -287,6 +288,18 @@ void intel_alpm_lobf_compute_config_late(struct intel_dp *intel_dp,
>>   	if (!intel_alpm_compute_params(intel_dp, crtc_state))
>>   		return;
>>   
>> +	/*
>> +	 * LOBF must be disabled if the number of lines within Window 1 is not
>> +	 * greater than ALPM_CTL[ALPM Entry Check]
>> +	 */
>> +	window1 = adjusted_mode->crtc_vtotal -
>> +		  (adjusted_mode->crtc_vdisplay +
>> +		   crtc_state->vrr.guardband +
>> +		   crtc_state->set_context_latency);
> vdisplay+guardband+scl isn't a value that means anything.
> So the extra parentheses make this rather confusing.

Yes true, vdisplay+guardband+scl doesnt make any sense in itself. What I 
intended was:

vblank = (adjusted_mode->crtc_vtotal - adjusted_mode->crtc_vdisplay);
window1 = vblank - (crtc_state->vrr.guardband + 
crtc_state->set_context_latency);

>
> This also doesn't account for the case where SCL would start already
> in vertical active on PTL+. I know we don't do that currently, but we
> should probably write this in a way that accounts for that so that we
> don't have to rewrite this yet again.

Fair point. Then something like:

vblank = (adjusted_mode->crtc_vtotal - adjusted_mode->crtc_vdisplay);

scl_gb_region = min(vblank, crtc_state->vrr.guardband + 
crtc_state->set_context_latency)
window1 = vblank - scl_gb_region;

>
> Apparently this lobf stuff depends on actually having a window1,
> which means it can only be used if we're using the VRR timing generator
> with an optimized guardband. That means it never did anything (or it
> did things wrong) until you enabled the optimized guardband.

Yeah right. Without optimized guardband, window1 would be always 0 so no 
scope to use this.


>
> It sort of looks like intel_alpm_{pre,post}_plane_update() might
> allow us to enable this even for the 'vrr.enable==true &&
> always_use_vrr_tg()==false' case. Either that or we should just
> enable it for the always_use_vrr_tg()==true case. Whatever the
> case, we definitely seem to be missing some kind of a VRR check
> here.

I was just short of adding check for always_use_vrr_tg() && 
intel_vrr_is_fixed_rr()

Bspec says that lobf can be enabled only with fixed refresh rate and 
mentions flipline = vmin =vmax.

I was not sure if its implicit to enable this only with VRR TG and not 
with legacy timing generator.

Makes sense to replace the existing check with for flipline=vmin=vmax with:

intel_vrr_always_use_vrr_tg() && intel_vrr_is_fixed_rr()


>
> I'm also 90% sure that alpm pre/post plane stuff isn't handling
> DRRS/LRR/etc correctly...

Hmm... ｡oO( need to check how DRRS and LRR interact with ALPM pre/post 
plane handling... )


Regards,

Ankit

>
>> +
>> +	if (window1 <= crtc_state->alpm_state.check_entry_lines)
>> +		return;
>> +
>>   	/*
>>   	 * LOBF can only be enabled if the time from the start of the SCL+Guardband
>>   	 * window to the position of the first SDP is greater than the time it takes
>> -- 
>> 2.45.2