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

[Runyan, Arthur J]

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ä <ville.syrjala at linux.intel.com>
Cc: Runyan, Arthur J <arthur.j.runyan at intel.com>; intel-gfx at lists.freedesktop.org; Kahola, Mika <mika.kahola at intel.com>; Navare, Manasi D <manasi.d.navare at intel.com>
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:ville.syrjala at linux.intel.com] 
> > Sent: Friday, 20 October, 2017 4:12 AM
> > To: Vivi, Rodrigo <rodrigo.vivi at intel.com>
> > Cc: intel-gfx at lists.freedesktop.org; Kahola, Mika <mika.kahola at intel.com>; Navare, Manasi D <manasi.d.navare at intel.com>; Runyan, Arthur J <arthur.j.runyan at intel.com>
> > 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ä <ville.syrjala at 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.
> > > > 
> > > > Cc: Mika Kahola <mika.kahola at intel.com>
> > > > Cc: Manasi Navare <manasi.d.navare at intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > 
> > > 
> > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > 
> > > Tested-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > (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