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 >
