Re: [Intel-gfx] [PATCH 1/4] drm/i915: Force DPLL calculation for TC ports after readout

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 22 Sep 2022, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> We always allocate two DPLLs (TC and TBT) for TC ports. This
> is because we can't know ahead of time wherher we need to put
> the PHY into DP-Alt or TBT mode.
>
> However during readout we can obviously only read out the state
> of the DPLL that the port is actually using. Thus the state after
> readout will not have both DPLLs populated.
>
> We run into problems if during readout the TC port is in DP-Alt
> mode, but we then perform a modeset on the port without going
> through the full .compute_config() machinery, and during said
> modeset the port cannot be switched back into DP-Alt mode and
> we need to take the TBT fallback path. Such a modeset can
> happen eg. due to cdclk reprogramming.
>
> This wasn't a problem earlier because we did all the DPLL
> calculations much later in the modeset. So even if flagged
> a modeset very late we'd still have gone through the DPLL
> calculations. But now all the DPLL calculations happen much
> earlier and so we need to deal with it, or else we'll attempt
> a modeset without a DPLL.
>
> To guarantee that we always have both DPLLs fully cal/ulated
> for TC ports force a full modeset computation during the
> initial commit.
>
> Reported-by: Lee Shawn C <shawn.c.lee@intel.com>
> Fixes: b000abd3b3d2 ("drm/i915: Do .crtc_compute_clock() earlier")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 643832d55c28..6278b8ea5bf1 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -3600,10 +3600,21 @@ static void intel_ddi_sync_state(struct intel_encoder *encoder,
>  static bool intel_ddi_initial_fastset_check(struct intel_encoder *encoder,
>  					    struct intel_crtc_state *crtc_state)
>  {
> +	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> +	enum phy phy = intel_port_to_phy(i915, encoder->port);
> +	bool ret = true;
> +
> +	if (intel_phy_is_tc(i915, phy)) {
> +		drm_dbg_kms(&i915->drm, "[ENCODER:%d:%s] Forcing full modeset to compute TC port DPLLs\n",
> +			    encoder->base.base.id, encoder->base.name);
> +		crtc_state->uapi.mode_changed = true;
> +		ret = false;
> +	}
> +
>  	if (intel_crtc_has_dp_encoder(crtc_state))
> -		return intel_dp_initial_fastset_check(encoder, crtc_state);
> +		ret &= intel_dp_initial_fastset_check(encoder, crtc_state);

I think there have been static checker warnings about mixing bitwise and
boolean AND like this. I guess there's implicit type conversion to int
and back to bool.

BR,
Jani.

>  
> -	return true;
> +	return ret;
>  }
>  
>  static enum intel_output_type

-- 
Jani Nikula, Intel Open Source Graphics Center