[PATCH] drm/i915/alpm: Introduce has_alpm to simplify alpm check in enable/disable

[Hogander, Jouni]

On Mon, 2025-04-28 at 15:28 +0530, Animesh Manna wrote:
> Simplify alpm check in enable/disable with has_alpm.
> Add a check for alpm during lobf disable which can be enabled
> with panel replay/psr2.
> 
> Suggested-by: Jouni Högander <jouni.hogander at intel.com>
> Signed-off-by: Animesh Manna <animesh.manna at intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_alpm.c     | 23 +++++++++++++----
> --
>  .../drm/i915/display/intel_display_types.h    |  3 +++
>  drivers/gpu/drm/i915/display/intel_psr.c      |  2 ++
>  3 files changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c
> b/drivers/gpu/drm/i915/display/intel_alpm.c
> index 1bf08b80c23f..aa3f442bf8bd 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
> @@ -322,6 +322,8 @@ void intel_alpm_lobf_compute_config(struct
> intel_dp *intel_dp,
>  
>  	crtc_state->has_lobf = (context_latency + guardband) >
>  		(first_sdp_position + waketime_in_lines);
> +
> +	crtc_state->has_alpm |= crtc_state->has_lobf;
>  }
>  
>  static void lnl_alpm_configure(struct intel_dp *intel_dp,
> @@ -332,8 +334,7 @@ static void lnl_alpm_configure(struct intel_dp
> *intel_dp,
>  	enum port port = dp_to_dig_port(intel_dp)->base.port;
>  	u32 alpm_ctl;
>  
> -	if (DISPLAY_VER(display) < 20 ||
> (!intel_psr_needs_alpm(intel_dp, crtc_state) &&
> -					  !crtc_state->has_lobf))
> +	if (DISPLAY_VER(display) < 20 || !crtc_state->has_alpm)
>  		return;
>  
>  	mutex_lock(&intel_dp->alpm_parameters.lock);
> @@ -417,12 +418,20 @@ 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_parameters.lock);
> +		mutex_lock(&intel_dp->alpm_parameters.lock);
> +		if (crtc_state->has_alpm) {
> +			u32 alpm_ctl = intel_de_read(display,
> ALPM_CTL(display, cpu_transcoder));
> +			if (alpm_ctl & ALPM_CTL_LOBF_ENABLE) {
> +				alpm_ctl &= ~ALPM_CTL_LOBF_ENABLE;
> +				intel_de_write(display,
> ALPM_CTL(display, cpu_transcoder), alpm_ctl);
> +				drm_dbg_kms(display->drm, "Link off
> between frames (LOBF) disabled\n");
> +			}
> +		} else {

I think you can now rely on has_lobf and has_alpm and use
intel_alpm_configure to disable only LOBF if needed.

>  			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_parameters.lock);
> +			drm_dbg_kms(display->drm,
> +				    "Link off between frames (LOBF)
> with ALPM disabled\n");
>  		}
> +		mutex_unlock(&intel_dp->alpm_parameters.lock);
>  	}
>  }
>  
> @@ -431,7 +440,7 @@ static void intel_alpm_enable_sink(struct
> intel_dp *intel_dp,
>  {
>  	u8 val;
>  
> -	if (!intel_psr_needs_alpm(intel_dp, crtc_state) &&
> !crtc_state->has_lobf)
> +	if (!crtc_state->has_alpm)
>  		return;
>  
>  	val = DP_ALPM_ENABLE | DP_ALPM_LOCK_ERROR_IRQ_HPD_ENABLE;
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 7415564d058a..6edcfa5d9c41 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1328,6 +1328,9 @@ struct intel_crtc_state {
>  
>  	/* LOBF flag */
>  	bool has_lobf;
> +
> +	/* ALPM flag */
> +	bool has_alpm;
>  };
>  
>  enum intel_pipe_crc_source {
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> b/drivers/gpu/drm/i915/display/intel_psr.c
> index ccd66bbc72f7..e643f36057f8 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -1707,6 +1707,8 @@ void intel_psr_compute_config(struct intel_dp
> *intel_dp,
>  
>  	crtc_state->has_sel_update =
> intel_sel_update_config_valid(intel_dp, crtc_state);
>  
> +	crtc_state->has_alpm = intel_psr_needs_alpm(intel_dp,
> crtc_state);
> +

As you are doing this you could now add intel_alpm_compute_config and
have this in there as well? If doing this, you can remove 
intel_alpm_compute_params call from intel_psr.c::alpm_config_valid and
from intel_alpm.c::intel_alpm_lobf_compute_config. This would be real
nice cleanup.

BR,

Jouni Högander

>  	/* Wa_18037818876 */
>  	if (intel_psr_needs_wa_18037818876(intel_dp, crtc_state)) {
>  		crtc_state->has_psr = false;