[02/10] media: ar0521: Add V4L2_CID_ANALOG_GAIN

  Add support for V4L2_CID_ANALOG_GAIN. The control programs the global
gain register which applies to all color channels.

As both the global digital and analog gains are controlled through a
single register, in order not to overwrite the configured digital gain
we need to read the current register value before modifying it.

Implement a function to read register values and use it before applying
the new analog gain.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ar0521.c | 64 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)
  

Hi Jacopo

On Wed, 5 Oct 2022 at 20:07, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Add support for V4L2_CID_ANALOG_GAIN. The control programs the global
> gain register which applies to all color channels.
>
> As both the global digital and analog gains are controlled through a
> single register, in order not to overwrite the configured digital gain
> we need to read the current register value before modifying it.
>
> Implement a function to read register values and use it before applying
> the new analog gain.
>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/i2c/ar0521.c | 64 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
>
> diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
> index 89f3c01f18ce..581f5e42994d 100644
> --- a/drivers/media/i2c/ar0521.c
> +++ b/drivers/media/i2c/ar0521.c
> @@ -5,6 +5,8 @@
>   * Written by Krzysztof Hałasa
>   */
>
> +#include <asm/unaligned.h>
> +
>  #include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/pm_runtime.h>
> @@ -35,6 +37,11 @@
>  #define AR0521_HEIGHT_BLANKING_MIN     38u /* must be even */
>  #define AR0521_TOTAL_WIDTH_MIN      2968u
>
> +#define AR0521_ANA_GAIN_MIN            0x00
> +#define AR0521_ANA_GAIN_MAX            0x3f
> +#define AR0521_ANA_GAIN_STEP           0x01
> +#define AR0521_ANA_GAIN_DEFAULT                0x00
> +
>  /* AR0521 registers */
>  #define AR0521_REG_VT_PIX_CLK_DIV              0x0300
>  #define AR0521_REG_FRAME_LENGTH_LINES          0x0340
> @@ -55,6 +62,7 @@
>  #define AR0521_REG_RED_GAIN                    0x305A
>  #define AR0521_REG_GREEN2_GAIN                 0x305C
>  #define AR0521_REG_GLOBAL_GAIN                 0x305E
> +#define AR0521_REG_GLOBAL_GAIN_ANA_MASK                0x3f
>
>  #define AR0521_REG_HISPI_TEST_MODE             0x3066
>  #define AR0521_REG_HISPI_TEST_MODE_LP11                  0x0004
> @@ -77,6 +85,7 @@ static const char * const ar0521_supply_names[] = {
>
>  struct ar0521_ctrls {
>         struct v4l2_ctrl_handler handler;
> +       struct v4l2_ctrl *ana_gain;
>         struct {
>                 struct v4l2_ctrl *gain;
>                 struct v4l2_ctrl *red_balance;
> @@ -167,6 +176,36 @@ static int ar0521_write_reg(struct ar0521_dev *sensor, u16 reg, u16 val)
>         return ar0521_write_regs(sensor, buf, 2);
>  }
>
> +static int ar0521_read_reg(struct ar0521_dev *sensor, u16 reg, u16 *val)
> +{
> +       struct i2c_client *client = sensor->i2c_client;
> +       unsigned char buf[2];
> +       struct i2c_msg msg;
> +       int ret;
> +
> +       msg.addr = client->addr;
> +       msg.flags = client->flags;
> +       msg.len = sizeof(u16);
> +       msg.buf = buf;
> +       put_unaligned_be16(reg, buf);
> +
> +       ret = i2c_transfer(client->adapter, &msg, 1);
> +       if (ret < 0)
> +               return ret;
> +
> +       msg.len = sizeof(u16);
> +       msg.flags = client->flags | I2C_M_RD;
> +       msg.buf = buf;
> +
> +       ret = i2c_transfer(client->adapter, &msg, 1);
> +       if (ret < 0)
> +               return ret;
> +
> +       *val = get_unaligned_be16(buf);
> +
> +       return 0;
> +}
> +
>  static int ar0521_set_geometry(struct ar0521_dev *sensor)
>  {
>         /* All dimensions are unsigned 12-bit integers */
> @@ -187,6 +226,21 @@ static int ar0521_set_geometry(struct ar0521_dev *sensor)
>         return ar0521_write_regs(sensor, regs, ARRAY_SIZE(regs));
>  }
>
> +static int ar0521_set_analog_gain(struct ar0521_dev *sensor)
> +{
> +       u16 global_gain;
> +       int ret;
> +
> +       ret = ar0521_read_reg(sensor, AR0521_REG_GLOBAL_GAIN, &global_gain);
> +       if (ret)
> +               return ret;
> +
> +       global_gain &= ~AR0521_REG_GLOBAL_GAIN_ANA_MASK;
> +       global_gain |= sensor->ctrls.ana_gain->val & AR0521_REG_GLOBAL_GAIN_ANA_MASK;
> +
> +       return ar0521_write_reg(sensor, AR0521_REG_GLOBAL_GAIN, global_gain);

Does this work without nasty interactions?

The register reference I have says that the bits 6:0 of 0x3056,
0x3058, 0x305a, 0x305c, and 0x305e are all aliased to register 0x3056.
That means that the writes from ar0521_set_gains for GAIN,
RED_BALANCE, and BLUE_BALANCE will stomp over your ANALOGUE_GAIN.

I also don't see a call to __v4l2_ctrl_handler_setup from
ar0521_set_stream, so whilst there is an explicit call to
ar0521_set_gains, ANALOGUE_GAIN won't be set.

  Dave

[1] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/ar0521.c#L190

> +}
> +
>  static int ar0521_set_gains(struct ar0521_dev *sensor)
>  {
>         int green = sensor->ctrls.gain->val;
> @@ -456,6 +510,9 @@ static int ar0521_s_ctrl(struct v4l2_ctrl *ctrl)
>         case V4L2_CID_VBLANK:
>                 ret = ar0521_set_geometry(sensor);
>                 break;
> +       case V4L2_CID_ANALOGUE_GAIN:
> +               ret = ar0521_set_analog_gain(sensor);
> +               break;
>         case V4L2_CID_GAIN:
>         case V4L2_CID_RED_BALANCE:
>         case V4L2_CID_BLUE_BALANCE:
> @@ -499,6 +556,13 @@ static int ar0521_init_controls(struct ar0521_dev *sensor)
>         /* We can use our own mutex for the ctrl lock */
>         hdl->lock = &sensor->lock;
>
> +       /* Analog gain */
> +       ctrls->ana_gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN,
> +                                           AR0521_ANA_GAIN_MIN,
> +                                           AR0521_ANA_GAIN_MAX,
> +                                           AR0521_ANA_GAIN_STEP,
> +                                           AR0521_ANA_GAIN_DEFAULT);
> +
>         /* Manual gain */
>         ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, 0, 511, 1, 0);
>         ctrls->red_balance = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_RED_BALANCE,
> --
> 2.37.3
>
  

Hi Dave

On Thu, Oct 06, 2022 at 03:44:54PM +0100, Dave Stevenson wrote:
> Hi Jacopo
>
> On Wed, 5 Oct 2022 at 20:07, Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > Add support for V4L2_CID_ANALOG_GAIN. The control programs the global
> > gain register which applies to all color channels.
> >
> > As both the global digital and analog gains are controlled through a
> > single register, in order not to overwrite the configured digital gain
> > we need to read the current register value before modifying it.
> >
> > Implement a function to read register values and use it before applying
> > the new analog gain.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  drivers/media/i2c/ar0521.c | 64 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 64 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
> > index 89f3c01f18ce..581f5e42994d 100644
> > --- a/drivers/media/i2c/ar0521.c
> > +++ b/drivers/media/i2c/ar0521.c
> > @@ -5,6 +5,8 @@
> >   * Written by Krzysztof Hałasa
> >   */
> >
> > +#include <asm/unaligned.h>
> > +
> >  #include <linux/clk.h>
> >  #include <linux/delay.h>
> >  #include <linux/pm_runtime.h>
> > @@ -35,6 +37,11 @@
> >  #define AR0521_HEIGHT_BLANKING_MIN     38u /* must be even */
> >  #define AR0521_TOTAL_WIDTH_MIN      2968u
> >
> > +#define AR0521_ANA_GAIN_MIN            0x00
> > +#define AR0521_ANA_GAIN_MAX            0x3f
> > +#define AR0521_ANA_GAIN_STEP           0x01
> > +#define AR0521_ANA_GAIN_DEFAULT                0x00
> > +
> >  /* AR0521 registers */
> >  #define AR0521_REG_VT_PIX_CLK_DIV              0x0300
> >  #define AR0521_REG_FRAME_LENGTH_LINES          0x0340
> > @@ -55,6 +62,7 @@
> >  #define AR0521_REG_RED_GAIN                    0x305A
> >  #define AR0521_REG_GREEN2_GAIN                 0x305C
> >  #define AR0521_REG_GLOBAL_GAIN                 0x305E
> > +#define AR0521_REG_GLOBAL_GAIN_ANA_MASK                0x3f
> >
> >  #define AR0521_REG_HISPI_TEST_MODE             0x3066
> >  #define AR0521_REG_HISPI_TEST_MODE_LP11                  0x0004
> > @@ -77,6 +85,7 @@ static const char * const ar0521_supply_names[] = {
> >
> >  struct ar0521_ctrls {
> >         struct v4l2_ctrl_handler handler;
> > +       struct v4l2_ctrl *ana_gain;
> >         struct {
> >                 struct v4l2_ctrl *gain;
> >                 struct v4l2_ctrl *red_balance;
> > @@ -167,6 +176,36 @@ static int ar0521_write_reg(struct ar0521_dev *sensor, u16 reg, u16 val)
> >         return ar0521_write_regs(sensor, buf, 2);
> >  }
> >
> > +static int ar0521_read_reg(struct ar0521_dev *sensor, u16 reg, u16 *val)
> > +{
> > +       struct i2c_client *client = sensor->i2c_client;
> > +       unsigned char buf[2];
> > +       struct i2c_msg msg;
> > +       int ret;
> > +
> > +       msg.addr = client->addr;
> > +       msg.flags = client->flags;
> > +       msg.len = sizeof(u16);
> > +       msg.buf = buf;
> > +       put_unaligned_be16(reg, buf);
> > +
> > +       ret = i2c_transfer(client->adapter, &msg, 1);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       msg.len = sizeof(u16);
> > +       msg.flags = client->flags | I2C_M_RD;
> > +       msg.buf = buf;
> > +
> > +       ret = i2c_transfer(client->adapter, &msg, 1);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       *val = get_unaligned_be16(buf);
> > +
> > +       return 0;
> > +}
> > +
> >  static int ar0521_set_geometry(struct ar0521_dev *sensor)
> >  {
> >         /* All dimensions are unsigned 12-bit integers */
> > @@ -187,6 +226,21 @@ static int ar0521_set_geometry(struct ar0521_dev *sensor)
> >         return ar0521_write_regs(sensor, regs, ARRAY_SIZE(regs));
> >  }
> >
> > +static int ar0521_set_analog_gain(struct ar0521_dev *sensor)
> > +{
> > +       u16 global_gain;
> > +       int ret;
> > +
> > +       ret = ar0521_read_reg(sensor, AR0521_REG_GLOBAL_GAIN, &global_gain);
> > +       if (ret)
> > +               return ret;
> > +
> > +       global_gain &= ~AR0521_REG_GLOBAL_GAIN_ANA_MASK;
> > +       global_gain |= sensor->ctrls.ana_gain->val & AR0521_REG_GLOBAL_GAIN_ANA_MASK;
> > +
> > +       return ar0521_write_reg(sensor, AR0521_REG_GLOBAL_GAIN, global_gain);
>
> Does this work without nasty interactions?
>

It seems so :)

> The register reference I have says that the bits 6:0 of 0x3056,
> 0x3058, 0x305a, 0x305c, and 0x305e are all aliased to register 0x3056.
> That means that the writes from ar0521_set_gains for GAIN,
> RED_BALANCE, and BLUE_BALANCE will stomp over your ANALOGUE_GAIN.
>

but you're right the interactions between the registers are not 100%
clear to me yet.

The fact is that libcamera only manipulates ANALOGUE_GAIN while the
other gains are not changed. I guess if one wants to manipulate the
single gains individually this is possible, but when setting the
global gain they will be overwritten ? This seems to be confirmed from
my experiments where changing the BLUE/RED gains has no visible
effects on the image as libcamera constantly adjusts the global gain

> I also don't see a call to __v4l2_ctrl_handler_setup from
> ar0521_set_stream, so whilst there is an explicit call to
> ar0521_set_gains, ANALOGUE_GAIN won't be set.
>
See [PATCH 08/10] media: ar0521: Setup controls at s_stream time
later in the series :)

>   Dave
>
> [1] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/ar0521.c#L190
>
> > +}
> > +
> >  static int ar0521_set_gains(struct ar0521_dev *sensor)
> >  {
> >         int green = sensor->ctrls.gain->val;
> > @@ -456,6 +510,9 @@ static int ar0521_s_ctrl(struct v4l2_ctrl *ctrl)
> >         case V4L2_CID_VBLANK:
> >                 ret = ar0521_set_geometry(sensor);
> >                 break;
> > +       case V4L2_CID_ANALOGUE_GAIN:
> > +               ret = ar0521_set_analog_gain(sensor);
> > +               break;
> >         case V4L2_CID_GAIN:
> >         case V4L2_CID_RED_BALANCE:
> >         case V4L2_CID_BLUE_BALANCE:
> > @@ -499,6 +556,13 @@ static int ar0521_init_controls(struct ar0521_dev *sensor)
> >         /* We can use our own mutex for the ctrl lock */
> >         hdl->lock = &sensor->lock;
> >
> > +       /* Analog gain */
> > +       ctrls->ana_gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN,
> > +                                           AR0521_ANA_GAIN_MIN,
> > +                                           AR0521_ANA_GAIN_MAX,
> > +                                           AR0521_ANA_GAIN_STEP,
> > +                                           AR0521_ANA_GAIN_DEFAULT);
> > +
> >         /* Manual gain */
> >         ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, 0, 511, 1, 0);
> >         ctrls->red_balance = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_RED_BALANCE,
> > --
> > 2.37.3
> >
  

