[{"id":5383,"web_url":"https://patchwork.libcamera.org/comment/5383/","msgid":"<20200625012345.GP5980@pendragon.ideasonboard.com>","date":"2020-06-25T01:23:45","subject":"Re: [libcamera-devel] [PATCH v3 07/10] libcamera: ipu3: cio2: Add\n\tfunction to generate configuration","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 Tue, Jun 23, 2020 at 04:09:27AM +0200, Niklas Söderlund wrote:\n> Collect the code used to generate configurations for the CIO2 block in\n> the CIO2Device class. This allows simplifying the code and allow further\n> changes to only happen at one code location.\n\ns/ / /\n\n> Signed-off-by: Niklas Söderlund \n> ---\n> * Changes since v2\n> - Remove unneeded code to pick sensor FourCC.\n> - Remove desiredPixelFormat from generateConfiguration() as it's not\n> needed.\n> - Rename sensorFormat_ cio2Configuration_\n> - Consolidate all format information in a single table.\n> \n> * Changes since v1\n> - Use anonymous namespace instead of static for sensorMbusToPixel map.\n> - Handle case where the requested mbus code is not supported by the sensor.\n> - Update commit message.\n> ---\n> src/libcamera/pipeline/ipu3/cio2.cpp | 54 +++++++++++++++++++++++----\n> src/libcamera/pipeline/ipu3/cio2.h | 3 ++\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 56 +++++++---------------------\n> 3 files changed, 63 insertions(+), 50 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> index f23128d412e6b1a5..d6bab896706dd60e 100644\n> --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> @@ -9,6 +9,9 @@\n> \n> #include \n> \n> +#include \n> +#include \n> +\n> #include \"libcamera/internal/camera_sensor.h\"\n> #include \"libcamera/internal/media_device.h\"\n> #include \"libcamera/internal/v4l2_subdevice.h\"\n> @@ -20,11 +23,16 @@ LOG_DECLARE_CATEGORY(IPU3)\n> \n> namespace {\n> \n> -static const std::map mbusCodesToInfo = {\n> -\t{ MEDIA_BUS_FMT_SBGGR10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10) },\n> -\t{ MEDIA_BUS_FMT_SGBRG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10) },\n> -\t{ MEDIA_BUS_FMT_SGRBG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10) },\n> -\t{ MEDIA_BUS_FMT_SRGGB10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10) },\n> +struct MbusInfo {\n> +\tPixelFormat pixelFormat;\n> +\tV4L2PixelFormat fourcc;\n> +};\n> +\n> +static const std::map mbusCodesToInfo = {\n> +\t{ MEDIA_BUS_FMT_SBGGR10_1X10, { formats::SBGGR10_IPU3, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10) } },\n> +\t{ MEDIA_BUS_FMT_SGBRG10_1X10, { formats::SGBRG10_IPU3, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10) } },\n> +\t{ MEDIA_BUS_FMT_SGRBG10_1X10, { formats::SGRBG10_IPU3, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10) } },\n> +\t{ MEDIA_BUS_FMT_SRGGB10_1X10, { formats::SRGGB10_IPU3, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10) } },\n> };\n\nWould it make sense to keep this as-is, and use PixelFormatInfo::info()\nto look up additional information (the V4L2 pixel format in this case,\nbut potentially more in the future) ? Otherwise we end up duplicating\nformat information in multiple locations.\n\n> \n> } /* namespace */\n> @@ -92,7 +100,7 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)\n> \tstd::set cio2Codes;\n> \tstd::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),\n> \t\tstd::inserter(cio2Codes, cio2Codes.begin()),\n> -\t\t[](const std::map::value_type &pair){ return pair.first; });\n> +\t\t[](const std::map::value_type &pair){ return pair.first; });\n> \tconst std::set &sensorCodes = sensor_->mbusCodes();\n> \tif (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(),\n> \t\t\t\tcio2Codes.begin(), cio2Codes.end())) {\n> @@ -139,7 +147,7 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)\n> \tstd::vector mbusCodes;\n> \tstd::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),\n> \t\tstd::back_inserter(mbusCodes),\n> -\t\t[](const std::map::value_type &pair){ return pair.first; });\n> +\t\t[](const std::map::value_type &pair){ return pair.first; });\n> \n> \tsensorFormat = sensor_->getFormat(mbusCodes, size);\n> \tret = sensor_->setFormat(&sensorFormat);\n> @@ -154,7 +162,7 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)\n> \tif (itInfo == mbusCodesToInfo.end())\n> \t\treturn -EINVAL;\n> \n> -\toutputFormat->fourcc = itInfo->second;\n> +\toutputFormat->fourcc = itInfo->second.fourcc;\n\n\nThis would become\n\n\tconst PixelFormatInfo &info = PixelFormatInfo::info(itInfo->second);\n\toutputFormat->fourcc = info.v4l2Format;\n\n> \toutputFormat->size = sensorFormat.size;\n> \toutputFormat->planesCount = 1;\n> \n> @@ -167,6 +175,36 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)\n> \treturn 0;\n> }\n> \n> +StreamConfiguration\n> +CIO2Device::generateConfiguration(const Size &desiredSize) const\n> +{\n> +\tStreamConfiguration cfg;\n> +\n> +\t/* If no desired size use the sensor resolution. */\n> +\tSize size = sensor_->resolution();\n> +\tif (desiredSize.width && desiredSize.height)\n> +\t\tsize = desiredSize;\n\nI've just sent a patch titled \"PATCH] libcamera: geometry: Add isNull()\nfunction to Size class\". You could write this code\n\n\tSize size = !desiredSize.isNull() ? desiredSize : sensor_->resolution();\n\nOr you could pass the desiredSize parameter by value instead of\nreference, rename it to size, and just write\n\n\t/* If no desired size use the sensor resolution. */\n\tif (size.isNull())\n\t\tsize = sensor_->resolution();\n\nI think that would be the simplest to read option.\n\n> +\n> +\t/* Query the sensor static information for closest match. */\n> +\tstd::vector mbusCodes;\n> +\tstd::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),\n> +\t\tstd::back_inserter(mbusCodes),\n> +\t\t[](const std::map::value_type &pair){ return pair.first; });\n\nI've submitted \"[PATCH] libcamera: utils: Add map_keys() function\", only\nto then realize that here you're storing mbus codes in a vector, not a\nset. I have a bit of a feeling that the usage of different types is a\nsign that the API isn't consistent. And modifying\nCameraSensor::getFormat() to take a set would likely require more\nreplacement of vectors with sets, which I'm not sure is a good idea (I\nhaven't checked how far it would spread).\n\nIn any case, if we decide to keep vectors, patch \"[PATCH] libcamera:\nutils: Add map_keys() function\" should be changed to return a vector, I\nthink it would still be useful.\n\n> +\n> +\tV4L2SubdeviceFormat sensorFormat = sensor_->getFormat(mbusCodes, size);\n> +\n> +\tif (!sensorFormat.mbus_code) {\n> +\t\tLOG(IPU3, Error) << \"Sensor does not support mbus code\";\n> +\t\treturn {};\n> +\t}\n> +\n> +\tcfg.size = sensorFormat.size;\n> +\tcfg.pixelFormat = mbusCodesToInfo.at(sensorFormat.mbus_code).pixelFormat;\n> +\tcfg.bufferCount = CIO2_BUFFER_COUNT;\n> +\n> +\treturn cfg;\n> +}\n> +\n> /**\n> * \\brief Allocate frame buffers for the CIO2 output\n> *\n> diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h\n> index b2c4f89d682d6cfb..6276573f2b585b25 100644\n> --- a/src/libcamera/pipeline/ipu3/cio2.h\n> +++ b/src/libcamera/pipeline/ipu3/cio2.h\n> @@ -20,6 +20,7 @@ class V4L2DeviceFormat;\n> class V4L2Subdevice;\n> class V4L2VideoDevice;\n> struct Size;\n> +struct StreamConfiguration;\n> \n> class CIO2Device\n> {\n> @@ -32,6 +33,8 @@ public:\n> \tint init(const MediaDevice *media, unsigned int index);\n> \tint configure(const Size &size, V4L2DeviceFormat *outputFormat);\n> \n> +\tStreamConfiguration generateConfiguration(const Size &desiredSize) const;\n> +\n> \tint allocateBuffers();\n> \tvoid freeBuffers();\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 6e5eb5609a3c2388..c0e727e592f46883 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -36,13 +36,6 @@ LOG_DEFINE_CATEGORY(IPU3)\n> \n> class IPU3CameraData;\n> \n> -static const std::map sensorMbusToPixel = {\n> -\t{ MEDIA_BUS_FMT_SBGGR10_1X10, formats::SBGGR10_IPU3 },\n> -\t{ MEDIA_BUS_FMT_SGBRG10_1X10, formats::SGBRG10_IPU3 },\n> -\t{ MEDIA_BUS_FMT_SGRBG10_1X10, formats::SGRBG10_IPU3 },\n> -\t{ MEDIA_BUS_FMT_SRGGB10_1X10, formats::SRGGB10_IPU3 },\n> -};\n> -\n> class ImgUDevice\n> {\n> public:\n> @@ -147,7 +140,7 @@ public:\n> \n> \tStatus validate() override;\n> \n> -\tconst V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }\n> +\tconst StreamConfiguration &cio2Format() const { return cio2Configuration_; };\n> \tconst std::vector &streams() { return streams_; }\n> \n> private:\n> @@ -165,7 +158,7 @@ private:\n> \tstd::shared_ptr camera_;\n> \tconst IPU3CameraData *data_;\n> \n> -\tV4L2SubdeviceFormat sensorFormat_;\n> +\tStreamConfiguration cio2Configuration_;\n\nThis may still be an abuse of StreamConfiguration, but I think we can\nlive with it for now until further refactoring.\n\n> \tstd::vector streams_;\n> };\n> \n> @@ -252,7 +245,7 @@ void IPU3CameraConfiguration::assignStreams()\n> \n> \t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> \t\t\tstream = &data_->rawStream_;\n> -\t\telse if (cfg.size == sensorFormat_.size)\n> +\t\telse if (cfg.size == cio2Configuration_.size)\n> \t\t\tstream = &data_->outStream_;\n> \t\telse\n> \t\t\tstream = &data_->vfStream_;\n> @@ -277,8 +270,8 @@ void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)\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 * sensorFormat_.size.height\n> -\t\t\t\t\t/ sensorFormat_.size.width;\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> @@ -297,7 +290,7 @@ void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)\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 = sensorFormat_.size;\n> +\t\tcfg.size = cio2Configuration_.size;\n> \t}\n> \n> \t/*\n> @@ -313,7 +306,6 @@ void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)\n> \n> CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> {\n> -\tconst CameraSensor *sensor = data_->cio2_.sensor_;\n> \tStatus status = Valid;\n> \n> \tif (config_.empty())\n> @@ -340,16 +332,10 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> \t\t\tsize.height = cfg.size.height;\n> \t}\n> \n> -\tif (!size.width || !size.height)\n> -\t\tsize = sensor->resolution();\n> -\n> -\tsensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10,\n> -\t\t\t\t\t MEDIA_BUS_FMT_SGBRG10_1X10,\n> -\t\t\t\t\t MEDIA_BUS_FMT_SGRBG10_1X10,\n> -\t\t\t\t\t MEDIA_BUS_FMT_SRGGB10_1X10 },\n> -\t\t\t\t\t size);\n> -\tif (!sensorFormat_.size.width || !sensorFormat_.size.height)\n> -\t\tsensorFormat_.size = sensor->resolution();\n> +\t/* Generate raw configuration from CIO2. */\n> +\tcio2Configuration_ = data_->cio2_.generateConfiguration(size);\n> +\tif (!cio2Configuration_.pixelFormat.isValid())\n> +\t\treturn Invalid;\n> \n> \t/* Assign streams to each configuration entry. */\n> \tassignStreams();\n> @@ -361,20 +347,13 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> \t\tconst IPU3Stream *stream = streams_[i];\n> \n> \t\tif (stream->raw_) {\n> -\t\t\tconst auto &itFormat =\n> -\t\t\t\tsensorMbusToPixel.find(sensorFormat_.mbus_code);\n> -\t\t\tif (itFormat == sensorMbusToPixel.end())\n> -\t\t\t\treturn Invalid;\n> -\n> -\t\t\tcfg.pixelFormat = itFormat->second;\n> -\t\t\tcfg.size = sensorFormat_.size;\n> +\t\t\tcfg = cio2Configuration_;\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}\n> \n> -\t\tcfg.bufferCount = IPU3_BUFFER_COUNT;\n> -\n> \t\tif (cfg.pixelFormat != oldCfg.pixelFormat ||\n> \t\t cfg.size != oldCfg.size) {\n> \t\t\tLOG(IPU3, Debug)\n> @@ -454,14 +433,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> \n> \t\t\tcfg.size = data->cio2_.sensor_->resolution();\n> \n> -\t\t\tV4L2SubdeviceFormat sensorFormat =\n> -\t\t\t\tdata->cio2_.sensor_->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10,\n> -\t\t\t\t\t\t\t\t MEDIA_BUS_FMT_SGBRG10_1X10,\n> -\t\t\t\t\t\t\t\t MEDIA_BUS_FMT_SGRBG10_1X10,\n> -\t\t\t\t\t\t\t\t MEDIA_BUS_FMT_SRGGB10_1X10 },\n> -\t\t\t\t\t\t\t cfg.size);\n> -\t\t\tcfg.pixelFormat =\n> -\t\t\t\tsensorMbusToPixel.at(sensorFormat.mbus_code);\n> +\t\t\tcfg = data->cio2_.generateConfiguration(cfg.size);\n> \t\t\tbreak;\n> \t\t}\n> \n> @@ -575,7 +547,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> \t * Pass the requested stream size to the CIO2 unit and get back the\n> \t * adjusted format to be propagated to the ImgU output devices.\n> \t */\n> -\tconst Size &sensorSize = config->sensorFormat().size;\n> +\tconst Size &sensorSize = config->cio2Format().size;\n> \tV4L2DeviceFormat cio2Format = {};\n> \tret = cio2->configure(sensorSize, &cio2Format);\n> \tif (ret)\n\nWith the above issues addressed,\n\nReviewed-by: Laurent Pinchart \n\nOverall I think this is a nice cleanup, even if there's room for\nimprovement (mainly addressing the possible abuse of\nStreamConfiguration, and the std::vector vs. std::set issue, the latter\nnot being specific to this patch).","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 CBA03C0100\n\tfor ;\n\tThu, 25 Jun 2020 01:23:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9A8F4609C3;\n\tThu, 25 Jun 2020 03:23:48 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1D84B603BB\n\tfor ;\n\tThu, 25 Jun 2020 03:23:47 +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 A217C2A8;\n\tThu, 25 Jun 2020 03:23:46 +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=\"he2KMMd1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593048226;\n\tbh=xNdrbql50yKUUb9+cJSy0+e7mpfqAihootEwwCaOnPw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=he2KMMd1H1siOwkKYsP0Vi5k1/zAzSPkCG4qwkVD5gcDrd3OFazl4y1whVgbX01Nl\n\tZ6zjpATmu1Gtx7NX758zANxs2XqQygnYLA9ivxmue068jFMRO2F70poa5ye/DgMm9o\n\tAAOJm56e1s3HNfj/AUlVN1gq6lWuFUhr8Yt/3pYw=","Date":"Thu, 25 Jun 2020 04:23:45 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200625012345.GP5980@pendragon.ideasonboard.com>","References":"<20200623020930.1781469-1-niklas.soderlund@ragnatech.se>\n\t<20200623020930.1781469-8-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200623020930.1781469-8-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v3 07/10] libcamera: ipu3: cio2: Add\n\tfunction to generate configuration","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":5385,"web_url":"https://patchwork.libcamera.org/comment/5385/","msgid":"<20200625012543.GQ5980@pendragon.ideasonboard.com>","date":"2020-06-25T01:25:43","subject":"Re: [libcamera-devel] [PATCH v3 07/10] libcamera: ipu3: cio2: Add\n\tfunction to generate configuration","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Jun 24, 2020 at 12:22:01PM +0200, Jacopo Mondi wrote:\n> On Tue, Jun 23, 2020 at 04:09:27AM +0200, Niklas Söderlund wrote:\n> > Collect the code used to generate configurations for the CIO2 block in\n> > the CIO2Device class. This allows simplifying the code and allow further\n> > changes to only happen at one code location.\n> >\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > * Changes since v2\n> > - Remove unneeded code to pick sensor FourCC.\n> > - Remove desiredPixelFormat from generateConfiguration() as it's not\n> > needed.\n> > - Rename sensorFormat_ cio2Configuration_\n> > - Consolidate all format information in a single table.\n> >\n> > * Changes since v1\n> > - Use anonymous namespace instead of static for sensorMbusToPixel map.\n> > - Handle case where the requested mbus code is not supported by the sensor.\n> > - Update commit message.\n> > ---\n> > src/libcamera/pipeline/ipu3/cio2.cpp | 54 +++++++++++++++++++++++----\n> > src/libcamera/pipeline/ipu3/cio2.h | 3 ++\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 56 +++++++---------------------\n> > 3 files changed, 63 insertions(+), 50 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > index f23128d412e6b1a5..d6bab896706dd60e 100644\n> > --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > @@ -9,6 +9,9 @@\n> >\n> > #include \n> >\n> > +#include \n> > +#include \n> > +\n> > #include \"libcamera/internal/camera_sensor.h\"\n> > #include \"libcamera/internal/media_device.h\"\n> > #include \"libcamera/internal/v4l2_subdevice.h\"\n> > @@ -20,11 +23,16 @@ LOG_DECLARE_CATEGORY(IPU3)\n> >\n> > namespace {\n> >\n> > -static const std::map mbusCodesToInfo = {\n> > -\t{ MEDIA_BUS_FMT_SBGGR10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10) },\n> > -\t{ MEDIA_BUS_FMT_SGBRG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10) },\n> > -\t{ MEDIA_BUS_FMT_SGRBG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10) },\n> > -\t{ MEDIA_BUS_FMT_SRGGB10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10) },\n> > +struct MbusInfo {\n> > +\tPixelFormat pixelFormat;\n> > +\tV4L2PixelFormat fourcc;\n> > +};\n> > +\n> > +static const std::map mbusCodesToInfo = {\n> > +\t{ MEDIA_BUS_FMT_SBGGR10_1X10, { formats::SBGGR10_IPU3, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10) } },\n> > +\t{ MEDIA_BUS_FMT_SGBRG10_1X10, { formats::SGBRG10_IPU3, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10) } },\n> > +\t{ MEDIA_BUS_FMT_SGRBG10_1X10, { formats::SGRBG10_IPU3, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10) } },\n> > +\t{ MEDIA_BUS_FMT_SRGGB10_1X10, { formats::SRGGB10_IPU3, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10) } },\n> > };\n> >\n> > } /* namespace */\n> > @@ -92,7 +100,7 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)\n> > \tstd::set cio2Codes;\n> > \tstd::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),\n> > \t\tstd::inserter(cio2Codes, cio2Codes.begin()),\n> > -\t\t[](const std::map::value_type &pair){ return pair.first; });\n> > +\t\t[](const std::map::value_type &pair){ return pair.first; });\n> > \tconst std::set &sensorCodes = sensor_->mbusCodes();\n> > \tif (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(),\n> > \t\t\t\tcio2Codes.begin(), cio2Codes.end())) {\n> > @@ -139,7 +147,7 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)\n> > \tstd::vector mbusCodes;\n> > \tstd::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),\n> > \t\tstd::back_inserter(mbusCodes),\n> > -\t\t[](const std::map::value_type &pair){ return pair.first; });\n> > +\t\t[](const std::map::value_type &pair){ return pair.first; });\n> >\n> > \tsensorFormat = sensor_->getFormat(mbusCodes, size);\n> > \tret = sensor_->setFormat(&sensorFormat);\n> > @@ -154,7 +162,7 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)\n> > \tif (itInfo == mbusCodesToInfo.end())\n> > \t\treturn -EINVAL;\n> >\n> > -\toutputFormat->fourcc = itInfo->second;\n> > +\toutputFormat->fourcc = itInfo->second.fourcc;\n> > \toutputFormat->size = sensorFormat.size;\n> > \toutputFormat->planesCount = 1;\n> >\n> > @@ -167,6 +175,36 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)\n> > \treturn 0;\n> > }\n> >\n> > +StreamConfiguration\n> > +CIO2Device::generateConfiguration(const Size &desiredSize) const\n> > +{\n> > +\tStreamConfiguration cfg;\n> > +\n> > +\t/* If no desired size use the sensor resolution. */\n> > +\tSize size = sensor_->resolution();\n> > +\tif (desiredSize.width && desiredSize.height)\n> > +\t\tsize = desiredSize;\n> > +\n> > +\t/* Query the sensor static information for closest match. */\n> > +\tstd::vector mbusCodes;\n> > +\tstd::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),\n> > +\t\tstd::back_inserter(mbusCodes),\n> > +\t\t[](const std::map::value_type &pair){ return pair.first; });\n> \n> Oh, one more :(\n> \n> What about a CIO2Device::mbusCodes() helper for this ? With C++ copy\n> elision you would not get any performance penality\n\nSee \"[PATCH] libcamera: utils: Add map_keys() function\" :-) We however\nhave to decide if the function should return a vector or a set.\n\n> > +\n> > +\tV4L2SubdeviceFormat sensorFormat = sensor_->getFormat(mbusCodes, size);\n> > +\n> \n> stray empty line\n> \n> The rest is good. I think we're abusing StreamConfiguration, as it is\n> supposed to report StreamFormats, but it's optional, and we use it\n\nI wonder what's wrong, we tend to agree a lot lately :-)\n\n> internally to transport plaing \"formats\" between components. You could\n> even use an ImageFormat if my series ever gets any feedback, but I\n> won't block this one because of this. But really, we need to fix this\n> as it's getting confusing enough :)\n> \n> Reviewed-by: Jacopo Mondi \n> \n> > +\tif (!sensorFormat.mbus_code) {\n> > +\t\tLOG(IPU3, Error) << \"Sensor does not support mbus code\";\n> > +\t\treturn {};\n> > +\t}\n> > +\n> > +\tcfg.size = sensorFormat.size;\n> > +\tcfg.pixelFormat = mbusCodesToInfo.at(sensorFormat.mbus_code).pixelFormat;\n> > +\tcfg.bufferCount = CIO2_BUFFER_COUNT;\n> > +\n> > +\treturn cfg;\n> > +}\n> > +\n> > /**\n> > * \\brief Allocate frame buffers for the CIO2 output\n> > *\n> > diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h\n> > index b2c4f89d682d6cfb..6276573f2b585b25 100644\n> > --- a/src/libcamera/pipeline/ipu3/cio2.h\n> > +++ b/src/libcamera/pipeline/ipu3/cio2.h\n> > @@ -20,6 +20,7 @@ class V4L2DeviceFormat;\n> > class V4L2Subdevice;\n> > class V4L2VideoDevice;\n> > struct Size;\n> > +struct StreamConfiguration;\n> >\n> > class CIO2Device\n> > {\n> > @@ -32,6 +33,8 @@ public:\n> > \tint init(const MediaDevice *media, unsigned int index);\n> > \tint configure(const Size &size, V4L2DeviceFormat *outputFormat);\n> >\n> > +\tStreamConfiguration generateConfiguration(const Size &desiredSize) const;\n> > +\n> > \tint allocateBuffers();\n> > \tvoid freeBuffers();\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 6e5eb5609a3c2388..c0e727e592f46883 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -36,13 +36,6 @@ LOG_DEFINE_CATEGORY(IPU3)\n> >\n> > class IPU3CameraData;\n> >\n> > -static const std::map sensorMbusToPixel = {\n> > -\t{ MEDIA_BUS_FMT_SBGGR10_1X10, formats::SBGGR10_IPU3 },\n> > -\t{ MEDIA_BUS_FMT_SGBRG10_1X10, formats::SGBRG10_IPU3 },\n> > -\t{ MEDIA_BUS_FMT_SGRBG10_1X10, formats::SGRBG10_IPU3 },\n> > -\t{ MEDIA_BUS_FMT_SRGGB10_1X10, formats::SRGGB10_IPU3 },\n> > -};\n> > -\n> > class ImgUDevice\n> > {\n> > public:\n> > @@ -147,7 +140,7 @@ public:\n> >\n> > \tStatus validate() override;\n> >\n> > -\tconst V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }\n> > +\tconst StreamConfiguration &cio2Format() const { return cio2Configuration_; };\n> > \tconst std::vector &streams() { return streams_; }\n> >\n> > private:\n> > @@ -165,7 +158,7 @@ private:\n> > \tstd::shared_ptr camera_;\n> > \tconst IPU3CameraData *data_;\n> >\n> > -\tV4L2SubdeviceFormat sensorFormat_;\n> > +\tStreamConfiguration cio2Configuration_;\n> > \tstd::vector streams_;\n> > };\n> >\n> > @@ -252,7 +245,7 @@ void IPU3CameraConfiguration::assignStreams()\n> >\n> > \t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> > \t\t\tstream = &data_->rawStream_;\n> > -\t\telse if (cfg.size == sensorFormat_.size)\n> > +\t\telse if (cfg.size == cio2Configuration_.size)\n> > \t\t\tstream = &data_->outStream_;\n> > \t\telse\n> > \t\t\tstream = &data_->vfStream_;\n> > @@ -277,8 +270,8 @@ void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)\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 * sensorFormat_.size.height\n> > -\t\t\t\t\t/ sensorFormat_.size.width;\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> > @@ -297,7 +290,7 @@ void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)\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 = sensorFormat_.size;\n> > +\t\tcfg.size = cio2Configuration_.size;\n> > \t}\n> >\n> > \t/*\n> > @@ -313,7 +306,6 @@ void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)\n> >\n> > CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > {\n> > -\tconst CameraSensor *sensor = data_->cio2_.sensor_;\n> > \tStatus status = Valid;\n> >\n> > \tif (config_.empty())\n> > @@ -340,16 +332,10 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > \t\t\tsize.height = cfg.size.height;\n> > \t}\n> >\n> > -\tif (!size.width || !size.height)\n> > -\t\tsize = sensor->resolution();\n> > -\n> > -\tsensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10,\n> > -\t\t\t\t\t MEDIA_BUS_FMT_SGBRG10_1X10,\n> > -\t\t\t\t\t MEDIA_BUS_FMT_SGRBG10_1X10,\n> > -\t\t\t\t\t MEDIA_BUS_FMT_SRGGB10_1X10 },\n> > -\t\t\t\t\t size);\n> > -\tif (!sensorFormat_.size.width || !sensorFormat_.size.height)\n> > -\t\tsensorFormat_.size = sensor->resolution();\n> > +\t/* Generate raw configuration from CIO2. */\n> > +\tcio2Configuration_ = data_->cio2_.generateConfiguration(size);\n> > +\tif (!cio2Configuration_.pixelFormat.isValid())\n> > +\t\treturn Invalid;\n> >\n> > \t/* Assign streams to each configuration entry. */\n> > \tassignStreams();\n> > @@ -361,20 +347,13 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > \t\tconst IPU3Stream *stream = streams_[i];\n> >\n> > \t\tif (stream->raw_) {\n> > -\t\t\tconst auto &itFormat =\n> > -\t\t\t\tsensorMbusToPixel.find(sensorFormat_.mbus_code);\n> > -\t\t\tif (itFormat == sensorMbusToPixel.end())\n> > -\t\t\t\treturn Invalid;\n> > -\n> > -\t\t\tcfg.pixelFormat = itFormat->second;\n> > -\t\t\tcfg.size = sensorFormat_.size;\n> > +\t\t\tcfg = cio2Configuration_;\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}\n> >\n> > -\t\tcfg.bufferCount = IPU3_BUFFER_COUNT;\n> > -\n> > \t\tif (cfg.pixelFormat != oldCfg.pixelFormat ||\n> > \t\t cfg.size != oldCfg.size) {\n> > \t\t\tLOG(IPU3, Debug)\n> > @@ -454,14 +433,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> >\n> > \t\t\tcfg.size = data->cio2_.sensor_->resolution();\n> >\n> > -\t\t\tV4L2SubdeviceFormat sensorFormat =\n> > -\t\t\t\tdata->cio2_.sensor_->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10,\n> > -\t\t\t\t\t\t\t\t MEDIA_BUS_FMT_SGBRG10_1X10,\n> > -\t\t\t\t\t\t\t\t MEDIA_BUS_FMT_SGRBG10_1X10,\n> > -\t\t\t\t\t\t\t\t MEDIA_BUS_FMT_SRGGB10_1X10 },\n> > -\t\t\t\t\t\t\t cfg.size);\n> > -\t\t\tcfg.pixelFormat =\n> > -\t\t\t\tsensorMbusToPixel.at(sensorFormat.mbus_code);\n> > +\t\t\tcfg = data->cio2_.generateConfiguration(cfg.size);\n> > \t\t\tbreak;\n> > \t\t}\n> >\n> > @@ -575,7 +547,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> > \t * Pass the requested stream size to the CIO2 unit and get back the\n> > \t * adjusted format to be propagated to the ImgU output devices.\n> > \t */\n> > -\tconst Size &sensorSize = config->sensorFormat().size;\n> > +\tconst Size &sensorSize = config->cio2Format().size;\n> > \tV4L2DeviceFormat cio2Format = {};\n> > \tret = cio2->configure(sensorSize, &cio2Format);\n> > \tif (ret)\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\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 0E74EC0101\n\tfor ;\n\tThu, 25 Jun 2020 01:25:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9C872609A5;\n\tThu, 25 Jun 2020 03:25:52 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AA170603BB\n\tfor ;\n\tThu, 25 Jun 2020 03:25:51 +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 266492A8;\n\tThu, 25 Jun 2020 03:25:51 +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=\"lkY9jH+F\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593048351;\n\tbh=OI9ILMK5etjE7HU83qXRFufDoyLNfYBi1hw4mfQLTx4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lkY9jH+F7oa3MlrOTMKWnxHGGGDuHW359YXmS8esMMcywALzStqv3kpzeVsMv2/HQ\n\tZHGvqFhJRBKLq2YYial5GivlxIRw3L+5umQXi8ecsVKYlDnW8tfzhu8p7UcCaoTYN7\n\tk3P4hVnyz2wxxtbhIPv07Oy+6YBy/1a4Q5eMo3lk=","Date":"Thu, 25 Jun 2020 04:25:43 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20200625012543.GQ5980@pendragon.ideasonboard.com>","References":"<20200623020930.1781469-1-niklas.soderlund@ragnatech.se>\n\t<20200623020930.1781469-8-niklas.soderlund@ragnatech.se>\n\t<20200624102201.26dhczlklqrxcxd3@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200624102201.26dhczlklqrxcxd3@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 07/10] libcamera: ipu3: cio2: Add\n\tfunction to generate configuration","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":10798,"web_url":"https://patchwork.libcamera.org/comment/10798/","msgid":"<20200624102201.26dhczlklqrxcxd3@uno.localdomain>","date":"2020-06-24T10:22:01","subject":"Re: [libcamera-devel] [PATCH v3 07/10] libcamera: ipu3: cio2: Add\n\tfunction to generate configuration","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Tue, Jun 23, 2020 at 04:09:27AM +0200, Niklas Söderlund wrote:\n> Collect the code used to generate configurations for the CIO2 block in\n> the CIO2Device class. This allows simplifying the code and allow further\n> changes to only happen at one code location.\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> * Changes since v2\n> - Remove unneeded code to pick sensor FourCC.\n> - Remove desiredPixelFormat from generateConfiguration() as it's not\n> needed.\n> - Rename sensorFormat_ cio2Configuration_\n> - Consolidate all format information in a single table.\n>\n> * Changes since v1\n> - Use anonymous namespace instead of static for sensorMbusToPixel map.\n> - Handle case where the requested mbus code is not supported by the sensor.\n> - Update commit message.\n> ---\n> src/libcamera/pipeline/ipu3/cio2.cpp | 54 +++++++++++++++++++++++----\n> src/libcamera/pipeline/ipu3/cio2.h | 3 ++\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 56 +++++++---------------------\n> 3 files changed, 63 insertions(+), 50 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> index f23128d412e6b1a5..d6bab896706dd60e 100644\n> --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> @@ -9,6 +9,9 @@\n>\n> #include \n>\n> +#include \n> +#include \n> +\n> #include \"libcamera/internal/camera_sensor.h\"\n> #include \"libcamera/internal/media_device.h\"\n> #include \"libcamera/internal/v4l2_subdevice.h\"\n> @@ -20,11 +23,16 @@ LOG_DECLARE_CATEGORY(IPU3)\n>\n> namespace {\n>\n> -static const std::map mbusCodesToInfo = {\n> -\t{ MEDIA_BUS_FMT_SBGGR10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10) },\n> -\t{ MEDIA_BUS_FMT_SGBRG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10) },\n> -\t{ MEDIA_BUS_FMT_SGRBG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10) },\n> -\t{ MEDIA_BUS_FMT_SRGGB10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10) },\n> +struct MbusInfo {\n> +\tPixelFormat pixelFormat;\n> +\tV4L2PixelFormat fourcc;\n> +};\n> +\n> +static const std::map mbusCodesToInfo = {\n> +\t{ MEDIA_BUS_FMT_SBGGR10_1X10, { formats::SBGGR10_IPU3, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10) } },\n> +\t{ MEDIA_BUS_FMT_SGBRG10_1X10, { formats::SGBRG10_IPU3, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10) } },\n> +\t{ MEDIA_BUS_FMT_SGRBG10_1X10, { formats::SGRBG10_IPU3, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10) } },\n> +\t{ MEDIA_BUS_FMT_SRGGB10_1X10, { formats::SRGGB10_IPU3, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10) } },\n> };\n>\n> } /* namespace */\n> @@ -92,7 +100,7 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)\n> \tstd::set cio2Codes;\n> \tstd::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),\n> \t\tstd::inserter(cio2Codes, cio2Codes.begin()),\n> -\t\t[](const std::map::value_type &pair){ return pair.first; });\n> +\t\t[](const std::map::value_type &pair){ return pair.first; });\n> \tconst std::set &sensorCodes = sensor_->mbusCodes();\n> \tif (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(),\n> \t\t\t\tcio2Codes.begin(), cio2Codes.end())) {\n> @@ -139,7 +147,7 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)\n> \tstd::vector mbusCodes;\n> \tstd::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),\n> \t\tstd::back_inserter(mbusCodes),\n> -\t\t[](const std::map::value_type &pair){ return pair.first; });\n> +\t\t[](const std::map::value_type &pair){ return pair.first; });\n>\n> \tsensorFormat = sensor_->getFormat(mbusCodes, size);\n> \tret = sensor_->setFormat(&sensorFormat);\n> @@ -154,7 +162,7 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)\n> \tif (itInfo == mbusCodesToInfo.end())\n> \t\treturn -EINVAL;\n>\n> -\toutputFormat->fourcc = itInfo->second;\n> +\toutputFormat->fourcc = itInfo->second.fourcc;\n> \toutputFormat->size = sensorFormat.size;\n> \toutputFormat->planesCount = 1;\n>\n> @@ -167,6 +175,36 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)\n> \treturn 0;\n> }\n>\n> +StreamConfiguration\n> +CIO2Device::generateConfiguration(const Size &desiredSize) const\n> +{\n> +\tStreamConfiguration cfg;\n> +\n> +\t/* If no desired size use the sensor resolution. */\n> +\tSize size = sensor_->resolution();\n> +\tif (desiredSize.width && desiredSize.height)\n> +\t\tsize = desiredSize;\n> +\n> +\t/* Query the sensor static information for closest match. */\n> +\tstd::vector mbusCodes;\n> +\tstd::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),\n> +\t\tstd::back_inserter(mbusCodes),\n> +\t\t[](const std::map::value_type &pair){ return pair.first; });\n\nOh, one more :(\n\nWhat about a CIO2Device::mbusCodes() helper for this ? With C++ copy\nelision you would not get any performance penality\n\n> +\n> +\tV4L2SubdeviceFormat sensorFormat = sensor_->getFormat(mbusCodes, size);\n> +\n\nstray empty line\n\nThe rest is good. I think we're abusing StreamConfiguration, as it is\nsupposed to report StreamFormats, but it's optional, and we use it\ninternally to transport plaing \"formats\" between components. You could\neven use an ImageFormat if my series ever gets any feedback, but I\nwon't block this one because of this. But really, we need to fix this\nas it's getting confusing enough :)\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> +\tif (!sensorFormat.mbus_code) {\n> +\t\tLOG(IPU3, Error) << \"Sensor does not support mbus code\";\n> +\t\treturn {};\n> +\t}\n> +\n> +\tcfg.size = sensorFormat.size;\n> +\tcfg.pixelFormat = mbusCodesToInfo.at(sensorFormat.mbus_code).pixelFormat;\n> +\tcfg.bufferCount = CIO2_BUFFER_COUNT;\n> +\n> +\treturn cfg;\n> +}\n> +\n> /**\n> * \\brief Allocate frame buffers for the CIO2 output\n> *\n> diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h\n> index b2c4f89d682d6cfb..6276573f2b585b25 100644\n> --- a/src/libcamera/pipeline/ipu3/cio2.h\n> +++ b/src/libcamera/pipeline/ipu3/cio2.h\n> @@ -20,6 +20,7 @@ class V4L2DeviceFormat;\n> class V4L2Subdevice;\n> class V4L2VideoDevice;\n> struct Size;\n> +struct StreamConfiguration;\n>\n> class CIO2Device\n> {\n> @@ -32,6 +33,8 @@ public:\n> \tint init(const MediaDevice *media, unsigned int index);\n> \tint configure(const Size &size, V4L2DeviceFormat *outputFormat);\n>\n> +\tStreamConfiguration generateConfiguration(const Size &desiredSize) const;\n> +\n> \tint allocateBuffers();\n> \tvoid freeBuffers();\n>\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 6e5eb5609a3c2388..c0e727e592f46883 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -36,13 +36,6 @@ LOG_DEFINE_CATEGORY(IPU3)\n>\n> class IPU3CameraData;\n>\n> -static const std::map sensorMbusToPixel = {\n> -\t{ MEDIA_BUS_FMT_SBGGR10_1X10, formats::SBGGR10_IPU3 },\n> -\t{ MEDIA_BUS_FMT_SGBRG10_1X10, formats::SGBRG10_IPU3 },\n> -\t{ MEDIA_BUS_FMT_SGRBG10_1X10, formats::SGRBG10_IPU3 },\n> -\t{ MEDIA_BUS_FMT_SRGGB10_1X10, formats::SRGGB10_IPU3 },\n> -};\n> -\n> class ImgUDevice\n> {\n> public:\n> @@ -147,7 +140,7 @@ public:\n>\n> \tStatus validate() override;\n>\n> -\tconst V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }\n> +\tconst StreamConfiguration &cio2Format() const { return cio2Configuration_; };\n> \tconst std::vector &streams() { return streams_; }\n>\n> private:\n> @@ -165,7 +158,7 @@ private:\n> \tstd::shared_ptr camera_;\n> \tconst IPU3CameraData *data_;\n>\n> -\tV4L2SubdeviceFormat sensorFormat_;\n> +\tStreamConfiguration cio2Configuration_;\n> \tstd::vector streams_;\n> };\n>\n> @@ -252,7 +245,7 @@ void IPU3CameraConfiguration::assignStreams()\n>\n> \t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> \t\t\tstream = &data_->rawStream_;\n> -\t\telse if (cfg.size == sensorFormat_.size)\n> +\t\telse if (cfg.size == cio2Configuration_.size)\n> \t\t\tstream = &data_->outStream_;\n> \t\telse\n> \t\t\tstream = &data_->vfStream_;\n> @@ -277,8 +270,8 @@ void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)\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 * sensorFormat_.size.height\n> -\t\t\t\t\t/ sensorFormat_.size.width;\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> @@ -297,7 +290,7 @@ void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)\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 = sensorFormat_.size;\n> +\t\tcfg.size = cio2Configuration_.size;\n> \t}\n>\n> \t/*\n> @@ -313,7 +306,6 @@ void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)\n>\n> CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> {\n> -\tconst CameraSensor *sensor = data_->cio2_.sensor_;\n> \tStatus status = Valid;\n>\n> \tif (config_.empty())\n> @@ -340,16 +332,10 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> \t\t\tsize.height = cfg.size.height;\n> \t}\n>\n> -\tif (!size.width || !size.height)\n> -\t\tsize = sensor->resolution();\n> -\n> -\tsensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10,\n> -\t\t\t\t\t MEDIA_BUS_FMT_SGBRG10_1X10,\n> -\t\t\t\t\t MEDIA_BUS_FMT_SGRBG10_1X10,\n> -\t\t\t\t\t MEDIA_BUS_FMT_SRGGB10_1X10 },\n> -\t\t\t\t\t size);\n> -\tif (!sensorFormat_.size.width || !sensorFormat_.size.height)\n> -\t\tsensorFormat_.size = sensor->resolution();\n> +\t/* Generate raw configuration from CIO2. */\n> +\tcio2Configuration_ = data_->cio2_.generateConfiguration(size);\n> +\tif (!cio2Configuration_.pixelFormat.isValid())\n> +\t\treturn Invalid;\n>\n> \t/* Assign streams to each configuration entry. */\n> \tassignStreams();\n> @@ -361,20 +347,13 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> \t\tconst IPU3Stream *stream = streams_[i];\n>\n> \t\tif (stream->raw_) {\n> -\t\t\tconst auto &itFormat =\n> -\t\t\t\tsensorMbusToPixel.find(sensorFormat_.mbus_code);\n> -\t\t\tif (itFormat == sensorMbusToPixel.end())\n> -\t\t\t\treturn Invalid;\n> -\n> -\t\t\tcfg.pixelFormat = itFormat->second;\n> -\t\t\tcfg.size = sensorFormat_.size;\n> +\t\t\tcfg = cio2Configuration_;\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}\n>\n> -\t\tcfg.bufferCount = IPU3_BUFFER_COUNT;\n> -\n> \t\tif (cfg.pixelFormat != oldCfg.pixelFormat ||\n> \t\t cfg.size != oldCfg.size) {\n> \t\t\tLOG(IPU3, Debug)\n> @@ -454,14 +433,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n>\n> \t\t\tcfg.size = data->cio2_.sensor_->resolution();\n>\n> -\t\t\tV4L2SubdeviceFormat sensorFormat =\n> -\t\t\t\tdata->cio2_.sensor_->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10,\n> -\t\t\t\t\t\t\t\t MEDIA_BUS_FMT_SGBRG10_1X10,\n> -\t\t\t\t\t\t\t\t MEDIA_BUS_FMT_SGRBG10_1X10,\n> -\t\t\t\t\t\t\t\t MEDIA_BUS_FMT_SRGGB10_1X10 },\n> -\t\t\t\t\t\t\t cfg.size);\n> -\t\t\tcfg.pixelFormat =\n> -\t\t\t\tsensorMbusToPixel.at(sensorFormat.mbus_code);\n> +\t\t\tcfg = data->cio2_.generateConfiguration(cfg.size);\n> \t\t\tbreak;\n> \t\t}\n>\n> @@ -575,7 +547,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> \t * Pass the requested stream size to the CIO2 unit and get back the\n> \t * adjusted format to be propagated to the ImgU output devices.\n> \t */\n> -\tconst Size &sensorSize = config->sensorFormat().size;\n> +\tconst Size &sensorSize = config->cio2Format().size;\n> \tV4L2DeviceFormat cio2Format = {};\n> \tret = cio2->configure(sensorSize, &cio2Format);\n> \tif (ret)\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":"","Received":["from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D245660103\n\tfor ;\n\tWed, 24 Jun 2020 12:18:33 +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 relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 3EC01C0012;\n\tWed, 24 Jun 2020 10:18:32 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Wed, 24 Jun 2020 12:22:01 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200624102201.26dhczlklqrxcxd3@uno.localdomain>","References":"<20200623020930.1781469-1-niklas.soderlund@ragnatech.se>\n\t<20200623020930.1781469-8-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200623020930.1781469-8-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v3 07/10] libcamera: ipu3: cio2: Add\n\tfunction to generate configuration","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","X-List-Received-Date":"Wed, 24 Jun 2020 10:18:34 -0000"}}]