[libcamera-devel] [PATCH 19/30] libcamera: v4l2_videodevice: Add new buffer interface

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Mon, Dec 02, 2019 at 12:25:48PM +0100, Jacopo Mondi wrote:
> 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

I agree with Jacopo here, externalBuffers() is a bad name. You could
reuse the names exportBuffers() and importBuffers() as the arguments are
different. importBuffers() isn't the best name ever though, as the
method doesn't really import buffers, so feel free to propose better
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);

const struct v4l2_buffer &buf

It should have become an automatic reflex by now :-)

> >  	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// ?

s/local/allocated/

> > + * \return 0 on success or a negative error code otherwise
> > + */
> > +int V4L2VideoDevice::allocateBuffers(unsigned int count, std::vector<FrameBuffer *> *buffers)

I would wrap this to avoid long lines.

> > +{
> > +	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

"Buffers already allocated" ?

> > +		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

	for (unsigned int i = 0 ...)

> > +		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;

ARRAY_SIZE(planes) ?

> > +		buf.m.planes = planes;
> > +
> > +		ret = ioctl(VIDIOC_QUERYBUF, &buf);
> > +		if (ret < 0) {
> 
> ret = -errno;

This is V4L2Device::ioctl(), not std::ioctl(), so the function returns
the error code.

> > +			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

s/createBuffers/createBuffer/

> add the newly created buffer to the vector. In this way you could
> return an error code..

I think it would make the interface of createBuffer() a bit more
cryptic, but it's an internal API.

I'm tempted to start using ERR_PTR() but I don't think it would be a
good idea :-)

> > +	}
> > +
> > +	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)

How about calling this method createFrameBuffer() ?

> > +{
> > +	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;

s/frame/frameBuffer/

> > +
> > +	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);

This won't save anything compared to push_back(). You should use

		planes.emplace_back(plane.fd, length);

with a constructor for FrameBuffer::Plane, or use .push_back() to avoid
giving the false impression that the code is optimized.

> > +	}
> > +
> > +	if (!planes.empty())
> > +		frame = new FrameBuffer(planes);
> > +	else
> > +		LOG(V4L2, Error) << "Failed to get planes";

This can only happen if the caller gives us a buf.length == 0. Let's
move this to the beginning of the method with

	if (numPlanes == 0 || numPlanes > VIDEO_MAX_PLANES) {
		LOG(V4L2, Error) << "Invalid number of planes";
		return nullptr;
	}

(but maybe we trust the kernel and this could be dropped completely ?)

and here just do

	frameBuffer = new FrameBuffer(planes);

Missing empty line.

> > +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;

And with proper use of the FileDescriptor class (with move semantics
where appropriate), you should be able to drop the close() calls and
return instead of goto out in the loop.

> > +
> > +	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

How about

 * \brief Prepare the device to import \a count buffers

> > + * \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.
> > +	*/

Wrong indentation.

> > +	if (cache_)
> > +		return 0;
> 
> I would log an error, as you do the same in allocate

And return an error too.

> > +
> > +	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

Maybe
	"Prepared to use " << count << " external buffers";

or

	"Prepared to import " << count << " buffers";

> > +
> > +	return 0;
> > +}
> > +
> >  /**
> >   * \brief Release all internally allocated buffers
> >   */

-- 
Regards,

Laurent Pinchart