[Intel-gfx] [PATCH 08/13] drm/i915: Refactor platform specifics out of intel_get_shared_dpll()
Maarten Lankhorst
maarten.lankhorst at linux.intel.com
Mon Mar 7 09:59:08 UTC 2016
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 at 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.
More information about the Intel-gfx
mailing list