Re: [PATCH] drm/i915/display: Pcode sets the CD clock frequency voltage levels

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Nov 06, 2024 at 05:20:23PM +0200, Mika Kahola wrote:
> On xe3lpd the CD clock frequency is set by the driver
> without setting voltage levels. Previously, we had 4
> levels of voltages to choose from. However, having only 4
> levels is rather coarse and we may end up consuming more
> power than necessary. Therefore, these are now removed
> and choosing the correct voltage level is now handed over
> to Pcode firmware which is able to set voltage level with
> higher granularity.
> 
> Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.c | 48 +++++++++++-----------
>  drivers/gpu/drm/i915/i915_reg.h            |  3 ++
>  2 files changed, 28 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index 03c4eef3f92a..72da06e77815 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -1627,16 +1627,6 @@ static u8 rplu_calc_voltage_level(int cdclk)
>  				  rplu_voltage_level_max_cdclk);
>  }
>  
> -static u8 xe3lpd_calc_voltage_level(int cdclk)
> -{
> -	/*
> -	 * Starting with xe3lpd power controller does not need the voltage
> -	 * index when doing the modeset update. This function is best left
> -	 * defined but returning 0 to the mask.
> -	 */
> -	return 0;
> -}

I think I prefer the original approach here. Avoids all those
ugly if statements all over the place.

> -
>  static void icl_readout_refclk(struct intel_display *display,
>  			       struct intel_cdclk_config *cdclk_config)
>  {
> @@ -1758,8 +1748,9 @@ static void bxt_get_cdclk(struct intel_display *display,
>  	 * Can't read this out :( Let's assume it's
>  	 * at least what the CDCLK frequency requires.
>  	 */
> -	cdclk_config->voltage_level =
> -		intel_cdclk_calc_voltage_level(display, cdclk_config->cdclk);
> +	if (DISPLAY_VER(display) < 30)
> +		cdclk_config->voltage_level =
> +			intel_cdclk_calc_voltage_level(display, cdclk_config->cdclk);
>  }
>  
>  static void bxt_de_pll_disable(struct intel_display *display)
> @@ -2209,7 +2200,7 @@ static void bxt_set_cdclk(struct intel_display *display,
>  
>  	intel_update_cdclk(display);
>  
> -	if (DISPLAY_VER(display) >= 11)
> +	if (DISPLAY_VER(display) >= 11 && DISPLAY_VER(display) < 30)
>  		/*
>  		 * Can't read out the voltage level :(
>  		 * Let's just assume everything is as expected.
> @@ -2288,8 +2279,10 @@ static void bxt_cdclk_init_hw(struct intel_display *display)
>  	 */
>  	cdclk_config.cdclk = bxt_calc_cdclk(display, 0);
>  	cdclk_config.vco = bxt_calc_cdclk_pll_vco(display, cdclk_config.cdclk);
> -	cdclk_config.voltage_level =
> -		intel_cdclk_calc_voltage_level(display, cdclk_config.cdclk);
> +
> +	if (DISPLAY_VER(display) < 30)
> +		cdclk_config.voltage_level =
> +			intel_cdclk_calc_voltage_level(display, cdclk_config.cdclk);
>  
>  	bxt_set_cdclk(display, &cdclk_config, INVALID_PIPE);
>  }
> @@ -2300,8 +2293,10 @@ static void bxt_cdclk_uninit_hw(struct intel_display *display)
>  
>  	cdclk_config.cdclk = cdclk_config.bypass;
>  	cdclk_config.vco = 0;
> -	cdclk_config.voltage_level =
> -		intel_cdclk_calc_voltage_level(display, cdclk_config.cdclk);
> +
> +	if (DISPLAY_VER(display) < 30)
> +		cdclk_config.voltage_level =
> +			intel_cdclk_calc_voltage_level(display, cdclk_config.cdclk);
>  
>  	bxt_set_cdclk(display, &cdclk_config, INVALID_PIPE);
>  }
> @@ -2477,10 +2472,15 @@ void intel_cdclk_dump_config(struct intel_display *display,
>  			     const struct intel_cdclk_config *cdclk_config,
>  			     const char *context)
>  {
> -	drm_dbg_kms(display->drm, "%s %d kHz, VCO %d kHz, ref %d kHz, bypass %d kHz, voltage level %d\n",
> -		    context, cdclk_config->cdclk, cdclk_config->vco,
> -		    cdclk_config->ref, cdclk_config->bypass,
> -		    cdclk_config->voltage_level);
> +	if (DISPLAY_VER(display) == 30)
> +		drm_dbg_kms(display->drm, "%s %d kHz, VCO %d kHz, ref %d kHz, bypass %d kHz\n",
> +			    context, cdclk_config->cdclk, cdclk_config->vco,
> +			    cdclk_config->ref, cdclk_config->bypass);
> +	else
> +		drm_dbg_kms(display->drm, "%s %d kHz, VCO %d kHz, ref %d kHz, bypass %d kHz, voltage level %d\n",
> +			    context, cdclk_config->cdclk, cdclk_config->vco,
> +			    cdclk_config->ref, cdclk_config->bypass,
> +			    cdclk_config->voltage_level);
>  }
>  
>  static void intel_pcode_notify(struct intel_display *display,
> @@ -2497,7 +2497,10 @@ static void intel_pcode_notify(struct intel_display *display,
>  	if (!IS_DG2(i915))
>  		return;
>  
> -	update_mask = DISPLAY_TO_PCODE_UPDATE_MASK(cdclk, active_pipe_count, voltage_level);
> +	if (DISPLAY_VER(i915) == 30)
> +		update_mask = DISPLAY30_TO_PCODE_UPDATE_MASK(cdclk, active_pipe_count);
> +	else
> +		update_mask = DISPLAY_TO_PCODE_UPDATE_MASK(cdclk, active_pipe_count, voltage_level);
>  
>  	if (cdclk_update_valid)
>  		update_mask |= DISPLAY_TO_PCODE_CDCLK_VALID;
> @@ -3699,7 +3702,6 @@ static const struct intel_cdclk_funcs xe3lpd_cdclk_funcs = {
>  	.get_cdclk = bxt_get_cdclk,
>  	.set_cdclk = bxt_set_cdclk,
>  	.modeset_calc_cdclk = bxt_modeset_calc_cdclk,
> -	.calc_voltage_level = xe3lpd_calc_voltage_level,
>  };
>  
>  static const struct intel_cdclk_funcs rplu_cdclk_funcs = {
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index c160e087972a..5d0b13460631 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3581,6 +3581,9 @@
>  		((DISPLAY_TO_PCODE_CDCLK(cdclk)) | \
>  		(DISPLAY_TO_PCODE_PIPE_COUNT(num_pipes)) | \
>  		(DISPLAY_TO_PCODE_VOLTAGE(voltage_level)))
> +#define   DISPLAY30_TO_PCODE_UPDATE_MASK(cdclk, num_pipes) \
> +		((DISPLAY_TO_PCODE_CDCLK(cdclk)) | \
> +		(DISPLAY_TO_PCODE_PIPE_COUNT(num_pipes)))
>  #define   ICL_PCODE_SAGV_DE_MEM_SS_CONFIG	0xe
>  #define     ICL_PCODE_REP_QGV_MASK		REG_GENMASK(1, 0)
>  #define     ICL_PCODE_REP_QGV_SAFE		REG_FIELD_PREP(ICL_PCODE_REP_QGV_MASK, 0)
> -- 
> 2.43.0

-- 
Ville Syrjälä
Intel