[libcamera-devel] [PATCH 6/9] libcamera: stream: Add operation to map buffers
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Jul 8 13:39:31 CEST 2019
Hi Jacopo,
On 06/07/2019 13:00, 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 ;-)
Can we handle any mapping/LRU requirements in the bufferPool class instead?
The Bufferpool could have a map instead of the stream? which then
removes the friend requirements ...
>
>> 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?
>
> 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.
>
>> +
>> + 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/
s/s/that/then//s/that/than// :D ok that's unreadable:
How about : s/that/than/
:D
>
>> + * ones it actually uses.
>
> s/it//
the 'it' pronoun is needed, so it should stay.
>> + */
>> + 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
--
Kieran
More information about the libcamera-devel
mailing list