lkml.org 
[lkml]   [2023]   [Jun]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v3,03/11] media: mediatek: vcodec: re-write shared interface
    Date
    Hi Nicolas,

    Thanks for your suggestion.

    On Mon, 2023-06-19 at 10:53 -0400, Nicolas Dufresne wrote:
    >
    > External email : Please do not click links or open attachments until
    > you have verified the sender or the content.
    >
    > Hi Yunfei,
    >
    >
    > Le samedi 17 juin 2023 à 18:32 +0800, Yunfei Dong a écrit :
    > > Re-write shared interface which encoder and decoder used at
    > > the same time. Using the common struct as the parameter of
    > > these interface in order to remove the depedency.
    > >
    > > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
    > > ---
    > > .../mediatek/vcodec/mtk_vcodec_intr.c | 30 ++++++++++++---
    > ----
    > > .../mediatek/vcodec/mtk_vcodec_intr.h | 3 +-
    > > .../mediatek/vcodec/mtk_vcodec_util.c | 19 +++++-------
    > > .../mediatek/vcodec/mtk_vcodec_util.h | 9 ++----
    > > .../mediatek/vcodec/vdec/vdec_vp8_if.c | 14 ++++-----
    > > .../mediatek/vcodec/venc/venc_h264_if.c | 2 +-
    > > .../mediatek/vcodec/venc/venc_vp8_if.c | 2 +-
    > > 7 files changed, 39 insertions(+), 40 deletions(-)
    > >
    > > diff --git
    > a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_intr.c
    > b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_intr.c
    > > index 552b4c93d972..daa44f635727 100644
    > > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_intr.c
    > > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_intr.c
    > > @@ -11,32 +11,40 @@
    > > #include "mtk_vcodec_intr.h"
    > > #include "mtk_vcodec_util.h"
    > >
    > > -int mtk_vcodec_wait_for_done_ctx(struct mtk_vcodec_ctx *ctx,
    > > - int command, unsigned int timeout_ms,
    > > +int mtk_vcodec_wait_for_done_ctx(void *priv, int command, unsigned
    > int timeout_ms,
    > > unsigned int hw_id)
    > > {
    > > +struct mtk_vcodec_ctx *ctx = priv;
    > > long timeout_jiff, ret;
    > > -int status = 0;
    > > +int status = 0, ctx_id, ctx_type;
    > > +int *ctx_int_cond, *ctx_int_type;
    > > +wait_queue_head_t *ctx_queue;
    > > +
    > > +ctx_id = ctx->id;
    > > +ctx_type = ctx->type;
    > > +ctx_int_cond = ctx->int_cond;
    > > +ctx_int_type = ctx->int_type;
    > > +ctx_queue = ctx->queue;
    > >
    > > timeout_jiff = msecs_to_jiffies(timeout_ms);
    > > -ret = wait_event_interruptible_timeout(ctx->queue[hw_id],
    > > - ctx->int_cond[hw_id],
    > > +ret = wait_event_interruptible_timeout(ctx_queue[hw_id],
    > > + ctx_int_cond[hw_id],
    > > timeout_jiff);
    > >
    > > if (!ret) {
    > > status = -1;/* timeout */
    > > mtk_v4l2_err("[%d] cmd=%d, type=%d, dec timeout=%ums (%d %d)",
    > > - ctx->id, command, ctx->type, timeout_ms,
    > > - ctx->int_cond[hw_id], ctx->int_type[hw_id]);
    > > + ctx_id, command, ctx_type, timeout_ms,
    > > + ctx_int_cond[hw_id], ctx_int_type[hw_id]);
    > > } else if (-ERESTARTSYS == ret) {
    > > status = -1;
    > > mtk_v4l2_err("[%d] cmd=%d, type=%d, dec inter fail (%d %d)",
    > > - ctx->id, command, ctx->type,
    > > - ctx->int_cond[hw_id], ctx->int_type[hw_id]);
    > > + ctx_id, command, ctx_type,
    > > + ctx_int_cond[hw_id], ctx_int_type[hw_id]);
    > > }
    > >
    > > -ctx->int_cond[hw_id] = 0;
    > > -ctx->int_type[hw_id] = 0;
    > > +ctx_int_cond[hw_id] = 0;
    > > +ctx_int_type[hw_id] = 0;
    > >
    > > return status;
    > > }
    > > diff --git
    > a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_intr.h
    > b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_intr.h
    > > index 9681f492813b..11bf0ef94d5d 100644
    > > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_intr.h
    > > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_intr.h
    > > @@ -12,8 +12,7 @@
    > > struct mtk_vcodec_ctx;
    >
    > You have a forward declaration here.
    >
    I will check it again.
    > >
    > > /* timeout is ms */
    > > -int mtk_vcodec_wait_for_done_ctx(struct mtk_vcodec_ctx *ctx,
    > > - int command, unsigned int timeout_ms,
    > > +int mtk_vcodec_wait_for_done_ctx(void *priv, int command, unsigned
    > int timeout_ms,
    > > unsigned int hw_id);
    >
    > So has the CTX is only uses has a pointer, its hard to follow why you
    > need to
    > hide the type here. At least its not clear to me how this helps with
    > the goal
    > set in the commit message and would simply like to understand before
    > giving an
    > r-b.
    >
    For this function is used by encoder and decoder, the 'struct
    mtk_vcodec_ctx' will be change to 'struct mtk_vcodec_enc_ctx' and
    'struct mtk_vcodec_dec_ctx' in order to separate encoder and decoder
    the in patch 7.

    I will write the commit message detail in next v4.

    Best Regards,
    Yunfei Dong
    > >
    > > #endif /* _MTK_VCODEC_INTR_H_ */
    > > diff --git
    > a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.c
    > b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.c
    > > index f214e6f67005..847e321f4fcc 100644
    > > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.c
    > > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.c
    > > @@ -21,24 +21,20 @@ int mtk_v4l2_dbg_level;
    > > EXPORT_SYMBOL(mtk_v4l2_dbg_level);
    > > #endif
    > >
    > > -void __iomem *mtk_vcodec_get_reg_addr(struct mtk_vcodec_ctx *data,
    > > -unsigned int reg_idx)
    > > +void __iomem *mtk_vcodec_get_reg_addr(void __iomem **reg_base,
    > unsigned int reg_idx)
    > > {
    > > -struct mtk_vcodec_ctx *ctx = (struct mtk_vcodec_ctx *)data;
    > > -
    > > -if (!data || reg_idx >= NUM_MAX_VCODEC_REG_BASE) {
    > > +if (reg_idx >= NUM_MAX_VCODEC_REG_BASE) {
    > > mtk_v4l2_err("Invalid arguments, reg_idx=%d", reg_idx);
    > > return NULL;
    > > }
    > > -return ctx->dev->reg_base[reg_idx];
    > > +return reg_base[reg_idx];
    > > }
    > > EXPORT_SYMBOL(mtk_vcodec_get_reg_addr);
    > >
    > > -int mtk_vcodec_mem_alloc(struct mtk_vcodec_ctx *data,
    > > -struct mtk_vcodec_mem *mem)
    > > +int mtk_vcodec_mem_alloc(void *priv, struct mtk_vcodec_mem *mem)
    > > {
    > > unsigned long size = mem->size;
    > > -struct mtk_vcodec_ctx *ctx = (struct mtk_vcodec_ctx *)data;
    > > +struct mtk_vcodec_ctx *ctx = priv;
    > > struct device *dev = &ctx->dev->plat_dev->dev;
    > >
    > > mem->va = dma_alloc_coherent(dev, size, &mem->dma_addr,
    > GFP_KERNEL);
    > > @@ -57,11 +53,10 @@ int mtk_vcodec_mem_alloc(struct mtk_vcodec_ctx
    > *data,
    > > }
    > > EXPORT_SYMBOL(mtk_vcodec_mem_alloc);
    > >
    > > -void mtk_vcodec_mem_free(struct mtk_vcodec_ctx *data,
    > > -struct mtk_vcodec_mem *mem)
    > > +void mtk_vcodec_mem_free(void *priv, struct mtk_vcodec_mem *mem)
    > > {
    > > unsigned long size = mem->size;
    > > -struct mtk_vcodec_ctx *ctx = (struct mtk_vcodec_ctx *)data;
    > > +struct mtk_vcodec_ctx *ctx = priv;
    > > struct device *dev = &ctx->dev->plat_dev->dev;
    > >
    > > if (!mem->va) {
    > > diff --git
    > a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.h
    > b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.h
    > > index 88d389b65f13..827937bcb4b4 100644
    > > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.h
    > > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_util.h
    > > @@ -68,12 +68,9 @@ extern int mtk_vcodec_dbg;
    > > #define mtk_vcodec_debug_enter(h) mtk_vcodec_debug(h, "+")
    > > #define mtk_vcodec_debug_leave(h) mtk_vcodec_debug(h, "-")
    > >
    > > -void __iomem *mtk_vcodec_get_reg_addr(struct mtk_vcodec_ctx *data,
    > > -unsigned int reg_idx);
    > > -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,
    > > -struct mtk_vcodec_mem *mem);
    > > +void __iomem *mtk_vcodec_get_reg_addr(void __iomem **reg_base,
    > unsigned int reg_idx);
    > > +int mtk_vcodec_mem_alloc(void *priv, struct mtk_vcodec_mem *mem);
    > > +void mtk_vcodec_mem_free(void *priv, struct mtk_vcodec_mem *mem);
    > > void mtk_vcodec_set_curr_ctx(struct mtk_vcodec_dev *vdec_dev,
    > > struct mtk_vcodec_ctx *ctx, int hw_idx);
    > > struct mtk_vcodec_ctx *mtk_vcodec_get_curr_ctx(struct
    > mtk_vcodec_dev *vdec_dev,
    > > diff --git
    > a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp8_if.c
    > b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp8_if.c
    > > index 88c046731754..5edbccc9ae68 100644
    > > --- a/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp8_if.c
    > > +++ b/drivers/media/platform/mediatek/vcodec/vdec/vdec_vp8_if.c
    > > @@ -167,13 +167,13 @@ struct vdec_vp8_inst {
    > >
    > > static void get_hw_reg_base(struct vdec_vp8_inst *inst)
    > > {
    > > -inst->reg_base.top = mtk_vcodec_get_reg_addr(inst->ctx, VDEC_TOP);
    > > -inst->reg_base.cm = mtk_vcodec_get_reg_addr(inst->ctx, VDEC_CM);
    > > -inst->reg_base.hwd = mtk_vcodec_get_reg_addr(inst->ctx, VDEC_HWD);
    > > -inst->reg_base.sys = mtk_vcodec_get_reg_addr(inst->ctx, VDEC_SYS);
    > > -inst->reg_base.misc = mtk_vcodec_get_reg_addr(inst->ctx,
    > VDEC_MISC);
    > > -inst->reg_base.ld = mtk_vcodec_get_reg_addr(inst->ctx, VDEC_LD);
    > > -inst->reg_base.hwb = mtk_vcodec_get_reg_addr(inst->ctx, VDEC_HWB);
    > > +inst->reg_base.top = mtk_vcodec_get_reg_addr(inst->ctx->dev-
    > >reg_base, VDEC_TOP);
    > > +inst->reg_base.cm = mtk_vcodec_get_reg_addr(inst->ctx->dev-
    > >reg_base, VDEC_CM);
    > > +inst->reg_base.hwd = mtk_vcodec_get_reg_addr(inst->ctx->dev-
    > >reg_base, VDEC_HWD);
    > > +inst->reg_base.sys = mtk_vcodec_get_reg_addr(inst->ctx->dev-
    > >reg_base, VDEC_SYS);
    > > +inst->reg_base.misc = mtk_vcodec_get_reg_addr(inst->ctx->dev-
    > >reg_base, VDEC_MISC);
    > > +inst->reg_base.ld = mtk_vcodec_get_reg_addr(inst->ctx->dev-
    > >reg_base, VDEC_LD);
    > > +inst->reg_base.hwb = mtk_vcodec_get_reg_addr(inst->ctx->dev-
    > >reg_base, VDEC_HWB);
    > > }
    > >
    > > static void write_hw_segmentation_data(struct vdec_vp8_inst *inst)
    > > 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 60fd165c0d94..10365c95ebbe 100644
    > > --- a/drivers/media/platform/mediatek/vcodec/venc/venc_h264_if.c
    > > +++ b/drivers/media/platform/mediatek/vcodec/venc/venc_h264_if.c
    > > @@ -612,7 +612,7 @@ static int h264_enc_init(struct mtk_vcodec_ctx
    > *ctx)
    > > inst->ctx = ctx;
    > > inst->vpu_inst.ctx = ctx;
    > > inst->vpu_inst.id = is_ext ? SCP_IPI_VENC_H264 : IPI_VENC_H264;
    > > -inst->hw_base = mtk_vcodec_get_reg_addr(inst->ctx, VENC_SYS);
    > > +inst->hw_base = mtk_vcodec_get_reg_addr(inst->ctx->dev->reg_base,
    > VENC_SYS);
    > >
    > > mtk_vcodec_debug_enter(inst);
    > >
    > > diff --git
    > a/drivers/media/platform/mediatek/vcodec/venc/venc_vp8_if.c
    > b/drivers/media/platform/mediatek/vcodec/venc/venc_vp8_if.c
    > > index 56ce58f761f1..73ebc35d7c99 100644
    > > --- a/drivers/media/platform/mediatek/vcodec/venc/venc_vp8_if.c
    > > +++ b/drivers/media/platform/mediatek/vcodec/venc/venc_vp8_if.c
    > > @@ -336,7 +336,7 @@ static int vp8_enc_init(struct mtk_vcodec_ctx
    > *ctx)
    > > inst->ctx = ctx;
    > > inst->vpu_inst.ctx = ctx;
    > > inst->vpu_inst.id = IPI_VENC_VP8;
    > > -inst->hw_base = mtk_vcodec_get_reg_addr(inst->ctx, VENC_LT_SYS);
    > > +inst->hw_base = mtk_vcodec_get_reg_addr(inst->ctx->dev->reg_base,
    > VENC_LT_SYS);
    > >
    > > mtk_vcodec_debug_enter(inst);
    > >
    >
    >
    \
     
     \ /
      Last update: 2023-06-20 05:00    [W:2.571 / U:0.148 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site