[1/1] libcamera: Add camera sensor properties for ciri
diff mbox series

Message ID 20240923071618.2395064-2-chenghaoyang@google.com
State Superseded
Headers show
Series
  • Add camera sensor properties for ciri
Related show

Commit Message

Harvey Yang Sept. 23, 2024, 7:09 a.m. UTC 
From: Han-Lin Chen <hanlinchen@chromium.org>

ciri has sensors: hi1339, gc08a3, and gc05a2.

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>
---
 .../sensor/camera_sensor_properties.cpp       | 21 +++++++++++++++++++
 1 file changed, 21 insertions(+)

Comments

Laurent Pinchart Sept. 23, 2024, 8:47 a.m. UTC | #1 
Hi Harvey and Han-Lin,

Thank you for the patch.

On Mon, Sep 23, 2024 at 07:09:56AM +0000, Harvey Yang wrote:
> From: Han-Lin Chen <hanlinchen@chromium.org>
> 
> ciri has sensors: hi1339, gc08a3, and gc05a2.
> 
> 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>
> ---
>  .../sensor/camera_sensor_properties.cpp       | 21 +++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/src/libcamera/sensor/camera_sensor_properties.cpp b/src/libcamera/sensor/camera_sensor_properties.cpp
> index 4e5217ab..a224f8d2 100644
> --- a/src/libcamera/sensor/camera_sensor_properties.cpp
> +++ b/src/libcamera/sensor/camera_sensor_properties.cpp
> @@ -276,6 +276,27 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>  				{ controls::draft::TestPatternModeColorBars, 1 },
>  			},
>  		} },
> +		{ "hi1339", {
> +			.unitCellSize = { 1120, 1120 },
> +			.testPatternModes = {
> +				{ controls::draft::TestPatternModeOff, 0 },
> +				{ controls::draft::TestPatternModeColorBars, 2 },
> +			},
> +		} },

Unless I'm mistaken, the driver for this sensor hasn't been posted to
the linux-media mailing list. The policy in libcamera is that drivers
need to be on their way to upstream. You could split this patch in two
to merge support for the gc08a3 and gc05a2 already, and address the
hi1339 when the driver gets posted.

> +		{ "gc08a3", {
> +			.unitCellSize = { 1120, 1120 },
> +			.testPatternModes = {
> +				{ controls::draft::TestPatternModeOff, 0 },
> +				{ controls::draft::TestPatternModeColorBars, 2 },
> +			},
> +		} },

Sensor support also requires adding a sensor helper in
src/ipa/libipa/camera_sensor_helper.cpp.

> +		{ "gc05a2", {
> +			.unitCellSize = { 1120, 1120 },

Do those three sensors really have a pixel size of 1.12µm, or was that
by any chance copied from the previous entry in the table ?

> +			.testPatternModes = {
> +				{ controls::draft::TestPatternModeOff, 0 },
> +				{ controls::draft::TestPatternModeColorBars, 1 },
> +			},
> +		} },

Please sort entries alphabetically.

>  	};
>  
>  	const auto it = sensorProps.find(sensor);
Harvey Yang Sept. 24, 2024, 9:01 a.m. UTC | #2 
Thanks Laurent for the quick review,

On Mon, Sep 23, 2024 at 4:48 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Harvey and Han-Lin,
>
> Thank you for the patch.
>
> On Mon, Sep 23, 2024 at 07:09:56AM +0000, Harvey Yang wrote:
> > From: Han-Lin Chen <hanlinchen@chromium.org>
> >
> > ciri has sensors: hi1339, gc08a3, and gc05a2.
> >
> > 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>
> > ---
> >  .../sensor/camera_sensor_properties.cpp       | 21 +++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> >
> > diff --git a/src/libcamera/sensor/camera_sensor_properties.cpp b/src/libcamera/sensor/camera_sensor_properties.cpp
> > index 4e5217ab..a224f8d2 100644
> > --- a/src/libcamera/sensor/camera_sensor_properties.cpp
> > +++ b/src/libcamera/sensor/camera_sensor_properties.cpp
> > @@ -276,6 +276,27 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> >                               { controls::draft::TestPatternModeColorBars, 1 },
> >                       },
> >               } },
> > +             { "hi1339", {
> > +                     .unitCellSize = { 1120, 1120 },
> > +                     .testPatternModes = {
> > +                             { controls::draft::TestPatternModeOff, 0 },
> > +                             { controls::draft::TestPatternModeColorBars, 2 },
> > +                     },
> > +             } },
>
> Unless I'm mistaken, the driver for this sensor hasn't been posted to
> the linux-media mailing list. The policy in libcamera is that drivers
> need to be on their way to upstream. You could split this patch in two
> to merge support for the gc08a3 and gc05a2 already, and address the
> hi1339 when the driver gets posted.

