[libcamera-devel] [PATCH 19/30] libcamera: v4l2_videodevice: Add new buffer interface
Jacopo Mondi
jacopo at jmondi.org
Mon Dec 2 12:25:48 CET 2019
Hi Niklas,
On Wed, Nov 27, 2019 at 12:36:09AM +0100, Niklas Söderlund wrote:
> Extend V4L2VideoDevice with two new functions, one to deal with
> allocating buffers from the video device and one to prepare the video
> device to use external buffers.
>
> The two new functions intend to replace exportBuffers() and
> importBuffers() once the transition to the FrameBuffer interface is
> complete.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> src/libcamera/include/v4l2_videodevice.h | 4 +
> src/libcamera/v4l2_videodevice.cpp | 126 ++++++++++++++++++++++-
> 2 files changed, 129 insertions(+), 1 deletion(-)
>
> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> index 254f8797af42dd8a..f4cbcfbd26ea540c 100644
> --- a/src/libcamera/include/v4l2_videodevice.h
> +++ b/src/libcamera/include/v4l2_videodevice.h
> @@ -162,6 +162,8 @@ public:
>
> int exportBuffers(BufferPool *pool);
> int importBuffers(BufferPool *pool);
> + int allocateBuffers(unsigned int count, std::vector<FrameBuffer *> *buffers);
> + int externalBuffers(unsigned int count);
I agree with s/export/allocate, but why s/import/external ?
Furthermore allocate/import is a nice combination of names
> int releaseBuffers();
>
> int queueBuffer(Buffer *buffer);
> @@ -197,6 +199,7 @@ private:
> int requestBuffers(unsigned int count);
> int createPlane(BufferMemory *buffer, unsigned int index,
> unsigned int plane, unsigned int length);
> + FrameBuffer *createBuffer(struct v4l2_buffer buf);
> int exportDmaBuffer(unsigned int index, unsigned int plane);
>
> Buffer *dequeueBuffer();
> @@ -208,6 +211,7 @@ private:
> enum v4l2_memory memoryType_;
>
> BufferPool *bufferPool_;
> + V4L2BufferCache *cache_;
> std::map<unsigned int, Buffer *> queuedBuffers_;
>
> EventNotifier *fdEvent_;
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index c82f2829601bd14c..9fe66018ec502626 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -377,7 +377,8 @@ const std::string V4L2DeviceFormat::toString() const
> * \param[in] deviceNode The file-system path to the video device node
> */
> V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)
> - : V4L2Device(deviceNode), bufferPool_(nullptr), fdEvent_(nullptr)
> + : V4L2Device(deviceNode), bufferPool_(nullptr), cache_(nullptr),
> + fdEvent_(nullptr)
> {
> /*
> * We default to an MMAP based CAPTURE video device, however this will
> @@ -1042,6 +1043,100 @@ int V4L2VideoDevice::importBuffers(BufferPool *pool)
> return 0;
> }
>
> +/**
> + * \brief Operate using buffers allocated from local video device
Allocate buffers from (in?) the video device
> + * \param[in] count Number of buffers to allocate
> + * \param[out] buffers Vector to store local buffers
s/local// ?
> + * \return 0 on success or a negative error code otherwise
> + */
> +int V4L2VideoDevice::allocateBuffers(unsigned int count, std::vector<FrameBuffer *> *buffers)
> +{
> + unsigned int i;
> + int ret;
declare variables when you first use them
> +
> + if (cache_) {
> + LOG(V4L2, Error) << "Can't allocate buffers when cache exists";
The presence of cache_ is an indication that memory has been
allocated, not the sole reason of failure. I would
LOG(V4L2, Error) << "Can't allocate buffers multiple times";
Or something similar
> + return -EINVAL;
> + }
> +
> + memoryType_ = V4L2_MEMORY_MMAP;
> +
> + ret = requestBuffers(count);
> + if (ret < 0)
> + return ret;
> +
> + for (i = 0; i < count; ++i) {
Furthermore i is only used inside this loop
> + struct v4l2_buffer buf = {};
> + struct v4l2_plane planes[VIDEO_MAX_PLANES] = {};
> +
> + buf.index = i;
> + buf.type = bufferType_;
> + buf.memory = memoryType_;
> + buf.length = VIDEO_MAX_PLANES;
> + buf.m.planes = planes;
> +
> + ret = ioctl(VIDIOC_QUERYBUF, &buf);
> + if (ret < 0) {
ret = -errno;
> + LOG(V4L2, Error)
> + << "Unable to query buffer " << i << ": "
> + << strerror(-ret);
> + goto err_buf;
> + }
> +
> + FrameBuffer *buffer = createBuffer(buf);
> + if (!buffer) {
> + LOG(V4L2, Error) << "Unable to create buffer";
> + ret = -EINVAL;
> + goto err_buf;
> + }
> +
> + buffers->push_back(buffer);
We're dealing with pointers, so not a huge loss, but couldn't you
provide the buffers vector to createBuffers and make that operation
add the newly created buffer to the vector. In this way you could
return an error code..
> + }
> +
> + cache_ = new V4L2BufferCache(*buffers);
> +
> + return count;
empty line ?
> +err_buf:
> + requestBuffers(0);
> +
> + for (FrameBuffer *buffer : *buffers)
> + delete buffer;
> +
> + buffers->clear();
> +
> + return ret;
> +}
> +
> +FrameBuffer *V4L2VideoDevice::createBuffer(struct v4l2_buffer buf)
> +{
> + const unsigned int numPlanes = V4L2_TYPE_IS_MULTIPLANAR(buf.type) ? buf.length : 1;
Can you break at 80 cols here?
> + std::vector<FrameBuffer::Plane> planes;
> + FrameBuffer *frame = nullptr;
> +
> + for (unsigned int nplane = 0; nplane < numPlanes; nplane++) {
> + int fd = exportDmaBuffer(buf.index, nplane);
> + if (fd < 0)
> + goto out;
> +
> + FrameBuffer::Plane plane;
> + plane.fd = fd;
> + plane.length = V4L2_TYPE_IS_MULTIPLANAR(buf.type) ?
> + buf.m.planes[nplane].length : buf.length;
> +
> + planes.emplace_back(plane);
> + }
> +
> + if (!planes.empty())
> + frame = new FrameBuffer(planes);
> + else
> + LOG(V4L2, Error) << "Failed to get planes";
> +out:
> + for (FrameBuffer::Plane &plane : planes)
> + ::close(plane.fd);
Can the plane fd be closed -before- creating the FrameBuffer ?
If so you could
Anway, I would return earlier if planes.empty()
if (planes.empty()) {
LOG(V4L2, Error) << "Failed to get planes";
return nullptr;
}
FrameBufffer *frame = new FrameBuffer(planes);
for (FrameBuffer::Plane &plane : planes)
::close(plane.fd);
return frame;
Or if you provide buffers to this operation
buffers.emaplce_back(planes);
return 0;
> +
> + return frame;
> +}
> +
> int V4L2VideoDevice::exportDmaBuffer(unsigned int index, unsigned int plane)
> {
> struct v4l2_exportbuffer expbuf = {};
> @@ -1062,6 +1157,35 @@ int V4L2VideoDevice::exportDmaBuffer(unsigned int index, unsigned int plane)
> return expbuf.fd;
> }
>
> +/**
> + * \brief Operate using buffers imported from external source
Import buffers externally allocated into the video device
> + * \param[in] count Number of buffers to prepare for
to prepare for/to import
> + * \return 0 on success or a negative error code otherwise
> + */
> +int V4L2VideoDevice::externalBuffers(unsigned int count)
> +{
> + /*
> + * We can only prepare the device to use external buffers when no
> + * internal buffers have been allocated.
> + */
> + if (cache_)
> + return 0;
I would log an error, as you do the same in allocate
> +
> + int ret;
declare when use
> +
> + memoryType_ = V4L2_MEMORY_DMABUF;
> +
> + ret = requestBuffers(count);
> + if (ret)
> + return ret;
> +
> + cache_ = new V4L2BufferCache(count);
> +
> + LOG(V4L2, Debug) << "provided for " << count << " external buffers";
"provided for" sounds weird to me
Thanks
j
> +
> + return 0;
> +}
> +
> /**
> * \brief Release all internally allocated buffers
> */
> --
> 2.24.0
>
> _______________________________________________
> 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/20191202/617d4a18/attachment.sig>
More information about the libcamera-devel
mailing list