linux-kernel - Re: [PATCH v13 7/7] media: nuvoton: Add driver for NPCM video capture and encode engine

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] [day] [month] [year] [list]

Message-ID: <CADnNmFoxgFZi3c9wGoERW4rrPBvMiGyBvUZ7YFu=MpVHbS97pg@mail.gmail.com>
Date:   Tue, 15 Aug 2023 20:02:13 +0800
From:   Kun-Fa Lin <milkfafa@...il.com>
To:     Hans Verkuil <hverkuil-cisco@...all.nl>
Cc:     mchehab@...nel.org, avifishman70@...il.com, tmaimon77@...il.com,
        tali.perry1@...il.com, venture@...gle.com, yuenn@...gle.com,
        benjaminfair@...gle.com, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, andrzej.p@...labora.com,
        devicetree@...r.kernel.org, linux-media@...r.kernel.org,
        linux-kernel@...r.kernel.org, openbmc@...ts.ozlabs.org,
        kwliu@...oton.com, kflin@...oton.com
Subject: Re: [PATCH v13 7/7] media: nuvoton: Add driver for NPCM video capture
 and encode engine

Hi Hans,

Thanks for the review.

> > can compress the frame data into HEXTITLE format. This driver implements
>
> HEXTITLE -> HEXTILE

> > +       data into HEXTITLE format.
>
> Ditto.

> > +     video->active_timings = video->detected_timings;
>
> Why not let npcm_video_set_resolution() set active_timings?
> And also update pix_fmt? That will also simplify npcm_video_set_dv_timings().

> > +     .type = V4L2_CTRL_TYPE_INTEGER,
>
> This must be of TYPE_MENU. It selects between two
> modes, so that is typically a MENU control. That way you can list
> the possible modes and get a human-readable name for each setting.

These problems will be addressed in v14.


> > +static const struct v4l2_ctrl_config npcm_ctrl_rect_count = {
> > +     .ops = &npcm_video_ctrl_ops,
> > +     .id = V4L2_CID_NPCM_RECT_COUNT,
> > +     .name = "NPCM Compressed Hextile Rectangle Count",
> > +     .type = V4L2_CTRL_TYPE_INTEGER,
> > +     .flags = V4L2_CTRL_FLAG_VOLATILE,
> > +     .min = 0,
> > +     .max = (MAX_WIDTH / RECT_W) * (MAX_HEIGHT / RECT_H),
> > +     .step = 1,
> > +     .def = 0,
> > +};
>
> I'm wondering if this shouldn't be an INTEGER array control.
> Either dynamic or just fixed to size VIDEO_MAX_FRAME. That way
> you can set the rectangle count for each buffer index. You wouldn't
> need this to be volatile either in that case.
>
> I don't really like the way it is set up now since if userspace is
> slow in processing a frame the control might have been updated already
> for the next frame and you get the wrong value for the buffer you are
> processing.

When userspace app dequeues a buffer, it needs to know the count of
HEXTILE rectangles in the buffer,
so app will call this control to get the rect count after dequeueing the buffer.

And when a buffer is dequeued, npcm_video_buf_finish() will be called,
in which the buffer index (vb->index) will be stored.
Then when userspace app calls this control,
npcm_video_get_volatile_ctrl() will return the rect count of the
desired buffer index.
In this way, I think the buffer index is always correct even if
userspace is slow.


> > +     if (*num_buffers > VIDEO_MAX_FRAME)
> > +             *num_buffers = VIDEO_MAX_FRAME;
>
> You can drop this test, it's done automatically by the vb2 core.

> > +     for (i = 0; i < *num_buffers; i++)
> > +             INIT_LIST_HEAD(&video->list[i]);
>
> This is incomplete: if VIDIOC_CREATE_BUFS is called additional buffers can be added.
> In that case this function is called with *num_planes already set and *num_buffers
> being the additional buffers you want to add. So in the 'if (*num_planes)' code
> above you need to take care of that.
>
> However, isn't it much easier to just have a fixed 'video->list[VIDEO_MAX_FRAME]' array
> rather than dynamically allocating it? It would simplify the code, and all you need to
> do here is call INIT_LIST_HEAD for all (VIDEO_MAX_FRAME) array elements.

> > +     video->num_buffers = *num_buffers;
>
> You can drop the num_buffers field: just use the num_buffers field of vb2_queue.
>
> This code is incomplete anyway since it doesn't deal with VIDIOC_CREATE_BUFS.
> Much easier to just rely on the vb2_queue information.

Will modify as you suggested. Thanks.

Regards,
Marvin