| Message ID | 1456494866-7665-9-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: > The function intel_get_shared_dpll() had a more or less generic > implementation with some platform specific checks to handle smaller > differences between platforms. However, the minimalist approach forces > bigger differences between platforms to be implemented outside of the > shared dpll code (see the *_ddi_pll_select() functions in intel_ddi.c, > for instance). > > This patch changes the implementation of intel_get_share_dpll() so that > a completely platform specific version can be used, providing helpers to > reduce code duplication. This should allow the code from the ddi pll > select functions to be moved, and also make room for making more dplls > managed by the shared dpll infrastructure. > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_dpll_mgr.c | 226 +++++++++++++++++++++------------- > drivers/gpu/drm/i915/intel_dpll_mgr.h | 2 + > 3 files changed, 145 insertions(+), 84 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 6de93dc..b858801 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1802,6 +1802,7 @@ struct drm_i915_private { > /* dpll and cdclk state is protected by connection_mutex */ > int num_shared_dpll; > struct intel_shared_dpll shared_dplls[I915_NUM_PLLS]; > + const struct intel_dpll_mgr *dpll_mgr; > > unsigned int active_crtcs; > unsigned int min_pixclk[I915_MAX_PIPES]; > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c > index e88dc46..3553324 100644 > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c > @@ -174,66 +174,20 @@ void intel_disable_shared_dpll(struct intel_crtc *crtc) > intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS); > } > > -static enum intel_dpll_id > -ibx_get_fixed_dpll(struct intel_crtc *crtc, > - struct intel_crtc_state *crtc_state) > -{ > - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > - struct intel_shared_dpll *pll; > - enum intel_dpll_id i; > - > - /* Ironlake PCH has a fixed PLL->PCH pipe mapping. */ > - i = (enum intel_dpll_id) crtc->pipe; > - pll = &dev_priv->shared_dplls[i]; > - > - DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n", > - crtc->base.base.id, pll->name); > - > - return i; > -} > - > -static enum intel_dpll_id > -bxt_get_fixed_dpll(struct intel_crtc *crtc, > - struct intel_crtc_state *crtc_state) > -{ > - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > - struct intel_encoder *encoder; > - struct intel_digital_port *intel_dig_port; > - struct intel_shared_dpll *pll; > - enum intel_dpll_id i; > - > - /* PLL is attached to port in bxt */ > - encoder = intel_ddi_get_crtc_new_encoder(crtc_state); > - if (WARN_ON(!encoder)) > - return DPLL_ID_PRIVATE; > - > - intel_dig_port = enc_to_dig_port(&encoder->base); > - /* 1:1 mapping between ports and PLLs */ > - i = (enum intel_dpll_id)intel_dig_port->port; > - pll = &dev_priv->shared_dplls[i]; > - DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n", > - crtc->base.base.id, pll->name); > - > - return i; > -} > - > -static enum intel_dpll_id > +static struct intel_shared_dpll * > intel_find_shared_dpll(struct intel_crtc *crtc, > - struct intel_crtc_state *crtc_state) > + struct intel_crtc_state *crtc_state, > + enum intel_dpll_id range_min, > + enum intel_dpll_id range_max) > { > struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; > struct intel_shared_dpll *pll; > struct intel_shared_dpll_config *shared_dpll; > enum intel_dpll_id i; > - int max = dev_priv->num_shared_dpll; > - > - if (INTEL_INFO(dev_priv)->gen < 9 && HAS_DDI(dev_priv)) > - /* Do not consider SPLL */ > - max = 2; > > shared_dpll = intel_atomic_get_shared_dpll_state(crtc_state->base.state); > > - for (i = 0; i < max; i++) { > + for (i = range_min; i <= range_max; i++) { > pll = &dev_priv->shared_dplls[i]; > > /* Only want to check enabled timings first */ > @@ -247,49 +201,33 @@ intel_find_shared_dpll(struct intel_crtc *crtc, > crtc->base.base.id, pll->name, > shared_dpll[i].crtc_mask, > pll->active); > - return i; > + return pll; > } > } > > /* Ok no matching timings, maybe there's a free one? */ > - for (i = 0; i < dev_priv->num_shared_dpll; i++) { > + for (i = range_min; i <= range_max; i++) { > pll = &dev_priv->shared_dplls[i]; > if (shared_dpll[i].crtc_mask == 0) { > DRM_DEBUG_KMS("CRTC:%d allocated %s\n", > crtc->base.base.id, pll->name); > - return i; > + return pll; > } > } > > - return DPLL_ID_PRIVATE; > + return NULL; > } > > -struct intel_shared_dpll * > -intel_get_shared_dpll(struct intel_crtc *crtc, > - struct intel_crtc_state *crtc_state) > +static void > +intel_reference_shared_dpll(struct intel_shared_dpll *pll, > + struct intel_crtc_state *crtc_state) > { > - struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; > - struct intel_shared_dpll *pll; > struct intel_shared_dpll_config *shared_dpll; > - enum intel_dpll_id i; > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); > + enum intel_dpll_id i = pll->id; > > shared_dpll = intel_atomic_get_shared_dpll_state(crtc_state->base.state); > > - if (HAS_PCH_IBX(dev_priv->dev)) { > - i = ibx_get_fixed_dpll(crtc, crtc_state); > - WARN_ON(shared_dpll[i].crtc_mask); > - } else if (IS_BROXTON(dev_priv->dev)) { > - i = bxt_get_fixed_dpll(crtc, crtc_state); > - WARN_ON(shared_dpll[i].crtc_mask); > - } else { > - i = intel_find_shared_dpll(crtc, crtc_state); > - } > - > - if (i < 0) > - return NULL; > - > - pll = &dev_priv->shared_dplls[i]; > - > if (shared_dpll[i].crtc_mask == 0) > shared_dpll[i].hw_state = > crtc_state->dpll_hw_state; > @@ -299,8 +237,6 @@ intel_get_shared_dpll(struct intel_crtc *crtc, > pipe_name(crtc->pipe)); > > intel_shared_dpll_config_get(shared_dpll, pll, crtc); > - > - return pll; > } > > void intel_shared_dpll_commit(struct drm_atomic_state *state) > @@ -398,6 +334,32 @@ static void ibx_pch_dpll_disable(struct drm_i915_private *dev_priv, > udelay(200); > } > > +static struct intel_shared_dpll * > +ibx_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) > +{ > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > + struct intel_shared_dpll *pll; > + enum intel_dpll_id i; > + > + if (HAS_PCH_IBX(dev_priv)) { > + /* Ironlake PCH has a fixed PLL->PCH pipe mapping. */ > + i = (enum intel_dpll_id) crtc->pipe; > + pll = &dev_priv->shared_dplls[i]; > + > + DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n", > + crtc->base.base.id, pll->name); > + } else { > + pll = intel_find_shared_dpll(crtc, crtc_state, > + DPLL_ID_PCH_PLL_A, > + DPLL_ID_PCH_PLL_B); > + } > + > + /* reference the pll */ > + intel_reference_shared_dpll(pll, crtc_state); > + > + return pll; > +} > + > static const struct intel_shared_dpll_funcs ibx_pch_dpll_funcs = { > .mode_set = ibx_pch_dpll_mode_set, > .enable = ibx_pch_dpll_enable, > @@ -475,6 +437,19 @@ static bool hsw_ddi_spll_get_hw_state(struct drm_i915_private *dev_priv, > return val & SPLL_PLL_ENABLE; > } > > +static struct intel_shared_dpll * > +hsw_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) > +{ > + struct intel_shared_dpll *pll; > + > + pll = intel_find_shared_dpll(crtc, crtc_state, > + DPLL_ID_WRPLL1, DPLL_ID_WRPLL2); > + if (pll) > + intel_reference_shared_dpll(pll, crtc_state); > + > + return pll; > +} > + > > static const struct intel_shared_dpll_funcs hsw_ddi_wrpll_funcs = { > .enable = hsw_ddi_wrpll_enable, > @@ -594,6 +569,19 @@ out: > return ret; > } > > +static struct intel_shared_dpll * > +skl_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) > +{ > + struct intel_shared_dpll *pll; > + > + pll = intel_find_shared_dpll(crtc, crtc_state, > + DPLL_ID_SKL_DPLL1, DPLL_ID_SKL_DPLL3); > + if (pll) > + intel_reference_shared_dpll(pll, crtc_state); > + > + return pll; > +} > + > static const struct intel_shared_dpll_funcs skl_ddi_pll_funcs = { > .enable = skl_ddi_pll_enable, > .disable = skl_ddi_pll_disable, > @@ -782,6 +770,32 @@ out: > return ret; > } > > +static struct intel_shared_dpll * > +bxt_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) > +{ > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > + struct intel_encoder *encoder; > + struct intel_digital_port *intel_dig_port; > + struct intel_shared_dpll *pll; > + enum intel_dpll_id i; > + > + /* PLL is attached to port in bxt */ > + encoder = intel_ddi_get_crtc_new_encoder(crtc_state); > + if (WARN_ON(!encoder)) > + return NULL; > + > + intel_dig_port = enc_to_dig_port(&encoder->base); > + /* 1:1 mapping between ports and PLLs */ > + i = (enum intel_dpll_id)intel_dig_port->port; > + pll = &dev_priv->shared_dplls[i]; > + DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n", > + crtc->base.base.id, pll->name); > + > + intel_reference_shared_dpll(pll, crtc_state); > + > + return pll; > +} > + > static const struct intel_shared_dpll_funcs bxt_ddi_pll_funcs = { > .enable = bxt_ddi_pll_enable, > .disable = bxt_ddi_pll_disable, > @@ -826,12 +840,24 @@ struct dpll_info { > const struct intel_shared_dpll_funcs *funcs; > }; > > +struct intel_dpll_mgr { > + const struct dpll_info *dpll_info; > + > + struct intel_shared_dpll *(*get_dpll)(struct intel_crtc *crtc, > + struct intel_crtc_state *crtc_state); > +}; > + > 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 intel_dpll_mgr pch_pll_mgr = { > + .dpll_info = pch_plls, > + .get_dpll = ibx_get_dpll, > +}; > + > 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 }, > @@ -839,6 +865,11 @@ static const struct dpll_info hsw_plls[] = { > { NULL, -1, NULL, }, > }; > > +static const struct intel_dpll_mgr hsw_pll_mgr = { > + .dpll_info = hsw_plls, > + .get_dpll = hsw_get_dpll, > +}; > + > 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 }, > @@ -846,6 +877,11 @@ static const struct dpll_info skl_plls[] = { > { NULL, -1, NULL, }, > }; > > +static const struct intel_dpll_mgr skl_pll_mgr = { > + .dpll_info = skl_plls, > + .get_dpll = skl_get_dpll, > +}; > + > static const struct dpll_info bxt_plls[] = { > { "PORT PLL A", 0, &bxt_ddi_pll_funcs }, > { "PORT PLL B", 1, &bxt_ddi_pll_funcs }, > @@ -853,26 +889,34 @@ static const struct dpll_info bxt_plls[] = { > { NULL, -1, NULL, }, > }; > > +static const struct intel_dpll_mgr bxt_pll_mgr = { > + .dpll_info = bxt_plls, > + .get_dpll = bxt_get_dpll, > +}; > + > 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; > + const struct intel_dpll_mgr *dpll_mgr = NULL; > + const struct dpll_info *dpll_info; > int i; > > if (IS_SKYLAKE(dev) || IS_KABYLAKE(dev)) > - dpll_info = skl_plls; > + dpll_mgr = &skl_pll_mgr; > else if IS_BROXTON(dev) > - dpll_info = bxt_plls; > + dpll_mgr = &bxt_pll_mgr; > else if (HAS_DDI(dev)) > - dpll_info = hsw_plls; > + dpll_mgr = &hsw_pll_mgr; > else if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) > - dpll_info = pch_plls; > + dpll_mgr = &pch_pll_mgr; > > - if (!dpll_info) { > + if (!dpll_mgr) { > dev_priv->num_shared_dpll = 0; > return; > } > > + dpll_info = dpll_mgr->dpll_info; > + > for (i = 0; dpll_info[i].id >= 0; i++) { > WARN_ON(i != dpll_info[i].id); > > @@ -881,6 +925,7 @@ void intel_shared_dpll_init(struct drm_device *dev) > dev_priv->shared_dplls[i].funcs = *dpll_info[i].funcs; > } > > + dev_priv->dpll_mgr = dpll_mgr; > dev_priv->num_shared_dpll = i; > > BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS); > @@ -889,3 +934,16 @@ void intel_shared_dpll_init(struct drm_device *dev) > if (HAS_DDI(dev)) > intel_ddi_pll_init(dev); > } > + > +struct intel_shared_dpll * > +intel_get_shared_dpll(struct intel_crtc *crtc, > + struct intel_crtc_state *crtc_state) > +{ > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > + const struct intel_dpll_mgr *dpll_mgr = dev_priv->dpll_mgr; > + > + if (dpll_mgr) > + return dpll_mgr->get_dpll(crtc, crtc_state); > + else > + return NULL; > +} should there be a WARN_ON here? Rest looks good, so for patch 8...11/13: Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
On Thu, 2016-03-03 at 15:08 +0100, Maarten Lankhorst wrote: > Op 26-02-16 om 14:54 schreef Ander Conselvan de Oliveira: > > The function intel_get_shared_dpll() had a more or less generic > > implementation with some platform specific checks to handle smaller > > differences between platforms. However, the minimalist approach forces > > bigger differences between platforms to be implemented outside of the > > shared dpll code (see the *_ddi_pll_select() functions in intel_ddi.c, > > for instance). > > > > This patch changes the implementation of intel_get_share_dpll() so that > > a completely platform specific version can be used, providing helpers to > > reduce code duplication. This should allow the code from the ddi pll > > select functions to be moved, and also make room for making more dplls > > managed by the shared dpll infrastructure. > > > > Signed-off-by: Ander Conselvan de Oliveira < > > ander.conselvan.de.oliveira@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > drivers/gpu/drm/i915/intel_dpll_mgr.c | 226 +++++++++++++++++++++---------- > > --- > > drivers/gpu/drm/i915/intel_dpll_mgr.h | 2 + > > 3 files changed, 145 insertions(+), 84 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h > > index 6de93dc..b858801 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1802,6 +1802,7 @@ struct drm_i915_private { > > /* dpll and cdclk state is protected by connection_mutex */ > > int num_shared_dpll; > > struct intel_shared_dpll shared_dplls[I915_NUM_PLLS]; > > + const struct intel_dpll_mgr *dpll_mgr; > > > > unsigned int active_crtcs; > > unsigned int min_pixclk[I915_MAX_PIPES]; > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c > > index e88dc46..3553324 100644 > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c > > @@ -174,66 +174,20 @@ void intel_disable_shared_dpll(struct intel_crtc > > *crtc) > > intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS); > > } > > > > -static enum intel_dpll_id > > -ibx_get_fixed_dpll(struct intel_crtc *crtc, > > - struct intel_crtc_state *crtc_state) > > -{ > > - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > - struct intel_shared_dpll *pll; > > - enum intel_dpll_id i; > > - > > - /* Ironlake PCH has a fixed PLL->PCH pipe mapping. */ > > - i = (enum intel_dpll_id) crtc->pipe; > > - pll = &dev_priv->shared_dplls[i]; > > - > > - DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n", > > - crtc->base.base.id, pll->name); > > - > > - return i; > > -} > > - > > -static enum intel_dpll_id > > -bxt_get_fixed_dpll(struct intel_crtc *crtc, > > - struct intel_crtc_state *crtc_state) > > -{ > > - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > - struct intel_encoder *encoder; > > - struct intel_digital_port *intel_dig_port; > > - struct intel_shared_dpll *pll; > > - enum intel_dpll_id i; > > - > > - /* PLL is attached to port in bxt */ > > - encoder = intel_ddi_get_crtc_new_encoder(crtc_state); > > - if (WARN_ON(!encoder)) > > - return DPLL_ID_PRIVATE; > > - > > - intel_dig_port = enc_to_dig_port(&encoder->base); > > - /* 1:1 mapping between ports and PLLs */ > > - i = (enum intel_dpll_id)intel_dig_port->port; > > - pll = &dev_priv->shared_dplls[i]; > > - DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n", > > - crtc->base.base.id, pll->name); > > - > > - return i; > > -} > > - > > -static enum intel_dpll_id > > +static struct intel_shared_dpll * > > intel_find_shared_dpll(struct intel_crtc *crtc, > > - struct intel_crtc_state *crtc_state) > > + struct intel_crtc_state *crtc_state, > > + enum intel_dpll_id range_min, > > + enum intel_dpll_id range_max) > > { > > struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; > > struct intel_shared_dpll *pll; > > struct intel_shared_dpll_config *shared_dpll; > > enum intel_dpll_id i; > > - int max = dev_priv->num_shared_dpll; > > - > > - if (INTEL_INFO(dev_priv)->gen < 9 && HAS_DDI(dev_priv)) > > - /* Do not consider SPLL */ > > - max = 2; > > > > shared_dpll = intel_atomic_get_shared_dpll_state(crtc_state > > ->base.state); > > > > - for (i = 0; i < max; i++) { > > + for (i = range_min; i <= range_max; i++) { > > pll = &dev_priv->shared_dplls[i]; > > > > /* Only want to check enabled timings first */ > > @@ -247,49 +201,33 @@ intel_find_shared_dpll(struct intel_crtc *crtc, > > crtc->base.base.id, pll->name, > > shared_dpll[i].crtc_mask, > > pll->active); > > - return i; > > + return pll; > > } > > } > > > > /* Ok no matching timings, maybe there's a free one? */ > > - for (i = 0; i < dev_priv->num_shared_dpll; i++) { > > + for (i = range_min; i <= range_max; i++) { > > pll = &dev_priv->shared_dplls[i]; > > if (shared_dpll[i].crtc_mask == 0) { > > DRM_DEBUG_KMS("CRTC:%d allocated %s\n", > > crtc->base.base.id, pll->name); > > - return i; > > + return pll; > > } > > } > > > > - return DPLL_ID_PRIVATE; > > + return NULL; > > } > > > > -struct intel_shared_dpll * > > -intel_get_shared_dpll(struct intel_crtc *crtc, > > - struct intel_crtc_state *crtc_state) > > +static void > > +intel_reference_shared_dpll(struct intel_shared_dpll *pll, > > + struct intel_crtc_state *crtc_state) > > { > > - struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; > > - struct intel_shared_dpll *pll; > > struct intel_shared_dpll_config *shared_dpll; > > - enum intel_dpll_id i; > > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); > > + enum intel_dpll_id i = pll->id; > > > > shared_dpll = intel_atomic_get_shared_dpll_state(crtc_state > > ->base.state); > > > > - if (HAS_PCH_IBX(dev_priv->dev)) { > > - i = ibx_get_fixed_dpll(crtc, crtc_state); > > - WARN_ON(shared_dpll[i].crtc_mask); > > - } else if (IS_BROXTON(dev_priv->dev)) { > > - i = bxt_get_fixed_dpll(crtc, crtc_state); > > - WARN_ON(shared_dpll[i].crtc_mask); > > - } else { > > - i = intel_find_shared_dpll(crtc, crtc_state); > > - } > > - > > - if (i < 0) > > - return NULL; > > - > > - pll = &dev_priv->shared_dplls[i]; > > - > > if (shared_dpll[i].crtc_mask == 0) > > shared_dpll[i].hw_state = > > crtc_state->dpll_hw_state; > > @@ -299,8 +237,6 @@ intel_get_shared_dpll(struct intel_crtc *crtc, > > pipe_name(crtc->pipe)); > > > > intel_shared_dpll_config_get(shared_dpll, pll, crtc); > > - > > - return pll; > > } > > > > void intel_shared_dpll_commit(struct drm_atomic_state *state) > > @@ -398,6 +334,32 @@ static void ibx_pch_dpll_disable(struct > > drm_i915_private *dev_priv, > > udelay(200); > > } > > > > +static struct intel_shared_dpll * > > +ibx_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > + struct intel_shared_dpll *pll; > > + enum intel_dpll_id i; > > + > > + if (HAS_PCH_IBX(dev_priv)) { > > + /* Ironlake PCH has a fixed PLL->PCH pipe mapping. */ > > + i = (enum intel_dpll_id) crtc->pipe; > > + pll = &dev_priv->shared_dplls[i]; > > + > > + DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n", > > + crtc->base.base.id, pll->name); > > + } else { > > + pll = intel_find_shared_dpll(crtc, crtc_state, > > + DPLL_ID_PCH_PLL_A, > > + DPLL_ID_PCH_PLL_B); > > + } > > + > > + /* reference the pll */ > > + intel_reference_shared_dpll(pll, crtc_state); > > + > > + return pll; > > +} > > + > > static const struct intel_shared_dpll_funcs ibx_pch_dpll_funcs = { > > .mode_set = ibx_pch_dpll_mode_set, > > .enable = ibx_pch_dpll_enable, > > @@ -475,6 +437,19 @@ static bool hsw_ddi_spll_get_hw_state(struct > > drm_i915_private *dev_priv, > > return val & SPLL_PLL_ENABLE; > > } > > > > +static struct intel_shared_dpll * > > +hsw_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) > > +{ > > + struct intel_shared_dpll *pll; > > + > > + pll = intel_find_shared_dpll(crtc, crtc_state, > > + DPLL_ID_WRPLL1, DPLL_ID_WRPLL2); > > + if (pll) > > + intel_reference_shared_dpll(pll, crtc_state); > > + > > + return pll; > > +} > > + > > > > static const struct intel_shared_dpll_funcs hsw_ddi_wrpll_funcs = { > > .enable = hsw_ddi_wrpll_enable, > > @@ -594,6 +569,19 @@ out: > > return ret; > > } > > > > +static struct intel_shared_dpll * > > +skl_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) > > +{ > > + struct intel_shared_dpll *pll; > > + > > + pll = intel_find_shared_dpll(crtc, crtc_state, > > + DPLL_ID_SKL_DPLL1, DPLL_ID_SKL_DPLL3); > > + if (pll) > > + intel_reference_shared_dpll(pll, crtc_state); > > + > > + return pll; > > +} > > + > > static const struct intel_shared_dpll_funcs skl_ddi_pll_funcs = { > > .enable = skl_ddi_pll_enable, > > .disable = skl_ddi_pll_disable, > > @@ -782,6 +770,32 @@ out: > > return ret; > > } > > > > +static struct intel_shared_dpll * > > +bxt_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > + struct intel_encoder *encoder; > > + struct intel_digital_port *intel_dig_port; > > + struct intel_shared_dpll *pll; > > + enum intel_dpll_id i; > > + > > + /* PLL is attached to port in bxt */ > > + encoder = intel_ddi_get_crtc_new_encoder(crtc_state); > > + if (WARN_ON(!encoder)) > > + return NULL; > > + > > + intel_dig_port = enc_to_dig_port(&encoder->base); > > + /* 1:1 mapping between ports and PLLs */ > > + i = (enum intel_dpll_id)intel_dig_port->port; > > + pll = &dev_priv->shared_dplls[i]; > > + DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n", > > + crtc->base.base.id, pll->name); > > + > > + intel_reference_shared_dpll(pll, crtc_state); > > + > > + return pll; > > +} > > + > > static const struct intel_shared_dpll_funcs bxt_ddi_pll_funcs = { > > .enable = bxt_ddi_pll_enable, > > .disable = bxt_ddi_pll_disable, > > @@ -826,12 +840,24 @@ struct dpll_info { > > const struct intel_shared_dpll_funcs *funcs; > > }; > > > > +struct intel_dpll_mgr { > > + const struct dpll_info *dpll_info; > > + > > + struct intel_shared_dpll *(*get_dpll)(struct intel_crtc *crtc, > > + struct intel_crtc_state > > *crtc_state); > > +}; > > + > > 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 intel_dpll_mgr pch_pll_mgr = { > > + .dpll_info = pch_plls, > > + .get_dpll = ibx_get_dpll, > > +}; > > + > > 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 }, > > @@ -839,6 +865,11 @@ static const struct dpll_info hsw_plls[] = { > > { NULL, -1, NULL, }, > > }; > > > > +static const struct intel_dpll_mgr hsw_pll_mgr = { > > + .dpll_info = hsw_plls, > > + .get_dpll = hsw_get_dpll, > > +}; > > + > > 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 }, > > @@ -846,6 +877,11 @@ static const struct dpll_info skl_plls[] = { > > { NULL, -1, NULL, }, > > }; > > > > +static const struct intel_dpll_mgr skl_pll_mgr = { > > + .dpll_info = skl_plls, > > + .get_dpll = skl_get_dpll, > > +}; > > + > > static const struct dpll_info bxt_plls[] = { > > { "PORT PLL A", 0, &bxt_ddi_pll_funcs }, > > { "PORT PLL B", 1, &bxt_ddi_pll_funcs }, > > @@ -853,26 +889,34 @@ static const struct dpll_info bxt_plls[] = { > > { NULL, -1, NULL, }, > > }; > > > > +static const struct intel_dpll_mgr bxt_pll_mgr = { > > + .dpll_info = bxt_plls, > > + .get_dpll = bxt_get_dpll, > > +}; > > + > > 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; > > + const struct intel_dpll_mgr *dpll_mgr = NULL; > > + const struct dpll_info *dpll_info; > > int i; > > > > if (IS_SKYLAKE(dev) || IS_KABYLAKE(dev)) > > - dpll_info = skl_plls; > > + dpll_mgr = &skl_pll_mgr; > > else if IS_BROXTON(dev) > > - dpll_info = bxt_plls; > > + dpll_mgr = &bxt_pll_mgr; > > else if (HAS_DDI(dev)) > > - dpll_info = hsw_plls; > > + dpll_mgr = &hsw_pll_mgr; > > else if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) > > - dpll_info = pch_plls; > > + dpll_mgr = &pch_pll_mgr; > > > > - if (!dpll_info) { > > + if (!dpll_mgr) { > > dev_priv->num_shared_dpll = 0; > > return; > > } > > > > + dpll_info = dpll_mgr->dpll_info; > > + > > for (i = 0; dpll_info[i].id >= 0; i++) { > > WARN_ON(i != dpll_info[i].id); > > > > @@ -881,6 +925,7 @@ void intel_shared_dpll_init(struct drm_device *dev) > > dev_priv->shared_dplls[i].funcs = *dpll_info[i].funcs; > > } > > > > + dev_priv->dpll_mgr = dpll_mgr; > > dev_priv->num_shared_dpll = i; > > > > BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS); > > @@ -889,3 +934,16 @@ void intel_shared_dpll_init(struct drm_device *dev) > > if (HAS_DDI(dev)) > > intel_ddi_pll_init(dev); > > } > > + > > +struct intel_shared_dpll * > > +intel_get_shared_dpll(struct intel_crtc *crtc, > > + struct intel_crtc_state *crtc_state) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > + const struct intel_dpll_mgr *dpll_mgr = dev_priv->dpll_mgr; > > + > > + if (dpll_mgr) > > + return dpll_mgr->get_dpll(crtc, crtc_state); > > + else > > + return NULL; > > +} > should there be a WARN_ON here? > Makes sense. I'll send v2. Thanks, Ander > Rest looks good, so for patch 8...11/13: > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, 2016-03-03 at 15:08 +0100, Maarten Lankhorst wrote: > Op 26-02-16 om 14:54 schreef Ander Conselvan de Oliveira: > > The function intel_get_shared_dpll() had a more or less generic > > implementation with some platform specific checks to handle smaller > > differences between platforms. However, the minimalist approach forces > > bigger differences between platforms to be implemented outside of the > > shared dpll code (see the *_ddi_pll_select() functions in intel_ddi.c, > > for instance). > > > > This patch changes the implementation of intel_get_share_dpll() so that > > a completely platform specific version can be used, providing helpers to > > reduce code duplication. This should allow the code from the ddi pll > > select functions to be moved, and also make room for making more dplls > > managed by the shared dpll infrastructure. > > > > Signed-off-by: Ander Conselvan de Oliveira < > > ander.conselvan.de.oliveira@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > drivers/gpu/drm/i915/intel_dpll_mgr.c | 226 +++++++++++++++++++++---------- > > --- > > drivers/gpu/drm/i915/intel_dpll_mgr.h | 2 + > > 3 files changed, 145 insertions(+), 84 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h > > index 6de93dc..b858801 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1802,6 +1802,7 @@ struct drm_i915_private { > > /* dpll and cdclk state is protected by connection_mutex */ > > int num_shared_dpll; > > struct intel_shared_dpll shared_dplls[I915_NUM_PLLS]; > > + const struct intel_dpll_mgr *dpll_mgr; > > > > unsigned int active_crtcs; > > unsigned int min_pixclk[I915_MAX_PIPES]; > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c > > index e88dc46..3553324 100644 > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c > > @@ -174,66 +174,20 @@ void intel_disable_shared_dpll(struct intel_crtc > > *crtc) > > intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS); > > } > > > > -static enum intel_dpll_id > > -ibx_get_fixed_dpll(struct intel_crtc *crtc, > > - struct intel_crtc_state *crtc_state) > > -{ > > - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > - struct intel_shared_dpll *pll; > > - enum intel_dpll_id i; > > - > > - /* Ironlake PCH has a fixed PLL->PCH pipe mapping. */ > > - i = (enum intel_dpll_id) crtc->pipe; > > - pll = &dev_priv->shared_dplls[i]; > > - > > - DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n", > > - crtc->base.base.id, pll->name); > > - > > - return i; > > -} > > - > > -static enum intel_dpll_id > > -bxt_get_fixed_dpll(struct intel_crtc *crtc, > > - struct intel_crtc_state *crtc_state) > > -{ > > - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > - struct intel_encoder *encoder; > > - struct intel_digital_port *intel_dig_port; > > - struct intel_shared_dpll *pll; > > - enum intel_dpll_id i; > > - > > - /* PLL is attached to port in bxt */ > > - encoder = intel_ddi_get_crtc_new_encoder(crtc_state); > > - if (WARN_ON(!encoder)) > > - return DPLL_ID_PRIVATE; > > - > > - intel_dig_port = enc_to_dig_port(&encoder->base); > > - /* 1:1 mapping between ports and PLLs */ > > - i = (enum intel_dpll_id)intel_dig_port->port; > > - pll = &dev_priv->shared_dplls[i]; > > - DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n", > > - crtc->base.base.id, pll->name); > > - > > - return i; > > -} > > - > > -static enum intel_dpll_id > > +static struct intel_shared_dpll * > > intel_find_shared_dpll(struct intel_crtc *crtc, > > - struct intel_crtc_state *crtc_state) > > + struct intel_crtc_state *crtc_state, > > + enum intel_dpll_id range_min, > > + enum intel_dpll_id range_max) > > { > > struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; > > struct intel_shared_dpll *pll; > > struct intel_shared_dpll_config *shared_dpll; > > enum intel_dpll_id i; > > - int max = dev_priv->num_shared_dpll; > > - > > - if (INTEL_INFO(dev_priv)->gen < 9 && HAS_DDI(dev_priv)) > > - /* Do not consider SPLL */ > > - max = 2; > > > > shared_dpll = intel_atomic_get_shared_dpll_state(crtc_state > > ->base.state); > > > > - for (i = 0; i < max; i++) { > > + for (i = range_min; i <= range_max; i++) { > > pll = &dev_priv->shared_dplls[i]; > > > > /* Only want to check enabled timings first */ > > @@ -247,49 +201,33 @@ intel_find_shared_dpll(struct intel_crtc *crtc, > > crtc->base.base.id, pll->name, > > shared_dpll[i].crtc_mask, > > pll->active); > > - return i; > > + return pll; > > } > > } > > > > /* Ok no matching timings, maybe there's a free one? */ > > - for (i = 0; i < dev_priv->num_shared_dpll; i++) { > > + for (i = range_min; i <= range_max; i++) { > > pll = &dev_priv->shared_dplls[i]; > > if (shared_dpll[i].crtc_mask == 0) { > > DRM_DEBUG_KMS("CRTC:%d allocated %s\n", > > crtc->base.base.id, pll->name); > > - return i; > > + return pll; > > } > > } > > > > - return DPLL_ID_PRIVATE; > > + return NULL; > > } > > > > -struct intel_shared_dpll * > > -intel_get_shared_dpll(struct intel_crtc *crtc, > > - struct intel_crtc_state *crtc_state) > > +static void > > +intel_reference_shared_dpll(struct intel_shared_dpll *pll, > > + struct intel_crtc_state *crtc_state) > > { > > - struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; > > - struct intel_shared_dpll *pll; > > struct intel_shared_dpll_config *shared_dpll; > > - enum intel_dpll_id i; > > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); > > + enum intel_dpll_id i = pll->id; > > > > shared_dpll = intel_atomic_get_shared_dpll_state(crtc_state > > ->base.state); > > > > - if (HAS_PCH_IBX(dev_priv->dev)) { > > - i = ibx_get_fixed_dpll(crtc, crtc_state); > > - WARN_ON(shared_dpll[i].crtc_mask); > > - } else if (IS_BROXTON(dev_priv->dev)) { > > - i = bxt_get_fixed_dpll(crtc, crtc_state); > > - WARN_ON(shared_dpll[i].crtc_mask); > > - } else { > > - i = intel_find_shared_dpll(crtc, crtc_state); > > - } > > - > > - if (i < 0) > > - return NULL; > > - > > - pll = &dev_priv->shared_dplls[i]; > > - > > if (shared_dpll[i].crtc_mask == 0) > > shared_dpll[i].hw_state = > > crtc_state->dpll_hw_state; > > @@ -299,8 +237,6 @@ intel_get_shared_dpll(struct intel_crtc *crtc, > > pipe_name(crtc->pipe)); > > > > intel_shared_dpll_config_get(shared_dpll, pll, crtc); > > - > > - return pll; > > } > > > > void intel_shared_dpll_commit(struct drm_atomic_state *state) > > @@ -398,6 +334,32 @@ static void ibx_pch_dpll_disable(struct > > drm_i915_private *dev_priv, > > udelay(200); > > } > > > > +static struct intel_shared_dpll * > > +ibx_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > + struct intel_shared_dpll *pll; > > + enum intel_dpll_id i; > > + > > + if (HAS_PCH_IBX(dev_priv)) { > > + /* Ironlake PCH has a fixed PLL->PCH pipe mapping. */ > > + i = (enum intel_dpll_id) crtc->pipe; > > + pll = &dev_priv->shared_dplls[i]; > > + > > + DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n", > > + crtc->base.base.id, pll->name); > > + } else { > > + pll = intel_find_shared_dpll(crtc, crtc_state, > > + DPLL_ID_PCH_PLL_A, > > + DPLL_ID_PCH_PLL_B); > > + } > > + > > + /* reference the pll */ > > + intel_reference_shared_dpll(pll, crtc_state); > > + > > + return pll; > > +} > > + > > static const struct intel_shared_dpll_funcs ibx_pch_dpll_funcs = { > > .mode_set = ibx_pch_dpll_mode_set, > > .enable = ibx_pch_dpll_enable, > > @@ -475,6 +437,19 @@ static bool hsw_ddi_spll_get_hw_state(struct > > drm_i915_private *dev_priv, > > return val & SPLL_PLL_ENABLE; > > } > > > > +static struct intel_shared_dpll * > > +hsw_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) > > +{ > > + struct intel_shared_dpll *pll; > > + > > + pll = intel_find_shared_dpll(crtc, crtc_state, > > + DPLL_ID_WRPLL1, DPLL_ID_WRPLL2); > > + if (pll) > > + intel_reference_shared_dpll(pll, crtc_state); > > + > > + return pll; > > +} > > + > > > > static const struct intel_shared_dpll_funcs hsw_ddi_wrpll_funcs = { > > .enable = hsw_ddi_wrpll_enable, > > @@ -594,6 +569,19 @@ out: > > return ret; > > } > > > > +static struct intel_shared_dpll * > > +skl_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) > > +{ > > + struct intel_shared_dpll *pll; > > + > > + pll = intel_find_shared_dpll(crtc, crtc_state, > > + DPLL_ID_SKL_DPLL1, DPLL_ID_SKL_DPLL3); > > + if (pll) > > + intel_reference_shared_dpll(pll, crtc_state); > > + > > + return pll; > > +} > > + > > static const struct intel_shared_dpll_funcs skl_ddi_pll_funcs = { > > .enable = skl_ddi_pll_enable, > > .disable = skl_ddi_pll_disable, > > @@ -782,6 +770,32 @@ out: > > return ret; > > } > > > > +static struct intel_shared_dpll * > > +bxt_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > + struct intel_encoder *encoder; > > + struct intel_digital_port *intel_dig_port; > > + struct intel_shared_dpll *pll; > > + enum intel_dpll_id i; > > + > > + /* PLL is attached to port in bxt */ > > + encoder = intel_ddi_get_crtc_new_encoder(crtc_state); > > + if (WARN_ON(!encoder)) > > + return NULL; > > + > > + intel_dig_port = enc_to_dig_port(&encoder->base); > > + /* 1:1 mapping between ports and PLLs */ > > + i = (enum intel_dpll_id)intel_dig_port->port; > > + pll = &dev_priv->shared_dplls[i]; > > + DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n", > > + crtc->base.base.id, pll->name); > > + > > + intel_reference_shared_dpll(pll, crtc_state); > > + > > + return pll; > > +} > > + > > static const struct intel_shared_dpll_funcs bxt_ddi_pll_funcs = { > > .enable = bxt_ddi_pll_enable, > > .disable = bxt_ddi_pll_disable, > > @@ -826,12 +840,24 @@ struct dpll_info { > > const struct intel_shared_dpll_funcs *funcs; > > }; > > > > +struct intel_dpll_mgr { > > + const struct dpll_info *dpll_info; > > + > > + struct intel_shared_dpll *(*get_dpll)(struct intel_crtc *crtc, > > + struct intel_crtc_state > > *crtc_state); > > +}; > > + > > 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 intel_dpll_mgr pch_pll_mgr = { > > + .dpll_info = pch_plls, > > + .get_dpll = ibx_get_dpll, > > +}; > > + > > 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 }, > > @@ -839,6 +865,11 @@ static const struct dpll_info hsw_plls[] = { > > { NULL, -1, NULL, }, > > }; > > > > +static const struct intel_dpll_mgr hsw_pll_mgr = { > > + .dpll_info = hsw_plls, > > + .get_dpll = hsw_get_dpll, > > +}; > > + > > 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 }, > > @@ -846,6 +877,11 @@ static const struct dpll_info skl_plls[] = { > > { NULL, -1, NULL, }, > > }; > > > > +static const struct intel_dpll_mgr skl_pll_mgr = { > > + .dpll_info = skl_plls, > > + .get_dpll = skl_get_dpll, > > +}; > > + > > static const struct dpll_info bxt_plls[] = { > > { "PORT PLL A", 0, &bxt_ddi_pll_funcs }, > > { "PORT PLL B", 1, &bxt_ddi_pll_funcs }, > > @@ -853,26 +889,34 @@ static const struct dpll_info bxt_plls[] = { > > { NULL, -1, NULL, }, > > }; > > > > +static const struct intel_dpll_mgr bxt_pll_mgr = { > > + .dpll_info = bxt_plls, > > + .get_dpll = bxt_get_dpll, > > +}; > > + > > 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; > > + const struct intel_dpll_mgr *dpll_mgr = NULL; > > + const struct dpll_info *dpll_info; > > int i; > > > > if (IS_SKYLAKE(dev) || IS_KABYLAKE(dev)) > > - dpll_info = skl_plls; > > + dpll_mgr = &skl_pll_mgr; > > else if IS_BROXTON(dev) > > - dpll_info = bxt_plls; > > + dpll_mgr = &bxt_pll_mgr; > > else if (HAS_DDI(dev)) > > - dpll_info = hsw_plls; > > + dpll_mgr = &hsw_pll_mgr; > > else if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) > > - dpll_info = pch_plls; > > + dpll_mgr = &pch_pll_mgr; > > > > - if (!dpll_info) { > > + if (!dpll_mgr) { > > dev_priv->num_shared_dpll = 0; > > return; > > } > > > > + dpll_info = dpll_mgr->dpll_info; > > + > > for (i = 0; dpll_info[i].id >= 0; i++) { > > WARN_ON(i != dpll_info[i].id); > > > > @@ -881,6 +925,7 @@ void intel_shared_dpll_init(struct drm_device *dev) > > dev_priv->shared_dplls[i].funcs = *dpll_info[i].funcs; > > } > > > > + dev_priv->dpll_mgr = dpll_mgr; > > dev_priv->num_shared_dpll = i; > > > > BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS); > > @@ -889,3 +934,16 @@ void intel_shared_dpll_init(struct drm_device *dev) > > if (HAS_DDI(dev)) > > intel_ddi_pll_init(dev); > > } > > + > > +struct intel_shared_dpll * > > +intel_get_shared_dpll(struct intel_crtc *crtc, > > + struct intel_crtc_state *crtc_state) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > + const struct intel_dpll_mgr *dpll_mgr = dev_priv->dpll_mgr; > > + > > + if (dpll_mgr) > > + return dpll_mgr->get_dpll(crtc, crtc_state); > > + else > > + return NULL; > > +} > should there be a WARN_ON here? > > Rest looks good, so for patch 8...11/13: Did you have comments on patch 12? I seem to have missed it. Ander
Op 04-03-16 om 07:49 schreef Ander Conselvan De Oliveira: > On Thu, 2016-03-03 at 15:08 +0100, Maarten Lankhorst wrote: >> Op 26-02-16 om 14:54 schreef Ander Conselvan de Oliveira: >>> The function intel_get_shared_dpll() had a more or less generic >>> implementation with some platform specific checks to handle smaller >>> differences between platforms. However, the minimalist approach forces >>> bigger differences between platforms to be implemented outside of the >>> shared dpll code (see the *_ddi_pll_select() functions in intel_ddi.c, >>> for instance). >>> >>> This patch changes the implementation of intel_get_share_dpll() so that >>> a completely platform specific version can be used, providing helpers to >>> reduce code duplication. This should allow the code from the ddi pll >>> select functions to be moved, and also make room for making more dplls >>> managed by the shared dpll infrastructure. >>> >>> Signed-off-by: Ander Conselvan de Oliveira < >>> ander.conselvan.de.oliveira@intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_drv.h | 1 + >>> drivers/gpu/drm/i915/intel_dpll_mgr.c | 226 +++++++++++++++++++++---------- >>> --- >>> drivers/gpu/drm/i915/intel_dpll_mgr.h | 2 + >>> 3 files changed, 145 insertions(+), 84 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_drv.h >>> b/drivers/gpu/drm/i915/i915_drv.h >>> index 6de93dc..b858801 100644 >>> --- a/drivers/gpu/drm/i915/i915_drv.h >>> +++ b/drivers/gpu/drm/i915/i915_drv.h >>> @@ -1802,6 +1802,7 @@ struct drm_i915_private { >>> /* dpll and cdclk state is protected by connection_mutex */ >>> int num_shared_dpll; >>> struct intel_shared_dpll shared_dplls[I915_NUM_PLLS]; >>> + const struct intel_dpll_mgr *dpll_mgr; >>> >>> unsigned int active_crtcs; >>> unsigned int min_pixclk[I915_MAX_PIPES]; >>> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c >>> b/drivers/gpu/drm/i915/intel_dpll_mgr.c >>> index e88dc46..3553324 100644 >>> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c >>> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c >>> @@ -174,66 +174,20 @@ void intel_disable_shared_dpll(struct intel_crtc >>> *crtc) >>> intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS); >>> } >>> >>> -static enum intel_dpll_id >>> -ibx_get_fixed_dpll(struct intel_crtc *crtc, >>> - struct intel_crtc_state *crtc_state) >>> -{ >>> - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >>> - struct intel_shared_dpll *pll; >>> - enum intel_dpll_id i; >>> - >>> - /* Ironlake PCH has a fixed PLL->PCH pipe mapping. */ >>> - i = (enum intel_dpll_id) crtc->pipe; >>> - pll = &dev_priv->shared_dplls[i]; >>> - >>> - DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n", >>> - crtc->base.base.id, pll->name); >>> - >>> - return i; >>> -} >>> - >>> -static enum intel_dpll_id >>> -bxt_get_fixed_dpll(struct intel_crtc *crtc, >>> - struct intel_crtc_state *crtc_state) >>> -{ >>> - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >>> - struct intel_encoder *encoder; >>> - struct intel_digital_port *intel_dig_port; >>> - struct intel_shared_dpll *pll; >>> - enum intel_dpll_id i; >>> - >>> - /* PLL is attached to port in bxt */ >>> - encoder = intel_ddi_get_crtc_new_encoder(crtc_state); >>> - if (WARN_ON(!encoder)) >>> - return DPLL_ID_PRIVATE; >>> - >>> - intel_dig_port = enc_to_dig_port(&encoder->base); >>> - /* 1:1 mapping between ports and PLLs */ >>> - i = (enum intel_dpll_id)intel_dig_port->port; >>> - pll = &dev_priv->shared_dplls[i]; >>> - DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n", >>> - crtc->base.base.id, pll->name); >>> - >>> - return i; >>> -} >>> - >>> -static enum intel_dpll_id >>> +static struct intel_shared_dpll * >>> intel_find_shared_dpll(struct intel_crtc *crtc, >>> - struct intel_crtc_state *crtc_state) >>> + struct intel_crtc_state *crtc_state, >>> + enum intel_dpll_id range_min, >>> + enum intel_dpll_id range_max) >>> { >>> struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; >>> struct intel_shared_dpll *pll; >>> struct intel_shared_dpll_config *shared_dpll; >>> enum intel_dpll_id i; >>> - int max = dev_priv->num_shared_dpll; >>> - >>> - if (INTEL_INFO(dev_priv)->gen < 9 && HAS_DDI(dev_priv)) >>> - /* Do not consider SPLL */ >>> - max = 2; >>> >>> shared_dpll = intel_atomic_get_shared_dpll_state(crtc_state >>> ->base.state); >>> >>> - for (i = 0; i < max; i++) { >>> + for (i = range_min; i <= range_max; i++) { >>> pll = &dev_priv->shared_dplls[i]; >>> >>> /* Only want to check enabled timings first */ >>> @@ -247,49 +201,33 @@ intel_find_shared_dpll(struct intel_crtc *crtc, >>> crtc->base.base.id, pll->name, >>> shared_dpll[i].crtc_mask, >>> pll->active); >>> - return i; >>> + return pll; >>> } >>> } >>> >>> /* Ok no matching timings, maybe there's a free one? */ >>> - for (i = 0; i < dev_priv->num_shared_dpll; i++) { >>> + for (i = range_min; i <= range_max; i++) { >>> pll = &dev_priv->shared_dplls[i]; >>> if (shared_dpll[i].crtc_mask == 0) { >>> DRM_DEBUG_KMS("CRTC:%d allocated %s\n", >>> crtc->base.base.id, pll->name); >>> - return i; >>> + return pll; >>> } >>> } >>> >>> - return DPLL_ID_PRIVATE; >>> + return NULL; >>> } >>> >>> -struct intel_shared_dpll * >>> -intel_get_shared_dpll(struct intel_crtc *crtc, >>> - struct intel_crtc_state *crtc_state) >>> +static void >>> +intel_reference_shared_dpll(struct intel_shared_dpll *pll, >>> + struct intel_crtc_state *crtc_state) >>> { >>> - struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; >>> - struct intel_shared_dpll *pll; >>> struct intel_shared_dpll_config *shared_dpll; >>> - enum intel_dpll_id i; >>> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); >>> + enum intel_dpll_id i = pll->id; >>> >>> shared_dpll = intel_atomic_get_shared_dpll_state(crtc_state >>> ->base.state); >>> >>> - if (HAS_PCH_IBX(dev_priv->dev)) { >>> - i = ibx_get_fixed_dpll(crtc, crtc_state); >>> - WARN_ON(shared_dpll[i].crtc_mask); >>> - } else if (IS_BROXTON(dev_priv->dev)) { >>> - i = bxt_get_fixed_dpll(crtc, crtc_state); >>> - WARN_ON(shared_dpll[i].crtc_mask); >>> - } else { >>> - i = intel_find_shared_dpll(crtc, crtc_state); >>> - } >>> - >>> - if (i < 0) >>> - return NULL; >>> - >>> - pll = &dev_priv->shared_dplls[i]; >>> - >>> if (shared_dpll[i].crtc_mask == 0) >>> shared_dpll[i].hw_state = >>> crtc_state->dpll_hw_state; >>> @@ -299,8 +237,6 @@ intel_get_shared_dpll(struct intel_crtc *crtc, >>> pipe_name(crtc->pipe)); >>> >>> intel_shared_dpll_config_get(shared_dpll, pll, crtc); >>> - >>> - return pll; >>> } >>> >>> void intel_shared_dpll_commit(struct drm_atomic_state *state) >>> @@ -398,6 +334,32 @@ static void ibx_pch_dpll_disable(struct >>> drm_i915_private *dev_priv, >>> udelay(200); >>> } >>> >>> +static struct intel_shared_dpll * >>> +ibx_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) >>> +{ >>> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >>> + struct intel_shared_dpll *pll; >>> + enum intel_dpll_id i; >>> + >>> + if (HAS_PCH_IBX(dev_priv)) { >>> + /* Ironlake PCH has a fixed PLL->PCH pipe mapping. */ >>> + i = (enum intel_dpll_id) crtc->pipe; >>> + pll = &dev_priv->shared_dplls[i]; >>> + >>> + DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n", >>> + crtc->base.base.id, pll->name); >>> + } else { >>> + pll = intel_find_shared_dpll(crtc, crtc_state, >>> + DPLL_ID_PCH_PLL_A, >>> + DPLL_ID_PCH_PLL_B); >>> + } >>> + >>> + /* reference the pll */ >>> + intel_reference_shared_dpll(pll, crtc_state); >>> + >>> + return pll; >>> +} >>> + >>> static const struct intel_shared_dpll_funcs ibx_pch_dpll_funcs = { >>> .mode_set = ibx_pch_dpll_mode_set, >>> .enable = ibx_pch_dpll_enable, >>> @@ -475,6 +437,19 @@ static bool hsw_ddi_spll_get_hw_state(struct >>> drm_i915_private *dev_priv, >>> return val & SPLL_PLL_ENABLE; >>> } >>> >>> +static struct intel_shared_dpll * >>> +hsw_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) >>> +{ >>> + struct intel_shared_dpll *pll; >>> + >>> + pll = intel_find_shared_dpll(crtc, crtc_state, >>> + DPLL_ID_WRPLL1, DPLL_ID_WRPLL2); >>> + if (pll) >>> + intel_reference_shared_dpll(pll, crtc_state); >>> + >>> + return pll; >>> +} >>> + >>> >>> static const struct intel_shared_dpll_funcs hsw_ddi_wrpll_funcs = { >>> .enable = hsw_ddi_wrpll_enable, >>> @@ -594,6 +569,19 @@ out: >>> return ret; >>> } >>> >>> +static struct intel_shared_dpll * >>> +skl_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) >>> +{ >>> + struct intel_shared_dpll *pll; >>> + >>> + pll = intel_find_shared_dpll(crtc, crtc_state, >>> + DPLL_ID_SKL_DPLL1, DPLL_ID_SKL_DPLL3); >>> + if (pll) >>> + intel_reference_shared_dpll(pll, crtc_state); >>> + >>> + return pll; >>> +} >>> + >>> static const struct intel_shared_dpll_funcs skl_ddi_pll_funcs = { >>> .enable = skl_ddi_pll_enable, >>> .disable = skl_ddi_pll_disable, >>> @@ -782,6 +770,32 @@ out: >>> return ret; >>> } >>> >>> +static struct intel_shared_dpll * >>> +bxt_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) >>> +{ >>> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >>> + struct intel_encoder *encoder; >>> + struct intel_digital_port *intel_dig_port; >>> + struct intel_shared_dpll *pll; >>> + enum intel_dpll_id i; >>> + >>> + /* PLL is attached to port in bxt */ >>> + encoder = intel_ddi_get_crtc_new_encoder(crtc_state); >>> + if (WARN_ON(!encoder)) >>> + return NULL; >>> + >>> + intel_dig_port = enc_to_dig_port(&encoder->base); >>> + /* 1:1 mapping between ports and PLLs */ >>> + i = (enum intel_dpll_id)intel_dig_port->port; >>> + pll = &dev_priv->shared_dplls[i]; >>> + DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n", >>> + crtc->base.base.id, pll->name); >>> + >>> + intel_reference_shared_dpll(pll, crtc_state); >>> + >>> + return pll; >>> +} >>> + >>> static const struct intel_shared_dpll_funcs bxt_ddi_pll_funcs = { >>> .enable = bxt_ddi_pll_enable, >>> .disable = bxt_ddi_pll_disable, >>> @@ -826,12 +840,24 @@ struct dpll_info { >>> const struct intel_shared_dpll_funcs *funcs; >>> }; >>> >>> +struct intel_dpll_mgr { >>> + const struct dpll_info *dpll_info; >>> + >>> + struct intel_shared_dpll *(*get_dpll)(struct intel_crtc *crtc, >>> + struct intel_crtc_state >>> *crtc_state); >>> +}; >>> + >>> 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 intel_dpll_mgr pch_pll_mgr = { >>> + .dpll_info = pch_plls, >>> + .get_dpll = ibx_get_dpll, >>> +}; >>> + >>> 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 }, >>> @@ -839,6 +865,11 @@ static const struct dpll_info hsw_plls[] = { >>> { NULL, -1, NULL, }, >>> }; >>> >>> +static const struct intel_dpll_mgr hsw_pll_mgr = { >>> + .dpll_info = hsw_plls, >>> + .get_dpll = hsw_get_dpll, >>> +}; >>> + >>> 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 }, >>> @@ -846,6 +877,11 @@ static const struct dpll_info skl_plls[] = { >>> { NULL, -1, NULL, }, >>> }; >>> >>> +static const struct intel_dpll_mgr skl_pll_mgr = { >>> + .dpll_info = skl_plls, >>> + .get_dpll = skl_get_dpll, >>> +}; >>> + >>> static const struct dpll_info bxt_plls[] = { >>> { "PORT PLL A", 0, &bxt_ddi_pll_funcs }, >>> { "PORT PLL B", 1, &bxt_ddi_pll_funcs }, >>> @@ -853,26 +889,34 @@ static const struct dpll_info bxt_plls[] = { >>> { NULL, -1, NULL, }, >>> }; >>> >>> +static const struct intel_dpll_mgr bxt_pll_mgr = { >>> + .dpll_info = bxt_plls, >>> + .get_dpll = bxt_get_dpll, >>> +}; >>> + >>> 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; >>> + const struct intel_dpll_mgr *dpll_mgr = NULL; >>> + const struct dpll_info *dpll_info; >>> int i; >>> >>> if (IS_SKYLAKE(dev) || IS_KABYLAKE(dev)) >>> - dpll_info = skl_plls; >>> + dpll_mgr = &skl_pll_mgr; >>> else if IS_BROXTON(dev) >>> - dpll_info = bxt_plls; >>> + dpll_mgr = &bxt_pll_mgr; >>> else if (HAS_DDI(dev)) >>> - dpll_info = hsw_plls; >>> + dpll_mgr = &hsw_pll_mgr; >>> else if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) >>> - dpll_info = pch_plls; >>> + dpll_mgr = &pch_pll_mgr; >>> >>> - if (!dpll_info) { >>> + if (!dpll_mgr) { >>> dev_priv->num_shared_dpll = 0; >>> return; >>> } >>> >>> + dpll_info = dpll_mgr->dpll_info; >>> + >>> for (i = 0; dpll_info[i].id >= 0; i++) { >>> WARN_ON(i != dpll_info[i].id); >>> >>> @@ -881,6 +925,7 @@ void intel_shared_dpll_init(struct drm_device *dev) >>> dev_priv->shared_dplls[i].funcs = *dpll_info[i].funcs; >>> } >>> >>> + dev_priv->dpll_mgr = dpll_mgr; >>> dev_priv->num_shared_dpll = i; >>> >>> BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS); >>> @@ -889,3 +934,16 @@ void intel_shared_dpll_init(struct drm_device *dev) >>> if (HAS_DDI(dev)) >>> intel_ddi_pll_init(dev); >>> } >>> + >>> +struct intel_shared_dpll * >>> +intel_get_shared_dpll(struct intel_crtc *crtc, >>> + struct intel_crtc_state *crtc_state) >>> +{ >>> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >>> + const struct intel_dpll_mgr *dpll_mgr = dev_priv->dpll_mgr; >>> + >>> + if (dpll_mgr) >>> + return dpll_mgr->get_dpll(crtc, crtc_state); >>> + else >>> + return NULL; >>> +} >> should there be a WARN_ON here? >> >> Rest looks good, so for patch 8...11/13: > Did you have comments on patch 12? I seem to have missed it. > No, I didn't get around to looking yet.
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 6de93dc..b858801 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1802,6 +1802,7 @@ struct drm_i915_private { /* dpll and cdclk state is protected by connection_mutex */ int num_shared_dpll; struct intel_shared_dpll shared_dplls[I915_NUM_PLLS]; + const struct intel_dpll_mgr *dpll_mgr; unsigned int active_crtcs; unsigned int min_pixclk[I915_MAX_PIPES]; diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c index e88dc46..3553324 100644 --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c @@ -174,66 +174,20 @@ void intel_disable_shared_dpll(struct intel_crtc *crtc) intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS); } -static enum intel_dpll_id -ibx_get_fixed_dpll(struct intel_crtc *crtc, - struct intel_crtc_state *crtc_state) -{ - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); - struct intel_shared_dpll *pll; - enum intel_dpll_id i; - - /* Ironlake PCH has a fixed PLL->PCH pipe mapping. */ - i = (enum intel_dpll_id) crtc->pipe; - pll = &dev_priv->shared_dplls[i]; - - DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n", - crtc->base.base.id, pll->name); - - return i; -} - -static enum intel_dpll_id -bxt_get_fixed_dpll(struct intel_crtc *crtc, - struct intel_crtc_state *crtc_state) -{ - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); - struct intel_encoder *encoder; - struct intel_digital_port *intel_dig_port; - struct intel_shared_dpll *pll; - enum intel_dpll_id i; - - /* PLL is attached to port in bxt */ - encoder = intel_ddi_get_crtc_new_encoder(crtc_state); - if (WARN_ON(!encoder)) - return DPLL_ID_PRIVATE; - - intel_dig_port = enc_to_dig_port(&encoder->base); - /* 1:1 mapping between ports and PLLs */ - i = (enum intel_dpll_id)intel_dig_port->port; - pll = &dev_priv->shared_dplls[i]; - DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n", - crtc->base.base.id, pll->name); - - return i; -} - -static enum intel_dpll_id +static struct intel_shared_dpll * intel_find_shared_dpll(struct intel_crtc *crtc, - struct intel_crtc_state *crtc_state) + struct intel_crtc_state *crtc_state, + enum intel_dpll_id range_min, + enum intel_dpll_id range_max) { struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; struct intel_shared_dpll *pll; struct intel_shared_dpll_config *shared_dpll; enum intel_dpll_id i; - int max = dev_priv->num_shared_dpll; - - if (INTEL_INFO(dev_priv)->gen < 9 && HAS_DDI(dev_priv)) - /* Do not consider SPLL */ - max = 2; shared_dpll = intel_atomic_get_shared_dpll_state(crtc_state->base.state); - for (i = 0; i < max; i++) { + for (i = range_min; i <= range_max; i++) { pll = &dev_priv->shared_dplls[i]; /* Only want to check enabled timings first */ @@ -247,49 +201,33 @@ intel_find_shared_dpll(struct intel_crtc *crtc, crtc->base.base.id, pll->name, shared_dpll[i].crtc_mask, pll->active); - return i; + return pll; } } /* Ok no matching timings, maybe there's a free one? */ - for (i = 0; i < dev_priv->num_shared_dpll; i++) { + for (i = range_min; i <= range_max; i++) { pll = &dev_priv->shared_dplls[i]; if (shared_dpll[i].crtc_mask == 0) { DRM_DEBUG_KMS("CRTC:%d allocated %s\n", crtc->base.base.id, pll->name); - return i; + return pll; } } - return DPLL_ID_PRIVATE; + return NULL; } -struct intel_shared_dpll * -intel_get_shared_dpll(struct intel_crtc *crtc, - struct intel_crtc_state *crtc_state) +static void +intel_reference_shared_dpll(struct intel_shared_dpll *pll, + struct intel_crtc_state *crtc_state) { - struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; - struct intel_shared_dpll *pll; struct intel_shared_dpll_config *shared_dpll; - enum intel_dpll_id i; + struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc); + enum intel_dpll_id i = pll->id; shared_dpll = intel_atomic_get_shared_dpll_state(crtc_state->base.state); - if (HAS_PCH_IBX(dev_priv->dev)) { - i = ibx_get_fixed_dpll(crtc, crtc_state); - WARN_ON(shared_dpll[i].crtc_mask); - } else if (IS_BROXTON(dev_priv->dev)) { - i = bxt_get_fixed_dpll(crtc, crtc_state); - WARN_ON(shared_dpll[i].crtc_mask); - } else { - i = intel_find_shared_dpll(crtc, crtc_state); - } - - if (i < 0) - return NULL; - - pll = &dev_priv->shared_dplls[i]; - if (shared_dpll[i].crtc_mask == 0) shared_dpll[i].hw_state = crtc_state->dpll_hw_state; @@ -299,8 +237,6 @@ intel_get_shared_dpll(struct intel_crtc *crtc, pipe_name(crtc->pipe)); intel_shared_dpll_config_get(shared_dpll, pll, crtc); - - return pll; } void intel_shared_dpll_commit(struct drm_atomic_state *state) @@ -398,6 +334,32 @@ static void ibx_pch_dpll_disable(struct drm_i915_private *dev_priv, udelay(200); } +static struct intel_shared_dpll * +ibx_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) +{ + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); + struct intel_shared_dpll *pll; + enum intel_dpll_id i; + + if (HAS_PCH_IBX(dev_priv)) { + /* Ironlake PCH has a fixed PLL->PCH pipe mapping. */ + i = (enum intel_dpll_id) crtc->pipe; + pll = &dev_priv->shared_dplls[i]; + + DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n", + crtc->base.base.id, pll->name); + } else { + pll = intel_find_shared_dpll(crtc, crtc_state, + DPLL_ID_PCH_PLL_A, + DPLL_ID_PCH_PLL_B); + } + + /* reference the pll */ + intel_reference_shared_dpll(pll, crtc_state); + + return pll; +} + static const struct intel_shared_dpll_funcs ibx_pch_dpll_funcs = { .mode_set = ibx_pch_dpll_mode_set, .enable = ibx_pch_dpll_enable, @@ -475,6 +437,19 @@ static bool hsw_ddi_spll_get_hw_state(struct drm_i915_private *dev_priv, return val & SPLL_PLL_ENABLE; } +static struct intel_shared_dpll * +hsw_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) +{ + struct intel_shared_dpll *pll; + + pll = intel_find_shared_dpll(crtc, crtc_state, + DPLL_ID_WRPLL1, DPLL_ID_WRPLL2); + if (pll) + intel_reference_shared_dpll(pll, crtc_state); + + return pll; +} + static const struct intel_shared_dpll_funcs hsw_ddi_wrpll_funcs = { .enable = hsw_ddi_wrpll_enable, @@ -594,6 +569,19 @@ out: return ret; } +static struct intel_shared_dpll * +skl_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) +{ + struct intel_shared_dpll *pll; + + pll = intel_find_shared_dpll(crtc, crtc_state, + DPLL_ID_SKL_DPLL1, DPLL_ID_SKL_DPLL3); + if (pll) + intel_reference_shared_dpll(pll, crtc_state); + + return pll; +} + static const struct intel_shared_dpll_funcs skl_ddi_pll_funcs = { .enable = skl_ddi_pll_enable, .disable = skl_ddi_pll_disable, @@ -782,6 +770,32 @@ out: return ret; } +static struct intel_shared_dpll * +bxt_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state) +{ + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); + struct intel_encoder *encoder; + struct intel_digital_port *intel_dig_port; + struct intel_shared_dpll *pll; + enum intel_dpll_id i; + + /* PLL is attached to port in bxt */ + encoder = intel_ddi_get_crtc_new_encoder(crtc_state); + if (WARN_ON(!encoder)) + return NULL; + + intel_dig_port = enc_to_dig_port(&encoder->base); + /* 1:1 mapping between ports and PLLs */ + i = (enum intel_dpll_id)intel_dig_port->port; + pll = &dev_priv->shared_dplls[i]; + DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n", + crtc->base.base.id, pll->name); + + intel_reference_shared_dpll(pll, crtc_state); + + return pll; +} + static const struct intel_shared_dpll_funcs bxt_ddi_pll_funcs = { .enable = bxt_ddi_pll_enable, .disable = bxt_ddi_pll_disable, @@ -826,12 +840,24 @@ struct dpll_info { const struct intel_shared_dpll_funcs *funcs; }; +struct intel_dpll_mgr { + const struct dpll_info *dpll_info; + + struct intel_shared_dpll *(*get_dpll)(struct intel_crtc *crtc, + struct intel_crtc_state *crtc_state); +}; + 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 intel_dpll_mgr pch_pll_mgr = { + .dpll_info = pch_plls, + .get_dpll = ibx_get_dpll, +}; + 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 }, @@ -839,6 +865,11 @@ static const struct dpll_info hsw_plls[] = { { NULL, -1, NULL, }, }; +static const struct intel_dpll_mgr hsw_pll_mgr = { + .dpll_info = hsw_plls, + .get_dpll = hsw_get_dpll, +}; + 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 }, @@ -846,6 +877,11 @@ static const struct dpll_info skl_plls[] = { { NULL, -1, NULL, }, }; +static const struct intel_dpll_mgr skl_pll_mgr = { + .dpll_info = skl_plls, + .get_dpll = skl_get_dpll, +}; + static const struct dpll_info bxt_plls[] = { { "PORT PLL A", 0, &bxt_ddi_pll_funcs }, { "PORT PLL B", 1, &bxt_ddi_pll_funcs }, @@ -853,26 +889,34 @@ static const struct dpll_info bxt_plls[] = { { NULL, -1, NULL, }, }; +static const struct intel_dpll_mgr bxt_pll_mgr = { + .dpll_info = bxt_plls, + .get_dpll = bxt_get_dpll, +}; + 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; + const struct intel_dpll_mgr *dpll_mgr = NULL; + const struct dpll_info *dpll_info; int i; if (IS_SKYLAKE(dev) || IS_KABYLAKE(dev)) - dpll_info = skl_plls; + dpll_mgr = &skl_pll_mgr; else if IS_BROXTON(dev) - dpll_info = bxt_plls; + dpll_mgr = &bxt_pll_mgr; else if (HAS_DDI(dev)) - dpll_info = hsw_plls; + dpll_mgr = &hsw_pll_mgr; else if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) - dpll_info = pch_plls; + dpll_mgr = &pch_pll_mgr; - if (!dpll_info) { + if (!dpll_mgr) { dev_priv->num_shared_dpll = 0; return; } + dpll_info = dpll_mgr->dpll_info; + for (i = 0; dpll_info[i].id >= 0; i++) { WARN_ON(i != dpll_info[i].id); @@ -881,6 +925,7 @@ void intel_shared_dpll_init(struct drm_device *dev) dev_priv->shared_dplls[i].funcs = *dpll_info[i].funcs; } + dev_priv->dpll_mgr = dpll_mgr; dev_priv->num_shared_dpll = i; BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS); @@ -889,3 +934,16 @@ void intel_shared_dpll_init(struct drm_device *dev) if (HAS_DDI(dev)) intel_ddi_pll_init(dev); } + +struct intel_shared_dpll * +intel_get_shared_dpll(struct intel_crtc *crtc, + struct intel_crtc_state *crtc_state) +{ + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); + const struct intel_dpll_mgr *dpll_mgr = dev_priv->dpll_mgr; + + if (dpll_mgr) + return dpll_mgr->get_dpll(crtc, crtc_state); + else + return NULL; +} diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h index 8be2478..7794c7a 100644 --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h @@ -30,6 +30,8 @@ struct intel_crtc; struct intel_crtc_state; struct intel_shared_dpll; +struct intel_dpll_mgr; + enum intel_dpll_id { DPLL_ID_PRIVATE = -1, /* non-shared dpll in use */ /* real shared dpll ids must be >= 0 */
The function intel_get_shared_dpll() had a more or less generic implementation with some platform specific checks to handle smaller differences between platforms. However, the minimalist approach forces bigger differences between platforms to be implemented outside of the shared dpll code (see the *_ddi_pll_select() functions in intel_ddi.c, for instance). This patch changes the implementation of intel_get_share_dpll() so that a completely platform specific version can be used, providing helpers to reduce code duplication. This should allow the code from the ddi pll select functions to be moved, and also make room for making more dplls managed by the shared dpll infrastructure. Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_dpll_mgr.c | 226 +++++++++++++++++++++------------- drivers/gpu/drm/i915/intel_dpll_mgr.h | 2 + 3 files changed, 145 insertions(+), 84 deletions(-)