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.syrj...@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.kah...@intel.com>
> > > Cc: Manasi Navare <manasi.d.nav...@intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.v...@intel.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrj...@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"?
>
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
