[libcamera-devel] [PATCH 4/5] libcamera: v4l2_device: Simplify exportBuffers()
Jacopo Mondi
jacopo at jmondi.org
Thu Feb 7 23:28:29 CET 2019
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 at 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190207/8e963659/attachment.sig>
More information about the libcamera-devel
mailing list