| Message ID | 20240825232449.25905-1-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: videobuf2: Drop minimum allocation requirement of 2 buffers | expand |
On 26/08/2024 01:24, Laurent Pinchart wrote: > When introducing the ability for drivers to indicate the minimum number > of buffers they require an application to allocate, commit 6662edcd32cc > ("media: videobuf2: Add min_reqbufs_allocation field to vb2_queue > structure") also introduced a global minimum of 2 buffers. It turns out > this breaks the Renesas R-Car VSP test suite, where a test that > allocates a single buffer fails when two buffers are used. > > One may consider debatable whether test suite failures without failures > in production use cases should be considered as a regression, but > operation with a single buffer is a valid use case. While full frame > rate can't be maintained, memory-to-memory devices can still be used > with a decent efficiency, and requiring applications to allocate > multiple buffers for single-shot use cases with capture devices would > just waste memory. > > For those reasons, fix the regression by dropping the global minimum of > buffers. Individual drivers can still set their own minimum. > > Fixes: 6662edcd32cc ("media: videobuf2: Add min_reqbufs_allocation field to vb2_queue structure") > Cc: stable@vger.kernel.org > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Regards, Hans > --- > drivers/media/common/videobuf2/videobuf2-core.c | 7 ------- > 1 file changed, 7 deletions(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index 500a4e0c84ab..29a8d876e6c2 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -2632,13 +2632,6 @@ int vb2_core_queue_init(struct vb2_queue *q) > if (WARN_ON(q->supports_requests && q->min_queued_buffers)) > return -EINVAL; > > - /* > - * The minimum requirement is 2: one buffer is used > - * by the hardware while the other is being processed by userspace. > - */ > - if (q->min_reqbufs_allocation < 2) > - q->min_reqbufs_allocation = 2; > - > /* > * If the driver needs 'min_queued_buffers' in the queue before > * calling start_streaming() then the minimum requirement is > > base-commit: a043ea54bbb975ca9239c69fd17f430488d33522
On Mon, Aug 26, 2024 at 8:24 AM Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> wrote: > > When introducing the ability for drivers to indicate the minimum number > of buffers they require an application to allocate, commit 6662edcd32cc > ("media: videobuf2: Add min_reqbufs_allocation field to vb2_queue > structure") also introduced a global minimum of 2 buffers. It turns out > this breaks the Renesas R-Car VSP test suite, where a test that > allocates a single buffer fails when two buffers are used. > > One may consider debatable whether test suite failures without failures > in production use cases should be considered as a regression, but > operation with a single buffer is a valid use case. While full frame > rate can't be maintained, memory-to-memory devices can still be used > with a decent efficiency, and requiring applications to allocate > multiple buffers for single-shot use cases with capture devices would > just waste memory. > > For those reasons, fix the regression by dropping the global minimum of > buffers. Individual drivers can still set their own minimum. > > Fixes: 6662edcd32cc ("media: videobuf2: Add min_reqbufs_allocation field to vb2_queue structure") > Cc: stable@vger.kernel.org > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > drivers/media/common/videobuf2/videobuf2-core.c | 7 ------- > 1 file changed, 7 deletions(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index 500a4e0c84ab..29a8d876e6c2 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -2632,13 +2632,6 @@ int vb2_core_queue_init(struct vb2_queue *q) > if (WARN_ON(q->supports_requests && q->min_queued_buffers)) > return -EINVAL; > > - /* > - * The minimum requirement is 2: one buffer is used > - * by the hardware while the other is being processed by userspace. > - */ > - if (q->min_reqbufs_allocation < 2) > - q->min_reqbufs_allocation = 2; > - > /* > * If the driver needs 'min_queued_buffers' in the queue before > * calling start_streaming() then the minimum requirement is > > base-commit: a043ea54bbb975ca9239c69fd17f430488d33522 Thanks for the patch! Acked-by: Tomasz Figa <tfiga@chromium.org> Best regards, Tomasz
Hi Hans, Please let me know if you expect a pull request, otherwise I'll consider you will take this in your tree. On Mon, Aug 26, 2024 at 08:31:13AM +0200, Hans Verkuil wrote: > On 26/08/2024 01:24, Laurent Pinchart wrote: > > When introducing the ability for drivers to indicate the minimum number > > of buffers they require an application to allocate, commit 6662edcd32cc > > ("media: videobuf2: Add min_reqbufs_allocation field to vb2_queue > > structure") also introduced a global minimum of 2 buffers. It turns out > > this breaks the Renesas R-Car VSP test suite, where a test that > > allocates a single buffer fails when two buffers are used. > > > > One may consider debatable whether test suite failures without failures > > in production use cases should be considered as a regression, but > > operation with a single buffer is a valid use case. While full frame > > rate can't be maintained, memory-to-memory devices can still be used > > with a decent efficiency, and requiring applications to allocate > > multiple buffers for single-shot use cases with capture devices would > > just waste memory. > > > > For those reasons, fix the regression by dropping the global minimum of > > buffers. Individual drivers can still set their own minimum. > > > > Fixes: 6662edcd32cc ("media: videobuf2: Add min_reqbufs_allocation field to vb2_queue structure") > > Cc: stable@vger.kernel.org > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > > --- > > drivers/media/common/videobuf2/videobuf2-core.c | 7 ------- > > 1 file changed, 7 deletions(-) > > > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > > index 500a4e0c84ab..29a8d876e6c2 100644 > > --- a/drivers/media/common/videobuf2/videobuf2-core.c > > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > > @@ -2632,13 +2632,6 @@ int vb2_core_queue_init(struct vb2_queue *q) > > if (WARN_ON(q->supports_requests && q->min_queued_buffers)) > > return -EINVAL; > > > > - /* > > - * The minimum requirement is 2: one buffer is used > > - * by the hardware while the other is being processed by userspace. > > - */ > > - if (q->min_reqbufs_allocation < 2) > > - q->min_reqbufs_allocation = 2; > > - > > /* > > * If the driver needs 'min_queued_buffers' in the queue before > > * calling start_streaming() then the minimum requirement is > > > > base-commit: a043ea54bbb975ca9239c69fd17f430488d33522
On 8/27/24 11:15, Laurent Pinchart wrote: > Hi Hans, > > Please let me know if you expect a pull request, otherwise I'll consider > you will take this in your tree. Can you add it to your "Miscellaneous V4L2 patches" PR and post a v2 of that today? Regards, Hans > > On Mon, Aug 26, 2024 at 08:31:13AM +0200, Hans Verkuil wrote: >> On 26/08/2024 01:24, Laurent Pinchart wrote: >>> When introducing the ability for drivers to indicate the minimum number >>> of buffers they require an application to allocate, commit 6662edcd32cc >>> ("media: videobuf2: Add min_reqbufs_allocation field to vb2_queue >>> structure") also introduced a global minimum of 2 buffers. It turns out >>> this breaks the Renesas R-Car VSP test suite, where a test that >>> allocates a single buffer fails when two buffers are used. >>> >>> One may consider debatable whether test suite failures without failures >>> in production use cases should be considered as a regression, but >>> operation with a single buffer is a valid use case. While full frame >>> rate can't be maintained, memory-to-memory devices can still be used >>> with a decent efficiency, and requiring applications to allocate >>> multiple buffers for single-shot use cases with capture devices would >>> just waste memory. >>> >>> For those reasons, fix the regression by dropping the global minimum of >>> buffers. Individual drivers can still set their own minimum. >>> >>> Fixes: 6662edcd32cc ("media: videobuf2: Add min_reqbufs_allocation field to vb2_queue structure") >>> Cc: stable@vger.kernel.org >>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> >> >> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> >>> --- >>> drivers/media/common/videobuf2/videobuf2-core.c | 7 ------- >>> 1 file changed, 7 deletions(-) >>> >>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c >>> index 500a4e0c84ab..29a8d876e6c2 100644 >>> --- a/drivers/media/common/videobuf2/videobuf2-core.c >>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c >>> @@ -2632,13 +2632,6 @@ int vb2_core_queue_init(struct vb2_queue *q) >>> if (WARN_ON(q->supports_requests && q->min_queued_buffers)) >>> return -EINVAL; >>> >>> - /* >>> - * The minimum requirement is 2: one buffer is used >>> - * by the hardware while the other is being processed by userspace. >>> - */ >>> - if (q->min_reqbufs_allocation < 2) >>> - q->min_reqbufs_allocation = 2; >>> - >>> /* >>> * If the driver needs 'min_queued_buffers' in the queue before >>> * calling start_streaming() then the minimum requirement is >>> >>> base-commit: a043ea54bbb975ca9239c69fd17f430488d33522 >
On Tue, Aug 27, 2024 at 11:19:22AM +0200, Hans Verkuil wrote: > On 8/27/24 11:15, Laurent Pinchart wrote: > > Hi Hans, > > > > Please let me know if you expect a pull request, otherwise I'll consider > > you will take this in your tree. > > Can you add it to your "Miscellaneous V4L2 patches" PR and post a v2 > of that today? I'd rather not, as I don't want to rebase and retest that one. I'll send a separate pull request for this patch. > > On Mon, Aug 26, 2024 at 08:31:13AM +0200, Hans Verkuil wrote: > >> On 26/08/2024 01:24, Laurent Pinchart wrote: > >>> When introducing the ability for drivers to indicate the minimum number > >>> of buffers they require an application to allocate, commit 6662edcd32cc > >>> ("media: videobuf2: Add min_reqbufs_allocation field to vb2_queue > >>> structure") also introduced a global minimum of 2 buffers. It turns out > >>> this breaks the Renesas R-Car VSP test suite, where a test that > >>> allocates a single buffer fails when two buffers are used. > >>> > >>> One may consider debatable whether test suite failures without failures > >>> in production use cases should be considered as a regression, but > >>> operation with a single buffer is a valid use case. While full frame > >>> rate can't be maintained, memory-to-memory devices can still be used > >>> with a decent efficiency, and requiring applications to allocate > >>> multiple buffers for single-shot use cases with capture devices would > >>> just waste memory. > >>> > >>> For those reasons, fix the regression by dropping the global minimum of > >>> buffers. Individual drivers can still set their own minimum. > >>> > >>> Fixes: 6662edcd32cc ("media: videobuf2: Add min_reqbufs_allocation field to vb2_queue structure") > >>> Cc: stable@vger.kernel.org > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > >> > >> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > >> > >>> --- > >>> drivers/media/common/videobuf2/videobuf2-core.c | 7 ------- > >>> 1 file changed, 7 deletions(-) > >>> > >>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > >>> index 500a4e0c84ab..29a8d876e6c2 100644 > >>> --- a/drivers/media/common/videobuf2/videobuf2-core.c > >>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c > >>> @@ -2632,13 +2632,6 @@ int vb2_core_queue_init(struct vb2_queue *q) > >>> if (WARN_ON(q->supports_requests && q->min_queued_buffers)) > >>> return -EINVAL; > >>> > >>> - /* > >>> - * The minimum requirement is 2: one buffer is used > >>> - * by the hardware while the other is being processed by userspace. > >>> - */ > >>> - if (q->min_reqbufs_allocation < 2) > >>> - q->min_reqbufs_allocation = 2; > >>> - > >>> /* > >>> * If the driver needs 'min_queued_buffers' in the queue before > >>> * calling start_streaming() then the minimum requirement is > >>> > >>> base-commit: a043ea54bbb975ca9239c69fd17f430488d33522
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index 500a4e0c84ab..29a8d876e6c2 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -2632,13 +2632,6 @@ int vb2_core_queue_init(struct vb2_queue *q) if (WARN_ON(q->supports_requests && q->min_queued_buffers)) return -EINVAL; - /* - * The minimum requirement is 2: one buffer is used - * by the hardware while the other is being processed by userspace. - */ - if (q->min_reqbufs_allocation < 2) - q->min_reqbufs_allocation = 2; - /* * If the driver needs 'min_queued_buffers' in the queue before * calling start_streaming() then the minimum requirement is
When introducing the ability for drivers to indicate the minimum number of buffers they require an application to allocate, commit 6662edcd32cc ("media: videobuf2: Add min_reqbufs_allocation field to vb2_queue structure") also introduced a global minimum of 2 buffers. It turns out this breaks the Renesas R-Car VSP test suite, where a test that allocates a single buffer fails when two buffers are used. One may consider debatable whether test suite failures without failures in production use cases should be considered as a regression, but operation with a single buffer is a valid use case. While full frame rate can't be maintained, memory-to-memory devices can still be used with a decent efficiency, and requiring applications to allocate multiple buffers for single-shot use cases with capture devices would just waste memory. For those reasons, fix the regression by dropping the global minimum of buffers. Individual drivers can still set their own minimum. Fixes: 6662edcd32cc ("media: videobuf2: Add min_reqbufs_allocation field to vb2_queue structure") Cc: stable@vger.kernel.org Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> --- drivers/media/common/videobuf2/videobuf2-core.c | 7 ------- 1 file changed, 7 deletions(-) base-commit: a043ea54bbb975ca9239c69fd17f430488d33522