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

[PATCH v2 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 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   | 11 +++++++++--
 drivers/media/platform/qcom/venus/vdec.c       | 10 +++++++++-
 5 files changed, 41 insertions(+), 3 deletions(-)

-- 
2.7.4

[PATCH v2 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: 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

[PATCH v2 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: 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/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 v2 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: 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>
---
 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 v2 1/3] venus: add firmware version based check

[Konrad Dybcio]

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

[Stanimir Varbanov]

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 3/3] venus: fix EOS handling in decoder stop command

[Dmitry Baryshkov]

On 07/04/2023 09:25, Dikshita Agarwal wrote:
> 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: Dikshita Agarwal <quic_dikshita@quicinc.com>
> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com>

Order of sign-offs is incorrect. The SoB of the sender should be the 
last one.

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

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On 07/04/2023 09:25, 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: Dikshita Agarwal <quic_dikshita@quicinc.com>
> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com>

Order of sign-offs is incorrect. The SoB of the sender should be the 
last one.

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

-- 
With best wishes
Dmitry

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

[Konrad Dybcio]

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

[Konrad Dybcio]

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

[Konrad Dybcio]

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

[Dikshita Agarwal]

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");

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

[Dikshita Agarwal]

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");
>