| Message ID | 1456494866-7665-8-git-send-email-ander.conselvan.de.oliveira@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
Hi Maarten, Thanks for reviewing. Comments below. On Wed, 2016-03-02 at 16:30 +0100, Maarten Lankhorst wrote: > 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. There are places in the code that assume those are equal. I considered not including the id, but concluded that having the id and a WARN_ON() in intel_shared_dpll_init() documents that assumption better. I think it would be better to remove that assumption, but that require changes to intel_atomic_state and users. But I think it would be nice if we could have a per-DPLL state object. > If you also move name to funcs you could kill this struct. In a later patch I add a flags field to this struct. I guess I could move that to funcs too, but then we need to come up with a better name for that struct. "funcs" starts to sound wrong. IMO having that extra struct is fine, so I rather let things settle first and then do another round of clean ups. But if that is a no-go, I can re-spin. Thanks, Ander
Op 03-03-16 om 12:32 schreef Ander Conselvan De Oliveira: > Hi Maarten, > > Thanks for reviewing. Comments below. > > On Wed, 2016-03-02 at 16:30 +0100, Maarten Lankhorst wrote: >> 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. > There are places in the code that assume those are equal. I considered not > including the id, but concluded that having the id and a WARN_ON() in > intel_shared_dpll_init() documents that assumption better. > > I think it would be better to remove that assumption, but that require changes > to intel_atomic_state and users. But I think it would be nice if we could have a > per-DPLL state object. Ok, I think the current way is fine then. >> If you also move name to funcs you could kill this struct. > In a later patch I add a flags field to this struct. I guess I could move that > to funcs too, but then we need to come up with a better name for that struct. > "funcs" starts to sound wrong. > > IMO having that extra struct is fine, so I rather let things settle first and > then do another round of clean ups. But if that is a no-go, I can re-spin. > I don't have a strong opinion, you can keep it like it is. Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
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; +}; + +static const struct dpll_info pch_plls[] = { + { "PCH DPLL A", DPLL_ID_PCH_PLL_A, &ibx_pch_dpll_funcs }, + { "PCH DPLL B", DPLL_ID_PCH_PLL_B, &ibx_pch_dpll_funcs }, + { NULL, -1, NULL }, +}; + +static const struct dpll_info hsw_plls[] = { + { "WRPLL 1", DPLL_ID_WRPLL1, &hsw_ddi_wrpll_funcs }, + { "WRPLL 2", DPLL_ID_WRPLL2, &hsw_ddi_wrpll_funcs }, + { "SPLL", DPLL_ID_SPLL, &hsw_ddi_spll_funcs }, + { NULL, -1, NULL, }, +}; + +static const struct dpll_info skl_plls[] = { + { "DPPL 1", DPLL_ID_SKL_DPLL1, &skl_ddi_pll_funcs }, + { "DPPL 2", DPLL_ID_SKL_DPLL2, &skl_ddi_pll_funcs }, + { "DPPL 3", DPLL_ID_SKL_DPLL3, &skl_ddi_pll_funcs }, + { NULL, -1, NULL, }, +}; + +static const struct dpll_info bxt_plls[] = { + { "PORT PLL A", 0, &bxt_ddi_pll_funcs }, + { "PORT PLL B", 1, &bxt_ddi_pll_funcs }, + { "PORT PLL C", 2, &bxt_ddi_pll_funcs }, + { NULL, -1, NULL, }, +}; + void intel_shared_dpll_init(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; + const struct dpll_info *dpll_info = NULL; + int i; - if (HAS_DDI(dev)) - intel_ddi_pll_init(dev); + if (IS_SKYLAKE(dev) || IS_KABYLAKE(dev)) + dpll_info = skl_plls; + else if IS_BROXTON(dev) + dpll_info = bxt_plls; + else if (HAS_DDI(dev)) + dpll_info = hsw_plls; else if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) - ibx_pch_dpll_init(dev); - else + dpll_info = pch_plls; + + if (!dpll_info) { dev_priv->num_shared_dpll = 0; + return; + } + + for (i = 0; dpll_info[i].id >= 0; i++) { + WARN_ON(i != dpll_info[i].id); + + dev_priv->shared_dplls[i].id = dpll_info[i].id; + dev_priv->shared_dplls[i].name = dpll_info[i].name; + dev_priv->shared_dplls[i].funcs = *dpll_info[i].funcs; + } + + dev_priv->num_shared_dpll = i; BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS); + + /* FIXME: Move this to a more suitable place */ + if (HAS_DDI(dev)) + intel_ddi_pll_init(dev); } diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h index a2ecf80..8be2478 100644 --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h @@ -28,6 +28,7 @@ struct drm_i915_private; struct intel_crtc; struct intel_crtc_state; +struct intel_shared_dpll; enum intel_dpll_id { DPLL_ID_PRIVATE = -1, /* non-shared dpll in use */ @@ -78,14 +79,7 @@ struct intel_shared_dpll_config { struct intel_dpll_hw_state hw_state; }; -struct intel_shared_dpll { - struct intel_shared_dpll_config config; - - int active; /* count of number of active CRTCs (i.e. DPMS on) */ - bool on; /* is the PLL actually active? Disabled during modeset */ - const char *name; - /* should match the index in the dev_priv->shared_dplls array */ - enum intel_dpll_id id; +struct intel_shared_dpll_funcs { /* The mode_set hook is optional and should be used together with the * intel_prepare_shared_dpll function. */ void (*mode_set)(struct drm_i915_private *dev_priv, @@ -99,6 +93,18 @@ struct intel_shared_dpll { struct intel_dpll_hw_state *hw_state); }; +struct intel_shared_dpll { + struct intel_shared_dpll_config config; + + int active; /* count of number of active CRTCs (i.e. DPMS on) */ + bool on; /* is the PLL actually active? Disabled during modeset */ + const char *name; + /* should match the index in the dev_priv->shared_dplls array */ + enum intel_dpll_id id; + + struct intel_shared_dpll_funcs funcs; +}; + #define SKL_DPLL0 0 #define SKL_DPLL1 1 #define SKL_DPLL2 2
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(-)