[{"id":19902,"web_url":"https://patchwork.libcamera.org/comment/19902/","msgid":"","date":"2021-09-28T00:04:49","subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Wait on fences in\n\tCameraStream::process()","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo, thank you for the patch\n\nOn Tue, Sep 28, 2021 at 6:36 AM Jacopo Mondi wrote:\n>\n> Acquisition fences for streams of type Mapped generated by\n> post-processing, are not correctly handled and are currently\n> ignored by the camera HAL.\n>\n> Fix this by adding CameraStream::waitFence(), executed before\n> starting the post-processing in CameraStream::process().\n>\n> The change applies to all streams generated by post-processing (Mapped\n> and Internal) but currently acquire fences of Internal streams are\n> handled by the camera worker. Post-pone that to post-processing time by\n> passing -1 to the worker for Internal streams.\n>\n> The CameraStream::waitFence() implementation is copied from the\n> CameraWorker::Worker::waitFence() one, and both should be replaced by a\n> libcamera core mechanism.\n>\n> Signed-off-by: Jacopo Mondi \n> ---\n> src/android/camera_device.cpp | 6 +++--\n> src/android/camera_stream.cpp | 48 +++++++++++++++++++++++++++++++++--\n> src/android/camera_stream.h | 4 ++-\n> 3 files changed, 53 insertions(+), 5 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 3c9609d74402..8b0caf2bda7d 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -973,6 +973,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> const camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];\n> camera3_stream *camera3Stream = camera3Buffer.stream;\n> CameraStream *cameraStream = static_cast(camera3Stream->priv);\n> + int acquireFence = -1;\n\nI would like to have a comment that acquireFence -1 is passed in the\ncase of Internal and reason.\n>\n> std::stringstream ss;\n> ss << i << \" - (\" << camera3Stream->width << \"x\"\n> @@ -1008,6 +1009,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> cameraStream->configuration().size));\n>\n> buffer = descriptor.frameBuffers_.back().get();\n> + acquireFence = camera3Buffer.acquire_fence;\n> LOG(HAL, Debug) << ss.str() << \" (direct)\";\n> break;\n>\n> @@ -1030,7 +1032,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> }\n>\n> descriptor.request_->addBuffer(cameraStream->stream(), buffer,\n> - camera3Buffer.acquire_fence);\n> + acquireFence);\n> }\n>\n> /*\n> @@ -1186,7 +1188,7 @@ void CameraDevice::requestComplete(Request *request)\n> continue;\n> }\n>\n> - int ret = cameraStream->process(*src, *buffer.buffer,\n> + int ret = cameraStream->process(*src, buffer,\n> descriptor.settings_,\n> resultMetadata.get());\n> /*\n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index e30c7ee4fcb3..40dcb361749f 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -7,7 +7,10 @@\n>\n> #include \"camera_stream.h\"\n>\n> +#include \n> #include \n> +#include \n> +#include \n>\n> #include \n>\n> @@ -109,11 +112,52 @@ int CameraStream::configure()\n> return 0;\n> }\n>\n> +int CameraStream::waitFence(int fence)\n> +{\n> + /*\n> + * \\todo The implementation here is copied from camera_worker.cpp\n> + * and both should be removed once libcamera is instrumented to handle\n> + * fences waiting in the core.\n> + *\n> + * \\todo Better characterize the timeout. Currently equal to the one\n> + * used by the Rockchip Camera HAL on ChromeOS.\n> + */\n> + constexpr unsigned int timeoutMs = 300;\n> + struct pollfd fds = { fence, POLLIN, 0 };\n> +\n> + do {\n> + int ret = poll(&fds, 1, timeoutMs);\n> + if (ret == 0)\n> + return -ETIME;\n> +\n> + if (ret > 0) {\n> + if (fds.revents & (POLLERR | POLLNVAL))\n> + return -EINVAL;\n> +\n> + return 0;\n> + }\n> + } while (errno == EINTR || errno == EAGAIN);\n> +\n> + return -errno;\n> +}\n> +\n> int CameraStream::process(const FrameBuffer &source,\n> - buffer_handle_t camera3Dest,\n> + camera3_stream_buffer_t &camera3Buffer,\n> const CameraMetadata &requestMetadata,\n> CameraMetadata *resultMetadata)\n> {\n> + /* Handle waiting on fences on the destination buffer. */\n> + int fence = camera3Buffer.acquire_fence;\n> + if (fence != -1) {\n\nPerhaps ASSERT(type_ == Internal || type_ == Mapped) is useful here?\n\n> + int ret = waitFence(fence);\n> + ::close(fence);\n> + if (ret < 0) {\n> + LOG(HAL, Error) << \"Failed waiting for fence: \"\n> + << fence << \": \" << strerror(-ret);\n> + return ret;\n> + }\n> + }\n> +\n> if (!postProcessor_)\n> return 0;\n>\n> @@ -122,7 +166,7 @@ int CameraStream::process(const FrameBuffer &source,\n> * separate thread.\n> */\n> const StreamConfiguration &output = configuration();\n> - CameraBuffer dest(camera3Dest, formats::MJPEG, output.size,\n> + CameraBuffer dest(*camera3Buffer.buffer, formats::MJPEG, output.size,\n> PROT_READ | PROT_WRITE);\n> if (!dest.isValid()) {\n> LOG(HAL, Error) << \"Failed to map android blob buffer\";\n> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> index 2dab6c3a37d1..454a33dcdaaa 100644\n> --- a/src/android/camera_stream.h\n> +++ b/src/android/camera_stream.h\n> @@ -119,7 +119,7 @@ public:\n>\n> int configure();\n> int process(const libcamera::FrameBuffer &source,\n> - buffer_handle_t camera3Dest,\n> + camera3_stream_buffer_t &camera3Buffer,\n> const CameraMetadata &requestMetadata,\n> CameraMetadata *resultMetadata);\n> libcamera::FrameBuffer *getBuffer();\n> @@ -140,6 +140,8 @@ private:\n> */\n> std::unique_ptr mutex_;\n> std::unique_ptr postProcessor_;\n> +\n> + int waitFence(int fence);\n\nA function should be put before member variables.\n\nWith these nits,\nReviewed-by: Hirokazu Honda \n\n-Hiro\n> };\n>\n> #endif /* __ANDROID_CAMERA_STREAM__ */\n> --\n> 2.32.0\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 16BC5C3243\n\tfor ;\n\tTue, 28 Sep 2021 00:05:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 82B3C6918B;\n\tTue, 28 Sep 2021 02:05:02 +0200 (CEST)","from mail-ed1-x534.google.com (mail-ed1-x534.google.com\n\t[IPv6:2a00:1450:4864:20::534])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 275526012D\n\tfor ;\n\tTue, 28 Sep 2021 02:05:01 +0200 (CEST)","by mail-ed1-x534.google.com with SMTP id o23so4101573edt.12\n\tfor ;\n\tMon, 27 Sep 2021 17:05:01 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"hnUFLmaC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=rQwqKV1Ru3yuodPN5cBJTOXD1uaDBGTOILBs6Vcc/oQ=;\n\tb=hnUFLmaCIq3Snq62DdJAxi10OaVtIzH2j6k71CDdRxoXJM5k2kGGztPGIKyvQ7nLG3\n\tcb7tdnePSIMorg+i1oKoLx9NOeA0xKQsgz1+v95qrKvsElOzrrSyjczBlZKDQSz3zIT7\n\tojL/oww96FVfoGxf0eGIDFFnjOOJV9Cqtd+0I=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=rQwqKV1Ru3yuodPN5cBJTOXD1uaDBGTOILBs6Vcc/oQ=;\n\tb=RwhRf+vh+t9A73qYH6IDmPzfrjj3ppb4AVKqSlPlG2QcSOH/OeiEObS0FiDiEfc7i9\n\txbdL4MlJdLgmxLT0dNVr+5Okf7fN75ZDXuLh104BXT0sGdzmV8SEg6Eo1bqZIWbzJ1sP\n\tchUs4mk9gwZ0ev47sV9hiLhi4wZy2siaB1caAPD9KyopJWAA97ofWo8k8Mx1NBXX+NKL\n\tFee0OsPJ3x6HSxvYCe4kMXfKNQ6hcyTlWjCpgEJInJSB5QALtoykZiK1yqLksXAvXdU9\n\tNwqowqMZvYXe7lBCKUsYm469PEBEM+LWazw5DocZtO3lCSnhYHtZDNsEjeE/GuhFYrab\n\tI9Mg==","X-Gm-Message-State":"AOAM5333GGOulSWZVQJGAlhD31G34LHmv5ihv3oB5bdsX/n1yL4QF6NP\n\tt7vR5KIEnJVoTyTfRpcJC0pr3JqmmjBFTxzguevE/zwUELA=","X-Google-Smtp-Source":"ABdhPJwT00Ruoibwyt3aaEEvU2AvP1dyu3IlvUDUPvGYqDXCnZtrXVTccZON755qL3LHzyJkasfpDn+GAhBOOicfT6c=","X-Received":"by 2002:a17:906:1f49:: with SMTP id\n\td9mr3490309ejk.150.1632787500584; \n\tMon, 27 Sep 2021 17:05:00 -0700 (PDT)","MIME-Version":"1.0","References":"<20210927213700.25365-1-jacopo@jmondi.org>\n\t<20210927213700.25365-3-jacopo@jmondi.org>","In-Reply-To":"<20210927213700.25365-3-jacopo@jmondi.org>","From":"Hirokazu Honda ","Date":"Tue, 28 Sep 2021 09:04:49 +0900","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Wait on fences in\n\tCameraStream::process()","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19904,"web_url":"https://patchwork.libcamera.org/comment/19904/","msgid":"","date":"2021-09-28T01:03:57","subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Wait on fences in\n\tCameraStream::process()","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 Mon, Sep 27, 2021 at 11:37:00PM +0200, Jacopo Mondi wrote:\n> Acquisition fences for streams of type Mapped generated by\n\ns/Acquisition/Acquire/\n\n> post-processing, are not correctly handled and are currently\n\ns/processing,/processing/\n\n> ignored by the camera HAL.\n> \n> Fix this by adding CameraStream::waitFence(), executed before\n> starting the post-processing in CameraStream::process().\n> \n> The change applies to all streams generated by post-processing (Mapped\n> and Internal) but currently acquire fences of Internal streams are\n> handled by the camera worker. Post-pone that to post-processing time by\n\ns/Post-pone/Postpone/\n\n> passing -1 to the worker for Internal streams.\n> \n> The CameraStream::waitFence() implementation is copied from the\n> CameraWorker::Worker::waitFence() one, and both should be replaced by a\n> libcamera core mechanism.\n\nI don't think that's correct, the post-processing streams will stay\ninternal to the HAL, so libcamera won't be able to handle fences for\nthem. The implementation in this patch is fine, but comments should be\nupdated to not state it's temporary.\n\n> Signed-off-by: Jacopo Mondi \n> ---\n> src/android/camera_device.cpp | 6 +++--\n> src/android/camera_stream.cpp | 48 +++++++++++++++++++++++++++++++++--\n> src/android/camera_stream.h | 4 ++-\n> 3 files changed, 53 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 3c9609d74402..8b0caf2bda7d 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -973,6 +973,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\tconst camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];\n> \t\tcamera3_stream *camera3Stream = camera3Buffer.stream;\n> \t\tCameraStream *cameraStream = static_cast(camera3Stream->priv);\n> +\t\tint acquireFence = -1;\n\nI'd move this just before the switch.\n\n> \n> \t\tstd::stringstream ss;\n> \t\tss << i << \" - (\" << camera3Stream->width << \"x\"\n> @@ -1008,6 +1009,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\t\t\t\t\t cameraStream->configuration().size));\n> \n> \t\t\tbuffer = descriptor.frameBuffers_.back().get();\n> +\t\t\tacquireFence = camera3Buffer.acquire_fence;\n> \t\t\tLOG(HAL, Debug) << ss.str() << \" (direct)\";\n> \t\t\tbreak;\n> \n> @@ -1030,7 +1032,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\t}\n> \n> \t\tdescriptor.request_->addBuffer(cameraStream->stream(), buffer,\n> -\t\t\t\t\t\tcamera3Buffer.acquire_fence);\n> +\t\t\t\t\t acquireFence);\n> \t}\n> \n> \t/*\n> @@ -1186,7 +1188,7 @@ void CameraDevice::requestComplete(Request *request)\n> \t\t\tcontinue;\n> \t\t}\n> \n> -\t\tint ret = cameraStream->process(*src, *buffer.buffer,\n> +\t\tint ret = cameraStream->process(*src, buffer,\n> \t\t\t\t\t\tdescriptor.settings_,\n> \t\t\t\t\t\tresultMetadata.get());\n> \t\t/*\n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index e30c7ee4fcb3..40dcb361749f 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -7,7 +7,10 @@\n> \n> #include \"camera_stream.h\"\n> \n> +#include \n\nYou need string.h for strerror().\n\n> #include \n> +#include \n> +#include \n> \n> #include \n> \n> @@ -109,11 +112,52 @@ int CameraStream::configure()\n> \treturn 0;\n> }\n> \n> +int CameraStream::waitFence(int fence)\n> +{\n> +\t/*\n> +\t * \\todo The implementation here is copied from camera_worker.cpp\n> +\t * and both should be removed once libcamera is instrumented to handle\n> +\t * fences waiting in the core.\n> +\t *\n> +\t * \\todo Better characterize the timeout. Currently equal to the one\n> +\t * used by the Rockchip Camera HAL on ChromeOS.\n> +\t */\n> +\tconstexpr unsigned int timeoutMs = 300;\n> +\tstruct pollfd fds = { fence, POLLIN, 0 };\n> +\n> +\tdo {\n> +\t\tint ret = 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 (errno == EINTR || errno == EAGAIN);\n> +\n> +\treturn -errno;\n> +}\n> +\n> int CameraStream::process(const FrameBuffer &source,\n> -\t\t\t buffer_handle_t camera3Dest,\n> +\t\t\t camera3_stream_buffer_t &camera3Buffer,\n> \t\t\t const CameraMetadata &requestMetadata,\n> \t\t\t CameraMetadata *resultMetadata)\n> {\n> +\t/* Handle waiting on fences on the destination buffer. */\n> +\tint fence = camera3Buffer.acquire_fence;\n> +\tif (fence != -1) {\n> +\t\tint ret = waitFence(fence);\n> +\t\t::close(fence);\n\nI would set\n\n\t\tcamera3Buffer.acquire_fence = -1;\n\nhere as I think it can simplify the implementation in CameraDevice (and\nit's nice in any case to keep the state in sync with that is happening).\n\n> +\t\tif (ret < 0) {\n> +\t\t\tLOG(HAL, Error) << \"Failed waiting for fence: \"\n> +\t\t\t\t\t<< fence << \": \" << strerror(-ret);\n> +\t\t\treturn ret;\n> +\t\t}\n> +\t}\n> +\n> \tif (!postProcessor_)\n> \t\treturn 0;\n> \n> @@ -122,7 +166,7 @@ int CameraStream::process(const FrameBuffer &source,\n> \t * separate thread.\n> \t */\n> \tconst StreamConfiguration &output = configuration();\n> -\tCameraBuffer dest(camera3Dest, formats::MJPEG, output.size,\n> +\tCameraBuffer dest(*camera3Buffer.buffer, formats::MJPEG, output.size,\n> \t\t\t PROT_READ | PROT_WRITE);\n> \tif (!dest.isValid()) {\n> \t\tLOG(HAL, Error) << \"Failed to map android blob buffer\";\n> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> index 2dab6c3a37d1..454a33dcdaaa 100644\n> --- a/src/android/camera_stream.h\n> +++ b/src/android/camera_stream.h\n> @@ -119,7 +119,7 @@ public:\n> \n> \tint configure();\n> \tint process(const libcamera::FrameBuffer &source,\n> -\t\t buffer_handle_t camera3Dest,\n> +\t\t camera3_stream_buffer_t &camera3Buffer,\n> \t\t const CameraMetadata &requestMetadata,\n> \t\t CameraMetadata *resultMetadata);\n> \tlibcamera::FrameBuffer *getBuffer();\n> @@ -140,6 +140,8 @@ private:\n> \t */\n> \tstd::unique_ptr mutex_;\n> \tstd::unique_ptr postProcessor_;\n> +\n> +\tint waitFence(int fence);\n\nMember functions should go above member variables.\n\n> };\n> \n> #endif /* __ANDROID_CAMERA_STREAM__ */","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 E28E9C3243\n\tfor ;\n\tTue, 28 Sep 2021 01:04:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 351996918B;\n\tTue, 28 Sep 2021 03:04:06 +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 8937D6012D\n\tfor ;\n\tTue, 28 Sep 2021 03:04:04 +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 EB18D3F1;\n\tTue, 28 Sep 2021 03:04:03 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"U7l6BSxr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632791044;\n\tbh=9FXhTk/0QMkrX5tMfM1Yf6wUC+VmVu6Ln3LIH65+PCk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=U7l6BSxr18SSHtpN22KSAjlvbFhCmX+FeVgbG6c534DyNASNW6n3XAAKKiPnfFUeV\n\tuvNzmH/Rsxu2eJOC890KoxU4Knif912Q+6YWcjTd3/NGGO5OdMed+UhTzR26P5Lhio\n\tiOSSBrsuW52U00VlBiFFvMjHNj7hxRtGEcNCPfHY=","Date":"Tue, 28 Sep 2021 04:03:57 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210927213700.25365-1-jacopo@jmondi.org>\n\t<20210927213700.25365-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210927213700.25365-3-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Wait on fences in\n\tCameraStream::process()","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19918,"web_url":"https://patchwork.libcamera.org/comment/19918/","msgid":"<20210928100539.kw3hiovago2rytzm@uno.localdomain>","date":"2021-09-28T10:05:39","subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Wait on fences in\n\tCameraStream::process()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Tue, Sep 28, 2021 at 04:03:57AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Mon, Sep 27, 2021 at 11:37:00PM +0200, Jacopo Mondi wrote:\n> > Acquisition fences for streams of type Mapped generated by\n>\n> s/Acquisition/Acquire/\n>\n> > post-processing, are not correctly handled and are currently\n>\n> s/processing,/processing/\n>\n> > ignored by the camera HAL.\n> >\n> > Fix this by adding CameraStream::waitFence(), executed before\n> > starting the post-processing in CameraStream::process().\n> >\n> > The change applies to all streams generated by post-processing (Mapped\n> > and Internal) but currently acquire fences of Internal streams are\n> > handled by the camera worker. Post-pone that to post-processing time by\n>\n> s/Post-pone/Postpone/\n>\n> > passing -1 to the worker for Internal streams.\n> >\n> > The CameraStream::waitFence() implementation is copied from the\n> > CameraWorker::Worker::waitFence() one, and both should be replaced by a\n> > libcamera core mechanism.\n>\n> I don't think that's correct, the post-processing streams will stay\n> internal to the HAL, so libcamera won't be able to handle fences for\n> them. The implementation in this patch is fine, but comments should be\n> updated to not state it's temporary.\n>\n\nNot having clarified yet in my head how the core fence handling\nmechanism would look like, I added this more as a todo than anything\nelse. I'll drop.\n\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/android/camera_device.cpp | 6 +++--\n> > src/android/camera_stream.cpp | 48 +++++++++++++++++++++++++++++++++--\n> > src/android/camera_stream.h | 4 ++-\n> > 3 files changed, 53 insertions(+), 5 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 3c9609d74402..8b0caf2bda7d 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -973,6 +973,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > \t\tconst camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];\n> > \t\tcamera3_stream *camera3Stream = camera3Buffer.stream;\n> > \t\tCameraStream *cameraStream = static_cast(camera3Stream->priv);\n> > +\t\tint acquireFence = -1;\n>\n> I'd move this just before the switch.\n>\n> >\n> > \t\tstd::stringstream ss;\n> > \t\tss << i << \" - (\" << camera3Stream->width << \"x\"\n> > @@ -1008,6 +1009,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > \t\t\t\t\t\t cameraStream->configuration().size));\n> >\n> > \t\t\tbuffer = descriptor.frameBuffers_.back().get();\n> > +\t\t\tacquireFence = camera3Buffer.acquire_fence;\n> > \t\t\tLOG(HAL, Debug) << ss.str() << \" (direct)\";\n> > \t\t\tbreak;\n> >\n> > @@ -1030,7 +1032,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > \t\t}\n> >\n> > \t\tdescriptor.request_->addBuffer(cameraStream->stream(), buffer,\n> > -\t\t\t\t\t\tcamera3Buffer.acquire_fence);\n> > +\t\t\t\t\t acquireFence);\n> > \t}\n> >\n> > \t/*\n> > @@ -1186,7 +1188,7 @@ void CameraDevice::requestComplete(Request *request)\n> > \t\t\tcontinue;\n> > \t\t}\n> >\n> > -\t\tint ret = cameraStream->process(*src, *buffer.buffer,\n> > +\t\tint ret = cameraStream->process(*src, buffer,\n> > \t\t\t\t\t\tdescriptor.settings_,\n> > \t\t\t\t\t\tresultMetadata.get());\n> > \t\t/*\n> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > index e30c7ee4fcb3..40dcb361749f 100644\n> > --- a/src/android/camera_stream.cpp\n> > +++ b/src/android/camera_stream.cpp\n> > @@ -7,7 +7,10 @@\n> >\n> > #include \"camera_stream.h\"\n> >\n> > +#include \n>\n> You need string.h for strerror().\n>\n> > #include \n> > +#include \n> > +#include \n> >\n> > #include \n> >\n> > @@ -109,11 +112,52 @@ int CameraStream::configure()\n> > \treturn 0;\n> > }\n> >\n> > +int CameraStream::waitFence(int fence)\n> > +{\n> > +\t/*\n> > +\t * \\todo The implementation here is copied from camera_worker.cpp\n> > +\t * and both should be removed once libcamera is instrumented to handle\n> > +\t * fences waiting in the core.\n> > +\t *\n> > +\t * \\todo Better characterize the timeout. Currently equal to the one\n> > +\t * used by the Rockchip Camera HAL on ChromeOS.\n> > +\t */\n> > +\tconstexpr unsigned int timeoutMs = 300;\n> > +\tstruct pollfd fds = { fence, POLLIN, 0 };\n> > +\n> > +\tdo {\n> > +\t\tint ret = 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 (errno == EINTR || errno == EAGAIN);\n> > +\n> > +\treturn -errno;\n> > +}\n> > +\n> > int CameraStream::process(const FrameBuffer &source,\n> > -\t\t\t buffer_handle_t camera3Dest,\n> > +\t\t\t camera3_stream_buffer_t &camera3Buffer,\n> > \t\t\t const CameraMetadata &requestMetadata,\n> > \t\t\t CameraMetadata *resultMetadata)\n> > {\n> > +\t/* Handle waiting on fences on the destination buffer. */\n> > +\tint fence = camera3Buffer.acquire_fence;\n> > +\tif (fence != -1) {\n> > +\t\tint ret = waitFence(fence);\n> > +\t\t::close(fence);\n>\n> I would set\n>\n> \t\tcamera3Buffer.acquire_fence = -1;\n>\n> here as I think it can simplify the implementation in CameraDevice (and\n> it's nice in any case to keep the state in sync with that is happening).\n>\n\nI considered that as well, but I was blocked by the fact the\nCameraWorker doesn't do it (yet).\n\nI can record that the Worker should be instrumented to do so with a\ntodo in the previous patch and move setting the fence to -1 here\n\n> > +\t\tif (ret < 0) {\n> > +\t\t\tLOG(HAL, Error) << \"Failed waiting for fence: \"\n> > +\t\t\t\t\t<< fence << \": \" << strerror(-ret);\n> > +\t\t\treturn ret;\n> > +\t\t}\n> > +\t}\n> > +\n> > \tif (!postProcessor_)\n> > \t\treturn 0;\n> >\n> > @@ -122,7 +166,7 @@ int CameraStream::process(const FrameBuffer &source,\n> > \t * separate thread.\n> > \t */\n> > \tconst StreamConfiguration &output = configuration();\n> > -\tCameraBuffer dest(camera3Dest, formats::MJPEG, output.size,\n> > +\tCameraBuffer dest(*camera3Buffer.buffer, formats::MJPEG, output.size,\n> > \t\t\t PROT_READ | PROT_WRITE);\n> > \tif (!dest.isValid()) {\n> > \t\tLOG(HAL, Error) << \"Failed to map android blob buffer\";\n> > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> > index 2dab6c3a37d1..454a33dcdaaa 100644\n> > --- a/src/android/camera_stream.h\n> > +++ b/src/android/camera_stream.h\n> > @@ -119,7 +119,7 @@ public:\n> >\n> > \tint configure();\n> > \tint process(const libcamera::FrameBuffer &source,\n> > -\t\t buffer_handle_t camera3Dest,\n> > +\t\t camera3_stream_buffer_t &camera3Buffer,\n> > \t\t const CameraMetadata &requestMetadata,\n> > \t\t CameraMetadata *resultMetadata);\n> > \tlibcamera::FrameBuffer *getBuffer();\n> > @@ -140,6 +140,8 @@ private:\n> > \t */\n> > \tstd::unique_ptr mutex_;\n> > \tstd::unique_ptr postProcessor_;\n> > +\n> > +\tint waitFence(int fence);\n>\n> Member functions should go above member variables.\n\nUps, don't know why I was sure about the opposite.\n\nThanks\n j\n\n>\n> > };\n> >\n> > #endif /* __ANDROID_CAMERA_STREAM__ */\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 353FEBDC71\n\tfor ;\n\tTue, 28 Sep 2021 10:04:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8D54C6918C;\n\tTue, 28 Sep 2021 12:04:53 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 60D6769185\n\tfor ;\n\tTue, 28 Sep 2021 12:04:52 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id C7CA41C0014;\n\tTue, 28 Sep 2021 10:04:51 +0000 (UTC)"],"Date":"Tue, 28 Sep 2021 12:05:39 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210928100539.kw3hiovago2rytzm@uno.localdomain>","References":"<20210927213700.25365-1-jacopo@jmondi.org>\n\t<20210927213700.25365-3-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Wait on fences in\n\tCameraStream::process()","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19919,"web_url":"https://patchwork.libcamera.org/comment/19919/","msgid":"<20210928102937.tycifm2ulpfog2ks@uno.localdomain>","date":"2021-09-28T10:29:37","subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Wait on fences in\n\tCameraStream::process()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Tue, Sep 28, 2021 at 09:04:49AM +0900, Hirokazu Honda wrote:\n> Hi Jacopo, thank you for the patch\n>\n> On Tue, Sep 28, 2021 at 6:36 AM Jacopo Mondi wrote:\n> >\n> > Acquisition fences for streams of type Mapped generated by\n> > post-processing, are not correctly handled and are currently\n> > ignored by the camera HAL.\n> >\n> > Fix this by adding CameraStream::waitFence(), executed before\n> > starting the post-processing in CameraStream::process().\n> >\n> > The change applies to all streams generated by post-processing (Mapped\n> > and Internal) but currently acquire fences of Internal streams are\n> > handled by the camera worker. Post-pone that to post-processing time by\n> > passing -1 to the worker for Internal streams.\n> >\n> > The CameraStream::waitFence() implementation is copied from the\n> > CameraWorker::Worker::waitFence() one, and both should be replaced by a\n> > libcamera core mechanism.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/android/camera_device.cpp | 6 +++--\n> > src/android/camera_stream.cpp | 48 +++++++++++++++++++++++++++++++++--\n> > src/android/camera_stream.h | 4 ++-\n> > 3 files changed, 53 insertions(+), 5 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 3c9609d74402..8b0caf2bda7d 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -973,6 +973,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > const camera3_stream_buffer_t &camera3Buffer = descriptor.buffers_[i];\n> > camera3_stream *camera3Stream = camera3Buffer.stream;\n> > CameraStream *cameraStream = static_cast(camera3Stream->priv);\n> > + int acquireFence = -1;\n>\n> I would like to have a comment that acquireFence -1 is passed in the\n> case of Internal and reason.\n> >\n> > std::stringstream ss;\n> > ss << i << \" - (\" << camera3Stream->width << \"x\"\n> > @@ -1008,6 +1009,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > cameraStream->configuration().size));\n> >\n> > buffer = descriptor.frameBuffers_.back().get();\n> > + acquireFence = camera3Buffer.acquire_fence;\n> > LOG(HAL, Debug) << ss.str() << \" (direct)\";\n> > break;\n> >\n> > @@ -1030,7 +1032,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > }\n> >\n> > descriptor.request_->addBuffer(cameraStream->stream(), buffer,\n> > - camera3Buffer.acquire_fence);\n> > + acquireFence);\n> > }\n> >\n> > /*\n> > @@ -1186,7 +1188,7 @@ void CameraDevice::requestComplete(Request *request)\n> > continue;\n> > }\n> >\n> > - int ret = cameraStream->process(*src, *buffer.buffer,\n> > + int ret = cameraStream->process(*src, buffer,\n> > descriptor.settings_,\n> > resultMetadata.get());\n> > /*\n> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > index e30c7ee4fcb3..40dcb361749f 100644\n> > --- a/src/android/camera_stream.cpp\n> > +++ b/src/android/camera_stream.cpp\n> > @@ -7,7 +7,10 @@\n> >\n> > #include \"camera_stream.h\"\n> >\n> > +#include \n> > #include \n> > +#include \n> > +#include \n> >\n> > #include \n> >\n> > @@ -109,11 +112,52 @@ int CameraStream::configure()\n> > return 0;\n> > }\n> >\n> > +int CameraStream::waitFence(int fence)\n> > +{\n> > + /*\n> > + * \\todo The implementation here is copied from camera_worker.cpp\n> > + * and both should be removed once libcamera is instrumented to handle\n> > + * fences waiting in the core.\n> > + *\n> > + * \\todo Better characterize the timeout. Currently equal to the one\n> > + * used by the Rockchip Camera HAL on ChromeOS.\n> > + */\n> > + constexpr unsigned int timeoutMs = 300;\n> > + struct pollfd fds = { fence, POLLIN, 0 };\n> > +\n> > + do {\n> > + int ret = poll(&fds, 1, timeoutMs);\n> > + if (ret == 0)\n> > + return -ETIME;\n> > +\n> > + if (ret > 0) {\n> > + if (fds.revents & (POLLERR | POLLNVAL))\n> > + return -EINVAL;\n> > +\n> > + return 0;\n> > + }\n> > + } while (errno == EINTR || errno == EAGAIN);\n> > +\n> > + return -errno;\n> > +}\n> > +\n> > int CameraStream::process(const FrameBuffer &source,\n> > - buffer_handle_t camera3Dest,\n> > + camera3_stream_buffer_t &camera3Buffer,\n> > const CameraMetadata &requestMetadata,\n> > CameraMetadata *resultMetadata)\n> > {\n> > + /* Handle waiting on fences on the destination buffer. */\n> > + int fence = camera3Buffer.acquire_fence;\n> > + if (fence != -1) {\n>\n> Perhaps ASSERT(type_ == Internal || type_ == Mapped) is useful here?\n>\n\nI guess we can have fences set to -1 from the camera service, the\ncheck for -1 here is for this reason, not to validate the stream type.\n\nI can add the assertion, but it's an unrelated change.\n\n> > + int ret = waitFence(fence);\n> > + ::close(fence);\n> > + if (ret < 0) {\n> > + LOG(HAL, Error) << \"Failed waiting for fence: \"\n> > + << fence << \": \" << strerror(-ret);\n> > + return ret;\n> > + }\n> > + }\n> > +\n> > if (!postProcessor_)\n> > return 0;\n> >\n> > @@ -122,7 +166,7 @@ int CameraStream::process(const FrameBuffer &source,\n> > * separate thread.\n> > */\n> > const StreamConfiguration &output = configuration();\n> > - CameraBuffer dest(camera3Dest, formats::MJPEG, output.size,\n> > + CameraBuffer dest(*camera3Buffer.buffer, formats::MJPEG, output.size,\n> > PROT_READ | PROT_WRITE);\n> > if (!dest.isValid()) {\n> > LOG(HAL, Error) << \"Failed to map android blob buffer\";\n> > diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> > index 2dab6c3a37d1..454a33dcdaaa 100644\n> > --- a/src/android/camera_stream.h\n> > +++ b/src/android/camera_stream.h\n> > @@ -119,7 +119,7 @@ public:\n> >\n> > int configure();\n> > int process(const libcamera::FrameBuffer &source,\n> > - buffer_handle_t camera3Dest,\n> > + camera3_stream_buffer_t &camera3Buffer,\n> > const CameraMetadata &requestMetadata,\n> > CameraMetadata *resultMetadata);\n> > libcamera::FrameBuffer *getBuffer();\n> > @@ -140,6 +140,8 @@ private:\n> > */\n> > std::unique_ptr mutex_;\n> > std::unique_ptr postProcessor_;\n> > +\n> > + int waitFence(int fence);\n>\n> A function should be put before member variables.\n>\n> With these nits,\n> Reviewed-by: Hirokazu Honda \n>\n> -Hiro\n> > };\n> >\n> > #endif /* __ANDROID_CAMERA_STREAM__ */\n> > --\n> > 2.32.0\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 B8C79C3243\n\tfor ;\n\tTue, 28 Sep 2021 10:28:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D88B96918E;\n\tTue, 28 Sep 2021 12:28:52 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D638469185\n\tfor ;\n\tTue, 28 Sep 2021 12:28:50 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 59F5E1C0005;\n\tTue, 28 Sep 2021 10:28:50 +0000 (UTC)"],"Date":"Tue, 28 Sep 2021 12:29:37 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210928102937.tycifm2ulpfog2ks@uno.localdomain>","References":"<20210927213700.25365-1-jacopo@jmondi.org>\n\t<20210927213700.25365-3-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: Wait on fences in\n\tCameraStream::process()","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]