Re: [PATCH v3 2/4] media: rockchip: Introduce the rkvdec2 driver

Hi Datlev,

Some small and quick feedback below.

On 2024-06-20 16:19, Detlev Casanova wrote:

[snip]

+
+/* Data structure describing auxiliary buffer format. */
+struct rkvdec2_h264_priv_tbl {
+	u32 cabac_table[928];

This should probably now match rkvdec, i.e. s8 cabac_table[4][464][2];

+	struct rkvdec2_h264_scaling_list scaling_list;
+	struct rkvdec2_sps_pps param_set[256];
+	struct rkvdec2_rps rps;
+};
+
+struct rkvdec2_h264_reflists {
+	struct v4l2_h264_reference p[V4L2_H264_REF_LIST_LEN];
+	struct v4l2_h264_reference b0[V4L2_H264_REF_LIST_LEN];
+	struct v4l2_h264_reference b1[V4L2_H264_REF_LIST_LEN];
+};
+
+struct rkvdec2_h264_run {
+	struct rkvdec2_run					base;
+	const struct v4l2_ctrl_h264_decode_params		*decode_params;
+	const struct v4l2_ctrl_h264_sps				*sps;
+	const struct v4l2_ctrl_h264_pps				*pps;
+	const struct v4l2_ctrl_h264_scaling_matrix		*scaling_matrix;

These are aligned, others are not, please be consistent.

+	struct vb2_buffer *ref_buf[V4L2_H264_NUM_DPB_ENTRIES];
+};
+
+struct rkvdec2_h264_ctx {
+	struct rkvdec2_aux_buf		priv_tbl;
+	struct rkvdec2_h264_reflists	reflists;
+	struct rkvdec2_regs_h264	regs;

Same as above, please be consistent for all structs.

+};
+
+static void assemble_hw_pps(struct rkvdec2_ctx *ctx,
+			    struct rkvdec2_h264_run *run)
+{
+	struct rkvdec2_h264_ctx *h264_ctx = ctx->priv;
+	const struct v4l2_ctrl_h264_sps *sps = run->sps;
+	const struct v4l2_ctrl_h264_pps *pps = run->pps;
+	const struct v4l2_ctrl_h264_decode_params *dec_params = run->decode_params;
+	const struct v4l2_h264_dpb_entry *dpb = dec_params->dpb;
+	struct rkvdec2_h264_priv_tbl *priv_tbl = h264_ctx->priv_tbl.cpu;
+	struct rkvdec2_sps_pps *hw_ps;
+	dma_addr_t scaling_list_address;
+	u32 scaling_distance;
+	u32 i;
+
+	/*
+	 * HW read the SPS/PPS information from PPS packet index by PPS id.
+	 * offset from the base can be calculated by PPS_id * 32 (size per PPS
+	 * packet unit). so the driver copy SPS/PPS information to the exact PPS
+	 * packet unit for HW accessing.
+	 */
+	hw_ps = &priv_tbl->param_set[pps->pic_parameter_set_id];
+	memset(hw_ps, 0, sizeof(*hw_ps));
+
+	/* write sps */
+	hw_ps->sps.seq_parameter_set_id = 0xf;

This should use sps->seq_parameter_set_id

+	hw_ps->sps.profile_idc = 0xff;

This shold use sps->profile_idc

+	hw_ps->sps.constraint_set3_flag = 1;

This should use !!(sps->constraint_set_flags & (1 << 3))

+	hw_ps->sps.chroma_format_idc = sps->chroma_format_idc;
+	hw_ps->sps.bit_depth_luma = sps->bit_depth_luma_minus8;
+	hw_ps->sps.bit_depth_chroma = sps->bit_depth_chroma_minus8;
+	hw_ps->sps.qpprime_y_zero_transform_bypass_flag = 0;

This should use

  !!(sps->flags & V4L2_H264_SPS_FLAG_QPPRIME_Y_ZERO_TRANSFORM_BYPASS)

+	hw_ps->sps.log2_max_frame_num_minus4 = sps->log2_max_frame_num_minus4;
+	hw_ps->sps.max_num_ref_frames = sps->max_num_ref_frames;
+	hw_ps->sps.pic_order_cnt_type = sps->pic_order_cnt_type;
+	hw_ps->sps.log2_max_pic_order_cnt_lsb_minus4 =
+		sps->log2_max_pic_order_cnt_lsb_minus4;
+	hw_ps->sps.delta_pic_order_always_zero_flag =
+		!!(sps->flags & V4L2_H264_SPS_FLAG_DELTA_PIC_ORDER_ALWAYS_ZERO);
+	hw_ps->sps.mvc_extension_enable = 1;
+	hw_ps->sps.num_views = 1;
+
+	/*
+	 * Use the SPS values since they are already in macroblocks
+	 * dimensions, height can be field height (halved) if
+	 * V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY is not set and also it allows
+	 * decoding smaller images into larger allocation which can be used
+	 * to implementing SVC spatial layer support.
+	 */
+	hw_ps->sps.pic_width_in_mbs = sps->pic_width_in_mbs_minus1 + 1;
+	hw_ps->sps.pic_height_in_mbs = sps->pic_height_in_map_units_minus1 + 1;
+	hw_ps->sps.frame_mbs_only_flag =
+		!!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY);
+	hw_ps->sps.mb_adaptive_frame_field_flag =
+		!!(sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD);
+	hw_ps->sps.direct_8x8_inference_flag =
+		!!(sps->flags & V4L2_H264_SPS_FLAG_DIRECT_8X8_INFERENCE);
+
+	/* write pps */
+	hw_ps->pps.pic_parameter_set_id = 0xff;

