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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Sun, May 18, 2025 at 10:42:56AM +0100, Kieran Bingham wrote:
> 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@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 ...

I've seen other sensors (in particular the AR0830) that also combine
analogue and digital gains in a single register. The analogue gain is
typically quite coarse in that case, and the digital gain is used for
fine adjustments. In the ar0830 driver I'm writing, I've split the two
components in two separate controls.

> 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.
> 
> > >         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;
> > >  }

-- 
Regards,

Laurent Pinchart