[v2] fix decoder issues with firmware version check

[PATCH v2 1/3] venus: add firmware version based check

Posted by Dikshita Agarwal 2 years, 10 months ago

Add firmware version based checks to enable/disable
features for different SOCs.

Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com>
Tested-by: Nathan Hebert <nhebert@chromium.org>
---
 drivers/media/platform/qcom/venus/core.h     | 20 ++++++++++++++++++++
 drivers/media/platform/qcom/venus/hfi_msgs.c | 11 +++++++++--
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 32551c2..9d1e4b2 100644
--- a/drivers/media/platform/qcom/venus/core.h
+++ b/drivers/media/platform/qcom/venus/core.h
@@ -202,6 +202,11 @@ struct venus_core {
 	unsigned int core0_usage_count;
 	unsigned int core1_usage_count;
 	struct dentry *root;
+	struct venus_img_version {
+		u32 major;
+		u32 minor;
+		u32 rev;
+	} venus_ver;
 };
 
 struct vdec_controls {
@@ -500,4 +505,19 @@ venus_caps_by_codec(struct venus_core *core, u32 codec, u32 domain)
 	return NULL;
 }
 
+static inline int
+is_fw_rev_or_newer(struct venus_core *core, u32 vmajor, u32 vminor, u32 vrev)
+{
+	return ((core)->venus_ver.major == vmajor &&
+		(core)->venus_ver.minor == vminor &&
+		(core)->venus_ver.rev >= vrev);
+}
+
+static inline int
+is_fw_rev_or_older(struct venus_core *core, u32 vmajor, u32 vminor, u32 vrev)
+{
+	return ((core)->venus_ver.major == vmajor &&
+		(core)->venus_ver.minor == vminor &&
+		(core)->venus_ver.rev <= vrev);
+}
 #endif
diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c b/drivers/media/platform/qcom/venus/hfi_msgs.c
index df96db3..07ac0fc 100644
--- a/drivers/media/platform/qcom/venus/hfi_msgs.c
+++ b/drivers/media/platform/qcom/venus/hfi_msgs.c
@@ -248,9 +248,10 @@ static void hfi_sys_init_done(struct venus_core *core, struct venus_inst *inst,
 }
 
 static void
