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

[Naushir Patuck]

Hi Laurent,

Thank you for your comments.  I will apply your suggestions for both
patches in this series and submit a v3 to merge.

Regards,
Naush


On Mon, 16 Nov 2020 at 12:28, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Naush,
>
> Thank you for the patch.
>
> On Mon, Nov 16, 2020 at 11:24:57AM +0000, 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>
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 33 ++++++++++++++-----
> >  1 file changed, 25 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index dd62dfc7..faa06c00 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,13 @@ int PipelineHandlerRPi::prepareBuffers(Camera
> *camera)
> >                       return ret;
> >       }
> >
> > +     if (!data->sensorMetadata_) {
> > +             for (auto const &it :
> data->unicam_[Unicam::Embedded].getBuffers()) {
> > +                     data->mappedEmbeddedBuffers_.emplace(it.first,
> > +
> MappedFrameBuffer(it.second, PROT_READ | PROT_WRITE));
>
> That's a fairly long line, could we write
>
>                         MappedFrameBuffer fb(it.second, PROT_READ |
> PROT_WRITE);
>                         data->mappedEmbeddedBuffers_.emplace(it.first,
> std::move(fb));
>
> Alternatively, if you want to avoid the move construction of the
> element, you could write
>
>
> data->mappedEmbeddedBuffers_.emplace(std::piecewise_construct,
>
>  std::forward_as_tuple(it.first),
>
>  std::forward_as_tuple(it.second,
>
>          PROT_READ | PROT_WRITE));
>
> > +             }
> > +     }
> > +
> >       /*
> >        * Pass the stats and embedded data buffers to the IPA. No other
> >        * buffers need to be passed.
> > @@ -1078,6 +1092,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 +1325,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);
> > +                     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());
> > +                             mem[0] = ctrl[V4L2_CID_EXPOSURE];
> > +                             mem[1] = ctrl[V4L2_CID_ANALOGUE_GAIN];
> > +                     } else {
> > +                             LOG(RPI, Warning) << "Failed to find
> embedded buffer "
> > +                                               << bufferId;
> > +                     }
>
> Out of scope for this series, but I wonder if we shouldn't improve this
> with a cleaner API to the IPA instead of faking an embedded data buffer.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> >               }
> >       }
> >
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20201116/b99f7b0e/attachment-0001.htm>