Re: [PATCH 2/4] drm/i915: add intel_calc_cdclk()

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

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@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.

> 
> 
> > 
> > +
> >  static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> > @@ -1503,14 +1544,7 @@ static int vlv_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> >  	int max_pixclk = intel_max_pixel_rate(state);
> >  	int cdclk;
> >  
> > -	/*
> > -	 * FIXME: Broadwell should also account for plane ratio
> > once 64bpp pixel
> > -	 * formats are supported.
> > -	 */
> > -	if (IS_BROADWELL(dev_priv))
> > -		cdclk = bdw_calc_cdclk(max_pixclk);
> > -	else
> > -		cdclk = vlv_calc_cdclk(dev_priv, max_pixclk);
> > +	intel_calc_cdclk(intel_state, max_pixclk, &cdclk, NULL);
> >  
> >  	if (cdclk > dev_priv->max_cdclk_freq) {
> >  		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds
> > max (%d kHz)\n",
> > @@ -1521,11 +1555,7 @@ static int vlv_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> >  	intel_state->cdclk.logical.cdclk = cdclk;
> >  
> >  	if (!intel_state->active_crtcs) {
> > -		if (IS_BROADWELL(dev_priv))
> > -			cdclk = bdw_calc_cdclk(0);
> > -		else
> > -			cdclk = vlv_calc_cdclk(dev_priv, 0);
> > -
> > +		intel_calc_cdclk(intel_state, 0, &cdclk, NULL);
> >  		intel_state->cdclk.actual.cdclk = cdclk;
> >  	} else {
> >  		intel_state->cdclk.actual = intel_state-
> > >cdclk.logical;
> > @@ -1541,22 +1571,7 @@ static int skl_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> >  	int max_pixclk = intel_max_pixel_rate(state);
> >  	int cdclk, vco;
> >  
> > -	/*
> > -	 * FIXME: Skylake/Kabylake should also account for plane
> > ratio once
> > -	 * 64bpp pixel formats are supported.
> > -	 */
> > -	if (IS_GEMINILAKE(dev_priv)) {
> > -		cdclk = glk_calc_cdclk(max_pixclk);
> > -		vco = glk_de_pll_vco(dev_priv, cdclk);
> > -	} else if (IS_BROXTON(dev_priv)) {
> > -		cdclk = bxt_calc_cdclk(max_pixclk);
> > -		vco = bxt_de_pll_vco(dev_priv, cdclk);
> > -	} else {
> > -		vco = intel_state->cdclk.logical.vco;
> > -		if (!vco)
> > -			vco = dev_priv->skl_preferred_vco_freq;
> > -		cdclk = skl_calc_cdclk(max_pixclk, vco);
> > -	}
> > +	intel_calc_cdclk(intel_state, max_pixclk, &cdclk, &vco);
> >  
> >  	if (cdclk > dev_priv->max_cdclk_freq) {
> >  		DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds
> > max (%d kHz)\n",
> > @@ -1568,16 +1583,7 @@ static int skl_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> >  	intel_state->cdclk.logical.cdclk = cdclk;
> >  
> >  	if (!intel_state->active_crtcs) {
> > -		if (IS_GEMINILAKE(dev_priv)) {
> > -			cdclk = glk_calc_cdclk(0);
> > -			vco = glk_de_pll_vco(dev_priv, cdclk);
> > -		} else if (IS_BROXTON(dev_priv)) {
> > -			cdclk = bxt_calc_cdclk(0);
> > -			vco = bxt_de_pll_vco(dev_priv, cdclk);
> > -		} else {
> > -			cdclk = skl_calc_cdclk(0, vco);
> > -		}
> > -
> > +		intel_calc_cdclk(intel_state, 0, &cdclk, &vco);
> >  		intel_state->cdclk.actual.vco = vco;
> >  		intel_state->cdclk.actual.cdclk = cdclk;
> >  	} else {
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > 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