| Message ID | 20221115121237.28361-2-quic_vboma@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix for VP9 DRC and Decoder STOP issue. | expand |
On Tue, Nov 15, 2022 at 4:12 AM <quic_vboma@quicinc.com> wrote: > > From: Viswanath Boma <quic_vboma@quicinc.com> > > 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 > With this patch and the latest Venus SC7180 firmware, VP9 interframe resolution changes are handled and decoded correctly. Tested-by: Nathan Hebert <nhebert@chromium.org> > Signed-off-by: Vikash Garodia <vgarodia@qti.qualcomm.com> > Signed-off-by: Viswanath Boma <quic_vboma@quicinc.com> > --- > drivers/media/platform/qcom/venus/core.h | 10 ++++++++++ > 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, 33 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h > index 32551c2602a9..538d7cd41edd 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 { > @@ -472,6 +477,11 @@ struct venus_inst { > #define IS_V3(core) ((core)->res->hfi_version == HFI_VERSION_3XX) > #define IS_V4(core) ((core)->res->hfi_version == HFI_VERSION_4XX) > #define IS_V6(core) ((core)->res->hfi_version == HFI_VERSION_6XX) > +#define IS_FW_REV_OR_NEWER(core, vmajor, vminor, vrev) ((core)->venus_ver.major == vmajor && \ > + (core)->venus_ver.minor == vminor && (core)->venus_ver.rev >= vrev) > +#define IS_FW_REV_OR_OLDER(core, vmajor, vminor, vrev) ((core)->venus_ver.major == vmajor && \ > + (core)->venus_ver.minor == vminor && (core)->venus_ver.rev <= vrev) > + > > #define ctrl_to_inst(ctrl) \ > container_of((ctrl)->handler, struct venus_inst, ctrl_handler) > diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.c b/drivers/media/platform/qcom/venus/hfi_cmds.c > index 930b743f225e..e2539b58340f 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 d2d6719a2ba4..20516b4361d3 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..4a398f77fe9c 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..cc19eb62b723 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 >
From: Viswanath Boma <quic_vboma@quicinc.com>
Tested the changes on v5.15 and v5.4 kernels .
For testing Chrome Utilities were used .
Viswanath Boma (1):
venus: Enable sufficient sequence change support for sc7180 and fix
for Decoder STOP command issue.
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(-)
On Thu, 2 Feb 2023 at 08:47, <quic_vboma@quicinc.com> wrote: > > From: Viswanath Boma <quic_vboma@quicinc.com> > > Tested the changes on v5.15 and v5.4 kernels . Please test your patches properly. It is 6.2-rc already. > For testing Chrome Utilities were used . > > Viswanath Boma (1): > venus: Enable sufficient sequence change support for sc7180 and fix > for Decoder STOP command issue. > > 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(-)
On Thu, 2 Feb 2023 at 08:47, <quic_vboma@quicinc.com> wrote: > > From: Viswanath Boma <quic_vboma@quicinc.com> > > 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> > --- > 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(-) Several generic comments: - Please move your work on top of the recent kernels. 5.15 was released half of the year ago. I'm not going to mention 5.4 age. - Please split your patch into smaller logical patches. > > diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h > index 32551c2602a9..8f94d795cc2b 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); Please make the indentation logical here (and below). Also is 5.6.1 (e.g.) newer than 5.4.51? Or 5.4.1 newer than 4.2.0? > +} > + > +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 930b743f225e..e2539b58340f 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 d2d6719a2ba4..20516b4361d3 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); What about older (V1/V3) cores? > > 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")) { Do newer chips support this property? Do you intend to list all of them here? > + 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;
Will ensure more on V3 patch if any comments from Stan/Vikash . Thanks, viswanath -----Original Message----- From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Sent: Thursday, February 2, 2023 3:41 PM To: Viswanath Boma (Temp) (QUIC) <quic_vboma@quicinc.com> Cc: stanimir.varbanov@linaro.org; Vikash Garodia (QUIC) <quic_vgarodia@quicinc.com>; Andy Gross <agross@kernel.org>; bjorn.andersson@linaro.org; Konrad Dybcio <konrad.dybcio@linaro.org>; Mauro Carvalho Chehab <mchehab@kernel.org>; linux-media@vger.kernel.org; linux-arm-msm@vger.kernel.org; linux-kernel@vger.kernel.org; Vikash Garodia <vgarodia@qti.qualcomm.com> Subject: Re: [PATCH V2 1/1] venus: Enable sufficient sequence change support for sc7180 and fix for Decoder STOP command issue. WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros. On Thu, 2 Feb 2023 at 08:47, <quic_vboma@quicinc.com> wrote: > > From: Viswanath Boma <quic_vboma@quicinc.com> > > 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> > --- > 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(-) Several generic comments: - Please move your work on top of the recent kernels. 5.15 was released half of the year ago. I'm not going to mention 5.4 age. - Please split your patch into smaller logical patches. [vboma] >> As per the current client environment working on 5.15 kernel and the same changes were also ensured on 5.4 . >> Current changes related bringing up the utility functions to fix couple of bugs on latest firmware versions. >> In future , As sugggested will split the changes if they can be isolated as smaller meaningful part . > > diff --git a/drivers/media/platform/qcom/venus/core.h > b/drivers/media/platform/qcom/venus/core.h > index 32551c2602a9..8f94d795cc2b 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); Please make the indentation logical here (and below). Also is 5.6.1 (e.g.) newer than 5.4.51? Or 5.4.1 newer than 4.2.0? [vboma] >> Expected one more indent to right ? will ensure . >> These versions check related to major/minor versions of the Firmware releases to address the mentioned issues and also if any role back preserves the older behavior. > +} > + > +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 930b743f225e..e2539b58340f 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 d2d6719a2ba4..20516b4361d3 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); What about older (V1/V3) cores? [vboma] >> Older cores not having these issues , hence as required V4 and V6 were handled as per Current client issues. > > 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")) { Do newer chips support this property? Do you intend to list all of them here? [vboma] >> Basing on capability of the valid chipset vs firmware support ,current changes were added . > + 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
HI Dmirty , Thanks for reviews . > -----Original Message----- > From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Sent: Thursday, March 16, 2023 3:58 PM > To: Viswanath Boma (Temp) <vboma@qti.qualcomm.com> > Cc: Viswanath Boma (Temp) (QUIC) <quic_vboma@quicinc.com>; > stanimir.varbanov@linaro.org; Vikash Garodia (QUIC) > <quic_vgarodia@quicinc.com>; Andy Gross <agross@kernel.org>; > bjorn.andersson@linaro.org; Konrad Dybcio <konrad.dybcio@linaro.org>; > Mauro Carvalho Chehab <mchehab@kernel.org>; linux- > media@vger.kernel.org; linux-arm-msm@vger.kernel.org; linux- > kernel@vger.kernel.org; Vikash Garodia <vgarodia@qti.qualcomm.com> > Subject: Re: [PATCH V2 1/1] venus: Enable sufficient sequence change support > for sc7180 and fix for Decoder STOP command issue. > > WARNING: This email originated from outside of Qualcomm. Please be wary of > any links or attachments, and do not enable macros. > > On Fri, 3 Feb 2023 at 06:24, Viswanath Boma (Temp) > <vboma@qti.qualcomm.com> wrote: > > > > > > Will ensure more on V3 patch if any comments from Stan/Vikash . > > Thanks, > > viswanath > > Please fix your email configuration. Top-posting is generally frowned upon. All > the headers bellow are frowned upon too. > Sure, will ensure proper version tag in the next patch set. > > -----Original Message----- > > From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > Sent: Thursday, February 2, 2023 3:41 PM > > To: Viswanath Boma (Temp) (QUIC) <quic_vboma@quicinc.com> > > Cc: stanimir.varbanov@linaro.org; Vikash Garodia (QUIC) > > <quic_vgarodia@quicinc.com>; Andy Gross <agross@kernel.org>; > > bjorn.andersson@linaro.org; Konrad Dybcio <konrad.dybcio@linaro.org>; > > Mauro Carvalho Chehab <mchehab@kernel.org>; > > linux-media@vger.kernel.org; linux-arm-msm@vger.kernel.org; > > linux-kernel@vger.kernel.org; Vikash Garodia > > <vgarodia@qti.qualcomm.com> > > Subject: Re: [PATCH V2 1/1] venus: Enable sufficient sequence change support > for sc7180 and fix for Decoder STOP command issue. > > > > WARNING: This email originated from outside of Qualcomm. Please be wary > of any links or attachments, and do not enable macros. > > > > On Thu, 2 Feb 2023 at 08:47, <quic_vboma@quicinc.com> wrote: > > > > > > From: Viswanath Boma <quic_vboma@quicinc.com> > > > > > > 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> > > > --- > > > 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(-) > > > > Several generic comments: > > - Please move your work on top of the recent kernels. 5.15 was released half > of the year ago. I'm not going to mention 5.4 age. > > - Please split your patch into smaller logical patches. > > [vboma] > > >> As per the current client environment working on 5.15 kernel and the > same changes were also ensured on 5.4 . > > >> Current changes related bringing up the utility functions to fix couple of > bugs on latest firmware versions. > > >> In future , As sugggested will split the changes if they can be isolated as > smaller meaningful part . > > UGH. This looks like a third level quotation, not like your reply. > Could you please spend some time and fix your email client? It is close to > impossible to notice your replies otherwise. > > Were these changes validated on top of linux-master or linux-next? If not, > please go ahead and validate them before sending the next patch revision. > > > > > > > > diff --git a/drivers/media/platform/qcom/venus/core.h > > > b/drivers/media/platform/qcom/venus/core.h > > > index 32551c2602a9..8f94d795cc2b 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); > > > > Please make the indentation logical here (and below). > > Also is 5.6.1 (e.g.) newer than 5.4.51? Or 5.4.1 newer than 4.2.0? > > [vboma] > > >> Expected one more indent to right ? will ensure . > > Expected is to have the indentation logical. Saying if ((major == A) && (minor > == > B)) is not logical. > > A good (and easier to comprehend) example might be: > > if ((major == A) && > (minor == B)) { > } > > > >> These versions check related to major/minor versions of the Firmware > releases to address the mentioned issues and also if any role back preserves > the older behavior. > > > > > +} > > > + > > > +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 930b743f225e..e2539b58340f 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 d2d6719a2ba4..20516b4361d3 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); > > > > What about older (V1/V3) cores? > > [vboma] > > >> Older cores not having these issues , hence as required V4 and V6 were > handled as per Current client issues. > > There are no clients here and no client issues. If you are handling parsing > versions, please make it work for all supported devices. > > > > > > > > > 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")) { > > > > Do newer chips support this property? Do you intend to list all of them here? > > [vboma] > > >> Basing on capability of the valid chipset vs firmware support ,current > changes were added . > > Are there any other chipsets using 5.4 firmware. If they are, are you going to > list them here? If not, you can skip the of_device_is_compatible and just check > for fw 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; > > > > > > > > -- > > With best wishes > > Dmitry > > > > -- > With best wishes > Dmitry
diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h index 32551c2602a9..538d7cd41edd 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 { @@ -472,6 +477,11 @@ struct venus_inst { #define IS_V3(core) ((core)->res->hfi_version == HFI_VERSION_3XX) #define IS_V4(core) ((core)->res->hfi_version == HFI_VERSION_4XX) #define IS_V6(core) ((core)->res->hfi_version == HFI_VERSION_6XX) +#define IS_FW_REV_OR_NEWER(core, vmajor, vminor, vrev) ((core)->venus_ver.major == vmajor && \ + (core)->venus_ver.minor == vminor && (core)->venus_ver.rev >= vrev) +#define IS_FW_REV_OR_OLDER(core, vmajor, vminor, vrev) ((core)->venus_ver.major == vmajor && \ + (core)->venus_ver.minor == vminor && (core)->venus_ver.rev <= vrev) + #define ctrl_to_inst(ctrl) \ container_of((ctrl)->handler, struct venus_inst, ctrl_handler) diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.c b/drivers/media/platform/qcom/venus/hfi_cmds.c index 930b743f225e..e2539b58340f 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 d2d6719a2ba4..20516b4361d3 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..4a398f77fe9c 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..cc19eb62b723 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;