[{"id":12514,"web_url":"https://patchwork.libcamera.org/comment/12514/","msgid":"<20200915013217.GY15543@pendragon.ideasonboard.com>","date":"2020-09-15T01:32:17","subject":"Re: [libcamera-devel] [PATCH v2 12/13] libcamera: pipeline: rkisp1:\n\tAdd format validation for self path","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Mon, Sep 14, 2020 at 04:21:48PM +0200, Niklas Söderlund wrote:\n> Extend the format validation to work with both man and self path. The\n\ns/man/main/\ns/path/paths/\n\n> heuristics honors that the first stream in the configuration have the\n\ns/have/has/\n\n> highest priority while still examining both streams for a best match.\n> \n> This change extends the formats supported by the Cameras backed by this\n\ns/Cameras/cameras/\n\n> pipeline to also include RGB888 and RGB656, that is only available on\n> the self path.\n> \n> It is not possible to capture from the self path as the self stream is\n> not yet exposed to applications.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> * Changes since v1\n> - Store formats in std::vector instead of std::array to avoid template\n> usage for validate function.\n\nAs commented (too late) on v1, you don't have to do this, you can keep\nan array, and pass a Span to validate().\n\n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 222 +++++++++++++++++------\n> 1 file changed, 171 insertions(+), 51 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index bc961f8e78f2c979..851ff68f138b98dd 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -9,6 +9,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -40,7 +41,7 @@ LOG_DEFINE_CATEGORY(RkISP1)\n> namespace {\n> \tconstexpr Size RKISP1_RSZ_MP_SRC_MIN { 32, 16 };\n> \tconstexpr Size RKISP1_RSZ_MP_SRC_MAX { 4416, 3312 };\n> -\tconstexpr std::array RKISP1_RSZ_MP_FORMATS{\n> +\tconst std::vector RKISP1_RSZ_MP_FORMATS{\n> \t\tformats::YUYV,\n> \t\tformats::YVYU,\n> \t\tformats::VYUY,\n> @@ -50,7 +51,21 @@ namespace {\n> \t\tformats::NV12,\n> \t\t/* \\todo Add support for 8-bit greyscale to DRM formats */\n> \t};\n> -}\n> +\n> +\tconstexpr Size RKISP1_RSZ_SP_SRC_MIN { 32, 16 };\n> +\tconstexpr Size RKISP1_RSZ_SP_SRC_MAX { 1920, 1920 };\n> +\tconst std::vector RKISP1_RSZ_SP_FORMATS{\n> +\t\tformats::YUYV,\n> +\t\tformats::YVYU,\n> +\t\tformats::VYUY,\n> +\t\tformats::NV16,\n> +\t\tformats::NV61,\n> +\t\tformats::NV21,\n> +\t\tformats::NV12,\n> +\t\tformats::BGR888,\n> +\t\tformats::RGB565,\n> +\t};\n> +};\n> \n> class PipelineHandlerRkISP1;\n> class RkISP1ActionQueueBuffers;\n> @@ -181,13 +196,22 @@ public:\n> private:\n> \tstatic constexpr unsigned int RKISP1_BUFFER_COUNT = 4;\n> \n> +\tbool fitAnyPath(const StreamConfiguration &cfg);\n> +\n> +\tCameraConfiguration::Status validatePath(StreamConfiguration *cfg,\n> +\t\t\t\t\t\t const std::vector &formats,\n> +\t\t\t\t\t\t const Size &min, const Size &max,\n> +\t\t\t\t\t\t V4L2VideoDevice *video);\n> +\tCameraConfiguration::Status validateMainPath(StreamConfiguration *cfg);\n> +\tCameraConfiguration::Status validateSelfPath(StreamConfiguration *cfg);\n> +\n> \t/*\n> \t * The RkISP1CameraData instance is guaranteed to be valid as long as the\n> \t * corresponding Camera instance is valid. In order to borrow a\n> \t * reference to the camera data, store a new reference to the camera.\n> \t */\n> \tstd::shared_ptr camera_;\n> -\tconst RkISP1CameraData *data_;\n> +\tRkISP1CameraData *data_;\n\nThis isn't nice :-( I'd like to find a wait to keep it const. Can be\ndone in a separate patch.\n\n> \n> \tV4L2SubdeviceFormat sensorFormat_;\n> };\n> @@ -497,6 +521,75 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n> \tdata_ = data;\n> }\n> \n> +bool RkISP1CameraConfiguration::fitAnyPath(const StreamConfiguration &cfg)\n> +{\n> +\tif (std::find(RKISP1_RSZ_MP_FORMATS.begin(),\n> +\t\t RKISP1_RSZ_MP_FORMATS.end(), cfg.pixelFormat) ==\n> +\t RKISP1_RSZ_MP_FORMATS.end())\n> +\t\treturn false;\n> +\n> +\tif (cfg.size < RKISP1_RSZ_MP_SRC_MIN || cfg.size > RKISP1_RSZ_MP_SRC_MAX)\n> +\t\treturn false;\n> +\n> +\tif (std::find(RKISP1_RSZ_SP_FORMATS.begin(),\n> +\t\t RKISP1_RSZ_SP_FORMATS.end(), cfg.pixelFormat) ==\n> +\t RKISP1_RSZ_SP_FORMATS.end())\n> +\t\treturn false;\n> +\n> +\tif (cfg.size < RKISP1_RSZ_SP_SRC_MIN || cfg.size > RKISP1_RSZ_SP_SRC_MAX)\n> +\t\treturn false;\n> +\n> +\treturn true;\n\nThe implementation looks like a fitsAllPaths(), not any path.\n\n> +}\n> +\n> +CameraConfiguration::Status RkISP1CameraConfiguration::validatePath(\n> +\tStreamConfiguration *cfg, const std::vector &formats,\n> +\tconst Size &min, const Size &max, V4L2VideoDevice *video)\n\nformats, min and max are also candidates to be stored in the RkISP1Path\nclass.\n\n> +{\n> +\tconst StreamConfiguration reqCfg = *cfg;\n> +\tStatus status = Valid;\n> +\n> +\tif (std::find(formats.begin(), formats.end(), cfg->pixelFormat) ==\n> +\t formats.end())\n> +\t\tcfg->pixelFormat = formats::NV12;\n> +\n> +\tcfg->size.boundTo(max);\n> +\tcfg->size.expandTo(min);\n> +\tcfg->bufferCount = RKISP1_BUFFER_COUNT;\n> +\n> +\tV4L2DeviceFormat format = {};\n> +\tformat.fourcc = video->toV4L2PixelFormat(cfg->pixelFormat);\n> +\tformat.size = cfg->size;\n> +\n> +\tint ret = video->tryFormat(&format);\n> +\tif (ret)\n> +\t\treturn Invalid;\n> +\n> +\tcfg->stride = format.planes[0].bpl;\n> +\tcfg->frameSize = format.planes[0].size;\n> +\n> +\tif (cfg->pixelFormat != reqCfg.pixelFormat || cfg->size != reqCfg.size) {\n> +\t\tLOG(RkISP1, Debug)\n> +\t\t\t<< \"Adjusting format from \" << reqCfg.toString()\n> +\t\t\t<< \" to \" << cfg->toString();\n> +\t\tstatus = Adjusted;\n> +\t}\n> +\n> +\treturn status;\n> +}\n> +\n> +CameraConfiguration::Status RkISP1CameraConfiguration::validateMainPath(StreamConfiguration *cfg)\n> +{\n> +\treturn validatePath(cfg, RKISP1_RSZ_MP_FORMATS, RKISP1_RSZ_MP_SRC_MIN,\n> +\t\t\t RKISP1_RSZ_MP_SRC_MAX, data_->mainPathVideo_);\n> +}\n> +\n> +CameraConfiguration::Status RkISP1CameraConfiguration::validateSelfPath(StreamConfiguration *cfg)\n> +{\n> +\treturn validatePath(cfg, RKISP1_RSZ_SP_FORMATS, RKISP1_RSZ_SP_SRC_MIN,\n> +\t\t\t RKISP1_RSZ_SP_SRC_MAX, data_->selfPathVideo_);\n> +}\n\nAnd those two functions could then disappear. Are you really sure you\nwant to address all that on top ?\n\n> +\n> CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> {\n> \tconst CameraSensor *sensor = data_->sensor_;\n> @@ -506,22 +599,86 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \t\treturn Invalid;\n> \n> \t/* Cap the number of entries to the available streams. */\n> -\tif (config_.size() > 1) {\n> -\t\tconfig_.resize(1);\n> +\tif (config_.size() > 2) {\n> +\t\tconfig_.resize(2);\n> \t\tstatus = Adjusted;\n> \t}\n> \n> -\tStreamConfiguration &cfg = config_[0];\n> -\n> -\t/* Adjust the pixel format. */\n> -\tif (std::find(RKISP1_RSZ_MP_FORMATS.begin(), RKISP1_RSZ_MP_FORMATS.end(),\n> -\t\t cfg.pixelFormat) == RKISP1_RSZ_MP_FORMATS.end()) {\n> -\t\tLOG(RkISP1, Debug) << \"Adjusting format to NV12\";\n> -\t\tcfg.pixelFormat = formats::NV12,\n> -\t\tstatus = Adjusted;\n> +\t/*\n> +\t * If there are more then one stream in the configuration figure out the\n> +\t * order to evaluate them streams. The first stream have the highest\n\n\"them streams\" ?\ns/have/has/\n\n> +\t * priority but if both main path and self path can satisfy it evaluate\n> +\t * second stream first.\n\nWhy ?\n\n> +\t */\n> +\tstd::vector order(config_.size());\n> +\tstd::iota(order.begin(), order.end(), 0);\n> +\tif (config_.size() == 2 && fitAnyPath(config_[0]))\n> +\t\tstd::reverse(order.begin(), order.end());\n\nA bit complicated for a vector of two elements :-)\n\n> +\n> +\tbool mainPathAvailable = true;\n> +\tbool selfPathAvailable = true;\n> +\tfor (unsigned int index : order) {\n> +\t\tStreamConfiguration &cfg = config_[index];\n> +\n> +\t\t/* Try to match stream without adjusting configuration. */\n> +\t\tif (mainPathAvailable) {\n> +\t\t\tStreamConfiguration tryCfg = cfg;\n> +\t\t\tif (validateMainPath(&tryCfg) == Valid) {\n> +\t\t\t\tmainPathAvailable = false;\n> +\t\t\t\tcfg = tryCfg;\n> +\t\t\t\tcfg.setStream(&data_->mainPathStream_);\n> +\t\t\t\tLOG(RkISP1, Debug) << \"Exact match main\";\n\nThe message is a bit cryptic for someone reading the log without knowing\nthe code.\n\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tif (selfPathAvailable) {\n> +\t\t\tStreamConfiguration tryCfg = cfg;\n> +\t\t\tif (validateSelfPath(&tryCfg) == Valid) {\n> +\t\t\t\tselfPathAvailable = false;\n> +\t\t\t\tcfg = tryCfg;\n> +\t\t\t\tcfg.setStream(&data_->selfPathStream_);\n> +\t\t\t\tLOG(RkISP1, Debug) << \"Exact match self\";\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\t/* Try to match stream allowing adjusting configuration. */\n> +\t\tif (mainPathAvailable) {\n> +\t\t\tStreamConfiguration tryCfg = cfg;\n> +\t\t\tif (validateMainPath(&tryCfg) == Adjusted) {\n> +\t\t\t\tmainPathAvailable = false;\n> +\t\t\t\tcfg = tryCfg;\n> +\t\t\t\tcfg.setStream(&data_->mainPathStream_);\n> +\t\t\t\tstatus = Adjusted;\n> +\t\t\t\tLOG(RkISP1, Debug) << \"Adjust match main\";\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tif (selfPathAvailable) {\n> +\t\t\tStreamConfiguration tryCfg = cfg;\n> +\t\t\tif (validateSelfPath(&tryCfg) == Adjusted) {\n> +\t\t\t\tselfPathAvailable = false;\n> +\t\t\t\tcfg = tryCfg;\n> +\t\t\t\tcfg.setStream(&data_->selfPathStream_);\n> +\t\t\t\tstatus = Adjusted;\n> +\t\t\t\tLOG(RkISP1, Debug) << \"Adjust match self\";\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\t\t}\n\nThere's lots of code duplication, adding a RkISP1Path class would help.\n\n> +\n> +\t\t/* All paths rejected configuraiton. */\n> +\t\tLOG(RkISP1, Debug) << \"Camera configuration not supported \"\n> +\t\t\t\t << cfg.toString();\n> +\t\treturn Invalid;\n> \t}\n> \n> \t/* Select the sensor format. */\n> +\tSize maxSize;\n> +\tfor (const StreamConfiguration &cfg : config_)\n> +\t\tmaxSize = std::max(maxSize, cfg.size);\n> +\n> \tsensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,\n> \t\t\t\t\t MEDIA_BUS_FMT_SGBRG12_1X12,\n> \t\t\t\t\t MEDIA_BUS_FMT_SGRBG12_1X12,\n> @@ -534,47 +691,10 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \t\t\t\t\t MEDIA_BUS_FMT_SGBRG8_1X8,\n> \t\t\t\t\t MEDIA_BUS_FMT_SGRBG8_1X8,\n> \t\t\t\t\t MEDIA_BUS_FMT_SRGGB8_1X8 },\n> -\t\t\t\t\t cfg.size);\n> +\t\t\t\t\t maxSize);\n> \tif (sensorFormat_.size.isNull())\n> \t\tsensorFormat_.size = sensor->resolution();\n> \n> -\t/*\n> -\t * Provide a suitable default that matches the sensor aspect\n> -\t * ratio and clamp the size to the hardware bounds.\n> -\t *\n> -\t * \\todo: Check the hardware alignment constraints.\n> -\t */\n> -\tconst Size size = cfg.size;\n> -\n> -\tif (cfg.size.isNull()) {\n> -\t\tcfg.size.width = 1280;\n> -\t\tcfg.size.height = 1280 * sensorFormat_.size.height\n> -\t\t\t\t/ sensorFormat_.size.width;\n> -\t}\n> -\n> -\tcfg.size.boundTo(RKISP1_RSZ_MP_SRC_MAX);\n> -\tcfg.size.expandTo(RKISP1_RSZ_MP_SRC_MIN);\n> -\n> -\tif (cfg.size != size) {\n> -\t\tLOG(RkISP1, Debug)\n> -\t\t\t<< \"Adjusting size from \" << size.toString()\n> -\t\t\t<< \" to \" << cfg.size.toString();\n> -\t\tstatus = Adjusted;\n> -\t}\n> -\n> -\tcfg.bufferCount = RKISP1_BUFFER_COUNT;\n> -\n> -\tV4L2DeviceFormat format = {};\n> -\tformat.fourcc = data_->mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat);\n> -\tformat.size = cfg.size;\n> -\n> -\tint ret = data_->mainPathVideo_->tryFormat(&format);\n> -\tif (ret)\n> -\t\treturn Invalid;\n> -\n> -\tcfg.stride = format.planes[0].bpl;\n> -\tcfg.frameSize = format.planes[0].size;\n> -\n> \treturn status;\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 5CF1ABF01C\n\tfor ;\n\tTue, 15 Sep 2020 01:32:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DFA8662DAB;\n\tTue, 15 Sep 2020 03:32:47 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1A26062901\n\tfor ;\n\tTue, 15 Sep 2020 03:32:46 +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 9B788275;\n\tTue, 15 Sep 2020 03:32:45 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"PTpAAYH1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1600133565;\n\tbh=ZuCWSGoG/mB+BPGnQp+mXKGzfAZAf8LfLCMqAbuKXyw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PTpAAYH1/pYPpGgmNAvDmF/W3XiII5lhXmQM+nyVi0W5ywFTkoAwC4QCH+0Nx7dtt\n\t1K/SptOLEGJ7kM0RX+eUbP2uH5/chZykGIlV5gyT8CojWXRb1lyzDgRtHLwqHButw3\n\th9pGZNGOQJYt1kIN1TTHU65rbCI88RzPHJtjODRk=","Date":"Tue, 15 Sep 2020 04:32:17 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200915013217.GY15543@pendragon.ideasonboard.com>","References":"<20200914142149.63857-1-niklas.soderlund@ragnatech.se>\n\t<20200914142149.63857-13-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200914142149.63857-13-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v2 12/13] libcamera: pipeline: rkisp1:\n\tAdd format validation for self path","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","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12642,"web_url":"https://patchwork.libcamera.org/comment/12642/","msgid":"<20200922144818.GB502216@oden.dyn.berto.se>","date":"2020-09-22T14:48:18","subject":"Re: [libcamera-devel] [PATCH v2 12/13] libcamera: pipeline: rkisp1:\n\tAdd format validation for self path","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your feedback.\n\nOn 2020-09-15 04:32:17 +0300, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Mon, Sep 14, 2020 at 04:21:48PM +0200, Niklas Söderlund wrote:\n> > Extend the format validation to work with both man and self path. The\n> \n> s/man/main/\n> s/path/paths/\n> \n> > heuristics honors that the first stream in the configuration have the\n> \n> s/have/has/\n> \n> > highest priority while still examining both streams for a best match.\n> > \n> > This change extends the formats supported by the Cameras backed by this\n> \n> s/Cameras/cameras/\n> \n> > pipeline to also include RGB888 and RGB656, that is only available on\n> > the self path.\n> > \n> > It is not possible to capture from the self path as the self stream is\n> > not yet exposed to applications.\n> > \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > * Changes since v1\n> > - Store formats in std::vector instead of std::array to avoid template\n> > usage for validate function.\n> \n> As commented (too late) on v1, you don't have to do this, you can keep\n> an array, and pass a Span to validate().\n> \n> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 222 +++++++++++++++++------\n> > 1 file changed, 171 insertions(+), 51 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index bc961f8e78f2c979..851ff68f138b98dd 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -9,6 +9,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > \n> > #include \n> > @@ -40,7 +41,7 @@ LOG_DEFINE_CATEGORY(RkISP1)\n> > namespace {\n> > \tconstexpr Size RKISP1_RSZ_MP_SRC_MIN { 32, 16 };\n> > \tconstexpr Size RKISP1_RSZ_MP_SRC_MAX { 4416, 3312 };\n> > -\tconstexpr std::array RKISP1_RSZ_MP_FORMATS{\n> > +\tconst std::vector RKISP1_RSZ_MP_FORMATS{\n> > \t\tformats::YUYV,\n> > \t\tformats::YVYU,\n> > \t\tformats::VYUY,\n> > @@ -50,7 +51,21 @@ namespace {\n> > \t\tformats::NV12,\n> > \t\t/* \\todo Add support for 8-bit greyscale to DRM formats */\n> > \t};\n> > -}\n> > +\n> > +\tconstexpr Size RKISP1_RSZ_SP_SRC_MIN { 32, 16 };\n> > +\tconstexpr Size RKISP1_RSZ_SP_SRC_MAX { 1920, 1920 };\n> > +\tconst std::vector RKISP1_RSZ_SP_FORMATS{\n> > +\t\tformats::YUYV,\n> > +\t\tformats::YVYU,\n> > +\t\tformats::VYUY,\n> > +\t\tformats::NV16,\n> > +\t\tformats::NV61,\n> > +\t\tformats::NV21,\n> > +\t\tformats::NV12,\n> > +\t\tformats::BGR888,\n> > +\t\tformats::RGB565,\n> > +\t};\n> > +};\n> > \n> > class PipelineHandlerRkISP1;\n> > class RkISP1ActionQueueBuffers;\n> > @@ -181,13 +196,22 @@ public:\n> > private:\n> > \tstatic constexpr unsigned int RKISP1_BUFFER_COUNT = 4;\n> > \n> > +\tbool fitAnyPath(const StreamConfiguration &cfg);\n> > +\n> > +\tCameraConfiguration::Status validatePath(StreamConfiguration *cfg,\n> > +\t\t\t\t\t\t const std::vector &formats,\n> > +\t\t\t\t\t\t const Size &min, const Size &max,\n> > +\t\t\t\t\t\t V4L2VideoDevice *video);\n> > +\tCameraConfiguration::Status validateMainPath(StreamConfiguration *cfg);\n> > +\tCameraConfiguration::Status validateSelfPath(StreamConfiguration *cfg);\n> > +\n> > \t/*\n> > \t * The RkISP1CameraData instance is guaranteed to be valid as long as the\n> > \t * corresponding Camera instance is valid. In order to borrow a\n> > \t * reference to the camera data, store a new reference to the camera.\n> > \t */\n> > \tstd::shared_ptr camera_;\n> > -\tconst RkISP1CameraData *data_;\n> > +\tRkISP1CameraData *data_;\n> \n> This isn't nice :-( I'd like to find a wait to keep it const. Can be\n> done in a separate patch.\n\nI really tried to keep this const but all I can think of is the fetch \nthe Stream* from camera_ instead of data_ in the rather odd hack,\n\n Stream *mainStream = nullptr;\n Stream *selfStream = nullptr;\n for (Stream *stream : camera_->streams()) {\n if (stream == &data_->mainPathStream_)\n mainStream = stream;\n else if (stream == &data_->selfPathStream_)\n selfStream = stream;\n }\n ASSERT(mainStream && selfStream);\n\nBut this seems like casting a const variable to a non-const with extra \nsteps. Looking at the IPU3 pipeline handler this is exactly what is \ndone, from IPU3CameraConfiguration::validate():\n\n cfg->setStream(const_cast(&data_->outStream_));\n\nI think this highlights an issue with our overall design and something \nthat should be corrected. As this transient more pipeline handlers I \nwill record this as a todo and use the IPU3 pattern here to make it easy \nto find a solution which solves the root problem.\n\n> \n> > \n> > \tV4L2SubdeviceFormat sensorFormat_;\n> > };\n> > @@ -497,6 +521,75 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n> > \tdata_ = data;\n> > }\n> > \n> > +bool RkISP1CameraConfiguration::fitAnyPath(const StreamConfiguration &cfg)\n> > +{\n> > +\tif (std::find(RKISP1_RSZ_MP_FORMATS.begin(),\n> > +\t\t RKISP1_RSZ_MP_FORMATS.end(), cfg.pixelFormat) ==\n> > +\t RKISP1_RSZ_MP_FORMATS.end())\n> > +\t\treturn false;\n> > +\n> > +\tif (cfg.size < RKISP1_RSZ_MP_SRC_MIN || cfg.size > RKISP1_RSZ_MP_SRC_MAX)\n> > +\t\treturn false;\n> > +\n> > +\tif (std::find(RKISP1_RSZ_SP_FORMATS.begin(),\n> > +\t\t RKISP1_RSZ_SP_FORMATS.end(), cfg.pixelFormat) ==\n> > +\t RKISP1_RSZ_SP_FORMATS.end())\n> > +\t\treturn false;\n> > +\n> > +\tif (cfg.size < RKISP1_RSZ_SP_SRC_MIN || cfg.size > RKISP1_RSZ_SP_SRC_MAX)\n> > +\t\treturn false;\n> > +\n> > +\treturn true;\n> \n> The implementation looks like a fitsAllPaths(), not any path.\n> \n> > +}\n> > +\n> > +CameraConfiguration::Status RkISP1CameraConfiguration::validatePath(\n> > +\tStreamConfiguration *cfg, const std::vector &formats,\n> > +\tconst Size &min, const Size &max, V4L2VideoDevice *video)\n> \n> formats, min and max are also candidates to be stored in the RkISP1Path\n> class.\n> \n> > +{\n> > +\tconst StreamConfiguration reqCfg = *cfg;\n> > +\tStatus status = Valid;\n> > +\n> > +\tif (std::find(formats.begin(), formats.end(), cfg->pixelFormat) ==\n> > +\t formats.end())\n> > +\t\tcfg->pixelFormat = formats::NV12;\n> > +\n> > +\tcfg->size.boundTo(max);\n> > +\tcfg->size.expandTo(min);\n> > +\tcfg->bufferCount = RKISP1_BUFFER_COUNT;\n> > +\n> > +\tV4L2DeviceFormat format = {};\n> > +\tformat.fourcc = video->toV4L2PixelFormat(cfg->pixelFormat);\n> > +\tformat.size = cfg->size;\n> > +\n> > +\tint ret = video->tryFormat(&format);\n> > +\tif (ret)\n> > +\t\treturn Invalid;\n> > +\n> > +\tcfg->stride = format.planes[0].bpl;\n> > +\tcfg->frameSize = format.planes[0].size;\n> > +\n> > +\tif (cfg->pixelFormat != reqCfg.pixelFormat || cfg->size != reqCfg.size) {\n> > +\t\tLOG(RkISP1, Debug)\n> > +\t\t\t<< \"Adjusting format from \" << reqCfg.toString()\n> > +\t\t\t<< \" to \" << cfg->toString();\n> > +\t\tstatus = Adjusted;\n> > +\t}\n> > +\n> > +\treturn status;\n> > +}\n> > +\n> > +CameraConfiguration::Status RkISP1CameraConfiguration::validateMainPath(StreamConfiguration *cfg)\n> > +{\n> > +\treturn validatePath(cfg, RKISP1_RSZ_MP_FORMATS, RKISP1_RSZ_MP_SRC_MIN,\n> > +\t\t\t RKISP1_RSZ_MP_SRC_MAX, data_->mainPathVideo_);\n> > +}\n> > +\n> > +CameraConfiguration::Status RkISP1CameraConfiguration::validateSelfPath(StreamConfiguration *cfg)\n> > +{\n> > +\treturn validatePath(cfg, RKISP1_RSZ_SP_FORMATS, RKISP1_RSZ_SP_SRC_MIN,\n> > +\t\t\t RKISP1_RSZ_SP_SRC_MAX, data_->selfPathVideo_);\n> > +}\n> \n> And those two functions could then disappear. Are you really sure you\n> want to address all that on top ?\n\nThat is my current plan. But to extinguish any doubt of that work I will \ndo it on-top now and submit it as part of v3.\n\n> \n> > +\n> > CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > {\n> > \tconst CameraSensor *sensor = data_->sensor_;\n> > @@ -506,22 +599,86 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > \t\treturn Invalid;\n> > \n> > \t/* Cap the number of entries to the available streams. */\n> > -\tif (config_.size() > 1) {\n> > -\t\tconfig_.resize(1);\n> > +\tif (config_.size() > 2) {\n> > +\t\tconfig_.resize(2);\n> > \t\tstatus = Adjusted;\n> > \t}\n> > \n> > -\tStreamConfiguration &cfg = config_[0];\n> > -\n> > -\t/* Adjust the pixel format. */\n> > -\tif (std::find(RKISP1_RSZ_MP_FORMATS.begin(), RKISP1_RSZ_MP_FORMATS.end(),\n> > -\t\t cfg.pixelFormat) == RKISP1_RSZ_MP_FORMATS.end()) {\n> > -\t\tLOG(RkISP1, Debug) << \"Adjusting format to NV12\";\n> > -\t\tcfg.pixelFormat = formats::NV12,\n> > -\t\tstatus = Adjusted;\n> > +\t/*\n> > +\t * If there are more then one stream in the configuration figure out the\n> > +\t * order to evaluate them streams. The first stream have the highest\n> \n> \"them streams\" ?\n> s/have/has/\n> \n> > +\t * priority but if both main path and self path can satisfy it evaluate\n> > +\t * second stream first.\n> \n> Why ?\n> \n> > +\t */\n> > +\tstd::vector order(config_.size());\n> > +\tstd::iota(order.begin(), order.end(), 0);\n> > +\tif (config_.size() == 2 && fitAnyPath(config_[0]))\n> > +\t\tstd::reverse(order.begin(), order.end());\n> \n> A bit complicated for a vector of two elements :-)\n> \n> > +\n> > +\tbool mainPathAvailable = true;\n> > +\tbool selfPathAvailable = true;\n> > +\tfor (unsigned int index : order) {\n> > +\t\tStreamConfiguration &cfg = config_[index];\n> > +\n> > +\t\t/* Try to match stream without adjusting configuration. */\n> > +\t\tif (mainPathAvailable) {\n> > +\t\t\tStreamConfiguration tryCfg = cfg;\n> > +\t\t\tif (validateMainPath(&tryCfg) == Valid) {\n> > +\t\t\t\tmainPathAvailable = false;\n> > +\t\t\t\tcfg = tryCfg;\n> > +\t\t\t\tcfg.setStream(&data_->mainPathStream_);\n> > +\t\t\t\tLOG(RkISP1, Debug) << \"Exact match main\";\n> \n> The message is a bit cryptic for someone reading the log without knowing\n> the code.\n\nSure, but is not the Debug level intended for people debugging the code? \nI would be fine dropping the LOG() statements all together. I found them \nuseful when testing this series and chased to leave them in.\n\n> \n> > +\t\t\t\tcontinue;\n> > +\t\t\t}\n> > +\t\t}\n> > +\n> > +\t\tif (selfPathAvailable) {\n> > +\t\t\tStreamConfiguration tryCfg = cfg;\n> > +\t\t\tif (validateSelfPath(&tryCfg) == Valid) {\n> > +\t\t\t\tselfPathAvailable = false;\n> > +\t\t\t\tcfg = tryCfg;\n> > +\t\t\t\tcfg.setStream(&data_->selfPathStream_);\n> > +\t\t\t\tLOG(RkISP1, Debug) << \"Exact match self\";\n> > +\t\t\t\tcontinue;\n> > +\t\t\t}\n> > +\t\t}\n> > +\n> > +\t\t/* Try to match stream allowing adjusting configuration. */\n> > +\t\tif (mainPathAvailable) {\n> > +\t\t\tStreamConfiguration tryCfg = cfg;\n> > +\t\t\tif (validateMainPath(&tryCfg) == Adjusted) {\n> > +\t\t\t\tmainPathAvailable = false;\n> > +\t\t\t\tcfg = tryCfg;\n> > +\t\t\t\tcfg.setStream(&data_->mainPathStream_);\n> > +\t\t\t\tstatus = Adjusted;\n> > +\t\t\t\tLOG(RkISP1, Debug) << \"Adjust match main\";\n> > +\t\t\t\tcontinue;\n> > +\t\t\t}\n> > +\t\t}\n> > +\n> > +\t\tif (selfPathAvailable) {\n> > +\t\t\tStreamConfiguration tryCfg = cfg;\n> > +\t\t\tif (validateSelfPath(&tryCfg) == Adjusted) {\n> > +\t\t\t\tselfPathAvailable = false;\n> > +\t\t\t\tcfg = tryCfg;\n> > +\t\t\t\tcfg.setStream(&data_->selfPathStream_);\n> > +\t\t\t\tstatus = Adjusted;\n> > +\t\t\t\tLOG(RkISP1, Debug) << \"Adjust match self\";\n> > +\t\t\t\tcontinue;\n> > +\t\t\t}\n> > +\t\t}\n> \n> There's lots of code duplication, adding a RkISP1Path class would help.\n> \n> > +\n> > +\t\t/* All paths rejected configuraiton. */\n> > +\t\tLOG(RkISP1, Debug) << \"Camera configuration not supported \"\n> > +\t\t\t\t << cfg.toString();\n> > +\t\treturn Invalid;\n> > \t}\n> > \n> > \t/* Select the sensor format. */\n> > +\tSize maxSize;\n> > +\tfor (const StreamConfiguration &cfg : config_)\n> > +\t\tmaxSize = std::max(maxSize, cfg.size);\n> > +\n> > \tsensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,\n> > \t\t\t\t\t MEDIA_BUS_FMT_SGBRG12_1X12,\n> > \t\t\t\t\t MEDIA_BUS_FMT_SGRBG12_1X12,\n> > @@ -534,47 +691,10 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > \t\t\t\t\t MEDIA_BUS_FMT_SGBRG8_1X8,\n> > \t\t\t\t\t MEDIA_BUS_FMT_SGRBG8_1X8,\n> > \t\t\t\t\t MEDIA_BUS_FMT_SRGGB8_1X8 },\n> > -\t\t\t\t\t cfg.size);\n> > +\t\t\t\t\t maxSize);\n> > \tif (sensorFormat_.size.isNull())\n> > \t\tsensorFormat_.size = sensor->resolution();\n> > \n> > -\t/*\n> > -\t * Provide a suitable default that matches the sensor aspect\n> > -\t * ratio and clamp the size to the hardware bounds.\n> > -\t *\n> > -\t * \\todo: Check the hardware alignment constraints.\n> > -\t */\n> > -\tconst Size size = cfg.size;\n> > -\n> > -\tif (cfg.size.isNull()) {\n> > -\t\tcfg.size.width = 1280;\n> > -\t\tcfg.size.height = 1280 * sensorFormat_.size.height\n> > -\t\t\t\t/ sensorFormat_.size.width;\n> > -\t}\n> > -\n> > -\tcfg.size.boundTo(RKISP1_RSZ_MP_SRC_MAX);\n> > -\tcfg.size.expandTo(RKISP1_RSZ_MP_SRC_MIN);\n> > -\n> > -\tif (cfg.size != size) {\n> > -\t\tLOG(RkISP1, Debug)\n> > -\t\t\t<< \"Adjusting size from \" << size.toString()\n> > -\t\t\t<< \" to \" << cfg.size.toString();\n> > -\t\tstatus = Adjusted;\n> > -\t}\n> > -\n> > -\tcfg.bufferCount = RKISP1_BUFFER_COUNT;\n> > -\n> > -\tV4L2DeviceFormat format = {};\n> > -\tformat.fourcc = data_->mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat);\n> > -\tformat.size = cfg.size;\n> > -\n> > -\tint ret = data_->mainPathVideo_->tryFormat(&format);\n> > -\tif (ret)\n> > -\t\treturn Invalid;\n> > -\n> > -\tcfg.stride = format.planes[0].bpl;\n> > -\tcfg.frameSize = format.planes[0].size;\n> > -\n> > \treturn status;\n> > }\n> > \n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 4817FC3B5B\n\tfor ;\n\tTue, 22 Sep 2020 14:48:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C303662FD8;\n\tTue, 22 Sep 2020 16:48:22 +0200 (CEST)","from mail-lf1-x131.google.com (mail-lf1-x131.google.com\n\t[IPv6:2a00:1450:4864:20::131])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2D94162FD6\n\tfor ;\n\tTue, 22 Sep 2020 16:48:21 +0200 (CEST)","by mail-lf1-x131.google.com with SMTP id y11so18347340lfl.5\n\tfor ;\n\tTue, 22 Sep 2020 07:48:21 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tv11sm3560200lfg.39.2020.09.22.07.48.18\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 22 Sep 2020 07:48:19 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"bBtA6wMU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=TMwauzFBDhOuRGo+lv2vqvocCfdnPm+cQ5FCVObIEuE=;\n\tb=bBtA6wMUTerC/DDBQtl1jWZ/x+erSaj2ys1UVjrMTAdCRD0JJl0f772HpbFx0IgUG+\n\to2MHpLLXRr9lok3lKAxHh7p0yEeGcKEjGOEv3X//6lubIMzvovvi+UMQn0a6YunCQDkn\n\t3El6gNjKnDX0qmKdVE4i0yKvsSw/bf5XMexUg3DyUQVHnUZSJVyxNRIxEWYUQvsOoStv\n\t0yOzvMLJgzqaTubWNlnliUp/x6JphSCgmRQydnZKYuk5i8sdwaL0RncrUdpmvP9+ioKq\n\tFnePg3PlTBKMdngb5NEsQCvaQnHjTQL5XpNxX7ndPsGHdXpuIYe40UyAx1gXb8oLRUIR\n\tmeRg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=TMwauzFBDhOuRGo+lv2vqvocCfdnPm+cQ5FCVObIEuE=;\n\tb=Wo7sea/EWYl28k47BLwph1z6/zclPhK9JoyBnko4Je/x7m34CWj0xlMXsnKIp/2iV0\n\t6fbVBm1OMyo2RPa4//PfllISMjaRHAy234C01Q6YSVXKO69LaRT/DIfBAs/Sch50XeRN\n\tIGvvGTX8Kc9DNnnU+0gpYmpJx9lnRWgrP4rG5RrKzCWI1RqiaFc2YAoZ6VOwlihdYcMB\n\tkUb7hvWW8gMx/JBQ7FaHx4rnNDn4Ti5StL8XWsqMrpBQTHGDyVu5bxnqmqXgnc9U6Kgh\n\tIVJmFY6cB9HOQMHfKmmwb2Dj+/O90y8Uf24t3U7kkeFp9ZlJSusfWPKd0aGPsv6xgMJH\n\t3bjQ==","X-Gm-Message-State":"AOAM530qakrfuElA8RH8W5jWszYCTeTiwo68pkakpRmBz9TCAGMokSbU\n\tQ7SlxtN2Lk3BYOL9zeVQIxtyKcQUwWLF7Q==","X-Google-Smtp-Source":"ABdhPJyxVleQXHX5c86+zN+Z4waKrWkd9pRIfikM+Reb5xSXed0yOLKKbdApRuDjlRtVjvdgknVKeA==","X-Received":"by 2002:ac2:44b7:: with SMTP id\n\tc23mr1746031lfm.128.1600786100265; \n\tTue, 22 Sep 2020 07:48:20 -0700 (PDT)","Date":"Tue, 22 Sep 2020 16:48:18 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Message-ID":"<20200922144818.GB502216@oden.dyn.berto.se>","References":"<20200914142149.63857-1-niklas.soderlund@ragnatech.se>\n\t<20200914142149.63857-13-niklas.soderlund@ragnatech.se>\n\t<20200915013217.GY15543@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200915013217.GY15543@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 12/13] libcamera: pipeline: rkisp1:\n\tAdd format validation for self path","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","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]