[libcamera-devel] [PATCH 1/2] pipeline: raspberrypi: Use MappedFrameBuffer for embedded data buffers

[Kieran Bingham]

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>
> ---
>  .../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?

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)));

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>



> +				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