From: Rajendra Nayak <rnayak@codeaurora.org>
To: Stanimir Varbanov <stanimir.varbanov@linaro.org>,
viresh.kumar@linaro.org, sboyd@kernel.org,
bjorn.andersson@linaro.org, agross@kernel.org
Cc: linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, mka@chromium.org,
linux-media@vger.kernel.org
Subject: Re: [PATCH v3 12/17] media: venus: core: Add support for opp tables/perf voting
Date: Wed, 29 Apr 2020 20:40:42 +0530 [thread overview]
Message-ID: <425a8828-be17-fe79-99f5-9e20eff51b13@codeaurora.org> (raw)
In-Reply-To: <b091dc29-d2e8-ed3f-fe1c-ae60e16d5d78@linaro.org>
Hey Stan,
On 4/29/2020 8:06 PM, Stanimir Varbanov wrote:
> Hi Rajendra,
>
> Thanks for the patch!
>
[]..
>> #include <linux/pm_runtime.h>
>> #include <media/videobuf2-v4l2.h>
>> #include <media/v4l2-mem2mem.h>
>> @@ -214,6 +215,20 @@ static int venus_probe(struct platform_device *pdev)
>> if (!core->pm_ops)
>> return -ENODEV;
>>
>> + core->opp_table = dev_pm_opp_set_clkname(dev, "core");
>
> Should we set opp clkname if opp_of_add_table fails? We have platforms
> which don't have opp tables in Venus DT node. We have to be backward
> compatible for them.
so the way its designed, you are expected to call dev_pm_opp_set_clkname()
*before* adding any OPPs from the OPP table.
As for backward compatibility its already handled by the OPP core now [1]
which makes sure dev_pm_opp_set_rate() is equivalent to a clk_set_rate()
in case of a missing OPP table.
>> + if (IS_ERR(core->opp_table))
>> + return PTR_ERR(core->opp_table);
>> +
>> + if (core->res->opp_pmdomain) {
>> + ret = dev_pm_opp_of_add_table(dev);
>> + if (!ret) {
>> + core->has_opp_table = true;
>> + } else if (ret != -ENODEV) {
>
> Is it possible dev_pm_opp_of_add_table() to return EPROBE_DEFER?
Nope, it does not, I had checked.
>> + dev_err(dev, "Invalid OPP table in Device tree\n");
>
> ... if so, please drop dev_err.
>
>> + return ret;
>> + }
>> + }
[]..
>> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
>> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
>> @@ -9,6 +9,7 @@
>> #include <linux/iopoll.h>
>> #include <linux/kernel.h>
>> #include <linux/pm_domain.h>
>> +#include <linux/pm_opp.h>
>> #include <linux/pm_runtime.h>
>> #include <linux/types.h>
>> #include <media/v4l2-mem2mem.h>
>> @@ -66,10 +67,9 @@ static void core_clks_disable(struct venus_core *core)
>>
>> static int core_clks_set_rate(struct venus_core *core, unsigned long freq)
>> {
>> - struct clk *clk = core->clks[0];
>> int ret;
>>
>> - ret = clk_set_rate(clk, freq);
>> + ret = dev_pm_opp_set_rate(core->dev, freq);
>
> Is this will work for legacy platforms without OPP tables?
yes, see [1] which is already merged in mainline.
>
> Also what about the other clocks (vicodec0,1) in this function.
We continue to use clk_set_rate() for those. The performance state
is associated with only the core clk.
>> if (ret)
>> return ret;
>>
>> @@ -740,13 +740,15 @@ static int venc_power_v4(struct device *dev, int on)
>>
>> static int vcodec_domains_get(struct device *dev)
>> {
>> + struct opp_table *opp_table;
>> + struct device **opp_virt_dev;
>> struct venus_core *core = dev_get_drvdata(dev);
>> const struct venus_resources *res = core->res;
>> struct device *pd;
>> unsigned int i;
>>
>> if (!res->vcodec_pmdomains_num)
>> - return -ENODEV;
>> + goto skip_pmdomains;
>>
>> for (i = 0; i < res->vcodec_pmdomains_num; i++) {
>> pd = dev_pm_domain_attach_by_name(dev,
>> @@ -763,6 +765,24 @@ static int vcodec_domains_get(struct device *dev)
>> if (!core->pd_dl_venus)
>> return -ENODEV;
>>
>> +skip_pmdomains:
>> + if (!res->opp_pmdomain || !core->has_opp_table)
>> + return 0;
>> +
>> + /* Attach the power domain for setting performance state */
>> + opp_table = dev_pm_opp_attach_genpd(dev, res->opp_pmdomain, &opp_virt_dev);
>> + if (IS_ERR(opp_table)) {
>> + return PTR_ERR(opp_table);
>> + } else if (opp_virt_dev) {
>> + core->opp_pmdomain = *opp_virt_dev;
>> + core->opp_dl_venus = device_link_add(dev, core->opp_pmdomain,
>> + DL_FLAG_RPM_ACTIVE |
>> + DL_FLAG_PM_RUNTIME |
>> + DL_FLAG_STATELESS);
>> + if (!core->opp_dl_venus)
>> + return -ENODEV;
>
> I think as you return ENODEV you have to detach opp domain here because
> vcodec_domains_put() is not called in error path.
Ok, I'll fix that up.
Thanks for the review.
[1] https://lkml.org/lkml/2020/4/8/413
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
next prev parent reply other threads:[~2020-04-29 15:10 UTC|newest]
Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-28 13:32 [PATCH v3 00/17] DVFS for IO devices on sdm845 and sc7180 Rajendra Nayak
2020-04-28 13:32 ` [PATCH v3 01/17] tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state Rajendra Nayak
2020-04-28 22:49 ` Matthias Kaehlcke
2020-04-28 13:32 ` [PATCH v3 02/17] spi: spi-geni-qcom: " Rajendra Nayak
2020-04-28 23:04 ` Matthias Kaehlcke
2020-04-28 13:32 ` [PATCH v3 03/17] arm64: dts: sdm845: Add OPP table for all qup devices Rajendra Nayak
2020-04-29 0:02 ` Matthias Kaehlcke
2020-04-29 14:15 ` Rajendra Nayak
2020-04-29 14:53 ` Rajendra Nayak
2020-04-29 16:10 ` Matthias Kaehlcke
2020-04-29 16:38 ` Rajendra Nayak
2020-04-30 6:15 ` Viresh Kumar
2020-04-28 13:32 ` [PATCH v3 04/17] arm64: dts: sc7180: " Rajendra Nayak
2020-06-25 15:17 ` Matthias Kaehlcke
2020-06-29 11:24 ` Rajendra Nayak
2020-04-28 13:32 ` [PATCH v3 05/17] drm/msm/dpu: Use OPP API to set clk/perf state Rajendra Nayak
2020-04-28 16:32 ` Rob Clark
2020-04-29 14:07 ` Rajendra Nayak
2020-04-29 0:14 ` Matthias Kaehlcke
2020-04-29 14:16 ` Rajendra Nayak
2020-04-28 13:32 ` [PATCH v3 06/17] drm/msm: dsi: " Rajendra Nayak
2020-04-28 13:32 ` [PATCH v3 07/17] arm64: dts: sdm845: Add DSI and MDP OPP tables and power-domains Rajendra Nayak
2020-04-29 0:27 ` Matthias Kaehlcke
2020-04-29 14:18 ` Rajendra Nayak
2020-04-28 13:32 ` [PATCH v3 08/17] arm64: dts: sc7180: " Rajendra Nayak
2020-04-28 13:32 ` [PATCH v3 09/17] mmc: sdhci-msm: Fix error handling for dev_pm_opp_of_add_table() Rajendra Nayak
2020-04-28 18:29 ` Ulf Hansson
2020-04-29 14:09 ` Rajendra Nayak
2020-05-05 11:33 ` Ulf Hansson
2020-05-05 13:32 ` Rajendra Nayak
2020-04-28 13:32 ` [PATCH v3 10/17] arm64: dts: sdm845: Add sdhc opps and power-domains Rajendra Nayak
2020-04-28 13:32 ` [PATCH v3 11/17] arm64: dts: sc7180: " Rajendra Nayak
2020-04-28 13:33 ` [PATCH v3 12/17] media: venus: core: Add support for opp tables/perf voting Rajendra Nayak
2020-04-29 0:39 ` Matthias Kaehlcke
2020-04-29 14:19 ` Rajendra Nayak
2020-04-29 14:36 ` Stanimir Varbanov
2020-04-29 15:10 ` Rajendra Nayak [this message]
2020-04-28 13:33 ` [PATCH v3 13/17] arm64: dts: sdm845: Add OPP tables and power-domains for venus Rajendra Nayak
2020-04-29 0:42 ` Matthias Kaehlcke
2020-04-28 13:33 ` [PATCH v3 14/17] arm64: dts: sc7180: " Rajendra Nayak
2020-04-28 13:33 ` [PATCH v3 15/17] spi: spi-qcom-qspi: Use OPP API to set clk/perf state Rajendra Nayak
2020-04-29 0:49 ` Matthias Kaehlcke
2020-04-29 14:21 ` Rajendra Nayak
2020-04-28 13:33 ` [PATCH v3 16/17] arm64: dts: sdm845: Add qspi opps and power-domains Rajendra Nayak
2020-04-29 0:52 ` Matthias Kaehlcke
2020-04-28 13:33 ` [PATCH v3 17/17] arm64: dts: sc7180: " Rajendra Nayak
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=425a8828-be17-fe79-99f5-9e20eff51b13@codeaurora.org \
--to=rnayak@codeaurora.org \
--cc=agross@kernel.org \
--cc=bjorn.andersson@linaro.org \
--cc=devicetree@vger.kernel.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mka@chromium.org \
--cc=sboyd@kernel.org \
--cc=stanimir.varbanov@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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).