Re: [PATCH 1/5] drm/i915/alpm: Compute LOBF late after guardband is already determined

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

On 12/17/2025 6:41 PM, Hogander, Jouni wrote:
> On Wed, 2025-11-19 at 19:21 +0530, Ankit Nautiyal wrote:
>> Currently intel_alpm_lobf_compute_config() tries to account for
>> guardband +SCL requirements during encoder->compute_config() phase,
>> even before guardband is computed.
>> Also, LOBF depends on crtc_state->has_psr which can be modified in
>> encoder->compute_config_late().
>>
>> Account for lobf requirements while optimizing the guardband and add
>> checks for final guardband in encoder->compute_config_late() phase
>> after
>> the guardband and the final state of crtc_state->has_psr are already
>> computed.
>>
>> Use crtc_state->vrr.guardband and crtc_state->set_context_latency for
>> the computation and add more documentation for the dependency of
>> first
>> sdp position, guardband, set context latency and wake lines.
>>
>> v2: Add helper to use min guardband required for lobf.
>>
>> Bspec:71041
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_alpm.c  | 63 +++++++++++++++++---
>> --
>>   drivers/gpu/drm/i915/display/intel_alpm.h  |  3 ++
>>   drivers/gpu/drm/i915/display/intel_dp.c    |  2 +
>>   drivers/gpu/drm/i915/display/intel_intel.c |  0
>>   drivers/gpu/drm/i915/display/intel_vrr.c   |  2 +
>>   5 files changed, 56 insertions(+), 14 deletions(-)
>>   create mode 100644 drivers/gpu/drm/i915/display/intel_intel.c
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c
>> b/drivers/gpu/drm/i915/display/intel_alpm.c
>> index 6372f533f65b..98cbf5dde73b 100644
>> --- a/drivers/gpu/drm/i915/display/intel_alpm.c
>> +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
>> @@ -15,6 +15,7 @@
>>   #include "intel_dp_aux.h"
>>   #include "intel_psr.h"
>>   #include "intel_psr_regs.h"
>> +#include "intel_vrr.h"
>>   
>>   #define SILENCE_PERIOD_MIN_TIME	80
>>   #define SILENCE_PERIOD_MAX_TIME	180
>> @@ -248,14 +249,58 @@ bool intel_alpm_compute_params(struct intel_dp
>> *intel_dp,
>>   	return true;
>>   }
>>   
>> +int intel_alpm_lobf_min_guardband(struct intel_crtc_state
>> *crtc_state)
>> +{
>> +	struct drm_display_mode *adjusted_mode = &crtc_state-
>>> hw.adjusted_mode;
>> +	int first_sdp_position = adjusted_mode->crtc_vtotal -
>> +				 adjusted_mode->crtc_vsync_start;
>> +	int waketime_in_lines = max(crtc_state-
>>> alpm_state.io_wake_lines,
>> +				    crtc_state-
>>> alpm_state.aux_less_wake_lines);
> I think we should fix this to use proper value instead of just max. At
> this point FIXME would be enough.
I agree need to check if io_wake_lines or aux_less_wake_lines is to be 
used based on feature.

But currently this information is not available in crtc_state, so went 
ahead with max for now.

I will add a #FIXME note here and send new version.



>
>> +
>> +	if (!crtc_state->has_lobf)
>> +		return 0;
>> +
>> +	return first_sdp_position + waketime_in_lines + crtc_state-
>>> set_context_latency;
>> +}
>> +
>> +void intel_alpm_lobf_compute_config_late(struct intel_dp *intel_dp,
>> +					 struct intel_crtc_state
>> *crtc_state)
>> +{
>> +	struct drm_display_mode *adjusted_mode = &crtc_state-
>>> hw.adjusted_mode;
>> +	int waketime_in_lines, first_sdp_position;
>> +
>> +	if (!crtc_state->has_lobf)
>> +		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
>> +	 * to wake the main link.
>> +	 *
>> +	 * Position of first sdp : vsync_start
>> +	 * start of scl + guardband : vtotal - (scl + guardband)
>> +	 * time in lines to wake main link : waketime_in_lines
>> +	 *
>> +	 * Position of first sdp - start of (scl + guardband) > time
>> in lines to wake main link
>> +	 * vsync_start - (vtotal - (scl + guardband)) >
>> waketime_in_lines
>> +	 * vsync_start - vtotal + scl + guardband >
>> waketime_in_lines
>> +	 * scl + guardband > waketime_in_lines + (vtotal -
>> vsync_start)
>> +	 */
>> +	first_sdp_position = adjusted_mode->crtc_vtotal -
>> adjusted_mode->crtc_vsync_start;
>> +	if (intel_alpm_aux_less_wake_supported(intel_dp))
>> +		waketime_in_lines = crtc_state-
>>> alpm_state.io_wake_lines;
>> +	else
>> +		waketime_in_lines = crtc_state-
>>> alpm_state.aux_less_wake_lines;
>> +
>> +	crtc_state->has_lobf = (crtc_state->set_context_latency +
>> crtc_state->vrr.guardband) >
>> +			       (first_sdp_position +
>> waketime_in_lines);
> Now if crtc_state->has_lobf is switching to false at this point we
> still have lobf guardband requirement included in our optimized
> guardband. Is that ok?

I think that's should be alright.

When we optimize the guardband we assume all features might be enabled 
and take min(vblank, optimized guardband).

If at last guardband is not matching means we went with full vblank 
length, so we should be good.

Regards,

Ankit


> BR,
> Jouni Högander
>
>> +}
>> +
>>   void intel_alpm_lobf_compute_config(struct intel_dp *intel_dp,
>>   				    struct intel_crtc_state
>> *crtc_state,
>>   				    struct drm_connector_state
>> *conn_state)
>>   {
>>   	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 context_latency, guardband;
>>   
>>   	if (intel_dp->alpm.lobf_disable_debug) {
>>   		drm_dbg_kms(display->drm, "LOBF is disabled by debug
>> flag\n");
>> @@ -288,17 +333,7 @@ void intel_alpm_lobf_compute_config(struct
>> intel_dp *intel_dp,
>>   	if (!intel_alpm_compute_params(intel_dp, crtc_state))
>>   		return;
>>   
>> -	context_latency = adjusted_mode->crtc_vblank_start -
>> adjusted_mode->crtc_vdisplay;
>> -	guardband = adjusted_mode->crtc_vtotal -
>> -		    adjusted_mode->crtc_vdisplay - context_latency;
>> -	first_sdp_position = adjusted_mode->crtc_vtotal -
>> adjusted_mode->crtc_vsync_start;
>> -	if (intel_alpm_aux_less_wake_supported(intel_dp))
>> -		waketime_in_lines = crtc_state-
>>> alpm_state.io_wake_lines;
>> -	else
>> -		waketime_in_lines = crtc_state-
>>> alpm_state.aux_less_wake_lines;
>> -
>> -	crtc_state->has_lobf = (context_latency + guardband) >
>> -		(first_sdp_position + waketime_in_lines);
>> +	crtc_state->has_lobf = true;
>>   }
>>   
>>   static void lnl_alpm_configure(struct intel_dp *intel_dp,
>> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.h
>> b/drivers/gpu/drm/i915/display/intel_alpm.h
>> index 53599b464dea..14dc49fee4c3 100644
>> --- a/drivers/gpu/drm/i915/display/intel_alpm.h
>> +++ b/drivers/gpu/drm/i915/display/intel_alpm.h
>> @@ -38,4 +38,7 @@ bool intel_alpm_is_alpm_aux_less(struct intel_dp
>> *intel_dp,
>>   				 const struct intel_crtc_state
>> *crtc_state);
>>   void intel_alpm_disable(struct intel_dp *intel_dp);
>>   bool intel_alpm_get_error(struct intel_dp *intel_dp);
>> +void intel_alpm_lobf_compute_config_late(struct intel_dp *intel_dp,
>> +					 struct intel_crtc_state
>> *crtc_state);
>> +int intel_alpm_lobf_min_guardband(struct intel_crtc_state
>> *crtc_state);
>>   #endif
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
>> b/drivers/gpu/drm/i915/display/intel_dp.c
>> index 0ec82fcbcf48..782e981bbc89 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -7020,6 +7020,8 @@ int intel_dp_compute_config_late(struct
>> intel_encoder *encoder,
>>   	if (ret)
>>   		return ret;
>>   
>> +	intel_alpm_lobf_compute_config_late(intel_dp, crtc_state);
>> +
>>   	return 0;
>>   }
>>   
>> diff --git a/drivers/gpu/drm/i915/display/intel_intel.c
>> b/drivers/gpu/drm/i915/display/intel_intel.c
>> new file mode 100644
>> index 000000000000..e69de29bb2d1
>> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c
>> b/drivers/gpu/drm/i915/display/intel_vrr.c
>> index b92c42fde937..fcecdf3dc78c 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
>> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
>> @@ -6,6 +6,7 @@
>>   
>>   #include <drm/drm_print.h>
>>   
>> +#include "intel_alpm.h"
>>   #include "intel_de.h"
>>   #include "intel_display_regs.h"
>>   #include "intel_display_types.h"
>> @@ -451,6 +452,7 @@ int intel_vrr_compute_optimized_guardband(struct
>> intel_crtc_state *crtc_state)
>>   	if (intel_crtc_has_dp_encoder(crtc_state)) {
>>   		guardband = max(guardband,
>> intel_psr_min_guardband(crtc_state));
>>   		guardband = max(guardband,
>> intel_dp_sdp_min_guardband(crtc_state, true));
>> +		guardband = max(guardband,
>> intel_alpm_lobf_min_guardband(crtc_state));
>>   	}
>>   
>>   	return guardband;