Yeah right. hi1339 is the old sensor used on an old ciri model in the
development process. Therefore, I don't think we're using it in
production and MTK may just skip upstreaming it.

I'll drop this in the next version.


>
> > +             { "gc08a3", {
> > +                     .unitCellSize = { 1120, 1120 },
> > +                     .testPatternModes = {
> > +                             { controls::draft::TestPatternModeOff, 0 },
> > +                             { controls::draft::TestPatternModeColorBars, 2 },
> > +                     },
> > +             } },
>
> Sensor support also requires adding a sensor helper in
> src/ipa/libipa/camera_sensor_helper.cpp.

Ah got it, while I'm not sure if we want to use it in
mtkisp7's ipa, as the algorithm returns an integer
as the analogue gain [1], instead of double.
The algo also doesn't need the gain fed back. It
keeps the recent values searchable by request
ids.

[1]: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/ipa/mtkisp7/hal3a/hal_3a.cpp;l=946

>
> > +             { "gc05a2", {
> > +                     .unitCellSize = { 1120, 1120 },
>
> Do those three sensors really have a pixel size of 1.12µm, or was that
> by any chance copied from the previous entry in the table ?

It's 1.12um for "gc05a2" [1] and "gc08a3" [2] at least.

[1]: https://en.gcoreinc.com/products/index?cid=2&subcid=5
[2]: https://en.gcoreinc.com/products/index?cid=2&subcid=4

>
> > +                     .testPatternModes = {
> > +                             { controls::draft::TestPatternModeOff, 0 },
> > +                             { controls::draft::TestPatternModeColorBars, 1 },
> > +                     },
> > +             } },
>
> Please sort entries alphabetically.

Will do.

BR,
Harvey


>
> >       };
> >
> >       const auto it = sensorProps.find(sensor);
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart Sept. 24, 2024, 3:30 p.m. UTC | #3 
On Tue, Sep 24, 2024 at 05:01:25PM +0800, Cheng-Hao Yang wrote:
> On Mon, Sep 23, 2024 at 4:48 PM Laurent Pinchart wrote:
> > On Mon, Sep 23, 2024 at 07:09:56AM +0000, Harvey Yang wrote:
> > > From: Han-Lin Chen <hanlinchen@chromium.org>
> > >
> > > ciri has sensors: hi1339, gc08a3, and gc05a2.
> > >
> > > 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>
> > > ---
> > >  .../sensor/camera_sensor_properties.cpp       | 21 +++++++++++++++++++
> > >  1 file changed, 21 insertions(+)
> > >
> > > diff --git a/src/libcamera/sensor/camera_sensor_properties.cpp b/src/libcamera/sensor/camera_sensor_properties.cpp
> > > index 4e5217ab..a224f8d2 100644
> > > --- a/src/libcamera/sensor/camera_sensor_properties.cpp
> > > +++ b/src/libcamera/sensor/camera_sensor_properties.cpp
> > > @@ -276,6 +276,27 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
> > >                               { controls::draft::TestPatternModeColorBars, 1 },
> > >                       },
> > >               } },
> > > +             { "hi1339", {
> > > +                     .unitCellSize = { 1120, 1120 },
> > > +                     .testPatternModes = {
> > > +                             { controls::draft::TestPatternModeOff, 0 },
> > > +                             { controls::draft::TestPatternModeColorBars, 2 },
> > > +                     },
> > > +             } },
> >
> > Unless I'm mistaken, the driver for this sensor hasn't been posted to
> > the linux-media mailing list. The policy in libcamera is that drivers
> > need to be on their way to upstream. You could split this patch in two
> > to merge support for the gc08a3 and gc05a2 already, and address the
> > hi1339 when the driver gets posted.
> 
> Yeah right. hi1339 is the old sensor used on an old ciri model in the
> development process. Therefore, I don't think we're using it in
> production and MTK may just skip upstreaming it.
> 
> I'll drop this in the next version.
> 
> > > +             { "gc08a3", {
> > > +                     .unitCellSize = { 1120, 1120 },
> > > +                     .testPatternModes = {
> > > +                             { controls::draft::TestPatternModeOff, 0 },
> > > +                             { controls::draft::TestPatternModeColorBars, 2 },
> > > +                     },
> > > +             } },
> >
> > Sensor support also requires adding a sensor helper in
> > src/ipa/libipa/camera_sensor_helper.cpp.
> 
> Ah got it, while I'm not sure if we want to use it in
> mtkisp7's ipa, as the algorithm returns an integer
> as the analogue gain [1], instead of double.

