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: <20230828091859.3889817-1-milkfafa@gmail.com> <20230828091859.3889817-8-milkfafa@gmail.com>
 <0c91f68d-d172-bd14-9115-b6977ad2d01f@xs4all.nl> <CADnNmFqVbRWs5Uf_tJdrM0AopF_CmSHYwBK8-+1go_K6Hefkcw@mail.gmail.com>
 <ecc13f10-82b9-f99b-8ebf-ff505d65fe2e@xs4all.nl>
In-Reply-To: <ecc13f10-82b9-f99b-8ebf-ff505d65fe2e@xs4all.nl>
From:   Kun-Fa Lin <milkfafa@gmail.com>
Date:   Mon, 4 Sep 2023 12:49:46 +0800
Message-ID: <CADnNmFoRjyHghQPA72HUmmCEr81oUBJPDG+QtuTFaem_imNB=Q@mail.gmail.com>
Subject: Re: [PATCH v14 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,

Thanks for the reply.

> > 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?
>
> Ah, I don't think I have ever seen anyone use buf_finish in that way!
>
> Very inventive, and perfectly legal. Actually a very nice idea!
>
> So, with that in mind there are still some things that need to change.
>
> First of all, you can drop the 'VOLATILE' flag from the control, instead
> just call v4l2_ctrl_s_ctrl() from buf_finish() to update the control.
> And in stop_streaming the control value should probably be set to 0.
>
> The use of volatile for a control is a last resort, and in this case it
> is not volatile at all.
>
> Secondly, this behavior has to be documented: in buf_finish add a comment
> along the lines of: "This callback is called when the buffer is dequeued,
> so update this control with the number of rectangles."
>
> And where the control is defined, refer to buf_finish to explain where it
> is set.
>
> Finally the user-facing documentation has to be updated (npcm-video.rst)
> to explain this behavior.

OK. Will drop the VOLATILE flag and update comment/document in the next version.

Regards,
Marvin