Re: [PATCH 09/13] drm/i915/cnl: Invert dvfs default level.

[Manasi Navare <manasi.d.navare@intel.com>]

On Wed, Oct 04, 2017 at 12:36:42PM -0700, Rodrigo Vivi wrote:
> On Wed, Oct 04, 2017 at 09:46:41AM +0000, Mika Kahola wrote:
> > On Tue, 2017-10-03 at 00:06 -0700, Rodrigo Vivi wrote:
> > > According to spec "If voltage is set too low,
> > > it will break functionality. If voltage is set too high,
> > >  it will waste power."
> > > 
> > > So, let's prefer the waste of power instead of breaking
> > > functionality.
> > > 
> > > But also the logic of deciding the level on spec
> > > tells "Else, use level 2."
> > > So, default is actually "2", not "0".
> > The spec also says
> > 
> > "If CD clock = 168 MHz AND DDI clock ≤ 594 MHz, use level 0.
> > Else If CD clock = 336 MHz AND DDI clock ≤ 594 MHz, use level 1.
> > Else, use level 2."
> > 
> > Should we add check for DDI clock rate as well here?
> 
> By this time dpll are disabled and not associated to any
> port yet. So portclock is 0. So if we invert the default
> we do respect the same algorightm you pasted.
> Also if you notice I'm just inverting this in a separeted
> patch to make it clear and if needed to bisect we end up
> on this single point of change. But on a later patch on
> this series I change to use your function with this algoright
> there, but giving portclock = 0.
> 
> So by the end of this series the flow is:
> 
> - enable cdclk
> - request dvfs level enough for cdclk in question.
> - enable pll
> - adjust dvfs level only if needed taking the port clock into account.
> - disable pll
> - put back level to cdclock level if needed.
> 
> > 
> > > 
> > > v2: Rebase moving it up to avoid some temporary code
> > >     duplication.
> > > 
> > > Cc: Mika Kahola <mika.kahola@intel.com>
> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_cdclk.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> > > b/drivers/gpu/drm/i915/intel_cdclk.c
> > > index af8411c2a6b9..7e9c4444c844 100644
> > > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > > @@ -1562,15 +1562,15 @@ static void cnl_set_cdclk(struct
> > > drm_i915_private *dev_priv,
> > >  	}
> > >  
> > >  	switch (cdclk) {
> > > -	case 528000:
> > > -		pcu_ack = 2;
> > > +	case 168000:
> > > +		pcu_ack = 0;
> > >  		break;
> > >  	case 336000:
> > >  		pcu_ack = 1;
> > >  		break;
> > > -	case 168000:
> > > +	case 528000:
> > >  	default:
> > > -		pcu_ack = 0;
> > > +		pcu_ack = 2;
> > >  		break;

The spec says "Else If CD clock ≤ 556.8 AND DDI clock > 594 MHz, use level 1"
So in case of cdclock = 528000, it is still < 556.8Mhz and the level should be 1

Manasi

> > >  	}
> > >  
> > -- 
> > Mika Kahola - Intel OTC
> > 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx