[libcamera-devel,v2] libcamera: Add support for OmniVision OV8858
diff mbox series

Message ID 20230125090528.46218-1-jacopo.mondi@ideasonboard.com
State Accepted
Headers show
Series
  • [libcamera-devel,v2] libcamera: Add support for OmniVision OV8858
Related show

Commit Message

Jacopo Mondi Jan. 25, 2023, 9:05 a.m. UTC 
From: Nicholas Roth <nicholas@rothemail.net>

Support for the OmniVision OV8858 sensor is scheduled for inclusion in
the Linux kernel in version v6.3.

Add support for the sensor in libcamera by providing static properties
and a camera sensor helper in libipa.

The camera sensor helper expresses analogue gain increments in 1/128
step which differs from what is reported in the sensor documentation in
section "5.8 manual exposure compensation/ manual gain compensation" [0]

A more detailed analysis of the sensor gain model is reported at:
https://patchwork.linuxtv.org/project/linux-media/patch/20221106171129.166892-2-nicholas@rothemail.net/#142267

Record with a \todo note a reference to discussion on the gain model
implementation.

Signed-off-by: Nicholas Roth <nicholas@rothemail.net>
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---

Compared to initial Nicholas' submission:
- Change gain step to 128 (link to the driver discussion)
- Add fadeToGray test patter and adjust comment

---
 src/ipa/libipa/camera_sensor_helper.cpp    | 18 ++++++++++++++++++
 src/libcamera/camera_sensor_properties.cpp | 13 +++++++++++++
 2 files changed, 31 insertions(+)

--
2.39.0

Comments

