Re: [PATCH 07/13] drm/i915: Use a table to initilize shared dplls

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 26-02-16 om 14:54 schreef Ander Conselvan de Oliveira:
> Use a table to store the per-platform shared dpll information in one
> place. This way, there is no need for platform specific init funtions.
>
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c  |  16 +--
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 189 ++++++++++++++++------------------
>  drivers/gpu/drm/i915/intel_dpll_mgr.h |  22 ++--
>  3 files changed, 108 insertions(+), 119 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e723323..133b6b7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9148,8 +9148,8 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
>  			intel_get_shared_dpll_by_id(dev_priv, pll_id);
>  		pll = pipe_config->shared_dpll;
>  
> -		WARN_ON(!pll->get_hw_state(dev_priv, pll,
> -					   &pipe_config->dpll_hw_state));
> +		WARN_ON(!pll->funcs.get_hw_state(dev_priv, pll,
> +						 &pipe_config->dpll_hw_state));
>  
>  		tmp = pipe_config->dpll_hw_state.dpll;
>  		pipe_config->pixel_multiplier =
> @@ -9695,8 +9695,8 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
>  
>  	pll = pipe_config->shared_dpll;
>  	if (pll) {
> -		WARN_ON(!pll->get_hw_state(dev_priv, pll,
> -					   &pipe_config->dpll_hw_state));
> +		WARN_ON(!pll->funcs.get_hw_state(dev_priv, pll,
> +						 &pipe_config->dpll_hw_state));
>  	}
>  
>  	/*
> @@ -12728,7 +12728,7 @@ check_shared_dpll_state(struct drm_device *dev)
>  
>  		DRM_DEBUG_KMS("%s\n", pll->name);
>  
> -		active = pll->get_hw_state(dev_priv, pll, &dpll_hw_state);
> +		active = pll->funcs.get_hw_state(dev_priv, pll, &dpll_hw_state);
>  
>  		I915_STATE_WARN(pll->active > hweight32(pll->config.crtc_mask),
>  		     "more active pll users than references: %i vs %i\n",
> @@ -15466,8 +15466,8 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>  		struct intel_shared_dpll *pll = &dev_priv->shared_dplls[i];
>  
> -		pll->on = pll->get_hw_state(dev_priv, pll,
> -					    &pll->config.hw_state);
> +		pll->on = pll->funcs.get_hw_state(dev_priv, pll,
> +						  &pll->config.hw_state);
>  		pll->active = 0;
>  		pll->config.crtc_mask = 0;
>  		for_each_intel_crtc(dev, crtc) {
> @@ -15602,7 +15602,7 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
>  
>  		DRM_DEBUG_KMS("%s enabled but not in use, disabling\n", pll->name);
>  
> -		pll->disable(dev_priv, pll);
> +		pll->funcs.disable(dev_priv, pll);
>  		pll->on = false;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index 889ceed..e88dc46 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -74,7 +74,7 @@ void assert_shared_dpll(struct drm_i915_private *dev_priv,
>  	if (WARN(!pll, "asserting DPLL %s with no DPLL\n", onoff(state)))
>  		return;
>  
> -	cur_state = pll->get_hw_state(dev_priv, pll, &hw_state);
> +	cur_state = pll->funcs.get_hw_state(dev_priv, pll, &hw_state);
>  	I915_STATE_WARN(cur_state != state,
>  	     "%s assertion failure (expected %s, current %s)\n",
>  			pll->name, onoff(state), onoff(cur_state));
> @@ -95,7 +95,7 @@ void intel_prepare_shared_dpll(struct intel_crtc *crtc)
>  		WARN_ON(pll->on);
>  		assert_shared_dpll_disabled(dev_priv, pll);
>  
> -		pll->mode_set(dev_priv, pll);
> +		pll->funcs.mode_set(dev_priv, pll);
>  	}
>  }
>  
> @@ -133,7 +133,7 @@ void intel_enable_shared_dpll(struct intel_crtc *crtc)
>  	intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
>  
>  	DRM_DEBUG_KMS("enabling %s\n", pll->name);
> -	pll->enable(dev_priv, pll);
> +	pll->funcs.enable(dev_priv, pll);
>  	pll->on = true;
>  }
>  
> @@ -168,7 +168,7 @@ void intel_disable_shared_dpll(struct intel_crtc *crtc)
>  		return;
>  
>  	DRM_DEBUG_KMS("disabling %s\n", pll->name);
> -	pll->disable(dev_priv, pll);
> +	pll->funcs.disable(dev_priv, pll);
>  	pll->on = false;
>  
>  	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
> @@ -398,29 +398,13 @@ static void ibx_pch_dpll_disable(struct drm_i915_private *dev_priv,
>  	udelay(200);
>  }
>  
> -static char *ibx_pch_dpll_names[] = {
> -	"PCH DPLL A",
> -	"PCH DPLL B",
> +static const struct intel_shared_dpll_funcs ibx_pch_dpll_funcs = {
> +	.mode_set = ibx_pch_dpll_mode_set,
> +	.enable = ibx_pch_dpll_enable,
> +	.disable = ibx_pch_dpll_disable,
> +	.get_hw_state = ibx_pch_dpll_get_hw_state,
>  };
>  
> -static void ibx_pch_dpll_init(struct drm_device *dev)
> -{
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	int i;
> -
> -	dev_priv->num_shared_dpll = 2;
> -
> -	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
> -		dev_priv->shared_dplls[i].id = i;
> -		dev_priv->shared_dplls[i].name = ibx_pch_dpll_names[i];
> -		dev_priv->shared_dplls[i].mode_set = ibx_pch_dpll_mode_set;
> -		dev_priv->shared_dplls[i].enable = ibx_pch_dpll_enable;
> -		dev_priv->shared_dplls[i].disable = ibx_pch_dpll_disable;
> -		dev_priv->shared_dplls[i].get_hw_state =
> -			ibx_pch_dpll_get_hw_state;
> -	}
> -}
> -
>  static void hsw_ddi_wrpll_enable(struct drm_i915_private *dev_priv,
>  			       struct intel_shared_dpll *pll)
>  {
> @@ -492,40 +476,16 @@ static bool hsw_ddi_spll_get_hw_state(struct drm_i915_private *dev_priv,
>  }
>  
>  
> -static const char * const hsw_ddi_pll_names[] = {
> -	"WRPLL 1",
> -	"WRPLL 2",
> -	"SPLL"
> +static const struct intel_shared_dpll_funcs hsw_ddi_wrpll_funcs = {
> +	.enable = hsw_ddi_wrpll_enable,
> +	.disable = hsw_ddi_wrpll_disable,
> +	.get_hw_state = hsw_ddi_wrpll_get_hw_state,
>  };
>  
> -static void hsw_shared_dplls_init(struct drm_i915_private *dev_priv)
> -{
> -	int i;
> -
> -	dev_priv->num_shared_dpll = 3;
> -
> -	for (i = 0; i < 2; i++) {
> -		dev_priv->shared_dplls[i].id = i;
> -		dev_priv->shared_dplls[i].name = hsw_ddi_pll_names[i];
> -		dev_priv->shared_dplls[i].disable = hsw_ddi_wrpll_disable;
> -		dev_priv->shared_dplls[i].enable = hsw_ddi_wrpll_enable;
> -		dev_priv->shared_dplls[i].get_hw_state =
> -			hsw_ddi_wrpll_get_hw_state;
> -	}
> -
> -	/* SPLL is special, but needs to be initialized anyway.. */
> -	dev_priv->shared_dplls[i].id = i;
> -	dev_priv->shared_dplls[i].name = hsw_ddi_pll_names[i];
> -	dev_priv->shared_dplls[i].disable = hsw_ddi_spll_disable;
> -	dev_priv->shared_dplls[i].enable = hsw_ddi_spll_enable;
> -	dev_priv->shared_dplls[i].get_hw_state = hsw_ddi_spll_get_hw_state;
> -
> -}
> -
> -static const char * const skl_ddi_pll_names[] = {
> -	"DPLL 1",
> -	"DPLL 2",
> -	"DPLL 3",
> +static const struct intel_shared_dpll_funcs hsw_ddi_spll_funcs = {
> +	.enable = hsw_ddi_spll_enable,
> +	.disable = hsw_ddi_spll_disable,
> +	.get_hw_state = hsw_ddi_spll_get_hw_state,
>  };
>  
>  struct skl_dpll_regs {
> @@ -634,26 +594,10 @@ out:
>  	return ret;
>  }
>  
> -static void skl_shared_dplls_init(struct drm_i915_private *dev_priv)
> -{
> -	int i;
> -
> -	dev_priv->num_shared_dpll = 3;
> -
> -	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
> -		dev_priv->shared_dplls[i].id = i;
> -		dev_priv->shared_dplls[i].name = skl_ddi_pll_names[i];
> -		dev_priv->shared_dplls[i].disable = skl_ddi_pll_disable;
> -		dev_priv->shared_dplls[i].enable = skl_ddi_pll_enable;
> -		dev_priv->shared_dplls[i].get_hw_state =
> -			skl_ddi_pll_get_hw_state;
> -	}
> -}
> -
> -static const char * const bxt_ddi_pll_names[] = {
> -	"PORT PLL A",
> -	"PORT PLL B",
> -	"PORT PLL C",
> +static const struct intel_shared_dpll_funcs skl_ddi_pll_funcs = {
> +	.enable = skl_ddi_pll_enable,
> +	.disable = skl_ddi_pll_disable,
> +	.get_hw_state = skl_ddi_pll_get_hw_state,
>  };
>  
>  static void bxt_ddi_pll_enable(struct drm_i915_private *dev_priv,
> @@ -838,34 +782,17 @@ out:
>  	return ret;
>  }
>  
> -static void bxt_shared_dplls_init(struct drm_i915_private *dev_priv)
> -{
> -	int i;
> -
> -	dev_priv->num_shared_dpll = 3;
> -
> -	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
> -		dev_priv->shared_dplls[i].id = i;
> -		dev_priv->shared_dplls[i].name = bxt_ddi_pll_names[i];
> -		dev_priv->shared_dplls[i].disable = bxt_ddi_pll_disable;
> -		dev_priv->shared_dplls[i].enable = bxt_ddi_pll_enable;
> -		dev_priv->shared_dplls[i].get_hw_state =
> -			bxt_ddi_pll_get_hw_state;
> -	}
> -}
> +static const struct intel_shared_dpll_funcs bxt_ddi_pll_funcs = {
> +	.enable = bxt_ddi_pll_enable,
> +	.disable = bxt_ddi_pll_disable,
> +	.get_hw_state = bxt_ddi_pll_get_hw_state,
> +};
>  
>  static void intel_ddi_pll_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	uint32_t val = I915_READ(LCPLL_CTL);
>  
> -	if (IS_SKYLAKE(dev) || IS_KABYLAKE(dev))
> -		skl_shared_dplls_init(dev_priv);
> -	else if (IS_BROXTON(dev))
> -		bxt_shared_dplls_init(dev_priv);
> -	else
> -		hsw_shared_dplls_init(dev_priv);
> -
>  	if (IS_SKYLAKE(dev) || IS_KABYLAKE(dev)) {
>  		int cdclk_freq;
>  
> @@ -893,16 +820,72 @@ static void intel_ddi_pll_init(struct drm_device *dev)
>  	}
>  }
>  
> +struct dpll_info {
> +	const char *name;
> +	const int id;
> +	const struct intel_shared_dpll_funcs *funcs;
> +};
> +
Seems shared_dplls[i].id == i, so that could be removed. If you also move name to funcs you could kill this struct.

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx