[{"id":13148,"web_url":"https://patchwork.libcamera.org/comment/13148/","msgid":"<20201009173628.GA25040@pendragon.ideasonboard.com>","date":"2020-10-09T17:36:28","subject":"Re: [libcamera-devel] [PATCH 1/2] android: camera_worker: Introduce\n\tCameraWorker","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Fri, Oct 09, 2020 at 03:39:55PM +0200, Jacopo Mondi wrote:\n> The Android camera framework provides for each buffer part of a capture\n> request an acquisition fence the camera HAL is supposed to wait on\n> before using the buffer. As the libcamera HAL runs in the camera service\n> thread, it is not possible to perform a synchronous wait there.\n> \n> Introduce a CameraWorker class that runs an internal thread to wait\n> on a set of fences before queueing a capture request to the\n> libcamera::Camera.\n> \n> Fences completion is handled through a simple poll, similar in\n> implementation to the sync_wait() function provided by libdrm.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/android/camera_worker.cpp | 88 +++++++++++++++++++++++++++++++++++\n> src/android/camera_worker.h | 73 +++++++++++++++++++++++++++++\n> src/android/meson.build | 1 +\n> 3 files changed, 162 insertions(+)\n> create mode 100644 src/android/camera_worker.cpp\n> create mode 100644 src/android/camera_worker.h\n> \n> diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp\n> new file mode 100644\n> index 000000000000..775473da9f6e\n> --- /dev/null\n> +++ b/src/android/camera_worker.cpp\n> @@ -0,0 +1,88 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * camera_worker.cpp - Process capture request on behalf of the Camera HAL\n> + */\n> +\n> +#include \"camera_worker.h\"\n> +\n> +#include \n> +#include \n> +\n> +#include \"camera_device.h\"\n> +\n> +using namespace libcamera;\n> +\n> +void CaptureRequest::addBuffer(Stream *stream, FrameBuffer *buffer, int fence)\n> +{\n> +\trequest_->addBuffer(stream, buffer);\n> +\tacquireFences_.push_back(fence);\n> +}\n> +\n> +void CaptureRequest::queue(Camera *camera)\n> +{\n> +\tcamera->queueRequest(request_);\n> +}\n> +\n> +CameraWorker::CameraWorker(const std::shared_ptr &camera)\n> +\t: worker_(camera)\n> +{\n> +\tworker_.moveToThread(&thread_);\n> +}\n> +\n> +void CameraWorker::start()\n> +{\n> +\tthread_.start();\n> +}\n> +\n> +void CameraWorker::stop()\n> +{\n> +\tthread_.exit();\n> +\tthread_.wait();\n> +}\n> +\n> +void CameraWorker::queueRequest(std::unique_ptr request)\n> +{\n> +\tCaptureRequest *req = request.release();\n> +\tworker_.invokeMethod(&Worker::processRequest, ConnectionTypeQueued, req);\n> +}\n> +\n> +int CameraWorker::Worker::waitFence(int fence)\n> +{\n> +\tstruct pollfd fds = { fence, POLLIN, 0 };\n> +\tconstexpr unsigned int timeoutMs = 300;\n> +\tint ret;\n> +\n> +\tdo {\n> +\t\tret = poll(&fds, 1, timeoutMs);\n> +\t\tif (ret == 0)\n> +\t\t\treturn -ETIME;\n> +\n> +\t\tif (ret > 0) {\n> +\t\t\tif (fds.revents & (POLLERR | POLLNVAL))\n> +\t\t\t\treturn -EINVAL;\n> +\n> +\t\t\treturn 0;\n> +\t\t}\n> +\t} while (ret == -1 && (errno == EINTR || errno == EAGAIN));\n\nYou could drop ret == -1 as that's guaranteed by the checks above.\n\n> +\n> +\treturn ret;\n\nShould you return -errno here ?\n\n> +}\n> +\n> +void CameraWorker::Worker::processRequest(CaptureRequest *request)\n> +{\n> +\tfor (int fence : request->fences()) {\n> +\t\tif (fence == -1)\n> +\t\t\tcontinue;\n> +\n> +\t\tint ret = waitFence(fence);\n> +\t\tif (ret < 0)\n> +\t\t\treturn;\n> +\n> +\t\tclose(fence);\n> +\t}\n> +\n> +\trequest->queue(camera_.get());\n\nI would have gone for\n\n\tcamera_->queueRequest(request->request());\n\nwith an inline\n\n\tRequest *request() const { return request_; }\n\naccessor in the CaptureRequest class. Up to you.\n\nReviewed-by: Laurent Pinchart \n\n> +\tdelete request;\n> +}\n> diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h\n> new file mode 100644\n> index 000000000000..7e193513d6f4\n> --- /dev/null\n> +++ b/src/android/camera_worker.h\n> @@ -0,0 +1,73 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * camera_worker.h - Process capture request on behalf of the Camera HAL\n> + */\n> +#ifndef __ANDROID_CAMERA_WORKER_H__\n> +#define __ANDROID_CAMERA_WORKER_H__\n> +\n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/thread.h\"\n> +\n> +class CameraDevice;\n> +\n> +struct CaptureRequest {\n> +\tCaptureRequest(libcamera::Request *request)\n> +\t\t: request_(request)\n> +\t{\n> +\t}\n> +\n> +\tconst std::vector &fences() const { return acquireFences_; }\n> +\n> +\tvoid addBuffer(libcamera::Stream *stream,\n> +\t\t libcamera::FrameBuffer *buffer, int fence);\n> +\tvoid queue(libcamera::Camera *camera);\n> +\n> +private:\n> +\tstd::vector acquireFences_;\n> +\tlibcamera::Request *request_;\n> +};\n> +\n> +class CameraWorker\n> +{\n> +public:\n> +\tCameraWorker(const std::shared_ptr &camera);\n> +\n> +\tvoid start();\n> +\tvoid stop();\n> +\n> +\tvoid queueRequest(std::unique_ptr request);\n> +\n> +private:\n> +\tclass Worker : public libcamera::Object\n> +\t{\n> +\tpublic:\n> +\t\tWorker(const std::shared_ptr &camera)\n> +\t\t\t: camera_(camera)\n> +\t\t{\n> +\t\t}\n> +\t\t~Worker()\n> +\t\t{\n> +\t\t}\n> +\n> +\t\tvoid processRequest(CaptureRequest *request);\n> +\n> +\tprivate:\n> +\t\tint waitFence(int fence);\n> +\n> +\t\tconst std::shared_ptr camera_;\n> +\t};\n> +\n> +\tWorker worker_;\n> +\tlibcamera::Thread thread_;\n> +};\n> +\n> +#endif /* __ANDROID_CAMERA_WORKER_H__ */\n> diff --git a/src/android/meson.build b/src/android/meson.build\n> index 802bb89afe57..b2b2293cf62d 100644\n> --- a/src/android/meson.build\n> +++ b/src/android/meson.build\n> @@ -21,6 +21,7 @@ android_hal_sources = files([\n> 'camera_metadata.cpp',\n> 'camera_ops.cpp',\n> 'camera_stream.cpp',\n> + 'camera_worker.cpp',\n> 'jpeg/encoder_libjpeg.cpp',\n> 'jpeg/exif.cpp',\n> ])","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0662EBEEDF\n\tfor ;\n\tFri, 9 Oct 2020 17:37:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 76ABF607D6;\n\tFri, 9 Oct 2020 19:37:13 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DA8C660358\n\tfor ;\n\tFri, 9 Oct 2020 19:37:11 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 56AA3539;\n\tFri, 9 Oct 2020 19:37:11 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"o6+K3AmN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1602265031;\n\tbh=AHrb4atkNUSLfnxOQDaQ4YCC3tyVCx9/xubOs0BJDgw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=o6+K3AmNzLxMAk69G5gUapWZilE/u2WkkK5Q66XppQYQEvypEn3dTIzmR873lcGwQ\n\te3q/A6Gch5mPdrDhjsYjavbHsCi4LFiju6R59+1MkxwXpl5WYfRfkRHer33TvONbdA\n\tu1WGegJiNJkgLcJm9Sq7qE6a+q5LczTv1r6eDDzU=","Date":"Fri, 9 Oct 2020 20:36:28 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20201009173628.GA25040@pendragon.ideasonboard.com>","References":"<20201009133956.77396-1-jacopo@jmondi.org>\n\t<20201009133956.77396-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201009133956.77396-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 1/2] android: camera_worker: Introduce\n\tCameraWorker","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13157,"web_url":"https://patchwork.libcamera.org/comment/13157/","msgid":"<20201010072746.4qpri5tnxwkpyv53@uno.localdomain>","date":"2020-10-10T07:27:46","subject":"Re: [libcamera-devel] [PATCH 1/2] android: camera_worker: Introduce\n\tCameraWorker","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Fri, Oct 09, 2020 at 08:36:28PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Fri, Oct 09, 2020 at 03:39:55PM +0200, Jacopo Mondi wrote:\n> > The Android camera framework provides for each buffer part of a capture\n> > request an acquisition fence the camera HAL is supposed to wait on\n> > before using the buffer. As the libcamera HAL runs in the camera service\n> > thread, it is not possible to perform a synchronous wait there.\n> >\n> > Introduce a CameraWorker class that runs an internal thread to wait\n> > on a set of fences before queueing a capture request to the\n> > libcamera::Camera.\n> >\n> > Fences completion is handled through a simple poll, similar in\n> > implementation to the sync_wait() function provided by libdrm.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/android/camera_worker.cpp | 88 +++++++++++++++++++++++++++++++++++\n> > src/android/camera_worker.h | 73 +++++++++++++++++++++++++++++\n> > src/android/meson.build | 1 +\n> > 3 files changed, 162 insertions(+)\n> > create mode 100644 src/android/camera_worker.cpp\n> > create mode 100644 src/android/camera_worker.h\n> >\n> > diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp\n> > new file mode 100644\n> > index 000000000000..775473da9f6e\n> > --- /dev/null\n> > +++ b/src/android/camera_worker.cpp\n> > @@ -0,0 +1,88 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Google Inc.\n> > + *\n> > + * camera_worker.cpp - Process capture request on behalf of the Camera HAL\n> > + */\n> > +\n> > +#include \"camera_worker.h\"\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \"camera_device.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +void CaptureRequest::addBuffer(Stream *stream, FrameBuffer *buffer, int fence)\n> > +{\n> > +\trequest_->addBuffer(stream, buffer);\n> > +\tacquireFences_.push_back(fence);\n> > +}\n> > +\n> > +void CaptureRequest::queue(Camera *camera)\n> > +{\n> > +\tcamera->queueRequest(request_);\n> > +}\n> > +\n> > +CameraWorker::CameraWorker(const std::shared_ptr &camera)\n> > +\t: worker_(camera)\n> > +{\n> > +\tworker_.moveToThread(&thread_);\n> > +}\n> > +\n> > +void CameraWorker::start()\n> > +{\n> > +\tthread_.start();\n> > +}\n> > +\n> > +void CameraWorker::stop()\n> > +{\n> > +\tthread_.exit();\n> > +\tthread_.wait();\n> > +}\n> > +\n> > +void CameraWorker::queueRequest(std::unique_ptr request)\n> > +{\n> > +\tCaptureRequest *req = request.release();\n> > +\tworker_.invokeMethod(&Worker::processRequest, ConnectionTypeQueued, req);\n> > +}\n> > +\n> > +int CameraWorker::Worker::waitFence(int fence)\n> > +{\n> > +\tstruct pollfd fds = { fence, POLLIN, 0 };\n> > +\tconstexpr unsigned int timeoutMs = 300;\n> > +\tint ret;\n> > +\n> > +\tdo {\n> > +\t\tret = poll(&fds, 1, timeoutMs);\n> > +\t\tif (ret == 0)\n> > +\t\t\treturn -ETIME;\n> > +\n> > +\t\tif (ret > 0) {\n> > +\t\t\tif (fds.revents & (POLLERR | POLLNVAL))\n> > +\t\t\t\treturn -EINVAL;\n> > +\n> > +\t\t\treturn 0;\n> > +\t\t}\n> > +\t} while (ret == -1 && (errno == EINTR || errno == EAGAIN));\n>\n> You could drop ret == -1 as that's guaranteed by the checks above.\n>\n\npoll should not return anything < -1, so I can drop the == - 1 check\n\n> > +\n> > +\treturn ret;\n>\n> Should you return -errno here ?\n>\n\nDoesn't matter much, the erro code is ignored, but I can.\n\n> > +}\n> > +\n> > +void CameraWorker::Worker::processRequest(CaptureRequest *request)\n> > +{\n> > +\tfor (int fence : request->fences()) {\n> > +\t\tif (fence == -1)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tint ret = waitFence(fence);\n> > +\t\tif (ret < 0)\n> > +\t\t\treturn;\n> > +\n> > +\t\tclose(fence);\n> > +\t}\n> > +\n> > +\trequest->queue(camera_.get());\n>\n> I would have gone for\n>\n> \tcamera_->queueRequest(request->request());\n>\n> with an inline\n>\n> \tRequest *request() const { return request_; }\n>\n> accessor in the CaptureRequest class. Up to you.\n\nThis is a bit in-flux due to rebase with reusable request, next\nversion will be slightly different (I would like to have\nrequest->queue() in the end)\n\nWe'll see what goes in first.\n\n>\n> Reviewed-by: Laurent Pinchart \n>\n> > +\tdelete request;\n> > +}\n> > diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h\n> > new file mode 100644\n> > index 000000000000..7e193513d6f4\n> > --- /dev/null\n> > +++ b/src/android/camera_worker.h\n> > @@ -0,0 +1,73 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Google Inc.\n> > + *\n> > + * camera_worker.h - Process capture request on behalf of the Camera HAL\n> > + */\n> > +#ifndef __ANDROID_CAMERA_WORKER_H__\n> > +#define __ANDROID_CAMERA_WORKER_H__\n> > +\n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"libcamera/internal/thread.h\"\n> > +\n> > +class CameraDevice;\n> > +\n> > +struct CaptureRequest {\n> > +\tCaptureRequest(libcamera::Request *request)\n> > +\t\t: request_(request)\n> > +\t{\n> > +\t}\n> > +\n> > +\tconst std::vector &fences() const { return acquireFences_; }\n> > +\n> > +\tvoid addBuffer(libcamera::Stream *stream,\n> > +\t\t libcamera::FrameBuffer *buffer, int fence);\n> > +\tvoid queue(libcamera::Camera *camera);\n> > +\n> > +private:\n> > +\tstd::vector acquireFences_;\n> > +\tlibcamera::Request *request_;\n> > +};\n> > +\n> > +class CameraWorker\n> > +{\n> > +public:\n> > +\tCameraWorker(const std::shared_ptr &camera);\n> > +\n> > +\tvoid start();\n> > +\tvoid stop();\n> > +\n> > +\tvoid queueRequest(std::unique_ptr request);\n> > +\n> > +private:\n> > +\tclass Worker : public libcamera::Object\n> > +\t{\n> > +\tpublic:\n> > +\t\tWorker(const std::shared_ptr &camera)\n> > +\t\t\t: camera_(camera)\n> > +\t\t{\n> > +\t\t}\n> > +\t\t~Worker()\n> > +\t\t{\n> > +\t\t}\n> > +\n> > +\t\tvoid processRequest(CaptureRequest *request);\n> > +\n> > +\tprivate:\n> > +\t\tint waitFence(int fence);\n> > +\n> > +\t\tconst std::shared_ptr camera_;\n> > +\t};\n> > +\n> > +\tWorker worker_;\n> > +\tlibcamera::Thread thread_;\n> > +};\n> > +\n> > +#endif /* __ANDROID_CAMERA_WORKER_H__ */\n> > diff --git a/src/android/meson.build b/src/android/meson.build\n> > index 802bb89afe57..b2b2293cf62d 100644\n> > --- a/src/android/meson.build\n> > +++ b/src/android/meson.build\n> > @@ -21,6 +21,7 @@ android_hal_sources = files([\n> > 'camera_metadata.cpp',\n> > 'camera_ops.cpp',\n> > 'camera_stream.cpp',\n> > + 'camera_worker.cpp',\n> > 'jpeg/encoder_libjpeg.cpp',\n> > 'jpeg/exif.cpp',\n> > ])\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 19680BEEE0\n\tfor ;\n\tSat, 10 Oct 2020 07:23:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 80EBB60730;\n\tSat, 10 Oct 2020 09:23:49 +0200 (CEST)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C8965603C1\n\tfor ;\n\tSat, 10 Oct 2020 09:23:47 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id 6210E240004;\n\tSat, 10 Oct 2020 07:23:47 +0000 (UTC)"],"Date":"Sat, 10 Oct 2020 09:27:46 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20201010072746.4qpri5tnxwkpyv53@uno.localdomain>","References":"<20201009133956.77396-1-jacopo@jmondi.org>\n\t<20201009133956.77396-2-jacopo@jmondi.org>\n\t<20201009173628.GA25040@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201009173628.GA25040@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/2] android: camera_worker: Introduce\n\tCameraWorker","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]