[PATCH v3 0/3] fix decoder issues with firmware version check

[PATCH v3 0/3] fix decoder issues with firmware version check

[Dikshita Agarwal]

This series includes the changes to
  - add firmware version based check to enable/disable some feature.
  - add support of new HFI to notify sequence change event to
    driver during resolution change at interframe.
  - use firmware version based check to fix EOS handling for different
    firmware versions.

With this series, divided the previous version [1] into
multiple patches as suggested in review comments.

[1] https://patchwork.kernel.org/project/linux-media/list/?series=733169

change since v2:
  added firmware version based check for all supported firmwares.
  added return value check of scanf. 
  addressed other review comments.

change since v1:
  addressed coding comments.

Dikshita Agarwal (3):
  venus: add firmware version based check
  venus: enable sufficient sequence change support for vp9
  venus: fix EOS handling in decoder stop command

 drivers/media/platform/qcom/venus/core.h       | 20 +++++++++++++++++++
 drivers/media/platform/qcom/venus/hfi_cmds.c   |  1 +
 drivers/media/platform/qcom/venus/hfi_helper.h |  2 ++
 drivers/media/platform/qcom/venus/hfi_msgs.c   | 27 ++++++++++++++++++++++++--
 drivers/media/platform/qcom/venus/vdec.c       | 10 +++++++++-
 5 files changed, 57 insertions(+), 3 deletions(-)

-- 
2.7.4

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

[Dikshita Agarwal]

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

Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com>
Signed-off-by: Dikshita Agarwal <quic_dikshita@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 | 27 +++++++++++++++++++++++++--
 2 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 32551c2..2f2176f 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 bool
+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 bool
+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..4854863 100644
--- a/drivers/media/platform/qcom/venus/hfi_msgs.c
+++ b/drivers/media/platform/qcom/venus/hfi_msgs.c
@@ -248,13 +248,16 @@ 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;
 	size_t smem_blk_sz;
+	int ret;
+	u8 *ver_str;
 
 	req_bytes = pkt->hdr.size - sizeof(*pkt);
 
@@ -263,6 +266,26 @@ sys_get_prop_image_version(struct device *dev,
 		return;
 
 	img_ver = pkt->data;
+	if (IS_V6(core) && core->res->num_vpp_pipes == 1) {
+		ret = sscanf(img_ver, "14:video-firmware.%u.%u-%u",
+			     &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev);
+		if (ret != 2) {
+			dev_dbg(dev, VDBGL "error reading F/W version\n");
+			return;
+		}
+	} else {
+		if (IS_V6(core))
+			ver_str = "14:VIDEO.VPU.%u.%u-%u";
+		else
+			ver_str = "14:VIDEO.VE.%u.%u-%u";
+
+		ret = sscanf(img_ver, "14:VIDEO.VE.%u.%u-%u",
+			     &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev);
+		if (ret != 3) {
+			dev_dbg(dev, VDBGL "error reading F/W version\n");
+			return;
+		}
+	}
 
 	dev_dbg(dev, VDBGL "F/W version: %s\n", img_ver);
 
@@ -286,7 +309,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

[PATCH v3 2/3] venus: enable sufficient sequence change support for vp9

[Dikshita Agarwal]

VP9 supports resolution change at interframe.
Currenlty, if sequence change is detected at interframe and
resources are sufficient, sequence change event is not raised
by firmware to driver until the next keyframe.
This change add the HFI to notify the sequence change in this
case to driver.

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

diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.c b/drivers/media/platform/qcom/venus/hfi_cmds.c
index 930b743..e2539b5 100644
--- a/drivers/media/platform/qcom/venus/hfi_cmds.c
+++ b/drivers/media/platform/qcom/venus/hfi_cmds.c
@@ -521,6 +521,7 @@ static int pkt_session_set_property_1x(struct hfi_session_set_property_pkt *pkt,
 		pkt->shdr.hdr.size += sizeof(u32) + sizeof(*en);
 		break;
 	}
+	case HFI_PROPERTY_PARAM_VDEC_ENABLE_SUFFICIENT_SEQCHANGE_EVENT:
 	case HFI_PROPERTY_CONFIG_VDEC_POST_LOOP_DEBLOCKER: {
 		struct hfi_enable *in = pdata;
 		struct hfi_enable *en = prop_data;
diff --git a/drivers/media/platform/qcom/venus/hfi_helper.h b/drivers/media/platform/qcom/venus/hfi_helper.h
index d2d6719..2e03b6e 100644
--- a/drivers/media/platform/qcom/venus/hfi_helper.h
+++ b/drivers/media/platform/qcom/venus/hfi_helper.h
@@ -469,6 +469,8 @@
 #define HFI_PROPERTY_PARAM_VDEC_PIXEL_BITDEPTH			0x1003007
 #define HFI_PROPERTY_PARAM_VDEC_PIC_STRUCT			0x1003009
 #define HFI_PROPERTY_PARAM_VDEC_COLOUR_SPACE			0x100300a
+#define HFI_PROPERTY_PARAM_VDEC_ENABLE_SUFFICIENT_SEQCHANGE_EVENT \
+								0x100300b
 
 /*
  * HFI_PROPERTY_CONFIG_VDEC_COMMON_START
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 4ceaba3..f0394b9 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -671,6 +671,14 @@ static int vdec_set_properties(struct venus_inst *inst)
 			return ret;
 	}
 
+	/* Enabling sufficient sequence change support for VP9 */
+	if (is_fw_rev_or_newer(inst->core, 5, 4, 51)) {
+		ptype = HFI_PROPERTY_PARAM_VDEC_ENABLE_SUFFICIENT_SEQCHANGE_EVENT;
+		ret = hfi_session_set_property(inst, ptype, &en);
+		if (ret)
+			return ret;
+	}
+
 	ptype = HFI_PROPERTY_PARAM_VDEC_CONCEAL_COLOR;
 	conceal = ctr->conceal_color & 0xffff;
 	conceal |= ((ctr->conceal_color >> 16) & 0xffff) << 10;
-- 
2.7.4

[PATCH v3 3/3] venus: fix EOS handling in decoder stop command

[Dikshita Agarwal]

Use firmware version based check to assign correct
device address for EOS buffer to fix the EOS handling
with different firmware version.

Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com>
Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
Tested-by: Nathan Hebert <nhebert@chromium.org>
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/venus/vdec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index f0394b9..c59b34f 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -545,7 +545,7 @@ vdec_decoder_cmd(struct file *file, void *fh, struct v4l2_decoder_cmd *cmd)
 
 		fdata.buffer_type = HFI_BUFFER_INPUT;
 		fdata.flags |= HFI_BUFFERFLAG_EOS;
-		if (IS_V6(inst->core))
+		if (IS_V6(inst->core) && is_fw_rev_or_older(inst->core, 1, 0, 87))
 			fdata.device_addr = 0;
 		else
 			fdata.device_addr = 0xdeadb000;
-- 
2.7.4

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

[Konrad Dybcio]

On 4.05.2023 11:39, Dikshita Agarwal wrote:
> Add firmware version based checks to enable/disable
> features for different SOCs.
> 
> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com>
> Signed-off-by: Dikshita Agarwal <quic_dikshita@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 | 27 +++++++++++++++++++++++++--
>  2 files changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index 32551c2..2f2176f 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 bool
> +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 bool
> +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..4854863 100644
> --- a/drivers/media/platform/qcom/venus/hfi_msgs.c
> +++ b/drivers/media/platform/qcom/venus/hfi_msgs.c
> @@ -248,13 +248,16 @@ 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;
>  	size_t smem_blk_sz;
> +	int ret;
> +	u8 *ver_str;
>  
>  	req_bytes = pkt->hdr.size - sizeof(*pkt);
>  
> @@ -263,6 +266,26 @@ sys_get_prop_image_version(struct device *dev,
>  		return;
>  
>  	img_ver = pkt->data;
> +	if (IS_V6(core) && core->res->num_vpp_pipes == 1) {
> +		ret = sscanf(img_ver, "14:video-firmware.%u.%u-%u",
> +			     &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev);
This is still not perfect, 8350 has 4 vpp pipes and its firmware is
also denominated with "video-firmware".. perhaps we can just try
each variant until we reach ret == 3?

> +		if (ret != 2) {
3?

Konrad
> +			dev_dbg(dev, VDBGL "error reading F/W version\n");
> +			return;
> +		}
> +	} else {
> +		if (IS_V6(core))
> +			ver_str = "14:VIDEO.VPU.%u.%u-%u";
> +		else
> +			ver_str = "14:VIDEO.VE.%u.%u-%u";
> +
> +		ret = sscanf(img_ver, "14:VIDEO.VE.%u.%u-%u",
> +			     &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev);
> +		if (ret != 3) {
> +			dev_dbg(dev, VDBGL "error reading F/W version\n");
> +			return;
> +		}
> +	}
>  
>  	dev_dbg(dev, VDBGL "F/W version: %s\n", img_ver);
>  
> @@ -286,7 +309,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 v3 2/3] venus: enable sufficient sequence change support for vp9

[Konrad Dybcio]

On 4.05.2023 11:39, Dikshita Agarwal wrote:
> VP9 supports resolution change at interframe.
> Currenlty, if sequence change is detected at interframe and
> resources are sufficient, sequence change event is not raised
> by firmware to driver until the next keyframe.
> This change add the HFI to notify the sequence change in this
> case to driver.
> 
> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com>
> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
> Tested-by: Nathan Hebert <nhebert@chromium.org>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
>  drivers/media/platform/qcom/venus/hfi_cmds.c   | 1 +
>  drivers/media/platform/qcom/venus/hfi_helper.h | 2 ++
>  drivers/media/platform/qcom/venus/vdec.c       | 8 ++++++++
>  3 files changed, 11 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.c b/drivers/media/platform/qcom/venus/hfi_cmds.c
> index 930b743..e2539b5 100644
> --- a/drivers/media/platform/qcom/venus/hfi_cmds.c
> +++ b/drivers/media/platform/qcom/venus/hfi_cmds.c
> @@ -521,6 +521,7 @@ static int pkt_session_set_property_1x(struct hfi_session_set_property_pkt *pkt,
>  		pkt->shdr.hdr.size += sizeof(u32) + sizeof(*en);
>  		break;
>  	}
> +	case HFI_PROPERTY_PARAM_VDEC_ENABLE_SUFFICIENT_SEQCHANGE_EVENT:
>  	case HFI_PROPERTY_CONFIG_VDEC_POST_LOOP_DEBLOCKER: {
>  		struct hfi_enable *in = pdata;
>  		struct hfi_enable *en = prop_data;
> diff --git a/drivers/media/platform/qcom/venus/hfi_helper.h b/drivers/media/platform/qcom/venus/hfi_helper.h
> index d2d6719..2e03b6e 100644
> --- a/drivers/media/platform/qcom/venus/hfi_helper.h
> +++ b/drivers/media/platform/qcom/venus/hfi_helper.h
> @@ -469,6 +469,8 @@
>  #define HFI_PROPERTY_PARAM_VDEC_PIXEL_BITDEPTH			0x1003007
>  #define HFI_PROPERTY_PARAM_VDEC_PIC_STRUCT			0x1003009
>  #define HFI_PROPERTY_PARAM_VDEC_COLOUR_SPACE			0x100300a
> +#define HFI_PROPERTY_PARAM_VDEC_ENABLE_SUFFICIENT_SEQCHANGE_EVENT \
> +								0x100300b
>  
>  /*
>   * HFI_PROPERTY_CONFIG_VDEC_COMMON_START
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index 4ceaba3..f0394b9 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -671,6 +671,14 @@ static int vdec_set_properties(struct venus_inst *inst)
>  			return ret;
>  	}
>  
> +	/* Enabling sufficient sequence change support for VP9 */
> +	if (is_fw_rev_or_newer(inst->core, 5, 4, 51)) {
> +		ptype = HFI_PROPERTY_PARAM_VDEC_ENABLE_SUFFICIENT_SEQCHANGE_EVENT;
> +		ret = hfi_session_set_property(inst, ptype, &en);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	ptype = HFI_PROPERTY_PARAM_VDEC_CONCEAL_COLOR;
>  	conceal = ctr->conceal_color & 0xffff;
>  	conceal |= ((ctr->conceal_color >> 16) & 0xffff) << 10;

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

[Dikshita Agarwal]

On 5/4/2023 3:12 PM, Konrad Dybcio wrote:
>
> On 4.05.2023 11:39, Dikshita Agarwal wrote:
>> Add firmware version based checks to enable/disable
>> features for different SOCs.
>>
>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
>> Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com>
>> Signed-off-by: Dikshita Agarwal <quic_dikshita@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 | 27 +++++++++++++++++++++++++--
>>   2 files changed, 45 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
>> index 32551c2..2f2176f 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 bool
>> +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 bool
>> +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..4854863 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_msgs.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_msgs.c
>> @@ -248,13 +248,16 @@ 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;
>>   	size_t smem_blk_sz;
>> +	int ret;
>> +	u8 *ver_str;
>>   
>>   	req_bytes = pkt->hdr.size - sizeof(*pkt);
>>   
>> @@ -263,6 +266,26 @@ sys_get_prop_image_version(struct device *dev,
>>   		return;
>>   
>>   	img_ver = pkt->data;
>> +	if (IS_V6(core) && core->res->num_vpp_pipes == 1) {
>> +		ret = sscanf(img_ver, "14:video-firmware.%u.%u-%u",
>> +			     &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev);
> This is still not perfect, 8350 has 4 vpp pipes and its firmware is
> also denominated with "video-firmware".. perhaps we can just try
> each variant until we reach ret == 3?

sc7280 onward firmware have image string as "video-firmware".

Support for 8350 is not yet added in venus driver, any required change 
for the same can be done

when support will be added for the same.

>> +		if (ret != 2) {
> 3?

this image version string doesn't return valid revision hence checking 
against 2 (major and minor versions)

Thanks,

Dikshita

>
> Konrad
>> +			dev_dbg(dev, VDBGL "error reading F/W version\n");
>> +			return;
>> +		}
>> +	} else {
>> +		if (IS_V6(core))
>> +			ver_str = "14:VIDEO.VPU.%u.%u-%u";
>> +		else
>> +			ver_str = "14:VIDEO.VE.%u.%u-%u";
>> +
>> +		ret = sscanf(img_ver, "14:VIDEO.VE.%u.%u-%u",
>> +			     &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev);
>> +		if (ret != 3) {
>> +			dev_dbg(dev, VDBGL "error reading F/W version\n");
>> +			return;
>> +		}
>> +	}
>>   
>>   	dev_dbg(dev, VDBGL "F/W version: %s\n", img_ver);
>>   
>> @@ -286,7 +309,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 v3 1/3] venus: add firmware version based check

[Konrad Dybcio]

On 4.05.2023 11:49, Dikshita Agarwal wrote:
> 
> On 5/4/2023 3:12 PM, Konrad Dybcio wrote:
>>
>> On 4.05.2023 11:39, Dikshita Agarwal wrote:
>>> Add firmware version based checks to enable/disable
>>> features for different SOCs.
>>>
>>> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
>>> Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com>
>>> Signed-off-by: Dikshita Agarwal <quic_dikshita@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 | 27 +++++++++++++++++++++++++--
>>>   2 files changed, 45 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
>>> index 32551c2..2f2176f 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 bool
>>> +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 bool
>>> +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..4854863 100644
>>> --- a/drivers/media/platform/qcom/venus/hfi_msgs.c
>>> +++ b/drivers/media/platform/qcom/venus/hfi_msgs.c
>>> @@ -248,13 +248,16 @@ 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;
>>>       size_t smem_blk_sz;
>>> +    int ret;
>>> +    u8 *ver_str;
>>>         req_bytes = pkt->hdr.size - sizeof(*pkt);
>>>   @@ -263,6 +266,26 @@ sys_get_prop_image_version(struct device *dev,
>>>           return;
>>>         img_ver = pkt->data;
>>> +    if (IS_V6(core) && core->res->num_vpp_pipes == 1) {
>>> +        ret = sscanf(img_ver, "14:video-firmware.%u.%u-%u",
>>> +                 &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev);
>> This is still not perfect, 8350 has 4 vpp pipes and its firmware is
>> also denominated with "video-firmware".. perhaps we can just try
>> each variant until we reach ret == 3?
> 
> sc7280 onward firmware have image string as "video-firmware".
> 
> Support for 8350 is not yet added in venus driver, any required change for the same can be done
> 
> when support will be added for the same.
I understand, but that doesn't mean this can't be improved:

ret = sscanf(.. video-firmware .. )
if (ret == 2) {
	/* video-firmware string doesn't provide rev info */
	ver.rev = (something);
	goto done;
}

ret = sscanf(.. VIDEO.VE .. )
if (ret == 3)
	goto done;

ret = sscanf(.. VIDEO.VPU .. )
if (ret != 3) {
	/* We ran out of options! */
	return -EINVAL;
}

done:
	// continue the code flow


> 
>>> +        if (ret != 2) {
>> 3?
> 
> this image version string doesn't return valid revision hence checking against 2 (major and minor versions)
So why are you filling the revision field with an invalid value?

let's take a version string from a firmware I have on hand:

14:video-firmware.1.0-3fb5add1d3ac96f8f74facd537845a6ceb5a99e4

this will evaluate to:

maj = 1
min = 0
rev = 3

since it's incorrect, drop the last argument and initialize it with
something like UINT_MAX or 0xdeadbeef

On a note, you left ver_str unused in this patch, so it doesn't work
for VIDEO.VPU anyway

Konrad
> 
> Thanks,
> 
> Dikshita
> 
>>
>> Konrad
>>> +            dev_dbg(dev, VDBGL "error reading F/W version\n");
>>> +            return;
>>> +        }
>>> +    } else {
>>> +        if (IS_V6(core))
>>> +            ver_str = "14:VIDEO.VPU.%u.%u-%u";
>>> +        else
>>> +            ver_str = "14:VIDEO.VE.%u.%u-%u";
>>> +
>>> +        ret = sscanf(img_ver, "14:VIDEO.VE.%u.%u-%u",
>>> +                 &core->venus_ver.major, &core->venus_ver.minor, &core->venus_ver.rev);
>>> +        if (ret != 3) {
>>> +            dev_dbg(dev, VDBGL "error reading F/W version\n");
>>> +            return;
>>> +        }
>>> +    }
>>>         dev_dbg(dev, VDBGL "F/W version: %s\n", img_ver);
>>>   @@ -286,7 +309,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");