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 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. 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