Hello,

On Thu, Oct 06, 2022 at 05:00:15PM +0200, Jacopo Mondi wrote:
> On Thu, Oct 06, 2022 at 03:44:54PM +0100, Dave Stevenson wrote:
> > On Wed, 5 Oct 2022 at 20:07, Jacopo Mondi wrote:
> > >
> > > Add support for V4L2_CID_ANALOG_GAIN. The control programs the global
> > > gain register which applies to all color channels.
> > >
> > > As both the global digital and analog gains are controlled through a
> > > single register, in order not to overwrite the configured digital gain
> > > we need to read the current register value before modifying it.
> > >
> > > Implement a function to read register values and use it before applying
> > > the new analog gain.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  drivers/media/i2c/ar0521.c | 64 ++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 64 insertions(+)
> > >
> > > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
> > > index 89f3c01f18ce..581f5e42994d 100644
> > > --- a/drivers/media/i2c/ar0521.c
> > > +++ b/drivers/media/i2c/ar0521.c
> > > @@ -5,6 +5,8 @@
> > >   * Written by Krzysztof Hałasa
> > >   */
> > >
> > > +#include <asm/unaligned.h>
> > > +
> > >  #include <linux/clk.h>
> > >  #include <linux/delay.h>
> > >  #include <linux/pm_runtime.h>
> > > @@ -35,6 +37,11 @@
> > >  #define AR0521_HEIGHT_BLANKING_MIN     38u /* must be even */
> > >  #define AR0521_TOTAL_WIDTH_MIN      2968u
> > >
> > > +#define AR0521_ANA_GAIN_MIN            0x00
> > > +#define AR0521_ANA_GAIN_MAX            0x3f
> > > +#define AR0521_ANA_GAIN_STEP           0x01
> > > +#define AR0521_ANA_GAIN_DEFAULT                0x00
> > > +
> > >  /* AR0521 registers */
> > >  #define AR0521_REG_VT_PIX_CLK_DIV              0x0300
> > >  #define AR0521_REG_FRAME_LENGTH_LINES          0x0340
> > > @@ -55,6 +62,7 @@
> > >  #define AR0521_REG_RED_GAIN                    0x305A
> > >  #define AR0521_REG_GREEN2_GAIN                 0x305C
> > >  #define AR0521_REG_GLOBAL_GAIN                 0x305E
> > > +#define AR0521_REG_GLOBAL_GAIN_ANA_MASK                0x3f
> > >
> > >  #define AR0521_REG_HISPI_TEST_MODE             0x3066
> > >  #define AR0521_REG_HISPI_TEST_MODE_LP11                  0x0004
> > > @@ -77,6 +85,7 @@ static const char * const ar0521_supply_names[] = {
> > >
> > >  struct ar0521_ctrls {
> > >         struct v4l2_ctrl_handler handler;
> > > +       struct v4l2_ctrl *ana_gain;
> > >         struct {
> > >                 struct v4l2_ctrl *gain;
> > >                 struct v4l2_ctrl *red_balance;
> > > @@ -167,6 +176,36 @@ static int ar0521_write_reg(struct ar0521_dev *sensor, u16 reg, u16 val)
> > >         return ar0521_write_regs(sensor, buf, 2);
> > >  }
> > >
> > > +static int ar0521_read_reg(struct ar0521_dev *sensor, u16 reg, u16 *val)
> > > +{
> > > +       struct i2c_client *client = sensor->i2c_client;
> > > +       unsigned char buf[2];
> > > +       struct i2c_msg msg;
> > > +       int ret;
> > > +
> > > +       msg.addr = client->addr;
> > > +       msg.flags = client->flags;
> > > +       msg.len = sizeof(u16);
> > > +       msg.buf = buf;
> > > +       put_unaligned_be16(reg, buf);
> > > +
> > > +       ret = i2c_transfer(client->adapter, &msg, 1);
> > > +       if (ret < 0)
> > > +               return ret;
> > > +
> > > +       msg.len = sizeof(u16);
> > > +       msg.flags = client->flags | I2C_M_RD;
> > > +       msg.buf = buf;
> > > +
> > > +       ret = i2c_transfer(client->adapter, &msg, 1);
> > > +       if (ret < 0)
> > > +               return ret;
> > > +
> > > +       *val = get_unaligned_be16(buf);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > >  static int ar0521_set_geometry(struct ar0521_dev *sensor)
> > >  {
> > >         /* All dimensions are unsigned 12-bit integers */
> > > @@ -187,6 +226,21 @@ static int ar0521_set_geometry(struct ar0521_dev *sensor)
> > >         return ar0521_write_regs(sensor, regs, ARRAY_SIZE(regs));
> > >  }
> > >
> > > +static int ar0521_set_analog_gain(struct ar0521_dev *sensor)
> > > +{
> > > +       u16 global_gain;
> > > +       int ret;
> > > +
> > > +       ret = ar0521_read_reg(sensor, AR0521_REG_GLOBAL_GAIN, &global_gain);
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       global_gain &= ~AR0521_REG_GLOBAL_GAIN_ANA_MASK;
> > > +       global_gain |= sensor->ctrls.ana_gain->val & AR0521_REG_GLOBAL_GAIN_ANA_MASK;
> > > +
> > > +       return ar0521_write_reg(sensor, AR0521_REG_GLOBAL_GAIN, global_gain);
> >
> > Does this work without nasty interactions?
> 
> It seems so :)
> 
> > The register reference I have says that the bits 6:0 of 0x3056,
> > 0x3058, 0x305a, 0x305c, and 0x305e are all aliased to register 0x3056.
> > That means that the writes from ar0521_set_gains for GAIN,
> > RED_BALANCE, and BLUE_BALANCE will stomp over your ANALOGUE_GAIN.
> 
> but you're right the interactions between the registers are not 100%
> clear to me yet.
> 
> The fact is that libcamera only manipulates ANALOGUE_GAIN while the
> other gains are not changed. I guess if one wants to manipulate the
> single gains individually this is possible, but when setting the
> global gain they will be overwritten ? This seems to be confirmed from
> my experiments where changing the BLUE/RED gains has no visible
> effects on the image as libcamera constantly adjusts the global gain

I'm tempted to drop support for the colour gains really, and turn the
V4L2_CID_GAIN into V4L2_CID_DIGITAL_GAIN. Digital colour gains can still
be useful on platforms that have no ISP, but I think we need an array of
gains in that case, not abusing V4L2_CID_RED_BALANCE and
V4L2_CID_BLUE_BALANCE. Any objection ?

> > I also don't see a call to __v4l2_ctrl_handler_setup from
> > ar0521_set_stream, so whilst there is an explicit call to
> > ar0521_set_gains, ANALOGUE_GAIN won't be set.
>
> See [PATCH 08/10] media: ar0521: Setup controls at s_stream time
> later in the series :)
> 
> > [1] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/ar0521.c#L190
> >
> > > +}
> > > +
> > >  static int ar0521_set_gains(struct ar0521_dev *sensor)
> > >  {
> > >         int green = sensor->ctrls.gain->val;
> > > @@ -456,6 +510,9 @@ static int ar0521_s_ctrl(struct v4l2_ctrl *ctrl)
> > >         case V4L2_CID_VBLANK:
> > >                 ret = ar0521_set_geometry(sensor);
> > >                 break;
> > > +       case V4L2_CID_ANALOGUE_GAIN:
> > > +               ret = ar0521_set_analog_gain(sensor);
> > > +               break;
> > >         case V4L2_CID_GAIN:
> > >         case V4L2_CID_RED_BALANCE:
> > >         case V4L2_CID_BLUE_BALANCE:
> > > @@ -499,6 +556,13 @@ static int ar0521_init_controls(struct ar0521_dev *sensor)
> > >         /* We can use our own mutex for the ctrl lock */
> > >         hdl->lock = &sensor->lock;
> > >
> > > +       /* Analog gain */
> > > +       ctrls->ana_gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN,
> > > +                                           AR0521_ANA_GAIN_MIN,
> > > +                                           AR0521_ANA_GAIN_MAX,
> > > +                                           AR0521_ANA_GAIN_STEP,
> > > +                                           AR0521_ANA_GAIN_DEFAULT);
> > > +
> > >         /* Manual gain */
> > >         ctrls->gain = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_GAIN, 0, 511, 1, 0);
> > >         ctrls->red_balance = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_RED_BALANCE,
  

