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

10042 diff mbox series

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@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

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@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@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',
>  ])

Hi Jacopo,

Thanks for your work.

On 2020-10-10 11:58:28 +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@jmondi.org>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  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: "
> +					<< 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_;
> +	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',
>  ])
> -- 
> 2.28.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel