Re: [Intel-gfx] [PATCH] drm/i915: Fix fastsets on TypeC ports following a non-blocking modeset

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Nov 15, 2021 at 07:26:59PM +0200, Imre Deak wrote:
> On Mon, Nov 15, 2021 at 05:38:58PM +0200, Ville Syrjälä wrote:
> > On Mon, Nov 15, 2021 at 02:45:36PM +0200, Imre Deak wrote:
> > > On Fri, Nov 12, 2021 at 11:14:52PM +0200, Ville Syrjälä wrote:
> > > > On Fri, Nov 12, 2021 at 09:09:04PM +0200, Imre Deak wrote:
> > > > > After a non-blocking modeset on a TypeC port's CRTC - possibly blocked
> > > > > later in drm_atomic_helper_wait_for_dependencies() - a fastset on the
> > > > > same CRTC may copy the state of CRTC before this gets updated to reflect
> > > > > the up-to-date DP-alt vs. TBT-alt TypeC mode DPLL used for the CRTC. In
> > > > > this case after the first (non-blocking) commit completes enabling the
> > > > > DPLL required for the up-to-date TypeC mode the following fastset will
> > > > > update the CRTC state pointing to the wrong DPLL. A subsequent disabling
> > > > > modeset will try to disable the wrong PLL, triggering a state checker
> > > > > WARN (and leaving the DPLL which is actually used active for good).
> > > > > 
> > > > > Fix the above race by copying the DPLL state for fastset CRTCs from the
> > > > > old CRTC state at the point where it's guaranteed to be up-to-date
> > > > > already. This could be handled in the encoder's update_prepare() hook as
> > > > > well, but that's a bigger change, which is better done as a follow-up.
> > > > > 
> > > > > Testcase: igt/kms_busy/extended-modeset-hang-newfb-with-reset
> > > > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/4308
> > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > 
> > > > This is getting a bit unpleasant.
> > > 
> > > Thanks for looking into this. Yes, special casing the fastset case and
> > > copying from the old crtc state is odd. I don't see a problem with it
> > > though, so is it acceptable as a minimal fix until a proper solution?
> > > 
> > > > Maybe we should just get rid of shared_dpll entirely and track the
> > > > currently active pll entirely elsewhere, I guess maybe in intel_crtc?
> > > > That would at least make it a bit more clear that it's no longer your
> > > > normal pre-computed state thing.
> > > 
> > > I also considered this initially (using intel_digital_port::tc_mode),
> > > but there were arguments against storing state in DRM objects. I agree
> > > that keeping crtc_state intact after pre-computing it and tracking more
> > > dynamic state in intel_crtc (akin to intel_crtc::active for instance)
> > > is the proper way, I can look into this as a follow-up.
> > > 
> > > > Though that would have some implications for state readout, so might
> > > > turn a bit hairy as well.  Dunno.
> > > 
> > > AFAICS, icl_port_dplls would still remain in intel_crtc_state - checked
> > > by intel_pipe_config_compare() - and
> > > intel_crtc_state::shared_dpll/dpll_hw_state could be moved to intel_crtc
> > > (as a pointer/index to icl_port_dplls), which would be checked in
> > > verify_crtc_state()/verify_shared_dpll_state().
> > 
> > Well, the issue is that during readout we don't want to clobber the
> > stuff stored under intel_crtc. So that would need its own special step
> > during the initial state readout, and perhaps some kind of extra sanity
> > check in the state checker. So could turn out far more annoying than the
> > current method.
> 
> The only additional thing the state checker would need is the active
> port_pll index. We could also add a valid flag to struct port_dpll
> and have intel_pipe_config_compare() etc., check only the valid port
> PLLs (so the new intel_crtc::active_port_pll index would be only
> set/used by the modesetting code, but not by the state checker).
> 
> > Though we could perhaps make the current thing a bit less confusing by
> > always using the port_pll[] stuff on every platform, and just change
> > the current shared_pll to point at the selected port_pll[] instead.
> > That would also shrink the crtc state a bit by removing one redundant
> > pll state.
> 
> Sounds ok too, but that would mean keeping the intel_crtc_state
> overwriting in this patch (if only for the shared_pll pointer).
> 
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_display.c | 25 ++++++++++++++++----
> > > > >  1 file changed, 20 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > index 0ceee8ac66717..76ebb3c91a75b 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > @@ -1572,10 +1572,24 @@ intel_connector_primary_encoder(struct intel_connector *connector)
> > > > >  
> > > > >  static void intel_encoders_update_prepare(struct intel_atomic_state *state)
> > > > >  {
> > > > > +	struct intel_crtc_state *new_crtc_state, *old_crtc_state;
> > > > > +	struct intel_crtc *crtc;
> > > > >  	struct drm_connector_state *new_conn_state;
> > > > >  	struct drm_connector *connector;
> > > > >  	int i;
> > > > >  
> > > > > +	/*
> > > > > +	 * Make sure the DPLL state is up-to-date for fastset TypeC ports after non-blocking commits.
> > > > > +	 * TODO: Update the DPLL state for all cases in the encoder->update_prepare() hook.
> > > > > +	 */
> > > > > +	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> > > > > +		if (!intel_crtc_needs_modeset(new_crtc_state))
> > > > > +			new_crtc_state->shared_dpll = old_crtc_state->shared_dpll;
> > > > > +	}
> > > > 
> > > > Don't we want to copy the pll state as well?
> > > 
> > > Yes, forgot about that. (I don't think it's used anywhere after having
> > > enabled the PLL and having checked its state, but needs to be copied for
> > > consistency.)
> > > 
> > > We'd also need
> > > BUG_ON(sizeof(crtc_state->dpll_hw_state) < sizeof(crtc_state->mpllb_state));
> > > at places where this is assumed,
> > 
> > Or just not do the copy if shared_pll (or maybe dpll_mgr?) is NULL?
> 
> Checking again, mpllb_state seems to be needed for the state checker
> crtc_state->update_pipe case to work (and for fastsets on DG2, though
> intel_pipe_config_compare() still lacks the check for that). So imo we
> should always copy dpll_hw_state/mpllb_state here (maybe have a helper
> and use it also in
> copy_bigjoiner_crtc_state()/intel_crtc_prepare_cleared_state()).

How could the mpllb state not be correct?

> 
> > > and eventually make mpllb_state part of
> > > dpll_hw_state (maybe changing dpll_hw_state to be a union of per-platform
> > > dpll state structs?).
> > 
> > Yeah, the current mpllb stuff is quite annoying. Should just convert
> > it work like all the other PLLs on modern platforms to get rid of
> > all the special casing.
> > 
> > -- 
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel