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 */
+		if ((!core->dev_dec || !pm_runtime_active(core->dev_dec)) &&
+		    (!core->dev_enc || !pm_runtime_active(core->dev_enc)))
 			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;
+
 	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;
+};
+
 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;
 	const char *enc_nodename;
 	const char *dec_nodename;
 };
-- 
2.34.1

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;
>   };

On 17/07/25 00:37:33, Bryan O'Donoghue wrote:
> 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.

what about "One or both nodes may be unavailable" ?

> 
> > +		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 ?

I think this functionality - the ability to enable or disable individual
encode/decode nodes based on firmware capabilities - should be standard
across multimedia drivers.

For example, on the AR50_LITE platform, the _current_ driver/firmware
combo does not support encoding as it requires secure buffer handling
which is not yet implemented in the kernel (changes to iommu, etc)

So, rather than disabling Venus entirely, I think it makes sense to
expose the decoder node, which remains fully functional and unaffected
by the secure buffer requirement.

Hence this commit (so yeah, I am not trying to sneak a fix, I swear!) 

> 
> >   			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.

but the check isn't just about the firmware version: it is about whether
the firmware in use supports a specific coded based on the firmware
version knowledge built in the driver.

so really, while the logic involves a version check, it is really about
the codec capability.

I could rename it as is_codec_enabled_by_fw() if the current naming is
not clear.

> 
> > +
> >   	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

I guess you right- doing so will enable future extensibility if we need
to do some different version checks. ok.

> 
> > +
> >   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;

ack

> 
> >   	const char *enc_nodename;
> >   	const char *dec_nodename;
> >   };

On 17/07/2025 07:51, Jorge Ramirez wrote:
> On 17/07/25 00:37:33, Bryan O'Donoghue wrote:
>> 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.
> 
> what about "One or both nodes may be unavailable" ?

Ah great that actually explains it then, as you can see I didn't get the 
meaning from the comment.

>>
>>> +		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 ?
> 
> I think this functionality - the ability to enable or disable individual
> encode/decode nodes based on firmware capabilities - should be standard
> across multimedia drivers.
> 
> For example, on the AR50_LITE platform, the _current_ driver/firmware
> combo does not support encoding as it requires secure buffer handling
> which is not yet implemented in the kernel (changes to iommu, etc)
> 
> So, rather than disabling Venus entirely, I think it makes sense to
> expose the decoder node, which remains fully functional and unaffected
> by the secure buffer requirement.
> 
> Hence this commit (so yeah, I am not trying to sneak a fix, I swear!)

grand so.

> 
>>
>>>    			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.
> 
> but the check isn't just about the firmware version: it is about whether
> the firmware in use supports a specific coded based on the firmware
> version knowledge built in the driver.

No OK "codec" is the right word.

---
bod

On 17/07/25 09:55:08, Bryan O'Donoghue wrote:
> On 17/07/2025 07:51, Jorge Ramirez wrote:
> > On 17/07/25 00:37:33, Bryan O'Donoghue wrote:
> > > 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.
> > 
> > what about "One or both nodes may be unavailable" ?
> 
> Ah great that actually explains it then, as you can see I didn't get the
> meaning from the comment.
> 
> > > 
> > > > +		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 ?
> > 
> > I think this functionality - the ability to enable or disable individual
> > encode/decode nodes based on firmware capabilities - should be standard
> > across multimedia drivers.
> > 
> > For example, on the AR50_LITE platform, the _current_ driver/firmware
> > combo does not support encoding as it requires secure buffer handling
> > which is not yet implemented in the kernel (changes to iommu, etc)
> > 
> > So, rather than disabling Venus entirely, I think it makes sense to
> > expose the decoder node, which remains fully functional and unaffected
> > by the secure buffer requirement.
> > 
> > Hence this commit (so yeah, I am not trying to sneak a fix, I swear!)
> 
> grand so.
> 
> > 
> > > 
> > > >    			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.
> > 
> > but the check isn't just about the firmware version: it is about whether
> > the firmware in use supports a specific coded based on the firmware
> > version knowledge built in the driver.
> 
> No OK "codec" is the right word.
> 
> ---
> bod

as per internal discussion - offline - I am replacing this feature for a
simplified an "all or nothing" version: either the firmware version
can support both the encoder and the decoder or none of them.