[libcamera-devel,v4,6/9] libcamera: pipeline: raspberrypi: Rework stream buffer logic for zero-copy

8868 diff mbox series

Stop using v4l2_videodevice::allocateBuffer() for internal buffers and
instead export/import all buffers. This allows the pipeline to return
any stream buffer requested by the application as zero-copy.

Advertise the Unicam Image stream as the RAW capture stream now.

The RPiStream object now maintains a new list of buffers that are
available to queue into a device. This is needed to distinguish between
FrameBuffers allocated for internal use vs externally provided buffers.
When a Request comes in, if a buffer is not provided for an exported
stream, we re-use a buffer from this list.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 .../pipeline/raspberrypi/raspberrypi.cpp      | 228 ++++++++++--------
 .../pipeline/raspberrypi/rpi_stream.cpp       | 122 +++++++---
 .../pipeline/raspberrypi/rpi_stream.h         |  30 ++-
 3 files changed, 226 insertions(+), 154 deletions(-)

Hi Naush,

On 20/07/2020 10:13, Naushir Patuck wrote:
> Stop using v4l2_videodevice::allocateBuffer() for internal buffers and
> instead export/import all buffers. This allows the pipeline to return
> any stream buffer requested by the application as zero-copy.
> 
> Advertise the Unicam Image stream as the RAW capture stream now.
> 
> The RPiStream object now maintains a new list of buffers that are
> available to queue into a device. This is needed to distinguish between
> FrameBuffers allocated for internal use vs externally provided buffers.
> When a Request comes in, if a buffer is not provided for an exported
> stream, we re-use a buffer from this list.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

A few small nit fixups below but they could possibly be fixed while
applying anyway if you prefer.

I've hit review fatigue though, so I'll look at this again tomorrow.


> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 228 ++++++++++--------
>  .../pipeline/raspberrypi/rpi_stream.cpp       | 122 +++++++---
>  .../pipeline/raspberrypi/rpi_stream.h         |  30 ++-
>  3 files changed, 226 insertions(+), 154 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 8f6a999b..dbc22521 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -187,7 +187,8 @@ private:
>  	void checkRequestCompleted();
>  	void tryRunPipeline();
>  	void tryFlushQueues();
> -	FrameBuffer *updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp, V4L2VideoDevice *dev);
> +	FrameBuffer *updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp,
> +				 RPi::RPiStream *stream);
>  
>  	unsigned int ispOutputCount_;
>  };
> @@ -508,8 +509,15 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  		StreamConfiguration &cfg = config->at(i);
>  
>  		if (isRaw(cfg.pixelFormat)) {
> -			cfg.setStream(&data->isp_[Isp::Input]);
> -			data->isp_[Isp::Input].setExternal(true);
> +			cfg.setStream(&data->unicam_[Unicam::Image]);
> +			/*
> +			 * We must set both Unicam streams as external, even
> +			 * though the application may only request RAW frames.
> +			 * This is because we match timestamps on both streams
> +			 * to synchronise buffers.
> +			 */
> +			data->unicam_[Unicam::Image].setExternal(true);
> +			data->unicam_[Unicam::Embedded].setExternal(true);
>  			continue;
>  		}
>  
> @@ -612,7 +620,7 @@ int PipelineHandlerRPi::exportFrameBuffers(Camera *camera, Stream *stream,
>  	unsigned int count = stream->configuration().bufferCount;
>  	int ret = s->dev()->exportBuffers(count, buffers);
>  
> -	s->setExternalBuffers(buffers);
> +	s->setExportedBuffers(buffers);
>  
>  	return ret;
>  }
> @@ -712,14 +720,23 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)
>  	if (data->state_ == RPiCameraData::State::Stopped)
>  		return -EINVAL;
>  
> -	/* Ensure all external streams have associated buffers! */
> -	for (auto &stream : data->isp_) {
> -		if (!stream.isExternal())
> -			continue;
> +	LOG(RPI, Debug) << "queueRequestDevice: New request.";
>  
> -		if (!request->findBuffer(&stream)) {
> -			LOG(RPI, Error) << "Attempt to queue request with invalid stream.";
> -			return -ENOENT;
> +	/* Push all buffers supplied in the Request to the respective streams. */
> +	for (auto stream : data->streams_) {
> +		if (stream->isExternal()) {


Are all streams now 'external' ?


> +			FrameBuffer *buffer = request->findBuffer(stream);
> +			/*
> +			 * A nullptr in buffer means that we should queue an internally
> +			 * allocated buffer.
> +			 *
> +			 * The below queueBuffer() call will do nothing if there are not
> +			 * enough internal buffers allocated, but this will be handled by
> +			 * queuing the request for buffers in the RPiStream object.
> +			 */
> +			int ret = stream->queueBuffer(buffer);
> +			if (ret)
> +				return ret;
>  		}
>  	}
>  
> @@ -808,12 +825,10 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
>  	/* Initialize the camera properties. */
>  	data->properties_ = data->sensor_->properties();
>  
> -	/*
> -	 * List the available output streams.
> -	 * Currently cannot do Unicam streams!
> -	 */
> +	/*List the available streams an application may request. */

Minor nit, there's a space missing between /* and List ...


>  	std::set<Stream *> streams;
> -	streams.insert(&data->isp_[Isp::Input]);
> +	streams.insert(&data->unicam_[Unicam::Image]);
> +	streams.insert(&data->unicam_[Unicam::Embedded]);
>  	streams.insert(&data->isp_[Isp::Output0]);
>  	streams.insert(&data->isp_[Isp::Output1]);
>  	streams.insert(&data->isp_[Isp::Stats]);
> @@ -831,9 +846,28 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
>  	int ret;
>  
>  	for (auto const stream : data->streams_) {
> -		ret = stream->queueBuffers();
> -		if (ret < 0)
> -			return ret;
> +		if (!stream->isExternal()) {
> +			ret = stream->queueAllBuffers();
> +			if (ret < 0)
> +				return ret;
> +		} else {
> +			/*
> +			 * For external streams, we must queue up a set of internal
> +			 * buffers to handle the number of drop frames requested by
> +			 * the IPA. This is done by passing nullptr in queueBuffer().
> +			 *
> +			 * 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.
> +			 */
> +			unsigned int i;
> +			for (i = 0; i < data->dropFrameCount_; i++) {
> +				int ret = stream->queueBuffer(nullptr);
> +				if (ret)
> +					return ret;
> +			}
> +		}
>  	}
>  
>  	return 0;
> @@ -847,7 +881,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  	/*
>  	 * Decide how many internal buffers to allocate. For now, simply look
>  	 * at how many external buffers will be provided. Will need to improve
> -	 * this logic.
> +	 * this logic. However, we really must have all stream allocate the same

s/stream/streams/

> +	 * number of buffers to simplify error handling in queueRequestDevice().
>  	 */

Does this include Raw streams? I thought that allocates less buffers? Or
perhaps it's the same number of internal buffers, and only 2 extra
external buffers for that case...


>  	unsigned int maxBuffers = 0;
>  	for (const Stream *s : camera->streams())
> @@ -855,33 +890,9 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  			maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);
>  
>  	for (auto const stream : data->streams_) {
> -		if (stream->isExternal() || stream->isImporter()) {
> -			/*
> -			 * If a stream is marked as external reserve memory to
> -			 * prepare to import as many buffers are requested in
> -			 * the stream configuration.
> -			 *
> -			 * If a stream is an internal stream with importer
> -			 * role, reserve as many buffers as possible.
> -			 */
> -			unsigned int count = stream->isExternal()
> -						     ? stream->configuration().bufferCount
> -						     : maxBuffers;
> -			ret = stream->importBuffers(count);
> -			if (ret < 0)
> -				return ret;
> -		} else {
> -			/*
> -			 * If the stream is an internal exporter allocate and
> -			 * export as many buffers as possible to its internal
> -			 * pool.
> -			 */
> -			ret = stream->allocateBuffers(maxBuffers);
> -			if (ret < 0) {
> -				freeBuffers(camera);
> -				return ret;
> -			}
> -		}
> +		ret = stream->prepareBuffers(maxBuffers);
> +		if (ret < 0)
> +			return ret;
>  	}
>  
>  	/*
> @@ -889,7 +900,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  	 * the IPA and RPI_IPA_EVENT_SIGNAL_ISP_PREPARE event.
>  	 */
>  	count = 0;
> -	for (auto const &b : *data->unicam_[Unicam::Image].getBuffers()) {
> +	for (auto const &b : data->unicam_[Unicam::Image].getBuffers()) {
>  		b->setCookie(count++);
>  	}
>  
> @@ -898,14 +909,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>  	 * the IPA.
>  	 */
>  	count = 0;
> -	for (auto const &b : *data->isp_[Isp::Stats].getBuffers()) {
> +	for (auto const &b : data->isp_[Isp::Stats].getBuffers()) {
>  		b->setCookie(count++);
>  		data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | b->cookie(),
>  					      .planes = b->planes() });
>  	}
>  
>  	count = 0;
> -	for (auto const &b : *data->unicam_[Unicam::Embedded].getBuffers()) {
> +	for (auto const &b : data->unicam_[Unicam::Embedded].getBuffers()) {
>  		b->setCookie(count++);
>  		data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | b->cookie(),
>  					      .planes = b->planes() });
> @@ -1066,7 +1077,7 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
>  	switch (action.operation) {
>  	case RPI_IPA_ACTION_STATS_METADATA_COMPLETE: {
>  		unsigned int bufferId = action.data[0];
> -		FrameBuffer *buffer = isp_[Isp::Stats].getBuffers()->at(bufferId).get();
> +		FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);
>  
>  		handleStreamBuffer(buffer, &isp_[Isp::Stats]);
>  		/* Fill the Request metadata buffer with what the IPA has provided */
> @@ -1077,19 +1088,19 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
>  
>  	case RPI_IPA_ACTION_EMBEDDED_COMPLETE: {
>  		unsigned int bufferId = action.data[0];
> -		FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers()->at(bufferId).get();
> +		FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);
>  		handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
>  		break;
>  	}
>  
>  	case RPI_IPA_ACTION_RUN_ISP: {
>  		unsigned int bufferId = action.data[0];
> -		FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers()->at(bufferId).get();
> +		FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);
>  
>  		LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << buffer->cookie()
>  				<< ", timestamp: " << buffer->metadata().timestamp;
>  
> -		isp_[Isp::Input].dev()->queueBuffer(buffer);
> +		isp_[Isp::Input].queueBuffer(buffer);
>  		ispOutputCount_ = 0;
>  		break;
>  	}
> @@ -1190,22 +1201,27 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
>  			<< ", buffer id " << buffer->cookie()
>  			<< ", timestamp: " << buffer->metadata().timestamp;
>  
> -	handleStreamBuffer(buffer, stream);
> -
>  	/*
> -	 * Increment the number of ISP outputs generated.
> -	 * This is needed to track dropped frames.
> +	 * ISP statistics buffer must not be re-queued or sent back to the
> +	 * application until after the IPA signals so.
>  	 */
> -	ispOutputCount_++;
> -
> -	/* If this is a stats output, hand it to the IPA now. */
>  	if (stream == &isp_[Isp::Stats]) {
>  		IPAOperationData op;
>  		op.operation = RPI_IPA_EVENT_SIGNAL_STAT_READY;
>  		op.data = { RPiIpaMask::STATS | buffer->cookie() };
>  		ipa_->processEvent(op);
> +	} else {
> +		/* Any other ISP output can be handed back to the application now. */
> +		handleStreamBuffer(buffer, stream);
>  	}
>  
> +	/*
> +	 * Increment the number of ISP outputs generated.
> +	 * This is needed to track dropped frames.
> +	 */
> +	ispOutputCount_++;
> +
> +
>  	handleState();
>  }
>  
> @@ -1218,8 +1234,12 @@ void RPiCameraData::clearIncompleteRequests()
>  	 */
>  	for (auto const request : requestQueue_) {
>  		for (auto const stream : streams_) {
> -			if (stream->isExternal())
> -				stream->dev()->queueBuffer(request->findBuffer(stream));
> +			if (!stream->isExternal())
> +				continue;
> +
> +			FrameBuffer *buffer = request->findBuffer(stream);
> +			if (buffer)
> +				stream->queueBuffer(buffer);
>  		}
>  	}
>  
> @@ -1248,7 +1268,7 @@ void RPiCameraData::clearIncompleteRequests()
>  			 * Has the buffer already been handed back to the
>  			 * request? If not, do so now.
>  			 */
> -			if (buffer->request())
> +			if (buffer && buffer->request())
>  				pipe_->completeBuffer(camera_, request, buffer);
>  		}
>  
> @@ -1260,30 +1280,24 @@ void RPiCameraData::clearIncompleteRequests()
>  void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stream)
>  {
>  	if (stream->isExternal()) {
> -		if (!dropFrameCount_) {
> -			Request *request = buffer->request();
> +		Request *request = requestQueue_.front();
> +
> +		if (!dropFrameCount_ && request->findBuffer(stream) == buffer) {
> +			/*
> +			 * Tag the buffer as completed, returning it to the
> +			 * application.
> +			 */
>  			pipe_->completeBuffer(camera_, request, buffer);
> +		} else {
> +			/*
> +			 * This buffer was not part of the Request, so we can
> +			 * recycle it.
> +			 */
> +			stream->returnBuffer(buffer);
>  		}
>  	} else {
> -		/* Special handling for RAW buffer Requests.
> -		 *
> -		 * The ISP input stream is alway an import stream, but if the
> -		 * current Request has been made for a buffer on the stream,
> -		 * simply memcpy to the Request buffer and requeue back to the
> -		 * device.
> -		 */
> -		if (stream == &unicam_[Unicam::Image] && !dropFrameCount_) {
> -			const Stream *rawStream = static_cast<const Stream *>(&isp_[Isp::Input]);
> -			Request *request = requestQueue_.front();
> -			FrameBuffer *raw = request->findBuffer(const_cast<Stream *>(rawStream));
> -			if (raw) {
> -				raw->copyFrom(buffer);
> -				pipe_->completeBuffer(camera_, request, raw);
> -			}
> -		}
> -
> -		/* Simply requeue the buffer. */
> -		stream->dev()->queueBuffer(buffer);
> +		/* Simply re-queue the buffer to the requested stream. */
> +		stream->queueBuffer(buffer);
>  	}
>  }
>  
> @@ -1367,7 +1381,7 @@ void RPiCameraData::tryRunPipeline()
>  	 * current bayer buffer will be removed and re-queued to the driver.
>  	 */
>  	embeddedBuffer = updateQueue(embeddedQueue_, bayerBuffer->metadata().timestamp,
> -				     unicam_[Unicam::Embedded].dev());
> +				     &unicam_[Unicam::Embedded]);
>  
>  	if (!embeddedBuffer) {
>  		LOG(RPI, Debug) << "Could not find matching embedded buffer";
> @@ -1386,7 +1400,7 @@ void RPiCameraData::tryRunPipeline()
>  
>  		embeddedBuffer = embeddedQueue_.front();
>  		bayerBuffer = updateQueue(bayerQueue_, embeddedBuffer->metadata().timestamp,
> -					  unicam_[Unicam::Image].dev());
> +					  &unicam_[Unicam::Image]);
>  
>  		if (!bayerBuffer) {
>  			LOG(RPI, Debug) << "Could not find matching bayer buffer - ending.";
> @@ -1394,11 +1408,7 @@ void RPiCameraData::tryRunPipeline()
>  		}
>  	}
>  
> -	/*
> -	 * Take the first request from the queue and action the IPA.
> -	 * Unicam buffers for the request have already been queued as they come
> -	 * in.
> -	 */
> +	/* Take the first request from the queue and action the IPA. */
>  	Request *request = requestQueue_.front();
>  
>  	/*
> @@ -1410,12 +1420,6 @@ void RPiCameraData::tryRunPipeline()
>  	op.controls = { request->controls() };
>  	ipa_->processEvent(op);
>  
> -	/* Queue up any ISP buffers passed into the request. */
> -	for (auto &stream : isp_) {
> -		if (stream.isExternal())
> -			stream.dev()->queueBuffer(request->findBuffer(&stream));
> -	}
> -
>  	/* Ready to use the buffers, pop them off the queue. */
>  	bayerQueue_.pop();
>  	embeddedQueue_.pop();
> @@ -1445,32 +1449,42 @@ void RPiCameraData::tryFlushQueues()
>  	 * and give a chance for the hardware to return to lock-step. We do have
>  	 * to drop all interim frames.
>  	 */
> -	if (unicam_[Unicam::Image].getBuffers()->size() == bayerQueue_.size() &&
> -	    unicam_[Unicam::Embedded].getBuffers()->size() == embeddedQueue_.size()) {
> +	if (unicam_[Unicam::Image].getBuffers().size() == bayerQueue_.size() &&
> +	    unicam_[Unicam::Embedded].getBuffers().size() == embeddedQueue_.size()) {
> +		/* This cannot happen when Unicam streams are external. */
> +		assert(!unicam_[Unicam::Image].isExternal());
> +
>  		LOG(RPI, Warning) << "Flushing all buffer queues!";
>  
>  		while (!bayerQueue_.empty()) {
> -			unicam_[Unicam::Image].dev()->queueBuffer(bayerQueue_.front());
> +			unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());
>  			bayerQueue_.pop();
>  		}
>  
>  		while (!embeddedQueue_.empty()) {
> -			unicam_[Unicam::Embedded].dev()->queueBuffer(embeddedQueue_.front());
> +			unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());
>  			embeddedQueue_.pop();
>  		}
>  	}
>  }
>  
>  FrameBuffer *RPiCameraData::updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp,
> -					V4L2VideoDevice *dev)
> +					RPi::RPiStream *stream)
>  {
> +	/*
> +	 * If the unicam streams are external (both have to the same), then we

both have to 'be' the same?

> +	 * can only return out the top buffer in the queue, and assume they have
> +	 * been synced by queuing at the same time. We cannot drop these frames,
> +	 * as they may have been provided externally.
> +	 */
>  	while (!q.empty()) {
>  		FrameBuffer *b = q.front();
> -		if (b->metadata().timestamp < timestamp) {
> +		if (!stream->isExternal() && b->metadata().timestamp < timestamp) {
>  			q.pop();
> -			dev->queueBuffer(b);
> -			LOG(RPI, Warning) << "Dropping input frame!";
> -		} else if (b->metadata().timestamp == timestamp) {
> +			stream->queueBuffer(b);
> +			LOG(RPI, Warning) << "Dropping unmatched input frame in stream "
> +					  << stream->name();
> +		} else if (stream->isExternal() || b->metadata().timestamp == timestamp) {
>  			/* The calling function will pop the item from the queue. */
>  			return b;
>  		} else {
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> index 2edb8b59..02f8d3e0 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> @@ -21,30 +21,20 @@ V4L2VideoDevice *RPiStream::dev() const
>  
>  void RPiStream::setExternal(bool external)
>  {
> +	/* Import streams cannot be external. */
> +	assert(!external || !importOnly_);
>  	external_ = external;
>  }
>  
>  bool RPiStream::isExternal() const
>  {
> -	/*
> -	 * Import streams cannot be external.
> -	 *
> -	 * RAW capture is a special case where we simply copy the RAW
> -	 * buffer out of the request. All other buffer handling happens
> -	 * as if the stream is internal.
> -	 */
> -	return external_ && !importOnly_;
> -}
> -
> -bool RPiStream::isImporter() const
> -{
> -	return importOnly_;
> +	return external_;
>  }
>  
>  void RPiStream::reset()
>  {
>  	external_ = false;
> -	internalBuffers_.clear();
> +	clearBuffers();
>  }
>  
>  std::string RPiStream::name() const
> @@ -52,65 +42,123 @@ std::string RPiStream::name() const
>  	return name_;
>  }
>  
> -void RPiStream::setExternalBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> +void RPiStream::setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>  {
> -	externalBuffers_ = buffers;
> +	std::transform(buffers->begin(), buffers->end(), std::back_inserter(bufferList_),
> +		       [](std::unique_ptr<FrameBuffer> &b) { return b.get(); });
>  }
>  
> -const std::vector<std::unique_ptr<FrameBuffer>> *RPiStream::getBuffers() const
> +const std::vector<FrameBuffer *> &RPiStream::getBuffers() const
>  {
> -	return external_ ? externalBuffers_ : &internalBuffers_;
> +	return bufferList_;
>  }
>  
>  void RPiStream::releaseBuffers()
>  {
>  	dev_->releaseBuffers();
> -	if (!external_ && !importOnly_)
> -		internalBuffers_.clear();
> +	clearBuffers();
>  }
>  
> -int RPiStream::importBuffers(unsigned int count)
> +int RPiStream::prepareBuffers(unsigned int count)
>  {
> +	int ret;
> +
> +	if (!importOnly_) {
> +		if (count) {
> +			/* Export some frame buffers for internal use. */
> +			ret = dev_->exportBuffers(count, &internalBuffers_);
> +			if (ret < 0)
> +				return ret;
> +
> +			/* Add these exported buffers to the internal/external buffer list. */
> +			setExportedBuffers(&internalBuffers_);
> +
> +			/* Add these buffers to the queue of internal usable buffers. */
> +			for (auto const &buffer : internalBuffers_)
> +				availableBuffers_.push(buffer.get());
> +		}
> +
> +		/* We must import all internal/external exported buffers. */
> +		count = bufferList_.size();
> +	}
> +
>  	return dev_->importBuffers(count);
>  }
>  
> -int RPiStream::allocateBuffers(unsigned int count)
> +int RPiStream::queueAllBuffers()
>  {
> -	return dev_->allocateBuffers(count, &internalBuffers_);
> -}
> +	int ret;
>  
> -int RPiStream::queueBuffers()
> -{
>  	if (external_)
>  		return 0;
>  
> -	for (auto &b : internalBuffers_) {
> -		int ret = dev_->queueBuffer(b.get());
> -		if (ret) {
> -			LOG(RPISTREAM, Error) << "Failed to queue buffers for "
> -					      << name_;
> +	while (!availableBuffers_.empty()) {
> +		ret = queueBuffer(availableBuffers_.front());
> +		if (ret < 0)
>  			return ret;
> -		}
> +
> +		availableBuffers_.pop();
>  	}
>  
>  	return 0;
>  }
>  
> -bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const
> +int RPiStream::queueBuffer(FrameBuffer *buffer)
> +{
> +	/*
> +	 * A nullptr buffer implies an external stream, but no external
> +	 * buffer has been supplied. So, pick one from the availableBuffers_
> +	 * queue.
> +	 */
> +	if (!buffer) {
> +		if (availableBuffers_.empty()) {
> +			LOG(RPISTREAM, Warning) << "No buffers available for "
> +						<< name_;
> +			return -EINVAL;
> +		}
> +
> +		buffer = availableBuffers_.front();
> +		availableBuffers_.pop();
> +	}
> +
> +	LOG(RPISTREAM, Debug) << "Queuing buffer " << buffer->cookie()
> +			      << " for " << name_;
> +
> +	int ret = dev_->queueBuffer(buffer);
> +	if (ret) {
> +		LOG(RPISTREAM, Error) << "Failed to queue buffer for "
> +				      << name_;
> +	}
> +
> +	return ret;
> +}
> +
> +void RPiStream::returnBuffer(FrameBuffer *buffer)
>  {
> -	auto start = external_ ? externalBuffers_->begin() : internalBuffers_.begin();
> -	auto end = external_ ? externalBuffers_->end() : internalBuffers_.end();
> +	/* This can only be called for external streams. */
> +	assert(external_);
> +
> +	availableBuffers_.push(buffer);
> +}
>  
> +bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const
> +{
>  	if (importOnly_)
>  		return false;
>  
> -	if (std::find_if(start, end,
> -			 [buffer](std::unique_ptr<FrameBuffer> const &ref) { return ref.get() == buffer; }) != end)
> +	if (std::find(bufferList_.begin(), bufferList_.end(), buffer) != bufferList_.end())
>  		return true;
>  
>  	return false;
>  }
>  
> +void RPiStream::clearBuffers()
> +{
> +	availableBuffers_ = std::queue<FrameBuffer *>{};
> +	internalBuffers_.clear();
> +	bufferList_.clear();
> +}
> +
>  } /* namespace RPi */
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> index 3957e342..af9c2ad2 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> @@ -39,21 +39,22 @@ public:
>  	V4L2VideoDevice *dev() const;
>  	void setExternal(bool external);
>  	bool isExternal() const;
> -	bool isImporter() const;
>  	void reset();
>  	std::string name() const;
> -	void setExternalBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> -	const std::vector<std::unique_ptr<FrameBuffer>> *getBuffers() const;
> +	void setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> +	const std::vector<FrameBuffer *> &getBuffers() const;
>  	void releaseBuffers();
> -	int importBuffers(unsigned int count);
> -	int allocateBuffers(unsigned int count);
> -	int queueBuffers();
> +	int prepareBuffers(unsigned int count);
> +	int queueAllBuffers();
> +	int queueBuffer(FrameBuffer *buffer);
> +	void returnBuffer(FrameBuffer *buffer);
>  	bool findFrameBuffer(FrameBuffer *buffer) const;


Perhaps we should break those up into sections in patch 1/9. It's hard
to see the wood-for-the-trees in the wall of function declarations.


>  
>  private:
> +	void clearBuffers();
>  	/*
>  	 * Indicates that this stream is active externally, i.e. the buffers
> -	 * are provided by the application.
> +	 * might be provided by (and returned to) the application.
>  	 */
>  	bool external_;
>  	/* Indicates that this stream only imports buffers, e.g. ISP input. */
> @@ -62,10 +63,19 @@ private:
>  	std::string name_;
>  	/* The actual device stream. */
>  	std::unique_ptr<V4L2VideoDevice> dev_;
> -	/* Internally allocated framebuffers associated with this device stream. */
> +	/* All framebuffers associated with this device stream. */
> +	std::vector<FrameBuffer *> bufferList_;
> +	/*
> +	 * List of frame buffer that we can use if none have been provided by
> +	 * the application for external streams. This is populated by the
> +	 * buffers exported internally.
> +	 */
> +	std::queue<FrameBuffer *> availableBuffers_;
> +	/*
> +	 * This is a list of buffers exported internally. Need to keep this around
> +	 * as the stream needs to maintain ownership of these buffers.
> +	 */
>  	std::vector<std::unique_ptr<FrameBuffer>> internalBuffers_;
> -	/* Externally allocated framebuffers associated with this device stream. */
> -	std::vector<std::unique_ptr<FrameBuffer>> *externalBuffers_;

Likewise here, some whitespace to break up these members would really
help, as by the time I've got here I've got definite review fatigue and
lots of lines close together just become a blur ... :S


>  };
>  
>  /*
>

Hi Kieran,

Thank you for the review.

On Tue, 21 Jul 2020 at 16:41, Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Hi Naush,
>
> On 20/07/2020 10:13, Naushir Patuck wrote:
> > Stop using v4l2_videodevice::allocateBuffer() for internal buffers and
> > instead export/import all buffers. This allows the pipeline to return
> > any stream buffer requested by the application as zero-copy.
> >
> > Advertise the Unicam Image stream as the RAW capture stream now.
> >
> > The RPiStream object now maintains a new list of buffers that are
> > available to queue into a device. This is needed to distinguish between
> > FrameBuffers allocated for internal use vs externally provided buffers.
> > When a Request comes in, if a buffer is not provided for an exported
> > stream, we re-use a buffer from this list.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
>
> A few small nit fixups below but they could possibly be fixed while
> applying anyway if you prefer.
>
> I've hit review fatigue though, so I'll look at this again tomorrow.

I understand :)
I will make the necessary fixups based on your review comments, and
submit a new patch set.  Will wait until you have finished reviewing
this one before I send the next revision.

>
>
> > ---
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 228 ++++++++++--------
> >  .../pipeline/raspberrypi/rpi_stream.cpp       | 122 +++++++---
> >  .../pipeline/raspberrypi/rpi_stream.h         |  30 ++-
> >  3 files changed, 226 insertions(+), 154 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 8f6a999b..dbc22521 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -187,7 +187,8 @@ private:
> >       void checkRequestCompleted();
> >       void tryRunPipeline();
> >       void tryFlushQueues();
> > -     FrameBuffer *updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp, V4L2VideoDevice *dev);
> > +     FrameBuffer *updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp,
> > +                              RPi::RPiStream *stream);
> >
> >       unsigned int ispOutputCount_;
> >  };
> > @@ -508,8 +509,15 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> >               StreamConfiguration &cfg = config->at(i);
> >
> >               if (isRaw(cfg.pixelFormat)) {
> > -                     cfg.setStream(&data->isp_[Isp::Input]);
> > -                     data->isp_[Isp::Input].setExternal(true);
> > +                     cfg.setStream(&data->unicam_[Unicam::Image]);
> > +                     /*
> > +                      * We must set both Unicam streams as external, even
> > +                      * though the application may only request RAW frames.
> > +                      * This is because we match timestamps on both streams
> > +                      * to synchronise buffers.
> > +                      */
> > +                     data->unicam_[Unicam::Image].setExternal(true);
> > +                     data->unicam_[Unicam::Embedded].setExternal(true);
> >                       continue;
> >               }
> >
> > @@ -612,7 +620,7 @@ int PipelineHandlerRPi::exportFrameBuffers(Camera *camera, Stream *stream,
> >       unsigned int count = stream->configuration().bufferCount;
> >       int ret = s->dev()->exportBuffers(count, buffers);
> >
> > -     s->setExternalBuffers(buffers);
> > +     s->setExportedBuffers(buffers);
> >
> >       return ret;
> >  }
> > @@ -712,14 +720,23 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)
> >       if (data->state_ == RPiCameraData::State::Stopped)
> >               return -EINVAL;
> >
> > -     /* Ensure all external streams have associated buffers! */
> > -     for (auto &stream : data->isp_) {
> > -             if (!stream.isExternal())
> > -                     continue;
> > +     LOG(RPI, Debug) << "queueRequestDevice: New request.";
> >
> > -             if (!request->findBuffer(&stream)) {
> > -                     LOG(RPI, Error) << "Attempt to queue request with invalid stream.";
> > -                     return -ENOENT;
> > +     /* Push all buffers supplied in the Request to the respective streams. */
> > +     for (auto stream : data->streams_) {
> > +             if (stream->isExternal()) {
>
>
> Are all streams now 'external' ?

All streams but the ISP input (which only imports buffers) *can* be
external.  They are marked external only if the app configures the
pipeline handler by saying it may request a buffer from the stream.
In reality, all streams could be marked external all the time, but the
buffer handling will take a slightly less efficient path, so I kept
the distinction.

>
>
> > +                     FrameBuffer *buffer = request->findBuffer(stream);
> > +                     /*
> > +                      * A nullptr in buffer means that we should queue an internally
> > +                      * allocated buffer.
> > +                      *
> > +                      * The below queueBuffer() call will do nothing if there are not
> > +                      * enough internal buffers allocated, but this will be handled by
> > +                      * queuing the request for buffers in the RPiStream object.
> > +                      */
> > +                     int ret = stream->queueBuffer(buffer);
> > +                     if (ret)
> > +                             return ret;
> >               }
> >       }
> >
> > @@ -808,12 +825,10 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
> >       /* Initialize the camera properties. */
> >       data->properties_ = data->sensor_->properties();
> >
> > -     /*
> > -      * List the available output streams.
> > -      * Currently cannot do Unicam streams!
> > -      */
> > +     /*List the available streams an application may request. */
>
> Minor nit, there's a space missing between /* and List ...
>
>
> >       std::set<Stream *> streams;
> > -     streams.insert(&data->isp_[Isp::Input]);
> > +     streams.insert(&data->unicam_[Unicam::Image]);
> > +     streams.insert(&data->unicam_[Unicam::Embedded]);
> >       streams.insert(&data->isp_[Isp::Output0]);
> >       streams.insert(&data->isp_[Isp::Output1]);
> >       streams.insert(&data->isp_[Isp::Stats]);
> > @@ -831,9 +846,28 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
> >       int ret;
> >
> >       for (auto const stream : data->streams_) {
> > -             ret = stream->queueBuffers();
> > -             if (ret < 0)
> > -                     return ret;
> > +             if (!stream->isExternal()) {
> > +                     ret = stream->queueAllBuffers();
> > +                     if (ret < 0)
> > +                             return ret;
> > +             } else {
> > +                     /*
> > +                      * For external streams, we must queue up a set of internal
> > +                      * buffers to handle the number of drop frames requested by
> > +                      * the IPA. This is done by passing nullptr in queueBuffer().
> > +                      *
> > +                      * 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.
> > +                      */
> > +                     unsigned int i;
> > +                     for (i = 0; i < data->dropFrameCount_; i++) {
> > +                             int ret = stream->queueBuffer(nullptr);
> > +                             if (ret)
> > +                                     return ret;
> > +                     }
> > +             }
> >       }
> >
> >       return 0;
> > @@ -847,7 +881,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> >       /*
> >        * Decide how many internal buffers to allocate. For now, simply look
> >        * at how many external buffers will be provided. Will need to improve
> > -      * this logic.
> > +      * this logic. However, we really must have all stream allocate the same
>
> s/stream/streams/
>
> > +      * number of buffers to simplify error handling in queueRequestDevice().
> >        */
>
> Does this include Raw streams? I thought that allocates less buffers? Or
> perhaps it's the same number of internal buffers, and only 2 extra
> external buffers for that case...

That's correct.  For now all (including Raw) streams allocate the same
number of internal buffers.  Hence the comment that we could improve
this logic, it is a bit wasteful on memory.  This bit does need
refinement at some point.

>
>
> >       unsigned int maxBuffers = 0;
> >       for (const Stream *s : camera->streams())
> > @@ -855,33 +890,9 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> >                       maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);
> >
> >       for (auto const stream : data->streams_) {
> > -             if (stream->isExternal() || stream->isImporter()) {
> > -                     /*
> > -                      * If a stream is marked as external reserve memory to
> > -                      * prepare to import as many buffers are requested in
> > -                      * the stream configuration.
> > -                      *
> > -                      * If a stream is an internal stream with importer
> > -                      * role, reserve as many buffers as possible.
> > -                      */
> > -                     unsigned int count = stream->isExternal()
> > -                                                  ? stream->configuration().bufferCount
> > -                                                  : maxBuffers;
> > -                     ret = stream->importBuffers(count);
> > -                     if (ret < 0)
> > -                             return ret;
> > -             } else {
> > -                     /*
> > -                      * If the stream is an internal exporter allocate and
> > -                      * export as many buffers as possible to its internal
> > -                      * pool.
> > -                      */
> > -                     ret = stream->allocateBuffers(maxBuffers);
> > -                     if (ret < 0) {
> > -                             freeBuffers(camera);
> > -                             return ret;
> > -                     }
> > -             }
> > +             ret = stream->prepareBuffers(maxBuffers);
> > +             if (ret < 0)
> > +                     return ret;
> >       }
> >
> >       /*
> > @@ -889,7 +900,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> >        * the IPA and RPI_IPA_EVENT_SIGNAL_ISP_PREPARE event.
> >        */
> >       count = 0;
> > -     for (auto const &b : *data->unicam_[Unicam::Image].getBuffers()) {
> > +     for (auto const &b : data->unicam_[Unicam::Image].getBuffers()) {
> >               b->setCookie(count++);
> >       }
> >
> > @@ -898,14 +909,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> >        * the IPA.
> >        */
> >       count = 0;
> > -     for (auto const &b : *data->isp_[Isp::Stats].getBuffers()) {
> > +     for (auto const &b : data->isp_[Isp::Stats].getBuffers()) {
> >               b->setCookie(count++);
> >               data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | b->cookie(),
> >                                             .planes = b->planes() });
> >       }
> >
> >       count = 0;
> > -     for (auto const &b : *data->unicam_[Unicam::Embedded].getBuffers()) {
> > +     for (auto const &b : data->unicam_[Unicam::Embedded].getBuffers()) {
> >               b->setCookie(count++);
> >               data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | b->cookie(),
> >                                             .planes = b->planes() });
> > @@ -1066,7 +1077,7 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
> >       switch (action.operation) {
> >       case RPI_IPA_ACTION_STATS_METADATA_COMPLETE: {
> >               unsigned int bufferId = action.data[0];
> > -             FrameBuffer *buffer = isp_[Isp::Stats].getBuffers()->at(bufferId).get();
> > +             FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);
> >
> >               handleStreamBuffer(buffer, &isp_[Isp::Stats]);
> >               /* Fill the Request metadata buffer with what the IPA has provided */
> > @@ -1077,19 +1088,19 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
> >
> >       case RPI_IPA_ACTION_EMBEDDED_COMPLETE: {
> >               unsigned int bufferId = action.data[0];
> > -             FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers()->at(bufferId).get();
> > +             FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);
> >               handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
> >               break;
> >       }
> >
> >       case RPI_IPA_ACTION_RUN_ISP: {
> >               unsigned int bufferId = action.data[0];
> > -             FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers()->at(bufferId).get();
> > +             FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);
> >
> >               LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << buffer->cookie()
> >                               << ", timestamp: " << buffer->metadata().timestamp;
> >
> > -             isp_[Isp::Input].dev()->queueBuffer(buffer);
> > +             isp_[Isp::Input].queueBuffer(buffer);
> >               ispOutputCount_ = 0;
> >               break;
> >       }
> > @@ -1190,22 +1201,27 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
> >                       << ", buffer id " << buffer->cookie()
> >                       << ", timestamp: " << buffer->metadata().timestamp;
> >
> > -     handleStreamBuffer(buffer, stream);
> > -
> >       /*
> > -      * Increment the number of ISP outputs generated.
> > -      * This is needed to track dropped frames.
> > +      * ISP statistics buffer must not be re-queued or sent back to the
> > +      * application until after the IPA signals so.
> >        */
> > -     ispOutputCount_++;
> > -
> > -     /* If this is a stats output, hand it to the IPA now. */
> >       if (stream == &isp_[Isp::Stats]) {
> >               IPAOperationData op;
> >               op.operation = RPI_IPA_EVENT_SIGNAL_STAT_READY;
> >               op.data = { RPiIpaMask::STATS | buffer->cookie() };
> >               ipa_->processEvent(op);
> > +     } else {
> > +             /* Any other ISP output can be handed back to the application now. */
> > +             handleStreamBuffer(buffer, stream);
> >       }
> >
> > +     /*
> > +      * Increment the number of ISP outputs generated.
> > +      * This is needed to track dropped frames.
> > +      */
> > +     ispOutputCount_++;
> > +
> > +
> >       handleState();
> >  }
> >
> > @@ -1218,8 +1234,12 @@ void RPiCameraData::clearIncompleteRequests()
> >        */
> >       for (auto const request : requestQueue_) {
> >               for (auto const stream : streams_) {
> > -                     if (stream->isExternal())
> > -                             stream->dev()->queueBuffer(request->findBuffer(stream));
> > +                     if (!stream->isExternal())
> > +                             continue;
> > +
> > +                     FrameBuffer *buffer = request->findBuffer(stream);
> > +                     if (buffer)
> > +                             stream->queueBuffer(buffer);
> >               }
> >       }
> >
> > @@ -1248,7 +1268,7 @@ void RPiCameraData::clearIncompleteRequests()
> >                        * Has the buffer already been handed back to the
> >                        * request? If not, do so now.
> >                        */
> > -                     if (buffer->request())
> > +                     if (buffer && buffer->request())
> >                               pipe_->completeBuffer(camera_, request, buffer);
> >               }
> >
> > @@ -1260,30 +1280,24 @@ void RPiCameraData::clearIncompleteRequests()
> >  void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stream)
> >  {
> >       if (stream->isExternal()) {
> > -             if (!dropFrameCount_) {
> > -                     Request *request = buffer->request();
> > +             Request *request = requestQueue_.front();
> > +
> > +             if (!dropFrameCount_ && request->findBuffer(stream) == buffer) {
> > +                     /*
> > +                      * Tag the buffer as completed, returning it to the
> > +                      * application.
> > +                      */
> >                       pipe_->completeBuffer(camera_, request, buffer);
> > +             } else {
> > +                     /*
> > +                      * This buffer was not part of the Request, so we can
> > +                      * recycle it.
> > +                      */
> > +                     stream->returnBuffer(buffer);
> >               }
> >       } else {
> > -             /* Special handling for RAW buffer Requests.
> > -              *
> > -              * The ISP input stream is alway an import stream, but if the
> > -              * current Request has been made for a buffer on the stream,
> > -              * simply memcpy to the Request buffer and requeue back to the
> > -              * device.
> > -              */
> > -             if (stream == &unicam_[Unicam::Image] && !dropFrameCount_) {
> > -                     const Stream *rawStream = static_cast<const Stream *>(&isp_[Isp::Input]);
> > -                     Request *request = requestQueue_.front();
> > -                     FrameBuffer *raw = request->findBuffer(const_cast<Stream *>(rawStream));
> > -                     if (raw) {
> > -                             raw->copyFrom(buffer);
> > -                             pipe_->completeBuffer(camera_, request, raw);
> > -                     }
> > -             }
> > -
> > -             /* Simply requeue the buffer. */
> > -             stream->dev()->queueBuffer(buffer);
> > +             /* Simply re-queue the buffer to the requested stream. */
> > +             stream->queueBuffer(buffer);
> >       }
> >  }
> >
> > @@ -1367,7 +1381,7 @@ void RPiCameraData::tryRunPipeline()
> >        * current bayer buffer will be removed and re-queued to the driver.
> >        */
> >       embeddedBuffer = updateQueue(embeddedQueue_, bayerBuffer->metadata().timestamp,
> > -                                  unicam_[Unicam::Embedded].dev());
> > +                                  &unicam_[Unicam::Embedded]);
> >
> >       if (!embeddedBuffer) {
> >               LOG(RPI, Debug) << "Could not find matching embedded buffer";
> > @@ -1386,7 +1400,7 @@ void RPiCameraData::tryRunPipeline()
> >
> >               embeddedBuffer = embeddedQueue_.front();
> >               bayerBuffer = updateQueue(bayerQueue_, embeddedBuffer->metadata().timestamp,
> > -                                       unicam_[Unicam::Image].dev());
> > +                                       &unicam_[Unicam::Image]);
> >
> >               if (!bayerBuffer) {
> >                       LOG(RPI, Debug) << "Could not find matching bayer buffer - ending.";
> > @@ -1394,11 +1408,7 @@ void RPiCameraData::tryRunPipeline()
> >               }
> >       }
> >
> > -     /*
> > -      * Take the first request from the queue and action the IPA.
> > -      * Unicam buffers for the request have already been queued as they come
> > -      * in.
> > -      */
> > +     /* Take the first request from the queue and action the IPA. */
> >       Request *request = requestQueue_.front();
> >
> >       /*
> > @@ -1410,12 +1420,6 @@ void RPiCameraData::tryRunPipeline()
> >       op.controls = { request->controls() };
> >       ipa_->processEvent(op);
> >
> > -     /* Queue up any ISP buffers passed into the request. */
> > -     for (auto &stream : isp_) {
> > -             if (stream.isExternal())
> > -                     stream.dev()->queueBuffer(request->findBuffer(&stream));
> > -     }
> > -
> >       /* Ready to use the buffers, pop them off the queue. */
> >       bayerQueue_.pop();
> >       embeddedQueue_.pop();
> > @@ -1445,32 +1449,42 @@ void RPiCameraData::tryFlushQueues()
> >        * and give a chance for the hardware to return to lock-step. We do have
> >        * to drop all interim frames.
> >        */
> > -     if (unicam_[Unicam::Image].getBuffers()->size() == bayerQueue_.size() &&
> > -         unicam_[Unicam::Embedded].getBuffers()->size() == embeddedQueue_.size()) {
> > +     if (unicam_[Unicam::Image].getBuffers().size() == bayerQueue_.size() &&
> > +         unicam_[Unicam::Embedded].getBuffers().size() == embeddedQueue_.size()) {
> > +             /* This cannot happen when Unicam streams are external. */
> > +             assert(!unicam_[Unicam::Image].isExternal());
> > +
> >               LOG(RPI, Warning) << "Flushing all buffer queues!";
> >
> >               while (!bayerQueue_.empty()) {
> > -                     unicam_[Unicam::Image].dev()->queueBuffer(bayerQueue_.front());
> > +                     unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());
> >                       bayerQueue_.pop();
> >               }
> >
> >               while (!embeddedQueue_.empty()) {
> > -                     unicam_[Unicam::Embedded].dev()->queueBuffer(embeddedQueue_.front());
> > +                     unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());
> >                       embeddedQueue_.pop();
> >               }
> >       }
> >  }
> >
> >  FrameBuffer *RPiCameraData::updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp,
> > -                                     V4L2VideoDevice *dev)
> > +                                     RPi::RPiStream *stream)
> >  {
> > +     /*
> > +      * If the unicam streams are external (both have to the same), then we
>
> both have to 'be' the same?
>
> > +      * can only return out the top buffer in the queue, and assume they have
> > +      * been synced by queuing at the same time. We cannot drop these frames,
> > +      * as they may have been provided externally.
> > +      */
> >       while (!q.empty()) {
> >               FrameBuffer *b = q.front();
> > -             if (b->metadata().timestamp < timestamp) {
> > +             if (!stream->isExternal() && b->metadata().timestamp < timestamp) {
> >                       q.pop();
> > -                     dev->queueBuffer(b);
> > -                     LOG(RPI, Warning) << "Dropping input frame!";
> > -             } else if (b->metadata().timestamp == timestamp) {
> > +                     stream->queueBuffer(b);
> > +                     LOG(RPI, Warning) << "Dropping unmatched input frame in stream "
> > +                                       << stream->name();
> > +             } else if (stream->isExternal() || b->metadata().timestamp == timestamp) {
> >                       /* The calling function will pop the item from the queue. */
> >                       return b;
> >               } else {
> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > index 2edb8b59..02f8d3e0 100644
> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > @@ -21,30 +21,20 @@ V4L2VideoDevice *RPiStream::dev() const
> >
> >  void RPiStream::setExternal(bool external)
> >  {
> > +     /* Import streams cannot be external. */
> > +     assert(!external || !importOnly_);
> >       external_ = external;
> >  }
> >
> >  bool RPiStream::isExternal() const
> >  {
> > -     /*
> > -      * Import streams cannot be external.
> > -      *
> > -      * RAW capture is a special case where we simply copy the RAW
> > -      * buffer out of the request. All other buffer handling happens
> > -      * as if the stream is internal.
> > -      */
> > -     return external_ && !importOnly_;
> > -}
> > -
> > -bool RPiStream::isImporter() const
> > -{
> > -     return importOnly_;
> > +     return external_;
> >  }
> >
> >  void RPiStream::reset()
> >  {
> >       external_ = false;
> > -     internalBuffers_.clear();
> > +     clearBuffers();
> >  }
> >
> >  std::string RPiStream::name() const
> > @@ -52,65 +42,123 @@ std::string RPiStream::name() const
> >       return name_;
> >  }
> >
> > -void RPiStream::setExternalBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > +void RPiStream::setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> >  {
> > -     externalBuffers_ = buffers;
> > +     std::transform(buffers->begin(), buffers->end(), std::back_inserter(bufferList_),
> > +                    [](std::unique_ptr<FrameBuffer> &b) { return b.get(); });
> >  }
> >
> > -const std::vector<std::unique_ptr<FrameBuffer>> *RPiStream::getBuffers() const
> > +const std::vector<FrameBuffer *> &RPiStream::getBuffers() const
> >  {
> > -     return external_ ? externalBuffers_ : &internalBuffers_;
> > +     return bufferList_;
> >  }
> >
> >  void RPiStream::releaseBuffers()
> >  {
> >       dev_->releaseBuffers();
> > -     if (!external_ && !importOnly_)
> > -             internalBuffers_.clear();
> > +     clearBuffers();
> >  }
> >
> > -int RPiStream::importBuffers(unsigned int count)
> > +int RPiStream::prepareBuffers(unsigned int count)
> >  {
> > +     int ret;
> > +
> > +     if (!importOnly_) {
> > +             if (count) {
> > +                     /* Export some frame buffers for internal use. */
> > +                     ret = dev_->exportBuffers(count, &internalBuffers_);
> > +                     if (ret < 0)
> > +                             return ret;
> > +
> > +                     /* Add these exported buffers to the internal/external buffer list. */
> > +                     setExportedBuffers(&internalBuffers_);
> > +
> > +                     /* Add these buffers to the queue of internal usable buffers. */
> > +                     for (auto const &buffer : internalBuffers_)
> > +                             availableBuffers_.push(buffer.get());
> > +             }
> > +
> > +             /* We must import all internal/external exported buffers. */
> > +             count = bufferList_.size();
> > +     }
> > +
> >       return dev_->importBuffers(count);
> >  }
> >
> > -int RPiStream::allocateBuffers(unsigned int count)
> > +int RPiStream::queueAllBuffers()
> >  {
> > -     return dev_->allocateBuffers(count, &internalBuffers_);
> > -}
> > +     int ret;
> >
> > -int RPiStream::queueBuffers()
> > -{
> >       if (external_)
> >               return 0;
> >
> > -     for (auto &b : internalBuffers_) {
> > -             int ret = dev_->queueBuffer(b.get());
> > -             if (ret) {
> > -                     LOG(RPISTREAM, Error) << "Failed to queue buffers for "
> > -                                           << name_;
> > +     while (!availableBuffers_.empty()) {
> > +             ret = queueBuffer(availableBuffers_.front());
> > +             if (ret < 0)
> >                       return ret;
> > -             }
> > +
> > +             availableBuffers_.pop();
> >       }
> >
> >       return 0;
> >  }
> >
> > -bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const
> > +int RPiStream::queueBuffer(FrameBuffer *buffer)
> > +{
> > +     /*
> > +      * A nullptr buffer implies an external stream, but no external
> > +      * buffer has been supplied. So, pick one from the availableBuffers_
> > +      * queue.
> > +      */
> > +     if (!buffer) {
> > +             if (availableBuffers_.empty()) {
> > +                     LOG(RPISTREAM, Warning) << "No buffers available for "
> > +                                             << name_;
> > +                     return -EINVAL;
> > +             }
> > +
> > +             buffer = availableBuffers_.front();
> > +             availableBuffers_.pop();
> > +     }
> > +
> > +     LOG(RPISTREAM, Debug) << "Queuing buffer " << buffer->cookie()
> > +                           << " for " << name_;
> > +
> > +     int ret = dev_->queueBuffer(buffer);
> > +     if (ret) {
> > +             LOG(RPISTREAM, Error) << "Failed to queue buffer for "
> > +                                   << name_;
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> > +void RPiStream::returnBuffer(FrameBuffer *buffer)
> >  {
> > -     auto start = external_ ? externalBuffers_->begin() : internalBuffers_.begin();
> > -     auto end = external_ ? externalBuffers_->end() : internalBuffers_.end();
> > +     /* This can only be called for external streams. */
> > +     assert(external_);
> > +
> > +     availableBuffers_.push(buffer);
> > +}
> >
> > +bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const
> > +{
> >       if (importOnly_)
> >               return false;
> >
> > -     if (std::find_if(start, end,
> > -                      [buffer](std::unique_ptr<FrameBuffer> const &ref) { return ref.get() == buffer; }) != end)
> > +     if (std::find(bufferList_.begin(), bufferList_.end(), buffer) != bufferList_.end())
> >               return true;
> >
> >       return false;
> >  }
> >
> > +void RPiStream::clearBuffers()
> > +{
> > +     availableBuffers_ = std::queue<FrameBuffer *>{};
> > +     internalBuffers_.clear();
> > +     bufferList_.clear();
> > +}
> > +
> >  } /* namespace RPi */
> >
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > index 3957e342..af9c2ad2 100644
> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > @@ -39,21 +39,22 @@ public:
> >       V4L2VideoDevice *dev() const;
> >       void setExternal(bool external);
> >       bool isExternal() const;
> > -     bool isImporter() const;
> >       void reset();
> >       std::string name() const;
> > -     void setExternalBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> > -     const std::vector<std::unique_ptr<FrameBuffer>> *getBuffers() const;
> > +     void setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> > +     const std::vector<FrameBuffer *> &getBuffers() const;
> >       void releaseBuffers();
> > -     int importBuffers(unsigned int count);
> > -     int allocateBuffers(unsigned int count);
> > -     int queueBuffers();
> > +     int prepareBuffers(unsigned int count);
> > +     int queueAllBuffers();
> > +     int queueBuffer(FrameBuffer *buffer);
> > +     void returnBuffer(FrameBuffer *buffer);
> >       bool findFrameBuffer(FrameBuffer *buffer) const;
>
>
> Perhaps we should break those up into sections in patch 1/9. It's hard
> to see the wood-for-the-trees in the wall of function declarations.

Not sure I follow what you mean?

>
>
> >
> >  private:
> > +     void clearBuffers();
> >       /*
> >        * Indicates that this stream is active externally, i.e. the buffers
> > -      * are provided by the application.
> > +      * might be provided by (and returned to) the application.
> >        */
> >       bool external_;
> >       /* Indicates that this stream only imports buffers, e.g. ISP input. */
> > @@ -62,10 +63,19 @@ private:
> >       std::string name_;
> >       /* The actual device stream. */
> >       std::unique_ptr<V4L2VideoDevice> dev_;
> > -     /* Internally allocated framebuffers associated with this device stream. */
> > +     /* All framebuffers associated with this device stream. */
> > +     std::vector<FrameBuffer *> bufferList_;
> > +     /*
> > +      * List of frame buffer that we can use if none have been provided by
> > +      * the application for external streams. This is populated by the
> > +      * buffers exported internally.
> > +      */
> > +     std::queue<FrameBuffer *> availableBuffers_;
> > +     /*
> > +      * This is a list of buffers exported internally. Need to keep this around
> > +      * as the stream needs to maintain ownership of these buffers.
> > +      */
> >       std::vector<std::unique_ptr<FrameBuffer>> internalBuffers_;
> > -     /* Externally allocated framebuffers associated with this device stream. */
> > -     std::vector<std::unique_ptr<FrameBuffer>> *externalBuffers_;
>
> Likewise here, some whitespace to break up these members would really
> help, as by the time I've got here I've got definite review fatigue and
> lots of lines close together just become a blur ... :S

That's not a problem.

>
>
> >  };
> >
> >  /*
> >
>
> --
> Regards
> --
> Kieran

Regards,
Naush

Hi Naush,

On 22/07/2020 09:04, Naushir Patuck wrote:
> Hi Kieran,
> 
> Thank you for the review.
> 
> On Tue, 21 Jul 2020 at 16:41, Kieran Bingham
> <kieran.bingham@ideasonboard.com> wrote:
>>
>> Hi Naush,
>>
>> On 20/07/2020 10:13, Naushir Patuck wrote:
>>> Stop using v4l2_videodevice::allocateBuffer() for internal buffers and
>>> instead export/import all buffers. This allows the pipeline to return
>>> any stream buffer requested by the application as zero-copy.
>>>
>>> Advertise the Unicam Image stream as the RAW capture stream now.
>>>
>>> The RPiStream object now maintains a new list of buffers that are
>>> available to queue into a device. This is needed to distinguish between
>>> FrameBuffers allocated for internal use vs externally provided buffers.
>>> When a Request comes in, if a buffer is not provided for an exported
>>> stream, we re-use a buffer from this list.
>>>
>>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
>>
>> A few small nit fixups below but they could possibly be fixed while
>> applying anyway if you prefer.
>>
>> I've hit review fatigue though, so I'll look at this again tomorrow.
> 
> I understand :)
> I will make the necessary fixups based on your review comments, and
> submit a new patch set.  Will wait until you have finished reviewing
> this one before I send the next revision.

Looking good, I think the only actionable thing remaining here is
potentially to use ASSERT() over assert() :-)


> 
>>
>>
>>> ---
>>>  .../pipeline/raspberrypi/raspberrypi.cpp      | 228 ++++++++++--------
>>>  .../pipeline/raspberrypi/rpi_stream.cpp       | 122 +++++++---
>>>  .../pipeline/raspberrypi/rpi_stream.h         |  30 ++-
>>>  3 files changed, 226 insertions(+), 154 deletions(-)
>>>
>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>>> index 8f6a999b..dbc22521 100644
>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>>> @@ -187,7 +187,8 @@ private:
>>>       void checkRequestCompleted();
>>>       void tryRunPipeline();
>>>       void tryFlushQueues();
>>> -     FrameBuffer *updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp, V4L2VideoDevice *dev);
>>> +     FrameBuffer *updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp,
>>> +                              RPi::RPiStream *stream);
>>>
>>>       unsigned int ispOutputCount_;
>>>  };
>>> @@ -508,8 +509,15 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>>>               StreamConfiguration &cfg = config->at(i);
>>>
>>>               if (isRaw(cfg.pixelFormat)) {
>>> -                     cfg.setStream(&data->isp_[Isp::Input]);
>>> -                     data->isp_[Isp::Input].setExternal(true);
>>> +                     cfg.setStream(&data->unicam_[Unicam::Image]);
>>> +                     /*
>>> +                      * We must set both Unicam streams as external, even
>>> +                      * though the application may only request RAW frames.
>>> +                      * This is because we match timestamps on both streams
>>> +                      * to synchronise buffers.
>>> +                      */
>>> +                     data->unicam_[Unicam::Image].setExternal(true);
>>> +                     data->unicam_[Unicam::Embedded].setExternal(true);
>>>                       continue;
>>>               }
>>>
>>> @@ -612,7 +620,7 @@ int PipelineHandlerRPi::exportFrameBuffers(Camera *camera, Stream *stream,
>>>       unsigned int count = stream->configuration().bufferCount;
>>>       int ret = s->dev()->exportBuffers(count, buffers);
>>>
>>> -     s->setExternalBuffers(buffers);
>>> +     s->setExportedBuffers(buffers);
>>>
>>>       return ret;
>>>  }
>>> @@ -712,14 +720,23 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)
>>>       if (data->state_ == RPiCameraData::State::Stopped)
>>>               return -EINVAL;
>>>
>>> -     /* Ensure all external streams have associated buffers! */
>>> -     for (auto &stream : data->isp_) {
>>> -             if (!stream.isExternal())
>>> -                     continue;
>>> +     LOG(RPI, Debug) << "queueRequestDevice: New request.";
>>>
>>> -             if (!request->findBuffer(&stream)) {
>>> -                     LOG(RPI, Error) << "Attempt to queue request with invalid stream.";
>>> -                     return -ENOENT;
>>> +     /* Push all buffers supplied in the Request to the respective streams. */
>>> +     for (auto stream : data->streams_) {
>>> +             if (stream->isExternal()) {
>>
>>
>> Are all streams now 'external' ?
> 
> All streams but the ISP input (which only imports buffers) *can* be
> external.  They are marked external only if the app configures the
> pipeline handler by saying it may request a buffer from the stream.
> In reality, all streams could be marked external all the time, but the
> buffer handling will take a slightly less efficient path, so I kept
> the distinction.
> 
>>
>>
>>> +                     FrameBuffer *buffer = request->findBuffer(stream);
>>> +                     /*
>>> +                      * A nullptr in buffer means that we should queue an internally
>>> +                      * allocated buffer.

Now I understand what's going on here, perhaps this could be expanded a
bit to explain?

"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 which will not be given back
to the application, but can be used to support the internal pipeline flow."


Now I see how this leads towards zero-copy raw support ;-)



>>> +                      *
>>> +                      * 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;
>>>               }
>>>       }
>>>
>>> @@ -808,12 +825,10 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
>>>       /* Initialize the camera properties. */
>>>       data->properties_ = data->sensor_->properties();
>>>
>>> -     /*
>>> -      * List the available output streams.
>>> -      * Currently cannot do Unicam streams!
>>> -      */
>>> +     /*List the available streams an application may request. */
>>
>> Minor nit, there's a space missing between /* and List ...
>>
>>
>>>       std::set<Stream *> streams;
>>> -     streams.insert(&data->isp_[Isp::Input]);
>>> +     streams.insert(&data->unicam_[Unicam::Image]);
>>> +     streams.insert(&data->unicam_[Unicam::Embedded]);
>>>       streams.insert(&data->isp_[Isp::Output0]);
>>>       streams.insert(&data->isp_[Isp::Output1]);
>>>       streams.insert(&data->isp_[Isp::Stats]);
>>> @@ -831,9 +846,28 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
>>>       int ret;
>>>
>>>       for (auto const stream : data->streams_) {
>>> -             ret = stream->queueBuffers();
>>> -             if (ret < 0)
>>> -                     return ret;
>>> +             if (!stream->isExternal()) {
>>> +                     ret = stream->queueAllBuffers();
>>> +                     if (ret < 0)
>>> +                             return ret;
>>> +             } else {
>>> +                     /*
>>> +                      * For external streams, we must queue up a set of internal
>>> +                      * buffers to handle the number of drop frames requested by
>>> +                      * the IPA. This is done by passing nullptr in queueBuffer().
>>> +                      *
>>> +                      * 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.
>>> +                      */
>>> +                     unsigned int i;
>>> +                     for (i = 0; i < data->dropFrameCount_; i++) {
>>> +                             int ret = stream->queueBuffer(nullptr);
>>> +                             if (ret)
>>> +                                     return ret;
>>> +                     }
>>> +             }
>>>       }
>>>
>>>       return 0;
>>> @@ -847,7 +881,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>>>       /*
>>>        * Decide how many internal buffers to allocate. For now, simply look
>>>        * at how many external buffers will be provided. Will need to improve
>>> -      * this logic.
>>> +      * this logic. However, we really must have all stream allocate the same
>>
>> s/stream/streams/
>>
>>> +      * number of buffers to simplify error handling in queueRequestDevice().
>>>        */
>>
>> Does this include Raw streams? I thought that allocates less buffers? Or
>> perhaps it's the same number of internal buffers, and only 2 extra
>> external buffers for that case...
> 
> That's correct.  For now all (including Raw) streams allocate the same
> number of internal buffers.  Hence the comment that we could improve
> this logic, it is a bit wasteful on memory.  This bit does need
> refinement at some point.
> 
>>
>>
>>>       unsigned int maxBuffers = 0;
>>>       for (const Stream *s : camera->streams())
>>> @@ -855,33 +890,9 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>>>                       maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);
>>>
>>>       for (auto const stream : data->streams_) {
>>> -             if (stream->isExternal() || stream->isImporter()) {
>>> -                     /*
>>> -                      * If a stream is marked as external reserve memory to
>>> -                      * prepare to import as many buffers are requested in
>>> -                      * the stream configuration.
>>> -                      *
>>> -                      * If a stream is an internal stream with importer
>>> -                      * role, reserve as many buffers as possible.
>>> -                      */
>>> -                     unsigned int count = stream->isExternal()
>>> -                                                  ? stream->configuration().bufferCount
>>> -                                                  : maxBuffers;
>>> -                     ret = stream->importBuffers(count);
>>> -                     if (ret < 0)
>>> -                             return ret;
>>> -             } else {
>>> -                     /*
>>> -                      * If the stream is an internal exporter allocate and
>>> -                      * export as many buffers as possible to its internal
>>> -                      * pool.
>>> -                      */
>>> -                     ret = stream->allocateBuffers(maxBuffers);
>>> -                     if (ret < 0) {
>>> -                             freeBuffers(camera);
>>> -                             return ret;
>>> -                     }
>>> -             }
>>> +             ret = stream->prepareBuffers(maxBuffers);
>>> +             if (ret < 0)
>>> +                     return ret;
>>>       }
>>>
>>>       /*
>>> @@ -889,7 +900,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>>>        * the IPA and RPI_IPA_EVENT_SIGNAL_ISP_PREPARE event.
>>>        */
>>>       count = 0;
>>> -     for (auto const &b : *data->unicam_[Unicam::Image].getBuffers()) {
>>> +     for (auto const &b : data->unicam_[Unicam::Image].getBuffers()) {
>>>               b->setCookie(count++);
>>>       }
>>>
>>> @@ -898,14 +909,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
>>>        * the IPA.
>>>        */
>>>       count = 0;
>>> -     for (auto const &b : *data->isp_[Isp::Stats].getBuffers()) {
>>> +     for (auto const &b : data->isp_[Isp::Stats].getBuffers()) {
>>>               b->setCookie(count++);
>>>               data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | b->cookie(),
>>>                                             .planes = b->planes() });
>>>       }
>>>
>>>       count = 0;
>>> -     for (auto const &b : *data->unicam_[Unicam::Embedded].getBuffers()) {
>>> +     for (auto const &b : data->unicam_[Unicam::Embedded].getBuffers()) {
>>>               b->setCookie(count++);
>>>               data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | b->cookie(),
>>>                                             .planes = b->planes() });
>>> @@ -1066,7 +1077,7 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
>>>       switch (action.operation) {
>>>       case RPI_IPA_ACTION_STATS_METADATA_COMPLETE: {
>>>               unsigned int bufferId = action.data[0];
>>> -             FrameBuffer *buffer = isp_[Isp::Stats].getBuffers()->at(bufferId).get();
>>> +             FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);
>>>
>>>               handleStreamBuffer(buffer, &isp_[Isp::Stats]);
>>>               /* Fill the Request metadata buffer with what the IPA has provided */
>>> @@ -1077,19 +1088,19 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
>>>
>>>       case RPI_IPA_ACTION_EMBEDDED_COMPLETE: {
>>>               unsigned int bufferId = action.data[0];
>>> -             FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers()->at(bufferId).get();
>>> +             FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);
>>>               handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
>>>               break;
>>>       }
>>>
>>>       case RPI_IPA_ACTION_RUN_ISP: {
>>>               unsigned int bufferId = action.data[0];
>>> -             FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers()->at(bufferId).get();
>>> +             FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);
>>>
>>>               LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << buffer->cookie()
>>>                               << ", timestamp: " << buffer->metadata().timestamp;
>>>
>>> -             isp_[Isp::Input].dev()->queueBuffer(buffer);
>>> +             isp_[Isp::Input].queueBuffer(buffer);
>>>               ispOutputCount_ = 0;
>>>               break;
>>>       }
>>> @@ -1190,22 +1201,27 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
>>>                       << ", buffer id " << buffer->cookie()
>>>                       << ", timestamp: " << buffer->metadata().timestamp;
>>>
>>> -     handleStreamBuffer(buffer, stream);
>>> -
>>>       /*
>>> -      * Increment the number of ISP outputs generated.
>>> -      * This is needed to track dropped frames.
>>> +      * ISP statistics buffer must not be re-queued or sent back to the
>>> +      * application until after the IPA signals so.
>>>        */
>>> -     ispOutputCount_++;
>>> -
>>> -     /* If this is a stats output, hand it to the IPA now. */
>>>       if (stream == &isp_[Isp::Stats]) {
>>>               IPAOperationData op;
>>>               op.operation = RPI_IPA_EVENT_SIGNAL_STAT_READY;
>>>               op.data = { RPiIpaMask::STATS | buffer->cookie() };
>>>               ipa_->processEvent(op);
>>> +     } else {
>>> +             /* Any other ISP output can be handed back to the application now. */
>>> +             handleStreamBuffer(buffer, stream);
>>>       }
>>>
>>> +     /*
>>> +      * Increment the number of ISP outputs generated.
>>> +      * This is needed to track dropped frames.
>>> +      */
>>> +     ispOutputCount_++;
>>> +
>>> +
>>>       handleState();
>>>  }
>>>
>>> @@ -1218,8 +1234,12 @@ void RPiCameraData::clearIncompleteRequests()
>>>        */
>>>       for (auto const request : requestQueue_) {
>>>               for (auto const stream : streams_) {
>>> -                     if (stream->isExternal())
>>> -                             stream->dev()->queueBuffer(request->findBuffer(stream));
>>> +                     if (!stream->isExternal())
>>> +                             continue;
>>> +
>>> +                     FrameBuffer *buffer = request->findBuffer(stream);
>>> +                     if (buffer)
>>> +                             stream->queueBuffer(buffer);
>>>               }
>>>       }
>>>
>>> @@ -1248,7 +1268,7 @@ void RPiCameraData::clearIncompleteRequests()
>>>                        * Has the buffer already been handed back to the
>>>                        * request? If not, do so now.
>>>                        */
>>> -                     if (buffer->request())
>>> +                     if (buffer && buffer->request())
>>>                               pipe_->completeBuffer(camera_, request, buffer);
>>>               }
>>>
>>> @@ -1260,30 +1280,24 @@ void RPiCameraData::clearIncompleteRequests()
>>>  void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stream)
>>>  {
>>>       if (stream->isExternal()) {
>>> -             if (!dropFrameCount_) {
>>> -                     Request *request = buffer->request();
>>> +             Request *request = requestQueue_.front();
>>> +
>>> +             if (!dropFrameCount_ && request->findBuffer(stream) == buffer) {
>>> +                     /*
>>> +                      * Tag the buffer as completed, returning it to the
>>> +                      * application.
>>> +                      */
>>>                       pipe_->completeBuffer(camera_, request, buffer);
>>> +             } else {
>>> +                     /*
>>> +                      * This buffer was not part of the Request, so we can
>>> +                      * recycle it.
>>> +                      */
>>> +                     stream->returnBuffer(buffer);
>>>               }
>>>       } else {
>>> -             /* Special handling for RAW buffer Requests.
>>> -              *
>>> -              * The ISP input stream is alway an import stream, but if the
>>> -              * current Request has been made for a buffer on the stream,
>>> -              * simply memcpy to the Request buffer and requeue back to the
>>> -              * device.
>>> -              */
>>> -             if (stream == &unicam_[Unicam::Image] && !dropFrameCount_) {
>>> -                     const Stream *rawStream = static_cast<const Stream *>(&isp_[Isp::Input]);
>>> -                     Request *request = requestQueue_.front();
>>> -                     FrameBuffer *raw = request->findBuffer(const_cast<Stream *>(rawStream));
>>> -                     if (raw) {
>>> -                             raw->copyFrom(buffer);
>>> -                             pipe_->completeBuffer(camera_, request, raw);
>>> -                     }
>>> -             }
>>> -
>>> -             /* Simply requeue the buffer. */
>>> -             stream->dev()->queueBuffer(buffer);
>>> +             /* Simply re-queue the buffer to the requested stream. */
>>> +             stream->queueBuffer(buffer);
>>>       }
>>>  }
>>>
>>> @@ -1367,7 +1381,7 @@ void RPiCameraData::tryRunPipeline()
>>>        * current bayer buffer will be removed and re-queued to the driver.
>>>        */
>>>       embeddedBuffer = updateQueue(embeddedQueue_, bayerBuffer->metadata().timestamp,
>>> -                                  unicam_[Unicam::Embedded].dev());
>>> +                                  &unicam_[Unicam::Embedded]);
>>>
>>>       if (!embeddedBuffer) {
>>>               LOG(RPI, Debug) << "Could not find matching embedded buffer";
>>> @@ -1386,7 +1400,7 @@ void RPiCameraData::tryRunPipeline()
>>>
>>>               embeddedBuffer = embeddedQueue_.front();
>>>               bayerBuffer = updateQueue(bayerQueue_, embeddedBuffer->metadata().timestamp,
>>> -                                       unicam_[Unicam::Image].dev());
>>> +                                       &unicam_[Unicam::Image]);
>>>
>>>               if (!bayerBuffer) {
>>>                       LOG(RPI, Debug) << "Could not find matching bayer buffer - ending.";
>>> @@ -1394,11 +1408,7 @@ void RPiCameraData::tryRunPipeline()
>>>               }
>>>       }
>>>
>>> -     /*
>>> -      * Take the first request from the queue and action the IPA.
>>> -      * Unicam buffers for the request have already been queued as they come
>>> -      * in.
>>> -      */
>>> +     /* Take the first request from the queue and action the IPA. */
>>>       Request *request = requestQueue_.front();
>>>
>>>       /*
>>> @@ -1410,12 +1420,6 @@ void RPiCameraData::tryRunPipeline()
>>>       op.controls = { request->controls() };
>>>       ipa_->processEvent(op);
>>>
>>> -     /* Queue up any ISP buffers passed into the request. */
>>> -     for (auto &stream : isp_) {
>>> -             if (stream.isExternal())
>>> -                     stream.dev()->queueBuffer(request->findBuffer(&stream));
>>> -     }
>>> -
>>>       /* Ready to use the buffers, pop them off the queue. */
>>>       bayerQueue_.pop();
>>>       embeddedQueue_.pop();
>>> @@ -1445,32 +1449,42 @@ void RPiCameraData::tryFlushQueues()
>>>        * and give a chance for the hardware to return to lock-step. We do have
>>>        * to drop all interim frames.
>>>        */
>>> -     if (unicam_[Unicam::Image].getBuffers()->size() == bayerQueue_.size() &&
>>> -         unicam_[Unicam::Embedded].getBuffers()->size() == embeddedQueue_.size()) {
>>> +     if (unicam_[Unicam::Image].getBuffers().size() == bayerQueue_.size() &&
>>> +         unicam_[Unicam::Embedded].getBuffers().size() == embeddedQueue_.size()) {
>>> +             /* This cannot happen when Unicam streams are external. */
>>> +             assert(!unicam_[Unicam::Image].isExternal());
>>> +
>>>               LOG(RPI, Warning) << "Flushing all buffer queues!";
>>>
>>>               while (!bayerQueue_.empty()) {
>>> -                     unicam_[Unicam::Image].dev()->queueBuffer(bayerQueue_.front());
>>> +                     unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());
>>>                       bayerQueue_.pop();
>>>               }
>>>
>>>               while (!embeddedQueue_.empty()) {
>>> -                     unicam_[Unicam::Embedded].dev()->queueBuffer(embeddedQueue_.front());
>>> +                     unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());
>>>                       embeddedQueue_.pop();
>>>               }
>>>       }
>>>  }
>>>
>>>  FrameBuffer *RPiCameraData::updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp,
>>> -                                     V4L2VideoDevice *dev)
>>> +                                     RPi::RPiStream *stream)
>>>  {
>>> +     /*
>>> +      * If the unicam streams are external (both have to the same), then we
>>
>> both have to 'be' the same?
>>
>>> +      * can only return out the top buffer in the queue, and assume they have
>>> +      * been synced by queuing at the same time. We cannot drop these frames,
>>> +      * as they may have been provided externally.
>>> +      */
>>>       while (!q.empty()) {
>>>               FrameBuffer *b = q.front();
>>> -             if (b->metadata().timestamp < timestamp) {
>>> +             if (!stream->isExternal() && b->metadata().timestamp < timestamp) {
>>>                       q.pop();
>>> -                     dev->queueBuffer(b);
>>> -                     LOG(RPI, Warning) << "Dropping input frame!";
>>> -             } else if (b->metadata().timestamp == timestamp) {
>>> +                     stream->queueBuffer(b);
>>> +                     LOG(RPI, Warning) << "Dropping unmatched input frame in stream "
>>> +                                       << stream->name();
>>> +             } else if (stream->isExternal() || b->metadata().timestamp == timestamp) {
>>>                       /* The calling function will pop the item from the queue. */
>>>                       return b;
>>>               } else {
>>> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
>>> index 2edb8b59..02f8d3e0 100644
>>> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
>>> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
>>> @@ -21,30 +21,20 @@ V4L2VideoDevice *RPiStream::dev() const
>>>
>>>  void RPiStream::setExternal(bool external)
>>>  {
>>> +     /* Import streams cannot be external. */
>>> +     assert(!external || !importOnly_);
>>>       external_ = external;
>>>  }
>>>
>>>  bool RPiStream::isExternal() const
>>>  {
>>> -     /*
>>> -      * Import streams cannot be external.
>>> -      *
>>> -      * RAW capture is a special case where we simply copy the RAW
>>> -      * buffer out of the request. All other buffer handling happens
>>> -      * as if the stream is internal.
>>> -      */
>>> -     return external_ && !importOnly_;
>>> -}
>>> -
>>> -bool RPiStream::isImporter() const
>>> -{
>>> -     return importOnly_;
>>> +     return external_;
>>>  }
>>>
>>>  void RPiStream::reset()
>>>  {
>>>       external_ = false;
>>> -     internalBuffers_.clear();
>>> +     clearBuffers();
>>>  }
>>>
>>>  std::string RPiStream::name() const
>>> @@ -52,65 +42,123 @@ std::string RPiStream::name() const
>>>       return name_;
>>>  }
>>>
>>> -void RPiStream::setExternalBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>>> +void RPiStream::setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>>>  {
>>> -     externalBuffers_ = buffers;
>>> +     std::transform(buffers->begin(), buffers->end(), std::back_inserter(bufferList_),
>>> +                    [](std::unique_ptr<FrameBuffer> &b) { return b.get(); });
>>>  }
>>>
>>> -const std::vector<std::unique_ptr<FrameBuffer>> *RPiStream::getBuffers() const
>>> +const std::vector<FrameBuffer *> &RPiStream::getBuffers() const
>>>  {
>>> -     return external_ ? externalBuffers_ : &internalBuffers_;
>>> +     return bufferList_;
>>>  }
>>>
>>>  void RPiStream::releaseBuffers()
>>>  {
>>>       dev_->releaseBuffers();
>>> -     if (!external_ && !importOnly_)
>>> -             internalBuffers_.clear();
>>> +     clearBuffers();
>>>  }
>>>
>>> -int RPiStream::importBuffers(unsigned int count)
>>> +int RPiStream::prepareBuffers(unsigned int count)
>>>  {
>>> +     int ret;
>>> +
>>> +     if (!importOnly_) {
>>> +             if (count) {
>>> +                     /* Export some frame buffers for internal use. */
>>> +                     ret = dev_->exportBuffers(count, &internalBuffers_);
>>> +                     if (ret < 0)
>>> +                             return ret;
>>> +
>>> +                     /* Add these exported buffers to the internal/external buffer list. */
>>> +                     setExportedBuffers(&internalBuffers_);
>>> +
>>> +                     /* Add these buffers to the queue of internal usable buffers. */
>>> +                     for (auto const &buffer : internalBuffers_)
>>> +                             availableBuffers_.push(buffer.get());
>>> +             }
>>> +
>>> +             /* We must import all internal/external exported buffers. */
>>> +             count = bufferList_.size();
>>> +     }
>>> +
>>>       return dev_->importBuffers(count);
>>>  }
>>>
>>> -int RPiStream::allocateBuffers(unsigned int count)
>>> +int RPiStream::queueAllBuffers()
>>>  {
>>> -     return dev_->allocateBuffers(count, &internalBuffers_);
>>> -}
>>> +     int ret;
>>>
>>> -int RPiStream::queueBuffers()
>>> -{
>>>       if (external_)
>>>               return 0;
>>>
>>> -     for (auto &b : internalBuffers_) {
>>> -             int ret = dev_->queueBuffer(b.get());
>>> -             if (ret) {
>>> -                     LOG(RPISTREAM, Error) << "Failed to queue buffers for "
>>> -                                           << name_;
>>> +     while (!availableBuffers_.empty()) {
>>> +             ret = queueBuffer(availableBuffers_.front());
>>> +             if (ret < 0)
>>>                       return ret;
>>> -             }
>>> +
>>> +             availableBuffers_.pop();
>>>       }
>>>
>>>       return 0;
>>>  }
>>>
>>> -bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const
>>> +int RPiStream::queueBuffer(FrameBuffer *buffer)
>>> +{
>>> +     /*
>>> +      * A nullptr buffer implies an external stream, but no external
>>> +      * buffer has been supplied. So, pick one from the availableBuffers_
>>> +      * queue.

Aha, now I see this here, maybe the comment addition I suggested above
is possibly redundant, as it's explained here ...



>>> +      */
>>> +     if (!buffer) {
>>> +             if (availableBuffers_.empty()) {
>>> +                     LOG(RPISTREAM, Warning) << "No buffers available for "
>>> +                                             << name_;
>>> +                     return -EINVAL;
>>> +             }
>>> +
>>> +             buffer = availableBuffers_.front();
>>> +             availableBuffers_.pop();
>>> +     }
>>> +
>>> +     LOG(RPISTREAM, Debug) << "Queuing buffer " << buffer->cookie()
>>> +                           << " for " << name_;
>>> +
>>> +     int ret = dev_->queueBuffer(buffer);
>>> +     if (ret) {
>>> +             LOG(RPISTREAM, Error) << "Failed to queue buffer for "
>>> +                                   << name_;
>>> +     }
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +void RPiStream::returnBuffer(FrameBuffer *buffer)
>>>  {
>>> -     auto start = external_ ? externalBuffers_->begin() : internalBuffers_.begin();
>>> -     auto end = external_ ? externalBuffers_->end() : internalBuffers_.end();
>>> +     /* This can only be called for external streams. */
>>> +     assert(external_);

Could this use ASSERT() from libcamera/internal/log.h ?

>>> +
>>> +     availableBuffers_.push(buffer);
>>> +}
>>>
>>> +bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const
>>> +{
>>>       if (importOnly_)
>>>               return false;
>>>
>>> -     if (std::find_if(start, end,
>>> -                      [buffer](std::unique_ptr<FrameBuffer> const &ref) { return ref.get() == buffer; }) != end)
>>> +     if (std::find(bufferList_.begin(), bufferList_.end(), buffer) != bufferList_.end())
>>>               return true;
>>>
>>>       return false;
>>>  }
>>>
>>> +void RPiStream::clearBuffers()
>>> +{
>>> +     availableBuffers_ = std::queue<FrameBuffer *>{};
>>> +     internalBuffers_.clear();
>>> +     bufferList_.clear();
>>> +}
>>> +
>>>  } /* namespace RPi */
>>>
>>>  } /* namespace libcamera */
>>> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
>>> index 3957e342..af9c2ad2 100644
>>> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
>>> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
>>> @@ -39,21 +39,22 @@ public:
>>>       V4L2VideoDevice *dev() const;
>>>       void setExternal(bool external);
>>>       bool isExternal() const;
>>> -     bool isImporter() const;
>>>       void reset();
>>>       std::string name() const;
>>> -     void setExternalBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers);
>>> -     const std::vector<std::unique_ptr<FrameBuffer>> *getBuffers() const;
>>> +     void setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers);
>>> +     const std::vector<FrameBuffer *> &getBuffers() const;
>>>       void releaseBuffers();
>>> -     int importBuffers(unsigned int count);
>>> -     int allocateBuffers(unsigned int count);
>>> -     int queueBuffers();
>>> +     int prepareBuffers(unsigned int count);
>>> +     int queueAllBuffers();
>>> +     int queueBuffer(FrameBuffer *buffer);
>>> +     void returnBuffer(FrameBuffer *buffer);
>>>       bool findFrameBuffer(FrameBuffer *buffer) const;
>>
>>
>> Perhaps we should break those up into sections in patch 1/9. It's hard
>> to see the wood-for-the-trees in the wall of function declarations.
> 
> Not sure I follow what you mean?


There is a lot of text all bunched up together, so it's hard to get my
eyes around the class members.

It would be nice to group functions with some line breaks to make it
easier on the eye.

But that should happen in patch 1/9, so I'll go comment on it there. I
skipped it before because that patch just moves code around, but I thnik
it can also improve formatting for readability.



> 
>>
>>
>>>
>>>  private:
>>> +     void clearBuffers();
>>>       /*
>>>        * Indicates that this stream is active externally, i.e. the buffers
>>> -      * are provided by the application.
>>> +      * might be provided by (and returned to) the application.
>>>        */
>>>       bool external_;
>>>       /* Indicates that this stream only imports buffers, e.g. ISP input. */
>>> @@ -62,10 +63,19 @@ private:
>>>       std::string name_;
>>>       /* The actual device stream. */
>>>       std::unique_ptr<V4L2VideoDevice> dev_;
>>> -     /* Internally allocated framebuffers associated with this device stream. */
>>> +     /* All framebuffers associated with this device stream. */
>>> +     std::vector<FrameBuffer *> bufferList_;
>>> +     /*
>>> +      * List of frame buffer that we can use if none have been provided by
>>> +      * the application for external streams. This is populated by the
>>> +      * buffers exported internally.
>>> +      */
>>> +     std::queue<FrameBuffer *> availableBuffers_;
>>> +     /*
>>> +      * This is a list of buffers exported internally. Need to keep this around
>>> +      * as the stream needs to maintain ownership of these buffers.
>>> +      */
>>>       std::vector<std::unique_ptr<FrameBuffer>> internalBuffers_;
>>> -     /* Externally allocated framebuffers associated with this device stream. */
>>> -     std::vector<std::unique_ptr<FrameBuffer>> *externalBuffers_;
>>
>> Likewise here, some whitespace to break up these members would really
>> help, as by the time I've got here I've got definite review fatigue and
>> lots of lines close together just become a blur ... :S
> 
> That's not a problem.


But for this patch, I can't see anything wrong ... and I can see how
it's helping manage buffers to allow optional buffers on a stream for
RAW zero copy support.

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


> 
>>
>>
>>>  };
>>>
>>>  /*
>>>
>>
>> --
>> Regards
>> --
>> Kieran
> 
> Regards,
> Naush
>

Hi Naush,

Thank you for the patch.

On Wed, Jul 22, 2020 at 01:21:33PM +0100, Kieran Bingham wrote:
> On 22/07/2020 09:04, Naushir Patuck wrote:
> > On Tue, 21 Jul 2020 at 16:41, Kieran Bingham wrote:
> >> On 20/07/2020 10:13, Naushir Patuck wrote:
> >>> Stop using v4l2_videodevice::allocateBuffer() for internal buffers and
> >>> instead export/import all buffers. This allows the pipeline to return
> >>> any stream buffer requested by the application as zero-copy.
> >>>
> >>> Advertise the Unicam Image stream as the RAW capture stream now.
> >>>
> >>> The RPiStream object now maintains a new list of buffers that are
> >>> available to queue into a device. This is needed to distinguish between
> >>> FrameBuffers allocated for internal use vs externally provided buffers.
> >>> When a Request comes in, if a buffer is not provided for an exported
> >>> stream, we re-use a buffer from this list.
> >>>
> >>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> >>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> >>
> >> A few small nit fixups below but they could possibly be fixed while
> >> applying anyway if you prefer.
> >>
> >> I've hit review fatigue though, so I'll look at this again tomorrow.
> > 
> > I understand :)
> > I will make the necessary fixups based on your review comments, and
> > submit a new patch set.  Will wait until you have finished reviewing
> > this one before I send the next revision.
> 
> Looking good, I think the only actionable thing remaining here is
> potentially to use ASSERT() over assert() :-)
> 
> >>> ---
> >>>  .../pipeline/raspberrypi/raspberrypi.cpp      | 228 ++++++++++--------
> >>>  .../pipeline/raspberrypi/rpi_stream.cpp       | 122 +++++++---
> >>>  .../pipeline/raspberrypi/rpi_stream.h         |  30 ++-
> >>>  3 files changed, 226 insertions(+), 154 deletions(-)
> >>>
> >>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>> index 8f6a999b..dbc22521 100644
> >>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>> @@ -187,7 +187,8 @@ private:
> >>>       void checkRequestCompleted();
> >>>       void tryRunPipeline();
> >>>       void tryFlushQueues();
> >>> -     FrameBuffer *updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp, V4L2VideoDevice *dev);
> >>> +     FrameBuffer *updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp,
> >>> +                              RPi::RPiStream *stream);
> >>>
> >>>       unsigned int ispOutputCount_;
> >>>  };
> >>> @@ -508,8 +509,15 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> >>>               StreamConfiguration &cfg = config->at(i);
> >>>
> >>>               if (isRaw(cfg.pixelFormat)) {
> >>> -                     cfg.setStream(&data->isp_[Isp::Input]);
> >>> -                     data->isp_[Isp::Input].setExternal(true);
> >>> +                     cfg.setStream(&data->unicam_[Unicam::Image]);
> >>> +                     /*
> >>> +                      * We must set both Unicam streams as external, even
> >>> +                      * though the application may only request RAW frames.
> >>> +                      * This is because we match timestamps on both streams
> >>> +                      * to synchronise buffers.
> >>> +                      */
> >>> +                     data->unicam_[Unicam::Image].setExternal(true);
> >>> +                     data->unicam_[Unicam::Embedded].setExternal(true);
> >>>                       continue;
> >>>               }
> >>>
> >>> @@ -612,7 +620,7 @@ int PipelineHandlerRPi::exportFrameBuffers(Camera *camera, Stream *stream,
> >>>       unsigned int count = stream->configuration().bufferCount;
> >>>       int ret = s->dev()->exportBuffers(count, buffers);
> >>>
> >>> -     s->setExternalBuffers(buffers);
> >>> +     s->setExportedBuffers(buffers);
> >>>
> >>>       return ret;
> >>>  }
> >>> @@ -712,14 +720,23 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)
> >>>       if (data->state_ == RPiCameraData::State::Stopped)
> >>>               return -EINVAL;
> >>>
> >>> -     /* Ensure all external streams have associated buffers! */
> >>> -     for (auto &stream : data->isp_) {
> >>> -             if (!stream.isExternal())
> >>> -                     continue;
> >>> +     LOG(RPI, Debug) << "queueRequestDevice: New request.";
> >>>
> >>> -             if (!request->findBuffer(&stream)) {
> >>> -                     LOG(RPI, Error) << "Attempt to queue request with invalid stream.";
> >>> -                     return -ENOENT;
> >>> +     /* Push all buffers supplied in the Request to the respective streams. */
> >>> +     for (auto stream : data->streams_) {
> >>> +             if (stream->isExternal()) {
> >>
> >> Are all streams now 'external' ?
> > 
> > All streams but the ISP input (which only imports buffers) *can* be
> > external.  They are marked external only if the app configures the
> > pipeline handler by saying it may request a buffer from the stream.
> > In reality, all streams could be marked external all the time, but the
> > buffer handling will take a slightly less efficient path, so I kept
> > the distinction.
> > 
> >>> +                     FrameBuffer *buffer = request->findBuffer(stream);
> >>> +                     /*
> >>> +                      * A nullptr in buffer means that we should queue an internally
> >>> +                      * allocated buffer.
> 
> Now I understand what's going on here, perhaps this could be expanded a
> bit to explain?
> 
> "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 which will not be given back
> to the application, but can be used to support the internal pipeline flow."
>
> Now I see how this leads towards zero-copy raw support ;-)
> 
> >>> +                      *
> >>> +                      * 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;
> >>>               }
> >>>       }
> >>>
> >>> @@ -808,12 +825,10 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
> >>>       /* Initialize the camera properties. */
> >>>       data->properties_ = data->sensor_->properties();
> >>>
> >>> -     /*
> >>> -      * List the available output streams.
> >>> -      * Currently cannot do Unicam streams!
> >>> -      */
> >>> +     /*List the available streams an application may request. */
> >>
> >> Minor nit, there's a space missing between /* and List ...
> >>
> >>>       std::set<Stream *> streams;
> >>> -     streams.insert(&data->isp_[Isp::Input]);
> >>> +     streams.insert(&data->unicam_[Unicam::Image]);
> >>> +     streams.insert(&data->unicam_[Unicam::Embedded]);

Can an application request the embedded data ?

> >>>       streams.insert(&data->isp_[Isp::Output0]);
> >>>       streams.insert(&data->isp_[Isp::Output1]);
> >>>       streams.insert(&data->isp_[Isp::Stats]);
> >>> @@ -831,9 +846,28 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
> >>>       int ret;
> >>>
> >>>       for (auto const stream : data->streams_) {
> >>> -             ret = stream->queueBuffers();
> >>> -             if (ret < 0)
> >>> -                     return ret;
> >>> +             if (!stream->isExternal()) {
> >>> +                     ret = stream->queueAllBuffers();
> >>> +                     if (ret < 0)
> >>> +                             return ret;
> >>> +             } else {
> >>> +                     /*
> >>> +                      * For external streams, we must queue up a set of internal
> >>> +                      * buffers to handle the number of drop frames requested by
> >>> +                      * the IPA. This is done by passing nullptr in queueBuffer().
> >>> +                      *
> >>> +                      * 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.
> >>> +                      */
> >>> +                     unsigned int i;
> >>> +                     for (i = 0; i < data->dropFrameCount_; i++) {
> >>> +                             int ret = stream->queueBuffer(nullptr);
> >>> +                             if (ret)
> >>> +                                     return ret;
> >>> +                     }
> >>> +             }
> >>>       }
> >>>
> >>>       return 0;
> >>> @@ -847,7 +881,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> >>>       /*
> >>>        * Decide how many internal buffers to allocate. For now, simply look
> >>>        * at how many external buffers will be provided. Will need to improve
> >>> -      * this logic.
> >>> +      * this logic. However, we really must have all stream allocate the same
> >>
> >> s/stream/streams/
> >>
> >>> +      * number of buffers to simplify error handling in queueRequestDevice().
> >>>        */
> >>
> >> Does this include Raw streams? I thought that allocates less buffers? Or
> >> perhaps it's the same number of internal buffers, and only 2 extra
> >> external buffers for that case...
> > 
> > That's correct.  For now all (including Raw) streams allocate the same
> > number of internal buffers.  Hence the comment that we could improve
> > this logic, it is a bit wasteful on memory.  This bit does need
> > refinement at some point.
> > 
> >>>       unsigned int maxBuffers = 0;
> >>>       for (const Stream *s : camera->streams())
> >>> @@ -855,33 +890,9 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> >>>                       maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);
> >>>
> >>>       for (auto const stream : data->streams_) {
> >>> -             if (stream->isExternal() || stream->isImporter()) {
> >>> -                     /*
> >>> -                      * If a stream is marked as external reserve memory to
> >>> -                      * prepare to import as many buffers are requested in
> >>> -                      * the stream configuration.
> >>> -                      *
> >>> -                      * If a stream is an internal stream with importer
> >>> -                      * role, reserve as many buffers as possible.
> >>> -                      */
> >>> -                     unsigned int count = stream->isExternal()
> >>> -                                                  ? stream->configuration().bufferCount
> >>> -                                                  : maxBuffers;
> >>> -                     ret = stream->importBuffers(count);
> >>> -                     if (ret < 0)
> >>> -                             return ret;
> >>> -             } else {
> >>> -                     /*
> >>> -                      * If the stream is an internal exporter allocate and
> >>> -                      * export as many buffers as possible to its internal
> >>> -                      * pool.
> >>> -                      */
> >>> -                     ret = stream->allocateBuffers(maxBuffers);
> >>> -                     if (ret < 0) {
> >>> -                             freeBuffers(camera);
> >>> -                             return ret;
> >>> -                     }
> >>> -             }
> >>> +             ret = stream->prepareBuffers(maxBuffers);
> >>> +             if (ret < 0)
> >>> +                     return ret;

This doesn't free buffers anymore on failure, is that an oversight ?

> >>>       }
> >>>
> >>>       /*
> >>> @@ -889,7 +900,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> >>>        * the IPA and RPI_IPA_EVENT_SIGNAL_ISP_PREPARE event.
> >>>        */
> >>>       count = 0;
> >>> -     for (auto const &b : *data->unicam_[Unicam::Image].getBuffers()) {
> >>> +     for (auto const &b : data->unicam_[Unicam::Image].getBuffers()) {
> >>>               b->setCookie(count++);
> >>>       }
> >>>
> >>> @@ -898,14 +909,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> >>>        * the IPA.
> >>>        */
> >>>       count = 0;
> >>> -     for (auto const &b : *data->isp_[Isp::Stats].getBuffers()) {
> >>> +     for (auto const &b : data->isp_[Isp::Stats].getBuffers()) {
> >>>               b->setCookie(count++);
> >>>               data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | b->cookie(),
> >>>                                             .planes = b->planes() });
> >>>       }
> >>>
> >>>       count = 0;
> >>> -     for (auto const &b : *data->unicam_[Unicam::Embedded].getBuffers()) {
> >>> +     for (auto const &b : data->unicam_[Unicam::Embedded].getBuffers()) {
> >>>               b->setCookie(count++);
> >>>               data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | b->cookie(),
> >>>                                             .planes = b->planes() });
> >>> @@ -1066,7 +1077,7 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
> >>>       switch (action.operation) {
> >>>       case RPI_IPA_ACTION_STATS_METADATA_COMPLETE: {
> >>>               unsigned int bufferId = action.data[0];
> >>> -             FrameBuffer *buffer = isp_[Isp::Stats].getBuffers()->at(bufferId).get();
> >>> +             FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);
> >>>
> >>>               handleStreamBuffer(buffer, &isp_[Isp::Stats]);
> >>>               /* Fill the Request metadata buffer with what the IPA has provided */
> >>> @@ -1077,19 +1088,19 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
> >>>
> >>>       case RPI_IPA_ACTION_EMBEDDED_COMPLETE: {
> >>>               unsigned int bufferId = action.data[0];
> >>> -             FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers()->at(bufferId).get();
> >>> +             FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);
> >>>               handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
> >>>               break;
> >>>       }
> >>>
> >>>       case RPI_IPA_ACTION_RUN_ISP: {
> >>>               unsigned int bufferId = action.data[0];
> >>> -             FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers()->at(bufferId).get();
> >>> +             FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);
> >>>
> >>>               LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << buffer->cookie()
> >>>                               << ", timestamp: " << buffer->metadata().timestamp;
> >>>
> >>> -             isp_[Isp::Input].dev()->queueBuffer(buffer);
> >>> +             isp_[Isp::Input].queueBuffer(buffer);
> >>>               ispOutputCount_ = 0;
> >>>               break;
> >>>       }
> >>> @@ -1190,22 +1201,27 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
> >>>                       << ", buffer id " << buffer->cookie()
> >>>                       << ", timestamp: " << buffer->metadata().timestamp;
> >>>
> >>> -     handleStreamBuffer(buffer, stream);
> >>> -
> >>>       /*
> >>> -      * Increment the number of ISP outputs generated.
> >>> -      * This is needed to track dropped frames.
> >>> +      * ISP statistics buffer must not be re-queued or sent back to the
> >>> +      * application until after the IPA signals so.
> >>>        */
> >>> -     ispOutputCount_++;
> >>> -
> >>> -     /* If this is a stats output, hand it to the IPA now. */
> >>>       if (stream == &isp_[Isp::Stats]) {
> >>>               IPAOperationData op;
> >>>               op.operation = RPI_IPA_EVENT_SIGNAL_STAT_READY;
> >>>               op.data = { RPiIpaMask::STATS | buffer->cookie() };
> >>>               ipa_->processEvent(op);
> >>> +     } else {
> >>> +             /* Any other ISP output can be handed back to the application now. */
> >>> +             handleStreamBuffer(buffer, stream);
> >>>       }
> >>>
> >>> +     /*
> >>> +      * Increment the number of ISP outputs generated.
> >>> +      * This is needed to track dropped frames.
> >>> +      */
> >>> +     ispOutputCount_++;
> >>> +
> >>> +

Extra blank line ?

> >>>       handleState();
> >>>  }
> >>>
> >>> @@ -1218,8 +1234,12 @@ void RPiCameraData::clearIncompleteRequests()
> >>>        */
> >>>       for (auto const request : requestQueue_) {
> >>>               for (auto const stream : streams_) {
> >>> -                     if (stream->isExternal())
> >>> -                             stream->dev()->queueBuffer(request->findBuffer(stream));
> >>> +                     if (!stream->isExternal())
> >>> +                             continue;
> >>> +
> >>> +                     FrameBuffer *buffer = request->findBuffer(stream);
> >>> +                     if (buffer)
> >>> +                             stream->queueBuffer(buffer);
> >>>               }
> >>>       }
> >>>
> >>> @@ -1248,7 +1268,7 @@ void RPiCameraData::clearIncompleteRequests()
> >>>                        * Has the buffer already been handed back to the
> >>>                        * request? If not, do so now.
> >>>                        */
> >>> -                     if (buffer->request())
> >>> +                     if (buffer && buffer->request())
> >>>                               pipe_->completeBuffer(camera_, request, buffer);
> >>>               }
> >>>
> >>> @@ -1260,30 +1280,24 @@ void RPiCameraData::clearIncompleteRequests()
> >>>  void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stream)
> >>>  {
> >>>       if (stream->isExternal()) {
> >>> -             if (!dropFrameCount_) {
> >>> -                     Request *request = buffer->request();
> >>> +             Request *request = requestQueue_.front();
> >>> +
> >>> +             if (!dropFrameCount_ && request->findBuffer(stream) == buffer) {
> >>> +                     /*
> >>> +                      * Tag the buffer as completed, returning it to the
> >>> +                      * application.
> >>> +                      */
> >>>                       pipe_->completeBuffer(camera_, request, buffer);
> >>> +             } else {
> >>> +                     /*
> >>> +                      * This buffer was not part of the Request, so we can
> >>> +                      * recycle it.
> >>> +                      */
> >>> +                     stream->returnBuffer(buffer);
> >>>               }
> >>>       } else {
> >>> -             /* Special handling for RAW buffer Requests.
> >>> -              *
> >>> -              * The ISP input stream is alway an import stream, but if the
> >>> -              * current Request has been made for a buffer on the stream,
> >>> -              * simply memcpy to the Request buffer and requeue back to the
> >>> -              * device.
> >>> -              */
> >>> -             if (stream == &unicam_[Unicam::Image] && !dropFrameCount_) {
> >>> -                     const Stream *rawStream = static_cast<const Stream *>(&isp_[Isp::Input]);
> >>> -                     Request *request = requestQueue_.front();
> >>> -                     FrameBuffer *raw = request->findBuffer(const_cast<Stream *>(rawStream));
> >>> -                     if (raw) {
> >>> -                             raw->copyFrom(buffer);
> >>> -                             pipe_->completeBuffer(camera_, request, raw);
> >>> -                     }
> >>> -             }
> >>> -
> >>> -             /* Simply requeue the buffer. */
> >>> -             stream->dev()->queueBuffer(buffer);
> >>> +             /* Simply re-queue the buffer to the requested stream. */
> >>> +             stream->queueBuffer(buffer);
> >>>       }
> >>>  }
> >>>
> >>> @@ -1367,7 +1381,7 @@ void RPiCameraData::tryRunPipeline()
> >>>        * current bayer buffer will be removed and re-queued to the driver.
> >>>        */
> >>>       embeddedBuffer = updateQueue(embeddedQueue_, bayerBuffer->metadata().timestamp,
> >>> -                                  unicam_[Unicam::Embedded].dev());
> >>> +                                  &unicam_[Unicam::Embedded]);
> >>>
> >>>       if (!embeddedBuffer) {
> >>>               LOG(RPI, Debug) << "Could not find matching embedded buffer";
> >>> @@ -1386,7 +1400,7 @@ void RPiCameraData::tryRunPipeline()
> >>>
> >>>               embeddedBuffer = embeddedQueue_.front();
> >>>               bayerBuffer = updateQueue(bayerQueue_, embeddedBuffer->metadata().timestamp,
> >>> -                                       unicam_[Unicam::Image].dev());
> >>> +                                       &unicam_[Unicam::Image]);
> >>>
> >>>               if (!bayerBuffer) {
> >>>                       LOG(RPI, Debug) << "Could not find matching bayer buffer - ending.";
> >>> @@ -1394,11 +1408,7 @@ void RPiCameraData::tryRunPipeline()
> >>>               }
> >>>       }
> >>>
> >>> -     /*
> >>> -      * Take the first request from the queue and action the IPA.
> >>> -      * Unicam buffers for the request have already been queued as they come
> >>> -      * in.
> >>> -      */
> >>> +     /* Take the first request from the queue and action the IPA. */
> >>>       Request *request = requestQueue_.front();
> >>>
> >>>       /*
> >>> @@ -1410,12 +1420,6 @@ void RPiCameraData::tryRunPipeline()
> >>>       op.controls = { request->controls() };
> >>>       ipa_->processEvent(op);
> >>>
> >>> -     /* Queue up any ISP buffers passed into the request. */
> >>> -     for (auto &stream : isp_) {
> >>> -             if (stream.isExternal())
> >>> -                     stream.dev()->queueBuffer(request->findBuffer(&stream));
> >>> -     }
> >>> -
> >>>       /* Ready to use the buffers, pop them off the queue. */
> >>>       bayerQueue_.pop();
> >>>       embeddedQueue_.pop();
> >>> @@ -1445,32 +1449,42 @@ void RPiCameraData::tryFlushQueues()
> >>>        * and give a chance for the hardware to return to lock-step. We do have
> >>>        * to drop all interim frames.
> >>>        */
> >>> -     if (unicam_[Unicam::Image].getBuffers()->size() == bayerQueue_.size() &&
> >>> -         unicam_[Unicam::Embedded].getBuffers()->size() == embeddedQueue_.size()) {
> >>> +     if (unicam_[Unicam::Image].getBuffers().size() == bayerQueue_.size() &&
> >>> +         unicam_[Unicam::Embedded].getBuffers().size() == embeddedQueue_.size()) {
> >>> +             /* This cannot happen when Unicam streams are external. */
> >>> +             assert(!unicam_[Unicam::Image].isExternal());
> >>> +
> >>>               LOG(RPI, Warning) << "Flushing all buffer queues!";
> >>>
> >>>               while (!bayerQueue_.empty()) {
> >>> -                     unicam_[Unicam::Image].dev()->queueBuffer(bayerQueue_.front());
> >>> +                     unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());
> >>>                       bayerQueue_.pop();
> >>>               }
> >>>
> >>>               while (!embeddedQueue_.empty()) {
> >>> -                     unicam_[Unicam::Embedded].dev()->queueBuffer(embeddedQueue_.front());
> >>> +                     unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());
> >>>                       embeddedQueue_.pop();
> >>>               }
> >>>       }
> >>>  }
> >>>
> >>>  FrameBuffer *RPiCameraData::updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp,
> >>> -                                     V4L2VideoDevice *dev)
> >>> +                                     RPi::RPiStream *stream)
> >>>  {
> >>> +     /*
> >>> +      * If the unicam streams are external (both have to the same), then we
> >>
> >> both have to 'be' the same?
> >>
> >>> +      * can only return out the top buffer in the queue, and assume they have
> >>> +      * been synced by queuing at the same time. We cannot drop these frames,
> >>> +      * as they may have been provided externally.
> >>> +      */
> >>>       while (!q.empty()) {
> >>>               FrameBuffer *b = q.front();
> >>> -             if (b->metadata().timestamp < timestamp) {
> >>> +             if (!stream->isExternal() && b->metadata().timestamp < timestamp) {
> >>>                       q.pop();
> >>> -                     dev->queueBuffer(b);
> >>> -                     LOG(RPI, Warning) << "Dropping input frame!";
> >>> -             } else if (b->metadata().timestamp == timestamp) {
> >>> +                     stream->queueBuffer(b);
> >>> +                     LOG(RPI, Warning) << "Dropping unmatched input frame in stream "
> >>> +                                       << stream->name();
> >>> +             } else if (stream->isExternal() || b->metadata().timestamp == timestamp) {
> >>>                       /* The calling function will pop the item from the queue. */
> >>>                       return b;
> >>>               } else {
> >>> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> >>> index 2edb8b59..02f8d3e0 100644
> >>> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> >>> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> >>> @@ -21,30 +21,20 @@ V4L2VideoDevice *RPiStream::dev() const
> >>>
> >>>  void RPiStream::setExternal(bool external)
> >>>  {
> >>> +     /* Import streams cannot be external. */
> >>> +     assert(!external || !importOnly_);
> >>>       external_ = external;
> >>>  }
> >>>
> >>>  bool RPiStream::isExternal() const
> >>>  {
> >>> -     /*
> >>> -      * Import streams cannot be external.
> >>> -      *
> >>> -      * RAW capture is a special case where we simply copy the RAW
> >>> -      * buffer out of the request. All other buffer handling happens
> >>> -      * as if the stream is internal.
> >>> -      */
> >>> -     return external_ && !importOnly_;
> >>> -}
> >>> -
> >>> -bool RPiStream::isImporter() const
> >>> -{
> >>> -     return importOnly_;
> >>> +     return external_;
> >>>  }
> >>>
> >>>  void RPiStream::reset()
> >>>  {
> >>>       external_ = false;
> >>> -     internalBuffers_.clear();
> >>> +     clearBuffers();
> >>>  }
> >>>
> >>>  std::string RPiStream::name() const
> >>> @@ -52,65 +42,123 @@ std::string RPiStream::name() const
> >>>       return name_;
> >>>  }
> >>>
> >>> -void RPiStream::setExternalBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> >>> +void RPiStream::setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> >>>  {
> >>> -     externalBuffers_ = buffers;
> >>> +     std::transform(buffers->begin(), buffers->end(), std::back_inserter(bufferList_),
> >>> +                    [](std::unique_ptr<FrameBuffer> &b) { return b.get(); });
> >>>  }
> >>>
> >>> -const std::vector<std::unique_ptr<FrameBuffer>> *RPiStream::getBuffers() const
> >>> +const std::vector<FrameBuffer *> &RPiStream::getBuffers() const
> >>>  {
> >>> -     return external_ ? externalBuffers_ : &internalBuffers_;
> >>> +     return bufferList_;
> >>>  }
> >>>
> >>>  void RPiStream::releaseBuffers()
> >>>  {
> >>>       dev_->releaseBuffers();
> >>> -     if (!external_ && !importOnly_)
> >>> -             internalBuffers_.clear();
> >>> +     clearBuffers();
> >>>  }
> >>>
> >>> -int RPiStream::importBuffers(unsigned int count)
> >>> +int RPiStream::prepareBuffers(unsigned int count)
> >>>  {
> >>> +     int ret;
> >>> +
> >>> +     if (!importOnly_) {
> >>> +             if (count) {
> >>> +                     /* Export some frame buffers for internal use. */
> >>> +                     ret = dev_->exportBuffers(count, &internalBuffers_);
> >>> +                     if (ret < 0)
> >>> +                             return ret;
> >>> +
> >>> +                     /* Add these exported buffers to the internal/external buffer list. */
> >>> +                     setExportedBuffers(&internalBuffers_);
> >>> +
> >>> +                     /* Add these buffers to the queue of internal usable buffers. */
> >>> +                     for (auto const &buffer : internalBuffers_)
> >>> +                             availableBuffers_.push(buffer.get());
> >>> +             }
> >>> +
> >>> +             /* We must import all internal/external exported buffers. */
> >>> +             count = bufferList_.size();
> >>> +     }
> >>> +
> >>>       return dev_->importBuffers(count);
> >>>  }
> >>>
> >>> -int RPiStream::allocateBuffers(unsigned int count)
> >>> +int RPiStream::queueAllBuffers()
> >>>  {
> >>> -     return dev_->allocateBuffers(count, &internalBuffers_);
> >>> -}
> >>> +     int ret;
> >>>
> >>> -int RPiStream::queueBuffers()
> >>> -{
> >>>       if (external_)
> >>>               return 0;
> >>>
> >>> -     for (auto &b : internalBuffers_) {
> >>> -             int ret = dev_->queueBuffer(b.get());
> >>> -             if (ret) {
> >>> -                     LOG(RPISTREAM, Error) << "Failed to queue buffers for "
> >>> -                                           << name_;
> >>> +     while (!availableBuffers_.empty()) {
> >>> +             ret = queueBuffer(availableBuffers_.front());
> >>> +             if (ret < 0)
> >>>                       return ret;
> >>> -             }
> >>> +
> >>> +             availableBuffers_.pop();
> >>>       }
> >>>
> >>>       return 0;
> >>>  }
> >>>
> >>> -bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const
> >>> +int RPiStream::queueBuffer(FrameBuffer *buffer)
> >>> +{
> >>> +     /*
> >>> +      * A nullptr buffer implies an external stream, but no external
> >>> +      * buffer has been supplied. So, pick one from the availableBuffers_
> >>> +      * queue.
> 
> Aha, now I see this here, maybe the comment addition I suggested above
> is possibly redundant, as it's explained here ...
> 
> >>> +      */
> >>> +     if (!buffer) {
> >>> +             if (availableBuffers_.empty()) {
> >>> +                     LOG(RPISTREAM, Warning) << "No buffers available for "
> >>> +                                             << name_;
> >>> +                     return -EINVAL;
> >>> +             }
> >>> +
> >>> +             buffer = availableBuffers_.front();
> >>> +             availableBuffers_.pop();
> >>> +     }
> >>> +
> >>> +     LOG(RPISTREAM, Debug) << "Queuing buffer " << buffer->cookie()
> >>> +                           << " for " << name_;
> >>> +
> >>> +     int ret = dev_->queueBuffer(buffer);
> >>> +     if (ret) {
> >>> +             LOG(RPISTREAM, Error) << "Failed to queue buffer for "
> >>> +                                   << name_;
> >>> +     }
> >>> +
> >>> +     return ret;
> >>> +}
> >>> +
> >>> +void RPiStream::returnBuffer(FrameBuffer *buffer)
> >>>  {
> >>> -     auto start = external_ ? externalBuffers_->begin() : internalBuffers_.begin();
> >>> -     auto end = external_ ? externalBuffers_->end() : internalBuffers_.end();
> >>> +     /* This can only be called for external streams. */
> >>> +     assert(external_);
> 
> Could this use ASSERT() from libcamera/internal/log.h ?
> 
> >>> +
> >>> +     availableBuffers_.push(buffer);
> >>> +}
> >>>
> >>> +bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const
> >>> +{
> >>>       if (importOnly_)
> >>>               return false;
> >>>
> >>> -     if (std::find_if(start, end,
> >>> -                      [buffer](std::unique_ptr<FrameBuffer> const &ref) { return ref.get() == buffer; }) != end)
> >>> +     if (std::find(bufferList_.begin(), bufferList_.end(), buffer) != bufferList_.end())
> >>>               return true;
> >>>
> >>>       return false;
> >>>  }
> >>>
> >>> +void RPiStream::clearBuffers()
> >>> +{
> >>> +     availableBuffers_ = std::queue<FrameBuffer *>{};
> >>> +     internalBuffers_.clear();
> >>> +     bufferList_.clear();
> >>> +}
> >>> +
> >>>  } /* namespace RPi */
> >>>
> >>>  } /* namespace libcamera */
> >>> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> >>> index 3957e342..af9c2ad2 100644
> >>> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> >>> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> >>> @@ -39,21 +39,22 @@ public:
> >>>       V4L2VideoDevice *dev() const;
> >>>       void setExternal(bool external);
> >>>       bool isExternal() const;
> >>> -     bool isImporter() const;
> >>>       void reset();
> >>>       std::string name() const;
> >>> -     void setExternalBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> >>> -     const std::vector<std::unique_ptr<FrameBuffer>> *getBuffers() const;
> >>> +     void setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> >>> +     const std::vector<FrameBuffer *> &getBuffers() const;
> >>>       void releaseBuffers();
> >>> -     int importBuffers(unsigned int count);
> >>> -     int allocateBuffers(unsigned int count);
> >>> -     int queueBuffers();
> >>> +     int prepareBuffers(unsigned int count);
> >>> +     int queueAllBuffers();
> >>> +     int queueBuffer(FrameBuffer *buffer);
> >>> +     void returnBuffer(FrameBuffer *buffer);
> >>>       bool findFrameBuffer(FrameBuffer *buffer) const;
> >>
> >> Perhaps we should break those up into sections in patch 1/9. It's hard
> >> to see the wood-for-the-trees in the wall of function declarations.
> > 
> > Not sure I follow what you mean?
> 
> There is a lot of text all bunched up together, so it's hard to get my
> eyes around the class members.
> 
> It would be nice to group functions with some line breaks to make it
> easier on the eye.
> 
> But that should happen in patch 1/9, so I'll go comment on it there. I
> skipped it before because that patch just moves code around, but I thnik
> it can also improve formatting for readability.
> 
> >>>
> >>>  private:
> >>> +     void clearBuffers();
> >>>       /*
> >>>        * Indicates that this stream is active externally, i.e. the buffers
> >>> -      * are provided by the application.
> >>> +      * might be provided by (and returned to) the application.
> >>>        */
> >>>       bool external_;
> >>>       /* Indicates that this stream only imports buffers, e.g. ISP input. */
> >>> @@ -62,10 +63,19 @@ private:
> >>>       std::string name_;
> >>>       /* The actual device stream. */
> >>>       std::unique_ptr<V4L2VideoDevice> dev_;
> >>> -     /* Internally allocated framebuffers associated with this device stream. */
> >>> +     /* All framebuffers associated with this device stream. */
> >>> +     std::vector<FrameBuffer *> bufferList_;
> >>> +     /*
> >>> +      * List of frame buffer that we can use if none have been provided by
> >>> +      * the application for external streams. This is populated by the
> >>> +      * buffers exported internally.
> >>> +      */
> >>> +     std::queue<FrameBuffer *> availableBuffers_;
> >>> +     /*
> >>> +      * This is a list of buffers exported internally. Need to keep this around
> >>> +      * as the stream needs to maintain ownership of these buffers.
> >>> +      */
> >>>       std::vector<std::unique_ptr<FrameBuffer>> internalBuffers_;
> >>> -     /* Externally allocated framebuffers associated with this device stream. */
> >>> -     std::vector<std::unique_ptr<FrameBuffer>> *externalBuffers_;
> >>
> >> Likewise here, some whitespace to break up these members would really
> >> help, as by the time I've got here I've got definite review fatigue and
> >> lots of lines close together just become a blur ... :S
> > 
> > That's not a problem.
> 
> But for this patch, I can't see anything wrong ... and I can see how
> it's helping manage buffers to allow optional buffers on a stream for
> RAW zero copy support.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> >>>  };
> >>>
> >>>  /*

Hi Laurent,

Thank you for the review.

On Wed, 22 Jul 2020 at 16:25, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Naush,
>
> Thank you for the patch.
>
> On Wed, Jul 22, 2020 at 01:21:33PM +0100, Kieran Bingham wrote:
> > On 22/07/2020 09:04, Naushir Patuck wrote:
> > > On Tue, 21 Jul 2020 at 16:41, Kieran Bingham wrote:
> > >> On 20/07/2020 10:13, Naushir Patuck wrote:
> > >>> Stop using v4l2_videodevice::allocateBuffer() for internal buffers and
> > >>> instead export/import all buffers. This allows the pipeline to return
> > >>> any stream buffer requested by the application as zero-copy.
> > >>>
> > >>> Advertise the Unicam Image stream as the RAW capture stream now.
> > >>>
> > >>> The RPiStream object now maintains a new list of buffers that are
> > >>> available to queue into a device. This is needed to distinguish between
> > >>> FrameBuffers allocated for internal use vs externally provided buffers.
> > >>> When a Request comes in, if a buffer is not provided for an exported
> > >>> stream, we re-use a buffer from this list.
> > >>>
> > >>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > >>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > >>
> > >> A few small nit fixups below but they could possibly be fixed while
> > >> applying anyway if you prefer.
> > >>
> > >> I've hit review fatigue though, so I'll look at this again tomorrow.
> > >
> > > I understand :)
> > > I will make the necessary fixups based on your review comments, and
> > > submit a new patch set.  Will wait until you have finished reviewing
> > > this one before I send the next revision.
> >
> > Looking good, I think the only actionable thing remaining here is
> > potentially to use ASSERT() over assert() :-)
> >
> > >>> ---
> > >>>  .../pipeline/raspberrypi/raspberrypi.cpp      | 228 ++++++++++--------
> > >>>  .../pipeline/raspberrypi/rpi_stream.cpp       | 122 +++++++---
> > >>>  .../pipeline/raspberrypi/rpi_stream.h         |  30 ++-
> > >>>  3 files changed, 226 insertions(+), 154 deletions(-)
> > >>>
> > >>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > >>> index 8f6a999b..dbc22521 100644
> > >>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > >>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > >>> @@ -187,7 +187,8 @@ private:
> > >>>       void checkRequestCompleted();
> > >>>       void tryRunPipeline();
> > >>>       void tryFlushQueues();
> > >>> -     FrameBuffer *updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp, V4L2VideoDevice *dev);
> > >>> +     FrameBuffer *updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp,
> > >>> +                              RPi::RPiStream *stream);
> > >>>
> > >>>       unsigned int ispOutputCount_;
> > >>>  };
> > >>> @@ -508,8 +509,15 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> > >>>               StreamConfiguration &cfg = config->at(i);
> > >>>
> > >>>               if (isRaw(cfg.pixelFormat)) {
> > >>> -                     cfg.setStream(&data->isp_[Isp::Input]);
> > >>> -                     data->isp_[Isp::Input].setExternal(true);
> > >>> +                     cfg.setStream(&data->unicam_[Unicam::Image]);
> > >>> +                     /*
> > >>> +                      * We must set both Unicam streams as external, even
> > >>> +                      * though the application may only request RAW frames.
> > >>> +                      * This is because we match timestamps on both streams
> > >>> +                      * to synchronise buffers.
> > >>> +                      */
> > >>> +                     data->unicam_[Unicam::Image].setExternal(true);
> > >>> +                     data->unicam_[Unicam::Embedded].setExternal(true);
> > >>>                       continue;
> > >>>               }
> > >>>
> > >>> @@ -612,7 +620,7 @@ int PipelineHandlerRPi::exportFrameBuffers(Camera *camera, Stream *stream,
> > >>>       unsigned int count = stream->configuration().bufferCount;
> > >>>       int ret = s->dev()->exportBuffers(count, buffers);
> > >>>
> > >>> -     s->setExternalBuffers(buffers);
> > >>> +     s->setExportedBuffers(buffers);
> > >>>
> > >>>       return ret;
> > >>>  }
> > >>> @@ -712,14 +720,23 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)
> > >>>       if (data->state_ == RPiCameraData::State::Stopped)
> > >>>               return -EINVAL;
> > >>>
> > >>> -     /* Ensure all external streams have associated buffers! */
> > >>> -     for (auto &stream : data->isp_) {
> > >>> -             if (!stream.isExternal())
> > >>> -                     continue;
> > >>> +     LOG(RPI, Debug) << "queueRequestDevice: New request.";
> > >>>
> > >>> -             if (!request->findBuffer(&stream)) {
> > >>> -                     LOG(RPI, Error) << "Attempt to queue request with invalid stream.";
> > >>> -                     return -ENOENT;
> > >>> +     /* Push all buffers supplied in the Request to the respective streams. */
> > >>> +     for (auto stream : data->streams_) {
> > >>> +             if (stream->isExternal()) {
> > >>
> > >> Are all streams now 'external' ?
> > >
> > > All streams but the ISP input (which only imports buffers) *can* be
> > > external.  They are marked external only if the app configures the
> > > pipeline handler by saying it may request a buffer from the stream.
> > > In reality, all streams could be marked external all the time, but the
> > > buffer handling will take a slightly less efficient path, so I kept
> > > the distinction.
> > >
> > >>> +                     FrameBuffer *buffer = request->findBuffer(stream);
> > >>> +                     /*
> > >>> +                      * A nullptr in buffer means that we should queue an internally
> > >>> +                      * allocated buffer.
> >
> > Now I understand what's going on here, perhaps this could be expanded a
> > bit to explain?
> >
> > "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 which will not be given back
> > to the application, but can be used to support the internal pipeline flow."
> >
> > Now I see how this leads towards zero-copy raw support ;-)
> >
> > >>> +                      *
> > >>> +                      * 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;
> > >>>               }
> > >>>       }
> > >>>
> > >>> @@ -808,12 +825,10 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
> > >>>       /* Initialize the camera properties. */
> > >>>       data->properties_ = data->sensor_->properties();
> > >>>
> > >>> -     /*
> > >>> -      * List the available output streams.
> > >>> -      * Currently cannot do Unicam streams!
> > >>> -      */
> > >>> +     /*List the available streams an application may request. */
> > >>
> > >> Minor nit, there's a space missing between /* and List ...
> > >>
> > >>>       std::set<Stream *> streams;
> > >>> -     streams.insert(&data->isp_[Isp::Input]);
> > >>> +     streams.insert(&data->unicam_[Unicam::Image]);
> > >>> +     streams.insert(&data->unicam_[Unicam::Embedded]);
>
> Can an application request the embedded data ?

With this change, we can now advertise embedded data as well as isp
statistics available for the application to request.  The only thing
missing is a way for the application to do so.  In a separate
discussion we talked about libcamera::PixelFormat not being the right
place as these are data formats, not image formats.  No conclusion has
been reached yet, but I will resurrect that discussion shortly :-)

>
> > >>>       streams.insert(&data->isp_[Isp::Output0]);
> > >>>       streams.insert(&data->isp_[Isp::Output1]);
> > >>>       streams.insert(&data->isp_[Isp::Stats]);
> > >>> @@ -831,9 +846,28 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
> > >>>       int ret;
> > >>>
> > >>>       for (auto const stream : data->streams_) {
> > >>> -             ret = stream->queueBuffers();
> > >>> -             if (ret < 0)
> > >>> -                     return ret;
> > >>> +             if (!stream->isExternal()) {
> > >>> +                     ret = stream->queueAllBuffers();
> > >>> +                     if (ret < 0)
> > >>> +                             return ret;
> > >>> +             } else {
> > >>> +                     /*
> > >>> +                      * For external streams, we must queue up a set of internal
> > >>> +                      * buffers to handle the number of drop frames requested by
> > >>> +                      * the IPA. This is done by passing nullptr in queueBuffer().
> > >>> +                      *
> > >>> +                      * 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.
> > >>> +                      */
> > >>> +                     unsigned int i;
> > >>> +                     for (i = 0; i < data->dropFrameCount_; i++) {
> > >>> +                             int ret = stream->queueBuffer(nullptr);
> > >>> +                             if (ret)
> > >>> +                                     return ret;
> > >>> +                     }
> > >>> +             }
> > >>>       }
> > >>>
> > >>>       return 0;
> > >>> @@ -847,7 +881,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> > >>>       /*
> > >>>        * Decide how many internal buffers to allocate. For now, simply look
> > >>>        * at how many external buffers will be provided. Will need to improve
> > >>> -      * this logic.
> > >>> +      * this logic. However, we really must have all stream allocate the same
> > >>
> > >> s/stream/streams/
> > >>
> > >>> +      * number of buffers to simplify error handling in queueRequestDevice().
> > >>>        */
> > >>
> > >> Does this include Raw streams? I thought that allocates less buffers? Or
> > >> perhaps it's the same number of internal buffers, and only 2 extra
> > >> external buffers for that case...
> > >
> > > That's correct.  For now all (including Raw) streams allocate the same
> > > number of internal buffers.  Hence the comment that we could improve
> > > this logic, it is a bit wasteful on memory.  This bit does need
> > > refinement at some point.
> > >
> > >>>       unsigned int maxBuffers = 0;
> > >>>       for (const Stream *s : camera->streams())
> > >>> @@ -855,33 +890,9 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> > >>>                       maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);
> > >>>
> > >>>       for (auto const stream : data->streams_) {
> > >>> -             if (stream->isExternal() || stream->isImporter()) {
> > >>> -                     /*
> > >>> -                      * If a stream is marked as external reserve memory to
> > >>> -                      * prepare to import as many buffers are requested in
> > >>> -                      * the stream configuration.
> > >>> -                      *
> > >>> -                      * If a stream is an internal stream with importer
> > >>> -                      * role, reserve as many buffers as possible.
> > >>> -                      */
> > >>> -                     unsigned int count = stream->isExternal()
> > >>> -                                                  ? stream->configuration().bufferCount
> > >>> -                                                  : maxBuffers;
> > >>> -                     ret = stream->importBuffers(count);
> > >>> -                     if (ret < 0)
> > >>> -                             return ret;
> > >>> -             } else {
> > >>> -                     /*
> > >>> -                      * If the stream is an internal exporter allocate and
> > >>> -                      * export as many buffers as possible to its internal
> > >>> -                      * pool.
> > >>> -                      */
> > >>> -                     ret = stream->allocateBuffers(maxBuffers);
> > >>> -                     if (ret < 0) {
> > >>> -                             freeBuffers(camera);
> > >>> -                             return ret;
> > >>> -                     }
> > >>> -             }
> > >>> +             ret = stream->prepareBuffers(maxBuffers);
> > >>> +             if (ret < 0)
> > >>> +                     return ret;
>
> This doesn't free buffers anymore on failure, is that an oversight ?

Oversight.  Will fix.

>
> > >>>       }
> > >>>
> > >>>       /*
> > >>> @@ -889,7 +900,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> > >>>        * the IPA and RPI_IPA_EVENT_SIGNAL_ISP_PREPARE event.
> > >>>        */
> > >>>       count = 0;
> > >>> -     for (auto const &b : *data->unicam_[Unicam::Image].getBuffers()) {
> > >>> +     for (auto const &b : data->unicam_[Unicam::Image].getBuffers()) {
> > >>>               b->setCookie(count++);
> > >>>       }
> > >>>
> > >>> @@ -898,14 +909,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> > >>>        * the IPA.
> > >>>        */
> > >>>       count = 0;
> > >>> -     for (auto const &b : *data->isp_[Isp::Stats].getBuffers()) {
> > >>> +     for (auto const &b : data->isp_[Isp::Stats].getBuffers()) {
> > >>>               b->setCookie(count++);
> > >>>               data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | b->cookie(),
> > >>>                                             .planes = b->planes() });
> > >>>       }
> > >>>
> > >>>       count = 0;
> > >>> -     for (auto const &b : *data->unicam_[Unicam::Embedded].getBuffers()) {
> > >>> +     for (auto const &b : data->unicam_[Unicam::Embedded].getBuffers()) {
> > >>>               b->setCookie(count++);
> > >>>               data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | b->cookie(),
> > >>>                                             .planes = b->planes() });
> > >>> @@ -1066,7 +1077,7 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
> > >>>       switch (action.operation) {
> > >>>       case RPI_IPA_ACTION_STATS_METADATA_COMPLETE: {
> > >>>               unsigned int bufferId = action.data[0];
> > >>> -             FrameBuffer *buffer = isp_[Isp::Stats].getBuffers()->at(bufferId).get();
> > >>> +             FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);
> > >>>
> > >>>               handleStreamBuffer(buffer, &isp_[Isp::Stats]);
> > >>>               /* Fill the Request metadata buffer with what the IPA has provided */
> > >>> @@ -1077,19 +1088,19 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
> > >>>
> > >>>       case RPI_IPA_ACTION_EMBEDDED_COMPLETE: {
> > >>>               unsigned int bufferId = action.data[0];
> > >>> -             FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers()->at(bufferId).get();
> > >>> +             FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);
> > >>>               handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
> > >>>               break;
> > >>>       }
> > >>>
> > >>>       case RPI_IPA_ACTION_RUN_ISP: {
> > >>>               unsigned int bufferId = action.data[0];
> > >>> -             FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers()->at(bufferId).get();
> > >>> +             FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);
> > >>>
> > >>>               LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << buffer->cookie()
> > >>>                               << ", timestamp: " << buffer->metadata().timestamp;
> > >>>
> > >>> -             isp_[Isp::Input].dev()->queueBuffer(buffer);
> > >>> +             isp_[Isp::Input].queueBuffer(buffer);
> > >>>               ispOutputCount_ = 0;
> > >>>               break;
> > >>>       }
> > >>> @@ -1190,22 +1201,27 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
> > >>>                       << ", buffer id " << buffer->cookie()
> > >>>                       << ", timestamp: " << buffer->metadata().timestamp;
> > >>>
> > >>> -     handleStreamBuffer(buffer, stream);
> > >>> -
> > >>>       /*
> > >>> -      * Increment the number of ISP outputs generated.
> > >>> -      * This is needed to track dropped frames.
> > >>> +      * ISP statistics buffer must not be re-queued or sent back to the
> > >>> +      * application until after the IPA signals so.
> > >>>        */
> > >>> -     ispOutputCount_++;
> > >>> -
> > >>> -     /* If this is a stats output, hand it to the IPA now. */
> > >>>       if (stream == &isp_[Isp::Stats]) {
> > >>>               IPAOperationData op;
> > >>>               op.operation = RPI_IPA_EVENT_SIGNAL_STAT_READY;
> > >>>               op.data = { RPiIpaMask::STATS | buffer->cookie() };
> > >>>               ipa_->processEvent(op);
> > >>> +     } else {
> > >>> +             /* Any other ISP output can be handed back to the application now. */
> > >>> +             handleStreamBuffer(buffer, stream);
> > >>>       }
> > >>>
> > >>> +     /*
> > >>> +      * Increment the number of ISP outputs generated.
> > >>> +      * This is needed to track dropped frames.
> > >>> +      */
> > >>> +     ispOutputCount_++;
> > >>> +
> > >>> +
>
> Extra blank line ?
>
> > >>>       handleState();
> > >>>  }
> > >>>
> > >>> @@ -1218,8 +1234,12 @@ void RPiCameraData::clearIncompleteRequests()
> > >>>        */
> > >>>       for (auto const request : requestQueue_) {
> > >>>               for (auto const stream : streams_) {
> > >>> -                     if (stream->isExternal())
> > >>> -                             stream->dev()->queueBuffer(request->findBuffer(stream));
> > >>> +                     if (!stream->isExternal())
> > >>> +                             continue;
> > >>> +
> > >>> +                     FrameBuffer *buffer = request->findBuffer(stream);
> > >>> +                     if (buffer)
> > >>> +                             stream->queueBuffer(buffer);
> > >>>               }
> > >>>       }
> > >>>
> > >>> @@ -1248,7 +1268,7 @@ void RPiCameraData::clearIncompleteRequests()
> > >>>                        * Has the buffer already been handed back to the
> > >>>                        * request? If not, do so now.
> > >>>                        */
> > >>> -                     if (buffer->request())
> > >>> +                     if (buffer && buffer->request())
> > >>>                               pipe_->completeBuffer(camera_, request, buffer);
> > >>>               }
> > >>>
> > >>> @@ -1260,30 +1280,24 @@ void RPiCameraData::clearIncompleteRequests()
> > >>>  void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stream)
> > >>>  {
> > >>>       if (stream->isExternal()) {
> > >>> -             if (!dropFrameCount_) {
> > >>> -                     Request *request = buffer->request();
> > >>> +             Request *request = requestQueue_.front();
> > >>> +
> > >>> +             if (!dropFrameCount_ && request->findBuffer(stream) == buffer) {
> > >>> +                     /*
> > >>> +                      * Tag the buffer as completed, returning it to the
> > >>> +                      * application.
> > >>> +                      */
> > >>>                       pipe_->completeBuffer(camera_, request, buffer);
> > >>> +             } else {
> > >>> +                     /*
> > >>> +                      * This buffer was not part of the Request, so we can
> > >>> +                      * recycle it.
> > >>> +                      */
> > >>> +                     stream->returnBuffer(buffer);
> > >>>               }
> > >>>       } else {
> > >>> -             /* Special handling for RAW buffer Requests.
> > >>> -              *
> > >>> -              * The ISP input stream is alway an import stream, but if the
> > >>> -              * current Request has been made for a buffer on the stream,
> > >>> -              * simply memcpy to the Request buffer and requeue back to the
> > >>> -              * device.
> > >>> -              */
> > >>> -             if (stream == &unicam_[Unicam::Image] && !dropFrameCount_) {
> > >>> -                     const Stream *rawStream = static_cast<const Stream *>(&isp_[Isp::Input]);
> > >>> -                     Request *request = requestQueue_.front();
> > >>> -                     FrameBuffer *raw = request->findBuffer(const_cast<Stream *>(rawStream));
> > >>> -                     if (raw) {
> > >>> -                             raw->copyFrom(buffer);
> > >>> -                             pipe_->completeBuffer(camera_, request, raw);
> > >>> -                     }
> > >>> -             }
> > >>> -
> > >>> -             /* Simply requeue the buffer. */
> > >>> -             stream->dev()->queueBuffer(buffer);
> > >>> +             /* Simply re-queue the buffer to the requested stream. */
> > >>> +             stream->queueBuffer(buffer);
> > >>>       }
> > >>>  }
> > >>>
> > >>> @@ -1367,7 +1381,7 @@ void RPiCameraData::tryRunPipeline()
> > >>>        * current bayer buffer will be removed and re-queued to the driver.
> > >>>        */
> > >>>       embeddedBuffer = updateQueue(embeddedQueue_, bayerBuffer->metadata().timestamp,
> > >>> -                                  unicam_[Unicam::Embedded].dev());
> > >>> +                                  &unicam_[Unicam::Embedded]);
> > >>>
> > >>>       if (!embeddedBuffer) {
> > >>>               LOG(RPI, Debug) << "Could not find matching embedded buffer";
> > >>> @@ -1386,7 +1400,7 @@ void RPiCameraData::tryRunPipeline()
> > >>>
> > >>>               embeddedBuffer = embeddedQueue_.front();
> > >>>               bayerBuffer = updateQueue(bayerQueue_, embeddedBuffer->metadata().timestamp,
> > >>> -                                       unicam_[Unicam::Image].dev());
> > >>> +                                       &unicam_[Unicam::Image]);
> > >>>
> > >>>               if (!bayerBuffer) {
> > >>>                       LOG(RPI, Debug) << "Could not find matching bayer buffer - ending.";
> > >>> @@ -1394,11 +1408,7 @@ void RPiCameraData::tryRunPipeline()
> > >>>               }
> > >>>       }
> > >>>
> > >>> -     /*
> > >>> -      * Take the first request from the queue and action the IPA.
> > >>> -      * Unicam buffers for the request have already been queued as they come
> > >>> -      * in.
> > >>> -      */
> > >>> +     /* Take the first request from the queue and action the IPA. */
> > >>>       Request *request = requestQueue_.front();
> > >>>
> > >>>       /*
> > >>> @@ -1410,12 +1420,6 @@ void RPiCameraData::tryRunPipeline()
> > >>>       op.controls = { request->controls() };
> > >>>       ipa_->processEvent(op);
> > >>>
> > >>> -     /* Queue up any ISP buffers passed into the request. */
> > >>> -     for (auto &stream : isp_) {
> > >>> -             if (stream.isExternal())
> > >>> -                     stream.dev()->queueBuffer(request->findBuffer(&stream));
> > >>> -     }
> > >>> -
> > >>>       /* Ready to use the buffers, pop them off the queue. */
> > >>>       bayerQueue_.pop();
> > >>>       embeddedQueue_.pop();
> > >>> @@ -1445,32 +1449,42 @@ void RPiCameraData::tryFlushQueues()
> > >>>        * and give a chance for the hardware to return to lock-step. We do have
> > >>>        * to drop all interim frames.
> > >>>        */
> > >>> -     if (unicam_[Unicam::Image].getBuffers()->size() == bayerQueue_.size() &&
> > >>> -         unicam_[Unicam::Embedded].getBuffers()->size() == embeddedQueue_.size()) {
> > >>> +     if (unicam_[Unicam::Image].getBuffers().size() == bayerQueue_.size() &&
> > >>> +         unicam_[Unicam::Embedded].getBuffers().size() == embeddedQueue_.size()) {
> > >>> +             /* This cannot happen when Unicam streams are external. */
> > >>> +             assert(!unicam_[Unicam::Image].isExternal());
> > >>> +
> > >>>               LOG(RPI, Warning) << "Flushing all buffer queues!";
> > >>>
> > >>>               while (!bayerQueue_.empty()) {
> > >>> -                     unicam_[Unicam::Image].dev()->queueBuffer(bayerQueue_.front());
> > >>> +                     unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());
> > >>>                       bayerQueue_.pop();
> > >>>               }
> > >>>
> > >>>               while (!embeddedQueue_.empty()) {
> > >>> -                     unicam_[Unicam::Embedded].dev()->queueBuffer(embeddedQueue_.front());
> > >>> +                     unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());
> > >>>                       embeddedQueue_.pop();
> > >>>               }
> > >>>       }
> > >>>  }
> > >>>
> > >>>  FrameBuffer *RPiCameraData::updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp,
> > >>> -                                     V4L2VideoDevice *dev)
> > >>> +                                     RPi::RPiStream *stream)
> > >>>  {
> > >>> +     /*
> > >>> +      * If the unicam streams are external (both have to the same), then we
> > >>
> > >> both have to 'be' the same?
> > >>
> > >>> +      * can only return out the top buffer in the queue, and assume they have
> > >>> +      * been synced by queuing at the same time. We cannot drop these frames,
> > >>> +      * as they may have been provided externally.
> > >>> +      */
> > >>>       while (!q.empty()) {
> > >>>               FrameBuffer *b = q.front();
> > >>> -             if (b->metadata().timestamp < timestamp) {
> > >>> +             if (!stream->isExternal() && b->metadata().timestamp < timestamp) {
> > >>>                       q.pop();
> > >>> -                     dev->queueBuffer(b);
> > >>> -                     LOG(RPI, Warning) << "Dropping input frame!";
> > >>> -             } else if (b->metadata().timestamp == timestamp) {
> > >>> +                     stream->queueBuffer(b);
> > >>> +                     LOG(RPI, Warning) << "Dropping unmatched input frame in stream "
> > >>> +                                       << stream->name();
> > >>> +             } else if (stream->isExternal() || b->metadata().timestamp == timestamp) {
> > >>>                       /* The calling function will pop the item from the queue. */
> > >>>                       return b;
> > >>>               } else {
> > >>> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > >>> index 2edb8b59..02f8d3e0 100644
> > >>> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > >>> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > >>> @@ -21,30 +21,20 @@ V4L2VideoDevice *RPiStream::dev() const
> > >>>
> > >>>  void RPiStream::setExternal(bool external)
> > >>>  {
> > >>> +     /* Import streams cannot be external. */
> > >>> +     assert(!external || !importOnly_);
> > >>>       external_ = external;
> > >>>  }
> > >>>
> > >>>  bool RPiStream::isExternal() const
> > >>>  {
> > >>> -     /*
> > >>> -      * Import streams cannot be external.
> > >>> -      *
> > >>> -      * RAW capture is a special case where we simply copy the RAW
> > >>> -      * buffer out of the request. All other buffer handling happens
> > >>> -      * as if the stream is internal.
> > >>> -      */
> > >>> -     return external_ && !importOnly_;
> > >>> -}
> > >>> -
> > >>> -bool RPiStream::isImporter() const
> > >>> -{
> > >>> -     return importOnly_;
> > >>> +     return external_;
> > >>>  }
> > >>>
> > >>>  void RPiStream::reset()
> > >>>  {
> > >>>       external_ = false;
> > >>> -     internalBuffers_.clear();
> > >>> +     clearBuffers();
> > >>>  }
> > >>>
> > >>>  std::string RPiStream::name() const
> > >>> @@ -52,65 +42,123 @@ std::string RPiStream::name() const
> > >>>       return name_;
> > >>>  }
> > >>>
> > >>> -void RPiStream::setExternalBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > >>> +void RPiStream::setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > >>>  {
> > >>> -     externalBuffers_ = buffers;
> > >>> +     std::transform(buffers->begin(), buffers->end(), std::back_inserter(bufferList_),
> > >>> +                    [](std::unique_ptr<FrameBuffer> &b) { return b.get(); });
> > >>>  }
> > >>>
> > >>> -const std::vector<std::unique_ptr<FrameBuffer>> *RPiStream::getBuffers() const
> > >>> +const std::vector<FrameBuffer *> &RPiStream::getBuffers() const
> > >>>  {
> > >>> -     return external_ ? externalBuffers_ : &internalBuffers_;
> > >>> +     return bufferList_;
> > >>>  }
> > >>>
> > >>>  void RPiStream::releaseBuffers()
> > >>>  {
> > >>>       dev_->releaseBuffers();
> > >>> -     if (!external_ && !importOnly_)
> > >>> -             internalBuffers_.clear();
> > >>> +     clearBuffers();
> > >>>  }
> > >>>
> > >>> -int RPiStream::importBuffers(unsigned int count)
> > >>> +int RPiStream::prepareBuffers(unsigned int count)
> > >>>  {
> > >>> +     int ret;
> > >>> +
> > >>> +     if (!importOnly_) {
> > >>> +             if (count) {
> > >>> +                     /* Export some frame buffers for internal use. */
> > >>> +                     ret = dev_->exportBuffers(count, &internalBuffers_);
> > >>> +                     if (ret < 0)
> > >>> +                             return ret;
> > >>> +
> > >>> +                     /* Add these exported buffers to the internal/external buffer list. */
> > >>> +                     setExportedBuffers(&internalBuffers_);
> > >>> +
> > >>> +                     /* Add these buffers to the queue of internal usable buffers. */
> > >>> +                     for (auto const &buffer : internalBuffers_)
> > >>> +                             availableBuffers_.push(buffer.get());
> > >>> +             }
> > >>> +
> > >>> +             /* We must import all internal/external exported buffers. */
> > >>> +             count = bufferList_.size();
> > >>> +     }
> > >>> +
> > >>>       return dev_->importBuffers(count);
> > >>>  }
> > >>>
> > >>> -int RPiStream::allocateBuffers(unsigned int count)
> > >>> +int RPiStream::queueAllBuffers()
> > >>>  {
> > >>> -     return dev_->allocateBuffers(count, &internalBuffers_);
> > >>> -}
> > >>> +     int ret;
> > >>>
> > >>> -int RPiStream::queueBuffers()
> > >>> -{
> > >>>       if (external_)
> > >>>               return 0;
> > >>>
> > >>> -     for (auto &b : internalBuffers_) {
> > >>> -             int ret = dev_->queueBuffer(b.get());
> > >>> -             if (ret) {
> > >>> -                     LOG(RPISTREAM, Error) << "Failed to queue buffers for "
> > >>> -                                           << name_;
> > >>> +     while (!availableBuffers_.empty()) {
> > >>> +             ret = queueBuffer(availableBuffers_.front());
> > >>> +             if (ret < 0)
> > >>>                       return ret;
> > >>> -             }
> > >>> +
> > >>> +             availableBuffers_.pop();
> > >>>       }
> > >>>
> > >>>       return 0;
> > >>>  }
> > >>>
> > >>> -bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const
> > >>> +int RPiStream::queueBuffer(FrameBuffer *buffer)
> > >>> +{
> > >>> +     /*
> > >>> +      * A nullptr buffer implies an external stream, but no external
> > >>> +      * buffer has been supplied. So, pick one from the availableBuffers_
> > >>> +      * queue.
> >
> > Aha, now I see this here, maybe the comment addition I suggested above
> > is possibly redundant, as it's explained here ...
> >
> > >>> +      */
> > >>> +     if (!buffer) {
> > >>> +             if (availableBuffers_.empty()) {
> > >>> +                     LOG(RPISTREAM, Warning) << "No buffers available for "
> > >>> +                                             << name_;
> > >>> +                     return -EINVAL;
> > >>> +             }
> > >>> +
> > >>> +             buffer = availableBuffers_.front();
> > >>> +             availableBuffers_.pop();
> > >>> +     }
> > >>> +
> > >>> +     LOG(RPISTREAM, Debug) << "Queuing buffer " << buffer->cookie()
> > >>> +                           << " for " << name_;
> > >>> +
> > >>> +     int ret = dev_->queueBuffer(buffer);
> > >>> +     if (ret) {
> > >>> +             LOG(RPISTREAM, Error) << "Failed to queue buffer for "
> > >>> +                                   << name_;
> > >>> +     }
> > >>> +
> > >>> +     return ret;
> > >>> +}
> > >>> +
> > >>> +void RPiStream::returnBuffer(FrameBuffer *buffer)
> > >>>  {
> > >>> -     auto start = external_ ? externalBuffers_->begin() : internalBuffers_.begin();
> > >>> -     auto end = external_ ? externalBuffers_->end() : internalBuffers_.end();
> > >>> +     /* This can only be called for external streams. */
> > >>> +     assert(external_);
> >
> > Could this use ASSERT() from libcamera/internal/log.h ?
> >
> > >>> +
> > >>> +     availableBuffers_.push(buffer);
> > >>> +}
> > >>>
> > >>> +bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const
> > >>> +{
> > >>>       if (importOnly_)
> > >>>               return false;
> > >>>
> > >>> -     if (std::find_if(start, end,
> > >>> -                      [buffer](std::unique_ptr<FrameBuffer> const &ref) { return ref.get() == buffer; }) != end)
> > >>> +     if (std::find(bufferList_.begin(), bufferList_.end(), buffer) != bufferList_.end())
> > >>>               return true;
> > >>>
> > >>>       return false;
> > >>>  }
> > >>>
> > >>> +void RPiStream::clearBuffers()
> > >>> +{
> > >>> +     availableBuffers_ = std::queue<FrameBuffer *>{};
> > >>> +     internalBuffers_.clear();
> > >>> +     bufferList_.clear();
> > >>> +}
> > >>> +
> > >>>  } /* namespace RPi */
> > >>>
> > >>>  } /* namespace libcamera */
> > >>> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > >>> index 3957e342..af9c2ad2 100644
> > >>> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > >>> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > >>> @@ -39,21 +39,22 @@ public:
> > >>>       V4L2VideoDevice *dev() const;
> > >>>       void setExternal(bool external);
> > >>>       bool isExternal() const;
> > >>> -     bool isImporter() const;
> > >>>       void reset();
> > >>>       std::string name() const;
> > >>> -     void setExternalBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> > >>> -     const std::vector<std::unique_ptr<FrameBuffer>> *getBuffers() const;
> > >>> +     void setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> > >>> +     const std::vector<FrameBuffer *> &getBuffers() const;
> > >>>       void releaseBuffers();
> > >>> -     int importBuffers(unsigned int count);
> > >>> -     int allocateBuffers(unsigned int count);
> > >>> -     int queueBuffers();
> > >>> +     int prepareBuffers(unsigned int count);
> > >>> +     int queueAllBuffers();
> > >>> +     int queueBuffer(FrameBuffer *buffer);
> > >>> +     void returnBuffer(FrameBuffer *buffer);
> > >>>       bool findFrameBuffer(FrameBuffer *buffer) const;
> > >>
> > >> Perhaps we should break those up into sections in patch 1/9. It's hard
> > >> to see the wood-for-the-trees in the wall of function declarations.
> > >
> > > Not sure I follow what you mean?
> >
> > There is a lot of text all bunched up together, so it's hard to get my
> > eyes around the class members.
> >
> > It would be nice to group functions with some line breaks to make it
> > easier on the eye.
> >
> > But that should happen in patch 1/9, so I'll go comment on it there. I
> > skipped it before because that patch just moves code around, but I thnik
> > it can also improve formatting for readability.
> >
> > >>>
> > >>>  private:
> > >>> +     void clearBuffers();
> > >>>       /*
> > >>>        * Indicates that this stream is active externally, i.e. the buffers
> > >>> -      * are provided by the application.
> > >>> +      * might be provided by (and returned to) the application.
> > >>>        */
> > >>>       bool external_;
> > >>>       /* Indicates that this stream only imports buffers, e.g. ISP input. */
> > >>> @@ -62,10 +63,19 @@ private:
> > >>>       std::string name_;
> > >>>       /* The actual device stream. */
> > >>>       std::unique_ptr<V4L2VideoDevice> dev_;
> > >>> -     /* Internally allocated framebuffers associated with this device stream. */
> > >>> +     /* All framebuffers associated with this device stream. */
> > >>> +     std::vector<FrameBuffer *> bufferList_;
> > >>> +     /*
> > >>> +      * List of frame buffer that we can use if none have been provided by
> > >>> +      * the application for external streams. This is populated by the
> > >>> +      * buffers exported internally.
> > >>> +      */
> > >>> +     std::queue<FrameBuffer *> availableBuffers_;
> > >>> +     /*
> > >>> +      * This is a list of buffers exported internally. Need to keep this around
> > >>> +      * as the stream needs to maintain ownership of these buffers.
> > >>> +      */
> > >>>       std::vector<std::unique_ptr<FrameBuffer>> internalBuffers_;
> > >>> -     /* Externally allocated framebuffers associated with this device stream. */
> > >>> -     std::vector<std::unique_ptr<FrameBuffer>> *externalBuffers_;
> > >>
> > >> Likewise here, some whitespace to break up these members would really
> > >> help, as by the time I've got here I've got definite review fatigue and
> > >> lots of lines close together just become a blur ... :S
> > >
> > > That's not a problem.
> >
> > But for this patch, I can't see anything wrong ... and I can see how
> > it's helping manage buffers to allow optional buffers on a stream for
> > RAW zero copy support.
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > >>>  };
> > >>>
> > >>>  /*
>
> --
> Regards,
>
> Laurent Pinchart

Regards,
Naush

Hi Naush,

On Wed, Jul 22, 2020 at 04:33:08PM +0100, Naushir Patuck wrote:
> On Wed, 22 Jul 2020 at 16:25, Laurent Pinchart wrote:
> > On Wed, Jul 22, 2020 at 01:21:33PM +0100, Kieran Bingham wrote:
> >> On 22/07/2020 09:04, Naushir Patuck wrote:
> >>> On Tue, 21 Jul 2020 at 16:41, Kieran Bingham wrote:
> >>>> On 20/07/2020 10:13, Naushir Patuck wrote:
> >>>>> Stop using v4l2_videodevice::allocateBuffer() for internal buffers and
> >>>>> instead export/import all buffers. This allows the pipeline to return
> >>>>> any stream buffer requested by the application as zero-copy.
> >>>>>
> >>>>> Advertise the Unicam Image stream as the RAW capture stream now.
> >>>>>
> >>>>> The RPiStream object now maintains a new list of buffers that are
> >>>>> available to queue into a device. This is needed to distinguish between
> >>>>> FrameBuffers allocated for internal use vs externally provided buffers.
> >>>>> When a Request comes in, if a buffer is not provided for an exported
> >>>>> stream, we re-use a buffer from this list.
> >>>>>
> >>>>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> >>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> >>>>
> >>>> A few small nit fixups below but they could possibly be fixed while
> >>>> applying anyway if you prefer.
> >>>>
> >>>> I've hit review fatigue though, so I'll look at this again tomorrow.
> >>>
> >>> I understand :)
> >>> I will make the necessary fixups based on your review comments, and
> >>> submit a new patch set.  Will wait until you have finished reviewing
> >>> this one before I send the next revision.
> >>
> >> Looking good, I think the only actionable thing remaining here is
> >> potentially to use ASSERT() over assert() :-)
> >>
> >>>>> ---
> >>>>>  .../pipeline/raspberrypi/raspberrypi.cpp      | 228 ++++++++++--------
> >>>>>  .../pipeline/raspberrypi/rpi_stream.cpp       | 122 +++++++---
> >>>>>  .../pipeline/raspberrypi/rpi_stream.h         |  30 ++-
> >>>>>  3 files changed, 226 insertions(+), 154 deletions(-)
> >>>>>
> >>>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>>>> index 8f6a999b..dbc22521 100644
> >>>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>>>> @@ -187,7 +187,8 @@ private:
> >>>>>       void checkRequestCompleted();
> >>>>>       void tryRunPipeline();
> >>>>>       void tryFlushQueues();
> >>>>> -     FrameBuffer *updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp, V4L2VideoDevice *dev);
> >>>>> +     FrameBuffer *updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp,
> >>>>> +                              RPi::RPiStream *stream);
> >>>>>
> >>>>>       unsigned int ispOutputCount_;
> >>>>>  };
> >>>>> @@ -508,8 +509,15 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> >>>>>               StreamConfiguration &cfg = config->at(i);
> >>>>>
> >>>>>               if (isRaw(cfg.pixelFormat)) {
> >>>>> -                     cfg.setStream(&data->isp_[Isp::Input]);
> >>>>> -                     data->isp_[Isp::Input].setExternal(true);
> >>>>> +                     cfg.setStream(&data->unicam_[Unicam::Image]);
> >>>>> +                     /*
> >>>>> +                      * We must set both Unicam streams as external, even
> >>>>> +                      * though the application may only request RAW frames.
> >>>>> +                      * This is because we match timestamps on both streams
> >>>>> +                      * to synchronise buffers.
> >>>>> +                      */
> >>>>> +                     data->unicam_[Unicam::Image].setExternal(true);
> >>>>> +                     data->unicam_[Unicam::Embedded].setExternal(true);
> >>>>>                       continue;
> >>>>>               }
> >>>>>
> >>>>> @@ -612,7 +620,7 @@ int PipelineHandlerRPi::exportFrameBuffers(Camera *camera, Stream *stream,
> >>>>>       unsigned int count = stream->configuration().bufferCount;
> >>>>>       int ret = s->dev()->exportBuffers(count, buffers);
> >>>>>
> >>>>> -     s->setExternalBuffers(buffers);
> >>>>> +     s->setExportedBuffers(buffers);
> >>>>>
> >>>>>       return ret;
> >>>>>  }
> >>>>> @@ -712,14 +720,23 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)
> >>>>>       if (data->state_ == RPiCameraData::State::Stopped)
> >>>>>               return -EINVAL;
> >>>>>
> >>>>> -     /* Ensure all external streams have associated buffers! */
> >>>>> -     for (auto &stream : data->isp_) {
> >>>>> -             if (!stream.isExternal())
> >>>>> -                     continue;
> >>>>> +     LOG(RPI, Debug) << "queueRequestDevice: New request.";
> >>>>>
> >>>>> -             if (!request->findBuffer(&stream)) {
> >>>>> -                     LOG(RPI, Error) << "Attempt to queue request with invalid stream.";
> >>>>> -                     return -ENOENT;
> >>>>> +     /* Push all buffers supplied in the Request to the respective streams. */
> >>>>> +     for (auto stream : data->streams_) {
> >>>>> +             if (stream->isExternal()) {
> >>>>
> >>>> Are all streams now 'external' ?
> >>>
> >>> All streams but the ISP input (which only imports buffers) *can* be
> >>> external.  They are marked external only if the app configures the
> >>> pipeline handler by saying it may request a buffer from the stream.
> >>> In reality, all streams could be marked external all the time, but the
> >>> buffer handling will take a slightly less efficient path, so I kept
> >>> the distinction.
> >>>
> >>>>> +                     FrameBuffer *buffer = request->findBuffer(stream);
> >>>>> +                     /*
> >>>>> +                      * A nullptr in buffer means that we should queue an internally
> >>>>> +                      * allocated buffer.
> >>
> >> Now I understand what's going on here, perhaps this could be expanded a
> >> bit to explain?
> >>
> >> "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 which will not be given back
> >> to the application, but can be used to support the internal pipeline flow."
> >>
> >> Now I see how this leads towards zero-copy raw support ;-)
> >>
> >>>>> +                      *
> >>>>> +                      * 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;
> >>>>>               }
> >>>>>       }
> >>>>>
> >>>>> @@ -808,12 +825,10 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)
> >>>>>       /* Initialize the camera properties. */
> >>>>>       data->properties_ = data->sensor_->properties();
> >>>>>
> >>>>> -     /*
> >>>>> -      * List the available output streams.
> >>>>> -      * Currently cannot do Unicam streams!
> >>>>> -      */
> >>>>> +     /*List the available streams an application may request. */
> >>>>
> >>>> Minor nit, there's a space missing between /* and List ...
> >>>>
> >>>>>       std::set<Stream *> streams;
> >>>>> -     streams.insert(&data->isp_[Isp::Input]);
> >>>>> +     streams.insert(&data->unicam_[Unicam::Image]);
> >>>>> +     streams.insert(&data->unicam_[Unicam::Embedded]);
> >
> > Can an application request the embedded data ?
> 
> With this change, we can now advertise embedded data as well as isp
> statistics available for the application to request.  The only thing
> missing is a way for the application to do so.

Could we avoid exposing non-image streams to applications in the
meantime though ?

> In a separate discussion we talked about libcamera::PixelFormat not
> being the right place as these are data formats, not image formats.
> No conclusion has been reached yet, but I will resurrect that
> discussion shortly :-)

Take your time :-) Jokes aside, I'm working on support for
memory-to-memory processing (a.k.a. reprocessing), and I think that
should be merged first as a base to provide statistics to applications.
I expect it will take at least a couple of months until the reprocessing
API is ready, so there's no hurry at all.

> >>>>>       streams.insert(&data->isp_[Isp::Output0]);
> >>>>>       streams.insert(&data->isp_[Isp::Output1]);
> >>>>>       streams.insert(&data->isp_[Isp::Stats]);
> >>>>> @@ -831,9 +846,28 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
> >>>>>       int ret;
> >>>>>
> >>>>>       for (auto const stream : data->streams_) {
> >>>>> -             ret = stream->queueBuffers();
> >>>>> -             if (ret < 0)
> >>>>> -                     return ret;
> >>>>> +             if (!stream->isExternal()) {
> >>>>> +                     ret = stream->queueAllBuffers();
> >>>>> +                     if (ret < 0)
> >>>>> +                             return ret;
> >>>>> +             } else {
> >>>>> +                     /*
> >>>>> +                      * For external streams, we must queue up a set of internal
> >>>>> +                      * buffers to handle the number of drop frames requested by
> >>>>> +                      * the IPA. This is done by passing nullptr in queueBuffer().
> >>>>> +                      *
> >>>>> +                      * 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.
> >>>>> +                      */
> >>>>> +                     unsigned int i;
> >>>>> +                     for (i = 0; i < data->dropFrameCount_; i++) {
> >>>>> +                             int ret = stream->queueBuffer(nullptr);
> >>>>> +                             if (ret)
> >>>>> +                                     return ret;
> >>>>> +                     }
> >>>>> +             }
> >>>>>       }
> >>>>>
> >>>>>       return 0;
> >>>>> @@ -847,7 +881,8 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> >>>>>       /*
> >>>>>        * Decide how many internal buffers to allocate. For now, simply look
> >>>>>        * at how many external buffers will be provided. Will need to improve
> >>>>> -      * this logic.
> >>>>> +      * this logic. However, we really must have all stream allocate the same
> >>>>
> >>>> s/stream/streams/
> >>>>
> >>>>> +      * number of buffers to simplify error handling in queueRequestDevice().
> >>>>>        */
> >>>>
> >>>> Does this include Raw streams? I thought that allocates less buffers? Or
> >>>> perhaps it's the same number of internal buffers, and only 2 extra
> >>>> external buffers for that case...
> >>>
> >>> That's correct.  For now all (including Raw) streams allocate the same
> >>> number of internal buffers.  Hence the comment that we could improve
> >>> this logic, it is a bit wasteful on memory.  This bit does need
> >>> refinement at some point.
> >>>
> >>>>>       unsigned int maxBuffers = 0;
> >>>>>       for (const Stream *s : camera->streams())
> >>>>> @@ -855,33 +890,9 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> >>>>>                       maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);
> >>>>>
> >>>>>       for (auto const stream : data->streams_) {
> >>>>> -             if (stream->isExternal() || stream->isImporter()) {
> >>>>> -                     /*
> >>>>> -                      * If a stream is marked as external reserve memory to
> >>>>> -                      * prepare to import as many buffers are requested in
> >>>>> -                      * the stream configuration.
> >>>>> -                      *
> >>>>> -                      * If a stream is an internal stream with importer
> >>>>> -                      * role, reserve as many buffers as possible.
> >>>>> -                      */
> >>>>> -                     unsigned int count = stream->isExternal()
> >>>>> -                                                  ? stream->configuration().bufferCount
> >>>>> -                                                  : maxBuffers;
> >>>>> -                     ret = stream->importBuffers(count);
> >>>>> -                     if (ret < 0)
> >>>>> -                             return ret;
> >>>>> -             } else {
> >>>>> -                     /*
> >>>>> -                      * If the stream is an internal exporter allocate and
> >>>>> -                      * export as many buffers as possible to its internal
> >>>>> -                      * pool.
> >>>>> -                      */
> >>>>> -                     ret = stream->allocateBuffers(maxBuffers);
> >>>>> -                     if (ret < 0) {
> >>>>> -                             freeBuffers(camera);
> >>>>> -                             return ret;
> >>>>> -                     }
> >>>>> -             }
> >>>>> +             ret = stream->prepareBuffers(maxBuffers);
> >>>>> +             if (ret < 0)
> >>>>> +                     return ret;
> >
> > This doesn't free buffers anymore on failure, is that an oversight ?
> 
> Oversight.  Will fix.
> 
> >
> >>>>>       }
> >>>>>
> >>>>>       /*
> >>>>> @@ -889,7 +900,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> >>>>>        * the IPA and RPI_IPA_EVENT_SIGNAL_ISP_PREPARE event.
> >>>>>        */
> >>>>>       count = 0;
> >>>>> -     for (auto const &b : *data->unicam_[Unicam::Image].getBuffers()) {
> >>>>> +     for (auto const &b : data->unicam_[Unicam::Image].getBuffers()) {
> >>>>>               b->setCookie(count++);
> >>>>>       }
> >>>>>
> >>>>> @@ -898,14 +909,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> >>>>>        * the IPA.
> >>>>>        */
> >>>>>       count = 0;
> >>>>> -     for (auto const &b : *data->isp_[Isp::Stats].getBuffers()) {
> >>>>> +     for (auto const &b : data->isp_[Isp::Stats].getBuffers()) {
> >>>>>               b->setCookie(count++);
> >>>>>               data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | b->cookie(),
> >>>>>                                             .planes = b->planes() });
> >>>>>       }
> >>>>>
> >>>>>       count = 0;
> >>>>> -     for (auto const &b : *data->unicam_[Unicam::Embedded].getBuffers()) {
> >>>>> +     for (auto const &b : data->unicam_[Unicam::Embedded].getBuffers()) {
> >>>>>               b->setCookie(count++);
> >>>>>               data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | b->cookie(),
> >>>>>                                             .planes = b->planes() });
> >>>>> @@ -1066,7 +1077,7 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
> >>>>>       switch (action.operation) {
> >>>>>       case RPI_IPA_ACTION_STATS_METADATA_COMPLETE: {
> >>>>>               unsigned int bufferId = action.data[0];
> >>>>> -             FrameBuffer *buffer = isp_[Isp::Stats].getBuffers()->at(bufferId).get();
> >>>>> +             FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);
> >>>>>
> >>>>>               handleStreamBuffer(buffer, &isp_[Isp::Stats]);
> >>>>>               /* Fill the Request metadata buffer with what the IPA has provided */
> >>>>> @@ -1077,19 +1088,19 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
> >>>>>
> >>>>>       case RPI_IPA_ACTION_EMBEDDED_COMPLETE: {
> >>>>>               unsigned int bufferId = action.data[0];
> >>>>> -             FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers()->at(bufferId).get();
> >>>>> +             FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);
> >>>>>               handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
> >>>>>               break;
> >>>>>       }
> >>>>>
> >>>>>       case RPI_IPA_ACTION_RUN_ISP: {
> >>>>>               unsigned int bufferId = action.data[0];
> >>>>> -             FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers()->at(bufferId).get();
> >>>>> +             FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);
> >>>>>
> >>>>>               LOG(RPI, Debug) << "Input re-queue to ISP, buffer id " << buffer->cookie()
> >>>>>                               << ", timestamp: " << buffer->metadata().timestamp;
> >>>>>
> >>>>> -             isp_[Isp::Input].dev()->queueBuffer(buffer);
> >>>>> +             isp_[Isp::Input].queueBuffer(buffer);
> >>>>>               ispOutputCount_ = 0;
> >>>>>               break;
> >>>>>       }
> >>>>> @@ -1190,22 +1201,27 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
> >>>>>                       << ", buffer id " << buffer->cookie()
> >>>>>                       << ", timestamp: " << buffer->metadata().timestamp;
> >>>>>
> >>>>> -     handleStreamBuffer(buffer, stream);
> >>>>> -
> >>>>>       /*
> >>>>> -      * Increment the number of ISP outputs generated.
> >>>>> -      * This is needed to track dropped frames.
> >>>>> +      * ISP statistics buffer must not be re-queued or sent back to the
> >>>>> +      * application until after the IPA signals so.
> >>>>>        */
> >>>>> -     ispOutputCount_++;
> >>>>> -
> >>>>> -     /* If this is a stats output, hand it to the IPA now. */
> >>>>>       if (stream == &isp_[Isp::Stats]) {
> >>>>>               IPAOperationData op;
> >>>>>               op.operation = RPI_IPA_EVENT_SIGNAL_STAT_READY;
> >>>>>               op.data = { RPiIpaMask::STATS | buffer->cookie() };
> >>>>>               ipa_->processEvent(op);
> >>>>> +     } else {
> >>>>> +             /* Any other ISP output can be handed back to the application now. */
> >>>>> +             handleStreamBuffer(buffer, stream);
> >>>>>       }
> >>>>>
> >>>>> +     /*
> >>>>> +      * Increment the number of ISP outputs generated.
> >>>>> +      * This is needed to track dropped frames.
> >>>>> +      */
> >>>>> +     ispOutputCount_++;
> >>>>> +
> >>>>> +
> >
> > Extra blank line ?
> >
> >>>>>       handleState();
> >>>>>  }
> >>>>>
> >>>>> @@ -1218,8 +1234,12 @@ void RPiCameraData::clearIncompleteRequests()
> >>>>>        */
> >>>>>       for (auto const request : requestQueue_) {
> >>>>>               for (auto const stream : streams_) {
> >>>>> -                     if (stream->isExternal())
> >>>>> -                             stream->dev()->queueBuffer(request->findBuffer(stream));
> >>>>> +                     if (!stream->isExternal())
> >>>>> +                             continue;
> >>>>> +
> >>>>> +                     FrameBuffer *buffer = request->findBuffer(stream);
> >>>>> +                     if (buffer)
> >>>>> +                             stream->queueBuffer(buffer);
> >>>>>               }
> >>>>>       }
> >>>>>
> >>>>> @@ -1248,7 +1268,7 @@ void RPiCameraData::clearIncompleteRequests()
> >>>>>                        * Has the buffer already been handed back to the
> >>>>>                        * request? If not, do so now.
> >>>>>                        */
> >>>>> -                     if (buffer->request())
> >>>>> +                     if (buffer && buffer->request())
> >>>>>                               pipe_->completeBuffer(camera_, request, buffer);
> >>>>>               }
> >>>>>
> >>>>> @@ -1260,30 +1280,24 @@ void RPiCameraData::clearIncompleteRequests()
> >>>>>  void RPiCameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::RPiStream *stream)
> >>>>>  {
> >>>>>       if (stream->isExternal()) {
> >>>>> -             if (!dropFrameCount_) {
> >>>>> -                     Request *request = buffer->request();
> >>>>> +             Request *request = requestQueue_.front();
> >>>>> +
> >>>>> +             if (!dropFrameCount_ && request->findBuffer(stream) == buffer) {
> >>>>> +                     /*
> >>>>> +                      * Tag the buffer as completed, returning it to the
> >>>>> +                      * application.
> >>>>> +                      */
> >>>>>                       pipe_->completeBuffer(camera_, request, buffer);
> >>>>> +             } else {
> >>>>> +                     /*
> >>>>> +                      * This buffer was not part of the Request, so we can
> >>>>> +                      * recycle it.
> >>>>> +                      */
> >>>>> +                     stream->returnBuffer(buffer);
> >>>>>               }
> >>>>>       } else {
> >>>>> -             /* Special handling for RAW buffer Requests.
> >>>>> -              *
> >>>>> -              * The ISP input stream is alway an import stream, but if the
> >>>>> -              * current Request has been made for a buffer on the stream,
> >>>>> -              * simply memcpy to the Request buffer and requeue back to the
> >>>>> -              * device.
> >>>>> -              */
> >>>>> -             if (stream == &unicam_[Unicam::Image] && !dropFrameCount_) {
> >>>>> -                     const Stream *rawStream = static_cast<const Stream *>(&isp_[Isp::Input]);
> >>>>> -                     Request *request = requestQueue_.front();
> >>>>> -                     FrameBuffer *raw = request->findBuffer(const_cast<Stream *>(rawStream));
> >>>>> -                     if (raw) {
> >>>>> -                             raw->copyFrom(buffer);
> >>>>> -                             pipe_->completeBuffer(camera_, request, raw);
> >>>>> -                     }
> >>>>> -             }
> >>>>> -
> >>>>> -             /* Simply requeue the buffer. */
> >>>>> -             stream->dev()->queueBuffer(buffer);
> >>>>> +             /* Simply re-queue the buffer to the requested stream. */
> >>>>> +             stream->queueBuffer(buffer);
> >>>>>       }
> >>>>>  }
> >>>>>
> >>>>> @@ -1367,7 +1381,7 @@ void RPiCameraData::tryRunPipeline()
> >>>>>        * current bayer buffer will be removed and re-queued to the driver.
> >>>>>        */
> >>>>>       embeddedBuffer = updateQueue(embeddedQueue_, bayerBuffer->metadata().timestamp,
> >>>>> -                                  unicam_[Unicam::Embedded].dev());
> >>>>> +                                  &unicam_[Unicam::Embedded]);
> >>>>>
> >>>>>       if (!embeddedBuffer) {
> >>>>>               LOG(RPI, Debug) << "Could not find matching embedded buffer";
> >>>>> @@ -1386,7 +1400,7 @@ void RPiCameraData::tryRunPipeline()
> >>>>>
> >>>>>               embeddedBuffer = embeddedQueue_.front();
> >>>>>               bayerBuffer = updateQueue(bayerQueue_, embeddedBuffer->metadata().timestamp,
> >>>>> -                                       unicam_[Unicam::Image].dev());
> >>>>> +                                       &unicam_[Unicam::Image]);
> >>>>>
> >>>>>               if (!bayerBuffer) {
> >>>>>                       LOG(RPI, Debug) << "Could not find matching bayer buffer - ending.";
> >>>>> @@ -1394,11 +1408,7 @@ void RPiCameraData::tryRunPipeline()
> >>>>>               }
> >>>>>       }
> >>>>>
> >>>>> -     /*
> >>>>> -      * Take the first request from the queue and action the IPA.
> >>>>> -      * Unicam buffers for the request have already been queued as they come
> >>>>> -      * in.
> >>>>> -      */
> >>>>> +     /* Take the first request from the queue and action the IPA. */
> >>>>>       Request *request = requestQueue_.front();
> >>>>>
> >>>>>       /*
> >>>>> @@ -1410,12 +1420,6 @@ void RPiCameraData::tryRunPipeline()
> >>>>>       op.controls = { request->controls() };
> >>>>>       ipa_->processEvent(op);
> >>>>>
> >>>>> -     /* Queue up any ISP buffers passed into the request. */
> >>>>> -     for (auto &stream : isp_) {
> >>>>> -             if (stream.isExternal())
> >>>>> -                     stream.dev()->queueBuffer(request->findBuffer(&stream));
> >>>>> -     }
> >>>>> -
> >>>>>       /* Ready to use the buffers, pop them off the queue. */
> >>>>>       bayerQueue_.pop();
> >>>>>       embeddedQueue_.pop();
> >>>>> @@ -1445,32 +1449,42 @@ void RPiCameraData::tryFlushQueues()
> >>>>>        * and give a chance for the hardware to return to lock-step. We do have
> >>>>>        * to drop all interim frames.
> >>>>>        */
> >>>>> -     if (unicam_[Unicam::Image].getBuffers()->size() == bayerQueue_.size() &&
> >>>>> -         unicam_[Unicam::Embedded].getBuffers()->size() == embeddedQueue_.size()) {
> >>>>> +     if (unicam_[Unicam::Image].getBuffers().size() == bayerQueue_.size() &&
> >>>>> +         unicam_[Unicam::Embedded].getBuffers().size() == embeddedQueue_.size()) {
> >>>>> +             /* This cannot happen when Unicam streams are external. */
> >>>>> +             assert(!unicam_[Unicam::Image].isExternal());
> >>>>> +
> >>>>>               LOG(RPI, Warning) << "Flushing all buffer queues!";
> >>>>>
> >>>>>               while (!bayerQueue_.empty()) {
> >>>>> -                     unicam_[Unicam::Image].dev()->queueBuffer(bayerQueue_.front());
> >>>>> +                     unicam_[Unicam::Image].queueBuffer(bayerQueue_.front());
> >>>>>                       bayerQueue_.pop();
> >>>>>               }
> >>>>>
> >>>>>               while (!embeddedQueue_.empty()) {
> >>>>> -                     unicam_[Unicam::Embedded].dev()->queueBuffer(embeddedQueue_.front());
> >>>>> +                     unicam_[Unicam::Embedded].queueBuffer(embeddedQueue_.front());
> >>>>>                       embeddedQueue_.pop();
> >>>>>               }
> >>>>>       }
> >>>>>  }
> >>>>>
> >>>>>  FrameBuffer *RPiCameraData::updateQueue(std::queue<FrameBuffer *> &q, uint64_t timestamp,
> >>>>> -                                     V4L2VideoDevice *dev)
> >>>>> +                                     RPi::RPiStream *stream)
> >>>>>  {
> >>>>> +     /*
> >>>>> +      * If the unicam streams are external (both have to the same), then we
> >>>>
> >>>> both have to 'be' the same?
> >>>>
> >>>>> +      * can only return out the top buffer in the queue, and assume they have
> >>>>> +      * been synced by queuing at the same time. We cannot drop these frames,
> >>>>> +      * as they may have been provided externally.
> >>>>> +      */
> >>>>>       while (!q.empty()) {
> >>>>>               FrameBuffer *b = q.front();
> >>>>> -             if (b->metadata().timestamp < timestamp) {
> >>>>> +             if (!stream->isExternal() && b->metadata().timestamp < timestamp) {
> >>>>>                       q.pop();
> >>>>> -                     dev->queueBuffer(b);
> >>>>> -                     LOG(RPI, Warning) << "Dropping input frame!";
> >>>>> -             } else if (b->metadata().timestamp == timestamp) {
> >>>>> +                     stream->queueBuffer(b);
> >>>>> +                     LOG(RPI, Warning) << "Dropping unmatched input frame in stream "
> >>>>> +                                       << stream->name();
> >>>>> +             } else if (stream->isExternal() || b->metadata().timestamp == timestamp) {
> >>>>>                       /* The calling function will pop the item from the queue. */
> >>>>>                       return b;
> >>>>>               } else {
> >>>>> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> >>>>> index 2edb8b59..02f8d3e0 100644
> >>>>> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> >>>>> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> >>>>> @@ -21,30 +21,20 @@ V4L2VideoDevice *RPiStream::dev() const
> >>>>>
> >>>>>  void RPiStream::setExternal(bool external)
> >>>>>  {
> >>>>> +     /* Import streams cannot be external. */
> >>>>> +     assert(!external || !importOnly_);
> >>>>>       external_ = external;
> >>>>>  }
> >>>>>
> >>>>>  bool RPiStream::isExternal() const
> >>>>>  {
> >>>>> -     /*
> >>>>> -      * Import streams cannot be external.
> >>>>> -      *
> >>>>> -      * RAW capture is a special case where we simply copy the RAW
> >>>>> -      * buffer out of the request. All other buffer handling happens
> >>>>> -      * as if the stream is internal.
> >>>>> -      */
> >>>>> -     return external_ && !importOnly_;
> >>>>> -}
> >>>>> -
> >>>>> -bool RPiStream::isImporter() const
> >>>>> -{
> >>>>> -     return importOnly_;
> >>>>> +     return external_;
> >>>>>  }
> >>>>>
> >>>>>  void RPiStream::reset()
> >>>>>  {
> >>>>>       external_ = false;
> >>>>> -     internalBuffers_.clear();
> >>>>> +     clearBuffers();
> >>>>>  }
> >>>>>
> >>>>>  std::string RPiStream::name() const
> >>>>> @@ -52,65 +42,123 @@ std::string RPiStream::name() const
> >>>>>       return name_;
> >>>>>  }
> >>>>>
> >>>>> -void RPiStream::setExternalBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> >>>>> +void RPiStream::setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> >>>>>  {
> >>>>> -     externalBuffers_ = buffers;
> >>>>> +     std::transform(buffers->begin(), buffers->end(), std::back_inserter(bufferList_),
> >>>>> +                    [](std::unique_ptr<FrameBuffer> &b) { return b.get(); });
> >>>>>  }
> >>>>>
> >>>>> -const std::vector<std::unique_ptr<FrameBuffer>> *RPiStream::getBuffers() const
> >>>>> +const std::vector<FrameBuffer *> &RPiStream::getBuffers() const
> >>>>>  {
> >>>>> -     return external_ ? externalBuffers_ : &internalBuffers_;
> >>>>> +     return bufferList_;
> >>>>>  }
> >>>>>
> >>>>>  void RPiStream::releaseBuffers()
> >>>>>  {
> >>>>>       dev_->releaseBuffers();
> >>>>> -     if (!external_ && !importOnly_)
> >>>>> -             internalBuffers_.clear();
> >>>>> +     clearBuffers();
> >>>>>  }
> >>>>>
> >>>>> -int RPiStream::importBuffers(unsigned int count)
> >>>>> +int RPiStream::prepareBuffers(unsigned int count)
> >>>>>  {
> >>>>> +     int ret;
> >>>>> +
> >>>>> +     if (!importOnly_) {
> >>>>> +             if (count) {
> >>>>> +                     /* Export some frame buffers for internal use. */
> >>>>> +                     ret = dev_->exportBuffers(count, &internalBuffers_);
> >>>>> +                     if (ret < 0)
> >>>>> +                             return ret;
> >>>>> +
> >>>>> +                     /* Add these exported buffers to the internal/external buffer list. */
> >>>>> +                     setExportedBuffers(&internalBuffers_);
> >>>>> +
> >>>>> +                     /* Add these buffers to the queue of internal usable buffers. */
> >>>>> +                     for (auto const &buffer : internalBuffers_)
> >>>>> +                             availableBuffers_.push(buffer.get());
> >>>>> +             }
> >>>>> +
> >>>>> +             /* We must import all internal/external exported buffers. */
> >>>>> +             count = bufferList_.size();
> >>>>> +     }
> >>>>> +
> >>>>>       return dev_->importBuffers(count);
> >>>>>  }
> >>>>>
> >>>>> -int RPiStream::allocateBuffers(unsigned int count)
> >>>>> +int RPiStream::queueAllBuffers()
> >>>>>  {
> >>>>> -     return dev_->allocateBuffers(count, &internalBuffers_);
> >>>>> -}
> >>>>> +     int ret;
> >>>>>
> >>>>> -int RPiStream::queueBuffers()
> >>>>> -{
> >>>>>       if (external_)
> >>>>>               return 0;
> >>>>>
> >>>>> -     for (auto &b : internalBuffers_) {
> >>>>> -             int ret = dev_->queueBuffer(b.get());
> >>>>> -             if (ret) {
> >>>>> -                     LOG(RPISTREAM, Error) << "Failed to queue buffers for "
> >>>>> -                                           << name_;
> >>>>> +     while (!availableBuffers_.empty()) {
> >>>>> +             ret = queueBuffer(availableBuffers_.front());
> >>>>> +             if (ret < 0)
> >>>>>                       return ret;
> >>>>> -             }
> >>>>> +
> >>>>> +             availableBuffers_.pop();
> >>>>>       }
> >>>>>
> >>>>>       return 0;
> >>>>>  }
> >>>>>
> >>>>> -bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const
> >>>>> +int RPiStream::queueBuffer(FrameBuffer *buffer)
> >>>>> +{
> >>>>> +     /*
> >>>>> +      * A nullptr buffer implies an external stream, but no external
> >>>>> +      * buffer has been supplied. So, pick one from the availableBuffers_
> >>>>> +      * queue.
> >>
> >> Aha, now I see this here, maybe the comment addition I suggested above
> >> is possibly redundant, as it's explained here ...
> >>
> >>>>> +      */
> >>>>> +     if (!buffer) {
> >>>>> +             if (availableBuffers_.empty()) {
> >>>>> +                     LOG(RPISTREAM, Warning) << "No buffers available for "
> >>>>> +                                             << name_;
> >>>>> +                     return -EINVAL;
> >>>>> +             }
> >>>>> +
> >>>>> +             buffer = availableBuffers_.front();
> >>>>> +             availableBuffers_.pop();
> >>>>> +     }
> >>>>> +
> >>>>> +     LOG(RPISTREAM, Debug) << "Queuing buffer " << buffer->cookie()
> >>>>> +                           << " for " << name_;
> >>>>> +
> >>>>> +     int ret = dev_->queueBuffer(buffer);
> >>>>> +     if (ret) {
> >>>>> +             LOG(RPISTREAM, Error) << "Failed to queue buffer for "
> >>>>> +                                   << name_;
> >>>>> +     }
> >>>>> +
> >>>>> +     return ret;
> >>>>> +}
> >>>>> +
> >>>>> +void RPiStream::returnBuffer(FrameBuffer *buffer)
> >>>>>  {
> >>>>> -     auto start = external_ ? externalBuffers_->begin() : internalBuffers_.begin();
> >>>>> -     auto end = external_ ? externalBuffers_->end() : internalBuffers_.end();
> >>>>> +     /* This can only be called for external streams. */
> >>>>> +     assert(external_);
> >>
> >> Could this use ASSERT() from libcamera/internal/log.h ?
> >>
> >>>>> +
> >>>>> +     availableBuffers_.push(buffer);
> >>>>> +}
> >>>>>
> >>>>> +bool RPiStream::findFrameBuffer(FrameBuffer *buffer) const
> >>>>> +{
> >>>>>       if (importOnly_)
> >>>>>               return false;
> >>>>>
> >>>>> -     if (std::find_if(start, end,
> >>>>> -                      [buffer](std::unique_ptr<FrameBuffer> const &ref) { return ref.get() == buffer; }) != end)
> >>>>> +     if (std::find(bufferList_.begin(), bufferList_.end(), buffer) != bufferList_.end())
> >>>>>               return true;
> >>>>>
> >>>>>       return false;
> >>>>>  }
> >>>>>
> >>>>> +void RPiStream::clearBuffers()
> >>>>> +{
> >>>>> +     availableBuffers_ = std::queue<FrameBuffer *>{};
> >>>>> +     internalBuffers_.clear();
> >>>>> +     bufferList_.clear();
> >>>>> +}
> >>>>> +
> >>>>>  } /* namespace RPi */
> >>>>>
> >>>>>  } /* namespace libcamera */
> >>>>> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> >>>>> index 3957e342..af9c2ad2 100644
> >>>>> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> >>>>> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> >>>>> @@ -39,21 +39,22 @@ public:
> >>>>>       V4L2VideoDevice *dev() const;
> >>>>>       void setExternal(bool external);
> >>>>>       bool isExternal() const;
> >>>>> -     bool isImporter() const;
> >>>>>       void reset();
> >>>>>       std::string name() const;
> >>>>> -     void setExternalBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> >>>>> -     const std::vector<std::unique_ptr<FrameBuffer>> *getBuffers() const;
> >>>>> +     void setExportedBuffers(std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> >>>>> +     const std::vector<FrameBuffer *> &getBuffers() const;
> >>>>>       void releaseBuffers();
> >>>>> -     int importBuffers(unsigned int count);
> >>>>> -     int allocateBuffers(unsigned int count);
> >>>>> -     int queueBuffers();
> >>>>> +     int prepareBuffers(unsigned int count);
> >>>>> +     int queueAllBuffers();
> >>>>> +     int queueBuffer(FrameBuffer *buffer);
> >>>>> +     void returnBuffer(FrameBuffer *buffer);
> >>>>>       bool findFrameBuffer(FrameBuffer *buffer) const;
> >>>>
> >>>> Perhaps we should break those up into sections in patch 1/9. It's hard
> >>>> to see the wood-for-the-trees in the wall of function declarations.
> >>>
> >>> Not sure I follow what you mean?
> >>
> >> There is a lot of text all bunched up together, so it's hard to get my
> >> eyes around the class members.
> >>
> >> It would be nice to group functions with some line breaks to make it
> >> easier on the eye.
> >>
> >> But that should happen in patch 1/9, so I'll go comment on it there. I
> >> skipped it before because that patch just moves code around, but I thnik
> >> it can also improve formatting for readability.
> >>
> >>>>>
> >>>>>  private:
> >>>>> +     void clearBuffers();
> >>>>>       /*
> >>>>>        * Indicates that this stream is active externally, i.e. the buffers
> >>>>> -      * are provided by the application.
> >>>>> +      * might be provided by (and returned to) the application.
> >>>>>        */
> >>>>>       bool external_;
> >>>>>       /* Indicates that this stream only imports buffers, e.g. ISP input. */
> >>>>> @@ -62,10 +63,19 @@ private:
> >>>>>       std::string name_;
> >>>>>       /* The actual device stream. */
> >>>>>       std::unique_ptr<V4L2VideoDevice> dev_;
> >>>>> -     /* Internally allocated framebuffers associated with this device stream. */
> >>>>> +     /* All framebuffers associated with this device stream. */
> >>>>> +     std::vector<FrameBuffer *> bufferList_;
> >>>>> +     /*
> >>>>> +      * List of frame buffer that we can use if none have been provided by
> >>>>> +      * the application for external streams. This is populated by the
> >>>>> +      * buffers exported internally.
> >>>>> +      */
> >>>>> +     std::queue<FrameBuffer *> availableBuffers_;
> >>>>> +     /*
> >>>>> +      * This is a list of buffers exported internally. Need to keep this around
> >>>>> +      * as the stream needs to maintain ownership of these buffers.
> >>>>> +      */
> >>>>>       std::vector<std::unique_ptr<FrameBuffer>> internalBuffers_;
> >>>>> -     /* Externally allocated framebuffers associated with this device stream. */
> >>>>> -     std::vector<std::unique_ptr<FrameBuffer>> *externalBuffers_;
> >>>>
> >>>> Likewise here, some whitespace to break up these members would really
> >>>> help, as by the time I've got here I've got definite review fatigue and
> >>>> lots of lines close together just become a blur ... :S
> >>>
> >>> That's not a problem.
> >>
> >> But for this patch, I can't see anything wrong ... and I can see how
> >> it's helping manage buffers to allow optional buffers on a stream for
> >> RAW zero copy support.
> >>
> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >>
> >>>>>  };
> >>>>>
> >>>>>  /*