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

Commit Message

Ville Syrjälä Oct. 18, 2017, 8:48 p.m. UTC

From: Ville Syrjälä <[e-mail]>

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.

Cc: Mika Kahola <[e-mail]>
Cc: Manasi Navare <[e-mail]>
Cc: Rodrigo Vivi <[e-mail]>
Signed-off-by: Ville Syrjälä <[e-mail]>
---
 drivers/gpu/drm/i915/i915_drv.h      |  2 ++
 drivers/gpu/drm/i915/intel_cdclk.c   | 34 +++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_ddi.c     | 13 +++++++++++++
 drivers/gpu/drm/i915/intel_display.c |  6 ++++++
 drivers/gpu/drm/i915/intel_dp_mst.c  |  5 +++++
 drivers/gpu/drm/i915/intel_drv.h     |  6 ++++++
 6 files changed, 65 insertions(+), 1 deletion(-)

Comments

Rodrigo Vivi Oct. 19, 2017, 11:54 p.m. UTC | #1

On Wed, Oct 18, 2017 at 08:48:25PM +0000, Ville Syrjala wrote:
> From: Ville Syrjälä <[e-mail]>
> 
> 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.
> 
> Cc: Mika Kahola <[e-mail]>
> Cc: Manasi Navare <[e-mail]>
> Cc: Rodrigo Vivi <[e-mail]>
> Signed-off-by: Ville Syrjälä <[e-mail]>


Reviewed-by: Rodrigo Vivi <[e-mail]>

Tested-by: Rodrigo Vivi <[e-mail]>
(Although I didn't find cases where I could force a higher voltage level,
everything works well on CNL with this.)


> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  2 ++
>  drivers/gpu/drm/i915/intel_cdclk.c   | 34 +++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_ddi.c     | 13 +++++++++++++
>  drivers/gpu/drm/i915/intel_display.c |  6 ++++++
>  drivers/gpu/drm/i915/intel_dp_mst.c  |  5 +++++
>  drivers/gpu/drm/i915/intel_drv.h     |  6 ++++++
>  6 files changed, 65 insertions(+), 1 deletion(-)
> 
> 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 795a18f64c4c..035c44858908 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1666,6 +1666,13 @@ 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 this out :( Can't rely on .get_cdclk() since it
> +	 * doesn't know about DDI voltage requirements. 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)
> @@ -1935,6 +1942,29 @@ static int intel_compute_min_cdclk(struct drm_atomic_state *state)
>  	return min_cdclk;
>  }
>  
> +static u8 intel_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)
> +		state->min_voltage[i] = crtc_state->min_voltage;
> +
> +	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);
> @@ -2091,7 +2121,9 @@ static int cnl_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 = cnl_calc_voltage(cdclk);
> +	intel_state->cdclk.logical.voltage =
> +		max(cnl_calc_voltage(cdclk),
> +		    intel_compute_min_voltage(intel_state));
>  
>  	if (!intel_state->active_crtcs) {
>  		cdclk = cnl_calc_cdclk(0);
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index adf51b328844..f02e3c2f4ad2 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2525,6 +2525,15 @@ bool intel_ddi_is_audio_enabled(struct drm_i915_private *dev_priv,
>  	return false;
>  }
>  
> +void intel_ddi_compute_min_voltage(struct drm_i915_private *dev_priv,
> +				   struct intel_crtc_state *crtc_state)
> +{
> +	if (INTEL_GEN(dev_priv) >= 10 && crtc_state->port_clock > 594000)
> +		crtc_state->min_voltage = 2;
> +	else
> +		crtc_state->min_voltage = 0;
> +}
> +
>  void intel_ddi_get_config(struct intel_encoder *encoder,
>  			  struct intel_crtc_state *pipe_config)
>  {
> @@ -2624,6 +2633,8 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
>  	if (IS_GEN9_LP(dev_priv))
>  		pipe_config->lane_lat_optim_mask =
>  			bxt_ddi_phy_get_lane_lat_optim_mask(encoder);
> +
> +	intel_ddi_compute_min_voltage(dev_priv, pipe_config);
>  }
>  
>  static bool intel_ddi_compute_config(struct intel_encoder *encoder,
> @@ -2650,6 +2661,8 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
>  			bxt_ddi_phy_calc_lane_lat_optim_mask(encoder,
>  							     pipe_config->lane_count);
>  
> +	intel_ddi_compute_min_voltage(dev_priv, pipe_config);
> +
>  	return ret;
>  
>  }
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 32e7cca52da2..7d293ecb51c4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5938,6 +5938,7 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc,
>  
>  	dev_priv->active_crtcs &= ~(1 << intel_crtc->pipe);
>  	dev_priv->min_cdclk[intel_crtc->pipe] = 0;
> +	dev_priv->min_voltage[intel_crtc->pipe] = 0;
>  }
>  
>  /*
> @@ -11304,6 +11305,8 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
>  	PIPE_CONF_CHECK_CLOCK_FUZZY(base.adjusted_mode.crtc_clock);
>  	PIPE_CONF_CHECK_CLOCK_FUZZY(port_clock);
>  
> +	PIPE_CONF_CHECK_I(min_voltage);
> +
>  #undef PIPE_CONF_CHECK_X
>  #undef PIPE_CONF_CHECK_I
>  #undef PIPE_CONF_CHECK_P
> @@ -12532,6 +12535,8 @@ static int intel_atomic_commit(struct drm_device *dev,
>  	if (intel_state->modeset) {
>  		memcpy(dev_priv->min_cdclk, intel_state->min_cdclk,
>  		       sizeof(intel_state->min_cdclk));
> +		memcpy(dev_priv->min_voltage, intel_state->min_voltage,
> +		       sizeof(intel_state->min_voltage));
>  		dev_priv->active_crtcs = intel_state->active_crtcs;
>  		dev_priv->cdclk.logical = intel_state->cdclk.logical;
>  		dev_priv->cdclk.actual = intel_state->cdclk.actual;
> @@ -15042,6 +15047,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  		}
>  
>  		dev_priv->min_cdclk[crtc->pipe] = min_cdclk;
> +		dev_priv->min_voltage[crtc->pipe] = crtc_state->min_voltage;
>  
>  		intel_pipe_config_sanity_check(dev_priv, crtc_state);
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 772521440a9f..6c1da88b5fef 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -34,6 +34,7 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  					struct intel_crtc_state *pipe_config,
>  					struct drm_connector_state *conn_state)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder->base);
>  	struct intel_digital_port *intel_dig_port = intel_mst->primary;
>  	struct intel_dp *intel_dp = &intel_dig_port->dp;
> @@ -87,6 +88,8 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  
>  	pipe_config->dp_m_n.tu = slots;
>  
> +	intel_ddi_compute_min_voltage(dev_priv, pipe_config);
> +
>  	return true;
>  }
>  
> @@ -307,6 +310,8 @@ static void intel_dp_mst_enc_get_config(struct intel_encoder *encoder,
>  	intel_dp_get_m_n(crtc, pipe_config);
>  
>  	intel_ddi_clock_get(&intel_dig_port->base, pipe_config);
> +
> +	intel_ddi_compute_min_voltage(dev_priv, pipe_config);
>  }
>  
>  static int intel_dp_mst_get_ddc_modes(struct drm_connector *connector)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 765a737700c5..002894b13d69 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -386,6 +386,8 @@ struct intel_atomic_state {
>  	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];
>  
>  	struct intel_shared_dpll_state shared_dpll[I915_NUM_PLLS];
>  
> @@ -739,6 +741,8 @@ struct intel_crtc_state {
>  	 */
>  	uint8_t lane_lat_optim_mask;
>  
> +	u8 min_voltage;
> +
>  	/* Panel fitter controls for gen2-gen4 + VLV */
>  	struct {
>  		u32 control;
> @@ -1294,6 +1298,8 @@ void intel_ddi_clock_get(struct intel_encoder *encoder,
>  			 struct intel_crtc_state *pipe_config);
>  void intel_ddi_set_vc_payload_alloc(const struct intel_crtc_state *crtc_state,
>  				    bool state);
> +void intel_ddi_compute_min_voltage(struct drm_i915_private *dev_priv,
> +				   struct intel_crtc_state *crtc_state);
>  u32 bxt_signal_levels(struct intel_dp *intel_dp);
>  uint32_t ddi_signal_levels(struct intel_dp *intel_dp);
>  u8 intel_ddi_dp_voltage_max(struct intel_encoder *encoder);
> -- 
> 2.13.6
>

Ville Syrjälä Oct. 20, 2017, 11:11 a.m. UTC | #2

On Thu, Oct 19, 2017 at 04:54:56PM -0700, Rodrigo Vivi wrote:
> 
> On Wed, Oct 18, 2017 at 08:48:25PM +0000, Ville Syrjala wrote:
> > From: Ville Syrjälä <[e-mail]>
> > 
> > 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.
> > 
> > Cc: Mika Kahola <[e-mail]>
> > Cc: Manasi Navare <[e-mail]>
> > Cc: Rodrigo Vivi <[e-mail]>
> > Signed-off-by: Ville Syrjälä <[e-mail]>
> 
> 
> Reviewed-by: Rodrigo Vivi <[e-mail]>
> 
> Tested-by: Rodrigo Vivi <[e-mail]>
> (Although I didn't find cases where I could force a higher voltage level,
> everything works well on CNL with this.)

I went and actually read the spec now :) so I can now see that this
doesn't really match the sequence listed in the spec.

