[libcamera-devel,v7,12/12] pipeline: ipa: raspberrypi: Handle any externally allocated FrameBuffer

9528 diff mbox series

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 the stream internal buffer list to
identify when used.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 include/libcamera/ipa/raspberrypi.h           | 11 ++--
 src/ipa/raspberrypi/raspberrypi.cpp           |  6 +-
 .../pipeline/raspberrypi/raspberrypi.cpp      | 64 +++++++++++++------
 .../pipeline/raspberrypi/rpi_stream.cpp       | 30 ++++++++-
 .../pipeline/raspberrypi/rpi_stream.h         |  3 +
 5 files changed, 85 insertions(+), 29 deletions(-)

Hi Naushir,

Thanks for your patch.

On 2020-09-08 08:49:12 +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 the stream internal buffer list to
> identify when used.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  include/libcamera/ipa/raspberrypi.h           | 11 ++--
>  src/ipa/raspberrypi/raspberrypi.cpp           |  6 +-
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 64 +++++++++++++------
>  .../pipeline/raspberrypi/rpi_stream.cpp       | 30 ++++++++-
>  .../pipeline/raspberrypi/rpi_stream.h         |  3 +
>  5 files changed, 85 insertions(+), 29 deletions(-)
> 
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index 262fc6f3..dd6ebeac 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -28,11 +28,12 @@ enum RPiOperations {
>  	RPI_IPA_EVENT_QUEUE_REQUEST,
>  };
>  
> -enum RPiIpaMask {
> -	ID		= 0x0ffff,
> -	STATS		= 0x10000,
> -	EMBEDDED_DATA	= 0x20000,
> -	BAYER_DATA	= 0x40000
> +enum RPiBufferMask {
> +	ID		= 0x00ffff,
> +	STATS		= 0x010000,
> +	EMBEDDED_DATA	= 0x020000,
> +	BAYER_DATA	= 0x040000,
> +	EXTERNAL_BUFFER	= 0x100000,
>  };
>  
>  /* Size of the LS grid allocation. */
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 688d2efc..0555cc4e 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -352,7 +352,7 @@ void IPARPi::processEvent(const IPAOperationData &event)
>  
>  		IPAOperationData op;
>  		op.operation = RPI_IPA_ACTION_STATS_METADATA_COMPLETE;
> -		op.data = { bufferId & RPiIpaMask::ID };
> +		op.data = { bufferId & RPiBufferMask::ID };
>  		op.controls = { libcameraMetadata_ };
>  		queueFrameAction.emit(0, op);
>  		break;
> @@ -373,7 +373,7 @@ void IPARPi::processEvent(const IPAOperationData &event)
>  		/* Ready to push the input buffer into the ISP. */
>  		IPAOperationData op;
>  		op.operation = RPI_IPA_ACTION_RUN_ISP;
> -		op.data = { bayerbufferId & RPiIpaMask::ID };
> +		op.data = { bayerbufferId & RPiBufferMask::ID };
>  		queueFrameAction.emit(0, op);
>  		break;
>  	}
> @@ -700,7 +700,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)
>  {
>  	IPAOperationData op;
>  	op.operation = RPI_IPA_ACTION_EMBEDDED_COMPLETE;
> -	op.data = { bufferId & RPiIpaMask::ID };
> +	op.data = { bufferId & RPiBufferMask::ID };
>  	queueFrameAction.emit(0, op);
>  }
>  
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 5621f182..f5e0d1cd 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -151,6 +151,7 @@ public:
>  
>  	void clearIncompleteRequests();
>  	void handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stream);
> +	void handleExternalBuffer(FrameBuffer *buffer, RPi::RPiStream *stream);
>  	void handleState();
>  
>  	CameraSensor *sensor_;
> @@ -725,23 +726,32 @@ 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->getBufferId(buffer) == -1) {
>  			/*
> -			 * If no buffer is provided by the request for this stream, we
> -			 * queue a nullptr to the stream to signify that it must use an
> -			 * internally allocated buffer for this capture request. This
> -			 * buffer will not be given back to the application, but is used
> -			 * to support the internal pipeline flow.
> -			 *
> -			 * 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);
>  		}
> +		/*
> +		 * If no buffer is provided by the request for this stream, we
> +		 * queue a nullptr to the stream to signify that it must use an
> +		 * internally allocated buffer for this capture request. This
> +		 * buffer will not be given back to the application, but is used
> +		 * to support the internal pipeline flow.
> +		 *
> +		 * 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. */
> @@ -915,8 +925,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  	 * 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);
> +	mapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), RPiBufferMask::STATS);
> +	mapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), RPiBufferMask::EMBEDDED_DATA);
>  
>  	return 0;
>  }
> @@ -1219,7 +1229,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
>  	if (stream == &isp_[Isp::Stats]) {
>  		IPAOperationData op;
>  		op.operation = RPI_IPA_EVENT_SIGNAL_STAT_READY;
> -		op.data = { RPiIpaMask::STATS | static_cast<unsigned int>(index) };
> +		op.data = { RPiBufferMask::STATS | static_cast<unsigned int>(index) };
>  		ipa_->processEvent(op);
>  	} else {
>  		/* Any other ISP output can be handed back to the application now. */
> @@ -1293,6 +1303,11 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stre
>  		Request *request = requestQueue_.front();
>  
>  		if (!dropFrameCount_ && request->findBuffer(stream) == buffer) {
> +			/*
> +			 * Check if this is an externally provided buffer, and if
> +			 * so, we must stop tracking it in the pipeline handler.
> +			 */
> +			handleExternalBuffer(buffer, stream);
>  			/*
>  			 * Tag the buffer as completed, returning it to the
>  			 * application.
> @@ -1311,6 +1326,17 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stre
>  	}
>  }
>  
> +void RPiCameraData::handleExternalBuffer(FrameBuffer *buffer, RPi::RPiStream *stream)
> +{
> +	unsigned int id = stream->getBufferId(buffer);
> +
> +	if (!(id & RPiBufferMask::EXTERNAL_BUFFER))
> +		return;
> +
> +	/* Stop the Stream object from tracking the buffer. */
> +	stream->removeExternalBuffer(buffer);
> +}
> +
>  void RPiCameraData::handleState()
>  {
>  	switch (state_) {
> @@ -1443,8 +1469,8 @@ void RPiCameraData::tryRunPipeline()
>  			<< " Embedded buffer id: " << embeddedIndex;
>  
>  	op.operation = RPI_IPA_EVENT_SIGNAL_ISP_PREPARE;
> -	op.data = { RPiIpaMask::EMBEDDED_DATA | embeddedIndex,
> -		    RPiIpaMask::BAYER_DATA | bayerIndex };
> +	op.data = { RPiBufferMask::EMBEDDED_DATA | embeddedIndex,
> +		    RPiBufferMask::BAYER_DATA | bayerIndex };
>  	ipa_->processEvent(op);
>  }
>  
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> index 879e25ba..c09f14c9 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> @@ -6,6 +6,8 @@
>   */
>  #include "rpi_stream.h"
>  
> +#include <libcamera/ipa/raspberrypi.h>
> +
>  #include "libcamera/internal/log.h"
>  
>  namespace libcamera {
> @@ -44,8 +46,13 @@ bool RPiStream::isExternal() const
>  
>  void RPiStream::setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>  {
> -	for (auto const &buffer : *buffers)
> -		bufferMap_.emplace(id_++, buffer.get());
> +	/* Ensure we are using a sensible number of buffers. */
> +	ASSERT(id_ < RPiBufferMask::ID);
> +
> +	for (auto const &buffer : *buffers) {
> +		bufferMap_.emplace(id_ & RPiBufferMask::ID, buffer.get());
> +		id_++;
> +	}
>  }
>  
>  const BufferMap &RPiStream::getBuffers() const
> @@ -68,6 +75,25 @@ int RPiStream::getBufferId(FrameBuffer *buffer) const
>  	return it->first;
>  }
>  
> +void RPiStream::setExternalBuffer(FrameBuffer *buffer)
> +{
> +	/* Ensure we are using a sensible number of buffers. */
> +	ASSERT(id_ < RPiBufferMask::ID);
> +
> +	bufferMap_.emplace(RPiBufferMask::EXTERNAL_BUFFER | (id_ & RPiBufferMask::ID),
> +			   buffer);
> +	id_++;
> +}
> +
> +void RPiStream::removeExternalBuffer(FrameBuffer *buffer)
> +{
> +	int id = getBufferId(buffer);
> +
> +	/* Ensure we have this buffer in the stream, and it is marked external. */
> +	ASSERT(id != -1 && (id & RPiBufferMask::EXTERNAL_BUFFER));
> +	bufferMap_.erase(id);
> +}
> +
>  int RPiStream::prepareBuffers(unsigned int count)
>  {
>  	int ret;
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> index ed517c22..8b23c4b2 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> @@ -51,6 +51,9 @@ public:
>  	const BufferMap &getBuffers() const;
>  	int getBufferId(FrameBuffer *buffer) const;
>  
> +	void setExternalBuffer(FrameBuffer *buffer);
> +	void removeExternalBuffer(FrameBuffer *buffer);
> +
>  	int prepareBuffers(unsigned int count);
>  	int queueBuffer(FrameBuffer *buffer);
>  	void returnBuffer(FrameBuffer *buffer);
> -- 
> 2.25.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Naush,

On 08/09/2020 08:49, 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 the stream internal buffer list to
> identify when used.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  include/libcamera/ipa/raspberrypi.h           | 11 ++--
>  src/ipa/raspberrypi/raspberrypi.cpp           |  6 +-
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 64 +++++++++++++------
>  .../pipeline/raspberrypi/rpi_stream.cpp       | 30 ++++++++-
>  .../pipeline/raspberrypi/rpi_stream.h         |  3 +
>  5 files changed, 85 insertions(+), 29 deletions(-)
> 
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index 262fc6f3..dd6ebeac 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -28,11 +28,12 @@ enum RPiOperations {
>  	RPI_IPA_EVENT_QUEUE_REQUEST,
>  };
>  
> -enum RPiIpaMask {
> -	ID		= 0x0ffff,
> -	STATS		= 0x10000,
> -	EMBEDDED_DATA	= 0x20000,
> -	BAYER_DATA	= 0x40000
> +enum RPiBufferMask {
> +	ID		= 0x00ffff,
> +	STATS		= 0x010000,
> +	EMBEDDED_DATA	= 0x020000,
> +	BAYER_DATA	= 0x040000,
> +	EXTERNAL_BUFFER	= 0x100000,
>  };
>  
>  /* Size of the LS grid allocation. */
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 688d2efc..0555cc4e 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -352,7 +352,7 @@ void IPARPi::processEvent(const IPAOperationData &event)
>  
>  		IPAOperationData op;
>  		op.operation = RPI_IPA_ACTION_STATS_METADATA_COMPLETE;
> -		op.data = { bufferId & RPiIpaMask::ID };
> +		op.data = { bufferId & RPiBufferMask::ID };
>  		op.controls = { libcameraMetadata_ };
>  		queueFrameAction.emit(0, op);
>  		break;
> @@ -373,7 +373,7 @@ void IPARPi::processEvent(const IPAOperationData &event)
>  		/* Ready to push the input buffer into the ISP. */
>  		IPAOperationData op;
>  		op.operation = RPI_IPA_ACTION_RUN_ISP;
> -		op.data = { bayerbufferId & RPiIpaMask::ID };
> +		op.data = { bayerbufferId & RPiBufferMask::ID };
>  		queueFrameAction.emit(0, op);
>  		break;
>  	}
> @@ -700,7 +700,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)
>  {
>  	IPAOperationData op;
>  	op.operation = RPI_IPA_ACTION_EMBEDDED_COMPLETE;
> -	op.data = { bufferId & RPiIpaMask::ID };
> +	op.data = { bufferId & RPiBufferMask::ID };
>  	queueFrameAction.emit(0, op);
>  }
>  
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 5621f182..f5e0d1cd 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -151,6 +151,7 @@ public:
>  
>  	void clearIncompleteRequests();
>  	void handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stream);
> +	void handleExternalBuffer(FrameBuffer *buffer, RPi::RPiStream *stream);
>  	void handleState();
>  
>  	CameraSensor *sensor_;
> @@ -725,23 +726,32 @@ 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->getBufferId(buffer) == -1) {
>  			/*
> -			 * If no buffer is provided by the request for this stream, we
> -			 * queue a nullptr to the stream to signify that it must use an
> -			 * internally allocated buffer for this capture request. This
> -			 * buffer will not be given back to the application, but is used
> -			 * to support the internal pipeline flow.
> -			 *
> -			 * 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);
>  		}
> +		/*
> +		 * If no buffer is provided by the request for this stream, we
> +		 * queue a nullptr to the stream to signify that it must use an
> +		 * internally allocated buffer for this capture request. This
> +		 * buffer will not be given back to the application, but is used
> +		 * to support the internal pipeline flow.
> +		 *
> +		 * 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. */
> @@ -915,8 +925,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  	 * 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);
> +	mapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), RPiBufferMask::STATS);
> +	mapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), RPiBufferMask::EMBEDDED_DATA);
>  
>  	return 0;
>  }
> @@ -1219,7 +1229,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
>  	if (stream == &isp_[Isp::Stats]) {
>  		IPAOperationData op;
>  		op.operation = RPI_IPA_EVENT_SIGNAL_STAT_READY;
> -		op.data = { RPiIpaMask::STATS | static_cast<unsigned int>(index) };
> +		op.data = { RPiBufferMask::STATS | static_cast<unsigned int>(index) };
>  		ipa_->processEvent(op);
>  	} else {
>  		/* Any other ISP output can be handed back to the application now. */
> @@ -1293,6 +1303,11 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stre
>  		Request *request = requestQueue_.front();
>  
>  		if (!dropFrameCount_ && request->findBuffer(stream) == buffer) {
> +			/*
> +			 * Check if this is an externally provided buffer, and if
> +			 * so, we must stop tracking it in the pipeline handler.
> +			 */
> +			handleExternalBuffer(buffer, stream);
>  			/*
>  			 * Tag the buffer as completed, returning it to the
>  			 * application.
> @@ -1311,6 +1326,17 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stre
>  	}
>  }
>  
> +void RPiCameraData::handleExternalBuffer(FrameBuffer *buffer, RPi::RPiStream *stream)
> +{
> +	unsigned int id = stream->getBufferId(buffer);
> +
> +	if (!(id & RPiBufferMask::EXTERNAL_BUFFER))
> +		return;
> +
> +	/* Stop the Stream object from tracking the buffer. */
> +	stream->removeExternalBuffer(buffer);
> +}
> +
>  void RPiCameraData::handleState()
>  {
>  	switch (state_) {
> @@ -1443,8 +1469,8 @@ void RPiCameraData::tryRunPipeline()
>  			<< " Embedded buffer id: " << embeddedIndex;
>  
>  	op.operation = RPI_IPA_EVENT_SIGNAL_ISP_PREPARE;
> -	op.data = { RPiIpaMask::EMBEDDED_DATA | embeddedIndex,
> -		    RPiIpaMask::BAYER_DATA | bayerIndex };
> +	op.data = { RPiBufferMask::EMBEDDED_DATA | embeddedIndex,
> +		    RPiBufferMask::BAYER_DATA | bayerIndex };
>  	ipa_->processEvent(op);
>  }
>  
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> index 879e25ba..c09f14c9 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> @@ -6,6 +6,8 @@
>   */
>  #include "rpi_stream.h"
>  
> +#include <libcamera/ipa/raspberrypi.h>
> +
>  #include "libcamera/internal/log.h"
>  
>  namespace libcamera {
> @@ -44,8 +46,13 @@ bool RPiStream::isExternal() const
>  
>  void RPiStream::setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>  {
> -	for (auto const &buffer : *buffers)
> -		bufferMap_.emplace(id_++, buffer.get());
> +	/* Ensure we are using a sensible number of buffers. */
> +	ASSERT(id_ < RPiBufferMask::ID);
> +
> +	for (auto const &buffer : *buffers) {
> +		bufferMap_.emplace(id_ & RPiBufferMask::ID, buffer.get());
> +		id_++;
> +	}

I half wonder if that assert shouldn't come 'after' the updates to the
bufferMap to catch if the id_ has overflowed after increase.

But maybe this is a tiny corner case and not expected anyway.


>  }
>  
>  const BufferMap &RPiStream::getBuffers() const
> @@ -68,6 +75,25 @@ int RPiStream::getBufferId(FrameBuffer *buffer) const
>  	return it->first;
>  }
>  
> +void RPiStream::setExternalBuffer(FrameBuffer *buffer)
> +{
> +	/* Ensure we are using a sensible number of buffers. */
> +	ASSERT(id_ < RPiBufferMask::ID);
> +
> +	bufferMap_.emplace(RPiBufferMask::EXTERNAL_BUFFER | (id_ & RPiBufferMask::ID),
> +			   buffer);
> +	id_++;

Oh, ... is there no reduction of the id_?

What happens if an application continually provides an external buffer
... won't that constantly increase this id_ with every frame?

Are the ID's re-used? or is it just that the stream doesn't generate a
new ID for each usage of an external buffer (i.e. it really would have
to be more than 65535 entirely different buffers...)

I guess this can be easily validated by setting RPiBufferMask::ID to
0xFF or even 0xF to see if it hits any assertions quickly

As long as this doesn't leak and cause an assertion/overflow after
0xFFFF/RPiBufferMask::ID requests are queued ...

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> +}
> +
> +void RPiStream::removeExternalBuffer(FrameBuffer *buffer)
> +{
> +	int id = getBufferId(buffer);
> +
> +	/* Ensure we have this buffer in the stream, and it is marked external. */
> +	ASSERT(id != -1 && (id & RPiBufferMask::EXTERNAL_BUFFER));
> +	bufferMap_.erase(id);
> +}
> +
>  int RPiStream::prepareBuffers(unsigned int count)
>  {
>  	int ret;
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> index ed517c22..8b23c4b2 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> @@ -51,6 +51,9 @@ public:
>  	const BufferMap &getBuffers() const;
>  	int getBufferId(FrameBuffer *buffer) const;
>  
> +	void setExternalBuffer(FrameBuffer *buffer);
> +	void removeExternalBuffer(FrameBuffer *buffer);
> +
>  	int prepareBuffers(unsigned int count);
>  	int queueBuffer(FrameBuffer *buffer);
>  	void returnBuffer(FrameBuffer *buffer);
>

Hi Naush, Kieran,

On 2020-09-15 14:59:23 +0100, Kieran Bingham wrote:
> Hi Naush,
> 
> On 08/09/2020 08:49, 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 the stream internal buffer list to
> > identify when used.
> > 
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  include/libcamera/ipa/raspberrypi.h           | 11 ++--
> >  src/ipa/raspberrypi/raspberrypi.cpp           |  6 +-
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 64 +++++++++++++------
> >  .../pipeline/raspberrypi/rpi_stream.cpp       | 30 ++++++++-
> >  .../pipeline/raspberrypi/rpi_stream.h         |  3 +
> >  5 files changed, 85 insertions(+), 29 deletions(-)
> > 
> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> > index 262fc6f3..dd6ebeac 100644
> > --- a/include/libcamera/ipa/raspberrypi.h
> > +++ b/include/libcamera/ipa/raspberrypi.h
> > @@ -28,11 +28,12 @@ enum RPiOperations {
> >  	RPI_IPA_EVENT_QUEUE_REQUEST,
> >  };
> >  
> > -enum RPiIpaMask {
> > -	ID		= 0x0ffff,
> > -	STATS		= 0x10000,
> > -	EMBEDDED_DATA	= 0x20000,
> > -	BAYER_DATA	= 0x40000
> > +enum RPiBufferMask {
> > +	ID		= 0x00ffff,
> > +	STATS		= 0x010000,
> > +	EMBEDDED_DATA	= 0x020000,
> > +	BAYER_DATA	= 0x040000,
> > +	EXTERNAL_BUFFER	= 0x100000,
> >  };
> >  
> >  /* Size of the LS grid allocation. */
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 688d2efc..0555cc4e 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -352,7 +352,7 @@ void IPARPi::processEvent(const IPAOperationData &event)
> >  
> >  		IPAOperationData op;
> >  		op.operation = RPI_IPA_ACTION_STATS_METADATA_COMPLETE;
> > -		op.data = { bufferId & RPiIpaMask::ID };
> > +		op.data = { bufferId & RPiBufferMask::ID };
> >  		op.controls = { libcameraMetadata_ };
> >  		queueFrameAction.emit(0, op);
> >  		break;
> > @@ -373,7 +373,7 @@ void IPARPi::processEvent(const IPAOperationData &event)
> >  		/* Ready to push the input buffer into the ISP. */
> >  		IPAOperationData op;
> >  		op.operation = RPI_IPA_ACTION_RUN_ISP;
> > -		op.data = { bayerbufferId & RPiIpaMask::ID };
> > +		op.data = { bayerbufferId & RPiBufferMask::ID };
> >  		queueFrameAction.emit(0, op);
> >  		break;
> >  	}
> > @@ -700,7 +700,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)
> >  {
> >  	IPAOperationData op;
> >  	op.operation = RPI_IPA_ACTION_EMBEDDED_COMPLETE;
> > -	op.data = { bufferId & RPiIpaMask::ID };
> > +	op.data = { bufferId & RPiBufferMask::ID };
> >  	queueFrameAction.emit(0, op);
> >  }
> >  
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 5621f182..f5e0d1cd 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -151,6 +151,7 @@ public:
> >  
> >  	void clearIncompleteRequests();
> >  	void handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stream);
> > +	void handleExternalBuffer(FrameBuffer *buffer, RPi::RPiStream *stream);
> >  	void handleState();
> >  
> >  	CameraSensor *sensor_;
> > @@ -725,23 +726,32 @@ 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->getBufferId(buffer) == -1) {
> >  			/*
> > -			 * If no buffer is provided by the request for this stream, we
> > -			 * queue a nullptr to the stream to signify that it must use an
> > -			 * internally allocated buffer for this capture request. This
> > -			 * buffer will not be given back to the application, but is used
> > -			 * to support the internal pipeline flow.
> > -			 *
> > -			 * 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);
> >  		}
> > +		/*
> > +		 * If no buffer is provided by the request for this stream, we
> > +		 * queue a nullptr to the stream to signify that it must use an
> > +		 * internally allocated buffer for this capture request. This
> > +		 * buffer will not be given back to the application, but is used
> > +		 * to support the internal pipeline flow.
> > +		 *
> > +		 * 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. */
> > @@ -915,8 +925,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> >  	 * 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);
> > +	mapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), RPiBufferMask::STATS);
> > +	mapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), RPiBufferMask::EMBEDDED_DATA);
> >  
> >  	return 0;
> >  }
> > @@ -1219,7 +1229,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
> >  	if (stream == &isp_[Isp::Stats]) {
> >  		IPAOperationData op;
> >  		op.operation = RPI_IPA_EVENT_SIGNAL_STAT_READY;
> > -		op.data = { RPiIpaMask::STATS | static_cast<unsigned int>(index) };
> > +		op.data = { RPiBufferMask::STATS | static_cast<unsigned int>(index) };
> >  		ipa_->processEvent(op);
> >  	} else {
> >  		/* Any other ISP output can be handed back to the application now. */
> > @@ -1293,6 +1303,11 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stre
> >  		Request *request = requestQueue_.front();
> >  
> >  		if (!dropFrameCount_ && request->findBuffer(stream) == buffer) {
> > +			/*
> > +			 * Check if this is an externally provided buffer, and if
> > +			 * so, we must stop tracking it in the pipeline handler.
> > +			 */
> > +			handleExternalBuffer(buffer, stream);
> >  			/*
> >  			 * Tag the buffer as completed, returning it to the
> >  			 * application.
> > @@ -1311,6 +1326,17 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stre
> >  	}
> >  }
> >  
> > +void RPiCameraData::handleExternalBuffer(FrameBuffer *buffer, RPi::RPiStream *stream)
> > +{
> > +	unsigned int id = stream->getBufferId(buffer);
> > +
> > +	if (!(id & RPiBufferMask::EXTERNAL_BUFFER))
> > +		return;
> > +
> > +	/* Stop the Stream object from tracking the buffer. */
> > +	stream->removeExternalBuffer(buffer);
> > +}
> > +
> >  void RPiCameraData::handleState()
> >  {
> >  	switch (state_) {
> > @@ -1443,8 +1469,8 @@ void RPiCameraData::tryRunPipeline()
> >  			<< " Embedded buffer id: " << embeddedIndex;
> >  
> >  	op.operation = RPI_IPA_EVENT_SIGNAL_ISP_PREPARE;
> > -	op.data = { RPiIpaMask::EMBEDDED_DATA | embeddedIndex,
> > -		    RPiIpaMask::BAYER_DATA | bayerIndex };
> > +	op.data = { RPiBufferMask::EMBEDDED_DATA | embeddedIndex,
> > +		    RPiBufferMask::BAYER_DATA | bayerIndex };
> >  	ipa_->processEvent(op);
> >  }
> >  
> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > index 879e25ba..c09f14c9 100644
> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > @@ -6,6 +6,8 @@
> >   */
> >  #include "rpi_stream.h"
> >  
> > +#include <libcamera/ipa/raspberrypi.h>
> > +
> >  #include "libcamera/internal/log.h"
> >  
> >  namespace libcamera {
> > @@ -44,8 +46,13 @@ bool RPiStream::isExternal() const
> >  
> >  void RPiStream::setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> >  {
> > -	for (auto const &buffer : *buffers)
> > -		bufferMap_.emplace(id_++, buffer.get());
> > +	/* Ensure we are using a sensible number of buffers. */
> > +	ASSERT(id_ < RPiBufferMask::ID);
> > +
> > +	for (auto const &buffer : *buffers) {
> > +		bufferMap_.emplace(id_ & RPiBufferMask::ID, buffer.get());
> > +		id_++;
> > +	}
> 
> I half wonder if that assert shouldn't come 'after' the updates to the
> bufferMap to catch if the id_ has overflowed after increase.
> 
> But maybe this is a tiny corner case and not expected anyway.
> 
> 
> >  }
> >  
> >  const BufferMap &RPiStream::getBuffers() const
> > @@ -68,6 +75,25 @@ int RPiStream::getBufferId(FrameBuffer *buffer) const
> >  	return it->first;
> >  }
> >  
> > +void RPiStream::setExternalBuffer(FrameBuffer *buffer)
> > +{
> > +	/* Ensure we are using a sensible number of buffers. */
> > +	ASSERT(id_ < RPiBufferMask::ID);
> > +
> > +	bufferMap_.emplace(RPiBufferMask::EXTERNAL_BUFFER | (id_ & RPiBufferMask::ID),
> > +			   buffer);
> > +	id_++;
> 
> Oh, ... is there no reduction of the id_?
> 
> What happens if an application continually provides an external buffer
> ... won't that constantly increase this id_ with every frame?
> 
> Are the ID's re-used? or is it just that the stream doesn't generate a
> new ID for each usage of an external buffer (i.e. it really would have
> to be more than 65535 entirely different buffers...)
> 
> I guess this can be easily validated by setting RPiBufferMask::ID to
> 0xFF or even 0xF to see if it hits any assertions quickly
> 
> As long as this doesn't leak and cause an assertion/overflow after
> 0xFFFF/RPiBufferMask::ID requests are queued ...

This is a very good observation that I missed in my review. Good thing 
you looked at it as I was about to merge this series.

Naush would it be possible to rework this logic slightly to makesure the 
id_ used can rollover while at the same time is not overwriting an 
existing mapping? With this solved I think we are ready to merge this 
series \o/.

> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > +}
> > +
> > +void RPiStream::removeExternalBuffer(FrameBuffer *buffer)
> > +{
> > +	int id = getBufferId(buffer);
> > +
> > +	/* Ensure we have this buffer in the stream, and it is marked external. */
> > +	ASSERT(id != -1 && (id & RPiBufferMask::EXTERNAL_BUFFER));
> > +	bufferMap_.erase(id);
> > +}
> > +
> >  int RPiStream::prepareBuffers(unsigned int count)
> >  {
> >  	int ret;
> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > index ed517c22..8b23c4b2 100644
> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > @@ -51,6 +51,9 @@ public:
> >  	const BufferMap &getBuffers() const;
> >  	int getBufferId(FrameBuffer *buffer) const;
> >  
> > +	void setExternalBuffer(FrameBuffer *buffer);
> > +	void removeExternalBuffer(FrameBuffer *buffer);
> > +
> >  	int prepareBuffers(unsigned int count);
> >  	int queueBuffer(FrameBuffer *buffer);
> >  	void returnBuffer(FrameBuffer *buffer);
> > 
> 
> -- 
> Regards
> --
> Kieran
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Niklas and Kieran,

Thank you both for the reviews.

On Tue, 15 Sep 2020 at 15:55, Niklas Söderlund
<niklas.soderlund@ragnatech.se> wrote:
>
> Hi Naush, Kieran,
>
> On 2020-09-15 14:59:23 +0100, Kieran Bingham wrote:
> > Hi Naush,
> >
> > On 08/09/2020 08:49, 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 the stream internal buffer list to
> > > identify when used.
> > >
> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > ---
> > >  include/libcamera/ipa/raspberrypi.h           | 11 ++--
> > >  src/ipa/raspberrypi/raspberrypi.cpp           |  6 +-
> > >  .../pipeline/raspberrypi/raspberrypi.cpp      | 64 +++++++++++++------
> > >  .../pipeline/raspberrypi/rpi_stream.cpp       | 30 ++++++++-
> > >  .../pipeline/raspberrypi/rpi_stream.h         |  3 +
> > >  5 files changed, 85 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> > > index 262fc6f3..dd6ebeac 100644
> > > --- a/include/libcamera/ipa/raspberrypi.h
> > > +++ b/include/libcamera/ipa/raspberrypi.h
> > > @@ -28,11 +28,12 @@ enum RPiOperations {
> > >     RPI_IPA_EVENT_QUEUE_REQUEST,
> > >  };
> > >
> > > -enum RPiIpaMask {
> > > -   ID              = 0x0ffff,
> > > -   STATS           = 0x10000,
> > > -   EMBEDDED_DATA   = 0x20000,
> > > -   BAYER_DATA      = 0x40000
> > > +enum RPiBufferMask {
> > > +   ID              = 0x00ffff,
> > > +   STATS           = 0x010000,
> > > +   EMBEDDED_DATA   = 0x020000,
> > > +   BAYER_DATA      = 0x040000,
> > > +   EXTERNAL_BUFFER = 0x100000,
> > >  };
> > >
> > >  /* Size of the LS grid allocation. */
> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > > index 688d2efc..0555cc4e 100644
> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > @@ -352,7 +352,7 @@ void IPARPi::processEvent(const IPAOperationData &event)
> > >
> > >             IPAOperationData op;
> > >             op.operation = RPI_IPA_ACTION_STATS_METADATA_COMPLETE;
> > > -           op.data = { bufferId & RPiIpaMask::ID };
> > > +           op.data = { bufferId & RPiBufferMask::ID };
> > >             op.controls = { libcameraMetadata_ };
> > >             queueFrameAction.emit(0, op);
> > >             break;
> > > @@ -373,7 +373,7 @@ void IPARPi::processEvent(const IPAOperationData &event)
> > >             /* Ready to push the input buffer into the ISP. */
> > >             IPAOperationData op;
> > >             op.operation = RPI_IPA_ACTION_RUN_ISP;
> > > -           op.data = { bayerbufferId & RPiIpaMask::ID };
> > > +           op.data = { bayerbufferId & RPiBufferMask::ID };
> > >             queueFrameAction.emit(0, op);
> > >             break;
> > >     }
> > > @@ -700,7 +700,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)
> > >  {
> > >     IPAOperationData op;
> > >     op.operation = RPI_IPA_ACTION_EMBEDDED_COMPLETE;
> > > -   op.data = { bufferId & RPiIpaMask::ID };
> > > +   op.data = { bufferId & RPiBufferMask::ID };
> > >     queueFrameAction.emit(0, op);
> > >  }
> > >
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index 5621f182..f5e0d1cd 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -151,6 +151,7 @@ public:
> > >
> > >     void clearIncompleteRequests();
> > >     void handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stream);
> > > +   void handleExternalBuffer(FrameBuffer *buffer, RPi::RPiStream *stream);
> > >     void handleState();
> > >
> > >     CameraSensor *sensor_;
> > > @@ -725,23 +726,32 @@ 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->getBufferId(buffer) == -1) {
> > >                     /*
> > > -                    * If no buffer is provided by the request for this stream, we
> > > -                    * queue a nullptr to the stream to signify that it must use an
> > > -                    * internally allocated buffer for this capture request. This
> > > -                    * buffer will not be given back to the application, but is used
> > > -                    * to support the internal pipeline flow.
> > > -                    *
> > > -                    * 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);
> > >             }
> > > +           /*
> > > +            * If no buffer is provided by the request for this stream, we
> > > +            * queue a nullptr to the stream to signify that it must use an
> > > +            * internally allocated buffer for this capture request. This
> > > +            * buffer will not be given back to the application, but is used
> > > +            * to support the internal pipeline flow.
> > > +            *
> > > +            * 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. */
> > > @@ -915,8 +925,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> > >      * 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);
> > > +   mapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), RPiBufferMask::STATS);
> > > +   mapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), RPiBufferMask::EMBEDDED_DATA);
> > >
> > >     return 0;
> > >  }
> > > @@ -1219,7 +1229,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
> > >     if (stream == &isp_[Isp::Stats]) {
> > >             IPAOperationData op;
> > >             op.operation = RPI_IPA_EVENT_SIGNAL_STAT_READY;
> > > -           op.data = { RPiIpaMask::STATS | static_cast<unsigned int>(index) };
> > > +           op.data = { RPiBufferMask::STATS | static_cast<unsigned int>(index) };
> > >             ipa_->processEvent(op);
> > >     } else {
> > >             /* Any other ISP output can be handed back to the application now. */
> > > @@ -1293,6 +1303,11 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stre
> > >             Request *request = requestQueue_.front();
> > >
> > >             if (!dropFrameCount_ && request->findBuffer(stream) == buffer) {
> > > +                   /*
> > > +                    * Check if this is an externally provided buffer, and if
> > > +                    * so, we must stop tracking it in the pipeline handler.
> > > +                    */
> > > +                   handleExternalBuffer(buffer, stream);
> > >                     /*
> > >                      * Tag the buffer as completed, returning it to the
> > >                      * application.
> > > @@ -1311,6 +1326,17 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stre
> > >     }
> > >  }
> > >
> > > +void RPiCameraData::handleExternalBuffer(FrameBuffer *buffer, RPi::RPiStream *stream)
> > > +{
> > > +   unsigned int id = stream->getBufferId(buffer);
> > > +
> > > +   if (!(id & RPiBufferMask::EXTERNAL_BUFFER))
> > > +           return;
> > > +
> > > +   /* Stop the Stream object from tracking the buffer. */
> > > +   stream->removeExternalBuffer(buffer);
> > > +}
> > > +
> > >  void RPiCameraData::handleState()
> > >  {
> > >     switch (state_) {
> > > @@ -1443,8 +1469,8 @@ void RPiCameraData::tryRunPipeline()
> > >                     << " Embedded buffer id: " << embeddedIndex;
> > >
> > >     op.operation = RPI_IPA_EVENT_SIGNAL_ISP_PREPARE;
> > > -   op.data = { RPiIpaMask::EMBEDDED_DATA | embeddedIndex,
> > > -               RPiIpaMask::BAYER_DATA | bayerIndex };
> > > +   op.data = { RPiBufferMask::EMBEDDED_DATA | embeddedIndex,
> > > +               RPiBufferMask::BAYER_DATA | bayerIndex };
> > >     ipa_->processEvent(op);
> > >  }
> > >
> > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > > index 879e25ba..c09f14c9 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > > @@ -6,6 +6,8 @@
> > >   */
> > >  #include "rpi_stream.h"
> > >
> > > +#include <libcamera/ipa/raspberrypi.h>
> > > +
> > >  #include "libcamera/internal/log.h"
> > >
> > >  namespace libcamera {
> > > @@ -44,8 +46,13 @@ bool RPiStream::isExternal() const
> > >
> > >  void RPiStream::setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > >  {
> > > -   for (auto const &buffer : *buffers)
> > > -           bufferMap_.emplace(id_++, buffer.get());
> > > +   /* Ensure we are using a sensible number of buffers. */
> > > +   ASSERT(id_ < RPiBufferMask::ID);
> > > +
> > > +   for (auto const &buffer : *buffers) {
> > > +           bufferMap_.emplace(id_ & RPiBufferMask::ID, buffer.get());
> > > +           id_++;
> > > +   }
> >
> > I half wonder if that assert shouldn't come 'after' the updates to the
> > bufferMap to catch if the id_ has overflowed after increase.
> >
> > But maybe this is a tiny corner case and not expected anyway.
> >
> >
> > >  }
> > >
> > >  const BufferMap &RPiStream::getBuffers() const
> > > @@ -68,6 +75,25 @@ int RPiStream::getBufferId(FrameBuffer *buffer) const
> > >     return it->first;
> > >  }
> > >
> > > +void RPiStream::setExternalBuffer(FrameBuffer *buffer)
> > > +{
> > > +   /* Ensure we are using a sensible number of buffers. */
> > > +   ASSERT(id_ < RPiBufferMask::ID);
> > > +
> > > +   bufferMap_.emplace(RPiBufferMask::EXTERNAL_BUFFER | (id_ & RPiBufferMask::ID),
> > > +                      buffer);
> > > +   id_++;
> >
> > Oh, ... is there no reduction of the id_?
> >
> > What happens if an application continually provides an external buffer
> > ... won't that constantly increase this id_ with every frame?
> >
> > Are the ID's re-used? or is it just that the stream doesn't generate a
> > new ID for each usage of an external buffer (i.e. it really would have
> > to be more than 65535 entirely different buffers...)
> >
> > I guess this can be easily validated by setting RPiBufferMask::ID to
> > 0xFF or even 0xF to see if it hits any assertions quickly
> >
> > As long as this doesn't leak and cause an assertion/overflow after
> > 0xFFFF/RPiBufferMask::ID requests are queued ...
>
> This is a very good observation that I missed in my review. Good thing
> you looked at it as I was about to merge this series.
>
> Naush would it be possible to rework this logic slightly to makesure the
> id_ used can rollover while at the same time is not overwriting an
> existing mapping? With this solved I think we are ready to merge this
> series \o/.

Currently we will hit an assertion if id goes over 0xffff as it is
never decremented.  If this is going to be a problem (and it sounds
like it will be for android that only ever uses external buffers?), I
will think about how to rework this to allow for reusing a previously
returned id.

Regards,
Naush

>
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > > +}
> > > +
> > > +void RPiStream::removeExternalBuffer(FrameBuffer *buffer)
> > > +{
> > > +   int id = getBufferId(buffer);
> > > +
> > > +   /* Ensure we have this buffer in the stream, and it is marked external. */
> > > +   ASSERT(id != -1 && (id & RPiBufferMask::EXTERNAL_BUFFER));
> > > +   bufferMap_.erase(id);
> > > +}
> > > +
> > >  int RPiStream::prepareBuffers(unsigned int count)
> > >  {
> > >     int ret;
> > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > > index ed517c22..8b23c4b2 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > > @@ -51,6 +51,9 @@ public:
> > >     const BufferMap &getBuffers() const;
> > >     int getBufferId(FrameBuffer *buffer) const;
> > >
> > > +   void setExternalBuffer(FrameBuffer *buffer);
> > > +   void removeExternalBuffer(FrameBuffer *buffer);
> > > +
> > >     int prepareBuffers(unsigned int count);
> > >     int queueBuffer(FrameBuffer *buffer);
> > >     void returnBuffer(FrameBuffer *buffer);
> > >
> >
> > --
> > Regards
> > --
> > Kieran
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund

Hi Naushir,

On 2020-09-15 16:18:07 +0100, Naushir Patuck wrote:
> Hi Niklas and Kieran,
> 
> Thank you both for the reviews.
> 
> On Tue, 15 Sep 2020 at 15:55, Niklas Söderlund
> <niklas.soderlund@ragnatech.se> wrote:
> >
> > Hi Naush, Kieran,
> >
> > On 2020-09-15 14:59:23 +0100, Kieran Bingham wrote:
> > > Hi Naush,
> > >
> > > On 08/09/2020 08:49, 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 the stream internal buffer list to
> > > > identify when used.
> > > >
> > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > > ---
> > > >  include/libcamera/ipa/raspberrypi.h           | 11 ++--
> > > >  src/ipa/raspberrypi/raspberrypi.cpp           |  6 +-
> > > >  .../pipeline/raspberrypi/raspberrypi.cpp      | 64 +++++++++++++------
> > > >  .../pipeline/raspberrypi/rpi_stream.cpp       | 30 ++++++++-
> > > >  .../pipeline/raspberrypi/rpi_stream.h         |  3 +
> > > >  5 files changed, 85 insertions(+), 29 deletions(-)
> > > >
> > > > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> > > > index 262fc6f3..dd6ebeac 100644
> > > > --- a/include/libcamera/ipa/raspberrypi.h
> > > > +++ b/include/libcamera/ipa/raspberrypi.h
> > > > @@ -28,11 +28,12 @@ enum RPiOperations {
> > > >     RPI_IPA_EVENT_QUEUE_REQUEST,
> > > >  };
> > > >
> > > > -enum RPiIpaMask {
> > > > -   ID              = 0x0ffff,
> > > > -   STATS           = 0x10000,
> > > > -   EMBEDDED_DATA   = 0x20000,
> > > > -   BAYER_DATA      = 0x40000
> > > > +enum RPiBufferMask {
> > > > +   ID              = 0x00ffff,
> > > > +   STATS           = 0x010000,
> > > > +   EMBEDDED_DATA   = 0x020000,
> > > > +   BAYER_DATA      = 0x040000,
> > > > +   EXTERNAL_BUFFER = 0x100000,
> > > >  };
> > > >
> > > >  /* Size of the LS grid allocation. */
> > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > index 688d2efc..0555cc4e 100644
> > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > @@ -352,7 +352,7 @@ void IPARPi::processEvent(const IPAOperationData &event)
> > > >
> > > >             IPAOperationData op;
> > > >             op.operation = RPI_IPA_ACTION_STATS_METADATA_COMPLETE;
> > > > -           op.data = { bufferId & RPiIpaMask::ID };
> > > > +           op.data = { bufferId & RPiBufferMask::ID };
> > > >             op.controls = { libcameraMetadata_ };
> > > >             queueFrameAction.emit(0, op);
> > > >             break;
> > > > @@ -373,7 +373,7 @@ void IPARPi::processEvent(const IPAOperationData &event)
> > > >             /* Ready to push the input buffer into the ISP. */
> > > >             IPAOperationData op;
> > > >             op.operation = RPI_IPA_ACTION_RUN_ISP;
> > > > -           op.data = { bayerbufferId & RPiIpaMask::ID };
> > > > +           op.data = { bayerbufferId & RPiBufferMask::ID };
> > > >             queueFrameAction.emit(0, op);
> > > >             break;
> > > >     }
> > > > @@ -700,7 +700,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)
> > > >  {
> > > >     IPAOperationData op;
> > > >     op.operation = RPI_IPA_ACTION_EMBEDDED_COMPLETE;
> > > > -   op.data = { bufferId & RPiIpaMask::ID };
> > > > +   op.data = { bufferId & RPiBufferMask::ID };
> > > >     queueFrameAction.emit(0, op);
> > > >  }
> > > >
> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > index 5621f182..f5e0d1cd 100644
> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > @@ -151,6 +151,7 @@ public:
> > > >
> > > >     void clearIncompleteRequests();
> > > >     void handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stream);
> > > > +   void handleExternalBuffer(FrameBuffer *buffer, RPi::RPiStream *stream);
> > > >     void handleState();
> > > >
> > > >     CameraSensor *sensor_;
> > > > @@ -725,23 +726,32 @@ 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->getBufferId(buffer) == -1) {
> > > >                     /*
> > > > -                    * If no buffer is provided by the request for this stream, we
> > > > -                    * queue a nullptr to the stream to signify that it must use an
> > > > -                    * internally allocated buffer for this capture request. This
> > > > -                    * buffer will not be given back to the application, but is used
> > > > -                    * to support the internal pipeline flow.
> > > > -                    *
> > > > -                    * 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);
> > > >             }
> > > > +           /*
> > > > +            * If no buffer is provided by the request for this stream, we
> > > > +            * queue a nullptr to the stream to signify that it must use an
> > > > +            * internally allocated buffer for this capture request. This
> > > > +            * buffer will not be given back to the application, but is used
> > > > +            * to support the internal pipeline flow.
> > > > +            *
> > > > +            * 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. */
> > > > @@ -915,8 +925,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> > > >      * 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);
> > > > +   mapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), RPiBufferMask::STATS);
> > > > +   mapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), RPiBufferMask::EMBEDDED_DATA);
> > > >
> > > >     return 0;
> > > >  }
> > > > @@ -1219,7 +1229,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
> > > >     if (stream == &isp_[Isp::Stats]) {
> > > >             IPAOperationData op;
> > > >             op.operation = RPI_IPA_EVENT_SIGNAL_STAT_READY;
> > > > -           op.data = { RPiIpaMask::STATS | static_cast<unsigned int>(index) };
> > > > +           op.data = { RPiBufferMask::STATS | static_cast<unsigned int>(index) };
> > > >             ipa_->processEvent(op);
> > > >     } else {
> > > >             /* Any other ISP output can be handed back to the application now. */
> > > > @@ -1293,6 +1303,11 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stre
> > > >             Request *request = requestQueue_.front();
> > > >
> > > >             if (!dropFrameCount_ && request->findBuffer(stream) == buffer) {
> > > > +                   /*
> > > > +                    * Check if this is an externally provided buffer, and if
> > > > +                    * so, we must stop tracking it in the pipeline handler.
> > > > +                    */
> > > > +                   handleExternalBuffer(buffer, stream);
> > > >                     /*
> > > >                      * Tag the buffer as completed, returning it to the
> > > >                      * application.
> > > > @@ -1311,6 +1326,17 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stre
> > > >     }
> > > >  }
> > > >
> > > > +void RPiCameraData::handleExternalBuffer(FrameBuffer *buffer, RPi::RPiStream *stream)
> > > > +{
> > > > +   unsigned int id = stream->getBufferId(buffer);
> > > > +
> > > > +   if (!(id & RPiBufferMask::EXTERNAL_BUFFER))
> > > > +           return;
> > > > +
> > > > +   /* Stop the Stream object from tracking the buffer. */
> > > > +   stream->removeExternalBuffer(buffer);
> > > > +}
> > > > +
> > > >  void RPiCameraData::handleState()
> > > >  {
> > > >     switch (state_) {
> > > > @@ -1443,8 +1469,8 @@ void RPiCameraData::tryRunPipeline()
> > > >                     << " Embedded buffer id: " << embeddedIndex;
> > > >
> > > >     op.operation = RPI_IPA_EVENT_SIGNAL_ISP_PREPARE;
> > > > -   op.data = { RPiIpaMask::EMBEDDED_DATA | embeddedIndex,
> > > > -               RPiIpaMask::BAYER_DATA | bayerIndex };
> > > > +   op.data = { RPiBufferMask::EMBEDDED_DATA | embeddedIndex,
> > > > +               RPiBufferMask::BAYER_DATA | bayerIndex };
> > > >     ipa_->processEvent(op);
> > > >  }
> > > >
> > > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > > > index 879e25ba..c09f14c9 100644
> > > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > > > @@ -6,6 +6,8 @@
> > > >   */
> > > >  #include "rpi_stream.h"
> > > >
> > > > +#include <libcamera/ipa/raspberrypi.h>
> > > > +
> > > >  #include "libcamera/internal/log.h"
> > > >
> > > >  namespace libcamera {
> > > > @@ -44,8 +46,13 @@ bool RPiStream::isExternal() const
> > > >
> > > >  void RPiStream::setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > > >  {
> > > > -   for (auto const &buffer : *buffers)
> > > > -           bufferMap_.emplace(id_++, buffer.get());
> > > > +   /* Ensure we are using a sensible number of buffers. */
> > > > +   ASSERT(id_ < RPiBufferMask::ID);
> > > > +
> > > > +   for (auto const &buffer : *buffers) {
> > > > +           bufferMap_.emplace(id_ & RPiBufferMask::ID, buffer.get());
> > > > +           id_++;
> > > > +   }
> > >
> > > I half wonder if that assert shouldn't come 'after' the updates to the
> > > bufferMap to catch if the id_ has overflowed after increase.
> > >
> > > But maybe this is a tiny corner case and not expected anyway.
> > >
> > >
> > > >  }
> > > >
> > > >  const BufferMap &RPiStream::getBuffers() const
> > > > @@ -68,6 +75,25 @@ int RPiStream::getBufferId(FrameBuffer *buffer) const
> > > >     return it->first;
> > > >  }
> > > >
> > > > +void RPiStream::setExternalBuffer(FrameBuffer *buffer)
> > > > +{
> > > > +   /* Ensure we are using a sensible number of buffers. */
> > > > +   ASSERT(id_ < RPiBufferMask::ID);
> > > > +
> > > > +   bufferMap_.emplace(RPiBufferMask::EXTERNAL_BUFFER | (id_ & RPiBufferMask::ID),
> > > > +                      buffer);
> > > > +   id_++;
> > >
> > > Oh, ... is there no reduction of the id_?
> > >
> > > What happens if an application continually provides an external buffer
> > > ... won't that constantly increase this id_ with every frame?
> > >
> > > Are the ID's re-used? or is it just that the stream doesn't generate a
> > > new ID for each usage of an external buffer (i.e. it really would have
> > > to be more than 65535 entirely different buffers...)
> > >
> > > I guess this can be easily validated by setting RPiBufferMask::ID to
> > > 0xFF or even 0xF to see if it hits any assertions quickly
> > >
> > > As long as this doesn't leak and cause an assertion/overflow after
> > > 0xFFFF/RPiBufferMask::ID requests are queued ...
> >
> > This is a very good observation that I missed in my review. Good thing
> > you looked at it as I was about to merge this series.
> >
> > Naush would it be possible to rework this logic slightly to makesure the
> > id_ used can rollover while at the same time is not overwriting an
> > existing mapping? With this solved I think we are ready to merge this
> > series \o/.
> 
> Currently we will hit an assertion if id goes over 0xffff as it is
> never decremented.  If this is going to be a problem (and it sounds
> like it will be for android that only ever uses external buffers?), I
> will think about how to rework this to allow for reusing a previously
> returned id.

How about something like this,

    /* Find next free buffer id. */
    unsigned int id = (id_ + 1) % RPiBufferMask::ID;
    while (id != id_ && bufferMap_.find(id) != bufferMap_.end()) {
        id = (id + 1) % RPiBufferMask::ID;

    /* No free id could be found, this really should not happen. */
    ASSERT(id != id_);

    id_ = id;

Of course untested ;-)

> 
> Regards,
> Naush
> 
> >
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > >
> > > > +}
> > > > +
> > > > +void RPiStream::removeExternalBuffer(FrameBuffer *buffer)
> > > > +{
> > > > +   int id = getBufferId(buffer);
> > > > +
> > > > +   /* Ensure we have this buffer in the stream, and it is marked external. */
> > > > +   ASSERT(id != -1 && (id & RPiBufferMask::EXTERNAL_BUFFER));
> > > > +   bufferMap_.erase(id);
> > > > +}
> > > > +
> > > >  int RPiStream::prepareBuffers(unsigned int count)
> > > >  {
> > > >     int ret;
> > > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > > > index ed517c22..8b23c4b2 100644
> > > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > > > @@ -51,6 +51,9 @@ public:
> > > >     const BufferMap &getBuffers() const;
> > > >     int getBufferId(FrameBuffer *buffer) const;
> > > >
> > > > +   void setExternalBuffer(FrameBuffer *buffer);
> > > > +   void removeExternalBuffer(FrameBuffer *buffer);
> > > > +
> > > >     int prepareBuffers(unsigned int count);
> > > >     int queueBuffer(FrameBuffer *buffer);
> > > >     void returnBuffer(FrameBuffer *buffer);
> > > >
> > >
> > > --
> > > Regards
> > > --
> > > Kieran
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel@lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> >
> > --
> > Regards,
> > Niklas Söderlund

Hi Niklas,

On Tue, 15 Sep 2020 at 18:31, Niklas Söderlund
<niklas.soderlund@ragnatech.se> wrote:
>
> Hi Naushir,
>
> On 2020-09-15 16:18:07 +0100, Naushir Patuck wrote:
> > Hi Niklas and Kieran,
> >
> > Thank you both for the reviews.
> >
> > On Tue, 15 Sep 2020 at 15:55, Niklas Söderlund
> > <niklas.soderlund@ragnatech.se> wrote:
> > >
> > > Hi Naush, Kieran,
> > >
> > > On 2020-09-15 14:59:23 +0100, Kieran Bingham wrote:
> > > > Hi Naush,
> > > >
> > > > On 08/09/2020 08:49, 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 the stream internal buffer list to
> > > > > identify when used.
> > > > >
> > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > > > ---
> > > > >  include/libcamera/ipa/raspberrypi.h           | 11 ++--
> > > > >  src/ipa/raspberrypi/raspberrypi.cpp           |  6 +-
> > > > >  .../pipeline/raspberrypi/raspberrypi.cpp      | 64 +++++++++++++------
> > > > >  .../pipeline/raspberrypi/rpi_stream.cpp       | 30 ++++++++-
> > > > >  .../pipeline/raspberrypi/rpi_stream.h         |  3 +
> > > > >  5 files changed, 85 insertions(+), 29 deletions(-)
> > > > >
> > > > > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> > > > > index 262fc6f3..dd6ebeac 100644
> > > > > --- a/include/libcamera/ipa/raspberrypi.h
> > > > > +++ b/include/libcamera/ipa/raspberrypi.h
> > > > > @@ -28,11 +28,12 @@ enum RPiOperations {
> > > > >     RPI_IPA_EVENT_QUEUE_REQUEST,
> > > > >  };
> > > > >
> > > > > -enum RPiIpaMask {
> > > > > -   ID              = 0x0ffff,
> > > > > -   STATS           = 0x10000,
> > > > > -   EMBEDDED_DATA   = 0x20000,
> > > > > -   BAYER_DATA      = 0x40000
> > > > > +enum RPiBufferMask {
> > > > > +   ID              = 0x00ffff,
> > > > > +   STATS           = 0x010000,
> > > > > +   EMBEDDED_DATA   = 0x020000,
> > > > > +   BAYER_DATA      = 0x040000,
> > > > > +   EXTERNAL_BUFFER = 0x100000,
> > > > >  };
> > > > >
> > > > >  /* Size of the LS grid allocation. */
> > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > index 688d2efc..0555cc4e 100644
> > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > @@ -352,7 +352,7 @@ void IPARPi::processEvent(const IPAOperationData &event)
> > > > >
> > > > >             IPAOperationData op;
> > > > >             op.operation = RPI_IPA_ACTION_STATS_METADATA_COMPLETE;
> > > > > -           op.data = { bufferId & RPiIpaMask::ID };
> > > > > +           op.data = { bufferId & RPiBufferMask::ID };
> > > > >             op.controls = { libcameraMetadata_ };
> > > > >             queueFrameAction.emit(0, op);
> > > > >             break;
> > > > > @@ -373,7 +373,7 @@ void IPARPi::processEvent(const IPAOperationData &event)
> > > > >             /* Ready to push the input buffer into the ISP. */
> > > > >             IPAOperationData op;
> > > > >             op.operation = RPI_IPA_ACTION_RUN_ISP;
> > > > > -           op.data = { bayerbufferId & RPiIpaMask::ID };
> > > > > +           op.data = { bayerbufferId & RPiBufferMask::ID };
> > > > >             queueFrameAction.emit(0, op);
> > > > >             break;
> > > > >     }
> > > > > @@ -700,7 +700,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)
> > > > >  {
> > > > >     IPAOperationData op;
> > > > >     op.operation = RPI_IPA_ACTION_EMBEDDED_COMPLETE;
> > > > > -   op.data = { bufferId & RPiIpaMask::ID };
> > > > > +   op.data = { bufferId & RPiBufferMask::ID };
> > > > >     queueFrameAction.emit(0, op);
> > > > >  }
> > > > >
> > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > index 5621f182..f5e0d1cd 100644
> > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > @@ -151,6 +151,7 @@ public:
> > > > >
> > > > >     void clearIncompleteRequests();
> > > > >     void handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stream);
> > > > > +   void handleExternalBuffer(FrameBuffer *buffer, RPi::RPiStream *stream);
> > > > >     void handleState();
> > > > >
> > > > >     CameraSensor *sensor_;
> > > > > @@ -725,23 +726,32 @@ 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->getBufferId(buffer) == -1) {
> > > > >                     /*
> > > > > -                    * If no buffer is provided by the request for this stream, we
> > > > > -                    * queue a nullptr to the stream to signify that it must use an
> > > > > -                    * internally allocated buffer for this capture request. This
> > > > > -                    * buffer will not be given back to the application, but is used
> > > > > -                    * to support the internal pipeline flow.
> > > > > -                    *
> > > > > -                    * 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);
> > > > >             }
> > > > > +           /*
> > > > > +            * If no buffer is provided by the request for this stream, we
> > > > > +            * queue a nullptr to the stream to signify that it must use an
> > > > > +            * internally allocated buffer for this capture request. This
> > > > > +            * buffer will not be given back to the application, but is used
> > > > > +            * to support the internal pipeline flow.
> > > > > +            *
> > > > > +            * 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. */
> > > > > @@ -915,8 +925,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> > > > >      * 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);
> > > > > +   mapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), RPiBufferMask::STATS);
> > > > > +   mapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), RPiBufferMask::EMBEDDED_DATA);
> > > > >
> > > > >     return 0;
> > > > >  }
> > > > > @@ -1219,7 +1229,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
> > > > >     if (stream == &isp_[Isp::Stats]) {
> > > > >             IPAOperationData op;
> > > > >             op.operation = RPI_IPA_EVENT_SIGNAL_STAT_READY;
> > > > > -           op.data = { RPiIpaMask::STATS | static_cast<unsigned int>(index) };
> > > > > +           op.data = { RPiBufferMask::STATS | static_cast<unsigned int>(index) };
> > > > >             ipa_->processEvent(op);
> > > > >     } else {
> > > > >             /* Any other ISP output can be handed back to the application now. */
> > > > > @@ -1293,6 +1303,11 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stre
> > > > >             Request *request = requestQueue_.front();
> > > > >
> > > > >             if (!dropFrameCount_ && request->findBuffer(stream) == buffer) {
> > > > > +                   /*
> > > > > +                    * Check if this is an externally provided buffer, and if
> > > > > +                    * so, we must stop tracking it in the pipeline handler.
> > > > > +                    */
> > > > > +                   handleExternalBuffer(buffer, stream);
> > > > >                     /*
> > > > >                      * Tag the buffer as completed, returning it to the
> > > > >                      * application.
> > > > > @@ -1311,6 +1326,17 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stre
> > > > >     }
> > > > >  }
> > > > >
> > > > > +void RPiCameraData::handleExternalBuffer(FrameBuffer *buffer, RPi::RPiStream *stream)
> > > > > +{
> > > > > +   unsigned int id = stream->getBufferId(buffer);
> > > > > +
> > > > > +   if (!(id & RPiBufferMask::EXTERNAL_BUFFER))
> > > > > +           return;
> > > > > +
> > > > > +   /* Stop the Stream object from tracking the buffer. */
> > > > > +   stream->removeExternalBuffer(buffer);
> > > > > +}
> > > > > +
> > > > >  void RPiCameraData::handleState()
> > > > >  {
> > > > >     switch (state_) {
> > > > > @@ -1443,8 +1469,8 @@ void RPiCameraData::tryRunPipeline()
> > > > >                     << " Embedded buffer id: " << embeddedIndex;
> > > > >
> > > > >     op.operation = RPI_IPA_EVENT_SIGNAL_ISP_PREPARE;
> > > > > -   op.data = { RPiIpaMask::EMBEDDED_DATA | embeddedIndex,
> > > > > -               RPiIpaMask::BAYER_DATA | bayerIndex };
> > > > > +   op.data = { RPiBufferMask::EMBEDDED_DATA | embeddedIndex,
> > > > > +               RPiBufferMask::BAYER_DATA | bayerIndex };
> > > > >     ipa_->processEvent(op);
> > > > >  }
> > > > >
> > > > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > > > > index 879e25ba..c09f14c9 100644
> > > > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > > > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > > > > @@ -6,6 +6,8 @@
> > > > >   */
> > > > >  #include "rpi_stream.h"
> > > > >
> > > > > +#include <libcamera/ipa/raspberrypi.h>
> > > > > +
> > > > >  #include "libcamera/internal/log.h"
> > > > >
> > > > >  namespace libcamera {
> > > > > @@ -44,8 +46,13 @@ bool RPiStream::isExternal() const
> > > > >
> > > > >  void RPiStream::setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > > > >  {
> > > > > -   for (auto const &buffer : *buffers)
> > > > > -           bufferMap_.emplace(id_++, buffer.get());
> > > > > +   /* Ensure we are using a sensible number of buffers. */
> > > > > +   ASSERT(id_ < RPiBufferMask::ID);
> > > > > +
> > > > > +   for (auto const &buffer : *buffers) {
> > > > > +           bufferMap_.emplace(id_ & RPiBufferMask::ID, buffer.get());
> > > > > +           id_++;
> > > > > +   }
> > > >
> > > > I half wonder if that assert shouldn't come 'after' the updates to the
> > > > bufferMap to catch if the id_ has overflowed after increase.
> > > >
> > > > But maybe this is a tiny corner case and not expected anyway.
> > > >
> > > >
> > > > >  }
> > > > >
> > > > >  const BufferMap &RPiStream::getBuffers() const
> > > > > @@ -68,6 +75,25 @@ int RPiStream::getBufferId(FrameBuffer *buffer) const
> > > > >     return it->first;
> > > > >  }
> > > > >
> > > > > +void RPiStream::setExternalBuffer(FrameBuffer *buffer)
> > > > > +{
> > > > > +   /* Ensure we are using a sensible number of buffers. */
> > > > > +   ASSERT(id_ < RPiBufferMask::ID);
> > > > > +
> > > > > +   bufferMap_.emplace(RPiBufferMask::EXTERNAL_BUFFER | (id_ & RPiBufferMask::ID),
> > > > > +                      buffer);
> > > > > +   id_++;
> > > >
> > > > Oh, ... is there no reduction of the id_?
> > > >
> > > > What happens if an application continually provides an external buffer
> > > > ... won't that constantly increase this id_ with every frame?
> > > >
> > > > Are the ID's re-used? or is it just that the stream doesn't generate a
> > > > new ID for each usage of an external buffer (i.e. it really would have
> > > > to be more than 65535 entirely different buffers...)
> > > >
> > > > I guess this can be easily validated by setting RPiBufferMask::ID to
> > > > 0xFF or even 0xF to see if it hits any assertions quickly
> > > >
> > > > As long as this doesn't leak and cause an assertion/overflow after
> > > > 0xFFFF/RPiBufferMask::ID requests are queued ...
> > >
> > > This is a very good observation that I missed in my review. Good thing
> > > you looked at it as I was about to merge this series.
> > >
> > > Naush would it be possible to rework this logic slightly to makesure the
> > > id_ used can rollover while at the same time is not overwriting an
> > > existing mapping? With this solved I think we are ready to merge this
> > > series \o/.
> >
> > Currently we will hit an assertion if id goes over 0xffff as it is
> > never decremented.  If this is going to be a problem (and it sounds
> > like it will be for android that only ever uses external buffers?), I
> > will think about how to rework this to allow for reusing a previously
> > returned id.
>
> How about something like this,
>
>     /* Find next free buffer id. */
>     unsigned int id = (id_ + 1) % RPiBufferMask::ID;
>     while (id != id_ && bufferMap_.find(id) != bufferMap_.end()) {
>         id = (id + 1) % RPiBufferMask::ID;
>
>     /* No free id could be found, this really should not happen. */
>     ASSERT(id != id_);
>
>     id_ = id;
>
> Of course untested ;-)

Something along those lines popped into my head as well.  However, I
worry about the efficiency of map::find() on every frame, across all
ids if the map becomes dense with a large number of buffers.  Instead,
perhaps track recyclable ids in a separate queue so we can avoid the
costly lookup?

>
> >
> > Regards,
> > Naush
> >
> > >
> > > >
> > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > >
> > > > > +}
> > > > > +
> > > > > +void RPiStream::removeExternalBuffer(FrameBuffer *buffer)
> > > > > +{
> > > > > +   int id = getBufferId(buffer);
> > > > > +
> > > > > +   /* Ensure we have this buffer in the stream, and it is marked external. */
> > > > > +   ASSERT(id != -1 && (id & RPiBufferMask::EXTERNAL_BUFFER));
> > > > > +   bufferMap_.erase(id);
> > > > > +}
> > > > > +
> > > > >  int RPiStream::prepareBuffers(unsigned int count)
> > > > >  {
> > > > >     int ret;
> > > > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > > > > index ed517c22..8b23c4b2 100644
> > > > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > > > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > > > > @@ -51,6 +51,9 @@ public:
> > > > >     const BufferMap &getBuffers() const;
> > > > >     int getBufferId(FrameBuffer *buffer) const;
> > > > >
> > > > > +   void setExternalBuffer(FrameBuffer *buffer);
> > > > > +   void removeExternalBuffer(FrameBuffer *buffer);
> > > > > +
> > > > >     int prepareBuffers(unsigned int count);
> > > > >     int queueBuffer(FrameBuffer *buffer);
> > > > >     void returnBuffer(FrameBuffer *buffer);
> > > > >
> > > >
> > > > --
> > > > Regards
> > > > --
> > > > Kieran
> > > > _______________________________________________
> > > > libcamera-devel mailing list
> > > > libcamera-devel@lists.libcamera.org
> > > > https://lists.libcamera.org/listinfo/libcamera-devel
> > >
> > > --
> > > Regards,
> > > Niklas Söderlund
>
> --
> Regards,
> Niklas Söderlund

Hi Naush,

On Tue, Sep 15, 2020 at 06:40:00PM +0100, Naushir Patuck wrote:
> On Tue, 15 Sep 2020 at 18:31, Niklas Söderlund wrote:
> > On 2020-09-15 16:18:07 +0100, Naushir Patuck wrote:
> > > On Tue, 15 Sep 2020 at 15:55, Niklas Söderlund wrote:
> > > > On 2020-09-15 14:59:23 +0100, Kieran Bingham wrote:
> > > > > On 08/09/2020 08:49, 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 the stream internal buffer list to
> > > > > > identify when used.
> > > > > >
> > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > > > > ---
> > > > > >  include/libcamera/ipa/raspberrypi.h           | 11 ++--
> > > > > >  src/ipa/raspberrypi/raspberrypi.cpp           |  6 +-
> > > > > >  .../pipeline/raspberrypi/raspberrypi.cpp      | 64 +++++++++++++------
> > > > > >  .../pipeline/raspberrypi/rpi_stream.cpp       | 30 ++++++++-
> > > > > >  .../pipeline/raspberrypi/rpi_stream.h         |  3 +
> > > > > >  5 files changed, 85 insertions(+), 29 deletions(-)
> > > > > >
> > > > > > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> > > > > > index 262fc6f3..dd6ebeac 100644
> > > > > > --- a/include/libcamera/ipa/raspberrypi.h
> > > > > > +++ b/include/libcamera/ipa/raspberrypi.h
> > > > > > @@ -28,11 +28,12 @@ enum RPiOperations {
> > > > > >     RPI_IPA_EVENT_QUEUE_REQUEST,
> > > > > >  };
> > > > > >
> > > > > > -enum RPiIpaMask {
> > > > > > -   ID              = 0x0ffff,
> > > > > > -   STATS           = 0x10000,
> > > > > > -   EMBEDDED_DATA   = 0x20000,
> > > > > > -   BAYER_DATA      = 0x40000
> > > > > > +enum RPiBufferMask {
> > > > > > +   ID              = 0x00ffff,
> > > > > > +   STATS           = 0x010000,
> > > > > > +   EMBEDDED_DATA   = 0x020000,
> > > > > > +   BAYER_DATA      = 0x040000,
> > > > > > +   EXTERNAL_BUFFER = 0x100000,
> > > > > >  };
> > > > > >
> > > > > >  /* Size of the LS grid allocation. */
> > > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > > index 688d2efc..0555cc4e 100644
> > > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > > @@ -352,7 +352,7 @@ void IPARPi::processEvent(const IPAOperationData &event)
> > > > > >
> > > > > >             IPAOperationData op;
> > > > > >             op.operation = RPI_IPA_ACTION_STATS_METADATA_COMPLETE;
> > > > > > -           op.data = { bufferId & RPiIpaMask::ID };
> > > > > > +           op.data = { bufferId & RPiBufferMask::ID };
> > > > > >             op.controls = { libcameraMetadata_ };
> > > > > >             queueFrameAction.emit(0, op);
> > > > > >             break;
> > > > > > @@ -373,7 +373,7 @@ void IPARPi::processEvent(const IPAOperationData &event)
> > > > > >             /* Ready to push the input buffer into the ISP. */
> > > > > >             IPAOperationData op;
> > > > > >             op.operation = RPI_IPA_ACTION_RUN_ISP;
> > > > > > -           op.data = { bayerbufferId & RPiIpaMask::ID };
> > > > > > +           op.data = { bayerbufferId & RPiBufferMask::ID };
> > > > > >             queueFrameAction.emit(0, op);
> > > > > >             break;
> > > > > >     }
> > > > > > @@ -700,7 +700,7 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)
> > > > > >  {
> > > > > >     IPAOperationData op;
> > > > > >     op.operation = RPI_IPA_ACTION_EMBEDDED_COMPLETE;
> > > > > > -   op.data = { bufferId & RPiIpaMask::ID };
> > > > > > +   op.data = { bufferId & RPiBufferMask::ID };
> > > > > >     queueFrameAction.emit(0, op);
> > > > > >  }
> > > > > >
> > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > index 5621f182..f5e0d1cd 100644
> > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > @@ -151,6 +151,7 @@ public:
> > > > > >
> > > > > >     void clearIncompleteRequests();
> > > > > >     void handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stream);
> > > > > > +   void handleExternalBuffer(FrameBuffer *buffer, RPi::RPiStream *stream);
> > > > > >     void handleState();
> > > > > >
> > > > > >     CameraSensor *sensor_;
> > > > > > @@ -725,23 +726,32 @@ 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->getBufferId(buffer) == -1) {
> > > > > >                     /*
> > > > > > -                    * If no buffer is provided by the request for this stream, we
> > > > > > -                    * queue a nullptr to the stream to signify that it must use an
> > > > > > -                    * internally allocated buffer for this capture request. This
> > > > > > -                    * buffer will not be given back to the application, but is used
> > > > > > -                    * to support the internal pipeline flow.
> > > > > > -                    *
> > > > > > -                    * 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);
> > > > > >             }
> > > > > > +           /*
> > > > > > +            * If no buffer is provided by the request for this stream, we
> > > > > > +            * queue a nullptr to the stream to signify that it must use an
> > > > > > +            * internally allocated buffer for this capture request. This
> > > > > > +            * buffer will not be given back to the application, but is used
> > > > > > +            * to support the internal pipeline flow.
> > > > > > +            *
> > > > > > +            * 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. */
> > > > > > @@ -915,8 +925,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> > > > > >      * 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);
> > > > > > +   mapBuffers(camera, data->isp_[Isp::Stats].getBuffers(), RPiBufferMask::STATS);
> > > > > > +   mapBuffers(camera, data->unicam_[Unicam::Embedded].getBuffers(), RPiBufferMask::EMBEDDED_DATA);
> > > > > >
> > > > > >     return 0;
> > > > > >  }
> > > > > > @@ -1219,7 +1229,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
> > > > > >     if (stream == &isp_[Isp::Stats]) {
> > > > > >             IPAOperationData op;
> > > > > >             op.operation = RPI_IPA_EVENT_SIGNAL_STAT_READY;
> > > > > > -           op.data = { RPiIpaMask::STATS | static_cast<unsigned int>(index) };
> > > > > > +           op.data = { RPiBufferMask::STATS | static_cast<unsigned int>(index) };
> > > > > >             ipa_->processEvent(op);
> > > > > >     } else {
> > > > > >             /* Any other ISP output can be handed back to the application now. */
> > > > > > @@ -1293,6 +1303,11 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stre
> > > > > >             Request *request = requestQueue_.front();
> > > > > >
> > > > > >             if (!dropFrameCount_ && request->findBuffer(stream) == buffer) {
> > > > > > +                   /*
> > > > > > +                    * Check if this is an externally provided buffer, and if
> > > > > > +                    * so, we must stop tracking it in the pipeline handler.
> > > > > > +                    */
> > > > > > +                   handleExternalBuffer(buffer, stream);
> > > > > >                     /*
> > > > > >                      * Tag the buffer as completed, returning it to the
> > > > > >                      * application.
> > > > > > @@ -1311,6 +1326,17 @@ void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stre
> > > > > >     }
> > > > > >  }
> > > > > >
> > > > > > +void RPiCameraData::handleExternalBuffer(FrameBuffer *buffer, RPi::RPiStream *stream)
> > > > > > +{
> > > > > > +   unsigned int id = stream->getBufferId(buffer);
> > > > > > +
> > > > > > +   if (!(id & RPiBufferMask::EXTERNAL_BUFFER))
> > > > > > +           return;
> > > > > > +
> > > > > > +   /* Stop the Stream object from tracking the buffer. */
> > > > > > +   stream->removeExternalBuffer(buffer);
> > > > > > +}
> > > > > > +
> > > > > >  void RPiCameraData::handleState()
> > > > > >  {
> > > > > >     switch (state_) {
> > > > > > @@ -1443,8 +1469,8 @@ void RPiCameraData::tryRunPipeline()
> > > > > >                     << " Embedded buffer id: " << embeddedIndex;
> > > > > >
> > > > > >     op.operation = RPI_IPA_EVENT_SIGNAL_ISP_PREPARE;
> > > > > > -   op.data = { RPiIpaMask::EMBEDDED_DATA | embeddedIndex,
> > > > > > -               RPiIpaMask::BAYER_DATA | bayerIndex };
> > > > > > +   op.data = { RPiBufferMask::EMBEDDED_DATA | embeddedIndex,
> > > > > > +               RPiBufferMask::BAYER_DATA | bayerIndex };
> > > > > >     ipa_->processEvent(op);
> > > > > >  }
> > > > > >
> > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > > > > > index 879e25ba..c09f14c9 100644
> > > > > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > > > > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > > > > > @@ -6,6 +6,8 @@
> > > > > >   */
> > > > > >  #include "rpi_stream.h"
> > > > > >
> > > > > > +#include <libcamera/ipa/raspberrypi.h>
> > > > > > +
> > > > > >  #include "libcamera/internal/log.h"
> > > > > >
> > > > > >  namespace libcamera {
> > > > > > @@ -44,8 +46,13 @@ bool RPiStream::isExternal() const
> > > > > >
> > > > > >  void RPiStream::setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > > > > >  {
> > > > > > -   for (auto const &buffer : *buffers)
> > > > > > -           bufferMap_.emplace(id_++, buffer.get());
> > > > > > +   /* Ensure we are using a sensible number of buffers. */
> > > > > > +   ASSERT(id_ < RPiBufferMask::ID);
> > > > > > +
> > > > > > +   for (auto const &buffer : *buffers) {
> > > > > > +           bufferMap_.emplace(id_ & RPiBufferMask::ID, buffer.get());
> > > > > > +           id_++;
> > > > > > +   }
> > > > >
> > > > > I half wonder if that assert shouldn't come 'after' the updates to the
> > > > > bufferMap to catch if the id_ has overflowed after increase.
> > > > >
> > > > > But maybe this is a tiny corner case and not expected anyway.
> > > > >
> > > > >
> > > > > >  }
> > > > > >
> > > > > >  const BufferMap &RPiStream::getBuffers() const
> > > > > > @@ -68,6 +75,25 @@ int RPiStream::getBufferId(FrameBuffer *buffer) const
> > > > > >     return it->first;
> > > > > >  }
> > > > > >
> > > > > > +void RPiStream::setExternalBuffer(FrameBuffer *buffer)
> > > > > > +{
> > > > > > +   /* Ensure we are using a sensible number of buffers. */
> > > > > > +   ASSERT(id_ < RPiBufferMask::ID);
> > > > > > +
> > > > > > +   bufferMap_.emplace(RPiBufferMask::EXTERNAL_BUFFER | (id_ & RPiBufferMask::ID),
> > > > > > +                      buffer);
> > > > > > +   id_++;
> > > > >
> > > > > Oh, ... is there no reduction of the id_?
> > > > >
> > > > > What happens if an application continually provides an external buffer
> > > > > ... won't that constantly increase this id_ with every frame?
> > > > >
> > > > > Are the ID's re-used? or is it just that the stream doesn't generate a
> > > > > new ID for each usage of an external buffer (i.e. it really would have
> > > > > to be more than 65535 entirely different buffers...)
> > > > >
> > > > > I guess this can be easily validated by setting RPiBufferMask::ID to
> > > > > 0xFF or even 0xF to see if it hits any assertions quickly
> > > > >
> > > > > As long as this doesn't leak and cause an assertion/overflow after
> > > > > 0xFFFF/RPiBufferMask::ID requests are queued ...
> > > >
> > > > This is a very good observation that I missed in my review. Good thing
> > > > you looked at it as I was about to merge this series.
> > > >
> > > > Naush would it be possible to rework this logic slightly to makesure the
> > > > id_ used can rollover while at the same time is not overwriting an
> > > > existing mapping? With this solved I think we are ready to merge this
> > > > series \o/.
> > >
> > > Currently we will hit an assertion if id goes over 0xffff as it is
> > > never decremented.  If this is going to be a problem (and it sounds
> > > like it will be for android that only ever uses external buffers?), I
> > > will think about how to rework this to allow for reusing a previously
> > > returned id.
> >
> > How about something like this,
> >
> >     /* Find next free buffer id. */
> >     unsigned int id = (id_ + 1) % RPiBufferMask::ID;
> >     while (id != id_ && bufferMap_.find(id) != bufferMap_.end()) {
> >         id = (id + 1) % RPiBufferMask::ID;
> >
> >     /* No free id could be found, this really should not happen. */
> >     ASSERT(id != id_);
> >
> >     id_ = id;
> >
> > Of course untested ;-)
> 
> Something along those lines popped into my head as well.  However, I
> worry about the efficiency of map::find() on every frame, across all
> ids if the map becomes dense with a large number of buffers.  Instead,
> perhaps track recyclable ids in a separate queue so we can avoid the
> costly lookup?

Something more efficient than the above code would indeed be nice. Do we
have an upper bound on the number of IDs we will need concurrently ? If
it's low enough a bitmask combined with ffs() (and similar functions)
should be fairly straightforward. Otherwise, "efficient ID allocator"
may be a good query for a search engine :-)

> > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > >
> > > > > > +}
> > > > > > +
> > > > > > +void RPiStream::removeExternalBuffer(FrameBuffer *buffer)
> > > > > > +{
> > > > > > +   int id = getBufferId(buffer);
> > > > > > +
> > > > > > +   /* Ensure we have this buffer in the stream, and it is marked external. */
> > > > > > +   ASSERT(id != -1 && (id & RPiBufferMask::EXTERNAL_BUFFER));
> > > > > > +   bufferMap_.erase(id);
> > > > > > +}
> > > > > > +
> > > > > >  int RPiStream::prepareBuffers(unsigned int count)
> > > > > >  {
> > > > > >     int ret;
> > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > > > > > index ed517c22..8b23c4b2 100644
> > > > > > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > > > > > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > > > > > @@ -51,6 +51,9 @@ public:
> > > > > >     const BufferMap &getBuffers() const;
> > > > > >     int getBufferId(FrameBuffer *buffer) const;
> > > > > >
> > > > > > +   void setExternalBuffer(FrameBuffer *buffer);
> > > > > > +   void removeExternalBuffer(FrameBuffer *buffer);
> > > > > > +
> > > > > >     int prepareBuffers(unsigned int count);
> > > > > >     int queueBuffer(FrameBuffer *buffer);
> > > > > >     void returnBuffer(FrameBuffer *buffer);