[{"id":18184,"web_url":"https://patchwork.libcamera.org/comment/18184/","msgid":"<5c7a5506-11b3-ef74-384b-b99be14b0da7@ideasonboard.com>","date":"2021-07-15T09:45:56","subject":"Re: [libcamera-devel] [PATCH v6 01/10] libcamera: property: Add\n\tMinNumRequests property","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Nicolas,\n\nOn 14/07/2021 19:38, Nícolas F. R. A. Prado wrote:\n> The MinNumRequests property reports the bare minimum number of requests\n> needed in the camera pipeline for capture to be possible. It is equal to\n> the number of buffers required by the driver. At this quantity, there's\n> no guarantee that frames won't be dropped or that manual per-frame\n> controls will apply correctly. The quantity needed for those may be\n> added as separate properties in the future.\n> \n> Signed-off-by: Nícolas F. R. A. Prado \n> ---\n> \n> Changes in v6:\n> - Thanks to Naushir:\n> - Removed comment from Raspberrypi MinNumRequests setting\n> - Removed an extra blank line\n> \n> src/libcamera/pipeline/ipu3/ipu3.cpp | 6 ++++++\n> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 2 ++\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 ++\n> src/libcamera/pipeline/simple/simple.cpp | 2 ++\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 ++\n> src/libcamera/pipeline/vimc/vimc.cpp | 2 ++\n> src/libcamera/property_ids.yaml | 8 ++++++++\n> 7 files changed, 24 insertions(+)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 76c3bb3d8aa9..017018c845fa 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -1111,6 +1111,12 @@ int PipelineHandlerIPU3::registerCameras()\n> \t\t/* Initialize the camera properties. */\n> \t\tdata->properties_ = cio2->sensor()->properties();\n> \n> +\t\t/*\n> +\t\t * \\todo Make sure this is correct even after the stream is\n> +\t\t * configured to CIO2\n\nWhat does this comment mean?\n\nI see that you don't have a reference to IPU3 in your cover letter, so\nperhaps you're not able to test and validate this.\n\nWhat needs to be tested further?\n\nI'd rather see this tested and set correctly with the series than being\nincorrectly introduced, and having to fix up later, as IPU3 is one of\nour key targets.\n\n\n\n> +\t\t */\n> +\t\tdata->properties_.set(properties::MinNumRequests, 1);\n> +\n> \t\tret = initControls(data.get());\n> \t\tif (ret)\n> \t\t\tcontinue;\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index f821d8fe1b6c..98a97ffca15d 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1046,6 +1046,8 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> \t/* Initialize the camera properties. */\n> \tdata->properties_ = data->sensor_->properties();\n> \n> +\tdata->properties_.set(properties::MinNumRequests, 1);\n> +\n> \t/*\n> \t * Set a default value for the ScalerCropMaximum property to show\n> \t * that we support its use, however, initialise it to zero because\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 42911a8fdfdb..c81ebf14c6bf 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -24,6 +24,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> \n> @@ -944,6 +945,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> \n> \t/* Initialize the camera properties. */\n> \tdata->properties_ = data->sensor_->properties();\n> +\tdata->properties_.set(properties::MinNumRequests, 3);\n> \n> \t/*\n> \t * \\todo Read dealy values from the sensor itself or from a\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index b29fff9820e5..c4adea61519f 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -25,6 +25,7 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> #include \n> \n> @@ -436,6 +437,7 @@ int SimpleCameraData::init()\n> \t}\n> \n> \tproperties_ = sensor_->properties();\n> +\tproperties_.set(properties::MinNumRequests, 3);\n> \n> \treturn 0;\n> }\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 0f634b8da609..0258111ad6cf 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -525,6 +525,8 @@ int UVCCameraData::init(MediaDevice *media)\n> \tproperties_.set(properties::PixelArraySize, resolution);\n> \tproperties_.set(properties::PixelArrayActiveAreas, { Rectangle(resolution) });\n> \n> +\tproperties_.set(properties::MinNumRequests, 1);\n\nIs there any value in initialising this to one in the CameraSensor\nconstructor? Or is it better to make sure every pipeline sets it.\n\nWill lc-complicance have a test to ensure that all pipelines set this?\n\n\n> +\n> \t/* Initialise the supported controls. */\n> \tControlInfoMap::Map ctrls;\n> \n> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> index 12f7517fd0ae..60ec473db0ba 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -21,6 +21,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> \n> @@ -516,6 +517,7 @@ int VimcCameraData::init()\n> \n> \t/* Initialize the camera properties. */\n> \tproperties_ = sensor_->properties();\n> +\tproperties_.set(properties::MinNumRequests, 2);\n> \n> \treturn 0;\n> }\n> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> index 12ecbce5eed4..0208f92074d3 100644\n> --- a/src/libcamera/property_ids.yaml\n> +++ b/src/libcamera/property_ids.yaml\n> @@ -678,6 +678,14 @@ controls:\n> \\todo Turn this property into a \"maximum control value\" for the\n> ScalerCrop control once \"dynamic\" controls have been implemented.\n> \n> + - MinNumRequests:\n\nAs it's v6, perhaps this has been discussed/(bikeshedded) already - but\nMinNumRequests grates on me a little\n\nWe have property names such as\n - ScalerCropMaximum:\n - ColorFilterArrangement:\n\nso I see full words being used (Particularly looking at the 'Maximum').\n\nThat would suggest to me:\n\n - MinimumNumberRequests\n\nwhich seems too verbose, so would then push me to:\n\n - MinimumRequests\n\n\n\n> + type: int32_t\n> + description: |\n> + The bare minimum number of requests needed in the camera pipeline for\n> + capture to be possible, as required by the driver. Note that this\n> + quantity does not guarantee that frames won't be dropped or that manual\n> + per-frame controls will be applied properly.\n> +\n> # ----------------------------------------------------------------------------\n> # Draft properties section\n> \n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id ACE90C3225\n\tfor ;\n\tThu, 15 Jul 2021 09:46:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EEB5B68528;\n\tThu, 15 Jul 2021 11:46:00 +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 DAAB260281\n\tfor ;\n\tThu, 15 Jul 2021 11:45:58 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 43D68340;\n\tThu, 15 Jul 2021 11:45:58 +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=\"PHnvgmY1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626342358;\n\tbh=VkCN0ZQNIzgiEz/QA3lIGkrf0iYnVLXwYKHkYplEQDU=;\n\th=To:Cc:References:From:Subject:Date:In-Reply-To:From;\n\tb=PHnvgmY1YSmYXYaKtPjZ2Bpn9suDHoVkdKHy8RCv96e2NgBeU9RdTGVk7eZUKW7Xo\n\tNMyYiW/VZeAMWXQ0CEZZ6dkRg3SUcSj+eK22pBJSNzxbGw5Z2lywW0UgkF9UqIyvUN\n\t+ZQmNSiDBE9Ex2PBVuFLTzgEPKCVj2Is0ywG1Lkk=","To":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4gUHJhZG8=?= ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210714183857.2046425-1-nfraprado@collabora.com>\n\t<20210714183857.2046425-2-nfraprado@collabora.com>","From":"Kieran Bingham ","Message-ID":"<5c7a5506-11b3-ef74-384b-b99be14b0da7@ideasonboard.com>","Date":"Thu, 15 Jul 2021 10:45:56 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210714183857.2046425-2-nfraprado@collabora.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v6 01/10] libcamera: property: Add\n\tMinNumRequests property","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","Cc":"kernel@collabora.com, =?utf-8?q?Andr=C3=A9_Almeida?=\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18196,"web_url":"https://patchwork.libcamera.org/comment/18196/","msgid":"<20210715145250.xensy6lnhcwwbujm@notapiano>","date":"2021-07-15T14:52:50","subject":"Re: [libcamera-devel] [PATCH v6 01/10] libcamera: property: Add\n\tMinNumRequests property","submitter":{"id":84,"url":"https://patchwork.libcamera.org/api/people/84/","name":"Nícolas F. R. A. Prado","email":"nfraprado@collabora.com"},"content":"Hi Kieran,\n\nOn Thu, Jul 15, 2021 at 10:45:56AM +0100, Kieran Bingham wrote:\n> Hi Nicolas,\n> \n> On 14/07/2021 19:38, Nícolas F. R. A. Prado wrote:\n> > The MinNumRequests property reports the bare minimum number of requests\n> > needed in the camera pipeline for capture to be possible. It is equal to\n> > the number of buffers required by the driver. At this quantity, there's\n> > no guarantee that frames won't be dropped or that manual per-frame\n> > controls will apply correctly. The quantity needed for those may be\n> > added as separate properties in the future.\n> > \n> > Signed-off-by: Nícolas F. R. A. Prado \n> > ---\n> > \n> > Changes in v6:\n> > - Thanks to Naushir:\n> > - Removed comment from Raspberrypi MinNumRequests setting\n> > - Removed an extra blank line\n> > \n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 ++++++\n> > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 2 ++\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 ++\n> > src/libcamera/pipeline/simple/simple.cpp | 2 ++\n> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 ++\n> > src/libcamera/pipeline/vimc/vimc.cpp | 2 ++\n> > src/libcamera/property_ids.yaml | 8 ++++++++\n> > 7 files changed, 24 insertions(+)\n> > \n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 76c3bb3d8aa9..017018c845fa 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -1111,6 +1111,12 @@ int PipelineHandlerIPU3::registerCameras()\n> > \t\t/* Initialize the camera properties. */\n> > \t\tdata->properties_ = cio2->sensor()->properties();\n> > \n> > +\t\t/*\n> > +\t\t * \\todo Make sure this is correct even after the stream is\n> > +\t\t * configured to CIO2\n> \n> What does this comment mean?\n> \n> I see that you don't have a reference to IPU3 in your cover letter, so\n> perhaps you're not able to test and validate this.\n> \n> What needs to be tested further?\n> \n> I'd rather see this tested and set correctly with the series than being\n> incorrectly introduced, and having to fix up later, as IPU3 is one of\n> our key targets.\n\nI'm just not sure that the IPU3 driver can work with a single request in all\nStreamConfigurations (maybe different pipeline topologies require different\nnumber of requests?).\n\nAs you noted I don't have the hardware to test. So if you could, testing this\nwould be just a matter of running lc-compliance with the new Overflow test:\n\n\tlc-compliance -f '*Overflow*'\n\nIf it passes in all StreamConfigurations then we're good and I can remove that\ncomment :).\n\n> \n> \n> \n> > +\t\t */\n> > +\t\tdata->properties_.set(properties::MinNumRequests, 1);\n> > +\n> > \t\tret = initControls(data.get());\n> > \t\tif (ret)\n> > \t\t\tcontinue;\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index f821d8fe1b6c..98a97ffca15d 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -1046,6 +1046,8 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > \t/* Initialize the camera properties. */\n> > \tdata->properties_ = data->sensor_->properties();\n> > \n> > +\tdata->properties_.set(properties::MinNumRequests, 1);\n> > +\n> > \t/*\n> > \t * Set a default value for the ScalerCropMaximum property to show\n> > \t * that we support its use, however, initialise it to zero because\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 42911a8fdfdb..c81ebf14c6bf 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -24,6 +24,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > \n> > @@ -944,6 +945,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > \n> > \t/* Initialize the camera properties. */\n> > \tdata->properties_ = data->sensor_->properties();\n> > +\tdata->properties_.set(properties::MinNumRequests, 3);\n> > \n> > \t/*\n> > \t * \\todo Read dealy values from the sensor itself or from a\n> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > index b29fff9820e5..c4adea61519f 100644\n> > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > @@ -25,6 +25,7 @@\n> > \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > \n> > @@ -436,6 +437,7 @@ int SimpleCameraData::init()\n> > \t}\n> > \n> > \tproperties_ = sensor_->properties();\n> > +\tproperties_.set(properties::MinNumRequests, 3);\n> > \n> > \treturn 0;\n> > }\n> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > index 0f634b8da609..0258111ad6cf 100644\n> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > @@ -525,6 +525,8 @@ int UVCCameraData::init(MediaDevice *media)\n> > \tproperties_.set(properties::PixelArraySize, resolution);\n> > \tproperties_.set(properties::PixelArrayActiveAreas, { Rectangle(resolution) });\n> > \n> > +\tproperties_.set(properties::MinNumRequests, 1);\n> \n> Is there any value in initialising this to one in the CameraSensor\n> constructor? Or is it better to make sure every pipeline sets it.\n\nI think it would make sense to have this set to 1 by default in the CameraSensor\nconstructor, and any pipeline that has a different value can override it.\n\nOne issue though is that the uvcvideo pipeline doesn't have a CameraSensor to\ninitialize its properties from. So we'd still need to manually set it in that\ncase.\n\n> \n> Will lc-complicance have a test to ensure that all pipelines set this?\n\nI hadn't created such a test. If we don't go that route of setting it by default\nI'll create it.\n\n> \n> \n> > +\n> > \t/* Initialise the supported controls. */\n> > \tControlInfoMap::Map ctrls;\n> > \n> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> > index 12f7517fd0ae..60ec473db0ba 100644\n> > --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> > @@ -21,6 +21,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > \n> > @@ -516,6 +517,7 @@ int VimcCameraData::init()\n> > \n> > \t/* Initialize the camera properties. */\n> > \tproperties_ = sensor_->properties();\n> > +\tproperties_.set(properties::MinNumRequests, 2);\n> > \n> > \treturn 0;\n> > }\n> > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> > index 12ecbce5eed4..0208f92074d3 100644\n> > --- a/src/libcamera/property_ids.yaml\n> > +++ b/src/libcamera/property_ids.yaml\n> > @@ -678,6 +678,14 @@ controls:\n> > \\todo Turn this property into a \"maximum control value\" for the\n> > ScalerCrop control once \"dynamic\" controls have been implemented.\n> > \n> > + - MinNumRequests:\n> \n> As it's v6, perhaps this has been discussed/(bikeshedded) already - but\n> MinNumRequests grates on me a little\n> \n> We have property names such as\n> - ScalerCropMaximum:\n> - ColorFilterArrangement:\n> \n> so I see full words being used (Particularly looking at the 'Maximum').\n> \n> That would suggest to me:\n> \n> - MinimumNumberRequests\n> \n> which seems too verbose, so would then push me to:\n> \n> - MinimumRequests\n\nThat sounds like a better name :)\n\nThanks,\nNícolas\n\n> \n> \n> \n> > + type: int32_t\n> > + description: |\n> > + The bare minimum number of requests needed in the camera pipeline for\n> > + capture to be possible, as required by the driver. Note that this\n> > + quantity does not guarantee that frames won't be dropped or that manual\n> > + per-frame controls will be applied properly.\n> > +\n> > # ----------------------------------------------------------------------------\n> > # Draft properties section\n> > \n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4E783C3226\n\tfor ;\n\tThu, 15 Jul 2021 14:52:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 264996853B;\n\tThu, 15 Jul 2021 16:52:57 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk [46.235.227.227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 96ECE6059F\n\tfor ;\n\tThu, 15 Jul 2021 16:52:55 +0200 (CEST)","from notapiano (unknown\n\t[IPv6:2804:14c:1a9:2434:4535:6bb0:bc92:d82])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\t(Authenticated sender: nfraprado)\n\tby bhuna.collabora.co.uk (Postfix) with ESMTPSA id 40C991F4354B;\n\tThu, 15 Jul 2021 15:52:54 +0100 (BST)"],"Date":"Thu, 15 Jul 2021 11:52:50 -0300","From":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado ","To":"Kieran Bingham ","Message-ID":"<20210715145250.xensy6lnhcwwbujm@notapiano>","References":"<20210714183857.2046425-1-nfraprado@collabora.com>\n\t<20210714183857.2046425-2-nfraprado@collabora.com>\n\t<5c7a5506-11b3-ef74-384b-b99be14b0da7@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<5c7a5506-11b3-ef74-384b-b99be14b0da7@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v6 01/10] libcamera: property: Add\n\tMinNumRequests property","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","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?b?bmRyw6k=?= Almeida ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18197,"web_url":"https://patchwork.libcamera.org/comment/18197/","msgid":"<20210715153050.h7hcynsu77irsztr@uno.localdomain>","date":"2021-07-15T15:30:50","subject":"Re: [libcamera-devel] [PATCH v6 01/10] libcamera: property: Add\n\tMinNumRequests property","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Helo,\n\nOn Thu, Jul 15, 2021 at 11:52:50AM -0300, Nícolas F. R. A. Prado wrote:\n> Hi Kieran,\n>\n> On Thu, Jul 15, 2021 at 10:45:56AM +0100, Kieran Bingham wrote:\n> > Hi Nicolas,\n> >\n> > On 14/07/2021 19:38, Nícolas F. R. A. Prado wrote:\n> > > The MinNumRequests property reports the bare minimum number of requests\n> > > needed in the camera pipeline for capture to be possible. It is equal to\n> > > the number of buffers required by the driver. At this quantity, there's\n> > > no guarantee that frames won't be dropped or that manual per-frame\n> > > controls will apply correctly. The quantity needed for those may be\n> > > added as separate properties in the future.\n> > >\n> > > Signed-off-by: Nícolas F. R. A. Prado \n> > > ---\n> > >\n> > > Changes in v6:\n> > > - Thanks to Naushir:\n> > > - Removed comment from Raspberrypi MinNumRequests setting\n> > > - Removed an extra blank line\n> > >\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 ++++++\n> > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 2 ++\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 ++\n> > > src/libcamera/pipeline/simple/simple.cpp | 2 ++\n> > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 ++\n> > > src/libcamera/pipeline/vimc/vimc.cpp | 2 ++\n> > > src/libcamera/property_ids.yaml | 8 ++++++++\n> > > 7 files changed, 24 insertions(+)\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 76c3bb3d8aa9..017018c845fa 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -1111,6 +1111,12 @@ int PipelineHandlerIPU3::registerCameras()\n> > > \t\t/* Initialize the camera properties. */\n> > > \t\tdata->properties_ = cio2->sensor()->properties();\n> > >\n> > > +\t\t/*\n> > > +\t\t * \\todo Make sure this is correct even after the stream is\n> > > +\t\t * configured to CIO2\n> >\n> > What does this comment mean?\n> >\n> > I see that you don't have a reference to IPU3 in your cover letter, so\n> > perhaps you're not able to test and validate this.\n> >\n> > What needs to be tested further?\n> >\n> > I'd rather see this tested and set correctly with the series than being\n> > incorrectly introduced, and having to fix up later, as IPU3 is one of\n> > our key targets.\n>\n> I'm just not sure that the IPU3 driver can work with a single request in all\n> StreamConfigurations (maybe different pipeline topologies require different\n> number of requests?).\n>\n> As you noted I don't have the hardware to test. So if you could, testing this\n> would be just a matter of running lc-compliance with the new Overflow test:\n>\n> \tlc-compliance -f '*Overflow*'\n>\n> If it passes in all StreamConfigurations then we're good and I can remove that\n> comment :).\n>\n> >\n> >\n> >\n> > > +\t\t */\n> > > +\t\tdata->properties_.set(properties::MinNumRequests, 1);\n> > > +\n> > > \t\tret = initControls(data.get());\n> > > \t\tif (ret)\n> > > \t\t\tcontinue;\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index f821d8fe1b6c..98a97ffca15d 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -1046,6 +1046,8 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > > \t/* Initialize the camera properties. */\n> > > \tdata->properties_ = data->sensor_->properties();\n> > >\n> > > +\tdata->properties_.set(properties::MinNumRequests, 1);\n> > > +\n> > > \t/*\n> > > \t * Set a default value for the ScalerCropMaximum property to show\n> > > \t * that we support its use, however, initialise it to zero because\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index 42911a8fdfdb..c81ebf14c6bf 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -24,6 +24,7 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > >\n> > > @@ -944,6 +945,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > >\n> > > \t/* Initialize the camera properties. */\n> > > \tdata->properties_ = data->sensor_->properties();\n> > > +\tdata->properties_.set(properties::MinNumRequests, 3);\n> > >\n> > > \t/*\n> > > \t * \\todo Read dealy values from the sensor itself or from a\n> > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > > index b29fff9820e5..c4adea61519f 100644\n> > > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > > @@ -25,6 +25,7 @@\n> > >\n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > >\n> > > @@ -436,6 +437,7 @@ int SimpleCameraData::init()\n> > > \t}\n> > >\n> > > \tproperties_ = sensor_->properties();\n> > > +\tproperties_.set(properties::MinNumRequests, 3);\n> > >\n> > > \treturn 0;\n> > > }\n> > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > index 0f634b8da609..0258111ad6cf 100644\n> > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > @@ -525,6 +525,8 @@ int UVCCameraData::init(MediaDevice *media)\n> > > \tproperties_.set(properties::PixelArraySize, resolution);\n> > > \tproperties_.set(properties::PixelArrayActiveAreas, { Rectangle(resolution) });\n> > >\n> > > +\tproperties_.set(properties::MinNumRequests, 1);\n> >\n> > Is there any value in initialising this to one in the CameraSensor\n> > constructor? Or is it better to make sure every pipeline sets it.\n>\n> I think it would make sense to have this set to 1 by default in the CameraSensor\n> constructor, and any pipeline that has a different value can override it.\n>\n> One issue though is that the uvcvideo pipeline doesn't have a CameraSensor to\n> initialize its properties from. So we'd still need to manually set it in that\n> case.\n>\n> >\n> > Will lc-complicance have a test to ensure that all pipelines set this?\n>\n> I hadn't created such a test. If we don't go that route of setting it by default\n> I'll create it.\n>\n> >\n> >\n> > > +\n> > > \t/* Initialise the supported controls. */\n> > > \tControlInfoMap::Map ctrls;\n> > >\n> > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> > > index 12f7517fd0ae..60ec473db0ba 100644\n> > > --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> > > @@ -21,6 +21,7 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > >\n> > > @@ -516,6 +517,7 @@ int VimcCameraData::init()\n> > >\n> > > \t/* Initialize the camera properties. */\n> > > \tproperties_ = sensor_->properties();\n> > > +\tproperties_.set(properties::MinNumRequests, 2);\n> > >\n> > > \treturn 0;\n> > > }\n> > > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> > > index 12ecbce5eed4..0208f92074d3 100644\n> > > --- a/src/libcamera/property_ids.yaml\n> > > +++ b/src/libcamera/property_ids.yaml\n> > > @@ -678,6 +678,14 @@ controls:\n> > > \\todo Turn this property into a \"maximum control value\" for the\n> > > ScalerCrop control once \"dynamic\" controls have been implemented.\n> > >\n> > > + - MinNumRequests:\n> >\n> > As it's v6, perhaps this has been discussed/(bikeshedded) already - but\n> > MinNumRequests grates on me a little\n> >\n> > We have property names such as\n> > - ScalerCropMaximum:\n> > - ColorFilterArrangement:\n> >\n> > so I see full words being used (Particularly looking at the 'Maximum').\n> >\n> > That would suggest to me:\n> >\n> > - MinimumNumberRequests\n> >\n> > which seems too verbose, so would then push me to:\n> >\n> > - MinimumRequests\n>\n> That sounds like a better name :)\n>\n> Thanks,\n> Nícolas\n>\n> >\n> >\n> >\n> > > + type: int32_t\n> > > + description: |\n> > > + The bare minimum number of requests needed in the camera pipeline for\n> > > + capture to be possible, as required by the driver. Note that this\n> > > + quantity does not guarantee that frames won't be dropped or that manual\n> > > + per-frame controls will be applied properly.\n\nI would generalize it a bit and leave per-frame control out, unless\nI've missed the implications of this property in relation to it.\n\nWhat about something like\n\n The minimum number of capture requests that the camera\n pipeline requires to have queued to sustain the\n capture operations.\n\n The requested number of queued requests usually\n corresponds to the minimum number of memory buffers\n required to satisfy the requirements of the hardware\n processing blocks that compose the capture pipeline\n and represents the minimum depth of the capture\n request queue.\n\nSuggestion or corrections welcome, of course.\n\n> > > +\n> > > # ----------------------------------------------------------------------------\n> > > # Draft properties section\n> > >\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 12420C3225\n\tfor ;\n\tThu, 15 Jul 2021 15:30:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 760A268539;\n\tThu, 15 Jul 2021 17:30:08 +0200 (CEST)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 273726059F\n\tfor ;\n\tThu, 15 Jul 2021 17:30:07 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 56862C000F;\n\tThu, 15 Jul 2021 15:30:04 +0000 (UTC)"],"Date":"Thu, 15 Jul 2021 17:30:50 +0200","From":"Jacopo Mondi ","To":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado ","Message-ID":"<20210715153050.h7hcynsu77irsztr@uno.localdomain>","References":"<20210714183857.2046425-1-nfraprado@collabora.com>\n\t<20210714183857.2046425-2-nfraprado@collabora.com>\n\t<5c7a5506-11b3-ef74-384b-b99be14b0da7@ideasonboard.com>\n\t<20210715145250.xensy6lnhcwwbujm@notapiano>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210715145250.xensy6lnhcwwbujm@notapiano>","Subject":"Re: [libcamera-devel] [PATCH v6 01/10] libcamera: property: Add\n\tMinNumRequests property","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","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?b?bmRyw6k=?= Almeida ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18198,"web_url":"https://patchwork.libcamera.org/comment/18198/","msgid":"<9e0cff16-2783-8894-d6e6-d4a1f89ef950@ideasonboard.com>","date":"2021-07-15T16:45:58","subject":"Re: [libcamera-devel] [PATCH v6 01/10] libcamera: property: Add\n\tMinNumRequests property","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Nicolas,\n\nOn 15/07/2021 15:52, Nícolas F. R. A. Prado wrote:\n> Hi Kieran,\n> \n> On Thu, Jul 15, 2021 at 10:45:56AM +0100, Kieran Bingham wrote:\n>> Hi Nicolas,\n>>\n>> On 14/07/2021 19:38, Nícolas F. R. A. Prado wrote:\n>>> The MinNumRequests property reports the bare minimum number of requests\n>>> needed in the camera pipeline for capture to be possible. It is equal to\n>>> the number of buffers required by the driver. At this quantity, there's\n>>> no guarantee that frames won't be dropped or that manual per-frame\n>>> controls will apply correctly. The quantity needed for those may be\n>>> added as separate properties in the future.\n>>>\n>>> Signed-off-by: Nícolas F. R. A. Prado \n>>> ---\n>>>\n>>> Changes in v6:\n>>> - Thanks to Naushir:\n>>> - Removed comment from Raspberrypi MinNumRequests setting\n>>> - Removed an extra blank line\n>>>\n>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 6 ++++++\n>>> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 2 ++\n>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 ++\n>>> src/libcamera/pipeline/simple/simple.cpp | 2 ++\n>>> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 ++\n>>> src/libcamera/pipeline/vimc/vimc.cpp | 2 ++\n>>> src/libcamera/property_ids.yaml | 8 ++++++++\n>>> 7 files changed, 24 insertions(+)\n>>>\n>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> index 76c3bb3d8aa9..017018c845fa 100644\n>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> @@ -1111,6 +1111,12 @@ int PipelineHandlerIPU3::registerCameras()\n>>> \t\t/* Initialize the camera properties. */\n>>> \t\tdata->properties_ = cio2->sensor()->properties();\n>>> \n>>> +\t\t/*\n>>> +\t\t * \\todo Make sure this is correct even after the stream is\n>>> +\t\t * configured to CIO2\n>>\n>> What does this comment mean?\n>>\n>> I see that you don't have a reference to IPU3 in your cover letter, so\n>> perhaps you're not able to test and validate this.\n>>\n>> What needs to be tested further?\n>>\n>> I'd rather see this tested and set correctly with the series than being\n>> incorrectly introduced, and having to fix up later, as IPU3 is one of\n>> our key targets.\n> \n> I'm just not sure that the IPU3 driver can work with a single request in all\n> StreamConfigurations (maybe different pipeline topologies require different\n> number of requests?).\n> \n> As you noted I don't have the hardware to test. So if you could, testing this\n> would be just a matter of running lc-compliance with the new Overflow test:\n> \n> \tlc-compliance -f '*Overflow*'\n> \n\nUsing camera \\_SB_.PCI0.I2C2.CAM0\nNote: Google Test filter = *Overflow*\n[==========] Running 4 tests from 1 test suite.\n[----------] Global test environment set-up.\n[----------] 4 tests from CaptureTests/RoleParametrizedTest\n[ RUN ] CaptureTests/RoleParametrizedTest.Overflow/Raw\n[2:17:21.161407453] [25870] INFO Camera camera.cpp:906 configuring\nstreams: (0) 4224x3136-SGRBG10_IPU3\n[2:17:21.161682517] [25871] ERROR V4L2 v4l2_subdevice.cpp:286 'ov13858\n8-0010': Unable to get rectangle 0 on pad 0: Inappropriate ioctl for device\n[2:17:21.161703674] [25871] WARN CameraSensor camera_sensor.cpp:737\n'ov13858 8-0010': The analogue crop rectangle has been defaulted to the\nactive area size\n\n\n\nBut I'm not sure if that's this overflow tests fault, as I'm not\nconvinced it runs without your series yet anyway, as it looks like the\ntests are failing due to incomplete Raw stream support perhaps ...\n\n\n\n\n> If it passes in all StreamConfigurations then we're good and I can remove that\n> comment :).\n> \n>>\n>>\n>>\n>>> +\t\t */\n>>> +\t\tdata->properties_.set(properties::MinNumRequests, 1);\n>>> +\n>>> \t\tret = initControls(data.get());\n>>> \t\tif (ret)\n>>> \t\t\tcontinue;\n>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>> index f821d8fe1b6c..98a97ffca15d 100644\n>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>> @@ -1046,6 +1046,8 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n>>> \t/* Initialize the camera properties. */\n>>> \tdata->properties_ = data->sensor_->properties();\n>>> \n>>> +\tdata->properties_.set(properties::MinNumRequests, 1);\n>>> +\n>>> \t/*\n>>> \t * Set a default value for the ScalerCropMaximum property to show\n>>> \t * that we support its use, however, initialise it to zero because\n>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>> index 42911a8fdfdb..c81ebf14c6bf 100644\n>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>> @@ -24,6 +24,7 @@\n>>> #include \n>>> #include \n>>> #include \n>>> +#include \n>>> #include \n>>> #include \n>>> \n>>> @@ -944,6 +945,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n>>> \n>>> \t/* Initialize the camera properties. */\n>>> \tdata->properties_ = data->sensor_->properties();\n>>> +\tdata->properties_.set(properties::MinNumRequests, 3);\n>>> \n>>> \t/*\n>>> \t * \\todo Read dealy values from the sensor itself or from a\n>>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n>>> index b29fff9820e5..c4adea61519f 100644\n>>> --- a/src/libcamera/pipeline/simple/simple.cpp\n>>> +++ b/src/libcamera/pipeline/simple/simple.cpp\n>>> @@ -25,6 +25,7 @@\n>>> \n>>> #include \n>>> #include \n>>> +#include \n>>> #include \n>>> #include \n>>> \n>>> @@ -436,6 +437,7 @@ int SimpleCameraData::init()\n>>> \t}\n>>> \n>>> \tproperties_ = sensor_->properties();\n>>> +\tproperties_.set(properties::MinNumRequests, 3);\n>>> \n>>> \treturn 0;\n>>> }\n>>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>>> index 0f634b8da609..0258111ad6cf 100644\n>>> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>>> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>>> @@ -525,6 +525,8 @@ int UVCCameraData::init(MediaDevice *media)\n>>> \tproperties_.set(properties::PixelArraySize, resolution);\n>>> \tproperties_.set(properties::PixelArrayActiveAreas, { Rectangle(resolution) });\n>>> \n>>> +\tproperties_.set(properties::MinNumRequests, 1);\n>>\n>> Is there any value in initialising this to one in the CameraSensor\n>> constructor? Or is it better to make sure every pipeline sets it.\n> \n> I think it would make sense to have this set to 1 by default in the CameraSensor\n> constructor, and any pipeline that has a different value can override it.\n> \n> One issue though is that the uvcvideo pipeline doesn't have a CameraSensor to\n> initialize its properties from. So we'd still need to manually set it in that\n> case.\n> \n>>\n>> Will lc-complicance have a test to ensure that all pipelines set this?\n> \n> I hadn't created such a test. If we don't go that route of setting it by default\n> I'll create it.\n> \n>>\n>>\n>>> +\n>>> \t/* Initialise the supported controls. */\n>>> \tControlInfoMap::Map ctrls;\n>>> \n>>> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n>>> index 12f7517fd0ae..60ec473db0ba 100644\n>>> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n>>> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n>>> @@ -21,6 +21,7 @@\n>>> #include \n>>> #include \n>>> #include \n>>> +#include \n>>> #include \n>>> #include \n>>> \n>>> @@ -516,6 +517,7 @@ int VimcCameraData::init()\n>>> \n>>> \t/* Initialize the camera properties. */\n>>> \tproperties_ = sensor_->properties();\n>>> +\tproperties_.set(properties::MinNumRequests, 2);\n>>> \n>>> \treturn 0;\n>>> }\n>>> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n>>> index 12ecbce5eed4..0208f92074d3 100644\n>>> --- a/src/libcamera/property_ids.yaml\n>>> +++ b/src/libcamera/property_ids.yaml\n>>> @@ -678,6 +678,14 @@ controls:\n>>> \\todo Turn this property into a \"maximum control value\" for the\n>>> ScalerCrop control once \"dynamic\" controls have been implemented.\n>>> \n>>> + - MinNumRequests:\n>>\n>> As it's v6, perhaps this has been discussed/(bikeshedded) already - but\n>> MinNumRequests grates on me a little\n>>\n>> We have property names such as\n>> - ScalerCropMaximum:\n>> - ColorFilterArrangement:\n>>\n>> so I see full words being used (Particularly looking at the 'Maximum').\n>>\n>> That would suggest to me:\n>>\n>> - MinimumNumberRequests\n>>\n>> which seems too verbose, so would then push me to:\n>>\n>> - MinimumRequests\n> \n> That sounds like a better name :)\n> \n> Thanks,\n> Nícolas\n> \n>>\n>>\n>>\n>>> + type: int32_t\n>>> + description: |\n>>> + The bare minimum number of requests needed in the camera pipeline for\n>>> + capture to be possible, as required by the driver. Note that this\n>>> + quantity does not guarantee that frames won't be dropped or that manual\n>>> + per-frame controls will be applied properly.\n>>> +\n>>> # ----------------------------------------------------------------------------\n>>> # Draft properties section\n>>> \n>>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9742FC3225\n\tfor ;\n\tThu, 15 Jul 2021 16:46:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 17FE968539;\n\tThu, 15 Jul 2021 18:46:03 +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 CDC6A6059F\n\tfor ;\n\tThu, 15 Jul 2021 18:46:01 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3972C340;\n\tThu, 15 Jul 2021 18:46:01 +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=\"FW0E64/l\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626367561;\n\tbh=tUfm1kI316TPKzA1973M9DEEeWWKfeIK352VJc6muW0=;\n\th=From:Subject:To:Cc:References:Date:In-Reply-To:From;\n\tb=FW0E64/lO33N3tzjI3fNbHofxLTbE3FDVXRb5dLwAypqv6TD5+ubOfi6aX2jsBsGD\n\tdz4ZUSw4CJ95jmGkVhYgSero6AJZnOUky82iRCGmzp5QufTtotSwja7XzPNeHTzONp\n\tdfA3yo1nnlhgNPDe1QvjJdtaa+hOId4l+OOYaDV4=","From":"Kieran Bingham ","To":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4gUHJhZG8=?= ","References":"<20210714183857.2046425-1-nfraprado@collabora.com>\n\t<20210714183857.2046425-2-nfraprado@collabora.com>\n\t<5c7a5506-11b3-ef74-384b-b99be14b0da7@ideasonboard.com>\n\t<20210715145250.xensy6lnhcwwbujm@notapiano>","Message-ID":"<9e0cff16-2783-8894-d6e6-d4a1f89ef950@ideasonboard.com>","Date":"Thu, 15 Jul 2021 17:45:58 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210715145250.xensy6lnhcwwbujm@notapiano>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v6 01/10] libcamera: property: Add\n\tMinNumRequests property","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","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?q?ndr=C3=A9_Almeida?= ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18199,"web_url":"https://patchwork.libcamera.org/comment/18199/","msgid":"<20210715171658.mx6mgeemfng5nqir@notapiano>","date":"2021-07-15T17:16:58","subject":"Re: [libcamera-devel] [PATCH v6 01/10] libcamera: property: Add\n\tMinNumRequests property","submitter":{"id":84,"url":"https://patchwork.libcamera.org/api/people/84/","name":"Nícolas F. R. A. Prado","email":"nfraprado@collabora.com"},"content":"Hi Kieran,\n\nOn Thu, Jul 15, 2021 at 05:45:58PM +0100, Kieran Bingham wrote:\n> Hi Nicolas,\n> \n> On 15/07/2021 15:52, Nícolas F. R. A. Prado wrote:\n> > Hi Kieran,\n> > \n> > On Thu, Jul 15, 2021 at 10:45:56AM +0100, Kieran Bingham wrote:\n> >> Hi Nicolas,\n> >>\n> >> On 14/07/2021 19:38, Nícolas F. R. A. Prado wrote:\n> >>> The MinNumRequests property reports the bare minimum number of requests\n> >>> needed in the camera pipeline for capture to be possible. It is equal to\n> >>> the number of buffers required by the driver. At this quantity, there's\n> >>> no guarantee that frames won't be dropped or that manual per-frame\n> >>> controls will apply correctly. The quantity needed for those may be\n> >>> added as separate properties in the future.\n> >>>\n> >>> Signed-off-by: Nícolas F. R. A. Prado \n> >>> ---\n> >>>\n> >>> Changes in v6:\n> >>> - Thanks to Naushir:\n> >>> - Removed comment from Raspberrypi MinNumRequests setting\n> >>> - Removed an extra blank line\n> >>>\n> >>> src/libcamera/pipeline/ipu3/ipu3.cpp | 6 ++++++\n> >>> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 2 ++\n> >>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 ++\n> >>> src/libcamera/pipeline/simple/simple.cpp | 2 ++\n> >>> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 ++\n> >>> src/libcamera/pipeline/vimc/vimc.cpp | 2 ++\n> >>> src/libcamera/property_ids.yaml | 8 ++++++++\n> >>> 7 files changed, 24 insertions(+)\n> >>>\n> >>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>> index 76c3bb3d8aa9..017018c845fa 100644\n> >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> >>> @@ -1111,6 +1111,12 @@ int PipelineHandlerIPU3::registerCameras()\n> >>> \t\t/* Initialize the camera properties. */\n> >>> \t\tdata->properties_ = cio2->sensor()->properties();\n> >>> \n> >>> +\t\t/*\n> >>> +\t\t * \\todo Make sure this is correct even after the stream is\n> >>> +\t\t * configured to CIO2\n> >>\n> >> What does this comment mean?\n> >>\n> >> I see that you don't have a reference to IPU3 in your cover letter, so\n> >> perhaps you're not able to test and validate this.\n> >>\n> >> What needs to be tested further?\n> >>\n> >> I'd rather see this tested and set correctly with the series than being\n> >> incorrectly introduced, and having to fix up later, as IPU3 is one of\n> >> our key targets.\n> > \n> > I'm just not sure that the IPU3 driver can work with a single request in all\n> > StreamConfigurations (maybe different pipeline topologies require different\n> > number of requests?).\n> > \n> > As you noted I don't have the hardware to test. So if you could, testing this\n> > would be just a matter of running lc-compliance with the new Overflow test:\n> > \n> > \tlc-compliance -f '*Overflow*'\n> > \n> \n> Using camera \\_SB_.PCI0.I2C2.CAM0\n> Note: Google Test filter = *Overflow*\n> [==========] Running 4 tests from 1 test suite.\n> [----------] Global test environment set-up.\n> [----------] 4 tests from CaptureTests/RoleParametrizedTest\n> [ RUN ] CaptureTests/RoleParametrizedTest.Overflow/Raw\n> [2:17:21.161407453] [25870] INFO Camera camera.cpp:906 configuring\n> streams: (0) 4224x3136-SGRBG10_IPU3\n> [2:17:21.161682517] [25871] ERROR V4L2 v4l2_subdevice.cpp:286 'ov13858\n> 8-0010': Unable to get rectangle 0 on pad 0: Inappropriate ioctl for device\n> [2:17:21.161703674] [25871] WARN CameraSensor camera_sensor.cpp:737\n> 'ov13858 8-0010': The analogue crop rectangle has been defaulted to the\n> active area size\n> \n> \n> \n> But I'm not sure if that's this overflow tests fault, as I'm not\n> convinced it runs without your series yet anyway, as it looks like the\n> tests are failing due to incomplete Raw stream support perhaps ...\n\nHmm. Well, you could try running one of the current raw tests, eg\n\n\tlc-compliance -f '*Capture/Raw_5'\n\nand also a test with a different role\n\n\tlc-compliance -f '*Capture/StillCapture_5'\n\nto find out if it's the pipeline or the Overflow test's fault.\n\nYou could also run the Overflow test for all roles but Raw with\n\n\tlc-compliance -f '*Overflow*:-*Raw*'\n\nalthough even if all pass, we might still be missing an issue with this test on\nthe raw streamrole...\n\nThanks,\nNícolas\n\n> \n> \n> \n> \n> > If it passes in all StreamConfigurations then we're good and I can remove that\n> > comment :).\n> > \n> >>\n> >>\n> >>\n> >>> +\t\t */\n> >>> +\t\tdata->properties_.set(properties::MinNumRequests, 1);\n> >>> +\n> >>> \t\tret = initControls(data.get());\n> >>> \t\tif (ret)\n> >>> \t\t\tcontinue;\n> >>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>> index f821d8fe1b6c..98a97ffca15d 100644\n> >>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >>> @@ -1046,6 +1046,8 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> >>> \t/* Initialize the camera properties. */\n> >>> \tdata->properties_ = data->sensor_->properties();\n> >>> \n> >>> +\tdata->properties_.set(properties::MinNumRequests, 1);\n> >>> +\n> >>> \t/*\n> >>> \t * Set a default value for the ScalerCropMaximum property to show\n> >>> \t * that we support its use, however, initialise it to zero because\n> >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>> index 42911a8fdfdb..c81ebf14c6bf 100644\n> >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >>> @@ -24,6 +24,7 @@\n> >>> #include \n> >>> #include \n> >>> #include \n> >>> +#include \n> >>> #include \n> >>> #include \n> >>> \n> >>> @@ -944,6 +945,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> >>> \n> >>> \t/* Initialize the camera properties. */\n> >>> \tdata->properties_ = data->sensor_->properties();\n> >>> +\tdata->properties_.set(properties::MinNumRequests, 3);\n> >>> \n> >>> \t/*\n> >>> \t * \\todo Read dealy values from the sensor itself or from a\n> >>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> >>> index b29fff9820e5..c4adea61519f 100644\n> >>> --- a/src/libcamera/pipeline/simple/simple.cpp\n> >>> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> >>> @@ -25,6 +25,7 @@\n> >>> \n> >>> #include \n> >>> #include \n> >>> +#include \n> >>> #include \n> >>> #include \n> >>> \n> >>> @@ -436,6 +437,7 @@ int SimpleCameraData::init()\n> >>> \t}\n> >>> \n> >>> \tproperties_ = sensor_->properties();\n> >>> +\tproperties_.set(properties::MinNumRequests, 3);\n> >>> \n> >>> \treturn 0;\n> >>> }\n> >>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> >>> index 0f634b8da609..0258111ad6cf 100644\n> >>> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> >>> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> >>> @@ -525,6 +525,8 @@ int UVCCameraData::init(MediaDevice *media)\n> >>> \tproperties_.set(properties::PixelArraySize, resolution);\n> >>> \tproperties_.set(properties::PixelArrayActiveAreas, { Rectangle(resolution) });\n> >>> \n> >>> +\tproperties_.set(properties::MinNumRequests, 1);\n> >>\n> >> Is there any value in initialising this to one in the CameraSensor\n> >> constructor? Or is it better to make sure every pipeline sets it.\n> > \n> > I think it would make sense to have this set to 1 by default in the CameraSensor\n> > constructor, and any pipeline that has a different value can override it.\n> > \n> > One issue though is that the uvcvideo pipeline doesn't have a CameraSensor to\n> > initialize its properties from. So we'd still need to manually set it in that\n> > case.\n> > \n> >>\n> >> Will lc-complicance have a test to ensure that all pipelines set this?\n> > \n> > I hadn't created such a test. If we don't go that route of setting it by default\n> > I'll create it.\n> > \n> >>\n> >>\n> >>> +\n> >>> \t/* Initialise the supported controls. */\n> >>> \tControlInfoMap::Map ctrls;\n> >>> \n> >>> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> >>> index 12f7517fd0ae..60ec473db0ba 100644\n> >>> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> >>> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> >>> @@ -21,6 +21,7 @@\n> >>> #include \n> >>> #include \n> >>> #include \n> >>> +#include \n> >>> #include \n> >>> #include \n> >>> \n> >>> @@ -516,6 +517,7 @@ int VimcCameraData::init()\n> >>> \n> >>> \t/* Initialize the camera properties. */\n> >>> \tproperties_ = sensor_->properties();\n> >>> +\tproperties_.set(properties::MinNumRequests, 2);\n> >>> \n> >>> \treturn 0;\n> >>> }\n> >>> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> >>> index 12ecbce5eed4..0208f92074d3 100644\n> >>> --- a/src/libcamera/property_ids.yaml\n> >>> +++ b/src/libcamera/property_ids.yaml\n> >>> @@ -678,6 +678,14 @@ controls:\n> >>> \\todo Turn this property into a \"maximum control value\" for the\n> >>> ScalerCrop control once \"dynamic\" controls have been implemented.\n> >>> \n> >>> + - MinNumRequests:\n> >>\n> >> As it's v6, perhaps this has been discussed/(bikeshedded) already - but\n> >> MinNumRequests grates on me a little\n> >>\n> >> We have property names such as\n> >> - ScalerCropMaximum:\n> >> - ColorFilterArrangement:\n> >>\n> >> so I see full words being used (Particularly looking at the 'Maximum').\n> >>\n> >> That would suggest to me:\n> >>\n> >> - MinimumNumberRequests\n> >>\n> >> which seems too verbose, so would then push me to:\n> >>\n> >> - MinimumRequests\n> > \n> > That sounds like a better name :)\n> > \n> > Thanks,\n> > Nícolas\n> > \n> >>\n> >>\n> >>\n> >>> + type: int32_t\n> >>> + description: |\n> >>> + The bare minimum number of requests needed in the camera pipeline for\n> >>> + capture to be possible, as required by the driver. Note that this\n> >>> + quantity does not guarantee that frames won't be dropped or that manual\n> >>> + per-frame controls will be applied properly.\n> >>> +\n> >>> # ----------------------------------------------------------------------------\n> >>> # Draft properties section\n> >>> \n> >>>\n> \n> -- \n> To unsubscribe, send mail to kernel-unsubscribe@lists.collabora.co.uk.","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 2B42AC3226\n\tfor ;\n\tThu, 15 Jul 2021 17:17:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A9D016852A;\n\tThu, 15 Jul 2021 19:17:06 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 117186059F\n\tfor ;\n\tThu, 15 Jul 2021 19:17:05 +0200 (CEST)","from notapiano (unknown\n\t[IPv6:2804:14c:1a9:2434:4535:6bb0:bc92:d82])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\t(Authenticated sender: nfraprado)\n\tby bhuna.collabora.co.uk (Postfix) with ESMTPSA id 755061F43A0C;\n\tThu, 15 Jul 2021 18:17:03 +0100 (BST)"],"Date":"Thu, 15 Jul 2021 14:16:58 -0300","From":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado ","To":"Kieran Bingham ","Message-ID":"<20210715171658.mx6mgeemfng5nqir@notapiano>","References":"<20210714183857.2046425-1-nfraprado@collabora.com>\n\t<20210714183857.2046425-2-nfraprado@collabora.com>\n\t<5c7a5506-11b3-ef74-384b-b99be14b0da7@ideasonboard.com>\n\t<20210715145250.xensy6lnhcwwbujm@notapiano>\n\t<9e0cff16-2783-8894-d6e6-d4a1f89ef950@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<9e0cff16-2783-8894-d6e6-d4a1f89ef950@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v6 01/10] libcamera: property: Add\n\tMinNumRequests property","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","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?b?bmRyw6k=?= Almeida ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18232,"web_url":"https://patchwork.libcamera.org/comment/18232/","msgid":"<20210719140050.obsx34wwyqsp4o76@notapiano>","date":"2021-07-19T14:00:50","subject":"Re: [libcamera-devel] [PATCH v6 01/10] libcamera: property: Add\n\tMinNumRequests property","submitter":{"id":84,"url":"https://patchwork.libcamera.org/api/people/84/","name":"Nícolas F. R. A. Prado","email":"nfraprado@collabora.com"},"content":"Hi Jacopo,\n\nOn Thu, Jul 15, 2021 at 05:30:50PM +0200, Jacopo Mondi wrote:\n> Helo,\n> \n> On Thu, Jul 15, 2021 at 11:52:50AM -0300, Nícolas F. R. A. Prado wrote:\n> > Hi Kieran,\n> >\n> > On Thu, Jul 15, 2021 at 10:45:56AM +0100, Kieran Bingham wrote:\n> > > Hi Nicolas,\n> > >\n> > > On 14/07/2021 19:38, Nícolas F. R. A. Prado wrote:\n> > > > The MinNumRequests property reports the bare minimum number of requests\n> > > > needed in the camera pipeline for capture to be possible. It is equal to\n> > > > the number of buffers required by the driver. At this quantity, there's\n> > > > no guarantee that frames won't be dropped or that manual per-frame\n> > > > controls will apply correctly. The quantity needed for those may be\n> > > > added as separate properties in the future.\n> > > >\n> > > > Signed-off-by: Nícolas F. R. A. Prado \n> > > > ---\n> > > >\n> > > > Changes in v6:\n> > > > - Thanks to Naushir:\n> > > > - Removed comment from Raspberrypi MinNumRequests setting\n> > > > - Removed an extra blank line\n> > > >\n> > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 ++++++\n> > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 2 ++\n> > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 ++\n> > > > src/libcamera/pipeline/simple/simple.cpp | 2 ++\n> > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 ++\n> > > > src/libcamera/pipeline/vimc/vimc.cpp | 2 ++\n> > > > src/libcamera/property_ids.yaml | 8 ++++++++\n> > > > 7 files changed, 24 insertions(+)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > index 76c3bb3d8aa9..017018c845fa 100644\n> > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > @@ -1111,6 +1111,12 @@ int PipelineHandlerIPU3::registerCameras()\n> > > > \t\t/* Initialize the camera properties. */\n> > > > \t\tdata->properties_ = cio2->sensor()->properties();\n> > > >\n> > > > +\t\t/*\n> > > > +\t\t * \\todo Make sure this is correct even after the stream is\n> > > > +\t\t * configured to CIO2\n> > >\n> > > What does this comment mean?\n> > >\n> > > I see that you don't have a reference to IPU3 in your cover letter, so\n> > > perhaps you're not able to test and validate this.\n> > >\n> > > What needs to be tested further?\n> > >\n> > > I'd rather see this tested and set correctly with the series than being\n> > > incorrectly introduced, and having to fix up later, as IPU3 is one of\n> > > our key targets.\n> >\n> > I'm just not sure that the IPU3 driver can work with a single request in all\n> > StreamConfigurations (maybe different pipeline topologies require different\n> > number of requests?).\n> >\n> > As you noted I don't have the hardware to test. So if you could, testing this\n> > would be just a matter of running lc-compliance with the new Overflow test:\n> >\n> > \tlc-compliance -f '*Overflow*'\n> >\n> > If it passes in all StreamConfigurations then we're good and I can remove that\n> > comment :).\n> >\n> > >\n> > >\n> > >\n> > > > +\t\t */\n> > > > +\t\tdata->properties_.set(properties::MinNumRequests, 1);\n> > > > +\n> > > > \t\tret = initControls(data.get());\n> > > > \t\tif (ret)\n> > > > \t\t\tcontinue;\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > index f821d8fe1b6c..98a97ffca15d 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > @@ -1046,6 +1046,8 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > > > \t/* Initialize the camera properties. */\n> > > > \tdata->properties_ = data->sensor_->properties();\n> > > >\n> > > > +\tdata->properties_.set(properties::MinNumRequests, 1);\n> > > > +\n> > > > \t/*\n> > > > \t * Set a default value for the ScalerCropMaximum property to show\n> > > > \t * that we support its use, however, initialise it to zero because\n> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > index 42911a8fdfdb..c81ebf14c6bf 100644\n> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > @@ -24,6 +24,7 @@\n> > > > #include \n> > > > #include \n> > > > #include \n> > > > +#include \n> > > > #include \n> > > > #include \n> > > >\n> > > > @@ -944,6 +945,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > > >\n> > > > \t/* Initialize the camera properties. */\n> > > > \tdata->properties_ = data->sensor_->properties();\n> > > > +\tdata->properties_.set(properties::MinNumRequests, 3);\n> > > >\n> > > > \t/*\n> > > > \t * \\todo Read dealy values from the sensor itself or from a\n> > > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > > > index b29fff9820e5..c4adea61519f 100644\n> > > > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > > > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > > > @@ -25,6 +25,7 @@\n> > > >\n> > > > #include \n> > > > #include \n> > > > +#include \n> > > > #include \n> > > > #include \n> > > >\n> > > > @@ -436,6 +437,7 @@ int SimpleCameraData::init()\n> > > > \t}\n> > > >\n> > > > \tproperties_ = sensor_->properties();\n> > > > +\tproperties_.set(properties::MinNumRequests, 3);\n> > > >\n> > > > \treturn 0;\n> > > > }\n> > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > > index 0f634b8da609..0258111ad6cf 100644\n> > > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > > @@ -525,6 +525,8 @@ int UVCCameraData::init(MediaDevice *media)\n> > > > \tproperties_.set(properties::PixelArraySize, resolution);\n> > > > \tproperties_.set(properties::PixelArrayActiveAreas, { Rectangle(resolution) });\n> > > >\n> > > > +\tproperties_.set(properties::MinNumRequests, 1);\n> > >\n> > > Is there any value in initialising this to one in the CameraSensor\n> > > constructor? Or is it better to make sure every pipeline sets it.\n> >\n> > I think it would make sense to have this set to 1 by default in the CameraSensor\n> > constructor, and any pipeline that has a different value can override it.\n> >\n> > One issue though is that the uvcvideo pipeline doesn't have a CameraSensor to\n> > initialize its properties from. So we'd still need to manually set it in that\n> > case.\n> >\n> > >\n> > > Will lc-complicance have a test to ensure that all pipelines set this?\n> >\n> > I hadn't created such a test. If we don't go that route of setting it by default\n> > I'll create it.\n> >\n> > >\n> > >\n> > > > +\n> > > > \t/* Initialise the supported controls. */\n> > > > \tControlInfoMap::Map ctrls;\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> > > > index 12f7517fd0ae..60ec473db0ba 100644\n> > > > --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> > > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> > > > @@ -21,6 +21,7 @@\n> > > > #include \n> > > > #include \n> > > > #include \n> > > > +#include \n> > > > #include \n> > > > #include \n> > > >\n> > > > @@ -516,6 +517,7 @@ int VimcCameraData::init()\n> > > >\n> > > > \t/* Initialize the camera properties. */\n> > > > \tproperties_ = sensor_->properties();\n> > > > +\tproperties_.set(properties::MinNumRequests, 2);\n> > > >\n> > > > \treturn 0;\n> > > > }\n> > > > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> > > > index 12ecbce5eed4..0208f92074d3 100644\n> > > > --- a/src/libcamera/property_ids.yaml\n> > > > +++ b/src/libcamera/property_ids.yaml\n> > > > @@ -678,6 +678,14 @@ controls:\n> > > > \\todo Turn this property into a \"maximum control value\" for the\n> > > > ScalerCrop control once \"dynamic\" controls have been implemented.\n> > > >\n> > > > + - MinNumRequests:\n> > >\n> > > As it's v6, perhaps this has been discussed/(bikeshedded) already - but\n> > > MinNumRequests grates on me a little\n> > >\n> > > We have property names such as\n> > > - ScalerCropMaximum:\n> > > - ColorFilterArrangement:\n> > >\n> > > so I see full words being used (Particularly looking at the 'Maximum').\n> > >\n> > > That would suggest to me:\n> > >\n> > > - MinimumNumberRequests\n> > >\n> > > which seems too verbose, so would then push me to:\n> > >\n> > > - MinimumRequests\n> >\n> > That sounds like a better name :)\n> >\n> > Thanks,\n> > Nícolas\n> >\n> > >\n> > >\n> > >\n> > > > + type: int32_t\n> > > > + description: |\n> > > > + The bare minimum number of requests needed in the camera pipeline for\n> > > > + capture to be possible, as required by the driver. Note that this\n> > > > + quantity does not guarantee that frames won't be dropped or that manual\n> > > > + per-frame controls will be applied properly.\n> \n> I would generalize it a bit and leave per-frame control out, unless\n> I've missed the implications of this property in relation to it.\n> \n> What about something like\n> \n> The minimum number of capture requests that the camera\n> pipeline requires to have queued to sustain the\n> capture operations.\n> \n> The requested number of queued requests usually\n\nI would change the above sentence to \"This number\". \"The requested number of\nqueued requests\" sounds confusing to me. Or maybe you meant \"The required number\nof queued requests\"?\n\nAnd I'm not sure about the \"usually\": Is there ever a case where this isn't\ntrue? I see that a request can have multiple buffers for multiple streams in a\npipeline, but aren't the pipelines independent in that case? That is, if you're\nusing a single stream which has MinimumRequests set to 3, you just need 3\nrequests with a single buffer each. But if you're using two streams you need 3\nrequests with 2 buffers each. Either way the pipeline depth and the number of\nrequests is the same, and the user is the one responsible for knowing how many\nstreams they're using and therefore how many buffers are needed.\n\nThanks,\nNícolas\n\n> corresponds to the minimum number of memory buffers\n> required to satisfy the requirements of the hardware\n> processing blocks that compose the capture pipeline\n> and represents the minimum depth of the capture\n> request queue.\n> \n> Suggestion or corrections welcome, of course.\n> \n> > > > +\n> > > > # ----------------------------------------------------------------------------\n> > > > # Draft properties section\n> > > >\n> > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8B8DEC322B\n\tfor ;\n\tMon, 19 Jul 2021 14:00:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0DD246853C;\n\tMon, 19 Jul 2021 16:00:59 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk [46.235.227.227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C20B16851F\n\tfor ;\n\tMon, 19 Jul 2021 16:00:57 +0200 (CEST)","from notapiano (unknown\n\t[IPv6:2804:14c:1a9:2434:2e2f:cb19:fca8:1dff])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits))\n\t(No client certificate requested) (Authenticated sender: nfraprado)\n\tby bhuna.collabora.co.uk (Postfix) with ESMTPSA id C79D41F42BDF;\n\tMon, 19 Jul 2021 15:00:55 +0100 (BST)"],"Date":"Mon, 19 Jul 2021 11:00:50 -0300","From":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado ","To":"Jacopo Mondi ","Message-ID":"<20210719140050.obsx34wwyqsp4o76@notapiano>","References":"<20210714183857.2046425-1-nfraprado@collabora.com>\n\t<20210714183857.2046425-2-nfraprado@collabora.com>\n\t<5c7a5506-11b3-ef74-384b-b99be14b0da7@ideasonboard.com>\n\t<20210715145250.xensy6lnhcwwbujm@notapiano>\n\t<20210715153050.h7hcynsu77irsztr@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210715153050.h7hcynsu77irsztr@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v6 01/10] libcamera: property: Add\n\tMinNumRequests property","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","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?b?bmRyw6k=?= Almeida ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18272,"web_url":"https://patchwork.libcamera.org/comment/18272/","msgid":"<20210722175420.2julboueealjvjai@notapiano>","date":"2021-07-22T17:54:20","subject":"Re: [libcamera-devel] [PATCH v6 01/10] libcamera: property: Add\n\tMinNumRequests property","submitter":{"id":84,"url":"https://patchwork.libcamera.org/api/people/84/","name":"Nícolas F. R. A. Prado","email":"nfraprado@collabora.com"},"content":"Hi Kieran,\n\nOn Thu, Jul 15, 2021 at 02:16:58PM -0300, Nícolas F. R. A. Prado wrote:\n> Hi Kieran,\n> \n> On Thu, Jul 15, 2021 at 05:45:58PM +0100, Kieran Bingham wrote:\n> > Hi Nicolas,\n> > \n> > On 15/07/2021 15:52, Nícolas F. R. A. Prado wrote:\n> > > Hi Kieran,\n> > > \n> > > On Thu, Jul 15, 2021 at 10:45:56AM +0100, Kieran Bingham wrote:\n> > >> Hi Nicolas,\n> > >>\n> > >> On 14/07/2021 19:38, Nícolas F. R. A. Prado wrote:\n> > >>> The MinNumRequests property reports the bare minimum number of requests\n> > >>> needed in the camera pipeline for capture to be possible. It is equal to\n> > >>> the number of buffers required by the driver. At this quantity, there's\n> > >>> no guarantee that frames won't be dropped or that manual per-frame\n> > >>> controls will apply correctly. The quantity needed for those may be\n> > >>> added as separate properties in the future.\n> > >>>\n> > >>> Signed-off-by: Nícolas F. R. A. Prado \n> > >>> ---\n> > >>>\n> > >>> Changes in v6:\n> > >>> - Thanks to Naushir:\n> > >>> - Removed comment from Raspberrypi MinNumRequests setting\n> > >>> - Removed an extra blank line\n> > >>>\n> > >>> src/libcamera/pipeline/ipu3/ipu3.cpp | 6 ++++++\n> > >>> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 2 ++\n> > >>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 ++\n> > >>> src/libcamera/pipeline/simple/simple.cpp | 2 ++\n> > >>> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 ++\n> > >>> src/libcamera/pipeline/vimc/vimc.cpp | 2 ++\n> > >>> src/libcamera/property_ids.yaml | 8 ++++++++\n> > >>> 7 files changed, 24 insertions(+)\n> > >>>\n> > >>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > >>> index 76c3bb3d8aa9..017018c845fa 100644\n> > >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > >>> @@ -1111,6 +1111,12 @@ int PipelineHandlerIPU3::registerCameras()\n> > >>> \t\t/* Initialize the camera properties. */\n> > >>> \t\tdata->properties_ = cio2->sensor()->properties();\n> > >>> \n> > >>> +\t\t/*\n> > >>> +\t\t * \\todo Make sure this is correct even after the stream is\n> > >>> +\t\t * configured to CIO2\n> > >>\n> > >> What does this comment mean?\n> > >>\n> > >> I see that you don't have a reference to IPU3 in your cover letter, so\n> > >> perhaps you're not able to test and validate this.\n> > >>\n> > >> What needs to be tested further?\n> > >>\n> > >> I'd rather see this tested and set correctly with the series than being\n> > >> incorrectly introduced, and having to fix up later, as IPU3 is one of\n> > >> our key targets.\n> > > \n> > > I'm just not sure that the IPU3 driver can work with a single request in all\n> > > StreamConfigurations (maybe different pipeline topologies require different\n> > > number of requests?).\n> > > \n> > > As you noted I don't have the hardware to test. So if you could, testing this\n> > > would be just a matter of running lc-compliance with the new Overflow test:\n> > > \n> > > \tlc-compliance -f '*Overflow*'\n> > > \n> > \n> > Using camera \\_SB_.PCI0.I2C2.CAM0\n> > Note: Google Test filter = *Overflow*\n> > [==========] Running 4 tests from 1 test suite.\n> > [----------] Global test environment set-up.\n> > [----------] 4 tests from CaptureTests/RoleParametrizedTest\n> > [ RUN ] CaptureTests/RoleParametrizedTest.Overflow/Raw\n> > [2:17:21.161407453] [25870] INFO Camera camera.cpp:906 configuring\n> > streams: (0) 4224x3136-SGRBG10_IPU3\n> > [2:17:21.161682517] [25871] ERROR V4L2 v4l2_subdevice.cpp:286 'ov13858\n> > 8-0010': Unable to get rectangle 0 on pad 0: Inappropriate ioctl for device\n> > [2:17:21.161703674] [25871] WARN CameraSensor camera_sensor.cpp:737\n> > 'ov13858 8-0010': The analogue crop rectangle has been defaulted to the\n> > active area size\n> > \n> > \n> > \n> > But I'm not sure if that's this overflow tests fault, as I'm not\n> > convinced it runs without your series yet anyway, as it looks like the\n> > tests are failing due to incomplete Raw stream support perhaps ...\n> \n> Hmm. Well, you could try running one of the current raw tests, eg\n> \n> \tlc-compliance -f '*Capture/Raw_5'\n> \n> and also a test with a different role\n> \n> \tlc-compliance -f '*Capture/StillCapture_5'\n> \n> to find out if it's the pipeline or the Overflow test's fault.\n> \n> You could also run the Overflow test for all roles but Raw with\n> \n> \tlc-compliance -f '*Overflow*:-*Raw*'\n> \n> although even if all pass, we might still be missing an issue with this test on\n> the raw streamrole...\n\nActually, I think I'm just overthinking here :). I just looked at the ipu3\ndriver and it has:\n\n\tvbq->min_buffers_needed = 1;\n\nIt's constant. Different stream roles won't matter. I'll go ahead and drop the\ncomment for v7.\n\nThanks,\nNícolas\n\n> \n> Thanks,\n> Nícolas\n> \n> > \n> > \n> > \n> > \n> > > If it passes in all StreamConfigurations then we're good and I can remove that\n> > > comment :).\n> > > \n> > >>\n> > >>\n> > >>\n> > >>> +\t\t */\n> > >>> +\t\tdata->properties_.set(properties::MinNumRequests, 1);\n> > >>> +\n> > >>> \t\tret = initControls(data.get());\n> > >>> \t\tif (ret)\n> > >>> \t\t\tcontinue;\n> > >>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >>> index f821d8fe1b6c..98a97ffca15d 100644\n> > >>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >>> @@ -1046,6 +1046,8 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > >>> \t/* Initialize the camera properties. */\n> > >>> \tdata->properties_ = data->sensor_->properties();\n> > >>> \n> > >>> +\tdata->properties_.set(properties::MinNumRequests, 1);\n> > >>> +\n> > >>> \t/*\n> > >>> \t * Set a default value for the ScalerCropMaximum property to show\n> > >>> \t * that we support its use, however, initialise it to zero because\n> > >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > >>> index 42911a8fdfdb..c81ebf14c6bf 100644\n> > >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > >>> @@ -24,6 +24,7 @@\n> > >>> #include \n> > >>> #include \n> > >>> #include \n> > >>> +#include \n> > >>> #include \n> > >>> #include \n> > >>> \n> > >>> @@ -944,6 +945,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > >>> \n> > >>> \t/* Initialize the camera properties. */\n> > >>> \tdata->properties_ = data->sensor_->properties();\n> > >>> +\tdata->properties_.set(properties::MinNumRequests, 3);\n> > >>> \n> > >>> \t/*\n> > >>> \t * \\todo Read dealy values from the sensor itself or from a\n> > >>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > >>> index b29fff9820e5..c4adea61519f 100644\n> > >>> --- a/src/libcamera/pipeline/simple/simple.cpp\n> > >>> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > >>> @@ -25,6 +25,7 @@\n> > >>> \n> > >>> #include \n> > >>> #include \n> > >>> +#include \n> > >>> #include \n> > >>> #include \n> > >>> \n> > >>> @@ -436,6 +437,7 @@ int SimpleCameraData::init()\n> > >>> \t}\n> > >>> \n> > >>> \tproperties_ = sensor_->properties();\n> > >>> +\tproperties_.set(properties::MinNumRequests, 3);\n> > >>> \n> > >>> \treturn 0;\n> > >>> }\n> > >>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > >>> index 0f634b8da609..0258111ad6cf 100644\n> > >>> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > >>> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > >>> @@ -525,6 +525,8 @@ int UVCCameraData::init(MediaDevice *media)\n> > >>> \tproperties_.set(properties::PixelArraySize, resolution);\n> > >>> \tproperties_.set(properties::PixelArrayActiveAreas, { Rectangle(resolution) });\n> > >>> \n> > >>> +\tproperties_.set(properties::MinNumRequests, 1);\n> > >>\n> > >> Is there any value in initialising this to one in the CameraSensor\n> > >> constructor? Or is it better to make sure every pipeline sets it.\n> > > \n> > > I think it would make sense to have this set to 1 by default in the CameraSensor\n> > > constructor, and any pipeline that has a different value can override it.\n> > > \n> > > One issue though is that the uvcvideo pipeline doesn't have a CameraSensor to\n> > > initialize its properties from. So we'd still need to manually set it in that\n> > > case.\n> > > \n> > >>\n> > >> Will lc-complicance have a test to ensure that all pipelines set this?\n> > > \n> > > I hadn't created such a test. If we don't go that route of setting it by default\n> > > I'll create it.\n> > > \n> > >>\n> > >>\n> > >>> +\n> > >>> \t/* Initialise the supported controls. */\n> > >>> \tControlInfoMap::Map ctrls;\n> > >>> \n> > >>> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> > >>> index 12f7517fd0ae..60ec473db0ba 100644\n> > >>> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> > >>> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> > >>> @@ -21,6 +21,7 @@\n> > >>> #include \n> > >>> #include \n> > >>> #include \n> > >>> +#include \n> > >>> #include \n> > >>> #include \n> > >>> \n> > >>> @@ -516,6 +517,7 @@ int VimcCameraData::init()\n> > >>> \n> > >>> \t/* Initialize the camera properties. */\n> > >>> \tproperties_ = sensor_->properties();\n> > >>> +\tproperties_.set(properties::MinNumRequests, 2);\n> > >>> \n> > >>> \treturn 0;\n> > >>> }\n> > >>> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> > >>> index 12ecbce5eed4..0208f92074d3 100644\n> > >>> --- a/src/libcamera/property_ids.yaml\n> > >>> +++ b/src/libcamera/property_ids.yaml\n> > >>> @@ -678,6 +678,14 @@ controls:\n> > >>> \\todo Turn this property into a \"maximum control value\" for the\n> > >>> ScalerCrop control once \"dynamic\" controls have been implemented.\n> > >>> \n> > >>> + - MinNumRequests:\n> > >>\n> > >> As it's v6, perhaps this has been discussed/(bikeshedded) already - but\n> > >> MinNumRequests grates on me a little\n> > >>\n> > >> We have property names such as\n> > >> - ScalerCropMaximum:\n> > >> - ColorFilterArrangement:\n> > >>\n> > >> so I see full words being used (Particularly looking at the 'Maximum').\n> > >>\n> > >> That would suggest to me:\n> > >>\n> > >> - MinimumNumberRequests\n> > >>\n> > >> which seems too verbose, so would then push me to:\n> > >>\n> > >> - MinimumRequests\n> > > \n> > > That sounds like a better name :)\n> > > \n> > > Thanks,\n> > > Nícolas\n> > > \n> > >>\n> > >>\n> > >>\n> > >>> + type: int32_t\n> > >>> + description: |\n> > >>> + The bare minimum number of requests needed in the camera pipeline for\n> > >>> + capture to be possible, as required by the driver. Note that this\n> > >>> + quantity does not guarantee that frames won't be dropped or that manual\n> > >>> + per-frame controls will be applied properly.\n> > >>> +\n> > >>> # ----------------------------------------------------------------------------\n> > >>> # Draft properties section\n> > >>> \n> > >>>\n> > \n> > -- \n> > To unsubscribe, send mail to kernel-unsubscribe@lists.collabora.co.uk.","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 93409C0109\n\tfor ;\n\tThu, 22 Jul 2021 17:54:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AB386687A6;\n\tThu, 22 Jul 2021 19:54:31 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk [46.235.227.227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0F2E468779\n\tfor ;\n\tThu, 22 Jul 2021 19:54:30 +0200 (CEST)","from notapiano (unknown\n\t[IPv6:2804:14c:1a9:2434:a4c5:aa5e:1d8c:5e2c])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits))\n\t(No client certificate requested) (Authenticated sender: nfraprado)\n\tby bhuna.collabora.co.uk (Postfix) with ESMTPSA id 675971F4425E;\n\tThu, 22 Jul 2021 18:54:28 +0100 (BST)"],"Date":"Thu, 22 Jul 2021 14:54:20 -0300","From":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado ","To":"Kieran Bingham ","Message-ID":"<20210722175420.2julboueealjvjai@notapiano>","References":"<20210714183857.2046425-1-nfraprado@collabora.com>\n\t<20210714183857.2046425-2-nfraprado@collabora.com>\n\t<5c7a5506-11b3-ef74-384b-b99be14b0da7@ideasonboard.com>\n\t<20210715145250.xensy6lnhcwwbujm@notapiano>\n\t<9e0cff16-2783-8894-d6e6-d4a1f89ef950@ideasonboard.com>\n\t<20210715171658.mx6mgeemfng5nqir@notapiano>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210715171658.mx6mgeemfng5nqir@notapiano>","Subject":"Re: [libcamera-devel] [PATCH v6 01/10] libcamera: property: Add\n\tMinNumRequests property","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","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?b?bmRyw6k=?= Almeida ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]