For example, let's assume we have a modeset where we have to disable
a DPLL, change CDCLK, and finally enable a DPLL again.

What the spec says we should do is something like this:
1.  DVFS pre sequence
2.  disable DPLL
3.  DVFS post sequence
4.  disable DPLL power
...
5.  DVFS pre sequence
6.  configure CDCLK_CTL
7.  DVFS post sequence
...
8.  enable DPLL power
9.  DVFS pre sequence
10. enable DPLL
11. DVFS post sequence

With my code what we'd end up doing is this instead:
1. disable DPLL
2. disable DPLL power
...
3. DVFS pre sequence
4. configure CDCLK_CTL
5. DVFS post sequence
...
6. enable DPLL power
7. enable DPLL

So my way always results in running the DVFS sequences at most once.
With the way the spec has things listed we might have to run the
sequences multiple times. 

Art, is there a good reason why we'd actually have to run the
DVFS sequences around each DPLL_ENABLE write, instead of just
doing it once at any point between disabling and enabling
DPLLs?

Ville Syrjälä Oct. 20, 2017, 2:18 p.m. UTC | #3

On Thu, Oct 19, 2017 at 04:54:56PM -0700, Rodrigo Vivi wrote:
> 
> On Wed, Oct 18, 2017 at 08:48:25PM +0000, Ville Syrjala wrote:
> > From: Ville Syrjälä <[e-mail]>
> > 
> > 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.
> > 
> > Cc: Mika Kahola <[e-mail]>
> > Cc: Manasi Navare <[e-mail]>
> > Cc: Rodrigo Vivi <[e-mail]>
> > Signed-off-by: Ville Syrjälä <[e-mail]>
> 
> 
> Reviewed-by: Rodrigo Vivi <[e-mail]>
> 
> Tested-by: Rodrigo Vivi <[e-mail]>
> (Although I didn't find cases where I could force a higher voltage level,
> everything works well on CNL with this.)

For a quick test hack you could just reduce the port clock limit in
intel_ddi_compute_min_voltage() to something you can reach.

So probably something like:
1. enable one pipe with low port clock that can operate at min voltage
2. enable a second pipe with higher port clock that need max voltage
3. stare at the logs to make sure we ramp up the voltage correctly,
   and that the first pipe didn't undergo a modeset needlessly

Oh, and I guess you'd have to make sure that both pipes can operate at
the same low cdclk value so that cdclk changes won't interfere with
your observations.

Actually this kind of hack could be tested on any platform, assuming
they will tolerate voltage changes with pipes on. I guess I can give
it a quick whirl on my SKL here and see if the smoke escapes ;)

