[libcamera-devel] [PATCH 1/2] libcamera: v4l2_videodevice: Normalise multiPlanar configuration
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Oct 21 12:40:24 CEST 2019
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 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 :-)
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;
>> }
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list