Re: [Intel-gfx] [PATCH] drm/i915: Apply CMTG clock disabling WA while DPLL0 is enabled

["Souza, Jose" <jose.souza@intel.com>]

On Mon, 2021-08-02 at 22:01 +0300, Imre Deak wrote:
> CI test results/further experiments show that the workaround added in
> 
> commit 573d7ce4f69a ("drm/i915/adlp: Add workaround to disable CMTG clock gating")
> 
> can be applied only while DPLL0 is enabled. If it's disabled the
> TRANS_CMTG_CHICKEN register is not accessible. Accordingly move the WA
> to DPLL0 HW state sanitization and enabling.
> 
> This fixes an issue where the WA won't get applied (and a WARN is thrown
> due to an unexpected value in TRANS_CMTG_CHICKEN) if the driver is
> loaded without DPLL0 being enabled: booting without BIOS enabling an
> output with this PLL, or reloading the driver.
> 
> While at it also add a debug print for the unexpected register value.

Workaround do not mention nothing about this DPLL0 dependency, maybe would be nice to comment in HSD about this.
Have you tried to check if the workaround applies if DPLL1 is enabled? We could comment DPLL0 out from the adlp_plls table.

> 
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c  | 18 ----------
>  drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 34 ++++++++++++++++++-
>  2 files changed, 33 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 4ca354f154215..98f7fbede6226 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -13284,24 +13284,6 @@ static void intel_early_display_was(struct drm_i915_private *dev_priv)
>  			     KBL_ARB_FILL_SPARE_13 | KBL_ARB_FILL_SPARE_14,
>  			     KBL_ARB_FILL_SPARE_14);
>  	}
> -
> -	if (IS_ADLP_DISPLAY_STEP(dev_priv, STEP_A0, STEP_B0)) {
> -		u32 val;
> -
> -		/*
> -		 * Wa_16011069516:adl-p[a0]
> -		 *
> -		 * All CMTG regs are unreliable until CMTG clock gating is
> -		 * disabled, so we can only assume the default CMTG_CHICKEN
> -		 * reg value and sanity check this assumption with a double
> -		 * read, which presumably returns the correct value even with
> -		 * clock gating on.
> -		 */
> -		val = intel_de_read(dev_priv, TRANS_CMTG_CHICKEN);
> -		val = intel_de_read(dev_priv, TRANS_CMTG_CHICKEN);
> -		intel_de_write(dev_priv, TRANS_CMTG_CHICKEN, DISABLE_DPT_CLK_GATING);
> -		drm_WARN_ON(&dev_priv->drm, val & ~DISABLE_DPT_CLK_GATING);
> -	}
>  }
>  
>  static void ibx_sanitize_pch_hdmi_port(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> index 0d72917e5670f..5c91d125a3371 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> @@ -3735,6 +3735,31 @@ static void icl_pll_enable(struct drm_i915_private *dev_priv,
>  		drm_err(&dev_priv->drm, "PLL %d not locked\n", pll->info->id);
>  }
>  
> +static void adlp_cmtg_clock_gating_wa(struct drm_i915_private *i915, struct intel_shared_dpll *pll)
> +{
> +	u32 val;
> +
> +	if (!IS_ADLP_DISPLAY_STEP(i915, STEP_A0, STEP_B0) ||
> +	    pll->info->id != DPLL_ID_ICL_DPLL0)
> +		return;
> +	/*
> +	 * Wa_16011069516:adl-p[a0]
> +	 *
> +	 * All CMTG regs are unreliable until CMTG clock gating is disabled,
> +	 * so we can only assume the default TRANS_CMTG_CHICKEN reg value and
> +	 * sanity check this assumption with a double read, which presumably
> +	 * returns the correct value even with clock gating on.
> +	 *
> +	 * Instead of the usual place for workarounds we apply this one here,
> +	 * since TRANS_CMTG_CHICKEN is only accessible while DPLL0 is enabled.
> +	 */
> +	val = intel_de_read(i915, TRANS_CMTG_CHICKEN);
> +	val = intel_de_read(i915, TRANS_CMTG_CHICKEN);
> +	intel_de_write(i915, TRANS_CMTG_CHICKEN, DISABLE_DPT_CLK_GATING);
> +	if (drm_WARN_ON(&i915->drm, val & ~DISABLE_DPT_CLK_GATING))
> +		drm_dbg_kms(&i915->drm, "Unexpected flags in TRANS_CMTG_CHICKEN: %08x\n", val);
> +}
> +
>  static void combo_pll_enable(struct drm_i915_private *dev_priv,
>  			     struct intel_shared_dpll *pll)
>  {
> @@ -3764,6 +3789,8 @@ static void combo_pll_enable(struct drm_i915_private *dev_priv,
>  
>  	icl_pll_enable(dev_priv, pll, enable_reg);
>  
> +	adlp_cmtg_clock_gating_wa(dev_priv, pll);
> +
>  	/* DVFS post sequence would be here. See the comment above. */
>  }
>  
> @@ -4273,7 +4300,12 @@ void intel_dpll_readout_hw_state(struct drm_i915_private *i915)
>  static void sanitize_dpll_state(struct drm_i915_private *i915,
>  				struct intel_shared_dpll *pll)
>  {
> -	if (!pll->on || pll->active_mask)
> +	if (!pll->on)
> +		return;
> +
> +	adlp_cmtg_clock_gating_wa(i915, pll);
> +
> +	if (pll->active_mask)
>  		return;
>  
>  	drm_dbg_kms(&i915->drm,