| Message ID | 20200720224232.153717-2-kieran.bingham@ideasonboard.com |
|---|---|
| State | RFC |
| Headers | show |
| Series |
|
| Related | show |
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@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. > 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 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 ? > + > + 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 * ... */ > +/** > + * \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 ? > + } > + > + maps_.push_back({address, plane.length}); > + } > + > + valid_ = buffer->planes().size() == maps_.size(); > +} > + > +MappedFrameBuffer::~MappedFrameBuffer() > +{ > + for (MappedPlane map : maps_) for (MappedPlane &map : maps_) > + 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); > +} > + > } /* namespace libcamera */
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@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 */ >
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@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 */
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@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 */ >
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. */ + MappedFrameBuffer(MappedFrameBuffer &&rhs); + + struct MappedPlane { + void *address; + size_t length; + }; + + 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; } +/** + * \brief Map all planes of a FrameBuffer + * \param[in] src Buffer to be mapped + * \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; + } + + maps_.push_back({address, plane.length}); + } + + valid_ = buffer->planes().size() == maps_.size(); +} + +MappedFrameBuffer::~MappedFrameBuffer() +{ + for (MappedPlane map : maps_) + munmap(map.address, map.length); +} + +MappedFrameBuffer::MappedFrameBuffer(MappedFrameBuffer &&rhs) +{ + error_ = rhs.error_; + valid_ = rhs.valid_; + maps_ = std::move(rhs.maps_); +} + } /* namespace libcamera */
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@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 ++++++++++++++++++++ src/libcamera/buffer.cpp | 43 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+)