[libcamera-devel] [PATCH 4/5] libcamera: v4l2_device: Simplify exportBuffers()
Jacopo Mondi
jacopo at jmondi.org
Tue Feb 12 09:52:50 CET 2019
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 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.
Great, please have my
Reviewed-by: Jacopo Mondi <jacopo at 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 at lists.libcamera.org
> >> https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards
> --
> Kieran
-------------- 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/20190212/cebb3e9c/attachment.sig>
More information about the libcamera-devel
mailing list