Re: [PATCH 07/13] drm/i915/cnl: DVFS for PLL enabling

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Oct 17, 2017 at 04:23:07PM -0700, Rodrigo Vivi wrote:
> On Tue, Oct 17, 2017 at 08:36:14PM +0000, Ville Syrjälä wrote:
> > On Tue, Oct 17, 2017 at 09:02:05PM +0300, Ville Syrjälä wrote:
> > > On Tue, Oct 17, 2017 at 10:45:22AM -0700, Rodrigo Vivi wrote:
> > > > On Tue, Oct 17, 2017 at 05:23:20PM +0000, Ville Syrjälä wrote:
> > > > > On Tue, Oct 17, 2017 at 09:47:05AM -0700, Rodrigo Vivi wrote:
> > > > > > On Tue, Oct 17, 2017 at 03:44:21PM +0000, Ville Syrjälä wrote:
> > > > > > > On Tue, Oct 03, 2017 at 12:06:08AM -0700, Rodrigo Vivi wrote:
> > > > > > > > From: "Kahola, Mika" <mika.kahola@intel.com>
> > > > > > > > 
> > > > > > > > Display Voltage and Frequency Switching (DVFS) is used to adjust the
> > > > > > > > display voltage to match the display clock frequencies. If voltage is
> > > > > > > > set too low, it will break functionality. If voltage is set too high,
> > > > > > > > it will waste power. Voltage level is selected based on CD clock and
> > > > > > > > DDI clock.
> > > > > > > > 
> > > > > > > > The sequence before frequency change is the following and it requests
> > > > > > > > the power controller to raise voltage to maximum
> > > > > > > > 
> > > > > > > > - Ensure any previous GT Driver Mailbox transaction is complete.
> > > > > > > > - Write GT Driver Mailbox Data Low = 0x3.
> > > > > > > > - Write GT Driver Mailbox Data High = 0x0.
> > > > > > > > - Write GT Driver Mailbox Interface = 0x80000007.
> > > > > > > > - Poll GT Driver Mailbox Interface for Run/Busy indication cleared (bit 31 = 0).
> > > > > > > > - Read GT Driver Mailbox Data Low, if bit 0 is 0x1, continue, else restart the sequence.
> > > > > > > >   Timeout after 3ms
> > > > > > > > 
> > > > > > > > The sequence after frequency change is the following and it requests
> > > > > > > > the port controller to raise voltage to the requested level.
> > > > > > > > 
> > > > > > > > - Write GT Driver Mailbox Data Low
> > > > > > > >  * For level 0, write 0x0
> > > > > > > >  * For level 1, write 0x1
> > > > > > > >  * For level 2, write 0x2
> > > > > > > >  * For level 3, write 0x3
> > > > > > > >    - Write GT Driver Mailbox Data High = 0x0.
> > > > > > > >    - Write GT Driver Mailbox Interface = 0x80000007.
> > > > > > > > 
> > > > > > > > For Cannonlake, the level 3 is not used and it aliases to level 2.
> > > > > > > > 
> > > > > > > > v2: reuse Paulo's work on cdclk. This patch depends on Paulo's patch
> > > > > > > >     [PATCH 02/12] drm/i915/cnl: extract cnl_dvfs_{pre,post}_change
> > > > > > > > v3: (By Rodrigo): Remove duplicated commend and fix typo on Paulo's name.
> > > > > > > > v4: (By Rodrigo): Rebase on top "Unify and export gen9+ port_clock calculation"
> > > > > > > >     The port clock calculation here was only addressing DP, so let's reuse
> > > > > > > >     the current port calculation that is already in place without any duplication.
> > > > > > > >     Alos fix portclk <= 594000 instead of portclk < 594000.
> > > > > > > > 
> > > > > > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > Signed-off-by: Kahola, Mika <mika.kahola@intel.com>
> > > > > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > > > ---
> > > > > > > >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 30 ++++++++++++++++++++++--------
> > > > > > > >  1 file changed, 22 insertions(+), 8 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > > > > index a2a3d93d67bd..6030fbafa580 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > > > > @@ -1966,10 +1966,23 @@ static const struct intel_dpll_mgr bxt_pll_mgr = {
> > > > > > > >  	.dump_hw_state = bxt_dump_hw_state,
> > > > > > > >  };
> > > > > > > >  
> > > > > > > > +static int cnl_get_dvfs_level(int cdclk, int portclk)
> > > > > > > > +{
> > > > > > > > +	if (cdclk == 168000 && portclk <= 594000)
> > > > > > > > +		return 0;
> > > > > > > > +	else if (cdclk == 336000 && portclk <= 594000)
> > > > > > > > +		return 1;
> > > > > > > > +	else
> > > > > > > > +		return 2;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
> > > > > > > >  			       struct intel_shared_dpll *pll)
> > > > > > > >  {
> > > > > > > >  	uint32_t val;
> > > > > > > > +	int ret;
> > > > > > > > +	int level;
> > > > > > > > +	int cdclk, portclk;
> > > > > > > >  
> > > > > > > >  	/* 1. Enable DPLL power in DPLL_ENABLE. */
> > > > > > > >  	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
> > > > > > > > @@ -2006,11 +2019,9 @@ static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
> > > > > > > >  	/*
> > > > > > > >  	 * 5. If the frequency will result in a change to the voltage
> > > > > > > >  	 * requirement, follow the Display Voltage Frequency Switching
> > > > > > > > -	 * Sequence Before Frequency Change
> > > > > > > > -	 *
> > > > > > > > -	 * FIXME: (DVFS) is used to adjust the display voltage to match the
> > > > > > > > -	 * display clock frequencies
> > > > > > > > +	 * (DVFS) Sequence Before Frequency Change
> > > > > > > >  	 */
> > > > > > > > +	ret = cnl_dvfs_pre_change(dev_priv);
> > > > > > > >  
> > > > > > > >  	/* 6. Enable DPLL in DPLL_ENABLE. */
> > > > > > > >  	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
> > > > > > > > @@ -2028,11 +2039,14 @@ static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv,
> > > > > > > >  	/*
> > > > > > > >  	 * 8. If the frequency will result in a change to the voltage
> > > > > > > >  	 * requirement, follow the Display Voltage Frequency Switching
> > > > > > > > -	 * Sequence After Frequency Change
> > > > > > > > -	 *
> > > > > > > > -	 * FIXME: (DVFS) is used to adjust the display voltage to match the
> > > > > > > > -	 * display clock frequencies
> > > > > > > > +	 * (DVFS) Sequence After Frequency Change
> > > > > > > >  	 */
> > > > > > > > +	if (ret == 0) {
> > > > > > > > +		cdclk = dev_priv->cdclk.hw.cdclk;
> > > > > > > > +		portclk = intel_ddi_port_clock(dev_priv, pll->id);
> > > > > > > > +		level = cnl_get_dvfs_level(cdclk, portclk);
> > > > > > > > +		cnl_dvfs_post_change(dev_priv, level);
> > > > > > > 
> > > > > > > This isn't how I imagined we'd handle this. What I was after is
> > > > > > > pre-computing the correct "dfvfs level" (or what I'd probably just call
> > > > > > > "voltage" or something like that), and then we'd just let the normal
> > > > > > > cdclk programming sequence do its thing.
> > > > > > > 
> > > > > > > Is it even safe to do this with other pipes/ports enabled? I would
> > > > > > > have assumed no, but maybe I'm mistaken?
> > > > > > 
> > > > > > Well, this is how the spec is written. So I assume it is safe to change
> > > > > > that at that point.
> > > > > > 
> > > > > > Also we enable CDCLK during boot while we have no way to compute the
> > > > > > link rate. So I don't see a better way of hadling that.
> > > > > 
> > > > > We do a full state readout, which should tell us what voltage we need.
> > > > > And if the cdclk isn't even enabled, then obviously we can't have any
> > > > > pipes running either, so we can safely enable it with any frequency
> > > > > even before the full state readout.
> > > > > 
> > > > > > So, Art is it ok in the way that I'm proposing here:
> > > > > > 
> > > > > > on core display init:
> > > > > > 
> > > > > > - dvfs-pre;
> > > > > > - enable cdclk;
> > > > > > - dvfs-post considering cdclk only and link rate 0; 
> > > > > > 
> > > > > > on pll init:
> > > > > > 
> > > > > > - consider cdclk and link rate and if desired level is different from what is currently set: dvfs-pre
> > > > > > - enable pll
> > > > > > - consider cdclk and link rate and if desired level is different from what is currently set: dvfs-post with new level.
> > > > > > 
> > > > > > This is how I read the spec, but please let me know what I'm missing.
> > > > > 
> > > > > Hmm. OK, so maybe we can change the voltage while things are up and
> > > > > running.
> > > > > 
> > > > > That said, the voltage level is a global thing, so whenever we change it
> > > > > we must consider the state of all ports, and the cdclk. This looks like
> > > > > it only considers the current port. Thus if we first enable a port that
> > > > > needs high voltage, and later a port that can make due with a lower
> > > > > voltage we'll end up with something not working.
> > > > 
> > > > ouch! this is true! So, what link rate should we consider on that dvfs spec?
> > > > so, always the highest avaiblable at that time?
> > > 
> > > Yes.
> > > 
> > > > 
> > > > So we cache somewhere the highest on atomic check? and then during
> > > > these dvfs sequences we just check for the highest?
> > > 
> > > IIRC I pastebinned a sketch for this at some point. Hmm. Found it, and
> > > pushed it here:
> > > git://github.com/vsyrjala/linux.git dvfs_voltage_2
> > > 
> > > I tried to also pimp it a little bit to not force a modeset on all
> > > pipes if just the voltage changes.
> > > 
> > > I *think* this should work, after someone fills out the code for
> > > DDI .compute_config() and .get_config(). But totally untested of course
> > > so YMMV.
> > 
> > Except I forgot about the lack of a pcode command to read out the current
> > voltage setting :( So I had to add some extra kludges to the state readout 
> > on top. Branch updated.
> 
> hmmm... thanks!
> 
> ok, we need to cache something somewhere.
> 
> But for me it seems a bit hard to associate with the port clock on the pll still.

All that should be needed with my branch is set the crtc state 
min_voltage based on port_clock. No need to think about PLLS.

> 
> So I wonder if we should cache the link_rate directly into dpll_hw_state,
> on same places we set cfgcr0 values...

I wouldn't comlicate this by involving PLLs.

> so whenever we need to figure out the minimal level we do a
> for_each_pll
> max_link_clock = max(max_link_clock, pll->hw_state->link_clock)
> cnl_calc_voltage(cdclk, max_link_clock)a
> 
> or actually move this for inside cnl_calc_voltage(cdclk).
> 
> and also I don't believe we need the full modeset to change the voltage level.

That shouldn't happen with the latest stuff I pushed to the branch.

> I'd like to stay as close as possible to the spec sequence.

That would mean more code paths to do the same thing, which means
they'll accumulate different bugs. Not something I would cherish.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx