[libcamera-devel] [PATCH 04/10] ipa: libcamera: add metadata for the ov8858 sensor

[Jacopo Mondi]

Hi Nicholas,

please pardon me for being pedantic, but I would:
libcamera: Add support for ov8858 sensor

instead of:
ipa: libcamera: add metadata for the ov8858 sensor

"metadata" is an overloaded already term and usually refers to the
properties associated to a captured frame (the term comes from the
Android lingo, but we use it too).

On Thu, Oct 27, 2022 at 12:55:09AM -0500, Nicholas Roth via libcamera-devel wrote:
> From: Nicholas Roth <nicholas at rothemail.net>
>
> Currently, libcamera does not have information for the ov8858 sensor
> used in the PinePhone Pro, a phone designed to run Linux.
>
> This commit adds metadata, especially that sensor gain is reported and
> set in 1/16 discrete increments.
>
> For more information, see "5.8 manual exposure compensation/ manual
> gain compensation" in [0].
>
> [0] http://www.ahdsensor.com/uploadfile/202008/55322e75316871.pdf

The sensor doesn't seem to be supported mainline :(
libcamera has a policy that requires supported components to be
upstream or at least on their way to upstream (ie posted to the
linux-media mailing list). The policy is there because different
downstream driver implementations might behave differently one from
the other, making it impossible for libcamera to support the part
fully. The policy is a strict requirement for ISPs, I guess we're a
bit more elastic when it comes to sensor. Howver knowing what driver
you are using, where it lives, and if there's any plan to upstream it
would be great.

Let's start from the first point: where does this driver lives ? What
kernel are you using ?

Knowing what driver you're using is relevant, in example, as the
OV5688 sensor computes exposure in 1/16 of line length. This is not
what libcamera expects, and sensor drivers are expected to express the
V4L2_CID_EXPOSURE control in line units.

>From Documentation/sensor_driver_requirements.rst

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
incorrect results.

>
> Signed-off-by: Nicholas Roth <nicholas at rothemail.net>
> ---
>  src/ipa/libipa/camera_sensor_helper.cpp    | 11 +++++++++++
>  src/libcamera/camera_sensor_properties.cpp | 14 ++++++++++++++
>  2 files changed, 25 insertions(+)
>
> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> index 35056bec..f2040cbd 100644
> --- a/src/ipa/libipa/camera_sensor_helper.cpp
> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> @@ -476,6 +476,17 @@ public:
>  };
>  REGISTER_CAMERA_SENSOR_HELPER("ov5693", CameraSensorHelperOv5693)
>
> +class CameraSensorHelperOv8858 : public CameraSensorHelper
> +{
> +public:
> +	CameraSensorHelperOv8858()
> +	{
> +		gainType_ = AnalogueGainLinear;
> +		gainConstants_.linear = { 1, 0, 0, 16 };
> +	}
> +};

Nice this matches the CCS defined linear gain model.
However the sensor allows to select two formats for the analogue gain
the "real gain" format and "sensor gain" format. The selection is made by
register 0x3503[2] and it would be nice to validate that the driver
uses the format that you expect.

> +REGISTER_CAMERA_SENSOR_HELPER("m00_f_ov8858", CameraSensorHelperOv8858)

Ah! that's a weird name for the sensor entity :)

> +
>  class CameraSensorHelperOv8865 : public CameraSensorHelper
>  {
>  public:
> diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp
> index e5f27f06..d0757c15 100644
> --- a/src/libcamera/camera_sensor_properties.cpp
> +++ b/src/libcamera/camera_sensor_properties.cpp
> @@ -146,6 +146,20 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>  				 */
>  			},
>  		} },
> +		{ "m00_f_ov8858", {
> +			.unitCellSize = { 1200, 1200 },

I read 1.12 um x 1.12 um

> +			.testPatternModes = {
> +				{ controls::draft::TestPatternModeOff, 0 },
> +				{ controls::draft::TestPatternModeColorBars, 1 },
> +				/*
> +				 * No best corresponding test pattern for:
> +				 * 1: "Vertical Color Bar Type 1",
> +				 * 2: "Vertical Color Bar Type 2",
> +				 * 3: "Vertical Color Bar Type 3",
> +				 * 4: "Vertical Color Bar Type 4"
> +				 */
> +			},
> +		} },
>  		{ "ov8865", {
>  			.unitCellSize = { 1400, 1400 },
>  			.testPatternModes = {
> --
> 2.34.1
>