[Intel-gfx] [Intel-xe] [PATCH 34/42] drm/i915/lnl: Start using CDCLK through PLL

[Lucas De Marchi]

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 at intel.com>
>>
>> Introduce correspondent definitions and for choosing between CD2X CDCLK
>> and PLL CDCLK as a source.
>>
>> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at 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