diff mbox series

media: rkisp1: Reduce min_queued_buffers to 1

Message ID 20241007124225.63463-1-jacopo.mondi@ideasonboard.com (mailing list archive)
State New
Headers show
Series media: rkisp1: Reduce min_queued_buffers to 1 | expand

Commit Message

Jacopo Mondi Oct. 7, 2024, 12:42 p.m. UTC 
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 1 to allow applications to
operate with a single queued buffer.

Tested with libcamera, by operating with a single capture a 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@ideasonboard.com>
---

Adam,
   a few months ago you were exercizing your pinhole app with a single capture
request for StillCapture operations and you got the video device to hang because
no enough buffers where provided.

This small change should be enough to unblock you. Could you maybe give it a
spin if you're still working on this ?

Thanks
   j
---

 drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

--
2.46.1

Comments

Sebastian Fricke Oct. 7, 2024, 12:57 p.m. UTC | #1 
Hey Jacopo,

On 07.10.2024 14:42, 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 1 to allow applications to
>operate with a single queued buffer.
>
>Tested with libcamera, by operating with a single capture a 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@ideasonboard.com>
>---
>
>Adam,
>   a few months ago you were exercizing your pinhole app with a single capture
>request for StillCapture operations and you got the video device to hang because
>no enough buffers where provided.
>
>This small change should be enough to unblock you. Could you maybe give it a
>spin if you're still working on this ?
>
>Thanks
>   j
>---
>
> 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..34adaecdee54 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;

It looks like RKISP1_MIN_BUFFERS_NEEDED is used only here, so can you
remove the define as well?

rg 'RKISP1_MIN_BUFFERS_NEEDED' 
drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
38:#define RKISP1_MIN_BUFFERS_NEEDED 3
1566:	q->min_queued_buffers = RKISP1_MIN_BUFFERS_NEEDED;

Or maybe just change the value, but I am not sure whether this can be
considered a magic value.

Regards,
Sebastian Fricke

>+	q->min_queued_buffers = 1;
> 	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> 	q->lock = &node->vlock;
> 	q->dev = cap->rkisp1->dev;
>--
>2.46.1
>
>
Laurent Pinchart Oct. 7, 2024, 1:47 p.m. UTC | #2 
On Mon, Oct 07, 2024 at 02:57:30PM +0200, Sebastian Fricke wrote:
> Hey Jacopo,
> 
> On 07.10.2024 14:42, 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 1 to allow applications to
> > operate with a single queued buffer.
> > 
> > Tested with libcamera, by operating with a single capture a 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@ideasonboard.com>
> > ---
> > 
> > Adam,
> >    a few months ago you were exercizing your pinhole app with a single capture
> > request for StillCapture operations and you got the video device to hang because
> > no enough buffers where provided.
> > 
> > This small change should be enough to unblock you. Could you maybe give it a
> > spin if you're still working on this ?
> > 
> > Thanks
> >    j
> > ---
> > 
> >  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..34adaecdee54 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;
> 
> It looks like RKISP1_MIN_BUFFERS_NEEDED is used only here, so can you
> remove the define as well?

Isn't that exactly what this patch is doing ?

> rg 'RKISP1_MIN_BUFFERS_NEEDED' 
> drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> 38:#define RKISP1_MIN_BUFFERS_NEEDED 3
> 1566:	q->min_queued_buffers = RKISP1_MIN_BUFFERS_NEEDED;
> 
> Or maybe just change the value, but I am not sure whether this can be
> considered a magic value.
> 
> > +	q->min_queued_buffers = 1;
> >  	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> >  	q->lock = &node->vlock;
> >  	q->dev = cap->rkisp1->dev;
Laurent Pinchart Oct. 7, 2024, 1:55 p.m. UTC | #3 
Hi Jacopo,

Thank you for the patch.

On Mon, Oct 07, 2024 at 02:42:24PM +0200, 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 1 to allow applications to
> operate with a single queued buffer.
> 
> Tested with libcamera, by operating with a single capture a request. The

s/capture a/capture/

> 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@ideasonboard.com>
> ---
> 
> Adam,
>    a few months ago you were exercizing your pinhole app with a single capture
> request for StillCapture operations and you got the video device to hang because
> no enough buffers where provided.
> 	
> This small change should be enough to unblock you. Could you maybe give it a
> spin if you're still working on this ?
> 
> Thanks
>    j
> ---
> 
>  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..34adaecdee54 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 = 1;

min_queued_buffers controls two things in vb2:

