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

[Kun-Fa Lin]

Hi Hans,

Thanks for the review.

> > +     snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s", DEVICE_NAME);
>
> You can drop this last line, it is already filled in by the V4L2 core for
> platform devices.

> > +static const char * const npcm_ctrl_capture_mode_menu[] = {
> > +     "COMPLETE mode",
> > +     "DIFF mode",
>
> Hmm, I would drop the 'mode' bit, since it is already obvious that
> these are the modes.

OK. Will drop them in the next version.

> > +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,
> > +};
>
> Just to confirm: you decided against using an integer array control?
>
> There is a real danger that if userspace isn't reading this control
> quickly enough (i.e. before the next frame arrives at the driver), then
> the control's value is that of that next frame instead of the current
> frame.
>
> It doesn't feel robust to me.

Actually the driver will store the frames and counts for each buffer
index till userspace dequeues them.

Ex. assume that driver has captured 3 frames:
- 1st capture (buffer index = 0):
     video->list[0] => store the list of HEXTILE rects for the 1st frame
     video->rect[0] => store the rect count of video->list[0]
- 2nd capture (buffer index = 1):
     video->list[1] => store the list of HEXTILE rects for the 2nd frame
     video->rect[1] => store the rect count of video->list[1]
- 3rd capture (buffer index = 2):
     video->list[2] => store the list of HEXTILE rects for the 3rd frame
     video->rect[2] => store the rect count of video->list[2]

When userspace dequeues the 1st buffer (video->list[0]), it needs to
know the count of HEXTILE rectangles in the buffer,
so after dequeuing the buffer it will call this control to get the
rect count (video->rect[0]). And when a buffer is dequeued,
npcm_video_buf_finish() will be called, in which the buffer index (in
this example, buffer index = 0) will be stored to video->vb_index.
Then when userspace calls this control, npcm_video_get_volatile_ctrl()
will return the rect count of vb_index = 0.
In this way, I think userspace is always reading the correct control's
value even if userspace is slow.
Does it make sense to you or is there anything I missed?

Regards,
Marvin