[{"id":2245,"web_url":"https://patchwork.libcamera.org/comment/2245/","msgid":"<20190714070652.GA31085@wyvern>","date":"2019-07-14T07:06:52","subject":"Re: [libcamera-devel] [PATCH v2 03/16] libcamera: pipeline_handler:\n\tSimplify request completion","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your work.\n\nOn 2019-07-13 20:23:38 +0300, Laurent Pinchart wrote:\n> libcamera guarantees that requests complete in sequence. This\n> requirement is currently pushed down to pipeline handlers. Three out of\n> four of our pipeline handlers implement that requirement based on the\n> sole assumption that buffers will always complete in sequeuence, while\n> the IPU3 pipeline handler implements a more complex logic.\n> \n> It turns out that the logic can be moved to the base PipelineHandler\n> class with support from the Request class. Do so to simplify the\n> pipeline handlers.\n> \n> Signed-off-by: Laurent Pinchart \n\nReviewed-by: Niklas Söderlund \n\n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 10 ++-------\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 +---\n> src/libcamera/pipeline/uvcvideo.cpp | 2 +-\n> src/libcamera/pipeline/vimc.cpp | 2 +-\n> src/libcamera/pipeline_handler.cpp | 26 ++++++++++++++++--------\n> 5 files changed, 22 insertions(+), 22 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index e4efb9722f76..1abd20e5fee5 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -927,14 +927,8 @@ void IPU3CameraData::imguOutputBufferReady(Buffer *buffer)\n> \t\t/* Request not completed yet, return here. */\n> \t\treturn;\n> \n> -\t/* Complete the pending requests in queueing order. */\n> -\twhile (1) {\n> -\t\trequest = queuedRequests_.front();\n> -\t\tif (request->hasPendingBuffers())\n> -\t\t\tbreak;\n> -\n> -\t\tpipe_->completeRequest(camera_, request);\n> -\t}\n> +\t/* Mark the request as complete. */\n> +\tpipe_->completeRequest(camera_, request);\n> }\n> \n> /**\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 4a5898d25f91..383236435a7f 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -491,9 +491,7 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)\n> {\n> \tASSERT(activeCamera_);\n> -\n> -\tRkISP1CameraData *data = cameraData(activeCamera_);\n> -\tRequest *request = data->queuedRequests_.front();\n> +\tRequest *request = buffer->request();\n> \n> \tcompleteBuffer(activeCamera_, request, buffer);\n> \tcompleteRequest(activeCamera_, request);\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index 72313cfd246f..387d71ef567c 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -382,7 +382,7 @@ int UVCCameraData::init(MediaEntity *entity)\n> \n> void UVCCameraData::bufferReady(Buffer *buffer)\n> {\n> -\tRequest *request = queuedRequests_.front();\n> +\tRequest *request = buffer->request();\n> \n> \tpipe_->completeBuffer(camera_, request, buffer);\n> \tpipe_->completeRequest(camera_, request);\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index f8a58be060bb..ff2529c974fd 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -376,7 +376,7 @@ int VimcCameraData::init(MediaDevice *media)\n> \n> void VimcCameraData::bufferReady(Buffer *buffer)\n> {\n> -\tRequest *request = queuedRequests_.front();\n> +\tRequest *request = buffer->request();\n> \n> \tpipe_->completeBuffer(camera_, request, buffer);\n> \tpipe_->completeRequest(camera_, request);\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 0283e4e5ad51..c609200252f1 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -391,8 +391,9 @@ int PipelineHandler::queueRequest(Camera *camera, Request *request)\n> * This method shall be called by pipeline handlers to signal completion of the\n> * \\a buffer part of the \\a request. It notifies applications of buffer\n> * completion and updates the request's internal buffer tracking. The request\n> - * is not completed automatically when the last buffer completes, pipeline\n> - * handlers shall complete requests explicitly with completeRequest().\n> + * is not completed automatically when the last buffer completes to give\n> + * pipeline handlers a chance to perform any operation that may still be\n> + * needed. They shall complete requests explicitly with completeRequest().\n> *\n> * \\return True if all buffers contained in the request have completed, false\n> * otherwise\n> @@ -413,17 +414,24 @@ bool PipelineHandler::completeBuffer(Camera *camera, Request *request,\n> * request has completed. The request is deleted and shall not be accessed once\n> * this method returns.\n> *\n> - * The pipeline handler shall ensure that requests complete in the same order\n> - * they are submitted.\n> + * This method ensures that requests will be returned to the application in\n> + * submission order, the pipeline handler may call it on any complete request\n> + * without any ordering constraint.\n> */\n> void PipelineHandler::completeRequest(Camera *camera, Request *request)\n> {\n> -\tCameraData *data = cameraData(camera);\n> -\tASSERT(request == data->queuedRequests_.front());\n> -\tdata->queuedRequests_.pop_front();\n> -\n> \trequest->complete(Request::RequestComplete);\n> -\tcamera->requestComplete(request);\n> +\n> +\tCameraData *data = cameraData(camera);\n> +\n> +\twhile (!data->queuedRequests_.empty()) {\n> +\t\trequest = data->queuedRequests_.front();\n> +\t\tif (request->hasPendingBuffers())\n> +\t\t\tbreak;\n> +\n> +\t\tdata->queuedRequests_.pop_front();\n> +\t\tcamera->requestComplete(request);\n> +\t}\n> }\n> \n> /**\n> -- \n> Regards,\n> \n> Laurent Pinchart\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-pg1-x543.google.com (mail-pg1-x543.google.com\n\t[IPv6:2607:f8b0:4864:20::543])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8AD4660E3F\n\tfor ;\n\tSun, 14 Jul 2019 09:06:57 +0200 (CEST)","by mail-pg1-x543.google.com with SMTP id w10so6291246pgj.7\n\tfor ;\n\tSun, 14 Jul 2019 00:06:57 -0700 (PDT)","from localhost (softbank126159224182.bbtec.net. [126.159.224.182])\n\tby smtp.gmail.com with ESMTPSA id\n\ts22sm13995139pfh.107.2019.07.14.00.06.54\n\t(version=TLS1_3 cipher=AEAD-AES256-GCM-SHA384 bits=256/256);\n\tSun, 14 Jul 2019 00:06:55 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=Qs0PxaEqDMeulWyP3q3ZaYeIUbDVcCQ7R10lUE4H8wQ=;\n\tb=Howxb0llzyj0y6xHvax9XWMEMtZOBCcg4/NtlhUNTdfDzrF1Xj6P+qenrRbNTLXLC6\n\t8a9/suNXy6y5c8BXu94exFADkQY1ijtTm0cArKgt+w6kSZxp7khKd7yK1vhijY48cRqS\n\tp/XKZSyKWo9fCKgapXAq5lK1xhclK/iyuUsVMeILh5RW7yEliyu5GM/tzcLHRkc39XOu\n\tSF7FfreWhTAw2tgA6KoRYyU7KVNsVugPTh8KnRFTxjhV+WVdSa5/X1VyupFqNCGvsRT+\n\thkvy9xHUr0I1ZTbd9MbGi9z2Sb6AWBeDnCh7NJR1w6vYaskZlDuqtjmODzczQ52rl+NR\n\tvz5Q==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=Qs0PxaEqDMeulWyP3q3ZaYeIUbDVcCQ7R10lUE4H8wQ=;\n\tb=Ivl3OrRT4Pa68yVT6Mp2wnPeSIWYc7ot0DnLn+Pgrdc0rZcQnqaYyDZQjbmSyTM2Ya\n\tglFNiQtlQC8yCIQvykyu5E30a0vWjgi9nseCGIdkm70zQhH+EVI4p9c+uub8t/ChxlS9\n\tx3oIrZqQ2HC6bDXjpWjqiLLx2epOqBqT1JRtDOxAW2Xiz0V7zNMzd9jGUTEUq9o7LR76\n\t0wwRdhcG323YadutpFnnGXIRoCGpt4PPb9GPpA9aREGiSZEDONAXMHgVgBbwRQQCN8US\n\t/VkHhmqPbc5fiUW9KoVD4cOfAer4IeTyGqo4meo1DCJ6JDQX/7hnCZi+UrPgS6nQ/5aA\n\tjgeA==","X-Gm-Message-State":"APjAAAU4iiV4a1tola4xIqxGY5mVnr4KzpnmqH+9/OEY4OfzGSN9DUGF\n\thi7/05/2CWyoFotNJYlQYIE=","X-Google-Smtp-Source":"APXvYqzOY7cd6hxJbjGnUVIWO9HTZoTktXobnxFNOvcCLmZ29tyAbXy8szBD6q6Ss/czibmP3QbY8g==","X-Received":"by 2002:a65:52c5:: with SMTP id z5mr5953407pgp.118.1563088016151;\n\tSun, 14 Jul 2019 00:06:56 -0700 (PDT)","Date":"Sun, 14 Jul 2019 16:06:52 +0900","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190714070652.GA31085@wyvern>","References":"<20190713172351.25452-1-laurent.pinchart@ideasonboard.com>\n\t<20190713172351.25452-4-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190713172351.25452-4-laurent.pinchart@ideasonboard.com>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH v2 03/16] libcamera: pipeline_handler:\n\tSimplify request completion","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sun, 14 Jul 2019 07:06:58 -0000"}},{"id":2251,"web_url":"https://patchwork.libcamera.org/comment/2251/","msgid":"<20190714073027.ppmkmz24gdo4olmw@uno.localdomain>","date":"2019-07-14T07:30:27","subject":"Re: [libcamera-devel] [PATCH v2 03/16] libcamera: pipeline_handler:\n\tSimplify request completion","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"HI Laurent,\n\nOn Sat, Jul 13, 2019 at 08:23:38PM +0300, Laurent Pinchart wrote:\n> libcamera guarantees that requests complete in sequence. This\n> requirement is currently pushed down to pipeline handlers. Three out of\n> four of our pipeline handlers implement that requirement based on the\n> sole assumption that buffers will always complete in sequeuence, while\n> the IPU3 pipeline handler implements a more complex logic.\n>\n> It turns out that the logic can be moved to the base PipelineHandler\n> class with support from the Request class. Do so to simplify the\n> pipeline handlers.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 10 ++-------\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 +---\n> src/libcamera/pipeline/uvcvideo.cpp | 2 +-\n> src/libcamera/pipeline/vimc.cpp | 2 +-\n> src/libcamera/pipeline_handler.cpp | 26 ++++++++++++++++--------\n> 5 files changed, 22 insertions(+), 22 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index e4efb9722f76..1abd20e5fee5 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -927,14 +927,8 @@ void IPU3CameraData::imguOutputBufferReady(Buffer *buffer)\n> \t\t/* Request not completed yet, return here. */\n> \t\treturn;\n>\n> -\t/* Complete the pending requests in queueing order. */\n> -\twhile (1) {\n> -\t\trequest = queuedRequests_.front();\n> -\t\tif (request->hasPendingBuffers())\n> -\t\t\tbreak;\n> -\n> -\t\tpipe_->completeRequest(camera_, request);\n> -\t}\n> +\t/* Mark the request as complete. */\n> +\tpipe_->completeRequest(camera_, request);\n> }\n>\n> /**\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 4a5898d25f91..383236435a7f 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -491,9 +491,7 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)\n> {\n> \tASSERT(activeCamera_);\n> -\n> -\tRkISP1CameraData *data = cameraData(activeCamera_);\n> -\tRequest *request = data->queuedRequests_.front();\n> +\tRequest *request = buffer->request();\n>\n> \tcompleteBuffer(activeCamera_, request, buffer);\n> \tcompleteRequest(activeCamera_, request);\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index 72313cfd246f..387d71ef567c 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -382,7 +382,7 @@ int UVCCameraData::init(MediaEntity *entity)\n>\n> void UVCCameraData::bufferReady(Buffer *buffer)\n> {\n> -\tRequest *request = queuedRequests_.front();\n> +\tRequest *request = buffer->request();\n>\n> \tpipe_->completeBuffer(camera_, request, buffer);\n> \tpipe_->completeRequest(camera_, request);\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index f8a58be060bb..ff2529c974fd 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -376,7 +376,7 @@ int VimcCameraData::init(MediaDevice *media)\n>\n> void VimcCameraData::bufferReady(Buffer *buffer)\n> {\n> -\tRequest *request = queuedRequests_.front();\n> +\tRequest *request = buffer->request();\n>\n> \tpipe_->completeBuffer(camera_, request, buffer);\n> \tpipe_->completeRequest(camera_, request);\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 0283e4e5ad51..c609200252f1 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -391,8 +391,9 @@ int PipelineHandler::queueRequest(Camera *camera, Request *request)\n> * This method shall be called by pipeline handlers to signal completion of the\n> * \\a buffer part of the \\a request. It notifies applications of buffer\n> * completion and updates the request's internal buffer tracking. The request\n> - * is not completed automatically when the last buffer completes, pipeline\n> - * handlers shall complete requests explicitly with completeRequest().\n> + * is not completed automatically when the last buffer completes to give\n> + * pipeline handlers a chance to perform any operation that may still be\n> + * needed. They shall complete requests explicitly with completeRequest().\n> *\n> * \\return True if all buffers contained in the request have completed, false\n> * otherwise\n> @@ -413,17 +414,24 @@ bool PipelineHandler::completeBuffer(Camera *camera, Request *request,\n> * request has completed. The request is deleted and shall not be accessed once\n> * this method returns.\n> *\n> - * The pipeline handler shall ensure that requests complete in the same order\n> - * they are submitted.\n> + * This method ensures that requests will be returned to the application in\n> + * submission order, the pipeline handler may call it on any complete request\n> + * without any ordering constraint.\n> */\n> void PipelineHandler::completeRequest(Camera *camera, Request *request)\n> {\n> -\tCameraData *data = cameraData(camera);\n> -\tASSERT(request == data->queuedRequests_.front());\n> -\tdata->queuedRequests_.pop_front();\n> -\n> \trequest->complete(Request::RequestComplete);\n> -\tcamera->requestComplete(request);\n> +\n\nNit: empty line\n\n> +\tCameraData *data = cameraData(camera);\n> +\n> +\twhile (!data->queuedRequests_.empty()) {\n> +\t\trequest = data->queuedRequests_.front();\n> +\t\tif (request->hasPendingBuffers())\n> +\t\t\tbreak;\n> +\n> +\t\tdata->queuedRequests_.pop_front();\n> +\t\tcamera->requestComplete(request);\n> +\t}\n> }\n>\n> /**\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 209B860E3F\n\tfor ;\n\tSun, 14 Jul 2019 09:29:50 +0200 (CEST)","from uno.localdomain (softbank126159224182.bbtec.net\n\t[126.159.224.182]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 3498C240007;\n\tSun, 14 Jul 2019 07:29:11 +0000 (UTC)"],"X-Originating-IP":"126.159.224.182","Date":"Sun, 14 Jul 2019 09:30:27 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190714073027.ppmkmz24gdo4olmw@uno.localdomain>","References":"<20190713172351.25452-1-laurent.pinchart@ideasonboard.com>\n\t<20190713172351.25452-4-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"qhijmwhq6kzzza6x\"","Content-Disposition":"inline","In-Reply-To":"<20190713172351.25452-4-laurent.pinchart@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 03/16] libcamera: pipeline_handler:\n\tSimplify request completion","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sun, 14 Jul 2019 07:29:50 -0000"}}]