[libcamera-devel,v3,7/9] V4L2VideDevice::createBuffer() creates the same number of FrameBuffer::Planes as V4L2 format planes. Therefore, if the v4l2 format single is single-planar format, the created number of FrameBuffer::Planes is 1. It should rather cr
diff mbox series

Message ID 20210826112539.170694-8-hiroh@chromium.org
State Superseded
Headers show
Series
  • Add offset to FrameBuffer::Plane
Related show

Commit Message

Hirokazu Honda Aug. 26, 2021, 11:25 a.m. UTC
Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 include/libcamera/internal/v4l2_videodevice.h |  10 +-
 src/libcamera/v4l2_videodevice.cpp            | 141 ++++++++++++++----
 test/v4l2_videodevice/buffer_cache.cpp        |   6 +-
 3 files changed, 123 insertions(+), 34 deletions(-)

Comments

Laurent Pinchart Aug. 26, 2021, 11:32 a.m. UTC | #1
Hi Hiro,

Thank you for the patch.

I think the subject line is a bit long ;-)

I'll review this as part of the series right after "[PATCH v3 0/3]
Improve CameraBuffer implementation".

On Thu, Aug 26, 2021 at 08:25:37PM +0900, Hirokazu Honda wrote:
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  include/libcamera/internal/v4l2_videodevice.h |  10 +-
>  src/libcamera/v4l2_videodevice.cpp            | 141 ++++++++++++++----
>  test/v4l2_videodevice/buffer_cache.cpp        |   6 +-
>  3 files changed, 123 insertions(+), 34 deletions(-)
> 
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index e767ec84..bfda6726 100644
> --- a/include/libcamera/internal/v4l2_videodevice.h
> +++ b/include/libcamera/internal/v4l2_videodevice.h
> @@ -114,8 +114,9 @@ struct V4L2Capability final : v4l2_capability {
>  class V4L2BufferCache
>  {
>  public:
> -	V4L2BufferCache(unsigned int numEntries);
> -	V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers);
> +	V4L2BufferCache(unsigned int numEntries, unsigned int numPlanes);
> +	V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers,
> +			unsigned int numPlanes);
>  	~V4L2BufferCache();
>  
>  	int get(const FrameBuffer &buffer);
> @@ -126,7 +127,8 @@ private:
>  	{
>  	public:
>  		Entry();
> -		Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer);
> +		Entry(bool free, uint64_t lastUsed, unsigned int numPlanes,
> +		      const FrameBuffer &buffer);
>  
>  		bool operator==(const FrameBuffer &buffer) const;
>  
> @@ -149,6 +151,7 @@ private:
>  
>  	std::atomic<uint64_t> lastUsedCounter_;
>  	std::vector<Entry> cache_;
> +	unsigned int numPlanes_;
>  	/* \todo Expose the miss counter through an instrumentation API. */
>  	unsigned int missCounter_;
>  };
> @@ -242,6 +245,7 @@ private:
>  	FrameBuffer *dequeueBuffer();
>  
>  	V4L2Capability caps_;
> +	V4L2DeviceFormat format_;
>  
>  	enum v4l2_buf_type bufferType_;
>  	enum v4l2_memory memoryType_;
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 2ff25af2..f5f8741a 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -25,6 +25,7 @@
>  
>  #include <libcamera/file_descriptor.h>
>  
> +#include "libcamera/internal/formats.h"
>  #include "libcamera/internal/media_device.h"
>  #include "libcamera/internal/media_object.h"
>  
> @@ -157,13 +158,15 @@ LOG_DECLARE_CATEGORY(V4L2)
>  /**
>   * \brief Create an empty cache with \a numEntries entries
>   * \param[in] numEntries Number of entries to reserve in the cache
> + * \param[in] numPlanes Number of V4L2 buffer planes
>   *
> - * Create a cache with \a numEntries entries all marked as unused. The entries
> - * will be populated as the cache is used. This is typically used to implement
> - * buffer import, with buffers added to the cache as they are queued.
> + * Create a cache with \a numEntries entries all marked as unused. The entry is
> + * for \a numPlanes planes V4L2 buffer. The entries will be populated as the
> + * cache is used. This is typically used to implement buffer import, with
> + * buffers added to the cache as they are queued.
>   */
> -V4L2BufferCache::V4L2BufferCache(unsigned int numEntries)
> -	: lastUsedCounter_(1), missCounter_(0)
> +V4L2BufferCache::V4L2BufferCache(unsigned int numEntries, unsigned int numPlanes)
> +	: lastUsedCounter_(1), numPlanes_(numPlanes), missCounter_(0)
>  {
>  	cache_.resize(numEntries);
>  }
> @@ -171,17 +174,20 @@ V4L2BufferCache::V4L2BufferCache(unsigned int numEntries)
>  /**
>   * \brief Create a pre-populated cache
>   * \param[in] buffers Array of buffers to pre-populated with
> + * \param[in] numPlanes Number of V4L2 buffer planes
>   *
> - * Create a cache pre-populated with \a buffers. This is typically used to
> - * implement buffer export, with all buffers added to the cache when they are
> - * allocated.
> + * Create a cache pre-populated with \a buffers. The entry is for \a numPlanes
> + * planes V4L2 buffer. This is typically used to implement buffer export, with
> + * all buffers added to the cache when they are allocated.
>   */
> -V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers)
> -	: lastUsedCounter_(1), missCounter_(0)
> +V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers,
> +				 unsigned int numPlanes)
> +	: lastUsedCounter_(1), numPlanes_(numPlanes), missCounter_(0)
>  {
>  	for (const std::unique_ptr<FrameBuffer> &buffer : buffers)
>  		cache_.emplace_back(true,
>  				    lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),
> +				    numPlanes_,
>  				    *buffer.get());
>  }
>  
> @@ -237,6 +243,7 @@ int V4L2BufferCache::get(const FrameBuffer &buffer)
>  
>  	cache_[use] = Entry(false,
>  			    lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),
> +			    numPlanes_,
>  			    buffer);
>  
>  	return use;
> @@ -257,24 +264,53 @@ V4L2BufferCache::Entry::Entry()
>  {
>  }
>  
> -V4L2BufferCache::Entry::Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer)
> +V4L2BufferCache::Entry::Entry(bool free, uint64_t lastUsed,
> +			      unsigned int numPlanes, const FrameBuffer &buffer)
>  	: free_(free), lastUsed_(lastUsed)
>  {
> -	for (const FrameBuffer::Plane &plane : buffer.planes())
> -		planes_.emplace_back(plane);
> +	ASSERT(numPlanes <= buffer.planes().size());
> +	unsigned int length = 0;
> +	for (const FrameBuffer::Plane &plane : buffer.planes()) {
> +		ASSERT(plane.offset == length);
> +		length += plane.length;
> +
> +		if (planes_.size() < numPlanes)
> +			planes_.emplace_back(plane);
> +	}
> +
> +	if (numPlanes == 1)
> +		planes_[0].length = length;
>  }
>  
>  bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const
>  {
>  	const std::vector<FrameBuffer::Plane> &planes = buffer.planes();
>  
> -	if (planes_.size() != planes.size())
> +	if (planes_.size() != planes.size() || planes_.size() == 1)
>  		return false;
>  
> -	for (unsigned int i = 0; i < planes.size(); i++)
> -		if (planes_[i].fd != planes[i].fd.fd() ||
> -		    planes_[i].length != planes[i].length)
> +	if (planes_.size() == 1) {
> +		/*
> +		 * planes_ is V4L2 single-planar format buffer. fds of
> +		 * FrameBuffer::Plane must be identical and the sum of plane
> +		 * size is the V4L2 buffer length.
> +		 */
> +		unsigned int length = 0;
> +		for (unsigned int i = 0; i < planes.size(); i++) {
> +			if (planes_[0].fd != planes[i].fd.fd())
> +				return false;
> +			length += planes[i].length;
> +		}
> +		if (length != planes_[0].length)
>  			return false;
> +	} else {
> +		/* planes_ is V4L2 multi-planar format buffer. */
> +		for (unsigned int i = 0; i < planes.size(); i++)
> +			if (planes_[i].fd != planes[i].fd.fd() ||
> +			    planes_[i].length != planes[i].length)
> +				return false;
> +	}
> +
>  	return true;
>  }
>  
> @@ -579,6 +615,12 @@ int V4L2VideoDevice::open()
>  		<< "Opened device " << caps_.bus_info() << ": "
>  		<< caps_.driver() << ": " << caps_.card();
>  
> +	ret = getFormat(&format_);
> +	if (ret) {
> +		LOG(V4L2, Error) << "Failed to get format";
> +		return ret;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -668,6 +710,12 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)
>  		<< "Opened device " << caps_.bus_info() << ": "
>  		<< caps_.driver() << ": " << caps_.card();
>  
> +	ret = getFormat(&format_);
> +	if (ret) {
> +		LOG(V4L2, Error) << "Failed to get format";
> +		return ret;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -761,12 +809,19 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format)
>   */
>  int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format)
>  {
> +	int ret = 0;
>  	if (caps_.isMeta())
> -		return trySetFormatMeta(format, true);
> +		ret = trySetFormatMeta(format, true);
>  	else if (caps_.isMultiplanar())
> -		return trySetFormatMultiplane(format, true);
> +		ret = trySetFormatMultiplane(format, true);
>  	else
> -		return trySetFormatSingleplane(format, true);
> +		ret = trySetFormatSingleplane(format, true);
> +
> +	/* Cache the set format on success. */
> +	if (ret == 0)
> +		format_ = *format;
> +
> +	return ret;
>  }
>  
>  int V4L2VideoDevice::getFormatMeta(V4L2DeviceFormat *format)
> @@ -1152,8 +1207,13 @@ int V4L2VideoDevice::requestBuffers(unsigned int count,
>   * successful return the driver's internal buffer management is initialized in
>   * MMAP mode, and the video device is ready to accept queueBuffer() calls.
>   *
> - * The number of planes and the plane sizes for the allocation are determined
> - * by the currently active format on the device as set by setFormat().
> + * The number of planes and their offsets and sizes are determined by the
> + * currently active format on the device as set by setFormat(). They do not map
> + * to the V4L2 buffer planes, but to colour planes of the pixel format. For
> + * instance, if the active format is formats::NV12, the allocated FrameBuffer
> + * instances will have two planes, for the luma and chroma components,
> + * regardless of whether the device uses V4L2_PIX_FMT_NV12 or
> + * V4L2_PIX_FMT_NV12M.
>   *
>   * Buffers allocated with this function shall later be free with
>   * releaseBuffers(). If buffers have already been allocated with
> @@ -1171,7 +1231,7 @@ int V4L2VideoDevice::allocateBuffers(unsigned int count,
>  	if (ret < 0)
>  		return ret;
>  
> -	cache_ = new V4L2BufferCache(*buffers);
> +	cache_ = new V4L2BufferCache(*buffers, format_.planesCount);
>  	memoryType_ = V4L2_MEMORY_MMAP;
>  
>  	return ret;
> @@ -1190,8 +1250,13 @@ int V4L2VideoDevice::allocateBuffers(unsigned int count,
>   * usable with any V4L2 video device in DMABUF mode, or with other dmabuf
>   * importers.
>   *
> - * The number of planes and the plane sizes for the allocation are determined
> - * by the currently active format on the device as set by setFormat().
> + * The number of planes and their offsets and sizes are determined by the
> + * currently active format on the device as set by setFormat(). They do not map
> + * to the V4L2 buffer planes, but to colour planes of the pixel format. For
> + * instance, if the active format is formats::NV12, the allocated FrameBuffer
> + * instances will have two planes, for the luma and chroma components,
> + * regardless of whether the device uses V4L2_PIX_FMT_NV12 or
> + * V4L2_PIX_FMT_NV12M.
>   *
>   * Multiple independent sets of buffers can be allocated with multiple calls to
>   * this function. Device-specific limitations may apply regarding the minimum
> @@ -1289,12 +1354,32 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
>  		 * \todo Set the right offset once V4L2 API provides a way.
>  		 */
>  		plane.offset = 0;
> -		plane.length = multiPlanar ?
> -			buf.m.planes[nplane].length : buf.length;
> +		plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length;
>  
>  		planes.push_back(std::move(plane));
>  	}
>  
> +	const auto &info = PixelFormatInfo::info(format_.fourcc);
> +	if (info.isValid() && info.numPlanes() != numPlanes) {
> +		ASSERT(numPlanes == 1u);
> +		const size_t numColorPlanes = info.numPlanes();
> +		planes.resize(numColorPlanes);
> +		const FileDescriptor &fd = planes[0].fd;
> +		size_t offset = 0;
> +		for (size_t i = 0; i < numColorPlanes; ++i) {
> +			planes[i].fd = fd;
> +			planes[i].offset = offset;
> +
> +			/* \todo Take the V4L2 stride into account */
> +			const unsigned int vertSubSample =
> +				info.planes[i].verticalSubSampling;
> +			planes[i].length =
> +				info.stride(format_.size.width, i, 1u) *
> +				((format_.size.height + vertSubSample - 1) / vertSubSample);
> +			offset += planes[i].length;
> +		}
> +	}
> +
>  	return std::make_unique<FrameBuffer>(std::move(planes));
>  }
>  
> @@ -1352,7 +1437,7 @@ int V4L2VideoDevice::importBuffers(unsigned int count)
>  	if (ret)
>  		return ret;
>  
> -	cache_ = new V4L2BufferCache(count);
> +	cache_ = new V4L2BufferCache(count, format_.planesCount);
>  
>  	LOG(V4L2, Debug) << "Prepared to import " << count << " buffers";
>  
> diff --git a/test/v4l2_videodevice/buffer_cache.cpp b/test/v4l2_videodevice/buffer_cache.cpp
> index b3f2bec1..48f748bc 100644
> --- a/test/v4l2_videodevice/buffer_cache.cpp
> +++ b/test/v4l2_videodevice/buffer_cache.cpp
> @@ -166,7 +166,7 @@ public:
>  		 * Test cache of same size as there are buffers, the cache is
>  		 * created from a list of buffers and will be pre-populated.
>  		 */
> -		V4L2BufferCache cacheFromBuffers(buffers);
> +		V4L2BufferCache cacheFromBuffers(buffers, 1u);
>  
>  		if (testSequential(&cacheFromBuffers, buffers) != TestPass)
>  			return TestFail;
> @@ -181,7 +181,7 @@ public:
>  		 * Test cache of same size as there are buffers, the cache is
>  		 * not pre-populated.
>  		 */
> -		V4L2BufferCache cacheFromNumbers(numBuffers);
> +		V4L2BufferCache cacheFromNumbers(numBuffers, 1u);
>  
>  		if (testSequential(&cacheFromNumbers, buffers) != TestPass)
>  			return TestFail;
> @@ -196,7 +196,7 @@ public:
>  		 * Test cache half the size of number of buffers used, the cache
>  		 * is not pre-populated.
>  		 */
> -		V4L2BufferCache cacheHalf(numBuffers / 2);
> +		V4L2BufferCache cacheHalf(numBuffers / 2, 1u);
>  
>  		if (testRandom(&cacheHalf, buffers) != TestPass)
>  			return TestFail;
Hirokazu Honda Aug. 26, 2021, 11:33 a.m. UTC | #2
Hi Laurent,

On Thu, Aug 26, 2021 at 8:32 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> I think the subject line is a bit long ;-)
>

