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

[Cheng-Hao Yang]

Hi Kieran and Laurent,

On Tue, Sep 24, 2024 at 9:19 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> 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?

> >
> > 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...

BR,
Harvey

>
> > > +
> > > +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