On Mon, Mar 25, 2024 at 07:01:03PM +0200, Lisovskiy, Stanislav wrote:
> On Mon, Mar 25, 2024 at 04:45:49PM +0200, Ville Syrjälä wrote:
> > On Mon, Mar 25, 2024 at 01:23:26PM +0200, Stanislav Lisovskiy wrote:
> > > According to BSpec we need to do correspondent MBUS updates before
> > > or after DBUF reallocation, depending on whether we are enabling
> > > or disabling mbus joining(typical scenario is swithing between
> > > multiple and single displays).
> > >
> > > Also we need to be able to update dbuf min tracker and mdclk ratio
> > > separately if mbus_join state didn't change, so lets add one
> > > degree of freedom and make it possible.
> > >
> > > Reviewed-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
> > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovs...@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/display/skl_watermark.c | 54 +++++++++++++-------
> > > 1 file changed, 35 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c
> > > b/drivers/gpu/drm/i915/display/skl_watermark.c
> > > index bc341abcab2fe..2b947870527fc 100644
> > > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > > @@ -3570,16 +3570,38 @@ void intel_dbuf_mdclk_cdclk_ratio_update(struct
> > > drm_i915_private *i915, u8 ratio
> > > DBUF_MIN_TRACKER_STATE_SERVICE(ratio - 1));
> > > }
> > >
> > > +static void intel_dbuf_mdclk_min_tracker_update(struct
> > > intel_atomic_state *state)
> > > +{
> > > + struct drm_i915_private *i915 = to_i915(state->base.dev);
> > > + const struct intel_dbuf_state *old_dbuf_state =
> > > + intel_atomic_get_old_dbuf_state(state);
> > > + const struct intel_dbuf_state *new_dbuf_state =
> > > + intel_atomic_get_new_dbuf_state(state);
> > > +
> > > + if (DISPLAY_VER(i915) >= 20 &&
> > > + old_dbuf_state->mdclk_cdclk_ratio !=
> > > new_dbuf_state->mdclk_cdclk_ratio) {
> > > + /*
> > > + * For Xe2LPD and beyond, when there is a change in the ratio
> > > + * between MDCLK and CDCLK, updates to related registers need to
> > > + * happen at a specific point in the CDCLK change sequence. In
> > > + * that case, we defer to the call to
> > > + * intel_dbuf_mdclk_cdclk_ratio_update() to the CDCLK logic.
> > > + */
> > > + return;
> > > + }
> >
> > That still needs to be removed or else we'll not update the ratio at
> > all during the mbus_join changes. I don't think I saw any removal
> > in subsequent patches.
> >
> > > +
> > > + intel_dbuf_mdclk_cdclk_ratio_update(i915,
> > > new_dbuf_state->mdclk_cdclk_ratio,
>
> I don't get what is happening here.
>
> "That whole condition I think needs to go. We want to update the ratio
> also when changing mbus joining. But that behavioural change doesn't
> really belong in this patch, so this is
>
> Reviewed-by: Ville Syrjälä <ville.syrj...@linux.intel.com>"
>
> Now it again needs to be changed or changed in other patch(in this series or
> which one),
> I don't follow.
> Should it be the patch changing MBUS join value?
Yeah, probably should be in the last patch. Perhaps we
could change it before that, but that would need some
extra brain power to make sure it doesn't temporarily
break something. So probably not worth the hassle
to do as a separate patch.
>
> Stan
>
> >
> > And it just occurred to me that this thing will in fact be wrong
> > during the pre/post ddb hooks *and* cdclk is getting decreased
> > from the post plane update hook.
> >
> > I can't immediately think of a super nice way to handle this.
> >
> > Perhaps the most stragithforward idea is to just get the mdclk/cdclk
> > ratio from i915->display.cdclk.hw during the pre/post ddb hooks.
> > cdclk serialization should guard against parallel updates from
> > two both places and thus isplay.cdclk.hw should be safe to use.
> >
> > The other option would be to determine if a cdclk decrease
> > is going to happen or not, and depending on that use the
> > old vs. new dbuf_state when updating the ratio in the
> > pre/post ddb hooks.
> >
> > > + new_dbuf_state->joined_mbus);
> > > +}
> > > +
> > > /*
> > > * Configure MBUS_CTL and all DBUF_CTL_S of each slice to join_mbus
> > > state before
> > > * update the request state of all DBUS slices.
> > > */
> > > -static void update_mbus_pre_enable(struct intel_atomic_state *state)
> > > +static void intel_dbuf_mbus_join_update(struct intel_atomic_state *state)
> > > {
> > > struct drm_i915_private *i915 = to_i915(state->base.dev);
> > > u32 mbus_ctl;
> > > - const struct intel_dbuf_state *old_dbuf_state =
> > > - intel_atomic_get_old_dbuf_state(state);
> > > const struct intel_dbuf_state *new_dbuf_state =
> > > intel_atomic_get_new_dbuf_state(state);
> > >
> > > @@ -3600,21 +3622,6 @@ static void update_mbus_pre_enable(struct
> > > intel_atomic_state *state)
> > > intel_de_rmw(i915, MBUS_CTL,
> > > MBUS_HASHING_MODE_MASK | MBUS_JOIN |
> > > MBUS_JOIN_PIPE_SELECT_MASK, mbus_ctl);
> > > -
> > > - if (DISPLAY_VER(i915) >= 20 &&
> > > - old_dbuf_state->mdclk_cdclk_ratio !=
> > > new_dbuf_state->mdclk_cdclk_ratio) {
> > > - /*
> > > - * For Xe2LPD and beyond, when there is a change in the ratio
> > > - * between MDCLK and CDCLK, updates to related registers need to
> > > - * happen at a specific point in the CDCLK change sequence. In
> > > - * that case, we defer to the call to
> > > - * intel_dbuf_mdclk_cdclk_ratio_update() to the CDCLK logic.
> > > - */
> > > - return;
> > > - }
> > > -
> > > - intel_dbuf_mdclk_cdclk_ratio_update(i915,
> > > new_dbuf_state->mdclk_cdclk_ratio,
> > > - new_dbuf_state->joined_mbus);
> > > }
> > >
> > > void intel_dbuf_pre_plane_update(struct intel_atomic_state *state)
> > > @@ -3632,7 +3639,11 @@ void intel_dbuf_pre_plane_update(struct
> > > intel_atomic_state *state)
> > >
> > > WARN_ON(!new_dbuf_state->base.changed);
> > >
> > > - update_mbus_pre_enable(state);
> > > + if (!old_dbuf_state->joined_mbus && new_dbuf_state->joined_mbus) {
> >
> > I think you squashed that stuff into the wrong patch.
> > This one should have a pure refactoring patch.
> >
> > > + intel_dbuf_mbus_join_update(state);
> > > + intel_dbuf_mdclk_min_tracker_update(state);
> > > + }
> > > +
> > > gen9_dbuf_slices_update(i915,
> > > old_dbuf_state->enabled_slices |
> > > new_dbuf_state->enabled_slices);
> > > @@ -3653,6 +3664,11 @@ void intel_dbuf_post_plane_update(struct
> > > intel_atomic_state *state)
> > >
> > > WARN_ON(!new_dbuf_state->base.changed);
> > >
> > > + if (old_dbuf_state->joined_mbus && !new_dbuf_state->joined_mbus) {
> > > + intel_dbuf_mbus_join_update(state);
> > > + intel_dbuf_mdclk_min_tracker_update(state);
> > > + }
> > > +
> > > gen9_dbuf_slices_update(i915,
> > > new_dbuf_state->enabled_slices);
> > > }
> > > --
> > > 2.37.3
> >
> > --
> > Ville Syrjälä
> > Intel
--
Ville Syrjälä
Intel
