[{"id":1280,"web_url":"https://patchwork.libcamera.org/comment/1280/","msgid":"<20190405114835.GB23466@bigcity.dyn.berto.se>","date":"2019-04-05T11:48:35","subject":"Re: [libcamera-devel] [PATCH v3 7/8] libcamera: pipeline: Add\n\tmethod to retrieve Request from Buffer","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2019-04-03 17:07:34 +0200, Jacopo Mondi wrote:\n> Add a method to CameraData base class to retrieve a pointer to the\n> Request that contains a given buffer. Intended users are buffer\n> completion slots that needs to associate a Request to a just completed\n> Buffer.\n> \n> In preparation to support multiple requests from different streams,\n> update all the pipeline handler implementations to use this method\n> instead of using the last queued request.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/request.h | 2 ++\n> src/libcamera/include/pipeline_handler.h | 3 +++\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 3 ++-\n> src/libcamera/pipeline/uvcvideo.cpp | 3 ++-\n> src/libcamera/pipeline/vimc.cpp | 3 ++-\n> src/libcamera/pipeline_handler.cpp | 29 ++++++++++++++++++++++++\n> src/libcamera/request.cpp | 7 ++++++\n> 7 files changed, 47 insertions(+), 3 deletions(-)\n> \n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index 5ac4d20d1d9f..8f5892fd3111 100644\n> --- a/include/libcamera/request.h\n> +++ b/include/libcamera/request.h\n> @@ -38,6 +38,8 @@ public:\n> \n> \tconst std::list streams() const;\n> \n> +\tconst std::unordered_set &pending() const { return pending_; }\n> +\n> \tStatus status() const { return status_; }\n> \n> private:\n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index 920b57609470..6cdadcbdc3ea 100644\n> --- a/src/libcamera/include/pipeline_handler.h\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -39,6 +39,9 @@ public:\n> \tPipelineHandler *pipe_;\n> \tstd::list queuedRequests_;\n> \n> +protected:\n> +\tRequest *requestFromBuffer(Buffer *buffer);\n> +\n> private:\n> \tCameraData(const CameraData &) = delete;\n> \tCameraData &operator=(const CameraData &) = delete;\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 8c67cf985d1e..17e3e8677e28 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -801,7 +801,8 @@ void PipelineHandlerIPU3::IPU3CameraData::imguInputBufferReady(Buffer *buffer)\n> */\n> void PipelineHandlerIPU3::IPU3CameraData::imguOutputBufferReady(Buffer *buffer)\n> {\n> -\tRequest *request = queuedRequests_.front();\n> +\tRequest *request = requestFromBuffer(buffer);\n> +\tASSERT(request);\n> \n> \tpipe_->completeBuffer(camera_, request, buffer);\n> \tpipe_->completeRequest(camera_, request);\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index 128f0c49dba3..d571b8b4ea83 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -226,7 +226,8 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n> \n> void PipelineHandlerUVC::UVCCameraData::bufferReady(Buffer *buffer)\n> {\n> -\tRequest *request = queuedRequests_.front();\n> +\tRequest *request = requestFromBuffer(buffer);\n\nThis make me feel uneasy :-(\n\nWe talked in the past about how to handle request completion. The idea \nfrom the beginning was conceived with the reequest API in mind, which \nIIRC guarantees that requests are completed in the same order they are \nqueued. While V4L2 have no such guarantees for for buffers. This was one \nof the issues I hit with my single stream old UVC camera where the first \nbuffer queue would contains errors and the uvcvideo driver would not \nreturn it to user-space and move on to the second buffer queued and \nlibcamera fail as buffers are dequeued out of order (from it's point of \nview).\n\nI understand what you try to do in this patch and I agree it might be \nneeded. But I fear we first need to discuss how we should handle V4L2 \nbuffers being dequeued out of order. As this change would hide this \nbehavior while it's intent is indeed something different.\n\n> +\tASSERT(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 6735940799d8..e83416effad8 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -223,7 +223,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)\n> \n> void PipelineHandlerVimc::VimcCameraData::bufferReady(Buffer *buffer)\n> {\n> -\tRequest *request = queuedRequests_.front();\n> +\tRequest *request = requestFromBuffer(buffer);\n> +\tASSERT(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 9a8a4fde57e6..830ff354ed3e 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -86,6 +86,35 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> * PipelineHandler::completeRequest()\n> */\n> \n> +/**\n> + * \\brief Retrieve the pending request that contains \\a buffer\n> + * \\param[in] buffer The buffer contained in the returned request\n> + *\n> + * Return the request that contains \\a buffer, or nullptr if no such request\n> + * is found. The intended callers of this method are buffer completion slots\n> + * implemented in CameraData sub-classes which needs to associated a request\n> + * to the just completed buffer. It is up to the caller of this function to\n> + * deal with the case the buffer does not belong to any previously queued\n> + * request or the request has already completed, possibly because of a\n> + * duplicated buffer completion notification. This is generally considered\n> + * a fatal error, and callers are expected to assert the validity of the\n> + * returned request.\n> + *\n> + * \\return A pointer to the pending Request that contains the Buffer \\a buffer,\n> + * or nullptr if no such request is found\n> + */\n> +Request *CameraData::requestFromBuffer(Buffer *buffer)\n> +{\n> +\tfor (Request *req : queuedRequests_) {\n> +\t\tfor (Buffer *b : req->pending()) {\n> +\t\t\tif (b == buffer)\n> +\t\t\t\treturn req;\n> +\t\t}\n> +\t}\n> +\n> +\treturn nullptr;\n> +}\n> +\n> /**\n> * \\class PipelineHandler\n> * \\brief Create and manage cameras based on a set of media devices\n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index 3a7841fb2bb3..c555752b2c0b 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -108,6 +108,13 @@ const std::list Request::streams() const\n> \treturn streams;\n> }\n> \n> +/**\n> + * \\fn Request::pending()\n> + * \\brief Retrieve the list of not-yet-completed buffers\n> + *\n> + * \\return The set of pending buffers\n> + */\n> +\n> /**\n> * \\fn Request::status()\n> * \\brief Retrieve the request completion status\n> -- \n> 2.21.0\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-lf1-f49.google.com (mail-lf1-f49.google.com\n\t[209.85.167.49])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6CF4B60DB3\n\tfor ;\n\tFri, 5 Apr 2019 13:49:38 +0200 (CEST)","by mail-lf1-f49.google.com with SMTP id v24so1175531lfe.9\n\tfor ;\n\tFri, 05 Apr 2019 04:49:38 -0700 (PDT)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\to17sm4675965ljc.66.2019.04.05.04.48.35\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tFri, 05 Apr 2019 04:48:35 -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=oX3qIrK116Ly9yL6/VYjAaewi1hiJp8d24URElSCuqE=;\n\tb=GMTwuj4wZ1nn7/25DLdO2qM2jbqIql8GzU4VzCpIRmB+F3y7wpTo6Rh7Jt3QdDDqzr\n\tFBE4hSnOK+G5ab+vhp+6ieB4Q8oBy4XJeJlOi80d3AnpWa3g75D/2C/WVdtaO81jqaIa\n\tx9FxVOY3PReZoW3vIT3h/60uv4OvYmrFLqGmXDXlkVKIEdkjIDQYXMoKJvu/Kf7O9gyE\n\tL9ajI4+vEkPPFLWYPrHP24ZHcv1g5F38zFtSH1GzohZQk8OOfSmqqlGIxczlhaNo7Z6O\n\t9M5jnBcBX7btWGqNXoPuEIT4ZoygOepX7arXuIZDUYhMgfRIO53HOfJZHkPrUu92qlmV\n\tuJow==","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=oX3qIrK116Ly9yL6/VYjAaewi1hiJp8d24URElSCuqE=;\n\tb=abUPYQYFoc0D4VXxEA/p12AHfrvjHIeBAUYRPABy7p/b86YzZeP0GwkdWHDOdcLC/8\n\tRKlvAObCGgp8CcJzrpKIjgqW5eLrvUnxoAW/8hc3RZDc/Aro0muj94YAoAX5fZu9g4E1\n\tczR8ujwnM1sDzKWtYaNij4oZQQnYpDFOBG2kZVEumQv8qJbe3zpSUiilhtVj+QXnnvoJ\n\tg62XJa6RbRKusjk+0iKNQ7tPbByV7uuMjRbogZHndPqHKq9IMOev1pG7hzdSR/aGDd3t\n\txEDWMjLqiNFOYyo9fc/CgaPtfkBi/j9A7YikGHBh9Ux7Jyf+DkudZjlLzObkiD0ZKoti\n\tgY0Q==","X-Gm-Message-State":"APjAAAUyredqbwSfldHWMOfbqCEaHazbU6Ioyg8sB44JCZNFc1KPzfpa\n\tt5NAXRZAoSwvIRSHsrmt5Rh0EA==","X-Google-Smtp-Source":"APXvYqxhXtPkWjPhn1ByTz6lTjl9c+kwIYO0Or/dfCXthmoTC1lQhhN/DvYtbZInHXCuvf2vD1a8DA==","X-Received":"by 2002:a19:f618:: with SMTP id\n\tx24mr1413939lfe.134.1554464916751; \n\tFri, 05 Apr 2019 04:48:36 -0700 (PDT)","Date":"Fri, 5 Apr 2019 13:48:35 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190405114835.GB23466@bigcity.dyn.berto.se>","References":"<20190403150735.27580-1-jacopo@jmondi.org>\n\t<20190403150735.27580-8-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190403150735.27580-8-jacopo@jmondi.org>","User-Agent":"Mutt/1.11.3 (2019-02-01)","Subject":"Re: [libcamera-devel] [PATCH v3 7/8] libcamera: pipeline: Add\n\tmethod to retrieve Request from Buffer","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":"Fri, 05 Apr 2019 11:49:38 -0000"}},{"id":1281,"web_url":"https://patchwork.libcamera.org/comment/1281/","msgid":"<20190405115223.GC23466@bigcity.dyn.berto.se>","date":"2019-04-05T11:52:23","subject":"Re: [libcamera-devel] [PATCH v3 7/8] libcamera: pipeline: Add\n\tmethod to retrieve Request from Buffer","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi again,\n\nOn 2019-04-03 17:07:34 +0200, Jacopo Mondi wrote:\n> Add a method to CameraData base class to retrieve a pointer to the\n> Request that contains a given buffer. Intended users are buffer\n> completion slots that needs to associate a Request to a just completed\n> Buffer.\n> \n> In preparation to support multiple requests from different streams,\n> update all the pipeline handler implementations to use this method\n> instead of using the last queued request.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/request.h | 2 ++\n> src/libcamera/include/pipeline_handler.h | 3 +++\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 3 ++-\n> src/libcamera/pipeline/uvcvideo.cpp | 3 ++-\n> src/libcamera/pipeline/vimc.cpp | 3 ++-\n> src/libcamera/pipeline_handler.cpp | 29 ++++++++++++++++++++++++\n> src/libcamera/request.cpp | 7 ++++++\n> 7 files changed, 47 insertions(+), 3 deletions(-)\n> \n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index 5ac4d20d1d9f..8f5892fd3111 100644\n> --- a/include/libcamera/request.h\n> +++ b/include/libcamera/request.h\n> @@ -38,6 +38,8 @@ public:\n> \n> \tconst std::list streams() const;\n> \n> +\tconst std::unordered_set &pending() const { return pending_; }\n> +\n\nYou could break adding of this function out to a separate patch or merge \nit with the one adding streams() earlier in the series.\n\n> \tStatus status() const { return status_; }\n> \n> private:\n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index 920b57609470..6cdadcbdc3ea 100644\n> --- a/src/libcamera/include/pipeline_handler.h\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -39,6 +39,9 @@ public:\n> \tPipelineHandler *pipe_;\n> \tstd::list queuedRequests_;\n> \n> +protected:\n> +\tRequest *requestFromBuffer(Buffer *buffer);\n> +\n> private:\n> \tCameraData(const CameraData &) = delete;\n> \tCameraData &operator=(const CameraData &) = delete;\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 8c67cf985d1e..17e3e8677e28 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -801,7 +801,8 @@ void PipelineHandlerIPU3::IPU3CameraData::imguInputBufferReady(Buffer *buffer)\n> */\n> void PipelineHandlerIPU3::IPU3CameraData::imguOutputBufferReady(Buffer *buffer)\n> {\n> -\tRequest *request = queuedRequests_.front();\n> +\tRequest *request = requestFromBuffer(buffer);\n> +\tASSERT(request);\n> \n> \tpipe_->completeBuffer(camera_, request, buffer);\n> \tpipe_->completeRequest(camera_, request);\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index 128f0c49dba3..d571b8b4ea83 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -226,7 +226,8 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n> \n> void PipelineHandlerUVC::UVCCameraData::bufferReady(Buffer *buffer)\n> {\n> -\tRequest *request = queuedRequests_.front();\n> +\tRequest *request = requestFromBuffer(buffer);\n> +\tASSERT(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 6735940799d8..e83416effad8 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -223,7 +223,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)\n> \n> void PipelineHandlerVimc::VimcCameraData::bufferReady(Buffer *buffer)\n> {\n> -\tRequest *request = queuedRequests_.front();\n> +\tRequest *request = requestFromBuffer(buffer);\n> +\tASSERT(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 9a8a4fde57e6..830ff354ed3e 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -86,6 +86,35 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> * PipelineHandler::completeRequest()\n> */\n> \n> +/**\n> + * \\brief Retrieve the pending request that contains \\a buffer\n> + * \\param[in] buffer The buffer contained in the returned request\n> + *\n> + * Return the request that contains \\a buffer, or nullptr if no such request\n> + * is found. The intended callers of this method are buffer completion slots\n> + * implemented in CameraData sub-classes which needs to associated a request\n> + * to the just completed buffer. It is up to the caller of this function to\n> + * deal with the case the buffer does not belong to any previously queued\n> + * request or the request has already completed, possibly because of a\n> + * duplicated buffer completion notification. This is generally considered\n> + * a fatal error, and callers are expected to assert the validity of the\n> + * returned request.\n> + *\n> + * \\return A pointer to the pending Request that contains the Buffer \\a buffer,\n> + * or nullptr if no such request is found\n> + */\n> +Request *CameraData::requestFromBuffer(Buffer *buffer)\n> +{\n> +\tfor (Request *req : queuedRequests_) {\n> +\t\tfor (Buffer *b : req->pending()) {\n> +\t\t\tif (b == buffer)\n> +\t\t\t\treturn req;\n> +\t\t}\n> +\t}\n> +\n> +\treturn nullptr;\n> +}\n> +\n> /**\n> * \\class PipelineHandler\n> * \\brief Create and manage cameras based on a set of media devices\n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index 3a7841fb2bb3..c555752b2c0b 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -108,6 +108,13 @@ const std::list Request::streams() const\n> \treturn streams;\n> }\n> \n> +/**\n> + * \\fn Request::pending()\n> + * \\brief Retrieve the list of not-yet-completed buffers\n> + *\n> + * \\return The set of pending buffers\n> + */\n> +\n> /**\n> * \\fn Request::status()\n> * \\brief Retrieve the request completion status\n> -- \n> 2.21.0\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-lj1-f171.google.com (mail-lj1-f171.google.com\n\t[209.85.208.171])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 587AE60DB3\n\tfor ;\n\tFri, 5 Apr 2019 13:53:26 +0200 (CEST)","by mail-lj1-f171.google.com with SMTP id q66so4991994ljq.7\n\tfor ;\n\tFri, 05 Apr 2019 04:53:26 -0700 (PDT)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tv11sm4676267ljk.19.2019.04.05.04.52.24\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tFri, 05 Apr 2019 04:52:24 -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=qXK16mqkpTPquFiwKQEjUZ0u9BnWRanGk4h3mSFPehg=;\n\tb=gud9qz+J+zIvkuHO53sxZuqSk7q1B/z9H5brMKX6pFuIihNChqJM888MAqd4GGOcng\n\tIJ/s8/jiyXBAYB2ThXz1p6vtneWg9FPUm2ct6qQeNRT3M927BG3fPCRCzLBCeazu2hWC\n\thnj/847dyJV0CpjxLRF9ZX0QLO9XYDknUxoGGhcmd0ilV2TZ6XU3V3Wd1v8TLj9VuA7J\n\tdGdayYcuqhcMsCwjltbA3RcCLkoKEWREwXI2IAFfd+2mB+A4lhp6DeT5IO1RyyDstoVk\n\tHacU0qAweaLKivKkHsCbX82Io49DlaACtGxOuJ9xqbe0OYPXpFgcG64xt9bc4RsITl0v\n\tAElQ==","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=qXK16mqkpTPquFiwKQEjUZ0u9BnWRanGk4h3mSFPehg=;\n\tb=mzEIO0Nnlk2n4l48yEq03usPWFZuSbOdjRVOgTFt+hXquGzevOZYJC6HtfSvzl5Ibg\n\tHyFK3JnqnzMQbBnM1ZclSTK0MLdsfnb4XIiAaorIORE9AhXPw1y/PnngGD7DgfEKkVIq\n\tJ7Oc05GJolqLclWadgr7pcUWj6yDijMgrAlI+yvHYtmVkRWcw+N/twj7ogK5pMJBZaFL\n\t6t/YpyHLQLjMfAhoF/rh1ApXFWD5xGnlGO2gyLJ57/PGBOuRI4G5YL6voL1rzAEG8QAx\n\tK1If95OUah50ArcxoXbVWwhBMHpoq9GoblRkSvD4e4ibHRaJOwS0swUqNd67F0C8V6F5\n\tVJxg==","X-Gm-Message-State":"APjAAAU55jhht+i5XDJsa8SyFGmyyzVav9EvvCYcjJwKFv514y5OEFei\n\tmpHJZVeuLQg6pHsedcp72zTCIw==","X-Google-Smtp-Source":"APXvYqwNCDbjFzZDcysk2/PHkh+TJe9PLdldcYTsfvvqtHgtycYaURYvC/mX5/ByH8BiykUoXGoUPA==","X-Received":"by 2002:a2e:968c:: with SMTP id q12mr6673602lji.36.1554465144771;\n\tFri, 05 Apr 2019 04:52:24 -0700 (PDT)","Date":"Fri, 5 Apr 2019 13:52:23 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190405115223.GC23466@bigcity.dyn.berto.se>","References":"<20190403150735.27580-1-jacopo@jmondi.org>\n\t<20190403150735.27580-8-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190403150735.27580-8-jacopo@jmondi.org>","User-Agent":"Mutt/1.11.3 (2019-02-01)","Subject":"Re: [libcamera-devel] [PATCH v3 7/8] libcamera: pipeline: Add\n\tmethod to retrieve Request from Buffer","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":"Fri, 05 Apr 2019 11:53:26 -0000"}},{"id":1288,"web_url":"https://patchwork.libcamera.org/comment/1288/","msgid":"<20190405154516.GA5184@pendragon.ideasonboard.com>","date":"2019-04-05T15:45:16","subject":"Re: [libcamera-devel] [PATCH v3 7/8] libcamera: pipeline: Add\n\tmethod to retrieve Request from Buffer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello Jacopo,\n\nThank you for the patch.\n\nOn Fri, Apr 05, 2019 at 01:48:35PM +0200, Niklas Söderlund wrote:\n> On 2019-04-03 17:07:34 +0200, Jacopo Mondi wrote:\n> > Add a method to CameraData base class to retrieve a pointer to the\n> > Request that contains a given buffer. Intended users are buffer\n> > completion slots that needs to associate a Request to a just completed\n> > Buffer.\n> > \n> > In preparation to support multiple requests from different streams,\n> > update all the pipeline handler implementations to use this method\n> > instead of using the last queued request.\n> > \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/request.h | 2 ++\n> > src/libcamera/include/pipeline_handler.h | 3 +++\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 3 ++-\n> > src/libcamera/pipeline/uvcvideo.cpp | 3 ++-\n> > src/libcamera/pipeline/vimc.cpp | 3 ++-\n> > src/libcamera/pipeline_handler.cpp | 29 ++++++++++++++++++++++++\n> > src/libcamera/request.cpp | 7 ++++++\n> > 7 files changed, 47 insertions(+), 3 deletions(-)\n> > \n> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > index 5ac4d20d1d9f..8f5892fd3111 100644\n> > --- a/include/libcamera/request.h\n> > +++ b/include/libcamera/request.h\n> > @@ -38,6 +38,8 @@ public:\n> > \n> > \tconst std::list streams() const;\n> > \n> > +\tconst std::unordered_set &pending() const { return pending_; }\n> > +\n> > \tStatus status() const { return status_; }\n> > \n> > private:\n> > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> > index 920b57609470..6cdadcbdc3ea 100644\n> > --- a/src/libcamera/include/pipeline_handler.h\n> > +++ b/src/libcamera/include/pipeline_handler.h\n> > @@ -39,6 +39,9 @@ public:\n> > \tPipelineHandler *pipe_;\n> > \tstd::list queuedRequests_;\n> > \n> > +protected:\n> > +\tRequest *requestFromBuffer(Buffer *buffer);\n> > +\n> > private:\n> > \tCameraData(const CameraData &) = delete;\n> > \tCameraData &operator=(const CameraData &) = delete;\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 8c67cf985d1e..17e3e8677e28 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -801,7 +801,8 @@ void PipelineHandlerIPU3::IPU3CameraData::imguInputBufferReady(Buffer *buffer)\n> > */\n> > void PipelineHandlerIPU3::IPU3CameraData::imguOutputBufferReady(Buffer *buffer)\n> > {\n> > -\tRequest *request = queuedRequests_.front();\n> > +\tRequest *request = requestFromBuffer(buffer);\n> > +\tASSERT(request);\n> > \n> > \tpipe_->completeBuffer(camera_, request, buffer);\n> > \tpipe_->completeRequest(camera_, request);\n> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> > index 128f0c49dba3..d571b8b4ea83 100644\n> > --- a/src/libcamera/pipeline/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> > @@ -226,7 +226,8 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n> > \n> > void PipelineHandlerUVC::UVCCameraData::bufferReady(Buffer *buffer)\n> > {\n> > -\tRequest *request = queuedRequests_.front();\n> > +\tRequest *request = requestFromBuffer(buffer);\n> \n> This make me feel uneasy :-(\n> \n> We talked in the past about how to handle request completion. The idea \n> from the beginning was conceived with the reequest API in mind, which \n> IIRC guarantees that requests are completed in the same order they are \n> queued. While V4L2 have no such guarantees for for buffers. This was one \n> of the issues I hit with my single stream old UVC camera where the first \n> buffer queue would contains errors and the uvcvideo driver would not \n> return it to user-space and move on to the second buffer queued and \n> libcamera fail as buffers are dequeued out of order (from it's point of \n> view).\n> \n> I understand what you try to do in this patch and I agree it might be \n> needed. But I fear we first need to discuss how we should handle V4L2 \n> buffers being dequeued out of order. As this change would hide this \n> behavior while it's intent is indeed something different.\n\nI share Niklas' concern here. Buffers may complete out of order at the\nV4L2 level, but we must ensure that requests complete in order. Niklas,\nhave you thought about how you would like to solve this ?\n\nThinking out loud, I'm thinking about the following.\n\n- When a buffer is dequeued, find the corresponding request.\n- If the buffer doesn't belong to any request this is an error, but\n ASSERT is a bit too harsh, we should notify the application instead to\n allow a clean shutdown.\n- Otherwise, mark the buffer as complete in the request (remove it from\n the pending list).\n- Then, in the pipeline handler, if no more buffer is pending, and if\n all other data that need to be stored in the request are available,\n mark the request as complete. We don't have any additional data beyond\n buffers yet, but the point here is that this decision should be under\n control of the pipeline handler to make this possible.\n- If the request just marked as complete is not the first in the queue,\n leave and there.\n- Otherwise, notify the application of request completion for all\n complete requests starting at the beginning of the queue.\n\n> > +\tASSERT(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 6735940799d8..e83416effad8 100644\n> > --- a/src/libcamera/pipeline/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc.cpp\n> > @@ -223,7 +223,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)\n> > \n> > void PipelineHandlerVimc::VimcCameraData::bufferReady(Buffer *buffer)\n> > {\n> > -\tRequest *request = queuedRequests_.front();\n> > +\tRequest *request = requestFromBuffer(buffer);\n> > +\tASSERT(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 9a8a4fde57e6..830ff354ed3e 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -86,6 +86,35 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> > * PipelineHandler::completeRequest()\n> > */\n> > \n> > +/**\n> > + * \\brief Retrieve the pending request that contains \\a buffer\n> > + * \\param[in] buffer The buffer contained in the returned request\n> > + *\n> > + * Return the request that contains \\a buffer, or nullptr if no such request\n> > + * is found. The intended callers of this method are buffer completion slots\n> > + * implemented in CameraData sub-classes which needs to associated a request\n> > + * to the just completed buffer. It is up to the caller of this function to\n> > + * deal with the case the buffer does not belong to any previously queued\n> > + * request or the request has already completed, possibly because of a\n> > + * duplicated buffer completion notification. This is generally considered\n> > + * a fatal error, and callers are expected to assert the validity of the\n> > + * returned request.\n> > + *\n> > + * \\return A pointer to the pending Request that contains the Buffer \\a buffer,\n> > + * or nullptr if no such request is found\n> > + */\n> > +Request *CameraData::requestFromBuffer(Buffer *buffer)\n> > +{\n> > +\tfor (Request *req : queuedRequests_) {\n> > +\t\tfor (Buffer *b : req->pending()) {\n> > +\t\t\tif (b == buffer)\n> > +\t\t\t\treturn req;\n> > +\t\t}\n> > +\t}\n\n>From an implementation point of view we may want to add a Request\npointer to the Buffer class to make this more efficient.\n\n> > +\n> > +\treturn nullptr;\n> > +}\n> > +\n> > /**\n> > * \\class PipelineHandler\n> > * \\brief Create and manage cameras based on a set of media devices\n> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > index 3a7841fb2bb3..c555752b2c0b 100644\n> > --- a/src/libcamera/request.cpp\n> > +++ b/src/libcamera/request.cpp\n> > @@ -108,6 +108,13 @@ const std::list Request::streams() const\n> > \treturn streams;\n> > }\n> > \n> > +/**\n> > + * \\fn Request::pending()\n> > + * \\brief Retrieve the list of not-yet-completed buffers\n> > + *\n> > + * \\return The set of pending buffers\n> > + */\n> > +\n> > /**\n> > * \\fn Request::status()\n> > * \\brief Retrieve the request completion status","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 07C0C60DB3\n\tfor ;\n\tFri, 5 Apr 2019 17:45:29 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [109.140.214.47])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BC83DE2;\n\tFri, 5 Apr 2019 17:45:26 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1554479127;\n\tbh=1jCwwlNcncK6sqM/kw+9NXvBHke0CHr2XaSrwnBgYJE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Hxa3HAk49m3YjElrclVWdJ8oIn8ndRO5hF9S6zMpr33nd+OkeCV9VB0o6iTKiOYSC\n\ty9uPG/3kcCQVuUqGcqV+dhjkLUzTVdV8QdmNPHpYPAFIYWS8c58tYNKRVGk7dH7sdB\n\tl5PXPeZZ2SWLVlicVVw2rS5/ktQVZzD22UDaQ1RI=","Date":"Fri, 5 Apr 2019 18:45:16 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190405154516.GA5184@pendragon.ideasonboard.com>","References":"<20190403150735.27580-1-jacopo@jmondi.org>\n\t<20190403150735.27580-8-jacopo@jmondi.org>\n\t<20190405114835.GB23466@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190405114835.GB23466@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 7/8] libcamera: pipeline: Add\n\tmethod to retrieve Request from Buffer","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":"Fri, 05 Apr 2019 15:45:29 -0000"}},{"id":1308,"web_url":"https://patchwork.libcamera.org/comment/1308/","msgid":"<20190408080558.hsra3cibnn56xgo3@uno.localdomain>","date":"2019-04-08T08:05:58","subject":"Re: [libcamera-devel] [PATCH v3 7/8] libcamera: pipeline: Add\n\tmethod to retrieve Request from Buffer","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent, Niklas,\n\nOn Fri, Apr 05, 2019 at 06:45:16PM +0300, Laurent Pinchart wrote:\n> Hello Jacopo,\n>\n> Thank you for the patch.\n>\n> On Fri, Apr 05, 2019 at 01:48:35PM +0200, Niklas Söderlund wrote:\n> > On 2019-04-03 17:07:34 +0200, Jacopo Mondi wrote:\n> > > Add a method to CameraData base class to retrieve a pointer to the\n> > > Request that contains a given buffer. Intended users are buffer\n> > > completion slots that needs to associate a Request to a just completed\n> > > Buffer.\n> > >\n> > > In preparation to support multiple requests from different streams,\n> > > update all the pipeline handler implementations to use this method\n> > > instead of using the last queued request.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > include/libcamera/request.h | 2 ++\n> > > src/libcamera/include/pipeline_handler.h | 3 +++\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 3 ++-\n> > > src/libcamera/pipeline/uvcvideo.cpp | 3 ++-\n> > > src/libcamera/pipeline/vimc.cpp | 3 ++-\n> > > src/libcamera/pipeline_handler.cpp | 29 ++++++++++++++++++++++++\n> > > src/libcamera/request.cpp | 7 ++++++\n> > > 7 files changed, 47 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > > index 5ac4d20d1d9f..8f5892fd3111 100644\n> > > --- a/include/libcamera/request.h\n> > > +++ b/include/libcamera/request.h\n> > > @@ -38,6 +38,8 @@ public:\n> > >\n> > > \tconst std::list streams() const;\n> > >\n> > > +\tconst std::unordered_set &pending() const { return pending_; }\n> > > +\n> > > \tStatus status() const { return status_; }\n> > >\n> > > private:\n> > > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> > > index 920b57609470..6cdadcbdc3ea 100644\n> > > --- a/src/libcamera/include/pipeline_handler.h\n> > > +++ b/src/libcamera/include/pipeline_handler.h\n> > > @@ -39,6 +39,9 @@ public:\n> > > \tPipelineHandler *pipe_;\n> > > \tstd::list queuedRequests_;\n> > >\n> > > +protected:\n> > > +\tRequest *requestFromBuffer(Buffer *buffer);\n> > > +\n> > > private:\n> > > \tCameraData(const CameraData &) = delete;\n> > > \tCameraData &operator=(const CameraData &) = delete;\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 8c67cf985d1e..17e3e8677e28 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -801,7 +801,8 @@ void PipelineHandlerIPU3::IPU3CameraData::imguInputBufferReady(Buffer *buffer)\n> > > */\n> > > void PipelineHandlerIPU3::IPU3CameraData::imguOutputBufferReady(Buffer *buffer)\n> > > {\n> > > -\tRequest *request = queuedRequests_.front();\n> > > +\tRequest *request = requestFromBuffer(buffer);\n> > > +\tASSERT(request);\n> > >\n> > > \tpipe_->completeBuffer(camera_, request, buffer);\n> > > \tpipe_->completeRequest(camera_, request);\n> > > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> > > index 128f0c49dba3..d571b8b4ea83 100644\n> > > --- a/src/libcamera/pipeline/uvcvideo.cpp\n> > > +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> > > @@ -226,7 +226,8 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n> > >\n> > > void PipelineHandlerUVC::UVCCameraData::bufferReady(Buffer *buffer)\n> > > {\n> > > -\tRequest *request = queuedRequests_.front();\n> > > +\tRequest *request = requestFromBuffer(buffer);\n> >\n> > This make me feel uneasy :-(\n> >\n> > We talked in the past about how to handle request completion. The idea\n> > from the beginning was conceived with the reequest API in mind, which\n> > IIRC guarantees that requests are completed in the same order they are\n> > queued. While V4L2 have no such guarantees for for buffers. This was one\n> > of the issues I hit with my single stream old UVC camera where the first\n> > buffer queue would contains errors and the uvcvideo driver would not\n> > return it to user-space and move on to the second buffer queued and\n> > libcamera fail as buffers are dequeued out of order (from it's point of\n> > view).\n> >\n> > I understand what you try to do in this patch and I agree it might be\n> > needed. But I fear we first need to discuss how we should handle V4L2\n> > buffers being dequeued out of order. As this change would hide this\n> > behavior while it's intent is indeed something different.\n>\n> I share Niklas' concern here. Buffers may complete out of order at the\n> V4L2 level, but we must ensure that requests complete in order. Niklas,\n> have you thought about how you would like to solve this ?\n>\n> Thinking out loud, I'm thinking about the following.\n>\n> - When a buffer is dequeued, find the corresponding request.\n> - If the buffer doesn't belong to any request this is an error, but\n> ASSERT is a bit too harsh, we should notify the application instead to\n> allow a clean shutdown.\n> - Otherwise, mark the buffer as complete in the request (remove it from\n> the pending list).\n> - Then, in the pipeline handler, if no more buffer is pending, and if\n> all other data that need to be stored in the request are available,\n> mark the request as complete. We don't have any additional data beyond\n> buffers yet, but the point here is that this decision should be under\n> control of the pipeline handler to make this possible.\n> - If the request just marked as complete is not the first in the queue,\n> leave and there.\n> - Otherwise, notify the application of request completion for all\n> complete requests starting at the beginning of the queue.\n>\n\nI wonder how much of this could be moved to the PipelineHandler base class\nand make it transparent for pipeline handler implementations.\n\n> > > +\tASSERT(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 6735940799d8..e83416effad8 100644\n> > > --- a/src/libcamera/pipeline/vimc.cpp\n> > > +++ b/src/libcamera/pipeline/vimc.cpp\n> > > @@ -223,7 +223,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)\n> > >\n> > > void PipelineHandlerVimc::VimcCameraData::bufferReady(Buffer *buffer)\n> > > {\n> > > -\tRequest *request = queuedRequests_.front();\n> > > +\tRequest *request = requestFromBuffer(buffer);\n> > > +\tASSERT(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 9a8a4fde57e6..830ff354ed3e 100644\n> > > --- a/src/libcamera/pipeline_handler.cpp\n> > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > @@ -86,6 +86,35 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> > > * PipelineHandler::completeRequest()\n> > > */\n> > >\n> > > +/**\n> > > + * \\brief Retrieve the pending request that contains \\a buffer\n> > > + * \\param[in] buffer The buffer contained in the returned request\n> > > + *\n> > > + * Return the request that contains \\a buffer, or nullptr if no such request\n> > > + * is found. The intended callers of this method are buffer completion slots\n> > > + * implemented in CameraData sub-classes which needs to associated a request\n> > > + * to the just completed buffer. It is up to the caller of this function to\n> > > + * deal with the case the buffer does not belong to any previously queued\n> > > + * request or the request has already completed, possibly because of a\n> > > + * duplicated buffer completion notification. This is generally considered\n> > > + * a fatal error, and callers are expected to assert the validity of the\n> > > + * returned request.\n> > > + *\n> > > + * \\return A pointer to the pending Request that contains the Buffer \\a buffer,\n> > > + * or nullptr if no such request is found\n> > > + */\n> > > +Request *CameraData::requestFromBuffer(Buffer *buffer)\n> > > +{\n> > > +\tfor (Request *req : queuedRequests_) {\n> > > +\t\tfor (Buffer *b : req->pending()) {\n> > > +\t\t\tif (b == buffer)\n> > > +\t\t\t\treturn req;\n> > > +\t\t}\n> > > +\t}\n>\n> From an implementation point of view we may want to add a Request\n> pointer to the Buffer class to make this more efficient.\n>\n\nIndeed, as Niklas pointed out, there are enough loops on the Stream\nand Buffers containers around.\n\nThanks\n j\n\n> > > +\n> > > +\treturn nullptr;\n> > > +}\n> > > +\n> > > /**\n> > > * \\class PipelineHandler\n> > > * \\brief Create and manage cameras based on a set of media devices\n> > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > > index 3a7841fb2bb3..c555752b2c0b 100644\n> > > --- a/src/libcamera/request.cpp\n> > > +++ b/src/libcamera/request.cpp\n> > > @@ -108,6 +108,13 @@ const std::list Request::streams() const\n> > > \treturn streams;\n> > > }\n> > >\n> > > +/**\n> > > + * \\fn Request::pending()\n> > > + * \\brief Retrieve the list of not-yet-completed buffers\n> > > + *\n> > > + * \\return The set of pending buffers\n> > > + */\n> > > +\n> > > /**\n> > > * \\fn Request::status()\n> > > * \\brief Retrieve the request completion status\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3C92E60B23\n\tfor ;\n\tMon, 8 Apr 2019 10:05:11 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 4EC76E0010;\n\tMon, 8 Apr 2019 08:05:10 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 8 Apr 2019 10:05:58 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190408080558.hsra3cibnn56xgo3@uno.localdomain>","References":"<20190403150735.27580-1-jacopo@jmondi.org>\n\t<20190403150735.27580-8-jacopo@jmondi.org>\n\t<20190405114835.GB23466@bigcity.dyn.berto.se>\n\t<20190405154516.GA5184@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"qhywltfr65qchzxb\"","Content-Disposition":"inline","In-Reply-To":"<20190405154516.GA5184@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v3 7/8] libcamera: pipeline: Add\n\tmethod to retrieve Request from Buffer","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":"Mon, 08 Apr 2019 08:05:11 -0000"}},{"id":1314,"web_url":"https://patchwork.libcamera.org/comment/1314/","msgid":"<20190408130658.GD4888@pendragon.ideasonboard.com>","date":"2019-04-08T13:06:58","subject":"Re: [libcamera-devel] [PATCH v3 7/8] libcamera: pipeline: Add\n\tmethod to retrieve Request from Buffer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Apr 08, 2019 at 10:05:58AM +0200, Jacopo Mondi wrote:\n> On Fri, Apr 05, 2019 at 06:45:16PM +0300, Laurent Pinchart wrote:\n> > On Fri, Apr 05, 2019 at 01:48:35PM +0200, Niklas Söderlund wrote:\n> >> On 2019-04-03 17:07:34 +0200, Jacopo Mondi wrote:\n> >>> Add a method to CameraData base class to retrieve a pointer to the\n> >>> Request that contains a given buffer. Intended users are buffer\n> >>> completion slots that needs to associate a Request to a just completed\n> >>> Buffer.\n> >>>\n> >>> In preparation to support multiple requests from different streams,\n> >>> update all the pipeline handler implementations to use this method\n> >>> instead of using the last queued request.\n> >>>\n> >>> Signed-off-by: Jacopo Mondi \n> >>> ---\n> >>> include/libcamera/request.h | 2 ++\n> >>> src/libcamera/include/pipeline_handler.h | 3 +++\n> >>> src/libcamera/pipeline/ipu3/ipu3.cpp | 3 ++-\n> >>> src/libcamera/pipeline/uvcvideo.cpp | 3 ++-\n> >>> src/libcamera/pipeline/vimc.cpp | 3 ++-\n> >>> src/libcamera/pipeline_handler.cpp | 29 ++++++++++++++++++++++++\n> >>> src/libcamera/request.cpp | 7 ++++++\n> >>> 7 files changed, 47 insertions(+), 3 deletions(-)\n> >>>\n> >>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> >>> index 5ac4d20d1d9f..8f5892fd3111 100644\n> >>> --- a/include/libcamera/request.h\n> >>> +++ b/include/libcamera/request.h\n> >>> @@ -38,6 +38,8 @@ public:\n> >>>\n> >>> \tconst std::list streams() const;\n> >>>\n> >>> +\tconst std::unordered_set &pending() const { return pending_; }\n> >>> +\n> >>> \tStatus status() const { return status_; }\n> >>>\n> >>> private:\n> >>> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> >>> index 920b57609470..6cdadcbdc3ea 100644\n> >>> --- a/src/libcamera/include/pipeline_handler.h\n> >>> +++ b/src/libcamera/include/pipeline_handler.h\n> >>> @@ -39,6 +39,9 @@ public:\n> >>> \tPipelineHandler *pipe_;\n> >>> \tstd::list queuedRequests_;\n> >>>\n> >>> +protected:\n> >>> +\tRequest *requestFromBuffer(Buffer *buffer);\n> >>> +\n> >>> private:\n> >>> \tCameraData(const CameraData &) = delete;\n> >>> \tCameraData &operator=(const CameraData &) = delete;\n> >>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>> index 8c67cf985d1e..17e3e8677e28 100644\n> >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>> @@ -801,7 +801,8 @@ void PipelineHandlerIPU3::IPU3CameraData::imguInputBufferReady(Buffer *buffer)\n> >>> */\n> >>> void PipelineHandlerIPU3::IPU3CameraData::imguOutputBufferReady(Buffer *buffer)\n> >>> {\n> >>> -\tRequest *request = queuedRequests_.front();\n> >>> +\tRequest *request = requestFromBuffer(buffer);\n> >>> +\tASSERT(request);\n> >>>\n> >>> \tpipe_->completeBuffer(camera_, request, buffer);\n> >>> \tpipe_->completeRequest(camera_, request);\n> >>> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> >>> index 128f0c49dba3..d571b8b4ea83 100644\n> >>> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> >>> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> >>> @@ -226,7 +226,8 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n> >>>\n> >>> void PipelineHandlerUVC::UVCCameraData::bufferReady(Buffer *buffer)\n> >>> {\n> >>> -\tRequest *request = queuedRequests_.front();\n> >>> +\tRequest *request = requestFromBuffer(buffer);\n> >>\n> >> This make me feel uneasy :-(\n> >>\n> >> We talked in the past about how to handle request completion. The idea\n> >> from the beginning was conceived with the reequest API in mind, which\n> >> IIRC guarantees that requests are completed in the same order they are\n> >> queued. While V4L2 have no such guarantees for for buffers. This was one\n> >> of the issues I hit with my single stream old UVC camera where the first\n> >> buffer queue would contains errors and the uvcvideo driver would not\n> >> return it to user-space and move on to the second buffer queued and\n> >> libcamera fail as buffers are dequeued out of order (from it's point of\n> >> view).\n> >>\n> >> I understand what you try to do in this patch and I agree it might be\n> >> needed. But I fear we first need to discuss how we should handle V4L2\n> >> buffers being dequeued out of order. As this change would hide this\n> >> behavior while it's intent is indeed something different.\n> >\n> > I share Niklas' concern here. Buffers may complete out of order at the\n> > V4L2 level, but we must ensure that requests complete in order. Niklas,\n> > have you thought about how you would like to solve this ?\n> >\n> > Thinking out loud, I'm thinking about the following.\n> >\n> > - When a buffer is dequeued, find the corresponding request.\n> > - If the buffer doesn't belong to any request this is an error, but\n> > ASSERT is a bit too harsh, we should notify the application instead to\n> > allow a clean shutdown.\n> > - Otherwise, mark the buffer as complete in the request (remove it from\n> > the pending list).\n> > - Then, in the pipeline handler, if no more buffer is pending, and if\n> > all other data that need to be stored in the request are available,\n> > mark the request as complete. We don't have any additional data beyond\n> > buffers yet, but the point here is that this decision should be under\n> > control of the pipeline handler to make this possible.\n> > - If the request just marked as complete is not the first in the queue,\n> > leave and there.\n> > - Otherwise, notify the application of request completion for all\n> > complete requests starting at the beginning of the queue.\n> \n> I wonder how much of this could be moved to the PipelineHandler base class\n> and make it transparent for pipeline handler implementations.\n\nProbably quite a bit, but I think we should implement it in the IPU3\npipeline handler first, and then refactor the code when a second\npipeline handler will need this (of course if you already have a clear\nview of how it will look like, you can also have a go :-)).\n\n> >>> +\tASSERT(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 6735940799d8..e83416effad8 100644\n> >>> --- a/src/libcamera/pipeline/vimc.cpp\n> >>> +++ b/src/libcamera/pipeline/vimc.cpp\n> >>> @@ -223,7 +223,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)\n> >>>\n> >>> void PipelineHandlerVimc::VimcCameraData::bufferReady(Buffer *buffer)\n> >>> {\n> >>> -\tRequest *request = queuedRequests_.front();\n> >>> +\tRequest *request = requestFromBuffer(buffer);\n> >>> +\tASSERT(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 9a8a4fde57e6..830ff354ed3e 100644\n> >>> --- a/src/libcamera/pipeline_handler.cpp\n> >>> +++ b/src/libcamera/pipeline_handler.cpp\n> >>> @@ -86,6 +86,35 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> >>> * PipelineHandler::completeRequest()\n> >>> */\n> >>>\n> >>> +/**\n> >>> + * \\brief Retrieve the pending request that contains \\a buffer\n> >>> + * \\param[in] buffer The buffer contained in the returned request\n> >>> + *\n> >>> + * Return the request that contains \\a buffer, or nullptr if no such request\n> >>> + * is found. The intended callers of this method are buffer completion slots\n> >>> + * implemented in CameraData sub-classes which needs to associated a request\n> >>> + * to the just completed buffer. It is up to the caller of this function to\n> >>> + * deal with the case the buffer does not belong to any previously queued\n> >>> + * request or the request has already completed, possibly because of a\n> >>> + * duplicated buffer completion notification. This is generally considered\n> >>> + * a fatal error, and callers are expected to assert the validity of the\n> >>> + * returned request.\n> >>> + *\n> >>> + * \\return A pointer to the pending Request that contains the Buffer \\a buffer,\n> >>> + * or nullptr if no such request is found\n> >>> + */\n> >>> +Request *CameraData::requestFromBuffer(Buffer *buffer)\n> >>> +{\n> >>> +\tfor (Request *req : queuedRequests_) {\n> >>> +\t\tfor (Buffer *b : req->pending()) {\n> >>> +\t\t\tif (b == buffer)\n> >>> +\t\t\t\treturn req;\n> >>> +\t\t}\n> >>> +\t}\n> >\n> > From an implementation point of view we may want to add a Request\n> > pointer to the Buffer class to make this more efficient.\n> \n> Indeed, as Niklas pointed out, there are enough loops on the Stream\n> and Buffers containers around.\n> \n> >>> +\n> >>> +\treturn nullptr;\n> >>> +}\n> >>> +\n> >>> /**\n> >>> * \\class PipelineHandler\n> >>> * \\brief Create and manage cameras based on a set of media devices\n> >>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> >>> index 3a7841fb2bb3..c555752b2c0b 100644\n> >>> --- a/src/libcamera/request.cpp\n> >>> +++ b/src/libcamera/request.cpp\n> >>> @@ -108,6 +108,13 @@ const std::list Request::streams() const\n> >>> \treturn streams;\n> >>> }\n> >>>\n> >>> +/**\n> >>> + * \\fn Request::pending()\n> >>> + * \\brief Retrieve the list of not-yet-completed buffers\n> >>> + *\n> >>> + * \\return The set of pending buffers\n> >>> + */\n> >>> +\n> >>> /**\n> >>> * \\fn Request::status()\n> >>> * \\brief Retrieve the request completion status","headers":{"Return-Path":"","Received":["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 AE8B260B2E\n\tfor ;\n\tMon, 8 Apr 2019 15:07:12 +0200 (CEST)","from pendragon.ideasonboard.com (85-76-86-94-nat.elisa-mobile.fi\n\t[85.76.86.94])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BD89B587;\n\tMon, 8 Apr 2019 15:07:10 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1554728831;\n\tbh=G9pvr9/gYYpLaOxKPv7wgXDmyW6z4xcWPj5tgav1SpI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=heljYvbmQDOO6jj5/OTQilSboV5ZTkqrDOFOfmkv9YE5zIIOm9ZLJG/plNkuOFn/6\n\tQOJTnndLyYXZxnDoJerpkJV5/bDE5CMcEMSlDXkhyXG0pXzkrSUUv5nrjO8+iFUjQ+\n\tAl4ohgJ7/uLpBzBZlfACDwN6Lqorly3JP3G51O1M=","Date":"Mon, 8 Apr 2019 16:06:58 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190408130658.GD4888@pendragon.ideasonboard.com>","References":"<20190403150735.27580-1-jacopo@jmondi.org>\n\t<20190403150735.27580-8-jacopo@jmondi.org>\n\t<20190405114835.GB23466@bigcity.dyn.berto.se>\n\t<20190405154516.GA5184@pendragon.ideasonboard.com>\n\t<20190408080558.hsra3cibnn56xgo3@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190408080558.hsra3cibnn56xgo3@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 7/8] libcamera: pipeline: Add\n\tmethod to retrieve Request from Buffer","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":"Mon, 08 Apr 2019 13:07:12 -0000"}}]