android: Remove CameraWorker

The CameraWorker class purpose was to handle acquire fences for incoming
capture requests directed to libcamera.

Now that fences are handled by the core library, it is not required to
handle them in the HAL and the CameraWorker and CaptureRequest classes
can be dropped.

Update the core in CameraDevice class accordingly to queue Requests
directly to the libcamera::Camera and set the release_fence to the
value of the FrameBuffer::fence() for streams of type ::Direct.

While at it make CameraRequest::StreamBuffer::fence a UniqueFD to ease
the management of the fences file descriptor values.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

8 files changed, 28 insertions, 236 deletions

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index 9f772b58..f28fdd6a 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -14,10 +14,12 @@
 #include <vector>
 
 #include <libcamera/base/log.h>
+#include <libcamera/base/unique_fd.h>
 #include <libcamera/base/utils.h>
 
 #include <libcamera/control_ids.h>
 #include <libcamera/controls.h>
+#include <libcamera/fence.h>
 #include <libcamera/formats.h>
 #include <libcamera/property_ids.h>
 
@@ -420,7 +422,6 @@ void CameraDevice::flush()
 		state_ = State::Flushing;
 	}
 
-	worker_.stop();
 	camera_->stop();
 
 	MutexLocker stateLock(stateMutex_);
@@ -433,7 +434,6 @@ void CameraDevice::stop()
 	if (state_ == State::Stopped)
 		return;
 
-	worker_.stop();
 	camera_->stop();
 
 	{
@@ -930,13 +930,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 
 		/*
 		 * Inspect the camera stream type, create buffers opportunely
-		 * and add them to the Request if required. Only acquire fences
-		 * for streams of type Direct are handled by the CameraWorker,
-		 * while fences for streams of type Internal and Mapped are
-		 * handled at post-processing time.
+		 * and add them to the Request if required.
 		 */
 		FrameBuffer *frameBuffer = nullptr;
-		int acquireFence = -1;
+		UniqueFD acquireFence;
 
 		MutexLocker lock(descriptor->streamsProcessMutex_);
 
@@ -964,7 +961,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 						  cameraStream->configuration().pixelFormat,
 						  cameraStream->configuration().size);
 			frameBuffer = buffer.frameBuffer.get();
-			acquireFence = buffer.fence;
+			acquireFence = std::move(buffer.fence);
 			LOG(HAL, Debug) << ss.str() << " (direct)";
 			break;
 
@@ -990,13 +987,14 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 			return -ENOMEM;
 		}
 
+		auto fence = std::make_unique<Fence>(std::move(acquireFence));
 		descriptor->request_->addBuffer(cameraStream->stream(),
-						frameBuffer, acquireFence);
+						frameBuffer, std::move(fence));
 	}
 
 	/*
 	 * Translate controls from Android to libcamera and queue the request
-	 * to the CameraWorker thread.
+	 * to the camera.
 	 */
 	int ret = processControls(descriptor.get());
 	if (ret)
@@ -1022,26 +1020,23 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 	}
 
 	if (state_ == State::Stopped) {
-		worker_.start();
-
 		ret = camera_->start();
 		if (ret) {
 			LOG(HAL, Error) << "Failed to start camera";
-			worker_.stop();
 			return ret;
 		}
 
 		state_ = State::Running;
 	}
 
-	CaptureRequest *request = descriptor->request_.get();
+	Request *request = descriptor->request_.get();
 
 	{
 		MutexLocker descriptorsLock(descriptorsMutex_);
 		descriptors_.push(std::move(descriptor));
 	}
 
-	worker_.queueRequest(request);
+	camera_->queueRequest(request);
 
 	return 0;
 }
