Re: [PATCH 3/3] drm/i915/icl: decouple dpll ids from type

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Feb 26, 2019 at 11:02:58AM -0800, Lucas De Marchi wrote:
> On Tue, Feb 26, 2019 at 04:48:23PM +0200, Ville Syrjälä wrote:
> >> >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.
> 
> I'm talking about the extra function you added here. The vfunc will call
> this, which then calls the real common function.
> 
> >> 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?
> 
> In this case I'm referring to icl_pll_enable() being inlined inside
> mg_pll_enable() and combo_pll_enable().
> 
> But let's leave alone the inlines and extra function calls and talk
> about the organization below.
> 
> >> 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.
> 
> because here the function can share more when I consider the *type* of
> the pll, not if it's reg 0x10, 0x30 or 0x40.
> 
> >>
> >> 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.
> 
> 
> both of them already exist - flags is already there. If I handle the
> *types* differently with your approach I would basically have:
> 
>     enum pll_type {
>         A,
>         B,
>         C,
>     }
> 
>     pll_enable()
>     {
>         ...
> 
>         if (type == A)
>         else if (type == B)
>         else

This thing shouldn't have any ifs in it if this is done right.

The more ifs you have the harder it is to follow the code.
Ideally we'd have none.

> 
>         ...
>     }
> 
>     a_pll_enable()
>     {
>         return pll_enable(A);
>     }
> 
>     b_pll_enable()
>     {
>         return pll_enable(B);
>     }
> 
>     c_pll_enable()
>     {
>         return pll_enable(C);
>     }
> 
>     static const struct funcs a_funcs = {
>         .enable = a_pll_enable(),
>     };
>     static const struct funcs b_funcs = {
>         .eanble = b_pll_enable(),
>     };
>     static const struct funcs c_funcs = {
>         .enable = c_pll_enable(),
>     };
> 
>     static const struct plls[] = {
>         { a_funcs, ... },
>         { b_funcs, ... },
>         { c_funcs, ... },
>     };
> 
> 
> This approach has its value when the implementations are completely
> different - e.g. the mg vs combo approach in patch 1. When the
> implementation is very similar, what I'm proposing is to be able to do:
> 
>     enum pll_type {
>         A,
>         B,
>         C,
>     }
> 
>     pll_enable()
>     {
>         ...
> 
>         if (type == A)
>         else if (type == B)
>         else
> 
>         ...
>     }
> 
>     static const struct funcs funcs = {
>         .enable = pll_enable(),
>     };
>  
>     static const struct plls[] = {
>         { funcs, A, ... }
>         { funcs, B, ... }
>         { funcs, C, ... }
>     }
> 
> We have less boilerplate code and the information is in the table rather
> than spread across multiple functions. Don't get me wrong, the other
> approach has its place and is even used in patch 1 because the impl
> is totally different.
> 
> In the ICL case, the type in the table is used to tweak the behavior for
> MG vs TBT, because they reuse most of the same calls. Combo vs MG is
> handled in patch 1, not here.
> 
> Lucas De Marchi

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx