[libcamera-devel] [PATCH v3 30/33] cam: Cache buffer memory mapping

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Fri, Jan 10, 2020 at 08:38:05PM +0100, Niklas Söderlund wrote:
> With the buffer allocator in use it's possible to cache the dmabuf
> memory mappings when starting the camera instead of mapping and
> unmapping them each time.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/cam/buffer_writer.cpp | 27 ++++++++++++++++++++++-----
>  src/cam/buffer_writer.h   |  5 +++++
>  src/cam/capture.cpp       |  3 +++
>  3 files changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp
> index 1d7366c87714cd91..c5a5eb46224ac3eb 100644
> --- a/src/cam/buffer_writer.cpp
> +++ b/src/cam/buffer_writer.cpp
> @@ -22,6 +22,27 @@ BufferWriter::BufferWriter(const std::string &pattern)
>  {
>  }
>  
> +BufferWriter::~BufferWriter()
> +{
> +	for (auto &iter : mappedBuffers_) {
> +		void *memory = iter.second.first;
> +		unsigned int length = iter.second.second;

Usage of std::pair<> makes the code a bit cryptic :-S I'm tempted to
propose a refactoring, but we already know we need to implement a buffer
mmap helper, so this code will go away.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +		munmap(memory, length);
> +	}
> +	mappedBuffers_.clear();
> +}
> +
> +void BufferWriter::mapBuffer(FrameBuffer *buffer)
> +{
> +	for (const FrameBuffer::Plane &plane : buffer->planes()) {
> +		void *memory = mmap(NULL, plane.length, PROT_READ, MAP_SHARED,
> +				    plane.fd.fd(), 0);
> +
> +		mappedBuffers_[plane.fd.fd()] =
> +			std::make_pair(memory, plane.length);
> +	}
> +}
> +
>  int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)
>  {
>  	std::string filename;
> @@ -44,9 +65,7 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)
>  		return -errno;
>  
>  	for (const FrameBuffer::Plane &plane : buffer->planes()) {
> -		/* \todo Once the FrameBuffer is done cache mapped memory. */
> -		void *data = mmap(NULL, plane.length, PROT_READ, MAP_SHARED,
> -				  plane.fd.fd(), 0);
> +		void *data = mappedBuffers_[plane.fd.fd()].first;
>  		unsigned int length = plane.length;
>  
>  		ret = ::write(fd, data, length);
> @@ -61,8 +80,6 @@ int BufferWriter::write(FrameBuffer *buffer, const std::string &streamName)
>  				  << length << std::endl;
>  			break;
>  		}
> -
> -		munmap(data, length);
>  	}
>  
>  	close(fd);
> diff --git a/src/cam/buffer_writer.h b/src/cam/buffer_writer.h
> index 5917a7dfb5e28106..8c9b2436fdae4fc3 100644
> --- a/src/cam/buffer_writer.h
> +++ b/src/cam/buffer_writer.h
> @@ -7,6 +7,7 @@
>  #ifndef __LIBCAMERA_BUFFER_WRITER_H__
>  #define __LIBCAMERA_BUFFER_WRITER_H__
>  
> +#include <map>
>  #include <string>
>  
>  #include <libcamera/buffer.h>
> @@ -15,12 +16,16 @@ class BufferWriter
>  {
>  public:
>  	BufferWriter(const std::string &pattern = "frame-#.bin");
> +	~BufferWriter();
> +
> +	void mapBuffer(libcamera::FrameBuffer *buffer);
>  
>  	int write(libcamera::FrameBuffer *buffer,
>  		  const std::string &streamName);
>  
>  private:
>  	std::string pattern_;
> +	std::map<int, std::pair<void *, unsigned int>> mappedBuffers_;
>  };
>  
>  #endif /* __LIBCAMERA_BUFFER_WRITER_H__ */
> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> index dd078eb0ae4a2c62..738fa1c267eb6e36 100644
> --- a/src/cam/capture.cpp
> +++ b/src/cam/capture.cpp
> @@ -116,6 +116,9 @@ int Capture::capture(EventLoop *loop, FrameBufferAllocator *allocator)
>  					  << std::endl;
>  				return ret;
>  			}
> +
> +			if (writer_)
> +				writer_->mapBuffer(buffer.get());
>  		}
>  
>  		requests.push_back(request);

-- 
Regards,

Laurent Pinchart