[libcamera-devel,v3,13/13] pipeline: rkisp1: Support raw Bayer capture configuration
diff mbox series

Message ID 20221024000356.29521-14-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • Add Bayer format support for RkISP1
Related show

Commit Message

Laurent Pinchart Oct. 24, 2022, 12:03 a.m. UTC 
From: Florian Sylvestre <fsylvestre@baylibre.com>

Implement support for raw Bayer capture during configuration generation,
validation and camera configuration.

While at it, fix a typo in a comment.

Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v2:

- Nearly complete rewrite
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 116 +++++++---
 src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 209 +++++++++++++++---
 src/libcamera/pipeline/rkisp1/rkisp1_path.h   |   3 +-
 3 files changed, 273 insertions(+), 55 deletions(-)

Comments

Nicolas Dufresne via libcamera-devel Oct. 24, 2022, 9:46 a.m. UTC | #1 
On Mon, Oct 24, 2022 at 03:03:56AM +0300, Laurent Pinchart wrote:
> From: Florian Sylvestre <fsylvestre@baylibre.com>
> 
> Implement support for raw Bayer capture during configuration generation,
> validation and camera configuration.
> 
> While at it, fix a typo in a comment.
> 
> Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
> Changes since v2:
> 
> - Nearly complete rewrite
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 116 +++++++---
>  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 209 +++++++++++++++---
>  src/libcamera/pipeline/rkisp1/rkisp1_path.h   |   3 +-
>  3 files changed, 273 insertions(+), 55 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index e57411544f7a..891fd2d50270 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -403,6 +403,30 @@ void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &meta
>  	pipe()->tryCompleteRequest(info);
>  }
>  
> +/* -----------------------------------------------------------------------------
> + * Camera Configuration
> + */
> +
> +namespace {
> +
> +/* Keep in sync with the supported raw formats in rkisp1_path.cpp. */
> +const std::map<PixelFormat, uint32_t> rawFormats = {
> +	{ formats::SBGGR8, MEDIA_BUS_FMT_SBGGR8_1X8 },
> +	{ formats::SGBRG8, MEDIA_BUS_FMT_SGBRG8_1X8 },
> +	{ formats::SGRBG8, MEDIA_BUS_FMT_SGRBG8_1X8 },
> +	{ formats::SRGGB8, MEDIA_BUS_FMT_SRGGB8_1X8 },
> +	{ formats::SBGGR10, MEDIA_BUS_FMT_SBGGR10_1X10 },
> +	{ formats::SGBRG10, MEDIA_BUS_FMT_SGBRG10_1X10 },
> +	{ formats::SGRBG10, MEDIA_BUS_FMT_SGRBG10_1X10 },
> +	{ formats::SRGGB10, MEDIA_BUS_FMT_SRGGB10_1X10 },
> +	{ formats::SBGGR12, MEDIA_BUS_FMT_SBGGR12_1X12 },
> +	{ formats::SGBRG12, MEDIA_BUS_FMT_SGBRG12_1X12 },
> +	{ formats::SGRBG12, MEDIA_BUS_FMT_SGRBG12_1X12 },
> +	{ formats::SRGGB12, MEDIA_BUS_FMT_SRGGB12_1X12 },
> +};
> +
> +} /* namespace */
> +
>  RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
>  						     RkISP1CameraData *data)
>  	: CameraConfiguration()
> @@ -449,6 +473,21 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  		status = Adjusted;
>  	}
>  
> +	/*
> +	 * Simultaneous capture of raw and processed streams isn't possible. If
> +	 * there is any raw stream, cap the number of streams to one.
> +	 */
> +	if (config_.size() > 1) {
> +		for (const auto &cfg : config_) {
> +			if (PixelFormatInfo::info(cfg.pixelFormat).colourEncoding ==
> +			    PixelFormatInfo::ColourEncodingRAW) {
> +				config_.resize(1);
> +				status = Adjusted;
> +			}
> +			break;
> +		}
> +	}
> +
>  	/*
>  	 * If there are more than one stream in the configuration figure out the
>  	 * order to evaluate the streams. The first stream has the highest
> @@ -510,45 +549,51 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  			}
>  		}
>  
> -		/* All paths rejected configuraiton. */
> +		/* All paths rejected configuration. */
>  		LOG(RkISP1, Debug) << "Camera configuration not supported "
>  				   << cfg.toString();
>  		return Invalid;
>  	}
>  
>  	/* Select the sensor format. */
> +	PixelFormat rawFormat;
>  	Size maxSize;
> -	for (const StreamConfiguration &cfg : config_)
> +
> +	for (const StreamConfiguration &cfg : config_) {
> +		const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
> +		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
> +			rawFormat = cfg.pixelFormat;
> +
>  		maxSize = std::max(maxSize, cfg.size);
> +	}
> +
> +	std::vector<unsigned int> mbusCodes;
> +
> +	if (rawFormat.isValid()) {
> +		mbusCodes = { rawFormats.at(rawFormat) };
> +	} else {
> +		std::transform(rawFormats.begin(), rawFormats.end(),
> +			       std::back_inserter(mbusCodes),
> +			       [](const auto &value) { return value.second; });
> +	}
> +
> +	sensorFormat_ = sensor->getFormat(mbusCodes, maxSize);
>  
> -	sensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,
> -					    MEDIA_BUS_FMT_SGBRG12_1X12,
> -					    MEDIA_BUS_FMT_SGRBG12_1X12,
> -					    MEDIA_BUS_FMT_SRGGB12_1X12,
> -					    MEDIA_BUS_FMT_SBGGR10_1X10,
> -					    MEDIA_BUS_FMT_SGBRG10_1X10,
> -					    MEDIA_BUS_FMT_SGRBG10_1X10,
> -					    MEDIA_BUS_FMT_SRGGB10_1X10,
> -					    MEDIA_BUS_FMT_SBGGR8_1X8,
> -					    MEDIA_BUS_FMT_SGBRG8_1X8,
> -					    MEDIA_BUS_FMT_SGRBG8_1X8,
> -					    MEDIA_BUS_FMT_SRGGB8_1X8 },
> -					  maxSize);
>  	if (sensorFormat_.size.isNull())
>  		sensorFormat_.size = sensor->resolution();
>  
>  	return status;
>  }
>  
> +/* -----------------------------------------------------------------------------
> + * Pipeline Operations
> + */
> +
>  PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)
>  	: PipelineHandler(manager), hasSelfPath_(true)
>  {
>  }
>  
> -/* -----------------------------------------------------------------------------
> - * Pipeline Operations
> - */
> -
>  std::unique_ptr<CameraConfiguration>
>  PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
>  	const StreamRoles &roles)
> @@ -604,23 +649,38 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
>  				colorSpace = ColorSpace::Rec709;
>  			break;
>  
> +		case StreamRole::Raw:
> +			if (roles.size() > 1) {
> +				LOG(RkISP1, Error)
> +					<< "Can't capture both raw and processed streams";
> +				return nullptr;
> +			}
> +
> +			useMainPath = true;
> +			colorSpace = ColorSpace::Raw;
> +			break;
> +
>  		default:
>  			LOG(RkISP1, Warning)
>  				<< "Requested stream role not supported: " << role;
>  			return nullptr;
>  		}
>  
> -		StreamConfiguration cfg;
> +		RkISP1Path *path;
> +
>  		if (useMainPath) {
> -			cfg = data->mainPath_->generateConfiguration(
> -				data->sensor_.get());
> +			path = data->mainPath_;
>  			mainPathAvailable = false;
>  		} else {
> -			cfg = data->selfPath_->generateConfiguration(
> -				data->sensor_.get());
> +			path = data->selfPath_;
>  			selfPathAvailable = false;
>  		}
>  
> +		StreamConfiguration cfg =
> +			path->generateConfiguration(data->sensor_.get(), role);
> +		if (!cfg.pixelFormat.isValid())
> +			return nullptr;
> +
>  		cfg.colorSpace = colorSpace;
>  		config->addConfiguration(cfg);
>  	}
> @@ -674,10 +734,14 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>  		<< "ISP input pad configured with " << format
>  		<< " crop " << rect;
>  
> -	isRaw_ = false;
> +	const PixelFormat &streamFormat = config->at(0).pixelFormat;
> +	const PixelFormatInfo &info = PixelFormatInfo::info(streamFormat);
> +	isRaw_ = info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;
>  
>  	/* YUYV8_2X8 is required on the ISP source path pad for YUV output. */
> -	format.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8;
> +	if (!isRaw_)
> +		format.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8;
> +
>  	LOG(RkISP1, Debug)
>  		<< "Configuring ISP output pad with " << format
>  		<< " crop " << rect;
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> index cc2ac66e6939..2994bd665ebb 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> @@ -21,6 +21,39 @@ namespace libcamera {
>  
>  LOG_DECLARE_CATEGORY(RkISP1)
>  
> +namespace {
> +
> +/* Keep in sync with the supported raw formats in rkisp1.cpp. */
> +const std::map<PixelFormat, uint32_t> formatToMediaBus = {
> +	{ formats::UYVY, MEDIA_BUS_FMT_YUYV8_2X8 },
> +	{ formats::YUYV, MEDIA_BUS_FMT_YUYV8_2X8 },
> +	{ formats::NV12, MEDIA_BUS_FMT_YUYV8_1_5X8 },
> +	{ formats::NV21, MEDIA_BUS_FMT_YUYV8_1_5X8 },
> +	{ formats::NV16, MEDIA_BUS_FMT_YUYV8_2X8 },
> +	{ formats::NV61, MEDIA_BUS_FMT_YUYV8_2X8 },
> +	{ formats::YUV420, MEDIA_BUS_FMT_YUYV8_1_5X8 },
> +	{ formats::YVU420, MEDIA_BUS_FMT_YUYV8_1_5X8 },
> +	{ formats::YUV422, MEDIA_BUS_FMT_YUYV8_2X8 },
> +	{ formats::YVU422, MEDIA_BUS_FMT_YUYV8_2X8 },
> +	{ formats::R8, MEDIA_BUS_FMT_YUYV8_2X8 },
> +	{ formats::RGB565, MEDIA_BUS_FMT_YUYV8_2X8 },
> +	{ formats::XRGB8888, MEDIA_BUS_FMT_YUYV8_2X8 },
> +	{ formats::SBGGR8, MEDIA_BUS_FMT_SBGGR8_1X8 },
> +	{ formats::SGBRG8, MEDIA_BUS_FMT_SGBRG8_1X8 },
> +	{ formats::SGRBG8, MEDIA_BUS_FMT_SGRBG8_1X8 },
> +	{ formats::SRGGB8, MEDIA_BUS_FMT_SRGGB8_1X8 },
> +	{ formats::SBGGR10, MEDIA_BUS_FMT_SBGGR10_1X10 },
> +	{ formats::SGBRG10, MEDIA_BUS_FMT_SGBRG10_1X10 },
> +	{ formats::SGRBG10, MEDIA_BUS_FMT_SGRBG10_1X10 },
> +	{ formats::SRGGB10, MEDIA_BUS_FMT_SRGGB10_1X10 },
> +	{ formats::SBGGR12, MEDIA_BUS_FMT_SBGGR12_1X12 },
> +	{ formats::SGBRG12, MEDIA_BUS_FMT_SGBRG12_1X12 },
> +	{ formats::SGRBG12, MEDIA_BUS_FMT_SGRBG12_1X12 },
> +	{ formats::SRGGB12, MEDIA_BUS_FMT_SRGGB12_1X12 },
> +};
> +
> +} /* namespace */
> +
>  RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats,
>  		       const Size &minResolution, const Size &maxResolution)
>  	: name_(name), running_(false), formats_(formats),
> @@ -69,11 +102,18 @@ void RkISP1Path::populateFormats()
>  	std::vector<PixelFormat> formats;
>  	for (const auto &[format, sizes] : v4l2Formats) {
>  		const PixelFormat pixelFormat = format.toPixelFormat();
> -		const PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat);
>  
> -		/* \todo Add support for RAW formats. */
> -		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
> +		/*
> +		 * As a defensive measure, skip any pixel format exposed by the
> +		 * driver that we don't know about. This ensures that looking up
> +		 * formats in formatToMediaBus using a key from streamFormats_
> +		 * will never fail in any of the other functions.
> +		 */
> +		if (!formatToMediaBus.count(pixelFormat)) {
> +			LOG(RkISP1, Warning)
> +				<< "Unsupported pixel format " << pixelFormat;
>  			continue;
> +		}
>  
>  		streamFormats_.insert(pixelFormat);
>  
> @@ -86,21 +126,68 @@ void RkISP1Path::populateFormats()
>  	}
>  }
>  
> -StreamConfiguration RkISP1Path::generateConfiguration(const CameraSensor *sensor)
> +StreamConfiguration
> +RkISP1Path::generateConfiguration(const CameraSensor *sensor, StreamRole role)
>  {
> +	const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();
>  	const Size &resolution = sensor->resolution();
>  
>  	Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
>  					   .boundedTo(resolution);
>  	Size minResolution = minResolution_.expandedToAspectRatio(resolution);
>  
> +	/* Create the list of supported stream formats. */
>  	std::map<PixelFormat, std::vector<SizeRange>> streamFormats;
> -	for (const auto &format : streamFormats_)
> -		streamFormats[format] = { { minResolution, maxResolution } };
> +	unsigned int rawBitsPerPixel = 0;
> +	PixelFormat rawFormat;
> +
> +	for (const auto &format : streamFormats_) {
> +		const PixelFormatInfo &info = PixelFormatInfo::info(format);
> +
> +		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {
> +			/* Skip raw formats not supported by the sensor. */
> +			uint32_t mbusCode = formatToMediaBus.at(format);
> +			if (std::find(mbusCodes.begin(), mbusCodes.end(), mbusCode) ==
> +			    mbusCodes.end())
> +				continue;
> +
> +			streamFormats[format] = { { resolution, resolution } };
> +
> +			/*
> +			 * Store the raw format with the higher bits per pixel
> +			 * for later usage.
> +			 */
> +			if (info.bitsPerPixel > rawBitsPerPixel) {
> +				rawBitsPerPixel = info.bitsPerPixel;
> +				rawFormat = format;
> +			}
> +		} else {
> +			streamFormats[format] = { { minResolution, maxResolution } };
> +		}
> +	}
> +
> +	/*
> +	 * Pick a suitable pixel format for the role. Raw streams need to use a
> +	 * raw format, processed streams use NV12 by default.
> +	 */
> +	PixelFormat format;
> +
> +	if (role == StreamRole::Raw) {
> +		if (!rawFormat.isValid()) {
> +			LOG(RkISP1, Error)
> +				<< "Sensor " << sensor->model()
> +				<< " doesn't support raw capture";
> +			return {};
> +		}
> +
> +		format = rawFormat;
> +	} else {
> +		format = formats::NV12;
> +	}
>  
>  	StreamFormats formats(streamFormats);
>  	StreamConfiguration cfg(formats);
> -	cfg.pixelFormat = formats::NV12;
> +	cfg.pixelFormat = format;
>  	cfg.size = maxResolution;
>  	cfg.bufferCount = RKISP1_BUFFER_COUNT;
>  
> @@ -110,26 +197,85 @@ StreamConfiguration RkISP1Path::generateConfiguration(const CameraSensor *sensor
>  CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
>  						 StreamConfiguration *cfg)
>  {
> +	const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();
>  	const Size &resolution = sensor->resolution();
>  
>  	const StreamConfiguration reqCfg = *cfg;
>  	CameraConfiguration::Status status = CameraConfiguration::Valid;
>  
>  	/*
> -	 * Default to NV12 if the requested format is not supported. All
> -	 * versions of the ISP are guaranteed to support NV12 on both the main
> -	 * and self paths.
> +	 * Validate the pixel format. If the requested format isn't supported,
> +	 * default to either NV12 (all versions of the ISP are guaranteed to
> +	 * support NV12 on both the main and self paths) if the requested format
> +	 * is not a raw format, or to the supported raw format with the highest
> +	 * bits per pixel otherwise.
>  	 */
> -	if (!streamFormats_.count(cfg->pixelFormat))
> -		cfg->pixelFormat = formats::NV12;
> +	unsigned int rawBitsPerPixel = 0;
> +	PixelFormat rawFormat;
> +	bool found = false;
> +
> +	for (const auto &format : streamFormats_) {
> +		const PixelFormatInfo &info = PixelFormatInfo::info(format);
> +
> +		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {
> +			/* Skip raw formats not supported by the sensor. */
> +			uint32_t mbusCode = formatToMediaBus.at(format);
> +			if (std::find(mbusCodes.begin(), mbusCodes.end(), mbusCode) ==
> +			    mbusCodes.end())
> +				continue;
> +
> +			/*
> +			 * Store the raw format with the higher bits per pixel
> +			 * for later usage.
> +			 */
> +			if (info.bitsPerPixel > rawBitsPerPixel) {
> +				rawBitsPerPixel = info.bitsPerPixel;
> +				rawFormat = format;
> +			}
> +		}
> +
> +		if (cfg->pixelFormat == format) {
> +			found = true;
> +			break;
> +		}
> +	}
> +
> +	bool isRaw = PixelFormatInfo::info(cfg->pixelFormat).colourEncoding ==
> +		     PixelFormatInfo::ColourEncodingRAW;
>  
>  	/*
> -	 * Adjust the size based on the sensor resolution and absolute limits
> -	 * of the ISP.
> +	 * If no raw format supported by the sensor has been found, use a
> +	 * processed format.
>  	 */
> -	Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
> -					   .boundedTo(resolution);
> -	Size minResolution = minResolution_.expandedToAspectRatio(resolution);
> +	if (!rawFormat.isValid())
> +		isRaw = false;
> +
> +	if (!found)
> +		cfg->pixelFormat = isRaw ? rawFormat : formats::NV12;
> +
> +	Size minResolution;
> +	Size maxResolution;
> +
> +	if (isRaw) {
> +		/*
> +		 * Use the sensor output size closest to the requested stream
> +		 * size.
> +		 */
> +		uint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);
> +		V4L2SubdeviceFormat sensorFormat =
> +			sensor->getFormat({ mbusCode }, cfg->size);
> +
> +		minResolution = sensorFormat.size;
> +		maxResolution = sensorFormat.size;
> +	} else {
> +		/*
> +		 * Adjust the size based on the sensor resolution and absolute
> +		 * limits of the ISP.
> +		 */
> +		minResolution = minResolution_.expandedToAspectRatio(resolution);
> +		maxResolution = maxResolution_.boundedToAspectRatio(resolution)
> +					      .boundedTo(resolution);
> +	}
>  
>  	cfg->size.boundTo(maxResolution);
>  	cfg->size.expandTo(minResolution);
> @@ -182,15 +328,11 @@ int RkISP1Path::configure(const StreamConfiguration &config,
>  		<< "Configuring " << name_ << " resizer output pad with "
>  		<< ispFormat;
>  
> -	switch (config.pixelFormat) {
> -	case formats::NV12:
> -	case formats::NV21:
> -		ispFormat.mbus_code = MEDIA_BUS_FMT_YUYV8_1_5X8;
> -		break;
> -	default:
> -		ispFormat.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8;
> -		break;
> -	}
> +	/*
> +	 * The configuration has been validated, the pixel format is guaranteed
> +	 * to be supported and thus found in formatToMediaBus.
> +	 */
> +	ispFormat.mbus_code = formatToMediaBus.at(config.pixelFormat);
>  
>  	ret = resizer_->setFormat(1, &ispFormat);
>  	if (ret < 0)
> @@ -266,14 +408,25 @@ void RkISP1Path::stop()
>  namespace {
>  constexpr Size RKISP1_RSZ_MP_SRC_MIN{ 32, 16 };
>  constexpr Size RKISP1_RSZ_MP_SRC_MAX{ 4416, 3312 };
> -constexpr std::array<PixelFormat, 6> RKISP1_RSZ_MP_FORMATS{
> +constexpr std::array<PixelFormat, 18> RKISP1_RSZ_MP_FORMATS{
>  	formats::YUYV,
>  	formats::NV16,
>  	formats::NV61,
>  	formats::NV21,
>  	formats::NV12,
>  	formats::R8,
> -	/* \todo Add support for RAW formats. */
> +	formats::SBGGR8,
> +	formats::SGBRG8,
> +	formats::SGRBG8,
> +	formats::SRGGB8,
> +	formats::SBGGR10,
> +	formats::SGBRG10,
> +	formats::SGRBG10,
> +	formats::SRGGB10,
> +	formats::SBGGR12,
> +	formats::SGBRG12,
> +	formats::SGRBG12,
> +	formats::SRGGB12,
>  };
>  
>  constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 };
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> index bf4ad18fbbf2..bdf3f95b95e1 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> @@ -40,7 +40,8 @@ public:
>  	int setEnabled(bool enable) { return link_->setEnabled(enable); }
>  	bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }
>  
> -	StreamConfiguration generateConfiguration(const CameraSensor *sensor);
> +	StreamConfiguration generateConfiguration(const CameraSensor *sensor,
> +						  StreamRole role);
>  	CameraConfiguration::Status validate(const CameraSensor *sensor,
>  					     StreamConfiguration *cfg);
>  
> -- 
> Regards,
> 
> Laurent Pinchart
>
Jacopo Mondi Oct. 26, 2022, 5:31 p.m. UTC | #2 
Hi Laurent

On Mon, Oct 24, 2022 at 03:03:56AM +0300, Laurent Pinchart via libcamera-devel wrote:
> From: Florian Sylvestre <fsylvestre@baylibre.com>
>
> Implement support for raw Bayer capture during configuration generation,
> validation and camera configuration.
>
> While at it, fix a typo in a comment.
>
> Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v2:
>
> - Nearly complete rewrite
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 116 +++++++---
>  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 209 +++++++++++++++---
>  src/libcamera/pipeline/rkisp1/rkisp1_path.h   |   3 +-
>  3 files changed, 273 insertions(+), 55 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index e57411544f7a..891fd2d50270 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -403,6 +403,30 @@ void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &meta
>  	pipe()->tryCompleteRequest(info);
>  }
>
> +/* -----------------------------------------------------------------------------
> + * Camera Configuration
> + */
> +
> +namespace {
> +
> +/* Keep in sync with the supported raw formats in rkisp1_path.cpp. */
> +const std::map<PixelFormat, uint32_t> rawFormats = {
> +	{ formats::SBGGR8, MEDIA_BUS_FMT_SBGGR8_1X8 },
> +	{ formats::SGBRG8, MEDIA_BUS_FMT_SGBRG8_1X8 },
> +	{ formats::SGRBG8, MEDIA_BUS_FMT_SGRBG8_1X8 },
> +	{ formats::SRGGB8, MEDIA_BUS_FMT_SRGGB8_1X8 },
> +	{ formats::SBGGR10, MEDIA_BUS_FMT_SBGGR10_1X10 },
> +	{ formats::SGBRG10, MEDIA_BUS_FMT_SGBRG10_1X10 },
> +	{ formats::SGRBG10, MEDIA_BUS_FMT_SGRBG10_1X10 },
> +	{ formats::SRGGB10, MEDIA_BUS_FMT_SRGGB10_1X10 },
> +	{ formats::SBGGR12, MEDIA_BUS_FMT_SBGGR12_1X12 },
> +	{ formats::SGBRG12, MEDIA_BUS_FMT_SGBRG12_1X12 },
> +	{ formats::SGRBG12, MEDIA_BUS_FMT_SGRBG12_1X12 },
> +	{ formats::SRGGB12, MEDIA_BUS_FMT_SRGGB12_1X12 },
> +};
> +
> +} /* namespace */
> +
>  RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
>  						     RkISP1CameraData *data)
>  	: CameraConfiguration()
> @@ -449,6 +473,21 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  		status = Adjusted;
>  	}
>
> +	/*
> +	 * Simultaneous capture of raw and processed streams isn't possible. If
> +	 * there is any raw stream, cap the number of streams to one.
> +	 */
> +	if (config_.size() > 1) {
> +		for (const auto &cfg : config_) {
> +			if (PixelFormatInfo::info(cfg.pixelFormat).colourEncoding ==
> +			    PixelFormatInfo::ColourEncodingRAW) {
> +				config_.resize(1);
> +				status = Adjusted;
> +			}
> +			break;

Does this belong to the previous if() ?


> +		}
> +	}
> +
>  	/*
>  	 * If there are more than one stream in the configuration figure out the
>  	 * order to evaluate the streams. The first stream has the highest
> @@ -510,45 +549,51 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  			}
>  		}
>
> -		/* All paths rejected configuraiton. */
> +		/* All paths rejected configuration. */
>  		LOG(RkISP1, Debug) << "Camera configuration not supported "
>  				   << cfg.toString();
>  		return Invalid;
>  	}
>
>  	/* Select the sensor format. */
> +	PixelFormat rawFormat;
>  	Size maxSize;
> -	for (const StreamConfiguration &cfg : config_)
> +
> +	for (const StreamConfiguration &cfg : config_) {
> +		const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
> +		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
> +			rawFormat = cfg.pixelFormat;
> +
>  		maxSize = std::max(maxSize, cfg.size);
> +	}
> +
> +	std::vector<unsigned int> mbusCodes;
> +
> +	if (rawFormat.isValid()) {
> +		mbusCodes = { rawFormats.at(rawFormat) };
> +	} else {
> +		std::transform(rawFormats.begin(), rawFormats.end(),
> +			       std::back_inserter(mbusCodes),
> +			       [](const auto &value) { return value.second; });
> +	}
> +
> +	sensorFormat_ = sensor->getFormat(mbusCodes, maxSize);
>
> -	sensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,
> -					    MEDIA_BUS_FMT_SGBRG12_1X12,
> -					    MEDIA_BUS_FMT_SGRBG12_1X12,
> -					    MEDIA_BUS_FMT_SRGGB12_1X12,
> -					    MEDIA_BUS_FMT_SBGGR10_1X10,
> -					    MEDIA_BUS_FMT_SGBRG10_1X10,
> -					    MEDIA_BUS_FMT_SGRBG10_1X10,
> -					    MEDIA_BUS_FMT_SRGGB10_1X10,
> -					    MEDIA_BUS_FMT_SBGGR8_1X8,
> -					    MEDIA_BUS_FMT_SGBRG8_1X8,
> -					    MEDIA_BUS_FMT_SGRBG8_1X8,
> -					    MEDIA_BUS_FMT_SRGGB8_1X8 },
> -					  maxSize);
>  	if (sensorFormat_.size.isNull())
>  		sensorFormat_.size = sensor->resolution();
>
>  	return status;
>  }
>
> +/* -----------------------------------------------------------------------------
> + * Pipeline Operations
> + */
> +
>  PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)
>  	: PipelineHandler(manager), hasSelfPath_(true)
>  {
>  }
>
> -/* -----------------------------------------------------------------------------
> - * Pipeline Operations
> - */
> -
>  std::unique_ptr<CameraConfiguration>
>  PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
>  	const StreamRoles &roles)
> @@ -604,23 +649,38 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
>  				colorSpace = ColorSpace::Rec709;
>  			break;
>
> +		case StreamRole::Raw:
> +			if (roles.size() > 1) {
> +				LOG(RkISP1, Error)
> +					<< "Can't capture both raw and processed streams";
> +				return nullptr;
> +			}
> +
> +			useMainPath = true;
> +			colorSpace = ColorSpace::Raw;
> +			break;
> +
>  		default:
>  			LOG(RkISP1, Warning)
>  				<< "Requested stream role not supported: " << role;
>  			return nullptr;
>  		}
>
> -		StreamConfiguration cfg;
> +		RkISP1Path *path;
> +
>  		if (useMainPath) {
> -			cfg = data->mainPath_->generateConfiguration(
> -				data->sensor_.get());
> +			path = data->mainPath_;
>  			mainPathAvailable = false;
>  		} else {
> -			cfg = data->selfPath_->generateConfiguration(
> -				data->sensor_.get());
> +			path = data->selfPath_;
>  			selfPathAvailable = false;
>  		}
>
> +		StreamConfiguration cfg =
> +			path->generateConfiguration(data->sensor_.get(), role);
> +		if (!cfg.pixelFormat.isValid())
> +			return nullptr;
> +
>  		cfg.colorSpace = colorSpace;
>  		config->addConfiguration(cfg);
>  	}
> @@ -674,10 +734,14 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>  		<< "ISP input pad configured with " << format
>  		<< " crop " << rect;
>
> -	isRaw_ = false;
> +	const PixelFormat &streamFormat = config->at(0).pixelFormat;
> +	const PixelFormatInfo &info = PixelFormatInfo::info(streamFormat);
> +	isRaw_ = info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;
>
>  	/* YUYV8_2X8 is required on the ISP source path pad for YUV output. */
> -	format.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8;
> +	if (!isRaw_)
> +		format.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8;
> +
>  	LOG(RkISP1, Debug)
>  		<< "Configuring ISP output pad with " << format
>  		<< " crop " << rect;
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> index cc2ac66e6939..2994bd665ebb 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> @@ -21,6 +21,39 @@ namespace libcamera {
>
>  LOG_DECLARE_CATEGORY(RkISP1)
>
> +namespace {
> +
> +/* Keep in sync with the supported raw formats in rkisp1.cpp. */
> +const std::map<PixelFormat, uint32_t> formatToMediaBus = {
> +	{ formats::UYVY, MEDIA_BUS_FMT_YUYV8_2X8 },
> +	{ formats::YUYV, MEDIA_BUS_FMT_YUYV8_2X8 },
> +	{ formats::NV12, MEDIA_BUS_FMT_YUYV8_1_5X8 },
> +	{ formats::NV21, MEDIA_BUS_FMT_YUYV8_1_5X8 },
> +	{ formats::NV16, MEDIA_BUS_FMT_YUYV8_2X8 },
> +	{ formats::NV61, MEDIA_BUS_FMT_YUYV8_2X8 },
> +	{ formats::YUV420, MEDIA_BUS_FMT_YUYV8_1_5X8 },
> +	{ formats::YVU420, MEDIA_BUS_FMT_YUYV8_1_5X8 },
> +	{ formats::YUV422, MEDIA_BUS_FMT_YUYV8_2X8 },
> +	{ formats::YVU422, MEDIA_BUS_FMT_YUYV8_2X8 },
> +	{ formats::R8, MEDIA_BUS_FMT_YUYV8_2X8 },
> +	{ formats::RGB565, MEDIA_BUS_FMT_YUYV8_2X8 },
> +	{ formats::XRGB8888, MEDIA_BUS_FMT_YUYV8_2X8 },
> +	{ formats::SBGGR8, MEDIA_BUS_FMT_SBGGR8_1X8 },
> +	{ formats::SGBRG8, MEDIA_BUS_FMT_SGBRG8_1X8 },
> +	{ formats::SGRBG8, MEDIA_BUS_FMT_SGRBG8_1X8 },
> +	{ formats::SRGGB8, MEDIA_BUS_FMT_SRGGB8_1X8 },
> +	{ formats::SBGGR10, MEDIA_BUS_FMT_SBGGR10_1X10 },
> +	{ formats::SGBRG10, MEDIA_BUS_FMT_SGBRG10_1X10 },
> +	{ formats::SGRBG10, MEDIA_BUS_FMT_SGRBG10_1X10 },
> +	{ formats::SRGGB10, MEDIA_BUS_FMT_SRGGB10_1X10 },
> +	{ formats::SBGGR12, MEDIA_BUS_FMT_SBGGR12_1X12 },
> +	{ formats::SGBRG12, MEDIA_BUS_FMT_SGBRG12_1X12 },
> +	{ formats::SGRBG12, MEDIA_BUS_FMT_SGRBG12_1X12 },
> +	{ formats::SRGGB12, MEDIA_BUS_FMT_SRGGB12_1X12 },
> +};
> +
> +} /* namespace */
> +
>  RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats,
>  		       const Size &minResolution, const Size &maxResolution)
>  	: name_(name), running_(false), formats_(formats),
> @@ -69,11 +102,18 @@ void RkISP1Path::populateFormats()
>  	std::vector<PixelFormat> formats;
>  	for (const auto &[format, sizes] : v4l2Formats) {
>  		const PixelFormat pixelFormat = format.toPixelFormat();
> -		const PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat);
>
> -		/* \todo Add support for RAW formats. */
> -		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
> +		/*
> +		 * As a defensive measure, skip any pixel format exposed by the
> +		 * driver that we don't know about. This ensures that looking up
> +		 * formats in formatToMediaBus using a key from streamFormats_
> +		 * will never fail in any of the other functions.
> +		 */
> +		if (!formatToMediaBus.count(pixelFormat)) {
> +			LOG(RkISP1, Warning)
> +				<< "Unsupported pixel format " << pixelFormat;
>  			continue;
> +		}
>
>  		streamFormats_.insert(pixelFormat);
>
> @@ -86,21 +126,68 @@ void RkISP1Path::populateFormats()
>  	}
>  }
>
> -StreamConfiguration RkISP1Path::generateConfiguration(const CameraSensor *sensor)
> +StreamConfiguration
> +RkISP1Path::generateConfiguration(const CameraSensor *sensor, StreamRole role)
>  {
> +	const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();
>  	const Size &resolution = sensor->resolution();
>
>  	Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
>  					   .boundedTo(resolution);
>  	Size minResolution = minResolution_.expandedToAspectRatio(resolution);
>
> +	/* Create the list of supported stream formats. */
>  	std::map<PixelFormat, std::vector<SizeRange>> streamFormats;
> -	for (const auto &format : streamFormats_)
> -		streamFormats[format] = { { minResolution, maxResolution } };
> +	unsigned int rawBitsPerPixel = 0;
> +	PixelFormat rawFormat;
> +
> +	for (const auto &format : streamFormats_) {
> +		const PixelFormatInfo &info = PixelFormatInfo::info(format);
> +
> +		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {
> +			/* Skip raw formats not supported by the sensor. */
> +			uint32_t mbusCode = formatToMediaBus.at(format);
> +			if (std::find(mbusCodes.begin(), mbusCodes.end(), mbusCode) ==
> +			    mbusCodes.end())
> +				continue;
> +
> +			streamFormats[format] = { { resolution, resolution } };

Can't the ISP crop with RAW formats ? I guess not..

> +
> +			/*
> +			 * Store the raw format with the higher bits per pixel
> +			 * for later usage.
> +			 */
> +			if (info.bitsPerPixel > rawBitsPerPixel) {
> +				rawBitsPerPixel = info.bitsPerPixel;
> +				rawFormat = format;
> +			}
> +		} else {
> +			streamFormats[format] = { { minResolution, maxResolution } };
> +		}

You could save one indentation level by

		if (info.colourEncoding != PixelFormatInfo::ColourEncodingRAW) {
			streamFormats[format] = { { minResolution, maxResolution } };
                        continue;
                }

> +	}
> +
> +	/*
> +	 * Pick a suitable pixel format for the role. Raw streams need to use a
> +	 * raw format, processed streams use NV12 by default.
> +	 */
> +	PixelFormat format;
> +
> +	if (role == StreamRole::Raw) {
> +		if (!rawFormat.isValid()) {
> +			LOG(RkISP1, Error)
> +				<< "Sensor " << sensor->model()
> +				<< " doesn't support raw capture";
> +			return {};
> +		}
> +
> +		format = rawFormat;
> +	} else {
> +		format = formats::NV12;
> +	}
>
>  	StreamFormats formats(streamFormats);
>  	StreamConfiguration cfg(formats);
> -	cfg.pixelFormat = formats::NV12;
> +	cfg.pixelFormat = format;
>  	cfg.size = maxResolution;
>  	cfg.bufferCount = RKISP1_BUFFER_COUNT;
>
> @@ -110,26 +197,85 @@ StreamConfiguration RkISP1Path::generateConfiguration(const CameraSensor *sensor
>  CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
>  						 StreamConfiguration *cfg)
>  {
> +	const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();
>  	const Size &resolution = sensor->resolution();
>
>  	const StreamConfiguration reqCfg = *cfg;
>  	CameraConfiguration::Status status = CameraConfiguration::Valid;
>
>  	/*
> -	 * Default to NV12 if the requested format is not supported. All
> -	 * versions of the ISP are guaranteed to support NV12 on both the main
> -	 * and self paths.
> +	 * Validate the pixel format. If the requested format isn't supported,
> +	 * default to either NV12 (all versions of the ISP are guaranteed to
> +	 * support NV12 on both the main and self paths) if the requested format
> +	 * is not a raw format, or to the supported raw format with the highest
> +	 * bits per pixel otherwise.
>  	 */
> -	if (!streamFormats_.count(cfg->pixelFormat))
> -		cfg->pixelFormat = formats::NV12;
> +	unsigned int rawBitsPerPixel = 0;
> +	PixelFormat rawFormat;
> +	bool found = false;
> +
> +	for (const auto &format : streamFormats_) {
> +		const PixelFormatInfo &info = PixelFormatInfo::info(format);
> +
> +		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {
> +			/* Skip raw formats not supported by the sensor. */
> +			uint32_t mbusCode = formatToMediaBus.at(format);
> +			if (std::find(mbusCodes.begin(), mbusCodes.end(), mbusCode) ==
> +			    mbusCodes.end())
> +				continue;
> +
> +			/*
> +			 * Store the raw format with the higher bits per pixel
> +			 * for later usage.
> +			 */
> +			if (info.bitsPerPixel > rawBitsPerPixel) {
> +				rawBitsPerPixel = info.bitsPerPixel;
> +				rawFormat = format;
> +			}
> +		}
> +
> +		if (cfg->pixelFormat == format) {
> +			found = true;
> +			break;
> +		}
> +	}
> +
> +	bool isRaw = PixelFormatInfo::info(cfg->pixelFormat).colourEncoding ==
> +		     PixelFormatInfo::ColourEncodingRAW;
>
>  	/*
> -	 * Adjust the size based on the sensor resolution and absolute limits
> -	 * of the ISP.
> +	 * If no raw format supported by the sensor has been found, use a
> +	 * processed format.
>  	 */
> -	Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
> -					   .boundedTo(resolution);
> -	Size minResolution = minResolution_.expandedToAspectRatio(resolution);
> +	if (!rawFormat.isValid())
> +		isRaw = false;
> +
> +	if (!found)
> +		cfg->pixelFormat = isRaw ? rawFormat : formats::NV12;
> +
> +	Size minResolution;
> +	Size maxResolution;
> +
> +	if (isRaw) {
> +		/*
> +		 * Use the sensor output size closest to the requested stream
> +		 * size.
> +		 */
> +		uint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);
> +		V4L2SubdeviceFormat sensorFormat =
> +			sensor->getFormat({ mbusCode }, cfg->size);
> +
> +		minResolution = sensorFormat.size;
> +		maxResolution = sensorFormat.size;
> +	} else {
> +		/*
> +		 * Adjust the size based on the sensor resolution and absolute
> +		 * limits of the ISP.
> +		 */
> +		minResolution = minResolution_.expandedToAspectRatio(resolution);
> +		maxResolution = maxResolution_.boundedToAspectRatio(resolution)
> +					      .boundedTo(resolution);
> +	}
>
>  	cfg->size.boundTo(maxResolution);
>  	cfg->size.expandTo(minResolution);
> @@ -182,15 +328,11 @@ int RkISP1Path::configure(const StreamConfiguration &config,
>  		<< "Configuring " << name_ << " resizer output pad with "
>  		<< ispFormat;
>
> -	switch (config.pixelFormat) {
> -	case formats::NV12:
> -	case formats::NV21:
> -		ispFormat.mbus_code = MEDIA_BUS_FMT_YUYV8_1_5X8;
> -		break;
> -	default:
> -		ispFormat.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8;
> -		break;
> -	}
> +	/*
> +	 * The configuration has been validated, the pixel format is guaranteed
> +	 * to be supported and thus found in formatToMediaBus.
> +	 */
> +	ispFormat.mbus_code = formatToMediaBus.at(config.pixelFormat);

Uff, made to end of it and it seems all fine!

Thanks
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

>
>  	ret = resizer_->setFormat(1, &ispFormat);
>  	if (ret < 0)
> @@ -266,14 +408,25 @@ void RkISP1Path::stop()
>  namespace {
>  constexpr Size RKISP1_RSZ_MP_SRC_MIN{ 32, 16 };
>  constexpr Size RKISP1_RSZ_MP_SRC_MAX{ 4416, 3312 };
> -constexpr std::array<PixelFormat, 6> RKISP1_RSZ_MP_FORMATS{
> +constexpr std::array<PixelFormat, 18> RKISP1_RSZ_MP_FORMATS{
>  	formats::YUYV,
>  	formats::NV16,
>  	formats::NV61,
>  	formats::NV21,
>  	formats::NV12,
>  	formats::R8,
> -	/* \todo Add support for RAW formats. */
> +	formats::SBGGR8,
> +	formats::SGBRG8,
> +	formats::SGRBG8,
> +	formats::SRGGB8,
> +	formats::SBGGR10,
> +	formats::SGBRG10,
> +	formats::SGRBG10,
> +	formats::SRGGB10,
> +	formats::SBGGR12,
> +	formats::SGBRG12,
> +	formats::SGRBG12,
> +	formats::SRGGB12,
>  };
>
>  constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 };
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> index bf4ad18fbbf2..bdf3f95b95e1 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> @@ -40,7 +40,8 @@ public:
>  	int setEnabled(bool enable) { return link_->setEnabled(enable); }
>  	bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }
>
> -	StreamConfiguration generateConfiguration(const CameraSensor *sensor);
> +	StreamConfiguration generateConfiguration(const CameraSensor *sensor,
> +						  StreamRole role);
>  	CameraConfiguration::Status validate(const CameraSensor *sensor,
>  					     StreamConfiguration *cfg);
>
> --
> Regards,
>
> Laurent Pinchart
>
Laurent Pinchart Oct. 30, 2022, 5:32 p.m. UTC | #3 
Hi Jacopo,

On Wed, Oct 26, 2022 at 07:31:44PM +0200, Jacopo Mondi wrote:
> On Mon, Oct 24, 2022 at 03:03:56AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > From: Florian Sylvestre <fsylvestre@baylibre.com>
> >
> > Implement support for raw Bayer capture during configuration generation,
> > validation and camera configuration.
> >
> > While at it, fix a typo in a comment.
> >
> > Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v2:
> >
> > - Nearly complete rewrite
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 116 +++++++---
> >  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 209 +++++++++++++++---
> >  src/libcamera/pipeline/rkisp1/rkisp1_path.h   |   3 +-
> >  3 files changed, 273 insertions(+), 55 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index e57411544f7a..891fd2d50270 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -403,6 +403,30 @@ void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &meta
> >  	pipe()->tryCompleteRequest(info);
> >  }
> >
> > +/* -----------------------------------------------------------------------------
> > + * Camera Configuration
> > + */
> > +
> > +namespace {
> > +
> > +/* Keep in sync with the supported raw formats in rkisp1_path.cpp. */
> > +const std::map<PixelFormat, uint32_t> rawFormats = {
> > +	{ formats::SBGGR8, MEDIA_BUS_FMT_SBGGR8_1X8 },
> > +	{ formats::SGBRG8, MEDIA_BUS_FMT_SGBRG8_1X8 },
> > +	{ formats::SGRBG8, MEDIA_BUS_FMT_SGRBG8_1X8 },
> > +	{ formats::SRGGB8, MEDIA_BUS_FMT_SRGGB8_1X8 },
> > +	{ formats::SBGGR10, MEDIA_BUS_FMT_SBGGR10_1X10 },
> > +	{ formats::SGBRG10, MEDIA_BUS_FMT_SGBRG10_1X10 },
> > +	{ formats::SGRBG10, MEDIA_BUS_FMT_SGRBG10_1X10 },
> > +	{ formats::SRGGB10, MEDIA_BUS_FMT_SRGGB10_1X10 },
> > +	{ formats::SBGGR12, MEDIA_BUS_FMT_SBGGR12_1X12 },
> > +	{ formats::SGBRG12, MEDIA_BUS_FMT_SGBRG12_1X12 },
> > +	{ formats::SGRBG12, MEDIA_BUS_FMT_SGRBG12_1X12 },
> > +	{ formats::SRGGB12, MEDIA_BUS_FMT_SRGGB12_1X12 },
> > +};
> > +
> > +} /* namespace */
> > +
> >  RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
> >  						     RkISP1CameraData *data)
> >  	: CameraConfiguration()
> > @@ -449,6 +473,21 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> >  		status = Adjusted;
> >  	}
> >
> > +	/*
> > +	 * Simultaneous capture of raw and processed streams isn't possible. If
> > +	 * there is any raw stream, cap the number of streams to one.
> > +	 */
> > +	if (config_.size() > 1) {
> > +		for (const auto &cfg : config_) {
> > +			if (PixelFormatInfo::info(cfg.pixelFormat).colourEncoding ==
> > +			    PixelFormatInfo::ColourEncodingRAW) {
> > +				config_.resize(1);
> > +				status = Adjusted;
> > +			}
> > +			break;
> 
> Does this belong to the previous if() ?

It does, thanks for spotting it.

> > +		}
> > +	}
> > +
> >  	/*
> >  	 * If there are more than one stream in the configuration figure out the
> >  	 * order to evaluate the streams. The first stream has the highest
> > @@ -510,45 +549,51 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> >  			}
> >  		}
> >
> > -		/* All paths rejected configuraiton. */
> > +		/* All paths rejected configuration. */
> >  		LOG(RkISP1, Debug) << "Camera configuration not supported "
> >  				   << cfg.toString();
> >  		return Invalid;
> >  	}
> >
> >  	/* Select the sensor format. */
> > +	PixelFormat rawFormat;
> >  	Size maxSize;
> > -	for (const StreamConfiguration &cfg : config_)
> > +
> > +	for (const StreamConfiguration &cfg : config_) {
> > +		const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
> > +		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
> > +			rawFormat = cfg.pixelFormat;
> > +
> >  		maxSize = std::max(maxSize, cfg.size);
> > +	}
> > +
> > +	std::vector<unsigned int> mbusCodes;
> > +
> > +	if (rawFormat.isValid()) {
> > +		mbusCodes = { rawFormats.at(rawFormat) };
> > +	} else {
> > +		std::transform(rawFormats.begin(), rawFormats.end(),
> > +			       std::back_inserter(mbusCodes),
> > +			       [](const auto &value) { return value.second; });
> > +	}
> > +
> > +	sensorFormat_ = sensor->getFormat(mbusCodes, maxSize);
> >
> > -	sensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,
> > -					    MEDIA_BUS_FMT_SGBRG12_1X12,
> > -					    MEDIA_BUS_FMT_SGRBG12_1X12,
> > -					    MEDIA_BUS_FMT_SRGGB12_1X12,
> > -					    MEDIA_BUS_FMT_SBGGR10_1X10,
> > -					    MEDIA_BUS_FMT_SGBRG10_1X10,
> > -					    MEDIA_BUS_FMT_SGRBG10_1X10,
> > -					    MEDIA_BUS_FMT_SRGGB10_1X10,
> > -					    MEDIA_BUS_FMT_SBGGR8_1X8,
> > -					    MEDIA_BUS_FMT_SGBRG8_1X8,
> > -					    MEDIA_BUS_FMT_SGRBG8_1X8,
> > -					    MEDIA_BUS_FMT_SRGGB8_1X8 },
> > -					  maxSize);
> >  	if (sensorFormat_.size.isNull())
> >  		sensorFormat_.size = sensor->resolution();
> >
> >  	return status;
> >  }
> >
> > +/* -----------------------------------------------------------------------------
> > + * Pipeline Operations
> > + */
> > +
> >  PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)
> >  	: PipelineHandler(manager), hasSelfPath_(true)
> >  {
> >  }
> >
> > -/* -----------------------------------------------------------------------------
> > - * Pipeline Operations
> > - */
> > -
> >  std::unique_ptr<CameraConfiguration>
> >  PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> >  	const StreamRoles &roles)
> > @@ -604,23 +649,38 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> >  				colorSpace = ColorSpace::Rec709;
> >  			break;
> >
> > +		case StreamRole::Raw:
> > +			if (roles.size() > 1) {
> > +				LOG(RkISP1, Error)
> > +					<< "Can't capture both raw and processed streams";
> > +				return nullptr;
> > +			}
> > +
> > +			useMainPath = true;
> > +			colorSpace = ColorSpace::Raw;
> > +			break;
> > +
> >  		default:
> >  			LOG(RkISP1, Warning)
> >  				<< "Requested stream role not supported: " << role;
> >  			return nullptr;
> >  		}
> >
> > -		StreamConfiguration cfg;
> > +		RkISP1Path *path;
> > +
> >  		if (useMainPath) {
> > -			cfg = data->mainPath_->generateConfiguration(
> > -				data->sensor_.get());
> > +			path = data->mainPath_;
> >  			mainPathAvailable = false;
> >  		} else {
> > -			cfg = data->selfPath_->generateConfiguration(
> > -				data->sensor_.get());
> > +			path = data->selfPath_;
> >  			selfPathAvailable = false;
> >  		}
> >
> > +		StreamConfiguration cfg =
> > +			path->generateConfiguration(data->sensor_.get(), role);
> > +		if (!cfg.pixelFormat.isValid())
> > +			return nullptr;
> > +
> >  		cfg.colorSpace = colorSpace;
> >  		config->addConfiguration(cfg);
> >  	}
> > @@ -674,10 +734,14 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> >  		<< "ISP input pad configured with " << format
> >  		<< " crop " << rect;
> >
> > -	isRaw_ = false;
> > +	const PixelFormat &streamFormat = config->at(0).pixelFormat;
> > +	const PixelFormatInfo &info = PixelFormatInfo::info(streamFormat);
> > +	isRaw_ = info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;
> >
> >  	/* YUYV8_2X8 is required on the ISP source path pad for YUV output. */
> > -	format.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8;
> > +	if (!isRaw_)
> > +		format.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8;
> > +
> >  	LOG(RkISP1, Debug)
> >  		<< "Configuring ISP output pad with " << format
> >  		<< " crop " << rect;
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > index cc2ac66e6939..2994bd665ebb 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > @@ -21,6 +21,39 @@ namespace libcamera {
> >
> >  LOG_DECLARE_CATEGORY(RkISP1)
> >
> > +namespace {
> > +
> > +/* Keep in sync with the supported raw formats in rkisp1.cpp. */
> > +const std::map<PixelFormat, uint32_t> formatToMediaBus = {
> > +	{ formats::UYVY, MEDIA_BUS_FMT_YUYV8_2X8 },
> > +	{ formats::YUYV, MEDIA_BUS_FMT_YUYV8_2X8 },
> > +	{ formats::NV12, MEDIA_BUS_FMT_YUYV8_1_5X8 },
> > +	{ formats::NV21, MEDIA_BUS_FMT_YUYV8_1_5X8 },
> > +	{ formats::NV16, MEDIA_BUS_FMT_YUYV8_2X8 },
> > +	{ formats::NV61, MEDIA_BUS_FMT_YUYV8_2X8 },
> > +	{ formats::YUV420, MEDIA_BUS_FMT_YUYV8_1_5X8 },
> > +	{ formats::YVU420, MEDIA_BUS_FMT_YUYV8_1_5X8 },
> > +	{ formats::YUV422, MEDIA_BUS_FMT_YUYV8_2X8 },
> > +	{ formats::YVU422, MEDIA_BUS_FMT_YUYV8_2X8 },
> > +	{ formats::R8, MEDIA_BUS_FMT_YUYV8_2X8 },
> > +	{ formats::RGB565, MEDIA_BUS_FMT_YUYV8_2X8 },
> > +	{ formats::XRGB8888, MEDIA_BUS_FMT_YUYV8_2X8 },
> > +	{ formats::SBGGR8, MEDIA_BUS_FMT_SBGGR8_1X8 },
> > +	{ formats::SGBRG8, MEDIA_BUS_FMT_SGBRG8_1X8 },
> > +	{ formats::SGRBG8, MEDIA_BUS_FMT_SGRBG8_1X8 },
> > +	{ formats::SRGGB8, MEDIA_BUS_FMT_SRGGB8_1X8 },
> > +	{ formats::SBGGR10, MEDIA_BUS_FMT_SBGGR10_1X10 },
> > +	{ formats::SGBRG10, MEDIA_BUS_FMT_SGBRG10_1X10 },
> > +	{ formats::SGRBG10, MEDIA_BUS_FMT_SGRBG10_1X10 },
> > +	{ formats::SRGGB10, MEDIA_BUS_FMT_SRGGB10_1X10 },
> > +	{ formats::SBGGR12, MEDIA_BUS_FMT_SBGGR12_1X12 },
> > +	{ formats::SGBRG12, MEDIA_BUS_FMT_SGBRG12_1X12 },
> > +	{ formats::SGRBG12, MEDIA_BUS_FMT_SGRBG12_1X12 },
> > +	{ formats::SRGGB12, MEDIA_BUS_FMT_SRGGB12_1X12 },
> > +};
> > +
> > +} /* namespace */
> > +
> >  RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats,
> >  		       const Size &minResolution, const Size &maxResolution)
> >  	: name_(name), running_(false), formats_(formats),
> > @@ -69,11 +102,18 @@ void RkISP1Path::populateFormats()
> >  	std::vector<PixelFormat> formats;
> >  	for (const auto &[format, sizes] : v4l2Formats) {
> >  		const PixelFormat pixelFormat = format.toPixelFormat();
> > -		const PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat);
> >
> > -		/* \todo Add support for RAW formats. */
> > -		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
> > +		/*
> > +		 * As a defensive measure, skip any pixel format exposed by the
> > +		 * driver that we don't know about. This ensures that looking up
> > +		 * formats in formatToMediaBus using a key from streamFormats_
> > +		 * will never fail in any of the other functions.
> > +		 */
> > +		if (!formatToMediaBus.count(pixelFormat)) {
> > +			LOG(RkISP1, Warning)
> > +				<< "Unsupported pixel format " << pixelFormat;
> >  			continue;
> > +		}
> >
> >  		streamFormats_.insert(pixelFormat);
> >
> > @@ -86,21 +126,68 @@ void RkISP1Path::populateFormats()
> >  	}
> >  }
> >
> > -StreamConfiguration RkISP1Path::generateConfiguration(const CameraSensor *sensor)
> > +StreamConfiguration
> > +RkISP1Path::generateConfiguration(const CameraSensor *sensor, StreamRole role)
> >  {
> > +	const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();
> >  	const Size &resolution = sensor->resolution();
> >
> >  	Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
> >  					   .boundedTo(resolution);
> >  	Size minResolution = minResolution_.expandedToAspectRatio(resolution);
> >
> > +	/* Create the list of supported stream formats. */
> >  	std::map<PixelFormat, std::vector<SizeRange>> streamFormats;
> > -	for (const auto &format : streamFormats_)
> > -		streamFormats[format] = { { minResolution, maxResolution } };
> > +	unsigned int rawBitsPerPixel = 0;
> > +	PixelFormat rawFormat;
> > +
> > +	for (const auto &format : streamFormats_) {
> > +		const PixelFormatInfo &info = PixelFormatInfo::info(format);
> > +
> > +		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {
> > +			/* Skip raw formats not supported by the sensor. */
> > +			uint32_t mbusCode = formatToMediaBus.at(format);
> > +			if (std::find(mbusCodes.begin(), mbusCodes.end(), mbusCode) ==
> > +			    mbusCodes.end())
> > +				continue;
> > +
> > +			streamFormats[format] = { { resolution, resolution } };
> 
> Can't the ISP crop with RAW formats ? I guess not..

Not that I know of, it's completely bypassed.

> > +
> > +			/*
> > +			 * Store the raw format with the higher bits per pixel
> > +			 * for later usage.
> > +			 */
> > +			if (info.bitsPerPixel > rawBitsPerPixel) {
> > +				rawBitsPerPixel = info.bitsPerPixel;
> > +				rawFormat = format;
> > +			}
> > +		} else {
> > +			streamFormats[format] = { { minResolution, maxResolution } };
> > +		}
> 
> You could save one indentation level by
> 
> 		if (info.colourEncoding != PixelFormatInfo::ColourEncodingRAW) {
> 			streamFormats[format] = { { minResolution, maxResolution } };
>                         continue;
>                 }

OK.

> > +	}
> > +
> > +	/*
> > +	 * Pick a suitable pixel format for the role. Raw streams need to use a
> > +	 * raw format, processed streams use NV12 by default.
> > +	 */
> > +	PixelFormat format;
> > +
> > +	if (role == StreamRole::Raw) {
> > +		if (!rawFormat.isValid()) {
> > +			LOG(RkISP1, Error)
> > +				<< "Sensor " << sensor->model()
> > +				<< " doesn't support raw capture";
> > +			return {};
> > +		}
> > +
> > +		format = rawFormat;
> > +	} else {
> > +		format = formats::NV12;
> > +	}
> >
> >  	StreamFormats formats(streamFormats);
> >  	StreamConfiguration cfg(formats);
> > -	cfg.pixelFormat = formats::NV12;
> > +	cfg.pixelFormat = format;
> >  	cfg.size = maxResolution;
> >  	cfg.bufferCount = RKISP1_BUFFER_COUNT;
> >
> > @@ -110,26 +197,85 @@ StreamConfiguration RkISP1Path::generateConfiguration(const CameraSensor *sensor
> >  CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
> >  						 StreamConfiguration *cfg)
> >  {
> > +	const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();
> >  	const Size &resolution = sensor->resolution();
> >
> >  	const StreamConfiguration reqCfg = *cfg;
> >  	CameraConfiguration::Status status = CameraConfiguration::Valid;
> >
> >  	/*
> > -	 * Default to NV12 if the requested format is not supported. All
> > -	 * versions of the ISP are guaranteed to support NV12 on both the main
> > -	 * and self paths.
> > +	 * Validate the pixel format. If the requested format isn't supported,
> > +	 * default to either NV12 (all versions of the ISP are guaranteed to
> > +	 * support NV12 on both the main and self paths) if the requested format
> > +	 * is not a raw format, or to the supported raw format with the highest
> > +	 * bits per pixel otherwise.
> >  	 */
> > -	if (!streamFormats_.count(cfg->pixelFormat))
> > -		cfg->pixelFormat = formats::NV12;
> > +	unsigned int rawBitsPerPixel = 0;
> > +	PixelFormat rawFormat;
> > +	bool found = false;
> > +
> > +	for (const auto &format : streamFormats_) {
> > +		const PixelFormatInfo &info = PixelFormatInfo::info(format);
> > +
> > +		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {
> > +			/* Skip raw formats not supported by the sensor. */
> > +			uint32_t mbusCode = formatToMediaBus.at(format);
> > +			if (std::find(mbusCodes.begin(), mbusCodes.end(), mbusCode) ==
> > +			    mbusCodes.end())
> > +				continue;
> > +
> > +			/*
> > +			 * Store the raw format with the higher bits per pixel
> > +			 * for later usage.
> > +			 */
> > +			if (info.bitsPerPixel > rawBitsPerPixel) {
> > +				rawBitsPerPixel = info.bitsPerPixel;
> > +				rawFormat = format;
> > +			}
> > +		}
> > +
> > +		if (cfg->pixelFormat == format) {
> > +			found = true;
> > +			break;
> > +		}
> > +	}
> > +
> > +	bool isRaw = PixelFormatInfo::info(cfg->pixelFormat).colourEncoding ==
> > +		     PixelFormatInfo::ColourEncodingRAW;
> >
> >  	/*
> > -	 * Adjust the size based on the sensor resolution and absolute limits
> > -	 * of the ISP.
> > +	 * If no raw format supported by the sensor has been found, use a
> > +	 * processed format.
> >  	 */
> > -	Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
> > -					   .boundedTo(resolution);
> > -	Size minResolution = minResolution_.expandedToAspectRatio(resolution);
> > +	if (!rawFormat.isValid())
> > +		isRaw = false;
> > +
> > +	if (!found)
> > +		cfg->pixelFormat = isRaw ? rawFormat : formats::NV12;
> > +
> > +	Size minResolution;
> > +	Size maxResolution;
> > +
> > +	if (isRaw) {
> > +		/*
> > +		 * Use the sensor output size closest to the requested stream
> > +		 * size.
> > +		 */
> > +		uint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);
> > +		V4L2SubdeviceFormat sensorFormat =
> > +			sensor->getFormat({ mbusCode }, cfg->size);
> > +
> > +		minResolution = sensorFormat.size;
> > +		maxResolution = sensorFormat.size;
> > +	} else {
> > +		/*
> > +		 * Adjust the size based on the sensor resolution and absolute
> > +		 * limits of the ISP.
> > +		 */
> > +		minResolution = minResolution_.expandedToAspectRatio(resolution);
> > +		maxResolution = maxResolution_.boundedToAspectRatio(resolution)
> > +					      .boundedTo(resolution);
> > +	}
> >
> >  	cfg->size.boundTo(maxResolution);
> >  	cfg->size.expandTo(minResolution);
> > @@ -182,15 +328,11 @@ int RkISP1Path::configure(const StreamConfiguration &config,
> >  		<< "Configuring " << name_ << " resizer output pad with "
> >  		<< ispFormat;
> >
> > -	switch (config.pixelFormat) {
> > -	case formats::NV12:
> > -	case formats::NV21:
> > -		ispFormat.mbus_code = MEDIA_BUS_FMT_YUYV8_1_5X8;
> > -		break;
> > -	default:
> > -		ispFormat.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8;
> > -		break;
> > -	}
> > +	/*
> > +	 * The configuration has been validated, the pixel format is guaranteed
> > +	 * to be supported and thus found in formatToMediaBus.
> > +	 */
> > +	ispFormat.mbus_code = formatToMediaBus.at(config.pixelFormat);
> 
> Uff, made to end of it and it seems all fine!
> 
> Thanks
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> >  	ret = resizer_->setFormat(1, &ispFormat);
> >  	if (ret < 0)
> > @@ -266,14 +408,25 @@ void RkISP1Path::stop()
> >  namespace {
> >  constexpr Size RKISP1_RSZ_MP_SRC_MIN{ 32, 16 };
> >  constexpr Size RKISP1_RSZ_MP_SRC_MAX{ 4416, 3312 };
> > -constexpr std::array<PixelFormat, 6> RKISP1_RSZ_MP_FORMATS{
> > +constexpr std::array<PixelFormat, 18> RKISP1_RSZ_MP_FORMATS{
> >  	formats::YUYV,
> >  	formats::NV16,
> >  	formats::NV61,
> >  	formats::NV21,
> >  	formats::NV12,
> >  	formats::R8,
> > -	/* \todo Add support for RAW formats. */
> > +	formats::SBGGR8,
> > +	formats::SGBRG8,
> > +	formats::SGRBG8,
> > +	formats::SRGGB8,
> > +	formats::SBGGR10,
> > +	formats::SGBRG10,
> > +	formats::SGRBG10,
> > +	formats::SRGGB10,
> > +	formats::SBGGR12,
> > +	formats::SGBRG12,
> > +	formats::SGRBG12,
> > +	formats::SRGGB12,
> >  };
> >
> >  constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 };
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> > index bf4ad18fbbf2..bdf3f95b95e1 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> > @@ -40,7 +40,8 @@ public:
> >  	int setEnabled(bool enable) { return link_->setEnabled(enable); }
> >  	bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }
> >
> > -	StreamConfiguration generateConfiguration(const CameraSensor *sensor);
> > +	StreamConfiguration generateConfiguration(const CameraSensor *sensor,
> > +						  StreamRole role);
> >  	CameraConfiguration::Status validate(const CameraSensor *sensor,
> >  					     StreamConfiguration *cfg);
> >

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index e57411544f7a..891fd2d50270 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -403,6 +403,30 @@  void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &meta
 	pipe()->tryCompleteRequest(info);
 }
 
+/* -----------------------------------------------------------------------------
+ * Camera Configuration
+ */
+
+namespace {
+
+/* Keep in sync with the supported raw formats in rkisp1_path.cpp. */
+const std::map<PixelFormat, uint32_t> rawFormats = {
+	{ formats::SBGGR8, MEDIA_BUS_FMT_SBGGR8_1X8 },
+	{ formats::SGBRG8, MEDIA_BUS_FMT_SGBRG8_1X8 },
+	{ formats::SGRBG8, MEDIA_BUS_FMT_SGRBG8_1X8 },
+	{ formats::SRGGB8, MEDIA_BUS_FMT_SRGGB8_1X8 },
+	{ formats::SBGGR10, MEDIA_BUS_FMT_SBGGR10_1X10 },
+	{ formats::SGBRG10, MEDIA_BUS_FMT_SGBRG10_1X10 },
+	{ formats::SGRBG10, MEDIA_BUS_FMT_SGRBG10_1X10 },
+	{ formats::SRGGB10, MEDIA_BUS_FMT_SRGGB10_1X10 },
+	{ formats::SBGGR12, MEDIA_BUS_FMT_SBGGR12_1X12 },
+	{ formats::SGBRG12, MEDIA_BUS_FMT_SGBRG12_1X12 },
+	{ formats::SGRBG12, MEDIA_BUS_FMT_SGRBG12_1X12 },
+	{ formats::SRGGB12, MEDIA_BUS_FMT_SRGGB12_1X12 },
+};
+
+} /* namespace */
+
 RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
 						     RkISP1CameraData *data)
 	: CameraConfiguration()