This should use pps->pic_parameter_set_id

+	hw_ps->pps.pps_seq_parameter_set_id = 0x1f;

This should use pps->seq_parameter_set_id

Fluster score is not affected by these changes and follow

https://lore.kernel.org/linux-media/20240618194647.742037-5-jonas@kwiboo.se/ (local)

+	hw_ps->pps.entropy_coding_mode_flag =
+		!!(pps->flags & V4L2_H264_PPS_FLAG_ENTROPY_CODING_MODE);
+	hw_ps->pps.bottom_field_pic_order_in_frame_present_flag =
+		!!(pps->flags & V4L2_H264_PPS_FLAG_BOTTOM_FIELD_PIC_ORDER_IN_FRAME_PRESENT);
+	hw_ps->pps.num_ref_idx_l0_default_active_minus1 =
+		pps->num_ref_idx_l0_default_active_minus1;
+	hw_ps->pps.num_ref_idx_l1_default_active_minus1 =
+		pps->num_ref_idx_l1_default_active_minus1;
+	hw_ps->pps.weighted_pred_flag =
+		!!(pps->flags & V4L2_H264_PPS_FLAG_WEIGHTED_PRED);
+	hw_ps->pps.weighted_bipred_idc = pps->weighted_bipred_idc;
+	hw_ps->pps.pic_init_qp_minus26 = pps->pic_init_qp_minus26;
+	hw_ps->pps.pic_init_qs_minus26 = pps->pic_init_qs_minus26;
+	hw_ps->pps.chroma_qp_index_offset = pps->chroma_qp_index_offset;
+	hw_ps->pps.deblocking_filter_control_present_flag =
+		!!(pps->flags & V4L2_H264_PPS_FLAG_DEBLOCKING_FILTER_CONTROL_PRESENT);
+	hw_ps->pps.constrained_intra_pred_flag =
+		!!(pps->flags & V4L2_H264_PPS_FLAG_CONSTRAINED_INTRA_PRED);
+	hw_ps->pps.redundant_pic_cnt_present =
+		!!(pps->flags & V4L2_H264_PPS_FLAG_REDUNDANT_PIC_CNT_PRESENT);
+	hw_ps->pps.transform_8x8_mode_flag =
+		!!(pps->flags & V4L2_H264_PPS_FLAG_TRANSFORM_8X8_MODE);
+	hw_ps->pps.second_chroma_qp_index_offset = pps->second_chroma_qp_index_offset;
+	hw_ps->pps.scaling_list_enable_flag =
+		!!(pps->flags & V4L2_H264_PPS_FLAG_SCALING_MATRIX_PRESENT);
+
+	/*
+	 * To be on the safe side, program the scaling matrix address
+	 *
+	 * With this set here,
+	 *  RKVDEC_SWREG12_SENCODARY_EN:sw_scanlist_addr_valid_en
+	 * can stay at 0
+	 */
+	scaling_distance = offsetof(struct rkvdec2_h264_priv_tbl, scaling_list);
+	scaling_list_address = h264_ctx->priv_tbl.dma + scaling_distance;
+	hw_ps->pps.scaling_list_address = scaling_list_address;
+
+	for (i = 0; i < ARRAY_SIZE(dec_params->dpb); i++) {
+		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM)
+			hw_ps->pps.is_longterm |= (1 << i);
+	}
+}
+

