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

[Lucas De Marchi]

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

        ...
    }

    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