[libcamera-devel,1/2] libcamera: v4l2_videodevice: Normalise multiPlanar configuration
diff mbox series

Message ID 20191018154201.13540-2-kieran.bingham@ideasonboard.com
State Superseded
Delegated to: Kieran Bingham
Headers show
Series
  • V4L2 Video Device MPLANE improvements
Related show

Commit Message

Kieran Bingham Oct. 18, 2019, 3:42 p.m. UTC 
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@ideasonboard.com>
---
 src/libcamera/include/v4l2_videodevice.h |  1 +
 src/libcamera/v4l2_videodevice.cpp       | 13 ++++++++-----
 2 files changed, 9 insertions(+), 5 deletions(-)

Comments

Laurent Pinchart Oct. 20, 2019, 5:23 a.m. UTC | #1 
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@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;
>  	}
Kieran Bingham Oct. 21, 2019, 10:40 a.m. UTC | #2 
Hi Laurent,

On 20/10/2019 06:23, Laurent Pinchart wrote:
> 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@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 :-)

Yes, I wasn't too worried about the single bool.


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

Ugh, I hadn't considered that someone could give us a single planar
buffer if we are configured in multiplanar.

That would indeed indicate someone had screwed up - so we shouldn't
leave it to go silent.

My main reason for this was that I later add another point of having to
split code path for mplane/splane - and I felt having a single variable
would make this clearer/simplified.



>>  	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;
>>  	}
>
Laurent Pinchart Oct. 23, 2019, 3:11 p.m. UTC | #3 
Hi Kieran,

On Mon, Oct 21, 2019 at 11:40:24AM +0100, Kieran Bingham wrote:
> On 20/10/2019 06:23, Laurent Pinchart wrote:
> > 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@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 :-)
> 
> Yes, I wasn't too worried about the single bool.
> 
> > 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 ?
> 
> Ugh, I hadn't considered that someone could give us a single planar
> buffer if we are configured in multiplanar.
> 
> That would indeed indicate someone had screwed up - so we shouldn't
> leave it to go silent.

Or we could improve the API by making the error impossible to trigger,
but that would require creating a custom structure that duplicates
v4l2_buffer without the type. I don't think that's worth it.

Another option is to ignore buf.type everywhere, using bufferType_
unconditionally, and remove all code that sets it manually.

> My main reason for this was that I later add another point of having to
> split code path for mplane/splane - and I felt having a single variable
> would make this clearer/simplified.
> 
> >>  	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;
> >>  	}

Patch
diff mbox series 

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_;
 
 	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;
 	}