[Intel-gfx] [PATCH 0/1] Implement workaround for PLL enabling for DG2/MTL

[Intel-gfx] [PATCH 0/1] Implement workaround for PLL enabling for DG2/MTL

[Stanislav Lisovskiy]

It has been noticed by HW team, that there are might be problems
when PLL is being enabled with CDCLK squashing being turned on,
which might result in loosing register access and/or FIFO underrun.
As a workaround it has been proposed to disable CDCLK squashing right
before PLL is enabled and enable squashing later, if needed.

Stanislav Lisovskiy (1):
  drm/i915: Implement workaround for CDCLK PLL disable/enable

 drivers/gpu/drm/i915/display/intel_cdclk.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

-- 
2.37.3

[Intel-gfx] [PATCH 1/1] drm/i915: Implement workaround for CDCLK PLL disable/enable

[Stanislav Lisovskiy]

It was reported that we might get a hung and loss of register access in
some cases when CDCLK PLL is disabled and then enabled, while squashing
is enabled.
As a workaround it was proposed by HW team that SW should disable squashing
when CDCLK PLL is being reenabled.

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_cdclk.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
index 0c107a38f9d0..e338f288c9ac 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -1801,6 +1801,13 @@ static bool cdclk_compute_crawl_and_squash_midpoint(struct drm_i915_private *i91
 	return true;
 }
 
+static bool pll_enable_wa_needed(struct drm_i915_private *dev_priv)
+{
+	return ((IS_DG2(dev_priv) || IS_METEORLAKE(dev_priv))
+		&& dev_priv->display.cdclk.hw.vco > 0
+		&& HAS_CDCLK_SQUASH(dev_priv));
+}
+
 static void _bxt_set_cdclk(struct drm_i915_private *dev_priv,
 			   const struct intel_cdclk_config *cdclk_config,
 			   enum pipe pipe)
@@ -1815,9 +1822,12 @@ static void _bxt_set_cdclk(struct drm_i915_private *dev_priv,
 	    !cdclk_pll_is_unknown(dev_priv->display.cdclk.hw.vco)) {
 		if (dev_priv->display.cdclk.hw.vco != vco)
 			adlp_cdclk_pll_crawl(dev_priv, vco);
-	} else if (DISPLAY_VER(dev_priv) >= 11)
+	} else if (DISPLAY_VER(dev_priv) >= 11) {
+		if (pll_enable_wa_needed(dev_priv))
+			dg2_cdclk_squash_program(dev_priv, 0);
+
 		icl_cdclk_pll_update(dev_priv, vco);
-	else
+	} else
 		bxt_cdclk_pll_update(dev_priv, vco);
 
 	waveform = cdclk_squash_waveform(dev_priv, cdclk);
-- 
2.37.3

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Implement workaround for PLL enabling for DG2/MTL

[Patchwork]

== Series Details ==

Series: Implement workaround for PLL enabling for DG2/MTL
URL   : https://patchwork.freedesktop.org/series/111311/
State : warning

== Summary ==

Error: dim checkpatch failed
526c2d192fcc drm/i915: Implement workaround for CDCLK PLL disable/enable
-:25: CHECK:LOGICAL_CONTINUATIONS: Logical continuations should be on the previous line
#25: FILE: drivers/gpu/drm/i915/display/intel_cdclk.c:1807:
+	return ((IS_DG2(dev_priv) || IS_METEORLAKE(dev_priv))
+		&& dev_priv->display.cdclk.hw.vco > 0

-:26: CHECK:LOGICAL_CONTINUATIONS: Logical continuations should be on the previous line
#26: FILE: drivers/gpu/drm/i915/display/intel_cdclk.c:1808:
+		&& dev_priv->display.cdclk.hw.vco > 0
+		&& HAS_CDCLK_SQUASH(dev_priv));

-:43: CHECK:BRACES: Unbalanced braces around else statement
#43: FILE: drivers/gpu/drm/i915/display/intel_cdclk.c:1830:
+	} else

total: 0 errors, 0 warnings, 3 checks, 27 lines checked

[Intel-gfx] ✗ Fi.CI.DOCS: warning for Implement workaround for PLL enabling for DG2/MTL

[Patchwork]

== Series Details ==

Series: Implement workaround for PLL enabling for DG2/MTL
URL   : https://patchwork.freedesktop.org/series/111311/
State : warning

== Summary ==

Error: make htmldocs had i915 warnings
./drivers/gpu/drm/i915/gt/intel_gt_mcr.c:739: warning: expecting prototype for intel_gt_mcr_wait_for_reg_fw(). Prototype was for intel_gt_mcr_wait_for_reg() instead
./drivers/gpu/drm/i915/gt/intel_gt_mcr.c:739: warning: expecting prototype for intel_gt_mcr_wait_for_reg_fw(). Prototype was for intel_gt_mcr_wait_for_reg() instead

[Intel-gfx] ✓ Fi.CI.BAT: success for Implement workaround for PLL enabling for DG2/MTL

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 4221 bytes --]

== Series Details ==

Series: Implement workaround for PLL enabling for DG2/MTL
URL   : https://patchwork.freedesktop.org/series/111311/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_12429 -> Patchwork_111311v1
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111311v1/index.html

Participating hosts (36 -> 36)
------------------------------

  Additional (1): fi-hsw-4770 
  Missing    (1): fi-ctg-p8600 

