[Intel-gfx] [PATCH 3/3] drm/i915/icl: decouple dpll ids from type
Lucas De Marchi
lucas.demarchi at intel.com
Mon Feb 25 21:28:23 UTC 2019
On Mon, Feb 25, 2019 at 10:45:34PM +0200, Ville Syrjälä wrote:
>On Fri, Feb 22, 2019 at 03:23:24PM -0800, Lucas De Marchi wrote:
>> Use the first 3 bits of dpll_info.platform_flags to mark the type of the
>> PLL instead of relying on the IDs. This is more future proof for
>> allowing the same set of functions to be reused, even if the IDs change.
>>
>> The warning about PLL id not corresponding to a combo phy in intel_display
>> was removed as I don't think it should ever trigger.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_display.c | 3 --
>> drivers/gpu/drm/i915/intel_dpll_mgr.c | 54 +++++++++++++++++----------
>> drivers/gpu/drm/i915/intel_dpll_mgr.h | 1 -
>> 3 files changed, 35 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index b1d63c32ca94..a2be35118dd5 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -9592,9 +9592,6 @@ static void icelake_get_ddi_pll(struct drm_i915_private *dev_priv,
>> temp = I915_READ(DPCLKA_CFGCR0_ICL) &
>> DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(port);
>> id = temp >> DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(port);
>> -
>> - if (WARN_ON(!intel_dpll_is_combophy(id)))
>> - return;
>> } else if (intel_port_is_tc(dev_priv, port)) {
>> id = icl_tc_port_to_pll_id(intel_port_to_tc(dev_priv, port));
>> } else {
>> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
>> index e4ec73d415d9..cdb4463bab5d 100644
>> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
>> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
>> @@ -2639,6 +2639,13 @@ int icl_calc_dp_combo_pll_link(struct drm_i915_private *dev_priv,
>> return link_clock;
>> }
>>
>> +enum icl_dpll_flags {
>> + ICL_DPLL_TYPE_COMBOPHY,
>> + ICL_DPLL_TYPE_TBT,
>> + ICL_DPLL_TYPE_MG,
>> + _ICL_DPLL_TYPE_MASK = 0x7,
>> +};
>> +
>> static enum tc_port icl_pll_id_to_tc_port(enum intel_dpll_id id)
>> {
>> return id - DPLL_ID_ICL_MGPLL1;
>> @@ -2649,9 +2656,9 @@ enum intel_dpll_id icl_tc_port_to_pll_id(enum tc_port tc_port)
>> return tc_port + DPLL_ID_ICL_MGPLL1;
>> }
>>
>> -bool intel_dpll_is_combophy(enum intel_dpll_id id)
>> +static enum icl_dpll_flags icl_dpll_type(const struct dpll_info *info)
>> {
>> - return id == DPLL_ID_ICL_DPLL0 || id == DPLL_ID_ICL_DPLL1;
>> + return info->platform_flags & _ICL_DPLL_TYPE_MASK;
>> }
>>
>> static bool icl_mg_pll_find_divisors(int clock_khz, bool is_dp, bool use_ssc,
>> @@ -2956,14 +2963,22 @@ icl_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state,
>> return pll;
>> }
>>
>> -static i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id)
>> +static i915_reg_t
>> +icl_pll_info_to_enable_reg(const struct dpll_info *info)
>> {
>> - if (intel_dpll_is_combophy(id))
>> - return CNL_DPLL_ENABLE(id);
>> - else if (id == DPLL_ID_ICL_TBTPLL)
>> - return TBT_PLL_ENABLE;
>> + enum icl_dpll_flags type = icl_dpll_type(info);
>>
>> - return MG_PLL_ENABLE(icl_pll_id_to_tc_port(id));
>> + switch (type) {
>> + case ICL_DPLL_TYPE_COMBOPHY:
>> + return CNL_DPLL_ENABLE(info->id);
>> + case ICL_DPLL_TYPE_TBT:
>> + return TBT_PLL_ENABLE;
>> + case ICL_DPLL_TYPE_MG:
>> + return MG_PLL_ENABLE(icl_pll_id_to_tc_port(info->id));
>> + default:
>> + MISSING_CASE(type);
>> + return CNL_DPLL_ENABLE(info->id);
>> + }
>> }
>
>This seems a rather roundabout way of doing things when we already have
>the vfuncs.
>
>How about just:
>
>mg_pll_enable()
>{
> icl_pll_enable(MG_REG);
>}
>
>combo_pll_enable()
>{
> icl_pll_enable(COMBO_REG);
>}
>
>or something along those lines.
humn... I thought about this approach as an intermediate step to the
full blown "add another vfunc struct and pass that instead". Platforms
are free to use this for small differences that don't justify going that
route.
In your approach you go the route of "always use vfunc and add
additional arguments to the common function".
Compared to the approach here, it's not subjective on what to do in
similar cases, but has its downsides as well.
1) The function can't be inlined so there's and extra hop when calling
the vfunc
2) if the callee is inlined we basically duplicate .text, but I doubt
any compiler would do that
3) reg as the argument is probably not a good choice as it may change
in the next platform - so we would need to add a "type" nonetheless
Since flags is already there and under-utilized I don't see much
advantage on the vfunc-only approach. I'm not strongly opposed though:
IMO both are better than the current state.
Lucas De Marchi
>
>
>>
>> static bool mg_pll_get_hw_state(struct drm_i915_private *dev_priv,
>> @@ -3042,7 +3057,7 @@ static bool icl_pll_get_hw_state(struct drm_i915_private *dev_priv,
>> if (!wakeref)
>> return false;
>>
>> - val = I915_READ(icl_pll_id_to_enable_reg(id));
>> + val = I915_READ(icl_pll_info_to_enable_reg(pll->info));
>> if (!(val & PLL_ENABLE))
>> goto out;
>>
>> @@ -3120,7 +3135,8 @@ static void icl_pll_enable(struct drm_i915_private *dev_priv,
>> struct intel_shared_dpll *pll)
>> {
>> const enum intel_dpll_id id = pll->info->id;
>> - i915_reg_t enable_reg = icl_pll_id_to_enable_reg(id);
>> + enum icl_dpll_flags type = icl_dpll_type(pll->info);
>> + i915_reg_t enable_reg = icl_pll_info_to_enable_reg(pll->info);
>> u32 val;
>>
>> val = I915_READ(enable_reg);
>> @@ -3135,7 +3151,7 @@ static void icl_pll_enable(struct drm_i915_private *dev_priv,
>> PLL_POWER_STATE, 1))
>> DRM_ERROR("PLL %d Power not enabled\n", id);
>>
>> - if (intel_dpll_is_combophy(id) || id == DPLL_ID_ICL_TBTPLL)
>> + if (type == ICL_DPLL_TYPE_COMBOPHY || type == ICL_DPLL_TYPE_TBT)
>> icl_dpll_write(dev_priv, pll);
>> else
>> icl_mg_pll_write(dev_priv, pll);
>> @@ -3161,7 +3177,7 @@ static void icl_pll_disable(struct drm_i915_private *dev_priv,
>> struct intel_shared_dpll *pll)
>> {
>> const enum intel_dpll_id id = pll->info->id;
>> - i915_reg_t enable_reg = icl_pll_id_to_enable_reg(id);
>> + i915_reg_t enable_reg = icl_pll_info_to_enable_reg(pll->info);
>> u32 val;
>>
>> /* The first steps are done by intel_ddi_post_disable(). */
>> @@ -3230,13 +3246,13 @@ static const struct intel_shared_dpll_funcs mg_pll_funcs = {
>> };
>>
>> static const struct dpll_info icl_plls[] = {
>> - { "DPLL 0", &icl_pll_funcs, DPLL_ID_ICL_DPLL0, 0 },
>> - { "DPLL 1", &icl_pll_funcs, DPLL_ID_ICL_DPLL1, 0 },
>> - { "TBT PLL", &icl_pll_funcs, DPLL_ID_ICL_TBTPLL, 0 },
>> - { "MG PLL 1", &mg_pll_funcs, DPLL_ID_ICL_MGPLL1, 0 },
>> - { "MG PLL 2", &mg_pll_funcs, DPLL_ID_ICL_MGPLL2, 0 },
>> - { "MG PLL 3", &mg_pll_funcs, DPLL_ID_ICL_MGPLL3, 0 },
>> - { "MG PLL 4", &mg_pll_funcs, DPLL_ID_ICL_MGPLL4, 0 },
>> + { "DPLL 0", &icl_pll_funcs, DPLL_ID_ICL_DPLL0, 0, ICL_DPLL_TYPE_COMBOPHY },
>> + { "DPLL 1", &icl_pll_funcs, DPLL_ID_ICL_DPLL1, 0, ICL_DPLL_TYPE_COMBOPHY },
>> + { "TBT PLL", &icl_pll_funcs, DPLL_ID_ICL_TBTPLL, 0, ICL_DPLL_TYPE_TBT },
>> + { "MG PLL 1", &mg_pll_funcs, DPLL_ID_ICL_MGPLL1, 0, ICL_DPLL_TYPE_MG },
>> + { "MG PLL 2", &mg_pll_funcs, DPLL_ID_ICL_MGPLL2, 0, ICL_DPLL_TYPE_MG },
>> + { "MG PLL 3", &mg_pll_funcs, DPLL_ID_ICL_MGPLL3, 0, ICL_DPLL_TYPE_MG },
>> + { "MG PLL 4", &mg_pll_funcs, DPLL_ID_ICL_MGPLL4, 0, ICL_DPLL_TYPE_MG },
>> { },
>> };
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
>> index b1fdce1be942..12ffe83598aa 100644
>> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
>> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
>> @@ -352,6 +352,5 @@ int icl_calc_dp_combo_pll_link(struct drm_i915_private *dev_priv,
>> u32 pll_id);
>> int cnl_hdmi_pll_ref_clock(struct drm_i915_private *dev_priv);
>> enum intel_dpll_id icl_tc_port_to_pll_id(enum tc_port tc_port);
>> -bool intel_dpll_is_combophy(enum intel_dpll_id id);
>>
>> #endif /* _INTEL_DPLL_MGR_H_ */
>> --
>> 2.20.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>--
>Ville Syrjälä
>Intel
More information about the Intel-gfx
mailing list