[{"id":11734,"web_url":"https://patchwork.libcamera.org/comment/11734/","msgid":"<20200730164845.GD6107@pendragon.ideasonboard.com>","date":"2020-07-30T16:48:45","subject":"Re: [libcamera-devel] [PATCH v4 13/19] libcamera: ipu3: Adjust and\n\tassign streams in validate()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Mon, Jul 20, 2020 at 12:47:30PM +0200, Jacopo Mondi wrote:\n> Remove the adjustStream() and assignStream() methods, and perform stream\n> adjustment and assignment while iterating the StreamConfiguration\n> items.\n> \n> The adjustStream() implementation had some arbitrary assumption, like\n> the main output having to be as large as the sensor resolution, and did\n> not take into account the different alignment requirements between the\n> main output and the viewfinder output.\n> \n> The assignStream() implementation also assumes only full-size streams\n> can be produced by the main output, and having it as a separate function\n> prevents adjusting streams according to which output they are assigned.\n> \n> Blend the two implementation in a single loop and perform the required\n> stream adjustment and assignment in one go.\n> \n> As streams are now assigned at validate() time, remove the same\n> operation from the configure() function.\n> \n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> ---\n>  src/libcamera/pipeline/ipu3/ipu3.cpp | 250 ++++++++++++---------------\n>  1 file changed, 111 insertions(+), 139 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 6eaba401d68e..24ad37eb1831 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -70,9 +70,6 @@ public:\n>  \tconst std::vector<const Stream *> &streams() { return streams_; }\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>  \t * corresponding Camera instance is valid. In order to borrow a\n> @@ -137,96 +134,6 @@ IPU3CameraConfiguration::IPU3CameraConfiguration(Camera *camera,\n>  \tdata_ = data;\n>  }\n>  \n> -void IPU3CameraConfiguration::assignStreams()\n> -{\n> -\t/*\n> -\t * Verify and update all configuration entries, and assign a stream to\n> -\t * each of them. The viewfinder stream can scale, while the output\n> -\t * stream can crop only, so select the output stream when the requested\n> -\t * resolution is equal to the sensor resolution, and the viewfinder\n> -\t * stream otherwise.\n> -\t */\n> -\tstd::set<const Stream *> availableStreams = {\n> -\t\t&data_->outStream_,\n> -\t\t&data_->vfStream_,\n> -\t\t&data_->rawStream_,\n> -\t};\n> -\n> -\t/*\n> -\t * The caller is responsible to limit the number of requested streams\n> -\t * to a number supported by the pipeline before calling this function.\n> -\t */\n> -\tASSERT(availableStreams.size() >= config_.size());\n> -\n> -\tstreams_.clear();\n> -\tstreams_.reserve(config_.size());\n> -\n> -\tfor (const StreamConfiguration &cfg : config_) {\n> -\t\tconst PixelFormatInfo &info =\n> -\t\t\tPixelFormatInfo::info(cfg.pixelFormat);\n> -\t\tconst Stream *stream;\n> -\n> -\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> -\t\t\tstream = &data_->rawStream_;\n> -\t\telse if (cfg.size == cio2Configuration_.size)\n> -\t\t\tstream = &data_->outStream_;\n> -\t\telse\n> -\t\t\tstream = &data_->vfStream_;\n> -\n> -\t\tif (availableStreams.find(stream) == availableStreams.end())\n> -\t\t\tstream = *availableStreams.begin();\n> -\n> -\t\tstreams_.push_back(stream);\n> -\t\tavailableStreams.erase(stream);\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.isNull()) {\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> @@ -242,27 +149,27 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n>  \n>  \t/*\n>  \t * Validate the requested stream configuration and select the sensor\n> -\t * format by collecting the maximum width and height and picking the\n> -\t * closest larger match, as the IPU3 can downscale only. If no\n> -\t * resolution is requested for any stream, or if no sensor resolution is\n> -\t * large enough, pick the largest one.\n> +\t * format by collecting the maximum RAW stream width and height and\n> +\t * picking the closest larger match, as the IPU3 can downscale only. If\n> +\t * no resolution is requested for the RAW stream, use the one from the\n> +\t * largest YUV stream, plus margins pixels for the IF and BDS to scale.\n> +\t * If no resolution is requested for any stream, pick the largest one.\n>  \t */\n>  \tunsigned int rawCount = 0;\n>  \tunsigned int yuvCount = 0;\n> -\tSize size;\n> +\tSize maxYuvSize;\n> +\tSize maxRawSize;\n>  \n>  \tfor (const StreamConfiguration &cfg : config_) {\n>  \t\tconst PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);\n>  \n> -\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\n>  \t\t\trawCount++;\n> -\t\telse\n> +\t\t\tmaxRawSize.expandTo(cfg.size);\n> +\t\t} else {\n>  \t\t\tyuvCount++;\n> -\n> -\t\tif (cfg.size.width > size.width)\n> -\t\t\tsize.width = cfg.size.width;\n> -\t\tif (cfg.size.height > size.height)\n> -\t\t\tsize.height = cfg.size.height;\n> +\t\t\tmaxYuvSize.expandTo(cfg.size);\n> +\t\t}\n>  \t}\n>  \n>  \tif (rawCount > 1 || yuvCount > 2) {\n> @@ -270,43 +177,113 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n>  \t\treturn Invalid;\n>  \t}\n>  \n> -\t/* Generate raw configuration from CIO2. */\n> -\tcio2Configuration_ = data_->cio2_.generateConfiguration(size);\n> +\tif (maxRawSize.isNull())\n> +\t\tmaxRawSize = maxYuvSize.alignedUpTo(IMGU_OUTPUT_WIDTH_MARGIN,\n> +\t\t\t\t\t\t    IMGU_OUTPUT_HEIGHT_MARGIN)\n> +\t\t\t\t       .boundedTo(data_->cio2_.sensor()->resolution());\n> +\n> +\t/*\n> +\t * Generate raw configuration from CIO2.\n> +\t *\n> +\t * The output YUV streams will be limited in size to the maximum\n> +\t * frame size requested for the RAW stream.\n> +\t */\n> +\tcio2Configuration_ = data_->cio2_.generateConfiguration(maxRawSize);\n>  \tif (!cio2Configuration_.pixelFormat.isValid())\n>  \t\treturn Invalid;\n>  \n> -\t/* Assign streams to each configuration entry. */\n> -\tassignStreams();\n> +\tLOG(IPU3, Debug) << \"CIO2 configuration: \" << cio2Configuration_.toString();\n>  \n> -\t/* Verify and adjust configuration if needed. */\n> +\t/*\n> +\t * Adjust the configurations if needed and assign streams while\n> +\t * iterating them.\n> +\t */\n> +\tbool mainOutputAvailable = true;\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> -\n> -\t\tif (stream == &data_->rawStream_) {\n> -\t\t\tcfg.size = cio2Configuration_.size;\n> -\t\t\tcfg.pixelFormat = cio2Configuration_.pixelFormat;\n> -\t\t\tcfg.bufferCount = cio2Configuration_.bufferCount;\n> +\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(config_[i].pixelFormat);\n> +\t\tconst StreamConfiguration originalCfg = config_[i];\n> +\t\tStreamConfiguration *cfg = &config_[i];\n> +\n> +\t\tLOG(IPU3, Debug) << \"Validating stream: \" << config_[i].toString();\n> +\n> +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\n> +\t\t\t/* Initialize the RAW stream with the CIO2 configuration. */\n> +\t\t\tcfg->size = cio2Configuration_.size;\n> +\t\t\tcfg->pixelFormat = cio2Configuration_.pixelFormat;\n> +\t\t\tcfg->bufferCount = cio2Configuration_.bufferCount;\n> +\t\t\tcfg->stride = info.stride(cfg->size.width, 0, 64);\n> +\t\t\tcfg->frameSize = info.frameSize(cfg->size, 64);\n> +\t\t\tcfg->setStream(const_cast<Stream *>(&data_->rawStream_));\n> +\n> +\t\t\tLOG(IPU3, Debug) << \"Assigned \" << cfg->toString()\n> +\t\t\t\t\t << \" to the raw stream\";\n>  \t\t} else {\n> -\t\t\tbool scale = stream == &data_->vfStream_;\n> -\t\t\tadjustStream(config_[i], scale);\n> -\t\t\tcfg.bufferCount = IPU3_BUFFER_COUNT;\n> +\t\t\t/* Assign and configure the main and viewfinder outputs. */\n> +\n> +\t\t\t/*\n> +\t\t\t * Clamp the size to match the ImgU size limits and the\n> +\t\t\t * margins from the CIO2 output frame size.\n> +\t\t\t *\n> +\t\t\t * The ImgU outputs needs to be strictly smaller than\n> +\t\t\t * the CIO2 output frame and rounded down to 64\n> +\t\t\t * pixels in width and 32 pixels in height. This comes\n> +\t\t\t * from inspecting the pipe configuration script results\n> +\t\t\t * and the available suggested configurations in the\n> +\t\t\t * ChromeOS BSP .xml camera tuning files.\n> +\t\t\t *\n> +\t\t\t * \\todo Clarify what are the hardware constraints\n> +\t\t\t * that require this alignements, if any. It might\n> +\t\t\t * depend on the BDS scaling factor of 1/32, as the main\n> +\t\t\t * output has no YUV scaler as the viewfinder output has.\n> +\t\t\t */\n> +\t\t\tunsigned int limit;\n> +\t\t\tlimit = utils::alignDown(cio2Configuration_.size.width - 1,\n> +\t\t\t\t\t\t IMGU_OUTPUT_WIDTH_MARGIN);\n> +\t\t\tcfg->size.width = utils::clamp(cfg->size.width,\n> +\t\t\t\t\t\t       IMGU_OUTPUT_MIN_SIZE.width,\n> +\t\t\t\t\t\t       limit);\n> +\n> +\t\t\tlimit = utils::alignDown(cio2Configuration_.size.height - 1,\n> +\t\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN);\n> +\t\t\tcfg->size.height = utils::clamp(cfg->size.height,\n> +\t\t\t\t\t\t\tIMGU_OUTPUT_MIN_SIZE.height,\n> +\t\t\t\t\t\t\tlimit);\n> +\n> +\t\t\tcfg->size.alignDownTo(IMGU_OUTPUT_WIDTH_ALIGN,\n> +\t\t\t\t\t      IMGU_OUTPUT_HEIGHT_ALIGN);\n> +\n> +\t\t\tcfg->pixelFormat = formats::NV12;\n> +\t\t\tcfg->bufferCount = IPU3_BUFFER_COUNT;\n> +\t\t\tcfg->stride = info.stride(cfg->size.width, 0, 1);\n> +\t\t\tcfg->frameSize = info.frameSize(cfg->size, 1);\n> +\n> +\t\t\t/*\n> +\t\t\t * Use the main output stream in case only one stream is\n> +\t\t\t * requested or if the current configuration is the one\n> +\t\t\t * with the maximum YUV output size.\n> +\t\t\t */\n> +\t\t\tif (mainOutputAvailable &&\n> +\t\t\t    (originalCfg.size == maxYuvSize || yuvCount == 1)) {\n> +\t\t\t\tcfg->setStream(const_cast<Stream *>(&data_->outStream_));\n> +\t\t\t\tmainOutputAvailable = false;\n> +\n> +\t\t\t\tLOG(IPU3, Debug) << \"Assigned \" << cfg->toString()\n> +\t\t\t\t\t\t << \" to the main output\";\n> +\t\t\t} else {\n> +\t\t\t\tcfg->setStream(const_cast<Stream *>(&data_->vfStream_));\n> +\n> +\t\t\t\tLOG(IPU3, Debug) << \"Assigned \" << cfg->toString()\n> +\t\t\t\t\t\t << \" to the viewfinder output\";\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 != originalCfg.pixelFormat ||\n> +\t\t    cfg->size != originalCfg.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> +\t\t\t\t<< cfg->toString();\n>  \t\t\tstatus = Adjusted;\n>  \t\t}\n> -\n> -\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);\n> -\t\tbool packedRaw = info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> -\n> -\t\tcfg.stride = info.stride(cfg.size.width, 0, packedRaw ? 64 : 1);\n> -\t\tcfg.frameSize = info.frameSize(cfg.size, packedRaw ? 64 : 1);\n>  \t}\n>  \n>  \treturn status;\n> @@ -340,6 +317,9 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n>  \t\t\t * strictly smaller than the sensor resolution (limited\n>  \t\t\t * to the ImgU  maximum output size) and aligned down to\n>  \t\t\t * the required frame margin.\n> +\t\t\t *\n> +\t\t\t * The alignment constraints should be clarified\n> +\t\t\t * as per the todo item in validate().\n\nCould you write this as\n\n\t\t\t * \\todo Clarify the alignment constraints as explained\n\t\t\t * in validate()\n\nto make sure this shows up in the todo list ? Otherwise we may miss it.\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n>  \t\t\t */\n>  \t\t\tsize = sensorResolution.boundedTo(IMGU_OUTPUT_MAX_SIZE);\n>  \t\t\tsize.width = utils::alignDown(size.width - 1,\n> @@ -476,16 +456,8 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n>  \tbool vfActive = false;\n>  \n>  \tfor (unsigned int i = 0; i < config->size(); ++i) {\n> -\t\t/*\n> -\t\t * Use a const_cast<> here instead of storing a mutable stream\n> -\t\t * pointer in the configuration to let the compiler catch\n> -\t\t * unwanted modifications of camera data in the configuration\n> -\t\t * validate() implementation.\n> -\t\t */\n> -\t\tStream *stream = const_cast<Stream *>(config->streams()[i]);\n>  \t\tStreamConfiguration &cfg = (*config)[i];\n> -\n> -\t\tcfg.setStream(stream);\n> +\t\tStream *stream = cfg.stream();\n>  \n>  \t\tif (stream == outStream) {\n>  \t\t\tret = imgu->configureOutput(cfg, &outputFormat);","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 7B1E6BD86F\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 30 Jul 2020 16:48:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F22B161DC3;\n\tThu, 30 Jul 2020 18:48:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 21135611A2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 30 Jul 2020 18:48:55 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7FF049B1;\n\tThu, 30 Jul 2020 18:48:54 +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=\"M6zCnMP4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596127734;\n\tbh=qLb4dmW6suRLpzx2CTLBBqvxgBJ2WVtHBcrncL5tkGk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=M6zCnMP4QY8nWmi/0qxoohTrq+TPIcRFIbBq4htiCSRiX1Nv/bW3680T3DdL67NW0\n\tGfbXxmrUskBwXvNQqHaRVy1eIO9oE3xFUbrsLPIqbmygBB39xR9QxtyFO5vSTow+j1\n\tkyYyRxQvJ13NNgj9n8ABrDkSyMnHig3ChxlIl4P8=","Date":"Thu, 30 Jul 2020 19:48:45 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<20200730164845.GD6107@pendragon.ideasonboard.com>","References":"<20200720104736.19986-1-jacopo@jmondi.org>\n\t<20200720104736.19986-14-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200720104736.19986-14-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v4 13/19] libcamera: ipu3: Adjust and\n\tassign streams in validate()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]