[v3,1/2] libcamera: libipa: camera_sensor: Add MediaTek gc05a2 sensor properties
diff mbox series

Message ID 20241119073913.500732-2-chenghaoyang@chromium.org
State Superseded
Headers show
Series
  • Add camera sensor properties for ciri
Related show

Commit Message

Harvey Yang Nov. 19, 2024, 7:37 a.m. UTC 
From: Harvey Yang <chenghaoyang@google.com>

Provide the MediaTek gc05a2 camera sensor properties and registration
with libipa for the gain code helpers.

Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
Co-developed-by: Xing Gu <xinggu@chromium.org>
Co-developed-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
---
 src/ipa/libipa/camera_sensor_helper.cpp           | 11 +++++++++++
 src/libcamera/sensor/camera_sensor_properties.cpp |  7 +++++++
 2 files changed, 18 insertions(+)

Comments

Laurent Pinchart Nov. 19, 2024, 7:56 a.m. UTC | #1 
Hi Harvey,

Thank you for the patch.

On Tue, Nov 19, 2024 at 07:37:35AM +0000, Harvey Yang wrote:
> From: Harvey Yang <chenghaoyang@google.com>
> 
> Provide the MediaTek gc05a2 camera sensor properties and registration

I think you meant GalaxyCore, not MediaTek, both in the subject line and
here.