> 
> 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |  2 ++
> >  drivers/gpu/drm/i915/intel_cdclk.c   | 34 +++++++++++++++++++++++++++++++++-
> >  drivers/gpu/drm/i915/intel_ddi.c     | 13 +++++++++++++
> >  drivers/gpu/drm/i915/intel_display.c |  6 ++++++
> >  drivers/gpu/drm/i915/intel_dp_mst.c  |  5 +++++
> >  drivers/gpu/drm/i915/intel_drv.h     |  6 ++++++
> >  6 files changed, 65 insertions(+), 1 deletion(-)
> > 
> > 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 795a18f64c4c..035c44858908 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -1666,6 +1666,13 @@ 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 this out :( Can't rely on .get_cdclk() since it
> > +	 * doesn't know about DDI voltage requirements. 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)
> > @@ -1935,6 +1942,29 @@ static int intel_compute_min_cdclk(struct drm_atomic_state *state)
> >  	return min_cdclk;
> >  }
> >  
> > +static u8 intel_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)
> > +		state->min_voltage[i] = crtc_state->min_voltage;
> > +
> > +	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);
> > @@ -2091,7 +2121,9 @@ static int cnl_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 = cnl_calc_voltage(cdclk);
> > +	intel_state->cdclk.logical.voltage =
> > +		max(cnl_calc_voltage(cdclk),
> > +		    intel_compute_min_voltage(intel_state));
> >  
> >  	if (!intel_state->active_crtcs) {
> >  		cdclk = cnl_calc_cdclk(0);
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index adf51b328844..f02e3c2f4ad2 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -2525,6 +2525,15 @@ bool intel_ddi_is_audio_enabled(struct drm_i915_private *dev_priv,
> >  	return false;
> >  }
> >  
> > +void intel_ddi_compute_min_voltage(struct drm_i915_private *dev_priv,
> > +				   struct intel_crtc_state *crtc_state)
> > +{
> > +	if (INTEL_GEN(dev_priv) >= 10 && crtc_state->port_clock > 594000)
> > +		crtc_state->min_voltage = 2;
> > +	else
> > +		crtc_state->min_voltage = 0;
> > +}
> > +
> >  void intel_ddi_get_config(struct intel_encoder *encoder,
> >  			  struct intel_crtc_state *pipe_config)
> >  {
> > @@ -2624,6 +2633,8 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
> >  	if (IS_GEN9_LP(dev_priv))
> >  		pipe_config->lane_lat_optim_mask =
> >  			bxt_ddi_phy_get_lane_lat_optim_mask(encoder);
> > +
> > +	intel_ddi_compute_min_voltage(dev_priv, pipe_config);
> >  }
> >  
> >  static bool intel_ddi_compute_config(struct intel_encoder *encoder,
> > @@ -2650,6 +2661,8 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
> >  			bxt_ddi_phy_calc_lane_lat_optim_mask(encoder,
> >  							     pipe_config->lane_count);
> >  
> > +	intel_ddi_compute_min_voltage(dev_priv, pipe_config);
> > +
> >  	return ret;
> >  
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 32e7cca52da2..7d293ecb51c4 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5938,6 +5938,7 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc,
> >  
> >  	dev_priv->active_crtcs &= ~(1 << intel_crtc->pipe);
> >  	dev_priv->min_cdclk[intel_crtc->pipe] = 0;
> > +	dev_priv->min_voltage[intel_crtc->pipe] = 0;
> >  }
> >  
> >  /*
> > @@ -11304,6 +11305,8 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
> >  	PIPE_CONF_CHECK_CLOCK_FUZZY(base.adjusted_mode.crtc_clock);
> >  	PIPE_CONF_CHECK_CLOCK_FUZZY(port_clock);
> >  
> > +	PIPE_CONF_CHECK_I(min_voltage);
> > +
> >  #undef PIPE_CONF_CHECK_X
> >  #undef PIPE_CONF_CHECK_I
> >  #undef PIPE_CONF_CHECK_P
> > @@ -12532,6 +12535,8 @@ static int intel_atomic_commit(struct drm_device *dev,
> >  	if (intel_state->modeset) {
> >  		memcpy(dev_priv->min_cdclk, intel_state->min_cdclk,
> >  		       sizeof(intel_state->min_cdclk));
> > +		memcpy(dev_priv->min_voltage, intel_state->min_voltage,
> > +		       sizeof(intel_state->min_voltage));
> >  		dev_priv->active_crtcs = intel_state->active_crtcs;
> >  		dev_priv->cdclk.logical = intel_state->cdclk.logical;
> >  		dev_priv->cdclk.actual = intel_state->cdclk.actual;
> > @@ -15042,6 +15047,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> >  		}
> >  
> >  		dev_priv->min_cdclk[crtc->pipe] = min_cdclk;
> > +		dev_priv->min_voltage[crtc->pipe] = crtc_state->min_voltage;
> >  
> >  		intel_pipe_config_sanity_check(dev_priv, crtc_state);
> >  	}
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index 772521440a9f..6c1da88b5fef 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -34,6 +34,7 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
> >  					struct intel_crtc_state *pipe_config,
> >  					struct drm_connector_state *conn_state)
> >  {
> > +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >  	struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder->base);
> >  	struct intel_digital_port *intel_dig_port = intel_mst->primary;
> >  	struct intel_dp *intel_dp = &intel_dig_port->dp;
> > @@ -87,6 +88,8 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
> >  
> >  	pipe_config->dp_m_n.tu = slots;
> >  
> > +	intel_ddi_compute_min_voltage(dev_priv, pipe_config);
> > +
> >  	return true;
> >  }
> >  
> > @@ -307,6 +310,8 @@ static void intel_dp_mst_enc_get_config(struct intel_encoder *encoder,
> >  	intel_dp_get_m_n(crtc, pipe_config);
> >  
> >  	intel_ddi_clock_get(&intel_dig_port->base, pipe_config);
> > +
> > +	intel_ddi_compute_min_voltage(dev_priv, pipe_config);
> >  }
> >  
> >  static int intel_dp_mst_get_ddc_modes(struct drm_connector *connector)
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 765a737700c5..002894b13d69 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -386,6 +386,8 @@ struct intel_atomic_state {
> >  	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];
> >  
> >  	struct intel_shared_dpll_state shared_dpll[I915_NUM_PLLS];
> >  
> > @@ -739,6 +741,8 @@ struct intel_crtc_state {
> >  	 */
> >  	uint8_t lane_lat_optim_mask;
> >  
> > +	u8 min_voltage;
> > +
> >  	/* Panel fitter controls for gen2-gen4 + VLV */
> >  	struct {
> >  		u32 control;
> > @@ -1294,6 +1298,8 @@ void intel_ddi_clock_get(struct intel_encoder *encoder,
> >  			 struct intel_crtc_state *pipe_config);
> >  void intel_ddi_set_vc_payload_alloc(const struct intel_crtc_state *crtc_state,
> >  				    bool state);
> > +void intel_ddi_compute_min_voltage(struct drm_i915_private *dev_priv,
> > +				   struct intel_crtc_state *crtc_state);
> >  u32 bxt_signal_levels(struct intel_dp *intel_dp);
> >  uint32_t ddi_signal_levels(struct intel_dp *intel_dp);
> >  u8 intel_ddi_dp_voltage_max(struct intel_encoder *encoder);
> > -- 
> > 2.13.6
> >

Ville Syrjälä Oct. 20, 2017, 4:11 p.m. UTC | #4

On Fri, Oct 20, 2017 at 05:18:04PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 19, 2017 at 04:54:56PM -0700, Rodrigo Vivi wrote:
> > 
> > On Wed, Oct 18, 2017 at 08:48:25PM +0000, Ville Syrjala wrote:
> > > From: Ville Syrjälä <[e-mail]>
> > > 
> > > 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.
> > > 
> > > Cc: Mika Kahola <[e-mail]>
> > > Cc: Manasi Navare <[e-mail]>
> > > Cc: Rodrigo Vivi <[e-mail]>
> > > Signed-off-by: Ville Syrjälä <[e-mail]>
> > 
> > 
> > Reviewed-by: Rodrigo Vivi <[e-mail]>
> > 
> > Tested-by: Rodrigo Vivi <[e-mail]>
> > (Although I didn't find cases where I could force a higher voltage level,
> > everything works well on CNL with this.)
> 
> For a quick test hack you could just reduce the port clock limit in
> intel_ddi_compute_min_voltage() to something you can reach.
> 
> So probably something like:
> 1. enable one pipe with low port clock that can operate at min voltage
> 2. enable a second pipe with higher port clock that need max voltage
> 3. stare at the logs to make sure we ramp up the voltage correctly,
>    and that the first pipe didn't undergo a modeset needlessly
> 
> Oh, and I guess you'd have to make sure that both pipes can operate at
> the same low cdclk value so that cdclk changes won't interfere with
> your observations.
> 
> Actually this kind of hack could be tested on any platform, assuming
> they will tolerate voltage changes with pipes on. I guess I can give
> it a quick whirl on my SKL here and see if the smoke escapes ;)

OK, quick test here found some bugs. I sent a v2 which seems to do the
right thing on my SKL with the aforementioned hacks in place.

> 
> > 
> > 
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h      |  2 ++
> > >  drivers/gpu/drm/i915/intel_cdclk.c   | 34 +++++++++++++++++++++++++++++++++-
> > >  drivers/gpu/drm/i915/intel_ddi.c     | 13 +++++++++++++
> > >  drivers/gpu/drm/i915/intel_display.c |  6 ++++++
> > >  drivers/gpu/drm/i915/intel_dp_mst.c  |  5 +++++
> > >  drivers/gpu/drm/i915/intel_drv.h     |  6 ++++++
> > >  6 files changed, 65 insertions(+), 1 deletion(-)
> > > 
> > > 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 795a18f64c4c..035c44858908 100644
> > > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > > @@ -1666,6 +1666,13 @@ 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 this out :( Can't rely on .get_cdclk() since it
> > > +	 * doesn't know about DDI voltage requirements. 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)
> > > @@ -1935,6 +1942,29 @@ static int intel_compute_min_cdclk(struct drm_atomic_state *state)
> > >  	return min_cdclk;
> > >  }
> > >  
> > > +static u8 intel_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)
> > > +		state->min_voltage[i] = crtc_state->min_voltage;
> > > +
> > > +	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);
> > > @@ -2091,7 +2121,9 @@ static int cnl_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 = cnl_calc_voltage(cdclk);
> > > +	intel_state->cdclk.logical.voltage =
> > > +		max(cnl_calc_voltage(cdclk),
> > > +		    intel_compute_min_voltage(intel_state));
> > >  
> > >  	if (!intel_state->active_crtcs) {
> > >  		cdclk = cnl_calc_cdclk(0);
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > index adf51b328844..f02e3c2f4ad2 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -2525,6 +2525,15 @@ bool intel_ddi_is_audio_enabled(struct drm_i915_private *dev_priv,
> > >  	return false;
> > >  }
> > >  
> > > +void intel_ddi_compute_min_voltage(struct drm_i915_private *dev_priv,
> > > +				   struct intel_crtc_state *crtc_state)
> > > +{
> > > +	if (INTEL_GEN(dev_priv) >= 10 && crtc_state->port_clock > 594000)
> > > +		crtc_state->min_voltage = 2;
> > > +	else
> > > +		crtc_state->min_voltage = 0;
> > > +}
> > > +
> > >  void intel_ddi_get_config(struct intel_encoder *encoder,
> > >  			  struct intel_crtc_state *pipe_config)
> > >  {
> > > @@ -2624,6 +2633,8 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
> > >  	if (IS_GEN9_LP(dev_priv))
> > >  		pipe_config->lane_lat_optim_mask =
> > >  			bxt_ddi_phy_get_lane_lat_optim_mask(encoder);
> > > +
> > > +	intel_ddi_compute_min_voltage(dev_priv, pipe_config);
> > >  }
> > >  
> > >  static bool intel_ddi_compute_config(struct intel_encoder *encoder,
> > > @@ -2650,6 +2661,8 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
> > >  			bxt_ddi_phy_calc_lane_lat_optim_mask(encoder,
> > >  							     pipe_config->lane_count);
> > >  
> > > +	intel_ddi_compute_min_voltage(dev_priv, pipe_config);
> > > +
> > >  	return ret;
> > >  
> > >  }
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 32e7cca52da2..7d293ecb51c4 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -5938,6 +5938,7 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc,
> > >  
> > >  	dev_priv->active_crtcs &= ~(1 << intel_crtc->pipe);
> > >  	dev_priv->min_cdclk[intel_crtc->pipe] = 0;
> > > +	dev_priv->min_voltage[intel_crtc->pipe] = 0;
> > >  }
> > >  
> > >  /*
> > > @@ -11304,6 +11305,8 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
> > >  	PIPE_CONF_CHECK_CLOCK_FUZZY(base.adjusted_mode.crtc_clock);
> > >  	PIPE_CONF_CHECK_CLOCK_FUZZY(port_clock);
> > >  
> > > +	PIPE_CONF_CHECK_I(min_voltage);
> > > +
> > >  #undef PIPE_CONF_CHECK_X
> > >  #undef PIPE_CONF_CHECK_I
> > >  #undef PIPE_CONF_CHECK_P
> > > @@ -12532,6 +12535,8 @@ static int intel_atomic_commit(struct drm_device *dev,
> > >  	if (intel_state->modeset) {
> > >  		memcpy(dev_priv->min_cdclk, intel_state->min_cdclk,
> > >  		       sizeof(intel_state->min_cdclk));
> > > +		memcpy(dev_priv->min_voltage, intel_state->min_voltage,
> > > +		       sizeof(intel_state->min_voltage));
> > >  		dev_priv->active_crtcs = intel_state->active_crtcs;
> > >  		dev_priv->cdclk.logical = intel_state->cdclk.logical;
> > >  		dev_priv->cdclk.actual = intel_state->cdclk.actual;
> > > @@ -15042,6 +15047,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> > >  		}
> > >  
> > >  		dev_priv->min_cdclk[crtc->pipe] = min_cdclk;
> > > +		dev_priv->min_voltage[crtc->pipe] = crtc_state->min_voltage;
> > >  
> > >  		intel_pipe_config_sanity_check(dev_priv, crtc_state);
> > >  	}
> > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > index 772521440a9f..6c1da88b5fef 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > @@ -34,6 +34,7 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
> > >  					struct intel_crtc_state *pipe_config,
> > >  					struct drm_connector_state *conn_state)
> > >  {
> > > +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > >  	struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder->base);
> > >  	struct intel_digital_port *intel_dig_port = intel_mst->primary;
> > >  	struct intel_dp *intel_dp = &intel_dig_port->dp;
> > > @@ -87,6 +88,8 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
> > >  
> > >  	pipe_config->dp_m_n.tu = slots;
> > >  
> > > +	intel_ddi_compute_min_voltage(dev_priv, pipe_config);
> > > +
> > >  	return true;
> > >  }
> > >  
> > > @@ -307,6 +310,8 @@ static void intel_dp_mst_enc_get_config(struct intel_encoder *encoder,
> > >  	intel_dp_get_m_n(crtc, pipe_config);
> > >  
> > >  	intel_ddi_clock_get(&intel_dig_port->base, pipe_config);
> > > +
> > > +	intel_ddi_compute_min_voltage(dev_priv, pipe_config);
> > >  }
> > >  
> > >  static int intel_dp_mst_get_ddc_modes(struct drm_connector *connector)
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index 765a737700c5..002894b13d69 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -386,6 +386,8 @@ struct intel_atomic_state {
> > >  	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];
> > >  
> > >  	struct intel_shared_dpll_state shared_dpll[I915_NUM_PLLS];
> > >  
> > > @@ -739,6 +741,8 @@ struct intel_crtc_state {
> > >  	 */
> > >  	uint8_t lane_lat_optim_mask;
> > >  
> > > +	u8 min_voltage;
> > > +
> > >  	/* Panel fitter controls for gen2-gen4 + VLV */
> > >  	struct {
> > >  		u32 control;
> > > @@ -1294,6 +1298,8 @@ void intel_ddi_clock_get(struct intel_encoder *encoder,
> > >  			 struct intel_crtc_state *pipe_config);
> > >  void intel_ddi_set_vc_payload_alloc(const struct intel_crtc_state *crtc_state,
> > >  				    bool state);
> > > +void intel_ddi_compute_min_voltage(struct drm_i915_private *dev_priv,
> > > +				   struct intel_crtc_state *crtc_state);
> > >  u32 bxt_signal_levels(struct intel_dp *intel_dp);
> > >  uint32_t ddi_signal_levels(struct intel_dp *intel_dp);
> > >  u8 intel_ddi_dp_voltage_max(struct intel_encoder *encoder);
> > > -- 
> > > 2.13.6
> > > 
> 
> -- 
> Ville Syrjälä
> Intel OTC

Runyan, Arthur J Oct. 20, 2017, 5:48 p.m. UTC | #5

Sorry about top reply from corporate email...
If you know in advance that you will just be temporarily disabling the PLL, then your sequence works. 

-----Original Message-----
From: Ville Syrjälä [mailto:[e-mail]] 
Sent: Friday, 20 October, 2017 4:12 AM
To: Vivi, Rodrigo <[e-mail]>
Cc: [e-mail]; Kahola, Mika <[e-mail]>; Navare, Manasi D <[e-mail]>; Runyan, Arthur J <[e-mail]>
Subject: Re: [PATCH 8/8] drm/i915: Adjust system agent voltage on CNL if required by DDI ports

On Thu, Oct 19, 2017 at 04:54:56PM -0700, Rodrigo Vivi wrote:
> 
> On Wed, Oct 18, 2017 at 08:48:25PM +0000, Ville Syrjala wrote:
> > From: Ville Syrjälä <[e-mail]>
> > 
> > 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.
> > 
> > Cc: Mika Kahola <[e-mail]>
> > Cc: Manasi Navare <[e-mail]>
> > Cc: Rodrigo Vivi <[e-mail]>
> > Signed-off-by: Ville Syrjälä <[e-mail]>
> 
> 
> Reviewed-by: Rodrigo Vivi <[e-mail]>
> 
> Tested-by: Rodrigo Vivi <[e-mail]>
> (Although I didn't find cases where I could force a higher voltage level,
> everything works well on CNL with this.)

I went and actually read the spec now :) so I can now see that this
doesn't really match the sequence listed in the spec.

