[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <0dd1f38e-eaba-4c99-b386-4ff8fade5b9b@linaro.org>
Date: Thu, 17 Jul 2025 09:55:08 +0100
From: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To: Jorge Ramirez <jorge.ramirez@....qualcomm.com>
Cc: 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/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@....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
Powered by blists - more mailing lists