linux-kernel - Re: [PATCH v5 06/14] media: mtk-vcodec: vdec: move stateful ops into their own file

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: <CAPBb6MXFrneXmhA=1FshkrGLZOder4_KeyzD7xGdw66o0hTBZw@mail.gmail.com>
Date:   Thu, 27 May 2021 19:10:28 +0900
From:   Alexandre Courbot <acourbot@...omium.org>
To:     Tzung-Bi Shih <tzungbi@...gle.com>
Cc:     Tiffany Lin <tiffany.lin@...iatek.com>,
        Andrew-CT Chen <andrew-ct.chen@...iatek.com>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        Dafna Hirschfeld <dafna.hirschfeld@...labora.com>,
        Yunfei Dong <yunfei.dong@...iatek.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Linux Media Mailing List <linux-media@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-mediatek@...ts.infradead.org>
Subject: Re: [PATCH v5 06/14] media: mtk-vcodec: vdec: move stateful ops into
 their own file

On Fri, May 21, 2021 at 10:37 PM Tzung-Bi Shih <tzungbi@...gle.com> wrote:
>
> On Wed, May 19, 2021 at 10:31 PM Alexandre Courbot
> <acourbot@...omium.org> wrote:
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_stateful.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_stateful.c
> > new file mode 100644
> > index 000000000000..ed4b1308a0e4
> > --- /dev/null
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_stateful.c
> > @@ -0,0 +1,667 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <media/v4l2-event.h>
> > +#include <media/v4l2-mem2mem.h>
> > +#include <media/videobuf2-dma-contig.h>
> > +
> > +#include "mtk_vcodec_drv.h"
> > +#include "mtk_vcodec_dec.h"
> > +#include "mtk_vcodec_intr.h"
> > +#include "mtk_vcodec_util.h"
> > +#include "vdec_drv_if.h"
> > +#include "mtk_vcodec_dec_pm.h"
>
> Would be good practice to sort them.

Did what I could, but the order seems to somehow matter. :/

>
> > +static struct vb2_buffer *get_display_buffer(struct mtk_vcodec_ctx *ctx)
> > +{
> > +       struct vdec_fb *disp_frame_buffer = NULL;
> > +       struct mtk_video_dec_buf *dstbuf;
> > +       struct vb2_v4l2_buffer *vb;
> > +
> > +       mtk_v4l2_debug(3, "[%d]", ctx->id);
> > +       if (vdec_if_get_param(ctx,
> > +                       GET_PARAM_DISP_FRAME_BUFFER,
> > +                       &disp_frame_buffer)) {
> > +               mtk_v4l2_err("[%d]Cannot get param : GET_PARAM_DISP_FRAME_BUFFER",
> > +                       ctx->id);
> > +               return NULL;
> > +       }
> > +
> > +       if (disp_frame_buffer == NULL) {
>
> Although this is a move refactor, to be neat, !disp_frame_buffer.

These were also raised by checkpatch.pl --strict, they are now all fixed.

>
> > +static struct vb2_buffer *get_free_buffer(struct mtk_vcodec_ctx *ctx)
> > +{
> > +       struct mtk_video_dec_buf *dstbuf;
> > +       struct vdec_fb *free_frame_buffer = NULL;
> > +       struct vb2_v4l2_buffer *vb;
> > +
> > +       if (vdec_if_get_param(ctx,
> > +                               GET_PARAM_FREE_FRAME_BUFFER,
> > +                               &free_frame_buffer)) {
> > +               mtk_v4l2_err("[%d] Error!! Cannot get param", ctx->id);
> > +               return NULL;
> > +       }
> > +       if (free_frame_buffer == NULL) {
>
> The same concern as above.
>
> > +static void clean_display_buffer(struct mtk_vcodec_ctx *ctx)
> > +{
> > +       struct vb2_buffer *framptr;
> > +
> > +       do {
> > +               framptr = get_display_buffer(ctx);
> > +       } while (framptr);
> > +}
> > +
> > +static void clean_free_buffer(struct mtk_vcodec_ctx *ctx)
> > +{
> > +       struct vb2_buffer *framptr;
> > +
> > +       do {
> > +               framptr = get_free_buffer(ctx);
> > +       } while (framptr);
> > +}
>
> while (get_display_buffer(ctx)) ;
> and
> while (get_free_buffer(ctx)) ;
> looks better.

Done.

>
> > +static int mtk_vdec_flush_decoder(struct mtk_vcodec_ctx *ctx)
> > +{
> > +       bool res_chg;
> > +       int ret = 0;
>
> No need to initialize.

Done.

>
> > +static void mtk_vdec_worker(struct work_struct *work)
> > +{
> > +       struct mtk_vcodec_ctx *ctx = container_of(work, struct mtk_vcodec_ctx,
> > +                               decode_work);
> > +       struct mtk_vcodec_dev *dev = ctx->dev;
> > +       struct vb2_v4l2_buffer *src_buf, *dst_buf;
> > +       struct mtk_vcodec_mem buf;
> > +       struct vdec_fb *pfb;
> > +       bool res_chg = false;
> > +       int ret;
> > +       struct mtk_video_dec_buf *dst_buf_info, *src_buf_info;
> > +
> > +       src_buf = v4l2_m2m_next_src_buf(ctx->m2m_ctx);
> > +       if (src_buf == NULL) {
>
> To neat, !src_buf.
>
> > +       dst_buf = v4l2_m2m_next_dst_buf(ctx->m2m_ctx);
> > +       if (dst_buf == NULL) {
>
> To neat, !dst_buf.
>
> > +static void vb2ops_vdec_stateful_buf_queue(struct vb2_buffer *vb)
> > +{
> > +       struct vb2_v4l2_buffer *src_buf;
> > +       struct mtk_vcodec_mem src_mem;
> > +       bool res_chg = false;
> > +       int ret = 0;
> > +       unsigned int dpbsize = 1, i = 0;
> > +       struct mtk_vcodec_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
> > +       struct vb2_v4l2_buffer *vb2_v4l2 = NULL;
>
> ret, i, and vb2_v4l2 are obviously no need to initialize.

Done, thanks!