| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <8c3fe4712e183118b3488c987195d9c9aae966de.camel@mediatek.com>
Date: Fri, 24 Jun 2022 17:47:29 +0800
From: Irui Wang <irui.wang@...iatek.com>
To: AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com>,
Hans Verkuil <hverkuil-cisco@...all.nl>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Matthias Brugger <matthias.bgg@...il.com>,
Tomasz Figa <tfiga@...gle.com>,
Tzung-Bi Shih <tzungbi@...omium.org>,
Alexandre Courbot <acourbot@...omium.org>,
"Tiffany Lin" <tiffany.lin@...iatek.com>,
Andrew-CT Chen <andrew-ct.chen@...iatek.com>,
<nicolas.dufresne@...labora.com>
CC: Hsin-Yi Wang <hsinyi@...omium.org>,
Maoguang Meng <maoguang.meng@...iatek.com>,
Longfei Wang <longfei.wang@...iatek.com>,
Yunfei Dong <yunfei.dong@...iatek.com>,
<linux-media@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>,
<srv_heupstream@...iatek.com>,
<linux-mediatek@...ts.infradead.org>,
<Project_Global_Chrome_Upstream_Group@...iatek.com>
Subject: Re: [PATCH v4, 4/8] media: mediatek: vcodec: Add more extra
processing for multi-core encoding
Dear Angelo,
Many thanks for your reviewing and comments. I will fix them in next
version.
On Fri, 2022-06-24 at 11:00 +0200, AngeloGioacchino Del Regno wrote:
> Il 24/06/22 10:23, Irui Wang ha scritto:
> > Add a bit for indicating support multi-core encoding, because
> > multi-core
> > encoding need more working buffers for encoder hardware. The
> > working
> > buffers are allocated from kernel side then pass to scp firmware
> > side
> > through shared memory, the struct definition must be kept align
> > between
> > kernel and scp firmware side. New another shared memory struct for
> > multi-core encoding.
> >
> > Signed-off-by: Irui Wang <irui.wang@...iatek.com>
> > ---
> > .../platform/mediatek/vcodec/mtk_vcodec_drv.h | 2 +
> > .../mediatek/vcodec/mtk_vcodec_util.c | 19 ++
> > .../mediatek/vcodec/mtk_vcodec_util.h | 2 +
> > .../mediatek/vcodec/venc/venc_h264_if.c | 171
> > +++++++++++++++---
> > 4 files changed, 170 insertions(+), 24 deletions(-)
> >
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h
> > b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h
> > index 8919bdf2eef5..3a291cac6b5e 100644
> > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h
> > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_drv.h
> > @@ -29,6 +29,8 @@
> > #define WAIT_INTR_TIMEOUT_MS 1000
> > #define IS_VDEC_LAT_ARCH(hw_arch) ((hw_arch) >=
> > MTK_VDEC_LAT_SINGLE_CORE)
> > #define IS_VDEC_INNER_RACING(capability) ((capability) &
> > MTK_VCODEC_INNER_RACING)
> > +#define MTK_VENC_MULTICORE_ENABLE BIT(1)
> > +#define IS_VENC_MULTICORE(capability) ((capability) &
> > MTK_VENC_MULTICORE_ENABLE)
> >
> > /*
> > * enum mtk_hw_reg_idx - MTK hw register base index
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.c
> > b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.c
> > index ace78c4b5b9e..a723243626c0 100644
> > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.c
> > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.c
> > @@ -11,6 +11,7 @@
> >
> > #include "mtk_vcodec_dec_hw.h"
> > #include "mtk_vcodec_drv.h"
> > +#include "mtk_vcodec_enc_hw.h"
> > #include "mtk_vcodec_util.h"
> >
> > void __iomem *mtk_vcodec_get_reg_addr(struct mtk_vcodec_ctx
> > *data,
> > @@ -26,6 +27,24 @@ void __iomem *mtk_vcodec_get_reg_addr(struct
> > mtk_vcodec_ctx *data,
> > }
> > EXPORT_SYMBOL(mtk_vcodec_get_reg_addr);
> >
> > +void __iomem *mtk_venc_get_core_reg_addr(struct mtk_vcodec_ctx
> > *ctx,
> > + unsigned int hw_id)
>
> This is enum mtk_venc_hw_id.....
fix in next version.
>
> > +{
> > + struct mtk_venc_hw_dev *sub_core;
> > +
> > + if (hw_id >= MTK_VENC_HW_MAX) {
> > + mtk_v4l2_err("Invalid hw_id = %d", hw_id);
> > + return NULL;
> > + }
> > +
> > + sub_core = (struct mtk_venc_hw_dev *)ctx->dev-
> > >enc_hw_dev[hw_id];
> > + if (!sub_core)
> > + return NULL;
> > +
> > + return sub_core->reg_base;
> > +}
> > +EXPORT_SYMBOL(mtk_venc_get_core_reg_addr);
> > +
> > int mtk_vcodec_mem_alloc(struct mtk_vcodec_ctx *data,
> > struct mtk_vcodec_mem *mem)
> > {
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.h
> > b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.h
> > index 71956627a0e2..a74c98aa355b 100644
> > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.h
> > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.h
> > @@ -50,6 +50,8 @@ struct mtk_vcodec_dev;
> >
> > void __iomem *mtk_vcodec_get_reg_addr(struct mtk_vcodec_ctx
> > *data,
> > unsigned int reg_idx);
> > +void __iomem *mtk_venc_get_core_reg_addr(struct mtk_vcodec_ctx
> > *data,
> > + unsigned int hw_id);
> > int mtk_vcodec_mem_alloc(struct mtk_vcodec_ctx *data,
> > struct mtk_vcodec_mem *mem);
> > void mtk_vcodec_mem_free(struct mtk_vcodec_ctx *data,
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/venc/venc_h264_if.c
> > b/drivers/media/platform/mediatek/vcodec/venc/venc_h264_if.c
> > index 4d9b8798dffe..888cee39d324 100644
> > --- a/drivers/media/platform/mediatek/vcodec/venc/venc_h264_if.c
> > +++ b/drivers/media/platform/mediatek/vcodec/venc/venc_h264_if.c
>
> ..snip..
>
> > @@ -143,8 +184,8 @@ struct venc_h264_vsi {
> > * @ctx: context for v4l2 layer integration
> > */
> > struct venc_h264_inst {
> > - void __iomem *hw_base;
> > - struct mtk_vcodec_mem work_bufs[VENC_H264_VPU_WORK_BUF_MAX];
> > + void __iomem *hw_base[MTK_VENC_HW_MAX];
> > + struct mtk_vcodec_mem work_bufs[VENC_MULTI_CORE_WORK_BUF_MAX];
> > struct mtk_vcodec_mem pps_buf;
> > bool work_buf_allocated;
> > unsigned int frm_cnt;
> > @@ -152,12 +193,13 @@ struct venc_h264_inst {
> > unsigned int prepend_hdr;
> > struct venc_vpu_inst vpu_inst;
> > struct venc_h264_vsi *vsi;
> > + struct venc_multi_core_vsi *core_vsi;
>
> You're adding a new struct member without adding it to the
> documentation,
> please add it there too.
Add it in next version.
>
> > struct mtk_vcodec_ctx *ctx;
> > };
> >
> > static inline u32 h264_read_reg(struct venc_h264_inst *inst, u32
> > addr)
> > {
> > - return readl(inst->hw_base + addr);
> > + return readl(inst->hw_base[MTK_VENC_CORE_0] + addr);
> > }
> >
> > static unsigned int h264_get_profile(struct venc_h264_inst *inst,
> > @@ -228,13 +270,20 @@ static unsigned int h264_get_level(struct
> > venc_h264_inst *inst,
> > static void h264_enc_free_work_buf(struct venc_h264_inst *inst)
> > {
> > int i;
> > + struct mtk_vcodec_ctx *ctx = inst->ctx;
> > + int max_work_buf;
>
> int i, max_work_buf;
>
> >
> > mtk_vcodec_debug_enter(inst);
> >
> > + if (IS_VENC_MULTICORE(ctx->dev->enc_capability))
> > + max_work_buf = VENC_MULTI_CORE_WORK_BUF_MAX;
> > + else
> > + max_work_buf = VENC_H264_VPU_WORK_BUF_MAX;
> > +
> > /* Except the SKIP_FRAME buffers,
> > * other buffers need to be freed by AP.
> > */
> > - for (i = 0; i < VENC_H264_VPU_WORK_BUF_MAX; i++) {
> > + for (i = 0; i < max_work_buf; i++) {
> > if (i != VENC_H264_VPU_WORK_BUF_SKIP_FRAME)
> > mtk_vcodec_mem_free(inst->ctx, &inst-
> > >work_bufs[i]);
> > }
> > @@ -248,11 +297,21 @@ static int h264_enc_alloc_work_buf(struct
> > venc_h264_inst *inst)
> > {
> > int i;
> > int ret = 0;
> > - struct venc_h264_vpu_buf *wb = inst->vsi->work_bufs;
> > + struct mtk_vcodec_ctx *ctx = inst->ctx;
> > + struct venc_h264_vpu_buf *wb;
> > + int max_work_buf;
>
> While at it, can you please fix the order of these declarations?
>
> struct mtk_vcodec_ctx *ctx = inst->ctx;
> struct venc_h264_vpu_buf *wb;
> int i, max_work_buf;
> int ret = 0;
>
fix it in next version.
> >
> > mtk_vcodec_debug_enter(inst);
> >
> > - for (i = 0; i < VENC_H264_VPU_WORK_BUF_MAX; i++) {
> > + if (IS_VENC_MULTICORE(ctx->dev->enc_capability)) {
> > + wb = inst->core_vsi->work_bufs;
> > + max_work_buf = VENC_MULTI_CORE_WORK_BUF_MAX;
> > + } else {
> > + wb = inst->vsi->work_bufs;
> > + max_work_buf = VENC_H264_VPU_WORK_BUF_MAX;
> > + }
> > +
> > + for (i = 0; i < max_work_buf; i++) {
> > /*
> > * This 'wb' structure is set by VPU side and shared to
> > AP for
> > * buffer allocation and IO virtual addr mapping. For
> > most of
> > @@ -358,6 +417,26 @@ static int h264_frame_type(struct
> > venc_h264_inst *inst)
> > return VENC_H264_P_FRM; /* Note: B frames are not
> > supported */
> > }
> > }
> > +
> > +static int h264_core_frame_type(struct venc_h264_inst *inst)
> > +{
> > + struct venc_multi_core_vsi *vsi = inst->core_vsi;
> > +
> > + if ((vsi->config.gop_size != 0 &&
> > + (inst->frm_cnt % vsi->config.gop_size) == 0) ||
> > + (inst->frm_cnt == 0 && vsi->config.gop_size == 0)) {
> > + /* IDR frame */
> > + return VENC_H264_IDR_FRM;
>
> You can get to a maximum of 100 columns and, thinking about
> readability...:
>
> if ((vsi->config.gop_size != 0 && (inst->frm_cnt % vsi-
> >config.gop_size) == 0) ||
> (inst->frm_cnt == 0 && vsi->config.gop_size == 0)) {
>
> this gets to 90 columns, so it's acceptable (and more readable).
>
> > + } else if ((vsi->config.intra_period != 0 &&
> > + (inst->frm_cnt % vsi->config.intra_period) == 0) ||
> > + (inst->frm_cnt == 0 && vsi->config.intra_period ==
> > 0)) {
>
> ..but, unfortunately, this doesn't, so we have to live with it.
thanks, I will check it.
>
>
> > + /* I frame */
> > + return VENC_H264_I_FRM;
> > + } else {
> > + return VENC_H264_P_FRM; /* Note: B frames are not
> > supported */
> > + }
> > +}
> > +
> > static int h264_encode_sps(struct venc_h264_inst *inst,
> > struct mtk_vcodec_mem *bs_buf,
> > unsigned int *bs_size)
> > @@ -440,12 +519,16 @@ static int h264_encode_frame(struct
> > venc_h264_inst *inst,
> > int ret = 0;
> > unsigned int irq_status;
> > struct venc_frame_info frame_info;
> > + struct mtk_vcodec_ctx *ctx = inst->ctx;
> >
> > mtk_vcodec_debug_enter(inst);
> > mtk_vcodec_debug(inst, "frm_cnt = %d\n ", inst->frm_cnt);
> > frame_info.frm_count = inst->frm_cnt;
> > frame_info.skip_frm_count = inst->skip_frm_cnt;
> > - frame_info.frm_type = h264_frame_type(inst);
> > + if (IS_VENC_MULTICORE(ctx->dev->enc_capability))
> > + frame_info.frm_type = h264_core_frame_type(inst);
> > + else
> > + frame_info.frm_type = h264_frame_type(inst);
> > mtk_vcodec_debug(inst, "frm_count = %d,skip_frm_count
> > =%d,frm_type=%d.\n",
> > frame_info.frm_count,
> > frame_info.skip_frm_count,
> > frame_info.frm_type);
> > @@ -501,7 +584,7 @@ static void h264_encode_filler(struct
> > venc_h264_inst *inst, void *buf,
> > static int h264_enc_init(struct mtk_vcodec_ctx *ctx)
> > {
> > const bool is_ext = MTK_ENC_CTX_IS_EXT(ctx);
> > - int ret = 0;
> > + int ret, i;
>
> Can you also move this after `inst` please?
>
fix it in next version.
> > struct venc_h264_inst *inst;
> >
>
> Cheers,
> Angelo
Thanks
Best Regards
Powered by blists - more mailing lists