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

[Dave Stevenson]

HI Kieran & Laurent

On Sat, 12 Nov 2022 at 20:55, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Kieran
>
> On Fri, Nov 11, 2022 at 11:54:16PM +0000, Kieran Bingham via libcamera-devel wrote:
> > Quoting Dave Stevenson (2022-11-11 17:00:25)
> > > On Fri, 11 Nov 2022 at 16:37, Kieran Bingham 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.

Sakari's latest pull for 6.2 has just picked the main series, but not
the regulator support (needs a review on the driver changes, but DT
has been Acked-by RobH). The regulator series has been tagged as
"Under Review" in Patchwork for longer than the main series, but
perhaps that means nothing.

At least I can go and fix up my Pi kernel PR with the accepted
patches, and the Pi kernel will be switching over.

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

My pending PR picks up patches so that the ov9281 compatible string is
added to the driver and it follows through to the sensor name, so it
makes no difference to us in userspace.
For those using ov9281 on other platforms, they need to make their own
choice on extra patches or userspace name.

> > 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?
>
> There's not much data to duplicate at the moment, so I don't mind
> separate entries, but matching on a set of identifiers is fine too.
>
> > 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 ?
>
> I'd expect that to affect the tuning file mostly, not the helpers.

Most likely lens shading, but that will change based on the lens
attached anyway and we have no way of representing that in the
system.....

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

But where is the desperate desire to be able to control test patterns
within libcamera? It seems a huge amount of effort to support for very
little real world use. Just my opinion.

  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 =  {
>
> --
> Regards,
>
> Laurent Pinchart