[libcamera-devel] [PATCH v3 10/10] libcamera: pipeline: raspberrypi: Handle any externally allocated FrameBuffer

[Niklas Söderlund]

Hi Naushir,

Thanks for your work.

On 2020-07-17 09:54:10 +0100, Naushir Patuck wrote:
> Handle the case where a FrameBuffer that has been externally allocated
> (i.e. not through the v4l2 video device) is passed into a Request.
> 
> We must store the buffer pointer in our internal buffer list to identify
> when used, as well as mmap the buffer in the IPA if needed.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 94 +++++++++++--------
>  .../pipeline/raspberrypi/rpi_stream.cpp       |  5 +
>  .../pipeline/raspberrypi/rpi_stream.h         |  1 +
>  3 files changed, 63 insertions(+), 37 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index e400c10c..99896eee 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -171,8 +171,8 @@ public:
>  	RPi::RPiDevice<Isp, 4> isp_;
>  	/* The vector below is just for convenience when iterating over all streams. */
>  	std::vector<RPi::RPiStream *> streams_;
> -	/* Buffers passed to the IPA. */
> -	std::vector<IPABuffer> ipaBuffers_;
> +	/* Stores the ids of the buffers mapped in the IPA. */
> +	std::vector<unsigned int> ipaBufferIds_;
>  
>  	/* VCSM allocation helper. */
>  	::RPi::Vcsm vcsm_;
> @@ -192,7 +192,6 @@ public:
>  	std::queue<FrameBuffer *> bayerQueue_;
>  	std::queue<FrameBuffer *> embeddedQueue_;
>  	std::deque<Request *> requestQueue_;
> -
>  	unsigned int dropFrameCount_;
>  
>  private:
> @@ -243,6 +242,8 @@ private:
>  	int queueAllBuffers(Camera *camera);
>  	int prepareBuffers(Camera *camera);
>  	void freeBuffers(Camera *camera);
> +	void mapBuffers(Camera *camera, const std::vector<FrameBuffer *> &buffers,
> +			unsigned int startId);
>  
>  	MediaDevice *unicam_;
>  	MediaDevice *isp_;
> @@ -736,20 +737,38 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)
>  
>  	/* Push all buffers supplied in the Request to the respective streams. */
>  	for (auto stream : data->streams_) {
> -		if (stream->isExternal()) {
> -			FrameBuffer *buffer = request->findBuffer(stream);
> +		if (!stream->isExternal())
> +			continue;
> +
> +		FrameBuffer *buffer = request->findBuffer(stream);
> +		if (buffer && stream->getBufferIndex(buffer) == -1) {
>  			/*
> -			 * A nullptr in buffer means that we should queue an internally
> -			 * allocated buffer.
> -			 *
> -			 * The below queueBuffer() call will do nothing if there are not
> -			 * enough internal buffers allocated, but this will be handled by
> -			 * queuing the request for buffers in the RPiStream object.
> +			 * This buffer is not recognised, so it must have been allocated
> +			 * outside the v4l2 device. Store it in the stream buffer list
> +			 * so we can track it.
>  			 */
> -			int ret = stream->queueBuffer(buffer);
> -			if (ret)
> -				return ret;
> +			stream->setExternalBuffer(buffer);

There is noting wrong tracking external buffers in this way but I feel 
it's ill prepared for the scenario where a new external buffer is used 
each time. As the tracking is not undone when foreign buffer buffer 
leaves the pipeline handler, perhaps never to be seen again.

> +
> +			/* Also get the IPA to mmap it if needed. */
> +			if (stream == &data->unicam_[Unicam::Embedded]) {
> +				mapBuffers(camera, { buffer },
> +					   RPiIpaMask::EMBEDDED_DATA | (stream->getBuffers().size() - 1));
> +			} else if (stream == &data->isp_[Isp::Stats]) {
> +				mapBuffers(camera, { buffer },
> +					   RPiIpaMask::STATS | (stream->getBuffers().size() - 1));
> +			}

Same argument here as above but with a hight cost. What if the buffers 
mapped here are never reused and a new buffer is attached to each new 
request? Maybe some form of time-to-live would be a middle ground for 
efficiency between always mapping a foreign buffer when it enters the 
pipeline and unmap it when it exits?

>  		}
> +		/*
> +		 * A nullptr in buffer means that we should queue an internally
> +		 * allocated buffer.
> +		 *
> +		 * The below queueBuffer() call will do nothing if there are not
> +		 * enough internal buffers allocated, but this will be handled by
> +		 * queuing the request for buffers in the RPiStream object.
> +		 */
> +		int ret = stream->queueBuffer(buffer);
> +		if (ret)
> +			return ret;
>  	}
>  
>  	/* Push the request to the back of the queue. */
> @@ -888,7 +907,6 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
>  int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  {
>  	RPiCameraData *data = cameraData(camera);
> -	unsigned int index;
>  	int ret;
>  
>  	/*
> @@ -908,42 +926,44 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  			return ret;
>  	}
>  
> +	/*
> +	 * Pass the stats and embedded data buffers to the IPA. No other
> +	 * buffers need to be passed.
> +	 */
> +	mapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), RPiIpaMask::STATS);
> +	mapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), RPiIpaMask::EMBEDDED_DATA);
> +
> +	return 0;
> +}
> +
> +void PipelineHandlerRPi::mapBuffers(Camera *camera, const std::vector<FrameBuffer *> &buffers,
> +				    unsigned int startId)
> +{
> +	RPiCameraData *data = cameraData(camera);
> +	std::vector<IPABuffer> ipaBuffers;
> +	unsigned int id = startId;
>  	/*
>  	 * Link the FrameBuffers with the index of their position in the vector
> -	 * stored in the RPi stream object.
> +	 * stored in the RPi stream object - along with an identifer mask.
>  	 *
>  	 * This will allow us to identify buffers passed between the pipeline
>  	 * handler and the IPA.
>  	 */
> -	index = 0;
> -	for (auto const &b : data->isp_[Isp::Stats].getBuffers()) {
> -		data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | index,
> -					      .planes = b->planes() });
> -		index++;
> -	}
> -
> -	index = 0;
> -	for (auto const &b : data->unicam_[Unicam::Embedded].getBuffers()) {
> -		data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | index,
> -					      .planes = b->planes() });
> -		index++;
> +	for (auto const &b : buffers) {
> +		ipaBuffers.push_back({ .id = id, .planes = b->planes() });
> +		data->ipaBufferIds_.push_back(id);
> +		id++;
>  	}
>  
> -	data->ipa_->mapBuffers(data->ipaBuffers_);
> -
> -	return 0;
> +	data->ipa_->mapBuffers(ipaBuffers);
>  }
>  
>  void PipelineHandlerRPi::freeBuffers(Camera *camera)
>  {
>  	RPiCameraData *data = cameraData(camera);
>  
> -	std::vector<unsigned int> ids;
> -	for (IPABuffer &ipabuf : data->ipaBuffers_)
> -		ids.push_back(ipabuf.id);
> -
> -	data->ipa_->unmapBuffers(ids);
> -	data->ipaBuffers_.clear();
> +	data->ipa_->unmapBuffers(data->ipaBufferIds_);
> +	data->ipaBufferIds_.clear();
>  
>  	for (auto const stream : data->streams_)
>  		stream->releaseBuffers();
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> index 61226e94..e4158e3a 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> @@ -48,6 +48,11 @@ void RPiStream::setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *bu
>  		       [](std::unique_ptr<FrameBuffer> &b) { return b.get(); });
>  }
>  
> +void RPiStream::setExternalBuffer(FrameBuffer *buffer)
> +{
> +	bufferList_.push_back(buffer);
> +}
> +
>  const std::vector<FrameBuffer *> &RPiStream::getBuffers() const
>  {
>  	return bufferList_;
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> index a6fd5c8e..e2534a32 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> @@ -41,6 +41,7 @@ public:
>  	void reset();
>  	std::string name() const;
>  	void setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> +	void setExternalBuffer(FrameBuffer *buffer);
>  	const std::vector<FrameBuffer *> &getBuffers() const;
>  	void releaseBuffers();
>  	int prepareBuffers(unsigned int count);
> -- 
> 2.25.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund