Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230502084430.234182-1-milkfafa@gmail.com> <20230502084430.234182-8-milkfafa@gmail.com>
 <e43029b5-edbb-9358-a0a6-a104ff2fa154@xs4all.nl>
In-Reply-To: <e43029b5-edbb-9358-a0a6-a104ff2fa154@xs4all.nl>
From:   Kun-Fa Lin <milkfafa@gmail.com>
Date:   Thu, 29 Jun 2023 20:48:56 +0800
Message-ID: <CADnNmFpSg+nU+gvc-CUzYRJ6newCrgLesoLda6kHJ6o2a8Su5A@mail.gmail.com>
Subject: Re: [PATCH v12 7/7] media: nuvoton: Add driver for NPCM video capture
 and encode engine
To:     Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc:     mchehab@kernel.org, avifishman70@gmail.com, tmaimon77@gmail.com,
        tali.perry1@gmail.com, venture@google.com, yuenn@google.com,
        benjaminfair@google.com, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, andrzej.p@collabora.com,
        devicetree@vger.kernel.org, linux-media@vger.kernel.org,
        linux-kernel@vger.kernel.org, openbmc@lists.ozlabs.org,
        kwliu@nuvoton.com, kflin@nuvoton.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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