SM8350 and SC8280XP both implement IRIS2, with the bigger SoC having
a bit of a beefier unit, capable of reaching a higher core clock.
Rebased atop Stan's venus-for-6.6.
Signed-off-by: Konrad Dybcio <[email protected]>
---
Konrad Dybcio (6):
media: dt-bindings: Document SC8280XP/SM8350 Venus
media: venus: core: Remove trailing commas from of match entries
media: venus: hfi_venus: Support only updating certain bits with presets
media: platform: venus: Add optional LLCC path
media: venus: core: Add SM8350 resource struct
media: venus: core: Add SC8280XP resource struct
.../bindings/media/qcom,sm8350-venus.yaml | 149 +++++++++++++++++++++
drivers/media/platform/qcom/venus/core.c | 119 ++++++++++++++--
drivers/media/platform/qcom/venus/core.h | 4 +
drivers/media/platform/qcom/venus/hfi_venus.c | 15 ++-
drivers/media/platform/qcom/venus/pm_helpers.c | 3 +
5 files changed, 279 insertions(+), 11 deletions(-)
---
base-commit: 210fefeb11b4bf5d4c5597f126425c2d3fea1aa9
change-id: 20230731-topic-8280_venus-8f506ae723a6
Best regards,
--
Konrad Dybcio <[email protected]>
Add SM8350 configuration data and related compatible.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/media/platform/qcom/venus/core.c | 39 ++++++++++++++++++++++++++++++++
1 file changed, 39 insertions(+)
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index db363061748f..5f285ae75e9d 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -844,6 +844,44 @@ static const struct venus_resources sm8250_res = {
.fwname = "qcom/vpu-1.0/venus.mdt",
};
+static const struct reg_val sm8350_reg_preset[] = {
+ { 0xb0088, 0, 0x11 },
+};
+
+static const struct venus_resources sm8350_res = {
+ .freq_tbl = sm8250_freq_table,
+ .freq_tbl_size = ARRAY_SIZE(sm8250_freq_table),
+ .reg_tbl = sm8350_reg_preset,
+ .reg_tbl_size = ARRAY_SIZE(sm8350_reg_preset),
+ .bw_tbl_enc = sm8250_bw_table_enc,
+ .bw_tbl_enc_size = ARRAY_SIZE(sm8250_bw_table_enc),
+ .bw_tbl_dec = sm8250_bw_table_dec,
+ .bw_tbl_dec_size = ARRAY_SIZE(sm8250_bw_table_dec),
+ .clks = { "core", "iface" },
+ .clks_num = 2,
+ .resets = { "core" },
+ .resets_num = 1,
+ .vcodec0_clks = { "vcodec0_core" },
+ .vcodec_clks_num = 1,
+ .vcodec_pmdomains = { "venus", "vcodec0" },
+ .vcodec_pmdomains_num = 2,
+ .opp_pmdomain = (const char *[]) { "mx", NULL },
+ .vcodec_num = 1,
+ .max_load = 7833600, /* 7680x4320@60fps */
+ .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 = GENMASK(31, 29) - 1,
+ .cp_start = 0,
+ .cp_size = 0x25800000,
+ .cp_nonpixel_start = 0x1000000,
+ .cp_nonpixel_size = 0x24800000,
+ .fwname = "qcom/vpu-2.0/venus.mbn",
+};
+
static const struct freq_tbl sc7280_freq_table[] = {
{ 0, 460000000 },
{ 0, 424000000 },
@@ -911,6 +949,7 @@ static const struct of_device_id venus_dt_match[] = {
{ .compatible = "qcom,sc7180-venus", .data = &sc7180_res },
{ .compatible = "qcom,sc7280-venus", .data = &sc7280_res },
{ .compatible = "qcom,sm8250-venus", .data = &sm8250_res },
+ { .compatible = "qcom,sm8350-venus", .data = &sm8350_res },
{ }
};
MODULE_DEVICE_TABLE(of, venus_dt_match);
--
2.41.0
On some platforms (like SM8350) we're expected to only touch certain bits
(such as 0 and 4 corresponding to mask 0x11). Add support for doing so.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/media/platform/qcom/venus/core.h | 1 +
drivers/media/platform/qcom/venus/hfi_venus.c | 15 ++++++++++++---
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index d996abd339e1..2999c24392f5 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -38,6 +38,7 @@ struct freq_tbl {
struct reg_val {
u32 reg;
u32 value;
+ u32 mask;
};
struct bw_tbl {
diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index 19fc6575a489..d4bad66f7293 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -349,10 +349,19 @@ static void venus_set_registers(struct venus_hfi_device *hdev)
const struct venus_resources *res = hdev->core->res;
const struct reg_val *tbl = res->reg_tbl;
unsigned int count = res->reg_tbl_size;
- unsigned int i;
+ unsigned int i, val;
+
+ for (i = 0; i < count; i++) {
+ val = tbl[i].value;
- for (i = 0; i < count; i++)
- writel(tbl[i].value, hdev->core->base + tbl[i].reg);
+ /* In some cases, we only want to update certain bits */
+ if (tbl[i].mask) {
+ val = readl(hdev->core->base + tbl[i].reg);
+ val = (val & ~tbl[i].mask) | (tbl[i].value & tbl[i].mask);
+ }
+
+ writel(val, hdev->core->base + tbl[i].reg);
+ }
}
static void venus_soft_int(struct venus_hfi_device *hdev)
--
2.41.0
On 04/08/2023 21:50, Bryan O'Donoghue wrote:
> On 04/08/2023 21:09, Konrad Dybcio wrote:
>> On some platforms (like SM8350) we're expected to only touch certain bits
>> (such as 0 and 4 corresponding to mask 0x11). Add support for doing so.
>>
>> Signed-off-by: Konrad Dybcio <[email protected]>
>> ---
>> drivers/media/platform/qcom/venus/core.h | 1 +
>> drivers/media/platform/qcom/venus/hfi_venus.c | 15 ++++++++++++---
>> 2 files changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/core.h
>> b/drivers/media/platform/qcom/venus/core.h
>> index d996abd339e1..2999c24392f5 100644
>> --- a/drivers/media/platform/qcom/venus/core.h
>> +++ b/drivers/media/platform/qcom/venus/core.h
>> @@ -38,6 +38,7 @@ struct freq_tbl {
>> struct reg_val {
>> u32 reg;
>> u32 value;
>> + u32 mask;
>> };
>> struct bw_tbl {
>> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c
>> b/drivers/media/platform/qcom/venus/hfi_venus.c
>> index 19fc6575a489..d4bad66f7293 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
>> @@ -349,10 +349,19 @@ static void venus_set_registers(struct
>> venus_hfi_device *hdev)
>> const struct venus_resources *res = hdev->core->res;
>> const struct reg_val *tbl = res->reg_tbl;
>> unsigned int count = res->reg_tbl_size;
>> - unsigned int i;
>> + unsigned int i, val;
>
> u32 val;
>
>> +
>> + for (i = 0; i < count; i++) {
>> + val = tbl[i].value;
>
> struct reg_val looks like this
>
> struct reg_val {
> u32 reg;
> u32 value;
> };
>
> val should be declared a u32
>
>> - for (i = 0; i < count; i++)
>> - writel(tbl[i].value, hdev->core->base + tbl[i].reg);
>> + /* In some cases, we only want to update certain bits */
>
> I'll trust this is a legitimate and true statement.
>
>> + if (tbl[i].mask) {
>> + val = readl(hdev->core->base + tbl[i].reg);
>> + val = (val & ~tbl[i].mask) | (tbl[i].value & tbl[i].mask);
>
> feels like something regmap_update_bits() already does though, I prefer
> this because there's less code in it.
>
>> + }
>> +
>> + writel(val, hdev->core->base + tbl[i].reg);
>> + }
>
> With the val declaration fix
>
> Reviewed-by: Bryan O'Donoghue <[email protected]>
Reviewed-by: Bryan O'Donoghue <[email protected]>
On 04/08/2023 21:09, Konrad Dybcio wrote:
> + .freq_tbl = sm8250_freq_table,
> + .freq_tbl_size = ARRAY_SIZE(sm8250_freq_table),
> + .reg_tbl = sm8350_reg_preset,
> + .reg_tbl_size = ARRAY_SIZE(sm8350_reg_preset),
> + .bw_tbl_enc = sm8250_bw_table_enc,
> + .bw_tbl_enc_size = ARRAY_SIZE(sm8250_bw_table_enc),
> + .bw_tbl_dec = sm8250_bw_table_dec,
> + .bw_tbl_dec_size = ARRAY_SIZE(sm8250_bw_table_dec),
The very same freq and bandwidth tables ?
---
bod
Some newer SoCs (such as SM8350) have a third interconnect path. Add
it and make it optional.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/media/platform/qcom/venus/core.c | 19 +++++++++++++++++++
drivers/media/platform/qcom/venus/core.h | 3 +++
drivers/media/platform/qcom/venus/pm_helpers.c | 3 +++
3 files changed, 25 insertions(+)
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 0af45faec247..db363061748f 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -302,6 +302,15 @@ static int venus_probe(struct platform_device *pdev)
if (IS_ERR(core->cpucfg_path))
return PTR_ERR(core->cpucfg_path);
+ core->llcc_path = devm_of_icc_get(dev, "video-llcc");
+ if (IS_ERR(core->llcc_path)) {
+ /* LLCC path is optional */
+ if (PTR_ERR(core->llcc_path) == -ENODATA)
+ core->llcc_path = NULL;
+ else
+ return PTR_ERR(core->llcc_path);
+ }
+
core->irq = platform_get_irq(pdev, 0);
if (core->irq < 0)
return core->irq;
@@ -479,12 +488,18 @@ static __maybe_unused int venus_runtime_suspend(struct device *dev)
if (ret)
goto err_cpucfg_path;
+ ret = icc_set_bw(core->llcc_path, 0, 0);
+ if (ret)
+ goto err_llcc_path;
+
ret = icc_set_bw(core->video_path, 0, 0);
if (ret)
goto err_video_path;
return ret;
+err_llcc_path:
+ icc_set_bw(core->video_path, kbps_to_icc(20000), 0);
err_video_path:
icc_set_bw(core->cpucfg_path, kbps_to_icc(1000), 0);
err_cpucfg_path:
@@ -504,6 +519,10 @@ static __maybe_unused int venus_runtime_resume(struct device *dev)
if (ret)
return ret;
+ ret = icc_set_bw(core->llcc_path, kbps_to_icc(20000), 0);
+ if (ret)
+ return ret;
+
ret = icc_set_bw(core->cpucfg_path, kbps_to_icc(1000), 0);
if (ret)
return ret;
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 2999c24392f5..45ed1551c2db 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -65,6 +65,7 @@ struct venus_resources {
unsigned int bw_tbl_enc_size;
const struct bw_tbl *bw_tbl_dec;
unsigned int bw_tbl_dec_size;
+ bool has_llcc_path;
const struct reg_val *reg_tbl;
unsigned int reg_tbl_size;
const struct hfi_ubwc_config *ubwc_conf;
@@ -134,6 +135,7 @@ struct venus_format {
* @vcodec1_clks: an array of vcodec1 struct clk pointers
* @video_path: an interconnect handle to video to/from memory path
* @cpucfg_path: an interconnect handle to cpu configuration path
+ * @llcc_path: an interconnect handle to video to/from llcc path
* @has_opp_table: does OPP table exist
* @pmdomains: an array of pmdomains struct device pointers
* @opp_dl_venus: an device-link for device OPP
@@ -186,6 +188,7 @@ struct venus_core {
struct clk *vcodec1_clks[VIDC_VCODEC_CLKS_NUM_MAX];
struct icc_path *video_path;
struct icc_path *cpucfg_path;
+ struct icc_path *llcc_path;
bool has_opp_table;
struct device *pmdomains[VIDC_PMDOMAINS_NUM_MAX];
struct device_link *opp_dl_venus;
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index 48c9084bb4db..79392ff8fa06 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -237,6 +237,9 @@ static int load_scale_bw(struct venus_core *core)
dev_dbg(core->dev, VDBGL "total: avg_bw: %u, peak_bw: %u\n",
total_avg, total_peak);
+ if (core->res->has_llcc_path)
+ icc_set_bw(core->llcc_path, total_avg, total_peak);
+
return icc_set_bw(core->video_path, total_avg, total_peak);
}
--
2.41.0
Both of these SoCs implement an IRIS2 block, with SC8280XP being able
to clock it a bit higher.
Document it.
Signed-off-by: Konrad Dybcio <[email protected]>
---
.../bindings/media/qcom,sm8350-venus.yaml | 149 +++++++++++++++++++++
1 file changed, 149 insertions(+)
diff --git a/Documentation/devicetree/bindings/media/qcom,sm8350-venus.yaml b/Documentation/devicetree/bindings/media/qcom,sm8350-venus.yaml
new file mode 100644
index 000000000000..8a31bce27c18
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/qcom,sm8350-venus.yaml
@@ -0,0 +1,149 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/qcom,sm8350-venus.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm SM8350 Venus video encode and decode accelerators
+
+maintainers:
+ - Konrad Dybcio <[email protected]>
+
+description: |
+ The Venus Iris2 IP is a video encode and decode accelerator present
+ on Qualcomm platforms
+
+allOf:
+ - $ref: qcom,venus-common.yaml#
+
+properties:
+ compatible:
+ enum:
+ - qcom,sc8280xp-venus
+ - qcom,sm8350-venus
+
+ clocks:
+ maxItems: 3
+
+ clock-names:
+ items:
+ - const: iface
+ - const: core
+ - const: vcodec0_core
+
+ resets:
+ maxItems: 1
+
+ reset-names:
+ items:
+ - const: core
+
+ power-domains:
+ maxItems: 3
+
+ power-domain-names:
+ items:
+ - const: venus
+ - const: vcodec0
+ - const: mx
+
+ interconnects:
+ maxItems: 3
+
+ interconnect-names:
+ items:
+ - const: cpu-cfg
+ - const: video-mem
+ - const: video-llcc
+
+ operating-points-v2: true
+ opp-table:
+ type: object
+
+ iommus:
+ maxItems: 1
+
+ video-decoder:
+ type: object
+
+ properties:
+ compatible:
+ const: venus-decoder
+
+ required:
+ - compatible
+
+ additionalProperties: false
+
+ video-encoder:
+ type: object
+
+ properties:
+ compatible:
+ const: venus-encoder
+
+ required:
+ - compatible
+
+ additionalProperties: false
+
+required:
+ - compatible
+ - power-domain-names
+ - iommus
+ - video-decoder
+ - video-encoder
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ #include <dt-bindings/clock/qcom,gcc-sm8350.h>
+ #include <dt-bindings/clock/qcom,sm8350-videocc.h>
+ #include <dt-bindings/interconnect/qcom,sm8350.h>
+ #include <dt-bindings/power/qcom-rpmpd.h>
+
+ venus: video-codec@aa00000 {
+ compatible = "qcom,sm8350-venus";
+ reg = <0x0aa00000 0x100000>;
+ interrupts = <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH>;
+
+ clocks = <&gcc GCC_VIDEO_AXI0_CLK>,
+ <&videocc VIDEO_CC_MVS0C_CLK>,
+ <&videocc VIDEO_CC_MVS0_CLK>;
+ clock-names = "iface",
+ "core",
+ "vcodec0_core";
+
+ resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>;
+ reset-names = "core";
+
+ power-domains = <&videocc MVS0C_GDSC>,
+ <&videocc MVS0_GDSC>,
+ <&rpmhpd SM8350_MX>;
+ power-domain-names = "venus",
+ "vcodec0",
+ "mx";
+
+ interconnects = <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_VENUS_CFG 0>,
+ <&mmss_noc MASTER_VIDEO_P0 0 &mc_virt SLAVE_EBI1 0>,
+ <&mmss_noc MASTER_VIDEO_P0 0 &gem_noc SLAVE_LLCC 0>;
+ interconnect-names = "cpu-cfg",
+ "video-mem",
+ "video-llcc";
+
+ operating-points-v2 = <&venus_opp_table>;
+ iommus = <&apps_smmu 0x2100 0x400>;
+ memory-region = <&pil_video_mem>;
+
+ status = "disabled";
+
+ video-decoder {
+ compatible = "venus-decoder";
+ };
+
+ video-encoder {
+ compatible = "venus-encoder";
+ };
+ };
--
2.41.0
On 04/08/2023 21:09, Konrad Dybcio wrote:
> On some platforms (like SM8350) we're expected to only touch certain bits
> (such as 0 and 4 corresponding to mask 0x11). Add support for doing so.
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> drivers/media/platform/qcom/venus/core.h | 1 +
> drivers/media/platform/qcom/venus/hfi_venus.c | 15 ++++++++++++---
> 2 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index d996abd339e1..2999c24392f5 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -38,6 +38,7 @@ struct freq_tbl {
> struct reg_val {
> u32 reg;
> u32 value;
> + u32 mask;
> };
>
> struct bw_tbl {
> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> index 19fc6575a489..d4bad66f7293 100644
> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> @@ -349,10 +349,19 @@ static void venus_set_registers(struct venus_hfi_device *hdev)
> const struct venus_resources *res = hdev->core->res;
> const struct reg_val *tbl = res->reg_tbl;
> unsigned int count = res->reg_tbl_size;
> - unsigned int i;
> + unsigned int i, val;
u32 val;
> +
> + for (i = 0; i < count; i++) {
> + val = tbl[i].value;
struct reg_val looks like this
struct reg_val {
u32 reg;
u32 value;
};
val should be declared a u32
> - for (i = 0; i < count; i++)
> - writel(tbl[i].value, hdev->core->base + tbl[i].reg);
> + /* In some cases, we only want to update certain bits */
I'll trust this is a legitimate and true statement.
> + if (tbl[i].mask) {
> + val = readl(hdev->core->base + tbl[i].reg);
> + val = (val & ~tbl[i].mask) | (tbl[i].value & tbl[i].mask);
feels like something regmap_update_bits() already does though, I prefer
this because there's less code in it.
> + }
> +
> + writel(val, hdev->core->base + tbl[i].reg);
> + }
With the val declaration fix
Reviewed-by: Bryan O'Donoghue <[email protected]>
Add SC8280XP configuration data and related compatible.
Signed-off-by: Konrad Dybcio <[email protected]>
---
drivers/media/platform/qcom/venus/core.c | 45 ++++++++++++++++++++++++++++++++
1 file changed, 45 insertions(+)
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index 5f285ae75e9d..32591b624a36 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -940,6 +940,50 @@ static const struct venus_resources sc7280_res = {
.fwname = "qcom/vpu-2.0/venus.mbn",
};
+static const struct freq_tbl sc8280xp_freq_table[] = {
+ { 0, 239999999 },
+ { 0, 338000000 },
+ { 0, 366000000 },
+ { 0, 444000000 },
+ { 0, 533000000 },
+ { 0, 560000000 },
+};
+
+static const struct venus_resources sc8280xp_res = {
+ .freq_tbl = sc8280xp_freq_table,
+ .freq_tbl_size = ARRAY_SIZE(sc8280xp_freq_table),
+ .reg_tbl = sm8350_reg_preset,
+ .reg_tbl_size = ARRAY_SIZE(sm8350_reg_preset),
+ .bw_tbl_enc = sm8250_bw_table_enc,
+ .bw_tbl_enc_size = ARRAY_SIZE(sm8250_bw_table_enc),
+ .bw_tbl_dec = sm8250_bw_table_dec,
+ .bw_tbl_dec_size = ARRAY_SIZE(sm8250_bw_table_dec),
+ .clks = { "core", "iface" },
+ .clks_num = 2,
+ .resets = { "core" },
+ .resets_num = 1,
+ .vcodec0_clks = { "vcodec0_core" },
+ .vcodec_clks_num = 1,
+ .vcodec_pmdomains = { "venus", "vcodec0" },
+ .vcodec_pmdomains_num = 2,
+ .opp_pmdomain = (const char *[]) { "mx", NULL },
+ .vcodec_num = 1,
+ .max_load = 7833600, /* 7680x4320@60fps */
+ .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 = GENMASK(31, 29) - 1,
+ .cp_start = 0,
+ .cp_size = 0x25800000,
+ .cp_nonpixel_start = 0x1000000,
+ .cp_nonpixel_size = 0x24800000,
+ .fwname = "qcom/vpu-2.0/venus.mbn",
+};
+
+
static const struct of_device_id venus_dt_match[] = {
{ .compatible = "qcom,msm8916-venus", .data = &msm8916_res },
{ .compatible = "qcom,msm8996-venus", .data = &msm8996_res },
@@ -948,6 +992,7 @@ static const struct of_device_id venus_dt_match[] = {
{ .compatible = "qcom,sdm845-venus-v2", .data = &sdm845_res_v2 },
{ .compatible = "qcom,sc7180-venus", .data = &sc7180_res },
{ .compatible = "qcom,sc7280-venus", .data = &sc7280_res },
+ { .compatible = "qcom,sc8280xp-venus", .data = &sc8280xp_res },
{ .compatible = "qcom,sm8250-venus", .data = &sm8250_res },
{ .compatible = "qcom,sm8350-venus", .data = &sm8350_res },
{ }
--
2.41.0
On 04/08/2023 21:09, Konrad Dybcio wrote:
> Some newer SoCs (such as SM8350) have a third interconnect path. Add
> it and make it optional.
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> drivers/media/platform/qcom/venus/core.c | 19 +++++++++++++++++++
> drivers/media/platform/qcom/venus/core.h | 3 +++
> drivers/media/platform/qcom/venus/pm_helpers.c | 3 +++
> 3 files changed, 25 insertions(+)
>
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 0af45faec247..db363061748f 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -302,6 +302,15 @@ static int venus_probe(struct platform_device *pdev)
> if (IS_ERR(core->cpucfg_path))
> return PTR_ERR(core->cpucfg_path);
>
> + core->llcc_path = devm_of_icc_get(dev, "video-llcc");
> + if (IS_ERR(core->llcc_path)) {
> + /* LLCC path is optional */
> + if (PTR_ERR(core->llcc_path) == -ENODATA)
> + core->llcc_path = NULL;
> + else
> + return PTR_ERR(core->llcc_path);
> + }
> +
> core->irq = platform_get_irq(pdev, 0);
> if (core->irq < 0)
> return core->irq;
> @@ -479,12 +488,18 @@ static __maybe_unused int venus_runtime_suspend(struct device *dev)
> if (ret)
> goto err_cpucfg_path;
>
> + ret = icc_set_bw(core->llcc_path, 0, 0);
> + if (ret)
> + goto err_llcc_path;
> +
> ret = icc_set_bw(core->video_path, 0, 0);
> if (ret)
> goto err_video_path;
>
> return ret;
>
> +err_llcc_path:
> + icc_set_bw(core->video_path, kbps_to_icc(20000), 0);
> err_video_path:
> icc_set_bw(core->cpucfg_path, kbps_to_icc(1000), 0);
> err_cpucfg_path:
> @@ -504,6 +519,10 @@ static __maybe_unused int venus_runtime_resume(struct device *dev)
> if (ret)
> return ret;
>
> + ret = icc_set_bw(core->llcc_path, kbps_to_icc(20000), 0);
> + if (ret)
> + return ret;
> +
I would scream if someone left me this comment but...
In probe we have
video_path =
cpu_cfgpath =
llc_path =
suspend
icc_set_bw(cpu_cfgpath,);
icc_set_bw(llc_path,);
icc_set_bw(video_path,);
resume
icc_set_bw(video_path,);
icc_set_bw(llc_path,);
icc_set_bw(cpu_cfgpath,);
it would be nice to have probe match the ordering ...
> ret = icc_set_bw(core->cpucfg_path, kbps_to_icc(1000), 0);
> if (ret)
> return ret;
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index 2999c24392f5..45ed1551c2db 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -65,6 +65,7 @@ struct venus_resources {
> unsigned int bw_tbl_enc_size;
> const struct bw_tbl *bw_tbl_dec;
> unsigned int bw_tbl_dec_size;
> + bool has_llcc_path;
Why do you need this bool, you can get for llc_path == NULL
---
bod
On 4.08.2023 23:08, Bryan O'Donoghue wrote:
> On 04/08/2023 21:09, Konrad Dybcio wrote:
>> + .freq_tbl = sm8250_freq_table,
>> + .freq_tbl_size = ARRAY_SIZE(sm8250_freq_table),
>> + .reg_tbl = sm8350_reg_preset,
>> + .reg_tbl_size = ARRAY_SIZE(sm8350_reg_preset),
>> + .bw_tbl_enc = sm8250_bw_table_enc,
>> + .bw_tbl_enc_size = ARRAY_SIZE(sm8250_bw_table_enc),
>> + .bw_tbl_dec = sm8250_bw_table_dec,
>> + .bw_tbl_dec_size = ARRAY_SIZE(sm8250_bw_table_dec),
>
> The very same freq and bandwidth tables ?
yep!
Konrad
On 04/08/2023 22:09, Konrad Dybcio wrote:
> Both of these SoCs implement an IRIS2 block, with SC8280XP being able
> to clock it a bit higher.
>
...
> +
> + iommus:
> + maxItems: 1
> +
> + video-decoder:
> + type: object
> +
> + properties:
> + compatible:
> + const: venus-decoder
That's not how compatibles are constructed... missing vendor prefix, SoC
or IP block name.
> +
> + required:
> + - compatible
> +
> + additionalProperties: false
Why do you need this child node? Child nodes without properties are
usually useless.
> +
> + video-encoder:
> + type: object
> +
> + properties:
> + compatible:
> + const: venus-encoder
> +
> + required:
> + - compatible
> +
> + additionalProperties: false
> +
> +required:
> + - compatible
> + - power-domain-names
> + - iommus
> + - video-decoder
> + - video-encoder
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> + #include <dt-bindings/clock/qcom,gcc-sm8350.h>
> + #include <dt-bindings/clock/qcom,sm8350-videocc.h>
> + #include <dt-bindings/interconnect/qcom,sm8350.h>
> + #include <dt-bindings/power/qcom-rpmpd.h>
> +
> + venus: video-codec@aa00000 {
> + compatible = "qcom,sm8350-venus";
> + reg = <0x0aa00000 0x100000>;
> + interrupts = <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH>;
> +
> + clocks = <&gcc GCC_VIDEO_AXI0_CLK>,
> + <&videocc VIDEO_CC_MVS0C_CLK>,
> + <&videocc VIDEO_CC_MVS0_CLK>;
> + clock-names = "iface",
> + "core",
> + "vcodec0_core";
> +
> + resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>;
> + reset-names = "core";
> +
> + power-domains = <&videocc MVS0C_GDSC>,
> + <&videocc MVS0_GDSC>,
> + <&rpmhpd SM8350_MX>;
> + power-domain-names = "venus",
> + "vcodec0",
> + "mx";
> +
> + interconnects = <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_VENUS_CFG 0>,
> + <&mmss_noc MASTER_VIDEO_P0 0 &mc_virt SLAVE_EBI1 0>,
> + <&mmss_noc MASTER_VIDEO_P0 0 &gem_noc SLAVE_LLCC 0>;
> + interconnect-names = "cpu-cfg",
> + "video-mem",
> + "video-llcc";
> +
> + operating-points-v2 = <&venus_opp_table>;
> + iommus = <&apps_smmu 0x2100 0x400>;
> + memory-region = <&pil_video_mem>;
> +
> + status = "disabled";
Drop status.
> +
> + video-decoder {
Best regards,
Krzysztof
On Fri, Aug 04, 2023 at 10:09:11PM +0200, Konrad Dybcio wrote:
> @@ -479,12 +488,18 @@ static __maybe_unused int venus_runtime_suspend(struct device *dev)
> if (ret)
> goto err_cpucfg_path;
>
> + ret = icc_set_bw(core->llcc_path, 0, 0);
> + if (ret)
> + goto err_llcc_path;
> +
> ret = icc_set_bw(core->video_path, 0, 0);
> if (ret)
> goto err_video_path;
>
> return ret;
>
> +err_llcc_path:
> + icc_set_bw(core->video_path, kbps_to_icc(20000), 0);
This looks wrong; you should not try to restore the video path bw which
you have not yet updated here.
Also error labels should be named after what they do, not after where
you jump from (e.g. to avoid mistakes like the above). Perhaps you can
clean up the existing labels before adding the new one.
> err_video_path:
> icc_set_bw(core->cpucfg_path, kbps_to_icc(1000), 0);
> err_cpucfg_path:
Johan
On 5.08.2023 21:29, Krzysztof Kozlowski wrote:
> On 04/08/2023 22:09, Konrad Dybcio wrote:
>> Both of these SoCs implement an IRIS2 block, with SC8280XP being able
>> to clock it a bit higher.
>>
>
> ...
>
>> +
>> + iommus:
>> + maxItems: 1
>> +
>> + video-decoder:
>> + type: object
>> +
>> + properties:
>> + compatible:
>> + const: venus-decoder
>
> That's not how compatibles are constructed... missing vendor prefix, SoC
> or IP block name.
>
>> +
>> + required:
>> + - compatible
>> +
>> + additionalProperties: false
>
> Why do you need this child node? Child nodes without properties are
> usually useless.
For both comments: I aligned with what was there..
The driver abuses these compats to probe enc/dec submodules, even though
every Venus implementation (to my knowledge) is implicitly enc/dec capable..
Perhaps a bigger clean-up is due. I guess I could just create the venc/vdec
devices from the venus core probe and get rid of this fake stuff?
Konrad
On 7.08.2023 12:43, Johan Hovold wrote:
> On Fri, Aug 04, 2023 at 10:09:11PM +0200, Konrad Dybcio wrote:
>
>> @@ -479,12 +488,18 @@ static __maybe_unused int venus_runtime_suspend(struct device *dev)
>> if (ret)
>> goto err_cpucfg_path;
>>
>> + ret = icc_set_bw(core->llcc_path, 0, 0);
>> + if (ret)
>> + goto err_llcc_path;
>> +
>> ret = icc_set_bw(core->video_path, 0, 0);
>> if (ret)
>> goto err_video_path;
>>
>> return ret;
>>
>> +err_llcc_path:
>> + icc_set_bw(core->video_path, kbps_to_icc(20000), 0);
>
> This looks wrong; you should not try to restore the video path bw which
> you have not yet updated here.
Oh whoops :D
>
> Also error labels should be named after what they do, not after where
> you jump from (e.g. to avoid mistakes like the above). Perhaps you can
> clean up the existing labels before adding the new one.
Ack, I wouldn't mind giving this some cleanup.
Konrad
On 07/08/2023 14:41, Konrad Dybcio wrote:
> On 5.08.2023 21:29, Krzysztof Kozlowski wrote:
>> On 04/08/2023 22:09, Konrad Dybcio wrote:
>>> Both of these SoCs implement an IRIS2 block, with SC8280XP being able
>>> to clock it a bit higher.
>>>
>>
>> ...
>>
>>> +
>>> + iommus:
>>> + maxItems: 1
>>> +
>>> + video-decoder:
>>> + type: object
>>> +
>>> + properties:
>>> + compatible:
>>> + const: venus-decoder
>>
>> That's not how compatibles are constructed... missing vendor prefix, SoC
>> or IP block name.
>>
>>> +
>>> + required:
>>> + - compatible
>>> +
>>> + additionalProperties: false
>>
>> Why do you need this child node? Child nodes without properties are
>> usually useless.
> For both comments: I aligned with what was there..
>
> The driver abuses these compats to probe enc/dec submodules, even though
> every Venus implementation (to my knowledge) is implicitly enc/dec capable..
Holy crap, I see...
>
> Perhaps a bigger clean-up is due. I guess I could just create the venc/vdec
> devices from the venus core probe and get rid of this fake stuff?
Few devices (qcom,msm8996-venus.yaml, sdm660, sdm845) have clocks there,
so we actually could stay with these subnodes, just correct the
compatibles to a list with correct prefixes:
qcom,sc8280xp-venus-decoder + qcom,venus-decoder
Best regards,
Krzysztof
On 07/08/2023 17:02, Konrad Dybcio wrote:
> On 7.08.2023 16:04, Krzysztof Kozlowski wrote:
>> On 07/08/2023 14:41, Konrad Dybcio wrote:
>>> On 5.08.2023 21:29, Krzysztof Kozlowski wrote:
>>>> On 04/08/2023 22:09, Konrad Dybcio wrote:
>>>>> Both of these SoCs implement an IRIS2 block, with SC8280XP being able
>>>>> to clock it a bit higher.
>>>>>
>>>>
>>>> ...
>>>>
>>>>> +
>>>>> + iommus:
>>>>> + maxItems: 1
>>>>> +
>>>>> + video-decoder:
>>>>> + type: object
>>>>> +
>>>>> + properties:
>>>>> + compatible:
>>>>> + const: venus-decoder
>>>>
>>>> That's not how compatibles are constructed... missing vendor prefix, SoC
>>>> or IP block name.
>>>>
>>>>> +
>>>>> + required:
>>>>> + - compatible
>>>>> +
>>>>> + additionalProperties: false
>>>>
>>>> Why do you need this child node? Child nodes without properties are
>>>> usually useless.
>>> For both comments: I aligned with what was there..
>>>
>>> The driver abuses these compats to probe enc/dec submodules, even though
>>> every Venus implementation (to my knowledge) is implicitly enc/dec capable..
>>
>> Holy crap, I see...
>>
>>>
>>> Perhaps a bigger clean-up is due. I guess I could just create the venc/vdec
>>> devices from the venus core probe and get rid of this fake stuff?
>>
>> Few devices (qcom,msm8996-venus.yaml, sdm660, sdm845) have clocks there,
>> so we actually could stay with these subnodes, just correct the
>> compatibles to a list with correct prefixes:
>>
>> qcom,sc8280xp-venus-decoder + qcom,venus-decoder
> Hm.. looks like pre-845-v2 (with the v2 being "v2 binding" and not
> "v2 chip" or "v2 hardware") these were used to look up clocks but
> then they were moved to the root node.
>
> I am not quite sure if it makes sense to distinguish e.g.
> sc8280xp-venus-decoder within sc8280xp-venus..>
> Perhaps deprecating the "8916 way" (clocks under subnodes), adding
> some boilerplate to look up clocks/pds in both places and converting
> everybody to the "7180 way" way of doing things (clocks under venus),
> and then getting rid of venus encoder/decoder completely (by calling
> device creation from venus probe) would be better. WDYT?
Yes, this makes more sense. I think this is controlled by
"legacy_binding" variable (see
drivers/media/platform/qcom/venus/pm_helpers.c).
Once all bindings are converted to new one (clocks in main/parent node),
then we can drop the children and instantiate devices as MFD.
Best regards,
Krzysztof
On 7.08.2023 16:04, Krzysztof Kozlowski wrote:
> On 07/08/2023 14:41, Konrad Dybcio wrote:
>> On 5.08.2023 21:29, Krzysztof Kozlowski wrote:
>>> On 04/08/2023 22:09, Konrad Dybcio wrote:
>>>> Both of these SoCs implement an IRIS2 block, with SC8280XP being able
>>>> to clock it a bit higher.
>>>>
>>>
>>> ...
>>>
>>>> +
>>>> + iommus:
>>>> + maxItems: 1
>>>> +
>>>> + video-decoder:
>>>> + type: object
>>>> +
>>>> + properties:
>>>> + compatible:
>>>> + const: venus-decoder
>>>
>>> That's not how compatibles are constructed... missing vendor prefix, SoC
>>> or IP block name.
>>>
>>>> +
>>>> + required:
>>>> + - compatible
>>>> +
>>>> + additionalProperties: false
>>>
>>> Why do you need this child node? Child nodes without properties are
>>> usually useless.
>> For both comments: I aligned with what was there..
>>
>> The driver abuses these compats to probe enc/dec submodules, even though
>> every Venus implementation (to my knowledge) is implicitly enc/dec capable..
>
> Holy crap, I see...
>
>>
>> Perhaps a bigger clean-up is due. I guess I could just create the venc/vdec
>> devices from the venus core probe and get rid of this fake stuff?
>
> Few devices (qcom,msm8996-venus.yaml, sdm660, sdm845) have clocks there,
> so we actually could stay with these subnodes, just correct the
> compatibles to a list with correct prefixes:
>
> qcom,sc8280xp-venus-decoder + qcom,venus-decoder
Hm.. looks like pre-845-v2 (with the v2 being "v2 binding" and not
"v2 chip" or "v2 hardware") these were used to look up clocks but
then they were moved to the root node.
I am not quite sure if it makes sense to distinguish e.g.
sc8280xp-venus-decoder within sc8280xp-venus..
Perhaps deprecating the "8916 way" (clocks under subnodes), adding
some boilerplate to look up clocks/pds in both places and converting
everybody to the "7180 way" way of doing things (clocks under venus),
and then getting rid of venus encoder/decoder completely (by calling
device creation from venus probe) would be better. WDYT?
Konrad
On 7.08.2023 20:44, Bryan O'Donoghue wrote:
> On 07/08/2023 16:02, Konrad Dybcio wrote:
>> On 7.08.2023 16:04, Krzysztof Kozlowski wrote:
>>> On 07/08/2023 14:41, Konrad Dybcio wrote:
>>>> On 5.08.2023 21:29, Krzysztof Kozlowski wrote:
>>>>> On 04/08/2023 22:09, Konrad Dybcio wrote:
>>>>>> Both of these SoCs implement an IRIS2 block, with SC8280XP being able
>>>>>> to clock it a bit higher.
>>>>>>
>>>>>
>>>>> ...
>>>>>
>>>>>> +
>>>>>> + iommus:
>>>>>> + maxItems: 1
>>>>>> +
>>>>>> + video-decoder:
>>>>>> + type: object
>>>>>> +
>>>>>> + properties:
>>>>>> + compatible:
>>>>>> + const: venus-decoder
>>>>>
>>>>> That's not how compatibles are constructed... missing vendor prefix, SoC
>>>>> or IP block name.
>>>>>
>>>>>> +
>>>>>> + required:
>>>>>> + - compatible
>>>>>> +
>>>>>> + additionalProperties: false
>>>>>
>>>>> Why do you need this child node? Child nodes without properties are
>>>>> usually useless.
>>>> For both comments: I aligned with what was there..
>>>>
>>>> The driver abuses these compats to probe enc/dec submodules, even though
>>>> every Venus implementation (to my knowledge) is implicitly enc/dec capable..
>>>
>>> Holy crap, I see...
>>>
>>>>
>>>> Perhaps a bigger clean-up is due. I guess I could just create the venc/vdec
>>>> devices from the venus core probe and get rid of this fake stuff?
>>>
>>> Few devices (qcom,msm8996-venus.yaml, sdm660, sdm845) have clocks there,
>>> so we actually could stay with these subnodes, just correct the
>>> compatibles to a list with correct prefixes:
>>>
>>> qcom,sc8280xp-venus-decoder + qcom,venus-decoder
>> Hm.. looks like pre-845-v2 (with the v2 being "v2 binding" and not
>> "v2 chip" or "v2 hardware") these were used to look up clocks but
>> then they were moved to the root node.
>>
>> I am not quite sure if it makes sense to distinguish e.g.
>> sc8280xp-venus-decoder within sc8280xp-venus..
>>
>> Perhaps deprecating the "8916 way" (clocks under subnodes), adding
>> some boilerplate to look up clocks/pds in both places and converting
>> everybody to the "7180 way" way of doing things (clocks under venus),
>> and then getting rid of venus encoder/decoder completely (by calling
>> device creation from venus probe) would be better. WDYT?
>>
>> Konrad
>
> As I understand it though, for some classes of venus hardware - earlier, it was possible to have two encoders or two decoders and it really didn't - perhaps still doesn't matter which order they are declared in.
>
> That's the logic behind having a compat string that assigns either encoder or decoder to one of the logical blocks.
>
> You can have any mixture of
> - encoder
> - decoder
>
> - encoder
> - encoder
>
> - decoder
> - decoder
>
> - decoder
> - encoder
>
> - encoder
>
> - decoder
>
> I think it should *still* be the case - whether it is a practical reality or not, that any of those mapping can be selected and supported.
That can be taken care of with match data.
Konrad
On 07/08/2023 16:02, Konrad Dybcio wrote:
> On 7.08.2023 16:04, Krzysztof Kozlowski wrote:
>> On 07/08/2023 14:41, Konrad Dybcio wrote:
>>> On 5.08.2023 21:29, Krzysztof Kozlowski wrote:
>>>> On 04/08/2023 22:09, Konrad Dybcio wrote:
>>>>> Both of these SoCs implement an IRIS2 block, with SC8280XP being able
>>>>> to clock it a bit higher.
>>>>>
>>>>
>>>> ...
>>>>
>>>>> +
>>>>> + iommus:
>>>>> + maxItems: 1
>>>>> +
>>>>> + video-decoder:
>>>>> + type: object
>>>>> +
>>>>> + properties:
>>>>> + compatible:
>>>>> + const: venus-decoder
>>>>
>>>> That's not how compatibles are constructed... missing vendor prefix, SoC
>>>> or IP block name.
>>>>
>>>>> +
>>>>> + required:
>>>>> + - compatible
>>>>> +
>>>>> + additionalProperties: false
>>>>
>>>> Why do you need this child node? Child nodes without properties are
>>>> usually useless.
>>> For both comments: I aligned with what was there..
>>>
>>> The driver abuses these compats to probe enc/dec submodules, even though
>>> every Venus implementation (to my knowledge) is implicitly enc/dec capable..
>>
>> Holy crap, I see...
>>
>>>
>>> Perhaps a bigger clean-up is due. I guess I could just create the venc/vdec
>>> devices from the venus core probe and get rid of this fake stuff?
>>
>> Few devices (qcom,msm8996-venus.yaml, sdm660, sdm845) have clocks there,
>> so we actually could stay with these subnodes, just correct the
>> compatibles to a list with correct prefixes:
>>
>> qcom,sc8280xp-venus-decoder + qcom,venus-decoder
> Hm.. looks like pre-845-v2 (with the v2 being "v2 binding" and not
> "v2 chip" or "v2 hardware") these were used to look up clocks but
> then they were moved to the root node.
>
> I am not quite sure if it makes sense to distinguish e.g.
> sc8280xp-venus-decoder within sc8280xp-venus..
>
> Perhaps deprecating the "8916 way" (clocks under subnodes), adding
> some boilerplate to look up clocks/pds in both places and converting
> everybody to the "7180 way" way of doing things (clocks under venus),
> and then getting rid of venus encoder/decoder completely (by calling
> device creation from venus probe) would be better. WDYT?
>
> Konrad
As I understand it though, for some classes of venus hardware - earlier,
it was possible to have two encoders or two decoders and it really
didn't - perhaps still doesn't matter which order they are declared in.
That's the logic behind having a compat string that assigns either
encoder or decoder to one of the logical blocks.
You can have any mixture of
- encoder
- decoder
- encoder
- encoder
- decoder
- decoder
- decoder
- encoder
- encoder
- decoder
I think it should *still* be the case - whether it is a practical
reality or not, that any of those mapping can be selected and supported.
---
bod
On 7.08.2023 20:49, Bryan O'Donoghue wrote:
> On 07/08/2023 19:45, Konrad Dybcio wrote:
>> That can be taken care of with match data.
>>
>> Konrad
>
> Well perhaps.
>
> I'm just sticking my oar in, to elucidate.
>
> The compat sub-nodes aren't just a random choice with no logic. They exist to select between what you assign the blocks to be, encoder, decoder or any admixture thereof.
>
> A functionality we want to maintain.
Surely something like a modparam would be more suitable here?
Konrad
On 07/08/2023 19:55, Konrad Dybcio wrote:
> On 7.08.2023 20:49, Bryan O'Donoghue wrote:
>> On 07/08/2023 19:45, Konrad Dybcio wrote:
>>> That can be taken care of with match data.
>>>
>>> Konrad
>>
>> Well perhaps.
>>
>> I'm just sticking my oar in, to elucidate.
>>
>> The compat sub-nodes aren't just a random choice with no logic. They exist to select between what you assign the blocks to be, encoder, decoder or any admixture thereof.
>>
>> A functionality we want to maintain.
> Surely something like a modparam would be more suitable here?
>
> Konrad
Hmm.
Well from earlier in the thread the question "why do we have these
compat strings" is because we can have any combination of
encoder/decoder assigned.
If there's a cogent argument _still_ to be made to transition to some
new way of assignment then fine so long as we don't break that basic
flexibility.
Though my own €0.02 is that a module parameter is more of a PITA than a
compat string.
OTOH I could make the argument, that the high probability is most people
- probably all, just instantiate a single encoder and decoder and aren't
aware of or using the inbuilt flexibility.
@stan probably has the right idea what to do.
---
bod
On 07/08/2023 19:45, Konrad Dybcio wrote:
> That can be taken care of with match data.
>
> Konrad
Well perhaps.
I'm just sticking my oar in, to elucidate.
The compat sub-nodes aren't just a random choice with no logic. They
exist to select between what you assign the blocks to be, encoder,
decoder or any admixture thereof.
A functionality we want to maintain.
---
bod
On 7.08.2023 21:05, Bryan O'Donoghue wrote:
> On 07/08/2023 19:55, Konrad Dybcio wrote:
>> On 7.08.2023 20:49, Bryan O'Donoghue wrote:
>>> On 07/08/2023 19:45, Konrad Dybcio wrote:
>>>> That can be taken care of with match data.
>>>>
>>>> Konrad
>>>
>>> Well perhaps.
>>>
>>> I'm just sticking my oar in, to elucidate.
>>>
>>> The compat sub-nodes aren't just a random choice with no logic. They exist to select between what you assign the blocks to be, encoder, decoder or any admixture thereof.
>>>
>>> A functionality we want to maintain.
>> Surely something like a modparam would be more suitable here?
>>
>> Konrad
>
> Hmm.
>
> Well from earlier in the thread the question "why do we have these compat strings" is because we can have any combination of encoder/decoder assigned.
>
> If there's a cogent argument _still_ to be made to transition to some new way of assignment then fine so long as we don't break that basic flexibility.
>
> Though my own €0.02 is that a module parameter is more of a PITA than a compat string.
>
> OTOH I could make the argument, that the high probability is most people - probably all, just instantiate a single encoder and decoder and aren't aware of or using the inbuilt flexibility.
>
> @stan probably has the right idea what to do.
Actually..
Has anybody tested this, ever, with the mainline driver?
Do we have anyone using this?
Is anybody willing to maintain that, test for regressions and
fix them in a reasonable amount of time?
If we don't have at least 2x "yes" here, I don't think it makes sense
to worry about it..
Konrad
On 09/08/2023 13:15, Konrad Dybcio wrote:
>> Hmm.
>>
>> Well from earlier in the thread the question "why do we have these compat strings" is because we can have any combination of encoder/decoder assigned.
>>
>> If there's a cogent argument_still_ to be made to transition to some new way of assignment then fine so long as we don't break that basic flexibility.
>>
>> Though my own €0.02 is that a module parameter is more of a PITA than a compat string.
>>
>> OTOH I could make the argument, that the high probability is most people - probably all, just instantiate a single encoder and decoder and aren't aware of or using the inbuilt flexibility.
>>
>> @stan probably has the right idea what to do.
> Actually..
>
> Has anybody tested this, ever, with the mainline driver?
I assume Stan has.
> Do we have anyone using this?
Can't say.
> Is anybody willing to maintain that, test for regressions and
> fix them in a reasonable amount of time?
>
>
> If we don't have at least 2x "yes" here, I don't think it makes sense
> to worry about it..
Hmm.
We decide if we are encoding or decoding when we init a session and the
blocks are symmetrical. The hw blocks themselves are not bound to a
particular encode/decode mode.
Having two parallel encoders or decoders is exactly the same effort as
having a parallel encoder/decoder.
We don't test parallel encoding/decoding but we should. I'd not be
surprised to find there are bugs but, that's not a reason to exclude
rather to find and fix bugs.
---
bod
On 04/08/2023 21:09, Konrad Dybcio wrote:
> Add SC8280XP configuration data and related compatible.
>
> Signed-off-by: Konrad Dybcio <[email protected]>
> ---
> drivers/media/platform/qcom/venus/core.c | 45 ++++++++++++++++++++++++++++++++
> 1 file changed, 45 insertions(+)
>
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 5f285ae75e9d..32591b624a36 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
Reviewing this series, I think my input here has not been helpful or
correct.
1. Declaring encoders/decoders in dts or yaml is wrong, accepted.
2. We can make a platform choice to hard-code that here in the
platform declarations.
3. Remove the requirement from yaml for sc8280xp to declare decoder
encoder
3. Profit.
Existing dtb all, literally all do the same thing first block decoder,
second block encoder.
Rather than perform extensive surgery to venus to remediate the original
yaml sin - hard-code decoder/encoder into platform code and deprecate
the legacy over time.
Yes that means fixing to block 0 as decoder and block 1 as encoder but
that is the defacto situation we have now, we may as well make it dejure.
---
bod