| Message ID | 20210630112126.2263641-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thanks for the patch. On 30/06/2021 13:21, Kieran Bingham wrote: > Extend the CameraSensorHelper factory with support for an > OV13858 sensor as found in the Soraka Chromebook. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/ipa/libipa/camera_sensor_helper.cpp | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp > index 23335ed66b02..8e95b8b10102 100644 > --- a/src/ipa/libipa/camera_sensor_helper.cpp > +++ b/src/ipa/libipa/camera_sensor_helper.cpp > @@ -313,6 +313,20 @@ public: > }; > REGISTER_CAMERA_SENSOR_HELPER("ov5693", CameraSensorHelperOv5693) > > +/** > + * \class CameraSensorHelperOv13858 > + * \brief Create and give helpers for the ov13858 sensor > + */ As proposed by Laurent, I have a patch which (among other things) removes the Doxygen comments for the sub-classes as it is not really needed: https://patchwork.libcamera.org/patch/12736/ > +class CameraSensorHelperOv13858 : public CameraSensorHelper > +{ > +public: > + CameraSensorHelperOv13858() > + { > + analogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 256 }; This is based on minimum, maximum and step value, as you don't have the datasheet... That's something we should be thinking about: how to be sure of the values when we don't have the datasheet. A tuning code which would set the gains from min to max and compare the raw pixel values from one gain to another to deduce a linear (or not) relationship ? > + } > +}; > +REGISTER_CAMERA_SENSOR_HELPER("ov13858", CameraSensorHelperOv13858) > + > } /* namespace ipa */ > > } /* namespace libcamera */ >
On Wed, 30 Jun 2021 at 12:26, Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> wrote: > > Hi Kieran, > > Thanks for the patch. > > On 30/06/2021 13:21, Kieran Bingham wrote: > > Extend the CameraSensorHelper factory with support for an > > OV13858 sensor as found in the Soraka Chromebook. > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > src/ipa/libipa/camera_sensor_helper.cpp | 14 ++++++++++++++ > > 1 file changed, 14 insertions(+) > > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp > > index 23335ed66b02..8e95b8b10102 100644 > > --- a/src/ipa/libipa/camera_sensor_helper.cpp > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp > > @@ -313,6 +313,20 @@ public: > > }; > > REGISTER_CAMERA_SENSOR_HELPER("ov5693", CameraSensorHelperOv5693) > > > > +/** > > + * \class CameraSensorHelperOv13858 > > + * \brief Create and give helpers for the ov13858 sensor > > + */ > > As proposed by Laurent, I have a patch which (among other things) > removes the Doxygen comments for the sub-classes as it is not really > needed: https://patchwork.libcamera.org/patch/12736/ > > > +class CameraSensorHelperOv13858 : public CameraSensorHelper > > +{ > > +public: > > + CameraSensorHelperOv13858() > > + { > > + analogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 256 }; > > This is based on minimum, maximum and step value, as you don't have the > datasheet... OV13850 appears to be a very similar 10bit 4224x3136 sensor, and there is a datasheet online at http://download.t-firefly.com/product/RK3288/Docs/Peripherals/OV13850%20datasheet/Sensor_OV13850-G04A_OmniVision_SpecificationV1.pdf It matches the driver with analogue gain being registers 0x3508/9, and confirms that the "Low 4 bits are fraction bits which are not supported and should always be 0". It's making some assumptions, but seems a reasonable match. Dave > That's something we should be thinking about: how to be sure of the > values when we don't have the datasheet. > A tuning code which would set the gains from min to max and compare the > raw pixel values from one gain to another to deduce a linear (or not) > relationship ? > > > + } > > +}; > > +REGISTER_CAMERA_SENSOR_HELPER("ov13858", CameraSensorHelperOv13858) > > + > > } /* namespace ipa */ > > > > } /* namespace libcamera */ > >
Hi Dave, On Wed, Jun 30, 2021 at 01:57:12PM +0100, Dave Stevenson wrote: > On Wed, 30 Jun 2021 at 12:26, Jean-Michel Hautbois wrote: > > On 30/06/2021 13:21, Kieran Bingham wrote: > > > Extend the CameraSensorHelper factory with support for an > > > OV13858 sensor as found in the Soraka Chromebook. > > > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > --- > > > src/ipa/libipa/camera_sensor_helper.cpp | 14 ++++++++++++++ > > > 1 file changed, 14 insertions(+) > > > > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp > > > index 23335ed66b02..8e95b8b10102 100644 > > > --- a/src/ipa/libipa/camera_sensor_helper.cpp > > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp > > > @@ -313,6 +313,20 @@ public: > > > }; > > > REGISTER_CAMERA_SENSOR_HELPER("ov5693", CameraSensorHelperOv5693) > > > > > > +/** > > > + * \class CameraSensorHelperOv13858 > > > + * \brief Create and give helpers for the ov13858 sensor > > > + */ > > > > As proposed by Laurent, I have a patch which (among other things) > > removes the Doxygen comments for the sub-classes as it is not really > > needed: https://patchwork.libcamera.org/patch/12736/ > > > > > +class CameraSensorHelperOv13858 : public CameraSensorHelper > > > +{ > > > +public: > > > + CameraSensorHelperOv13858() > > > + { > > > + analogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 256 }; > > > > This is based on minimum, maximum and step value, as you don't have the > > datasheet... > > OV13850 appears to be a very similar 10bit 4224x3136 sensor, and there > is a datasheet online at > http://download.t-firefly.com/product/RK3288/Docs/Peripherals/OV13850%20datasheet/Sensor_OV13850-G04A_OmniVision_SpecificationV1.pdf > > It matches the driver with analogue gain being registers 0x3508/9, and Does it ? The OV13850 datasheet lists register 0x3509 as a control register, and the manual gain is set in 0x3504/0x3505 or 0x350a/0x350b. It's probably similar though. > confirms that the "Low 4 bits are fraction bits which are not > supported and should always be 0". > It's making some assumptions, but seems a reasonable match. > > > That's something we should be thinking about: how to be sure of the > > values when we don't have the datasheet. > > A tuning code which would set the gains from min to max and compare the > > raw pixel values from one gain to another to deduce a linear (or not) > > relationship ? > > > > > + } > > > +}; > > > +REGISTER_CAMERA_SENSOR_HELPER("ov13858", CameraSensorHelperOv13858) > > > + > > > } /* namespace ipa */ > > > > > > } /* namespace libcamera */ > > >
Hi Laurent On Wed, 30 Jun 2021 at 14:05, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Dave, > > On Wed, Jun 30, 2021 at 01:57:12PM +0100, Dave Stevenson wrote: > > On Wed, 30 Jun 2021 at 12:26, Jean-Michel Hautbois wrote: > > > On 30/06/2021 13:21, Kieran Bingham wrote: > > > > Extend the CameraSensorHelper factory with support for an > > > > OV13858 sensor as found in the Soraka Chromebook. > > > > > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > --- > > > > src/ipa/libipa/camera_sensor_helper.cpp | 14 ++++++++++++++ > > > > 1 file changed, 14 insertions(+) > > > > > > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp > > > > index 23335ed66b02..8e95b8b10102 100644 > > > > --- a/src/ipa/libipa/camera_sensor_helper.cpp > > > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp > > > > @@ -313,6 +313,20 @@ public: > > > > }; > > > > REGISTER_CAMERA_SENSOR_HELPER("ov5693", CameraSensorHelperOv5693) > > > > > > > > +/** > > > > + * \class CameraSensorHelperOv13858 > > > > + * \brief Create and give helpers for the ov13858 sensor > > > > + */ > > > > > > As proposed by Laurent, I have a patch which (among other things) > > > removes the Doxygen comments for the sub-classes as it is not really > > > needed: https://patchwork.libcamera.org/patch/12736/ > > > > > > > +class CameraSensorHelperOv13858 : public CameraSensorHelper > > > > +{ > > > > +public: > > > > + CameraSensorHelperOv13858() > > > > + { > > > > + analogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 256 }; > > > > > > This is based on minimum, maximum and step value, as you don't have the > > > datasheet... > > > > OV13850 appears to be a very similar 10bit 4224x3136 sensor, and there > > is a datasheet online at > > http://download.t-firefly.com/product/RK3288/Docs/Peripherals/OV13850%20datasheet/Sensor_OV13850-G04A_OmniVision_SpecificationV1.pdf > > > > It matches the driver with analogue gain being registers 0x3508/9, and > > Does it ? The OV13850 datasheet lists register 0x3509 as a control > register, and the manual gain is set in 0x3504/0x3505 or 0x350a/0x350b. > It's probably similar though. Doh, that's me going cross-eyed! I'd misread exposure for gain in about 3 places. Sorry for the noise. Dave > > confirms that the "Low 4 bits are fraction bits which are not > > supported and should always be 0". > > It's making some assumptions, but seems a reasonable match. > > > > > That's something we should be thinking about: how to be sure of the > > > values when we don't have the datasheet. > > > A tuning code which would set the gains from min to max and compare the > > > raw pixel values from one gain to another to deduce a linear (or not) > > > relationship ? > > > > > > > + } > > > > +}; > > > > +REGISTER_CAMERA_SENSOR_HELPER("ov13858", CameraSensorHelperOv13858) > > > > + > > > > } /* namespace ipa */ > > > > > > > > } /* namespace libcamera */ > > > > > > -- > Regards, > > Laurent Pinchart
Hi JM, On 30/06/2021 12:26, Jean-Michel Hautbois wrote: > Hi Kieran, > > Thanks for the patch. > > On 30/06/2021 13:21, Kieran Bingham wrote: >> Extend the CameraSensorHelper factory with support for an >> OV13858 sensor as found in the Soraka Chromebook. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> src/ipa/libipa/camera_sensor_helper.cpp | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp >> index 23335ed66b02..8e95b8b10102 100644 >> --- a/src/ipa/libipa/camera_sensor_helper.cpp >> +++ b/src/ipa/libipa/camera_sensor_helper.cpp >> @@ -313,6 +313,20 @@ public: >> }; >> REGISTER_CAMERA_SENSOR_HELPER("ov5693", CameraSensorHelperOv5693) >> >> +/** >> + * \class CameraSensorHelperOv13858 >> + * \brief Create and give helpers for the ov13858 sensor >> + */ > > As proposed by Laurent, I have a patch which (among other things) > removes the Doxygen comments for the sub-classes as it is not really > needed: https://patchwork.libcamera.org/patch/12736/ Yes, these comments should be removed indeed. >> +class CameraSensorHelperOv13858 : public CameraSensorHelper >> +{ >> +public: >> + CameraSensorHelperOv13858() >> + { >> + analogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 256 }; > > This is based on minimum, maximum and step value, as you don't have the > datasheet... > That's something we should be thinking about: how to be sure of the > values when we don't have the datasheet. > A tuning code which would set the gains from min to max and compare the > raw pixel values from one gain to another to deduce a linear (or not) > relationship ? > Definitely interesting ideas for future sensors, fortunately it sounds like we can try to match this up against a 'close' data sheet - or try to find out if we can confirm the correct details somehow. -- Kieran >> + } >> +}; >> +REGISTER_CAMERA_SENSOR_HELPER("ov13858", CameraSensorHelperOv13858) >> + >> } /* namespace ipa */ >> >> } /* namespace libcamera */ >>
diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp index 23335ed66b02..8e95b8b10102 100644 --- a/src/ipa/libipa/camera_sensor_helper.cpp +++ b/src/ipa/libipa/camera_sensor_helper.cpp @@ -313,6 +313,20 @@ public: }; REGISTER_CAMERA_SENSOR_HELPER("ov5693", CameraSensorHelperOv5693) +/** + * \class CameraSensorHelperOv13858 + * \brief Create and give helpers for the ov13858 sensor + */ +class CameraSensorHelperOv13858 : public CameraSensorHelper +{ +public: + CameraSensorHelperOv13858() + { + analogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 256 }; + } +}; +REGISTER_CAMERA_SENSOR_HELPER("ov13858", CameraSensorHelperOv13858) + } /* namespace ipa */ } /* namespace libcamera */
Extend the CameraSensorHelper factory with support for an OV13858 sensor as found in the Soraka Chromebook. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/ipa/libipa/camera_sensor_helper.cpp | 14 ++++++++++++++ 1 file changed, 14 insertions(+)