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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CADnNmFpSg+nU+gvc-CUzYRJ6newCrgLesoLda6kHJ6o2a8Su5A@mail.gmail.com>
Date:   Thu, 29 Jun 2023 20:48:56 +0800
From:   Kun-Fa Lin <milkfafa@...il.com>
To:     Hans Verkuil <hverkuil-cisco@...all.nl>
Cc:     mchehab@...nel.org, avifishman70@...il.com, tmaimon77@...il.com,
        tali.perry1@...il.com, venture@...gle.com, yuenn@...gle.com,
        benjaminfair@...gle.com, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, andrzej.p@...labora.com,
        devicetree@...r.kernel.org, linux-media@...r.kernel.org,
        linux-kernel@...r.kernel.org, openbmc@...ts.ozlabs.org,
        kwliu@...oton.com, kflin@...oton.com
Subject: Re: [PATCH v12 7/7] media: nuvoton: Add driver for NPCM video capture
 and encode engine

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