[libcamera-devel] [PATCH 2/3] libcamera: camera_sensor: Restrict sensor info to raw sensors

[Jacopo Mondi]

Hi Laurent

On Sun, Jan 31, 2021 at 08:17:21PM +0200, Laurent Pinchart wrote:
> YUV sensors don't provide the necessary information to fill
> CameraSensorInfo, as they include an ISP and provide a higher-level API
> that doesn't always expose low-level information. The CameraSensor class
> makes low-level V4L2 controls mandatory for all sensors, which prevents
> usage of YUV sensors with the simple pipeline handler.
>
> Make CameraSensor::sensorInfo() available for raw sensors only. This
> won't introduce any regression in pipeline handlers that currently use
> the sensorInfo() function as they all operate with raw sensors, and
> won't be a limitation for the simple pipeline handler as well as it
> doesn't use sensor info. If part of the sensor info (such as the active
> pixel array size for instance) becomes useful to expose for YUV sensors
> in the future, the sensorInfo() function can be extended to report that
> information only and skip data that is only available for raw sensors.
>

It makes sense, and I see the reasons why you won't this.
But I'm a little concerned that with the sensorInfo() function
returning -EINVAL we're limiting pipeline handlers that use
sensorInfo to work with raw sensor only, while I assume it's totally
plausible that someone wants to connect a YUV sensor to the pi, in
example.

Alternatively, sensorInfo can return only information that are
available in YUV sensor, but then the pipeline-IPA would break anyway.

This is less trivial than it seems and I'm afraid it will require a
differnet handling of RAW/YUV sensor in the pipeline handlers. Or do
you think a YUV sensor connected to a platform with a ISP it's not
that likely as a combination and we should not care too much ?

Thanks
  j

> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  Documentation/sensor_driver_requirements.rst |  2 +-
>  src/libcamera/camera_sensor.cpp              | 59 ++++++++++++--------
>  2 files changed, 36 insertions(+), 25 deletions(-)
>
> diff --git a/Documentation/sensor_driver_requirements.rst b/Documentation/sensor_driver_requirements.rst
> index 6dcd4e68d64d..64e00fd5fc7c 100644
> --- a/Documentation/sensor_driver_requirements.rst
> +++ b/Documentation/sensor_driver_requirements.rst
> @@ -22,7 +22,7 @@ Mandatory Requirements
>
>  The sensor driver is assumed to be fully compliant with the V4L2 specification.
>
> -The sensor driver shall support the following V4L2 controls:
> +For RAW sensors, the sensor driver shall support the following V4L2 controls:
>
>  * `V4L2_CID_EXPOSURE`_
>  * `V4L2_CID_HBLANK`_
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 35312857ff90..10713d3a0c29 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -246,27 +246,6 @@ int CameraSensor::init()
>
>  int CameraSensor::validateSensorDriver()
>  {
> -	/*
> -	 * Make sure the sensor driver supports the mandatory controls
> -	 * required by the CameraSensor class.
> -	 */
> -	const std::vector<uint32_t> mandatoryControls{
> -		V4L2_CID_EXPOSURE,
> -		V4L2_CID_HBLANK,
> -		V4L2_CID_PIXEL_RATE,
> -	};
> -
> -	ControlList ctrls = subdev_->getControls(mandatoryControls);
> -	if (ctrls.empty()) {
> -		LOG(CameraSensor, Error)
> -			<< "Mandatory V4L2 controls not available";
> -		LOG(CameraSensor, Error)
> -			<< "The sensor kernel driver needs to be fixed";
> -		LOG(CameraSensor, Error)
> -			<< "See Documentation/sensor_driver_requirements.rst in the libcamera sources for more information";
> -		return -EINVAL;
> -	}
> -
>  	/*
>  	 * Optional controls are used to register optional sensor properties. If
>  	 * not present, some values will be defaulted.
> @@ -276,7 +255,7 @@ int CameraSensor::validateSensorDriver()
>  		V4L2_CID_CAMERA_SENSOR_ROTATION,
>  	};
>
> -	ctrls = subdev_->getControls(optionalControls);
> +	ControlList ctrls = subdev_->getControls(optionalControls);
>  	if (ctrls.empty())
>  		LOG(CameraSensor, Debug) << "Optional V4L2 controls not supported";
>
> @@ -326,6 +305,30 @@ int CameraSensor::validateSensorDriver()
>  			<< "See Documentation/sensor_driver_requirements.rst in the libcamera sources for more information";
>  	}
>
> +	if (!bayerFormat_)
> +		return 0;
> +
> +	/*
> +	 * For raw sensors, make sure the sensor driver supports the controls
> +	 * required by the CameraSensor class.
> +	 */
> +	const std::vector<uint32_t> mandatoryControls{
> +		V4L2_CID_EXPOSURE,
> +		V4L2_CID_HBLANK,
> +		V4L2_CID_PIXEL_RATE,
> +	};
> +
> +	ctrls = subdev_->getControls(mandatoryControls);
> +	if (ctrls.empty()) {
> +		LOG(CameraSensor, Error)
> +			<< "Mandatory V4L2 controls not available";
> +		LOG(CameraSensor, Error)
> +			<< "The sensor kernel driver needs to be fixed";
> +		LOG(CameraSensor, Error)
> +			<< "See Documentation/sensor_driver_requirements.rst in the libcamera sources for more information";
> +		return -EINVAL;
> +	}
> +
>  	return 0;
>  }
>
> @@ -662,13 +665,21 @@ int CameraSensor::setControls(ControlList *ctrls)
>   * \brief Assemble and return the camera sensor info
>   * \param[out] info The camera sensor info
>   *
> - * The CameraSensorInfo content is assembled by inspecting the currently
> - * applied sensor configuration and the sensor static properties.
> + * This function fills \a info with information that describes the camera sensor
> + * and its current configuration. The information combines static data (such as
> + * the the sensor model or active pixel array size) and data specific to the
> + * current sensor configuration (such as the line length and pixel rate).
> + *
> + * Sensor information is only available for raw sensors. When called for a YUV
> + * sensor, this function returns -EINVAL.
>   *
>   * \return 0 on success, a negative error code otherwise
>   */
>  int CameraSensor::sensorInfo(CameraSensorInfo *info) const
>  {
> +	if (!bayerFormat_)
> +		return -EINVAL;
> +
>  	info->model = model();
>
>  	/*
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel