[libcamera-devel] [PATCH 13/30] libcamera: v4l2_videodevice: Extract exportDmaBuffer() to export DMA buffer

[Laurent Pinchart]

Hello,

On Wed, Nov 27, 2019 at 03:53:52PM +0100, Jacopo Mondi wrote:
> On Wed, Nov 27, 2019 at 12:36:03AM +0100, Niklas Söderlund wrote:
> > The part in createPlane() that exports a dma buffer from a video device
> > will be used directly by the FrameBuffer interface. Break it out to a
> > own function.

"its own function" or "a separate function".

> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  src/libcamera/include/v4l2_videodevice.h |  1 +
> >  src/libcamera/v4l2_videodevice.cpp       | 41 +++++++++++++++---------
> >  2 files changed, 27 insertions(+), 15 deletions(-)
> >
> > diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> > index fdf11b3a6ec9b702..34bbff41760753bd 100644
> > --- a/src/libcamera/include/v4l2_videodevice.h
> > +++ b/src/libcamera/include/v4l2_videodevice.h
> > @@ -179,6 +179,7 @@ private:
> >  	int requestBuffers(unsigned int count);
> >  	int createPlane(BufferMemory *buffer, unsigned int index,
> >  			unsigned int plane, unsigned int length);
> > +	int exportDmaBuffer(unsigned int index, unsigned int plane);
> 
> I would have named this exportBuffer()

exportBuffer() can be interpreted to refer to either the Buffer class
(which isn't correct here), or to a v4l2_buffer, and thus multiple
dmabuf instances (which is also equally incorrect). exportDmaBuffer(),
on the other hand, is clearer, but doesn't follow any common naming
scheme. I think the right function name here, given the Dmabuf class, is
exportDmabuf(). But then one would ask the question of why this function
doesn't return a Dmabuf :-)

We clearly need to standardize on common terms. If the method can't be
modified to return a Dmabuf, I would name it exportDmabufFd().

> >
> >  	Buffer *dequeueBuffer();
> >  	void bufferAvailable(EventNotifier *notifier);
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index 644e4545a2f33b2e..cc0a1c9382a2b1ed 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -901,28 +901,19 @@ int V4L2VideoDevice::exportBuffers(BufferPool *pool)
> >  int V4L2VideoDevice::createPlane(BufferMemory *buffer, unsigned int index,
> >  				 unsigned int planeIndex, unsigned int length)
> >  {
> > -	struct v4l2_exportbuffer expbuf = {};
> > -	int ret;
> > +	int fd;
> >
> >  	LOG(V4L2, Debug)
> >  		<< "Buffer " << index
> >  		<< " plane " << planeIndex
> >  		<< ": length=" << length;
> >
> > -	expbuf.type = bufferType_;
> > -	expbuf.index = index;
> > -	expbuf.plane = planeIndex;
> > -	expbuf.flags = O_RDWR;
> > -
> > -	ret = ioctl(VIDIOC_EXPBUF, &expbuf);
> > -	if (ret < 0) {
> > -		LOG(V4L2, Error)
> > -			<< "Failed to export buffer: " << strerror(-ret);
> > -		return ret;
> > -	}
> > +	fd = exportDmaBuffer(index, planeIndex);
> > +	if (fd < 0)
> > +		return fd;
> >
> > -	buffer->planes().emplace_back(expbuf.fd, length);
> > -	::close(expbuf.fd);
> > +	buffer->planes().emplace_back(fd, length);
> > +	::close(fd);
> >
> >  	return 0;
> >  }
> > @@ -948,6 +939,26 @@ int V4L2VideoDevice::importBuffers(BufferPool *pool)
> >  	return 0;
> >  }
> >
> > +int V4L2VideoDevice::exportDmaBuffer(unsigned int index, unsigned int plane)
> > +{
> > +	struct v4l2_exportbuffer expbuf = {};
> > +	int ret;
> > +
> > +	expbuf.type = bufferType_;
> > +	expbuf.index = index;
> > +	expbuf.plane = plane;
> > +	expbuf.flags = O_RDWR;
> > +
> > +	ret = ioctl(VIDIOC_EXPBUF, &expbuf);
> > +	if (ret < 0) {
> 
> Do you need the usual ret = -errno dance to avoid errno being
> overwritten by LOG() ?
> 
> > +		LOG(V4L2, Error)
> > +			<< "Failed to export buffer: " << strerror(-ret);
> 
> Ah, you really need to assign ret before this one :)

The ioctl() call above is a member function of V4L2Device, it's not
::ioctl(). It returns the error code directly.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> > +		return ret;
> > +	}
> > +
> > +	return expbuf.fd;
> > +}
> > +
> 
> With this addressed
> 
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> >  /**
> >   * \brief Release all internally allocated buffers
> >   */

-- 
Regards,

Laurent Pinchart