On Mon, Nov 15, 2021 at 07:33:58PM +0200, Ville Syrjälä wrote: > 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@xxxxxxxxxxxxxxx> > > > > > > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx> > > > > > > > > > > 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? Ah yes, it doesn't depend on tc_mode changes. Ok, will copy here only if dpll_mgr!=NULL. > > > > 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