Oops... I somehow remove a blank line between the subject and message. :-)


> I'll review this as part of the series right after "[PATCH v3 0/3]
> Improve CameraBuffer implementation".
>
> On Thu, Aug 26, 2021 at 08:25:37PM +0900, Hirokazu Honda wrote:
> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > ---
> >  include/libcamera/internal/v4l2_videodevice.h |  10 +-
> >  src/libcamera/v4l2_videodevice.cpp            | 141 ++++++++++++++----
> >  test/v4l2_videodevice/buffer_cache.cpp        |   6 +-
> >  3 files changed, 123 insertions(+), 34 deletions(-)
> >
> > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> > index e767ec84..bfda6726 100644
> > --- a/include/libcamera/internal/v4l2_videodevice.h
> > +++ b/include/libcamera/internal/v4l2_videodevice.h
> > @@ -114,8 +114,9 @@ struct V4L2Capability final : v4l2_capability {
> >  class V4L2BufferCache
> >  {
> >  public:
> > -     V4L2BufferCache(unsigned int numEntries);
> > -     V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers);
> > +     V4L2BufferCache(unsigned int numEntries, unsigned int numPlanes);
> > +     V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers,
> > +                     unsigned int numPlanes);
> >       ~V4L2BufferCache();
> >
> >       int get(const FrameBuffer &buffer);
> > @@ -126,7 +127,8 @@ private:
> >       {
> >       public:
> >               Entry();
> > -             Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer);
> > +             Entry(bool free, uint64_t lastUsed, unsigned int numPlanes,
> > +                   const FrameBuffer &buffer);
> >
> >               bool operator==(const FrameBuffer &buffer) const;
> >
> > @@ -149,6 +151,7 @@ private:
> >
> >       std::atomic<uint64_t> lastUsedCounter_;
> >       std::vector<Entry> cache_;
> > +     unsigned int numPlanes_;
> >       /* \todo Expose the miss counter through an instrumentation API. */
> >       unsigned int missCounter_;
> >  };
> > @@ -242,6 +245,7 @@ private:
> >       FrameBuffer *dequeueBuffer();
> >
> >       V4L2Capability caps_;
> > +     V4L2DeviceFormat format_;
> >
> >       enum v4l2_buf_type bufferType_;
> >       enum v4l2_memory memoryType_;
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index 2ff25af2..f5f8741a 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -25,6 +25,7 @@
> >
> >  #include <libcamera/file_descriptor.h>
> >
> > +#include "libcamera/internal/formats.h"
> >  #include "libcamera/internal/media_device.h"
> >  #include "libcamera/internal/media_object.h"
> >
> > @@ -157,13 +158,15 @@ LOG_DECLARE_CATEGORY(V4L2)
> >  /**
> >   * \brief Create an empty cache with \a numEntries entries
> >   * \param[in] numEntries Number of entries to reserve in the cache
> > + * \param[in] numPlanes Number of V4L2 buffer planes
> >   *
> > - * Create a cache with \a numEntries entries all marked as unused. The entries
> > - * will be populated as the cache is used. This is typically used to implement
> > - * buffer import, with buffers added to the cache as they are queued.
> > + * Create a cache with \a numEntries entries all marked as unused. The entry is
> > + * for \a numPlanes planes V4L2 buffer. The entries will be populated as the
> > + * cache is used. This is typically used to implement buffer import, with
> > + * buffers added to the cache as they are queued.
> >   */
> > -V4L2BufferCache::V4L2BufferCache(unsigned int numEntries)
> > -     : lastUsedCounter_(1), missCounter_(0)
> > +V4L2BufferCache::V4L2BufferCache(unsigned int numEntries, unsigned int numPlanes)
> > +     : lastUsedCounter_(1), numPlanes_(numPlanes), missCounter_(0)
> >  {
> >       cache_.resize(numEntries);
> >  }
> > @@ -171,17 +174,20 @@ V4L2BufferCache::V4L2BufferCache(unsigned int numEntries)
> >  /**
> >   * \brief Create a pre-populated cache
> >   * \param[in] buffers Array of buffers to pre-populated with
> > + * \param[in] numPlanes Number of V4L2 buffer planes
> >   *
> > - * Create a cache pre-populated with \a buffers. This is typically used to
> > - * implement buffer export, with all buffers added to the cache when they are
> > - * allocated.
> > + * Create a cache pre-populated with \a buffers. The entry is for \a numPlanes
> > + * planes V4L2 buffer. This is typically used to implement buffer export, with
> > + * all buffers added to the cache when they are allocated.
> >   */
> > -V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers)
> > -     : lastUsedCounter_(1), missCounter_(0)
> > +V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers,
> > +                              unsigned int numPlanes)
> > +     : lastUsedCounter_(1), numPlanes_(numPlanes), missCounter_(0)
> >  {
> >       for (const std::unique_ptr<FrameBuffer> &buffer : buffers)
> >               cache_.emplace_back(true,
> >                                   lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),
> > +                                 numPlanes_,
> >                                   *buffer.get());
> >  }
> >
> > @@ -237,6 +243,7 @@ int V4L2BufferCache::get(const FrameBuffer &buffer)
> >
> >       cache_[use] = Entry(false,
> >                           lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),
> > +                         numPlanes_,
> >                           buffer);
> >
> >       return use;
> > @@ -257,24 +264,53 @@ V4L2BufferCache::Entry::Entry()
> >  {
> >  }
> >
> > -V4L2BufferCache::Entry::Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer)
> > +V4L2BufferCache::Entry::Entry(bool free, uint64_t lastUsed,
> > +                           unsigned int numPlanes, const FrameBuffer &buffer)
> >       : free_(free), lastUsed_(lastUsed)
> >  {
> > -     for (const FrameBuffer::Plane &plane : buffer.planes())
> > -             planes_.emplace_back(plane);
> > +     ASSERT(numPlanes <= buffer.planes().size());
> > +     unsigned int length = 0;
> > +     for (const FrameBuffer::Plane &plane : buffer.planes()) {
> > +             ASSERT(plane.offset == length);
> > +             length += plane.length;
> > +
> > +             if (planes_.size() < numPlanes)
> > +                     planes_.emplace_back(plane);
> > +     }
> > +
> > +     if (numPlanes == 1)
> > +             planes_[0].length = length;
> >  }
> >
> >  bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const
> >  {
> >       const std::vector<FrameBuffer::Plane> &planes = buffer.planes();
> >
> > -     if (planes_.size() != planes.size())
> > +     if (planes_.size() != planes.size() || planes_.size() == 1)
> >               return false;
> >
> > -     for (unsigned int i = 0; i < planes.size(); i++)
> > -             if (planes_[i].fd != planes[i].fd.fd() ||
> > -                 planes_[i].length != planes[i].length)
> > +     if (planes_.size() == 1) {
> > +             /*
> > +              * planes_ is V4L2 single-planar format buffer. fds of
> > +              * FrameBuffer::Plane must be identical and the sum of plane
> > +              * size is the V4L2 buffer length.
> > +              */
> > +             unsigned int length = 0;
> > +             for (unsigned int i = 0; i < planes.size(); i++) {
> > +                     if (planes_[0].fd != planes[i].fd.fd())
> > +                             return false;
> > +                     length += planes[i].length;
> > +             }
> > +             if (length != planes_[0].length)
> >                       return false;
> > +     } else {
> > +             /* planes_ is V4L2 multi-planar format buffer. */
> > +             for (unsigned int i = 0; i < planes.size(); i++)
> > +                     if (planes_[i].fd != planes[i].fd.fd() ||
> > +                         planes_[i].length != planes[i].length)
> > +                             return false;
> > +     }
> > +
> >       return true;
> >  }
> >
> > @@ -579,6 +615,12 @@ int V4L2VideoDevice::open()
> >               << "Opened device " << caps_.bus_info() << ": "
> >               << caps_.driver() << ": " << caps_.card();
> >
> > +     ret = getFormat(&format_);
> > +     if (ret) {
> > +             LOG(V4L2, Error) << "Failed to get format";
> > +             return ret;
> > +     }
> > +
> >       return 0;
> >  }
> >
> > @@ -668,6 +710,12 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)
> >               << "Opened device " << caps_.bus_info() << ": "
> >               << caps_.driver() << ": " << caps_.card();
> >
> > +     ret = getFormat(&format_);
> > +     if (ret) {
> > +             LOG(V4L2, Error) << "Failed to get format";
> > +             return ret;
> > +     }
> > +
> >       return 0;
> >  }
> >
> > @@ -761,12 +809,19 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format)
> >   */
> >  int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format)
> >  {
> > +     int ret = 0;
> >       if (caps_.isMeta())
> > -             return trySetFormatMeta(format, true);
> > +             ret = trySetFormatMeta(format, true);
> >       else if (caps_.isMultiplanar())
> > -             return trySetFormatMultiplane(format, true);
> > +             ret = trySetFormatMultiplane(format, true);
> >       else
> > -             return trySetFormatSingleplane(format, true);
> > +             ret = trySetFormatSingleplane(format, true);
> > +
> > +     /* Cache the set format on success. */
> > +     if (ret == 0)
> > +             format_ = *format;
> > +
> > +     return ret;
> >  }
> >
> >  int V4L2VideoDevice::getFormatMeta(V4L2DeviceFormat *format)
> > @@ -1152,8 +1207,13 @@ int V4L2VideoDevice::requestBuffers(unsigned int count,
> >   * successful return the driver's internal buffer management is initialized in
> >   * MMAP mode, and the video device is ready to accept queueBuffer() calls.
> >   *
> > - * The number of planes and the plane sizes for the allocation are determined
> > - * by the currently active format on the device as set by setFormat().
> > + * The number of planes and their offsets and sizes are determined by the
> > + * currently active format on the device as set by setFormat(). They do not map
> > + * to the V4L2 buffer planes, but to colour planes of the pixel format. For
> > + * instance, if the active format is formats::NV12, the allocated FrameBuffer
> > + * instances will have two planes, for the luma and chroma components,
> > + * regardless of whether the device uses V4L2_PIX_FMT_NV12 or
> > + * V4L2_PIX_FMT_NV12M.
> >   *
> >   * Buffers allocated with this function shall later be free with
> >   * releaseBuffers(). If buffers have already been allocated with
> > @@ -1171,7 +1231,7 @@ int V4L2VideoDevice::allocateBuffers(unsigned int count,
> >       if (ret < 0)
> >               return ret;
> >
> > -     cache_ = new V4L2BufferCache(*buffers);
> > +     cache_ = new V4L2BufferCache(*buffers, format_.planesCount);
> >       memoryType_ = V4L2_MEMORY_MMAP;
> >
> >       return ret;
> > @@ -1190,8 +1250,13 @@ int V4L2VideoDevice::allocateBuffers(unsigned int count,
> >   * usable with any V4L2 video device in DMABUF mode, or with other dmabuf
> >   * importers.
> >   *
> > - * The number of planes and the plane sizes for the allocation are determined
> > - * by the currently active format on the device as set by setFormat().
> > + * The number of planes and their offsets and sizes are determined by the
> > + * currently active format on the device as set by setFormat(). They do not map
> > + * to the V4L2 buffer planes, but to colour planes of the pixel format. For
> > + * instance, if the active format is formats::NV12, the allocated FrameBuffer
> > + * instances will have two planes, for the luma and chroma components,
> > + * regardless of whether the device uses V4L2_PIX_FMT_NV12 or
> > + * V4L2_PIX_FMT_NV12M.
> >   *
> >   * Multiple independent sets of buffers can be allocated with multiple calls to
> >   * this function. Device-specific limitations may apply regarding the minimum
> > @@ -1289,12 +1354,32 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
> >                * \todo Set the right offset once V4L2 API provides a way.
> >                */
> >               plane.offset = 0;
> > -             plane.length = multiPlanar ?
> > -                     buf.m.planes[nplane].length : buf.length;
> > +             plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length;
> >
> >               planes.push_back(std::move(plane));
> >       }
> >
> > +     const auto &info = PixelFormatInfo::info(format_.fourcc);
> > +     if (info.isValid() && info.numPlanes() != numPlanes) {
> > +             ASSERT(numPlanes == 1u);
> > +             const size_t numColorPlanes = info.numPlanes();
> > +             planes.resize(numColorPlanes);
> > +             const FileDescriptor &fd = planes[0].fd;
> > +             size_t offset = 0;
> > +             for (size_t i = 0; i < numColorPlanes; ++i) {
> > +                     planes[i].fd = fd;
> > +                     planes[i].offset = offset;
> > +
> > +                     /* \todo Take the V4L2 stride into account */
> > +                     const unsigned int vertSubSample =
> > +                             info.planes[i].verticalSubSampling;
> > +                     planes[i].length =
> > +                             info.stride(format_.size.width, i, 1u) *
> > +                             ((format_.size.height + vertSubSample - 1) / vertSubSample);
> > +                     offset += planes[i].length;
> > +             }
> > +     }
> > +
> >       return std::make_unique<FrameBuffer>(std::move(planes));
> >  }
> >
> > @@ -1352,7 +1437,7 @@ int V4L2VideoDevice::importBuffers(unsigned int count)
> >       if (ret)
> >               return ret;
> >
> > -     cache_ = new V4L2BufferCache(count);
> > +     cache_ = new V4L2BufferCache(count, format_.planesCount);
> >
> >       LOG(V4L2, Debug) << "Prepared to import " << count << " buffers";
> >
> > diff --git a/test/v4l2_videodevice/buffer_cache.cpp b/test/v4l2_videodevice/buffer_cache.cpp
> > index b3f2bec1..48f748bc 100644
> > --- a/test/v4l2_videodevice/buffer_cache.cpp
> > +++ b/test/v4l2_videodevice/buffer_cache.cpp
> > @@ -166,7 +166,7 @@ public:
> >                * Test cache of same size as there are buffers, the cache is
> >                * created from a list of buffers and will be pre-populated.
> >                */
> > -             V4L2BufferCache cacheFromBuffers(buffers);
> > +             V4L2BufferCache cacheFromBuffers(buffers, 1u);
> >
> >               if (testSequential(&cacheFromBuffers, buffers) != TestPass)
> >                       return TestFail;
> > @@ -181,7 +181,7 @@ public:
> >                * Test cache of same size as there are buffers, the cache is
> >                * not pre-populated.
> >                */
> > -             V4L2BufferCache cacheFromNumbers(numBuffers);
> > +             V4L2BufferCache cacheFromNumbers(numBuffers, 1u);
> >
> >               if (testSequential(&cacheFromNumbers, buffers) != TestPass)
> >                       return TestFail;
> > @@ -196,7 +196,7 @@ public:
> >                * Test cache half the size of number of buffers used, the cache
> >                * is not pre-populated.
> >                */
> > -             V4L2BufferCache cacheHalf(numBuffers / 2);
> > +             V4L2BufferCache cacheHalf(numBuffers / 2, 1u);
> >
> >               if (testRandom(&cacheHalf, buffers) != TestPass)
> >                       return TestFail;
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart Aug. 26, 2021, 1:03 p.m. UTC | #3
Hi Hiro,

Thank you for the patch.

On Thu, Aug 26, 2021 at 08:25:37PM +0900, Hirokazu Honda wrote:
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  include/libcamera/internal/v4l2_videodevice.h |  10 +-
>  src/libcamera/v4l2_videodevice.cpp            | 141 ++++++++++++++----
>  test/v4l2_videodevice/buffer_cache.cpp        |   6 +-
>  3 files changed, 123 insertions(+), 34 deletions(-)
> 
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index e767ec84..bfda6726 100644
> --- a/include/libcamera/internal/v4l2_videodevice.h
> +++ b/include/libcamera/internal/v4l2_videodevice.h
> @@ -114,8 +114,9 @@ struct V4L2Capability final : v4l2_capability {
>  class V4L2BufferCache
>  {
>  public:
> -	V4L2BufferCache(unsigned int numEntries);
> -	V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers);
> +	V4L2BufferCache(unsigned int numEntries, unsigned int numPlanes);
> +	V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers,
> +			unsigned int numPlanes);
>  	~V4L2BufferCache();
>  
>  	int get(const FrameBuffer &buffer);
> @@ -126,7 +127,8 @@ private:
>  	{
>  	public:
>  		Entry();
> -		Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer);
> +		Entry(bool free, uint64_t lastUsed, unsigned int numPlanes,
> +		      const FrameBuffer &buffer);
>  
>  		bool operator==(const FrameBuffer &buffer) const;
>  
> @@ -149,6 +151,7 @@ private:
>  
>  	std::atomic<uint64_t> lastUsedCounter_;
>  	std::vector<Entry> cache_;
> +	unsigned int numPlanes_;
>  	/* \todo Expose the miss counter through an instrumentation API. */
>  	unsigned int missCounter_;
>  };
> @@ -242,6 +245,7 @@ private:
>  	FrameBuffer *dequeueBuffer();
>  
>  	V4L2Capability caps_;
> +	V4L2DeviceFormat format_;
>  
>  	enum v4l2_buf_type bufferType_;
>  	enum v4l2_memory memoryType_;
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 2ff25af2..f5f8741a 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -25,6 +25,7 @@
>  
>  #include <libcamera/file_descriptor.h>
>  
> +#include "libcamera/internal/formats.h"
>  #include "libcamera/internal/media_device.h"
>  #include "libcamera/internal/media_object.h"
>  
> @@ -157,13 +158,15 @@ LOG_DECLARE_CATEGORY(V4L2)
>  /**
>   * \brief Create an empty cache with \a numEntries entries
>   * \param[in] numEntries Number of entries to reserve in the cache
> + * \param[in] numPlanes Number of V4L2 buffer planes
>   *
> - * Create a cache with \a numEntries entries all marked as unused. The entries
> - * will be populated as the cache is used. This is typically used to implement
> - * buffer import, with buffers added to the cache as they are queued.
> + * Create a cache with \a numEntries entries all marked as unused. The entry is
> + * for \a numPlanes planes V4L2 buffer. The entries will be populated as the
> + * cache is used. This is typically used to implement buffer import, with
> + * buffers added to the cache as they are queued.
>   */
> -V4L2BufferCache::V4L2BufferCache(unsigned int numEntries)
> -	: lastUsedCounter_(1), missCounter_(0)
> +V4L2BufferCache::V4L2BufferCache(unsigned int numEntries, unsigned int numPlanes)
> +	: lastUsedCounter_(1), numPlanes_(numPlanes), missCounter_(0)
>  {
>  	cache_.resize(numEntries);
>  }
> @@ -171,17 +174,20 @@ V4L2BufferCache::V4L2BufferCache(unsigned int numEntries)
>  /**
>   * \brief Create a pre-populated cache
>   * \param[in] buffers Array of buffers to pre-populated with
> + * \param[in] numPlanes Number of V4L2 buffer planes
>   *
> - * Create a cache pre-populated with \a buffers. This is typically used to
> - * implement buffer export, with all buffers added to the cache when they are
> - * allocated.
> + * Create a cache pre-populated with \a buffers. The entry is for \a numPlanes
> + * planes V4L2 buffer. This is typically used to implement buffer export, with
> + * all buffers added to the cache when they are allocated.
>   */
> -V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers)
> -	: lastUsedCounter_(1), missCounter_(0)
> +V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers,
> +				 unsigned int numPlanes)
> +	: lastUsedCounter_(1), numPlanes_(numPlanes), missCounter_(0)
>  {
>  	for (const std::unique_ptr<FrameBuffer> &buffer : buffers)
>  		cache_.emplace_back(true,
>  				    lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),
> +				    numPlanes_,
>  				    *buffer.get());
>  }
>  
> @@ -237,6 +243,7 @@ int V4L2BufferCache::get(const FrameBuffer &buffer)
>  
>  	cache_[use] = Entry(false,
>  			    lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),
> +			    numPlanes_,
>  			    buffer);
>  
>  	return use;
> @@ -257,24 +264,53 @@ V4L2BufferCache::Entry::Entry()
>  {
>  }
>  
> -V4L2BufferCache::Entry::Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer)
> +V4L2BufferCache::Entry::Entry(bool free, uint64_t lastUsed,
> +			      unsigned int numPlanes, const FrameBuffer &buffer)
>  	: free_(free), lastUsed_(lastUsed)
>  {
> -	for (const FrameBuffer::Plane &plane : buffer.planes())
> -		planes_.emplace_back(plane);
> +	ASSERT(numPlanes <= buffer.planes().size());
> +	unsigned int length = 0;
> +	for (const FrameBuffer::Plane &plane : buffer.planes()) {
> +		ASSERT(plane.offset == length);
> +		length += plane.length;
> +
> +		if (planes_.size() < numPlanes)
> +			planes_.emplace_back(plane);
> +	}
> +
> +	if (numPlanes == 1)
> +		planes_[0].length = length;
>  }
>  
>  bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const
>  {
>  	const std::vector<FrameBuffer::Plane> &planes = buffer.planes();
>  
> -	if (planes_.size() != planes.size())
> +	if (planes_.size() != planes.size() || planes_.size() == 1)
>  		return false;
>  
> -	for (unsigned int i = 0; i < planes.size(); i++)
> -		if (planes_[i].fd != planes[i].fd.fd() ||
> -		    planes_[i].length != planes[i].length)
> +	if (planes_.size() == 1) {
> +		/*
> +		 * planes_ is V4L2 single-planar format buffer. fds of
> +		 * FrameBuffer::Plane must be identical and the sum of plane
> +		 * size is the V4L2 buffer length.
> +		 */
> +		unsigned int length = 0;
> +		for (unsigned int i = 0; i < planes.size(); i++) {
> +			if (planes_[0].fd != planes[i].fd.fd())
> +				return false;
> +			length += planes[i].length;
> +		}
> +		if (length != planes_[0].length)
>  			return false;
> +	} else {
> +		/* planes_ is V4L2 multi-planar format buffer. */
> +		for (unsigned int i = 0; i < planes.size(); i++)
> +			if (planes_[i].fd != planes[i].fd.fd() ||
> +			    planes_[i].length != planes[i].length)
> +				return false;
> +	}
> +
>  	return true;

Is all of this actually needed ? What's the issue if we store, in the
cache entries, the colour planes from FrameBuffer instance of the V4L2
buffer planes ?

>  }
>  
> @@ -579,6 +615,12 @@ int V4L2VideoDevice::open()
>  		<< "Opened device " << caps_.bus_info() << ": "
>  		<< caps_.driver() << ": " << caps_.card();
>  
> +	ret = getFormat(&format_);
> +	if (ret) {
> +		LOG(V4L2, Error) << "Failed to get format";
> +		return ret;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -668,6 +710,12 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)
>  		<< "Opened device " << caps_.bus_info() << ": "
>  		<< caps_.driver() << ": " << caps_.card();
>  
> +	ret = getFormat(&format_);
> +	if (ret) {
> +		LOG(V4L2, Error) << "Failed to get format";
> +		return ret;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -761,12 +809,19 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format)
>   */
>  int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format)
>  {
> +	int ret = 0;
>  	if (caps_.isMeta())
> -		return trySetFormatMeta(format, true);
> +		ret = trySetFormatMeta(format, true);
>  	else if (caps_.isMultiplanar())
> -		return trySetFormatMultiplane(format, true);
> +		ret = trySetFormatMultiplane(format, true);
>  	else
> -		return trySetFormatSingleplane(format, true);
> +		ret = trySetFormatSingleplane(format, true);
> +
> +	/* Cache the set format on success. */
> +	if (ret == 0)
> +		format_ = *format;
> +
> +	return ret;

How about

	if (ret)
		return ret;

	format_ = *format;
	return 0;

We usually return early on error, and handle the normal case in the top
scope of the function.

>  }
>  
>  int V4L2VideoDevice::getFormatMeta(V4L2DeviceFormat *format)
> @@ -1152,8 +1207,13 @@ int V4L2VideoDevice::requestBuffers(unsigned int count,
>   * successful return the driver's internal buffer management is initialized in
>   * MMAP mode, and the video device is ready to accept queueBuffer() calls.
>   *
> - * The number of planes and the plane sizes for the allocation are determined
> - * by the currently active format on the device as set by setFormat().
> + * The number of planes and their offsets and sizes are determined by the
> + * currently active format on the device as set by setFormat(). They do not map
> + * to the V4L2 buffer planes, but to colour planes of the pixel format. For
> + * instance, if the active format is formats::NV12, the allocated FrameBuffer
> + * instances will have two planes, for the luma and chroma components,
> + * regardless of whether the device uses V4L2_PIX_FMT_NV12 or
> + * V4L2_PIX_FMT_NV12M.
>   *
>   * Buffers allocated with this function shall later be free with
>   * releaseBuffers(). If buffers have already been allocated with
> @@ -1171,7 +1231,7 @@ int V4L2VideoDevice::allocateBuffers(unsigned int count,
>  	if (ret < 0)
>  		return ret;
>  
> -	cache_ = new V4L2BufferCache(*buffers);
> +	cache_ = new V4L2BufferCache(*buffers, format_.planesCount);
>  	memoryType_ = V4L2_MEMORY_MMAP;
>  
>  	return ret;
> @@ -1190,8 +1250,13 @@ int V4L2VideoDevice::allocateBuffers(unsigned int count,
>   * usable with any V4L2 video device in DMABUF mode, or with other dmabuf
>   * importers.
>   *
> - * The number of planes and the plane sizes for the allocation are determined
> - * by the currently active format on the device as set by setFormat().
> + * The number of planes and their offsets and sizes are determined by the
> + * currently active format on the device as set by setFormat(). They do not map
> + * to the V4L2 buffer planes, but to colour planes of the pixel format. For
> + * instance, if the active format is formats::NV12, the allocated FrameBuffer
> + * instances will have two planes, for the luma and chroma components,
> + * regardless of whether the device uses V4L2_PIX_FMT_NV12 or
> + * V4L2_PIX_FMT_NV12M.
>   *
>   * Multiple independent sets of buffers can be allocated with multiple calls to
>   * this function. Device-specific limitations may apply regarding the minimum
> @@ -1289,12 +1354,32 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
>  		 * \todo Set the right offset once V4L2 API provides a way.
>  		 */
>  		plane.offset = 0;
> -		plane.length = multiPlanar ?
> -			buf.m.planes[nplane].length : buf.length;
> +		plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length;
>  
>  		planes.push_back(std::move(plane));
>  	}
>  
> +	const auto &info = PixelFormatInfo::info(format_.fourcc);
> +	if (info.isValid() && info.numPlanes() != numPlanes) {
> +		ASSERT(numPlanes == 1u);
> +		const size_t numColorPlanes = info.numPlanes();
> +		planes.resize(numColorPlanes);
> +		const FileDescriptor &fd = planes[0].fd;
> +		size_t offset = 0;
> +		for (size_t i = 0; i < numColorPlanes; ++i) {
> +			planes[i].fd = fd;
> +			planes[i].offset = offset;
> +
> +			/* \todo Take the V4L2 stride into account */
> +			const unsigned int vertSubSample =
> +				info.planes[i].verticalSubSampling;
> +			planes[i].length =
> +				info.stride(format_.size.width, i, 1u) *
> +				((format_.size.height + vertSubSample - 1) / vertSubSample);
> +			offset += planes[i].length;
> +		}
> +	}
> +
>  	return std::make_unique<FrameBuffer>(std::move(planes));
>  }
>  
> @@ -1352,7 +1437,7 @@ int V4L2VideoDevice::importBuffers(unsigned int count)
>  	if (ret)
>  		return ret;
>  
> -	cache_ = new V4L2BufferCache(count);
> +	cache_ = new V4L2BufferCache(count, format_.planesCount);
>  
>  	LOG(V4L2, Debug) << "Prepared to import " << count << " buffers";
>  
> diff --git a/test/v4l2_videodevice/buffer_cache.cpp b/test/v4l2_videodevice/buffer_cache.cpp
> index b3f2bec1..48f748bc 100644
> --- a/test/v4l2_videodevice/buffer_cache.cpp
> +++ b/test/v4l2_videodevice/buffer_cache.cpp
> @@ -166,7 +166,7 @@ public:
>  		 * Test cache of same size as there are buffers, the cache is
>  		 * created from a list of buffers and will be pre-populated.
>  		 */
> -		V4L2BufferCache cacheFromBuffers(buffers);
> +		V4L2BufferCache cacheFromBuffers(buffers, 1u);
>  
>  		if (testSequential(&cacheFromBuffers, buffers) != TestPass)
>  			return TestFail;
> @@ -181,7 +181,7 @@ public:
>  		 * Test cache of same size as there are buffers, the cache is
>  		 * not pre-populated.
>  		 */
> -		V4L2BufferCache cacheFromNumbers(numBuffers);
> +		V4L2BufferCache cacheFromNumbers(numBuffers, 1u);
>  
>  		if (testSequential(&cacheFromNumbers, buffers) != TestPass)
>  			return TestFail;
> @@ -196,7 +196,7 @@ public:
>  		 * Test cache half the size of number of buffers used, the cache
>  		 * is not pre-populated.
>  		 */
> -		V4L2BufferCache cacheHalf(numBuffers / 2);
> +		V4L2BufferCache cacheHalf(numBuffers / 2, 1u);
>  
>  		if (testRandom(&cacheHalf, buffers) != TestPass)
>  			return TestFail;

I'm afraid the test now fails :-S Could you have a look at that ?
Hirokazu Honda Aug. 27, 2021, 6:42 a.m. UTC | #4
Hi Laurent,


On Thu, Aug 26, 2021 at 10:03 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> On Thu, Aug 26, 2021 at 08:25:37PM +0900, Hirokazu Honda wrote:
> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > ---
> >  include/libcamera/internal/v4l2_videodevice.h |  10 +-
> >  src/libcamera/v4l2_videodevice.cpp            | 141 ++++++++++++++----
> >  test/v4l2_videodevice/buffer_cache.cpp        |   6 +-
> >  3 files changed, 123 insertions(+), 34 deletions(-)
> >
> > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> > index e767ec84..bfda6726 100644
> > --- a/include/libcamera/internal/v4l2_videodevice.h
> > +++ b/include/libcamera/internal/v4l2_videodevice.h
> > @@ -114,8 +114,9 @@ struct V4L2Capability final : v4l2_capability {
> >  class V4L2BufferCache
> >  {
> >  public:
> > -     V4L2BufferCache(unsigned int numEntries);
> > -     V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers);
> > +     V4L2BufferCache(unsigned int numEntries, unsigned int numPlanes);
> > +     V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers,
> > +                     unsigned int numPlanes);
> >       ~V4L2BufferCache();
> >
> >       int get(const FrameBuffer &buffer);
> > @@ -126,7 +127,8 @@ private:
> >       {
> >       public:
> >               Entry();
> > -             Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer);
> > +             Entry(bool free, uint64_t lastUsed, unsigned int numPlanes,
> > +                   const FrameBuffer &buffer);
> >
> >               bool operator==(const FrameBuffer &buffer) const;
> >
> > @@ -149,6 +151,7 @@ private:
> >
> >       std::atomic<uint64_t> lastUsedCounter_;
> >       std::vector<Entry> cache_;
> > +     unsigned int numPlanes_;
> >       /* \todo Expose the miss counter through an instrumentation API. */
> >       unsigned int missCounter_;
> >  };
> > @@ -242,6 +245,7 @@ private:
> >       FrameBuffer *dequeueBuffer();
> >
> >       V4L2Capability caps_;
> > +     V4L2DeviceFormat format_;
> >
> >       enum v4l2_buf_type bufferType_;
> >       enum v4l2_memory memoryType_;
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index 2ff25af2..f5f8741a 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -25,6 +25,7 @@
> >
> >  #include <libcamera/file_descriptor.h>
> >
> > +#include "libcamera/internal/formats.h"
> >  #include "libcamera/internal/media_device.h"
> >  #include "libcamera/internal/media_object.h"
> >
> > @@ -157,13 +158,15 @@ LOG_DECLARE_CATEGORY(V4L2)
> >  /**
> >   * \brief Create an empty cache with \a numEntries entries
> >   * \param[in] numEntries Number of entries to reserve in the cache
> > + * \param[in] numPlanes Number of V4L2 buffer planes
> >   *
> > - * Create a cache with \a numEntries entries all marked as unused. The entries
> > - * will be populated as the cache is used. This is typically used to implement
> > - * buffer import, with buffers added to the cache as they are queued.
> > + * Create a cache with \a numEntries entries all marked as unused. The entry is
> > + * for \a numPlanes planes V4L2 buffer. The entries will be populated as the
> > + * cache is used. This is typically used to implement buffer import, with
> > + * buffers added to the cache as they are queued.
> >   */
> > -V4L2BufferCache::V4L2BufferCache(unsigned int numEntries)
> > -     : lastUsedCounter_(1), missCounter_(0)
> > +V4L2BufferCache::V4L2BufferCache(unsigned int numEntries, unsigned int numPlanes)
> > +     : lastUsedCounter_(1), numPlanes_(numPlanes), missCounter_(0)
> >  {
> >       cache_.resize(numEntries);
> >  }
> > @@ -171,17 +174,20 @@ V4L2BufferCache::V4L2BufferCache(unsigned int numEntries)
> >  /**
> >   * \brief Create a pre-populated cache
> >   * \param[in] buffers Array of buffers to pre-populated with
> > + * \param[in] numPlanes Number of V4L2 buffer planes
> >   *
> > - * Create a cache pre-populated with \a buffers. This is typically used to
> > - * implement buffer export, with all buffers added to the cache when they are
> > - * allocated.
> > + * Create a cache pre-populated with \a buffers. The entry is for \a numPlanes
> > + * planes V4L2 buffer. This is typically used to implement buffer export, with
> > + * all buffers added to the cache when they are allocated.
> >   */
> > -V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers)
> > -     : lastUsedCounter_(1), missCounter_(0)
> > +V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers,
> > +                              unsigned int numPlanes)
> > +     : lastUsedCounter_(1), numPlanes_(numPlanes), missCounter_(0)
> >  {
> >       for (const std::unique_ptr<FrameBuffer> &buffer : buffers)
> >               cache_.emplace_back(true,
> >                                   lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),
> > +                                 numPlanes_,
> >                                   *buffer.get());
> >  }
> >
> > @@ -237,6 +243,7 @@ int V4L2BufferCache::get(const FrameBuffer &buffer)
> >
> >       cache_[use] = Entry(false,
> >                           lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),
> > +                         numPlanes_,
> >                           buffer);
> >
> >       return use;
> > @@ -257,24 +264,53 @@ V4L2BufferCache::Entry::Entry()
> >  {
> >  }
> >
> > -V4L2BufferCache::Entry::Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer)
> > +V4L2BufferCache::Entry::Entry(bool free, uint64_t lastUsed,
> > +                           unsigned int numPlanes, const FrameBuffer &buffer)
> >       : free_(free), lastUsed_(lastUsed)
> >  {
> > -     for (const FrameBuffer::Plane &plane : buffer.planes())
> > -             planes_.emplace_back(plane);
> > +     ASSERT(numPlanes <= buffer.planes().size());
> > +     unsigned int length = 0;
> > +     for (const FrameBuffer::Plane &plane : buffer.planes()) {
> > +             ASSERT(plane.offset == length);
> > +             length += plane.length;
> > +
> > +             if (planes_.size() < numPlanes)
> > +                     planes_.emplace_back(plane);
> > +     }
> > +
> > +     if (numPlanes == 1)
> > +             planes_[0].length = length;
> >  }
> >
> >  bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const
> >  {
> >       const std::vector<FrameBuffer::Plane> &planes = buffer.planes();
> >
> > -     if (planes_.size() != planes.size())
> > +     if (planes_.size() != planes.size() || planes_.size() == 1)
> >               return false;
> >
> > -     for (unsigned int i = 0; i < planes.size(); i++)
> > -             if (planes_[i].fd != planes[i].fd.fd() ||
> > -                 planes_[i].length != planes[i].length)
> > +     if (planes_.size() == 1) {
> > +             /*
> > +              * planes_ is V4L2 single-planar format buffer. fds of
> > +              * FrameBuffer::Plane must be identical and the sum of plane
> > +              * size is the V4L2 buffer length.
> > +              */
> > +             unsigned int length = 0;
> > +             for (unsigned int i = 0; i < planes.size(); i++) {
> > +                     if (planes_[0].fd != planes[i].fd.fd())
> > +                             return false;
> > +                     length += planes[i].length;
> > +             }
> > +             if (length != planes_[0].length)
> >                       return false;
> > +     } else {
> > +             /* planes_ is V4L2 multi-planar format buffer. */
> > +             for (unsigned int i = 0; i < planes.size(); i++)
> > +                     if (planes_[i].fd != planes[i].fd.fd() ||
> > +                         planes_[i].length != planes[i].length)
> > +                             return false;
> > +     }
> > +
> >       return true;
>
> Is all of this actually needed ? What's the issue if we store, in the
> cache entries, the colour planes from FrameBuffer instance of the V4L2
> buffer planes ?
>

Hmm, you're right. As far as we compare the V4L2 buffer planes, the
conversion is unnecessary.
I reverted this part of the change.

> >  }
> >
> > @@ -579,6 +615,12 @@ int V4L2VideoDevice::open()
> >               << "Opened device " << caps_.bus_info() << ": "
> >               << caps_.driver() << ": " << caps_.card();
> >
> > +     ret = getFormat(&format_);
> > +     if (ret) {
> > +             LOG(V4L2, Error) << "Failed to get format";
> > +             return ret;
> > +     }
> > +
> >       return 0;
> >  }
> >
> > @@ -668,6 +710,12 @@ int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)
> >               << "Opened device " << caps_.bus_info() << ": "
> >               << caps_.driver() << ": " << caps_.card();
> >
> > +     ret = getFormat(&format_);
> > +     if (ret) {
> > +             LOG(V4L2, Error) << "Failed to get format";
> > +             return ret;
> > +     }
> > +
> >       return 0;
> >  }
> >
> > @@ -761,12 +809,19 @@ int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format)
> >   */
> >  int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format)
> >  {
> > +     int ret = 0;
> >       if (caps_.isMeta())
> > -             return trySetFormatMeta(format, true);
> > +             ret = trySetFormatMeta(format, true);
> >       else if (caps_.isMultiplanar())
> > -             return trySetFormatMultiplane(format, true);
> > +             ret = trySetFormatMultiplane(format, true);
> >       else
> > -             return trySetFormatSingleplane(format, true);
> > +             ret = trySetFormatSingleplane(format, true);
> > +
> > +     /* Cache the set format on success. */
> > +     if (ret == 0)
> > +             format_ = *format;
> > +
> > +     return ret;
>
> How about
>
>         if (ret)
>                 return ret;
>
>         format_ = *format;
>         return 0;
>
> We usually return early on error, and handle the normal case in the top
> scope of the function.
>
> >  }
> >
> >  int V4L2VideoDevice::getFormatMeta(V4L2DeviceFormat *format)
> > @@ -1152,8 +1207,13 @@ int V4L2VideoDevice::requestBuffers(unsigned int count,
> >   * successful return the driver's internal buffer management is initialized in
> >   * MMAP mode, and the video device is ready to accept queueBuffer() calls.
> >   *
> > - * The number of planes and the plane sizes for the allocation are determined
> > - * by the currently active format on the device as set by setFormat().
> > + * The number of planes and their offsets and sizes are determined by the
> > + * currently active format on the device as set by setFormat(). They do not map
> > + * to the V4L2 buffer planes, but to colour planes of the pixel format. For
> > + * instance, if the active format is formats::NV12, the allocated FrameBuffer
> > + * instances will have two planes, for the luma and chroma components,
> > + * regardless of whether the device uses V4L2_PIX_FMT_NV12 or
> > + * V4L2_PIX_FMT_NV12M.
> >   *
> >   * Buffers allocated with this function shall later be free with
> >   * releaseBuffers(). If buffers have already been allocated with
> > @@ -1171,7 +1231,7 @@ int V4L2VideoDevice::allocateBuffers(unsigned int count,
> >       if (ret < 0)
> >               return ret;
> >
> > -     cache_ = new V4L2BufferCache(*buffers);
> > +     cache_ = new V4L2BufferCache(*buffers, format_.planesCount);
> >       memoryType_ = V4L2_MEMORY_MMAP;
> >
> >       return ret;
> > @@ -1190,8 +1250,13 @@ int V4L2VideoDevice::allocateBuffers(unsigned int count,
> >   * usable with any V4L2 video device in DMABUF mode, or with other dmabuf
> >   * importers.
> >   *
> > - * The number of planes and the plane sizes for the allocation are determined
> > - * by the currently active format on the device as set by setFormat().
> > + * The number of planes and their offsets and sizes are determined by the
> > + * currently active format on the device as set by setFormat(). They do not map
> > + * to the V4L2 buffer planes, but to colour planes of the pixel format. For
> > + * instance, if the active format is formats::NV12, the allocated FrameBuffer
> > + * instances will have two planes, for the luma and chroma components,
> > + * regardless of whether the device uses V4L2_PIX_FMT_NV12 or
> > + * V4L2_PIX_FMT_NV12M.
> >   *
> >   * Multiple independent sets of buffers can be allocated with multiple calls to
> >   * this function. Device-specific limitations may apply regarding the minimum
> > @@ -1289,12 +1354,32 @@ std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
> >                * \todo Set the right offset once V4L2 API provides a way.
> >                */
> >               plane.offset = 0;
> > -             plane.length = multiPlanar ?
> > -                     buf.m.planes[nplane].length : buf.length;
> > +             plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length;
> >
> >               planes.push_back(std::move(plane));
> >       }
> >
> > +     const auto &info = PixelFormatInfo::info(format_.fourcc);
> > +     if (info.isValid() && info.numPlanes() != numPlanes) {
> > +             ASSERT(numPlanes == 1u);
> > +             const size_t numColorPlanes = info.numPlanes();
> > +             planes.resize(numColorPlanes);
> > +             const FileDescriptor &fd = planes[0].fd;
> > +             size_t offset = 0;
> > +             for (size_t i = 0; i < numColorPlanes; ++i) {
> > +                     planes[i].fd = fd;
> > +                     planes[i].offset = offset;
> > +
> > +                     /* \todo Take the V4L2 stride into account */
> > +                     const unsigned int vertSubSample =
> > +                             info.planes[i].verticalSubSampling;
> > +                     planes[i].length =
> > +                             info.stride(format_.size.width, i, 1u) *
> > +                             ((format_.size.height + vertSubSample - 1) / vertSubSample);
> > +                     offset += planes[i].length;
> > +             }
> > +     }
> > +
> >       return std::make_unique<FrameBuffer>(std::move(planes));
> >  }
> >
> > @@ -1352,7 +1437,7 @@ int V4L2VideoDevice::importBuffers(unsigned int count)
> >       if (ret)
> >               return ret;
> >
> > -     cache_ = new V4L2BufferCache(count);
> > +     cache_ = new V4L2BufferCache(count, format_.planesCount);
> >
> >       LOG(V4L2, Debug) << "Prepared to import " << count << " buffers";
> >
> > diff --git a/test/v4l2_videodevice/buffer_cache.cpp b/test/v4l2_videodevice/buffer_cache.cpp
> > index b3f2bec1..48f748bc 100644
> > --- a/test/v4l2_videodevice/buffer_cache.cpp
> > +++ b/test/v4l2_videodevice/buffer_cache.cpp
> > @@ -166,7 +166,7 @@ public:
> >                * Test cache of same size as there are buffers, the cache is
> >                * created from a list of buffers and will be pre-populated.
> >                */
> > -             V4L2BufferCache cacheFromBuffers(buffers);
> > +             V4L2BufferCache cacheFromBuffers(buffers, 1u);
> >
> >               if (testSequential(&cacheFromBuffers, buffers) != TestPass)
> >                       return TestFail;
> > @@ -181,7 +181,7 @@ public:
> >                * Test cache of same size as there are buffers, the cache is
> >                * not pre-populated.
> >                */
> > -             V4L2BufferCache cacheFromNumbers(numBuffers);
> > +             V4L2BufferCache cacheFromNumbers(numBuffers, 1u);
> >
> >               if (testSequential(&cacheFromNumbers, buffers) != TestPass)
> >                       return TestFail;
> > @@ -196,7 +196,7 @@ public:
> >                * Test cache half the size of number of buffers used, the cache
> >                * is not pre-populated.
> >                */
> > -             V4L2BufferCache cacheHalf(numBuffers / 2);
> > +             V4L2BufferCache cacheHalf(numBuffers / 2, 1u);
> >
> >               if (testRandom(&cacheHalf, buffers) != TestPass)
> >                       return TestFail;
>
> I'm afraid the test now fails :-S Could you have a look at that ?
>

Sorry, I don't have a vivid environment. I hope the issue is gone by
reverting V4L2BufferCache change.

-Hiro
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series

diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
index e767ec84..bfda6726 100644
--- a/include/libcamera/internal/v4l2_videodevice.h
+++ b/include/libcamera/internal/v4l2_videodevice.h
@@ -114,8 +114,9 @@  struct V4L2Capability final : v4l2_capability {
 class V4L2BufferCache
 {
 public:
-	V4L2BufferCache(unsigned int numEntries);
-	V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers);
+	V4L2BufferCache(unsigned int numEntries, unsigned int numPlanes);
+	V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers,
+			unsigned int numPlanes);
 	~V4L2BufferCache();
 
 	int get(const FrameBuffer &buffer);
@@ -126,7 +127,8 @@  private:
 	{
 	public:
 		Entry();
-		Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer);
+		Entry(bool free, uint64_t lastUsed, unsigned int numPlanes,
+		      const FrameBuffer &buffer);
 
 		bool operator==(const FrameBuffer &buffer) const;
 
@@ -149,6 +151,7 @@  private:
 
 	std::atomic<uint64_t> lastUsedCounter_;
 	std::vector<Entry> cache_;
+	unsigned int numPlanes_;
 	/* \todo Expose the miss counter through an instrumentation API. */
 	unsigned int missCounter_;
 };
@@ -242,6 +245,7 @@  private:
 	FrameBuffer *dequeueBuffer();
 
 	V4L2Capability caps_;
+	V4L2DeviceFormat format_;
 
 	enum v4l2_buf_type bufferType_;
 	enum v4l2_memory memoryType_;
diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
index 2ff25af2..f5f8741a 100644
--- a/src/libcamera/v4l2_videodevice.cpp
+++ b/src/libcamera/v4l2_videodevice.cpp
@@ -25,6 +25,7 @@ 
 
 #include <libcamera/file_descriptor.h>
 
+#include "libcamera/internal/formats.h"
 #include "libcamera/internal/media_device.h"
 #include "libcamera/internal/media_object.h"
 
@@ -157,13 +158,15 @@  LOG_DECLARE_CATEGORY(V4L2)
 /**
  * \brief Create an empty cache with \a numEntries entries
  * \param[in] numEntries Number of entries to reserve in the cache
+ * \param[in] numPlanes Number of V4L2 buffer planes
  *
- * Create a cache with \a numEntries entries all marked as unused. The entries
- * will be populated as the cache is used. This is typically used to implement
- * buffer import, with buffers added to the cache as they are queued.
+ * Create a cache with \a numEntries entries all marked as unused. The entry is
+ * for \a numPlanes planes V4L2 buffer. The entries will be populated as the
+ * cache is used. This is typically used to implement buffer import, with
+ * buffers added to the cache as they are queued.
  */