> with libipa for the gain code helpers.
> 
> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> Co-developed-by: Xing Gu <xinggu@chromium.org>
> Co-developed-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
> Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> ---
>  src/ipa/libipa/camera_sensor_helper.cpp           | 11 +++++++++++
>  src/libcamera/sensor/camera_sensor_properties.cpp |  7 +++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> index c6169bdc7..36555680b 100644
> --- a/src/ipa/libipa/camera_sensor_helper.cpp
> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> @@ -519,6 +519,17 @@ private:
>  };
>  REGISTER_CAMERA_SENSOR_HELPER("ar0521", CameraSensorHelperAr0521)
>  
> +class CameraSensorHelperGc05a2 : public CameraSensorHelper
> +{
> +public:
> +	CameraSensorHelperGc05a2()
> +	{
> +		gainType_ = AnalogueGainLinear;
> +		gainConstants_.linear = { 100, 0, 0, 1024 };

We also need to know the black level (a.k.a. data pedestal).

The rest of the patch looks good to me. With the black level value
added, I expect this can be merged.

Same comment for 2/2.

> +	}
> +};
> +REGISTER_CAMERA_SENSOR_HELPER("gc05a2", CameraSensorHelperGc05a2)
> +
>  class CameraSensorHelperImx214 : public CameraSensorHelper
>  {
>  public:
> diff --git a/src/libcamera/sensor/camera_sensor_properties.cpp b/src/libcamera/sensor/camera_sensor_properties.cpp
> index 6d4136d03..2f048d4a3 100644
> --- a/src/libcamera/sensor/camera_sensor_properties.cpp
> +++ b/src/libcamera/sensor/camera_sensor_properties.cpp
> @@ -70,6 +70,13 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>  				{ controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
>  			},
>  		} },
> +		{ "gc05a2", {
> +			.unitCellSize = { 1120, 1120 },
> +			.testPatternModes = {
> +				{ controls::draft::TestPatternModeOff, 0 },
> +				{ controls::draft::TestPatternModeColorBars, 1 },
> +			},
> +		} },
>  		{ "hi846", {
>  			.unitCellSize = { 1120, 1120 },
>  			.testPatternModes = {
Harvey Yang Nov. 20, 2024, 7:39 a.m. UTC | #2 
Hi Laurent,

On Tue, Nov 19, 2024 at 3:56 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Harvey,
>
> Thank you for the patch.
>
> On Tue, Nov 19, 2024 at 07:37:35AM +0000, Harvey Yang wrote:
> > From: Harvey Yang <chenghaoyang@google.com>
> >
> > Provide the MediaTek gc05a2 camera sensor properties and registration
>
> I think you meant GalaxyCore, not MediaTek, both in the subject line and
> here.

Yes, thanks for the catch.

>
> > with libipa for the gain code helpers.
> >
> > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> > Co-developed-by: Xing Gu <xinggu@chromium.org>
> > Co-developed-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
> > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > ---
> >  src/ipa/libipa/camera_sensor_helper.cpp           | 11 +++++++++++
> >  src/libcamera/sensor/camera_sensor_properties.cpp |  7 +++++++
> >  2 files changed, 18 insertions(+)
> >
> > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> > index c6169bdc7..36555680b 100644
> > --- a/src/ipa/libipa/camera_sensor_helper.cpp
> > +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> > @@ -519,6 +519,17 @@ private:
> >  };
> >  REGISTER_CAMERA_SENSOR_HELPER("ar0521", CameraSensorHelperAr0521)
> >
> > +class CameraSensorHelperGc05a2 : public CameraSensorHelper
> > +{
> > +public:
> > +     CameraSensorHelperGc05a2()
> > +     {
> > +             gainType_ = AnalogueGainLinear;
> > +             gainConstants_.linear = { 100, 0, 0, 1024 };
>
> We also need to know the black level (a.k.a. data pedestal).
>
> The rest of the patch looks good to me. With the black level value
> added, I expect this can be merged.
>
> Same comment for 2/2.

Confirmed with MediaTek that both sensors have a black level 0x40.
Updated in the next version.

BR,
Harvey

>
> > +     }
> > +};
> > +REGISTER_CAMERA_SENSOR_HELPER("gc05a2", CameraSensorHelperGc05a2)
> > +
> >  class CameraSensorHelperImx214 : public CameraSensorHelper
> >  {
> >  public:
> > diff --git a/src/libcamera/sensor/camera_sensor_properties.cpp b/src/libcamera/sensor/camera_sensor_properties.cpp
> > index 6d4136d03..2f048d4a3 100644
> > --- a/src/libcamera/sensor/camera_sensor_properties.cpp
> > +++ b/src/libcamera/sensor/camera_sensor_properties.cpp
> > @@ -70,6 +70,13 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> >                               { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
> >                       },
> >               } },
> > +             { "gc05a2", {
> > +                     .unitCellSize = { 1120, 1120 },
> > +                     .testPatternModes = {
> > +                             { controls::draft::TestPatternModeOff, 0 },
> > +                             { controls::draft::TestPatternModeColorBars, 1 },
> > +                     },
> > +             } },
> >               { "hi846", {
> >                       .unitCellSize = { 1120, 1120 },
> >                       .testPatternModes = {
>
> --
> Regards,
>
> Laurent Pinchart
Harvey Yang Nov. 20, 2024, 3:56 p.m. UTC | #3 
Hi Kieran,

On Wed, Nov 20, 2024 at 6:18 PM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting Cheng-Hao Yang (2024-11-20 07:39:16)
> > Hi Laurent,
> >
> > On Tue, Nov 19, 2024 at 3:56 PM Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> > >
> > > Hi Harvey,
> > >
> > > Thank you for the patch.
> > >
> > > On Tue, Nov 19, 2024 at 07:37:35AM +0000, Harvey Yang wrote:
> > > > From: Harvey Yang <chenghaoyang@google.com>
> > > >
> > > > Provide the MediaTek gc05a2 camera sensor properties and registration
> > >
> > > I think you meant GalaxyCore, not MediaTek, both in the subject line and
> > > here.
> >
> > Yes, thanks for the catch.
> >
> > >
> > > > with libipa for the gain code helpers.
> > > >
> > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> > > > Co-developed-by: Xing Gu <xinggu@chromium.org>
> > > > Co-developed-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
> > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > > > ---
> > > >  src/ipa/libipa/camera_sensor_helper.cpp           | 11 +++++++++++
> > > >  src/libcamera/sensor/camera_sensor_properties.cpp |  7 +++++++
> > > >  2 files changed, 18 insertions(+)
> > > >
> > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> > > > index c6169bdc7..36555680b 100644
> > > > --- a/src/ipa/libipa/camera_sensor_helper.cpp
> > > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> > > > @@ -519,6 +519,17 @@ private:
> > > >  };
> > > >  REGISTER_CAMERA_SENSOR_HELPER("ar0521", CameraSensorHelperAr0521)
> > > >
> > > > +class CameraSensorHelperGc05a2 : public CameraSensorHelper
> > > > +{
> > > > +public:
> > > > +     CameraSensorHelperGc05a2()
> > > > +     {
> > > > +             gainType_ = AnalogueGainLinear;
> > > > +             gainConstants_.linear = { 100, 0, 0, 1024 };
> > >
> > > We also need to know the black level (a.k.a. data pedestal).
> > >
> > > The rest of the patch looks good to me. With the black level value
> > > added, I expect this can be merged.
> > >
> > > Same comment for 2/2.
> >
> > Confirmed with MediaTek that both sensors have a black level 0x40.
> > Updated in the next version.
>
> At what bit depth? We store the black level as a 16 bit value in
> libcamera so it needs to be bit-shifted accordingly.

According to the docs that they provided to me, the field only takes
8 bits. 0x40 must mean 64 in decimal, IIUC.

BR,
Harvey

>
> --
> Kieran
>
> >
> > BR,
> > Harvey
> >
> > >
> > > > +     }
> > > > +};
> > > > +REGISTER_CAMERA_SENSOR_HELPER("gc05a2", CameraSensorHelperGc05a2)
> > > > +
> > > >  class CameraSensorHelperImx214 : public CameraSensorHelper
> > > >  {
> > > >  public:
> > > > diff --git a/src/libcamera/sensor/camera_sensor_properties.cpp b/src/libcamera/sensor/camera_sensor_properties.cpp
> > > > index 6d4136d03..2f048d4a3 100644
> > > > --- a/src/libcamera/sensor/camera_sensor_properties.cpp
> > > > +++ b/src/libcamera/sensor/camera_sensor_properties.cpp
> > > > @@ -70,6 +70,13 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > > >                               { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
> > > >                       },
> > > >               } },
> > > > +             { "gc05a2", {
> > > > +                     .unitCellSize = { 1120, 1120 },
> > > > +                     .testPatternModes = {
> > > > +                             { controls::draft::TestPatternModeOff, 0 },
> > > > +                             { controls::draft::TestPatternModeColorBars, 1 },
> > > > +                     },
> > > > +             } },
> > > >               { "hi846", {
> > > >                       .unitCellSize = { 1120, 1120 },
> > > >                       .testPatternModes = {
> > >
> > > --
> > > Regards,
> > >
> > > Laurent Pinchart
Kieran Bingham Nov. 20, 2024, 4:15 p.m. UTC | #4 
Quoting Cheng-Hao Yang (2024-11-20 15:56:31)
> Hi Kieran,
> 
> On Wed, Nov 20, 2024 at 6:18 PM Kieran Bingham
> <kieran.bingham@ideasonboard.com> wrote:
> >
> > Quoting Cheng-Hao Yang (2024-11-20 07:39:16)
> > > Hi Laurent,
> > >
> > > On Tue, Nov 19, 2024 at 3:56 PM Laurent Pinchart
> > > <laurent.pinchart@ideasonboard.com> wrote:
> > > >
> > > > Hi Harvey,
> > > >
> > > > Thank you for the patch.
> > > >
> > > > On Tue, Nov 19, 2024 at 07:37:35AM +0000, Harvey Yang wrote:
> > > > > From: Harvey Yang <chenghaoyang@google.com>
> > > > >
> > > > > Provide the MediaTek gc05a2 camera sensor properties and registration
> > > >
> > > > I think you meant GalaxyCore, not MediaTek, both in the subject line and
> > > > here.
> > >
> > > Yes, thanks for the catch.
> > >
> > > >
> > > > > with libipa for the gain code helpers.
> > > > >
> > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> > > > > Co-developed-by: Xing Gu <xinggu@chromium.org>
> > > > > Co-developed-by: Yudhistira Erlandinata <yerlandinata@chromium.org>
> > > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > > > > ---
> > > > >  src/ipa/libipa/camera_sensor_helper.cpp           | 11 +++++++++++
> > > > >  src/libcamera/sensor/camera_sensor_properties.cpp |  7 +++++++
> > > > >  2 files changed, 18 insertions(+)
> > > > >
> > > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> > > > > index c6169bdc7..36555680b 100644
> > > > > --- a/src/ipa/libipa/camera_sensor_helper.cpp
> > > > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> > > > > @@ -519,6 +519,17 @@ private:
> > > > >  };
> > > > >  REGISTER_CAMERA_SENSOR_HELPER("ar0521", CameraSensorHelperAr0521)
> > > > >
> > > > > +class CameraSensorHelperGc05a2 : public CameraSensorHelper
> > > > > +{
> > > > > +public:
> > > > > +     CameraSensorHelperGc05a2()
> > > > > +     {
> > > > > +             gainType_ = AnalogueGainLinear;
> > > > > +             gainConstants_.linear = { 100, 0, 0, 1024 };
> > > >
> > > > We also need to know the black level (a.k.a. data pedestal).
> > > >
> > > > The rest of the patch looks good to me. With the black level value
> > > > added, I expect this can be merged.
> > > >
> > > > Same comment for 2/2.
> > >
> > > Confirmed with MediaTek that both sensors have a black level 0x40.
> > > Updated in the next version.
> >
> > At what bit depth? We store the black level as a 16 bit value in
> > libcamera so it needs to be bit-shifted accordingly.
> 
> According to the docs that they provided to me, the field only takes
> 8 bits. 0x40 must mean 64 in decimal, IIUC.

Taking a guess from
https://en.gcoreinc.com/products/index?cid=2&subcid=5 Does the sensor
output RAW10 ?

If so - then this is 0x40 at 10 bits. And therefore storing it in
libcamera we would represent this as a 16 bit value and shift
accordingly.

Therefore it would be:

                /* From datasheet: 64 at 10bits. */
                blackLevel_ = 4096;

or

                /* From datasheet: 64 at 10bits. */
		blackLevel_ = 64 << 6;

--
Kieran




> 
> BR,
> Harvey
> 
> >
> > --
> > Kieran
> >
> > >
> > > BR,
> > > Harvey
> > >
> > > >
> > > > > +     }
> > > > > +};
> > > > > +REGISTER_CAMERA_SENSOR_HELPER("gc05a2", CameraSensorHelperGc05a2)
> > > > > +
> > > > >  class CameraSensorHelperImx214 : public CameraSensorHelper
> > > > >  {
> > > > >  public:
> > > > > diff --git a/src/libcamera/sensor/camera_sensor_properties.cpp b/src/libcamera/sensor/camera_sensor_properties.cpp
> > > > > index 6d4136d03..2f048d4a3 100644
> > > > > --- a/src/libcamera/sensor/camera_sensor_properties.cpp
> > > > > +++ b/src/libcamera/sensor/camera_sensor_properties.cpp
> > > > > @@ -70,6 +70,13 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > > > >                               { controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
> > > > >                       },
> > > > >               } },
> > > > > +             { "gc05a2", {
> > > > > +                     .unitCellSize = { 1120, 1120 },
> > > > > +                     .testPatternModes = {
> > > > > +                             { controls::draft::TestPatternModeOff, 0 },
> > > > > +                             { controls::draft::TestPatternModeColorBars, 1 },
> > > > > +                     },
> > > > > +             } },
> > > > >               { "hi846", {
> > > > >                       .unitCellSize = { 1120, 1120 },
> > > > >                       .testPatternModes = {
> > > >
> > > > --
> > > > Regards,
> > > >
> > > > Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
index c6169bdc7..36555680b 100644
--- a/src/ipa/libipa/camera_sensor_helper.cpp
+++ b/src/ipa/libipa/camera_sensor_helper.cpp
@@ -519,6 +519,17 @@  private:
 };
 REGISTER_CAMERA_SENSOR_HELPER("ar0521", CameraSensorHelperAr0521)
 
+class CameraSensorHelperGc05a2 : public CameraSensorHelper
+{
+public:
+	CameraSensorHelperGc05a2()
+	{
+		gainType_ = AnalogueGainLinear;
+		gainConstants_.linear = { 100, 0, 0, 1024 };
+	}
+};
+REGISTER_CAMERA_SENSOR_HELPER("gc05a2", CameraSensorHelperGc05a2)
+
 class CameraSensorHelperImx214 : public CameraSensorHelper
 {
 public:
diff --git a/src/libcamera/sensor/camera_sensor_properties.cpp b/src/libcamera/sensor/camera_sensor_properties.cpp
index 6d4136d03..2f048d4a3 100644
--- a/src/libcamera/sensor/camera_sensor_properties.cpp
+++ b/src/libcamera/sensor/camera_sensor_properties.cpp
@@ -70,6 +70,13 @@  const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
 				{ controls::draft::TestPatternModeColorBarsFadeToGray, 3 },
 			},
 		} },
+		{ "gc05a2", {
+			.unitCellSize = { 1120, 1120 },
+			.testPatternModes = {
+				{ controls::draft::TestPatternModeOff, 0 },
+				{ controls::draft::TestPatternModeColorBars, 1 },
+			},
+		} },
 		{ "hi846", {
 			.unitCellSize = { 1120, 1120 },
 			.testPatternModes = {