[PATCH 2/4] drm/i915: Break intel_dbuf_mbus_update into 2 separate parts

[Ville Syrjälä]

On Mon, Mar 18, 2024 at 11:44:21PM +0200, Lisovskiy, Stanislav wrote:
> On Mon, Mar 18, 2024 at 08:01:41PM +0200, Ville Syrjälä wrote:
> > On Sun, Mar 17, 2024 at 07:24:14PM +0200, Stanislav Lisovskiy wrote:
> > > 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.
> > > 
> > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/skl_watermark.c | 55 ++++++++++++--------
> > >  1 file changed, 33 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
> > > index 2d3b08c2f8d78..d7d2278fd201c 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;
> > > +	}
> > > +
> > > +	intel_dbuf_mdclk_cdclk_ratio_update(i915, new_dbuf_state->mdclk_cdclk_ratio,
> > > +					    new_dbuf_state->joined_mbus);
> > > +}
> > 
> > Argh. The hardware is turning into a disaster with all these
> > links between different units.
> > 
> > This whole thing looks rather suspicious as the cdclk changes
> > and mbus joining changes don't happen in sync.
> > 
> > AFAICS the sequence should end up doing more or less like this:
> > 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
> > 
> > So step 2.2 should keep using the old mbus_join valued when
> > updating the ratio, and steps 3.2,5.1,7.1 should use the new
> > value. That's assuming there is actually some ordering
> > requirements between these steps (whch bspec does seem to
> > imply).
> 
> So does it mean, we are missing updating dbuf tracker value
> in CDCLK programming path?

I'm saying we need to change it to use the correct (old)
value of mbus_join in intel_set_cdclk_pre_plane_update().

The whole state dance between dbuf and cdclk states
is rather annoying since it needs careful sequencing.
But perhaps there is no better way to deal with this.

And one thing that should be fixed at least are the
redundant intel_atomic_lock_global_state() calls in 
intel_cdclk_state_set_joined_mbus() and
intel_dbuf_state_set_mdclk_cdclk_ratio().

I was actually pondering about an earlier discussion we
had wrt. potentially having some kind of need_lock/need_serialize 
funcs for each global obj. We could perhaps have those as vfuncs
in the global obj and have it do the right thing automagically
at the end of atomic_check(), without the need to sprinke any
lock()/serialize() calls anywhere else. The downside would be
that we take the locks much later, so it could in theory cause
more -DEADLK retries. But dunno how much that would matter.

-- 
Ville Syrjälä
Intel