@@ -1063,16 +1058,17 @@ void CameraDevice::requestComplete(Request *request)
 		/*
 		 * Streams of type Direct have been queued to the
 		 * libcamera::Camera and their acquire fences have
-		 * already been waited on by the CameraWorker.
+		 * already been waited on by the library.
 		 *
 		 * Acquire fences of streams of type Internal and Mapped
 		 * will be handled during post-processing.
-		 *
-		 * \todo Instrument the CameraWorker to set the acquire
-		 * fence to -1 once it has handled it and remove this check.
 		 */
-		if (stream->type() == CameraStream::Type::Direct)
-			buffer.fence = -1;
+		if (stream->type() == CameraStream::Type::Direct) {
+			/* If handling of the fence has failed restore buffer.fence. */
+			std::unique_ptr<Fence> fence = buffer.frameBuffer->releaseFence();
+			if (fence)
+				buffer.fence = fence->release();
+		}
 		buffer.status = Camera3RequestDescriptor::Status::Success;
 	}
 
@@ -1193,7 +1189,7 @@ void CameraDevice::sendCaptureResults()
 		std::vector<camera3_stream_buffer_t> resultBuffers;
 		resultBuffers.reserve(descriptor->buffers_.size());
 
-		for (const auto &buffer : descriptor->buffers_) {
+		for (auto &buffer : descriptor->buffers_) {
 			camera3_buffer_status status = CAMERA3_BUFFER_STATUS_ERROR;
 
 			if (buffer.status == Camera3RequestDescriptor::Status::Success)
@@ -1207,7 +1203,7 @@ void CameraDevice::sendCaptureResults()
 			 */
 			resultBuffers.push_back({ buffer.stream->camera3Stream(),
 						  buffer.camera3Buffer, status,
-						  -1, buffer.fence });
+						  -1, buffer.fence.release() });
 		}
 
 		captureResult.num_output_buffers = resultBuffers.size();
diff --git a/src/android/camera_device.h b/src/android/camera_device.h
index e5d9cda2..64050416 100644
--- a/src/android/camera_device.h
+++ b/src/android/camera_device.h
@@ -29,7 +29,6 @@
 #include "camera_capabilities.h"
 #include "camera_metadata.h"
 #include "camera_stream.h"
-#include "camera_worker.h"
 #include "jpeg/encoder.h"
 
 class Camera3RequestDescriptor;
@@ -105,8 +104,6 @@ private:
 	unsigned int id_;
 	camera3_device_t camera3Device_;
 
-	CameraWorker worker_;
-
 	libcamera::Mutex stateMutex_; /* Protects access to the camera state. */
 	State state_ LIBCAMERA_TSA_GUARDED_BY(stateMutex_);
 
diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp
index 8162aa78..91776907 100644
--- a/src/android/camera_request.cpp
+++ b/src/android/camera_request.cpp
@@ -47,8 +47,7 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(
 	 * Create the CaptureRequest, stored as a unique_ptr<> to tie its
 	 * lifetime to the descriptor.
 	 */
-	request_ = std::make_unique<CaptureRequest>(camera,
-						    reinterpret_cast<uint64_t>(this));
+	request_ = camera->createRequest(reinterpret_cast<uint64_t>(this));
 }
 
 Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;
diff --git a/src/android/camera_request.h b/src/android/camera_request.h
index d9b04f16..37b6ae32 100644
--- a/src/android/camera_request.h
+++ b/src/android/camera_request.h
@@ -13,6 +13,7 @@
 
 #include <libcamera/base/class.h>
 #include <libcamera/base/mutex.h>
+#include <libcamera/base/unique_fd.h>
 
 #include <libcamera/camera.h>
 #include <libcamera/framebuffer.h>
@@ -20,7 +21,6 @@
 #include <hardware/camera3.h>
 
 #include "camera_metadata.h"
-#include "camera_worker.h"
 
 class CameraBuffer;
 class CameraStream;
@@ -45,7 +45,7 @@ public:
 		CameraStream *stream;
 		buffer_handle_t *camera3Buffer;
 		std::unique_ptr<libcamera::FrameBuffer> frameBuffer;
