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

[Kieran Bingham]

On 02/09/2021 19:07, Laurent Pinchart wrote:
> 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

Did you miss the part where I said "If we cache the inode in
FileDescriptor"?

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


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

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;
>>>  }
>>>  
>>>  /**
>