<div dir="ltr"><div><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div dir="ltr"><div style="font-size:12.8px"><div style="color:rgb(136,136,136);font-size:12.8px"><div style="font-size:small"><font face="arial, helvetica, sans-serif" color="#000000">> </font><span style="color:rgb(34,34,34)">please pardon me for being pedantic, but I would:</span></div><span style="color:rgb(34,34,34);font-size:small">> libcamera: Add support for ov8858 sensor</span><br style="color:rgb(34,34,34);font-size:small">> <br style="color:rgb(34,34,34);font-size:small"><span style="color:rgb(34,34,34);font-size:small">> instead of:</span><br style="color:rgb(34,34,34);font-size:small"><span style="color:rgb(34,34,34);font-size:small">> ipa: libcamera: add metadata for the ov8858 sensor</span><br style="color:rgb(34,34,34);font-size:small"></div><div style="color:rgb(136,136,136);font-size:12.8px"><span style="color:rgb(34,34,34);font-size:small"><br></span></div><div style="color:rgb(136,136,136);font-size:12.8px"><span style="color:rgb(34,34,34);font-size:small">Happy to make the change.</span></div><div style="color:rgb(136,136,136);font-size:12.8px"><span style="color:rgb(34,34,34);font-size:small"><br></span></div><div style="color:rgb(136,136,136);font-size:12.8px"><span style="color:rgb(34,34,34);font-size:small">> </span><span style="font-size:small;color:rgb(34,34,34)">The sensor doesn't seem to be supported mainline :(...</span></div><div style="color:rgb(136,136,136);font-size:12.8px"><span style="font-size:small;color:rgb(34,34,34)">> </span><span style="font-size:small;color:rgb(34,34,34)">Let's start from the first point: where does this driver lives ? What</span></div><span style="font-size:small">> kernel are you using ?</span></div><div style="font-size:12.8px"><span style="font-size:small">I'm using Manjaro's kernel </span><span style="font-size:small">5.19.8-1-MANJARO-ARM (from uname -r).</span></div><div style="font-size:12.8px"><span style="font-size:small"><br></span></div><div style="font-size:12.8px"><span style="font-size:small">It looks like my package manager is picking up the Manjaro kernel from </span><span style="font-size:small"><a href="https://gitlab.manjaro.org/manjaro-arm/packages/core/linux-pinephonepro/-/blob/5.19-megi/config">https://gitlab.manjaro.org/manjaro-arm/packages/core/linux-pinephonepro/-/blob/5.19-megi/config</a>, which sets "</span><span style="font-size:small">CONFIG_VIDEO_OV8858=m" in /config otherwise has no 8858-related changes. This ultimately pulls the driver from </span><span style="font-size:small"><a href="https://github.com/megous/linux/blob/orange-pi-5.19/drivers/media/i2c/ov8858.c">https://github.com/megous/linux/blob/orange-pi-5.19/drivers/media/i2c/ov8858.c</a>.</span></div><div style="font-size:12.8px"><span style="font-size:small"><br></span></div><div style="font-size:12.8px"><span style="font-size:small">Looks like this has been in and out of Torvalds' peripheral view, e.g.:</span></div><div style="font-size:12.8px"><a href="https://lkml.iu.edu/hypermail/linux/kernel/1711.1/06245.html">https://lkml.iu.edu/hypermail/linux/kernel/1711.1/06245.html</a><span style="font-size:small"><br></span></div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px">If you think that it would be valuable to try and mainline this, I'd be interested to do so once I can get libcamera working on my hardware, though I'll likely need some guidance in the process, since I'll be learning a lot.</div><div style="font-size:12.8px"><br></div><div style="font-size:12.8px"><span style="font-size:small">> Nice this matches the CCS defined linear gain model.</span><br style="font-size:small"><span style="font-size:small">> However the sensor allows to select two formats for the analogue gain</span><br style="font-size:small"><span style="font-size:small">> the "real gain" format and "sensor gain" format. The selection is made by</span><br style="font-size:small"><span style="font-size:small">> register 0x3503[2] and it would be nice to validate that the driver</span><br style="font-size:small"><span style="font-size:small">> uses the format that you expect.</span><br></div><div style="font-size:12.8px"><span style="font-size:small">Frankly, I'm not sure what you mean by this, but I'll try to find out from the driver above and verify.</span></div><div style="font-size:12.8px"><span style="font-size:small"><br></span></div><div style="font-size:12.8px"><span style="font-size:small">> Knowing what driver you're using is relevant, in example, as the</span><br style="font-size:small"><span style="font-size:small">> OV5688 sensor computes exposure in 1/16 of line length. This is not</span><br style="font-size:small"><span style="font-size:small">> what libcamera expects, and sensor drivers are expected to express the</span><br style="font-size:small"><span style="font-size:small">> V4L2_CID_EXPOSURE control in line units.</span><br style="font-size:small"><br style="font-size:small"><span style="font-size:small">> From Documentation/sensor_driver_re</span><span style="font-size:small">quirements.rst</span><br style="font-size:small"><br style="font-size:small"><span style="font-size:small">> While V4L2 doesn't specify a unit for the ``EXPOSURE`` control, libcamera</span><br style="font-size:small"><span style="font-size:small">> requires it to be expressed as a number of image lines. Camera sensor drivers</span><br style="font-size:small"><span style="font-size:small">> that do not comply with this requirement will need to be adapted or will produce</span><br style="font-size:small"><span style="font-size:small">> incorrect results.</span><span style="font-size:small"><br></span></div><div style="font-size:12.8px"><span style="font-size:small"><br></span></div><div style="font-size:12.8px"><span style="font-size:small">Let me read the driver and get back to you.</span></div></div></div></div></div><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Oct 27, 2022 at 11:41 AM Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Nicholas,<br>
<br>
please pardon me for being pedantic, but I would:<br>
libcamera: Add support for ov8858 sensor<br>
<br>
instead of:<br>
ipa: libcamera: add metadata for the ov8858 sensor<br>
<br>
"metadata" is an overloaded already term and usually refers to the<br>
properties associated to a captured frame (the term comes from the<br>
Android lingo, but we use it too).<br>
<br>
On Thu, Oct 27, 2022 at 12:55:09AM -0500, Nicholas Roth via libcamera-devel wrote:<br>
> From: Nicholas Roth <<a href="mailto:nicholas@rothemail.net" target="_blank">nicholas@rothemail.net</a>><br>
><br>
> Currently, libcamera does not have information for the ov8858 sensor<br>
> used in the PinePhone Pro, a phone designed to run Linux.<br>
><br>
> This commit adds metadata, especially that sensor gain is reported and<br>
> set in 1/16 discrete increments.<br>
><br>
> For more information, see "5.8 manual exposure compensation/ manual<br>
> gain compensation" in [0].<br>
><br>
> [0] <a href="http://www.ahdsensor.com/uploadfile/202008/55322e75316871.pdf" rel="noreferrer" target="_blank">http://www.ahdsensor.com/uploadfile/202008/55322e75316871.pdf</a><br>
<br>
The sensor doesn't seem to be supported mainline :(<br>
libcamera has a policy that requires supported components to be<br>
upstream or at least on their way to upstream (ie posted to the<br>
linux-media mailing list). The policy is there because different<br>
downstream driver implementations might behave differently one from<br>
the other, making it impossible for libcamera to support the part<br>
fully. The policy is a strict requirement for ISPs, I guess we're a<br>
bit more elastic when it comes to sensor. Howver knowing what driver<br>
you are using, where it lives, and if there's any plan to upstream it<br>
would be great.<br>
<br>
Let's start from the first point: where does this driver lives ? What<br>
kernel are you using ?<br>
<br>
Knowing what driver you're using is relevant, in example, as the<br>
OV5688 sensor computes exposure in 1/16 of line length. This is not<br>
what libcamera expects, and sensor drivers are expected to express the<br>
V4L2_CID_EXPOSURE control in line units.<br>
<br>
>From Documentation/sensor_driver_requirements.rst<br>
<br>
While V4L2 doesn't specify a unit for the ``EXPOSURE`` control, libcamera<br>
requires it to be expressed as a number of image lines. Camera sensor drivers<br>
that do not comply with this requirement will need to be adapted or will produce<br>
incorrect results.<br>
<br>
><br>
> Signed-off-by: Nicholas Roth <<a href="mailto:nicholas@rothemail.net" target="_blank">nicholas@rothemail.net</a>><br>
> ---<br>
> src/ipa/libipa/camera_sensor_helper.cpp | 11 +++++++++++<br>
> src/libcamera/camera_sensor_properties.cpp | 14 ++++++++++++++<br>
> 2 files changed, 25 insertions(+)<br>
><br>
> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp<br>
> index 35056bec..f2040cbd 100644<br>
> --- a/src/ipa/libipa/camera_sensor_helper.cpp<br>
> +++ b/src/ipa/libipa/camera_sensor_helper.cpp<br>
> @@ -476,6 +476,17 @@ public:<br>
> };<br>
> REGISTER_CAMERA_SENSOR_HELPER("ov5693", CameraSensorHelperOv5693)<br>
><br>
> +class CameraSensorHelperOv8858 : public CameraSensorHelper<br>
> +{<br>
> +public:<br>
> + CameraSensorHelperOv8858()<br>
> + {<br>
> + gainType_ = AnalogueGainLinear;<br>
> + gainConstants_.linear = { 1, 0, 0, 16 };<br>
> + }<br>
> +};<br>
<br>
Nice this matches the CCS defined linear gain model.<br>
However the sensor allows to select two formats for the analogue gain<br>
the "real gain" format and "sensor gain" format. The selection is made by<br>
register 0x3503[2] and it would be nice to validate that the driver<br>
uses the format that you expect.<br>
<br>
> +REGISTER_CAMERA_SENSOR_HELPER("m00_f_ov8858", CameraSensorHelperOv8858)<br>
<br>
Ah! that's a weird name for the sensor entity :)<br>
<br>
> +<br>
> class CameraSensorHelperOv8865 : public CameraSensorHelper<br>
> {<br>
> public:<br>
> diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp<br>
> index e5f27f06..d0757c15 100644<br>
> --- a/src/libcamera/camera_sensor_properties.cpp<br>
> +++ b/src/libcamera/camera_sensor_properties.cpp<br>
> @@ -146,6 +146,20 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen<br>
> */<br>
> },<br>
> } },<br>
> + { "m00_f_ov8858", {<br>
> + .unitCellSize = { 1200, 1200 },<br>
<br>
I read 1.12 um x 1.12 um<br>
<br>
> + .testPatternModes = {<br>
> + { controls::draft::TestPatternModeOff, 0 },<br>
> + { controls::draft::TestPatternModeColorBars, 1 },<br>
> + /*<br>
> + * No best corresponding test pattern for:<br>
> + * 1: "Vertical Color Bar Type 1",<br>
> + * 2: "Vertical Color Bar Type 2",<br>
> + * 3: "Vertical Color Bar Type 3",<br>
> + * 4: "Vertical Color Bar Type 4"<br>
> + */<br>
> + },<br>
> + } },<br>
> { "ov8865", {<br>
> .unitCellSize = { 1400, 1400 },<br>
> .testPatternModes = {<br>
> --<br>
> 2.34.1<br>
><br>
</blockquote></div>