- It controls the minimum number of buffers that can be allocated, by
  setting

        if (q->min_reqbufs_allocation < q->min_queued_buffers + 1)
                q->min_reqbufs_allocation = q->min_queued_buffers + 1;

  in vb2_core_queue_init(). Note that this ony impacts the
  VIDIOC_REQBUFS ioctl, VIDIOC_CREATE_BUFS can still allocate a lower
  number of buffers.

- It delays the .start_streaming() call until min_queued_buffers buffers
  have been queued.

This patch brings a clear improvement as it allows operating with a
single buffer. Ideally though, it would be nice to set
min_queued_buffers to 0, so that .start_streaming() gets called
synchronously with VIDIOC_STREAMON. Otherwise stream start errors can be
reported later, at VIDIOC_QBUF time.

I expect going for 0 will require more changes in the driver, so I'm
fine merging this patch as-is as a first step.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>  	q->lock = &node->vlock;
>  	q->dev = cap->rkisp1->dev;
Sebastian Fricke Oct. 7, 2024, 2:05 p.m. UTC | #4 
On 07.10.2024 16:47, Laurent Pinchart wrote:
>On Mon, Oct 07, 2024 at 02:57:30PM +0200, Sebastian Fricke wrote:
>> Hey Jacopo,
>>
>> On 07.10.2024 14:42, 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 1 to allow applications to
>> > operate with a single queued buffer.
>> >
>> > Tested with libcamera, by operating with a single capture a 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@ideasonboard.com>
>> > ---
>> >
>> > Adam,
>> >    a few months ago you were exercizing your pinhole app with a single capture
>> > request for StillCapture operations and you got the video device to hang because
>> > no enough buffers where provided.
>> >
>> > This small change should be enough to unblock you. Could you maybe give it a
>> > spin if you're still working on this ?
>> >
>> > Thanks
>> >    j
>> > ---
>> >
>> >  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..34adaecdee54 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;
>>
>> It looks like RKISP1_MIN_BUFFERS_NEEDED is used only here, so can you
>> remove the define as well?
>
>Isn't that exactly what this patch is doing ?

Oh *facepalm* ... I missed that please disregard ...

but my question below remains whether to not just change the value.

Sorry,
Sebastian

>
>> rg 'RKISP1_MIN_BUFFERS_NEEDED'
>> drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
>> 38:#define RKISP1_MIN_BUFFERS_NEEDED 3
>> 1566:	q->min_queued_buffers = RKISP1_MIN_BUFFERS_NEEDED;
>>
>> Or maybe just change the value, but I am not sure whether this can be
>> considered a magic value.
>>
>> > +	q->min_queued_buffers = 1;
>> >  	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>> >  	q->lock = &node->vlock;
>> >  	q->dev = cap->rkisp1->dev;
>
>-- 
>Regards,
>
>Laurent Pinchart
Sebastian Fricke
Consultant Software Engineer

Collabora Ltd
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
Registered in England & Wales no 5513718.
Jacopo Mondi Oct. 7, 2024, 7:35 p.m. UTC | #5 
Hi Sebastian,

On Mon, Oct 07, 2024 at 04:05:01PM GMT, Sebastian Fricke wrote:
> On 07.10.2024 16:47, Laurent Pinchart wrote:
> > On Mon, Oct 07, 2024 at 02:57:30PM +0200, Sebastian Fricke wrote:
> > > Hey Jacopo,
> > >
> > > On 07.10.2024 14:42, 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 1 to allow applications to
> > > > operate with a single queued buffer.
> > > >
> > > > Tested with libcamera, by operating with a single capture a 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@ideasonboard.com>
> > > > ---
> > > >
> > > > Adam,
> > > >    a few months ago you were exercizing your pinhole app with a single capture
> > > > request for StillCapture operations and you got the video device to hang because
> > > > no enough buffers where provided.
> > > >
> > > > This small change should be enough to unblock you. Could you maybe give it a
> > > > spin if you're still working on this ?
> > > >
> > > > Thanks
> > > >    j
> > > > ---
> > > >
> > > >  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..34adaecdee54 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;
> > >
> > > It looks like RKISP1_MIN_BUFFERS_NEEDED is used only here, so can you
> > > remove the define as well?
> >
> > Isn't that exactly what this patch is doing ?
>
> Oh *facepalm* ... I missed that please disregard ...
>
> but my question below remains whether to not just change the value.

Do you mean

-#define RKISP1_MIN_BUFFERS_NEEDED 3
+#define RKISP1_MIN_BUFFERS_NEEDED 1

?

I would rather avoid defining a value used in a single place. If it
was some magic number a macro name would maybe help giving come
context, but considering this is assigned to min_queued_buffers it's
imho clear enough ?

