[libcamera-devel] [PATCH v2 03/14] libcamera: ipu3: Get default image sizes from sensor

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Mar 12, 2019 at 01:12:31PM +0100, Jacopo Mondi wrote:
> Inspect all image sizes provided by the sensor and select the
> biggest of them, associated with an image format code supported by the
> CIO2 unit.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 94 ++++++++++++++++++++++++----
>  1 file changed, 82 insertions(+), 12 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 55489c31df2d..0f18e4692e77 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -6,8 +6,11 @@
>   */
>  
>  #include <memory>
> +#include <iomanip>

Alphabetical order please.

>  #include <vector>
>  
> +#include <linux/media-bus-format.h>
> +
>  #include <libcamera/camera.h>
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
> @@ -72,12 +75,16 @@ private:
>  		Stream stream_;
>  	};
>  
> +	static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> +
>  	IPU3CameraData *cameraData(const Camera *camera)
>  	{
>  		return static_cast<IPU3CameraData *>(
>  			PipelineHandler::cameraData(camera));
>  	}
>  
> +	int mediaBusToCIO2Format(unsigned int code);

s/;/const ;/

> +
>  	void registerCameras();
>  
>  	std::shared_ptr<MediaDevice> cio2_;
> @@ -102,26 +109,45 @@ std::map<Stream *, StreamConfiguration>
>  PipelineHandlerIPU3::streamConfiguration(Camera *camera,
>  					 std::set<Stream *> &streams)
>  {
> -	IPU3CameraData *data = cameraData(camera);
>  	std::map<Stream *, StreamConfiguration> configs;
> -	V4L2SubdeviceFormat format = {};
> +	IPU3CameraData *data = cameraData(camera);
> +	V4L2Subdevice *sensor = data->sensor_;
> +	StreamConfiguration *config = &configs[&data->stream_];
>  
>  	/*
> -	 * FIXME: As of now, return the image format reported by the sensor.
> -	 * In future good defaults should be provided for each stream.
> +	 * Make sure the sensor produces a raw format compatible with the
> +	 * CIO2 and use the largest image size the sensor provides.
>  	 */
> -	if (data->sensor_->getFormat(0, &format)) {
> -		LOG(IPU3, Error) << "Failed to create stream configurations";
> +	const SubdevFormatEnum formats = sensor->formats(0);
> +	for (auto it = formats.begin(); it != formats.end(); ++it) {
> +		int cio2Code = mediaBusToCIO2Format(it->first);
> +		if (cio2Code == -EINVAL)
> +			continue;
> +
> +		for (const SizeRange &range : it->second) {
> +			if (range.maxWidth <= config->width ||
> +			    range.maxHeight <= config->height)
> +				continue;
> +
> +			config->width = range.maxWidth;
> +			config->height = range.maxHeight;
> +			config->pixelFormat = cio2Code;
> +		}

Didn't we decide to use the area instead of the width and height ?

> +	}
> +
> +	/* If not suitable format has been found, return an empty config. */
> +	if (!config->pixelFormat) {

This relies on config being zeroed when constructed, which isn't
guaranteed as far as I can tell.

> +		LOG(IPU3, Error) << "Sensor image format not supported";
> +
>  		return configs;
>  	}

Shouldn't this be done at match time and cached ? We shouldn't create a
camera whose sensor can't provide a supported format.

>  
> -	StreamConfiguration config = {};
> -	config.width = format.width;
> -	config.height = format.height;
> -	config.pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10;
> -	config.bufferCount = 4;
> +	config->bufferCount = IPU3_BUFFER_COUNT;
>  
> -	configs[&data->stream_] = config;
> +	LOG(IPU3, Debug)
> +		<< "Stream format set to = (" << config->width << "x"
> +		<< config->height << ") - 0x" << std::hex << std::setfill('0')
> +		<< std::setw(8) << config->pixelFormat;
>  
>  	return configs;
>  }
> @@ -327,6 +353,50 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
>  	return true;
>  }
>  
> +int PipelineHandlerIPU3::mediaBusToCIO2Format(unsigned int code)
> +{
> +	switch(code) {
> +	case MEDIA_BUS_FMT_SBGGR8_1X8:
> +	case MEDIA_BUS_FMT_SBGGR10_1X10:
> +	case MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_BE:
> +	case MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE:
> +	case MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_BE:
> +	case MEDIA_BUS_FMT_SBGGR10_2X8_PADLO_LE:
> +	case MEDIA_BUS_FMT_SBGGR10_ALAW8_1X8:
> +	case MEDIA_BUS_FMT_SBGGR10_DPCM8_1X8:
> +	case MEDIA_BUS_FMT_SBGGR12_1X12:
> +	case MEDIA_BUS_FMT_SBGGR14_1X14:
> +	case MEDIA_BUS_FMT_SBGGR16_1X16:
> +		return V4L2_PIX_FMT_IPU3_SBGGR10;
> +	case MEDIA_BUS_FMT_SGBRG8_1X8:
> +	case MEDIA_BUS_FMT_SGBRG10_1X10:
> +	case MEDIA_BUS_FMT_SGBRG10_ALAW8_1X8:
> +	case MEDIA_BUS_FMT_SGBRG10_DPCM8_1X8:
> +	case MEDIA_BUS_FMT_SGBRG12_1X12:
> +	case MEDIA_BUS_FMT_SGBRG14_1X14:
> +	case MEDIA_BUS_FMT_SGBRG16_1X16:
> +		return V4L2_PIX_FMT_IPU3_SGBRG10;
> +	case MEDIA_BUS_FMT_SGRBG8_1X8:
> +	case MEDIA_BUS_FMT_SGRBG10_1X10:
> +	case MEDIA_BUS_FMT_SGRBG10_ALAW8_1X8:
> +	case MEDIA_BUS_FMT_SGRBG10_DPCM8_1X8:
> +	case MEDIA_BUS_FMT_SGRBG12_1X12:
> +	case MEDIA_BUS_FMT_SGRBG14_1X14:
> +	case MEDIA_BUS_FMT_SGRBG16_1X16:
> +		return V4L2_PIX_FMT_IPU3_SGRBG10;
> +	case MEDIA_BUS_FMT_SRGGB8_1X8:
> +	case MEDIA_BUS_FMT_SRGGB10_1X10:
> +	case MEDIA_BUS_FMT_SRGGB10_ALAW8_1X8:
> +	case MEDIA_BUS_FMT_SRGGB10_DPCM8_1X8:
> +	case MEDIA_BUS_FMT_SRGGB12_1X12:
> +	case MEDIA_BUS_FMT_SRGGB14_1X14:
> +	case MEDIA_BUS_FMT_SRGGB16_1X16:
> +		return V4L2_PIX_FMT_IPU3_SRGGB10;

Does the CIO2 really support all these formats, especially the ALAW8,
DPCM8 and > 10bpp formats ?

> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  /*
>   * Cameras are created associating an image sensor (represented by a
>   * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four

-- 
Regards,

Laurent Pinchart