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. 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. Though that would have some implications for state readout, so might turn a bit hairy as well. Dunno. > --- > 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? > + > + if (!state->modeset) > + return; > + > for_each_new_connector_in_state(&state->base, connector, new_conn_state, > i) { > struct intel_connector *intel_connector; > @@ -1602,6 +1616,9 @@ static void intel_encoders_update_complete(struct intel_atomic_state *state) > struct drm_connector *connector; > int i; > > + if (!state->modeset) > + return; > + > for_each_new_connector_in_state(&state->base, connector, new_conn_state, > i) { > struct intel_connector *intel_connector; > @@ -8670,8 +8687,7 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state) > } > } > > - if (state->modeset) > - intel_encoders_update_prepare(state); > + intel_encoders_update_prepare(state); > > intel_dbuf_pre_plane_update(state); > > @@ -8683,11 +8699,10 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state) > /* Now enable the clocks, plane, pipe, and connectors that we set up. */ > dev_priv->display->commit_modeset_enables(state); > > - if (state->modeset) { > - intel_encoders_update_complete(state); > + intel_encoders_update_complete(state); > > + if (state->modeset) > intel_set_cdclk_post_plane_update(state); > - } > > intel_wait_for_vblank_workers(state); > > -- > 2.27.0 -- Ville Syrjälä Intel
