| Message ID | 20241029082117.55385-1-jacopo.mondi@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 6c10d1adae82e1c8da16e7ebd2320e69f20b9d6f |
| Headers | show |
| Series | [v3] media: rkisp1: Remove min_queued_buffers | expand |
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;
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; >
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;
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 >> >>
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 >> >>
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 > >> > >> >
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;
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;
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;
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(-) -- 2.47.0