[libcamera-devel,v2,5/6] libcamera: imx8-isi: Split Bayer/YUV config generation
diff mbox series

Message ID 20230313091944.9530-6-jacopo.mondi@ideasonboard.com
State Accepted
Headers show
Series
  • libcamera: imx8-isi: Remove pixelformat-2-media-bus map
Related show

Commit Message

Jacopo Mondi March 13, 2023, 9:19 a.m. UTC 
At generateConfiguration() a YUV/RGB pixel format is preferred for the
StillCapture/VideoRecording/Viewfinder roles, but currently there are no
guarantees in place that the sensor provides a non-Bayer bus format from
which YUV/RGB can be generated.

This makes the default configuration generated for those roles not to
work if the sensor is a RAW-only one.

To improve the situation split the configuration generation in two,
one for YUV modes and one for Raw Bayer mode.

StreamRoles assigned to a YUV mode will try to first generate a YUV
configuration and then fallback to RAW if that's what the sensor can
provide.

As an additional requirement, for YUV streams, the generated mode has to
be validated with the sensor to confirm the desired sizes can be
generated. In order to test a format use the newly introduced
CameraSensor::tryFormat().

Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Tested-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 231 +++++++++++++------
 1 file changed, 155 insertions(+), 76 deletions(-)

Comments

Laurent Pinchart April 24, 2023, 6:06 a.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On Mon, Mar 13, 2023 at 10:19:43AM +0100, Jacopo Mondi wrote:
> At generateConfiguration() a YUV/RGB pixel format is preferred for the
> StillCapture/VideoRecording/Viewfinder roles, but currently there are no
> guarantees in place that the sensor provides a non-Bayer bus format from
> which YUV/RGB can be generated.
> 
> This makes the default configuration generated for those roles not to
> work if the sensor is a RAW-only one.
> 
> To improve the situation split the configuration generation in two,
> one for YUV modes and one for Raw Bayer mode.
> 
> StreamRoles assigned to a YUV mode will try to first generate a YUV
> configuration and then fallback to RAW if that's what the sensor can
> provide.
> 
> As an additional requirement, for YUV streams, the generated mode has to
> be validated with the sensor to confirm the desired sizes can be
> generated. In order to test a format use the newly introduced
> CameraSensor::tryFormat().

Please explain in the commit message (and/or a comment in the code) why
this is needed.

I'm also tempted to ask for this new requirement to be split to a
separate patch, to ease review.

> Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> Tested-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
>  src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 231 +++++++++++++------
>  1 file changed, 155 insertions(+), 76 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> index 146ba7465012..a4f437f55b26 100644
> --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> @@ -145,6 +145,10 @@ private:
>  
>  	Pipe *pipeFromStream(Camera *camera, const Stream *stream);
>  
> +	StreamConfiguration generateYUVConfiguration(Camera *camera,
> +						     const Size &size);
> +	StreamConfiguration generateRawConfiguration(Camera *camera);
> +
>  	void bufferReady(FrameBuffer *buffer);
>  
>  	MediaDevice *isiDev_;
> @@ -705,6 +709,129 @@ PipelineHandlerISI::PipelineHandlerISI(CameraManager *manager)
>  {
>  }
>  
> +/*
> + * Generate a StreamConfiguration for YUV/RGB use case.
> + *
> + * Verify it the sensor can produce a YUV/RGB media bus format and collect
> + * all the processed pixel formats the ISI can generate as supported stream
> + * configurations.
> + */
> +StreamConfiguration PipelineHandlerISI::generateYUVConfiguration(Camera *camera,
> +								 const Size &size)
> +{
> +	ISICameraData *data = cameraData(camera);
> +	PixelFormat pixelFormat = formats::YUYV;
> +	unsigned int mbusCode;
> +
> +	mbusCode = data->getYuvMediaBusFormat(&pixelFormat);
> +	if (!mbusCode)
> +		return {};
> +
> +	/* Adjust the requested size to the sensor's capabilities. */
> +	const CameraSensor *sensor = data->sensor_.get();
> +
> +	V4L2SubdeviceFormat sensorFmt;
> +	sensorFmt.mbus_code = mbusCode;
> +	sensorFmt.size = size;
> +
> +	int ret = sensor->tryFormat(&sensorFmt);

	int ret = data->sensor_->tryFormat(&sensorFmt);

and drop the local sensor variable.

> +	if (ret) {
> +		LOG(ISI, Error) << "Failed to try sensor format.";
> +		return {};
> +	}
> +
> +	Size sensorSize = sensorFmt.size;
> +
> +	/*
> +	 * Populate the StreamConfiguration.
> +	 *
> +	 * As the sensor supports at least one YUV/RGB media bus format all the
> +	 * processed ones in formatsMap_ can be generated from it.
> +	 */
> +	std::map<PixelFormat, std::vector<SizeRange>> streamFormats;
> +
> +	for (const auto &[pixFmt, pipeFmt] : ISICameraConfiguration::formatsMap_) {
> +		const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);
> +		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
> +			continue;
> +
> +		streamFormats[pixFmt] = { { kMinISISize, sensorSize } };
> +	}
> +
> +	StreamFormats formats(streamFormats);
> +
> +	StreamConfiguration cfg(formats);
> +	cfg.pixelFormat = pixelFormat;
> +	cfg.size = sensorSize;
> +	cfg.bufferCount = 4;
> +
> +	return cfg;
> +}
> +
> +/*
> + * Generate a StreamConfiguration for Raw Bayer use case. Verify if the sensor
> + * can produce the requested RAW bayer format and eventually adjust it to
> + * the one with the largest bit-depth the sensor can produce.
> + */
> +StreamConfiguration PipelineHandlerISI::generateRawConfiguration(Camera *camera)
> +{
> +	static const std::map<unsigned int, PixelFormat> rawFormats = {
> +		{ MEDIA_BUS_FMT_SBGGR8_1X8, formats::SBGGR8 },
> +		{ MEDIA_BUS_FMT_SGBRG8_1X8, formats::SGBRG8 },
> +		{ MEDIA_BUS_FMT_SGRBG8_1X8, formats::SGRBG8 },
> +		{ MEDIA_BUS_FMT_SRGGB8_1X8, formats::SRGGB8 },
> +		{ MEDIA_BUS_FMT_SBGGR10_1X10, formats::SBGGR10 },
> +		{ MEDIA_BUS_FMT_SGBRG10_1X10, formats::SGBRG10 },
> +		{ MEDIA_BUS_FMT_SGRBG10_1X10, formats::SGRBG10 },
> +		{ MEDIA_BUS_FMT_SRGGB10_1X10, formats::SRGGB10 },
> +		{ MEDIA_BUS_FMT_SBGGR12_1X12, formats::SBGGR12 },
> +		{ MEDIA_BUS_FMT_SGBRG12_1X12, formats::SGBRG12 },
> +		{ MEDIA_BUS_FMT_SGRBG12_1X12, formats::SGRBG12 },
> +		{ MEDIA_BUS_FMT_SRGGB12_1X12, formats::SRGGB12 },

Now that libcamera has RAW14 formats, could you add them here ?

> +	};
> +
> +	ISICameraData *data = cameraData(camera);
> +	PixelFormat pixelFormat = formats::SBGGR10;
> +	unsigned int mbusCode;
> +
> +	/* pixelFormat will be adjusted, if the sensor can produce RAW. */
> +	mbusCode = data->getRawMediaBusFormat(&pixelFormat);
> +	if (!mbusCode)
> +		return {};
> +
> +	/*
> +	 * Populate the StreamConfiguration with all the supported Bayer
> +	 * formats the sensor can produce.
> +	 */
> +	std::map<PixelFormat, std::vector<SizeRange>> streamFormats;
> +	const CameraSensor *sensor = data->sensor_.get();
> +
> +	for (unsigned int code : sensor->mbusCodes()) {
> +		/* Find a Bayer media bus code from the sensor. */
> +		const BayerFormat &bayerFormat = BayerFormat::fromMbusCode(code);
> +		if (!bayerFormat.isValid())
> +			continue;
> +
> +		auto it = rawFormats.find(code);
> +		if (it == rawFormats.end()) {
> +			LOG(ISI, Warning) << bayerFormat
> +					  << " not supported in ISI formats map.";
> +			continue;
> +		}
> +
> +		streamFormats[it->second] = { { sensor->resolution(), sensor->resolution() } };
> +	}
> +
> +	StreamFormats formats(streamFormats);
> +
> +	StreamConfiguration cfg(formats);
> +	cfg.size = sensor->resolution();
> +	cfg.pixelFormat = pixelFormat;
> +	cfg.bufferCount = 4;
> +
> +	return cfg;
> +}
> +
>  std::unique_ptr<CameraConfiguration>
>  PipelineHandlerISI::generateConfiguration(Camera *camera,
>  					  const StreamRoles &roles)
> @@ -722,79 +849,45 @@ PipelineHandlerISI::generateConfiguration(Camera *camera,
>  		return nullptr;
>  	}
>  
> -	bool isRaw = false;
>  	for (const auto &role : roles) {
>  		/*
> -		 * Prefer the following formats
> +		 * Prefer the following formats:
>  		 * - Still Capture: Full resolution YUYV
>  		 * - ViewFinder/VideoRecording: 1080p YUYV
> -		 * - RAW: sensor's native format and resolution
> +		 * - RAW: Full resolution Bayer
>  		 */
> -		std::map<PixelFormat, std::vector<SizeRange>> streamFormats;
> -		PixelFormat pixelFormat;
> -		Size size;
> +		StreamConfiguration cfg;
>  
>  		switch (role) {
>  		case StreamRole::StillCapture:
> -			/*
> -			 * \todo Make sure the sensor can produce non-RAW formats
> -			 * compatible with the ones supported by the pipeline.
> -			 */
> -			size = data->sensor_->resolution();
> -			pixelFormat = formats::YUYV;
> +			cfg = generateYUVConfiguration(camera,
> +						       data->sensor_->resolution());
> +			if (!cfg.pixelFormat.isValid()) {
> +				/*
> +				 * Fallback to use a Bayer format if that's what
> +				 * the sensor supports.
> +				 */
> +				cfg = generateRawConfiguration(camera);
> +			}
> +
>  			break;
>  
>  		case StreamRole::Viewfinder:
>  		case StreamRole::VideoRecording:
> -			/*
> -			 * \todo Make sure the sensor can produce non-RAW formats
> -			 * compatible with the ones supported by the pipeline.
> -			 */
> -			size = PipelineHandlerISI::kPreviewSize;
> -			pixelFormat = formats::YUYV;
> -			break;
> -
> -		case StreamRole::Raw: {
> -			/*
> -			 * Make sure the sensor can generate a RAW format and
> -			 * prefer the ones with a larger bitdepth.
> -			 */
> -			const ISICameraConfiguration::FormatMap::value_type *rawPipeFormat = nullptr;
> -			unsigned int maxDepth = 0;
> -
> -			for (unsigned int code : data->sensor_->mbusCodes()) {
> -				const BayerFormat &bayerFormat = BayerFormat::fromMbusCode(code);
> -				if (!bayerFormat.isValid())
> -					continue;
> -
> -				/* Make sure the format is supported by the pipeline handler. */
> -				auto it = std::find_if(ISICameraConfiguration::formatsMap_.begin(),
> -						       ISICameraConfiguration::formatsMap_.end(),
> -						       [code](auto &isiFormat) {
> -							        auto &pipe = isiFormat.second;
> -							        return pipe.sensorCode == code;
> -						       });
> -				if (it == ISICameraConfiguration::formatsMap_.end())
> -					continue;
> -
> -				if (bayerFormat.bitDepth > maxDepth) {
> -					maxDepth = bayerFormat.bitDepth;
> -					rawPipeFormat = &(*it);
> -				}
> -			}
> -
> -			if (!rawPipeFormat) {
> -				LOG(ISI, Error)
> -					<< "Cannot generate a configuration for RAW stream";
> -				return nullptr;
> +			cfg = generateYUVConfiguration(camera,
> +						       PipelineHandlerISI::kPreviewSize);
> +			if (!cfg.pixelFormat.isValid()) {
> +				/*
> +				 * Fallback to use a Bayer format if that's what
> +				 * the sensor supports.
> +				 */
> +				cfg = generateRawConfiguration(camera);
>  			}
>  
> -			size = data->sensor_->resolution();
> -			pixelFormat = rawPipeFormat->first;
> -
> -			streamFormats[pixelFormat] = { { kMinISISize, size } };
> -			isRaw = true;
> +			break;
>  
> +		case StreamRole::Raw: {
> +			cfg = generateRawConfiguration(camera);
>  			break;
>  		}
>  

Would this be clearer ?

		switch (role) {
		case StreamRole::StillCapture:
		case StreamRole::Viewfinder:
		case StreamRole::VideoRecording:
			Size size = role == StreamRole::StillCapture
				  ? data->sensor_->resolution()
				  : PipelineHandlerISI::kPreviewSize;
			cfg = generateYUVConfiguration(camera, size);
			if (cfg.pixelFormat.isValid())
				break;

			/*
			 * Fallback to use a Bayer format if that's what the
			 * sensor supports.
			 */
			[[fallthrough]]

		case StreamRole::Raw: {
			cfg = generateRawConfiguration(camera);
			break;
		}

Up to you.

> @@ -803,26 +896,12 @@ PipelineHandlerISI::generateConfiguration(Camera *camera,
>  			return nullptr;
>  		}
>  
> -		/*
> -		 * For non-RAW configurations the ISI can perform colorspace
> -		 * conversion. List all the supported output formats here.
> -		 */
> -		if (!isRaw) {
> -			for (const auto &[pixFmt, pipeFmt] : ISICameraConfiguration::formatsMap_) {
> -				const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);
> -				if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
> -					continue;
> -
> -				streamFormats[pixFmt] = { { kMinISISize, size } };
> -			}
> +		if (!cfg.pixelFormat.isValid()) {
> +			LOG(ISI, Error)
> +				<< "Cannot generate configuration for role: " << role;
> +			return nullptr;
>  		}
>  
> -		StreamFormats formats(streamFormats);
> -
> -		StreamConfiguration cfg(formats);
> -		cfg.pixelFormat = pixelFormat;
> -		cfg.size = size;
> -		cfg.bufferCount = 4;
>  		config->addConfiguration(cfg);
>  	}
>
Jacopo Mondi April 26, 2023, 1 p.m. UTC | #2 
Hi Laurent

On Mon, Apr 24, 2023 at 09:06:44AM +0300, Laurent Pinchart via libcamera-devel wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, Mar 13, 2023 at 10:19:43AM +0100, Jacopo Mondi wrote:
> > At generateConfiguration() a YUV/RGB pixel format is preferred for the
> > StillCapture/VideoRecording/Viewfinder roles, but currently there are no
> > guarantees in place that the sensor provides a non-Bayer bus format from
> > which YUV/RGB can be generated.
> >
> > This makes the default configuration generated for those roles not to
> > work if the sensor is a RAW-only one.
> >
> > To improve the situation split the configuration generation in two,
> > one for YUV modes and one for Raw Bayer mode.
> >
> > StreamRoles assigned to a YUV mode will try to first generate a YUV
> > configuration and then fallback to RAW if that's what the sensor can
> > provide.
> >
> > As an additional requirement, for YUV streams, the generated mode has to
> > be validated with the sensor to confirm the desired sizes can be
> > generated. In order to test a format use the newly introduced
> > CameraSensor::tryFormat().
>
> Please explain in the commit message (and/or a comment in the code) why
> this is needed.
>

What do you mean ?

"for YUV streams, the generated mode has to be validated with the sensor
to confirm the desired sizes can be generated."

The ISI cannot upscale, what's controversial here ?

> I'm also tempted to ask for this new requirement to be split to a
> separate patch, to ease review.
>

It's not a new requirement, it was simply ignored by the existing and
known-to-be-broken implementation


> > Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> > Tested-by: Daniel Scally <dan.scally@ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 231 +++++++++++++------
> >  1 file changed, 155 insertions(+), 76 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > index 146ba7465012..a4f437f55b26 100644
> > --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > @@ -145,6 +145,10 @@ private:
> >
> >  	Pipe *pipeFromStream(Camera *camera, const Stream *stream);
> >
> > +	StreamConfiguration generateYUVConfiguration(Camera *camera,
> > +						     const Size &size);
> > +	StreamConfiguration generateRawConfiguration(Camera *camera);
> > +
> >  	void bufferReady(FrameBuffer *buffer);
> >
> >  	MediaDevice *isiDev_;
> > @@ -705,6 +709,129 @@ PipelineHandlerISI::PipelineHandlerISI(CameraManager *manager)
> >  {
> >  }
> >
> > +/*
> > + * Generate a StreamConfiguration for YUV/RGB use case.
> > + *
> > + * Verify it the sensor can produce a YUV/RGB media bus format and collect
> > + * all the processed pixel formats the ISI can generate as supported stream
> > + * configurations.
> > + */
> > +StreamConfiguration PipelineHandlerISI::generateYUVConfiguration(Camera *camera,
> > +								 const Size &size)
> > +{
> > +	ISICameraData *data = cameraData(camera);
> > +	PixelFormat pixelFormat = formats::YUYV;
> > +	unsigned int mbusCode;
> > +
> > +	mbusCode = data->getYuvMediaBusFormat(&pixelFormat);
> > +	if (!mbusCode)
> > +		return {};
> > +
> > +	/* Adjust the requested size to the sensor's capabilities. */
> > +	const CameraSensor *sensor = data->sensor_.get();
> > +
> > +	V4L2SubdeviceFormat sensorFmt;
> > +	sensorFmt.mbus_code = mbusCode;
> > +	sensorFmt.size = size;
> > +
> > +	int ret = sensor->tryFormat(&sensorFmt);
>
> 	int ret = data->sensor_->tryFormat(&sensorFmt);
>
> and drop the local sensor variable.
>
> > +	if (ret) {
> > +		LOG(ISI, Error) << "Failed to try sensor format.";
> > +		return {};
> > +	}
> > +
> > +	Size sensorSize = sensorFmt.size;
> > +
> > +	/*
> > +	 * Populate the StreamConfiguration.
> > +	 *
> > +	 * As the sensor supports at least one YUV/RGB media bus format all the
> > +	 * processed ones in formatsMap_ can be generated from it.
> > +	 */
> > +	std::map<PixelFormat, std::vector<SizeRange>> streamFormats;
> > +
> > +	for (const auto &[pixFmt, pipeFmt] : ISICameraConfiguration::formatsMap_) {
> > +		const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);
> > +		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
> > +			continue;
> > +
> > +		streamFormats[pixFmt] = { { kMinISISize, sensorSize } };
> > +	}
> > +
> > +	StreamFormats formats(streamFormats);
> > +
> > +	StreamConfiguration cfg(formats);
> > +	cfg.pixelFormat = pixelFormat;
> > +	cfg.size = sensorSize;
> > +	cfg.bufferCount = 4;
> > +
> > +	return cfg;
> > +}
> > +
> > +/*
> > + * Generate a StreamConfiguration for Raw Bayer use case. Verify if the sensor
> > + * can produce the requested RAW bayer format and eventually adjust it to
> > + * the one with the largest bit-depth the sensor can produce.
> > + */
> > +StreamConfiguration PipelineHandlerISI::generateRawConfiguration(Camera *camera)
> > +{
> > +	static const std::map<unsigned int, PixelFormat> rawFormats = {
> > +		{ MEDIA_BUS_FMT_SBGGR8_1X8, formats::SBGGR8 },
> > +		{ MEDIA_BUS_FMT_SGBRG8_1X8, formats::SGBRG8 },
> > +		{ MEDIA_BUS_FMT_SGRBG8_1X8, formats::SGRBG8 },
> > +		{ MEDIA_BUS_FMT_SRGGB8_1X8, formats::SRGGB8 },
> > +		{ MEDIA_BUS_FMT_SBGGR10_1X10, formats::SBGGR10 },
> > +		{ MEDIA_BUS_FMT_SGBRG10_1X10, formats::SGBRG10 },
> > +		{ MEDIA_BUS_FMT_SGRBG10_1X10, formats::SGRBG10 },
> > +		{ MEDIA_BUS_FMT_SRGGB10_1X10, formats::SRGGB10 },
> > +		{ MEDIA_BUS_FMT_SBGGR12_1X12, formats::SBGGR12 },
> > +		{ MEDIA_BUS_FMT_SGBRG12_1X12, formats::SGBRG12 },
> > +		{ MEDIA_BUS_FMT_SGRBG12_1X12, formats::SGRBG12 },
> > +		{ MEDIA_BUS_FMT_SRGGB12_1X12, formats::SRGGB12 },
>
> Now that libcamera has RAW14 formats, could you add them here ?
>
> > +	};
> > +
> > +	ISICameraData *data = cameraData(camera);
> > +	PixelFormat pixelFormat = formats::SBGGR10;
> > +	unsigned int mbusCode;
> > +
> > +	/* pixelFormat will be adjusted, if the sensor can produce RAW. */
> > +	mbusCode = data->getRawMediaBusFormat(&pixelFormat);
> > +	if (!mbusCode)
> > +		return {};
> > +
> > +	/*
> > +	 * Populate the StreamConfiguration with all the supported Bayer
> > +	 * formats the sensor can produce.
> > +	 */
> > +	std::map<PixelFormat, std::vector<SizeRange>> streamFormats;
> > +	const CameraSensor *sensor = data->sensor_.get();
> > +
> > +	for (unsigned int code : sensor->mbusCodes()) {
> > +		/* Find a Bayer media bus code from the sensor. */
> > +		const BayerFormat &bayerFormat = BayerFormat::fromMbusCode(code);
> > +		if (!bayerFormat.isValid())
> > +			continue;
> > +
> > +		auto it = rawFormats.find(code);
> > +		if (it == rawFormats.end()) {
> > +			LOG(ISI, Warning) << bayerFormat
> > +					  << " not supported in ISI formats map.";
> > +			continue;
> > +		}
> > +
> > +		streamFormats[it->second] = { { sensor->resolution(), sensor->resolution() } };
> > +	}
> > +
> > +	StreamFormats formats(streamFormats);
> > +
> > +	StreamConfiguration cfg(formats);
> > +	cfg.size = sensor->resolution();
> > +	cfg.pixelFormat = pixelFormat;
> > +	cfg.bufferCount = 4;
> > +
> > +	return cfg;
> > +}
> > +
> >  std::unique_ptr<CameraConfiguration>
> >  PipelineHandlerISI::generateConfiguration(Camera *camera,
> >  					  const StreamRoles &roles)
> > @@ -722,79 +849,45 @@ PipelineHandlerISI::generateConfiguration(Camera *camera,
> >  		return nullptr;
> >  	}
> >
> > -	bool isRaw = false;
> >  	for (const auto &role : roles) {
> >  		/*
> > -		 * Prefer the following formats
> > +		 * Prefer the following formats:
> >  		 * - Still Capture: Full resolution YUYV
> >  		 * - ViewFinder/VideoRecording: 1080p YUYV
> > -		 * - RAW: sensor's native format and resolution
> > +		 * - RAW: Full resolution Bayer
> >  		 */
> > -		std::map<PixelFormat, std::vector<SizeRange>> streamFormats;
> > -		PixelFormat pixelFormat;
> > -		Size size;
> > +		StreamConfiguration cfg;
> >
> >  		switch (role) {
> >  		case StreamRole::StillCapture:
> > -			/*
> > -			 * \todo Make sure the sensor can produce non-RAW formats
> > -			 * compatible with the ones supported by the pipeline.
> > -			 */
> > -			size = data->sensor_->resolution();
> > -			pixelFormat = formats::YUYV;
> > +			cfg = generateYUVConfiguration(camera,
> > +						       data->sensor_->resolution());
> > +			if (!cfg.pixelFormat.isValid()) {
> > +				/*
> > +				 * Fallback to use a Bayer format if that's what
> > +				 * the sensor supports.
> > +				 */
> > +				cfg = generateRawConfiguration(camera);
> > +			}
> > +
> >  			break;
> >
> >  		case StreamRole::Viewfinder:
> >  		case StreamRole::VideoRecording:
> > -			/*
> > -			 * \todo Make sure the sensor can produce non-RAW formats
> > -			 * compatible with the ones supported by the pipeline.
> > -			 */
> > -			size = PipelineHandlerISI::kPreviewSize;
> > -			pixelFormat = formats::YUYV;
> > -			break;
> > -
> > -		case StreamRole::Raw: {
> > -			/*
> > -			 * Make sure the sensor can generate a RAW format and
> > -			 * prefer the ones with a larger bitdepth.
> > -			 */
> > -			const ISICameraConfiguration::FormatMap::value_type *rawPipeFormat = nullptr;
> > -			unsigned int maxDepth = 0;
> > -
> > -			for (unsigned int code : data->sensor_->mbusCodes()) {
> > -				const BayerFormat &bayerFormat = BayerFormat::fromMbusCode(code);
> > -				if (!bayerFormat.isValid())
> > -					continue;
> > -
> > -				/* Make sure the format is supported by the pipeline handler. */
> > -				auto it = std::find_if(ISICameraConfiguration::formatsMap_.begin(),
> > -						       ISICameraConfiguration::formatsMap_.end(),
> > -						       [code](auto &isiFormat) {
> > -							        auto &pipe = isiFormat.second;
> > -							        return pipe.sensorCode == code;
> > -						       });
> > -				if (it == ISICameraConfiguration::formatsMap_.end())
> > -					continue;
> > -
> > -				if (bayerFormat.bitDepth > maxDepth) {
> > -					maxDepth = bayerFormat.bitDepth;
> > -					rawPipeFormat = &(*it);
> > -				}
> > -			}
> > -
> > -			if (!rawPipeFormat) {
> > -				LOG(ISI, Error)
> > -					<< "Cannot generate a configuration for RAW stream";
> > -				return nullptr;
> > +			cfg = generateYUVConfiguration(camera,
> > +						       PipelineHandlerISI::kPreviewSize);
> > +			if (!cfg.pixelFormat.isValid()) {
> > +				/*
> > +				 * Fallback to use a Bayer format if that's what
> > +				 * the sensor supports.
> > +				 */
> > +				cfg = generateRawConfiguration(camera);
> >  			}
> >
> > -			size = data->sensor_->resolution();
> > -			pixelFormat = rawPipeFormat->first;
> > -
> > -			streamFormats[pixelFormat] = { { kMinISISize, size } };
> > -			isRaw = true;
> > +			break;
> >
> > +		case StreamRole::Raw: {
> > +			cfg = generateRawConfiguration(camera);
> >  			break;
> >  		}
> >
>
> Would this be clearer ?
>
> 		switch (role) {
> 		case StreamRole::StillCapture:
> 		case StreamRole::Viewfinder:
> 		case StreamRole::VideoRecording:
> 			Size size = role == StreamRole::StillCapture
> 				  ? data->sensor_->resolution()
> 				  : PipelineHandlerISI::kPreviewSize;
> 			cfg = generateYUVConfiguration(camera, size);
> 			if (cfg.pixelFormat.isValid())
> 				break;
>
> 			/*
> 			 * Fallback to use a Bayer format if that's what the
> 			 * sensor supports.
> 			 */
> 			[[fallthrough]]
>
> 		case StreamRole::Raw: {
> 			cfg = generateRawConfiguration(camera);
> 			break;
> 		}
>
> Up to you.
>
> > @@ -803,26 +896,12 @@ PipelineHandlerISI::generateConfiguration(Camera *camera,
> >  			return nullptr;
> >  		}
> >
> > -		/*
> > -		 * For non-RAW configurations the ISI can perform colorspace
> > -		 * conversion. List all the supported output formats here.
> > -		 */
> > -		if (!isRaw) {
> > -			for (const auto &[pixFmt, pipeFmt] : ISICameraConfiguration::formatsMap_) {
> > -				const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);
> > -				if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
> > -					continue;
> > -
> > -				streamFormats[pixFmt] = { { kMinISISize, size } };
> > -			}
> > +		if (!cfg.pixelFormat.isValid()) {
> > +			LOG(ISI, Error)
> > +				<< "Cannot generate configuration for role: " << role;
> > +			return nullptr;
> >  		}
> >
> > -		StreamFormats formats(streamFormats);
> > -
> > -		StreamConfiguration cfg(formats);
> > -		cfg.pixelFormat = pixelFormat;
> > -		cfg.size = size;
> > -		cfg.bufferCount = 4;
> >  		config->addConfiguration(cfg);
> >  	}
> >
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
index 146ba7465012..a4f437f55b26 100644
--- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
+++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
@@ -145,6 +145,10 @@  private:
 
 	Pipe *pipeFromStream(Camera *camera, const Stream *stream);
 
+	StreamConfiguration generateYUVConfiguration(Camera *camera,
+						     const Size &size);
+	StreamConfiguration generateRawConfiguration(Camera *camera);
+
 	void bufferReady(FrameBuffer *buffer);
 
 	MediaDevice *isiDev_;
@@ -705,6 +709,129 @@  PipelineHandlerISI::PipelineHandlerISI(CameraManager *manager)
 {
 }
 
+/*
+ * Generate a StreamConfiguration for YUV/RGB use case.
+ *
+ * Verify it the sensor can produce a YUV/RGB media bus format and collect
+ * all the processed pixel formats the ISI can generate as supported stream
+ * configurations.
+ */
+StreamConfiguration PipelineHandlerISI::generateYUVConfiguration(Camera *camera,
+								 const Size &size)
+{
+	ISICameraData *data = cameraData(camera);
+	PixelFormat pixelFormat = formats::YUYV;
+	unsigned int mbusCode;
+
+	mbusCode = data->getYuvMediaBusFormat(&pixelFormat);
+	if (!mbusCode)
+		return {};
+
+	/* Adjust the requested size to the sensor's capabilities. */
+	const CameraSensor *sensor = data->sensor_.get();
+
+	V4L2SubdeviceFormat sensorFmt;
+	sensorFmt.mbus_code = mbusCode;
+	sensorFmt.size = size;
+
+	int ret = sensor->tryFormat(&sensorFmt);
+	if (ret) {
+		LOG(ISI, Error) << "Failed to try sensor format.";
+		return {};
+	}
+
+	Size sensorSize = sensorFmt.size;
+
+	/*
+	 * Populate the StreamConfiguration.
+	 *
+	 * As the sensor supports at least one YUV/RGB media bus format all the
+	 * processed ones in formatsMap_ can be generated from it.
+	 */
+	std::map<PixelFormat, std::vector<SizeRange>> streamFormats;
+
+	for (const auto &[pixFmt, pipeFmt] : ISICameraConfiguration::formatsMap_) {
+		const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);
+		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
+			continue;
+
+		streamFormats[pixFmt] = { { kMinISISize, sensorSize } };
+	}
+
+	StreamFormats formats(streamFormats);
+
+	StreamConfiguration cfg(formats);
+	cfg.pixelFormat = pixelFormat;
+	cfg.size = sensorSize;
+	cfg.bufferCount = 4;
+
+	return cfg;
+}
+
+/*
+ * Generate a StreamConfiguration for Raw Bayer use case. Verify if the sensor
+ * can produce the requested RAW bayer format and eventually adjust it to
+ * the one with the largest bit-depth the sensor can produce.
+ */
+StreamConfiguration PipelineHandlerISI::generateRawConfiguration(Camera *camera)
+{
+	static const std::map<unsigned int, PixelFormat> rawFormats = {
+		{ MEDIA_BUS_FMT_SBGGR8_1X8, formats::SBGGR8 },
+		{ MEDIA_BUS_FMT_SGBRG8_1X8, formats::SGBRG8 },
+		{ MEDIA_BUS_FMT_SGRBG8_1X8, formats::SGRBG8 },
+		{ MEDIA_BUS_FMT_SRGGB8_1X8, formats::SRGGB8 },
+		{ MEDIA_BUS_FMT_SBGGR10_1X10, formats::SBGGR10 },
+		{ MEDIA_BUS_FMT_SGBRG10_1X10, formats::SGBRG10 },
+		{ MEDIA_BUS_FMT_SGRBG10_1X10, formats::SGRBG10 },
+		{ MEDIA_BUS_FMT_SRGGB10_1X10, formats::SRGGB10 },
+		{ MEDIA_BUS_FMT_SBGGR12_1X12, formats::SBGGR12 },
+		{ MEDIA_BUS_FMT_SGBRG12_1X12, formats::SGBRG12 },
+		{ MEDIA_BUS_FMT_SGRBG12_1X12, formats::SGRBG12 },
+		{ MEDIA_BUS_FMT_SRGGB12_1X12, formats::SRGGB12 },
+	};
+
+	ISICameraData *data = cameraData(camera);
+	PixelFormat pixelFormat = formats::SBGGR10;
+	unsigned int mbusCode;
+
+	/* pixelFormat will be adjusted, if the sensor can produce RAW. */
+	mbusCode = data->getRawMediaBusFormat(&pixelFormat);
+	if (!mbusCode)
+		return {};
+
+	/*
+	 * Populate the StreamConfiguration with all the supported Bayer
+	 * formats the sensor can produce.
+	 */
+	std::map<PixelFormat, std::vector<SizeRange>> streamFormats;
+	const CameraSensor *sensor = data->sensor_.get();
+
+	for (unsigned int code : sensor->mbusCodes()) {
+		/* Find a Bayer media bus code from the sensor. */
+		const BayerFormat &bayerFormat = BayerFormat::fromMbusCode(code);
+		if (!bayerFormat.isValid())
+			continue;
+
+		auto it = rawFormats.find(code);
+		if (it == rawFormats.end()) {
+			LOG(ISI, Warning) << bayerFormat
+					  << " not supported in ISI formats map.";
+			continue;
+		}
+
+		streamFormats[it->second] = { { sensor->resolution(), sensor->resolution() } };
+	}
+
+	StreamFormats formats(streamFormats);
+
+	StreamConfiguration cfg(formats);
+	cfg.size = sensor->resolution();
+	cfg.pixelFormat = pixelFormat;
+	cfg.bufferCount = 4;
+
+	return cfg;
+}
+
 std::unique_ptr<CameraConfiguration>
 PipelineHandlerISI::generateConfiguration(Camera *camera,
 					  const StreamRoles &roles)
@@ -722,79 +849,45 @@  PipelineHandlerISI::generateConfiguration(Camera *camera,
 		return nullptr;
 	}
 
-	bool isRaw = false;
 	for (const auto &role : roles) {
 		/*
-		 * Prefer the following formats
+		 * Prefer the following formats:
 		 * - Still Capture: Full resolution YUYV
 		 * - ViewFinder/VideoRecording: 1080p YUYV
-		 * - RAW: sensor's native format and resolution
+		 * - RAW: Full resolution Bayer
 		 */
-		std::map<PixelFormat, std::vector<SizeRange>> streamFormats;
-		PixelFormat pixelFormat;
-		Size size;
+		StreamConfiguration cfg;
 
 		switch (role) {
 		case StreamRole::StillCapture:
-			/*
-			 * \todo Make sure the sensor can produce non-RAW formats
-			 * compatible with the ones supported by the pipeline.
-			 */
-			size = data->sensor_->resolution();
-			pixelFormat = formats::YUYV;
+			cfg = generateYUVConfiguration(camera,
+						       data->sensor_->resolution());
+			if (!cfg.pixelFormat.isValid()) {
+				/*
+				 * Fallback to use a Bayer format if that's what
+				 * the sensor supports.
+				 */
+				cfg = generateRawConfiguration(camera);
+			}
+
 			break;
 
 		case StreamRole::Viewfinder:
 		case StreamRole::VideoRecording:
-			/*
-			 * \todo Make sure the sensor can produce non-RAW formats
-			 * compatible with the ones supported by the pipeline.
-			 */
-			size = PipelineHandlerISI::kPreviewSize;
-			pixelFormat = formats::YUYV;
-			break;
-
-		case StreamRole::Raw: {
-			/*
-			 * Make sure the sensor can generate a RAW format and
-			 * prefer the ones with a larger bitdepth.
-			 */
-			const ISICameraConfiguration::FormatMap::value_type *rawPipeFormat = nullptr;
-			unsigned int maxDepth = 0;
-
-			for (unsigned int code : data->sensor_->mbusCodes()) {
-				const BayerFormat &bayerFormat = BayerFormat::fromMbusCode(code);
-				if (!bayerFormat.isValid())
-					continue;
-
-				/* Make sure the format is supported by the pipeline handler. */
-				auto it = std::find_if(ISICameraConfiguration::formatsMap_.begin(),
-						       ISICameraConfiguration::formatsMap_.end(),
-						       [code](auto &isiFormat) {
-							        auto &pipe = isiFormat.second;
-							        return pipe.sensorCode == code;
-						       });
-				if (it == ISICameraConfiguration::formatsMap_.end())
-					continue;
-
-				if (bayerFormat.bitDepth > maxDepth) {
-					maxDepth = bayerFormat.bitDepth;
-					rawPipeFormat = &(*it);
-				}
-			}
-
-			if (!rawPipeFormat) {
-				LOG(ISI, Error)
-					<< "Cannot generate a configuration for RAW stream";
-				return nullptr;
+			cfg = generateYUVConfiguration(camera,
+						       PipelineHandlerISI::kPreviewSize);
+			if (!cfg.pixelFormat.isValid()) {
+				/*
+				 * Fallback to use a Bayer format if that's what
+				 * the sensor supports.
+				 */
+				cfg = generateRawConfiguration(camera);
 			}
 
-			size = data->sensor_->resolution();
-			pixelFormat = rawPipeFormat->first;
-
-			streamFormats[pixelFormat] = { { kMinISISize, size } };
-			isRaw = true;
+			break;
 
+		case StreamRole::Raw: {
+			cfg = generateRawConfiguration(camera);
 			break;
 		}
 
@@ -803,26 +896,12 @@  PipelineHandlerISI::generateConfiguration(Camera *camera,
 			return nullptr;
 		}
 
-		/*
-		 * For non-RAW configurations the ISI can perform colorspace
-		 * conversion. List all the supported output formats here.
-		 */
-		if (!isRaw) {
-			for (const auto &[pixFmt, pipeFmt] : ISICameraConfiguration::formatsMap_) {
-				const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);
-				if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
-					continue;
-
-				streamFormats[pixFmt] = { { kMinISISize, size } };
-			}
+		if (!cfg.pixelFormat.isValid()) {
+			LOG(ISI, Error)
+				<< "Cannot generate configuration for role: " << role;
+			return nullptr;
 		}
 
-		StreamFormats formats(streamFormats);
-
-		StreamConfiguration cfg(formats);
-		cfg.pixelFormat = pixelFormat;
-		cfg.size = size;
-		cfg.bufferCount = 4;
 		config->addConfiguration(cfg);
 	}