Re: [PATCH v7 2/7] media: venus: Conditionally register codec nodes based on firmware version

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

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@oss.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;
>   };