[{"id":4965,"web_url":"https://patchwork.libcamera.org/comment/4965/","msgid":"<20200602120837.e75sxsg274loe24w@uno.localdomain>","date":"2020-06-02T12:08:37","subject":"Re: [libcamera-devel] [PATCH 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 02, 2020 at 03:39:06AM +0200, Niklas Söderlund wrote:\n> Collect the code used to generate configurations for the CIO2 block in\n> the CIO2Device class. This allows for both simplifying the code while\n> extending it's functionality. With this change applications can now\n> switch which Bayer format pattern are used instead being more or less\n> forced to use SBGGR10.\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/pipeline/ipu3/cio2.cpp | 44 +++++++++++++++++++++\n> src/libcamera/pipeline/ipu3/cio2.h | 5 +++\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 57 ++++++----------------------\n> 3 files changed, 60 insertions(+), 46 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> index 113486e3e3d0f2f1..2263d6530ec6b672 100644\n> --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> @@ -11,6 +11,13 @@ namespace libcamera {\n>\n> LOG_DECLARE_CATEGORY(IPU3)\n>\n> +static const std::map sensorMbusToPixel = {\n> +\t{ MEDIA_BUS_FMT_SBGGR10_1X10, PixelFormat(DRM_FORMAT_SBGGR10, IPU3_FORMAT_MOD_PACKED) },\n> +\t{ MEDIA_BUS_FMT_SGBRG10_1X10, PixelFormat(DRM_FORMAT_SGBRG10, IPU3_FORMAT_MOD_PACKED) },\n> +\t{ MEDIA_BUS_FMT_SGRBG10_1X10, PixelFormat(DRM_FORMAT_SGRBG10, IPU3_FORMAT_MOD_PACKED) },\n> +\t{ MEDIA_BUS_FMT_SRGGB10_1X10, PixelFormat(DRM_FORMAT_SRGGB10, IPU3_FORMAT_MOD_PACKED) },\n> +};\n> +\n\nWhat about an anonymous namespace instead of static ?\n\n> /**\n> * \\brief Initialize components of the CIO2 device with \\a index\n> * \\param[in] media The CIO2 media device\n> @@ -153,6 +160,43 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)\n> \treturn 0;\n> }\n>\n\nNo documentation ?\nsrc/libcamera/pipeline/ is excluded from the Doxyegen search path, but\nI would like to see documentation anyhow, especially about the\n(optional) parameters.\n\n> +StreamConfiguration\n> +CIO2Device::generateConfiguration(const PixelFormat desiredPixelFormat,\n> +\t\t\t\t const Size desiredSize) const\n\nIs generateConfiguration the correct name ? That's the Camera and\npipeline handler API name, while this is internal to the IPU3\nmachinery. I don't have much better ideas, so feel free to keep it if\nyou are not concerned about the name clash.\n\n> +{\n> +\tStreamConfiguration cfg;\n> +\n> +\t/* If no desired pixelformat allow all supported.*/\n\ns/all supported/all supported ones/\nAlso, missing space at the end\n\n> +\tstd::vector mbusCodes = {\n\nYou should use array<> as the size is fixed. Also, const.\n\n> +\t\tMEDIA_BUS_FMT_SBGGR10_1X10,\n> +\t\tMEDIA_BUS_FMT_SGBRG10_1X10,\n> +\t\tMEDIA_BUS_FMT_SGRBG10_1X10,\n> +\t\tMEDIA_BUS_FMT_SRGGB10_1X10\n> +\t};\n> +\tif (desiredPixelFormat.isValid()) {\n> +\t\tfor (const auto &iter : sensorMbusToPixel) {\n> +\t\t\tif (iter.second == desiredPixelFormat) {\n> +\t\t\t\tmbusCodes = { iter.first };\n\nAh no, you can't use array<>\n\nHowever this code doesn't make sure you actually find any matching\nformat, and if you don't application has requested an unsupported raw\nformat, so you should fail loudly here imo.\n\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\t}\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\nWhich static information ? You get the current configured format and\nuse it...\n\n> +\tV4L2SubdeviceFormat sensorFormat = sensor_->getFormat(mbusCodes, size);\n> +\n> +\tcfg.size = sensorFormat.size;\n> +\tcfg.pixelFormat = sensorMbusToPixel.at(sensorFormat.mbus_code);\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 d923038bb4ba356f..2e268a7154b2d241 100644\n> --- a/src/libcamera/pipeline/ipu3/cio2.h\n> +++ b/src/libcamera/pipeline/ipu3/cio2.h\n> @@ -12,6 +12,8 @@\n>\n> #include \n>\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> @@ -39,6 +41,9 @@ public:\n> \tint init(const MediaDevice *media, unsigned int index);\n> \tint configure(const Size &size, V4L2DeviceFormat *outputFormat);\n>\n> +\tStreamConfiguration generateConfiguration(const PixelFormat desiredPixelFormat = {},\n> +\t\t\t\t\t\t const Size desiredSize = {}) const;\n> +\n\nThis could probably be simplified in its implementation by providing\ntwo overloaded methods instead of defaulting parameters and\nmix-matching them in the implementation (and default parameters in C++\nare a pain to follow, as they are declared as optional in the class\ndeclaration only).\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 2047deac299dbf75..56cc3ca10414f0d2 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -35,13 +35,6 @@ LOG_DEFINE_CATEGORY(IPU3)\n>\n> class IPU3CameraData;\n>\n> -static const std::map sensorMbusToPixel = {\n> -\t{ MEDIA_BUS_FMT_SBGGR10_1X10, PixelFormat(DRM_FORMAT_SBGGR10, IPU3_FORMAT_MOD_PACKED) },\n> -\t{ MEDIA_BUS_FMT_SGBRG10_1X10, PixelFormat(DRM_FORMAT_SGBRG10, IPU3_FORMAT_MOD_PACKED) },\n> -\t{ MEDIA_BUS_FMT_SGRBG10_1X10, PixelFormat(DRM_FORMAT_SGRBG10, IPU3_FORMAT_MOD_PACKED) },\n> -\t{ MEDIA_BUS_FMT_SRGGB10_1X10, PixelFormat(DRM_FORMAT_SRGGB10, IPU3_FORMAT_MOD_PACKED) },\n> -};\n> -\n> class ImgUDevice\n> {\n> public:\n> @@ -146,7 +139,7 @@ public:\n>\n> \tStatus validate() override;\n>\n> -\tconst V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }\n> +\tconst StreamConfiguration &sensorFormat() const { return sensorFormat_; };\n> \tconst std::vector &streams() { return streams_; }\n>\n> private:\n> @@ -164,7 +157,7 @@ private:\n> \tstd::shared_ptr camera_;\n> \tconst IPU3CameraData *data_;\n>\n> -\tV4L2SubdeviceFormat sensorFormat_;\n> +\tStreamConfiguration sensorFormat_;\n> \tstd::vector streams_;\n> };\n>\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> @@ -325,32 +317,21 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> \t\tstatus = Adjusted;\n> \t}\n>\n> -\t/*\n> -\t * Select the sensor format by collecting the maximum width and height\n> -\t * and picking the closest larger match, as the IPU3 can downscale\n> -\t * only. If no resolution is requested for any stream, or if no sensor\n> -\t * resolution is large enough, pick the largest one.\n> -\t */\n> +\t/* Find largets size and raw format (if any) in the configuration. */\n> \tSize size = {};\n> -\n> +\tPixelFormat pixelFormat = {};\n> \tfor (const StreamConfiguration &cfg : config_) {\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> +\n> +\t\tif (cfg.pixelFormat.modifier() == IPU3_FORMAT_MOD_PACKED)\n> +\t\t\tpixelFormat = cfg.pixelFormat;\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> -\n> +\t/* Generate raw configuration from CIO2. */\n> +\tsensorFormat_ = data_->cio2_.generateConfiguration(pixelFormat, size);\n>\n> \t/* Assign streams to each configuration entry. */\n> \tif (updateStreams())\n> @@ -363,13 +344,7 @@ 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 = sensorFormat_;\n> \t\t} else {\n> \t\t\tbool scale = stream == &data_->vfStream_;\n> \t\t\tadjustStream(config_[i], scale);\n> @@ -452,17 +427,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> \t\t\t}\n>\n> \t\t\tstream = &data->rawStream_;\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();\n> \t\t\tbreak;\n> \t\t}\n>\n> --\n> 2.26.2\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 B65AD603CB\n\tfor ;\n\tTue, 2 Jun 2020 14:05:16 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 36CC2C0015;\n\tTue, 2 Jun 2020 12:05:15 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Tue, 2 Jun 2020 14:08:37 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200602120837.e75sxsg274loe24w@uno.localdomain>","References":"<20200602013909.3170593-1-niklas.soderlund@ragnatech.se>\n\t<20200602013909.3170593-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":"<20200602013909.3170593-8-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH 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":"Tue, 02 Jun 2020 12:05:16 -0000"}},{"id":5072,"web_url":"https://patchwork.libcamera.org/comment/5072/","msgid":"<20200605220634.GK26752@pendragon.ideasonboard.com>","date":"2020-06-05T22:06:34","subject":"Re: [libcamera-devel] [PATCH 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 02, 2020 at 02:08:37PM +0200, Jacopo Mondi wrote:\n> On Tue, Jun 02, 2020 at 03:39:06AM +0200, Niklas Söderlund wrote:\n> > Collect the code used to generate configurations for the CIO2 block in\n> > the CIO2Device class. This allows for both simplifying the code while\n> > extending it's functionality. With this change applications can now\n\ns/it's/its/\n\n> > switch which Bayer format pattern are used instead being more or less\n> > forced to use SBGGR10.\n\nIsn't the Bayer pattern fixed by the sensor ?\n\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/pipeline/ipu3/cio2.cpp | 44 +++++++++++++++++++++\n> > src/libcamera/pipeline/ipu3/cio2.h | 5 +++\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 57 ++++++----------------------\n> > 3 files changed, 60 insertions(+), 46 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > index 113486e3e3d0f2f1..2263d6530ec6b672 100644\n> > --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > @@ -11,6 +11,13 @@ namespace libcamera {\n> >\n> > LOG_DECLARE_CATEGORY(IPU3)\n> >\n> > +static const std::map sensorMbusToPixel = {\n> > +\t{ MEDIA_BUS_FMT_SBGGR10_1X10, PixelFormat(DRM_FORMAT_SBGGR10, IPU3_FORMAT_MOD_PACKED) },\n> > +\t{ MEDIA_BUS_FMT_SGBRG10_1X10, PixelFormat(DRM_FORMAT_SGBRG10, IPU3_FORMAT_MOD_PACKED) },\n> > +\t{ MEDIA_BUS_FMT_SGRBG10_1X10, PixelFormat(DRM_FORMAT_SGRBG10, IPU3_FORMAT_MOD_PACKED) },\n> > +\t{ MEDIA_BUS_FMT_SRGGB10_1X10, PixelFormat(DRM_FORMAT_SRGGB10, IPU3_FORMAT_MOD_PACKED) },\n> > +};\n> > +\n> \n> What about an anonymous namespace instead of static ?\n\nThat's what we tend to use, yes.\n\n> > /**\n> > * \\brief Initialize components of the CIO2 device with \\a index\n> > * \\param[in] media The CIO2 media device\n> > @@ -153,6 +160,43 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)\n> > \treturn 0;\n> > }\n> \n> No documentation ?\n> src/libcamera/pipeline/ is excluded from the Doxyegen search path, but\n> I would like to see documentation anyhow, especially about the\n> (optional) parameters.\n> \n> > +StreamConfiguration\n> > +CIO2Device::generateConfiguration(const PixelFormat desiredPixelFormat,\n> > +\t\t\t\t const Size desiredSize) const\n> \n> Is generateConfiguration the correct name ? That's the Camera and\n> pipeline handler API name, while this is internal to the IPU3\n> machinery. I don't have much better ideas, so feel free to keep it if\n> you are not concerned about the name clash.\n> \n> > +{\n> > +\tStreamConfiguration cfg;\n> > +\n> > +\t/* If no desired pixelformat allow all supported.*/\n> \n> s/all supported/all supported ones/\n\n\"all *the* supported ones\"\n\n> Also, missing space at the end\n> \n> > +\tstd::vector mbusCodes = {\n> \n> You should use array<> as the size is fixed. Also, const.\n> \n> > +\t\tMEDIA_BUS_FMT_SBGGR10_1X10,\n> > +\t\tMEDIA_BUS_FMT_SGBRG10_1X10,\n> > +\t\tMEDIA_BUS_FMT_SGRBG10_1X10,\n> > +\t\tMEDIA_BUS_FMT_SRGGB10_1X10\n> > +\t};\n> > +\tif (desiredPixelFormat.isValid()) {\n> > +\t\tfor (const auto &iter : sensorMbusToPixel) {\n> > +\t\t\tif (iter.second == desiredPixelFormat) {\n> > +\t\t\t\tmbusCodes = { iter.first };\n> \n> Ah no, you can't use array<>\n> \n> However this code doesn't make sure you actually find any matching\n> format, and if you don't application has requested an unsupported raw\n> format, so you should fail loudly here imo.\n> \n> > +\t\t\t\tbreak;\n> > +\t\t\t}\n> > +\t\t}\n> > +\t}\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> \n> Which static information ? You get the current configured format and\n> use it...\n> \n> > +\tV4L2SubdeviceFormat sensorFormat = sensor_->getFormat(mbusCodes, size);\n\nIf the requested format isn't supported by the sensor, this function\nwill return a default constructed V4L2SubdeviceFormat...\n\n> > +\n> > +\tcfg.size = sensorFormat.size;\n> > +\tcfg.pixelFormat = sensorMbusToPixel.at(sensorFormat.mbus_code);\n\n... and this will throw an exception.\n\n> > +\tcfg.bufferCount = CIO2_BUFFER_COUNT;\n> > +\n> > +\treturn cfg;\n> > +}\n\nI don't think this belongs to the CIO2Device class, as it has nothing to\ndo with the CIO2. As Jacopo commented separately, the CIO2Device class\nshould focus on the CIO2. The sensor should be handled by the top-level\npipeline handler.\n\nI would by the way also split the ImgUDevice class to a separate file.\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 d923038bb4ba356f..2e268a7154b2d241 100644\n> > --- a/src/libcamera/pipeline/ipu3/cio2.h\n> > +++ b/src/libcamera/pipeline/ipu3/cio2.h\n> > @@ -12,6 +12,8 @@\n> >\n> > #include \n> >\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> > @@ -39,6 +41,9 @@ public:\n> > \tint init(const MediaDevice *media, unsigned int index);\n> > \tint configure(const Size &size, V4L2DeviceFormat *outputFormat);\n> >\n> > +\tStreamConfiguration generateConfiguration(const PixelFormat desiredPixelFormat = {},\n> > +\t\t\t\t\t\t const Size desiredSize = {}) const;\n> > +\n> \n> This could probably be simplified in its implementation by providing\n> two overloaded methods instead of defaulting parameters and\n> mix-matching them in the implementation (and default parameters in C++\n> are a pain to follow, as they are declared as optional in the class\n> declaration only).\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 2047deac299dbf75..56cc3ca10414f0d2 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -35,13 +35,6 @@ LOG_DEFINE_CATEGORY(IPU3)\n> >\n> > class IPU3CameraData;\n> >\n> > -static const std::map sensorMbusToPixel = {\n> > -\t{ MEDIA_BUS_FMT_SBGGR10_1X10, PixelFormat(DRM_FORMAT_SBGGR10, IPU3_FORMAT_MOD_PACKED) },\n> > -\t{ MEDIA_BUS_FMT_SGBRG10_1X10, PixelFormat(DRM_FORMAT_SGBRG10, IPU3_FORMAT_MOD_PACKED) },\n> > -\t{ MEDIA_BUS_FMT_SGRBG10_1X10, PixelFormat(DRM_FORMAT_SGRBG10, IPU3_FORMAT_MOD_PACKED) },\n> > -\t{ MEDIA_BUS_FMT_SRGGB10_1X10, PixelFormat(DRM_FORMAT_SRGGB10, IPU3_FORMAT_MOD_PACKED) },\n> > -};\n> > -\n> > class ImgUDevice\n> > {\n> > public:\n> > @@ -146,7 +139,7 @@ public:\n> >\n> > \tStatus validate() override;\n> >\n> > -\tconst V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }\n> > +\tconst StreamConfiguration &sensorFormat() const { return sensorFormat_; };\n> > \tconst std::vector &streams() { return streams_; }\n> >\n> > private:\n> > @@ -164,7 +157,7 @@ private:\n> > \tstd::shared_ptr camera_;\n> > \tconst IPU3CameraData *data_;\n> >\n> > -\tV4L2SubdeviceFormat sensorFormat_;\n> > +\tStreamConfiguration sensorFormat_;\n> > \tstd::vector streams_;\n> > };\n> >\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> > @@ -325,32 +317,21 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > \t\tstatus = Adjusted;\n> > \t}\n> >\n> > -\t/*\n> > -\t * Select the sensor format by collecting the maximum width and height\n> > -\t * and picking the closest larger match, as the IPU3 can downscale\n> > -\t * only. If no resolution is requested for any stream, or if no sensor\n> > -\t * resolution is large enough, pick the largest one.\n> > -\t */\n> > +\t/* Find largets size and raw format (if any) in the configuration. */\n> > \tSize size = {};\n> > -\n> > +\tPixelFormat pixelFormat = {};\n> > \tfor (const StreamConfiguration &cfg : config_) {\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> > +\n> > +\t\tif (cfg.pixelFormat.modifier() == IPU3_FORMAT_MOD_PACKED)\n> > +\t\t\tpixelFormat = cfg.pixelFormat;\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> > -\n> > +\t/* Generate raw configuration from CIO2. */\n> > +\tsensorFormat_ = data_->cio2_.generateConfiguration(pixelFormat, size);\n> >\n> > \t/* Assign streams to each configuration entry. */\n> > \tif (updateStreams())\n> > @@ -363,13 +344,7 @@ 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 = sensorFormat_;\n> > \t\t} else {\n> > \t\t\tbool scale = stream == &data_->vfStream_;\n> > \t\t\tadjustStream(config_[i], scale);\n> > @@ -452,17 +427,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> > \t\t\t}\n> >\n> > \t\t\tstream = &data->rawStream_;\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();\n> > \t\t\tbreak;\n> > \t\t}\n> >","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 97843603C8\n\tfor ;\n\tSat, 6 Jun 2020 00:06:53 +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 12E9827C;\n\tSat, 6 Jun 2020 00:06:53 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"a7N6WVkt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1591394813;\n\tbh=qnBXN9Bwio2BDhpEAUc3e16qFNakSqm2h2zq4/jbV0U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=a7N6WVktKnMeJ+9ouxoDErPZ/b13xLN9f9QR68B2M/oAc3ohosmd1B6hCi4NSyBS5\n\t8MEw+XrKAXLRi5PvLRMJki3Vfdb24ZsfBpLvQhg8POicF8HhUD+LpLI+GK4OcFnCqP\n\t31iAePzC4mWD/GHc07qlDuxdlZf8XP1dPiIsul5Y=","Date":"Sat, 6 Jun 2020 01:06:34 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20200605220634.GK26752@pendragon.ideasonboard.com>","References":"<20200602013909.3170593-1-niklas.soderlund@ragnatech.se>\n\t<20200602013909.3170593-8-niklas.soderlund@ragnatech.se>\n\t<20200602120837.e75sxsg274loe24w@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200602120837.e75sxsg274loe24w@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 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":"Fri, 05 Jun 2020 22:06:53 -0000"}},{"id":5090,"web_url":"https://patchwork.libcamera.org/comment/5090/","msgid":"<20200606135550.GA395571@oden.dyn.berto.se>","date":"2020-06-06T13:55:50","subject":"Re: [libcamera-devel] [PATCH 07/10] libcamera: ipu3: cio2: Add\n\tfunction to generate configuration","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hello,\n\nOn 2020-06-06 01:06:34 +0300, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Tue, Jun 02, 2020 at 02:08:37PM +0200, Jacopo Mondi wrote:\n> > On Tue, Jun 02, 2020 at 03:39:06AM +0200, Niklas Söderlund wrote:\n> > > Collect the code used to generate configurations for the CIO2 block in\n> > > the CIO2Device class. This allows for both simplifying the code while\n> > > extending it's functionality. With this change applications can now\n> \n> s/it's/its/\n> \n> > > switch which Bayer format pattern are used instead being more or less\n> > > forced to use SBGGR10.\n> \n> Isn't the Bayer pattern fixed by the sensor ?\n\nIt is, what I tried to expressed is that other pixelformats then SBGGR10 \nis now possible to select from the application, will update.\n\n> \n> > > Signed-off-by: Niklas Söderlund \n> > > ---\n> > > src/libcamera/pipeline/ipu3/cio2.cpp | 44 +++++++++++++++++++++\n> > > src/libcamera/pipeline/ipu3/cio2.h | 5 +++\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 57 ++++++----------------------\n> > > 3 files changed, 60 insertions(+), 46 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > index 113486e3e3d0f2f1..2263d6530ec6b672 100644\n> > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > @@ -11,6 +11,13 @@ namespace libcamera {\n> > >\n> > > LOG_DECLARE_CATEGORY(IPU3)\n> > >\n> > > +static const std::map sensorMbusToPixel = {\n> > > +\t{ MEDIA_BUS_FMT_SBGGR10_1X10, PixelFormat(DRM_FORMAT_SBGGR10, IPU3_FORMAT_MOD_PACKED) },\n> > > +\t{ MEDIA_BUS_FMT_SGBRG10_1X10, PixelFormat(DRM_FORMAT_SGBRG10, IPU3_FORMAT_MOD_PACKED) },\n> > > +\t{ MEDIA_BUS_FMT_SGRBG10_1X10, PixelFormat(DRM_FORMAT_SGRBG10, IPU3_FORMAT_MOD_PACKED) },\n> > > +\t{ MEDIA_BUS_FMT_SRGGB10_1X10, PixelFormat(DRM_FORMAT_SRGGB10, IPU3_FORMAT_MOD_PACKED) },\n> > > +};\n> > > +\n> > \n> > What about an anonymous namespace instead of static ?\n> \n> That's what we tend to use, yes.\n> \n> > > /**\n> > > * \\brief Initialize components of the CIO2 device with \\a index\n> > > * \\param[in] media The CIO2 media device\n> > > @@ -153,6 +160,43 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)\n> > > \treturn 0;\n> > > }\n> > \n> > No documentation ?\n> > src/libcamera/pipeline/ is excluded from the Doxyegen search path, but\n> > I would like to see documentation anyhow, especially about the\n> > (optional) parameters.\n> > \n> > > +StreamConfiguration\n> > > +CIO2Device::generateConfiguration(const PixelFormat desiredPixelFormat,\n> > > +\t\t\t\t const Size desiredSize) const\n> > \n> > Is generateConfiguration the correct name ? That's the Camera and\n> > pipeline handler API name, while this is internal to the IPU3\n> > machinery. I don't have much better ideas, so feel free to keep it if\n> > you are not concerned about the name clash.\n> > \n> > > +{\n> > > +\tStreamConfiguration cfg;\n> > > +\n> > > +\t/* If no desired pixelformat allow all supported.*/\n> > \n> > s/all supported/all supported ones/\n> \n> \"all *the* supported ones\"\n> \n> > Also, missing space at the end\n> > \n> > > +\tstd::vector mbusCodes = {\n> > \n> > You should use array<> as the size is fixed. Also, const.\n\nIt can't be const as it's (possibly) modified bellow.\n\n> > \n> > > +\t\tMEDIA_BUS_FMT_SBGGR10_1X10,\n> > > +\t\tMEDIA_BUS_FMT_SGBRG10_1X10,\n> > > +\t\tMEDIA_BUS_FMT_SGRBG10_1X10,\n> > > +\t\tMEDIA_BUS_FMT_SRGGB10_1X10\n> > > +\t};\n> > > +\tif (desiredPixelFormat.isValid()) {\n> > > +\t\tfor (const auto &iter : sensorMbusToPixel) {\n> > > +\t\t\tif (iter.second == desiredPixelFormat) {\n> > > +\t\t\t\tmbusCodes = { iter.first };\n> > \n> > Ah no, you can't use array<>\n> > \n> > However this code doesn't make sure you actually find any matching\n> > format, and if you don't application has requested an unsupported raw\n> > format, so you should fail loudly here imo.\n\nIf no match is found the mbusCodes is not modified and all 4 possible \nmbus codes are considered when probing the sensor for a format. No need \nto warn IMHO.\n\n> > \n> > > +\t\t\t\tbreak;\n> > > +\t\t\t}\n> > > +\t\t}\n> > > +\t}\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> > \n> > Which static information ? You get the current configured format and\n> > use it...\n\nNot true, CameraSensor::getFormat() returns the best match for the mbus \ncode and size from the static/cached information it enumerated in \nCameraSensor::init().\n\n> > \n> > > +\tV4L2SubdeviceFormat sensorFormat = sensor_->getFormat(mbusCodes, size);\n> \n> If the requested format isn't supported by the sensor, this function\n> will return a default constructed V4L2SubdeviceFormat...\n\nGood point, will fix.\n\n> \n> > > +\n> > > +\tcfg.size = sensorFormat.size;\n> > > +\tcfg.pixelFormat = sensorMbusToPixel.at(sensorFormat.mbus_code);\n> \n> ... and this will throw an exception.\n> \n> > > +\tcfg.bufferCount = CIO2_BUFFER_COUNT;\n> > > +\n> > > +\treturn cfg;\n> > > +}\n> \n> I don't think this belongs to the CIO2Device class, as it has nothing to\n> do with the CIO2. As Jacopo commented separately, the CIO2Device class\n> should focus on the CIO2. The sensor should be handled by the top-level\n> pipeline handler.\n\nAs discussed on IRC I disagree wit this. I think the sensor belongs \ninside the CIO2Device as it's part of the CIO2 media graph. Handling it \noutside the CIO2Device will IMHO only create a spaghetti mess similar to \nthe one we already have where all objects in the IPU3 pipeline access \nmember variables all over the place.\n\nI will for v2 remove the proxy helpers to access the sensor and replace \nit with a CIO2Device::sensor() in hops this will suite Jacopos and your \nconcerns, lets see how it plays out :-)\n\n> \n> I would by the way also split the ImgUDevice class to a separate file.\n\nI plan to do so on top of this series, as mentioned in the cover letter.\n\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 d923038bb4ba356f..2e268a7154b2d241 100644\n> > > --- a/src/libcamera/pipeline/ipu3/cio2.h\n> > > +++ b/src/libcamera/pipeline/ipu3/cio2.h\n> > > @@ -12,6 +12,8 @@\n> > >\n> > > #include \n> > >\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> > > @@ -39,6 +41,9 @@ public:\n> > > \tint init(const MediaDevice *media, unsigned int index);\n> > > \tint configure(const Size &size, V4L2DeviceFormat *outputFormat);\n> > >\n> > > +\tStreamConfiguration generateConfiguration(const PixelFormat desiredPixelFormat = {},\n> > > +\t\t\t\t\t\t const Size desiredSize = {}) const;\n> > > +\n> > \n> > This could probably be simplified in its implementation by providing\n> > two overloaded methods instead of defaulting parameters and\n> > mix-matching them in the implementation (and default parameters in C++\n> > are a pain to follow, as they are declared as optional in the class\n> > declaration only).\n\nI slightly prefer default arguments over multiple implementations, but \nit's not a strong feeling. But if we are to drop default arguments lets \ndo so as part of a series that address the issue everywhere.\n\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 2047deac299dbf75..56cc3ca10414f0d2 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -35,13 +35,6 @@ LOG_DEFINE_CATEGORY(IPU3)\n> > >\n> > > class IPU3CameraData;\n> > >\n> > > -static const std::map sensorMbusToPixel = {\n> > > -\t{ MEDIA_BUS_FMT_SBGGR10_1X10, PixelFormat(DRM_FORMAT_SBGGR10, IPU3_FORMAT_MOD_PACKED) },\n> > > -\t{ MEDIA_BUS_FMT_SGBRG10_1X10, PixelFormat(DRM_FORMAT_SGBRG10, IPU3_FORMAT_MOD_PACKED) },\n> > > -\t{ MEDIA_BUS_FMT_SGRBG10_1X10, PixelFormat(DRM_FORMAT_SGRBG10, IPU3_FORMAT_MOD_PACKED) },\n> > > -\t{ MEDIA_BUS_FMT_SRGGB10_1X10, PixelFormat(DRM_FORMAT_SRGGB10, IPU3_FORMAT_MOD_PACKED) },\n> > > -};\n> > > -\n> > > class ImgUDevice\n> > > {\n> > > public:\n> > > @@ -146,7 +139,7 @@ public:\n> > >\n> > > \tStatus validate() override;\n> > >\n> > > -\tconst V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }\n> > > +\tconst StreamConfiguration &sensorFormat() const { return sensorFormat_; };\n> > > \tconst std::vector &streams() { return streams_; }\n> > >\n> > > private:\n> > > @@ -164,7 +157,7 @@ private:\n> > > \tstd::shared_ptr camera_;\n> > > \tconst IPU3CameraData *data_;\n> > >\n> > > -\tV4L2SubdeviceFormat sensorFormat_;\n> > > +\tStreamConfiguration sensorFormat_;\n> > > \tstd::vector streams_;\n> > > };\n> > >\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> > > @@ -325,32 +317,21 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > > \t\tstatus = Adjusted;\n> > > \t}\n> > >\n> > > -\t/*\n> > > -\t * Select the sensor format by collecting the maximum width and height\n> > > -\t * and picking the closest larger match, as the IPU3 can downscale\n> > > -\t * only. If no resolution is requested for any stream, or if no sensor\n> > > -\t * resolution is large enough, pick the largest one.\n> > > -\t */\n> > > +\t/* Find largets size and raw format (if any) in the configuration. */\n> > > \tSize size = {};\n> > > -\n> > > +\tPixelFormat pixelFormat = {};\n> > > \tfor (const StreamConfiguration &cfg : config_) {\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> > > +\n> > > +\t\tif (cfg.pixelFormat.modifier() == IPU3_FORMAT_MOD_PACKED)\n> > > +\t\t\tpixelFormat = cfg.pixelFormat;\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> > > -\n> > > +\t/* Generate raw configuration from CIO2. */\n> > > +\tsensorFormat_ = data_->cio2_.generateConfiguration(pixelFormat, size);\n> > >\n> > > \t/* Assign streams to each configuration entry. */\n> > > \tif (updateStreams())\n> > > @@ -363,13 +344,7 @@ 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 = sensorFormat_;\n> > > \t\t} else {\n> > > \t\t\tbool scale = stream == &data_->vfStream_;\n> > > \t\t\tadjustStream(config_[i], scale);\n> > > @@ -452,17 +427,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> > > \t\t\t}\n> > >\n> > > \t\t\tstream = &data->rawStream_;\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();\n> > > \t\t\tbreak;\n> > > \t\t}\n> > >\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0943161167\n\tfor ;\n\tSat, 6 Jun 2020 15:55:54 +0200 (CEST)","by mail-lj1-x242.google.com with SMTP id z9so15134047ljh.13\n\tfor ;\n\tSat, 06 Jun 2020 06:55:53 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tj5sm1916992ljg.78.2020.06.06.06.55.51\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSat, 06 Jun 2020 06:55:51 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com header.b=\"LL4z6GFk\"; \n\tdkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=hJ2ekIQ606wZ7WZtfarn+wtXV0NkD2yKooRT4CHLZz4=;\n\tb=LL4z6GFkkuM1JRXN0pjpd0tZLXGdgfXUNZIxafKcaIbLVbu9yR8tSklJpuEhyRgDii\n\t1LP/bjx/iW2DoA/ydd7sV9ksBBQZ8AH6WSALFRBf2m83i96DNponiBw3LdT/48Vfk75L\n\tdOMaZNdQf4W8z7RjjRM3BOIElBXF2ABv3/gdD1lffU4qb0O5DUXZrT+5RU78aR773YWQ\n\tnJ/ysKgqK53QQArVmyntRqKPLeVFczssM+y/Rd8SaS4R9MGTbuN+2+DLA2YIg2fTNVLe\n\tpPKnRlHt7rKL3nn41U9D6IjaLwb3I7D4uR+pVmChYWJ/mF+nLjXca2d1JZJcHlQf3wWj\n\tu6OQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=hJ2ekIQ606wZ7WZtfarn+wtXV0NkD2yKooRT4CHLZz4=;\n\tb=mrHHhyUckmjprzNKAi2FXQ/ctTIENQSRRoQyLHkqpQQICH+DkMLIjpEmmrFf4YEX31\n\tFF0DtPl9vpexUBx8d4eF7D1NkgOR8jvUl4N27vtp8jFn1h0nn4sm1ohPRuTgxaAr5iV3\n\tYSWRBqV51pJ1zG48t2XMdHohRJwm/sMQJmfPP9yigyeDftKrKvqwUTRCeX34eWnM1M0X\n\tzM6UE+B0xuLP1P5594GDYaMfgf2blgeU4WJ8cA8SVFp4I+sWpdcIOPIvgabSDjWjZzWC\n\tctErh2RipL+doJmv9ppJiSVBxXGi9hCitY01EnN73hMk07trHsvMv9Q6lZyteKfQSu5X\n\tE6ZQ==","X-Gm-Message-State":"AOAM532hjR21nUh6e6NdxOFH3mM6cBAJj0BJXpGhTfQQFcNVMD6Iuha6\n\tLREWAAJjczj/618zTGzs3ZpInWp5UBw=","X-Google-Smtp-Source":"ABdhPJx4gX/htO4YbSF4/PxYJ7r6ICIVslOtuFjf2ZJaEUjEoKJ6sXBcGzgQTDK1yGJ2PfUCbOOvug==","X-Received":"by 2002:a2e:9c97:: with SMTP id x23mr1487300lji.36.1591451752762;\n\tSat, 06 Jun 2020 06:55:52 -0700 (PDT)","Date":"Sat, 6 Jun 2020 15:55:50 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20200606135550.GA395571@oden.dyn.berto.se>","References":"<20200602013909.3170593-1-niklas.soderlund@ragnatech.se>\n\t<20200602013909.3170593-8-niklas.soderlund@ragnatech.se>\n\t<20200602120837.e75sxsg274loe24w@uno.localdomain>\n\t<20200605220634.GK26752@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200605220634.GK26752@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 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":"Sat, 06 Jun 2020 13:55:54 -0000"}},{"id":5093,"web_url":"https://patchwork.libcamera.org/comment/5093/","msgid":"<20200606170349.GB7339@pendragon.ideasonboard.com>","date":"2020-06-06T17:03:49","subject":"Re: [libcamera-devel] [PATCH 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\nOn Sat, Jun 06, 2020 at 03:55:50PM +0200, Niklas Söderlund wrote:\n> On 2020-06-06 01:06:34 +0300, Laurent Pinchart wrote:\n> > On Tue, Jun 02, 2020 at 02:08:37PM +0200, Jacopo Mondi wrote:\n> > > On Tue, Jun 02, 2020 at 03:39:06AM +0200, Niklas Söderlund wrote:\n> > > > Collect the code used to generate configurations for the CIO2 block in\n> > > > the CIO2Device class. This allows for both simplifying the code while\n> > > > extending it's functionality. With this change applications can now\n> > \n> > s/it's/its/\n> > \n> > > > switch which Bayer format pattern are used instead being more or less\n> > > > forced to use SBGGR10.\n> > \n> > Isn't the Bayer pattern fixed by the sensor ?\n> \n> It is, what I tried to expressed is that other pixelformats then SBGGR10 \n> is now possible to select from the application, will update.\n> \n> > > > Signed-off-by: Niklas Söderlund \n> > > > ---\n> > > > src/libcamera/pipeline/ipu3/cio2.cpp | 44 +++++++++++++++++++++\n> > > > src/libcamera/pipeline/ipu3/cio2.h | 5 +++\n> > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 57 ++++++----------------------\n> > > > 3 files changed, 60 insertions(+), 46 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > > index 113486e3e3d0f2f1..2263d6530ec6b672 100644\n> > > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > > @@ -11,6 +11,13 @@ namespace libcamera {\n> > > >\n> > > > LOG_DECLARE_CATEGORY(IPU3)\n> > > >\n> > > > +static const std::map sensorMbusToPixel = {\n> > > > +\t{ MEDIA_BUS_FMT_SBGGR10_1X10, PixelFormat(DRM_FORMAT_SBGGR10, IPU3_FORMAT_MOD_PACKED) },\n> > > > +\t{ MEDIA_BUS_FMT_SGBRG10_1X10, PixelFormat(DRM_FORMAT_SGBRG10, IPU3_FORMAT_MOD_PACKED) },\n> > > > +\t{ MEDIA_BUS_FMT_SGRBG10_1X10, PixelFormat(DRM_FORMAT_SGRBG10, IPU3_FORMAT_MOD_PACKED) },\n> > > > +\t{ MEDIA_BUS_FMT_SRGGB10_1X10, PixelFormat(DRM_FORMAT_SRGGB10, IPU3_FORMAT_MOD_PACKED) },\n> > > > +};\n> > > > +\n> > > \n> > > What about an anonymous namespace instead of static ?\n> > \n> > That's what we tend to use, yes.\n> > \n> > > > /**\n> > > > * \\brief Initialize components of the CIO2 device with \\a index\n> > > > * \\param[in] media The CIO2 media device\n> > > > @@ -153,6 +160,43 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)\n> > > > \treturn 0;\n> > > > }\n> > > \n> > > No documentation ?\n> > > src/libcamera/pipeline/ is excluded from the Doxyegen search path, but\n> > > I would like to see documentation anyhow, especially about the\n> > > (optional) parameters.\n> > > \n> > > > +StreamConfiguration\n> > > > +CIO2Device::generateConfiguration(const PixelFormat desiredPixelFormat,\n> > > > +\t\t\t\t const Size desiredSize) const\n> > > \n> > > Is generateConfiguration the correct name ? That's the Camera and\n> > > pipeline handler API name, while this is internal to the IPU3\n> > > machinery. I don't have much better ideas, so feel free to keep it if\n> > > you are not concerned about the name clash.\n> > > \n> > > > +{\n> > > > +\tStreamConfiguration cfg;\n> > > > +\n> > > > +\t/* If no desired pixelformat allow all supported.*/\n> > > \n> > > s/all supported/all supported ones/\n> > \n> > \"all *the* supported ones\"\n> > \n> > > Also, missing space at the end\n> > > \n> > > > +\tstd::vector mbusCodes = {\n> > > \n> > > You should use array<> as the size is fixed. Also, const.\n> \n> It can't be const as it's (possibly) modified bellow.\n> \n> > > > +\t\tMEDIA_BUS_FMT_SBGGR10_1X10,\n> > > > +\t\tMEDIA_BUS_FMT_SGBRG10_1X10,\n> > > > +\t\tMEDIA_BUS_FMT_SGRBG10_1X10,\n> > > > +\t\tMEDIA_BUS_FMT_SRGGB10_1X10\n> > > > +\t};\n> > > > +\tif (desiredPixelFormat.isValid()) {\n> > > > +\t\tfor (const auto &iter : sensorMbusToPixel) {\n> > > > +\t\t\tif (iter.second == desiredPixelFormat) {\n> > > > +\t\t\t\tmbusCodes = { iter.first };\n> > > \n> > > Ah no, you can't use array<>\n> > > \n> > > However this code doesn't make sure you actually find any matching\n> > > format, and if you don't application has requested an unsupported raw\n> > > format, so you should fail loudly here imo.\n> \n> If no match is found the mbusCodes is not modified and all 4 possible \n> mbus codes are considered when probing the sensor for a format. No need \n> to warn IMHO.\n> \n> > > > +\t\t\t\tbreak;\n> > > > +\t\t\t}\n> > > > +\t\t}\n> > > > +\t}\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> > > \n> > > Which static information ? You get the current configured format and\n> > > use it...\n> \n> Not true, CameraSensor::getFormat() returns the best match for the mbus \n> code and size from the static/cached information it enumerated in \n> CameraSensor::init().\n> \n> > > \n> > > > +\tV4L2SubdeviceFormat sensorFormat = sensor_->getFormat(mbusCodes, size);\n> > \n> > If the requested format isn't supported by the sensor, this function\n> > will return a default constructed V4L2SubdeviceFormat...\n> \n> Good point, will fix.\n> \n> > > > +\n> > > > +\tcfg.size = sensorFormat.size;\n> > > > +\tcfg.pixelFormat = sensorMbusToPixel.at(sensorFormat.mbus_code);\n> > \n> > ... and this will throw an exception.\n> > \n> > > > +\tcfg.bufferCount = CIO2_BUFFER_COUNT;\n> > > > +\n> > > > +\treturn cfg;\n> > > > +}\n> > \n> > I don't think this belongs to the CIO2Device class, as it has nothing to\n> > do with the CIO2. As Jacopo commented separately, the CIO2Device class\n> > should focus on the CIO2. The sensor should be handled by the top-level\n> > pipeline handler.\n> \n> As discussed on IRC I disagree wit this. I think the sensor belongs \n> inside the CIO2Device as it's part of the CIO2 media graph.\n\nThere are pros and cons in both cases, but the fact that the sensor is\npart of the CIO2 media graph isn't a decise argument in my opinion. The\nmedia graph is a kernel abstraction, and we should design a userspace\nabstraction that makes the userspace implementation as clean and easy to\nunderstand as possible. It may match the media graph partition, or it\nmay not.\n\n> Handling it \n> outside the CIO2Device will IMHO only create a spaghetti mess similar to \n> the one we already have where all objects in the IPU3 pipeline access \n> member variables all over the place.\n> \n> I will for v2 remove the proxy helpers to access the sensor and replace \n> it with a CIO2Device::sensor() in hops this will suite Jacopos and your \n> concerns, lets see how it plays out :-)\n\nI think that's a good step forward, I'll check if I find it clean enough\n:-)\n\n> > I would by the way also split the ImgUDevice class to a separate file.\n> \n> I plan to do so on top of this series, as mentioned in the cover letter.\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 d923038bb4ba356f..2e268a7154b2d241 100644\n> > > > --- a/src/libcamera/pipeline/ipu3/cio2.h\n> > > > +++ b/src/libcamera/pipeline/ipu3/cio2.h\n> > > > @@ -12,6 +12,8 @@\n> > > >\n> > > > #include \n> > > >\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> > > > @@ -39,6 +41,9 @@ public:\n> > > > \tint init(const MediaDevice *media, unsigned int index);\n> > > > \tint configure(const Size &size, V4L2DeviceFormat *outputFormat);\n> > > >\n> > > > +\tStreamConfiguration generateConfiguration(const PixelFormat desiredPixelFormat = {},\n> > > > +\t\t\t\t\t\t const Size desiredSize = {}) const;\n> > > > +\n> > > \n> > > This could probably be simplified in its implementation by providing\n> > > two overloaded methods instead of defaulting parameters and\n> > > mix-matching them in the implementation (and default parameters in C++\n> > > are a pain to follow, as they are declared as optional in the class\n> > > declaration only).\n> \n> I slightly prefer default arguments over multiple implementations, but \n> it's not a strong feeling. But if we are to drop default arguments lets \n> do so as part of a series that address the issue everywhere.\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 2047deac299dbf75..56cc3ca10414f0d2 100644\n> > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > @@ -35,13 +35,6 @@ LOG_DEFINE_CATEGORY(IPU3)\n> > > >\n> > > > class IPU3CameraData;\n> > > >\n> > > > -static const std::map sensorMbusToPixel = {\n> > > > -\t{ MEDIA_BUS_FMT_SBGGR10_1X10, PixelFormat(DRM_FORMAT_SBGGR10, IPU3_FORMAT_MOD_PACKED) },\n> > > > -\t{ MEDIA_BUS_FMT_SGBRG10_1X10, PixelFormat(DRM_FORMAT_SGBRG10, IPU3_FORMAT_MOD_PACKED) },\n> > > > -\t{ MEDIA_BUS_FMT_SGRBG10_1X10, PixelFormat(DRM_FORMAT_SGRBG10, IPU3_FORMAT_MOD_PACKED) },\n> > > > -\t{ MEDIA_BUS_FMT_SRGGB10_1X10, PixelFormat(DRM_FORMAT_SRGGB10, IPU3_FORMAT_MOD_PACKED) },\n> > > > -};\n> > > > -\n> > > > class ImgUDevice\n> > > > {\n> > > > public:\n> > > > @@ -146,7 +139,7 @@ public:\n> > > >\n> > > > \tStatus validate() override;\n> > > >\n> > > > -\tconst V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }\n> > > > +\tconst StreamConfiguration &sensorFormat() const { return sensorFormat_; };\n> > > > \tconst std::vector &streams() { return streams_; }\n> > > >\n> > > > private:\n> > > > @@ -164,7 +157,7 @@ private:\n> > > > \tstd::shared_ptr camera_;\n> > > > \tconst IPU3CameraData *data_;\n> > > >\n> > > > -\tV4L2SubdeviceFormat sensorFormat_;\n> > > > +\tStreamConfiguration sensorFormat_;\n> > > > \tstd::vector streams_;\n> > > > };\n> > > >\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> > > > @@ -325,32 +317,21 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > > > \t\tstatus = Adjusted;\n> > > > \t}\n> > > >\n> > > > -\t/*\n> > > > -\t * Select the sensor format by collecting the maximum width and height\n> > > > -\t * and picking the closest larger match, as the IPU3 can downscale\n> > > > -\t * only. If no resolution is requested for any stream, or if no sensor\n> > > > -\t * resolution is large enough, pick the largest one.\n> > > > -\t */\n> > > > +\t/* Find largets size and raw format (if any) in the configuration. */\n> > > > \tSize size = {};\n> > > > -\n> > > > +\tPixelFormat pixelFormat = {};\n> > > > \tfor (const StreamConfiguration &cfg : config_) {\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> > > > +\n> > > > +\t\tif (cfg.pixelFormat.modifier() == IPU3_FORMAT_MOD_PACKED)\n> > > > +\t\t\tpixelFormat = cfg.pixelFormat;\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> > > > -\n> > > > +\t/* Generate raw configuration from CIO2. */\n> > > > +\tsensorFormat_ = data_->cio2_.generateConfiguration(pixelFormat, size);\n> > > >\n> > > > \t/* Assign streams to each configuration entry. */\n> > > > \tif (updateStreams())\n> > > > @@ -363,13 +344,7 @@ 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 = sensorFormat_;\n> > > > \t\t} else {\n> > > > \t\t\tbool scale = stream == &data_->vfStream_;\n> > > > \t\t\tadjustStream(config_[i], scale);\n> > > > @@ -452,17 +427,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> > > > \t\t\t}\n> > > >\n> > > > \t\t\tstream = &data->rawStream_;\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();\n> > > > \t\t\tbreak;\n> > > > \t\t}\n> > > >","headers":{"Return-Path":"","Received":["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 981CD61167\n\tfor ;\n\tSat, 6 Jun 2020 19:04:08 +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 0388E30D;\n\tSat, 6 Jun 2020 19:04:07 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"SaX0bOC1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1591463048;\n\tbh=UBaxgyUCc//jICQyj8Q00BcrJFB2EVla1Z0mdSwDYbM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SaX0bOC1sGbjxloMuGgJSn8WYyVrXZU/FJjGfCCrDlOwYVCA/gwAKiSupx82P/uqC\n\tP3XFHE79TINHZJcGyGOTvFv4stBNcPMcuTBcQiQm7s0CiUkhrmhWHIvDm1n6P18iZe\n\tR4iysF2RojSZGk1/PPEPI2ZVBJplJFwfWL63MqJU=","Date":"Sat, 6 Jun 2020 20:03:49 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Message-ID":"<20200606170349.GB7339@pendragon.ideasonboard.com>","References":"<20200602013909.3170593-1-niklas.soderlund@ragnatech.se>\n\t<20200602013909.3170593-8-niklas.soderlund@ragnatech.se>\n\t<20200602120837.e75sxsg274loe24w@uno.localdomain>\n\t<20200605220634.GK26752@pendragon.ideasonboard.com>\n\t<20200606135550.GA395571@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200606135550.GA395571@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH 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":"Sat, 06 Jun 2020 17:04:08 -0000"}},{"id":5124,"web_url":"https://patchwork.libcamera.org/comment/5124/","msgid":"<20200608083030.sxin3nrugg45myou@uno.localdomain>","date":"2020-06-08T08:30:30","subject":"Re: [libcamera-devel] [PATCH 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 Sat, Jun 06, 2020 at 03:55:50PM +0200, Niklas Söderlund wrote:\n> Hello,\n>\n> On 2020-06-06 01:06:34 +0300, Laurent Pinchart wrote:\n> > Hi Niklas,\n> >\n> > Thank you for the patch.\n> >\n> > On Tue, Jun 02, 2020 at 02:08:37PM +0200, Jacopo Mondi wrote:\n> > > On Tue, Jun 02, 2020 at 03:39:06AM +0200, Niklas Söderlund wrote:\n> > > > Collect the code used to generate configurations for the CIO2 block in\n> > > > the CIO2Device class. This allows for both simplifying the code while\n> > > > extending it's functionality. With this change applications can now\n> >\n> > s/it's/its/\n> >\n> > > > switch which Bayer format pattern are used instead being more or less\n> > > > forced to use SBGGR10.\n> >\n> > Isn't the Bayer pattern fixed by the sensor ?\n>\n> It is, what I tried to expressed is that other pixelformats then SBGGR10\n> is now possible to select from the application, will update.\n>\n> >\n> > > > Signed-off-by: Niklas Söderlund \n> > > > ---\n> > > > src/libcamera/pipeline/ipu3/cio2.cpp | 44 +++++++++++++++++++++\n> > > > src/libcamera/pipeline/ipu3/cio2.h | 5 +++\n> > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 57 ++++++----------------------\n> > > > 3 files changed, 60 insertions(+), 46 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > > index 113486e3e3d0f2f1..2263d6530ec6b672 100644\n> > > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > > > @@ -11,6 +11,13 @@ namespace libcamera {\n> > > >\n> > > > LOG_DECLARE_CATEGORY(IPU3)\n> > > >\n> > > > +static const std::map sensorMbusToPixel = {\n> > > > +\t{ MEDIA_BUS_FMT_SBGGR10_1X10, PixelFormat(DRM_FORMAT_SBGGR10, IPU3_FORMAT_MOD_PACKED) },\n> > > > +\t{ MEDIA_BUS_FMT_SGBRG10_1X10, PixelFormat(DRM_FORMAT_SGBRG10, IPU3_FORMAT_MOD_PACKED) },\n> > > > +\t{ MEDIA_BUS_FMT_SGRBG10_1X10, PixelFormat(DRM_FORMAT_SGRBG10, IPU3_FORMAT_MOD_PACKED) },\n> > > > +\t{ MEDIA_BUS_FMT_SRGGB10_1X10, PixelFormat(DRM_FORMAT_SRGGB10, IPU3_FORMAT_MOD_PACKED) },\n> > > > +};\n> > > > +\n> > >\n> > > What about an anonymous namespace instead of static ?\n> >\n> > That's what we tend to use, yes.\n> >\n> > > > /**\n> > > > * \\brief Initialize components of the CIO2 device with \\a index\n> > > > * \\param[in] media The CIO2 media device\n> > > > @@ -153,6 +160,43 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)\n> > > > \treturn 0;\n> > > > }\n> > >\n> > > No documentation ?\n> > > src/libcamera/pipeline/ is excluded from the Doxyegen search path, but\n> > > I would like to see documentation anyhow, especially about the\n> > > (optional) parameters.\n> > >\n> > > > +StreamConfiguration\n> > > > +CIO2Device::generateConfiguration(const PixelFormat desiredPixelFormat,\n> > > > +\t\t\t\t const Size desiredSize) const\n> > >\n> > > Is generateConfiguration the correct name ? That's the Camera and\n> > > pipeline handler API name, while this is internal to the IPU3\n> > > machinery. I don't have much better ideas, so feel free to keep it if\n> > > you are not concerned about the name clash.\n> > >\n> > > > +{\n> > > > +\tStreamConfiguration cfg;\n> > > > +\n> > > > +\t/* If no desired pixelformat allow all supported.*/\n> > >\n> > > s/all supported/all supported ones/\n> >\n> > \"all *the* supported ones\"\n> >\n> > > Also, missing space at the end\n> > >\n> > > > +\tstd::vector mbusCodes = {\n> > >\n> > > You should use array<> as the size is fixed. Also, const.\n>\n> It can't be const as it's (possibly) modified bellow.\n>\n> > >\n> > > > +\t\tMEDIA_BUS_FMT_SBGGR10_1X10,\n> > > > +\t\tMEDIA_BUS_FMT_SGBRG10_1X10,\n> > > > +\t\tMEDIA_BUS_FMT_SGRBG10_1X10,\n> > > > +\t\tMEDIA_BUS_FMT_SRGGB10_1X10\n> > > > +\t};\n> > > > +\tif (desiredPixelFormat.isValid()) {\n> > > > +\t\tfor (const auto &iter : sensorMbusToPixel) {\n> > > > +\t\t\tif (iter.second == desiredPixelFormat) {\n> > > > +\t\t\t\tmbusCodes = { iter.first };\n> > >\n> > > Ah no, you can't use array<>\n> > >\n> > > However this code doesn't make sure you actually find any matching\n> > > format, and if you don't application has requested an unsupported raw\n> > > format, so you should fail loudly here imo.\n>\n> If no match is found the mbusCodes is not modified and all 4 possible\n> mbus codes are considered when probing the sensor for a format. No need\n> to warn IMHO.\n>\n\nActually, to me, if the user requires something, and it gets silently\ndefaulted to something else, that's an issue which is worth signaling\nout. Not sure how often it happens here, but to me if the user is\ngiven somehing different from what it has requested, it should be\nsignaled. I would say the function should fail in that case, but maybe\nit's too harsh ?\n\n> > >\n> > > > +\t\t\t\tbreak;\n> > > > +\t\t\t}\n> > > > +\t\t}\n> > > > +\t}\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> > >\n> > > Which static information ? You get the current configured format and\n> > > use it...\n>\n> Not true, CameraSensor::getFormat() returns the best match for the mbus\n> code and size from the static/cached information it enumerated in\n> CameraSensor::init().\n\nOk, got confused from the 'static' in the comment.\n\n>\n> > >\n> > > > +\tV4L2SubdeviceFormat sensorFormat = sensor_->getFormat(mbusCodes, size);\n> >\n> > If the requested format isn't supported by the sensor, this function\n> > will return a default constructed V4L2SubdeviceFormat...\n>\n> Good point, will fix.\n\nI wonder if this is a good API in first place.\nWouldn't returning an integer and accepting an input\nV4L2SubdeviceFormat * make failues in V4L2Subdevice::getFormat() harder\nto overlook ?\n\n>\n> >\n> > > > +\n> > > > +\tcfg.size = sensorFormat.size;\n> > > > +\tcfg.pixelFormat = sensorMbusToPixel.at(sensorFormat.mbus_code);\n> >\n> > ... and this will throw an exception.\n> >\n> > > > +\tcfg.bufferCount = CIO2_BUFFER_COUNT;\n> > > > +\n> > > > +\treturn cfg;\n> > > > +}\n> >\n> > I don't think this belongs to the CIO2Device class, as it has nothing to\n> > do with the CIO2. As Jacopo commented separately, the CIO2Device class\n> > should focus on the CIO2. The sensor should be handled by the top-level\n> > pipeline handler.\n>\n> As discussed on IRC I disagree wit this. I think the sensor belongs\n> inside the CIO2Device as it's part of the CIO2 media graph. Handling it\n> outside the CIO2Device will IMHO only create a spaghetti mess similar to\n> the one we already have where all objects in the IPU3 pipeline access\n> member variables all over the place.\n>\n> I will for v2 remove the proxy helpers to access the sensor and replace\n> it with a CIO2Device::sensor() in hops this will suite Jacopos and your\n> concerns, lets see how it plays out :-)\n>\n\nI understand the reasoning about the fact they belong to same media\ngraph. I really disliked accessors, as they created an abstraction\nwhich imho hides where those information come from. Accessing the\nsensor * and retreiveing information from there, I'm not opposed.\n\n> >\n> > I would by the way also split the ImgUDevice class to a separate file.\n>\n> I plan to do so on top of this series, as mentioned in the cover letter.\n>\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 d923038bb4ba356f..2e268a7154b2d241 100644\n> > > > --- a/src/libcamera/pipeline/ipu3/cio2.h\n> > > > +++ b/src/libcamera/pipeline/ipu3/cio2.h\n> > > > @@ -12,6 +12,8 @@\n> > > >\n> > > > #include \n> > > >\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> > > > @@ -39,6 +41,9 @@ public:\n> > > > \tint init(const MediaDevice *media, unsigned int index);\n> > > > \tint configure(const Size &size, V4L2DeviceFormat *outputFormat);\n> > > >\n> > > > +\tStreamConfiguration generateConfiguration(const PixelFormat desiredPixelFormat = {},\n> > > > +\t\t\t\t\t\t const Size desiredSize = {}) const;\n> > > > +\n> > >\n> > > This could probably be simplified in its implementation by providing\n> > > two overloaded methods instead of defaulting parameters and\n> > > mix-matching them in the implementation (and default parameters in C++\n> > > are a pain to follow, as they are declared as optional in the class\n> > > declaration only).\n>\n> I slightly prefer default arguments over multiple implementations, but\n> it's not a strong feeling. But if we are to drop default arguments lets\n> do so as part of a series that address the issue everywhere.\n>\n\nI'm not against default parameters in general, but if their useage\nmakes the function flow like\n\n if (param1 is ok) {\n if (param2 is ok) {{\n\n } else {\n\n }\n } else {\n\n }\n\nIt means to me these are actually two logically different functions\nmashed up together. This case is not that bad as you get away with\njust 2 ifs, but to me it's harder to follow than two separate\nfunctions (one of which might just be a wrapper that defauls the mbus\ncodes if not provided). Up to you.\n\nThanks\n j\n\n\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 2047deac299dbf75..56cc3ca10414f0d2 100644\n> > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > @@ -35,13 +35,6 @@ LOG_DEFINE_CATEGORY(IPU3)\n> > > >\n> > > > class IPU3CameraData;\n> > > >\n> > > > -static const std::map sensorMbusToPixel = {\n> > > > -\t{ MEDIA_BUS_FMT_SBGGR10_1X10, PixelFormat(DRM_FORMAT_SBGGR10, IPU3_FORMAT_MOD_PACKED) },\n> > > > -\t{ MEDIA_BUS_FMT_SGBRG10_1X10, PixelFormat(DRM_FORMAT_SGBRG10, IPU3_FORMAT_MOD_PACKED) },\n> > > > -\t{ MEDIA_BUS_FMT_SGRBG10_1X10, PixelFormat(DRM_FORMAT_SGRBG10, IPU3_FORMAT_MOD_PACKED) },\n> > > > -\t{ MEDIA_BUS_FMT_SRGGB10_1X10, PixelFormat(DRM_FORMAT_SRGGB10, IPU3_FORMAT_MOD_PACKED) },\n> > > > -};\n> > > > -\n> > > > class ImgUDevice\n> > > > {\n> > > > public:\n> > > > @@ -146,7 +139,7 @@ public:\n> > > >\n> > > > \tStatus validate() override;\n> > > >\n> > > > -\tconst V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }\n> > > > +\tconst StreamConfiguration &sensorFormat() const { return sensorFormat_; };\n> > > > \tconst std::vector &streams() { return streams_; }\n> > > >\n> > > > private:\n> > > > @@ -164,7 +157,7 @@ private:\n> > > > \tstd::shared_ptr camera_;\n> > > > \tconst IPU3CameraData *data_;\n> > > >\n> > > > -\tV4L2SubdeviceFormat sensorFormat_;\n> > > > +\tStreamConfiguration sensorFormat_;\n> > > > \tstd::vector streams_;\n> > > > };\n> > > >\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> > > > @@ -325,32 +317,21 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > > > \t\tstatus = Adjusted;\n> > > > \t}\n> > > >\n> > > > -\t/*\n> > > > -\t * Select the sensor format by collecting the maximum width and height\n> > > > -\t * and picking the closest larger match, as the IPU3 can downscale\n> > > > -\t * only. If no resolution is requested for any stream, or if no sensor\n> > > > -\t * resolution is large enough, pick the largest one.\n> > > > -\t */\n> > > > +\t/* Find largets size and raw format (if any) in the configuration. */\n> > > > \tSize size = {};\n> > > > -\n> > > > +\tPixelFormat pixelFormat = {};\n> > > > \tfor (const StreamConfiguration &cfg : config_) {\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> > > > +\n> > > > +\t\tif (cfg.pixelFormat.modifier() == IPU3_FORMAT_MOD_PACKED)\n> > > > +\t\t\tpixelFormat = cfg.pixelFormat;\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> > > > -\n> > > > +\t/* Generate raw configuration from CIO2. */\n> > > > +\tsensorFormat_ = data_->cio2_.generateConfiguration(pixelFormat, size);\n> > > >\n> > > > \t/* Assign streams to each configuration entry. */\n> > > > \tif (updateStreams())\n> > > > @@ -363,13 +344,7 @@ 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 = sensorFormat_;\n> > > > \t\t} else {\n> > > > \t\t\tbool scale = stream == &data_->vfStream_;\n> > > > \t\t\tadjustStream(config_[i], scale);\n> > > > @@ -452,17 +427,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> > > > \t\t\t}\n> > > >\n> > > > \t\t\tstream = &data->rawStream_;\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();\n> > > > \t\t\tbreak;\n> > > > \t\t}\n> > > >\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\n>\n> --\n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"","Received":["from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6E37B603C7\n\tfor ;\n\tMon, 8 Jun 2020 10:27:08 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 12ED34001B;\n\tMon, 8 Jun 2020 08:27:05 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 8 Jun 2020 10:30:30 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20200608083030.sxin3nrugg45myou@uno.localdomain>","References":"<20200602013909.3170593-1-niklas.soderlund@ragnatech.se>\n\t<20200602013909.3170593-8-niklas.soderlund@ragnatech.se>\n\t<20200602120837.e75sxsg274loe24w@uno.localdomain>\n\t<20200605220634.GK26752@pendragon.ideasonboard.com>\n\t<20200606135550.GA395571@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200606135550.GA395571@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH 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":"Mon, 08 Jun 2020 08:27:08 -0000"}}]