[{"id":18451,"web_url":"https://patchwork.libcamera.org/comment/18451/","msgid":"","date":"2021-08-01T16:45:24","subject":"Re: [libcamera-devel] [PATCH v7 01/11] libcamera: property: Add\n\tMinimumRequests property","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nícolas,\n\nThank you for the patch.\n\nOn Thu, Jul 22, 2021 at 08:28:41PM -0300, Nícolas F. R. A. Prado wrote:\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. At this quantity, there's no guarantee that\n> frames won't be dropped or that manual per-frame controls will apply\n> correctly. The quantity needed for those may be added as separate\n> properties in the future.\n\nThe second sentence should be part of the property definition, as it's\nfairly important.\n\n> By default the property is set to 1 in the CameraSensor constructor, and\n> it can be overridden by each pipeline. The uvcvideo pipeline explicitly\n> sets the property to 1 since it doesn't have a CameraSensor.\n\nThe value is a property of a pipeline, so I don't think setting a\ndefault in CameraSensor is a good idea. I'd rather have all pipeline\nhandlers setting the property. Most of them do already, so it shouldn't\nbe a big issue.\n\n> Signed-off-by: Nícolas F. R. A. Prado \n> ---\n> \n> Changes in v7:\n> - Thanks to Kieran:\n> - Renamed property from MinNumRequests to MinimumRequests\n> - Thanks to Jacopo:\n> - Changed property's description\n> \n> Changes in v6:\n> - Thanks to Naushir:\n> - Removed comment from Raspberrypi MinNumRequests setting\n> - Removed an extra blank line\n> \n> src/libcamera/camera_sensor.cpp | 1 +\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 ++\n> src/libcamera/pipeline/simple/simple.cpp | 2 ++\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 ++\n> src/libcamera/pipeline/vimc/vimc.cpp | 2 ++\n> src/libcamera/property_ids.yaml | 10 ++++++++++\n> 6 files changed, 19 insertions(+)\n> \n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index cde431cc4c80..d8ed010bfd08 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -57,6 +57,7 @@ CameraSensor::CameraSensor(const MediaEntity *entity)\n> \t: entity_(entity), pad_(UINT_MAX), bayerFormat_(nullptr),\n> \t properties_(properties::properties)\n> {\n> +\tproperties_.set(properties::MinimumRequests, 1);\n> }\n> \n> /**\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 42911a8fdfdb..cb4ca9a85fa8 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -24,6 +24,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> \n> @@ -944,6 +945,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> \n> \t/* Initialize the camera properties. */\n> \tdata->properties_ = data->sensor_->properties();\n> +\tdata->properties_.set(properties::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 b29fff9820e5..348c554c8be4 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -25,6 +25,7 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> #include \n> \n> @@ -436,6 +437,7 @@ int SimpleCameraData::init()\n> \t}\n> \n> \tproperties_ = sensor_->properties();\n> +\tproperties_.set(properties::MinimumRequests, 3);\n\nThe value needs to be device-dependent. Could you store it in the\nSimplePipelineInfo structure ? imx7-csi needs 2 buffers, sun6i-csi needs\n3, and I believe qcom-camss needs 1.\n\n> \n> \treturn 0;\n> }\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 0f634b8da609..6ad7fb3c9f0c 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -525,6 +525,8 @@ int UVCCameraData::init(MediaDevice *media)\n> \tproperties_.set(properties::PixelArraySize, resolution);\n> \tproperties_.set(properties::PixelArrayActiveAreas, { Rectangle(resolution) });\n> \n> +\tproperties_.set(properties::MinimumRequests, 1);\n> +\n> \t/* Initialise the supported controls. */\n> \tControlInfoMap::Map ctrls;\n> \n> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> index 12f7517fd0ae..44c198ccf8b8 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -21,6 +21,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> \n> @@ -516,6 +517,7 @@ int VimcCameraData::init()\n> \n> \t/* Initialize the camera properties. */\n> \tproperties_ = sensor_->properties();\n> +\tproperties_.set(properties::MinimumRequests, 2);\n\nIt would be nice if the vimc driver could function with a single buffer\n(out of scope for this patch series of course).\n\n> \n> \treturn 0;\n> }\n> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> index 12ecbce5eed4..23ef0e9d38c4 100644\n> --- a/src/libcamera/property_ids.yaml\n> +++ b/src/libcamera/property_ids.yaml\n> @@ -678,6 +678,16 @@ controls:\n> \\todo Turn this property into a \"maximum control value\" for the\n> ScalerCrop control once \"dynamic\" controls have been implemented.\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.\n\nSo this would become\n\n\tThe minimum number of capture requests that the camera pipeline requires\n\tto have queued in order to sustain capture operations. At this quantity,\n\tthere's no guarantee that frames won't be dropped or that manual\n\tper-frame controls will apply correctly.\n\n> +\n> + This property usually corresponds to the minimum number of memory\n> + buffers needed to fill the capture pipeline composed of hardware\n> + processing blocks.\n\nCould you add the following comment ?\n\n\t\\todo Extend this property to expose the different minimum buffer and\n\trequest counts (the minimum number of buffers to be able to capture at\n\tall, the minimum number of buffers to sustain capture without frame\n\tdrop, and the minimum number of requests to ensure proper operation of\n\tper-frame controls).\n\nWith this,\n\nReviewed-by: Laurent Pinchart \n\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 6CACABD878\n\tfor ;\n\tSun, 1 Aug 2021 16:45:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C41FE687B1;\n\tSun, 1 Aug 2021 18:45:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 90573687B1\n\tfor ;\n\tSun, 1 Aug 2021 18:45:35 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0CBA087C;\n\tSun, 1 Aug 2021 18:45:34 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"uAIxdEEi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627836335;\n\tbh=PCaQRge2lA/OP1xdOF09Bh6//IPZ9GfDADB25Fr+RdU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uAIxdEEiqZ6AkGFY8uyxXqVKbLZvQFfRrhWUQ1antkbC1S1tkLcYMLtVljajf1uSi\n\t3oqdA65hp4jdiriiOUlDfCi4P7h4yIPogjfqhVoAHrQCwxuU2FESghLQOJRbRSnRoe\n\tB/39D5VYKifvPLtJznGRIliz3X0+ZrcrLtgURpm8=","Date":"Sun, 1 Aug 2021 19:45:24 +0300","From":"Laurent Pinchart ","To":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado ","Message-ID":"","References":"<20210722232851.747614-1-nfraprado@collabora.com>\n\t<20210722232851.747614-2-nfraprado@collabora.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210722232851.747614-2-nfraprado@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH v7 01/11] 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","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?b?bmRyw6k=?= Almeida ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]