[libcamera-devel,v3,07/10] libcamera: ipu3: cio2: Add function to generate configuration
diff mbox series

Message ID 20200623020930.1781469-8-niklas.soderlund@ragnatech.se
State Superseded
Headers show
Series
  • libcamera: ipu3: Allow zero-copy RAW stream
Related show

Commit Message

Niklas Söderlund June 23, 2020, 2:09 a.m. UTC 
Collect the code used to generate configurations for the CIO2 block in
the CIO2Device class. This allows simplifying the code and allow further
changes to only  happen at one code location.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
* Changes since v2
- Remove unneeded code to pick sensor FourCC.
- Remove desiredPixelFormat from generateConfiguration() as it's not
  needed.
- Rename sensorFormat_ cio2Configuration_
- Consolidate all format information in a single table.

* Changes since v1
- Use anonymous namespace instead of static for sensorMbusToPixel map.
- Handle case where the requested mbus code is not supported by the sensor.
- Update commit message.
---
 src/libcamera/pipeline/ipu3/cio2.cpp | 54 +++++++++++++++++++++++----
 src/libcamera/pipeline/ipu3/cio2.h   |  3 ++
 src/libcamera/pipeline/ipu3/ipu3.cpp | 56 +++++++---------------------
 3 files changed, 63 insertions(+), 50 deletions(-)

Comments

Jacopo Mondi June 24, 2020, 10:22 a.m. UTC | #1 
Hi Niklas,

On Tue, Jun 23, 2020 at 04:09:27AM +0200, Niklas Söderlund wrote:
> Collect the code used to generate configurations for the CIO2 block in
> the CIO2Device class. This allows simplifying the code and allow further
> changes to only  happen at one code location.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
> * Changes since v2
> - Remove unneeded code to pick sensor FourCC.
> - Remove desiredPixelFormat from generateConfiguration() as it's not
>   needed.
> - Rename sensorFormat_ cio2Configuration_
> - Consolidate all format information in a single table.
>
> * Changes since v1
> - Use anonymous namespace instead of static for sensorMbusToPixel map.
> - Handle case where the requested mbus code is not supported by the sensor.
> - Update commit message.
> ---
>  src/libcamera/pipeline/ipu3/cio2.cpp | 54 +++++++++++++++++++++++----
>  src/libcamera/pipeline/ipu3/cio2.h   |  3 ++
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 56 +++++++---------------------
>  3 files changed, 63 insertions(+), 50 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index f23128d412e6b1a5..d6bab896706dd60e 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> @@ -9,6 +9,9 @@
>
>  #include <linux/media-bus-format.h>
>
> +#include <libcamera/formats.h>
> +#include <libcamera/stream.h>
> +
>  #include "libcamera/internal/camera_sensor.h"
>  #include "libcamera/internal/media_device.h"
>  #include "libcamera/internal/v4l2_subdevice.h"
> @@ -20,11 +23,16 @@ LOG_DECLARE_CATEGORY(IPU3)
>
>  namespace {
>
> -static const std::map<uint32_t, V4L2PixelFormat> mbusCodesToInfo = {
> -	{ MEDIA_BUS_FMT_SBGGR10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10) },
> -	{ MEDIA_BUS_FMT_SGBRG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10) },
> -	{ MEDIA_BUS_FMT_SGRBG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10) },
> -	{ MEDIA_BUS_FMT_SRGGB10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10) },
> +struct MbusInfo {
> +	PixelFormat pixelFormat;
> +	V4L2PixelFormat fourcc;
> +};
> +
> +static const std::map<uint32_t, MbusInfo> mbusCodesToInfo = {
> +	{ MEDIA_BUS_FMT_SBGGR10_1X10, { formats::SBGGR10_IPU3, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10) } },
> +	{ MEDIA_BUS_FMT_SGBRG10_1X10, { formats::SGBRG10_IPU3, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10) } },
> +	{ MEDIA_BUS_FMT_SGRBG10_1X10, { formats::SGRBG10_IPU3, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10) } },
> +	{ MEDIA_BUS_FMT_SRGGB10_1X10, { formats::SRGGB10_IPU3, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10) } },
>  };
>
>  } /* namespace */
> @@ -92,7 +100,7 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
>  	std::set<unsigned int> cio2Codes;
>  	std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),
>  		std::inserter(cio2Codes, cio2Codes.begin()),
> -		[](const std::map<uint32_t, V4L2PixelFormat>::value_type &pair){ return pair.first; });
> +		[](const std::map<uint32_t, MbusInfo>::value_type &pair){ return pair.first; });
>  	const std::set<unsigned int> &sensorCodes = sensor_->mbusCodes();
>  	if (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(),
>  				cio2Codes.begin(), cio2Codes.end())) {
> @@ -139,7 +147,7 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)
>  	std::vector<unsigned int> mbusCodes;
>  	std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),
>  		std::back_inserter(mbusCodes),
> -		[](const std::map<uint32_t, V4L2PixelFormat>::value_type &pair){ return pair.first; });
> +		[](const std::map<uint32_t, MbusInfo>::value_type &pair){ return pair.first; });
>
>  	sensorFormat = sensor_->getFormat(mbusCodes, size);
>  	ret = sensor_->setFormat(&sensorFormat);
> @@ -154,7 +162,7 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)
>  	if (itInfo == mbusCodesToInfo.end())
>  		return -EINVAL;
>
> -	outputFormat->fourcc = itInfo->second;
> +	outputFormat->fourcc = itInfo->second.fourcc;
>  	outputFormat->size = sensorFormat.size;
>  	outputFormat->planesCount = 1;
>
> @@ -167,6 +175,36 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)
>  	return 0;
>  }
>
> +StreamConfiguration
> +CIO2Device::generateConfiguration(const Size &desiredSize) const
> +{
> +	StreamConfiguration cfg;
> +
> +	/* If no desired size use the sensor resolution. */
> +	Size size = sensor_->resolution();
> +	if (desiredSize.width && desiredSize.height)
> +		size = desiredSize;
> +
> +	/* Query the sensor static information for closest match. */
> +	std::vector<unsigned int> mbusCodes;
> +	std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),
> +		std::back_inserter(mbusCodes),
> +		[](const std::map<uint32_t, MbusInfo>::value_type &pair){ return pair.first; });

