[PATCH v2 1/2] drm/i915/lobf: Add lobf enablement in post plane update

[Hogander, Jouni]

On Mon, 2024-12-09 at 11:19 +0000, Manna, Animesh wrote:
> 
> 
> > -----Original Message-----
> > From: Hogander, Jouni <jouni.hogander at intel.com>
> > Sent: Monday, December 9, 2024 2:06 PM
> > To: 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 v2 1/2] drm/i915/lobf: Add lobf enablement in
> > post
> > plane update
> > 
> > On Wed, 2024-12-04 at 15:33 +0530, Animesh Manna wrote:
> > > Enablement of LOBF is added in post plane update whenever
> > > has_lobf
> > > flag is set. As LOBF can be enabled in non-psr case as well so
> > > adding
> > > in post plane update. There is no change of configuring alpm with
> > > psr
> > > path.
> > > 
> > > v1: Initial version.
> > > v2: Use encoder-mask to find the associated encoder from crtc-
> > > state.
> > > [Jani]
> > > 
> > > Signed-off-by: Animesh Manna <animesh.manna at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_alpm.c    | 25
> > > ++++++++++++++++++++
> > >  drivers/gpu/drm/i915/display/intel_alpm.h    |  4 ++++
> > >  drivers/gpu/drm/i915/display/intel_display.c |  3 +++
> > >  3 files changed, 32 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c
> > > b/drivers/gpu/drm/i915/display/intel_alpm.c
> > > index 55f3ae1e68c9..75063342a100 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
> > > @@ -367,6 +367,31 @@ void intel_alpm_configure(struct intel_dp
> > > *intel_dp,
> > >         lnl_alpm_configure(intel_dp, crtc_state);
> > >  }
> > > 
> > > +void intel_alpm_post_plane_update(struct intel_atomic_state
> > > *state,
> > > +                                 struct intel_crtc *crtc) {
> > > +       struct intel_display *display = to_intel_display(state);
> > > +       const struct intel_crtc_state *crtc_state =
> > > +               intel_atomic_get_new_crtc_state(state, crtc);
> > > +       struct intel_encoder *encoder;
> > > +
> > > +       if (!crtc_state->has_lobf)
> > > +               return;
> > > +
> > > +       for_each_intel_encoder_mask(display->drm, encoder,
> > > +                                   crtc_state-
> > > >uapi.encoder_mask) {
> > > +               struct intel_dp *intel_dp;
> > > +
> > > +               if (!intel_encoder_is_dp(encoder))
> > > +                       continue;
> > > +
> > > +               intel_dp = enc_to_intel_dp(encoder);
> > > +
> > > +               if (intel_dp_is_edp(intel_dp))
> > > +                       intel_alpm_configure(intel_dp,
> > > crtc_state);
> > 
> > This is reconfiguring lobf even if it's already configured? Is that
> > wanted?
> 
> has_psr and has_lobf are mutually exclusive, so alpm_configure() will
> be called once.

has_psr is unrelated. Consider e.g. case where xe.enable_psr=0 is in
bootcmd. has_psr is always false. ALPM_CTL is written with lobf enabled
on every commit if has_lobf is true.

Another thing here is that lobf is never disabled after it gets
enabled?

Please also review cases where lobf needs to be disabled. E.g. refresh
rate change. See bspec 71041.

BR,

Jouni Högander
> 
> Regards,
> Animesh
>  
> > 
> > BR,
> > 
> > Jouni Högander
> > > +       }
> > > +}
> > > +
> > >  static int i915_edp_lobf_info_show(struct seq_file *m, void
> > > *data)
> > >  {
> > >         struct intel_connector *connector = m->private; diff --
> > > git
> > > a/drivers/gpu/drm/i915/display/intel_alpm.h
> > > b/drivers/gpu/drm/i915/display/intel_alpm.h
> > > index 8c409b10dce6..2f862b0476a8 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_alpm.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_alpm.h
> > > @@ -12,6 +12,8 @@ struct intel_dp;
> > >  struct intel_crtc_state;
> > >  struct drm_connector_state;
> > >  struct intel_connector;
> > > +struct intel_atomic_state;
> > > +struct intel_crtc;
> > > 
> > >  void intel_alpm_init_dpcd(struct intel_dp *intel_dp);
> > >  bool intel_alpm_compute_params(struct intel_dp *intel_dp, @@ -
> > > 21,6
> > > +23,8 @@ void intel_alpm_lobf_compute_config(struct intel_dp
> > > *intel_dp,
> > >                                     struct drm_connector_state
> > > *conn_state);
> > >  void intel_alpm_configure(struct intel_dp *intel_dp,
> > >                           const struct intel_crtc_state
> > > *crtc_state);
> > > +void intel_alpm_post_plane_update(struct intel_atomic_state
> > > *state,
> > > +                                 struct intel_crtc *crtc);
> > >  void intel_alpm_lobf_debugfs_add(struct intel_connector
> > > *connector);
> > >  bool intel_alpm_aux_wake_supported(struct intel_dp *intel_dp);
> > >  bool intel_alpm_aux_less_wake_supported(struct intel_dp
> > > *intel_dp);
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 4805bf682d43..ffd96b187e5e 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -55,6 +55,7 @@
> > >  #include "i9xx_plane.h"
> > >  #include "i9xx_plane_regs.h"
> > >  #include "i9xx_wm.h"
> > > +#include "intel_alpm.h"
> > >  #include "intel_atomic.h"
> > >  #include "intel_atomic_plane.h"
> > >  #include "intel_audio.h"
> > > @@ -1190,6 +1191,8 @@ static void intel_post_plane_update(struct
> > > intel_atomic_state *state,
> > > 
> > >         intel_psr_post_plane_update(state, crtc);
> > > 
> > > +       intel_alpm_post_plane_update(state, crtc);
> > > +
> > >         intel_frontbuffer_flip(dev_priv, new_crtc_state-
> > > >fb_bits);
> > > 
> > >         if (new_crtc_state->update_wm_post && new_crtc_state-
> > > > hw.active)
>