Message-ID: <aHidibqhMyexExXJ@trex>
Date: Thu, 17 Jul 2025 08:51:53 +0200
From: Jorge Ramirez <jorge.ramirez@....qualcomm.com>
To: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
Cc: 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, 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 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@....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;
> >   };

Powered by blists - more mailing lists