[Intel-gfx] [PATCH 3/3] drm/i915/icl: decouple dpll ids from type

[Ville Syrjälä]

On Mon, Feb 25, 2019 at 01:28:23PM -0800, Lucas De Marchi wrote:
> 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

We already have the vfunc. And even if we didn't, an extra vfunc in
modesetting code is probably in the noise.

> 2) if the callee is inlined we basically duplicate .text, but I doubt
> any compiler would do that

Either it inlines or not. Why should we care in this particular case?

> 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

Not sure what you mean. If the reg changes you pass in a different reg.
If other things differ significantly you write a new function.

> 
> 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.

If the existing mechanism is sufficient to the task then we should
generally use it rather than adding yet another mechanism. This
keeps the code more uniform and thus easier for everyone to
understand.

-- 
Ville Syrjälä
Intel