[libcamera-devel] [PATCH 1/2] pipeline: raspberrypi: Use MappedFrameBuffer for embedded data buffers
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Nov 12 23:58:27 CET 2020
Hi Naush,
On 12/11/2020 22:47, Naushir Patuck wrote:
> Hi Kieran,
>
>
> On Thu, 12 Nov 2020 at 22:16, Kieran Bingham
> <kieran.bingham at ideasonboard.com
> <mailto: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
> <mailto: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 :-)
>
piecewise requirements stretched mine a bit too. Back in January Laurent
did some code simplification around this, and I didn't really understand
what it was doing then so I (while sat next to him, quite the novelty)
got him to write a long commit message explaining it more..
Not sure if it helps or not, but this is the relevant commit message there:
https://git.libcamera.org/libcamera/libcamera.git/commit/?id=38dd90307ab2b0d25a0a233eae04455f769153b4
> 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
I think by doing the emplace we're already heading to that efficiency.
The piecewise is needed when the thing being emplaced is more complex or
when one of the things being emplaced needs more than one argument.
> 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.
I think
emplace(std::piecewise_construct,
std::forward_as_tuple(it.first),
std::forward_as_tuple(it.second, PROT_READ | PROT_WRITE)));
would ensure the performance gain, but I would fear:
emplace(std::piecewise_construct,
std::forward_as_tuple(it.first),
std::forward_as_tuple(MappedFrameBuffer(it.second, PROT_READ
| PROT_WRITE))));
would be invoking the MappedFrameBuffer constructor explicitly, forcing
a copy or a move. But I'm not really sure what the compiler does here
without going through and putting some prints in to see what gets invoked.
As this construct is only at initialise I'm not worried by that, but I
don't think the MappedFrameBuffers are copyable anyway, as that would
lead to two objects that could unmap the same memory.
I assume I handled that all in the MappedFrameBuffer class though ;-)
Well - on the topic of not losing any sleep over this - I'm going to go
and try to find some! hehe
--
Kieran
>
>
> 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
> <mailto: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
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list