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

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

On Wed, 2025-04-23 at 14:53 +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 | 25
> ++++++++++++++++++++++-
>  drivers/gpu/drm/i915/display/intel_psr.c  | 23 ---------------------
>  2 files changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c
> b/drivers/gpu/drm/i915/display/intel_alpm.c
> index c4b6da2e6ccf..d1f3852e51b5 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
> @@ -426,6 +426,27 @@ 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
> +	 */

I think you can drop this or add it into intel_psr_needs_alpm. These
details are not interesting here anymore as you added that
intel_psr_needs_alpm.

Consider what you do with this comment. Anyways from my point of view:

Reviewed-by: Jouni Högander <jouni.hogander@intel.com>

> +	if (!intel_psr_needs_alpm(intel_dp, crtc_state) &&
> !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)))
> +		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)
>  {
> @@ -449,8 +470,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 1c5ec3f0b066..b034cb2e0975 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);