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]>
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
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
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
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
There is no reason to keep reset_get code local to HFIv4/v6.
Move it to the common part.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/media/platform/qcom/venus/core.c | 9 ++++++++-
drivers/media/platform/qcom/venus/pm_helpers.c | 23 -----------------------
2 files changed, 8 insertions(+), 24 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 5ab3c414ec0f..0652065cb113 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -15,6 +15,7 @@
#include <linux/of_platform.h>
#include <linux/platform_device.h>
#include <linux/pm_opp.h>
+#include <linux/reset.h>
#include <linux/slab.h>
#include <linux/types.h>
#include <linux/pm_domain.h>
@@ -286,7 +287,7 @@ static int venus_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct venus_core *core;
- int ret;
+ int i, ret;
core = devm_kzalloc(dev, sizeof(*core), GFP_KERNEL);
if (!core)
@@ -324,6 +325,12 @@ static int venus_probe(struct platform_device *pdev)
if (ret)
return ret;
+ for (i = 0; i < core->res->resets_num; i++) {
+ core->resets[i] = devm_reset_control_get_exclusive(dev, core->res->resets[i]);
+ if (IS_ERR(core->resets[i]))
+ return PTR_ERR(core->resets[i]);
+ }
+
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 7193075e8c04..6017a9236bff 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -939,25 +939,6 @@ static int core_resets_reset(struct venus_core *core)
return ret;
}
-static int core_resets_get(struct venus_core *core)
-{
- struct device *dev = core->dev;
- const struct venus_resources *res = core->res;
- unsigned int i;
- int ret;
-
- for (i = 0; i < res->resets_num; i++) {
- core->resets[i] =
- devm_reset_control_get_exclusive(dev, res->resets[i]);
- if (IS_ERR(core->resets[i])) {
- ret = PTR_ERR(core->resets[i]);
- return ret;
- }
- }
-
- return 0;
-}
-
static int core_get_v4(struct venus_core *core)
{
struct device *dev = core->dev;
@@ -981,10 +962,6 @@ static int core_get_v4(struct venus_core *core)
if (ret)
return ret;
- ret = core_resets_get(core);
- if (ret)
- return ret;
-
if (legacy_binding)
return 0;
--
2.43.0
Nothing inside the resource struct needs to be mutable. Sprinkle
'const' all over it. A lot of 'const'.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/media/platform/qcom/venus/core.h | 58 ++++++++++++++++----------------
1 file changed, 29 insertions(+), 29 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 6a77de374454..6b1887f7d9cb 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -56,39 +56,39 @@ enum vpu_version {
};
struct venus_resources {
- u64 dma_mask;
- const struct freq_tbl *freq_tbl;
- unsigned int freq_tbl_size;
- const struct bw_tbl *bw_tbl_enc;
- unsigned int bw_tbl_enc_size;
- const struct bw_tbl *bw_tbl_dec;
- unsigned int bw_tbl_dec_size;
- const struct reg_val *reg_tbl;
- unsigned int reg_tbl_size;
- const struct hfi_ubwc_config *ubwc_conf;
+ const u64 dma_mask;
+ const struct freq_tbl * const freq_tbl;
+ const unsigned int freq_tbl_size;
+ const struct bw_tbl * const bw_tbl_enc;
+ const unsigned int bw_tbl_enc_size;
+ const struct bw_tbl * const bw_tbl_dec;
+ const unsigned int bw_tbl_dec_size;
+ const struct reg_val * const reg_tbl;
+ const unsigned int reg_tbl_size;
+ const struct hfi_ubwc_config * const ubwc_conf;
const char * const clks[VIDC_CLKS_NUM_MAX];
- unsigned int clks_num;
+ const unsigned int clks_num;
const char * const vcodec0_clks[VIDC_VCODEC_CLKS_NUM_MAX];
const char * const vcodec1_clks[VIDC_VCODEC_CLKS_NUM_MAX];
- unsigned int vcodec_clks_num;
- const char **vcodec_pmdomains;
- unsigned int vcodec_pmdomains_num;
- const char **opp_pmdomain;
- unsigned int vcodec_num;
+ const unsigned int vcodec_clks_num;
+ const char * const *vcodec_pmdomains;
+ const unsigned int vcodec_pmdomains_num;
+ const char * const * const opp_pmdomain;
+ const unsigned int vcodec_num;
const char * const resets[VIDC_RESETS_NUM_MAX];
- unsigned int resets_num;
- enum hfi_version hfi_version;
- enum vpu_version vpu_version;
- u8 num_vpp_pipes;
- u32 max_load;
- unsigned int vmem_id;
- u32 vmem_size;
- u32 vmem_addr;
- u32 cp_start;
- u32 cp_size;
- u32 cp_nonpixel_start;
- u32 cp_nonpixel_size;
- const char *fwname;
+ const unsigned int resets_num;
+ const enum hfi_version hfi_version;
+ const enum vpu_version vpu_version;
+ const u8 num_vpp_pipes;
+ const u32 max_load;
+ const unsigned int vmem_id;
+ const u32 vmem_size;
+ const u32 vmem_addr;
+ const u32 cp_start;
+ const u32 cp_size;
+ const u32 cp_nonpixel_start;
+ const u32 cp_nonpixel_size;
+ const char * const fwname;
};
enum venus_fmt {
--
2.43.0
That field was only introduced to differentiate between the legacy and
non-legacy SDM845 binding. Get rid of it.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/media/platform/qcom/venus/core.c | 5 -----
drivers/media/platform/qcom/venus/core.h | 1 -
drivers/media/platform/qcom/venus/pm_helpers.c | 2 +-
3 files changed, 1 insertion(+), 7 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 83ac68f1566f..1307aa9cf951 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -651,7 +651,6 @@ static const struct venus_resources sdm660_res = {
.vcodec0_clks = { "vcodec0_core" },
.vcodec1_clks = { "vcodec0_core" },
.vcodec_clks_num = 1,
- .vcodec_num = 1,
.max_load = 1036800,
.hfi_version = HFI_VERSION_3XX,
.vmem_id = VIDC_RESOURCE_NONE,
@@ -725,7 +724,6 @@ static const struct venus_resources sdm845_res_v2 = {
.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0", "vcodec1" },
.vcodec_pmdomains_num = 3,
.opp_pmdomain = pd_names_cx,
- .vcodec_num = 2,
.max_load = 3110400, /* 4096x2160@90 */
.hfi_version = HFI_VERSION_4XX,
.vpu_version = VPU_VERSION_AR50,
@@ -774,7 +772,6 @@ static const struct venus_resources sc7180_res = {
.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
.vcodec_pmdomains_num = 2,
.opp_pmdomain = pd_names_cx,
- .vcodec_num = 1,
.hfi_version = HFI_VERSION_4XX,
.vpu_version = VPU_VERSION_AR50,
.vmem_id = VIDC_RESOURCE_NONE,
@@ -831,7 +828,6 @@ static const struct venus_resources sm8250_res = {
.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
.vcodec_pmdomains_num = 2,
.opp_pmdomain = pd_names_mx,
- .vcodec_num = 1,
.max_load = 7833600,
.hfi_version = HFI_VERSION_6XX,
.vpu_version = VPU_VERSION_IRIS2,
@@ -890,7 +886,6 @@ static const struct venus_resources sc7280_res = {
.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
.vcodec_pmdomains_num = 2,
.opp_pmdomain = pd_names_cx,
- .vcodec_num = 1,
.hfi_version = HFI_VERSION_6XX,
.vpu_version = VPU_VERSION_IRIS2_1,
.num_vpp_pipes = 1,
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 6b1887f7d9cb..22f998637618 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -74,7 +74,6 @@ struct venus_resources {
const char * const *vcodec_pmdomains;
const unsigned int vcodec_pmdomains_num;
const char * const * const opp_pmdomain;
- const unsigned int vcodec_num;
const char * const resets[VIDC_RESETS_NUM_MAX];
const unsigned int resets_num;
const enum hfi_version hfi_version;
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index 6017a9236bff..8412deb68ed1 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -622,7 +622,7 @@ min_loaded_core(struct venus_inst *inst, u32 *min_coreid, u32 *min_load, bool lo
VIDC_CORE_ID_1 : VIDC_CORE_ID_2;
*min_load = min(core1_load, core2_load);
- if (cores_max < VIDC_CORE_ID_2 || core->res->vcodec_num < 2) {
+ if (cores_max < VIDC_CORE_ID_2 || legacy_binding) {
*min_coreid = VIDC_CORE_ID_1;
*min_load = core1_load;
}
--
2.43.0
Currently VMEM/OCMEM/LLCC is disabled on all platforms.
Make it unconditional to save on space.
These caches will not be enabled until the Venus driver can reference
them as chunks of SRAM (they're modelled as separate devices) to avoid
hardcoding magic addresses and rougely accessing the hardware,
bypassing the normal accessors.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/media/platform/qcom/venus/core.c | 24 ------------------------
drivers/media/platform/qcom/venus/core.h | 3 ---
drivers/media/platform/qcom/venus/hfi_venus.c | 10 ++++------
3 files changed, 4 insertions(+), 33 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 1307aa9cf951..43105e765c53 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -562,9 +562,6 @@ static const struct venus_resources msm8916_res = {
.clks_num = 3,
.max_load = 352800, /* 720p@30 + 1080p@30 */
.hfi_version = HFI_VERSION_1XX,
- .vmem_id = VIDC_RESOURCE_NONE,
- .vmem_size = 0,
- .vmem_addr = 0,
.dma_mask = 0xddc00000 - 1,
.fwname = "qcom/venus-1.8/venus.mbn",
};
@@ -595,9 +592,6 @@ static const struct venus_resources msm8996_res = {
.vcodec_clks_num = 1,
.max_load = 2563200,
.hfi_version = HFI_VERSION_3XX,
- .vmem_id = VIDC_RESOURCE_NONE,
- .vmem_size = 0,
- .vmem_addr = 0,
.dma_mask = 0xddc00000 - 1,
.fwname = "qcom/venus-4.2/venus.mbn",
};
@@ -653,9 +647,6 @@ static const struct venus_resources sdm660_res = {
.vcodec_clks_num = 1,
.max_load = 1036800,
.hfi_version = HFI_VERSION_3XX,
- .vmem_id = VIDC_RESOURCE_NONE,
- .vmem_size = 0,
- .vmem_addr = 0,
.cp_start = 0,
.cp_size = 0x79000000,
.cp_nonpixel_start = 0x1000000,
@@ -702,9 +693,6 @@ static const struct venus_resources sdm845_res = {
.max_load = 3110400, /* 4096x2160@90 */
.hfi_version = HFI_VERSION_4XX,
.vpu_version = VPU_VERSION_AR50,
- .vmem_id = VIDC_RESOURCE_NONE,
- .vmem_size = 0,
- .vmem_addr = 0,
.dma_mask = 0xe0000000 - 1,
.fwname = "qcom/venus-5.2/venus.mbn",
};
@@ -727,9 +715,6 @@ static const struct venus_resources sdm845_res_v2 = {
.max_load = 3110400, /* 4096x2160@90 */
.hfi_version = HFI_VERSION_4XX,
.vpu_version = VPU_VERSION_AR50,
- .vmem_id = VIDC_RESOURCE_NONE,
- .vmem_size = 0,
- .vmem_addr = 0,
.dma_mask = 0xe0000000 - 1,
.cp_start = 0,
.cp_size = 0x70800000,
@@ -774,9 +759,6 @@ static const struct venus_resources sc7180_res = {
.opp_pmdomain = pd_names_cx,
.hfi_version = HFI_VERSION_4XX,
.vpu_version = VPU_VERSION_AR50,
- .vmem_id = VIDC_RESOURCE_NONE,
- .vmem_size = 0,
- .vmem_addr = 0,
.dma_mask = 0xe0000000 - 1,
.cp_start = 0,
.cp_size = 0x70800000,
@@ -832,9 +814,6 @@ static const struct venus_resources sm8250_res = {
.hfi_version = HFI_VERSION_6XX,
.vpu_version = VPU_VERSION_IRIS2,
.num_vpp_pipes = 4,
- .vmem_id = VIDC_RESOURCE_NONE,
- .vmem_size = 0,
- .vmem_addr = 0,
.dma_mask = 0xe0000000 - 1,
.fwname = "qcom/vpu-1.0/venus.mbn",
};
@@ -889,9 +868,6 @@ static const struct venus_resources sc7280_res = {
.hfi_version = HFI_VERSION_6XX,
.vpu_version = VPU_VERSION_IRIS2_1,
.num_vpp_pipes = 1,
- .vmem_id = VIDC_RESOURCE_NONE,
- .vmem_size = 0,
- .vmem_addr = 0,
.dma_mask = 0xe0000000 - 1,
.cp_start = 0,
.cp_size = 0x25800000,
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 22f998637618..b1d0687d294f 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -80,9 +80,6 @@ struct venus_resources {
const enum vpu_version vpu_version;
const u8 num_vpp_pipes;
const u32 max_load;
- const unsigned int vmem_id;
- const u32 vmem_size;
- const u32 vmem_addr;
const u32 cp_start;
const u32 cp_size;
const u32 cp_nonpixel_start;
diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index f9437b6412b9..42ff96f71235 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -1067,17 +1067,14 @@ static void venus_process_msg_sys_error(struct venus_hfi_device *hdev,
static irqreturn_t venus_isr_thread(struct venus_core *core)
{
struct venus_hfi_device *hdev = to_hfi_priv(core);
- const struct venus_resources *res;
void *pkt;
u32 msg_ret;
if (!hdev)
return IRQ_NONE;
- res = hdev->core->res;
pkt = hdev->pkt_buf;
-
while (!venus_iface_msgq_read(hdev, pkt)) {
msg_ret = hfi_process_msg_packet(core, pkt);
switch (msg_ret) {
@@ -1085,9 +1082,10 @@ static irqreturn_t venus_isr_thread(struct venus_core *core)
venus_process_msg_sys_error(hdev, pkt);
break;
case HFI_MSG_SYS_INIT:
- venus_hfi_core_set_resource(core, res->vmem_id,
- res->vmem_size,
- res->vmem_addr,
+ /* Disable OCMEM/VMEM unconditionally until support is added */
+ venus_hfi_core_set_resource(core, VIDC_RESOURCE_NONE,
+ 0,
+ 0,
hdev);
break;
case HFI_MSG_SYS_RELEASE_RESOURCE:
--
2.43.0
The raw literals mean very little. Substitute it with more telling
bitops macros.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/media/platform/qcom/venus/core.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 43105e765c53..06b78e98cebd 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -562,7 +562,7 @@ static const struct venus_resources msm8916_res = {
.clks_num = 3,
.max_load = 352800, /* 720p@30 + 1080p@30 */
.hfi_version = HFI_VERSION_1XX,
- .dma_mask = 0xddc00000 - 1,
+ .dma_mask = (GENMASK(31, 30) | GENMASK(28, 26) | GENMASK(24, 22)) - 1,
.fwname = "qcom/venus-1.8/venus.mbn",
};
@@ -592,7 +592,7 @@ static const struct venus_resources msm8996_res = {
.vcodec_clks_num = 1,
.max_load = 2563200,
.hfi_version = HFI_VERSION_3XX,
- .dma_mask = 0xddc00000 - 1,
+ .dma_mask = (GENMASK(31, 30) | GENMASK(28, 26) | GENMASK(24, 22)) - 1,
.fwname = "qcom/venus-4.2/venus.mbn",
};
@@ -693,7 +693,7 @@ static const struct venus_resources sdm845_res = {
.max_load = 3110400, /* 4096x2160@90 */
.hfi_version = HFI_VERSION_4XX,
.vpu_version = VPU_VERSION_AR50,
- .dma_mask = 0xe0000000 - 1,
+ .dma_mask = GENMASK(31, 29) - 1,
.fwname = "qcom/venus-5.2/venus.mbn",
};
@@ -715,7 +715,7 @@ static const struct venus_resources sdm845_res_v2 = {
.max_load = 3110400, /* 4096x2160@90 */
.hfi_version = HFI_VERSION_4XX,
.vpu_version = VPU_VERSION_AR50,
- .dma_mask = 0xe0000000 - 1,
+ .dma_mask = GENMASK(31, 29) - 1,
.cp_start = 0,
.cp_size = 0x70800000,
.cp_nonpixel_start = 0x1000000,
@@ -759,7 +759,7 @@ static const struct venus_resources sc7180_res = {
.opp_pmdomain = pd_names_cx,
.hfi_version = HFI_VERSION_4XX,
.vpu_version = VPU_VERSION_AR50,
- .dma_mask = 0xe0000000 - 1,
+ .dma_mask = GENMASK(31, 29) - 1,
.cp_start = 0,
.cp_size = 0x70800000,
.cp_nonpixel_start = 0x1000000,
@@ -814,7 +814,7 @@ static const struct venus_resources sm8250_res = {
.hfi_version = HFI_VERSION_6XX,
.vpu_version = VPU_VERSION_IRIS2,
.num_vpp_pipes = 4,
- .dma_mask = 0xe0000000 - 1,
+ .dma_mask = GENMASK(31, 29) - 1,
.fwname = "qcom/vpu-1.0/venus.mbn",
};
@@ -868,7 +868,7 @@ static const struct venus_resources sc7280_res = {
.hfi_version = HFI_VERSION_6XX,
.vpu_version = VPU_VERSION_IRIS2_1,
.num_vpp_pipes = 1,
- .dma_mask = 0xe0000000 - 1,
+ .dma_mask = GENMASK(31, 29) - 1,
.cp_start = 0,
.cp_size = 0x25800000,
.cp_nonpixel_start = 0x1000000,
--
2.43.0
It's hardcoded to be zero. Always. Ever since msm-3.10. Or maybe
even before. Remove it!
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/media/platform/qcom/venus/core.c | 4 ----
drivers/media/platform/qcom/venus/core.h | 1 -
drivers/media/platform/qcom/venus/firmware.c | 3 +--
3 files changed, 1 insertion(+), 7 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 06b78e98cebd..65a9e815e6ba 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -647,7 +647,6 @@ static const struct venus_resources sdm660_res = {
.vcodec_clks_num = 1,
.max_load = 1036800,
.hfi_version = HFI_VERSION_3XX,
- .cp_start = 0,
.cp_size = 0x79000000,
.cp_nonpixel_start = 0x1000000,
.cp_nonpixel_size = 0x28000000,
@@ -716,7 +715,6 @@ static const struct venus_resources sdm845_res_v2 = {
.hfi_version = HFI_VERSION_4XX,
.vpu_version = VPU_VERSION_AR50,
.dma_mask = GENMASK(31, 29) - 1,
- .cp_start = 0,
.cp_size = 0x70800000,
.cp_nonpixel_start = 0x1000000,
.cp_nonpixel_size = 0x24800000,
@@ -760,7 +758,6 @@ static const struct venus_resources sc7180_res = {
.hfi_version = HFI_VERSION_4XX,
.vpu_version = VPU_VERSION_AR50,
.dma_mask = GENMASK(31, 29) - 1,
- .cp_start = 0,
.cp_size = 0x70800000,
.cp_nonpixel_start = 0x1000000,
.cp_nonpixel_size = 0x24800000,
@@ -869,7 +866,6 @@ static const struct venus_resources sc7280_res = {
.vpu_version = VPU_VERSION_IRIS2_1,
.num_vpp_pipes = 1,
.dma_mask = GENMASK(31, 29) - 1,
- .cp_start = 0,
.cp_size = 0x25800000,
.cp_nonpixel_start = 0x1000000,
.cp_nonpixel_size = 0x24800000,
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index b1d0687d294f..9dacf533c7ad 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -80,7 +80,6 @@ struct venus_resources {
const enum vpu_version vpu_version;
const u8 num_vpp_pipes;
const u32 max_load;
- const u32 cp_start;
const u32 cp_size;
const u32 cp_nonpixel_start;
const u32 cp_nonpixel_size;
diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c
index fe7da2b30482..16e578780be7 100644
--- a/drivers/media/platform/qcom/venus/firmware.c
+++ b/drivers/media/platform/qcom/venus/firmware.c
@@ -245,7 +245,6 @@ int venus_boot(struct venus_core *core)
if (core->use_tz && res->cp_size) {
/*
* Clues for porting using downstream data:
- * cp_start = 0
* cp_size = venus_ns/virtual-addr-pool[0] - yes, address and not size!
* This works, as the non-secure context bank is placed
* contiguously right after the Content Protection region.
@@ -253,7 +252,7 @@ int venus_boot(struct venus_core *core)
* cp_nonpixel_start = venus_sec_non_pixel/virtual-addr-pool[0]
* cp_nonpixel_size = venus_sec_non_pixel/virtual-addr-pool[1]
*/
- ret = qcom_scm_mem_protect_video_var(res->cp_start,
+ ret = qcom_scm_mem_protect_video_var(0,
res->cp_size,
res->cp_nonpixel_start,
res->cp_nonpixel_size);
--
2.43.0
Instead of redefining the same literals over and over again, define
them once and point the reference to that definition.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/media/platform/qcom/venus/core.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 0652065cb113..83ac68f1566f 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -538,6 +538,9 @@ static const struct dev_pm_ops venus_pm_ops = {
SET_RUNTIME_PM_OPS(venus_runtime_suspend, venus_runtime_resume, NULL)
};
+static const char * const pd_names_cx[] = { "cx", NULL };
+static const char * const pd_names_mx[] = { "mx", NULL };
+
static const struct freq_tbl msm8916_freq_table[] = {
{ 352800, 228570000 }, /* 1920x1088 @ 30 + 1280x720 @ 30 */
{ 244800, 160000000 }, /* 1920x1088 @ 30 */
@@ -721,7 +724,7 @@ static const struct venus_resources sdm845_res_v2 = {
.vcodec_clks_num = 2,
.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0", "vcodec1" },
.vcodec_pmdomains_num = 3,
- .opp_pmdomain = (const char *[]) { "cx", NULL },
+ .opp_pmdomain = pd_names_cx,
.vcodec_num = 2,
.max_load = 3110400, /* 4096x2160@90 */
.hfi_version = HFI_VERSION_4XX,
@@ -770,7 +773,7 @@ static const struct venus_resources sc7180_res = {
.vcodec_clks_num = 2,
.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
.vcodec_pmdomains_num = 2,
- .opp_pmdomain = (const char *[]) { "cx", NULL },
+ .opp_pmdomain = pd_names_cx,
.vcodec_num = 1,
.hfi_version = HFI_VERSION_4XX,
.vpu_version = VPU_VERSION_AR50,
@@ -827,7 +830,7 @@ static const struct venus_resources sm8250_res = {
.vcodec_clks_num = 1,
.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
.vcodec_pmdomains_num = 2,
- .opp_pmdomain = (const char *[]) { "mx", NULL },
+ .opp_pmdomain = pd_names_mx,
.vcodec_num = 1,
.max_load = 7833600,
.hfi_version = HFI_VERSION_6XX,
@@ -886,7 +889,7 @@ static const struct venus_resources sc7280_res = {
.vcodec_clks_num = 2,
.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
.vcodec_pmdomains_num = 2,
- .opp_pmdomain = (const char *[]) { "cx", NULL },
+ .opp_pmdomain = pd_names_cx,
.vcodec_num = 1,
.hfi_version = HFI_VERSION_6XX,
.vpu_version = VPU_VERSION_IRIS2_1,
--
2.43.0
core_power_v4 called with num_resets = 0 and core->pmdomains[0] == NULL
does exactly the same thing as core_power_v1. Unify them!
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/media/platform/qcom/venus/core.c | 21 +++++++--------------
drivers/media/platform/qcom/venus/pm_helpers.c | 17 +----------------
drivers/media/platform/qcom/venus/pm_helpers.h | 2 +-
3 files changed, 9 insertions(+), 31 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 65a9e815e6ba..9bfd2a30084b 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -477,18 +477,15 @@ static void venus_core_shutdown(struct platform_device *pdev)
static __maybe_unused int venus_runtime_suspend(struct device *dev)
{
struct venus_core *core = dev_get_drvdata(dev);
- const struct venus_pm_ops *pm_ops = core->pm_ops;
int ret;
ret = hfi_core_suspend(core);
if (ret)
return ret;
- if (pm_ops->core_power) {
- ret = pm_ops->core_power(core, POWER_OFF);
- if (ret)
- return ret;
- }
+ ret = venus_core_power(core, POWER_OFF);
+ if (ret)
+ return ret;
ret = icc_set_bw(core->cpucfg_path, 0, 0);
if (ret)
@@ -503,8 +500,7 @@ static __maybe_unused int venus_runtime_suspend(struct device *dev)
err_video_path:
icc_set_bw(core->cpucfg_path, kbps_to_icc(1000), 0);
err_cpucfg_path:
- if (pm_ops->core_power)
- pm_ops->core_power(core, POWER_ON);
+ venus_core_power(core, POWER_ON);
return ret;
}
@@ -512,7 +508,6 @@ static __maybe_unused int venus_runtime_suspend(struct device *dev)
static __maybe_unused int venus_runtime_resume(struct device *dev)
{
struct venus_core *core = dev_get_drvdata(dev);
- const struct venus_pm_ops *pm_ops = core->pm_ops;
int ret;
ret = icc_set_bw(core->video_path, kbps_to_icc(20000), 0);
@@ -523,11 +518,9 @@ static __maybe_unused int venus_runtime_resume(struct device *dev)
if (ret)
return ret;
- if (pm_ops->core_power) {
- ret = pm_ops->core_power(core, POWER_ON);
- if (ret)
- return ret;
- }
+ ret = venus_core_power(core, POWER_ON);
+ if (ret)
+ return ret;
return hfi_core_resume(core, false);
}
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index 8412deb68ed1..6f6de9ef1c6c 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -322,22 +322,9 @@ static void core_put_v1(struct venus_core *core)
{
}
-static int core_power_v1(struct venus_core *core, int on)
-{
- int ret = 0;
-
- if (on == POWER_ON)
- ret = core_clks_enable(core);
- else
- core_clks_disable(core);
-
- return ret;
-}
-
static const struct venus_pm_ops pm_ops_v1 = {
.core_get = venus_clks_get,
.core_put = core_put_v1,
- .core_power = core_power_v1,
.load_scale = load_scale_v1,
};
@@ -410,7 +397,6 @@ static int venc_power_v3(struct device *dev, int on)
static const struct venus_pm_ops pm_ops_v3 = {
.core_get = venus_clks_get,
.core_put = core_put_v1,
- .core_power = core_power_v1,
.vdec_get = vdec_get_v3,
.vdec_power = vdec_power_v3,
.venc_get = venc_get_v3,
@@ -990,7 +976,7 @@ static void core_put_v4(struct venus_core *core)
vcodec_domains_put(core);
}
-static int core_power_v4(struct venus_core *core, int on)
+int venus_core_power(struct venus_core *core, int on)
{
struct device *dev = core->dev;
struct device *pmctrl = core->pmdomains ?
@@ -1138,7 +1124,6 @@ static int load_scale_v4(struct venus_inst *inst)
static const struct venus_pm_ops pm_ops_v4 = {
.core_get = core_get_v4,
.core_put = core_put_v4,
- .core_power = core_power_v4,
.vdec_get = vdec_get_v4,
.vdec_put = vdec_put_v4,
.vdec_power = vdec_power_v4,
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.h b/drivers/media/platform/qcom/venus/pm_helpers.h
index a492c50c5543..77db940a265c 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.h
+++ b/drivers/media/platform/qcom/venus/pm_helpers.h
@@ -12,7 +12,6 @@ struct venus_core;
struct venus_pm_ops {
int (*core_get)(struct venus_core *core);
void (*core_put)(struct venus_core *core);
- int (*core_power)(struct venus_core *core, int on);
int (*vdec_get)(struct device *dev);
void (*vdec_put)(struct device *dev);
@@ -28,6 +27,7 @@ struct venus_pm_ops {
};
const struct venus_pm_ops *venus_pm_get(enum hfi_version version);
+int venus_core_power(struct venus_core *core, int on);
static inline int venus_pm_load_scale(struct venus_inst *inst)
{
--
2.43.0
Currently the infrastructure is set up for vast expandability, but
it's far too complex for what is just 0-2 clocks. Categorize the
clocks and simplify their getting.
One notable change is that vcodec clocks are switched to use
devm_clk_get_optional, which will let us commonize the code further
while leaving the burden of figuring out which SoCs need codec-specific
clocks and which don't to the bindings checker.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/media/platform/qcom/venus/core.c | 18 ----
drivers/media/platform/qcom/venus/core.h | 9 +-
drivers/media/platform/qcom/venus/pm_helpers.c | 129 +++++++++++++------------
3 files changed, 69 insertions(+), 87 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 70c3c9dc49c6..680674dd0d68 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -581,9 +581,6 @@ static const struct venus_resources msm8996_res = {
.reg_tbl_size = ARRAY_SIZE(msm8996_reg_preset),
.clks = {"core", "iface", "bus", "mbus" },
.clks_num = 4,
- .vcodec0_clks = { "core" },
- .vcodec1_clks = { "core" },
- .vcodec_clks_num = 1,
.max_load = 2563200,
.hfi_version = HFI_VERSION_3XX,
.dma_mask = (GENMASK(31, 30) | GENMASK(28, 26) | GENMASK(24, 22)) - 1,
@@ -636,9 +633,6 @@ static const struct venus_resources sdm660_res = {
.bw_tbl_dec_size = ARRAY_SIZE(sdm660_bw_table_dec),
.clks = {"core", "iface", "bus", "bus_throttle" },
.clks_num = 4,
- .vcodec0_clks = { "vcodec0_core" },
- .vcodec1_clks = { "vcodec0_core" },
- .vcodec_clks_num = 1,
.max_load = 1036800,
.hfi_version = HFI_VERSION_3XX,
.cp_size = 0x79000000,
@@ -680,9 +674,6 @@ static const struct venus_resources sdm845_res = {
.bw_tbl_dec_size = ARRAY_SIZE(sdm845_bw_table_dec),
.clks = {"core", "iface", "bus" },
.clks_num = 3,
- .vcodec0_clks = { "core", "bus" },
- .vcodec1_clks = { "core", "bus" },
- .vcodec_clks_num = 2,
.max_load = 3110400, /* 4096x2160@90 */
.hfi_version = HFI_VERSION_4XX,
.vpu_version = VPU_VERSION_AR50,
@@ -699,9 +690,6 @@ static const struct venus_resources sdm845_res_v2 = {
.bw_tbl_dec_size = ARRAY_SIZE(sdm845_bw_table_dec),
.clks = {"core", "iface", "bus" },
.clks_num = 3,
- .vcodec0_clks = { "vcodec0_core", "vcodec0_bus" },
- .vcodec1_clks = { "vcodec1_core", "vcodec1_bus" },
- .vcodec_clks_num = 2,
.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0", "vcodec1" },
.vcodec_pmdomains_num = 3,
.opp_pmdomain = pd_names_cx,
@@ -744,8 +732,6 @@ static const struct venus_resources sc7180_res = {
.bw_tbl_dec_size = ARRAY_SIZE(sc7180_bw_table_dec),
.clks = {"core", "iface", "bus" },
.clks_num = 3,
- .vcodec0_clks = { "vcodec0_core", "vcodec0_bus" },
- .vcodec_clks_num = 2,
.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
.vcodec_pmdomains_num = 2,
.opp_pmdomain = pd_names_cx,
@@ -796,8 +782,6 @@ static const struct venus_resources sm8250_res = {
.clks_num = 2,
.resets = { "bus", "core" },
.resets_num = 2,
- .vcodec0_clks = { "vcodec0_core" },
- .vcodec_clks_num = 1,
.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
.vcodec_pmdomains_num = 2,
.opp_pmdomain = pd_names_mx,
@@ -851,8 +835,6 @@ static const struct venus_resources sc7280_res = {
.ubwc_conf = &sc7280_ubwc_config,
.clks = {"core", "bus", "iface"},
.clks_num = 3,
- .vcodec0_clks = {"vcodec_core", "vcodec_bus"},
- .vcodec_clks_num = 2,
.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0" },
.vcodec_pmdomains_num = 2,
.opp_pmdomain = pd_names_cx,
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 9dacf533c7ad..6ecaa3e38cac 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -24,9 +24,10 @@
#define VDBGFW "VenusFW : "
#define VIDC_CLKS_NUM_MAX 4
-#define VIDC_VCODEC_CLKS_NUM_MAX 2
#define VIDC_RESETS_NUM_MAX 2
+#define MAX_NUM_VCODECS 2
+
extern int venus_fw_debug;
struct freq_tbl {
@@ -68,8 +69,6 @@ struct venus_resources {
const struct hfi_ubwc_config * const ubwc_conf;
const char * const clks[VIDC_CLKS_NUM_MAX];
const unsigned int clks_num;
- const char * const vcodec0_clks[VIDC_VCODEC_CLKS_NUM_MAX];
- const char * const vcodec1_clks[VIDC_VCODEC_CLKS_NUM_MAX];
const unsigned int vcodec_clks_num;
const char * const *vcodec_pmdomains;
const unsigned int vcodec_pmdomains_num;
@@ -176,8 +175,8 @@ struct venus_core {
void __iomem *aon_base;
int irq;
struct clk *clks[VIDC_CLKS_NUM_MAX];
- struct clk *vcodec0_clks[VIDC_VCODEC_CLKS_NUM_MAX];
- struct clk *vcodec1_clks[VIDC_VCODEC_CLKS_NUM_MAX];
+ struct clk *vcodec_core_clks[MAX_NUM_VCODECS];
+ struct clk *vcodec_bus_clks[MAX_NUM_VCODECS];
struct icc_path *video_path;
struct icc_path *cpucfg_path;
bool has_opp_table;
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index 32f9ccfa9d8a..a292c788ffba 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -110,67 +110,74 @@ static void core_clks_disable(struct venus_core *core)
static int core_clks_set_rate(struct venus_core *core, unsigned long freq)
{
- int ret;
+ int i, ret;
ret = dev_pm_opp_set_rate(core->dev, freq);
if (ret)
return ret;
- ret = clk_set_rate(core->vcodec0_clks[0], freq);
- if (ret)
- return ret;
-
- ret = clk_set_rate(core->vcodec1_clks[0], freq);
- if (ret)
- return ret;
+ for (i = 0; i < MAX_NUM_VCODECS; i++) {
+ ret = clk_set_rate(core->vcodec_core_clks[i], freq);
+ if (ret)
+ return ret;
+ }
return 0;
}
-static int vcodec_clks_get(struct venus_core *core, struct device *dev,
- struct clk **clks, const char * const *id)
+static int vcodec_clks_get(struct venus_core *core, struct device *dev, u8 id)
{
- const struct venus_resources *res = core->res;
- unsigned int i;
+ char buf[13] = { 0 }; /* vcodecX_core\0 */
- for (i = 0; i < res->vcodec_clks_num; i++) {
- if (!id[i])
- continue;
- clks[i] = devm_clk_get(dev, id[i]);
- if (IS_ERR(clks[i]))
- return PTR_ERR(clks[i]);
+ /* Best we can do is 2 cores */
+ if (id > MAX_NUM_VCODECS - 1) {
+ dev_err(dev, "Got impossible vcodec id %u\n", id);
+ return -EINVAL;
+ };
+
+ snprintf(buf, sizeof(buf), "vcodec%u_core", id);
+
+ /* First try the non-legacy name */
+ core->vcodec_core_clks[id] = devm_clk_get_optional(dev, buf);
+ if (IS_ERR(core->vcodec_core_clks[id])) {
+ /* Try again, with the legacy name */
+ core->vcodec_core_clks[id] = devm_clk_get_optional(dev, "core");
+ if (IS_ERR(core->vcodec_core_clks[id]))
+ return PTR_ERR(core->vcodec_core_clks[id]);
+ }
+
+ memset(buf, 0, sizeof(buf));
+ snprintf(buf, sizeof(buf), "vcodec%u_bus", id);
+
+ core->vcodec_bus_clks[id] = devm_clk_get_optional(dev, buf);
+ if (IS_ERR(core->vcodec_bus_clks[id])) {
+ core->vcodec_bus_clks[id] = devm_clk_get_optional(dev, "bus");
+ if (IS_ERR(core->vcodec_bus_clks[id]))
+ return PTR_ERR(core->vcodec_bus_clks[id]);
}
return 0;
}
-static int vcodec_clks_enable(struct venus_core *core, struct clk **clks)
+static int vcodec_clks_enable(struct venus_core *core, u8 id)
{
- const struct venus_resources *res = core->res;
- unsigned int i;
int ret;
- for (i = 0; i < res->vcodec_clks_num; i++) {
- ret = clk_prepare_enable(clks[i]);
- if (ret)
- goto err;
- }
+ ret = clk_prepare_enable(core->vcodec_core_clks[id]);
+ if (ret)
+ return ret;
- return 0;
-err:
- while (i--)
- clk_disable_unprepare(clks[i]);
+ ret = clk_prepare_enable(core->vcodec_bus_clks[id]);
+ if (ret)
+ clk_disable_unprepare(core->vcodec_core_clks[id]);
return ret;
}
-static void vcodec_clks_disable(struct venus_core *core, struct clk **clks)
+static void vcodec_clks_disable(struct venus_core *core, u8 id)
{
- const struct venus_resources *res = core->res;
- unsigned int i = res->vcodec_clks_num;
-
- while (i--)
- clk_disable_unprepare(clks[i]);
+ clk_disable_unprepare(core->vcodec_bus_clks[id]);
+ clk_disable_unprepare(core->vcodec_core_clks[id]);
}
static u32 load_per_instance(struct venus_inst *inst)
@@ -343,8 +350,7 @@ static int vdec_get_v3(struct device *dev)
{
struct venus_core *core = dev_get_drvdata(dev);
- return vcodec_clks_get(core, dev, core->vcodec0_clks,
- core->res->vcodec0_clks);
+ return vcodec_clks_get(core, dev, 0);
}
static int vdec_power_v3(struct device *dev, int on)
@@ -355,9 +361,9 @@ static int vdec_power_v3(struct device *dev, int on)
vcodec_control_v3(core, VIDC_SESSION_TYPE_DEC, true);
if (on == POWER_ON)
- ret = vcodec_clks_enable(core, core->vcodec0_clks);
+ ret = vcodec_clks_enable(core, 0);
else
- vcodec_clks_disable(core, core->vcodec0_clks);
+ vcodec_clks_disable(core, 0);
vcodec_control_v3(core, VIDC_SESSION_TYPE_DEC, false);
@@ -368,8 +374,7 @@ static int venc_get_v3(struct device *dev)
{
struct venus_core *core = dev_get_drvdata(dev);
- return vcodec_clks_get(core, dev, core->vcodec1_clks,
- core->res->vcodec1_clks);
+ return vcodec_clks_get(core, dev, 1);
}
static int venc_power_v3(struct device *dev, int on)
@@ -380,9 +385,9 @@ static int venc_power_v3(struct device *dev, int on)
vcodec_control_v3(core, VIDC_SESSION_TYPE_ENC, true);
if (on == POWER_ON)
- ret = vcodec_clks_enable(core, core->vcodec1_clks);
+ ret = vcodec_clks_enable(core, 1);
else
- vcodec_clks_disable(core, core->vcodec1_clks);
+ vcodec_clks_disable(core, 1);
vcodec_control_v3(core, VIDC_SESSION_TYPE_ENC, false);
@@ -441,7 +446,7 @@ static int poweroff_coreid(struct venus_core *core, unsigned int coreid_mask)
if (ret)
return ret;
- vcodec_clks_disable(core, core->vcodec0_clks);
+ vcodec_clks_disable(core, 0);
ret = vcodec_control_v4(core, VIDC_CORE_ID_1, false);
if (ret)
@@ -457,7 +462,7 @@ static int poweroff_coreid(struct venus_core *core, unsigned int coreid_mask)
if (ret)
return ret;
- vcodec_clks_disable(core, core->vcodec1_clks);
+ vcodec_clks_disable(core, 1);
ret = vcodec_control_v4(core, VIDC_CORE_ID_2, false);
if (ret)
@@ -484,7 +489,7 @@ static int poweron_coreid(struct venus_core *core, unsigned int coreid_mask)
if (ret)
return ret;
- ret = vcodec_clks_enable(core, core->vcodec0_clks);
+ ret = vcodec_clks_enable(core, 0);
if (ret)
return ret;
@@ -502,7 +507,7 @@ static int poweron_coreid(struct venus_core *core, unsigned int coreid_mask)
if (ret)
return ret;
- ret = vcodec_clks_enable(core, core->vcodec1_clks);
+ ret = vcodec_clks_enable(core, 1);
if (ret)
return ret;
@@ -763,20 +768,18 @@ static int vdec_get_v4(struct device *dev)
if (!legacy_binding)
return 0;
- return vcodec_clks_get(core, dev, core->vcodec0_clks,
- core->res->vcodec0_clks);
+ return vcodec_clks_get(core, dev, 0);
}
static void vdec_put_v4(struct device *dev)
{
struct venus_core *core = dev_get_drvdata(dev);
- unsigned int i;
if (!legacy_binding)
return;
- for (i = 0; i < core->res->vcodec_clks_num; i++)
- core->vcodec0_clks[i] = NULL;
+ core->vcodec_core_clks[0] = NULL;
+ core->vcodec_bus_clks[0] = NULL;
}
static int vdec_power_v4(struct device *dev, int on)
@@ -792,9 +795,9 @@ static int vdec_power_v4(struct device *dev, int on)
return ret;
if (on == POWER_ON)
- ret = vcodec_clks_enable(core, core->vcodec0_clks);
+ ret = vcodec_clks_enable(core, 0);
else
- vcodec_clks_disable(core, core->vcodec0_clks);
+ vcodec_clks_disable(core, 0);
vcodec_control_v4(core, VIDC_CORE_ID_1, false);
@@ -808,20 +811,18 @@ static int venc_get_v4(struct device *dev)
if (!legacy_binding)
return 0;
- return vcodec_clks_get(core, dev, core->vcodec1_clks,
- core->res->vcodec1_clks);
+ return vcodec_clks_get(core, dev, 1);
}
static void venc_put_v4(struct device *dev)
{
struct venus_core *core = dev_get_drvdata(dev);
- unsigned int i;
if (!legacy_binding)
return;
- for (i = 0; i < core->res->vcodec_clks_num; i++)
- core->vcodec1_clks[i] = NULL;
+ core->vcodec_core_clks[1] = NULL;
+ core->vcodec_bus_clks[1] = NULL;
}
static int venc_power_v4(struct device *dev, int on)
@@ -837,9 +838,9 @@ static int venc_power_v4(struct device *dev, int on)
return ret;
if (on == POWER_ON)
- ret = vcodec_clks_enable(core, core->vcodec1_clks);
+ ret = vcodec_clks_enable(core, 1);
else
- vcodec_clks_disable(core, core->vcodec1_clks);
+ vcodec_clks_disable(core, 1);
vcodec_control_v4(core, VIDC_CORE_ID_2, false);
@@ -934,11 +935,11 @@ static int core_get_v4(struct venus_core *core)
dev_info(dev, "%s legacy binding\n", legacy_binding ? "" : "non");
- ret = vcodec_clks_get(core, dev, core->vcodec0_clks, res->vcodec0_clks);
+ ret = vcodec_clks_get(core, dev, 0);
if (ret)
return ret;
- ret = vcodec_clks_get(core, dev, core->vcodec1_clks, res->vcodec1_clks);
+ ret = vcodec_clks_get(core, dev, 1);
if (ret)
return ret;
--
2.43.0
As has been the story with the past few commits, much of the resource
acquisition logic is totally identical between different generations
and there's no good reason to invent a new function for each one.
Commonize core_get() and rename it to venus_get_resources() to be more
meaningful.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/media/platform/qcom/venus/core.c | 8 +++-----
drivers/media/platform/qcom/venus/pm_helpers.c | 5 +----
drivers/media/platform/qcom/venus/pm_helpers.h | 3 +--
3 files changed, 5 insertions(+), 11 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 680674dd0d68..873affe17537 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -334,11 +334,9 @@ static int venus_probe(struct platform_device *pdev)
return PTR_ERR(core->resets[i]);
}
- if (core->pm_ops->core_get) {
- ret = core->pm_ops->core_get(core);
- if (ret)
- return ret;
- }
+ ret = venus_get_resources(core);
+ if (ret)
+ return ret;
ret = dma_set_mask_and_coherent(dev, res->dma_mask);
if (ret)
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index a292c788ffba..1cbcffbc29af 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -326,7 +326,6 @@ static int load_scale_v1(struct venus_inst *inst)
}
static const struct venus_pm_ops pm_ops_v1 = {
- .core_get = venus_clks_get,
.load_scale = load_scale_v1,
};
@@ -395,7 +394,6 @@ static int venc_power_v3(struct device *dev, int on)
}
static const struct venus_pm_ops pm_ops_v3 = {
- .core_get = venus_clks_get,
.vdec_get = vdec_get_v3,
.vdec_power = vdec_power_v3,
.venc_get = venc_get_v3,
@@ -920,7 +918,7 @@ static int core_resets_reset(struct venus_core *core)
return ret;
}
-static int core_get_v4(struct venus_core *core)
+int venus_get_resources(struct venus_core *core)
{
struct device *dev = core->dev;
const struct venus_resources *res = core->res;
@@ -1109,7 +1107,6 @@ static int load_scale_v4(struct venus_inst *inst)
}
static const struct venus_pm_ops pm_ops_v4 = {
- .core_get = core_get_v4,
.vdec_get = vdec_get_v4,
.vdec_put = vdec_put_v4,
.vdec_power = vdec_power_v4,
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.h b/drivers/media/platform/qcom/venus/pm_helpers.h
index 3014b39aa6e3..7a55a55029f3 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.h
+++ b/drivers/media/platform/qcom/venus/pm_helpers.h
@@ -10,8 +10,6 @@ struct venus_core;
#define POWER_OFF 0
struct venus_pm_ops {
- int (*core_get)(struct venus_core *core);
-
int (*vdec_get)(struct device *dev);
void (*vdec_put)(struct device *dev);
int (*vdec_power)(struct device *dev, int on);
@@ -28,6 +26,7 @@ struct venus_pm_ops {
const struct venus_pm_ops *venus_pm_get(enum hfi_version version);
int venus_core_power(struct venus_core *core, int on);
void vcodec_domains_put(struct venus_core *core);
+int venus_get_resources(struct venus_core *core);
static inline int venus_pm_load_scale(struct venus_inst *inst)
{
--
2.43.0
Without an OPP table and with vcodec_pmdomains_num (so, v1, v3 and
sdm845_legacy targets), core_put_v4 is a NOP, jut like core_put_v1.
Unify them!
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/media/platform/qcom/venus/core.c | 8 +++-----
drivers/media/platform/qcom/venus/pm_helpers.c | 17 +----------------
drivers/media/platform/qcom/venus/pm_helpers.h | 2 +-
3 files changed, 5 insertions(+), 22 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 9bfd2a30084b..666adc5aac38 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -426,15 +426,14 @@ static int venus_probe(struct platform_device *pdev)
err_core_deinit:
hfi_core_deinit(core, false);
err_core_put:
- if (core->pm_ops->core_put)
- core->pm_ops->core_put(core);
+ vcodec_domains_put(core);
+
return ret;
}
static void venus_remove(struct platform_device *pdev)
{
struct venus_core *core = platform_get_drvdata(pdev);
- const struct venus_pm_ops *pm_ops = core->pm_ops;
struct device *dev = core->dev;
int ret;
@@ -452,8 +451,7 @@ static void venus_remove(struct platform_device *pdev)
pm_runtime_put_sync(dev);
pm_runtime_disable(dev);
- if (pm_ops->core_put)
- pm_ops->core_put(core);
+ vcodec_domains_put(core);
v4l2_device_unregister(&core->v4l2_dev);
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index 6f6de9ef1c6c..32f9ccfa9d8a 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -318,13 +318,8 @@ static int load_scale_v1(struct venus_inst *inst)
return ret;
}
-static void core_put_v1(struct venus_core *core)
-{
-}
-
static const struct venus_pm_ops pm_ops_v1 = {
.core_get = venus_clks_get,
- .core_put = core_put_v1,
.load_scale = load_scale_v1,
};
@@ -396,7 +391,6 @@ static int venc_power_v3(struct device *dev, int on)
static const struct venus_pm_ops pm_ops_v3 = {
.core_get = venus_clks_get,
- .core_put = core_put_v1,
.vdec_get = vdec_get_v3,
.vdec_power = vdec_power_v3,
.venc_get = venc_get_v3,
@@ -893,7 +887,7 @@ static int vcodec_domains_get(struct venus_core *core)
return ret;
}
-static void vcodec_domains_put(struct venus_core *core)
+void vcodec_domains_put(struct venus_core *core)
{
dev_pm_domain_detach_list(core->pmdomains);
@@ -968,14 +962,6 @@ static int core_get_v4(struct venus_core *core)
return 0;
}
-static void core_put_v4(struct venus_core *core)
-{
- if (legacy_binding)
- return;
-
- vcodec_domains_put(core);
-}
-
int venus_core_power(struct venus_core *core, int on)
{
struct device *dev = core->dev;
@@ -1123,7 +1109,6 @@ static int load_scale_v4(struct venus_inst *inst)
static const struct venus_pm_ops pm_ops_v4 = {
.core_get = core_get_v4,
- .core_put = core_put_v4,
.vdec_get = vdec_get_v4,
.vdec_put = vdec_put_v4,
.vdec_power = vdec_power_v4,
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.h b/drivers/media/platform/qcom/venus/pm_helpers.h
index 77db940a265c..3014b39aa6e3 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.h
+++ b/drivers/media/platform/qcom/venus/pm_helpers.h
@@ -11,7 +11,6 @@ struct venus_core;
struct venus_pm_ops {
int (*core_get)(struct venus_core *core);
- void (*core_put)(struct venus_core *core);
int (*vdec_get)(struct device *dev);
void (*vdec_put)(struct device *dev);
@@ -28,6 +27,7 @@ struct venus_pm_ops {
const struct venus_pm_ops *venus_pm_get(enum hfi_version version);
int venus_core_power(struct venus_core *core, int on);
+void vcodec_domains_put(struct venus_core *core);
static inline int venus_pm_load_scale(struct venus_inst *inst)
{
--
2.43.0
To make the code more concise, define a new variable 'res' pointing to
the abundantly referenced core->res.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/media/platform/qcom/venus/core.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 666adc5aac38..70c3c9dc49c6 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -285,6 +285,7 @@ static irqreturn_t venus_isr_thread(int irq, void *dev_id)
static int venus_probe(struct platform_device *pdev)
{
+ const struct venus_resources *res;
struct device *dev = &pdev->dev;
struct venus_core *core;
int i, ret;
@@ -315,9 +316,11 @@ static int venus_probe(struct platform_device *pdev)
if (!core->res)
return -ENODEV;
+ res = core->res;
+
mutex_init(&core->pm_lock);
- core->pm_ops = venus_pm_get(core->res->hfi_version);
+ core->pm_ops = venus_pm_get(res->hfi_version);
if (!core->pm_ops)
return -ENODEV;
@@ -325,8 +328,8 @@ static int venus_probe(struct platform_device *pdev)
if (ret)
return ret;
- for (i = 0; i < core->res->resets_num; i++) {
- core->resets[i] = devm_reset_control_get_exclusive(dev, core->res->resets[i]);
+ for (i = 0; i < res->resets_num; i++) {
+ core->resets[i] = devm_reset_control_get_exclusive(dev, res->resets[i]);
if (IS_ERR(core->resets[i]))
return PTR_ERR(core->resets[i]);
}
@@ -337,7 +340,7 @@ static int venus_probe(struct platform_device *pdev)
return ret;
}
- ret = dma_set_mask_and_coherent(dev, core->res->dma_mask);
+ ret = dma_set_mask_and_coherent(dev, res->dma_mask);
if (ret)
goto err_core_put;
--
2.43.0
This function can be very easily commonized between the supported gens.
Do so!
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/media/platform/qcom/venus/pm_helpers.c | 22 ++--------------------
drivers/media/platform/qcom/venus/pm_helpers.h | 2 +-
drivers/media/platform/qcom/venus/vdec.c | 9 +++++++--
3 files changed, 10 insertions(+), 23 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index 1cbcffbc29af..cf0794acf5d0 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -125,7 +125,7 @@ static int core_clks_set_rate(struct venus_core *core, unsigned long freq)
return 0;
}
-static int vcodec_clks_get(struct venus_core *core, struct device *dev, u8 id)
+int vcodec_clks_get(struct venus_core *core, struct device *dev, u8 id)
{
char buf[13] = { 0 }; /* vcodecX_core\0 */
@@ -158,6 +158,7 @@ static int vcodec_clks_get(struct venus_core *core, struct device *dev, u8 id)
return 0;
}
+EXPORT_SYMBOL_GPL(vcodec_clks_get);
static int vcodec_clks_enable(struct venus_core *core, u8 id)
{
@@ -345,13 +346,6 @@ vcodec_control_v3(struct venus_core *core, u32 session_type, bool enable)
writel(1, ctrl);
}
-static int vdec_get_v3(struct device *dev)
-{
- struct venus_core *core = dev_get_drvdata(dev);
-
- return vcodec_clks_get(core, dev, 0);
-}
-
static int vdec_power_v3(struct device *dev, int on)
{
struct venus_core *core = dev_get_drvdata(dev);
@@ -394,7 +388,6 @@ static int venc_power_v3(struct device *dev, int on)
}
static const struct venus_pm_ops pm_ops_v3 = {
- .vdec_get = vdec_get_v3,
.vdec_power = vdec_power_v3,
.venc_get = venc_get_v3,
.venc_power = venc_power_v3,
@@ -759,16 +752,6 @@ static int coreid_power_v4(struct venus_inst *inst, int on)
return ret;
}
-static int vdec_get_v4(struct device *dev)
-{
- struct venus_core *core = dev_get_drvdata(dev);
-
- if (!legacy_binding)
- return 0;
-
- return vcodec_clks_get(core, dev, 0);
-}
-
static void vdec_put_v4(struct device *dev)
{
struct venus_core *core = dev_get_drvdata(dev);
@@ -1107,7 +1090,6 @@ static int load_scale_v4(struct venus_inst *inst)
}
static const struct venus_pm_ops pm_ops_v4 = {
- .vdec_get = vdec_get_v4,
.vdec_put = vdec_put_v4,
.vdec_power = vdec_power_v4,
.venc_get = venc_get_v4,
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.h b/drivers/media/platform/qcom/venus/pm_helpers.h
index 7a55a55029f3..4afc57dac865 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.h
+++ b/drivers/media/platform/qcom/venus/pm_helpers.h
@@ -10,7 +10,6 @@ struct venus_core;
#define POWER_OFF 0
struct venus_pm_ops {
- int (*vdec_get)(struct device *dev);
void (*vdec_put)(struct device *dev);
int (*vdec_power)(struct device *dev, int on);
@@ -27,6 +26,7 @@ const struct venus_pm_ops *venus_pm_get(enum hfi_version version);
int venus_core_power(struct venus_core *core, int on);
void vcodec_domains_put(struct venus_core *core);
int venus_get_resources(struct venus_core *core);
+int vcodec_clks_get(struct venus_core *core, struct device *dev, u8 id);
static inline int venus_pm_load_scale(struct venus_inst *inst)
{
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 29130a9441e7..d127311100cd 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -1788,8 +1788,13 @@ static int vdec_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, core);
- if (core->pm_ops->vdec_get) {
- ret = core->pm_ops->vdec_get(dev);
+ /*
+ * If the vcodec core clock is missing by now, it either doesn't exist
+ * (8916) or deprecated bindings with pre-assigned core functions and
+ * resources under the decoder node are in use.
+ */
+ if (!core->vcodec_core_clks[0]) {
+ ret = vcodec_clks_get(core, dev, 0);
if (ret)
return ret;
}
--
2.43.0
"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
This function can be very easily commonized between the supported gens.
Do so!
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/media/platform/qcom/venus/pm_helpers.c | 19 -------------------
drivers/media/platform/qcom/venus/pm_helpers.h | 1 -
drivers/media/platform/qcom/venus/venc.c | 9 +++++++--
3 files changed, 7 insertions(+), 22 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index cf0794acf5d0..9df8f2292c17 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -363,13 +363,6 @@ static int vdec_power_v3(struct device *dev, int on)
return ret;
}
-static int venc_get_v3(struct device *dev)
-{
- struct venus_core *core = dev_get_drvdata(dev);
-
- return vcodec_clks_get(core, dev, 1);
-}
-
static int venc_power_v3(struct device *dev, int on)
{
struct venus_core *core = dev_get_drvdata(dev);
@@ -389,7 +382,6 @@ static int venc_power_v3(struct device *dev, int on)
static const struct venus_pm_ops pm_ops_v3 = {
.vdec_power = vdec_power_v3,
- .venc_get = venc_get_v3,
.venc_power = venc_power_v3,
.load_scale = load_scale_v1,
};
@@ -785,16 +777,6 @@ static int vdec_power_v4(struct device *dev, int on)
return ret;
}
-static int venc_get_v4(struct device *dev)
-{
- struct venus_core *core = dev_get_drvdata(dev);
-
- if (!legacy_binding)
- return 0;
-
- return vcodec_clks_get(core, dev, 1);
-}
-
static void venc_put_v4(struct device *dev)
{
struct venus_core *core = dev_get_drvdata(dev);
@@ -1092,7 +1074,6 @@ static int load_scale_v4(struct venus_inst *inst)
static const struct venus_pm_ops pm_ops_v4 = {
.vdec_put = vdec_put_v4,
.vdec_power = vdec_power_v4,
- .venc_get = venc_get_v4,
.venc_put = venc_put_v4,
.venc_power = venc_power_v4,
.coreid_power = coreid_power_v4,
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.h b/drivers/media/platform/qcom/venus/pm_helpers.h
index 4afc57dac865..cbf54e6c6eab 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.h
+++ b/drivers/media/platform/qcom/venus/pm_helpers.h
@@ -13,7 +13,6 @@ struct venus_pm_ops {
void (*vdec_put)(struct device *dev);
int (*vdec_power)(struct device *dev, int on);
- int (*venc_get)(struct device *dev);
void (*venc_put)(struct device *dev);
int (*venc_power)(struct device *dev, int on);
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index 3ec2fb8d9fab..d17aeba74b49 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -1557,8 +1557,13 @@ static int venc_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, core);
- if (core->pm_ops->venc_get) {
- ret = core->pm_ops->venc_get(dev);
+ /*
+ * If the vcodec core clock is missing by now, it either doesn't exist
+ * (8916) or deprecated bindings with pre-assigned core functions and
+ * resources under the decoder node are in use.
+ */
+ if (!core->vcodec_core_clks[1]) {
+ ret = vcodec_clks_get(core, dev, 1);
if (ret)
return ret;
}
--
2.43.0
All of the resets are toggled together. Use the bulk api to save on some
code complexity.
The delay between resets is now correctly determined by the reset
framework.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/media/platform/qcom/venus/core.c | 15 ++++++++++-----
drivers/media/platform/qcom/venus/core.h | 4 ++--
drivers/media/platform/qcom/venus/pm_helpers.c | 15 +++------------
3 files changed, 15 insertions(+), 19 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 873affe17537..ff5601a5ce77 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -328,11 +328,16 @@ static int venus_probe(struct platform_device *pdev)
if (ret)
return ret;
- for (i = 0; i < res->resets_num; i++) {
- core->resets[i] = devm_reset_control_get_exclusive(dev, res->resets[i]);
- if (IS_ERR(core->resets[i]))
- return PTR_ERR(core->resets[i]);
- }
+ core->resets = devm_kcalloc(dev, res->resets_num, sizeof(*core->resets), GFP_KERNEL);
+ if (res->resets_num && !core->resets)
+ return -ENOMEM;
+
+ for (i = 0; i < res->resets_num; i++)
+ core->resets[i].id = res->resets[i];
+
+ ret = devm_reset_control_bulk_get_exclusive(dev, res->resets_num, core->resets);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to get resets\n");
ret = venus_get_resources(core);
if (ret)
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 6ecaa3e38cac..2376b9cbdf2c 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -130,7 +130,7 @@ struct venus_format {
* @pmdomains: a pointer to a list of pmdomains
* @opp_dl_venus: an device-link for device OPP
* @opp_pmdomain: an OPP power-domain
- * @resets: an array of reset signals
+ * @resets: a reset_control_bulk_data array of hardware reset signals
* @vdev_dec: a reference to video device structure for decoder instances
* @vdev_enc: a reference to video device structure for encoder instances
* @v4l2_dev: a holder for v4l2 device structure
@@ -183,7 +183,7 @@ struct venus_core {
struct dev_pm_domain_list *pmdomains;
struct device_link *opp_dl_venus;
struct device *opp_pmdomain;
- struct reset_control *resets[VIDC_RESETS_NUM_MAX];
+ struct reset_control_bulk_data *resets;
struct video_device *vdev_dec;
struct video_device *vdev_enc;
struct v4l2_device v4l2_dev;
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index 9df8f2292c17..170fb131cb1e 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -865,21 +865,12 @@ void vcodec_domains_put(struct venus_core *core)
static int core_resets_reset(struct venus_core *core)
{
const struct venus_resources *res = core->res;
- unsigned int i;
int ret;
- for (i = 0; i < res->resets_num; i++) {
- ret = reset_control_assert(core->resets[i]);
- if (ret)
- goto err;
-
- usleep_range(150, 250);
- ret = reset_control_deassert(core->resets[i]);
- if (ret)
- goto err;
- }
+ ret = reset_control_bulk_reset(res->resets_num, core->resets);
+ if (ret)
+ dev_err(core->dev, "Failed to toggle resets: %d\n", ret);
-err:
return ret;
}
--
2.43.0
On 09/02/2024 21:09, Konrad Dybcio wrote:
> Definitely needs testing on:
>
> - SDM845 with old bindings
> - SDM845 with new bindings or 7180
I can do 8916 for you but, I think we have these boards available
remotely on qc.lab.
Could you test those boards out remotely yourself ?
---
bod
Hi Konrad,
On Fr, 2024-02-09 at 22:10 +0100, Konrad Dybcio wrote:
> All of the resets are toggled together. Use the bulk api to save on some
> code complexity.
>
> The delay between resets is now correctly determined by the reset
> framework.
If this is a recent change, could you reference the commit?
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> drivers/media/platform/qcom/venus/core.c | 15 ++++++++++-----
> drivers/media/platform/qcom/venus/core.h | 4 ++--
> drivers/media/platform/qcom/venus/pm_helpers.c | 15 +++------------
> 3 files changed, 15 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 873affe17537..ff5601a5ce77 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -328,11 +328,16 @@ static int venus_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> - for (i = 0; i < res->resets_num; i++) {
> - core->resets[i] = devm_reset_control_get_exclusive(dev, res->resets[i]);
> - if (IS_ERR(core->resets[i]))
> - return PTR_ERR(core->resets[i]);
> - }
> + core->resets = devm_kcalloc(dev, res->resets_num, sizeof(*core->resets), GFP_KERNEL);
Since VIDC_RESETS_NUM_MAX is only 2, I don't think a separate
allocation is worth it.
> + if (res->resets_num && !core->resets)
> + return -ENOMEM;
> +
> + for (i = 0; i < res->resets_num; i++)
> + core->resets[i].id = res->resets[i];
> +
> + ret = devm_reset_control_bulk_get_exclusive(dev, res->resets_num, core->resets);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to get resets\n");
>
> ret = venus_get_resources(core);
> if (ret)
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index 6ecaa3e38cac..2376b9cbdf2c 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -130,7 +130,7 @@ struct venus_format {
> * @pmdomains: a pointer to a list of pmdomains
> * @opp_dl_venus: an device-link for device OPP
> * @opp_pmdomain: an OPP power-domain
> - * @resets: an array of reset signals
> + * @resets: a reset_control_bulk_data array of hardware reset signals
> * @vdev_dec: a reference to video device structure for decoder instances
> * @vdev_enc: a reference to video device structure for encoder instances
> * @v4l2_dev: a holder for v4l2 device structure
> @@ -183,7 +183,7 @@ struct venus_core {
> struct dev_pm_domain_list *pmdomains;
> struct device_link *opp_dl_venus;
> struct device *opp_pmdomain;
> - struct reset_control *resets[VIDC_RESETS_NUM_MAX];
> + struct reset_control_bulk_data *resets;
Any reason not to just keep this as an array[VIDC_RESETS_NUM_MAX]?
> struct video_device *vdev_dec;
> struct video_device *vdev_enc;
> struct v4l2_device v4l2_dev;
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
> index 9df8f2292c17..170fb131cb1e 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
> @@ -865,21 +865,12 @@ void vcodec_domains_put(struct venus_core *core)
> static int core_resets_reset(struct venus_core *core)
> {
> const struct venus_resources *res = core->res;
> - unsigned int i;
> int ret;
>
> - for (i = 0; i < res->resets_num; i++) {
> - ret = reset_control_assert(core->resets[i]);
> - if (ret)
> - goto err;
> -
> - usleep_range(150, 250);
> - ret = reset_control_deassert(core->resets[i]);
> - if (ret)
> - goto err;
> - }
> + ret = reset_control_bulk_reset(res->resets_num, core->resets);
> + if (ret)
> + dev_err(core->dev, "Failed to toggle resets: %d\n", ret);
>
> -err:
> return ret;
Could be simplified to:
return reset_control_bulk_reset(res->resets_num, core-
>resets);
regards
Philipp
On 14.02.2024 14:31, Philipp Zabel wrote:
> Hi Konrad,
>
> On Fr, 2024-02-09 at 22:10 +0100, Konrad Dybcio wrote:
>> All of the resets are toggled together. Use the bulk api to save on some
>> code complexity.
>>
>> The delay between resets is now correctly determined by the reset
>> framework.
>
> If this is a recent change, could you reference the commit?
It's a series that recently landed in -next [1]
[...]
>
> Since VIDC_RESETS_NUM_MAX is only 2, I don't think a separate
> allocation is worth it.
It's 2 today, anyway. I wanted to keep it flexible
[...]
>> + ret = reset_control_bulk_reset(res->resets_num, core->resets);
>> + if (ret)
>> + dev_err(core->dev, "Failed to toggle resets: %d\n", ret);
>>
>> -err:
>> return ret;
>
> Could be simplified to:
>
> return reset_control_bulk_reset(res->resets_num, core-
>> resets);
I intentionally kept the if (ret) to print a specific error message
in case the call fails, this driver doesn't go a good job of telling
the user/developer what went wrong.
Konrad
On 14.02.2024 13:56, Bryan O'Donoghue wrote:
> On 09/02/2024 21:09, Konrad Dybcio wrote:
>> Definitely needs testing on:
>>
>> - SDM845 with old bindings
>> - SDM845 with new bindings or 7180
>
> I can do 8916 for you but, I think we have these boards available remotely on qc.lab.
>
> Could you test those boards out remotely yourself ?
As I mentioned in the cover letter, I am not sure if anybody cares about
this breaking or not.. But I'm willing to test not-deprecated-bindings 845/
7180.
Konrad
On Fri, Feb 09, 2024 at 10:09:51PM +0100, Konrad Dybcio wrote:
> Nothing inside the resource struct needs to be mutable. Sprinkle
> 'const' all over it. A lot of 'const'.
We already have 'const struct venus_resources'. Is that not sufficient?
Maybe it's just style, since the string arrays are const?
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> drivers/media/platform/qcom/venus/core.h | 58 ++++++++++++++++----------------
> 1 file changed, 29 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index 6a77de374454..6b1887f7d9cb 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -56,39 +56,39 @@ enum vpu_version {
> };
>
> struct venus_resources {
> - u64 dma_mask;
> - const struct freq_tbl *freq_tbl;
> - unsigned int freq_tbl_size;
> - const struct bw_tbl *bw_tbl_enc;
> - unsigned int bw_tbl_enc_size;
> - const struct bw_tbl *bw_tbl_dec;
> - unsigned int bw_tbl_dec_size;
> - const struct reg_val *reg_tbl;
> - unsigned int reg_tbl_size;
> - const struct hfi_ubwc_config *ubwc_conf;
> + const u64 dma_mask;
> + const struct freq_tbl * const freq_tbl;
> + const unsigned int freq_tbl_size;
> + const struct bw_tbl * const bw_tbl_enc;
> + const unsigned int bw_tbl_enc_size;
> + const struct bw_tbl * const bw_tbl_dec;
> + const unsigned int bw_tbl_dec_size;
> + const struct reg_val * const reg_tbl;
> + const unsigned int reg_tbl_size;
> + const struct hfi_ubwc_config * const ubwc_conf;
> const char * const clks[VIDC_CLKS_NUM_MAX];
> - unsigned int clks_num;
> + const unsigned int clks_num;
> const char * const vcodec0_clks[VIDC_VCODEC_CLKS_NUM_MAX];
> const char * const vcodec1_clks[VIDC_VCODEC_CLKS_NUM_MAX];
> - unsigned int vcodec_clks_num;
> - const char **vcodec_pmdomains;
> - unsigned int vcodec_pmdomains_num;
> - const char **opp_pmdomain;
> - unsigned int vcodec_num;
> + const unsigned int vcodec_clks_num;
> + const char * const *vcodec_pmdomains;
This doesn't error on:
const char *pmdomains[] = { "venus", "vcodec0", "vcodec1" };
struct venus_resources res;
res.vcodec_pmdomains = pmdomains;
res.vcodec_pmdomains = NULL;
> + const unsigned int vcodec_pmdomains_num;
> + const char * const * const opp_pmdomain;
> + const unsigned int vcodec_num;
> const char * const resets[VIDC_RESETS_NUM_MAX];
> - unsigned int resets_num;
> - enum hfi_version hfi_version;
> - enum vpu_version vpu_version;
> - u8 num_vpp_pipes;
> - u32 max_load;
> - unsigned int vmem_id;
> - u32 vmem_size;
> - u32 vmem_addr;
> - u32 cp_start;
> - u32 cp_size;
> - u32 cp_nonpixel_start;
> - u32 cp_nonpixel_size;
> - const char *fwname;
> + const unsigned int resets_num;
> + const enum hfi_version hfi_version;
> + const enum vpu_version vpu_version;
> + const u8 num_vpp_pipes;
> + const u32 max_load;
> + const unsigned int vmem_id;
> + const u32 vmem_size;
> + const u32 vmem_addr;
> + const u32 cp_start;
> + const u32 cp_size;
> + const u32 cp_nonpixel_start;
> + const u32 cp_nonpixel_size;
> + const char * const fwname;
> };
>
> enum venus_fmt {
>
> --
> 2.43.0
>
On Mi, 2024-02-14 at 22:20 +0100, Konrad Dybcio wrote:
> On 14.02.2024 14:31, Philipp Zabel wrote:
> > Hi Konrad,
> >
> > On Fr, 2024-02-09 at 22:10 +0100, Konrad Dybcio wrote:
> > > All of the resets are toggled together. Use the bulk api to save on some
> > > code complexity.
> > >
> > > The delay between resets is now correctly determined by the reset
> > > framework.
> >
> > If this is a recent change, could you reference the commit?
>
> It's a series that recently landed in -next [1]
Missing link?
> [...]
>
> >
> > Since VIDC_RESETS_NUM_MAX is only 2, I don't think a separate
> > allocation is worth it.
>
> It's 2 today, anyway. I wanted to keep it flexible
If this is expected to grow, fine.
> [...]
>
> > > + ret = reset_control_bulk_reset(res->resets_num, core->resets);
> > > + if (ret)
> > > + dev_err(core->dev, "Failed to toggle resets: %d\n", ret);
> > >
> > > -err:
> > > return ret;
> >
> > Could be simplified to:
> >
> > return reset_control_bulk_reset(res->resets_num, core-
> > > resets);
>
> I intentionally kept the if (ret) to print a specific error message
> in case the call fails, this driver doesn't go a good job of telling
> the user/developer what went wrong.
Oh, ok.
regards
Philipp
On 21.02.2024 14:34, Philipp Zabel wrote:
> On Mi, 2024-02-14 at 22:20 +0100, Konrad Dybcio wrote:
>> On 14.02.2024 14:31, Philipp Zabel wrote:
>>> Hi Konrad,
>>>
>>> On Fr, 2024-02-09 at 22:10 +0100, Konrad Dybcio wrote:
>>>> All of the resets are toggled together. Use the bulk api to save on some
>>>> code complexity.
>>>>
>>>> The delay between resets is now correctly determined by the reset
>>>> framework.
>>>
>>> If this is a recent change, could you reference the commit?
>>
>> It's a series that recently landed in -next [1]
>
> Missing link?
Yes, sorry!
Konrad
[1] https://lore.kernel.org/linux-arm-msm/[email protected]/
On Mi, 2024-02-21 at 14:37 +0100, Konrad Dybcio wrote:
> On 21.02.2024 14:34, Philipp Zabel wrote:
> > On Mi, 2024-02-14 at 22:20 +0100, Konrad Dybcio wrote:
> > > On 14.02.2024 14:31, Philipp Zabel wrote:
> > > > Hi Konrad,
> > > >
> > > > On Fr, 2024-02-09 at 22:10 +0100, Konrad Dybcio wrote:
> > > > > All of the resets are toggled together. Use the bulk api to save on some
> > > > > code complexity.
> > > > >
> > > > > The delay between resets is now correctly determined by the reset
> > > > > framework.
> > > >
> > > > If this is a recent change, could you reference the commit?
> > >
> > > It's a series that recently landed in -next [1]
> >
> > Missing link?
>
> Yes, sorry!
>
> Konrad
>
> [1] https://lore.kernel.org/linux-arm-msm/[email protected]/
Thank you. With that,
Reviewed-by: Philipp Zabel <[email protected]>
regards
Philipp
On 2/10/2024 2:39 AM, Konrad Dybcio wrote:
> 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;
> -
Removing the if check and relying only on for loop is good.
but I don't see the for loop here.
> ret = dev_pm_domain_attach_list(dev, &vcodec_data, &core->pmdomains);
> if (ret < 0)
> return ret;
>
Also, what's the base of this change? I don't see above API in the code
anywhere.
> -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]);
>
On 2/10/2024 2:39 AM, Konrad Dybcio wrote:
> There is no reason to keep reset_get code local to HFIv4/v6.
>
> Move it to the common part.
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> drivers/media/platform/qcom/venus/core.c | 9 ++++++++-
> drivers/media/platform/qcom/venus/pm_helpers.c | 23 -----------------------
> 2 files changed, 8 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 5ab3c414ec0f..0652065cb113 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -15,6 +15,7 @@
> #include <linux/of_platform.h>
> #include <linux/platform_device.h>
> #include <linux/pm_opp.h>
> +#include <linux/reset.h>
> #include <linux/slab.h>
> #include <linux/types.h>
> #include <linux/pm_domain.h>
> @@ -286,7 +287,7 @@ static int venus_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> struct venus_core *core;
> - int ret;
> + int i, ret;
>
> core = devm_kzalloc(dev, sizeof(*core), GFP_KERNEL);
> if (!core)
> @@ -324,6 +325,12 @@ static int venus_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> + for (i = 0; i < core->res->resets_num; i++) {
> + core->resets[i] = devm_reset_control_get_exclusive(dev, core->res->resets[i]);
> + if (IS_ERR(core->resets[i]))
> + return PTR_ERR(core->resets[i]);
> + }
> +
> 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 7193075e8c04..6017a9236bff 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
> @@ -939,25 +939,6 @@ static int core_resets_reset(struct venus_core *core)
> return ret;
> }
>
> -static int core_resets_get(struct venus_core *core)
> -{
> - struct device *dev = core->dev;
> - const struct venus_resources *res = core->res;
> - unsigned int i;
> - int ret;
> -
> - for (i = 0; i < res->resets_num; i++) {
> - core->resets[i] =
> - devm_reset_control_get_exclusive(dev, res->resets[i]);
> - if (IS_ERR(core->resets[i])) {
> - ret = PTR_ERR(core->resets[i]);
> - return ret;
> - }
> - }
> -
> - return 0;
> -}
> -
resets are applicable to only v6 so it should be ok to keep this only in
core_get_v4 which is invoked for v6 as well. common code should be common
for all SOCs.
> static int core_get_v4(struct venus_core *core)
> {
> struct device *dev = core->dev;
> @@ -981,10 +962,6 @@ static int core_get_v4(struct venus_core *core)
> if (ret)
> return ret;
>
> - ret = core_resets_get(core);
> - if (ret)
> - return ret;
> -
> if (legacy_binding)
> return 0;
>
>
On 2/10/2024 2:39 AM, Konrad Dybcio wrote:
> Without an OPP table and with vcodec_pmdomains_num (so, v1, v3 and
> sdm845_legacy targets), core_put_v4 is a NOP, jut like core_put_v1.
> Unify them!
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> drivers/media/platform/qcom/venus/core.c | 8 +++-----
> drivers/media/platform/qcom/venus/pm_helpers.c | 17 +----------------
> drivers/media/platform/qcom/venus/pm_helpers.h | 2 +-
> 3 files changed, 5 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 9bfd2a30084b..666adc5aac38 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -426,15 +426,14 @@ static int venus_probe(struct platform_device *pdev)
> err_core_deinit:
> hfi_core_deinit(core, false);
> err_core_put:
> - if (core->pm_ops->core_put)
> - core->pm_ops->core_put(core);
> + vcodec_domains_put(core);
> +
> return ret;
> }
>
> static void venus_remove(struct platform_device *pdev)
> {
> struct venus_core *core = platform_get_drvdata(pdev);
> - const struct venus_pm_ops *pm_ops = core->pm_ops;
> struct device *dev = core->dev;
> int ret;
>
> @@ -452,8 +451,7 @@ static void venus_remove(struct platform_device *pdev)
> pm_runtime_put_sync(dev);
> pm_runtime_disable(dev);
>
> - if (pm_ops->core_put)
> - pm_ops->core_put(core);
> + vcodec_domains_put(core);
>
> v4l2_device_unregister(&core->v4l2_dev);
>
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
> index 6f6de9ef1c6c..32f9ccfa9d8a 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
> @@ -318,13 +318,8 @@ static int load_scale_v1(struct venus_inst *inst)
> return ret;
> }
>
> -static void core_put_v1(struct venus_core *core)
> -{
> -}
> -
> static const struct venus_pm_ops pm_ops_v1 = {
> .core_get = venus_clks_get,
> - .core_put = core_put_v1,
> .load_scale = load_scale_v1,
> };
>
> @@ -396,7 +391,6 @@ static int venc_power_v3(struct device *dev, int on)
>
> static const struct venus_pm_ops pm_ops_v3 = {
> .core_get = venus_clks_get,
> - .core_put = core_put_v1,
> .vdec_get = vdec_get_v3,
> .vdec_power = vdec_power_v3,
> .venc_get = venc_get_v3,
> @@ -893,7 +887,7 @@ static int vcodec_domains_get(struct venus_core *core)
> return ret;
> }
>
> -static void vcodec_domains_put(struct venus_core *core)
> +void vcodec_domains_put(struct venus_core *core)
> {
> dev_pm_domain_detach_list(core->pmdomains);
>
what is the base of this change?
I don't see dev_pm_domain_detach_list in mainline code.
Am I missing anything here?
> @@ -968,14 +962,6 @@ static int core_get_v4(struct venus_core *core)
> return 0;
> }
>
> -static void core_put_v4(struct venus_core *core)
> -{
> - if (legacy_binding)
> - return;
> -
> - vcodec_domains_put(core);
> -}
> -
> int venus_core_power(struct venus_core *core, int on)
> {
> struct device *dev = core->dev;
> @@ -1123,7 +1109,6 @@ static int load_scale_v4(struct venus_inst *inst)
>
> static const struct venus_pm_ops pm_ops_v4 = {
> .core_get = core_get_v4,
> - .core_put = core_put_v4,
> .vdec_get = vdec_get_v4,
> .vdec_put = vdec_put_v4,
> .vdec_power = vdec_power_v4,
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.h b/drivers/media/platform/qcom/venus/pm_helpers.h
> index 77db940a265c..3014b39aa6e3 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.h
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.h
> @@ -11,7 +11,6 @@ struct venus_core;
>
> struct venus_pm_ops {
> int (*core_get)(struct venus_core *core);
> - void (*core_put)(struct venus_core *core);
>
> int (*vdec_get)(struct device *dev);
> void (*vdec_put)(struct device *dev);
> @@ -28,6 +27,7 @@ struct venus_pm_ops {
>
> const struct venus_pm_ops *venus_pm_get(enum hfi_version version);
> int venus_core_power(struct venus_core *core, int on);
> +void vcodec_domains_put(struct venus_core *core);
>
> static inline int venus_pm_load_scale(struct venus_inst *inst)
> {
>
On 2/10/2024 2:40 AM, Konrad Dybcio wrote:
> As has been the story with the past few commits, much of the resource
> acquisition logic is totally identical between different generations
> and there's no good reason to invent a new function for each one.
>
> Commonize core_get() and rename it to venus_get_resources() to be more
> meaningful.
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> drivers/media/platform/qcom/venus/core.c | 8 +++-----
> drivers/media/platform/qcom/venus/pm_helpers.c | 5 +----
> drivers/media/platform/qcom/venus/pm_helpers.h | 3 +--
> 3 files changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 680674dd0d68..873affe17537 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -334,11 +334,9 @@ static int venus_probe(struct platform_device *pdev)
> return PTR_ERR(core->resets[i]);
> }
>
> - if (core->pm_ops->core_get) {
> - ret = core->pm_ops->core_get(core);
> - if (ret)
> - return ret;
> - }
> + ret = venus_get_resources(core);
> + if (ret)
> + return ret;
>
> ret = dma_set_mask_and_coherent(dev, res->dma_mask);
> if (ret)
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
> index a292c788ffba..1cbcffbc29af 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
> @@ -326,7 +326,6 @@ static int load_scale_v1(struct venus_inst *inst)
> }
>
> static const struct venus_pm_ops pm_ops_v1 = {
> - .core_get = venus_clks_get,
core_get is initialized with venus_clks_get in patch 4 and then being
removed here. It would be better to combine both patches.
> .load_scale = load_scale_v1,
> };
>
> @@ -395,7 +394,6 @@ static int venc_power_v3(struct device *dev, int on)
> }
>
> static const struct venus_pm_ops pm_ops_v3 = {
> - .core_get = venus_clks_get,
> .vdec_get = vdec_get_v3,
> .vdec_power = vdec_power_v3,
> .venc_get = venc_get_v3,
> @@ -920,7 +918,7 @@ static int core_resets_reset(struct venus_core *core)
> return ret;
> }
>
> -static int core_get_v4(struct venus_core *core)
> +int venus_get_resources(struct venus_core *core)
> {
> struct device *dev = core->dev;
> const struct venus_resources *res = core->res;
> @@ -1109,7 +1107,6 @@ static int load_scale_v4(struct venus_inst *inst)
> }
>
> static const struct venus_pm_ops pm_ops_v4 = {
> - .core_get = core_get_v4,
> .vdec_get = vdec_get_v4,
> .vdec_put = vdec_put_v4,
> .vdec_power = vdec_power_v4,
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.h b/drivers/media/platform/qcom/venus/pm_helpers.h
> index 3014b39aa6e3..7a55a55029f3 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.h
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.h
> @@ -10,8 +10,6 @@ struct venus_core;
> #define POWER_OFF 0
>
> struct venus_pm_ops {
> - int (*core_get)(struct venus_core *core);
> -
> int (*vdec_get)(struct device *dev);
> void (*vdec_put)(struct device *dev);
> int (*vdec_power)(struct device *dev, int on);
> @@ -28,6 +26,7 @@ struct venus_pm_ops {
> const struct venus_pm_ops *venus_pm_get(enum hfi_version version);
> int venus_core_power(struct venus_core *core, int on);
> void vcodec_domains_put(struct venus_core *core);
> +int venus_get_resources(struct venus_core *core);
>
> static inline int venus_pm_load_scale(struct venus_inst *inst)
> {
>
On 3/4/24 07:57, Dikshita Agarwal wrote:
>
>
> On 2/10/2024 2:39 AM, Konrad Dybcio wrote:
>> Without an OPP table and with vcodec_pmdomains_num (so, v1, v3 and
>> sdm845_legacy targets), core_put_v4 is a NOP, jut like core_put_v1.
>> Unify them!
>>
>> Signed-off-by: Konrad Dybcio <[email protected]>
>> ---
[...]
>> -static void vcodec_domains_put(struct venus_core *core)
>> +void vcodec_domains_put(struct venus_core *core)
>> {
>> dev_pm_domain_detach_list(core->pmdomains);
>>
> what is the base of this change?
> I don't see dev_pm_domain_detach_list in mainline code.
> Am I missing anything here?
The base is specified in the cover letter.
The code in question is indeed here since January, perhaps
you're looking at something older than -next.
Konrad
On 3/5/2024 4:22 AM, Konrad Dybcio wrote:
>
>
> On 3/4/24 07:57, Dikshita Agarwal wrote:
>>
>>
>> On 2/10/2024 2:39 AM, Konrad Dybcio wrote:
>>> Without an OPP table and with vcodec_pmdomains_num (so, v1, v3 and
>>> sdm845_legacy targets), core_put_v4 is a NOP, jut like core_put_v1.
>>> Unify them!
>>>
>>> Signed-off-by: Konrad Dybcio <[email protected]>
>>> ---
>
> [...]
>
>
>>> -static void vcodec_domains_put(struct venus_core *core)
>>> +void vcodec_domains_put(struct venus_core *core)
>>> {
>>> dev_pm_domain_detach_list(core->pmdomains);
>>>
>> what is the base of this change?
>> I don't see dev_pm_domain_detach_list in mainline code.
>> Am I missing anything here?
>
> The base is specified in the cover letter.
>
> The code in question is indeed here since January, perhaps
> you're looking at something older than -next.
>
Thanks for the clarification, I was looking at media tree which
surprisingly doesn't have the base change.
> Konrad
>
On 09/02/2024 21:09, Konrad Dybcio wrote:
> "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;
>
>
We have vcodec_clks_get(). It seems a bit nit-picky but if you are
tidying up the namepsace, then I'd suggest venus_core_clks_get() or
vcore_clks_get().
Seems more consistent.
---
bod
On 09/02/2024 21:09, Konrad Dybcio wrote:
> 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
Get non-codec Venus clocks.
> + * @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).
(0-2pcs) is hard for me to decode => zero to two parts?
Why are we double quoting "main logic" feels a bit "Dr Evil"
Suggest hyphenating which would do the same thing:
'one clock for the core-logic||main-logic'
> + *
> + * 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.
"statically defined" not "statically proclaimed"
> + *
> + * SDM845 features two video cores, each one of which may or may not be
> + * subdivided into 2 enc/dec threads.
"into two encoder/decoder 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).
One not 1
> + *
> + * 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..
"We need to support legacy bindings"
"sub-node"
> + *
> + * Return: 0 on success, negative errno on failure.
> + */
> static int venus_clks_get(struct venus_core *core)
> {
> const struct venus_resources *res = core->res;
>
With that fixed.
Reviewed-by: Bryan O'Donoghue <[email protected]>
On 09/02/2024 21:09, Konrad Dybcio wrote:
> Instead of redefining the same literals over and over again, define
> them once and point the reference to that definition.
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
Reviewed-by: Bryan O'Donoghue <[email protected]>
On 09/02/2024 21:09, Konrad Dybcio wrote:
> That field was only introduced to differentiate between the legacy and
> non-legacy SDM845 binding. Get rid of it.
>
> Signed-off-by: Konrad Dybcio <[email protected]>
Reviewed-by: Bryan O'Donoghue <[email protected]>
On 09/02/2024 21:09, Konrad Dybcio wrote:
> Currently VMEM/OCMEM/LLCC is disabled on all platforms.
>
> Make it unconditional to save on space.
>
> These caches will not be enabled until the Venus driver can reference
> them as chunks of SRAM (they're modelled as separate devices) to avoid
> hardcoding magic addresses and rougely accessing the hardware,
> bypassing the normal accessors.
>
> Signed-off-by: Konrad Dybcio <[email protected]>
Agreed, this is dead code.
Reviewed-by: Bryan O'Donoghue <[email protected]>
On 09/02/2024 21:09, Konrad Dybcio wrote:
> The raw literals mean very little. Substitute it with more telling
> bitops macros.
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
Reviewed-by: Bryan O'Donoghue <[email protected]>
On 09/02/2024 21:09, Konrad Dybcio wrote:
> It's hardcoded to be zero. Always. Ever since msm-3.10. Or maybe
> even before. Remove it!
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
Reviewed-by: Bryan O'Donoghue <[email protected]>
On 6.03.2024 1:20 PM, Bryan O'Donoghue wrote:
> On 09/02/2024 21:09, Konrad Dybcio wrote:
>> 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
>
> Get non-codec Venus clocks.
No, this is not necessarily the case.. these may still include
vcodec clocks, that are specified under the root venus node (which
is the only way we'd like to keep)
I applied the rest of your suggestions, do I keep your rb?
Konrad
On 6.03.2024 12:48 PM, Bryan O'Donoghue wrote:
> On 09/02/2024 21:09, Konrad Dybcio wrote:
>> "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;
>>
>
> We have vcodec_clks_get(). It seems a bit nit-picky but if you are tidying up the namepsace, then I'd suggest venus_core_clks_get() or vcore_clks_get().
>
> Seems more consistent.
No, that's not the point, you got confused by the inconsistent namespace :/
These are not any "core clocks", they're either "all clocks except vcodec" or
"all clocks", depending on the binding type used
Konrad
On 4.03.2024 8:13 AM, Dikshita Agarwal wrote:
>
>
> On 2/10/2024 2:40 AM, Konrad Dybcio wrote:
>> As has been the story with the past few commits, much of the resource
>> acquisition logic is totally identical between different generations
>> and there's no good reason to invent a new function for each one.
>>
>> Commonize core_get() and rename it to venus_get_resources() to be more
>> meaningful.
>>
>> Signed-off-by: Konrad Dybcio <[email protected]>
>> ---
>> drivers/media/platform/qcom/venus/core.c | 8 +++-----
>> drivers/media/platform/qcom/venus/pm_helpers.c | 5 +----
>> drivers/media/platform/qcom/venus/pm_helpers.h | 3 +--
>> 3 files changed, 5 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
>> index 680674dd0d68..873affe17537 100644
>> --- a/drivers/media/platform/qcom/venus/core.c
>> +++ b/drivers/media/platform/qcom/venus/core.c
>> @@ -334,11 +334,9 @@ static int venus_probe(struct platform_device *pdev)
>> return PTR_ERR(core->resets[i]);
>> }
>>
>> - if (core->pm_ops->core_get) {
>> - ret = core->pm_ops->core_get(core);
>> - if (ret)
>> - return ret;
>> - }
>> + ret = venus_get_resources(core);
>> + if (ret)
>> + return ret;
>>
>> ret = dma_set_mask_and_coherent(dev, res->dma_mask);
>> if (ret)
>> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
>> index a292c788ffba..1cbcffbc29af 100644
>> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
>> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
>> @@ -326,7 +326,6 @@ static int load_scale_v1(struct venus_inst *inst)
>> }
>>
>> static const struct venus_pm_ops pm_ops_v1 = {
>> - .core_get = venus_clks_get,
> core_get is initialized with venus_clks_get in patch 4 and then being
> removed here. It would be better to combine both patches.
Generally I'd agree, but this series is rather big and both patch 4 and
this one are logically separated well enough, so I'd rather not waste time
moving things around and fighting merge conflicts for no functional change
Konrad
On 4.03.2024 6:40 AM, Dikshita Agarwal wrote:
>
>
> On 2/10/2024 2:39 AM, Konrad Dybcio wrote:
>> 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;
>> -
> Removing the if check and relying only on for loop is good.
> but I don't see the for loop here.
>> ret = dev_pm_domain_attach_list(dev, &vcodec_data, &core->pmdomains);
>> if (ret < 0)
>> return ret;
>>
> Also, what's the base of this change? I don't see above API in the code
> anywhere.
It's inside the dev_pm_domain_attach_list helper.. It was there explicitly
when I first submitted the patch.
Konrad
On 26/03/2024 21:23, Konrad Dybcio wrote:
> On 6.03.2024 1:20 PM, Bryan O'Donoghue wrote:
>> On 09/02/2024 21:09, Konrad Dybcio wrote:
>>> 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
>>
>> Get non-codec Venus clocks.
>
> No, this is not necessarily the case.. these may still include
> vcodec clocks, that are specified under the root venus node (which
> is the only way we'd like to keep)
>
> I applied the rest of your suggestions, do I keep your rb?
>
> Konrad
>
Sure
BTW, I plan to test this series when I can - do you have a working tree ?
---
bod
On 27.03.2024 10:55 AM, Bryan O'Donoghue wrote:
> On 26/03/2024 21:23, Konrad Dybcio wrote:
>> On 6.03.2024 1:20 PM, Bryan O'Donoghue wrote:
>>> On 09/02/2024 21:09, Konrad Dybcio wrote:
>>>> 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
>>>
>>> Get non-codec Venus clocks.
>>
>> No, this is not necessarily the case.. these may still include
>> vcodec clocks, that are specified under the root venus node (which
>> is the only way we'd like to keep)
>>
>> I applied the rest of your suggestions, do I keep your rb?
>>
>> Konrad
>>
>
> Sure
>
> BTW, I plan to test this series when I can - do you have a working tree ?
next + the patchset in question is a working tree..
Konrad