Re: [RFC PATCH v3] media: mediatek: vcodec: support stateless AV1 decoder

[Daniel Almeida <daniel.almeida@collabora.com>]

Hi Xiaoyong.

Comments below (other code removed for brevity)

+/**
+ * struct vdec_av1_slice_slot - slot info need save in global instance
+ * @frame_info: frame info for each slot
+ * @timestamp:  time stamp info
+ */
+struct vdec_av1_slice_slot {
+	struct vdec_av1_slice_frame_info frame_info[AV1_MAX_FRAME_BUF_COUNT];
+	u64 timestamp[AV1_MAX_FRAME_BUF_COUNT];
+};

nit: slot info that needs to be saved in the global instance

+static int vdec_av1_slice_get_qindex(struct 
vdec_av1_slice_uncompressed_header *uh,
+				     int segmentation_id)
+{
+	struct vdec_av1_slice_seg *seg = &uh->seg;
+	struct vdec_av1_slice_quantization *quant = &uh->quant;
+	int data = 0, qindex = 0;
+
+	if (seg->segmentation_enabled &&
+	    (seg->feature_enabled_mask[segmentation_id] & BIT(0))) {
+		data = seg->feature_data[segmentation_id][0];


Maybe you should replace the 0 above by SEG_LVL_ALT_Q to be more 
explicit. Same goes for BIT(0).

+static void vdec_av1_slice_setup_lr(struct vdec_av1_slice_lr *lr,
+				    struct v4l2_av1_loop_restoration  *ctrl_lr)
+{
+	int i;
+
+	for (i = 0; i < V4L2_AV1_NUM_PLANES_MAX; i++) {
+		lr->frame_restoration_type[i] = ctrl_lr->frame_restoration_type[i];
+		lr->loop_restoration_size[i] = ctrl_lr->loop_restoration_size[i];
+	}
+	lr->use_lr = !!lr->frame_restoration_type[0];
+	lr->use_chroma_lr = !!lr->frame_restoration_type[1];
+}

 From a first glance, this looks a bit divergent from the spec?

for ( i = 0; i < NumPlanes; i++ ) {
     lr_type
     FrameRestorationType[i] = Remap_Lr_Type[lr_type]
     if ( FrameRestorationType[i] != RESTORE_NONE ) {
         UsesLr = 1
         if ( i > 0 ) {
             usesChromaLr = 1
         }
     }
}

I will include these two variables in the next iteration of the uapi if 
computing them in the driver is problematic.

+static void vdec_av1_slice_setup_lf(struct vdec_av1_slice_loop_filter *lf,
+				    struct v4l2_av1_loop_filter *ctrl_lf)
+{
+	int i;
+
+	for (i = 0; i < 4; i++)
+		lf->loop_filter_level[i] = ctrl_lf->level[i];
+
+	for (i = 0; i < V4L2_AV1_TOTAL_REFS_PER_FRAME; i++)
+		lf->loop_filter_ref_deltas[i] = ctrl_lf->ref_deltas[i];
+
+	for (i = 0; i < 2; i++)
+		lf->loop_filter_mode_deltas[i] = ctrl_lf->mode_deltas[i];
+
+	lf->loop_filter_sharpness = ctrl_lf->sharpness;
+	lf->loop_filter_delta_enabled =
+		   BIT_FLAG(ctrl_lf, V4L2_AV1_LOOP_FILTER_FLAG_DELTA_ENABLED);
+}

Maybe ARRAY_SIZE can be of use in the loop indices here?

+static void vdec_av1_slice_setup_cdef(struct vdec_av1_slice_cdef *cdef,
+				      struct v4l2_av1_cdef *ctrl_cdef)
+{
+	int i;
+
+	cdef->cdef_damping = ctrl_cdef->damping_minus_3 + 3;
+	cdef->cdef_bits = ctrl_cdef->bits;
+
+	for (i = 0; i < V4L2_AV1_CDEF_MAX; i++) {
+		if (ctrl_cdef->y_sec_strength[i] == 4)
+			ctrl_cdef->y_sec_strength[i] -= 1;
+
+		if (ctrl_cdef->uv_sec_strength[i] == 4)
+			ctrl_cdef->uv_sec_strength[i] -= 1;
+
+		cdef->cdef_y_strength[i] = ctrl_cdef->y_pri_strength[i] << 2 |
+					   ctrl_cdef->y_sec_strength[i];
+		cdef->cdef_uv_strength[i] = ctrl_cdef->uv_pri_strength[i] << 2 |
+					    ctrl_cdef->uv_sec_strength[i];
+	}
+}

Maybe:

#define SECONDARY_FILTER_STRENGTH_NUM_BITS 2

+		cdef->cdef_y_strength[i] = ctrl_cdef->y_pri_strength[i] << 
SECONDARY_FILTER_STRENGTH_NUM_BITS |
+					   ctrl_cdef->y_sec_strength[i];
+		cdef->cdef_uv_strength[i] = ctrl_cdef->uv_pri_strength[i] << 
SECONDARY_FILTER_STRENGTH_NUM_BITS |
+					    ctrl_cdef->uv_sec_strength[i];

This should make it clearer.

+		sb_boundary_x_m1 =
+			(tile->mi_col_starts[tile_col + 1] - tile->mi_col_starts[tile_col] - 
1) &
+			0x3F;
+		sb_boundary_y_m1 =
+			(tile->mi_row_starts[tile_row + 1] - tile->mi_row_starts[tile_row] - 
1) &
+			0x1FF;
+

IIRC there's a preference for lower case hex values in the media subsystem.

+static void vdec_av1_slice_get_dpb_size(struct vdec_av1_slice_instance 
*instance, u32 *dpb_sz)
+{
+	/* refer av1 specification */
+	*dpb_sz = 9;
+}

That's actually defined as 8 in the spec, i.e.:

NUM_REF_FRAMES 8 Number of frames that can be stored for future
reference.

It's helpful to indicate the section if you reference the specification, 
as it makes it easier for the reviewer to cross check.

+	/* get buffer address from vb2buf */
+	for (i = 0; i < V4L2_AV1_REFS_PER_FRAME; i++) {
+		struct vdec_av1_slice_fb *vref = &vsi->ref[i];
+		int idx = vb2_find_timestamp(vq, pfc->ref_idx[i], 0);

Needs to be converted to vb2_find_buffer in light of 
https://lore.kernel.org/lkml/20220706182657.210650-3-ezequiel@vanguardiasur.com.ar/T/

-- Daniel