[libcamera-devel] [PATCH 1/2] pipeline: raspberrypi: Use MappedFrameBuffer for embedded data buffers

[Naushir Patuck]

Hi Kieran,


On Thu, 12 Nov 2020 at 22:16, Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:

> Hi Naush,
>
> On 26/10/2020 09:53, Naushir Patuck wrote:
> > Use a MappedFrameBuffer to mmap embedded data buffers for the pipeline
> > handler to use in the cases where the sensor does not fill it in. This
> > avoids the need to mmap and unmap on every frame.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 35 ++++++++++++++-----
> >  1 file changed, 27 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index dd62dfc7..8817915b 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -24,6 +24,7 @@
> >  #include <linux/videodev2.h>
> >
> >  #include "libcamera/internal/bayer_format.h"
> > +#include "libcamera/internal/buffer.h"
> >  #include "libcamera/internal/camera_sensor.h"
> >  #include "libcamera/internal/device_enumerator.h"
> >  #include "libcamera/internal/ipa_manager.h"
> > @@ -165,6 +166,12 @@ public:
> >       /* Stores the ids of the buffers mapped in the IPA. */
> >       std::unordered_set<unsigned int> ipaBuffers_;
> >
> > +     /*
> > +      * Map of (internal) mmaped embedded data buffers, to avoid having
> to
> > +      * map/unmap on every frame.
> > +      */
> > +     std::map<unsigned int, MappedFrameBuffer> mappedEmbeddedBuffers_;
> > +
> >       /* DMAHEAP allocation helper. */
> >       RPi::DmaHeap dmaHeap_;
> >       FileDescriptor lsTable_;
> > @@ -1040,6 +1047,15 @@ int PipelineHandlerRPi::prepareBuffers(Camera
> *camera)
> >                       return ret;
> >       }
> >
> > +     if (!data->sensorMetadata_) {
> > +             for (auto const &it :
> data->unicam_[Unicam::Embedded].getBuffers()) {
> > +
>  data->mappedEmbeddedBuffers_.emplace(std::piecewise_construct,
> > +
> std::forward_as_tuple(it.first),
> > +
> std::forward_as_tuple(
> > +
> MappedFrameBuffer(it.second, PROT_READ | PROT_WRITE)));
>
> Does this need to use piecewise?
>
> Can't you just use:
>
>    emplace(it.first,
>            MappedFrameBuffer(it.second, PROT_READ | PROT_WRITE));
> directly?
>

This is hitting the limits of my C++ knowledge :-)

I thought using a piecewise_construct might be more efficient here, as you
do not create a temporary object of MappedFrameBuffer on the stack and do a
copy.  Although, the compiler might possibly optimise that out anyway....?
Not sure.  Either way, this is in startup code, and any possible penalty of
constructing and copying temporary objects is not really going to cause me
to stay up at night.  I'll change it to the above and post an update.


> Or is that then trying to use an invalid copy or move constructor?
>
> If you do need to piecewise, Does it make sense to shorten it to:
>
> emplace(std::piecewise_construct,
>         std::forward_as_tuple(it.first),
>         std::forward_as_tuple(it.second, PROT_READ | PROT_WRITE)));
>

I did not think this would work, but admittedly never tried it!

Regards,
Naush



> Although maybe it's more clear showing the MappedFrameBuffer type.
>
> Not really a problem what route is used as long as it works I think ;-)
>
>
> Just tried out a few options and they all seem to compile, so I'd try to
> remove the piecewise if it's not needed ...
>
> > +             }
> > +     }
> > +
> >       /*
> >        * Pass the stats and embedded data buffers to the IPA. No other
> >        * buffers need to be passed.
> > @@ -1078,6 +1094,7 @@ void PipelineHandlerRPi::freeBuffers(Camera
> *camera)
> >       std::vector<unsigned int> ipaBuffers(data->ipaBuffers_.begin(),
> data->ipaBuffers_.end());
> >       data->ipa_->unmapBuffers(ipaBuffers);
> >       data->ipaBuffers_.clear();
> > +     data->mappedEmbeddedBuffers_.clear();
> >
> >       for (auto const stream : data->streams_)
> >               stream->releaseBuffers();
> > @@ -1310,14 +1327,16 @@ void
> RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
> >                * metadata buffer.
> >                */
> >               if (!sensorMetadata_) {
> > -                     const FrameBuffer &fb = buffer->planes();
> > -                     uint32_t *mem = static_cast<uint32_t
> *>(::mmap(nullptr, fb.planes()[0].length,
> > -
> PROT_READ | PROT_WRITE,
> > -
> MAP_SHARED,
> > -
> fb.planes()[0].fd.fd(), 0));
> > -                     mem[0] = ctrl[V4L2_CID_EXPOSURE];
> > -                     mem[1] = ctrl[V4L2_CID_ANALOGUE_GAIN];
> > -                     munmap(mem, fb.planes()[0].length);
>
> Oh, yes, keeping these mappings around rather than mapping and unmapping
> each frame will definitely help.
>
> > +                     unsigned int bufferId =
> unicam_[Unicam::Embedded].getBufferId(buffer);
> > +                     auto it = mappedEmbeddedBuffers_.find(bufferId);
> > +                     if (it != mappedEmbeddedBuffers_.end()) {
> > +                             uint32_t *mem = reinterpret_cast<uint32_t
> *>(it->second.maps()[0].data());
>
> I wonder if we should add a helper to MappedFrameBuffer to return a more
> friendly data type to avoid casts like this, but that's not for this patch.
>
>
> With the piecewise constructs either clarified that you want to keep
> them, or simplified out:
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
>
>
> > +                             mem[0] = ctrl[V4L2_CID_EXPOSURE];
> > +                             mem[1] = ctrl[V4L2_CID_ANALOGUE_GAIN];
> > +                     } else {
> > +                             LOG(RPI, Warning) << "Failed to find
> embedded buffer "
> > +                                               << bufferId;
> > +                     }
> >               }
> >       }
> >
> >
>
> --
> Regards
> --
> Kieran
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20201112/b6473601/attachment-0001.htm>