[Intel-gfx] [PATCH 2/8] drm/i915: Start tracking voltage level in the cdclk state
Rodrigo Vivi
rodrigo.vivi at intel.com
Mon Oct 23 17:14:10 UTC 2017
On Mon, Oct 23, 2017 at 12:13:46PM +0000, Ville Syrjälä wrote:
> On Fri, Oct 20, 2017 at 01:43:51PM -0700, Rodrigo Vivi wrote:
> > On Fri, Oct 20, 2017 at 02:01:37PM +0000, Ville Syrjälä wrote:
> > > On Wed, Oct 18, 2017 at 11:48:19PM +0300, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > >
> > > > For CNL we'll need to start considering the port clocks when we select
> > > > the voltage level for the system agent. To that end start tracking the
> > > > voltage in the cdclk state (since that already has to adjust it).
> > > >
> > > > Cc: Mika Kahola <mika.kahola at intel.com>
> > > > Cc: Manasi Navare <manasi.d.navare at intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/i915_drv.h | 1 +
> > > > drivers/gpu/drm/i915/intel_cdclk.c | 31 ++++++++++++++++++++++++-------
> > > > drivers/gpu/drm/i915/intel_display.c | 8 ++++----
> > > > drivers/gpu/drm/i915/intel_drv.h | 4 +++-
> > > > drivers/gpu/drm/i915/intel_runtime_pm.c | 3 ++-
> > > > 5 files changed, 34 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > index f01c80076c59..d3ac58dc275f 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -2227,6 +2227,7 @@ struct i915_oa_ops {
> > > >
> > > > struct intel_cdclk_state {
> > > > unsigned int cdclk, vco, ref;
> > > > + u8 voltage;
> > >
> > > BTW now that I decided to abuse this for all platforms the name
> > > "voltage" might not be entirely accurate anymore. Especially on VLV/CHV
> > > the DSPFREQUAR value isn't directly a voltage value, but instead a
> > > request to punit to change the cdclk frequency to a specific value.
> > > Although naturally punit will also make sure the voltage will be
> > > set sufficiently high as well.
> > >
> > > So I'm not entirely sure we want to call it "voltage" anymore. Something
> > > abstract like "level" is also what came to mind when I was thingking
> > > about this. If anyone is irked by "voltage" and would like to see
> > > something different there, let me know. Otherwise I think I might just
> > > leave it as is for now.
> >
> > I honestly prefer "dvfs_level", voltage_level or anything_level.
>
> I think voltage_level makes a ton of sense, and looks like that's
> actually what the CNL spec calls it as well (at least in some places).
> While still not 100% accurate for VLV/CHV I suppose, I think it's more
> clear than just "level" for more recent platforms.
>
> Any objections to "voltage_level"?
none from my side.
feel free to carry the rv-b on patches changing to new name.
>
> >
> > Even if on VLV/CHV if it was also voltage related and not
> > frequency related. It is strange for my brain to read
> > voltage 1,2,3... my poor braing automatically reads 1V, 2V, 3V!
> > So level would be better imho.
> >
> > But it is up to you.
> >
> > >
> > > > };
> > > >
> > > > struct drm_i915_private {
> > > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> > > > index 4bffd31a8924..522222f8bb50 100644
> > > > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > > > @@ -1690,17 +1690,34 @@ void cnl_uninit_cdclk(struct drm_i915_private *dev_priv)
> > > > }
> > > >
> > > > /**
> > > > - * intel_cdclk_state_compare - Determine if two CDCLK states differ
> > > > + * intel_cdclk_needs_modeset - Determine if two CDCLK states require a modeset on all pipes
> > > > * @a: first CDCLK state
> > > > * @b: second CDCLK state
> > > > *
> > > > * Returns:
> > > > - * True if the CDCLK states are identical, false if they differ.
> > > > + * True if the CDCLK states require pipes to be off during reprogramming, false if not.
> > > > */
> > > > -bool intel_cdclk_state_compare(const struct intel_cdclk_state *a,
> > > > +bool intel_cdclk_needs_modeset(const struct intel_cdclk_state *a,
> > > > const struct intel_cdclk_state *b)
> > > > {
> > > > - return memcmp(a, b, sizeof(*a)) == 0;
> > > > + return a->cdclk != b->cdclk ||
> > > > + a->vco != b->vco ||
> > > > + a->ref != b->ref;
> > > > +}
> > > > +
> > > > +/**
> > > > + * intel_cdclk_changed - Determine if two CDCLK states are different
> > > > + * @a: first CDCLK state
> > > > + * @b: second CDCLK state
> > > > + *
> > > > + * Returns:
> > > > + * True if the CDCLK states don't match, false if they do.
> > > > + */
> > > > +bool intel_cdclk_changed(const struct intel_cdclk_state *a,
> > > > + const struct intel_cdclk_state *b)
> > > > +{
> > > > + return intel_cdclk_needs_modeset(a, b) ||
> > > > + a->voltage != b->voltage;
> > > > }
> > > >
> > > > /**
> > > > @@ -1714,15 +1731,15 @@ bool intel_cdclk_state_compare(const struct intel_cdclk_state *a,
> > > > void intel_set_cdclk(struct drm_i915_private *dev_priv,
> > > > const struct intel_cdclk_state *cdclk_state)
> > > > {
> > > > - if (intel_cdclk_state_compare(&dev_priv->cdclk.hw, cdclk_state))
> > > > + if (!intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_state))
> > > > return;
> > > >
> > > > if (WARN_ON_ONCE(!dev_priv->display.set_cdclk))
> > > > return;
> > > >
> > > > - DRM_DEBUG_DRIVER("Changing CDCLK to %d kHz, VCO %d kHz, ref %d kHz\n",
> > > > + DRM_DEBUG_DRIVER("Changing CDCLK to %d kHz, VCO %d kHz, ref %d kHz, voltage %d\n",
> > > > cdclk_state->cdclk, cdclk_state->vco,
> > > > - cdclk_state->ref);
> > > > + cdclk_state->ref, cdclk_state->voltage);
> > > >
> > > > dev_priv->display.set_cdclk(dev_priv, cdclk_state);
> > > > }
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index bd62c0a65bcd..32e7cca52da2 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -11946,16 +11946,16 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
> > > > * holding all the crtc locks, even if we don't end up
> > > > * touching the hardware
> > > > */
> > > > - if (!intel_cdclk_state_compare(&dev_priv->cdclk.logical,
> > > > - &intel_state->cdclk.logical)) {
> > > > + if (intel_cdclk_changed(&dev_priv->cdclk.logical,
> > > > + &intel_state->cdclk.logical)) {
> > > > ret = intel_lock_all_pipes(state);
> > > > if (ret < 0)
> > > > return ret;
> > > > }
> > > >
> > > > /* All pipes must be switched off while we change the cdclk. */
> > > > - if (!intel_cdclk_state_compare(&dev_priv->cdclk.actual,
> > > > - &intel_state->cdclk.actual)) {
> > > > + if (intel_cdclk_needs_modeset(&dev_priv->cdclk.actual,
> > > > + &intel_state->cdclk.actual)) {
> > > > ret = intel_modeset_all_pipes(state);
> > > > if (ret < 0)
> > > > return ret;
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > index a05ab3a1ab27..765a737700c5 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -1324,8 +1324,10 @@ void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv);
> > > > void intel_update_max_cdclk(struct drm_i915_private *dev_priv);
> > > > void intel_update_cdclk(struct drm_i915_private *dev_priv);
> > > > void intel_update_rawclk(struct drm_i915_private *dev_priv);
> > > > -bool intel_cdclk_state_compare(const struct intel_cdclk_state *a,
> > > > +bool intel_cdclk_needs_modeset(const struct intel_cdclk_state *a,
> > > > const struct intel_cdclk_state *b);
> > > > +bool intel_cdclk_changed(const struct intel_cdclk_state *a,
> > > > + const struct intel_cdclk_state *b);
> > > > void intel_set_cdclk(struct drm_i915_private *dev_priv,
> > > > const struct intel_cdclk_state *cdclk_state);
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > index 8af286c63d3b..5ac553fab7c7 100644
> > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > @@ -705,7 +705,8 @@ static void gen9_dc_off_power_well_enable(struct drm_i915_private *dev_priv,
> > > > gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> > > >
> > > > dev_priv->display.get_cdclk(dev_priv, &cdclk_state);
> > > > - WARN_ON(!intel_cdclk_state_compare(&dev_priv->cdclk.hw, &cdclk_state));
> > > > + /* Can't read out the voltage so can't use intel_cdclk_changed() */
> > > > + WARN_ON(intel_cdclk_needs_modeset(&dev_priv->cdclk.hw, &cdclk_state));
> > > >
> > > > gen9_assert_dbuf_enabled(dev_priv);
> > > >
> > > > --
> > > > 2.13.6
> > >
> > > --
> > > Ville Syrjälä
> > > Intel OTC
>
> --
> Ville Syrjälä
> Intel OTC
More information about the Intel-gfx
mailing list