| Message ID | 20230227204254.3965883-3-kieran.bingham@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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@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 > 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.. > + .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 Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > + } }, > { "ov8858", { > .unitCellSize = { 1120, 1120 }, > .testPatternModes = { > -- > 2.34.1 >
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@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 > > > 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@ideasonboard.com> Thanks > > Thanks > j > > > + } }, > > { "ov8858", { > > .unitCellSize = { 1120, 1120 }, > > .testPatternModes = { > > -- > > 2.34.1 > >
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@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 ?) > > > > > 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@ideasonboard.com> > > > Thanks > > > > > > Thanks > > j > > > > > + } }, > > > { "ov8858", { > > > .unitCellSize = { 1120, 1120 }, > > > .testPatternModes = { > > > -- > > > 2.34.1 > > >
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@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@ideasonboard.com> > > > > > > Thanks > > > > > > > > > > Thanks > > > j > > > > > > > + } }, > > > > { "ov8858", { > > > > .unitCellSize = { 1120, 1120 }, > > > > .testPatternModes = { > > > > -- > > > > 2.34.1 > > > >
On 3/1/23 8:33 PM, Kieran Bingham via libcamera-devel 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@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 ?) This will help - it gives a notion that more empirical testing can be done(by users/community?) and validated the information encapsulated in Properties and helpers. But the point is whether we want to put up a global banner comment or be specific with each entry ? > > >>>> 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@ideasonboard.com> same, Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> >> >> Thanks >> >> >>> Thanks >>> j >>> >>>> + } }, >>>> { "ov8858", { >>>> .unitCellSize = { 1120, 1120 }, >>>> .testPatternModes = { >>>> -- >>>> 2.34.1 >>>>
Hi Kieran, Thank you for the patch. 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@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 }; This seems a bit weird, given that the driver sets the minimum gain to 0x10 and the maximum (and default) gain to 0xf8. I'd be surprised if the sensors supported gains lower than one, and particularly as low as 0.125. A 1/16 gain step would be more plausible. > + } > +}; > +REGISTER_CAMERA_SENSOR_HELPER("ov5695", CameraSensorHelperOv5695) > + > 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 }, > + .testPatternModes = { > + { controls::draft::TestPatternModeOff, 0 }, > + { controls::draft::TestPatternModeColorBars, 2 }, > + { controls::draft::TestPatternModeColorBarsFadeToGray, 4}, > + }, > + } }, > { "ov8858", { > .unitCellSize = { 1120, 1120 }, > .testPatternModes = {
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) + 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 }, + .testPatternModes = { + { controls::draft::TestPatternModeOff, 0 }, + { controls::draft::TestPatternModeColorBars, 2 }, + { controls::draft::TestPatternModeColorBarsFadeToGray, 4}, + }, + } }, { "ov8858", { .unitCellSize = { 1120, 1120 }, .testPatternModes = {
Provide a CameraSensorHelper for the OV5695, along with the corresponding camera sensor properties. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/ipa/libipa/camera_sensor_helper.cpp | 11 +++++++++++ src/libcamera/camera_sensor_properties.cpp | 8 ++++++++ 2 files changed, 19 insertions(+)