diff mbox series

[34/42] drm/i915/lnl: Start using CDCLK through PLL

Message ID 20230823170740.1180212-35-lucas.demarchi@intel.com (mailing list archive)
State New, archived
Headers show
Series Enable Lunar Lake display | expand

Commit Message

Lucas De Marchi Aug. 23, 2023, 5:07 p.m. UTC 
From: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

Introduce correspondent definitions and for choosing between CD2X CDCLK
and PLL CDCLK as a source.

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/display/intel_cdclk.c | 14 ++++++++++++--
 drivers/gpu/drm/i915/i915_reg.h            |  3 +++
 2 files changed, 15 insertions(+), 2 deletions(-)

Comments

Matt Roper Aug. 23, 2023, 10:01 p.m. UTC | #1 
On Wed, Aug 23, 2023 at 10:07:32AM -0700, Lucas De Marchi wrote:
> From: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> 
> Introduce correspondent definitions and for choosing between CD2X CDCLK
> and PLL CDCLK as a source.
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  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 >= 500000)
>  		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
>
Lucas De Marchi Aug. 29, 2023, 6:45 p.m. UTC | #2 
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 <stanislav.lisovskiy@intel.com>
>>
>> Introduce correspondent definitions and for choosing between CD2X CDCLK
>> and PLL CDCLK as a source.
>>
>> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>  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 >= 500000)
>>  		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_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.

will change in v2.

thanks
Lucas De Marchi

>
>
>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
diff mbox series

Patch

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 >= 500000)
 		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.
+		 */
+		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)
+#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)