[libcamera-devel] [PATCH v5 09/19] libcamera: ipu3: Set stream configuration from sensor

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Mar 26, 2019 at 09:38:52AM +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.

This isn't correct anymore, you don't inspect all sizes here, you use
the cached maximum size.

"While at it, replace the hardcoded numerical value for the number of
buffers with a named constexpr."

> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 35 +++++++++++++---------------
>  1 file changed, 16 insertions(+), 19 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index d42c81273cc6..04cd02653711 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -5,6 +5,7 @@
>   * ipu3.cpp - Pipeline handler for Intel IPU3
>   */
>  
> +#include <iomanip>
>  #include <memory>
>  #include <vector>
>  
> @@ -94,6 +95,8 @@ private:
>  		std::pair<unsigned int, SizeRange> maxSizes_;
>  	};
>  
> +	static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> +
>  	IPU3CameraData *cameraData(const Camera *camera)
>  	{
>  		return static_cast<IPU3CameraData *>(
> @@ -124,26 +127,20 @@ std::map<Stream *, StreamConfiguration>
>  PipelineHandlerIPU3::streamConfiguration(Camera *camera,
>  					 std::set<Stream *> &streams)
>  {
> -	IPU3CameraData *data = cameraData(camera);
>  	std::map<Stream *, StreamConfiguration> configs;
> -	V4L2SubdeviceFormat format = {};
> -
> -	/*
> -	 * FIXME: As of now, return the image format reported by the sensor.
> -	 * In future good defaults should be provided for each stream.
> -	 */
> -	if (data->sensor_->getFormat(0, &format)) {
> -		LOG(IPU3, Error) << "Failed to create stream configurations";
> -		return configs;
> -	}
> -
> -	StreamConfiguration config = {};
> -	config.width = format.width;
> -	config.height = format.height;
> -	config.pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10;
> -	config.bufferCount = 4;
> -
> -	configs[&data->stream_] = config;
> +	IPU3CameraData *data = cameraData(camera);

I'd keep this above the configs variable declaration to make the diff
more readable (unless you think there's an advantage in this new order).

> +	StreamConfiguration *config = &configs[&data->stream_];
> +	SizeRange &maxRange = data->maxSizes_.second;
> +
> +	config->width = maxRange.maxWidth;
> +	config->height = maxRange.maxHeight;
> +	config->pixelFormat = data->maxSizes_.first;
> +	config->bufferCount = IPU3_BUFFER_COUNT;
> +
> +	LOG(IPU3, Debug)
> +		<< "Stream format set to: " << config->width << "x"

s/to:/to/

> +		<< config->height << "-0x" << std::hex << std::setfill('0')
> +		<< std::setw(4) << config->pixelFormat;

Something to keep in mind for later, adding a Format class may be a good
idea, to store pixel format, width and height. We should then give it a
toString() method. Or do you think it would make sense to add a
toString() method to StreamConfiguration and use it here ?

>  	return configs;
>  }

-- 
Regards,

Laurent Pinchart