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

[Laurent Pinchart]

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

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.

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