libipa models the analog gain as a real value, and the sensor helpers
are tasked with converting that value to the analog gain value that the
sensor expects. This assumes that the kernel driver passes the value to
the sensor without performing any conversion, which is what we require
from drivers (the driver can still clamp the value to the supported
range, and should expose the range through the min/max values for the
control). Your IPA module doesn't have to use libipa, but new sensors
need to be added to libipa so that they will work with all the platforms
we support.

> The algo also doesn't need the gain fed back. It
> keeps the recent values searchable by request
> ids.

We still need the back conversion to be implemented in libipa, for
platform that need it.

> [1]: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/ipa/mtkisp7/hal3a/hal_3a.cpp;l=946
> 
> > > +             { "gc05a2", {
> > > +                     .unitCellSize = { 1120, 1120 },
> >
> > Do those three sensors really have a pixel size of 1.12µm, or was that
> > by any chance copied from the previous entry in the table ?
> 
> It's 1.12um for "gc05a2" [1] and "gc08a3" [2] at least.
> 
> [1]: https://en.gcoreinc.com/products/index?cid=2&subcid=5
> [2]: https://en.gcoreinc.com/products/index?cid=2&subcid=4

Thanks.

> > > +                     .testPatternModes = {
> > > +                             { controls::draft::TestPatternModeOff, 0 },
> > > +                             { controls::draft::TestPatternModeColorBars, 1 },
> > > +                     },
> > > +             } },
> >
> > Please sort entries alphabetically.
> 
> Will do.
> 
> > >       };
> > >
> > >       const auto it = sensorProps.find(sensor);

Patch
diff mbox series 

diff --git a/src/libcamera/sensor/camera_sensor_properties.cpp b/src/libcamera/sensor/camera_sensor_properties.cpp
index 4e5217ab..a224f8d2 100644
--- a/src/libcamera/sensor/camera_sensor_properties.cpp
+++ b/src/libcamera/sensor/camera_sensor_properties.cpp
@@ -276,6 +276,27 @@  const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
 				{ controls::draft::TestPatternModeColorBars, 1 },
 			},
 		} },
+		{ "hi1339", {
+			.unitCellSize = { 1120, 1120 },
+			.testPatternModes = {
+				{ controls::draft::TestPatternModeOff, 0 },
+				{ controls::draft::TestPatternModeColorBars, 2 },
+			},
+		} },
+		{ "gc08a3", {
+			.unitCellSize = { 1120, 1120 },
+			.testPatternModes = {
+				{ controls::draft::TestPatternModeOff, 0 },
+				{ controls::draft::TestPatternModeColorBars, 2 },
+			},
+		} },
+		{ "gc05a2", {
+			.unitCellSize = { 1120, 1120 },
+			.testPatternModes = {
+				{ controls::draft::TestPatternModeOff, 0 },
+				{ controls::draft::TestPatternModeColorBars, 1 },
+			},
+		} },
 	};
 
 	const auto it = sensorProps.find(sensor);