linux-kernel - Re: media: mtk-vcodec: reset segment data then trig decoder

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPBb6MXhiNK84PuUy8=RUUeSh5j4VXw-Ar5SbZDHZAobp7xpEQ@mail.gmail.com>
Date:   Wed, 15 Jan 2020 12:47:32 +0900
From:   Alexandre Courbot <acourbot@...omium.org>
To:     gtk_ruiwang <gtk_ruiwang@...iatek.com>
Cc:     Hans Verkuil <hverkuil@...all.nl>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Tomasz Figa <tfiga@...omium.org>,
        Tiffany Lin <tiffany.lin@...iatek.com>,
        Longfei Wang <longfei.wang@...iatek.com>,
        Yunfei Dong <yunfei.dong@...iatek.com>,
        Maoguang Meng <maoguang.meng@...iatek.com>,
        Linux Media Mailing List <linux-media@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-arm-kernel@...ts.infradead.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-mediatek@...ts.infradead.org>, srv_heupstream@...iatek.com
Subject: Re: media: mtk-vcodec: reset segment data then trig decoder

On Tue, Jan 14, 2020 at 12:32 PM <gtk_ruiwang@...iatek.com> wrote:
>
> From: gtk_ruiwang <gtk_ruiwang@...iatek.com>
>
> VP9 bitstream specification indicate segment data should reset to
> default when meet key frames, intra only frames or enable error
> resilience mode. So memset segmentation map buffer before every
> decode process is not appropriate.
>
> Reset segment data only when needed, then trig decoder hardware
>
> Signed-off-by: Rui Wang <gtk_ruiwang@...iatek.com>
> ---
>  .../platform/mtk-vcodec/vdec/vdec_vp9_if.c    | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/platform/mtk-vcodec/vdec/vdec_vp9_if.c b/drivers/media/platform/mtk-vcodec/vdec/vdec_vp9_if.c
> index 24c1f0bf2147..42c9c3c98076 100644
> --- a/drivers/media/platform/mtk-vcodec/vdec/vdec_vp9_if.c
> +++ b/drivers/media/platform/mtk-vcodec/vdec/vdec_vp9_if.c
> @@ -110,7 +110,9 @@ struct vp9_sf_ref_fb {
>   * @buf_len_sz_c : size used to store cbcr plane ufo info (AP-R, VPU-W)
>
>   * @profile : profile sparsed from vpu (AP-R, VPU-W)
> - * @show_frame : display this frame or not (AP-R, VPU-W)
> + * @show_frame : [BIT(0)] display this frame or not (AP-R, VPU-W)
> + *     [BIT(14)] reset segment data or not (AP-R, VPU-W)
> + *     [BIT(15)] trig decoder hardware or not (AP-R, VPU-W)
>   * @show_existing_frame : inform this frame is show existing frame
>   *     (AP-R, VPU-W)
>   * @frm_to_show_idx : index to show frame (AP-R, VPU-W)
> @@ -494,12 +496,12 @@ static void vp9_swap_frm_bufs(struct vdec_vp9_inst *inst)
>                                         frm_to_show->fb->base_y.size);
>                 }
>                 if (!vp9_is_sf_ref_fb(inst, inst->cur_fb)) {
> -                       if (vsi->show_frame)
> +                       if (vsi->show_frame & BIT(0))
>                                 vp9_add_to_fb_disp_list(inst, inst->cur_fb);
>                 }
>         } else {
>                 if (!vp9_is_sf_ref_fb(inst, inst->cur_fb)) {
> -                       if (vsi->show_frame)
> +                       if (vsi->show_frame & BIT(0))
>                                 vp9_add_to_fb_disp_list(inst, frm_to_show->fb);
>                 }
>         }
> @@ -870,13 +872,22 @@ static int vdec_vp9_decode(void *h_vdec, struct mtk_vcodec_mem *bs,
>                                         vsi->sf_frm_sz[idx]);
>                         }
>                 }
> -               memset(inst->seg_id_buf.va, 0, inst->seg_id_buf.size);
>                 ret = vpu_dec_start(&inst->vpu, data, 3);
>                 if (ret) {
>                         mtk_vcodec_err(inst, "vpu_dec_start failed");
>                         goto DECODE_ERROR;
>                 }
>
> +               if ((vsi->show_frame & BIT(15)) &&
> +                   (vsi->show_frame & BIT(14))) {

Using the new bits in this manner means this patch is not compatible
with the older firmware.

On an older firmware, these bits will be 0, which means the decoder
will never be started. To preserve compatibility, the behavior should
be reversed: *do not* reset and/or start the decoder if the bits are
set.

Also both bits are only used together - we should either separate the
data segment reset and decoder start, or rely on only one bit for
this.

> +                       memset(inst->seg_id_buf.va, 0, inst->seg_id_buf.size);
> +                       ret = vpu_dec_start(&inst->vpu, NULL, 0);
> +                       if (ret) {
> +                               mtk_vcodec_err(inst, "vpu trig decoder failed");
> +                               goto DECODE_ERROR;
> +                       }
> +               }
> +
>                 ret = validate_vsi_array_indexes(inst, vsi);
>                 if (ret) {
>                         mtk_vcodec_err(inst, "Invalid values from VPU.");
> --
> 2.18.0