[snip]

Should be V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_BASELINE

+		.cfg.max = V4L2_MPEG_VIDEO_H264_PROFILE_HIGH,
+		.cfg.menu_skip_mask =
+			BIT(V4L2_MPEG_VIDEO_H264_PROFILE_EXTENDED),
+		.cfg.def = V4L2_MPEG_VIDEO_H264_PROFILE_MAIN,
+	},
+	{
+		.cfg.id = V4L2_CID_MPEG_VIDEO_H264_LEVEL,
+		.cfg.min = V4L2_MPEG_VIDEO_H264_LEVEL_1_0,
+		.cfg.max = V4L2_MPEG_VIDEO_H264_LEVEL_6_1,
+	},
+};
+
+static const struct rkvdec2_ctrls rkvdec2_h264_ctrls = {
+	.ctrls = rkvdec2_h264_ctrl_descs,
+	.num_ctrls = ARRAY_SIZE(rkvdec2_h264_ctrl_descs),
+};
+
+static const u32 rkvdec2_h264_decoded_fmts[] = {
+	V4L2_PIX_FMT_NV12
+};
+
+static const struct rkvdec2_coded_fmt_desc rkvdec2_coded_fmts[] = {
+	{
+		.fourcc = V4L2_PIX_FMT_H264_SLICE,
+		.frmsize = {
+			.min_width = 16,
+			.max_width =  65520,
+			.step_width = 16,
+			.min_height = 16,
+			.max_height =  65520,
+			.step_height = 16,
+		},
+		.ctrls = &rkvdec2_h264_ctrls,
+		.ops = &rkvdec2_h264_fmt_ops,
+		.num_decoded_fmts = ARRAY_SIZE(rkvdec2_h264_decoded_fmts),
+		.decoded_fmts = rkvdec2_h264_decoded_fmts,
+		.subsystem_flags = VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF,
+	},
+};
+
+enum rcb_axis {
+	PIC_WIDTH = 0,
+	PIC_HEIGHT = 1
+};
+
+struct rcb_size_info {
+	u8 multiplier;
+	enum rcb_axis axis;
+};
+
+static struct rcb_size_info rcb_sizes[] = {
+	{6,	PIC_WIDTH},	// intrar
+	{1,	PIC_WIDTH},	// transdr (Is actually 0.4*pic_width)
+	{1,	PIC_HEIGHT},	// transdc (Is actually 0.1*pic_height)
+	{3,	PIC_WIDTH},	// streamdr
+	{6,	PIC_WIDTH},	// interr
+	{3,	PIC_HEIGHT},	// interc
+	{22,	PIC_WIDTH},	// dblkr
+	{6,	PIC_WIDTH},	// saor
+	{11,	PIC_WIDTH},	// fbcr
+	{67,	PIC_HEIGHT},	// filtc col
+};
+
+#define RCB_SIZE(n) (rcb_sizes[(n)].multiplier * (rcb_sizes[(n)].axis ? height : width))

