Linux-OMAP Archive on lore.kernel.org
 help / color / Atom feed
From: Lukasz Luba <lukasz.luba@arm.com>
To: Quentin Perret <qperret@google.com>
Cc: linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	dri-devel@lists.freedesktop.org, linux-omap@vger.kernel.org,
	linux-mediatek@lists.infradead.org,
	linux-arm-msm@vger.kernel.org, linux-imx@nxp.com,
	Dietmar.Eggemann@arm.com, cw00.choi@samsung.com,
	b.zolnierkie@samsung.com, rjw@rjwysocki.net,
	sudeep.holla@arm.com, viresh.kumar@linaro.org, nm@ti.com,
	sboyd@kernel.org, rui.zhang@intel.com,
	amit.kucheria@verdurent.com, daniel.lezcano@linaro.org,
	mingo@redhat.com, peterz@infradead.org, juri.lelli@redhat.com,
	vincent.guittot@linaro.org, rostedt@goodmis.org,
	bsegall@google.com, mgorman@suse.de, shawnguo@kernel.org,
	s.hauer@pengutronix.de, festevam@gmail.com,
	kernel@pengutronix.de, khilman@kernel.org, agross@kernel.org,
	bjorn.andersson@linaro.org, robh@kernel.org,
	matthias.bgg@gmail.com, steven.price@arm.com,
	tomeu.vizoso@collabora.com, alyssa.rosenzweig@collabora.com,
	airlied@linux.ie, daniel@ffwll.ch, liviu.dudau@arm.com,
	lorenzo.pieralisi@arm.com, patrick.bellasi@matbug.net,
	orjan.eide@arm.com, rdunlap@infradead.org, mka@chromium.org
Subject: Re: [PATCH v7 04/15] PM / EM: add support for other devices than CPUs in Energy Model
Date: Tue, 12 May 2020 12:38:40 +0100
Message-ID: <270f3e37-8de8-2841-dca3-8d70089f7317@arm.com> (raw)
In-Reply-To: <20200511134319.GA29112@google.com>



On 5/11/20 2:43 PM, Quentin Perret wrote:
> Hey Lukasz,
> 
> On Monday 11 May 2020 at 12:19:01 (+0100), Lukasz Luba wrote:
> <snip>
>> @@ -27,12 +29,15 @@ struct em_perf_state {
>>    * em_perf_domain - Performance domain
>>    * @table:		List of performance states, in ascending order
>>    * @nr_perf_states:	Number of performance states
>> - * @cpus:		Cpumask covering the CPUs of the domain
>> + * @cpus:		Cpumask covering the CPUs of the domain. It's here
>> + *			for performance reasons to avoid potential cache
>> + *			misses during energy calculations in the scheduler
> 
> And because that saves a pointer, and simplifies allocating/freeing that
> memory region :)

True, I will add this also:
'and simplifies allocating/freeing that memory region'

> 
> <snip>
>> diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
>> index 5b8a1566526a..9cc7f2973600 100644
>> --- a/kernel/power/energy_model.c
>> +++ b/kernel/power/energy_model.c
>> @@ -2,8 +2,9 @@
>>   /*
>>    * Energy Model of CPUs
> 
> Should this comment change too?

Yes, indeed. I will adjust it.

> 
> <snip>
>> -static void em_debug_create_pd(struct em_perf_domain *pd, int cpu)
>> +static void em_debug_create_pd(struct device *dev)
>>   {
>>   	struct dentry *d;
>> -	char name[8];
>>   	int i;
>>   
>> -	snprintf(name, sizeof(name), "pd%d", cpu);
>> -
>>   	/* Create the directory of the performance domain */
>> -	d = debugfs_create_dir(name, rootdir);
>> +	d = debugfs_create_dir(dev_name(dev), rootdir);
> 
> So what will be the name for the perf domain of CPUs now? cpuX?

yeap, it will be 'cpu0', 'cpu4', etc...

