With the driver supporting multiple generations of hardware, some mold
has definitely grown over the code..
This series attempts to amend this situation a bit by commonizing some
code paths and fixing some bugs while at it.
Only tested on SM8250.
Definitely needs testing on:
- SDM845 with old bindings
- SDM845 with new bindings or 7180
- MSM8916
- MSM8996
Signed-off-by: Konrad Dybcio <[email protected]>
---
Changes in v2:
- Fix "set but unused" warning in "Drop cache properties in resource struct"
- Fix modular build with "Commonize vdec_get()"
- Rebase
- Test again on 8250, since nobody else tested other platforms since the last
submission (or at least hasn't reported that), I'm assuming nobody cares
- Needs to be tested atop [1] and similar, it's in latest -next already
- Link to v1: https://lore.kernel.org/r/[email protected]
[1] https://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git/commit/?h=for-next&id=d2cd22c9c384aa50c0b4530e842bd078427e6279
---
Konrad Dybcio (20):
media: venus: pm_helpers: Only set rate of the core clock in core_clks_enable
media: venus: pm_helpers: Rename core_clks_get to venus_clks_get
media: venus: pm_helpers: Add kerneldoc to venus_clks_get()
media: venus: core: Set OPP clkname in a common code path
media: venus: pm_helpers: Kill dead code
media: venus: pm_helpers: Move reset acquisition to common code
media: venus: core: Constify all members of the resource struct
media: venus: core: Deduplicate OPP genpd names
media: venus: core: Get rid of vcodec_num
media: venus: core: Drop cache properties in resource struct
media: venus: core: Use GENMASK for dma_mask
media: venus: core: Remove cp_start
media: venus: pm_helpers: Commonize core_power
media: venus: pm_helpers: Remove pm_ops->core_put
media: venus: core: Define a pointer to core->res
media: venus: pm_helpers: Simplify vcodec clock handling
media: venus: pm_helpers: Commonize getting clocks and GenPDs
media: venus: pm_helpers: Commonize vdec_get()
media: venus: pm_helpers: Commonize venc_get()
media: venus: pm_helpers: Use reset_bulk API
drivers/media/platform/qcom/venus/core.c | 139 ++++-------
drivers/media/platform/qcom/venus/core.h | 66 +++--
drivers/media/platform/qcom/venus/firmware.c | 3 +-
drivers/media/platform/qcom/venus/hfi_venus.c | 10 +-
drivers/media/platform/qcom/venus/pm_helpers.c | 323 +++++++++----------------
drivers/media/platform/qcom/venus/pm_helpers.h | 10 +-
drivers/media/platform/qcom/venus/vdec.c | 9 +-
drivers/media/platform/qcom/venus/venc.c | 9 +-
8 files changed, 213 insertions(+), 356 deletions(-)
---
base-commit: 445a555e0623387fa9b94e68e61681717e70200a
change-id: 20230911-topic-mars-e60bb2269411
Best regards,
--
Konrad Dybcio <[email protected]>
"core" is used in multiple contexts when talking about Venus, rename
the function to save on confusion.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/media/platform/qcom/venus/pm_helpers.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index 8bd0ce4ce69d..ac7c83404c6e 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -23,7 +23,7 @@
static bool legacy_binding;
-static int core_clks_get(struct venus_core *core)
+static int venus_clks_get(struct venus_core *core)
{
const struct venus_resources *res = core->res;
struct device *dev = core->dev;
@@ -294,7 +294,7 @@ static int core_get_v1(struct venus_core *core)
{
int ret;
- ret = core_clks_get(core);
+ ret = venus_clks_get(core);
if (ret)
return ret;
@@ -961,7 +961,7 @@ static int core_get_v4(struct venus_core *core)
const struct venus_resources *res = core->res;
int ret;
- ret = core_clks_get(core);
+ ret = venus_clks_get(core);
if (ret)
return ret;
--
2.43.0
Commit c22b1a29497c ("media: venus: core,pm: Vote for min clk freq
during venus boot") intended to up the rate of the Venus core clock
from the XO minimum to something more reasonable, based on the per-
SoC frequency table.
Unfortunately, it ended up calling set_rate with that same argument
on all clocks in res->clks. Fix that using the OPP API.
Fixes: c22b1a29497c ("media: venus: core,pm: Vote for min clk freq during venus boot")
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/media/platform/qcom/venus/pm_helpers.c | 23 +++++++++++------------
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index 502822059498..8bd0ce4ce69d 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -41,24 +41,23 @@ static int core_clks_get(struct venus_core *core)
static int core_clks_enable(struct venus_core *core)
{
const struct venus_resources *res = core->res;
- const struct freq_tbl *freq_tbl = core->res->freq_tbl;
- unsigned int freq_tbl_size = core->res->freq_tbl_size;
- unsigned long freq;
+ struct dev_pm_opp *opp;
+ unsigned long freq = 0;
unsigned int i;
int ret;
- if (!freq_tbl)
- return -EINVAL;
+ if (core->has_opp_table) {
+ opp = dev_pm_opp_find_freq_ceil(core->dev, &freq);
+ if (IS_ERR(opp))
+ return PTR_ERR(opp);
+ dev_pm_opp_put(opp);
- freq = freq_tbl[freq_tbl_size - 1].freq;
+ ret = dev_pm_opp_set_rate(core->dev, freq);
+ if (ret)
+ return ret;
+ }
for (i = 0; i < res->clks_num; i++) {
- if (IS_V6(core)) {
- ret = clk_set_rate(core->clks[i], freq);
- if (ret)
- goto err;
- }
-
ret = clk_prepare_enable(core->clks[i]);
if (ret)
goto err;
--
2.43.0
Calling devm_pm_opp_set_clkname() is repeated for all HFI versions in
pm_ops->core_power.
Move it to the common codepath.
This also lets us get rid of core_get_v1.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/media/platform/qcom/venus/core.c | 5 +++++
drivers/media/platform/qcom/venus/pm_helpers.c | 23 ++---------------------
2 files changed, 7 insertions(+), 21 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index ce206b709754..5ab3c414ec0f 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -14,6 +14,7 @@
#include <linux/of.h>
#include <linux/of_platform.h>
#include <linux/platform_device.h>
+#include <linux/pm_opp.h>
#include <linux/slab.h>
#include <linux/types.h>
#include <linux/pm_domain.h>
@@ -319,6 +320,10 @@ static int venus_probe(struct platform_device *pdev)
if (!core->pm_ops)
return -ENODEV;
+ ret = devm_pm_opp_set_clkname(dev, "core");
+ if (ret)
+ return ret;
+
if (core->pm_ops->core_get) {
ret = core->pm_ops->core_get(core);
if (ret)
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index ea0a7d4601e2..1ba65345a5e2 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -318,21 +318,6 @@ static int load_scale_v1(struct venus_inst *inst)
return ret;
}
-static int core_get_v1(struct venus_core *core)
-{
- int ret;
-
- ret = venus_clks_get(core);
- if (ret)
- return ret;
-
- ret = devm_pm_opp_set_clkname(core->dev, "core");
- if (ret)
- return ret;
-
- return 0;
-}
-
static void core_put_v1(struct venus_core *core)
{
}
@@ -350,7 +335,7 @@ static int core_power_v1(struct venus_core *core, int on)
}
static const struct venus_pm_ops pm_ops_v1 = {
- .core_get = core_get_v1,
+ .core_get = venus_clks_get,
.core_put = core_put_v1,
.core_power = core_power_v1,
.load_scale = load_scale_v1,
@@ -423,7 +408,7 @@ static int venc_power_v3(struct device *dev, int on)
}
static const struct venus_pm_ops pm_ops_v3 = {
- .core_get = core_get_v1,
+ .core_get = venus_clks_get,
.core_put = core_put_v1,
.core_power = core_power_v1,
.vdec_get = vdec_get_v3,
@@ -1013,10 +998,6 @@ static int core_get_v4(struct venus_core *core)
if (legacy_binding)
return 0;
- ret = devm_pm_opp_set_clkname(dev, "core");
- if (ret)
- return ret;
-
ret = vcodec_domains_get(core);
if (ret)
return ret;
--
2.43.0
A situation like:
if (!foo)
goto bar;
for (i = 0; i < foo; i++)
...1...
bar:
...2...
is totally identical to:
for (i = 0; i < 0; i++) // === if (0)
...1...
..2...
Get rid of such boilerplate.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/media/platform/qcom/venus/pm_helpers.c | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index 1ba65345a5e2..7193075e8c04 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -878,14 +878,10 @@ static int vcodec_domains_get(struct venus_core *core)
.pd_flags = PD_FLAG_NO_DEV_LINK,
};
- if (!res->vcodec_pmdomains_num)
- goto skip_pmdomains;
-
ret = dev_pm_domain_attach_list(dev, &vcodec_data, &core->pmdomains);
if (ret < 0)
return ret;
-skip_pmdomains:
if (!core->res->opp_pmdomain)
return 0;
@@ -928,9 +924,6 @@ static int core_resets_reset(struct venus_core *core)
unsigned int i;
int ret;
- if (!res->resets_num)
- return 0;
-
for (i = 0; i < res->resets_num; i++) {
ret = reset_control_assert(core->resets[i]);
if (ret)
@@ -953,9 +946,6 @@ static int core_resets_get(struct venus_core *core)
unsigned int i;
int ret;
- if (!res->resets_num)
- return 0;
-
for (i = 0; i < res->resets_num; i++) {
core->resets[i] =
devm_reset_control_get_exclusive(dev, res->resets[i]);
--
2.43.0
To make it easier to understand the various clock requirements within
this driver, add kerneldoc to venus_clk_get() explaining the fluff.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/media/platform/qcom/venus/pm_helpers.c | 28 ++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index ac7c83404c6e..ea0a7d4601e2 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -23,6 +23,34 @@
static bool legacy_binding;
+/**
+ * venus_clks_get() - Get Venus clocks that are not bound to a vcodec
+ * @core: A pointer to the venus core resource
+ *
+ * The Venus block (depending on the generation) can be split into a couple
+ * of clock domains: one for "main logic" and one for each video core (0-2pcs).
+ *
+ * MSM8916 (and possibly other HFIv1 users) only feature the "main logic"
+ * domain, so this function is the only kind if clk_get necessary there.
+ *
+ * MSM8996 (and other HFIv3 users) feature two video cores, with core0 being
+ * statically proclaimed a decoder and core1 an encoder, with both having
+ * their own clock domains.
+ *
+ * SDM845 features two video cores, each one of which may or may not be
+ * subdivided into 2 enc/dec threads.
+ *
+ * Other SoCs either feature a single video core (with its own clock domain)
+ * or 1 video core and 1 CVP (Computer Vision Processor) core. In both cases
+ * we treat it the same (CVP only happens to live near-by Venus on the SoC).
+ *
+ * Due to unfortunate developments in the past, we have to support bindings
+ * (MSM8996, SDM660, SDM845) that require specifying the clocks and
+ * power-domains associated with a video core domain in a bogus subnode,
+ * which means that additional fluff is necessary..
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
static int venus_clks_get(struct venus_core *core)
{
const struct venus_resources *res = core->res;
--
2.43.0
On 9.02.2024 22:09, Konrad Dybcio wrote:
> With the driver supporting multiple generations of hardware, some mold
> has definitely grown over the code..
>
> This series attempts to amend this situation a bit by commonizing some
> code paths and fixing some bugs while at it.
>
> Only tested on SM8250.
>
> Definitely needs testing on:
>
> - SDM845 with old bindings
> - SDM845 with new bindings or 7180
> - MSM8916
> - MSM8996
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
Apologies for sending this twice. The other submission should be looked at.
Konrad