[{"id":5097,"web_url":"https://patchwork.libcamera.org/comment/5097/","msgid":"<20200606213852.GF7339@pendragon.ideasonboard.com>","date":"2020-06-06T21:38:52","subject":"Re: [libcamera-devel] [PATCH v2 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 Sat, Jun 06, 2020 at 05:04:33PM +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 its functionality. With this change applications can now\n> function with pixelformats other then SBGGR10 which previously was hard\n> coded.\n> \n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\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 | 55 ++++++++++++++++++++++++++\n>  src/libcamera/pipeline/ipu3/cio2.h   |  8 +++-\n>  src/libcamera/pipeline/ipu3/ipu3.cpp | 58 +++++++---------------------\n>  3 files changed, 75 insertions(+), 46 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> index 1a9f185bc4e161c5..0d961ae8f5a0682b 100644\n> --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> @@ -9,6 +9,8 @@\n>  \n>  #include <linux/media-bus-format.h>\n>  \n> +#include <libcamera/stream.h>\n> +\n>  #include \"libcamera/internal/camera_sensor.h\"\n>  #include \"libcamera/internal/media_device.h\"\n>  #include \"libcamera/internal/v4l2_subdevice.h\"\n> @@ -18,6 +20,17 @@ namespace libcamera {\n>  \n>  LOG_DECLARE_CATEGORY(IPU3)\n>  \n> +namespace {\n> +\n> +const std::map<uint32_t, PixelFormat> 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> +} /* namespace */\n> +\n>  CIO2Device::CIO2Device()\n>  \t: output_(nullptr), csi2_(nullptr), sensor_(nullptr)\n>  {\n> @@ -172,6 +185,48 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)\n>  \treturn 0;\n>  }\n>  \n> +StreamConfiguration\n> +CIO2Device::generateConfiguration(const PixelFormat desiredPixelFormat,\n> +\t\t\t\t  const Size desiredSize) const\n\nI think Jacopo mentioned he would like this function to be documented. I\nthink it's a good idea in general, even if the documentation is ignored\nby Doxygen, as the code may serve as a reference implementation.\n\n> +{\n> +\tStreamConfiguration cfg;\n> +\n> +\t/* If no desired pixelformat allow all the supported ones. */\n> +\tstd::vector<unsigned int> mbusCodes = {\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> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n\nI still don't really see the point of this. Maybe I'm missing something\nobvious, but given that the sensor has a hardcoded Bayer pattern,\nwhat's the point in trying to select a particular desired pattern ? What\ncould be selected is the number of bits per pixels, but we only support\n10bpp for now.\n\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> +\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 = 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 b2c4f89d682d6cfb..b7eb69a4e104f400 100644\n> --- a/src/libcamera/pipeline/ipu3/cio2.h\n> +++ b/src/libcamera/pipeline/ipu3/cio2.h\n> @@ -11,6 +11,9 @@\n>  #include <queue>\n>  #include <vector>\n>  \n> +#include <libcamera/geometry.h>\n> +#include <libcamera/pixel_format.h>\n> +\n>  namespace libcamera {\n>  \n>  class CameraSensor;\n> @@ -19,7 +22,7 @@ class MediaDevice;\n>  class V4L2DeviceFormat;\n>  class V4L2Subdevice;\n>  class V4L2VideoDevice;\n> -struct Size;\n> +struct StreamConfiguration;\n>  \n>  class CIO2Device\n>  {\n> @@ -32,6 +35,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\nIf the pixel format isn't a reference, const isn't required.\n\n> +\t\t\t\t\t\t  const Size desiredSize = {}) const;\n\nShouldn't you pass a reference to Size ? You could then keep the forward\ndeclaration of Size.\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 257f5441c9ad7f5d..85d4e64396e77222 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<uint32_t, PixelFormat> 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<const IPU3Stream *> &streams() { return streams_; }\n>  \n>  private:\n> @@ -164,7 +157,7 @@ private:\n>  \tstd::shared_ptr<Camera> camera_;\n>  \tconst IPU3CameraData *data_;\n>  \n> -\tV4L2SubdeviceFormat sensorFormat_;\n> +\tStreamConfiguration sensorFormat_;\n\nThat seems an abuse of StreamConfiguration :-( You should at the very\nleast rename the variable, both because it's not a format anymore, and\nbecause it's the CIO2 output format, not the sensor format. The\nsensorFormat() function should also be renamed accordingly.\n\n>  \tstd::vector<const IPU3Stream *> streams_;\n>  };\n>  \n> @@ -304,7 +297,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> @@ -316,31 +308,23 @@ 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\nNo need for explicit initialization of pixelFormat (or size, for that\nmatter).\n\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> +\t/* Generate raw configuration from CIO2. */\n> +\tsensorFormat_ = data_->cio2_.generateConfiguration(pixelFormat, size);\n> +\tif (!sensorFormat_.pixelFormat.isValid())\n> +\t\treturn Invalid;\n>  \n>  \t/* Assign streams to each configuration entry. */\n>  \tassignStreams();\n> @@ -352,13 +336,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> @@ -442,17 +420,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":"<laurent.pinchart@ideasonboard.com>","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 44F1B61167\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat,  6 Jun 2020 23:39:12 +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 A628A30D;\n\tSat,  6 Jun 2020 23:39:11 +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=\"hGn4Gxt0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1591479551;\n\tbh=1ax+Y0tLxPc2MZJGti1w8iOoxbqSImGJ6jSatKlAxGA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hGn4Gxt0Z0PHm3EyrC2ceUljsonRO+LTTrlrZY/APCl7VokRTRCG5uZMESzZzOm4k\n\txJfkTKK8bRlY2przs/PnpEH9JWOX3UZwqxQ8Gj+/NUaaolwxio6YZGzr7GqLOW5cve\n\tYzPBQIpSWOA+VlQhnnQq5cKPFiEwe+TE5Hy/GN/0=","Date":"Sun, 7 Jun 2020 00:38:52 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200606213852.GF7339@pendragon.ideasonboard.com>","References":"<20200606150436.1851700-1-niklas.soderlund@ragnatech.se>\n\t<20200606150436.1851700-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":"<20200606150436.1851700-8-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v2 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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Sat, 06 Jun 2020 21:39:12 -0000"}}]