-V4L2BufferCache::V4L2BufferCache(unsigned int numEntries)
-	: lastUsedCounter_(1), missCounter_(0)
+V4L2BufferCache::V4L2BufferCache(unsigned int numEntries, unsigned int numPlanes)
+	: lastUsedCounter_(1), numPlanes_(numPlanes), missCounter_(0)
 {
 	cache_.resize(numEntries);
 }
@@ -171,17 +174,20 @@  V4L2BufferCache::V4L2BufferCache(unsigned int numEntries)
 /**
  * \brief Create a pre-populated cache
  * \param[in] buffers Array of buffers to pre-populated with
+ * \param[in] numPlanes Number of V4L2 buffer planes
  *
- * Create a cache pre-populated with \a buffers. This is typically used to
- * implement buffer export, with all buffers added to the cache when they are
- * allocated.
+ * Create a cache pre-populated with \a buffers. The entry is for \a numPlanes
+ * planes V4L2 buffer. This is typically used to implement buffer export, with
+ * all buffers added to the cache when they are allocated.
  */
-V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers)
-	: lastUsedCounter_(1), missCounter_(0)
+V4L2BufferCache::V4L2BufferCache(const std::vector<std::unique_ptr<FrameBuffer>> &buffers,
+				 unsigned int numPlanes)
+	: lastUsedCounter_(1), numPlanes_(numPlanes), missCounter_(0)
 {
 	for (const std::unique_ptr<FrameBuffer> &buffer : buffers)
 		cache_.emplace_back(true,
 				    lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),
+				    numPlanes_,
 				    *buffer.get());
 }
 
