[PATCH] drm/i915/display: Pcode sets the CD clock frequency voltage levels
Kahola, Mika
mika.kahola at intel.com
Thu Nov 7 08:55:30 UTC 2024
> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Sent: Wednesday, 6 November 2024 17.48
> To: Kahola, Mika <mika.kahola at intel.com>
> Cc: intel-xe at lists.freedesktop.org
> Subject: Re: [PATCH] drm/i915/display: Pcode sets the CD clock frequency voltage
> levels
>
> 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 at 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.
Maybe we can then drop this patch then. We can keep the original approach.
Thanks!
>
> > -
> > 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
More information about the Intel-xe
mailing list