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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



HI Dmirty ,

Thanks for reviews .


> -----Original Message-----
> From: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> Sent: Thursday, March 16, 2023 3:58 PM
> To: Viswanath Boma (Temp) <vboma@xxxxxxxxxxxxxxxx>
> Cc: Viswanath Boma (Temp) (QUIC) <quic_vboma@xxxxxxxxxxx>;
> 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 Fri, 3 Feb 2023 at 06:24, Viswanath Boma (Temp)
> <vboma@xxxxxxxxxxxxxxxx> 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@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 .
> 
> 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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux