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

10820 diff mbox series

Initialize the camera properties by registering the 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@jmondi.org>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 43 +++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

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@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)