[v5,2/3] libcamera: simple: Track requests in conversionQueue_
diff mbox series

Message ID 20241021133718.894374-3-mzamazal@redhat.com
State Accepted
Headers show
Series
  • Clean up pending requests on stop in software ISP
Related show

Commit Message

Milan Zamazal Oct. 21, 2024, 1:37 p.m. UTC 
Simple pipeline retrieves the requests to complete from the
conversionQueue_.  This patch stores the requests in conversionQueue_
explicitly.  This explicit tracking is supposed to be preferred to
implicit retrieval and it simplifies the completion code a bit here and
in the followup patch that adds request cleanup on stop.

The change as implemented assumes that all the buffers in each of the
conversionQueue_ elements point to the same request, the one specified.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/libcamera/pipeline/simple/simple.cpp | 26 ++++++++++++------------
 1 file changed, 13 insertions(+), 13 deletions(-)

Comments

Hans de Goede Nov. 3, 2024, 1:22 p.m. UTC | #1 
Hi,

On 21-Oct-24 3:37 PM, Milan Zamazal wrote:
> Simple pipeline retrieves the requests to complete from the
> conversionQueue_.  This patch stores the requests in conversionQueue_
> explicitly.  This explicit tracking is supposed to be preferred to
> implicit retrieval and it simplifies the completion code a bit here and
> in the followup patch that adds request cleanup on stop.
> 
> The change as implemented assumes that all the buffers in each of the
> conversionQueue_ elements point to the same request, the one specified.
> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans




> ---
>  src/libcamera/pipeline/simple/simple.cpp | 26 ++++++++++++------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 3ddce71d3..f3a6b1635 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -282,7 +282,11 @@ public:
>  	std::unique_ptr<DelayedControls> delayedCtrls_;
>  
>  	std::vector<std::unique_ptr<FrameBuffer>> conversionBuffers_;
> -	std::queue<std::map<const Stream *, FrameBuffer *>> conversionQueue_;
> +	struct RequestOutputs {
> +		Request *request;
> +		std::map<const Stream *, FrameBuffer *> outputs;
> +	};
> +	std::queue<RequestOutputs> conversionQueue_;
>  	bool useConversion_;
>  
>  	std::unique_ptr<Converter> converter_;
> @@ -813,16 +817,12 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)
>  		if (conversionQueue_.empty())
>  			return;
>  
> -		Request *request = nullptr;
> -		for (auto &item : conversionQueue_.front()) {
> -			FrameBuffer *outputBuffer = item.second;
> -			request = outputBuffer->request();
> -			pipe->completeBuffer(request, outputBuffer);
> -		}
> +		Request *request = conversionQueue_.front().request;
> +		for (auto &item : conversionQueue_.front().outputs)
> +			pipe->completeBuffer(request, item.second);
> +		pipe->completeRequest(request);
>  		conversionQueue_.pop();
>  
> -		if (request)
> -			pipe->completeRequest(request);
>  		return;
>  	}
>  
> @@ -838,7 +838,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)
>  
>  	if (useConversion_ && !conversionQueue_.empty()) {
>  		const std::map<const Stream *, FrameBuffer *> &outputs =
> -			conversionQueue_.front();
> +			conversionQueue_.front().outputs;
>  		if (!outputs.empty()) {
>  			FrameBuffer *outputBuffer = outputs.begin()->second;
>  			if (outputBuffer)
> @@ -862,7 +862,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)
>  		}
>  
>  		if (converter_)
> -			converter_->queueBuffers(buffer, conversionQueue_.front());
> +			converter_->queueBuffers(buffer, conversionQueue_.front().outputs);
>  		else
>  			/*
>  			 * request->sequence() cannot be retrieved from `buffer' inside
> @@ -870,7 +870,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)
>  			 * already here.
>  			 */
>  			swIsp_->queueBuffers(request->sequence(), buffer,
> -					     conversionQueue_.front());
> +					     conversionQueue_.front().outputs);
>  
>  		conversionQueue_.pop();
>  		return;
> @@ -1434,7 +1434,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)
>  	}
>  
>  	if (data->useConversion_) {
> -		data->conversionQueue_.push(std::move(buffers));
> +		data->conversionQueue_.push({ .request = request, .outputs = std::move(buffers) });
>  		if (data->swIsp_)
>  			data->swIsp_->queueRequest(request->sequence(), request->controls());
>  	}
Laurent Pinchart Nov. 6, 2024, 11:45 a.m. UTC | #2 
Hi Milan,

Thank you for the patch.

On Mon, Oct 21, 2024 at 03:37:17PM +0200, Milan Zamazal wrote:
> Simple pipeline retrieves the requests to complete from the
> conversionQueue_.  This patch stores the requests in conversionQueue_
> explicitly.  This explicit tracking is supposed to be preferred to
> implicit retrieval and it simplifies the completion code a bit here and
> in the followup patch that adds request cleanup on stop.
> 
> The change as implemented assumes that all the buffers in each of the
> conversionQueue_ elements point to the same request, the one specified.
> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/libcamera/pipeline/simple/simple.cpp | 26 ++++++++++++------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 3ddce71d3..f3a6b1635 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -282,7 +282,11 @@ public:
>  	std::unique_ptr<DelayedControls> delayedCtrls_;
>  
>  	std::vector<std::unique_ptr<FrameBuffer>> conversionBuffers_;
> -	std::queue<std::map<const Stream *, FrameBuffer *>> conversionQueue_;
> +	struct RequestOutputs {
> +		Request *request;
> +		std::map<const Stream *, FrameBuffer *> outputs;

I would have named this buffers, but I don't want to bikeshed on the
name.

> +	};
> +	std::queue<RequestOutputs> conversionQueue_;
>  	bool useConversion_;
>  
>  	std::unique_ptr<Converter> converter_;
> @@ -813,16 +817,12 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)
>  		if (conversionQueue_.empty())
>  			return;
>  
> -		Request *request = nullptr;
> -		for (auto &item : conversionQueue_.front()) {
> -			FrameBuffer *outputBuffer = item.second;
> -			request = outputBuffer->request();
> -			pipe->completeBuffer(request, outputBuffer);
> -		}
> +		Request *request = conversionQueue_.front().request;
> +		for (auto &item : conversionQueue_.front().outputs)
> +			pipe->completeBuffer(request, item.second);
> +		pipe->completeRequest(request);
>  		conversionQueue_.pop();

