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

10039 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 | 88 +++++++++++++++++++++++++++++++++++
 src/android/camera_worker.h   | 73 +++++++++++++++++++++++++++++
 src/android/meson.build       |  1 +
 3 files changed, 162 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 Fri, Oct 09, 2020 at 03:39:55PM +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 | 88 +++++++++++++++++++++++++++++++++++
>  src/android/camera_worker.h   | 73 +++++++++++++++++++++++++++++
>  src/android/meson.build       |  1 +
>  3 files changed, 162 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..775473da9f6e
> --- /dev/null
> +++ b/src/android/camera_worker.cpp
> @@ -0,0 +1,88 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * camera_worker.cpp - Process capture request on behalf of the Camera HAL
> + */
> +
> +#include "camera_worker.h"
> +
> +#include <errno.h>
> +#include <sys/poll.h>
> +
> +#include "camera_device.h"
> +
> +using namespace libcamera;
> +
> +void CaptureRequest::addBuffer(Stream *stream, FrameBuffer *buffer, int fence)
> +{
> +	request_->addBuffer(stream, buffer);
> +	acquireFences_.push_back(fence);
> +}
> +
> +void CaptureRequest::queue(Camera *camera)
> +{
> +	camera->queueRequest(request_);
> +}
> +
> +CameraWorker::CameraWorker(const std::shared_ptr<Camera> &camera)
> +	: worker_(camera)
> +{
> +	worker_.moveToThread(&thread_);
> +}
> +
> +void CameraWorker::start()
> +{
> +	thread_.start();
> +}
> +
> +void CameraWorker::stop()
> +{
> +	thread_.exit();
> +	thread_.wait();
> +}
> +
> +void CameraWorker::queueRequest(std::unique_ptr<CaptureRequest> request)
> +{
> +	CaptureRequest *req = request.release();
> +	worker_.invokeMethod(&Worker::processRequest, ConnectionTypeQueued, req);
> +}
> +
> +int CameraWorker::Worker::waitFence(int fence)
> +{
> +	struct pollfd fds = { fence, POLLIN, 0 };
> +	constexpr unsigned int timeoutMs = 300;
> +	int ret;
> +
> +	do {
> +		ret = poll(&fds, 1, timeoutMs);
> +		if (ret == 0)
> +			return -ETIME;
> +
> +		if (ret > 0) {
> +			if (fds.revents & (POLLERR | POLLNVAL))
> +				return -EINVAL;
> +
> +			return 0;
> +		}
> +	} while (ret == -1 && (errno == EINTR || errno == EAGAIN));

You could drop ret == -1 as that's guaranteed by the checks above.

> +
> +	return ret;

Should you return -errno here ?

> +}
> +
> +void CameraWorker::Worker::processRequest(CaptureRequest *request)
> +{
> +	for (int fence : request->fences()) {
> +		if (fence == -1)
> +			continue;
> +
> +		int ret = waitFence(fence);
> +		if (ret < 0)
> +			return;
> +
> +		close(fence);
> +	}
> +
> +	request->queue(camera_.get());

I would have gone for

	camera_->queueRequest(request->request());

with an inline

	Request *request() const { return request_; }

accessor in the CaptureRequest class. Up to you.

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

> +	delete request;
> +}
> diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h
> new file mode 100644
> index 000000000000..7e193513d6f4
> --- /dev/null
> +++ b/src/android/camera_worker.h
> @@ -0,0 +1,73 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * camera_worker.h - Process capture request 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;
> +
> +struct CaptureRequest {
> +	CaptureRequest(libcamera::Request *request)
> +		: request_(request)
> +	{
> +	}
> +
> +	const std::vector<int> &fences() const { return acquireFences_; }
> +
> +	void addBuffer(libcamera::Stream *stream,
> +		       libcamera::FrameBuffer *buffer, int fence);
> +	void queue(libcamera::Camera *camera);
> +
> +private:
> +	std::vector<int> acquireFences_;
> +	libcamera::Request *request_;
> +};
> +
> +class CameraWorker
> +{
> +public:
> +	CameraWorker(const std::shared_ptr<libcamera::Camera> &camera);
> +
> +	void start();
> +	void stop();
> +
> +	void queueRequest(std::unique_ptr<CaptureRequest> request);
> +
> +private:
> +	class Worker : public libcamera::Object
> +	{
> +	public:
> +		Worker(const std::shared_ptr<libcamera::Camera> &camera)
> +			: camera_(camera)
> +		{
> +		}
> +		~Worker()
> +		{
> +		}
> +
> +		void processRequest(CaptureRequest *request);
> +
> +	private:
> +		int waitFence(int fence);
> +
> +		const std::shared_ptr<libcamera::Camera> camera_;
> +	};
> +
> +	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 Laurent,

On Fri, Oct 09, 2020 at 08:36:28PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Fri, Oct 09, 2020 at 03:39:55PM +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 | 88 +++++++++++++++++++++++++++++++++++
> >  src/android/camera_worker.h   | 73 +++++++++++++++++++++++++++++
> >  src/android/meson.build       |  1 +
> >  3 files changed, 162 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..775473da9f6e
> > --- /dev/null
> > +++ b/src/android/camera_worker.cpp
> > @@ -0,0 +1,88 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2020, Google Inc.
> > + *
> > + * camera_worker.cpp - Process capture request on behalf of the Camera HAL
> > + */
> > +
> > +#include "camera_worker.h"
> > +
> > +#include <errno.h>
> > +#include <sys/poll.h>
> > +
> > +#include "camera_device.h"
> > +
> > +using namespace libcamera;
> > +
> > +void CaptureRequest::addBuffer(Stream *stream, FrameBuffer *buffer, int fence)
> > +{
> > +	request_->addBuffer(stream, buffer);
> > +	acquireFences_.push_back(fence);
> > +}
> > +
> > +void CaptureRequest::queue(Camera *camera)
> > +{
> > +	camera->queueRequest(request_);
> > +}
> > +
> > +CameraWorker::CameraWorker(const std::shared_ptr<Camera> &camera)
> > +	: worker_(camera)
> > +{
> > +	worker_.moveToThread(&thread_);
> > +}
> > +
> > +void CameraWorker::start()
> > +{
> > +	thread_.start();
> > +}
> > +
> > +void CameraWorker::stop()
> > +{
> > +	thread_.exit();
> > +	thread_.wait();
> > +}
> > +
> > +void CameraWorker::queueRequest(std::unique_ptr<CaptureRequest> request)
> > +{
> > +	CaptureRequest *req = request.release();
> > +	worker_.invokeMethod(&Worker::processRequest, ConnectionTypeQueued, req);
> > +}
> > +
> > +int CameraWorker::Worker::waitFence(int fence)
> > +{
> > +	struct pollfd fds = { fence, POLLIN, 0 };
> > +	constexpr unsigned int timeoutMs = 300;
> > +	int ret;
> > +
> > +	do {
> > +		ret = poll(&fds, 1, timeoutMs);
> > +		if (ret == 0)
> > +			return -ETIME;
> > +
> > +		if (ret > 0) {
> > +			if (fds.revents & (POLLERR | POLLNVAL))
> > +				return -EINVAL;
> > +
> > +			return 0;
> > +		}
> > +	} while (ret == -1 && (errno == EINTR || errno == EAGAIN));
>
> You could drop ret == -1 as that's guaranteed by the checks above.
>

poll should not return anything < -1, so I can drop the == - 1 check

> > +
> > +	return ret;
>
> Should you return -errno here ?
>

Doesn't matter much, the erro code is ignored, but I can.

> > +}
> > +
> > +void CameraWorker::Worker::processRequest(CaptureRequest *request)
> > +{
> > +	for (int fence : request->fences()) {
> > +		if (fence == -1)
> > +			continue;
> > +
> > +		int ret = waitFence(fence);
> > +		if (ret < 0)
> > +			return;
> > +
> > +		close(fence);
> > +	}
> > +
> > +	request->queue(camera_.get());
>
> I would have gone for
>
> 	camera_->queueRequest(request->request());
>
> with an inline
>
> 	Request *request() const { return request_; }
>
> accessor in the CaptureRequest class. Up to you.

This is a bit in-flux due to rebase with reusable request, next
version will be slightly different (I would like to have
request->queue() in the end)

We'll see what goes in first.

>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > +	delete request;
> > +}
> > diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h
> > new file mode 100644
> > index 000000000000..7e193513d6f4
> > --- /dev/null
> > +++ b/src/android/camera_worker.h
> > @@ -0,0 +1,73 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2020, Google Inc.
> > + *
> > + * camera_worker.h - Process capture request 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;
> > +
> > +struct CaptureRequest {
> > +	CaptureRequest(libcamera::Request *request)
> > +		: request_(request)
> > +	{
> > +	}
> > +
> > +	const std::vector<int> &fences() const { return acquireFences_; }
> > +
> > +	void addBuffer(libcamera::Stream *stream,
> > +		       libcamera::FrameBuffer *buffer, int fence);
> > +	void queue(libcamera::Camera *camera);
> > +
> > +private:
> > +	std::vector<int> acquireFences_;
> > +	libcamera::Request *request_;
> > +};
> > +
> > +class CameraWorker
> > +{
> > +public:
> > +	CameraWorker(const std::shared_ptr<libcamera::Camera> &camera);
> > +
> > +	void start();
> > +	void stop();
> > +
> > +	void queueRequest(std::unique_ptr<CaptureRequest> request);
> > +
> > +private:
> > +	class Worker : public libcamera::Object
> > +	{
> > +	public:
> > +		Worker(const std::shared_ptr<libcamera::Camera> &camera)
> > +			: camera_(camera)
> > +		{
> > +		}
> > +		~Worker()
> > +		{
> > +		}
> > +
> > +		void processRequest(CaptureRequest *request);
> > +
> > +	private:
> > +		int waitFence(int fence);
> > +
> > +		const std::shared_ptr<libcamera::Camera> camera_;
> > +	};
> > +
> > +	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