@@ -449,6 +473,21 @@  CameraConfiguration::Status RkISP1CameraConfiguration::validate()
 		status = Adjusted;
 	}
 
+	/*
+	 * Simultaneous capture of raw and processed streams isn't possible. If
+	 * there is any raw stream, cap the number of streams to one.
+	 */
+	if (config_.size() > 1) {
+		for (const auto &cfg : config_) {
+			if (PixelFormatInfo::info(cfg.pixelFormat).colourEncoding ==
+			    PixelFormatInfo::ColourEncodingRAW) {
+				config_.resize(1);
+				status = Adjusted;
+			}
+			break;
+		}
+	}
+
 	/*
 	 * If there are more than one stream in the configuration figure out the
 	 * order to evaluate the streams. The first stream has the highest
@@ -510,45 +549,51 @@  CameraConfiguration::Status RkISP1CameraConfiguration::validate()
 			}
 		}
 
-		/* All paths rejected configuraiton. */
+		/* All paths rejected configuration. */
 		LOG(RkISP1, Debug) << "Camera configuration not supported "
 				   << cfg.toString();
 		return Invalid;
 	}
 
 	/* Select the sensor format. */
+	PixelFormat rawFormat;
 	Size maxSize;
-	for (const StreamConfiguration &cfg : config_)
+
+	for (const StreamConfiguration &cfg : config_) {
+		const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
+		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
+			rawFormat = cfg.pixelFormat;
+
 		maxSize = std::max(maxSize, cfg.size);
+	}
+
+	std::vector<unsigned int> mbusCodes;
+
+	if (rawFormat.isValid()) {
+		mbusCodes = { rawFormats.at(rawFormat) };
+	} else {
+		std::transform(rawFormats.begin(), rawFormats.end(),
+			       std::back_inserter(mbusCodes),
+			       [](const auto &value) { return value.second; });
+	}
+
+	sensorFormat_ = sensor->getFormat(mbusCodes, maxSize);
 