This should probably take width and height, e.g.:

  RCB_SIZE(n,w,h) (rcb_sizes[(n)].multiplier * (rcb_sizes[(n)].axis ? (h) : (w)))

+
+static const struct rkvdec2_coded_fmt_desc *
+rkvdec2_find_coded_fmt_desc(u32 fourcc)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(rkvdec2_coded_fmts); i++) {
+		if (rkvdec2_coded_fmts[i].fourcc == fourcc)
+			return &rkvdec2_coded_fmts[i];
+	}
+
+	return NULL;
+}
+
+static void rkvdec2_reset_fmt(struct rkvdec2_ctx *ctx, struct v4l2_format *f,
+			      u32 fourcc)
+{
+	memset(f, 0, sizeof(*f));
+	f->fmt.pix_mp.pixelformat = fourcc;
+	f->fmt.pix_mp.field = V4L2_FIELD_NONE;
+	f->fmt.pix_mp.colorspace = V4L2_COLORSPACE_REC709;
+	f->fmt.pix_mp.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
+	f->fmt.pix_mp.quantization = V4L2_QUANTIZATION_DEFAULT;
+	f->fmt.pix_mp.xfer_func = V4L2_XFER_FUNC_DEFAULT;
+}
+
+static void rkvdec2_reset_coded_fmt(struct rkvdec2_ctx *ctx)
+{
+	struct v4l2_format *f = &ctx->coded_fmt;
+
+	ctx->coded_fmt_desc = &rkvdec2_coded_fmts[0];
+	rkvdec2_reset_fmt(ctx, f, ctx->coded_fmt_desc->fourcc);
+
+	f->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
+	f->fmt.pix_mp.width = ctx->coded_fmt_desc->frmsize.min_width;
+	f->fmt.pix_mp.height = ctx->coded_fmt_desc->frmsize.min_height;
+
+	if (ctx->coded_fmt_desc->ops->adjust_fmt)
+		ctx->coded_fmt_desc->ops->adjust_fmt(ctx, f);
+}
+
+static void rkvdec2_reset_decoded_fmt(struct rkvdec2_ctx *ctx)
+{
+	struct v4l2_format *f = &ctx->decoded_fmt;
+
+	rkvdec2_reset_fmt(ctx, f, ctx->coded_fmt_desc->decoded_fmts[0]);
+	f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
+	v4l2_fill_pixfmt_mp(&f->fmt.pix_mp,
+			    ctx->coded_fmt_desc->decoded_fmts[0],
+			    ctx->coded_fmt.fmt.pix_mp.width,
+			    ctx->coded_fmt.fmt.pix_mp.height);
+
+	ctx->colmv_offset = f->fmt.pix_mp.plane_fmt[0].sizeimage;
+
+	f->fmt.pix_mp.plane_fmt[0].sizeimage += 128 *
+		DIV_ROUND_UP(f->fmt.pix_mp.width, 16) *
+		DIV_ROUND_UP(f->fmt.pix_mp.height, 16);
+}
+
+static int rkvdec2_enum_framesizes(struct file *file, void *priv,
+				   struct v4l2_frmsizeenum *fsize)
+{
+	const struct rkvdec2_coded_fmt_desc *fmt;

This should probably be named desc to match other uses.

Regards,
Jonas

+
+	if (fsize->index != 0)
+		return -EINVAL;
+
+	fmt = rkvdec2_find_coded_fmt_desc(fsize->pixel_format);
+	if (!fmt)
+		return -EINVAL;
+
+	fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE;
+	fsize->stepwise = fmt->frmsize;
+	return 0;
+}
+

[snip]