Hi Jacopo and Co,

Jacopo Mondi <jacopo@jmondi.org> writes:

> +static int ar0521_read_reg(struct ar0521_dev *sensor, u16 reg, u16 *val)
> +{
> +	struct i2c_client *client = sensor->i2c_client;
> +	unsigned char buf[2];
> +	struct i2c_msg msg;
> +	int ret;
> +
> +	msg.addr = client->addr;
> +	msg.flags = client->flags;
> +	msg.len = sizeof(u16);
> +	msg.buf = buf;
> +	put_unaligned_be16(reg, buf);
> +
> +	ret = i2c_transfer(client->adapter, &msg, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	msg.len = sizeof(u16);
> +	msg.flags = client->flags | I2C_M_RD;
> +	msg.buf = buf;
> +
> +	ret = i2c_transfer(client->adapter, &msg, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = get_unaligned_be16(buf);
> +
> +	return 0;
> +}

Why not simply use a shadow register?

> +static int ar0521_set_analog_gain(struct ar0521_dev *sensor)
> +{
> +	u16 global_gain;
> +	int ret;
> +
> +	ret = ar0521_read_reg(sensor, AR0521_REG_GLOBAL_GAIN, &global_gain);
> +	if (ret)
> +		return ret;
> +
> +	global_gain &= ~AR0521_REG_GLOBAL_GAIN_ANA_MASK;
> +	global_gain |= sensor->ctrls.ana_gain->val & AR0521_REG_GLOBAL_GAIN_ANA_MASK;
> +
> +	return ar0521_write_reg(sensor, AR0521_REG_GLOBAL_GAIN, global_gain);

This one is simple: you can't write to AR0521_REG_GLOBAL_GAIN.
You can write to individual color gain registers (any will do for analog
gain), but writing to AR0521_REG_GLOBAL_GAIN will reset all the digital
gains as well. Reading the register doesn't give you anything
interesting, either (the analog gain which you overwrite anyway).

BTW ISP can't really do that color balancing for you, since the sensor
operates at its native bit resolution and ISP is limited to the output
format, which is currently only 8-bit.
  

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> I'm tempted to drop support for the colour gains really, and turn the
> V4L2_CID_GAIN into V4L2_CID_DIGITAL_GAIN. Digital colour gains can still
> be useful on platforms that have no ISP, but I think we need an array of
> gains in that case, not abusing V4L2_CID_RED_BALANCE and
> V4L2_CID_BLUE_BALANCE. Any objection ?

I'm fine with spliting it into analog/digital as long as there is a way
to set individual R/G/B (digital) gain values.
  

Hi Krzysztof

On Fri, Oct 07, 2022 at 07:20:51AM +0200, Krzysztof Hałasa wrote:
> Hi Jacopo and Co,
>
> Jacopo Mondi <jacopo@jmondi.org> writes:
>
> > +static int ar0521_read_reg(struct ar0521_dev *sensor, u16 reg, u16 *val)
> > +{
> > +	struct i2c_client *client = sensor->i2c_client;
> > +	unsigned char buf[2];
> > +	struct i2c_msg msg;
> > +	int ret;
> > +
> > +	msg.addr = client->addr;
> > +	msg.flags = client->flags;
> > +	msg.len = sizeof(u16);
> > +	msg.buf = buf;
> > +	put_unaligned_be16(reg, buf);
> > +
> > +	ret = i2c_transfer(client->adapter, &msg, 1);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	msg.len = sizeof(u16);
> > +	msg.flags = client->flags | I2C_M_RD;
> > +	msg.buf = buf;
> > +
> > +	ret = i2c_transfer(client->adapter, &msg, 1);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	*val = get_unaligned_be16(buf);
> > +
> > +	return 0;
> > +}
>
> Why not simply use a shadow register?
>

Sorry I didn't get you. Care to expand ?

> > +static int ar0521_set_analog_gain(struct ar0521_dev *sensor)
> > +{
> > +	u16 global_gain;
> > +	int ret;
> > +
> > +	ret = ar0521_read_reg(sensor, AR0521_REG_GLOBAL_GAIN, &global_gain);
> > +	if (ret)
> > +		return ret;
> > +
> > +	global_gain &= ~AR0521_REG_GLOBAL_GAIN_ANA_MASK;
> > +	global_gain |= sensor->ctrls.ana_gain->val & AR0521_REG_GLOBAL_GAIN_ANA_MASK;
> > +
> > +	return ar0521_write_reg(sensor, AR0521_REG_GLOBAL_GAIN, global_gain);
>
> This one is simple: you can't write to AR0521_REG_GLOBAL_GAIN.

Uh

I can guarantee you it works :)

> You can write to individual color gain registers (any will do for analog
> gain), but writing to AR0521_REG_GLOBAL_GAIN will reset all the digital
> gains as well. Reading the register doesn't give you anything

I think that's ok, isn't it ? If one wants to control the global gain
it goes through this register, if individual gains need to be
configured one should not set the global gain ?

> interesting, either (the analog gain which you overwrite anyway).

The high bits are the global digital gain, and I need to read its value in
order not to overwrite them.

>
> BTW ISP can't really do that color balancing for you, since the sensor
> operates at its native bit resolution and ISP is limited to the output
> format, which is currently only 8-bit.

I'm not sure what do you mean here either :)

> --
> Krzysztof "Chris" Hałasa
>
> Sieć Badawcza Łukasiewicz
> Przemysłowy Instytut Automatyki i Pomiarów PIAP
> Al. Jerozolimskie 202, 02-486 Warszawa
  

Hi Krzysztof,

On Fri, Oct 07, 2022 at 07:28:46AM +0200, Krzysztof Hałasa wrote:
> Laurent Pinchart writes:
> 
> > I'm tempted to drop support for the colour gains really, and turn the
> > V4L2_CID_GAIN into V4L2_CID_DIGITAL_GAIN. Digital colour gains can still
> > be useful on platforms that have no ISP, but I think we need an array of
> > gains in that case, not abusing V4L2_CID_RED_BALANCE and
> > V4L2_CID_BLUE_BALANCE. Any objection ?
> 
> I'm fine with spliting it into analog/digital as long as there is a way
> to set individual R/G/B (digital) gain values.

With the controls we have today in V4L2, we could map
V4L2_CID_RED_BALANCE and V4L2_CID_BLUE_BALANCE to the red and blue
digital gains, with V4L2_CID_DIGITAL_GAIN setting the global digital
gain.

I'm tempted to bite the bullet and define a new
V4L2_CID_DIGITAL_COLOR_GAINS control that would expose an array of
gains, but if we extend the API for that, I think we should also include
support for HDR at the same time, with at least T1/T2 sets of gains.

Sakari, any opinion ?
  

Hi Jacopo,

On Fri, Oct 07, 2022 at 09:17:25AM +0200, Jacopo Mondi wrote:
> On Fri, Oct 07, 2022 at 07:20:51AM +0200, Krzysztof Hałasa wrote:
> > Jacopo Mondi writes:
> >
> > > +static int ar0521_read_reg(struct ar0521_dev *sensor, u16 reg, u16 *val)
> > > +{
> > > +	struct i2c_client *client = sensor->i2c_client;
> > > +	unsigned char buf[2];
> > > +	struct i2c_msg msg;
> > > +	int ret;
> > > +
> > > +	msg.addr = client->addr;
> > > +	msg.flags = client->flags;
> > > +	msg.len = sizeof(u16);
> > > +	msg.buf = buf;
> > > +	put_unaligned_be16(reg, buf);
> > > +
> > > +	ret = i2c_transfer(client->adapter, &msg, 1);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	msg.len = sizeof(u16);
> > > +	msg.flags = client->flags | I2C_M_RD;
> > > +	msg.buf = buf;
> > > +
> > > +	ret = i2c_transfer(client->adapter, &msg, 1);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	*val = get_unaligned_be16(buf);
> > > +
> > > +	return 0;
> > > +}
> >
> > Why not simply use a shadow register?
> 
> Sorry I didn't get you. Care to expand ?

I think Krzysztof meant caching the value in the ar0521_dev structure,
so it doesn't have to be read back. I2C is slow, let's avoid reads as
much as possible.

This being said, if all gain controls are in the same cluster, you won't
need to read back or cache anything yourself, the control framework will
handle that for you.

> > > +static int ar0521_set_analog_gain(struct ar0521_dev *sensor)
> > > +{
> > > +	u16 global_gain;
> > > +	int ret;
> > > +
> > > +	ret = ar0521_read_reg(sensor, AR0521_REG_GLOBAL_GAIN, &global_gain);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	global_gain &= ~AR0521_REG_GLOBAL_GAIN_ANA_MASK;
> > > +	global_gain |= sensor->ctrls.ana_gain->val & AR0521_REG_GLOBAL_GAIN_ANA_MASK;
> > > +
> > > +	return ar0521_write_reg(sensor, AR0521_REG_GLOBAL_GAIN, global_gain);
> >
> > This one is simple: you can't write to AR0521_REG_GLOBAL_GAIN.
> 
> Uh
> 
> I can guarantee you it works :)
> 
> > You can write to individual color gain registers (any will do for analog
> > gain), but writing to AR0521_REG_GLOBAL_GAIN will reset all the digital
> > gains as well. Reading the register doesn't give you anything
> 
> I think that's ok, isn't it ? If one wants to control the global gain
> it goes through this register, if individual gains need to be
> configured one should not set the global gain ?

The issue is that if the user has set different digital gains for the
different channels, you will overwrite them with the same below for all
channels. That's not good.

What you could experiment with is register 0x0204
(analog_gain_code_global) which seem to provide a global analog gain
without overwriting the digital gains, but it's not entirely clear from
the documentation if it will work. The register name comes from
SMIA++/CCS, but the documentation doesn't match the coarse/fine gain
model, experiments would be needed. Another option is register 0x3028,
which is also named analog_gain_code_global, but is documented
differently.

Could you btw read registers 0x0000 to 0x00ff and provide the data ?

> > interesting, either (the analog gain which you overwrite anyway).
> 
> The high bits are the global digital gain, and I need to read its value in
> order not to overwrite them.
> 
> > BTW ISP can't really do that color balancing for you, since the sensor
> > operates at its native bit resolution and ISP is limited to the output
> > format, which is currently only 8-bit.
> 
> I'm not sure what do you mean here either :)

I'm also not sure to see the problem.
  

Jacopo Mondi <jacopo@jmondi.org> writes:

>> BTW ISP can't really do that color balancing for you, since the sensor
>> operates at its native bit resolution and ISP is limited to the output
>> format, which is currently only 8-bit.
>
> I'm not sure what do you mean here either :)

Well, the sensor does DSP on 12 bits internally, or something alike.
Then it outputs (currently) 8 bits of data, the rest being lost. If you
do color balancing etc. on this, you may get worse results (like color
banding).
  

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> I think Krzysztof meant caching the value in the ar0521_dev structure,
> so it doesn't have to be read back. I2C is slow, let's avoid reads as
> much as possible.

Right, ATM there is no I2C read function at all in this driver.

> This being said, if all gain controls are in the same cluster, you won't
> need to read back or cache anything yourself, the control framework will
> handle that for you.

Yep. Then there could be a common gain for all 3 colors, or a set of 3
for R/G/B. I only don't know what should the former one return on read,
if individual values are used.
  

On Fri, Oct 07, 2022 at 02:01:19PM +0200, Krzysztof Hałasa wrote:
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
> 
> > I think Krzysztof meant caching the value in the ar0521_dev structure,
> > so it doesn't have to be read back. I2C is slow, let's avoid reads as
> > much as possible.
> 
> Right, ATM there is no I2C read function at all in this driver.
> 
> > This being said, if all gain controls are in the same cluster, you won't
> > need to read back or cache anything yourself, the control framework will
> > handle that for you.
> 
> Yep. Then there could be a common gain for all 3 colors, or a set of 3
> for R/G/B. I only don't know what should the former one return on read,
> if individual values are used.

That's an interesting question. We could make it a write-only control,
but that would complicate userspace applications that don't need the
per-color controls. This being said, I'm not sure I've seen any use case
for reading back gain controls (beside on sensors that implement AEGC).
  

On Fri, Oct 07, 2022 at 01:56:11PM +0200, Krzysztof Hałasa wrote:
> Jacopo Mondi writes:
> 
> >> BTW ISP can't really do that color balancing for you, since the sensor
> >> operates at its native bit resolution and ISP is limited to the output
> >> format, which is currently only 8-bit.
> >
> > I'm not sure what do you mean here either :)
> 
> Well, the sensor does DSP on 12 bits internally, or something alike.
> Then it outputs (currently) 8 bits of data, the rest being lost. If you
> do color balancing etc. on this, you may get worse results (like color
> banding).

But if you apply colour gains in the sensor, they will apply before
black level correction or lens shading compensation in the ISP, which I
think will cause other issues, possibly worse.

Out of curiosity, if you can tell, what do you use this sensor with ?
  

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> But if you apply colour gains in the sensor, they will apply before
> black level correction or lens shading compensation in the ISP, which I
> think will cause other issues, possibly worse.

Maybe. I don't need these, but I need white balance control. Video
streaming only.

> Out of curiosity, if you can tell, what do you use this sensor with ?

i.MX6 + its built-in H.264 encoder. NEON fixed-point debayering with
half the resolution etc.

That's a "mature" :-) project, though.
  

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> That's an interesting question. We could make it a write-only control,
> but that would complicate userspace applications that don't need the
> per-color controls. This being said, I'm not sure I've seen any use case
> for reading back gain controls (beside on sensors that implement AEGC).

Maybe we could have a control which is write-only "on demand"?
I mean when an individual control is written, and until the main
control is written again.
  

Hi Laurent,

On Fri, Oct 07, 2022 at 11:20:13AM +0300, Laurent Pinchart wrote:
> Hi Krzysztof,
> 
> On Fri, Oct 07, 2022 at 07:28:46AM +0200, Krzysztof Hałasa wrote:
> > Laurent Pinchart writes:
> > 
> > > I'm tempted to drop support for the colour gains really, and turn the
> > > V4L2_CID_GAIN into V4L2_CID_DIGITAL_GAIN. Digital colour gains can still
> > > be useful on platforms that have no ISP, but I think we need an array of
> > > gains in that case, not abusing V4L2_CID_RED_BALANCE and
> > > V4L2_CID_BLUE_BALANCE. Any objection ?
> > 
> > I'm fine with spliting it into analog/digital as long as there is a way
> > to set individual R/G/B (digital) gain values.
> 
> With the controls we have today in V4L2, we could map
> V4L2_CID_RED_BALANCE and V4L2_CID_BLUE_BALANCE to the red and blue
> digital gains, with V4L2_CID_DIGITAL_GAIN setting the global digital
> gain.
> 
> I'm tempted to bite the bullet and define a new
> V4L2_CID_DIGITAL_COLOR_GAINS control that would expose an array of
> gains, but if we extend the API for that, I think we should also include
> support for HDR at the same time, with at least T1/T2 sets of gains.
> 
> Sakari, any opinion ?

Would you use multiple controls for that or just a single one?

The size of a matrix control is not changeable dynamically so I presume the
driver would create as large control as needed, and program to hardware as
much as needed.
  

Hi Sakari,

(CC'ing Hans for the discussion about the control framework)

On Wed, Oct 12, 2022 at 09:54:54PM +0300, Sakari Ailus wrote:
> On Fri, Oct 07, 2022 at 11:20:13AM +0300, Laurent Pinchart wrote:
> > On Fri, Oct 07, 2022 at 07:28:46AM +0200, Krzysztof Hałasa wrote:
> > > Laurent Pinchart writes:
> > > 
> > > > I'm tempted to drop support for the colour gains really, and turn the
> > > > V4L2_CID_GAIN into V4L2_CID_DIGITAL_GAIN. Digital colour gains can still
> > > > be useful on platforms that have no ISP, but I think we need an array of
> > > > gains in that case, not abusing V4L2_CID_RED_BALANCE and
> > > > V4L2_CID_BLUE_BALANCE. Any objection ?
> > > 
> > > I'm fine with spliting it into analog/digital as long as there is a way
> > > to set individual R/G/B (digital) gain values.
> > 
> > With the controls we have today in V4L2, we could map
> > V4L2_CID_RED_BALANCE and V4L2_CID_BLUE_BALANCE to the red and blue
> > digital gains, with V4L2_CID_DIGITAL_GAIN setting the global digital
> > gain.
> > 
> > I'm tempted to bite the bullet and define a new
> > V4L2_CID_DIGITAL_COLOR_GAINS control that would expose an array of
> > gains, but if we extend the API for that, I think we should also include
> > support for HDR at the same time, with at least T1/T2 sets of gains.
> > 
> > Sakari, any opinion ?
> 
> Would you use multiple controls for that or just a single one?

That's what we need to decide :-)

> The size of a matrix control is not changeable dynamically so I presume the

We now have

    * - ``V4L2_CTRL_FLAG_DYNAMIC_ARRAY``
      - 0x0800
      - This control is a dynamically sized 1-dimensional array. It
        behaves the same as a regular array, except that the number
        of elements as reported by the ``elems`` field is between 1 and
        ``dims[0]``. So setting the control with a differently sized
        array will change the ``elems`` field when the control is
        queried afterwards.

that allows changing a control size dynamically from userspace. It only
works for 1D controls though. The kernel can also change the dimensions
of a control at any time, and that could be done when the HDR control
would be set to dual-gain mode to turn the gain controls into arrays.

> driver would create as large control as needed, and program to hardware as
> much as needed.

That's what we need to decide :-) I'm tempted to go for a single control
that would cover both the multi-channel and multi-gain needs.

V4L2_CID_ANALOG_GAIN control would become an array control of two
elements, one for T1 and one for T2. It may confuse some applications.
For new drivers that could be fine. For existing drivers, an application
that sets the control as if it were a regular V4L2_CID_ANALOG_GAIN of
type V4L2_CTRL_TYPE_INTEGER would break.

This may be fixable in the V4L2 control framework, by only allowing
get/set of a subset of an array control. I'm not sure that't desirable
in the general case though. If the control dimensions were changed by
the driver when turning dual-gain mode on (or off) we would only need (I
think) to update v4l2_s_ctrl() to allow writing array controls when the
array contains a single element, which should be less of a problem.

For the digital gains, V4L2_CID_DIGITAL_GAIN would become a 2D array,
with one dimension covering the colour channels, and the other dimension
covering the T1/T2 gains.
  

I'm back with a few more data...

On Fri, Oct 07, 2022 at 11:30:32AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Fri, Oct 07, 2022 at 09:17:25AM +0200, Jacopo Mondi wrote:
> > On Fri, Oct 07, 2022 at 07:20:51AM +0200, Krzysztof Hałasa wrote:
> > > Jacopo Mondi writes:
> > >
> > > > +static int ar0521_read_reg(struct ar0521_dev *sensor, u16 reg, u16 *val)
> > > > +{
> > > > +	struct i2c_client *client = sensor->i2c_client;
> > > > +	unsigned char buf[2];
> > > > +	struct i2c_msg msg;
> > > > +	int ret;
> > > > +
> > > > +	msg.addr = client->addr;
> > > > +	msg.flags = client->flags;
> > > > +	msg.len = sizeof(u16);
> > > > +	msg.buf = buf;
> > > > +	put_unaligned_be16(reg, buf);
> > > > +
> > > > +	ret = i2c_transfer(client->adapter, &msg, 1);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	msg.len = sizeof(u16);
> > > > +	msg.flags = client->flags | I2C_M_RD;
> > > > +	msg.buf = buf;
> > > > +
> > > > +	ret = i2c_transfer(client->adapter, &msg, 1);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	*val = get_unaligned_be16(buf);
> > > > +
> > > > +	return 0;
> > > > +}
> > >
> > > Why not simply use a shadow register?
> >
> > Sorry I didn't get you. Care to expand ?
>
> I think Krzysztof meant caching the value in the ar0521_dev structure,
> so it doesn't have to be read back. I2C is slow, let's avoid reads as
> much as possible.
>
> This being said, if all gain controls are in the same cluster, you won't
> need to read back or cache anything yourself, the control framework will
> handle that for you.
>
> > > > +static int ar0521_set_analog_gain(struct ar0521_dev *sensor)
> > > > +{
> > > > +	u16 global_gain;
> > > > +	int ret;
> > > > +
> > > > +	ret = ar0521_read_reg(sensor, AR0521_REG_GLOBAL_GAIN, &global_gain);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	global_gain &= ~AR0521_REG_GLOBAL_GAIN_ANA_MASK;
> > > > +	global_gain |= sensor->ctrls.ana_gain->val & AR0521_REG_GLOBAL_GAIN_ANA_MASK;
> > > > +
> > > > +	return ar0521_write_reg(sensor, AR0521_REG_GLOBAL_GAIN, global_gain);
> > >
> > > This one is simple: you can't write to AR0521_REG_GLOBAL_GAIN.
> >
> > Uh
> >
> > I can guarantee you it works :)
> >
> > > You can write to individual color gain registers (any will do for analog
> > > gain), but writing to AR0521_REG_GLOBAL_GAIN will reset all the digital
> > > gains as well. Reading the register doesn't give you anything
> >
> > I think that's ok, isn't it ? If one wants to control the global gain
> > it goes through this register, if individual gains need to be
> > configured one should not set the global gain ?
>
> The issue is that if the user has set different digital gains for the
> different channels, you will overwrite them with the same below for all
> channels. That's not good.
>

Yes, the global digital gain overwrites the per-channel ones

> What you could experiment with is register 0x0204

Nope, that's a no-op

> (analog_gain_code_global) which seem to provide a global analog gain
> without overwriting the digital gains, but it's not entirely clear from
> the documentation if it will work. The register name comes from
> SMIA++/CCS, but the documentation doesn't match the coarse/fine gain
> model, experiments would be needed. Another option is register 0x3028,

0x3028, albeit documented differently, effectively changes the global
analog gain as the lower 6 bits of 0x305e do.

Values set to 0x3028 are reflected in 0x305e and viceversa, so I think
that V4L2_CID_ANALOG_GAIN can be safely directed to 0x3028 without
the need to read back the current digital gain value before reading the
register.

The per-channel analog gains component will be overwritten
but considering that the existing CID_GAIN, CID_BLUE_BALANCE and
CID_RED_BALANCE cluster computes the green/red/blue analog and digital
gains as follows:

	int green = sensor->ctrls.gain->val;
	int red = max(green + sensor->ctrls.red_balance->val, 0);
	int blue = max(green + sensor->ctrls.blue_balance->val, 0);
	unsigned int gain = min(red, min(green, blue));
	unsigned int analog = min(gain, 64u); /* range is 0 - 127 */

So that CID_GAIN is mapped on the green channel, the only way to make
this less nasty would be to actually define multi-dimensional
DIGITAL and ANALOG gain controls, where the three channels are mapped
to the three dimensions, and use CID_DIGITAL_GAIN and CID_ANALOG_GAIN
as global control gains (with the caveat that the global gains are
meant to override the per channel ones).

Personally I'm fine with a single, non-clusterized CID_ANALOG_GAIN and
leave the existing cluster as it is. The multi-dimensional control
might indeed prove useful albeit it will break existing applications
that rely on the CID_GAIN/RED,BLUE_BALANCE cluster.

> which is also named analog_gain_code_global, but is documented
> differently.
>
> Could you btw read registers 0x0000 to 0x00ff and provide the data ?

There is nothing interesting there if not default values. I was hoping
that analogue_gain_m0 analogue_gain_c0 and analogue_gain_m1
analogue_gain_c1 would provide a way to inject gains using the
standard CCS gain model, but those registers are said to be read-only
and do not change when the global analog gain changes, so I wonder if
the SMIA/CCS interface for this chip is actually enabled (it might
depend on the fw revision ?)

>
> > > interesting, either (the analog gain which you overwrite anyway).
> >
> > The high bits are the global digital gain, and I need to read its value in
> > order not to overwrite them.
> >
> > > BTW ISP can't really do that color balancing for you, since the sensor
> > > operates at its native bit resolution and ISP is limited to the output
> > > format, which is currently only 8-bit.
> >
> > I'm not sure what do you mean here either :)
>
> I'm also not sure to see the problem.
>
> --
> Regards,
>
> Laurent Pinchart
  

Hi Jacopo,

On Mon, Oct 17, 2022 at 05:10:03PM +0200, Jacopo Mondi wrote:
> > which is also named analog_gain_code_global, but is documented
> > differently.
> >
> > Could you btw read registers 0x0000 to 0x00ff and provide the data ?
> 
> There is nothing interesting there if not default values. I was hoping
> that analogue_gain_m0 analogue_gain_c0 and analogue_gain_m1
> analogue_gain_c1 would provide a way to inject gains using the
> standard CCS gain model, but those registers are said to be read-only

The m[01] and c[01] factors in the CCS analogue gain formula are constants
that determine how the sensor's analogue gain setting translates to actual
analogue gain. They are not intended to be modifiable at runtime.
  

Hi Sakari,

On Mon, Oct 17, 2022 at 06:57:48PM +0300, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Mon, Oct 17, 2022 at 05:10:03PM +0200, Jacopo Mondi wrote:
> > > which is also named analog_gain_code_global, but is documented
> > > differently.
> > >
> > > Could you btw read registers 0x0000 to 0x00ff and provide the data ?
> >
> > There is nothing interesting there if not default values. I was hoping
> > that analogue_gain_m0 analogue_gain_c0 and analogue_gain_m1
> > analogue_gain_c1 would provide a way to inject gains using the
> > standard CCS gain model, but those registers are said to be read-only
>
> The m[01] and c[01] factors in the CCS analogue gain formula are constants
> that determine how the sensor's analogue gain setting translates to actual
> analogue gain. They are not intended to be modifiable at runtime.
>

You're right sorry, indeed they're constant.

For this sensor:
analogue_gain_type: 0
analogue_gain_m0: 1
analogue_gain_c0: 0
analogue_gain_m1: 0
analogue_gain_c1: 4

I should be capable of programming the global analog gain using the linear
CCS gain model if the sensor is actually CCS compliant.

        gain = m0 * x + c0 / m1 * x + c1
             = R0x0204 / 4

However, the application developer guide shows the gain to be set
through manufacturer specific registers (0x3028 or 0x305e) and I cannot
find much correlations between the manufacturer specific gain model
(a piecewise exponential function) and the model described by CCS, which
seems way simpler.

> --
> Kind regards,
>
> Sakari Ailus
  

Hi Jacopo,

On Mon, Oct 17, 2022 at 06:31:59PM +0200, Jacopo Mondi wrote:
> Hi Sakari,
> 
> On Mon, Oct 17, 2022 at 06:57:48PM +0300, Sakari Ailus wrote:
> > Hi Jacopo,
> >
> > On Mon, Oct 17, 2022 at 05:10:03PM +0200, Jacopo Mondi wrote:
> > > > which is also named analog_gain_code_global, but is documented
> > > > differently.
> > > >
> > > > Could you btw read registers 0x0000 to 0x00ff and provide the data ?
> > >
> > > There is nothing interesting there if not default values. I was hoping
> > > that analogue_gain_m0 analogue_gain_c0 and analogue_gain_m1
> > > analogue_gain_c1 would provide a way to inject gains using the
> > > standard CCS gain model, but those registers are said to be read-only
> >
> > The m[01] and c[01] factors in the CCS analogue gain formula are constants
> > that determine how the sensor's analogue gain setting translates to actual
> > analogue gain. They are not intended to be modifiable at runtime.
> >
> 
> You're right sorry, indeed they're constant.
> 
> For this sensor:
> analogue_gain_type: 0
> analogue_gain_m0: 1
> analogue_gain_c0: 0
> analogue_gain_m1: 0
> analogue_gain_c1: 4
> 
> I should be capable of programming the global analog gain using the linear
> CCS gain model if the sensor is actually CCS compliant.
> 
>         gain = m0 * x + c0 / m1 * x + c1
>              = R0x0204 / 4
> 
> However, the application developer guide shows the gain to be set
> through manufacturer specific registers (0x3028 or 0x305e) and I cannot
> find much correlations between the manufacturer specific gain model
> (a piecewise exponential function) and the model described by CCS, which
> seems way simpler.

I wonder if the values in these registers are just leftovers from an
earlier sensor. If the device implements a device specific gain model, it
is unlikely to support the CCS model(s).

There is also an alternative to the traditional CCS gain model, in section
9.3.2 of the spec version 1.1. The formula in that case is:

	gain = analogue_linear_gain_global *
	       2 ^ analogue_exponential_gain_global
  

Hi Jacopo

On Mon, 17 Oct 2022 at 17:32, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Sakari,
>
> On Mon, Oct 17, 2022 at 06:57:48PM +0300, Sakari Ailus wrote:
> > Hi Jacopo,
> >
> > On Mon, Oct 17, 2022 at 05:10:03PM +0200, Jacopo Mondi wrote:
> > > > which is also named analog_gain_code_global, but is documented
> > > > differently.
> > > >
> > > > Could you btw read registers 0x0000 to 0x00ff and provide the data ?
> > >
> > > There is nothing interesting there if not default values. I was hoping
> > > that analogue_gain_m0 analogue_gain_c0 and analogue_gain_m1
> > > analogue_gain_c1 would provide a way to inject gains using the
> > > standard CCS gain model, but those registers are said to be read-only
> >
> > The m[01] and c[01] factors in the CCS analogue gain formula are constants
> > that determine how the sensor's analogue gain setting translates to actual
> > analogue gain. They are not intended to be modifiable at runtime.
> >
>
> You're right sorry, indeed they're constant.
>
> For this sensor:
> analogue_gain_type: 0
> analogue_gain_m0: 1
> analogue_gain_c0: 0
> analogue_gain_m1: 0
> analogue_gain_c1: 4
>
> I should be capable of programming the global analog gain using the linear
> CCS gain model if the sensor is actually CCS compliant.
>
>         gain = m0 * x + c0 / m1 * x + c1
>              = R0x0204 / 4
>
> However, the application developer guide shows the gain to be set
> through manufacturer specific registers (0x3028 or 0x305e) and I cannot
> find much correlations between the manufacturer specific gain model
> (a piecewise exponential function) and the model described by CCS, which
> seems way simpler.

I do see a reference to register 0x0204 as analogue_gain_code_global
in the register reference (page 4), and it is listed as programmable
(7 bits). No idea if it works or not.

   Dave

> > --
> > Kind regards,
> >
> > Sakari Ailus