[PATCH v2] media: rkisp1: Reduce min_queued_buffers to 0

[Jacopo Mondi]

Hi Laurent

On Mon, Oct 28, 2024 at 08:39:36PM +0200, Laurent Pinchart wrote:
> On Mon, Oct 28, 2024 at 06:08:18PM +0100, Jacopo Mondi wrote:
> > On Mon, Oct 28, 2024 at 06:21:41PM +0200, Laurent Pinchart wrote:
> > > On Mon, Oct 28, 2024 at 04:48:55PM +0100, Hans Verkuil wrote:
> > > > On 28/10/2024 16:30, Jacopo Mondi wrote:
> > > > > On Mon, Oct 28, 2024 at 04:02:13PM +0100, Hans Verkuil wrote:
> > > > >> On 28/10/2024 15:35, Jacopo Mondi wrote:
> > > > >>> There apparently is no reason to require 3 queued buffers to call
> > > > >>> streamon() for the RkISP1 as the driver operates with a scratch buffer
> > > > >>> where frames can be directed to if there's no available buffer provided
> > > > >>> by userspace.
> > > > >>>
> > > > >>> Reduce the number of required buffers to 0 to allow applications to
> > > > >>> operate by queueing capture buffers on-demand.
> > > > >>>
> > > > >>> Tested with libcamera, by operating with a single capture request. The
> > > > >>> same request (and associated capture buffer) gets recycled once
> > > > >>> completed. This of course causes a frame rate drop but doesn't hinder
> > > > >>> operations.
> > > > >>>
> > > > >>> Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > > > >>> ---
> > > > >>> The first version of this patch set min_queued_buffers to 1, but setting it
> > > > >>> to 0 doesn't compromise operations and it's even better as it allows application
> > > > >>> to queue buffers to the capture devices on-demand. If a buffer is not provided
> > > > >>> to the DMA engines, image data gets directed to the driver's internal scratch
> > > > >>> buffer.
> > > > >>> ---
> > > > >>>  drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c | 4 +---
> > > > >>>  1 file changed, 1 insertion(+), 3 deletions(-)
> > > > >>>
> > > > >>> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > > > >>> index 2bddb4fa8a5c..5fcf9731f41b 100644
> > > > >>> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > > > >>> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > > > >>> @@ -35,8 +35,6 @@
> > > > >>>  #define RKISP1_SP_DEV_NAME	RKISP1_DRIVER_NAME "_selfpath"
> > > > >>>  #define RKISP1_MP_DEV_NAME	RKISP1_DRIVER_NAME "_mainpath"
> > > > >>>
> > > > >>> -#define RKISP1_MIN_BUFFERS_NEEDED 3
> > > > >>> -
> > > > >>>  enum rkisp1_plane {
> > > > >>>  	RKISP1_PLANE_Y	= 0,
> > > > >>>  	RKISP1_PLANE_CB	= 1,
> > > > >>> @@ -1563,7 +1561,7 @@ static int rkisp1_register_capture(struct rkisp1_capture *cap)
> > > > >>>  	q->ops = &rkisp1_vb2_ops;
> > > > >>>  	q->mem_ops = &vb2_dma_contig_memops;
> > > > >>>  	q->buf_struct_size = sizeof(struct rkisp1_buffer);
> > > > >>> -	q->min_queued_buffers = RKISP1_MIN_BUFFERS_NEEDED;
> > > > >>> +	q->min_queued_buffers = 0;
> > > > >>
> > > > >> You can probably just drop this since the vb2_queue struct is zeroed when it
> > > > >> is allocated. So no need to set it to 0.
> > > > >
> > > > > I suspected so :)
> > > > >
> > > > >>
> > > > >> And is the RKISP1_MIN_BUFFERS_NEEDED define still needed after this change?
> > > > >
> > > > > No, and this patch removes it in facts
> > > > >
> > > > >  -#define RKISP1_MIN_BUFFERS_NEEDED 3
> > > > >  -
> > > >
> > > > I should have checked the patch :-) Sorry for the noise.
> > > >
> > > > >>
> > > > >> Also, see my RFC I posted today:
> > > > >>
> > > > >> https://lore.kernel.org/linux-media/126cd76a-6224-483b-a18d-a3cc89e5ff2d@xs4all.nl/T/#u
> > > > >>
> > > > >> My main concern is that applications that just call VIDIOC_REQBUFS with count = 1
> > > > >> and expect the driver to change that to a workable value, will, in fact, now just get
> > > > >> one buffer. And streaming that will cause lots of frame drops.
> > > > >>
> > > > >> It makes sense to leave min_queued_buffers at 0 if a scratch buffer is available,
> > > > >> but I'm unhappy with the fact that you get a poor experience when REQBUFS(1) is called.
> > > > >
> > > > > Yeah, I've read the discussion between you and Tomi and it seemed like
> > > > > a good time to re-send this patch.
> > > > >
> > > > >> My RFC suggests improvements in the uAPI. With that in place you can use CREATE_BUFS in
> > > > >> libcamera to get much better control over how many buffers should be allocated.
> > > > >
> > > > > In my understanding min_queued_buffers identifies how many buffers
> > > > > should be queued before calling start_streaming, and this comes
> > > > > directly from an hw/driver requirement. This doesn't mean that at
> > > > > least min_queue_buffers should be queued at all the times during
> > > > > streaming, at least, I don't see how and where videobuf2 enforces
> > > > > this. Or does it ?
> > > >
> > > > It's an intrinsic property of the HW/driver: e.g. if it needs two buffers
> > > > queued up for the DMA engine to work, then it really is always holding on
> > > > to two buffers. The only thing the framework does is postpone calling
> > > > start_streaming until that number of buffers is queued to ensure the
> > > > DMA engine has what it needs to start. But after that vb2 doesn't check
> > > > it.
> > >
> > > The "driver" part of "HW/driver" is important here, as drivers can
> > > influence this in multiple ways. One of them is usage of scratch
> > > buffers, but even without that, a DMA engine that requires two buffers
> > > can easily be operated with a single buffer by programming the DMA
> > > engine with the same buffer address twice. Drivers should really do so
> > > unless they really can't.
> > >
> > > > > If the above is correct, then the number of buffers to be queued
> > > > > during streaming is, in my opinion, less an hw/driver requirement but
> > > > > more an application decision.
> > > >
> > > > No, min_queued_buffers is a HW/drivers property: the DMA engine can't
> > > > start until that many buffers are queued up, and once it is started
> > > > it will always hold on to that many buffers.
> >
> > I get it, my point was that once start_streaming has been called, even
> > if min_queued_buffers=2, there is nothing preventing userspace from
> > queing one buffer at the time once the first two have completed. Sure, the
> > hw/driver might not like it, but while delaying start_streaming
> > prevents bad things from happening, there is nothing in the core that
> > prevents applications from potentially stalling the capture
> > operations.
> >
> > But I get your point, if the system needs 2 buffers to start
> > streaming, it will probably need two buffers to continue producing
> > frames.
> >
> > > That's not always true. The imx7-media-csi driver, for instance, sets
> > > min_queued_buffers to 2, but allocates scratch buffers and uses them at
> > > runtime, so that it can return all queued buffers to userspace.
> >
> > That's interesting. From your mention of "scratch buffers" I get there
> > actually is a need to have 2 buffers queued to the HW ? How does
> > that work, after all queuing a buffer to the DMA engine usually means
> > pointing its write engine to one (set of) addresses.
> >
> > Or is it a driver-only requirement to ask for two buffers ?
>
> The hardware has a ping-pong mechanism with two addresses and switches
> between them automatically.
>
> > > Grepping for min_queued_buffers I see drivers setting it to 4
> > > (rcar-dma.c, rzg2l-video.c), 6 (cxusb-analog.c) or even 9
> > > (zoran_driver.c) ! I doubt the zoran driver holds on to 9 buffers at
> > > runtime. Your statement is not universally true today?.
> > >
> > > This could be considered as driver issues, and the min_queued_buffers
> > > values should be fixed to match the runtime behaviour. In some cases I
> > > expect it will require more work than just changing the value, as
> > > drivers may implement the logic to operate with less buffers at runtime
> > > but not at start time. This would be fixable, but it may also call for
> > > asking if the start at runtime behaviours need to be identical.
> > >
> > > > So the application has to know somehow how many buffers are needed to
> > > > actually stream. One way is via VIDIOC_REQBUFS since that is supposed to
> > > > always return a workable number of buffers, the other is by actually
> > > > reporting the minimum number of buffers as per my RFC.
> > > >
> > > > > As you said an application should be good with> 3 buffers (one queued, one currently being written to, one to be
> > > > > consumed by the application), but in very specific cases where an
> > > > > application retains the buffer for longer, for whatever reason, it
> > > > > might need a larger number of queued buffers to provide the DMA
> > > > > engines a space where to write data without them being discarded (to
> > > > > scratch buffers or discarded by the DMA engine itself, if the HW
> > > > > supports that). Or maybe an application is fine to drop frames and
> > > > > only queue buffers sporadically (if the HW supports that ofc).
> > > > >
> > > > > For libcamera, and for this specific platform in particular, we're
> > > > > going to base new developments on the assumption that
> > > > > min_queued_buffers == 0, and it would be more convenient for use to be
> > > > > able to access its value from userspace to identify if we're running
> > > > > on a kernel with or without this patch being applied.
> > > >
> > > > So my proposal in my RFC to expose min_num_buffers would work for libcamera?
> > > > It sounds like that's what you need.
> >
> > My immediate need is to know if I'm running on a "legacy" version of
> > this driver that still requires 3 buffers for no apparent reason, or
> > on a new version. Your proposal might work, but I still feel like we
> > should report the HW/driver requirement (min_queued_buffers) instead
> > of trying to suggest applications how many buffers they need to
> > allocate to get "smooth streaming" or similar, as the use cases
> > might be different.
> >
> > > It may be useful, but I think we may also just require min_num_buffers
> > > == 0 for a device to be supported in libcamera. We have to implement
> >
> > While I concur this would be ideal, how would it work for existing
> > rkisp1 implementation that do not include this patch ? libcamera
> > should be able to run on both, probably in two different "modes" /o\
>
> A simple option is to check the kernel version, we do that in a few
> places. Over time we'll increase the minimum kernel version and drop

I considered that, but this patch is pretty easy to backport, having
something that tells to userspace the value of
min_queued_buffers might be useful indeed.

> support for legacy APIs.
>
> > > APIs such as the Android camera HAL that has no concept of buffers being
> > > kept by the device. This could possibly be handled within libcamera by
> > > allocating scratch buffers in userspace, but that comes with other
> > > challenges. I would like to at least try to get help from the kernel
> > > until proven that it's a bad idea.
>
> --
> Regards,
>
> Laurent Pinchart