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

2361 diff mbox series

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.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/libcamera/include/v4l2_videodevice.h |  1 +
 src/libcamera/v4l2_videodevice.cpp       | 41 +++++++++++++++---------
 2 files changed, 27 insertions(+), 15 deletions(-)

Hi Niklas,

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.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@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()

>
>  	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 :)
> +		return ret;
> +	}
> +
> +	return expbuf.fd;
> +}
> +

With this addressed

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

>  /**
>   * \brief Release all internally allocated buffers
>   */
> --
> 2.24.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

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@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@ideasonboard.com>

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