[{"id":12775,"web_url":"https://patchwork.libcamera.org/comment/12775/","msgid":"<20200925150245.fw2vfp4ddosm5pxp@uno.localdomain>","date":"2020-09-25T15:02:45","subject":"Re: [libcamera-devel] [PATCH v3 13/22] libcamera: pipeline: rkisp1:\n\tAdd format validation for self path","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Fri, Sep 25, 2020 at 03:41:58AM +0200, Niklas Söderlund wrote:\n> Extend the format validation to work with both main and self paths. The\n> heuristics honors that the first stream in the configuration has the\n> highest priority while still examining both streams for a best match.\n\nI've heard you mentioning the priority of streams being defined by\ntheir order multiple time, but this seems to be specific to this\npipeline handler, or have I missed it ?\n\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 v2\n> - Fix spelling in commit message.\n> - Use Span<> instead of turning arrays to vectors.\n> - Keep data_ const and cast 'const Streams*' to non-const using\n> const_cast() to match the IPU3 pipeline.\n> - Rename fitAnyPath() to fitsAllPaths().\n> - Expand documentation for why second stream is evaluated first if the\n> fist stream can use either stream.\n> - Drop support for RGB888 and RGB656 for selfpath which was present in\n> v2 as the driver delivers odd data when the frames are observed.\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> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 210 +++++++++++++++++------\n> 1 file changed, 162 insertions(+), 48 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index cd3049485746edd6..bd53183a034efaff 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> @@ -19,6 +20,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n>\n> #include \"libcamera/internal/camera_sensor.h\"\n> @@ -50,6 +52,19 @@ constexpr std::array RKISP1_RSZ_MP_FORMATS{\n> \tformats::NV12,\n> \t/* \\todo Add support for 8-bit greyscale to DRM formats */\n> };\n> +\n> +constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 };\n> +constexpr Size RKISP1_RSZ_SP_SRC_MAX{ 1920, 1920 };\n\n1920x1920 ? Maybe it's just the way it actually is\n\n> +constexpr std::array RKISP1_RSZ_SP_FORMATS{\n> +\tformats::YUYV,\n> +\tformats::YVYU,\n> +\tformats::VYUY,\n> +\tformats::NV16,\n> +\tformats::NV61,\n> +\tformats::NV21,\n> +\tformats::NV12,\n> +\t/* \\todo Add support for BGR888 and RGB565 */\n> +};\n\nAren't these equal to the main path ones ?\n\n> } /* namespace */\n>\n> class PipelineHandlerRkISP1;\n> @@ -181,6 +196,14 @@ public:\n> private:\n> \tstatic constexpr unsigned int RKISP1_BUFFER_COUNT = 4;\n>\n> +\tCameraConfiguration::Status validatePath(StreamConfiguration *cfg,\n> +\t\t\t\t\t\t const Span &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> +\tbool fitsAllPaths(const 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> @@ -492,6 +515,69 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n> \tdata_ = data;\n> }\n>\n> +CameraConfiguration::Status RkISP1CameraConfiguration::validatePath(\n> +\tStreamConfiguration *cfg, const Span &formats,\n> +\tconst Size &min, const Size &max, V4L2VideoDevice *video)\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\nThis could be a one liner, but it's a matter of tastes\n\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\nShould you check for bufferCount too ?\n\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> +bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n> +{\n> +\tStreamConfiguration config;\n> +\n> +\tconfig = cfg;\n> +\tif (validateMainPath(&config) != Valid)\n\nShouldn't we accept Adjusted too if we want to check if the stream\n'fits' the path ?\n\n> +\t\treturn false;\n> +\n> +\tconfig = cfg;\n> +\tif (validateSelfPath(&config) != Valid)\n> +\t\treturn false;\n> +\n> +\treturn true;\n> +}\n> +\n> CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> {\n> \tconst CameraSensor *sensor = data_->sensor_;\n> @@ -501,22 +587,87 @@ 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 the streams. The first stream has the highest\n> +\t * priority but if both main path and self path can satisfy it evaluate\n> +\t * second stream first as the first stream is guaranteed to work with\n> +\t * whichever path is not used by the second one.\n> +\t */\n> +\tstd::vector order(config_.size());\n> +\tstd::iota(order.begin(), order.end(), 0);\n> +\tif (config_.size() == 2 && fitsAllPaths(config_[0]))\n> +\t\tstd::reverse(order.begin(), order.end());\n\nLet alone the implementation which is nice, is really the order of the\nconfigurations that defines to which output they should be assigned ?\nI'm not familiar with the platform, but in example, I would expect the\nlarger to go to the main path (as at this time they support the same\nformats)\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(const_cast(&data_->mainPathStream_));\n> +\t\t\t\tLOG(RkISP1, Debug) << \"Exact 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) == Valid) {\n> +\t\t\t\tselfPathAvailable = false;\n> +\t\t\t\tcfg = tryCfg;\n\nDo you need to re-assign it if it's valid ?\n\n> +\t\t\t\tcfg.setStream(const_cast(&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(const_cast(&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(const_cast(&data_->selfPathStream_));\n> +\t\t\t\tstatus = Adjusted;\n> +\t\t\t\tLOG(RkISP1, Debug) << \"Adjust match self\";\n\nIf I were to read this without reading the code, I would be puzzled.\nsame for other messages :)\n\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\t\t}\n\nI dug out the comment from the review of the first version where I\nasked \"why\" and the answer was that it's done not to adjust a stream\nwhere it is not needed, so you initially only wants \"valid\" then try\naccept \"adjusted\" if \"valid\" cannot be obtained.\n\nSo, what are the resons for \"adjusting\" ? Or either the pixel format\nis wrong, and you would need to adjust on both nodes, or if the sizes\nare larger the ones supported by the selfpath, in that case you\nfallback to the main path and even if in that case they're too large,\nadjust the stream. I think you could work this out easily if you sort\nstreams by size, but as I didn't get where \"first as higher priority\"\nconstraints come from, I might be mistaken.\n\nIf size sorted you try with main path first, if it's not taken and you\naccept Valid || Adjusted. The second stream goes to the selfpath, and\nyou accept Valid || Adjusted there too without duplicating the check.\n\nBut if the priority order is justified or even part of the libcamera\nAPI documentation, I think what you have here is fine.\n\nAlso, I don't know how RAW will work with this pipeline, but will this\ndouble pass scale well in that case or there shouldn't be particular\nissues ?\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> @@ -529,47 +680,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> 2.28.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 D3DF7C3B5B\n\tfor ;\n\tFri, 25 Sep 2020 14:58:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5DFD163049;\n\tFri, 25 Sep 2020 16:58:56 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 273C963041\n\tfor ;\n\tFri, 25 Sep 2020 16:58:55 +0200 (CEST)","from uno.localdomain (host-87-18-63-10.retail.telecomitalia.it\n\t[87.18.63.10]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id B5ECB40010;\n\tFri, 25 Sep 2020 14:58:53 +0000 (UTC)"],"X-Originating-IP":"87.18.63.10","Date":"Fri, 25 Sep 2020 17:02:45 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200925150245.fw2vfp4ddosm5pxp@uno.localdomain>","References":"<20200925014207.1455796-1-niklas.soderlund@ragnatech.se>\n\t<20200925014207.1455796-14-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200925014207.1455796-14-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v3 13/22] 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":12784,"web_url":"https://patchwork.libcamera.org/comment/12784/","msgid":"<20200925164825.GF1757254@oden.dyn.berto.se>","date":"2020-09-25T16:48:25","subject":"Re: [libcamera-devel] [PATCH v3 13/22] 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 Jacopo,\n\nThanks for your feedback.\n\nOn 2020-09-25 17:02:45 +0200, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Fri, Sep 25, 2020 at 03:41:58AM +0200, Niklas Söderlund wrote:\n> > Extend the format validation to work with both main and self paths. The\n> > heuristics honors that the first stream in the configuration has the\n> > highest priority while still examining both streams for a best match.\n> \n> I've heard you mentioning the priority of streams being defined by\n> their order multiple time, but this seems to be specific to this\n> pipeline handler, or have I missed it ?\n\nWell the first role in the array given to generateConfiguration() have \nhigher priority then the second. So this laves that the first element in \nthe returned configuration shall have higher priority then the second \nright?\n\n> \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 v2\n> > - Fix spelling in commit message.\n> > - Use Span<> instead of turning arrays to vectors.\n> > - Keep data_ const and cast 'const Streams*' to non-const using\n> > const_cast() to match the IPU3 pipeline.\n> > - Rename fitAnyPath() to fitsAllPaths().\n> > - Expand documentation for why second stream is evaluated first if the\n> > fist stream can use either stream.\n> > - Drop support for RGB888 and RGB656 for selfpath which was present in\n> > v2 as the driver delivers odd data when the frames are observed.\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> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 210 +++++++++++++++++------\n> > 1 file changed, 162 insertions(+), 48 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index cd3049485746edd6..bd53183a034efaff 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> > @@ -19,6 +20,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> >\n> > #include \"libcamera/internal/camera_sensor.h\"\n> > @@ -50,6 +52,19 @@ constexpr std::array RKISP1_RSZ_MP_FORMATS{\n> > \tformats::NV12,\n> > \t/* \\todo Add support for 8-bit greyscale to DRM formats */\n> > };\n> > +\n> > +constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 };\n> > +constexpr Size RKISP1_RSZ_SP_SRC_MAX{ 1920, 1920 };\n> \n> 1920x1920 ? Maybe it's just the way it actually is\n\nAt least in the kernel sources.\n\n> \n> > +constexpr std::array RKISP1_RSZ_SP_FORMATS{\n> > +\tformats::YUYV,\n> > +\tformats::YVYU,\n> > +\tformats::VYUY,\n> > +\tformats::NV16,\n> > +\tformats::NV61,\n> > +\tformats::NV21,\n> > +\tformats::NV12,\n> > +\t/* \\todo Add support for BGR888 and RGB565 */\n> > +};\n> \n> Aren't these equal to the main path ones ?\n\nYes and no :-) The enabled ones are, but the todos are not.\n\n> \n> > } /* namespace */\n> >\n> > class PipelineHandlerRkISP1;\n> > @@ -181,6 +196,14 @@ public:\n> > private:\n> > \tstatic constexpr unsigned int RKISP1_BUFFER_COUNT = 4;\n> >\n> > +\tCameraConfiguration::Status validatePath(StreamConfiguration *cfg,\n> > +\t\t\t\t\t\t const Span &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> > +\tbool fitsAllPaths(const 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> > @@ -492,6 +515,69 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n> > \tdata_ = data;\n> > }\n> >\n> > +CameraConfiguration::Status RkISP1CameraConfiguration::validatePath(\n> > +\tStreamConfiguration *cfg, const Span &formats,\n> > +\tconst Size &min, const Size &max, V4L2VideoDevice *video)\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> \n> This could be a one liner, but it's a matter of tastes\n\nAs in prev version I like this much better then to group them in on \nline. But as you say it's matter of taste.\n\n> \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> \n> Should you check for bufferCount too ?\n\nI don't think so, similar to that we don't check stride and frameSize \nright?\n\n> \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> > +bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n> > +{\n> > +\tStreamConfiguration config;\n> > +\n> > +\tconfig = cfg;\n> > +\tif (validateMainPath(&config) != Valid)\n> \n> Shouldn't we accept Adjusted too if we want to check if the stream\n> 'fits' the path ?\n\nNo.\n\nWe wish to check for exact match of the requested format. What we wish \nto learn is if the requested configuration can be satisfied on both \npaths without being adjusted.\n\n> \n> > +\t\treturn false;\n> > +\n> > +\tconfig = cfg;\n> > +\tif (validateSelfPath(&config) != Valid)\n> > +\t\treturn false;\n> > +\n> > +\treturn true;\n> > +}\n> > +\n> > CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > {\n> > \tconst CameraSensor *sensor = data_->sensor_;\n> > @@ -501,22 +587,87 @@ 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 the streams. The first stream has the highest\n> > +\t * priority but if both main path and self path can satisfy it evaluate\n> > +\t * second stream first as the first stream is guaranteed to work with\n> > +\t * whichever path is not used by the second one.\n> > +\t */\n> > +\tstd::vector order(config_.size());\n> > +\tstd::iota(order.begin(), order.end(), 0);\n> > +\tif (config_.size() == 2 && fitsAllPaths(config_[0]))\n> > +\t\tstd::reverse(order.begin(), order.end());\n> \n> Let alone the implementation which is nice, is really the order of the\n> configurations that defines to which output they should be assigned ?\n> I'm not familiar with the platform, but in example, I would expect the\n> larger to go to the main path (as at this time they support the same\n> formats)\n\nNo this logic tries to maximize the possibility for both streams to be \nfully satisfied while still respecting that the first configuration have \nhigher priority if not both can be satisfied.\n\nThe for-loop bellows assign hardware in a first come first serve fashion \nand once a path have been assigned the assignment is never re-evaluated. \nIt first tries to see if the configuration can be an exact match on any \nof the paths and then if it can be adjusted to fit (that is why two \npasses are needed).\n\nAbove we check with fitsAllPaths(config_[0]) if the first configuration \n(with has the highest priority) can fit on any path without being \nadjusted. If so then we know it's safe to swap the order and of the \nconfigurations as to maximize config_[1] to find an exact match on one \nof the pats. We can do this as we already know config_[0] will have en \nexact match on either of them.\n\nIf config_[0] can not find an exact match on both paths we do our best \nto make it fit first and config_[1] are stick with what ever resources \nare left.\n\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(const_cast(&data_->mainPathStream_));\n> > +\t\t\t\tLOG(RkISP1, Debug) << \"Exact 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) == Valid) {\n> > +\t\t\t\tselfPathAvailable = false;\n> > +\t\t\t\tcfg = tryCfg;\n> \n> Do you need to re-assign it if it's valid ?\n\nGood point, no it's not needed.\n\n> \n> > +\t\t\t\tcfg.setStream(const_cast(&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(const_cast(&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(const_cast(&data_->selfPathStream_));\n> > +\t\t\t\tstatus = Adjusted;\n> > +\t\t\t\tLOG(RkISP1, Debug) << \"Adjust match self\";\n> \n> If I were to read this without reading the code, I would be puzzled.\n> same for other messages :)\n\nI will drop them as they seem to confuse more then help.\n\n> \n> > +\t\t\t\tcontinue;\n> > +\t\t\t}\n> > +\t\t}\n> \n> I dug out the comment from the review of the first version where I\n> asked \"why\" and the answer was that it's done not to adjust a stream\n> where it is not needed, so you initially only wants \"valid\" then try\n> accept \"adjusted\" if \"valid\" cannot be obtained.\n> \n> So, what are the resons for \"adjusting\" ? Or either the pixel format\n> is wrong, and you would need to adjust on both nodes, or if the sizes\n> are larger the ones supported by the selfpath, in that case you\n> fallback to the main path and even if in that case they're too large,\n> adjust the stream. I think you could work this out easily if you sort\n> streams by size, but as I didn't get where \"first as higher priority\"\n> constraints come from, I might be mistaken.\n> \n> If size sorted you try with main path first, if it's not taken and you\n> accept Valid || Adjusted. The second stream goes to the selfpath, and\n> you accept Valid || Adjusted there too without duplicating the check.\n> \n> But if the priority order is justified or even part of the libcamera\n> API documentation, I think what you have here is fine.\n\nIf it was only sizes that differed between the two yes, but it's not. \nThe main path can support RAW formats while the self path can support \nRGB. So sorting by size is unfortunately not possible as we also need to \nconsider the formats.\n\n> \n> Also, I don't know how RAW will work with this pipeline, but will this\n> double pass scale well in that case or there shouldn't be particular\n> issues ?\n\nRAW will be a special case on this pipeline as it will limit it to one \nstream. When capturing RAW only the main path can be active.\n\nBut same question for RGB formats which are only supported by the self \npath. No this is tested with that in mind as adding BGR888 to the list \nof supported formats in RKISP1_RSZ_SP_FORMATS is all that is needed to \nenable it to be captured. But the frames looks a bit wierd in qcam so I \nwill investigate this on top of this series but I do not wish to block \nthis already too large series on also enabeling support for BGR888 and \nRGB656.\n\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> > @@ -529,47 +680,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> > 2.28.0\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","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 A010AC3B5C\n\tfor ;\n\tFri, 25 Sep 2020 16:48:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2CCDE6303C;\n\tFri, 25 Sep 2020 18:48:30 +0200 (CEST)","from mail-lf1-x12a.google.com (mail-lf1-x12a.google.com\n\t[IPv6:2a00:1450:4864:20::12a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E635D62FD8\n\tfor ;\n\tFri, 25 Sep 2020 18:48:27 +0200 (CEST)","by mail-lf1-x12a.google.com with SMTP id 77so3565536lfj.0\n\tfor ;\n\tFri, 25 Sep 2020 09:48:27 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tc11sm2705550lff.3.2020.09.25.09.48.26\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 25 Sep 2020 09:48:26 -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=\"fUsRGzuh\"; 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=ON98iMrz75rcxDIVzVvKOYKW0USU+ve4QWjBjyj/CKM=;\n\tb=fUsRGzuhEalAk9Cd2+WzfoIhQ+8Aic8WJx/VKvdnZDtk3i97rA/59/uol8JKu1zuOw\n\tm9r/F9oa2PtvCnHXQz8PMbI3a59idhrIXXxT0rsEszooVEcKL2SPQZTQa2gFLbqWyUzU\n\tTXC/rM600ZsIgpQjhn+mNP0JUP/mGEApu1W6oQGUrFrtszQ9d9iz273gMsMSjuewvF67\n\tc2o5Te6CPUsCogvK+w03v8VQHPFmbtrx2jocDX3bBZ7HwT5txUTTQ3Eg6Ci9Xl3GhXmW\n\t7rhY8NKUEcqCvtQ7vSQHgxTxsugUEknC8kj36828oMGbj5Pke+NIUUkqXkuaQhUzPHE8\n\tanQg==","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=ON98iMrz75rcxDIVzVvKOYKW0USU+ve4QWjBjyj/CKM=;\n\tb=SQ962Gm4y/jFn9iw8Aq8VTHHzMgw8dPkXHxtLf8AQk1fPgZHuSpC8DvijJm13YamVZ\n\tqJ23gJP0Q5aU4p6Q7YPhZaCikEfBiCPRf/ocVjA81oxS96bXHl+/T5yN2ZxsUO2/23AH\n\tt0AV1NeINoqWJzpYRodFxvRsvmV3eDajwtNp969rn6Q4tS4WCLH+VjhNiRDecsv3ocD0\n\trrZ0nrYu3sYXu3zLbdqWzgJd1gH9/L9EvuYcTrKny0KnCvyqHtwVQSVIGm5UefN/zH4I\n\t9Tqv8SjBCABbm0w7HJ+RFxwKoIagnV15vgZaEuim3XK+BIij9spzgCDOKm08+VLEEFNc\n\t/ZOw==","X-Gm-Message-State":"AOAM531PclulPYPpOjRW/E61boVlS6c+TTQ5W5p3wsG8/CN4p3CUlLL6\n\tXg8RLsiZwpqNyyHaIjHZ2NHhHx8RB9bZ3A==","X-Google-Smtp-Source":"ABdhPJyCrop0JBFKIYH9pVbrU0tlzVBXIPuKLYMyfpGSgR6BBp9mo0Fc58OCOaa8HOCW9KFgatPgMw==","X-Received":"by 2002:a19:6147:: with SMTP id m7mr1844881lfk.108.1601052507067;\n\tFri, 25 Sep 2020 09:48:27 -0700 (PDT)","Date":"Fri, 25 Sep 2020 18:48:25 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"<20200925164825.GF1757254@oden.dyn.berto.se>","References":"<20200925014207.1455796-1-niklas.soderlund@ragnatech.se>\n\t<20200925014207.1455796-14-niklas.soderlund@ragnatech.se>\n\t<20200925150245.fw2vfp4ddosm5pxp@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200925150245.fw2vfp4ddosm5pxp@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 13/22] 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\" "}},{"id":12795,"web_url":"https://patchwork.libcamera.org/comment/12795/","msgid":"<20200928083449.twwtosv6x2wcoley@uno.localdomain>","date":"2020-09-28T08:34:49","subject":"Re: [libcamera-devel] [PATCH v3 13/22] libcamera: pipeline: rkisp1:\n\tAdd format validation for self path","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Fri, Sep 25, 2020 at 06:48:25PM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your feedback.\n>\n> On 2020-09-25 17:02:45 +0200, Jacopo Mondi wrote:\n> > Hi Niklas,\n> >\n> > On Fri, Sep 25, 2020 at 03:41:58AM +0200, Niklas Söderlund wrote:\n> > > Extend the format validation to work with both main and self paths. The\n> > > heuristics honors that the first stream in the configuration has the\n> > > highest priority while still examining both streams for a best match.\n> >\n> > I've heard you mentioning the priority of streams being defined by\n> > their order multiple time, but this seems to be specific to this\n> > pipeline handler, or have I missed it ?\n>\n> Well the first role in the array given to generateConfiguration() have\n> higher priority then the second. So this laves that the first element in\n> the returned configuration shall have higher priority then the second\n> right?\n>\n\nMy point, in this patch and other, is that I don't see where the\npriority order constraint comes from. I don't see it in the\nCamera::generateConfiguration() or Camera::configure() documentation\nand I'm wondering if that's a pipeline-specific behaviour (which, in\ngeneral, we should try to introduce, imho).\n\nThanks\n j\n\n> >\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 v2\n> > > - Fix spelling in commit message.\n> > > - Use Span<> instead of turning arrays to vectors.\n> > > - Keep data_ const and cast 'const Streams*' to non-const using\n> > > const_cast() to match the IPU3 pipeline.\n> > > - Rename fitAnyPath() to fitsAllPaths().\n> > > - Expand documentation for why second stream is evaluated first if the\n> > > fist stream can use either stream.\n> > > - Drop support for RGB888 and RGB656 for selfpath which was present in\n> > > v2 as the driver delivers odd data when the frames are observed.\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> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 210 +++++++++++++++++------\n> > > 1 file changed, 162 insertions(+), 48 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index cd3049485746edd6..bd53183a034efaff 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> > > @@ -19,6 +20,7 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > >\n> > > #include \"libcamera/internal/camera_sensor.h\"\n> > > @@ -50,6 +52,19 @@ constexpr std::array RKISP1_RSZ_MP_FORMATS{\n> > > \tformats::NV12,\n> > > \t/* \\todo Add support for 8-bit greyscale to DRM formats */\n> > > };\n> > > +\n> > > +constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 };\n> > > +constexpr Size RKISP1_RSZ_SP_SRC_MAX{ 1920, 1920 };\n> >\n> > 1920x1920 ? Maybe it's just the way it actually is\n>\n> At least in the kernel sources.\n>\n> >\n> > > +constexpr std::array RKISP1_RSZ_SP_FORMATS{\n> > > +\tformats::YUYV,\n> > > +\tformats::YVYU,\n> > > +\tformats::VYUY,\n> > > +\tformats::NV16,\n> > > +\tformats::NV61,\n> > > +\tformats::NV21,\n> > > +\tformats::NV12,\n> > > +\t/* \\todo Add support for BGR888 and RGB565 */\n> > > +};\n> >\n> > Aren't these equal to the main path ones ?\n>\n> Yes and no :-) The enabled ones are, but the todos are not.\n>\n> >\n> > > } /* namespace */\n> > >\n> > > class PipelineHandlerRkISP1;\n> > > @@ -181,6 +196,14 @@ public:\n> > > private:\n> > > \tstatic constexpr unsigned int RKISP1_BUFFER_COUNT = 4;\n> > >\n> > > +\tCameraConfiguration::Status validatePath(StreamConfiguration *cfg,\n> > > +\t\t\t\t\t\t const Span &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> > > +\tbool fitsAllPaths(const 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> > > @@ -492,6 +515,69 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n> > > \tdata_ = data;\n> > > }\n> > >\n> > > +CameraConfiguration::Status RkISP1CameraConfiguration::validatePath(\n> > > +\tStreamConfiguration *cfg, const Span &formats,\n> > > +\tconst Size &min, const Size &max, V4L2VideoDevice *video)\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> >\n> > This could be a one liner, but it's a matter of tastes\n>\n> As in prev version I like this much better then to group them in on\n> line. But as you say it's matter of taste.\n>\n> >\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> >\n> > Should you check for bufferCount too ?\n>\n> I don't think so, similar to that we don't check stride and frameSize\n> right?\n>\n\nnot sure you know... stride and frameSize are output information,\nbufferCount can actually be set by the application, doesn't it ?\n\n> >\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> > > +bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n> > > +{\n> > > +\tStreamConfiguration config;\n> > > +\n> > > +\tconfig = cfg;\n> > > +\tif (validateMainPath(&config) != Valid)\n> >\n> > Shouldn't we accept Adjusted too if we want to check if the stream\n> > 'fits' the path ?\n>\n> No.\n>\n> We wish to check for exact match of the requested format. What we wish\n> to learn is if the requested configuration can be satisfied on both\n> paths without being adjusted.\n>\n> >\n> > > +\t\treturn false;\n> > > +\n> > > +\tconfig = cfg;\n> > > +\tif (validateSelfPath(&config) != Valid)\n> > > +\t\treturn false;\n> > > +\n> > > +\treturn true;\n> > > +}\n> > > +\n> > > CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > > {\n> > > \tconst CameraSensor *sensor = data_->sensor_;\n> > > @@ -501,22 +587,87 @@ 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 the streams. The first stream has the highest\n> > > +\t * priority but if both main path and self path can satisfy it evaluate\n> > > +\t * second stream first as the first stream is guaranteed to work with\n> > > +\t * whichever path is not used by the second one.\n> > > +\t */\n> > > +\tstd::vector order(config_.size());\n> > > +\tstd::iota(order.begin(), order.end(), 0);\n> > > +\tif (config_.size() == 2 && fitsAllPaths(config_[0]))\n> > > +\t\tstd::reverse(order.begin(), order.end());\n> >\n> > Let alone the implementation which is nice, is really the order of the\n> > configurations that defines to which output they should be assigned ?\n\nAsking the same question again, I might have missed where the priority\nassignment requirement comes from.\n\n\n> > I'm not familiar with the platform, but in example, I would expect the\n> > larger to go to the main path (as at this time they support the same\n> > formats)\n>\n> No this logic tries to maximize the possibility for both streams to be\n> fully satisfied while still respecting that the first configuration have\n> higher priority if not both can be satisfied.\n>\n> The for-loop bellows assign hardware in a first come first serve fashion\n> and once a path have been assigned the assignment is never re-evaluated.\n> It first tries to see if the configuration can be an exact match on any\n> of the paths and then if it can be adjusted to fit (that is why two\n> passes are needed).\n>\n> Above we check with fitsAllPaths(config_[0]) if the first configuration\n> (with has the highest priority) can fit on any path without being\n> adjusted. If so then we know it's safe to swap the order and of the\n> configurations as to maximize config_[1] to find an exact match on one\n> of the pats. We can do this as we already know config_[0] will have en\n> exact match on either of them.\n>\n> If config_[0] can not find an exact match on both paths we do our best\n> to make it fit first and config_[1] are stick with what ever resources\n> are left.\n>\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(const_cast(&data_->mainPathStream_));\n> > > +\t\t\t\tLOG(RkISP1, Debug) << \"Exact 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) == Valid) {\n> > > +\t\t\t\tselfPathAvailable = false;\n> > > +\t\t\t\tcfg = tryCfg;\n> >\n> > Do you need to re-assign it if it's valid ?\n>\n> Good point, no it's not needed.\n>\n\nWait, maybe bufferCount or stride has changed, as their modification\ndoesn't 'Adjust' the stream status.\n\n> >\n> > > +\t\t\t\tcfg.setStream(const_cast(&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(const_cast(&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(const_cast(&data_->selfPathStream_));\n> > > +\t\t\t\tstatus = Adjusted;\n> > > +\t\t\t\tLOG(RkISP1, Debug) << \"Adjust match self\";\n> >\n> > If I were to read this without reading the code, I would be puzzled.\n> > same for other messages :)\n>\n> I will drop them as they seem to confuse more then help.\n>\n\nThanks, I think it could help if expanded. As a reference on IPU3:\n\n LOG(IPU3, Debug) << \"Assigned \" << cfg->toString()\n << \" to the main output\";\n\n> >\n> > > +\t\t\t\tcontinue;\n> > > +\t\t\t}\n> > > +\t\t}\n> >\n> > I dug out the comment from the review of the first version where I\n> > asked \"why\" and the answer was that it's done not to adjust a stream\n> > where it is not needed, so you initially only wants \"valid\" then try\n> > accept \"adjusted\" if \"valid\" cannot be obtained.\n> >\n> > So, what are the resons for \"adjusting\" ? Or either the pixel format\n> > is wrong, and you would need to adjust on both nodes, or if the sizes\n> > are larger the ones supported by the selfpath, in that case you\n> > fallback to the main path and even if in that case they're too large,\n> > adjust the stream. I think you could work this out easily if you sort\n> > streams by size, but as I didn't get where \"first as higher priority\"\n> > constraints come from, I might be mistaken.\n> >\n> > If size sorted you try with main path first, if it's not taken and you\n> > accept Valid || Adjusted. The second stream goes to the selfpath, and\n> > you accept Valid || Adjusted there too without duplicating the check.\n> >\n> > But if the priority order is justified or even part of the libcamera\n> > API documentation, I think what you have here is fine.\n>\n> If it was only sizes that differed between the two yes, but it's not.\n> The main path can support RAW formats while the self path can support\n> RGB. So sorting by size is unfortunately not possible as we also need to\n> consider the formats.\n\nBut in that case assigning by format is even easier. RAW goes to main\nand RGB goes to self.\n\n>\n> >\n> > Also, I don't know how RAW will work with this pipeline, but will this\n> > double pass scale well in that case or there shouldn't be particular\n> > issues ?\n>\n> RAW will be a special case on this pipeline as it will limit it to one\n> stream. When capturing RAW only the main path can be active.\n>\n> But same question for RGB formats which are only supported by the self\n> path. No this is tested with that in mind as adding BGR888 to the list\n> of supported formats in RKISP1_RSZ_SP_FORMATS is all that is needed to\n> enable it to be captured. But the frames looks a bit wierd in qcam so I\n> will investigate this on top of this series but I do not wish to block\n> this already too large series on also enabeling support for BGR888 and\n> RGB656.\n\nAgreed on non blocking to wait for RGB.\n\n>\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> > > @@ -529,47 +680,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> > > 2.28.0\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund","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 4C12DC3B5B\n\tfor ;\n\tMon, 28 Sep 2020 08:30:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B7CB460CE7;\n\tMon, 28 Sep 2020 10:30:56 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B505F6035F\n\tfor ;\n\tMon, 28 Sep 2020 10:30:54 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 2C769FF811;\n\tMon, 28 Sep 2020 08:30:53 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Mon, 28 Sep 2020 10:34:49 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200928083449.twwtosv6x2wcoley@uno.localdomain>","References":"<20200925014207.1455796-1-niklas.soderlund@ragnatech.se>\n\t<20200925014207.1455796-14-niklas.soderlund@ragnatech.se>\n\t<20200925150245.fw2vfp4ddosm5pxp@uno.localdomain>\n\t<20200925164825.GF1757254@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200925164825.GF1757254@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH v3 13/22] 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":12823,"web_url":"https://patchwork.libcamera.org/comment/12823/","msgid":"<20200928202313.GO23539@pendragon.ideasonboard.com>","date":"2020-09-28T20:23:13","subject":"Re: [libcamera-devel] [PATCH v3 13/22] 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 Jacopo,\n\nOn Mon, Sep 28, 2020 at 10:34:49AM +0200, Jacopo Mondi wrote:\n> On Fri, Sep 25, 2020 at 06:48:25PM +0200, Niklas Söderlund wrote:\n> > On 2020-09-25 17:02:45 +0200, Jacopo Mondi wrote:\n> > > On Fri, Sep 25, 2020 at 03:41:58AM +0200, Niklas Söderlund wrote:\n> > > > Extend the format validation to work with both main and self paths. The\n> > > > heuristics honors that the first stream in the configuration has the\n> > > > highest priority while still examining both streams for a best match.\n> > >\n> > > I've heard you mentioning the priority of streams being defined by\n> > > their order multiple time, but this seems to be specific to this\n> > > pipeline handler, or have I missed it ?\n> >\n> > Well the first role in the array given to generateConfiguration() have\n> > higher priority then the second. So this laves that the first element in\n> > the returned configuration shall have higher priority then the second\n> > right?\n> \n> My point, in this patch and other, is that I don't see where the\n> priority order constraint comes from. I don't see it in the\n> Camera::generateConfiguration() or Camera::configure() documentation\n> and I'm wondering if that's a pipeline-specific behaviour (which, in\n> general, we should try to introduce, imho).\n\nI would have sworn this was documented, but it seems not to be the case.\nI consider this as a standard behaviour, not API specific. I'll make\nsure to document priorities in the configuration API rework.\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 v2\n> > > > - Fix spelling in commit message.\n> > > > - Use Span<> instead of turning arrays to vectors.\n> > > > - Keep data_ const and cast 'const Streams*' to non-const using\n> > > > const_cast() to match the IPU3 pipeline.\n> > > > - Rename fitAnyPath() to fitsAllPaths().\n> > > > - Expand documentation for why second stream is evaluated first if the\n> > > > fist stream can use either stream.\n> > > > - Drop support for RGB888 and RGB656 for selfpath which was present in\n> > > > v2 as the driver delivers odd data when the frames are observed.\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> > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 210 +++++++++++++++++------\n> > > > 1 file changed, 162 insertions(+), 48 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > index cd3049485746edd6..bd53183a034efaff 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> > > > @@ -19,6 +20,7 @@\n> > > > #include \n> > > > #include \n> > > > #include \n> > > > +#include \n> > > > #include \n> > > >\n> > > > #include \"libcamera/internal/camera_sensor.h\"\n> > > > @@ -50,6 +52,19 @@ constexpr std::array RKISP1_RSZ_MP_FORMATS{\n> > > > \tformats::NV12,\n> > > > \t/* \\todo Add support for 8-bit greyscale to DRM formats */\n> > > > };\n> > > > +\n> > > > +constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 };\n> > > > +constexpr Size RKISP1_RSZ_SP_SRC_MAX{ 1920, 1920 };\n> > >\n> > > 1920x1920 ? Maybe it's just the way it actually is\n> >\n> > At least in the kernel sources.\n> >\n> > > > +constexpr std::array RKISP1_RSZ_SP_FORMATS{\n> > > > +\tformats::YUYV,\n> > > > +\tformats::YVYU,\n> > > > +\tformats::VYUY,\n> > > > +\tformats::NV16,\n> > > > +\tformats::NV61,\n> > > > +\tformats::NV21,\n> > > > +\tformats::NV12,\n> > > > +\t/* \\todo Add support for BGR888 and RGB565 */\n> > > > +};\n> > >\n> > > Aren't these equal to the main path ones ?\n> >\n> > Yes and no :-) The enabled ones are, but the todos are not.\n> >\n> > > > } /* namespace */\n> > > >\n> > > > class PipelineHandlerRkISP1;\n> > > > @@ -181,6 +196,14 @@ public:\n> > > > private:\n> > > > \tstatic constexpr unsigned int RKISP1_BUFFER_COUNT = 4;\n> > > >\n> > > > +\tCameraConfiguration::Status validatePath(StreamConfiguration *cfg,\n> > > > +\t\t\t\t\t\t const Span &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> > > > +\tbool fitsAllPaths(const 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> > > > @@ -492,6 +515,69 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n> > > > \tdata_ = data;\n> > > > }\n> > > >\n> > > > +CameraConfiguration::Status RkISP1CameraConfiguration::validatePath(\n> > > > +\tStreamConfiguration *cfg, const Span &formats,\n> > > > +\tconst Size &min, const Size &max, V4L2VideoDevice *video)\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> > >\n> > > This could be a one liner, but it's a matter of tastes\n> >\n> > As in prev version I like this much better then to group them in on\n> > line. But as you say it's matter of taste.\n> >\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> > >\n> > > Should you check for bufferCount too ?\n> >\n> > I don't think so, similar to that we don't check stride and frameSize\n> > right?\n> \n> not sure you know... stride and frameSize are output information,\n> bufferCount can actually be set by the application, doesn't it ?\n> \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> > > > +bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n> > > > +{\n> > > > +\tStreamConfiguration config;\n> > > > +\n> > > > +\tconfig = cfg;\n> > > > +\tif (validateMainPath(&config) != Valid)\n> > >\n> > > Shouldn't we accept Adjusted too if we want to check if the stream\n> > > 'fits' the path ?\n> >\n> > No.\n> >\n> > We wish to check for exact match of the requested format. What we wish\n> > to learn is if the requested configuration can be satisfied on both\n> > paths without being adjusted.\n> >\n> > > > +\t\treturn false;\n> > > > +\n> > > > +\tconfig = cfg;\n> > > > +\tif (validateSelfPath(&config) != Valid)\n> > > > +\t\treturn false;\n> > > > +\n> > > > +\treturn true;\n> > > > +}\n> > > > +\n> > > > CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > > > {\n> > > > \tconst CameraSensor *sensor = data_->sensor_;\n> > > > @@ -501,22 +587,87 @@ 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 the streams. The first stream has the highest\n> > > > +\t * priority but if both main path and self path can satisfy it evaluate\n> > > > +\t * second stream first as the first stream is guaranteed to work with\n> > > > +\t * whichever path is not used by the second one.\n> > > > +\t */\n> > > > +\tstd::vector order(config_.size());\n> > > > +\tstd::iota(order.begin(), order.end(), 0);\n> > > > +\tif (config_.size() == 2 && fitsAllPaths(config_[0]))\n> > > > +\t\tstd::reverse(order.begin(), order.end());\n> > >\n> > > Let alone the implementation which is nice, is really the order of the\n> > > configurations that defines to which output they should be assigned ?\n> \n> Asking the same question again, I might have missed where the priority\n> assignment requirement comes from.\n> \n> > > I'm not familiar with the platform, but in example, I would expect the\n> > > larger to go to the main path (as at this time they support the same\n> > > formats)\n> >\n> > No this logic tries to maximize the possibility for both streams to be\n> > fully satisfied while still respecting that the first configuration have\n> > higher priority if not both can be satisfied.\n> >\n> > The for-loop bellows assign hardware in a first come first serve fashion\n> > and once a path have been assigned the assignment is never re-evaluated.\n> > It first tries to see if the configuration can be an exact match on any\n> > of the paths and then if it can be adjusted to fit (that is why two\n> > passes are needed).\n> >\n> > Above we check with fitsAllPaths(config_[0]) if the first configuration\n> > (with has the highest priority) can fit on any path without being\n> > adjusted. If so then we know it's safe to swap the order and of the\n> > configurations as to maximize config_[1] to find an exact match on one\n> > of the pats. We can do this as we already know config_[0] will have en\n> > exact match on either of them.\n> >\n> > If config_[0] can not find an exact match on both paths we do our best\n> > to make it fit first and config_[1] are stick with what ever resources\n> > are left.\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(const_cast(&data_->mainPathStream_));\n> > > > +\t\t\t\tLOG(RkISP1, Debug) << \"Exact 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) == Valid) {\n> > > > +\t\t\t\tselfPathAvailable = false;\n> > > > +\t\t\t\tcfg = tryCfg;\n> > >\n> > > Do you need to re-assign it if it's valid ?\n> >\n> > Good point, no it's not needed.\n> \n> Wait, maybe bufferCount or stride has changed, as their modification\n> doesn't 'Adjust' the stream status.\n> \n> > > > +\t\t\t\tcfg.setStream(const_cast(&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(const_cast(&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(const_cast(&data_->selfPathStream_));\n> > > > +\t\t\t\tstatus = Adjusted;\n> > > > +\t\t\t\tLOG(RkISP1, Debug) << \"Adjust match self\";\n> > >\n> > > If I were to read this without reading the code, I would be puzzled.\n> > > same for other messages :)\n> >\n> > I will drop them as they seem to confuse more then help.\n> \n> Thanks, I think it could help if expanded. As a reference on IPU3:\n> \n> LOG(IPU3, Debug) << \"Assigned \" << cfg->toString()\n> << \" to the main output\";\n> \n> > > > +\t\t\t\tcontinue;\n> > > > +\t\t\t}\n> > > > +\t\t}\n> > >\n> > > I dug out the comment from the review of the first version where I\n> > > asked \"why\" and the answer was that it's done not to adjust a stream\n> > > where it is not needed, so you initially only wants \"valid\" then try\n> > > accept \"adjusted\" if \"valid\" cannot be obtained.\n> > >\n> > > So, what are the resons for \"adjusting\" ? Or either the pixel format\n> > > is wrong, and you would need to adjust on both nodes, or if the sizes\n> > > are larger the ones supported by the selfpath, in that case you\n> > > fallback to the main path and even if in that case they're too large,\n> > > adjust the stream. I think you could work this out easily if you sort\n> > > streams by size, but as I didn't get where \"first as higher priority\"\n> > > constraints come from, I might be mistaken.\n> > >\n> > > If size sorted you try with main path first, if it's not taken and you\n> > > accept Valid || Adjusted. The second stream goes to the selfpath, and\n> > > you accept Valid || Adjusted there too without duplicating the check.\n> > >\n> > > But if the priority order is justified or even part of the libcamera\n> > > API documentation, I think what you have here is fine.\n> >\n> > If it was only sizes that differed between the two yes, but it's not.\n> > The main path can support RAW formats while the self path can support\n> > RGB. So sorting by size is unfortunately not possible as we also need to\n> > consider the formats.\n> \n> But in that case assigning by format is even easier. RAW goes to main\n> and RGB goes to self.\n> \n> > > Also, I don't know how RAW will work with this pipeline, but will this\n> > > double pass scale well in that case or there shouldn't be particular\n> > > issues ?\n> >\n> > RAW will be a special case on this pipeline as it will limit it to one\n> > stream. When capturing RAW only the main path can be active.\n> >\n> > But same question for RGB formats which are only supported by the self\n> > path. No this is tested with that in mind as adding BGR888 to the list\n> > of supported formats in RKISP1_RSZ_SP_FORMATS is all that is needed to\n> > enable it to be captured. But the frames looks a bit wierd in qcam so I\n> > will investigate this on top of this series but I do not wish to block\n> > this already too large series on also enabeling support for BGR888 and\n> > RGB656.\n> \n> Agreed on non blocking to wait for RGB.\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> > > > @@ -529,47 +680,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 C4AF0C3B5B\n\tfor ;\n\tMon, 28 Sep 2020 20:23:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3AD4D60BF7;\n\tMon, 28 Sep 2020 22:23:50 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CB94A60366\n\tfor ;\n\tMon, 28 Sep 2020 22:23:48 +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 377D3A58;\n\tMon, 28 Sep 2020 22:23:48 +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=\"rUUvWtpi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601324628;\n\tbh=2N6wPITubA6z8odbzQjqefW/2OCx9IRasKVTPwdu0fI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rUUvWtpip3jStCJzX5Xl1HZhQNC1fiseKR7ohiAT7DWloyQsf6Tm6LCGIct2yiQNH\n\t2NiAivGVov6LqejEwiWzhCsPGJgC0BM5/VxHRlJr0AD5FJpo+4NbVpfxReNwrrj5oo\n\tRgMsKzJFl3+yYXlrwgpo/gOQVfllibzgHt9G9aNg=","Date":"Mon, 28 Sep 2020 23:23:13 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20200928202313.GO23539@pendragon.ideasonboard.com>","References":"<20200925014207.1455796-1-niklas.soderlund@ragnatech.se>\n\t<20200925014207.1455796-14-niklas.soderlund@ragnatech.se>\n\t<20200925150245.fw2vfp4ddosm5pxp@uno.localdomain>\n\t<20200925164825.GF1757254@oden.dyn.berto.se>\n\t<20200928083449.twwtosv6x2wcoley@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200928083449.twwtosv6x2wcoley@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 13/22] 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":12824,"web_url":"https://patchwork.libcamera.org/comment/12824/","msgid":"<20200928203350.GP23539@pendragon.ideasonboard.com>","date":"2020-09-28T20:33:50","subject":"Re: [libcamera-devel] [PATCH v3 13/22] 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 Fri, Sep 25, 2020 at 03:41:58AM +0200, Niklas Söderlund wrote:\n> Extend the format validation to work with both main and self paths. The\n> heuristics honors that the first stream in the configuration has the\n> highest priority while still examining both streams for a best match.\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 v2\n> - Fix spelling in commit message.\n> - Use Span<> instead of turning arrays to vectors.\n> - Keep data_ const and cast 'const Streams*' to non-const using\n> const_cast() to match the IPU3 pipeline.\n> - Rename fitAnyPath() to fitsAllPaths().\n> - Expand documentation for why second stream is evaluated first if the\n> fist stream can use either stream.\n> - Drop support for RGB888 and RGB656 for selfpath which was present in\n> v2 as the driver delivers odd data when the frames are observed.\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> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 210 +++++++++++++++++------\n> 1 file changed, 162 insertions(+), 48 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index cd3049485746edd6..bd53183a034efaff 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> @@ -19,6 +20,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> \n> #include \"libcamera/internal/camera_sensor.h\"\n> @@ -50,6 +52,19 @@ constexpr std::array RKISP1_RSZ_MP_FORMATS{\n> \tformats::NV12,\n> \t/* \\todo Add support for 8-bit greyscale to DRM formats */\n> };\n> +\n> +constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 };\n> +constexpr Size RKISP1_RSZ_SP_SRC_MAX{ 1920, 1920 };\n> +constexpr std::array RKISP1_RSZ_SP_FORMATS{\n> +\tformats::YUYV,\n> +\tformats::YVYU,\n> +\tformats::VYUY,\n> +\tformats::NV16,\n> +\tformats::NV61,\n> +\tformats::NV21,\n> +\tformats::NV12,\n> +\t/* \\todo Add support for BGR888 and RGB565 */\n> +};\n> } /* namespace */\n> \n> class PipelineHandlerRkISP1;\n> @@ -181,6 +196,14 @@ public:\n> private:\n> \tstatic constexpr unsigned int RKISP1_BUFFER_COUNT = 4;\n> \n> +\tCameraConfiguration::Status validatePath(StreamConfiguration *cfg,\n> +\t\t\t\t\t\t const Span &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> +\tbool fitsAllPaths(const 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> @@ -492,6 +515,69 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n> \tdata_ = data;\n> }\n> \n> +CameraConfiguration::Status RkISP1CameraConfiguration::validatePath(\n> +\tStreamConfiguration *cfg, const Span &formats,\n> +\tconst Size &min, const Size &max, V4L2VideoDevice *video)\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> +bool RkISP1CameraConfiguration::fitsAllPaths(const StreamConfiguration &cfg)\n> +{\n> +\tStreamConfiguration config;\n> +\n> +\tconfig = cfg;\n> +\tif (validateMainPath(&config) != Valid)\n> +\t\treturn false;\n> +\n> +\tconfig = cfg;\n> +\tif (validateSelfPath(&config) != Valid)\n> +\t\treturn false;\n> +\n> +\treturn true;\n> +}\n> +\n> CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> {\n> \tconst CameraSensor *sensor = data_->sensor_;\n> @@ -501,22 +587,87 @@ 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\ns/then/than/\n\n> +\t * order to evaluate the streams. The first stream has the highest\n> +\t * priority but if both main path and self path can satisfy it evaluate\n> +\t * second stream first as the first stream is guaranteed to work with\n\ns/second/the second/\n\n> +\t * whichever path is not used by the second one.\n> +\t */\n> +\tstd::vector order(config_.size());\n> +\tstd::iota(order.begin(), order.end(), 0);\n> +\tif (config_.size() == 2 && fitsAllPaths(config_[0]))\n> +\t\tstd::reverse(order.begin(), order.end());\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(const_cast(&data_->mainPathStream_));\n> +\t\t\t\tLOG(RkISP1, Debug) << \"Exact match main\";\n\nI'd either drop the message, or make it more explicit. Something like\nthis maybe.\n\n\t\t\t\tLOG(RkISP1, Debug)\n\t\t\t\t\t<< \"Stream \" << index\n\t\t\t\t\t<< \" configuration matches main path exactly\";\n\nSame below.\n\nReviewed-by: Laurent Pinchart \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(const_cast(&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(const_cast(&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(const_cast(&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> +\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> @@ -529,47 +680,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 50739C3B5C\n\tfor ;\n\tMon, 28 Sep 2020 20:34:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B81C96163B;\n\tMon, 28 Sep 2020 22:34:26 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D124760366\n\tfor ;\n\tMon, 28 Sep 2020 22:34:25 +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 2FE10A58;\n\tMon, 28 Sep 2020 22:34:25 +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=\"mh7QDPbe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601325265;\n\tbh=oX28h+V24RWfpW5z69BqQ4BuumEH/8Tja8B0PXF8VZM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mh7QDPbetylA8dtMJhO0ktqusQdv8eF4ahIhMNjCCR7Tt8hnSUUDsHtIgiSRktQwL\n\tVsXBjgcA/JO7VG2oEiFQNlTwpzNndwU654+p4UsCHFU1O+dDppN3pfMHJejmSjUhaG\n\t1g6Lt42q767LnFIedo80wtQ74EkMTsyFFnvOPJXA=","Date":"Mon, 28 Sep 2020 23:33:50 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200928203350.GP23539@pendragon.ideasonboard.com>","References":"<20200925014207.1455796-1-niklas.soderlund@ragnatech.se>\n\t<20200925014207.1455796-14-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200925014207.1455796-14-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v3 13/22] 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\" "}}]