Re: [PATCH 2/2] drm/i915: Implement vblank synchronized MBUS join changes

["Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>]

On Fri, Mar 08, 2024 at 05:11:42PM +0200, Ville Syrjälä wrote:
> On Fri, Mar 08, 2024 at 03:43:35PM +0200, Lisovskiy, Stanislav wrote:
> > On Fri, Mar 08, 2024 at 12:07:19PM +0200, Ville Syrjälä wrote:
> > > On Wed, Feb 28, 2024 at 10:02:13AM +0200, Stanislav Lisovskiy wrote:
> > > > Currently we can't change MBUS join status without doing a modeset,
> > > > because we are lacking mechanism to synchronize those with vblank.
> > > > However then this means that we can't do a fastset, if there is a need
> > > > to change MBUS join state. Fix that by implementing such change.
> > > > We already call correspondent check and update at pre_plane dbuf update,
> > > > so the only thing left is to have a non-modeset version of that.
> > > > If active pipes stay the same then fastset is possible and only MBUS
> > > > join state/ddb allocation updates would be committed.
> > > > 
> > > > v2: Implement additional changes according to BSpec.
> > > >     Vblank wait is needed after MBus/Dbuf programming in case if
> > > >     no modeset is done and we are switching from single to multiple
> > > >     displays, i.e mbus join state switches from "joined" to  "non-joined"
> > > >     state. Otherwise vblank wait is not needed according to spec.
> > > > 
> > > > v3: Split mbus and dbox programming into to pre/post plane update parts,
> > > >     how it should be done according to BSpec.
> > > > 
> > > > v4: - Place "single display to multiple displays scenario" MBUS/DBOX programming
> > > >       after DDB reallocation, but before crtc enabling(that is where is has
> > > >       to be according to spec).
> > > >     - Check if crtc is still active, not only the old state.
> > > >     - Do a vblank wait if MBUX DBOX register was modified.
> > > >     - And of course do vblank wait only if crtc was active.
> > > >     - Do vblank wait only if we are not doing a modeset, if we are doing
> > > >       something before *commit_modeset_enables, because all crtcs might be
> > > >       disabled at this moment, so we will get WARN if try waiting for vblank
> > > >       then.
> > > >     - Still getting FIFO underrun so try waiting for vblank in pre_plane update
> > > >       as well.
> > > >     - Write also pipe that we need to sync with to MBUS_CTL register.
> > > > 
> > > > v5: - Do vblank wait only for the first pipe, if mbus is joined
> > > >     - Check also if new/old_dbuf_state is not NULL, before getting single pipe
> > > >       and active pipes.
> > > > 
> > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_display.c |  13 ++-
> > > >  drivers/gpu/drm/i915/display/skl_watermark.c | 104 +++++++++++++++----
> > > >  drivers/gpu/drm/i915/display/skl_watermark.h |   1 +
> > > >  3 files changed, 96 insertions(+), 22 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > index 00ac65a140298..989818f5d342f 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > @@ -6906,6 +6906,17 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> > > >  		}
> > > >  	}
> > > >  
> > > > +	/*
> > > > +	 * Some MBUS/DBuf update scenarios(mbus join disable) require that
> > > > +	 * changes are done _after_ DDB reallocation, but _before_ crtc enabling.
> > > > +	 * Typically we are disabling resources in post_plane/crtc_enable hooks,
> > > > +	 * however in that case BSpec explicitly states that this should be done
> > > > +	 * before we enable additional displays.
> > > > +	 * FIXME: Should we still call this also there(post_plane hook)
> > > > +	 * for extra-safety? If so, how to make sure, we don't call it twice?
> > > > +	 */
> > > > +	intel_dbuf_mbus_post_ddb_update(state);
> > > > +
> > > >  	update_pipes = modeset_pipes;
> > > >  
> > > >  	/*
> > > > @@ -7148,9 +7159,7 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
> > > >  	}
> > > >  
> > > >  	intel_encoders_update_prepare(state);
> > > > -
> > > >  	intel_dbuf_pre_plane_update(state);
> > > > -	intel_mbus_dbox_update(state);
> > > >  
> > > >  	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> > > >  		if (new_crtc_state->do_async_flip)
> > > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
> > > > index 606b7ba9db9ce..f0604ede399f7 100644
> > > > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > > > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > > > @@ -2628,13 +2628,6 @@ skl_compute_ddb(struct intel_atomic_state *state)
> > > >  		if (ret)
> > > >  			return ret;
> > > >  
> > > > -		if (old_dbuf_state->joined_mbus != new_dbuf_state->joined_mbus) {
> > > > -			/* TODO: Implement vblank synchronized MBUS joining changes */
> > > > -			ret = intel_modeset_all_pipes_late(state, "MBUS joining change");
> > > > -			if (ret)
> > > > -				return ret;
> > > > -		}
> > > > -
> > > >  		drm_dbg_kms(&i915->drm,
> > > >  			    "Enabled dbuf slices 0x%x -> 0x%x (total dbuf slices 0x%x), mbus joined? %s->%s\n",
> > > >  			    old_dbuf_state->enabled_slices,
> > > > @@ -3539,8 +3532,9 @@ static void intel_dbuf_mbus_update(struct intel_atomic_state *state)
> > > >  	struct drm_i915_private *i915 = to_i915(state->base.dev);
> > > >  	u32 mbus_ctl, dbuf_min_tracker_val;
> > > >  	enum dbuf_slice slice;
> > > > -	const struct intel_dbuf_state *dbuf_state =
> > > > +	const struct intel_dbuf_state *new_dbuf_state =
> > > >  		intel_atomic_get_new_dbuf_state(state);
> > > > +	enum pipe pipe = ffs(new_dbuf_state->active_pipes) - 1;
> > > 
> > > That pipe might not even be enabled at this point.
> > 
> > Which scenario do you mean?
> > intel_dbuf_mbus_update is called in two cases:
> > 
> > 1) When switching from single display to multiple displays, according
> >    to spec we should program it before enabling additional displays,
> >    but after ddb allocation happens.
> > 
> > 2) When switching from multiple displays to a single display,
> >    we program it after disabling all displays except one, but
> >    before ddb reallocation happens.
> 
> You seem to call it when the number of active pipes changes.
> That doesn't necessarily mean anything for mbus joining.

