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

[Laurent Pinchart]

Hi Kieran,

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

		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.

> > +		 */
> > +		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