> 
> <snip>
>> @@ -142,8 +149,8 @@ em_create_pd(struct device *dev, int nr_states, struct em_data_callback *cb,
>>   		 */
>>   		opp_eff = freq / power;
>>   		if (opp_eff >= prev_opp_eff)
>> -			pr_warn("pd%d: hertz/watts ratio non-monotonically decreasing: em_perf_state %d >= em_perf_state%d\n",
>> -					cpu, i, i - 1);
>> +			dev_dbg(dev, "EM: hertz/watts ratio non-monotonically decreasing: em_perf_state %d >= em_perf_state%d\n",
>> +					i, i - 1);
> 
> It feels like changing from warn to debug doesn't really belong to this
> patch no?

I thought that these prints are not worth to introduce another patch.
This warning is a bit tricky, because we (SW eng) basically are not able
to tweak OPPs, we can only remove them to calm down this warning.

There are platforms, with dozen of OPPs, seeing this. Warnings triggers 
the automated tests scripts, which are sensitive to dmesg log level and
cause developers to spent time and investigate the issue.

Then, what if these OPPs are needed because the thermal was tested OK
with some OPPs which unfortunately are triggering also this warning.
They cannot remove these OPPS, but the warning would stay. We might see
this also for GPUs.

I decided to change it into dbg, due to the reason above.

> 
> <snip>
>> @@ -216,47 +274,50 @@ int em_dev_register_perf_domain(struct device *dev, unsigned int nr_states,
>>   	 */
>>   	mutex_lock(&em_pd_mutex);
>>   
>> -	for_each_cpu(cpu, span) {
>> -		/* Make sure we don't register again an existing domain. */
>> -		if (READ_ONCE(per_cpu(em_data, cpu))) {
>> -			ret = -EEXIST;
>> -			goto unlock;
>> -		}
>> +	if (dev->em_pd) {
>> +		ret = -EEXIST;
>> +		goto unlock;
>> +	}
>>   
>> -		/*
>> -		 * All CPUs of a domain must have the same micro-architecture
>> -		 * since they all share the same table.
>> -		 */
>> -		cap = arch_scale_cpu_capacity(cpu);
>> -		if (prev_cap && prev_cap != cap) {
>> -			pr_err("CPUs of %*pbl must have the same capacity\n",
>> -							cpumask_pr_args(span));
>> +	if (_is_cpu_device(dev)) {
> 
> Something like
> 
> 	if (!_is_cpu_device(dev))
> 		goto device;
> 
> would limit the diff a bit, but that may just be personal taste.

Possible

> 
> But appart from these nits, the patch LGTM.

Thank you for the review.

I will wait for Daniel's (because he suggested the em_pd inside
device struct) comments and if there is no other issues I will just
resend the patch with adjusted comment fields in response.

Regards,
Lukasz

> 
> Thanks,
> Quentin
> 

  reply index

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-11 11:18 [PATCH v7 00/15] Add support for devices in the " Lukasz Luba
2020-05-11 11:18 ` [PATCH v7 01/15] PM / EM: change naming convention from 'capacity' to 'performance' Lukasz Luba
2020-05-11 11:46   ` Quentin Perret
2020-05-11 11:18 ` [PATCH v7 02/15] PM / EM: introduce em_dev_register_perf_domain function Lukasz Luba
2020-05-11 11:51   ` Quentin Perret
2020-05-11 11:19 ` [PATCH v7 03/15] PM / EM: update callback structure and add device pointer Lukasz Luba
2020-05-11 11:57   ` Quentin Perret
2020-05-12 11:11     ` Lukasz Luba
2020-05-11 11:19 ` [PATCH v7 04/15] PM / EM: add support for other devices than CPUs in Energy Model Lukasz Luba
2020-05-11 13:43   ` Quentin Perret
2020-05-12 11:38     ` Lukasz Luba [this message]
2020-05-11 11:19 ` [PATCH v7 05/15] PM / EM: remove em_register_perf_domain Lukasz Luba
2020-05-11 13:44   ` Quentin Perret
2020-05-11 11:19 ` [PATCH v7 06/15] PM / EM: change name of em_pd_energy to em_cpu_energy Lukasz Luba
2020-05-11 13:45   ` Quentin Perret
2020-05-11 11:19 ` [PATCH v7 07/15] Documentation: power: update Energy Model description Lukasz Luba
2020-05-11 13:48   ` Quentin Perret
2020-05-11 11:19 ` [PATCH v7 08/15] OPP: refactor dev_pm_opp_of_register_em() and update related drivers Lukasz Luba
2020-05-11 11:19 ` [PATCH v7 09/15] thermal: devfreq_cooling: change tracing function and arguments Lukasz Luba
2020-05-11 11:19 ` [PATCH v7 10/15] thermal: devfreq_cooling: get device load and frequency directly Lukasz Luba
2020-05-11 11:19 ` [PATCH v7 11/15] thermal: devfreq_cooling: work on a copy of device status Lukasz Luba
2020-05-11 11:19 ` [PATCH v7 12/15] thermal: devfreq_cooling: add new registration functions with Energy Model Lukasz Luba
2020-05-11 11:19 ` [PATCH v7 13/15] thermal: devfreq_cooling: remove old power model and use EM Lukasz Luba
2020-05-11 11:19 ` [PATCH v7 14/15] thermal: devfreq_cooling: update license to use SPDX Lukasz Luba
2020-05-11 11:19 ` [PATCH v7 15/15] drm/panfrost: Register devfreq cooling and attempt to add Energy Model Lukasz Luba
2020-05-22 10:43 ` [PATCH v7 00/15] Add support for devices in the " Daniel Lezcano
2020-05-22 12:58   ` Lukasz Luba
2020-05-22 13:01     ` Daniel Lezcano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=270f3e37-8de8-2841-dca3-8d70089f7317@arm.com \
    --to=lukasz.luba@arm.com \
    --cc=Dietmar.Eggemann@arm.com \
    --cc=agross@kernel.org \
    --cc=airlied@linux.ie \
    --cc=alyssa.rosenzweig@collabora.com \
    --cc=amit.kucheria@verdurent.com \
    --cc=b.zolnierkie@samsung.com \
    --cc=bjorn.andersson@linaro.org \
    --cc=bsegall@google.com \
    --cc=cw00.choi@samsung.com \
    --cc=daniel.lezcano@linaro.org \
    --cc=daniel@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=festevam@gmail.com \
    --cc=juri.lelli@redhat.com \
    --cc=kernel@pengutronix.de \
    --cc=khilman@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-imx@nxp.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=linux-omap@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=liviu.dudau@arm.com \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=matthias.bgg@gmail.com \
    --cc=mgorman@suse.de \
    --cc=mingo@redhat.com \
    --cc=mka@chromium.org \
    --cc=nm@ti.com \
    --cc=orjan.eide@arm.com \
    --cc=patrick.bellasi@matbug.net \
    --cc=peterz@infradead.org \
    --cc=qperret@google.com \
    --cc=rdunlap@infradead.org \
    --cc=rjw@rjwysocki.net \
    --cc=robh@kernel.org \
    --cc=rostedt@goodmis.org \
    --cc=rui.zhang@intel.com \
    --cc=s.hauer@pengutronix.de \
    --cc=sboyd@kernel.org \
    --cc=shawnguo@kernel.org \
    --cc=steven.price@arm.com \
    --cc=sudeep.holla@arm.com \
    --cc=tomeu.vizoso@collabora.com \
    --cc=vincent.guittot@linaro.org \
    --cc=viresh.kumar@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-OMAP Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-omap/0 linux-omap/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-omap linux-omap/ https://lore.kernel.org/linux-omap \
		linux-omap@vger.kernel.org
	public-inbox-index linux-omap

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-omap


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git