@@ -237,6 +243,7 @@  int V4L2BufferCache::get(const FrameBuffer &buffer)
 
 	cache_[use] = Entry(false,
 			    lastUsedCounter_.fetch_add(1, std::memory_order_acq_rel),
+			    numPlanes_,
 			    buffer);
 
 	return use;
@@ -257,24 +264,53 @@  V4L2BufferCache::Entry::Entry()
 {
 }
 
-V4L2BufferCache::Entry::Entry(bool free, uint64_t lastUsed, const FrameBuffer &buffer)
+V4L2BufferCache::Entry::Entry(bool free, uint64_t lastUsed,
+			      unsigned int numPlanes, const FrameBuffer &buffer)
 	: free_(free), lastUsed_(lastUsed)
 {
-	for (const FrameBuffer::Plane &plane : buffer.planes())
-		planes_.emplace_back(plane);
+	ASSERT(numPlanes <= buffer.planes().size());
+	unsigned int length = 0;
+	for (const FrameBuffer::Plane &plane : buffer.planes()) {
+		ASSERT(plane.offset == length);
+		length += plane.length;
+
+		if (planes_.size() < numPlanes)
+			planes_.emplace_back(plane);
+	}
+
+	if (numPlanes == 1)
+		planes_[0].length = length;
 }
 
 bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const
 {
 	const std::vector<FrameBuffer::Plane> &planes = buffer.planes();
 
-	if (planes_.size() != planes.size())
+	if (planes_.size() != planes.size() || planes_.size() == 1)
 		return false;
 
-	for (unsigned int i = 0; i < planes.size(); i++)
-		if (planes_[i].fd != planes[i].fd.fd() ||
-		    planes_[i].length != planes[i].length)
+	if (planes_.size() == 1) {
+		/*
+		 * planes_ is V4L2 single-planar format buffer. fds of
+		 * FrameBuffer::Plane must be identical and the sum of plane
+		 * size is the V4L2 buffer length.
+		 */
+		unsigned int length = 0;
+		for (unsigned int i = 0; i < planes.size(); i++) {
+			if (planes_[0].fd != planes[i].fd.fd())
+				return false;
+			length += planes[i].length;
+		}
+		if (length != planes_[0].length)
 			return false;
+	} else {
+		/* planes_ is V4L2 multi-planar format buffer. */
+		for (unsigned int i = 0; i < planes.size(); i++)
+			if (planes_[i].fd != planes[i].fd.fd() ||
+			    planes_[i].length != planes[i].length)
+				return false;
+	}
+
 	return true;
 }
 
