[libcamera-devel] [PATCH 04/10] ipa: libcamera: add metadata for the ov8858 sensor
Jacopo Mondi
jacopo at jmondi.org
Fri Oct 28 10:14:27 CEST 2022
Hi Nicholas,
+ Laurent question below
On Thu, Oct 27, 2022 at 05:21:37PM -0500, Nicholas Roth wrote:
> > 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.
>
> I think I understand this better after reading the driver. In the register
> value settings, I see:
> {0x3502, 0x40}, // exposure L
> {0x3503, 0x80}, // gain delay ?, exposure delay 1 frame, real gain
> {0x3505, 0x80}, // gain option
>
> The 0 low-nibbl indicates we're using "real gain."
>
> According to the datasheet:
> 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
>
> This tells me that we need to divide reported gain from the driver by 16 to
> get a properly-scaled double-value gain, which these gain constants do.
>
Here you go!
Looking at the driver you're using I indeed see 3503[2]=0 in all modes
> > 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.
>
> Looks like the driver multiples inputs x16 to convert to 1/16 line length
> before writing to the register...
>
> case V4L2_CID_EXPOSURE:
> /* 4 least significant bits of expsoure are fractional part */
> ret = ov8858_write_reg(ov8858->client,
> OV8858_REG_EXPOSURE,
> OV8858_REG_VALUE_24BIT,
> ctrl->val << 4);
>
Great, it means that towards userspace the exposure is indeed in units
of 1 line.
>
> Let me know if this looks good, and what needs to happen to merge this!
I think I had a comment on the pixel cell size.
Apart from that, let me check with Laurent about the policy when it
comes to supporting sensor without an upstream driver.
I would not be super happy of adding support to "m00_f_ov8858" as if the
driver will be upstreamed the entity name should at least change.
As a middle term solution we can add support for "m00_f_ov8858" with a
todo note to move to a more canonical "ov8858" name once the driver is
upstreamed, or at least submitted upstream.
Thanks
j
>
> Thanks,
> -Nicholas
>
> On Thu, Oct 27, 2022 at 12:25 PM Nicholas Roth <nicholas at rothemail.net>
> 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
> > .
> >
> > 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
> >
> > 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.
> >
> > > 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.
> >
> > > 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