RE: [PATCH V2 1/1] venus: Enable sufficient sequence change support for sc7180 and fix for Decoder STOP command issue.

[Viswanath Boma (Temp)]

Will ensure more on V3 patch if any comments from Stan/Vikash .
Thanks,
viswanath
-----Original Message-----
From: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> 
Sent: Thursday, February 2, 2023 3:41 PM
To: Viswanath Boma (Temp) (QUIC) <quic_vboma@xxxxxxxxxxx>
Cc: stanimir.varbanov@xxxxxxxxxx; Vikash Garodia (QUIC) <quic_vgarodia@xxxxxxxxxxx>; Andy Gross <agross@xxxxxxxxxx>; bjorn.andersson@xxxxxxxxxx; Konrad Dybcio <konrad.dybcio@xxxxxxxxxx>; Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>; linux-media@xxxxxxxxxxxxxxx; linux-arm-msm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Vikash Garodia <vgarodia@xxxxxxxxxxxxxxxx>
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@xxxxxxxxxxx> wrote:
>
> From: Viswanath Boma <quic_vboma@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxx>
> Signed-off-by: Viswanath Boma <quic_vboma@xxxxxxxxxxx>
> Tested-by: Nathan Hebert <nhebert@xxxxxxxxxxxx>
> ---
>  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