[PATCH v4] venus: Enable sufficient sequence change support for sc7180 and fix for Decoder STOP command issue.

[PATCH v4] venus: Enable sufficient sequence change support for sc7180 and fix for Decoder STOP command issue.

[Viswanath Boma]

For VP9 bitstreams, there could be a change in resolution at interframe,
for driver to get notified of such resolution change,
enable the property in video firmware.
Also, EOS handling is now made same in video firmware across all V6 SOCs,
hence above a certain firmware version, the driver handling is
made generic for all V6s

Signed-off-by: Vikash Garodia <vgarodia@qti.qualcomm.com>
Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com>
Tested-by: Nathan Hebert <nhebert@chromium.org>
---
Since v3 : Addressed comments to rectify email address.

 drivers/media/platform/qcom/venus/core.h       | 18 ++++++++++++++++++
 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       | 12 +++++++++++-
 5 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
index 32551c2602a9..ee8b70a34656 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,17 @@ 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_cmds.c b/drivers/media/platform/qcom/venus/hfi_cmds.c
index bc3f8ff05840..9efe04961890 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 105792a68060..c8aaf870829c 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 \
+								0x0100300b
 
 /*
  * HFI_PROPERTY_CONFIG_VDEC_COMMON_START
diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c b/drivers/media/platform/qcom/venus/hfi_msgs.c
index df96db3761a7..07ac0fcd2852 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");
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 4ceaba37e2e5..36c88858ea9d 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;
@@ -671,6 +671,16 @@ static int vdec_set_properties(struct venus_inst *inst)
 			return ret;
 	}
 
+	/* Enabling sufficient sequence change support for VP9 */
+	if (of_device_is_compatible(inst->core->dev->of_node, "qcom,sc7180-venus")) {
+		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.17.1

Re: [PATCH v4] venus: Enable sufficient sequence change support for sc7180 and fix for Decoder STOP command issue.

[Dmitry Baryshkov]

On 23/03/2023 15:01, Viswanath Boma wrote:
> For VP9 bitstreams, there could be a change in resolution at interframe,
> for driver to get notified of such resolution change,
> enable the property in video firmware.
> Also, EOS handling is now made same in video firmware across all V6 SOCs,
> hence above a certain firmware version, the driver handling is
> made generic for all V6s

Having "Do abc. Also do defgh." is a clear sign that this patch should 
be split into two.

> 
> Signed-off-by: Vikash Garodia <vgarodia@qti.qualcomm.com>
> Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com>
> Tested-by: Nathan Hebert <nhebert@chromium.org>
> ---
> Since v3 : Addressed comments to rectify email address.
> 
>   drivers/media/platform/qcom/venus/core.h       | 18 ++++++++++++++++++
>   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       | 12 +++++++++++-
>   5 files changed, 41 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index 32551c2602a9..ee8b70a34656 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,17 @@ 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_cmds.c b/drivers/media/platform/qcom/venus/hfi_cmds.c
> index bc3f8ff05840..9efe04961890 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 105792a68060..c8aaf870829c 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 \
> +								0x0100300b
>   
>   /*
>    * HFI_PROPERTY_CONFIG_VDEC_COMMON_START
> diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c b/drivers/media/platform/qcom/venus/hfi_msgs.c
> index df96db3761a7..07ac0fcd2852 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");
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index 4ceaba37e2e5..36c88858ea9d 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))

This should go into a separate patch.

>   			fdata.device_addr = 0;
>   		else
>   			fdata.device_addr = 0xdeadb000;
> @@ -671,6 +671,16 @@ static int vdec_set_properties(struct venus_inst *inst)
>   			return ret;
>   	}
>   
> +	/* Enabling sufficient sequence change support for VP9 */
> +	if (of_device_is_compatible(inst->core->dev->of_node, "qcom,sc7180-venus")) {

Let me repeat my question from v3:

Is it really specific just to sc7180 or will it be applicable to any
other platform using venus-5.4 firmware?

> +		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 v4] venus: Enable sufficient sequence change support for sc7180 and fix for Decoder STOP command issue.

[Vikash Garodia]

On 3/29/2023 3:49 AM, Dmitry Baryshkov wrote:
> On 23/03/2023 15:01, Viswanath Boma wrote:
>> For VP9 bitstreams, there could be a change in resolution at interframe,
>> for driver to get notified of such resolution change,
>> enable the property in video firmware.
>> Also, EOS handling is now made same in video firmware across all V6 
>> SOCs,
>> hence above a certain firmware version, the driver handling is
>> made generic for all V6s
>
> Having "Do abc. Also do defgh." is a clear sign that this patch should 
> be split into two.

I agree, it could have split into patches. The patch introduces way to 
store venus firmware

version and take some decision for various version. For ex. here STOP 
handling and enabling

DRC event for specific firmware revision and onwards. Since both the 
handling was primarily

dependent of firmware version, and since the handlings were smaller, it 
was combined as single

patch. Let me know, if you have any further review comments, else, will 
raise a new version with

2 patches probably.

>>
>> Signed-off-by: Vikash Garodia <vgarodia@qti.qualcomm.com>
>> Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com>
>> Tested-by: Nathan Hebert <nhebert@chromium.org>
>> ---
>> Since v3 : Addressed comments to rectify email address.
>>
>>   drivers/media/platform/qcom/venus/core.h       | 18 ++++++++++++++++++
>>   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       | 12 +++++++++++-
>>   5 files changed, 41 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/core.h 
>> b/drivers/media/platform/qcom/venus/core.h
>> index 32551c2602a9..ee8b70a34656 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,17 @@ 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_cmds.c 
>> b/drivers/media/platform/qcom/venus/hfi_cmds.c
>> index bc3f8ff05840..9efe04961890 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 105792a68060..c8aaf870829c 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 \
>> +                                0x0100300b
>>     /*
>>    * HFI_PROPERTY_CONFIG_VDEC_COMMON_START
>> diff --git a/drivers/media/platform/qcom/venus/hfi_msgs.c 
>> b/drivers/media/platform/qcom/venus/hfi_msgs.c
>> index df96db3761a7..07ac0fcd2852 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");
>> diff --git a/drivers/media/platform/qcom/venus/vdec.c 
>> b/drivers/media/platform/qcom/venus/vdec.c
>> index 4ceaba37e2e5..36c88858ea9d 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))
>
> This should go into a separate patch.
Yes.
>
>>               fdata.device_addr = 0;
>>           else
>>               fdata.device_addr = 0xdeadb000;
>> @@ -671,6 +671,16 @@ static int vdec_set_properties(struct venus_inst 
>> *inst)
>>               return ret;
>>       }
>>   +    /* Enabling sufficient sequence change support for VP9 */
>> +    if (of_device_is_compatible(inst->core->dev->of_node, 
>> "qcom,sc7180-venus")) {
>
> Let me repeat my question from v3:
>
> Is it really specific just to sc7180 or will it be applicable to any
> other platform using venus-5.4 firmware?

The HFI "HFI_PROPERTY_PARAM_VDEC_ENABLE_SUFFICIENT_SEQCHANGE_EVENT" is 
implemented

only for sc7180. Calling this for any other venus-5.4 would error out 
the session with error as

unsupported property from firmware.

>
>> +        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 v4] venus: Enable sufficient sequence change support for sc7180 and fix for Decoder STOP command issue.

[Dmitry Baryshkov]

29 марта 2023 г. 10:48:23 GMT+03:00, Vikash Garodia <quic_vgarodia@quicinc.com> пишет:
>On 3/29/2023 3:49 AM, Dmitry Baryshkov wrote:
>> On 23/03/2023 15:01, Viswanath Boma wrote:
>>> For VP9 bitstreams, there could be a change in resolution at interframe,
>>> for driver to get notified of such resolution change,
>>> enable the property in video firmware.
>>> Also, EOS handling is now made same in video firmware across all V6 SOCs,
>>> hence above a certain firmware version, the driver handling is
>>> made generic for all V6s
>> 
>> Having "Do abc. Also do defgh." is a clear sign that this patch should be split into two.
>
>I agree, it could have split into patches. The patch introduces way to store venus firmware
>
>version and take some decision for various version. For ex. here STOP handling and enabling
>
>DRC event for specific firmware revision and onwards. Since both the handling was primarily
>
>dependent of firmware version, and since the handlings were smaller, it was combined as single
>
>patch. Let me know, if you have any further review comments, else, will raise a new version with
>
>2 patches probably.

Thanks!

>
>>> 
>>> Signed-off-by: Vikash Garodia <vgarodia@qti.qualcomm.com>
>>> Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com>
>>> Tested-by: Nathan Hebert <nhebert@chromium.org>
>>> ---
>>> Since v3 : Addressed comments to rectify email address.
>>> 
>>>   drivers/media/platform/qcom/venus/core.h       | 18 ++++++++++++++++++
>>>   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       | 12 +++++++++++-
>>>   5 files changed, 41 insertions(+), 3 deletions(-)
>>> 

(Skipped)



>>> @@ -671,6 +671,16 @@ static int vdec_set_properties(struct venus_inst *inst)
>>>               return ret;
>>>       }
>>>   +    /* Enabling sufficient sequence change support for VP9 */
>>> +    if (of_device_is_compatible(inst->core->dev->of_node, "qcom,sc7180-venus")) {
>> 
>> Let me repeat my question from v3:
>> 
>> Is it really specific just to sc7180 or will it be applicable to any
>> other platform using venus-5.4 firmware?
>
>The HFI "HFI_PROPERTY_PARAM_VDEC_ENABLE_SUFFICIENT_SEQCHANGE_EVENT" is implemented
>
>only for sc7180. Calling this for any other venus-5.4 would error out the session with error as
>
>unsupported property from firmware.


How can we be sure that other platforms do not end up using sc7180 firmware? Or that sc7180 didn't end up using some other firmware?

I see generic  qcom/venus-5.4/venus.mbn in Linux firmware. It's version is VIDEO.VE.5.4-00053-PROD-1. It can be used with any unfused device which uses firmware 5.4

>
>> 
>>> +        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 v4] venus: Enable sufficient sequence change support for sc7180 and fix for Decoder STOP command issue.

