[v2,09/35] pipeline: rkisp1: Use V4L2 requests for the dewarper
diff mbox series

Message ID 20251023144841.403689-10-stefan.klug@ideasonboard.com
State New
Headers show
Series
  • Full dewarper support on imx8mp
Related show

Commit Message

Stefan Klug Oct. 23, 2025, 2:48 p.m. UTC 
Use the V4L2 requests API if the dewarper supports it.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

---

Changes in v2:
- Replace strerrorname_np() by strerror() because the former is not
  supported on all our targets
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 59 ++++++++++++++++++++++--
 1 file changed, 56 insertions(+), 3 deletions(-)

Comments

Barnabás Pőcze Oct. 24, 2025, 2:32 p.m. UTC | #1 
Hi

2025. 10. 23. 16:48 keltezéssel, Stefan Klug írta:
> Use the V4L2 requests API if the dewarper supports it.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> 
> ---
> 
> Changes in v2:
> - Replace strerrorname_np() by strerror() because the former is not
>    supported on all our targets
> ---
>   src/libcamera/pipeline/rkisp1/rkisp1.cpp | 59 ++++++++++++++++++++++--
>   1 file changed, 56 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 0e2a210e1e80..dd3907e2fb5f 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -44,6 +44,7 @@
>   #include "libcamera/internal/media_device.h"
>   #include "libcamera/internal/media_pipeline.h"
>   #include "libcamera/internal/pipeline_handler.h"
> +#include "libcamera/internal/v4l2_request.h"
>   #include "libcamera/internal/v4l2_subdevice.h"
>   #include "libcamera/internal/v4l2_videodevice.h"
>   
> @@ -210,6 +211,7 @@ private:
>   	void imageBufferReady(FrameBuffer *buffer);
>   	void paramBufferReady(FrameBuffer *buffer);
>   	void statBufferReady(FrameBuffer *buffer);
> +	void dewarpRequestReady(V4L2Request *request);
>   	void dewarpBufferReady(FrameBuffer *buffer);
>   	void frameStart(uint32_t sequence);
>   
> @@ -239,6 +241,9 @@ private:
>   	std::vector<std::unique_ptr<FrameBuffer>> mainPathBuffers_;
>   	std::queue<FrameBuffer *> availableMainPathBuffers_;
>   
> +	std::vector<std::unique_ptr<V4L2Request>> dewarpRequests_;
> +	std::queue<V4L2Request *> availableDewarpRequests_;
> +
>   	std::vector<std::unique_ptr<FrameBuffer>> paramBuffers_;
>   	std::vector<std::unique_ptr<FrameBuffer>> statBuffers_;
>   	std::queue<FrameBuffer *> availableParamBuffers_;
> @@ -1034,6 +1039,18 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
>   
>   		for (std::unique_ptr<FrameBuffer> &buffer : mainPathBuffers_)
>   			availableMainPathBuffers_.push(buffer.get());
> +
> +		if (dewarper_->supportsRequests()) {
> +			ret = dewarper_->allocateRequests(kRkISP1MinBufferCount,
> +							  &dewarpRequests_);
> +			if (ret < 0)
> +				LOG(RkISP1, Error) << "Failed to allocate requests.";
> +		}
> +
> +		for (std::unique_ptr<V4L2Request> &request : dewarpRequests_) {
> +			request->requestDone.connect(this, &PipelineHandlerRkISP1::dewarpRequestReady);
> +			availableDewarpRequests_.push(request.get());
> +		}

I think `errorCleanup` should be updated with the following:

   availableDewarpRequests_.clear();
   dewarpRequests_.clear();


>   	}
>   
>   	auto pushBuffers = [&](const std::vector<std::unique_ptr<FrameBuffer>> &buffers,
> @@ -1075,6 +1092,11 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)
>   	statBuffers_.clear();
>   	mainPathBuffers_.clear();
>   
> +	while (!availableDewarpRequests_.empty())
> +		availableDewarpRequests_.pop();

`std::queue` does not have `clear()`... maybe we should use `std::dequeue`
(which is the default container of `std::queue` and it has `clear()`).


