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

[Kun-Fa Lin]

Hi Hans,

> Apologies for the delay in reviewing this. As you may have noticed, we
> have too many incoming patches and not enough reviewers, so it takes
> too often way too long before I have time to review drivers like this.

That's OK. I appreciate your time and comments.

> > +     /* Resolution changed */
> > +     if (status & VCD_STAT_VHT_CHG || status & VCD_STAT_HAC_CHG)
> > +             schedule_work(&video->res_work);
>
> I don't think you need to schedule work. If the resolution changed,
> then you can just call vb2_queue_error and queue the SOURCE_CHANGED
> event here. You don't need to detect the resolution, you know it has changed,
> so just inform userspace and that will call QUERY_DV_TIMINGS.

OK. Will modify it as you suggested.

> > +     if (status & VCD_STAT_IFOR || status & VCD_STAT_IFOT) {
> > +             dev_warn(video->dev, "VCD FIFO overrun or over thresholds\n");
> > +             npcm_video_stop(video);
> > +             npcm_video_start(video);
>
> This is dangerous: video_start detects the resolution and can update the
> width/height. So now there can be a mismatch between what userspace expects
> and what the DMA sends.
>
> I would make a new npcm_video_init(video) function that does the initial
> timings detection. Call that on the first open. The npcm_video_start drops
> that code and just uses the last set timings.
>
> Feel free to use an alternative to this, as long as restarting the video
> here doesn't change the width/height/format as a side-effect.

Understood. I've checked that it can just call npcm_video_start_frame (in which
npcm_video_vcd_state_machine_reset will be called to reset VCD state
machine and FIFOs) and
the width/height/format will not be changed.

> > +     if (*num_buffers > MAX_REQ_BUFS)
> > +             *num_buffers = MAX_REQ_BUFS;
>
> Why limit this? Can't you just use rect[VIDEO_MAX_FRAME]?

I just realized VIDEO_MAX_FRAME is a common define in videodev2.h.
Will change to use it.

> > +     /*
> > +      * When a video buffer is dequeued, free associated rect_list and
> > +      * capture next frame.
> > +      */
> > +     head = &video->list[video->vb_index];
> > +     list_for_each_safe(pos, nx, head) {
> > +             tmp = list_entry(pos, struct rect_list, list);
> > +             list_del(&tmp->list);
> > +             kfree(tmp);
> > +     }
> > +
> > +     if (npcm_video_start_frame(video)) {
>
> This is weird. This is not normally done here since you never know when
> userspace will dequeue a buffer.
>
> I would expect to see this called:
>
> 1) In start_streaming (so that works)
> 2) When a buffer is captured and vb2_buffer_done is called: if another
>    empty buffer is available, then use that.
> 3) in buf_queue: if the buffer list was empty, and vb2_start_streaming_called()
>    is true, then you can start capturing again.

Will modify as you suggested. Thanks for the guide.

Regards,
Marvin