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
