[{"id":34121,"web_url":"https://patchwork.libcamera.org/comment/34121/","msgid":"","date":"2025-05-02T16:28:30","subject":"Re: [PATCH v11 01/19] libcamera: property: Add MinimumRequests\n\tproperty","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Sevn,\n thanks for resurecting this!\n\nCan I ask if this solves any actual problem and on which platform ?\n\nOn Mon, Apr 28, 2025 at 11:02:26AM +0200, Sven Püschel wrote:\n> From: Nícolas F. R. A. Prado \n>\n> The MinimumRequests property reports the minimum number of capture\n> requests that the camera pipeline requires to have queued in order to\n> sustain capture operations without frame drops. At this quantity,\n> there's no guarantee that manual per-frame controls will apply\n> correctly. The quantity needed for that may be added as a separate\n> property in the future.\n>\n> The mali-c55 driver defines min_queued_buffers = 1 [1]. Therefore set\n> set the minimum requests to 2 to account one request in the userspace.\n>\n> The dcmipp and j721e drivers both defines min_queued_buffers = 1\n> in the kernel under\n> drivers/media/platform/st/stm32/stm32-dcmipp/dcmipp-bytecap.c\n> and drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c .\n> Therefore use these values, as they are added 1 more.\n>\n> For the intel-ipu6, mtk-seninf simple devices and the virtual pipeline\n> use a conservative value of 3 minimumBuffers, as no further requirements\n> are known about them.\n>\n> [1] https://lore.kernel.org/linux-media/20240131174355.GB20792@pendragon.ideasonboard.com/T/#t\n>\n> Signed-off-by: Nícolas F. R. A. Prado \n> Signed-off-by: Paul Elder \n> Acked-by: Umang Jain \n> Signed-off-by: Sven Püschel \n>\n> ---\n> Changes in v11\n> - Add mali-c55, dcmipp, virtual, j721e, intel-ipu6 and mtk-seninf\n> - Hold only the minimum buffers instead of the whole deviceInfo pointer in SimpleCameraData\n> - Adjusted min_buffers_needed property to min_reqbufs_allocation in docs\n> - Relax property description from no frame drops -> only minimal frame\n> drops, based on the comment from Naush [10]\n> - Changed property type from int32_t -> uint32_t to match all of the\n> indirect casts to an unsigned value throughout the existing patchset.\n> - Removed Reviewed-by: Laurent Pinchart \n> - Removed Reviewed-by: Paul Elder \n>\n> [10] https://lists.libcamera.org/pipermail/libcamera-devel/2022-December/035872.html\n>\n> Changes in v10:\n> - ipu3: add a constant to populate MinimumRequests, as we'll also use it\n> elsewhere\n> - update pipeline handler guide to set vivid' MinimumRequests to 2\n> - expand the MinimumRequests property description to include a line\n> about startup\n> - add isi\n>\n> Changes in v9:\n> - rebased\n>\n> Changes in v8:\n> - Changed the MinimumRequests property meaning to require that frames aren't\n> dropped\n> - Set MinimumRequests on SimplePipeline depending on device and usage of\n> converter\n> - Undid definition of default MinimumRequests on CameraSensor constructor\n> - Updated pipeline-handler guides with new MinimumRequests property\n>\n> Changes in v7:\n> - Renamed property from MinNumRequests to MinimumRequests\n> - Changed MinimumRequests property's description\n>\n> Changes in v6:\n> - Removed comment from Raspberrypi MinNumRequests setting\n> - Removed an extra blank line\n> ---\n> Documentation/guides/pipeline-handler.rst | 20 ++++++---\n> src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 2 +\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++\n> src/libcamera/pipeline/mali-c55/mali-c55.cpp | 1 +\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 1 +\n> .../pipeline/rpi/common/pipeline_base.cpp | 2 +\n> src/libcamera/pipeline/simple/simple.cpp | 41 +++++++++++++++----\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +\n> src/libcamera/pipeline/vimc/vimc.cpp | 2 +\n> src/libcamera/pipeline/virtual/virtual.cpp | 1 +\n> src/libcamera/property_ids_core.yaml | 22 ++++++++++\n> 11 files changed, 85 insertions(+), 13 deletions(-)\n>\n\n[snip]\n\n> index 834454a4..cc28d677 100644\n> --- a/src/libcamera/property_ids_core.yaml\n> +++ b/src/libcamera/property_ids_core.yaml\n> @@ -701,4 +701,26 @@ controls:\n>\n> Different cameras may report identical devices.\n>\n> + - MinimumRequests:\n\nI'm still not at ease with the definition of this property, I'm\nafraid.\n\nIt seems to be mixing two different things (maybe 3 :)\n\n> + type: uint32_t\n> + description: |\n> + The minimum number of capture requests that the camera pipeline requires\n> + to have queued in order to sustain capture operations with only minimal\n\n\nThe reference to \"minimal frame drops\" is a bit too loosely\nspecified ?\n\n> + frame drops. At this quantity, there's no guarantee that manual per\n> + frame controls will apply correctly. This is also the minimum number of\n\nThe per-frame control pipeline depth does depend on other factors\nsuch as the sensor delays, specifically for manual AE/AWB control\n\n\n> + requests that must be queued before capture starts.\n\nThe number of buffers required to start streaming comes directly from\nthe min_queued_buffers variable defined by drivers (unfortunatetly not\navailable to users through any uAPI). One thing I would like to see\nhappening is drivers stop setting min_queued_buffers at all, or if not\npossible maybe expose it through an api (a RO v4l2 control ?).\n\nDrivers should allocate internal buffers and use them if the\napplication doesn't keep, and handle both the runtime underruns and\nstart streaming conditions using scratch buffers.\n\nEven if this won't happen in drivers, the requirement should not be\nexposed to libcamera users. In the \"great scheme of things\" the idea\nis to decouple application's Requests from the frames pipeline, and\nproviding enough buffers to sustain the frame rate should be a\npipeline/IPA job.\n\nTrue, for platforms where drivers already require min_queued_buffers\napplications right now have to queue enough requests before\nstart receiving frames back, and we goofly report it through\nStreamConfiguration::bufferCount.\n\nUnless there's an actual use case or any angle I'm grossly\nmissing I don't think this property is a good idea..\n\n> +\n> + This property is based on the minimum number of memory buffers\n> + needed to fill the capture pipeline composed of hardware processing\n> + blocks plus as many buffers as needed to take into account propagation\n> + delays and avoid dropping frames.\n> +\n> + \\todo Should this be a per-stream property?\n> +\n> + \\todo Extend this property to expose the different minimum buffer and\n> + request counts (the minimum number of buffers to be able to capture at\n> + all, the minimum number of buffers to sustain capture without frame\n> + drop, and the minimum number of requests to ensure proper operation of\n> + per-frame controls).\n> +\n> ...\n> --\n> 2.49.0\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 F2D31C327D\n\tfor ;\n\tFri, 2 May 2025 16:28:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E15FE68AD8;\n\tFri, 2 May 2025 18:28:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AF00368AD3\n\tfor ;\n\tFri, 2 May 2025 18:28:34 +0200 (CEST)","from ideasonboard.com (mob-5-90-54-226.net.vodafone.it\n\t[5.90.54.226])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9D3A9AF;\n\tFri, 2 May 2025 18:28: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=\"udady8fE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1746203306;\n\tbh=V4xWuF+YvmR9z1QmAeQoAkYCzglN02oOeKqosfkMwLY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=udady8fEAcedHiU+1lD3+baFAtUQFd3gBemkNBndn45yUerx29aE/pU7XcQ+MtknI\n\t31T5f/OkLj96vs5hch1MalpUEs3kPOa/lUYZUQ6SW8LbEgG5H8vnRbbEGAShlCDqNK\n\tt7t5+bSKdRIM0UDUKgN2aEGOH1PTJJ4edrzKyK/I=","Date":"Fri, 2 May 2025 18:28:30 +0200","From":"Jacopo Mondi ","To":"Sven =?utf-8?q?P=C3=BCschel?= ","Cc":"libcamera-devel@lists.libcamera.org, =?utf-8?b?TsOtY29sYXMgRi4gUi4g?=\n\t=?utf-8?q?A=2E?= Prado ,\n\tPaul Elder , Umang Jain\n\t","Subject":"Re: [PATCH v11 01/19] libcamera: property: Add MinimumRequests\n\tproperty","Message-ID":"","References":"<20250428090413.38234-1-s.pueschel@pengutronix.de>\n\t<20250428090413.38234-2-s.pueschel@pengutronix.de>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20250428090413.38234-2-s.pueschel@pengutronix.de>","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":34129,"web_url":"https://patchwork.libcamera.org/comment/34129/","msgid":"<1a211f82-704f-445a-a057-369501b467b3@pengutronix.de>","date":"2025-05-05T12:23:13","subject":"Re: [PATCH v11 01/19] libcamera: property: Add MinimumRequests\n\tproperty","submitter":{"id":225,"url":"https://patchwork.libcamera.org/api/people/225/","name":"Sven Püschel","email":"s.pueschel@pengutronix.de"},"content":"Hi Jacopo,\n\nOn 5/2/25 18:28, Jacopo Mondi wrote:\n> Hi Sevn,\n> thanks for resurecting this!\n>\n> Can I ask if this solves any actual problem and on which platform ?\n\nThe problem I'm facing is that in a more complex setup with the rkisp1 \nwe need to keep multiple dmabufs in the userspace. But the hardcoded \nvalue of 4 buffers in the rkisp1 driver seems to expect that the \napplication only hold around one buffer and have the other 3 buffers \nqueued for a smooth frame capture. But due to holding more dmabufs, I'm \nrunning into massive frame drops due to usually only having one request \nqueued and waiting for it to finish.\n\nTherefore I want to allocate more dmabufs for this problematic use case \nand avoid patching libcamera to increase the hardcoded buffer count of \nrkisp1 (which would also apply globally).\n\nKieran then pointed me towards this patchset to allow the application to \ncontrol the amount of dmabufs allocated.\n\n\n> On Mon, Apr 28, 2025 at 11:02:26AM +0200, Sven Püschel wrote:\n>> From: Nícolas F. R. A. Prado \n>>\n>> The MinimumRequests property reports the minimum number of capture\n>> requests that the camera pipeline requires to have queued in order to\n>> sustain capture operations without frame drops. At this quantity,\n>> there's no guarantee that manual per-frame controls will apply\n>> correctly. The quantity needed for that may be added as a separate\n>> property in the future.\n>>\n>> The mali-c55 driver defines min_queued_buffers = 1 [1]. Therefore set\n>> set the minimum requests to 2 to account one request in the userspace.\n>>\n>> The dcmipp and j721e drivers both defines min_queued_buffers = 1\n>> in the kernel under\n>> drivers/media/platform/st/stm32/stm32-dcmipp/dcmipp-bytecap.c\n>> and drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c .\n>> Therefore use these values, as they are added 1 more.\n>>\n>> For the intel-ipu6, mtk-seninf simple devices and the virtual pipeline\n>> use a conservative value of 3 minimumBuffers, as no further requirements\n>> are known about them.\n>>\n>> [1] https://lore.kernel.org/linux-media/20240131174355.GB20792@pendragon.ideasonboard.com/T/#t\n>>\n>> Signed-off-by: Nícolas F. R. A. Prado \n>> Signed-off-by: Paul Elder \n>> Acked-by: Umang Jain \n>> Signed-off-by: Sven Püschel \n>>\n>> ---\n>> Changes in v11\n>> - Add mali-c55, dcmipp, virtual, j721e, intel-ipu6 and mtk-seninf\n>> - Hold only the minimum buffers instead of the whole deviceInfo pointer in SimpleCameraData\n>> - Adjusted min_buffers_needed property to min_reqbufs_allocation in docs\n>> - Relax property description from no frame drops -> only minimal frame\n>> drops, based on the comment from Naush [10]\n>> - Changed property type from int32_t -> uint32_t to match all of the\n>> indirect casts to an unsigned value throughout the existing patchset.\n>> - Removed Reviewed-by: Laurent Pinchart \n>> - Removed Reviewed-by: Paul Elder \n>>\n>> [10] https://lists.libcamera.org/pipermail/libcamera-devel/2022-December/035872.html\n>>\n>> Changes in v10:\n>> - ipu3: add a constant to populate MinimumRequests, as we'll also use it\n>> elsewhere\n>> - update pipeline handler guide to set vivid' MinimumRequests to 2\n>> - expand the MinimumRequests property description to include a line\n>> about startup\n>> - add isi\n>>\n>> Changes in v9:\n>> - rebased\n>>\n>> Changes in v8:\n>> - Changed the MinimumRequests property meaning to require that frames aren't\n>> dropped\n>> - Set MinimumRequests on SimplePipeline depending on device and usage of\n>> converter\n>> - Undid definition of default MinimumRequests on CameraSensor constructor\n>> - Updated pipeline-handler guides with new MinimumRequests property\n>>\n>> Changes in v7:\n>> - Renamed property from MinNumRequests to MinimumRequests\n>> - Changed MinimumRequests property's description\n>>\n>> Changes in v6:\n>> - Removed comment from Raspberrypi MinNumRequests setting\n>> - Removed an extra blank line\n>> ---\n>> Documentation/guides/pipeline-handler.rst | 20 ++++++---\n>> src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 2 +\n>> src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++\n>> src/libcamera/pipeline/mali-c55/mali-c55.cpp | 1 +\n>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 1 +\n>> .../pipeline/rpi/common/pipeline_base.cpp | 2 +\n>> src/libcamera/pipeline/simple/simple.cpp | 41 +++++++++++++++----\n>> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +\n>> src/libcamera/pipeline/vimc/vimc.cpp | 2 +\n>> src/libcamera/pipeline/virtual/virtual.cpp | 1 +\n>> src/libcamera/property_ids_core.yaml | 22 ++++++++++\n>> 11 files changed, 85 insertions(+), 13 deletions(-)\n>>\n> [snip]\n>\n>> index 834454a4..cc28d677 100644\n>> --- a/src/libcamera/property_ids_core.yaml\n>> +++ b/src/libcamera/property_ids_core.yaml\n>> @@ -701,4 +701,26 @@ controls:\n>>\n>> Different cameras may report identical devices.\n>>\n>> + - MinimumRequests:\n> I'm still not at ease with the definition of this property, I'm\n> afraid.\n>\n> It seems to be mixing two different things (maybe 3 :)\n>> + type: uint32_t\n>> + description: |\n>> + The minimum number of capture requests that the camera pipeline requires\n>> + to have queued in order to sustain capture operations with only minimal\n>\n> The reference to \"minimal frame drops\" is a bit too loosely\n> specified ?\n\nI've changed it due to Naushir's claim that there is no practical number \nto gurantee no frame drops [1]\n\n[1] \nhttps://lists.libcamera.org/pipermail/libcamera-devel/2022-December/035872.html\n\n\n>> + frame drops. At this quantity, there's no guarantee that manual per\n>> + frame controls will apply correctly. This is also the minimum number of\n> The per-frame control pipeline depth does depend on other factors\n> such as the sensor delays, specifically for manual AE/AWB control\n\nand that is the reason it isn't or cannot be part of this property? Or \nwhat's your point?\n\n>> + requests that must be queued before capture starts.\n> The number of buffers required to start streaming comes directly from\n> the min_queued_buffers variable defined by drivers (unfortunatetly not\n> available to users through any uAPI). One thing I would like to see\n> happening is drivers stop setting min_queued_buffers at all, or if not\n> possible maybe expose it through an api (a RO v4l2 control ?).\n>\n> Drivers should allocate internal buffers and use them if the\n> application doesn't keep, and handle both the runtime underruns and\n> start streaming conditions using scratch buffers.\n>\n> Even if this won't happen in drivers, the requirement should not be\n> exposed to libcamera users. In the \"great scheme of things\" the idea\n> is to decouple application's Requests from the frames pipeline, and\n> providing enough buffers to sustain the frame rate should be a\n> pipeline/IPA job.\n>\n> True, for platforms where drivers already require min_queued_buffers\n> applications right now have to queue enough requests before\n> start receiving frames back, and we goofly report it through\n> StreamConfiguration::bufferCount.\n>\n> Unless there's an actual use case or any angle I'm grossly\n> missing I don't think this property is a good idea..\n\nI understand your problem with this property. But I don't know if we can \nget away without providing the application a hint how many dmabufs it \nshould allocate at least.\n\nTo recall my problem, the rkisp1 driver seems to already use scratch \nbuffers when it doesn't have enough buffers queued. So while queuing one \nbuffer at a time worked, it wasn't desired due to the frame drops from \nthe scratch buffers.\n\nBut the application developer cannot really know how many buffers are \nreally required. From my perspective the current operation mode is that \nlibcamera allocates a sensible fixed amount of dmabufs and application \ndevelopers just queue all unused buffers to libcamera [2]. So when the \nallocation is now put into the hands of the application developer, it \nmay just always allocate 2 buffers (to have still one queued when \nhandling a completed buffer) and therefore only queue a maximum of 2 \nbuffers. The driver fills the lack of buffers with scratch buffers, \nwhich cause unintended frame drops.\n\nOne potential solution may be to pass the desired amount of buffers held \nin the application to the allocator. Then the pipeline could allocate a \nsensible amount of buffers (e.g. 1 requested for the application + 3 \nqueued => 4 buffers) and the application would queue all unused buffers \nas usual.\n\nOther than that I don't see how libcamera could abstract this property \naway, as applications control which buffers are used for a given request.\n\n\n[2] \nhttps://www.libcamera.org/guides/application-developer.html#request-queueing \n\n\n\n>> + This property is based on the minimum number of memory buffers\n>> + needed to fill the capture pipeline composed of hardware processing\n>> + blocks plus as many buffers as needed to take into account propagation\n>> + delays and avoid dropping frames.\n>> +\n>> + \\todo Should this be a per-stream property?\n>> +\n>> + \\todo Extend this property to expose the different minimum buffer and\n>> + request counts (the minimum number of buffers to be able to capture at\n>> + all, the minimum number of buffers to sustain capture without frame\n>> + drop, and the minimum number of requests to ensure proper operation of\n>> + per-frame controls).\n>> +\n>> ...\n>> --\n>> 2.49.0\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 A0C82C3200\n\tfor ;\n\tMon, 5 May 2025 12:23:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B2CE168B2C;\n\tMon, 5 May 2025 14:23:16 +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 A383568ADE\n\tfor ;\n\tMon, 5 May 2025 14:23:14 +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 )\n\tid 1uBuqo-0002Oc-6l; Mon, 05 May 2025 14:23:14 +0200"],"Message-ID":"<1a211f82-704f-445a-a057-369501b467b3@pengutronix.de>","Date":"Mon, 5 May 2025 14:23:13 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v11 01/19] libcamera: property: Add MinimumRequests\n\tproperty","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, =?utf-8?b?TsOtY29sYXMgRi4gUi4g?=\n\t=?utf-8?q?A=2E_Prado?= ,\n\tPaul Elder , Umang Jain\n\t","References":"<20250428090413.38234-1-s.pueschel@pengutronix.de>\n\t<20250428090413.38234-2-s.pueschel@pengutronix.de>\n\t","Content-Language":"en-US","From":"=?utf-8?q?Sven_P=C3=BCschel?= ","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":34130,"web_url":"https://patchwork.libcamera.org/comment/34130/","msgid":"","date":"2025-05-05T13:59:33","subject":"Re: [PATCH v11 01/19] libcamera: property: Add MinimumRequests\n\tproperty","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Sven\n\nOn Mon, May 05, 2025 at 02:23:13PM +0200, Sven Püschel wrote:\n> Hi Jacopo,\n>\n> On 5/2/25 18:28, Jacopo Mondi wrote:\n> > Hi Sevn,\n> > thanks for resurecting this!\n> >\n> > Can I ask if this solves any actual problem and on which platform ?\n>\n> The problem I'm facing is that in a more complex setup with the rkisp1 we\n> need to keep multiple dmabufs in the userspace. But the hardcoded value of 4\n> buffers in the rkisp1 driver seems to expect that the application only hold\n> around one buffer and have the other 3 buffers queued for a smooth frame\n> capture. But due to holding more dmabufs, I'm running into massive frame\n> drops due to usually only having one request queued and waiting for it to\n> finish.\n\nThanks for the overview.\n\nCan't more buffers be allocated by setting a larger value in\nStreamConfiguration::bufferCount (which is indeed now hardcoded to 4) ?\n\n>\n> Therefore I want to allocate more dmabufs for this problematic use case and\n> avoid patching libcamera to increase the hardcoded buffer count of rkisp1\n> (which would also apply globally).\n>\n> Kieran then pointed me towards this patchset to allow the application to\n> control the amount of dmabufs allocated.\n>\n>\n> > On Mon, Apr 28, 2025 at 11:02:26AM +0200, Sven Püschel wrote:\n> > > From: Nícolas F. R. A. Prado \n> > >\n> > > The MinimumRequests property reports the minimum number of capture\n> > > requests that the camera pipeline requires to have queued in order to\n> > > sustain capture operations without frame drops. At this quantity,\n> > > there's no guarantee that manual per-frame controls will apply\n> > > correctly. The quantity needed for that may be added as a separate\n> > > property in the future.\n> > >\n> > > The mali-c55 driver defines min_queued_buffers = 1 [1]. Therefore set\n> > > set the minimum requests to 2 to account one request in the userspace.\n> > >\n> > > The dcmipp and j721e drivers both defines min_queued_buffers = 1\n> > > in the kernel under\n> > > drivers/media/platform/st/stm32/stm32-dcmipp/dcmipp-bytecap.c\n> > > and drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c .\n> > > Therefore use these values, as they are added 1 more.\n> > >\n> > > For the intel-ipu6, mtk-seninf simple devices and the virtual pipeline\n> > > use a conservative value of 3 minimumBuffers, as no further requirements\n> > > are known about them.\n> > >\n> > > [1] https://lore.kernel.org/linux-media/20240131174355.GB20792@pendragon.ideasonboard.com/T/#t\n> > >\n> > > Signed-off-by: Nícolas F. R. A. Prado \n> > > Signed-off-by: Paul Elder \n> > > Acked-by: Umang Jain \n> > > Signed-off-by: Sven Püschel \n> > >\n> > > ---\n> > > Changes in v11\n> > > - Add mali-c55, dcmipp, virtual, j721e, intel-ipu6 and mtk-seninf\n> > > - Hold only the minimum buffers instead of the whole deviceInfo pointer in SimpleCameraData\n> > > - Adjusted min_buffers_needed property to min_reqbufs_allocation in docs\n> > > - Relax property description from no frame drops -> only minimal frame\n> > > drops, based on the comment from Naush [10]\n> > > - Changed property type from int32_t -> uint32_t to match all of the\n> > > indirect casts to an unsigned value throughout the existing patchset.\n> > > - Removed Reviewed-by: Laurent Pinchart \n> > > - Removed Reviewed-by: Paul Elder \n> > >\n> > > [10] https://lists.libcamera.org/pipermail/libcamera-devel/2022-December/035872.html\n> > >\n> > > Changes in v10:\n> > > - ipu3: add a constant to populate MinimumRequests, as we'll also use it\n> > > elsewhere\n> > > - update pipeline handler guide to set vivid' MinimumRequests to 2\n> > > - expand the MinimumRequests property description to include a line\n> > > about startup\n> > > - add isi\n> > >\n> > > Changes in v9:\n> > > - rebased\n> > >\n> > > Changes in v8:\n> > > - Changed the MinimumRequests property meaning to require that frames aren't\n> > > dropped\n> > > - Set MinimumRequests on SimplePipeline depending on device and usage of\n> > > converter\n> > > - Undid definition of default MinimumRequests on CameraSensor constructor\n> > > - Updated pipeline-handler guides with new MinimumRequests property\n> > >\n> > > Changes in v7:\n> > > - Renamed property from MinNumRequests to MinimumRequests\n> > > - Changed MinimumRequests property's description\n> > >\n> > > Changes in v6:\n> > > - Removed comment from Raspberrypi MinNumRequests setting\n> > > - Removed an extra blank line\n> > > ---\n> > > Documentation/guides/pipeline-handler.rst | 20 ++++++---\n> > > src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 2 +\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++\n> > > src/libcamera/pipeline/mali-c55/mali-c55.cpp | 1 +\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 1 +\n> > > .../pipeline/rpi/common/pipeline_base.cpp | 2 +\n> > > src/libcamera/pipeline/simple/simple.cpp | 41 +++++++++++++++----\n> > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +\n> > > src/libcamera/pipeline/vimc/vimc.cpp | 2 +\n> > > src/libcamera/pipeline/virtual/virtual.cpp | 1 +\n> > > src/libcamera/property_ids_core.yaml | 22 ++++++++++\n> > > 11 files changed, 85 insertions(+), 13 deletions(-)\n> > >\n> > [snip]\n> >\n> > > index 834454a4..cc28d677 100644\n> > > --- a/src/libcamera/property_ids_core.yaml\n> > > +++ b/src/libcamera/property_ids_core.yaml\n> > > @@ -701,4 +701,26 @@ controls:\n> > >\n> > > Different cameras may report identical devices.\n> > >\n> > > + - MinimumRequests:\n> > I'm still not at ease with the definition of this property, I'm\n> > afraid.\n> >\n> > It seems to be mixing two different things (maybe 3 :)\n> > > + type: uint32_t\n> > > + description: |\n> > > + The minimum number of capture requests that the camera pipeline requires\n> > > + to have queued in order to sustain capture operations with only minimal\n> >\n> > The reference to \"minimal frame drops\" is a bit too loosely\n> > specified ?\n>\n> I've changed it due to Naushir's claim that there is no practical number to\n> gurantee no frame drops [1]\n>\n> [1] https://lists.libcamera.org/pipermail/libcamera-devel/2022-December/035872.html\n>\n>\n> > > + frame drops. At this quantity, there's no guarantee that manual per\n> > > + frame controls will apply correctly. This is also the minimum number of\n> > The per-frame control pipeline depth does depend on other factors\n> > such as the sensor delays, specifically for manual AE/AWB control\n>\n> and that is the reason it isn't or cannot be part of this property? Or\n> what's your point?\n>\n\nMy point is that the pipeline depth (which would need a more formal\ndefinition as we use it quite freely without having ever set in stone\nits definition) is a property unrelated to the minium number of\nbuffers or requests that has to be queued.\n\nI would leave pipeline depth and per-frame control out of the picture.\n\n> > > + requests that must be queued before capture starts.\n> > The number of buffers required to start streaming comes directly from\n> > the min_queued_buffers variable defined by drivers (unfortunatetly not\n> > available to users through any uAPI). One thing I would like to see\n> > happening is drivers stop setting min_queued_buffers at all, or if not\n> > possible maybe expose it through an api (a RO v4l2 control ?).\n> >\n> > Drivers should allocate internal buffers and use them if the\n> > application doesn't keep, and handle both the runtime underruns and\n> > start streaming conditions using scratch buffers.\n> >\n> > Even if this won't happen in drivers, the requirement should not be\n> > exposed to libcamera users. In the \"great scheme of things\" the idea\n> > is to decouple application's Requests from the frames pipeline, and\n> > providing enough buffers to sustain the frame rate should be a\n> > pipeline/IPA job.\n> >\n> > True, for platforms where drivers already require min_queued_buffers\n> > applications right now have to queue enough requests before\n> > start receiving frames back, and we goofly report it through\n> > StreamConfiguration::bufferCount.\n> >\n> > Unless there's an actual use case or any angle I'm grossly\n> > missing I don't think this property is a good idea..\n>\n> I understand your problem with this property. But I don't know if we can get\n> away without providing the application a hint how many dmabufs it should\n> allocate at least.\n>\n\nWell, the default value set by generateConfiguration() should be an\nindication of what value the pipeline expects.\n\n> To recall my problem, the rkisp1 driver seems to already use scratch buffers\n\nYes, I would love all drivers to remove min_queued_buffers as it was\ndone for rkisp1\nhttps://www.spinics.net/lists/linux-media/msg264330.html\n\n> when it doesn't have enough buffers queued. So while queuing one buffer at a\n> time worked, it wasn't desired due to the frame drops from the scratch\n> buffers.\n\nKeeping up with the frame rate shouldn't be a driver concern but an\napplication's one. But of course if you can't allocate enough buffers\nto queue them back fast enough, you can't do that.\n\n>\n> But the application developer cannot really know how many buffers are really\n> required. From my perspective the current operation mode is that libcamera\n\nThe fact is that \"really required\" is an ill-formed concepts. As said\nthere are many different aspects at play: keeping up with the sensor's\nframe rate, the pipeline depth required to guarantee per-frame control\nand the number of buffers required to start streaming.\n\nWe currently report some min number of buffers (most of the times, but\nnot always coming from min_queued_buffers in the driver) in\nStreamConfiguration::bufferCount, but this is certainly is a\nsub-optimal API.\n\n> allocates a sensible fixed amount of dmabufs and application developers just\n> queue all unused buffers to libcamera [2]. So when the allocation is now put\n> into the hands of the application developer, it may just always allocate 2\n> buffers (to have still one queued when handling a completed buffer) and\n> therefore only queue a maximum of 2 buffers. The driver fills the lack of\n> buffers with scratch buffers, which cause unintended frame drops.\n\nThat's what should happen in the driver. The alternative would be not\nbeing able to dequeue buffers at all. Simply, if an application\nallocates two buffers only, it's calling for troubles, isn't it ?\n\n>\n> One potential solution may be to pass the desired amount of buffers held in\n> the application to the allocator. Then the pipeline could allocate a\n> sensible amount of buffers (e.g. 1 requested for the application + 3 queued\n> => 4 buffers) and the application would queue all unused buffers as usual.\n>\n\nI wonder why RkISP1Path::validate() reset bufferCount to 4.\nWe should allow apps to allocate more buffers if they want to.\n\nKieran, Stefan, ideas ?\n\n> Other than that I don't see how libcamera could abstract this property away,\n> as applications control which buffers are used for a given request.\n>\n\nWe certainly need a way to allow apps to allocate more than 4 buffers.\nMy main problem is with this property, which mixes in too many things\nand is ill-defined.\n\n>\n> [2]\n> https://www.libcamera.org/guides/application-developer.html#request-queueing\n>\n>\n>\n> > > + This property is based on the minimum number of memory buffers\n> > > + needed to fill the capture pipeline composed of hardware processing\n> > > + blocks plus as many buffers as needed to take into account propagation\n> > > + delays and avoid dropping frames.\n> > > +\n> > > + \\todo Should this be a per-stream property?\n> > > +\n> > > + \\todo Extend this property to expose the different minimum buffer and\n> > > + request counts (the minimum number of buffers to be able to capture at\n> > > + all, the minimum number of buffers to sustain capture without frame\n> > > + drop, and the minimum number of requests to ensure proper operation of\n> > > + per-frame controls).\n> > > +\n> > > ...\n> > > --\n> > > 2.49.0\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 0CAC7C3226\n\tfor ;\n\tMon, 5 May 2025 13:59:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F235768B25;\n\tMon, 5 May 2025 15:59:38 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 228DA68ADE\n\tfor ;\n\tMon, 5 May 2025 15:59:37 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DAB18289;\n\tMon, 5 May 2025 15:59: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=\"ZcWJHfFn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1746453567;\n\tbh=GFr7/SdbYNfSIkYNykgC6WmbC6fIUsPb76G/vNJoFqE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZcWJHfFn5aAvBRY8JNqqlp3h2Ai5CRQgkxxofgbIG4XQ7/jYd7oApQ8j38tI6TJdJ\n\twYHsLkdzob5EFxxT/4Uc/y+FSlFxYnyKwqoMHqpBuxI9gEe/vlRhvPQAgTcKvslTa5\n\tmaRtKeS5vP28JCMl12BpSBRUYribGAyFKaJaRjwk=","Date":"Mon, 5 May 2025 15:59:33 +0200","From":"Jacopo Mondi ","To":"Sven =?utf-8?q?P=C3=BCschel?= ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org, =?utf-8?b?TsOtY29sYXMgRi4gUi4g?=\n\t=?utf-8?q?A=2E?= Prado ,\n\tPaul Elder , Umang Jain\n\t","Subject":"Re: [PATCH v11 01/19] libcamera: property: Add MinimumRequests\n\tproperty","Message-ID":"","References":"<20250428090413.38234-1-s.pueschel@pengutronix.de>\n\t<20250428090413.38234-2-s.pueschel@pengutronix.de>\n\t\n\t<1a211f82-704f-445a-a057-369501b467b3@pengutronix.de>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<1a211f82-704f-445a-a057-369501b467b3@pengutronix.de>","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":34132,"web_url":"https://patchwork.libcamera.org/comment/34132/","msgid":"","date":"2025-05-05T14:40:53","subject":"Re: [PATCH v11 01/19] libcamera: property: Add MinimumRequests\n\tproperty","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, May 05, 2025 at 03:59:33PM +0200, Jacopo Mondi wrote:\n> Hi Sven\n> \n> On Mon, May 05, 2025 at 02:23:13PM +0200, Sven Püschel wrote:\n> > Hi Jacopo,\n> >\n> > On 5/2/25 18:28, Jacopo Mondi wrote:\n> > > Hi Sevn,\n> > > thanks for resurecting this!\n> > >\n> > > Can I ask if this solves any actual problem and on which platform ?\n> >\n> > The problem I'm facing is that in a more complex setup with the rkisp1 we\n> > need to keep multiple dmabufs in the userspace. But the hardcoded value of 4\n> > buffers in the rkisp1 driver seems to expect that the application only hold\n> > around one buffer and have the other 3 buffers queued for a smooth frame\n> > capture. But due to holding more dmabufs, I'm running into massive frame\n> > drops due to usually only having one request queued and waiting for it to\n> > finish.\n> \n> Thanks for the overview.\n> \n> Can't more buffers be allocated by setting a larger value in\n> StreamConfiguration::bufferCount (which is indeed now hardcoded to 4) ?\n> \n> >\n> > Therefore I want to allocate more dmabufs for this problematic use case and\n> > avoid patching libcamera to increase the hardcoded buffer count of rkisp1\n> > (which would also apply globally).\n> >\n> > Kieran then pointed me towards this patchset to allow the application to\n> > control the amount of dmabufs allocated.\n> >\n> >\n> > > On Mon, Apr 28, 2025 at 11:02:26AM +0200, Sven Püschel wrote:\n> > > > From: Nícolas F. R. A. Prado \n> > > >\n> > > > The MinimumRequests property reports the minimum number of capture\n> > > > requests that the camera pipeline requires to have queued in order to\n> > > > sustain capture operations without frame drops. At this quantity,\n> > > > there's no guarantee that manual per-frame controls will apply\n> > > > correctly. The quantity needed for that may be added as a separate\n> > > > property in the future.\n> > > >\n> > > > The mali-c55 driver defines min_queued_buffers = 1 [1]. Therefore set\n> > > > set the minimum requests to 2 to account one request in the userspace.\n> > > >\n> > > > The dcmipp and j721e drivers both defines min_queued_buffers = 1\n> > > > in the kernel under\n> > > > drivers/media/platform/st/stm32/stm32-dcmipp/dcmipp-bytecap.c\n> > > > and drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c .\n> > > > Therefore use these values, as they are added 1 more.\n> > > >\n> > > > For the intel-ipu6, mtk-seninf simple devices and the virtual pipeline\n> > > > use a conservative value of 3 minimumBuffers, as no further requirements\n> > > > are known about them.\n> > > >\n> > > > [1] https://lore.kernel.org/linux-media/20240131174355.GB20792@pendragon.ideasonboard.com/T/#t\n> > > >\n> > > > Signed-off-by: Nícolas F. R. A. Prado \n> > > > Signed-off-by: Paul Elder \n> > > > Acked-by: Umang Jain \n> > > > Signed-off-by: Sven Püschel \n> > > >\n> > > > ---\n> > > > Changes in v11\n> > > > - Add mali-c55, dcmipp, virtual, j721e, intel-ipu6 and mtk-seninf\n> > > > - Hold only the minimum buffers instead of the whole deviceInfo pointer in SimpleCameraData\n> > > > - Adjusted min_buffers_needed property to min_reqbufs_allocation in docs\n> > > > - Relax property description from no frame drops -> only minimal frame\n> > > > drops, based on the comment from Naush [10]\n> > > > - Changed property type from int32_t -> uint32_t to match all of the\n> > > > indirect casts to an unsigned value throughout the existing patchset.\n> > > > - Removed Reviewed-by: Laurent Pinchart \n> > > > - Removed Reviewed-by: Paul Elder \n> > > >\n> > > > [10] https://lists.libcamera.org/pipermail/libcamera-devel/2022-December/035872.html\n> > > >\n> > > > Changes in v10:\n> > > > - ipu3: add a constant to populate MinimumRequests, as we'll also use it\n> > > > elsewhere\n> > > > - update pipeline handler guide to set vivid' MinimumRequests to 2\n> > > > - expand the MinimumRequests property description to include a line\n> > > > about startup\n> > > > - add isi\n> > > >\n> > > > Changes in v9:\n> > > > - rebased\n> > > >\n> > > > Changes in v8:\n> > > > - Changed the MinimumRequests property meaning to require that frames aren't\n> > > > dropped\n> > > > - Set MinimumRequests on SimplePipeline depending on device and usage of\n> > > > converter\n> > > > - Undid definition of default MinimumRequests on CameraSensor constructor\n> > > > - Updated pipeline-handler guides with new MinimumRequests property\n> > > >\n> > > > Changes in v7:\n> > > > - Renamed property from MinNumRequests to MinimumRequests\n> > > > - Changed MinimumRequests property's description\n> > > >\n> > > > Changes in v6:\n> > > > - Removed comment from Raspberrypi MinNumRequests setting\n> > > > - Removed an extra blank line\n> > > > ---\n> > > > Documentation/guides/pipeline-handler.rst | 20 ++++++---\n> > > > src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 2 +\n> > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++\n> > > > src/libcamera/pipeline/mali-c55/mali-c55.cpp | 1 +\n> > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 1 +\n> > > > .../pipeline/rpi/common/pipeline_base.cpp | 2 +\n> > > > src/libcamera/pipeline/simple/simple.cpp | 41 +++++++++++++++----\n> > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +\n> > > > src/libcamera/pipeline/vimc/vimc.cpp | 2 +\n> > > > src/libcamera/pipeline/virtual/virtual.cpp | 1 +\n> > > > src/libcamera/property_ids_core.yaml | 22 ++++++++++\n> > > > 11 files changed, 85 insertions(+), 13 deletions(-)\n> > > >\n> > > [snip]\n> > >\n> > > > index 834454a4..cc28d677 100644\n> > > > --- a/src/libcamera/property_ids_core.yaml\n> > > > +++ b/src/libcamera/property_ids_core.yaml\n> > > > @@ -701,4 +701,26 @@ controls:\n> > > >\n> > > > Different cameras may report identical devices.\n> > > >\n> > > > + - MinimumRequests:\n> > > I'm still not at ease with the definition of this property, I'm\n> > > afraid.\n> > >\n> > > It seems to be mixing two different things (maybe 3 :)\n> > > > + type: uint32_t\n> > > > + description: |\n> > > > + The minimum number of capture requests that the camera pipeline requires\n> > > > + to have queued in order to sustain capture operations with only minimal\n> > >\n> > > The reference to \"minimal frame drops\" is a bit too loosely\n> > > specified ?\n> >\n> > I've changed it due to Naushir's claim that there is no practical number to\n> > gurantee no frame drops [1]\n> >\n> > [1] https://lists.libcamera.org/pipermail/libcamera-devel/2022-December/035872.html\n> >\n> >\n> > > > + frame drops. At this quantity, there's no guarantee that manual per\n> > > > + frame controls will apply correctly. This is also the minimum number of\n> > > The per-frame control pipeline depth does depend on other factors\n> > > such as the sensor delays, specifically for manual AE/AWB control\n> >\n> > and that is the reason it isn't or cannot be part of this property? Or\n> > what's your point?\n> >\n> \n> My point is that the pipeline depth (which would need a more formal\n> definition as we use it quite freely without having ever set in stone\n> its definition) is a property unrelated to the minium number of\n> buffers or requests that has to be queued.\n> \n> I would leave pipeline depth and per-frame control out of the picture.\n> \n> > > > + requests that must be queued before capture starts.\n> > > The number of buffers required to start streaming comes directly from\n> > > the min_queued_buffers variable defined by drivers (unfortunatetly not\n> > > available to users through any uAPI). One thing I would like to see\n> > > happening is drivers stop setting min_queued_buffers at all, or if not\n> > > possible maybe expose it through an api (a RO v4l2 control ?).\n> > >\n> > > Drivers should allocate internal buffers and use them if the\n> > > application doesn't keep, and handle both the runtime underruns and\n> > > start streaming conditions using scratch buffers.\n> > >\n> > > Even if this won't happen in drivers, the requirement should not be\n> > > exposed to libcamera users. In the \"great scheme of things\" the idea\n> > > is to decouple application's Requests from the frames pipeline, and\n> > > providing enough buffers to sustain the frame rate should be a\n> > > pipeline/IPA job.\n> > >\n> > > True, for platforms where drivers already require min_queued_buffers\n> > > applications right now have to queue enough requests before\n> > > start receiving frames back, and we goofly report it through\n> > > StreamConfiguration::bufferCount.\n> > >\n> > > Unless there's an actual use case or any angle I'm grossly\n> > > missing I don't think this property is a good idea..\n> >\n> > I understand your problem with this property. But I don't know if we can get\n> > away without providing the application a hint how many dmabufs it should\n> > allocate at least.\n> >\n> \n> Well, the default value set by generateConfiguration() should be an\n> indication of what value the pipeline expects.\n> \n> > To recall my problem, the rkisp1 driver seems to already use scratch buffers\n> \n> Yes, I would love all drivers to remove min_queued_buffers as it was\n> done for rkisp1\n> https://www.spinics.net/lists/linux-media/msg264330.html\n> \n> > when it doesn't have enough buffers queued. So while queuing one buffer at a\n> > time worked, it wasn't desired due to the frame drops from the scratch\n> > buffers.\n> \n> Keeping up with the frame rate shouldn't be a driver concern but an\n> application's one. But of course if you can't allocate enough buffers\n> to queue them back fast enough, you can't do that.\n> \n> >\n> > But the application developer cannot really know how many buffers are really\n> > required. From my perspective the current operation mode is that libcamera\n> \n> The fact is that \"really required\" is an ill-formed concepts. As said\n> there are many different aspects at play: keeping up with the sensor's\n> frame rate, the pipeline depth required to guarantee per-frame control\n> and the number of buffers required to start streaming.\n> \n> We currently report some min number of buffers (most of the times, but\n> not always coming from min_queued_buffers in the driver) in\n> StreamConfiguration::bufferCount, but this is certainly is a\n> sub-optimal API.\n> \n> > allocates a sensible fixed amount of dmabufs and application developers just\n> > queue all unused buffers to libcamera [2]. So when the allocation is now put\n> > into the hands of the application developer, it may just always allocate 2\n> > buffers (to have still one queued when handling a completed buffer) and\n> > therefore only queue a maximum of 2 buffers. The driver fills the lack of\n> > buffers with scratch buffers, which cause unintended frame drops.\n> \n> That's what should happen in the driver. The alternative would be not\n> being able to dequeue buffers at all. Simply, if an application\n> allocates two buffers only, it's calling for troubles, isn't it ?\n> \n> >\n> > One potential solution may be to pass the desired amount of buffers held in\n> > the application to the allocator. Then the pipeline could allocate a\n> > sensible amount of buffers (e.g. 1 requested for the application + 3 queued\n> > => 4 buffers) and the application would queue all unused buffers as usual.\n> >\n> \n> I wonder why RkISP1Path::validate() reset bufferCount to 4.\n> We should allow apps to allocate more buffers if they want to.\n> \n> Kieran, Stefan, ideas ?\n\nThat code dates back 5 years, so I can only guess that it was done to\nensure stable operation without becoming too laggy. I completely agree\nthat an application should be able to freely decide how many buffers to\nallocate. The issue with RkISP1 is that the algorithm control loop has\nthe same length as the buffer count. So when you increase that hardcoded\nvalue to a bigger value, the regulation get's awfully slow. If I\nremember correctly this get's partially solved/worked around in this\nseries. It is also addressed in the PFC topics we'll discuss this week. \n\nSo I think we need a bit of internal discussions to come up with a well\ndefined path forward.\n\nBest regards,\nStefan\n\n> \n> > Other than that I don't see how libcamera could abstract this property away,\n> > as applications control which buffers are used for a given request.\n> >\n> \n> We certainly need a way to allow apps to allocate more than 4 buffers.\n> My main problem is with this property, which mixes in too many things\n> and is ill-defined.\n> \n> >\n> > [2]\n> > https://www.libcamera.org/guides/application-developer.html#request-queueing\n> >\n> >\n> >\n> > > > + This property is based on the minimum number of memory buffers\n> > > > + needed to fill the capture pipeline composed of hardware processing\n> > > > + blocks plus as many buffers as needed to take into account propagation\n> > > > + delays and avoid dropping frames.\n> > > > +\n> > > > + \\todo Should this be a per-stream property?\n> > > > +\n> > > > + \\todo Extend this property to expose the different minimum buffer and\n> > > > + request counts (the minimum number of buffers to be able to capture at\n> > > > + all, the minimum number of buffers to sustain capture without frame\n> > > > + drop, and the minimum number of requests to ensure proper operation of\n> > > > + per-frame controls).\n> > > > +\n> > > > ...\n> > > > --\n> > > > 2.49.0\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 69F3DC3226\n\tfor ;\n\tMon, 5 May 2025 14:40:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B81C268B2D;\n\tMon, 5 May 2025 16:40:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C25BA68B25\n\tfor ;\n\tMon, 5 May 2025 16:40:55 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:979c:6955:cf34:e7d4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 90DE1289;\n\tMon, 5 May 2025 16:40:45 +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=\"J+32NT2n\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1746456045;\n\tbh=lFCjyo4i0tfMlqCf7DOeUzC5s/ZTcOpRUac3UUkSEG4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=J+32NT2nlD1Cr7DF542CuXFwVpx32G1rqLed/vEr33m3XeAt+L29UF/FPtMCKTZpJ\n\tr5W3QGtWUou1nKyJbjqPXfnnoq05SEP+r6IGmpDAvlPouLiYSZYHc6o0W4Lwc0+dZn\n\tjEnYOIDNCL9t8v58aGE/JGWgNFSaFK+8ok2CNVto=","Date":"Mon, 5 May 2025 16:40:53 +0200","From":"Stefan Klug ","To":"Jacopo Mondi ","Cc":"Sven =?utf-8?q?P=C3=BCschel?= ,\n\tlibcamera-devel@lists.libcamera.org, =?utf-8?b?TsOtY29sYXMgRi4gUi4g?=\n\t=?utf-8?q?A=2E?= Prado ,\n\tPaul Elder , Umang Jain\n\t","Subject":"Re: [PATCH v11 01/19] libcamera: property: Add MinimumRequests\n\tproperty","Message-ID":"","References":"<20250428090413.38234-1-s.pueschel@pengutronix.de>\n\t<20250428090413.38234-2-s.pueschel@pengutronix.de>\n\t\n\t<1a211f82-704f-445a-a057-369501b467b3@pengutronix.de>\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":34133,"web_url":"https://patchwork.libcamera.org/comment/34133/","msgid":"","date":"2025-05-05T16:43:19","subject":"Re: [PATCH v11 01/19] libcamera: property: Add MinimumRequests\n\tproperty","submitter":{"id":225,"url":"https://patchwork.libcamera.org/api/people/225/","name":"Sven Püschel","email":"s.pueschel@pengutronix.de"},"content":"Hi Jacopo, hi Stefan,\n\nOn 5/5/25 16:40, Stefan Klug wrote:\n> Hi Jacopo,\n>\n> On Mon, May 05, 2025 at 03:59:33PM +0200, Jacopo Mondi wrote:\n>> Hi Sven\n>>\n>> On Mon, May 05, 2025 at 02:23:13PM +0200, Sven Püschel wrote:\n>>> Hi Jacopo,\n>>>\n>>> On 5/2/25 18:28, Jacopo Mondi wrote:\n>>>> Hi Sevn,\n>>>> thanks for resurecting this!\n>>>>\n>>>> Can I ask if this solves any actual problem and on which platform ?\n>>> The problem I'm facing is that in a more complex setup with the rkisp1 we\n>>> need to keep multiple dmabufs in the userspace. But the hardcoded value of 4\n>>> buffers in the rkisp1 driver seems to expect that the application only hold\n>>> around one buffer and have the other 3 buffers queued for a smooth frame\n>>> capture. But due to holding more dmabufs, I'm running into massive frame\n>>> drops due to usually only having one request queued and waiting for it to\n>>> finish.\n>> Thanks for the overview.\n>>\n>> Can't more buffers be allocated by setting a larger value in\n>> StreamConfiguration::bufferCount (which is indeed now hardcoded to 4) ?\n\nCurrently the bufferCount value is just overwritten in the validation of \nmost pipeline handlers. The documentation also provides this as an \nexample to set the bufferCount value to a fixed value in the validate() \nmethod [1]. But I think the raspberrypi pipeline handler already allows \nthe bufferCount to be set by the application. I wouldn't mind using the \nbufferCount to set the actual frame buffer values, as it would allow me \nto shrink this patchset down to the rkisp1 part and probably would also \nbe easier to merge (as it doesn't change the allocate function signature).\n\n[1] \nhttps://www.libcamera.org/guides/pipeline-handler.html#generating-a-default-configuration\n\n>>> Therefore I want to allocate more dmabufs for this problematic use case and\n>>> avoid patching libcamera to increase the hardcoded buffer count of rkisp1\n>>> (which would also apply globally).\n>>>\n>>> Kieran then pointed me towards this patchset to allow the application to\n>>> control the amount of dmabufs allocated.\n>>>\n>>>\n>>>> On Mon, Apr 28, 2025 at 11:02:26AM +0200, Sven Püschel wrote:\n>>>>> From: Nícolas F. R. A. Prado \n>>>>>\n>>>>> The MinimumRequests property reports the minimum number of capture\n>>>>> requests that the camera pipeline requires to have queued in order to\n>>>>> sustain capture operations without frame drops. At this quantity,\n>>>>> there's no guarantee that manual per-frame controls will apply\n>>>>> correctly. The quantity needed for that may be added as a separate\n>>>>> property in the future.\n>>>>>\n>>>>> The mali-c55 driver defines min_queued_buffers = 1 [1]. Therefore set\n>>>>> set the minimum requests to 2 to account one request in the userspace.\n>>>>>\n>>>>> The dcmipp and j721e drivers both defines min_queued_buffers = 1\n>>>>> in the kernel under\n>>>>> drivers/media/platform/st/stm32/stm32-dcmipp/dcmipp-bytecap.c\n>>>>> and drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c .\n>>>>> Therefore use these values, as they are added 1 more.\n>>>>>\n>>>>> For the intel-ipu6, mtk-seninf simple devices and the virtual pipeline\n>>>>> use a conservative value of 3 minimumBuffers, as no further requirements\n>>>>> are known about them.\n>>>>>\n>>>>> [1] https://lore.kernel.org/linux-media/20240131174355.GB20792@pendragon.ideasonboard.com/T/#t\n>>>>>\n>>>>> Signed-off-by: Nícolas F. R. A. Prado \n>>>>> Signed-off-by: Paul Elder \n>>>>> Acked-by: Umang Jain \n>>>>> Signed-off-by: Sven Püschel \n>>>>>\n>>>>> ---\n>>>>> Changes in v11\n>>>>> - Add mali-c55, dcmipp, virtual, j721e, intel-ipu6 and mtk-seninf\n>>>>> - Hold only the minimum buffers instead of the whole deviceInfo pointer in SimpleCameraData\n>>>>> - Adjusted min_buffers_needed property to min_reqbufs_allocation in docs\n>>>>> - Relax property description from no frame drops -> only minimal frame\n>>>>> drops, based on the comment from Naush [10]\n>>>>> - Changed property type from int32_t -> uint32_t to match all of the\n>>>>> indirect casts to an unsigned value throughout the existing patchset.\n>>>>> - Removed Reviewed-by: Laurent Pinchart \n>>>>> - Removed Reviewed-by: Paul Elder \n>>>>>\n>>>>> [10] https://lists.libcamera.org/pipermail/libcamera-devel/2022-December/035872.html\n>>>>>\n>>>>> Changes in v10:\n>>>>> - ipu3: add a constant to populate MinimumRequests, as we'll also use it\n>>>>> elsewhere\n>>>>> - update pipeline handler guide to set vivid' MinimumRequests to 2\n>>>>> - expand the MinimumRequests property description to include a line\n>>>>> about startup\n>>>>> - add isi\n>>>>>\n>>>>> Changes in v9:\n>>>>> - rebased\n>>>>>\n>>>>> Changes in v8:\n>>>>> - Changed the MinimumRequests property meaning to require that frames aren't\n>>>>> dropped\n>>>>> - Set MinimumRequests on SimplePipeline depending on device and usage of\n>>>>> converter\n>>>>> - Undid definition of default MinimumRequests on CameraSensor constructor\n>>>>> - Updated pipeline-handler guides with new MinimumRequests property\n>>>>>\n>>>>> Changes in v7:\n>>>>> - Renamed property from MinNumRequests to MinimumRequests\n>>>>> - Changed MinimumRequests property's description\n>>>>>\n>>>>> Changes in v6:\n>>>>> - Removed comment from Raspberrypi MinNumRequests setting\n>>>>> - Removed an extra blank line\n>>>>> ---\n>>>>> Documentation/guides/pipeline-handler.rst | 20 ++++++---\n>>>>> src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 2 +\n>>>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++\n>>>>> src/libcamera/pipeline/mali-c55/mali-c55.cpp | 1 +\n>>>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 1 +\n>>>>> .../pipeline/rpi/common/pipeline_base.cpp | 2 +\n>>>>> src/libcamera/pipeline/simple/simple.cpp | 41 +++++++++++++++----\n>>>>> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +\n>>>>> src/libcamera/pipeline/vimc/vimc.cpp | 2 +\n>>>>> src/libcamera/pipeline/virtual/virtual.cpp | 1 +\n>>>>> src/libcamera/property_ids_core.yaml | 22 ++++++++++\n>>>>> 11 files changed, 85 insertions(+), 13 deletions(-)\n>>>>>\n>>>> [snip]\n>>>>\n>>>>> index 834454a4..cc28d677 100644\n>>>>> --- a/src/libcamera/property_ids_core.yaml\n>>>>> +++ b/src/libcamera/property_ids_core.yaml\n>>>>> @@ -701,4 +701,26 @@ controls:\n>>>>>\n>>>>> Different cameras may report identical devices.\n>>>>>\n>>>>> + - MinimumRequests:\n>>>> I'm still not at ease with the definition of this property, I'm\n>>>> afraid.\n>>>>\n>>>> It seems to be mixing two different things (maybe 3 :)\n>>>>> + type: uint32_t\n>>>>> + description: |\n>>>>> + The minimum number of capture requests that the camera pipeline requires\n>>>>> + to have queued in order to sustain capture operations with only minimal\n>>>> The reference to \"minimal frame drops\" is a bit too loosely\n>>>> specified ?\n>>> I've changed it due to Naushir's claim that there is no practical number to\n>>> gurantee no frame drops [1]\n>>>\n>>> [1] https://lists.libcamera.org/pipermail/libcamera-devel/2022-December/035872.html\n>>>\n>>>\n>>>>> + frame drops. At this quantity, there's no guarantee that manual per\n>>>>> + frame controls will apply correctly. This is also the minimum number of\n>>>> The per-frame control pipeline depth does depend on other factors\n>>>> such as the sensor delays, specifically for manual AE/AWB control\n>>> and that is the reason it isn't or cannot be part of this property? Or\n>>> what's your point?\n>>>\n>> My point is that the pipeline depth (which would need a more formal\n>> definition as we use it quite freely without having ever set in stone\n>> its definition) is a property unrelated to the minium number of\n>> buffers or requests that has to be queued.\n>>\n>> I would leave pipeline depth and per-frame control out of the picture.\n>>\n>>>>> + requests that must be queued before capture starts.\n>>>> The number of buffers required to start streaming comes directly from\n>>>> the min_queued_buffers variable defined by drivers (unfortunatetly not\n>>>> available to users through any uAPI). One thing I would like to see\n>>>> happening is drivers stop setting min_queued_buffers at all, or if not\n>>>> possible maybe expose it through an api (a RO v4l2 control ?).\n>>>>\n>>>> Drivers should allocate internal buffers and use them if the\n>>>> application doesn't keep, and handle both the runtime underruns and\n>>>> start streaming conditions using scratch buffers.\n>>>>\n>>>> Even if this won't happen in drivers, the requirement should not be\n>>>> exposed to libcamera users. In the \"great scheme of things\" the idea\n>>>> is to decouple application's Requests from the frames pipeline, and\n>>>> providing enough buffers to sustain the frame rate should be a\n>>>> pipeline/IPA job.\n>>>>\n>>>> True, for platforms where drivers already require min_queued_buffers\n>>>> applications right now have to queue enough requests before\n>>>> start receiving frames back, and we goofly report it through\n>>>> StreamConfiguration::bufferCount.\n>>>>\n>>>> Unless there's an actual use case or any angle I'm grossly\n>>>> missing I don't think this property is a good idea..\n>>> I understand your problem with this property. But I don't know if we can get\n>>> away without providing the application a hint how many dmabufs it should\n>>> allocate at least.\n>>>\n>> Well, the default value set by generateConfiguration() should be an\n>> indication of what value the pipeline expects.\n\nThe patchset removes the bufferCount variable entirely. So the property \ntries to be (some kind of) a replacement to the info provided by \nbufferCount.\n\n>>> To recall my problem, the rkisp1 driver seems to already use scratch buffers\n>> Yes, I would love all drivers to remove min_queued_buffers as it was\n>> done for rkisp1\n>> https://www.spinics.net/lists/linux-media/msg264330.html\n>>\n>>> when it doesn't have enough buffers queued. So while queuing one buffer at a\n>>> time worked, it wasn't desired due to the frame drops from the scratch\n>>> buffers.\n>> Keeping up with the frame rate shouldn't be a driver concern but an\n>> application's one. But of course if you can't allocate enough buffers\n>> to queue them back fast enough, you can't do that.\n>>\n>>> But the application developer cannot really know how many buffers are really\n>>> required. From my perspective the current operation mode is that libcamera\n>> The fact is that \"really required\" is an ill-formed concepts. As said\n>> there are many different aspects at play: keeping up with the sensor's\n>> frame rate, the pipeline depth required to guarantee per-frame control\n>> and the number of buffers required to start streaming.\n>>\n>> We currently report some min number of buffers (most of the times, but\n>> not always coming from min_queued_buffers in the driver) in\n>> StreamConfiguration::bufferCount, but this is certainly is a\n>> sub-optimal API.\n>>\n>>> allocates a sensible fixed amount of dmabufs and application developers just\n>>> queue all unused buffers to libcamera [2]. So when the allocation is now put\n>>> into the hands of the application developer, it may just always allocate 2\n>>> buffers (to have still one queued when handling a completed buffer) and\n>>> therefore only queue a maximum of 2 buffers. The driver fills the lack of\n>>> buffers with scratch buffers, which cause unintended frame drops.\n>> That's what should happen in the driver. The alternative would be not\n>> being able to dequeue buffers at all. Simply, if an application\n>> allocates two buffers only, it's calling for troubles, isn't it ?\n>>\n>>> One potential solution may be to pass the desired amount of buffers held in\n>>> the application to the allocator. Then the pipeline could allocate a\n>>> sensible amount of buffers (e.g. 1 requested for the application + 3 queued\n>>> => 4 buffers) and the application would queue all unused buffers as usual.\n>>>\n>> I wonder why RkISP1Path::validate() reset bufferCount to 4.\n>> We should allow apps to allocate more buffers if they want to.\n>>\n>> Kieran, Stefan, ideas ?\n> That code dates back 5 years, so I can only guess that it was done to\n> ensure stable operation without becoming too laggy. I completely agree\n> that an application should be able to freely decide how many buffers to\n> allocate. The issue with RkISP1 is that the algorithm control loop has\n> the same length as the buffer count. So when you increase that hardcoded\n> value to a bigger value, the regulation get's awfully slow. If I\n> remember correctly this get's partially solved/worked around in this\n> series. It is also addressed in the PFC topics we'll discuss this week.\nThat's correct. This patchset sets the number of parameter/statistic \nbuffers to a fixed value (which allows proper operation), whereas the \nnumber of buffers the application desires is set using the allocate \nparameter (these are the \"Don't rely on bufferCount\" commits). Later in \nthe series, I've cherry-picked another patchset that queues additional \nrequests from the application, which fixes requests being canceled, when \nthe application queues too many buffers (and we don't have \nparameter/statistic buffers available).\n> So I think we need a bit of internal discussions to come up with a well\n> defined path forward.\nI'm excited for the result ^-^\n>\n> Best regards,\n> Stefan\n>\n>>> Other than that I don't see how libcamera could abstract this property away,\n>>> as applications control which buffers are used for a given request.\n>>>\n>> We certainly need a way to allow apps to allocate more than 4 buffers.\n>> My main problem is with this property, which mixes in too many things\n>> and is ill-defined.\n>>\n>>> [2]\n>>> https://www.libcamera.org/guides/application-developer.html#request-queueing\n>>>\n>>>\n>>>\n>>>>> + This property is based on the minimum number of memory buffers\n>>>>> + needed to fill the capture pipeline composed of hardware processing\n>>>>> + blocks plus as many buffers as needed to take into account propagation\n>>>>> + delays and avoid dropping frames.\n>>>>> +\n>>>>> + \\todo Should this be a per-stream property?\n>>>>> +\n>>>>> + \\todo Extend this property to expose the different minimum buffer and\n>>>>> + request counts (the minimum number of buffers to be able to capture at\n>>>>> + all, the minimum number of buffers to sustain capture without frame\n>>>>> + drop, and the minimum number of requests to ensure proper operation of\n>>>>> + per-frame controls).\n>>>>> +\n>>>>> ...\n>>>>> --\n>>>>> 2.49.0\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 AEDD0C3200\n\tfor ;\n\tMon, 5 May 2025 16:43:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E453D68B2D;\n\tMon, 5 May 2025 18:43:21 +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 32EF468B25\n\tfor ;\n\tMon, 5 May 2025 18:43:21 +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 )\n\tid 1uByuW-0000ti-CY; Mon, 05 May 2025 18:43:20 +0200"],"Message-ID":"","Date":"Mon, 5 May 2025 18:43:19 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v11 01/19] libcamera: property: Add MinimumRequests\n\tproperty","To":"Stefan Klug ,\n\tJacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, =?utf-8?b?TsOtY29sYXMgRi4gUi4g?=\n\t=?utf-8?q?A=2E_Prado?= ,\n\tPaul Elder , Umang Jain\n\t","References":"<20250428090413.38234-1-s.pueschel@pengutronix.de>\n\t<20250428090413.38234-2-s.pueschel@pengutronix.de>\n\t\n\t<1a211f82-704f-445a-a057-369501b467b3@pengutronix.de>\n\t\n\t","Content-Language":"en-US","From":"=?utf-8?q?Sven_P=C3=BCschel?= ","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":34352,"web_url":"https://patchwork.libcamera.org/comment/34352/","msgid":"<7d240903-86ac-4779-9ad8-52f6b2e5cbba@pengutronix.de>","date":"2025-05-26T14:08:21","subject":"Re: [PATCH v11 01/19] libcamera: property: Add MinimumRequests\n\tproperty","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 5/5/25 16:40, Stefan Klug wrote:\n> Hi Jacopo,\n>\n> On Mon, May 05, 2025 at 03:59:33PM +0200, Jacopo Mondi wrote:\n>> Hi Sven\n>>\n>> On Mon, May 05, 2025 at 02:23:13PM +0200, Sven Püschel wrote:\n>>> Hi Jacopo,\n>>>\n>>> On 5/2/25 18:28, Jacopo Mondi wrote:\n>>>> Hi Sevn,\n>>>> thanks for resurecting this!\n>>>>\n>>>> Can I ask if this solves any actual problem and on which platform ?\n>>> The problem I'm facing is that in a more complex setup with the rkisp1 we\n>>> need to keep multiple dmabufs in the userspace. But the hardcoded value of 4\n>>> buffers in the rkisp1 driver seems to expect that the application only hold\n>>> around one buffer and have the other 3 buffers queued for a smooth frame\n>>> capture. But due to holding more dmabufs, I'm running into massive frame\n>>> drops due to usually only having one request queued and waiting for it to\n>>> finish.\n>> Thanks for the overview.\n>>\n>> Can't more buffers be allocated by setting a larger value in\n>> StreamConfiguration::bufferCount (which is indeed now hardcoded to 4) ?\n>>\n>>> Therefore I want to allocate more dmabufs for this problematic use case and\n>>> avoid patching libcamera to increase the hardcoded buffer count of rkisp1\n>>> (which would also apply globally).\n>>>\n>>> Kieran then pointed me towards this patchset to allow the application to\n>>> control the amount of dmabufs allocated.\n>>>\n>>>\n>>>> On Mon, Apr 28, 2025 at 11:02:26AM +0200, Sven Püschel wrote:\n>>>>> From: Nícolas F. R. A. Prado \n>>>>>\n>>>>> The MinimumRequests property reports the minimum number of capture\n>>>>> requests that the camera pipeline requires to have queued in order to\n>>>>> sustain capture operations without frame drops. At this quantity,\n>>>>> there's no guarantee that manual per-frame controls will apply\n>>>>> correctly. The quantity needed for that may be added as a separate\n>>>>> property in the future.\n>>>>>\n>>>>> The mali-c55 driver defines min_queued_buffers = 1 [1]. Therefore set\n>>>>> set the minimum requests to 2 to account one request in the userspace.\n>>>>>\n>>>>> The dcmipp and j721e drivers both defines min_queued_buffers = 1\n>>>>> in the kernel under\n>>>>> drivers/media/platform/st/stm32/stm32-dcmipp/dcmipp-bytecap.c\n>>>>> and drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c .\n>>>>> Therefore use these values, as they are added 1 more.\n>>>>>\n>>>>> For the intel-ipu6, mtk-seninf simple devices and the virtual pipeline\n>>>>> use a conservative value of 3 minimumBuffers, as no further requirements\n>>>>> are known about them.\n>>>>>\n>>>>> [1] https://lore.kernel.org/linux-media/20240131174355.GB20792@pendragon.ideasonboard.com/T/#t\n>>>>>\n>>>>> Signed-off-by: Nícolas F. R. A. Prado \n>>>>> Signed-off-by: Paul Elder \n>>>>> Acked-by: Umang Jain \n>>>>> Signed-off-by: Sven Püschel \n>>>>>\n>>>>> ---\n>>>>> Changes in v11\n>>>>> - Add mali-c55, dcmipp, virtual, j721e, intel-ipu6 and mtk-seninf\n>>>>> - Hold only the minimum buffers instead of the whole deviceInfo pointer in SimpleCameraData\n>>>>> - Adjusted min_buffers_needed property to min_reqbufs_allocation in docs\n>>>>> - Relax property description from no frame drops -> only minimal frame\n>>>>> drops, based on the comment from Naush [10]\n>>>>> - Changed property type from int32_t -> uint32_t to match all of the\n>>>>> indirect casts to an unsigned value throughout the existing patchset.\n>>>>> - Removed Reviewed-by: Laurent Pinchart \n>>>>> - Removed Reviewed-by: Paul Elder \n>>>>>\n>>>>> [10] https://lists.libcamera.org/pipermail/libcamera-devel/2022-December/035872.html\n>>>>>\n>>>>> Changes in v10:\n>>>>> - ipu3: add a constant to populate MinimumRequests, as we'll also use it\n>>>>> elsewhere\n>>>>> - update pipeline handler guide to set vivid' MinimumRequests to 2\n>>>>> - expand the MinimumRequests property description to include a line\n>>>>> about startup\n>>>>> - add isi\n>>>>>\n>>>>> Changes in v9:\n>>>>> - rebased\n>>>>>\n>>>>> Changes in v8:\n>>>>> - Changed the MinimumRequests property meaning to require that frames aren't\n>>>>> dropped\n>>>>> - Set MinimumRequests on SimplePipeline depending on device and usage of\n>>>>> converter\n>>>>> - Undid definition of default MinimumRequests on CameraSensor constructor\n>>>>> - Updated pipeline-handler guides with new MinimumRequests property\n>>>>>\n>>>>> Changes in v7:\n>>>>> - Renamed property from MinNumRequests to MinimumRequests\n>>>>> - Changed MinimumRequests property's description\n>>>>>\n>>>>> Changes in v6:\n>>>>> - Removed comment from Raspberrypi MinNumRequests setting\n>>>>> - Removed an extra blank line\n>>>>> ---\n>>>>> Documentation/guides/pipeline-handler.rst | 20 ++++++---\n>>>>> src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 2 +\n>>>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++\n>>>>> src/libcamera/pipeline/mali-c55/mali-c55.cpp | 1 +\n>>>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 1 +\n>>>>> .../pipeline/rpi/common/pipeline_base.cpp | 2 +\n>>>>> src/libcamera/pipeline/simple/simple.cpp | 41 +++++++++++++++----\n>>>>> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +\n>>>>> src/libcamera/pipeline/vimc/vimc.cpp | 2 +\n>>>>> src/libcamera/pipeline/virtual/virtual.cpp | 1 +\n>>>>> src/libcamera/property_ids_core.yaml | 22 ++++++++++\n>>>>> 11 files changed, 85 insertions(+), 13 deletions(-)\n>>>>>\n>>>> [snip]\n>>>>\n>>>>> index 834454a4..cc28d677 100644\n>>>>> --- a/src/libcamera/property_ids_core.yaml\n>>>>> +++ b/src/libcamera/property_ids_core.yaml\n>>>>> @@ -701,4 +701,26 @@ controls:\n>>>>>\n>>>>> Different cameras may report identical devices.\n>>>>>\n>>>>> + - MinimumRequests:\n>>>> I'm still not at ease with the definition of this property, I'm\n>>>> afraid.\n>>>>\n>>>> It seems to be mixing two different things (maybe 3 :)\n>>>>> + type: uint32_t\n>>>>> + description: |\n>>>>> + The minimum number of capture requests that the camera pipeline requires\n>>>>> + to have queued in order to sustain capture operations with only minimal\n>>>> The reference to \"minimal frame drops\" is a bit too loosely\n>>>> specified ?\n>>> I've changed it due to Naushir's claim that there is no practical number to\n>>> gurantee no frame drops [1]\n>>>\n>>> [1] https://lists.libcamera.org/pipermail/libcamera-devel/2022-December/035872.html\n>>>\n>>>\n>>>>> + frame drops. At this quantity, there's no guarantee that manual per\n>>>>> + frame controls will apply correctly. This is also the minimum number of\n>>>> The per-frame control pipeline depth does depend on other factors\n>>>> such as the sensor delays, specifically for manual AE/AWB control\n>>> and that is the reason it isn't or cannot be part of this property? Or\n>>> what's your point?\n>>>\n>> My point is that the pipeline depth (which would need a more formal\n>> definition as we use it quite freely without having ever set in stone\n>> its definition) is a property unrelated to the minium number of\n>> buffers or requests that has to be queued.\n>>\n>> I would leave pipeline depth and per-frame control out of the picture.\n>>\n>>>>> + requests that must be queued before capture starts.\n>>>> The number of buffers required to start streaming comes directly from\n>>>> the min_queued_buffers variable defined by drivers (unfortunatetly not\n>>>> available to users through any uAPI). One thing I would like to see\n>>>> happening is drivers stop setting min_queued_buffers at all, or if not\n>>>> possible maybe expose it through an api (a RO v4l2 control ?).\n>>>>\n>>>> Drivers should allocate internal buffers and use them if the\n>>>> application doesn't keep, and handle both the runtime underruns and\n>>>> start streaming conditions using scratch buffers.\n>>>>\n>>>> Even if this won't happen in drivers, the requirement should not be\n>>>> exposed to libcamera users. In the \"great scheme of things\" the idea\n>>>> is to decouple application's Requests from the frames pipeline, and\n>>>> providing enough buffers to sustain the frame rate should be a\n>>>> pipeline/IPA job.\n>>>>\n>>>> True, for platforms where drivers already require min_queued_buffers\n>>>> applications right now have to queue enough requests before\n>>>> start receiving frames back, and we goofly report it through\n>>>> StreamConfiguration::bufferCount.\n>>>>\n>>>> Unless there's an actual use case or any angle I'm grossly\n>>>> missing I don't think this property is a good idea..\n>>> I understand your problem with this property. But I don't know if we can get\n>>> away without providing the application a hint how many dmabufs it should\n>>> allocate at least.\n>>>\n>> Well, the default value set by generateConfiguration() should be an\n>> indication of what value the pipeline expects.\n>>\n>>> To recall my problem, the rkisp1 driver seems to already use scratch buffers\n>> Yes, I would love all drivers to remove min_queued_buffers as it was\n>> done for rkisp1\n>> https://www.spinics.net/lists/linux-media/msg264330.html\n>>\n>>> when it doesn't have enough buffers queued. So while queuing one buffer at a\n>>> time worked, it wasn't desired due to the frame drops from the scratch\n>>> buffers.\n>> Keeping up with the frame rate shouldn't be a driver concern but an\n>> application's one. But of course if you can't allocate enough buffers\n>> to queue them back fast enough, you can't do that.\n>>\n>>> But the application developer cannot really know how many buffers are really\n>>> required. From my perspective the current operation mode is that libcamera\n>> The fact is that \"really required\" is an ill-formed concepts. As said\n>> there are many different aspects at play: keeping up with the sensor's\n>> frame rate, the pipeline depth required to guarantee per-frame control\n>> and the number of buffers required to start streaming.\n>>\n>> We currently report some min number of buffers (most of the times, but\n>> not always coming from min_queued_buffers in the driver) in\n>> StreamConfiguration::bufferCount, but this is certainly is a\n>> sub-optimal API.\n>>\n>>> allocates a sensible fixed amount of dmabufs and application developers just\n>>> queue all unused buffers to libcamera [2]. So when the allocation is now put\n>>> into the hands of the application developer, it may just always allocate 2\n>>> buffers (to have still one queued when handling a completed buffer) and\n>>> therefore only queue a maximum of 2 buffers. The driver fills the lack of\n>>> buffers with scratch buffers, which cause unintended frame drops.\n>> That's what should happen in the driver. The alternative would be not\n>> being able to dequeue buffers at all. Simply, if an application\n>> allocates two buffers only, it's calling for troubles, isn't it ?\n>>\n>>> One potential solution may be to pass the desired amount of buffers held in\n>>> the application to the allocator. Then the pipeline could allocate a\n>>> sensible amount of buffers (e.g. 1 requested for the application + 3 queued\n>>> => 4 buffers) and the application would queue all unused buffers as usual.\n>>>\n>> I wonder why RkISP1Path::validate() reset bufferCount to 4.\n>> We should allow apps to allocate more buffers if they want to.\n>>\n>> Kieran, Stefan, ideas ?\n> That code dates back 5 years, so I can only guess that it was done to\n> ensure stable operation without becoming too laggy. I completely agree\n> that an application should be able to freely decide how many buffers to\n> allocate. The issue with RkISP1 is that the algorithm control loop has\n> the same length as the buffer count. So when you increase that hardcoded\n> value to a bigger value, the regulation get's awfully slow. If I\n> remember correctly this get's partially solved/worked around in this\n> series. It is also addressed in the PFC topics we'll discuss this week.\n\nJust noticed: Why only \"partially solved/worked around\"? What did I miss?\n\n\n> So I think we need a bit of internal discussions to come up with a well\n> defined path forward.\n\nGiven that now 3 weeks gone by, are there any updates on the internal \ndiscussions?\n\nAs noted in [1] I wouldn't mind to drop the property and instead allow \napplications to set the bufferCount variable to specify the desired \namount of buffers to allocate. But I would like to know if this is the \ndesired path or if some other path is better suited before investing the \ntime to create a new patch series.\n\n[1] \nhttps://lists.libcamera.org/pipermail/libcamera-devel/2025-May/050167.html\n\n\nSincerely\n\n     Sven\n\n\n>\n> Best regards,\n> Stefan\n>\n>>> Other than that I don't see how libcamera could abstract this property away,\n>>> as applications control which buffers are used for a given request.\n>>>\n>> We certainly need a way to allow apps to allocate more than 4 buffers.\n>> My main problem is with this property, which mixes in too many things\n>> and is ill-defined.\n>>\n>>> [2]\n>>> https://www.libcamera.org/guides/application-developer.html#request-queueing\n>>>\n>>>\n>>>\n>>>>> + This property is based on the minimum number of memory buffers\n>>>>> + needed to fill the capture pipeline composed of hardware processing\n>>>>> + blocks plus as many buffers as needed to take into account propagation\n>>>>> + delays and avoid dropping frames.\n>>>>> +\n>>>>> + \\todo Should this be a per-stream property?\n>>>>> +\n>>>>> + \\todo Extend this property to expose the different minimum buffer and\n>>>>> + request counts (the minimum number of buffers to be able to capture at\n>>>>> + all, the minimum number of buffers to sustain capture without frame\n>>>>> + drop, and the minimum number of requests to ensure proper operation of\n>>>>> + per-frame controls).\n>>>>> +\n>>>>> ...\n>>>>> --\n>>>>> 2.49.0\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 DDFEFC3237\n\tfor ;\n\tMon, 26 May 2025 14:08:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AE01368D9D;\n\tMon, 26 May 2025 16:08:25 +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 414C0627DA\n\tfor ;\n\tMon, 26 May 2025 16:08:23 +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 )\n\tid 1uJYV4-0006cN-2g; Mon, 26 May 2025 16:08:22 +0200"],"Message-ID":"<7d240903-86ac-4779-9ad8-52f6b2e5cbba@pengutronix.de>","Date":"Mon, 26 May 2025 16:08:21 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v11 01/19] libcamera: property: Add MinimumRequests\n\tproperty","To":"Stefan Klug ,\n\tJacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, =?utf-8?b?TsOtY29sYXMgRi4gUi4g?=\n\t=?utf-8?q?A=2E_Prado?= ,\n\tPaul Elder , Umang Jain\n\t, graphics@pengutronix.de","References":"<20250428090413.38234-1-s.pueschel@pengutronix.de>\n\t<20250428090413.38234-2-s.pueschel@pengutronix.de>\n\t\n\t<1a211f82-704f-445a-a057-369501b467b3@pengutronix.de>\n\t\n\t","Content-Language":"en-US","From":"=?utf-8?q?Sven_P=C3=BCschel?= ","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":34355,"web_url":"https://patchwork.libcamera.org/comment/34355/","msgid":"<174829642379.9053.4400503789871012128@localhost>","date":"2025-05-26T21:53:43","subject":"Re: [PATCH v11 01/19] libcamera: property: Add MinimumRequests\n\tproperty","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Sven,\n\nQuoting Sven Püschel (2025-05-26 16:08:21)\n> Hi Stefan,\n> \n> On 5/5/25 16:40, Stefan Klug wrote:\n> > Hi Jacopo,\n> >\n> > On Mon, May 05, 2025 at 03:59:33PM +0200, Jacopo Mondi wrote:\n> >> Hi Sven\n> >>\n> >> On Mon, May 05, 2025 at 02:23:13PM +0200, Sven Püschel wrote:\n> >>> Hi Jacopo,\n> >>>\n> >>> On 5/2/25 18:28, Jacopo Mondi wrote:\n> >>>> Hi Sevn,\n> >>>> thanks for resurecting this!\n> >>>>\n> >>>> Can I ask if this solves any actual problem and on which platform ?\n> >>> The problem I'm facing is that in a more complex setup with the rkisp1 we\n> >>> need to keep multiple dmabufs in the userspace. But the hardcoded value of 4\n> >>> buffers in the rkisp1 driver seems to expect that the application only hold\n> >>> around one buffer and have the other 3 buffers queued for a smooth frame\n> >>> capture. But due to holding more dmabufs, I'm running into massive frame\n> >>> drops due to usually only having one request queued and waiting for it to\n> >>> finish.\n> >> Thanks for the overview.\n> >>\n> >> Can't more buffers be allocated by setting a larger value in\n> >> StreamConfiguration::bufferCount (which is indeed now hardcoded to 4) ?\n> >>\n> >>> Therefore I want to allocate more dmabufs for this problematic use case and\n> >>> avoid patching libcamera to increase the hardcoded buffer count of rkisp1\n> >>> (which would also apply globally).\n> >>>\n> >>> Kieran then pointed me towards this patchset to allow the application to\n> >>> control the amount of dmabufs allocated.\n> >>>\n> >>>\n> >>>> On Mon, Apr 28, 2025 at 11:02:26AM +0200, Sven Püschel wrote:\n> >>>>> From: Nícolas F. R. A. Prado \n> >>>>>\n> >>>>> The MinimumRequests property reports the minimum number of capture\n> >>>>> requests that the camera pipeline requires to have queued in order to\n> >>>>> sustain capture operations without frame drops. At this quantity,\n> >>>>> there's no guarantee that manual per-frame controls will apply\n> >>>>> correctly. The quantity needed for that may be added as a separate\n> >>>>> property in the future.\n> >>>>>\n> >>>>> The mali-c55 driver defines min_queued_buffers = 1 [1]. Therefore set\n> >>>>> set the minimum requests to 2 to account one request in the userspace.\n> >>>>>\n> >>>>> The dcmipp and j721e drivers both defines min_queued_buffers = 1\n> >>>>> in the kernel under\n> >>>>> drivers/media/platform/st/stm32/stm32-dcmipp/dcmipp-bytecap.c\n> >>>>> and drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c .\n> >>>>> Therefore use these values, as they are added 1 more.\n> >>>>>\n> >>>>> For the intel-ipu6, mtk-seninf simple devices and the virtual pipeline\n> >>>>> use a conservative value of 3 minimumBuffers, as no further requirements\n> >>>>> are known about them.\n> >>>>>\n> >>>>> [1] https://lore.kernel.org/linux-media/20240131174355.GB20792@pendragon.ideasonboard.com/T/#t\n> >>>>>\n> >>>>> Signed-off-by: Nícolas F. R. A. Prado \n> >>>>> Signed-off-by: Paul Elder \n> >>>>> Acked-by: Umang Jain \n> >>>>> Signed-off-by: Sven Püschel \n> >>>>>\n> >>>>> ---\n> >>>>> Changes in v11\n> >>>>> - Add mali-c55, dcmipp, virtual, j721e, intel-ipu6 and mtk-seninf\n> >>>>> - Hold only the minimum buffers instead of the whole deviceInfo pointer in SimpleCameraData\n> >>>>> - Adjusted min_buffers_needed property to min_reqbufs_allocation in docs\n> >>>>> - Relax property description from no frame drops -> only minimal frame\n> >>>>> drops, based on the comment from Naush [10]\n> >>>>> - Changed property type from int32_t -> uint32_t to match all of the\n> >>>>> indirect casts to an unsigned value throughout the existing patchset.\n> >>>>> - Removed Reviewed-by: Laurent Pinchart \n> >>>>> - Removed Reviewed-by: Paul Elder \n> >>>>>\n> >>>>> [10] https://lists.libcamera.org/pipermail/libcamera-devel/2022-December/035872.html\n> >>>>>\n> >>>>> Changes in v10:\n> >>>>> - ipu3: add a constant to populate MinimumRequests, as we'll also use it\n> >>>>> elsewhere\n> >>>>> - update pipeline handler guide to set vivid' MinimumRequests to 2\n> >>>>> - expand the MinimumRequests property description to include a line\n> >>>>> about startup\n> >>>>> - add isi\n> >>>>>\n> >>>>> Changes in v9:\n> >>>>> - rebased\n> >>>>>\n> >>>>> Changes in v8:\n> >>>>> - Changed the MinimumRequests property meaning to require that frames aren't\n> >>>>> dropped\n> >>>>> - Set MinimumRequests on SimplePipeline depending on device and usage of\n> >>>>> converter\n> >>>>> - Undid definition of default MinimumRequests on CameraSensor constructor\n> >>>>> - Updated pipeline-handler guides with new MinimumRequests property\n> >>>>>\n> >>>>> Changes in v7:\n> >>>>> - Renamed property from MinNumRequests to MinimumRequests\n> >>>>> - Changed MinimumRequests property's description\n> >>>>>\n> >>>>> Changes in v6:\n> >>>>> - Removed comment from Raspberrypi MinNumRequests setting\n> >>>>> - Removed an extra blank line\n> >>>>> ---\n> >>>>> Documentation/guides/pipeline-handler.rst | 20 ++++++---\n> >>>>> src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 2 +\n> >>>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++\n> >>>>> src/libcamera/pipeline/mali-c55/mali-c55.cpp | 1 +\n> >>>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 1 +\n> >>>>> .../pipeline/rpi/common/pipeline_base.cpp | 2 +\n> >>>>> src/libcamera/pipeline/simple/simple.cpp | 41 +++++++++++++++----\n> >>>>> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +\n> >>>>> src/libcamera/pipeline/vimc/vimc.cpp | 2 +\n> >>>>> src/libcamera/pipeline/virtual/virtual.cpp | 1 +\n> >>>>> src/libcamera/property_ids_core.yaml | 22 ++++++++++\n> >>>>> 11 files changed, 85 insertions(+), 13 deletions(-)\n> >>>>>\n> >>>> [snip]\n> >>>>\n> >>>>> index 834454a4..cc28d677 100644\n> >>>>> --- a/src/libcamera/property_ids_core.yaml\n> >>>>> +++ b/src/libcamera/property_ids_core.yaml\n> >>>>> @@ -701,4 +701,26 @@ controls:\n> >>>>>\n> >>>>> Different cameras may report identical devices.\n> >>>>>\n> >>>>> + - MinimumRequests:\n> >>>> I'm still not at ease with the definition of this property, I'm\n> >>>> afraid.\n> >>>>\n> >>>> It seems to be mixing two different things (maybe 3 :)\n> >>>>> + type: uint32_t\n> >>>>> + description: |\n> >>>>> + The minimum number of capture requests that the camera pipeline requires\n> >>>>> + to have queued in order to sustain capture operations with only minimal\n> >>>> The reference to \"minimal frame drops\" is a bit too loosely\n> >>>> specified ?\n> >>> I've changed it due to Naushir's claim that there is no practical number to\n> >>> gurantee no frame drops [1]\n> >>>\n> >>> [1] https://lists.libcamera.org/pipermail/libcamera-devel/2022-December/035872.html\n> >>>\n> >>>\n> >>>>> + frame drops. At this quantity, there's no guarantee that manual per\n> >>>>> + frame controls will apply correctly. This is also the minimum number of\n> >>>> The per-frame control pipeline depth does depend on other factors\n> >>>> such as the sensor delays, specifically for manual AE/AWB control\n> >>> and that is the reason it isn't or cannot be part of this property? Or\n> >>> what's your point?\n> >>>\n> >> My point is that the pipeline depth (which would need a more formal\n> >> definition as we use it quite freely without having ever set in stone\n> >> its definition) is a property unrelated to the minium number of\n> >> buffers or requests that has to be queued.\n> >>\n> >> I would leave pipeline depth and per-frame control out of the picture.\n> >>\n> >>>>> + requests that must be queued before capture starts.\n> >>>> The number of buffers required to start streaming comes directly from\n> >>>> the min_queued_buffers variable defined by drivers (unfortunatetly not\n> >>>> available to users through any uAPI). One thing I would like to see\n> >>>> happening is drivers stop setting min_queued_buffers at all, or if not\n> >>>> possible maybe expose it through an api (a RO v4l2 control ?).\n> >>>>\n> >>>> Drivers should allocate internal buffers and use them if the\n> >>>> application doesn't keep, and handle both the runtime underruns and\n> >>>> start streaming conditions using scratch buffers.\n> >>>>\n> >>>> Even if this won't happen in drivers, the requirement should not be\n> >>>> exposed to libcamera users. In the \"great scheme of things\" the idea\n> >>>> is to decouple application's Requests from the frames pipeline, and\n> >>>> providing enough buffers to sustain the frame rate should be a\n> >>>> pipeline/IPA job.\n> >>>>\n> >>>> True, for platforms where drivers already require min_queued_buffers\n> >>>> applications right now have to queue enough requests before\n> >>>> start receiving frames back, and we goofly report it through\n> >>>> StreamConfiguration::bufferCount.\n> >>>>\n> >>>> Unless there's an actual use case or any angle I'm grossly\n> >>>> missing I don't think this property is a good idea..\n> >>> I understand your problem with this property. But I don't know if we can get\n> >>> away without providing the application a hint how many dmabufs it should\n> >>> allocate at least.\n> >>>\n> >> Well, the default value set by generateConfiguration() should be an\n> >> indication of what value the pipeline expects.\n> >>\n> >>> To recall my problem, the rkisp1 driver seems to already use scratch buffers\n> >> Yes, I would love all drivers to remove min_queued_buffers as it was\n> >> done for rkisp1\n> >> https://www.spinics.net/lists/linux-media/msg264330.html\n> >>\n> >>> when it doesn't have enough buffers queued. So while queuing one buffer at a\n> >>> time worked, it wasn't desired due to the frame drops from the scratch\n> >>> buffers.\n> >> Keeping up with the frame rate shouldn't be a driver concern but an\n> >> application's one. But of course if you can't allocate enough buffers\n> >> to queue them back fast enough, you can't do that.\n> >>\n> >>> But the application developer cannot really know how many buffers are really\n> >>> required. From my perspective the current operation mode is that libcamera\n> >> The fact is that \"really required\" is an ill-formed concepts. As said\n> >> there are many different aspects at play: keeping up with the sensor's\n> >> frame rate, the pipeline depth required to guarantee per-frame control\n> >> and the number of buffers required to start streaming.\n> >>\n> >> We currently report some min number of buffers (most of the times, but\n> >> not always coming from min_queued_buffers in the driver) in\n> >> StreamConfiguration::bufferCount, but this is certainly is a\n> >> sub-optimal API.\n> >>\n> >>> allocates a sensible fixed amount of dmabufs and application developers just\n> >>> queue all unused buffers to libcamera [2]. So when the allocation is now put\n> >>> into the hands of the application developer, it may just always allocate 2\n> >>> buffers (to have still one queued when handling a completed buffer) and\n> >>> therefore only queue a maximum of 2 buffers. The driver fills the lack of\n> >>> buffers with scratch buffers, which cause unintended frame drops.\n> >> That's what should happen in the driver. The alternative would be not\n> >> being able to dequeue buffers at all. Simply, if an application\n> >> allocates two buffers only, it's calling for troubles, isn't it ?\n> >>\n> >>> One potential solution may be to pass the desired amount of buffers held in\n> >>> the application to the allocator. Then the pipeline could allocate a\n> >>> sensible amount of buffers (e.g. 1 requested for the application + 3 queued\n> >>> => 4 buffers) and the application would queue all unused buffers as usual.\n> >>>\n> >> I wonder why RkISP1Path::validate() reset bufferCount to 4.\n> >> We should allow apps to allocate more buffers if they want to.\n> >>\n> >> Kieran, Stefan, ideas ?\n> > That code dates back 5 years, so I can only guess that it was done to\n> > ensure stable operation without becoming too laggy. I completely agree\n> > that an application should be able to freely decide how many buffers to\n> > allocate. The issue with RkISP1 is that the algorithm control loop has\n> > the same length as the buffer count. So when you increase that hardcoded\n> > value to a bigger value, the regulation get's awfully slow. If I\n> > remember correctly this get's partially solved/worked around in this\n> > series. It is also addressed in the PFC topics we'll discuss this week.\n> \n> Just noticed: Why only \"partially solved/worked around\"? What did I miss?\n\nThe issue here is that a lot of discussions circulate around the larger\ntopic of per frame controls and the idea of splitting requests from\nbuffers. Adding a new control in that area break the API at a point in\ntime where the concepts have not yet fully settled.\n\n> \n> \n> > So I think we need a bit of internal discussions to come up with a well\n> > defined path forward.\n> \n> Given that now 3 weeks gone by, are there any updates on the internal \n> discussions?\n\nThank you for your patience and the ping. I' not at work this week, but\nwanted to give you some feedback. I just posted an RFC with the result\nof those discussions. As you also mentioned below we believe sticking to\nbufferCount for now is the least intrusive way forward and leaves room\nfor the upcoming PFC changes.\n\n> \n> As noted in [1] I wouldn't mind to drop the property and instead allow \n> applications to set the bufferCount variable to specify the desired \n> amount of buffers to allocate. But I would like to know if this is the \n> desired path or if some other path is better suited before investing the \n> time to create a new patch series.\n\nThe implementation proposed in the RFC lives inside the PipelineHandler\nand can therefore be reused by all pipelines. I didn't check how easy it\nwould be to adapt the rest of your series and if not mistaken, Paul\nalready pulled in one of those patches.\n\nWould be gerat if you could give the RFC a try. As noted it is not too\nwell tested...\n\nBest regards,\nStefan\n\n\n> \n> [1] \n> https://lists.libcamera.org/pipermail/libcamera-devel/2025-May/050167.html\n> \n> \n> Sincerely\n> \n>     Sven\n> \n> \n> >\n> > Best regards,\n> > Stefan\n> >\n> >>> Other than that I don't see how libcamera could abstract this property away,\n> >>> as applications control which buffers are used for a given request.\n> >>>\n> >> We certainly need a way to allow apps to allocate more than 4 buffers.\n> >> My main problem is with this property, which mixes in too many things\n> >> and is ill-defined.\n> >>\n> >>> [2]\n> >>> https://www.libcamera.org/guides/application-developer.html#request-queueing\n> >>>\n> >>>\n> >>>\n> >>>>> + This property is based on the minimum number of memory buffers\n> >>>>> + needed to fill the capture pipeline composed of hardware processing\n> >>>>> + blocks plus as many buffers as needed to take into account propagation\n> >>>>> + delays and avoid dropping frames.\n> >>>>> +\n> >>>>> + \\todo Should this be a per-stream property?\n> >>>>> +\n> >>>>> + \\todo Extend this property to expose the different minimum buffer and\n> >>>>> + request counts (the minimum number of buffers to be able to capture at\n> >>>>> + all, the minimum number of buffers to sustain capture without frame\n> >>>>> + drop, and the minimum number of requests to ensure proper operation of\n> >>>>> + per-frame controls).\n> >>>>> +\n> >>>>> ...\n> >>>>> --\n> >>>>> 2.49.0\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 4D945C3237\n\tfor ;\n\tMon, 26 May 2025 21:53:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 64E4968D9D;\n\tMon, 26 May 2025 23:53:49 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 11540627DA\n\tfor ;\n\tMon, 26 May 2025 23:53:48 +0200 (CEST)","from ideasonboard.com (tmo-070-11.customers.d1-online.com\n\t[80.187.70.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 695CD581;\n\tMon, 26 May 2025 23:53:22 +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=\"r9uqfcPz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1748296402;\n\tbh=VvykvdEF6n2ipsyYd6kylfX9q6ZHICtw1Vtrp1EXSbU=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=r9uqfcPzDhfCzMld6Ff4AmYcnk3rKfUTckaXBdid1TKTkd8K/I3SvXp9wAL08B3NG\n\tv1B5gCQ9lDmNau5O1m1sv4wacYj87hn4a7emF7QyPdcec1+kYzC339Y4maUqgrGfty\n\t5+ghH28Qg3OxNoH8BTFx0CO2zrP5fMQ08blGUAl4=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<7d240903-86ac-4779-9ad8-52f6b2e5cbba@pengutronix.de>","References":"<20250428090413.38234-1-s.pueschel@pengutronix.de>\n\t<20250428090413.38234-2-s.pueschel@pengutronix.de>\n\t\n\t<1a211f82-704f-445a-a057-369501b467b3@pengutronix.de>\n\t\n\t\n\t<7d240903-86ac-4779-9ad8-52f6b2e5cbba@pengutronix.de>","Subject":"Re: [PATCH v11 01/19] libcamera: property: Add MinimumRequests\n\tproperty","From":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org, =?utf-8?q?N=C3=ADcolas?=\n\tF. R. A. Prado , Paul Elder\n\t, Umang Jain ,\n\tgraphics@pengutronix.de","To":"Jacopo Mondi , Sven =?utf-8?b?UMO8c2No?=\n\t=?utf-8?q?el?= ","Date":"Mon, 26 May 2025 23:53:43 +0200","Message-ID":"<174829642379.9053.4400503789871012128@localhost>","User-Agent":"alot/0.12.dev16+g501a9541e2e6.d20250519","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\" "}}]