[libcamera-devel] [PATCH 04/10] ipa: libcamera: add metadata for the ov8858 sensor
Jacopo Mondi
jacopo at jmondi.org
Fri Oct 28 09:57:28 CEST 2022
Hi Nicholas
On Thu, Oct 27, 2022 at 12:25:20PM -0500, Nicholas Roth wrote:
> > 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
>
> Happy to make the change.
>
> > The sensor doesn't seem to be supported mainline :(...
> > Let's start from the first point: where does this driver lives ? What
> > kernel are you using ?
> I'm using Manjaro's kernel 5.19.8-1-MANJARO-ARM (from uname -r).
>
> It looks like my package manager is picking up the Manjaro kernel from
> https://gitlab.manjaro.org/manjaro-arm/packages/core/linux-pinephonepro/-/blob/5.19-megi/config,
> which sets "CONFIG_VIDEO_OV8858=m" in /config otherwise has no 8858-related
> changes. This ultimately pulls the driver from
> https://github.com/megous/linux/blob/orange-pi-5.19/drivers/media/i2c/ov8858.c
> .
Right! So indeed that's downstream stuff, probably from Rockchip's
BSP.
The driver is not -that- bad, there's a little vendor's code crud here
and there but it looks like a decent candidate for upstreaming.
I would be happy to help if you're willing to try. I also own a pinephone
pro, which so far I haven't had enough time to play with
>
> Looks like this has been in and out of Torvalds' peripheral view, e.g.:
> https://lkml.iu.edu/hypermail/linux/kernel/1711.1/06245.html
The driver mentioned lives in:
drivers/staging/media/atomisp/i2c/
The AtomISP support was upstreamed with its own set of sensor drivers,
which were probably not compliant with the mainline V4L2 kAPI and used
some vendor defined abstractions. That's why they used to live in staging.
However they might serve as reference is something goes wrong, and
sometimes they're even copies of regular drivers/media/i2c/ drivers
adapted to work with the custom abstractions. AtomISP has also been
removed from upstream a few years ago. It's being re-upstreamed in
these days by Hans de Goede, hopefully without custom sensor drivers.
TL;DR do not consider drivers/staging/media/atomisp/i2c drivers as
candidates for upstreaming, even if they might sometime provide
references to hw quirks and configurations.
>
> 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.
Happy to help if you want to give it a go!
>
> > 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.
> Frankly, I'm not sure what you mean by this, but I'll try to find out from
> the driver above and verify.
>
Sorry for having given a few things for granted.
CCS is the MIPI Alliance specification for a "Camera Command Set", an
attempt to define a standard for image sensors register interfaces.
As part of the register interface definition, CCS specifies how sensor
drivers expose their configuration parameters including the analogue
gain model.
The specs are publicly available
https://www.mipi.org/specifications/camera-command-set
and you'll find in section 9.3 how the Analogue Global Gain control is
implemented.
When it comes to this specific sensor, the datasheet documents
register 0x3503[2] as
0: Input gain as real gain format
1: Input gain as sensor gain format
The "real gain" vs "sensor gain" modes are described in the
documentation of register 0x3509
0x3503[2]=0, gain[7:0] is real gain format,
where low 4 bits are fraction bits, for
example, 0x10 is 1x gain, 0x28 is 2.5x gain
If 0x3503[2]=1, gain[7:0] is sensor gain
format, gain[7:4] is coarse gain, 00000: 1x,
00001: 2x, 00011: 4x, 00111: 8x, gain[7] is
1, gain[3:0] is fine gain. For example, 0x10
is 1x gain, 0x30 is 2x gain, 0x70 is 4x gain
So what you want here is to have the sensor operate in "real gain"
mode, where 0x01 == 1/16 increment in gain
> > 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.
>
> Let me read the driver and get back to you.
>
>
> On Thu, Oct 27, 2022 at 11:41 AM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> > 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
> > >
> >
More information about the libcamera-devel
mailing list