On Tue, 2025-04-22 at 08:07 +0000, Manna, Animesh wrote:
>
>
> > -----Original Message-----
> > From: Hogander, Jouni <jouni.hogan...@intel.com>
> > Sent: Thursday, April 17, 2025 7:12 PM
> > To: intel...@lists.freedesktop.org; Manna, Animesh
> > <animesh.ma...@intel.com>; intel-gfx@lists.freedesktop.org
> > Cc: Nikula, Jani <jani.nik...@intel.com>; B, Jeevan
> > <jeeva...@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.ma...@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);
>