For example, let's assume we have a modeset where we have to disable
a DPLL, change CDCLK, and finally enable a DPLL again.

What the spec says we should do is something like this:
1.  DVFS pre sequence
2.  disable DPLL
3.  DVFS post sequence
4.  disable DPLL power
...
5.  DVFS pre sequence
6.  configure CDCLK_CTL
7.  DVFS post sequence
...
8.  enable DPLL power
9.  DVFS pre sequence
10. enable DPLL
11. DVFS post sequence

With my code what we'd end up doing is this instead:
1. disable DPLL
2. disable DPLL power
...
3. DVFS pre sequence
4. configure CDCLK_CTL
5. DVFS post sequence
...
6. enable DPLL power
7. enable DPLL

So my way always results in running the DVFS sequences at most once.
With the way the spec has things listed we might have to run the
sequences multiple times. 

Art, is there a good reason why we'd actually have to run the
DVFS sequences around each DPLL_ENABLE write, instead of just
doing it once at any point between disabling and enabling
DPLLs?

Ville Syrjälä Oct. 20, 2017, 8:07 p.m. UTC | #6

On Fri, Oct 20, 2017 at 05:48:54PM +0000, Runyan, Arthur J wrote:
> Sorry about top reply from corporate email...
> If you know in advance that you will just be temporarily disabling the PLL, then your sequence works. 