-	sensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,
-					    MEDIA_BUS_FMT_SGBRG12_1X12,
-					    MEDIA_BUS_FMT_SGRBG12_1X12,
-					    MEDIA_BUS_FMT_SRGGB12_1X12,
-					    MEDIA_BUS_FMT_SBGGR10_1X10,
-					    MEDIA_BUS_FMT_SGBRG10_1X10,
-					    MEDIA_BUS_FMT_SGRBG10_1X10,
-					    MEDIA_BUS_FMT_SRGGB10_1X10,
-					    MEDIA_BUS_FMT_SBGGR8_1X8,
-					    MEDIA_BUS_FMT_SGBRG8_1X8,
-					    MEDIA_BUS_FMT_SGRBG8_1X8,
-					    MEDIA_BUS_FMT_SRGGB8_1X8 },
-					  maxSize);
 	if (sensorFormat_.size.isNull())
 		sensorFormat_.size = sensor->resolution();
 
 	return status;
 }
 
+/* -----------------------------------------------------------------------------
+ * Pipeline Operations
+ */
+
 PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)
 	: PipelineHandler(manager), hasSelfPath_(true)
 {
 }
 
-/* -----------------------------------------------------------------------------
- * Pipeline Operations
- */
-
 std::unique_ptr<CameraConfiguration>
 PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
 	const StreamRoles &roles)
