[{"id":34734,"web_url":"https://patchwork.libcamera.org/comment/34734/","msgid":"","date":"2025-06-30T10:11:31","subject":"Re: [PATCH v1 2/6] libcamera: pipeline_handler: Allow to limit the\n\tnumber of queued requests","submitter":{"id":232,"url":"https://patchwork.libcamera.org/api/people/232/","name":"Umang Jain","email":"uajain@igalia.com"},"content":"Hi Stefan,\n\nOn Mon, Jun 30, 2025 at 10:11:17AM +0200, Stefan Klug wrote:\n> Add a maxQueuedRequestsDevice constructor parameter to allow pipeline\n> handler classes to limit the maximum number of requests that get queued\n> to the device in queueRequestDevice().\n> \n> The default value is set to an arbitrary number of 32 which is big\n> enough for all currently known use cases.\n> \n> Signed-off-by: Stefan Klug \n> \n> ---\n> \n> Changes in v1:\n> - Used a const member variable to carry the maximum number of requests\n> - Improved commit message\n> - Added docs\n> ---\n> include/libcamera/internal/pipeline_handler.h | 4 +++-\n> src/libcamera/pipeline_handler.cpp | 20 ++++++++++++++-----\n> 2 files changed, 18 insertions(+), 6 deletions(-)\n> \n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index be017ad47219..e89d6a33e398 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -33,7 +33,8 @@ class PipelineHandler : public std::enable_shared_from_this,\n> \t\t\tpublic Object\n> {\n> public:\n> -\tPipelineHandler(CameraManager *manager);\n> +\tPipelineHandler(CameraManager *manager,\n> +\t\t\tunsigned int maxQueuedRequestsDevice = 32);\n> \tvirtual ~PipelineHandler();\n> \n> \tvirtual bool match(DeviceEnumerator *enumerator) = 0;\n> @@ -80,6 +81,7 @@ protected:\n> \tvirtual void releaseDevice(Camera *camera);\n> \n> \tCameraManager *manager_;\n> +\tconst unsigned int maxQueuedRequestsDevice_;\n> \n> private:\n> \tvoid unlockMediaDevices();\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index dc4086aa9bb5..383c6ad0c4aa 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -62,13 +62,17 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> /**\n> * \\brief Construct a PipelineHandler instance\n> * \\param[in] manager The camera manager\n> + * \\param[in] maxQueuedRequestsDevice The maximum number of requests queued to\n> + * the device\n> *\n> * In order to honour the std::enable_shared_from_this<> contract,\n> * PipelineHandler instances shall never be constructed manually, but always\n> * through the PipelineHandlerFactoryBase::create() function.\n> */\n> -PipelineHandler::PipelineHandler(CameraManager *manager)\n> -\t: manager_(manager), useCount_(0)\n> +PipelineHandler::PipelineHandler(CameraManager *manager,\n> +\t\t\t\t unsigned int maxQueuedRequestsDevice)\n> +\t: manager_(manager), maxQueuedRequestsDevice_(maxQueuedRequestsDevice),\n> +\t useCount_(0)\n> {\n> }\n> \n> @@ -430,9 +434,9 @@ void PipelineHandler::registerRequest(Request *request)\n> * requests which have to be prepared to make sure they are ready for being\n> * queued to the pipeline handler.\n> *\n> - * The queue of waiting requests is iterated and all prepared requests are\n> - * passed to the pipeline handler in the same order they have been queued by\n> - * calling this function.\n> + * The queue of waiting requests is iterated and up to \\a\n> + * maxQueuedRequestsDevice_ prepared requests are passed to the pipeline handler\n> + * in the same order they have been queued by calling this function.\n> *\n> * If a Request fails during the preparation phase or if the pipeline handler\n> * fails in queuing the request to the hardware the request is cancelled.\n> @@ -487,6 +491,9 @@ void PipelineHandler::doQueueRequests(Camera *camera)\n> {\n> \tCamera::Private *data = camera->_d();\n> \twhile (!data->waitingRequests_.empty()) {\n> +\t\tif (data->queuedRequests_.size() == maxQueuedRequestsDevice_)\n> +\t\t\tbreak;\n> +\n> \t\tRequest *request = data->waitingRequests_.front();\n> \t\tif (!request->_d()->prepared_)\n> \t\t\tbreak;\n> @@ -568,6 +575,9 @@ void PipelineHandler::completeRequest(Request *request)\n> \t\tdata->queuedRequests_.pop_front();\n> \t\tcamera->requestComplete(req);\n> \t}\n> +\n> +\t/* Allow any waiting requests to be queued to the pipeline. */\n> +\tdoQueueRequests(camera);\n\nIs this change intended ? I am looking at your original RFC:\nhttps://patchwork.libcamera.org/patch/23447/ which doesn't have this.\n\nI am asking this because, for me, not only this change is orthogonal\nto the patch but also, it seems a bit 'off' to try to queue waiting\nrequests in completeRequest(). Especially considering the fact that\ncompleteRequest() can be called on request-cancellation (probably on\na stop() sequence), and then we try to queue more requests.\n\nNow with this change, we have patch 6/6 which seems to handle the side\neffect of this. The stop() sequence is cancelling requests, followed\nby completeRequest(), hence to stop doQueueRequests() from queuing\nthe requests, we need to clear the waiting queue early-on. I feel, this\nis a bit inter-twined.\n\nI haven't explored any concrete solutions yet, but I think we need\nto first split off this change from this patch atleast.\n\n> }\n> \n> /**\n> -- \n> 2.48.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 97332BDCBF\n\tfor ;\n\tMon, 30 Jun 2025 10:11:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AC20868E09;\n\tMon, 30 Jun 2025 12:11:30 +0200 (CEST)","from fanzine2.igalia.com (fanzine2.igalia.com [213.97.179.56])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5472868E00\n\tfor ;\n\tMon, 30 Jun 2025 12:11:28 +0200 (CEST)","from [49.36.69.141] (helo=uajain)\n\tby fanzine2.igalia.com with esmtpsa \n\t(Cipher TLS1.3:ECDHE_SECP256R1__RSA_PSS_RSAE_SHA256__AES_256_GCM:256)\n\t(Exim) id 1uWBTz-00AQVA-6H; Mon, 30 Jun 2025 12:11:27 +0200"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=igalia.com header.i=@igalia.com\n\theader.b=\"ccT4NNdl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=igalia.com;\n\ts=20170329;\n\th=In-Reply-To:Content-Type:MIME-Version:References:Message-ID:\n\tSubject:Cc:To:From:Date:Sender:Reply-To:Content-Transfer-Encoding:Content-ID:\n\tContent-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc\n\t:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe:\n\tList-Post:List-Owner:List-Archive;\n\tbh=FvIWYaJw798a7RHAaG+SxfcBU6rC1I9ivZnr48Qcf98=;\n\tb=ccT4NNdlLRAGYvq2UB3k4bt9Qa\n\twkYMw46enXLbUQW6Cb2aCZk4VMKomazXE2tZV6agyQ6Ga6CAgnk8xyKn5qulIsLalL5K9TyWm3LVS\n\tRI4ZxVfOVK1LUUItMcBtIp2WntykHNVAQekOtz1yu2F/kDHzgXMCIpOpXG4n4KhyYPaWdCUFniGj3\n\tNHoD6wlyP+5/GSj+wicAxXBBYVkqp+PCy9vg2Mki9MCbwXjjLwMz7qBMvW8x9Fs3KHT0H9DKa9QHm\n\tG3E/WJcpG3T34EhofrOZyENvCr7BSNRjQHM4Y02/BC58q1nDHrUk3L2lKEKMrJahYkP8vxQrHI4Vm\n\tqzKBbvng==;","Date":"Mon, 30 Jun 2025 15:41:31 +0530","From":"Umang Jain ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 2/6] libcamera: pipeline_handler: Allow to limit the\n\tnumber of queued requests","Message-ID":"","References":"<20250630081126.2384387-1-stefan.klug@ideasonboard.com>\n\t<20250630081126.2384387-3-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20250630081126.2384387-3-stefan.klug@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34737,"web_url":"https://patchwork.libcamera.org/comment/34737/","msgid":"<4364469b-7c12-44e4-b72a-4431647b26c7@ideasonboard.com>","date":"2025-06-30T10:53:44","subject":"Re: [PATCH v1 2/6] libcamera: pipeline_handler: Allow to limit the\n\tnumber of queued requests","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 06. 30. 12:11 keltezéssel, Umang Jain írta:\n> Hi Stefan,\n> \n> On Mon, Jun 30, 2025 at 10:11:17AM +0200, Stefan Klug wrote:\n>> Add a maxQueuedRequestsDevice constructor parameter to allow pipeline\n>> handler classes to limit the maximum number of requests that get queued\n>> to the device in queueRequestDevice().\n>>\n>> The default value is set to an arbitrary number of 32 which is big\n>> enough for all currently known use cases.\n>>\n>> Signed-off-by: Stefan Klug \n>>\n>> ---\n>>\n>> Changes in v1:\n>> - Used a const member variable to carry the maximum number of requests\n>> - Improved commit message\n>> - Added docs\n>> ---\n>> include/libcamera/internal/pipeline_handler.h | 4 +++-\n>> src/libcamera/pipeline_handler.cpp | 20 ++++++++++++++-----\n>> 2 files changed, 18 insertions(+), 6 deletions(-)\n>>\n>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n>> index be017ad47219..e89d6a33e398 100644\n>> --- a/include/libcamera/internal/pipeline_handler.h\n>> +++ b/include/libcamera/internal/pipeline_handler.h\n>> @@ -33,7 +33,8 @@ class PipelineHandler : public std::enable_shared_from_this,\n>> \t\t\tpublic Object\n>> {\n>> public:\n>> -\tPipelineHandler(CameraManager *manager);\n>> +\tPipelineHandler(CameraManager *manager,\n>> +\t\t\tunsigned int maxQueuedRequestsDevice = 32);\n>> \tvirtual ~PipelineHandler();\n>> \n>> \tvirtual bool match(DeviceEnumerator *enumerator) = 0;\n>> @@ -80,6 +81,7 @@ protected:\n>> \tvirtual void releaseDevice(Camera *camera);\n>> \n>> \tCameraManager *manager_;\n>> +\tconst unsigned int maxQueuedRequestsDevice_;\n>> \n>> private:\n>> \tvoid unlockMediaDevices();\n>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n>> index dc4086aa9bb5..383c6ad0c4aa 100644\n>> --- a/src/libcamera/pipeline_handler.cpp\n>> +++ b/src/libcamera/pipeline_handler.cpp\n>> @@ -62,13 +62,17 @@ LOG_DEFINE_CATEGORY(Pipeline)\n>> /**\n>> * \\brief Construct a PipelineHandler instance\n>> * \\param[in] manager The camera manager\n>> + * \\param[in] maxQueuedRequestsDevice The maximum number of requests queued to\n>> + * the device\n>> *\n>> * In order to honour the std::enable_shared_from_this<> contract,\n>> * PipelineHandler instances shall never be constructed manually, but always\n>> * through the PipelineHandlerFactoryBase::create() function.\n>> */\n>> -PipelineHandler::PipelineHandler(CameraManager *manager)\n>> -\t: manager_(manager), useCount_(0)\n>> +PipelineHandler::PipelineHandler(CameraManager *manager,\n>> +\t\t\t\t unsigned int maxQueuedRequestsDevice)\n>> +\t: manager_(manager), maxQueuedRequestsDevice_(maxQueuedRequestsDevice),\n>> +\t useCount_(0)\n>> {\n>> }\n>> \n>> @@ -430,9 +434,9 @@ void PipelineHandler::registerRequest(Request *request)\n>> * requests which have to be prepared to make sure they are ready for being\n>> * queued to the pipeline handler.\n>> *\n>> - * The queue of waiting requests is iterated and all prepared requests are\n>> - * passed to the pipeline handler in the same order they have been queued by\n>> - * calling this function.\n>> + * The queue of waiting requests is iterated and up to \\a\n>> + * maxQueuedRequestsDevice_ prepared requests are passed to the pipeline handler\n>> + * in the same order they have been queued by calling this function.\n>> *\n>> * If a Request fails during the preparation phase or if the pipeline handler\n>> * fails in queuing the request to the hardware the request is cancelled.\n>> @@ -487,6 +491,9 @@ void PipelineHandler::doQueueRequests(Camera *camera)\n>> {\n>> \tCamera::Private *data = camera->_d();\n>> \twhile (!data->waitingRequests_.empty()) {\n>> +\t\tif (data->queuedRequests_.size() == maxQueuedRequestsDevice_)\n>> +\t\t\tbreak;\n>> +\n>> \t\tRequest *request = data->waitingRequests_.front();\n>> \t\tif (!request->_d()->prepared_)\n>> \t\t\tbreak;\n>> @@ -568,6 +575,9 @@ void PipelineHandler::completeRequest(Request *request)\n>> \t\tdata->queuedRequests_.pop_front();\n>> \t\tcamera->requestComplete(req);\n>> \t}\n>> +\n>> +\t/* Allow any waiting requests to be queued to the pipeline. */\n>> +\tdoQueueRequests(camera);\n> \n> Is this change intended ? I am looking at your original RFC:\n> https://patchwork.libcamera.org/patch/23447/ which doesn't have this.\n> \n> I am asking this because, for me, not only this change is orthogonal\n> to the patch but also, it seems a bit 'off' to try to queue waiting\n> requests in completeRequest(). Especially considering the fact that\n> completeRequest() can be called on request-cancellation (probably on\n> a stop() sequence), and then we try to queue more requests.\n\nI think the RFC is incomplete, and this addition is required, in order to drain\n`waitingRequests_`. Any time an item is removed from `queuedRequests_`, the\n`waitingRequests_` queue needs to be checked and the eligible requests need to\nbe queued to the pipeline handler. Maybe this is not the ideal location since this\nalso introduces a potential re-entrancy issue: a pipeline handler calling completeRequest()\nmight get a call back into the pipeline handler, which it might not be able to handle.\nBut I think something like this is needed in any case.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n> Now with this change, we have patch 6/6 which seems to handle the side\n> effect of this. The stop() sequence is cancelling requests, followed\n> by completeRequest(), hence to stop doQueueRequests() from queuing\n> the requests, we need to clear the waiting queue early-on. I feel, this\n> is a bit inter-twined.\n> \n> I haven't explored any concrete solutions yet, but I think we need\n> to first split off this change from this patch atleast.\n> \n>> }\n>> \n>> /**\n>> -- \n>> 2.48.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 117F8BDCBF\n\tfor ;\n\tMon, 30 Jun 2025 10:53:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BA17E68E04;\n\tMon, 30 Jun 2025 12:53:50 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5FC4B61527\n\tfor ;\n\tMon, 30 Jun 2025 12:53:48 +0200 (CEST)","from [192.168.33.21] (185.221.143.107.nat.pool.zt.hu\n\t[185.221.143.107])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 398D1C75;\n\tMon, 30 Jun 2025 12:53:26 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"l2mBllYI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1751280806;\n\tbh=7eAuIQIYpKrATEgIOzMQbX4DegAKNunKlBODk6yYzhY=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=l2mBllYI7OZo/puyASgsfH2s1RrEn9eIS+bneAzMIX81pJh334tIRE2JmDTJviq1n\n\tGwL+YJkMV5dIOMA6TurF44TLokX/EdoUECexn+YmG1XowNgKhWgrS0ohrgfY1hzMdC\n\tWqd3BxUhxK1SB0s1/8l1TVWCa7ZhpczfMyGat/Ho=","Message-ID":"<4364469b-7c12-44e4-b72a-4431647b26c7@ideasonboard.com>","Date":"Mon, 30 Jun 2025 12:53:44 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1 2/6] libcamera: pipeline_handler: Allow to limit the\n\tnumber of queued requests","To":"Umang Jain ,\n\tStefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250630081126.2384387-1-stefan.klug@ideasonboard.com>\n\t<20250630081126.2384387-3-stefan.klug@ideasonboard.com>\n\t","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34739,"web_url":"https://patchwork.libcamera.org/comment/34739/","msgid":"","date":"2025-06-30T11:41:35","subject":"Re: [PATCH v1 2/6] libcamera: pipeline_handler: Allow to limit the\n\tnumber of queued requests","submitter":{"id":232,"url":"https://patchwork.libcamera.org/api/people/232/","name":"Umang Jain","email":"uajain@igalia.com"},"content":"Hi,\n\nOn Mon, Jun 30, 2025 at 12:53:44PM +0200, Barnabás Pőcze wrote:\n> Hi\n> \n> 2025. 06. 30. 12:11 keltezéssel, Umang Jain írta:\n> > Hi Stefan,\n> > \n> > On Mon, Jun 30, 2025 at 10:11:17AM +0200, Stefan Klug wrote:\n> > > Add a maxQueuedRequestsDevice constructor parameter to allow pipeline\n> > > handler classes to limit the maximum number of requests that get queued\n> > > to the device in queueRequestDevice().\n> > > \n> > > The default value is set to an arbitrary number of 32 which is big\n> > > enough for all currently known use cases.\n> > > \n> > > Signed-off-by: Stefan Klug \n> > > \n> > > ---\n> > > \n> > > Changes in v1:\n> > > - Used a const member variable to carry the maximum number of requests\n> > > - Improved commit message\n> > > - Added docs\n> > > ---\n> > > include/libcamera/internal/pipeline_handler.h | 4 +++-\n> > > src/libcamera/pipeline_handler.cpp | 20 ++++++++++++++-----\n> > > 2 files changed, 18 insertions(+), 6 deletions(-)\n> > > \n> > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > > index be017ad47219..e89d6a33e398 100644\n> > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > @@ -33,7 +33,8 @@ class PipelineHandler : public std::enable_shared_from_this,\n> > > \t\t\tpublic Object\n> > > {\n> > > public:\n> > > -\tPipelineHandler(CameraManager *manager);\n> > > +\tPipelineHandler(CameraManager *manager,\n> > > +\t\t\tunsigned int maxQueuedRequestsDevice = 32);\n> > > \tvirtual ~PipelineHandler();\n> > > \tvirtual bool match(DeviceEnumerator *enumerator) = 0;\n> > > @@ -80,6 +81,7 @@ protected:\n> > > \tvirtual void releaseDevice(Camera *camera);\n> > > \tCameraManager *manager_;\n> > > +\tconst unsigned int maxQueuedRequestsDevice_;\n> > > private:\n> > > \tvoid unlockMediaDevices();\n> > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > index dc4086aa9bb5..383c6ad0c4aa 100644\n> > > --- a/src/libcamera/pipeline_handler.cpp\n> > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > @@ -62,13 +62,17 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> > > /**\n> > > * \\brief Construct a PipelineHandler instance\n> > > * \\param[in] manager The camera manager\n> > > + * \\param[in] maxQueuedRequestsDevice The maximum number of requests queued to\n> > > + * the device\n> > > *\n> > > * In order to honour the std::enable_shared_from_this<> contract,\n> > > * PipelineHandler instances shall never be constructed manually, but always\n> > > * through the PipelineHandlerFactoryBase::create() function.\n> > > */\n> > > -PipelineHandler::PipelineHandler(CameraManager *manager)\n> > > -\t: manager_(manager), useCount_(0)\n> > > +PipelineHandler::PipelineHandler(CameraManager *manager,\n> > > +\t\t\t\t unsigned int maxQueuedRequestsDevice)\n> > > +\t: manager_(manager), maxQueuedRequestsDevice_(maxQueuedRequestsDevice),\n> > > +\t useCount_(0)\n> > > {\n> > > }\n> > > @@ -430,9 +434,9 @@ void PipelineHandler::registerRequest(Request *request)\n> > > * requests which have to be prepared to make sure they are ready for being\n> > > * queued to the pipeline handler.\n> > > *\n> > > - * The queue of waiting requests is iterated and all prepared requests are\n> > > - * passed to the pipeline handler in the same order they have been queued by\n> > > - * calling this function.\n> > > + * The queue of waiting requests is iterated and up to \\a\n> > > + * maxQueuedRequestsDevice_ prepared requests are passed to the pipeline handler\n> > > + * in the same order they have been queued by calling this function.\n> > > *\n> > > * If a Request fails during the preparation phase or if the pipeline handler\n> > > * fails in queuing the request to the hardware the request is cancelled.\n> > > @@ -487,6 +491,9 @@ void PipelineHandler::doQueueRequests(Camera *camera)\n> > > {\n> > > \tCamera::Private *data = camera->_d();\n> > > \twhile (!data->waitingRequests_.empty()) {\n> > > +\t\tif (data->queuedRequests_.size() == maxQueuedRequestsDevice_)\n> > > +\t\t\tbreak;\n> > > +\n> > > \t\tRequest *request = data->waitingRequests_.front();\n> > > \t\tif (!request->_d()->prepared_)\n> > > \t\t\tbreak;\n> > > @@ -568,6 +575,9 @@ void PipelineHandler::completeRequest(Request *request)\n> > > \t\tdata->queuedRequests_.pop_front();\n> > > \t\tcamera->requestComplete(req);\n> > > \t}\n> > > +\n> > > +\t/* Allow any waiting requests to be queued to the pipeline. */\n> > > +\tdoQueueRequests(camera);\n> > \n> > Is this change intended ? I am looking at your original RFC:\n> > https://patchwork.libcamera.org/patch/23447/ which doesn't have this.\n> > \n> > I am asking this because, for me, not only this change is orthogonal\n> > to the patch but also, it seems a bit 'off' to try to queue waiting\n> > requests in completeRequest(). Especially considering the fact that\n> > completeRequest() can be called on request-cancellation (probably on\n> > a stop() sequence), and then we try to queue more requests.\n> \n> I think the RFC is incomplete, and this addition is required, in order to drain\n> `waitingRequests_`. Any time an item is removed from `queuedRequests_`, the\n\nWhy the waitingRequests_ needs to be drained? It simply needs to be\ncancelled/cleared on stop(), as a cleanup and to prevent it\n(accidently) getting queued down to the hardware..\n\n> `waitingRequests_` queue needs to be checked and the eligible requests need to\n> be queued to the pipeline handler. Maybe this is not the ideal location since this\n> also introduces a potential re-entrancy issue: a pipeline handler calling completeRequest()\n> might get a call back into the pipeline handler, which it might not be able to handle.\n> But I think something like this is needed in any case.\n\nI am not sure what use-case/sequence led to this particular change\n(description somewhere would have been nice), but to me,\ndoQueueRequests() does the right thing i.e. being executed when the\nrequest emits 'prepared', which is chained to\nPipelineHandler::queueRequest().\n\nIn normal/most situation,\n\ncompleteRequest() -> queuedRequests_.pop() -> Request::reuse()\n-> queueRequest() -> prepared signal emit -> doQueueRequests()\n-> waitingRequest_.front() queued.\n\nSo, yes changes to queuedRequest_ does end up checking waitingRequests_\nin the end, as mentioned above.\n\n> \n> \n> Regards,\n> Barnabás Pőcze\n> \n> \n> > \n> > Now with this change, we have patch 6/6 which seems to handle the side\n> > effect of this. The stop() sequence is cancelling requests, followed\n> > by completeRequest(), hence to stop doQueueRequests() from queuing\n> > the requests, we need to clear the waiting queue early-on. I feel, this\n> > is a bit inter-twined.\n> > \n> > I haven't explored any concrete solutions yet, but I think we need\n> > to first split off this change from this patch atleast.\n> > \n> > > }\n> > > /**\n> > > -- \n> > > 2.48.1\n> > > \n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B33F5BDCBF\n\tfor ;\n\tMon, 30 Jun 2025 11:41:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4B3F768E00;\n\tMon, 30 Jun 2025 13:41:39 +0200 (CEST)","from fanzine2.igalia.com (fanzine2.igalia.com [213.97.179.56])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3E1C361527\n\tfor ;\n\tMon, 30 Jun 2025 13:41:37 +0200 (CEST)","from [49.36.69.141] (helo=uajain)\n\tby fanzine2.igalia.com with esmtpsa \n\t(Cipher TLS1.3:ECDHE_SECP256R1__RSA_PSS_RSAE_SHA256__AES_256_GCM:256)\n\t(Exim) id 1uWCtE-00ASMn-4P; Mon, 30 Jun 2025 13:41:36 +0200"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=igalia.com header.i=@igalia.com\n\theader.b=\"nkUQSzkI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=igalia.com;\n\ts=20170329;\n\th=In-Reply-To:Content-Transfer-Encoding:Content-Type:MIME-Version\n\t:References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To:Content-ID:\n\tContent-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc\n\t:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe:\n\tList-Post:List-Owner:List-Archive;\n\tbh=kr4A1maAXYK5Wm8widnHkPt3ZaWOnVhONg3Gq58OfZk=;\n\tb=nkUQSzkI6SvFgiVoNryHVxoG1q\n\t2kJfP6mVsWKUwUfdvJxpVDvllAsRQAg8fGbQf158yLRRFnfKMGjX1ml5jqyvqAx/UxDN5KNbz61NG\n\ttxaG6alxfKuWj56TENkpYd24WQY+aPgJKBmZauKbSL9cx7BaIm1iG2HkOOUpuuSJOU7SSbzJKnoHm\n\tY6qs1pJbyaEt+jeYQkda6Q1fVqhEG/XioE0kK+H09M2fYjAeJq4cUVJD502iDn0B/7t2+nuhl0UvU\n\tsWNrjqgxObCoZFK8WDxBYaqkyaKL+6umyG3A1CT3Acmh4vS6VGuGZf8J+oygeWsqAdgPdTkU/v3fy\n\tkOXy3Gyw==;","Date":"Mon, 30 Jun 2025 17:11:35 +0530","From":"Umang Jain ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Stefan Klug , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 2/6] libcamera: pipeline_handler: Allow to limit the\n\tnumber of queued requests","Message-ID":"","References":"<20250630081126.2384387-1-stefan.klug@ideasonboard.com>\n\t<20250630081126.2384387-3-stefan.klug@ideasonboard.com>\n\t\n\t<4364469b-7c12-44e4-b72a-4431647b26c7@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<4364469b-7c12-44e4-b72a-4431647b26c7@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34741,"web_url":"https://patchwork.libcamera.org/comment/34741/","msgid":"","date":"2025-06-30T12:43:21","subject":"Re: [PATCH v1 2/6] libcamera: pipeline_handler: Allow to limit the\n\tnumber of queued requests","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 06. 30. 13:41 keltezéssel, Umang Jain írta:\n> Hi,\n> \n> On Mon, Jun 30, 2025 at 12:53:44PM +0200, Barnabás Pőcze wrote:\n>> Hi\n>>\n>> 2025. 06. 30. 12:11 keltezéssel, Umang Jain írta:\n>>> Hi Stefan,\n>>>\n>>> On Mon, Jun 30, 2025 at 10:11:17AM +0200, Stefan Klug wrote:\n>>>> Add a maxQueuedRequestsDevice constructor parameter to allow pipeline\n>>>> handler classes to limit the maximum number of requests that get queued\n>>>> to the device in queueRequestDevice().\n>>>>\n>>>> The default value is set to an arbitrary number of 32 which is big\n>>>> enough for all currently known use cases.\n>>>>\n>>>> Signed-off-by: Stefan Klug \n>>>>\n>>>> ---\n>>>>\n>>>> Changes in v1:\n>>>> - Used a const member variable to carry the maximum number of requests\n>>>> - Improved commit message\n>>>> - Added docs\n>>>> ---\n>>>> include/libcamera/internal/pipeline_handler.h | 4 +++-\n>>>> src/libcamera/pipeline_handler.cpp | 20 ++++++++++++++-----\n>>>> 2 files changed, 18 insertions(+), 6 deletions(-)\n>>>>\n>>>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n>>>> index be017ad47219..e89d6a33e398 100644\n>>>> --- a/include/libcamera/internal/pipeline_handler.h\n>>>> +++ b/include/libcamera/internal/pipeline_handler.h\n>>>> @@ -33,7 +33,8 @@ class PipelineHandler : public std::enable_shared_from_this,\n>>>> \t\t\tpublic Object\n>>>> {\n>>>> public:\n>>>> -\tPipelineHandler(CameraManager *manager);\n>>>> +\tPipelineHandler(CameraManager *manager,\n>>>> +\t\t\tunsigned int maxQueuedRequestsDevice = 32);\n>>>> \tvirtual ~PipelineHandler();\n>>>> \tvirtual bool match(DeviceEnumerator *enumerator) = 0;\n>>>> @@ -80,6 +81,7 @@ protected:\n>>>> \tvirtual void releaseDevice(Camera *camera);\n>>>> \tCameraManager *manager_;\n>>>> +\tconst unsigned int maxQueuedRequestsDevice_;\n>>>> private:\n>>>> \tvoid unlockMediaDevices();\n>>>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n>>>> index dc4086aa9bb5..383c6ad0c4aa 100644\n>>>> --- a/src/libcamera/pipeline_handler.cpp\n>>>> +++ b/src/libcamera/pipeline_handler.cpp\n>>>> @@ -62,13 +62,17 @@ LOG_DEFINE_CATEGORY(Pipeline)\n>>>> /**\n>>>> * \\brief Construct a PipelineHandler instance\n>>>> * \\param[in] manager The camera manager\n>>>> + * \\param[in] maxQueuedRequestsDevice The maximum number of requests queued to\n>>>> + * the device\n>>>> *\n>>>> * In order to honour the std::enable_shared_from_this<> contract,\n>>>> * PipelineHandler instances shall never be constructed manually, but always\n>>>> * through the PipelineHandlerFactoryBase::create() function.\n>>>> */\n>>>> -PipelineHandler::PipelineHandler(CameraManager *manager)\n>>>> -\t: manager_(manager), useCount_(0)\n>>>> +PipelineHandler::PipelineHandler(CameraManager *manager,\n>>>> +\t\t\t\t unsigned int maxQueuedRequestsDevice)\n>>>> +\t: manager_(manager), maxQueuedRequestsDevice_(maxQueuedRequestsDevice),\n>>>> +\t useCount_(0)\n>>>> {\n>>>> }\n>>>> @@ -430,9 +434,9 @@ void PipelineHandler::registerRequest(Request *request)\n>>>> * requests which have to be prepared to make sure they are ready for being\n>>>> * queued to the pipeline handler.\n>>>> *\n>>>> - * The queue of waiting requests is iterated and all prepared requests are\n>>>> - * passed to the pipeline handler in the same order they have been queued by\n>>>> - * calling this function.\n>>>> + * The queue of waiting requests is iterated and up to \\a\n>>>> + * maxQueuedRequestsDevice_ prepared requests are passed to the pipeline handler\n>>>> + * in the same order they have been queued by calling this function.\n>>>> *\n>>>> * If a Request fails during the preparation phase or if the pipeline handler\n>>>> * fails in queuing the request to the hardware the request is cancelled.\n>>>> @@ -487,6 +491,9 @@ void PipelineHandler::doQueueRequests(Camera *camera)\n>>>> {\n>>>> \tCamera::Private *data = camera->_d();\n>>>> \twhile (!data->waitingRequests_.empty()) {\n>>>> +\t\tif (data->queuedRequests_.size() == maxQueuedRequestsDevice_)\n>>>> +\t\t\tbreak;\n>>>> +\n>>>> \t\tRequest *request = data->waitingRequests_.front();\n>>>> \t\tif (!request->_d()->prepared_)\n>>>> \t\t\tbreak;\n>>>> @@ -568,6 +575,9 @@ void PipelineHandler::completeRequest(Request *request)\n>>>> \t\tdata->queuedRequests_.pop_front();\n>>>> \t\tcamera->requestComplete(req);\n>>>> \t}\n>>>> +\n>>>> +\t/* Allow any waiting requests to be queued to the pipeline. */\n>>>> +\tdoQueueRequests(camera);\n>>>\n>>> Is this change intended ? I am looking at your original RFC:\n>>> https://patchwork.libcamera.org/patch/23447/ which doesn't have this.\n>>>\n>>> I am asking this because, for me, not only this change is orthogonal\n>>> to the patch but also, it seems a bit 'off' to try to queue waiting\n>>> requests in completeRequest(). Especially considering the fact that\n>>> completeRequest() can be called on request-cancellation (probably on\n>>> a stop() sequence), and then we try to queue more requests.\n>>\n>> I think the RFC is incomplete, and this addition is required, in order to drain\n>> `waitingRequests_`. Any time an item is removed from `queuedRequests_`, the\n> \n> Why the waitingRequests_ needs to be drained? It simply needs to be\n> cancelled/cleared on stop(), as a cleanup and to prevent it\n> (accidently) getting queued down to the hardware..\n\nConsider the scenario where one queues `maxQueuedRequestsDevice_ + 1` requests,\nand waits for all of them to complete, then stops the camera. If the requests\nare all queued before the first one could be completed, then `waitingRequests_`\nwill have the last request, while `queuedRequests_` will have the first `maxQueuedRequestsDevice_`\nrequests. If the `waitingRequests_` is not processed on completion, then it\nwill stay there forever, and it will never complete. (Unless I am missing something?)\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n>> `waitingRequests_` queue needs to be checked and the eligible requests need to\n>> be queued to the pipeline handler. Maybe this is not the ideal location since this\n>> also introduces a potential re-entrancy issue: a pipeline handler calling completeRequest()\n>> might get a call back into the pipeline handler, which it might not be able to handle.\n>> But I think something like this is needed in any case.\n> \n> I am not sure what use-case/sequence led to this particular change\n> (description somewhere would have been nice), but to me,\n> doQueueRequests() does the right thing i.e. being executed when the\n> request emits 'prepared', which is chained to\n> PipelineHandler::queueRequest().\n> \n> In normal/most situation,\n> \n> completeRequest() -> queuedRequests_.pop() -> Request::reuse()\n> -> queueRequest() -> prepared signal emit -> doQueueRequests()\n> -> waitingRequest_.front() queued.\n> \n> So, yes changes to queuedRequest_ does end up checking waitingRequests_\n> in the end, as mentioned above.\n> \n>>\n>>\n>> Regards,\n>> Barnabás Pőcze\n>>\n>>\n>>>\n>>> Now with this change, we have patch 6/6 which seems to handle the side\n>>> effect of this. The stop() sequence is cancelling requests, followed\n>>> by completeRequest(), hence to stop doQueueRequests() from queuing\n>>> the requests, we need to clear the waiting queue early-on. I feel, this\n>>> is a bit inter-twined.\n>>>\n>>> I haven't explored any concrete solutions yet, but I think we need\n>>> to first split off this change from this patch atleast.\n>>>\n>>>> }\n>>>> /**\n>>>> -- \n>>>> 2.48.1\n>>>>\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 2AA59BDCBF\n\tfor ;\n\tMon, 30 Jun 2025 12:43:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AFB7468E04;\n\tMon, 30 Jun 2025 14:43:27 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AD5CD61527\n\tfor ;\n\tMon, 30 Jun 2025 14:43:26 +0200 (CEST)","from [192.168.33.21] (185.221.143.107.nat.pool.zt.hu\n\t[185.221.143.107])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8967AEFF;\n\tMon, 30 Jun 2025 14:43:04 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"eXCQcMWQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1751287384;\n\tbh=Tn+VdbaA8S93Qi4mDGll5Ar23ocxE899jtLe8WMrAG4=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=eXCQcMWQvPYdmITSaUDQ6GI/hCY2n3rymlMuK51M/OATsaaJxtMdi927mecL4BoQG\n\t9oP4wBXy79ZDc/V0YBj8EjMk395th/EvI+dYkVEBPhDBKyZ/pz3zIs5GJVHWbOVsUP\n\t4fXXcOtAZ4ADZBZ5+d/Hrbr53trCOb8aVQYsNaBI=","Message-ID":"","Date":"Mon, 30 Jun 2025 14:43:21 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1 2/6] libcamera: pipeline_handler: Allow to limit the\n\tnumber of queued requests","To":"Umang Jain ","Cc":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250630081126.2384387-1-stefan.klug@ideasonboard.com>\n\t<20250630081126.2384387-3-stefan.klug@ideasonboard.com>\n\t\n\t<4364469b-7c12-44e4-b72a-4431647b26c7@ideasonboard.com>\n\t","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34742,"web_url":"https://patchwork.libcamera.org/comment/34742/","msgid":"<6oksvrtnkaqpkurkevthh6fsyasdy43of3iilavjzbp5skqkju@jtt2cm3khre3>","date":"2025-06-30T15:03:52","subject":"Re: [PATCH v1 2/6] libcamera: pipeline_handler: Allow to limit the\n\tnumber of queued requests","submitter":{"id":232,"url":"https://patchwork.libcamera.org/api/people/232/","name":"Umang Jain","email":"uajain@igalia.com"},"content":"On Mon, Jun 30, 2025 at 02:43:21PM +0200, Barnabás Pőcze wrote:\n> Hi\n> \n> 2025. 06. 30. 13:41 keltezéssel, Umang Jain írta:\n> > Hi,\n> > \n> > On Mon, Jun 30, 2025 at 12:53:44PM +0200, Barnabás Pőcze wrote:\n> > > Hi\n> > > \n> > > 2025. 06. 30. 12:11 keltezéssel, Umang Jain írta:\n> > > > Hi Stefan,\n> > > > \n> > > > On Mon, Jun 30, 2025 at 10:11:17AM +0200, Stefan Klug wrote:\n> > > > > Add a maxQueuedRequestsDevice constructor parameter to allow pipeline\n> > > > > handler classes to limit the maximum number of requests that get queued\n> > > > > to the device in queueRequestDevice().\n> > > > > \n> > > > > The default value is set to an arbitrary number of 32 which is big\n> > > > > enough for all currently known use cases.\n> > > > > \n> > > > > Signed-off-by: Stefan Klug \n> > > > > \n> > > > > ---\n> > > > > \n> > > > > Changes in v1:\n> > > > > - Used a const member variable to carry the maximum number of requests\n> > > > > - Improved commit message\n> > > > > - Added docs\n> > > > > ---\n> > > > > include/libcamera/internal/pipeline_handler.h | 4 +++-\n> > > > > src/libcamera/pipeline_handler.cpp | 20 ++++++++++++++-----\n> > > > > 2 files changed, 18 insertions(+), 6 deletions(-)\n> > > > > \n> > > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > > > > index be017ad47219..e89d6a33e398 100644\n> > > > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > > > @@ -33,7 +33,8 @@ class PipelineHandler : public std::enable_shared_from_this,\n> > > > > \t\t\tpublic Object\n> > > > > {\n> > > > > public:\n> > > > > -\tPipelineHandler(CameraManager *manager);\n> > > > > +\tPipelineHandler(CameraManager *manager,\n> > > > > +\t\t\tunsigned int maxQueuedRequestsDevice = 32);\n> > > > > \tvirtual ~PipelineHandler();\n> > > > > \tvirtual bool match(DeviceEnumerator *enumerator) = 0;\n> > > > > @@ -80,6 +81,7 @@ protected:\n> > > > > \tvirtual void releaseDevice(Camera *camera);\n> > > > > \tCameraManager *manager_;\n> > > > > +\tconst unsigned int maxQueuedRequestsDevice_;\n> > > > > private:\n> > > > > \tvoid unlockMediaDevices();\n> > > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > > > index dc4086aa9bb5..383c6ad0c4aa 100644\n> > > > > --- a/src/libcamera/pipeline_handler.cpp\n> > > > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > > > @@ -62,13 +62,17 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> > > > > /**\n> > > > > * \\brief Construct a PipelineHandler instance\n> > > > > * \\param[in] manager The camera manager\n> > > > > + * \\param[in] maxQueuedRequestsDevice The maximum number of requests queued to\n> > > > > + * the device\n> > > > > *\n> > > > > * In order to honour the std::enable_shared_from_this<> contract,\n> > > > > * PipelineHandler instances shall never be constructed manually, but always\n> > > > > * through the PipelineHandlerFactoryBase::create() function.\n> > > > > */\n> > > > > -PipelineHandler::PipelineHandler(CameraManager *manager)\n> > > > > -\t: manager_(manager), useCount_(0)\n> > > > > +PipelineHandler::PipelineHandler(CameraManager *manager,\n> > > > > +\t\t\t\t unsigned int maxQueuedRequestsDevice)\n> > > > > +\t: manager_(manager), maxQueuedRequestsDevice_(maxQueuedRequestsDevice),\n> > > > > +\t useCount_(0)\n> > > > > {\n> > > > > }\n> > > > > @@ -430,9 +434,9 @@ void PipelineHandler::registerRequest(Request *request)\n> > > > > * requests which have to be prepared to make sure they are ready for being\n> > > > > * queued to the pipeline handler.\n> > > > > *\n> > > > > - * The queue of waiting requests is iterated and all prepared requests are\n> > > > > - * passed to the pipeline handler in the same order they have been queued by\n> > > > > - * calling this function.\n> > > > > + * The queue of waiting requests is iterated and up to \\a\n> > > > > + * maxQueuedRequestsDevice_ prepared requests are passed to the pipeline handler\n> > > > > + * in the same order they have been queued by calling this function.\n> > > > > *\n> > > > > * If a Request fails during the preparation phase or if the pipeline handler\n> > > > > * fails in queuing the request to the hardware the request is cancelled.\n> > > > > @@ -487,6 +491,9 @@ void PipelineHandler::doQueueRequests(Camera *camera)\n> > > > > {\n> > > > > \tCamera::Private *data = camera->_d();\n> > > > > \twhile (!data->waitingRequests_.empty()) {\n> > > > > +\t\tif (data->queuedRequests_.size() == maxQueuedRequestsDevice_)\n> > > > > +\t\t\tbreak;\n> > > > > +\n> > > > > \t\tRequest *request = data->waitingRequests_.front();\n> > > > > \t\tif (!request->_d()->prepared_)\n> > > > > \t\t\tbreak;\n> > > > > @@ -568,6 +575,9 @@ void PipelineHandler::completeRequest(Request *request)\n> > > > > \t\tdata->queuedRequests_.pop_front();\n> > > > > \t\tcamera->requestComplete(req);\n> > > > > \t}\n> > > > > +\n> > > > > +\t/* Allow any waiting requests to be queued to the pipeline. */\n> > > > > +\tdoQueueRequests(camera);\n> > > > \n> > > > Is this change intended ? I am looking at your original RFC:\n> > > > https://patchwork.libcamera.org/patch/23447/ which doesn't have this.\n> > > > \n> > > > I am asking this because, for me, not only this change is orthogonal\n> > > > to the patch but also, it seems a bit 'off' to try to queue waiting\n> > > > requests in completeRequest(). Especially considering the fact that\n> > > > completeRequest() can be called on request-cancellation (probably on\n> > > > a stop() sequence), and then we try to queue more requests.\n> > > \n> > > I think the RFC is incomplete, and this addition is required, in order to drain\n> > > `waitingRequests_`. Any time an item is removed from `queuedRequests_`, the\n> > \n> > Why the waitingRequests_ needs to be drained? It simply needs to be\n> > cancelled/cleared on stop(), as a cleanup and to prevent it\n> > (accidently) getting queued down to the hardware..\n> \n> Consider the scenario where one queues `maxQueuedRequestsDevice_ + 1` requests,\n> and waits for all of them to complete, then stops the camera. If the requests\n> are all queued before the first one could be completed, then `waitingRequests_`\n> will have the last request, while `queuedRequests_` will have the first `maxQueuedRequestsDevice_`\n> requests. If the `waitingRequests_` is not processed on completion, then it\n> will stay there forever, and it will never complete. (Unless I am missing something?)\n\nIn your scenario, the last request 'maxQueuedRequestsDevice_ + 1`th\nin waitingRequests_ will complete on calling stop(). It will be complete\nby PipelineHandler::cancelRequest().\n\nPipelineHandler::queueRequest() documentation also states:\n\n * If a Request fails during the preparation phase or if the pipeline handler \n * fails in queuing the request to the hardware the request is cancelled. \n\nDoes waitingRequests_ dwould fall in: \"fails in queuing the request to\nthe hardware\" category? Becuase the user queued requests too fast and\nalso beyond maxQueuedRequestsDevice_ ?\n\n> \n> \n> Regards,\n> Barnabás Pőcze\n> \n> \n> > \n> > > `waitingRequests_` queue needs to be checked and the eligible requests need to\n> > > be queued to the pipeline handler. Maybe this is not the ideal location since this\n> > > also introduces a potential re-entrancy issue: a pipeline handler calling completeRequest()\n> > > might get a call back into the pipeline handler, which it might not be able to handle.\n> > > But I think something like this is needed in any case.\n> > \n> > I am not sure what use-case/sequence led to this particular change\n> > (description somewhere would have been nice), but to me,\n> > doQueueRequests() does the right thing i.e. being executed when the\n> > request emits 'prepared', which is chained to\n> > PipelineHandler::queueRequest().\n> > \n> > In normal/most situation,\n> > \n> > completeRequest() -> queuedRequests_.pop() -> Request::reuse()\n> > -> queueRequest() -> prepared signal emit -> doQueueRequests()\n> > -> waitingRequest_.front() queued.\n> > \n> > So, yes changes to queuedRequest_ does end up checking waitingRequests_\n> > in the end, as mentioned above.\n> > \n> > > \n> > > \n> > > Regards,\n> > > Barnabás Pőcze\n> > > \n> > > \n> > > > \n> > > > Now with this change, we have patch 6/6 which seems to handle the side\n> > > > effect of this. The stop() sequence is cancelling requests, followed\n> > > > by completeRequest(), hence to stop doQueueRequests() from queuing\n> > > > the requests, we need to clear the waiting queue early-on. I feel, this\n> > > > is a bit inter-twined.\n> > > > \n> > > > I haven't explored any concrete solutions yet, but I think we need\n> > > > to first split off this change from this patch atleast.\n> > > > \n> > > > > }\n> > > > > /**\n> > > > > -- \n> > > > > 2.48.1\n> > > > > \n> > > \n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 297D1C3237\n\tfor ;\n\tMon, 30 Jun 2025 15:03:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 40E0768E04;\n\tMon, 30 Jun 2025 17:03:52 +0200 (CEST)","from fanzine2.igalia.com (fanzine2.igalia.com [213.97.179.56])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DF25B61527\n\tfor ;\n\tMon, 30 Jun 2025 17:03:50 +0200 (CEST)","from [49.36.69.141] (helo=uajain)\n\tby fanzine2.igalia.com with esmtpsa \n\t(Cipher TLS1.3:ECDHE_SECP256R1__RSA_PSS_RSAE_SHA256__AES_256_GCM:256)\n\t(Exim) id 1uWG2v-00AWhH-KR; Mon, 30 Jun 2025 17:03:50 +0200"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=igalia.com header.i=@igalia.com\n\theader.b=\"ljpwYlOH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=igalia.com;\n\ts=20170329;\n\th=In-Reply-To:Content-Transfer-Encoding:Content-Type:MIME-Version\n\t:References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To:Content-ID:\n\tContent-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc\n\t:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe:\n\tList-Post:List-Owner:List-Archive;\n\tbh=oVcw4h3TANttVg7pu7wpG0TZi3XRT2wc3e1raPcQWUM=;\n\tb=ljpwYlOHs6ZLZHJ62hVk+uWrNO\n\t8CpYMGPGL2Nvk8CSpkUgFcQ+RDwAGogmGitBE4KYmf3qO8JdCj/NMp/gQJlh6CPhb0hyah+T6NLxU\n\tzluGNGkbjgjuqH8jTuW8LnuyYgZUphmBX0bwr858OukBIY3oU+12cZo5fUU3ZQIpmDVWpuO6SLaWi\n\t/x8eLjA6pbgX7D4Wi1nP4ruK76nHnRGcu/P4gQeu+YVsztWYqMPewUPBvplPqw/spdpGuUxnLTVNd\n\thvtUYgF6w+oSy36or8c3nlv/733kl61IOfHRhqaR8re2BqKjy62TJ9qVjMuuXsuAtpMZePuAqL9OP\n\tweFdzpPw==;","Date":"Mon, 30 Jun 2025 20:33:52 +0530","From":"Umang Jain ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Stefan Klug , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 2/6] libcamera: pipeline_handler: Allow to limit the\n\tnumber of queued requests","Message-ID":"<6oksvrtnkaqpkurkevthh6fsyasdy43of3iilavjzbp5skqkju@jtt2cm3khre3>","References":"<20250630081126.2384387-1-stefan.klug@ideasonboard.com>\n\t<20250630081126.2384387-3-stefan.klug@ideasonboard.com>\n\t\n\t<4364469b-7c12-44e4-b72a-4431647b26c7@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34744,"web_url":"https://patchwork.libcamera.org/comment/34744/","msgid":"<7d577511-577a-49fd-8597-1a79947328cb@ideasonboard.com>","date":"2025-06-30T15:21:59","subject":"Re: [PATCH v1 2/6] libcamera: pipeline_handler: Allow to limit the\n\tnumber of queued requests","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 06. 30. 17:03 keltezéssel, Umang Jain írta:\n> On Mon, Jun 30, 2025 at 02:43:21PM +0200, Barnabás Pőcze wrote:\n>> Hi\n>>\n>> 2025. 06. 30. 13:41 keltezéssel, Umang Jain írta:\n>>> Hi,\n>>>\n>>> On Mon, Jun 30, 2025 at 12:53:44PM +0200, Barnabás Pőcze wrote:\n>>>> Hi\n>>>>\n>>>> 2025. 06. 30. 12:11 keltezéssel, Umang Jain írta:\n>>>>> Hi Stefan,\n>>>>>\n>>>>> On Mon, Jun 30, 2025 at 10:11:17AM +0200, Stefan Klug wrote:\n>>>>>> Add a maxQueuedRequestsDevice constructor parameter to allow pipeline\n>>>>>> handler classes to limit the maximum number of requests that get queued\n>>>>>> to the device in queueRequestDevice().\n>>>>>>\n>>>>>> The default value is set to an arbitrary number of 32 which is big\n>>>>>> enough for all currently known use cases.\n>>>>>>\n>>>>>> Signed-off-by: Stefan Klug \n>>>>>>\n>>>>>> ---\n>>>>>>\n>>>>>> Changes in v1:\n>>>>>> - Used a const member variable to carry the maximum number of requests\n>>>>>> - Improved commit message\n>>>>>> - Added docs\n>>>>>> ---\n>>>>>> include/libcamera/internal/pipeline_handler.h | 4 +++-\n>>>>>> src/libcamera/pipeline_handler.cpp | 20 ++++++++++++++-----\n>>>>>> 2 files changed, 18 insertions(+), 6 deletions(-)\n>>>>>>\n>>>>>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n>>>>>> index be017ad47219..e89d6a33e398 100644\n>>>>>> --- a/include/libcamera/internal/pipeline_handler.h\n>>>>>> +++ b/include/libcamera/internal/pipeline_handler.h\n>>>>>> @@ -33,7 +33,8 @@ class PipelineHandler : public std::enable_shared_from_this,\n>>>>>> \t\t\tpublic Object\n>>>>>> {\n>>>>>> public:\n>>>>>> -\tPipelineHandler(CameraManager *manager);\n>>>>>> +\tPipelineHandler(CameraManager *manager,\n>>>>>> +\t\t\tunsigned int maxQueuedRequestsDevice = 32);\n>>>>>> \tvirtual ~PipelineHandler();\n>>>>>> \tvirtual bool match(DeviceEnumerator *enumerator) = 0;\n>>>>>> @@ -80,6 +81,7 @@ protected:\n>>>>>> \tvirtual void releaseDevice(Camera *camera);\n>>>>>> \tCameraManager *manager_;\n>>>>>> +\tconst unsigned int maxQueuedRequestsDevice_;\n>>>>>> private:\n>>>>>> \tvoid unlockMediaDevices();\n>>>>>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n>>>>>> index dc4086aa9bb5..383c6ad0c4aa 100644\n>>>>>> --- a/src/libcamera/pipeline_handler.cpp\n>>>>>> +++ b/src/libcamera/pipeline_handler.cpp\n>>>>>> @@ -62,13 +62,17 @@ LOG_DEFINE_CATEGORY(Pipeline)\n>>>>>> /**\n>>>>>> * \\brief Construct a PipelineHandler instance\n>>>>>> * \\param[in] manager The camera manager\n>>>>>> + * \\param[in] maxQueuedRequestsDevice The maximum number of requests queued to\n>>>>>> + * the device\n>>>>>> *\n>>>>>> * In order to honour the std::enable_shared_from_this<> contract,\n>>>>>> * PipelineHandler instances shall never be constructed manually, but always\n>>>>>> * through the PipelineHandlerFactoryBase::create() function.\n>>>>>> */\n>>>>>> -PipelineHandler::PipelineHandler(CameraManager *manager)\n>>>>>> -\t: manager_(manager), useCount_(0)\n>>>>>> +PipelineHandler::PipelineHandler(CameraManager *manager,\n>>>>>> +\t\t\t\t unsigned int maxQueuedRequestsDevice)\n>>>>>> +\t: manager_(manager), maxQueuedRequestsDevice_(maxQueuedRequestsDevice),\n>>>>>> +\t useCount_(0)\n>>>>>> {\n>>>>>> }\n>>>>>> @@ -430,9 +434,9 @@ void PipelineHandler::registerRequest(Request *request)\n>>>>>> * requests which have to be prepared to make sure they are ready for being\n>>>>>> * queued to the pipeline handler.\n>>>>>> *\n>>>>>> - * The queue of waiting requests is iterated and all prepared requests are\n>>>>>> - * passed to the pipeline handler in the same order they have been queued by\n>>>>>> - * calling this function.\n>>>>>> + * The queue of waiting requests is iterated and up to \\a\n>>>>>> + * maxQueuedRequestsDevice_ prepared requests are passed to the pipeline handler\n>>>>>> + * in the same order they have been queued by calling this function.\n>>>>>> *\n>>>>>> * If a Request fails during the preparation phase or if the pipeline handler\n>>>>>> * fails in queuing the request to the hardware the request is cancelled.\n>>>>>> @@ -487,6 +491,9 @@ void PipelineHandler::doQueueRequests(Camera *camera)\n>>>>>> {\n>>>>>> \tCamera::Private *data = camera->_d();\n>>>>>> \twhile (!data->waitingRequests_.empty()) {\n>>>>>> +\t\tif (data->queuedRequests_.size() == maxQueuedRequestsDevice_)\n>>>>>> +\t\t\tbreak;\n>>>>>> +\n>>>>>> \t\tRequest *request = data->waitingRequests_.front();\n>>>>>> \t\tif (!request->_d()->prepared_)\n>>>>>> \t\t\tbreak;\n>>>>>> @@ -568,6 +575,9 @@ void PipelineHandler::completeRequest(Request *request)\n>>>>>> \t\tdata->queuedRequests_.pop_front();\n>>>>>> \t\tcamera->requestComplete(req);\n>>>>>> \t}\n>>>>>> +\n>>>>>> +\t/* Allow any waiting requests to be queued to the pipeline. */\n>>>>>> +\tdoQueueRequests(camera);\n>>>>>\n>>>>> Is this change intended ? I am looking at your original RFC:\n>>>>> https://patchwork.libcamera.org/patch/23447/ which doesn't have this.\n>>>>>\n>>>>> I am asking this because, for me, not only this change is orthogonal\n>>>>> to the patch but also, it seems a bit 'off' to try to queue waiting\n>>>>> requests in completeRequest(). Especially considering the fact that\n>>>>> completeRequest() can be called on request-cancellation (probably on\n>>>>> a stop() sequence), and then we try to queue more requests.\n>>>>\n>>>> I think the RFC is incomplete, and this addition is required, in order to drain\n>>>> `waitingRequests_`. Any time an item is removed from `queuedRequests_`, the\n>>>\n>>> Why the waitingRequests_ needs to be drained? It simply needs to be\n>>> cancelled/cleared on stop(), as a cleanup and to prevent it\n>>> (accidently) getting queued down to the hardware..\n>>\n>> Consider the scenario where one queues `maxQueuedRequestsDevice_ + 1` requests,\n>> and waits for all of them to complete, then stops the camera. If the requests\n>> are all queued before the first one could be completed, then `waitingRequests_`\n>> will have the last request, while `queuedRequests_` will have the first `maxQueuedRequestsDevice_`\n>> requests. If the `waitingRequests_` is not processed on completion, then it\n>> will stay there forever, and it will never complete. (Unless I am missing something?)\n> \n> In your scenario, the last request 'maxQueuedRequestsDevice_ + 1`th\n> in waitingRequests_ will complete on calling stop(). It will be complete\n> by PipelineHandler::cancelRequest().\n\nIn my scenario `Camera::stop()` would only be called after the completion of the\n`maxQueuedRequestsDevice_ + 1` requests, which never happens because the last\nrequest stays in `waitingRequests_` without ever being queued.\n\n\n> \n> PipelineHandler::queueRequest() documentation also states:\n> \n> * If a Request fails during the preparation phase or if the pipeline handler\n> * fails in queuing the request to the hardware the request is cancelled.\n> \n> Does waitingRequests_ dwould fall in: \"fails in queuing the request to\n> the hardware\" category? Becuase the user queued requests too fast and\n> also beyond maxQueuedRequestsDevice_ ?\n\nMy assumption was that the point of this patch is to allow an application to\nqueue as many requests as it wants, and let libcamera deal with the limits, etc.\n\n\nRegards,\nBarnabás Pőcze\n\n> \n>>\n>>\n>> Regards,\n>> Barnabás Pőcze\n>>\n>>\n>>>\n>>>> `waitingRequests_` queue needs to be checked and the eligible requests need to\n>>>> be queued to the pipeline handler. Maybe this is not the ideal location since this\n>>>> also introduces a potential re-entrancy issue: a pipeline handler calling completeRequest()\n>>>> might get a call back into the pipeline handler, which it might not be able to handle.\n>>>> But I think something like this is needed in any case.\n>>>\n>>> I am not sure what use-case/sequence led to this particular change\n>>> (description somewhere would have been nice), but to me,\n>>> doQueueRequests() does the right thing i.e. being executed when the\n>>> request emits 'prepared', which is chained to\n>>> PipelineHandler::queueRequest().\n>>>\n>>> In normal/most situation,\n>>>\n>>> completeRequest() -> queuedRequests_.pop() -> Request::reuse()\n>>> -> queueRequest() -> prepared signal emit -> doQueueRequests()\n>>> -> waitingRequest_.front() queued.\n>>>\n>>> So, yes changes to queuedRequest_ does end up checking waitingRequests_\n>>> in the end, as mentioned above.\n>>>\n>>>>\n>>>>\n>>>> Regards,\n>>>> Barnabás Pőcze\n>>>>\n>>>>\n>>>>>\n>>>>> Now with this change, we have patch 6/6 which seems to handle the side\n>>>>> effect of this. The stop() sequence is cancelling requests, followed\n>>>>> by completeRequest(), hence to stop doQueueRequests() from queuing\n>>>>> the requests, we need to clear the waiting queue early-on. I feel, this\n>>>>> is a bit inter-twined.\n>>>>>\n>>>>> I haven't explored any concrete solutions yet, but I think we need\n>>>>> to first split off this change from this patch atleast.\n>>>>>\n>>>>>> }\n>>>>>> /**\n>>>>>> -- \n>>>>>> 2.48.1\n>>>>>>\n>>>>\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5B8A7C3237\n\tfor ;\n\tMon, 30 Jun 2025 15:22:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 16C3D68E04;\n\tMon, 30 Jun 2025 17:22:07 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 777D861527\n\tfor ;\n\tMon, 30 Jun 2025 17:22:05 +0200 (CEST)","from [192.168.33.21] (185.221.143.107.nat.pool.zt.hu\n\t[185.221.143.107])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5D051928;\n\tMon, 30 Jun 2025 17:21:40 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"aeYsYTa1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1751296903;\n\tbh=8YvKP+SRLuO15/nb6O3opQUdB5/wntgWex3O01Ts6wY=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=aeYsYTa1ic8wKJAlm7ZjoxRi5iOBhzWLOEOplkLGLm7TZ3y1TtnXHx0ZKTn5vPMSn\n\tlqlHVW5v57yfTSF2GYpEHGwZ0NZwzw5lDxt5vwFjlgk0L+PhWTJjFGr3xVdhKbLNYb\n\tFHUWdd59wGEGdskmwoy0sLzlUrXbyC0fkkyEfxqE=","Message-ID":"<7d577511-577a-49fd-8597-1a79947328cb@ideasonboard.com>","Date":"Mon, 30 Jun 2025 17:21:59 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1 2/6] libcamera: pipeline_handler: Allow to limit the\n\tnumber of queued requests","To":"Umang Jain ","Cc":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250630081126.2384387-1-stefan.klug@ideasonboard.com>\n\t<20250630081126.2384387-3-stefan.klug@ideasonboard.com>\n\t\n\t<4364469b-7c12-44e4-b72a-4431647b26c7@ideasonboard.com>\n\t\n\t\n\t<6oksvrtnkaqpkurkevthh6fsyasdy43of3iilavjzbp5skqkju@jtt2cm3khre3>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<6oksvrtnkaqpkurkevthh6fsyasdy43of3iilavjzbp5skqkju@jtt2cm3khre3>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34750,"web_url":"https://patchwork.libcamera.org/comment/34750/","msgid":"","date":"2025-07-01T05:27:16","subject":"Re: [PATCH v1 2/6] libcamera: pipeline_handler: Allow to limit the\n\tnumber of queued requests","submitter":{"id":232,"url":"https://patchwork.libcamera.org/api/people/232/","name":"Umang Jain","email":"uajain@igalia.com"},"content":"On Mon, Jun 30, 2025 at 05:21:59PM +0200, Barnabás Pőcze wrote:\n> 2025. 06. 30. 17:03 keltezéssel, Umang Jain írta:\n> > On Mon, Jun 30, 2025 at 02:43:21PM +0200, Barnabás Pőcze wrote:\n> > > Hi\n> > > \n> > > 2025. 06. 30. 13:41 keltezéssel, Umang Jain írta:\n> > > > Hi,\n> > > > \n> > > > On Mon, Jun 30, 2025 at 12:53:44PM +0200, Barnabás Pőcze wrote:\n> > > > > Hi\n> > > > > \n> > > > > 2025. 06. 30. 12:11 keltezéssel, Umang Jain írta:\n> > > > > > Hi Stefan,\n> > > > > > \n> > > > > > On Mon, Jun 30, 2025 at 10:11:17AM +0200, Stefan Klug wrote:\n> > > > > > > Add a maxQueuedRequestsDevice constructor parameter to allow pipeline\n> > > > > > > handler classes to limit the maximum number of requests that get queued\n> > > > > > > to the device in queueRequestDevice().\n> > > > > > > \n> > > > > > > The default value is set to an arbitrary number of 32 which is big\n> > > > > > > enough for all currently known use cases.\n> > > > > > > \n> > > > > > > Signed-off-by: Stefan Klug \n> > > > > > > \n> > > > > > > ---\n> > > > > > > \n> > > > > > > Changes in v1:\n> > > > > > > - Used a const member variable to carry the maximum number of requests\n> > > > > > > - Improved commit message\n> > > > > > > - Added docs\n> > > > > > > ---\n> > > > > > > include/libcamera/internal/pipeline_handler.h | 4 +++-\n> > > > > > > src/libcamera/pipeline_handler.cpp | 20 ++++++++++++++-----\n> > > > > > > 2 files changed, 18 insertions(+), 6 deletions(-)\n> > > > > > > \n> > > > > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > > > > > > index be017ad47219..e89d6a33e398 100644\n> > > > > > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > > > > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > > > > > @@ -33,7 +33,8 @@ class PipelineHandler : public std::enable_shared_from_this,\n> > > > > > > \t\t\tpublic Object\n> > > > > > > {\n> > > > > > > public:\n> > > > > > > -\tPipelineHandler(CameraManager *manager);\n> > > > > > > +\tPipelineHandler(CameraManager *manager,\n> > > > > > > +\t\t\tunsigned int maxQueuedRequestsDevice = 32);\n> > > > > > > \tvirtual ~PipelineHandler();\n> > > > > > > \tvirtual bool match(DeviceEnumerator *enumerator) = 0;\n> > > > > > > @@ -80,6 +81,7 @@ protected:\n> > > > > > > \tvirtual void releaseDevice(Camera *camera);\n> > > > > > > \tCameraManager *manager_;\n> > > > > > > +\tconst unsigned int maxQueuedRequestsDevice_;\n> > > > > > > private:\n> > > > > > > \tvoid unlockMediaDevices();\n> > > > > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > > > > > index dc4086aa9bb5..383c6ad0c4aa 100644\n> > > > > > > --- a/src/libcamera/pipeline_handler.cpp\n> > > > > > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > > > > > @@ -62,13 +62,17 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> > > > > > > /**\n> > > > > > > * \\brief Construct a PipelineHandler instance\n> > > > > > > * \\param[in] manager The camera manager\n> > > > > > > + * \\param[in] maxQueuedRequestsDevice The maximum number of requests queued to\n> > > > > > > + * the device\n> > > > > > > *\n> > > > > > > * In order to honour the std::enable_shared_from_this<> contract,\n> > > > > > > * PipelineHandler instances shall never be constructed manually, but always\n> > > > > > > * through the PipelineHandlerFactoryBase::create() function.\n> > > > > > > */\n> > > > > > > -PipelineHandler::PipelineHandler(CameraManager *manager)\n> > > > > > > -\t: manager_(manager), useCount_(0)\n> > > > > > > +PipelineHandler::PipelineHandler(CameraManager *manager,\n> > > > > > > +\t\t\t\t unsigned int maxQueuedRequestsDevice)\n> > > > > > > +\t: manager_(manager), maxQueuedRequestsDevice_(maxQueuedRequestsDevice),\n> > > > > > > +\t useCount_(0)\n> > > > > > > {\n> > > > > > > }\n> > > > > > > @@ -430,9 +434,9 @@ void PipelineHandler::registerRequest(Request *request)\n> > > > > > > * requests which have to be prepared to make sure they are ready for being\n> > > > > > > * queued to the pipeline handler.\n> > > > > > > *\n> > > > > > > - * The queue of waiting requests is iterated and all prepared requests are\n> > > > > > > - * passed to the pipeline handler in the same order they have been queued by\n> > > > > > > - * calling this function.\n> > > > > > > + * The queue of waiting requests is iterated and up to \\a\n> > > > > > > + * maxQueuedRequestsDevice_ prepared requests are passed to the pipeline handler\n> > > > > > > + * in the same order they have been queued by calling this function.\n> > > > > > > *\n> > > > > > > * If a Request fails during the preparation phase or if the pipeline handler\n> > > > > > > * fails in queuing the request to the hardware the request is cancelled.\n> > > > > > > @@ -487,6 +491,9 @@ void PipelineHandler::doQueueRequests(Camera *camera)\n> > > > > > > {\n> > > > > > > \tCamera::Private *data = camera->_d();\n> > > > > > > \twhile (!data->waitingRequests_.empty()) {\n> > > > > > > +\t\tif (data->queuedRequests_.size() == maxQueuedRequestsDevice_)\n> > > > > > > +\t\t\tbreak;\n> > > > > > > +\n> > > > > > > \t\tRequest *request = data->waitingRequests_.front();\n> > > > > > > \t\tif (!request->_d()->prepared_)\n> > > > > > > \t\t\tbreak;\n> > > > > > > @@ -568,6 +575,9 @@ void PipelineHandler::completeRequest(Request *request)\n> > > > > > > \t\tdata->queuedRequests_.pop_front();\n> > > > > > > \t\tcamera->requestComplete(req);\n> > > > > > > \t}\n> > > > > > > +\n> > > > > > > +\t/* Allow any waiting requests to be queued to the pipeline. */\n> > > > > > > +\tdoQueueRequests(camera);\n> > > > > > \n> > > > > > Is this change intended ? I am looking at your original RFC:\n> > > > > > https://patchwork.libcamera.org/patch/23447/ which doesn't have this.\n> > > > > > \n> > > > > > I am asking this because, for me, not only this change is orthogonal\n> > > > > > to the patch but also, it seems a bit 'off' to try to queue waiting\n> > > > > > requests in completeRequest(). Especially considering the fact that\n> > > > > > completeRequest() can be called on request-cancellation (probably on\n> > > > > > a stop() sequence), and then we try to queue more requests.\n> > > > > \n> > > > > I think the RFC is incomplete, and this addition is required, in order to drain\n> > > > > `waitingRequests_`. Any time an item is removed from `queuedRequests_`, the\n> > > > \n> > > > Why the waitingRequests_ needs to be drained? It simply needs to be\n> > > > cancelled/cleared on stop(), as a cleanup and to prevent it\n> > > > (accidently) getting queued down to the hardware..\n> > > \n> > > Consider the scenario where one queues `maxQueuedRequestsDevice_ + 1` requests,\n> > > and waits for all of them to complete, then stops the camera. If the requests\n> > > are all queued before the first one could be completed, then `waitingRequests_`\n> > > will have the last request, while `queuedRequests_` will have the first `maxQueuedRequestsDevice_`\n> > > requests. If the `waitingRequests_` is not processed on completion, then it\n> > > will stay there forever, and it will never complete. (Unless I am missing something?)\n> > \n> > In your scenario, the last request 'maxQueuedRequestsDevice_ + 1`th\n> > in waitingRequests_ will complete on calling stop(). It will be complete\n> > by PipelineHandler::cancelRequest().\n> \n> In my scenario `Camera::stop()` would only be called after the completion of the\n> `maxQueuedRequestsDevice_ + 1` requests, which never happens because the last\n> request stays in `waitingRequests_` without ever being queued.\n> \n\nIs this a design goal we are after - Queuing exactly (x) requests to get\n(x) requests out? I don't know how much it is feasible as of today,\ngiven the multi-stage pipelines we have today.\n\nBut if this is a design target, it should clearly stated.\n> \n> > \n> > PipelineHandler::queueRequest() documentation also states:\n> > \n> > * If a Request fails during the preparation phase or if the pipeline handler\n> > * fails in queuing the request to the hardware the request is cancelled.\n> > \n> > Does waitingRequests_ dwould fall in: \"fails in queuing the request to\n> > the hardware\" category? Becuase the user queued requests too fast and\n> > also beyond maxQueuedRequestsDevice_ ?\n> \n> My assumption was that the point of this patch is to allow an application to\n> queue as many requests as it wants, and let libcamera deal with the limits, etc.\n\nDitto.\n> \n> \n> Regards,\n> Barnabás Pőcze\n> \n> > \n> > > \n> > > \n> > > Regards,\n> > > Barnabás Pőcze\n> > > \n> > > \n> > > > \n> > > > > `waitingRequests_` queue needs to be checked and the eligible requests need to\n> > > > > be queued to the pipeline handler. Maybe this is not the ideal location since this\n> > > > > also introduces a potential re-entrancy issue: a pipeline handler calling completeRequest()\n> > > > > might get a call back into the pipeline handler, which it might not be able to handle.\n> > > > > But I think something like this is needed in any case.\n> > > > \n> > > > I am not sure what use-case/sequence led to this particular change\n> > > > (description somewhere would have been nice), but to me,\n> > > > doQueueRequests() does the right thing i.e. being executed when the\n> > > > request emits 'prepared', which is chained to\n> > > > PipelineHandler::queueRequest().\n> > > > \n> > > > In normal/most situation,\n> > > > \n> > > > completeRequest() -> queuedRequests_.pop() -> Request::reuse()\n> > > > -> queueRequest() -> prepared signal emit -> doQueueRequests()\n> > > > -> waitingRequest_.front() queued.\n> > > > \n> > > > So, yes changes to queuedRequest_ does end up checking waitingRequests_\n> > > > in the end, as mentioned above.\n> > > > \n> > > > > \n> > > > > \n> > > > > Regards,\n> > > > > Barnabás Pőcze\n> > > > > \n> > > > > \n> > > > > > \n> > > > > > Now with this change, we have patch 6/6 which seems to handle the side\n> > > > > > effect of this. The stop() sequence is cancelling requests, followed\n> > > > > > by completeRequest(), hence to stop doQueueRequests() from queuing\n> > > > > > the requests, we need to clear the waiting queue early-on. I feel, this\n> > > > > > is a bit inter-twined.\n> > > > > > \n> > > > > > I haven't explored any concrete solutions yet, but I think we need\n> > > > > > to first split off this change from this patch atleast.\n> > > > > > \n> > > > > > > }\n> > > > > > > /**\n> > > > > > > -- \n> > > > > > > 2.48.1\n> > > > > > > \n> > > > > \n> > > \n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0ED04BDCBF\n\tfor ;\n\tTue, 1 Jul 2025 05:27:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 545EB68E0E;\n\tTue, 1 Jul 2025 07:27:15 +0200 (CEST)","from fanzine2.igalia.com (fanzine2.igalia.com [213.97.179.56])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BC13661524\n\tfor ;\n\tTue, 1 Jul 2025 07:27:13 +0200 (CEST)","from [49.36.69.141] (helo=uajain)\n\tby fanzine2.igalia.com with esmtpsa \n\t(Cipher TLS1.3:ECDHE_SECP256R1__RSA_PSS_RSAE_SHA256__AES_256_GCM:256)\n\t(Exim) id 1uWTWS-00AotR-Kg; Tue, 01 Jul 2025 07:27:13 +0200"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=igalia.com header.i=@igalia.com\n\theader.b=\"J5qXdXW1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=igalia.com;\n\ts=20170329;\n\th=In-Reply-To:Content-Transfer-Encoding:Content-Type:MIME-Version\n\t:References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To:Content-ID:\n\tContent-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc\n\t:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe:\n\tList-Post:List-Owner:List-Archive;\n\tbh=ddKwSMhJy6ftQndyGxsaqxd2cTS6BUvFhnjtaIviFNc=;\n\tb=J5qXdXW1JcKUJSBZMFtsIp6YTh\n\tkFrsIK0FviwtGHGRATWVpbA/tLywpypmJM2K+ImRu2ZWHNdzpeHhHOR46yueGaC80hrS1cbXTn78P\n\txzOpHRtE3LbJL3rECJsDR7ZHcifvlsP9pk66X++INytr59AXBfbY+KZGOVg7w4nNl5HRzGGtFoYDJ\n\tNH4T40naqTeG6Awig+7l9AsofVNl87wG5pUwTRJ+TynQmTv0wldXAha3b6hvyvC+gzWiV9XGKLuxe\n\tpaxKkB8rZqVBnqhkA+GsEtnIbRq3hPtH4s1qN0I5LoiuYln89vafIu+o8jeVpeKZqruOw2kgy8RJO\n\t46E1annw==;","Date":"Tue, 1 Jul 2025 10:57:16 +0530","From":"Umang Jain ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Stefan Klug , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 2/6] libcamera: pipeline_handler: Allow to limit the\n\tnumber of queued requests","Message-ID":"","References":"<20250630081126.2384387-1-stefan.klug@ideasonboard.com>\n\t<20250630081126.2384387-3-stefan.klug@ideasonboard.com>\n\t\n\t<4364469b-7c12-44e4-b72a-4431647b26c7@ideasonboard.com>\n\t\n\t\n\t<6oksvrtnkaqpkurkevthh6fsyasdy43of3iilavjzbp5skqkju@jtt2cm3khre3>\n\t<7d577511-577a-49fd-8597-1a79947328cb@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<7d577511-577a-49fd-8597-1a79947328cb@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34751,"web_url":"https://patchwork.libcamera.org/comment/34751/","msgid":"<175135620616.2426344.6419396843387579280@localhost>","date":"2025-07-01T07:50:06","subject":"Re: [PATCH v1 2/6] libcamera: pipeline_handler: Allow to limit the\n\tnumber of queued requests","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Umang,\n\nQuoting Umang Jain (2025-07-01 07:27:16)\n> On Mon, Jun 30, 2025 at 05:21:59PM +0200, Barnabás Pőcze wrote:\n> > 2025. 06. 30. 17:03 keltezéssel, Umang Jain írta:\n> > > On Mon, Jun 30, 2025 at 02:43:21PM +0200, Barnabás Pőcze wrote:\n> > > > Hi\n> > > > \n> > > > 2025. 06. 30. 13:41 keltezéssel, Umang Jain írta:\n> > > > > Hi,\n> > > > > \n> > > > > On Mon, Jun 30, 2025 at 12:53:44PM +0200, Barnabás Pőcze wrote:\n> > > > > > Hi\n> > > > > > \n> > > > > > 2025. 06. 30. 12:11 keltezéssel, Umang Jain írta:\n> > > > > > > Hi Stefan,\n> > > > > > > \n> > > > > > > On Mon, Jun 30, 2025 at 10:11:17AM +0200, Stefan Klug wrote:\n> > > > > > > > Add a maxQueuedRequestsDevice constructor parameter to allow pipeline\n> > > > > > > > handler classes to limit the maximum number of requests that get queued\n> > > > > > > > to the device in queueRequestDevice().\n> > > > > > > > \n> > > > > > > > The default value is set to an arbitrary number of 32 which is big\n> > > > > > > > enough for all currently known use cases.\n> > > > > > > > \n> > > > > > > > Signed-off-by: Stefan Klug \n> > > > > > > > \n> > > > > > > > ---\n> > > > > > > > \n> > > > > > > > Changes in v1:\n> > > > > > > > - Used a const member variable to carry the maximum number of requests\n> > > > > > > > - Improved commit message\n> > > > > > > > - Added docs\n> > > > > > > > ---\n> > > > > > > > include/libcamera/internal/pipeline_handler.h | 4 +++-\n> > > > > > > > src/libcamera/pipeline_handler.cpp | 20 ++++++++++++++-----\n> > > > > > > > 2 files changed, 18 insertions(+), 6 deletions(-)\n> > > > > > > > \n> > > > > > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > > > > > > > index be017ad47219..e89d6a33e398 100644\n> > > > > > > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > > > > > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > > > > > > @@ -33,7 +33,8 @@ class PipelineHandler : public std::enable_shared_from_this,\n> > > > > > > > public Object\n> > > > > > > > {\n> > > > > > > > public:\n> > > > > > > > - PipelineHandler(CameraManager *manager);\n> > > > > > > > + PipelineHandler(CameraManager *manager,\n> > > > > > > > + unsigned int maxQueuedRequestsDevice = 32);\n> > > > > > > > virtual ~PipelineHandler();\n> > > > > > > > virtual bool match(DeviceEnumerator *enumerator) = 0;\n> > > > > > > > @@ -80,6 +81,7 @@ protected:\n> > > > > > > > virtual void releaseDevice(Camera *camera);\n> > > > > > > > CameraManager *manager_;\n> > > > > > > > + const unsigned int maxQueuedRequestsDevice_;\n> > > > > > > > private:\n> > > > > > > > void unlockMediaDevices();\n> > > > > > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > > > > > > > index dc4086aa9bb5..383c6ad0c4aa 100644\n> > > > > > > > --- a/src/libcamera/pipeline_handler.cpp\n> > > > > > > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > > > > > > @@ -62,13 +62,17 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> > > > > > > > /**\n> > > > > > > > * \\brief Construct a PipelineHandler instance\n> > > > > > > > * \\param[in] manager The camera manager\n> > > > > > > > + * \\param[in] maxQueuedRequestsDevice The maximum number of requests queued to\n> > > > > > > > + * the device\n> > > > > > > > *\n> > > > > > > > * In order to honour the std::enable_shared_from_this<> contract,\n> > > > > > > > * PipelineHandler instances shall never be constructed manually, but always\n> > > > > > > > * through the PipelineHandlerFactoryBase::create() function.\n> > > > > > > > */\n> > > > > > > > -PipelineHandler::PipelineHandler(CameraManager *manager)\n> > > > > > > > - : manager_(manager), useCount_(0)\n> > > > > > > > +PipelineHandler::PipelineHandler(CameraManager *manager,\n> > > > > > > > + unsigned int maxQueuedRequestsDevice)\n> > > > > > > > + : manager_(manager), maxQueuedRequestsDevice_(maxQueuedRequestsDevice),\n> > > > > > > > + useCount_(0)\n> > > > > > > > {\n> > > > > > > > }\n> > > > > > > > @@ -430,9 +434,9 @@ void PipelineHandler::registerRequest(Request *request)\n> > > > > > > > * requests which have to be prepared to make sure they are ready for being\n> > > > > > > > * queued to the pipeline handler.\n> > > > > > > > *\n> > > > > > > > - * The queue of waiting requests is iterated and all prepared requests are\n> > > > > > > > - * passed to the pipeline handler in the same order they have been queued by\n> > > > > > > > - * calling this function.\n> > > > > > > > + * The queue of waiting requests is iterated and up to \\a\n> > > > > > > > + * maxQueuedRequestsDevice_ prepared requests are passed to the pipeline handler\n> > > > > > > > + * in the same order they have been queued by calling this function.\n> > > > > > > > *\n> > > > > > > > * If a Request fails during the preparation phase or if the pipeline handler\n> > > > > > > > * fails in queuing the request to the hardware the request is cancelled.\n> > > > > > > > @@ -487,6 +491,9 @@ void PipelineHandler::doQueueRequests(Camera *camera)\n> > > > > > > > {\n> > > > > > > > Camera::Private *data = camera->_d();\n> > > > > > > > while (!data->waitingRequests_.empty()) {\n> > > > > > > > + if (data->queuedRequests_.size() == maxQueuedRequestsDevice_)\n> > > > > > > > + break;\n> > > > > > > > +\n> > > > > > > > Request *request = data->waitingRequests_.front();\n> > > > > > > > if (!request->_d()->prepared_)\n> > > > > > > > break;\n> > > > > > > > @@ -568,6 +575,9 @@ void PipelineHandler::completeRequest(Request *request)\n> > > > > > > > data->queuedRequests_.pop_front();\n> > > > > > > > camera->requestComplete(req);\n> > > > > > > > }\n> > > > > > > > +\n> > > > > > > > + /* Allow any waiting requests to be queued to the pipeline. */\n> > > > > > > > + doQueueRequests(camera);\n> > > > > > > \n> > > > > > > Is this change intended ? I am looking at your original RFC:\n> > > > > > > https://patchwork.libcamera.org/patch/23447/ which doesn't have this.\n> > > > > > > \n> > > > > > > I am asking this because, for me, not only this change is orthogonal\n> > > > > > > to the patch but also, it seems a bit 'off' to try to queue waiting\n> > > > > > > requests in completeRequest(). Especially considering the fact that\n> > > > > > > completeRequest() can be called on request-cancellation (probably on\n> > > > > > > a stop() sequence), and then we try to queue more requests.\n> > > > > > \n> > > > > > I think the RFC is incomplete, and this addition is required, in order to drain\n> > > > > > `waitingRequests_`. Any time an item is removed from `queuedRequests_`, the\n\nYes, exactly. And it opened a bug that got addressed in patch 6/6. Think\nof an application queuing 100 requests in advance (That really starts to\nmake sense when we split buffers from controls). In that case there\nwould be 96 requests in waitingRequests_ and these need to be fed into\nthe pipeline handler as soon as there is room (which is after\ncompleteRequest()) \n\n> > > > > \n> > > > > Why the waitingRequests_ needs to be drained? It simply needs to be\n> > > > > cancelled/cleared on stop(), as a cleanup and to prevent it\n> > > > > (accidently) getting queued down to the hardware..\n> > > > \n> > > > Consider the scenario where one queues `maxQueuedRequestsDevice_ + 1` requests,\n> > > > and waits for all of them to complete, then stops the camera. If the requests\n> > > > are all queued before the first one could be completed, then `waitingRequests_`\n> > > > will have the last request, while `queuedRequests_` will have the first `maxQueuedRequestsDevice_`\n> > > > requests. If the `waitingRequests_` is not processed on completion, then it\n> > > > will stay there forever, and it will never complete. (Unless I am missing something?)\n> > > \n> > > In your scenario, the last request 'maxQueuedRequestsDevice_ + 1`th\n> > > in waitingRequests_ will complete on calling stop(). It will be complete\n> > > by PipelineHandler::cancelRequest().\n> > \n> > In my scenario `Camera::stop()` would only be called after the completion of the\n> > `maxQueuedRequestsDevice_ + 1` requests, which never happens because the last\n> > request stays in `waitingRequests_` without ever being queued.\n> > \n> \n> Is this a design goal we are after - Queuing exactly (x) requests to get\n> (x) requests out? I don't know how much it is feasible as of today,\n> given the multi-stage pipelines we have today.\n\nI think I'm missing something here. The idea of this series is to allow\nto queue more than pipeline depth requests (practically an arbitrary\nnumber) into libcamera. The pipeline handler decides how many requests\nit wants to consume. All the others stay in the waitingQueue until there\nis room again in the pipeline handler. Maybe I'm missing a bigger\nconcept that I wasn't aware of.\n\nBest regards,\nStefan\n\n> \n> But if this is a design target, it should clearly stated.\n> > \n> > > \n> > > PipelineHandler::queueRequest() documentation also states:\n> > > \n> > > * If a Request fails during the preparation phase or if the pipeline handler\n> > > * fails in queuing the request to the hardware the request is cancelled.\n> > > \n> > > Does waitingRequests_ dwould fall in: \"fails in queuing the request to\n> > > the hardware\" category? Becuase the user queued requests too fast and\n> > > also beyond maxQueuedRequestsDevice_ ?\n> > \n> > My assumption was that the point of this patch is to allow an application to\n> > queue as many requests as it wants, and let libcamera deal with the limits, etc.\n> \n> Ditto.\n> > \n> > \n> > Regards,\n> > Barnabás Pőcze\n> > \n> > > \n> > > > \n> > > > \n> > > > Regards,\n> > > > Barnabás Pőcze\n> > > > \n> > > > \n> > > > > \n> > > > > > `waitingRequests_` queue needs to be checked and the eligible requests need to\n> > > > > > be queued to the pipeline handler. Maybe this is not the ideal location since this\n> > > > > > also introduces a potential re-entrancy issue: a pipeline handler calling completeRequest()\n> > > > > > might get a call back into the pipeline handler, which it might not be able to handle.\n> > > > > > But I think something like this is needed in any case.\n> > > > > \n> > > > > I am not sure what use-case/sequence led to this particular change\n> > > > > (description somewhere would have been nice), but to me,\n> > > > > doQueueRequests() does the right thing i.e. being executed when the\n> > > > > request emits 'prepared', which is chained to\n> > > > > PipelineHandler::queueRequest().\n> > > > > \n> > > > > In normal/most situation,\n> > > > > \n> > > > > completeRequest() -> queuedRequests_.pop() -> Request::reuse()\n> > > > > -> queueRequest() -> prepared signal emit -> doQueueRequests()\n> > > > > -> waitingRequest_.front() queued.\n> > > > > \n> > > > > So, yes changes to queuedRequest_ does end up checking waitingRequests_\n> > > > > in the end, as mentioned above.\n> > > > > \n> > > > > > \n> > > > > > \n> > > > > > Regards,\n> > > > > > Barnabás Pőcze\n> > > > > > \n> > > > > > \n> > > > > > > \n> > > > > > > Now with this change, we have patch 6/6 which seems to handle the side\n> > > > > > > effect of this. The stop() sequence is cancelling requests, followed\n> > > > > > > by completeRequest(), hence to stop doQueueRequests() from queuing\n> > > > > > > the requests, we need to clear the waiting queue early-on. I feel, this\n> > > > > > > is a bit inter-twined.\n> > > > > > > \n> > > > > > > I haven't explored any concrete solutions yet, but I think we need\n> > > > > > > to first split off this change from this patch atleast.\n> > > > > > > \n> > > > > > > > }\n> > > > > > > > /**\n> > > > > > > > -- \n> > > > > > > > 2.48.1\n> > > > > > > > \n> > > > > > \n> > > > \n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id ECE82C3237\n\tfor ;\n\tTue, 1 Jul 2025 07:50:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ADBC768E11;\n\tTue, 1 Jul 2025 09:50:10 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8145761528\n\tfor ;\n\tTue, 1 Jul 2025 09:50:09 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:7cdc:2548:b8ee:3b98])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id D74787E1;\n\tTue, 1 Jul 2025 09:49:46 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"anYRUKaD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1751356186;\n\tbh=LyhUnufNKwKwlmwZ1rUJu1fzgo1oFmWlNp4oxqZ+g34=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=anYRUKaDl5BykZg27ecFmS9wPfhcGnIU6DONzI85sAP0XbpQCh5ZkvdgBSi1R0Ug3\n\tcFjLTTTCbJTuaA8A9KEYCfwy5YBy/m0fQKqPsgQHl8BU3cAV0KgmzLT7ZiUySpxH7D\n\taH3O2AjeKl+YTwG5YgJrP5O9xmAVAoTk+qGA4LpI=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20250630081126.2384387-1-stefan.klug@ideasonboard.com>\n\t<20250630081126.2384387-3-stefan.klug@ideasonboard.com>\n\t\n\t<4364469b-7c12-44e4-b72a-4431647b26c7@ideasonboard.com>\n\t\n\t\n\t<6oksvrtnkaqpkurkevthh6fsyasdy43of3iilavjzbp5skqkju@jtt2cm3khre3>\n\t<7d577511-577a-49fd-8597-1a79947328cb@ideasonboard.com>\n\t","Subject":"Re: [PATCH v1 2/6] libcamera: pipeline_handler: Allow to limit the\n\tnumber of queued requests","From":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tUmang Jain ","Date":"Tue, 01 Jul 2025 09:50:06 +0200","Message-ID":"<175135620616.2426344.6419396843387579280@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34774,"web_url":"https://patchwork.libcamera.org/comment/34774/","msgid":"","date":"2025-07-02T06:40:10","subject":"Re: [PATCH v1 2/6] libcamera: pipeline_handler: Allow to limit the\n\tnumber of queued requests","submitter":{"id":232,"url":"https://patchwork.libcamera.org/api/people/232/","name":"Umang Jain","email":"uajain@igalia.com"},"content":"Hi,\n\nSmall follow ups questions:\n\nOn Mon, Jun 30, 2025 at 10:11:17AM +0200, Stefan Klug wrote:\n> Add a maxQueuedRequestsDevice constructor parameter to allow pipeline\n> handler classes to limit the maximum number of requests that get queued\n> to the device in queueRequestDevice().\n> \n> The default value is set to an arbitrary number of 32 which is big\n> enough for all currently known use cases.\n\nIs the default:32, some how derived from VB2_MAX_FRAME ? \n> \n> Signed-off-by: Stefan Klug \n> \n> ---\n> \n> Changes in v1:\n> - Used a const member variable to carry the maximum number of requests\n> - Improved commit message\n> - Added docs\n> ---\n> include/libcamera/internal/pipeline_handler.h | 4 +++-\n> src/libcamera/pipeline_handler.cpp | 20 ++++++++++++++-----\n> 2 files changed, 18 insertions(+), 6 deletions(-)\n> \n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index be017ad47219..e89d6a33e398 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -33,7 +33,8 @@ class PipelineHandler : public std::enable_shared_from_this,\n> \t\t\tpublic Object\n> {\n> public:\n> -\tPipelineHandler(CameraManager *manager);\n> +\tPipelineHandler(CameraManager *manager,\n> +\t\t\tunsigned int maxQueuedRequestsDevice = 32);\n\nIs clamping needed, or Pipeline handlers are free to set\nmaxQueuedRequestsDevice > 32 ?\n\n> \tvirtual ~PipelineHandler();\n> \n> \tvirtual bool match(DeviceEnumerator *enumerator) = 0;\n> @@ -80,6 +81,7 @@ protected:\n> \tvirtual void releaseDevice(Camera *camera);\n> \n> \tCameraManager *manager_;\n> +\tconst unsigned int maxQueuedRequestsDevice_;\n> \n> private:\n> \tvoid unlockMediaDevices();\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index dc4086aa9bb5..383c6ad0c4aa 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -62,13 +62,17 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> /**\n> * \\brief Construct a PipelineHandler instance\n> * \\param[in] manager The camera manager\n> + * \\param[in] maxQueuedRequestsDevice The maximum number of requests queued to\n> + * the device\n> *\n> * In order to honour the std::enable_shared_from_this<> contract,\n> * PipelineHandler instances shall never be constructed manually, but always\n> * through the PipelineHandlerFactoryBase::create() function.\n> */\n> -PipelineHandler::PipelineHandler(CameraManager *manager)\n> -\t: manager_(manager), useCount_(0)\n> +PipelineHandler::PipelineHandler(CameraManager *manager,\n> +\t\t\t\t unsigned int maxQueuedRequestsDevice)\n> +\t: manager_(manager), maxQueuedRequestsDevice_(maxQueuedRequestsDevice),\n> +\t useCount_(0)\n> {\n> }\n> \n> @@ -430,9 +434,9 @@ void PipelineHandler::registerRequest(Request *request)\n> * requests which have to be prepared to make sure they are ready for being\n> * queued to the pipeline handler.\n> *\n> - * The queue of waiting requests is iterated and all prepared requests are\n> - * passed to the pipeline handler in the same order they have been queued by\n> - * calling this function.\n> + * The queue of waiting requests is iterated and up to \\a\n> + * maxQueuedRequestsDevice_ prepared requests are passed to the pipeline handler\n> + * in the same order they have been queued by calling this function.\n> *\n> * If a Request fails during the preparation phase or if the pipeline handler\n> * fails in queuing the request to the hardware the request is cancelled.\n> @@ -487,6 +491,9 @@ void PipelineHandler::doQueueRequests(Camera *camera)\n> {\n> \tCamera::Private *data = camera->_d();\n> \twhile (!data->waitingRequests_.empty()) {\n> +\t\tif (data->queuedRequests_.size() == maxQueuedRequestsDevice_)\n> +\t\t\tbreak;\n> +\n\nThought exercise probably, does this make sense?\n\n +\t\tif (data->queuedRequests_.size() == data->maxQueuedRequestsDevice_)\n +\t\t\tbreak;\n\ni.e. maxQueuedRequestsDevice_ is per-camera pipeline, and not per\npipeline-handler. I do not have concrete use cases, but if forced to\nthink:\n\ncamera0: sensor <> (de)/(se)rializer <> mipi-csi0 <> isp0 and\ncamera1: sensor <> mipi-csi1 <> isp1\n\non i.MX8MPi/pi5, and serializers components have other (lesser) queue\nlimits?\n\n> \t\tRequest *request = data->waitingRequests_.front();\n> \t\tif (!request->_d()->prepared_)\n> \t\t\tbreak;\n> @@ -568,6 +575,9 @@ void PipelineHandler::completeRequest(Request *request)\n> \t\tdata->queuedRequests_.pop_front();\n> \t\tcamera->requestComplete(req);\n> \t}\n> +\n> +\t/* Allow any waiting requests to be queued to the pipeline. */\n> +\tdoQueueRequests(camera);\n> }\n> \n> /**\n> -- \n> 2.48.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 5A796C3237\n\tfor ;\n\tWed, 2 Jul 2025 06:40:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 92C5468E30;\n\tWed, 2 Jul 2025 08:40:12 +0200 (CEST)","from fanzine2.igalia.com (fanzine2.igalia.com [213.97.179.56])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 894CC6151E\n\tfor ;\n\tWed, 2 Jul 2025 08:40:10 +0200 (CEST)","from [49.36.69.17] (helo=uajain) by fanzine2.igalia.com with\n\tesmtpsa \n\t(Cipher TLS1.3:ECDHE_SECP256R1__RSA_PSS_RSAE_SHA256__AES_256_GCM:256)\n\t(Exim) id 1uWr8a-00BKd4-3T; Wed, 02 Jul 2025 08:40:08 +0200"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=igalia.com header.i=@igalia.com\n\theader.b=\"rBUBFJ4u\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=igalia.com;\n\ts=20170329;\n\th=In-Reply-To:Content-Type:MIME-Version:References:Message-ID:\n\tSubject:Cc:To:From:Date:Sender:Reply-To:Content-Transfer-Encoding:Content-ID:\n\tContent-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc\n\t:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe:\n\tList-Post:List-Owner:List-Archive;\n\tbh=YchPDKU5Iw/iY7aQZkAeoy3LaVooh5+IHlI2g22qBIU=;\n\tb=rBUBFJ4uTBCQaSrtCXKd39L95R\n\tkv5wvj2alecJ1evgTSjZNrgi1bGp5W5w62yCnHgTxC5zHYIsOCtKPelaRlZV9XqbSuDMU4SDSkges\n\t97HeoHTsLk/aWM7yrRjUw+N6N/nyv2wumEqCrzJMT9IHOfSZ4P6+cPgNujnZF/wS43QTAxhfMLld0\n\tLYmaeT4JasLMfAIspIXFCLSPE8j++8egJBbbHUwjAHPRYpAl6hU0TZRNND1Y80vnN67+5fv1qVjdB\n\tRCKqVt3/bmUah1CrBIma1zv3CTmLj15gw4lf29CpmOBrDGhC/znBLv//yr0BtVBwllknFwWnsvwH3\n\tHuLTvtIA==;","Date":"Wed, 2 Jul 2025 12:10:10 +0530","From":"Umang Jain ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 2/6] libcamera: pipeline_handler: Allow to limit the\n\tnumber of queued requests","Message-ID":"","References":"<20250630081126.2384387-1-stefan.klug@ideasonboard.com>\n\t<20250630081126.2384387-3-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20250630081126.2384387-3-stefan.klug@ideasonboard.com>","User-Agent":"NeoMutt/20250510-dirty","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34785,"web_url":"https://patchwork.libcamera.org/comment/34785/","msgid":"<76942e19-1074-48b4-b048-13614b30ac1e@pengutronix.de>","date":"2025-07-07T07:26:31","subject":"Re: [PATCH v1 2/6] libcamera: pipeline_handler: Allow to limit the\n\tnumber of queued requests","submitter":{"id":225,"url":"https://patchwork.libcamera.org/api/people/225/","name":"Sven Püschel","email":"s.pueschel@pengutronix.de"},"content":"Hi Stefan,\n\nOn 6/30/25 10:11, Stefan Klug wrote:\n> Add a maxQueuedRequestsDevice constructor parameter to allow pipeline\n> handler classes to limit the maximum number of requests that get queued\n> to the device in queueRequestDevice().\n>\n> The default value is set to an arbitrary number of 32 which is big\n> enough for all currently known use cases.\n>\n> Signed-off-by: Stefan Klug \n>\n> ---\n>\n> Changes in v1:\n> - Used a const member variable to carry the maximum number of requests\n> - Improved commit message\n> - Added docs\n> ---\n> include/libcamera/internal/pipeline_handler.h | 4 +++-\n> src/libcamera/pipeline_handler.cpp | 20 ++++++++++++++-----\n> 2 files changed, 18 insertions(+), 6 deletions(-)\n>\n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index be017ad47219..e89d6a33e398 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -33,7 +33,8 @@ class PipelineHandler : public std::enable_shared_from_this,\n> \t\t\tpublic Object\n> {\n> public:\n> -\tPipelineHandler(CameraManager *manager);\n> +\tPipelineHandler(CameraManager *manager,\n> +\t\t\tunsigned int maxQueuedRequestsDevice = 32);\n> \tvirtual ~PipelineHandler();\n> \n> \tvirtual bool match(DeviceEnumerator *enumerator) = 0;\n> @@ -80,6 +81,7 @@ protected:\n> \tvirtual void releaseDevice(Camera *camera);\n> \n> \tCameraManager *manager_;\n> +\tconst unsigned int maxQueuedRequestsDevice_;\n> \n> private:\n> \tvoid unlockMediaDevices();\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index dc4086aa9bb5..383c6ad0c4aa 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -62,13 +62,17 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> /**\n> * \\brief Construct a PipelineHandler instance\n> * \\param[in] manager The camera manager\n> + * \\param[in] maxQueuedRequestsDevice The maximum number of requests queued to\n> + * the device\n> *\n> * In order to honour the std::enable_shared_from_this<> contract,\n> * PipelineHandler instances shall never be constructed manually, but always\n> * through the PipelineHandlerFactoryBase::create() function.\n> */\n> -PipelineHandler::PipelineHandler(CameraManager *manager)\n> -\t: manager_(manager), useCount_(0)\n> +PipelineHandler::PipelineHandler(CameraManager *manager,\n> +\t\t\t\t unsigned int maxQueuedRequestsDevice)\n> +\t: manager_(manager), maxQueuedRequestsDevice_(maxQueuedRequestsDevice),\n> +\t useCount_(0)\n> {\n> }\n> \n> @@ -430,9 +434,9 @@ void PipelineHandler::registerRequest(Request *request)\n> * requests which have to be prepared to make sure they are ready for being\n> * queued to the pipeline handler.\n> *\n> - * The queue of waiting requests is iterated and all prepared requests are\n> - * passed to the pipeline handler in the same order they have been queued by\n> - * calling this function.\n> + * The queue of waiting requests is iterated and up to \\a\n> + * maxQueuedRequestsDevice_ prepared requests are passed to the pipeline handler\n> + * in the same order they have been queued by calling this function.\n> *\n> * If a Request fails during the preparation phase or if the pipeline handler\n> * fails in queuing the request to the hardware the request is cancelled.\n> @@ -487,6 +491,9 @@ void PipelineHandler::doQueueRequests(Camera *camera)\n> {\n> \tCamera::Private *data = camera->_d();\n> \twhile (!data->waitingRequests_.empty()) {\n> +\t\tif (data->queuedRequests_.size() == maxQueuedRequestsDevice_)\n\nI'd suggest to replace == with >=\n\nI think this was already mentioned in the RFC patchset. Or was there a \nreason against this? Then I'd suggest adding an \nASSERT(data->queuedRequests_.size() <= maxQueuedRequestsDevice_)\n\nSincerely Sven\n\n> +\t\t\tbreak;\n> +\n> \t\tRequest *request = data->waitingRequests_.front();\n> \t\tif (!request->_d()->prepared_)\n> \t\t\tbreak;\n> @@ -568,6 +575,9 @@ void PipelineHandler::completeRequest(Request *request)\n> \t\tdata->queuedRequests_.pop_front();\n> \t\tcamera->requestComplete(req);\n> \t}\n> +\n> +\t/* Allow any waiting requests to be queued to the pipeline. */\n> +\tdoQueueRequests(camera);\n> }\n> \n> /**","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 78E86C3237\n\tfor ;\n\tMon, 7 Jul 2025 07:26:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CFD3E68E7E;\n\tMon, 7 Jul 2025 09:26:33 +0200 (CEST)","from metis.whiteo.stw.pengutronix.de\n\t(metis.whiteo.stw.pengutronix.de [IPv6:2a0a:edc0:2:b01:1d::104])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EF03D68E66\n\tfor ;\n\tMon, 7 Jul 2025 09:26:31 +0200 (CEST)","from ptz.office.stw.pengutronix.de ([2a0a:edc0:0:900:1d::77]\n\thelo=[127.0.0.1])\n\tby metis.whiteo.stw.pengutronix.de with esmtp (Exim 4.92)\n\t(envelope-from ) id 1uYgFD-0006Sv-AC\n\tfor libcamera-devel@lists.libcamera.org;\n\tMon, 07 Jul 2025 09:26:31 +0200"],"Message-ID":"<76942e19-1074-48b4-b048-13614b30ac1e@pengutronix.de>","Date":"Mon, 7 Jul 2025 09:26:31 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1 2/6] libcamera: pipeline_handler: Allow to limit the\n\tnumber of queued requests","To":"libcamera-devel@lists.libcamera.org","References":"<20250630081126.2384387-1-stefan.klug@ideasonboard.com>\n\t<20250630081126.2384387-3-stefan.klug@ideasonboard.com>","Content-Language":"en-US","From":"=?utf-8?q?Sven_P=C3=BCschel?= ","In-Reply-To":"<20250630081126.2384387-3-stefan.klug@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","X-SA-Exim-Connect-IP":"2a0a:edc0:0:900:1d::77","X-SA-Exim-Mail-From":"s.pueschel@pengutronix.de","X-SA-Exim-Scanned":"No (on metis.whiteo.stw.pengutronix.de);\n\tSAEximRunCond expanded to false","X-PTX-Original-Recipient":"libcamera-devel@lists.libcamera.org","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34786,"web_url":"https://patchwork.libcamera.org/comment/34786/","msgid":"<175187361962.2426344.18413863794511170293@localhost>","date":"2025-07-07T07:33:39","subject":"Re: [PATCH v1 2/6] libcamera: pipeline_handler: Allow to limit the\n\tnumber of queued requests","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Umang,\n\nThanks for the feedback,\n\nQuoting Umang Jain (2025-07-02 08:40:10)\n> Hi,\n> \n> Small follow ups questions:\n> \n> On Mon, Jun 30, 2025 at 10:11:17AM +0200, Stefan Klug wrote:\n> > Add a maxQueuedRequestsDevice constructor parameter to allow pipeline\n> > handler classes to limit the maximum number of requests that get queued\n> > to the device in queueRequestDevice().\n> > \n> > The default value is set to an arbitrary number of 32 which is big\n> > enough for all currently known use cases.\n> \n> Is the default:32, some how derived from VB2_MAX_FRAME ? \n\nMerely by coincidence.\n\n> > \n> > Signed-off-by: Stefan Klug \n> > \n> > ---\n> > \n> > Changes in v1:\n> > - Used a const member variable to carry the maximum number of requests\n> > - Improved commit message\n> > - Added docs\n> > ---\n> > include/libcamera/internal/pipeline_handler.h | 4 +++-\n> > src/libcamera/pipeline_handler.cpp | 20 ++++++++++++++-----\n> > 2 files changed, 18 insertions(+), 6 deletions(-)\n> > \n> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > index be017ad47219..e89d6a33e398 100644\n> > --- a/include/libcamera/internal/pipeline_handler.h\n> > +++ b/include/libcamera/internal/pipeline_handler.h\n> > @@ -33,7 +33,8 @@ class PipelineHandler : public std::enable_shared_from_this,\n> > public Object\n> > {\n> > public:\n> > - PipelineHandler(CameraManager *manager);\n> > + PipelineHandler(CameraManager *manager,\n> > + unsigned int maxQueuedRequestsDevice = 32);\n> \n> Is clamping needed, or Pipeline handlers are free to set\n> maxQueuedRequestsDevice > 32 ?\n\nI think a pipeline handler should have the knowledge if it is capable of\nhandling > 32 requests. I wouldn't limit that.\n\n> \n> > virtual ~PipelineHandler();\n> > \n> > virtual bool match(DeviceEnumerator *enumerator) = 0;\n> > @@ -80,6 +81,7 @@ protected:\n> > virtual void releaseDevice(Camera *camera);\n> > \n> > CameraManager *manager_;\n> > + const unsigned int maxQueuedRequestsDevice_;\n> > \n> > private:\n> > void unlockMediaDevices();\n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index dc4086aa9bb5..383c6ad0c4aa 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -62,13 +62,17 @@ LOG_DEFINE_CATEGORY(Pipeline)\n> > /**\n> > * \\brief Construct a PipelineHandler instance\n> > * \\param[in] manager The camera manager\n> > + * \\param[in] maxQueuedRequestsDevice The maximum number of requests queued to\n> > + * the device\n> > *\n> > * In order to honour the std::enable_shared_from_this<> contract,\n> > * PipelineHandler instances shall never be constructed manually, but always\n> > * through the PipelineHandlerFactoryBase::create() function.\n> > */\n> > -PipelineHandler::PipelineHandler(CameraManager *manager)\n> > - : manager_(manager), useCount_(0)\n> > +PipelineHandler::PipelineHandler(CameraManager *manager,\n> > + unsigned int maxQueuedRequestsDevice)\n> > + : manager_(manager), maxQueuedRequestsDevice_(maxQueuedRequestsDevice),\n> > + useCount_(0)\n> > {\n> > }\n> > \n> > @@ -430,9 +434,9 @@ void PipelineHandler::registerRequest(Request *request)\n> > * requests which have to be prepared to make sure they are ready for being\n> > * queued to the pipeline handler.\n> > *\n> > - * The queue of waiting requests is iterated and all prepared requests are\n> > - * passed to the pipeline handler in the same order they have been queued by\n> > - * calling this function.\n> > + * The queue of waiting requests is iterated and up to \\a\n> > + * maxQueuedRequestsDevice_ prepared requests are passed to the pipeline handler\n> > + * in the same order they have been queued by calling this function.\n> > *\n> > * If a Request fails during the preparation phase or if the pipeline handler\n> > * fails in queuing the request to the hardware the request is cancelled.\n> > @@ -487,6 +491,9 @@ void PipelineHandler::doQueueRequests(Camera *camera)\n> > {\n> > Camera::Private *data = camera->_d();\n> > while (!data->waitingRequests_.empty()) {\n> > + if (data->queuedRequests_.size() == maxQueuedRequestsDevice_)\n> > + break;\n> > +\n> \n> Thought exercise probably, does this make sense?\n> \n> + if (data->queuedRequests_.size() == data->maxQueuedRequestsDevice_)\n> + break;\n> \n> i.e. maxQueuedRequestsDevice_ is per-camera pipeline, and not per\n> pipeline-handler. I do not have concrete use cases, but if forced to\n> think:\n> \n> camera0: sensor <> (de)/(se)rializer <> mipi-csi0 <> isp0 and\n> camera1: sensor <> mipi-csi1 <> isp1\n\nThis is a valid objection and when we strive for the smallest possible\nrequest count it might be camera specific. Currently\nmaxQueuedRequestsDevice has a bit of a security margin above the\ntheoretical minimum and I think that is fine. I expect the pipeline\nhandlers to further improve in the context of PFC. Then this limit might\nnot be needed anymore at all. So I'd stick with the simpler solution for\nnow and see where need arises on the longer run.\n\nBest regards,\nStefan\n\n> \n> on i.MX8MPi/pi5, and serializers components have other (lesser) queue\n> limits?\n> \n> > Request *request = data->waitingRequests_.front();\n> > if (!request->_d()->prepared_)\n> > break;\n> > @@ -568,6 +575,9 @@ void PipelineHandler::completeRequest(Request *request)\n> > data->queuedRequests_.pop_front();\n> > camera->requestComplete(req);\n> > }\n> > +\n> > + /* Allow any waiting requests to be queued to the pipeline. */\n> > + doQueueRequests(camera);\n> > }\n> > \n> > /**\n> > -- \n> > 2.48.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 74F95C0DA4\n\tfor ;\n\tMon, 7 Jul 2025 07:33:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A651468E7E;\n\tMon, 7 Jul 2025 09:33:44 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0606168E66\n\tfor ;\n\tMon, 7 Jul 2025 09:33:42 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:4349:87b4:b32:c477])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 38EFE9CA;\n\tMon, 7 Jul 2025 09:33:16 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"OPADM3ki\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1751873596;\n\tbh=pvaIqd+Rl0IsD8vYjcKM1RXaXooISfN3hKV8IcX3I14=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=OPADM3kitxBIfjZ+GAc5dfcqb84LiR1DrsNvl1UOlmhPsJR1my3UKYXmlRzoP8J9K\n\t8WkT9ZhOj3r23QfXw0zugVJG7feBFtcxsq8iMV+wTymHE1wri0LS7DQ+mK6eivw5XY\n\tbdhrHpSRkYmkuzdGb+BTCU/aN9g8ABhy/AA2826w=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20250630081126.2384387-1-stefan.klug@ideasonboard.com>\n\t<20250630081126.2384387-3-stefan.klug@ideasonboard.com>\n\t","Subject":"Re: [PATCH v1 2/6] libcamera: pipeline_handler: Allow to limit the\n\tnumber of queued requests","From":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","To":"Umang Jain ","Date":"Mon, 07 Jul 2025 09:33:39 +0200","Message-ID":"<175187361962.2426344.18413863794511170293@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]