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