| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <75bbf320eb1c54d2834012c1aafcc87105204e8d.camel@mediatek.com>
Date: Fri, 30 Sep 2022 14:19:26 +0800
From: "xiaoyong.lu@...iatek.com" <xiaoyong.lu@...iatek.com>
To: Daniel Almeida <daniel.almeida@...labora.com>,
Yunfei Dong <yunfei.dong@...iatek.com>,
Alexandre Courbot <acourbot@...omium.org>,
Nicolas Dufresne <nicolas@...fresne.ca>,
Hans Verkuil <hverkuil-cisco@...all.nl>,
AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com>,
Benjamin Gaignard <benjamin.gaignard@...labora.com>,
Tiffany Lin <tiffany.lin@...iatek.com>,
Andrew-CT Chen <andrew-ct.chen@...iatek.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Matthias Brugger <matthias.bgg@...il.com>,
Tomasz Figa <tfiga@...gle.com>
CC: Irui Wang <irui.wang@...iatek.com>,
George Sun <george.sun@...iatek.com>,
Steve Cho <stevecho@...omium.org>,
<srv_heupstream@...iatek.com>, <devicetree@...r.kernel.org>,
<Project_Global_Chrome_Upstream_Group@...iatek.com>,
<linux-kernel@...r.kernel.org>,
dri-devel <dri-devel@...ts.freedesktop.org>,
<linux-mediatek@...ts.infradead.org>,
Hsin-Yi Wang <hsinyi@...omium.org>,
Fritz Koenig <frkoenig@...omium.org>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-media@...r.kernel.org>
Subject: Re: [RFC PATCH v3] media: mediatek: vcodec: support stateless AV1
decoder
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@mediatek.com/
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@vanguardiasur.com.ar/T/
>
> -- Daniel
>
Powered by blists - more mailing lists