| Message ID | 20210823131221.1034059-6-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, Thank you for the patch. On Mon, Aug 23, 2021 at 10:12:16PM +0900, Hirokazu Honda wrote: > IPABuffer is represented by FrameBuffer. FrameBuffer::Plane has > now an offset. This uses the offset variable to map the IPABuffer. The commit message and subject line should be updated, this patch now moves to MappedFrameBuffer. > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > --- > .../libcamera/internal/mapped_framebuffer.h | 1 + > src/ipa/rkisp1/rkisp1.cpp | 31 +++++++------------ > src/libcamera/mapped_framebuffer.cpp | 7 +++++ > 3 files changed, 19 insertions(+), 20 deletions(-) > > diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h > index 42479541..ee0583d0 100644 > --- a/include/libcamera/internal/mapped_framebuffer.h > +++ b/include/libcamera/internal/mapped_framebuffer.h > @@ -55,6 +55,7 @@ public: > > using MapFlags = Flags<MapFlag>; > > + MappedFrameBuffer(); > MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags); > }; > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index 06fb9640..54cf2885 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -10,7 +10,6 @@ > #include <queue> > #include <stdint.h> > #include <string.h> > -#include <sys/mman.h> > > #include <linux/rkisp1-config.h> > #include <linux/v4l2-controls.h> > @@ -24,6 +23,8 @@ > #include <libcamera/ipa/rkisp1_ipa_interface.h> > #include <libcamera/request.h> > > +#include <libcamera/internal/mapped_framebuffer.h> > + > namespace libcamera { > > LOG_DEFINE_CATEGORY(IPARkISP1) > @@ -54,7 +55,7 @@ private: > void metadataReady(unsigned int frame, unsigned int aeState); > > std::map<unsigned int, FrameBuffer> buffers_; > - std::map<unsigned int, void *> buffersMemory_; > + std::map<unsigned int, MappedFrameBuffer> mappedBuffers_; > > ControlInfoMap ctrls_; > > @@ -160,21 +161,10 @@ void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers) > std::forward_as_tuple(buffer.planes)); > const FrameBuffer &fb = elem.first->second; > > - /* > - * \todo Provide a helper to mmap() buffers (possibly exposed > - * to applications). > - */ > - buffersMemory_[buffer.id] = mmap(NULL, > - fb.planes()[0].length, > - PROT_READ | PROT_WRITE, > - MAP_SHARED, > - fb.planes()[0].fd.fd(), > - 0); > - > - if (buffersMemory_[buffer.id] == MAP_FAILED) { > - int ret = -errno; > + mappedBuffers_[buffer.id] = MappedFrameBuffer(&fb, MappedFrameBuffer::MapFlag::ReadWrite); We could use emplace() here to avoid the need for a default constructor, but we wouldn't be able to use operator[]() below, which isn't nice. I wonder if we could maybe store a std::unique_ptr<MappedFrameBuffer> in mappedBuffers_, to avoid the default constructor ? Kieran, you've designed the MappedFrameBuffer class, what do you think ? Apart from this, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + if (!mappedBuffers_[buffer.id].isValid()) { > LOG(IPARkISP1, Fatal) << "Failed to mmap buffer: " > - << strerror(-ret); > + << strerror(mappedBuffers_[buffer.id].error()); > } > } > } > @@ -186,8 +176,7 @@ void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids) > if (fb == buffers_.end()) > continue; > > - munmap(buffersMemory_[id], fb->second.planes()[0].length); > - buffersMemory_.erase(id); > + mappedBuffers_.erase(id); > buffers_.erase(id); > } > } > @@ -200,7 +189,8 @@ void IPARkISP1::processEvent(const RkISP1Event &event) > unsigned int bufferId = event.bufferId; > > const rkisp1_stat_buffer *stats = > - static_cast<rkisp1_stat_buffer *>(buffersMemory_[bufferId]); > + reinterpret_cast<rkisp1_stat_buffer *>( > + mappedBuffers_[bufferId].maps()[0].data()); > > updateStatistics(frame, stats); > break; > @@ -210,7 +200,8 @@ void IPARkISP1::processEvent(const RkISP1Event &event) > unsigned int bufferId = event.bufferId; > > rkisp1_params_cfg *params = > - static_cast<rkisp1_params_cfg *>(buffersMemory_[bufferId]); > + reinterpret_cast<rkisp1_params_cfg *>( > + mappedBuffers_[bufferId].maps()[0].data()); > > queueRequest(frame, params, event.controls); > break; > diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp > index 03425dea..34d9564d 100644 > --- a/src/libcamera/mapped_framebuffer.cpp > +++ b/src/libcamera/mapped_framebuffer.cpp > @@ -168,6 +168,13 @@ MappedBuffer::~MappedBuffer() > * \brief A bitwise combination of MappedFrameBuffer::MapFlag values > */ > > +/** > + * \brief Construct an empty MappedFrameBuffer > + */ > +MappedFrameBuffer::MappedFrameBuffer() > + : MappedBuffer() > +{ > +} > /** > * \brief Map all planes of a FrameBuffer > * \param[in] buffer FrameBuffer to be mapped
Hi, On 23/08/2021 23:55, Laurent Pinchart wrote: > Hi Hiro, > > Thank you for the patch. > > On Mon, Aug 23, 2021 at 10:12:16PM +0900, Hirokazu Honda wrote: >> IPABuffer is represented by FrameBuffer. FrameBuffer::Plane has >> now an offset. This uses the offset variable to map the IPABuffer. > > The commit message and subject line should be updated, this patch now > moves to MappedFrameBuffer. > >> Signed-off-by: Hirokazu Honda <hiroh@chromium.org> >> --- >> .../libcamera/internal/mapped_framebuffer.h | 1 + >> src/ipa/rkisp1/rkisp1.cpp | 31 +++++++------------ >> src/libcamera/mapped_framebuffer.cpp | 7 +++++ >> 3 files changed, 19 insertions(+), 20 deletions(-) >> >> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h >> index 42479541..ee0583d0 100644 >> --- a/include/libcamera/internal/mapped_framebuffer.h >> +++ b/include/libcamera/internal/mapped_framebuffer.h >> @@ -55,6 +55,7 @@ public: >> >> using MapFlags = Flags<MapFlag>; >> >> + MappedFrameBuffer(); >> MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags); >> }; >> >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp >> index 06fb9640..54cf2885 100644 >> --- a/src/ipa/rkisp1/rkisp1.cpp >> +++ b/src/ipa/rkisp1/rkisp1.cpp >> @@ -10,7 +10,6 @@ >> #include <queue> >> #include <stdint.h> >> #include <string.h> >> -#include <sys/mman.h> >> >> #include <linux/rkisp1-config.h> >> #include <linux/v4l2-controls.h> >> @@ -24,6 +23,8 @@ >> #include <libcamera/ipa/rkisp1_ipa_interface.h> >> #include <libcamera/request.h> >> >> +#include <libcamera/internal/mapped_framebuffer.h> >> + >> namespace libcamera { >> >> LOG_DEFINE_CATEGORY(IPARkISP1) >> @@ -54,7 +55,7 @@ private: >> void metadataReady(unsigned int frame, unsigned int aeState); >> >> std::map<unsigned int, FrameBuffer> buffers_; >> - std::map<unsigned int, void *> buffersMemory_; >> + std::map<unsigned int, MappedFrameBuffer> mappedBuffers_; >> >> ControlInfoMap ctrls_; >> >> @@ -160,21 +161,10 @@ void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers) >> std::forward_as_tuple(buffer.planes)); >> const FrameBuffer &fb = elem.first->second; >> >> - /* >> - * \todo Provide a helper to mmap() buffers (possibly exposed >> - * to applications). >> - */ >> - buffersMemory_[buffer.id] = mmap(NULL, >> - fb.planes()[0].length, >> - PROT_READ | PROT_WRITE, >> - MAP_SHARED, >> - fb.planes()[0].fd.fd(), >> - 0); >> - >> - if (buffersMemory_[buffer.id] == MAP_FAILED) { >> - int ret = -errno; >> + mappedBuffers_[buffer.id] = MappedFrameBuffer(&fb, MappedFrameBuffer::MapFlag::ReadWrite); > > We could use emplace() here to avoid the need for a default constructor, > but we wouldn't be able to use operator[]() below, which isn't nice. I > wonder if we could maybe store a std::unique_ptr<MappedFrameBuffer> in > mappedBuffers_, to avoid the default constructor ? > > Kieran, you've designed the MappedFrameBuffer class, what do you think ? Hrm, firstly the error_ flag needs to be fixed up if there's a default constructor - mentioned below in the Constructor code. But otherwise, even though we do have the isValid() check, I don't think we have many users that actually check it. The aim was to make it simple and easy to create the mapping, which is why it happens on construction. If this is causing problems, we can separate construction and mapping... but then I'd expect to have map()/unmap() as well. Looking at existing users of MappedFrameBuffer, I can see that: IPAVimc::mapBuffers() currently uses emplace, (with a piecewise_construct, to be able to specify the mapping flags as read-only), and only uses find() to get them. So does IPU3. ... And unsurprisingly, so does RPi. However none of those implementations are checking the isValid() flag. So perhaps they need to be updated to do so. And if we mandate that the isValid() should be checked, then it's not much different to separating construction and mapping... The main goal for the current design is to allow other buffer types to be constructed, so perhaps there could be a MappedDRMBuffer or MappedDMABuffer or such. As long as that's kept in mind, I don't mind if the main interface changes to suit needs as we determine them. Would using [], require checking the .isValid() flag at every usage? (Because we might be given a new empty mapping, if the search failed?) That might become less convenient ... ? > Apart from this, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> + if (!mappedBuffers_[buffer.id].isValid()) { >> LOG(IPARkISP1, Fatal) << "Failed to mmap buffer: " >> - << strerror(-ret); >> + << strerror(mappedBuffers_[buffer.id].error()); >> } >> } >> } >> @@ -186,8 +176,7 @@ void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids) >> if (fb == buffers_.end()) >> continue; >> >> - munmap(buffersMemory_[id], fb->second.planes()[0].length); >> - buffersMemory_.erase(id); >> + mappedBuffers_.erase(id); >> buffers_.erase(id); >> } >> } >> @@ -200,7 +189,8 @@ void IPARkISP1::processEvent(const RkISP1Event &event) >> unsigned int bufferId = event.bufferId; >> >> const rkisp1_stat_buffer *stats = >> - static_cast<rkisp1_stat_buffer *>(buffersMemory_[bufferId]); >> + reinterpret_cast<rkisp1_stat_buffer *>( >> + mappedBuffers_[bufferId].maps()[0].data()); >> >> updateStatistics(frame, stats); >> break; >> @@ -210,7 +200,8 @@ void IPARkISP1::processEvent(const RkISP1Event &event) >> unsigned int bufferId = event.bufferId; >> >> rkisp1_params_cfg *params = >> - static_cast<rkisp1_params_cfg *>(buffersMemory_[bufferId]); >> + reinterpret_cast<rkisp1_params_cfg *>( >> + mappedBuffers_[bufferId].maps()[0].data()); >> >> queueRequest(frame, params, event.controls); >> break; >> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp >> index 03425dea..34d9564d 100644 >> --- a/src/libcamera/mapped_framebuffer.cpp >> +++ b/src/libcamera/mapped_framebuffer.cpp >> @@ -168,6 +168,13 @@ MappedBuffer::~MappedBuffer() >> * \brief A bitwise combination of MappedFrameBuffer::MapFlag values >> */ >> >> +/** >> + * \brief Construct an empty MappedFrameBuffer >> + */ >> +MappedFrameBuffer::MappedFrameBuffer() >> + : MappedBuffer() >> +{ If this is created, error_ would have to be set to an ... error here. Perhaps something like -ENOMEM, so that 'default constructed' empty mappings are not 'valid'. That would then necessitate changine the ordering of how error_ is used, currently it's only set when an error occurs on mapping. It would have to be set to 0 on successful mappings as well/instead. >> +} >> /** >> * \brief Map all planes of a FrameBuffer >> * \param[in] buffer FrameBuffer to be mapped >
Hi Laurent and Kieran, thank you for reviewing. On Tue, Aug 24, 2021 at 9:44 PM Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Hi, > > On 23/08/2021 23:55, Laurent Pinchart wrote: > > Hi Hiro, > > > > Thank you for the patch. > > > > On Mon, Aug 23, 2021 at 10:12:16PM +0900, Hirokazu Honda wrote: > >> IPABuffer is represented by FrameBuffer. FrameBuffer::Plane has > >> now an offset. This uses the offset variable to map the IPABuffer. > > > > The commit message and subject line should be updated, this patch now > > moves to MappedFrameBuffer. > > > >> Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > >> --- > >> .../libcamera/internal/mapped_framebuffer.h | 1 + > >> src/ipa/rkisp1/rkisp1.cpp | 31 +++++++------------ > >> src/libcamera/mapped_framebuffer.cpp | 7 +++++ > >> 3 files changed, 19 insertions(+), 20 deletions(-) > >> > >> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h > >> index 42479541..ee0583d0 100644 > >> --- a/include/libcamera/internal/mapped_framebuffer.h > >> +++ b/include/libcamera/internal/mapped_framebuffer.h > >> @@ -55,6 +55,7 @@ public: > >> > >> using MapFlags = Flags<MapFlag>; > >> > >> + MappedFrameBuffer(); > >> MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags); > >> }; > >> > >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > >> index 06fb9640..54cf2885 100644 > >> --- a/src/ipa/rkisp1/rkisp1.cpp > >> +++ b/src/ipa/rkisp1/rkisp1.cpp > >> @@ -10,7 +10,6 @@ > >> #include <queue> > >> #include <stdint.h> > >> #include <string.h> > >> -#include <sys/mman.h> > >> > >> #include <linux/rkisp1-config.h> > >> #include <linux/v4l2-controls.h> > >> @@ -24,6 +23,8 @@ > >> #include <libcamera/ipa/rkisp1_ipa_interface.h> > >> #include <libcamera/request.h> > >> > >> +#include <libcamera/internal/mapped_framebuffer.h> > >> + > >> namespace libcamera { > >> > >> LOG_DEFINE_CATEGORY(IPARkISP1) > >> @@ -54,7 +55,7 @@ private: > >> void metadataReady(unsigned int frame, unsigned int aeState); > >> > >> std::map<unsigned int, FrameBuffer> buffers_; > >> - std::map<unsigned int, void *> buffersMemory_; > >> + std::map<unsigned int, MappedFrameBuffer> mappedBuffers_; > >> > >> ControlInfoMap ctrls_; > >> > >> @@ -160,21 +161,10 @@ void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers) > >> std::forward_as_tuple(buffer.planes)); > >> const FrameBuffer &fb = elem.first->second; > >> > >> - /* > >> - * \todo Provide a helper to mmap() buffers (possibly exposed > >> - * to applications). > >> - */ > >> - buffersMemory_[buffer.id] = mmap(NULL, > >> - fb.planes()[0].length, > >> - PROT_READ | PROT_WRITE, > >> - MAP_SHARED, > >> - fb.planes()[0].fd.fd(), > >> - 0); > >> - > >> - if (buffersMemory_[buffer.id] == MAP_FAILED) { > >> - int ret = -errno; > >> + mappedBuffers_[buffer.id] = MappedFrameBuffer(&fb, MappedFrameBuffer::MapFlag::ReadWrite); > > > > We could use emplace() here to avoid the need for a default constructor, > > but we wouldn't be able to use operator[]() below, which isn't nice. I > > wonder if we could maybe store a std::unique_ptr<MappedFrameBuffer> in > > mappedBuffers_, to avoid the default constructor ? > > > > Kieran, you've designed the MappedFrameBuffer class, what do you think ? > > Hrm, firstly the error_ flag needs to be fixed up if there's a default > constructor - mentioned below in the Constructor code. > > But otherwise, even though we do have the isValid() check, I don't think > we have many users that actually check it. > > The aim was to make it simple and easy to create the mapping, which is > why it happens on construction. > > If this is causing problems, we can separate construction and mapping... > but then I'd expect to have map()/unmap() as well. > > > Looking at existing users of MappedFrameBuffer, I can see that: > > IPAVimc::mapBuffers() currently uses emplace, (with a > piecewise_construct, to be able to specify the mapping flags as > read-only), and only uses find() to get them. > > > So does IPU3. ... > > And unsurprisingly, so does RPi. > > > However none of those implementations are checking the isValid() flag. > So perhaps they need to be updated to do so. > > And if we mandate that the isValid() should be checked, then it's not > much different to separating construction and mapping... > > > The main goal for the current design is to allow other buffer types to > be constructed, so perhaps there could be a MappedDRMBuffer or > MappedDMABuffer or such. > > As long as that's kept in mind, I don't mind if the main interface > changes to suit needs as we determine them. > > Would using [], require checking the .isValid() flag at every usage? > (Because we might be given a new empty mapping, if the search failed?) > > That might become less convenient ... ? > > I store std::unique_ptr<MappedFrameBuffer> to map as Laurent suggested. So I can remove the default constructor of MappedFrameBuffer. Regards, -Hiro > > > Apart from this, > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > >> + if (!mappedBuffers_[buffer.id].isValid()) { > >> LOG(IPARkISP1, Fatal) << "Failed to mmap buffer: " > >> - << strerror(-ret); > >> + << strerror(mappedBuffers_[buffer.id].error()); > >> } > >> } > >> } > >> @@ -186,8 +176,7 @@ void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids) > >> if (fb == buffers_.end()) > >> continue; > >> > >> - munmap(buffersMemory_[id], fb->second.planes()[0].length); > >> - buffersMemory_.erase(id); > >> + mappedBuffers_.erase(id); > >> buffers_.erase(id); > >> } > >> } > >> @@ -200,7 +189,8 @@ void IPARkISP1::processEvent(const RkISP1Event &event) > >> unsigned int bufferId = event.bufferId; > >> > >> const rkisp1_stat_buffer *stats = > >> - static_cast<rkisp1_stat_buffer *>(buffersMemory_[bufferId]); > >> + reinterpret_cast<rkisp1_stat_buffer *>( > >> + mappedBuffers_[bufferId].maps()[0].data()); > >> > >> updateStatistics(frame, stats); > >> break; > >> @@ -210,7 +200,8 @@ void IPARkISP1::processEvent(const RkISP1Event &event) > >> unsigned int bufferId = event.bufferId; > >> > >> rkisp1_params_cfg *params = > >> - static_cast<rkisp1_params_cfg *>(buffersMemory_[bufferId]); > >> + reinterpret_cast<rkisp1_params_cfg *>( > >> + mappedBuffers_[bufferId].maps()[0].data()); > >> > >> queueRequest(frame, params, event.controls); > >> break; > >> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp > >> index 03425dea..34d9564d 100644 > >> --- a/src/libcamera/mapped_framebuffer.cpp > >> +++ b/src/libcamera/mapped_framebuffer.cpp > >> @@ -168,6 +168,13 @@ MappedBuffer::~MappedBuffer() > >> * \brief A bitwise combination of MappedFrameBuffer::MapFlag values > >> */ > >> > >> +/** > >> + * \brief Construct an empty MappedFrameBuffer > >> + */ > >> +MappedFrameBuffer::MappedFrameBuffer() > >> + : MappedBuffer() > >> +{ > > If this is created, error_ would have to be set to an ... error here. > > Perhaps something like -ENOMEM, so that 'default constructed' empty > mappings are not 'valid'. > > That would then necessitate changine the ordering of how error_ is used, > currently it's only set when an error occurs on mapping. > > It would have to be set to 0 on successful mappings as well/instead. > > > > >> +} > >> /** > >> * \brief Map all planes of a FrameBuffer > >> * \param[in] buffer FrameBuffer to be mapped > >
Hi Hiro, On 25/08/2021 07:13, Hirokazu Honda wrote: > Hi Laurent and Kieran, thank you for reviewing. <snip> >>> >>> We could use emplace() here to avoid the need for a default constructor, >>> but we wouldn't be able to use operator[]() below, which isn't nice. I >>> wonder if we could maybe store a std::unique_ptr<MappedFrameBuffer> in >>> mappedBuffers_, to avoid the default constructor ? >>> >>> Kieran, you've designed the MappedFrameBuffer class, what do you think ? >> >> Hrm, firstly the error_ flag needs to be fixed up if there's a default >> constructor - mentioned below in the Constructor code. >> >> But otherwise, even though we do have the isValid() check, I don't think >> we have many users that actually check it. >> >> The aim was to make it simple and easy to create the mapping, which is >> why it happens on construction. >> >> If this is causing problems, we can separate construction and mapping... >> but then I'd expect to have map()/unmap() as well. >> >> >> Looking at existing users of MappedFrameBuffer, I can see that: >> >> IPAVimc::mapBuffers() currently uses emplace, (with a >> piecewise_construct, to be able to specify the mapping flags as >> read-only), and only uses find() to get them. >> >> >> So does IPU3. ... >> >> And unsurprisingly, so does RPi. >> >> >> However none of those implementations are checking the isValid() flag. >> So perhaps they need to be updated to do so. >> >> And if we mandate that the isValid() should be checked, then it's not >> much different to separating construction and mapping... >> >> >> The main goal for the current design is to allow other buffer types to >> be constructed, so perhaps there could be a MappedDRMBuffer or >> MappedDMABuffer or such. >> >> As long as that's kept in mind, I don't mind if the main interface >> changes to suit needs as we determine them. >> >> Would using [], require checking the .isValid() flag at every usage? >> (Because we might be given a new empty mapping, if the search failed?) >> >> That might become less convenient ... ? >> >> > > I store std::unique_ptr<MappedFrameBuffer> to map as Laurent suggested. > So I can remove the default constructor of MappedFrameBuffer. Do we need to change all the other IPA's to use the same pattern ? -- Kieran
Hi Kieran, On Wed, Aug 25, 2021 at 5:06 PM Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Hi Hiro, > > On 25/08/2021 07:13, Hirokazu Honda wrote: > > Hi Laurent and Kieran, thank you for reviewing. > > <snip> > > >>> > >>> We could use emplace() here to avoid the need for a default constructor, > >>> but we wouldn't be able to use operator[]() below, which isn't nice. I > >>> wonder if we could maybe store a std::unique_ptr<MappedFrameBuffer> in > >>> mappedBuffers_, to avoid the default constructor ? > >>> > >>> Kieran, you've designed the MappedFrameBuffer class, what do you think ? > >> > >> Hrm, firstly the error_ flag needs to be fixed up if there's a default > >> constructor - mentioned below in the Constructor code. > >> > >> But otherwise, even though we do have the isValid() check, I don't think > >> we have many users that actually check it. > >> > >> The aim was to make it simple and easy to create the mapping, which is > >> why it happens on construction. > >> > >> If this is causing problems, we can separate construction and mapping... > >> but then I'd expect to have map()/unmap() as well. > >> > >> > >> Looking at existing users of MappedFrameBuffer, I can see that: > >> > >> IPAVimc::mapBuffers() currently uses emplace, (with a > >> piecewise_construct, to be able to specify the mapping flags as > >> read-only), and only uses find() to get them. > >> > >> > >> So does IPU3. ... > >> > >> And unsurprisingly, so does RPi. > >> > >> > >> However none of those implementations are checking the isValid() flag. > >> So perhaps they need to be updated to do so. > >> > >> And if we mandate that the isValid() should be checked, then it's not > >> much different to separating construction and mapping... > >> > >> > >> The main goal for the current design is to allow other buffer types to > >> be constructed, so perhaps there could be a MappedDRMBuffer or > >> MappedDMABuffer or such. > >> > >> As long as that's kept in mind, I don't mind if the main interface > >> changes to suit needs as we determine them. > >> > >> Would using [], require checking the .isValid() flag at every usage? > >> (Because we might be given a new empty mapping, if the search failed?) > >> > >> That might become less convenient ... ? > >> > >> > > > > I store std::unique_ptr<MappedFrameBuffer> to map as Laurent suggested. > > So I can remove the default constructor of MappedFrameBuffer. > > Do we need to change all the other IPA's to use the same pattern ? > Yes, that should be good. Which do you prefer, storing MappedFrameBuffer as unique_ptr or having MappedFrameBuffer constructor? -Hiro > -- > Kieran >
Hi Kieran, On Tue, Aug 24, 2021 at 01:44:15PM +0100, Kieran Bingham wrote: > On 23/08/2021 23:55, Laurent Pinchart wrote: > > On Mon, Aug 23, 2021 at 10:12:16PM +0900, Hirokazu Honda wrote: > >> IPABuffer is represented by FrameBuffer. FrameBuffer::Plane has > >> now an offset. This uses the offset variable to map the IPABuffer. > > > > The commit message and subject line should be updated, this patch now > > moves to MappedFrameBuffer. > > > >> Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > >> --- > >> .../libcamera/internal/mapped_framebuffer.h | 1 + > >> src/ipa/rkisp1/rkisp1.cpp | 31 +++++++------------ > >> src/libcamera/mapped_framebuffer.cpp | 7 +++++ > >> 3 files changed, 19 insertions(+), 20 deletions(-) > >> > >> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h > >> index 42479541..ee0583d0 100644 > >> --- a/include/libcamera/internal/mapped_framebuffer.h > >> +++ b/include/libcamera/internal/mapped_framebuffer.h > >> @@ -55,6 +55,7 @@ public: > >> > >> using MapFlags = Flags<MapFlag>; > >> > >> + MappedFrameBuffer(); > >> MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags); > >> }; > >> > >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > >> index 06fb9640..54cf2885 100644 > >> --- a/src/ipa/rkisp1/rkisp1.cpp > >> +++ b/src/ipa/rkisp1/rkisp1.cpp > >> @@ -10,7 +10,6 @@ > >> #include <queue> > >> #include <stdint.h> > >> #include <string.h> > >> -#include <sys/mman.h> > >> > >> #include <linux/rkisp1-config.h> > >> #include <linux/v4l2-controls.h> > >> @@ -24,6 +23,8 @@ > >> #include <libcamera/ipa/rkisp1_ipa_interface.h> > >> #include <libcamera/request.h> > >> > >> +#include <libcamera/internal/mapped_framebuffer.h> > >> + > >> namespace libcamera { > >> > >> LOG_DEFINE_CATEGORY(IPARkISP1) > >> @@ -54,7 +55,7 @@ private: > >> void metadataReady(unsigned int frame, unsigned int aeState); > >> > >> std::map<unsigned int, FrameBuffer> buffers_; > >> - std::map<unsigned int, void *> buffersMemory_; > >> + std::map<unsigned int, MappedFrameBuffer> mappedBuffers_; > >> > >> ControlInfoMap ctrls_; > >> > >> @@ -160,21 +161,10 @@ void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers) > >> std::forward_as_tuple(buffer.planes)); > >> const FrameBuffer &fb = elem.first->second; > >> > >> - /* > >> - * \todo Provide a helper to mmap() buffers (possibly exposed > >> - * to applications). > >> - */ > >> - buffersMemory_[buffer.id] = mmap(NULL, > >> - fb.planes()[0].length, > >> - PROT_READ | PROT_WRITE, > >> - MAP_SHARED, > >> - fb.planes()[0].fd.fd(), > >> - 0); > >> - > >> - if (buffersMemory_[buffer.id] == MAP_FAILED) { > >> - int ret = -errno; > >> + mappedBuffers_[buffer.id] = MappedFrameBuffer(&fb, MappedFrameBuffer::MapFlag::ReadWrite); > > > > We could use emplace() here to avoid the need for a default constructor, > > but we wouldn't be able to use operator[]() below, which isn't nice. I > > wonder if we could maybe store a std::unique_ptr<MappedFrameBuffer> in > > mappedBuffers_, to avoid the default constructor ? > > > > Kieran, you've designed the MappedFrameBuffer class, what do you think ? > > Hrm, firstly the error_ flag needs to be fixed up if there's a default > constructor - mentioned below in the Constructor code. > > But otherwise, even though we do have the isValid() check, I don't think > we have many users that actually check it. > > The aim was to make it simple and easy to create the mapping, which is > why it happens on construction. > > If this is causing problems, we can separate construction and mapping... > but then I'd expect to have map()/unmap() as well. > Looking at existing users of MappedFrameBuffer, I can see that: > > IPAVimc::mapBuffers() currently uses emplace, (with a > piecewise_construct, to be able to specify the mapping flags as > read-only), and only uses find() to get them. > > > So does IPU3. ... > > And unsurprisingly, so does RPi. > > > However none of those implementations are checking the isValid() flag. > So perhaps they need to be updated to do so. > > And if we mandate that the isValid() should be checked, then it's not > much different to separating construction and mapping... I like having construction and mapping grouped together, but you're right that separating them could indeed force users to check for errors (if we annotate the map() function with __nodiscard). > The main goal for the current design is to allow other buffer types to > be constructed, so perhaps there could be a MappedDRMBuffer or > MappedDMABuffer or such. > > As long as that's kept in mind, I don't mind if the main interface > changes to suit needs as we determine them. > > Would using [], require checking the .isValid() flag at every usage? > (Because we might be given a new empty mapping, if the search failed?) Not really, a user could be trusted to pass valid keys to operator[](). Storing the instances in a std::map<std::unique_ptr<MappedFrameBuffer>> as I've proposed would result in a null pointer dereference if the code tried to access an entry with an invalid key, which wouldn't be very different than using a default-constructed MappedFrameBuffer. We would "just" crash later in that case :-) > That might become less convenient ... ? > > > Apart from this, > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > >> + if (!mappedBuffers_[buffer.id].isValid()) { > >> LOG(IPARkISP1, Fatal) << "Failed to mmap buffer: " > >> - << strerror(-ret); > >> + << strerror(mappedBuffers_[buffer.id].error()); > >> } > >> } > >> } > >> @@ -186,8 +176,7 @@ void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids) > >> if (fb == buffers_.end()) > >> continue; > >> > >> - munmap(buffersMemory_[id], fb->second.planes()[0].length); > >> - buffersMemory_.erase(id); > >> + mappedBuffers_.erase(id); > >> buffers_.erase(id); > >> } > >> } > >> @@ -200,7 +189,8 @@ void IPARkISP1::processEvent(const RkISP1Event &event) > >> unsigned int bufferId = event.bufferId; > >> > >> const rkisp1_stat_buffer *stats = > >> - static_cast<rkisp1_stat_buffer *>(buffersMemory_[bufferId]); > >> + reinterpret_cast<rkisp1_stat_buffer *>( > >> + mappedBuffers_[bufferId].maps()[0].data()); > >> > >> updateStatistics(frame, stats); > >> break; > >> @@ -210,7 +200,8 @@ void IPARkISP1::processEvent(const RkISP1Event &event) > >> unsigned int bufferId = event.bufferId; > >> > >> rkisp1_params_cfg *params = > >> - static_cast<rkisp1_params_cfg *>(buffersMemory_[bufferId]); > >> + reinterpret_cast<rkisp1_params_cfg *>( > >> + mappedBuffers_[bufferId].maps()[0].data()); > >> > >> queueRequest(frame, params, event.controls); > >> break; > >> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp > >> index 03425dea..34d9564d 100644 > >> --- a/src/libcamera/mapped_framebuffer.cpp > >> +++ b/src/libcamera/mapped_framebuffer.cpp > >> @@ -168,6 +168,13 @@ MappedBuffer::~MappedBuffer() > >> * \brief A bitwise combination of MappedFrameBuffer::MapFlag values > >> */ > >> > >> +/** > >> + * \brief Construct an empty MappedFrameBuffer > >> + */ > >> +MappedFrameBuffer::MappedFrameBuffer() > >> + : MappedBuffer() > >> +{ > > If this is created, error_ would have to be set to an ... error here. > > Perhaps something like -ENOMEM, so that 'default constructed' empty > mappings are not 'valid'. > > That would then necessitate changine the ordering of how error_ is used, > currently it's only set when an error occurs on mapping. > > It would have to be set to 0 on successful mappings as well/instead. > > >> +} > >> /** > >> * \brief Map all planes of a FrameBuffer > >> * \param[in] buffer FrameBuffer to be mapped > >
Hi Hiro, On Mon, Aug 23, 2021 at 10:12:16PM +0900, Hirokazu Honda wrote: > IPABuffer is represented by FrameBuffer. FrameBuffer::Plane has > now an offset. This uses the offset variable to map the IPABuffer. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > --- > .../libcamera/internal/mapped_framebuffer.h | 1 + > src/ipa/rkisp1/rkisp1.cpp | 31 +++++++------------ > src/libcamera/mapped_framebuffer.cpp | 7 +++++ > 3 files changed, 19 insertions(+), 20 deletions(-) > > diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h > index 42479541..ee0583d0 100644 > --- a/include/libcamera/internal/mapped_framebuffer.h > +++ b/include/libcamera/internal/mapped_framebuffer.h > @@ -55,6 +55,7 @@ public: > > using MapFlags = Flags<MapFlag>; > > + MappedFrameBuffer(); > MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags); > }; > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index 06fb9640..54cf2885 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -10,7 +10,6 @@ > #include <queue> > #include <stdint.h> > #include <string.h> > -#include <sys/mman.h> > > #include <linux/rkisp1-config.h> > #include <linux/v4l2-controls.h> > @@ -24,6 +23,8 @@ > #include <libcamera/ipa/rkisp1_ipa_interface.h> > #include <libcamera/request.h> > > +#include <libcamera/internal/mapped_framebuffer.h> > + > namespace libcamera { > > LOG_DEFINE_CATEGORY(IPARkISP1) > @@ -54,7 +55,7 @@ private: > void metadataReady(unsigned int frame, unsigned int aeState); > > std::map<unsigned int, FrameBuffer> buffers_; > - std::map<unsigned int, void *> buffersMemory_; > + std::map<unsigned int, MappedFrameBuffer> mappedBuffers_; > > ControlInfoMap ctrls_; > > @@ -160,21 +161,10 @@ void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers) > std::forward_as_tuple(buffer.planes)); > const FrameBuffer &fb = elem.first->second; > > - /* > - * \todo Provide a helper to mmap() buffers (possibly exposed > - * to applications). > - */ > - buffersMemory_[buffer.id] = mmap(NULL, > - fb.planes()[0].length, > - PROT_READ | PROT_WRITE, > - MAP_SHARED, > - fb.planes()[0].fd.fd(), > - 0); > - > - if (buffersMemory_[buffer.id] == MAP_FAILED) { > - int ret = -errno; > + mappedBuffers_[buffer.id] = MappedFrameBuffer(&fb, MappedFrameBuffer::MapFlag::ReadWrite); > + if (!mappedBuffers_[buffer.id].isValid()) { > LOG(IPARkISP1, Fatal) << "Failed to mmap buffer: " > - << strerror(-ret); > + << strerror(mappedBuffers_[buffer.id].error()); > } Another point I wanted to mention is that we could also fix the default constructor issue by avoiding operator[](). Here we could have MappedFrameBuffer mappedBuffer{ &fb, MappedFrameBuffer::MapFlag::ReadWrite }; if (!mappedBuffer.isValid()) { LOG(IPARkISP1, Fatal) << "Failed to mmap buffer: " << strerror(mappedBuffer.error()); } mappedBuffers_.emplace(buffer.id, std::move(mappedBuffer)); > } > } > @@ -186,8 +176,7 @@ void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids) > if (fb == buffers_.end()) > continue; > > - munmap(buffersMemory_[id], fb->second.planes()[0].length); > - buffersMemory_.erase(id); > + mappedBuffers_.erase(id); > buffers_.erase(id); > } > } > @@ -200,7 +189,8 @@ void IPARkISP1::processEvent(const RkISP1Event &event) > unsigned int bufferId = event.bufferId; > > const rkisp1_stat_buffer *stats = > - static_cast<rkisp1_stat_buffer *>(buffersMemory_[bufferId]); > + reinterpret_cast<rkisp1_stat_buffer *>( > + mappedBuffers_[bufferId].maps()[0].data()); And here and below, you could use mappedBuffers_.at(bufferId).maps()[0].data()); > > updateStatistics(frame, stats); > break; > @@ -210,7 +200,8 @@ void IPARkISP1::processEvent(const RkISP1Event &event) > unsigned int bufferId = event.bufferId; > > rkisp1_params_cfg *params = > - static_cast<rkisp1_params_cfg *>(buffersMemory_[bufferId]); > + reinterpret_cast<rkisp1_params_cfg *>( > + mappedBuffers_[bufferId].maps()[0].data()); > > queueRequest(frame, params, event.controls); > break; > diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp > index 03425dea..34d9564d 100644 > --- a/src/libcamera/mapped_framebuffer.cpp > +++ b/src/libcamera/mapped_framebuffer.cpp > @@ -168,6 +168,13 @@ MappedBuffer::~MappedBuffer() > * \brief A bitwise combination of MappedFrameBuffer::MapFlag values > */ > > +/** > + * \brief Construct an empty MappedFrameBuffer > + */ > +MappedFrameBuffer::MappedFrameBuffer() > + : MappedBuffer() > +{ > +} > /** > * \brief Map all planes of a FrameBuffer > * \param[in] buffer FrameBuffer to be mapped
On 25/08/2021 21:49, Laurent Pinchart wrote: > Hi Kieran, > > On Tue, Aug 24, 2021 at 01:44:15PM +0100, Kieran Bingham wrote: >> On 23/08/2021 23:55, Laurent Pinchart wrote: >>> On Mon, Aug 23, 2021 at 10:12:16PM +0900, Hirokazu Honda wrote: >>>> IPABuffer is represented by FrameBuffer. FrameBuffer::Plane has >>>> now an offset. This uses the offset variable to map the IPABuffer. >>> >>> The commit message and subject line should be updated, this patch now >>> moves to MappedFrameBuffer. >>> >>>> Signed-off-by: Hirokazu Honda <hiroh@chromium.org> >>>> --- >>>> .../libcamera/internal/mapped_framebuffer.h | 1 + >>>> src/ipa/rkisp1/rkisp1.cpp | 31 +++++++------------ >>>> src/libcamera/mapped_framebuffer.cpp | 7 +++++ >>>> 3 files changed, 19 insertions(+), 20 deletions(-) >>>> >>>> diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h >>>> index 42479541..ee0583d0 100644 >>>> --- a/include/libcamera/internal/mapped_framebuffer.h >>>> +++ b/include/libcamera/internal/mapped_framebuffer.h >>>> @@ -55,6 +55,7 @@ public: >>>> >>>> using MapFlags = Flags<MapFlag>; >>>> >>>> + MappedFrameBuffer(); >>>> MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags); >>>> }; >>>> >>>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp >>>> index 06fb9640..54cf2885 100644 >>>> --- a/src/ipa/rkisp1/rkisp1.cpp >>>> +++ b/src/ipa/rkisp1/rkisp1.cpp >>>> @@ -10,7 +10,6 @@ >>>> #include <queue> >>>> #include <stdint.h> >>>> #include <string.h> >>>> -#include <sys/mman.h> >>>> >>>> #include <linux/rkisp1-config.h> >>>> #include <linux/v4l2-controls.h> >>>> @@ -24,6 +23,8 @@ >>>> #include <libcamera/ipa/rkisp1_ipa_interface.h> >>>> #include <libcamera/request.h> >>>> >>>> +#include <libcamera/internal/mapped_framebuffer.h> >>>> + >>>> namespace libcamera { >>>> >>>> LOG_DEFINE_CATEGORY(IPARkISP1) >>>> @@ -54,7 +55,7 @@ private: >>>> void metadataReady(unsigned int frame, unsigned int aeState); >>>> >>>> std::map<unsigned int, FrameBuffer> buffers_; >>>> - std::map<unsigned int, void *> buffersMemory_; >>>> + std::map<unsigned int, MappedFrameBuffer> mappedBuffers_; >>>> >>>> ControlInfoMap ctrls_; >>>> >>>> @@ -160,21 +161,10 @@ void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers) >>>> std::forward_as_tuple(buffer.planes)); >>>> const FrameBuffer &fb = elem.first->second; >>>> >>>> - /* >>>> - * \todo Provide a helper to mmap() buffers (possibly exposed >>>> - * to applications). >>>> - */ >>>> - buffersMemory_[buffer.id] = mmap(NULL, >>>> - fb.planes()[0].length, >>>> - PROT_READ | PROT_WRITE, >>>> - MAP_SHARED, >>>> - fb.planes()[0].fd.fd(), >>>> - 0); >>>> - >>>> - if (buffersMemory_[buffer.id] == MAP_FAILED) { >>>> - int ret = -errno; >>>> + mappedBuffers_[buffer.id] = MappedFrameBuffer(&fb, MappedFrameBuffer::MapFlag::ReadWrite); >>> >>> We could use emplace() here to avoid the need for a default constructor, >>> but we wouldn't be able to use operator[]() below, which isn't nice. I >>> wonder if we could maybe store a std::unique_ptr<MappedFrameBuffer> in >>> mappedBuffers_, to avoid the default constructor ? >>> >>> Kieran, you've designed the MappedFrameBuffer class, what do you think ? >> >> Hrm, firstly the error_ flag needs to be fixed up if there's a default >> constructor - mentioned below in the Constructor code. >> >> But otherwise, even though we do have the isValid() check, I don't think >> we have many users that actually check it. >> >> The aim was to make it simple and easy to create the mapping, which is >> why it happens on construction. >> >> If this is causing problems, we can separate construction and mapping... >> but then I'd expect to have map()/unmap() as well. >> Looking at existing users of MappedFrameBuffer, I can see that: >> >> IPAVimc::mapBuffers() currently uses emplace, (with a >> piecewise_construct, to be able to specify the mapping flags as >> read-only), and only uses find() to get them. >> >> >> So does IPU3. ... >> >> And unsurprisingly, so does RPi. >> >> >> However none of those implementations are checking the isValid() flag. >> So perhaps they need to be updated to do so. >> >> And if we mandate that the isValid() should be checked, then it's not >> much different to separating construction and mapping... > > I like having construction and mapping grouped together, but you're > right that separating them could indeed force users to check for errors > (if we annotate the map() function with __nodiscard). Yes, the point was to make it easy to use. But if it's verging on promotion to an actual API exposed to applications, it's worth considering correctness >> The main goal for the current design is to allow other buffer types to >> be constructed, so perhaps there could be a MappedDRMBuffer or >> MappedDMABuffer or such. >> >> As long as that's kept in mind, I don't mind if the main interface >> changes to suit needs as we determine them. >> >> Would using [], require checking the .isValid() flag at every usage? >> (Because we might be given a new empty mapping, if the search failed?) > > Not really, a user could be trusted to pass valid keys to operator[](). > Storing the instances in a std::map<std::unique_ptr<MappedFrameBuffer>> > as I've proposed would result in a null pointer dereference if the code > tried to access an entry with an invalid key, which wouldn't be very > different than using a default-constructed MappedFrameBuffer. We would > "just" crash later in that case :-) So I'm not opposed to supporting the [], as long as it doesn't lead to potential mis-usages of a MappedFrameBuffer which aren't valid. And the .at() works too. > >> That might become less convenient ... ? >> >>> Apart from this, >>> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> >>>> + if (!mappedBuffers_[buffer.id].isValid()) { >>>> LOG(IPARkISP1, Fatal) << "Failed to mmap buffer: " >>>> - << strerror(-ret); >>>> + << strerror(mappedBuffers_[buffer.id].error()); >>>> } >>>> } >>>> } >>>> @@ -186,8 +176,7 @@ void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids) >>>> if (fb == buffers_.end()) >>>> continue; >>>> >>>> - munmap(buffersMemory_[id], fb->second.planes()[0].length); >>>> - buffersMemory_.erase(id); >>>> + mappedBuffers_.erase(id); >>>> buffers_.erase(id); >>>> } >>>> } >>>> @@ -200,7 +189,8 @@ void IPARkISP1::processEvent(const RkISP1Event &event) >>>> unsigned int bufferId = event.bufferId; >>>> >>>> const rkisp1_stat_buffer *stats = >>>> - static_cast<rkisp1_stat_buffer *>(buffersMemory_[bufferId]); >>>> + reinterpret_cast<rkisp1_stat_buffer *>( >>>> + mappedBuffers_[bufferId].maps()[0].data()); >>>> >>>> updateStatistics(frame, stats); >>>> break; >>>> @@ -210,7 +200,8 @@ void IPARkISP1::processEvent(const RkISP1Event &event) >>>> unsigned int bufferId = event.bufferId; >>>> >>>> rkisp1_params_cfg *params = >>>> - static_cast<rkisp1_params_cfg *>(buffersMemory_[bufferId]); >>>> + reinterpret_cast<rkisp1_params_cfg *>( >>>> + mappedBuffers_[bufferId].maps()[0].data()); >>>> >>>> queueRequest(frame, params, event.controls); >>>> break; >>>> diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp >>>> index 03425dea..34d9564d 100644 >>>> --- a/src/libcamera/mapped_framebuffer.cpp >>>> +++ b/src/libcamera/mapped_framebuffer.cpp >>>> @@ -168,6 +168,13 @@ MappedBuffer::~MappedBuffer() >>>> * \brief A bitwise combination of MappedFrameBuffer::MapFlag values >>>> */ >>>> >>>> +/** >>>> + * \brief Construct an empty MappedFrameBuffer >>>> + */ >>>> +MappedFrameBuffer::MappedFrameBuffer() >>>> + : MappedBuffer() >>>> +{ >> >> If this is created, error_ would have to be set to an ... error here. >> >> Perhaps something like -ENOMEM, so that 'default constructed' empty >> mappings are not 'valid'. >> >> That would then necessitate changine the ordering of how error_ is used, >> currently it's only set when an error occurs on mapping. >> >> It would have to be set to 0 on successful mappings as well/instead. >> >>>> +} >>>> /** >>>> * \brief Map all planes of a FrameBuffer >>>> * \param[in] buffer FrameBuffer to be mapped >>> >
diff --git a/include/libcamera/internal/mapped_framebuffer.h b/include/libcamera/internal/mapped_framebuffer.h index 42479541..ee0583d0 100644 --- a/include/libcamera/internal/mapped_framebuffer.h +++ b/include/libcamera/internal/mapped_framebuffer.h @@ -55,6 +55,7 @@ public: using MapFlags = Flags<MapFlag>; + MappedFrameBuffer(); MappedFrameBuffer(const FrameBuffer *buffer, MapFlags flags); }; diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index 06fb9640..54cf2885 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -10,7 +10,6 @@ #include <queue> #include <stdint.h> #include <string.h> -#include <sys/mman.h> #include <linux/rkisp1-config.h> #include <linux/v4l2-controls.h> @@ -24,6 +23,8 @@ #include <libcamera/ipa/rkisp1_ipa_interface.h> #include <libcamera/request.h> +#include <libcamera/internal/mapped_framebuffer.h> + namespace libcamera { LOG_DEFINE_CATEGORY(IPARkISP1) @@ -54,7 +55,7 @@ private: void metadataReady(unsigned int frame, unsigned int aeState); std::map<unsigned int, FrameBuffer> buffers_; - std::map<unsigned int, void *> buffersMemory_; + std::map<unsigned int, MappedFrameBuffer> mappedBuffers_; ControlInfoMap ctrls_; @@ -160,21 +161,10 @@ void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers) std::forward_as_tuple(buffer.planes)); const FrameBuffer &fb = elem.first->second; - /* - * \todo Provide a helper to mmap() buffers (possibly exposed - * to applications). - */ - buffersMemory_[buffer.id] = mmap(NULL, - fb.planes()[0].length, - PROT_READ | PROT_WRITE, - MAP_SHARED, - fb.planes()[0].fd.fd(), - 0); - - if (buffersMemory_[buffer.id] == MAP_FAILED) { - int ret = -errno; + mappedBuffers_[buffer.id] = MappedFrameBuffer(&fb, MappedFrameBuffer::MapFlag::ReadWrite); + if (!mappedBuffers_[buffer.id].isValid()) { LOG(IPARkISP1, Fatal) << "Failed to mmap buffer: " - << strerror(-ret); + << strerror(mappedBuffers_[buffer.id].error()); } } } @@ -186,8 +176,7 @@ void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids) if (fb == buffers_.end()) continue; - munmap(buffersMemory_[id], fb->second.planes()[0].length); - buffersMemory_.erase(id); + mappedBuffers_.erase(id); buffers_.erase(id); } } @@ -200,7 +189,8 @@ void IPARkISP1::processEvent(const RkISP1Event &event) unsigned int bufferId = event.bufferId; const rkisp1_stat_buffer *stats = - static_cast<rkisp1_stat_buffer *>(buffersMemory_[bufferId]); + reinterpret_cast<rkisp1_stat_buffer *>( + mappedBuffers_[bufferId].maps()[0].data()); updateStatistics(frame, stats); break; @@ -210,7 +200,8 @@ void IPARkISP1::processEvent(const RkISP1Event &event) unsigned int bufferId = event.bufferId; rkisp1_params_cfg *params = - static_cast<rkisp1_params_cfg *>(buffersMemory_[bufferId]); + reinterpret_cast<rkisp1_params_cfg *>( + mappedBuffers_[bufferId].maps()[0].data()); queueRequest(frame, params, event.controls); break; diff --git a/src/libcamera/mapped_framebuffer.cpp b/src/libcamera/mapped_framebuffer.cpp index 03425dea..34d9564d 100644 --- a/src/libcamera/mapped_framebuffer.cpp +++ b/src/libcamera/mapped_framebuffer.cpp @@ -168,6 +168,13 @@ MappedBuffer::~MappedBuffer() * \brief A bitwise combination of MappedFrameBuffer::MapFlag values */ +/** + * \brief Construct an empty MappedFrameBuffer + */ +MappedFrameBuffer::MappedFrameBuffer() + : MappedBuffer() +{ +} /** * \brief Map all planes of a FrameBuffer * \param[in] buffer FrameBuffer to be mapped
IPABuffer is represented by FrameBuffer. FrameBuffer::Plane has now an offset. This uses the offset variable to map the IPABuffer. Signed-off-by: Hirokazu Honda <hiroh@chromium.org> --- .../libcamera/internal/mapped_framebuffer.h | 1 + src/ipa/rkisp1/rkisp1.cpp | 31 +++++++------------ src/libcamera/mapped_framebuffer.cpp | 7 +++++ 3 files changed, 19 insertions(+), 20 deletions(-)