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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx