[PATCH 05/23] media: atomisp: gc0310: Use V4L2_CID_ANALOGUE_GAIN for gain control

[Kieran Bingham]

Hi Hans,

+Cc: libcamera-devel

Digging in here I found this part interesting (i.e. perhaps we need to
clarify the expected behavours better)



Quoting Kieran Bingham (2025-05-17 22:09:13)
> Quoting Hans de Goede (2025-05-17 12:40:48)
> > Use V4L2_CID_ANALOGUE_GAIN for gain control, as expected by userspace.
> > 
> > Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> > ---
> >  drivers/staging/media/atomisp/i2c/atomisp-gc0310.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> > index ee039f3be4da..756e56f639b7 100644
> > --- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> > +++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> > @@ -289,7 +289,7 @@ static int gc0310_s_ctrl(struct v4l2_ctrl *ctrl)
> >                 ret = cci_write(sensor->regmap, GC0310_AEC_PK_EXPO_REG,
> >                                 ctrl->val, NULL);
> >                 break;
> > -       case V4L2_CID_GAIN:
> > +       case V4L2_CID_ANALOGUE_GAIN:
> >                 ret = gc0310_gain_set(sensor, ctrl->val);
> >                 break;
> >         default:
> > @@ -533,7 +533,7 @@ static int gc0310_init_controls(struct gc0310_device *sensor)
> >  
> >         /* 32 steps at base gain 1 + 64 half steps at base gain 2 */
> 
> sounds like a curious gain model...
> 
> Will be interesting when we get the sensor calibration tools up and
> running to plot this. (Or is there already a public datasheet
> documenting this?)
> 
> Is there a split here between analogue gain and digital gain ? Or is it
> all expected to be 'analogue gain' ?

I looked deeper, and this does seem to be a split between analogue and
digital gain. It also seems like this control might be doing additional
calculations which would then have to be accounted for as part of the
gain model in libcamera, so then instead of 'sensor specific' it would
be 'this linux sensor driver specific' - so maybe the gain functions
should be simplified more.

Adding in libcamera-devel - because I think we need to figure out what's
best for handling this (overall for all sensors with A+D gain)

There are some sensors I've seen where the digital gain can only be
applied 'on top' of the analogue gain, and so it does act like a single
control ...

But we probably want to be able to distinguish between analogue gain and
digital gain in libcamera / userspace.

However, even if we distinguish ... I suspect there are cases where if
we need more gain than just the analogue gain can provide - adding the
large steps at the sensor - and then only applying very small amounts of
fine-grain digital gain on an ISP would make things simpler or easier
overall.

So somehow I think we need to figure out and correctly document and
manage the splits between analogue and digital gains, and that will
likely have to have a corresponding mapping in either the camera sensor
helpers or the tuning files in some part.

--
Kieran


> 
> 
> >         sensor->ctrls.gain =
> > -               v4l2_ctrl_new_std(hdl, &ctrl_ops, V4L2_CID_GAIN, 0, 95, 1, 31);
> > +               v4l2_ctrl_new_std(hdl, &ctrl_ops, V4L2_CID_ANALOGUE_GAIN, 0, 95, 1, 31);
> >  
> >         return hdl->error;
> >  }
> > -- 
> > 2.49.0
> >