[{"id":25538,"web_url":"https://patchwork.libcamera.org/comment/25538/","msgid":"<20221024094620.GH3874866@pyrite.rasen.tech>","date":"2022-10-24T09:46:20","subject":"Re: [libcamera-devel] [PATCH v3 13/13] pipeline: rkisp1: Support\n\traw Bayer capture configuration","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"On Mon, Oct 24, 2022 at 03:03:56AM +0300, Laurent Pinchart wrote:\n> From: Florian Sylvestre \n> \n> Implement support for raw Bayer capture during configuration generation,\n> validation and camera configuration.\n> \n> While at it, fix a typo in a comment.\n> \n> Signed-off-by: Florian Sylvestre \n> Signed-off-by: Paul Elder \n> Signed-off-by: Laurent Pinchart \n\nReviewed-by: Paul Elder \n\n> ---\n> Changes since v2:\n> \n> - Nearly complete rewrite\n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 116 +++++++---\n> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 209 +++++++++++++++---\n> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 3 +-\n> 3 files changed, 273 insertions(+), 55 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index e57411544f7a..891fd2d50270 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -403,6 +403,30 @@ void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &meta\n> \tpipe()->tryCompleteRequest(info);\n> }\n> \n> +/* -----------------------------------------------------------------------------\n> + * Camera Configuration\n> + */\n> +\n> +namespace {\n> +\n> +/* Keep in sync with the supported raw formats in rkisp1_path.cpp. */\n> +const std::map rawFormats = {\n> +\t{ formats::SBGGR8, MEDIA_BUS_FMT_SBGGR8_1X8 },\n> +\t{ formats::SGBRG8, MEDIA_BUS_FMT_SGBRG8_1X8 },\n> +\t{ formats::SGRBG8, MEDIA_BUS_FMT_SGRBG8_1X8 },\n> +\t{ formats::SRGGB8, MEDIA_BUS_FMT_SRGGB8_1X8 },\n> +\t{ formats::SBGGR10, MEDIA_BUS_FMT_SBGGR10_1X10 },\n> +\t{ formats::SGBRG10, MEDIA_BUS_FMT_SGBRG10_1X10 },\n> +\t{ formats::SGRBG10, MEDIA_BUS_FMT_SGRBG10_1X10 },\n> +\t{ formats::SRGGB10, MEDIA_BUS_FMT_SRGGB10_1X10 },\n> +\t{ formats::SBGGR12, MEDIA_BUS_FMT_SBGGR12_1X12 },\n> +\t{ formats::SGBRG12, MEDIA_BUS_FMT_SGBRG12_1X12 },\n> +\t{ formats::SGRBG12, MEDIA_BUS_FMT_SGRBG12_1X12 },\n> +\t{ formats::SRGGB12, MEDIA_BUS_FMT_SRGGB12_1X12 },\n> +};\n> +\n> +} /* namespace */\n> +\n> RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n> \t\t\t\t\t\t RkISP1CameraData *data)\n> \t: CameraConfiguration()\n> @@ -449,6 +473,21 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \t\tstatus = Adjusted;\n> \t}\n> \n> +\t/*\n> +\t * Simultaneous capture of raw and processed streams isn't possible. If\n> +\t * there is any raw stream, cap the number of streams to one.\n> +\t */\n> +\tif (config_.size() > 1) {\n> +\t\tfor (const auto &cfg : config_) {\n> +\t\t\tif (PixelFormatInfo::info(cfg.pixelFormat).colourEncoding ==\n> +\t\t\t PixelFormatInfo::ColourEncodingRAW) {\n> +\t\t\t\tconfig_.resize(1);\n> +\t\t\t\tstatus = Adjusted;\n> +\t\t\t}\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> \t/*\n> \t * If there are more than one stream in the configuration figure out the\n> \t * order to evaluate the streams. The first stream has the highest\n> @@ -510,45 +549,51 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \t\t\t}\n> \t\t}\n> \n> -\t\t/* All paths rejected configuraiton. */\n> +\t\t/* All paths rejected configuration. */\n> \t\tLOG(RkISP1, Debug) << \"Camera configuration not supported \"\n> \t\t\t\t << cfg.toString();\n> \t\treturn Invalid;\n> \t}\n> \n> \t/* Select the sensor format. */\n> +\tPixelFormat rawFormat;\n> \tSize maxSize;\n> -\tfor (const StreamConfiguration &cfg : config_)\n> +\n> +\tfor (const StreamConfiguration &cfg : config_) {\n> +\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);\n> +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> +\t\t\trawFormat = cfg.pixelFormat;\n> +\n> \t\tmaxSize = std::max(maxSize, cfg.size);\n> +\t}\n> +\n> +\tstd::vector mbusCodes;\n> +\n> +\tif (rawFormat.isValid()) {\n> +\t\tmbusCodes = { rawFormats.at(rawFormat) };\n> +\t} else {\n> +\t\tstd::transform(rawFormats.begin(), rawFormats.end(),\n> +\t\t\t std::back_inserter(mbusCodes),\n> +\t\t\t [](const auto &value) { return value.second; });\n> +\t}\n> +\n> +\tsensorFormat_ = sensor->getFormat(mbusCodes, maxSize);\n> \n> -\tsensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,\n> -\t\t\t\t\t MEDIA_BUS_FMT_SGBRG12_1X12,\n> -\t\t\t\t\t MEDIA_BUS_FMT_SGRBG12_1X12,\n> -\t\t\t\t\t MEDIA_BUS_FMT_SRGGB12_1X12,\n> -\t\t\t\t\t MEDIA_BUS_FMT_SBGGR10_1X10,\n> -\t\t\t\t\t MEDIA_BUS_FMT_SGBRG10_1X10,\n> -\t\t\t\t\t MEDIA_BUS_FMT_SGRBG10_1X10,\n> -\t\t\t\t\t MEDIA_BUS_FMT_SRGGB10_1X10,\n> -\t\t\t\t\t MEDIA_BUS_FMT_SBGGR8_1X8,\n> -\t\t\t\t\t MEDIA_BUS_FMT_SGBRG8_1X8,\n> -\t\t\t\t\t MEDIA_BUS_FMT_SGRBG8_1X8,\n> -\t\t\t\t\t MEDIA_BUS_FMT_SRGGB8_1X8 },\n> -\t\t\t\t\t maxSize);\n> \tif (sensorFormat_.size.isNull())\n> \t\tsensorFormat_.size = sensor->resolution();\n> \n> \treturn status;\n> }\n> \n> +/* -----------------------------------------------------------------------------\n> + * Pipeline Operations\n> + */\n> +\n> PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)\n> \t: PipelineHandler(manager), hasSelfPath_(true)\n> {\n> }\n> \n> -/* -----------------------------------------------------------------------------\n> - * Pipeline Operations\n> - */\n> -\n> std::unique_ptr\n> PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> \tconst StreamRoles &roles)\n> @@ -604,23 +649,38 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> \t\t\t\tcolorSpace = ColorSpace::Rec709;\n> \t\t\tbreak;\n> \n> +\t\tcase StreamRole::Raw:\n> +\t\t\tif (roles.size() > 1) {\n> +\t\t\t\tLOG(RkISP1, Error)\n> +\t\t\t\t\t<< \"Can't capture both raw and processed streams\";\n> +\t\t\t\treturn nullptr;\n> +\t\t\t}\n> +\n> +\t\t\tuseMainPath = true;\n> +\t\t\tcolorSpace = ColorSpace::Raw;\n> +\t\t\tbreak;\n> +\n> \t\tdefault:\n> \t\t\tLOG(RkISP1, Warning)\n> \t\t\t\t<< \"Requested stream role not supported: \" << role;\n> \t\t\treturn nullptr;\n> \t\t}\n> \n> -\t\tStreamConfiguration cfg;\n> +\t\tRkISP1Path *path;\n> +\n> \t\tif (useMainPath) {\n> -\t\t\tcfg = data->mainPath_->generateConfiguration(\n> -\t\t\t\tdata->sensor_.get());\n> +\t\t\tpath = data->mainPath_;\n> \t\t\tmainPathAvailable = false;\n> \t\t} else {\n> -\t\t\tcfg = data->selfPath_->generateConfiguration(\n> -\t\t\t\tdata->sensor_.get());\n> +\t\t\tpath = data->selfPath_;\n> \t\t\tselfPathAvailable = false;\n> \t\t}\n> \n> +\t\tStreamConfiguration cfg =\n> +\t\t\tpath->generateConfiguration(data->sensor_.get(), role);\n> +\t\tif (!cfg.pixelFormat.isValid())\n> +\t\t\treturn nullptr;\n> +\n> \t\tcfg.colorSpace = colorSpace;\n> \t\tconfig->addConfiguration(cfg);\n> \t}\n> @@ -674,10 +734,14 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \t\t<< \"ISP input pad configured with \" << format\n> \t\t<< \" crop \" << rect;\n> \n> -\tisRaw_ = false;\n> +\tconst PixelFormat &streamFormat = config->at(0).pixelFormat;\n> +\tconst PixelFormatInfo &info = PixelFormatInfo::info(streamFormat);\n> +\tisRaw_ = info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> \n> \t/* YUYV8_2X8 is required on the ISP source path pad for YUV output. */\n> -\tformat.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8;\n> +\tif (!isRaw_)\n> +\t\tformat.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8;\n> +\n> \tLOG(RkISP1, Debug)\n> \t\t<< \"Configuring ISP output pad with \" << format\n> \t\t<< \" crop \" << rect;\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> index cc2ac66e6939..2994bd665ebb 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> @@ -21,6 +21,39 @@ namespace libcamera {\n> \n> LOG_DECLARE_CATEGORY(RkISP1)\n> \n> +namespace {\n> +\n> +/* Keep in sync with the supported raw formats in rkisp1.cpp. */\n> +const std::map formatToMediaBus = {\n> +\t{ formats::UYVY, MEDIA_BUS_FMT_YUYV8_2X8 },\n> +\t{ formats::YUYV, MEDIA_BUS_FMT_YUYV8_2X8 },\n> +\t{ formats::NV12, MEDIA_BUS_FMT_YUYV8_1_5X8 },\n> +\t{ formats::NV21, MEDIA_BUS_FMT_YUYV8_1_5X8 },\n> +\t{ formats::NV16, MEDIA_BUS_FMT_YUYV8_2X8 },\n> +\t{ formats::NV61, MEDIA_BUS_FMT_YUYV8_2X8 },\n> +\t{ formats::YUV420, MEDIA_BUS_FMT_YUYV8_1_5X8 },\n> +\t{ formats::YVU420, MEDIA_BUS_FMT_YUYV8_1_5X8 },\n> +\t{ formats::YUV422, MEDIA_BUS_FMT_YUYV8_2X8 },\n> +\t{ formats::YVU422, MEDIA_BUS_FMT_YUYV8_2X8 },\n> +\t{ formats::R8, MEDIA_BUS_FMT_YUYV8_2X8 },\n> +\t{ formats::RGB565, MEDIA_BUS_FMT_YUYV8_2X8 },\n> +\t{ formats::XRGB8888, MEDIA_BUS_FMT_YUYV8_2X8 },\n> +\t{ formats::SBGGR8, MEDIA_BUS_FMT_SBGGR8_1X8 },\n> +\t{ formats::SGBRG8, MEDIA_BUS_FMT_SGBRG8_1X8 },\n> +\t{ formats::SGRBG8, MEDIA_BUS_FMT_SGRBG8_1X8 },\n> +\t{ formats::SRGGB8, MEDIA_BUS_FMT_SRGGB8_1X8 },\n> +\t{ formats::SBGGR10, MEDIA_BUS_FMT_SBGGR10_1X10 },\n> +\t{ formats::SGBRG10, MEDIA_BUS_FMT_SGBRG10_1X10 },\n> +\t{ formats::SGRBG10, MEDIA_BUS_FMT_SGRBG10_1X10 },\n> +\t{ formats::SRGGB10, MEDIA_BUS_FMT_SRGGB10_1X10 },\n> +\t{ formats::SBGGR12, MEDIA_BUS_FMT_SBGGR12_1X12 },\n> +\t{ formats::SGBRG12, MEDIA_BUS_FMT_SGBRG12_1X12 },\n> +\t{ formats::SGRBG12, MEDIA_BUS_FMT_SGRBG12_1X12 },\n> +\t{ formats::SRGGB12, MEDIA_BUS_FMT_SRGGB12_1X12 },\n> +};\n> +\n> +} /* namespace */\n> +\n> RkISP1Path::RkISP1Path(const char *name, const Span &formats,\n> \t\t const Size &minResolution, const Size &maxResolution)\n> \t: name_(name), running_(false), formats_(formats),\n> @@ -69,11 +102,18 @@ void RkISP1Path::populateFormats()\n> \tstd::vector formats;\n> \tfor (const auto &[format, sizes] : v4l2Formats) {\n> \t\tconst PixelFormat pixelFormat = format.toPixelFormat();\n> -\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat);\n> \n> -\t\t/* \\todo Add support for RAW formats. */\n> -\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> +\t\t/*\n> +\t\t * As a defensive measure, skip any pixel format exposed by the\n> +\t\t * driver that we don't know about. This ensures that looking up\n> +\t\t * formats in formatToMediaBus using a key from streamFormats_\n> +\t\t * will never fail in any of the other functions.\n> +\t\t */\n> +\t\tif (!formatToMediaBus.count(pixelFormat)) {\n> +\t\t\tLOG(RkISP1, Warning)\n> +\t\t\t\t<< \"Unsupported pixel format \" << pixelFormat;\n> \t\t\tcontinue;\n> +\t\t}\n> \n> \t\tstreamFormats_.insert(pixelFormat);\n> \n> @@ -86,21 +126,68 @@ void RkISP1Path::populateFormats()\n> \t}\n> }\n> \n> -StreamConfiguration RkISP1Path::generateConfiguration(const CameraSensor *sensor)\n> +StreamConfiguration\n> +RkISP1Path::generateConfiguration(const CameraSensor *sensor, StreamRole role)\n> {\n> +\tconst std::vector &mbusCodes = sensor->mbusCodes();\n> \tconst Size &resolution = sensor->resolution();\n> \n> \tSize maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n> \t\t\t\t\t .boundedTo(resolution);\n> \tSize minResolution = minResolution_.expandedToAspectRatio(resolution);\n> \n> +\t/* Create the list of supported stream formats. */\n> \tstd::map> streamFormats;\n> -\tfor (const auto &format : streamFormats_)\n> -\t\tstreamFormats[format] = { { minResolution, maxResolution } };\n> +\tunsigned int rawBitsPerPixel = 0;\n> +\tPixelFormat rawFormat;\n> +\n> +\tfor (const auto &format : streamFormats_) {\n> +\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(format);\n> +\n> +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\n> +\t\t\t/* Skip raw formats not supported by the sensor. */\n> +\t\t\tuint32_t mbusCode = formatToMediaBus.at(format);\n> +\t\t\tif (std::find(mbusCodes.begin(), mbusCodes.end(), mbusCode) ==\n> +\t\t\t mbusCodes.end())\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tstreamFormats[format] = { { resolution, resolution } };\n> +\n> +\t\t\t/*\n> +\t\t\t * Store the raw format with the higher bits per pixel\n> +\t\t\t * for later usage.\n> +\t\t\t */\n> +\t\t\tif (info.bitsPerPixel > rawBitsPerPixel) {\n> +\t\t\t\trawBitsPerPixel = info.bitsPerPixel;\n> +\t\t\t\trawFormat = format;\n> +\t\t\t}\n> +\t\t} else {\n> +\t\t\tstreamFormats[format] = { { minResolution, maxResolution } };\n> +\t\t}\n> +\t}\n> +\n> +\t/*\n> +\t * Pick a suitable pixel format for the role. Raw streams need to use a\n> +\t * raw format, processed streams use NV12 by default.\n> +\t */\n> +\tPixelFormat format;\n> +\n> +\tif (role == StreamRole::Raw) {\n> +\t\tif (!rawFormat.isValid()) {\n> +\t\t\tLOG(RkISP1, Error)\n> +\t\t\t\t<< \"Sensor \" << sensor->model()\n> +\t\t\t\t<< \" doesn't support raw capture\";\n> +\t\t\treturn {};\n> +\t\t}\n> +\n> +\t\tformat = rawFormat;\n> +\t} else {\n> +\t\tformat = formats::NV12;\n> +\t}\n> \n> \tStreamFormats formats(streamFormats);\n> \tStreamConfiguration cfg(formats);\n> -\tcfg.pixelFormat = formats::NV12;\n> +\tcfg.pixelFormat = format;\n> \tcfg.size = maxResolution;\n> \tcfg.bufferCount = RKISP1_BUFFER_COUNT;\n> \n> @@ -110,26 +197,85 @@ StreamConfiguration RkISP1Path::generateConfiguration(const CameraSensor *sensor\n> CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> \t\t\t\t\t\t StreamConfiguration *cfg)\n> {\n> +\tconst std::vector &mbusCodes = sensor->mbusCodes();\n> \tconst Size &resolution = sensor->resolution();\n> \n> \tconst StreamConfiguration reqCfg = *cfg;\n> \tCameraConfiguration::Status status = CameraConfiguration::Valid;\n> \n> \t/*\n> -\t * Default to NV12 if the requested format is not supported. All\n> -\t * versions of the ISP are guaranteed to support NV12 on both the main\n> -\t * and self paths.\n> +\t * Validate the pixel format. If the requested format isn't supported,\n> +\t * default to either NV12 (all versions of the ISP are guaranteed to\n> +\t * support NV12 on both the main and self paths) if the requested format\n> +\t * is not a raw format, or to the supported raw format with the highest\n> +\t * bits per pixel otherwise.\n> \t */\n> -\tif (!streamFormats_.count(cfg->pixelFormat))\n> -\t\tcfg->pixelFormat = formats::NV12;\n> +\tunsigned int rawBitsPerPixel = 0;\n> +\tPixelFormat rawFormat;\n> +\tbool found = false;\n> +\n> +\tfor (const auto &format : streamFormats_) {\n> +\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(format);\n> +\n> +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\n> +\t\t\t/* Skip raw formats not supported by the sensor. */\n> +\t\t\tuint32_t mbusCode = formatToMediaBus.at(format);\n> +\t\t\tif (std::find(mbusCodes.begin(), mbusCodes.end(), mbusCode) ==\n> +\t\t\t mbusCodes.end())\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\t/*\n> +\t\t\t * Store the raw format with the higher bits per pixel\n> +\t\t\t * for later usage.\n> +\t\t\t */\n> +\t\t\tif (info.bitsPerPixel > rawBitsPerPixel) {\n> +\t\t\t\trawBitsPerPixel = info.bitsPerPixel;\n> +\t\t\t\trawFormat = format;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tif (cfg->pixelFormat == format) {\n> +\t\t\tfound = true;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tbool isRaw = PixelFormatInfo::info(cfg->pixelFormat).colourEncoding ==\n> +\t\t PixelFormatInfo::ColourEncodingRAW;\n> \n> \t/*\n> -\t * Adjust the size based on the sensor resolution and absolute limits\n> -\t * of the ISP.\n> +\t * If no raw format supported by the sensor has been found, use a\n> +\t * processed format.\n> \t */\n> -\tSize maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n> -\t\t\t\t\t .boundedTo(resolution);\n> -\tSize minResolution = minResolution_.expandedToAspectRatio(resolution);\n> +\tif (!rawFormat.isValid())\n> +\t\tisRaw = false;\n> +\n> +\tif (!found)\n> +\t\tcfg->pixelFormat = isRaw ? rawFormat : formats::NV12;\n> +\n> +\tSize minResolution;\n> +\tSize maxResolution;\n> +\n> +\tif (isRaw) {\n> +\t\t/*\n> +\t\t * Use the sensor output size closest to the requested stream\n> +\t\t * size.\n> +\t\t */\n> +\t\tuint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);\n> +\t\tV4L2SubdeviceFormat sensorFormat =\n> +\t\t\tsensor->getFormat({ mbusCode }, cfg->size);\n> +\n> +\t\tminResolution = sensorFormat.size;\n> +\t\tmaxResolution = sensorFormat.size;\n> +\t} else {\n> +\t\t/*\n> +\t\t * Adjust the size based on the sensor resolution and absolute\n> +\t\t * limits of the ISP.\n> +\t\t */\n> +\t\tminResolution = minResolution_.expandedToAspectRatio(resolution);\n> +\t\tmaxResolution = maxResolution_.boundedToAspectRatio(resolution)\n> +\t\t\t\t\t .boundedTo(resolution);\n> +\t}\n> \n> \tcfg->size.boundTo(maxResolution);\n> \tcfg->size.expandTo(minResolution);\n> @@ -182,15 +328,11 @@ int RkISP1Path::configure(const StreamConfiguration &config,\n> \t\t<< \"Configuring \" << name_ << \" resizer output pad with \"\n> \t\t<< ispFormat;\n> \n> -\tswitch (config.pixelFormat) {\n> -\tcase formats::NV12:\n> -\tcase formats::NV21:\n> -\t\tispFormat.mbus_code = MEDIA_BUS_FMT_YUYV8_1_5X8;\n> -\t\tbreak;\n> -\tdefault:\n> -\t\tispFormat.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8;\n> -\t\tbreak;\n> -\t}\n> +\t/*\n> +\t * The configuration has been validated, the pixel format is guaranteed\n> +\t * to be supported and thus found in formatToMediaBus.\n> +\t */\n> +\tispFormat.mbus_code = formatToMediaBus.at(config.pixelFormat);\n> \n> \tret = resizer_->setFormat(1, &ispFormat);\n> \tif (ret < 0)\n> @@ -266,14 +408,25 @@ void RkISP1Path::stop()\n> namespace {\n> constexpr Size RKISP1_RSZ_MP_SRC_MIN{ 32, 16 };\n> constexpr Size RKISP1_RSZ_MP_SRC_MAX{ 4416, 3312 };\n> -constexpr std::array RKISP1_RSZ_MP_FORMATS{\n> +constexpr std::array RKISP1_RSZ_MP_FORMATS{\n> \tformats::YUYV,\n> \tformats::NV16,\n> \tformats::NV61,\n> \tformats::NV21,\n> \tformats::NV12,\n> \tformats::R8,\n> -\t/* \\todo Add support for RAW formats. */\n> +\tformats::SBGGR8,\n> +\tformats::SGBRG8,\n> +\tformats::SGRBG8,\n> +\tformats::SRGGB8,\n> +\tformats::SBGGR10,\n> +\tformats::SGBRG10,\n> +\tformats::SGRBG10,\n> +\tformats::SRGGB10,\n> +\tformats::SBGGR12,\n> +\tformats::SGBRG12,\n> +\tformats::SGRBG12,\n> +\tformats::SRGGB12,\n> };\n> \n> constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 };\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> index bf4ad18fbbf2..bdf3f95b95e1 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> @@ -40,7 +40,8 @@ public:\n> \tint setEnabled(bool enable) { return link_->setEnabled(enable); }\n> \tbool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }\n> \n> -\tStreamConfiguration generateConfiguration(const CameraSensor *sensor);\n> +\tStreamConfiguration generateConfiguration(const CameraSensor *sensor,\n> +\t\t\t\t\t\t StreamRole role);\n> \tCameraConfiguration::Status validate(const CameraSensor *sensor,\n> \t\t\t\t\t StreamConfiguration *cfg);\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart\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 684B8BDB16\n\tfor ;\n\tMon, 24 Oct 2022 09:46:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D84E462F04;\n\tMon, 24 Oct 2022 11:46:29 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8C19661F4D\n\tfor ;\n\tMon, 24 Oct 2022 11:46:28 +0200 (CEST)","from pyrite.rasen.tech (h175-177-042-159.catv02.itscom.jp\n\t[175.177.42.159])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 75D9F8BF;\n\tMon, 24 Oct 2022 11:46:26 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666604789;\n\tbh=8LLWyibJ2/jj0C/P3K6R3csxH8lXhR061Z/GJ2vmbis=;\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=wDnrq2Xw4bQfXPBgtncxZ2VZcayksLRXPn0B5WJQIFtRv0nl1P5yJ6zaIGNIQ3EEu\n\thJPET3u/a+XceX7jawA/Jy+5HZIcehgEcnjzk9YhULcDcXQ7cxP3x6vTltBNORQ40/\n\tnmMrvJ53YAq380xXPVWmuBIqbDwJygmg2tT0wVJxx/IaD3IKsE8YoyuQxU7qZ6Qsfg\n\tmppDzpmFEtJmIceThrLiHmxajXgHzGLUFKon5s1hghKPMgSY6x3sO0xnLmT04cfuQ3\n\trn3FAMJFKnmF93QtfjYmDEMUz/lqJgeGtmh7njj7tSJfTY26feJpp47xuWiiAkLSUu\n\taJRvl8RvTjDgQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1666604787;\n\tbh=8LLWyibJ2/jj0C/P3K6R3csxH8lXhR061Z/GJ2vmbis=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QeQpPbdUI6BhscHCio/3DxzqZYfdgQbIwgMPf2JJjojCdf59+5V1S2uWlAnSGa9IZ\n\tyLeqvzGWWu7w8aMXBM7PbepHU4Bx9vEFeL/MElPQrqIaqLm6NJ/i/U8ZB/V65Q2QG9\n\tmx8gj1MRwt6+zcwNQK/SPvZGO+qz2pNNP59M/+6U="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"QeQpPbdU\"; dkim-atps=neutral","Date":"Mon, 24 Oct 2022 18:46:20 +0900","To":"Laurent Pinchart ","Message-ID":"<20221024094620.GH3874866@pyrite.rasen.tech>","References":"<20221024000356.29521-1-laurent.pinchart@ideasonboard.com>\n\t<20221024000356.29521-14-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20221024000356.29521-14-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 13/13] pipeline: rkisp1: Support\n\traw Bayer capture configuration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Paul Elder via libcamera-devel ","Reply-To":"paul.elder@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25613,"web_url":"https://patchwork.libcamera.org/comment/25613/","msgid":"<20221026173144.lljhou3zuitunyc4@uno.localdomain>","date":"2022-10-26T17:31:44","subject":"Re: [libcamera-devel] [PATCH v3 13/13] pipeline: rkisp1: Support\n\traw Bayer capture configuration","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Mon, Oct 24, 2022 at 03:03:56AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> From: Florian Sylvestre \n>\n> Implement support for raw Bayer capture during configuration generation,\n> validation and camera configuration.\n>\n> While at it, fix a typo in a comment.\n>\n> Signed-off-by: Florian Sylvestre \n> Signed-off-by: Paul Elder \n> Signed-off-by: Laurent Pinchart \n> ---\n> Changes since v2:\n>\n> - Nearly complete rewrite\n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 116 +++++++---\n> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 209 +++++++++++++++---\n> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 3 +-\n> 3 files changed, 273 insertions(+), 55 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index e57411544f7a..891fd2d50270 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -403,6 +403,30 @@ void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &meta\n> \tpipe()->tryCompleteRequest(info);\n> }\n>\n> +/* -----------------------------------------------------------------------------\n> + * Camera Configuration\n> + */\n> +\n> +namespace {\n> +\n> +/* Keep in sync with the supported raw formats in rkisp1_path.cpp. */\n> +const std::map rawFormats = {\n> +\t{ formats::SBGGR8, MEDIA_BUS_FMT_SBGGR8_1X8 },\n> +\t{ formats::SGBRG8, MEDIA_BUS_FMT_SGBRG8_1X8 },\n> +\t{ formats::SGRBG8, MEDIA_BUS_FMT_SGRBG8_1X8 },\n> +\t{ formats::SRGGB8, MEDIA_BUS_FMT_SRGGB8_1X8 },\n> +\t{ formats::SBGGR10, MEDIA_BUS_FMT_SBGGR10_1X10 },\n> +\t{ formats::SGBRG10, MEDIA_BUS_FMT_SGBRG10_1X10 },\n> +\t{ formats::SGRBG10, MEDIA_BUS_FMT_SGRBG10_1X10 },\n> +\t{ formats::SRGGB10, MEDIA_BUS_FMT_SRGGB10_1X10 },\n> +\t{ formats::SBGGR12, MEDIA_BUS_FMT_SBGGR12_1X12 },\n> +\t{ formats::SGBRG12, MEDIA_BUS_FMT_SGBRG12_1X12 },\n> +\t{ formats::SGRBG12, MEDIA_BUS_FMT_SGRBG12_1X12 },\n> +\t{ formats::SRGGB12, MEDIA_BUS_FMT_SRGGB12_1X12 },\n> +};\n> +\n> +} /* namespace */\n> +\n> RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n> \t\t\t\t\t\t RkISP1CameraData *data)\n> \t: CameraConfiguration()\n> @@ -449,6 +473,21 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \t\tstatus = Adjusted;\n> \t}\n>\n> +\t/*\n> +\t * Simultaneous capture of raw and processed streams isn't possible. If\n> +\t * there is any raw stream, cap the number of streams to one.\n> +\t */\n> +\tif (config_.size() > 1) {\n> +\t\tfor (const auto &cfg : config_) {\n> +\t\t\tif (PixelFormatInfo::info(cfg.pixelFormat).colourEncoding ==\n> +\t\t\t PixelFormatInfo::ColourEncodingRAW) {\n> +\t\t\t\tconfig_.resize(1);\n> +\t\t\t\tstatus = Adjusted;\n> +\t\t\t}\n> +\t\t\tbreak;\n\nDoes this belong to the previous if() ?\n\n\n> +\t\t}\n> +\t}\n> +\n> \t/*\n> \t * If there are more than one stream in the configuration figure out the\n> \t * order to evaluate the streams. The first stream has the highest\n> @@ -510,45 +549,51 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> \t\t\t}\n> \t\t}\n>\n> -\t\t/* All paths rejected configuraiton. */\n> +\t\t/* All paths rejected configuration. */\n> \t\tLOG(RkISP1, Debug) << \"Camera configuration not supported \"\n> \t\t\t\t << cfg.toString();\n> \t\treturn Invalid;\n> \t}\n>\n> \t/* Select the sensor format. */\n> +\tPixelFormat rawFormat;\n> \tSize maxSize;\n> -\tfor (const StreamConfiguration &cfg : config_)\n> +\n> +\tfor (const StreamConfiguration &cfg : config_) {\n> +\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);\n> +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> +\t\t\trawFormat = cfg.pixelFormat;\n> +\n> \t\tmaxSize = std::max(maxSize, cfg.size);\n> +\t}\n> +\n> +\tstd::vector mbusCodes;\n> +\n> +\tif (rawFormat.isValid()) {\n> +\t\tmbusCodes = { rawFormats.at(rawFormat) };\n> +\t} else {\n> +\t\tstd::transform(rawFormats.begin(), rawFormats.end(),\n> +\t\t\t std::back_inserter(mbusCodes),\n> +\t\t\t [](const auto &value) { return value.second; });\n> +\t}\n> +\n> +\tsensorFormat_ = sensor->getFormat(mbusCodes, maxSize);\n>\n> -\tsensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,\n> -\t\t\t\t\t MEDIA_BUS_FMT_SGBRG12_1X12,\n> -\t\t\t\t\t MEDIA_BUS_FMT_SGRBG12_1X12,\n> -\t\t\t\t\t MEDIA_BUS_FMT_SRGGB12_1X12,\n> -\t\t\t\t\t MEDIA_BUS_FMT_SBGGR10_1X10,\n> -\t\t\t\t\t MEDIA_BUS_FMT_SGBRG10_1X10,\n> -\t\t\t\t\t MEDIA_BUS_FMT_SGRBG10_1X10,\n> -\t\t\t\t\t MEDIA_BUS_FMT_SRGGB10_1X10,\n> -\t\t\t\t\t MEDIA_BUS_FMT_SBGGR8_1X8,\n> -\t\t\t\t\t MEDIA_BUS_FMT_SGBRG8_1X8,\n> -\t\t\t\t\t MEDIA_BUS_FMT_SGRBG8_1X8,\n> -\t\t\t\t\t MEDIA_BUS_FMT_SRGGB8_1X8 },\n> -\t\t\t\t\t maxSize);\n> \tif (sensorFormat_.size.isNull())\n> \t\tsensorFormat_.size = sensor->resolution();\n>\n> \treturn status;\n> }\n>\n> +/* -----------------------------------------------------------------------------\n> + * Pipeline Operations\n> + */\n> +\n> PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)\n> \t: PipelineHandler(manager), hasSelfPath_(true)\n> {\n> }\n>\n> -/* -----------------------------------------------------------------------------\n> - * Pipeline Operations\n> - */\n> -\n> std::unique_ptr\n> PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> \tconst StreamRoles &roles)\n> @@ -604,23 +649,38 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> \t\t\t\tcolorSpace = ColorSpace::Rec709;\n> \t\t\tbreak;\n>\n> +\t\tcase StreamRole::Raw:\n> +\t\t\tif (roles.size() > 1) {\n> +\t\t\t\tLOG(RkISP1, Error)\n> +\t\t\t\t\t<< \"Can't capture both raw and processed streams\";\n> +\t\t\t\treturn nullptr;\n> +\t\t\t}\n> +\n> +\t\t\tuseMainPath = true;\n> +\t\t\tcolorSpace = ColorSpace::Raw;\n> +\t\t\tbreak;\n> +\n> \t\tdefault:\n> \t\t\tLOG(RkISP1, Warning)\n> \t\t\t\t<< \"Requested stream role not supported: \" << role;\n> \t\t\treturn nullptr;\n> \t\t}\n>\n> -\t\tStreamConfiguration cfg;\n> +\t\tRkISP1Path *path;\n> +\n> \t\tif (useMainPath) {\n> -\t\t\tcfg = data->mainPath_->generateConfiguration(\n> -\t\t\t\tdata->sensor_.get());\n> +\t\t\tpath = data->mainPath_;\n> \t\t\tmainPathAvailable = false;\n> \t\t} else {\n> -\t\t\tcfg = data->selfPath_->generateConfiguration(\n> -\t\t\t\tdata->sensor_.get());\n> +\t\t\tpath = data->selfPath_;\n> \t\t\tselfPathAvailable = false;\n> \t\t}\n>\n> +\t\tStreamConfiguration cfg =\n> +\t\t\tpath->generateConfiguration(data->sensor_.get(), role);\n> +\t\tif (!cfg.pixelFormat.isValid())\n> +\t\t\treturn nullptr;\n> +\n> \t\tcfg.colorSpace = colorSpace;\n> \t\tconfig->addConfiguration(cfg);\n> \t}\n> @@ -674,10 +734,14 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> \t\t<< \"ISP input pad configured with \" << format\n> \t\t<< \" crop \" << rect;\n>\n> -\tisRaw_ = false;\n> +\tconst PixelFormat &streamFormat = config->at(0).pixelFormat;\n> +\tconst PixelFormatInfo &info = PixelFormatInfo::info(streamFormat);\n> +\tisRaw_ = info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n>\n> \t/* YUYV8_2X8 is required on the ISP source path pad for YUV output. */\n> -\tformat.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8;\n> +\tif (!isRaw_)\n> +\t\tformat.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8;\n> +\n> \tLOG(RkISP1, Debug)\n> \t\t<< \"Configuring ISP output pad with \" << format\n> \t\t<< \" crop \" << rect;\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> index cc2ac66e6939..2994bd665ebb 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> @@ -21,6 +21,39 @@ namespace libcamera {\n>\n> LOG_DECLARE_CATEGORY(RkISP1)\n>\n> +namespace {\n> +\n> +/* Keep in sync with the supported raw formats in rkisp1.cpp. */\n> +const std::map formatToMediaBus = {\n> +\t{ formats::UYVY, MEDIA_BUS_FMT_YUYV8_2X8 },\n> +\t{ formats::YUYV, MEDIA_BUS_FMT_YUYV8_2X8 },\n> +\t{ formats::NV12, MEDIA_BUS_FMT_YUYV8_1_5X8 },\n> +\t{ formats::NV21, MEDIA_BUS_FMT_YUYV8_1_5X8 },\n> +\t{ formats::NV16, MEDIA_BUS_FMT_YUYV8_2X8 },\n> +\t{ formats::NV61, MEDIA_BUS_FMT_YUYV8_2X8 },\n> +\t{ formats::YUV420, MEDIA_BUS_FMT_YUYV8_1_5X8 },\n> +\t{ formats::YVU420, MEDIA_BUS_FMT_YUYV8_1_5X8 },\n> +\t{ formats::YUV422, MEDIA_BUS_FMT_YUYV8_2X8 },\n> +\t{ formats::YVU422, MEDIA_BUS_FMT_YUYV8_2X8 },\n> +\t{ formats::R8, MEDIA_BUS_FMT_YUYV8_2X8 },\n> +\t{ formats::RGB565, MEDIA_BUS_FMT_YUYV8_2X8 },\n> +\t{ formats::XRGB8888, MEDIA_BUS_FMT_YUYV8_2X8 },\n> +\t{ formats::SBGGR8, MEDIA_BUS_FMT_SBGGR8_1X8 },\n> +\t{ formats::SGBRG8, MEDIA_BUS_FMT_SGBRG8_1X8 },\n> +\t{ formats::SGRBG8, MEDIA_BUS_FMT_SGRBG8_1X8 },\n> +\t{ formats::SRGGB8, MEDIA_BUS_FMT_SRGGB8_1X8 },\n> +\t{ formats::SBGGR10, MEDIA_BUS_FMT_SBGGR10_1X10 },\n> +\t{ formats::SGBRG10, MEDIA_BUS_FMT_SGBRG10_1X10 },\n> +\t{ formats::SGRBG10, MEDIA_BUS_FMT_SGRBG10_1X10 },\n> +\t{ formats::SRGGB10, MEDIA_BUS_FMT_SRGGB10_1X10 },\n> +\t{ formats::SBGGR12, MEDIA_BUS_FMT_SBGGR12_1X12 },\n> +\t{ formats::SGBRG12, MEDIA_BUS_FMT_SGBRG12_1X12 },\n> +\t{ formats::SGRBG12, MEDIA_BUS_FMT_SGRBG12_1X12 },\n> +\t{ formats::SRGGB12, MEDIA_BUS_FMT_SRGGB12_1X12 },\n> +};\n> +\n> +} /* namespace */\n> +\n> RkISP1Path::RkISP1Path(const char *name, const Span &formats,\n> \t\t const Size &minResolution, const Size &maxResolution)\n> \t: name_(name), running_(false), formats_(formats),\n> @@ -69,11 +102,18 @@ void RkISP1Path::populateFormats()\n> \tstd::vector formats;\n> \tfor (const auto &[format, sizes] : v4l2Formats) {\n> \t\tconst PixelFormat pixelFormat = format.toPixelFormat();\n> -\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat);\n>\n> -\t\t/* \\todo Add support for RAW formats. */\n> -\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> +\t\t/*\n> +\t\t * As a defensive measure, skip any pixel format exposed by the\n> +\t\t * driver that we don't know about. This ensures that looking up\n> +\t\t * formats in formatToMediaBus using a key from streamFormats_\n> +\t\t * will never fail in any of the other functions.\n> +\t\t */\n> +\t\tif (!formatToMediaBus.count(pixelFormat)) {\n> +\t\t\tLOG(RkISP1, Warning)\n> +\t\t\t\t<< \"Unsupported pixel format \" << pixelFormat;\n> \t\t\tcontinue;\n> +\t\t}\n>\n> \t\tstreamFormats_.insert(pixelFormat);\n>\n> @@ -86,21 +126,68 @@ void RkISP1Path::populateFormats()\n> \t}\n> }\n>\n> -StreamConfiguration RkISP1Path::generateConfiguration(const CameraSensor *sensor)\n> +StreamConfiguration\n> +RkISP1Path::generateConfiguration(const CameraSensor *sensor, StreamRole role)\n> {\n> +\tconst std::vector &mbusCodes = sensor->mbusCodes();\n> \tconst Size &resolution = sensor->resolution();\n>\n> \tSize maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n> \t\t\t\t\t .boundedTo(resolution);\n> \tSize minResolution = minResolution_.expandedToAspectRatio(resolution);\n>\n> +\t/* Create the list of supported stream formats. */\n> \tstd::map> streamFormats;\n> -\tfor (const auto &format : streamFormats_)\n> -\t\tstreamFormats[format] = { { minResolution, maxResolution } };\n> +\tunsigned int rawBitsPerPixel = 0;\n> +\tPixelFormat rawFormat;\n> +\n> +\tfor (const auto &format : streamFormats_) {\n> +\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(format);\n> +\n> +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\n> +\t\t\t/* Skip raw formats not supported by the sensor. */\n> +\t\t\tuint32_t mbusCode = formatToMediaBus.at(format);\n> +\t\t\tif (std::find(mbusCodes.begin(), mbusCodes.end(), mbusCode) ==\n> +\t\t\t mbusCodes.end())\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tstreamFormats[format] = { { resolution, resolution } };\n\nCan't the ISP crop with RAW formats ? I guess not..\n\n> +\n> +\t\t\t/*\n> +\t\t\t * Store the raw format with the higher bits per pixel\n> +\t\t\t * for later usage.\n> +\t\t\t */\n> +\t\t\tif (info.bitsPerPixel > rawBitsPerPixel) {\n> +\t\t\t\trawBitsPerPixel = info.bitsPerPixel;\n> +\t\t\t\trawFormat = format;\n> +\t\t\t}\n> +\t\t} else {\n> +\t\t\tstreamFormats[format] = { { minResolution, maxResolution } };\n> +\t\t}\n\nYou could save one indentation level by\n\n\t\tif (info.colourEncoding != PixelFormatInfo::ColourEncodingRAW) {\n\t\t\tstreamFormats[format] = { { minResolution, maxResolution } };\n continue;\n }\n\n> +\t}\n> +\n> +\t/*\n> +\t * Pick a suitable pixel format for the role. Raw streams need to use a\n> +\t * raw format, processed streams use NV12 by default.\n> +\t */\n> +\tPixelFormat format;\n> +\n> +\tif (role == StreamRole::Raw) {\n> +\t\tif (!rawFormat.isValid()) {\n> +\t\t\tLOG(RkISP1, Error)\n> +\t\t\t\t<< \"Sensor \" << sensor->model()\n> +\t\t\t\t<< \" doesn't support raw capture\";\n> +\t\t\treturn {};\n> +\t\t}\n> +\n> +\t\tformat = rawFormat;\n> +\t} else {\n> +\t\tformat = formats::NV12;\n> +\t}\n>\n> \tStreamFormats formats(streamFormats);\n> \tStreamConfiguration cfg(formats);\n> -\tcfg.pixelFormat = formats::NV12;\n> +\tcfg.pixelFormat = format;\n> \tcfg.size = maxResolution;\n> \tcfg.bufferCount = RKISP1_BUFFER_COUNT;\n>\n> @@ -110,26 +197,85 @@ StreamConfiguration RkISP1Path::generateConfiguration(const CameraSensor *sensor\n> CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> \t\t\t\t\t\t StreamConfiguration *cfg)\n> {\n> +\tconst std::vector &mbusCodes = sensor->mbusCodes();\n> \tconst Size &resolution = sensor->resolution();\n>\n> \tconst StreamConfiguration reqCfg = *cfg;\n> \tCameraConfiguration::Status status = CameraConfiguration::Valid;\n>\n> \t/*\n> -\t * Default to NV12 if the requested format is not supported. All\n> -\t * versions of the ISP are guaranteed to support NV12 on both the main\n> -\t * and self paths.\n> +\t * Validate the pixel format. If the requested format isn't supported,\n> +\t * default to either NV12 (all versions of the ISP are guaranteed to\n> +\t * support NV12 on both the main and self paths) if the requested format\n> +\t * is not a raw format, or to the supported raw format with the highest\n> +\t * bits per pixel otherwise.\n> \t */\n> -\tif (!streamFormats_.count(cfg->pixelFormat))\n> -\t\tcfg->pixelFormat = formats::NV12;\n> +\tunsigned int rawBitsPerPixel = 0;\n> +\tPixelFormat rawFormat;\n> +\tbool found = false;\n> +\n> +\tfor (const auto &format : streamFormats_) {\n> +\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(format);\n> +\n> +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\n> +\t\t\t/* Skip raw formats not supported by the sensor. */\n> +\t\t\tuint32_t mbusCode = formatToMediaBus.at(format);\n> +\t\t\tif (std::find(mbusCodes.begin(), mbusCodes.end(), mbusCode) ==\n> +\t\t\t mbusCodes.end())\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\t/*\n> +\t\t\t * Store the raw format with the higher bits per pixel\n> +\t\t\t * for later usage.\n> +\t\t\t */\n> +\t\t\tif (info.bitsPerPixel > rawBitsPerPixel) {\n> +\t\t\t\trawBitsPerPixel = info.bitsPerPixel;\n> +\t\t\t\trawFormat = format;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tif (cfg->pixelFormat == format) {\n> +\t\t\tfound = true;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tbool isRaw = PixelFormatInfo::info(cfg->pixelFormat).colourEncoding ==\n> +\t\t PixelFormatInfo::ColourEncodingRAW;\n>\n> \t/*\n> -\t * Adjust the size based on the sensor resolution and absolute limits\n> -\t * of the ISP.\n> +\t * If no raw format supported by the sensor has been found, use a\n> +\t * processed format.\n> \t */\n> -\tSize maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n> -\t\t\t\t\t .boundedTo(resolution);\n> -\tSize minResolution = minResolution_.expandedToAspectRatio(resolution);\n> +\tif (!rawFormat.isValid())\n> +\t\tisRaw = false;\n> +\n> +\tif (!found)\n> +\t\tcfg->pixelFormat = isRaw ? rawFormat : formats::NV12;\n> +\n> +\tSize minResolution;\n> +\tSize maxResolution;\n> +\n> +\tif (isRaw) {\n> +\t\t/*\n> +\t\t * Use the sensor output size closest to the requested stream\n> +\t\t * size.\n> +\t\t */\n> +\t\tuint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);\n> +\t\tV4L2SubdeviceFormat sensorFormat =\n> +\t\t\tsensor->getFormat({ mbusCode }, cfg->size);\n> +\n> +\t\tminResolution = sensorFormat.size;\n> +\t\tmaxResolution = sensorFormat.size;\n> +\t} else {\n> +\t\t/*\n> +\t\t * Adjust the size based on the sensor resolution and absolute\n> +\t\t * limits of the ISP.\n> +\t\t */\n> +\t\tminResolution = minResolution_.expandedToAspectRatio(resolution);\n> +\t\tmaxResolution = maxResolution_.boundedToAspectRatio(resolution)\n> +\t\t\t\t\t .boundedTo(resolution);\n> +\t}\n>\n> \tcfg->size.boundTo(maxResolution);\n> \tcfg->size.expandTo(minResolution);\n> @@ -182,15 +328,11 @@ int RkISP1Path::configure(const StreamConfiguration &config,\n> \t\t<< \"Configuring \" << name_ << \" resizer output pad with \"\n> \t\t<< ispFormat;\n>\n> -\tswitch (config.pixelFormat) {\n> -\tcase formats::NV12:\n> -\tcase formats::NV21:\n> -\t\tispFormat.mbus_code = MEDIA_BUS_FMT_YUYV8_1_5X8;\n> -\t\tbreak;\n> -\tdefault:\n> -\t\tispFormat.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8;\n> -\t\tbreak;\n> -\t}\n> +\t/*\n> +\t * The configuration has been validated, the pixel format is guaranteed\n> +\t * to be supported and thus found in formatToMediaBus.\n> +\t */\n> +\tispFormat.mbus_code = formatToMediaBus.at(config.pixelFormat);\n\nUff, made to end of it and it seems all fine!\n\nThanks\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n>\n> \tret = resizer_->setFormat(1, &ispFormat);\n> \tif (ret < 0)\n> @@ -266,14 +408,25 @@ void RkISP1Path::stop()\n> namespace {\n> constexpr Size RKISP1_RSZ_MP_SRC_MIN{ 32, 16 };\n> constexpr Size RKISP1_RSZ_MP_SRC_MAX{ 4416, 3312 };\n> -constexpr std::array RKISP1_RSZ_MP_FORMATS{\n> +constexpr std::array RKISP1_RSZ_MP_FORMATS{\n> \tformats::YUYV,\n> \tformats::NV16,\n> \tformats::NV61,\n> \tformats::NV21,\n> \tformats::NV12,\n> \tformats::R8,\n> -\t/* \\todo Add support for RAW formats. */\n> +\tformats::SBGGR8,\n> +\tformats::SGBRG8,\n> +\tformats::SGRBG8,\n> +\tformats::SRGGB8,\n> +\tformats::SBGGR10,\n> +\tformats::SGBRG10,\n> +\tformats::SGRBG10,\n> +\tformats::SRGGB10,\n> +\tformats::SBGGR12,\n> +\tformats::SGBRG12,\n> +\tformats::SGRBG12,\n> +\tformats::SRGGB12,\n> };\n>\n> constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 };\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> index bf4ad18fbbf2..bdf3f95b95e1 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> @@ -40,7 +40,8 @@ public:\n> \tint setEnabled(bool enable) { return link_->setEnabled(enable); }\n> \tbool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }\n>\n> -\tStreamConfiguration generateConfiguration(const CameraSensor *sensor);\n> +\tStreamConfiguration generateConfiguration(const CameraSensor *sensor,\n> +\t\t\t\t\t\t StreamRole role);\n> \tCameraConfiguration::Status validate(const CameraSensor *sensor,\n> \t\t\t\t\t StreamConfiguration *cfg);\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 84042BD16B\n\tfor ;\n\tWed, 26 Oct 2022 17:31:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BDFD862F65;\n\tWed, 26 Oct 2022 19:31:47 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3777861F4B\n\tfor ;\n\tWed, 26 Oct 2022 19:31:46 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id A940740005;\n\tWed, 26 Oct 2022 17:31:45 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666805507;\n\tbh=mlgz+dL9hbLcPs/Hv2E/7/rQ215ngH7IzzeMswz/XQI=;\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=jAW7aGvfvnWYLr9UdpnmmsMR+aIIybnvGBOv8JGu87K/1TjN8RwzTtBeeEco/E2Ap\n\tJCwNUnxUJJm4LaHAOBnrAME7GbxrOUUp4ABQB/Vq/0A/okxBnoOiLvh4Ei6u0qi2YS\n\t/ZGt6/D0kqbCVm7ylEmEpJgfC2Br9TSSUMKS2SYcNgJqsxM/sdC19nEt+af1Nadf0T\n\ttWxu9O4UOO9D0AFYaW5MHu2JO0/Dlnn1ZUbD1czoDgl1GbGhcAEs2BKXcB4tcnKYc1\n\tOO6E/4G1QLGIbfpcAQyu3p2/i0/EmH4QvPZfk0Ft4rfJl7Bw4C8WmnBiG59BBav43T\n\tzTJNL9GyXWiig==","Date":"Wed, 26 Oct 2022 19:31:44 +0200","To":"Laurent Pinchart ","Message-ID":"<20221026173144.lljhou3zuitunyc4@uno.localdomain>","References":"<20221024000356.29521-1-laurent.pinchart@ideasonboard.com>\n\t<20221024000356.29521-14-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221024000356.29521-14-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 13/13] pipeline: rkisp1: Support\n\traw Bayer capture configuration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","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":25688,"web_url":"https://patchwork.libcamera.org/comment/25688/","msgid":"","date":"2022-10-30T17:32:13","subject":"Re: [libcamera-devel] [PATCH v3 13/13] pipeline: rkisp1: Support\n\traw Bayer capture configuration","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Oct 26, 2022 at 07:31:44PM +0200, Jacopo Mondi wrote:\n> On Mon, Oct 24, 2022 at 03:03:56AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > From: Florian Sylvestre \n> >\n> > Implement support for raw Bayer capture during configuration generation,\n> > validation and camera configuration.\n> >\n> > While at it, fix a typo in a comment.\n> >\n> > Signed-off-by: Florian Sylvestre \n> > Signed-off-by: Paul Elder \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > Changes since v2:\n> >\n> > - Nearly complete rewrite\n> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 116 +++++++---\n> > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 209 +++++++++++++++---\n> > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 3 +-\n> > 3 files changed, 273 insertions(+), 55 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index e57411544f7a..891fd2d50270 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -403,6 +403,30 @@ void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &meta\n> > \tpipe()->tryCompleteRequest(info);\n> > }\n> >\n> > +/* -----------------------------------------------------------------------------\n> > + * Camera Configuration\n> > + */\n> > +\n> > +namespace {\n> > +\n> > +/* Keep in sync with the supported raw formats in rkisp1_path.cpp. */\n> > +const std::map rawFormats = {\n> > +\t{ formats::SBGGR8, MEDIA_BUS_FMT_SBGGR8_1X8 },\n> > +\t{ formats::SGBRG8, MEDIA_BUS_FMT_SGBRG8_1X8 },\n> > +\t{ formats::SGRBG8, MEDIA_BUS_FMT_SGRBG8_1X8 },\n> > +\t{ formats::SRGGB8, MEDIA_BUS_FMT_SRGGB8_1X8 },\n> > +\t{ formats::SBGGR10, MEDIA_BUS_FMT_SBGGR10_1X10 },\n> > +\t{ formats::SGBRG10, MEDIA_BUS_FMT_SGBRG10_1X10 },\n> > +\t{ formats::SGRBG10, MEDIA_BUS_FMT_SGRBG10_1X10 },\n> > +\t{ formats::SRGGB10, MEDIA_BUS_FMT_SRGGB10_1X10 },\n> > +\t{ formats::SBGGR12, MEDIA_BUS_FMT_SBGGR12_1X12 },\n> > +\t{ formats::SGBRG12, MEDIA_BUS_FMT_SGBRG12_1X12 },\n> > +\t{ formats::SGRBG12, MEDIA_BUS_FMT_SGRBG12_1X12 },\n> > +\t{ formats::SRGGB12, MEDIA_BUS_FMT_SRGGB12_1X12 },\n> > +};\n> > +\n> > +} /* namespace */\n> > +\n> > RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n> > \t\t\t\t\t\t RkISP1CameraData *data)\n> > \t: CameraConfiguration()\n> > @@ -449,6 +473,21 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > \t\tstatus = Adjusted;\n> > \t}\n> >\n> > +\t/*\n> > +\t * Simultaneous capture of raw and processed streams isn't possible. If\n> > +\t * there is any raw stream, cap the number of streams to one.\n> > +\t */\n> > +\tif (config_.size() > 1) {\n> > +\t\tfor (const auto &cfg : config_) {\n> > +\t\t\tif (PixelFormatInfo::info(cfg.pixelFormat).colourEncoding ==\n> > +\t\t\t PixelFormatInfo::ColourEncodingRAW) {\n> > +\t\t\t\tconfig_.resize(1);\n> > +\t\t\t\tstatus = Adjusted;\n> > +\t\t\t}\n> > +\t\t\tbreak;\n> \n> Does this belong to the previous if() ?\n\nIt does, thanks for spotting it.\n\n> > +\t\t}\n> > +\t}\n> > +\n> > \t/*\n> > \t * If there are more than one stream in the configuration figure out the\n> > \t * order to evaluate the streams. The first stream has the highest\n> > @@ -510,45 +549,51 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > \t\t\t}\n> > \t\t}\n> >\n> > -\t\t/* All paths rejected configuraiton. */\n> > +\t\t/* All paths rejected configuration. */\n> > \t\tLOG(RkISP1, Debug) << \"Camera configuration not supported \"\n> > \t\t\t\t << cfg.toString();\n> > \t\treturn Invalid;\n> > \t}\n> >\n> > \t/* Select the sensor format. */\n> > +\tPixelFormat rawFormat;\n> > \tSize maxSize;\n> > -\tfor (const StreamConfiguration &cfg : config_)\n> > +\n> > +\tfor (const StreamConfiguration &cfg : config_) {\n> > +\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);\n> > +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> > +\t\t\trawFormat = cfg.pixelFormat;\n> > +\n> > \t\tmaxSize = std::max(maxSize, cfg.size);\n> > +\t}\n> > +\n> > +\tstd::vector mbusCodes;\n> > +\n> > +\tif (rawFormat.isValid()) {\n> > +\t\tmbusCodes = { rawFormats.at(rawFormat) };\n> > +\t} else {\n> > +\t\tstd::transform(rawFormats.begin(), rawFormats.end(),\n> > +\t\t\t std::back_inserter(mbusCodes),\n> > +\t\t\t [](const auto &value) { return value.second; });\n> > +\t}\n> > +\n> > +\tsensorFormat_ = sensor->getFormat(mbusCodes, maxSize);\n> >\n> > -\tsensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,\n> > -\t\t\t\t\t MEDIA_BUS_FMT_SGBRG12_1X12,\n> > -\t\t\t\t\t MEDIA_BUS_FMT_SGRBG12_1X12,\n> > -\t\t\t\t\t MEDIA_BUS_FMT_SRGGB12_1X12,\n> > -\t\t\t\t\t MEDIA_BUS_FMT_SBGGR10_1X10,\n> > -\t\t\t\t\t MEDIA_BUS_FMT_SGBRG10_1X10,\n> > -\t\t\t\t\t MEDIA_BUS_FMT_SGRBG10_1X10,\n> > -\t\t\t\t\t MEDIA_BUS_FMT_SRGGB10_1X10,\n> > -\t\t\t\t\t MEDIA_BUS_FMT_SBGGR8_1X8,\n> > -\t\t\t\t\t MEDIA_BUS_FMT_SGBRG8_1X8,\n> > -\t\t\t\t\t MEDIA_BUS_FMT_SGRBG8_1X8,\n> > -\t\t\t\t\t MEDIA_BUS_FMT_SRGGB8_1X8 },\n> > -\t\t\t\t\t maxSize);\n> > \tif (sensorFormat_.size.isNull())\n> > \t\tsensorFormat_.size = sensor->resolution();\n> >\n> > \treturn status;\n> > }\n> >\n> > +/* -----------------------------------------------------------------------------\n> > + * Pipeline Operations\n> > + */\n> > +\n> > PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)\n> > \t: PipelineHandler(manager), hasSelfPath_(true)\n> > {\n> > }\n> >\n> > -/* -----------------------------------------------------------------------------\n> > - * Pipeline Operations\n> > - */\n> > -\n> > std::unique_ptr\n> > PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> > \tconst StreamRoles &roles)\n> > @@ -604,23 +649,38 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> > \t\t\t\tcolorSpace = ColorSpace::Rec709;\n> > \t\t\tbreak;\n> >\n> > +\t\tcase StreamRole::Raw:\n> > +\t\t\tif (roles.size() > 1) {\n> > +\t\t\t\tLOG(RkISP1, Error)\n> > +\t\t\t\t\t<< \"Can't capture both raw and processed streams\";\n> > +\t\t\t\treturn nullptr;\n> > +\t\t\t}\n> > +\n> > +\t\t\tuseMainPath = true;\n> > +\t\t\tcolorSpace = ColorSpace::Raw;\n> > +\t\t\tbreak;\n> > +\n> > \t\tdefault:\n> > \t\t\tLOG(RkISP1, Warning)\n> > \t\t\t\t<< \"Requested stream role not supported: \" << role;\n> > \t\t\treturn nullptr;\n> > \t\t}\n> >\n> > -\t\tStreamConfiguration cfg;\n> > +\t\tRkISP1Path *path;\n> > +\n> > \t\tif (useMainPath) {\n> > -\t\t\tcfg = data->mainPath_->generateConfiguration(\n> > -\t\t\t\tdata->sensor_.get());\n> > +\t\t\tpath = data->mainPath_;\n> > \t\t\tmainPathAvailable = false;\n> > \t\t} else {\n> > -\t\t\tcfg = data->selfPath_->generateConfiguration(\n> > -\t\t\t\tdata->sensor_.get());\n> > +\t\t\tpath = data->selfPath_;\n> > \t\t\tselfPathAvailable = false;\n> > \t\t}\n> >\n> > +\t\tStreamConfiguration cfg =\n> > +\t\t\tpath->generateConfiguration(data->sensor_.get(), role);\n> > +\t\tif (!cfg.pixelFormat.isValid())\n> > +\t\t\treturn nullptr;\n> > +\n> > \t\tcfg.colorSpace = colorSpace;\n> > \t\tconfig->addConfiguration(cfg);\n> > \t}\n> > @@ -674,10 +734,14 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> > \t\t<< \"ISP input pad configured with \" << format\n> > \t\t<< \" crop \" << rect;\n> >\n> > -\tisRaw_ = false;\n> > +\tconst PixelFormat &streamFormat = config->at(0).pixelFormat;\n> > +\tconst PixelFormatInfo &info = PixelFormatInfo::info(streamFormat);\n> > +\tisRaw_ = info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> >\n> > \t/* YUYV8_2X8 is required on the ISP source path pad for YUV output. */\n> > -\tformat.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8;\n> > +\tif (!isRaw_)\n> > +\t\tformat.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8;\n> > +\n> > \tLOG(RkISP1, Debug)\n> > \t\t<< \"Configuring ISP output pad with \" << format\n> > \t\t<< \" crop \" << rect;\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > index cc2ac66e6939..2994bd665ebb 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > @@ -21,6 +21,39 @@ namespace libcamera {\n> >\n> > LOG_DECLARE_CATEGORY(RkISP1)\n> >\n> > +namespace {\n> > +\n> > +/* Keep in sync with the supported raw formats in rkisp1.cpp. */\n> > +const std::map formatToMediaBus = {\n> > +\t{ formats::UYVY, MEDIA_BUS_FMT_YUYV8_2X8 },\n> > +\t{ formats::YUYV, MEDIA_BUS_FMT_YUYV8_2X8 },\n> > +\t{ formats::NV12, MEDIA_BUS_FMT_YUYV8_1_5X8 },\n> > +\t{ formats::NV21, MEDIA_BUS_FMT_YUYV8_1_5X8 },\n> > +\t{ formats::NV16, MEDIA_BUS_FMT_YUYV8_2X8 },\n> > +\t{ formats::NV61, MEDIA_BUS_FMT_YUYV8_2X8 },\n> > +\t{ formats::YUV420, MEDIA_BUS_FMT_YUYV8_1_5X8 },\n> > +\t{ formats::YVU420, MEDIA_BUS_FMT_YUYV8_1_5X8 },\n> > +\t{ formats::YUV422, MEDIA_BUS_FMT_YUYV8_2X8 },\n> > +\t{ formats::YVU422, MEDIA_BUS_FMT_YUYV8_2X8 },\n> > +\t{ formats::R8, MEDIA_BUS_FMT_YUYV8_2X8 },\n> > +\t{ formats::RGB565, MEDIA_BUS_FMT_YUYV8_2X8 },\n> > +\t{ formats::XRGB8888, MEDIA_BUS_FMT_YUYV8_2X8 },\n> > +\t{ formats::SBGGR8, MEDIA_BUS_FMT_SBGGR8_1X8 },\n> > +\t{ formats::SGBRG8, MEDIA_BUS_FMT_SGBRG8_1X8 },\n> > +\t{ formats::SGRBG8, MEDIA_BUS_FMT_SGRBG8_1X8 },\n> > +\t{ formats::SRGGB8, MEDIA_BUS_FMT_SRGGB8_1X8 },\n> > +\t{ formats::SBGGR10, MEDIA_BUS_FMT_SBGGR10_1X10 },\n> > +\t{ formats::SGBRG10, MEDIA_BUS_FMT_SGBRG10_1X10 },\n> > +\t{ formats::SGRBG10, MEDIA_BUS_FMT_SGRBG10_1X10 },\n> > +\t{ formats::SRGGB10, MEDIA_BUS_FMT_SRGGB10_1X10 },\n> > +\t{ formats::SBGGR12, MEDIA_BUS_FMT_SBGGR12_1X12 },\n> > +\t{ formats::SGBRG12, MEDIA_BUS_FMT_SGBRG12_1X12 },\n> > +\t{ formats::SGRBG12, MEDIA_BUS_FMT_SGRBG12_1X12 },\n> > +\t{ formats::SRGGB12, MEDIA_BUS_FMT_SRGGB12_1X12 },\n> > +};\n> > +\n> > +} /* namespace */\n> > +\n> > RkISP1Path::RkISP1Path(const char *name, const Span &formats,\n> > \t\t const Size &minResolution, const Size &maxResolution)\n> > \t: name_(name), running_(false), formats_(formats),\n> > @@ -69,11 +102,18 @@ void RkISP1Path::populateFormats()\n> > \tstd::vector formats;\n> > \tfor (const auto &[format, sizes] : v4l2Formats) {\n> > \t\tconst PixelFormat pixelFormat = format.toPixelFormat();\n> > -\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat);\n> >\n> > -\t\t/* \\todo Add support for RAW formats. */\n> > -\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)\n> > +\t\t/*\n> > +\t\t * As a defensive measure, skip any pixel format exposed by the\n> > +\t\t * driver that we don't know about. This ensures that looking up\n> > +\t\t * formats in formatToMediaBus using a key from streamFormats_\n> > +\t\t * will never fail in any of the other functions.\n> > +\t\t */\n> > +\t\tif (!formatToMediaBus.count(pixelFormat)) {\n> > +\t\t\tLOG(RkISP1, Warning)\n> > +\t\t\t\t<< \"Unsupported pixel format \" << pixelFormat;\n> > \t\t\tcontinue;\n> > +\t\t}\n> >\n> > \t\tstreamFormats_.insert(pixelFormat);\n> >\n> > @@ -86,21 +126,68 @@ void RkISP1Path::populateFormats()\n> > \t}\n> > }\n> >\n> > -StreamConfiguration RkISP1Path::generateConfiguration(const CameraSensor *sensor)\n> > +StreamConfiguration\n> > +RkISP1Path::generateConfiguration(const CameraSensor *sensor, StreamRole role)\n> > {\n> > +\tconst std::vector &mbusCodes = sensor->mbusCodes();\n> > \tconst Size &resolution = sensor->resolution();\n> >\n> > \tSize maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n> > \t\t\t\t\t .boundedTo(resolution);\n> > \tSize minResolution = minResolution_.expandedToAspectRatio(resolution);\n> >\n> > +\t/* Create the list of supported stream formats. */\n> > \tstd::map> streamFormats;\n> > -\tfor (const auto &format : streamFormats_)\n> > -\t\tstreamFormats[format] = { { minResolution, maxResolution } };\n> > +\tunsigned int rawBitsPerPixel = 0;\n> > +\tPixelFormat rawFormat;\n> > +\n> > +\tfor (const auto &format : streamFormats_) {\n> > +\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(format);\n> > +\n> > +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\n> > +\t\t\t/* Skip raw formats not supported by the sensor. */\n> > +\t\t\tuint32_t mbusCode = formatToMediaBus.at(format);\n> > +\t\t\tif (std::find(mbusCodes.begin(), mbusCodes.end(), mbusCode) ==\n> > +\t\t\t mbusCodes.end())\n> > +\t\t\t\tcontinue;\n> > +\n> > +\t\t\tstreamFormats[format] = { { resolution, resolution } };\n> \n> Can't the ISP crop with RAW formats ? I guess not..\n\nNot that I know of, it's completely bypassed.\n\n> > +\n> > +\t\t\t/*\n> > +\t\t\t * Store the raw format with the higher bits per pixel\n> > +\t\t\t * for later usage.\n> > +\t\t\t */\n> > +\t\t\tif (info.bitsPerPixel > rawBitsPerPixel) {\n> > +\t\t\t\trawBitsPerPixel = info.bitsPerPixel;\n> > +\t\t\t\trawFormat = format;\n> > +\t\t\t}\n> > +\t\t} else {\n> > +\t\t\tstreamFormats[format] = { { minResolution, maxResolution } };\n> > +\t\t}\n> \n> You could save one indentation level by\n> \n> \t\tif (info.colourEncoding != PixelFormatInfo::ColourEncodingRAW) {\n> \t\t\tstreamFormats[format] = { { minResolution, maxResolution } };\n> continue;\n> }\n\nOK.\n\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * Pick a suitable pixel format for the role. Raw streams need to use a\n> > +\t * raw format, processed streams use NV12 by default.\n> > +\t */\n> > +\tPixelFormat format;\n> > +\n> > +\tif (role == StreamRole::Raw) {\n> > +\t\tif (!rawFormat.isValid()) {\n> > +\t\t\tLOG(RkISP1, Error)\n> > +\t\t\t\t<< \"Sensor \" << sensor->model()\n> > +\t\t\t\t<< \" doesn't support raw capture\";\n> > +\t\t\treturn {};\n> > +\t\t}\n> > +\n> > +\t\tformat = rawFormat;\n> > +\t} else {\n> > +\t\tformat = formats::NV12;\n> > +\t}\n> >\n> > \tStreamFormats formats(streamFormats);\n> > \tStreamConfiguration cfg(formats);\n> > -\tcfg.pixelFormat = formats::NV12;\n> > +\tcfg.pixelFormat = format;\n> > \tcfg.size = maxResolution;\n> > \tcfg.bufferCount = RKISP1_BUFFER_COUNT;\n> >\n> > @@ -110,26 +197,85 @@ StreamConfiguration RkISP1Path::generateConfiguration(const CameraSensor *sensor\n> > CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> > \t\t\t\t\t\t StreamConfiguration *cfg)\n> > {\n> > +\tconst std::vector &mbusCodes = sensor->mbusCodes();\n> > \tconst Size &resolution = sensor->resolution();\n> >\n> > \tconst StreamConfiguration reqCfg = *cfg;\n> > \tCameraConfiguration::Status status = CameraConfiguration::Valid;\n> >\n> > \t/*\n> > -\t * Default to NV12 if the requested format is not supported. All\n> > -\t * versions of the ISP are guaranteed to support NV12 on both the main\n> > -\t * and self paths.\n> > +\t * Validate the pixel format. If the requested format isn't supported,\n> > +\t * default to either NV12 (all versions of the ISP are guaranteed to\n> > +\t * support NV12 on both the main and self paths) if the requested format\n> > +\t * is not a raw format, or to the supported raw format with the highest\n> > +\t * bits per pixel otherwise.\n> > \t */\n> > -\tif (!streamFormats_.count(cfg->pixelFormat))\n> > -\t\tcfg->pixelFormat = formats::NV12;\n> > +\tunsigned int rawBitsPerPixel = 0;\n> > +\tPixelFormat rawFormat;\n> > +\tbool found = false;\n> > +\n> > +\tfor (const auto &format : streamFormats_) {\n> > +\t\tconst PixelFormatInfo &info = PixelFormatInfo::info(format);\n> > +\n> > +\t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\n> > +\t\t\t/* Skip raw formats not supported by the sensor. */\n> > +\t\t\tuint32_t mbusCode = formatToMediaBus.at(format);\n> > +\t\t\tif (std::find(mbusCodes.begin(), mbusCodes.end(), mbusCode) ==\n> > +\t\t\t mbusCodes.end())\n> > +\t\t\t\tcontinue;\n> > +\n> > +\t\t\t/*\n> > +\t\t\t * Store the raw format with the higher bits per pixel\n> > +\t\t\t * for later usage.\n> > +\t\t\t */\n> > +\t\t\tif (info.bitsPerPixel > rawBitsPerPixel) {\n> > +\t\t\t\trawBitsPerPixel = info.bitsPerPixel;\n> > +\t\t\t\trawFormat = format;\n> > +\t\t\t}\n> > +\t\t}\n> > +\n> > +\t\tif (cfg->pixelFormat == format) {\n> > +\t\t\tfound = true;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tbool isRaw = PixelFormatInfo::info(cfg->pixelFormat).colourEncoding ==\n> > +\t\t PixelFormatInfo::ColourEncodingRAW;\n> >\n> > \t/*\n> > -\t * Adjust the size based on the sensor resolution and absolute limits\n> > -\t * of the ISP.\n> > +\t * If no raw format supported by the sensor has been found, use a\n> > +\t * processed format.\n> > \t */\n> > -\tSize maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n> > -\t\t\t\t\t .boundedTo(resolution);\n> > -\tSize minResolution = minResolution_.expandedToAspectRatio(resolution);\n> > +\tif (!rawFormat.isValid())\n> > +\t\tisRaw = false;\n> > +\n> > +\tif (!found)\n> > +\t\tcfg->pixelFormat = isRaw ? rawFormat : formats::NV12;\n> > +\n> > +\tSize minResolution;\n> > +\tSize maxResolution;\n> > +\n> > +\tif (isRaw) {\n> > +\t\t/*\n> > +\t\t * Use the sensor output size closest to the requested stream\n> > +\t\t * size.\n> > +\t\t */\n> > +\t\tuint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);\n> > +\t\tV4L2SubdeviceFormat sensorFormat =\n> > +\t\t\tsensor->getFormat({ mbusCode }, cfg->size);\n> > +\n> > +\t\tminResolution = sensorFormat.size;\n> > +\t\tmaxResolution = sensorFormat.size;\n> > +\t} else {\n> > +\t\t/*\n> > +\t\t * Adjust the size based on the sensor resolution and absolute\n> > +\t\t * limits of the ISP.\n> > +\t\t */\n> > +\t\tminResolution = minResolution_.expandedToAspectRatio(resolution);\n> > +\t\tmaxResolution = maxResolution_.boundedToAspectRatio(resolution)\n> > +\t\t\t\t\t .boundedTo(resolution);\n> > +\t}\n> >\n> > \tcfg->size.boundTo(maxResolution);\n> > \tcfg->size.expandTo(minResolution);\n> > @@ -182,15 +328,11 @@ int RkISP1Path::configure(const StreamConfiguration &config,\n> > \t\t<< \"Configuring \" << name_ << \" resizer output pad with \"\n> > \t\t<< ispFormat;\n> >\n> > -\tswitch (config.pixelFormat) {\n> > -\tcase formats::NV12:\n> > -\tcase formats::NV21:\n> > -\t\tispFormat.mbus_code = MEDIA_BUS_FMT_YUYV8_1_5X8;\n> > -\t\tbreak;\n> > -\tdefault:\n> > -\t\tispFormat.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8;\n> > -\t\tbreak;\n> > -\t}\n> > +\t/*\n> > +\t * The configuration has been validated, the pixel format is guaranteed\n> > +\t * to be supported and thus found in formatToMediaBus.\n> > +\t */\n> > +\tispFormat.mbus_code = formatToMediaBus.at(config.pixelFormat);\n> \n> Uff, made to end of it and it seems all fine!\n> \n> Thanks\n> Reviewed-by: Jacopo Mondi \n> \n> > \tret = resizer_->setFormat(1, &ispFormat);\n> > \tif (ret < 0)\n> > @@ -266,14 +408,25 @@ void RkISP1Path::stop()\n> > namespace {\n> > constexpr Size RKISP1_RSZ_MP_SRC_MIN{ 32, 16 };\n> > constexpr Size RKISP1_RSZ_MP_SRC_MAX{ 4416, 3312 };\n> > -constexpr std::array RKISP1_RSZ_MP_FORMATS{\n> > +constexpr std::array RKISP1_RSZ_MP_FORMATS{\n> > \tformats::YUYV,\n> > \tformats::NV16,\n> > \tformats::NV61,\n> > \tformats::NV21,\n> > \tformats::NV12,\n> > \tformats::R8,\n> > -\t/* \\todo Add support for RAW formats. */\n> > +\tformats::SBGGR8,\n> > +\tformats::SGBRG8,\n> > +\tformats::SGRBG8,\n> > +\tformats::SRGGB8,\n> > +\tformats::SBGGR10,\n> > +\tformats::SGBRG10,\n> > +\tformats::SGRBG10,\n> > +\tformats::SRGGB10,\n> > +\tformats::SBGGR12,\n> > +\tformats::SGBRG12,\n> > +\tformats::SGRBG12,\n> > +\tformats::SRGGB12,\n> > };\n> >\n> > constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 };\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> > index bf4ad18fbbf2..bdf3f95b95e1 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> > @@ -40,7 +40,8 @@ public:\n> > \tint setEnabled(bool enable) { return link_->setEnabled(enable); }\n> > \tbool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }\n> >\n> > -\tStreamConfiguration generateConfiguration(const CameraSensor *sensor);\n> > +\tStreamConfiguration generateConfiguration(const CameraSensor *sensor,\n> > +\t\t\t\t\t\t StreamRole role);\n> > \tCameraConfiguration::Status validate(const CameraSensor *sensor,\n> > \t\t\t\t\t StreamConfiguration *cfg);\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 67389BDB16\n\tfor ;\n\tSun, 30 Oct 2022 17:32:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C03CC63026;\n\tSun, 30 Oct 2022 18:32:41 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B445361F47\n\tfor ;\n\tSun, 30 Oct 2022 18:32:39 +0100 (CET)","from pendragon.ideasonboard.com (85-76-13-148-nat.elisa-mobile.fi\n\t[85.76.13.148])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 139D82F5;\n\tSun, 30 Oct 2022 18:32:37 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1667151161;\n\tbh=KMspHtdUajW9PjxXRR+KoH/Xt9F7TWbh0BXlXJg4CVE=;\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=uAM9n1mZfeNeRUekn8QT378Vbd3nnQsKn4SHRFnsPEVeUA6cHkPMi9eyi2OwpQsXT\n\tBsvuXZUXBWe1WOVIu82nmyQEwdOGqzmooscoxOyy+0d0nUOTsg1ldCwRNbnHsA657D\n\t/GReo39cFAimT+KzbzzW+bkcLeqSbYbBz+yLPyj0hbvOSbyjNDRwTvs8yF/55b7uXE\n\t4Rvvgahu638d7WAl1w1poAxxBxSIAYoCb3lqVEg58c3jqg3mtzvMMvsE8/+mY3+aPW\n\tIXGpDtzPNWcUqktNGp7g6oxXZALY4f8H0iUQxI5GYkADXu5kEHxnZUbAtTX20ovyi9\n\td1kStyju9t0Ug==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1667151159;\n\tbh=KMspHtdUajW9PjxXRR+KoH/Xt9F7TWbh0BXlXJg4CVE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=oWx6w3U0qfmpacsLjOBJCKgk/mFUAqGUQ6DxVAvEIzAPDSu0UyqTwHwqQtIIkUt9X\n\tZtVe447FZHXnU9YPeo5BduJdfb4dFLyxExbkY4xOuo+qYDq6SogPlkzi3ozJaFB/MH\n\tv+PyFNMt8uyKxgZgY4qaS0MjXM/qD446NF/mZ9zA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"oWx6w3U0\"; dkim-atps=neutral","Date":"Sun, 30 Oct 2022 19:32:13 +0200","To":"Jacopo Mondi ","Message-ID":"","References":"<20221024000356.29521-1-laurent.pinchart@ideasonboard.com>\n\t<20221024000356.29521-14-laurent.pinchart@ideasonboard.com>\n\t<20221026173144.lljhou3zuitunyc4@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221026173144.lljhou3zuitunyc4@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 13/13] pipeline: rkisp1: Support\n\traw Bayer capture configuration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","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\" "}}]