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@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Cc: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > 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.
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