Would the following be clearer ? I always find .first and .second
confusing.

		const RequestOutputs &outputs = conversionQueue_.front();
		for (auto &[stream, buffer] : outputs.outputs)
			pipe->completeBuffer(outputs.request, buffer);
		pipe->completeRequest(outputs.request);
		conversionQueue_.pop();

>  
> -		if (request)
> -			pipe->completeRequest(request);
>  		return;
>  	}
>  
> @@ -838,7 +838,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)
>  
>  	if (useConversion_ && !conversionQueue_.empty()) {
>  		const std::map<const Stream *, FrameBuffer *> &outputs =
> -			conversionQueue_.front();
> +			conversionQueue_.front().outputs;
>  		if (!outputs.empty()) {
>  			FrameBuffer *outputBuffer = outputs.begin()->second;
>  			if (outputBuffer)
> @@ -862,7 +862,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)
>  		}
>  
>  		if (converter_)
> -			converter_->queueBuffers(buffer, conversionQueue_.front());
> +			converter_->queueBuffers(buffer, conversionQueue_.front().outputs);
>  		else
>  			/*
>  			 * request->sequence() cannot be retrieved from `buffer' inside
> @@ -870,7 +870,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)
>  			 * already here.
>  			 */
>  			swIsp_->queueBuffers(request->sequence(), buffer,
> -					     conversionQueue_.front());
> +					     conversionQueue_.front().outputs);
>  
>  		conversionQueue_.pop();
>  		return;
> @@ -1434,7 +1434,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)
>  	}
>  
>  	if (data->useConversion_) {
> -		data->conversionQueue_.push(std::move(buffers));
> +		data->conversionQueue_.push({ .request = request, .outputs = std::move(buffers) });

Designated initializers are a C++20 feature, you should use

		data->conversionQueue_.push({ request, std::move(buffers) });

With this,

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

>  		if (data->swIsp_)
>  			data->swIsp_->queueRequest(request->sequence(), request->controls());
>  	}

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index 3ddce71d3..f3a6b1635 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -282,7 +282,11 @@  public:
 	std::unique_ptr<DelayedControls> delayedCtrls_;
 
 	std::vector<std::unique_ptr<FrameBuffer>> conversionBuffers_;
-	std::queue<std::map<const Stream *, FrameBuffer *>> conversionQueue_;
+	struct RequestOutputs {
+		Request *request;
+		std::map<const Stream *, FrameBuffer *> outputs;
+	};
+	std::queue<RequestOutputs> conversionQueue_;
 	bool useConversion_;
 
 	std::unique_ptr<Converter> converter_;
@@ -813,16 +817,12 @@  void SimpleCameraData::bufferReady(FrameBuffer *buffer)
 		if (conversionQueue_.empty())
 			return;
 
-		Request *request = nullptr;
-		for (auto &item : conversionQueue_.front()) {
-			FrameBuffer *outputBuffer = item.second;
-			request = outputBuffer->request();
-			pipe->completeBuffer(request, outputBuffer);
-		}
+		Request *request = conversionQueue_.front().request;
+		for (auto &item : conversionQueue_.front().outputs)
+			pipe->completeBuffer(request, item.second);
+		pipe->completeRequest(request);
 		conversionQueue_.pop();
 
-		if (request)
-			pipe->completeRequest(request);
 		return;
 	}
 
@@ -838,7 +838,7 @@  void SimpleCameraData::bufferReady(FrameBuffer *buffer)
 
 	if (useConversion_ && !conversionQueue_.empty()) {
 		const std::map<const Stream *, FrameBuffer *> &outputs =
-			conversionQueue_.front();
+			conversionQueue_.front().outputs;
 		if (!outputs.empty()) {
 			FrameBuffer *outputBuffer = outputs.begin()->second;
 			if (outputBuffer)
@@ -862,7 +862,7 @@  void SimpleCameraData::bufferReady(FrameBuffer *buffer)
 		}
 
 		if (converter_)
-			converter_->queueBuffers(buffer, conversionQueue_.front());
+			converter_->queueBuffers(buffer, conversionQueue_.front().outputs);
 		else
 			/*
 			 * request->sequence() cannot be retrieved from `buffer' inside
@@ -870,7 +870,7 @@  void SimpleCameraData::bufferReady(FrameBuffer *buffer)
 			 * already here.
 			 */
 			swIsp_->queueBuffers(request->sequence(), buffer,
-					     conversionQueue_.front());
+					     conversionQueue_.front().outputs);
 
 		conversionQueue_.pop();
 		return;
@@ -1434,7 +1434,7 @@  int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)
 	}
 
 	if (data->useConversion_) {
-		data->conversionQueue_.push(std::move(buffers));
+		data->conversionQueue_.push({ .request = request, .outputs = std::move(buffers) });
 		if (data->swIsp_)
 			data->swIsp_->queueRequest(request->sequence(), request->controls());
 	}