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

[Laurent Pinchart]

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.

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

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

Laurent Pinchart