Actually we would end up using this sequence even if disable the PLL for
good.

Eg. if we're just disabling the entire pipe+DPLL, we'd do this (assuming
cdclk doesn't need changing, but voltage can be lowered due to the port
no longer requiring it):
1. disable DPLL
2. disable DPLL power
...
3. DVFS pre sequence
4. DVFS post sequence

And when starting up a pipe from the cold with a new DPLL we'd conversly
do this (again assuming no cdclk change, but voltage would have to be
ramped up for the port):
1. DVFS pre sequence
2. DVFS post sequence
...
3. enable DPLL power
4. enable DPLL

It does look a bit strange doing the DVFS sequences on their own like
that, but I don't see why that should be problem. From where I'm sitting
it doesn't really look any different than the following seqeunces:

Disable with cdclk changing but port clock not having any effect
on the voltage:
1. disable DPLL
2. disable DPLL power
...
3. DVFS pre sequence
4. change cdclk
5. DVFS post sequence

Enable with cdclk changing but port clock not having any effect
on the voltage:
1. DVFS pre sequence
2. change cdclk
3. DVFS post sequence
...
4. enable DPLL power
5. enable DPLL

So unless something is snooping the actual state of the DPLLs, and based
on that second guessing our choice of voltage, I don't really see how
these two cases differ at all.

> 
> -----Original Message-----
> From: Ville Syrjälä [mailto:[e-mail]] 
> Sent: Friday, 20 October, 2017 4:12 AM
> To: Vivi, Rodrigo <[e-mail]>
> Cc: [e-mail]; Kahola, Mika <[e-mail]>; Navare, Manasi D <[e-mail]>; Runyan, Arthur J <[e-mail]>
> Subject: Re: [PATCH 8/8] drm/i915: Adjust system agent voltage on CNL if required by DDI ports
> 
> On Thu, Oct 19, 2017 at 04:54:56PM -0700, Rodrigo Vivi wrote:
> > 
> > On Wed, Oct 18, 2017 at 08:48:25PM +0000, Ville Syrjala wrote:
> > > From: Ville Syrjälä <[e-mail]>
> > > 
> > > 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.
> > > 
> > > Cc: Mika Kahola <[e-mail]>
> > > Cc: Manasi Navare <[e-mail]>
> > > Cc: Rodrigo Vivi <[e-mail]>
> > > Signed-off-by: Ville Syrjälä <[e-mail]>
> > 
> > 
> > Reviewed-by: Rodrigo Vivi <[e-mail]>
> > 
> > Tested-by: Rodrigo Vivi <[e-mail]>
> > (Although I didn't find cases where I could force a higher voltage level,
> > everything works well on CNL with this.)
> 
> I went and actually read the spec now :) so I can now see that this
> doesn't really match the sequence listed in the spec.
> 
> For example, let's assume we have a modeset where we have to disable
> a DPLL, change CDCLK, and finally enable a DPLL again.
> 
> What the spec says we should do is something like this:
> 1.  DVFS pre sequence
> 2.  disable DPLL
> 3.  DVFS post sequence
> 4.  disable DPLL power
> ...
> 5.  DVFS pre sequence
> 6.  configure CDCLK_CTL
> 7.  DVFS post sequence
> ...
> 8.  enable DPLL power
> 9.  DVFS pre sequence
> 10. enable DPLL
> 11. DVFS post sequence
> 
> With my code what we'd end up doing is this instead:
> 1. disable DPLL
> 2. disable DPLL power
> ...
> 3. DVFS pre sequence
> 4. configure CDCLK_CTL
> 5. DVFS post sequence
> ...
> 6. enable DPLL power
> 7. enable DPLL
> 
> So my way always results in running the DVFS sequences at most once.
> With the way the spec has things listed we might have to run the
> sequences multiple times. 
> 
> Art, is there a good reason why we'd actually have to run the
> DVFS sequences around each DPLL_ENABLE write, instead of just
> doing it once at any point between disabling and enabling
> DPLLs?
> 
> -- 
> Ville Syrjälä
> Intel OTC

Rodrigo Vivi Oct. 20, 2017, 8:36 p.m. UTC | #7

On Fri, Oct 20, 2017 at 08:07:07PM +0000, Ville Syrjälä wrote:
> On Fri, Oct 20, 2017 at 05:48:54PM +0000, Runyan, Arthur J wrote:
> > Sorry about top reply from corporate email...
> > If you know in advance that you will just be temporarily disabling the PLL, then your sequence works. 
> 
> Actually we would end up using this sequence even if disable the PLL for
> good.
> 
> Eg. if we're just disabling the entire pipe+DPLL, we'd do this (assuming
> cdclk doesn't need changing, but voltage can be lowered due to the port
> no longer requiring it):
> 1. disable DPLL
> 2. disable DPLL power
> ...
> 3. DVFS pre sequence
> 4. DVFS post sequence
> 
> And when starting up a pipe from the cold with a new DPLL we'd conversly
> do this (again assuming no cdclk change, but voltage would have to be
> ramped up for the port):
> 1. DVFS pre sequence
> 2. DVFS post sequence
> ...
> 3. enable DPLL power
> 4. enable DPLL
> 
> It does look a bit strange doing the DVFS sequences on their own like
> that, but I don't see why that should be problem. From where I'm sitting
> it doesn't really look any different than the following seqeunces:
> 
> Disable with cdclk changing but port clock not having any effect
> on the voltage:
> 1. disable DPLL
> 2. disable DPLL power
> ...
> 3. DVFS pre sequence
> 4. change cdclk
> 5. DVFS post sequence
> 
> Enable with cdclk changing but port clock not having any effect
> on the voltage:
> 1. DVFS pre sequence
> 2. change cdclk
> 3. DVFS post sequence
> ...
> 4. enable DPLL power
> 5. enable DPLL
> 
> So unless something is snooping the actual state of the DPLLs, and based
> on that second guessing our choice of voltage, I don't really see how
> these two cases differ at all.

Well, I admit that I'm kind of lost on the discussion now.

but spec tells that we need use pre and post DVFS sequence
always on CDCLK part and on DPLL only "If the frequency will result in a
change to the voltage requirement,"
So in the temporary disable-re-enable case we would be safe, because
there woulnd't be no need to keep tweaking the voltage...

However on the other case you mentioned for the full pipe disable
it seems that we have a situation of non optimal power getting
wasted, right?!

Any idea how to solve that in the atomic way?
Maybe caching the global min voltage required outside cdclk struct
in a place where we can have access from both cdclk and pll state?