[Vikash Garodia]

On 3/29/2023 7:06 PM, Dmitry Baryshkov wrote:
> 29 марта 2023 г. 10:48:23 GMT+03:00, Vikash Garodia <quic_vgarodia@quicinc.com> пишет:
>> On 3/29/2023 3:49 AM, Dmitry Baryshkov wrote:
>>> On 23/03/2023 15:01, Viswanath Boma wrote:
>>>> For VP9 bitstreams, there could be a change in resolution at interframe,
>>>> for driver to get notified of such resolution change,
>>>> enable the property in video firmware.
>>>> Also, EOS handling is now made same in video firmware across all V6 SOCs,
>>>> hence above a certain firmware version, the driver handling is
>>>> made generic for all V6s
>>> Having "Do abc. Also do defgh." is a clear sign that this patch should be split into two.
>> I agree, it could have split into patches. The patch introduces way to store venus firmware
>>
>> version and take some decision for various version. For ex. here STOP handling and enabling
>>
>> DRC event for specific firmware revision and onwards. Since both the handling was primarily
>>
>> dependent of firmware version, and since the handlings were smaller, it was combined as single
>>
>> patch. Let me know, if you have any further review comments, else, will raise a new version with
>>
>> 2 patches probably.
> Thanks!
>
>>>> Signed-off-by: Vikash Garodia <vgarodia@qti.qualcomm.com>
>>>> Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com>
>>>> Tested-by: Nathan Hebert <nhebert@chromium.org>
>>>> ---
>>>> Since v3 : Addressed comments to rectify email address.
>>>>
>>>>    drivers/media/platform/qcom/venus/core.h       | 18 ++++++++++++++++++
>>>>    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       | 12 +++++++++++-
>>>>    5 files changed, 41 insertions(+), 3 deletions(-)
>>>>
> (Skipped)
>
>
>
>>>> @@ -671,6 +671,16 @@ static int vdec_set_properties(struct venus_inst *inst)
>>>>                return ret;
>>>>        }
>>>>    +    /* Enabling sufficient sequence change support for VP9 */
>>>> +    if (of_device_is_compatible(inst->core->dev->of_node, "qcom,sc7180-venus")) {
>>> Let me repeat my question from v3:
>>>
>>> Is it really specific just to sc7180 or will it be applicable to any
>>> other platform using venus-5.4 firmware?
>> The HFI "HFI_PROPERTY_PARAM_VDEC_ENABLE_SUFFICIENT_SEQCHANGE_EVENT" is implemented
>>
>> only for sc7180. Calling this for any other venus-5.4 would error out the session with error as
>>
>> unsupported property from firmware.
>
> How can we be sure that other platforms do not end up using sc7180 firmware? Or that sc7180 didn't end up using some other firmware?
>
> I see generic  qcom/venus-5.4/venus.mbn in Linux firmware. It's version is VIDEO.VE.5.4-00053-PROD-1. It can be used with any unfused device which uses firmware 5.4

Driver defines resources for every platforms and there it specifies the 
firmware to be used for that platform. For ex, for sc7180, the firmware 
is specified at [1].

The various firmware supported by different platforms are also available 
in linux firmware.

[1] 
https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/media/platform/qcom/venus/core.c#L765 


>>>> +        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 v4] venus: Enable sufficient sequence change support for sc7180 and fix for Decoder STOP command issue.

