Re: [PATCH 2/8] drm/i915: Start tracking voltage level in the cdclk state

Rodrigo Vivi
2017-10-19T23:32:54Z

On Wed, Oct 18, 2017 at 08:48:19PM +0000, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@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@intel.com>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@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;
> };
>
> 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