[libcamera-devel] [PATCH v2 1/3] android: camera_worker: Introduce CameraWorker

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Sat, Oct 10, 2020 at 11:58:28AM +0200, Jacopo Mondi wrote:
> The Android camera framework provides for each buffer part of a capture
> request an acquisition fence the camera HAL is supposed to wait on
> before using the buffer. As the libcamera HAL runs in the camera service
> thread, it is not possible to perform a synchronous wait there.
> 
> Introduce a CameraWorker class that runs an internal thread to wait
> on a set of fences before queueing a capture request to the
> libcamera::Camera.
> 
> Fences completion is handled through a simple poll, similar in
> implementation to the sync_wait() function provided by libdrm.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/android/camera_worker.cpp | 122 ++++++++++++++++++++++++++++++++++
>  src/android/camera_worker.h   |  63 ++++++++++++++++++
>  src/android/meson.build       |   1 +
>  3 files changed, 186 insertions(+)
>  create mode 100644 src/android/camera_worker.cpp
>  create mode 100644 src/android/camera_worker.h
> 
> diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp
> new file mode 100644
> index 000000000000..682ad1a82386
> --- /dev/null
> +++ b/src/android/camera_worker.cpp
> @@ -0,0 +1,122 @@
> +/* 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 "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(libcamera::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(&thread_);
> +}
> +
> +void CameraWorker::start()
> +{
> +	thread_.start();
> +}
> +
> +void CameraWorker::stop()
> +{
> +	thread_.exit();
> +	thread_.wait();
> +}
> +
> +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 handling fence: "

Maybe "Failed waiting for fence: " or "Waiting for fence failed: " ?

> +					<< fence << ": " << strerror(-ret);
> +			return;
> +		}
> +	}
> +
> +	request->queue();
> +}
> diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h
> new file mode 100644
> index 000000000000..fff5021708d7
> --- /dev/null
> +++ b/src/android/camera_worker.h
> @@ -0,0 +1,63 @@
> +/* 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
> + */
> +#ifndef __ANDROID_CAMERA_WORKER_H__
> +#define __ANDROID_CAMERA_WORKER_H__
> +
> +#include <memory>
> +
> +#include <libcamera/buffer.h>
> +#include <libcamera/camera.h>
> +#include <libcamera/object.h>
> +#include <libcamera/request.h>
> +#include <libcamera/stream.h>
> +
> +#include "libcamera/internal/thread.h"
> +
> +class CameraDevice;
> +
> +class CaptureRequest
> +{
> +public:
> +	CaptureRequest(libcamera::Camera *camera, uint64_t cookie);
> +
> +	const std::vector<int> &fences() const { return acquireFences_; }
> +
> +	void addBuffer(libcamera::Stream *stream,
> +		       libcamera::FrameBuffer *buffer, int fence);
> +	void queue();
> +
> +private:
> +	libcamera::Camera *camera_;

This duplicates the camera pointer in every CaptureRequest instance, but
that's not a big deal as we'll rework this when moving fence handling to
the libcamera core.

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

> +	std::vector<int> acquireFences_;
> +	std::unique_ptr<libcamera::Request> request_;
> +};
> +
> +class CameraWorker
> +{
> +public:
> +	CameraWorker();
> +
> +	void start();
> +	void stop();
> +
> +	void queueRequest(CaptureRequest *request);
> +
> +private:
> +	class Worker : public libcamera::Object
> +	{
> +	public:
> +		void processRequest(CaptureRequest *request);
> +
> +	private:
> +		int waitFence(int fence);
> +	};
> +
> +	Worker worker_;
> +	libcamera::Thread thread_;
> +};
> +
> +#endif /* __ANDROID_CAMERA_WORKER_H__ */
> diff --git a/src/android/meson.build b/src/android/meson.build
> index 802bb89afe57..b2b2293cf62d 100644
> --- a/src/android/meson.build
> +++ b/src/android/meson.build
> @@ -21,6 +21,7 @@ android_hal_sources = files([
>      'camera_metadata.cpp',
>      'camera_ops.cpp',
>      'camera_stream.cpp',
> +    'camera_worker.cpp',
>      'jpeg/encoder_libjpeg.cpp',
>      'jpeg/exif.cpp',
>  ])

-- 
Regards,

Laurent Pinchart