[libcamera-devel] [PATCH 2/2] ipa: libipa: Add OV5695 Camera Sensor Helper

[Jacopo Mondi]

Hi Kieran

On Wed, Mar 01, 2023 at 03:03:13PM +0000, Kieran Bingham wrote:
> Quoting Kieran Bingham (2023-03-01 14:55:25)
> > Quoting Jacopo Mondi (2023-02-28 15:33:22)
> > > Hi Kieran
> > >
> > > On Mon, Feb 27, 2023 at 08:42:54PM +0000, Kieran Bingham via libcamera-devel wrote:
> > > > Provide a CameraSensorHelper for the OV5695, along with the
> > > > corresponding camera sensor properties.
> > > >
> > > > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > > ---
> > > >  src/ipa/libipa/camera_sensor_helper.cpp    | 11 +++++++++++
> > > >  src/libcamera/camera_sensor_properties.cpp |  8 ++++++++
> > > >  2 files changed, 19 insertions(+)
> > > >
> > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> > > > index d1051cc25656..a38fefc75372 100644
> > > > --- a/src/ipa/libipa/camera_sensor_helper.cpp
> > > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> > > > @@ -527,6 +527,17 @@ public:
> > > >  };
> > > >  REGISTER_CAMERA_SENSOR_HELPER("ov5693", CameraSensorHelperOv5693)
> > > >
> > > > +class CameraSensorHelperOv5695 : public CameraSensorHelper
> > > > +{
> > > > +public:
> > > > +     CameraSensorHelperOv5695()
> > > > +     {
> > > > +             gainType_ = AnalogueGainLinear;
> > > > +             gainConstants_.linear = { 1, 0, 0, 128 };
> > > > +     }
> > > > +};
> > > > +REGISTER_CAMERA_SENSOR_HELPER("ov5695", CameraSensorHelperOv5695)
> > > > +
> > >
> > > I don't have a datasheet, but again, seeing this in action it seems
> > > reasonable
>
> Me neither. But I wonder if that will be most sensors we have to add
> here. I could also add:
>
> +               /* This has been validated with some empirical testing only. */
>
> But that might be applicable to 'most' of the CameraSensorHelpers?
> Do we need to have a way to mark which ones would benefit from extra
> validation? (Maybe a comment is enough ?)
>

For now I would be fine with a comment and I'm fine with the comment
you proposed in the other patch in the series.

>
>
> > >
> > > >  class CameraSensorHelperOv8858 : public CameraSensorHelper
> > > >  {
> > > >  public:
> > > > diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp
> > > > index 7652c5f3e24c..a92c13b87b7f 100644
> > > > --- a/src/libcamera/camera_sensor_properties.cpp
> > > > +++ b/src/libcamera/camera_sensor_properties.cpp
> > > > @@ -204,6 +204,14 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > > >                                */
> > > >                       },
> > > >               } },
> > > > +             { "ov5695", {
> > > > +                     .unitCellSize = { 1400, 1400 },
> > >
> > > Can't validate this without a datasheet..
> >
> > It's listed in the publicly available product brief:
> >
> >  pixel size: 1.4 μm x 1.4 μm
> >
> >
> >
> > >
> > > > +                     .testPatternModes = {
> > > > +                             { controls::draft::TestPatternModeOff, 0 },
> > > > +                             { controls::draft::TestPatternModeColorBars, 2 },
> > > > +                             { controls::draft::TestPatternModeColorBarsFadeToGray, 4},
> > > > +                     },
> > >
> > > The driver reports
> > >
> > > static const char * const ov5695_test_pattern_menu[] = {
> > >         "Disabled",
> > >         "Vertical Color Bar Type 1",
> > >         "Vertical Color Bar Type 2",
> > >         "Vertical Color Bar Type 3",
> > >         "Vertical Color Bar Type 4"
> > > };
> > >
> > > Have you inspected the produced test pattern ?
> > > If so
> >
> > Yes, that was my best identifications from enabling the test patterns.
> > Annoyingly - the AGC/AWB quite quickly destroys the TPG patterns so you
> > have to be quick to see what's output. But that's a separate (and I
> > think known) issue...
> >
> > > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> >
> >
> > Thanks
> >
> >
> > >
> > > Thanks
> > >   j
> > >
> > > > +             } },
> > > >               { "ov8858", {
> > > >                       .unitCellSize = { 1120, 1120 },
> > > >                       .testPatternModes = {
> > > > --
> > > > 2.34.1
> > > >