[libcamera-devel,1/2] ipa: libipa: Add OV2685 Camera Sensor Helper
diff mbox series

Message ID 20230227204254.3965883-2-kieran.bingham@ideasonboard.com
State Accepted
Headers show
Series
  • ipa: libipa: Add sensor helpers for the Acer Chrometab 10
Related show

Commit Message

Kieran Bingham Feb. 27, 2023, 8:42 p.m. UTC 
Provide a CameraSensorHelper for the OV2685, 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 | 14 ++++++++++++++
 2 files changed, 25 insertions(+)

Comments

Jacopo Mondi Feb. 28, 2023, 3:30 p.m. UTC | #1 
Hi Kieran

On Mon, Feb 27, 2023 at 08:42:53PM +0000, Kieran Bingham via libcamera-devel wrote:
> Provide a CameraSensorHelper for the OV2685, 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 | 14 ++++++++++++++
>  2 files changed, 25 insertions(+)
>
> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> index 7977d7eb6c07..d1051cc25656 100644
> --- a/src/ipa/libipa/camera_sensor_helper.cpp
> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> @@ -450,6 +450,17 @@ public:
>  };
>  REGISTER_CAMERA_SENSOR_HELPER("imx477", CameraSensorHelperImx477)
>
> +class CameraSensorHelperOv2685 : public CameraSensorHelper
> +{
> +public:
> +	CameraSensorHelperOv2685()
> +	{
> +		gainType_ = AnalogueGainLinear;
> +		gainConstants_.linear = { 1, 0, 0, 128 };

The sensor manual I have doesn't specify a gain model at all.

I've seen this working and it's reasonable, I would add a coment to
record that the gain model is not documented, but otherwise... ship
it!

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

> +	}
> +};
> +REGISTER_CAMERA_SENSOR_HELPER("ov2685", CameraSensorHelperOv2685)
> +
>  class CameraSensorHelperOv2740 : public CameraSensorHelper
>  {
>  public:
> diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp
> index b8c532699684..7652c5f3e24c 100644
> --- a/src/libcamera/camera_sensor_properties.cpp
> +++ b/src/libcamera/camera_sensor_properties.cpp
> @@ -133,6 +133,20 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>  				{ controls::draft::TestPatternModePn9, 4 },
>  			},
>  		} },
> +		{ "ov2685", {
> +			.unitCellSize = { 1750, 1750 },
> +			.testPatternModes = {
> +				{ controls::draft::TestPatternModeOff, 0 },
> +				{ controls::draft::TestPatternModeColorBars, 1},
> +				{ controls::draft::TestPatternModeColorBarsFadeToGray, 2 },
> +				/*
> +				 * Also exposed by the driver:
> +				 *  - OV2685_TEST_PATTERN_RANDOM,
> +				 *  - OV2685_TEST_PATTERN_BW_SQUARE,
> +				 *  - OV2685_TEST_PATTERN_COLOR_SQUARE,
> +				 */
> +			},
> +		} },
>  		{ "ov2740", {
>  			.unitCellSize = { 1400, 1400 },
>  			.testPatternModes = {
> --
> 2.34.1
>
Kieran Bingham March 1, 2023, 3:01 p.m. UTC | #2 
Quoting Jacopo Mondi (2023-02-28 15:30:51)
> Hi Kieran
> 
> On Mon, Feb 27, 2023 at 08:42:53PM +0000, Kieran Bingham via libcamera-devel wrote:
> > Provide a CameraSensorHelper for the OV2685, 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 | 14 ++++++++++++++
> >  2 files changed, 25 insertions(+)
> >
> > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> > index 7977d7eb6c07..d1051cc25656 100644
> > --- a/src/ipa/libipa/camera_sensor_helper.cpp
> > +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> > @@ -450,6 +450,17 @@ public:
> >  };
> >  REGISTER_CAMERA_SENSOR_HELPER("imx477", CameraSensorHelperImx477)
> >
> > +class CameraSensorHelperOv2685 : public CameraSensorHelper
> > +{
> > +public:
> > +     CameraSensorHelperOv2685()
> > +     {
> > +             gainType_ = AnalogueGainLinear;
> > +             gainConstants_.linear = { 1, 0, 0, 128 };
> 
> The sensor manual I have doesn't specify a gain model at all.
> 
> I've seen this working and it's reasonable, I would add a coment to
> record that the gain model is not documented, but otherwise... ship
> it!

Is this ok for you ?

+               /*
+                * The Sensor Manual doesn't appear to document the gain model.
+                * This has been validated with some empirical testing only.
+                */

> 
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> > +     }
> > +};
> > +REGISTER_CAMERA_SENSOR_HELPER("ov2685", CameraSensorHelperOv2685)
> > +
> >  class CameraSensorHelperOv2740 : public CameraSensorHelper
> >  {
> >  public:
> > diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp
> > index b8c532699684..7652c5f3e24c 100644
> > --- a/src/libcamera/camera_sensor_properties.cpp
> > +++ b/src/libcamera/camera_sensor_properties.cpp
> > @@ -133,6 +133,20 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> >                               { controls::draft::TestPatternModePn9, 4 },
> >                       },
> >               } },
> > +             { "ov2685", {
> > +                     .unitCellSize = { 1750, 1750 },
> > +                     .testPatternModes = {
> > +                             { controls::draft::TestPatternModeOff, 0 },
> > +                             { controls::draft::TestPatternModeColorBars, 1},
> > +                             { controls::draft::TestPatternModeColorBarsFadeToGray, 2 },
> > +                             /*
> > +                              * Also exposed by the driver:
> > +                              *  - OV2685_TEST_PATTERN_RANDOM,
> > +                              *  - OV2685_TEST_PATTERN_BW_SQUARE,
> > +                              *  - OV2685_TEST_PATTERN_COLOR_SQUARE,
> > +                              */
> > +                     },
> > +             } },
> >               { "ov2740", {
> >                       .unitCellSize = { 1400, 1400 },
> >                       .testPatternModes = {
> > --
> > 2.34.1
> >
Laurent Pinchart March 9, 2023, 10:21 a.m. UTC | #3 
Hi Kieran,

Thank you for the patch.

On Mon, Feb 27, 2023 at 08:42:53PM +0000, Kieran Bingham via libcamera-devel wrote:
> Provide a CameraSensorHelper for the OV2685, along with the
> corresponding camera sensor properties.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

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

> ---
>  src/ipa/libipa/camera_sensor_helper.cpp    | 11 +++++++++++
>  src/libcamera/camera_sensor_properties.cpp | 14 ++++++++++++++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> index 7977d7eb6c07..d1051cc25656 100644
> --- a/src/ipa/libipa/camera_sensor_helper.cpp
> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> @@ -450,6 +450,17 @@ public:
>  };
>  REGISTER_CAMERA_SENSOR_HELPER("imx477", CameraSensorHelperImx477)
>  
> +class CameraSensorHelperOv2685 : public CameraSensorHelper
> +{
> +public:
> +	CameraSensorHelperOv2685()
> +	{
> +		gainType_ = AnalogueGainLinear;
> +		gainConstants_.linear = { 1, 0, 0, 128 };
> +	}
> +};
> +REGISTER_CAMERA_SENSOR_HELPER("ov2685", CameraSensorHelperOv2685)
> +
>  class CameraSensorHelperOv2740 : public CameraSensorHelper
>  {
>  public:
> diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp
> index b8c532699684..7652c5f3e24c 100644
> --- a/src/libcamera/camera_sensor_properties.cpp
> +++ b/src/libcamera/camera_sensor_properties.cpp
> @@ -133,6 +133,20 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>  				{ controls::draft::TestPatternModePn9, 4 },
>  			},
>  		} },
> +		{ "ov2685", {
> +			.unitCellSize = { 1750, 1750 },
> +			.testPatternModes = {
> +				{ controls::draft::TestPatternModeOff, 0 },
> +				{ controls::draft::TestPatternModeColorBars, 1},
> +				{ controls::draft::TestPatternModeColorBarsFadeToGray, 2 },
> +				/*
> +				 * Also exposed by the driver:
> +				 *  - OV2685_TEST_PATTERN_RANDOM,
> +				 *  - OV2685_TEST_PATTERN_BW_SQUARE,
> +				 *  - OV2685_TEST_PATTERN_COLOR_SQUARE,
> +				 */
> +			},
> +		} },
>  		{ "ov2740", {
>  			.unitCellSize = { 1400, 1400 },
>  			.testPatternModes = {
Umang Jain March 9, 2023, 10:22 a.m. UTC | #4 
Hi Kieran

On 3/1/23 8:31 PM, Kieran Bingham via libcamera-devel wrote:
> Quoting Jacopo Mondi (2023-02-28 15:30:51)
>> Hi Kieran
>>
>> On Mon, Feb 27, 2023 at 08:42:53PM +0000, Kieran Bingham via libcamera-devel wrote:
>>> Provide a CameraSensorHelper for the OV2685, 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 | 14 ++++++++++++++
>>>   2 files changed, 25 insertions(+)
>>>
>>> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
>>> index 7977d7eb6c07..d1051cc25656 100644
>>> --- a/src/ipa/libipa/camera_sensor_helper.cpp
>>> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
>>> @@ -450,6 +450,17 @@ public:
>>>   };
>>>   REGISTER_CAMERA_SENSOR_HELPER("imx477", CameraSensorHelperImx477)
>>>
>>> +class CameraSensorHelperOv2685 : public CameraSensorHelper
>>> +{
>>> +public:
>>> +     CameraSensorHelperOv2685()
>>> +     {
>>> +             gainType_ = AnalogueGainLinear;
>>> +             gainConstants_.linear = { 1, 0, 0, 128 };
>> The sensor manual I have doesn't specify a gain model at all.
>>
>> I've seen this working and it's reasonable, I would add a coment to
>> record that the gain model is not documented, but otherwise... ship
>> it!
> Is this ok for you ?
>
> +               /*
> +                * The Sensor Manual doesn't appear to document the gain model.
> +                * This has been validated with some empirical testing only.
> +                */
>
>> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>
>>> +     }
>>> +};
>>> +REGISTER_CAMERA_SENSOR_HELPER("ov2685", CameraSensorHelperOv2685)
>>> +
>>>   class CameraSensorHelperOv2740 : public CameraSensorHelper
>>>   {
>>>   public:
>>> diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp
>>> index b8c532699684..7652c5f3e24c 100644
>>> --- a/src/libcamera/camera_sensor_properties.cpp
>>> +++ b/src/libcamera/camera_sensor_properties.cpp
>>> @@ -133,6 +133,20 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>>>                                { controls::draft::TestPatternModePn9, 4 },
>>>                        },
>>>                } },
>>> +             { "ov2685", {
>>> +                     .unitCellSize = { 1750, 1750 },
>>> +                     .testPatternModes = {
>>> +                             { controls::draft::TestPatternModeOff, 0 },
>>> +                             { controls::draft::TestPatternModeColorBars, 1},
>>> +                             { controls::draft::TestPatternModeColorBarsFadeToGray, 2 },
>>> +                             /*
>>> +                              * Also exposed by the driver:

This doesn't tell convey the what's the issue here?  Is it a case of 
exposed by driver but not found while testing?

In that case, I would align with one of the pre-existing comment(s) from 
other entries:

     /*
      * No corresponding test pattern mode for:
     ....

or similar.

In any case, minor nitpick so,

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>



>>> +                              *  - OV2685_TEST_PATTERN_RANDOM,
>>> +                              *  - OV2685_TEST_PATTERN_BW_SQUARE,
>>> +                              *  - OV2685_TEST_PATTERN_COLOR_SQUARE,
>>> +                              */
>>> +                     },
>>> +             } },
>>>                { "ov2740", {
>>>                        .unitCellSize = { 1400, 1400 },
>>>                        .testPatternModes = {
>>> --
>>> 2.34.1
>>>

Patch
diff mbox series 

diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
index 7977d7eb6c07..d1051cc25656 100644
--- a/src/ipa/libipa/camera_sensor_helper.cpp
+++ b/src/ipa/libipa/camera_sensor_helper.cpp
@@ -450,6 +450,17 @@  public:
 };
 REGISTER_CAMERA_SENSOR_HELPER("imx477", CameraSensorHelperImx477)
 
+class CameraSensorHelperOv2685 : public CameraSensorHelper
+{
+public:
+	CameraSensorHelperOv2685()
+	{
+		gainType_ = AnalogueGainLinear;
+		gainConstants_.linear = { 1, 0, 0, 128 };
+	}
+};
+REGISTER_CAMERA_SENSOR_HELPER("ov2685", CameraSensorHelperOv2685)
+
 class CameraSensorHelperOv2740 : public CameraSensorHelper
 {
 public:
diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp
index b8c532699684..7652c5f3e24c 100644
--- a/src/libcamera/camera_sensor_properties.cpp
+++ b/src/libcamera/camera_sensor_properties.cpp
@@ -133,6 +133,20 @@  const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
 				{ controls::draft::TestPatternModePn9, 4 },
 			},
 		} },
+		{ "ov2685", {
+			.unitCellSize = { 1750, 1750 },
+			.testPatternModes = {
+				{ controls::draft::TestPatternModeOff, 0 },
+				{ controls::draft::TestPatternModeColorBars, 1},
+				{ controls::draft::TestPatternModeColorBarsFadeToGray, 2 },
+				/*
+				 * Also exposed by the driver:
+				 *  - OV2685_TEST_PATTERN_RANDOM,
+				 *  - OV2685_TEST_PATTERN_BW_SQUARE,
+				 *  - OV2685_TEST_PATTERN_COLOR_SQUARE,
+				 */
+			},
+		} },
 		{ "ov2740", {
 			.unitCellSize = { 1400, 1400 },
 			.testPatternModes = {