[PATCH 1/1] libcamera: Add camera sensor properties for ciri

[Laurent Pinchart]

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 at chromium.org>
> > >
> > > ciri has sensors: hi1339, gc08a3, and gc05a2.
> > >
> > > Signed-off-by: Han-Lin Chen <hanlinchen at chromium.org>
> > > Co-developed-by: Xing Gu <xinggu at chromium.org>
> > > Co-developed-by: Yudhistira Erlandinata <yerlandinata at chromium.org>
> > > Co-developed-by: Harvey Yang <chenghaoyang at 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);

-- 
Regards,

Laurent Pinchart