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