[libcamera-devel] [PATCH 1/2] libcamera: v4l2_videodevice: Normalise multiPlanar configuration

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Fri, Oct 18, 2019 at 04:42:00PM +0100, Kieran Bingham wrote:
> The use of either single planar or multi planar API is constant
> throughout the usage of a device, based upon the buffer type of the
> stream, and once determined it does not change.
> 
> Rather than interrogate the bufferType_ each time, set a device flag at
> open() time as to whether we are using planar or multi-planar buffer
> APIs.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  src/libcamera/include/v4l2_videodevice.h |  1 +
>  src/libcamera/v4l2_videodevice.cpp       | 13 ++++++++-----
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> index 4b8cf9394eb9..60fca01670c6 100644
> --- a/src/libcamera/include/v4l2_videodevice.h
> +++ b/src/libcamera/include/v4l2_videodevice.h
> @@ -182,6 +182,7 @@ private:
>  
>  	enum v4l2_buf_type bufferType_;
>  	enum v4l2_memory memoryType_;
> +	bool multiPlanar_;

I'm not really opposed to this, but does this change really add value to
offset for the larger memory usage ? That's of course a bit of a
rhetorical question as one bool isn't much :-)

If we decide to move forward, how should the method below react when
passing a buf.type that doesn't match multiPlanar_ ? Currently the
kernel will return an error, is it a good idea to ignore the issue
silently ?

>  
>  	BufferPool *bufferPool_;
>  	std::map<unsigned int, Buffer *> queuedBuffers_;
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 208ab54199b1..37f61b90ac46 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -350,6 +350,8 @@ int V4L2VideoDevice::open()
>  		return -EINVAL;
>  	}
>  
> +	multiPlanar_ = V4L2_TYPE_IS_MULTIPLANAR(bufferType_);
> +
>  	fdEvent_->activated.connect(this, &V4L2VideoDevice::bufferAvailable);
>  	fdEvent_->setEnabled(false);
>  
> @@ -436,6 +438,8 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)
>  		return -EINVAL;
>  	}
>  
> +	multiPlanar_ = V4L2_TYPE_IS_MULTIPLANAR(bufferType_);
> +
>  	fdEvent_->activated.connect(this, &V4L2VideoDevice::bufferAvailable);
>  	fdEvent_->setEnabled(false);
>  
> @@ -870,7 +874,7 @@ int V4L2VideoDevice::exportBuffers(BufferPool *pool)
>  			break;
>  		}
>  
> -		if (V4L2_TYPE_IS_MULTIPLANAR(buf.type)) {
> +		if (multiPlanar_) {
>  			for (unsigned int p = 0; p < buf.length; ++p) {
>  				ret = createPlane(&buffer, i, p,
>  						  buf.m.planes[p].length);
> @@ -993,12 +997,11 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)
>  	buf.memory = memoryType_;
>  	buf.field = V4L2_FIELD_NONE;
>  
> -	bool multiPlanar = V4L2_TYPE_IS_MULTIPLANAR(buf.type);
>  	BufferMemory *mem = &bufferPool_->buffers()[buf.index];
>  	const std::vector<Plane> &planes = mem->planes();
>  
>  	if (buf.memory == V4L2_MEMORY_DMABUF) {
> -		if (multiPlanar) {
> +		if (multiPlanar_) {
>  			for (unsigned int p = 0; p < planes.size(); ++p)
>  				v4l2Planes[p].m.fd = planes[p].dmabuf();
>  		} else {
> @@ -1006,7 +1009,7 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)
>  		}
>  	}
>  
> -	if (multiPlanar) {
> +	if (multiPlanar_) {
>  		buf.length = planes.size();
>  		buf.m.planes = v4l2Planes;
>  	}
> @@ -1099,7 +1102,7 @@ Buffer *V4L2VideoDevice::dequeueBuffer()
>  	buf.type = bufferType_;
>  	buf.memory = memoryType_;
>  
> -	if (V4L2_TYPE_IS_MULTIPLANAR(buf.type)) {
> +	if (multiPlanar_) {
>  		buf.length = VIDEO_MAX_PLANES;
>  		buf.m.planes = planes;
>  	}

-- 
Regards,

Laurent Pinchart