[libcamera-devel] [PATCH 06/12] libcamera: ipu3: Initialize ScalerCropMaximum property

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Jan 05, 2021 at 08:05:16PM +0100, Jacopo Mondi wrote:
> Initialize the camera properties by registering the sensor provided

s/sensor provided/sensor-provided/

> ones and the ScalerCropMaximum property computed by the pipeline
> handler by using the analogue crop rectangle of the sensor resolution.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 43 +++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 879057dab328..418301b33a5e 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -14,6 +14,7 @@
>  #include <libcamera/camera.h>
>  #include <libcamera/control_ids.h>
>  #include <libcamera/formats.h>
> +#include <libcamera/property_ids.h>
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
>  
> @@ -121,6 +122,7 @@ private:
>  			PipelineHandler::cameraData(camera));
>  	}
>  
> +	int initProperties(IPU3CameraData *data);
>  	int initControls(IPU3CameraData *data);
>  	int registerCameras();
>  
> @@ -734,6 +736,43 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
>  	return ret == 0;
>  }
>  
> +/*
> + * \brief Initialize the camera properties
> + * \param[in] data The camera data
> + *
> + * Initialize the camera properties by registering the pipeline handler
> + * ones along with the properties assembled by inspecting the sensor
> + * capabilities.

That seems to be a copy&paste from patch 03/12, and doesn't really match
the implementation. How about the following ?

 * Initialize the camera properties by combining the properties reported by the
 * camera sensor with pipeline properties dynamically created based on device
 * capabilities.

Or something along those lines.

> + *
> + * \return 0 on success or a negative error code for error

s/for error/otherwise/

> + */
> +int PipelineHandlerIPU3::initProperties(IPU3CameraData *data)
> +{
> +	CameraSensor *sensor = data->cio2_.sensor();
> +	data->properties_ = sensor->properties();
> +
> +	/*
> +	 * \todo The ScalerCropMaximum property depends on the sensor
> +	 * configuration. Initialize the property using the analogue crop
> +	 * rectangle of the largest sensor resolution. To be updated later when
> +	 * a new one is applied.
> +	 */
> +	V4L2SubdeviceFormat format;
> +	format.size = sensor->resolution();
> +	int ret = sensor->setFormat(&format);
> +	if (ret)
> +		return ret;
> +
> +	CameraSensorInfo sensorInfo{};
> +	ret = sensor->sensorInfo(&sensorInfo);
> +	if (ret)
> +		return ret;
> +
> +	data->properties_.set(properties::ScalerCropMaximum, sensorInfo.analogCrop);

Unless I'm mistaken, you don't use this property in the HAL. How about
handling it the same way as in the RPi pipeline handler, initializing it
to Rectangle{} here, and updating it in configure() ? That's not great,
but it will be easier (I think) to improve the implementation if all
pipeline handlers implement the same hack.

> +
> +	return 0;
> +}
> +
>  /*
>   * \brief Initialize the camera controls
>   * \param[in] data The camera data
> @@ -831,7 +870,9 @@ int PipelineHandlerIPU3::registerCameras()
>  			continue;
>  
>  		/* Initialize the camera properties. */
> -		data->properties_ = cio2->sensor()->properties();
> +		ret = initProperties(data.get());
> +		if (ret)
> +			continue;
>  
>  		ret = initControls(data.get());
>  		if (ret)

-- 
Regards,

Laurent Pinchart