[{"id":26928,"web_url":"https://patchwork.libcamera.org/comment/26928/","msgid":"<20230424060644.GA4926@pendragon.ideasonboard.com>","date":"2023-04-24T06:06:44","subject":"Re: [libcamera-devel] [PATCH v2 5/6] libcamera: imx8-isi: Split\n\tBayer/YUV config generation","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, Mar 13, 2023 at 10:19:43AM +0100, Jacopo Mondi wrote:\n> At generateConfiguration() a YUV/RGB pixel format is preferred for the\n> StillCapture/VideoRecording/Viewfinder roles, but currently there are no\n> guarantees in place that the sensor provides a non-Bayer bus format from\n> which YUV/RGB can be generated.\n> \n> This makes the default configuration generated for those roles not to\n> work if the sensor is a RAW-only one.\n> \n> To improve the situation split the configuration generation in two,\n> one for YUV modes and one for Raw Bayer mode.\n> \n> StreamRoles assigned to a YUV mode will try to first generate a YUV\n> configuration and then fallback to RAW if that's what the sensor can\n> provide.\n> \n> As an additional requirement, for YUV streams, the generated mode has to\n> be validated with the sensor to confirm the desired sizes can be\n> generated. In order to test a format use the newly introduced\n> CameraSensor::tryFormat().\n\nPlease explain in the commit message (and/or a comment in the code) why\nthis is needed.\n\nI'm also tempted to ask for this new requirement to be split to a\nseparate patch, to ease review.\n\n> Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>\n> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>\n> Tested-by: Daniel Scally <dan.scally@ideasonboard.com>\n> ---\n>  src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 231 +++++++++++++------\n>  1 file changed, 155 insertions(+), 76 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> index 146ba7465012..a4f437f55b26 100644\n> --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> @@ -145,6 +145,10 @@ private:\n>  \n>  \tPipe *pipeFromStream(Camera *camera, const Stream *stream);\n>  \n> +\tStreamConfiguration generateYUVConfiguration(Camera *camera,\n> +\t\t\t\t\t\t     const Size &size);\n> +\tStreamConfiguration generateRawConfiguration(Camera *camera);\n> +\n>  \tvoid bufferReady(FrameBuffer *buffer);\n>  \n>  \tMediaDevice *isiDev_;\n> @@ -705,6 +709,129 @@ PipelineHandlerISI::PipelineHandlerISI(CameraManager *manager)\n>  {\n>  }\n>  \n> +/*\n> + * Generate a StreamConfiguration for YUV/RGB use case.\n> + *\n> + * Verify it the sensor can produce a YUV/RGB media bus format and collect\n> + * all the processed pixel formats the ISI can generate as supported stream\n> + * configurations.\n> + */\n> +StreamConfiguration PipelineHandlerISI::generateYUVConfiguration(Camera *camera,\n> +\t\t\t\t\t\t\t\t const Size &size)\n> +{\n> +\tISICameraData *data = cameraData(camera);\n> +\tPixelFormat pixelFormat = formats::YUYV;\n> +\tunsigned int mbusCode;\n> +\n> +\tmbusCode = data->getYuvMediaBusFormat(&pixelFormat);\n> +\tif (!mbusCode)\n> +\t\treturn {};\n> +\n> +\t/* Adjust the requested size to the sensor's capabilities. */\n> +\tconst CameraSensor *sensor = data->sensor_.get();\n> +\n> +\tV4L2SubdeviceFormat sensorFmt;\n> +\tsensorFmt.mbus_code = mbusCode;\n> +\tsensorFmt.size = size;\n> +\n> +\tint ret = sensor->tryFormat(&sensorFmt);\n\n\tint ret = data->sensor_->tryFormat(&sensorFmt);\n\nand drop the local sensor variable.\n\n> +\tif (ret) {\n> +\t\tLOG(ISI, Error) << \"Failed to try sensor format.\";\n> +\t\treturn {};\n> +\t}\n> +\n> +\tSize sensorSize = sensorFmt.size;\n> +\n> +\t/*\n> +\t * Populate the StreamConfiguration.\n> +\t *\n> +\t * As the sensor supports at least one YUV/RGB media bus format all the\n> +\t * processed ones in formatsMap_ can be generated from it.\n> +\t */\n> +\tstd::map<PixelFormat, std::vector<SizeRange>> streamFormats;\n> +\n> +\tfor (const auto &[pixFmt, pipeFmt] : ISICameraConfiguration::formatsMap_) {\n> +\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);\n> +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> +\t\t\tcontinue;\n> +\n> +\t\tstreamFormats[pixFmt] = { { kMinISISize, sensorSize } };\n> +\t}\n> +\n> +\tStreamFormats formats(streamFormats);\n> +\n> +\tStreamConfiguration cfg(formats);\n> +\tcfg.pixelFormat = pixelFormat;\n> +\tcfg.size = sensorSize;\n> +\tcfg.bufferCount = 4;\n> +\n> +\treturn cfg;\n> +}\n> +\n> +/*\n> + * Generate a StreamConfiguration for Raw Bayer use case. Verify if the sensor\n> + * can produce the requested RAW bayer format and eventually adjust it to\n> + * the one with the largest bit-depth the sensor can produce.\n> + */\n> +StreamConfiguration PipelineHandlerISI::generateRawConfiguration(Camera *camera)\n> +{\n> +\tstatic const std::map<unsigned int, PixelFormat> rawFormats = {\n> +\t\t{ MEDIA_BUS_FMT_SBGGR8_1X8, formats::SBGGR8 },\n> +\t\t{ MEDIA_BUS_FMT_SGBRG8_1X8, formats::SGBRG8 },\n> +\t\t{ MEDIA_BUS_FMT_SGRBG8_1X8, formats::SGRBG8 },\n> +\t\t{ MEDIA_BUS_FMT_SRGGB8_1X8, formats::SRGGB8 },\n> +\t\t{ MEDIA_BUS_FMT_SBGGR10_1X10, formats::SBGGR10 },\n> +\t\t{ MEDIA_BUS_FMT_SGBRG10_1X10, formats::SGBRG10 },\n> +\t\t{ MEDIA_BUS_FMT_SGRBG10_1X10, formats::SGRBG10 },\n> +\t\t{ MEDIA_BUS_FMT_SRGGB10_1X10, formats::SRGGB10 },\n> +\t\t{ MEDIA_BUS_FMT_SBGGR12_1X12, formats::SBGGR12 },\n> +\t\t{ MEDIA_BUS_FMT_SGBRG12_1X12, formats::SGBRG12 },\n> +\t\t{ MEDIA_BUS_FMT_SGRBG12_1X12, formats::SGRBG12 },\n> +\t\t{ MEDIA_BUS_FMT_SRGGB12_1X12, formats::SRGGB12 },\n\nNow that libcamera has RAW14 formats, could you add them here ?\n\n> +\t};\n> +\n> +\tISICameraData *data = cameraData(camera);\n> +\tPixelFormat pixelFormat = formats::SBGGR10;\n> +\tunsigned int mbusCode;\n> +\n> +\t/* pixelFormat will be adjusted, if the sensor can produce RAW. */\n> +\tmbusCode = data->getRawMediaBusFormat(&pixelFormat);\n> +\tif (!mbusCode)\n> +\t\treturn {};\n> +\n> +\t/*\n> +\t * Populate the StreamConfiguration with all the supported Bayer\n> +\t * formats the sensor can produce.\n> +\t */\n> +\tstd::map<PixelFormat, std::vector<SizeRange>> streamFormats;\n> +\tconst CameraSensor *sensor = data->sensor_.get();\n> +\n> +\tfor (unsigned int code : sensor->mbusCodes()) {\n> +\t\t/* Find a Bayer media bus code from the sensor. */\n> +\t\tconst BayerFormat &bayerFormat = BayerFormat::fromMbusCode(code);\n> +\t\tif (!bayerFormat.isValid())\n> +\t\t\tcontinue;\n> +\n> +\t\tauto it = rawFormats.find(code);\n> +\t\tif (it == rawFormats.end()) {\n> +\t\t\tLOG(ISI, Warning) << bayerFormat\n> +\t\t\t\t\t  << \" not supported in ISI formats map.\";\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tstreamFormats[it->second] = { { sensor->resolution(), sensor->resolution() } };\n> +\t}\n> +\n> +\tStreamFormats formats(streamFormats);\n> +\n> +\tStreamConfiguration cfg(formats);\n> +\tcfg.size = sensor->resolution();\n> +\tcfg.pixelFormat = pixelFormat;\n> +\tcfg.bufferCount = 4;\n> +\n> +\treturn cfg;\n> +}\n> +\n>  std::unique_ptr<CameraConfiguration>\n>  PipelineHandlerISI::generateConfiguration(Camera *camera,\n>  \t\t\t\t\t  const StreamRoles &roles)\n> @@ -722,79 +849,45 @@ PipelineHandlerISI::generateConfiguration(Camera *camera,\n>  \t\treturn nullptr;\n>  \t}\n>  \n> -\tbool isRaw = false;\n>  \tfor (const auto &role : roles) {\n>  \t\t/*\n> -\t\t * Prefer the following formats\n> +\t\t * Prefer the following formats:\n>  \t\t * - Still Capture: Full resolution YUYV\n>  \t\t * - ViewFinder/VideoRecording: 1080p YUYV\n> -\t\t * - RAW: sensor's native format and resolution\n> +\t\t * - RAW: Full resolution Bayer\n>  \t\t */\n> -\t\tstd::map<PixelFormat, std::vector<SizeRange>> streamFormats;\n> -\t\tPixelFormat pixelFormat;\n> -\t\tSize size;\n> +\t\tStreamConfiguration cfg;\n>  \n>  \t\tswitch (role) {\n>  \t\tcase StreamRole::StillCapture:\n> -\t\t\t/*\n> -\t\t\t * \\todo Make sure the sensor can produce non-RAW formats\n> -\t\t\t * compatible with the ones supported by the pipeline.\n> -\t\t\t */\n> -\t\t\tsize = data->sensor_->resolution();\n> -\t\t\tpixelFormat = formats::YUYV;\n> +\t\t\tcfg = generateYUVConfiguration(camera,\n> +\t\t\t\t\t\t       data->sensor_->resolution());\n> +\t\t\tif (!cfg.pixelFormat.isValid()) {\n> +\t\t\t\t/*\n> +\t\t\t\t * Fallback to use a Bayer format if that's what\n> +\t\t\t\t * the sensor supports.\n> +\t\t\t\t */\n> +\t\t\t\tcfg = generateRawConfiguration(camera);\n> +\t\t\t}\n> +\n>  \t\t\tbreak;\n>  \n>  \t\tcase StreamRole::Viewfinder:\n>  \t\tcase StreamRole::VideoRecording:\n> -\t\t\t/*\n> -\t\t\t * \\todo Make sure the sensor can produce non-RAW formats\n> -\t\t\t * compatible with the ones supported by the pipeline.\n> -\t\t\t */\n> -\t\t\tsize = PipelineHandlerISI::kPreviewSize;\n> -\t\t\tpixelFormat = formats::YUYV;\n> -\t\t\tbreak;\n> -\n> -\t\tcase StreamRole::Raw: {\n> -\t\t\t/*\n> -\t\t\t * Make sure the sensor can generate a RAW format and\n> -\t\t\t * prefer the ones with a larger bitdepth.\n> -\t\t\t */\n> -\t\t\tconst ISICameraConfiguration::FormatMap::value_type *rawPipeFormat = nullptr;\n> -\t\t\tunsigned int maxDepth = 0;\n> -\n> -\t\t\tfor (unsigned int code : data->sensor_->mbusCodes()) {\n> -\t\t\t\tconst BayerFormat &bayerFormat = BayerFormat::fromMbusCode(code);\n> -\t\t\t\tif (!bayerFormat.isValid())\n> -\t\t\t\t\tcontinue;\n> -\n> -\t\t\t\t/* Make sure the format is supported by the pipeline handler. */\n> -\t\t\t\tauto it = std::find_if(ISICameraConfiguration::formatsMap_.begin(),\n> -\t\t\t\t\t\t       ISICameraConfiguration::formatsMap_.end(),\n> -\t\t\t\t\t\t       [code](auto &isiFormat) {\n> -\t\t\t\t\t\t\t        auto &pipe = isiFormat.second;\n> -\t\t\t\t\t\t\t        return pipe.sensorCode == code;\n> -\t\t\t\t\t\t       });\n> -\t\t\t\tif (it == ISICameraConfiguration::formatsMap_.end())\n> -\t\t\t\t\tcontinue;\n> -\n> -\t\t\t\tif (bayerFormat.bitDepth > maxDepth) {\n> -\t\t\t\t\tmaxDepth = bayerFormat.bitDepth;\n> -\t\t\t\t\trawPipeFormat = &(*it);\n> -\t\t\t\t}\n> -\t\t\t}\n> -\n> -\t\t\tif (!rawPipeFormat) {\n> -\t\t\t\tLOG(ISI, Error)\n> -\t\t\t\t\t<< \"Cannot generate a configuration for RAW stream\";\n> -\t\t\t\treturn nullptr;\n> +\t\t\tcfg = generateYUVConfiguration(camera,\n> +\t\t\t\t\t\t       PipelineHandlerISI::kPreviewSize);\n> +\t\t\tif (!cfg.pixelFormat.isValid()) {\n> +\t\t\t\t/*\n> +\t\t\t\t * Fallback to use a Bayer format if that's what\n> +\t\t\t\t * the sensor supports.\n> +\t\t\t\t */\n> +\t\t\t\tcfg = generateRawConfiguration(camera);\n>  \t\t\t}\n>  \n> -\t\t\tsize = data->sensor_->resolution();\n> -\t\t\tpixelFormat = rawPipeFormat->first;\n> -\n> -\t\t\tstreamFormats[pixelFormat] = { { kMinISISize, size } };\n> -\t\t\tisRaw = true;\n> +\t\t\tbreak;\n>  \n> +\t\tcase StreamRole::Raw: {\n> +\t\t\tcfg = generateRawConfiguration(camera);\n>  \t\t\tbreak;\n>  \t\t}\n>  \n\nWould this be clearer ?\n\n\t\tswitch (role) {\n\t\tcase StreamRole::StillCapture:\n\t\tcase StreamRole::Viewfinder:\n\t\tcase StreamRole::VideoRecording:\n\t\t\tSize size = role == StreamRole::StillCapture\n\t\t\t\t  ? data->sensor_->resolution()\n\t\t\t\t  : PipelineHandlerISI::kPreviewSize;\n\t\t\tcfg = generateYUVConfiguration(camera, size);\n\t\t\tif (cfg.pixelFormat.isValid())\n\t\t\t\tbreak;\n\n\t\t\t/*\n\t\t\t * Fallback to use a Bayer format if that's what the\n\t\t\t * sensor supports.\n\t\t\t */\n\t\t\t[[fallthrough]]\n\n\t\tcase StreamRole::Raw: {\n\t\t\tcfg = generateRawConfiguration(camera);\n\t\t\tbreak;\n\t\t}\n\nUp to you.\n\n> @@ -803,26 +896,12 @@ PipelineHandlerISI::generateConfiguration(Camera *camera,\n>  \t\t\treturn nullptr;\n>  \t\t}\n>  \n> -\t\t/*\n> -\t\t * For non-RAW configurations the ISI can perform colorspace\n> -\t\t * conversion. List all the supported output formats here.\n> -\t\t */\n> -\t\tif (!isRaw) {\n> -\t\t\tfor (const auto &[pixFmt, pipeFmt] : ISICameraConfiguration::formatsMap_) {\n> -\t\t\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);\n> -\t\t\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> -\t\t\t\t\tcontinue;\n> -\n> -\t\t\t\tstreamFormats[pixFmt] = { { kMinISISize, size } };\n> -\t\t\t}\n> +\t\tif (!cfg.pixelFormat.isValid()) {\n> +\t\t\tLOG(ISI, Error)\n> +\t\t\t\t<< \"Cannot generate configuration for role: \" << role;\n> +\t\t\treturn nullptr;\n>  \t\t}\n>  \n> -\t\tStreamFormats formats(streamFormats);\n> -\n> -\t\tStreamConfiguration cfg(formats);\n> -\t\tcfg.pixelFormat = pixelFormat;\n> -\t\tcfg.size = size;\n> -\t\tcfg.bufferCount = 4;\n>  \t\tconfig->addConfiguration(cfg);\n>  \t}\n>","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 695A6C3272\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 24 Apr 2023 06:06:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B4161627D1;\n\tMon, 24 Apr 2023 08:06:33 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DDA16627CF\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 24 Apr 2023 08:06:31 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(133-32-181-51.west.xps.vectant.ne.jp [133.32.181.51])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 621184AD;\n\tMon, 24 Apr 2023 08:06:21 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682316393;\n\tbh=iJxrkhawaMVffCOt89n1tDNuA/o5ZzCH3IDyRQKbPnU=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=RNb3TjLJ+xxcMIfeL+TNhk2GhknDpcop5MidxIWQ37IJH5XtVJMAQJigUlJmnymXP\n\tlI1IkaCveIxWcTaYIz3/v6GBAAN3/vd+ps79C8gBOB+lV2vGF+35BzkJ2+x+wp/MjD\n\t8B1ZG8RxSlyChbz0cpkB0rINze2yyWja6TH3A6Et9fRn/3D1OKCycpGQ5yNog6bbCD\n\tlXL8lX/wVGAFrIEDUTbzdh2uFYlyTcd3l/D9W1cenJDE2m930XSh0UsamgW3f8gT3V\n\tIo9y3Aojtbrrg5aNxVOxXQtUo7aDhw1z/dOSa+8cvfhpWv5Wy2tIO/+nOsW4EqJ46J\n\tVogJsgUmODsKQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1682316382;\n\tbh=iJxrkhawaMVffCOt89n1tDNuA/o5ZzCH3IDyRQKbPnU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=p426oqYx/f4ENK/i4gCb/XZAeSEp26/6ynJ/5No9HEoPB04ufjtD258UrwWL0V1py\n\tRl2eTssjATysYMKdSRTaSoFyYE2dLbZkrCOqNtASHTFay3aFqlXN5Km5IzNk12WU6g\n\tQBzpW7uubEU7JpPmptTxlvHWHVKQYuqFpKtEmseE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"p426oqYx\"; dkim-atps=neutral","Date":"Mon, 24 Apr 2023 09:06:44 +0300","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Message-ID":"<20230424060644.GA4926@pendragon.ideasonboard.com>","References":"<20230313091944.9530-1-jacopo.mondi@ideasonboard.com>\n\t<20230313091944.9530-6-jacopo.mondi@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230313091944.9530-6-jacopo.mondi@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 5/6] libcamera: imx8-isi: Split\n\tBayer/YUV config generation","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>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":26936,"web_url":"https://patchwork.libcamera.org/comment/26936/","msgid":"<2ckzybxn7lwfjz2qnqe74wetpyi4lk5xbow3y44whztm3aus3p@s3b3ckfyfzsg>","date":"2023-04-26T13:00:08","subject":"Re: [libcamera-devel] [PATCH v2 5/6] libcamera: imx8-isi: Split\n\tBayer/YUV config generation","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Laurent\n\nOn Mon, Apr 24, 2023 at 09:06:44AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Mon, Mar 13, 2023 at 10:19:43AM +0100, Jacopo Mondi wrote:\n> > At generateConfiguration() a YUV/RGB pixel format is preferred for the\n> > StillCapture/VideoRecording/Viewfinder roles, but currently there are no\n> > guarantees in place that the sensor provides a non-Bayer bus format from\n> > which YUV/RGB can be generated.\n> >\n> > This makes the default configuration generated for those roles not to\n> > work if the sensor is a RAW-only one.\n> >\n> > To improve the situation split the configuration generation in two,\n> > one for YUV modes and one for Raw Bayer mode.\n> >\n> > StreamRoles assigned to a YUV mode will try to first generate a YUV\n> > configuration and then fallback to RAW if that's what the sensor can\n> > provide.\n> >\n> > As an additional requirement, for YUV streams, the generated mode has to\n> > be validated with the sensor to confirm the desired sizes can be\n> > generated. In order to test a format use the newly introduced\n> > CameraSensor::tryFormat().\n>\n> Please explain in the commit message (and/or a comment in the code) why\n> this is needed.\n>\n\nWhat do you mean ?\n\n\"for YUV streams, the generated mode has to be validated with the sensor\nto confirm the desired sizes can be generated.\"\n\nThe ISI cannot upscale, what's controversial here ?\n\n> I'm also tempted to ask for this new requirement to be split to a\n> separate patch, to ease review.\n>\n\nIt's not a new requirement, it was simply ignored by the existing and\nknown-to-be-broken implementation\n\n\n> > Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>\n> > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>\n> > Tested-by: Daniel Scally <dan.scally@ideasonboard.com>\n> > ---\n> >  src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 231 +++++++++++++------\n> >  1 file changed, 155 insertions(+), 76 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> > index 146ba7465012..a4f437f55b26 100644\n> > --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> > +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> > @@ -145,6 +145,10 @@ private:\n> >\n> >  \tPipe *pipeFromStream(Camera *camera, const Stream *stream);\n> >\n> > +\tStreamConfiguration generateYUVConfiguration(Camera *camera,\n> > +\t\t\t\t\t\t     const Size &size);\n> > +\tStreamConfiguration generateRawConfiguration(Camera *camera);\n> > +\n> >  \tvoid bufferReady(FrameBuffer *buffer);\n> >\n> >  \tMediaDevice *isiDev_;\n> > @@ -705,6 +709,129 @@ PipelineHandlerISI::PipelineHandlerISI(CameraManager *manager)\n> >  {\n> >  }\n> >\n> > +/*\n> > + * Generate a StreamConfiguration for YUV/RGB use case.\n> > + *\n> > + * Verify it the sensor can produce a YUV/RGB media bus format and collect\n> > + * all the processed pixel formats the ISI can generate as supported stream\n> > + * configurations.\n> > + */\n> > +StreamConfiguration PipelineHandlerISI::generateYUVConfiguration(Camera *camera,\n> > +\t\t\t\t\t\t\t\t const Size &size)\n> > +{\n> > +\tISICameraData *data = cameraData(camera);\n> > +\tPixelFormat pixelFormat = formats::YUYV;\n> > +\tunsigned int mbusCode;\n> > +\n> > +\tmbusCode = data->getYuvMediaBusFormat(&pixelFormat);\n> > +\tif (!mbusCode)\n> > +\t\treturn {};\n> > +\n> > +\t/* Adjust the requested size to the sensor's capabilities. */\n> > +\tconst CameraSensor *sensor = data->sensor_.get();\n> > +\n> > +\tV4L2SubdeviceFormat sensorFmt;\n> > +\tsensorFmt.mbus_code = mbusCode;\n> > +\tsensorFmt.size = size;\n> > +\n> > +\tint ret = sensor->tryFormat(&sensorFmt);\n>\n> \tint ret = data->sensor_->tryFormat(&sensorFmt);\n>\n> and drop the local sensor variable.\n>\n> > +\tif (ret) {\n> > +\t\tLOG(ISI, Error) << \"Failed to try sensor format.\";\n> > +\t\treturn {};\n> > +\t}\n> > +\n> > +\tSize sensorSize = sensorFmt.size;\n> > +\n> > +\t/*\n> > +\t * Populate the StreamConfiguration.\n> > +\t *\n> > +\t * As the sensor supports at least one YUV/RGB media bus format all the\n> > +\t * processed ones in formatsMap_ can be generated from it.\n> > +\t */\n> > +\tstd::map<PixelFormat, std::vector<SizeRange>> streamFormats;\n> > +\n> > +\tfor (const auto &[pixFmt, pipeFmt] : ISICameraConfiguration::formatsMap_) {\n> > +\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);\n> > +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tstreamFormats[pixFmt] = { { kMinISISize, sensorSize } };\n> > +\t}\n> > +\n> > +\tStreamFormats formats(streamFormats);\n> > +\n> > +\tStreamConfiguration cfg(formats);\n> > +\tcfg.pixelFormat = pixelFormat;\n> > +\tcfg.size = sensorSize;\n> > +\tcfg.bufferCount = 4;\n> > +\n> > +\treturn cfg;\n> > +}\n> > +\n> > +/*\n> > + * Generate a StreamConfiguration for Raw Bayer use case. Verify if the sensor\n> > + * can produce the requested RAW bayer format and eventually adjust it to\n> > + * the one with the largest bit-depth the sensor can produce.\n> > + */\n> > +StreamConfiguration PipelineHandlerISI::generateRawConfiguration(Camera *camera)\n> > +{\n> > +\tstatic const std::map<unsigned int, PixelFormat> rawFormats = {\n> > +\t\t{ MEDIA_BUS_FMT_SBGGR8_1X8, formats::SBGGR8 },\n> > +\t\t{ MEDIA_BUS_FMT_SGBRG8_1X8, formats::SGBRG8 },\n> > +\t\t{ MEDIA_BUS_FMT_SGRBG8_1X8, formats::SGRBG8 },\n> > +\t\t{ MEDIA_BUS_FMT_SRGGB8_1X8, formats::SRGGB8 },\n> > +\t\t{ MEDIA_BUS_FMT_SBGGR10_1X10, formats::SBGGR10 },\n> > +\t\t{ MEDIA_BUS_FMT_SGBRG10_1X10, formats::SGBRG10 },\n> > +\t\t{ MEDIA_BUS_FMT_SGRBG10_1X10, formats::SGRBG10 },\n> > +\t\t{ MEDIA_BUS_FMT_SRGGB10_1X10, formats::SRGGB10 },\n> > +\t\t{ MEDIA_BUS_FMT_SBGGR12_1X12, formats::SBGGR12 },\n> > +\t\t{ MEDIA_BUS_FMT_SGBRG12_1X12, formats::SGBRG12 },\n> > +\t\t{ MEDIA_BUS_FMT_SGRBG12_1X12, formats::SGRBG12 },\n> > +\t\t{ MEDIA_BUS_FMT_SRGGB12_1X12, formats::SRGGB12 },\n>\n> Now that libcamera has RAW14 formats, could you add them here ?\n>\n> > +\t};\n> > +\n> > +\tISICameraData *data = cameraData(camera);\n> > +\tPixelFormat pixelFormat = formats::SBGGR10;\n> > +\tunsigned int mbusCode;\n> > +\n> > +\t/* pixelFormat will be adjusted, if the sensor can produce RAW. */\n> > +\tmbusCode = data->getRawMediaBusFormat(&pixelFormat);\n> > +\tif (!mbusCode)\n> > +\t\treturn {};\n> > +\n> > +\t/*\n> > +\t * Populate the StreamConfiguration with all the supported Bayer\n> > +\t * formats the sensor can produce.\n> > +\t */\n> > +\tstd::map<PixelFormat, std::vector<SizeRange>> streamFormats;\n> > +\tconst CameraSensor *sensor = data->sensor_.get();\n> > +\n> > +\tfor (unsigned int code : sensor->mbusCodes()) {\n> > +\t\t/* Find a Bayer media bus code from the sensor. */\n> > +\t\tconst BayerFormat &bayerFormat = BayerFormat::fromMbusCode(code);\n> > +\t\tif (!bayerFormat.isValid())\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tauto it = rawFormats.find(code);\n> > +\t\tif (it == rawFormats.end()) {\n> > +\t\t\tLOG(ISI, Warning) << bayerFormat\n> > +\t\t\t\t\t  << \" not supported in ISI formats map.\";\n> > +\t\t\tcontinue;\n> > +\t\t}\n> > +\n> > +\t\tstreamFormats[it->second] = { { sensor->resolution(), sensor->resolution() } };\n> > +\t}\n> > +\n> > +\tStreamFormats formats(streamFormats);\n> > +\n> > +\tStreamConfiguration cfg(formats);\n> > +\tcfg.size = sensor->resolution();\n> > +\tcfg.pixelFormat = pixelFormat;\n> > +\tcfg.bufferCount = 4;\n> > +\n> > +\treturn cfg;\n> > +}\n> > +\n> >  std::unique_ptr<CameraConfiguration>\n> >  PipelineHandlerISI::generateConfiguration(Camera *camera,\n> >  \t\t\t\t\t  const StreamRoles &roles)\n> > @@ -722,79 +849,45 @@ PipelineHandlerISI::generateConfiguration(Camera *camera,\n> >  \t\treturn nullptr;\n> >  \t}\n> >\n> > -\tbool isRaw = false;\n> >  \tfor (const auto &role : roles) {\n> >  \t\t/*\n> > -\t\t * Prefer the following formats\n> > +\t\t * Prefer the following formats:\n> >  \t\t * - Still Capture: Full resolution YUYV\n> >  \t\t * - ViewFinder/VideoRecording: 1080p YUYV\n> > -\t\t * - RAW: sensor's native format and resolution\n> > +\t\t * - RAW: Full resolution Bayer\n> >  \t\t */\n> > -\t\tstd::map<PixelFormat, std::vector<SizeRange>> streamFormats;\n> > -\t\tPixelFormat pixelFormat;\n> > -\t\tSize size;\n> > +\t\tStreamConfiguration cfg;\n> >\n> >  \t\tswitch (role) {\n> >  \t\tcase StreamRole::StillCapture:\n> > -\t\t\t/*\n> > -\t\t\t * \\todo Make sure the sensor can produce non-RAW formats\n> > -\t\t\t * compatible with the ones supported by the pipeline.\n> > -\t\t\t */\n> > -\t\t\tsize = data->sensor_->resolution();\n> > -\t\t\tpixelFormat = formats::YUYV;\n> > +\t\t\tcfg = generateYUVConfiguration(camera,\n> > +\t\t\t\t\t\t       data->sensor_->resolution());\n> > +\t\t\tif (!cfg.pixelFormat.isValid()) {\n> > +\t\t\t\t/*\n> > +\t\t\t\t * Fallback to use a Bayer format if that's what\n> > +\t\t\t\t * the sensor supports.\n> > +\t\t\t\t */\n> > +\t\t\t\tcfg = generateRawConfiguration(camera);\n> > +\t\t\t}\n> > +\n> >  \t\t\tbreak;\n> >\n> >  \t\tcase StreamRole::Viewfinder:\n> >  \t\tcase StreamRole::VideoRecording:\n> > -\t\t\t/*\n> > -\t\t\t * \\todo Make sure the sensor can produce non-RAW formats\n> > -\t\t\t * compatible with the ones supported by the pipeline.\n> > -\t\t\t */\n> > -\t\t\tsize = PipelineHandlerISI::kPreviewSize;\n> > -\t\t\tpixelFormat = formats::YUYV;\n> > -\t\t\tbreak;\n> > -\n> > -\t\tcase StreamRole::Raw: {\n> > -\t\t\t/*\n> > -\t\t\t * Make sure the sensor can generate a RAW format and\n> > -\t\t\t * prefer the ones with a larger bitdepth.\n> > -\t\t\t */\n> > -\t\t\tconst ISICameraConfiguration::FormatMap::value_type *rawPipeFormat = nullptr;\n> > -\t\t\tunsigned int maxDepth = 0;\n> > -\n> > -\t\t\tfor (unsigned int code : data->sensor_->mbusCodes()) {\n> > -\t\t\t\tconst BayerFormat &bayerFormat = BayerFormat::fromMbusCode(code);\n> > -\t\t\t\tif (!bayerFormat.isValid())\n> > -\t\t\t\t\tcontinue;\n> > -\n> > -\t\t\t\t/* Make sure the format is supported by the pipeline handler. */\n> > -\t\t\t\tauto it = std::find_if(ISICameraConfiguration::formatsMap_.begin(),\n> > -\t\t\t\t\t\t       ISICameraConfiguration::formatsMap_.end(),\n> > -\t\t\t\t\t\t       [code](auto &isiFormat) {\n> > -\t\t\t\t\t\t\t        auto &pipe = isiFormat.second;\n> > -\t\t\t\t\t\t\t        return pipe.sensorCode == code;\n> > -\t\t\t\t\t\t       });\n> > -\t\t\t\tif (it == ISICameraConfiguration::formatsMap_.end())\n> > -\t\t\t\t\tcontinue;\n> > -\n> > -\t\t\t\tif (bayerFormat.bitDepth > maxDepth) {\n> > -\t\t\t\t\tmaxDepth = bayerFormat.bitDepth;\n> > -\t\t\t\t\trawPipeFormat = &(*it);\n> > -\t\t\t\t}\n> > -\t\t\t}\n> > -\n> > -\t\t\tif (!rawPipeFormat) {\n> > -\t\t\t\tLOG(ISI, Error)\n> > -\t\t\t\t\t<< \"Cannot generate a configuration for RAW stream\";\n> > -\t\t\t\treturn nullptr;\n> > +\t\t\tcfg = generateYUVConfiguration(camera,\n> > +\t\t\t\t\t\t       PipelineHandlerISI::kPreviewSize);\n> > +\t\t\tif (!cfg.pixelFormat.isValid()) {\n> > +\t\t\t\t/*\n> > +\t\t\t\t * Fallback to use a Bayer format if that's what\n> > +\t\t\t\t * the sensor supports.\n> > +\t\t\t\t */\n> > +\t\t\t\tcfg = generateRawConfiguration(camera);\n> >  \t\t\t}\n> >\n> > -\t\t\tsize = data->sensor_->resolution();\n> > -\t\t\tpixelFormat = rawPipeFormat->first;\n> > -\n> > -\t\t\tstreamFormats[pixelFormat] = { { kMinISISize, size } };\n> > -\t\t\tisRaw = true;\n> > +\t\t\tbreak;\n> >\n> > +\t\tcase StreamRole::Raw: {\n> > +\t\t\tcfg = generateRawConfiguration(camera);\n> >  \t\t\tbreak;\n> >  \t\t}\n> >\n>\n> Would this be clearer ?\n>\n> \t\tswitch (role) {\n> \t\tcase StreamRole::StillCapture:\n> \t\tcase StreamRole::Viewfinder:\n> \t\tcase StreamRole::VideoRecording:\n> \t\t\tSize size = role == StreamRole::StillCapture\n> \t\t\t\t  ? data->sensor_->resolution()\n> \t\t\t\t  : PipelineHandlerISI::kPreviewSize;\n> \t\t\tcfg = generateYUVConfiguration(camera, size);\n> \t\t\tif (cfg.pixelFormat.isValid())\n> \t\t\t\tbreak;\n>\n> \t\t\t/*\n> \t\t\t * Fallback to use a Bayer format if that's what the\n> \t\t\t * sensor supports.\n> \t\t\t */\n> \t\t\t[[fallthrough]]\n>\n> \t\tcase StreamRole::Raw: {\n> \t\t\tcfg = generateRawConfiguration(camera);\n> \t\t\tbreak;\n> \t\t}\n>\n> Up to you.\n>\n> > @@ -803,26 +896,12 @@ PipelineHandlerISI::generateConfiguration(Camera *camera,\n> >  \t\t\treturn nullptr;\n> >  \t\t}\n> >\n> > -\t\t/*\n> > -\t\t * For non-RAW configurations the ISI can perform colorspace\n> > -\t\t * conversion. List all the supported output formats here.\n> > -\t\t */\n> > -\t\tif (!isRaw) {\n> > -\t\t\tfor (const auto &[pixFmt, pipeFmt] : ISICameraConfiguration::formatsMap_) {\n> > -\t\t\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);\n> > -\t\t\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> > -\t\t\t\t\tcontinue;\n> > -\n> > -\t\t\t\tstreamFormats[pixFmt] = { { kMinISISize, size } };\n> > -\t\t\t}\n> > +\t\tif (!cfg.pixelFormat.isValid()) {\n> > +\t\t\tLOG(ISI, Error)\n> > +\t\t\t\t<< \"Cannot generate configuration for role: \" << role;\n> > +\t\t\treturn nullptr;\n> >  \t\t}\n> >\n> > -\t\tStreamFormats formats(streamFormats);\n> > -\n> > -\t\tStreamConfiguration cfg(formats);\n> > -\t\tcfg.pixelFormat = pixelFormat;\n> > -\t\tcfg.size = size;\n> > -\t\tcfg.bufferCount = 4;\n> >  \t\tconfig->addConfiguration(cfg);\n> >  \t}\n> >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 54EB2C3272\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 26 Apr 2023 13:00:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B9957627DF;\n\tWed, 26 Apr 2023 15:00:13 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D7642627D4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 26 Apr 2023 15:00:11 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3BF172D8;\n\tWed, 26 Apr 2023 15:00:00 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1682514013;\n\tbh=/n7ZGa4/6nKaTeuYa7ROpaALjEGd6iRhc+usbiNwLvY=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=J949yw4481CBXB53FcPhtU2j23kPsG9M/yPwuk1oDp2SauUcIPywa0h6X/sRbpxeC\n\tPNbSmYeolsiqc/KAFBuDM18M3RiiJb/kU1xIg3Y2svTcOWE+F+rnkMaBaL6sj8nYRb\n\tS6JLnfSS1LoZx6f3Iki9I0w0nm3HtCSa6Mk05CQRDFRhbvtTczdHunQ0xk/pIuyC7W\n\tEq79JeaSN9yxHI/+WdYCOCkg5UA21n+nNB+vUXLsQhfsHGGTi8J43MLdo5ILR+vgY+\n\tquBRrqbTh3frZuXi3vtLHsQ1ZBivlIrJB091QkqNtTADZnpAlFnaPSvL3FYU7VLJVe\n\t8Z4Ess3cGfIog==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1682514000;\n\tbh=/n7ZGa4/6nKaTeuYa7ROpaALjEGd6iRhc+usbiNwLvY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BNgKVkrat3EUZgQ6iO9cWZHJRCIKs/8FGeUsuXZcoSWOAdone4j6aMnqbD6liD5LE\n\tQsjLNewHg395Oab7AGb5LFWuYAq7BwD4S77X4S0jqZMKQZKKr8iC9cySrPmOQVzBwJ\n\tyzh/Bd08OUJsQS8SXrB7TvHHiT9Io0xDz9WLsQAQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"BNgKVkra\"; dkim-atps=neutral","Date":"Wed, 26 Apr 2023 15:00:08 +0200","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<2ckzybxn7lwfjz2qnqe74wetpyi4lk5xbow3y44whztm3aus3p@s3b3ckfyfzsg>","References":"<20230313091944.9530-1-jacopo.mondi@ideasonboard.com>\n\t<20230313091944.9530-6-jacopo.mondi@ideasonboard.com>\n\t<20230424060644.GA4926@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230424060644.GA4926@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 5/6] libcamera: imx8-isi: Split\n\tBayer/YUV config generation","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>","From":"Jacopo Mondi via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Cc":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]