On Tue, Mar 26, 2024 at 02:12:47PM +0200, Ville Syrjälä wrote:
> On Mon, Mar 25, 2024 at 09:03:32PM +0200, Lisovskiy, Stanislav wrote:
> > On Mon, Mar 25, 2024 at 08:43:10PM +0200, Ville Syrjälä wrote:
> > > On Mon, Mar 25, 2024 at 08:29:56PM +0200, Lisovskiy, Stanislav wrote:
> > > > On Mon, Mar 25, 2024 at 07:11:21PM +0200, Ville Syrjälä wrote:
> > > > > 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.
> > > >
> > > > First of all why that
> > > > condition above prevents update when mbus join changes?
> > > > It exits when mdclk_cdclk ratio is changed not mbus_join?
> > >
> > > And what happens when mbus_join needs to be changed
> > > but mdclk_cdclk_ratio remains unchanged?
> >
> > If it is not changed, that condition won't exit,
> > intel_dbuf_mdclk_cdclk_ratio_update will get called.
>
> Hmm, right. I read it the wrong way around I guess. But it's
> still wrong. It will be called only when we change cdclk
> (and perhaps not always even in that case) not when we
> change mbus_join. We need to call it in both cases because
> they happen at different times in the sequence and we
> want to keep this stuff in sync with the actual hardware
> state (so same deal as in the the pre plane cdclk hook).
>
> > >
> > > >
> > > > That review process to me seems rather chaotic.
> > > > Constantly something new pops up, moreover we did previously agree
> > > > about that code.
> > >
> > > The review process exists to make sure the code actually
> > > works correctly. New things come up because of how human
> > > brains work, not all things are immediately apparent to
> > > everyone. If that were the case then you should have
> > > been able to make the code 100% correct from the start,
> > > and I wouldn't be able to come up with new ways in
> > > which it can fail. So I guess you're the pot and
> > > I'm the kettle?
> >
> > So do you mean that all code that you commit or give r-b
> > doesn't have issue and/or will never be required to improve?
>
> Rb == says what it does on the tin and has no known
> problems that can cause real problems. So far this
> does not meet that criteria.
>
> It's fine to have eg. known gaps in functionality
> if we plan on dealing with those later. This is,
> for example, what we did for the mbus joining
> originally. But every patch must work correctly
> regardless of those gaps. Of course we sometimes fail
> at that and bugs do slip in, but introducing issues
> on purpose is not acceptable.
>
> So we need to make sure the ratio gets correctly programmed
> in all the steps of the sequence I outlined before. Let me
> list it here again:
> 1. disable pipes
> 2. increase cdclk
> 2.1 reprogram cdclk
> 2.2 update dbuf tracker value
> 3. enable mbus joining if necessary
> 3.1 update mbus_ctl
> 3.2 update dbuf tracker value
> 4. reallocate dbuf for planes on active pipes
> 5. disable mbus joining if necessary
> 5.1 update dbuf tracker value
> 5.2 update mbus_ctl
> 6. enable pipes
> 7. decrease cdclk, mbus joining is unchanged
> 7.1 update dbuf tracker value
> 7.2 reprogram cdclk
Ugh. I had a look at our cdclk pre/post hooks and those actually
look very scary. It seems we never updated the logic there to
correctly handle crawl/squash. So it will now happily do the
cdclk update always from intel_set_cdclk_pre_plane_update()
even when decreasing the cdclk frequency. I'll go cook up
a patch...
--
Ville Syrjälä
Intel
