| Message ID | 20241204100328.3738778-2-animesh.manna@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | LOBF enablement fix | expand |
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@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? 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)
> -----Original Message----- > From: Hogander, Jouni <jouni.hogander@intel.com> > Sent: Monday, December 9, 2024 2:06 PM > To: Manna, Animesh <animesh.manna@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: Nikula, Jani <jani.nikula@intel.com>; B, Jeevan <jeevan.b@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@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. 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)
On Mon, 2024-12-09 at 11:19 +0000, Manna, Animesh wrote: > > > > -----Original Message----- > > From: Hogander, Jouni <jouni.hogander@intel.com> > > Sent: Monday, December 9, 2024 2:06 PM > > To: Manna, Animesh <animesh.manna@intel.com>; intel- > > gfx@lists.freedesktop.org > > Cc: Nikula, Jani <jani.nikula@intel.com>; B, Jeevan > > <jeevan.b@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@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) >
> -----Original Message----- > From: Hogander, Jouni <jouni.hogander@intel.com> > Sent: Monday, December 9, 2024 5:04 PM > To: Manna, Animesh <animesh.manna@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: Nikula, Jani <jani.nikula@intel.com>; B, Jeevan <jeevan.b@intel.com> > 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@intel.com> > > > Sent: Monday, December 9, 2024 2:06 PM > > > To: Manna, Animesh <animesh.manna@intel.com>; intel- > > > gfx@lists.freedesktop.org > > > Cc: Nikula, Jani <jani.nikula@intel.com>; B, Jeevan > > > <jeevan.b@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@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. 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) > >
On Mon, 2024-12-09 at 11:54 +0000, Manna, Animesh wrote: > > > > -----Original Message----- > > From: Hogander, Jouni <jouni.hogander@intel.com> > > Sent: Monday, December 9, 2024 5:04 PM > > To: Manna, Animesh <animesh.manna@intel.com>; intel- > > gfx@lists.freedesktop.org > > Cc: Nikula, Jani <jani.nikula@intel.com>; B, Jeevan > > <jeevan.b@intel.com> > > 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@intel.com> > > > > Sent: Monday, December 9, 2024 2:06 PM > > > > To: Manna, Animesh <animesh.manna@intel.com>; intel- > > > > gfx@lists.freedesktop.org > > > > Cc: Nikula, Jani <jani.nikula@intel.com>; B, Jeevan > > > > <jeevan.b@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@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. > > 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. Please note that it is not about just resetting the flag. Someone has to reset lobf enable bit in ALPM_CTL as well. BR, Jouni Högander > > 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) > > > >
> -----Original Message----- > From: Hogander, Jouni <jouni.hogander@intel.com> > Sent: Monday, December 9, 2024 5:29 PM > To: Manna, Animesh <animesh.manna@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: Nikula, Jani <jani.nikula@intel.com>; B, Jeevan <jeevan.b@intel.com> > Subject: Re: [PATCH v2 1/2] drm/i915/lobf: Add lobf enablement in post > plane update > > On Mon, 2024-12-09 at 11:54 +0000, Manna, Animesh wrote: > > > > > > > -----Original Message----- > > > From: Hogander, Jouni <jouni.hogander@intel.com> > > > Sent: Monday, December 9, 2024 5:04 PM > > > To: Manna, Animesh <animesh.manna@intel.com>; intel- > > > gfx@lists.freedesktop.org > > > Cc: Nikula, Jani <jani.nikula@intel.com>; B, Jeevan > > > <jeevan.b@intel.com> > > > 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@intel.com> > > > > > Sent: Monday, December 9, 2024 2:06 PM > > > > > To: Manna, Animesh <animesh.manna@intel.com>; intel- > > > > > gfx@lists.freedesktop.org > > > > > Cc: Nikula, Jani <jani.nikula@intel.com>; B, Jeevan > > > > > <jeevan.b@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@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. > > > > 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. > > Please note that it is not about just resetting the flag. Someone has to reset > lobf enable bit in ALPM_CTL as well. Agree, will add a check for (old_crtc_state->has_lobf && !new_crtc_state->has_lobf) to clear the bitfield of ALPM_CTL. Regards, Animesh > > BR, > > Jouni Högander > > > > > 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) > > > > > >
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); + } +} + 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)
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@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(+)