[Dmitry Baryshkov]

On Wed, 29 Mar 2023 at 20:16, Vikash Garodia <quic_vgarodia@quicinc.com> wrote:
>
>
> On 3/29/2023 7:06 PM, Dmitry Baryshkov wrote:
> > 29 марта 2023 г. 10:48:23 GMT+03:00, Vikash Garodia <quic_vgarodia@quicinc.com> пишет:
> >> On 3/29/2023 3:49 AM, Dmitry Baryshkov wrote:
> >>> On 23/03/2023 15:01, Viswanath Boma wrote:
> >>>> For VP9 bitstreams, there could be a change in resolution at interframe,
> >>>> for driver to get notified of such resolution change,
> >>>> enable the property in video firmware.
> >>>> Also, EOS handling is now made same in video firmware across all V6 SOCs,
> >>>> hence above a certain firmware version, the driver handling is
> >>>> made generic for all V6s
> >>> Having "Do abc. Also do defgh." is a clear sign that this patch should be split into two.
> >> I agree, it could have split into patches. The patch introduces way to store venus firmware
> >>
> >> version and take some decision for various version. For ex. here STOP handling and enabling
> >>
> >> DRC event for specific firmware revision and onwards. Since both the handling was primarily
> >>
> >> dependent of firmware version, and since the handlings were smaller, it was combined as single
> >>
> >> patch. Let me know, if you have any further review comments, else, will raise a new version with
> >>
> >> 2 patches probably.
> > Thanks!
> >
> >>>> Signed-off-by: Vikash Garodia <vgarodia@qti.qualcomm.com>
> >>>> Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com>
> >>>> Tested-by: Nathan Hebert <nhebert@chromium.org>
> >>>> ---
> >>>> Since v3 : Addressed comments to rectify email address.
> >>>>
> >>>>    drivers/media/platform/qcom/venus/core.h       | 18 ++++++++++++++++++
> >>>>    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       | 12 +++++++++++-
> >>>>    5 files changed, 41 insertions(+), 3 deletions(-)
> >>>>
> > (Skipped)
> >
> >
> >
> >>>> @@ -671,6 +671,16 @@ static int vdec_set_properties(struct venus_inst *inst)
> >>>>                return ret;
> >>>>        }
> >>>>    +    /* Enabling sufficient sequence change support for VP9 */
> >>>> +    if (of_device_is_compatible(inst->core->dev->of_node, "qcom,sc7180-venus")) {
> >>> Let me repeat my question from v3:
> >>>
> >>> Is it really specific just to sc7180 or will it be applicable to any
> >>> other platform using venus-5.4 firmware?
> >> The HFI "HFI_PROPERTY_PARAM_VDEC_ENABLE_SUFFICIENT_SEQCHANGE_EVENT" is implemented
> >>
> >> only for sc7180. Calling this for any other venus-5.4 would error out the session with error as
> >>
> >> unsupported property from firmware.
> >
> > How can we be sure that other platforms do not end up using sc7180 firmware? Or that sc7180 didn't end up using some other firmware?
> >
> > I see generic  qcom/venus-5.4/venus.mbn in Linux firmware. It's version is VIDEO.VE.5.4-00053-PROD-1. It can be used with any unfused device which uses firmware 5.4
>
> Driver defines resources for every platforms and there it specifies the
> firmware to be used for that platform. For ex, for sc7180, the firmware
> is specified at [1].

And note that the firmware doesn't have an SoC name in it. This file
will be used by all unfused devices that use 5.4 firmware family.

> The various firmware supported by different platforms are also available
> in linux firmware.
>
> [1]
> https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/media/platform/qcom/venus/core.c#L765

And in that file sc7180 is the only platform having firmware 5.4.

I think that the check for sc7180 is redundant. Just check that the
firmware is from 5.4 family and it is 5.4.51 or newer.

> >>>> +        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 v4] venus: Enable sufficient sequence change support for sc7180 and fix for Decoder STOP command issue.