Oh, one more :(

What about a CIO2Device::mbusCodes() helper for this ? With C++ copy
elision you would not get any performance penality

> +
> +	V4L2SubdeviceFormat sensorFormat = sensor_->getFormat(mbusCodes, size);
> +

stray empty line

The rest is good. I think we're abusing StreamConfiguration, as it is
supposed to report StreamFormats, but it's optional, and we use it
internally to transport plaing "formats" between components. You could
even use an ImageFormat if my series ever gets any feedback, but I
won't block this one because of this. But really, we need to fix this
as it's getting confusing enough :)

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

Thanks
  j

> +	if (!sensorFormat.mbus_code) {
> +		LOG(IPU3, Error) << "Sensor does not support mbus code";
> +		return {};
> +	}
> +
> +	cfg.size = sensorFormat.size;
> +	cfg.pixelFormat = mbusCodesToInfo.at(sensorFormat.mbus_code).pixelFormat;
> +	cfg.bufferCount = CIO2_BUFFER_COUNT;
> +
> +	return cfg;
> +}
> +
>  /**
>   * \brief Allocate frame buffers for the CIO2 output
>   *
> diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
> index b2c4f89d682d6cfb..6276573f2b585b25 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.h
> +++ b/src/libcamera/pipeline/ipu3/cio2.h
> @@ -20,6 +20,7 @@ class V4L2DeviceFormat;
>  class V4L2Subdevice;
>  class V4L2VideoDevice;
>  struct Size;
> +struct StreamConfiguration;
>
>  class CIO2Device
>  {
> @@ -32,6 +33,8 @@ public:
>  	int init(const MediaDevice *media, unsigned int index);
>  	int configure(const Size &size, V4L2DeviceFormat *outputFormat);
>
> +	StreamConfiguration generateConfiguration(const Size &desiredSize) const;
> +
>  	int allocateBuffers();
>  	void freeBuffers();
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 6e5eb5609a3c2388..c0e727e592f46883 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -36,13 +36,6 @@ LOG_DEFINE_CATEGORY(IPU3)
>
>  class IPU3CameraData;
>
> -static const std::map<uint32_t, PixelFormat> sensorMbusToPixel = {
> -	{ MEDIA_BUS_FMT_SBGGR10_1X10, formats::SBGGR10_IPU3 },
> -	{ MEDIA_BUS_FMT_SGBRG10_1X10, formats::SGBRG10_IPU3 },
> -	{ MEDIA_BUS_FMT_SGRBG10_1X10, formats::SGRBG10_IPU3 },
> -	{ MEDIA_BUS_FMT_SRGGB10_1X10, formats::SRGGB10_IPU3 },
> -};
> -
>  class ImgUDevice
>  {
>  public:
> @@ -147,7 +140,7 @@ public:
>
>  	Status validate() override;
>
> -	const V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }
> +	const StreamConfiguration &cio2Format() const { return cio2Configuration_; };
>  	const std::vector<const IPU3Stream *> &streams() { return streams_; }
>
>  private:
> @@ -165,7 +158,7 @@ private:
>  	std::shared_ptr<Camera> camera_;
>  	const IPU3CameraData *data_;
>
> -	V4L2SubdeviceFormat sensorFormat_;
> +	StreamConfiguration cio2Configuration_;
>  	std::vector<const IPU3Stream *> streams_;
>  };
>
> @@ -252,7 +245,7 @@ void IPU3CameraConfiguration::assignStreams()
>
>  		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
>  			stream = &data_->rawStream_;
> -		else if (cfg.size == sensorFormat_.size)
> +		else if (cfg.size == cio2Configuration_.size)
>  			stream = &data_->outStream_;
>  		else
>  			stream = &data_->vfStream_;
> @@ -277,8 +270,8 @@ void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)
>  		 */
>  		if (!cfg.size.width || !cfg.size.height) {
>  			cfg.size.width = 1280;
> -			cfg.size.height = 1280 * sensorFormat_.size.height
> -					/ sensorFormat_.size.width;
> +			cfg.size.height = 1280 * cio2Configuration_.size.height
> +					/ cio2Configuration_.size.width;
>  		}
>
>  		/*
> @@ -297,7 +290,7 @@ void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)
>  		 * \todo: Properly support cropping when the ImgU driver
>  		 * interface will be cleaned up.
>  		 */
> -		cfg.size = sensorFormat_.size;
> +		cfg.size = cio2Configuration_.size;
>  	}
>
>  	/*
> @@ -313,7 +306,6 @@ void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)
>
>  CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  {
> -	const CameraSensor *sensor = data_->cio2_.sensor_;
>  	Status status = Valid;
>
>  	if (config_.empty())
> @@ -340,16 +332,10 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  			size.height = cfg.size.height;
>  	}
>
> -	if (!size.width || !size.height)
> -		size = sensor->resolution();
> -
> -	sensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10,
> -					    MEDIA_BUS_FMT_SGBRG10_1X10,
> -					    MEDIA_BUS_FMT_SGRBG10_1X10,
> -					    MEDIA_BUS_FMT_SRGGB10_1X10 },
> -					  size);
> -	if (!sensorFormat_.size.width || !sensorFormat_.size.height)
> -		sensorFormat_.size = sensor->resolution();
> +	/* Generate raw configuration from CIO2. */
> +	cio2Configuration_ = data_->cio2_.generateConfiguration(size);
> +	if (!cio2Configuration_.pixelFormat.isValid())
> +		return Invalid;
>
>  	/* Assign streams to each configuration entry. */
>  	assignStreams();
> @@ -361,20 +347,13 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  		const IPU3Stream *stream = streams_[i];
>
>  		if (stream->raw_) {
> -			const auto &itFormat =
> -				sensorMbusToPixel.find(sensorFormat_.mbus_code);
> -			if (itFormat == sensorMbusToPixel.end())
> -				return Invalid;
> -
> -			cfg.pixelFormat = itFormat->second;
> -			cfg.size = sensorFormat_.size;
> +			cfg = cio2Configuration_;
>  		} else {
>  			bool scale = stream == &data_->vfStream_;
>  			adjustStream(config_[i], scale);
> +			cfg.bufferCount = IPU3_BUFFER_COUNT;
>  		}
>
> -		cfg.bufferCount = IPU3_BUFFER_COUNT;
> -
>  		if (cfg.pixelFormat != oldCfg.pixelFormat ||
>  		    cfg.size != oldCfg.size) {
>  			LOG(IPU3, Debug)
> @@ -454,14 +433,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
>
>  			cfg.size = data->cio2_.sensor_->resolution();
>
> -			V4L2SubdeviceFormat sensorFormat =
> -				data->cio2_.sensor_->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10,
> -								 MEDIA_BUS_FMT_SGBRG10_1X10,
> -								 MEDIA_BUS_FMT_SGRBG10_1X10,
> -								 MEDIA_BUS_FMT_SRGGB10_1X10 },
> -							       cfg.size);
> -			cfg.pixelFormat =
> -				sensorMbusToPixel.at(sensorFormat.mbus_code);
> +			cfg = data->cio2_.generateConfiguration(cfg.size);
>  			break;
>  		}
>
> @@ -575,7 +547,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>  	 * Pass the requested stream size to the CIO2 unit and get back the
>  	 * adjusted format to be propagated to the ImgU output devices.
>  	 */
> -	const Size &sensorSize = config->sensorFormat().size;
> +	const Size &sensorSize = config->cio2Format().size;
>  	V4L2DeviceFormat cio2Format = {};
>  	ret = cio2->configure(sensorSize, &cio2Format);
>  	if (ret)
> --
> 2.27.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart June 25, 2020, 1:23 a.m. UTC | #2 
Hi Niklas,

