[libcamera-devel] [RFC PATCH v2 09/12] pipeline: ipu3: Add controls for FULL compliance

[Jacopo Mondi]

Hi Paul,

On Thu, Apr 22, 2021 at 06:40:59PM +0900, Paul Elder wrote:
> Add controls to IPU3Controls that are necessary for FULL compliance.
>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 28e849a4..70a5e9ce 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -48,8 +48,27 @@ static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64;
>  static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32;
>  static constexpr Size IPU3ViewfinderSize(1280, 720);
>
> +const std::array<const ControlValue, 3> IPU3NoiseReductionModeValues = {
> +	static_cast<int32_t>(controls::draft::NoiseReductionModeOff),
> +	static_cast<int32_t>(controls::draft::NoiseReductionModeFast),
> +	static_cast<int32_t>(controls::draft::NoiseReductionModeHighQuality),
> +};
> +
>  static const ControlInfoMap::Map IPU3Controls = {
>  	{ &controls::draft::PipelineDepth, ControlInfo(2, 3) },
> +	{ &controls::draft::BlackLevelLocked, ControlInfo(false, true) },
> +	{ &controls::AeLocked, ControlInfo(false, true) },
> +	{ &controls::draft::AePrecaptureTrigger,
> +		ControlInfo(controls::draft::AePrecaptureTriggerValues) },
> +	{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
> +	{ &controls::AwbLocked, ControlInfo(false, true) },
> +	{ &controls::draft::EdgeMode,
> +		ControlInfo(controls::draft::EdgeModeValues) },
> +	{ &controls::draft::NoiseReductionMode,
> +		ControlInfo(IPU3NoiseReductionModeValues) },

Interesting you've ruled out IPU3NoiseReductionModeZSL.
What if we plumb an IPA that supports that ? We have two IPAs for
IPU3, depending on which one is in use we might have different
features supported. Going forward, do we need the IPA capabilities to
be discoverable ?

> +	{ &controls::draft::SensorSensitivity, ControlInfo(32, 2400) },

This should probably come from the sensor.

The following V4L2 control is described as:

V4L2_CID_ISO_SENSITIVITY (integer menu)
Determines ISO equivalent of an image sensor indicating the sensor’s
sensitivity to light. The numbers are expressed in arithmetic scale,
as per ISO 12232:2006 standard, where doubling the sensor sensitivity
is represented by doubling the numerical ISO value. Applications
should interpret the values as standard ISO values multiplied by 1000,
e.g. control value 800 stands for ISO 0.8. Drivers will usually
support only a subset of standard ISO values. The effect of setting
this control while the V4L2_CID_ISO_SENSITIVITY_AUTO control is set to
a value other than V4L2_CID_ISO_SENSITIVITY_MANUAL is undefined,
drivers should ignore such requests.

It's good the standard the control refers to is the same as
android.sensor.sensitivity control:

The sensitivity is the standard ISO sensitivity value,as defined in ISO 12232:2006.

Anyway, that's more a policy decision. Do we require/mandate that
control in sensor drivers (keeping in mind we keep rising the bar for
drivers to qualify libcamera-compatible) or do we go through the
sensor database ?

Thanks
  j

> +	{ &controls::draft::TonemapMode,
> +		ControlInfo(controls::draft::TonemapModeValues) },
>  };
>
>  class IPU3CameraData : public CameraData
> --
> 2.27.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel