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

[Manna, Animesh]

> -----Original Message-----
> From: Hogander, Jouni <jouni.hogander at intel.com>
> Sent: Thursday, April 17, 2025 7:12 PM
> To: intel-xe at lists.freedesktop.org; Manna, Animesh
> <animesh.manna at intel.com>; intel-gfx at lists.freedesktop.org
> Cc: Nikula, Jani <jani.nikula at intel.com>; B, Jeevan <jeevan.b at intel.com>
> Subject: Re: [PATCH v9 10/10] drm/i915/display: Disintegrate sink alpm
> enable from psr with lobf
> 
> 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 at 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?

As we are trying to disintegrate alpm from psr, intel_psr_needs_alpm() cannot be used for lobf.
So we need two different api - intel_lobf_needs_alpm() and one for psr.
Do you want me to add the above or something better. 

Regards,
Animesh

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