| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2fd0d1a7-70ee-43ac-af84-d2321c40e8f8@linaro.org>
Date: Thu, 17 Jul 2025 00:37:33 +0100
From: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To: Jorge Ramirez-Ortiz <jorge.ramirez@....qualcomm.com>,
quic_vgarodia@...cinc.com, quic_dikshita@...cinc.com, krzk+dt@...nel.org,
konradybcio@...nel.org, mchehab@...nel.org, andersson@...nel.org,
conor+dt@...nel.org, amit.kucheria@....qualcomm.com
Cc: linux-media@...r.kernel.org, linux-arm-msm@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 2/7] media: venus: Conditionally register codec nodes
based on firmware version
On 15/07/2025 21:47, Jorge Ramirez-Ortiz wrote:
> The encoding and decoding capabilities of a VPU can vary depending on the
> firmware version in use.
>
> This commit adds support for platforms with OF_DYNAMIC enabled to
> conditionally skip the creation of codec device nodes at runtime if the
> loaded firmware does not support the corresponding functionality.
>
> Note that the driver becomes aware of the firmware version only after the
> HFI layer has been initialized.
>
> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez@....qualcomm.com>
> ---
> drivers/media/platform/qcom/venus/core.c | 76 +++++++++++++++---------
> drivers/media/platform/qcom/venus/core.h | 8 +++
> 2 files changed, 57 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 4c049c694d9c..b7d6745b6124 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -28,6 +28,15 @@
> #include "pm_helpers.h"
> #include "hfi_venus_io.h"
>
> +static inline bool venus_fw_supports_codec(struct venus_core *core,
> + const struct venus_min_fw *ver)
> +{
> + if (!ver)
> + return true;
> +
> + return is_fw_rev_or_newer(core, ver->major, ver->minor, ver->rev);
> +}
> +
> static void venus_coredump(struct venus_core *core)
> {
> struct device *dev;
> @@ -103,7 +112,9 @@ static void venus_sys_error_handler(struct work_struct *work)
> core->state = CORE_UNINIT;
>
> for (i = 0; i < max_attempts; i++) {
> - if (!pm_runtime_active(core->dev_dec) && !pm_runtime_active(core->dev_enc))
> + /* Not both nodes might be available */
"Neither node available" the latter for preference.
> + if ((!core->dev_dec || !pm_runtime_active(core->dev_dec)) &&
> + (!core->dev_enc || !pm_runtime_active(core->dev_enc)))
Is this change about registration or is it a fix trying to sneak in
under the radar ?
> break;
> msleep(10);
> }
> @@ -202,7 +213,8 @@ static u32 to_v4l2_codec_type(u32 codec)
> }
> }
>
> -static int venus_enumerate_codecs(struct venus_core *core, u32 type)
> +static int venus_enumerate_codecs(struct venus_core *core, u32 type,
> + const struct venus_min_fw *ver)
> {
> const struct hfi_inst_ops dummy_ops = {};
> struct venus_inst *inst;
> @@ -213,6 +225,9 @@ static int venus_enumerate_codecs(struct venus_core *core, u32 type)
> if (core->res->hfi_version != HFI_VERSION_1XX)
> return 0;
>
> + if (!venus_fw_supports_codec(core, ver))
> + return 0;
Its not really a codec you're checking there, its a version.
The name should reflect that.
> +
> inst = kzalloc(sizeof(*inst), GFP_KERNEL);
> if (!inst)
> return -ENOMEM;
> @@ -288,14 +303,14 @@ static irqreturn_t venus_isr_thread(int irq, void *dev_id)
>
> #if defined(CONFIG_OF_DYNAMIC)
> static int venus_add_video_core(struct venus_core *core, const char *node_name,
> - const char *compat)
> + const char *compat, const struct venus_min_fw *ver)
> {
> struct of_changeset *ocs = core->ocs;
> struct device *dev = core->dev;
> struct device_node *np, *enp;
> int ret;
>
> - if (!node_name)
> + if (!node_name || !venus_fw_supports_codec(core, ver))
> return 0;
>
> enp = of_find_node_by_name(dev->of_node, node_name);
> @@ -330,11 +345,13 @@ static int venus_add_dynamic_nodes(struct venus_core *core)
>
> of_changeset_init(core->ocs);
>
> - ret = venus_add_video_core(core, core->res->dec_nodename, "venus-decoder");
> + ret = venus_add_video_core(core, core->res->dec_nodename, "venus-decoder",
> + core->res->dec_minfw);
> if (ret)
> goto err;
>
> - ret = venus_add_video_core(core, core->res->enc_nodename, "venus-encoder");
> + ret = venus_add_video_core(core, core->res->enc_nodename, "venus-encoder",
> + core->res->enc_minfw);
> if (ret)
> goto err;
>
> @@ -363,6 +380,9 @@ static void venus_remove_dynamic_nodes(struct venus_core *core)
> #else
> static int venus_add_dynamic_nodes(struct venus_core *core)
> {
> + WARN_ONCE(core->res->enc_minfw || core->res->dec_minfw,
> + "Feature not supported");
> +
> return 0;
> }
>
> @@ -432,7 +452,7 @@ static int venus_probe(struct platform_device *pdev)
> IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> "venus", core);
> if (ret)
> - goto err_core_put;
> + goto err_hfi_destroy;
>
> venus_assign_register_offsets(core);
>
> @@ -448,19 +468,9 @@ static int venus_probe(struct platform_device *pdev)
> if (ret < 0)
> goto err_runtime_disable;
>
> - if (core->res->dec_nodename || core->res->enc_nodename) {
> - ret = venus_add_dynamic_nodes(core);
> - if (ret)
> - goto err_runtime_disable;
> - }
> -
> - ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
> - if (ret)
> - goto err_remove_dynamic_nodes;
> -
> ret = venus_firmware_init(core);
> if (ret)
> - goto err_of_depopulate;
> + goto err_runtime_disable;
>
> ret = venus_boot(core);
> if (ret)
> @@ -474,34 +484,46 @@ static int venus_probe(struct platform_device *pdev)
> if (ret)
> goto err_venus_shutdown;
>
> - ret = venus_enumerate_codecs(core, VIDC_SESSION_TYPE_DEC);
> + if (core->res->dec_nodename || core->res->enc_nodename) {
> + ret = venus_add_dynamic_nodes(core);
> + if (ret)
> + goto err_core_deinit;
> + }
> +
> + ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
> if (ret)
> - goto err_core_deinit;
> + goto err_remove_dynamic_nodes;
> +
> + ret = venus_enumerate_codecs(core, VIDC_SESSION_TYPE_DEC,
> + core->res->dec_minfw);
> + if (ret)
> + goto err_of_depopulate;
>
> - ret = venus_enumerate_codecs(core, VIDC_SESSION_TYPE_ENC);
> + ret = venus_enumerate_codecs(core, VIDC_SESSION_TYPE_ENC,
> + core->res->enc_minfw);
> if (ret)
> - goto err_core_deinit;
> + goto err_of_depopulate;
>
> ret = pm_runtime_put_sync(dev);
> if (ret) {
> pm_runtime_get_noresume(dev);
> - goto err_core_deinit;
> + goto err_of_depopulate;
> }
>
> venus_dbgfs_init(core);
>
> return 0;
>
> +err_of_depopulate:
> + of_platform_depopulate(dev);
> +err_remove_dynamic_nodes:
> + venus_remove_dynamic_nodes(core);
> err_core_deinit:
> hfi_core_deinit(core, false);
> err_venus_shutdown:
> venus_shutdown(core);
> err_firmware_deinit:
> venus_firmware_deinit(core);
> -err_of_depopulate:
> - of_platform_depopulate(dev);
> -err_remove_dynamic_nodes:
> - venus_remove_dynamic_nodes(core);
> err_runtime_disable:
> pm_runtime_put_noidle(dev);
> pm_runtime_disable(dev);
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index 5b1ba1c69adb..3af8386b78be 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -50,6 +50,12 @@ struct bw_tbl {
> u32 peak_10bit;
> };
>
> +struct venus_min_fw {
> + u32 major;
> + u32 minor;
> + u32 rev;
> +};
I'd call this venus_firmware_version
> +
> enum vpu_version {
> VPU_VERSION_AR50,
> VPU_VERSION_AR50_LITE,
> @@ -92,6 +98,8 @@ struct venus_resources {
> u32 cp_nonpixel_start;
> u32 cp_nonpixel_size;
> const char *fwname;
> + const struct venus_min_fw *enc_minfw;
> + const struct venus_min_fw *dec_minfw;
and then I'd do as you have done here, indicate that the struct
venus_firmware_version is a *enc_min_fw_ver;
> const char *enc_nodename;
> const char *dec_nodename;
> };
Powered by blists - more mailing lists