[Intel-gfx] [PATCH 2/4] drm/i915: add intel_calc_cdclk()

[Ville Syrjälä]

On Fri, Feb 17, 2017 at 06:37:23PM -0200, Paulo Zanoni wrote:
> Em Sex, 2017-02-17 às 15:49 +0200, Ville Syrjälä escreveu:
> > On Fri, Feb 17, 2017 at 11:22:05AM -0200, Paulo Zanoni wrote:
> > > 
> > > Each x_modeset_calc_cdclk() has to do the same platform checks
> > > twice,
> > > so extract them to a single function. This way, the platform checks
> > > are all in the same place, and the platform-common code gets rid of
> > > all the platform-specific checks, which IMHO makes the code easier
> > > to
> > > read.
> > > 
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_cdclk.c | 84 ++++++++++++++++++++----
> > > --------------
> > >  1 file changed, 45 insertions(+), 39 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> > > b/drivers/gpu/drm/i915/intel_cdclk.c
> > > index d505ff1..6efc5f4 100644
> > > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > > @@ -1496,6 +1496,47 @@ static int intel_max_pixel_rate(struct
> > > drm_atomic_state *state)
> > >  	return max_pixel_rate;
> > >  }
> > >  
> > > +static void intel_calc_cdclk(struct intel_atomic_state *state, int
> > > max_pixclk,
> > > +			     int *cdclk, int *vco)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(state-
> > > >base.dev);
> > > +
> > > +	switch (INTEL_INFO(dev_priv)->platform) {
> > > +	case INTEL_VALLEYVIEW:
> > > +	case INTEL_CHERRYVIEW:
> > > +		*cdclk = vlv_calc_cdclk(dev_priv, max_pixclk);
> > > +		break;
> > > +	case INTEL_BROADWELL:
> > > +		/*
> > > +		 * FIXME: should also account for plane ratio once
> > > 64bpp pixel
> > > +		 * formats are supported.
> > > +		 */
> > > +		*cdclk = bdw_calc_cdclk(max_pixclk);
> > > +		break;
> > > +	case INTEL_SKYLAKE:
> > > +	case INTEL_KABYLAKE:
> > > +		/*
> > > +		 * FIXME: should also account for plane ratio once
> > > 64bpp pixel
> > > +		 * formats are supported.
> > > +		 */
> > > +		*vco = state->cdclk.logical.vco;
> > > +		if (!*vco)
> > > +			*vco = dev_priv->skl_preferred_vco_freq;
> > > +		*cdclk = skl_calc_cdclk(max_pixclk, *vco);
> > > +		break;
> > > +	case INTEL_BROXTON:
> > > +		*cdclk = bxt_calc_cdclk(max_pixclk);
> > > +		*vco = bxt_de_pll_vco(dev_priv, *cdclk);
> > > +		break;
> > > +	case INTEL_GEMINILAKE:
> > > +		*cdclk = glk_calc_cdclk(max_pixclk);
> > > +		*vco = glk_de_pll_vco(dev_priv, *cdclk);
> > > +		break;
> > > +	default:
> > > +		MISSING_CASE(INTEL_INFO(dev_priv)->platform);
> > > +	}
> > > +}
> > 
> > How about just replacing the .modeset_calc_cdclk() vfunc with a
> > slightly
> > lower level vfunc that just computes the cdclk/vco/whatever without
> > containing the active_crtcs logic?
> > 
> > Then we should have just
> > 
> > intel_modeset_calc_cdclk()
> > {
> > 	.calc_cdclk(logical, max_pixclk);
> > 
> > 	/*
> > 	 * maybe keep the max_cdclk check here, although it that
> > 	 * happens I think we have a bug somewhere, so perhaps
> > 	 * just convert it into a WARN, or drop entirely.
> > 	 */
> > 
> > 	if (!active_crtcs)
> > 		.calc_cdclk(actual, 0);
> > 	else
> > 		actual = logical;
> > }
> 
> Yeah, the code above is definitely a next step to the changes I did.
> 
> I'm just not a big fan of the .calc_cdclk vfunc since it will be just 2
> lines for each platform. Unless I inline them with the *real*
> x_calc_cdclk() funcs we have today, but then I'll have to check their
> other callers. So I'll take a look and try to submit a new patch.

At some point I had this idea of making the cdclk computation more data
driven. As in we'd store the various possible cdclk steps, required
guarbands etc. in some structure and thus avoid all the calc_cdclk()
functions which are mostly just if ladders with slightly different
numbers in them. But I never actually tried it, so not sure how
pretty/ugly it would turn out to be.

In the meantime, I think I'd prefer two line functions over the switch
statement. For one, it would allow us to keep the calculation part right
next to the other cdclk stuff for said platform. One thing that's a
slight concern is the future dvfs stuff we talked about. I've not yet
fully thought out where that needs to be done, but it might be that
some of it might nicely land in these two line function (making them
at least three lines ;).

-- 
Ville Syrjälä
Intel OTC