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@xxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxxx>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