Known issues
------------

  Here are the changes found in Patchwork_111311v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@kms_addfb_basic@addfb25-y-tiled-small-legacy:
    - fi-hsw-4770:        NOTRUN -> [SKIP][1] ([fdo#109271]) +11 similar issues
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111311v1/fi-hsw-4770/igt@kms_addfb_basic@addfb25-y-tiled-small-legacy.html

  * igt@kms_chamelium@dp-crc-fast:
    - fi-hsw-4770:        NOTRUN -> [SKIP][2] ([fdo#109271] / [fdo#111827]) +8 similar issues
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111311v1/fi-hsw-4770/igt@kms_chamelium@dp-crc-fast.html

  * igt@kms_psr@sprite_plane_onoff:
    - fi-hsw-4770:        NOTRUN -> [SKIP][3] ([fdo#109271] / [i915#1072]) +3 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111311v1/fi-hsw-4770/igt@kms_psr@sprite_plane_onoff.html

  
#### Possible fixes ####

  * igt@gem_exec_suspend@basic-s3@smem:
    - {bat-rpls-1}:       [DMESG-WARN][4] ([i915#6687]) -> [PASS][5]
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12429/bat-rpls-1/igt@gem_exec_suspend@basic-s3@smem.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111311v1/bat-rpls-1/igt@gem_exec_suspend@basic-s3@smem.html

  * igt@i915_selftest@live@migrate:
    - {bat-adlp-6}:       [INCOMPLETE][6] ([i915#7348]) -> [PASS][7]
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12429/bat-adlp-6/igt@i915_selftest@live@migrate.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111311v1/bat-adlp-6/igt@i915_selftest@live@migrate.html

  
#### Warnings ####

  * igt@i915_suspend@basic-s3-without-i915:
    - fi-rkl-11600:       [FAIL][8] ([fdo#103375]) -> [INCOMPLETE][9] ([i915#4817])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12429/fi-rkl-11600/igt@i915_suspend@basic-s3-without-i915.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111311v1/fi-rkl-11600/igt@i915_suspend@basic-s3-without-i915.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4817]: https://gitlab.freedesktop.org/drm/intel/issues/4817
  [i915#5278]: https://gitlab.freedesktop.org/drm/intel/issues/5278
  [i915#6434]: https://gitlab.freedesktop.org/drm/intel/issues/6434
  [i915#6687]: https://gitlab.freedesktop.org/drm/intel/issues/6687
  [i915#6997]: https://gitlab.freedesktop.org/drm/intel/issues/6997
  [i915#7328]: https://gitlab.freedesktop.org/drm/intel/issues/7328
  [i915#7348]: https://gitlab.freedesktop.org/drm/intel/issues/7348


Build changes
-------------

  * Linux: CI_DRM_12429 -> Patchwork_111311v1

  CI-20190529: 20190529
  CI_DRM_12429: 30adb45f7b1eddfcda3ef1de5e8dbcd2c1f5a57d @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7072: 69ba7163475925cdc69aebbdfa0e87453ae165c7 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_111311v1: 30adb45f7b1eddfcda3ef1de5e8dbcd2c1f5a57d @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

75d1f65bee73 drm/i915: Implement workaround for CDCLK PLL disable/enable

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111311v1/index.html

[-- Attachment #2: Type: text/html, Size: 4700 bytes --]

Re: [Intel-gfx] [PATCH 1/1] drm/i915: Implement workaround for CDCLK PLL disable/enable

[Srivatsa, Anusha]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Stanislav Lisovskiy
> Sent: Thursday, November 24, 2022 2:36 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: Nikula, Jani <jani.nikula@intel.com>
> Subject: [Intel-gfx] [PATCH 1/1] drm/i915: Implement workaround for CDCLK
> PLL disable/enable
> 
> It was reported that we might get a hung and loss of register access in some
> cases when CDCLK PLL is disabled and then enabled, while squashing is
> enabled.
> As a workaround it was proposed by HW team that SW should disable
> squashing when CDCLK PLL is being reenabled.
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index 0c107a38f9d0..e338f288c9ac 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -1801,6 +1801,13 @@ static bool
> cdclk_compute_crawl_and_squash_midpoint(struct drm_i915_private *i91
>  	return true;
>  }
> 
> +static bool pll_enable_wa_needed(struct drm_i915_private *dev_priv) {
> +	return ((IS_DG2(dev_priv) || IS_METEORLAKE(dev_priv))
> +		&& dev_priv->display.cdclk.hw.vco > 0
> +		&& HAS_CDCLK_SQUASH(dev_priv));
Redundant check. If it is MTL or DG2, then it will have HAS_CDCLK_SQUASH set to true always. Shouldn't vco be 0 instead of > 0. The commit message says the hang can be observed when moving from 0 to > 0 vco. 

Anusha
> +}
> +
>  static void _bxt_set_cdclk(struct drm_i915_private *dev_priv,
>  			   const struct intel_cdclk_config *cdclk_config,
>  			   enum pipe pipe)
> @@ -1815,9 +1822,12 @@ static void _bxt_set_cdclk(struct
> drm_i915_private *dev_priv,
>  	    !cdclk_pll_is_unknown(dev_priv->display.cdclk.hw.vco)) {
>  		if (dev_priv->display.cdclk.hw.vco != vco)
>  			adlp_cdclk_pll_crawl(dev_priv, vco);
> -	} else if (DISPLAY_VER(dev_priv) >= 11)
> +	} else if (DISPLAY_VER(dev_priv) >= 11) {
> +		if (pll_enable_wa_needed(dev_priv))
> +			dg2_cdclk_squash_program(dev_priv, 0);
> +
>  		icl_cdclk_pll_update(dev_priv, vco);
> -	else
> +	} else
>  		bxt_cdclk_pll_update(dev_priv, vco);
> 
>  	waveform = cdclk_squash_waveform(dev_priv, cdclk);
> --
> 2.37.3

Re: [Intel-gfx] [PATCH 1/1] drm/i915: Implement workaround for CDCLK PLL disable/enable

[Lisovskiy, Stanislav]

On Tue, Nov 29, 2022 at 09:19:40PM +0200, Srivatsa, Anusha wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
> > Stanislav Lisovskiy
> > Sent: Thursday, November 24, 2022 2:36 AM
> > To: intel-gfx@lists.freedesktop.org
> > Cc: Nikula, Jani <jani.nikula@intel.com>
> > Subject: [Intel-gfx] [PATCH 1/1] drm/i915: Implement workaround for CDCLK
> > PLL disable/enable
> > 
> > It was reported that we might get a hung and loss of register access in some
> > cases when CDCLK PLL is disabled and then enabled, while squashing is
> > enabled.
> > As a workaround it was proposed by HW team that SW should disable
> > squashing when CDCLK PLL is being reenabled.
> > 
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_cdclk.c | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index 0c107a38f9d0..e338f288c9ac 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -1801,6 +1801,13 @@ static bool
> > cdclk_compute_crawl_and_squash_midpoint(struct drm_i915_private *i91
> >  	return true;
> >  }
> > 
> > +static bool pll_enable_wa_needed(struct drm_i915_private *dev_priv) {
> > +	return ((IS_DG2(dev_priv) || IS_METEORLAKE(dev_priv))
> > +		&& dev_priv->display.cdclk.hw.vco > 0
> > +		&& HAS_CDCLK_SQUASH(dev_priv));
> Redundant check. If it is MTL or DG2, then it will have HAS_CDCLK_SQUASH set to true always. Shouldn't vco be 0 instead of > 0. The commit message says the hang can be observed when moving from 0 to > 0 vco. 
> 
> Anusha

Idea was that we probably should bind more to the feature rather than platform, I agree checking both "IS_DG2" and if
platform has squashing is redundant, because then we would have to add each new platform manually, so I would leave
HAS_CDCLK_SQUASH and then at some point just cut off using some INTEL_GEN or other check all the new platforms where
this is fixed in HW.

Regarding vco, the icl_cdclk_pll_update func works as follows:

if (i915->display.cdclk.hw.vco != 0 &&
    i915->display.cdclk.hw.vco != vco)
    icl_cdclk_pll_disable(i915);

if (i915->display.cdclk.hw.vco != vco)
    icl_cdclk_pll_enable(i915, vco);

1) if vco changes from zero value(i915->display.cdclk.hw.vco) to non-zero value(vco), that means
   currently squashing is anyway disabled(if vco == 0, cdclk is set to "bypass" and squash waveform
   is 0), so the W/A is not needed.

2) if vco changes from non-zero value in i915->display.cdclk.hw.vco to zero value(vco), we are not 
   going to call icl_cdclk_pll_enable anyway so W/A is also not needed.

3) if vco changes from some non-zero value in i915->display.cdclk.hw.vco to other non-zero value(vco),
   which can happen if CDCLK changes, then icl_cdclk_pll_disable(hw vco!=0 and vco!=0) and then 
   icl_cdclk_pll_enable would be called(hw vco is still not equal to vco)
   So that disabling enabling in between is what we are interested and where we should make sure
   squashing is disabled.
   BTW I have another question - is this code even correct? Shouldn't we avoid disabling/enabling PLL if we have
   squashing/crawling? As I understood the whole point of having swuashing/crawling is avoiding swithing the PLL
   on and off.


Stan


> > +}
> > +
> >  static void _bxt_set_cdclk(struct drm_i915_private *dev_priv,
> >  			   const struct intel_cdclk_config *cdclk_config,
> >  			   enum pipe pipe)
> > @@ -1815,9 +1822,12 @@ static void _bxt_set_cdclk(struct
> > drm_i915_private *dev_priv,
> >  	    !cdclk_pll_is_unknown(dev_priv->display.cdclk.hw.vco)) {
> >  		if (dev_priv->display.cdclk.hw.vco != vco)
> >  			adlp_cdclk_pll_crawl(dev_priv, vco);
> > -	} else if (DISPLAY_VER(dev_priv) >= 11)
> > +	} else if (DISPLAY_VER(dev_priv) >= 11) {
> > +		if (pll_enable_wa_needed(dev_priv))
> > +			dg2_cdclk_squash_program(dev_priv, 0);
> > +
> >  		icl_cdclk_pll_update(dev_priv, vco);
> > -	else
> > +	} else
> >  		bxt_cdclk_pll_update(dev_priv, vco);
> > 
> >  	waveform = cdclk_squash_waveform(dev_priv, cdclk);
> > --
> > 2.37.3
> 

Re: [Intel-gfx] [PATCH 1/1] drm/i915: Implement workaround for CDCLK PLL disable/enable

[Srivatsa, Anusha]

> -----Original Message-----
> From: Lisovskiy, Stanislav <stanislav.lisovskiy@intel.com>
> Sent: Wednesday, December 14, 2022 2:31 AM
> To: Srivatsa, Anusha <anusha.srivatsa@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>
> Subject: Re: [Intel-gfx] [PATCH 1/1] drm/i915: Implement workaround for
> CDCLK PLL disable/enable
> 
> On Tue, Nov 29, 2022 at 09:19:40PM +0200, Srivatsa, Anusha wrote:
> >
> >
> > > -----Original Message-----
> > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf
> > > Of Stanislav Lisovskiy
> > > Sent: Thursday, November 24, 2022 2:36 AM
> > > To: intel-gfx@lists.freedesktop.org
> > > Cc: Nikula, Jani <jani.nikula@intel.com>
> > > Subject: [Intel-gfx] [PATCH 1/1] drm/i915: Implement workaround for
> > > CDCLK PLL disable/enable
> > >
> > > It was reported that we might get a hung and loss of register access
> > > in some cases when CDCLK PLL is disabled and then enabled, while
> > > squashing is enabled.
> > > As a workaround it was proposed by HW team that SW should disable
> > > squashing when CDCLK PLL is being reenabled.
> > >
> > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_cdclk.c | 14 ++++++++++++--
> > >  1 file changed, 12 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > index 0c107a38f9d0..e338f288c9ac 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > @@ -1801,6 +1801,13 @@ static bool
> > > cdclk_compute_crawl_and_squash_midpoint(struct drm_i915_private
> *i91
> > >  	return true;
> > >  }
> > >
> > > +static bool pll_enable_wa_needed(struct drm_i915_private *dev_priv)
> {
> > > +	return ((IS_DG2(dev_priv) || IS_METEORLAKE(dev_priv))
> > > +		&& dev_priv->display.cdclk.hw.vco > 0
> > > +		&& HAS_CDCLK_SQUASH(dev_priv));
> > Redundant check. If it is MTL or DG2, then it will have
> HAS_CDCLK_SQUASH set to true always. Shouldn't vco be 0 instead of > 0.
> The commit message says the hang can be observed when moving from 0 to
> > 0 vco.
> >
> > Anusha
> 
> Idea was that we probably should bind more to the feature rather than
> platform, I agree checking both "IS_DG2" and if platform has squashing is
> redundant, because then we would have to add each new platform
> manually, so I would leave HAS_CDCLK_SQUASH and then at some point
> just cut off using some INTEL_GEN or other check all the new platforms
> where this is fixed in HW.
> 
> Regarding vco, the icl_cdclk_pll_update func works as follows:
> 
> if (i915->display.cdclk.hw.vco != 0 &&
>     i915->display.cdclk.hw.vco != vco)
>     icl_cdclk_pll_disable(i915);
> 
> if (i915->display.cdclk.hw.vco != vco)
>     icl_cdclk_pll_enable(i915, vco);
> 
> 1) if vco changes from zero value(i915->display.cdclk.hw.vco) to non-zero
> value(vco), that means
>    currently squashing is anyway disabled(if vco == 0, cdclk is set to "bypass"
> and squash waveform
>    is 0), so the W/A is not needed.
> 
> 2) if vco changes from non-zero value in i915->display.cdclk.hw.vco to zero
> value(vco), we are not
>    going to call icl_cdclk_pll_enable anyway so W/A is also not needed.
> 
> 3) if vco changes from some non-zero value in i915->display.cdclk.hw.vco to
> other non-zero value(vco),
>    which can happen if CDCLK changes, then icl_cdclk_pll_disable(hw vco!=0
> and vco!=0) and then
>    icl_cdclk_pll_enable would be called(hw vco is still not equal to vco)
>    So that disabling enabling in between is what we are interested and
> where we should make sure
>    squashing is disabled.
>    BTW I have another question - is this code even correct? Shouldn't we
> avoid disabling/enabling PLL if we have
>    squashing/crawling? As I understood the whole point of having
> swuashing/crawling is avoiding swithing the PLL
>    on and off.
> 
Squashing works when we don't need to change the PLL ratio. We fix the PLL ratio to say 307 (this can change from platform to platform). Then squashing can be used to vary frequencies below this. So we set squasher to ffff it will mean highest. We can use squasher to change frequency with squash waveform, max being ffff and any value lower will take lower frequencies. 
Cdclk Crawling is used when the PLL has to be changed. Eg higher than 307 then we need to update PLL. In that case we first program squashing to ffff(this will take to 307) n then use crawling to go up to the desired frequency.

Anusha
> Stan
> 
> 
> > > +}
> > > +
> > >  static void _bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > >  			   const struct intel_cdclk_config *cdclk_config,
> > >  			   enum pipe pipe)
> > > @@ -1815,9 +1822,12 @@ static void _bxt_set_cdclk(struct
> > > drm_i915_private *dev_priv,
> > >  	    !cdclk_pll_is_unknown(dev_priv->display.cdclk.hw.vco)) {
> > >  		if (dev_priv->display.cdclk.hw.vco != vco)
> > >  			adlp_cdclk_pll_crawl(dev_priv, vco);
> > > -	} else if (DISPLAY_VER(dev_priv) >= 11)
> > > +	} else if (DISPLAY_VER(dev_priv) >= 11) {
> > > +		if (pll_enable_wa_needed(dev_priv))
> > > +			dg2_cdclk_squash_program(dev_priv, 0);
> > > +
> > >  		icl_cdclk_pll_update(dev_priv, vco);
> > > -	else
> > > +	} else
> > >  		bxt_cdclk_pll_update(dev_priv, vco);
> > >
> > >  	waveform = cdclk_squash_waveform(dev_priv, cdclk);
> > > --
> > > 2.37.3
> >

Re: [Intel-gfx] [PATCH 1/1] drm/i915: Implement workaround for CDCLK PLL disable/enable

[Lisovskiy, Stanislav]

On Wed, Dec 14, 2022 at 09:15:07PM +0200, Srivatsa, Anusha wrote:
> 
> 
> > -----Original Message-----
> > From: Lisovskiy, Stanislav <stanislav.lisovskiy@intel.com>
> > Sent: Wednesday, December 14, 2022 2:31 AM
> > To: Srivatsa, Anusha <anusha.srivatsa@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>
> > Subject: Re: [Intel-gfx] [PATCH 1/1] drm/i915: Implement workaround for
> > CDCLK PLL disable/enable
> > 
> > On Tue, Nov 29, 2022 at 09:19:40PM +0200, Srivatsa, Anusha wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf
> > > > Of Stanislav Lisovskiy
> > > > Sent: Thursday, November 24, 2022 2:36 AM
> > > > To: intel-gfx@lists.freedesktop.org
> > > > Cc: Nikula, Jani <jani.nikula@intel.com>
> > > > Subject: [Intel-gfx] [PATCH 1/1] drm/i915: Implement workaround for
> > > > CDCLK PLL disable/enable
> > > >
> > > > It was reported that we might get a hung and loss of register access
> > > > in some cases when CDCLK PLL is disabled and then enabled, while
> > > > squashing is enabled.
> > > > As a workaround it was proposed by HW team that SW should disable
> > > > squashing when CDCLK PLL is being reenabled.
> > > >
> > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_cdclk.c | 14 ++++++++++++--
> > > >  1 file changed, 12 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > index 0c107a38f9d0..e338f288c9ac 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > @@ -1801,6 +1801,13 @@ static bool
> > > > cdclk_compute_crawl_and_squash_midpoint(struct drm_i915_private
> > *i91
> > > >  	return true;
> > > >  }
> > > >
> > > > +static bool pll_enable_wa_needed(struct drm_i915_private *dev_priv)
> > {
> > > > +	return ((IS_DG2(dev_priv) || IS_METEORLAKE(dev_priv))
> > > > +		&& dev_priv->display.cdclk.hw.vco > 0
> > > > +		&& HAS_CDCLK_SQUASH(dev_priv));
> > > Redundant check. If it is MTL or DG2, then it will have
> > HAS_CDCLK_SQUASH set to true always. Shouldn't vco be 0 instead of > 0.
> > The commit message says the hang can be observed when moving from 0 to
> > > 0 vco.
> > >
> > > Anusha
> > 
> > Idea was that we probably should bind more to the feature rather than
> > platform, I agree checking both "IS_DG2" and if platform has squashing is
> > redundant, because then we would have to add each new platform
> > manually, so I would leave HAS_CDCLK_SQUASH and then at some point
> > just cut off using some INTEL_GEN or other check all the new platforms
> > where this is fixed in HW.
> > 
> > Regarding vco, the icl_cdclk_pll_update func works as follows:
> > 
> > if (i915->display.cdclk.hw.vco != 0 &&
> >     i915->display.cdclk.hw.vco != vco)
> >     icl_cdclk_pll_disable(i915);
> > 
> > if (i915->display.cdclk.hw.vco != vco)
> >     icl_cdclk_pll_enable(i915, vco);
> > 
> > 1) if vco changes from zero value(i915->display.cdclk.hw.vco) to non-zero
> > value(vco), that means
> >    currently squashing is anyway disabled(if vco == 0, cdclk is set to "bypass"
> > and squash waveform
> >    is 0), so the W/A is not needed.
> > 
> > 2) if vco changes from non-zero value in i915->display.cdclk.hw.vco to zero
> > value(vco), we are not
> >    going to call icl_cdclk_pll_enable anyway so W/A is also not needed.
> > 
> > 3) if vco changes from some non-zero value in i915->display.cdclk.hw.vco to
> > other non-zero value(vco),
> >    which can happen if CDCLK changes, then icl_cdclk_pll_disable(hw vco!=0
> > and vco!=0) and then
> >    icl_cdclk_pll_enable would be called(hw vco is still not equal to vco)
> >    So that disabling enabling in between is what we are interested and
> > where we should make sure
> >    squashing is disabled.
> >    BTW I have another question - is this code even correct? Shouldn't we
> > avoid disabling/enabling PLL if we have
> >    squashing/crawling? As I understood the whole point of having
> > swuashing/crawling is avoiding swithing the PLL
> >    on and off.
> > 
> Squashing works when we don't need to change the PLL ratio. We fix the PLL ratio to say 307 (this can change from platform to platform). Then squashing can be used to vary frequencies below this. So we set squasher to ffff it will mean highest. We can use squasher to change frequency with squash waveform, max being ffff and any value lower will take lower frequencies. 
> Cdclk Crawling is used when the PLL has to be changed. Eg higher than 307 then we need to update PLL. In that case we first program squashing to ffff(this will take to 307) n then use crawling to go up to the desired frequency.

if (HAS_CDCLK_CRAWL(dev_priv) && dev_priv->display.cdclk.hw.vco > 0 && vco > 0 &&
    !cdclk_pll_is_unknown(dev_priv->display.cdclk.hw.vco)) {
	if (dev_priv->display.cdclk.hw.vco != vco)
		adlp_cdclk_pll_crawl(dev_priv, vco);
	} else if (DISPLAY_VER(dev_priv) >= 11) {
		if (pll_enable_wa_needed(dev_priv))
			dg2_cdclk_squash_program(dev_priv, 0);

		icl_cdclk_pll_update(dev_priv, vco);
	} else
		bxt_cdclk_pll_update(dev_priv, vco);

I think that condition above will trigger CDCLK crawl whenever vco is not ~0, CDCLK crawl is supported
and both hw.vco and vco are > 0 and vco has to be changed.

In bxt_set_cdclk which calls _bxt_set_cdclk we calculate the midpoint however I don't see
how _bxt_set_cdclk is going to distinguish between crawling and squashing.

I can see that squash waveform will be returned as 0, if CDCLK is >= 307 MHz for MTL for example,
or if CDCLK is equal to bypass, however the only way to skip crawling here seems to either have 
vco == ~0(probably after hw init) or vco == 0, but if vco == 0 we are going to then call 
icl_cdclk_pll_update which might disable pll, if current hw.vco is not 0.

So what am I missing here?

Stan

> 
> Anusha
> > Stan
> > 
> > 
> > > > +}
> > > > +
> > > >  static void _bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > > >  			   const struct intel_cdclk_config *cdclk_config,
> > > >  			   enum pipe pipe)
> > > > @@ -1815,9 +1822,12 @@ static void _bxt_set_cdclk(struct
> > > > drm_i915_private *dev_priv,
> > > >  	    !cdclk_pll_is_unknown(dev_priv->display.cdclk.hw.vco)) {
> > > >  		if (dev_priv->display.cdclk.hw.vco != vco)
> > > >  			adlp_cdclk_pll_crawl(dev_priv, vco);
> > > > -	} else if (DISPLAY_VER(dev_priv) >= 11)
> > > > +	} else if (DISPLAY_VER(dev_priv) >= 11) {
> > > > +		if (pll_enable_wa_needed(dev_priv))
> > > > +			dg2_cdclk_squash_program(dev_priv, 0);
> > > > +
> > > >  		icl_cdclk_pll_update(dev_priv, vco);
> > > > -	else
> > > > +	} else
> > > >  		bxt_cdclk_pll_update(dev_priv, vco);
> > > >
> > > >  	waveform = cdclk_squash_waveform(dev_priv, cdclk);
> > > > --
> > > > 2.37.3
> > >

Re: [Intel-gfx] [PATCH 1/1] drm/i915: Implement workaround for CDCLK PLL disable/enable

[Srivatsa, Anusha]

> -----Original Message-----
> From: Lisovskiy, Stanislav <stanislav.lisovskiy@intel.com>
> Sent: Thursday, December 15, 2022 2:14 AM
> To: Srivatsa, Anusha <anusha.srivatsa@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>
> Subject: Re: [Intel-gfx] [PATCH 1/1] drm/i915: Implement workaround for
> CDCLK PLL disable/enable
> 
> On Wed, Dec 14, 2022 at 09:15:07PM +0200, Srivatsa, Anusha wrote:
> >
> >
> > > -----Original Message-----
> > > From: Lisovskiy, Stanislav <stanislav.lisovskiy@intel.com>
> > > Sent: Wednesday, December 14, 2022 2:31 AM
> > > To: Srivatsa, Anusha <anusha.srivatsa@intel.com>
> > > Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani
> > > <jani.nikula@intel.com>
> > > Subject: Re: [Intel-gfx] [PATCH 1/1] drm/i915: Implement workaround
> > > for CDCLK PLL disable/enable
> > >
> > > On Tue, Nov 29, 2022 at 09:19:40PM +0200, Srivatsa, Anusha wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On
> > > > > Behalf Of Stanislav Lisovskiy
> > > > > Sent: Thursday, November 24, 2022 2:36 AM
> > > > > To: intel-gfx@lists.freedesktop.org
> > > > > Cc: Nikula, Jani <jani.nikula@intel.com>
> > > > > Subject: [Intel-gfx] [PATCH 1/1] drm/i915: Implement workaround
> > > > > for CDCLK PLL disable/enable
> > > > >
> > > > > It was reported that we might get a hung and loss of register
> > > > > access in some cases when CDCLK PLL is disabled and then
> > > > > enabled, while squashing is enabled.
> > > > > As a workaround it was proposed by HW team that SW should
> > > > > disable squashing when CDCLK PLL is being reenabled.
> > > > >
> > > > > Signed-off-by: Stanislav Lisovskiy
> > > > > <stanislav.lisovskiy@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_cdclk.c | 14 ++++++++++++--
> > > > >  1 file changed, 12 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > > b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > > index 0c107a38f9d0..e338f288c9ac 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > > @@ -1801,6 +1801,13 @@ static bool
> > > > > cdclk_compute_crawl_and_squash_midpoint(struct
> drm_i915_private
> > > *i91
> > > > >  	return true;
> > > > >  }
> > > > >
> > > > > +static bool pll_enable_wa_needed(struct drm_i915_private
> > > > > +*dev_priv)
> > > {
> > > > > +	return ((IS_DG2(dev_priv) || IS_METEORLAKE(dev_priv))
> > > > > +		&& dev_priv->display.cdclk.hw.vco > 0
> > > > > +		&& HAS_CDCLK_SQUASH(dev_priv));
> > > > Redundant check. If it is MTL or DG2, then it will have
> > > HAS_CDCLK_SQUASH set to true always. Shouldn't vco be 0 instead of >
> 0.
> > > The commit message says the hang can be observed when moving from
> 0
> > > to
> > > > 0 vco.
> > > >
> > > > Anusha
> > >
> > > Idea was that we probably should bind more to the feature rather
> > > than platform, I agree checking both "IS_DG2" and if platform has
> > > squashing is redundant, because then we would have to add each new
> > > platform manually, so I would leave HAS_CDCLK_SQUASH and then at
> > > some point just cut off using some INTEL_GEN or other check all the
> > > new platforms where this is fixed in HW.
> > >
> > > Regarding vco, the icl_cdclk_pll_update func works as follows:
> > >
> > > if (i915->display.cdclk.hw.vco != 0 &&
> > >     i915->display.cdclk.hw.vco != vco)
> > >     icl_cdclk_pll_disable(i915);
> > >
> > > if (i915->display.cdclk.hw.vco != vco)
> > >     icl_cdclk_pll_enable(i915, vco);
> > >
> > > 1) if vco changes from zero value(i915->display.cdclk.hw.vco) to
> > > non-zero value(vco), that means
> > >    currently squashing is anyway disabled(if vco == 0, cdclk is set to
> "bypass"
> > > and squash waveform
> > >    is 0), so the W/A is not needed.
> > >
> > > 2) if vco changes from non-zero value in i915->display.cdclk.hw.vco
> > > to zero value(vco), we are not
> > >    going to call icl_cdclk_pll_enable anyway so W/A is also not needed.
> > >
> > > 3) if vco changes from some non-zero value in
> > > i915->display.cdclk.hw.vco to other non-zero value(vco),
> > >    which can happen if CDCLK changes, then icl_cdclk_pll_disable(hw
> > > vco!=0 and vco!=0) and then
> > >    icl_cdclk_pll_enable would be called(hw vco is still not equal to vco)
> > >    So that disabling enabling in between is what we are interested
> > > and where we should make sure
> > >    squashing is disabled.
> > >    BTW I have another question - is this code even correct?
> > > Shouldn't we avoid disabling/enabling PLL if we have
> > >    squashing/crawling? As I understood the whole point of having
> > > swuashing/crawling is avoiding swithing the PLL
> > >    on and off.
> > >
> > Squashing works when we don't need to change the PLL ratio. We fix the
> PLL ratio to say 307 (this can change from platform to platform). Then
> squashing can be used to vary frequencies below this. So we set squasher
> to ffff it will mean highest. We can use squasher to change frequency with
> squash waveform, max being ffff and any value lower will take lower
> frequencies.
> > Cdclk Crawling is used when the PLL has to be changed. Eg higher than
> 307 then we need to update PLL. In that case we first program squashing to
> ffff(this will take to 307) n then use crawling to go up to the desired
> frequency.
> 
> if (HAS_CDCLK_CRAWL(dev_priv) && dev_priv->display.cdclk.hw.vco > 0 &&
> vco > 0 &&
>     !cdclk_pll_is_unknown(dev_priv->display.cdclk.hw.vco)) {
> 	if (dev_priv->display.cdclk.hw.vco != vco)
> 		adlp_cdclk_pll_crawl(dev_priv, vco);
> 	} else if (DISPLAY_VER(dev_priv) >= 11) {
> 		if (pll_enable_wa_needed(dev_priv))
> 			dg2_cdclk_squash_program(dev_priv, 0);
> 
> 		icl_cdclk_pll_update(dev_priv, vco);
> 	} else
> 		bxt_cdclk_pll_update(dev_priv, vco);
> 
> I think that condition above will trigger CDCLK crawl whenever vco is not ~0,
> CDCLK crawl is supported and both hw.vco and vco are > 0 and vco has to
> be changed.

Why?

> 
> In bxt_set_cdclk which calls _bxt_set_cdclk we calculate the midpoint
> however I don't see how _bxt_set_cdclk is going to distinguish between
> crawling and squashing.

We are populating the mid_cdclk_config according to the desired action.
"/* - If moving to a higher cdclk, the desired action is squashing.
         * The mid cdclk config should have the new (squash) waveform.
         * - If moving to a lower cdclk, the desired action is crawling.
         * The mid cdclk config should have the new vco.
         */"

Anusha
> I can see that squash waveform will be returned as 0, if CDCLK is >= 307
> MHz for MTL for example, or if CDCLK is equal to bypass, however the only
> way to skip crawling here seems to either have vco == ~0(probably after hw
> init) or vco == 0, but if vco == 0 we are going to then call
> icl_cdclk_pll_update which might disable pll, if current hw.vco is not 0.
> 
> So what am I missing here?
> 
> Stan
> 
> >
> > Anusha
> > > Stan
> > >
> > >
> > > > > +}
> > > > > +
> > > > >  static void _bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > > > >  			   const struct intel_cdclk_config
> *cdclk_config,
> > > > >  			   enum pipe pipe)
> > > > > @@ -1815,9 +1822,12 @@ static void _bxt_set_cdclk(struct
> > > > > drm_i915_private *dev_priv,
> > > > >  	    !cdclk_pll_is_unknown(dev_priv->display.cdclk.hw.vco)) {
> > > > >  		if (dev_priv->display.cdclk.hw.vco != vco)
> > > > >  			adlp_cdclk_pll_crawl(dev_priv, vco);
> > > > > -	} else if (DISPLAY_VER(dev_priv) >= 11)
> > > > > +	} else if (DISPLAY_VER(dev_priv) >= 11) {
> > > > > +		if (pll_enable_wa_needed(dev_priv))
> > > > > +			dg2_cdclk_squash_program(dev_priv, 0);
> > > > > +
> > > > >  		icl_cdclk_pll_update(dev_priv, vco);
> > > > > -	else
> > > > > +	} else
> > > > >  		bxt_cdclk_pll_update(dev_priv, vco);
> > > > >
> > > > >  	waveform = cdclk_squash_waveform(dev_priv, cdclk);
> > > > > --
> > > > > 2.37.3
> > > >

