[{"id":19943,"web_url":"https://patchwork.libcamera.org/comment/19943/","msgid":"","date":"2021-09-28T16:49:37","subject":"Re: [libcamera-devel] [PATCH v3 1/1] android: Wait on fences in\n\tCameraStream::process()","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn 9/28/21 5:55 PM, Jacopo Mondi wrote:\n> Acquire 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> Also correct the release_fence handling for failed captures, as the\n> framework requires the release fences to be set to the acquire fence\n> value if the acquire fence has not been waited on.\n>\n> Signed-off-by: Jacopo Mondi \n> Reviewed-by: Hirokazu Honda \n> ---\n> src/android/camera_device.cpp | 39 +++++++++++++++++++++++----\n> src/android/camera_stream.cpp | 50 +++++++++++++++++++++++++++++++++--\n> src/android/camera_stream.h | 4 ++-\n> 3 files changed, 85 insertions(+), 8 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index ab38116800cd..0fe11fe833b0 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -983,9 +983,13 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \n> \t\t/*\n> \t\t * Inspect the camera stream type, create buffers opportunely\n> -\t\t * and add them to the Request if required.\n> +\t\t * and add them to the Request if required. Only acquire fences\n> +\t\t * for streams of type Direct are handled by the CameraWorker,\n> +\t\t * while fences for streams of type Internal and Mapped are\n> +\t\t * handled at post-processing time.\n\n\n+1 for the comment, makes things clear\n\n> \t\t */\n> \t\tFrameBuffer *buffer = nullptr;\n> +\t\tint acquireFence = -1;\n> \t\tswitch (cameraStream->type()) {\n> \t\tcase CameraStream::Type::Mapped:\n> \t\t\t/*\n> @@ -1008,6 +1012,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 +1035,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\nOk, so by this worker will wait on these fences, but only for Type::Direct\n\n> \n> \t/*\n> @@ -1110,7 +1115,22 @@ void CameraDevice::requestComplete(Request *request)\n> \tcaptureResult.frame_number = descriptor.frameNumber_;\n> \tcaptureResult.num_output_buffers = descriptor.buffers_.size();\n> \tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> -\t\tbuffer.acquire_fence = -1;\n> +\t\tCameraStream *cameraStream =\n> +\t\t\tstatic_cast(buffer.stream->priv);\n> +\n> +\t\t/*\n> +\t\t * Streams of type Direct have been queued to the\n> +\t\t * libcamera::Camera and their acquisition fences have\n> +\t\t * already been waited on by the CameraWorker.\n> +\t\t *\n> +\t\t * Acquire fences of streams of type Internal and Mapped\n> +\t\t * will be handled during post-processing.\n> +\t\t *\n> +\t\t * \\todo Instrument the CameraWorker to set the acquire\n> +\t\t * fence to -1 once it has handled it and remove this check.\n> +\t\t */\n> +\t\tif (cameraStream->type() == CameraStream::Type::Direct)\n> +\t\t\tbuffer.acquire_fence = -1;\n\n\nYep, for Type::Direct they have  waited upon by worker already, so we \ncan set them to -1\n\nAnd release_fence for all buffers is to -1 as right below:\n\n> \t\tbuffer.release_fence = -1;\n> \t\tbuffer.status = CAMERA3_BUFFER_STATUS_OK;\n> \t}\n> @@ -1130,8 +1150,17 @@ void CameraDevice::requestComplete(Request *request)\n> \t\t\t CAMERA3_MSG_ERROR_REQUEST);\n> \n> \t\tcaptureResult.partial_result = 0;\n> -\t\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_)\n> +\t\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> +\t\t\t/*\n> +\t\t\t * Signal to the framework it has to handle fences that\n> +\t\t\t * have not been waited on by setting the release fence\n> +\t\t\t * to the acquire fence value.\n> +\t\t\t */\n> +\t\t\tbuffer.release_fence = buffer.acquire_fence;\n> +\t\t\tbuffer.acquire_fence = -1;\n> \t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> +\t\t}\n> +\n\n\nYep, that's what should be happening on error as per docs\n\n> \t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> \n> \t\treturn;\n> @@ -1181,7 +1210,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..2363985398fb 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -7,7 +7,11 @@\n> \n> #include \"camera_stream.h\"\n> \n> +#include \n> +#include \n> #include \n> +#include \n> +#include \n> \n> #include \n> \n> @@ -109,11 +113,53 @@ 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\n\nAh these are not Type::Direct, but Type::Mapped probably, so we need to \ncheck the fences and wait upon here\n\n> +\t\t::close(fence);\n> +\t\tcamera3Buffer.acquire_fence = -1;\n\n\nfence is closed (fixing the leak!) and acquire_fence = -1. Make sense.\n\nThis is looking quite logical already but I have a question\n\nIf I am seeing things correctly, I don't see a\n\n         buffer.release_fence = buffer.acquire_fence;\n\nin the post-processing error handling block. Should it be the case on \npost-processing failure or I am missing something? My reason is we do \nmark buffer as CAMERA3_BUFFER_STATUS_ERROR on post-processing failure, \nso maybe we should do the right thing with fences too ? But not sure, I \nhave limited understanding in this aspect.\n\nI have been testing this already with cts so,\n\nTested-by: Umang Jain \nReviewed-by: Umang Jain \n\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 +168,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..03ecfa94fc8c 100644\n> --- a/src/android/camera_stream.h\n> +++ b/src/android/camera_stream.h\n> @@ -119,13 +119,15 @@ 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> \tvoid putBuffer(libcamera::FrameBuffer *buffer);\n> \n> private:\n> +\tint waitFence(int fence);\n> +\n> \tCameraDevice *const cameraDevice_;\n> \tconst libcamera::CameraConfiguration *config_;\n> \tconst Type type_;","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 D91A9BDC71\n\tfor ;\n\tTue, 28 Sep 2021 16:50:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4DE1969191;\n\tTue, 28 Sep 2021 18:50:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C274469188\n\tfor ;\n\tTue, 28 Sep 2021 18:49:58 +0200 (CEST)","from [IPv6:2405:204:8500:b152:4879:ab55:a9fb:2b15] (unknown\n\t[IPv6:2405:204:8500:b152:4879:ab55:a9fb:2b15])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A8FAB3F1;\n\tTue, 28 Sep 2021 18:49:44 +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=\"Ou4Obi+I\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632847798;\n\tbh=aAk8Yr4+vq57JmjxR07G5pm2ao2pF6StNHirlsDwd6o=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=Ou4Obi+IBZMhiFNtmjsQxAcOXRz6LCb3VpcnoQf/6bbKDQEt34tcYKR736OGkg3fg\n\tjEuJqykHBxycKY3CAHOeVQrYilf/QqiFuhaePJz9aFJU5vs2jljuWfMXewMIF4Qg3U\n\t+MdNLmwBNVZ9VmKP+s1kfUdc5KRVaUYYyaqmwyB4=","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20210928122515.28034-1-jacopo@jmondi.org>\n\t<20210928122515.28034-2-jacopo@jmondi.org>","From":"Umang Jain ","Message-ID":"","Date":"Tue, 28 Sep 2021 22:19:37 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20210928122515.28034-2-jacopo@jmondi.org>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v3 1/1] 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19947,"web_url":"https://patchwork.libcamera.org/comment/19947/","msgid":"","date":"2021-09-28T20:25:14","subject":"Re: [libcamera-devel] [PATCH v3 1/1] 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":"Hello,\n\nOn Tue, Sep 28, 2021 at 10:19:37PM +0530, Umang Jain wrote:\n> On 9/28/21 5:55 PM, Jacopo Mondi wrote:\n> > Acquire 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\ns/Post-pone/Postpone/\n\n> > passing -1 to the Worker for Internal streams.\n> >\n> > Also correct the release_fence handling for failed captures, as the\n> > framework requires the release fences to be set to the acquire fence\n> > value if the acquire fence has not been waited on.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > Reviewed-by: Hirokazu Honda \n> > ---\n> > src/android/camera_device.cpp | 39 +++++++++++++++++++++++----\n> > src/android/camera_stream.cpp | 50 +++++++++++++++++++++++++++++++++--\n> > src/android/camera_stream.h | 4 ++-\n> > 3 files changed, 85 insertions(+), 8 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index ab38116800cd..0fe11fe833b0 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -983,9 +983,13 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > \n> > \t\t/*\n> > \t\t * Inspect the camera stream type, create buffers opportunely\n> > -\t\t * and add them to the Request if required.\n> > +\t\t * and add them to the Request if required. Only acquire fences\n> > +\t\t * for streams of type Direct are handled by the CameraWorker,\n> > +\t\t * while fences for streams of type Internal and Mapped are\n> > +\t\t * handled at post-processing time.\n\nWe don't need fences for internal streams, right ? Should this be\n\n\t\t * while fences for Mapped streams are handled at\n\t\t * post-processing time. Internal streams do not use fences at\n\t\t * all.\n\n> +1 for the comment, makes things clear\n> \n> > \t\t */\n> > \t\tFrameBuffer *buffer = nullptr;\n> > +\t\tint acquireFence = -1;\n> > \t\tswitch (cameraStream->type()) {\n> > \t\tcase CameraStream::Type::Mapped:\n> > \t\t\t/*\n> > @@ -1008,6 +1012,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 +1035,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> \n> Ok, so by this worker will wait on these fences, but only for Type::Direct\n> \n> > \n> > \t/*\n> > @@ -1110,7 +1115,22 @@ void CameraDevice::requestComplete(Request *request)\n> > \tcaptureResult.frame_number = descriptor.frameNumber_;\n> > \tcaptureResult.num_output_buffers = descriptor.buffers_.size();\n> > \tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > -\t\tbuffer.acquire_fence = -1;\n> > +\t\tCameraStream *cameraStream =\n> > +\t\t\tstatic_cast(buffer.stream->priv);\n> > +\n> > +\t\t/*\n> > +\t\t * Streams of type Direct have been queued to the\n> > +\t\t * libcamera::Camera and their acquisition fences have\n\ns/acquisition/acquire/\n\n> > +\t\t * already been waited on by the CameraWorker.\n> > +\t\t *\n> > +\t\t * Acquire fences of streams of type Internal and Mapped\n> > +\t\t * will be handled during post-processing.\n\nDitto here,\n\n\t\t * Acquire fences of Mapped streams will be handled during\n\t\t * post-processing.\n\n> > +\t\t *\n> > +\t\t * \\todo Instrument the CameraWorker to set the acquire\n> > +\t\t * fence to -1 once it has handled it and remove this check.\n> > +\t\t */\n> > +\t\tif (cameraStream->type() == CameraStream::Type::Direct)\n> > +\t\t\tbuffer.acquire_fence = -1;\n> \n> Yep, for Type::Direct they have  waited upon by worker already, so we \n> can set them to -1\n> \n> And release_fence for all buffers is to -1 as right below:\n> \n> > \t\tbuffer.release_fence = -1;\n> > \t\tbuffer.status = CAMERA3_BUFFER_STATUS_OK;\n> > \t}\n> > @@ -1130,8 +1150,17 @@ void CameraDevice::requestComplete(Request *request)\n> > \t\t\t CAMERA3_MSG_ERROR_REQUEST);\n> > \n> > \t\tcaptureResult.partial_result = 0;\n> > -\t\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_)\n> > +\t\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > +\t\t\t/*\n> > +\t\t\t * Signal to the framework it has to handle fences that\n> > +\t\t\t * have not been waited on by setting the release fence\n> > +\t\t\t * to the acquire fence value.\n> > +\t\t\t */\n> > +\t\t\tbuffer.release_fence = buffer.acquire_fence;\n> > +\t\t\tbuffer.acquire_fence = -1;\n> > \t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > +\t\t}\n> > +\n> \n> Yep, that's what should be happening on error as per docs\n> \n> > \t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> > \n> > \t\treturn;\n> > @@ -1181,7 +1210,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..2363985398fb 100644\n> > --- a/src/android/camera_stream.cpp\n> > +++ b/src/android/camera_stream.cpp\n> > @@ -7,7 +7,11 @@\n> > \n> > #include \"camera_stream.h\"\n> > \n> > +#include \n> > +#include \n> > #include \n> > +#include \n> > +#include \n> > \n> > #include \n> > \n> > @@ -109,11 +113,53 @@ 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\nCould we name the parameter camera3Dest, dest, dst or destination ?\nOtherwise it's not clear in the implementation of the function which\nbuffer \"camera3Buffer\" refers to.\n\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> \n> Ah these are not Type::Direct, but Type::Mapped probably, so we need to \n> check the fences and wait upon here\n> \n> > +\t\t::close(fence);\n> > +\t\tcamera3Buffer.acquire_fence = -1;\n> \n> fence is closed (fixing the leak!) and acquire_fence = -1. Make sense.\n> \n> This is looking quite logical already but I have a question\n> \n> If I am seeing things correctly, I don't see a\n> \n>         buffer.release_fence = buffer.acquire_fence;\n> \n> in the post-processing error handling block. Should it be the case on \n> post-processing failure or I am missing something? My reason is we do \n> mark buffer as CAMERA3_BUFFER_STATUS_ERROR on post-processing failure, \n> so maybe we should do the right thing with fences too ? But not sure, I \n> have limited understanding in this aspect.\n\nWe only need to do that if we haven't waited on the fence. If we wait\nbut processing then fails, there's no need to move the acquire_fence to\nthe release_fence.\n\nReviewed-by: Laurent Pinchart \n\n> I have been testing this already with cts so,\n> \n> Tested-by: Umang Jain \n> Reviewed-by: Umang Jain \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 +168,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..03ecfa94fc8c 100644\n> > --- a/src/android/camera_stream.h\n> > +++ b/src/android/camera_stream.h\n> > @@ -119,13 +119,15 @@ 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> > \tvoid putBuffer(libcamera::FrameBuffer *buffer);\n> > \n> > private:\n> > +\tint waitFence(int fence);\n> > +\n> > \tCameraDevice *const cameraDevice_;\n> > \tconst libcamera::CameraConfiguration *config_;\n> > \tconst Type type_;","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 400C8C3243\n\tfor ;\n\tTue, 28 Sep 2021 20:25:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6270369191;\n\tTue, 28 Sep 2021 22:25:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2BF0269188\n\tfor ;\n\tTue, 28 Sep 2021 22:25:16 +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 82DDC3F1;\n\tTue, 28 Sep 2021 22:25:15 +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=\"fdskA8oN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632860715;\n\tbh=WW8Dymn354rprI3uHvMiAi5a9lr0gTtBKb22RQ0MVEs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fdskA8oNxUF6tIbUnpxaQUVq0rNF8wphyPN956IrD2kg0cDl03OIi0KH0RPmdwaKv\n\tccRknTGa/AayCW/1DQRKeOJjwPd5iq2HU7uqsPTeCT9+pXXPaynnRaNEwjsFT9hr7n\n\tQIEgep6aA6MYLXc7IyKtXt1Tsq86QLlS8k7vZYP8=","Date":"Tue, 28 Sep 2021 23:25:14 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210928122515.28034-1-jacopo@jmondi.org>\n\t<20210928122515.28034-2-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 1/1] 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":19989,"web_url":"https://patchwork.libcamera.org/comment/19989/","msgid":"<20210929115954.52cvl3gihfzkq7zo@uno.localdomain>","date":"2021-09-29T11:59:54","subject":"Re: [libcamera-devel] [PATCH v3 1/1] 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, Umang,\n\n\nOn Tue, Sep 28, 2021 at 11:25:14PM +0300, Laurent Pinchart wrote:\n> Hello,\n>\n> On Tue, Sep 28, 2021 at 10:19:37PM +0530, Umang Jain wrote:\n> > On 9/28/21 5:55 PM, Jacopo Mondi wrote:\n> > > Acquire 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>\n> s/Post-pone/Postpone/\n>\n> > > passing -1 to the Worker for Internal streams.\n> > >\n> > > Also correct the release_fence handling for failed captures, as the\n> > > framework requires the release fences to be set to the acquire fence\n> > > value if the acquire fence has not been waited on.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > Reviewed-by: Hirokazu Honda \n> > > ---\n> > > src/android/camera_device.cpp | 39 +++++++++++++++++++++++----\n> > > src/android/camera_stream.cpp | 50 +++++++++++++++++++++++++++++++++--\n> > > src/android/camera_stream.h | 4 ++-\n> > > 3 files changed, 85 insertions(+), 8 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index ab38116800cd..0fe11fe833b0 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -983,9 +983,13 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > >\n> > > \t\t/*\n> > > \t\t * Inspect the camera stream type, create buffers opportunely\n> > > -\t\t * and add them to the Request if required.\n> > > +\t\t * and add them to the Request if required. Only acquire fences\n> > > +\t\t * for streams of type Direct are handled by the CameraWorker,\n> > > +\t\t * while fences for streams of type Internal and Mapped are\n> > > +\t\t * handled at post-processing time.\n>\n> We don't need fences for internal streams, right ? Should this be\n>\n> \t\t * while fences for Mapped streams are handled at\n> \t\t * post-processing time. Internal streams do not use fences at\n> \t\t * all.\n>\n\nInternal streams are created to support generating through\npost-processing a format the libcamera::Camera cannot produce, the\nmost notable (and only?) example is JPEG.\n\nThe destination buffer where the post-processor writes to might be\nassociated with an acquire_fence as well in my understanding.\n\nHave I missed something ?\n\n> > +1 for the comment, makes things clear\n> >\n> > > \t\t */\n> > > \t\tFrameBuffer *buffer = nullptr;\n> > > +\t\tint acquireFence = -1;\n> > > \t\tswitch (cameraStream->type()) {\n> > > \t\tcase CameraStream::Type::Mapped:\n> > > \t\t\t/*\n> > > @@ -1008,6 +1012,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 +1035,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> >\n> > Ok, so by this worker will wait on these fences, but only for Type::Direct\n> >\n> > >\n> > > \t/*\n> > > @@ -1110,7 +1115,22 @@ void CameraDevice::requestComplete(Request *request)\n> > > \tcaptureResult.frame_number = descriptor.frameNumber_;\n> > > \tcaptureResult.num_output_buffers = descriptor.buffers_.size();\n> > > \tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > > -\t\tbuffer.acquire_fence = -1;\n> > > +\t\tCameraStream *cameraStream =\n> > > +\t\t\tstatic_cast(buffer.stream->priv);\n> > > +\n> > > +\t\t/*\n> > > +\t\t * Streams of type Direct have been queued to the\n> > > +\t\t * libcamera::Camera and their acquisition fences have\n>\n> s/acquisition/acquire/\n>\n> > > +\t\t * already been waited on by the CameraWorker.\n> > > +\t\t *\n> > > +\t\t * Acquire fences of streams of type Internal and Mapped\n> > > +\t\t * will be handled during post-processing.\n>\n> Ditto here,\n>\n> \t\t * Acquire fences of Mapped streams will be handled during\n> \t\t * post-processing.\n>\n> > > +\t\t *\n> > > +\t\t * \\todo Instrument the CameraWorker to set the acquire\n> > > +\t\t * fence to -1 once it has handled it and remove this check.\n> > > +\t\t */\n> > > +\t\tif (cameraStream->type() == CameraStream::Type::Direct)\n> > > +\t\t\tbuffer.acquire_fence = -1;\n> >\n> > Yep, for Type::Direct they have  waited upon by worker already, so we\n> > can set them to -1\n> >\n> > And release_fence for all buffers is to -1 as right below:\n> >\n> > > \t\tbuffer.release_fence = -1;\n> > > \t\tbuffer.status = CAMERA3_BUFFER_STATUS_OK;\n> > > \t}\n> > > @@ -1130,8 +1150,17 @@ void CameraDevice::requestComplete(Request *request)\n> > > \t\t\t CAMERA3_MSG_ERROR_REQUEST);\n> > >\n> > > \t\tcaptureResult.partial_result = 0;\n> > > -\t\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_)\n> > > +\t\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > > +\t\t\t/*\n> > > +\t\t\t * Signal to the framework it has to handle fences that\n> > > +\t\t\t * have not been waited on by setting the release fence\n> > > +\t\t\t * to the acquire fence value.\n> > > +\t\t\t */\n> > > +\t\t\tbuffer.release_fence = buffer.acquire_fence;\n> > > +\t\t\tbuffer.acquire_fence = -1;\n> > > \t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > +\t\t}\n> > > +\n> >\n> > Yep, that's what should be happening on error as per docs\n> >\n> > > \t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> > >\n> > > \t\treturn;\n> > > @@ -1181,7 +1210,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..2363985398fb 100644\n> > > --- a/src/android/camera_stream.cpp\n> > > +++ b/src/android/camera_stream.cpp\n> > > @@ -7,7 +7,11 @@\n> > >\n> > > #include \"camera_stream.h\"\n> > >\n> > > +#include \n> > > +#include \n> > > #include \n> > > +#include \n> > > +#include \n> > >\n> > > #include \n> > >\n> > > @@ -109,11 +113,53 @@ 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>\n> Could we name the parameter camera3Dest, dest, dst or destination ?\n> Otherwise it's not clear in the implementation of the function which\n> buffer \"camera3Buffer\" refers to.\n>\n\nAccording to the badly enforced naming scheme in use in the HAL,\nvariable prefixed with camera3 are meant to represent object provided\nby the camera service to the HAL. I can change this though, also\nbecause it was the name originally in use.\n\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> >\n> > Ah these are not Type::Direct, but Type::Mapped probably, so we need to\n> > check the fences and wait upon here\n> >\n\nMapped or Internal\n\n> > > +\t\t::close(fence);\n> > > +\t\tcamera3Buffer.acquire_fence = -1;\n> >\n> > fence is closed (fixing the leak!) and acquire_fence = -1. Make sense.\n> >\n> > This is looking quite logical already but I have a question\n> >\n> > If I am seeing things correctly, I don't see a\n> >\n> >         buffer.release_fence = buffer.acquire_fence;\n> >\n> > in the post-processing error handling block. Should it be the case on\n> > post-processing failure or I am missing something? My reason is we do\n> > mark buffer as CAMERA3_BUFFER_STATUS_ERROR on post-processing failure,\n> > so maybe we should do the right thing with fences too ? But not sure, I\n> > have limited understanding in this aspect.\n>\n> We only need to do that if we haven't waited on the fence. If we wait\n> but processing then fails, there's no need to move the acquire_fence to\n> the release_fence.\n\nYes, and we close the fence unconditionally, so we should not ask the\ncamera service to close it for us.\n\n>\n> Reviewed-by: Laurent Pinchart \n>\n> > I have been testing this already with cts so,\n> >\n> > Tested-by: Umang Jain \n> > Reviewed-by: Umang Jain \n> >\n\nThank you both\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 +168,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..03ecfa94fc8c 100644\n> > > --- a/src/android/camera_stream.h\n> > > +++ b/src/android/camera_stream.h\n> > > @@ -119,13 +119,15 @@ 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> > > \tvoid putBuffer(libcamera::FrameBuffer *buffer);\n> > >\n> > > private:\n> > > +\tint waitFence(int fence);\n> > > +\n> > > \tCameraDevice *const cameraDevice_;\n> > > \tconst libcamera::CameraConfiguration *config_;\n> > > \tconst Type type_;\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 0D6BDC3243\n\tfor ;\n\tWed, 29 Sep 2021 11:59:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5954A691AA;\n\tWed, 29 Sep 2021 13:59:08 +0200 (CEST)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8514869185\n\tfor ;\n\tWed, 29 Sep 2021 13:59:07 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id D63C81BF212;\n\tWed, 29 Sep 2021 11:59:06 +0000 (UTC)"],"Date":"Wed, 29 Sep 2021 13:59:54 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210929115954.52cvl3gihfzkq7zo@uno.localdomain>","References":"<20210928122515.28034-1-jacopo@jmondi.org>\n\t<20210928122515.28034-2-jacopo@jmondi.org>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 1/1] 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":19992,"web_url":"https://patchwork.libcamera.org/comment/19992/","msgid":"","date":"2021-09-29T12:18:04","subject":"Re: [libcamera-devel] [PATCH v3 1/1] 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\nOn Wed, Sep 29, 2021 at 01:59:54PM +0200, Jacopo Mondi wrote:\n> On Tue, Sep 28, 2021 at 11:25:14PM +0300, Laurent Pinchart wrote:\n> > On Tue, Sep 28, 2021 at 10:19:37PM +0530, Umang Jain wrote:\n> > > On 9/28/21 5:55 PM, Jacopo Mondi wrote:\n> > > > Acquire 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> >\n> > s/Post-pone/Postpone/\n> >\n> > > > passing -1 to the Worker for Internal streams.\n> > > >\n> > > > Also correct the release_fence handling for failed captures, as the\n> > > > framework requires the release fences to be set to the acquire fence\n> > > > value if the acquire fence has not been waited on.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > Reviewed-by: Hirokazu Honda \n> > > > ---\n> > > > src/android/camera_device.cpp | 39 +++++++++++++++++++++++----\n> > > > src/android/camera_stream.cpp | 50 +++++++++++++++++++++++++++++++++--\n> > > > src/android/camera_stream.h | 4 ++-\n> > > > 3 files changed, 85 insertions(+), 8 deletions(-)\n> > > >\n> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > index ab38116800cd..0fe11fe833b0 100644\n> > > > --- a/src/android/camera_device.cpp\n> > > > +++ b/src/android/camera_device.cpp\n> > > > @@ -983,9 +983,13 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > >\n> > > > \t\t/*\n> > > > \t\t * Inspect the camera stream type, create buffers opportunely\n> > > > -\t\t * and add them to the Request if required.\n> > > > +\t\t * and add them to the Request if required. Only acquire fences\n> > > > +\t\t * for streams of type Direct are handled by the CameraWorker,\n> > > > +\t\t * while fences for streams of type Internal and Mapped are\n> > > > +\t\t * handled at post-processing time.\n> >\n> > We don't need fences for internal streams, right ? Should this be\n> >\n> > \t\t * while fences for Mapped streams are handled at\n> > \t\t * post-processing time. Internal streams do not use fences at\n> > \t\t * all.\n> \n> Internal streams are created to support generating through\n> post-processing a format the libcamera::Camera cannot produce, the\n> most notable (and only?) example is JPEG.\n> \n> The destination buffer where the post-processor writes to might be\n> associated with an acquire_fence as well in my understanding.\n> \n> Have I missed something ?\n\nAs far as I understand, the destination buffers for internal streams are\nallocated internally, where would a fence come from ?\n\n> > > +1 for the comment, makes things clear\n> > >\n> > > > \t\t */\n> > > > \t\tFrameBuffer *buffer = nullptr;\n> > > > +\t\tint acquireFence = -1;\n> > > > \t\tswitch (cameraStream->type()) {\n> > > > \t\tcase CameraStream::Type::Mapped:\n> > > > \t\t\t/*\n> > > > @@ -1008,6 +1012,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 +1035,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> > >\n> > > Ok, so by this worker will wait on these fences, but only for Type::Direct\n> > >\n> > > >\n> > > > \t/*\n> > > > @@ -1110,7 +1115,22 @@ void CameraDevice::requestComplete(Request *request)\n> > > > \tcaptureResult.frame_number = descriptor.frameNumber_;\n> > > > \tcaptureResult.num_output_buffers = descriptor.buffers_.size();\n> > > > \tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > > > -\t\tbuffer.acquire_fence = -1;\n> > > > +\t\tCameraStream *cameraStream =\n> > > > +\t\t\tstatic_cast(buffer.stream->priv);\n> > > > +\n> > > > +\t\t/*\n> > > > +\t\t * Streams of type Direct have been queued to the\n> > > > +\t\t * libcamera::Camera and their acquisition fences have\n> >\n> > s/acquisition/acquire/\n> >\n> > > > +\t\t * already been waited on by the CameraWorker.\n> > > > +\t\t *\n> > > > +\t\t * Acquire fences of streams of type Internal and Mapped\n> > > > +\t\t * will be handled during post-processing.\n> >\n> > Ditto here,\n> >\n> > \t\t * Acquire fences of Mapped streams will be handled during\n> > \t\t * post-processing.\n> >\n> > > > +\t\t *\n> > > > +\t\t * \\todo Instrument the CameraWorker to set the acquire\n> > > > +\t\t * fence to -1 once it has handled it and remove this check.\n> > > > +\t\t */\n> > > > +\t\tif (cameraStream->type() == CameraStream::Type::Direct)\n> > > > +\t\t\tbuffer.acquire_fence = -1;\n> > >\n> > > Yep, for Type::Direct they have  waited upon by worker already, so we\n> > > can set them to -1\n> > >\n> > > And release_fence for all buffers is to -1 as right below:\n> > >\n> > > > \t\tbuffer.release_fence = -1;\n> > > > \t\tbuffer.status = CAMERA3_BUFFER_STATUS_OK;\n> > > > \t}\n> > > > @@ -1130,8 +1150,17 @@ void CameraDevice::requestComplete(Request *request)\n> > > > \t\t\t CAMERA3_MSG_ERROR_REQUEST);\n> > > >\n> > > > \t\tcaptureResult.partial_result = 0;\n> > > > -\t\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_)\n> > > > +\t\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > > > +\t\t\t/*\n> > > > +\t\t\t * Signal to the framework it has to handle fences that\n> > > > +\t\t\t * have not been waited on by setting the release fence\n> > > > +\t\t\t * to the acquire fence value.\n> > > > +\t\t\t */\n> > > > +\t\t\tbuffer.release_fence = buffer.acquire_fence;\n> > > > +\t\t\tbuffer.acquire_fence = -1;\n> > > > \t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > > +\t\t}\n> > > > +\n> > >\n> > > Yep, that's what should be happening on error as per docs\n> > >\n> > > > \t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> > > >\n> > > > \t\treturn;\n> > > > @@ -1181,7 +1210,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..2363985398fb 100644\n> > > > --- a/src/android/camera_stream.cpp\n> > > > +++ b/src/android/camera_stream.cpp\n> > > > @@ -7,7 +7,11 @@\n> > > >\n> > > > #include \"camera_stream.h\"\n> > > >\n> > > > +#include \n> > > > +#include \n> > > > #include \n> > > > +#include \n> > > > +#include \n> > > >\n> > > > #include \n> > > >\n> > > > @@ -109,11 +113,53 @@ 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> >\n> > Could we name the parameter camera3Dest, dest, dst or destination ?\n> > Otherwise it's not clear in the implementation of the function which\n> > buffer \"camera3Buffer\" refers to.\n> \n> According to the badly enforced naming scheme in use in the HAL,\n> variable prefixed with camera3 are meant to represent object provided\n> by the camera service to the HAL. I can change this though, also\n> because it was the name originally in use.\n> \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> > >\n> > > Ah these are not Type::Direct, but Type::Mapped probably, so we need to\n> > > check the fences and wait upon here\n> \n> Mapped or Internal\n> \n> > > > +\t\t::close(fence);\n> > > > +\t\tcamera3Buffer.acquire_fence = -1;\n> > >\n> > > fence is closed (fixing the leak!) and acquire_fence = -1. Make sense.\n> > >\n> > > This is looking quite logical already but I have a question\n> > >\n> > > If I am seeing things correctly, I don't see a\n> > >\n> > >         buffer.release_fence = buffer.acquire_fence;\n> > >\n> > > in the post-processing error handling block. Should it be the case on\n> > > post-processing failure or I am missing something? My reason is we do\n> > > mark buffer as CAMERA3_BUFFER_STATUS_ERROR on post-processing failure,\n> > > so maybe we should do the right thing with fences too ? But not sure, I\n> > > have limited understanding in this aspect.\n> >\n> > We only need to do that if we haven't waited on the fence. If we wait\n> > but processing then fails, there's no need to move the acquire_fence to\n> > the release_fence.\n> \n> Yes, and we close the fence unconditionally, so we should not ask the\n> camera service to close it for us.\n> \n> > Reviewed-by: Laurent Pinchart \n> >\n> > > I have been testing this already with cts so,\n> > >\n> > > Tested-by: Umang Jain \n> > > Reviewed-by: Umang Jain \n> \n> Thank you both\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 +168,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..03ecfa94fc8c 100644\n> > > > --- a/src/android/camera_stream.h\n> > > > +++ b/src/android/camera_stream.h\n> > > > @@ -119,13 +119,15 @@ 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> > > > \tvoid putBuffer(libcamera::FrameBuffer *buffer);\n> > > >\n> > > > private:\n> > > > +\tint waitFence(int fence);\n> > > > +\n> > > > \tCameraDevice *const cameraDevice_;\n> > > > \tconst libcamera::CameraConfiguration *config_;\n> > > > \tconst Type type_;","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 07C98C3243\n\tfor ;\n\tWed, 29 Sep 2021 12:18:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 66F5B691AA;\n\tWed, 29 Sep 2021 14:18:08 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C423969185\n\tfor ;\n\tWed, 29 Sep 2021 14:18:06 +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 30B9D3F0;\n\tWed, 29 Sep 2021 14:18:06 +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=\"iyZ45a26\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632917886;\n\tbh=imgRR7Q3oQdhfZOZ/KGLIsjW7InC8hnQteOnRvdW52A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=iyZ45a26p8G8s0HYJ1/kusANcDtlo54/KOM0IkvmJib8hYibyqBf6tWhQCopmnEoi\n\tpSL1+jRSDpi+GvjOMpGlaYl0gw4np1GPC1ijFNVM+CeGNhVQmszQxGat9xCZPUPUAQ\n\tgdqK/z6l2dZJnO2AiG1X/WfOynIcNXGVPWw7CaxY=","Date":"Wed, 29 Sep 2021 15:18:04 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210928122515.28034-1-jacopo@jmondi.org>\n\t<20210928122515.28034-2-jacopo@jmondi.org>\n\t\n\t\n\t<20210929115954.52cvl3gihfzkq7zo@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210929115954.52cvl3gihfzkq7zo@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 1/1] 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\" "}}]