>
> Sorry,
> Sebastian
>
> >
> > > rg 'RKISP1_MIN_BUFFERS_NEEDED'
> > > drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > > 38:#define RKISP1_MIN_BUFFERS_NEEDED 3
> > > 1566:	q->min_queued_buffers = RKISP1_MIN_BUFFERS_NEEDED;
> > >
> > > Or maybe just change the value, but I am not sure whether this can be
> > > considered a magic value.
> > >
> > > > +	q->min_queued_buffers = 1;
> > > >  	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> > > >  	q->lock = &node->vlock;
> > > >  	q->dev = cap->rkisp1->dev;
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
> Sebastian Fricke
> Consultant Software Engineer
>
> Collabora Ltd
> Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
> Registered in England & Wales no 5513718.
Laurent Pinchart Oct. 7, 2024, 7:49 p.m. UTC | #6 
On Mon, Oct 07, 2024 at 09:35:49PM +0200, Jacopo Mondi wrote:
> Hi Sebastian,
> 
> On Mon, Oct 07, 2024 at 04:05:01PM GMT, Sebastian Fricke wrote:
> > On 07.10.2024 16:47, Laurent Pinchart wrote:
> > > On Mon, Oct 07, 2024 at 02:57:30PM +0200, Sebastian Fricke wrote:
> > > > Hey Jacopo,
> > > >
> > > > On 07.10.2024 14:42, 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 1 to allow applications to
> > > > > operate with a single queued buffer.
> > > > >
> > > > > Tested with libcamera, by operating with a single capture a 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@ideasonboard.com>
> > > > > ---
> > > > >
> > > > > Adam,
> > > > >    a few months ago you were exercizing your pinhole app with a single capture
> > > > > request for StillCapture operations and you got the video device to hang because
> > > > > no enough buffers where provided.
> > > > >
> > > > > This small change should be enough to unblock you. Could you maybe give it a
> > > > > spin if you're still working on this ?
> > > > >
> > > > > Thanks
> > > > >    j
> > > > > ---
> > > > >
> > > > >  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..34adaecdee54 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;
> > > >
> > > > It looks like RKISP1_MIN_BUFFERS_NEEDED is used only here, so can you
> > > > remove the define as well?
> > >
> > > Isn't that exactly what this patch is doing ?
> >
> > Oh *facepalm* ... I missed that please disregard ...
> >
> > but my question below remains whether to not just change the value.
> 
> Do you mean
> 
> -#define RKISP1_MIN_BUFFERS_NEEDED 3
> +#define RKISP1_MIN_BUFFERS_NEEDED 1
> 
> ?
> 
> I would rather avoid defining a value used in a single place. If it
> was some magic number a macro name would maybe help giving come
> context, but considering this is assigned to min_queued_buffers it's
> imho clear enough ?

I find it clear enough, I prefer dropping the macro as you do in this
patch.

> > > > rg 'RKISP1_MIN_BUFFERS_NEEDED'
> > > > drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > > > 38:#define RKISP1_MIN_BUFFERS_NEEDED 3
> > > > 1566:	q->min_queued_buffers = RKISP1_MIN_BUFFERS_NEEDED;
> > > >
> > > > Or maybe just change the value, but I am not sure whether this can be
> > > > considered a magic value.
> > > >
> > > > > +	q->min_queued_buffers = 1;
> > > > >  	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> > > > >  	q->lock = &node->vlock;
> > > > >  	q->dev = cap->rkisp1->dev;
Sebastian Fricke Oct. 8, 2024, 6:49 a.m. UTC | #7 
On 07.10.2024 22:49, Laurent Pinchart wrote:
>On Mon, Oct 07, 2024 at 09:35:49PM +0200, Jacopo Mondi wrote:
>> Hi Sebastian,
>>
>> On Mon, Oct 07, 2024 at 04:05:01PM GMT, Sebastian Fricke wrote:
>> > On 07.10.2024 16:47, Laurent Pinchart wrote:
>> > > On Mon, Oct 07, 2024 at 02:57:30PM +0200, Sebastian Fricke wrote:
>> > > > Hey Jacopo,
>> > > >
>> > > > On 07.10.2024 14:42, 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 1 to allow applications to
>> > > > > operate with a single queued buffer.
>> > > > >
>> > > > > Tested with libcamera, by operating with a single capture a 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@ideasonboard.com>
>> > > > > ---
>> > > > >
>> > > > > Adam,
>> > > > >    a few months ago you were exercizing your pinhole app with a single capture
>> > > > > request for StillCapture operations and you got the video device to hang because
>> > > > > no enough buffers where provided.
>> > > > >
>> > > > > This small change should be enough to unblock you. Could you maybe give it a
>> > > > > spin if you're still working on this ?
>> > > > >
>> > > > > Thanks
>> > > > >    j
>> > > > > ---
>> > > > >
>> > > > >  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..34adaecdee54 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;
>> > > >
>> > > > It looks like RKISP1_MIN_BUFFERS_NEEDED is used only here, so can you
>> > > > remove the define as well?
>> > >
>> > > Isn't that exactly what this patch is doing ?
>> >
>> > Oh *facepalm* ... I missed that please disregard ...
>> >
>> > but my question below remains whether to not just change the value.
>>
>> Do you mean
>>
>> -#define RKISP1_MIN_BUFFERS_NEEDED 3
>> +#define RKISP1_MIN_BUFFERS_NEEDED 1
>>
>> ?
>>
>> I would rather avoid defining a value used in a single place. If it
>> was some magic number a macro name would maybe help giving come
>> context, but considering this is assigned to min_queued_buffers it's
>> imho clear enough ?
>
>I find it clear enough, I prefer dropping the macro as you do in this
>patch.