Thank you for the patch.

On Tue, Jun 23, 2020 at 04:09:27AM +0200, Niklas Söderlund wrote:
> Collect the code used to generate configurations for the CIO2 block in
> the CIO2Device class. This allows simplifying the code and allow further
> changes to only  happen at one code location.

s/  / /

> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
> * Changes since v2
> - Remove unneeded code to pick sensor FourCC.
> - Remove desiredPixelFormat from generateConfiguration() as it's not
>   needed.
> - Rename sensorFormat_ cio2Configuration_
> - Consolidate all format information in a single table.
> 
> * Changes since v1
> - Use anonymous namespace instead of static for sensorMbusToPixel map.
> - Handle case where the requested mbus code is not supported by the sensor.
> - Update commit message.
> ---
>  src/libcamera/pipeline/ipu3/cio2.cpp | 54 +++++++++++++++++++++++----
>  src/libcamera/pipeline/ipu3/cio2.h   |  3 ++
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 56 +++++++---------------------
>  3 files changed, 63 insertions(+), 50 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index f23128d412e6b1a5..d6bab896706dd60e 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> @@ -9,6 +9,9 @@
>  
>  #include <linux/media-bus-format.h>
>  
> +#include <libcamera/formats.h>
> +#include <libcamera/stream.h>
> +
>  #include "libcamera/internal/camera_sensor.h"
>  #include "libcamera/internal/media_device.h"
>  #include "libcamera/internal/v4l2_subdevice.h"
> @@ -20,11 +23,16 @@ LOG_DECLARE_CATEGORY(IPU3)
>  
>  namespace {
>  
> -static const std::map<uint32_t, V4L2PixelFormat> mbusCodesToInfo = {
> -	{ MEDIA_BUS_FMT_SBGGR10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10) },
> -	{ MEDIA_BUS_FMT_SGBRG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10) },
> -	{ MEDIA_BUS_FMT_SGRBG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10) },
> -	{ MEDIA_BUS_FMT_SRGGB10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10) },
> +struct MbusInfo {
> +	PixelFormat pixelFormat;
> +	V4L2PixelFormat fourcc;
> +};
> +
> +static const std::map<uint32_t, MbusInfo> mbusCodesToInfo = {
> +	{ MEDIA_BUS_FMT_SBGGR10_1X10, { formats::SBGGR10_IPU3, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10) } },
> +	{ MEDIA_BUS_FMT_SGBRG10_1X10, { formats::SGBRG10_IPU3, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10) } },
> +	{ MEDIA_BUS_FMT_SGRBG10_1X10, { formats::SGRBG10_IPU3, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10) } },
> +	{ MEDIA_BUS_FMT_SRGGB10_1X10, { formats::SRGGB10_IPU3, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10) } },
>  };

Would it make sense to keep this as-is, and use PixelFormatInfo::info()
to look up additional information (the V4L2 pixel format in this case,
but potentially more in the future) ? Otherwise we end up duplicating
format information in multiple locations.

>  
>  } /* namespace */
> @@ -92,7 +100,7 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
>  	std::set<unsigned int> cio2Codes;
>  	std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),
>  		std::inserter(cio2Codes, cio2Codes.begin()),
> -		[](const std::map<uint32_t, V4L2PixelFormat>::value_type &pair){ return pair.first; });
> +		[](const std::map<uint32_t, MbusInfo>::value_type &pair){ return pair.first; });
>  	const std::set<unsigned int> &sensorCodes = sensor_->mbusCodes();
>  	if (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(),
>  				cio2Codes.begin(), cio2Codes.end())) {
> @@ -139,7 +147,7 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)
>  	std::vector<unsigned int> mbusCodes;
>  	std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),
>  		std::back_inserter(mbusCodes),
> -		[](const std::map<uint32_t, V4L2PixelFormat>::value_type &pair){ return pair.first; });
> +		[](const std::map<uint32_t, MbusInfo>::value_type &pair){ return pair.first; });
>  
>  	sensorFormat = sensor_->getFormat(mbusCodes, size);
>  	ret = sensor_->setFormat(&sensorFormat);
> @@ -154,7 +162,7 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)
>  	if (itInfo == mbusCodesToInfo.end())
>  		return -EINVAL;
>  
> -	outputFormat->fourcc = itInfo->second;
> +	outputFormat->fourcc = itInfo->second.fourcc;


This would become

	const PixelFormatInfo &info = PixelFormatInfo::info(itInfo->second);
	outputFormat->fourcc = info.v4l2Format;

>  	outputFormat->size = sensorFormat.size;
>  	outputFormat->planesCount = 1;
>  
> @@ -167,6 +175,36 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)
>  	return 0;
>  }
>  
> +StreamConfiguration
> +CIO2Device::generateConfiguration(const Size &desiredSize) const
> +{
> +	StreamConfiguration cfg;
> +
> +	/* If no desired size use the sensor resolution. */
> +	Size size = sensor_->resolution();
> +	if (desiredSize.width && desiredSize.height)
> +		size = desiredSize;

I've just sent a patch titled "PATCH] libcamera: geometry: Add isNull()
function to Size class". You could write this code

	Size size = !desiredSize.isNull() ? desiredSize : sensor_->resolution();

Or you could pass the desiredSize parameter by value instead of
reference, rename it to size, and just write

	/* If no desired size use the sensor resolution. */
	if (size.isNull())
		size = sensor_->resolution();

I think that would be the simplest to read option.

> +
> +	/* Query the sensor static information for closest match. */
> +	std::vector<unsigned int> mbusCodes;
> +	std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),
> +		std::back_inserter(mbusCodes),
> +		[](const std::map<uint32_t, MbusInfo>::value_type &pair){ return pair.first; });

