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

[Jacopo Mondi]

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