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

[xiaoyong.lu@xxxxxxxxxxxx]

Dear Daniel,
Thanks for your good suggestion!

I have updated v4 to fix
your comment.

Changes from v3:

- modify comment for struct vdec_av1_slice_slot
- add define SEG_LVL_ALT_Q
- change use_lr/use_chroma_lr parse from av1 spec
- use ARRAY_SIZE to replace size for loop_filter_level and
loop_filter_mode_deltas
- change array size of loop_filter_mode_deltas from 4 to 2
- add define SECONDARY_FILTER_STRENGTH_NUM_BITS
- change some hex values from upper case to lower case
- change *dpb_sz equal to V4L2_AV1_TOTAL_REFS_PER_FRAME + 1
- convert vb2_find_timestamp to vb2_find_buffer
- test by av1 fluster, result is 173/239

detail in link:

https://patchwork.kernel.org/project/linux-mediatek/patch/20220930033000.22579-1-xiaoyong.lu@xxxxxxxxxxxx/


thanks !
Xiaoyong Lu

On Thu, 2022-09-22 at 13:36 -0300, Daniel Almeida wrote:
> 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@xxxxxxxxxxxxxxxxxxxx/T/
> 
> -- Daniel
>