Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1583818 > unrolled thread

Re: [PATCH V2 4/6] PM / domain: Register for PM QOS performance notifier

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V2 4/6] PM / domain: Register for PM QOS performance notifier Kevin Hilman <khilman@baylibre.com> - 2017-02-18 01:10 +0100
    Re: [PATCH V2 4/6] PM / domain: Register for PM QOS performance  notifier Viresh Kumar <viresh.kumar@linaro.org> - 2017-02-20 06:10 +0100
      Re: [PATCH V2 4/6] PM / domain: Register for PM QOS performance notifier Ulf Hansson <ulf.hansson@linaro.org> - 2017-02-21 16:30 +0100
        Re: [PATCH V2 4/6] PM / domain: Register for PM QOS performance  notifier Viresh Kumar <viresh.kumar@linaro.org> - 2017-02-22 04:30 +0100

#1583818 — Re: [PATCH V2 4/6] PM / domain: Register for PM QOS performance notifier

Viresh Kumar <viresh.kumar@linaro.org> writes:

> Some platforms have the capability to configure the performance state of
> their Power Domains. The performance levels are represented by positive
> integer values, a lower value represents lower performance state.
>
> This patch registers the power domain framework for PM QOS performance
> notifier in order to manage performance state of power domains.

It seems to me it doesm't just register, but actually keeps track of the
performance_state by always tracking the max.

