| Message ID | 20190207212119.30299-5-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, On Thu, Feb 07, 2019 at 09:21:18PM +0000, Kieran Bingham wrote: > exportBuffers() can only operate on an existing BufferPool allocation. The "an interanlly allocated BufferPool" ? > pool identifies its size through its .count() method. > > Passing a count in to the exportBuffers() call is redundant and can be > incorrect if the value is not the same as the BufferPool size. > > Simplify the function and remove the unnecessary argument, correcting all uses > throughout the code base. > > While we're here, remove the createBuffers() helper from the V4L2DeviceTest > which only served to obfuscate which pool the buffers were being allocated for. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/include/v4l2_device.h | 2 +- > src/libcamera/pipeline/ipu3/ipu3.cpp | 3 +-- > src/libcamera/pipeline/uvcvideo.cpp | 2 +- > src/libcamera/pipeline/vimc.cpp | 2 +- > src/libcamera/v4l2_device.cpp | 15 ++++++--------- > test/v4l2_device/buffer_sharing.cpp | 2 +- > test/v4l2_device/capture_async.cpp | 4 ++-- > test/v4l2_device/request_buffers.cpp | 4 ++-- > test/v4l2_device/stream_on_off.cpp | 4 ++-- > test/v4l2_device/v4l2_device_test.h | 2 -- > 10 files changed, 17 insertions(+), 23 deletions(-) > > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h > index 3acb5e466d64..98fa2110efb5 100644 > --- a/src/libcamera/include/v4l2_device.h > +++ b/src/libcamera/include/v4l2_device.h > @@ -97,7 +97,7 @@ public: > int getFormat(V4L2DeviceFormat *format); > int setFormat(V4L2DeviceFormat *format); > > - int exportBuffers(unsigned int count, BufferPool *pool); > + int exportBuffers(BufferPool *pool); > int importBuffers(BufferPool *pool); > int releaseBuffers(); > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 34b03995ae31..677e127dd738 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -191,8 +191,7 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream) > if (!cfg.bufferCount) > return -EINVAL; > > - int ret = data->cio2_->exportBuffers(cfg.bufferCount, > - &stream->bufferPool()); > + int ret = data->cio2_->exportBuffers(&stream->bufferPool()); > if (ret) { > LOG(IPU3, Error) << "Failed to request memory"; > return ret; > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > index ed8228bb2fc6..1a0dcb582670 100644 > --- a/src/libcamera/pipeline/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo.cpp > @@ -102,7 +102,7 @@ int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream) > > LOG(UVC, Debug) << "Requesting " << cfg.bufferCount << " buffers"; > > - return video_->exportBuffers(cfg.bufferCount, &stream->bufferPool()); > + return video_->exportBuffers(&stream->bufferPool()); > } > > int PipelineHandlerUVC::freeBuffers(Camera *camera, Stream *stream) > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > index 0e9ad7b59ee5..06627b1cb847 100644 > --- a/src/libcamera/pipeline/vimc.cpp > +++ b/src/libcamera/pipeline/vimc.cpp > @@ -101,7 +101,7 @@ int PipeHandlerVimc::allocateBuffers(Camera *camera, Stream *stream) > > LOG(VIMC, Debug) << "Requesting " << cfg.bufferCount << " buffers"; > > - return video_->exportBuffers(cfg.bufferCount, &stream->bufferPool()); > + return video_->exportBuffers(&stream->bufferPool()); > } > > int PipeHandlerVimc::freeBuffers(Camera *camera, Stream *stream) > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index c654e6dd7b8d..c2e4d0ea8db2 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -526,13 +526,12 @@ int V4L2Device::requestBuffers(unsigned int count) > } > > /** > - * \brief Request \a count buffers to be allocated from the device and stored in > - * the buffer pool provided. > - * \param[in] count Number of buffers to allocate > + * \brief Request buffers to be allocated from the device and stored in the > + * buffer pool provided. > * \param[out] pool BufferPool to populate with buffers > * \return 0 on success or a negative error code otherwise > */ > -int V4L2Device::exportBuffers(unsigned int count, BufferPool *pool) > +int V4L2Device::exportBuffers(BufferPool *pool) > { > unsigned int allocatedBuffers; > unsigned int i; > @@ -540,21 +539,19 @@ int V4L2Device::exportBuffers(unsigned int count, BufferPool *pool) > > memoryType_ = V4L2_MEMORY_MMAP; > > - ret = requestBuffers(count); > + ret = requestBuffers(pool->count()); > if (ret < 0) > return ret; > > allocatedBuffers = ret; > - if (allocatedBuffers < count) { > + if (allocatedBuffers < pool->count()) { I have a feeling we discussed this, but could you drop allocatedBuffers now? The rest looks fine! Thanks j > LOG(V4L2, Error) << "Not enough buffers provided by V4L2Device"; > requestBuffers(0); > return -ENOMEM; > } > > - count = ret; > - > /* Map the buffers. */ > - for (i = 0; i < count; ++i) { > + for (i = 0; i < pool->count(); ++i) { > struct v4l2_plane planes[VIDEO_MAX_PLANES] = {}; > struct v4l2_buffer buf = {}; > struct Buffer &buffer = pool->buffers()[i]; > diff --git a/test/v4l2_device/buffer_sharing.cpp b/test/v4l2_device/buffer_sharing.cpp > index 0e96f7b894bd..ca086f2728f1 100644 > --- a/test/v4l2_device/buffer_sharing.cpp > +++ b/test/v4l2_device/buffer_sharing.cpp > @@ -80,7 +80,7 @@ protected: > > pool_.createBuffers(bufferCount); > > - ret = dev_->exportBuffers(bufferCount, &pool_); > + ret = dev_->exportBuffers(&pool_); > if (ret) > return TestFail; > > diff --git a/test/v4l2_device/capture_async.cpp b/test/v4l2_device/capture_async.cpp > index 7a0735f65535..d3e20ed86b98 100644 > --- a/test/v4l2_device/capture_async.cpp > +++ b/test/v4l2_device/capture_async.cpp > @@ -38,9 +38,9 @@ protected: > Timer timeout; > int ret; > > - createBuffers(bufferCount); > + pool_.createBuffers(bufferCount); > > - ret = dev_->exportBuffers(bufferCount, &pool_); > + ret = dev_->exportBuffers(&pool_); > if (ret) > return TestFail; > > diff --git a/test/v4l2_device/request_buffers.cpp b/test/v4l2_device/request_buffers.cpp > index bc6ff2c18a57..26d7d9528982 100644 > --- a/test/v4l2_device/request_buffers.cpp > +++ b/test/v4l2_device/request_buffers.cpp > @@ -19,9 +19,9 @@ protected: > */ > const unsigned int bufferCount = 8; > > - createBuffers(bufferCount); > + pool_.createBuffers(bufferCount); > > - int ret = dev_->exportBuffers(bufferCount, &pool_); > + int ret = dev_->exportBuffers(&pool_); > if (ret) > return TestFail; > > diff --git a/test/v4l2_device/stream_on_off.cpp b/test/v4l2_device/stream_on_off.cpp > index b564d2a2ab67..861d8d695813 100644 > --- a/test/v4l2_device/stream_on_off.cpp > +++ b/test/v4l2_device/stream_on_off.cpp > @@ -14,9 +14,9 @@ protected: > { > const unsigned int bufferCount = 8; > > - createBuffers(bufferCount); > + pool_.createBuffers(bufferCount); > > - int ret = dev_->exportBuffers(bufferCount, &pool_); > + int ret = dev_->exportBuffers(&pool_); > if (ret) > return TestFail; > > diff --git a/test/v4l2_device/v4l2_device_test.h b/test/v4l2_device/v4l2_device_test.h > index f22f0bb555d8..43539655f85b 100644 > --- a/test/v4l2_device/v4l2_device_test.h > +++ b/test/v4l2_device/v4l2_device_test.h > @@ -24,8 +24,6 @@ class V4L2DeviceTest : public Test > public: > V4L2DeviceTest() : dev_(nullptr){}; > > - void createBuffers(unsigned int qty) { pool_.createBuffers(qty); } > - > protected: > int init(); > void cleanup(); > -- > 2.19.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Kieran, Thank you for the patch. On Thu, Feb 07, 2019 at 09:21:18PM +0000, Kieran Bingham wrote: > exportBuffers() can only operate on an existing BufferPool allocation. The > pool identifies its size through its .count() method. > > Passing a count in to the exportBuffers() call is redundant and can be > incorrect if the value is not the same as the BufferPool size. > > Simplify the function and remove the unnecessary argument, correcting all uses > throughout the code base. > > While we're here, remove the createBuffers() helper from the V4L2DeviceTest > which only served to obfuscate which pool the buffers were being allocated for. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> This looks good to me. I wonder why we added a count parameter in the first place :-) Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/include/v4l2_device.h | 2 +- > src/libcamera/pipeline/ipu3/ipu3.cpp | 3 +-- > src/libcamera/pipeline/uvcvideo.cpp | 2 +- > src/libcamera/pipeline/vimc.cpp | 2 +- > src/libcamera/v4l2_device.cpp | 15 ++++++--------- > test/v4l2_device/buffer_sharing.cpp | 2 +- > test/v4l2_device/capture_async.cpp | 4 ++-- > test/v4l2_device/request_buffers.cpp | 4 ++-- > test/v4l2_device/stream_on_off.cpp | 4 ++-- > test/v4l2_device/v4l2_device_test.h | 2 -- > 10 files changed, 17 insertions(+), 23 deletions(-) > > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h > index 3acb5e466d64..98fa2110efb5 100644 > --- a/src/libcamera/include/v4l2_device.h > +++ b/src/libcamera/include/v4l2_device.h > @@ -97,7 +97,7 @@ public: > int getFormat(V4L2DeviceFormat *format); > int setFormat(V4L2DeviceFormat *format); > > - int exportBuffers(unsigned int count, BufferPool *pool); > + int exportBuffers(BufferPool *pool); > int importBuffers(BufferPool *pool); > int releaseBuffers(); > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 34b03995ae31..677e127dd738 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -191,8 +191,7 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream) > if (!cfg.bufferCount) > return -EINVAL; > > - int ret = data->cio2_->exportBuffers(cfg.bufferCount, > - &stream->bufferPool()); > + int ret = data->cio2_->exportBuffers(&stream->bufferPool()); > if (ret) { > LOG(IPU3, Error) << "Failed to request memory"; > return ret; > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > index ed8228bb2fc6..1a0dcb582670 100644 > --- a/src/libcamera/pipeline/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo.cpp > @@ -102,7 +102,7 @@ int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream) > > LOG(UVC, Debug) << "Requesting " << cfg.bufferCount << " buffers"; > > - return video_->exportBuffers(cfg.bufferCount, &stream->bufferPool()); > + return video_->exportBuffers(&stream->bufferPool()); > } > > int PipelineHandlerUVC::freeBuffers(Camera *camera, Stream *stream) > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > index 0e9ad7b59ee5..06627b1cb847 100644 > --- a/src/libcamera/pipeline/vimc.cpp > +++ b/src/libcamera/pipeline/vimc.cpp > @@ -101,7 +101,7 @@ int PipeHandlerVimc::allocateBuffers(Camera *camera, Stream *stream) > > LOG(VIMC, Debug) << "Requesting " << cfg.bufferCount << " buffers"; > > - return video_->exportBuffers(cfg.bufferCount, &stream->bufferPool()); > + return video_->exportBuffers(&stream->bufferPool()); > } > > int PipeHandlerVimc::freeBuffers(Camera *camera, Stream *stream) > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index c654e6dd7b8d..c2e4d0ea8db2 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -526,13 +526,12 @@ int V4L2Device::requestBuffers(unsigned int count) > } > > /** > - * \brief Request \a count buffers to be allocated from the device and stored in > - * the buffer pool provided. > - * \param[in] count Number of buffers to allocate > + * \brief Request buffers to be allocated from the device and stored in the > + * buffer pool provided. > * \param[out] pool BufferPool to populate with buffers > * \return 0 on success or a negative error code otherwise > */ > -int V4L2Device::exportBuffers(unsigned int count, BufferPool *pool) > +int V4L2Device::exportBuffers(BufferPool *pool) > { > unsigned int allocatedBuffers; > unsigned int i; > @@ -540,21 +539,19 @@ int V4L2Device::exportBuffers(unsigned int count, BufferPool *pool) > > memoryType_ = V4L2_MEMORY_MMAP; > > - ret = requestBuffers(count); > + ret = requestBuffers(pool->count()); > if (ret < 0) > return ret; > > allocatedBuffers = ret; > - if (allocatedBuffers < count) { > + if (allocatedBuffers < pool->count()) { > LOG(V4L2, Error) << "Not enough buffers provided by V4L2Device"; > requestBuffers(0); > return -ENOMEM; > } > > - count = ret; > - > /* Map the buffers. */ > - for (i = 0; i < count; ++i) { > + for (i = 0; i < pool->count(); ++i) { > struct v4l2_plane planes[VIDEO_MAX_PLANES] = {}; > struct v4l2_buffer buf = {}; > struct Buffer &buffer = pool->buffers()[i]; > diff --git a/test/v4l2_device/buffer_sharing.cpp b/test/v4l2_device/buffer_sharing.cpp > index 0e96f7b894bd..ca086f2728f1 100644 > --- a/test/v4l2_device/buffer_sharing.cpp > +++ b/test/v4l2_device/buffer_sharing.cpp > @@ -80,7 +80,7 @@ protected: > > pool_.createBuffers(bufferCount); > > - ret = dev_->exportBuffers(bufferCount, &pool_); > + ret = dev_->exportBuffers(&pool_); > if (ret) > return TestFail; > > diff --git a/test/v4l2_device/capture_async.cpp b/test/v4l2_device/capture_async.cpp > index 7a0735f65535..d3e20ed86b98 100644 > --- a/test/v4l2_device/capture_async.cpp > +++ b/test/v4l2_device/capture_async.cpp > @@ -38,9 +38,9 @@ protected: > Timer timeout; > int ret; > > - createBuffers(bufferCount); > + pool_.createBuffers(bufferCount); > > - ret = dev_->exportBuffers(bufferCount, &pool_); > + ret = dev_->exportBuffers(&pool_); > if (ret) > return TestFail; > > diff --git a/test/v4l2_device/request_buffers.cpp b/test/v4l2_device/request_buffers.cpp > index bc6ff2c18a57..26d7d9528982 100644 > --- a/test/v4l2_device/request_buffers.cpp > +++ b/test/v4l2_device/request_buffers.cpp > @@ -19,9 +19,9 @@ protected: > */ > const unsigned int bufferCount = 8; > > - createBuffers(bufferCount); > + pool_.createBuffers(bufferCount); > > - int ret = dev_->exportBuffers(bufferCount, &pool_); > + int ret = dev_->exportBuffers(&pool_); > if (ret) > return TestFail; > > diff --git a/test/v4l2_device/stream_on_off.cpp b/test/v4l2_device/stream_on_off.cpp > index b564d2a2ab67..861d8d695813 100644 > --- a/test/v4l2_device/stream_on_off.cpp > +++ b/test/v4l2_device/stream_on_off.cpp > @@ -14,9 +14,9 @@ protected: > { > const unsigned int bufferCount = 8; > > - createBuffers(bufferCount); > + pool_.createBuffers(bufferCount); > > - int ret = dev_->exportBuffers(bufferCount, &pool_); > + int ret = dev_->exportBuffers(&pool_); > if (ret) > return TestFail; > > diff --git a/test/v4l2_device/v4l2_device_test.h b/test/v4l2_device/v4l2_device_test.h > index f22f0bb555d8..43539655f85b 100644 > --- a/test/v4l2_device/v4l2_device_test.h > +++ b/test/v4l2_device/v4l2_device_test.h > @@ -24,8 +24,6 @@ class V4L2DeviceTest : public Test > public: > V4L2DeviceTest() : dev_(nullptr){}; > > - void createBuffers(unsigned int qty) { pool_.createBuffers(qty); } > - > protected: > int init(); > void cleanup();
Hi Jacopo, On 07/02/2019 22:28, Jacopo Mondi wrote: > Hi Kieran, > > On Thu, Feb 07, 2019 at 09:21:18PM +0000, Kieran Bingham wrote: >> exportBuffers() can only operate on an existing BufferPool allocation. The > > "an interanlly allocated BufferPool" ? Possibly but I'm not sure how to word this to be accurate. exportBuffers requires a BufferPool to exist and have non-populated buffers. *exportBuffers()* is doing the allocation, so it's not right to say it requries an internally allocated BufferPool - because it requires it to be not yet allocated. >> pool identifies its size through its .count() method. >> >> Passing a count in to the exportBuffers() call is redundant and can be >> incorrect if the value is not the same as the BufferPool size. >> >> Simplify the function and remove the unnecessary argument, correcting all uses >> throughout the code base. >> >> While we're here, remove the createBuffers() helper from the V4L2DeviceTest >> which only served to obfuscate which pool the buffers were being allocated for. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> src/libcamera/include/v4l2_device.h | 2 +- >> src/libcamera/pipeline/ipu3/ipu3.cpp | 3 +-- >> src/libcamera/pipeline/uvcvideo.cpp | 2 +- >> src/libcamera/pipeline/vimc.cpp | 2 +- >> src/libcamera/v4l2_device.cpp | 15 ++++++--------- >> test/v4l2_device/buffer_sharing.cpp | 2 +- >> test/v4l2_device/capture_async.cpp | 4 ++-- >> test/v4l2_device/request_buffers.cpp | 4 ++-- >> test/v4l2_device/stream_on_off.cpp | 4 ++-- >> test/v4l2_device/v4l2_device_test.h | 2 -- >> 10 files changed, 17 insertions(+), 23 deletions(-) >> >> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h >> index 3acb5e466d64..98fa2110efb5 100644 >> --- a/src/libcamera/include/v4l2_device.h >> +++ b/src/libcamera/include/v4l2_device.h >> @@ -97,7 +97,7 @@ public: >> int getFormat(V4L2DeviceFormat *format); >> int setFormat(V4L2DeviceFormat *format); >> >> - int exportBuffers(unsigned int count, BufferPool *pool); >> + int exportBuffers(BufferPool *pool); >> int importBuffers(BufferPool *pool); >> int releaseBuffers(); >> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp >> index 34b03995ae31..677e127dd738 100644 >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp >> @@ -191,8 +191,7 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream) >> if (!cfg.bufferCount) >> return -EINVAL; >> >> - int ret = data->cio2_->exportBuffers(cfg.bufferCount, >> - &stream->bufferPool()); >> + int ret = data->cio2_->exportBuffers(&stream->bufferPool()); >> if (ret) { >> LOG(IPU3, Error) << "Failed to request memory"; >> return ret; >> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp >> index ed8228bb2fc6..1a0dcb582670 100644 >> --- a/src/libcamera/pipeline/uvcvideo.cpp >> +++ b/src/libcamera/pipeline/uvcvideo.cpp >> @@ -102,7 +102,7 @@ int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream) >> >> LOG(UVC, Debug) << "Requesting " << cfg.bufferCount << " buffers"; >> >> - return video_->exportBuffers(cfg.bufferCount, &stream->bufferPool()); >> + return video_->exportBuffers(&stream->bufferPool()); >> } >> >> int PipelineHandlerUVC::freeBuffers(Camera *camera, Stream *stream) >> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp >> index 0e9ad7b59ee5..06627b1cb847 100644 >> --- a/src/libcamera/pipeline/vimc.cpp >> +++ b/src/libcamera/pipeline/vimc.cpp >> @@ -101,7 +101,7 @@ int PipeHandlerVimc::allocateBuffers(Camera *camera, Stream *stream) > >> >> LOG(VIMC, Debug) << "Requesting " << cfg.bufferCount << " buffers"; >> >> - return video_->exportBuffers(cfg.bufferCount, &stream->bufferPool()); >> + return video_->exportBuffers(&stream->bufferPool()); >> } >> >> int PipeHandlerVimc::freeBuffers(Camera *camera, Stream *stream) >> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp >> index c654e6dd7b8d..c2e4d0ea8db2 100644 >> --- a/src/libcamera/v4l2_device.cpp >> +++ b/src/libcamera/v4l2_device.cpp >> @@ -526,13 +526,12 @@ int V4L2Device::requestBuffers(unsigned int count) >> } >> >> /** >> - * \brief Request \a count buffers to be allocated from the device and stored in >> - * the buffer pool provided. >> - * \param[in] count Number of buffers to allocate >> + * \brief Request buffers to be allocated from the device and stored in the >> + * buffer pool provided. >> * \param[out] pool BufferPool to populate with buffers >> * \return 0 on success or a negative error code otherwise >> */ >> -int V4L2Device::exportBuffers(unsigned int count, BufferPool *pool) >> +int V4L2Device::exportBuffers(BufferPool *pool) >> { >> unsigned int allocatedBuffers; >> unsigned int i; >> @@ -540,21 +539,19 @@ int V4L2Device::exportBuffers(unsigned int count, BufferPool *pool) >> >> memoryType_ = V4L2_MEMORY_MMAP; >> >> - ret = requestBuffers(count); >> + ret = requestBuffers(pool->count()); >> if (ret < 0) >> return ret; >> >> allocatedBuffers = ret; >> - if (allocatedBuffers < count) { >> + if (allocatedBuffers < pool->count()) { > > I have a feeling we discussed this, but could you drop > allocatedBuffers now? > requestBuffers returns a signed int. pool->count() is an unsigned int. the allocatedBuffers is essentially a cleaner (I hope) way to write the expression such that it doesn't require casting of the signed/unsigned types comparision. It's safe because the first test "if (ret < 0)" ensures that the value can only be positive. Then the allocatedBuffers (unsigned) is set as the (guaranteed postive) ret. > The rest looks fine! > Thanks > j > >> LOG(V4L2, Error) << "Not enough buffers provided by V4L2Device"; >> requestBuffers(0); >> return -ENOMEM; >> } >> >> - count = ret; >> - >> /* Map the buffers. */ >> - for (i = 0; i < count; ++i) { >> + for (i = 0; i < pool->count(); ++i) { >> struct v4l2_plane planes[VIDEO_MAX_PLANES] = {}; >> struct v4l2_buffer buf = {}; >> struct Buffer &buffer = pool->buffers()[i]; >> diff --git a/test/v4l2_device/buffer_sharing.cpp b/test/v4l2_device/buffer_sharing.cpp >> index 0e96f7b894bd..ca086f2728f1 100644 >> --- a/test/v4l2_device/buffer_sharing.cpp >> +++ b/test/v4l2_device/buffer_sharing.cpp >> @@ -80,7 +80,7 @@ protected: >> >> pool_.createBuffers(bufferCount); >> >> - ret = dev_->exportBuffers(bufferCount, &pool_); >> + ret = dev_->exportBuffers(&pool_); >> if (ret) >> return TestFail; >> >> diff --git a/test/v4l2_device/capture_async.cpp b/test/v4l2_device/capture_async.cpp >> index 7a0735f65535..d3e20ed86b98 100644 >> --- a/test/v4l2_device/capture_async.cpp >> +++ b/test/v4l2_device/capture_async.cpp >> @@ -38,9 +38,9 @@ protected: >> Timer timeout; >> int ret; >> >> - createBuffers(bufferCount); >> + pool_.createBuffers(bufferCount); >> >> - ret = dev_->exportBuffers(bufferCount, &pool_); >> + ret = dev_->exportBuffers(&pool_); >> if (ret) >> return TestFail; >> >> diff --git a/test/v4l2_device/request_buffers.cpp b/test/v4l2_device/request_buffers.cpp >> index bc6ff2c18a57..26d7d9528982 100644 >> --- a/test/v4l2_device/request_buffers.cpp >> +++ b/test/v4l2_device/request_buffers.cpp >> @@ -19,9 +19,9 @@ protected: >> */ >> const unsigned int bufferCount = 8; >> >> - createBuffers(bufferCount); >> + pool_.createBuffers(bufferCount); >> >> - int ret = dev_->exportBuffers(bufferCount, &pool_); >> + int ret = dev_->exportBuffers(&pool_); >> if (ret) >> return TestFail; >> >> diff --git a/test/v4l2_device/stream_on_off.cpp b/test/v4l2_device/stream_on_off.cpp >> index b564d2a2ab67..861d8d695813 100644 >> --- a/test/v4l2_device/stream_on_off.cpp >> +++ b/test/v4l2_device/stream_on_off.cpp >> @@ -14,9 +14,9 @@ protected: >> { >> const unsigned int bufferCount = 8; >> >> - createBuffers(bufferCount); >> + pool_.createBuffers(bufferCount); >> >> - int ret = dev_->exportBuffers(bufferCount, &pool_); >> + int ret = dev_->exportBuffers(&pool_); >> if (ret) >> return TestFail; >> >> diff --git a/test/v4l2_device/v4l2_device_test.h b/test/v4l2_device/v4l2_device_test.h >> index f22f0bb555d8..43539655f85b 100644 >> --- a/test/v4l2_device/v4l2_device_test.h >> +++ b/test/v4l2_device/v4l2_device_test.h >> @@ -24,8 +24,6 @@ class V4L2DeviceTest : public Test >> public: >> V4L2DeviceTest() : dev_(nullptr){}; >> >> - void createBuffers(unsigned int qty) { pool_.createBuffers(qty); } >> - >> protected: >> int init(); >> void cleanup(); >> -- >> 2.19.1 >> >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, On 08/02/2019 17:32, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Thu, Feb 07, 2019 at 09:21:18PM +0000, Kieran Bingham wrote: >> exportBuffers() can only operate on an existing BufferPool allocation. The >> pool identifies its size through its .count() method. >> >> Passing a count in to the exportBuffers() call is redundant and can be >> incorrect if the value is not the same as the BufferPool size. >> >> Simplify the function and remove the unnecessary argument, correcting all uses >> throughout the code base. >> >> While we're here, remove the createBuffers() helper from the V4L2DeviceTest >> which only served to obfuscate which pool the buffers were being allocated for. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > This looks good to me. I wonder why we added a count parameter in the > first place :-) The original implementation used the count parameter to determine how many buffers to add to an empty pool. We modified this to provide an existing sized (but unpopulated) pool. I guess it was easy to miss because the count still looked reasonable :). It was almost techincally still correct but it does hide subtle issues in case the allocatedBuffers != pool.count() so it's best to remove it. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks. > >> --- >> src/libcamera/include/v4l2_device.h | 2 +- >> src/libcamera/pipeline/ipu3/ipu3.cpp | 3 +-- >> src/libcamera/pipeline/uvcvideo.cpp | 2 +- >> src/libcamera/pipeline/vimc.cpp | 2 +- >> src/libcamera/v4l2_device.cpp | 15 ++++++--------- >> test/v4l2_device/buffer_sharing.cpp | 2 +- >> test/v4l2_device/capture_async.cpp | 4 ++-- >> test/v4l2_device/request_buffers.cpp | 4 ++-- >> test/v4l2_device/stream_on_off.cpp | 4 ++-- >> test/v4l2_device/v4l2_device_test.h | 2 -- >> 10 files changed, 17 insertions(+), 23 deletions(-) >> >> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h >> index 3acb5e466d64..98fa2110efb5 100644 >> --- a/src/libcamera/include/v4l2_device.h >> +++ b/src/libcamera/include/v4l2_device.h >> @@ -97,7 +97,7 @@ public: >> int getFormat(V4L2DeviceFormat *format); >> int setFormat(V4L2DeviceFormat *format); >> >> - int exportBuffers(unsigned int count, BufferPool *pool); >> + int exportBuffers(BufferPool *pool); >> int importBuffers(BufferPool *pool); >> int releaseBuffers(); >> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp >> index 34b03995ae31..677e127dd738 100644 >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp >> @@ -191,8 +191,7 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream) >> if (!cfg.bufferCount) >> return -EINVAL; >> >> - int ret = data->cio2_->exportBuffers(cfg.bufferCount, >> - &stream->bufferPool()); >> + int ret = data->cio2_->exportBuffers(&stream->bufferPool()); >> if (ret) { >> LOG(IPU3, Error) << "Failed to request memory"; >> return ret; >> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp >> index ed8228bb2fc6..1a0dcb582670 100644 >> --- a/src/libcamera/pipeline/uvcvideo.cpp >> +++ b/src/libcamera/pipeline/uvcvideo.cpp >> @@ -102,7 +102,7 @@ int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream) >> >> LOG(UVC, Debug) << "Requesting " << cfg.bufferCount << " buffers"; >> >> - return video_->exportBuffers(cfg.bufferCount, &stream->bufferPool()); >> + return video_->exportBuffers(&stream->bufferPool()); >> } >> >> int PipelineHandlerUVC::freeBuffers(Camera *camera, Stream *stream) >> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp >> index 0e9ad7b59ee5..06627b1cb847 100644 >> --- a/src/libcamera/pipeline/vimc.cpp >> +++ b/src/libcamera/pipeline/vimc.cpp >> @@ -101,7 +101,7 @@ int PipeHandlerVimc::allocateBuffers(Camera *camera, Stream *stream) >> >> LOG(VIMC, Debug) << "Requesting " << cfg.bufferCount << " buffers"; >> >> - return video_->exportBuffers(cfg.bufferCount, &stream->bufferPool()); >> + return video_->exportBuffers(&stream->bufferPool()); >> } >> >> int PipeHandlerVimc::freeBuffers(Camera *camera, Stream *stream) >> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp >> index c654e6dd7b8d..c2e4d0ea8db2 100644 >> --- a/src/libcamera/v4l2_device.cpp >> +++ b/src/libcamera/v4l2_device.cpp >> @@ -526,13 +526,12 @@ int V4L2Device::requestBuffers(unsigned int count) >> } >> >> /** >> - * \brief Request \a count buffers to be allocated from the device and stored in >> - * the buffer pool provided. >> - * \param[in] count Number of buffers to allocate >> + * \brief Request buffers to be allocated from the device and stored in the >> + * buffer pool provided. >> * \param[out] pool BufferPool to populate with buffers >> * \return 0 on success or a negative error code otherwise >> */ >> -int V4L2Device::exportBuffers(unsigned int count, BufferPool *pool) >> +int V4L2Device::exportBuffers(BufferPool *pool) >> { >> unsigned int allocatedBuffers; >> unsigned int i; >> @@ -540,21 +539,19 @@ int V4L2Device::exportBuffers(unsigned int count, BufferPool *pool) >> >> memoryType_ = V4L2_MEMORY_MMAP; >> >> - ret = requestBuffers(count); >> + ret = requestBuffers(pool->count()); >> if (ret < 0) >> return ret; >> >> allocatedBuffers = ret; >> - if (allocatedBuffers < count) { >> + if (allocatedBuffers < pool->count()) { >> LOG(V4L2, Error) << "Not enough buffers provided by V4L2Device"; >> requestBuffers(0); >> return -ENOMEM; >> } >> >> - count = ret; >> - >> /* Map the buffers. */ >> - for (i = 0; i < count; ++i) { >> + for (i = 0; i < pool->count(); ++i) { >> struct v4l2_plane planes[VIDEO_MAX_PLANES] = {}; >> struct v4l2_buffer buf = {}; >> struct Buffer &buffer = pool->buffers()[i]; >> diff --git a/test/v4l2_device/buffer_sharing.cpp b/test/v4l2_device/buffer_sharing.cpp >> index 0e96f7b894bd..ca086f2728f1 100644 >> --- a/test/v4l2_device/buffer_sharing.cpp >> +++ b/test/v4l2_device/buffer_sharing.cpp >> @@ -80,7 +80,7 @@ protected: >> >> pool_.createBuffers(bufferCount); >> >> - ret = dev_->exportBuffers(bufferCount, &pool_); >> + ret = dev_->exportBuffers(&pool_); >> if (ret) >> return TestFail; >> >> diff --git a/test/v4l2_device/capture_async.cpp b/test/v4l2_device/capture_async.cpp >> index 7a0735f65535..d3e20ed86b98 100644 >> --- a/test/v4l2_device/capture_async.cpp >> +++ b/test/v4l2_device/capture_async.cpp >> @@ -38,9 +38,9 @@ protected: >> Timer timeout; >> int ret; >> >> - createBuffers(bufferCount); >> + pool_.createBuffers(bufferCount); >> >> - ret = dev_->exportBuffers(bufferCount, &pool_); >> + ret = dev_->exportBuffers(&pool_); >> if (ret) >> return TestFail; >> >> diff --git a/test/v4l2_device/request_buffers.cpp b/test/v4l2_device/request_buffers.cpp >> index bc6ff2c18a57..26d7d9528982 100644 >> --- a/test/v4l2_device/request_buffers.cpp >> +++ b/test/v4l2_device/request_buffers.cpp >> @@ -19,9 +19,9 @@ protected: >> */ >> const unsigned int bufferCount = 8; >> >> - createBuffers(bufferCount); >> + pool_.createBuffers(bufferCount); >> >> - int ret = dev_->exportBuffers(bufferCount, &pool_); >> + int ret = dev_->exportBuffers(&pool_); >> if (ret) >> return TestFail; >> >> diff --git a/test/v4l2_device/stream_on_off.cpp b/test/v4l2_device/stream_on_off.cpp >> index b564d2a2ab67..861d8d695813 100644 >> --- a/test/v4l2_device/stream_on_off.cpp >> +++ b/test/v4l2_device/stream_on_off.cpp >> @@ -14,9 +14,9 @@ protected: >> { >> const unsigned int bufferCount = 8; >> >> - createBuffers(bufferCount); >> + pool_.createBuffers(bufferCount); >> >> - int ret = dev_->exportBuffers(bufferCount, &pool_); >> + int ret = dev_->exportBuffers(&pool_); >> if (ret) >> return TestFail; >> >> diff --git a/test/v4l2_device/v4l2_device_test.h b/test/v4l2_device/v4l2_device_test.h >> index f22f0bb555d8..43539655f85b 100644 >> --- a/test/v4l2_device/v4l2_device_test.h >> +++ b/test/v4l2_device/v4l2_device_test.h >> @@ -24,8 +24,6 @@ class V4L2DeviceTest : public Test >> public: >> V4L2DeviceTest() : dev_(nullptr){}; >> >> - void createBuffers(unsigned int qty) { pool_.createBuffers(qty); } >> - >> protected: >> int init(); >> void cleanup(); >
On Mon, Feb 11, 2019 at 11:57:01AM +0000, Kieran Bingham wrote: > Hi Jacopo, > > On 07/02/2019 22:28, Jacopo Mondi wrote: > > Hi Kieran, > > > > On Thu, Feb 07, 2019 at 09:21:18PM +0000, Kieran Bingham wrote: > >> exportBuffers() can only operate on an existing BufferPool allocation. The > > > > "an interanlly allocated BufferPool" ? > > Possibly but I'm not sure how to word this to be accurate. > > exportBuffers requires a BufferPool to exist and have non-populated buffers. > > *exportBuffers()* is doing the allocation, so it's not right to say it > requries an internally allocated BufferPool - because it requires it to > be not yet allocated. Correct, my bad for the confusion. whatever is fine, is a commit message... > > > >> pool identifies its size through its .count() method. > >> > >> Passing a count in to the exportBuffers() call is redundant and can be > >> incorrect if the value is not the same as the BufferPool size. > >> > >> Simplify the function and remove the unnecessary argument, correcting all uses > >> throughout the code base. > >> > >> While we're here, remove the createBuffers() helper from the V4L2DeviceTest > >> which only served to obfuscate which pool the buffers were being allocated for. > >> > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> --- > >> src/libcamera/include/v4l2_device.h | 2 +- > >> src/libcamera/pipeline/ipu3/ipu3.cpp | 3 +-- > >> src/libcamera/pipeline/uvcvideo.cpp | 2 +- > >> src/libcamera/pipeline/vimc.cpp | 2 +- > >> src/libcamera/v4l2_device.cpp | 15 ++++++--------- > >> test/v4l2_device/buffer_sharing.cpp | 2 +- > >> test/v4l2_device/capture_async.cpp | 4 ++-- > >> test/v4l2_device/request_buffers.cpp | 4 ++-- > >> test/v4l2_device/stream_on_off.cpp | 4 ++-- > >> test/v4l2_device/v4l2_device_test.h | 2 -- > >> 10 files changed, 17 insertions(+), 23 deletions(-) > >> > >> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h > >> index 3acb5e466d64..98fa2110efb5 100644 > >> --- a/src/libcamera/include/v4l2_device.h > >> +++ b/src/libcamera/include/v4l2_device.h > >> @@ -97,7 +97,7 @@ public: > >> int getFormat(V4L2DeviceFormat *format); > >> int setFormat(V4L2DeviceFormat *format); > >> > >> - int exportBuffers(unsigned int count, BufferPool *pool); > >> + int exportBuffers(BufferPool *pool); > >> int importBuffers(BufferPool *pool); > >> int releaseBuffers(); > >> > >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> index 34b03995ae31..677e127dd738 100644 > >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> @@ -191,8 +191,7 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream) > >> if (!cfg.bufferCount) > >> return -EINVAL; > >> > >> - int ret = data->cio2_->exportBuffers(cfg.bufferCount, > >> - &stream->bufferPool()); > >> + int ret = data->cio2_->exportBuffers(&stream->bufferPool()); > >> if (ret) { > >> LOG(IPU3, Error) << "Failed to request memory"; > >> return ret; > >> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > >> index ed8228bb2fc6..1a0dcb582670 100644 > >> --- a/src/libcamera/pipeline/uvcvideo.cpp > >> +++ b/src/libcamera/pipeline/uvcvideo.cpp > >> @@ -102,7 +102,7 @@ int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream) > >> > >> LOG(UVC, Debug) << "Requesting " << cfg.bufferCount << " buffers"; > >> > >> - return video_->exportBuffers(cfg.bufferCount, &stream->bufferPool()); > >> + return video_->exportBuffers(&stream->bufferPool()); > >> } > >> > >> int PipelineHandlerUVC::freeBuffers(Camera *camera, Stream *stream) > >> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > >> index 0e9ad7b59ee5..06627b1cb847 100644 > >> --- a/src/libcamera/pipeline/vimc.cpp > >> +++ b/src/libcamera/pipeline/vimc.cpp > >> @@ -101,7 +101,7 @@ int PipeHandlerVimc::allocateBuffers(Camera *camera, Stream *stream) > > > >> > >> LOG(VIMC, Debug) << "Requesting " << cfg.bufferCount << " buffers"; > >> > >> - return video_->exportBuffers(cfg.bufferCount, &stream->bufferPool()); > >> + return video_->exportBuffers(&stream->bufferPool()); > >> } > >> > >> int PipeHandlerVimc::freeBuffers(Camera *camera, Stream *stream) > >> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > >> index c654e6dd7b8d..c2e4d0ea8db2 100644 > >> --- a/src/libcamera/v4l2_device.cpp > >> +++ b/src/libcamera/v4l2_device.cpp > >> @@ -526,13 +526,12 @@ int V4L2Device::requestBuffers(unsigned int count) > >> } > >> > >> /** > >> - * \brief Request \a count buffers to be allocated from the device and stored in > >> - * the buffer pool provided. > >> - * \param[in] count Number of buffers to allocate > >> + * \brief Request buffers to be allocated from the device and stored in the > >> + * buffer pool provided. > >> * \param[out] pool BufferPool to populate with buffers > >> * \return 0 on success or a negative error code otherwise > >> */ > >> -int V4L2Device::exportBuffers(unsigned int count, BufferPool *pool) > >> +int V4L2Device::exportBuffers(BufferPool *pool) > >> { > >> unsigned int allocatedBuffers; > >> unsigned int i; > >> @@ -540,21 +539,19 @@ int V4L2Device::exportBuffers(unsigned int count, BufferPool *pool) > >> > >> memoryType_ = V4L2_MEMORY_MMAP; > >> > >> - ret = requestBuffers(count); > >> + ret = requestBuffers(pool->count()); > >> if (ret < 0) > >> return ret; > >> > >> allocatedBuffers = ret; > >> - if (allocatedBuffers < count) { > >> + if (allocatedBuffers < pool->count()) { > > > > I have a feeling we discussed this, but could you drop > > allocatedBuffers now? > > > > requestBuffers returns a signed int. pool->count() is an unsigned int. > > the allocatedBuffers is essentially a cleaner (I hope) way to write the > expression such that it doesn't require casting of the signed/unsigned > types comparision. > > It's safe because the first test "if (ret < 0)" ensures that the value > can only be positive. Then the allocatedBuffers (unsigned) is set as the > (guaranteed postive) ret. Great, please have my Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> As I suspect a v2 will come, I'll review other patches in the next iteration. Thanks j > > > > > The rest looks fine! > > Thanks > > j > > > >> LOG(V4L2, Error) << "Not enough buffers provided by V4L2Device"; > >> requestBuffers(0); > >> return -ENOMEM; > >> } > >> > >> - count = ret; > >> - > >> /* Map the buffers. */ > >> - for (i = 0; i < count; ++i) { > >> + for (i = 0; i < pool->count(); ++i) { > >> struct v4l2_plane planes[VIDEO_MAX_PLANES] = {}; > >> struct v4l2_buffer buf = {}; > >> struct Buffer &buffer = pool->buffers()[i]; > >> diff --git a/test/v4l2_device/buffer_sharing.cpp b/test/v4l2_device/buffer_sharing.cpp > >> index 0e96f7b894bd..ca086f2728f1 100644 > >> --- a/test/v4l2_device/buffer_sharing.cpp > >> +++ b/test/v4l2_device/buffer_sharing.cpp > >> @@ -80,7 +80,7 @@ protected: > >> > >> pool_.createBuffers(bufferCount); > >> > >> - ret = dev_->exportBuffers(bufferCount, &pool_); > >> + ret = dev_->exportBuffers(&pool_); > >> if (ret) > >> return TestFail; > >> > >> diff --git a/test/v4l2_device/capture_async.cpp b/test/v4l2_device/capture_async.cpp > >> index 7a0735f65535..d3e20ed86b98 100644 > >> --- a/test/v4l2_device/capture_async.cpp > >> +++ b/test/v4l2_device/capture_async.cpp > >> @@ -38,9 +38,9 @@ protected: > >> Timer timeout; > >> int ret; > >> > >> - createBuffers(bufferCount); > >> + pool_.createBuffers(bufferCount); > >> > >> - ret = dev_->exportBuffers(bufferCount, &pool_); > >> + ret = dev_->exportBuffers(&pool_); > >> if (ret) > >> return TestFail; > >> > >> diff --git a/test/v4l2_device/request_buffers.cpp b/test/v4l2_device/request_buffers.cpp > >> index bc6ff2c18a57..26d7d9528982 100644 > >> --- a/test/v4l2_device/request_buffers.cpp > >> +++ b/test/v4l2_device/request_buffers.cpp > >> @@ -19,9 +19,9 @@ protected: > >> */ > >> const unsigned int bufferCount = 8; > >> > >> - createBuffers(bufferCount); > >> + pool_.createBuffers(bufferCount); > >> > >> - int ret = dev_->exportBuffers(bufferCount, &pool_); > >> + int ret = dev_->exportBuffers(&pool_); > >> if (ret) > >> return TestFail; > >> > >> diff --git a/test/v4l2_device/stream_on_off.cpp b/test/v4l2_device/stream_on_off.cpp > >> index b564d2a2ab67..861d8d695813 100644 > >> --- a/test/v4l2_device/stream_on_off.cpp > >> +++ b/test/v4l2_device/stream_on_off.cpp > >> @@ -14,9 +14,9 @@ protected: > >> { > >> const unsigned int bufferCount = 8; > >> > >> - createBuffers(bufferCount); > >> + pool_.createBuffers(bufferCount); > >> > >> - int ret = dev_->exportBuffers(bufferCount, &pool_); > >> + int ret = dev_->exportBuffers(&pool_); > >> if (ret) > >> return TestFail; > >> > >> diff --git a/test/v4l2_device/v4l2_device_test.h b/test/v4l2_device/v4l2_device_test.h > >> index f22f0bb555d8..43539655f85b 100644 > >> --- a/test/v4l2_device/v4l2_device_test.h > >> +++ b/test/v4l2_device/v4l2_device_test.h > >> @@ -24,8 +24,6 @@ class V4L2DeviceTest : public Test > >> public: > >> V4L2DeviceTest() : dev_(nullptr){}; > >> > >> - void createBuffers(unsigned int qty) { pool_.createBuffers(qty); } > >> - > >> protected: > >> int init(); > >> void cleanup(); > >> -- > >> 2.19.1 > >> > >> _______________________________________________ > >> libcamera-devel mailing list > >> libcamera-devel@lists.libcamera.org > >> https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards > -- > Kieran
diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h index 3acb5e466d64..98fa2110efb5 100644 --- a/src/libcamera/include/v4l2_device.h +++ b/src/libcamera/include/v4l2_device.h @@ -97,7 +97,7 @@ public: int getFormat(V4L2DeviceFormat *format); int setFormat(V4L2DeviceFormat *format); - int exportBuffers(unsigned int count, BufferPool *pool); + int exportBuffers(BufferPool *pool); int importBuffers(BufferPool *pool); int releaseBuffers(); diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 34b03995ae31..677e127dd738 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -191,8 +191,7 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream) if (!cfg.bufferCount) return -EINVAL; - int ret = data->cio2_->exportBuffers(cfg.bufferCount, - &stream->bufferPool()); + int ret = data->cio2_->exportBuffers(&stream->bufferPool()); if (ret) { LOG(IPU3, Error) << "Failed to request memory"; return ret; diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp index ed8228bb2fc6..1a0dcb582670 100644 --- a/src/libcamera/pipeline/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo.cpp @@ -102,7 +102,7 @@ int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream) LOG(UVC, Debug) << "Requesting " << cfg.bufferCount << " buffers"; - return video_->exportBuffers(cfg.bufferCount, &stream->bufferPool()); + return video_->exportBuffers(&stream->bufferPool()); } int PipelineHandlerUVC::freeBuffers(Camera *camera, Stream *stream) diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp index 0e9ad7b59ee5..06627b1cb847 100644 --- a/src/libcamera/pipeline/vimc.cpp +++ b/src/libcamera/pipeline/vimc.cpp @@ -101,7 +101,7 @@ int PipeHandlerVimc::allocateBuffers(Camera *camera, Stream *stream) LOG(VIMC, Debug) << "Requesting " << cfg.bufferCount << " buffers"; - return video_->exportBuffers(cfg.bufferCount, &stream->bufferPool()); + return video_->exportBuffers(&stream->bufferPool()); } int PipeHandlerVimc::freeBuffers(Camera *camera, Stream *stream) diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index c654e6dd7b8d..c2e4d0ea8db2 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -526,13 +526,12 @@ int V4L2Device::requestBuffers(unsigned int count) } /** - * \brief Request \a count buffers to be allocated from the device and stored in - * the buffer pool provided. - * \param[in] count Number of buffers to allocate + * \brief Request buffers to be allocated from the device and stored in the + * buffer pool provided. * \param[out] pool BufferPool to populate with buffers * \return 0 on success or a negative error code otherwise */ -int V4L2Device::exportBuffers(unsigned int count, BufferPool *pool) +int V4L2Device::exportBuffers(BufferPool *pool) { unsigned int allocatedBuffers; unsigned int i; @@ -540,21 +539,19 @@ int V4L2Device::exportBuffers(unsigned int count, BufferPool *pool) memoryType_ = V4L2_MEMORY_MMAP; - ret = requestBuffers(count); + ret = requestBuffers(pool->count()); if (ret < 0) return ret; allocatedBuffers = ret; - if (allocatedBuffers < count) { + if (allocatedBuffers < pool->count()) { LOG(V4L2, Error) << "Not enough buffers provided by V4L2Device"; requestBuffers(0); return -ENOMEM; } - count = ret; - /* Map the buffers. */ - for (i = 0; i < count; ++i) { + for (i = 0; i < pool->count(); ++i) { struct v4l2_plane planes[VIDEO_MAX_PLANES] = {}; struct v4l2_buffer buf = {}; struct Buffer &buffer = pool->buffers()[i]; diff --git a/test/v4l2_device/buffer_sharing.cpp b/test/v4l2_device/buffer_sharing.cpp index 0e96f7b894bd..ca086f2728f1 100644 --- a/test/v4l2_device/buffer_sharing.cpp +++ b/test/v4l2_device/buffer_sharing.cpp @@ -80,7 +80,7 @@ protected: pool_.createBuffers(bufferCount); - ret = dev_->exportBuffers(bufferCount, &pool_); + ret = dev_->exportBuffers(&pool_); if (ret) return TestFail; diff --git a/test/v4l2_device/capture_async.cpp b/test/v4l2_device/capture_async.cpp index 7a0735f65535..d3e20ed86b98 100644 --- a/test/v4l2_device/capture_async.cpp +++ b/test/v4l2_device/capture_async.cpp @@ -38,9 +38,9 @@ protected: Timer timeout; int ret; - createBuffers(bufferCount); + pool_.createBuffers(bufferCount); - ret = dev_->exportBuffers(bufferCount, &pool_); + ret = dev_->exportBuffers(&pool_); if (ret) return TestFail; diff --git a/test/v4l2_device/request_buffers.cpp b/test/v4l2_device/request_buffers.cpp index bc6ff2c18a57..26d7d9528982 100644 --- a/test/v4l2_device/request_buffers.cpp +++ b/test/v4l2_device/request_buffers.cpp @@ -19,9 +19,9 @@ protected: */ const unsigned int bufferCount = 8; - createBuffers(bufferCount); + pool_.createBuffers(bufferCount); - int ret = dev_->exportBuffers(bufferCount, &pool_); + int ret = dev_->exportBuffers(&pool_); if (ret) return TestFail; diff --git a/test/v4l2_device/stream_on_off.cpp b/test/v4l2_device/stream_on_off.cpp index b564d2a2ab67..861d8d695813 100644 --- a/test/v4l2_device/stream_on_off.cpp +++ b/test/v4l2_device/stream_on_off.cpp @@ -14,9 +14,9 @@ protected: { const unsigned int bufferCount = 8; - createBuffers(bufferCount); + pool_.createBuffers(bufferCount); - int ret = dev_->exportBuffers(bufferCount, &pool_); + int ret = dev_->exportBuffers(&pool_); if (ret) return TestFail; diff --git a/test/v4l2_device/v4l2_device_test.h b/test/v4l2_device/v4l2_device_test.h index f22f0bb555d8..43539655f85b 100644 --- a/test/v4l2_device/v4l2_device_test.h +++ b/test/v4l2_device/v4l2_device_test.h @@ -24,8 +24,6 @@ class V4L2DeviceTest : public Test public: V4L2DeviceTest() : dev_(nullptr){}; - void createBuffers(unsigned int qty) { pool_.createBuffers(qty); } - protected: int init(); void cleanup();
exportBuffers() can only operate on an existing BufferPool allocation. The pool identifies its size through its .count() method. Passing a count in to the exportBuffers() call is redundant and can be incorrect if the value is not the same as the BufferPool size. Simplify the function and remove the unnecessary argument, correcting all uses throughout the code base. While we're here, remove the createBuffers() helper from the V4L2DeviceTest which only served to obfuscate which pool the buffers were being allocated for. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/libcamera/include/v4l2_device.h | 2 +- src/libcamera/pipeline/ipu3/ipu3.cpp | 3 +-- src/libcamera/pipeline/uvcvideo.cpp | 2 +- src/libcamera/pipeline/vimc.cpp | 2 +- src/libcamera/v4l2_device.cpp | 15 ++++++--------- test/v4l2_device/buffer_sharing.cpp | 2 +- test/v4l2_device/capture_async.cpp | 4 ++-- test/v4l2_device/request_buffers.cpp | 4 ++-- test/v4l2_device/stream_on_off.cpp | 4 ++-- test/v4l2_device/v4l2_device_test.h | 2 -- 10 files changed, 17 insertions(+), 23 deletions(-)