[libcamera-devel] [RFC PATCH v2 05/10] ipa: rkisp1: Use offset in mapping IPABuffer
Hirokazu Honda
hiroh at chromium.org
Wed Aug 25 08:13:43 CEST 2021
Hi Laurent and Kieran, thank you for reviewing.
On Tue, Aug 24, 2021 at 9:44 PM Kieran Bingham
<kieran.bingham at 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 at 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 at 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
> >
More information about the libcamera-devel
mailing list