* [PATCH] media: rkisp1: Reduce min_queued_buffers to 1 @ 2024-10-07 12:42 Jacopo Mondi 2024-10-07 12:57 ` Sebastian Fricke 2024-10-07 13:55 ` Laurent Pinchart 0 siblings, 2 replies; 9+ messages in thread From: Jacopo Mondi @ 2024-10-07 12:42 UTC (permalink / raw) To: Dafna Hirschfeld, Laurent Pinchart, open list:ROCKCHIP ISP V1 DRIVER Cc: Jacopo Mondi, Mauro Carvalho Chehab, Heiko Stuebner, open list:ROCKCHIP ISP V1 DRIVER, Adam Pigg 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; + 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 _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] media: rkisp1: Reduce min_queued_buffers to 1 2024-10-07 12:42 [PATCH] media: rkisp1: Reduce min_queued_buffers to 1 Jacopo Mondi @ 2024-10-07 12:57 ` Sebastian Fricke 2024-10-07 13:47 ` Laurent Pinchart 2024-10-07 13:55 ` Laurent Pinchart 1 sibling, 1 reply; 9+ messages in thread From: Sebastian Fricke @ 2024-10-07 12:57 UTC (permalink / raw) To: Jacopo Mondi Cc: Dafna Hirschfeld, Laurent Pinchart, open list:ROCKCHIP ISP V1 DRIVER, Mauro Carvalho Chehab, Heiko Stuebner, open list:ROCKCHIP ISP V1 DRIVER, Adam Pigg 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 > > _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] media: rkisp1: Reduce min_queued_buffers to 1 2024-10-07 12:57 ` Sebastian Fricke @ 2024-10-07 13:47 ` Laurent Pinchart 2024-10-07 14:05 ` Sebastian Fricke 0 siblings, 1 reply; 9+ messages in thread From: Laurent Pinchart @ 2024-10-07 13:47 UTC (permalink / raw) To: Sebastian Fricke Cc: Jacopo Mondi, Dafna Hirschfeld, open list:ROCKCHIP ISP V1 DRIVER, Mauro Carvalho Chehab, Heiko Stuebner, open list:ROCKCHIP ISP V1 DRIVER, Adam Pigg 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; -- Regards, Laurent Pinchart _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] media: rkisp1: Reduce min_queued_buffers to 1 2024-10-07 13:47 ` Laurent Pinchart @ 2024-10-07 14:05 ` Sebastian Fricke 2024-10-07 19:35 ` Jacopo Mondi 0 siblings, 1 reply; 9+ messages in thread From: Sebastian Fricke @ 2024-10-07 14:05 UTC (permalink / raw) To: Laurent Pinchart Cc: Jacopo Mondi, Dafna Hirschfeld, open list:ROCKCHIP ISP V1 DRIVER, Mauro Carvalho Chehab, Heiko Stuebner, open list:ROCKCHIP ISP V1 DRIVER, Adam Pigg 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. _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] media: rkisp1: Reduce min_queued_buffers to 1 2024-10-07 14:05 ` Sebastian Fricke @ 2024-10-07 19:35 ` Jacopo Mondi 2024-10-07 19:49 ` Laurent Pinchart 0 siblings, 1 reply; 9+ messages in thread From: Jacopo Mondi @ 2024-10-07 19:35 UTC (permalink / raw) To: Sebastian Fricke Cc: Laurent Pinchart, Jacopo Mondi, Dafna Hirschfeld, open list:ROCKCHIP ISP V1 DRIVER, Mauro Carvalho Chehab, Heiko Stuebner, open list:ROCKCHIP ISP V1 DRIVER, Adam Pigg 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. _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] media: rkisp1: Reduce min_queued_buffers to 1 2024-10-07 19:35 ` Jacopo Mondi @ 2024-10-07 19:49 ` Laurent Pinchart 2024-10-08 6:49 ` Sebastian Fricke 0 siblings, 1 reply; 9+ messages in thread From: Laurent Pinchart @ 2024-10-07 19:49 UTC (permalink / raw) To: Jacopo Mondi Cc: Sebastian Fricke, Dafna Hirschfeld, open list:ROCKCHIP ISP V1 DRIVER, Mauro Carvalho Chehab, Heiko Stuebner, open list:ROCKCHIP ISP V1 DRIVER, Adam Pigg 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; -- Regards, Laurent Pinchart _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] media: rkisp1: Reduce min_queued_buffers to 1 2024-10-07 19:49 ` Laurent Pinchart @ 2024-10-08 6:49 ` Sebastian Fricke 0 siblings, 0 replies; 9+ messages in thread From: Sebastian Fricke @ 2024-10-08 6:49 UTC (permalink / raw) To: Laurent Pinchart Cc: Jacopo Mondi, Dafna Hirschfeld, open list:ROCKCHIP ISP V1 DRIVER, Mauro Carvalho Chehab, Heiko Stuebner, open list:ROCKCHIP ISP V1 DRIVER, Adam Pigg 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. _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] media: rkisp1: Reduce min_queued_buffers to 1 2024-10-07 12:42 [PATCH] media: rkisp1: Reduce min_queued_buffers to 1 Jacopo Mondi 2024-10-07 12:57 ` Sebastian Fricke @ 2024-10-07 13:55 ` Laurent Pinchart 2024-10-09 10:09 ` Jacopo Mondi 1 sibling, 1 reply; 9+ messages in thread From: Laurent Pinchart @ 2024-10-07 13:55 UTC (permalink / raw) To: Jacopo Mondi Cc: Dafna Hirschfeld, open list:ROCKCHIP ISP V1 DRIVER, Mauro Carvalho Chehab, Heiko Stuebner, open list:ROCKCHIP ISP V1 DRIVER, Adam Pigg 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; -- Regards, Laurent Pinchart _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] media: rkisp1: Reduce min_queued_buffers to 1 2024-10-07 13:55 ` Laurent Pinchart @ 2024-10-09 10:09 ` Jacopo Mondi 0 siblings, 0 replies; 9+ messages in thread From: Jacopo Mondi @ 2024-10-09 10:09 UTC (permalink / raw) To: Laurent Pinchart Cc: Jacopo Mondi, Dafna Hirschfeld, open list:ROCKCHIP ISP V1 DRIVER, Mauro Carvalho Chehab, Heiko Stuebner, open list:ROCKCHIP ISP V1 DRIVER, Adam Pigg 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 _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-10-09 10:12 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-10-07 12:42 [PATCH] media: rkisp1: Reduce min_queued_buffers to 1 Jacopo Mondi 2024-10-07 12:57 ` Sebastian Fricke 2024-10-07 13:47 ` Laurent Pinchart 2024-10-07 14:05 ` Sebastian Fricke 2024-10-07 19:35 ` Jacopo Mondi 2024-10-07 19:49 ` Laurent Pinchart 2024-10-08 6:49 ` Sebastian Fricke 2024-10-07 13:55 ` Laurent Pinchart 2024-10-09 10:09 ` Jacopo Mondi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox