[libcamera-devel] [PATCH 4/5] android: camera_worker: Notify fence wait failures
Hirokazu Honda
hiroh at chromium.org
Mon Sep 6 16:55:15 CEST 2021
Hi Jacopo, thank you for the patch.
On Mon, Sep 6, 2021 at 11:01 PM Jacopo Mondi <jacopo at jmondi.org> 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)
If I understand correctly, this must not happen because
requestQueueFailed() is invoked by worker::stop() and it is executed
before descriptors_.clear() always.
Correct?
Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
-Hiro
> + << "Unknown request: " << request->cookie();
> + return;
> + }
> +
> + descriptors_.extract(it);
> +
> + abortRequest(request->camera3Request());
> +}
> +
> 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);
> }
>
> /*
> @@ -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);
> --
> 2.32.0
>
More information about the libcamera-devel
mailing list