@@ -579,6 +615,12 @@  int V4L2VideoDevice::open()
 		<< "Opened device " << caps_.bus_info() << ": "
 		<< caps_.driver() << ": " << caps_.card();
 
+	ret = getFormat(&format_);
+	if (ret) {
+		LOG(V4L2, Error) << "Failed to get format";
+		return ret;
+	}
+
 	return 0;
 }
 
@@ -668,6 +710,12 @@  int V4L2VideoDevice::open(int handle, enum v4l2_buf_type type)
 		<< "Opened device " << caps_.bus_info() << ": "
 		<< caps_.driver() << ": " << caps_.card();
 
+	ret = getFormat(&format_);
+	if (ret) {
+		LOG(V4L2, Error) << "Failed to get format";
+		return ret;
+	}
+
 	return 0;
 }
 
@@ -761,12 +809,19 @@  int V4L2VideoDevice::tryFormat(V4L2DeviceFormat *format)
  */
 int V4L2VideoDevice::setFormat(V4L2DeviceFormat *format)
 {
+	int ret = 0;
 	if (caps_.isMeta())
-		return trySetFormatMeta(format, true);
+		ret = trySetFormatMeta(format, true);
 	else if (caps_.isMultiplanar())
-		return trySetFormatMultiplane(format, true);
+		ret = trySetFormatMultiplane(format, true);
 	else
-		return trySetFormatSingleplane(format, true);
+		ret = trySetFormatSingleplane(format, true);
+
+	/* Cache the set format on success. */
+	if (ret == 0)
+		format_ = *format;
+
+	return ret;
 }
 
 int V4L2VideoDevice::getFormatMeta(V4L2DeviceFormat *format)