> +
> +	dewarpRequests_.clear();
> +
>   	std::vector<unsigned int> ids;
>   	for (IPABuffer &ipabuf : data->ipaBuffers_)
>   		ids.push_back(ipabuf.id);
> @@ -1560,6 +1582,14 @@ void PipelineHandlerRkISP1::imageBufferReady(FrameBuffer *buffer)
>   		return;
>   	}
>   
> +	V4L2Request *dewarpRequest = nullptr;
> +	if (!dewarpRequests_.empty()) {
> +		/* If we have requests support, there must be one available */
> +		ASSERT(!availableDewarpRequests_.empty());
> +		dewarpRequest = availableDewarpRequests_.front();
> +		availableDewarpRequests_.pop();
> +	}
> +
>   	/* Handle scaler crop control. */
>   	const auto &crop = request->controls().get(controls::ScalerCrop);
>   	if (crop) {
> @@ -1597,14 +1627,37 @@ void PipelineHandlerRkISP1::imageBufferReady(FrameBuffer *buffer)
>   	 * buffers for the dewarper are the buffers of the request, supplied
>   	 * by the application.
>   	 */
> -	int ret = dewarper_->queueBuffers(buffer, request->buffers());
> -	if (ret < 0)
> -		LOG(RkISP1, Error) << "Cannot queue buffers to dewarper: "
> +	int ret = dewarper_->queueBuffers(buffer, request->buffers(), dewarpRequest);
> +	if (ret < 0) {
> +		LOG(RkISP1, Error) << "Failed to queue buffers to dewarper: -"
>   				   << strerror(-ret);
>   
> +		/* Push it back into the queue. */
> +		if (dewarpRequest)
> +			dewarpRequestReady(dewarpRequest);
> +
> +		return;
> +	}
> +
> +	if (dewarpRequest) {
> +		ret = dewarpRequest->queue();
> +		if (ret < 0) {
> +			LOG(RkISP1, Error) << "Failed to queue dewarp request: -"
> +					   << strerror(-ret);
> +			/* Push it back into the queue. */
> +			dewarpRequestReady(dewarpRequest);
> +		}
> +	}

I feel the error handling is a bit on the more complicated side, i.e. easy to miss things.
But I don't have a simple idea on how it could be improved.


Regards,
Barnabás Pőcze


> [...]
Paul Elder Nov. 5, 2025, 4:46 p.m. UTC | #2 
Quoting Stefan Klug (2025-10-23 23:48:10)
> Use the V4L2 requests API if the dewarper supports it.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> 
> ---
> 
> Changes in v2:
> - Replace strerrorname_np() by strerror() because the former is not
>   supported on all our targets
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 59 ++++++++++++++++++++++--
>  1 file changed, 56 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 0e2a210e1e80..dd3907e2fb5f 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -44,6 +44,7 @@
>  #include "libcamera/internal/media_device.h"
>  #include "libcamera/internal/media_pipeline.h"
>  #include "libcamera/internal/pipeline_handler.h"
> +#include "libcamera/internal/v4l2_request.h"
>  #include "libcamera/internal/v4l2_subdevice.h"
>  #include "libcamera/internal/v4l2_videodevice.h"
>  
> @@ -210,6 +211,7 @@ private:
>         void imageBufferReady(FrameBuffer *buffer);
>         void paramBufferReady(FrameBuffer *buffer);
>         void statBufferReady(FrameBuffer *buffer);
> +       void dewarpRequestReady(V4L2Request *request);
>         void dewarpBufferReady(FrameBuffer *buffer);
>         void frameStart(uint32_t sequence);
>  
> @@ -239,6 +241,9 @@ private:
>         std::vector<std::unique_ptr<FrameBuffer>> mainPathBuffers_;
>         std::queue<FrameBuffer *> availableMainPathBuffers_;
>  
> +       std::vector<std::unique_ptr<V4L2Request>> dewarpRequests_;
> +       std::queue<V4L2Request *> availableDewarpRequests_;
> +
>         std::vector<std::unique_ptr<FrameBuffer>> paramBuffers_;
>         std::vector<std::unique_ptr<FrameBuffer>> statBuffers_;
>         std::queue<FrameBuffer *> availableParamBuffers_;
> @@ -1034,6 +1039,18 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
>  
>                 for (std::unique_ptr<FrameBuffer> &buffer : mainPathBuffers_)
>                         availableMainPathBuffers_.push(buffer.get());
> +
> +               if (dewarper_->supportsRequests()) {
> +                       ret = dewarper_->allocateRequests(kRkISP1MinBufferCount,
> +                                                         &dewarpRequests_);
> +                       if (ret < 0)
> +                               LOG(RkISP1, Error) << "Failed to allocate requests.";
> +               }
> +
> +               for (std::unique_ptr<V4L2Request> &request : dewarpRequests_) {
> +                       request->requestDone.connect(this, &PipelineHandlerRkISP1::dewarpRequestReady);
> +                       availableDewarpRequests_.push(request.get());
> +               }
>         }
>  
>         auto pushBuffers = [&](const std::vector<std::unique_ptr<FrameBuffer>> &buffers,
> @@ -1075,6 +1092,11 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)
>         statBuffers_.clear();
>         mainPathBuffers_.clear();
>  
> +       while (!availableDewarpRequests_.empty())
> +               availableDewarpRequests_.pop();
> +
> +       dewarpRequests_.clear();
> +
>         std::vector<unsigned int> ids;
>         for (IPABuffer &ipabuf : data->ipaBuffers_)
>                 ids.push_back(ipabuf.id);
> @@ -1560,6 +1582,14 @@ void PipelineHandlerRkISP1::imageBufferReady(FrameBuffer *buffer)
>                 return;
>         }
>  
> +       V4L2Request *dewarpRequest = nullptr;
> +       if (!dewarpRequests_.empty()) {

To me it feels more correct to check availableDewarpRequests_ since that's what
we're dequeueing from, or supportsRequests() if you want to check capability.

> +               /* If we have requests support, there must be one available */
> +               ASSERT(!availableDewarpRequests_.empty());
> +               dewarpRequest = availableDewarpRequests_.front();
> +               availableDewarpRequests_.pop();
> +       }
> +
>         /* Handle scaler crop control. */
>         const auto &crop = request->controls().get(controls::ScalerCrop);
>         if (crop) {
> @@ -1597,14 +1627,37 @@ void PipelineHandlerRkISP1::imageBufferReady(FrameBuffer *buffer)
>          * buffers for the dewarper are the buffers of the request, supplied
>          * by the application.
>          */
> -       int ret = dewarper_->queueBuffers(buffer, request->buffers());
> -       if (ret < 0)
> -               LOG(RkISP1, Error) << "Cannot queue buffers to dewarper: "
> +       int ret = dewarper_->queueBuffers(buffer, request->buffers(), dewarpRequest);
> +       if (ret < 0) {
> +               LOG(RkISP1, Error) << "Failed to queue buffers to dewarper: -"
>                                    << strerror(-ret);
>  
> +               /* Push it back into the queue. */
> +               if (dewarpRequest)
> +                       dewarpRequestReady(dewarpRequest);
> +
> +               return;
> +       }
> +
> +       if (dewarpRequest) {
> +               ret = dewarpRequest->queue();

Is it possible for this to fail while queueBuffers() succeeds? Maybe ENOMEM if
not EIO? Should we assert or warn?


Thanks,

Paul

> +               if (ret < 0) {
> +                       LOG(RkISP1, Error) << "Failed to queue dewarp request: -"
> +                                          << strerror(-ret);
> +                       /* Push it back into the queue. */
> +                       dewarpRequestReady(dewarpRequest);
> +               }
> +       }
> +
>         request->metadata().set(controls::ScalerCrop, activeCrop_.value());
>  }
>  
> +void PipelineHandlerRkISP1::dewarpRequestReady(V4L2Request *request)
> +{
> +       request->reinit();
> +       availableDewarpRequests_.push(request);
> +}
> +
>  void PipelineHandlerRkISP1::dewarpBufferReady(FrameBuffer *buffer)
>  {
>         ASSERT(activeCamera_);
> -- 
> 2.48.1
>

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 0e2a210e1e80..dd3907e2fb5f 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -44,6 +44,7 @@ 
 #include "libcamera/internal/media_device.h"
 #include "libcamera/internal/media_pipeline.h"
 #include "libcamera/internal/pipeline_handler.h"
+#include "libcamera/internal/v4l2_request.h"
 #include "libcamera/internal/v4l2_subdevice.h"
 #include "libcamera/internal/v4l2_videodevice.h"
 
@@ -210,6 +211,7 @@  private:
 	void imageBufferReady(FrameBuffer *buffer);
 	void paramBufferReady(FrameBuffer *buffer);
 	void statBufferReady(FrameBuffer *buffer);
+	void dewarpRequestReady(V4L2Request *request);
 	void dewarpBufferReady(FrameBuffer *buffer);
 	void frameStart(uint32_t sequence);
 
@@ -239,6 +241,9 @@  private:
 	std::vector<std::unique_ptr<FrameBuffer>> mainPathBuffers_;
 	std::queue<FrameBuffer *> availableMainPathBuffers_;
 
+	std::vector<std::unique_ptr<V4L2Request>> dewarpRequests_;
+	std::queue<V4L2Request *> availableDewarpRequests_;
+
 	std::vector<std::unique_ptr<FrameBuffer>> paramBuffers_;
 	std::vector<std::unique_ptr<FrameBuffer>> statBuffers_;
 	std::queue<FrameBuffer *> availableParamBuffers_;
@@ -1034,6 +1039,18 @@  int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
 
 		for (std::unique_ptr<FrameBuffer> &buffer : mainPathBuffers_)
 			availableMainPathBuffers_.push(buffer.get());
+
+		if (dewarper_->supportsRequests()) {
+			ret = dewarper_->allocateRequests(kRkISP1MinBufferCount,
+							  &dewarpRequests_);
+			if (ret < 0)
+				LOG(RkISP1, Error) << "Failed to allocate requests.";
+		}
+
+		for (std::unique_ptr<V4L2Request> &request : dewarpRequests_) {
+			request->requestDone.connect(this, &PipelineHandlerRkISP1::dewarpRequestReady);
+			availableDewarpRequests_.push(request.get());
+		}
 	}
 
 	auto pushBuffers = [&](const std::vector<std::unique_ptr<FrameBuffer>> &buffers,
@@ -1075,6 +1092,11 @@  int PipelineHandlerRkISP1::freeBuffers(Camera *camera)
 	statBuffers_.clear();
 	mainPathBuffers_.clear();
 
+	while (!availableDewarpRequests_.empty())
+		availableDewarpRequests_.pop();
+
+	dewarpRequests_.clear();
+
 	std::vector<unsigned int> ids;
 	for (IPABuffer &ipabuf : data->ipaBuffers_)
 		ids.push_back(ipabuf.id);
@@ -1560,6 +1582,14 @@  void PipelineHandlerRkISP1::imageBufferReady(FrameBuffer *buffer)
 		return;
 	}
 