Re: [Intel-gfx] [PATCH 1/1] drm/i915: Implement workaround for CDCLK PLL disable/enable

[Lisovskiy, Stanislav]

On Thu, Jan 05, 2023 at 03:05:40AM +0200, Srivatsa, Anusha wrote:
> 
> 
> > -----Original Message-----
> > From: Lisovskiy, Stanislav <stanislav.lisovskiy@intel.com>
> > Sent: Thursday, December 15, 2022 2:14 AM
> > To: Srivatsa, Anusha <anusha.srivatsa@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani <jani.nikula@intel.com>
> > Subject: Re: [Intel-gfx] [PATCH 1/1] drm/i915: Implement workaround for
> > CDCLK PLL disable/enable
> > 
> > On Wed, Dec 14, 2022 at 09:15:07PM +0200, Srivatsa, Anusha wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Lisovskiy, Stanislav <stanislav.lisovskiy@intel.com>
> > > > Sent: Wednesday, December 14, 2022 2:31 AM
> > > > To: Srivatsa, Anusha <anusha.srivatsa@intel.com>
> > > > Cc: intel-gfx@lists.freedesktop.org; Nikula, Jani
> > > > <jani.nikula@intel.com>
> > > > Subject: Re: [Intel-gfx] [PATCH 1/1] drm/i915: Implement workaround
> > > > for CDCLK PLL disable/enable
> > > >
> > > > On Tue, Nov 29, 2022 at 09:19:40PM +0200, Srivatsa, Anusha wrote:
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On
> > > > > > Behalf Of Stanislav Lisovskiy
> > > > > > Sent: Thursday, November 24, 2022 2:36 AM
> > > > > > To: intel-gfx@lists.freedesktop.org
> > > > > > Cc: Nikula, Jani <jani.nikula@intel.com>
> > > > > > Subject: [Intel-gfx] [PATCH 1/1] drm/i915: Implement workaround
> > > > > > for CDCLK PLL disable/enable
> > > > > >
> > > > > > It was reported that we might get a hung and loss of register
> > > > > > access in some cases when CDCLK PLL is disabled and then
> > > > > > enabled, while squashing is enabled.
> > > > > > As a workaround it was proposed by HW team that SW should
> > > > > > disable squashing when CDCLK PLL is being reenabled.
> > > > > >
> > > > > > Signed-off-by: Stanislav Lisovskiy
> > > > > > <stanislav.lisovskiy@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/display/intel_cdclk.c | 14 ++++++++++++--
> > > > > >  1 file changed, 12 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > > > b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > > > index 0c107a38f9d0..e338f288c9ac 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > > > @@ -1801,6 +1801,13 @@ static bool
> > > > > > cdclk_compute_crawl_and_squash_midpoint(struct
> > drm_i915_private
> > > > *i91
> > > > > >  	return true;
> > > > > >  }
> > > > > >
> > > > > > +static bool pll_enable_wa_needed(struct drm_i915_private
> > > > > > +*dev_priv)
> > > > {
> > > > > > +	return ((IS_DG2(dev_priv) || IS_METEORLAKE(dev_priv))
> > > > > > +		&& dev_priv->display.cdclk.hw.vco > 0
> > > > > > +		&& HAS_CDCLK_SQUASH(dev_priv));
> > > > > Redundant check. If it is MTL or DG2, then it will have
> > > > HAS_CDCLK_SQUASH set to true always. Shouldn't vco be 0 instead of >
> > 0.
> > > > The commit message says the hang can be observed when moving from
> > 0
> > > > to
> > > > > 0 vco.
> > > > >
> > > > > Anusha
> > > >
> > > > Idea was that we probably should bind more to the feature rather
> > > > than platform, I agree checking both "IS_DG2" and if platform has
> > > > squashing is redundant, because then we would have to add each new
> > > > platform manually, so I would leave HAS_CDCLK_SQUASH and then at
> > > > some point just cut off using some INTEL_GEN or other check all the
> > > > new platforms where this is fixed in HW.
> > > >
> > > > Regarding vco, the icl_cdclk_pll_update func works as follows:
> > > >
> > > > if (i915->display.cdclk.hw.vco != 0 &&
> > > >     i915->display.cdclk.hw.vco != vco)
> > > >     icl_cdclk_pll_disable(i915);
> > > >
> > > > if (i915->display.cdclk.hw.vco != vco)
> > > >     icl_cdclk_pll_enable(i915, vco);
> > > >
> > > > 1) if vco changes from zero value(i915->display.cdclk.hw.vco) to
> > > > non-zero value(vco), that means
> > > >    currently squashing is anyway disabled(if vco == 0, cdclk is set to
> > "bypass"
> > > > and squash waveform
> > > >    is 0), so the W/A is not needed.
> > > >
> > > > 2) if vco changes from non-zero value in i915->display.cdclk.hw.vco
> > > > to zero value(vco), we are not
> > > >    going to call icl_cdclk_pll_enable anyway so W/A is also not needed.
> > > >
> > > > 3) if vco changes from some non-zero value in
> > > > i915->display.cdclk.hw.vco to other non-zero value(vco),
> > > >    which can happen if CDCLK changes, then icl_cdclk_pll_disable(hw
> > > > vco!=0 and vco!=0) and then
> > > >    icl_cdclk_pll_enable would be called(hw vco is still not equal to vco)
> > > >    So that disabling enabling in between is what we are interested
> > > > and where we should make sure
> > > >    squashing is disabled.
> > > >    BTW I have another question - is this code even correct?
> > > > Shouldn't we avoid disabling/enabling PLL if we have
> > > >    squashing/crawling? As I understood the whole point of having
> > > > swuashing/crawling is avoiding swithing the PLL
> > > >    on and off.
> > > >
> > > Squashing works when we don't need to change the PLL ratio. We fix the
> > PLL ratio to say 307 (this can change from platform to platform). Then
> > squashing can be used to vary frequencies below this. So we set squasher
> > to ffff it will mean highest. We can use squasher to change frequency with
> > squash waveform, max being ffff and any value lower will take lower
> > frequencies.
> > > Cdclk Crawling is used when the PLL has to be changed. Eg higher than
> > 307 then we need to update PLL. In that case we first program squashing to
> > ffff(this will take to 307) n then use crawling to go up to the desired
> > frequency.
> > 
> > if (HAS_CDCLK_CRAWL(dev_priv) && dev_priv->display.cdclk.hw.vco > 0 &&
> > vco > 0 &&
> >     !cdclk_pll_is_unknown(dev_priv->display.cdclk.hw.vco)) {
> > 	if (dev_priv->display.cdclk.hw.vco != vco)
> > 		adlp_cdclk_pll_crawl(dev_priv, vco);
> > 	} else if (DISPLAY_VER(dev_priv) >= 11) {
> > 		if (pll_enable_wa_needed(dev_priv))
> > 			dg2_cdclk_squash_program(dev_priv, 0);
> > 
> > 		icl_cdclk_pll_update(dev_priv, vco);
> > 	} else
> > 		bxt_cdclk_pll_update(dev_priv, vco);
> > 
> > I think that condition above will trigger CDCLK crawl whenever vco is not ~0,
> > CDCLK crawl is supported and both hw.vco and vco are > 0 and vco has to
> > be changed.
> 
> Why?

