[libcamera-devel] [PATCH v6 5/5] ipa: libcamera: add support for ov8858 sensor

[Nicholas Roth]

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

-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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20221031/93be54b9/attachment.htm>