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@xxxxxxxxxxxxxxxx>
>>>
>>> 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
>>>>
>>>>
>>
