[{"id":11052,"web_url":"https://patchwork.libcamera.org/comment/11052/","msgid":"<20200701164427.GH2399385@oden.dyn.berto.se>","date":"2020-07-01T16:44:27","subject":"Re: [libcamera-devel] [PATCH 06/15] libcamera: ipu3: Rework stream\n\tvalidation","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 work.\n\nOn 2020-07-01 14:30:27 +0200, Jacopo Mondi wrote:\n> With the goal of moving stream assignement to configuration from\n> validate() to configure(), rework the validate() function to not\n> inspect the stream assigned to a configuration to identify it.\n> \n> Re-work the validation logic to follow this simpler flow:\n> - if a stream is a raw stream use the sensor configuration\n> - if a stream is a processed one, make sure it respects the ImgU\n> output size and alignment constraints.\n> \n> Remove the 'adjustStream()' function as it depends on stream assignment\n> and was built on the assumption the main output should always work at\n> the maximum available resolution, and it addressed the case where no\n> width or height where supplied for a viewfinder stream, which should\n> only be validated against the ImgU output constraints, not defaulted to a\n> sane value.\n> \n> Retain the call to assignStreams() in validate() for the moment in order\n> not to break capture operations, but while at it clean up the code a bit\n> by removing a rogue empty line and make a conditional check fit on a\n> single line.\n> \n> Signed-off-by: Jacopo Mondi \n\nReviewed-by: Niklas Söderlund \n\n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 85 ++++++++++------------------\n> 1 file changed, 31 insertions(+), 54 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index ed2360347fb4..daa6d71dae72 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -68,7 +68,6 @@ public:\n> \n> private:\n> \tvoid assignStreams();\n> -\tvoid adjustStream(StreamConfiguration &cfg, bool scale);\n> \n> \t/*\n> \t * The IPU3CameraData instance is guaranteed to be valid as long as the\n> @@ -178,52 +177,6 @@ void IPU3CameraConfiguration::assignStreams()\n> \t}\n> }\n> \n> -void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)\n> -{\n> -\t/* The only pixel format the driver supports is NV12. */\n> -\tcfg.pixelFormat = formats::NV12;\n> -\n> -\tif (scale) {\n> -\t\t/*\n> -\t\t * Provide a suitable default that matches the sensor aspect\n> -\t\t * ratio.\n> -\t\t */\n> -\t\tif (!cfg.size.width || !cfg.size.height) {\n> -\t\t\tcfg.size.width = 1280;\n> -\t\t\tcfg.size.height = 1280 * cio2Configuration_.size.height\n> -\t\t\t\t\t/ cio2Configuration_.size.width;\n> -\t\t}\n> -\n> -\t\t/*\n> -\t\t * \\todo: Clamp the size to the hardware bounds when we will\n> -\t\t * figure them out.\n> -\t\t *\n> -\t\t * \\todo: Handle the scaler (BDS) restrictions. The BDS can\n> -\t\t * only scale with the same factor in both directions, and the\n> -\t\t * scaling factor is limited to a multiple of 1/32. At the\n> -\t\t * moment the ImgU driver hides these constraints by applying\n> -\t\t * additional cropping, this should be fixed on the driver\n> -\t\t * side, and cropping should be exposed to us.\n> -\t\t */\n> -\t} else {\n> -\t\t/*\n> -\t\t * \\todo: Properly support cropping when the ImgU driver\n> -\t\t * interface will be cleaned up.\n> -\t\t */\n> -\t\tcfg.size = cio2Configuration_.size;\n> -\t}\n> -\n> -\t/*\n> -\t * Clamp the size to match the ImgU alignment constraints. The width\n> -\t * shall be a multiple of 8 pixels and the height a multiple of 4\n> -\t * pixels.\n> -\t */\n> -\tif (cfg.size.width % 8 || cfg.size.height % 4) {\n> -\t\tcfg.size.width &= ~7;\n> -\t\tcfg.size.height &= ~3;\n> -\t}\n> -}\n> -\n> CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> {\n> \tStatus status = Valid;\n> @@ -244,7 +197,6 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> \t * resolution is large enough, pick the largest one.\n> \t */\n> \tSize size = {};\n> -\n> \tfor (const StreamConfiguration &cfg : config_) {\n> \t\tif (cfg.size.width > size.width)\n> \t\t\tsize.width = cfg.size.width;\n> @@ -264,20 +216,45 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> \tfor (unsigned int i = 0; i < config_.size(); ++i) {\n> \t\tStreamConfiguration &cfg = config_[i];\n> \t\tconst StreamConfiguration oldCfg = cfg;\n> -\t\tconst Stream *stream = streams_[i];\n> +\t\tconst PixelFormatInfo &info =\n> +\t\t\tPixelFormatInfo::info(cfg.pixelFormat);\n> \n> -\t\tif (stream == &data_->rawStream_) {\n> +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\n> \t\t\tcfg.size = cio2Configuration_.size;\n> \t\t\tcfg.pixelFormat = cio2Configuration_.pixelFormat;\n> \t\t\tcfg.bufferCount = cio2Configuration_.bufferCount;\n> \t\t} else {\n> -\t\t\tbool scale = stream == &data_->vfStream_;\n> -\t\t\tadjustStream(config_[i], scale);\n> +\t\t\t/*\n> +\t\t\t * Clamp the size to match the ImgU alignment\n> +\t\t\t * constraints.\n> +\t\t\t */\n> +\t\t\tsize.width = std::min(cfg.size.width,\n> +\t\t\t\t\t IPU3_OUTPUT_MAX_WIDTH);\n> +\t\t\tsize.height = std::min(cfg.size.height,\n> +\t\t\t\t\t IPU3_OUTPUT_MAX_HEIGHT);\n> +\t\t\tsize.width = std::max(cfg.size.width,\n> +\t\t\t\t\t minIPU3OutputSize.width);\n> +\t\t\tsize.height = std::max(cfg.size.height,\n> +\t\t\t\t\t minIPU3OutputSize.height);\n> +\t\t\tif (cfg.size.width % 8 || cfg.size.height % 4) {\n> +\t\t\t\tcfg.size.width &= ~7;\n> +\t\t\t\tcfg.size.height &= ~3;\n> +\t\t\t}\n> +\t\t\tcfg.pixelFormat = formats::NV12;\n> \t\t\tcfg.bufferCount = IPU3_BUFFER_COUNT;\n> +\n> +\t\t\t/*\n> +\t\t\t * \\todo: Handle the scaler (BDS) restrictions. The BDS\n> +\t\t\t * can only scale with the same factor in both\n> +\t\t\t * directions, and the scaling factor is limited to a\n> +\t\t\t * multiple of 1/32. At the moment the ImgU driver hides\n> +\t\t\t * these constraints by applying additional cropping,\n> +\t\t\t * this should be fixed on the driver side, and cropping\n> +\t\t\t * should be exposed to us.\n> +\t\t\t */\n> \t\t}\n> \n> -\t\tif (cfg.pixelFormat != oldCfg.pixelFormat ||\n> -\t\t cfg.size != oldCfg.size) {\n> +\t\tif (cfg.pixelFormat != oldCfg.pixelFormat || cfg.size != oldCfg.size) {\n> \t\t\tLOG(IPU3, Debug)\n> \t\t\t\t<< \"Stream \" << i << \" configuration adjusted to \"\n> \t\t\t\t<< cfg.toString();\n> -- \n> 2.27.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 DB31ABE905\n\tfor ;\n\tWed, 1 Jul 2020 16:44:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6673F60C59;\n\tWed, 1 Jul 2020 18:44:30 +0200 (CEST)","from mail-lj1-x244.google.com (mail-lj1-x244.google.com\n\t[IPv6:2a00:1450:4864:20::244])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B5CEF609A9\n\tfor ;\n\tWed, 1 Jul 2020 18:44:29 +0200 (CEST)","by mail-lj1-x244.google.com with SMTP id t25so23362508lji.12\n\tfor ;\n\tWed, 01 Jul 2020 09:44:29 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tb11sm2231112lfa.50.2020.07.01.09.44.28\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 01 Jul 2020 09:44:28 -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=\"qlk6jqTM\"; 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=2zrx7FuaIskmNdJgZR1eXY9VpHd2wYvFmHm7KYjXq0M=;\n\tb=qlk6jqTMlGM9tcWmXI2j6wr7oBArg6+97dWwwFFwV23BoAbko+TQ4kgUj8DMX5fMmr\n\t0DcddttH7TeH4rerELyYuK9rgQTm/OL6+XbEsiMj0EOzRpE7zXf4SECjnvRf4+VQ096i\n\tSibgNgm6q0QsKkY5YlA/V0b+U/H/+fCN94zYyP9skWoqfGWUMUfobA0OWy1jEAlFKZPO\n\tGoeAnp+3ej2vm52jN/2dQhMMVjTcfpbek8sKQzQjdNuSuFkhCozIpwUvpTpNsOfeFLuy\n\t0LppQCvobrsTnwSqp+GwqQYL5kpWWiTQ2ZZJvJ3qeoZ0lHTwKsbEw0UHWIRJhhINcBt/\n\tLzXg==","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=2zrx7FuaIskmNdJgZR1eXY9VpHd2wYvFmHm7KYjXq0M=;\n\tb=YfxZGP18EefP/mWjidrTSuywXWh3ShVaVFOOQtbaZSCAzGt27w8XEavr0EHV4pgMst\n\tg3q/o/qcIhaPbZSJCckMezruso20BrC98+74KLGWwtPZNC4n//bbxeOSeKtiiuElwznO\n\tMEpFNSGOIz6qd4VHnEbKW3X7FLHzicVQPV8zpwKsOl5eMeHfZa1ceGI6ukoRg7d10nEB\n\tCKGYIOZIZBjyFi0u/JQ5qRD9j9Lna2W1d7tq7DsPbgvgNjUDjI4zNyF5eJFdp+cXFVwx\n\ty8eRCt25LtUb101y6qT1vegoEX/AA6CvGzw7UtkodI6VQaieBMPQ4d6S/AoBxvDnXcqj\n\tjKuQ==","X-Gm-Message-State":"AOAM531+Fx4PuJqf5lQZ4HWCYc9HuCmbr7j8u2PefsU0ryAH2NFPpA0n\n\tflLEZEBoq6Nh+xhe+ZzyfqWglA==","X-Google-Smtp-Source":"ABdhPJzWc7q22SrGkmZNUAL3FCGoFj4Y5PsKAXXXV0wNaYnHFgLjoc9nWsuYi4jq2s6smPIgrtaK0g==","X-Received":"by 2002:a05:651c:119a:: with SMTP id\n\tw26mr13363315ljo.126.1593621868902; \n\tWed, 01 Jul 2020 09:44:28 -0700 (PDT)","Date":"Wed, 1 Jul 2020 18:44:27 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"<20200701164427.GH2399385@oden.dyn.berto.se>","References":"<20200701123036.51922-1-jacopo@jmondi.org>\n\t<20200701123036.51922-7-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200701123036.51922-7-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 06/15] libcamera: ipu3: Rework stream\n\tvalidation","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\" "}}]