@@ -1152,8 +1207,13 @@  int V4L2VideoDevice::requestBuffers(unsigned int count,
  * successful return the driver's internal buffer management is initialized in
  * MMAP mode, and the video device is ready to accept queueBuffer() calls.
  *
- * The number of planes and the plane sizes for the allocation are determined
- * by the currently active format on the device as set by setFormat().
+ * The number of planes and their offsets and sizes are determined by the
+ * currently active format on the device as set by setFormat(). They do not map
+ * to the V4L2 buffer planes, but to colour planes of the pixel format. For
+ * instance, if the active format is formats::NV12, the allocated FrameBuffer
+ * instances will have two planes, for the luma and chroma components,
+ * regardless of whether the device uses V4L2_PIX_FMT_NV12 or
+ * V4L2_PIX_FMT_NV12M.
  *
  * Buffers allocated with this function shall later be free with
  * releaseBuffers(). If buffers have already been allocated with
@@ -1171,7 +1231,7 @@  int V4L2VideoDevice::allocateBuffers(unsigned int count,
 	if (ret < 0)
 		return ret;
 
-	cache_ = new V4L2BufferCache(*buffers);
+	cache_ = new V4L2BufferCache(*buffers, format_.planesCount);
 	memoryType_ = V4L2_MEMORY_MMAP;
 
 	return ret;
@@ -1190,8 +1250,13 @@  int V4L2VideoDevice::allocateBuffers(unsigned int count,
  * usable with any V4L2 video device in DMABUF mode, or with other dmabuf
  * importers.
  *
- * The number of planes and the plane sizes for the allocation are determined
- * by the currently active format on the device as set by setFormat().
+ * The number of planes and their offsets and sizes are determined by the
+ * currently active format on the device as set by setFormat(). They do not map
+ * to the V4L2 buffer planes, but to colour planes of the pixel format. For
+ * instance, if the active format is formats::NV12, the allocated FrameBuffer
+ * instances will have two planes, for the luma and chroma components,
+ * regardless of whether the device uses V4L2_PIX_FMT_NV12 or
+ * V4L2_PIX_FMT_NV12M.
  *
  * Multiple independent sets of buffers can be allocated with multiple calls to
  * this function. Device-specific limitations may apply regarding the minimum
@@ -1289,12 +1354,32 @@  std::unique_ptr<FrameBuffer> V4L2VideoDevice::createBuffer(unsigned int index)
 		 * \todo Set the right offset once V4L2 API provides a way.
 		 */
 		plane.offset = 0;
-		plane.length = multiPlanar ?
-			buf.m.planes[nplane].length : buf.length;
+		plane.length = multiPlanar ? buf.m.planes[nplane].length : buf.length;
 
 		planes.push_back(std::move(plane));
 	}
 
+	const auto &info = PixelFormatInfo::info(format_.fourcc);
+	if (info.isValid() && info.numPlanes() != numPlanes) {
+		ASSERT(numPlanes == 1u);
+		const size_t numColorPlanes = info.numPlanes();
+		planes.resize(numColorPlanes);
+		const FileDescriptor &fd = planes[0].fd;
+		size_t offset = 0;
+		for (size_t i = 0; i < numColorPlanes; ++i) {
+			planes[i].fd = fd;
+			planes[i].offset = offset;
+
+			/* \todo Take the V4L2 stride into account */
+			const unsigned int vertSubSample =
+				info.planes[i].verticalSubSampling;
+			planes[i].length =
+				info.stride(format_.size.width, i, 1u) *
+				((format_.size.height + vertSubSample - 1) / vertSubSample);
+			offset += planes[i].length;
+		}
+	}
+
 	return std::make_unique<FrameBuffer>(std::move(planes));
 }
 
@@ -1352,7 +1437,7 @@  int V4L2VideoDevice::importBuffers(unsigned int count)
 	if (ret)
 		return ret;
 
-	cache_ = new V4L2BufferCache(count);
+	cache_ = new V4L2BufferCache(count, format_.planesCount);
 
 	LOG(V4L2, Debug) << "Prepared to import " << count << " buffers";
 
diff --git a/test/v4l2_videodevice/buffer_cache.cpp b/test/v4l2_videodevice/buffer_cache.cpp
index b3f2bec1..48f748bc 100644
--- a/test/v4l2_videodevice/buffer_cache.cpp
+++ b/test/v4l2_videodevice/buffer_cache.cpp
@@ -166,7 +166,7 @@  public:
 		 * Test cache of same size as there are buffers, the cache is
 		 * created from a list of buffers and will be pre-populated.
 		 */
-		V4L2BufferCache cacheFromBuffers(buffers);
+		V4L2BufferCache cacheFromBuffers(buffers, 1u);
 
 		if (testSequential(&cacheFromBuffers, buffers) != TestPass)
 			return TestFail;
@@ -181,7 +181,7 @@  public:
 		 * Test cache of same size as there are buffers, the cache is
 		 * not pre-populated.
 		 */
-		V4L2BufferCache cacheFromNumbers(numBuffers);
+		V4L2BufferCache cacheFromNumbers(numBuffers, 1u);
 
 		if (testSequential(&cacheFromNumbers, buffers) != TestPass)
 			return TestFail;
@@ -196,7 +196,7 @@  public:
 		 * Test cache half the size of number of buffers used, the cache
 		 * is not pre-populated.
 		 */
-		V4L2BufferCache cacheHalf(numBuffers / 2);
+		V4L2BufferCache cacheHalf(numBuffers / 2, 1u);
 
 		if (testRandom(&cacheHalf, buffers) != TestPass)
 			return TestFail;