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

[Hogander, Jouni]

On Tue, 2025-04-22 at 08:07 +0000, Manna, Animesh wrote:
> 
> 
> > -----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. 

I don't think this is conflicting with the idea of disintegrating alpm
from psr. I.e. keep these dirty details of which versions of PSR/Panel
Replay are using ALPM within PSR code.

BR,

Jouni Högander

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