diff mbox series

[02/10] media: ar0521: Add V4L2_CID_ANALOG_GAIN

Message ID 20221005190613.394277-3-jacopo@jmondi.org
State New
Headers show
Series media: ar0521: Add analog gain, rework clock tree | expand

Commit Message

Jacopo Mondi Oct. 5, 2022, 7:06 p.m. UTC
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(+)

Comments

Laurent Pinchart Oct. 13, 2022, 9:30 a.m. UTC | #1
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.
Jacopo Mondi Oct. 17, 2022, 3:10 p.m. UTC | #2
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
Sakari Ailus Oct. 17, 2022, 3:57 p.m. UTC | #3
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.
Sakari Ailus Oct. 17, 2022, 4:37 p.m. UTC | #4
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
diff mbox series

Patch

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);
+}
+
 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,