[libcamera-devel] [PATCH] libcamera: camera_sensor: Add OV9281 sensor properties

[Kieran Bingham]

Quoting Dave Stevenson (2022-11-11 17:00:25)
> Hi Kieran
> 
> On Fri, 11 Nov 2022 at 16:37, Kieran Bingham via libcamera-devel
> <libcamera-devel at lists.libcamera.org> wrote:
> >
> > Add an entry to the sensor properties for the OmniVision OV9281. Test
> > patterns are not enabled yet as the driver is not in an upstream kernel.
> 
> It's not going to be merged to an upstream kernel.
> OV9282 (which is upstream) is the same as OV9281 except for the CRA in
> the optical path.

Will the sensor differences be exposed to userspace? (i.e. distinct
compatibles / identifiers to userspace?) (Ok, I see that's a yes -
later).

I believe I had this patch locally, because I saw someone complain about
one of the libcamera warnings so I must have looked up the data sheet to
get the basics started.


> I've sent patches to make ov9282 work with libcamera ([1] and [2]),
> and the Pi kernel is hopefully going to drop our downstream ov9281
> driver [3] once the patches are accepted by Sakari.
> 

Ok - interesting, everything related to this so far in libcamera is
ov9281.
libcamera$ find | grep ov928

./install/libcamera-gcc/usr/share/libcamera/ipa/raspberrypi/ov9281.json
./build/package/src/ipa/raspberrypi/ipa_rpi.so.p/cam_helper_ov9281.cpp.o
./build/test/src/ipa/raspberrypi/ipa_rpi.so.p/cam_helper_ov9281.cpp.o
./build/rpi/bullseye/src/ipa/raspberrypi/ipa_rpi.so.p/cam_helper_ov9281.cpp.o
./build/gcc/src/ipa/raspberrypi/ipa_rpi.so.p/cam_helper_ov9281.cpp.o
./src/ipa/raspberrypi/data/ov9281_mono.json
./src/ipa/raspberrypi/cam_helper_ov9281.cpp

> It does leave the odd subject that Alexander started with regard
> adopting the compatible name rather than module name for the sensor
> [4], and that seems to have gone a bit quiet.

That sounds correct to me, the compatible string could / should
highlight the distinction between the two sensors IMO. I'll go throw my
hat into that thread to state that I see compatibles as the correct way
to support both of these, even if they are 'almost the same'.

Of course these 'two' sensors are *very* alike, so it does beg, what do
we really want to duplicate in libcamera. Or should we have the ability
to match a single helper to multiple camera identifiers?

I also now wonder if we need to track those differences in CRA in any
helper - or if any applications (or IPA) care about that specific
property ?

 
> Can I ask what the fascination is with test patterns? Are they really
> that useful for drivers that have been merged to mainline and
> therefore really should work?

It's purely to be able to map test patterns to the corresponding V4L2
control id. Because there's no standardised way to express it ... it's a
mapping of an expected pattern in libcamera, to what that should be
represented as in the V4L2 specific control on the sensor. Each sensor
seems to do things differently ... ?

I don't think it's anything special except for a workaround for a poorly
defined implementation of V4L2_CID_TEST_PATTERN and the like...

--
Kieran


> Cheers
>   Dave
> 
> [1] https://patchwork.linuxtv.org/project/linux-media/cover/20221005152018.3783890-1-dave.stevenson@raspberrypi.com/
> [2] https://patchwork.linuxtv.org/project/linux-media/cover/20221028160902.2696973-1-dave.stevenson@raspberrypi.com/
> [3] https://github.com/raspberrypi/linux/pull/5187
> [4] https://patchwork.linuxtv.org/project/linux-media/patch/20220728130237.3396663-7-alexander.stein@ew.tq-group.com/
> 
> > Unit cell size obtained from [0].
> >
> > [0] https://www.ovt.com/wp-content/uploads/2022/01/OV9281-OV9282-PB-v1.3-WEB.pdf
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> >  src/libcamera/camera_sensor_properties.cpp | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp
> > index e5f27f06eb1d..3a6682f37707 100644
> > --- a/src/libcamera/camera_sensor_properties.cpp
> > +++ b/src/libcamera/camera_sensor_properties.cpp
> > @@ -160,6 +160,12 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> >                                  */
> >                         },
> >                 } },
> > +               { "ov9281", {
> > +                       .unitCellSize = { 3000, 3000 },
> > +                       .testPatternModes = {
> > +                               { controls::draft::TestPatternModeOff, 0 },
> > +                       },
> > +               } },
> >                 { "ov13858", {
> >                         .unitCellSize = { 1120, 1120 },
> >                         .testPatternModes =  {
> > --
> > 2.34.1
> >