| Message ID | 20260618123844.656396-24-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás On Thu, Jun 18, 2026 at 02:38:40PM +0200, Barnabás Pőcze wrote: > Make `V4L2Camera::allocBuffers()` simply create as many requests as the > number of buffers allocated instead of taking it as an argument. The two > should be the same, so no functional changes intended. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > src/v4l2/v4l2_camera.cpp | 8 +++++--- > src/v4l2/v4l2_camera.h | 2 +- > src/v4l2/v4l2_camera_proxy.cpp | 11 +++++------ > 3 files changed, 11 insertions(+), 10 deletions(-) > > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp > index 648578c22b..75428b8adc 100644 > --- a/src/v4l2/v4l2_camera.cpp > +++ b/src/v4l2/v4l2_camera.cpp > @@ -154,7 +154,7 @@ int V4L2Camera::validateConfiguration(const PixelFormat &pixelFormat, > return 0; > } > > -int V4L2Camera::allocBuffers(unsigned int count) > +int V4L2Camera::allocBuffers() > { > Stream *stream = config_->at(0).stream(); > > @@ -162,7 +162,9 @@ int V4L2Camera::allocBuffers(unsigned int count) > if (ret < 0) > return ret; > > - for (unsigned int i = 0; i < count; i++) { > + const auto &buffers = bufferAllocator_->buffers(stream); > + > + for (size_t i = 0; i < buffers.size(); i++) { > std::unique_ptr<Request> request = camera_->createRequest(i); > if (!request) { > requestPool_.clear(); > @@ -171,7 +173,7 @@ int V4L2Camera::allocBuffers(unsigned int count) > requestPool_.push_back(std::move(request)); > } > > - return ret; > + return buffers.size(); I guess we could then simply return either 0 or < 0 now Anyway Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > } > > void V4L2Camera::freeBuffers() > diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h > index 1528f9aad8..8a58169d89 100644 > --- a/src/v4l2/v4l2_camera.h > +++ b/src/v4l2/v4l2_camera.h > @@ -54,7 +54,7 @@ public: > libcamera::ControlList &controls() { return controls_; } > const libcamera::ControlInfoMap &controlInfo() { return camera_->controls(); } > > - int allocBuffers(unsigned int count); > + int allocBuffers(); > void freeBuffers(); > int getBufferFd(unsigned int index); > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp > index 03cd4810cc..5281f10552 100644 > --- a/src/v4l2/v4l2_camera_proxy.cpp > +++ b/src/v4l2/v4l2_camera_proxy.cpp > @@ -543,17 +543,16 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuf > if (ret < 0) > return -EINVAL; > > - setFmtFromConfig(streamConfig_); > - > - arg->count = streamConfig_.bufferCount; > - bufferCount_ = arg->count; > - > - ret = vcam_->allocBuffers(arg->count); > + ret = vcam_->allocBuffers(); > if (ret < 0) { > arg->count = 0; > return ret; > } > > + bufferCount_ = arg->count = ret; > + > + setFmtFromConfig(streamConfig_); > + > buffers_.resize(arg->count); > for (unsigned int i = 0; i < arg->count; i++) { > struct v4l2_buffer buf = {}; > -- > 2.54.0 >
On Mon, Jun 22, 2026 at 03:52:44PM +0200, Jacopo Mondi wrote: > On Thu, Jun 18, 2026 at 02:38:40PM +0200, Barnabás Pőcze wrote: > > Make `V4L2Camera::allocBuffers()` simply create as many requests as the > > number of buffers allocated instead of taking it as an argument. The two > > should be the same, so no functional changes intended. > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > --- > > src/v4l2/v4l2_camera.cpp | 8 +++++--- > > src/v4l2/v4l2_camera.h | 2 +- > > src/v4l2/v4l2_camera_proxy.cpp | 11 +++++------ > > 3 files changed, 11 insertions(+), 10 deletions(-) > > > > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp > > index 648578c22b..75428b8adc 100644 > > --- a/src/v4l2/v4l2_camera.cpp > > +++ b/src/v4l2/v4l2_camera.cpp > > @@ -154,7 +154,7 @@ int V4L2Camera::validateConfiguration(const PixelFormat &pixelFormat, > > return 0; > > } > > > > -int V4L2Camera::allocBuffers(unsigned int count) > > +int V4L2Camera::allocBuffers() > > { > > Stream *stream = config_->at(0).stream(); > > > > @@ -162,7 +162,9 @@ int V4L2Camera::allocBuffers(unsigned int count) > > if (ret < 0) > > return ret; > > > > - for (unsigned int i = 0; i < count; i++) { > > + const auto &buffers = bufferAllocator_->buffers(stream); > > + > > + for (size_t i = 0; i < buffers.size(); i++) { > > std::unique_ptr<Request> request = camera_->createRequest(i); > > if (!request) { > > requestPool_.clear(); > > @@ -171,7 +173,7 @@ int V4L2Camera::allocBuffers(unsigned int count) > > requestPool_.push_back(std::move(request)); > > } > > > > - return ret; > > + return buffers.size(); > > I guess we could then simply return either 0 or < 0 now > Anyway > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > } > > > > void V4L2Camera::freeBuffers() > > diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h > > index 1528f9aad8..8a58169d89 100644 > > --- a/src/v4l2/v4l2_camera.h > > +++ b/src/v4l2/v4l2_camera.h > > @@ -54,7 +54,7 @@ public: > > libcamera::ControlList &controls() { return controls_; } > > const libcamera::ControlInfoMap &controlInfo() { return camera_->controls(); } > > > > - int allocBuffers(unsigned int count); > > + int allocBuffers(); > > void freeBuffers(); > > int getBufferFd(unsigned int index); > > > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp > > index 03cd4810cc..5281f10552 100644 > > --- a/src/v4l2/v4l2_camera_proxy.cpp > > +++ b/src/v4l2/v4l2_camera_proxy.cpp > > @@ -543,17 +543,16 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuf > > if (ret < 0) > > return -EINVAL; > > > > - setFmtFromConfig(streamConfig_); > > - > > - arg->count = streamConfig_.bufferCount; > > - bufferCount_ = arg->count; > > - > > - ret = vcam_->allocBuffers(arg->count); > > + ret = vcam_->allocBuffers(); > > if (ret < 0) { > > arg->count = 0; > > return ret; > > } > > > > + bufferCount_ = arg->count = ret; One assignment per line please. > > + > > + setFmtFromConfig(streamConfig_); Moving those after the call to allocBuffers() is good. I'm less convinced about the main change though. The FrameBufferAllocator implicit reliance on bufferCount is something I don't like, I think we'll have to redesign that code. That means that V4L2Camera::allocBuffers() would need to pass the buffer count to FrameBufferAllocator::allocate(). Is this patch required for your ongoing work that splits buffers from requests ? If not, I'd rather drop this for the time being. > > + > > buffers_.resize(arg->count); > > for (unsigned int i = 0; i < arg->count; i++) { > > struct v4l2_buffer buf = {};
2026. 06. 24. 0:49 keltezéssel, Laurent Pinchart írta: > On Mon, Jun 22, 2026 at 03:52:44PM +0200, Jacopo Mondi wrote: >> On Thu, Jun 18, 2026 at 02:38:40PM +0200, Barnabás Pőcze wrote: >>> Make `V4L2Camera::allocBuffers()` simply create as many requests as the >>> number of buffers allocated instead of taking it as an argument. The two >>> should be the same, so no functional changes intended. >>> >>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>> --- >>> src/v4l2/v4l2_camera.cpp | 8 +++++--- >>> src/v4l2/v4l2_camera.h | 2 +- >>> src/v4l2/v4l2_camera_proxy.cpp | 11 +++++------ >>> 3 files changed, 11 insertions(+), 10 deletions(-) >>> >>> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp >>> index 648578c22b..75428b8adc 100644 >>> --- a/src/v4l2/v4l2_camera.cpp >>> +++ b/src/v4l2/v4l2_camera.cpp >>> @@ -154,7 +154,7 @@ int V4L2Camera::validateConfiguration(const PixelFormat &pixelFormat, >>> return 0; >>> } >>> >>> -int V4L2Camera::allocBuffers(unsigned int count) >>> +int V4L2Camera::allocBuffers() >>> { >>> Stream *stream = config_->at(0).stream(); >>> >>> @@ -162,7 +162,9 @@ int V4L2Camera::allocBuffers(unsigned int count) >>> if (ret < 0) >>> return ret; >>> >>> - for (unsigned int i = 0; i < count; i++) { >>> + const auto &buffers = bufferAllocator_->buffers(stream); >>> + >>> + for (size_t i = 0; i < buffers.size(); i++) { >>> std::unique_ptr<Request> request = camera_->createRequest(i); >>> if (!request) { >>> requestPool_.clear(); >>> @@ -171,7 +173,7 @@ int V4L2Camera::allocBuffers(unsigned int count) >>> requestPool_.push_back(std::move(request)); >>> } >>> >>> - return ret; >>> + return buffers.size(); >> >> I guess we could then simply return either 0 or < 0 now >> Anyway >> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >> >>> } >>> >>> void V4L2Camera::freeBuffers() >>> diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h >>> index 1528f9aad8..8a58169d89 100644 >>> --- a/src/v4l2/v4l2_camera.h >>> +++ b/src/v4l2/v4l2_camera.h >>> @@ -54,7 +54,7 @@ public: >>> libcamera::ControlList &controls() { return controls_; } >>> const libcamera::ControlInfoMap &controlInfo() { return camera_->controls(); } >>> >>> - int allocBuffers(unsigned int count); >>> + int allocBuffers(); >>> void freeBuffers(); >>> int getBufferFd(unsigned int index); >>> >>> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp >>> index 03cd4810cc..5281f10552 100644 >>> --- a/src/v4l2/v4l2_camera_proxy.cpp >>> +++ b/src/v4l2/v4l2_camera_proxy.cpp >>> @@ -543,17 +543,16 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuf >>> if (ret < 0) >>> return -EINVAL; >>> >>> - setFmtFromConfig(streamConfig_); >>> - >>> - arg->count = streamConfig_.bufferCount; >>> - bufferCount_ = arg->count; >>> - >>> - ret = vcam_->allocBuffers(arg->count); >>> + ret = vcam_->allocBuffers(); >>> if (ret < 0) { >>> arg->count = 0; >>> return ret; >>> } >>> >>> + bufferCount_ = arg->count = ret; > > One assignment per line please. > >>> + >>> + setFmtFromConfig(streamConfig_); > > Moving those after the call to allocBuffers() is good. I'm less > convinced about the main change though. The FrameBufferAllocator > implicit reliance on bufferCount is something I don't like, I think > we'll have to redesign that code. That means that > V4L2Camera::allocBuffers() would need to pass the buffer count to > FrameBufferAllocator::allocate(). > > Is this patch required for your ongoing work that splits buffers from > requests ? If not, I'd rather drop this for the time being. I believe it is not a requirement per se, but I think it makes the code easier to understand; for example, by completely circumventing any interaction with `StreamConfiguration::bufferCount`. The goal is to allocate as many requests as many buffers have been allocated. I feel like the new version makes that more clear. > >>> + >>> buffers_.resize(arg->count); >>> for (unsigned int i = 0; i < arg->count; i++) { >>> struct v4l2_buffer buf = {}; > > -- > Regards, > > Laurent Pinchart
On Wed, Jun 24, 2026 at 10:18:02AM +0200, Barnabás Pőcze wrote: > 2026. 06. 24. 0:49 keltezéssel, Laurent Pinchart írta: > > On Mon, Jun 22, 2026 at 03:52:44PM +0200, Jacopo Mondi wrote: > >> On Thu, Jun 18, 2026 at 02:38:40PM +0200, Barnabás Pőcze wrote: > >>> Make `V4L2Camera::allocBuffers()` simply create as many requests as the > >>> number of buffers allocated instead of taking it as an argument. The two > >>> should be the same, so no functional changes intended. > >>> > >>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >>> --- > >>> src/v4l2/v4l2_camera.cpp | 8 +++++--- > >>> src/v4l2/v4l2_camera.h | 2 +- > >>> src/v4l2/v4l2_camera_proxy.cpp | 11 +++++------ > >>> 3 files changed, 11 insertions(+), 10 deletions(-) > >>> > >>> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp > >>> index 648578c22b..75428b8adc 100644 > >>> --- a/src/v4l2/v4l2_camera.cpp > >>> +++ b/src/v4l2/v4l2_camera.cpp > >>> @@ -154,7 +154,7 @@ int V4L2Camera::validateConfiguration(const PixelFormat &pixelFormat, > >>> return 0; > >>> } > >>> > >>> -int V4L2Camera::allocBuffers(unsigned int count) > >>> +int V4L2Camera::allocBuffers() > >>> { > >>> Stream *stream = config_->at(0).stream(); > >>> > >>> @@ -162,7 +162,9 @@ int V4L2Camera::allocBuffers(unsigned int count) > >>> if (ret < 0) > >>> return ret; > >>> > >>> - for (unsigned int i = 0; i < count; i++) { > >>> + const auto &buffers = bufferAllocator_->buffers(stream); > >>> + > >>> + for (size_t i = 0; i < buffers.size(); i++) { > >>> std::unique_ptr<Request> request = camera_->createRequest(i); > >>> if (!request) { > >>> requestPool_.clear(); > >>> @@ -171,7 +173,7 @@ int V4L2Camera::allocBuffers(unsigned int count) > >>> requestPool_.push_back(std::move(request)); > >>> } > >>> > >>> - return ret; > >>> + return buffers.size(); > >> > >> I guess we could then simply return either 0 or < 0 now > >> Anyway > >> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > >> > >>> } > >>> > >>> void V4L2Camera::freeBuffers() > >>> diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h > >>> index 1528f9aad8..8a58169d89 100644 > >>> --- a/src/v4l2/v4l2_camera.h > >>> +++ b/src/v4l2/v4l2_camera.h > >>> @@ -54,7 +54,7 @@ public: > >>> libcamera::ControlList &controls() { return controls_; } > >>> const libcamera::ControlInfoMap &controlInfo() { return camera_->controls(); } > >>> > >>> - int allocBuffers(unsigned int count); > >>> + int allocBuffers(); > >>> void freeBuffers(); > >>> int getBufferFd(unsigned int index); > >>> > >>> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp > >>> index 03cd4810cc..5281f10552 100644 > >>> --- a/src/v4l2/v4l2_camera_proxy.cpp > >>> +++ b/src/v4l2/v4l2_camera_proxy.cpp > >>> @@ -543,17 +543,16 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuf > >>> if (ret < 0) > >>> return -EINVAL; > >>> > >>> - setFmtFromConfig(streamConfig_); > >>> - > >>> - arg->count = streamConfig_.bufferCount; > >>> - bufferCount_ = arg->count; > >>> - > >>> - ret = vcam_->allocBuffers(arg->count); > >>> + ret = vcam_->allocBuffers(); > >>> if (ret < 0) { > >>> arg->count = 0; > >>> return ret; > >>> } > >>> > >>> + bufferCount_ = arg->count = ret; > > > > One assignment per line please. > > > >>> + > >>> + setFmtFromConfig(streamConfig_); > > > > Moving those after the call to allocBuffers() is good. I'm less > > convinced about the main change though. The FrameBufferAllocator > > implicit reliance on bufferCount is something I don't like, I think > > we'll have to redesign that code. That means that > > V4L2Camera::allocBuffers() would need to pass the buffer count to > > FrameBufferAllocator::allocate(). > > > > Is this patch required for your ongoing work that splits buffers from > > requests ? If not, I'd rather drop this for the time being. > > I believe it is not a requirement per se, but I think it makes the code > easier to understand; for example, by completely circumventing any interaction > with `StreamConfiguration::bufferCount`. The goal is to allocate as many requests > as many buffers have been allocated. I feel like the new version makes that > more clear. We could do that in the short term, but if we drop bufferCount from StreamConfiguration in the longer run, this patch does in the opposite direction. > >>> + > >>> buffers_.resize(arg->count); > >>> for (unsigned int i = 0; i < arg->count; i++) { > >>> struct v4l2_buffer buf = {};
diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp index 648578c22b..75428b8adc 100644 --- a/src/v4l2/v4l2_camera.cpp +++ b/src/v4l2/v4l2_camera.cpp @@ -154,7 +154,7 @@ int V4L2Camera::validateConfiguration(const PixelFormat &pixelFormat, return 0; } -int V4L2Camera::allocBuffers(unsigned int count) +int V4L2Camera::allocBuffers() { Stream *stream = config_->at(0).stream(); @@ -162,7 +162,9 @@ int V4L2Camera::allocBuffers(unsigned int count) if (ret < 0) return ret; - for (unsigned int i = 0; i < count; i++) { + const auto &buffers = bufferAllocator_->buffers(stream); + + for (size_t i = 0; i < buffers.size(); i++) { std::unique_ptr<Request> request = camera_->createRequest(i); if (!request) { requestPool_.clear(); @@ -171,7 +173,7 @@ int V4L2Camera::allocBuffers(unsigned int count) requestPool_.push_back(std::move(request)); } - return ret; + return buffers.size(); } void V4L2Camera::freeBuffers() diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h index 1528f9aad8..8a58169d89 100644 --- a/src/v4l2/v4l2_camera.h +++ b/src/v4l2/v4l2_camera.h @@ -54,7 +54,7 @@ public: libcamera::ControlList &controls() { return controls_; } const libcamera::ControlInfoMap &controlInfo() { return camera_->controls(); } - int allocBuffers(unsigned int count); + int allocBuffers(); void freeBuffers(); int getBufferFd(unsigned int index); diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp index 03cd4810cc..5281f10552 100644 --- a/src/v4l2/v4l2_camera_proxy.cpp +++ b/src/v4l2/v4l2_camera_proxy.cpp @@ -543,17 +543,16 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuf if (ret < 0) return -EINVAL; - setFmtFromConfig(streamConfig_); - - arg->count = streamConfig_.bufferCount; - bufferCount_ = arg->count; - - ret = vcam_->allocBuffers(arg->count); + ret = vcam_->allocBuffers(); if (ret < 0) { arg->count = 0; return ret; } + bufferCount_ = arg->count = ret; + + setFmtFromConfig(streamConfig_); + buffers_.resize(arg->count); for (unsigned int i = 0; i < arg->count; i++) { struct v4l2_buffer buf = {};
Make `V4L2Camera::allocBuffers()` simply create as many requests as the number of buffers allocated instead of taking it as an argument. The two should be the same, so no functional changes intended. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- src/v4l2/v4l2_camera.cpp | 8 +++++--- src/v4l2/v4l2_camera.h | 2 +- src/v4l2/v4l2_camera_proxy.cpp | 11 +++++------ 3 files changed, 11 insertions(+), 10 deletions(-)