Re: [PATCH v3 4/4] drm/panfrost: Register to the Energy Model with devfreq device

From: Lukasz Luba <lukasz.luba@arm.com>
To: Rob Herring <robh@kernel.org>
Cc: Nishanth Menon <nm@ti.com>,
	juri.lelli@redhat.com, Peter Zijlstra <peterz@infradead.org>,
	Viresh Kumar <viresh.kumar@linaro.org>,
	Liviu Dudau <liviu.dudau@arm.com>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	bsegall@google.com,
	Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>,
	Fabio Estevam <festevam@gmail.com>,
	Morten Rasmussen <Morten.Rasmussen@arm.com>,
	patrick.bellasi@matbug.net,
	Amit Kucheria <amit.kucheria@verdurent.com>,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Kevin Hilman <khilman@kernel.org>, Andy Gross <agross@kernel.org>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Steven Price <steven.price@arm.com>,
	Chanwoo Choi <cw00.choi@samsung.com>,
	Ingo Molnar <mingo@redhat.com>,
	NXP Linux Team <linux-imx@nxp.com>,
	Zhang Rui <rui.zhang@intel.com>,
	mgorman@suse.de, orjan.eide@arm.com,
	Daniel Vetter <daniel@ffwll.ch>,
	"open list:THERMAL" <linux-pm@vger.kernel.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	Steven Rostedt <rostedt@goodmis.org>,
	"moderated list:ARM/Mediatek SoC support"
	<linux-mediatek@lists.infradead.org>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	linux-omap <linux-omap@vger.kernel.org>,
	Dietmar.Eggemann@arm.com,
	"moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE"
	<linux-arm-kernel@lists.infradead.org>,
	David Airlie <airlied@linux.ie>,
	javi.merino@arm.com, Tomeu Vizoso <tomeu.vizoso@collabora.com>,
	qperret@google.com, Stephen Boyd <sboyd@kernel.org>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
	Sascha Hauer <kernel@pengutronix.de>,
	Sudeep Holla <sudeep.holla@arm.com>,
	Shawn Guo <shawnguo@kernel.org>
Subject: Re: [PATCH v3 4/4] drm/panfrost: Register to the Energy Model with devfreq device
Date: Wed, 26 Feb 2020 10:06:55 +0000	[thread overview]
Message-ID: <96d3287c-4559-7c4a-2f99-0a3aad9e42f6@arm.com> (raw)
In-Reply-To: <CAL_JsqJ1D6Cf2cxdskDS2JCEe8ja6uUeoSpA3i-wxNgi=S1SYA@mail.gmail.com>

Hi Rob,

On 2/25/20 8:57 PM, Rob Herring wrote:
> On Fri, Feb 21, 2020 at 1:48 PM Lukasz Luba <lukasz.luba@arm.com> wrote:
>>
>> Add device to the Energy Model framework. It will create a dedicated
>> and unified data structures used i.e. in the thermal framework.
>> The power model used in dev_pm_opp subsystem is simplified and created
>> based on DT 'dynamic-power-coefficient', volatage and frequency. It is
> 
> typo.

I'll fix it.

> 
>> similar to the CPU model used in Energy Aware Scheduler.
>>
>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>> ---
>>   drivers/gpu/drm/panfrost/panfrost_devfreq.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
>> index 413987038fbf..d527a5113950 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
>> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
>> @@ -105,6 +105,8 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
>>          }
>>          pfdev->devfreq.devfreq = devfreq;
>>
>> +       dev_pm_opp_of_register_em(dev, NULL);
> 
> Can't fail?

Yes, it can fail but the function does not return anything. It can
easily fail, it's looking for "dynamic-power-coefficient" in the device
node. The DT binding for the devfreq devices would also be good to add..

I would have to probably change it into returning 'int' and modify all
old cpufreq drivers.

> 
>> +
>>          cooling = of_devfreq_cooling_register(dev->of_node, devfreq);
>>          if (IS_ERR(cooling))
>>                  DRM_DEV_INFO(dev, "Failed to register cooling device\n");
>> @@ -118,6 +120,7 @@ void panfrost_devfreq_fini(struct panfrost_device *pfdev)
>>   {
>>          if (pfdev->devfreq.cooling)
>>                  devfreq_cooling_unregister(pfdev->devfreq.cooling);
>> +       dev_pm_opp_of_unregister_em(&pfdev->pdev->dev);
>>          dev_pm_opp_of_remove_table(&pfdev->pdev->dev);
> 
> Does it make sense to keep this (and the registration side) as
> separate calls? Perhaps there's some ordering requirement with
> everything between dev_pm_opp_of_add_table() and
> dev_pm_opp_of_register_em()?

Yes, dev_pm_opp_of_register_em() uses em_data_callback which operates
on OPPs to calculate power values and costs, so the the OPP table should
be already there.

> 
> While you're just adding 2 lines, it seems there's a lot of complexity
> exposed to the driver just to initialize devfreq/opp.

It depends, for example devfreq devices like buses would likely never
use the energy model. Potential clients would be GPUs, DSPs, ISPs.

Could you help me with defining a DT binding for this
"dynamic-power-coefficient" entry? It could be used in different types
of devices. Should it be placed in each of these devices documentation
file, or in some one common file?

Thank you for your comments.

Regards,
Lukasz

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek