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

[Hans Verkuil]

On 28/10/2024 16:30, Jacopo Mondi wrote:
> Hi Hans
> 
> 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.

> 
> 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.

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.

Regards,

	Hans