@@ -604,23 +649,38 @@  PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
 				colorSpace = ColorSpace::Rec709;
 			break;
 
+		case StreamRole::Raw:
+			if (roles.size() > 1) {
+				LOG(RkISP1, Error)
+					<< "Can't capture both raw and processed streams";
+				return nullptr;
+			}
+
+			useMainPath = true;
+			colorSpace = ColorSpace::Raw;
+			break;
+
 		default:
 			LOG(RkISP1, Warning)
 				<< "Requested stream role not supported: " << role;
 			return nullptr;
 		}
 
-		StreamConfiguration cfg;
+		RkISP1Path *path;
+
 		if (useMainPath) {
-			cfg = data->mainPath_->generateConfiguration(
-				data->sensor_.get());
+			path = data->mainPath_;
 			mainPathAvailable = false;
 		} else {
-			cfg = data->selfPath_->generateConfiguration(
-				data->sensor_.get());
+			path = data->selfPath_;
 			selfPathAvailable = false;
 		}
 
+		StreamConfiguration cfg =
+			path->generateConfiguration(data->sensor_.get(), role);
+		if (!cfg.pixelFormat.isValid())
+			return nullptr;
+
 		cfg.colorSpace = colorSpace;
 		config->addConfiguration(cfg);
 	}
@@ -674,10 +734,14 @@  int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
 		<< "ISP input pad configured with " << format
 		<< " crop " << rect;
 
