[libcamera-devel] [RFC PATCH v1 06/12] libcamera: framebuffer: Add a function to check if planes are contiguous

[Laurent Pinchart]

Hi Kieran,

On Fri, Sep 03, 2021 at 09:24:02AM +0100, Kieran Bingham wrote:
> On 02/09/2021 19:07, Laurent Pinchart wrote:
> > On Thu, Sep 02, 2021 at 10:43:56AM +0100, Kieran Bingham wrote:
> >> On 02/09/2021 05:22, Laurent Pinchart wrote:
> >>> Multi-planar frame buffers can store their planes contiguously in
> >>> memory, or split them in discontiguous memory areas. Add a private
> >>> function to check in which of these two categories the frame buffer
> >>> belongs. This will be used to correctly handle the differences between
> >>> the V4L2 single and multi planar APIs.
> >>>
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >>> ---
> >>>  include/libcamera/internal/framebuffer.h |  2 ++
> >>>  src/libcamera/framebuffer.cpp            | 45 +++++++++++++++++++++++-
> >>>  2 files changed, 46 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h
> >>> index 606aed2b4782..cd33c295466e 100644
> >>> --- a/include/libcamera/internal/framebuffer.h
> >>> +++ b/include/libcamera/internal/framebuffer.h
> >>> @@ -21,9 +21,11 @@ public:
> >>>  	Private();
> >>>  
> >>>  	void setRequest(Request *request) { request_ = request; }
> >>> +	bool isContiguous() const { return isContiguous_; }
> >>>  
> >>>  private:
> >>>  	Request *request_;
> >>> +	bool isContiguous_;
> >>>  };
> >>>  
> >>>  } /* namespace libcamera */
> >>> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> >>> index 53ef89bf458f..99265b44da43 100644
> >>> --- a/src/libcamera/framebuffer.cpp
> >>> +++ b/src/libcamera/framebuffer.cpp
> >>> @@ -106,7 +106,7 @@ LOG_DEFINE_CATEGORY(Buffer)
> >>>   */
> >>>  
> >>>  FrameBuffer::Private::Private()
> >>> -	: request_(nullptr)
> >>> +	: request_(nullptr), isContiguous_(true)
> >>>  {
> >>>  }
> >>>  
> >>> @@ -120,6 +120,17 @@ FrameBuffer::Private::Private()
> >>>   * handlers, it is called by the pipeline handlers themselves.
> >>>   */
> >>>  
> >>> +/**
> >>> + * \fn FrameBuffer::Private::isContiguous()
> >>> + * \brief Check if the frame buffer stores planes contiguously in memory
> >>> + *
> >>> + * Multi-planar frame buffers can store their planes contiguously in memory, or
> >>> + * split them in discontiguous memory areas. This function checks in which of
> >>
> >> 'split them into discontiguous'
> >>
> >>> + * these two categories the frame buffer belongs.
> >>> + *
> >>> + * \return True if the planes are stored contiguously in memory, false otherwise
> >>> + */
> >>> +
> >>>  /**
> >>>   * \class FrameBuffer
> >>>   * \brief Frame buffer data and its associated dynamic metadata
> >>> @@ -199,9 +210,41 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie)
> >>>  	: Extensible(std::make_unique<Private>()), planes_(planes),
> >>>  	  cookie_(cookie)
> >>>  {
> >>> +	unsigned int offset = 0;
> >>> +
> >>>  	/* \todo Remove the assertions after sufficient testing */
> >>>  	for (const auto &plane : planes_)
> >>>  		ASSERT(plane.offset != Plane::kInvalidOffset);
> >>> +
> >>> +	bool isContiguous = true;
> >>> +	ino_t inode = 0;
> >>> +
> >>> +	for (const auto &plane : planes_) {
> >>> +		if (plane.offset != offset) {
> >>> +			isContiguous = false;
> >>> +			break;
> >>> +		}
> >>> +
> >>> +		/*
> >>> +		 * Two different dmabuf file descriptors may still refer to the
> >>> +		 * same dmabuf instance. Check this using inodes.
> >>
> >> Going back to the FileDescriptor::inode() extension, if that cached the
> >> inode (it can't change) on either first call, or construction, couldn't
> >> this whole check be simplified to:
> >>
> >> 		if (plane.fd.inode() != planes_[0].fd.inode()) {
> >> 			isContiguous = false;
> >> 			break;
> >> 		}
> > 
> > This would call fstat() every time, while the fd comparison here is
> > meant to skip the fstat() calls in case the numerical fd match. We could
> > do
> 
> Did you miss the part where I said "If we cache the inode in
> FileDescriptor"?

No I didn't :-)

> My intention was that caching there would mean we can simplify
> comparisons here, without incurring repeated fstat calls.

If the numerical fds are identical, there's no need to compare the
inodes, and we can avoid calling fstat() at all.

> > 		if (plane.fd != planes_[0].fd)
> > 
> > if we introduced an operator==(), but as explained in a reply to the
> > patch that adds inode(), I'd like to handle that later, to avoid
> > blocking this series.
> 
> Ok, I still don't see the harm in caching the inode within
> FileDescriptor(), it can't change once read the first time.

What we'd need to do in operator==() is to first compare the numerical
fds, and only compare the inodes if the fds differ. Technically that's
not an issue, but it requires deciding what the semantics of equality is
as explaining in a separate e-mail, and that's the part that I'd like to
defer to avoid delaying the offset fixes.

> But, it can be deferred:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> >>> +		 */
> >>> +		if (plane.fd.fd() != planes_[0].fd.fd()) {
> >>> +			if (!inode)
> >>> +				inode = planes_[0].fd.inode();
> >>> +			if (plane.fd.inode() != inode) {
> >>> +				isContiguous = false;
> >>> +				break;
> >>> +			}
> >>> +		}
> >>> +
> >>> +		offset += plane.length;
> >>> +	}
> >>> +
> >>> +	LOG(Buffer, Debug)
> >>> +		<< "Buffer is " << (isContiguous ? "not " : "") << "contiguous";
> >>> +
> >>> +	_d()->isContiguous_ = isContiguous;
> >>>  }
> >>>  
> >>>  /**

-- 
Regards,

Laurent Pinchart