[libcamera-devel,v2,07/10] libcamera: ipu3: cio2: Add function to generate configuration

3969 diff mbox series

Collect the code used to generate configurations for the CIO2 block in
the CIO2Device class. This allows for both simplifying the code while
extending its functionality. With this change applications can now
function with pixelformats other then SBGGR10 which previously was hard
coded.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
* 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 | 55 ++++++++++++++++++++++++++
 src/libcamera/pipeline/ipu3/cio2.h   |  8 +++-
 src/libcamera/pipeline/ipu3/ipu3.cpp | 58 +++++++---------------------
 3 files changed, 75 insertions(+), 46 deletions(-)

Hi Niklas,

Thank you for the patch.

On Sat, Jun 06, 2020 at 05:04:33PM +0200, Niklas Söderlund wrote:
> Collect the code used to generate configurations for the CIO2 block in
> the CIO2Device class. This allows for both simplifying the code while
> extending its functionality. With this change applications can now
> function with pixelformats other then SBGGR10 which previously was hard
> coded.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
> * 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 | 55 ++++++++++++++++++++++++++
>  src/libcamera/pipeline/ipu3/cio2.h   |  8 +++-
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 58 +++++++---------------------
>  3 files changed, 75 insertions(+), 46 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index 1a9f185bc4e161c5..0d961ae8f5a0682b 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> @@ -9,6 +9,8 @@
>  
>  #include <linux/media-bus-format.h>
>  
> +#include <libcamera/stream.h>
> +
>  #include "libcamera/internal/camera_sensor.h"
>  #include "libcamera/internal/media_device.h"
>  #include "libcamera/internal/v4l2_subdevice.h"
> @@ -18,6 +20,17 @@ namespace libcamera {
>  
>  LOG_DECLARE_CATEGORY(IPU3)
>  
> +namespace {
> +
> +const std::map<uint32_t, PixelFormat> sensorMbusToPixel = {
> +	{ MEDIA_BUS_FMT_SBGGR10_1X10, PixelFormat(DRM_FORMAT_SBGGR10, IPU3_FORMAT_MOD_PACKED) },
> +	{ MEDIA_BUS_FMT_SGBRG10_1X10, PixelFormat(DRM_FORMAT_SGBRG10, IPU3_FORMAT_MOD_PACKED) },
> +	{ MEDIA_BUS_FMT_SGRBG10_1X10, PixelFormat(DRM_FORMAT_SGRBG10, IPU3_FORMAT_MOD_PACKED) },
> +	{ MEDIA_BUS_FMT_SRGGB10_1X10, PixelFormat(DRM_FORMAT_SRGGB10, IPU3_FORMAT_MOD_PACKED) },
> +};
> +
> +} /* namespace */
> +
>  CIO2Device::CIO2Device()
>  	: output_(nullptr), csi2_(nullptr), sensor_(nullptr)
>  {
> @@ -172,6 +185,48 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)
>  	return 0;
>  }
>  
> +StreamConfiguration
> +CIO2Device::generateConfiguration(const PixelFormat desiredPixelFormat,
> +				  const Size desiredSize) const

I think Jacopo mentioned he would like this function to be documented. I
think it's a good idea in general, even if the documentation is ignored
by Doxygen, as the code may serve as a reference implementation.

