[libcamera-devel] [PATCH 02/10] libcamera: ipu3: Get default image sizes from sensor

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Feb 28, 2019 at 09:04:02PM +0100, Jacopo Mondi wrote:
> Inspect all image sizes provided by the sensor and select the
> biggest one to be returned as default stream configuration instead of
> returning the currently applied one.
> 
> Hardcode the stream pixel format to the one produced by the CIO2 unit,
> to be changed to the one provided by the ImgU.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 44 ++++++++++++++++++++--------
>  1 file changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index d3f1d9a95f81..4f1ab72debf8 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -71,6 +71,8 @@ public:
>  	bool match(DeviceEnumerator *enumerator);
>  
>  private:
> +	static constexpr unsigned int IPU3_BUF_NUM = 4;

IPU3_BUF_COUNT or IPU3_BUFFER_COUNT to match the name of the bufferCount
field ?

> +
>  	IPU3CameraData *cameraData(const Camera *camera)
>  	{
>  		return static_cast<IPU3CameraData *>(
> @@ -102,27 +104,45 @@ std::map<Stream *, StreamConfiguration>
>  PipelineHandlerIPU3::streamConfiguration(Camera *camera,
>  					 std::vector<Stream *> &streams)
>  {
> +	std::map<unsigned int, std::vector<SizeRange>> formats;

How about movinf this line down to declare and initialize the variable
at the same time ?

>  	std::map<Stream *, StreamConfiguration> configs;
>  	IPU3CameraData *data = cameraData(camera);
>  	V4L2Subdevice *sensor = data->cio2.sensor;
> -	V4L2SubdeviceFormat format = {};
> +	StreamConfiguration *config = &configs[&data->stream_];
> +
> +	config->pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10;
> +	config->bufferCount = IPU3_BUF_NUM;
>  
>  	/*
> -	 * FIXME: As of now, return the image format reported by the sensor.
> -	 * In future good defaults should be provided for each stream.
> +	 * Use the largest image size the sensor provides or
> +	 * use a default one.
>  	 */
> -	if (sensor->getFormat(0, &format)) {
> -		LOG(IPU3, Error) << "Failed to create stream configurations";
> -		return configs;
> +	formats = sensor->formats(0);
> +	if (formats.empty()) {
> +		config->width = 1920;
> +		config->height = 1080;
> +		LOG(IPU3, Info)
> +			<< "Use default stream sizes " << config->width
> +			<< "x" << config->height;

It doesn't make much sense to fall back to a default value here, all
sensor drivers must report the sizes they support. If you're worried
they may not, let's add a check when creating the camera and error out.
You could then cache the formats supported by the sensor in the
CIO2Device structure.

>  	}
>  
> -	StreamConfiguration config = {};
> -	config.width = format.width;
> -	config.height = format.height;
> -	config.pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10;
> -	config.bufferCount = 4;
> +	auto it = formats.begin();
> +	while (it != formats.end()) {

Why do you dislike for loops ? :-)

	for (auto it = formats.begin(); it != formats.end(); ++it) {

And I'd even make it a const auto it and a const SizeRange & below as
you don't modify it.

> +		for (SizeRange &range : it->second) {
> +			if (range.maxWidth <= config->width ||
> +			    range.maxHeight <= config->height)
> +				continue;
> +
> +			config->width = range.maxWidth;
> +			config->height = range.maxHeight;
> +		}

What happens if the sensor supports two sizes with different aspect
ratios, let's say 4:3 and 16:9, where one has a smaller width by larger
height ? Shouldn't we compare the areas instead of the individual
dimensions ?

> +
> +		++it;
> +	}
>  
> -	configs[&data->stream_] = config;
> +	LOG(IPU3, Info) << "Stream format set to = (" << config->width << "x"
> +			<< config->height << ") - 0x" << std::hex

I know how much you dislike the streams API, but 0x%08x would be nice.

> +			<< config->pixelFormat;

Wouldn't Debug make more sense than Info here ?

>  
>  	return configs;
>  }

-- 
Regards,

Laurent Pinchart