[libcamera-devel] [PATCH 4/5] android: camera_worker: Notify fence wait failures

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Sep 06, 2021 at 04:01:51PM +0200, Jacopo Mondi wrote:
> The CameraDevice class stores one CaptureRequestDescriptor instance
> for each capture request queued to the Camera. Such descriptors are
> cleared when the Camera completes the Request in the
> CameraDevice::requestComplete() slot.
> 
> The CameraDevice class relies on an internal worker to off-load
> waiting on synchronization fences, and does unconditionally store
> the descriptor in the descriptors_ class member map to handle its
> lifetime.
> 
> If waiting on a fence fails, the created descriptor remains in the
> descriptors_ map until the next call to stop() or close(), as
> requestComplete() will never be called, and the camera
> service is never notified about the queueing failure.
> 
> Fix that by allowing the CameraWorker to notify to the CameraDevice if
> waiting on a fence has failed, by installing a new requestQueueFailed
> Signal.
> 
> The Signal is emitted by the CameraWorker internal worker if waiting on
> a fence has failed. The CameraDevice is augmented with a slot connected
> to the Signal that removes the descriptor from the map and notifies the
> camera framework about the failure.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/android/camera_device.cpp | 22 +++++++++++++++++++++-
>  src/android/camera_device.h   |  1 +
>  src/android/camera_worker.cpp | 10 ++++++----
>  src/android/camera_worker.h   |  9 +++++++--
>  4 files changed, 35 insertions(+), 7 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index a0ea138d9499..0ce9b699bfae 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -244,7 +244,7 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(
>  	 * Create the CaptureRequest, stored as a unique_ptr<> to tie its
>  	 * lifetime to the descriptor.
>  	 */
> -	request_ = std::make_unique<CaptureRequest>(camera);
> +	request_ = std::make_unique<CaptureRequest>(camera, camera3Request);
>  }
>  
>  /*
> @@ -264,6 +264,7 @@ CameraDevice::CameraDevice(unsigned int id, std::shared_ptr<Camera> camera)
>  	: id_(id), state_(State::Stopped), camera_(std::move(camera)),
>  	  facing_(CAMERA_FACING_FRONT), orientation_(0)
>  {
> +	worker_.requestQueueFailed.connect(this, &CameraDevice::requestQueueFailed);
>  	camera_->requestCompleted.connect(this, &CameraDevice::requestComplete);
>  
>  	maker_ = "libcamera";
> @@ -1060,6 +1061,25 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  	return 0;
>  }
>  
> +void CameraDevice::requestQueueFailed(CaptureRequest *request)
> +{
> +	/*
> +	 * Remove the descriptor from the map if the worker has failed
> +	 * to process it and notify the camera service about the failure.
> +	 */
> +	MutexLocker descriptorsLock(descriptorsMutex_);
> +	auto it = descriptors_.find(request->cookie());
> +	if (it == descriptors_.end()) {
> +		LOG(HAL, Fatal)
> +			<< "Unknown request: " << request->cookie();
> +		return;
> +	}
> +
> +	descriptors_.extract(it);
> +
> +	abortRequest(request->camera3Request());

I'd like to avoid adding the camera3_capture_request_t pointer to
CaptureRequest. Could we modify CameraDevice::abortRequest() to take a
Camera3RequestDescriptor (which we have in
CameraDevice::processCaptureRequest() where abortRequest() is called
today), and get it here from the descriptors_ map ?

I also wonder if we could somehow set the Request status to
RequestCancelled and call CameraDevice::requestComplete(). The rationale
is that we'll eventually move the fence handling to the libcamera core,
so the callback to the HAL will be requestComplete() then.

There's an additional problem, if a wait operation fails, you'll end up
signaling the request completion to the camera service out-of-order. I
wonder if it wouldn't be better to rebase this on Umang's ongoing work
that will include a pending request queue to address this issue.

