Re: [PATCH v9 10/10] drm/i915/display: Disintegrate sink alpm enable from psr with lobf

["Hogander, Jouni" <jouni.hogander@intel.com>]

On Thu, 2025-04-17 at 15:11 +0530, Animesh Manna wrote:
> Make a generic alpm enable function for sink which can be used for
> PSR2/PR/Lobf.
> 
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_alpm.c | 27
> ++++++++++++++++++++++-
>  drivers/gpu/drm/i915/display/intel_psr.c  | 23 -------------------
>  2 files changed, 26 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c
> b/drivers/gpu/drm/i915/display/intel_alpm.c
> index 3d4459881e7c..f4d869953045 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
> @@ -429,6 +429,29 @@ void intel_alpm_pre_plane_update(struct
> intel_atomic_state *state,
>  	}
>  }
>  
> +static void intel_alpm_enable_sink(struct intel_dp *intel_dp,
> +				   const struct intel_crtc_state
> *crtc_state)
> +{
> +	u8 val;
> +
> +	/*
> +	 * eDP Panel Replay uses always ALPM
> +	 * PSR2 uses ALPM but PSR1 doesn't
> +	 */
> +	if (!intel_dp_is_edp(intel_dp) || (!crtc_state-
> >has_panel_replay &&
> +					   !crtc_state-
> >has_sel_update &&
> +					   !crtc_state->has_lobf))
> +		return;
> +
> +	val = DP_ALPM_ENABLE | DP_ALPM_LOCK_ERROR_IRQ_HPD_ENABLE;
> +
> +	if (crtc_state->has_panel_replay || (crtc_state->has_lobf &&
> +					    
> intel_alpm_aux_less_wake_supported(intel_dp)))

I know this is kind of late comment. I'm sorry for that. Instead of
spreading these ugly checks outside PSR code you could add:

intel_psr_needs_alpm(struct intel_dp *intel_dp, const struct
intel_crtc_state *crtc_state))

into intel_psr.[ch] and use it here and other relevant places in
intel_alpm.c. E.g lnl_alpm_configure:

if (DISPLAY_VER(display) < 20 || (!crtc_state->has_sel_update &&
    !intel_dp_is_edp(intel_dp)))
	return;

to

if (DISPLAY_VER(display) < 20 || !intel_psr_needs_alpm(intel_dp,
    crtc_state)))
	return;

and so on. What do you think?

BR,

Jouni Högander

> +		val |= DP_ALPM_MODE_AUX_LESS;
> +
> +	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)
>  {
> @@ -452,8 +475,10 @@ 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))
> +		if (intel_dp_is_edp(intel_dp)) {
> +			intel_alpm_enable_sink(intel_dp,
> crtc_state);
>  			intel_alpm_configure(intel_dp, crtc_state);
> +		}
>  	}
>  }
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> b/drivers/gpu/drm/i915/display/intel_psr.c
> index 43ed166007eb..68952f7bdd7c 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -794,32 +794,9 @@ static void _psr_enable_sink(struct intel_dp
> *intel_dp,
>  	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, val);
>  }
>  
> -static void intel_psr_enable_sink_alpm(struct intel_dp *intel_dp,
> -				       const struct intel_crtc_state
> *crtc_state)
> -{
> -	u8 val;
> -
> -	/*
> -	 * eDP Panel Replay uses always ALPM
> -	 * PSR2 uses ALPM but PSR1 doesn't
> -	 */
> -	if (!intel_dp_is_edp(intel_dp) || (!crtc_state-
> >has_panel_replay &&
> -					   !crtc_state-
> >has_sel_update))
> -		return;
> -
> -	val = DP_ALPM_ENABLE | DP_ALPM_LOCK_ERROR_IRQ_HPD_ENABLE;
> -
> -	if (crtc_state->has_panel_replay)
> -		val |= DP_ALPM_MODE_AUX_LESS;
> -
> -	drm_dp_dpcd_writeb(&intel_dp->aux, DP_RECEIVER_ALPM_CONFIG,
> val);
> -}
> -
>  static void intel_psr_enable_sink(struct intel_dp *intel_dp,
>  				  const struct intel_crtc_state
> *crtc_state)
>  {
> -	intel_psr_enable_sink_alpm(intel_dp, crtc_state);
> -
>  	crtc_state->has_panel_replay ?
>  		_panel_replay_enable_sink(intel_dp, crtc_state) :
>  		_psr_enable_sink(intel_dp, crtc_state);