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

[Saravana Kannan <saravanak@google.com>]

On Wed, Jul 24, 2019 at 8:07 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 23-07-19, 07:47, Saravana Kannan wrote:
> > On Tue, Jul 23, 2019, 3:28 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > > $subject doesn't have correct property name.
> > >
> > > On 17-07-19, 15:23, Saravana Kannan wrote:
> > > > Currently, the linking of required-opps fails silently if the
> > > > destination OPP table hasn't been added before the source OPP table is
> > > > added. This puts an unnecessary requirement that the destination table
> > > > be added before the source table is added.
> > > >
> > > > In reality, the destination table is needed only when we try to
> > > > translate from source OPP to destination OPP. So, instead of
> > > > completely failing, retry linking the tables when the translation is
> > > > attempted.
> > > >
> > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > ---
> > > >  drivers/opp/core.c | 32 +++++++++++-----
> > > >  drivers/opp/of.c   | 91 ++++++++++++++++++++++------------------------
> > > >  drivers/opp/opp.h  |  5 +++
> > > >  3 files changed, 71 insertions(+), 57 deletions(-)
> > >
> > > Here is the general feedback and requirements I have:
> > >
> > > - We shouldn't do it from _set_required_opps() but way earlier, it
> > >   shouldn't affect the fast path (where we change the frequency).
> > >
> >
> > I don't think there's any other intermediate OPP call where we can try to
> > link the tables. Also if there tables are already fully linked, this is
> > just checking for not NULL for a few elements in the array.
>
> 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.

> 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 don't think
> > this is really going to allow down the fast path in anyway.
>
> > If you have other ideas, I'm willing to look at it. But as it is right now
> > seems the best to me.
> >
> 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. 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?

I'd even argue that doing it the way I do might actually reduce the
cache misses/warm the cache because those pointers are going to be
searched/used right after anyway.

-Saravana