[{"id":23375,"web_url":"https://patchwork.libcamera.org/comment/23375/","msgid":"<20220610074905.lvgqfe5mekruav2y@uno.localdomain>","date":"2022-06-10T07:49:05","subject":"Re: [libcamera-devel] [PATCH v1 3/4] ipa: ipu3: Prevent over\n\tqueuing of requests","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n\nOn Fri, Jun 03, 2022 at 03:22:58PM +0200, Umang Jain via libcamera-devel wrote:\n> The IPAIPU3 processes each IPAFrameContext present in the FCQueue\n> ring buffer. If the application queues the requests at a pace where\n> IPAFrameContext(s) can get potentially over-written without getting\n> processed by the IPA, it would lead to un-desirable effects.\n>\n> Hence, inspect the FCQueue if it is full and reject any further\n> queuing of requests. This requires changes to the IPAIPU3 mojom\n> interface. The IPAIPU3::queueRequest() cannot be [async] anymore\n> since we need to return a success/failure value.\n>\n> The rejection by the IPA doesn't mean it cannot be queued\n> later. The request is still preserved in\n> IPU3CameraData::pendingRequests_. It shall be queued sequentially\n> on subsequent calls to IPU3CameraData::queuePendingRequests().\n>\n> Signed-off-by: Umang Jain \n> ---\n> I am not super-happy with changing the interface and dropping [async]\n> but this is what I've for now. Would appreciate any other ideas...\n\nI feel like this change handles the issue at the very bottom of the\ncall stack, once a request has been queued from the ph to the IPA.\n\nI wonder if we shouldn't try to tackle the problem from the top\ninstead, which means at the time a request is queued to the camera by\nthe application.\n\nThe lenght of the FCQueue represents the maximum number of requests\nin-flight at a time, iow the number of requests queued to the camera\nand not yet completed.\n\nWould it make sense to make this a global camera property and have the\nbase pipeline handler class defer queueing requests when too many are\nin-flight ?\n\nThanks\n j\n\n> ---\n> include/libcamera/ipa/ipu3.mojom | 2 +-\n> src/ipa/ipu3/ipu3.cpp | 11 +++++++++--\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 10 +++++++++-\n> 3 files changed, 19 insertions(+), 4 deletions(-)\n>\n> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> index d1b1c6b8..3faffd9c 100644\n> --- a/include/libcamera/ipa/ipu3.mojom\n> +++ b/include/libcamera/ipa/ipu3.mojom\n> @@ -30,7 +30,7 @@ interface IPAIPU3Interface {\n> \tmapBuffers(array buffers);\n> \tunmapBuffers(array ids);\n>\n> -\t[async] queueRequest(uint32 frame, libcamera.ControlList controls);\n> +\tqueueRequest(uint32 frame, libcamera.ControlList controls) => (int32 ret);\n> \t[async] fillParamsBuffer(uint32 frame, uint32 bufferId);\n> \t[async] processStatsBuffer(uint32 frame, int64 frameTimestamp,\n> \t\t\t\t uint32 bufferId, libcamera.ControlList sensorControls);\n> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> index 1d6ee515..63aa9b56 100644\n> --- a/src/ipa/ipu3/ipu3.cpp\n> +++ b/src/ipa/ipu3/ipu3.cpp\n> @@ -145,7 +145,7 @@ public:\n> \tvoid mapBuffers(const std::vector &buffers) override;\n> \tvoid unmapBuffers(const std::vector &ids) override;\n>\n> -\tvoid queueRequest(const uint32_t frame, const ControlList &controls) override;\n> +\tint queueRequest(const uint32_t frame, const ControlList &controls) override;\n> \tvoid fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override;\n> \tvoid processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp,\n> \t\t\t\tconst uint32_t bufferId,\n> @@ -616,11 +616,18 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> *\n> * Parse the request to handle any IPA-managed controls that were set from the\n> * application such as manual sensor settings.\n> + *\n> + * \\return 0 if successful, -EPERM otherwise.\n> */\n> -void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)\n> +int IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)\n> {\n> +\tif (context_.frameContexts.isFull())\n> +\t\treturn -EPERM;\n> +\n> \t/* \\todo Start processing for 'frame' based on 'controls'. */\n> \t*context_.frameContexts.get(frame) = { frame, controls };\n> +\n> +\treturn 0;\n> }\n>\n> /**\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index fd989e61..d69e28d4 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -874,7 +874,15 @@ void IPU3CameraData::queuePendingRequests()\n>\n> \t\tinfo->rawBuffer = rawBuffer;\n>\n> -\t\tipa_->queueRequest(info->id, request->controls());\n> +\t\t/*\n> +\t\t * Queueing request to the IPA can fail in cases like over-flowing\n> +\t\t * its queue. Hence, keep the request preserved in pendingRequests_\n> +\t\t * if a failure occurs.\n> +\t\t */\n> +\t\tif (ipa_->queueRequest(info->id, request->controls()) != 0) {\n> +\t\t\tframeInfos_.remove(info);\n> +\t\t\tbreak;\n> +\t\t}\n>\n> \t\tpendingRequests_.pop();\n> \t\tprocessingRequests_.push(request);\n> --\n> 2.31.1\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 3F16BBD161\n\tfor ;\n\tFri, 10 Jun 2022 07:49:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8CA6A65632;\n\tFri, 10 Jun 2022 09:49:08 +0200 (CEST)","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 7846E600F4\n\tfor ;\n\tFri, 10 Jun 2022 09:49:07 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 0162FE000B;\n\tFri, 10 Jun 2022 07:49:06 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654847348;\n\tbh=3WQHiMG5RdEbAonv0SxRnGfvw2VbRSBSiKj2qCoooxM=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=1wMHm8i/BQlC1jPgkdjMs2UL9LkeJwZkl3lJ+YnxOyc1a3TSDwdJX2Gq2ntHur29o\n\tpwjh5zXAzMUIjSSJPv/pt95DJ08llsw+1WofJPhljEotRBswRYYfycVuGBZApdRAAj\n\tD+9GVE7mCgV6iCniOk4wx0m/1NYaNPAbDLY/aDFE4b+zLNK/ZC4qfKNHvzB53Adsd2\n\tRw1WGXXKL/qLAc2AMweLvRKqRPwXnjjlrBxPZq1XvDLDOfyVn66E/gskXcYV+jbkeR\n\t96QC2PZs7IQ8en2f8RYi/+jVvNPs796thaPpBC1Gyp5PPo6x503xz+Vc8CaTnK9Xrm\n\tsDdoKmQ8asLlg==","Date":"Fri, 10 Jun 2022 09:49:05 +0200","To":"Umang Jain ","Message-ID":"<20220610074905.lvgqfe5mekruav2y@uno.localdomain>","References":"<20220603132259.188845-1-umang.jain@ideasonboard.com>\n\t<20220603132259.188845-4-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220603132259.188845-4-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v1 3/4] ipa: ipu3: Prevent over\n\tqueuing of requests","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23377,"web_url":"https://patchwork.libcamera.org/comment/23377/","msgid":"","date":"2022-06-10T08:32:03","subject":"Re: [libcamera-devel] [PATCH v1 3/4] ipa: ipu3: Prevent over\n\tqueuing of requests","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 6/10/22 09:49, Jacopo Mondi wrote:\n> Hi Umang,\n>\n> On Fri, Jun 03, 2022 at 03:22:58PM +0200, Umang Jain via libcamera-devel wrote:\n>> The IPAIPU3 processes each IPAFrameContext present in the FCQueue\n>> ring buffer. If the application queues the requests at a pace where\n>> IPAFrameContext(s) can get potentially over-written without getting\n>> processed by the IPA, it would lead to un-desirable effects.\n>>\n>> Hence, inspect the FCQueue if it is full and reject any further\n>> queuing of requests. This requires changes to the IPAIPU3 mojom\n>> interface. The IPAIPU3::queueRequest() cannot be [async] anymore\n>> since we need to return a success/failure value.\n>>\n>> The rejection by the IPA doesn't mean it cannot be queued\n>> later. The request is still preserved in\n>> IPU3CameraData::pendingRequests_. It shall be queued sequentially\n>> on subsequent calls to IPU3CameraData::queuePendingRequests().\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> I am not super-happy with changing the interface and dropping [async]\n>> but this is what I've for now. Would appreciate any other ideas...\n> I feel like this change handles the issue at the very bottom of the\n> call stack, once a request has been queued from the ph to the IPA.\n>\n> I wonder if we shouldn't try to tackle the problem from the top\n> instead, which means at the time a request is queued to the camera by\n> the application.\n>\n> The lenght of the FCQueue represents the maximum number of requests\n> in-flight at a time, iow the number of requests queued to the camera\n> and not yet completed.\n>\n> Would it make sense to make this a global camera property and have the\n> base pipeline handler class defer queueing requests when too many are\n> in-flight ?\n\n\nIn the last call, I brought up this issue. However, it was geared \ntowards a solution to avoid dropping [async] from the mojom interface.\n\nThe design I have in mind and shared in the call as well is basically \nthat, the PH should be responsible to queue up requests to the IPA - in \na way that it doesn't do over-queuing.\n\nThe way it should be done is - communicating the size of the FCQueue to \nthe PH during init/configure phase (or make a global in mojom interface \nwhere it's shared to both parts - not sure yet, I haven't decided). The \nPH will queue requests to the IPA, increment a counter. We already have \na callback from IPA to PH when a frame processing completes - so \ndecrement the counter. The max the counter can reach is FCQueue.size() - \nbeyond that it should just stop queuing and let the request sit in \nPipelineHandler::pendingRequests_ queue. The queuePendingRequests() is a \nrecurring call on every request queue by the application - so as long as \nthe counter < FCQueue.size() - it will keep queuing and stop when it's > \nFCQueue.size(). As frames get completed, counter will decrement and \nqueuing shall start again.\n\n> Thanks\n> j\n>\n>> ---\n>> include/libcamera/ipa/ipu3.mojom | 2 +-\n>> src/ipa/ipu3/ipu3.cpp | 11 +++++++++--\n>> src/libcamera/pipeline/ipu3/ipu3.cpp | 10 +++++++++-\n>> 3 files changed, 19 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n>> index d1b1c6b8..3faffd9c 100644\n>> --- a/include/libcamera/ipa/ipu3.mojom\n>> +++ b/include/libcamera/ipa/ipu3.mojom\n>> @@ -30,7 +30,7 @@ interface IPAIPU3Interface {\n>> \tmapBuffers(array buffers);\n>> \tunmapBuffers(array ids);\n>>\n>> -\t[async] queueRequest(uint32 frame, libcamera.ControlList controls);\n>> +\tqueueRequest(uint32 frame, libcamera.ControlList controls) => (int32 ret);\n>> \t[async] fillParamsBuffer(uint32 frame, uint32 bufferId);\n>> \t[async] processStatsBuffer(uint32 frame, int64 frameTimestamp,\n>> \t\t\t\t uint32 bufferId, libcamera.ControlList sensorControls);\n>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>> index 1d6ee515..63aa9b56 100644\n>> --- a/src/ipa/ipu3/ipu3.cpp\n>> +++ b/src/ipa/ipu3/ipu3.cpp\n>> @@ -145,7 +145,7 @@ public:\n>> \tvoid mapBuffers(const std::vector &buffers) override;\n>> \tvoid unmapBuffers(const std::vector &ids) override;\n>>\n>> -\tvoid queueRequest(const uint32_t frame, const ControlList &controls) override;\n>> +\tint queueRequest(const uint32_t frame, const ControlList &controls) override;\n>> \tvoid fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override;\n>> \tvoid processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp,\n>> \t\t\t\tconst uint32_t bufferId,\n>> @@ -616,11 +616,18 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n>> *\n>> * Parse the request to handle any IPA-managed controls that were set from the\n>> * application such as manual sensor settings.\n>> + *\n>> + * \\return 0 if successful, -EPERM otherwise.\n>> */\n>> -void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)\n>> +int IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)\n>> {\n>> +\tif (context_.frameContexts.isFull())\n>> +\t\treturn -EPERM;\n>> +\n>> \t/* \\todo Start processing for 'frame' based on 'controls'. */\n>> \t*context_.frameContexts.get(frame) = { frame, controls };\n>> +\n>> +\treturn 0;\n>> }\n>>\n>> /**\n>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> index fd989e61..d69e28d4 100644\n>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>> @@ -874,7 +874,15 @@ void IPU3CameraData::queuePendingRequests()\n>>\n>> \t\tinfo->rawBuffer = rawBuffer;\n>>\n>> -\t\tipa_->queueRequest(info->id, request->controls());\n>> +\t\t/*\n>> +\t\t * Queueing request to the IPA can fail in cases like over-flowing\n>> +\t\t * its queue. Hence, keep the request preserved in pendingRequests_\n>> +\t\t * if a failure occurs.\n>> +\t\t */\n>> +\t\tif (ipa_->queueRequest(info->id, request->controls()) != 0) {\n>> +\t\t\tframeInfos_.remove(info);\n>> +\t\t\tbreak;\n>> +\t\t}\n>>\n>> \t\tpendingRequests_.pop();\n>> \t\tprocessingRequests_.push(request);\n>> --\n>> 2.31.1\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 CBA08BD161\n\tfor ;\n\tFri, 10 Jun 2022 08:32:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1BB18600F4;\n\tFri, 10 Jun 2022 10:32:08 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8721C600F4\n\tfor ;\n\tFri, 10 Jun 2022 10:32:06 +0200 (CEST)","from [192.168.101.240] (217.red-95-124-192.dynamicip.rima-tde.net\n\t[95.124.192.217])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E4886526;\n\tFri, 10 Jun 2022 10:32:05 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654849928;\n\tbh=92/HeTIYaKV/Veo4LGtSzViI00pbuCkpGD1kG/HiQ7Q=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=ICnQrjvsjAXCwsUWcL9QwGbTQqFFEWKNLDlWjkmUGYVr5j2RF1uiYBx7jIv15x02j\n\tiyKFUR8VxeFJ/PNYWCFCra7EP/prSpQQnXsyAouoCDZpP/xpaMQ+z5spjKiIq1MKPB\n\tiljI/2sPhYdxSed/ommQM7v8xw72TWXBRSIBm2mZxI1Hdm4M3TCgo1eSUACeXdFdlM\n\t86HrFXQww3MQVW5AIyBcZOrxRaLgmUD8k8QRh5GfQONsPJm6zpcOIvlCIAkMb6HSo7\n\t4PhWb5YFylHtSqgmdfI2CM7QUiCjx+tc1Cd3CT0/04U+vPzZtEw+Zc59HawZ5AI9la\n\tPN1v1Pn0lCmrQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1654849926;\n\tbh=92/HeTIYaKV/Veo4LGtSzViI00pbuCkpGD1kG/HiQ7Q=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=QfYgWKMFy5K7doUlephCNS7LRblKhriVLZmqeAxH4k5lxN2EEcisY/JqQWlFm81Fu\n\tvnt+x/vyqfMabrWb5vJLhDOV/ReD+uuKPWjVtYqzgbLzpv5In2OOQwVz9Vmh6aunNX\n\t3S01etxDu3WD1bf8LtnWUcRh2s+d3++VrZK/qduA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"QfYgWKMF\"; dkim-atps=neutral","Message-ID":"","Date":"Fri, 10 Jun 2022 10:32:03 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Jacopo Mondi ","References":"<20220603132259.188845-1-umang.jain@ideasonboard.com>\n\t<20220603132259.188845-4-umang.jain@ideasonboard.com>\n\t<20220610074905.lvgqfe5mekruav2y@uno.localdomain>","In-Reply-To":"<20220610074905.lvgqfe5mekruav2y@uno.localdomain>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v1 3/4] ipa: ipu3: Prevent over\n\tqueuing of requests","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","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23379,"web_url":"https://patchwork.libcamera.org/comment/23379/","msgid":"<20220610085228.7xxk6s3hcjmf7ber@uno.localdomain>","date":"2022-06-10T08:52:28","subject":"Re: [libcamera-devel] [PATCH v1 3/4] ipa: ipu3: Prevent over\n\tqueuing of requests","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n\nOn Fri, Jun 10, 2022 at 10:32:03AM +0200, Umang Jain wrote:\n> Hi Jacopo,\n>\n> On 6/10/22 09:49, Jacopo Mondi wrote:\n> > Hi Umang,\n> >\n> > On Fri, Jun 03, 2022 at 03:22:58PM +0200, Umang Jain via libcamera-devel wrote:\n> > > The IPAIPU3 processes each IPAFrameContext present in the FCQueue\n> > > ring buffer. If the application queues the requests at a pace where\n> > > IPAFrameContext(s) can get potentially over-written without getting\n> > > processed by the IPA, it would lead to un-desirable effects.\n> > >\n> > > Hence, inspect the FCQueue if it is full and reject any further\n> > > queuing of requests. This requires changes to the IPAIPU3 mojom\n> > > interface. The IPAIPU3::queueRequest() cannot be [async] anymore\n> > > since we need to return a success/failure value.\n> > >\n> > > The rejection by the IPA doesn't mean it cannot be queued\n> > > later. The request is still preserved in\n> > > IPU3CameraData::pendingRequests_. It shall be queued sequentially\n> > > on subsequent calls to IPU3CameraData::queuePendingRequests().\n> > >\n> > > Signed-off-by: Umang Jain \n> > > ---\n> > > I am not super-happy with changing the interface and dropping [async]\n> > > but this is what I've for now. Would appreciate any other ideas...\n> > I feel like this change handles the issue at the very bottom of the\n> > call stack, once a request has been queued from the ph to the IPA.\n> >\n> > I wonder if we shouldn't try to tackle the problem from the top\n> > instead, which means at the time a request is queued to the camera by\n> > the application.\n> >\n> > The lenght of the FCQueue represents the maximum number of requests\n> > in-flight at a time, iow the number of requests queued to the camera\n> > and not yet completed.\n> >\n> > Would it make sense to make this a global camera property and have the\n> > base pipeline handler class defer queueing requests when too many are\n> > in-flight ?\n>\n>\n> In the last call, I brought up this issue. However, it was geared towards a\n> solution to avoid dropping [async] from the mojom interface.\n\nI was clearly distracted, sorry\n\n>\n> The design I have in mind and shared in the call as well is basically that,\n> the PH should be responsible to queue up requests to the IPA - in a way that\n> it doesn't do over-queuing.\n>\n> The way it should be done is - communicating the size of the FCQueue to the\n> PH during init/configure phase (or make a global in mojom interface where\n> it's shared to both parts - not sure yet, I haven't decided). The PH will\n> queue requests to the IPA, increment a counter. We already have a callback\n> from IPA to PH when a frame processing completes - so decrement the counter.\n> The max the counter can reach is FCQueue.size() - beyond that it should just\n> stop queuing and let the request sit in PipelineHandler::pendingRequests_\n> queue. The queuePendingRequests() is a recurring call on every request queue\n> by the application - so as long as the counter < FCQueue.size() - it will\n> keep queuing and stop when it's > FCQueue.size(). As frames get completed,\n> counter will decrement and queuing shall start again.\n\nI agree on the principle, but I wonder if this couldn't be done at the\nbase PipelineHandler class level, where we have a waitingRequests_\nqueue already. Right now it only serves to handle waiting on fences,\nbut it could be probably be instrumented to also take a max number of\nrequests in flight counter.\n\nIf the length of FCQ can be made a property of the camera system (not\nsure if a proper application visibile libcamera::property or else) and\nthe parameter can be shared with the PH base class (it could even be a\nbase class member each derived PH class have to override as an\nopt-in mechanism) the PipelineHandler::doQueueRequests() function\ncan be made aware of this. We have PipelineHandler::completeRequest()\nas well, which is the request completion point where the counter could\nbe decreased and waiting requests queued again.\n\n>\n> > Thanks\n> > j\n> >\n> > > ---\n> > > include/libcamera/ipa/ipu3.mojom | 2 +-\n> > > src/ipa/ipu3/ipu3.cpp | 11 +++++++++--\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 10 +++++++++-\n> > > 3 files changed, 19 insertions(+), 4 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> > > index d1b1c6b8..3faffd9c 100644\n> > > --- a/include/libcamera/ipa/ipu3.mojom\n> > > +++ b/include/libcamera/ipa/ipu3.mojom\n> > > @@ -30,7 +30,7 @@ interface IPAIPU3Interface {\n> > > \tmapBuffers(array buffers);\n> > > \tunmapBuffers(array ids);\n> > >\n> > > -\t[async] queueRequest(uint32 frame, libcamera.ControlList controls);\n> > > +\tqueueRequest(uint32 frame, libcamera.ControlList controls) => (int32 ret);\n> > > \t[async] fillParamsBuffer(uint32 frame, uint32 bufferId);\n> > > \t[async] processStatsBuffer(uint32 frame, int64 frameTimestamp,\n> > > \t\t\t\t uint32 bufferId, libcamera.ControlList sensorControls);\n> > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > > index 1d6ee515..63aa9b56 100644\n> > > --- a/src/ipa/ipu3/ipu3.cpp\n> > > +++ b/src/ipa/ipu3/ipu3.cpp\n> > > @@ -145,7 +145,7 @@ public:\n> > > \tvoid mapBuffers(const std::vector &buffers) override;\n> > > \tvoid unmapBuffers(const std::vector &ids) override;\n> > >\n> > > -\tvoid queueRequest(const uint32_t frame, const ControlList &controls) override;\n> > > +\tint queueRequest(const uint32_t frame, const ControlList &controls) override;\n> > > \tvoid fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override;\n> > > \tvoid processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp,\n> > > \t\t\t\tconst uint32_t bufferId,\n> > > @@ -616,11 +616,18 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> > > *\n> > > * Parse the request to handle any IPA-managed controls that were set from the\n> > > * application such as manual sensor settings.\n> > > + *\n> > > + * \\return 0 if successful, -EPERM otherwise.\n> > > */\n> > > -void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)\n> > > +int IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)\n> > > {\n> > > +\tif (context_.frameContexts.isFull())\n> > > +\t\treturn -EPERM;\n> > > +\n> > > \t/* \\todo Start processing for 'frame' based on 'controls'. */\n> > > \t*context_.frameContexts.get(frame) = { frame, controls };\n> > > +\n> > > +\treturn 0;\n> > > }\n> > >\n> > > /**\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index fd989e61..d69e28d4 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -874,7 +874,15 @@ void IPU3CameraData::queuePendingRequests()\n> > >\n> > > \t\tinfo->rawBuffer = rawBuffer;\n> > >\n> > > -\t\tipa_->queueRequest(info->id, request->controls());\n> > > +\t\t/*\n> > > +\t\t * Queueing request to the IPA can fail in cases like over-flowing\n> > > +\t\t * its queue. Hence, keep the request preserved in pendingRequests_\n> > > +\t\t * if a failure occurs.\n> > > +\t\t */\n> > > +\t\tif (ipa_->queueRequest(info->id, request->controls()) != 0) {\n> > > +\t\t\tframeInfos_.remove(info);\n> > > +\t\t\tbreak;\n> > > +\t\t}\n> > >\n> > > \t\tpendingRequests_.pop();\n> > > \t\tprocessingRequests_.push(request);\n> > > --\n> > > 2.31.1\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 64213BD160\n\tfor ;\n\tFri, 10 Jun 2022 08:52:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C4E0D65632;\n\tFri, 10 Jun 2022 10:52:32 +0200 (CEST)","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 E8E79600F4\n\tfor ;\n\tFri, 10 Jun 2022 10:52:30 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 75A4B240008;\n\tFri, 10 Jun 2022 08:52:30 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654851152;\n\tbh=RQeR7CzA5MkapBg03W0WI2d7CuhavVKR/2mP/4cb4z8=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=NNHYw/zfcsmYnnw6QrRzCeNnU63TfCT3MhUECDq4XhTnx7MJNdFU2jzSg5/s0Fjhe\n\tuPfWg0OrfHRCcvmikvqlCKhoMOtn2ZjzBlIr4EFuxa47dRZlUMVOUzE+g4YriA/1yT\n\toJn4B04Pd6tEmYEMPmOMKhF0n0o8Zqyrjr1hsWBR5RG9KwhOxzuhqd1rNdardfKN0O\n\tpztHbeYZpx05n9pv7Kew5iO94t65OaL2PjlntDuHrAsyOJVY4Nkz8XMKOtEOY2Dn4+\n\t8+a7+bx1O+sUaYS/OEPZsSR95/KjCiqZf0AWZBzfU7qlH0mqG8Ot7ByoLRNYy9gCNY\n\tiKfS7q7lMCy1g==","Date":"Fri, 10 Jun 2022 10:52:28 +0200","To":"Umang Jain ","Message-ID":"<20220610085228.7xxk6s3hcjmf7ber@uno.localdomain>","References":"<20220603132259.188845-1-umang.jain@ideasonboard.com>\n\t<20220603132259.188845-4-umang.jain@ideasonboard.com>\n\t<20220610074905.lvgqfe5mekruav2y@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v1 3/4] ipa: ipu3: Prevent over\n\tqueuing of requests","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23380,"web_url":"https://patchwork.libcamera.org/comment/23380/","msgid":"","date":"2022-06-10T09:03:35","subject":"Re: [libcamera-devel] [PATCH v1 3/4] ipa: ipu3: Prevent over\n\tqueuing of requests","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 6/10/22 10:52, Jacopo Mondi wrote:\n> Hi Umang,\n>\n> On Fri, Jun 10, 2022 at 10:32:03AM +0200, Umang Jain wrote:\n>> Hi Jacopo,\n>>\n>> On 6/10/22 09:49, Jacopo Mondi wrote:\n>>> Hi Umang,\n>>>\n>>> On Fri, Jun 03, 2022 at 03:22:58PM +0200, Umang Jain via libcamera-devel wrote:\n>>>> The IPAIPU3 processes each IPAFrameContext present in the FCQueue\n>>>> ring buffer. If the application queues the requests at a pace where\n>>>> IPAFrameContext(s) can get potentially over-written without getting\n>>>> processed by the IPA, it would lead to un-desirable effects.\n>>>>\n>>>> Hence, inspect the FCQueue if it is full and reject any further\n>>>> queuing of requests. This requires changes to the IPAIPU3 mojom\n>>>> interface. The IPAIPU3::queueRequest() cannot be [async] anymore\n>>>> since we need to return a success/failure value.\n>>>>\n>>>> The rejection by the IPA doesn't mean it cannot be queued\n>>>> later. The request is still preserved in\n>>>> IPU3CameraData::pendingRequests_. It shall be queued sequentially\n>>>> on subsequent calls to IPU3CameraData::queuePendingRequests().\n>>>>\n>>>> Signed-off-by: Umang Jain \n>>>> ---\n>>>> I am not super-happy with changing the interface and dropping [async]\n>>>> but this is what I've for now. Would appreciate any other ideas...\n>>> I feel like this change handles the issue at the very bottom of the\n>>> call stack, once a request has been queued from the ph to the IPA.\n>>>\n>>> I wonder if we shouldn't try to tackle the problem from the top\n>>> instead, which means at the time a request is queued to the camera by\n>>> the application.\n>>>\n>>> The lenght of the FCQueue represents the maximum number of requests\n>>> in-flight at a time, iow the number of requests queued to the camera\n>>> and not yet completed.\n>>>\n>>> Would it make sense to make this a global camera property and have the\n>>> base pipeline handler class defer queueing requests when too many are\n>>> in-flight ?\n>>\n>> In the last call, I brought up this issue. However, it was geared towards a\n>> solution to avoid dropping [async] from the mojom interface.\n> I was clearly distracted, sorry\n>\n>> The design I have in mind and shared in the call as well is basically that,\n>> the PH should be responsible to queue up requests to the IPA - in a way that\n>> it doesn't do over-queuing.\n>>\n>> The way it should be done is - communicating the size of the FCQueue to the\n>> PH during init/configure phase (or make a global in mojom interface where\n>> it's shared to both parts - not sure yet, I haven't decided). The PH will\n>> queue requests to the IPA, increment a counter. We already have a callback\n>> from IPA to PH when a frame processing completes - so decrement the counter.\n>> The max the counter can reach is FCQueue.size() - beyond that it should just\n>> stop queuing and let the request sit in PipelineHandler::pendingRequests_\n>> queue. The queuePendingRequests() is a recurring call on every request queue\n>> by the application - so as long as the counter < FCQueue.size() - it will\n>> keep queuing and stop when it's > FCQueue.size(). As frames get completed,\n>> counter will decrement and queuing shall start again.\n> I agree on the principle, but I wonder if this couldn't be done at the\n> base PipelineHandler class level, where we have a waitingRequests_\n> queue already. Right now it only serves to handle waiting on fences,\n> but it could be probably be instrumented to also take a max number of\n> requests in flight counter.\n>\n> If the length of FCQ can be made a property of the camera system (not\n> sure if a proper application visibile libcamera::property or else) and\n> the parameter can be shared with the PH base class (it could even be a\n> base class member each derived PH class have to override as an\n> opt-in mechanism) the PipelineHandler::doQueueRequests() function\n> can be made aware of this. We have PipelineHandler::completeRequest()\n> as well, which is the request completion point where the counter could\n> be decreased and waiting requests queued again.\n\n\n2 Points:\n\n- I haven't had a look at the PH base for this yet. But reading from \nyour explanation (thanks for the details), it seems it should be quite \ndo-able. So yes, we have two \"similar\" options now\n\n- Next points is basically its PH \"base\". So \"generic\" stuff - which \nmight apply to other PH and IPAs as well. I don't want to go into doing \nit \"generically\" yet but touching the PH base for FCQueue (which is \nstill IPU3 sepcific) seems a violation of the layers to me. So I am a \nbit resistant going in that direction (for now).\n\nOne option could be - to implement the solution PipelinehandlerIPU3 \nclass for now and include a \\todo which basically summarises your \nsolution in the last reply. When we start moving these parts from the PH \n/ IPA to more \"generic\" location(s)- we can address that \\todo as well. \nThis way we don't have things spread out between specific and generic, I \nthink. The principle in both is similar basically. What do you think ?\n\n>\n>>> Thanks\n>>> j\n>>>\n>>>> ---\n>>>> include/libcamera/ipa/ipu3.mojom | 2 +-\n>>>> src/ipa/ipu3/ipu3.cpp | 11 +++++++++--\n>>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 10 +++++++++-\n>>>> 3 files changed, 19 insertions(+), 4 deletions(-)\n>>>>\n>>>> diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n>>>> index d1b1c6b8..3faffd9c 100644\n>>>> --- a/include/libcamera/ipa/ipu3.mojom\n>>>> +++ b/include/libcamera/ipa/ipu3.mojom\n>>>> @@ -30,7 +30,7 @@ interface IPAIPU3Interface {\n>>>> \tmapBuffers(array buffers);\n>>>> \tunmapBuffers(array ids);\n>>>>\n>>>> -\t[async] queueRequest(uint32 frame, libcamera.ControlList controls);\n>>>> +\tqueueRequest(uint32 frame, libcamera.ControlList controls) => (int32 ret);\n>>>> \t[async] fillParamsBuffer(uint32 frame, uint32 bufferId);\n>>>> \t[async] processStatsBuffer(uint32 frame, int64 frameTimestamp,\n>>>> \t\t\t\t uint32 bufferId, libcamera.ControlList sensorControls);\n>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n>>>> index 1d6ee515..63aa9b56 100644\n>>>> --- a/src/ipa/ipu3/ipu3.cpp\n>>>> +++ b/src/ipa/ipu3/ipu3.cpp\n>>>> @@ -145,7 +145,7 @@ public:\n>>>> \tvoid mapBuffers(const std::vector &buffers) override;\n>>>> \tvoid unmapBuffers(const std::vector &ids) override;\n>>>>\n>>>> -\tvoid queueRequest(const uint32_t frame, const ControlList &controls) override;\n>>>> +\tint queueRequest(const uint32_t frame, const ControlList &controls) override;\n>>>> \tvoid fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override;\n>>>> \tvoid processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp,\n>>>> \t\t\t\tconst uint32_t bufferId,\n>>>> @@ -616,11 +616,18 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n>>>> *\n>>>> * Parse the request to handle any IPA-managed controls that were set from the\n>>>> * application such as manual sensor settings.\n>>>> + *\n>>>> + * \\return 0 if successful, -EPERM otherwise.\n>>>> */\n>>>> -void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)\n>>>> +int IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)\n>>>> {\n>>>> +\tif (context_.frameContexts.isFull())\n>>>> +\t\treturn -EPERM;\n>>>> +\n>>>> \t/* \\todo Start processing for 'frame' based on 'controls'. */\n>>>> \t*context_.frameContexts.get(frame) = { frame, controls };\n>>>> +\n>>>> +\treturn 0;\n>>>> }\n>>>>\n>>>> /**\n>>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>> index fd989e61..d69e28d4 100644\n>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>> @@ -874,7 +874,15 @@ void IPU3CameraData::queuePendingRequests()\n>>>>\n>>>> \t\tinfo->rawBuffer = rawBuffer;\n>>>>\n>>>> -\t\tipa_->queueRequest(info->id, request->controls());\n>>>> +\t\t/*\n>>>> +\t\t * Queueing request to the IPA can fail in cases like over-flowing\n>>>> +\t\t * its queue. Hence, keep the request preserved in pendingRequests_\n>>>> +\t\t * if a failure occurs.\n>>>> +\t\t */\n>>>> +\t\tif (ipa_->queueRequest(info->id, request->controls()) != 0) {\n>>>> +\t\t\tframeInfos_.remove(info);\n>>>> +\t\t\tbreak;\n>>>> +\t\t}\n>>>>\n>>>> \t\tpendingRequests_.pop();\n>>>> \t\tprocessingRequests_.push(request);\n>>>> --\n>>>> 2.31.1\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 C296EBD161\n\tfor ;\n\tFri, 10 Jun 2022 09:03:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0E27C65635;\n\tFri, 10 Jun 2022 11:03:40 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EDF3D600F4\n\tfor ;\n\tFri, 10 Jun 2022 11:03:38 +0200 (CEST)","from [192.168.101.240] (217.red-95-124-192.dynamicip.rima-tde.net\n\t[95.124.192.217])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 53B2A526;\n\tFri, 10 Jun 2022 11:03:38 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654851820;\n\tbh=/NTW0NUihPuEc+FMY6VmbZ3Ac7Fa1CK/4VY89YglkCM=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=yUAB7RSuiE7cZRyRxg9IzMZeuk6vlJ4xhrRTwgd4iMnbeQ93iehEZ9IiE1X+QrVGL\n\tE8hC62FrPcMgU2YZpDRO0mY8juvkmL8eYPi1Cg1D9tw3HuHgc13s3C3hfJqDlHrqTo\n\t0xuGfV/RE3pseq4cL+VPrNUQyQXRlvDiWtPpWuMmmgEp0fyiZbUbU+qt7NkUuiQv5h\n\tuMFHi2ozQOyZ45hxx52QYqeINiH77LKOPLXVD+lbKOgGIz9jF8fYt2a3+KdurfCHTb\n\tKWp8jRHfIWndSnH3yvJ0UlIqA1MsmApsB5R+Jb2RzyFdHQVdLADOLcyaTrrngpdSUr\n\ttq3gBOimvR9jg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1654851818;\n\tbh=/NTW0NUihPuEc+FMY6VmbZ3Ac7Fa1CK/4VY89YglkCM=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=vAQa6vRWwAGW60nn5ReF+njArUYK0oazHg35/3u/DnlldMoqeV966bp5loe0pf8b2\n\t1QQ3DjNNQVY/YeDKBz+0970P3iguP0SQKU7dy31rVnUqU5FR7wsoNMmiLqnt3nVRON\n\ttjFAUceQD44iyTCG5ddeGQ+Yk2RxC/76HAj9ppUY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"vAQa6vRW\"; dkim-atps=neutral","Message-ID":"","Date":"Fri, 10 Jun 2022 11:03:35 +0200","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Jacopo Mondi ","References":"<20220603132259.188845-1-umang.jain@ideasonboard.com>\n\t<20220603132259.188845-4-umang.jain@ideasonboard.com>\n\t<20220610074905.lvgqfe5mekruav2y@uno.localdomain>\n\t\n\t<20220610085228.7xxk6s3hcjmf7ber@uno.localdomain>","In-Reply-To":"<20220610085228.7xxk6s3hcjmf7ber@uno.localdomain>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v1 3/4] ipa: ipu3: Prevent over\n\tqueuing of requests","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","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23381,"web_url":"https://patchwork.libcamera.org/comment/23381/","msgid":"<20220610092648.hnouy5qfjlmeskyk@uno.localdomain>","date":"2022-06-10T09:26:48","subject":"Re: [libcamera-devel] [PATCH v1 3/4] ipa: ipu3: Prevent over\n\tqueuing of requests","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n\nOn Fri, Jun 10, 2022 at 11:03:35AM +0200, Umang Jain wrote:\n> Hi Jacopo,\n>\n> On 6/10/22 10:52, Jacopo Mondi wrote:\n> > Hi Umang,\n> >\n> > On Fri, Jun 10, 2022 at 10:32:03AM +0200, Umang Jain wrote:\n> > > Hi Jacopo,\n> > >\n> > > On 6/10/22 09:49, Jacopo Mondi wrote:\n> > > > Hi Umang,\n> > > >\n> > > > On Fri, Jun 03, 2022 at 03:22:58PM +0200, Umang Jain via libcamera-devel wrote:\n> > > > > The IPAIPU3 processes each IPAFrameContext present in the FCQueue\n> > > > > ring buffer. If the application queues the requests at a pace where\n> > > > > IPAFrameContext(s) can get potentially over-written without getting\n> > > > > processed by the IPA, it would lead to un-desirable effects.\n> > > > >\n> > > > > Hence, inspect the FCQueue if it is full and reject any further\n> > > > > queuing of requests. This requires changes to the IPAIPU3 mojom\n> > > > > interface. The IPAIPU3::queueRequest() cannot be [async] anymore\n> > > > > since we need to return a success/failure value.\n> > > > >\n> > > > > The rejection by the IPA doesn't mean it cannot be queued\n> > > > > later. The request is still preserved in\n> > > > > IPU3CameraData::pendingRequests_. It shall be queued sequentially\n> > > > > on subsequent calls to IPU3CameraData::queuePendingRequests().\n> > > > >\n> > > > > Signed-off-by: Umang Jain \n> > > > > ---\n> > > > > I am not super-happy with changing the interface and dropping [async]\n> > > > > but this is what I've for now. Would appreciate any other ideas...\n> > > > I feel like this change handles the issue at the very bottom of the\n> > > > call stack, once a request has been queued from the ph to the IPA.\n> > > >\n> > > > I wonder if we shouldn't try to tackle the problem from the top\n> > > > instead, which means at the time a request is queued to the camera by\n> > > > the application.\n> > > >\n> > > > The lenght of the FCQueue represents the maximum number of requests\n> > > > in-flight at a time, iow the number of requests queued to the camera\n> > > > and not yet completed.\n> > > >\n> > > > Would it make sense to make this a global camera property and have the\n> > > > base pipeline handler class defer queueing requests when too many are\n> > > > in-flight ?\n> > >\n> > > In the last call, I brought up this issue. However, it was geared towards a\n> > > solution to avoid dropping [async] from the mojom interface.\n> > I was clearly distracted, sorry\n> >\n> > > The design I have in mind and shared in the call as well is basically that,\n> > > the PH should be responsible to queue up requests to the IPA - in a way that\n> > > it doesn't do over-queuing.\n> > >\n> > > The way it should be done is - communicating the size of the FCQueue to the\n> > > PH during init/configure phase (or make a global in mojom interface where\n> > > it's shared to both parts - not sure yet, I haven't decided). The PH will\n> > > queue requests to the IPA, increment a counter. We already have a callback\n> > > from IPA to PH when a frame processing completes - so decrement the counter.\n> > > The max the counter can reach is FCQueue.size() - beyond that it should just\n> > > stop queuing and let the request sit in PipelineHandler::pendingRequests_\n> > > queue. The queuePendingRequests() is a recurring call on every request queue\n> > > by the application - so as long as the counter < FCQueue.size() - it will\n> > > keep queuing and stop when it's > FCQueue.size(). As frames get completed,\n> > > counter will decrement and queuing shall start again.\n> > I agree on the principle, but I wonder if this couldn't be done at the\n> > base PipelineHandler class level, where we have a waitingRequests_\n> > queue already. Right now it only serves to handle waiting on fences,\n> > but it could be probably be instrumented to also take a max number of\n> > requests in flight counter.\n> >\n> > If the length of FCQ can be made a property of the camera system (not\n> > sure if a proper application visibile libcamera::property or else) and\n> > the parameter can be shared with the PH base class (it could even be a\n> > base class member each derived PH class have to override as an\n> > opt-in mechanism) the PipelineHandler::doQueueRequests() function\n> > can be made aware of this. We have PipelineHandler::completeRequest()\n> > as well, which is the request completion point where the counter could\n> > be decreased and waiting requests queued again.\n>\n>\n> 2 Points:\n>\n> - I haven't had a look at the PH base for this yet. But reading from your\n> explanation (thanks for the details), it seems it should be quite do-able.\n> So yes, we have two \"similar\" options now\n>\n> - Next points is basically its PH \"base\". So \"generic\" stuff - which might\n> apply to other PH and IPAs as well. I don't want to go into doing it\n> \"generically\" yet but touching the PH base for FCQueue (which is still IPU3\n> sepcific) seems a violation of the layers to me. So I am a bit resistant\n> going in that direction (for now).\n\nI do expect all pipeline handlers would potentially be interested in\nexposing a max number of in-flight requests and to have a mechanism\nthat allows them automatically be guaranteed not to receive more, even\nmore if they don't have to do anything to have that realized :)\n\nFor IPU3 this is currently the size of the FCQ too, for other\nplatforms might be different and could simply be set to the number of\nbuffers requested on the video device.\n\n>\n> One option could be - to implement the solution PipelinehandlerIPU3 class\n> for now and include a \\todo which basically summarises your solution in the\n> last reply. When we start moving these parts from the PH / IPA to more\n> \"generic\" location(s)- we can address that \\todo as well. This way we don't\n> have things spread out between specific and generic, I think. The principle\n> in both is similar basically. What do you think ?\n>\n\nDoing the work in the IPU3 ph to then have to re-implement it in the\nbase class seems like a work duplication to me, if the change can be\nself-contained and implemented in the base class already with the same\neffort.\n\nThe mechanism could be made opt-in, conditional to the presence of a\ncamera property or on the value of a base class member field which\nderived ph will override.\n\nI would be happy to help if you want this as not part of this series.\n\n> >\n> > > > Thanks\n> > > > j\n> > > >\n> > > > > ---\n> > > > > include/libcamera/ipa/ipu3.mojom | 2 +-\n> > > > > src/ipa/ipu3/ipu3.cpp | 11 +++++++++--\n> > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 10 +++++++++-\n> > > > > 3 files changed, 19 insertions(+), 4 deletions(-)\n> > > > >\n> > > > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> > > > > index d1b1c6b8..3faffd9c 100644\n> > > > > --- a/include/libcamera/ipa/ipu3.mojom\n> > > > > +++ b/include/libcamera/ipa/ipu3.mojom\n> > > > > @@ -30,7 +30,7 @@ interface IPAIPU3Interface {\n> > > > > \tmapBuffers(array buffers);\n> > > > > \tunmapBuffers(array ids);\n> > > > >\n> > > > > -\t[async] queueRequest(uint32 frame, libcamera.ControlList controls);\n> > > > > +\tqueueRequest(uint32 frame, libcamera.ControlList controls) => (int32 ret);\n> > > > > \t[async] fillParamsBuffer(uint32 frame, uint32 bufferId);\n> > > > > \t[async] processStatsBuffer(uint32 frame, int64 frameTimestamp,\n> > > > > \t\t\t\t uint32 bufferId, libcamera.ControlList sensorControls);\n> > > > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > > > > index 1d6ee515..63aa9b56 100644\n> > > > > --- a/src/ipa/ipu3/ipu3.cpp\n> > > > > +++ b/src/ipa/ipu3/ipu3.cpp\n> > > > > @@ -145,7 +145,7 @@ public:\n> > > > > \tvoid mapBuffers(const std::vector &buffers) override;\n> > > > > \tvoid unmapBuffers(const std::vector &ids) override;\n> > > > >\n> > > > > -\tvoid queueRequest(const uint32_t frame, const ControlList &controls) override;\n> > > > > +\tint queueRequest(const uint32_t frame, const ControlList &controls) override;\n> > > > > \tvoid fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override;\n> > > > > \tvoid processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp,\n> > > > > \t\t\t\tconst uint32_t bufferId,\n> > > > > @@ -616,11 +616,18 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> > > > > *\n> > > > > * Parse the request to handle any IPA-managed controls that were set from the\n> > > > > * application such as manual sensor settings.\n> > > > > + *\n> > > > > + * \\return 0 if successful, -EPERM otherwise.\n> > > > > */\n> > > > > -void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)\n> > > > > +int IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)\n> > > > > {\n> > > > > +\tif (context_.frameContexts.isFull())\n> > > > > +\t\treturn -EPERM;\n> > > > > +\n> > > > > \t/* \\todo Start processing for 'frame' based on 'controls'. */\n> > > > > \t*context_.frameContexts.get(frame) = { frame, controls };\n> > > > > +\n> > > > > +\treturn 0;\n> > > > > }\n> > > > >\n> > > > > /**\n> > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > index fd989e61..d69e28d4 100644\n> > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > @@ -874,7 +874,15 @@ void IPU3CameraData::queuePendingRequests()\n> > > > >\n> > > > > \t\tinfo->rawBuffer = rawBuffer;\n> > > > >\n> > > > > -\t\tipa_->queueRequest(info->id, request->controls());\n> > > > > +\t\t/*\n> > > > > +\t\t * Queueing request to the IPA can fail in cases like over-flowing\n> > > > > +\t\t * its queue. Hence, keep the request preserved in pendingRequests_\n> > > > > +\t\t * if a failure occurs.\n> > > > > +\t\t */\n> > > > > +\t\tif (ipa_->queueRequest(info->id, request->controls()) != 0) {\n> > > > > +\t\t\tframeInfos_.remove(info);\n> > > > > +\t\t\tbreak;\n> > > > > +\t\t}\n> > > > >\n> > > > > \t\tpendingRequests_.pop();\n> > > > > \t\tprocessingRequests_.push(request);\n> > > > > --\n> > > > > 2.31.1\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 EF433BD160\n\tfor ;\n\tFri, 10 Jun 2022 09:26:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4917365635;\n\tFri, 10 Jun 2022 11:26:52 +0200 (CEST)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::224])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3E973600F4\n\tfor ;\n\tFri, 10 Jun 2022 11:26:50 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id B72C9E000E;\n\tFri, 10 Jun 2022 09:26:49 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1654853212;\n\tbh=JhYmyxngLLGX+WmTQL0lvylKGk5f/ua0LXvm2lEFcyA=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=4CPhisJ3EDp7SpV73rvCysi0xFyCn79q5cseCPZ0UT7BZGVxcpenWLy4eSqlcEN0z\n\tAHNOsZjUmE5M1RCfRmflVwZrSwXCOUl2ehMa8gnK0sCNbWKJibNvueiAWY7j+Xhe2p\n\ti/WHochbZu+3NGgQlYCwJ+f9hCre/XlfOwVV+vWydrCYybsdaspjEXe4f1rpHCgY3f\n\txfHefOWe+4V+T7UA09pfbp8yJhrmJZHEfRe5yLDdl4jC0ZvbLY/dDpT3BVKm3YlKFZ\n\tlJJ+13YIm1Abn0/emhbTt9VOBTbRAs+o7/Rpav94ptTvtTJfw5vQOI7VUS+o4dK8i6\n\t/rG91TLyWWdEQ==","Date":"Fri, 10 Jun 2022 11:26:48 +0200","To":"Umang Jain ","Message-ID":"<20220610092648.hnouy5qfjlmeskyk@uno.localdomain>","References":"<20220603132259.188845-1-umang.jain@ideasonboard.com>\n\t<20220603132259.188845-4-umang.jain@ideasonboard.com>\n\t<20220610074905.lvgqfe5mekruav2y@uno.localdomain>\n\t\n\t<20220610085228.7xxk6s3hcjmf7ber@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v1 3/4] ipa: ipu3: Prevent over\n\tqueuing of requests","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23406,"web_url":"https://patchwork.libcamera.org/comment/23406/","msgid":"<165533358760.2586493.5965108113999082667@Monstersaurus>","date":"2022-06-15T22:53:07","subject":"Re: [libcamera-devel] [PATCH v1 3/4] ipa: ipu3: Prevent over\n\tqueuing of requests","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi via libcamera-devel (2022-06-10 10:26:48)\n> Hi Umang,\n> \n> On Fri, Jun 10, 2022 at 11:03:35AM +0200, Umang Jain wrote:\n> > Hi Jacopo,\n> >\n> > On 6/10/22 10:52, Jacopo Mondi wrote:\n> > > Hi Umang,\n> > >\n> > > On Fri, Jun 10, 2022 at 10:32:03AM +0200, Umang Jain wrote:\n> > > > Hi Jacopo,\n> > > >\n> > > > On 6/10/22 09:49, Jacopo Mondi wrote:\n> > > > > Hi Umang,\n> > > > >\n> > > > > On Fri, Jun 03, 2022 at 03:22:58PM +0200, Umang Jain via libcamera-devel wrote:\n> > > > > > The IPAIPU3 processes each IPAFrameContext present in the FCQueue\n> > > > > > ring buffer. If the application queues the requests at a pace where\n> > > > > > IPAFrameContext(s) can get potentially over-written without getting\n> > > > > > processed by the IPA, it would lead to un-desirable effects.\n> > > > > >\n> > > > > > Hence, inspect the FCQueue if it is full and reject any further\n> > > > > > queuing of requests. This requires changes to the IPAIPU3 mojom\n> > > > > > interface. The IPAIPU3::queueRequest() cannot be [async] anymore\n> > > > > > since we need to return a success/failure value.\n> > > > > >\n> > > > > > The rejection by the IPA doesn't mean it cannot be queued\n> > > > > > later. The request is still preserved in\n> > > > > > IPU3CameraData::pendingRequests_. It shall be queued sequentially\n> > > > > > on subsequent calls to IPU3CameraData::queuePendingRequests().\n> > > > > >\n> > > > > > Signed-off-by: Umang Jain \n> > > > > > ---\n> > > > > > I am not super-happy with changing the interface and dropping [async]\n> > > > > > but this is what I've for now. Would appreciate any other ideas...\n> > > > > I feel like this change handles the issue at the very bottom of the\n> > > > > call stack, once a request has been queued from the ph to the IPA.\n> > > > >\n> > > > > I wonder if we shouldn't try to tackle the problem from the top\n> > > > > instead, which means at the time a request is queued to the camera by\n> > > > > the application.\n> > > > >\n> > > > > The lenght of the FCQueue represents the maximum number of requests\n> > > > > in-flight at a time, iow the number of requests queued to the camera\n> > > > > and not yet completed.\n> > > > >\n> > > > > Would it make sense to make this a global camera property and have the\n> > > > > base pipeline handler class defer queueing requests when too many are\n> > > > > in-flight ?\n> > > >\n> > > > In the last call, I brought up this issue. However, it was geared towards a\n> > > > solution to avoid dropping [async] from the mojom interface.\n> > > I was clearly distracted, sorry\n> > >\n> > > > The design I have in mind and shared in the call as well is basically that,\n> > > > the PH should be responsible to queue up requests to the IPA - in a way that\n> > > > it doesn't do over-queuing.\n> > > >\n> > > > The way it should be done is - communicating the size of the FCQueue to the\n> > > > PH during init/configure phase (or make a global in mojom interface where\n> > > > it's shared to both parts - not sure yet, I haven't decided). The PH will\n> > > > queue requests to the IPA, increment a counter. We already have a callback\n> > > > from IPA to PH when a frame processing completes - so decrement the counter.\n> > > > The max the counter can reach is FCQueue.size() - beyond that it should just\n> > > > stop queuing and let the request sit in PipelineHandler::pendingRequests_\n> > > > queue. The queuePendingRequests() is a recurring call on every request queue\n> > > > by the application - so as long as the counter < FCQueue.size() - it will\n> > > > keep queuing and stop when it's > FCQueue.size(). As frames get completed,\n> > > > counter will decrement and queuing shall start again.\n> > > I agree on the principle, but I wonder if this couldn't be done at the\n> > > base PipelineHandler class level, where we have a waitingRequests_\n> > > queue already. Right now it only serves to handle waiting on fences,\n> > > but it could be probably be instrumented to also take a max number of\n> > > requests in flight counter.\n> > >\n> > > If the length of FCQ can be made a property of the camera system (not\n> > > sure if a proper application visibile libcamera::property or else) and\n> > > the parameter can be shared with the PH base class (it could even be a\n> > > base class member each derived PH class have to override as an\n> > > opt-in mechanism) the PipelineHandler::doQueueRequests() function\n> > > can be made aware of this. We have PipelineHandler::completeRequest()\n> > > as well, which is the request completion point where the counter could\n> > > be decreased and waiting requests queued again.\n> >\n> >\n> > 2 Points:\n> >\n> > - I haven't had a look at the PH base for this yet. But reading from your\n> > explanation (thanks for the details), it seems it should be quite do-able.\n> > So yes, we have two \"similar\" options now\n> >\n> > - Next points is basically its PH \"base\". So \"generic\" stuff - which might\n> > apply to other PH and IPAs as well. I don't want to go into doing it\n> > \"generically\" yet but touching the PH base for FCQueue (which is still IPU3\n> > sepcific) seems a violation of the layers to me. So I am a bit resistant\n> > going in that direction (for now).\n> \n> I do expect all pipeline handlers would potentially be interested in\n> exposing a max number of in-flight requests and to have a mechanism\n> that allows them automatically be guaranteed not to receive more, even\n> more if they don't have to do anything to have that realized :)\n> \n> For IPU3 this is currently the size of the FCQ too, for other\n> platforms might be different and could simply be set to the number of\n> buffers requested on the video device.\n> \n> >\n> > One option could be - to implement the solution PipelinehandlerIPU3 class\n> > for now and include a \\todo which basically summarises your solution in the\n> > last reply. When we start moving these parts from the PH / IPA to more\n> > \"generic\" location(s)- we can address that \\todo as well. This way we don't\n> > have things spread out between specific and generic, I think. The principle\n> > in both is similar basically. What do you think ?\n> >\n> \n> Doing the work in the IPU3 ph to then have to re-implement it in the\n> base class seems like a work duplication to me, if the change can be\n> self-contained and implemented in the base class already with the same\n> effort.\n> \n> The mechanism could be made opt-in, conditional to the presence of a\n> camera property or on the value of a base class member field which\n> derived ph will override.\n> \n> I would be happy to help if you want this as not part of this series.\n\nPreventing overflowing the IPA using the existing queue at the PH sounds\nquite optimal to me too.\n\nLet me know if I can help there in anyway too - or if we need to add it\nto a discussion point.\n\nIf we 'know' that we'll prevent overflowing the FCQ, we don't have to\nhandle it in this IPU3 series.\n\nI wonder if there are underflow issues still though...\n\n--\nKieran\n\n\n> \n> > >\n> > > > > Thanks\n> > > > > j\n> > > > >\n> > > > > > ---\n> > > > > > include/libcamera/ipa/ipu3.mojom | 2 +-\n> > > > > > src/ipa/ipu3/ipu3.cpp | 11 +++++++++--\n> > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 10 +++++++++-\n> > > > > > 3 files changed, 19 insertions(+), 4 deletions(-)\n> > > > > >\n> > > > > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom\n> > > > > > index d1b1c6b8..3faffd9c 100644\n> > > > > > --- a/include/libcamera/ipa/ipu3.mojom\n> > > > > > +++ b/include/libcamera/ipa/ipu3.mojom\n> > > > > > @@ -30,7 +30,7 @@ interface IPAIPU3Interface {\n> > > > > > mapBuffers(array buffers);\n> > > > > > unmapBuffers(array ids);\n> > > > > >\n> > > > > > - [async] queueRequest(uint32 frame, libcamera.ControlList controls);\n> > > > > > + queueRequest(uint32 frame, libcamera.ControlList controls) => (int32 ret);\n> > > > > > [async] fillParamsBuffer(uint32 frame, uint32 bufferId);\n> > > > > > [async] processStatsBuffer(uint32 frame, int64 frameTimestamp,\n> > > > > > uint32 bufferId, libcamera.ControlList sensorControls);\n> > > > > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp\n> > > > > > index 1d6ee515..63aa9b56 100644\n> > > > > > --- a/src/ipa/ipu3/ipu3.cpp\n> > > > > > +++ b/src/ipa/ipu3/ipu3.cpp\n> > > > > > @@ -145,7 +145,7 @@ public:\n> > > > > > void mapBuffers(const std::vector &buffers) override;\n> > > > > > void unmapBuffers(const std::vector &ids) override;\n> > > > > >\n> > > > > > - void queueRequest(const uint32_t frame, const ControlList &controls) override;\n> > > > > > + int queueRequest(const uint32_t frame, const ControlList &controls) override;\n> > > > > > void fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override;\n> > > > > > void processStatsBuffer(const uint32_t frame, const int64_t frameTimestamp,\n> > > > > > const uint32_t bufferId,\n> > > > > > @@ -616,11 +616,18 @@ void IPAIPU3::processStatsBuffer(const uint32_t frame,\n> > > > > > *\n> > > > > > * Parse the request to handle any IPA-managed controls that were set from the\n> > > > > > * application such as manual sensor settings.\n> > > > > > + *\n> > > > > > + * \\return 0 if successful, -EPERM otherwise.\n> > > > > > */\n> > > > > > -void IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)\n> > > > > > +int IPAIPU3::queueRequest(const uint32_t frame, const ControlList &controls)\n> > > > > > {\n> > > > > > + if (context_.frameContexts.isFull())\n> > > > > > + return -EPERM;\n> > > > > > +\n> > > > > > /* \\todo Start processing for 'frame' based on 'controls'. */\n> > > > > > *context_.frameContexts.get(frame) = { frame, controls };\n> > > > > > +\n> > > > > > + return 0;\n> > > > > > }\n> > > > > >\n> > > > > > /**\n> > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > index fd989e61..d69e28d4 100644\n> > > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > @@ -874,7 +874,15 @@ void IPU3CameraData::queuePendingRequests()\n> > > > > >\n> > > > > > info->rawBuffer = rawBuffer;\n> > > > > >\n> > > > > > - ipa_->queueRequest(info->id, request->controls());\n> > > > > > + /*\n> > > > > > + * Queueing request to the IPA can fail in cases like over-flowing\n> > > > > > + * its queue. Hence, keep the request preserved in pendingRequests_\n> > > > > > + * if a failure occurs.\n> > > > > > + */\n> > > > > > + if (ipa_->queueRequest(info->id, request->controls()) != 0) {\n> > > > > > + frameInfos_.remove(info);\n> > > > > > + break;\n> > > > > > + }\n> > > > > >\n> > > > > > pendingRequests_.pop();\n> > > > > > processingRequests_.push(request);\n> > > > > > --\n> > > > > > 2.31.1\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 DD024BD808\n\tfor ;\n\tWed, 15 Jun 2022 22:53:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F2AB165635;\n\tThu, 16 Jun 2022 00:53:12 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A1E32601EF\n\tfor ;\n\tThu, 16 Jun 2022 00:53:10 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1A6B8415;\n\tThu, 16 Jun 2022 00:53:10 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1655333593;\n\tbh=0RWRwvVXFhswlvmWZsITuE2XrnTxIoatpJRHsAhYxgM=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=b/lR0nBpx2Af3/B8y5Ia9yrnG8ZK7O5gWnAN3oC4J9Y/nL4NsI+dLj1Y4DidhpHZE\n\t1Yp+z46QKYcbA+ztl9DYrBLLzjAGoMKv2VEC4JSnv6thSpuJBW9luUHhXf2b001uoO\n\tGhDo/Cnejg3L2F4jKy/LvKCYDu4RLJXk3iTJGSpu1B3ajYHHM3QGa9A2G0wTwEUetc\n\t77VcsvQT2mhbN1j6wNmqQXXwKgVhgFdGAPxMBDREzo5SiqNdDyTxAyh+7PjCb1dMCy\n\tcT+8/GBUy07snAl4/d+Bycs/czJnQLtjtM6QZFwMgYjWJRU7+ggKWAQ3TkKQJBOrk8\n\txzm29gn8CIMLw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1655333590;\n\tbh=0RWRwvVXFhswlvmWZsITuE2XrnTxIoatpJRHsAhYxgM=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=rL2XKLN+/I3+D+zPzdajKnpWMLlCqdBuXaDyA9okVywJ/Y1+/NhpIux2ALS88tqkN\n\tcxFv6BQMDZPRCf69N7+BR6fi+OUvY9huafUeSLsvMTg6rElK2S+O5hjiQpAKnGS+Vi\n\tq6hWLMmKnIs0NoCFBliKoQ95TDHYnRJZnhHq4h2E="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"rL2XKLN+\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220610092648.hnouy5qfjlmeskyk@uno.localdomain>","References":"<20220603132259.188845-1-umang.jain@ideasonboard.com>\n\t<20220603132259.188845-4-umang.jain@ideasonboard.com>\n\t<20220610074905.lvgqfe5mekruav2y@uno.localdomain>\n\t\n\t<20220610085228.7xxk6s3hcjmf7ber@uno.localdomain>\n\t\n\t<20220610092648.hnouy5qfjlmeskyk@uno.localdomain>","To":"Jacopo Mondi , Jacopo Mondi via libcamera-devel\n\t, \n\tUmang Jain ","Date":"Wed, 15 Jun 2022 23:53:07 +0100","Message-ID":"<165533358760.2586493.5965108113999082667@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v1 3/4] ipa: ipu3: Prevent over\n\tqueuing of requests","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]