I've submitted "[PATCH] libcamera: utils: Add map_keys() function", only
to then realize that here you're storing mbus codes in a vector, not a
set. I have a bit of a feeling that the usage of different types is a
sign that the API isn't consistent. And modifying
CameraSensor::getFormat() to take a set would likely require more
replacement of vectors with sets, which I'm not sure is a good idea (I
haven't checked how far it would spread).

In any case, if we decide to keep vectors, patch "[PATCH] libcamera:
utils: Add map_keys() function" should be changed to return a vector, I
think it would still be useful.

> +
> +	V4L2SubdeviceFormat sensorFormat = sensor_->getFormat(mbusCodes, size);
> +
> +	if (!sensorFormat.mbus_code) {
> +		LOG(IPU3, Error) << "Sensor does not support mbus code";
> +		return {};
> +	}
> +
> +	cfg.size = sensorFormat.size;
> +	cfg.pixelFormat = mbusCodesToInfo.at(sensorFormat.mbus_code).pixelFormat;
> +	cfg.bufferCount = CIO2_BUFFER_COUNT;
> +
> +	return cfg;
> +}
> +
>  /**
>   * \brief Allocate frame buffers for the CIO2 output
>   *
> diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
> index b2c4f89d682d6cfb..6276573f2b585b25 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.h
> +++ b/src/libcamera/pipeline/ipu3/cio2.h
> @@ -20,6 +20,7 @@ class V4L2DeviceFormat;
>  class V4L2Subdevice;
>  class V4L2VideoDevice;
>  struct Size;
> +struct StreamConfiguration;
>  
>  class CIO2Device
>  {
> @@ -32,6 +33,8 @@ public:
>  	int init(const MediaDevice *media, unsigned int index);
>  	int configure(const Size &size, V4L2DeviceFormat *outputFormat);
>  
> +	StreamConfiguration generateConfiguration(const Size &desiredSize) const;
> +
>  	int allocateBuffers();
>  	void freeBuffers();
>  
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 6e5eb5609a3c2388..c0e727e592f46883 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -36,13 +36,6 @@ LOG_DEFINE_CATEGORY(IPU3)
>  
>  class IPU3CameraData;
>  
> -static const std::map<uint32_t, PixelFormat> sensorMbusToPixel = {
> -	{ MEDIA_BUS_FMT_SBGGR10_1X10, formats::SBGGR10_IPU3 },
> -	{ MEDIA_BUS_FMT_SGBRG10_1X10, formats::SGBRG10_IPU3 },
> -	{ MEDIA_BUS_FMT_SGRBG10_1X10, formats::SGRBG10_IPU3 },
> -	{ MEDIA_BUS_FMT_SRGGB10_1X10, formats::SRGGB10_IPU3 },
> -};
> -
>  class ImgUDevice
>  {
>  public:
> @@ -147,7 +140,7 @@ public:
>  
>  	Status validate() override;
>  
> -	const V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }
> +	const StreamConfiguration &cio2Format() const { return cio2Configuration_; };
>  	const std::vector<const IPU3Stream *> &streams() { return streams_; }
>  
>  private:
> @@ -165,7 +158,7 @@ private:
>  	std::shared_ptr<Camera> camera_;
>  	const IPU3CameraData *data_;
>  
> -	V4L2SubdeviceFormat sensorFormat_;
> +	StreamConfiguration cio2Configuration_;

This may still be an abuse of StreamConfiguration, but I think we can
live with it for now until further refactoring.

>  	std::vector<const IPU3Stream *> streams_;
>  };
>  
> @@ -252,7 +245,7 @@ void IPU3CameraConfiguration::assignStreams()
>  
>  		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
>  			stream = &data_->rawStream_;
> -		else if (cfg.size == sensorFormat_.size)
> +		else if (cfg.size == cio2Configuration_.size)
>  			stream = &data_->outStream_;
>  		else
>  			stream = &data_->vfStream_;
> @@ -277,8 +270,8 @@ void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)
>  		 */
>  		if (!cfg.size.width || !cfg.size.height) {
>  			cfg.size.width = 1280;
> -			cfg.size.height = 1280 * sensorFormat_.size.height
> -					/ sensorFormat_.size.width;
> +			cfg.size.height = 1280 * cio2Configuration_.size.height
> +					/ cio2Configuration_.size.width;
>  		}
>  
>  		/*
> @@ -297,7 +290,7 @@ void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)
>  		 * \todo: Properly support cropping when the ImgU driver
>  		 * interface will be cleaned up.
>  		 */
> -		cfg.size = sensorFormat_.size;
> +		cfg.size = cio2Configuration_.size;
>  	}
>  
>  	/*
> @@ -313,7 +306,6 @@ void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)
>  
>  CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  {
> -	const CameraSensor *sensor = data_->cio2_.sensor_;
>  	Status status = Valid;
>  
>  	if (config_.empty())
> @@ -340,16 +332,10 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  			size.height = cfg.size.height;
>  	}
>  
> -	if (!size.width || !size.height)
> -		size = sensor->resolution();
> -
> -	sensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10,
> -					    MEDIA_BUS_FMT_SGBRG10_1X10,
> -					    MEDIA_BUS_FMT_SGRBG10_1X10,
> -					    MEDIA_BUS_FMT_SRGGB10_1X10 },
> -					  size);
> -	if (!sensorFormat_.size.width || !sensorFormat_.size.height)
> -		sensorFormat_.size = sensor->resolution();
> +	/* Generate raw configuration from CIO2. */
> +	cio2Configuration_ = data_->cio2_.generateConfiguration(size);
> +	if (!cio2Configuration_.pixelFormat.isValid())
> +		return Invalid;
>  
>  	/* Assign streams to each configuration entry. */
>  	assignStreams();
> @@ -361,20 +347,13 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  		const IPU3Stream *stream = streams_[i];
>  
>  		if (stream->raw_) {
> -			const auto &itFormat =
> -				sensorMbusToPixel.find(sensorFormat_.mbus_code);
> -			if (itFormat == sensorMbusToPixel.end())
> -				return Invalid;
> -
> -			cfg.pixelFormat = itFormat->second;
> -			cfg.size = sensorFormat_.size;
> +			cfg = cio2Configuration_;
>  		} else {
>  			bool scale = stream == &data_->vfStream_;
>  			adjustStream(config_[i], scale);
> +			cfg.bufferCount = IPU3_BUFFER_COUNT;
>  		}
>  
> -		cfg.bufferCount = IPU3_BUFFER_COUNT;
> -
>  		if (cfg.pixelFormat != oldCfg.pixelFormat ||
>  		    cfg.size != oldCfg.size) {
>  			LOG(IPU3, Debug)
> @@ -454,14 +433,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
>  
>  			cfg.size = data->cio2_.sensor_->resolution();
>  
> -			V4L2SubdeviceFormat sensorFormat =
> -				data->cio2_.sensor_->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10,
> -								 MEDIA_BUS_FMT_SGBRG10_1X10,
> -								 MEDIA_BUS_FMT_SGRBG10_1X10,
> -								 MEDIA_BUS_FMT_SRGGB10_1X10 },
> -							       cfg.size);
> -			cfg.pixelFormat =
> -				sensorMbusToPixel.at(sensorFormat.mbus_code);
> +			cfg = data->cio2_.generateConfiguration(cfg.size);
>  			break;
>  		}
>  
> @@ -575,7 +547,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>  	 * Pass the requested stream size to the CIO2 unit and get back the
>  	 * adjusted format to be propagated to the ImgU output devices.
>  	 */
> -	const Size &sensorSize = config->sensorFormat().size;
> +	const Size &sensorSize = config->cio2Format().size;
>  	V4L2DeviceFormat cio2Format = {};
>  	ret = cio2->configure(sensorSize, &cio2Format);
>  	if (ret)

