diff mbox

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

Message ID 1487337727-9813-3-git-send-email-paulo.r.zanoni@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Zanoni, Paulo R Feb. 17, 2017, 1:22 p.m. UTC
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(-)

Comments

Ville Syrjälä Feb. 17, 2017, 1:49 p.m. UTC | #1
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;
}


> +
>  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
Zanoni, Paulo R Feb. 17, 2017, 8:37 p.m. UTC | #2
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
>
Ville Syrjälä Feb. 17, 2017, 8:48 p.m. UTC | #3
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@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 ;).
diff mbox

Patch

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);
+	}
+}
+
 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 {