Re: [PATCH] drm/i915/cnp+: update to the new RAWCLK_FREQ recommendations

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Oct 18, 2018 at 03:52:21PM +0300, Ville Syrjälä wrote:
> On Tue, Oct 16, 2018 at 04:09:19PM -0700, Rodrigo Vivi wrote:
> > On Tue, Oct 16, 2018 at 04:00:26PM -0700, Paulo Zanoni wrote:
> > > Em Sex, 2018-10-12 às 15:42 +0300, Ville Syrjälä escreveu:
> > > > On Thu, Oct 11, 2018 at 05:40:45PM -0700, Paulo Zanoni wrote:
> > > > > These are the new recommended values provided by our spec (18 -> 19
> > > > > and 23 -> 24). It seems this should help fixing GMBUS issues. Since
> > > > > we're doing pretty much the same thing for both CNP and ICP now,
> > > > > unify
> > > > > the functions using the ICP version since it's more straightforward
> > > > > by
> > > > > just matching the values listed in BSpec instead of recalculating
> > > > > them.
> > > > 
> > > > IMO calculating would be better. Would be trivial to see that the
> > > > values
> > > > are at least consistent. With four magic numbers you have no choice
> > > > but
> > > > to dig up the spec, which is painful. I don't like needless pain that
> > > > much.
> > > 
> > > Then I guess our workloads are different. IMHO the code is trivial when
> > > we can easily see that we set the exact magic values that the spec
> > > tells us to set.  With the formula, you have to do the calculations for
> > > both frequencies, then you take those values and compare against the
> > > spec, which is an extra step. Developing without the spec open is
> > > something that I never even consider.
> > 
> > I am assumed lazy, so for me it is much better if I can put something
> > side by side and compare. So having it matching with whatever/however
> > spec is telling us is always better than calculations.
> 
> If I'm changing a set of magic numbers derived from some formula
> I would do the calculations anyway because the spec could be wrong.
> So IMO it's better to do the calculations in the first place as
> that requires you to understand what those magic numbers mean.
> Having the formula in the code makes that easier as then you
> don't have to derive it from the spec every time.

this is a good point. And Paulo told he is open to send another
version

> 
> To me blindly copy pasting magic numbers from a spec seems like
> a job for an automaton rather than an engineer.

bummer... you are spoiling some ideas I have here to make bspec's svn
changes to check our code and file a jira for us ;)

> I definitely
> don't enjoy doing that kind of work, which means I won't be all
> that diligent when doing it.

also good point...

> And that increases the chance of
> human error in the process. If we can make due with a single
> magic number rather than four there's less opportunity to
> get it wrong.

although I bet this varies from person to person...

but I see what you mean ;)