> 
> > 
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:[e-mail]] 
> > Sent: Friday, 20 October, 2017 4:12 AM
> > To: Vivi, Rodrigo <[e-mail]>
> > Cc: [e-mail]; Kahola, Mika <[e-mail]>; Navare, Manasi D <[e-mail]>; Runyan, Arthur J <[e-mail]>
> > Subject: Re: [PATCH 8/8] drm/i915: Adjust system agent voltage on CNL if required by DDI ports
> > 
> > On Thu, Oct 19, 2017 at 04:54:56PM -0700, Rodrigo Vivi wrote:
> > > 
> > > On Wed, Oct 18, 2017 at 08:48:25PM +0000, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <[e-mail]>
> > > > 
> > > > 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.
> > > > 
> > > > Cc: Mika Kahola <[e-mail]>
> > > > Cc: Manasi Navare <[e-mail]>
> > > > Cc: Rodrigo Vivi <[e-mail]>
> > > > Signed-off-by: Ville Syrjälä <[e-mail]>
> > > 
> > > 
> > > Reviewed-by: Rodrigo Vivi <[e-mail]>
> > > 
> > > Tested-by: Rodrigo Vivi <[e-mail]>
> > > (Although I didn't find cases where I could force a higher voltage level,
> > > everything works well on CNL with this.)
> > 
> > I went and actually read the spec now :) so I can now see that this
> > doesn't really match the sequence listed in the spec.
> > 
> > For example, let's assume we have a modeset where we have to disable
> > a DPLL, change CDCLK, and finally enable a DPLL again.
> > 
> > What the spec says we should do is something like this:
> > 1.  DVFS pre sequence
> > 2.  disable DPLL
> > 3.  DVFS post sequence
> > 4.  disable DPLL power
> > ...
> > 5.  DVFS pre sequence
> > 6.  configure CDCLK_CTL
> > 7.  DVFS post sequence
> > ...
> > 8.  enable DPLL power
> > 9.  DVFS pre sequence
> > 10. enable DPLL
> > 11. DVFS post sequence
> > 
> > With my code what we'd end up doing is this instead:
> > 1. disable DPLL
> > 2. disable DPLL power
> > ...
> > 3. DVFS pre sequence
> > 4. configure CDCLK_CTL
> > 5. DVFS post sequence
> > ...
> > 6. enable DPLL power
> > 7. enable DPLL
> > 
> > So my way always results in running the DVFS sequences at most once.
> > With the way the spec has things listed we might have to run the
> > sequences multiple times. 
> > 
> > Art, is there a good reason why we'd actually have to run the
> > DVFS sequences around each DPLL_ENABLE write, instead of just
> > doing it once at any point between disabling and enabling
> > DPLLs?
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> 
> -- 
> Ville Syrjälä
> Intel OTC

Runyan, Arthur J Oct. 20, 2017, 9:44 p.m. UTC | #8

If you know which direction voltage is moving, then you can optimize.  Nobody snooping PLL.  Really only the DVFS post sequence is needed, placed either before or after the clock programming, depending on whether voltage is increasing or decreasing.  But you shouldn't optimize that far since nobody is validating it that way.
You do want to lower voltage to save power.

-----Original Message-----
From: Vivi, Rodrigo 
Sent: Friday, 20 October, 2017 1:37 PM
To: Ville Syrjälä <[e-mail]>
Cc: Runyan, Arthur J <[e-mail]>; [e-mail]; Kahola, Mika <[e-mail]>; Navare, Manasi D <[e-mail]>
Subject: Re: [PATCH 8/8] drm/i915: Adjust system agent voltage on CNL if required by DDI ports

On Fri, Oct 20, 2017 at 08:07:07PM +0000, Ville Syrjälä wrote:
> On Fri, Oct 20, 2017 at 05:48:54PM +0000, Runyan, Arthur J wrote:
> > Sorry about top reply from corporate email...
> > If you know in advance that you will just be temporarily disabling the PLL, then your sequence works. 
> 
> Actually we would end up using this sequence even if disable the PLL for
> good.
> 
> Eg. if we're just disabling the entire pipe+DPLL, we'd do this (assuming
> cdclk doesn't need changing, but voltage can be lowered due to the port
> no longer requiring it):
> 1. disable DPLL
> 2. disable DPLL power
> ...
> 3. DVFS pre sequence
> 4. DVFS post sequence
> 
> And when starting up a pipe from the cold with a new DPLL we'd conversly
> do this (again assuming no cdclk change, but voltage would have to be
> ramped up for the port):
> 1. DVFS pre sequence
> 2. DVFS post sequence
> ...
> 3. enable DPLL power
> 4. enable DPLL
> 
> It does look a bit strange doing the DVFS sequences on their own like
> that, but I don't see why that should be problem. From where I'm sitting
> it doesn't really look any different than the following seqeunces:
> 
> Disable with cdclk changing but port clock not having any effect
> on the voltage:
> 1. disable DPLL
> 2. disable DPLL power
> ...
> 3. DVFS pre sequence
> 4. change cdclk
> 5. DVFS post sequence
> 
> Enable with cdclk changing but port clock not having any effect
> on the voltage:
> 1. DVFS pre sequence
> 2. change cdclk
> 3. DVFS post sequence
> ...
> 4. enable DPLL power
> 5. enable DPLL
> 
> So unless something is snooping the actual state of the DPLLs, and based
> on that second guessing our choice of voltage, I don't really see how
> these two cases differ at all.

Well, I admit that I'm kind of lost on the discussion now.

but spec tells that we need use pre and post DVFS sequence
always on CDCLK part and on DPLL only "If the frequency will result in a
change to the voltage requirement,"
So in the temporary disable-re-enable case we would be safe, because
there woulnd't be no need to keep tweaking the voltage...

However on the other case you mentioned for the full pipe disable
it seems that we have a situation of non optimal power getting
wasted, right?!

Any idea how to solve that in the atomic way?
Maybe caching the global min voltage required outside cdclk struct
in a place where we can have access from both cdclk and pll state?

> 
> > 
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:[e-mail]] 
> > Sent: Friday, 20 October, 2017 4:12 AM
> > To: Vivi, Rodrigo <[e-mail]>
> > Cc: [e-mail]; Kahola, Mika <[e-mail]>; Navare, Manasi D <[e-mail]>; Runyan, Arthur J <[e-mail]>
> > Subject: Re: [PATCH 8/8] drm/i915: Adjust system agent voltage on CNL if required by DDI ports
> > 
> > On Thu, Oct 19, 2017 at 04:54:56PM -0700, Rodrigo Vivi wrote:
> > > 
> > > On Wed, Oct 18, 2017 at 08:48:25PM +0000, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <[e-mail]>
> > > > 
> > > > 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.
> > > > 
> > > > Cc: Mika Kahola <[e-mail]>
> > > > Cc: Manasi Navare <[e-mail]>
> > > > Cc: Rodrigo Vivi <[e-mail]>
> > > > Signed-off-by: Ville Syrjälä <[e-mail]>
> > > 
> > > 
> > > Reviewed-by: Rodrigo Vivi <[e-mail]>
> > > 
> > > Tested-by: Rodrigo Vivi <[e-mail]>
> > > (Although I didn't find cases where I could force a higher voltage level,
> > > everything works well on CNL with this.)
> > 
> > I went and actually read the spec now :) so I can now see that this
> > doesn't really match the sequence listed in the spec.
> > 
> > For example, let's assume we have a modeset where we have to disable
> > a DPLL, change CDCLK, and finally enable a DPLL again.
> > 
> > What the spec says we should do is something like this:
> > 1.  DVFS pre sequence
> > 2.  disable DPLL
> > 3.  DVFS post sequence
> > 4.  disable DPLL power
> > ...
> > 5.  DVFS pre sequence
> > 6.  configure CDCLK_CTL
> > 7.  DVFS post sequence
> > ...
> > 8.  enable DPLL power
> > 9.  DVFS pre sequence
> > 10. enable DPLL
> > 11. DVFS post sequence
> > 
> > With my code what we'd end up doing is this instead:
> > 1. disable DPLL
> > 2. disable DPLL power
> > ...
> > 3. DVFS pre sequence
> > 4. configure CDCLK_CTL
> > 5. DVFS post sequence
> > ...
> > 6. enable DPLL power
> > 7. enable DPLL
> > 
> > So my way always results in running the DVFS sequences at most once.
> > With the way the spec has things listed we might have to run the
> > sequences multiple times. 
> > 
> > Art, is there a good reason why we'd actually have to run the
> > DVFS sequences around each DPLL_ENABLE write, instead of just
> > doing it once at any point between disabling and enabling
> > DPLLs?
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> 
> -- 
> Ville Syrjälä
> Intel OTC

Ville Syrjälä Oct. 23, 2017, 11:48 a.m. UTC | #9