> This also allows the power domain drivers to implement a
> ->set_performance_state() callback, which will be called by the power
> domain core from the notifier routine.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/base/power/domain.c | 98 ++++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/pm_domain.h   |  5 +++
>  2 files changed, 101 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index a73d79670a64..1158a07f92de 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -367,6 +367,88 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb,
>  	return NOTIFY_DONE;
>  }
>  
> +static void update_domain_performance_state(struct generic_pm_domain *genpd,
> +					    int depth)
> +{
> +	struct generic_pm_domain_data *pd_data;
> +	struct generic_pm_domain *subdomain;
> +	struct pm_domain_data *pdd;
> +	unsigned int state = 0;
> +	struct gpd_link *link;
> +
> +	/* Traverse all devices within the domain */
> +	list_for_each_entry(pdd, &genpd->dev_list, list_node) {
> +		pd_data = to_gpd_data(pdd);
> +
> +		if (pd_data->performance_state > state)
> +			state = pd_data->performance_state;
> +	}

This seems to only update the state if it's bigger.  Maybe I'm missing
something here, but it seems like won't be able to lower the
performance_state after it's been raised?

> +	/* Traverse all subdomains within the domain */
> +	list_for_each_entry(link, &genpd->master_links, master_node) {
> +		subdomain = link->slave;
> +
> +		if (subdomain->performance_state > state)
> +			state = subdomain->performance_state;
> +	}

So subdomains are always assumed to influence the performance_state of
the parent domains?  Is that always the case?  I suspect this should be
probably be a reasonable default assumption, but maybe controlled with a
flag.

> +	if (genpd->performance_state == state)
> +		return;
> +
> +	genpd->performance_state = state;
> +
> +	if (genpd->set_performance_state) {
> +		genpd->set_performance_state(genpd, state);
> +		return;
> +	}

So is zero not a valid performance_state?  That doesn't seem quite right
to me, but either way, it should be documented.

> +	/* Propagate only if this domain has a single parent */

Why?  This limitation should be explained in the cover letter and
changelog.  I would also expect some sort of WARN here since this could
otherwise be a rather silent failures.

[...]

Kevin

[toc] | [next] | [standalone]

#1584353 — Re: [PATCH V2 4/6] PM / domain: Register for PM QOS performance notifier

On 17-02-17, 15:54, Kevin Hilman wrote:
> Viresh Kumar <viresh.kumar@linaro.org> writes:
> 
> > Some platforms have the capability to configure the performance state of
> > their Power Domains. The performance levels are represented by positive
> > integer values, a lower value represents lower performance state.
> >
> > This patch registers the power domain framework for PM QOS performance
> > notifier in order to manage performance state of power domains.
> 
> It seems to me it doesm't just register, but actually keeps track of the
> performance_state by always tracking the max.

Yes. Will update the commit log to make sure it is clear.

> > This also allows the power domain drivers to implement a
> > ->set_performance_state() callback, which will be called by the power
> > domain core from the notifier routine.
> >
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> >  drivers/base/power/domain.c | 98 ++++++++++++++++++++++++++++++++++++++++++++-
> >  include/linux/pm_domain.h   |  5 +++
> >  2 files changed, 101 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index a73d79670a64..1158a07f92de 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -367,6 +367,88 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb,
> >  	return NOTIFY_DONE;
> >  }
> >  
> > +static void update_domain_performance_state(struct generic_pm_domain *genpd,
> > +					    int depth)
> > +{
> > +	struct generic_pm_domain_data *pd_data;
> > +	struct generic_pm_domain *subdomain;
> > +	struct pm_domain_data *pdd;
> > +	unsigned int state = 0;
> > +	struct gpd_link *link;
> > +
> > +	/* Traverse all devices within the domain */
> > +	list_for_each_entry(pdd, &genpd->dev_list, list_node) {
> > +		pd_data = to_gpd_data(pdd);
> > +
> > +		if (pd_data->performance_state > state)
> > +			state = pd_data->performance_state;
> > +	}
> 
> This seems to only update the state if it's bigger.  Maybe I'm missing
> something here, but it seems like won't be able to lower the
> performance_state after it's been raised?

'state' is initialized to 0 to begin with and then the above loop
finds the highest state requested so far. The below code (after the
below loop) then changes the state if it isn't equal to the previous
one. That is, it would update the state if the new target state is
lower than the current one. Or am I missing a bug in there ?

> > +	/* Traverse all subdomains within the domain */
> > +	list_for_each_entry(link, &genpd->master_links, master_node) {
> > +		subdomain = link->slave;
> > +
> > +		if (subdomain->performance_state > state)
> > +			state = subdomain->performance_state;
> > +	}
> 
> So subdomains are always assumed to influence the performance_state of
> the parent domains?  Is that always the case? 

It is true for the hardware we have to work on right now, but I would
assume that being the most common case.

> I suspect this should be
> probably be a reasonable default assumption, but maybe controlled with a
> flag.

Yeah, maybe we can have a flag to ignore a subdomain while doing the
calculations, but I would rather defer that to the first platform that
needs it and leave it as is for now. I am afraid that code may never
get used and maybe we should wait for a real case first.

> > +	if (genpd->performance_state == state)
> > +		return;
> > +
> > +	genpd->performance_state = state;
> > +
> > +	if (genpd->set_performance_state) {
> > +		genpd->set_performance_state(genpd, state);
> > +		return;
> > +	}
> 
> So is zero not a valid performance_state?

Zero is a *valid* performance state. Are you getting confused with the
above 'if' block which checks for a valid set_performance_state()
callback and not the performance level ?

> > +	/* Propagate only if this domain has a single parent */
> 
> Why?  This limitation should be explained in the cover letter and
> changelog.  I would also expect some sort of WARN here since this could
> otherwise be a rather silent failures.

I think this limitation can just be removed, but I am confused a bit.

I thought that support for multiple parent domains isn't yet there,
isn't it? And that few people are trying to add it in kernel and the
stuff is still under review. Like this thread:

https://lkml.org/lkml/2016/11/22/792

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1585417

[...]

>
>> > +   /* Propagate only if this domain has a single parent */
>>
>> Why?  This limitation should be explained in the cover letter and
>> changelog.  I would also expect some sort of WARN here since this could
>> otherwise be a rather silent failures.
>
> I think this limitation can just be removed, but I am confused a bit.
>
> I thought that support for multiple parent domains isn't yet there,
> isn't it? And that few people are trying to add it in kernel and the
> stuff is still under review. Like this thread:

A genpd PM domain can have several parents (or to use the genpd
terminology, masters).

This is different from allowing a device to be attached to more than
one PM domains, which is what I think you are referring to. That isn't
supported.

>
> https://lkml.org/lkml/2016/11/22/792
>
> --
> viresh

Kind regards
Uffe

[toc] | [prev] | [next] | [standalone]

#1585906 — Re: [PATCH V2 4/6] PM / domain: Register for PM QOS performance notifier

On 21-02-17, 16:28, Ulf Hansson wrote:
> A genpd PM domain can have several parents (or to use the genpd
> terminology, masters).
> 
> This is different from allowing a device to be attached to more than
> one PM domains, which is what I think you are referring to. That isn't
> supported.

Thanks for clarifying. Got it now.

-- 
viresh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web