[libcamera-devel] [RFC 6/8] libcamera: stream: Add operation to map buffers

[Jacopo Mondi]

Hi Niklas,

On Tue, Jul 02, 2019 at 01:07:30AM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> On 2019-06-30 20:10:47 +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.
> >
> > For streams using internal memory allocation, the two buffers are the
> > same as applications effectively use Buffers from the Stream's pool
> > where the video device memory has been exported to.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  include/libcamera/buffer.h |  1 +
> >  include/libcamera/stream.h |  5 +++
> >  src/libcamera/stream.cpp   | 66 ++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 72 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;
> >  	friend class V4L2VideoDevice;
> >
> >  	void cancel();
> > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> > index 796f1aff2602..4c034c113ddb 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 *requestBuffer);
> > +
> >  protected:
> >  	friend class Camera;
> >
> > @@ -94,6 +96,9 @@ protected:
> >  	BufferPool bufferPool_;
> >  	StreamConfiguration configuration_;
> >  	MemoryType memoryType_;
> > +
> > +	std::vector<Buffer *> mappableBuffers_;
> > +	std::map<unsigned int, Buffer *> bufferMaps_;
> >  };
> >
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> > index b6292427d3a2..f36336857ad6 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"
> >
> >  /**
> > @@ -445,6 +447,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. */
> > +		buffer.planes().clear();
> > +		/* \todo: I would use VIDEO_MAX_PLANES but that's V4L2 stuff.. */
> > +		for (unsigned int i = 0; i < 3; ++i)
> > +			buffer.planes().emplace_back();
> > +
> > +		mappableBuffers_.push_back(&buffer);
> > +	}
> >  }
> >
> >  /**
> > @@ -457,6 +479,50 @@ void Stream::destroyBuffers()
> >  	createBuffers(0);
> >  }
> >
> > +/**
> > + * \brief Map the buffer an application has associated with a Request to an
> > + * internl one
> > + *
> > + * \todo Rewrite documentation
> > + * If the Stream uses external memory, we need to map the externally
> > + * provided buffer to an internal one, trying to keep a best effort
> > + * association based on the Buffer's last usage time.
>
> I would not use the word last here as it (at lest to me) implies that
> the buffer selected is the most recently used buffer and not the oldest
> buffer.
>
> > + * External and internal buffers are associated by using the dmabuf
> > + * fds as key.
> > + */
> > +Buffer *Stream::mapBuffer(Buffer *requestBuffer)
> > +{
> > +	/*
> > +	 * \todo Multiplane APIs have one fd per plane, the key should be
> > +	 * hashed using all the planes fds.
> > +	 */
> > +	unsigned int key = requestBuffer->planes()[0].dmabuf();
> > +
> > +	/* If the buffer has already been mapped, just return it. */
> > +	auto mapped = bufferMaps_.find(key);
> > +	if (mapped != bufferMaps_.end())
> > +		return mapped->second;
>
> On a hit should you not update the buffers position in mappableBuffers_
> to increase the hit rate and try to keep the cache as hot as possible?

You are right!

>
> One idea is to make mappableBuffers_ a std::set and index it on a
> timestmap property, that way you could just update the timestamp when a
> buffer is resued and when you need to evict a buffer from the list the
> oldes buffer will always be at the front (or end) of the set.

Or I could remove the buffer from the vector of used buffers and push it back
again. Your one is probably more elegant, but I do expect a very
limited number of buffers to cycle.
>
> > +
> > +	/*
> > +	 * Remove the last recently used buffer from the circular list and
> > +	 * use it for mapping.
> > +	 */
> > +	auto mappable = mappableBuffers_.begin();
> > +	Buffer *buffer = *mappable;
> > +	mappableBuffers_.erase(mappable);
> > +	mappableBuffers_.push_back(buffer);
>
> It's simpler to rotate the vector,
>
>   std::rotate(mappableBuffers_.begin(), mappableBuffers_.begin() + 1, mappableBuffers_.end());
>

Thanks! Also, if you have better names to suggest in place of
mappableBuffers_ I would be happy to hear that.

> > +
> > +	/* \todo: Support multiplanar external buffers. */
> > +	buffer->planes()[0].setDmabuf(key, 0);
> > +
> > +	/* Pipeline handlers use request_ at buffer completion time. */
> > +	buffer->request_ = requestBuffer->request();
> > +
> > +	bufferMaps_[key] = buffer;
>
> I'm no expert on our buffer handling, but when you evict a buffer from
> mappableBuffers_ should it not also be removed from bufferMaps_? Else if
> every buffer imported is a new dmabuf we will consume a lot of memory.
>

True, the map could grow if we keep receiving new buffers. Again, I
expect application to cycle on a limited number of buffers so I was
not too concerned, but yes, when a a buffer mapping is removed, the
entry on the map corresponding to that buffer key should be erased.

Thanks for spotting these!

> > +
> > +	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/20190702/fc78b15f/attachment.sig>