Kieran Bingham Jan. 25, 2023, 10:54 a.m. UTC | #1 
Quoting Jacopo Mondi via libcamera-devel (2023-01-25 09:05:28)
> From: Nicholas Roth <nicholas@rothemail.net>
> 
> Support for the OmniVision OV8858 sensor is scheduled for inclusion in
> the Linux kernel in version v6.3.
> 
> Add support for the sensor in libcamera by providing static properties
> and a camera sensor helper in libipa.
> 
> The camera sensor helper expresses analogue gain increments in 1/128
> step which differs from what is reported in the sensor documentation in
> section "5.8 manual exposure compensation/ manual gain compensation" [0]
> 
> A more detailed analysis of the sensor gain model is reported at:
> https://patchwork.linuxtv.org/project/linux-media/patch/20221106171129.166892-2-nicholas@rothemail.net/#142267
> 
> Record with a \todo note a reference to discussion on the gain model
> implementation.
> 
> Signed-off-by: Nicholas Roth <nicholas@rothemail.net>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
> 
> Compared to initial Nicholas' submission:
> - Change gain step to 128 (link to the driver discussion)
> - Add fadeToGray test patter and adjust comment
> 
> ---
>  src/ipa/libipa/camera_sensor_helper.cpp    | 18 ++++++++++++++++++
>  src/libcamera/camera_sensor_properties.cpp | 13 +++++++++++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> index 3d8a2835fa28..52e11dfb8ca2 100644
> --- a/src/ipa/libipa/camera_sensor_helper.cpp
> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> @@ -505,6 +505,24 @@ public:
>  };
>  REGISTER_CAMERA_SENSOR_HELPER("ov5693", CameraSensorHelperOv5693)
> 
> +class CameraSensorHelperOv8858 : public CameraSensorHelper
> +{
> +public:
> +       CameraSensorHelperOv8858()
> +       {
> +               gainType_ = AnalogueGainLinear;
> +
> +               /*
> +                * \todo Use an increment step of 1/128 which differs from
> +                * what the sensor manual describes.

This sounds like the todo is to *use* a step of 128, but isn't that what
it's already doing? 

> +                *
> +                * See: https://patchwork.linuxtv.org/project/linux-media/patch/20221106171129.166892-2-nicholas@rothemail.net/#142267
> +                */
> +               gainConstants_.linear = { 1, 0, 0, 128 };
> +       }
> +};
> +REGISTER_CAMERA_SENSOR_HELPER("ov8858", CameraSensorHelperOv8858)
> +
>  class CameraSensorHelperOv8865 : public CameraSensorHelper
>  {
>  public:
> diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp
> index c3c2caced906..7f6816e72773 100644
> --- a/src/libcamera/camera_sensor_properties.cpp
> +++ b/src/libcamera/camera_sensor_properties.cpp
> @@ -167,6 +167,19 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>                                  */
>                         },
>                 } },
> +               { "ov8858", {
> +                       .unitCellSize = { 1120, 1120 },
> +                       .testPatternModes = {
> +                               { controls::draft::TestPatternModeOff, 0 },
> +                               { controls::draft::TestPatternModeColorBars, 1 },
> +                               { controls::draft::TestPatternModeColorBarsFadeToGray, 2 },
> +                               /*
> +                                * No corresponding test patter mode

s/patter/pattern/


> +                                * 3: "Vertical Color Bar Type 3",
> +                                * 4: "Vertical Color Bar Type 4"
> +                                */
> +                       },
> +               } },
>                 { "ov8865", {
>                         .unitCellSize = { 1400, 1400 },
>                         .testPatternModes = {
> --
> 2.39.0
>
Jacopo Mondi Jan. 26, 2023, 5:11 p.m. UTC | #2 
Hi Kieran

On Wed, Jan 25, 2023 at 10:54:26AM +0000, Kieran Bingham via libcamera-devel wrote:
> Quoting Jacopo Mondi via libcamera-devel (2023-01-25 09:05:28)
> > From: Nicholas Roth <nicholas@rothemail.net>
> >
> > Support for the OmniVision OV8858 sensor is scheduled for inclusion in
> > the Linux kernel in version v6.3.
> >
> > Add support for the sensor in libcamera by providing static properties
> > and a camera sensor helper in libipa.
> >
> > The camera sensor helper expresses analogue gain increments in 1/128
> > step which differs from what is reported in the sensor documentation in
> > section "5.8 manual exposure compensation/ manual gain compensation" [0]
> >
> > A more detailed analysis of the sensor gain model is reported at:
> > https://patchwork.linuxtv.org/project/linux-media/patch/20221106171129.166892-2-nicholas@rothemail.net/#142267
> >
> > Record with a \todo note a reference to discussion on the gain model
> > implementation.
> >
> > Signed-off-by: Nicholas Roth <nicholas@rothemail.net>
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >
> > Compared to initial Nicholas' submission:
> > - Change gain step to 128 (link to the driver discussion)
> > - Add fadeToGray test patter and adjust comment
> >
> > ---
> >  src/ipa/libipa/camera_sensor_helper.cpp    | 18 ++++++++++++++++++
> >  src/libcamera/camera_sensor_properties.cpp | 13 +++++++++++++
> >  2 files changed, 31 insertions(+)
> >
> > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> > index 3d8a2835fa28..52e11dfb8ca2 100644
> > --- a/src/ipa/libipa/camera_sensor_helper.cpp
> > +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> > @@ -505,6 +505,24 @@ public:
> >  };
> >  REGISTER_CAMERA_SENSOR_HELPER("ov5693", CameraSensorHelperOv5693)
> >
> > +class CameraSensorHelperOv8858 : public CameraSensorHelper
> > +{
> > +public:
> > +       CameraSensorHelperOv8858()
> > +       {
> > +               gainType_ = AnalogueGainLinear;
> > +
> > +               /*
> > +                * \todo Use an increment step of 1/128 which differs from
> > +                * what the sensor manual describes.
>
> This sounds like the todo is to *use* a step of 128, but isn't that what
> it's already doing?
>

Right :)

I can change it to
                     * \todo Validate the selected 1/128 step value
                     * as it differs from what the sensor manual
                     * describes
                     *
                     * See: https://patchwork.linuxtv.org/project/linux-media/patch/20221106171129.166892-2-nicholas@rothemail.net/#142267
                     */

When applying


> > +               gainConstants_.linear = { 1, 0, 0, 128 };
> > +       }
> > +};
> > +REGISTER_CAMERA_SENSOR_HELPER("ov8858", CameraSensorHelperOv8858)
> > +
> >  class CameraSensorHelperOv8865 : public CameraSensorHelper
> >  {
> >  public:
> > diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp
> > index c3c2caced906..7f6816e72773 100644
> > --- a/src/libcamera/camera_sensor_properties.cpp
> > +++ b/src/libcamera/camera_sensor_properties.cpp
> > @@ -167,6 +167,19 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> >                                  */
> >                         },
> >                 } },
> > +               { "ov8858", {
> > +                       .unitCellSize = { 1120, 1120 },
> > +                       .testPatternModes = {
> > +                               { controls::draft::TestPatternModeOff, 0 },
> > +                               { controls::draft::TestPatternModeColorBars, 1 },
> > +                               { controls::draft::TestPatternModeColorBarsFadeToGray, 2 },
> > +                               /*
> > +                                * No corresponding test patter mode
>
> s/patter/pattern/
>
>
> > +                                * 3: "Vertical Color Bar Type 3",
> > +                                * 4: "Vertical Color Bar Type 4"
> > +                                */
> > +                       },
> > +               } },
> >                 { "ov8865", {
> >                         .unitCellSize = { 1400, 1400 },
> >                         .testPatternModes = {
> > --
> > 2.39.0
> >
Kieran Bingham Jan. 26, 2023, 5:24 p.m. UTC | #3 
Quoting Jacopo Mondi (2023-01-26 17:11:46)
> Hi Kieran
> 
> On Wed, Jan 25, 2023 at 10:54:26AM +0000, Kieran Bingham via libcamera-devel wrote:
> > Quoting Jacopo Mondi via libcamera-devel (2023-01-25 09:05:28)
> > > From: Nicholas Roth <nicholas@rothemail.net>
> > >
> > > Support for the OmniVision OV8858 sensor is scheduled for inclusion in
> > > the Linux kernel in version v6.3.
> > >
> > > Add support for the sensor in libcamera by providing static properties
> > > and a camera sensor helper in libipa.
> > >
> > > The camera sensor helper expresses analogue gain increments in 1/128
> > > step which differs from what is reported in the sensor documentation in
> > > section "5.8 manual exposure compensation/ manual gain compensation" [0]
> > >
> > > A more detailed analysis of the sensor gain model is reported at:
> > > https://patchwork.linuxtv.org/project/linux-media/patch/20221106171129.166892-2-nicholas@rothemail.net/#142267
> > >
> > > Record with a \todo note a reference to discussion on the gain model
> > > implementation.
> > >
> > > Signed-off-by: Nicholas Roth <nicholas@rothemail.net>
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > ---
> > >
> > > Compared to initial Nicholas' submission:
> > > - Change gain step to 128 (link to the driver discussion)
> > > - Add fadeToGray test patter and adjust comment
> > >
> > > ---
> > >  src/ipa/libipa/camera_sensor_helper.cpp    | 18 ++++++++++++++++++
> > >  src/libcamera/camera_sensor_properties.cpp | 13 +++++++++++++
> > >  2 files changed, 31 insertions(+)
> > >
> > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> > > index 3d8a2835fa28..52e11dfb8ca2 100644
> > > --- a/src/ipa/libipa/camera_sensor_helper.cpp
> > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> > > @@ -505,6 +505,24 @@ public:
> > >  };
> > >  REGISTER_CAMERA_SENSOR_HELPER("ov5693", CameraSensorHelperOv5693)
> > >
> > > +class CameraSensorHelperOv8858 : public CameraSensorHelper
> > > +{
> > > +public:
> > > +       CameraSensorHelperOv8858()
> > > +       {
> > > +               gainType_ = AnalogueGainLinear;
> > > +
> > > +               /*
> > > +                * \todo Use an increment step of 1/128 which differs from
> > > +                * what the sensor manual describes.
> >
> > This sounds like the todo is to *use* a step of 128, but isn't that what
> > it's already doing?
> >
> 
> Right :)
> 
> I can change it to
>                      * \todo Validate the selected 1/128 step value
>                      * as it differs from what the sensor manual
>                      * describes
>                      *
>                      * See: https://patchwork.linuxtv.org/project/linux-media/patch/20221106171129.166892-2-nicholas@rothemail.net/#142267
>                      */
> 
> When applying

Works for me ;-)


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> 
> 
> > > +               gainConstants_.linear = { 1, 0, 0, 128 };
> > > +       }
> > > +};
> > > +REGISTER_CAMERA_SENSOR_HELPER("ov8858", CameraSensorHelperOv8858)
> > > +
> > >  class CameraSensorHelperOv8865 : public CameraSensorHelper
> > >  {
> > >  public:
> > > diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp
> > > index c3c2caced906..7f6816e72773 100644
> > > --- a/src/libcamera/camera_sensor_properties.cpp
> > > +++ b/src/libcamera/camera_sensor_properties.cpp
> > > @@ -167,6 +167,19 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > >                                  */
> > >                         },
> > >                 } },
> > > +               { "ov8858", {
> > > +                       .unitCellSize = { 1120, 1120 },
> > > +                       .testPatternModes = {
> > > +                               { controls::draft::TestPatternModeOff, 0 },
> > > +                               { controls::draft::TestPatternModeColorBars, 1 },
> > > +                               { controls::draft::TestPatternModeColorBarsFadeToGray, 2 },
> > > +                               /*
> > > +                                * No corresponding test patter mode
> >
> > s/patter/pattern/
> >
> >
> > > +                                * 3: "Vertical Color Bar Type 3",
> > > +                                * 4: "Vertical Color Bar Type 4"
> > > +                                */
> > > +                       },
> > > +               } },
> > >                 { "ov8865", {
> > >                         .unitCellSize = { 1400, 1400 },
> > >                         .testPatternModes = {
> > > --
> > > 2.39.0
> > >
Laurent Pinchart Jan. 26, 2023, 5:53 p.m. UTC | #4 
On Thu, Jan 26, 2023 at 05:24:25PM +0000, Kieran Bingham via libcamera-devel wrote:
> Quoting Jacopo Mondi (2023-01-26 17:11:46)
> > On Wed, Jan 25, 2023 at 10:54:26AM +0000, Kieran Bingham via libcamera-devel wrote:
> > > Quoting Jacopo Mondi via libcamera-devel (2023-01-25 09:05:28)
> > > > From: Nicholas Roth <nicholas@rothemail.net>
> > > >
> > > > Support for the OmniVision OV8858 sensor is scheduled for inclusion in
> > > > the Linux kernel in version v6.3.
> > > >
> > > > Add support for the sensor in libcamera by providing static properties
> > > > and a camera sensor helper in libipa.
> > > >
> > > > The camera sensor helper expresses analogue gain increments in 1/128
> > > > step which differs from what is reported in the sensor documentation in
> > > > section "5.8 manual exposure compensation/ manual gain compensation" [0]
> > > >
> > > > A more detailed analysis of the sensor gain model is reported at:
> > > > https://patchwork.linuxtv.org/project/linux-media/patch/20221106171129.166892-2-nicholas@rothemail.net/#142267
> > > >
> > > > Record with a \todo note a reference to discussion on the gain model
> > > > implementation.
> > > >
> > > > Signed-off-by: Nicholas Roth <nicholas@rothemail.net>
> > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > ---
> > > >
> > > > Compared to initial Nicholas' submission:
> > > > - Change gain step to 128 (link to the driver discussion)
> > > > - Add fadeToGray test patter and adjust comment
> > > >
> > > > ---
> > > >  src/ipa/libipa/camera_sensor_helper.cpp    | 18 ++++++++++++++++++
> > > >  src/libcamera/camera_sensor_properties.cpp | 13 +++++++++++++
> > > >  2 files changed, 31 insertions(+)
> > > >
> > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> > > > index 3d8a2835fa28..52e11dfb8ca2 100644
> > > > --- a/src/ipa/libipa/camera_sensor_helper.cpp
> > > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> > > > @@ -505,6 +505,24 @@ public:
> > > >  };
> > > >  REGISTER_CAMERA_SENSOR_HELPER("ov5693", CameraSensorHelperOv5693)
> > > >
> > > > +class CameraSensorHelperOv8858 : public CameraSensorHelper
> > > > +{
> > > > +public:
> > > > +       CameraSensorHelperOv8858()
> > > > +       {
> > > > +               gainType_ = AnalogueGainLinear;
> > > > +
> > > > +               /*
> > > > +                * \todo Use an increment step of 1/128 which differs from
> > > > +                * what the sensor manual describes.
> > >
> > > This sounds like the todo is to *use* a step of 128, but isn't that what
> > > it's already doing?
> > >
> > 
> > Right :)
> > 
> > I can change it to
> >                      * \todo Validate the selected 1/128 step value
> >                      * as it differs from what the sensor manual
> >                      * describes
> >                      *
> >                      * See: https://patchwork.linuxtv.org/project/linux-media/patch/20221106171129.166892-2-nicholas@rothemail.net/#142267
> >                      */
> > 
> > When applying
> 
> Works for me ;-)
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > > > +               gainConstants_.linear = { 1, 0, 0, 128 };
> > > > +       }
> > > > +};
> > > > +REGISTER_CAMERA_SENSOR_HELPER("ov8858", CameraSensorHelperOv8858)
> > > > +
> > > >  class CameraSensorHelperOv8865 : public CameraSensorHelper
> > > >  {
> > > >  public:
> > > > diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp
> > > > index c3c2caced906..7f6816e72773 100644
> > > > --- a/src/libcamera/camera_sensor_properties.cpp
> > > > +++ b/src/libcamera/camera_sensor_properties.cpp
> > > > @@ -167,6 +167,19 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > > >                                  */
> > > >                         },
> > > >                 } },
> > > > +               { "ov8858", {
> > > > +                       .unitCellSize = { 1120, 1120 },
> > > > +                       .testPatternModes = {
> > > > +                               { controls::draft::TestPatternModeOff, 0 },
> > > > +                               { controls::draft::TestPatternModeColorBars, 1 },
> > > > +                               { controls::draft::TestPatternModeColorBarsFadeToGray, 2 },
> > > > +                               /*
> > > > +                                * No corresponding test patter mode
> > >
> > > s/patter/pattern/
> > >
> > > > +                                * 3: "Vertical Color Bar Type 3",
> > > > +                                * 4: "Vertical Color Bar Type 4"
> > > > +                                */
> > > > +                       },
> > > > +               } },
> > > >                 { "ov8865", {
> > > >                         .unitCellSize = { 1400, 1400 },
> > > >                         .testPatternModes = {

Patch
diff mbox series 

diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
index 3d8a2835fa28..52e11dfb8ca2 100644
--- a/src/ipa/libipa/camera_sensor_helper.cpp
+++ b/src/ipa/libipa/camera_sensor_helper.cpp
@@ -505,6 +505,24 @@  public:
 };
 REGISTER_CAMERA_SENSOR_HELPER("ov5693", CameraSensorHelperOv5693)

+class CameraSensorHelperOv8858 : public CameraSensorHelper
+{
+public:
+	CameraSensorHelperOv8858()
+	{
+		gainType_ = AnalogueGainLinear;
+
+		/*
+		 * \todo Use an increment step of 1/128 which differs from
+		 * what the sensor manual describes.
+		 *
+		 * See: https://patchwork.linuxtv.org/project/linux-media/patch/20221106171129.166892-2-nicholas@rothemail.net/#142267
+		 */
+		gainConstants_.linear = { 1, 0, 0, 128 };
+	}
+};
+REGISTER_CAMERA_SENSOR_HELPER("ov8858", CameraSensorHelperOv8858)
+
 class CameraSensorHelperOv8865 : public CameraSensorHelper
 {
 public:
diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp
index c3c2caced906..7f6816e72773 100644
--- a/src/libcamera/camera_sensor_properties.cpp
+++ b/src/libcamera/camera_sensor_properties.cpp
@@ -167,6 +167,19 @@  const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
 				 */
 			},
 		} },
+		{ "ov8858", {
+			.unitCellSize = { 1120, 1120 },
+			.testPatternModes = {
+				{ controls::draft::TestPatternModeOff, 0 },
+				{ controls::draft::TestPatternModeColorBars, 1 },
+				{ controls::draft::TestPatternModeColorBarsFadeToGray, 2 },
+				/*
+				 * No corresponding test patter mode
+				 * 3: "Vertical Color Bar Type 3",
+				 * 4: "Vertical Color Bar Type 4"
+				 */
+			},
+		} },
 		{ "ov8865", {
 			.unitCellSize = { 1400, 1400 },
 			.testPatternModes = {