> +{
> +	StreamConfiguration cfg;
> +
> +	/* If no desired pixelformat allow all the supported ones. */
> +	std::vector<unsigned int> mbusCodes = {
> +		MEDIA_BUS_FMT_SBGGR10_1X10,
> +		MEDIA_BUS_FMT_SGBRG10_1X10,
> +		MEDIA_BUS_FMT_SGRBG10_1X10,
> +		MEDIA_BUS_FMT_SRGGB10_1X10
> +	};
> +	if (desiredPixelFormat.isValid()) {
> +		for (const auto &iter : sensorMbusToPixel) {
> +			if (iter.second == desiredPixelFormat) {
> +				mbusCodes = { iter.first };
> +				break;
> +			}
> +		}
> +	}

I still don't really see the point of this. Maybe I'm missing something
obvious, but given that the sensor has a hardcoded Bayer pattern,
what's the point in trying to select a particular desired pattern ? What
could be selected is the number of bits per pixels, but we only support
10bpp for now.

> +
> +	/* 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. */
> +	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 = sensorMbusToPixel.at(sensorFormat.mbus_code);
> +	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..b7eb69a4e104f400 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.h
> +++ b/src/libcamera/pipeline/ipu3/cio2.h
> @@ -11,6 +11,9 @@
>  #include <queue>
>  #include <vector>
>  
> +#include <libcamera/geometry.h>
> +#include <libcamera/pixel_format.h>
> +
>  namespace libcamera {
>  
>  class CameraSensor;
> @@ -19,7 +22,7 @@ class MediaDevice;
>  class V4L2DeviceFormat;
>  class V4L2Subdevice;
>  class V4L2VideoDevice;
> -struct Size;
> +struct StreamConfiguration;
>  
>  class CIO2Device
>  {
> @@ -32,6 +35,9 @@ public:
>  	int init(const MediaDevice *media, unsigned int index);
>  	int configure(const Size &size, V4L2DeviceFormat *outputFormat);
>  
> +	StreamConfiguration generateConfiguration(const PixelFormat desiredPixelFormat = {},

If the pixel format isn't a reference, const isn't required.

> +						  const Size desiredSize = {}) const;

Shouldn't you pass a reference to Size ? You could then keep the forward
declaration of Size.

> +
>  	int allocateBuffers();
>  	void freeBuffers();
>  
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 257f5441c9ad7f5d..85d4e64396e77222 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -35,13 +35,6 @@ LOG_DEFINE_CATEGORY(IPU3)
>  
>  class IPU3CameraData;
>  
> -static const std::map<uint32_t, PixelFormat> sensorMbusToPixel = {
> -	{ MEDIA_BUS_FMT_SBGGR10_1X10, PixelFormat(DRM_FORMAT_SBGGR10, IPU3_FORMAT_MOD_PACKED) },
> -	{ MEDIA_BUS_FMT_SGBRG10_1X10, PixelFormat(DRM_FORMAT_SGBRG10, IPU3_FORMAT_MOD_PACKED) },
> -	{ MEDIA_BUS_FMT_SGRBG10_1X10, PixelFormat(DRM_FORMAT_SGRBG10, IPU3_FORMAT_MOD_PACKED) },
> -	{ MEDIA_BUS_FMT_SRGGB10_1X10, PixelFormat(DRM_FORMAT_SRGGB10, IPU3_FORMAT_MOD_PACKED) },
> -};
> -
>  class ImgUDevice
>  {
>  public:
> @@ -146,7 +139,7 @@ public:
>  
>  	Status validate() override;
>  
> -	const V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }
> +	const StreamConfiguration &sensorFormat() const { return sensorFormat_; };
>  	const std::vector<const IPU3Stream *> &streams() { return streams_; }
>  
>  private:
> @@ -164,7 +157,7 @@ private:
>  	std::shared_ptr<Camera> camera_;
>  	const IPU3CameraData *data_;
>  
> -	V4L2SubdeviceFormat sensorFormat_;
> +	StreamConfiguration sensorFormat_;

That seems an abuse of StreamConfiguration :-( You should at the very
least rename the variable, both because it's not a format anymore, and
because it's the CIO2 output format, not the sensor format. The
sensorFormat() function should also be renamed accordingly.

>  	std::vector<const IPU3Stream *> streams_;
>  };
>  
> @@ -304,7 +297,6 @@ void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)
>  
>  CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  {
> -	const CameraSensor *sensor = data_->cio2_.sensor_;
>  	Status status = Valid;
>  
>  	if (config_.empty())
> @@ -316,31 +308,23 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  		status = Adjusted;
>  	}
>  
> -	/*
> -	 * Select the sensor format by collecting the maximum width and height
> -	 * and picking the closest larger match, as the IPU3 can downscale
> -	 * only. If no resolution is requested for any stream, or if no sensor
> -	 * resolution is large enough, pick the largest one.
> -	 */
> +	/* Find largets size and raw format (if any) in the configuration. */
>  	Size size = {};
> -
> +	PixelFormat pixelFormat = {};

No need for explicit initialization of pixelFormat (or size, for that
matter).

>  	for (const StreamConfiguration &cfg : config_) {
>  		if (cfg.size.width > size.width)
>  			size.width = cfg.size.width;
>  		if (cfg.size.height > size.height)
>  			size.height = cfg.size.height;
> +
> +		if (cfg.pixelFormat.modifier() == IPU3_FORMAT_MOD_PACKED)
> +			pixelFormat = cfg.pixelFormat;
>  	}
>  
> -	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. */
> +	sensorFormat_ = data_->cio2_.generateConfiguration(pixelFormat, size);
> +	if (!sensorFormat_.pixelFormat.isValid())
> +		return Invalid;
>  
>  	/* Assign streams to each configuration entry. */
>  	assignStreams();
> @@ -352,13 +336,7 @@ 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 = sensorFormat_;
>  		} else {
>  			bool scale = stream == &data_->vfStream_;
>  			adjustStream(config_[i], scale);
> @@ -442,17 +420,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
>  			}
>  
>  			stream = &data->rawStream_;
> -
> -			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();
>  			break;
>  		}
>