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

[Ville Syrjälä]

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: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