[libcamera-devel] [PATCH v2 1/4] libcamera: Move IPA sensor controls validation to CameraSensor

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Nov 23, 2022 at 02:43:43PM +0100, Jacopo Mondi via libcamera-devel wrote:
> The CameraSensor class validates that the sensor driver in use supports
> the controls required for IPA modules to work correctly.
> 
> For in-tree IPA modules, whose pipeline handlers already use
> CameraSensor there's no need to validate such controls again.
> 
> Remove controls validation from the IPU3 and RkISP1 IPA modules and rely
> on CameraSensor doing that at initialization time.
> 
> The list of mandatory controls is expanded to add V4L2_CID_ANALOGUE_GAIN
> without which IPA modules cannot function.
> 
> The new requirement only applies to RAW sensors, platforms like UVC and
> Simple are not impacted by this change.
> 
> While at it, expand the sensor driver requirements documentation to
> include V4L2_ANALOGUE_GAIN in the list of mandatory controls a sensor
> driver has to support.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> ---
>  Documentation/sensor_driver_requirements.rst |  7 +++++
>  src/ipa/ipu3/ipu3.cpp                        | 29 --------------------
>  src/ipa/rkisp1/rkisp1.cpp                    | 12 +-------
>  src/libcamera/camera_sensor.cpp              |  1 +
>  4 files changed, 9 insertions(+), 40 deletions(-)
> 
> diff --git a/Documentation/sensor_driver_requirements.rst b/Documentation/sensor_driver_requirements.rst
> index b0854be3328a..3abc8f35924a 100644
> --- a/Documentation/sensor_driver_requirements.rst
> +++ b/Documentation/sensor_driver_requirements.rst
> @@ -24,16 +24,23 @@ The sensor driver is assumed to be fully compliant with the V4L2 specification.
>  
>  For RAW sensors, the sensor driver shall support the following V4L2 controls:
>  
> +* `V4L2_CID_ANALOGUE_GAIN`_
>  * `V4L2_CID_EXPOSURE`_
>  * `V4L2_CID_HBLANK`_
>  * `V4L2_CID_PIXEL_RATE`_
>  * `V4L2_CID_VBLANK`_
>  
> +.. _V4L2_CID_ANALOGUE_GAIN: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-image-source.html
>  .. _V4L2_CID_EXPOSURE: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/control.html
>  .. _V4L2_CID_HBLANK: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-image-source.html
>  .. _V4L2_CID_PIXEL_RATE: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-image-process.html
>  .. _V4L2_CID_VBLANK: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-image-source.html
>  
> +The ``ANALOGUE_GAIN`` control units are sensor-specific. libcamera requires
> +a sensor-specific CameraSensorHelper implementation to translate between the
> +sensor specific ``gain code`` and the analogue ``gain value`` expressed as an
> +absolute number as defined by ``controls::AnalogueGain``.
> +
>  While V4L2 doesn't specify a unit for the ``EXPOSURE`` control, libcamera
>  requires it to be expressed as a number of image lines. Camera sensor drivers
>  that do not comply with this requirement will need to be adapted or will produce
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index a9a2b49ca95b..08ee6eb30cc5 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -171,8 +171,6 @@ private:
>  			    ControlInfoMap *ipaControls);
>  	void updateSessionConfiguration(const ControlInfoMap &sensorControls);
>  
> -	bool validateSensorControls();
> -
>  	void setControls(unsigned int frame);
>  	void calculateBdsGrid(const Size &bdsOutputSize);
>  
> @@ -292,28 +290,6 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,
>  	*ipaControls = ControlInfoMap(std::move(controls), controls::controls);
>  }
>  
> -/**
> - * \brief Validate that the sensor controls mandatory for the IPA exists
> - */
> -bool IPAIPU3::validateSensorControls()
> -{
> -	static const uint32_t ctrls[] = {
> -		V4L2_CID_ANALOGUE_GAIN,
> -		V4L2_CID_EXPOSURE,
> -		V4L2_CID_VBLANK,
> -	};
> -
> -	for (auto c : ctrls) {
> -		if (sensorCtrls_.find(c) == sensorCtrls_.end()) {
> -			LOG(IPAIPU3, Error) << "Unable to find sensor control "
> -					    << utils::hex(c);
> -			return false;
> -		}
> -	}
> -
> -	return true;
> -}
> -
>  /**
>   * \brief Initialize the IPA module and its controls
>   *
> @@ -512,11 +488,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>  
>  	calculateBdsGrid(configInfo.bdsOutputSize);
>  
> -	if (!validateSensorControls()) {
> -		LOG(IPAIPU3, Error) << "Sensor control validation failed.";
> -		return -EINVAL;
> -	}
> -
>  	/* Update the camera controls using the new sensor settings. */
>  	updateControls(sensorInfo_, sensorCtrls_, ipaControls);
>  
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index fcb9dacccc3c..d237758f7bf0 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -216,20 +216,10 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
>  	ctrls_ = entityControls.at(0);
>  
>  	const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);
> -	if (itExp == ctrls_.end()) {
> -		LOG(IPARkISP1, Error) << "Can't find exposure control";
> -		return -EINVAL;
> -	}
> -
> -	const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);
> -	if (itGain == ctrls_.end()) {
> -		LOG(IPARkISP1, Error) << "Can't find gain control";
> -		return -EINVAL;
> -	}
> -
>  	int32_t minExposure = itExp->second.min().get<int32_t>();
>  	int32_t maxExposure = itExp->second.max().get<int32_t>();
>  
> +	const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);
>  	int32_t minGain = itGain->second.min().get<int32_t>();
>  	int32_t maxGain = itGain->second.max().get<int32_t>();
>  
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 572a313a8f99..ea373458a164 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -301,6 +301,7 @@ int CameraSensor::validateSensorDriver()
>  	 * required by the CameraSensor class.
>  	 */
>  	static constexpr uint32_t mandatoryControls[] = {
> +		V4L2_CID_ANALOGUE_GAIN,
>  		V4L2_CID_EXPOSURE,
>  		V4L2_CID_HBLANK,
>  		V4L2_CID_PIXEL_RATE,

-- 
Regards,

Laurent Pinchart