[libcamera-devel] [PATCH v6 5/5] ipa: libcamera: add support for ov8858 sensor
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Oct 31 18:07:56 CET 2022
Hi Nicholas,
Cc Sakari (Sensor drivers maintainer for the Kernel).
Quoting Nicholas Roth (2022-10-31 16:22:40)
> > So accepting 'm00_f_ov8858' at all ... seems to be really tricky.
>
> What I was trying to say in my earlier email is that the only requirement
> for the v4l2 "name" field be unique, although there are conventions. See
> comments above the regex logic
> in libcamera/src/libcamera/v4l2_subdevice.cpp.
* - The most common rule, used by I2C sensors, associates the model
* name with the I2C bus number and address (e.g. 'imx219 0-0010').
The OV8858 is an I2C controlled sensor though right ? So we should
follow the same convention.
> The current driver isn't doing anything that breaks the rules here as far
> as I can tell.
>
> A quick fix might be to modify the regex. A better fix might be to
> duplicate the logic in the Android HAL, which correctly reports the sensor
> as an "ov8858". Modifying the driver is an option as well, but for reasons
> pointed out above that will be problematic due to the downstream convention.
I understand, however there may be many downstream conventions, and only
one upstream convention (for I2C based sensors).
Your development will help downstream. When your driver is accepted
upstream, it will end up in their kernel. Either by them chosing to move
to it immediately, or when they rebase to a kernel where yours is
merged. If they have pain from things changing, that is only due to the
fact that they did not post it themselves at the earliest opportunity.
Making libcamera compatible with lots of defined and undefined
downstream conventions, vs - the conventions used in mainline, provides
clear differences in maintenance and support.
For instance, How should we generically handle another platform that
uses a convention of
"ov5640-front-0",
and another one that has
"FL IMX602"?
You are (rightly, it's the device you have) looking at this from the
point of view of getting libcamera to work on a single device, while we
look at it from a point of view of having this camera work on /any/
device. Imagine that once you have your driver merged in the kernel, you
will connect an OV8858 to a Raspberry Pi, and then use libcamera with
it.
If you are able to convince Sakari and Hans (who will maintain and
handle the OV8858 driver in the kernel), that the name should be kept as
it is, then we would have to consider how we'll track the naming in
libcamera. But ... I will be surprised.
--
Kieran
> -Nicholas
>
>
> On Mon, Oct 31, 2022 at 8:58 AM Kieran Bingham <
> kieran.bingham at ideasonboard.com> wrote:
>
> > Hi Nicholas,
> >
> > Quoting Nicholas Roth via libcamera-devel (2022-10-30 23:05:00)
> > > 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] and the driver in [1].
> > >
> > > [0] http://www.ahdsensor.com/uploadfile/202008/55322e75316871.pdf
> > > [1]
> > https://github.com/megous/linux/blob/orange-pi-5.19/drivers/media/i2c/ov8858.c
> > >
> > > 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 };
> > > + }
> > > +};
> > > +REGISTER_CAMERA_SENSOR_HELPER("m00_f_ov8858", CameraSensorHelperOv8858)
> >
> > "My OV8858 is the second on the rear." Suddenly this doesn't work.
> > - So we can only use "ov8858" here.
> >
> > We 'could' add a dupliate entry as
> >
> > +/*
> > + * \todo: Deprecated: The PinePhonePro has devices listed as m00_f_ov8858.
> > + * This entry should be removed as soon as the ov8858 driver is
> > + * accepted into a mainline kernel.
> > + */
> > +REGISTER_CAMERA_SENSOR_HELPER("m00_f_ov8858", CameraSensorHelperOv8858)
> > +REGISTER_CAMERA_SENSOR_HELPER("ov8858", CameraSensorHelperOv8858)
> >
> > But the problem there, is that suddenly users will find it 'works' until
> > the driver gets into mainline, when it breaks. Given that the correct
> > approach at that point will be to update to use the mainline driver,
> > perhaps that's ok ... but I fear potentially unhappy users.
> >
> > And perhaps also worryingly, it removes the impetus to get the driver
> > mainlined!
> >
> > So accepting 'm00_f_ov8858' at all ... seems to be really tricky.
> >
> > With just ov8858, and the 1.12 fix below this patch could probably go in
> > quite rapidly though. You could easily have the
> >
> > +REGISTER_CAMERA_SENSOR_HELPER("m00_f_ov8858", CameraSensorHelperOv8858)
> >
> > line locally while developing, but I'd expect that your device should be
> > quickly moving along with the mainline integration, so you probably
> > wouldn't need it.
> >
> >
> >
> > > +
> > > 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", {
> >
> > And it won't be as short to support here here, I guess the whole table
> > could be duplicated, but actually - missing camera-sensor properties
> > don't cause the camera to fail to load - so I think this should simply
> > be kept as ov8858 only.
> >
> >
> > > + .unitCellSize = { 1200, 1200 },
> >
> > http://www.ahdsensor.com/uploadfile/202008/55322e75316871.pdf states
> > this should be 1120, 1120.
> >
> > >> 1.12 μm x 1.12 μm pixel with OmniBSI-3™ technology
> >
> > > + .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