On Fri, Oct 20, 2017 at 01:36:42PM -0700, Rodrigo Vivi wrote:
> On Fri, Oct 20, 2017 at 08:07:07PM +0000, Ville Syrjälä wrote:
> > On Fri, Oct 20, 2017 at 05:48:54PM +0000, Runyan, Arthur J wrote:
> > > Sorry about top reply from corporate email...
> > > If you know in advance that you will just be temporarily disabling the PLL, then your sequence works. 
> > 
> > Actually we would end up using this sequence even if disable the PLL for
> > good.
> > 
> > Eg. if we're just disabling the entire pipe+DPLL, we'd do this (assuming
> > cdclk doesn't need changing, but voltage can be lowered due to the port
> > no longer requiring it):
> > 1. disable DPLL
> > 2. disable DPLL power
> > ...
> > 3. DVFS pre sequence
> > 4. DVFS post sequence
> > 
> > And when starting up a pipe from the cold with a new DPLL we'd conversly
> > do this (again assuming no cdclk change, but voltage would have to be
> > ramped up for the port):
> > 1. DVFS pre sequence
> > 2. DVFS post sequence
> > ...
> > 3. enable DPLL power
> > 4. enable DPLL
> > 
> > It does look a bit strange doing the DVFS sequences on their own like
> > that, but I don't see why that should be problem. From where I'm sitting
> > it doesn't really look any different than the following seqeunces:
> > 
> > Disable with cdclk changing but port clock not having any effect
> > on the voltage:
> > 1. disable DPLL
> > 2. disable DPLL power
> > ...
> > 3. DVFS pre sequence
> > 4. change cdclk
> > 5. DVFS post sequence
> > 
> > Enable with cdclk changing but port clock not having any effect
> > on the voltage:
> > 1. DVFS pre sequence
> > 2. change cdclk
> > 3. DVFS post sequence
> > ...
> > 4. enable DPLL power
> > 5. enable DPLL
> > 
> > So unless something is snooping the actual state of the DPLLs, and based
> > on that second guessing our choice of voltage, I don't really see how
> > these two cases differ at all.
> 
> Well, I admit that I'm kind of lost on the discussion now.
> 
> but spec tells that we need use pre and post DVFS sequence
> always on CDCLK part and on DPLL only "If the frequency will result in a
> change to the voltage requirement,"
> So in the temporary disable-re-enable case we would be safe, because
> there woulnd't be no need to keep tweaking the voltage...
> 
> However on the other case you mentioned for the full pipe disable
> it seems that we have a situation of non optimal power getting
> wasted, right?!

No. The voltage will be reduced, but only after all relevant DPLLs have
been disabled. And conversly the voltage will be increased before any
relevant DPLLs will be enabled. So instead of talking to pcode around
the actual DPLL enable/disable register writes we do it well before/after
we touch the DPLL(s).

Ville Syrjälä Oct. 23, 2017, 12:03 p.m. UTC | #10

On Fri, Oct 20, 2017 at 09:44:53PM +0000, Runyan, Arthur J wrote:
> If you know which direction voltage is moving, then you can optimize.  Nobody snooping PLL.  Really only the DVFS post sequence is needed, placed either before or after the clock programming, depending on whether voltage is increasing or decreasing.  But you shouldn't optimize that far since nobody is validating it that way.
> You do want to lower voltage to save power.

OK, so it looks like what I had in mind will work pefectly fine then.

And it's good that you mentioned the validation angle. We'll definitely
want to stick to using both the pre and post sequences just to be on the
safe side.

Thanks Art.

> 
> -----Original Message-----
> From: Vivi, Rodrigo 
> Sent: Friday, 20 October, 2017 1:37 PM
> To: Ville Syrjälä <[e-mail]>
> Cc: Runyan, Arthur J <[e-mail]>; [e-mail]; Kahola, Mika <[e-mail]>; Navare, Manasi D <[e-mail]>
> Subject: Re: [PATCH 8/8] drm/i915: Adjust system agent voltage on CNL if required by DDI ports
> 
> On Fri, Oct 20, 2017 at 08:07:07PM +0000, Ville Syrjälä wrote:
> > On Fri, Oct 20, 2017 at 05:48:54PM +0000, Runyan, Arthur J wrote:
> > > Sorry about top reply from corporate email...
> > > If you know in advance that you will just be temporarily disabling the PLL, then your sequence works. 
> > 
> > Actually we would end up using this sequence even if disable the PLL for
> > good.
> > 
> > Eg. if we're just disabling the entire pipe+DPLL, we'd do this (assuming
> > cdclk doesn't need changing, but voltage can be lowered due to the port
> > no longer requiring it):
> > 1. disable DPLL
> > 2. disable DPLL power
> > ...
> > 3. DVFS pre sequence
> > 4. DVFS post sequence
> > 
> > And when starting up a pipe from the cold with a new DPLL we'd conversly
> > do this (again assuming no cdclk change, but voltage would have to be
> > ramped up for the port):
> > 1. DVFS pre sequence
> > 2. DVFS post sequence
> > ...
> > 3. enable DPLL power
> > 4. enable DPLL
> > 
> > It does look a bit strange doing the DVFS sequences on their own like
> > that, but I don't see why that should be problem. From where I'm sitting
> > it doesn't really look any different than the following seqeunces:
> > 
> > Disable with cdclk changing but port clock not having any effect
> > on the voltage:
> > 1. disable DPLL
> > 2. disable DPLL power
> > ...
> > 3. DVFS pre sequence
> > 4. change cdclk
> > 5. DVFS post sequence
> > 
> > Enable with cdclk changing but port clock not having any effect
> > on the voltage:
> > 1. DVFS pre sequence
> > 2. change cdclk
> > 3. DVFS post sequence
> > ...
> > 4. enable DPLL power
> > 5. enable DPLL
> > 
> > So unless something is snooping the actual state of the DPLLs, and based
> > on that second guessing our choice of voltage, I don't really see how
> > these two cases differ at all.
> 
> Well, I admit that I'm kind of lost on the discussion now.
> 
> but spec tells that we need use pre and post DVFS sequence
> always on CDCLK part and on DPLL only "If the frequency will result in a
> change to the voltage requirement,"
> So in the temporary disable-re-enable case we would be safe, because
> there woulnd't be no need to keep tweaking the voltage...
> 
> However on the other case you mentioned for the full pipe disable
> it seems that we have a situation of non optimal power getting
> wasted, right?!
> 
> Any idea how to solve that in the atomic way?
> Maybe caching the global min voltage required outside cdclk struct
> in a place where we can have access from both cdclk and pll state?
> 
> > 
> > > 
> > > -----Original Message-----
> > > From: Ville Syrjälä [mailto:[e-mail]] 
> > > Sent: Friday, 20 October, 2017 4:12 AM
> > > To: Vivi, Rodrigo <[e-mail]>
> > > Cc: [e-mail]; Kahola, Mika <[e-mail]>; Navare, Manasi D <[e-mail]>; Runyan, Arthur J <[e-mail]>
> > > Subject: Re: [PATCH 8/8] drm/i915: Adjust system agent voltage on CNL if required by DDI ports
> > > 
> > > On Thu, Oct 19, 2017 at 04:54:56PM -0700, Rodrigo Vivi wrote:
> > > > 
> > > > On Wed, Oct 18, 2017 at 08:48:25PM +0000, Ville Syrjala wrote:
> > > > > From: Ville Syrjälä <[e-mail]>
> > > > > 
> > > > > 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.
> > > > > 
> > > > > Cc: Mika Kahola <[e-mail]>
> > > > > Cc: Manasi Navare <[e-mail]>
> > > > > Cc: Rodrigo Vivi <[e-mail]>
> > > > > Signed-off-by: Ville Syrjälä <[e-mail]>
> > > > 
> > > > 
> > > > Reviewed-by: Rodrigo Vivi <[e-mail]>
> > > > 
> > > > Tested-by: Rodrigo Vivi <[e-mail]>
> > > > (Although I didn't find cases where I could force a higher voltage level,
> > > > everything works well on CNL with this.)
> > > 
> > > I went and actually read the spec now :) so I can now see that this
> > > doesn't really match the sequence listed in the spec.
> > > 
> > > For example, let's assume we have a modeset where we have to disable
> > > a DPLL, change CDCLK, and finally enable a DPLL again.
> > > 
> > > What the spec says we should do is something like this:
> > > 1.  DVFS pre sequence
> > > 2.  disable DPLL
> > > 3.  DVFS post sequence
> > > 4.  disable DPLL power
> > > ...
> > > 5.  DVFS pre sequence
> > > 6.  configure CDCLK_CTL
> > > 7.  DVFS post sequence
> > > ...
> > > 8.  enable DPLL power
> > > 9.  DVFS pre sequence
> > > 10. enable DPLL
> > > 11. DVFS post sequence
> > > 
> > > With my code what we'd end up doing is this instead:
> > > 1. disable DPLL
> > > 2. disable DPLL power
> > > ...
> > > 3. DVFS pre sequence
> > > 4. configure CDCLK_CTL
> > > 5. DVFS post sequence
> > > ...
> > > 6. enable DPLL power
> > > 7. enable DPLL
> > > 
> > > So my way always results in running the DVFS sequences at most once.
> > > With the way the spec has things listed we might have to run the
> > > sequences multiple times. 
> > > 
> > > Art, is there a good reason why we'd actually have to run the
> > > DVFS sequences around each DPLL_ENABLE write, instead of just
> > > doing it once at any point between disabling and enabling
> > > DPLLs?
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel OTC
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC

