* [PATCH v3] media: rkisp1: Remove min_queued_buffers @ 2024-10-29 8:21 Jacopo Mondi 2024-10-29 11:29 ` Laurent Pinchart 2025-02-20 14:22 ` Jacopo Mondi 0 siblings, 2 replies; 11+ messages in thread From: Jacopo Mondi @ 2024-10-29 8:21 UTC (permalink / raw) To: Dafna Hirschfeld, Laurent Pinchart, Hans Verkuil, 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 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@ideasonboard.com> --- 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 _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3] media: rkisp1: Remove min_queued_buffers 2024-10-29 8:21 [PATCH v3] media: rkisp1: Remove min_queued_buffers Jacopo Mondi @ 2024-10-29 11:29 ` Laurent Pinchart 2024-10-29 11:50 ` Hans Verkuil 2025-02-20 14:22 ` Jacopo Mondi 1 sibling, 1 reply; 11+ messages in thread From: Laurent Pinchart @ 2024-10-29 11:29 UTC (permalink / raw) To: Jacopo Mondi Cc: Dafna Hirschfeld, Hans Verkuil, 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 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@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > 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; -- 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] 11+ messages in thread
* Re: [PATCH v3] media: rkisp1: Remove min_queued_buffers 2024-10-29 11:29 ` Laurent Pinchart @ 2024-10-29 11:50 ` Hans Verkuil 0 siblings, 0 replies; 11+ messages in thread From: Hans Verkuil @ 2024-10-29 11:50 UTC (permalink / raw) To: Laurent Pinchart, Jacopo Mondi Cc: Dafna Hirschfeld, open list:ROCKCHIP ISP V1 DRIVER, Mauro Carvalho Chehab, Heiko Stuebner, open list:ROCKCHIP ISP V1 DRIVER, Adam Pigg On 10/29/24 12:29, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > 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@ideasonboard.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Hans Verkuil <hverkuil@xs4all.nl> Regards, Hans > >> --- >> 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; > _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] media: rkisp1: Remove min_queued_buffers 2024-10-29 8:21 [PATCH v3] media: rkisp1: Remove min_queued_buffers Jacopo Mondi 2024-10-29 11:29 ` Laurent Pinchart @ 2025-02-20 14:22 ` Jacopo Mondi 2025-02-20 14:50 ` Laurent Pinchart ` (2 more replies) 1 sibling, 3 replies; 11+ messages in thread From: Jacopo Mondi @ 2025-02-20 14:22 UTC (permalink / raw) To: Jacopo Mondi, Hans Verkuil, Mauro Carvalho Chehab, Laurent Pinchart, Sakari Ailus Cc: Dafna Hirschfeld, Laurent Pinchart, Hans Verkuil, open list:ROCKCHIP ISP V1 DRIVER, Mauro Carvalho Chehab, Heiko Stuebner, open list:ROCKCHIP ISP V1 DRIVER, Adam Pigg 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@ideasonboard.com> 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 > > _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] media: rkisp1: Remove min_queued_buffers 2025-02-20 14:22 ` Jacopo Mondi @ 2025-02-20 14:50 ` Laurent Pinchart 2025-02-20 15:33 ` Hans Verkuil 2025-02-20 15:39 ` Hans Verkuil 2 siblings, 0 replies; 11+ messages in thread From: Laurent Pinchart @ 2025-02-20 14:50 UTC (permalink / raw) To: Jacopo Mondi Cc: Hans Verkuil, Mauro Carvalho Chehab, Sakari Ailus, Dafna Hirschfeld, open list:ROCKCHIP ISP V1 DRIVER, Heiko Stuebner, open list:ROCKCHIP ISP V1 DRIVER, Adam Pigg Hi Jacopo, On Thu, Feb 20, 2025 at 03:22:44PM +0100, Jacopo Mondi wrote: > 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@ideasonboard.com> > > I just noticed v2 of this series: > media: rkisp1: Reduce min_queued_buffers to 1 > > has been collected instead of this v3. That's my fault, I apologize. > 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. Amen. > > --- > > 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; -- 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] 11+ messages in thread
* Re: [PATCH v3] media: rkisp1: Remove min_queued_buffers 2025-02-20 14:22 ` Jacopo Mondi 2025-02-20 14:50 ` Laurent Pinchart @ 2025-02-20 15:33 ` Hans Verkuil 2025-02-20 16:38 ` Laurent Pinchart 2025-02-20 15:39 ` Hans Verkuil 2 siblings, 1 reply; 11+ messages in thread From: Hans Verkuil @ 2025-02-20 15:33 UTC (permalink / raw) To: Jacopo Mondi, Mauro Carvalho Chehab, Laurent Pinchart, Sakari Ailus Cc: Dafna Hirschfeld, open list:ROCKCHIP ISP V1 DRIVER, Heiko Stuebner, open list:ROCKCHIP ISP V1 DRIVER, Adam Pigg On 2/20/25 15:22, Jacopo Mondi wrote: > 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@ideasonboard.com> > > 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. The main gitlab blocker is that we need a merge bot. Ricardo said in the last meeting that he hopes to have that in a month. The other blocker is of course the gitlab freedesktop migration to another provider. Without a merge bot even with just two committers Mauro and myself are frequently stepping on each others toes, so that really is needed. Heck, if I have two CI pipelines in flight, I'm stepping on my own toes! Hopefully a merge bot should fix this issue. Otherwise it is going quite well on the gitlab front. The other is the "Document the new media-committer's model" series. I think Laurent scared Mauro off with his last set of comments. Regards, Hans > > >> --- >> 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 >> >> _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] media: rkisp1: Remove min_queued_buffers 2025-02-20 15:33 ` Hans Verkuil @ 2025-02-20 16:38 ` Laurent Pinchart 0 siblings, 0 replies; 11+ messages in thread From: Laurent Pinchart @ 2025-02-20 16:38 UTC (permalink / raw) To: Hans Verkuil Cc: Jacopo Mondi, Mauro Carvalho Chehab, Sakari Ailus, Dafna Hirschfeld, open list:ROCKCHIP ISP V1 DRIVER, Heiko Stuebner, open list:ROCKCHIP ISP V1 DRIVER, Adam Pigg On Thu, Feb 20, 2025 at 04:33:28PM +0100, Hans Verkuil wrote: > On 2/20/25 15:22, Jacopo Mondi wrote: > > 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@ideasonboard.com> > > > > 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. > > The main gitlab blocker is that we need a merge bot. Ricardo said in the > last meeting that he hopes to have that in a month. The other blocker is > of course the gitlab freedesktop migration to another provider. > > Without a merge bot even with just two committers Mauro and myself are > frequently stepping on each others toes, so that really is needed. Heck, if > I have two CI pipelines in flight, I'm stepping on my own toes! Hopefully a > merge bot should fix this issue. > > Otherwise it is going quite well on the gitlab front. > > The other is the "Document the new media-committer's model" series. I > think Laurent scared Mauro off with his last set of comments. I think there may be less fundamental disagreements that one may fear, and that part of the trouble is caused by misunderstandings. E-mail communication makes it worse, I'd like to reiterate an invitation to discuss this in a video call. Decisions will still be made on the list, with everybody involved, but explaining our positions live will I think improve mutual understanding. > >> --- > >> 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; -- 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] 11+ messages in thread
* Re: [PATCH v3] media: rkisp1: Remove min_queued_buffers 2025-02-20 14:22 ` Jacopo Mondi 2025-02-20 14:50 ` Laurent Pinchart 2025-02-20 15:33 ` Hans Verkuil @ 2025-02-20 15:39 ` Hans Verkuil 2025-02-20 16:27 ` Jacopo Mondi 2025-02-20 16:34 ` Laurent Pinchart 2 siblings, 2 replies; 11+ messages in thread From: Hans Verkuil @ 2025-02-20 15:39 UTC (permalink / raw) To: Jacopo Mondi, Mauro Carvalho Chehab, Laurent Pinchart, Sakari Ailus Cc: Dafna Hirschfeld, open list:ROCKCHIP ISP V1 DRIVER, Heiko Stuebner, open list:ROCKCHIP ISP V1 DRIVER, Adam Pigg On 2/20/25 15:22, Jacopo Mondi wrote: > 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@ideasonboard.com> > > I just noticed v2 of this series: > media: rkisp1: Reduce min_queued_buffers to 1 > > has been collected instead of this v3. Did you mark your v2 as Superseded in patchwork when you posted the v3? I marked your v1 and v2 as Superseded today when I was cleaning up patchwork, so I know you didn't :-) When I post a new version I always mark the old one as Superseded, exactly to prevent such things from happening. Also, more people should help keep patchwork clean. I think I am usually the person who is doing this, but this is a collective responsibility. Part of the job description for someone with commit rights is actually to keep patchwork clean. Regards, Hans > > 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 >> >> _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] media: rkisp1: Remove min_queued_buffers 2025-02-20 15:39 ` Hans Verkuil @ 2025-02-20 16:27 ` Jacopo Mondi 2025-02-20 17:01 ` Hans Verkuil 2025-02-20 16:34 ` Laurent Pinchart 1 sibling, 1 reply; 11+ messages in thread From: Jacopo Mondi @ 2025-02-20 16:27 UTC (permalink / raw) To: Hans Verkuil Cc: Jacopo Mondi, Mauro Carvalho Chehab, Laurent Pinchart, Sakari Ailus, Dafna Hirschfeld, open list:ROCKCHIP ISP V1 DRIVER, Heiko Stuebner, open list:ROCKCHIP ISP V1 DRIVER, Adam Pigg Hi Hans On Thu, Feb 20, 2025 at 04:39:59PM +0100, Hans Verkuil wrote: > On 2/20/25 15:22, Jacopo Mondi wrote: > > 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@ideasonboard.com> > > > > I just noticed v2 of this series: > > media: rkisp1: Reduce min_queued_buffers to 1 > > > > has been collected instead of this v3. > > Did you mark your v2 as Superseded in patchwork when you posted the v3? > I marked your v1 and v2 as Superseded today when I was cleaning up patchwork, > so I know you didn't :-) > > When I post a new version I always mark the old one as Superseded, exactly > to prevent such things from happening. > > Also, more people should help keep patchwork clean. I think I am usually the > person who is doing this, but this is a collective responsibility. Yes, and it's a thankless job. So thank you. > > Part of the job description for someone with commit rights is actually to > keep patchwork clean. > Part of the motivation to do admin tasks around patch handling actually comes from a feeling of ownership and responsibility. If one is given trust and responsibilities then doing admin tasks just becomes part of what you do as you get through the day. Don't get me wrong, this is not an excuse for being sluggish, but in the current process I'm not even sure if it's my responsibility to go through patchwork. In the end, right now, once I've sent a patch and replied to comments, then it's not "my business" anymore. Right ? Wrong ? Not sure, but I'm rather certain that if people is given ownership of something they will be motivated to care about it. Anyway, I would like to use this little hiccups as an example of something that went wrong (for $reasons) in the current model, and which requires energies from both submitter and maintainers to be corrected. All the discussion we had have gone into depicting doomsday scenarios potentially caused by the new model, but I have the feeling maintainers themselves sometimes do not appreciate the burden the current process inflicts on them. That's why I'm a little surprised this taking so long, as the first ones that should want this happening are the people which are doing most of thankless and underappreciated housekeeping work that keeps the subsystem functional. Thank you! > Regards, > > Hans > > > > > 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 > >> > >> > _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] media: rkisp1: Remove min_queued_buffers 2025-02-20 16:27 ` Jacopo Mondi @ 2025-02-20 17:01 ` Hans Verkuil 0 siblings, 0 replies; 11+ messages in thread From: Hans Verkuil @ 2025-02-20 17:01 UTC (permalink / raw) To: Jacopo Mondi Cc: Mauro Carvalho Chehab, Laurent Pinchart, Sakari Ailus, Dafna Hirschfeld, open list:ROCKCHIP ISP V1 DRIVER, Heiko Stuebner, open list:ROCKCHIP ISP V1 DRIVER, Adam Pigg On 20/02/2025 17:27, Jacopo Mondi wrote: > Hi Hans > > On Thu, Feb 20, 2025 at 04:39:59PM +0100, Hans Verkuil wrote: >> On 2/20/25 15:22, Jacopo Mondi wrote: >>> 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@ideasonboard.com> >>> >>> I just noticed v2 of this series: >>> media: rkisp1: Reduce min_queued_buffers to 1 >>> >>> has been collected instead of this v3. >> >> Did you mark your v2 as Superseded in patchwork when you posted the v3? >> I marked your v1 and v2 as Superseded today when I was cleaning up patchwork, >> so I know you didn't :-) >> >> When I post a new version I always mark the old one as Superseded, exactly >> to prevent such things from happening. >> > >> Also, more people should help keep patchwork clean. I think I am usually the >> person who is doing this, but this is a collective responsibility. > > Yes, and it's a thankless job. So thank you. > >> >> Part of the job description for someone with commit rights is actually to >> keep patchwork clean. >> > > Part of the motivation to do admin tasks around patch handling > actually comes from a feeling of ownership and responsibility. If > one is given trust and responsibilities then doing admin tasks just > becomes part of what you do as you get through the day. > > Don't get me wrong, this is not an excuse for being sluggish, but in > the current process I'm not even sure if it's my responsibility to go > through patchwork. In the end, right now, once I've sent a patch and > replied to comments, then it's not "my business" anymore. Right ? > Wrong ? Not sure, but I'm rather certain that if people is given > ownership of something they will be motivated to care about it. Let me put it this way: if active media developers who frequently post patches would also update patchwork whenever they post new versions, then maintainers would greatly appreciate it. It is not a requirement, though. To be honest, I kind of expected you to do that already, but perhaps nobody ever asked or mentioned it? You should be able to update the state of your own patches in patchwork (although you might have to make an account first, I'm not sure). It is, however, a requirement for maintainers with delegation rights in patchwork. Certainly for the area they maintain. Because if it isn't kept up to date it becomes useless. > > Anyway, I would like to use this little hiccups as an example of > something that went wrong (for $reasons) in the current model, and > which requires energies from both submitter and maintainers to be > corrected. All the discussion we had have gone into depicting doomsday > scenarios potentially caused by the new model, but I have the feeling > maintainers themselves sometimes do not appreciate the burden the > current process inflicts on them. That's why I'm a little surprised > this taking so long, as the first ones that should want this happening > are the people which are doing most of thankless and underappreciated > housekeeping work that keeps the subsystem functional. It definitely takes longer than expected. There were a lot of issues all over the place that needed to be fixed. But there is no point in starting this too soon and have it end up into chaos. The CI in particular already greatly helps me: I don't need to go back to the patch author as often as I used to to ask for changes caught by my scripts, since now it has already been caught by the CI. The lack of serializing merge requests is from a technical standpoint the only blocker to enabling multiple committers. Regards, Hans > > Thank you! > > >> Regards, >> >> Hans >> >>> >>> 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 >>>> >>>> >> _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] media: rkisp1: Remove min_queued_buffers 2025-02-20 15:39 ` Hans Verkuil 2025-02-20 16:27 ` Jacopo Mondi @ 2025-02-20 16:34 ` Laurent Pinchart 1 sibling, 0 replies; 11+ messages in thread From: Laurent Pinchart @ 2025-02-20 16:34 UTC (permalink / raw) To: Hans Verkuil Cc: Jacopo Mondi, Mauro Carvalho Chehab, Sakari Ailus, Dafna Hirschfeld, open list:ROCKCHIP ISP V1 DRIVER, Heiko Stuebner, open list:ROCKCHIP ISP V1 DRIVER, Adam Pigg On Thu, Feb 20, 2025 at 04:39:59PM +0100, Hans Verkuil wrote: > On 2/20/25 15:22, Jacopo Mondi wrote: > > 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@ideasonboard.com> > > > > I just noticed v2 of this series: > > media: rkisp1: Reduce min_queued_buffers to 1 > > > > has been collected instead of this v3. > > Did you mark your v2 as Superseded in patchwork when you posted the v3? > I marked your v1 and v2 as Superseded today when I was cleaning up patchwork, > so I know you didn't :-) > > When I post a new version I always mark the old one as Superseded, exactly > to prevent such things from happening. That won't be possible in all cases, not all patch submitters have patchwork accounts. In libcamera we use a patchwork bot to automatically mark series as superseded. It doesn't do a perfect job, but it considerably reduces the maintenance work. Could we do the same ? > Also, more people should help keep patchwork clean. I think I am usually the > person who is doing this, but this is a collective responsibility. > > Part of the job description for someone with commit rights is actually to > keep patchwork clean. I think that requiring committers to update the patchwork state of patches they merge is fine, the additional work is offset by the ability to commit patches directly. I don't think it would have helped in the case of patches submitted by non-committers. Even if I had merged this change myself, I missed Jacopo's v3 on the list, and I would have probably equally missed it in patchwork without v2 being marked as superseded. > > 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; -- 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] 11+ messages in thread
end of thread, other threads:[~2025-02-20 17:01 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-10-29 8:21 [PATCH v3] media: rkisp1: Remove min_queued_buffers Jacopo Mondi 2024-10-29 11:29 ` Laurent Pinchart 2024-10-29 11:50 ` Hans Verkuil 2025-02-20 14:22 ` Jacopo Mondi 2025-02-20 14:50 ` Laurent Pinchart 2025-02-20 15:33 ` Hans Verkuil 2025-02-20 16:38 ` Laurent Pinchart 2025-02-20 15:39 ` Hans Verkuil 2025-02-20 16:27 ` Jacopo Mondi 2025-02-20 17:01 ` Hans Verkuil 2025-02-20 16:34 ` Laurent Pinchart
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox