Re: [PATCH v6,17/24] media: mediatek: vcodec: re-construct h264 driver to support svp mode

[Chen-Yu Tsai]

On Thu, May 16, 2024 at 8:21 PM Yunfei Dong <yunfei.dong@xxxxxxxxxxxx> wrote:
>
> Need secure buffer size to convert secure handle to secure
> pa in optee-os, re-construct the vsi struct to store each
> secure buffer size.
>
> Separate svp and normal wait interrupt condition for svp mode
> waiting hardware interrupt in optee-os.
>
> Signed-off-by: Yunfei Dong <yunfei.dong@xxxxxxxxxxxx>
> ---
>  .../decoder/vdec/vdec_h264_req_multi_if.c     | 261 +++++++++++-------
>  .../mediatek/vcodec/decoder/vdec_msg_queue.c  |   9 +-
>  2 files changed, 168 insertions(+), 102 deletions(-)
>
> diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_h264_req_multi_if.c b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_h264_req_multi_if.c
> index d7fec1887ab5..40836673f7fe 100644
> --- a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_h264_req_multi_if.c
> +++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_h264_req_multi_if.c
> @@ -60,14 +60,36 @@ struct vdec_h264_slice_lat_dec_param {
>   * @crc:               Used to check whether hardware's status is right
>   */
>  struct vdec_h264_slice_info {
> +       u64 wdma_end_addr_offset;
>         u16 nal_info;
>         u16 timeout;
> -       u32 bs_buf_size;
> -       u64 bs_buf_addr;
> -       u64 y_fb_dma;
> -       u64 c_fb_dma;
>         u64 vdec_fb_va;
>         u32 crc[8];
> +       u32 reserved;
> +};
> +
> +/*
> + * struct vdec_h264_slice_mem - memory address and size
> + */
> +struct vdec_h264_slice_mem {
> +       union {
> +               u64 buf;
> +               u64 dma_addr;
> +       };
> +       union {
> +               size_t size;
> +               u64 dma_addr_end;
> +       };
> +};
> +
> +/**
> + * struct vdec_h264_slice_fb - frame buffer for decoding
> + * @y:  current y buffer address info
> + * @c:  current c buffer address info
> + */
> +struct vdec_h264_slice_fb {
> +       struct vdec_h264_slice_mem y;
> +       struct vdec_h264_slice_mem c;
>  };
>
>  /**
> @@ -92,18 +114,16 @@ struct vdec_h264_slice_info {
>   */
>  struct vdec_h264_slice_vsi {
>         /* LAT dec addr */
> -       u64 wdma_err_addr;
> -       u64 wdma_start_addr;
> -       u64 wdma_end_addr;
> -       u64 slice_bc_start_addr;
> -       u64 slice_bc_end_addr;
> -       u64 row_info_start_addr;
> -       u64 row_info_end_addr;
> -       u64 trans_start;
> -       u64 trans_end;
> -       u64 wdma_end_addr_offset;
> +       struct vdec_h264_slice_mem bs;
> +       struct vdec_h264_slice_fb fb;
>
> -       u64 mv_buf_dma[H264_MAX_MV_NUM];
> +       struct vdec_h264_slice_mem ube;
> +       struct vdec_h264_slice_mem trans;
> +       struct vdec_h264_slice_mem row_info;
> +       struct vdec_h264_slice_mem err_map;
> +       struct vdec_h264_slice_mem slice_bc;
> +
> +       struct vdec_h264_slice_mem mv_buf_dma[H264_MAX_MV_NUM];
>         struct vdec_h264_slice_info dec;
>         struct vdec_h264_slice_lat_dec_param h264_slice_params;
>  };

Hard NAK.

You are changing the interface with the firmware without any backward
compatibility. This breaks H.264 decoding on other platforms that use
this code path, including MT8192, MT8195, and MT8186. You cannot just
consider the current platform you are working on.

This is already showing in our downstream test results for Asurada / MT8192
on v6.1, same branch as MT8188, which has these patches applied.

The kernel needs to be able to run with older firmware. The firmware
needs to signal that it wants the new layout either with a version
number (which probably won't work with SCP here) or with capability
flags, like with 4K or AV1 or HEVC support we had before. The kernel
driver sees the capability flag and uses the new data structures.

Or you make changes to the layout in some backward compatible way, like
only expanding the data structure and keeping all the old fields, and
duplicating fields for new structures you add.


Regards
ChenYu

> @@ -392,6 +412,100 @@ static void vdec_h264_slice_get_crop_info(struct vdec_h264_slice_inst *inst,
>                        cr->left, cr->top, cr->width, cr->height);
>  }
>
> +static void vdec_h264_slice_setup_lat_buffer(struct vdec_h264_slice_inst *inst,
> +                                            struct mtk_vcodec_mem *bs,
> +                                            struct vdec_lat_buf *lat_buf)
> +{
> +       struct mtk_vcodec_mem *mem;
> +       int i;
> +
> +       inst->vsi->bs.dma_addr = (u64)bs->dma_addr;
> +       inst->vsi->bs.size = bs->size;
> +
> +       for (i = 0; i < H264_MAX_MV_NUM; i++) {
> +               mem = &inst->mv_buf[i];
> +               inst->vsi->mv_buf_dma[i].dma_addr = mem->dma_addr;
> +               inst->vsi->mv_buf_dma[i].size = mem->size;
> +       }
> +       inst->vsi->ube.dma_addr = lat_buf->ctx->msg_queue.wdma_addr.dma_addr;
> +       inst->vsi->ube.size = lat_buf->ctx->msg_queue.wdma_addr.size;
> +
> +       inst->vsi->row_info.dma_addr = 0;
> +       inst->vsi->row_info.size = 0;
> +
> +       inst->vsi->err_map.dma_addr = lat_buf->wdma_err_addr.dma_addr;
> +       inst->vsi->err_map.size = lat_buf->wdma_err_addr.size;
> +
> +       inst->vsi->slice_bc.dma_addr = lat_buf->slice_bc_addr.dma_addr;
> +       inst->vsi->slice_bc.size = lat_buf->slice_bc_addr.size;
> +
> +       inst->vsi->trans.dma_addr_end = inst->ctx->msg_queue.wdma_rptr_addr;
> +       inst->vsi->trans.dma_addr = inst->ctx->msg_queue.wdma_wptr_addr;
> +}
> +
> +static int vdec_h264_slice_setup_core_buffer(struct vdec_h264_slice_inst *inst,
> +                                            struct vdec_h264_slice_share_info *share_info,
> +                                            struct vdec_lat_buf *lat_buf)
> +{
> +       struct mtk_vcodec_mem *mem;
> +       struct mtk_vcodec_dec_ctx *ctx = inst->ctx;
> +       struct vb2_v4l2_buffer *vb2_v4l2;
> +       struct vdec_fb *fb;
> +       u64 y_fb_dma, c_fb_dma = 0;
> +       int i;
> +
> +       fb = ctx->dev->vdec_pdata->get_cap_buffer(ctx);
> +       if (!fb) {
> +               mtk_vdec_err(ctx, "fb buffer is NULL");
> +               return -EBUSY;
> +       }
> +
> +       y_fb_dma = (u64)fb->base_y.dma_addr;
> +       if (!ctx->is_secure_playback) {
> +               if (ctx->q_data[MTK_Q_DATA_DST].fmt->num_planes == 1)
> +                       c_fb_dma =
> +                               y_fb_dma + inst->ctx->picinfo.buf_w * inst->ctx->picinfo.buf_h;
> +               else
> +                       c_fb_dma = (u64)fb->base_c.dma_addr;
> +       }
> +
> +       mtk_vdec_debug(ctx, "[h264-core] y/c addr = 0x%llx 0x%llx", y_fb_dma, c_fb_dma);
> +
> +       inst->vsi_core->fb.y.dma_addr = y_fb_dma;
> +       inst->vsi_core->fb.y.size = ctx->picinfo.fb_sz[0];
> +       inst->vsi_core->fb.c.dma_addr = c_fb_dma;
> +       inst->vsi_core->fb.c.size = ctx->picinfo.fb_sz[1];
> +
> +       inst->vsi_core->dec.vdec_fb_va = (unsigned long)fb;
> +       inst->vsi_core->dec.nal_info = share_info->nal_info;
> +
> +       inst->vsi_core->ube.dma_addr = lat_buf->ctx->msg_queue.wdma_addr.dma_addr;
> +       inst->vsi_core->ube.size = lat_buf->ctx->msg_queue.wdma_addr.size;
> +
> +       inst->vsi_core->err_map.dma_addr = lat_buf->wdma_err_addr.dma_addr;
> +       inst->vsi_core->err_map.size = lat_buf->wdma_err_addr.size;
> +
> +       inst->vsi_core->slice_bc.dma_addr = lat_buf->slice_bc_addr.dma_addr;
> +       inst->vsi_core->slice_bc.size = lat_buf->slice_bc_addr.size;
> +
> +       inst->vsi_core->row_info.dma_addr = 0;
> +       inst->vsi_core->row_info.size = 0;
> +
> +       inst->vsi_core->trans.dma_addr = share_info->trans_start;
> +       inst->vsi_core->trans.dma_addr_end = share_info->trans_end;
> +
> +       for (i = 0; i < H264_MAX_MV_NUM; i++) {
> +               mem = &inst->mv_buf[i];
> +               inst->vsi_core->mv_buf_dma[i].dma_addr = mem->dma_addr;
> +               inst->vsi_core->mv_buf_dma[i].size = mem->size;
> +       }
> +
> +       vb2_v4l2 = v4l2_m2m_next_dst_buf(ctx->m2m_ctx);
> +       v4l2_m2m_buf_copy_metadata(&lat_buf->ts_info, vb2_v4l2, true);
> +
> +       return 0;
> +}
> +
>  static int vdec_h264_slice_init(struct mtk_vcodec_dec_ctx *ctx)
>  {
>         struct vdec_h264_slice_inst *inst;
> @@ -457,64 +571,22 @@ static void vdec_h264_slice_deinit(void *h_vdec)
>
>  static int vdec_h264_slice_core_decode(struct vdec_lat_buf *lat_buf)
>  {
> -       struct vdec_fb *fb;
> -       u64 vdec_fb_va;
> -       u64 y_fb_dma, c_fb_dma;
> -       int err, timeout, i;
> +       int err, timeout;
>         struct mtk_vcodec_dec_ctx *ctx = lat_buf->ctx;
>         struct vdec_h264_slice_inst *inst = ctx->drv_handle;
> -       struct vb2_v4l2_buffer *vb2_v4l2;
>         struct vdec_h264_slice_share_info *share_info = lat_buf->private_data;
> -       struct mtk_vcodec_mem *mem;
>         struct vdec_vpu_inst *vpu = &inst->vpu;
>
>         mtk_vdec_debug(ctx, "[h264-core] vdec_h264 core decode");
>         memcpy(&inst->vsi_core->h264_slice_params, &share_info->h264_slice_params,
>                sizeof(share_info->h264_slice_params));
>
> -       fb = ctx->dev->vdec_pdata->get_cap_buffer(ctx);
> -       if (!fb) {
> -               err = -EBUSY;
> -               mtk_vdec_err(ctx, "fb buffer is NULL");
> +       err = vdec_h264_slice_setup_core_buffer(inst, share_info, lat_buf);
> +       if (err)
>                 goto vdec_dec_end;
> -       }
> -
> -       vdec_fb_va = (unsigned long)fb;
> -       y_fb_dma = (u64)fb->base_y.dma_addr;
> -       if (ctx->q_data[MTK_Q_DATA_DST].fmt->num_planes == 1)
> -               c_fb_dma =
> -                       y_fb_dma + inst->ctx->picinfo.buf_w * inst->ctx->picinfo.buf_h;
> -       else
> -               c_fb_dma = (u64)fb->base_c.dma_addr;
> -
> -       mtk_vdec_debug(ctx, "[h264-core] y/c addr = 0x%llx 0x%llx", y_fb_dma, c_fb_dma);
> -
> -       inst->vsi_core->dec.y_fb_dma = y_fb_dma;
> -       inst->vsi_core->dec.c_fb_dma = c_fb_dma;
> -       inst->vsi_core->dec.vdec_fb_va = vdec_fb_va;
> -       inst->vsi_core->dec.nal_info = share_info->nal_info;
> -       inst->vsi_core->wdma_start_addr =
> -               lat_buf->ctx->msg_queue.wdma_addr.dma_addr;
> -       inst->vsi_core->wdma_end_addr =
> -               lat_buf->ctx->msg_queue.wdma_addr.dma_addr +
> -               lat_buf->ctx->msg_queue.wdma_addr.size;
> -       inst->vsi_core->wdma_err_addr = lat_buf->wdma_err_addr.dma_addr;
> -       inst->vsi_core->slice_bc_start_addr = lat_buf->slice_bc_addr.dma_addr;
> -       inst->vsi_core->slice_bc_end_addr = lat_buf->slice_bc_addr.dma_addr +
> -               lat_buf->slice_bc_addr.size;
> -       inst->vsi_core->trans_start = share_info->trans_start;
> -       inst->vsi_core->trans_end = share_info->trans_end;
> -       for (i = 0; i < H264_MAX_MV_NUM; i++) {
> -               mem = &inst->mv_buf[i];
> -               inst->vsi_core->mv_buf_dma[i] = mem->dma_addr;
> -       }
> -
> -       vb2_v4l2 = v4l2_m2m_next_dst_buf(ctx->m2m_ctx);
> -       v4l2_m2m_buf_copy_metadata(&lat_buf->ts_info, vb2_v4l2, true);
>
>         vdec_h264_slice_fill_decode_reflist(inst, &inst->vsi_core->h264_slice_params,
>                                             share_info);
> -
>         err = vpu_dec_core(vpu);
>         if (err) {
>                 mtk_vdec_err(ctx, "core decode err=%d", err);
> @@ -573,12 +645,11 @@ static int vdec_h264_slice_lat_decode(void *h_vdec, struct mtk_vcodec_mem *bs,
>         struct vdec_h264_slice_inst *inst = h_vdec;
>         struct vdec_vpu_inst *vpu = &inst->vpu;
>         struct mtk_video_dec_buf *src_buf_info;
> -       int nal_start_idx, err, timeout = 0, i;
> +       int nal_start_idx, err, timeout = 0;
>         unsigned int data[2];
>         struct vdec_lat_buf *lat_buf;
>         struct vdec_h264_slice_share_info *share_info;
>         unsigned char *buf;
> -       struct mtk_vcodec_mem *mem;
>
>         if (vdec_msg_queue_init(&inst->ctx->msg_queue, inst->ctx,
>                                 vdec_h264_slice_core_decode,
> @@ -617,11 +688,9 @@ static int vdec_h264_slice_lat_decode(void *h_vdec, struct mtk_vcodec_mem *bs,
>         if (err)
>                 goto err_free_fb_out;
>
> -       vdec_h264_insert_startcode(inst->ctx->dev, buf, &bs->size,
> -                                  &share_info->h264_slice_params.pps);
> -
> -       inst->vsi->dec.bs_buf_addr = (uint64_t)bs->dma_addr;
> -       inst->vsi->dec.bs_buf_size = bs->size;
> +       if (!inst->ctx->is_secure_playback)
> +               vdec_h264_insert_startcode(inst->ctx->dev, buf, &bs->size,
> +                                          &share_info->h264_slice_paramspps);
>
>         *res_chg = inst->resolution_changed;
>         if (inst->resolution_changed) {
> @@ -634,38 +703,27 @@ static int vdec_h264_slice_lat_decode(void *h_vdec, struct mtk_vcodec_mem *bs,
>                 }
>                 inst->resolution_changed = false;
>         }
> -       for (i = 0; i < H264_MAX_MV_NUM; i++) {
> -               mem = &inst->mv_buf[i];
> -               inst->vsi->mv_buf_dma[i] = mem->dma_addr;
> -       }
> -       inst->vsi->wdma_start_addr = lat_buf->ctx->msg_queue.wdma_addr.dma_addr;
> -       inst->vsi->wdma_end_addr = lat_buf->ctx->msg_queue.wdma_addr.dma_addr +
> -               lat_buf->ctx->msg_queue.wdma_addr.size;
> -       inst->vsi->wdma_err_addr = lat_buf->wdma_err_addr.dma_addr;
> -       inst->vsi->slice_bc_start_addr = lat_buf->slice_bc_addr.dma_addr;
> -       inst->vsi->slice_bc_end_addr = lat_buf->slice_bc_addr.dma_addr +
> -               lat_buf->slice_bc_addr.size;
> -
> -       inst->vsi->trans_end = inst->ctx->msg_queue.wdma_rptr_addr;
> -       inst->vsi->trans_start = inst->ctx->msg_queue.wdma_wptr_addr;
> -       mtk_vdec_debug(inst->ctx, "lat:trans(0x%llx 0x%llx) err:0x%llx",
> -                      inst->vsi->wdma_start_addr,
> -                      inst->vsi->wdma_end_addr,
> -                      inst->vsi->wdma_err_addr);
> -
> -       mtk_vdec_debug(inst->ctx, "slice(0x%llx 0x%llx) rprt((0x%llx 0x%llx))",
> -                      inst->vsi->slice_bc_start_addr,
> -                      inst->vsi->slice_bc_end_addr,
> -                      inst->vsi->trans_start,
> -                      inst->vsi->trans_end);
> +
> +       vdec_h264_slice_setup_lat_buffer(inst, bs, lat_buf);
> +       mtk_vdec_debug(inst->ctx, "lat:trans(0x%llx 0x%lx) err:0x%llx",
> +                      inst->vsi->ube.dma_addr, (unsigned long)inst->vsi->ube.size,
> +                      inst->vsi->err_map.dma_addr);
> +
> +       mtk_vdec_debug(inst->ctx, "slice(0x%llx 0x%lx) rprt((0x%llx 0x%llx))",
> +                      inst->vsi->slice_bc.dma_addr, (unsigned long)inst->vsi->slice_bc.size,
> +                      inst->vsi->trans.dma_addr, inst->vsi->trans.dma_addr_end);
>         err = vpu_dec_start(vpu, data, 2);
>         if (err) {
>                 mtk_vdec_debug(inst->ctx, "lat decode err: %d", err);
>                 goto err_free_fb_out;
>         }
>
> -       share_info->trans_end = inst->ctx->msg_queue.wdma_addr.dma_addr +
> -               inst->vsi->wdma_end_addr_offset;
> +       if (inst->ctx->is_secure_playback)
> +               share_info->trans_end = inst->vsi->dec.wdma_end_addr_offset;
> +       else
> +               share_info->trans_end = inst->ctx->msg_queue.wdma_addr.dma_addr +
> +                       inst->vsi->dec.wdma_end_addr_offset;
> +
>         share_info->trans_start = inst->ctx->msg_queue.wdma_wptr_addr;
>         share_info->nal_info = inst->vsi->dec.nal_info;
>
> @@ -691,8 +749,11 @@ static int vdec_h264_slice_lat_decode(void *h_vdec, struct mtk_vcodec_mem *bs,
>                 return -EINVAL;
>         }
>
> -       share_info->trans_end = inst->ctx->msg_queue.wdma_addr.dma_addr +
> -               inst->vsi->wdma_end_addr_offset;
> +       if (inst->ctx->is_secure_playback)
> +               share_info->trans_end = inst->vsi->dec.wdma_end_addr_offset;
> +       else
> +               share_info->trans_end = inst->ctx->msg_queue.wdma_addr.dma_addr +
> +                       inst->vsi->dec.wdma_end_addr_offset;
>         vdec_msg_queue_update_ube_wptr(&lat_buf->ctx->msg_queue, share_info->trans_end);
>
>         if (!IS_VDEC_INNER_RACING(inst->ctx->dev->dec_capability)) {
> @@ -737,10 +798,10 @@ static int vdec_h264_slice_single_decode(void *h_vdec, struct mtk_vcodec_mem *bs
>         mtk_vdec_debug(inst->ctx, "[h264-dec] [%d] y_dma=%llx c_dma=%llx",
>                        inst->ctx->decoded_frame_cnt, y_fb_dma, c_fb_dma);
>
> -       inst->vsi_ctx.dec.bs_buf_addr = (u64)bs->dma_addr;
> -       inst->vsi_ctx.dec.bs_buf_size = bs->size;
> -       inst->vsi_ctx.dec.y_fb_dma = y_fb_dma;
> -       inst->vsi_ctx.dec.c_fb_dma = c_fb_dma;
> +       inst->vsi_ctx.bs.dma_addr = (u64)bs->dma_addr;
> +       inst->vsi_ctx.bs.size = bs->size;
> +       inst->vsi_ctx.fb.y.dma_addr = y_fb_dma;
> +       inst->vsi_ctx.fb.c.dma_addr = c_fb_dma;
>         inst->vsi_ctx.dec.vdec_fb_va = (u64)(uintptr_t)fb;
>
>         v4l2_m2m_buf_copy_metadata(&src_buf_info->m2m_buf.vb,
> @@ -770,7 +831,7 @@ static int vdec_h264_slice_single_decode(void *h_vdec, struct mtk_vcodec_mem *bs
>
>                 for (i = 0; i < H264_MAX_MV_NUM; i++) {
>                         mem = &inst->mv_buf[i];
> -                       inst->vsi_ctx.mv_buf_dma[i] = mem->dma_addr;
> +                       inst->vsi_ctx.mv_buf_dma[i].dma_addr = mem->dma_addr;
>                 }
>         }
>
> diff --git a/drivers/media/platform/mediatek/vcodec/decoder/vdec_msg_queue.c b/drivers/media/platform/mediatek/vcodec/decoder/vdec_msg_queue.c
> index f283c4703dc6..c1310176ae05 100644
> --- a/drivers/media/platform/mediatek/vcodec/decoder/vdec_msg_queue.c
> +++ b/drivers/media/platform/mediatek/vcodec/decoder/vdec_msg_queue.c
> @@ -308,8 +308,13 @@ int vdec_msg_queue_init(struct vdec_msg_queue *msg_queue,
>                 msg_queue->wdma_addr.size = 0;
>                 return -ENOMEM;
>         }
> -       msg_queue->wdma_rptr_addr = msg_queue->wdma_addr.dma_addr;
> -       msg_queue->wdma_wptr_addr = msg_queue->wdma_addr.dma_addr;
> +       if (ctx->is_secure_playback) {
> +               msg_queue->wdma_rptr_addr = 0;
> +               msg_queue->wdma_wptr_addr = 0;
> +       } else {
> +               msg_queue->wdma_rptr_addr = msg_queue->wdma_addr.dma_addr;
> +               msg_queue->wdma_wptr_addr = msg_queue->wdma_addr.dma_addr;
> +       }
>
>         msg_queue->empty_lat_buf.ctx = ctx;
>         msg_queue->empty_lat_buf.core_decode = NULL;
> --
> 2.25.1
>