+	V4L2Request *dewarpRequest = nullptr;
+	if (!dewarpRequests_.empty()) {
+		/* If we have requests support, there must be one available */
+		ASSERT(!availableDewarpRequests_.empty());
+		dewarpRequest = availableDewarpRequests_.front();
+		availableDewarpRequests_.pop();
+	}
+
 	/* Handle scaler crop control. */
 	const auto &crop = request->controls().get(controls::ScalerCrop);
 	if (crop) {
@@ -1597,14 +1627,37 @@  void PipelineHandlerRkISP1::imageBufferReady(FrameBuffer *buffer)
 	 * buffers for the dewarper are the buffers of the request, supplied
 	 * by the application.
 	 */
-	int ret = dewarper_->queueBuffers(buffer, request->buffers());
-	if (ret < 0)
-		LOG(RkISP1, Error) << "Cannot queue buffers to dewarper: "
+	int ret = dewarper_->queueBuffers(buffer, request->buffers(), dewarpRequest);
+	if (ret < 0) {
+		LOG(RkISP1, Error) << "Failed to queue buffers to dewarper: -"
 				   << strerror(-ret);
 
+		/* Push it back into the queue. */
+		if (dewarpRequest)
+			dewarpRequestReady(dewarpRequest);
+
+		return;
+	}
+
+	if (dewarpRequest) {
+		ret = dewarpRequest->queue();
+		if (ret < 0) {
+			LOG(RkISP1, Error) << "Failed to queue dewarp request: -"
+					   << strerror(-ret);
+			/* Push it back into the queue. */
+			dewarpRequestReady(dewarpRequest);
+		}
+	}
+
 	request->metadata().set(controls::ScalerCrop, activeCrop_.value());
 }
 
+void PipelineHandlerRkISP1::dewarpRequestReady(V4L2Request *request)
+{
+	request->reinit();
+	availableDewarpRequests_.push(request);
+}
+
 void PipelineHandlerRkISP1::dewarpBufferReady(FrameBuffer *buffer)
 {
 	ASSERT(activeCamera_);