> +}
> +
>  void CameraDevice::requestComplete(Request *request)
>  {
>  	decltype(descriptors_)::node_type node;
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 54c4cb9ab499..65456ecca7e3 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -83,6 +83,7 @@ private:
>  		std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_;
>  		CameraMetadata settings_;
>  		std::unique_ptr<CaptureRequest> request_;
> +		const camera3_capture_request_t *camera3Request_;
>  	};
>  
>  	enum class State {
> diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp
> index 98dddd9eb13b..7f39fab01638 100644
> --- a/src/android/camera_worker.cpp
> +++ b/src/android/camera_worker.cpp
> @@ -27,8 +27,9 @@ LOG_DECLARE_CATEGORY(HAL)
>   * by the CameraWorker which queues it to the libcamera::Camera after handling
>   * fences.
>   */
> -CaptureRequest::CaptureRequest(libcamera::Camera *camera)
> -	: camera_(camera)
> +CaptureRequest::CaptureRequest(libcamera::Camera *camera,
> +			       const camera3_capture_request_t *camera3Request)
> +	: camera_(camera), camera3Request_(camera3Request)
>  {
>  	request_ = camera_->createRequest(reinterpret_cast<uint64_t>(this));
>  }
> @@ -77,7 +78,7 @@ void CameraWorker::queueRequest(CaptureRequest *request)
>  {
>  	/* Async process the request on the worker which runs its own thread. */
>  	worker_.invokeMethod(&Worker::processRequest, ConnectionTypeQueued,
> -			     request);
> +			     this, request);

We could also pass the CameraWorker pointer to the CameraWorker::Worker
constructor and store it (not sure it would be better actually). Another
option is to store the Signal in CameraWorker::Worker, with a
CameraWorker::requestQueueFailed() accessor function that returns a
reference to the signal.

>  }
>  
>  /*
> @@ -109,7 +110,7 @@ int CameraWorker::Worker::waitFence(int fence)
>  	return -errno;
>  }
>  
> -void CameraWorker::Worker::processRequest(CaptureRequest *request)
> +void CameraWorker::Worker::processRequest(CameraWorker *context, CaptureRequest *request)
>  {
>  	/* Wait on all fences before queuing the Request. */
>  	for (int fence : request->fences()) {
> @@ -121,6 +122,7 @@ void CameraWorker::Worker::processRequest(CaptureRequest *request)
>  		if (ret < 0) {
>  			LOG(HAL, Error) << "Failed waiting for fence: "
>  					<< fence << ": " << strerror(-ret);
> +			context->requestQueueFailed.emit(request);
>  			return;
>  		}
>  	}
> diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h
> index 67ae50bd9288..86f20f741e54 100644
> --- a/src/android/camera_worker.h
> +++ b/src/android/camera_worker.h
> @@ -10,6 +10,7 @@
>  #include <memory>
>  
>  #include <libcamera/base/object.h>
> +#include <libcamera/base/signal.h>
>  #include <libcamera/base/thread.h>
>  
>  #include <libcamera/camera.h>
> @@ -22,7 +23,8 @@ class CameraDevice;
>  class CaptureRequest
>  {
>  public:
> -	CaptureRequest(libcamera::Camera *camera);
> +	CaptureRequest(libcamera::Camera *camera,
> +		       const camera3_capture_request_t *camera3Request);
>  
>  	const std::vector<int> &fences() const { return acquireFences_; }
>  	libcamera::ControlList &controls() { return request_->controls(); }
> @@ -31,6 +33,7 @@ public:
>  		return request_->metadata();
>  	}
>  	unsigned long cookie() const { return request_->cookie(); }
> +	const camera3_capture_request_t *camera3Request() const { return camera3Request_; }
>  
>  	void addBuffer(libcamera::Stream *stream,
>  		       libcamera::FrameBuffer *buffer, int fence);
> @@ -40,6 +43,7 @@ private:
>  	libcamera::Camera *camera_;
>  	std::vector<int> acquireFences_;
>  	std::unique_ptr<libcamera::Request> request_;
> +	const camera3_capture_request_t *camera3Request_;
>  };
>  
>  class CameraWorker : private libcamera::Thread
> @@ -51,6 +55,7 @@ public:
>  	void stop();
>  
>  	void queueRequest(CaptureRequest *request);
> +	libcamera::Signal<CaptureRequest *> requestQueueFailed;
>  
>  protected:
>  	void run() override;
> @@ -59,7 +64,7 @@ private:
>  	class Worker : public libcamera::Object
>  	{
>  	public:
> -		void processRequest(CaptureRequest *request);
> +		void processRequest(CameraWorker *context, CaptureRequest *request);
>  
>  	private:
>  		int waitFence(int fence);

-- 
Regards,

Laurent Pinchart