With the above issues addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Overall I think this is a nice cleanup, even if there's room for
improvement (mainly addressing the possible abuse of
StreamConfiguration, and the std::vector vs. std::set issue, the latter
not being specific to this patch).
Laurent Pinchart June 25, 2020, 1:25 a.m. UTC | #3 
Hi Jacopo,

On Wed, Jun 24, 2020 at 12:22:01PM +0200, Jacopo Mondi wrote:
> On Tue, Jun 23, 2020 at 04:09:27AM +0200, Niklas Söderlund wrote:
> > Collect the code used to generate configurations for the CIO2 block in
> > the CIO2Device class. This allows simplifying the code and allow further
> > changes to only  happen at one code location.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> > * Changes since v2
> > - Remove unneeded code to pick sensor FourCC.
> > - Remove desiredPixelFormat from generateConfiguration() as it's not
> >   needed.
> > - Rename sensorFormat_ cio2Configuration_
> > - Consolidate all format information in a single table.
> >
> > * Changes since v1
> > - Use anonymous namespace instead of static for sensorMbusToPixel map.
> > - Handle case where the requested mbus code is not supported by the sensor.
> > - Update commit message.
> > ---
> >  src/libcamera/pipeline/ipu3/cio2.cpp | 54 +++++++++++++++++++++++----
> >  src/libcamera/pipeline/ipu3/cio2.h   |  3 ++
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 56 +++++++---------------------
> >  3 files changed, 63 insertions(+), 50 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> > index f23128d412e6b1a5..d6bab896706dd60e 100644
> > --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> > @@ -9,6 +9,9 @@
> >
> >  #include <linux/media-bus-format.h>
> >
> > +#include <libcamera/formats.h>
> > +#include <libcamera/stream.h>
> > +
> >  #include "libcamera/internal/camera_sensor.h"
> >  #include "libcamera/internal/media_device.h"
> >  #include "libcamera/internal/v4l2_subdevice.h"
> > @@ -20,11 +23,16 @@ LOG_DECLARE_CATEGORY(IPU3)
> >
> >  namespace {
> >
> > -static const std::map<uint32_t, V4L2PixelFormat> mbusCodesToInfo = {
> > -	{ MEDIA_BUS_FMT_SBGGR10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10) },
> > -	{ MEDIA_BUS_FMT_SGBRG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10) },
> > -	{ MEDIA_BUS_FMT_SGRBG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10) },
> > -	{ MEDIA_BUS_FMT_SRGGB10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10) },
> > +struct MbusInfo {
> > +	PixelFormat pixelFormat;
> > +	V4L2PixelFormat fourcc;
> > +};
> > +
> > +static const std::map<uint32_t, MbusInfo> mbusCodesToInfo = {
> > +	{ MEDIA_BUS_FMT_SBGGR10_1X10, { formats::SBGGR10_IPU3, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10) } },
> > +	{ MEDIA_BUS_FMT_SGBRG10_1X10, { formats::SGBRG10_IPU3, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10) } },
> > +	{ MEDIA_BUS_FMT_SGRBG10_1X10, { formats::SGRBG10_IPU3, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10) } },
> > +	{ MEDIA_BUS_FMT_SRGGB10_1X10, { formats::SRGGB10_IPU3, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10) } },
> >  };
> >
> >  } /* namespace */
> > @@ -92,7 +100,7 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
> >  	std::set<unsigned int> cio2Codes;
> >  	std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),
> >  		std::inserter(cio2Codes, cio2Codes.begin()),
> > -		[](const std::map<uint32_t, V4L2PixelFormat>::value_type &pair){ return pair.first; });
> > +		[](const std::map<uint32_t, MbusInfo>::value_type &pair){ return pair.first; });
> >  	const std::set<unsigned int> &sensorCodes = sensor_->mbusCodes();
> >  	if (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(),
> >  				cio2Codes.begin(), cio2Codes.end())) {
> > @@ -139,7 +147,7 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)
> >  	std::vector<unsigned int> mbusCodes;
> >  	std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),
> >  		std::back_inserter(mbusCodes),
> > -		[](const std::map<uint32_t, V4L2PixelFormat>::value_type &pair){ return pair.first; });
> > +		[](const std::map<uint32_t, MbusInfo>::value_type &pair){ return pair.first; });
> >
> >  	sensorFormat = sensor_->getFormat(mbusCodes, size);
> >  	ret = sensor_->setFormat(&sensorFormat);
> > @@ -154,7 +162,7 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)
> >  	if (itInfo == mbusCodesToInfo.end())
> >  		return -EINVAL;
> >
> > -	outputFormat->fourcc = itInfo->second;
> > +	outputFormat->fourcc = itInfo->second.fourcc;
> >  	outputFormat->size = sensorFormat.size;
> >  	outputFormat->planesCount = 1;
> >
> > @@ -167,6 +175,36 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)
> >  	return 0;
> >  }
> >
> > +StreamConfiguration
> > +CIO2Device::generateConfiguration(const Size &desiredSize) const
> > +{
> > +	StreamConfiguration cfg;
> > +
> > +	/* If no desired size use the sensor resolution. */
> > +	Size size = sensor_->resolution();
> > +	if (desiredSize.width && desiredSize.height)
> > +		size = desiredSize;
> > +
> > +	/* Query the sensor static information for closest match. */
> > +	std::vector<unsigned int> mbusCodes;
> > +	std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),
> > +		std::back_inserter(mbusCodes),
> > +		[](const std::map<uint32_t, MbusInfo>::value_type &pair){ return pair.first; });
> 
> Oh, one more :(
> 
> What about a CIO2Device::mbusCodes() helper for this ? With C++ copy
> elision you would not get any performance penality

See "[PATCH] libcamera: utils: Add map_keys() function" :-) We however
have to decide if the function should return a vector or a set.

> > +
> > +	V4L2SubdeviceFormat sensorFormat = sensor_->getFormat(mbusCodes, size);
> > +
> 
> stray empty line
> 
> The rest is good. I think we're abusing StreamConfiguration, as it is
> supposed to report StreamFormats, but it's optional, and we use it

I wonder what's wrong, we tend to agree a lot lately :-)

> internally to transport plaing "formats" between components. You could
> even use an ImageFormat if my series ever gets any feedback, but I
> won't block this one because of this. But really, we need to fix this
> as it's getting confusing enough :)
> 
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> > +	if (!sensorFormat.mbus_code) {
> > +		LOG(IPU3, Error) << "Sensor does not support mbus code";
> > +		return {};
> > +	}
> > +
> > +	cfg.size = sensorFormat.size;
> > +	cfg.pixelFormat = mbusCodesToInfo.at(sensorFormat.mbus_code).pixelFormat;
> > +	cfg.bufferCount = CIO2_BUFFER_COUNT;
> > +
> > +	return cfg;
> > +}
> > +
> >  /**
> >   * \brief Allocate frame buffers for the CIO2 output
> >   *
> > diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
> > index b2c4f89d682d6cfb..6276573f2b585b25 100644
> > --- a/src/libcamera/pipeline/ipu3/cio2.h
> > +++ b/src/libcamera/pipeline/ipu3/cio2.h
> > @@ -20,6 +20,7 @@ class V4L2DeviceFormat;
> >  class V4L2Subdevice;
> >  class V4L2VideoDevice;
> >  struct Size;
> > +struct StreamConfiguration;
> >
> >  class CIO2Device
> >  {
> > @@ -32,6 +33,8 @@ public:
> >  	int init(const MediaDevice *media, unsigned int index);
> >  	int configure(const Size &size, V4L2DeviceFormat *outputFormat);
> >
> > +	StreamConfiguration generateConfiguration(const Size &desiredSize) const;
> > +
> >  	int allocateBuffers();
> >  	void freeBuffers();
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 6e5eb5609a3c2388..c0e727e592f46883 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -36,13 +36,6 @@ LOG_DEFINE_CATEGORY(IPU3)
> >
> >  class IPU3CameraData;
> >
> > -static const std::map<uint32_t, PixelFormat> sensorMbusToPixel = {
> > -	{ MEDIA_BUS_FMT_SBGGR10_1X10, formats::SBGGR10_IPU3 },
> > -	{ MEDIA_BUS_FMT_SGBRG10_1X10, formats::SGBRG10_IPU3 },
> > -	{ MEDIA_BUS_FMT_SGRBG10_1X10, formats::SGRBG10_IPU3 },
> > -	{ MEDIA_BUS_FMT_SRGGB10_1X10, formats::SRGGB10_IPU3 },
> > -};
> > -
> >  class ImgUDevice
> >  {
> >  public:
> > @@ -147,7 +140,7 @@ public:
> >
> >  	Status validate() override;
> >
> > -	const V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }
> > +	const StreamConfiguration &cio2Format() const { return cio2Configuration_; };
> >  	const std::vector<const IPU3Stream *> &streams() { return streams_; }
> >
> >  private:
> > @@ -165,7 +158,7 @@ private:
> >  	std::shared_ptr<Camera> camera_;
> >  	const IPU3CameraData *data_;
> >
> > -	V4L2SubdeviceFormat sensorFormat_;
> > +	StreamConfiguration cio2Configuration_;
> >  	std::vector<const IPU3Stream *> streams_;
> >  };
> >
> > @@ -252,7 +245,7 @@ void IPU3CameraConfiguration::assignStreams()
> >
> >  		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
> >  			stream = &data_->rawStream_;
> > -		else if (cfg.size == sensorFormat_.size)
> > +		else if (cfg.size == cio2Configuration_.size)
> >  			stream = &data_->outStream_;
> >  		else
> >  			stream = &data_->vfStream_;
> > @@ -277,8 +270,8 @@ void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)
> >  		 */
> >  		if (!cfg.size.width || !cfg.size.height) {
> >  			cfg.size.width = 1280;
> > -			cfg.size.height = 1280 * sensorFormat_.size.height
> > -					/ sensorFormat_.size.width;
> > +			cfg.size.height = 1280 * cio2Configuration_.size.height
> > +					/ cio2Configuration_.size.width;
> >  		}
> >
> >  		/*
> > @@ -297,7 +290,7 @@ void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)
> >  		 * \todo: Properly support cropping when the ImgU driver
> >  		 * interface will be cleaned up.
> >  		 */
> > -		cfg.size = sensorFormat_.size;
> > +		cfg.size = cio2Configuration_.size;
> >  	}
> >
> >  	/*
> > @@ -313,7 +306,6 @@ void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)
> >
> >  CameraConfiguration::Status IPU3CameraConfiguration::validate()
> >  {
> > -	const CameraSensor *sensor = data_->cio2_.sensor_;
> >  	Status status = Valid;
> >
> >  	if (config_.empty())
> > @@ -340,16 +332,10 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> >  			size.height = cfg.size.height;
> >  	}
> >
> > -	if (!size.width || !size.height)
> > -		size = sensor->resolution();
> > -
> > -	sensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10,
> > -					    MEDIA_BUS_FMT_SGBRG10_1X10,
> > -					    MEDIA_BUS_FMT_SGRBG10_1X10,
> > -					    MEDIA_BUS_FMT_SRGGB10_1X10 },
> > -					  size);
> > -	if (!sensorFormat_.size.width || !sensorFormat_.size.height)
> > -		sensorFormat_.size = sensor->resolution();
> > +	/* Generate raw configuration from CIO2. */
> > +	cio2Configuration_ = data_->cio2_.generateConfiguration(size);
> > +	if (!cio2Configuration_.pixelFormat.isValid())
> > +		return Invalid;
> >
> >  	/* Assign streams to each configuration entry. */
> >  	assignStreams();
> > @@ -361,20 +347,13 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> >  		const IPU3Stream *stream = streams_[i];
> >
> >  		if (stream->raw_) {
> > -			const auto &itFormat =
> > -				sensorMbusToPixel.find(sensorFormat_.mbus_code);
> > -			if (itFormat == sensorMbusToPixel.end())
> > -				return Invalid;
> > -
> > -			cfg.pixelFormat = itFormat->second;
> > -			cfg.size = sensorFormat_.size;
> > +			cfg = cio2Configuration_;
> >  		} else {
> >  			bool scale = stream == &data_->vfStream_;
> >  			adjustStream(config_[i], scale);
> > +			cfg.bufferCount = IPU3_BUFFER_COUNT;
> >  		}
> >
> > -		cfg.bufferCount = IPU3_BUFFER_COUNT;
> > -
> >  		if (cfg.pixelFormat != oldCfg.pixelFormat ||
> >  		    cfg.size != oldCfg.size) {
> >  			LOG(IPU3, Debug)
> > @@ -454,14 +433,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> >
> >  			cfg.size = data->cio2_.sensor_->resolution();
> >
> > -			V4L2SubdeviceFormat sensorFormat =
> > -				data->cio2_.sensor_->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10,
> > -								 MEDIA_BUS_FMT_SGBRG10_1X10,
> > -								 MEDIA_BUS_FMT_SGRBG10_1X10,
> > -								 MEDIA_BUS_FMT_SRGGB10_1X10 },
> > -							       cfg.size);
> > -			cfg.pixelFormat =
> > -				sensorMbusToPixel.at(sensorFormat.mbus_code);
> > +			cfg = data->cio2_.generateConfiguration(cfg.size);
> >  			break;
> >  		}
> >
> > @@ -575,7 +547,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> >  	 * Pass the requested stream size to the CIO2 unit and get back the
> >  	 * adjusted format to be propagated to the ImgU output devices.
> >  	 */
> > -	const Size &sensorSize = config->sensorFormat().size;
> > +	const Size &sensorSize = config->cio2Format().size;
> >  	V4L2DeviceFormat cio2Format = {};
> >  	ret = cio2->configure(sensorSize, &cio2Format);
> >  	if (ret)
> > --
> > 2.27.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
index f23128d412e6b1a5..d6bab896706dd60e 100644
--- a/src/libcamera/pipeline/ipu3/cio2.cpp
+++ b/src/libcamera/pipeline/ipu3/cio2.cpp
@@ -9,6 +9,9 @@ 
 
 #include <linux/media-bus-format.h>
 
+#include <libcamera/formats.h>
+#include <libcamera/stream.h>
+
 #include "libcamera/internal/camera_sensor.h"
 #include "libcamera/internal/media_device.h"
 #include "libcamera/internal/v4l2_subdevice.h"
@@ -20,11 +23,16 @@  LOG_DECLARE_CATEGORY(IPU3)
 
 namespace {
 
-static const std::map<uint32_t, V4L2PixelFormat> mbusCodesToInfo = {
-	{ MEDIA_BUS_FMT_SBGGR10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10) },
-	{ MEDIA_BUS_FMT_SGBRG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10) },
-	{ MEDIA_BUS_FMT_SGRBG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10) },
-	{ MEDIA_BUS_FMT_SRGGB10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10) },
+struct MbusInfo {
+	PixelFormat pixelFormat;
+	V4L2PixelFormat fourcc;
+};
+
+static const std::map<uint32_t, MbusInfo> mbusCodesToInfo = {
+	{ MEDIA_BUS_FMT_SBGGR10_1X10, { formats::SBGGR10_IPU3, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10) } },
+	{ MEDIA_BUS_FMT_SGBRG10_1X10, { formats::SGBRG10_IPU3, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10) } },
+	{ MEDIA_BUS_FMT_SGRBG10_1X10, { formats::SGRBG10_IPU3, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10) } },
+	{ MEDIA_BUS_FMT_SRGGB10_1X10, { formats::SRGGB10_IPU3, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10) } },
 };
 
 } /* namespace */
@@ -92,7 +100,7 @@  int CIO2Device::init(const MediaDevice *media, unsigned int index)
 	std::set<unsigned int> cio2Codes;
 	std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),
 		std::inserter(cio2Codes, cio2Codes.begin()),
-		[](const std::map<uint32_t, V4L2PixelFormat>::value_type &pair){ return pair.first; });
+		[](const std::map<uint32_t, MbusInfo>::value_type &pair){ return pair.first; });
 	const std::set<unsigned int> &sensorCodes = sensor_->mbusCodes();
 	if (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(),
 				cio2Codes.begin(), cio2Codes.end())) {
@@ -139,7 +147,7 @@  int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)
 	std::vector<unsigned int> mbusCodes;
 	std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),
 		std::back_inserter(mbusCodes),
-		[](const std::map<uint32_t, V4L2PixelFormat>::value_type &pair){ return pair.first; });
+		[](const std::map<uint32_t, MbusInfo>::value_type &pair){ return pair.first; });
 
 	sensorFormat = sensor_->getFormat(mbusCodes, size);
 	ret = sensor_->setFormat(&sensorFormat);
@@ -154,7 +162,7 @@  int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)
 	if (itInfo == mbusCodesToInfo.end())
 		return -EINVAL;
 
-	outputFormat->fourcc = itInfo->second;
+	outputFormat->fourcc = itInfo->second.fourcc;
 	outputFormat->size = sensorFormat.size;
 	outputFormat->planesCount = 1;
 
@@ -167,6 +175,36 @@  int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)
 	return 0;
 }
 