-	isRaw_ = false;
+	const PixelFormat &streamFormat = config->at(0).pixelFormat;
+	const PixelFormatInfo &info = PixelFormatInfo::info(streamFormat);
+	isRaw_ = info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;
 
 	/* YUYV8_2X8 is required on the ISP source path pad for YUV output. */
-	format.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8;
+	if (!isRaw_)
+		format.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8;
+
 	LOG(RkISP1, Debug)
 		<< "Configuring ISP output pad with " << format
 		<< " crop " << rect;
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
index cc2ac66e6939..2994bd665ebb 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
@@ -21,6 +21,39 @@  namespace libcamera {
 
 LOG_DECLARE_CATEGORY(RkISP1)
 
+namespace {
+
+/* Keep in sync with the supported raw formats in rkisp1.cpp. */
+const std::map<PixelFormat, uint32_t> formatToMediaBus = {
+	{ formats::UYVY, MEDIA_BUS_FMT_YUYV8_2X8 },
+	{ formats::YUYV, MEDIA_BUS_FMT_YUYV8_2X8 },
+	{ formats::NV12, MEDIA_BUS_FMT_YUYV8_1_5X8 },
+	{ formats::NV21, MEDIA_BUS_FMT_YUYV8_1_5X8 },
+	{ formats::NV16, MEDIA_BUS_FMT_YUYV8_2X8 },
+	{ formats::NV61, MEDIA_BUS_FMT_YUYV8_2X8 },
+	{ formats::YUV420, MEDIA_BUS_FMT_YUYV8_1_5X8 },
+	{ formats::YVU420, MEDIA_BUS_FMT_YUYV8_1_5X8 },
+	{ formats::YUV422, MEDIA_BUS_FMT_YUYV8_2X8 },
+	{ formats::YVU422, MEDIA_BUS_FMT_YUYV8_2X8 },
+	{ formats::R8, MEDIA_BUS_FMT_YUYV8_2X8 },
+	{ formats::RGB565, MEDIA_BUS_FMT_YUYV8_2X8 },
+	{ formats::XRGB8888, MEDIA_BUS_FMT_YUYV8_2X8 },
+	{ formats::SBGGR8, MEDIA_BUS_FMT_SBGGR8_1X8 },
+	{ formats::SGBRG8, MEDIA_BUS_FMT_SGBRG8_1X8 },
+	{ formats::SGRBG8, MEDIA_BUS_FMT_SGRBG8_1X8 },
+	{ formats::SRGGB8, MEDIA_BUS_FMT_SRGGB8_1X8 },
+	{ formats::SBGGR10, MEDIA_BUS_FMT_SBGGR10_1X10 },
+	{ formats::SGBRG10, MEDIA_BUS_FMT_SGBRG10_1X10 },
+	{ formats::SGRBG10, MEDIA_BUS_FMT_SGRBG10_1X10 },
+	{ formats::SRGGB10, MEDIA_BUS_FMT_SRGGB10_1X10 },
+	{ formats::SBGGR12, MEDIA_BUS_FMT_SBGGR12_1X12 },
+	{ formats::SGBRG12, MEDIA_BUS_FMT_SGBRG12_1X12 },
+	{ formats::SGRBG12, MEDIA_BUS_FMT_SGRBG12_1X12 },
+	{ formats::SRGGB12, MEDIA_BUS_FMT_SRGGB12_1X12 },
+};
+
+} /* namespace */
+
 RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats,
 		       const Size &minResolution, const Size &maxResolution)
 	: name_(name), running_(false), formats_(formats),