-sys_get_prop_image_version(struct device *dev,
+sys_get_prop_image_version(struct venus_core *core,
 			   struct hfi_msg_sys_property_info_pkt *pkt)
 {
+	struct device *dev = core->dev;
 	u8 *smem_tbl_ptr;
 	u8 *img_ver;
 	int req_bytes;
@@ -263,6 +264,12 @@ sys_get_prop_image_version(struct device *dev,
 		return;
 
 	img_ver = pkt->data;
+	if (IS_V4(core))
+		sscanf(img_ver, "14:VIDEO.VE.%u.%u-%u-PROD",
+		       &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev);
+	else if (IS_V6(core))
+		sscanf(img_ver, "14:VIDEO.VPU.%u.%u-%u-PROD",
+		       &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev);
 
 	dev_dbg(dev, VDBGL "F/W version: %s\n", img_ver);
 
@@ -286,7 +293,7 @@ static void hfi_sys_property_info(struct venus_core *core,
 
 	switch (pkt->property) {
 	case HFI_PROPERTY_SYS_IMAGE_VERSION:
-		sys_get_prop_image_version(dev, pkt);
+		sys_get_prop_image_version(core, pkt);
 		break;
 	default:
 		dev_dbg(dev, VDBGL "unknown property data\n");
-- 
2.7.4

Re: [PATCH v2 1/3] venus: add firmware version based check

Posted by Stanimir Varbanov 2 years, 10 months ago

Hi Dikshita,

Thanks for the patch.

On 7.04.23 г. 9:25 ч., Dikshita Agarwal wrote:
> Add firmware version based checks to enable/disable
> features for different SOCs.
> 
> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com>
> Tested-by: Nathan Hebert <nhebert@chromium.org>
> ---
>   drivers/media/platform/qcom/venus/core.h     | 20 ++++++++++++++++++++
>   drivers/media/platform/qcom/venus/hfi_msgs.c | 11 +++++++++--
>   2 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index 32551c2..9d1e4b2 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -202,6 +202,11 @@ struct venus_core {
>   	unsigned int core0_usage_count;
>   	unsigned int core1_usage_count;
>   	struct dentry *root;
> +	struct venus_img_version {
> +		u32 major;
> +		u32 minor;
> +		u32 rev;
> +	} venus_ver;
>   };
>   
>   struct vdec_controls {
> @@ -500,4 +505,19 @@ venus_caps_by_codec(struct venus_core *core, u32 codec, u32 domain)
>   	return NULL;
>   }
>   
> +static inline int
> +is_fw_rev_or_newer(struct venus_core *core, u32 vmajor, u32 vminor, u32 vrev)
> +{
> +	return ((core)->venus_ver.major == vmajor &&
> +		(core)->venus_ver.minor == vminor &&
> +		(core)->venus_ver.rev >= vrev);
> +}
> +
> +static inline int
> +is_fw_rev_or_older(struct venus_core *core, u32 vmajor, u32 vminor, u32 vrev)
> +{
> +	return ((core)->venus_ver.major == vmajor &&
> +		(core)->venus_ver.minor == vminor &&
> +		(core)->venus_ver.rev <= vrev);
> +}

IMO those two should return bool

>   #endif
> diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c b/drivers/media/platform/qcom/venus/hfi_msgs.c
> index df96db3..07ac0fc 100644
> --- a/drivers/media/platform/qcom/venus/hfi_msgs.c
> +++ b/drivers/media/platform/qcom/venus/hfi_msgs.c
> @@ -248,9 +248,10 @@ static void hfi_sys_init_done(struct venus_core *core, struct venus_inst *inst,
>   }
>   
>   static void
> -sys_get_prop_image_version(struct device *dev,
> +sys_get_prop_image_version(struct venus_core *core,
>   			   struct hfi_msg_sys_property_info_pkt *pkt)
>   {
> +	struct device *dev = core->dev;
>   	u8 *smem_tbl_ptr;
>   	u8 *img_ver;
>   	int req_bytes;
> @@ -263,6 +264,12 @@ sys_get_prop_image_version(struct device *dev,
>   		return;
>   
>   	img_ver = pkt->data;
> +	if (IS_V4(core))
> +		sscanf(img_ver, "14:VIDEO.VE.%u.%u-%u-PROD",
> +		       &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev);
> +	else if (IS_V6(core))
> +		sscanf(img_ver, "14:VIDEO.VPU.%u.%u-%u-PROD",
> +		       &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev);
>   

what about if IS_V1?

>   	dev_dbg(dev, VDBGL "F/W version: %s\n", img_ver);

this will crash for v1.

>   
> @@ -286,7 +293,7 @@ static void hfi_sys_property_info(struct venus_core *core,
>   
>   	switch (pkt->property) {
>   	case HFI_PROPERTY_SYS_IMAGE_VERSION:
> -		sys_get_prop_image_version(dev, pkt);
> +		sys_get_prop_image_version(core, pkt);
>   		break;
>   	default:
>   		dev_dbg(dev, VDBGL "unknown property data\n");

-- 
regards,
Stan

Re: [PATCH v2 1/3] venus: add firmware version based check

Posted by Dikshita Agarwal 2 years, 9 months ago

On 4/9/2023 10:48 AM, Stanimir Varbanov wrote:
> Hi Dikshita,
>
> Thanks for the patch.
>
> On 7.04.23 г. 9:25 ч., Dikshita Agarwal wrote:
>> Add firmware version based checks to enable/disable
>> features for different SOCs.
>>
>> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
>> Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com>
>> Tested-by: Nathan Hebert <nhebert@chromium.org>
>> ---
>>   drivers/media/platform/qcom/venus/core.h     | 20 ++++++++++++++++++++
>>   drivers/media/platform/qcom/venus/hfi_msgs.c | 11 +++++++++--
>>   2 files changed, 29 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/core.h 
>> b/drivers/media/platform/qcom/venus/core.h
>> index 32551c2..9d1e4b2 100644
>> --- a/drivers/media/platform/qcom/venus/core.h
>> +++ b/drivers/media/platform/qcom/venus/core.h
>> @@ -202,6 +202,11 @@ struct venus_core {
>>       unsigned int core0_usage_count;
>>       unsigned int core1_usage_count;
>>       struct dentry *root;
>> +    struct venus_img_version {
>> +        u32 major;
>> +        u32 minor;
>> +        u32 rev;
>> +    } venus_ver;
>>   };
>>     struct vdec_controls {
>> @@ -500,4 +505,19 @@ venus_caps_by_codec(struct venus_core *core, u32 
>> codec, u32 domain)
>>       return NULL;
>>   }
>>   +static inline int
>> +is_fw_rev_or_newer(struct venus_core *core, u32 vmajor, u32 vminor, 
>> u32 vrev)
>> +{
>> +    return ((core)->venus_ver.major == vmajor &&
>> +        (core)->venus_ver.minor == vminor &&
>> +        (core)->venus_ver.rev >= vrev);
>> +}
>> +
>> +static inline int
>> +is_fw_rev_or_older(struct venus_core *core, u32 vmajor, u32 vminor, 
>> u32 vrev)
>> +{
>> +    return ((core)->venus_ver.major == vmajor &&
>> +        (core)->venus_ver.minor == vminor &&
>> +        (core)->venus_ver.rev <= vrev);
>> +}
>
> IMO those two should return bool
sure, will update.
>
>>   #endif
>> diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c 
>> b/drivers/media/platform/qcom/venus/hfi_msgs.c
>> index df96db3..07ac0fc 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_msgs.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_msgs.c
>> @@ -248,9 +248,10 @@ static void hfi_sys_init_done(struct venus_core 
>> *core, struct venus_inst *inst,
>>   }
>>     static void
>> -sys_get_prop_image_version(struct device *dev,
>> +sys_get_prop_image_version(struct venus_core *core,
>>                  struct hfi_msg_sys_property_info_pkt *pkt)
>>   {
>> +    struct device *dev = core->dev;
>>       u8 *smem_tbl_ptr;
>>       u8 *img_ver;
>>       int req_bytes;
>> @@ -263,6 +264,12 @@ sys_get_prop_image_version(struct device *dev,
>>           return;
>>         img_ver = pkt->data;
>> +    if (IS_V4(core))
>> +        sscanf(img_ver, "14:VIDEO.VE.%u.%u-%u-PROD",
>> +               &core->venus_ver.major, &core->venus_ver.minor, 
>> &core->venus_ver.rev);
>> +    else if (IS_V6(core))
>> +        sscanf(img_ver, "14:VIDEO.VPU.%u.%u-%u-PROD",
>> +               &core->venus_ver.major, &core->venus_ver.minor, 
>> &core->venus_ver.rev);
>
> what about if IS_V1?

the conditional code above is added only for v6 and v4.

The behavior for v1 remain same as before.

img_ver = pkt->data

>
>>       dev_dbg(dev, VDBGL "F/W version: %s\n", img_ver);
>
> this will crash for v1.
I didn't get why would it crash, could you pls explain?
>
>>   @@ -286,7 +293,7 @@ static void hfi_sys_property_info(struct 
>> venus_core *core,
>>         switch (pkt->property) {
>>       case HFI_PROPERTY_SYS_IMAGE_VERSION:
>> -        sys_get_prop_image_version(dev, pkt);
>> +        sys_get_prop_image_version(core, pkt);
>>           break;
>>       default:
>>           dev_dbg(dev, VDBGL "unknown property data\n");
>

Re: [PATCH v2 1/3] venus: add firmware version based check

Posted by Konrad Dybcio 2 years, 10 months ago


On 9.04.2023 07:18, Stanimir Varbanov wrote:
> Hi Dikshita,
> 
> Thanks for the patch.
> 
> On 7.04.23 г. 9:25 ч., Dikshita Agarwal wrote:
>> Add firmware version based checks to enable/disable
>> features for different SOCs.
>>
>> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
>> Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com>
>> Tested-by: Nathan Hebert <nhebert@chromium.org>
>> ---
>>   drivers/media/platform/qcom/venus/core.h     | 20 ++++++++++++++++++++
>>   drivers/media/platform/qcom/venus/hfi_msgs.c | 11 +++++++++--
>>   2 files changed, 29 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
>> index 32551c2..9d1e4b2 100644
>> --- a/drivers/media/platform/qcom/venus/core.h
>> +++ b/drivers/media/platform/qcom/venus/core.h
>> @@ -202,6 +202,11 @@ struct venus_core {
>>       unsigned int core0_usage_count;
>>       unsigned int core1_usage_count;
>>       struct dentry *root;
>> +    struct venus_img_version {
>> +        u32 major;
>> +        u32 minor;
>> +        u32 rev;
>> +    } venus_ver;
>>   };
>>     struct vdec_controls {
>> @@ -500,4 +505,19 @@ venus_caps_by_codec(struct venus_core *core, u32 codec, u32 domain)
>>       return NULL;
>>   }
>>   +static inline int
>> +is_fw_rev_or_newer(struct venus_core *core, u32 vmajor, u32 vminor, u32 vrev)
>> +{
>> +    return ((core)->venus_ver.major == vmajor &&
>> +        (core)->venus_ver.minor == vminor &&
>> +        (core)->venus_ver.rev >= vrev);
>> +}
>> +
>> +static inline int
>> +is_fw_rev_or_older(struct venus_core *core, u32 vmajor, u32 vminor, u32 vrev)
>> +{
>> +    return ((core)->venus_ver.major == vmajor &&
>> +        (core)->venus_ver.minor == vminor &&
>> +        (core)->venus_ver.rev <= vrev);
>> +}
> 
> IMO those two should return bool
> 
>>   #endif
>> diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c b/drivers/media/platform/qcom/venus/hfi_msgs.c
>> index df96db3..07ac0fc 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_msgs.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_msgs.c
>> @@ -248,9 +248,10 @@ static void hfi_sys_init_done(struct venus_core *core, struct venus_inst *inst,
>>   }
>>     static void
>> -sys_get_prop_image_version(struct device *dev,
>> +sys_get_prop_image_version(struct venus_core *core,
>>                  struct hfi_msg_sys_property_info_pkt *pkt)
>>   {
>> +    struct device *dev = core->dev;
>>       u8 *smem_tbl_ptr;
>>       u8 *img_ver;
>>       int req_bytes;
>> @@ -263,6 +264,12 @@ sys_get_prop_image_version(struct device *dev,
>>           return;
>>         img_ver = pkt->data;
>> +    if (IS_V4(core))
>> +        sscanf(img_ver, "14:VIDEO.VE.%u.%u-%u-PROD",
>> +               &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev);
>> +    else if (IS_V6(core))
>> +        sscanf(img_ver, "14:VIDEO.VPU.%u.%u-%u-PROD",
>> +               &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev);
>>   
> 
> what about if IS_V1?
Whooops, I missed that in my review as well...

Looks like the 8916 and 8996 FWs fall under the VIDEO.VE case
as well, that's the QC_VERSION_STRING they have..

Perhaps this could be an 

if (IS_V6)
	..
else
	..

Konrad
> 
>>       dev_dbg(dev, VDBGL "F/W version: %s\n", img_ver);
> 
> this will crash for v1.
> 
>>   @@ -286,7 +293,7 @@ static void hfi_sys_property_info(struct venus_core *core,
>>         switch (pkt->property) {
>>       case HFI_PROPERTY_SYS_IMAGE_VERSION:
>> -        sys_get_prop_image_version(dev, pkt);
>> +        sys_get_prop_image_version(core, pkt);
>>           break;
>>       default:
>>           dev_dbg(dev, VDBGL "unknown property data\n");
>

Re: [PATCH v2 1/3] venus: add firmware version based check

Posted by Konrad Dybcio 2 years, 10 months ago


On 11.04.2023 12:59, Konrad Dybcio wrote:
> 
> 
> On 9.04.2023 07:18, Stanimir Varbanov wrote:
>> Hi Dikshita,
>>
>> Thanks for the patch.
>>
>> On 7.04.23 г. 9:25 ч., Dikshita Agarwal wrote:
>>> Add firmware version based checks to enable/disable
>>> features for different SOCs.
>>>
>>> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
>>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
>>> Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com>
>>> Tested-by: Nathan Hebert <nhebert@chromium.org>
>>> ---
>>>   drivers/media/platform/qcom/venus/core.h     | 20 ++++++++++++++++++++
>>>   drivers/media/platform/qcom/venus/hfi_msgs.c | 11 +++++++++--
>>>   2 files changed, 29 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
>>> index 32551c2..9d1e4b2 100644
>>> --- a/drivers/media/platform/qcom/venus/core.h
>>> +++ b/drivers/media/platform/qcom/venus/core.h
>>> @@ -202,6 +202,11 @@ struct venus_core {
>>>       unsigned int core0_usage_count;
>>>       unsigned int core1_usage_count;
>>>       struct dentry *root;
>>> +    struct venus_img_version {
>>> +        u32 major;
>>> +        u32 minor;
>>> +        u32 rev;
>>> +    } venus_ver;
>>>   };
>>>     struct vdec_controls {
>>> @@ -500,4 +505,19 @@ venus_caps_by_codec(struct venus_core *core, u32 codec, u32 domain)
>>>       return NULL;
>>>   }
>>>   +static inline int
>>> +is_fw_rev_or_newer(struct venus_core *core, u32 vmajor, u32 vminor, u32 vrev)
>>> +{
>>> +    return ((core)->venus_ver.major == vmajor &&
>>> +        (core)->venus_ver.minor == vminor &&
>>> +        (core)->venus_ver.rev >= vrev);
>>> +}
>>> +
>>> +static inline int
>>> +is_fw_rev_or_older(struct venus_core *core, u32 vmajor, u32 vminor, u32 vrev)
>>> +{
>>> +    return ((core)->venus_ver.major == vmajor &&
>>> +        (core)->venus_ver.minor == vminor &&
>>> +        (core)->venus_ver.rev <= vrev);
>>> +}
>>
>> IMO those two should return bool
>>
>>>   #endif
>>> diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c b/drivers/media/platform/qcom/venus/hfi_msgs.c
>>> index df96db3..07ac0fc 100644
>>> --- a/drivers/media/platform/qcom/venus/hfi_msgs.c
>>> +++ b/drivers/media/platform/qcom/venus/hfi_msgs.c
>>> @@ -248,9 +248,10 @@ static void hfi_sys_init_done(struct venus_core *core, struct venus_inst *inst,
>>>   }
>>>     static void
>>> -sys_get_prop_image_version(struct device *dev,
>>> +sys_get_prop_image_version(struct venus_core *core,
>>>                  struct hfi_msg_sys_property_info_pkt *pkt)
>>>   {
>>> +    struct device *dev = core->dev;
>>>       u8 *smem_tbl_ptr;
>>>       u8 *img_ver;
>>>       int req_bytes;
>>> @@ -263,6 +264,12 @@ sys_get_prop_image_version(struct device *dev,
>>>           return;
>>>         img_ver = pkt->data;
>>> +    if (IS_V4(core))
>>> +        sscanf(img_ver, "14:VIDEO.VE.%u.%u-%u-PROD",
>>> +               &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev);
>>> +    else if (IS_V6(core))
>>> +        sscanf(img_ver, "14:VIDEO.VPU.%u.%u-%u-PROD",
>>> +               &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev);
>>>   
>>
>> what about if IS_V1?
> Whooops, I missed that in my review as well...
> 
> Looks like the 8916 and 8996 FWs fall under the VIDEO.VE case
> as well, that's the QC_VERSION_STRING they have..
On top of that, my 8350 fw reports:

F/W version: 14:video-firmware.1.0-3fb5add1d3ac96f8f74facd537845a6ceb5a99e4

Konrad
> 
> Perhaps this could be an 
> 
> if (IS_V6)
> 	..
> else
> 	..
> 
> Konrad
>>
>>>       dev_dbg(dev, VDBGL "F/W version: %s\n", img_ver);
>>
>> this will crash for v1.
>>
>>>   @@ -286,7 +293,7 @@ static void hfi_sys_property_info(struct venus_core *core,
>>>         switch (pkt->property) {
>>>       case HFI_PROPERTY_SYS_IMAGE_VERSION:
>>> -        sys_get_prop_image_version(dev, pkt);
>>> +        sys_get_prop_image_version(core, pkt);
>>>           break;
>>>       default:
>>>           dev_dbg(dev, VDBGL "unknown property data\n");
>>

Re: [PATCH v2 1/3] venus: add firmware version based check

Posted by Konrad Dybcio 2 years, 10 months ago


On 14.04.2023 01:01, Konrad Dybcio wrote:
> 
> 
> On 11.04.2023 12:59, Konrad Dybcio wrote:
>>
>>
>> On 9.04.2023 07:18, Stanimir Varbanov wrote:
>>> Hi Dikshita,
>>>
>>> Thanks for the patch.
>>>
>>> On 7.04.23 г. 9:25 ч., Dikshita Agarwal wrote:
>>>> Add firmware version based checks to enable/disable
>>>> features for different SOCs.
>>>>
>>>> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
>>>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
>>>> Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com>
>>>> Tested-by: Nathan Hebert <nhebert@chromium.org>
>>>> ---
>>>>   drivers/media/platform/qcom/venus/core.h     | 20 ++++++++++++++++++++
>>>>   drivers/media/platform/qcom/venus/hfi_msgs.c | 11 +++++++++--
>>>>   2 files changed, 29 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
>>>> index 32551c2..9d1e4b2 100644
>>>> --- a/drivers/media/platform/qcom/venus/core.h
>>>> +++ b/drivers/media/platform/qcom/venus/core.h
>>>> @@ -202,6 +202,11 @@ struct venus_core {
>>>>       unsigned int core0_usage_count;
>>>>       unsigned int core1_usage_count;
>>>>       struct dentry *root;
>>>> +    struct venus_img_version {
>>>> +        u32 major;
>>>> +        u32 minor;
>>>> +        u32 rev;
>>>> +    } venus_ver;
>>>>   };
>>>>     struct vdec_controls {
>>>> @@ -500,4 +505,19 @@ venus_caps_by_codec(struct venus_core *core, u32 codec, u32 domain)
>>>>       return NULL;
>>>>   }
>>>>   +static inline int
>>>> +is_fw_rev_or_newer(struct venus_core *core, u32 vmajor, u32 vminor, u32 vrev)
>>>> +{
>>>> +    return ((core)->venus_ver.major == vmajor &&
>>>> +        (core)->venus_ver.minor == vminor &&
>>>> +        (core)->venus_ver.rev >= vrev);
>>>> +}
>>>> +
>>>> +static inline int
>>>> +is_fw_rev_or_older(struct venus_core *core, u32 vmajor, u32 vminor, u32 vrev)
>>>> +{
>>>> +    return ((core)->venus_ver.major == vmajor &&
>>>> +        (core)->venus_ver.minor == vminor &&
>>>> +        (core)->venus_ver.rev <= vrev);
>>>> +}
>>>
>>> IMO those two should return bool
>>>
>>>>   #endif
>>>> diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c b/drivers/media/platform/qcom/venus/hfi_msgs.c
>>>> index df96db3..07ac0fc 100644
>>>> --- a/drivers/media/platform/qcom/venus/hfi_msgs.c
>>>> +++ b/drivers/media/platform/qcom/venus/hfi_msgs.c
>>>> @@ -248,9 +248,10 @@ static void hfi_sys_init_done(struct venus_core *core, struct venus_inst *inst,
>>>>   }
>>>>     static void
>>>> -sys_get_prop_image_version(struct device *dev,
>>>> +sys_get_prop_image_version(struct venus_core *core,
>>>>                  struct hfi_msg_sys_property_info_pkt *pkt)
>>>>   {
>>>> +    struct device *dev = core->dev;
>>>>       u8 *smem_tbl_ptr;
>>>>       u8 *img_ver;
>>>>       int req_bytes;
>>>> @@ -263,6 +264,12 @@ sys_get_prop_image_version(struct device *dev,
>>>>           return;
>>>>         img_ver = pkt->data;
>>>> +    if (IS_V4(core))
>>>> +        sscanf(img_ver, "14:VIDEO.VE.%u.%u-%u-PROD",
>>>> +               &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev);
>>>> +    else if (IS_V6(core))
>>>> +        sscanf(img_ver, "14:VIDEO.VPU.%u.%u-%u-PROD",
>>>> +               &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev);
>>>>   
>>>
>>> what about if IS_V1?
>> Whooops, I missed that in my review as well...
>>
>> Looks like the 8916 and 8996 FWs fall under the VIDEO.VE case
>> as well, that's the QC_VERSION_STRING they have..
> On top of that, my 8350 fw reports:
> 
> F/W version: 14:video-firmware.1.0-3fb5add1d3ac96f8f74facd537845a6ceb5a99e4
FWIW this cryptic version also needs fdata.device_addr = 0

(for reference - failling to do so will never stop the video
stream polling)

Konrad
> 
> Konrad
>>
>> Perhaps this could be an 
>>
>> if (IS_V6)
>> 	..
>> else
>> 	..
>>
>> Konrad
>>>
>>>>       dev_dbg(dev, VDBGL "F/W version: %s\n", img_ver);
>>>
>>> this will crash for v1.
>>>
>>>>   @@ -286,7 +293,7 @@ static void hfi_sys_property_info(struct venus_core *core,
>>>>         switch (pkt->property) {
>>>>       case HFI_PROPERTY_SYS_IMAGE_VERSION:
>>>> -        sys_get_prop_image_version(dev, pkt);
>>>> +        sys_get_prop_image_version(core, pkt);
>>>>           break;
>>>>       default:
>>>>           dev_dbg(dev, VDBGL "unknown property data\n");
>>>

Re: [PATCH v2 1/3] venus: add firmware version based check

Posted by Konrad Dybcio 2 years, 10 months ago


On 7.04.2023 08:25, Dikshita Agarwal wrote:
> Add firmware version based checks to enable/disable
> features for different SOCs.
> 
> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com>
> Tested-by: Nathan Hebert <nhebert@chromium.org>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

One extra question: some firmware builds have a TYPE-n suffix like
PROD-1 in:

14:VIDEO.VE.6.0-00042-PROD-1

Is the -1 a sign of an incremental build, or some "point release" of a
given fw revision? Does it matter as far as this checking function
goes?

Konrad
>  drivers/media/platform/qcom/venus/core.h     | 20 ++++++++++++++++++++
>  drivers/media/platform/qcom/venus/hfi_msgs.c | 11 +++++++++--
>  2 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index 32551c2..9d1e4b2 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -202,6 +202,11 @@ struct venus_core {
>  	unsigned int core0_usage_count;
>  	unsigned int core1_usage_count;
>  	struct dentry *root;
> +	struct venus_img_version {
> +		u32 major;
> +		u32 minor;
> +		u32 rev;
> +	} venus_ver;
>  };
>  
>  struct vdec_controls {
> @@ -500,4 +505,19 @@ venus_caps_by_codec(struct venus_core *core, u32 codec, u32 domain)
>  	return NULL;
>  }
>  
> +static inline int
> +is_fw_rev_or_newer(struct venus_core *core, u32 vmajor, u32 vminor, u32 vrev)
> +{
> +	return ((core)->venus_ver.major == vmajor &&
> +		(core)->venus_ver.minor == vminor &&
> +		(core)->venus_ver.rev >= vrev);
> +}
> +
> +static inline int
> +is_fw_rev_or_older(struct venus_core *core, u32 vmajor, u32 vminor, u32 vrev)
> +{
> +	return ((core)->venus_ver.major == vmajor &&
> +		(core)->venus_ver.minor == vminor &&
> +		(core)->venus_ver.rev <= vrev);
> +}
>  #endif
> diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c b/drivers/media/platform/qcom/venus/hfi_msgs.c
> index df96db3..07ac0fc 100644
> --- a/drivers/media/platform/qcom/venus/hfi_msgs.c
> +++ b/drivers/media/platform/qcom/venus/hfi_msgs.c
> @@ -248,9 +248,10 @@ static void hfi_sys_init_done(struct venus_core *core, struct venus_inst *inst,
>  }
>  
>  static void
> -sys_get_prop_image_version(struct device *dev,
> +sys_get_prop_image_version(struct venus_core *core,
>  			   struct hfi_msg_sys_property_info_pkt *pkt)
>  {
> +	struct device *dev = core->dev;
>  	u8 *smem_tbl_ptr;
>  	u8 *img_ver;
>  	int req_bytes;
> @@ -263,6 +264,12 @@ sys_get_prop_image_version(struct device *dev,
>  		return;
>  
>  	img_ver = pkt->data;
> +	if (IS_V4(core))
> +		sscanf(img_ver, "14:VIDEO.VE.%u.%u-%u-PROD",
> +		       &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev);
> +	else if (IS_V6(core))
> +		sscanf(img_ver, "14:VIDEO.VPU.%u.%u-%u-PROD",
> +		       &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev);
>  
>  	dev_dbg(dev, VDBGL "F/W version: %s\n", img_ver);
>  
> @@ -286,7 +293,7 @@ static void hfi_sys_property_info(struct venus_core *core,
>  
>  	switch (pkt->property) {
>  	case HFI_PROPERTY_SYS_IMAGE_VERSION:
> -		sys_get_prop_image_version(dev, pkt);
> +		sys_get_prop_image_version(core, pkt);
>  		break;
>  	default:
>  		dev_dbg(dev, VDBGL "unknown property data\n");

Re: [PATCH v2 1/3] venus: add firmware version based check

Posted by Dikshita Agarwal 2 years, 9 months ago

On 4/8/2023 12:45 PM, Konrad Dybcio wrote:
>
> On 7.04.2023 08:25, Dikshita Agarwal wrote:
>> Add firmware version based checks to enable/disable
>> features for different SOCs.
>>
>> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
>> Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com>
>> Tested-by: Nathan Hebert <nhebert@chromium.org>
>> ---
> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>
> One extra question: some firmware builds have a TYPE-n suffix like
> PROD-1 in:
>
> 14:VIDEO.VE.6.0-00042-PROD-1
>
> Is the -1 a sign of an incremental build, or some "point release" of a
> given fw revision? Does it matter as far as this checking function
> goes?

this part of string gets added based on the tool version used to build 
the firmware.

this doesn't matter and can be ignored.

Thanks,

Dikshita

> Konrad
>>   drivers/media/platform/qcom/venus/core.h     | 20 ++++++++++++++++++++
>>   drivers/media/platform/qcom/venus/hfi_msgs.c | 11 +++++++++--
>>   2 files changed, 29 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
>> index 32551c2..9d1e4b2 100644
>> --- a/drivers/media/platform/qcom/venus/core.h
>> +++ b/drivers/media/platform/qcom/venus/core.h
>> @@ -202,6 +202,11 @@ struct venus_core {
>>   	unsigned int core0_usage_count;
>>   	unsigned int core1_usage_count;
>>   	struct dentry *root;
>> +	struct venus_img_version {
>> +		u32 major;
>> +		u32 minor;
>> +		u32 rev;
>> +	} venus_ver;
>>   };
>>   
>>   struct vdec_controls {
>> @@ -500,4 +505,19 @@ venus_caps_by_codec(struct venus_core *core, u32 codec, u32 domain)
>>   	return NULL;
>>   }
>>   
>> +static inline int
>> +is_fw_rev_or_newer(struct venus_core *core, u32 vmajor, u32 vminor, u32 vrev)
>> +{
>> +	return ((core)->venus_ver.major == vmajor &&
>> +		(core)->venus_ver.minor == vminor &&
>> +		(core)->venus_ver.rev >= vrev);
>> +}
>> +
>> +static inline int
>> +is_fw_rev_or_older(struct venus_core *core, u32 vmajor, u32 vminor, u32 vrev)
>> +{
>> +	return ((core)->venus_ver.major == vmajor &&
>> +		(core)->venus_ver.minor == vminor &&
>> +		(core)->venus_ver.rev <= vrev);
>> +}
>>   #endif
>> diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c b/drivers/media/platform/qcom/venus/hfi_msgs.c
>> index df96db3..07ac0fc 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_msgs.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_msgs.c
>> @@ -248,9 +248,10 @@ static void hfi_sys_init_done(struct venus_core *core, struct venus_inst *inst,
>>   }
>>   
>>   static void
>> -sys_get_prop_image_version(struct device *dev,
>> +sys_get_prop_image_version(struct venus_core *core,
>>   			   struct hfi_msg_sys_property_info_pkt *pkt)
>>   {
>> +	struct device *dev = core->dev;
>>   	u8 *smem_tbl_ptr;
>>   	u8 *img_ver;
>>   	int req_bytes;
>> @@ -263,6 +264,12 @@ sys_get_prop_image_version(struct device *dev,
>>   		return;
>>   
>>   	img_ver = pkt->data;
>> +	if (IS_V4(core))
>> +		sscanf(img_ver, "14:VIDEO.VE.%u.%u-%u-PROD",
>> +		       &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev);
>> +	else if (IS_V6(core))
>> +		sscanf(img_ver, "14:VIDEO.VPU.%u.%u-%u-PROD",
>> +		       &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev);
>>   
>>   	dev_dbg(dev, VDBGL "F/W version: %s\n", img_ver);
>>   
>> @@ -286,7 +293,7 @@ static void hfi_sys_property_info(struct venus_core *core,
>>   
>>   	switch (pkt->property) {
>>   	case HFI_PROPERTY_SYS_IMAGE_VERSION:
>> -		sys_get_prop_image_version(dev, pkt);
>> +		sys_get_prop_image_version(core, pkt);
>>   		break;
>>   	default:
>>   		dev_dbg(dev, VDBGL "unknown property data\n");

[PATCH v2 1/3] venus: add firmware version based check
[PATCH v2 2/3] venus: enable sufficient sequence change support for vp9
[PATCH v2 3/3] venus: fix EOS handling in decoder stop command