Sounds good.

>
>> > > > rg 'RKISP1_MIN_BUFFERS_NEEDED'
>> > > > drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
>> > > > 38:#define RKISP1_MIN_BUFFERS_NEEDED 3
>> > > > 1566:	q->min_queued_buffers = RKISP1_MIN_BUFFERS_NEEDED;
>> > > >
>> > > > Or maybe just change the value, but I am not sure whether this can be
>> > > > considered a magic value.
>> > > >
>> > > > > +	q->min_queued_buffers = 1;
>> > > > >  	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>> > > > >  	q->lock = &node->vlock;
>> > > > >  	q->dev = cap->rkisp1->dev;
>
>-- 
>Regards,
>
>Laurent Pinchart
>
Sebastian Fricke
Consultant Software Engineer

Collabora Ltd
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
Registered in England & Wales no 5513718.
Jacopo Mondi Oct. 9, 2024, 10:09 a.m. UTC | #8 
Hi Laurent

On Mon, Oct 07, 2024 at 04:55:32PM GMT, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, Oct 07, 2024 at 02:42:24PM +0200, 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 1 to allow applications to
> > operate with a single queued buffer.
> >
> > Tested with libcamera, by operating with a single capture a request. The
>
> s/capture a/capture/
>
> > 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@ideasonboard.com>
> > ---
> >
> > Adam,
> >    a few months ago you were exercizing your pinhole app with a single capture
> > request for StillCapture operations and you got the video device to hang because
> > no enough buffers where provided.
> >
> > This small change should be enough to unblock you. Could you maybe give it a
> > spin if you're still working on this ?
> >
> > Thanks
> >    j
> > ---
> >
> >  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..34adaecdee54 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 = 1;
>
> min_queued_buffers controls two things in vb2:
>
> - It controls the minimum number of buffers that can be allocated, by
>   setting
>
>         if (q->min_reqbufs_allocation < q->min_queued_buffers + 1)
>                 q->min_reqbufs_allocation = q->min_queued_buffers + 1;
>
>   in vb2_core_queue_init(). Note that this ony impacts the
>   VIDIOC_REQBUFS ioctl, VIDIOC_CREATE_BUFS can still allocate a lower
>   number of buffers.
>
> - It delays the .start_streaming() call until min_queued_buffers buffers
>   have been queued.
>
> This patch brings a clear improvement as it allows operating with a
> single buffer. Ideally though, it would be nice to set
> min_queued_buffers to 0, so that .start_streaming() gets called
> synchronously with VIDIOC_STREAMON. Otherwise stream start errors can be
> reported later, at VIDIOC_QBUF time.
>
> I expect going for 0 will require more changes in the driver, so I'm
> fine merging this patch as-is as a first step.

Well well well, I tried setting it to 0 and not provide any buffer to
the video device.

I see the number of interrupts received from the rkisp1 driver
increase with a frequency compatible with the frame rate, so it might
be possible that things work without modifications.

I'll keep digging

>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> >  	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> >  	q->lock = &node->vlock;
> >  	q->dev = cap->rkisp1->dev;
>
> --
> Regards,
>
> Laurent Pinchart
diff mbox series

Patch

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
index 2bddb4fa8a5c..34adaecdee54 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 = 1;
 	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 	q->lock = &node->vlock;
 	q->dev = cap->rkisp1->dev;