@@ -69,11 +102,18 @@  void RkISP1Path::populateFormats()
 	std::vector<PixelFormat> formats;
 	for (const auto &[format, sizes] : v4l2Formats) {
 		const PixelFormat pixelFormat = format.toPixelFormat();
-		const PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat);
 
-		/* \todo Add support for RAW formats. */
-		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
+		/*
+		 * As a defensive measure, skip any pixel format exposed by the
+		 * driver that we don't know about. This ensures that looking up
+		 * formats in formatToMediaBus using a key from streamFormats_
+		 * will never fail in any of the other functions.
+		 */
+		if (!formatToMediaBus.count(pixelFormat)) {
+			LOG(RkISP1, Warning)
+				<< "Unsupported pixel format " << pixelFormat;
 			continue;
+		}
 
 		streamFormats_.insert(pixelFormat);
 
@@ -86,21 +126,68 @@  void RkISP1Path::populateFormats()
 	}
 }
 
-StreamConfiguration RkISP1Path::generateConfiguration(const CameraSensor *sensor)
+StreamConfiguration
+RkISP1Path::generateConfiguration(const CameraSensor *sensor, StreamRole role)
 {
+	const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();
 	const Size &resolution = sensor->resolution();
 
 	Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
 					   .boundedTo(resolution);
 	Size minResolution = minResolution_.expandedToAspectRatio(resolution);
 
+	/* Create the list of supported stream formats. */
 	std::map<PixelFormat, std::vector<SizeRange>> streamFormats;
-	for (const auto &format : streamFormats_)
-		streamFormats[format] = { { minResolution, maxResolution } };
+	unsigned int rawBitsPerPixel = 0;
+	PixelFormat rawFormat;
+
+	for (const auto &format : streamFormats_) {
+		const PixelFormatInfo &info = PixelFormatInfo::info(format);
+
+		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {
+			/* Skip raw formats not supported by the sensor. */
+			uint32_t mbusCode = formatToMediaBus.at(format);
+			if (std::find(mbusCodes.begin(), mbusCodes.end(), mbusCode) ==
+			    mbusCodes.end())
+				continue;
+
+			streamFormats[format] = { { resolution, resolution } };
+
+			/*
+			 * Store the raw format with the higher bits per pixel
+			 * for later usage.
+			 */
+			if (info.bitsPerPixel > rawBitsPerPixel) {
+				rawBitsPerPixel = info.bitsPerPixel;
+				rawFormat = format;
+			}
+		} else {
+			streamFormats[format] = { { minResolution, maxResolution } };
+		}
+	}
+
+	/*
+	 * Pick a suitable pixel format for the role. Raw streams need to use a
+	 * raw format, processed streams use NV12 by default.
+	 */
+	PixelFormat format;
+
+	if (role == StreamRole::Raw) {
+		if (!rawFormat.isValid()) {
+			LOG(RkISP1, Error)
+				<< "Sensor " << sensor->model()
+				<< " doesn't support raw capture";
+			return {};
+		}
+
+		format = rawFormat;
+	} else {
+		format = formats::NV12;
+	}
 
 	StreamFormats formats(streamFormats);
 	StreamConfiguration cfg(formats);
-	cfg.pixelFormat = formats::NV12;
+	cfg.pixelFormat = format;
 	cfg.size = maxResolution;
 	cfg.bufferCount = RKISP1_BUFFER_COUNT;
 
@@ -110,26 +197,85 @@  StreamConfiguration RkISP1Path::generateConfiguration(const CameraSensor *sensor
 CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
 						 StreamConfiguration *cfg)
 {
+	const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();
 	const Size &resolution = sensor->resolution();
 
 	const StreamConfiguration reqCfg = *cfg;
 	CameraConfiguration::Status status = CameraConfiguration::Valid;
 
 	/*
-	 * Default to NV12 if the requested format is not supported. All
-	 * versions of the ISP are guaranteed to support NV12 on both the main
-	 * and self paths.
+	 * Validate the pixel format. If the requested format isn't supported,
+	 * default to either NV12 (all versions of the ISP are guaranteed to
+	 * support NV12 on both the main and self paths) if the requested format
+	 * is not a raw format, or to the supported raw format with the highest
+	 * bits per pixel otherwise.
 	 */
-	if (!streamFormats_.count(cfg->pixelFormat))
-		cfg->pixelFormat = formats::NV12;
+	unsigned int rawBitsPerPixel = 0;
+	PixelFormat rawFormat;
+	bool found = false;
+
+	for (const auto &format : streamFormats_) {
+		const PixelFormatInfo &info = PixelFormatInfo::info(format);
+
+		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {
+			/* Skip raw formats not supported by the sensor. */
+			uint32_t mbusCode = formatToMediaBus.at(format);
+			if (std::find(mbusCodes.begin(), mbusCodes.end(), mbusCode) ==
+			    mbusCodes.end())
+				continue;
+
+			/*
+			 * Store the raw format with the higher bits per pixel
+			 * for later usage.
+			 */
+			if (info.bitsPerPixel > rawBitsPerPixel) {
+				rawBitsPerPixel = info.bitsPerPixel;
+				rawFormat = format;
+			}
+		}
+
+		if (cfg->pixelFormat == format) {
+			found = true;
+			break;
+		}
+	}
+
+	bool isRaw = PixelFormatInfo::info(cfg->pixelFormat).colourEncoding ==
+		     PixelFormatInfo::ColourEncodingRAW;
 
 	/*
-	 * Adjust the size based on the sensor resolution and absolute limits
-	 * of the ISP.
+	 * If no raw format supported by the sensor has been found, use a
+	 * processed format.
 	 */
-	Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
-					   .boundedTo(resolution);
-	Size minResolution = minResolution_.expandedToAspectRatio(resolution);
+	if (!rawFormat.isValid())
+		isRaw = false;
+
+	if (!found)
+		cfg->pixelFormat = isRaw ? rawFormat : formats::NV12;
+
+	Size minResolution;
+	Size maxResolution;
+
+	if (isRaw) {
+		/*
+		 * Use the sensor output size closest to the requested stream
+		 * size.
+		 */
+		uint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);
+		V4L2SubdeviceFormat sensorFormat =
+			sensor->getFormat({ mbusCode }, cfg->size);
+
+		minResolution = sensorFormat.size;
+		maxResolution = sensorFormat.size;
+	} else {
+		/*
+		 * Adjust the size based on the sensor resolution and absolute
+		 * limits of the ISP.
+		 */
+		minResolution = minResolution_.expandedToAspectRatio(resolution);
+		maxResolution = maxResolution_.boundedToAspectRatio(resolution)
+					      .boundedTo(resolution);
+	}
 
 	cfg->size.boundTo(maxResolution);
 	cfg->size.expandTo(minResolution);
@@ -182,15 +328,11 @@  int RkISP1Path::configure(const StreamConfiguration &config,
 		<< "Configuring " << name_ << " resizer output pad with "
 		<< ispFormat;
 
-	switch (config.pixelFormat) {
-	case formats::NV12:
-	case formats::NV21:
-		ispFormat.mbus_code = MEDIA_BUS_FMT_YUYV8_1_5X8;
-		break;
-	default:
-		ispFormat.mbus_code = MEDIA_BUS_FMT_YUYV8_2X8;
-		break;
-	}
+	/*
+	 * The configuration has been validated, the pixel format is guaranteed
+	 * to be supported and thus found in formatToMediaBus.
+	 */
+	ispFormat.mbus_code = formatToMediaBus.at(config.pixelFormat);
 
 	ret = resizer_->setFormat(1, &ispFormat);
 	if (ret < 0)
@@ -266,14 +408,25 @@  void RkISP1Path::stop()
 namespace {
 constexpr Size RKISP1_RSZ_MP_SRC_MIN{ 32, 16 };
 constexpr Size RKISP1_RSZ_MP_SRC_MAX{ 4416, 3312 };
-constexpr std::array<PixelFormat, 6> RKISP1_RSZ_MP_FORMATS{
+constexpr std::array<PixelFormat, 18> RKISP1_RSZ_MP_FORMATS{
 	formats::YUYV,
 	formats::NV16,
 	formats::NV61,
 	formats::NV21,
 	formats::NV12,
 	formats::R8,
-	/* \todo Add support for RAW formats. */
+	formats::SBGGR8,
+	formats::SGBRG8,
+	formats::SGRBG8,
+	formats::SRGGB8,
+	formats::SBGGR10,
+	formats::SGBRG10,
+	formats::SGRBG10,
+	formats::SRGGB10,
+	formats::SBGGR12,
+	formats::SGBRG12,
+	formats::SGRBG12,
+	formats::SRGGB12,
 };
 
 constexpr Size RKISP1_RSZ_SP_SRC_MIN{ 32, 16 };
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
index bf4ad18fbbf2..bdf3f95b95e1 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
+++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
@@ -40,7 +40,8 @@  public:
 	int setEnabled(bool enable) { return link_->setEnabled(enable); }
 	bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }
 
-	StreamConfiguration generateConfiguration(const CameraSensor *sensor);
+	StreamConfiguration generateConfiguration(const CameraSensor *sensor,
+						  StreamRole role);
 	CameraConfiguration::Status validate(const CameraSensor *sensor,
 					     StreamConfiguration *cfg);