Hello
On Tue, Oct 29, 2024 at 09:21:16AM +0100, Jacopo Mondi wrote:
> There apparently is no reason to require 3 queued buffers for RkISP1,
> as the driver operates with a scratch buffer where data can be
> directed to if there's no available buffer provided by userspace.
>
> Reduce the number of required buffers to 0 by removing the
> initialization of min_queued_buffers, 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 the 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@xxxxxxxxxxxxxxxx>
I just noticed v2 of this series:
media: rkisp1: Reduce min_queued_buffers to 1
has been collected instead of this v3.
And I noticed because a user complained to me about this.
Now, I can provide an update based on the now merged v2, not a big
deal, but this depresses me a bit as the discussion about
implementing multi-commiter model is apparently (again) stalled.
I know, sh*t happens (TM) and hiccups are expected in the process,
we all make mistakes and I'm not even sure through which path the
patch has been collected, but I could have handled this one easily,
and instead what we have is:
1) an unhappy user that will likely have to wait for the next release
2) me having to send an additional (rather trivial) patch
3) Someone will have to review, collect, PR etc etc
(and I'm not even mentioning this patch is 3 lines)
Issues like this one seems to be considered a fact of life we decided
is fine to live with, while every possible corner case of the proposed
multi-committer model is analyzed with great concern like we're
trading a perfect model for something that has to be equally perfect.
And while I agree the biggest reason for the proverbial v4l2 slow pace
is the reviewers scarcity and the limited maintainers bandwidth, now
that we have everything in place to reduce the system clogginess
it still seems we're not all sold for it. I really don't get it, sorry.
> ---
> v2->v3:
> - Remove min_queued_buffers initialization
>
> v1->v2:
> 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 | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> index 2bddb4fa8a5c..2f0c610e74b9 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,6 @@ 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->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> q->lock = &node->vlock;
> q->dev = cap->rkisp1->dev;
> --
> 2.47.0
>
>
