[libcamera-devel] [PATCH 6/9] libcamera: stream: Add operation to map buffers
Jacopo Mondi
jacopo at jmondi.org
Tue Jul 9 12:57:42 CEST 2019
Hi Niklas,
On Sat, Jul 06, 2019 at 02:00:03PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your patch.
>
> On 2019-07-05 00:53:31 +0200, Jacopo Mondi wrote:
> > Add and operation to map external buffers provided by applications in
> > a Request to the Stream's internal buffers used by the pipeline handlers
> > to interact with the video device.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > include/libcamera/buffer.h | 1 +
> > include/libcamera/stream.h | 6 ++
> > src/libcamera/stream.cpp | 116 +++++++++++++++++++++++++++++++++++++
> > 3 files changed, 123 insertions(+)
> >
> > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> > index 260a62e9e77e..d5d3dc90a096 100644
> > --- a/include/libcamera/buffer.h
> > +++ b/include/libcamera/buffer.h
> > @@ -59,6 +59,7 @@ private:
> > friend class BufferPool;
> > friend class PipelineHandler;
> > friend class Request;
> > + friend class Stream;
>
> I understand this is needed now so no problem. But for each new friend
> gained now is one more friend we have to try and design away later ;-)
>
> > friend class V4L2VideoDevice;
> >
> > void cancel();
> > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> > index 796f1aff2602..74415062cbdd 100644
> > --- a/include/libcamera/stream.h
> > +++ b/include/libcamera/stream.h
> > @@ -85,6 +85,8 @@ public:
> > const StreamConfiguration &configuration() const { return configuration_; }
> > MemoryType memoryType() const { return memoryType_; }
> >
> > + Buffer *mapBuffer(Buffer *applicationBuffer);
> > +
> > protected:
> > friend class Camera;
> >
> > @@ -94,6 +96,10 @@ protected:
> > BufferPool bufferPool_;
> > StreamConfiguration configuration_;
> > MemoryType memoryType_;
> > +
> > +private:
> > + std::vector<Buffer *> mappableBuffers_;
> > + std::map<unsigned int, Buffer *> bufferMap_;
> > };
> >
> > } /* namespace libcamera */
> > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> > index 97e0f429c9fb..4585c0db77a4 100644
> > --- a/src/libcamera/stream.cpp
> > +++ b/src/libcamera/stream.cpp
> > @@ -13,6 +13,8 @@
> > #include <iomanip>
> > #include <sstream>
> >
> > +#include <libcamera/request.h>
> > +
> > #include "log.h"
> >
> > /**
> > @@ -470,6 +472,26 @@ void Stream::createBuffers(unsigned int count)
> > return;
> >
> > bufferPool_.createBuffers(count);
> > +
> > + /* Streams with internal memory usage do not need buffer mapping. */
> > + if (memoryType_ == InternalMemory)
> > + return;
> > +
> > + /*
> > + * Prepare for buffer mapping by queuing all buffers from the internal
> > + * pool. Each external buffer presented by application will be mapped
> > + * on an internal one.
> > + */
> > + mappableBuffers_.clear();
> > + for (Buffer &buffer : bufferPool_.buffers()) {
> > + /*
> > + * Reserve all planes to support mapping multiplanar buffers.
> > + * \todo I would use VIDEO_MAX_PLANES but it's a V4L2 thing.
> > + */
> > + buffer.planes().resize(3);
> > +
> > + mappableBuffers_.push_back(&buffer);
> > + }
> > }
> >
> > /**
> > @@ -480,6 +502,100 @@ void Stream::destroyBuffers()
> > bufferPool_.destroyBuffers();
> > }
> >
> > +/**
> > + * \brief Map an application provided buffer to a stream buffer
> > + * \param applicationBuffer The application provided buffer
> > + *
> > + * If a Stream has been configured to use application provided buffers a
> > + * mapping between the external buffers and the internal ones, which are
> > + * actually used to interface with the video device, is required.
> > + *
> > + * The most commonly used mechanism to perform zero-copy memory sharing
> > + * on Linux-based system is dmabuf, which allows user-space applications to
> > + * share buffers by exchanging dmabuf generated file descriptors. This
> > + * operations assumes that all application provided buffers have each of their
> > + * used memory planes exported as dmabuf file descriptor, to copy them in
> > + * the buffer to be then queued on the video device by pipeline handlers.
> > + *
> > + * Perform mapping by maintaining a cache in a map associating the dmabuf file
> > + * descriptor of the application provided buffer to one of the stream's internal
> > + * buffers to provide pipeline handlers the buffer to use to interact with video
> > + * devices.
> > + *
> > + * Once the buffer completes, the mapping should be reverted to return to
> > + * the application the buffer it first provided here.
> > + *
> > + * \return The stream buffer which maps to the application provided buffer
> > + */
> > +Buffer *Stream::mapBuffer(Buffer *applicationBuffer)
> > +{
> > + unsigned int key = applicationBuffer->planes()[0].dmabuf();
> > +
> > + /*
> > + * Buffer hit: the application buffer has already been mapped, return
> > + * the assigned stream buffer
> > + */
> > + auto mapped = bufferMap_.find(key);
> > + if (mapped != bufferMap_.end()) {
> > + Buffer *buffer = mapped->second;
> > +
> > + /*
> > + * Keep the mappableBuffers_ vector warm: move the hit buffer to
> > + * the vector end as on a buffer miss buffers are below evicted
> > + * from the vector head.
> > + */
> > + auto it = std::find(mappableBuffers_.begin(),
> > + mappableBuffers_.end(), buffer);
> > +
> > + ASSERT(it != mappableBuffers_.end());
> > + std::rotate(it, it + 1, mappableBuffers_.end());
>
> This don't seem to be right.
>
> The it found will indeed be pushed to the back, but the buffer which was
> at the end will be swapped to the location it as at. Is not correct
> behavior here to delete the it position and insert it at the end?
Note that the iterator returned by end() point to the past-the-end
position of the vector
https://en.cppreference.com/w/cpp/container/vector/end
So swapping an element with it, it's safe.
To make sure, you can run this very simple test program too:
https://paste.debian.net/1090903/
Whiche gives you:
1234
Moving 2 to vector end
1342
As you can see, 2 does not get swapped with 4, but moved after it
instead.
>
> I assume the idea is to the closer to end() a buffer is, the "warmer" it
> is. So this swapping could lead to non optimal performance.
It's the other way around actually. Removing and re-inserting causes
the vector to be relocated, and it should be avoided as much as
possible.
>
> > +
> > + return buffer;
> > + }
> > +
> > + /*
> > + * Buffer miss: assign to the application buffer the stream buffer
> > + * at mappableBuffers_ begin, then move it to the end.
> > + */
> > + Buffer *buffer = *(mappableBuffers_.begin());
> > + std::rotate(mappableBuffers_.begin(), mappableBuffers_.begin() + 1,
> > + mappableBuffers_.end());
> > +
> > +
> > + /* Remove the [key, buffer] entry buffer from the buffer map */
> > + auto deadBuf = std::find_if(bufferMap_.begin(), bufferMap_.end(),
> > + [&](std::pair<const unsigned int, Buffer *> &map) {
> > + return bufferMap_[map.first] == buffer;
> > + });
> > + if (deadBuf != bufferMap_.end())
> > + bufferMap_.erase(deadBuf);
> > +
> > + /*
> > + * Assign the buffer by copying the dmabuf file descriptors from the
> > + * application provided buffer.
> > + */
> > + for (unsigned int i = 0; i < applicationBuffer->planes().size(); ++i) {
> > + int fd = applicationBuffer->planes()[i].dmabuf();
> > +
> > + /*
> > + * The ARC camera stack seems to report more planes that the
>
> s/that/then/
>
> > + * ones it actually uses.
>
> s/it//
>
> > + */
> > + if (fd < 0)
> > + break;
> > +
> > + buffer->planes()[i].setDmabuf(fd, 0);
> > + }
> > +
> > + /* Pipeline handlers use request_ at buffer completion time. */
> > + buffer->request_ = applicationBuffer->request();
> > +
> > + /* And finally, store the mapping for later re-use and return it. */
> > + bufferMap_[key] = buffer;
> > +
> > + return buffer;
> > +}
> > +
> > /**
> > * \var Stream::bufferPool_
> > * \brief The pool of buffers associated with the stream
> > --
> > 2.21.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190709/c34064b2/attachment.sig>
More information about the libcamera-devel
mailing list