[Vikash Garodia]

On 3/29/2023 11:03 PM, Dmitry Baryshkov wrote:
> On Wed, 29 Mar 2023 at 20:16, Vikash Garodia <quic_vgarodia@quicinc.com> wrote:
>>
>> On 3/29/2023 7:06 PM, Dmitry Baryshkov wrote:
>>> 29 марта 2023 г. 10:48:23 GMT+03:00, Vikash Garodia <quic_vgarodia@quicinc.com> пишет:
>>>> On 3/29/2023 3:49 AM, Dmitry Baryshkov wrote:
>>>>> On 23/03/2023 15:01, Viswanath Boma wrote:
>>>>>> For VP9 bitstreams, there could be a change in resolution at interframe,
>>>>>> for driver to get notified of such resolution change,
>>>>>> enable the property in video firmware.
>>>>>> Also, EOS handling is now made same in video firmware across all V6 SOCs,
>>>>>> hence above a certain firmware version, the driver handling is
>>>>>> made generic for all V6s
>>>>> Having "Do abc. Also do defgh." is a clear sign that this patch should be split into two.
>>>> I agree, it could have split into patches. The patch introduces way to store venus firmware
>>>>
>>>> version and take some decision for various version. For ex. here STOP handling and enabling
>>>>
>>>> DRC event for specific firmware revision and onwards. Since both the handling was primarily
>>>>
>>>> dependent of firmware version, and since the handlings were smaller, it was combined as single
>>>>
>>>> patch. Let me know, if you have any further review comments, else, will raise a new version with
>>>>
>>>> 2 patches probably.
>>> Thanks!
>>>
>>>>>> Signed-off-by: Vikash Garodia <vgarodia@qti.qualcomm.com>
>>>>>> Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com>
>>>>>> Tested-by: Nathan Hebert <nhebert@chromium.org>
>>>>>> ---
>>>>>> Since v3 : Addressed comments to rectify email address.
>>>>>>
>>>>>>     drivers/media/platform/qcom/venus/core.h       | 18 ++++++++++++++++++
>>>>>>     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       | 12 +++++++++++-
>>>>>>     5 files changed, 41 insertions(+), 3 deletions(-)
>>>>>>
>>> (Skipped)
>>>
>>>
>>>
>>>>>> @@ -671,6 +671,16 @@ static int vdec_set_properties(struct venus_inst *inst)
>>>>>>                 return ret;
>>>>>>         }
>>>>>>     +    /* Enabling sufficient sequence change support for VP9 */
>>>>>> +    if (of_device_is_compatible(inst->core->dev->of_node, "qcom,sc7180-venus")) {
>>>>> Let me repeat my question from v3:
>>>>>
>>>>> Is it really specific just to sc7180 or will it be applicable to any
>>>>> other platform using venus-5.4 firmware?
>>>> The HFI "HFI_PROPERTY_PARAM_VDEC_ENABLE_SUFFICIENT_SEQCHANGE_EVENT" is implemented
>>>>
>>>> only for sc7180. Calling this for any other venus-5.4 would error out the session with error as
>>>>
>>>> unsupported property from firmware.
>>> How can we be sure that other platforms do not end up using sc7180 firmware? Or that sc7180 didn't end up using some other firmware?
>>>
>>> I see generic  qcom/venus-5.4/venus.mbn in Linux firmware. It's version is VIDEO.VE.5.4-00053-PROD-1. It can be used with any unfused device which uses firmware 5.4
>> Driver defines resources for every platforms and there it specifies the
>> firmware to be used for that platform. For ex, for sc7180, the firmware
>> is specified at [1].
> And note that the firmware doesn't have an SoC name in it. This file
> will be used by all unfused devices that use 5.4 firmware family.
>
>> The various firmware supported by different platforms are also available
>> in linux firmware.
>>
>> [1]
>> https://elixir.bootlin.com/linux/v6.3-rc4/source/drivers/media/platform/qcom/venus/core.c#L765
> And in that file sc7180 is the only platform having firmware 5.4.
>
> I think that the check for sc7180 is redundant. Just check that the
> firmware is from 5.4 family and it is 5.4.51 or newer.
I agree. sc7180 check can be removed as the feature is applicable for 
all 5.4 firmwares, irrespective of platform.
>
>>>>>> +        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 v4] venus: Enable sufficient sequence change support for sc7180 and fix for Decoder STOP command issue.

[Dmitry Baryshkov]

On 23/03/2023 15:01, Viswanath Boma wrote:
> For VP9 bitstreams, there could be a change in resolution at interframe,
> for driver to get notified of such resolution change,
> enable the property in video firmware.
> Also, EOS handling is now made same in video firmware across all V6 SOCs,
> hence above a certain firmware version, the driver handling is
> made generic for all V6s
> 
> Signed-off-by: Vikash Garodia <vgarodia@qti.qualcomm.com>
> Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com>
> Tested-by: Nathan Hebert <nhebert@chromium.org>
> ---
> Since v3 : Addressed comments to rectify email address.

This is not true. Your to and cc lists are still incorrect.

-- 
With best wishes
Dmitry