[{"id":25793,"web_url":"https://patchwork.libcamera.org/comment/25793/","msgid":"","date":"2022-11-14T19:06:51","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: pipeline: Add IMX8 ISI\n\tpipeline","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Thu, Nov 10, 2022 at 08:14:33PM +0100, Jacopo Mondi via libcamera-devel wrote:\n> Add a pipeline handler for the ISI capture interface found on\n> several versions of the i.MX8 SoC generation.\n> \n> The pipeline handler supports capturing multiple streams from the same\n> camera in YUV, RGB or RAW formats. The number of streams is limited by\n> the number of ISI pipelines, and is currently hardcoded to 2 as the code\n> has been tested on the i.MX8MP only. Further development will make this\n> dynamic to support other SoCs.\n> \n> Signed-off-by: Jacopo Mondi \n> Reviewed-by: Kieran Bingham \n> ---\n> meson_options.txt | 2 +-\n> src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 957 +++++++++++++++++++\n> src/libcamera/pipeline/imx8-isi/meson.build | 5 +\n> 3 files changed, 963 insertions(+), 1 deletion(-)\n> create mode 100644 src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> create mode 100644 src/libcamera/pipeline/imx8-isi/meson.build\n> \n> diff --git a/meson_options.txt b/meson_options.txt\n> index f1d678089452..1ba6778ce257 100644\n> --- a/meson_options.txt\n> +++ b/meson_options.txt\n> @@ -37,7 +37,7 @@ option('lc-compliance',\n> \n> option('pipelines',\n> type : 'array',\n> - choices : ['ipu3', 'raspberrypi', 'rkisp1', 'simple', 'uvcvideo', 'vimc'],\n> + choices : ['imx8-isi', 'ipu3', 'raspberrypi', 'rkisp1', 'simple', 'uvcvideo', 'vimc'],\n> description : 'Select which pipeline handlers to include')\n> \n> option('qcam',\n> diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> new file mode 100644\n> index 000000000000..6e4b1ae290ef\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> @@ -0,0 +1,957 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2022 - Jacopo Mondi \n> + *\n> + * imx8-isi.cpp - Pipeline handler for ISI interface found on NXP i.MX8 SoC\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/bayer_format.h\"\n> +#include \"libcamera/internal/camera.h\"\n> +#include \"libcamera/internal/camera_sensor.h\"\n> +#include \"libcamera/internal/device_enumerator.h\"\n> +#include \"libcamera/internal/media_device.h\"\n> +#include \"libcamera/internal/pipeline_handler.h\"\n> +#include \"libcamera/internal/v4l2_subdevice.h\"\n> +#include \"libcamera/internal/v4l2_videodevice.h\"\n> +\n> +#include \"linux/media-bus-format.h\"\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(ISI)\n> +\n> +class PipelineHandlerISI;\n> +\n> +class ISICameraData : public Camera::Private\n> +{\n> +public:\n> +\tISICameraData(PipelineHandler *ph)\n> +\t\t: Camera::Private(ph)\n> +\t{\n> +\t\t/*\n> +\t\t * \\todo Assume 2 channels only for now, as that's the number of\n> +\t\t * available channels on i.MX8MP.\n> +\t\t */\n> +\t\tstreams_.resize(2);\n> +\t}\n> +\n> +\tPipelineHandlerISI *pipe();\n> +\n> +\tint init();\n> +\n> +\tunsigned int pipeIndex(const Stream *stream)\n> +\t{\n> +\t\treturn stream - &*streams_.begin();\n> +\t}\n> +\n> +\tstd::unique_ptr sensor_;\n> +\tstd::unique_ptr csis_;\n> +\n> +\tstd::vector streams_;\n> +\n> +\tstd::vector enabledStreams_;\n> +\n> +\tunsigned int xbarSink_;\n> +};\n> +\n> +class ISICameraConfiguration : public CameraConfiguration\n> +{\n> +public:\n> +\t/*\n> +\t * formatsMap_ records the association between an output pixel format\n> +\t * and the combination of V4L2 pixel format and media bus codes that have\n> +\t * to be applied to the pipeline.\n> +\t */\n> +\tstruct PipeFormat {\n> +\t\tunsigned int isiCode;\n> +\t\tunsigned int sensorCode;\n> +\t};\n> +\n> +\tusing FormatMap = std::map;\n> +\n> +\tISICameraConfiguration(ISICameraData *data)\n> +\t\t: data_(data)\n> +\t{\n> +\t}\n> +\n> +\tStatus validate() override;\n> +\n> +\tstatic const FormatMap formatsMap_;\n> +\n> +\tV4L2SubdeviceFormat sensorFormat_;\n> +\n> +private:\n> +\tconst ISICameraData *data_;\n> +\n> +\tCameraConfiguration::Status validateRaw(std::set &availableStreams,\n> +\t\t\t\t\t\tconst Size &maxResolution);\n> +\tCameraConfiguration::Status validateYuv(std::set &availableStreams,\n> +\t\t\t\t\t\tconst Size &maxResolution);\n\nFunctions should go before data.\n\n> +};\n> +\n> +class PipelineHandlerISI : public PipelineHandler\n> +{\n> +public:\n> +\tPipelineHandlerISI(CameraManager *manager);\n> +\n> +\tbool match(DeviceEnumerator *enumerator) override;\n> +\n> +\tstd::unique_ptr generateConfiguration(Camera *camera,\n> +\t\t\t\t\t\t\t\t const StreamRoles &roles) override;\n\nYou can also fold it as\n\n\tstd::unique_ptr\n\tgenerateConfiguration(Camera *camera, const StreamRoles &roles) override;\n\nto reduce the line length.\n\n> +\tint configure(Camera *camera, CameraConfiguration *config) override;\n> +\n> +\tint exportFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t std::vector> *buffers) override;\n> +\n> +\tint start(Camera *camera, const ControlList *controls) override;\n> +\n> +protected:\n> +\tvoid stopDevice(Camera *camera) override;\n> +\n> +\tint queueRequestDevice(Camera *camera, Request *request) override;\n> +\n> +private:\n> +\tstatic constexpr Size kPreviewSize = { 1920, 1080 };\n> +\tstatic constexpr Size kMinISISize = { 1, 1 };\n> +\n> +\tstruct Pipe {\n> +\t\tstd::unique_ptr isi;\n> +\t\tstd::unique_ptr capture;\n> +\t};\n> +\n> +\tISICameraData *cameraData(Camera *camera)\n> +\t{\n> +\t\treturn static_cast(camera->_d());\n> +\t}\n> +\n> +\tPipe *pipeFromStream(Camera *camera, const Stream *stream);\n> +\n> +\tvoid bufferReady(FrameBuffer *buffer);\n> +\n> +\tMediaDevice *isiDev_;\n> +\n> +\tstd::unique_ptr crossbar_;\n> +\tstd::vector pipes_;\n> +};\n> +\n> +/* -----------------------------------------------------------------------------\n> + * Camera Data\n> + */\n> +\n> +PipelineHandlerISI *ISICameraData::pipe()\n> +{\n> +\treturn static_cast(Camera::Private::pipe());\n> +}\n> +\n> +/* Open and initialize pipe components. */\n> +int ISICameraData::init()\n> +{\n> +\tint ret = sensor_->init();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = csis_->open();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tproperties_ = sensor_->properties();\n> +\n> +\treturn 0;\n> +}\n> +\n> +/* -----------------------------------------------------------------------------\n> + * Camera Configuration\n> + */\n> +\n> +const ISICameraConfiguration::FormatMap ISICameraConfiguration::formatsMap_ = {\n\nCould you add a todo comment to remind that the sensor format selection\nshould not be hardcoded ?\n\n> +\t{\n> +\t\tformats::YUV422,\n> +\t\t{ MEDIA_BUS_FMT_YUV8_1X24,\n> +\t\t MEDIA_BUS_FMT_UYVY8_1X16 },\n> +\t},\n> +\t{\n> +\t\tformats::YUYV,\n> +\t\t{ MEDIA_BUS_FMT_YUV8_1X24,\n> +\t\t MEDIA_BUS_FMT_UYVY8_1X16 },\n> +\t},\n> +\t{\n> +\t\tformats::RGB565,\n> +\t\t{ MEDIA_BUS_FMT_RGB888_1X24,\n> +\t\t MEDIA_BUS_FMT_RGB565_1X16 },\n> +\t},\n> +\t{\n> +\t\tformats::SBGGR8,\n> +\t\t{ MEDIA_BUS_FMT_SBGGR8_1X8,\n> +\t\t MEDIA_BUS_FMT_SBGGR8_1X8 },\n> +\t},\n> +\t{\n> +\t\tformats::SGBRG8,\n> +\t\t{ MEDIA_BUS_FMT_SGBRG8_1X8,\n> +\t\t MEDIA_BUS_FMT_SGBRG8_1X8 },\n> +\t},\n> +\t{\n> +\t\tformats::SGRBG8,\n> +\t\t{ MEDIA_BUS_FMT_SGRBG8_1X8,\n> +\t\t MEDIA_BUS_FMT_SGRBG8_1X8 },\n> +\t},\n> +\t{\n> +\t\tformats::SRGGB8,\n> +\t\t{ MEDIA_BUS_FMT_SRGGB8_1X8,\n> +\t\t MEDIA_BUS_FMT_SRGGB8_1X8 },\n> +\t},\n> +\t{\n> +\t\tformats::SBGGR10,\n> +\t\t{ MEDIA_BUS_FMT_SBGGR10_1X10,\n> +\t\t MEDIA_BUS_FMT_SBGGR10_1X10 },\n> +\t},\n> +\t{\n> +\t\tformats::SGBRG10,\n> +\t\t{ MEDIA_BUS_FMT_SGBRG10_1X10,\n> +\t\t MEDIA_BUS_FMT_SGBRG10_1X10 },\n> +\t},\n> +\t{\n> +\t\tformats::SGRBG10,\n> +\t\t{ MEDIA_BUS_FMT_SGRBG10_1X10,\n> +\t\t MEDIA_BUS_FMT_SGRBG10_1X10 },\n> +\t},\n> +\t{\n> +\t\tformats::SRGGB10,\n> +\t\t{ MEDIA_BUS_FMT_SRGGB10_1X10,\n> +\t\t MEDIA_BUS_FMT_SRGGB10_1X10 },\n> +\t},\n> +\t{\n> +\t\tformats::SBGGR12,\n> +\t\t{ MEDIA_BUS_FMT_SBGGR12_1X12,\n> +\t\t MEDIA_BUS_FMT_SBGGR12_1X12 },\n> +\t},\n> +\t{\n> +\t\tformats::SGBRG12,\n> +\t\t{ MEDIA_BUS_FMT_SGBRG12_1X12,\n> +\t\t MEDIA_BUS_FMT_SGBRG12_1X12 },\n> +\t},\n> +\t{\n> +\t\tformats::SGRBG12,\n> +\t\t{ MEDIA_BUS_FMT_SGRBG12_1X12,\n> +\t\t MEDIA_BUS_FMT_SGRBG12_1X12 },\n> +\t},\n> +\t{\n> +\t\tformats::SRGGB12,\n> +\t\t{ MEDIA_BUS_FMT_SRGGB12_1X12,\n> +\t\t MEDIA_BUS_FMT_SRGGB12_1X12 },\n> +\t},\n> +};\n> +\n> +/*\n> + * Adjust stream configuration when the first requested stream is RAW: all the\n> + * streams will have the same RAW pixelformat and size.\n> + */\n> +CameraConfiguration::Status ISICameraConfiguration::validateRaw(std::set &availableStreams,\n> +\t\t\t\t\t\t\t\tconst Size &maxResolution)\n\nCameraConfiguration::Status\nISICameraConfiguration::validateRaw(std::set &availableStreams,\n\t\t\t\t const Size &maxResolution)\n\n> +{\n> +\tCameraConfiguration::Status status = Valid;\n> +\n> +\t/*\n> +\t * Make sure the requested RAW format is supported by the\n> +\t * pipeline, otherwise adjust it.\n> +\t */\n> +\tstd::vector mbusCodes = data_->sensor_->mbusCodes();\n> +\tStreamConfiguration &rawConfig = config_[0];\n> +\n> +\tbool supported = false;\n> +\tauto it = formatsMap_.find(rawConfig.pixelFormat);\n> +\tif (it != formatsMap_.end()) {\n> +\t\tunsigned int mbusCode = it->second.sensorCode;\n> +\n> +\t\tif (std::count(mbusCodes.begin(), mbusCodes.end(), mbusCode))\n> +\t\t\tsupported = true;\n> +\t}\n> +\n> +\tif (!supported) {\n> +\t\t/*\n> +\t\t * Adjust to the first mbus code supported by both the\n> +\t\t * sensor and the pipeline.\n> +\t\t */\n> +\t\tconst FormatMap::value_type *pipeConfig = nullptr;\n> +\t\tfor (unsigned int code : data_->sensor_->mbusCodes()) {\n\nYou can reuse the mbusCodes variables.\n\n> +\t\t\tconst BayerFormat &bayerFormat = BayerFormat::fromMbusCode(code);\n> +\t\t\tif (!bayerFormat.isValid())\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tauto fmt = std::find_if(ISICameraConfiguration::formatsMap_.begin(),\n> +\t\t\t\t\t\tISICameraConfiguration::formatsMap_.end(),\n> +\t\t\t\t\t\t[code](auto &isiFormat) {\n\nconst auto\n\n> +\t\t\t\t\t\t\tauto &pipe = isiFormat.second;\n\nSame here.\n\n> +\t\t\t\t\t\t\treturn pipe.sensorCode == code;\n> +\t\t\t\t\t\t});\n> +\n> +\t\t\tif (fmt == ISICameraConfiguration::formatsMap_.end())\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tpipeConfig = &(*fmt);\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tif (!pipeConfig) {\n> +\t\t\tLOG(ISI, Error) << \"Cannot adjust RAW format \"\n> +\t\t\t\t\t<< rawConfig.pixelFormat;\n> +\t\t\treturn Invalid;\n> +\t\t}\n> +\n> +\t\trawConfig.pixelFormat = pipeConfig->first;\n> +\t\tLOG(ISI, Debug) << \"RAW pixelformat adjusted to \"\n> +\t\t\t\t<< pipeConfig->first;\n> +\t\tstatus = Adjusted;\n> +\t}\n> +\n> +\t/* Cap the RAW stream size to the maximum resolution. */\n> +\tSize configSize = rawConfig.size;\n\nconst\n\n> +\trawConfig.size.boundTo(maxResolution);\n\nI don't think this is enough, not all sizes smaller than the sensor\nresolution can be achieved as the pipeline can't scale raw formats. It\ncan be addressed on top.\n\n> +\tif (rawConfig.size != configSize) {\n> +\t\tLOG(ISI, Debug) << \"RAW size adjusted to \"\n> +\t\t\t\t<< rawConfig.size;\n> +\t\tstatus = Adjusted;\n> +\t}\n> +\n> +\t/* Adjust all other streams to RAW. */\n> +\tunsigned int i = 0;\n> +\tfor (StreamConfiguration &cfg : config_) {\n\n\tfor (auto &[i, cfg] : utils::enumerate(config_)) {\n\n> +\n> +\t\tLOG(ISI, Debug) << \"Stream \" << i << \": \" << cfg.toString();\n> +\t\tPixelFormat pixFmt = cfg.pixelFormat;\n> +\t\tSize size = cfg.size;\n\nconst for both.\n\n> +\n> +\t\tcfg.pixelFormat = rawConfig.pixelFormat;\n> +\t\tcfg.size = rawConfig.size;\n> +\n> +\t\tif (cfg.pixelFormat != pixFmt ||\n> +\t\t cfg.size != size) {\n\nThis holds on a single line.\n\n> +\t\t\tLOG(ISI, Debug) << \"Stream \" << i << \" adjusted to \"\n> +\t\t\t\t\t<< cfg.toString();\n> +\t\t\tstatus = Adjusted;\n> +\t\t}\n> +\n> +\t\tPixelFormatInfo info = PixelFormatInfo::info(cfg.pixelFormat);\n\nconst reference. It would be nice to disable copy and move for the\nPixelFormatInfo class.\n\n> +\t\tcfg.stride = info.stride(cfg.size.width, 0);\n> +\t\tcfg.frameSize = info.frameSize(cfg.size, info.bitsPerPixel);\n> +\n> +\t\t/* Assign streams in the order they are presented. */\n> +\t\tauto stream = availableStreams.extract(availableStreams.begin());\n> +\t\tcfg.setStream(stream.value());\n> +\n> +\t\ti++;\n> +\t}\n> +\n> +\treturn status;\n> +}\n> +\n> +/*\n> + * Adjust stream configuration when the first requested stream is not RAW: all\n> + * the streams will be either YUV or RGB processed formats.\n> + */\n> +CameraConfiguration::Status ISICameraConfiguration::validateYuv(std::set &availableStreams,\n> +\t\t\t\t\t\t\t\tconst Size &maxResolution)\n\nCameraConfiguration::Status\nISICameraConfiguration::validateYuv(std::set &availableStreams,\n\t\t\t\t const Size &maxResolution)\n\n> +{\n> +\tCameraConfiguration::Status status = Valid;\n> +\n> +\tunsigned int i = 0;\n> +\tfor (StreamConfiguration &cfg : config_) {\n\nutil::enumerate() here too.\n\n> +\n> +\t\tLOG(ISI, Debug) << \"Stream \" << i << \": \" << cfg.toString();\n> +\n> +\t\t/* If the stream is RAW or not supported default it to YUYV. */\n> +\t\tPixelFormatInfo info = PixelFormatInfo::info(cfg.pixelFormat);\n\nconst reference\n\n> +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW ||\n> +\t\t !formatsMap_.count(cfg.pixelFormat)) {\n> +\n> +\t\t\tLOG(ISI, Debug) << \"Stream \" << i << \" format: \"\n> +\t\t\t\t\t<< cfg.pixelFormat << \" adjusted to YUYV\";\n> +\n> +\t\t\tcfg.pixelFormat = formats::YUYV;\n> +\t\t\tstatus = Adjusted;\n> +\t\t}\n> +\n> +\t\t/* Cap the streams size to the maximum accepted resolution. */\n> +\t\tSize configSize = cfg.size;\n> +\t\tcfg.size.boundTo(maxResolution);\n> +\t\tif (cfg.size != configSize) {\n> +\t\t\tLOG(ISI, Debug)\n> +\t\t\t\t<< \"Stream \" << i << \" adjusted to \" << cfg.size;\n> +\t\t\tstatus = Adjusted;\n> +\t\t}\n> +\n> +\t\t/* Re-fetch the pixel format info in case it has been adjusted. */\n> +\t\tinfo = PixelFormatInfo::info(cfg.pixelFormat);\n> +\n> +\t\t/* \\todo Multiplane ? */\n> +\t\tcfg.stride = info.stride(cfg.size.width, 0);\n> +\t\tcfg.frameSize = info.frameSize(cfg.size, info.bitsPerPixel);\n> +\n> +\t\t/* Assign streams in the order they are presented. */\n> +\t\tauto stream = availableStreams.extract(availableStreams.begin());\n> +\t\tcfg.setStream(stream.value());\n> +\n> +\t\ti++;\n> +\t}\n> +\n> +\treturn status;\n> +}\n> +\n> +CameraConfiguration::Status ISICameraConfiguration::validate()\n> +{\n> +\tStatus status = Valid;\n> +\n> +\tstd::set availableStreams;\n> +\tstd::transform(data_->streams_.begin(), data_->streams_.end(),\n> +\t\t std::inserter(availableStreams, availableStreams.end()),\n> +\t\t [](const Stream &s) { return const_cast(&s); });\n> +\n> +\tif (config_.empty())\n> +\t\treturn Invalid;\n> +\n> +\t/* Cap the number of streams to the number of available ISI pipes. */\n> +\tif (config_.size() > availableStreams.size()) {\n> +\t\tconfig_.resize(availableStreams.size());\n> +\t\tstatus = Adjusted;\n> +\t}\n> +\n> +\t/*\n> +\t * If more than a single stream is requested, the maximum allowed input\n> +\t * image width is 2048. Cap the maximum image size accordingly.\n> +\t *\n> +\t * \\todo The (size > 1) check only applies to i.MX8MP which has 2 ISI\n> +\t * channels. SoCs with more channels than the i.MX8MP are capable of\n> +\t * supporting more streams with input width > 2048 by chaining\n> +\t * successive channels together. Define a policy for channels allocation\n> +\t * to fully support other SoCs.\n> +\t */\n> +\tCameraSensor *sensor = data_->sensor_.get();\n> +\tSize maxResolution = sensor->resolution();\n> +\tif (config_.size() > 1)\n> +\t\tmaxResolution.width = std::min(2048U, maxResolution.width);\n> +\n> +\t/* Validate streams according to the format of the first one. */\n> +\tconst PixelFormatInfo info = PixelFormatInfo::info(config_[0].pixelFormat);\n> +\n> +\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> +\t\tstatus = validateRaw(availableStreams, maxResolution);\n> +\telse\n> +\t\tstatus = validateYuv(availableStreams, maxResolution);\n\nThis may override the Adjusted status set when capping the number of\nstreams.\n\n> +\n> +\tif (status == Invalid)\n> +\t\treturn status;\n> +\n> +\t/*\n> +\t * Sensor format selection policy: the first stream selects the media\n> +\t * bus code to use, the largest stream selects the size.\n> +\t *\n> +\t * \\todo The sensor format selection policy could be changed to\n> +\t * prefer operating the sensor at full resolution to prioritize\n> +\t * image quality and FOV in exchange of a usually slower frame rate.\n\nBinning won't affect the field of view, I wouldn't mention FOV here.\n\n> +\t * Usage of the STILL_CAPTURE role could be consider for this.\n> +\t */\n> +\tconst PipeFormat &pipeFmt = formatsMap_.at(config_[0].pixelFormat);\n> +\n> +\tSize maxSize;\n> +\tfor (const auto &cfg : config_) {\n> +\t\tif (cfg.size > maxSize)\n> +\t\t\tmaxSize = cfg.size;\n> +\t}\n> +\n> +\tV4L2SubdeviceFormat sensorFormat{};\n> +\tsensorFormat.mbus_code = pipeFmt.sensorCode;\n> +\tsensorFormat.size = maxSize;\n> +\n> +\tLOG(ISI, Debug) << \"Computed sensor configuration: \" << sensorFormat;\n> +\n> +\t/*\n> +\t * We can't use CameraSensor::getFormat() as it might return a\n> +\t * format larger than our strict width limit, as that function\n> +\t * prioritizes formats with the same FOV ratio over formats with less\n\ns/FOV ratio/aspect ratio/\n\n> +\t * difference in size.\n> +\t *\n> +\t * Manually walk all the sensor supported sizes searching for\n> +\t * the smallest larger format without considering the FOV ratio\n\nDitto.\n\n> +\t * as the ISI can freely scale.\n> +\t */\n> +\tauto sizes = sensor->sizes(sensorFormat.mbus_code);\n> +\tSize bestSize;\n> +\n> +\tfor (const Size &s : sizes) {\n> +\t\t/* Ignore smaller sizes. */\n> +\t\tif (s.width < sensorFormat.size.width ||\n> +\t\t s.height < sensorFormat.size.height)\n> +\t\t\tcontinue;\n> +\n> +\t\t/* Make sure the width stays in the limits. */\n> +\t\tif (s.width > maxResolution.width)\n> +\t\t\tcontinue;\n> +\n> +\t\tbestSize = s;\n> +\t\tbreak;\n> +\t}\n> +\n> +\t/*\n> +\t * This should happen only if the sensor can only produce formats that\n> +\t * exceed the maximum allowed input width.\n> +\t */\n> +\tif (bestSize.isNull()) {\n> +\t\tLOG(ISI, Error) << \"Unable to find a suitable sensor format\";\n> +\t\treturn Invalid;\n> +\t}\n> +\n> +\tsensorFormat_.mbus_code = sensorFormat.mbus_code;\n> +\tsensorFormat_.size = bestSize;\n> +\n> +\tLOG(ISI, Debug) << \"Selected sensor format: \" << sensorFormat_;\n> +\n> +\treturn status;\n> +}\n> +\n> +/* -----------------------------------------------------------------------------\n> + * Pipeline Handler\n> + */\n> +\n> +PipelineHandlerISI::PipelineHandlerISI(CameraManager *manager)\n> +\t: PipelineHandler(manager)\n> +{\n> +}\n> +\n> +std::unique_ptr\n> +PipelineHandlerISI::generateConfiguration(Camera *camera,\n> +\t\t\t\t\t const StreamRoles &roles)\n> +{\n> +\tISICameraData *data = cameraData(camera);\n> +\tstd::unique_ptr config =\n> +\t\tstd::make_unique(data);\n> +\n> +\tif (roles.empty())\n> +\t\treturn config;\n> +\n> +\tif (roles.size() > data->streams_.size()) {\n> +\t\tLOG(ISI, Error) << \"Only up to \" << data->streams_.size()\n> +\t\t\t\t<< \" streams are supported\";\n> +\t\treturn nullptr;\n> +\t}\n> +\n> +\tfor (const auto &role : roles) {\n> +\t\t/*\n> +\t\t * Prefer the following formats\n> +\t\t * - Still Capture: Full resolution YUV422\n> +\t\t * - Preview/VideoRecording: 1080p YUYV\n\ns/preview/viewfinder/\n\n> +\t\t * - RAW: sensor's native format and resolution\n> +\t\t */\n> +\t\tstd::map> streamFormats;\n\nThis can move down.\n\n> +\t\tPixelFormat pixelFormat;\n> +\t\tSize size;\n> +\n> +\t\tswitch (role) {\n> +\t\tcase StillCapture:\n> +\t\t\t/*\n> +\t\t\t * \\todo Make sure the sensor can produce non-RAW formats\n> +\t\t\t * compatible with the pipeline supported ones.\n\n\"with the ones supported by the pipeline\" or just \"with the pipeline\".\nSame below.\n\n> +\t\t\t */\n> +\t\t\tsize = data->sensor_->resolution();\n> +\t\t\tpixelFormat = formats::YUV422;\n> +\t\t\tbreak;\n> +\n> +\t\tcase Viewfinder:\n\nYou should add VideoRecording here.\n\n> +\t\t\t/*\n> +\t\t\t * \\todo Make sure the sensor can produce non-RAW formats\n> +\t\t\t * compatible with the pipeline supported ones.\n> +\t\t\t */\n> +\t\t\tsize = PipelineHandlerISI::kPreviewSize;\n> +\t\t\tpixelFormat = formats::YUYV;\n> +\t\t\tbreak;\n> +\n> +\t\tcase Raw: {\n> +\t\t\t/*\n> +\t\t\t * Make sure the sensor can generate a RAW format and\n> +\t\t\t * prefer the ones with a larger bitdepth.\n> +\t\t\t */\n> +\t\t\tconst ISICameraConfiguration::FormatMap::value_type *rawPipeFormat = nullptr;\n> +\t\t\tunsigned int maxDepth = 0;\n> +\n> +\t\t\tfor (unsigned int code : data->sensor_->mbusCodes()) {\n> +\t\t\t\tconst BayerFormat &bayerFormat = BayerFormat::fromMbusCode(code);\n> +\t\t\t\tif (!bayerFormat.isValid())\n> +\t\t\t\t\tcontinue;\n> +\n> +\t\t\t\t/* Make sure the format is supported by the pipeline handler. */\n> +\t\t\t\tauto it = std::find_if(ISICameraConfiguration::formatsMap_.begin(),\n> +\t\t\t\t\t\t ISICameraConfiguration::formatsMap_.end(),\n> +\t\t\t\t\t\t [code](auto &isiFormat) {\n> +\t\t\t\t\t\t\t auto &pipe = isiFormat.second;\n> +\t\t\t\t\t\t\t return pipe.sensorCode == code;\n> +\t\t\t\t\t\t });\n> +\t\t\t\tif (it == ISICameraConfiguration::formatsMap_.end())\n> +\t\t\t\t\tcontinue;\n> +\n> +\t\t\t\tif (bayerFormat.bitDepth > maxDepth) {\n> +\t\t\t\t\tmaxDepth = bayerFormat.bitDepth;\n> +\t\t\t\t\trawPipeFormat = &(*it);\n> +\t\t\t\t}\n> +\t\t\t}\n> +\n> +\t\t\tif (!rawPipeFormat) {\n> +\t\t\t\tLOG(ISI, Error)\n> +\t\t\t\t\t<< \"Cannot generate a configuration for RAW stream\";\n> +\t\t\t\treturn nullptr;\n> +\t\t\t}\n> +\n> +\t\t\tsize = data->sensor_->resolution();\n> +\t\t\tpixelFormat = rawPipeFormat->first;\n> +\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tdefault:\n> +\t\t\tLOG(ISI, Error) << \"Requested stream role not supported: \" << role;\n> +\t\t\treturn nullptr;\n> +\t\t}\n> +\n\n\t\t/* \\todo Add all supported formats */\n\n> +\t\tstreamFormats[pixelFormat] = { { kMinISISize, size } };\n> +\t\tStreamFormats formats(streamFormats);\n> +\n> +\t\tStreamConfiguration cfg(formats);\n> +\t\tcfg.pixelFormat = pixelFormat;\n> +\t\tcfg.size = size;\n> +\t\tcfg.bufferCount = 4;\n> +\t\tconfig->addConfiguration(cfg);\n> +\t}\n> +\n> +\tconfig->validate();\n> +\n> +\treturn config;\n> +}\n> +\n> +int PipelineHandlerISI::configure(Camera *camera, CameraConfiguration *c)\n> +{\n> +\tISICameraConfiguration *camConfig = static_cast(c);\n> +\tISICameraData *data = cameraData(camera);\n> +\n> +\t/* All links are immutable except the sensor -> csis link. */\n> +\tconst MediaPad *sensorSrc = data->sensor_->entity()->getPadByIndex(0);\n> +\tsensorSrc->links()[0]->setEnabled(true);\n> +\n> +\t/*\n> +\t * Reset the crossbar switch routing and enable one route for each\n> +\t * requested stream configuration.\n> +\t *\n> +\t * \\todo Handle concurrent usage of multiple cameras by adjusting the\n> +\t * routing table instead of resetting it.\n> +\t */\n> +\tV4L2Subdevice::Routing routing = {};\n> +\n> +\tfor (const auto &[idx, config] : utils::enumerate(*c)) {\n> +\t\tstruct v4l2_subdev_route route = {\n> +\t\t\t.sink_pad = data->xbarSink_,\n> +\t\t\t.sink_stream = 0,\n> +\t\t\t/*\n> +\t\t\t * \\todo Verify that the number of crossbar inputs is\n> +\t\t\t * equal to 3 in all other SoCs.\n> +\t\t\t */\n\nSpoiler: it's not :-) Should you replace the line below with\n\n\t\t\t.source_pad = static_cast(crossbar_->pads().size() / 2 + 1),\n\nalready ?\n\n> +\t\t\t.source_pad = static_cast(3 + idx),\n> +\t\t\t.source_stream = 0,\n> +\t\t\t.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,\n> +\t\t\t.reserved = {}\n> +\t\t};\n> +\n> +\t\trouting.push_back(route);\n> +\t}\n> +\n> +\tint ret = crossbar_->setRouting(&routing, V4L2Subdevice::ActiveFormat);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\t/* Apply format to the sensor and CSIS receiver. */\n> +\tV4L2SubdeviceFormat format = camConfig->sensorFormat_;\n> +\tret = data->sensor_->setFormat(&format);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = data->csis_->setFormat(0, &format);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tret = crossbar_->setFormat(data->xbarSink_, &format);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\t/* Now configure the ISI and video node instances, one per stream. */\n> +\tdata->enabledStreams_.clear();\n> +\tfor (const auto &config : *c) {\n> +\t\tPipe *pipe = pipeFromStream(camera, config.stream());\n> +\n> +\t\t/*\n> +\t\t * Set the format on the ISI sink pad: it must match what is\n> +\t\t * received by the CSIS.\n> +\t\t */\n> +\t\tret = pipe->isi->setFormat(0, &format);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\n> +\t\t/*\n> +\t\t * Configure the ISI sink compose rectangle to downscale the\n> +\t\t * image.\n> +\t\t *\n> +\t\t * \\todo Additional cropping could be applied on the ISI source\n> +\t\t * pad to further reduce the output image size.\n> +\t\t */\n> +\t\tRectangle isiScale = {};\n> +\t\tisiScale.x = 0;\n> +\t\tisiScale.y = 0;\n> +\t\tisiScale.width = config.size.width;\n> +\t\tisiScale.height = config.size.height;\n\n\t\tRectangle isiScale{ config.size };\n\n> +\n> +\t\tret = pipe->isi->setSelection(0, V4L2_SEL_TGT_COMPOSE, &isiScale);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\n> +\t\t/*\n> +\t\t * Set the format on ISI source pad: only the media bus code\n> +\t\t * is relevant as it configures format conversion, while the\n> +\t\t * size is taken from the sink's COMPOSE (or source's CROP,\n> +\t\t * if any) rectangles.\n> +\t\t */\n> +\t\tconst ISICameraConfiguration::PipeFormat &pipeFormat =\n> +\t\t\tISICameraConfiguration::formatsMap_.at(config.pixelFormat);\n> +\n> +\t\tV4L2SubdeviceFormat isiFormat{};\n> +\t\tisiFormat.mbus_code = pipeFormat.isiCode;\n> +\t\tisiFormat.size = config.size;\n> +\n> +\t\tret = pipe->isi->setFormat(1, &isiFormat);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\n> +\t\tV4L2DeviceFormat captureFmt{};\n> +\t\tcaptureFmt.fourcc = pipe->capture->toV4L2PixelFormat(config.pixelFormat);\n> +\t\tcaptureFmt.size = config.size;\n> +\n> +\t\t/* \\todo Set stride and format. */\n> +\t\tret = pipe->capture->setFormat(&captureFmt);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\n> +\t\t/* Store the list of enabled streams for later use. */\n> +\t\tdata->enabledStreams_.push_back(config.stream());\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +int PipelineHandlerISI::exportFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t\t\t std::vector> *buffers)\n> +{\n> +\tunsigned int count = stream->configuration().bufferCount;\n> +\tPipe *pipe = pipeFromStream(camera, stream);\n> +\n> +\treturn pipe->capture->exportBuffers(count, buffers);\n> +}\n> +\n> +int PipelineHandlerISI::start(Camera *camera,\n> +\t\t\t [[maybe_unused]] const ControlList *controls)\n> +{\n> +\tISICameraData *data = cameraData(camera);\n> +\n> +\tfor (const auto &stream : data->enabledStreams_) {\n> +\t\tPipe *pipe = pipeFromStream(camera, stream);\n> +\t\tconst StreamConfiguration &config = stream->configuration();\n> +\n> +\t\tint ret = pipe->capture->importBuffers(config.bufferCount);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\n> +\t\tret = pipe->capture->streamOn();\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +void PipelineHandlerISI::stopDevice(Camera *camera)\n> +{\n> +\tISICameraData *data = cameraData(camera);\n> +\n> +\tfor (const auto &stream : data->enabledStreams_) {\n> +\t\tPipe *pipe = pipeFromStream(camera, stream);\n> +\n> +\t\tpipe->capture->streamOff();\n> +\t\tpipe->capture->releaseBuffers();\n> +\t}\n> +}\n> +\n> +int PipelineHandlerISI::queueRequestDevice(Camera *camera, Request *request)\n> +{\n> +\tfor (auto &[stream, buffer] : request->buffers()) {\n> +\t\tPipe *pipe = pipeFromStream(camera, stream);\n> +\n> +\t\tint ret = pipe->capture->queueBuffer(buffer);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)\n> +{\n> +\tDeviceMatch dm(\"mxc-isi\");\n> +\tdm.add(\"crossbar\");\n> +\tdm.add(\"mxc_isi.0\");\n> +\tdm.add(\"mxc_isi.0.capture\");\n> +\n> +\tisiDev_ = acquireMediaDevice(enumerator, dm);\n> +\tif (!isiDev_)\n> +\t\treturn false;\n> +\n> +\t/*\n> +\t * Acquire the subdevs and video nodes for the crossbar switch and the\n> +\t * processing pipelines.\n> +\t */\n> +\tcrossbar_ = V4L2Subdevice::fromEntityName(isiDev_, \"crossbar\");\n> +\tif (!crossbar_)\n> +\t\treturn false;\n> +\n> +\tint ret = crossbar_->open();\n> +\tif (ret)\n> +\t\treturn false;\n> +\n> +\tfor (unsigned int i = 0; ; ++i) {\n> +\t\tstd::string entityName = \"mxc_isi.\" + std::to_string(i);\n> +\t\tstd::unique_ptr isi =\n> +\t\t\tV4L2Subdevice::fromEntityName(isiDev_, entityName);\n> +\t\tif (!isi)\n> +\t\t\tbreak;\n> +\n> +\t\tret = isi->open();\n> +\t\tif (ret)\n> +\t\t\treturn false;\n> +\n> +\t\tentityName += \".capture\";\n> +\t\tstd::unique_ptr capture =\n> +\t\t\tV4L2VideoDevice::fromEntityName(isiDev_, entityName);\n> +\t\tASSERT(capture);\n\nWhen I said \"make it a fatal error\" in the review of v1, I meant \"return\nfalse\". Sorry for being unclear. I suppose this works too.\n\n> +\n> +\t\tcapture->bufferReady.connect(this, &PipelineHandlerISI::bufferReady);\n> +\n> +\t\tret = capture->open();\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\n> +\t\tpipes_.push_back({ std::move(isi), std::move(capture) });\n> +\t}\n> +\n> +\tif (pipes_.empty()) {\n> +\t\tLOG(ISI, Error) << \"Unable to enumerate pipes\";\n> +\t\treturn false;\n> +\t}\n> +\n> +\t/*\n> +\t * Loop over all the crossbar switch sink pads to find connected CSI-2\n> +\t * receivers and camera sensors.\n> +\t */\n> +\tunsigned int numCameras = 0;\n> +\tunsigned int numSinks = 0;\n> +\tfor (MediaPad *pad : crossbar_->entity()->pads()) {\n> +\t\tunsigned int sink = numSinks;\n> +\n> +\t\tif (!(pad->flags() & MEDIA_PAD_FL_SINK) || pad->links().empty())\n> +\t\t\tcontinue;\n> +\n> +\t\t/*\n> +\t\t * Count each crossbar sink pad to correctly configure\n> +\t\t * routing and format for this camera.\n> +\t\t */\n> +\t\tnumSinks++;\n> +\n> +\t\tMediaEntity *csi = pad->links()[0]->source()->entity();\n> +\t\tif (csi->pads().size() != 2) {\n> +\t\t\tLOG(ISI, Debug) << \"Skip unsupported CSI-2 receiver \"\n> +\t\t\t\t\t<< csi->name();\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tpad = csi->pads()[0];\n> +\t\tif (!(pad->flags() & MEDIA_PAD_FL_SINK) || pad->links().empty())\n> +\t\t\tcontinue;\n> +\n> +\t\tMediaEntity *sensor = pad->links()[0]->source()->entity();\n> +\t\tif (sensor->function() != MEDIA_ENT_F_CAM_SENSOR) {\n> +\t\t\tLOG(ISI, Debug) << \"Skip unsupported subdevice \"\n> +\t\t\t\t\t<< sensor->name();\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\t/* Create the camera data. */\n> +\t\tstd::unique_ptr data =\n> +\t\t\tstd::make_unique(this);\n> +\n> +\t\tdata->sensor_ = std::make_unique(sensor);\n> +\t\tdata->csis_ = std::make_unique(csi);\n> +\t\tdata->xbarSink_ = sink;\n> +\n> +\t\tret = data->init();\n> +\t\tif (ret) {\n> +\t\t\tLOG(ISI, Error) << \"Failed to initialize camera data\";\n> +\t\t\treturn false;\n> +\t\t}\n> +\n> +\t\t/* Register the camera. */\n> +\t\tconst std::string &id = data->sensor_->id();\n> +\t\tstd::set streams;\n> +\t\tstd::transform(data->streams_.begin(), data->streams_.end(),\n> +\t\t\t std::inserter(streams, streams.end()),\n> +\t\t\t [](Stream &s) { return &s; });\n> +\n> +\t\tstd::shared_ptr camera =\n> +\t\t\tCamera::create(std::move(data), id, streams);\n> +\n> +\t\tregisterCamera(std::move(camera));\n> +\t\tnumCameras++;\n> +\t}\n> +\n> +\treturn numCameras > 0;\n> +}\n> +\n> +PipelineHandlerISI::Pipe *PipelineHandlerISI::pipeFromStream(Camera *camera,\n> +\t\t\t\t\t\t\t const Stream *stream)\n> +{\n> +\tISICameraData *data = cameraData(camera);\n> +\tunsigned int pipeIndex = data->pipeIndex(stream);\n> +\n> +\tASSERT(pipeIndex < pipes_.size());\n> +\n> +\treturn &pipes_[pipeIndex];\n> +}\n> +\n> +void PipelineHandlerISI::bufferReady(FrameBuffer *buffer)\n> +{\n> +\tRequest *request = buffer->request();\n> +\n> +\tcompleteBuffer(request, buffer);\n> +\tif (request->hasPendingBuffers())\n> +\t\treturn;\n> +\n> +\tcompleteRequest(request);\n> +}\n> +\n> +REGISTER_PIPELINE_HANDLER(PipelineHandlerISI)\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/imx8-isi/meson.build b/src/libcamera/pipeline/imx8-isi/meson.build\n> new file mode 100644\n> index 000000000000..ffd0ce54ce92\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/imx8-isi/meson.build\n> @@ -0,0 +1,5 @@\n> +# SPDX-License-Identifier: CC0-1.0\n> +\n> +libcamera_sources += files([\n> + 'imx8-isi.cpp'\n> +])","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 7C82ABE08B\n\tfor ;\n\tMon, 14 Nov 2022 19:07:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DBA166307F;\n\tMon, 14 Nov 2022 20:07:13 +0100 (CET)","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 431A261F3B\n\tfor ;\n\tMon, 14 Nov 2022 20:07:12 +0100 (CET)","from pendragon.ideasonboard.com (unknown [46.183.103.8])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1516BFB;\n\tMon, 14 Nov 2022 20:07:11 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1668452833;\n\tbh=m1Jw6IaekGZCywhrdpijVgtxjkAQ2sSCpEkRaw5+uBw=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=QK1jUQJ4HNL4nI/tdqW9aDvxvtVA176cNHW8aTSjVALLLETmAbowUXERQDLMWXPYL\n\t7GjOr83Mi93Gn5IlZrTM0C9xpwWioW9cE4apV1AV59LLlJXl8s/C5OUNVDehG/98oW\n\tWktE+KCmfnfCHafeqfEUMjTuDLIc4tbFPSuyVvKkHGkMon7/KK+j43b48DBORikMY8\n\tNpSEG0tFh4lF9OcvFCCBLwKYW2zL4DOu/K8BhK6klgtNZYeo+Y1qSRRKF7qa8Pj/u1\n\tiF52jytkS2s9UcqrhOVBeOglib7ePNOOAkHfLKyOTloc3EC9YoQxBc9ga3PpPXvKGs\n\tkU30BqI7vVxZg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1668452831;\n\tbh=m1Jw6IaekGZCywhrdpijVgtxjkAQ2sSCpEkRaw5+uBw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=q2IzDZv7K//yoF/yRSU5/vEBP/DIjNXFiCutDqOQxO/GO8abkekuQamG3TH7zl3lF\n\tZhIQbiefjtA5et4GxN1tlwcCihz8R6OlmdRkMvNH0gxdVD5aTjvllruPSzuQX6Lw4F\n\tStzqGYSwjYVqofU/cbhELQEKVR66MgUOmlvATxKs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"q2IzDZv7\"; dkim-atps=neutral","Date":"Mon, 14 Nov 2022 21:06:51 +0200","To":"Jacopo Mondi ","Message-ID":"","References":"<20221110191433.5535-1-jacopo@jmondi.org>\n\t<20221110191433.5535-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221110191433.5535-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: pipeline: Add IMX8 ISI\n\tpipeline","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25794,"web_url":"https://patchwork.libcamera.org/comment/25794/","msgid":"<20221114203533.wmxud5vnlqiusjpf@uno.localdomain>","date":"2022-11-14T20:35:33","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: pipeline: Add IMX8 ISI\n\tpipeline","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Mon, Nov 14, 2022 at 09:06:51PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Thu, Nov 10, 2022 at 08:14:33PM +0100, Jacopo Mondi via libcamera-devel wrote:\n> > Add a pipeline handler for the ISI capture interface found on\n> > several versions of the i.MX8 SoC generation.\n> >\n> > The pipeline handler supports capturing multiple streams from the same\n> > camera in YUV, RGB or RAW formats. The number of streams is limited by\n> > the number of ISI pipelines, and is currently hardcoded to 2 as the code\n> > has been tested on the i.MX8MP only. Further development will make this\n> > dynamic to support other SoCs.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > Reviewed-by: Kieran Bingham \n> > ---\n> > meson_options.txt | 2 +-\n> > src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 957 +++++++++++++++++++\n> > src/libcamera/pipeline/imx8-isi/meson.build | 5 +\n> > 3 files changed, 963 insertions(+), 1 deletion(-)\n> > create mode 100644 src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> > create mode 100644 src/libcamera/pipeline/imx8-isi/meson.build\n> >\n> > diff --git a/meson_options.txt b/meson_options.txt\n> > index f1d678089452..1ba6778ce257 100644\n> > --- a/meson_options.txt\n> > +++ b/meson_options.txt\n> > @@ -37,7 +37,7 @@ option('lc-compliance',\n> >\n> > option('pipelines',\n> > type : 'array',\n> > - choices : ['ipu3', 'raspberrypi', 'rkisp1', 'simple', 'uvcvideo', 'vimc'],\n> > + choices : ['imx8-isi', 'ipu3', 'raspberrypi', 'rkisp1', 'simple', 'uvcvideo', 'vimc'],\n> > description : 'Select which pipeline handlers to include')\n> >\n> > option('qcam',\n> > diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> > new file mode 100644\n> > index 000000000000..6e4b1ae290ef\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> > @@ -0,0 +1,957 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2022 - Jacopo Mondi \n> > + *\n> > + * imx8-isi.cpp - Pipeline handler for ISI interface found on NXP i.MX8 SoC\n> > + */\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"libcamera/internal/bayer_format.h\"\n> > +#include \"libcamera/internal/camera.h\"\n> > +#include \"libcamera/internal/camera_sensor.h\"\n> > +#include \"libcamera/internal/device_enumerator.h\"\n> > +#include \"libcamera/internal/media_device.h\"\n> > +#include \"libcamera/internal/pipeline_handler.h\"\n> > +#include \"libcamera/internal/v4l2_subdevice.h\"\n> > +#include \"libcamera/internal/v4l2_videodevice.h\"\n> > +\n> > +#include \"linux/media-bus-format.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(ISI)\n> > +\n> > +class PipelineHandlerISI;\n> > +\n> > +class ISICameraData : public Camera::Private\n> > +{\n> > +public:\n> > +\tISICameraData(PipelineHandler *ph)\n> > +\t\t: Camera::Private(ph)\n> > +\t{\n> > +\t\t/*\n> > +\t\t * \\todo Assume 2 channels only for now, as that's the number of\n> > +\t\t * available channels on i.MX8MP.\n> > +\t\t */\n> > +\t\tstreams_.resize(2);\n> > +\t}\n> > +\n> > +\tPipelineHandlerISI *pipe();\n> > +\n> > +\tint init();\n> > +\n> > +\tunsigned int pipeIndex(const Stream *stream)\n> > +\t{\n> > +\t\treturn stream - &*streams_.begin();\n> > +\t}\n> > +\n> > +\tstd::unique_ptr sensor_;\n> > +\tstd::unique_ptr csis_;\n> > +\n> > +\tstd::vector streams_;\n> > +\n> > +\tstd::vector enabledStreams_;\n> > +\n> > +\tunsigned int xbarSink_;\n> > +};\n> > +\n> > +class ISICameraConfiguration : public CameraConfiguration\n> > +{\n> > +public:\n> > +\t/*\n> > +\t * formatsMap_ records the association between an output pixel format\n> > +\t * and the combination of V4L2 pixel format and media bus codes that have\n> > +\t * to be applied to the pipeline.\n> > +\t */\n> > +\tstruct PipeFormat {\n> > +\t\tunsigned int isiCode;\n> > +\t\tunsigned int sensorCode;\n> > +\t};\n> > +\n> > +\tusing FormatMap = std::map;\n> > +\n> > +\tISICameraConfiguration(ISICameraData *data)\n> > +\t\t: data_(data)\n> > +\t{\n> > +\t}\n> > +\n> > +\tStatus validate() override;\n> > +\n> > +\tstatic const FormatMap formatsMap_;\n> > +\n> > +\tV4L2SubdeviceFormat sensorFormat_;\n> > +\n> > +private:\n> > +\tconst ISICameraData *data_;\n> > +\n> > +\tCameraConfiguration::Status validateRaw(std::set &availableStreams,\n> > +\t\t\t\t\t\tconst Size &maxResolution);\n> > +\tCameraConfiguration::Status validateYuv(std::set &availableStreams,\n> > +\t\t\t\t\t\tconst Size &maxResolution);\n>\n> Functions should go before data.\n>\n\nAck\n\n> > +};\n> > +\n> > +class PipelineHandlerISI : public PipelineHandler\n> > +{\n> > +public:\n> > +\tPipelineHandlerISI(CameraManager *manager);\n> > +\n> > +\tbool match(DeviceEnumerator *enumerator) override;\n> > +\n> > +\tstd::unique_ptr generateConfiguration(Camera *camera,\n> > +\t\t\t\t\t\t\t\t const StreamRoles &roles) override;\n>\n> You can also fold it as\n>\n> \tstd::unique_ptr\n> \tgenerateConfiguration(Camera *camera, const StreamRoles &roles) override;\n>\n> to reduce the line length.\n\nok..\n\n>\n> > +\tint configure(Camera *camera, CameraConfiguration *config) override;\n> > +\n> > +\tint exportFrameBuffers(Camera *camera, Stream *stream,\n> > +\t\t\t std::vector> *buffers) override;\n> > +\n> > +\tint start(Camera *camera, const ControlList *controls) override;\n> > +\n> > +protected:\n> > +\tvoid stopDevice(Camera *camera) override;\n> > +\n> > +\tint queueRequestDevice(Camera *camera, Request *request) override;\n> > +\n> > +private:\n> > +\tstatic constexpr Size kPreviewSize = { 1920, 1080 };\n> > +\tstatic constexpr Size kMinISISize = { 1, 1 };\n> > +\n> > +\tstruct Pipe {\n> > +\t\tstd::unique_ptr isi;\n> > +\t\tstd::unique_ptr capture;\n> > +\t};\n> > +\n> > +\tISICameraData *cameraData(Camera *camera)\n> > +\t{\n> > +\t\treturn static_cast(camera->_d());\n> > +\t}\n> > +\n> > +\tPipe *pipeFromStream(Camera *camera, const Stream *stream);\n> > +\n> > +\tvoid bufferReady(FrameBuffer *buffer);\n> > +\n> > +\tMediaDevice *isiDev_;\n> > +\n> > +\tstd::unique_ptr crossbar_;\n> > +\tstd::vector pipes_;\n> > +};\n> > +\n> > +/* -----------------------------------------------------------------------------\n> > + * Camera Data\n> > + */\n> > +\n> > +PipelineHandlerISI *ISICameraData::pipe()\n> > +{\n> > +\treturn static_cast(Camera::Private::pipe());\n> > +}\n> > +\n> > +/* Open and initialize pipe components. */\n> > +int ISICameraData::init()\n> > +{\n> > +\tint ret = sensor_->init();\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tret = csis_->open();\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tproperties_ = sensor_->properties();\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +/* -----------------------------------------------------------------------------\n> > + * Camera Configuration\n> > + */\n> > +\n> > +const ISICameraConfiguration::FormatMap ISICameraConfiguration::formatsMap_ = {\n>\n> Could you add a todo comment to remind that the sensor format selection\n> should not be hardcoded ?\n>\n\nYes, it's reported in the code but I'll add here\n\n> > +\t{\n> > +\t\tformats::YUV422,\n> > +\t\t{ MEDIA_BUS_FMT_YUV8_1X24,\n> > +\t\t MEDIA_BUS_FMT_UYVY8_1X16 },\n> > +\t},\n> > +\t{\n> > +\t\tformats::YUYV,\n> > +\t\t{ MEDIA_BUS_FMT_YUV8_1X24,\n> > +\t\t MEDIA_BUS_FMT_UYVY8_1X16 },\n> > +\t},\n> > +\t{\n> > +\t\tformats::RGB565,\n> > +\t\t{ MEDIA_BUS_FMT_RGB888_1X24,\n> > +\t\t MEDIA_BUS_FMT_RGB565_1X16 },\n> > +\t},\n> > +\t{\n> > +\t\tformats::SBGGR8,\n> > +\t\t{ MEDIA_BUS_FMT_SBGGR8_1X8,\n> > +\t\t MEDIA_BUS_FMT_SBGGR8_1X8 },\n> > +\t},\n> > +\t{\n> > +\t\tformats::SGBRG8,\n> > +\t\t{ MEDIA_BUS_FMT_SGBRG8_1X8,\n> > +\t\t MEDIA_BUS_FMT_SGBRG8_1X8 },\n> > +\t},\n> > +\t{\n> > +\t\tformats::SGRBG8,\n> > +\t\t{ MEDIA_BUS_FMT_SGRBG8_1X8,\n> > +\t\t MEDIA_BUS_FMT_SGRBG8_1X8 },\n> > +\t},\n> > +\t{\n> > +\t\tformats::SRGGB8,\n> > +\t\t{ MEDIA_BUS_FMT_SRGGB8_1X8,\n> > +\t\t MEDIA_BUS_FMT_SRGGB8_1X8 },\n> > +\t},\n> > +\t{\n> > +\t\tformats::SBGGR10,\n> > +\t\t{ MEDIA_BUS_FMT_SBGGR10_1X10,\n> > +\t\t MEDIA_BUS_FMT_SBGGR10_1X10 },\n> > +\t},\n> > +\t{\n> > +\t\tformats::SGBRG10,\n> > +\t\t{ MEDIA_BUS_FMT_SGBRG10_1X10,\n> > +\t\t MEDIA_BUS_FMT_SGBRG10_1X10 },\n> > +\t},\n> > +\t{\n> > +\t\tformats::SGRBG10,\n> > +\t\t{ MEDIA_BUS_FMT_SGRBG10_1X10,\n> > +\t\t MEDIA_BUS_FMT_SGRBG10_1X10 },\n> > +\t},\n> > +\t{\n> > +\t\tformats::SRGGB10,\n> > +\t\t{ MEDIA_BUS_FMT_SRGGB10_1X10,\n> > +\t\t MEDIA_BUS_FMT_SRGGB10_1X10 },\n> > +\t},\n> > +\t{\n> > +\t\tformats::SBGGR12,\n> > +\t\t{ MEDIA_BUS_FMT_SBGGR12_1X12,\n> > +\t\t MEDIA_BUS_FMT_SBGGR12_1X12 },\n> > +\t},\n> > +\t{\n> > +\t\tformats::SGBRG12,\n> > +\t\t{ MEDIA_BUS_FMT_SGBRG12_1X12,\n> > +\t\t MEDIA_BUS_FMT_SGBRG12_1X12 },\n> > +\t},\n> > +\t{\n> > +\t\tformats::SGRBG12,\n> > +\t\t{ MEDIA_BUS_FMT_SGRBG12_1X12,\n> > +\t\t MEDIA_BUS_FMT_SGRBG12_1X12 },\n> > +\t},\n> > +\t{\n> > +\t\tformats::SRGGB12,\n> > +\t\t{ MEDIA_BUS_FMT_SRGGB12_1X12,\n> > +\t\t MEDIA_BUS_FMT_SRGGB12_1X12 },\n> > +\t},\n> > +};\n> > +\n> > +/*\n> > + * Adjust stream configuration when the first requested stream is RAW: all the\n> > + * streams will have the same RAW pixelformat and size.\n> > + */\n> > +CameraConfiguration::Status ISICameraConfiguration::validateRaw(std::set &availableStreams,\n> > +\t\t\t\t\t\t\t\tconst Size &maxResolution)\n>\n> CameraConfiguration::Status\n> ISICameraConfiguration::validateRaw(std::set &availableStreams,\n> \t\t\t\t const Size &maxResolution)\n>\n> > +{\n> > +\tCameraConfiguration::Status status = Valid;\n> > +\n> > +\t/*\n> > +\t * Make sure the requested RAW format is supported by the\n> > +\t * pipeline, otherwise adjust it.\n> > +\t */\n> > +\tstd::vector mbusCodes = data_->sensor_->mbusCodes();\n> > +\tStreamConfiguration &rawConfig = config_[0];\n> > +\n> > +\tbool supported = false;\n> > +\tauto it = formatsMap_.find(rawConfig.pixelFormat);\n> > +\tif (it != formatsMap_.end()) {\n> > +\t\tunsigned int mbusCode = it->second.sensorCode;\n> > +\n> > +\t\tif (std::count(mbusCodes.begin(), mbusCodes.end(), mbusCode))\n> > +\t\t\tsupported = true;\n> > +\t}\n> > +\n> > +\tif (!supported) {\n> > +\t\t/*\n> > +\t\t * Adjust to the first mbus code supported by both the\n> > +\t\t * sensor and the pipeline.\n> > +\t\t */\n> > +\t\tconst FormatMap::value_type *pipeConfig = nullptr;\n> > +\t\tfor (unsigned int code : data_->sensor_->mbusCodes()) {\n>\n> You can reuse the mbusCodes variables.\n>\n\nouch, that was actualy my idea initially\n\n> > +\t\t\tconst BayerFormat &bayerFormat = BayerFormat::fromMbusCode(code);\n> > +\t\t\tif (!bayerFormat.isValid())\n> > +\t\t\t\tcontinue;\n> > +\n> > +\t\t\tauto fmt = std::find_if(ISICameraConfiguration::formatsMap_.begin(),\n> > +\t\t\t\t\t\tISICameraConfiguration::formatsMap_.end(),\n> > +\t\t\t\t\t\t[code](auto &isiFormat) {\n>\n> const auto\n>\n> > +\t\t\t\t\t\t\tauto &pipe = isiFormat.second;\n>\n> Same here.\n>\n> > +\t\t\t\t\t\t\treturn pipe.sensorCode == code;\n> > +\t\t\t\t\t\t});\n> > +\n> > +\t\t\tif (fmt == ISICameraConfiguration::formatsMap_.end())\n> > +\t\t\t\tcontinue;\n> > +\n> > +\t\t\tpipeConfig = &(*fmt);\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\n> > +\t\tif (!pipeConfig) {\n> > +\t\t\tLOG(ISI, Error) << \"Cannot adjust RAW format \"\n> > +\t\t\t\t\t<< rawConfig.pixelFormat;\n> > +\t\t\treturn Invalid;\n> > +\t\t}\n> > +\n> > +\t\trawConfig.pixelFormat = pipeConfig->first;\n> > +\t\tLOG(ISI, Debug) << \"RAW pixelformat adjusted to \"\n> > +\t\t\t\t<< pipeConfig->first;\n> > +\t\tstatus = Adjusted;\n> > +\t}\n> > +\n> > +\t/* Cap the RAW stream size to the maximum resolution. */\n> > +\tSize configSize = rawConfig.size;\n>\n> const\n>\n> > +\trawConfig.size.boundTo(maxResolution);\n>\n> I don't think this is enough, not all sizes smaller than the sensor\n> resolution can be achieved as the pipeline can't scale raw formats. It\n> can be addressed on top.\n>\n> > +\tif (rawConfig.size != configSize) {\n> > +\t\tLOG(ISI, Debug) << \"RAW size adjusted to \"\n> > +\t\t\t\t<< rawConfig.size;\n> > +\t\tstatus = Adjusted;\n> > +\t}\n> > +\n> > +\t/* Adjust all other streams to RAW. */\n> > +\tunsigned int i = 0;\n> > +\tfor (StreamConfiguration &cfg : config_) {\n>\n> \tfor (auto &[i, cfg] : utils::enumerate(config_)) {\n>\n\nNope, I started with that too\n\n../src/libcamera/pipeline/imx8-isi/imx8-isi.cpp: In member function ‘libcamera::CameraConfiguration::Status libcamera::ISICameraConfiguration::validateRaw(std::set&, const libcamera::Size&)’:\n../src/libcamera/pipeline/imx8-isi/imx8-isi.cpp:331:55: error: cannot bind non-const lvalue reference of type ‘std::pair&’ to an rvalue of type ‘libcamera::utils::details::enumerate_iterator<__gnu_cxx::__normal_iterator > >::value_type’ {aka ‘std::pair’}\n 331 | for (auto &[i, cfg] : utils::enumerate(config_)) {\n\n Cannot bind non-const lvalue reference of type ‘std::pair&’\n to an rvalue of type std::pair\n\nI can fix it by removing the reference, but then the\nStreamCOnfiguration would be copied.\n\nI've not investigated why\n\n> > +\n> > +\t\tLOG(ISI, Debug) << \"Stream \" << i << \": \" << cfg.toString();\n> > +\t\tPixelFormat pixFmt = cfg.pixelFormat;\n> > +\t\tSize size = cfg.size;\n>\n> const for both.\n>\n> > +\n> > +\t\tcfg.pixelFormat = rawConfig.pixelFormat;\n> > +\t\tcfg.size = rawConfig.size;\n> > +\n> > +\t\tif (cfg.pixelFormat != pixFmt ||\n> > +\t\t cfg.size != size) {\n>\n> This holds on a single line.\n>\n> > +\t\t\tLOG(ISI, Debug) << \"Stream \" << i << \" adjusted to \"\n> > +\t\t\t\t\t<< cfg.toString();\n> > +\t\t\tstatus = Adjusted;\n> > +\t\t}\n> > +\n> > +\t\tPixelFormatInfo info = PixelFormatInfo::info(cfg.pixelFormat);\n>\n> const reference. It would be nice to disable copy and move for the\n> PixelFormatInfo class.\n>\n\nack\n\n> > +\t\tcfg.stride = info.stride(cfg.size.width, 0);\n> > +\t\tcfg.frameSize = info.frameSize(cfg.size, info.bitsPerPixel);\n> > +\n> > +\t\t/* Assign streams in the order they are presented. */\n> > +\t\tauto stream = availableStreams.extract(availableStreams.begin());\n> > +\t\tcfg.setStream(stream.value());\n> > +\n> > +\t\ti++;\n> > +\t}\n> > +\n> > +\treturn status;\n> > +}\n> > +\n> > +/*\n> > + * Adjust stream configuration when the first requested stream is not RAW: all\n> > + * the streams will be either YUV or RGB processed formats.\n> > + */\n> > +CameraConfiguration::Status ISICameraConfiguration::validateYuv(std::set &availableStreams,\n> > +\t\t\t\t\t\t\t\tconst Size &maxResolution)\n>\n> CameraConfiguration::Status\n> ISICameraConfiguration::validateYuv(std::set &availableStreams,\n> \t\t\t\t const Size &maxResolution)\n>\n> > +{\n> > +\tCameraConfiguration::Status status = Valid;\n> > +\n> > +\tunsigned int i = 0;\n> > +\tfor (StreamConfiguration &cfg : config_) {\n>\n> util::enumerate() here too.\n>\n\nsame\n\n> > +\n> > +\t\tLOG(ISI, Debug) << \"Stream \" << i << \": \" << cfg.toString();\n> > +\n> > +\t\t/* If the stream is RAW or not supported default it to YUYV. */\n> > +\t\tPixelFormatInfo info = PixelFormatInfo::info(cfg.pixelFormat);\n>\n> const reference\n>\n> > +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW ||\n> > +\t\t !formatsMap_.count(cfg.pixelFormat)) {\n> > +\n> > +\t\t\tLOG(ISI, Debug) << \"Stream \" << i << \" format: \"\n> > +\t\t\t\t\t<< cfg.pixelFormat << \" adjusted to YUYV\";\n> > +\n> > +\t\t\tcfg.pixelFormat = formats::YUYV;\n> > +\t\t\tstatus = Adjusted;\n> > +\t\t}\n> > +\n> > +\t\t/* Cap the streams size to the maximum accepted resolution. */\n> > +\t\tSize configSize = cfg.size;\n> > +\t\tcfg.size.boundTo(maxResolution);\n> > +\t\tif (cfg.size != configSize) {\n> > +\t\t\tLOG(ISI, Debug)\n> > +\t\t\t\t<< \"Stream \" << i << \" adjusted to \" << cfg.size;\n> > +\t\t\tstatus = Adjusted;\n> > +\t\t}\n> > +\n> > +\t\t/* Re-fetch the pixel format info in case it has been adjusted. */\n> > +\t\tinfo = PixelFormatInfo::info(cfg.pixelFormat);\n> > +\n> > +\t\t/* \\todo Multiplane ? */\n> > +\t\tcfg.stride = info.stride(cfg.size.width, 0);\n> > +\t\tcfg.frameSize = info.frameSize(cfg.size, info.bitsPerPixel);\n> > +\n> > +\t\t/* Assign streams in the order they are presented. */\n> > +\t\tauto stream = availableStreams.extract(availableStreams.begin());\n> > +\t\tcfg.setStream(stream.value());\n> > +\n> > +\t\ti++;\n> > +\t}\n> > +\n> > +\treturn status;\n> > +}\n> > +\n> > +CameraConfiguration::Status ISICameraConfiguration::validate()\n> > +{\n> > +\tStatus status = Valid;\n> > +\n> > +\tstd::set availableStreams;\n> > +\tstd::transform(data_->streams_.begin(), data_->streams_.end(),\n> > +\t\t std::inserter(availableStreams, availableStreams.end()),\n> > +\t\t [](const Stream &s) { return const_cast(&s); });\n> > +\n> > +\tif (config_.empty())\n> > +\t\treturn Invalid;\n> > +\n> > +\t/* Cap the number of streams to the number of available ISI pipes. */\n> > +\tif (config_.size() > availableStreams.size()) {\n> > +\t\tconfig_.resize(availableStreams.size());\n> > +\t\tstatus = Adjusted;\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * If more than a single stream is requested, the maximum allowed input\n> > +\t * image width is 2048. Cap the maximum image size accordingly.\n> > +\t *\n> > +\t * \\todo The (size > 1) check only applies to i.MX8MP which has 2 ISI\n> > +\t * channels. SoCs with more channels than the i.MX8MP are capable of\n> > +\t * supporting more streams with input width > 2048 by chaining\n> > +\t * successive channels together. Define a policy for channels allocation\n> > +\t * to fully support other SoCs.\n> > +\t */\n> > +\tCameraSensor *sensor = data_->sensor_.get();\n> > +\tSize maxResolution = sensor->resolution();\n> > +\tif (config_.size() > 1)\n> > +\t\tmaxResolution.width = std::min(2048U, maxResolution.width);\n> > +\n> > +\t/* Validate streams according to the format of the first one. */\n> > +\tconst PixelFormatInfo info = PixelFormatInfo::info(config_[0].pixelFormat);\n> > +\n> > +\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> > +\t\tstatus = validateRaw(availableStreams, maxResolution);\n> > +\telse\n> > +\t\tstatus = validateYuv(availableStreams, maxResolution);\n>\n> This may override the Adjusted status set when capping the number of\n> streams.\n>\n\nRight. I think I will have to expand it then or use an additional\nvariable\n\n> > +\n> > +\tif (status == Invalid)\n> > +\t\treturn status;\n> > +\n> > +\t/*\n> > +\t * Sensor format selection policy: the first stream selects the media\n> > +\t * bus code to use, the largest stream selects the size.\n> > +\t *\n> > +\t * \\todo The sensor format selection policy could be changed to\n> > +\t * prefer operating the sensor at full resolution to prioritize\n> > +\t * image quality and FOV in exchange of a usually slower frame rate.\n>\n> Binning won't affect the field of view, I wouldn't mention FOV here.\n>\n\nWho has mentioned binning ? smaller modes can be obtained by cropping\ntoo\n\nI can drop it if controversial\n\n> > +\t * Usage of the STILL_CAPTURE role could be consider for this.\n> > +\t */\n> > +\tconst PipeFormat &pipeFmt = formatsMap_.at(config_[0].pixelFormat);\n> > +\n> > +\tSize maxSize;\n> > +\tfor (const auto &cfg : config_) {\n> > +\t\tif (cfg.size > maxSize)\n> > +\t\t\tmaxSize = cfg.size;\n> > +\t}\n> > +\n> > +\tV4L2SubdeviceFormat sensorFormat{};\n> > +\tsensorFormat.mbus_code = pipeFmt.sensorCode;\n> > +\tsensorFormat.size = maxSize;\n> > +\n> > +\tLOG(ISI, Debug) << \"Computed sensor configuration: \" << sensorFormat;\n> > +\n> > +\t/*\n> > +\t * We can't use CameraSensor::getFormat() as it might return a\n> > +\t * format larger than our strict width limit, as that function\n> > +\t * prioritizes formats with the same FOV ratio over formats with less\n>\n> s/FOV ratio/aspect ratio/\n>\n> > +\t * difference in size.\n> > +\t *\n> > +\t * Manually walk all the sensor supported sizes searching for\n> > +\t * the smallest larger format without considering the FOV ratio\n>\n> Ditto.\n>\n> > +\t * as the ISI can freely scale.\n> > +\t */\n> > +\tauto sizes = sensor->sizes(sensorFormat.mbus_code);\n> > +\tSize bestSize;\n> > +\n> > +\tfor (const Size &s : sizes) {\n> > +\t\t/* Ignore smaller sizes. */\n> > +\t\tif (s.width < sensorFormat.size.width ||\n> > +\t\t s.height < sensorFormat.size.height)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\t/* Make sure the width stays in the limits. */\n> > +\t\tif (s.width > maxResolution.width)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tbestSize = s;\n> > +\t\tbreak;\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * This should happen only if the sensor can only produce formats that\n> > +\t * exceed the maximum allowed input width.\n> > +\t */\n> > +\tif (bestSize.isNull()) {\n> > +\t\tLOG(ISI, Error) << \"Unable to find a suitable sensor format\";\n> > +\t\treturn Invalid;\n> > +\t}\n> > +\n> > +\tsensorFormat_.mbus_code = sensorFormat.mbus_code;\n> > +\tsensorFormat_.size = bestSize;\n> > +\n> > +\tLOG(ISI, Debug) << \"Selected sensor format: \" << sensorFormat_;\n> > +\n> > +\treturn status;\n> > +}\n> > +\n> > +/* -----------------------------------------------------------------------------\n> > + * Pipeline Handler\n> > + */\n> > +\n> > +PipelineHandlerISI::PipelineHandlerISI(CameraManager *manager)\n> > +\t: PipelineHandler(manager)\n> > +{\n> > +}\n> > +\n> > +std::unique_ptr\n> > +PipelineHandlerISI::generateConfiguration(Camera *camera,\n> > +\t\t\t\t\t const StreamRoles &roles)\n> > +{\n> > +\tISICameraData *data = cameraData(camera);\n> > +\tstd::unique_ptr config =\n> > +\t\tstd::make_unique(data);\n> > +\n> > +\tif (roles.empty())\n> > +\t\treturn config;\n> > +\n> > +\tif (roles.size() > data->streams_.size()) {\n> > +\t\tLOG(ISI, Error) << \"Only up to \" << data->streams_.size()\n> > +\t\t\t\t<< \" streams are supported\";\n> > +\t\treturn nullptr;\n> > +\t}\n> > +\n> > +\tfor (const auto &role : roles) {\n> > +\t\t/*\n> > +\t\t * Prefer the following formats\n> > +\t\t * - Still Capture: Full resolution YUV422\n> > +\t\t * - Preview/VideoRecording: 1080p YUYV\n>\n> s/preview/viewfinder/\n>\n> > +\t\t * - RAW: sensor's native format and resolution\n> > +\t\t */\n> > +\t\tstd::map> streamFormats;\n>\n> This can move down.\n>\n> > +\t\tPixelFormat pixelFormat;\n> > +\t\tSize size;\n> > +\n> > +\t\tswitch (role) {\n> > +\t\tcase StillCapture:\n> > +\t\t\t/*\n> > +\t\t\t * \\todo Make sure the sensor can produce non-RAW formats\n> > +\t\t\t * compatible with the pipeline supported ones.\n>\n> \"with the ones supported by the pipeline\" or just \"with the pipeline\".\n> Same below.\n>\n> > +\t\t\t */\n> > +\t\t\tsize = data->sensor_->resolution();\n> > +\t\t\tpixelFormat = formats::YUV422;\n> > +\t\t\tbreak;\n> > +\n> > +\t\tcase Viewfinder:\n>\n> You should add VideoRecording here.\n>\n> > +\t\t\t/*\n> > +\t\t\t * \\todo Make sure the sensor can produce non-RAW formats\n> > +\t\t\t * compatible with the pipeline supported ones.\n> > +\t\t\t */\n> > +\t\t\tsize = PipelineHandlerISI::kPreviewSize;\n> > +\t\t\tpixelFormat = formats::YUYV;\n> > +\t\t\tbreak;\n> > +\n> > +\t\tcase Raw: {\n> > +\t\t\t/*\n> > +\t\t\t * Make sure the sensor can generate a RAW format and\n> > +\t\t\t * prefer the ones with a larger bitdepth.\n> > +\t\t\t */\n> > +\t\t\tconst ISICameraConfiguration::FormatMap::value_type *rawPipeFormat = nullptr;\n> > +\t\t\tunsigned int maxDepth = 0;\n> > +\n> > +\t\t\tfor (unsigned int code : data->sensor_->mbusCodes()) {\n> > +\t\t\t\tconst BayerFormat &bayerFormat = BayerFormat::fromMbusCode(code);\n> > +\t\t\t\tif (!bayerFormat.isValid())\n> > +\t\t\t\t\tcontinue;\n> > +\n> > +\t\t\t\t/* Make sure the format is supported by the pipeline handler. */\n> > +\t\t\t\tauto it = std::find_if(ISICameraConfiguration::formatsMap_.begin(),\n> > +\t\t\t\t\t\t ISICameraConfiguration::formatsMap_.end(),\n> > +\t\t\t\t\t\t [code](auto &isiFormat) {\n> > +\t\t\t\t\t\t\t auto &pipe = isiFormat.second;\n> > +\t\t\t\t\t\t\t return pipe.sensorCode == code;\n> > +\t\t\t\t\t\t });\n> > +\t\t\t\tif (it == ISICameraConfiguration::formatsMap_.end())\n> > +\t\t\t\t\tcontinue;\n> > +\n> > +\t\t\t\tif (bayerFormat.bitDepth > maxDepth) {\n> > +\t\t\t\t\tmaxDepth = bayerFormat.bitDepth;\n> > +\t\t\t\t\trawPipeFormat = &(*it);\n> > +\t\t\t\t}\n> > +\t\t\t}\n> > +\n> > +\t\t\tif (!rawPipeFormat) {\n> > +\t\t\t\tLOG(ISI, Error)\n> > +\t\t\t\t\t<< \"Cannot generate a configuration for RAW stream\";\n> > +\t\t\t\treturn nullptr;\n> > +\t\t\t}\n> > +\n> > +\t\t\tsize = data->sensor_->resolution();\n> > +\t\t\tpixelFormat = rawPipeFormat->first;\n> > +\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\n> > +\t\tdefault:\n> > +\t\t\tLOG(ISI, Error) << \"Requested stream role not supported: \" << role;\n> > +\t\t\treturn nullptr;\n> > +\t\t}\n> > +\n>\n> \t\t/* \\todo Add all supported formats */\n>\n> > +\t\tstreamFormats[pixelFormat] = { { kMinISISize, size } };\n> > +\t\tStreamFormats formats(streamFormats);\n> > +\n> > +\t\tStreamConfiguration cfg(formats);\n> > +\t\tcfg.pixelFormat = pixelFormat;\n> > +\t\tcfg.size = size;\n> > +\t\tcfg.bufferCount = 4;\n> > +\t\tconfig->addConfiguration(cfg);\n> > +\t}\n> > +\n> > +\tconfig->validate();\n> > +\n> > +\treturn config;\n> > +}\n> > +\n> > +int PipelineHandlerISI::configure(Camera *camera, CameraConfiguration *c)\n> > +{\n> > +\tISICameraConfiguration *camConfig = static_cast(c);\n> > +\tISICameraData *data = cameraData(camera);\n> > +\n> > +\t/* All links are immutable except the sensor -> csis link. */\n> > +\tconst MediaPad *sensorSrc = data->sensor_->entity()->getPadByIndex(0);\n> > +\tsensorSrc->links()[0]->setEnabled(true);\n> > +\n> > +\t/*\n> > +\t * Reset the crossbar switch routing and enable one route for each\n> > +\t * requested stream configuration.\n> > +\t *\n> > +\t * \\todo Handle concurrent usage of multiple cameras by adjusting the\n> > +\t * routing table instead of resetting it.\n> > +\t */\n> > +\tV4L2Subdevice::Routing routing = {};\n> > +\n> > +\tfor (const auto &[idx, config] : utils::enumerate(*c)) {\n> > +\t\tstruct v4l2_subdev_route route = {\n> > +\t\t\t.sink_pad = data->xbarSink_,\n> > +\t\t\t.sink_stream = 0,\n> > +\t\t\t/*\n> > +\t\t\t * \\todo Verify that the number of crossbar inputs is\n> > +\t\t\t * equal to 3 in all other SoCs.\n> > +\t\t\t */\n>\n> Spoiler: it's not :-) Should you replace the line below with\n>\n> \t\t\t.source_pad = static_cast(crossbar_->pads().size() / 2 + 1),\n>\n> already ?\n>\n\nOk\n\n> > +\t\t\t.source_pad = static_cast(3 + idx),\n> > +\t\t\t.source_stream = 0,\n> > +\t\t\t.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,\n> > +\t\t\t.reserved = {}\n> > +\t\t};\n> > +\n> > +\t\trouting.push_back(route);\n> > +\t}\n> > +\n> > +\tint ret = crossbar_->setRouting(&routing, V4L2Subdevice::ActiveFormat);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\t/* Apply format to the sensor and CSIS receiver. */\n> > +\tV4L2SubdeviceFormat format = camConfig->sensorFormat_;\n> > +\tret = data->sensor_->setFormat(&format);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tret = data->csis_->setFormat(0, &format);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tret = crossbar_->setFormat(data->xbarSink_, &format);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\t/* Now configure the ISI and video node instances, one per stream. */\n> > +\tdata->enabledStreams_.clear();\n> > +\tfor (const auto &config : *c) {\n> > +\t\tPipe *pipe = pipeFromStream(camera, config.stream());\n> > +\n> > +\t\t/*\n> > +\t\t * Set the format on the ISI sink pad: it must match what is\n> > +\t\t * received by the CSIS.\n> > +\t\t */\n> > +\t\tret = pipe->isi->setFormat(0, &format);\n> > +\t\tif (ret)\n> > +\t\t\treturn ret;\n> > +\n> > +\t\t/*\n> > +\t\t * Configure the ISI sink compose rectangle to downscale the\n> > +\t\t * image.\n> > +\t\t *\n> > +\t\t * \\todo Additional cropping could be applied on the ISI source\n> > +\t\t * pad to further reduce the output image size.\n> > +\t\t */\n> > +\t\tRectangle isiScale = {};\n> > +\t\tisiScale.x = 0;\n> > +\t\tisiScale.y = 0;\n> > +\t\tisiScale.width = config.size.width;\n> > +\t\tisiScale.height = config.size.height;\n>\n> \t\tRectangle isiScale{ config.size };\n>\n> > +\n> > +\t\tret = pipe->isi->setSelection(0, V4L2_SEL_TGT_COMPOSE, &isiScale);\n> > +\t\tif (ret)\n> > +\t\t\treturn ret;\n> > +\n> > +\t\t/*\n> > +\t\t * Set the format on ISI source pad: only the media bus code\n> > +\t\t * is relevant as it configures format conversion, while the\n> > +\t\t * size is taken from the sink's COMPOSE (or source's CROP,\n> > +\t\t * if any) rectangles.\n> > +\t\t */\n> > +\t\tconst ISICameraConfiguration::PipeFormat &pipeFormat =\n> > +\t\t\tISICameraConfiguration::formatsMap_.at(config.pixelFormat);\n> > +\n> > +\t\tV4L2SubdeviceFormat isiFormat{};\n> > +\t\tisiFormat.mbus_code = pipeFormat.isiCode;\n> > +\t\tisiFormat.size = config.size;\n> > +\n> > +\t\tret = pipe->isi->setFormat(1, &isiFormat);\n> > +\t\tif (ret)\n> > +\t\t\treturn ret;\n> > +\n> > +\t\tV4L2DeviceFormat captureFmt{};\n> > +\t\tcaptureFmt.fourcc = pipe->capture->toV4L2PixelFormat(config.pixelFormat);\n> > +\t\tcaptureFmt.size = config.size;\n> > +\n> > +\t\t/* \\todo Set stride and format. */\n> > +\t\tret = pipe->capture->setFormat(&captureFmt);\n> > +\t\tif (ret)\n> > +\t\t\treturn ret;\n> > +\n> > +\t\t/* Store the list of enabled streams for later use. */\n> > +\t\tdata->enabledStreams_.push_back(config.stream());\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +int PipelineHandlerISI::exportFrameBuffers(Camera *camera, Stream *stream,\n> > +\t\t\t\t\t std::vector> *buffers)\n> > +{\n> > +\tunsigned int count = stream->configuration().bufferCount;\n> > +\tPipe *pipe = pipeFromStream(camera, stream);\n> > +\n> > +\treturn pipe->capture->exportBuffers(count, buffers);\n> > +}\n> > +\n> > +int PipelineHandlerISI::start(Camera *camera,\n> > +\t\t\t [[maybe_unused]] const ControlList *controls)\n> > +{\n> > +\tISICameraData *data = cameraData(camera);\n> > +\n> > +\tfor (const auto &stream : data->enabledStreams_) {\n> > +\t\tPipe *pipe = pipeFromStream(camera, stream);\n> > +\t\tconst StreamConfiguration &config = stream->configuration();\n> > +\n> > +\t\tint ret = pipe->capture->importBuffers(config.bufferCount);\n> > +\t\tif (ret)\n> > +\t\t\treturn ret;\n> > +\n> > +\t\tret = pipe->capture->streamOn();\n> > +\t\tif (ret)\n> > +\t\t\treturn ret;\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +void PipelineHandlerISI::stopDevice(Camera *camera)\n> > +{\n> > +\tISICameraData *data = cameraData(camera);\n> > +\n> > +\tfor (const auto &stream : data->enabledStreams_) {\n> > +\t\tPipe *pipe = pipeFromStream(camera, stream);\n> > +\n> > +\t\tpipe->capture->streamOff();\n> > +\t\tpipe->capture->releaseBuffers();\n> > +\t}\n> > +}\n> > +\n> > +int PipelineHandlerISI::queueRequestDevice(Camera *camera, Request *request)\n> > +{\n> > +\tfor (auto &[stream, buffer] : request->buffers()) {\n> > +\t\tPipe *pipe = pipeFromStream(camera, stream);\n> > +\n> > +\t\tint ret = pipe->capture->queueBuffer(buffer);\n> > +\t\tif (ret)\n> > +\t\t\treturn ret;\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)\n> > +{\n> > +\tDeviceMatch dm(\"mxc-isi\");\n> > +\tdm.add(\"crossbar\");\n> > +\tdm.add(\"mxc_isi.0\");\n> > +\tdm.add(\"mxc_isi.0.capture\");\n> > +\n> > +\tisiDev_ = acquireMediaDevice(enumerator, dm);\n> > +\tif (!isiDev_)\n> > +\t\treturn false;\n> > +\n> > +\t/*\n> > +\t * Acquire the subdevs and video nodes for the crossbar switch and the\n> > +\t * processing pipelines.\n> > +\t */\n> > +\tcrossbar_ = V4L2Subdevice::fromEntityName(isiDev_, \"crossbar\");\n> > +\tif (!crossbar_)\n> > +\t\treturn false;\n> > +\n> > +\tint ret = crossbar_->open();\n> > +\tif (ret)\n> > +\t\treturn false;\n> > +\n> > +\tfor (unsigned int i = 0; ; ++i) {\n> > +\t\tstd::string entityName = \"mxc_isi.\" + std::to_string(i);\n> > +\t\tstd::unique_ptr isi =\n> > +\t\t\tV4L2Subdevice::fromEntityName(isiDev_, entityName);\n> > +\t\tif (!isi)\n> > +\t\t\tbreak;\n> > +\n> > +\t\tret = isi->open();\n> > +\t\tif (ret)\n> > +\t\t\treturn false;\n> > +\n> > +\t\tentityName += \".capture\";\n> > +\t\tstd::unique_ptr capture =\n> > +\t\t\tV4L2VideoDevice::fromEntityName(isiDev_, entityName);\n> > +\t\tASSERT(capture);\n>\n> When I said \"make it a fatal error\" in the review of v1, I meant \"return\n> false\". Sorry for being unclear. I suppose this works too.\n\nWell, it's a bit harsh, but without capture dev we surely can't\noperate\n\n>\n> > +\n> > +\t\tcapture->bufferReady.connect(this, &PipelineHandlerISI::bufferReady);\n> > +\n> > +\t\tret = capture->open();\n> > +\t\tif (ret)\n> > +\t\t\treturn ret;\n> > +\n> > +\t\tpipes_.push_back({ std::move(isi), std::move(capture) });\n> > +\t}\n> > +\n> > +\tif (pipes_.empty()) {\n> > +\t\tLOG(ISI, Error) << \"Unable to enumerate pipes\";\n> > +\t\treturn false;\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * Loop over all the crossbar switch sink pads to find connected CSI-2\n> > +\t * receivers and camera sensors.\n> > +\t */\n> > +\tunsigned int numCameras = 0;\n> > +\tunsigned int numSinks = 0;\n> > +\tfor (MediaPad *pad : crossbar_->entity()->pads()) {\n> > +\t\tunsigned int sink = numSinks;\n> > +\n> > +\t\tif (!(pad->flags() & MEDIA_PAD_FL_SINK) || pad->links().empty())\n> > +\t\t\tcontinue;\n> > +\n> > +\t\t/*\n> > +\t\t * Count each crossbar sink pad to correctly configure\n> > +\t\t * routing and format for this camera.\n> > +\t\t */\n> > +\t\tnumSinks++;\n> > +\n> > +\t\tMediaEntity *csi = pad->links()[0]->source()->entity();\n> > +\t\tif (csi->pads().size() != 2) {\n> > +\t\t\tLOG(ISI, Debug) << \"Skip unsupported CSI-2 receiver \"\n> > +\t\t\t\t\t<< csi->name();\n> > +\t\t\tcontinue;\n> > +\t\t}\n> > +\n> > +\t\tpad = csi->pads()[0];\n> > +\t\tif (!(pad->flags() & MEDIA_PAD_FL_SINK) || pad->links().empty())\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tMediaEntity *sensor = pad->links()[0]->source()->entity();\n> > +\t\tif (sensor->function() != MEDIA_ENT_F_CAM_SENSOR) {\n> > +\t\t\tLOG(ISI, Debug) << \"Skip unsupported subdevice \"\n> > +\t\t\t\t\t<< sensor->name();\n> > +\t\t\tcontinue;\n> > +\t\t}\n> > +\n> > +\t\t/* Create the camera data. */\n> > +\t\tstd::unique_ptr data =\n> > +\t\t\tstd::make_unique(this);\n> > +\n> > +\t\tdata->sensor_ = std::make_unique(sensor);\n> > +\t\tdata->csis_ = std::make_unique(csi);\n> > +\t\tdata->xbarSink_ = sink;\n> > +\n> > +\t\tret = data->init();\n> > +\t\tif (ret) {\n> > +\t\t\tLOG(ISI, Error) << \"Failed to initialize camera data\";\n> > +\t\t\treturn false;\n> > +\t\t}\n> > +\n> > +\t\t/* Register the camera. */\n> > +\t\tconst std::string &id = data->sensor_->id();\n> > +\t\tstd::set streams;\n> > +\t\tstd::transform(data->streams_.begin(), data->streams_.end(),\n> > +\t\t\t std::inserter(streams, streams.end()),\n> > +\t\t\t [](Stream &s) { return &s; });\n> > +\n> > +\t\tstd::shared_ptr camera =\n> > +\t\t\tCamera::create(std::move(data), id, streams);\n> > +\n> > +\t\tregisterCamera(std::move(camera));\n> > +\t\tnumCameras++;\n> > +\t}\n> > +\n> > +\treturn numCameras > 0;\n> > +}\n> > +\n> > +PipelineHandlerISI::Pipe *PipelineHandlerISI::pipeFromStream(Camera *camera,\n> > +\t\t\t\t\t\t\t const Stream *stream)\n> > +{\n> > +\tISICameraData *data = cameraData(camera);\n> > +\tunsigned int pipeIndex = data->pipeIndex(stream);\n> > +\n> > +\tASSERT(pipeIndex < pipes_.size());\n> > +\n> > +\treturn &pipes_[pipeIndex];\n> > +}\n> > +\n> > +void PipelineHandlerISI::bufferReady(FrameBuffer *buffer)\n> > +{\n> > +\tRequest *request = buffer->request();\n> > +\n> > +\tcompleteBuffer(request, buffer);\n> > +\tif (request->hasPendingBuffers())\n> > +\t\treturn;\n> > +\n> > +\tcompleteRequest(request);\n> > +}\n> > +\n> > +REGISTER_PIPELINE_HANDLER(PipelineHandlerISI)\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/imx8-isi/meson.build b/src/libcamera/pipeline/imx8-isi/meson.build\n> > new file mode 100644\n> > index 000000000000..ffd0ce54ce92\n> > --- /dev/null\n> > +++ b/src/libcamera/pipeline/imx8-isi/meson.build\n> > @@ -0,0 +1,5 @@\n> > +# SPDX-License-Identifier: CC0-1.0\n> > +\n> > +libcamera_sources += files([\n> > + 'imx8-isi.cpp'\n> > +])\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 4A253BD16B\n\tfor ;\n\tMon, 14 Nov 2022 20:35:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A39B76307F;\n\tMon, 14 Nov 2022 21:35:37 +0100 (CET)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::222])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D185B61F3B\n\tfor ;\n\tMon, 14 Nov 2022 21:35:35 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 23B7340006;\n\tMon, 14 Nov 2022 20:35:34 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1668458137;\n\tbh=jKEursGJpJ5PpdMgxZvVkjA2j2ZTj1hSRF3NMANOiWY=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=rl/7jCt/wT/3kVNxsCNvBxzr1VmaJ/yz9UdnIXTaWnFT0lhqL9UOzkhnxNUHkoHEY\n\tRYb7ITKJhNfCGKLzVkk3zcgGaq4DK9tMWDHemkm3UbldO2vprtv03LsUfiNDvBNO9b\n\twNSiylhlnknQoLbQ7zXSynxeqamqmRhb3LedmVezgsjuFBxF5aD6d45y95FXzGkcSm\n\tvNyXs9xmx2zu2N+A0G51OvEozd9HIJandIyFzfMh1Yzuu3f2j+LxCdyao0gcs92P8P\n\tEeFJR1XF5tweE7z9jJyXa6+C7GqfvImhrhNis/rsvucNLqu9cyEeHF1zTUNWlTgfMp\n\tbnAjyHTj/pV3A==","Date":"Mon, 14 Nov 2022 21:35:33 +0100","To":"Laurent Pinchart ","Message-ID":"<20221114203533.wmxud5vnlqiusjpf@uno.localdomain>","References":"<20221110191433.5535-1-jacopo@jmondi.org>\n\t<20221110191433.5535-2-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: pipeline: Add IMX8 ISI\n\tpipeline","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25806,"web_url":"https://patchwork.libcamera.org/comment/25806/","msgid":"","date":"2022-11-16T08:12:26","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: pipeline: Add IMX8 ISI\n\tpipeline","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Nov 14, 2022 at 09:35:33PM +0100, Jacopo Mondi wrote:\n> On Mon, Nov 14, 2022 at 09:06:51PM +0200, Laurent Pinchart wrote:\n> > On Thu, Nov 10, 2022 at 08:14:33PM +0100, Jacopo Mondi via libcamera-devel wrote:\n> > > Add a pipeline handler for the ISI capture interface found on\n> > > several versions of the i.MX8 SoC generation.\n> > >\n> > > The pipeline handler supports capturing multiple streams from the same\n> > > camera in YUV, RGB or RAW formats. The number of streams is limited by\n> > > the number of ISI pipelines, and is currently hardcoded to 2 as the code\n> > > has been tested on the i.MX8MP only. Further development will make this\n> > > dynamic to support other SoCs.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > Reviewed-by: Kieran Bingham \n> > > ---\n> > > meson_options.txt | 2 +-\n> > > src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 957 +++++++++++++++++++\n> > > src/libcamera/pipeline/imx8-isi/meson.build | 5 +\n> > > 3 files changed, 963 insertions(+), 1 deletion(-)\n> > > create mode 100644 src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> > > create mode 100644 src/libcamera/pipeline/imx8-isi/meson.build\n> > >\n> > > diff --git a/meson_options.txt b/meson_options.txt\n> > > index f1d678089452..1ba6778ce257 100644\n> > > --- a/meson_options.txt\n> > > +++ b/meson_options.txt\n> > > @@ -37,7 +37,7 @@ option('lc-compliance',\n> > >\n> > > option('pipelines',\n> > > type : 'array',\n> > > - choices : ['ipu3', 'raspberrypi', 'rkisp1', 'simple', 'uvcvideo', 'vimc'],\n> > > + choices : ['imx8-isi', 'ipu3', 'raspberrypi', 'rkisp1', 'simple', 'uvcvideo', 'vimc'],\n> > > description : 'Select which pipeline handlers to include')\n> > >\n> > > option('qcam',\n> > > diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> > > new file mode 100644\n> > > index 000000000000..6e4b1ae290ef\n> > > --- /dev/null\n> > > +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> > > @@ -0,0 +1,957 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2022 - Jacopo Mondi \n> > > + *\n> > > + * imx8-isi.cpp - Pipeline handler for ISI interface found on NXP i.MX8 SoC\n> > > + */\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \"libcamera/internal/bayer_format.h\"\n> > > +#include \"libcamera/internal/camera.h\"\n> > > +#include \"libcamera/internal/camera_sensor.h\"\n> > > +#include \"libcamera/internal/device_enumerator.h\"\n> > > +#include \"libcamera/internal/media_device.h\"\n> > > +#include \"libcamera/internal/pipeline_handler.h\"\n> > > +#include \"libcamera/internal/v4l2_subdevice.h\"\n> > > +#include \"libcamera/internal/v4l2_videodevice.h\"\n> > > +\n> > > +#include \"linux/media-bus-format.h\"\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +LOG_DEFINE_CATEGORY(ISI)\n> > > +\n> > > +class PipelineHandlerISI;\n> > > +\n> > > +class ISICameraData : public Camera::Private\n> > > +{\n> > > +public:\n> > > +\tISICameraData(PipelineHandler *ph)\n> > > +\t\t: Camera::Private(ph)\n> > > +\t{\n> > > +\t\t/*\n> > > +\t\t * \\todo Assume 2 channels only for now, as that's the number of\n> > > +\t\t * available channels on i.MX8MP.\n> > > +\t\t */\n> > > +\t\tstreams_.resize(2);\n> > > +\t}\n> > > +\n> > > +\tPipelineHandlerISI *pipe();\n> > > +\n> > > +\tint init();\n> > > +\n> > > +\tunsigned int pipeIndex(const Stream *stream)\n> > > +\t{\n> > > +\t\treturn stream - &*streams_.begin();\n> > > +\t}\n> > > +\n> > > +\tstd::unique_ptr sensor_;\n> > > +\tstd::unique_ptr csis_;\n> > > +\n> > > +\tstd::vector streams_;\n> > > +\n> > > +\tstd::vector enabledStreams_;\n> > > +\n> > > +\tunsigned int xbarSink_;\n> > > +};\n> > > +\n> > > +class ISICameraConfiguration : public CameraConfiguration\n> > > +{\n> > > +public:\n> > > +\t/*\n> > > +\t * formatsMap_ records the association between an output pixel format\n> > > +\t * and the combination of V4L2 pixel format and media bus codes that have\n> > > +\t * to be applied to the pipeline.\n> > > +\t */\n> > > +\tstruct PipeFormat {\n> > > +\t\tunsigned int isiCode;\n> > > +\t\tunsigned int sensorCode;\n> > > +\t};\n> > > +\n> > > +\tusing FormatMap = std::map;\n> > > +\n> > > +\tISICameraConfiguration(ISICameraData *data)\n> > > +\t\t: data_(data)\n> > > +\t{\n> > > +\t}\n> > > +\n> > > +\tStatus validate() override;\n> > > +\n> > > +\tstatic const FormatMap formatsMap_;\n> > > +\n> > > +\tV4L2SubdeviceFormat sensorFormat_;\n> > > +\n> > > +private:\n> > > +\tconst ISICameraData *data_;\n> > > +\n> > > +\tCameraConfiguration::Status validateRaw(std::set &availableStreams,\n> > > +\t\t\t\t\t\tconst Size &maxResolution);\n> > > +\tCameraConfiguration::Status validateYuv(std::set &availableStreams,\n> > > +\t\t\t\t\t\tconst Size &maxResolution);\n> >\n> > Functions should go before data.\n> \n> Ack\n> \n> > > +};\n> > > +\n> > > +class PipelineHandlerISI : public PipelineHandler\n> > > +{\n> > > +public:\n> > > +\tPipelineHandlerISI(CameraManager *manager);\n> > > +\n> > > +\tbool match(DeviceEnumerator *enumerator) override;\n> > > +\n> > > +\tstd::unique_ptr generateConfiguration(Camera *camera,\n> > > +\t\t\t\t\t\t\t\t const StreamRoles &roles) override;\n> >\n> > You can also fold it as\n> >\n> > \tstd::unique_ptr\n> > \tgenerateConfiguration(Camera *camera, const StreamRoles &roles) override;\n> >\n> > to reduce the line length.\n> \n> ok..\n> \n> >\n> > > +\tint configure(Camera *camera, CameraConfiguration *config) override;\n> > > +\n> > > +\tint exportFrameBuffers(Camera *camera, Stream *stream,\n> > > +\t\t\t std::vector> *buffers) override;\n> > > +\n> > > +\tint start(Camera *camera, const ControlList *controls) override;\n> > > +\n> > > +protected:\n> > > +\tvoid stopDevice(Camera *camera) override;\n> > > +\n> > > +\tint queueRequestDevice(Camera *camera, Request *request) override;\n> > > +\n> > > +private:\n> > > +\tstatic constexpr Size kPreviewSize = { 1920, 1080 };\n> > > +\tstatic constexpr Size kMinISISize = { 1, 1 };\n> > > +\n> > > +\tstruct Pipe {\n> > > +\t\tstd::unique_ptr isi;\n> > > +\t\tstd::unique_ptr capture;\n> > > +\t};\n> > > +\n> > > +\tISICameraData *cameraData(Camera *camera)\n> > > +\t{\n> > > +\t\treturn static_cast(camera->_d());\n> > > +\t}\n> > > +\n> > > +\tPipe *pipeFromStream(Camera *camera, const Stream *stream);\n> > > +\n> > > +\tvoid bufferReady(FrameBuffer *buffer);\n> > > +\n> > > +\tMediaDevice *isiDev_;\n> > > +\n> > > +\tstd::unique_ptr crossbar_;\n> > > +\tstd::vector pipes_;\n> > > +};\n> > > +\n> > > +/* -----------------------------------------------------------------------------\n> > > + * Camera Data\n> > > + */\n> > > +\n> > > +PipelineHandlerISI *ISICameraData::pipe()\n> > > +{\n> > > +\treturn static_cast(Camera::Private::pipe());\n> > > +}\n> > > +\n> > > +/* Open and initialize pipe components. */\n> > > +int ISICameraData::init()\n> > > +{\n> > > +\tint ret = sensor_->init();\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > +\tret = csis_->open();\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > +\tproperties_ = sensor_->properties();\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +/* -----------------------------------------------------------------------------\n> > > + * Camera Configuration\n> > > + */\n> > > +\n> > > +const ISICameraConfiguration::FormatMap ISICameraConfiguration::formatsMap_ = {\n> >\n> > Could you add a todo comment to remind that the sensor format selection\n> > should not be hardcoded ?\n> >\n> \n> Yes, it's reported in the code but I'll add here\n> \n> > > +\t{\n> > > +\t\tformats::YUV422,\n> > > +\t\t{ MEDIA_BUS_FMT_YUV8_1X24,\n> > > +\t\t MEDIA_BUS_FMT_UYVY8_1X16 },\n> > > +\t},\n> > > +\t{\n> > > +\t\tformats::YUYV,\n> > > +\t\t{ MEDIA_BUS_FMT_YUV8_1X24,\n> > > +\t\t MEDIA_BUS_FMT_UYVY8_1X16 },\n> > > +\t},\n> > > +\t{\n> > > +\t\tformats::RGB565,\n> > > +\t\t{ MEDIA_BUS_FMT_RGB888_1X24,\n> > > +\t\t MEDIA_BUS_FMT_RGB565_1X16 },\n> > > +\t},\n> > > +\t{\n> > > +\t\tformats::SBGGR8,\n> > > +\t\t{ MEDIA_BUS_FMT_SBGGR8_1X8,\n> > > +\t\t MEDIA_BUS_FMT_SBGGR8_1X8 },\n> > > +\t},\n> > > +\t{\n> > > +\t\tformats::SGBRG8,\n> > > +\t\t{ MEDIA_BUS_FMT_SGBRG8_1X8,\n> > > +\t\t MEDIA_BUS_FMT_SGBRG8_1X8 },\n> > > +\t},\n> > > +\t{\n> > > +\t\tformats::SGRBG8,\n> > > +\t\t{ MEDIA_BUS_FMT_SGRBG8_1X8,\n> > > +\t\t MEDIA_BUS_FMT_SGRBG8_1X8 },\n> > > +\t},\n> > > +\t{\n> > > +\t\tformats::SRGGB8,\n> > > +\t\t{ MEDIA_BUS_FMT_SRGGB8_1X8,\n> > > +\t\t MEDIA_BUS_FMT_SRGGB8_1X8 },\n> > > +\t},\n> > > +\t{\n> > > +\t\tformats::SBGGR10,\n> > > +\t\t{ MEDIA_BUS_FMT_SBGGR10_1X10,\n> > > +\t\t MEDIA_BUS_FMT_SBGGR10_1X10 },\n> > > +\t},\n> > > +\t{\n> > > +\t\tformats::SGBRG10,\n> > > +\t\t{ MEDIA_BUS_FMT_SGBRG10_1X10,\n> > > +\t\t MEDIA_BUS_FMT_SGBRG10_1X10 },\n> > > +\t},\n> > > +\t{\n> > > +\t\tformats::SGRBG10,\n> > > +\t\t{ MEDIA_BUS_FMT_SGRBG10_1X10,\n> > > +\t\t MEDIA_BUS_FMT_SGRBG10_1X10 },\n> > > +\t},\n> > > +\t{\n> > > +\t\tformats::SRGGB10,\n> > > +\t\t{ MEDIA_BUS_FMT_SRGGB10_1X10,\n> > > +\t\t MEDIA_BUS_FMT_SRGGB10_1X10 },\n> > > +\t},\n> > > +\t{\n> > > +\t\tformats::SBGGR12,\n> > > +\t\t{ MEDIA_BUS_FMT_SBGGR12_1X12,\n> > > +\t\t MEDIA_BUS_FMT_SBGGR12_1X12 },\n> > > +\t},\n> > > +\t{\n> > > +\t\tformats::SGBRG12,\n> > > +\t\t{ MEDIA_BUS_FMT_SGBRG12_1X12,\n> > > +\t\t MEDIA_BUS_FMT_SGBRG12_1X12 },\n> > > +\t},\n> > > +\t{\n> > > +\t\tformats::SGRBG12,\n> > > +\t\t{ MEDIA_BUS_FMT_SGRBG12_1X12,\n> > > +\t\t MEDIA_BUS_FMT_SGRBG12_1X12 },\n> > > +\t},\n> > > +\t{\n> > > +\t\tformats::SRGGB12,\n> > > +\t\t{ MEDIA_BUS_FMT_SRGGB12_1X12,\n> > > +\t\t MEDIA_BUS_FMT_SRGGB12_1X12 },\n> > > +\t},\n> > > +};\n> > > +\n> > > +/*\n> > > + * Adjust stream configuration when the first requested stream is RAW: all the\n> > > + * streams will have the same RAW pixelformat and size.\n> > > + */\n> > > +CameraConfiguration::Status ISICameraConfiguration::validateRaw(std::set &availableStreams,\n> > > +\t\t\t\t\t\t\t\tconst Size &maxResolution)\n> >\n> > CameraConfiguration::Status\n> > ISICameraConfiguration::validateRaw(std::set &availableStreams,\n> > \t\t\t\t const Size &maxResolution)\n> >\n> > > +{\n> > > +\tCameraConfiguration::Status status = Valid;\n> > > +\n> > > +\t/*\n> > > +\t * Make sure the requested RAW format is supported by the\n> > > +\t * pipeline, otherwise adjust it.\n> > > +\t */\n> > > +\tstd::vector mbusCodes = data_->sensor_->mbusCodes();\n> > > +\tStreamConfiguration &rawConfig = config_[0];\n> > > +\n> > > +\tbool supported = false;\n> > > +\tauto it = formatsMap_.find(rawConfig.pixelFormat);\n> > > +\tif (it != formatsMap_.end()) {\n> > > +\t\tunsigned int mbusCode = it->second.sensorCode;\n> > > +\n> > > +\t\tif (std::count(mbusCodes.begin(), mbusCodes.end(), mbusCode))\n> > > +\t\t\tsupported = true;\n> > > +\t}\n> > > +\n> > > +\tif (!supported) {\n> > > +\t\t/*\n> > > +\t\t * Adjust to the first mbus code supported by both the\n> > > +\t\t * sensor and the pipeline.\n> > > +\t\t */\n> > > +\t\tconst FormatMap::value_type *pipeConfig = nullptr;\n> > > +\t\tfor (unsigned int code : data_->sensor_->mbusCodes()) {\n> >\n> > You can reuse the mbusCodes variables.\n> >\n> \n> ouch, that was actualy my idea initially\n> \n> > > +\t\t\tconst BayerFormat &bayerFormat = BayerFormat::fromMbusCode(code);\n> > > +\t\t\tif (!bayerFormat.isValid())\n> > > +\t\t\t\tcontinue;\n> > > +\n> > > +\t\t\tauto fmt = std::find_if(ISICameraConfiguration::formatsMap_.begin(),\n> > > +\t\t\t\t\t\tISICameraConfiguration::formatsMap_.end(),\n> > > +\t\t\t\t\t\t[code](auto &isiFormat) {\n> >\n> > const auto\n> >\n> > > +\t\t\t\t\t\t\tauto &pipe = isiFormat.second;\n> >\n> > Same here.\n> >\n> > > +\t\t\t\t\t\t\treturn pipe.sensorCode == code;\n> > > +\t\t\t\t\t\t});\n> > > +\n> > > +\t\t\tif (fmt == ISICameraConfiguration::formatsMap_.end())\n> > > +\t\t\t\tcontinue;\n> > > +\n> > > +\t\t\tpipeConfig = &(*fmt);\n> > > +\t\t\tbreak;\n> > > +\t\t}\n> > > +\n> > > +\t\tif (!pipeConfig) {\n> > > +\t\t\tLOG(ISI, Error) << \"Cannot adjust RAW format \"\n> > > +\t\t\t\t\t<< rawConfig.pixelFormat;\n> > > +\t\t\treturn Invalid;\n> > > +\t\t}\n> > > +\n> > > +\t\trawConfig.pixelFormat = pipeConfig->first;\n> > > +\t\tLOG(ISI, Debug) << \"RAW pixelformat adjusted to \"\n> > > +\t\t\t\t<< pipeConfig->first;\n> > > +\t\tstatus = Adjusted;\n> > > +\t}\n> > > +\n> > > +\t/* Cap the RAW stream size to the maximum resolution. */\n> > > +\tSize configSize = rawConfig.size;\n> >\n> > const\n> >\n> > > +\trawConfig.size.boundTo(maxResolution);\n> >\n> > I don't think this is enough, not all sizes smaller than the sensor\n> > resolution can be achieved as the pipeline can't scale raw formats. It\n> > can be addressed on top.\n> >\n> > > +\tif (rawConfig.size != configSize) {\n> > > +\t\tLOG(ISI, Debug) << \"RAW size adjusted to \"\n> > > +\t\t\t\t<< rawConfig.size;\n> > > +\t\tstatus = Adjusted;\n> > > +\t}\n> > > +\n> > > +\t/* Adjust all other streams to RAW. */\n> > > +\tunsigned int i = 0;\n> > > +\tfor (StreamConfiguration &cfg : config_) {\n> >\n> > \tfor (auto &[i, cfg] : utils::enumerate(config_)) {\n> >\n> \n> Nope, I started with that too\n> \n> ../src/libcamera/pipeline/imx8-isi/imx8-isi.cpp: In member function ‘libcamera::CameraConfiguration::Status libcamera::ISICameraConfiguration::validateRaw(std::set&, const libcamera::Size&)’:\n> ../src/libcamera/pipeline/imx8-isi/imx8-isi.cpp:331:55: error: cannot bind non-const lvalue reference of type ‘std::pair&’ to an rvalue of type ‘libcamera::utils::details::enumerate_iterator<__gnu_cxx::__normal_iterator > >::value_type’ {aka ‘std::pair’}\n> 331 | for (auto &[i, cfg] : utils::enumerate(config_)) {\n> \n> Cannot bind non-const lvalue reference of type ‘std::pair&’\n> to an rvalue of type std::pair\n> \n> I can fix it by removing the reference, but then the\n> StreamCOnfiguration would be copied.\n> \n> I've not investigated why\n\n \tfor (const auto &[i, cfg] : utils::enumerate(config_)) {\n\nSimple, right ? :-)\n\nIt's a tricky one, utils::enumerate() returns a class instance\n(enumerate_adapter) that has begin() and end() functions. Those\nfunctions return an enumerate_iterator instance, whose value_type is a\nstd::pair. The C++ structured binding\ndeclaration creates a hidden variable e that holds the value of the\ninitializer, so without const we would essentially do\n\n\tenumerate_iterator it = ...;\n\tauto &e = *it;\n\nenumerate_iterator::operator*() returns an rvalue, and you can't bind a\nnon-const lvalue reference to an rvalue. The const qualifier fixes it.\n\nThen, the i and cfg names are bound to the first and second members of\nthe hidden variable e. As e is a std::pair,\ni takes the type const std::size, and cfg takes the type base_reference\n(which in this case is a StreamConfig &). The const qualifier in the\nexpression\n\n \tfor (const auto &[i, cfg] : utils::enumerate(config_)) {\n\ndoesn't apply to i and cfg, only to the hidden variable e.\n\n> > > +\n> > > +\t\tLOG(ISI, Debug) << \"Stream \" << i << \": \" << cfg.toString();\n> > > +\t\tPixelFormat pixFmt = cfg.pixelFormat;\n> > > +\t\tSize size = cfg.size;\n> >\n> > const for both.\n> >\n> > > +\n> > > +\t\tcfg.pixelFormat = rawConfig.pixelFormat;\n> > > +\t\tcfg.size = rawConfig.size;\n> > > +\n> > > +\t\tif (cfg.pixelFormat != pixFmt ||\n> > > +\t\t cfg.size != size) {\n> >\n> > This holds on a single line.\n> >\n> > > +\t\t\tLOG(ISI, Debug) << \"Stream \" << i << \" adjusted to \"\n> > > +\t\t\t\t\t<< cfg.toString();\n> > > +\t\t\tstatus = Adjusted;\n> > > +\t\t}\n> > > +\n> > > +\t\tPixelFormatInfo info = PixelFormatInfo::info(cfg.pixelFormat);\n> >\n> > const reference. It would be nice to disable copy and move for the\n> > PixelFormatInfo class.\n> >\n> \n> ack\n> \n> > > +\t\tcfg.stride = info.stride(cfg.size.width, 0);\n> > > +\t\tcfg.frameSize = info.frameSize(cfg.size, info.bitsPerPixel);\n> > > +\n> > > +\t\t/* Assign streams in the order they are presented. */\n> > > +\t\tauto stream = availableStreams.extract(availableStreams.begin());\n> > > +\t\tcfg.setStream(stream.value());\n> > > +\n> > > +\t\ti++;\n> > > +\t}\n> > > +\n> > > +\treturn status;\n> > > +}\n> > > +\n> > > +/*\n> > > + * Adjust stream configuration when the first requested stream is not RAW: all\n> > > + * the streams will be either YUV or RGB processed formats.\n> > > + */\n> > > +CameraConfiguration::Status ISICameraConfiguration::validateYuv(std::set &availableStreams,\n> > > +\t\t\t\t\t\t\t\tconst Size &maxResolution)\n> >\n> > CameraConfiguration::Status\n> > ISICameraConfiguration::validateYuv(std::set &availableStreams,\n> > \t\t\t\t const Size &maxResolution)\n> >\n> > > +{\n> > > +\tCameraConfiguration::Status status = Valid;\n> > > +\n> > > +\tunsigned int i = 0;\n> > > +\tfor (StreamConfiguration &cfg : config_) {\n> >\n> > util::enumerate() here too.\n> >\n> \n> same\n> \n> > > +\n> > > +\t\tLOG(ISI, Debug) << \"Stream \" << i << \": \" << cfg.toString();\n> > > +\n> > > +\t\t/* If the stream is RAW or not supported default it to YUYV. */\n> > > +\t\tPixelFormatInfo info = PixelFormatInfo::info(cfg.pixelFormat);\n> >\n> > const reference\n> >\n> > > +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW ||\n> > > +\t\t !formatsMap_.count(cfg.pixelFormat)) {\n> > > +\n> > > +\t\t\tLOG(ISI, Debug) << \"Stream \" << i << \" format: \"\n> > > +\t\t\t\t\t<< cfg.pixelFormat << \" adjusted to YUYV\";\n> > > +\n> > > +\t\t\tcfg.pixelFormat = formats::YUYV;\n> > > +\t\t\tstatus = Adjusted;\n> > > +\t\t}\n> > > +\n> > > +\t\t/* Cap the streams size to the maximum accepted resolution. */\n> > > +\t\tSize configSize = cfg.size;\n> > > +\t\tcfg.size.boundTo(maxResolution);\n> > > +\t\tif (cfg.size != configSize) {\n> > > +\t\t\tLOG(ISI, Debug)\n> > > +\t\t\t\t<< \"Stream \" << i << \" adjusted to \" << cfg.size;\n> > > +\t\t\tstatus = Adjusted;\n> > > +\t\t}\n> > > +\n> > > +\t\t/* Re-fetch the pixel format info in case it has been adjusted. */\n> > > +\t\tinfo = PixelFormatInfo::info(cfg.pixelFormat);\n> > > +\n> > > +\t\t/* \\todo Multiplane ? */\n> > > +\t\tcfg.stride = info.stride(cfg.size.width, 0);\n> > > +\t\tcfg.frameSize = info.frameSize(cfg.size, info.bitsPerPixel);\n> > > +\n> > > +\t\t/* Assign streams in the order they are presented. */\n> > > +\t\tauto stream = availableStreams.extract(availableStreams.begin());\n> > > +\t\tcfg.setStream(stream.value());\n> > > +\n> > > +\t\ti++;\n> > > +\t}\n> > > +\n> > > +\treturn status;\n> > > +}\n> > > +\n> > > +CameraConfiguration::Status ISICameraConfiguration::validate()\n> > > +{\n> > > +\tStatus status = Valid;\n> > > +\n> > > +\tstd::set availableStreams;\n> > > +\tstd::transform(data_->streams_.begin(), data_->streams_.end(),\n> > > +\t\t std::inserter(availableStreams, availableStreams.end()),\n> > > +\t\t [](const Stream &s) { return const_cast(&s); });\n> > > +\n> > > +\tif (config_.empty())\n> > > +\t\treturn Invalid;\n> > > +\n> > > +\t/* Cap the number of streams to the number of available ISI pipes. */\n> > > +\tif (config_.size() > availableStreams.size()) {\n> > > +\t\tconfig_.resize(availableStreams.size());\n> > > +\t\tstatus = Adjusted;\n> > > +\t}\n> > > +\n> > > +\t/*\n> > > +\t * If more than a single stream is requested, the maximum allowed input\n> > > +\t * image width is 2048. Cap the maximum image size accordingly.\n> > > +\t *\n> > > +\t * \\todo The (size > 1) check only applies to i.MX8MP which has 2 ISI\n> > > +\t * channels. SoCs with more channels than the i.MX8MP are capable of\n> > > +\t * supporting more streams with input width > 2048 by chaining\n> > > +\t * successive channels together. Define a policy for channels allocation\n> > > +\t * to fully support other SoCs.\n> > > +\t */\n> > > +\tCameraSensor *sensor = data_->sensor_.get();\n> > > +\tSize maxResolution = sensor->resolution();\n> > > +\tif (config_.size() > 1)\n> > > +\t\tmaxResolution.width = std::min(2048U, maxResolution.width);\n> > > +\n> > > +\t/* Validate streams according to the format of the first one. */\n> > > +\tconst PixelFormatInfo info = PixelFormatInfo::info(config_[0].pixelFormat);\n> > > +\n> > > +\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> > > +\t\tstatus = validateRaw(availableStreams, maxResolution);\n> > > +\telse\n> > > +\t\tstatus = validateYuv(availableStreams, maxResolution);\n> >\n> > This may override the Adjusted status set when capping the number of\n> > streams.\n> >\n> \n> Right. I think I will have to expand it then or use an additional\n> variable\n> \n> > > +\n> > > +\tif (status == Invalid)\n> > > +\t\treturn status;\n> > > +\n> > > +\t/*\n> > > +\t * Sensor format selection policy: the first stream selects the media\n> > > +\t * bus code to use, the largest stream selects the size.\n> > > +\t *\n> > > +\t * \\todo The sensor format selection policy could be changed to\n> > > +\t * prefer operating the sensor at full resolution to prioritize\n> > > +\t * image quality and FOV in exchange of a usually slower frame rate.\n> >\n> > Binning won't affect the field of view, I wouldn't mention FOV here.\n> \n> Who has mentioned binning ? smaller modes can be obtained by cropping\n> too\n\nThey really shouldn't, but that's a different story, it should be fixed\non the kernel side. They should never be any implicit cropping when\nsetting a format.\n\n> I can drop it if controversial\n> \n> > > +\t * Usage of the STILL_CAPTURE role could be consider for this.\n> > > +\t */\n> > > +\tconst PipeFormat &pipeFmt = formatsMap_.at(config_[0].pixelFormat);\n> > > +\n> > > +\tSize maxSize;\n> > > +\tfor (const auto &cfg : config_) {\n> > > +\t\tif (cfg.size > maxSize)\n> > > +\t\t\tmaxSize = cfg.size;\n> > > +\t}\n> > > +\n> > > +\tV4L2SubdeviceFormat sensorFormat{};\n> > > +\tsensorFormat.mbus_code = pipeFmt.sensorCode;\n> > > +\tsensorFormat.size = maxSize;\n> > > +\n> > > +\tLOG(ISI, Debug) << \"Computed sensor configuration: \" << sensorFormat;\n> > > +\n> > > +\t/*\n> > > +\t * We can't use CameraSensor::getFormat() as it might return a\n> > > +\t * format larger than our strict width limit, as that function\n> > > +\t * prioritizes formats with the same FOV ratio over formats with less\n> >\n> > s/FOV ratio/aspect ratio/\n> >\n> > > +\t * difference in size.\n> > > +\t *\n> > > +\t * Manually walk all the sensor supported sizes searching for\n> > > +\t * the smallest larger format without considering the FOV ratio\n> >\n> > Ditto.\n> >\n> > > +\t * as the ISI can freely scale.\n> > > +\t */\n> > > +\tauto sizes = sensor->sizes(sensorFormat.mbus_code);\n> > > +\tSize bestSize;\n> > > +\n> > > +\tfor (const Size &s : sizes) {\n> > > +\t\t/* Ignore smaller sizes. */\n> > > +\t\tif (s.width < sensorFormat.size.width ||\n> > > +\t\t s.height < sensorFormat.size.height)\n> > > +\t\t\tcontinue;\n> > > +\n> > > +\t\t/* Make sure the width stays in the limits. */\n> > > +\t\tif (s.width > maxResolution.width)\n> > > +\t\t\tcontinue;\n> > > +\n> > > +\t\tbestSize = s;\n> > > +\t\tbreak;\n> > > +\t}\n> > > +\n> > > +\t/*\n> > > +\t * This should happen only if the sensor can only produce formats that\n> > > +\t * exceed the maximum allowed input width.\n> > > +\t */\n> > > +\tif (bestSize.isNull()) {\n> > > +\t\tLOG(ISI, Error) << \"Unable to find a suitable sensor format\";\n> > > +\t\treturn Invalid;\n> > > +\t}\n> > > +\n> > > +\tsensorFormat_.mbus_code = sensorFormat.mbus_code;\n> > > +\tsensorFormat_.size = bestSize;\n> > > +\n> > > +\tLOG(ISI, Debug) << \"Selected sensor format: \" << sensorFormat_;\n> > > +\n> > > +\treturn status;\n> > > +}\n> > > +\n> > > +/* -----------------------------------------------------------------------------\n> > > + * Pipeline Handler\n> > > + */\n> > > +\n> > > +PipelineHandlerISI::PipelineHandlerISI(CameraManager *manager)\n> > > +\t: PipelineHandler(manager)\n> > > +{\n> > > +}\n> > > +\n> > > +std::unique_ptr\n> > > +PipelineHandlerISI::generateConfiguration(Camera *camera,\n> > > +\t\t\t\t\t const StreamRoles &roles)\n> > > +{\n> > > +\tISICameraData *data = cameraData(camera);\n> > > +\tstd::unique_ptr config =\n> > > +\t\tstd::make_unique(data);\n> > > +\n> > > +\tif (roles.empty())\n> > > +\t\treturn config;\n> > > +\n> > > +\tif (roles.size() > data->streams_.size()) {\n> > > +\t\tLOG(ISI, Error) << \"Only up to \" << data->streams_.size()\n> > > +\t\t\t\t<< \" streams are supported\";\n> > > +\t\treturn nullptr;\n> > > +\t}\n> > > +\n> > > +\tfor (const auto &role : roles) {\n> > > +\t\t/*\n> > > +\t\t * Prefer the following formats\n> > > +\t\t * - Still Capture: Full resolution YUV422\n> > > +\t\t * - Preview/VideoRecording: 1080p YUYV\n> >\n> > s/preview/viewfinder/\n> >\n> > > +\t\t * - RAW: sensor's native format and resolution\n> > > +\t\t */\n> > > +\t\tstd::map> streamFormats;\n> >\n> > This can move down.\n> >\n> > > +\t\tPixelFormat pixelFormat;\n> > > +\t\tSize size;\n> > > +\n> > > +\t\tswitch (role) {\n> > > +\t\tcase StillCapture:\n> > > +\t\t\t/*\n> > > +\t\t\t * \\todo Make sure the sensor can produce non-RAW formats\n> > > +\t\t\t * compatible with the pipeline supported ones.\n> >\n> > \"with the ones supported by the pipeline\" or just \"with the pipeline\".\n> > Same below.\n> >\n> > > +\t\t\t */\n> > > +\t\t\tsize = data->sensor_->resolution();\n> > > +\t\t\tpixelFormat = formats::YUV422;\n> > > +\t\t\tbreak;\n> > > +\n> > > +\t\tcase Viewfinder:\n> >\n> > You should add VideoRecording here.\n> >\n> > > +\t\t\t/*\n> > > +\t\t\t * \\todo Make sure the sensor can produce non-RAW formats\n> > > +\t\t\t * compatible with the pipeline supported ones.\n> > > +\t\t\t */\n> > > +\t\t\tsize = PipelineHandlerISI::kPreviewSize;\n> > > +\t\t\tpixelFormat = formats::YUYV;\n> > > +\t\t\tbreak;\n> > > +\n> > > +\t\tcase Raw: {\n> > > +\t\t\t/*\n> > > +\t\t\t * Make sure the sensor can generate a RAW format and\n> > > +\t\t\t * prefer the ones with a larger bitdepth.\n> > > +\t\t\t */\n> > > +\t\t\tconst ISICameraConfiguration::FormatMap::value_type *rawPipeFormat = nullptr;\n> > > +\t\t\tunsigned int maxDepth = 0;\n> > > +\n> > > +\t\t\tfor (unsigned int code : data->sensor_->mbusCodes()) {\n> > > +\t\t\t\tconst BayerFormat &bayerFormat = BayerFormat::fromMbusCode(code);\n> > > +\t\t\t\tif (!bayerFormat.isValid())\n> > > +\t\t\t\t\tcontinue;\n> > > +\n> > > +\t\t\t\t/* Make sure the format is supported by the pipeline handler. */\n> > > +\t\t\t\tauto it = std::find_if(ISICameraConfiguration::formatsMap_.begin(),\n> > > +\t\t\t\t\t\t ISICameraConfiguration::formatsMap_.end(),\n> > > +\t\t\t\t\t\t [code](auto &isiFormat) {\n> > > +\t\t\t\t\t\t\t auto &pipe = isiFormat.second;\n> > > +\t\t\t\t\t\t\t return pipe.sensorCode == code;\n> > > +\t\t\t\t\t\t });\n> > > +\t\t\t\tif (it == ISICameraConfiguration::formatsMap_.end())\n> > > +\t\t\t\t\tcontinue;\n> > > +\n> > > +\t\t\t\tif (bayerFormat.bitDepth > maxDepth) {\n> > > +\t\t\t\t\tmaxDepth = bayerFormat.bitDepth;\n> > > +\t\t\t\t\trawPipeFormat = &(*it);\n> > > +\t\t\t\t}\n> > > +\t\t\t}\n> > > +\n> > > +\t\t\tif (!rawPipeFormat) {\n> > > +\t\t\t\tLOG(ISI, Error)\n> > > +\t\t\t\t\t<< \"Cannot generate a configuration for RAW stream\";\n> > > +\t\t\t\treturn nullptr;\n> > > +\t\t\t}\n> > > +\n> > > +\t\t\tsize = data->sensor_->resolution();\n> > > +\t\t\tpixelFormat = rawPipeFormat->first;\n> > > +\n> > > +\t\t\tbreak;\n> > > +\t\t}\n> > > +\n> > > +\t\tdefault:\n> > > +\t\t\tLOG(ISI, Error) << \"Requested stream role not supported: \" << role;\n> > > +\t\t\treturn nullptr;\n> > > +\t\t}\n> > > +\n> >\n> > \t\t/* \\todo Add all supported formats */\n> >\n> > > +\t\tstreamFormats[pixelFormat] = { { kMinISISize, size } };\n> > > +\t\tStreamFormats formats(streamFormats);\n> > > +\n> > > +\t\tStreamConfiguration cfg(formats);\n> > > +\t\tcfg.pixelFormat = pixelFormat;\n> > > +\t\tcfg.size = size;\n> > > +\t\tcfg.bufferCount = 4;\n> > > +\t\tconfig->addConfiguration(cfg);\n> > > +\t}\n> > > +\n> > > +\tconfig->validate();\n> > > +\n> > > +\treturn config;\n> > > +}\n> > > +\n> > > +int PipelineHandlerISI::configure(Camera *camera, CameraConfiguration *c)\n> > > +{\n> > > +\tISICameraConfiguration *camConfig = static_cast(c);\n> > > +\tISICameraData *data = cameraData(camera);\n> > > +\n> > > +\t/* All links are immutable except the sensor -> csis link. */\n> > > +\tconst MediaPad *sensorSrc = data->sensor_->entity()->getPadByIndex(0);\n> > > +\tsensorSrc->links()[0]->setEnabled(true);\n> > > +\n> > > +\t/*\n> > > +\t * Reset the crossbar switch routing and enable one route for each\n> > > +\t * requested stream configuration.\n> > > +\t *\n> > > +\t * \\todo Handle concurrent usage of multiple cameras by adjusting the\n> > > +\t * routing table instead of resetting it.\n> > > +\t */\n> > > +\tV4L2Subdevice::Routing routing = {};\n> > > +\n> > > +\tfor (const auto &[idx, config] : utils::enumerate(*c)) {\n> > > +\t\tstruct v4l2_subdev_route route = {\n> > > +\t\t\t.sink_pad = data->xbarSink_,\n> > > +\t\t\t.sink_stream = 0,\n> > > +\t\t\t/*\n> > > +\t\t\t * \\todo Verify that the number of crossbar inputs is\n> > > +\t\t\t * equal to 3 in all other SoCs.\n> > > +\t\t\t */\n> >\n> > Spoiler: it's not :-) Should you replace the line below with\n> >\n> > \t\t\t.source_pad = static_cast(crossbar_->pads().size() / 2 + 1),\n> >\n> > already ?\n> >\n> \n> Ok\n> \n> > > +\t\t\t.source_pad = static_cast(3 + idx),\n> > > +\t\t\t.source_stream = 0,\n> > > +\t\t\t.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,\n> > > +\t\t\t.reserved = {}\n> > > +\t\t};\n> > > +\n> > > +\t\trouting.push_back(route);\n> > > +\t}\n> > > +\n> > > +\tint ret = crossbar_->setRouting(&routing, V4L2Subdevice::ActiveFormat);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > +\t/* Apply format to the sensor and CSIS receiver. */\n> > > +\tV4L2SubdeviceFormat format = camConfig->sensorFormat_;\n> > > +\tret = data->sensor_->setFormat(&format);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > +\tret = data->csis_->setFormat(0, &format);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > +\tret = crossbar_->setFormat(data->xbarSink_, &format);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > +\t/* Now configure the ISI and video node instances, one per stream. */\n> > > +\tdata->enabledStreams_.clear();\n> > > +\tfor (const auto &config : *c) {\n> > > +\t\tPipe *pipe = pipeFromStream(camera, config.stream());\n> > > +\n> > > +\t\t/*\n> > > +\t\t * Set the format on the ISI sink pad: it must match what is\n> > > +\t\t * received by the CSIS.\n> > > +\t\t */\n> > > +\t\tret = pipe->isi->setFormat(0, &format);\n> > > +\t\tif (ret)\n> > > +\t\t\treturn ret;\n> > > +\n> > > +\t\t/*\n> > > +\t\t * Configure the ISI sink compose rectangle to downscale the\n> > > +\t\t * image.\n> > > +\t\t *\n> > > +\t\t * \\todo Additional cropping could be applied on the ISI source\n> > > +\t\t * pad to further reduce the output image size.\n> > > +\t\t */\n> > > +\t\tRectangle isiScale = {};\n> > > +\t\tisiScale.x = 0;\n> > > +\t\tisiScale.y = 0;\n> > > +\t\tisiScale.width = config.size.width;\n> > > +\t\tisiScale.height = config.size.height;\n> >\n> > \t\tRectangle isiScale{ config.size };\n> >\n> > > +\n> > > +\t\tret = pipe->isi->setSelection(0, V4L2_SEL_TGT_COMPOSE, &isiScale);\n> > > +\t\tif (ret)\n> > > +\t\t\treturn ret;\n> > > +\n> > > +\t\t/*\n> > > +\t\t * Set the format on ISI source pad: only the media bus code\n> > > +\t\t * is relevant as it configures format conversion, while the\n> > > +\t\t * size is taken from the sink's COMPOSE (or source's CROP,\n> > > +\t\t * if any) rectangles.\n> > > +\t\t */\n> > > +\t\tconst ISICameraConfiguration::PipeFormat &pipeFormat =\n> > > +\t\t\tISICameraConfiguration::formatsMap_.at(config.pixelFormat);\n> > > +\n> > > +\t\tV4L2SubdeviceFormat isiFormat{};\n> > > +\t\tisiFormat.mbus_code = pipeFormat.isiCode;\n> > > +\t\tisiFormat.size = config.size;\n> > > +\n> > > +\t\tret = pipe->isi->setFormat(1, &isiFormat);\n> > > +\t\tif (ret)\n> > > +\t\t\treturn ret;\n> > > +\n> > > +\t\tV4L2DeviceFormat captureFmt{};\n> > > +\t\tcaptureFmt.fourcc = pipe->capture->toV4L2PixelFormat(config.pixelFormat);\n> > > +\t\tcaptureFmt.size = config.size;\n> > > +\n> > > +\t\t/* \\todo Set stride and format. */\n> > > +\t\tret = pipe->capture->setFormat(&captureFmt);\n> > > +\t\tif (ret)\n> > > +\t\t\treturn ret;\n> > > +\n> > > +\t\t/* Store the list of enabled streams for later use. */\n> > > +\t\tdata->enabledStreams_.push_back(config.stream());\n> > > +\t}\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +int PipelineHandlerISI::exportFrameBuffers(Camera *camera, Stream *stream,\n> > > +\t\t\t\t\t std::vector> *buffers)\n> > > +{\n> > > +\tunsigned int count = stream->configuration().bufferCount;\n> > > +\tPipe *pipe = pipeFromStream(camera, stream);\n> > > +\n> > > +\treturn pipe->capture->exportBuffers(count, buffers);\n> > > +}\n> > > +\n> > > +int PipelineHandlerISI::start(Camera *camera,\n> > > +\t\t\t [[maybe_unused]] const ControlList *controls)\n> > > +{\n> > > +\tISICameraData *data = cameraData(camera);\n> > > +\n> > > +\tfor (const auto &stream : data->enabledStreams_) {\n> > > +\t\tPipe *pipe = pipeFromStream(camera, stream);\n> > > +\t\tconst StreamConfiguration &config = stream->configuration();\n> > > +\n> > > +\t\tint ret = pipe->capture->importBuffers(config.bufferCount);\n> > > +\t\tif (ret)\n> > > +\t\t\treturn ret;\n> > > +\n> > > +\t\tret = pipe->capture->streamOn();\n> > > +\t\tif (ret)\n> > > +\t\t\treturn ret;\n> > > +\t}\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +void PipelineHandlerISI::stopDevice(Camera *camera)\n> > > +{\n> > > +\tISICameraData *data = cameraData(camera);\n> > > +\n> > > +\tfor (const auto &stream : data->enabledStreams_) {\n> > > +\t\tPipe *pipe = pipeFromStream(camera, stream);\n> > > +\n> > > +\t\tpipe->capture->streamOff();\n> > > +\t\tpipe->capture->releaseBuffers();\n> > > +\t}\n> > > +}\n> > > +\n> > > +int PipelineHandlerISI::queueRequestDevice(Camera *camera, Request *request)\n> > > +{\n> > > +\tfor (auto &[stream, buffer] : request->buffers()) {\n> > > +\t\tPipe *pipe = pipeFromStream(camera, stream);\n> > > +\n> > > +\t\tint ret = pipe->capture->queueBuffer(buffer);\n> > > +\t\tif (ret)\n> > > +\t\t\treturn ret;\n> > > +\t}\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)\n> > > +{\n> > > +\tDeviceMatch dm(\"mxc-isi\");\n> > > +\tdm.add(\"crossbar\");\n> > > +\tdm.add(\"mxc_isi.0\");\n> > > +\tdm.add(\"mxc_isi.0.capture\");\n> > > +\n> > > +\tisiDev_ = acquireMediaDevice(enumerator, dm);\n> > > +\tif (!isiDev_)\n> > > +\t\treturn false;\n> > > +\n> > > +\t/*\n> > > +\t * Acquire the subdevs and video nodes for the crossbar switch and the\n> > > +\t * processing pipelines.\n> > > +\t */\n> > > +\tcrossbar_ = V4L2Subdevice::fromEntityName(isiDev_, \"crossbar\");\n> > > +\tif (!crossbar_)\n> > > +\t\treturn false;\n> > > +\n> > > +\tint ret = crossbar_->open();\n> > > +\tif (ret)\n> > > +\t\treturn false;\n> > > +\n> > > +\tfor (unsigned int i = 0; ; ++i) {\n> > > +\t\tstd::string entityName = \"mxc_isi.\" + std::to_string(i);\n> > > +\t\tstd::unique_ptr isi =\n> > > +\t\t\tV4L2Subdevice::fromEntityName(isiDev_, entityName);\n> > > +\t\tif (!isi)\n> > > +\t\t\tbreak;\n> > > +\n> > > +\t\tret = isi->open();\n> > > +\t\tif (ret)\n> > > +\t\t\treturn false;\n> > > +\n> > > +\t\tentityName += \".capture\";\n> > > +\t\tstd::unique_ptr capture =\n> > > +\t\t\tV4L2VideoDevice::fromEntityName(isiDev_, entityName);\n> > > +\t\tASSERT(capture);\n> >\n> > When I said \"make it a fatal error\" in the review of v1, I meant \"return\n> > false\". Sorry for being unclear. I suppose this works too.\n> \n> Well, it's a bit harsh, but without capture dev we surely can't\n> operate\n> \n> >\n> > > +\n> > > +\t\tcapture->bufferReady.connect(this, &PipelineHandlerISI::bufferReady);\n> > > +\n> > > +\t\tret = capture->open();\n> > > +\t\tif (ret)\n> > > +\t\t\treturn ret;\n> > > +\n> > > +\t\tpipes_.push_back({ std::move(isi), std::move(capture) });\n> > > +\t}\n> > > +\n> > > +\tif (pipes_.empty()) {\n> > > +\t\tLOG(ISI, Error) << \"Unable to enumerate pipes\";\n> > > +\t\treturn false;\n> > > +\t}\n> > > +\n> > > +\t/*\n> > > +\t * Loop over all the crossbar switch sink pads to find connected CSI-2\n> > > +\t * receivers and camera sensors.\n> > > +\t */\n> > > +\tunsigned int numCameras = 0;\n> > > +\tunsigned int numSinks = 0;\n> > > +\tfor (MediaPad *pad : crossbar_->entity()->pads()) {\n> > > +\t\tunsigned int sink = numSinks;\n> > > +\n> > > +\t\tif (!(pad->flags() & MEDIA_PAD_FL_SINK) || pad->links().empty())\n> > > +\t\t\tcontinue;\n> > > +\n> > > +\t\t/*\n> > > +\t\t * Count each crossbar sink pad to correctly configure\n> > > +\t\t * routing and format for this camera.\n> > > +\t\t */\n> > > +\t\tnumSinks++;\n> > > +\n> > > +\t\tMediaEntity *csi = pad->links()[0]->source()->entity();\n> > > +\t\tif (csi->pads().size() != 2) {\n> > > +\t\t\tLOG(ISI, Debug) << \"Skip unsupported CSI-2 receiver \"\n> > > +\t\t\t\t\t<< csi->name();\n> > > +\t\t\tcontinue;\n> > > +\t\t}\n> > > +\n> > > +\t\tpad = csi->pads()[0];\n> > > +\t\tif (!(pad->flags() & MEDIA_PAD_FL_SINK) || pad->links().empty())\n> > > +\t\t\tcontinue;\n> > > +\n> > > +\t\tMediaEntity *sensor = pad->links()[0]->source()->entity();\n> > > +\t\tif (sensor->function() != MEDIA_ENT_F_CAM_SENSOR) {\n> > > +\t\t\tLOG(ISI, Debug) << \"Skip unsupported subdevice \"\n> > > +\t\t\t\t\t<< sensor->name();\n> > > +\t\t\tcontinue;\n> > > +\t\t}\n> > > +\n> > > +\t\t/* Create the camera data. */\n> > > +\t\tstd::unique_ptr data =\n> > > +\t\t\tstd::make_unique(this);\n> > > +\n> > > +\t\tdata->sensor_ = std::make_unique(sensor);\n> > > +\t\tdata->csis_ = std::make_unique(csi);\n> > > +\t\tdata->xbarSink_ = sink;\n> > > +\n> > > +\t\tret = data->init();\n> > > +\t\tif (ret) {\n> > > +\t\t\tLOG(ISI, Error) << \"Failed to initialize camera data\";\n> > > +\t\t\treturn false;\n> > > +\t\t}\n> > > +\n> > > +\t\t/* Register the camera. */\n> > > +\t\tconst std::string &id = data->sensor_->id();\n> > > +\t\tstd::set streams;\n> > > +\t\tstd::transform(data->streams_.begin(), data->streams_.end(),\n> > > +\t\t\t std::inserter(streams, streams.end()),\n> > > +\t\t\t [](Stream &s) { return &s; });\n> > > +\n> > > +\t\tstd::shared_ptr camera =\n> > > +\t\t\tCamera::create(std::move(data), id, streams);\n> > > +\n> > > +\t\tregisterCamera(std::move(camera));\n> > > +\t\tnumCameras++;\n> > > +\t}\n> > > +\n> > > +\treturn numCameras > 0;\n> > > +}\n> > > +\n> > > +PipelineHandlerISI::Pipe *PipelineHandlerISI::pipeFromStream(Camera *camera,\n> > > +\t\t\t\t\t\t\t const Stream *stream)\n> > > +{\n> > > +\tISICameraData *data = cameraData(camera);\n> > > +\tunsigned int pipeIndex = data->pipeIndex(stream);\n> > > +\n> > > +\tASSERT(pipeIndex < pipes_.size());\n> > > +\n> > > +\treturn &pipes_[pipeIndex];\n> > > +}\n> > > +\n> > > +void PipelineHandlerISI::bufferReady(FrameBuffer *buffer)\n> > > +{\n> > > +\tRequest *request = buffer->request();\n> > > +\n> > > +\tcompleteBuffer(request, buffer);\n> > > +\tif (request->hasPendingBuffers())\n> > > +\t\treturn;\n> > > +\n> > > +\tcompleteRequest(request);\n> > > +}\n> > > +\n> > > +REGISTER_PIPELINE_HANDLER(PipelineHandlerISI)\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/pipeline/imx8-isi/meson.build b/src/libcamera/pipeline/imx8-isi/meson.build\n> > > new file mode 100644\n> > > index 000000000000..ffd0ce54ce92\n> > > --- /dev/null\n> > > +++ b/src/libcamera/pipeline/imx8-isi/meson.build\n> > > @@ -0,0 +1,5 @@\n> > > +# SPDX-License-Identifier: CC0-1.0\n> > > +\n> > > +libcamera_sources += files([\n> > > + 'imx8-isi.cpp'\n> > > +])","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 9D7F0BD16B\n\tfor ;\n\tWed, 16 Nov 2022 08:12:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1930663092;\n\tWed, 16 Nov 2022 09:12:43 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4736761F36\n\tfor ;\n\tWed, 16 Nov 2022 09:12:42 +0100 (CET)","from pendragon.ideasonboard.com\n\t(host-62-245-140-144.customer.m-online.net [62.245.140.144])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id ADB43891;\n\tWed, 16 Nov 2022 09:12:41 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1668586363;\n\tbh=RnHfvmMrXGhrTGkP/vd1pS5TabvjtWyodEXnxt64yvU=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=OIlD/pSKZyi2fhBqRaS60qmlnzJQRCqHbPe/tXr/mHeOqb+N/+6SbwVw9vMAp6n2F\n\taK7z+I5eSyKvPjN8/ferK6Vn/6Dn5dDcCxSSYkztjYSXhy3uizeNUIUnpXB0MpHlG3\n\t8j3dbLfLQ/H+2R7LL0SeOxbtmJT2wQ+nZ9y4B8UWv93d7EPI9SrBqsgf34Mrq4gwVa\n\tb39rBDYSr1FLv9Aq28g6+yBcRFW26xFfvuE8g6gj2SROd+EjbQADRCn4BmnPVbCmVl\n\tkzrlvPtXHTl3WaxXq3z2gh0L3KrPm9NTkaKOD0bsqelwtbcGregnbFdrF6O/vKCGPE\n\tdqJRi47FQPICQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1668586361;\n\tbh=RnHfvmMrXGhrTGkP/vd1pS5TabvjtWyodEXnxt64yvU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nOVzu1dh4VlX0Ktw0N5cRqWXY8twhv6BK7AoFyDFOfAIO1ePG+zklpEnHDplDSFmX\n\tJq4vPaTlfKQgKxPmLdy3UsVgL1ZfDDdohUCOX1U/Ds8UKosk2kzK9Rch5khTQIydx+\n\tNcdXI4HqSQytMG8qojHHScGeRD7fpPZ1Bab7AOP8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"nOVzu1dh\"; dkim-atps=neutral","Date":"Wed, 16 Nov 2022 10:12:26 +0200","To":"Jacopo Mondi ","Message-ID":"","References":"<20221110191433.5535-1-jacopo@jmondi.org>\n\t<20221110191433.5535-2-jacopo@jmondi.org>\n\t\n\t<20221114203533.wmxud5vnlqiusjpf@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20221114203533.wmxud5vnlqiusjpf@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: pipeline: Add IMX8 ISI\n\tpipeline","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25808,"web_url":"https://patchwork.libcamera.org/comment/25808/","msgid":"<20221117091457.wf326rlmdwcu3tui@uno.localdomain>","date":"2022-11-17T09:16:42","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: pipeline: Add IMX8 ISI\n\tpipeline","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Wed, Nov 16, 2022 at 10:12:26AM +0200, Laurent Pinchart wrote:\n\n[snip]\n\n> > > > +\t/* Adjust all other streams to RAW. */\n> > > > +\tunsigned int i = 0;\n> > > > +\tfor (StreamConfiguration &cfg : config_) {\n> > >\n> > > \tfor (auto &[i, cfg] : utils::enumerate(config_)) {\n> > >\n> >\n> > Nope, I started with that too\n> >\n> > ../src/libcamera/pipeline/imx8-isi/imx8-isi.cpp: In member function ‘libcamera::CameraConfiguration::Status libcamera::ISICameraConfiguration::validateRaw(std::set&, const libcamera::Size&)’:\n> > ../src/libcamera/pipeline/imx8-isi/imx8-isi.cpp:331:55: error: cannot bind non-const lvalue reference of type ‘std::pair&’ to an rvalue of type ‘libcamera::utils::details::enumerate_iterator<__gnu_cxx::__normal_iterator > >::value_type’ {aka ‘std::pair’}\n> > 331 | for (auto &[i, cfg] : utils::enumerate(config_)) {\n> >\n> > Cannot bind non-const lvalue reference of type ‘std::pair&’\n> > to an rvalue of type std::pair\n> >\n> > I can fix it by removing the reference, but then the\n> > StreamCOnfiguration would be copied.\n> >\n> > I've not investigated why\n>\n> \tfor (const auto &[i, cfg] : utils::enumerate(config_)) {\n>\n> Simple, right ? :-)\n>\n> It's a tricky one, utils::enumerate() returns a class instance\n> (enumerate_adapter) that has begin() and end() functions. Those\n> functions return an enumerate_iterator instance, whose value_type is a\n> std::pair. The C++ structured binding\n> declaration creates a hidden variable e that holds the value of the\n> initializer, so without const we would essentially do\n>\n> \tenumerate_iterator it = ...;\n> \tauto &e = *it;\n>\n> enumerate_iterator::operator*() returns an rvalue, and you can't bind a\n> non-const lvalue reference to an rvalue. The const qualifier fixes it.\n>\n> Then, the i and cfg names are bound to the first and second members of\n> the hidden variable e. As e is a std::pair,\n> i takes the type const std::size, and cfg takes the type base_reference\n> (which in this case is a StreamConfig &). The const qualifier in the\n> expression\n>\n> \tfor (const auto &[i, cfg] : utils::enumerate(config_)) {\n>\n> doesn't apply to i and cfg, only to the hidden variable e.\n>\n\nAh, I didn't use the const version exactly because I wanted to modify\ncfg.. Thanks for the explanation, if not other comments on v3, I can\nchange this when pushing.\n\nThanks\n j","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 39D46BD16B\n\tfor ;\n\tThu, 17 Nov 2022 09:16:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6450B6309B;\n\tThu, 17 Nov 2022 10:16:47 +0100 (CET)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6819861F34\n\tfor ;\n\tThu, 17 Nov 2022 10:16:45 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id A7FFA240012;\n\tThu, 17 Nov 2022 09:16:44 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1668676607;\n\tbh=/ta/eljvduybM3bHhLlMnOXMLqpMKZRPlnfyiI9mkpY=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=DZrr7/RjVkUwP6eDf43n93DeGgDyophZo3xGlwSfDFY1aEqt1wSYfbOMMGpG+zckX\n\tqKkm6aGfkiGe/ZxQWtP4n+c7VZYFGvRS0NV9v2sDD/eowmFhi4v3sDrdaGBBcx7lbY\n\ty5VvzKCYHPmlVfSEsBxV/N8d54wzlEmpyj4/qc3k5LDHbB4evdYs6JkJQJM4c6bXmW\n\tgFfhLOk+oBb/4h89Fq4JtG0I4ejFpn+dFcaJgq9jpvPRNXq3dD6BIl+YIibnrX9WBQ\n\tBgIzIcaF4vfq2NZON31oQ5WR0wHX68WBtvg5nj04wy+JHukAktwbqLs6/vgekkX8WH\n\tS0HS2SN+9yhzA==","Date":"Thu, 17 Nov 2022 10:16:42 +0100","To":"Laurent Pinchart ","Message-ID":"<20221117091457.wf326rlmdwcu3tui@uno.localdomain>","References":"<20221110191433.5535-1-jacopo@jmondi.org>\n\t<20221110191433.5535-2-jacopo@jmondi.org>\n\t\n\t<20221114203533.wmxud5vnlqiusjpf@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: pipeline: Add IMX8 ISI\n\tpipeline","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]