Re: [PATCH v2 8/8] drm/i915: Adjust system agent voltage on CNL if required by DDI ports

Ville Syrjälä
2017-10-20T16:52:48Z

On Fri, Oct 20, 2017 at 07:09:45PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> On CNL we may need to bump up the system agent voltage not only due
> to CDCLK but also when driving DDI port with a sufficiently high clock.
> To that end start tracking the minimum acceptable voltage for each crtc.
> We do the tracking via crtcs because we don't have any kind of encoder
> state. Also there's no downside to doing it this way, and it matches how
> we track cdclk requirements on account of pixel rate.
>
> v2: Allow disabled crtcs to use the min voltage
> Add IS_CNL check to intel_ddi_compute_min_voltage() since
> we're using CNL specific values there
> s/intel_compute_min_voltage/cnl_compute_min_voltage/ since
> the function makes hw specific assumptions about the voltage
> values
>
> 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>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 2 ++
> drivers/gpu/drm/i915/intel_cdclk.c | 50 ++++++++++++++++++++++++++++++++++--
> drivers/gpu/drm/i915/intel_ddi.c | 11 ++++++++
> drivers/gpu/drm/i915/intel_display.c | 9 +++++++
> drivers/gpu/drm/i915/intel_dp_mst.c | 5 ++++
> drivers/gpu/drm/i915/intel_drv.h | 6 +++++
> 6 files changed, 81 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index d3ac58dc275f..185711a852b0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2415,6 +2415,8 @@ struct drm_i915_private {
> unsigned int active_crtcs;
> /* minimum acceptable cdclk for each pipe */
> int min_cdclk[I915_MAX_PIPES];
> + /* minimum acceptable voltage for each pipe */
> + u8 min_voltage[I915_MAX_PIPES];
>
> int dpio_phy_iosf_port[I915_NUM_PHYS_VLV];
>
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index 5bbf9f6023a2..78029f70e6ea 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1669,6 +1669,12 @@ static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
> mutex_unlock(&dev_priv->pcu_lock);
>
> intel_update_cdclk(dev_priv);
> +
> + /*
> + * Can't read out the voltage :( So let's
> + * just assume everything is as expected.
> + */
> + dev_priv->cdclk.hw.voltage = cdclk_state->voltage;
> }
>
> static int cnl_cdclk_pll_vco(struct drm_i915_private *dev_priv, int cdclk)
> @@ -1938,6 +1944,42 @@ static int intel_compute_min_cdclk(struct drm_atomic_state *state)
> return min_cdclk;
> }
>
> +/*
> + * Note that this functions assumes that 0 is
> + * the lowest voltage value, and higher values
> + * correspond to increasingly higher voltages.
> + *
> + * Should that relationship no longer hold on
> + * future platforms this code will need to be
> + * adjusted.
> + */
> +static u8 cnl_compute_min_voltage(struct intel_atomic_state *state)
> +{
> + struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> + struct intel_crtc *crtc;
> + struct intel_crtc_state *crtc_state;
> + u8 min_voltage;
> + int i;
> + enum pipe pipe;
> +
> + memcpy(state->min_voltage, dev_priv->min_voltage,
> + sizeof(state->min_voltage));
> +
> + for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
> + if (crtc_state->base.enable)
> + state->min_voltage[i] = crtc_state->min_voltage;
> + else
> + state->min_voltage[i] = 0;
> + }
> +
> + min_voltage = 0;
> + for_each_pipe(dev_priv, pipe)
> + min_voltage = max(state->min_voltage[pipe],
> + min_voltage);
> +
> + return min_voltage;
> +}
> +
> static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
> {
> struct drm_i915_private *dev_priv = to_i915(state->dev);
> @@ -2021,7 +2063,9 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
>
> intel_state->cdclk.logical.vco = vco;
> intel_state->cdclk.logical.cdclk = cdclk;
> - intel_state->cdclk.logical.voltage = skl_calc_voltage(cdclk);
> + intel_state->cdclk.logical.voltage =
> + max(skl_calc_voltage(cdclk),
> + cnl_compute_min_voltage(intel_state));

And of course this hunk shouldn't be here. It was part of my test hack.
v3 coming up.

>
> if (!intel_state->active_crtcs) {
> cdclk = skl_calc_cdclk(0, vco);

--
Ville Syrjälä
Intel OTC