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.syrj...@linux.intel.com>
> Signed-off-by: Imre Deak <imre.d...@intel.com>
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