Well... Because that is exactly what this condition does:
1) !cdclk_pll_is_unknown(dev_priv->display.cdclk.hw.vco) means hw.vco is not ~0
2) HAS_CDCLK_CRAWL(dev_priv) checks if CDCLK crawl is supported
3) dev_priv->display.cdclk.hw.vco > 0 && vco > 0 check if both hw.vco and vco are > 0
4) and if (dev_priv->display.cdclk.hw.vco != vco) checks if new vco is different from hw.vco

Summing all this exactly produces my statement above.


> 
> > 
> > In bxt_set_cdclk which calls _bxt_set_cdclk we calculate the midpoint
> > however I don't see how _bxt_set_cdclk is going to distinguish between
> > crawling and squashing.
> 
> We are populating the mid_cdclk_config according to the desired action.
> "/* - If moving to a higher cdclk, the desired action is squashing.
>          * The mid cdclk config should have the new (squash) waveform.
>          * - If moving to a lower cdclk, the desired action is crawling.
>          * The mid cdclk config should have the new vco.
>          */"
> 
> Anusha

Ok, lets imagine we are moving to a higher CDCLK, lets say from 480000 to
556800. 
As you say above desired action is then squashing.
new_cdclk_config->vco will be then different than current vco, because
it is calculated as dev_priv->display.cdclk.hw.ref * table[i].ratio and
ratios for those CDCLK's are different in mtl_cdclk_table.
However waveforms for those are the same(0x0000 in mtl_cdclk_table) which 
means cdclk_compute_crawl_and_squash_midpoint will return false.
Which means we want squash/crawl only. According to your comment above
we want squash only.
Then _bxt_set_cdclk gets called from bxt_set_cdclk here, without any midpoint:

} else {
        _bxt_set_cdclk(dev_priv, cdclk_config, pipe);
}