-		int fence;
+		libcamera::UniqueFD fence;
 		Status status = Status::Success;
 		libcamera::FrameBuffer *internalBuffer = nullptr;
 		const libcamera::FrameBuffer *srcBuffer = nullptr;
@@ -72,7 +72,7 @@ public:
 	std::vector<StreamBuffer> buffers_;
 
 	CameraMetadata settings_;
-	std::unique_ptr<CaptureRequest> request_;
+	std::unique_ptr<libcamera::Request> request_;
 	std::unique_ptr<CameraMetadata> resultMetadata_;
 
 	bool complete_ = false;
diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
index ee9bbe48..c2157450 100644
--- a/src/android/camera_stream.cpp
+++ b/src/android/camera_stream.cpp
@@ -170,16 +170,16 @@ int CameraStream::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)
 	ASSERT(type_ != Type::Direct);
 
 	/* Handle waiting on fences on the destination buffer. */
-	if (streamBuffer->fence != -1) {
-		int ret = waitFence(streamBuffer->fence);
+	if (streamBuffer->fence.isValid()) {
+		int ret = waitFence(streamBuffer->fence.get());
 		if (ret < 0) {
 			LOG(HAL, Error) << "Failed waiting for fence: "
-					<< streamBuffer->fence << ": " << strerror(-ret);
+					<< streamBuffer->fence.get() << ": "
+					<< strerror(-ret);
 			return ret;
 		}
 
-		::close(streamBuffer->fence);
-		streamBuffer->fence = -1;
+		streamBuffer->fence.reset();
 	}
 
 	const StreamConfiguration &output = configuration();
diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp
deleted file mode 100644
index dabb3054..00000000
--- a/src/android/camera_worker.cpp
+++ /dev/null
@@ -1,129 +0,0 @@
-/* SPDX-License-Identifier: LGPL-2.1-or-later */
-/*
- * Copyright (C) 2020, Google Inc.
- *
- * camera_worker.cpp - Process capture requests on behalf of the Camera HAL
- */
-
-#include "camera_worker.h"
-
-#include <errno.h>
-#include <string.h>
-#include <sys/poll.h>
-#include <unistd.h>
-
-#include "camera_device.h"
-
-using namespace libcamera;
-
-LOG_DECLARE_CATEGORY(HAL)
-
-/*
- * \class CaptureRequest
- * \brief Wrap a libcamera::Request associated with buffers and fences
- *
- * A CaptureRequest is constructed by the CameraDevice, filled with
- * buffers and fences provided by the camera3 framework and then processed
- * by the CameraWorker which queues it to the libcamera::Camera after handling
- * fences.
- */
-CaptureRequest::CaptureRequest(Camera *camera, uint64_t cookie)
-	: camera_(camera)
-{
-	request_ = camera_->createRequest(cookie);
-}
-
-void CaptureRequest::addBuffer(Stream *stream, FrameBuffer *buffer, int fence)
-{
-	request_->addBuffer(stream, buffer);
-	acquireFences_.push_back(fence);
-}
-
-void CaptureRequest::queue()
-{
-	camera_->queueRequest(request_.get());
-}
-
-/*
- * \class CameraWorker
- * \brief Process a CaptureRequest on an internal thread
- *
- * The CameraWorker class wraps a Worker that runs on an internal thread
- * and schedules processing of CaptureRequest through it.
- */
-CameraWorker::CameraWorker()
-{
-	worker_.moveToThread(this);
-}
-
-void CameraWorker::start()
-{
-	Thread::start();
-}
-
-void CameraWorker::stop()
-{
-	exit();
-	wait();
-}
-
-void CameraWorker::run()
-{
-	exec();
-	dispatchMessages(Message::Type::InvokeMessage);
-}
-
-void CameraWorker::queueRequest(CaptureRequest *request)
-{
-	/* Async process the request on the worker which runs its own thread. */
-	worker_.invokeMethod(&Worker::processRequest, ConnectionTypeQueued,
-			     request);
-}
-
-/*
- * \class CameraWorker::Worker
- * \brief Process a CaptureRequest handling acquisition fences
- */
-int CameraWorker::Worker::waitFence(int fence)
-{
-	/*
-	 * \todo Better characterize the timeout. Currently equal to the one
-	 * used by the Rockchip Camera HAL on ChromeOS.
-	 */
-	constexpr unsigned int timeoutMs = 300;
-	struct pollfd fds = { fence, POLLIN, 0 };
-
-	do {
-		int ret = poll(&fds, 1, timeoutMs);
-		if (ret == 0)
-			return -ETIME;
-
-		if (ret > 0) {
-			if (fds.revents & (POLLERR | POLLNVAL))
-				return -EINVAL;
-
-			return 0;
-		}
-	} while (errno == EINTR || errno == EAGAIN);
-
-	return -errno;
-}
-
-void CameraWorker::Worker::processRequest(CaptureRequest *request)
-{
-	/* Wait on all fences before queuing the Request. */
-	for (int fence : request->fences()) {
-		if (fence == -1)
-			continue;
-
-		int ret = waitFence(fence);
-		close(fence);
-		if (ret < 0) {
-			LOG(HAL, Error) << "Failed waiting for fence: "
-					<< fence << ": " << strerror(-ret);
-			return;
-		}
-	}
-
-	request->queue();
-}
diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h
deleted file mode 100644
index 26ecc273..00000000
--- a/src/android/camera_worker.h
+++ /dev/null
@@ -1,70 +0,0 @@
-/* SPDX-License-Identifier: LGPL-2.1-or-later */
-/*
- * Copyright (C) 2020, Google Inc.
- *
- * camera_worker.h - Process capture requests on behalf of the Camera HAL
- */
-
-#pragma once
-
-#include <memory>
-#include <stdint.h>
-
-#include <libcamera/base/object.h>
-#include <libcamera/base/thread.h>
-
-#include <libcamera/camera.h>
-#include <libcamera/framebuffer.h>
-#include <libcamera/request.h>
-#include <libcamera/stream.h>
-
-class CameraDevice;
-
-class CaptureRequest
-{
-public:
-	CaptureRequest(libcamera::Camera *camera, uint64_t cookie);
-
-	const std::vector<int> &fences() const { return acquireFences_; }
-	libcamera::ControlList &controls() { return request_->controls(); }
-	const libcamera::ControlList &metadata() const
-	{
-		return request_->metadata();
-	}
-	unsigned long cookie() const { return request_->cookie(); }
-
-	void addBuffer(libcamera::Stream *stream,
-		       libcamera::FrameBuffer *buffer, int fence);
-	void queue();
-
-private:
-	libcamera::Camera *camera_;
-	std::vector<int> acquireFences_;
-	std::unique_ptr<libcamera::Request> request_;
-};
-
-class CameraWorker : private libcamera::Thread
-{
-public:
-	CameraWorker();
-
-	void start();
-	void stop();
-
-	void queueRequest(CaptureRequest *request);
-
-protected:
-	void run() override;
-
-private:
-	class Worker : public libcamera::Object
-	{
-	public:
-		void processRequest(CaptureRequest *request);
-
-	private:
-		int waitFence(int fence);
-	};
-
-	Worker worker_;
-};
diff --git a/src/android/meson.build b/src/android/meson.build
index 332b177c..75b4bf20 100644
--- a/src/android/meson.build
+++ b/src/android/meson.build
@@ -47,7 +47,6 @@ android_hal_sources = files([
     'camera_ops.cpp',
     'camera_request.cpp',
     'camera_stream.cpp',
-    'camera_worker.cpp',
     'jpeg/encoder_libjpeg.cpp',
     'jpeg/exif.cpp',
     'jpeg/post_processor_jpeg.cpp',