> 
> Just my 2c. I presume not everyone will be convinced.
> 
> > 
> > In case spec changes and we get notification it is easier to get and
> > change values directly.
> > 
> > > 
> > > Anyway, I can submit another version with the cnl model, no problem.
> > > 
> > > > 
> > > > > 
> > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_reg.h    |  1 -
> > > > >  drivers/gpu/drm/i915/intel_cdclk.c | 37 ++++++------------------
> > > > > -------------
> > > > >  2 files changed, 6 insertions(+), 32 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > > b/drivers/gpu/drm/i915/i915_reg.h
> > > > > index 20785417953d..ffd564a8d339 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > @@ -7832,7 +7832,6 @@ enum {
> > > > >  #define  CNP_RAWCLK_DIV(div)	((div) << 16)
> > > > >  #define  CNP_RAWCLK_FRAC_MASK	(0xf << 26)
> > > > >  #define  CNP_RAWCLK_FRAC(frac)	((frac) << 26)
> > > > > -#define  ICP_RAWCLK_DEN(den)	((den) << 26)
> > > > >  #define  ICP_RAWCLK_NUM(num)	((num) << 11)
> > > > >  
> > > > >  #define PCH_DPLL_TMR_CFG        _MMIO(0xc6208)
> > > > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> > > > > b/drivers/gpu/drm/i915/intel_cdclk.c
> > > > > index 29075c763428..17d3f13d89db 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > > > > @@ -2660,48 +2660,25 @@ void intel_update_cdclk(struct
> > > > > drm_i915_private *dev_priv)
> > > > >  }
> > > > >  
> > > > >  static int cnp_rawclk(struct drm_i915_private *dev_priv)
> > > > > -{
> > > > > -	u32 rawclk;
> > > > > -	int divider, fraction;
> > > > > -
> > > > > -	if (I915_READ(SFUSE_STRAP) & SFUSE_STRAP_RAW_FREQUENCY) {
> > > > > -		/* 24 MHz */
> > > > > -		divider = 24000;
> > > > > -		fraction = 0;
> > > > > -	} else {
> > > > > -		/* 19.2 MHz */
> > > > > -		divider = 19000;
> > > > > -		fraction = 200;
> > > > > -	}
> > > > > -
> > > > > -	rawclk = CNP_RAWCLK_DIV((divider / 1000) - 1);
> > > > > -	if (fraction)
> > > > > -		rawclk |= CNP_RAWCLK_FRAC(DIV_ROUND_CLOSEST(1000,
> > > > > -							    fracti
> > > > > on) - 1);
> > > > > -
> > > > > -	I915_WRITE(PCH_RAWCLK_FREQ, rawclk);
> > > > > -	return divider + fraction;
> > > > > -}
> > > > > -
> > > > > -static int icp_rawclk(struct drm_i915_private *dev_priv)
> > > > >  {
> > > > >  	u32 rawclk;
> > > > >  	int divider, numerator, denominator, frequency;
> > > > >  
> > > > >  	if (I915_READ(SFUSE_STRAP) & SFUSE_STRAP_RAW_FREQUENCY) {
> > > > >  		frequency = 24000;
> > > > > -		divider = 23;
> > > > > +		divider = 24;
> > > > >  		numerator = 0;
> > > > >  		denominator = 0;
> > > > >  	} else {
> > > > >  		frequency = 19200;
> > > > > -		divider = 18;
> > > > > +		divider = 19;
> > > > >  		numerator = 1;
> > > > >  		denominator = 4;
> > > > 
> > > > These numbers are extremely confusing. The hardware wants the
> > > > numerator
> > > > as is but then it wants denominator-1. Which is a but odd. I think
> > > > I'd
> > > > move the -1 for the denominator into the macro (or the invocation of
> > > > the
> > > > macro, like cnp had). Alternatively we could at least write this as
> > > > 5-1
> > > > here, so that the reader has at least some chance to figure out what
> > > > this means.
> > > > 
> > > > >  	}
> > > > >  
> > > > > -	rawclk = CNP_RAWCLK_DIV(divider) |
> > > > > ICP_RAWCLK_NUM(numerator) |
> > > > > -		 ICP_RAWCLK_DEN(denominator);
> > > > > +	rawclk = CNP_RAWCLK_DIV(divider) |
> > > > > CNP_RAWCLK_FRAC(denominator);
> > > > > +	if (HAS_PCH_ICP(dev_priv))
> > > > > +		rawclk |= ICP_RAWCLK_NUM(numerator);
> > > > >  
> > > > >  	I915_WRITE(PCH_RAWCLK_FREQ, rawclk);
> > > > >  	return frequency;
> > > > > @@ -2754,9 +2731,7 @@ static int g4x_hrawclk(struct
> > > > > drm_i915_private *dev_priv)
> > > > >   */
> > > > >  void intel_update_rawclk(struct drm_i915_private *dev_priv)
> > > > >  {
> > > > > -	if (HAS_PCH_ICP(dev_priv))
> > > > > -		dev_priv->rawclk_freq = icp_rawclk(dev_priv);
> > > > > -	else if (HAS_PCH_CNP(dev_priv))
> > > > > +	if (HAS_PCH_CNP(dev_priv) || HAS_PCH_ICP(dev_priv))
> > > > >  		dev_priv->rawclk_freq = cnp_rawclk(dev_priv);
> > > > >  	else if (HAS_PCH_SPLIT(dev_priv))
> > > > >  		dev_priv->rawclk_freq = pch_rawclk(dev_priv);
> > > > > -- 
> > > > > 2.14.4
> > > > > 
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > 
> > > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux