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

[Kieran Bingham]

Hi Laurent,

On 24/07/2020 17:58, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> 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


--
Kieran


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

> 
>> +
>> +	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);
 	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.

--
Kieran



> 
>> +}
>> +
>>  } /* namespace libcamera */
> 

-- 
Regards
--
Kieran