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

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

On Wed, Oct 04, 2017 at 03:40:11PM -0700, Manasi Navare wrote:
> 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
>

Never mind, I was looking at a different section which had this logic.

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx