[PATCH v2 1/2] libcamera: Add gc05a2 camera sensor proprietary for ciri

[Laurent Pinchart]

Hi Harbey,

On Fri, Nov 08, 2024 at 02:50:33PM +0800, Cheng-Hao Yang wrote:
> On Tue, Sep 24, 2024 at 9:19 PM Laurent Pinchart wrote:
> > On Tue, Sep 24, 2024 at 01:48:09PM +0100, Kieran Bingham wrote:
> > > In $SUBJECT - I think you mean 'properties' not 'proprietary'
> > >
> > > But as for the commit message, the subject should say what this patch is
> > > for. libcamera has no knowledge of what a 'ciri' is.
> > >
> > > Lets keep it to the details of the image sensor.
> > >
> > > Quoting Harvey Yang (2024-09-24 09:26:10)
> > > > gc05a2 is the first sensor used by ciri.
> > >
> > > I don't think that's relevant to libcamera.
> > >
> > > Please describe what the gc05a2 is instead.
> > >
> > > > 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>
> > > > ---
> > > >  src/ipa/libipa/camera_sensor_helper.cpp         | 17 +++++++++++++++++
> > > >  .../sensor/camera_sensor_properties.cpp         |  7 +++++++
> > > >  2 files changed, 24 insertions(+)
> > > >
> > > > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> > > > index ffc7c1d7..cd7d12d6 100644
> > > > --- a/src/ipa/libipa/camera_sensor_helper.cpp
> > > > +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> > > > @@ -519,6 +519,23 @@ private:
> > > >  };
> > > >  REGISTER_CAMERA_SENSOR_HELPER("ar0521", CameraSensorHelperAr0521)
> > > >
> > > > +class CameraSensorHelperGc05a2 : public CameraSensorHelper
> > > > +{
> > > > +public:
> > > > +       uint32_t gainCode(double gain) const override
> > > > +       {
> > > > +               uint32_t aeGain = std::clamp((uint32_t)gain, kAeBaseGain_, kStep_ * kAeBaseGain_);
> >
> > This doesn't seem right. 'gain' is expressed as a real number, with 1.0
> > corresponding to a x1 gain.
> >
> > > > +               return aeGain * 0x400 / kAeBaseGain_;
> >
> > If the gain model is linear, you should be able to express it with the
> > standard gain model, without needing to override gain() and gainCode().
> >
> > > > +       }
> > > > +
> > > > +       // `double gain(uint32_t gainCode)` will not be used.
> > >
> > > So don't add it ?
> > >
> > > But how do you mean "won't be used" - you mean ... you won't use it? or
> > > it can't be used?
> > >
> > > How do we correctly determine the gain *used* from a configured gain
> > > code?
> 
> IIUC, If a configured ae gain is set, IIUC for per-frame control use cases,
> in mtkisp7, we feed this information to the IPA proprietary library, which
> calculates the corresponding gainCode for v4l2 camera sensor.
> 
> We don't have the use case to calculate the ae gain from a configured gain
> code. I've seen other pipeline handlers using this function to calculate the
> static metadata `controls::AnalogueGain` though. Is this the only use case?

It's used in open-source IPA modules. To merge support for a new sensor
or a new platform, a fully open source implementation is required. There
will need to be an open-source mtkisp7 IPA module that implements AGC,
so you will need this.

> > > I suspect ... the gain() function should be implemented if the
> > > gainCode() function is implemented. Otherwise callers on the helper will
> > > get inconsistent results.
> >
> > Yes, even if libipa isn't used by the MTK IPA, proper support for the
> > sensor needs to be implemented. Same for patch 2/2 in the series.
> 
> I've been contacting MTK's team, while they're quite inconsistent with
> themselves and very slow to response...
> 
> Previously the conversion is done in their proprietary algorithms, and
> we directly use the results in the IPA.
> 
> Therefore, we might not end up using these functions even if we finally
> dig out MTK's algorithm.
> For per-frame control use cases though, it might simplify the task flows,
> as we probably could calculate the gain code ourselves without waiting
> for IPA proprietary algorithm.
> 
> Let's wait for MTK's response for now...
> 
> > > > +
> > > > +private:
> > > > +       static constexpr uint32_t kStep_ = 16;
> > > > +       static constexpr uint32_t kAeBaseGain_ = 1024;
> > > > +};
> > > > +REGISTER_CAMERA_SENSOR_HELPER("gc05a2", CameraSensorHelperGc05a2)
> > > > +
> > > >  class CameraSensorHelperImx219 : public CameraSensorHelper
> > > >  {
> > > >  public:
> > > > diff --git a/src/libcamera/sensor/camera_sensor_properties.cpp b/src/libcamera/sensor/camera_sensor_properties.cpp
> > > > index 4e5217ab..3e1bd85e 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