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

[Kun-Fa Lin]

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