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

21340 diff mbox series

gc05a2 is the first sensor used by ciri.

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         | 17 +++++++++++++++++
 .../sensor/camera_sensor_properties.cpp         |  7 +++++++
 2 files changed, 24 insertions(+)

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@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         | 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_);
> +               return aeGain * 0x400 / kAeBaseGain_;
> +       }
> +
> +       // `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?

I suspect ... the gain() function should be implemented if the
gainCode() function is implemented. Otherwise callers on the helper will
get inconsistent results.




> +
> +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 = {
> -- 
> 2.46.0.792.g87dc391469-goog
>

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

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