+StreamConfiguration
+CIO2Device::generateConfiguration(const Size &desiredSize) const
+{
+	StreamConfiguration cfg;
+
+	/* If no desired size use the sensor resolution. */
+	Size size = sensor_->resolution();
+	if (desiredSize.width && desiredSize.height)
+		size = desiredSize;
+
+	/* Query the sensor static information for closest match. */
+	std::vector<unsigned int> mbusCodes;
+	std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(),
+		std::back_inserter(mbusCodes),
+		[](const std::map<uint32_t, MbusInfo>::value_type &pair){ return pair.first; });
+
+	V4L2SubdeviceFormat sensorFormat = sensor_->getFormat(mbusCodes, size);
+
+	if (!sensorFormat.mbus_code) {
+		LOG(IPU3, Error) << "Sensor does not support mbus code";
+		return {};
+	}
+
+	cfg.size = sensorFormat.size;
+	cfg.pixelFormat = mbusCodesToInfo.at(sensorFormat.mbus_code).pixelFormat;
+	cfg.bufferCount = CIO2_BUFFER_COUNT;
+
+	return cfg;
+}
+
 /**
  * \brief Allocate frame buffers for the CIO2 output
  *
diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
index b2c4f89d682d6cfb..6276573f2b585b25 100644
--- a/src/libcamera/pipeline/ipu3/cio2.h
+++ b/src/libcamera/pipeline/ipu3/cio2.h
@@ -20,6 +20,7 @@  class V4L2DeviceFormat;
 class V4L2Subdevice;
 class V4L2VideoDevice;
 struct Size;
+struct StreamConfiguration;
 
 class CIO2Device
 {
@@ -32,6 +33,8 @@  public:
 	int init(const MediaDevice *media, unsigned int index);
 	int configure(const Size &size, V4L2DeviceFormat *outputFormat);
 
+	StreamConfiguration generateConfiguration(const Size &desiredSize) const;
+
 	int allocateBuffers();
 	void freeBuffers();
 
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index 6e5eb5609a3c2388..c0e727e592f46883 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -36,13 +36,6 @@  LOG_DEFINE_CATEGORY(IPU3)
 
 class IPU3CameraData;
 
-static const std::map<uint32_t, PixelFormat> sensorMbusToPixel = {
-	{ MEDIA_BUS_FMT_SBGGR10_1X10, formats::SBGGR10_IPU3 },
-	{ MEDIA_BUS_FMT_SGBRG10_1X10, formats::SGBRG10_IPU3 },
-	{ MEDIA_BUS_FMT_SGRBG10_1X10, formats::SGRBG10_IPU3 },
-	{ MEDIA_BUS_FMT_SRGGB10_1X10, formats::SRGGB10_IPU3 },
-};
-
 class ImgUDevice
 {
 public:
@@ -147,7 +140,7 @@  public:
 
 	Status validate() override;
 
-	const V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }
+	const StreamConfiguration &cio2Format() const { return cio2Configuration_; };
 	const std::vector<const IPU3Stream *> &streams() { return streams_; }
 
 private:
@@ -165,7 +158,7 @@  private:
 	std::shared_ptr<Camera> camera_;
 	const IPU3CameraData *data_;
 
-	V4L2SubdeviceFormat sensorFormat_;
+	StreamConfiguration cio2Configuration_;
 	std::vector<const IPU3Stream *> streams_;
 };
 
@@ -252,7 +245,7 @@  void IPU3CameraConfiguration::assignStreams()
 
 		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
 			stream = &data_->rawStream_;
-		else if (cfg.size == sensorFormat_.size)
+		else if (cfg.size == cio2Configuration_.size)
 			stream = &data_->outStream_;
 		else
 			stream = &data_->vfStream_;
@@ -277,8 +270,8 @@  void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)
 		 */
 		if (!cfg.size.width || !cfg.size.height) {
 			cfg.size.width = 1280;
-			cfg.size.height = 1280 * sensorFormat_.size.height
-					/ sensorFormat_.size.width;
+			cfg.size.height = 1280 * cio2Configuration_.size.height
+					/ cio2Configuration_.size.width;
 		}
 
 		/*
@@ -297,7 +290,7 @@  void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)
 		 * \todo: Properly support cropping when the ImgU driver
 		 * interface will be cleaned up.
 		 */
-		cfg.size = sensorFormat_.size;
+		cfg.size = cio2Configuration_.size;
 	}
 
 	/*
@@ -313,7 +306,6 @@  void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)
 
 CameraConfiguration::Status IPU3CameraConfiguration::validate()
 {
-	const CameraSensor *sensor = data_->cio2_.sensor_;
 	Status status = Valid;
 
 	if (config_.empty())
@@ -340,16 +332,10 @@  CameraConfiguration::Status IPU3CameraConfiguration::validate()
 			size.height = cfg.size.height;
 	}
 
-	if (!size.width || !size.height)
-		size = sensor->resolution();
-
-	sensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10,
-					    MEDIA_BUS_FMT_SGBRG10_1X10,
-					    MEDIA_BUS_FMT_SGRBG10_1X10,
-					    MEDIA_BUS_FMT_SRGGB10_1X10 },
-					  size);
-	if (!sensorFormat_.size.width || !sensorFormat_.size.height)
-		sensorFormat_.size = sensor->resolution();
+	/* Generate raw configuration from CIO2. */
+	cio2Configuration_ = data_->cio2_.generateConfiguration(size);
+	if (!cio2Configuration_.pixelFormat.isValid())
+		return Invalid;
 
 	/* Assign streams to each configuration entry. */
 	assignStreams();
@@ -361,20 +347,13 @@  CameraConfiguration::Status IPU3CameraConfiguration::validate()
 		const IPU3Stream *stream = streams_[i];
 
 		if (stream->raw_) {
-			const auto &itFormat =
-				sensorMbusToPixel.find(sensorFormat_.mbus_code);
-			if (itFormat == sensorMbusToPixel.end())
-				return Invalid;
-
-			cfg.pixelFormat = itFormat->second;
-			cfg.size = sensorFormat_.size;
+			cfg = cio2Configuration_;
 		} else {
 			bool scale = stream == &data_->vfStream_;
 			adjustStream(config_[i], scale);
+			cfg.bufferCount = IPU3_BUFFER_COUNT;
 		}
 
-		cfg.bufferCount = IPU3_BUFFER_COUNT;
-
 		if (cfg.pixelFormat != oldCfg.pixelFormat ||
 		    cfg.size != oldCfg.size) {
 			LOG(IPU3, Debug)
@@ -454,14 +433,7 @@  CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
 
 			cfg.size = data->cio2_.sensor_->resolution();
 
-			V4L2SubdeviceFormat sensorFormat =
-				data->cio2_.sensor_->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10,
-								 MEDIA_BUS_FMT_SGBRG10_1X10,
-								 MEDIA_BUS_FMT_SGRBG10_1X10,
-								 MEDIA_BUS_FMT_SRGGB10_1X10 },
-							       cfg.size);
-			cfg.pixelFormat =
-				sensorMbusToPixel.at(sensorFormat.mbus_code);
+			cfg = data->cio2_.generateConfiguration(cfg.size);
 			break;
 		}
 
@@ -575,7 +547,7 @@  int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
 	 * Pass the requested stream size to the CIO2 unit and get back the
 	 * adjusted format to be propagated to the ImgU output devices.
 	 */
-	const Size &sensorSize = config->sensorFormat().size;
+	const Size &sensorSize = config->cio2Format().size;
 	V4L2DeviceFormat cio2Format = {};
 	ret = cio2->configure(sensorSize, &cio2Format);
 	if (ret)