[libcamera-devel] [PATCH 4/9] libcamera: v4l2_videodevice: Refactor allocateBuffers()

[Niklas Söderlund]

Hi Laurent,

Thanks for your patch.

On 2020-03-15 01:57:23 +0200, Laurent Pinchart wrote:
> Move the buffer creation code out of allocateBuffers() to a
> createBuffers() function. This prepare for the rework of buffer export
> and will avoid code duplication.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>

> ---
>  src/libcamera/include/v4l2_videodevice.h |  4 +-
>  src/libcamera/v4l2_videodevice.cpp       | 68 +++++++++++++-----------
>  2 files changed, 39 insertions(+), 33 deletions(-)
> 
> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> index 26f5e5917716..358d89e57414 100644
> --- a/src/libcamera/include/v4l2_videodevice.h
> +++ b/src/libcamera/include/v4l2_videodevice.h
> @@ -228,7 +228,9 @@ private:
>  	int setSelection(unsigned int target, Rectangle *rect);
>  
>  	int requestBuffers(unsigned int count, enum v4l2_memory memoryType);
> -	std::unique_ptr<FrameBuffer> createBuffer(const struct v4l2_buffer &buf);
> +	int createBuffers(unsigned int count,
> +			  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> +	std::unique_ptr<FrameBuffer> createBuffer(unsigned int index);
>  	FileDescriptor exportDmabufFd(unsigned int index, unsigned int plane);
>  
>  	void bufferAvailable(EventNotifier *notifier);
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 6911ab024fd7..d02b02ef77d6 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -1052,61 +1052,65 @@ int V4L2VideoDevice::requestBuffers(unsigned int count,
>   */
>  int V4L2VideoDevice::allocateBuffers(unsigned int count,
>  				     std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> +{
> +	int ret = createBuffers(count, buffers);
> +	if (ret < 0)
> +		return ret;
> +
> +	cache_ = new V4L2BufferCache(*buffers);
> +	memoryType_ = V4L2_MEMORY_MMAP;
> +
> +	return ret;
> +}
> +
> +int V4L2VideoDevice::createBuffers(unsigned int count,
> +				   std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>  {
>  	if (cache_) {
>  		LOG(V4L2, Error) << "Buffers already allocated";
>  		return -EINVAL;
>  	}
>  
> -	memoryType_ = V4L2_MEMORY_MMAP;
> -
>  	int ret = requestBuffers(count, V4L2_MEMORY_MMAP);
>  	if (ret < 0)
>  		return ret;
>  
>  	for (unsigned i = 0; i < count; ++i) {
> -		struct v4l2_buffer buf = {};
> -		struct v4l2_plane planes[VIDEO_MAX_PLANES] = {};
> -
> -		buf.index = i;
> -		buf.type = bufferType_;
> -		buf.memory = memoryType_;
> -		buf.length = ARRAY_SIZE(planes);
> -		buf.m.planes = planes;
> -
> -		ret = ioctl(VIDIOC_QUERYBUF, &buf);
> -		if (ret < 0) {
> -			LOG(V4L2, Error)
> -				<< "Unable to query buffer " << i << ": "
> -				<< strerror(-ret);
> -			goto err_buf;
> -		}
> -
> -		std::unique_ptr<FrameBuffer> buffer = createBuffer(buf);
> +		std::unique_ptr<FrameBuffer> buffer = createBuffer(i);
>  		if (!buffer) {
>  			LOG(V4L2, Error) << "Unable to create buffer";
> -			ret = -EINVAL;
> -			goto err_buf;
> +
> +			requestBuffers(0, V4L2_MEMORY_MMAP);
> +			buffers->clear();
> +
> +			return -EINVAL;
>  		}
>  
>  		buffers->push_back(std::move(buffer));
>  	}
>  
> -	cache_ = new V4L2BufferCache(*buffers);
> -
>  	return count;
> +}
>  
> -err_buf:
> -	requestBuffers(0, V4L2_MEMORY_MMAP);
> +std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
> +{
> +	struct v4l2_plane v4l2Planes[VIDEO_MAX_PLANES] = {};
> +	struct v4l2_buffer buf = {};
>  
> -	buffers->clear();
> +	buf.index = index;
> +	buf.type = bufferType_;
> +	buf.memory = V4L2_MEMORY_MMAP;
> +	buf.length = ARRAY_SIZE(v4l2Planes);
> +	buf.m.planes = v4l2Planes;
>  
> -	return ret;
> -}
> +	int ret = ioctl(VIDIOC_QUERYBUF, &buf);
> +	if (ret < 0) {
> +		LOG(V4L2, Error)
> +			<< "Unable to query buffer " << index << ": "
> +			<< strerror(-ret);
> +		return nullptr;
> +	}
>  
> -std::unique_ptr<FrameBuffer>
> -V4L2VideoDevice::createBuffer(const struct v4l2_buffer &buf)
> -{
>  	const bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);
>  	const unsigned int numPlanes = multiPlanar ? buf.length : 1;
>  
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund