Date:   Sat, 29 Jul 2017 10:24:00 +0200
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Viresh Kumar <viresh.kumar@...aro.org>
Cc:     Rafael Wysocki <rjw@...ysocki.net>,
        Kevin Hilman <khilman@...nel.org>,
        "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Stephen Boyd <sboyd@...eaurora.org>,
        Nishanth Menon <nm@...com>, Rob Herring <robh+dt@...nel.org>,
        Lina Iyer <lina.iyer@...aro.org>,
        Rajendra Nayak <rnayak@...eaurora.org>,
        Sudeep Holla <sudeep.holla@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Len Brown <len.brown@...el.com>, Pavel Machek <pavel@....cz>,
        Andy Gross <andy.gross@...aro.org>,
        David Brown <david.brown@...aro.org>
Subject: Re: [PATCH V8 1/6] PM / Domains: Add support to select
 performance-state of domains

On 28 July 2017 at 13:00, Viresh Kumar <viresh.kumar@...aro.org> wrote:
> On 21-07-17, 10:35, Ulf Hansson wrote:
>> >> > +/*
>> >> > + * Returns true if anyone in genpd's parent hierarchy has
>> >> > + * set_performance_state() set.
>> >> > + */
>> >> > +static bool genpd_has_set_performance_state(struct generic_pm_domain *genpd)
>> >> > +{
>> >>
>> >> So this function will be become in-directly called by generic drivers
>> >> that supports DVFS of the genpd for their devices.
>> >>
>> >> I think the data you validate here would be better to be pre-validated
>> >> at pm_genpd_init() and at pm_genpd_add|remove_subdomain() and the
>> >> result stored in a variable in the genpd struct. Especially when a
>> >> subdomain is added, that is a point when you can verify the
>> >> *_performance_state() callbacks, and thus make sure it's a correct
>> >> setup from the topology point of view.
>
> Looks like I have to keep this routine as is and your solution may not
> work well. :(
>
>> > Something like this ?
>> >
>> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> > index 4a898e095a1d..182c1911ea9c 100644
>> > --- a/drivers/base/power/domain.c
>> > +++ b/drivers/base/power/domain.c
>> > @@ -466,25 +466,6 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb,
>> >         return NOTIFY_DONE;
>> >  }
>> >
>> > -/*
>> > - * Returns true if anyone in genpd's parent hierarchy has
>> > - * set_performance_state() set.
>> > - */
>> > -static bool genpd_has_set_performance_state(struct generic_pm_domain *genpd)
>> > -{
>> > -       struct gpd_link *link;
>> > -
>> > -       if (genpd->set_performance_state)
>> > -               return true;
>> > -
>> > -       list_for_each_entry(link, &genpd->slave_links, slave_node) {
>> > -               if (genpd_has_set_performance_state(link->master))
>> > -                       return true;
>> > -       }
>> > -
>> > -       return false;
>> > -}
>> > -
>> >  /**
>> >   * pm_genpd_has_performance_state - Checks if power domain does performance
>> >   * state management.
>> > @@ -507,7 +488,7 @@ bool pm_genpd_has_performance_state(struct device *dev)
>> >
>> >         /* The parent domain must have set get_performance_state() */
>> >         if (!IS_ERR(genpd) && genpd->get_performance_state) {
>> > -               if (genpd_has_set_performance_state(genpd))
>> > +               if (genpd->can_set_performance_state)
>> >                         return true;
>> >
>> >                 /*
>> > @@ -1594,6 +1575,8 @@ static int genpd_add_subdomain(struct generic_pm_domain *genpd,
>> >         if (genpd_status_on(subdomain))
>> >                 genpd_sd_counter_inc(genpd);
>> >
>> > +       subdomain->can_set_performance_state += genpd->can_set_performance_state;
>> > +
>> >   out:
>> >         genpd_unlock(genpd);
>> >         genpd_unlock(subdomain);
>> > @@ -1654,6 +1637,8 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
>> >                 if (genpd_status_on(subdomain))
>> >                         genpd_sd_counter_dec(genpd);
>> >
>> > +               subdomain->can_set_performance_state -= genpd->can_set_performance_state;
>> > +
>> >                 ret = 0;
>> >                 break;
>> >         }
>> > @@ -1721,6 +1706,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>> >         genpd->max_off_time_changed = true;
>> >         genpd->provider = NULL;
>> >         genpd->has_provider = false;
>> > +       genpd->can_set_performance_state = !!genpd->set_performance_state;
>> >         genpd->domain.ops.runtime_suspend = genpd_runtime_suspend;
>> >         genpd->domain.ops.runtime_resume = genpd_runtime_resume;
>> >         genpd->domain.ops.prepare = pm_genpd_prepare;
>> > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> > index bf90177208a2..995d0cb1bc14 100644
>> > --- a/include/linux/pm_domain.h
>> > +++ b/include/linux/pm_domain.h
>> > @@ -64,6 +64,7 @@ struct generic_pm_domain {
>> >         unsigned int suspended_count;   /* System suspend device counter */
>> >         unsigned int prepared_count;    /* Suspend counter of prepared devices */
>> >         unsigned int performance_state; /* Max requested performance state */
>> > +       unsigned int can_set_performance_state; /* Number of parent domains supporting set state */
>> >         int (*power_off)(struct generic_pm_domain *domain);
>> >         int (*power_on)(struct generic_pm_domain *domain);
>> >         int (*get_performance_state)(struct device *dev, unsigned long rate);
>> >
>>
>> Yes!
>
> The above diff will work fine only for the case where the master
> domain has all its masters set properly before genpd_add_subdomain()
> is called for the subdomain, as the genpd->can_set_performance_state
> count wouldn't change after that. But if the masters of the
> master are linked to the master after genpd_add_subdomain() is called
> for the subdomain, then we wouldn't be update the
> subdomain->can_set_performance_state field later.
>
> For example, consider this scenario:
>
>                Domain A (has set_performance_state())
>
>        Domain B                Domain C        (both don't have set_performance_state())
>
>        Domain D                Domain E         (both don't have set_performance_state(), but have get_performance_state())
>
>
> and here is the call sequence:
>
> genpd_add_subdomain(B, D); can_set_performance_state of B and D = 0;
> genpd_add_subdomain(C, E); ... C and E = 0;
> genpd_add_subdomain(A, B); ... A = 1, B = 1;
> genpd_add_subdomain(A, C); ... A = 1, C = 1;
>
> While the count is set properly for A, B and C, it isn't propagated to
> C and E. :(
>
> Though everything would have worked fine if we had this sequence:
>
> genpd_add_subdomain(A, B); ... A = 1, B = 1;
> genpd_add_subdomain(A, C); ... A = 1, C = 1;
> genpd_add_subdomain(B, D); ... D = 1 ;
> genpd_add_subdomain(C, E); ... E = 1;
>
> How to fix it? I tried solving that by propagating the count to all
> the subdomains of the subdomain getting added here. But that requires
> locking and we can't do that in the reverse direction :(

Yeah, you are right!

>
> Anyway, genpd_has_set_performance_state() is supposed to be called
> only ONCE by the drivers and so its fine if we have to traverse the
> list of subdomains there.
>
> I will keep the original code unless you suggest a good way of getting
> around that.

Let's invent a new genpd flag, GENPD_FLAG_PERF_STATE!

The creator of the genpd then needs to set this before calling
pm_genpd_init(). Similar as we are dealing with GENPD_FLAG_PM_CLK.

The requirement for GENPD_FLAG_PERF_STATES, is to have the
->get_performance_state() assigned. This shall be verified during
pm_genpd_init().

The pm_genpd_has_performance_state() then only need to return true, in
cases the device's genpd has GENPD_FLAG_PERF_STATE set - else false.

Regarding ->set_performance_state(), let's just make it optional - and
when trying to set a new performance state, just walk the genpd
hierarchy, from bottom to up, then invoke the callback when it's
assigned.

Kind regards
Uffe

Powered by blists - more mailing lists