Re: [Intel-gfx] [Intel-xe] [PATCH 34/42] drm/i915/lnl: Start using CDCLK through PLL
On Wed, Aug 23, 2023 at 03:01:37PM -0700, Matt Roper wrote:
On Wed, Aug 23, 2023 at 10:07:32AM -0700, Lucas De Marchi wrote:
From: Stanislav Lisovskiy
Introduce correspondent definitions and for choosing between CD2X CDCLK
and PLL CDCLK as a source.
Signed-off-by: Stanislav Lisovskiy
Signed-off-by: Lucas De Marchi
---
drivers/gpu/drm/i915/display/intel_cdclk.c | 14 --
drivers/gpu/drm/i915/i915_reg.h| 3 +++
2 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
b/drivers/gpu/drm/i915/display/intel_cdclk.c
index ed45a2cf5c9a..04937aaabcee 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -1932,8 +1932,7 @@ static void _bxt_set_cdclk(struct drm_i915_private
*dev_priv,
dg2_cdclk_squash_program(dev_priv, waveform);
val = bxt_cdclk_cd2x_div_sel(dev_priv, clock, vco) |
- bxt_cdclk_cd2x_pipe(dev_priv, pipe) |
- skl_cdclk_decimal(cdclk);
+ bxt_cdclk_cd2x_pipe(dev_priv, pipe);
/*
* Disable SSA Precharge when CD clock frequency < 500 MHz,
@@ -1942,6 +1941,17 @@ static void _bxt_set_cdclk(struct drm_i915_private
*dev_priv,
if ((IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv)) &&
cdclk >= 50)
val |= BXT_CDCLK_SSA_PRECHARGE_ENABLE;
+
+ if (DISPLAY_VER(dev_priv) >= 20)
+ /*
+* Using CDCLK through PLL seems to be always better option when
+* its supported, both in terms of performance and power
+* consumption.
+*/
I'm not sure what this comment is based on. But the table on bspec
68861 specifically tells us to set this bit for the cdclk table we
implemented in the last patch, so the logic is correct regardless.
I think this is more a justification for having the entries in the
table. I agree there is no need for it here as the driver is simply
implementing what is the spec.
+ val |= CDCLK_SOURCE_SEL_CDCLK_PLL;
+ else
+ val |= skl_cdclk_decimal(cdclk);
+
intel_de_write(dev_priv, CDCLK_CTL, val);
if (pipe != INVALID_PIPE)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index fa85530afac3..d5850761a75a 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5933,6 +5933,9 @@ enum skl_power_gate {
#define CDCLK_FREQ_540REG_FIELD_PREP(CDCLK_FREQ_SEL_MASK, 1)
#define CDCLK_FREQ_337_308REG_FIELD_PREP(CDCLK_FREQ_SEL_MASK, 2)
#define CDCLK_FREQ_675_617REG_FIELD_PREP(CDCLK_FREQ_SEL_MASK, 3)
+#define CDCLK_SOURCE_SEL_MASK REG_BIT(25)
+#define CDCLK_SOURCE_SEL_CD2X REG_FIELD_PREP(CDCLK_SOURCE_SEL_MASK, 0)
No need to make a single-bit "mask" or define the unused
CDCLK_SOURCE_SEL_CD2X here. We can just define
CDCLK_SOURCE_SEL_CDCLK_PLL as REG_BIT(25) directly.
will change in v2.
thanks
Lucas De Marchi
Matt
+#define CDCLK_SOURCE_SEL_CDCLK_PLLREG_FIELD_PREP(CDCLK_SOURCE_SEL_MASK, 1)
#define BXT_CDCLK_CD2X_DIV_SEL_MASK REG_GENMASK(23, 22)
#define BXT_CDCLK_CD2X_DIV_SEL_1
REG_FIELD_PREP(BXT_CDCLK_CD2X_DIV_SEL_MASK, 0)
#define BXT_CDCLK_CD2X_DIV_SEL_1_5
REG_FIELD_PREP(BXT_CDCLK_CD2X_DIV_SEL_MASK, 1)
--
2.40.1
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
Re: [Intel-gfx] [Intel-xe] [PATCH 34/42] drm/i915/lnl: Start using CDCLK through PLL
On Wed, Aug 23, 2023 at 10:07:32AM -0700, Lucas De Marchi wrote:
> From: Stanislav Lisovskiy
>
> Introduce correspondent definitions and for choosing between CD2X CDCLK
> and PLL CDCLK as a source.
>
> Signed-off-by: Stanislav Lisovskiy
> Signed-off-by: Lucas De Marchi
> ---
> drivers/gpu/drm/i915/display/intel_cdclk.c | 14 --
> drivers/gpu/drm/i915/i915_reg.h| 3 +++
> 2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
> b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index ed45a2cf5c9a..04937aaabcee 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -1932,8 +1932,7 @@ static void _bxt_set_cdclk(struct drm_i915_private
> *dev_priv,
> dg2_cdclk_squash_program(dev_priv, waveform);
>
> val = bxt_cdclk_cd2x_div_sel(dev_priv, clock, vco) |
> - bxt_cdclk_cd2x_pipe(dev_priv, pipe) |
> - skl_cdclk_decimal(cdclk);
> + bxt_cdclk_cd2x_pipe(dev_priv, pipe);
>
> /*
>* Disable SSA Precharge when CD clock frequency < 500 MHz,
> @@ -1942,6 +1941,17 @@ static void _bxt_set_cdclk(struct drm_i915_private
> *dev_priv,
> if ((IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv)) &&
> cdclk >= 50)
> val |= BXT_CDCLK_SSA_PRECHARGE_ENABLE;
> +
> + if (DISPLAY_VER(dev_priv) >= 20)
> + /*
> + * Using CDCLK through PLL seems to be always better option when
> + * its supported, both in terms of performance and power
> + * consumption.
> + */
I'm not sure what this comment is based on. But the table on bspec
68861 specifically tells us to set this bit for the cdclk table we
implemented in the last patch, so the logic is correct regardless.
> + val |= CDCLK_SOURCE_SEL_CDCLK_PLL;
> + else
> + val |= skl_cdclk_decimal(cdclk);
> +
> intel_de_write(dev_priv, CDCLK_CTL, val);
>
> if (pipe != INVALID_PIPE)
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index fa85530afac3..d5850761a75a 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5933,6 +5933,9 @@ enum skl_power_gate {
> #define CDCLK_FREQ_540 REG_FIELD_PREP(CDCLK_FREQ_SEL_MASK, 1)
> #define CDCLK_FREQ_337_308 REG_FIELD_PREP(CDCLK_FREQ_SEL_MASK, 2)
> #define CDCLK_FREQ_675_617 REG_FIELD_PREP(CDCLK_FREQ_SEL_MASK, 3)
> +#define CDCLK_SOURCE_SEL_MASK REG_BIT(25)
> +#define CDCLK_SOURCE_SEL_CD2X
> REG_FIELD_PREP(CDCLK_SOURCE_SEL_MASK, 0)
No need to make a single-bit "mask" or define the unused
CDCLK_SOURCE_SEL_CD2X here. We can just define
CDCLK_SOURCE_SEL_CDCLK_PLL as REG_BIT(25) directly.
Matt
> +#define CDCLK_SOURCE_SEL_CDCLK_PLL REG_FIELD_PREP(CDCLK_SOURCE_SEL_MASK, 1)
> #define BXT_CDCLK_CD2X_DIV_SEL_MASK REG_GENMASK(23, 22)
> #define BXT_CDCLK_CD2X_DIV_SEL_1
> REG_FIELD_PREP(BXT_CDCLK_CD2X_DIV_SEL_MASK, 0)
> #define BXT_CDCLK_CD2X_DIV_SEL_1_5
> REG_FIELD_PREP(BXT_CDCLK_CD2X_DIV_SEL_MASK, 1)
> --
> 2.40.1
>
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