From code here:

+	if (!new_dbuf_state ||
+	    (new_dbuf_state->enabled_slices == old_dbuf_state->enabled_slices &&
+	     new_dbuf_state->joined_mbus == old_dbuf_state->joined_mbus))
+		return;
+
+	pipe = ffs(new_dbuf_state->active_pipes) - 1;
+	new_num_active_pipes = hweight8(new_dbuf_state->active_pipes);
+
+	if (old_dbuf_state)
+		old_num_active_pipes = hweight8(old_dbuf_state->active_pipes);
+
+	WARN_ON(!new_dbuf_state->base.changed);
+
+	/*
+	 * Switching from single to multiple display scenario
+	 */
+	if (new_num_active_pipes > old_num_active_pipes) {

We have a check above, so if we are at this point, means that either enabled_slices
had changed or joined_mbus state.
If for example new active pipe count is more than old active pipe count and 
join_mbus state had changed, I think it for sure means single => multiple switch scenario,
since there are currently no other scenarios, when mbus_join changes and active pipe count
increases except that one.
For instance if old active pipe count was anything > 1 and it increases, there would have been
no mbus_join state change.

However enabled slice count may change, so I wonder if I need to may be add, some additional
check like if ((new_num_active_pipes > old_num_active_pipes) && old_num_active_pipes == 1) to be sure.
In fact I had it in one of previous revisions, but can't recall now, why I removed it.

> 
> > Probably you mean the case when its called from intel_dbuf_pre_plane_update,
> > because commit_modeset_enables hasn't been yet called?
> 
> Yes, the pipe may still be off.
> 
> > That would be the case of switching from multiple displays to single one,
> > for non-modeset at least shoudln't be a problem, as I understand.
> 
> I don't know what the hardware does in this case. Better not
> to ask for trouble IMO in case it turns out the hardware won't
> like it.
> 
> > 
> > But where should this be called then from? 
> > 
> > We always called this function from intel_dbuf_pre_plane_update.
> 
> As mentioned later in my mail, I think we just want a pre/post
> ddb callsites for this stuff. Though the credit stuff (should we
> need to account for those changing) might complicate things further...

Okay, I already have post ddb call site, so means I need to do the same
for pre ddb callsite, the only thing I wonder if that change is going to 
screw up something once again.

> 
> -- 
> Ville Syrjälä
> Intel