by Alexandre Courbot

[permalink] [raw]

Subject: Re: [PATCH v3 06/15] media: mtk-vcodec: vdec: support stateless H.264 decoding

On Tue, Mar 16, 2021 at 12:21 AM Nicolas Dufresne <[email protected]> wrote:
>
> Le lundi 15 mars 2021 à 20:28 +0900, Alexandre Courbot a écrit :
> > Hi Ezequiel,
> >
> > On Thu, Mar 4, 2021 at 6:47 AM Ezequiel Garcia
> > <[email protected]> wrote:
> > >
> > > Hi Alex,
> > >
> > > Thanks for the patch.
> > >
> > > On Fri, 26 Feb 2021 at 07:06, Alexandre Courbot <[email protected]>
> > > wrote:
> > > >
> > > > From: Yunfei Dong <[email protected]>
> > > >
> > > > Add support for H.264 decoding using the stateless API, as supported by
> > > > MT8183. This support takes advantage of the V4L2 H.264 reference list
> > > > builders.
> > > >
> > > > Signed-off-by: Yunfei Dong <[email protected]>
> > > > [acourbot: refactor, cleanup and split]
> > > > Co-developed-by: Alexandre Courbot <[email protected]>
> > > > Signed-off-by: Alexandre Courbot <[email protected]>
> > > > ---
> > > > drivers/media/platform/Kconfig | 1 +
> > > > drivers/media/platform/mtk-vcodec/Makefile | 1 +
> > > > .../mtk-vcodec/vdec/vdec_h264_req_if.c | 807 ++++++++++++++++++
> > > > .../media/platform/mtk-vcodec/vdec_drv_if.c | 3 +
> > > > .../media/platform/mtk-vcodec/vdec_drv_if.h | 1 +
> > > > 5 files changed, 813 insertions(+)
> > > > create mode 100644 drivers/media/platform/mtk-
> > > > vcodec/vdec/vdec_h264_req_if.c
> > > >
> > > > diff --git a/drivers/media/platform/Kconfig
> > > > b/drivers/media/platform/Kconfig
> > > > index fd1831e97b22..c27db5643712 100644
> > > > --- a/drivers/media/platform/Kconfig
> > > > +++ b/drivers/media/platform/Kconfig
> > > > @@ -295,6 +295,7 @@ config VIDEO_MEDIATEK_VCODEC
> > > > select V4L2_MEM2MEM_DEV
> > > > select VIDEO_MEDIATEK_VCODEC_VPU if VIDEO_MEDIATEK_VPU
> > > > select VIDEO_MEDIATEK_VCODEC_SCP if MTK_SCP
> > > > + select V4L2_H264
> > > > help
> > > > Mediatek video codec driver provides HW capability to
> > > > encode and decode in a range of video formats on MT8173
> > > > diff --git a/drivers/media/platform/mtk-vcodec/Makefile
> > > > b/drivers/media/platform/mtk-vcodec/Makefile
> > > > index 4ba93d838ab6..ca8e9e7a9c4e 100644
> > > > --- a/drivers/media/platform/mtk-vcodec/Makefile
> > > > +++ b/drivers/media/platform/mtk-vcodec/Makefile
> > > > @@ -7,6 +7,7 @@ obj-$(CONFIG_VIDEO_MEDIATEK_VCODEC) += mtk-vcodec-dec.o \
> > > > mtk-vcodec-dec-y := vdec/vdec_h264_if.o \
> > > > vdec/vdec_vp8_if.o \
> > > > vdec/vdec_vp9_if.o \
> > > > + vdec/vdec_h264_req_if.o \
> > > > mtk_vcodec_dec_drv.o \
> > > > vdec_drv_if.o \
> > > > vdec_vpu_if.o \
> > > > diff --git a/drivers/media/platform/mtk-vcodec/vdec/vdec_h264_req_if.c
> > > > b/drivers/media/platform/mtk-vcodec/vdec/vdec_h264_req_if.c
> > > > new file mode 100644
> > > > index 000000000000..2fbbfbbcfbec
> > > > --- /dev/null
> > > > +++ b/drivers/media/platform/mtk-vcodec/vdec/vdec_h264_req_if.c
> > > > @@ -0,0 +1,807 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +
> > > > +#include <linux/module.h>
> > > > +#include <linux/slab.h>
> > > > +#include <media/v4l2-mem2mem.h>
> > > > +#include <media/v4l2-h264.h>
> > > > +#include <media/videobuf2-dma-contig.h>
> > > > +
> > > > +#include "../vdec_drv_if.h"
> > > > +#include "../mtk_vcodec_util.h"
> > > > +#include "../mtk_vcodec_dec.h"
> > > > +#include "../mtk_vcodec_intr.h"
> > > > +#include "../vdec_vpu_if.h"
> > > > +#include "../vdec_drv_base.h"
> > > > +
> > > > +#define NAL_NON_IDR_SLICE 0x01
> > > > +#define NAL_IDR_SLICE 0x05
> > > > +#define NAL_H264_PPS 0x08
> > >
> > > Not used?
> > >
> > > > +#define NAL_TYPE(value) ((value) & 0x1F)
> > > > +
> > >
> > > I believe you may not need the NAL type.
> >
> > True, removed this block of defines.
> >
> > >
> > > > +#define BUF_PREDICTION_SZ (64 * 4096)
> > > > +#define MB_UNIT_LEN 16
> > > > +
> > > > +/* get used parameters for sps/pps */
> > > > +#define GET_MTK_VDEC_FLAG(cond, flag) \
> > > > + { dst_param->cond = ((src_param->flags & flag) ? (1) : (0)); }
> > > > +#define GET_MTK_VDEC_PARAM(param) \
> > > > + { dst_param->param = src_param->param; }
> > > > +/* motion vector size (bytes) for every macro block */
> > > > +#define HW_MB_STORE_SZ 64
> > > > +
> > > > +#define H264_MAX_FB_NUM 17
> > > > +#define H264_MAX_MV_NUM 32
> > > > +#define HDR_PARSING_BUF_SZ 1024
> > > > +
> > > > +/**
> > > > + * struct mtk_h264_dpb_info - h264 dpb information
> > > > + * @y_dma_addr: Y bitstream physical address
> > > > + * @c_dma_addr: CbCr bitstream physical address
> > > > + * @reference_flag: reference picture flag (short/long term reference
> > > > picture)
> > > > + * @field: field picture flag
> > > > + */
> > > > +struct mtk_h264_dpb_info {
> > > > + dma_addr_t y_dma_addr;
> > > > + dma_addr_t c_dma_addr;
> > > > + int reference_flag;
> > > > + int field;
> > > > +};
> > > > +
> > > > +/**
> > > > + * struct mtk_h264_sps_param - parameters for sps
> > > > + */
> > > > +struct mtk_h264_sps_param {
> > > > + unsigned char chroma_format_idc;
> > > > + unsigned char bit_depth_luma_minus8;
> > > > + unsigned char bit_depth_chroma_minus8;
> > > > + unsigned char log2_max_frame_num_minus4;
> > > > + unsigned char pic_order_cnt_type;
> > > > + unsigned char log2_max_pic_order_cnt_lsb_minus4;
> > > > + unsigned char max_num_ref_frames;
> > > > + unsigned char separate_colour_plane_flag;
> > > > + unsigned short pic_width_in_mbs_minus1;
> > > > + unsigned short pic_height_in_map_units_minus1;
> > > > + unsigned int max_frame_nums;
> > > > + unsigned char qpprime_y_zero_transform_bypass_flag;
> > > > + unsigned char delta_pic_order_always_zero_flag;
> > > > + unsigned char frame_mbs_only_flag;
> > > > + unsigned char mb_adaptive_frame_field_flag;
> > > > + unsigned char direct_8x8_inference_flag;
> > > > + unsigned char reserved[3];
> > > > +};
> > > > +
> > > > +/**
> > > > + * struct mtk_h264_pps_param - parameters for pps
> > > > + */
> > > > +struct mtk_h264_pps_param {
> > > > + unsigned char num_ref_idx_l0_default_active_minus1;
> > > > + unsigned char num_ref_idx_l1_default_active_minus1;
> > > > + unsigned char weighted_bipred_idc;
> > > > + char pic_init_qp_minus26;
> > > > + char chroma_qp_index_offset;
> > > > + char second_chroma_qp_index_offset;
> > > > + unsigned char entropy_coding_mode_flag;
> > > > + unsigned char pic_order_present_flag;
> > > > + unsigned char deblocking_filter_control_present_flag;
> > > > + unsigned char constrained_intra_pred_flag;
> > > > + unsigned char weighted_pred_flag;
> > > > + unsigned char redundant_pic_cnt_present_flag;
> > > > + unsigned char transform_8x8_mode_flag;
> > > > + unsigned char scaling_matrix_present_flag;
> > > > + unsigned char reserved[2];
> > > > +};
> > > > +
> > > > +struct slice_api_h264_scaling_matrix {
> > >
> > > Equal to v4l2_ctrl_h264_scaling_matrix ?
> > > Well I guess you don't want to mix a hardware-specific
> > > thing with the V4L2 API maybe.
> >
> > That's the idea. Although the layout match and the ABI is now stable,
> > I think this communicates better the fact that this is a firmware
> > structure.
> >
> > >
> > > > + unsigned char scaling_list_4x4[6][16];
> > > > + unsigned char scaling_list_8x8[6][64];
> > > > +};
> > > > +
> > > > +struct slice_h264_dpb_entry {
> > > > + unsigned long long reference_ts;
> > > > + unsigned short frame_num;
> > > > + unsigned short pic_num;
> > > > + /* Note that field is indicated by v4l2_buffer.field */
> > > > + int top_field_order_cnt;
> > > > + int bottom_field_order_cnt;
> > > > + unsigned int flags; /* V4L2_H264_DPB_ENTRY_FLAG_* */
> > > > +};
> > > > +
> > > > +/**
> > > > + * struct slice_api_h264_decode_param - parameters for decode.
> > > > + */
> > > > +struct slice_api_h264_decode_param {
> > > > + struct slice_h264_dpb_entry dpb[16];
> > >
> > > V4L2_H264_NUM_DPB_ENTRIES?
> >
> > For the same reason as above (this being a firmware structure), I
> > think it is clearer to not use the kernel definitions here.
> >
> > >
> > > > + unsigned short num_slices;
> > > > + unsigned short nal_ref_idc;
> > > > + unsigned char ref_pic_list_p0[32];
> > > > + unsigned char ref_pic_list_b0[32];
> > > > + unsigned char ref_pic_list_b1[32];
> > >
> > > V4L2_H264_REF_LIST_LEN?
> >
> > Ditto.
> >
> > >
> > > > + int top_field_order_cnt;
> > > > + int bottom_field_order_cnt;
> > > > + unsigned int flags; /* V4L2_H264_DECODE_PARAM_FLAG_* */
> > > > +};
> > > > +
> > > > +/**
> > > > + * struct mtk_h264_dec_slice_param - parameters for decode current frame
> > > > + */
> > > > +struct mtk_h264_dec_slice_param {
> > > > + struct mtk_h264_sps_param sps;
> > > > + struct mtk_h264_pps_param pps;
> > > > + struct slice_api_h264_scaling_matrix scaling_matrix;
> > > > + struct slice_api_h264_decode_param decode_params;
> > > > + struct mtk_h264_dpb_info h264_dpb_info[16];
> > >
> > > V4L2_H264_NUM_DPB_ENTRIES?
> >
> > Ditto.
> >
> > >
> > > > +};
> > > > +
> > > > +/**
> > > > + * struct h264_fb - h264 decode frame buffer information
> > > > + * @vdec_fb_va : virtual address of struct vdec_fb
> > > > + * @y_fb_dma : dma address of Y frame buffer (luma)
> > > > + * @c_fb_dma : dma address of C frame buffer (chroma)
> > > > + * @poc : picture order count of frame buffer
> > > > + * @reserved : for 8 bytes alignment
> > > > + */
> > > > +struct h264_fb {
> > > > + uint64_t vdec_fb_va;
> > > > + uint64_t y_fb_dma;
> > > > + uint64_t c_fb_dma;
> > > > + int32_t poc;
> > > > + uint32_t reserved;
> > > > +};
> > > > +
> > > > +/**
> > > > + * struct vdec_h264_dec_info - decode information
> > > > + * @dpb_sz : decoding picture buffer size
> > > > + * @resolution_changed : resoltion change happen
> > > > + * @realloc_mv_buf : flag to notify driver to re-allocate mv buffer
> > > > + * @cap_num_planes : number planes of capture buffer
> > > > + * @bs_dma : Input bit-stream buffer dma address
> > > > + * @y_fb_dma : Y frame buffer dma address
> > > > + * @c_fb_dma : C frame buffer dma address
> > > > + * @vdec_fb_va : VDEC frame buffer struct virtual address
> > > > + */
> > > > +struct vdec_h264_dec_info {
> > > > + uint32_t dpb_sz;
> > > > + uint32_t resolution_changed;
> > > > + uint32_t realloc_mv_buf;
> > > > + uint32_t cap_num_planes;
> > > > + uint64_t bs_dma;
> > > > + uint64_t y_fb_dma;
> > > > + uint64_t c_fb_dma;
> > > > + uint64_t vdec_fb_va;
> > > > +};
> > > > +
> > > > +/**
> > > > + * struct vdec_h264_vsi - shared memory for decode information exchange
> > > > + * between VPU and Host.
> > > > + * The memory is allocated by VPU then mapping to
> > > > Host
> > > > + * in vpu_dec_init() and freed in vpu_dec_deinit()
> > > > + * by VPU.
> > > > + * AP-W/R : AP is writer/reader on this item
> > > > + * VPU-W/R: VPU is write/reader on this item
> > > > + * @pred_buf_dma : HW working predication buffer dma address (AP-W, VPU-
> > > > R)
> > > > + * @mv_buf_dma : HW working motion vector buffer dma address (AP-W,
> > > > VPU-R)
> > > > + * @dec : decode information (AP-R, VPU-W)
> > > > + * @pic : picture information (AP-R, VPU-W)
> > > > + * @crop : crop information (AP-R, VPU-W)
> > > > + */
> > > > +struct vdec_h264_vsi {
> > > > + uint64_t pred_buf_dma;
> > > > + uint64_t mv_buf_dma[H264_MAX_MV_NUM];
> > > > + struct vdec_h264_dec_info dec;
> > > > + struct vdec_pic_info pic;
> > > > + struct v4l2_rect crop;
> > > > + struct mtk_h264_dec_slice_param h264_slice_params;
> > > > +};
> > > > +
> > > > +/**
> > > > + * struct vdec_h264_slice_inst - h264 decoder instance
> > > > + * @num_nalu : how many nalus be decoded
> > > > + * @ctx : point to mtk_vcodec_ctx
> > > > + * @pred_buf : HW working predication buffer
> > > > + * @mv_buf : HW working motion vector buffer
> > > > + * @vpu : VPU instance
> > > > + * @vsi_ctx : Local VSI data for this decoding context
> > > > + */
> > > > +struct vdec_h264_slice_inst {
> > > > + unsigned int num_nalu;
> > > > + struct mtk_vcodec_ctx *ctx;
> > > > + struct mtk_vcodec_mem pred_buf;
> > > > + struct mtk_vcodec_mem mv_buf[H264_MAX_MV_NUM];
> > > > + struct vdec_vpu_inst vpu;
> > > > + struct vdec_h264_vsi vsi_ctx;
> > > > + struct mtk_h264_dec_slice_param h264_slice_param;
> > > > +
> > > > + struct v4l2_h264_dpb_entry dpb[16];
> > > > +};
> > > > +
> > > > +static void *get_ctrl_ptr(struct mtk_vcodec_ctx *ctx,
> > > > + int id)
> > > > +{
> > > > + struct v4l2_ctrl *ctrl = v4l2_ctrl_find(&ctx->ctrl_hdl, id);
> > > > +
> > > > + return ctrl->p_cur.p;
> > > > +}
> > > > +
> > > > +static void get_h264_dpb_list(struct vdec_h264_slice_inst *inst,
> > > > + struct mtk_h264_dec_slice_param
> > > > *slice_param)
> > > > +{
> > > > + struct vb2_queue *vq;
> > > > + struct vb2_buffer *vb;
> > > > + struct vb2_v4l2_buffer *vb2_v4l2;
> > > > + u64 index;
> > > > +
> > > > + vq = v4l2_m2m_get_vq(inst->ctx->m2m_ctx,
> > > > + V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
> > > > +
> > > > + for (index = 0; index < 16; index++) {
> > >
> > > Ditto, some macro instead of 16.
> >
> > Changed this to use ARRAY_SIZE() which is appropriate here.
> >
> > >
> > > > + const struct slice_h264_dpb_entry *dpb;
> > > > + int vb2_index;
> > > > +
> > > > + dpb = &slice_param->decode_params.dpb[index];
> > > > + if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)) {
> > > > + slice_param->h264_dpb_info[index].reference_flag =
> > > > 0;
> > > > + continue;
> > > > + }
> > > > +
> > > > + vb2_index = vb2_find_timestamp(vq, dpb->reference_ts, 0);
> > > > + if (vb2_index < 0) {
> > > > + mtk_vcodec_err(inst, "Reference invalid:
> > > > dpb_index(%lld) reference_ts(%lld)",
> > > > + index, dpb->reference_ts);
> > > > + continue;
> > > > + }
> > > > + /* 1 for short term reference, 2 for long term reference
> > > > */
> > > > + if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM))
> > > > + slice_param->h264_dpb_info[index].reference_flag =
> > > > 1;
> > > > + else
> > > > + slice_param->h264_dpb_info[index].reference_flag =
> > > > 2;
> > > > +
> > > > + vb = vq->bufs[vb2_index];
> > > > + vb2_v4l2 = container_of(vb, struct vb2_v4l2_buffer,
> > > > vb2_buf);
> > > > + slice_param->h264_dpb_info[index].field = vb2_v4l2->field;
> > > > +
> > > > + slice_param->h264_dpb_info[index].y_dma_addr =
> > > > + vb2_dma_contig_plane_dma_addr(vb, 0);
> > > > + if (inst->ctx->q_data[MTK_Q_DATA_DST].fmt->num_planes ==
> > > > 2) {
> > > > + slice_param->h264_dpb_info[index].c_dma_addr =
> > > > + vb2_dma_contig_plane_dma_addr(vb, 1);
> > > > + }
> > > > + }
> > > > +}
> > > > +
> > > > +static void get_h264_sps_parameters(struct mtk_h264_sps_param *dst_param,
> > > > + const struct v4l2_ctrl_h264_sps *src_param)
> > > > +{
> > > > + GET_MTK_VDEC_PARAM(chroma_format_idc);
> > > > + GET_MTK_VDEC_PARAM(bit_depth_luma_minus8);
> > > > + GET_MTK_VDEC_PARAM(bit_depth_chroma_minus8);
> > > > + GET_MTK_VDEC_PARAM(log2_max_frame_num_minus4);
> > > > + GET_MTK_VDEC_PARAM(pic_order_cnt_type);
> > > > + GET_MTK_VDEC_PARAM(log2_max_pic_order_cnt_lsb_minus4);
> > > > + GET_MTK_VDEC_PARAM(max_num_ref_frames);
> > > > + GET_MTK_VDEC_PARAM(pic_width_in_mbs_minus1);
> > > > + GET_MTK_VDEC_PARAM(pic_height_in_map_units_minus1);
> > > > +
> > > > + GET_MTK_VDEC_FLAG(separate_colour_plane_flag,
> > > > + V4L2_H264_SPS_FLAG_SEPARATE_COLOUR_PLANE);
> > > > + GET_MTK_VDEC_FLAG(qpprime_y_zero_transform_bypass_flag,
> > > > + V4L2_H264_SPS_FLAG_QPPRIME_Y_ZERO_TRANSFORM_BYPASS);
> > > > + GET_MTK_VDEC_FLAG(delta_pic_order_always_zero_flag,
> > > > + V4L2_H264_SPS_FLAG_DELTA_PIC_ORDER_ALWAYS_ZERO);
> > > > + GET_MTK_VDEC_FLAG(frame_mbs_only_flag,
> > > > + V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY);
> > > > + GET_MTK_VDEC_FLAG(mb_adaptive_frame_field_flag,
> > > > + V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD);
> > > > + GET_MTK_VDEC_FLAG(direct_8x8_inference_flag,
> > > > + V4L2_H264_SPS_FLAG_DIRECT_8X8_INFERENCE);
> > > > +}
> > > > +
> > > > +static void get_h264_pps_parameters(struct mtk_h264_pps_param *dst_param,
> > > > + const struct v4l2_ctrl_h264_pps *src_param)
> > > > +{
> > > > + GET_MTK_VDEC_PARAM(num_ref_idx_l0_default_active_minus1);
> > > > + GET_MTK_VDEC_PARAM(num_ref_idx_l1_default_active_minus1);
> > > > + GET_MTK_VDEC_PARAM(weighted_bipred_idc);
> > > > + GET_MTK_VDEC_PARAM(pic_init_qp_minus26);
> > > > + GET_MTK_VDEC_PARAM(chroma_qp_index_offset);
> > > > + GET_MTK_VDEC_PARAM(second_chroma_qp_index_offset);
> > > > +
> > > > + GET_MTK_VDEC_FLAG(entropy_coding_mode_flag,
> > > > + V4L2_H264_PPS_FLAG_ENTROPY_CODING_MODE);
> > > > + GET_MTK_VDEC_FLAG(pic_order_present_flag,
> > > > +
> > > > V4L2_H264_PPS_FLAG_BOTTOM_FIELD_PIC_ORDER_IN_FRAME_PRESENT);
> > > > + GET_MTK_VDEC_FLAG(weighted_pred_flag,
> > > > + V4L2_H264_PPS_FLAG_WEIGHTED_PRED);
> > > > + GET_MTK_VDEC_FLAG(deblocking_filter_control_present_flag,
> > > > + V4L2_H264_PPS_FLAG_DEBLOCKING_FILTER_CONTROL_PRESENT);
> > > > + GET_MTK_VDEC_FLAG(constrained_intra_pred_flag,
> > > > + V4L2_H264_PPS_FLAG_CONSTRAINED_INTRA_PRED);
> > > > + GET_MTK_VDEC_FLAG(redundant_pic_cnt_present_flag,
> > > > + V4L2_H264_PPS_FLAG_REDUNDANT_PIC_CNT_PRESENT);
> > > > + GET_MTK_VDEC_FLAG(transform_8x8_mode_flag,
> > > > + V4L2_H264_PPS_FLAG_TRANSFORM_8X8_MODE);
> > > > + GET_MTK_VDEC_FLAG(scaling_matrix_present_flag,
> > > > + V4L2_H264_PPS_FLAG_SCALING_MATRIX_PRESENT);
> > > > +}
> > > > +
> > > > +static void
> > > > +get_h264_scaling_matrix(struct slice_api_h264_scaling_matrix *dst_matrix,
> > > > + const struct v4l2_ctrl_h264_scaling_matrix
> > > > *src_matrix)
> > > > +{
> > > > + memcpy(dst_matrix->scaling_list_4x4, src_matrix->scaling_list_4x4,
> > > > + sizeof(dst_matrix->scaling_list_4x4));
> > > > +
> > > > + memcpy(dst_matrix->scaling_list_8x8, src_matrix->scaling_list_8x8,
> > > > + sizeof(dst_matrix->scaling_list_8x8));
> > > > +}
> > > > +
> > > > +static void get_h264_decode_parameters(
> > > > + struct slice_api_h264_decode_param *dst_params,
> > > > + const struct v4l2_ctrl_h264_decode_params *src_params,
> > > > + const struct v4l2_h264_dpb_entry dpb[V4L2_H264_NUM_DPB_ENTRIES])
> > > > +{
> > > > + int i;
> > > > +
> > > > + for (i = 0; i < ARRAY_SIZE(dst_params->dpb); i++) {
> > > > + struct slice_h264_dpb_entry *dst_entry = &dst_params-
> > > > >dpb[i];
> > > > + const struct v4l2_h264_dpb_entry *src_entry = &dpb[i];
> > > > +
> > > > + dst_entry->reference_ts = src_entry->reference_ts;
> > > > + dst_entry->frame_num = src_entry->frame_num;
> > > > + dst_entry->pic_num = src_entry->pic_num;
> > > > + dst_entry->top_field_order_cnt = src_entry-
> > > > >top_field_order_cnt;
> > > > + dst_entry->bottom_field_order_cnt =
> > > > + src_entry->bottom_field_order_cnt;
> > > > + dst_entry->flags = src_entry->flags;
> > > > + }
> > > > +
> > > > + // num_slices is a leftover from the old H.264 support and is
> > > > ignored
> > > > + // by the firmware.
> > > > + dst_params->num_slices = 0;
> > > > + dst_params->nal_ref_idc = src_params->nal_ref_idc;
> > > > + dst_params->top_field_order_cnt = src_params->top_field_order_cnt;
> > > > + dst_params->bottom_field_order_cnt = src_params-
> > > > >bottom_field_order_cnt;
> > > > + dst_params->flags = src_params->flags;
> > > > +}
> > > > +
> > > > +static bool dpb_entry_match(const struct v4l2_h264_dpb_entry *a,
> > > > + const struct v4l2_h264_dpb_entry *b)
> > > > +{
> > > > + return a->top_field_order_cnt == b->top_field_order_cnt &&
> > > > + a->bottom_field_order_cnt == b->bottom_field_order_cnt;
> > > > +}
> > > > +
> > > > +/*
> > > > + * Move DPB entries of dec_param that refer to a frame already existing
> > > > in dpb
> > > > + * into the already existing slot in dpb, and move other entries into new
> > > > slots.
> > > > + *
> > > > + * This function is an adaptation of the similarly-named function in
> > > > + * hantro_h264.c.
> > > > + */
> > > > +static void update_dpb(const struct v4l2_ctrl_h264_decode_params
> > > > *dec_param,
> > > > + struct v4l2_h264_dpb_entry *dpb)
> > > > +{
> > > > + DECLARE_BITMAP(new, ARRAY_SIZE(dec_param->dpb)) = { 0, };
> > > > + DECLARE_BITMAP(in_use, ARRAY_SIZE(dec_param->dpb)) = { 0, };
> > > > + DECLARE_BITMAP(used, ARRAY_SIZE(dec_param->dpb)) = { 0, };
> > > > + unsigned int i, j;
> > > > +
> > > > + /* Disable all entries by default, and mark the ones in use. */
> > > > + for (i = 0; i < ARRAY_SIZE(dec_param->dpb); i++) {
> > > > + if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE)
> > > > + set_bit(i, in_use);
> > > > + dpb[i].flags &= ~V4L2_H264_DPB_ENTRY_FLAG_ACTIVE;
> > > > + }
> > > > +
> > > > + /* Try to match new DPB entries with existing ones by their POCs.
> > > > */
> > > > + for (i = 0; i < ARRAY_SIZE(dec_param->dpb); i++) {
> > > > + const struct v4l2_h264_dpb_entry *ndpb = &dec_param-
> > > > >dpb[i];
> > > > +
> > > > + if (!(ndpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
> > > > + continue;
> > > > +
> > > > + /*
> > > > + * To cut off some comparisons, iterate only on target DPB
> > > > + * entries were already used.
> > > > + */
> > > > + for_each_set_bit(j, in_use, ARRAY_SIZE(dec_param->dpb)) {
> > > > + struct v4l2_h264_dpb_entry *cdpb;
> > > > +
> > > > + cdpb = &dpb[j];
> > > > + if (!dpb_entry_match(cdpb, ndpb))
> > > > + continue;
> > > > +
> > > > + *cdpb = *ndpb;
> > > > + set_bit(j, used);
> > > > + /* Don't reiterate on this one. */
> > > > + clear_bit(j, in_use);
> > > > + break;
> > > > + }
> > > > +
> > > > + if (j == ARRAY_SIZE(dec_param->dpb))
> > > > + set_bit(i, new);
> > > > + }
> > > > +
> > > > + /* For entries that could not be matched, use remaining free
> > > > slots. */
> > > > + for_each_set_bit(i, new, ARRAY_SIZE(dec_param->dpb)) {
> > > > + const struct v4l2_h264_dpb_entry *ndpb = &dec_param-
> > > > >dpb[i];
> > > > + struct v4l2_h264_dpb_entry *cdpb;
> > > > +
> > > > + /*
> > > > + * Both arrays are of the same sizes, so there is no way
> > > > + * we can end up with no space in target array, unless
> > > > + * something is buggy.
> > > > + */
> > > > + j = find_first_zero_bit(used, ARRAY_SIZE(dec_param->dpb));
> > > > + if (WARN_ON(j >= ARRAY_SIZE(dec_param->dpb)))
> > > > + return;
> > > > +
> > > > + cdpb = &dpb[j];
> > > > + *cdpb = *ndpb;
> > > > + set_bit(j, used);
> > > > + }
> > > > +}
> > > > +
> > > > +/*
> > > > + * The firmware expects unused reflist entries to have the value 0x20.
> > > > + */
> > > > +static void fixup_ref_list(u8 *ref_list, size_t num_valid)
> > > > +{
> > > > + memset(&ref_list[num_valid], 0x20, 32 - num_valid);
> > > > +}
> > > > +
> > > > +static void get_vdec_decode_parameters(struct vdec_h264_slice_inst *inst)
> > > > +{
> > > > + const struct v4l2_ctrl_h264_decode_params *dec_params =
> > > > + get_ctrl_ptr(inst->ctx,
> > > > V4L2_CID_STATELESS_H264_DECODE_PARAMS);
> > > > + const struct v4l2_ctrl_h264_sps *sps =
> > > > + get_ctrl_ptr(inst->ctx, V4L2_CID_STATELESS_H264_SPS);
> > > > + const struct v4l2_ctrl_h264_pps *pps =
> > > > + get_ctrl_ptr(inst->ctx, V4L2_CID_STATELESS_H264_PPS);
> > > > + const struct v4l2_ctrl_h264_scaling_matrix *scaling_matrix =
> > > > + get_ctrl_ptr(inst->ctx,
> > > > V4L2_CID_STATELESS_H264_SCALING_MATRIX);
> > > > + struct mtk_h264_dec_slice_param *slice_param = &inst-
> > > > >h264_slice_param;
> > > > + struct v4l2_h264_reflist_builder reflist_builder;
> > > > + enum v4l2_field dpb_fields[V4L2_H264_NUM_DPB_ENTRIES];
> > > > + u8 *p0_reflist = slice_param->decode_params.ref_pic_list_p0;
> > > > + u8 *b0_reflist = slice_param->decode_params.ref_pic_list_b0;
> > > > + u8 *b1_reflist = slice_param->decode_params.ref_pic_list_b1;
> > > > + int i;
> > > > +
> > > > + update_dpb(dec_params, inst->dpb);
> > > > +
> > > > + get_h264_sps_parameters(&slice_param->sps, sps);
> > > > + get_h264_pps_parameters(&slice_param->pps, pps);
> > > > + get_h264_scaling_matrix(&slice_param->scaling_matrix,
> > > > scaling_matrix);
> > > > + get_h264_decode_parameters(&slice_param->decode_params,
> > > > dec_params,
> > > > + inst->dpb);
> > > > + get_h264_dpb_list(inst, slice_param);
> > > > +
> > > > + /* Prepare the fields for our reference lists */
> > > > + for (i = 0; i < V4L2_H264_NUM_DPB_ENTRIES; i++)
> > > > + dpb_fields[i] = slice_param->h264_dpb_info[i].field;
> > > > + /* Build the reference lists */
> > > > + v4l2_h264_init_reflist_builder(&reflist_builder, dec_params, sps,
> > > > + inst->dpb);
> > > > + v4l2_h264_build_p_ref_list(&reflist_builder, p0_reflist);
> > > > + v4l2_h264_build_b_ref_lists(&reflist_builder, b0_reflist,
> > > > b1_reflist);
> > > > + /* Adapt the built lists to the firmware's expectations */
> > > > + fixup_ref_list(p0_reflist, reflist_builder.num_valid);
> > > > + fixup_ref_list(b0_reflist, reflist_builder.num_valid);
> > > > + fixup_ref_list(b1_reflist, reflist_builder.num_valid);
> > > > +
> > > > + memcpy(&inst->vsi_ctx.h264_slice_params, slice_param,
> > > > + sizeof(inst->vsi_ctx.h264_slice_params));
> > > > +}
> > > > +
> > > > +static unsigned int get_mv_buf_size(unsigned int width, unsigned int
> > > > height)
> > > > +{
> > > > + int unit_size = (width / MB_UNIT_LEN) * (height / MB_UNIT_LEN) +
> > > > 8;
> > > > +
> > > > + return HW_MB_STORE_SZ * unit_size;
> > > > +}
> > > > +
> > > > +static int allocate_predication_buf(struct vdec_h264_slice_inst *inst)
> > > > +{
> > > > + int err = 0;
> > > > +
> > > > + inst->pred_buf.size = BUF_PREDICTION_SZ;
> > > > + err = mtk_vcodec_mem_alloc(inst->ctx, &inst->pred_buf);
> > > > + if (err) {
> > > > + mtk_vcodec_err(inst, "failed to allocate ppl buf");
> > > > + return err;
> > > > + }
> > > > +
> > > > + inst->vsi_ctx.pred_buf_dma = inst->pred_buf.dma_addr;
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static void free_predication_buf(struct vdec_h264_slice_inst *inst)
> > > > +{
> > > > + struct mtk_vcodec_mem *mem = NULL;
> > > > +
> > > > + mtk_vcodec_debug_enter(inst);
> > > > +
> > > > + inst->vsi_ctx.pred_buf_dma = 0;
> > > > + mem = &inst->pred_buf;
> > > > + if (mem->va)
> > > > + mtk_vcodec_mem_free(inst->ctx, mem);
> > > > +}
> > > > +
> > > > +static int alloc_mv_buf(struct vdec_h264_slice_inst *inst,
> > > > + struct vdec_pic_info *pic)
> > > > +{
> > > > + int i;
> > > > + int err;
> > > > + struct mtk_vcodec_mem *mem = NULL;
> > > > + unsigned int buf_sz = get_mv_buf_size(pic->buf_w, pic->buf_h);
> > > > +
> > > > + mtk_v4l2_debug(3, "size = 0x%lx", buf_sz);
> > > > + for (i = 0; i < H264_MAX_MV_NUM; i++) {
> > > > + mem = &inst->mv_buf[i];
> > > > + if (mem->va)
> > > > + mtk_vcodec_mem_free(inst->ctx, mem);
> > > > + mem->size = buf_sz;
> > > > + err = mtk_vcodec_mem_alloc(inst->ctx, mem);
> > > > + if (err) {
> > > > + mtk_vcodec_err(inst, "failed to allocate mv buf");
> > > > + return err;
> > > > + }
> > > > + inst->vsi_ctx.mv_buf_dma[i] = mem->dma_addr;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static void free_mv_buf(struct vdec_h264_slice_inst *inst)
> > > > +{
> > > > + int i;
> > > > + struct mtk_vcodec_mem *mem = NULL;
> > > > +
> > > > + for (i = 0; i < H264_MAX_MV_NUM; i++) {
> > > > + inst->vsi_ctx.mv_buf_dma[i] = 0;
> > > > + mem = &inst->mv_buf[i];
> > > > + if (mem->va)
> > > > + mtk_vcodec_mem_free(inst->ctx, mem);
> > > > + }
> > > > +}
> > > > +
> > > > +static void get_pic_info(struct vdec_h264_slice_inst *inst,
> > > > + struct vdec_pic_info *pic)
> > > > +{
> > > > + struct mtk_vcodec_ctx *ctx = inst->ctx;
> > > > +
> > > > + ctx->picinfo.buf_w = (ctx->picinfo.pic_w + 15) & 0xFFFFFFF0;
> > > > + ctx->picinfo.buf_h = (ctx->picinfo.pic_h + 31) & 0xFFFFFFE0;
> > > > + ctx->picinfo.fb_sz[0] = ctx->picinfo.buf_w * ctx->picinfo.buf_h;
> > > > + ctx->picinfo.fb_sz[1] = ctx->picinfo.fb_sz[0] >> 1;
> > > > + inst->vsi_ctx.dec.cap_num_planes =
> > > > + ctx->q_data[MTK_Q_DATA_DST].fmt->num_planes;
> > > > +
> > > > + pic = &ctx->picinfo;
> > > > + mtk_vcodec_debug(inst, "pic(%d, %d), buf(%d, %d)",
> > > > + ctx->picinfo.pic_w, ctx->picinfo.pic_h,
> > > > + ctx->picinfo.buf_w, ctx->picinfo.buf_h);
> > > > + mtk_vcodec_debug(inst, "Y/C(%d, %d)", ctx->picinfo.fb_sz[0],
> > > > + ctx->picinfo.fb_sz[1]);
> > > > +
> > > > + if ((ctx->last_decoded_picinfo.pic_w != ctx->picinfo.pic_w) ||
> > > > + (ctx->last_decoded_picinfo.pic_h != ctx->picinfo.pic_h)) {
> > > > + inst->vsi_ctx.dec.resolution_changed = true;
> > > > + if ((ctx->last_decoded_picinfo.buf_w != ctx-
> > > > >picinfo.buf_w) ||
> > > > + (ctx->last_decoded_picinfo.buf_h != ctx-
> > > > >picinfo.buf_h))
> > > > + inst->vsi_ctx.dec.realloc_mv_buf = true;
> > > > +
> > > > + mtk_v4l2_debug(1, "ResChg: (%d %d) : old(%d, %d) ->
> > > > new(%d, %d)",
> > > > + inst->vsi_ctx.dec.resolution_changed,
> > > > + inst->vsi_ctx.dec.realloc_mv_buf,
> > > > + ctx->last_decoded_picinfo.pic_w,
> > > > + ctx->last_decoded_picinfo.pic_h,
> > > > + ctx->picinfo.pic_w, ctx->picinfo.pic_h);
> > > > + }
> > > > +}
> > > > +
> > > > +static void get_crop_info(struct vdec_h264_slice_inst *inst,
> > > > + struct v4l2_rect *cr)
> > > > +{
> > > > + cr->left = inst->vsi_ctx.crop.left;
> > > > + cr->top = inst->vsi_ctx.crop.top;
> > > > + cr->width = inst->vsi_ctx.crop.width;
> > > > + cr->height = inst->vsi_ctx.crop.height;
> > > > +
> > > > + mtk_vcodec_debug(inst, "l=%d, t=%d, w=%d, h=%d",
> > > > + cr->left, cr->top, cr->width, cr->height);
> > > > +}
> > > > +
> > > > +static void get_dpb_size(struct vdec_h264_slice_inst *inst,
> > > > + unsigned int *dpb_sz)
> > > > +{
> > > > + *dpb_sz = inst->vsi_ctx.dec.dpb_sz;
> > > > + mtk_vcodec_debug(inst, "sz=%d", *dpb_sz);
> > > > +}
> > > > +
> > > > +static int vdec_h264_slice_init(struct mtk_vcodec_ctx *ctx)
> > > > +{
> > > > + struct vdec_h264_slice_inst *inst = NULL;
> > > > + int err;
> > > > +
> > > > + inst = kzalloc(sizeof(*inst), GFP_KERNEL);
> > > > + if (!inst)
> > > > + return -ENOMEM;
> > > > +
> > > > + inst->ctx = ctx;
> > > > +
> > > > + inst->vpu.id = SCP_IPI_VDEC_H264;
> > > > + inst->vpu.ctx = ctx;
> > > > +
> > > > + err = vpu_dec_init(&inst->vpu);
> > > > + if (err) {
> > > > + mtk_vcodec_err(inst, "vdec_h264 init err=%d", err);
> > > > + goto error_free_inst;
> > > > + }
> > > > +
> > > > + memcpy(&inst->vsi_ctx, inst->vpu.vsi, sizeof(inst->vsi_ctx));
> > > > + inst->vsi_ctx.dec.resolution_changed = true;
> > > > + inst->vsi_ctx.dec.realloc_mv_buf = true;
> > > > +
> > > > + err = allocate_predication_buf(inst);
> > > > + if (err)
> > > > + goto error_deinit;
> > > > +
> > > > + mtk_vcodec_debug(inst, "struct size = %d,%d,%d,%d\n",
> > > > + sizeof(struct mtk_h264_sps_param),
> > > > + sizeof(struct mtk_h264_pps_param),
> > > > + sizeof(struct mtk_h264_dec_slice_param),
> > > > + sizeof(struct mtk_h264_dpb_info));
> > > > +
> > > > + mtk_vcodec_debug(inst, "H264 Instance >> %p", inst);
> > > > +
> > > > + ctx->drv_handle = inst;
> > > > + return 0;
> > > > +
> > > > +error_deinit:
> > > > + vpu_dec_deinit(&inst->vpu);
> > > > +
> > > > +error_free_inst:
> > > > + kfree(inst);
> > > > + return err;
> > > > +}
> > > > +
> > > > +static void vdec_h264_slice_deinit(void *h_vdec)
> > > > +{
> > > > + struct vdec_h264_slice_inst *inst =
> > > > + (struct vdec_h264_slice_inst *)h_vdec;
> > > > +
> > > > + mtk_vcodec_debug_enter(inst);
> > > > +
> > > > + vpu_dec_deinit(&inst->vpu);
> > > > + free_predication_buf(inst);
> > > > + free_mv_buf(inst);
> > > > +
> > > > + kfree(inst);
> > > > +}
> > > > +
> > > > +static int find_start_code(unsigned char *data, unsigned int data_sz)
> > > > +{
> > > > + if (data_sz > 3 && data[0] == 0 && data[1] == 0 && data[2] == 1)
> > > > + return 3;
> > > > +
> > > > + if (data_sz > 4 && data[0] == 0 && data[1] == 0 && data[2] == 0 &&
> > > > + data[3] == 1)
> > > > + return 4;
> > > > +
> > > > + return -1;
> > > > +}
> > > > +
> > > > +static int vdec_h264_slice_decode(void *h_vdec, struct mtk_vcodec_mem
> > > > *bs,
> > > > + struct vdec_fb *fb, bool *res_chg)
> > > > +{
> > > > + struct vdec_h264_slice_inst *inst =
> > > > + (struct vdec_h264_slice_inst *)h_vdec;
> > > > + struct vdec_vpu_inst *vpu = &inst->vpu;
> > > > + struct mtk_video_dec_buf *src_buf_info;
> > > > + int nal_start_idx = 0, err = 0;
> > > > + uint32_t nal_type, data[2];
> > > > + unsigned char *buf;
> > > > + uint64_t y_fb_dma;
> > > > + uint64_t c_fb_dma;
> > > > +
> > > > + mtk_vcodec_debug(inst, "+ [%d] FB y_dma=%llx c_dma=%llx va=%p",
> > > > + ++inst->num_nalu, y_fb_dma, c_fb_dma, fb);
> > > > +
> > > > + /* bs NULL means flush decoder */
> > > > + if (bs == NULL)
> > > > + return vpu_dec_reset(vpu);
> > > > +
> > > > + src_buf_info = container_of(bs, struct mtk_video_dec_buf,
> > > > bs_buffer);
> > > > +
> > > > + y_fb_dma = fb ? (u64)fb->base_y.dma_addr : 0;
> > > > + c_fb_dma = fb ? (u64)fb->base_c.dma_addr : 0;
> > > > +
> > > > + buf = (unsigned char *)bs->va;
> > >
> > > I can be completely wrong, but it would seem here
> > > is where the CPU mapping is used.
> >
> > I think you're right. :)
> >
> > >
> > > > + nal_start_idx = find_start_code(buf, bs->size);
> > > > + if (nal_start_idx < 0)
> > > > + goto err_free_fb_out;
> > > > +
> > > > + data[0] = bs->size;
> > > > + data[1] = buf[nal_start_idx];
> > > > + nal_type = NAL_TYPE(buf[nal_start_idx]);
> > >
> > > Which seems to be used to parse the NAL type. But shouldn't
> > > you expect here VLC NALUs only?
> > >
> > > I.e. you only get IDR or non-IDR frames, marked with
> > > V4L2_H264_DECODE_PARAM_FLAG_IDR_PIC.
> >
> > Yep, that's true. And as a matter of fact I can remove `nal_type` (and
> > the test using it below) and the driver is just as happy.
> >
> > >
> > > > + mtk_vcodec_debug(inst, "\n + NALU[%d] type %d +\n", inst-
> > > > >num_nalu,
> > > > + nal_type);
> > > > +
> > > > + inst->vsi_ctx.dec.bs_dma = (uint64_t)bs->dma_addr;
> > > > + inst->vsi_ctx.dec.y_fb_dma = y_fb_dma;
> > > > + inst->vsi_ctx.dec.c_fb_dma = c_fb_dma;
> > > > + inst->vsi_ctx.dec.vdec_fb_va = (u64)(uintptr_t)fb;
> > > > +
> > > > + get_vdec_decode_parameters(inst);
> > > > + *res_chg = inst->vsi_ctx.dec.resolution_changed;
> > > > + if (*res_chg) {
> > > > + mtk_vcodec_debug(inst, "- resolution changed -");
> > > > + if (inst->vsi_ctx.dec.realloc_mv_buf) {
> > > > + err = alloc_mv_buf(inst, &(inst->ctx->picinfo));
> > > > + inst->vsi_ctx.dec.realloc_mv_buf = false;
> > > > + if (err)
> > > > + goto err_free_fb_out;
> > > > + }
> > > > + *res_chg = false;
> > > > + }
> > > > +
> > > > + memcpy(inst->vpu.vsi, &inst->vsi_ctx, sizeof(inst->vsi_ctx));
> > > > + err = vpu_dec_start(vpu, data, 2);
> > >
> > > Then it seems this 2-bytes are passed to the firmware. Maybe you
> > > could test if that can be derived without the CPU mapping.
> > > That would allow you to set DMA_ATTR_NO_KERNEL_MAPPING.
> >
> > This one is a bit trickier. It seems the NAL type is passed as part of
> > the decode request to the firmware. Which should be absolutely not
> > needed since the firmware can check this from the buffer itself. Just
> > for fun I have tried setting this parameter unconditionally to 0x1
> > (non-IDR picture) and all I get is green frames with seemingly random
> > garbage. If I set it to 0x5 (IDR picture) I also get green frames with
> > a different kind of garbage, and once every while a properly rendered
> > frame (presumably when it is *really* an IDR frame).
>
> Can't you deduce this from the v4l2_ctrl_h264_slice_params.slice_type ?

This decoder is frame-based, so it doesn't receive a
v4l2_ctrl_h264_slice_params from user-space unfortunately. But
thankfully we can deduce this from the decode params.

2021-03-17 17:20:57

by Nicolas Dufresne

[permalink] [raw]

Subject: Re: [PATCH v3 05/15] media: mtk-vcodec: vdec: support stateless API

Le mercredi 17 mars 2021 à 12:13 +0900, Alexandre Courbot a écrit :
> On Tue, Mar 16, 2021 at 6:45 AM Ezequiel Garcia
> <[email protected]> wrote:
> >
> > Hi Alexandre,
> >
> > On Mon, 15 Mar 2021 at 08:28, Alexandre Courbot <[email protected]>
> > wrote:
> > >
> > > Hi Ezequiel, thanks for the feedback!
> > >
> > > On Thu, Mar 4, 2021 at 6:30 AM Ezequiel Garcia
> > > <[email protected]> wrote:
> > > >
> > > > Hello Alex,
> > > >
> > > > Thanks for the patch.
> > > >
> > > > On Fri, 26 Feb 2021 at 07:06, Alexandre Courbot <[email protected]>
> > > > wrote:
> > > > >
> > > > > From: Yunfei Dong <[email protected]>
> > > > >
> > > > > Support the stateless codec API that will be used by MT8183.
> > > > >
> > > > > Signed-off-by: Yunfei Dong <[email protected]>
> > > > > [acourbot: refactor, cleanup and split]
> > > > > Co-developed-by: Alexandre Courbot <[email protected]>
> > > > > Signed-off-by: Alexandre Courbot <[email protected]>
> > > > > ---
> > > > > drivers/media/platform/mtk-vcodec/Makefile    |   1 +
> > > > > .../platform/mtk-vcodec/mtk_vcodec_dec.c      | 66 ++-
> > > > > .../platform/mtk-vcodec/mtk_vcodec_dec.h      |   9 +-
> > > > > .../mtk-vcodec/mtk_vcodec_dec_stateless.c     | 427
> > > > > ++++++++++++++++++
> > > > > .../platform/mtk-vcodec/mtk_vcodec_drv.h      |   3 +
> > > > > 5 files changed, 503 insertions(+), 3 deletions(-)
> > > > > create mode 100644 drivers/media/platform/mtk-
> > > > > vcodec/mtk_vcodec_dec_stateless.c
> > > > >
> > > > [..]
> > > >
> > > > > +
> > > > > +static const struct mtk_stateless_control mtk_stateless_controls[] =
> > > > > {
> > > > > +       {
> > > > > +               .cfg = {
> > > > > +                       .id = V4L2_CID_STATELESS_H264_SPS,
> > > > > +               },
> > > > > +               .codec_type = V4L2_PIX_FMT_H264_SLICE,
> > > > > +               .needed_in_request = true,
> > > >
> > > > This "needed_in_request" is not really required, as controls
> > > > are not volatile, and their value is stored per-context (per-fd).
> > > >
> > > > It's perfectly valid for an application to pass the SPS control
> > > > at the beginning of the sequence, and then omit it
> > > > in further requests.
> > >
> > > If I understand how v4l2_ctrl_request_hdl_ctrl_find() works with
> > > requests, this boolean only checks that the control has been provided
> > > at least once, and not that it is provided with every request. Without
> > > it we could send a frame to the firmware without e.g. setting an SPS,
> > > which would be a problem.
> > >
> >
> > As Nicolas points out, in V4L2 controls have an initial value,
> > so no control can be unset.
>
> I see. So I guess the expectation is that failure will occur later as
> the firmware reports it cannot decode properly (or returns a corrupted
> frame). Thanks for the precision.

That is identical to userspace passing bad values. We just don't want to force
userspace to pass controls that haven't changed. The control framework isn't
exactly free in CPU time.

>
> >
> > > >
> > > > > +       },
> > > > > +       {
> > > > > +               .cfg = {
> > > > > +                       .id = V4L2_CID_STATELESS_H264_PPS,
> > > > > +               },
> > > > > +               .codec_type = V4L2_PIX_FMT_H264_SLICE,
> > > > > +               .needed_in_request = true,
> > > > > +       },
> > > > > +       {
> > > > > +               .cfg = {
> > > > > +                       .id = V4L2_CID_STATELESS_H264_SCALING_MATRIX,
> > > > > +               },
> > > > > +               .codec_type = V4L2_PIX_FMT_H264_SLICE,
> > > > > +               .needed_in_request = true,
> > > > > +       },
> > > > > +       {
> > > > > +               .cfg = {
> > > > > +                       .id = V4L2_CID_STATELESS_H264_DECODE_PARAMS,
> > > > > +               },
> > > > > +               .codec_type = V4L2_PIX_FMT_H264_SLICE,
> > > > > +               .needed_in_request = true,
> > > > > +       },
> > > > > +       {
> > > > > +               .cfg = {
> > > > > +                       .id = V4L2_CID_MPEG_VIDEO_H264_PROFILE,
> > > > > +                       .def = V4L2_MPEG_VIDEO_H264_PROFILE_MAIN,
> > > > > +                       .max = V4L2_MPEG_VIDEO_H264_PROFILE_HIGH,
> > > > > +                       .menu_skip_mask =
> > > > > +
> > > > > BIT(V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE) |
> > > > > +
> > > > > BIT(V4L2_MPEG_VIDEO_H264_PROFILE_EXTENDED),
> > > > > +               },
> > > > > +               .codec_type = V4L2_PIX_FMT_H264_SLICE,
> > > > > +       },
> > > > > +       {
> > > > > +               .cfg = {
> > > > > +                       .id = V4L2_CID_STATELESS_H264_DECODE_MODE,
> > > > > +                       .min =
> > > > > V4L2_STATELESS_H264_DECODE_MODE_FRAME_BASED,
> > > > > +                       .def =
> > > > > V4L2_STATELESS_H264_DECODE_MODE_FRAME_BASED,
> > > > > +                       .max =
> > > > > V4L2_STATELESS_H264_DECODE_MODE_FRAME_BASED,
> > > > > +               },
> > > > > +               .codec_type = V4L2_PIX_FMT_H264_SLICE,
> > > > > +       },
> > > > > +};
> > > >
> > > > Applications also need to know which V4L2_CID_STATELESS_H264_START_CODE
> > > > the driver supports. From a next patch, this case seems to be
> > > > V4L2_STATELESS_H264_START_CODE_ANNEX_B.
> > >
> > > Indeed - I've added the control, thanks for catching this!
> > >
> > > >
> > > > > +#define NUM_CTRLS ARRAY_SIZE(mtk_stateless_controls)
> > > > > +
> > > > > +static const struct mtk_video_fmt mtk_video_formats[] = {
> > > > > +       {
> > > > > +               .fourcc = V4L2_PIX_FMT_H264_SLICE,
> > > > > +               .type = MTK_FMT_DEC,
> > > > > +               .num_planes = 1,
> > > > > +       },
> > > > > +       {
> > > > > +               .fourcc = V4L2_PIX_FMT_MM21,
> > > > > +               .type = MTK_FMT_FRAME,
> > > > > +               .num_planes = 2,
> > > > > +       },
> > > > > +};
> > > > > +#define NUM_FORMATS ARRAY_SIZE(mtk_video_formats)
> > > > > +#define DEFAULT_OUT_FMT_IDX    0
> > > > > +#define DEFAULT_CAP_FMT_IDX    1
> > > > > +
> > > > > +static const struct mtk_codec_framesizes mtk_vdec_framesizes[] = {
> > > > > +       {
> > > > > +               .fourcc = V4L2_PIX_FMT_H264_SLICE,
> > > > > +               .stepwise = {
> > > > > +                       MTK_VDEC_MIN_W, MTK_VDEC_MAX_W, 16,
> > > > > +                       MTK_VDEC_MIN_H, MTK_VDEC_MAX_H, 16,
> > > > > +               },
> > > > > +       },
> > > > > +};
> > > > > +
> > > > > +#define NUM_SUPPORTED_FRAMESIZE ARRAY_SIZE(mtk_vdec_framesizes)
> > > > > +
> > > > > +static void mtk_vdec_stateless_set_dst_payload(struct mtk_vcodec_ctx
> > > > > *ctx,
> > > > > +                                              struct vdec_fb *fb)
> > > > > +{
> > > > > +       struct mtk_video_dec_buf *vdec_frame_buf =
> > > > > +               container_of(fb, struct mtk_video_dec_buf,
> > > > > frame_buffer);
> > > > > +       struct vb2_v4l2_buffer *vb = &vdec_frame_buf->m2m_buf.vb;
> > > > > +       unsigned int cap_y_size = ctx-
> > > > > >q_data[MTK_Q_DATA_DST].sizeimage[0];
> > > > > +
> > > > > +       vb2_set_plane_payload(&vb->vb2_buf, 0, cap_y_size);
> > > > > +       if (ctx->q_data[MTK_Q_DATA_DST].fmt->num_planes == 2) {
> > > > > +               unsigned int cap_c_size =
> > > > > +                       ctx->q_data[MTK_Q_DATA_DST].sizeimage[1];
> > > > > +
> > > > > +               vb2_set_plane_payload(&vb->vb2_buf, 1, cap_c_size);
> > > > > +       }
> > > > > +}
> > > > > +
> > > > > +static struct vdec_fb *vdec_get_cap_buffer(struct mtk_vcodec_ctx
> > > > > *ctx,
> > > > > +                                          struct vb2_v4l2_buffer
> > > > > *vb2_v4l2)
> > > > > +{
> > > > > +       struct mtk_video_dec_buf *framebuf =
> > > > > +               container_of(vb2_v4l2, struct mtk_video_dec_buf,
> > > > > m2m_buf.vb);
> > > > > +       struct vdec_fb *pfb = &framebuf->frame_buffer;
> > > > > +       struct vb2_buffer *dst_buf = &vb2_v4l2->vb2_buf;
> > > > > +
> > > > > +       pfb = &framebuf->frame_buffer;
> > > > > +       pfb->base_y.va = vb2_plane_vaddr(dst_buf, 0);
> > > >
> > > > Are you sure you need a CPU mapping? It seems strange.
> > > > I'll comment some more on the next patch(es).
> > >
> > > I'll answer on the next patch since this is where that mapping is being
> > > used.
> > >
> > > >
> > > > > +       pfb->base_y.dma_addr = vb2_dma_contig_plane_dma_addr(dst_buf,
> > > > > 0);
> > > > > +       pfb->base_y.size = ctx->q_data[MTK_Q_DATA_DST].sizeimage[0];
> > > > > +
> > > > > +       if (ctx->q_data[MTK_Q_DATA_DST].fmt->num_planes == 2) {
> > > > > +               pfb->base_c.va = vb2_plane_vaddr(dst_buf, 1);
> > > > > +               pfb->base_c.dma_addr =
> > > > > +                       vb2_dma_contig_plane_dma_addr(dst_buf, 1);
> > > > > +               pfb->base_c.size = ctx-
> > > > > >q_data[MTK_Q_DATA_DST].sizeimage[1];
> > > > > +       }
> > > > > +       mtk_v4l2_debug(1,
> > > > > +               "id=%d Framebuf pfb=%p VA=%p Y_DMA=%pad C_DMA=%pad
> > > > > Size=%zx frame_count = %d",
> > > > > +               dst_buf->index, pfb,
> > > > > +               pfb->base_y.va, &pfb->base_y.dma_addr,
> > > > > +               &pfb->base_c.dma_addr, pfb->base_y.size,
> > > > > +               ctx->decoded_frame_cnt);
> > > > > +
> > > > > +       return pfb;
> > > > > +}
> > > > > +
> > > > > +static void vb2ops_vdec_buf_request_complete(struct vb2_buffer *vb)
> > > > > +{
> > > > > +       struct mtk_vcodec_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
> > > > > +
> > > > > +       v4l2_ctrl_request_complete(vb->req_obj.req, &ctx->ctrl_hdl);
> > > > > +}
> > > > > +
> > > > > +static int fops_media_request_validate(struct media_request *mreq)
> > > > > +{
> > > > > +       const unsigned int buffer_cnt = vb2_request_buffer_cnt(mreq);
> > > > > +       struct mtk_vcodec_ctx *ctx = NULL;
> > > > > +       struct media_request_object *req_obj;
> > > > > +       struct v4l2_ctrl_handler *parent_hdl, *hdl;
> > > > > +       struct v4l2_ctrl *ctrl;
> > > > > +       unsigned int i;
> > > > > +
> > > > > +       switch (buffer_cnt) {
> > > > > +       case 1:
> > > > > +               /* We expect exactly one buffer with the request */
> > > > > +               break;
> > > > > +       case 0:
> > > > > +               mtk_v4l2_err("No buffer provided with the request");
> > > > > +               return -ENOENT;
> > > > > +       default:
> > > > > +               mtk_v4l2_err("Too many buffers (%d) provided with the
> > > > > request",
> > > > > +                            buffer_cnt);
> > > > > +               return -EINVAL;
> > > > > +       }
> > > > > +
> > > > > +       list_for_each_entry(req_obj, &mreq->objects, list) {
> > > > > +               struct vb2_buffer *vb;
> > > > > +
> > > > > +               if (vb2_request_object_is_buffer(req_obj)) {
> > > > > +                       vb = container_of(req_obj, struct vb2_buffer,
> > > > > req_obj);
> > > > > +                       ctx = vb2_get_drv_priv(vb->vb2_queue);
> > > > > +                       break;
> > > > > +               }
> > > > > +       }
> > > > > +
> > > > > +       if (!ctx) {
> > > > > +               mtk_v4l2_err("Cannot find buffer for request");
> > > > > +               return -ENOENT;
> > > > > +       }
> > > > > +
> > > > > +       parent_hdl = &ctx->ctrl_hdl;
> > > > > +
> > > > > +       hdl = v4l2_ctrl_request_hdl_find(mreq, parent_hdl);
> > > > > +       if (!hdl) {
> > > > > +               mtk_v4l2_err("Cannot find control handler for
> > > > > request\n");
> > > > > +               return -ENOENT;
> > > > > +       }
> > > > > +
> > > > > +       for (i = 0; i < NUM_CTRLS; i++) {
> > > > > +               if (mtk_stateless_controls[i].codec_type != ctx-
> > > > > >current_codec)
> > > > > +                       continue;
> > > > > +               if (!mtk_stateless_controls[i].needed_in_request)
> > > > > +                       continue;
> > > > > +
> > > > > +               ctrl = v4l2_ctrl_request_hdl_ctrl_find(hdl,
> > > > > +
> > > > > mtk_stateless_controls[i].cfg.id);
> > > > > +               if (!ctrl) {
> > > > > +                       mtk_v4l2_err("Missing required codec
> > > > > control\n");
> > > > > +                       return -ENOENT;
> > > > > +               }
> > > > > +       }
> > > > > +
> > > > > +       v4l2_ctrl_request_hdl_put(hdl);
> > > > > +
> > > > > +       return vb2_request_validate(mreq);
> > > > > +}
> > > > > +
> > > > > +static void mtk_vdec_worker(struct work_struct *work)
> > > > > +{
> > > > > +       struct mtk_vcodec_ctx *ctx =
> > > > > +               container_of(work, struct mtk_vcodec_ctx,
> > > > > decode_work);
> > > > > +       struct mtk_vcodec_dev *dev = ctx->dev;
> > > > > +       struct vb2_v4l2_buffer *vb2_v4l2_src, *vb2_v4l2_dst;
> > > > > +       struct vb2_buffer *vb2_src;
> > > > > +       struct mtk_vcodec_mem *bs_src;
> > > > > +       struct mtk_video_dec_buf *dec_buf_src;
> > > > > +       struct media_request *src_buf_req;
> > > > > +       struct vdec_fb *dst_buf;
> > > > > +       bool res_chg = false;
> > > > > +       int ret;
> > > > > +
> > > > > +       vb2_v4l2_src = v4l2_m2m_next_src_buf(ctx->m2m_ctx);
> > > > > +       if (vb2_v4l2_src == NULL) {
> > > > > +               v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
> > > > > +               mtk_v4l2_debug(1, "[%d] no available source buffer",
> > > > > ctx->id);
> > > > > +               return;
> > > > > +       }
> > > > > +
> > > > > +       vb2_v4l2_dst = v4l2_m2m_next_dst_buf(ctx->m2m_ctx);
> > > > > +       if (vb2_v4l2_dst == NULL) {
> > > > > +               v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
> > > > > +               mtk_v4l2_debug(1, "[%d] no available destination
> > > > > buffer", ctx->id);
> > > > > +               return;
> > > > > +       }
> > > > > +
> > > > > +       vb2_src = &vb2_v4l2_src->vb2_buf;
> > > > > +       dec_buf_src = container_of(vb2_v4l2_src, struct
> > > > > mtk_video_dec_buf,
> > > > > +                                  m2m_buf.vb);
> > > > > +       bs_src = &dec_buf_src->bs_buffer;
> > > > > +
> > > > > +       mtk_v4l2_debug(3, "[%d] (%d) id=%d, vb=%p buf_info = %p",
> > > > > +                       ctx->id, src_buf->vb2_queue->type,
> > > > > +                       src_buf->index, src_buf, src_buf_info);
> > > > > +
> > > > > +       bs_src->va = vb2_plane_vaddr(vb2_src, 0);
> > > > > +       bs_src->dma_addr = vb2_dma_contig_plane_dma_addr(vb2_src, 0);
> > > > > +       bs_src->size = (size_t)vb2_src->planes[0].bytesused;
> > > > > +       if (!bs_src->va) {
> > > > > +               v4l2_m2m_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx);
> > > > > +               mtk_v4l2_err("[%d] id=%d source buffer is NULL", ctx-
> > > > > >id,
> > > > > +                            vb2_src->index);
> > > > > +               return;
> > > > > +       }
> > > > > +
> > > > > +       mtk_v4l2_debug(3, "[%d] Bitstream VA=%p DMA=%pad Size=%zx
> > > > > vb=%p",
> > > > > +                       ctx->id, buf->va, &buf->dma_addr, buf->size,
> > > > > src_buf);
> > > > > +       /* Apply request controls. */
> > > > > +       src_buf_req = vb2_src->req_obj.req;
> > > > > +       if (src_buf_req)
> > > > > +               v4l2_ctrl_request_setup(src_buf_req, &ctx->ctrl_hdl);
> > > > > +       else
> > > > > +               mtk_v4l2_err("vb2 buffer media request is NULL");
> > > > > +
> > > > > +       dst_buf = vdec_get_cap_buffer(ctx, vb2_v4l2_dst);
> > > > > +       v4l2_m2m_buf_copy_metadata(vb2_v4l2_src, vb2_v4l2_dst, true);
> > > > > +       ret = vdec_if_decode(ctx, bs_src, dst_buf, &res_chg);
> > > > > +       if (ret) {
> > > > > +               mtk_v4l2_err(
> > > > > +                       " <===[%d], src_buf[%d] sz=0x%zx pts=%llu
> > > > > vdec_if_decode() ret=%d res_chg=%d===>",
> > > > > +                       ctx->id, vb2_src->index, bs_src->size,
> > > > > +                       vb2_src->timestamp, ret, res_chg);
> > > > > +               if (ret == -EIO) {
> > > > > +                       mutex_lock(&ctx->lock);
> > > > > +                       dec_buf_src->error = true;
> > > > > +                       mutex_unlock(&ctx->lock);
> > > > > +               }
> > > > > +       }
> > > > > +
> > > > > +       mtk_vdec_stateless_set_dst_payload(ctx, dst_buf);
> > > > > +
> > > > > +       v4l2_m2m_buf_done_and_job_finish(dev->m2m_dev_dec, ctx-
> > > > > >m2m_ctx,
> > > > > +               ret ? VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE);
> > > > > +
> > > > > +       v4l2_ctrl_request_complete(src_buf_req, &ctx->ctrl_hdl);
> > > > > +}
> > > > > +
> > > > > +static void vb2ops_vdec_stateless_buf_queue(struct vb2_buffer *vb)
> > > > > +{
> > > > > +       struct mtk_vcodec_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
> > > > > +       struct vb2_v4l2_buffer *vb2_v4l2 = to_vb2_v4l2_buffer(vb);
> > > > > +
> > > > > +       mtk_v4l2_debug(3, "[%d] (%d) id=%d, vb=%p",
> > > > > +                       ctx->id, vb->vb2_queue->type,
> > > > > +                       vb->index, vb);
> > > > > +
> > > > > +       mutex_lock(&ctx->lock);
> > > > > +       v4l2_m2m_buf_queue(ctx->m2m_ctx, vb2_v4l2);
> > > > > +       mutex_unlock(&ctx->lock);
> > > > > +       if (vb->vb2_queue->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> > > > > +               return;
> > > > > +
> > > > > +       mtk_v4l2_debug(3, "(%d) id=%d, bs=%p",
> > > > > +               vb->vb2_queue->type, vb->index, src_buf);
> > > > > +
> > > > > +       /* If an OUTPUT buffer, we may need to update the state */
> > > > > +       if (ctx->state == MTK_STATE_INIT) {
> > > > > +               ctx->state = MTK_STATE_HEADER;
> > > > > +               mtk_v4l2_debug(1, "Init driver from init to header.");
> > > >
> > > > This state thing seems just something to make the rest
> > > > of the stateful-based driver happy, right?
> > >
> > > Correct - if anything we should either use more of the state here
> > > (i.e. set the error state when relevant) or move the state entirely in
> > > the stateful part of the driver.
> > >
> > > >
> > > > Makes me wonder a bit if just splitting the stateless part to its
> > > > own driver, wouldn't make your maintenance easier.
> > > >
> > > > What's the motivation for sharing the driver?
> > >
> > > Technically you could do it both ways. Separating the driver would
> > > result in some boilerplate code and buffer-management structs
> > > duplication (unless we keep the shared part under another module - but
> > > in this case we are basically in the same situation as now). Also
> > > despite using different userspace-facing ABIs, MT8173 and MT8183
> > > follow a similar architecture and a similar firmware interface.
> > > Considering these similarities it seems simpler from an architectural
> > > point of view to have all the Mediatek codec support under the same
> > > driver. It also probably results in less code.
> > >
> > > That being said, the split can probably be improved as you pointed out
> > > with this state variable. But the current split is not too bad IMHO,
> > > at least not worse than how the code was originally.
> > >
> > > >
> > > > > +       } else {
> > > > > +               mtk_v4l2_debug(3, "[%d] already init driver %d",
> > > > > +                               ctx->id, ctx->state);
> > > > > +       }
> > > > > +}
> > > > > +
> > > > > +static int mtk_vdec_flush_decoder(struct mtk_vcodec_ctx *ctx)
> > > > > +{
> > > > > +       bool res_chg;
> > > > > +
> > > > > +       return vdec_if_decode(ctx, NULL, NULL, &res_chg);
> > > > > +}
> > > > > +
> > > > > +static const struct v4l2_ctrl_ops mtk_vcodec_dec_ctrl_ops = {
> > > > > +       .g_volatile_ctrl = mtk_vdec_g_v_ctrl,
> > > > > +};
> > > > > +
> > > > > +static int mtk_vcodec_dec_ctrls_setup(struct mtk_vcodec_ctx *ctx)
> > > > > +{
> > > > > +       struct v4l2_ctrl *ctrl;
> > > > > +       unsigned int i;
> > > > > +
> > > > > +       v4l2_ctrl_handler_init(&ctx->ctrl_hdl, NUM_CTRLS);
> > > > > +       if (ctx->ctrl_hdl.error) {
> > > > > +               mtk_v4l2_err("v4l2_ctrl_handler_init failed\n");
> > > > > +               return ctx->ctrl_hdl.error;
> > > > > +       }
> > > > > +
> > > > > +       ctrl = v4l2_ctrl_new_std(&ctx->ctrl_hdl,
> > > > > +                               &mtk_vcodec_dec_ctrl_ops,
> > > > > +                               V4L2_CID_MIN_BUFFERS_FOR_CAPTURE,
> > > > > +                               0, 32, 1, 1);
> > > > > +       ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE;
> > > >
> > > > Hm, this volatile control for MIN_BUFFERS_FOR_CAPTURE seems
> > > > to return the DPB size. However, isn't this something userspace already
> > > > knows?
> > >
> > > True, but that's also a control the driver is supposed to provide per
> > > the spec IIUC.
> > >
> >
> > I don't see the specification requiring this control. TBH, I'd just drop it
> > and if needed fix the application to support this as an optional
> > control.
> >
> > In any case, stateless devices should just need 1 output and 1 capture
> > buffer.
>
> Mmm, you're correct indeed, and checking with our user-space it does
> not rely on this control for stateless codecs. Moving this control to
> the stateful part of the driver.
>
>
> >
> > You might dislike this redundancy, note that you can also get the minimum
> > required buffers through VIDIOC_REQBUFS, where the count
> > v4l2_requestbuffers.field is returned back to userspace with the
> > number of allocated buffers.
> >
> > If you request just 1 buffer, and your driver needed 3, you should
> > get a 3 there (vb2_ops.queue_setup takes care of that).
> >
> > Thanks,
> > Ezequiel