Re: [PATCH v3 3/5] OPP: Improve require-opps linking

[Saravana Kannan <saravanak@google.com>]

On Thu, Jul 25, 2019 at 6:52 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Wed, Jul 24, 2019 at 10:17 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 24-07-19, 21:09, Saravana Kannan wrote:
> > > On Wed, Jul 24, 2019 at 8:07 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > > We should be doing this whenever a new OPP table is created, and see
> > > > if someone else was waiting for this OPP table to come alive.
> > >
> > > Searching the global OPP table list seems a ton more wasteful than
> > > doing the lazy linking. I'd rather not do this.
> >
> > We can see how best to optimize that, but it will be done only once
> > while a new OPP table is created and putting stress there is the right
> > thing to do IMO. And doing anything like that in a place like
> > opp-set-rate is the worst one. It will be a bad choice by design if
> > you ask me and so I am very much against that.
> >
> > > > Also we
> > > > must make sure that we do this linking only if the new OPP table has
> > > > its own required-opps links fixed, otherwise delay further.
> > >
> > > This can be done. Although even without doing that, this patch is
> > > making things better by not failing silently like it does today? Can I
> > > do this later as a separate patch set series?
> >
> > I would like this to get fixed now in a proper way, there is no hurry
> > for a quick fix currently. No band-aids please.
> >
> > > > Even then I don't want to add these checks to those places. For the
> > > > opp-set-rate routine, add another flag to the OPP table which
> > > > indicates if we are ready to do dvfs or not and mark it true only
> > > > after the required-opps are all set.
> > >
> > > Honestly, this seems like extra memory and micro optimization without
> > > any data to back it.
> >
> > Again, opp-set-rate isn't supposed to do something like this. It
> > shouldn't handle initializations of things, that is broken design.
> >
> > > Show me data that checking all these table
> > > pointers is noticeably slower than what I'm doing. What's the max
> > > "required tables count" you've seen in upstream so far?
> >
> > Running anything extra (specially some initialization stuff) in
> > opp-set-rate is wrong as per me and as a Maintainer of the OPP core it
> > is my responsibility to not allow such things to happen.
>
> Doing operations lazily right before they are needed isn't something
> new in the kernel. It's done all over the place (VFP save/restore?).
> It's not worth arguing though -- so I'll agree to disagree but follow
> the Maintainer's preference.
>

I was taking a closer look at the OPP framework code to try and do
what you ask above, but it's kind of a mess. The whole "the same OPP
table can be used by multiple devices without the opp-shared flag set"
is effectively breaking "required-opps" at a minimum and probably a
lot more cases. I don't think I can rewrite my patch the way you want
it without fixing the existing bugs.

Let's take this example DT (leaving out the irrelevant part):

OPP table 1:
    required-opps = <OPP table 2 entry>;

OPP table 2:
    <opp-shared property not set>

Device A:
    operating-points-v2 = <&OPP table 1>

Device B:
    operating-points-v2 = <&OPP table 2>

Device C:
    operating-points-v2 = <&OPP table 2>

Let's say device B and C add their OPP tables. They both get their own
"in-memory" copy of the OPP table. They can then enabled/disable
different OPP entries (rows) and not affect each other's OPP table.
Which is how it's expected to work.

Now if device A adds its OPP table 1, the "in-memory"
required_opp_tables pointer of OPP table 1 can end up pointing to
either Device A's copy of the OPP table or Device B's copy of the OPP
table depending on which happens to be added first. This effectively
random linking of OPP tables is mutually exclusive to the point of
required-opps.

Also, at a DT definition level, OPP table 1 pointing to OPP table 2
when OPP table 2 is used by more than one device doesn't make any
sense. Which device/genpd is OPP table 1 saying it "requires to
operate at a certain level"?

So I propose that we should consider the OPP table DT configuration
invalid if one OPP table points to another OPP tables that's NOT
shared but is ALSO pointed to by multiple devices. Basically the
example above would be considered an invalid DT configuration. Does
that sound okay to you? If I make changes to enforce that, will that
be acceptable?

If this sounds okay to you, then in the example above, assume Device C
isn't present. Then when OPP table 1 is added by device A, if OPP
table 2 hasn't been added already, I can just go ahead and allocate
OPP table 2. And then when device B tries to add OPP table 2, I can
just tie device B to OPP table 2 and fill up any of the missing
pieces.

This sounds better than trying to loop through existing OPP tables and
seeing if any other table is waiting for the newly added table and
marking the waiting tables as "linked". Especially because it gets a
lot more complicated and inefficient when you consider a chain of OPP
tables and many-to-many linking of OPP tables.

-Saravana