[{"id":17485,"web_url":"https://patchwork.libcamera.org/comment/17485/","msgid":"","date":"2021-06-08T20:42:48","subject":"Re: [libcamera-devel] [PATCH v5 8/8] android: Implement flush()\n\tcamera operation","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 Tue, Jun 08, 2021 at 05:16:33PM +0200, Jacopo Mondi wrote:\n> Implement the flush() camera operation in the CameraDevice class\n> and make it available to the camera framework by implementing the\n> operation wrapper in camera_ops.cpp.\n> \n> Introduce a new camera state State::Flushing to handle concurrent\n> flush() and process_capture_request() calls.\n> \n> As flush() can race with processCaptureRequest() protect it\n> by introducing a new State::Flushing state that\n> processCaptureRequest() inspects before queuing the Request to the\n> Camera. If flush() is in progress while processCaptureRequest() is\n> called, return the current Request immediately in error state. If\n> flush() has completed and a new call to processCaptureRequest() is\n> made just after, start the camera again before queuing the request.\n> \n> Signed-off-by: Jacopo Mondi \n> Reviewed-by: Hirokazu Honda \n> Reviewed-by: Niklas Söderlund \n> ---\n> src/android/camera_device.cpp | 77 +++++++++++++++++++++++++++--------\n> src/android/camera_device.h | 3 ++\n> src/android/camera_ops.cpp | 8 +++-\n> 3 files changed, 71 insertions(+), 17 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index b29c1edc9970..3adb657bfeca 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -798,6 +798,23 @@ void CameraDevice::close()\n> \tcamera_->release();\n> }\n> \n> +void CameraDevice::flush()\n> +{\n> +\t{\n> +\t\tMutexLocker stateLock(stateMutex_);\n> +\t\tif (state_ != State::Running)\n> +\t\t\treturn;\n> +\n> +\t\tstate_ = State::Flushing;\n> +\t}\n> +\n> +\tworker_.stop();\n> +\tcamera_->stop();\n> +\n> +\tMutexLocker stateLock(stateMutex_);\n> +\tstate_ = State::Stopped;\n> +}\n> +\n> void CameraDevice::stop()\n> {\n> \tMutexLocker stateLock(stateMutex_);\n> @@ -1896,27 +1913,31 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n> \treturn 0;\n> }\n> \n> -int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)\n> +void CameraDevice::abortRequest(camera3_capture_request_t *request)\n> {\n> -\tif (!isValidRequest(camera3Request))\n> -\t\treturn -EINVAL;\n> +\tnotifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n> \n> -\t{\n> -\t\tMutexLocker stateLock(stateMutex_);\n> +\tcamera3_capture_result_t result = {};\n> +\tresult.num_output_buffers = request->num_output_buffers;\n> +\tresult.frame_number = request->frame_number;\n> +\tresult.partial_result = 0;\n> \n> -\t\t/* Start the camera if that's the first request we handle. */\n> -\t\tif (state_ == State::Stopped) {\n> -\t\t\tworker_.start();\n> +\tstd::vector resultBuffers(result.num_output_buffers);\n> +\tfor (auto [i, buffer] : utils::enumerate(resultBuffers)) {\n> +\t\tbuffer = request->output_buffers[i];\n> +\t\tbuffer.release_fence = request->output_buffers[i].acquire_fence;\n> +\t\tbuffer.acquire_fence = -1;\n> +\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> +\t}\n> +\tresult.output_buffers = resultBuffers.data();\n> \n> -\t\t\tint ret = camera_->start();\n> -\t\t\tif (ret) {\n> -\t\t\t\tLOG(HAL, Error) << \"Failed to start camera\";\n> -\t\t\t\treturn ret;\n> -\t\t\t}\n> +\tcallbacks_->process_capture_result(callbacks_, &result);\n> +}\n> \n> -\t\t\tstate_ = State::Running;\n> -\t\t}\n> -\t}\n> +int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)\n> +{\n> +\tif (!isValidRequest(camera3Request))\n> +\t\treturn -EINVAL;\n> \n> \t/*\n> \t * Save the request descriptors for use at completion time.\n> @@ -2006,6 +2027,30 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \tif (ret)\n> \t\treturn ret;\n> \n> +\t/*\n> +\t * Just before queuing the request, make sure flush() has not\n> +\t * been called while this function was running. If flush is in progress\n> +\t * abort the request. If flush has completed and has stopped the camera\n> +\t * we have to re-start it to be able to process the request.\n> +\t */\n> +\tMutexLocker stateLock(stateMutex_);\n\nI'd add a blank line here.\n\nReviewed-by: Laurent Pinchart \n\n> +\tif (state_ == State::Flushing) {\n> +\t\tabortRequest(camera3Request);\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tif (state_ == State::Stopped) {\n> +\t\tworker_.start();\n> +\n> +\t\tret = camera_->start();\n> +\t\tif (ret) {\n> +\t\t\tLOG(HAL, Error) << \"Failed to start camera\";\n> +\t\t\treturn ret;\n> +\t\t}\n> +\n> +\t\tstate_ = State::Running;\n> +\t}\n> +\n> \tworker_.queueRequest(descriptor.request_.get());\n> \n> \t{\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index c949fa509ca4..4aadb27c562c 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -43,6 +43,7 @@ public:\n> \n> \tint open(const hw_module_t *hardwareModule);\n> \tvoid close();\n> +\tvoid flush();\n> \n> \tunsigned int id() const { return id_; }\n> \tcamera3_device_t *camera3Device() { return &camera3Device_; }\n> @@ -92,6 +93,7 @@ private:\n> \n> \tenum class State {\n> \t\tStopped,\n> +\t\tFlushing,\n> \t\tRunning,\n> \t};\n> \n> @@ -106,6 +108,7 @@ private:\n> \tgetRawResolutions(const libcamera::PixelFormat &pixelFormat);\n> \n> \tlibcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);\n> +\tvoid abortRequest(camera3_capture_request_t *request);\n> \tvoid notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> \tvoid notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n> \t\t\t camera3_error_msg_code code);\n> diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp\n> index 696e80436821..8a3cfa175ff5 100644\n> --- a/src/android/camera_ops.cpp\n> +++ b/src/android/camera_ops.cpp\n> @@ -66,8 +66,14 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev,\n> {\n> }\n> \n> -static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev)\n> +static int hal_dev_flush(const struct camera3_device *dev)\n> {\n> +\tif (!dev)\n> +\t\treturn -EINVAL;\n> +\n> +\tCameraDevice *camera = reinterpret_cast(dev->priv);\n> +\tcamera->flush();\n> +\n> \treturn 0;\n> }\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 59F29C3206\n\tfor ;\n\tTue, 8 Jun 2021 20:43:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A0A7C602C5;\n\tTue, 8 Jun 2021 22:43:11 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7EAC0602A7\n\tfor ;\n\tTue, 8 Jun 2021 22:43: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 071653E6;\n\tTue, 8 Jun 2021 22:43: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=\"hNzKBkSV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623184984;\n\tbh=FCgTB47TIPZSJ8V/tnr3MXyUl55s01rxKT7ordQ4ngU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hNzKBkSVc8xNsBQceQH6wZCrl8nAbM0wahj/4nNrjjPmP1P46pTmqDonZ22vU9FoQ\n\tskmKtaoMl7ys9ER5truGpYrbB6bxxaRFFzCMumnf+woKYI6t0+w8K/EvzBXiFgmfsv\n\tiISa9c7401IGEDmlAME6Cm2uvKIAOY32CiClYTaQ=","Date":"Tue, 8 Jun 2021 23:42:48 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210608151633.73465-1-jacopo@jmondi.org>\n\t<20210608151633.73465-9-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210608151633.73465-9-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v5 8/8] android: Implement flush()\n\tcamera operation","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":17509,"web_url":"https://patchwork.libcamera.org/comment/17509/","msgid":"<20210610162117.432tvqpq5p2wsmpd@uno.localdomain>","date":"2021-06-10T16:21:17","subject":"Re: [libcamera-devel] [PATCH v5 8/8] android: Implement flush()\n\tcamera operation","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Me and Laurent\n\nOn Tue, Jun 08, 2021 at 11:42:48PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Tue, Jun 08, 2021 at 05:16:33PM +0200, Jacopo Mondi wrote:\n> > Implement the flush() camera operation in the CameraDevice class\n> > and make it available to the camera framework by implementing the\n> > operation wrapper in camera_ops.cpp.\n> >\n> > Introduce a new camera state State::Flushing to handle concurrent\n> > flush() and process_capture_request() calls.\n> >\n> > As flush() can race with processCaptureRequest() protect it\n> > by introducing a new State::Flushing state that\n> > processCaptureRequest() inspects before queuing the Request to the\n> > Camera. If flush() is in progress while processCaptureRequest() is\n> > called, return the current Request immediately in error state. If\n> > flush() has completed and a new call to processCaptureRequest() is\n> > made just after, start the camera again before queuing the request.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > Reviewed-by: Hirokazu Honda \n> > Reviewed-by: Niklas Söderlund \n> > ---\n> > src/android/camera_device.cpp | 77 +++++++++++++++++++++++++++--------\n> > src/android/camera_device.h | 3 ++\n> > src/android/camera_ops.cpp | 8 +++-\n> > 3 files changed, 71 insertions(+), 17 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index b29c1edc9970..3adb657bfeca 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -798,6 +798,23 @@ void CameraDevice::close()\n> > \tcamera_->release();\n> > }\n> >\n> > +void CameraDevice::flush()\n> > +{\n> > +\t{\n> > +\t\tMutexLocker stateLock(stateMutex_);\n> > +\t\tif (state_ != State::Running)\n> > +\t\t\treturn;\n> > +\n> > +\t\tstate_ = State::Flushing;\n> > +\t}\n> > +\n> > +\tworker_.stop();\n> > +\tcamera_->stop();\n> > +\n> > +\tMutexLocker stateLock(stateMutex_);\n> > +\tstate_ = State::Stopped;\n> > +}\n> > +\n> > void CameraDevice::stop()\n> > {\n> > \tMutexLocker stateLock(stateMutex_);\n> > @@ -1896,27 +1913,31 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n> > \treturn 0;\n> > }\n> >\n> > -int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)\n> > +void CameraDevice::abortRequest(camera3_capture_request_t *request)\n> > {\n> > -\tif (!isValidRequest(camera3Request))\n> > -\t\treturn -EINVAL;\n> > +\tnotifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n> >\n> > -\t{\n> > -\t\tMutexLocker stateLock(stateMutex_);\n> > +\tcamera3_capture_result_t result = {};\n> > +\tresult.num_output_buffers = request->num_output_buffers;\n> > +\tresult.frame_number = request->frame_number;\n> > +\tresult.partial_result = 0;\n> >\n> > -\t\t/* Start the camera if that's the first request we handle. */\n> > -\t\tif (state_ == State::Stopped) {\n> > -\t\t\tworker_.start();\n> > +\tstd::vector resultBuffers(result.num_output_buffers);\n> > +\tfor (auto [i, buffer] : utils::enumerate(resultBuffers)) {\n> > +\t\tbuffer = request->output_buffers[i];\n> > +\t\tbuffer.release_fence = request->output_buffers[i].acquire_fence;\n> > +\t\tbuffer.acquire_fence = -1;\n> > +\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > +\t}\n> > +\tresult.output_buffers = resultBuffers.data();\n> >\n> > -\t\t\tint ret = camera_->start();\n> > -\t\t\tif (ret) {\n> > -\t\t\t\tLOG(HAL, Error) << \"Failed to start camera\";\n> > -\t\t\t\treturn ret;\n> > -\t\t\t}\n> > +\tcallbacks_->process_capture_result(callbacks_, &result);\n> > +}\n> >\n> > -\t\t\tstate_ = State::Running;\n> > -\t\t}\n> > -\t}\n> > +int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)\n> > +{\n> > +\tif (!isValidRequest(camera3Request))\n> > +\t\treturn -EINVAL;\n> >\n> > \t/*\n> > \t * Save the request descriptors for use at completion time.\n> > @@ -2006,6 +2027,30 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > \tif (ret)\n> > \t\treturn ret;\n> >\n> > +\t/*\n> > +\t * Just before queuing the request, make sure flush() has not\n> > +\t * been called while this function was running. If flush is in progress\n> > +\t * abort the request. If flush has completed and has stopped the camera\n> > +\t * we have to re-start it to be able to process the request.\n> > +\t */\n> > +\tMutexLocker stateLock(stateMutex_);\n>\n> I'd add a blank line here.\n\nI would also take the occasion to re-phrase the above comment to\n\n+ * If flush is in progress abort the request. If the camera has been\n+ * stopped we have to re-start it to be able to process the request.\n>\n> Reviewed-by: Laurent Pinchart \n>\n> > +\tif (state_ == State::Flushing) {\n> > +\t\tabortRequest(camera3Request);\n> > +\t\treturn 0;\n> > +\t}\n> > +\n> > +\tif (state_ == State::Stopped) {\n> > +\t\tworker_.start();\n> > +\n> > +\t\tret = camera_->start();\n> > +\t\tif (ret) {\n\nAnd stop the worker_ if we're returning here\n\nWith those I presume I can push the series\n\nThanks\n j\n\n> > +\t\t\tLOG(HAL, Error) << \"Failed to start camera\";\n> > +\t\t\treturn ret;\n> > +\t\t}\n> > +\n> > +\t\tstate_ = State::Running;\n> > +\t}\n> > +\n> > \tworker_.queueRequest(descriptor.request_.get());\n> >\n> > \t{\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index c949fa509ca4..4aadb27c562c 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -43,6 +43,7 @@ public:\n> >\n> > \tint open(const hw_module_t *hardwareModule);\n> > \tvoid close();\n> > +\tvoid flush();\n> >\n> > \tunsigned int id() const { return id_; }\n> > \tcamera3_device_t *camera3Device() { return &camera3Device_; }\n> > @@ -92,6 +93,7 @@ private:\n> >\n> > \tenum class State {\n> > \t\tStopped,\n> > +\t\tFlushing,\n> > \t\tRunning,\n> > \t};\n> >\n> > @@ -106,6 +108,7 @@ private:\n> > \tgetRawResolutions(const libcamera::PixelFormat &pixelFormat);\n> >\n> > \tlibcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);\n> > +\tvoid abortRequest(camera3_capture_request_t *request);\n> > \tvoid notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> > \tvoid notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n> > \t\t\t camera3_error_msg_code code);\n> > diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp\n> > index 696e80436821..8a3cfa175ff5 100644\n> > --- a/src/android/camera_ops.cpp\n> > +++ b/src/android/camera_ops.cpp\n> > @@ -66,8 +66,14 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev,\n> > {\n> > }\n> >\n> > -static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev)\n> > +static int hal_dev_flush(const struct camera3_device *dev)\n> > {\n> > +\tif (!dev)\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tCameraDevice *camera = reinterpret_cast(dev->priv);\n> > +\tcamera->flush();\n> > +\n> > \treturn 0;\n> > }\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 57BCBC320B\n\tfor ;\n\tThu, 10 Jun 2021 16:20:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ADA786892E;\n\tThu, 10 Jun 2021 18:20:30 +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 F30CD6029B\n\tfor ;\n\tThu, 10 Jun 2021 18:20:28 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 5FDDC1BF20B;\n\tThu, 10 Jun 2021 16:20:28 +0000 (UTC)"],"Date":"Thu, 10 Jun 2021 18:21:17 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210610162117.432tvqpq5p2wsmpd@uno.localdomain>","References":"<20210608151633.73465-1-jacopo@jmondi.org>\n\t<20210608151633.73465-9-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 v5 8/8] android: Implement flush()\n\tcamera operation","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\" "}}]