[{"id":26094,"web_url":"https://patchwork.libcamera.org/comment/26094/","msgid":"<20221216140059.gmhxp6w52q6gqrxf@uno.localdomain>","date":"2022-12-16T14:00:59","subject":"Re: [libcamera-devel] [PATCH v9 02/18] libcamera: property: Add\n\tMinimumRequests property","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul\n\nOn Fri, Dec 16, 2022 at 09:29:23PM +0900, Paul Elder via libcamera-devel 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> Signed-off-by: Nícolas F. R. A. Prado \n> Reviewed-by: Laurent Pinchart \n> Reviewed-by: Paul Elder \n> Signed-off-by: Paul Elder \n>\n\nSeems quite on-spot considering the thread Umang has just started\n\nTo be honest I would have expected this to generate more discussion,\nbut seeing Laurent's tag there makes me presume this has been blessed\nalready during the previous review.\n\n\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 | 22 +++++++---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +\n> .../pipeline/raspberrypi/raspberrypi.cpp | 2 +\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +\n> src/libcamera/pipeline/simple/simple.cpp | 40 +++++++++++++++++--\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +\n> src/libcamera/pipeline/vimc/vimc.cpp | 2 +\n> src/libcamera/property_ids.yaml | 21 ++++++++++\n> 8 files changed, 84 insertions(+), 9 deletions(-)\n>\n> diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst\n> index e1930fdf..a7356e4a 100644\n> --- a/Documentation/guides/pipeline-handler.rst\n> +++ b/Documentation/guides/pipeline-handler.rst\n> @@ -658,19 +658,31 @@ associated with immutable values, which represent static characteristics that ca\n> be used by applications to identify camera devices in the system. Properties can be\n> registered by inspecting the values of V4L2 controls from the video devices and\n> camera sensor (for example to retrieve the position and orientation of a camera)\n> -or to express other immutable characteristics. The example pipeline handler does\n> -not register any property, but examples are available in the libcamera code\n> -base.\n> +or to express other immutable characteristics.\n>\n> -.. TODO: Add a property example to the pipeline handler. At least the model.\n> +A required property is ``MinimumRequests``, which indicates how many requests\n> +need to be queued in the pipeline for capture without frame drops to be\n> +possible.\n> +\n> +In our case, the vivid driver requires two buffers before it'll start streaming\n> +(can be seen in the ``min_buffers_needed`` property for the ``vid_cap`` queue in\n> +vivid's driver code). In order to not drop frames we should have one extra\n> +buffer queued to the driver so that it is used as soon as the previous buffer\n> +completes. Therefore we will set our ``MinimumRequests`` to three. Append the\n> +following line to ``init()``:\n\nThe driver requires two buffers to operate, why an extra one is needed ?\nFor vivid in particular, where buffer's content is generated..\n\n> +\n> +.. code-block:: cpp\n> +\n> + properties_.set(properties::MinimumRequests, 3);\n>\n> At this point you need to add the following includes to the top of the file for\n> -handling controls:\n> +handling controls and properties:\n>\n> .. code-block:: cpp\n>\n> #include \n> #include \n> + #include \n>\n> Generating a default configuration\n> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index e4d79ea4..98a4a3e5 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -1126,6 +1126,8 @@ int PipelineHandlerIPU3::registerCameras()\n> \t\t/* Initialize the camera properties. */\n> \t\tdata->properties_ = cio2->sensor()->properties();\n>\n> +\t\tdata->properties_.set(properties::MinimumRequests, 3);\n> +\n\nI see\n\n static constexpr unsigned int kBufferCount = 4;\n\nWhich seems to suggest that at least 4 buffers need to be queued to\nthe video device before streaming is started.\n\nI wonder if I'm confusing the two concepts\n\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 8569df17..4a08d01e 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1063,6 +1063,8 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> \t/* Enable SOF event generation. */\n> \tdata->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);\n>\n> +\tdata->properties_.set(properties::MinimumRequests, 3);\n> +\n> \t/*\n> \t * Reset the delayed controls with the gain and exposure values set by\n> \t * the IPA.\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index f658d75e..45b6beaf 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -23,6 +23,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -1102,6 +1103,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n>\n> \t/* Initialize the camera properties. */\n> \tdata->properties_ = data->sensor_->properties();\n> +\tdata->properties_.set(properties::MinimumRequests, 3);\n\nSame question for RkISP1, it seems we don't even populate\nStreamConfiguration::bufferCount there, but I'm pretty sure you need 4\nrequests queued before capture can start..\n\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 24ded4db..8ed983fe 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> @@ -180,6 +181,10 @@ class SimplePipelineHandler;\n>\n> struct SimplePipelineInfo {\n> \tconst char *driver;\n> +\t/*\n> +\t * Minimum number of buffers required by the driver to start streaming.\n> +\t */\n> +\tunsigned int minimumBuffers;\n> \t/*\n> \t * Each converter in the list contains the name\n> \t * and the number of streams it supports.\n> @@ -190,10 +195,10 @@ struct SimplePipelineInfo {\n> namespace {\n>\n> static const SimplePipelineInfo supportedDevices[] = {\n> -\t{ \"imx7-csi\", { { \"pxp\", 1 } } },\n> -\t{ \"mxc-isi\", {} },\n> -\t{ \"qcom-camss\", {} },\n> -\t{ \"sun6i-csi\", {} },\n> +\t{ \"imx7-csi\", 2, { { \"pxp\", 1 } } },\n> +\t{ \"mxc-isi\", 3, {} },\n> +\t{ \"qcom-camss\", 1, {} },\n> +\t{ \"sun6i-csi\", 3, {} },\n> };\n>\n> } /* namespace */\n> @@ -271,6 +276,8 @@ public:\n> \tbool useConverter_;\n> \tstd::queue> converterQueue_;\n>\n> +\tconst SimplePipelineInfo *deviceInfo;\n> +\n> private:\n> \tvoid tryPipeline(unsigned int code, const Size &size);\n> \tstatic std::vector routedSourcePads(MediaPad *sink);\n> @@ -1168,6 +1175,29 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n> \tinputCfg.stride = captureFormat.planes[0].bpl;\n> \tinputCfg.bufferCount = kNumInternalBuffers;\n>\n> +\t/* Set the MinimumRequests property. */\n> +\tunsigned int minimumRequests;\n> +\n> +\tif (data->useConverter_) {\n> +\t\t/*\n> +\t\t * The application will interact only with the capture node of\n> +\t\t * the converter. Require two buffers for a frame drop free\n> +\t\t * conversion, plus one extra to account for requeue delays.\n> +\t\t */\n> +\t\tminimumRequests = 3;\n> +\t} else {\n> +\t\t/*\n> +\t\t * The application will interact directly with the video capture\n> +\t\t * device. Require the minimum required by the driver, plus one\n> +\t\t * extra to account for requeue delays. Force at least three\n> +\t\t * buffers in order to not drop frames.\n> +\t\t */\n> +\t\tminimumRequests = std::max(data->deviceInfo->minimumBuffers + 1,\n> +\t\t\t\t\t 3U);\n> +\t}\n> +\n> +\tdata->properties_.set(properties::MinimumRequests, minimumRequests);\n> +\n> \treturn data->converter_->configure(inputCfg, outputCfgs);\n> }\n>\n> @@ -1506,6 +1536,8 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)\n> \tbool registered = false;\n>\n> \tfor (std::unique_ptr &data : pipelines) {\n> +\t\tdata->deviceInfo = info;\n> +\n> \t\tint ret = data->init();\n> \t\tif (ret < 0)\n> \t\t\tcontinue;\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 277465b7..7f580955 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -500,6 +500,8 @@ int UVCCameraData::init(MediaDevice *media)\n> \tproperties_.set(properties::PixelArraySize, resolution);\n> \tproperties_.set(properties::PixelArrayActiveAreas, { Rectangle(resolution) });\n>\n> +\tproperties_.set(properties::MinimumRequests, 3);\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 204f5ad7..d2633be4 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> @@ -571,6 +572,7 @@ int VimcCameraData::init()\n>\n> \t/* Initialize the camera properties. */\n> \tproperties_ = sensor_->properties();\n> +\tproperties_.set(properties::MinimumRequests, 3);\n>\n> \treturn 0;\n> }\n> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> index cb55e0ed..c82ac17e 100644\n> --- a/src/libcamera/property_ids.yaml\n> +++ b/src/libcamera/property_ids.yaml\n> @@ -690,6 +690,27 @@ controls:\n> that is twice that of the full resolution mode. This value will be valid\n> after the configure method has returned successfully.\n>\n> + - MinimumRequests:\n> + type: int32_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 without frame\n> + drops. At this quantity, there's no guarantee that manual per-frame\n> + controls will apply correctly.\n\nThis property is also relevant for the startup phase, so mentioning\nthat this is the minium number of requests that need to be queued\nbefore capture starts might be necessary ?\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> # Draft properties section\n>\n> --\n> 2.35.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7BC32C3200\n\tfor ;\n\tFri, 16 Dec 2022 14:01:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 041006339A;\n\tFri, 16 Dec 2022 15:01:03 +0100 (CET)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::225])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B13DE603D0\n\tfor ;\n\tFri, 16 Dec 2022 15:01:00 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 41CB21C0010;\n\tFri, 16 Dec 2022 14:00:59 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1671199263;\n\tbh=BlHdu/obmi/onRlqjoRzq/nxmQEW+Gw+KxBaUwMASQk=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=nAhJVYldlt5rwpX+JEtwsASqZRqQaaQZj/1vc7s2Z0S95o+YuRRDoeo3BiHO1R6FA\n\tdFSxoKnrWyO3PR9BKRglr5eO/uZUDQJF97D07QKnuofdWPSgn8YFTOL+oayuJ2t9Nl\n\t9oNPlUCTxBotq05kz8kvym6IkRa1ah3VMkhIBvZM2Dmq7DQy4pwS5q7d8iox+05QY7\n\tPzVNhIqaKdsgVWgDt3QcIcg5y8cpueRvvljaKSR4+wz+5eG+y3mEz5A1px24ZV0SGA\n\tPrRFgRt3KyaME+rHU1t1l0GxAvW9eCCzKVDPcZ9ko4qNWhUdyJTe2kjGiX1gMNyaiV\n\tC2C6jdSlXzXiA==","Date":"Fri, 16 Dec 2022 15:00:59 +0100","To":"Paul Elder ","Message-ID":"<20221216140059.gmhxp6w52q6gqrxf@uno.localdomain>","References":"<20221216122939.256534-1-paul.elder@ideasonboard.com>\n\t<20221216122939.256534-3-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20221216122939.256534-3-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v9 02/18] libcamera: property: Add\n\tMinimumRequests 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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26114,"web_url":"https://patchwork.libcamera.org/comment/26114/","msgid":"","date":"2022-12-20T08:21:03","subject":"Re: [libcamera-devel] [PATCH v9 02/18] libcamera: property: Add\n\tMinimumRequests property","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Dec 16, 2022 at 03:00:59PM +0100, Jacopo Mondi wrote:\n> Hi Paul\n> \n> On Fri, Dec 16, 2022 at 09:29:23PM +0900, Paul Elder via libcamera-devel 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> > Signed-off-by: Nícolas F. R. A. Prado \n> > Reviewed-by: Laurent Pinchart \n> > Reviewed-by: Paul Elder \n> > Signed-off-by: Paul Elder \n> >\n> \n> Seems quite on-spot considering the thread Umang has just started\n> \n> To be honest I would have expected this to generate more discussion,\n> but seeing Laurent's tag there makes me presume this has been blessed\n> already during the previous review.\n> \n> \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 | 22 +++++++---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 2 +\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +\n> > src/libcamera/pipeline/simple/simple.cpp | 40 +++++++++++++++++--\n> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +\n> > src/libcamera/pipeline/vimc/vimc.cpp | 2 +\n> > src/libcamera/property_ids.yaml | 21 ++++++++++\n> > 8 files changed, 84 insertions(+), 9 deletions(-)\n> >\n> > diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst\n> > index e1930fdf..a7356e4a 100644\n> > --- a/Documentation/guides/pipeline-handler.rst\n> > +++ b/Documentation/guides/pipeline-handler.rst\n> > @@ -658,19 +658,31 @@ associated with immutable values, which represent static characteristics that ca\n> > be used by applications to identify camera devices in the system. Properties can be\n> > registered by inspecting the values of V4L2 controls from the video devices and\n> > camera sensor (for example to retrieve the position and orientation of a camera)\n> > -or to express other immutable characteristics. The example pipeline handler does\n> > -not register any property, but examples are available in the libcamera code\n> > -base.\n> > +or to express other immutable characteristics.\n> >\n> > -.. TODO: Add a property example to the pipeline handler. At least the model.\n> > +A required property is ``MinimumRequests``, which indicates how many requests\n> > +need to be queued in the pipeline for capture without frame drops to be\n> > +possible.\n> > +\n> > +In our case, the vivid driver requires two buffers before it'll start streaming\n> > +(can be seen in the ``min_buffers_needed`` property for the ``vid_cap`` queue in\n> > +vivid's driver code). In order to not drop frames we should have one extra\n> > +buffer queued to the driver so that it is used as soon as the previous buffer\n> > +completes. Therefore we will set our ``MinimumRequests`` to three. Append the\n> > +following line to ``init()``:\n> \n> The driver requires two buffers to operate, why an extra one is needed ?\n> For vivid in particular, where buffer's content is generated..\n\nI have a feeling that the intention was a spare buffer floating around\nin the application, but given the points that you've identified below,\nalso maybe not.\n\nAlso I feel like this description is mixing up Requests and buffers. I\nguess I should've vetted it a bit more (I was too distracted by merge\nconflicts).\n\n> \n> > +\n> > +.. code-block:: cpp\n> > +\n> > + properties_.set(properties::MinimumRequests, 3);\n> >\n> > At this point you need to add the following includes to the top of the file for\n> > -handling controls:\n> > +handling controls and properties:\n> >\n> > .. code-block:: cpp\n> >\n> > #include \n> > #include \n> > + #include \n> >\n> > Generating a default configuration\n> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index e4d79ea4..98a4a3e5 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -1126,6 +1126,8 @@ int PipelineHandlerIPU3::registerCameras()\n> > \t\t/* Initialize the camera properties. */\n> > \t\tdata->properties_ = cio2->sensor()->properties();\n> >\n> > +\t\tdata->properties_.set(properties::MinimumRequests, 3);\n> > +\n> \n> I see\n> \n> static constexpr unsigned int kBufferCount = 4;\n> \n> Which seems to suggest that at least 4 buffers need to be queued to\n> the video device before streaming is started.\n> \n> I wonder if I'm confusing the two concepts\n\nGah, the ipu3 code is so convoluted.\n\nkBufferCount is the number of internal buffers to allocate on the CIO2.\nAs far as I understand, they're only used if no buffer is provided in\nthe Request.\n\nSo yes, I think you are confusing the two concepts. MinimumRequests is\nthe minumum number of Requests that need to be queued in order to\nsustain capture without frame drops. kBufferCount is the number of\ninternal buffers to allocate on the CIO2.\n\n> \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 8569df17..4a08d01e 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -1063,6 +1063,8 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> > \t/* Enable SOF event generation. */\n> > \tdata->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);\n> >\n> > +\tdata->properties_.set(properties::MinimumRequests, 3);\n> > +\n> > \t/*\n> > \t * Reset the delayed controls with the gain and exposure values set by\n> > \t * the IPA.\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index f658d75e..45b6beaf 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -23,6 +23,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -1102,6 +1103,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> >\n> > \t/* Initialize the camera properties. */\n> > \tdata->properties_ = data->sensor_->properties();\n> > +\tdata->properties_.set(properties::MinimumRequests, 3);\n> \n> Same question for RkISP1, it seems we don't even populate\n> StreamConfiguration::bufferCount there, but I'm pretty sure you need 4\n> requests queued before capture can start..\n\nI see internal buffers but only for params and stats...\n\nI guess I have to check this again too then.\n\n> \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 24ded4db..8ed983fe 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> > @@ -180,6 +181,10 @@ class SimplePipelineHandler;\n> >\n> > struct SimplePipelineInfo {\n> > \tconst char *driver;\n> > +\t/*\n> > +\t * Minimum number of buffers required by the driver to start streaming.\n> > +\t */\n> > +\tunsigned int minimumBuffers;\n> > \t/*\n> > \t * Each converter in the list contains the name\n> > \t * and the number of streams it supports.\n> > @@ -190,10 +195,10 @@ struct SimplePipelineInfo {\n> > namespace {\n> >\n> > static const SimplePipelineInfo supportedDevices[] = {\n> > -\t{ \"imx7-csi\", { { \"pxp\", 1 } } },\n> > -\t{ \"mxc-isi\", {} },\n> > -\t{ \"qcom-camss\", {} },\n> > -\t{ \"sun6i-csi\", {} },\n> > +\t{ \"imx7-csi\", 2, { { \"pxp\", 1 } } },\n> > +\t{ \"mxc-isi\", 3, {} },\n> > +\t{ \"qcom-camss\", 1, {} },\n> > +\t{ \"sun6i-csi\", 3, {} },\n> > };\n> >\n> > } /* namespace */\n> > @@ -271,6 +276,8 @@ public:\n> > \tbool useConverter_;\n> > \tstd::queue> converterQueue_;\n> >\n> > +\tconst SimplePipelineInfo *deviceInfo;\n> > +\n> > private:\n> > \tvoid tryPipeline(unsigned int code, const Size &size);\n> > \tstatic std::vector routedSourcePads(MediaPad *sink);\n> > @@ -1168,6 +1175,29 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n> > \tinputCfg.stride = captureFormat.planes[0].bpl;\n> > \tinputCfg.bufferCount = kNumInternalBuffers;\n> >\n> > +\t/* Set the MinimumRequests property. */\n> > +\tunsigned int minimumRequests;\n> > +\n> > +\tif (data->useConverter_) {\n> > +\t\t/*\n> > +\t\t * The application will interact only with the capture node of\n> > +\t\t * the converter. Require two buffers for a frame drop free\n> > +\t\t * conversion, plus one extra to account for requeue delays.\n> > +\t\t */\n> > +\t\tminimumRequests = 3;\n> > +\t} else {\n> > +\t\t/*\n> > +\t\t * The application will interact directly with the video capture\n> > +\t\t * device. Require the minimum required by the driver, plus one\n> > +\t\t * extra to account for requeue delays. Force at least three\n> > +\t\t * buffers in order to not drop frames.\n> > +\t\t */\n> > +\t\tminimumRequests = std::max(data->deviceInfo->minimumBuffers + 1,\n> > +\t\t\t\t\t 3U);\n> > +\t}\n> > +\n> > +\tdata->properties_.set(properties::MinimumRequests, minimumRequests);\n> > +\n> > \treturn data->converter_->configure(inputCfg, outputCfgs);\n> > }\n> >\n> > @@ -1506,6 +1536,8 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)\n> > \tbool registered = false;\n> >\n> > \tfor (std::unique_ptr &data : pipelines) {\n> > +\t\tdata->deviceInfo = info;\n> > +\n> > \t\tint ret = data->init();\n> > \t\tif (ret < 0)\n> > \t\t\tcontinue;\n> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > index 277465b7..7f580955 100644\n> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > @@ -500,6 +500,8 @@ int UVCCameraData::init(MediaDevice *media)\n> > \tproperties_.set(properties::PixelArraySize, resolution);\n> > \tproperties_.set(properties::PixelArrayActiveAreas, { Rectangle(resolution) });\n> >\n> > +\tproperties_.set(properties::MinimumRequests, 3);\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 204f5ad7..d2633be4 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> > @@ -571,6 +572,7 @@ int VimcCameraData::init()\n> >\n> > \t/* Initialize the camera properties. */\n> > \tproperties_ = sensor_->properties();\n> > +\tproperties_.set(properties::MinimumRequests, 3);\n> >\n> > \treturn 0;\n> > }\n> > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> > index cb55e0ed..c82ac17e 100644\n> > --- a/src/libcamera/property_ids.yaml\n> > +++ b/src/libcamera/property_ids.yaml\n> > @@ -690,6 +690,27 @@ controls:\n> > that is twice that of the full resolution mode. This value will be valid\n> > after the configure method has returned successfully.\n> >\n> > + - MinimumRequests:\n> > + type: int32_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 without frame\n> > + drops. At this quantity, there's no guarantee that manual per-frame\n> > + controls will apply correctly.\n> \n> This property is also relevant for the startup phase, so mentioning\n> that this is the minium number of requests that need to be queued\n> before capture starts might be necessary ?\n\nIndeed.\n\n\nThanks,\n\nPaul\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> > # Draft properties section\n> >\n> > --\n> > 2.35.1\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 28347C3200\n\tfor ;\n\tTue, 20 Dec 2022 08:21:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A2A4B633A2;\n\tTue, 20 Dec 2022 09:21:13 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3490E63397\n\tfor ;\n\tTue, 20 Dec 2022 09:21:12 +0100 (CET)","from pyrite.rasen.tech\n\t(p4048023-ipxg22601hodogaya.kanagawa.ocn.ne.jp [180.53.122.23])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 862C5706;\n\tTue, 20 Dec 2022 09:21:10 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1671524473;\n\tbh=kEWaHB7KdKCgHftjsp3SmJ24Z6nfqDiSiPdwBq2IIXg=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=XMNPQK2QcReOi5NTInyEVmymyGb1v07nLF80dg7Ypbf9J10IjIaCG4ddHhjufuDvv\n\tSDdwDPvQHvakcr32nZhaKUA6Dfy+OwLgeAx+JytDiSVqCSEVvsNIXZETU4hz93yS37\n\tnFlEyHO4R52aKz99AFO28aia1NGgcTue6GI4+KMp+QvmjRZBVVMkbV8ws1sOtylBta\n\teGc4oKLYs6rVpT9TESYJ4yBajB0MXo95+yOEnGFBJ594Xwhv18+XdMmirynZSO2CAq\n\tRJ2fxdqLTf5J4aS37qO740LMBLRS3DbQvxXi8wT0KNYznfateGAEWlN0UM4rt+wwuj\n\thcTmlNIwNv1/Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1671524471;\n\tbh=kEWaHB7KdKCgHftjsp3SmJ24Z6nfqDiSiPdwBq2IIXg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AnVMNmIZskNRKWlcdZ4ZvcbVhimYe4vPoqkeiw7vd1z9CP24EnXAZRFuAChrg/VBd\n\tEX+PfvUq3xWeVFjanEPBFyC1B4qHGbojowatW7/8sAT0NGl+m5sy/EBeesW5BNnrsb\n\tsKoxb283fJFAE/ayN88064gckbREHGfkmU5mF5MY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"AnVMNmIZ\"; dkim-atps=neutral","Date":"Tue, 20 Dec 2022 17:21:03 +0900","To":"Jacopo Mondi ","Message-ID":"","References":"<20221216122939.256534-1-paul.elder@ideasonboard.com>\n\t<20221216122939.256534-3-paul.elder@ideasonboard.com>\n\t<20221216140059.gmhxp6w52q6gqrxf@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20221216140059.gmhxp6w52q6gqrxf@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v9 02/18] libcamera: property: Add\n\tMinimumRequests 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","From":"Paul Elder via libcamera-devel ","Reply-To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26115,"web_url":"https://patchwork.libcamera.org/comment/26115/","msgid":"<1a3f27d9-16d9-8d06-a55c-aa48b1a188d6@ideasonboard.com>","date":"2022-12-20T08:34:15","subject":"Re: [libcamera-devel] [PATCH v9 02/18] libcamera: property: Add\n\tMinimumRequests property","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Paul,\n\nOn 12/20/22 1:51 PM, Paul Elder via libcamera-devel wrote:\n> Hi Jacopo,\n>\n> On Fri, Dec 16, 2022 at 03:00:59PM +0100, Jacopo Mondi wrote:\n>> Hi Paul\n>>\n>> On Fri, Dec 16, 2022 at 09:29:23PM +0900, Paul Elder via libcamera-devel 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>>> Signed-off-by: Nícolas F. R. A. Prado \n>>> Reviewed-by: Laurent Pinchart \n>>> Reviewed-by: Paul Elder \n>>> Signed-off-by: Paul Elder \n>>>\n>> Seems quite on-spot considering the thread Umang has just started\n>>\n>> To be honest I would have expected this to generate more discussion,\n>> but seeing Laurent's tag there makes me presume this has been blessed\n>> already during the previous review.\n>>\n>>\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 | 22 +++++++---\n>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +\n>>> .../pipeline/raspberrypi/raspberrypi.cpp | 2 +\n>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +\n>>> src/libcamera/pipeline/simple/simple.cpp | 40 +++++++++++++++++--\n>>> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +\n>>> src/libcamera/pipeline/vimc/vimc.cpp | 2 +\n>>> src/libcamera/property_ids.yaml | 21 ++++++++++\n>>> 8 files changed, 84 insertions(+), 9 deletions(-)\n>>>\n>>> diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst\n>>> index e1930fdf..a7356e4a 100644\n>>> --- a/Documentation/guides/pipeline-handler.rst\n>>> +++ b/Documentation/guides/pipeline-handler.rst\n>>> @@ -658,19 +658,31 @@ associated with immutable values, which represent static characteristics that ca\n>>> be used by applications to identify camera devices in the system. Properties can be\n>>> registered by inspecting the values of V4L2 controls from the video devices and\n>>> camera sensor (for example to retrieve the position and orientation of a camera)\n>>> -or to express other immutable characteristics. The example pipeline handler does\n>>> -not register any property, but examples are available in the libcamera code\n>>> -base.\n>>> +or to express other immutable characteristics.\n>>>\n>>> -.. TODO: Add a property example to the pipeline handler. At least the model.\n>>> +A required property is ``MinimumRequests``, which indicates how many requests\n>>> +need to be queued in the pipeline for capture without frame drops to be\n>>> +possible.\n>>> +\n>>> +In our case, the vivid driver requires two buffers before it'll start streaming\n>>> +(can be seen in the ``min_buffers_needed`` property for the ``vid_cap`` queue in\n>>> +vivid's driver code). In order to not drop frames we should have one extra\n>>> +buffer queued to the driver so that it is used as soon as the previous buffer\n>>> +completes. Therefore we will set our ``MinimumRequests`` to three. Append the\n>>> +following line to ``init()``:\n>> The driver requires two buffers to operate, why an extra one is needed ?\n>> For vivid in particular, where buffer's content is generated..\n> I have a feeling that the intention was a spare buffer floating around\n> in the application, but given the points that you've identified below,\n> also maybe not.\n>\n> Also I feel like this description is mixing up Requests and buffers. I\n> guess I should've vetted it a bit more (I was too distracted by merge\n> conflicts).\n>\n>>> +\n>>> +.. code-block:: cpp\n>>> +\n>>> + properties_.set(properties::MinimumRequests, 3);\n>>>\n>>> At this point you need to add the following includes to the top of the file for\n>>> -handling controls:\n>>> +handling controls and properties:\n>>>\n>>> .. code-block:: cpp\n>>>\n>>> #include \n>>> #include \n>>> + #include \n>>>\n>>> Generating a default configuration\n>>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> index e4d79ea4..98a4a3e5 100644\n>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> @@ -1126,6 +1126,8 @@ int PipelineHandlerIPU3::registerCameras()\n>>> \t\t/* Initialize the camera properties. */\n>>> \t\tdata->properties_ = cio2->sensor()->properties();\n>>>\n>>> +\t\tdata->properties_.set(properties::MinimumRequests, 3);\n>>> +\n>> I see\n>>\n>> static constexpr unsigned int kBufferCount = 4;\n>>\n>> Which seems to suggest that at least 4 buffers need to be queued to\n>> the video device before streaming is started.\n>>\n>> I wonder if I'm confusing the two concepts\n> Gah, the ipu3 code is so convoluted.\n>\n> kBufferCount is the number of internal buffers to allocate on the CIO2.\n\nI see kBufferCount being set to non-raw cfg->bufferCount in \nIPU3CameraConfiguration::validate()\n> As far as I understand, they're only used if no buffer is provided in\n> the Request.\n>\n> So yes, I think you are confusing the two concepts. MinimumRequests is\n> the minumum number of Requests that need to be queued in order to\n> sustain capture without frame drops. kBufferCount is the number of\n> internal buffers to allocate on the CIO2.\n>\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 8569df17..4a08d01e 100644\n>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>> @@ -1063,6 +1063,8 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n>>> \t/* Enable SOF event generation. */\n>>> \tdata->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);\n>>>\n>>> +\tdata->properties_.set(properties::MinimumRequests, 3);\n>>> +\n>>> \t/*\n>>> \t * Reset the delayed controls with the gain and exposure values set by\n>>> \t * the IPA.\n>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>> index f658d75e..45b6beaf 100644\n>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>> @@ -23,6 +23,7 @@\n>>> #include \n>>> #include \n>>> #include \n>>> +#include \n>>> #include \n>>> #include \n>>> #include \n>>> @@ -1102,6 +1103,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n>>>\n>>> \t/* Initialize the camera properties. */\n>>> \tdata->properties_ = data->sensor_->properties();\n>>> +\tdata->properties_.set(properties::MinimumRequests, 3);\n>> Same question for RkISP1, it seems we don't even populate\n>> StreamConfiguration::bufferCount there, but I'm pretty sure you need 4\n>> requests queued before capture can start..\n> I see internal buffers but only for params and stats...\n>\n> I guess I have to check this again too then.\n\nI think MinimumRequests and minimum no. of buffers required to start and \nsustain capture - are closely related. More so, because you can't queue \nbuffers arbitrarily (from application's PoV) - capture buffers come \n_via_ queuing requests.\n\nThe internal buffers, I always understood as something not provided by \napplications like stats and params - completely internal to libcamera. \nThe minimumRequests property/criteria should not hinder/apply to \ninternal buffers right ?\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 24ded4db..8ed983fe 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>>> @@ -180,6 +181,10 @@ class SimplePipelineHandler;\n>>>\n>>> struct SimplePipelineInfo {\n>>> \tconst char *driver;\n>>> +\t/*\n>>> +\t * Minimum number of buffers required by the driver to start streaming.\n>>> +\t */\n>>> +\tunsigned int minimumBuffers;\n>>> \t/*\n>>> \t * Each converter in the list contains the name\n>>> \t * and the number of streams it supports.\n>>> @@ -190,10 +195,10 @@ struct SimplePipelineInfo {\n>>> namespace {\n>>>\n>>> static const SimplePipelineInfo supportedDevices[] = {\n>>> -\t{ \"imx7-csi\", { { \"pxp\", 1 } } },\n>>> -\t{ \"mxc-isi\", {} },\n>>> -\t{ \"qcom-camss\", {} },\n>>> -\t{ \"sun6i-csi\", {} },\n>>> +\t{ \"imx7-csi\", 2, { { \"pxp\", 1 } } },\n>>> +\t{ \"mxc-isi\", 3, {} },\n>>> +\t{ \"qcom-camss\", 1, {} },\n>>> +\t{ \"sun6i-csi\", 3, {} },\n>>> };\n>>>\n>>> } /* namespace */\n>>> @@ -271,6 +276,8 @@ public:\n>>> \tbool useConverter_;\n>>> \tstd::queue> converterQueue_;\n>>>\n>>> +\tconst SimplePipelineInfo *deviceInfo;\n>>> +\n>>> private:\n>>> \tvoid tryPipeline(unsigned int code, const Size &size);\n>>> \tstatic std::vector routedSourcePads(MediaPad *sink);\n>>> @@ -1168,6 +1175,29 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n>>> \tinputCfg.stride = captureFormat.planes[0].bpl;\n>>> \tinputCfg.bufferCount = kNumInternalBuffers;\n>>>\n>>> +\t/* Set the MinimumRequests property. */\n>>> +\tunsigned int minimumRequests;\n>>> +\n>>> +\tif (data->useConverter_) {\n>>> +\t\t/*\n>>> +\t\t * The application will interact only with the capture node of\n>>> +\t\t * the converter. Require two buffers for a frame drop free\n>>> +\t\t * conversion, plus one extra to account for requeue delays.\n>>> +\t\t */\n>>> +\t\tminimumRequests = 3;\n>>> +\t} else {\n>>> +\t\t/*\n>>> +\t\t * The application will interact directly with the video capture\n>>> +\t\t * device. Require the minimum required by the driver, plus one\n>>> +\t\t * extra to account for requeue delays. Force at least three\n>>> +\t\t * buffers in order to not drop frames.\n>>> +\t\t */\n>>> +\t\tminimumRequests = std::max(data->deviceInfo->minimumBuffers + 1,\n>>> +\t\t\t\t\t 3U);\n>>> +\t}\n>>> +\n>>> +\tdata->properties_.set(properties::MinimumRequests, minimumRequests);\n>>> +\n>>> \treturn data->converter_->configure(inputCfg, outputCfgs);\n>>> }\n>>>\n>>> @@ -1506,6 +1536,8 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)\n>>> \tbool registered = false;\n>>>\n>>> \tfor (std::unique_ptr &data : pipelines) {\n>>> +\t\tdata->deviceInfo = info;\n>>> +\n>>> \t\tint ret = data->init();\n>>> \t\tif (ret < 0)\n>>> \t\t\tcontinue;\n>>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>>> index 277465b7..7f580955 100644\n>>> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>>> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>>> @@ -500,6 +500,8 @@ int UVCCameraData::init(MediaDevice *media)\n>>> \tproperties_.set(properties::PixelArraySize, resolution);\n>>> \tproperties_.set(properties::PixelArrayActiveAreas, { Rectangle(resolution) });\n>>>\n>>> +\tproperties_.set(properties::MinimumRequests, 3);\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 204f5ad7..d2633be4 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>>> @@ -571,6 +572,7 @@ int VimcCameraData::init()\n>>>\n>>> \t/* Initialize the camera properties. */\n>>> \tproperties_ = sensor_->properties();\n>>> +\tproperties_.set(properties::MinimumRequests, 3);\n>>>\n>>> \treturn 0;\n>>> }\n>>> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n>>> index cb55e0ed..c82ac17e 100644\n>>> --- a/src/libcamera/property_ids.yaml\n>>> +++ b/src/libcamera/property_ids.yaml\n>>> @@ -690,6 +690,27 @@ controls:\n>>> that is twice that of the full resolution mode. This value will be valid\n>>> after the configure method has returned successfully.\n>>>\n>>> + - MinimumRequests:\n>>> + type: int32_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 without frame\n>>> + drops. At this quantity, there's no guarantee that manual per-frame\n>>> + controls will apply correctly.\n>> This property is also relevant for the startup phase, so mentioning\n>> that this is the minium number of requests that need to be queued\n>> before capture starts might be necessary ?\n> Indeed.\n>\n>\n> Thanks,\n>\n> Paul\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>>> # Draft properties section\n>>>\n>>> --\n>>> 2.35.1\n>>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D1A19C3213\n\tfor ;\n\tTue, 20 Dec 2022 08:34:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2FC8D633A2;\n\tTue, 20 Dec 2022 09:34:25 +0100 (CET)","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 E57A463397\n\tfor ;\n\tTue, 20 Dec 2022 09:34:23 +0100 (CET)","from [IPV6:2401:4900:1f3e:7d24:3f0:3e81:fb16:ab4d] (unknown\n\t[IPv6:2401:4900:1f3e:7d24:3f0:3e81:fb16:ab4d])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C2C45706;\n\tTue, 20 Dec 2022 09:34:21 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1671525265;\n\tbh=912VOxWzANtvRDStB1+X/jsMZs+QfMuhwAgT/JFs7zM=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=YpqwrVJO0ZFHJVbUdPcRL0O4qMa9xPis94bngKUfFwn6eL9AFT8tJ6j0VfyB2EDHt\n\tm3sIZtDBtZ6l1elQTvESEvSbrod5Y/SZLB8IFrfd5/0ZXbJZuqrjFj8c9Zophe+Jyt\n\tmEbW9KY7LphvgKGvdq80W58HqI+Yw6iFe7hkpbZHFBb8/7BB0wx+gqBjd6mhaKlH20\n\tyHc2mcHKxQNd/QYXI8Ob6ZaTqKZS4UvU68ATuCSD0xlesITKY7qqmlJYnv++uA7nAc\n\t5+Pkx+ijS/aQWLlj+tnqyW+WEheZ9eWnJcNqc88Cmh9bW21TMSAFwcWfEt69GTJ3sZ\n\tC536oFMSSYjYQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1671525263;\n\tbh=912VOxWzANtvRDStB1+X/jsMZs+QfMuhwAgT/JFs7zM=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=me98YDZaogEgn06UkwDwzNX9mJFOT5jiv6QfoNuywbz+IqhdhSZHD54X4I+GBBDXQ\n\tlV+w55yA6DW0wtBBuN4dHrfQRHPpBSCAQGCjjGHBzf77wsSNI8PGs09jZZRmCPFkmD\n\tkUQzI9HCT3c23U9ZnKgxiGGryYoB/UG3JV5ZWsvw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"me98YDZa\"; dkim-atps=neutral","Message-ID":"<1a3f27d9-16d9-8d06-a55c-aa48b1a188d6@ideasonboard.com>","Date":"Tue, 20 Dec 2022 14:04:15 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.5.1","To":"Paul Elder ,\n\tJacopo Mondi ","References":"<20221216122939.256534-1-paul.elder@ideasonboard.com>\n\t<20221216122939.256534-3-paul.elder@ideasonboard.com>\n\t<20221216140059.gmhxp6w52q6gqrxf@uno.localdomain>\n\t","Content-Language":"en-US","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v9 02/18] libcamera: property: Add\n\tMinimumRequests 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","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26116,"web_url":"https://patchwork.libcamera.org/comment/26116/","msgid":"","date":"2022-12-20T09:04:31","subject":"Re: [libcamera-devel] [PATCH v9 02/18] libcamera: property: Add\n\tMinimumRequests property","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Tue, Dec 20, 2022 at 02:04:15PM +0530, Umang Jain wrote:\n> Hi Paul,\n> \n> On 12/20/22 1:51 PM, Paul Elder via libcamera-devel wrote:\n> > Hi Jacopo,\n> > \n> > On Fri, Dec 16, 2022 at 03:00:59PM +0100, Jacopo Mondi wrote:\n> > > Hi Paul\n> > > \n> > > On Fri, Dec 16, 2022 at 09:29:23PM +0900, Paul Elder via libcamera-devel 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> > > > Signed-off-by: Nícolas F. R. A. Prado \n> > > > Reviewed-by: Laurent Pinchart \n> > > > Reviewed-by: Paul Elder \n> > > > Signed-off-by: Paul Elder \n> > > > \n> > > Seems quite on-spot considering the thread Umang has just started\n> > > \n> > > To be honest I would have expected this to generate more discussion,\n> > > but seeing Laurent's tag there makes me presume this has been blessed\n> > > already during the previous review.\n> > > \n> > > \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 | 22 +++++++---\n> > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +\n> > > > .../pipeline/raspberrypi/raspberrypi.cpp | 2 +\n> > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +\n> > > > src/libcamera/pipeline/simple/simple.cpp | 40 +++++++++++++++++--\n> > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +\n> > > > src/libcamera/pipeline/vimc/vimc.cpp | 2 +\n> > > > src/libcamera/property_ids.yaml | 21 ++++++++++\n> > > > 8 files changed, 84 insertions(+), 9 deletions(-)\n> > > > \n> > > > diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst\n> > > > index e1930fdf..a7356e4a 100644\n> > > > --- a/Documentation/guides/pipeline-handler.rst\n> > > > +++ b/Documentation/guides/pipeline-handler.rst\n> > > > @@ -658,19 +658,31 @@ associated with immutable values, which represent static characteristics that ca\n> > > > be used by applications to identify camera devices in the system. Properties can be\n> > > > registered by inspecting the values of V4L2 controls from the video devices and\n> > > > camera sensor (for example to retrieve the position and orientation of a camera)\n> > > > -or to express other immutable characteristics. The example pipeline handler does\n> > > > -not register any property, but examples are available in the libcamera code\n> > > > -base.\n> > > > +or to express other immutable characteristics.\n> > > > \n> > > > -.. TODO: Add a property example to the pipeline handler. At least the model.\n> > > > +A required property is ``MinimumRequests``, which indicates how many requests\n> > > > +need to be queued in the pipeline for capture without frame drops to be\n> > > > +possible.\n> > > > +\n> > > > +In our case, the vivid driver requires two buffers before it'll start streaming\n> > > > +(can be seen in the ``min_buffers_needed`` property for the ``vid_cap`` queue in\n> > > > +vivid's driver code). In order to not drop frames we should have one extra\n> > > > +buffer queued to the driver so that it is used as soon as the previous buffer\n> > > > +completes. Therefore we will set our ``MinimumRequests`` to three. Append the\n> > > > +following line to ``init()``:\n> > > The driver requires two buffers to operate, why an extra one is needed ?\n> > > For vivid in particular, where buffer's content is generated..\n> > I have a feeling that the intention was a spare buffer floating around\n> > in the application, but given the points that you've identified below,\n> > also maybe not.\n> > \n> > Also I feel like this description is mixing up Requests and buffers. I\n> > guess I should've vetted it a bit more (I was too distracted by merge\n> > conflicts).\n> > \n> > > > +\n> > > > +.. code-block:: cpp\n> > > > +\n> > > > + properties_.set(properties::MinimumRequests, 3);\n> > > > \n> > > > At this point you need to add the following includes to the top of the file for\n> > > > -handling controls:\n> > > > +handling controls and properties:\n> > > > \n> > > > .. code-block:: cpp\n> > > > \n> > > > #include \n> > > > #include \n> > > > + #include \n> > > > \n> > > > Generating a default configuration\n> > > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n> > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > index e4d79ea4..98a4a3e5 100644\n> > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > @@ -1126,6 +1126,8 @@ int PipelineHandlerIPU3::registerCameras()\n> > > > \t\t/* Initialize the camera properties. */\n> > > > \t\tdata->properties_ = cio2->sensor()->properties();\n> > > > \n> > > > +\t\tdata->properties_.set(properties::MinimumRequests, 3);\n> > > > +\n> > > I see\n> > > \n> > > static constexpr unsigned int kBufferCount = 4;\n> > > \n> > > Which seems to suggest that at least 4 buffers need to be queued to\n> > > the video device before streaming is started.\n> > > \n> > > I wonder if I'm confusing the two concepts\n> > Gah, the ipu3 code is so convoluted.\n> > \n> > kBufferCount is the number of internal buffers to allocate on the CIO2.\n> \n> I see kBufferCount being set to non-raw cfg->bufferCount in\n> IPU3CameraConfiguration::validate()\n\n...I don't see it\n\n> > As far as I understand, they're only used if no buffer is provided in\n> > the Request.\n> > \n> > So yes, I think you are confusing the two concepts. MinimumRequests is\n> > the minumum number of Requests that need to be queued in order to\n> > sustain capture without frame drops. kBufferCount is the number of\n> > internal buffers to allocate on the CIO2.\n> > \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 8569df17..4a08d01e 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > @@ -1063,6 +1063,8 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> > > > \t/* Enable SOF event generation. */\n> > > > \tdata->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);\n> > > > \n> > > > +\tdata->properties_.set(properties::MinimumRequests, 3);\n> > > > +\n> > > > \t/*\n> > > > \t * Reset the delayed controls with the gain and exposure values set by\n> > > > \t * the IPA.\n> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > index f658d75e..45b6beaf 100644\n> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > @@ -23,6 +23,7 @@\n> > > > #include \n> > > > #include \n> > > > #include \n> > > > +#include \n> > > > #include \n> > > > #include \n> > > > #include \n> > > > @@ -1102,6 +1103,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > > > \n> > > > \t/* Initialize the camera properties. */\n> > > > \tdata->properties_ = data->sensor_->properties();\n> > > > +\tdata->properties_.set(properties::MinimumRequests, 3);\n> > > Same question for RkISP1, it seems we don't even populate\n> > > StreamConfiguration::bufferCount there, but I'm pretty sure you need 4\n> > > requests queued before capture can start..\n\nYou only need 3.\n\n> > I see internal buffers but only for params and stats...\n> > \n> > I guess I have to check this again too then.\n> \n> I think MinimumRequests and minimum no. of buffers required to start and\n> sustain capture - are closely related. More so, because you can't queue\n> buffers arbitrarily (from application's PoV) - capture buffers come _via_\n> queuing requests.\n\nThat's true.\n\n> \n> The internal buffers, I always understood as something not provided by\n> applications like stats and params - completely internal to libcamera. The\n> minimumRequests property/criteria should not hinder/apply to internal\n> buffers right ?\n\nYeah, they shouldn't apply.\n\n\nPaul\n\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 24ded4db..8ed983fe 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> > > > @@ -180,6 +181,10 @@ class SimplePipelineHandler;\n> > > > \n> > > > struct SimplePipelineInfo {\n> > > > \tconst char *driver;\n> > > > +\t/*\n> > > > +\t * Minimum number of buffers required by the driver to start streaming.\n> > > > +\t */\n> > > > +\tunsigned int minimumBuffers;\n> > > > \t/*\n> > > > \t * Each converter in the list contains the name\n> > > > \t * and the number of streams it supports.\n> > > > @@ -190,10 +195,10 @@ struct SimplePipelineInfo {\n> > > > namespace {\n> > > > \n> > > > static const SimplePipelineInfo supportedDevices[] = {\n> > > > -\t{ \"imx7-csi\", { { \"pxp\", 1 } } },\n> > > > -\t{ \"mxc-isi\", {} },\n> > > > -\t{ \"qcom-camss\", {} },\n> > > > -\t{ \"sun6i-csi\", {} },\n> > > > +\t{ \"imx7-csi\", 2, { { \"pxp\", 1 } } },\n> > > > +\t{ \"mxc-isi\", 3, {} },\n> > > > +\t{ \"qcom-camss\", 1, {} },\n> > > > +\t{ \"sun6i-csi\", 3, {} },\n> > > > };\n> > > > \n> > > > } /* namespace */\n> > > > @@ -271,6 +276,8 @@ public:\n> > > > \tbool useConverter_;\n> > > > \tstd::queue> converterQueue_;\n> > > > \n> > > > +\tconst SimplePipelineInfo *deviceInfo;\n> > > > +\n> > > > private:\n> > > > \tvoid tryPipeline(unsigned int code, const Size &size);\n> > > > \tstatic std::vector routedSourcePads(MediaPad *sink);\n> > > > @@ -1168,6 +1175,29 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n> > > > \tinputCfg.stride = captureFormat.planes[0].bpl;\n> > > > \tinputCfg.bufferCount = kNumInternalBuffers;\n> > > > \n> > > > +\t/* Set the MinimumRequests property. */\n> > > > +\tunsigned int minimumRequests;\n> > > > +\n> > > > +\tif (data->useConverter_) {\n> > > > +\t\t/*\n> > > > +\t\t * The application will interact only with the capture node of\n> > > > +\t\t * the converter. Require two buffers for a frame drop free\n> > > > +\t\t * conversion, plus one extra to account for requeue delays.\n> > > > +\t\t */\n> > > > +\t\tminimumRequests = 3;\n> > > > +\t} else {\n> > > > +\t\t/*\n> > > > +\t\t * The application will interact directly with the video capture\n> > > > +\t\t * device. Require the minimum required by the driver, plus one\n> > > > +\t\t * extra to account for requeue delays. Force at least three\n> > > > +\t\t * buffers in order to not drop frames.\n> > > > +\t\t */\n> > > > +\t\tminimumRequests = std::max(data->deviceInfo->minimumBuffers + 1,\n> > > > +\t\t\t\t\t 3U);\n> > > > +\t}\n> > > > +\n> > > > +\tdata->properties_.set(properties::MinimumRequests, minimumRequests);\n> > > > +\n> > > > \treturn data->converter_->configure(inputCfg, outputCfgs);\n> > > > }\n> > > > \n> > > > @@ -1506,6 +1536,8 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)\n> > > > \tbool registered = false;\n> > > > \n> > > > \tfor (std::unique_ptr &data : pipelines) {\n> > > > +\t\tdata->deviceInfo = info;\n> > > > +\n> > > > \t\tint ret = data->init();\n> > > > \t\tif (ret < 0)\n> > > > \t\t\tcontinue;\n> > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > > index 277465b7..7f580955 100644\n> > > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > > @@ -500,6 +500,8 @@ int UVCCameraData::init(MediaDevice *media)\n> > > > \tproperties_.set(properties::PixelArraySize, resolution);\n> > > > \tproperties_.set(properties::PixelArrayActiveAreas, { Rectangle(resolution) });\n> > > > \n> > > > +\tproperties_.set(properties::MinimumRequests, 3);\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 204f5ad7..d2633be4 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> > > > @@ -571,6 +572,7 @@ int VimcCameraData::init()\n> > > > \n> > > > \t/* Initialize the camera properties. */\n> > > > \tproperties_ = sensor_->properties();\n> > > > +\tproperties_.set(properties::MinimumRequests, 3);\n> > > > \n> > > > \treturn 0;\n> > > > }\n> > > > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> > > > index cb55e0ed..c82ac17e 100644\n> > > > --- a/src/libcamera/property_ids.yaml\n> > > > +++ b/src/libcamera/property_ids.yaml\n> > > > @@ -690,6 +690,27 @@ controls:\n> > > > that is twice that of the full resolution mode. This value will be valid\n> > > > after the configure method has returned successfully.\n> > > > \n> > > > + - MinimumRequests:\n> > > > + type: int32_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 without frame\n> > > > + drops. At this quantity, there's no guarantee that manual per-frame\n> > > > + controls will apply correctly.\n> > > This property is also relevant for the startup phase, so mentioning\n> > > that this is the minium number of requests that need to be queued\n> > > before capture starts might be necessary ?\n> > Indeed.\n> > \n> > \n> > Thanks,\n> > \n> > Paul\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> > > > # Draft properties section\n> > > > \n> > > > --\n> > > > 2.35.1\n> > > > \n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5DB13C3200\n\tfor ;\n\tTue, 20 Dec 2022 09:04:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E602A6339B;\n\tTue, 20 Dec 2022 10:04:41 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 335CC63397\n\tfor ;\n\tTue, 20 Dec 2022 10:04:40 +0100 (CET)","from pyrite.rasen.tech (h175-177-042-159.catv02.itscom.jp\n\t[175.177.42.159])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6D4B6706;\n\tTue, 20 Dec 2022 10:04:38 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1671527081;\n\tbh=SNebUkrUO4Z6cX3m+AddxHSZvFCcC8048ilLL0rsnss=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=vsqLyWFSK5U8iPD4aAg046R6kXdJt1aMCiYz03DN0i/Q08X7QGwa2fVjR8nJv/QRM\n\theqUICJg8a1kwXUe/HfyErxHPNVxjZN8tyOHTRfWbGPBWVcQ743RUl+6l7wJPtpG6P\n\tT7ktJ210U4kq8oYs6FzMEcDgDnLjET6X9Uea3Xy6nuYMFUtGVlj5lhK9XcDFi+exO+\n\t9DTnMa8/nhEoArNtkwSfrlD75acO6J7aIhhS1N9ZlCF5EyPNSTEKw+fP3/2XJDUIeY\n\t4KsdT6PVSUjRG7TidiX6uJXCUQ4I2wg8Ayce8TABINFsbpFw5WpvthcmU8KGqCZuEA\n\t1QNH6tfWlJweg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1671527079;\n\tbh=SNebUkrUO4Z6cX3m+AddxHSZvFCcC8048ilLL0rsnss=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MBkgn61Mm19QSJFtTAlYidXkuywPT7H7ruH7OaBp+FDx2z5RKFbBr933CjnncT1EE\n\tk78Ha6nJ1kwUTrfd/ctmq0EX6HCJME2DPKtwof2+yJCEDKoC+xv2qwg+rE2GjcAT7i\n\t0e4CZYMJdPhs0Zl7b9JuHnsf8yyFkIpQ8a1mpRx0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"MBkgn61M\"; dkim-atps=neutral","Date":"Tue, 20 Dec 2022 18:04:31 +0900","To":"Umang Jain ","Message-ID":"","References":"<20221216122939.256534-1-paul.elder@ideasonboard.com>\n\t<20221216122939.256534-3-paul.elder@ideasonboard.com>\n\t<20221216140059.gmhxp6w52q6gqrxf@uno.localdomain>\n\t\n\t<1a3f27d9-16d9-8d06-a55c-aa48b1a188d6@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<1a3f27d9-16d9-8d06-a55c-aa48b1a188d6@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v9 02/18] libcamera: property: Add\n\tMinimumRequests 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","From":"Paul Elder via libcamera-devel ","Reply-To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26117,"web_url":"https://patchwork.libcamera.org/comment/26117/","msgid":"<0d83acdf-c062-4419-4fe5-27520e1200d2@ideasonboard.com>","date":"2022-12-20T09:33:26","subject":"Re: [libcamera-devel] [PATCH v9 02/18] libcamera: property: Add\n\tMinimumRequests property","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Paul,\n\nOn 12/20/22 2:34 PM, Paul Elder wrote:\n> On Tue, Dec 20, 2022 at 02:04:15PM +0530, Umang Jain wrote:\n>> Hi Paul,\n>>\n>> On 12/20/22 1:51 PM, Paul Elder via libcamera-devel wrote:\n>>> Hi Jacopo,\n>>>\n>>> On Fri, Dec 16, 2022 at 03:00:59PM +0100, Jacopo Mondi wrote:\n>>>> Hi Paul\n>>>>\n>>>> On Fri, Dec 16, 2022 at 09:29:23PM +0900, Paul Elder via libcamera-devel 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>>>>> Signed-off-by: Nícolas F. R. A. Prado \n>>>>> Reviewed-by: Laurent Pinchart \n>>>>> Reviewed-by: Paul Elder \n>>>>> Signed-off-by: Paul Elder \n>>>>>\n>>>> Seems quite on-spot considering the thread Umang has just started\n>>>>\n>>>> To be honest I would have expected this to generate more discussion,\n>>>> but seeing Laurent's tag there makes me presume this has been blessed\n>>>> already during the previous review.\n>>>>\n>>>>\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 | 22 +++++++---\n>>>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +\n>>>>> .../pipeline/raspberrypi/raspberrypi.cpp | 2 +\n>>>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +\n>>>>> src/libcamera/pipeline/simple/simple.cpp | 40 +++++++++++++++++--\n>>>>> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +\n>>>>> src/libcamera/pipeline/vimc/vimc.cpp | 2 +\n>>>>> src/libcamera/property_ids.yaml | 21 ++++++++++\n>>>>> 8 files changed, 84 insertions(+), 9 deletions(-)\n>>>>>\n>>>>> diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst\n>>>>> index e1930fdf..a7356e4a 100644\n>>>>> --- a/Documentation/guides/pipeline-handler.rst\n>>>>> +++ b/Documentation/guides/pipeline-handler.rst\n>>>>> @@ -658,19 +658,31 @@ associated with immutable values, which represent static characteristics that ca\n>>>>> be used by applications to identify camera devices in the system. Properties can be\n>>>>> registered by inspecting the values of V4L2 controls from the video devices and\n>>>>> camera sensor (for example to retrieve the position and orientation of a camera)\n>>>>> -or to express other immutable characteristics. The example pipeline handler does\n>>>>> -not register any property, but examples are available in the libcamera code\n>>>>> -base.\n>>>>> +or to express other immutable characteristics.\n>>>>>\n>>>>> -.. TODO: Add a property example to the pipeline handler. At least the model.\n>>>>> +A required property is ``MinimumRequests``, which indicates how many requests\n>>>>> +need to be queued in the pipeline for capture without frame drops to be\n>>>>> +possible.\n>>>>> +\n>>>>> +In our case, the vivid driver requires two buffers before it'll start streaming\n>>>>> +(can be seen in the ``min_buffers_needed`` property for the ``vid_cap`` queue in\n>>>>> +vivid's driver code). In order to not drop frames we should have one extra\n>>>>> +buffer queued to the driver so that it is used as soon as the previous buffer\n>>>>> +completes. Therefore we will set our ``MinimumRequests`` to three. Append the\n>>>>> +following line to ``init()``:\n>>>> The driver requires two buffers to operate, why an extra one is needed ?\n>>>> For vivid in particular, where buffer's content is generated..\n>>> I have a feeling that the intention was a spare buffer floating around\n>>> in the application, but given the points that you've identified below,\n>>> also maybe not.\n>>>\n>>> Also I feel like this description is mixing up Requests and buffers. I\n>>> guess I should've vetted it a bit more (I was too distracted by merge\n>>> conflicts).\n>>>\n>>>>> +\n>>>>> +.. code-block:: cpp\n>>>>> +\n>>>>> + properties_.set(properties::MinimumRequests, 3);\n>>>>>\n>>>>> At this point you need to add the following includes to the top of the file for\n>>>>> -handling controls:\n>>>>> +handling controls and properties:\n>>>>>\n>>>>> .. code-block:: cpp\n>>>>>\n>>>>> #include \n>>>>> #include \n>>>>> + #include \n>>>>>\n>>>>> Generating a default configuration\n>>>>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n>>>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>>> index e4d79ea4..98a4a3e5 100644\n>>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>>>> @@ -1126,6 +1126,8 @@ int PipelineHandlerIPU3::registerCameras()\n>>>>> \t\t/* Initialize the camera properties. */\n>>>>> \t\tdata->properties_ = cio2->sensor()->properties();\n>>>>>\n>>>>> +\t\tdata->properties_.set(properties::MinimumRequests, 3);\n>>>>> +\n>>>> I see\n>>>>\n>>>> static constexpr unsigned int kBufferCount = 4;\n>>>>\n>>>> Which seems to suggest that at least 4 buffers need to be queued to\n>>>> the video device before streaming is started.\n>>>>\n>>>> I wonder if I'm confusing the two concepts\n>>> Gah, the ipu3 code is so convoluted.\n>>>\n>>> kBufferCount is the number of internal buffers to allocate on the CIO2.\n>> I see kBufferCount being set to non-raw cfg->bufferCount in\n>> IPU3CameraConfiguration::validate()\n> ...I don't see it\n\nhttps://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/ipu3/ipu3.cpp#n371\n>\n>>> As far as I understand, they're only used if no buffer is provided in\n>>> the Request.\n>>>\n>>> So yes, I think you are confusing the two concepts. MinimumRequests is\n>>> the minumum number of Requests that need to be queued in order to\n>>> sustain capture without frame drops. kBufferCount is the number of\n>>> internal buffers to allocate on the CIO2.\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 8569df17..4a08d01e 100644\n>>>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>>>> @@ -1063,6 +1063,8 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n>>>>> \t/* Enable SOF event generation. */\n>>>>> \tdata->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);\n>>>>>\n>>>>> +\tdata->properties_.set(properties::MinimumRequests, 3);\n>>>>> +\n>>>>> \t/*\n>>>>> \t * Reset the delayed controls with the gain and exposure values set by\n>>>>> \t * the IPA.\n>>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>>> index f658d75e..45b6beaf 100644\n>>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>>> @@ -23,6 +23,7 @@\n>>>>> #include \n>>>>> #include \n>>>>> #include \n>>>>> +#include \n>>>>> #include \n>>>>> #include \n>>>>> #include \n>>>>> @@ -1102,6 +1103,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n>>>>>\n>>>>> \t/* Initialize the camera properties. */\n>>>>> \tdata->properties_ = data->sensor_->properties();\n>>>>> +\tdata->properties_.set(properties::MinimumRequests, 3);\n>>>> Same question for RkISP1, it seems we don't even populate\n>>>> StreamConfiguration::bufferCount there, but I'm pretty sure you need 4\n>>>> requests queued before capture can start..\n> You only need 3.\n>\n>>> I see internal buffers but only for params and stats...\n>>>\n>>> I guess I have to check this again too then.\n>> I think MinimumRequests and minimum no. of buffers required to start and\n>> sustain capture - are closely related. More so, because you can't queue\n>> buffers arbitrarily (from application's PoV) - capture buffers come _via_\n>> queuing requests.\n> That's true.\n>\n>> The internal buffers, I always understood as something not provided by\n>> applications like stats and params - completely internal to libcamera. The\n>> minimumRequests property/criteria should not hinder/apply to internal\n>> buffers right ?\n> Yeah, they shouldn't apply.\n>\n>\n> Paul\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 24ded4db..8ed983fe 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>>>>> @@ -180,6 +181,10 @@ class SimplePipelineHandler;\n>>>>>\n>>>>> struct SimplePipelineInfo {\n>>>>> \tconst char *driver;\n>>>>> +\t/*\n>>>>> +\t * Minimum number of buffers required by the driver to start streaming.\n>>>>> +\t */\n>>>>> +\tunsigned int minimumBuffers;\n>>>>> \t/*\n>>>>> \t * Each converter in the list contains the name\n>>>>> \t * and the number of streams it supports.\n>>>>> @@ -190,10 +195,10 @@ struct SimplePipelineInfo {\n>>>>> namespace {\n>>>>>\n>>>>> static const SimplePipelineInfo supportedDevices[] = {\n>>>>> -\t{ \"imx7-csi\", { { \"pxp\", 1 } } },\n>>>>> -\t{ \"mxc-isi\", {} },\n>>>>> -\t{ \"qcom-camss\", {} },\n>>>>> -\t{ \"sun6i-csi\", {} },\n>>>>> +\t{ \"imx7-csi\", 2, { { \"pxp\", 1 } } },\n>>>>> +\t{ \"mxc-isi\", 3, {} },\n>>>>> +\t{ \"qcom-camss\", 1, {} },\n>>>>> +\t{ \"sun6i-csi\", 3, {} },\n>>>>> };\n>>>>>\n>>>>> } /* namespace */\n>>>>> @@ -271,6 +276,8 @@ public:\n>>>>> \tbool useConverter_;\n>>>>> \tstd::queue> converterQueue_;\n>>>>>\n>>>>> +\tconst SimplePipelineInfo *deviceInfo;\n>>>>> +\n>>>>> private:\n>>>>> \tvoid tryPipeline(unsigned int code, const Size &size);\n>>>>> \tstatic std::vector routedSourcePads(MediaPad *sink);\n>>>>> @@ -1168,6 +1175,29 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n>>>>> \tinputCfg.stride = captureFormat.planes[0].bpl;\n>>>>> \tinputCfg.bufferCount = kNumInternalBuffers;\n>>>>>\n>>>>> +\t/* Set the MinimumRequests property. */\n>>>>> +\tunsigned int minimumRequests;\n>>>>> +\n>>>>> +\tif (data->useConverter_) {\n>>>>> +\t\t/*\n>>>>> +\t\t * The application will interact only with the capture node of\n>>>>> +\t\t * the converter. Require two buffers for a frame drop free\n>>>>> +\t\t * conversion, plus one extra to account for requeue delays.\n>>>>> +\t\t */\n>>>>> +\t\tminimumRequests = 3;\n>>>>> +\t} else {\n>>>>> +\t\t/*\n>>>>> +\t\t * The application will interact directly with the video capture\n>>>>> +\t\t * device. Require the minimum required by the driver, plus one\n>>>>> +\t\t * extra to account for requeue delays. Force at least three\n>>>>> +\t\t * buffers in order to not drop frames.\n>>>>> +\t\t */\n>>>>> +\t\tminimumRequests = std::max(data->deviceInfo->minimumBuffers + 1,\n>>>>> +\t\t\t\t\t 3U);\n>>>>> +\t}\n>>>>> +\n>>>>> +\tdata->properties_.set(properties::MinimumRequests, minimumRequests);\n>>>>> +\n>>>>> \treturn data->converter_->configure(inputCfg, outputCfgs);\n>>>>> }\n>>>>>\n>>>>> @@ -1506,6 +1536,8 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)\n>>>>> \tbool registered = false;\n>>>>>\n>>>>> \tfor (std::unique_ptr &data : pipelines) {\n>>>>> +\t\tdata->deviceInfo = info;\n>>>>> +\n>>>>> \t\tint ret = data->init();\n>>>>> \t\tif (ret < 0)\n>>>>> \t\t\tcontinue;\n>>>>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>>>>> index 277465b7..7f580955 100644\n>>>>> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>>>>> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>>>>> @@ -500,6 +500,8 @@ int UVCCameraData::init(MediaDevice *media)\n>>>>> \tproperties_.set(properties::PixelArraySize, resolution);\n>>>>> \tproperties_.set(properties::PixelArrayActiveAreas, { Rectangle(resolution) });\n>>>>>\n>>>>> +\tproperties_.set(properties::MinimumRequests, 3);\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 204f5ad7..d2633be4 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>>>>> @@ -571,6 +572,7 @@ int VimcCameraData::init()\n>>>>>\n>>>>> \t/* Initialize the camera properties. */\n>>>>> \tproperties_ = sensor_->properties();\n>>>>> +\tproperties_.set(properties::MinimumRequests, 3);\n>>>>>\n>>>>> \treturn 0;\n>>>>> }\n>>>>> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n>>>>> index cb55e0ed..c82ac17e 100644\n>>>>> --- a/src/libcamera/property_ids.yaml\n>>>>> +++ b/src/libcamera/property_ids.yaml\n>>>>> @@ -690,6 +690,27 @@ controls:\n>>>>> that is twice that of the full resolution mode. This value will be valid\n>>>>> after the configure method has returned successfully.\n>>>>>\n>>>>> + - MinimumRequests:\n>>>>> + type: int32_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 without frame\n>>>>> + drops. At this quantity, there's no guarantee that manual per-frame\n>>>>> + controls will apply correctly.\n>>>> This property is also relevant for the startup phase, so mentioning\n>>>> that this is the minium number of requests that need to be queued\n>>>> before capture starts might be necessary ?\n>>> Indeed.\n>>>\n>>>\n>>> Thanks,\n>>>\n>>> Paul\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>>>>> # Draft properties section\n>>>>>\n>>>>> --\n>>>>> 2.35.1\n>>>>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0C66EC3213\n\tfor ;\n\tTue, 20 Dec 2022 09:33:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 795D6633A2;\n\tTue, 20 Dec 2022 10:33:34 +0100 (CET)","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 9FBFC63397\n\tfor ;\n\tTue, 20 Dec 2022 10:33:33 +0100 (CET)","from [IPV6:2401:4900:1f3e:7d24:3f0:3e81:fb16:ab4d] (unknown\n\t[IPv6:2401:4900:1f3e:7d24:3f0:3e81:fb16:ab4d])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 91EA2706;\n\tTue, 20 Dec 2022 10:33:31 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1671528814;\n\tbh=pz8VJWpCDKEGfKY/yBc1nJeCxyYui8DngV2Ccp88bgA=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=DxlMn4I/CVpCzGvlIoZAJA7K/fsgOYEAYR4Ife+Ztor6fEDUR9WxDf7vIrIs7XFZ1\n\ttLsOIKavSTaB0LVopJ+5fCca2/n4Q/j24/K7DJSH5UAqWGyL5L8WTtMdTeQV3CuhZi\n\tNW0y3poB0bAZxWKPcNMsJDfhUYiWG22dtozEGRM/s6+K1vhzRGFF1Pv92BWkjECLy4\n\tJf1nkgouUv5o631HO0SvCt5VuK+M0zIZq1SFTeaDhxfjcIbIQ5+sv0wXV3E5/cGnW1\n\tm4Gc6pMA/O+mJFC4DLuavEwYSFUNNK8mQ/38AmSnvjgJlj2JMsxakTK38/54nlUpct\n\tOU5rwUvDIvzIw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1671528813;\n\tbh=pz8VJWpCDKEGfKY/yBc1nJeCxyYui8DngV2Ccp88bgA=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=Y7hT2pZKaWTQ5CJLfQs6rCgexomy6xqQA449WuBh8FWU72O2mS8lxaNsCvb9n/Ojz\n\tXJ6jyFSmn8xzlg2JkERRJIeL/ITwp3vRCUKnd11IFHR0z5rc5VcWVX1e85kkrOaXm7\n\tpWVt+xCz6JfL/vEzzBnxhXBUvdUGVRGTnxPDX2KQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Y7hT2pZK\"; dkim-atps=neutral","Message-ID":"<0d83acdf-c062-4419-4fe5-27520e1200d2@ideasonboard.com>","Date":"Tue, 20 Dec 2022 15:03:26 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.5.1","Content-Language":"en-US","To":"Paul Elder ","References":"<20221216122939.256534-1-paul.elder@ideasonboard.com>\n\t<20221216122939.256534-3-paul.elder@ideasonboard.com>\n\t<20221216140059.gmhxp6w52q6gqrxf@uno.localdomain>\n\t\n\t<1a3f27d9-16d9-8d06-a55c-aa48b1a188d6@ideasonboard.com>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v9 02/18] libcamera: property: Add\n\tMinimumRequests 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","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26120,"web_url":"https://patchwork.libcamera.org/comment/26120/","msgid":"","date":"2022-12-21T07:13:38","subject":"Re: [libcamera-devel] [PATCH v9 02/18] libcamera: property: Add\n\tMinimumRequests property","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Umang,\n\nOn Tue, Dec 20, 2022 at 03:03:26PM +0530, Umang Jain wrote:\n> Hi Paul,\n> \n> On 12/20/22 2:34 PM, Paul Elder wrote:\n> > On Tue, Dec 20, 2022 at 02:04:15PM +0530, Umang Jain wrote:\n> > > Hi Paul,\n> > > \n> > > On 12/20/22 1:51 PM, Paul Elder via libcamera-devel wrote:\n> > > > Hi Jacopo,\n> > > > \n> > > > On Fri, Dec 16, 2022 at 03:00:59PM +0100, Jacopo Mondi wrote:\n> > > > > Hi Paul\n> > > > > \n> > > > > On Fri, Dec 16, 2022 at 09:29:23PM +0900, Paul Elder via libcamera-devel 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> > > > > > Signed-off-by: Nícolas F. R. A. Prado \n> > > > > > Reviewed-by: Laurent Pinchart \n> > > > > > Reviewed-by: Paul Elder \n> > > > > > Signed-off-by: Paul Elder \n> > > > > > \n> > > > > Seems quite on-spot considering the thread Umang has just started\n> > > > > \n> > > > > To be honest I would have expected this to generate more discussion,\n> > > > > but seeing Laurent's tag there makes me presume this has been blessed\n> > > > > already during the previous review.\n> > > > > \n> > > > > \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 | 22 +++++++---\n> > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +\n> > > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 2 +\n> > > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +\n> > > > > > src/libcamera/pipeline/simple/simple.cpp | 40 +++++++++++++++++--\n> > > > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +\n> > > > > > src/libcamera/pipeline/vimc/vimc.cpp | 2 +\n> > > > > > src/libcamera/property_ids.yaml | 21 ++++++++++\n> > > > > > 8 files changed, 84 insertions(+), 9 deletions(-)\n> > > > > > \n> > > > > > diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst\n> > > > > > index e1930fdf..a7356e4a 100644\n> > > > > > --- a/Documentation/guides/pipeline-handler.rst\n> > > > > > +++ b/Documentation/guides/pipeline-handler.rst\n> > > > > > @@ -658,19 +658,31 @@ associated with immutable values, which represent static characteristics that ca\n> > > > > > be used by applications to identify camera devices in the system. Properties can be\n> > > > > > registered by inspecting the values of V4L2 controls from the video devices and\n> > > > > > camera sensor (for example to retrieve the position and orientation of a camera)\n> > > > > > -or to express other immutable characteristics. The example pipeline handler does\n> > > > > > -not register any property, but examples are available in the libcamera code\n> > > > > > -base.\n> > > > > > +or to express other immutable characteristics.\n> > > > > > \n> > > > > > -.. TODO: Add a property example to the pipeline handler. At least the model.\n> > > > > > +A required property is ``MinimumRequests``, which indicates how many requests\n> > > > > > +need to be queued in the pipeline for capture without frame drops to be\n> > > > > > +possible.\n> > > > > > +\n> > > > > > +In our case, the vivid driver requires two buffers before it'll start streaming\n> > > > > > +(can be seen in the ``min_buffers_needed`` property for the ``vid_cap`` queue in\n> > > > > > +vivid's driver code). In order to not drop frames we should have one extra\n> > > > > > +buffer queued to the driver so that it is used as soon as the previous buffer\n> > > > > > +completes. Therefore we will set our ``MinimumRequests`` to three. Append the\n> > > > > > +following line to ``init()``:\n> > > > > The driver requires two buffers to operate, why an extra one is needed ?\n> > > > > For vivid in particular, where buffer's content is generated..\n> > > > I have a feeling that the intention was a spare buffer floating around\n> > > > in the application, but given the points that you've identified below,\n> > > > also maybe not.\n> > > > \n> > > > Also I feel like this description is mixing up Requests and buffers. I\n> > > > guess I should've vetted it a bit more (I was too distracted by merge\n> > > > conflicts).\n> > > > \n> > > > > > +\n> > > > > > +.. code-block:: cpp\n> > > > > > +\n> > > > > > + properties_.set(properties::MinimumRequests, 3);\n> > > > > > \n> > > > > > At this point you need to add the following includes to the top of the file for\n> > > > > > -handling controls:\n> > > > > > +handling controls and properties:\n> > > > > > \n> > > > > > .. code-block:: cpp\n> > > > > > \n> > > > > > #include \n> > > > > > #include \n> > > > > > + #include \n> > > > > > \n> > > > > > Generating a default configuration\n> > > > > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n> > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > index e4d79ea4..98a4a3e5 100644\n> > > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > @@ -1126,6 +1126,8 @@ int PipelineHandlerIPU3::registerCameras()\n> > > > > > \t\t/* Initialize the camera properties. */\n> > > > > > \t\tdata->properties_ = cio2->sensor()->properties();\n> > > > > > \n> > > > > > +\t\tdata->properties_.set(properties::MinimumRequests, 3);\n> > > > > > +\n> > > > > I see\n> > > > > \n> > > > > static constexpr unsigned int kBufferCount = 4;\n> > > > > \n> > > > > Which seems to suggest that at least 4 buffers need to be queued to\n> > > > > the video device before streaming is started.\n> > > > > \n> > > > > I wonder if I'm confusing the two concepts\n> > > > Gah, the ipu3 code is so convoluted.\n> > > > \n> > > > kBufferCount is the number of internal buffers to allocate on the CIO2.\n> > > I see kBufferCount being set to non-raw cfg->bufferCount in\n> > > IPU3CameraConfiguration::validate()\n> > ...I don't see it\n> \n> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/ipu3/ipu3.cpp#n371\n\nIt's removed in patch 10/18 \"libcamera: stream: Remove bufferCount\"\n\n\nPaul\n\n> > \n> > > > As far as I understand, they're only used if no buffer is provided in\n> > > > the Request.\n> > > > \n> > > > So yes, I think you are confusing the two concepts. MinimumRequests is\n> > > > the minumum number of Requests that need to be queued in order to\n> > > > sustain capture without frame drops. kBufferCount is the number of\n> > > > internal buffers to allocate on the CIO2.\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 8569df17..4a08d01e 100644\n> > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > @@ -1063,6 +1063,8 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> > > > > > \t/* Enable SOF event generation. */\n> > > > > > \tdata->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);\n> > > > > > \n> > > > > > +\tdata->properties_.set(properties::MinimumRequests, 3);\n> > > > > > +\n> > > > > > \t/*\n> > > > > > \t * Reset the delayed controls with the gain and exposure values set by\n> > > > > > \t * the IPA.\n> > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > > index f658d75e..45b6beaf 100644\n> > > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > > @@ -23,6 +23,7 @@\n> > > > > > #include \n> > > > > > #include \n> > > > > > #include \n> > > > > > +#include \n> > > > > > #include \n> > > > > > #include \n> > > > > > #include \n> > > > > > @@ -1102,6 +1103,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > > > > > \n> > > > > > \t/* Initialize the camera properties. */\n> > > > > > \tdata->properties_ = data->sensor_->properties();\n> > > > > > +\tdata->properties_.set(properties::MinimumRequests, 3);\n> > > > > Same question for RkISP1, it seems we don't even populate\n> > > > > StreamConfiguration::bufferCount there, but I'm pretty sure you need 4\n> > > > > requests queued before capture can start..\n> > You only need 3.\n> > \n> > > > I see internal buffers but only for params and stats...\n> > > > \n> > > > I guess I have to check this again too then.\n> > > I think MinimumRequests and minimum no. of buffers required to start and\n> > > sustain capture - are closely related. More so, because you can't queue\n> > > buffers arbitrarily (from application's PoV) - capture buffers come _via_\n> > > queuing requests.\n> > That's true.\n> > \n> > > The internal buffers, I always understood as something not provided by\n> > > applications like stats and params - completely internal to libcamera. The\n> > > minimumRequests property/criteria should not hinder/apply to internal\n> > > buffers right ?\n> > Yeah, they shouldn't apply.\n> > \n> > \n> > Paul\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 24ded4db..8ed983fe 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> > > > > > @@ -180,6 +181,10 @@ class SimplePipelineHandler;\n> > > > > > \n> > > > > > struct SimplePipelineInfo {\n> > > > > > \tconst char *driver;\n> > > > > > +\t/*\n> > > > > > +\t * Minimum number of buffers required by the driver to start streaming.\n> > > > > > +\t */\n> > > > > > +\tunsigned int minimumBuffers;\n> > > > > > \t/*\n> > > > > > \t * Each converter in the list contains the name\n> > > > > > \t * and the number of streams it supports.\n> > > > > > @@ -190,10 +195,10 @@ struct SimplePipelineInfo {\n> > > > > > namespace {\n> > > > > > \n> > > > > > static const SimplePipelineInfo supportedDevices[] = {\n> > > > > > -\t{ \"imx7-csi\", { { \"pxp\", 1 } } },\n> > > > > > -\t{ \"mxc-isi\", {} },\n> > > > > > -\t{ \"qcom-camss\", {} },\n> > > > > > -\t{ \"sun6i-csi\", {} },\n> > > > > > +\t{ \"imx7-csi\", 2, { { \"pxp\", 1 } } },\n> > > > > > +\t{ \"mxc-isi\", 3, {} },\n> > > > > > +\t{ \"qcom-camss\", 1, {} },\n> > > > > > +\t{ \"sun6i-csi\", 3, {} },\n> > > > > > };\n> > > > > > \n> > > > > > } /* namespace */\n> > > > > > @@ -271,6 +276,8 @@ public:\n> > > > > > \tbool useConverter_;\n> > > > > > \tstd::queue> converterQueue_;\n> > > > > > \n> > > > > > +\tconst SimplePipelineInfo *deviceInfo;\n> > > > > > +\n> > > > > > private:\n> > > > > > \tvoid tryPipeline(unsigned int code, const Size &size);\n> > > > > > \tstatic std::vector routedSourcePads(MediaPad *sink);\n> > > > > > @@ -1168,6 +1175,29 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n> > > > > > \tinputCfg.stride = captureFormat.planes[0].bpl;\n> > > > > > \tinputCfg.bufferCount = kNumInternalBuffers;\n> > > > > > \n> > > > > > +\t/* Set the MinimumRequests property. */\n> > > > > > +\tunsigned int minimumRequests;\n> > > > > > +\n> > > > > > +\tif (data->useConverter_) {\n> > > > > > +\t\t/*\n> > > > > > +\t\t * The application will interact only with the capture node of\n> > > > > > +\t\t * the converter. Require two buffers for a frame drop free\n> > > > > > +\t\t * conversion, plus one extra to account for requeue delays.\n> > > > > > +\t\t */\n> > > > > > +\t\tminimumRequests = 3;\n> > > > > > +\t} else {\n> > > > > > +\t\t/*\n> > > > > > +\t\t * The application will interact directly with the video capture\n> > > > > > +\t\t * device. Require the minimum required by the driver, plus one\n> > > > > > +\t\t * extra to account for requeue delays. Force at least three\n> > > > > > +\t\t * buffers in order to not drop frames.\n> > > > > > +\t\t */\n> > > > > > +\t\tminimumRequests = std::max(data->deviceInfo->minimumBuffers + 1,\n> > > > > > +\t\t\t\t\t 3U);\n> > > > > > +\t}\n> > > > > > +\n> > > > > > +\tdata->properties_.set(properties::MinimumRequests, minimumRequests);\n> > > > > > +\n> > > > > > \treturn data->converter_->configure(inputCfg, outputCfgs);\n> > > > > > }\n> > > > > > \n> > > > > > @@ -1506,6 +1536,8 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)\n> > > > > > \tbool registered = false;\n> > > > > > \n> > > > > > \tfor (std::unique_ptr &data : pipelines) {\n> > > > > > +\t\tdata->deviceInfo = info;\n> > > > > > +\n> > > > > > \t\tint ret = data->init();\n> > > > > > \t\tif (ret < 0)\n> > > > > > \t\t\tcontinue;\n> > > > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > > > > index 277465b7..7f580955 100644\n> > > > > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > > > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > > > > @@ -500,6 +500,8 @@ int UVCCameraData::init(MediaDevice *media)\n> > > > > > \tproperties_.set(properties::PixelArraySize, resolution);\n> > > > > > \tproperties_.set(properties::PixelArrayActiveAreas, { Rectangle(resolution) });\n> > > > > > \n> > > > > > +\tproperties_.set(properties::MinimumRequests, 3);\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 204f5ad7..d2633be4 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> > > > > > @@ -571,6 +572,7 @@ int VimcCameraData::init()\n> > > > > > \n> > > > > > \t/* Initialize the camera properties. */\n> > > > > > \tproperties_ = sensor_->properties();\n> > > > > > +\tproperties_.set(properties::MinimumRequests, 3);\n> > > > > > \n> > > > > > \treturn 0;\n> > > > > > }\n> > > > > > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> > > > > > index cb55e0ed..c82ac17e 100644\n> > > > > > --- a/src/libcamera/property_ids.yaml\n> > > > > > +++ b/src/libcamera/property_ids.yaml\n> > > > > > @@ -690,6 +690,27 @@ controls:\n> > > > > > that is twice that of the full resolution mode. This value will be valid\n> > > > > > after the configure method has returned successfully.\n> > > > > > \n> > > > > > + - MinimumRequests:\n> > > > > > + type: int32_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 without frame\n> > > > > > + drops. At this quantity, there's no guarantee that manual per-frame\n> > > > > > + controls will apply correctly.\n> > > > > This property is also relevant for the startup phase, so mentioning\n> > > > > that this is the minium number of requests that need to be queued\n> > > > > before capture starts might be necessary ?\n> > > > Indeed.\n> > > > \n> > > > \n> > > > Thanks,\n> > > > \n> > > > Paul\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> > > > > > # Draft properties section\n> > > > > > \n> > > > > > --\n> > > > > > 2.35.1\n> > > > > > \n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5CADFC3213\n\tfor ;\n\tWed, 21 Dec 2022 07:13:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 94986633A7;\n\tWed, 21 Dec 2022 08:13:49 +0100 (CET)","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 C4BDB61F14\n\tfor ;\n\tWed, 21 Dec 2022 08:13:47 +0100 (CET)","from pyrite.rasen.tech (h175-177-042-159.catv02.itscom.jp\n\t[175.177.42.159])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 73DDAFB;\n\tWed, 21 Dec 2022 08:13:45 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1671606829;\n\tbh=hHtLmiFpWhgAdb9bfVQDaSIKAcDTrfjkZwt2fnDhWiQ=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=F/+jjqdrIfQD7AtyfgsFJ+x//LVyIxJiBnuUdIUzU/WclU9yXG6kppGFXa39d9sq9\n\tsUNBCaeVTLD4K98bVE53fwuh3a74LV6050kJMNCaCcSBl0/tndIzFEda4Vgy5UwaIC\n\twvIlbEU8l726R6SKofHnG8iLXy12/ZHY/v92jjVxesH+iiZICM2DNcwgjBfc99FRhP\n\tG4dCyL8oOaTl2il0/r8zftQkzuLYCSwDU7QVJsEaN8Luly1U4SZ/N1yrjcjCIQzAID\n\tEw2wJALHoJmnttE7NK8h+eFkbD/ImuCmQugZ6jp+vrbYvJ0SiTcCxoJEnJZxW3wt5B\n\tLvGZAka5XWRrw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1671606826;\n\tbh=hHtLmiFpWhgAdb9bfVQDaSIKAcDTrfjkZwt2fnDhWiQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ntpd9qeCGOV7yEPDLtGdphp4RmhmvzT/niTSLjEx8pjOO0pbeJyU4JJhvU/AHISwa\n\tamvI/rbLwUK5fEUFYc/z0FxRt7gsc5U5JAOaLRbHA4gy3NigaANFPEebehJ9+dfeIq\n\tNEbDARewMvW/UF9/Zz6cstqf8OKhxDruemvEda+E="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ntpd9qeC\"; dkim-atps=neutral","Date":"Wed, 21 Dec 2022 16:13:38 +0900","To":"Umang Jain ","Message-ID":"","References":"<20221216122939.256534-1-paul.elder@ideasonboard.com>\n\t<20221216122939.256534-3-paul.elder@ideasonboard.com>\n\t<20221216140059.gmhxp6w52q6gqrxf@uno.localdomain>\n\t\n\t<1a3f27d9-16d9-8d06-a55c-aa48b1a188d6@ideasonboard.com>\n\t\n\t<0d83acdf-c062-4419-4fe5-27520e1200d2@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<0d83acdf-c062-4419-4fe5-27520e1200d2@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v9 02/18] libcamera: property: Add\n\tMinimumRequests 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","From":"Paul Elder via libcamera-devel ","Reply-To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26123,"web_url":"https://patchwork.libcamera.org/comment/26123/","msgid":"<20221221090928.dl7quz4qjck4mmap@uno.localdomain>","date":"2022-12-21T09:09:28","subject":"Re: [libcamera-devel] [PATCH v9 02/18] libcamera: property: Add\n\tMinimumRequests property","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul\n\nOn Wed, Dec 21, 2022 at 04:13:38PM +0900, Paul Elder wrote:\n> Hi Umang,\n>\n> On Tue, Dec 20, 2022 at 03:03:26PM +0530, Umang Jain wrote:\n> > Hi Paul,\n> >\n> > On 12/20/22 2:34 PM, Paul Elder wrote:\n> > > On Tue, Dec 20, 2022 at 02:04:15PM +0530, Umang Jain wrote:\n> > > > Hi Paul,\n> > > >\n> > > > On 12/20/22 1:51 PM, Paul Elder via libcamera-devel wrote:\n> > > > > Hi Jacopo,\n> > > > >\n> > > > > On Fri, Dec 16, 2022 at 03:00:59PM +0100, Jacopo Mondi wrote:\n> > > > > > Hi Paul\n> > > > > >\n> > > > > > On Fri, Dec 16, 2022 at 09:29:23PM +0900, Paul Elder via libcamera-devel 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> > > > > > > Signed-off-by: Nícolas F. R. A. Prado \n> > > > > > > Reviewed-by: Laurent Pinchart \n> > > > > > > Reviewed-by: Paul Elder \n> > > > > > > Signed-off-by: Paul Elder \n> > > > > > >\n> > > > > > Seems quite on-spot considering the thread Umang has just started\n> > > > > >\n> > > > > > To be honest I would have expected this to generate more discussion,\n> > > > > > but seeing Laurent's tag there makes me presume this has been blessed\n> > > > > > already during the previous review.\n> > > > > >\n> > > > > >\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 | 22 +++++++---\n> > > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +\n> > > > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 2 +\n> > > > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +\n> > > > > > > src/libcamera/pipeline/simple/simple.cpp | 40 +++++++++++++++++--\n> > > > > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +\n> > > > > > > src/libcamera/pipeline/vimc/vimc.cpp | 2 +\n> > > > > > > src/libcamera/property_ids.yaml | 21 ++++++++++\n> > > > > > > 8 files changed, 84 insertions(+), 9 deletions(-)\n> > > > > > >\n> > > > > > > diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst\n> > > > > > > index e1930fdf..a7356e4a 100644\n> > > > > > > --- a/Documentation/guides/pipeline-handler.rst\n> > > > > > > +++ b/Documentation/guides/pipeline-handler.rst\n> > > > > > > @@ -658,19 +658,31 @@ associated with immutable values, which represent static characteristics that ca\n> > > > > > > be used by applications to identify camera devices in the system. Properties can be\n> > > > > > > registered by inspecting the values of V4L2 controls from the video devices and\n> > > > > > > camera sensor (for example to retrieve the position and orientation of a camera)\n> > > > > > > -or to express other immutable characteristics. The example pipeline handler does\n> > > > > > > -not register any property, but examples are available in the libcamera code\n> > > > > > > -base.\n> > > > > > > +or to express other immutable characteristics.\n> > > > > > >\n> > > > > > > -.. TODO: Add a property example to the pipeline handler. At least the model.\n> > > > > > > +A required property is ``MinimumRequests``, which indicates how many requests\n> > > > > > > +need to be queued in the pipeline for capture without frame drops to be\n> > > > > > > +possible.\n> > > > > > > +\n> > > > > > > +In our case, the vivid driver requires two buffers before it'll start streaming\n> > > > > > > +(can be seen in the ``min_buffers_needed`` property for the ``vid_cap`` queue in\n> > > > > > > +vivid's driver code). In order to not drop frames we should have one extra\n> > > > > > > +buffer queued to the driver so that it is used as soon as the previous buffer\n> > > > > > > +completes. Therefore we will set our ``MinimumRequests`` to three. Append the\n> > > > > > > +following line to ``init()``:\n> > > > > > The driver requires two buffers to operate, why an extra one is needed ?\n> > > > > > For vivid in particular, where buffer's content is generated..\n> > > > > I have a feeling that the intention was a spare buffer floating around\n> > > > > in the application, but given the points that you've identified below,\n> > > > > also maybe not.\n> > > > >\n> > > > > Also I feel like this description is mixing up Requests and buffers. I\n> > > > > guess I should've vetted it a bit more (I was too distracted by merge\n> > > > > conflicts).\n> > > > >\n> > > > > > > +\n> > > > > > > +.. code-block:: cpp\n> > > > > > > +\n> > > > > > > + properties_.set(properties::MinimumRequests, 3);\n> > > > > > >\n> > > > > > > At this point you need to add the following includes to the top of the file for\n> > > > > > > -handling controls:\n> > > > > > > +handling controls and properties:\n> > > > > > >\n> > > > > > > .. code-block:: cpp\n> > > > > > >\n> > > > > > > #include \n> > > > > > > #include \n> > > > > > > + #include \n> > > > > > >\n> > > > > > > Generating a default configuration\n> > > > > > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n> > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > > index e4d79ea4..98a4a3e5 100644\n> > > > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > > @@ -1126,6 +1126,8 @@ int PipelineHandlerIPU3::registerCameras()\n> > > > > > > \t\t/* Initialize the camera properties. */\n> > > > > > > \t\tdata->properties_ = cio2->sensor()->properties();\n> > > > > > >\n> > > > > > > +\t\tdata->properties_.set(properties::MinimumRequests, 3);\n> > > > > > > +\n> > > > > > I see\n> > > > > >\n> > > > > > static constexpr unsigned int kBufferCount = 4;\n> > > > > >\n> > > > > > Which seems to suggest that at least 4 buffers need to be queued to\n> > > > > > the video device before streaming is started.\n> > > > > >\n> > > > > > I wonder if I'm confusing the two concepts\n> > > > > Gah, the ipu3 code is so convoluted.\n> > > > >\n> > > > > kBufferCount is the number of internal buffers to allocate on the CIO2.\n> > > > I see kBufferCount being set to non-raw cfg->bufferCount in\n> > > > IPU3CameraConfiguration::validate()\n> > > ...I don't see it\n> >\n> > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/ipu3/ipu3.cpp#n371\n>\n> It's removed in patch 10/18 \"libcamera: stream: Remove bufferCount\"\n>\n\nSo you removed that lines in a patch -after- this one, don't you ?\n\nI guess what we both tried to express was \"currently we require 4\nbuffers, you're changing it to 3. Does it work ? Is this intentional\n?\"\n\n>\n> Paul\n>\n> > >\n> > > > > As far as I understand, they're only used if no buffer is provided in\n> > > > > the Request.\n> > > > >\n> > > > > So yes, I think you are confusing the two concepts. MinimumRequests is\n> > > > > the minumum number of Requests that need to be queued in order to\n> > > > > sustain capture without frame drops. kBufferCount is the number of\n> > > > > internal buffers to allocate on the CIO2.\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 8569df17..4a08d01e 100644\n> > > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > > @@ -1063,6 +1063,8 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> > > > > > > \t/* Enable SOF event generation. */\n> > > > > > > \tdata->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);\n> > > > > > >\n> > > > > > > +\tdata->properties_.set(properties::MinimumRequests, 3);\n> > > > > > > +\n> > > > > > > \t/*\n> > > > > > > \t * Reset the delayed controls with the gain and exposure values set by\n> > > > > > > \t * the IPA.\n> > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > > > index f658d75e..45b6beaf 100644\n> > > > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > > > @@ -23,6 +23,7 @@\n> > > > > > > #include \n> > > > > > > #include \n> > > > > > > #include \n> > > > > > > +#include \n> > > > > > > #include \n> > > > > > > #include \n> > > > > > > #include \n> > > > > > > @@ -1102,6 +1103,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > > > > > >\n> > > > > > > \t/* Initialize the camera properties. */\n> > > > > > > \tdata->properties_ = data->sensor_->properties();\n> > > > > > > +\tdata->properties_.set(properties::MinimumRequests, 3);\n> > > > > > Same question for RkISP1, it seems we don't even populate\n> > > > > > StreamConfiguration::bufferCount there, but I'm pretty sure you need 4\n> > > > > > requests queued before capture can start..\n> > > You only need 3.\n> > >\n> > > > > I see internal buffers but only for params and stats...\n> > > > >\n> > > > > I guess I have to check this again too then.\n> > > > I think MinimumRequests and minimum no. of buffers required to start and\n> > > > sustain capture - are closely related. More so, because you can't queue\n> > > > buffers arbitrarily (from application's PoV) - capture buffers come _via_\n> > > > queuing requests.\n> > > That's true.\n> > >\n> > > > The internal buffers, I always understood as something not provided by\n> > > > applications like stats and params - completely internal to libcamera. The\n> > > > minimumRequests property/criteria should not hinder/apply to internal\n> > > > buffers right ?\n> > > Yeah, they shouldn't apply.\n> > >\n> > >\n> > > Paul\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 24ded4db..8ed983fe 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> > > > > > > @@ -180,6 +181,10 @@ class SimplePipelineHandler;\n> > > > > > >\n> > > > > > > struct SimplePipelineInfo {\n> > > > > > > \tconst char *driver;\n> > > > > > > +\t/*\n> > > > > > > +\t * Minimum number of buffers required by the driver to start streaming.\n> > > > > > > +\t */\n> > > > > > > +\tunsigned int minimumBuffers;\n> > > > > > > \t/*\n> > > > > > > \t * Each converter in the list contains the name\n> > > > > > > \t * and the number of streams it supports.\n> > > > > > > @@ -190,10 +195,10 @@ struct SimplePipelineInfo {\n> > > > > > > namespace {\n> > > > > > >\n> > > > > > > static const SimplePipelineInfo supportedDevices[] = {\n> > > > > > > -\t{ \"imx7-csi\", { { \"pxp\", 1 } } },\n> > > > > > > -\t{ \"mxc-isi\", {} },\n> > > > > > > -\t{ \"qcom-camss\", {} },\n> > > > > > > -\t{ \"sun6i-csi\", {} },\n> > > > > > > +\t{ \"imx7-csi\", 2, { { \"pxp\", 1 } } },\n> > > > > > > +\t{ \"mxc-isi\", 3, {} },\n> > > > > > > +\t{ \"qcom-camss\", 1, {} },\n> > > > > > > +\t{ \"sun6i-csi\", 3, {} },\n> > > > > > > };\n> > > > > > >\n> > > > > > > } /* namespace */\n> > > > > > > @@ -271,6 +276,8 @@ public:\n> > > > > > > \tbool useConverter_;\n> > > > > > > \tstd::queue> converterQueue_;\n> > > > > > >\n> > > > > > > +\tconst SimplePipelineInfo *deviceInfo;\n> > > > > > > +\n> > > > > > > private:\n> > > > > > > \tvoid tryPipeline(unsigned int code, const Size &size);\n> > > > > > > \tstatic std::vector routedSourcePads(MediaPad *sink);\n> > > > > > > @@ -1168,6 +1175,29 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n> > > > > > > \tinputCfg.stride = captureFormat.planes[0].bpl;\n> > > > > > > \tinputCfg.bufferCount = kNumInternalBuffers;\n> > > > > > >\n> > > > > > > +\t/* Set the MinimumRequests property. */\n> > > > > > > +\tunsigned int minimumRequests;\n> > > > > > > +\n> > > > > > > +\tif (data->useConverter_) {\n> > > > > > > +\t\t/*\n> > > > > > > +\t\t * The application will interact only with the capture node of\n> > > > > > > +\t\t * the converter. Require two buffers for a frame drop free\n> > > > > > > +\t\t * conversion, plus one extra to account for requeue delays.\n> > > > > > > +\t\t */\n> > > > > > > +\t\tminimumRequests = 3;\n> > > > > > > +\t} else {\n> > > > > > > +\t\t/*\n> > > > > > > +\t\t * The application will interact directly with the video capture\n> > > > > > > +\t\t * device. Require the minimum required by the driver, plus one\n> > > > > > > +\t\t * extra to account for requeue delays. Force at least three\n> > > > > > > +\t\t * buffers in order to not drop frames.\n> > > > > > > +\t\t */\n> > > > > > > +\t\tminimumRequests = std::max(data->deviceInfo->minimumBuffers + 1,\n> > > > > > > +\t\t\t\t\t 3U);\n> > > > > > > +\t}\n> > > > > > > +\n> > > > > > > +\tdata->properties_.set(properties::MinimumRequests, minimumRequests);\n> > > > > > > +\n> > > > > > > \treturn data->converter_->configure(inputCfg, outputCfgs);\n> > > > > > > }\n> > > > > > >\n> > > > > > > @@ -1506,6 +1536,8 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)\n> > > > > > > \tbool registered = false;\n> > > > > > >\n> > > > > > > \tfor (std::unique_ptr &data : pipelines) {\n> > > > > > > +\t\tdata->deviceInfo = info;\n> > > > > > > +\n> > > > > > > \t\tint ret = data->init();\n> > > > > > > \t\tif (ret < 0)\n> > > > > > > \t\t\tcontinue;\n> > > > > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > > > > > index 277465b7..7f580955 100644\n> > > > > > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > > > > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > > > > > @@ -500,6 +500,8 @@ int UVCCameraData::init(MediaDevice *media)\n> > > > > > > \tproperties_.set(properties::PixelArraySize, resolution);\n> > > > > > > \tproperties_.set(properties::PixelArrayActiveAreas, { Rectangle(resolution) });\n> > > > > > >\n> > > > > > > +\tproperties_.set(properties::MinimumRequests, 3);\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 204f5ad7..d2633be4 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> > > > > > > @@ -571,6 +572,7 @@ int VimcCameraData::init()\n> > > > > > >\n> > > > > > > \t/* Initialize the camera properties. */\n> > > > > > > \tproperties_ = sensor_->properties();\n> > > > > > > +\tproperties_.set(properties::MinimumRequests, 3);\n> > > > > > >\n> > > > > > > \treturn 0;\n> > > > > > > }\n> > > > > > > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> > > > > > > index cb55e0ed..c82ac17e 100644\n> > > > > > > --- a/src/libcamera/property_ids.yaml\n> > > > > > > +++ b/src/libcamera/property_ids.yaml\n> > > > > > > @@ -690,6 +690,27 @@ controls:\n> > > > > > > that is twice that of the full resolution mode. This value will be valid\n> > > > > > > after the configure method has returned successfully.\n> > > > > > >\n> > > > > > > + - MinimumRequests:\n> > > > > > > + type: int32_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 without frame\n> > > > > > > + drops. At this quantity, there's no guarantee that manual per-frame\n> > > > > > > + controls will apply correctly.\n> > > > > > This property is also relevant for the startup phase, so mentioning\n> > > > > > that this is the minium number of requests that need to be queued\n> > > > > > before capture starts might be necessary ?\n> > > > > Indeed.\n> > > > >\n> > > > >\n> > > > > Thanks,\n> > > > >\n> > > > > Paul\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> > > > > > > # Draft properties section\n> > > > > > >\n> > > > > > > --\n> > > > > > > 2.35.1\n> > > > > > >\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 41259C3200\n\tfor ;\n\tWed, 21 Dec 2022 09:09:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A39F7633A7;\n\tWed, 21 Dec 2022 10:09:32 +0100 (CET)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7981661F1A\n\tfor ;\n\tWed, 21 Dec 2022 10:09:30 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id A8E4F20012;\n\tWed, 21 Dec 2022 09:09:29 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1671613772;\n\tbh=tb3zhhwPWAbchMC++yPRMa+2xiKuDwtLymue3/gLNgk=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=F25+pN6zFxRT2NpxU3RA0QUJycl51nc+R6QTMnEdPEg7Dp2kkcD/P0CxD4e11IPiT\n\tQDZ4oM519PWWuKCeSJonnV9V86ssOPNhNH1eF2Kb9GuzCfxl2h8I/1kDOxN4RWGlFD\n\tJo1nGUQlhuk3tgkomfLrIfzeJI3iUal3F3wSpLUsbasvQPTWl58PS1Hmi3/MNQV8Q5\n\ttYzu/QxZVfZlSJw60YHpot85+Dbvz8fJOhpGAc0qVHMA5/IsrjCbxYhurhWvLgY2QZ\n\tbyF7OhGBeXVUXu/kcrZjCW0SOPG0wUP35B5vIKEIfyod+3C9zVq77FplkKCrobzqIY\n\tMyo3GArM//dZg==","Date":"Wed, 21 Dec 2022 10:09:28 +0100","To":"Paul Elder ","Message-ID":"<20221221090928.dl7quz4qjck4mmap@uno.localdomain>","References":"<20221216122939.256534-1-paul.elder@ideasonboard.com>\n\t<20221216122939.256534-3-paul.elder@ideasonboard.com>\n\t<20221216140059.gmhxp6w52q6gqrxf@uno.localdomain>\n\t\n\t<1a3f27d9-16d9-8d06-a55c-aa48b1a188d6@ideasonboard.com>\n\t\n\t<0d83acdf-c062-4419-4fe5-27520e1200d2@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v9 02/18] libcamera: property: Add\n\tMinimumRequests 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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26124,"web_url":"https://patchwork.libcamera.org/comment/26124/","msgid":"<95670efe-0f3a-88eb-46f3-cc1bf1cd0000@ideasonboard.com>","date":"2022-12-21T09:45:43","subject":"Re: [libcamera-devel] [PATCH v9 02/18] libcamera: property: Add\n\tMinimumRequests property","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Paul, Nicolas\n\nThank you for the patch.\n\nOn 12/16/22 5:59 PM, Paul Elder via libcamera-devel 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> Signed-off-by: Nícolas F. R. A. Prado \n> Reviewed-by: Laurent Pinchart \n> Reviewed-by: Paul Elder \n> Signed-off-by: Paul Elder \n>\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 | 22 +++++++---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +\n> .../pipeline/raspberrypi/raspberrypi.cpp | 2 +\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +\n> src/libcamera/pipeline/simple/simple.cpp | 40 +++++++++++++++++--\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +\n> src/libcamera/pipeline/vimc/vimc.cpp | 2 +\n> src/libcamera/property_ids.yaml | 21 ++++++++++\n> 8 files changed, 84 insertions(+), 9 deletions(-)\n>\n> diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst\n> index e1930fdf..a7356e4a 100644\n> --- a/Documentation/guides/pipeline-handler.rst\n> +++ b/Documentation/guides/pipeline-handler.rst\n> @@ -658,19 +658,31 @@ associated with immutable values, which represent static characteristics that ca\n> be used by applications to identify camera devices in the system. Properties can be\n> registered by inspecting the values of V4L2 controls from the video devices and\n> camera sensor (for example to retrieve the position and orientation of a camera)\n> -or to express other immutable characteristics. The example pipeline handler does\n> -not register any property, but examples are available in the libcamera code\n> -base.\n> +or to express other immutable characteristics.\n> \n> -.. TODO: Add a property example to the pipeline handler. At least the model.\n> +A required property is ``MinimumRequests``, which indicates how many requests\n> +need to be queued in the pipeline for capture without frame drops to be\n> +possible.\n> +\n> +In our case, the vivid driver requires two buffers before it'll start streaming\n> +(can be seen in the ``min_buffers_needed`` property for the ``vid_cap`` queue in\n> +vivid's driver code). In order to not drop frames we should have one extra\n> +buffer queued to the driver so that it is used as soon as the previous buffer\n> +completes. Therefore we will set our ``MinimumRequests`` to three. Append the\n> +following line to ``init()``:\n> +\n> +.. code-block:: cpp\n> +\n> + properties_.set(properties::MinimumRequests, 3);\n> \n> At this point you need to add the following includes to the top of the file for\n> -handling controls:\n> +handling controls and properties:\n> \n> .. code-block:: cpp\n> \n> #include \n> #include \n> + #include \n> \n> Generating a default configuration\n> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index e4d79ea4..98a4a3e5 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -1126,6 +1126,8 @@ int PipelineHandlerIPU3::registerCameras()\n> \t\t/* Initialize the camera properties. */\n> \t\tdata->properties_ = cio2->sensor()->properties();\n> \n> +\t\tdata->properties_.set(properties::MinimumRequests, 3);\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 8569df17..4a08d01e 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1063,6 +1063,8 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> \t/* Enable SOF event generation. */\n> \tdata->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);\n> \n> +\tdata->properties_.set(properties::MinimumRequests, 3);\n> +\n\non my local kernel branch unicam reports min_buffers_needed  = 1 but \nbcm2835-isp doesn't report any min_buffers_needed (dumb guess it'll be 1).\n\nProbably worth checking in with RPi devs regarding the \nmin_buffers_needed for their ISP (adding a note in my todo list)\n> \t/*\n> \t * Reset the delayed controls with the gain and exposure values set by\n> \t * the IPA.\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index f658d75e..45b6beaf 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -23,6 +23,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -1102,6 +1103,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> \n> \t/* Initialize the camera properties. */\n> \tdata->properties_ = data->sensor_->properties();\n> +\tdata->properties_.set(properties::MinimumRequests, 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 24ded4db..8ed983fe 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> @@ -180,6 +181,10 @@ class SimplePipelineHandler;\n> \n> struct SimplePipelineInfo {\n> \tconst char *driver;\n> +\t/*\n> +\t * Minimum number of buffers required by the driver to start streaming.\n> +\t */\n> +\tunsigned int minimumBuffers;\n> \t/*\n> \t * Each converter in the list contains the name\n> \t * and the number of streams it supports.\n> @@ -190,10 +195,10 @@ struct SimplePipelineInfo {\n> namespace {\n> \n> static const SimplePipelineInfo supportedDevices[] = {\n> -\t{ \"imx7-csi\", { { \"pxp\", 1 } } },\n> -\t{ \"mxc-isi\", {} },\n> -\t{ \"qcom-camss\", {} },\n> -\t{ \"sun6i-csi\", {} },\n> +\t{ \"imx7-csi\", 2, { { \"pxp\", 1 } } },\n> +\t{ \"mxc-isi\", 3, {} },\n> +\t{ \"qcom-camss\", 1, {} },\n> +\t{ \"sun6i-csi\", 3, {} },\n> };\n> \n> } /* namespace */\n> @@ -271,6 +276,8 @@ public:\n> \tbool useConverter_;\n> \tstd::queue> converterQueue_;\n> \n> +\tconst SimplePipelineInfo *deviceInfo;\n> +\n> private:\n> \tvoid tryPipeline(unsigned int code, const Size &size);\n> \tstatic std::vector routedSourcePads(MediaPad *sink);\n> @@ -1168,6 +1175,29 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n> \tinputCfg.stride = captureFormat.planes[0].bpl;\n> \tinputCfg.bufferCount = kNumInternalBuffers;\n> \n> +\t/* Set the MinimumRequests property. */\n> +\tunsigned int minimumRequests;\n> +\n> +\tif (data->useConverter_) {\n> +\t\t/*\n> +\t\t * The application will interact only with the capture node of\n> +\t\t * the converter. Require two buffers for a frame drop free\n> +\t\t * conversion, plus one extra to account for requeue delays.\n> +\t\t */\n> +\t\tminimumRequests = 3;\n> +\t} else {\n> +\t\t/*\n> +\t\t * The application will interact directly with the video capture\n> +\t\t * device. Require the minimum required by the driver, plus one\n> +\t\t * extra to account for requeue delays. Force at least three\n> +\t\t * buffers in order to not drop frames.\n> +\t\t */\n> +\t\tminimumRequests = std::max(data->deviceInfo->minimumBuffers + 1,\n> +\t\t\t\t\t 3U);\n> +\t}\n> +\n> +\tdata->properties_.set(properties::MinimumRequests, minimumRequests);\n> +\n> \treturn data->converter_->configure(inputCfg, outputCfgs);\n> }\n> \n> @@ -1506,6 +1536,8 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)\n> \tbool registered = false;\n> \n> \tfor (std::unique_ptr &data : pipelines) {\n> +\t\tdata->deviceInfo = info;\n> +\n> \t\tint ret = data->init();\n> \t\tif (ret < 0)\n> \t\t\tcontinue;\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 277465b7..7f580955 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -500,6 +500,8 @@ int UVCCameraData::init(MediaDevice *media)\n> \tproperties_.set(properties::PixelArraySize, resolution);\n> \tproperties_.set(properties::PixelArrayActiveAreas, { Rectangle(resolution) });\n> \n> +\tproperties_.set(properties::MinimumRequests, 3);\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 204f5ad7..d2633be4 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> @@ -571,6 +572,7 @@ int VimcCameraData::init()\n> \n> \t/* Initialize the camera properties. */\n> \tproperties_ = sensor_->properties();\n> +\tproperties_.set(properties::MinimumRequests, 3);\n> \n> \treturn 0;\n> }\n> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> index cb55e0ed..c82ac17e 100644\n> --- a/src/libcamera/property_ids.yaml\n> +++ b/src/libcamera/property_ids.yaml\n> @@ -690,6 +690,27 @@ controls:\n> that is twice that of the full resolution mode. This value will be valid\n> after the configure method has returned successfully.\n> \n> + - MinimumRequests:\n> + type: int32_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 without frame\n> + drops. At this quantity, there's no guarantee that manual per-frame\n> + controls will apply correctly.\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\nSo, I like the direction here.\n\nBut given the questions mentioned in the \\todo(s) and provided how we \nactually split  Requests <> Buffers association going ahead - I think \nthis will get impacted (probably MinimumRequests will become \nMinimumBuffers and what not...)\n\nHowever, this is still under discussion, so I don't want to start yet \nanother parallel discussion here except that - in my opinion this \nproperty should be under \"Draft properties section\" for now. This will \ntake a considerable set of iterations to get it right (provided how we \ndefine and declare other properties on this front to sustain capture, \nframedrops etc. etc.).\n\nAcked-by: Umang Jain \n> # ----------------------------------------------------------------------------\n> # Draft properties section\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 B0480C3213\n\tfor ;\n\tWed, 21 Dec 2022 09:45:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E43C6633A7;\n\tWed, 21 Dec 2022 10:45:52 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4042961F1A\n\tfor ;\n\tWed, 21 Dec 2022 10:45:51 +0100 (CET)","from [IPV6:2401:4900:1f3f:d076:4da6:b729:f032:ed0a] (unknown\n\t[IPv6:2401:4900:1f3f:d076:4da6:b729:f032:ed0a])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C0620FB;\n\tWed, 21 Dec 2022 10:45:49 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1671615952;\n\tbh=ytfEVRMlfED3ymNWEqT8evR6Zg8mI9RtQD8wcxR2O5k=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=2dH3DiCJa4PfivzO7ny9bavmA6J9BSZFlBtFh9BQWOZb0T8SKBf9JJnODWBHlXWL6\n\t4De4YKbXbpKZlgjVcN3dTiBdnUGvyWUbZNdug+xK0rH+bZMH4uwbX4+OsJZPX6S8gX\n\tr9mBiWso0C4/Ag3gTNzaRflKr+DlyUFhloHvW+nfKDtH3JNANf/aRSh6uUlim3j285\n\t6dIrvbyzha7qzbGTP6LJ+5AlVs46C6dW2eGg45ISRbjo3WNADgFamLxdTkQcAhvX/Z\n\t7sTVQHjgBy8fae0reBd6fiT/N8O7UfynFF/wZFcGLqd4sVYAzVffYpfiV0XH63ZQAa\n\ta5LX0rt6jfh5g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1671615950;\n\tbh=ytfEVRMlfED3ymNWEqT8evR6Zg8mI9RtQD8wcxR2O5k=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=jCGm3w/bn5s/r1oCzdGh0xC4zgO3/ehJ9eSvFAIrdKKze1T8pGXMnNZgc+HMedSEt\n\tnqrSh0wKNgZr6182VwyIfsaKkZ6WePT7pgBHL0S4jQrWBc0+iyj0n5tz/p9X1LtIy8\n\tlzzVExvOL1MbIpdCA/gtJ7qrqO6YjICLe4k9tMSk="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"jCGm3w/b\"; dkim-atps=neutral","Message-ID":"<95670efe-0f3a-88eb-46f3-cc1bf1cd0000@ideasonboard.com>","Date":"Wed, 21 Dec 2022 15:15:43 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.5.1","Content-Language":"en-US","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20221216122939.256534-1-paul.elder@ideasonboard.com>\n\t<20221216122939.256534-3-paul.elder@ideasonboard.com>","In-Reply-To":"<20221216122939.256534-3-paul.elder@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v9 02/18] libcamera: property: Add\n\tMinimumRequests 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","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26167,"web_url":"https://patchwork.libcamera.org/comment/26167/","msgid":"","date":"2022-12-28T04:09:44","subject":"Re: [libcamera-devel] [PATCH v9 02/18] libcamera: property: Add\n\tMinimumRequests property","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Wed, Dec 21, 2022 at 03:15:43PM +0530, Umang Jain wrote:\n> Hi Paul, Nicolas\n> \n> Thank you for the patch.\n> \n> On 12/16/22 5:59 PM, Paul Elder via libcamera-devel 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> > Signed-off-by: Nícolas F. R. A. Prado \n> > Reviewed-by: Laurent Pinchart \n> > Reviewed-by: Paul Elder \n> > Signed-off-by: Paul Elder \n> > \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 | 22 +++++++---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 2 +\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +\n> > src/libcamera/pipeline/simple/simple.cpp | 40 +++++++++++++++++--\n> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +\n> > src/libcamera/pipeline/vimc/vimc.cpp | 2 +\n> > src/libcamera/property_ids.yaml | 21 ++++++++++\n> > 8 files changed, 84 insertions(+), 9 deletions(-)\n> > \n> > diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst\n> > index e1930fdf..a7356e4a 100644\n> > --- a/Documentation/guides/pipeline-handler.rst\n> > +++ b/Documentation/guides/pipeline-handler.rst\n> > @@ -658,19 +658,31 @@ associated with immutable values, which represent static characteristics that ca\n> > be used by applications to identify camera devices in the system. Properties can be\n> > registered by inspecting the values of V4L2 controls from the video devices and\n> > camera sensor (for example to retrieve the position and orientation of a camera)\n> > -or to express other immutable characteristics. The example pipeline handler does\n> > -not register any property, but examples are available in the libcamera code\n> > -base.\n> > +or to express other immutable characteristics.\n> > -.. TODO: Add a property example to the pipeline handler. At least the model.\n> > +A required property is ``MinimumRequests``, which indicates how many requests\n> > +need to be queued in the pipeline for capture without frame drops to be\n> > +possible.\n> > +\n> > +In our case, the vivid driver requires two buffers before it'll start streaming\n> > +(can be seen in the ``min_buffers_needed`` property for the ``vid_cap`` queue in\n> > +vivid's driver code). In order to not drop frames we should have one extra\n> > +buffer queued to the driver so that it is used as soon as the previous buffer\n> > +completes. Therefore we will set our ``MinimumRequests`` to three. Append the\n> > +following line to ``init()``:\n> > +\n> > +.. code-block:: cpp\n> > +\n> > + properties_.set(properties::MinimumRequests, 3);\n> > At this point you need to add the following includes to the top of the file for\n> > -handling controls:\n> > +handling controls and properties:\n> > .. code-block:: cpp\n> > #include \n> > #include \n> > + #include \n> > Generating a default configuration\n> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index e4d79ea4..98a4a3e5 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -1126,6 +1126,8 @@ int PipelineHandlerIPU3::registerCameras()\n> > \t\t/* Initialize the camera properties. */\n> > \t\tdata->properties_ = cio2->sensor()->properties();\n> > +\t\tdata->properties_.set(properties::MinimumRequests, 3);\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 8569df17..4a08d01e 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -1063,6 +1063,8 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n> > \t/* Enable SOF event generation. */\n> > \tdata->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);\n> > +\tdata->properties_.set(properties::MinimumRequests, 3);\n> > +\n> \n> on my local kernel branch unicam reports min_buffers_needed  = 1 but\n> bcm2835-isp doesn't report any min_buffers_needed (dumb guess it'll be 1).\n> \n> Probably worth checking in with RPi devs regarding the min_buffers_needed\n> for their ISP (adding a note in my todo list)\n> > \t/*\n> > \t * Reset the delayed controls with the gain and exposure values set by\n> > \t * the IPA.\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index f658d75e..45b6beaf 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -23,6 +23,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -1102,6 +1103,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> > \t/* Initialize the camera properties. */\n> > \tdata->properties_ = data->sensor_->properties();\n> > +\tdata->properties_.set(properties::MinimumRequests, 3);\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 24ded4db..8ed983fe 100644\n> > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > @@ -25,6 +25,7 @@\n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > @@ -180,6 +181,10 @@ class SimplePipelineHandler;\n> > struct SimplePipelineInfo {\n> > \tconst char *driver;\n> > +\t/*\n> > +\t * Minimum number of buffers required by the driver to start streaming.\n> > +\t */\n> > +\tunsigned int minimumBuffers;\n> > \t/*\n> > \t * Each converter in the list contains the name\n> > \t * and the number of streams it supports.\n> > @@ -190,10 +195,10 @@ struct SimplePipelineInfo {\n> > namespace {\n> > static const SimplePipelineInfo supportedDevices[] = {\n> > -\t{ \"imx7-csi\", { { \"pxp\", 1 } } },\n> > -\t{ \"mxc-isi\", {} },\n> > -\t{ \"qcom-camss\", {} },\n> > -\t{ \"sun6i-csi\", {} },\n> > +\t{ \"imx7-csi\", 2, { { \"pxp\", 1 } } },\n> > +\t{ \"mxc-isi\", 3, {} },\n> > +\t{ \"qcom-camss\", 1, {} },\n> > +\t{ \"sun6i-csi\", 3, {} },\n> > };\n> > } /* namespace */\n> > @@ -271,6 +276,8 @@ public:\n> > \tbool useConverter_;\n> > \tstd::queue> converterQueue_;\n> > +\tconst SimplePipelineInfo *deviceInfo;\n> > +\n> > private:\n> > \tvoid tryPipeline(unsigned int code, const Size &size);\n> > \tstatic std::vector routedSourcePads(MediaPad *sink);\n> > @@ -1168,6 +1175,29 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n> > \tinputCfg.stride = captureFormat.planes[0].bpl;\n> > \tinputCfg.bufferCount = kNumInternalBuffers;\n> > +\t/* Set the MinimumRequests property. */\n> > +\tunsigned int minimumRequests;\n> > +\n> > +\tif (data->useConverter_) {\n> > +\t\t/*\n> > +\t\t * The application will interact only with the capture node of\n> > +\t\t * the converter. Require two buffers for a frame drop free\n> > +\t\t * conversion, plus one extra to account for requeue delays.\n> > +\t\t */\n> > +\t\tminimumRequests = 3;\n> > +\t} else {\n> > +\t\t/*\n> > +\t\t * The application will interact directly with the video capture\n> > +\t\t * device. Require the minimum required by the driver, plus one\n> > +\t\t * extra to account for requeue delays. Force at least three\n> > +\t\t * buffers in order to not drop frames.\n> > +\t\t */\n> > +\t\tminimumRequests = std::max(data->deviceInfo->minimumBuffers + 1,\n> > +\t\t\t\t\t 3U);\n> > +\t}\n> > +\n> > +\tdata->properties_.set(properties::MinimumRequests, minimumRequests);\n> > +\n> > \treturn data->converter_->configure(inputCfg, outputCfgs);\n> > }\n> > @@ -1506,6 +1536,8 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)\n> > \tbool registered = false;\n> > \tfor (std::unique_ptr &data : pipelines) {\n> > +\t\tdata->deviceInfo = info;\n> > +\n> > \t\tint ret = data->init();\n> > \t\tif (ret < 0)\n> > \t\t\tcontinue;\n> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > index 277465b7..7f580955 100644\n> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > @@ -500,6 +500,8 @@ int UVCCameraData::init(MediaDevice *media)\n> > \tproperties_.set(properties::PixelArraySize, resolution);\n> > \tproperties_.set(properties::PixelArrayActiveAreas, { Rectangle(resolution) });\n> > +\tproperties_.set(properties::MinimumRequests, 3);\n> > +\n> > \t/* Initialise the supported controls. */\n> > \tControlInfoMap::Map ctrls;\n> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> > index 204f5ad7..d2633be4 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> > @@ -571,6 +572,7 @@ int VimcCameraData::init()\n> > \t/* Initialize the camera properties. */\n> > \tproperties_ = sensor_->properties();\n> > +\tproperties_.set(properties::MinimumRequests, 3);\n> > \treturn 0;\n> > }\n> > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> > index cb55e0ed..c82ac17e 100644\n> > --- a/src/libcamera/property_ids.yaml\n> > +++ b/src/libcamera/property_ids.yaml\n> > @@ -690,6 +690,27 @@ controls:\n> > that is twice that of the full resolution mode. This value will be valid\n> > after the configure method has returned successfully.\n> > + - MinimumRequests:\n> > + type: int32_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 without frame\n> > + drops. At this quantity, there's no guarantee that manual per-frame\n> > + controls will apply correctly.\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> So, I like the direction here.\n> \n> But given the questions mentioned in the \\todo(s) and provided how we\n> actually split  Requests <> Buffers association going ahead - I think this\n> will get impacted (probably MinimumRequests will become MinimumBuffers and\n> what not...)\n\nYeah it definitely will. I think we can deal with that when we come to\nit, though.\n\n> \n> However, this is still under discussion, so I don't want to start yet\n> another parallel discussion here except that - in my opinion this property\n> should be under \"Draft properties section\" for now. This will take a\n\nWell we want this to be a required property, so I don't think it can go\nin the draft properties.\n\n> considerable set of iterations to get it right (provided how we define and\n> declare other properties on this front to sustain capture, framedrops etc.\n> etc.).\n> \n> Acked-by: Umang Jain \n\n\nThanks,\n\nPaul\n\n> > # ----------------------------------------------------------------------------\n> > # Draft properties section\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 9AB6EBD78E\n\tfor ;\n\tWed, 28 Dec 2022 04:09:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7B3BA625CF;\n\tWed, 28 Dec 2022 05:09:54 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 66DA8603D6\n\tfor ;\n\tWed, 28 Dec 2022 05:09:52 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2604:2d80:ad8a:9000:1bf9:855b:22de:3645])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 308BE25B;\n\tWed, 28 Dec 2022 05:09:50 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1672200594;\n\tbh=Ns4jLGMRnD11kn0MJvhpkoqem7Hfhedy4mgL+xqPjRw=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=vPMBfdZVIkUECUf7zrGWh2/zSc4p/n3H/ZdDp19Pad9RE2CHzYich83wLqaqWYdgt\n\tjHD46v8ez02o9UVjql3psscY3hKYj2iHPXU2Fh5phA1J0SYs/0azJ476AAEhy8dA5a\n\t9FRN9i4vtnNEpD1pTBR6MzHE/vQFQEM3SqKOd865OEWiSRtHIhrHqYq5JFzSGjhQys\n\tzbpBQvsA1ogHjIQSkn7m/VD59bNEZIr+5Ec1FQwVtmET7vBbV3k9sNOT/DQbNZR6/i\n\tdEoHcaMkKJIJ6uaNRWEIZPKqo1TfrAi4HTGcB5Sn/drZSqmQLV7wjYhSWRfHUIsvth\n\tm0btOdGLhf6Qw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1672200592;\n\tbh=Ns4jLGMRnD11kn0MJvhpkoqem7Hfhedy4mgL+xqPjRw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DO9nVs4GZ9MIHj4zxEI5vrm0YX83yLeIil2SRelYJezilLruX86x61nCAnR6iiI6s\n\tIJmlf+B/2SKIG6gkcuMYauOYvG7OFsf+uvQ7q/Wab9kvOZBEf5ksIWMHvAvs5M0cSb\n\tWfV2i4vkW0OOTrDz/Xj0cn4tJXC5fOx7v8+P26tU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"DO9nVs4G\"; dkim-atps=neutral","Date":"Tue, 27 Dec 2022 22:09:44 -0600","To":"Umang Jain ","Message-ID":"","References":"<20221216122939.256534-1-paul.elder@ideasonboard.com>\n\t<20221216122939.256534-3-paul.elder@ideasonboard.com>\n\t<95670efe-0f3a-88eb-46f3-cc1bf1cd0000@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<95670efe-0f3a-88eb-46f3-cc1bf1cd0000@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v9 02/18] libcamera: property: Add\n\tMinimumRequests 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","From":"Paul Elder via libcamera-devel ","Reply-To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26168,"web_url":"https://patchwork.libcamera.org/comment/26168/","msgid":"","date":"2022-12-28T05:04:46","subject":"Re: [libcamera-devel] [PATCH v9 02/18] libcamera: property: Add\n\tMinimumRequests property","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Paul,\n\nOn 12/28/22 9:39 AM, Paul Elder wrote:\n> On Wed, Dec 21, 2022 at 03:15:43PM +0530, Umang Jain wrote:\n>> Hi Paul, Nicolas\n>>\n>> Thank you for the patch.\n>>\n>> On 12/16/22 5:59 PM, Paul Elder via libcamera-devel 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>>> Signed-off-by: Nícolas F. R. A. Prado \n>>> Reviewed-by: Laurent Pinchart \n>>> Reviewed-by: Paul Elder \n>>> Signed-off-by: Paul Elder \n>>>\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 | 22 +++++++---\n>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +\n>>> .../pipeline/raspberrypi/raspberrypi.cpp | 2 +\n>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +\n>>> src/libcamera/pipeline/simple/simple.cpp | 40 +++++++++++++++++--\n>>> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +\n>>> src/libcamera/pipeline/vimc/vimc.cpp | 2 +\n>>> src/libcamera/property_ids.yaml | 21 ++++++++++\n>>> 8 files changed, 84 insertions(+), 9 deletions(-)\n>>>\n>>> diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst\n>>> index e1930fdf..a7356e4a 100644\n>>> --- a/Documentation/guides/pipeline-handler.rst\n>>> +++ b/Documentation/guides/pipeline-handler.rst\n>>> @@ -658,19 +658,31 @@ associated with immutable values, which represent static characteristics that ca\n>>> be used by applications to identify camera devices in the system. Properties can be\n>>> registered by inspecting the values of V4L2 controls from the video devices and\n>>> camera sensor (for example to retrieve the position and orientation of a camera)\n>>> -or to express other immutable characteristics. The example pipeline handler does\n>>> -not register any property, but examples are available in the libcamera code\n>>> -base.\n>>> +or to express other immutable characteristics.\n>>> -.. TODO: Add a property example to the pipeline handler. At least the model.\n>>> +A required property is ``MinimumRequests``, which indicates how many requests\n>>> +need to be queued in the pipeline for capture without frame drops to be\n>>> +possible.\n>>> +\n>>> +In our case, the vivid driver requires two buffers before it'll start streaming\n>>> +(can be seen in the ``min_buffers_needed`` property for the ``vid_cap`` queue in\n>>> +vivid's driver code). In order to not drop frames we should have one extra\n>>> +buffer queued to the driver so that it is used as soon as the previous buffer\n>>> +completes. Therefore we will set our ``MinimumRequests`` to three. Append the\n>>> +following line to ``init()``:\n>>> +\n>>> +.. code-block:: cpp\n>>> +\n>>> + properties_.set(properties::MinimumRequests, 3);\n>>> At this point you need to add the following includes to the top of the file for\n>>> -handling controls:\n>>> +handling controls and properties:\n>>> .. code-block:: cpp\n>>> #include \n>>> #include \n>>> + #include \n>>> Generating a default configuration\n>>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> index e4d79ea4..98a4a3e5 100644\n>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> @@ -1126,6 +1126,8 @@ int PipelineHandlerIPU3::registerCameras()\n>>> \t\t/* Initialize the camera properties. */\n>>> \t\tdata->properties_ = cio2->sensor()->properties();\n>>> +\t\tdata->properties_.set(properties::MinimumRequests, 3);\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 8569df17..4a08d01e 100644\n>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>> @@ -1063,6 +1063,8 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)\n>>> \t/* Enable SOF event generation. */\n>>> \tdata->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);\n>>> +\tdata->properties_.set(properties::MinimumRequests, 3);\n>>> +\n>> on my local kernel branch unicam reports min_buffers_needed  = 1 but\n>> bcm2835-isp doesn't report any min_buffers_needed (dumb guess it'll be 1).\n>>\n>> Probably worth checking in with RPi devs regarding the min_buffers_needed\n>> for their ISP (adding a note in my todo list)\n>>> \t/*\n>>> \t * Reset the delayed controls with the gain and exposure values set by\n>>> \t * the IPA.\n>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>> index f658d75e..45b6beaf 100644\n>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>> @@ -23,6 +23,7 @@\n>>> #include \n>>> #include \n>>> #include \n>>> +#include \n>>> #include \n>>> #include \n>>> #include \n>>> @@ -1102,6 +1103,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n>>> \t/* Initialize the camera properties. */\n>>> \tdata->properties_ = data->sensor_->properties();\n>>> +\tdata->properties_.set(properties::MinimumRequests, 3);\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 24ded4db..8ed983fe 100644\n>>> --- a/src/libcamera/pipeline/simple/simple.cpp\n>>> +++ b/src/libcamera/pipeline/simple/simple.cpp\n>>> @@ -25,6 +25,7 @@\n>>> #include \n>>> #include \n>>> +#include \n>>> #include \n>>> #include \n>>> @@ -180,6 +181,10 @@ class SimplePipelineHandler;\n>>> struct SimplePipelineInfo {\n>>> \tconst char *driver;\n>>> +\t/*\n>>> +\t * Minimum number of buffers required by the driver to start streaming.\n>>> +\t */\n>>> +\tunsigned int minimumBuffers;\n>>> \t/*\n>>> \t * Each converter in the list contains the name\n>>> \t * and the number of streams it supports.\n>>> @@ -190,10 +195,10 @@ struct SimplePipelineInfo {\n>>> namespace {\n>>> static const SimplePipelineInfo supportedDevices[] = {\n>>> -\t{ \"imx7-csi\", { { \"pxp\", 1 } } },\n>>> -\t{ \"mxc-isi\", {} },\n>>> -\t{ \"qcom-camss\", {} },\n>>> -\t{ \"sun6i-csi\", {} },\n>>> +\t{ \"imx7-csi\", 2, { { \"pxp\", 1 } } },\n>>> +\t{ \"mxc-isi\", 3, {} },\n>>> +\t{ \"qcom-camss\", 1, {} },\n>>> +\t{ \"sun6i-csi\", 3, {} },\n>>> };\n>>> } /* namespace */\n>>> @@ -271,6 +276,8 @@ public:\n>>> \tbool useConverter_;\n>>> \tstd::queue> converterQueue_;\n>>> +\tconst SimplePipelineInfo *deviceInfo;\n>>> +\n>>> private:\n>>> \tvoid tryPipeline(unsigned int code, const Size &size);\n>>> \tstatic std::vector routedSourcePads(MediaPad *sink);\n>>> @@ -1168,6 +1175,29 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n>>> \tinputCfg.stride = captureFormat.planes[0].bpl;\n>>> \tinputCfg.bufferCount = kNumInternalBuffers;\n>>> +\t/* Set the MinimumRequests property. */\n>>> +\tunsigned int minimumRequests;\n>>> +\n>>> +\tif (data->useConverter_) {\n>>> +\t\t/*\n>>> +\t\t * The application will interact only with the capture node of\n>>> +\t\t * the converter. Require two buffers for a frame drop free\n>>> +\t\t * conversion, plus one extra to account for requeue delays.\n>>> +\t\t */\n>>> +\t\tminimumRequests = 3;\n>>> +\t} else {\n>>> +\t\t/*\n>>> +\t\t * The application will interact directly with the video capture\n>>> +\t\t * device. Require the minimum required by the driver, plus one\n>>> +\t\t * extra to account for requeue delays. Force at least three\n>>> +\t\t * buffers in order to not drop frames.\n>>> +\t\t */\n>>> +\t\tminimumRequests = std::max(data->deviceInfo->minimumBuffers + 1,\n>>> +\t\t\t\t\t 3U);\n>>> +\t}\n>>> +\n>>> +\tdata->properties_.set(properties::MinimumRequests, minimumRequests);\n>>> +\n>>> \treturn data->converter_->configure(inputCfg, outputCfgs);\n>>> }\n>>> @@ -1506,6 +1536,8 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)\n>>> \tbool registered = false;\n>>> \tfor (std::unique_ptr &data : pipelines) {\n>>> +\t\tdata->deviceInfo = info;\n>>> +\n>>> \t\tint ret = data->init();\n>>> \t\tif (ret < 0)\n>>> \t\t\tcontinue;\n>>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>>> index 277465b7..7f580955 100644\n>>> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>>> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>>> @@ -500,6 +500,8 @@ int UVCCameraData::init(MediaDevice *media)\n>>> \tproperties_.set(properties::PixelArraySize, resolution);\n>>> \tproperties_.set(properties::PixelArrayActiveAreas, { Rectangle(resolution) });\n>>> +\tproperties_.set(properties::MinimumRequests, 3);\n>>> +\n>>> \t/* Initialise the supported controls. */\n>>> \tControlInfoMap::Map ctrls;\n>>> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n>>> index 204f5ad7..d2633be4 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>>> @@ -571,6 +572,7 @@ int VimcCameraData::init()\n>>> \t/* Initialize the camera properties. */\n>>> \tproperties_ = sensor_->properties();\n>>> +\tproperties_.set(properties::MinimumRequests, 3);\n>>> \treturn 0;\n>>> }\n>>> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n>>> index cb55e0ed..c82ac17e 100644\n>>> --- a/src/libcamera/property_ids.yaml\n>>> +++ b/src/libcamera/property_ids.yaml\n>>> @@ -690,6 +690,27 @@ controls:\n>>> that is twice that of the full resolution mode. This value will be valid\n>>> after the configure method has returned successfully.\n>>> + - MinimumRequests:\n>>> + type: int32_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 without frame\n>>> + drops. At this quantity, there's no guarantee that manual per-frame\n>>> + controls will apply correctly.\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>> So, I like the direction here.\n>>\n>> But given the questions mentioned in the \\todo(s) and provided how we\n>> actually split  Requests <> Buffers association going ahead - I think this\n>> will get impacted (probably MinimumRequests will become MinimumBuffers and\n>> what not...)\n> Yeah it definitely will. I think we can deal with that when we come to\n> it, though.\n>\n>> However, this is still under discussion, so I don't want to start yet\n>> another parallel discussion here except that - in my opinion this property\n>> should be under \"Draft properties section\" for now. This will take a\n> Well we want this to be a required property, so I don't think it can go\n> in the draft properties.\n\nAh okay, I didn't know that such a constraint exists for required \nproperties.\n>\n>> considerable set of iterations to get it right (provided how we define and\n>> declare other properties on this front to sustain capture, framedrops etc.\n>> etc.).\n>>\n>> Acked-by: Umang Jain \n>\n> Thanks,\n>\n> Paul\n>\n>>> # ----------------------------------------------------------------------------\n>>> # Draft properties section","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 308B1C3200\n\tfor ;\n\tWed, 28 Dec 2022 05:04:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A5496625CF;\n\tWed, 28 Dec 2022 06:04:58 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A06B2603D6\n\tfor ;\n\tWed, 28 Dec 2022 06:04:57 +0100 (CET)","from [IPV6:2409:4042:4e0a:a444:7b5e:8ca3:daef:b773] (unknown\n\t[IPv6:2409:4042:4e0a:a444:7b5e:8ca3:daef:b773])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4961D25B;\n\tWed, 28 Dec 2022 06:04:54 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1672203898;\n\tbh=ZxGD5lABg4cFLEdlecFKi8qJq5iXtNYrnWXq21pomYE=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=ghnyj06J3zEDCqyaUWXO4s2N8l0y7wped3zowBgIZXEqa3hf38diTaqSERr5Rn0oH\n\tPFBI+SEn49nNAxukp8qtVbbD07pWDZEa7wbOTpXvgVljUHQVa414wgCdYA2hQa1T5c\n\tCplLj8N7QIwsCZ1HZtmZ6Vy+ep++qFJWDMHQER7NrqDsZUiF0hXvSwv2U4k8R1GUqH\n\tYcBuILOUA45X8GfjvfUVo0dkKtPak2/+OrsMAhrgTH9YEQxxVJYp/fNZPVGbqSI66G\n\tkCFiwIdD+bB3yhW8jHhAPzp72Kgre8iG7GszYPr2eIextg4ZX05J6A4mujnOeYO543\n\tR5ODTBgfh+USQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1672203897;\n\tbh=ZxGD5lABg4cFLEdlecFKi8qJq5iXtNYrnWXq21pomYE=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=WWKQUlocMjV2ZNkqn4aCUBLX6JZ0XBydnxlstWjJd7MaJlaqYGK3ituJY1P0vRZbP\n\t5nYOPIkr+c7FSqNwEqZzf/P9fQvq4d7NnBPow6uijXsoH+ZsZmSm0M3yIBZ/XPyNn7\n\tuDPTB6X1FWlGg2OeT8ILZ5IoS5KSqaWAeRJQQ2ys="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"WWKQUloc\"; dkim-atps=neutral","Message-ID":"","Date":"Wed, 28 Dec 2022 10:34:46 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.5.1","To":"Paul Elder ","References":"<20221216122939.256534-1-paul.elder@ideasonboard.com>\n\t<20221216122939.256534-3-paul.elder@ideasonboard.com>\n\t<95670efe-0f3a-88eb-46f3-cc1bf1cd0000@ideasonboard.com>\n\t","Content-Language":"en-US","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v9 02/18] libcamera: property: Add\n\tMinimumRequests 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","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]