[libcamera-devel] [RFC PATCH 1/8] libcamera: buffer: Create a MappedFrameBuffer

[Kieran Bingham]

Hi Laurent,

On 29/07/2020 15:18, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Wed, Jul 29, 2020 at 03:09:59PM +0100, Kieran Bingham wrote:
>> On 24/07/2020 17:58, Laurent Pinchart wrote:
>>> On Mon, Jul 20, 2020 at 11:42:25PM +0100, Kieran Bingham wrote:
>>>> Provide a MappedFrameBuffer helper class which will map
>>>> all of the Planes within a FrameBuffer and provide CPU addressable
>>>> pointers for those planes.
>>>>
>>>> Mappings are removed upon destruction.
>>>>
>>>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>>> ---
>>>>
>>>> This introduces an automatically mapping/unmapping MappedFrameBuffer
>>>> object, with a move constructor (essential to prevent un-desirable
>>>> unmapping).
>>>>
>>>>  include/libcamera/buffer.h | 23 ++++++++++++++++++++
>>>
>>> Should this be an internal helper ? The main foreseen flow is for
>>> applications to create frame buffers from dmabuf fds they recently
>>> outside of libcamera, so I would expect them to handle memory mapping.
>>> I'd thus rather start with an internal helper, which we could graduate
>>> to a public API later if needed/desired.
>>
>> I saw it as beneficial to be in the public-api, because FrameBuffer is
>> in the public API.
>>
>> If an application has created a FrameBuffer of their own, by the
>> definition of it, it can be used to construct a MappedFrameBuffer.
>>
>> We've already had a support-request in the past where someone had not
>> mapped the FrameBuffer because it wasn't obvious, so I thought providing
>> common helpers to map the Common FrameBuffer object was useful.
>>
>> I can move it to private API all the same though.
>>
>> As you say, it could be made public later.
>>
>> This implies that we would create
>>
>> 	include/libcamera/internal/buffer.h
>> as well as
>> 	include/libcamera/buffer.h
>>
>> Where both would be implemented in:
>>
>> 	src/libcamera/buffer.cpp
>>
>> Is that acceptable? or should this become:
>>
>> 	include/libcamera/internal/mapped_buffer.h
>> 	src/libcamera/mapped_buffer.cpp
> 
> Both options are fine with me.

I'll take the easy option, otherwise I'll convince myself buffer.cpp
will need a whole bunch of refactoring and renaming.


> 
>>>>  src/libcamera/buffer.cpp   | 43 ++++++++++++++++++++++++++++++++++++++
>>>>  2 files changed, 66 insertions(+)
>>>>
>>>> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
>>>> index 6bb2e4f8558f..881d736da2db 100644
>>>> --- a/include/libcamera/buffer.h
>>>> +++ b/include/libcamera/buffer.h
>>>> @@ -71,6 +71,29 @@ private:
>>>>  	unsigned int cookie_;
>>>>  };
>>>>  
>>>> +class MappedFrameBuffer {
>>>> +public:
>>>> +	MappedFrameBuffer(const FrameBuffer *buffer, int flags);
>>>> +	~MappedFrameBuffer();
>>>> +
>>>> +	/* Move constructor only, copying is not permitted. */
>>>
>>> We usually don't put comments in the header files. This should go to the
>>> \class documentation block below.
>>>
>>>> +	MappedFrameBuffer(MappedFrameBuffer &&rhs);
>>>
>>> Do you actually need this ? If it's only for the purpose of preventing
>>> copy-construction, you can simply write
>>
>> It was to self implement the Move constructor (and disable the copy
>> constructors yes).
>>
>> I think I had originally already put in the update to the rhs, which is
>> why I implemented it rather than using the default, but yet that change
>> didn't seem to make it into this patch ... :S
>>
>>> 	MappedFrameBuffer(const MappedFrameBuffer &other) = delete;
>>> 	MappedFrameBuffer operator=(const MappedFrameBuffer &other) = delete;
>>>
>>> The C++ rules governing implicitly-defined and defaulted copy and move
>>> constructors and assignment operators are not the easiest to follow.
>>> It's usually best to be explicit. If you need move semantics,
>>>
>>> 	MappedFrameBuffer(const MappedFrameBuffer &other) = delete;
>>> 	MappedFrameBuffer operator=(const MappedFrameBuffer &other) = delete;
>>>
>>> 	MappedFrameBuffer(MappedFrameBuffer &&other);
>>> 	MappedFrameBuffer operator=(MappedFrameBuffer &&other);
>>>
>>> (either neither or both of the move constructor and the move assignment
>>> operator should be defined) otherwise you can leave the latter two out.
>>>
>>>> +
>>>> +	struct MappedPlane {
>>>> +		void *address;
>>>> +		size_t length;
>>>> +	};
>>>
>>> Would it make sense to use Span<uint8_t> to replace MappedPlane ?
>>
>> Aha, I like that.
>>
>> I like the type naming though, so should it be
>>
>> using MappedPlane = Span<uint8_t>;
>>
>> Or would you prefer to use the Span directly?
> 
> I don't mind much. Aliases require looking up what they correspond to,
> but it can also clarify the code as the name better matches the purpose.
> Up to you.
> 
>>>> +
>>>> +	bool isValid() const { return valid_; }
>>>> +	int error() const { return error_; }
>>>> +	const std::vector<MappedPlane> &maps() const { return maps_; }
>>>> +
>>>> +private:
>>>> +	int error_;
>>>> +	bool valid_;
>>>> +	std::vector<MappedPlane> maps_;
>>>> +};
>>>> +
>>>>  } /* namespace libcamera */
>>>>  
>>>>  #endif /* __LIBCAMERA_BUFFER_H__ */
>>>> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp
>>>> index 1a1d4bac7aed..28b43d937f57 100644
>>>> --- a/src/libcamera/buffer.cpp
>>>> +++ b/src/libcamera/buffer.cpp
>>>> @@ -290,4 +290,47 @@ int FrameBuffer::copyFrom(const FrameBuffer *src)
>>>>  	return 0;
>>>>  }
>>>
>>> Missing
>>>
>>> /**
>>>  * \class MappedFrameBuffer
>>>  * ...
>>>  */
>>
>> Sure.
>>
>>>> +/**
>>>> + * \brief Map all planes of a FrameBuffer
>>>
>>> As this is a constructor, I think it should be documented as "Construct
>>> ..." to follow the usual pattern.
>>>
>>>> + * \param[in] src Buffer to be mapped
>>>
>>> s/src/buffer/
>>>
>>>> + * \param[in] flags Protection flags to apply to map
>>>> + *
>>>> + * Construct an object to map a frame buffer for CPU access.
>>>> + * The flags are passed directly to mmap and should be either PROT_READ,
>>>> + * PROT_WRITE, or a bitwise-or combination of both.
>>>> + */
>>>> +MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)
>>>> +	: error_(0)
>>>> +{
>>>> +	maps_.reserve(buffer->planes().size());
>>>> +
>>>> +	for (const FrameBuffer::Plane &plane : buffer->planes()) {
>>>> +		void *address = mmap(nullptr, plane.length, flags,
>>>> +				     MAP_SHARED, plane.fd.fd(), 0);
>>>> +
>>>> +		if (address == MAP_FAILED) {
>>>> +			error_ = -errno;
>>>> +			LOG(Buffer, Error) << "Failed to mmap plane";
>>>> +			continue;
>>>
>>> Shouldn't you break ?
>>
>> It will still fail the valid_ conditional below, but yes - I think break
>> would be better.
>>
>>>> +		}
>>>> +
>>>> +		maps_.push_back({address, plane.length});
>>>> +	}
>>>> +
>>>> +	valid_ = buffer->planes().size() == maps_.size();
>>>> +}
>>>> +
>>>> +MappedFrameBuffer::~MappedFrameBuffer()
>>>> +{
>>>> +	for (MappedPlane map : maps_)
>>>
>>> 	for (MappedPlane &map : maps_)
>>
>> Good spot.
>>
>>>> +		munmap(map.address, map.length);
>>>> +}
>>>> +
>>>> +MappedFrameBuffer::MappedFrameBuffer(MappedFrameBuffer &&rhs)
>>>> +{
>>>> +	error_ = rhs.error_;
>>>> +	valid_ = rhs.valid_;
>>>> +	maps_ = std::move(rhs.maps_);
>>>
>>> This is the default move constructor, so you could simply write
>>>
>>> 	MappedFrameBuffer(MappedFrameBuffer &&other) = default;
>>>
>>> in the class definition. However, I think you should keep this, and add
>>>
>>> 	rhs.error_ = 0;
>>> 	rhs.valid_ = false;
>>>
>>> Note that you can implement the move constructor in terms of the move
>>> assignment operator:
>>>
>>> 	*this = std::move(other);
>>
>> Ok, so I think we need to keep this and have it as:
>>
>> MappedFrameBuffer::MappedFrameBuffer(MappedFrameBuffer &&rhs)
>> {
>> 	*this = std::move(other);
>>  	rhs.error_ = 0;
>>  	rhs.valid_ = false;
>> }
>>
>> MappedFrameBuffer operator=(MappedFrameBuffer &&rhs)
>> {
>> 	*this = std::move(other);
> 
> Won't this cause a recursive call to the move assignement operator ?


Haha, maybe, I'd have discovered that when I actually got round to
compiling ;-)

I see now that you mean the 'move constructor' should just call the move
assignement operator ;-)



Thanks,



> 
>>  	rhs.error_ = 0;
>>  	rhs.valid_ = false;
>> }
>>
>> I was sure I already had the resetting of rhs, but I guess I dropped it
>> somewhere.
>>
>> It's a shame to have to duplicate for the assignment operator, but I
>> don't think that's too bad.
>>
>>>> +}
>>>> +
>>>>  } /* namespace libcamera */
> 

-- 
Regards
--
Kieran