In _bxt_set_cdclk we now at the condition:

if (HAS_CDCLK_CRAWL(dev_priv) && dev_priv->display.cdclk.hw.vco > 0 && vco > 0 &&
            !cdclk_pll_is_unknown(dev_priv->display.cdclk.hw.vco)) {
        if (dev_priv->display.cdclk.hw.vco != vco)

And _what_ now prevents it not to be always executed? 

Both old and new vco are > 0, HAS_CDCLK_CRAWL is true, hw.vco is not ~0 and
also obviously dev_priv->display.cdclk.hw.vco != vco is also true.

Then we get adlp_cdclk_pll_crawl always executed here.

So I think what is happening currently is driven by that mtl_cdclk_table
rather than new cdclk being higher or lower than old cdclk.

adlp_cdclk_pll_crawl will be called whenever vco had changed, regardless
or whether we want squashing or not.

Stan


> > I can see that squash waveform will be returned as 0, if CDCLK is >= 307
> > MHz for MTL for example, or if CDCLK is equal to bypass, however the only
> > way to skip crawling here seems to either have vco == ~0(probably after hw
> > init) or vco == 0, but if vco == 0 we are going to then call
> > icl_cdclk_pll_update which might disable pll, if current hw.vco is not 0.
> > 
> > So what am I missing here?
> > 
> > Stan
> > 
> > >
> > > Anusha
> > > > Stan
> > > >
> > > >
> > > > > > +}
> > > > > > +
> > > > > >  static void _bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > > > > >  			   const struct intel_cdclk_config
> > *cdclk_config,
> > > > > >  			   enum pipe pipe)
> > > > > > @@ -1815,9 +1822,12 @@ static void _bxt_set_cdclk(struct
> > > > > > drm_i915_private *dev_priv,
> > > > > >  	    !cdclk_pll_is_unknown(dev_priv->display.cdclk.hw.vco)) {
> > > > > >  		if (dev_priv->display.cdclk.hw.vco != vco)
> > > > > >  			adlp_cdclk_pll_crawl(dev_priv, vco);
> > > > > > -	} else if (DISPLAY_VER(dev_priv) >= 11)
> > > > > > +	} else if (DISPLAY_VER(dev_priv) >= 11) {
> > > > > > +		if (pll_enable_wa_needed(dev_priv))
> > > > > > +			dg2_cdclk_squash_program(dev_priv, 0);
> > > > > > +
> > > > > >  		icl_cdclk_pll_update(dev_priv, vco);
> > > > > > -	else
> > > > > > +	} else
> > > > > >  		bxt_cdclk_pll_update(dev_priv, vco);
> > > > > >
> > > > > >  	waveform = cdclk_squash_waveform(dev_priv, cdclk);
> > > > > > --
> > > > > > 2.37.3
> > > > >