Re: [PATCH 4/5] drm/i915/alpm: Simplify and align LOBF checks in pre/post plane update

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

On 12/17/2025 7:19 PM, Hogander, Jouni wrote:
> On Wed, 2025-11-19 at 19:21 +0530, Ankit Nautiyal wrote:
>> The pre_plane_update and post_plane_update hooks essentially
>> disable/enable lobf feature. Use the existing
>> _is_enabling/is_disabling
>> logic for this in the pre_plane_update and post_plane_update paths.
>>
>> Also rename the helpers to intel_alpm_lobf_{en,dis}able().
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_alpm.c    | 45 ++++++------------
>> --
>>   drivers/gpu/drm/i915/display/intel_alpm.h    |  6 +--
>>   drivers/gpu/drm/i915/display/intel_display.c | 28 +++++++++++-
>>   3 files changed, 41 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c
>> b/drivers/gpu/drm/i915/display/intel_alpm.c
>> index 5cfb9345776a..abeb70fcf92c 100644
>> --- a/drivers/gpu/drm/i915/display/intel_alpm.c
>> +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
>> @@ -447,25 +447,17 @@ void intel_alpm_port_configure(struct intel_dp
>> *intel_dp,
>>   	intel_de_write(display, PORT_ALPM_LFPS_CTL(port),
>> lfps_ctl_val);
>>   }
>>   
>> -void intel_alpm_pre_plane_update(struct intel_atomic_state *state,
>> -				 struct intel_crtc *crtc)
>> +void intel_alpm_lobf_disable(const struct intel_crtc_state
>> *new_crtc_state)
>>   {
>> -	struct intel_display *display = to_intel_display(state);
>> -	const struct intel_crtc_state *crtc_state =
>> -		intel_atomic_get_new_crtc_state(state, crtc);
>> -	const struct intel_crtc_state *old_crtc_state =
>> -		intel_atomic_get_old_crtc_state(state, crtc);
>> -	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>> +	struct intel_display *display =
>> to_intel_display(new_crtc_state);
>> +	enum transcoder cpu_transcoder = new_crtc_state-
>>> cpu_transcoder;
>>   	struct intel_encoder *encoder;
>>   
>>   	if (DISPLAY_VER(display) < 20)
>>   		return;
> You can remove this as well. intel_crtc_lobf_enabling is supposed to be
> true only on display version >= 20.


Ok sure will remove this check.


>
>>   
>> -	if (crtc_state->has_lobf || crtc_state->has_lobf ==
>> old_crtc_state->has_lobf)
>> -		return;
>> -
>>   	for_each_intel_encoder_mask(display->drm, encoder,
>> -				    crtc_state->uapi.encoder_mask) {
>> +				    new_crtc_state-
>>> uapi.encoder_mask) {
>>   		struct intel_dp *intel_dp;
>>   
>>   		if (!intel_encoder_is_dp(encoder))
>> @@ -476,12 +468,10 @@ void intel_alpm_pre_plane_update(struct
>> intel_atomic_state *state,
>>   		if (!intel_dp_is_edp(intel_dp))
>>   			continue;
>>   
>> -		if (old_crtc_state->has_lobf) {
>> -			mutex_lock(&intel_dp->alpm.lock);
>> -			intel_de_write(display, ALPM_CTL(display,
>> cpu_transcoder), 0);
>> -			drm_dbg_kms(display->drm, "Link off between
>> frames (LOBF) disabled\n");
>> -			mutex_unlock(&intel_dp->alpm.lock);
>> -		}
>> +		mutex_lock(&intel_dp->alpm.lock);
>> +		intel_de_write(display, ALPM_CTL(display,
>> cpu_transcoder), 0);
>> +		drm_dbg_kms(display->drm, "Link off between frames
>> (LOBF) disabled\n");
>> +		mutex_unlock(&intel_dp->alpm.lock);
>>   	}
>>   }
>>   
>> @@ -502,22 +492,13 @@ void intel_alpm_enable_sink(struct intel_dp
>> *intel_dp,
>>   	drm_dp_dpcd_writeb(&intel_dp->aux, DP_RECEIVER_ALPM_CONFIG,
>> val);
>>   }
>>   
>> -void intel_alpm_post_plane_update(struct intel_atomic_state *state,
>> -				  struct intel_crtc *crtc)
>> +void intel_alpm_lobf_enable(const struct intel_crtc_state
>> *new_crtc_state)
>>   {
>> -	struct intel_display *display = to_intel_display(state);
>> -	const struct intel_crtc_state *crtc_state =
>> -		intel_atomic_get_new_crtc_state(state, crtc);
>> -	const struct intel_crtc_state *old_crtc_state =
>> -		intel_atomic_get_old_crtc_state(state, crtc);
>> +	struct intel_display *display =
>> to_intel_display(new_crtc_state);
>>   	struct intel_encoder *encoder;
>>   
>> -	if (crtc_state->has_psr || !crtc_state->has_lobf ||
>> -	    crtc_state->has_lobf == old_crtc_state->has_lobf)
>> -		return;
>> -
>>   	for_each_intel_encoder_mask(display->drm, encoder,
>> -				    crtc_state->uapi.encoder_mask) {
>> +				    new_crtc_state-
>>> uapi.encoder_mask) {
>>   		struct intel_dp *intel_dp;
>>   
>>   		if (!intel_encoder_is_dp(encoder))
>> @@ -526,8 +507,8 @@ void intel_alpm_post_plane_update(struct
>> intel_atomic_state *state,
>>   		intel_dp = enc_to_intel_dp(encoder);
>>   
>>   		if (intel_dp_is_edp(intel_dp)) {
>> -			intel_alpm_enable_sink(intel_dp,
>> crtc_state);
>> -			intel_alpm_configure(intel_dp, crtc_state);
>> +			intel_alpm_enable_sink(intel_dp,
>> new_crtc_state);
>> +			intel_alpm_configure(intel_dp,
>> new_crtc_state);
>>   		}
>>   	}
>>   }
>> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.h
>> b/drivers/gpu/drm/i915/display/intel_alpm.h
>> index 14dc49fee4c3..0b2460b90465 100644
>> --- a/drivers/gpu/drm/i915/display/intel_alpm.h
>> +++ b/drivers/gpu/drm/i915/display/intel_alpm.h
>> @@ -25,12 +25,10 @@ void intel_alpm_configure(struct intel_dp
>> *intel_dp,
>>   			  const struct intel_crtc_state
>> *crtc_state);
>>   void intel_alpm_enable_sink(struct intel_dp *intel_dp,
>>   			    const struct intel_crtc_state
>> *crtc_state);
>> -void intel_alpm_pre_plane_update(struct intel_atomic_state *state,
>> -				 struct intel_crtc *crtc);
>> +void intel_alpm_lobf_disable(const struct intel_crtc_state
>> *new_crtc_state);
>>   void intel_alpm_port_configure(struct intel_dp *intel_dp,
>>   			       const struct intel_crtc_state
>> *crtc_state);
>> -void intel_alpm_post_plane_update(struct intel_atomic_state *state,
>> -				  struct intel_crtc *crtc);
>> +void intel_alpm_lobf_enable(const struct intel_crtc_state
>> *new_crtc_state);
>>   void intel_alpm_lobf_debugfs_add(struct intel_connector *connector);
>>   bool intel_alpm_aux_wake_supported(struct intel_dp *intel_dp);
>>   bool intel_alpm_aux_less_wake_supported(struct intel_dp *intel_dp);
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
>> b/drivers/gpu/drm/i915/display/intel_display.c
>> index 069967114bd9..ecbdc506a1fc 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -1009,6 +1009,27 @@ static bool intel_casf_disabling(const struct
>> intel_crtc_state *old_crtc_state,
>>   	return is_disabling(hw.casf_params.casf_enable,
>> old_crtc_state, new_crtc_state);
>>   }
>>   
>> +static bool intel_crtc_lobf_enabling(const struct intel_crtc_state
>> *old_crtc_state,
>> +				     const struct intel_crtc_state
>> *new_crtc_state)
>> +{
>> +	if (!new_crtc_state->hw.active)
>> +		return false;
> Is it possible that new_crtc_state->hw.active == false &&
> new_crtc_state->has_lobf == true

I dont think its possible.


>> +
>> +	if (!new_crtc_state->has_psr)
>> +		return false;
> I don't understand why you are checking psr here? I should have been
> checked already on compute phase?

hmm yes right this can be dropped here.


>
>> +
>> +	return is_enabling(has_lobf, old_crtc_state,
>> new_crtc_state);
>> +}
>> +
>> +static bool intel_crtc_lobf_disabling(const struct intel_crtc_state
>> *old_crtc_state,
>> +				      const struct intel_crtc_state
>> *new_crtc_state)
>> +{
>> +	if (!new_crtc_state->hw.active)
>> +		return false;
> No need to disable before everything else is disabled?

Sorry this should be :

        if (!old_crtc_state->hw.active)
                 return false;

If pipe was already inactive we don't need to do anything.

Thanks for catching this.

I will update the patch with suggested changes.

Thanks & Regards,

Ankit

>
> BR,
>
> Jouni Högander
>
>> +
>> +	return is_disabling(has_lobf, old_crtc_state,
>> new_crtc_state);
>> +}
>> +
>>   #undef is_disabling
>>   #undef is_enabling
>>   
>> @@ -1056,7 +1077,8 @@ static void intel_post_plane_update(struct
>> intel_atomic_state *state,
>>   			adl_scaler_ecc_unmask(new_crtc_state);
>>   	}
>>   
>> -	intel_alpm_post_plane_update(state, crtc);
>> +	if (intel_crtc_lobf_enabling(old_crtc_state,
>> new_crtc_state))
>> +		intel_alpm_lobf_enable(new_crtc_state);
>>   
>>   	intel_psr_post_plane_update(state, crtc);
>>   }
>> @@ -1153,7 +1175,9 @@ static void intel_pre_plane_update(struct
>> intel_atomic_state *state,
>>   		intel_atomic_get_new_crtc_state(state, crtc);
>>   	enum pipe pipe = crtc->pipe;
>>   
>> -	intel_alpm_pre_plane_update(state, crtc);
>> +	if (intel_crtc_lobf_disabling(old_crtc_state,
>> new_crtc_state))
>> +		intel_alpm_lobf_disable(new_crtc_state);
>> +
>>   	intel_psr_pre_plane_update(state, crtc);
>>   
>>   	if (intel_crtc_vrr_disabling(state, crtc)) {