[libcamera-devel] [PATCH 4/5] libcamera: v4l2_device: Simplify exportBuffers()
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Feb 11 12:59:05 CET 2019
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 at 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 at 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();
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list