2023-06-17 10:35:12

by Yunfei Dong (董云飞)

[permalink] [raw]
Subject: [PATCH v3,03/11] media: mediatek: vcodec: re-write shared interface

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 <[email protected]>
---
.../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;

/* 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);

#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);

--
2.18.0



2023-06-19 15:01:18

by Nicolas Dufresne

[permalink] [raw]
Subject: Re: [PATCH v3,03/11] media: mediatek: vcodec: re-write shared interface


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 <[email protected]>
> ---
> .../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.

>
> /* 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.

>
> #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);
>


2023-06-20 03:03:12

by Yunfei Dong (董云飞)

[permalink] [raw]
Subject: Re: [PATCH v3,03/11] media: mediatek: vcodec: re-write shared interface

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 <[email protected]>
> > ---
> > .../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);
> >
>
>