Patch

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 795a18f64c4c..035c44858908 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -1666,6 +1666,13 @@  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 this out :( Can't rely on .get_cdclk() since it
+	 * doesn't know about DDI voltage requirements. 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)
@@ -1935,6 +1942,29 @@  static int intel_compute_min_cdclk(struct drm_atomic_state *state)
 	return min_cdclk;
 }
 
+static u8 intel_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)
+		state->min_voltage[i] = crtc_state->min_voltage;
+
+	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);
@@ -2091,7 +2121,9 @@  static int cnl_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 = cnl_calc_voltage(cdclk);
+	intel_state->cdclk.logical.voltage =
+		max(cnl_calc_voltage(cdclk),
+		    intel_compute_min_voltage(intel_state));
 
 	if (!intel_state->active_crtcs) {
 		cdclk = cnl_calc_cdclk(0);
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index adf51b328844..f02e3c2f4ad2 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2525,6 +2525,15 @@  bool intel_ddi_is_audio_enabled(struct drm_i915_private *dev_priv,
 	return false;
 }
 
+void intel_ddi_compute_min_voltage(struct drm_i915_private *dev_priv,
+				   struct intel_crtc_state *crtc_state)
+{
+	if (INTEL_GEN(dev_priv) >= 10 && crtc_state->port_clock > 594000)
+		crtc_state->min_voltage = 2;
+	else
+		crtc_state->min_voltage = 0;
+}
+
 void intel_ddi_get_config(struct intel_encoder *encoder,
 			  struct intel_crtc_state *pipe_config)
 {
@@ -2624,6 +2633,8 @@  void intel_ddi_get_config(struct intel_encoder *encoder,
 	if (IS_GEN9_LP(dev_priv))
 		pipe_config->lane_lat_optim_mask =
 			bxt_ddi_phy_get_lane_lat_optim_mask(encoder);
+
+	intel_ddi_compute_min_voltage(dev_priv, pipe_config);
 }
 
 static bool intel_ddi_compute_config(struct intel_encoder *encoder,
@@ -2650,6 +2661,8 @@  static bool intel_ddi_compute_config(struct intel_encoder *encoder,
 			bxt_ddi_phy_calc_lane_lat_optim_mask(encoder,
 							     pipe_config->lane_count);
 
+	intel_ddi_compute_min_voltage(dev_priv, pipe_config);
+
 	return ret;
 
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 32e7cca52da2..7d293ecb51c4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5938,6 +5938,7 @@  static void intel_crtc_disable_noatomic(struct drm_crtc *crtc,
 
 	dev_priv->active_crtcs &= ~(1 << intel_crtc->pipe);
 	dev_priv->min_cdclk[intel_crtc->pipe] = 0;
+	dev_priv->min_voltage[intel_crtc->pipe] = 0;
 }
 
 /*
@@ -11304,6 +11305,8 @@  intel_pipe_config_compare(struct drm_i915_private *dev_priv,
 	PIPE_CONF_CHECK_CLOCK_FUZZY(base.adjusted_mode.crtc_clock);
 	PIPE_CONF_CHECK_CLOCK_FUZZY(port_clock);
 
+	PIPE_CONF_CHECK_I(min_voltage);
+
 #undef PIPE_CONF_CHECK_X
 #undef PIPE_CONF_CHECK_I
 #undef PIPE_CONF_CHECK_P
@@ -12532,6 +12535,8 @@  static int intel_atomic_commit(struct drm_device *dev,
 	if (intel_state->modeset) {
 		memcpy(dev_priv->min_cdclk, intel_state->min_cdclk,
 		       sizeof(intel_state->min_cdclk));
+		memcpy(dev_priv->min_voltage, intel_state->min_voltage,
+		       sizeof(intel_state->min_voltage));
 		dev_priv->active_crtcs = intel_state->active_crtcs;
 		dev_priv->cdclk.logical = intel_state->cdclk.logical;
 		dev_priv->cdclk.actual = intel_state->cdclk.actual;
@@ -15042,6 +15047,7 @@  static void intel_modeset_readout_hw_state(struct drm_device *dev)
 		}
 
 		dev_priv->min_cdclk[crtc->pipe] = min_cdclk;
+		dev_priv->min_voltage[crtc->pipe] = crtc_state->min_voltage;
 
 		intel_pipe_config_sanity_check(dev_priv, crtc_state);
 	}
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 772521440a9f..6c1da88b5fef 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -34,6 +34,7 @@  static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 					struct intel_crtc_state *pipe_config,
 					struct drm_connector_state *conn_state)
 {
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct intel_dp_mst_encoder *intel_mst = enc_to_mst(&encoder->base);
 	struct intel_digital_port *intel_dig_port = intel_mst->primary;
 	struct intel_dp *intel_dp = &intel_dig_port->dp;
@@ -87,6 +88,8 @@  static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 
 	pipe_config->dp_m_n.tu = slots;
 
+	intel_ddi_compute_min_voltage(dev_priv, pipe_config);
+
 	return true;
 }
 
@@ -307,6 +310,8 @@  static void intel_dp_mst_enc_get_config(struct intel_encoder *encoder,
 	intel_dp_get_m_n(crtc, pipe_config);
 
 	intel_ddi_clock_get(&intel_dig_port->base, pipe_config);
+
+	intel_ddi_compute_min_voltage(dev_priv, pipe_config);
 }
 
 static int intel_dp_mst_get_ddc_modes(struct drm_connector *connector)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 765a737700c5..002894b13d69 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -386,6 +386,8 @@  struct intel_atomic_state {
 	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];
 
 	struct intel_shared_dpll_state shared_dpll[I915_NUM_PLLS];
 
@@ -739,6 +741,8 @@  struct intel_crtc_state {
 	 */
 	uint8_t lane_lat_optim_mask;
 
+	u8 min_voltage;
+
 	/* Panel fitter controls for gen2-gen4 + VLV */
 	struct {
 		u32 control;
@@ -1294,6 +1298,8 @@  void intel_ddi_clock_get(struct intel_encoder *encoder,
 			 struct intel_crtc_state *pipe_config);
 void intel_ddi_set_vc_payload_alloc(const struct intel_crtc_state *crtc_state,
 				    bool state);
+void intel_ddi_compute_min_voltage(struct drm_i915_private *dev_priv,
+				   struct intel_crtc_state *crtc_state);
 u32 bxt_signal_levels(struct intel_dp *intel_dp);
 uint32_t ddi_signal_levels(struct intel_dp *intel_dp);
 u8 intel_ddi_dp_voltage_max(struct intel_encoder *encoder);