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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 28 Apr 2025, Animesh Manna <animesh.manna@intel.com> 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@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@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;

I'm averse to using bitwise operators on logical booleans.

>  }
>  
>  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 {
>  			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 */

What benefit does this or the above "LOBF flag" comment give?

If you don't know what "has_lobf" or "has_alpm" mean, how on earth would
adding the word "flag" help here?

> +	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);

Looks like the below thing can disable PSR usage, but you'll still leave
has_sel_update and (with this patch) has_alpm enabled. Are you taking
all those combos into account? Including in readout?

BR,
Jani.

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

-- 
Jani Nikula, Intel