> -----Original Message----- > From: Hogander, Jouni <jouni.hogander@xxxxxxxxx> > Sent: Monday, December 9, 2024 5:04 PM > To: Manna, Animesh <animesh.manna@xxxxxxxxx>; intel- > gfx@xxxxxxxxxxxxxxxxxxxxx > Cc: Nikula, Jani <jani.nikula@xxxxxxxxx>; B, Jeevan <jeevan.b@xxxxxxxxx> > Subject: Re: [PATCH v2 1/2] drm/i915/lobf: Add lobf enablement in post > plane update > > On Mon, 2024-12-09 at 11:19 +0000, Manna, Animesh wrote: > > > > > > > -----Original Message----- > > > From: Hogander, Jouni <jouni.hogander@xxxxxxxxx> > > > Sent: Monday, December 9, 2024 2:06 PM > > > To: Manna, Animesh <animesh.manna@xxxxxxxxx>; intel- > > > gfx@xxxxxxxxxxxxxxxxxxxxx > > > Cc: Nikula, Jani <jani.nikula@xxxxxxxxx>; B, Jeevan > > > <jeevan.b@xxxxxxxxx> > > > 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@xxxxxxxxx> > > > > --- > > > > 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. Currently VRR fixed refresh mode check is not added in this patch series which can take care of resetting the has_lobf flag as well, thanks for catching, will add in next version. Hope with the above change the below comments will be addressed. Regards, Animesh > > 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) > >