[v3,2/2] Documentation: v4l: Exposure/gain for camera sensor

  Document the suggested way to exposure controls for exposure and gain
for camera sensor drivers.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 .../driver-api/media/camera-sensor.rst        | 27 +++++++++++++++++++
 1 file changed, 27 insertions(+)
  

Hi Jacopo,

On Mon, Jul 10, 2023 at 03:22:40PM +0200, Jacopo Mondi wrote:
> Document the suggested way to exposure controls for exposure and gain
> for camera sensor drivers.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  .../driver-api/media/camera-sensor.rst        | 27 +++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
> index cd915ca119ea..67fe77b1edb9 100644
> --- a/Documentation/driver-api/media/camera-sensor.rst
> +++ b/Documentation/driver-api/media/camera-sensor.rst
> @@ -189,3 +189,30 @@ the ``V4L2_CID_VFLIP`` and ``V4L2_CID_HFLIP`` controls with the
>  a flip can potentially change the output buffer content layout. Flips should
>  also be taken into account when enumerating and handling media bus formats
>  on the camera sensor source pads.
> +
> +Exposure and Gain Control
> +-------------------------
> +
> +Camera sensor drivers that allow applications to control the image exposure
> +and gain should do so by exposing dedicated controls to applications.
> +
> +Exposure time is controlled by registering the ``V4L2_CID_EXPOSURE`` control.
> +The control definition does not specify a unit to allow maximum flexibility
> +for multiple device types, but when used for camera sensor drivers it should be
> +expressed in unit of lines whenever possible.

This part of the documentation applies to both raw and SoC cameras.

Should the exposure unit be something more user-friendly for SoC cameras?

We have two exposure controls now, V4L2_CID_EXPOSURE and
V4L2_CID_EXPOSURE_ABSOLUTE. The former doesn't specity a unit whereas the
latter suggests the unit of 100 µs.

As exposure is specific to cameras, I think at least a part of this should
make it to the controls documentation. The UVC, for instance, uses
EXPOSURE_ABSOLUTE.

Could we document V4L2_CID_EXPOSURE is in lines (if possible)?

> +
> +To convert lines into units of time, the total line length (visible and
> +not visible pixels) has to be divided by the pixel rate::
> +
> +        line duration = total line length / pixel rate
> +                      = (image width + horizontal blanking) / pixel rate
> +
> +Camera sensor driver should try whenever possible to distinguish between the
> +analogue and digital gain control functions. Analogue gain is a multiplication
> +factor applied to all color channels on the pixel array before they get
> +converted into the digital domain. It should be made controllable by

The analogue gain may not be linear. This depends on the sensor. I'd thus
drop the wording related to multiplication factor.

> +registering the ``V4L2_CID_ANALOGUE_GAIN`` control, expressed as a device
> +specific gain code. Digital gain control is optional and should be exposed to
> +applications by registering ``V4L2_CID_DIGITAL_GAIN``. Camera sensor drivers are
> +discouraged from using ``V4L2_CID_GAIN`` as it doesn't allow differentiation of
> +analogue vs digital gain.
  

Hi Sakari

On Mon, Jul 17, 2023 at 10:24:21AM +0000, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Mon, Jul 10, 2023 at 03:22:40PM +0200, Jacopo Mondi wrote:
> > Document the suggested way to exposure controls for exposure and gain
> > for camera sensor drivers.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  .../driver-api/media/camera-sensor.rst        | 27 +++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >
> > diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
> > index cd915ca119ea..67fe77b1edb9 100644
> > --- a/Documentation/driver-api/media/camera-sensor.rst
> > +++ b/Documentation/driver-api/media/camera-sensor.rst
> > @@ -189,3 +189,30 @@ the ``V4L2_CID_VFLIP`` and ``V4L2_CID_HFLIP`` controls with the
> >  a flip can potentially change the output buffer content layout. Flips should
> >  also be taken into account when enumerating and handling media bus formats
> >  on the camera sensor source pads.
> > +
> > +Exposure and Gain Control
> > +-------------------------
> > +
> > +Camera sensor drivers that allow applications to control the image exposure
> > +and gain should do so by exposing dedicated controls to applications.
> > +
> > +Exposure time is controlled by registering the ``V4L2_CID_EXPOSURE`` control.
> > +The control definition does not specify a unit to allow maximum flexibility
> > +for multiple device types, but when used for camera sensor drivers it should be
> > +expressed in unit of lines whenever possible.
>
> This part of the documentation applies to both raw and SoC cameras.
>
> Should the exposure unit be something more user-friendly for SoC cameras?

SoC cameras == YUV/RGB sensors ?

Are you thinking about using the actual exposure time for YUV/RGB
sensors ?

>
> We have two exposure controls now, V4L2_CID_EXPOSURE and
> V4L2_CID_EXPOSURE_ABSOLUTE. The former doesn't specity a unit whereas the

Apparently only 2 drivers in mainline register V4L2_CID_EXPOSURE_ABSOLUTE

> latter suggests the unit of 100 µs.
>
> As exposure is specific to cameras, I think at least a part of this should
> make it to the controls documentation. The UVC, for instance, uses
> EXPOSURE_ABSOLUTE.
>
> Could we document V4L2_CID_EXPOSURE is in lines (if possible)?

I would indeed be happy with something like "The suggested unit for
the control is lines"

>
> > +
> > +To convert lines into units of time, the total line length (visible and
> > +not visible pixels) has to be divided by the pixel rate::
> > +
> > +        line duration = total line length / pixel rate
> > +                      = (image width + horizontal blanking) / pixel rate
> > +
> > +Camera sensor driver should try whenever possible to distinguish between the
> > +analogue and digital gain control functions. Analogue gain is a multiplication
> > +factor applied to all color channels on the pixel array before they get
> > +converted into the digital domain. It should be made controllable by
>
> The analogue gain may not be linear. This depends on the sensor. I'd thus
> drop the wording related to multiplication factor.
>

I might have missed why the gain being linear or not has implications
on the fact it acts as a multiplication factor for the color
channels...

> > +registering the ``V4L2_CID_ANALOGUE_GAIN`` control, expressed as a device
> > +specific gain code. Digital gain control is optional and should be exposed to
> > +applications by registering ``V4L2_CID_DIGITAL_GAIN``. Camera sensor drivers are
> > +discouraged from using ``V4L2_CID_GAIN`` as it doesn't allow differentiation of
> > +analogue vs digital gain.
>
> --
> Kind regards,
>
> Sakari Ailus
  

Hi Jacopo,

Just found this old e-mail I apparently forgot to reply...

On Thu, Jul 27, 2023 at 11:42:45AM +0200, Jacopo Mondi wrote:
> Hi Sakari
> 
> On Mon, Jul 17, 2023 at 10:24:21AM +0000, Sakari Ailus wrote:
> > Hi Jacopo,
> >
> > On Mon, Jul 10, 2023 at 03:22:40PM +0200, Jacopo Mondi wrote:
> > > Document the suggested way to exposure controls for exposure and gain
> > > for camera sensor drivers.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > ---
> > >  .../driver-api/media/camera-sensor.rst        | 27 +++++++++++++++++++
> > >  1 file changed, 27 insertions(+)
> > >
> > > diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
> > > index cd915ca119ea..67fe77b1edb9 100644
> > > --- a/Documentation/driver-api/media/camera-sensor.rst
> > > +++ b/Documentation/driver-api/media/camera-sensor.rst
> > > @@ -189,3 +189,30 @@ the ``V4L2_CID_VFLIP`` and ``V4L2_CID_HFLIP`` controls with the
> > >  a flip can potentially change the output buffer content layout. Flips should
> > >  also be taken into account when enumerating and handling media bus formats
> > >  on the camera sensor source pads.
> > > +
> > > +Exposure and Gain Control
> > > +-------------------------
> > > +
> > > +Camera sensor drivers that allow applications to control the image exposure
> > > +and gain should do so by exposing dedicated controls to applications.
> > > +
> > > +Exposure time is controlled by registering the ``V4L2_CID_EXPOSURE`` control.
> > > +The control definition does not specify a unit to allow maximum flexibility
> > > +for multiple device types, but when used for camera sensor drivers it should be
> > > +expressed in unit of lines whenever possible.
> >
> > This part of the documentation applies to both raw and SoC cameras.
> >
> > Should the exposure unit be something more user-friendly for SoC cameras?
> 
> SoC cameras == YUV/RGB sensors ?
> 
> Are you thinking about using the actual exposure time for YUV/RGB
> sensors ?

Some devices support both but there are devices that don't natively support
it, including UVC and Alvium.

I wonder whether we should suggest using the control method that best works
with device-native units? I.e. if the device natively uses frame length and
line length, then use blankings + the pixel clock, otherwise
[gs]_frame_interval?

> 
> >
> > We have two exposure controls now, V4L2_CID_EXPOSURE and
> > V4L2_CID_EXPOSURE_ABSOLUTE. The former doesn't specity a unit whereas the
> 
> Apparently only 2 drivers in mainline register V4L2_CID_EXPOSURE_ABSOLUTE

It's not very popular, no. :-) 100 µs is also a long time, I would expect
to have issues with that large granularity.

> 
> > latter suggests the unit of 100 µs.
> >
> > As exposure is specific to cameras, I think at least a part of this should
> > make it to the controls documentation. The UVC, for instance, uses
> > EXPOSURE_ABSOLUTE.
> >
> > Could we document V4L2_CID_EXPOSURE is in lines (if possible)?
> 
> I would indeed be happy with something like "The suggested unit for
> the control is lines"

Should there be another control for exposure in (µ)s then?

> 
> >
> > > +
> > > +To convert lines into units of time, the total line length (visible and
> > > +not visible pixels) has to be divided by the pixel rate::
> > > +
> > > +        line duration = total line length / pixel rate
> > > +                      = (image width + horizontal blanking) / pixel rate
> > > +
> > > +Camera sensor driver should try whenever possible to distinguish between the
> > > +analogue and digital gain control functions. Analogue gain is a multiplication
> > > +factor applied to all color channels on the pixel array before they get
> > > +converted into the digital domain. It should be made controllable by
> >
> > The analogue gain may not be linear. This depends on the sensor. I'd thus
> > drop the wording related to multiplication factor.
> >
> 
> I might have missed why the gain being linear or not has implications
> on the fact it acts as a multiplication factor for the color
> channels...

I must have read this as the analogue gain being the control value. Could
you still add that the analogue gain factor may have a non-linear relation
to the control value?

> 
> > > +registering the ``V4L2_CID_ANALOGUE_GAIN`` control, expressed as a device
> > > +specific gain code. Digital gain control is optional and should be exposed to
> > > +applications by registering ``V4L2_CID_DIGITAL_GAIN``. Camera sensor drivers are
> > > +discouraged from using ``V4L2_CID_GAIN`` as it doesn't allow differentiation of
> > > +analogue vs digital gain.
> >
  

Hi Sakari

On Mon, Nov 20, 2023 at 08:25:45AM +0000, Sakari Ailus wrote:
> Hi Jacopo,
>
> Just found this old e-mail I apparently forgot to reply...
>

This really fell into the cracks for me as well

> On Thu, Jul 27, 2023 at 11:42:45AM +0200, Jacopo Mondi wrote:
> > Hi Sakari
> >
> > On Mon, Jul 17, 2023 at 10:24:21AM +0000, Sakari Ailus wrote:
> > > Hi Jacopo,
> > >
> > > On Mon, Jul 10, 2023 at 03:22:40PM +0200, Jacopo Mondi wrote:
> > > > Document the suggested way to exposure controls for exposure and gain
> > > > for camera sensor drivers.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > > ---
> > > >  .../driver-api/media/camera-sensor.rst        | 27 +++++++++++++++++++
> > > >  1 file changed, 27 insertions(+)
> > > >
> > > > diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
> > > > index cd915ca119ea..67fe77b1edb9 100644
> > > > --- a/Documentation/driver-api/media/camera-sensor.rst
> > > > +++ b/Documentation/driver-api/media/camera-sensor.rst
> > > > @@ -189,3 +189,30 @@ the ``V4L2_CID_VFLIP`` and ``V4L2_CID_HFLIP`` controls with the
> > > >  a flip can potentially change the output buffer content layout. Flips should
> > > >  also be taken into account when enumerating and handling media bus formats
> > > >  on the camera sensor source pads.
> > > > +
> > > > +Exposure and Gain Control
> > > > +-------------------------
> > > > +
> > > > +Camera sensor drivers that allow applications to control the image exposure
> > > > +and gain should do so by exposing dedicated controls to applications.
> > > > +
> > > > +Exposure time is controlled by registering the ``V4L2_CID_EXPOSURE`` control.
> > > > +The control definition does not specify a unit to allow maximum flexibility
> > > > +for multiple device types, but when used for camera sensor drivers it should be
> > > > +expressed in unit of lines whenever possible.
> > >
> > > This part of the documentation applies to both raw and SoC cameras.
> > >
> > > Should the exposure unit be something more user-friendly for SoC cameras?
> >
> > SoC cameras == YUV/RGB sensors ?
> >
> > Are you thinking about using the actual exposure time for YUV/RGB
> > sensors ?
>
> Some devices support both but there are devices that don't natively support
> it, including UVC and Alvium.
>
> I wonder whether we should suggest using the control method that best works
> with device-native units? I.e. if the device natively uses frame length and
> line length, then use blankings + the pixel clock, otherwise
> [gs]_frame_interval?
>

Are we mixing two things here ? The above documentation block is about
the suggested unit for the exposure control, while [gs]_frame_interval
vs {blankings + pixel_rate} is to control the frame duration ?

> >
> > >
> > > We have two exposure controls now, V4L2_CID_EXPOSURE and
> > > V4L2_CID_EXPOSURE_ABSOLUTE. The former doesn't specity a unit whereas the
> >
> > Apparently only 2 drivers in mainline register V4L2_CID_EXPOSURE_ABSOLUTE
>
> It's not very popular, no. :-) 100 µs is also a long time, I would expect
> to have issues with that large granularity.
>

is 100 micro-seconds a too large granularity when it comes to exposure
time ??

> >
> > > latter suggests the unit of 100 µs.
> > >
> > > As exposure is specific to cameras, I think at least a part of this should
> > > make it to the controls documentation. The UVC, for instance, uses
> > > EXPOSURE_ABSOLUTE.
> > >
> > > Could we document V4L2_CID_EXPOSURE is in lines (if possible)?
> >
> > I would indeed be happy with something like "The suggested unit for
> > the control is lines"
>
> Should there be another control for exposure in (µ)s then?
>

Isn't it V4L2_CID_EXPOSURE_ABSOLUTE ?

> >
> > >
> > > > +
> > > > +To convert lines into units of time, the total line length (visible and
> > > > +not visible pixels) has to be divided by the pixel rate::
> > > > +
> > > > +        line duration = total line length / pixel rate
> > > > +                      = (image width + horizontal blanking) / pixel rate
> > > > +
> > > > +Camera sensor driver should try whenever possible to distinguish between the
> > > > +analogue and digital gain control functions. Analogue gain is a multiplication
> > > > +factor applied to all color channels on the pixel array before they get
> > > > +converted into the digital domain. It should be made controllable by
> > >
> > > The analogue gain may not be linear. This depends on the sensor. I'd thus
> > > drop the wording related to multiplication factor.
> > >
> >
> > I might have missed why the gain being linear or not has implications
> > on the fact it acts as a multiplication factor for the color
> > channels...
>
> I must have read this as the analogue gain being the control value. Could
> you still add that the analogue gain factor may have a non-linear relation
> to the control value?
>

Sure!

Thanks for digging this one out!

> >
> > > > +registering the ``V4L2_CID_ANALOGUE_GAIN`` control, expressed as a device
> > > > +specific gain code. Digital gain control is optional and should be exposed to
> > > > +applications by registering ``V4L2_CID_DIGITAL_GAIN``. Camera sensor drivers are
> > > > +discouraged from using ``V4L2_CID_GAIN`` as it doesn't allow differentiation of
> > > > +analogue vs digital gain.
> > >
>
> --
> Regards,
>
> Sakari Ailus
  

Hi Jacopo,

On Mon, Nov 20, 2023 at 05:49:30PM +0100, Jacopo Mondi wrote:
> Hi Sakari
> 
> On Mon, Nov 20, 2023 at 08:25:45AM +0000, Sakari Ailus wrote:
> > Hi Jacopo,
> >
> > Just found this old e-mail I apparently forgot to reply...
> >
> 
> This really fell into the cracks for me as well
> 
> > On Thu, Jul 27, 2023 at 11:42:45AM +0200, Jacopo Mondi wrote:
> > > Hi Sakari
> > >
> > > On Mon, Jul 17, 2023 at 10:24:21AM +0000, Sakari Ailus wrote:
> > > > Hi Jacopo,
> > > >
> > > > On Mon, Jul 10, 2023 at 03:22:40PM +0200, Jacopo Mondi wrote:
> > > > > Document the suggested way to exposure controls for exposure and gain
> > > > > for camera sensor drivers.
> > > > >
> > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > > > ---
> > > > >  .../driver-api/media/camera-sensor.rst        | 27 +++++++++++++++++++
> > > > >  1 file changed, 27 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
> > > > > index cd915ca119ea..67fe77b1edb9 100644
> > > > > --- a/Documentation/driver-api/media/camera-sensor.rst
> > > > > +++ b/Documentation/driver-api/media/camera-sensor.rst
> > > > > @@ -189,3 +189,30 @@ the ``V4L2_CID_VFLIP`` and ``V4L2_CID_HFLIP`` controls with the
> > > > >  a flip can potentially change the output buffer content layout. Flips should
> > > > >  also be taken into account when enumerating and handling media bus formats
> > > > >  on the camera sensor source pads.
> > > > > +
> > > > > +Exposure and Gain Control
> > > > > +-------------------------
> > > > > +
> > > > > +Camera sensor drivers that allow applications to control the image exposure
> > > > > +and gain should do so by exposing dedicated controls to applications.
> > > > > +
> > > > > +Exposure time is controlled by registering the ``V4L2_CID_EXPOSURE`` control.
> > > > > +The control definition does not specify a unit to allow maximum flexibility
> > > > > +for multiple device types, but when used for camera sensor drivers it should be
> > > > > +expressed in unit of lines whenever possible.
> > > >
> > > > This part of the documentation applies to both raw and SoC cameras.
> > > >
> > > > Should the exposure unit be something more user-friendly for SoC cameras?
> > >
> > > SoC cameras == YUV/RGB sensors ?
> > >
> > > Are you thinking about using the actual exposure time for YUV/RGB
> > > sensors ?
> >
> > Some devices support both but there are devices that don't natively support
> > it, including UVC and Alvium.
> >
> > I wonder whether we should suggest using the control method that best works
> > with device-native units? I.e. if the device natively uses frame length and
> > line length, then use blankings + the pixel clock, otherwise
> > [gs]_frame_interval?
> >
> 
> Are we mixing two things here ? The above documentation block is about
> the suggested unit for the exposure control, while [gs]_frame_interval
> vs {blankings + pixel_rate} is to control the frame duration ?

Oops. The context was apparently garbled in the meantime. X-)

I meant using ISO units (i.e. second) in this case.

> 
> > >
> > > >
> > > > We have two exposure controls now, V4L2_CID_EXPOSURE and
> > > > V4L2_CID_EXPOSURE_ABSOLUTE. The former doesn't specity a unit whereas the
> > >
> > > Apparently only 2 drivers in mainline register V4L2_CID_EXPOSURE_ABSOLUTE
> >
> > It's not very popular, no. :-) 100 µs is also a long time, I would expect
> > to have issues with that large granularity.
> >
> 
> is 100 micro-seconds a too large granularity when it comes to exposure
> time ??

It's a pretty long time in bright lighting conditions. The problem is not a
value as such, but the granularity: a change of one has a major relative
effect on the exposure time.

In practice this value is translated to some number of lines, and the
granularity shouldn't be worse than that. I'd therefore make this µs
instead.

> 
> > >
> > > > latter suggests the unit of 100 µs.
> > > >
> > > > As exposure is specific to cameras, I think at least a part of this should
> > > > make it to the controls documentation. The UVC, for instance, uses
> > > > EXPOSURE_ABSOLUTE.
> > > >
> > > > Could we document V4L2_CID_EXPOSURE is in lines (if possible)?
> > >
> > > I would indeed be happy with something like "The suggested unit for
> > > the control is lines"
> >
> > Should there be another control for exposure in (µ)s then?
> >
> 
> Isn't it V4L2_CID_EXPOSURE_ABSOLUTE ?

I guess we could use that but the control name makes no sense, there is
also some amount of former use. I'd create a new one instead. At this point
it shouldn't really matter for the user space.

The existing drivers will continue to also use whatever they're using now
(I guess?).

> 
> > >
> > > >
> > > > > +
> > > > > +To convert lines into units of time, the total line length (visible and
> > > > > +not visible pixels) has to be divided by the pixel rate::
> > > > > +
> > > > > +        line duration = total line length / pixel rate
> > > > > +                      = (image width + horizontal blanking) / pixel rate
> > > > > +
> > > > > +Camera sensor driver should try whenever possible to distinguish between the
> > > > > +analogue and digital gain control functions. Analogue gain is a multiplication
> > > > > +factor applied to all color channels on the pixel array before they get
> > > > > +converted into the digital domain. It should be made controllable by
> > > >
> > > > The analogue gain may not be linear. This depends on the sensor. I'd thus
> > > > drop the wording related to multiplication factor.
> > > >
> > >
> > > I might have missed why the gain being linear or not has implications
> > > on the fact it acts as a multiplication factor for the color
> > > channels...
> >
> > I must have read this as the analogue gain being the control value. Could
> > you still add that the analogue gain factor may have a non-linear relation
> > to the control value?
> >
> 
> Sure!
> 
> Thanks for digging this one out!

You're welcome! :-)
  

Hi Sakari

On Mon, Nov 20, 2023 at 06:19:46PM +0000, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Mon, Nov 20, 2023 at 05:49:30PM +0100, Jacopo Mondi wrote:
> > Hi Sakari
> >
> > On Mon, Nov 20, 2023 at 08:25:45AM +0000, Sakari Ailus wrote:
> > > Hi Jacopo,
> > >
> > > Just found this old e-mail I apparently forgot to reply...
> > >
> >
> > This really fell into the cracks for me as well
> >
> > > On Thu, Jul 27, 2023 at 11:42:45AM +0200, Jacopo Mondi wrote:
> > > > Hi Sakari
> > > >
> > > > On Mon, Jul 17, 2023 at 10:24:21AM +0000, Sakari Ailus wrote:
> > > > > Hi Jacopo,
> > > > >
> > > > > On Mon, Jul 10, 2023 at 03:22:40PM +0200, Jacopo Mondi wrote:
> > > > > > Document the suggested way to exposure controls for exposure and gain
> > > > > > for camera sensor drivers.
> > > > > >
> > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > > > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > > > > ---
> > > > > >  .../driver-api/media/camera-sensor.rst        | 27 +++++++++++++++++++
> > > > > >  1 file changed, 27 insertions(+)
> > > > > >
> > > > > > diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
> > > > > > index cd915ca119ea..67fe77b1edb9 100644
> > > > > > --- a/Documentation/driver-api/media/camera-sensor.rst
> > > > > > +++ b/Documentation/driver-api/media/camera-sensor.rst
> > > > > > @@ -189,3 +189,30 @@ the ``V4L2_CID_VFLIP`` and ``V4L2_CID_HFLIP`` controls with the
> > > > > >  a flip can potentially change the output buffer content layout. Flips should
> > > > > >  also be taken into account when enumerating and handling media bus formats
> > > > > >  on the camera sensor source pads.
> > > > > > +
> > > > > > +Exposure and Gain Control
> > > > > > +-------------------------
> > > > > > +
> > > > > > +Camera sensor drivers that allow applications to control the image exposure
> > > > > > +and gain should do so by exposing dedicated controls to applications.
> > > > > > +
> > > > > > +Exposure time is controlled by registering the ``V4L2_CID_EXPOSURE`` control.
> > > > > > +The control definition does not specify a unit to allow maximum flexibility
> > > > > > +for multiple device types, but when used for camera sensor drivers it should be
> > > > > > +expressed in unit of lines whenever possible.
> > > > >
> > > > > This part of the documentation applies to both raw and SoC cameras.
> > > > >
> > > > > Should the exposure unit be something more user-friendly for SoC cameras?
> > > >
> > > > SoC cameras == YUV/RGB sensors ?
> > > >
> > > > Are you thinking about using the actual exposure time for YUV/RGB
> > > > sensors ?
> > >
> > > Some devices support both but there are devices that don't natively support
> > > it, including UVC and Alvium.
> > >
> > > I wonder whether we should suggest using the control method that best works
> > > with device-native units? I.e. if the device natively uses frame length and
> > > line length, then use blankings + the pixel clock, otherwise
> > > [gs]_frame_interval?
> > >
> >
> > Are we mixing two things here ? The above documentation block is about
> > the suggested unit for the exposure control, while [gs]_frame_interval
> > vs {blankings + pixel_rate} is to control the frame duration ?
>
> Oops. The context was apparently garbled in the meantime. X-)
>
> I meant using ISO units (i.e. second) in this case.
>
> >
> > > >
> > > > >
> > > > > We have two exposure controls now, V4L2_CID_EXPOSURE and
> > > > > V4L2_CID_EXPOSURE_ABSOLUTE. The former doesn't specity a unit whereas the
> > > >
> > > > Apparently only 2 drivers in mainline register V4L2_CID_EXPOSURE_ABSOLUTE
> > >
> > > It's not very popular, no. :-) 100 µs is also a long time, I would expect
> > > to have issues with that large granularity.
> > >
> >
> > is 100 micro-seconds a too large granularity when it comes to exposure
> > time ??
>
> It's a pretty long time in bright lighting conditions. The problem is not a
> value as such, but the granularity: a change of one has a major relative
> effect on the exposure time.
>
> In practice this value is translated to some number of lines, and the
> granularity shouldn't be worse than that. I'd therefore make this µs
> instead.
>

Yeah, you're right, the two controls' granularity should be similar.

I actually wonder if that's enough. C-PHY has a total bandwidth of 5.8Gbps
if I'm not mistaken, and assuming 12bpp and a line lenght of 4000
pixels (arbitrary pick) the line time is 8usec. I wonder if we
shouldn't go for nanoseconds and be done with that.

> >
> > > >
> > > > > latter suggests the unit of 100 µs.
> > > > >
> > > > > As exposure is specific to cameras, I think at least a part of this should
> > > > > make it to the controls documentation. The UVC, for instance, uses
> > > > > EXPOSURE_ABSOLUTE.
> > > > >
> > > > > Could we document V4L2_CID_EXPOSURE is in lines (if possible)?
> > > >
> > > > I would indeed be happy with something like "The suggested unit for
> > > > the control is lines"
> > >
> > > Should there be another control for exposure in (µ)s then?
> > >
> >
> > Isn't it V4L2_CID_EXPOSURE_ABSOLUTE ?
>
> I guess we could use that but the control name makes no sense, there is
> also some amount of former use. I'd create a new one instead. At this point
> it shouldn't really matter for the user space.
>
> The existing drivers will continue to also use whatever they're using now
> (I guess?).
>

if we introduce a new control V4L2_CID_EXPOSURE_USEC (name to be

   V4L2_CID_EXPOSURE in lines
   V4L2_CID_EXPOSURE_ABSOLUTE in 100usec
   V4L2_CID_EXPOSURE_USEC in 1usec/nanosecs

isn't it very confusing ?

Ideally there should have been V4L2_CID_EXPOSURE_LINES and
V4L2_CID_EXPOSURE_USEC from the start, but how to get there without
breaking existing users or duplicating controls is not easy...

> >
> > > >
> > > > >
> > > > > > +
> > > > > > +To convert lines into units of time, the total line length (visible and
> > > > > > +not visible pixels) has to be divided by the pixel rate::
> > > > > > +
> > > > > > +        line duration = total line length / pixel rate
> > > > > > +                      = (image width + horizontal blanking) / pixel rate
> > > > > > +
> > > > > > +Camera sensor driver should try whenever possible to distinguish between the
> > > > > > +analogue and digital gain control functions. Analogue gain is a multiplication
> > > > > > +factor applied to all color channels on the pixel array before they get
> > > > > > +converted into the digital domain. It should be made controllable by
> > > > >
> > > > > The analogue gain may not be linear. This depends on the sensor. I'd thus
> > > > > drop the wording related to multiplication factor.
> > > > >
> > > >
> > > > I might have missed why the gain being linear or not has implications
> > > > on the fact it acts as a multiplication factor for the color
> > > > channels...
> > >
> > > I must have read this as the analogue gain being the control value. Could
> > > you still add that the analogue gain factor may have a non-linear relation
> > > to the control value?
> > >
> >
> > Sure!
> >
> > Thanks for digging this one out!
>
> You're welcome! :-)
>
> --
> Regards,
>
> Sakari Ailus
  

Hi Jacopo,

On Mon, Nov 20, 2023 at 07:47:11PM +0100, Jacopo Mondi wrote:
> Hi Sakari
> 
> On Mon, Nov 20, 2023 at 06:19:46PM +0000, Sakari Ailus wrote:
> > Hi Jacopo,
> >
> > On Mon, Nov 20, 2023 at 05:49:30PM +0100, Jacopo Mondi wrote:
> > > Hi Sakari
> > >
> > > On Mon, Nov 20, 2023 at 08:25:45AM +0000, Sakari Ailus wrote:
> > > > Hi Jacopo,
> > > >
> > > > Just found this old e-mail I apparently forgot to reply...
> > > >
> > >
> > > This really fell into the cracks for me as well
> > >
> > > > On Thu, Jul 27, 2023 at 11:42:45AM +0200, Jacopo Mondi wrote:
> > > > > Hi Sakari
> > > > >
> > > > > On Mon, Jul 17, 2023 at 10:24:21AM +0000, Sakari Ailus wrote:
> > > > > > Hi Jacopo,
> > > > > >
> > > > > > On Mon, Jul 10, 2023 at 03:22:40PM +0200, Jacopo Mondi wrote:
> > > > > > > Document the suggested way to exposure controls for exposure and gain
> > > > > > > for camera sensor drivers.
> > > > > > >
> > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > > > > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > > > > > ---
> > > > > > >  .../driver-api/media/camera-sensor.rst        | 27 +++++++++++++++++++
> > > > > > >  1 file changed, 27 insertions(+)
> > > > > > >
> > > > > > > diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
> > > > > > > index cd915ca119ea..67fe77b1edb9 100644
> > > > > > > --- a/Documentation/driver-api/media/camera-sensor.rst
> > > > > > > +++ b/Documentation/driver-api/media/camera-sensor.rst
> > > > > > > @@ -189,3 +189,30 @@ the ``V4L2_CID_VFLIP`` and ``V4L2_CID_HFLIP`` controls with the
> > > > > > >  a flip can potentially change the output buffer content layout. Flips should
> > > > > > >  also be taken into account when enumerating and handling media bus formats
> > > > > > >  on the camera sensor source pads.
> > > > > > > +
> > > > > > > +Exposure and Gain Control
> > > > > > > +-------------------------
> > > > > > > +
> > > > > > > +Camera sensor drivers that allow applications to control the image exposure
> > > > > > > +and gain should do so by exposing dedicated controls to applications.
> > > > > > > +
> > > > > > > +Exposure time is controlled by registering the ``V4L2_CID_EXPOSURE`` control.
> > > > > > > +The control definition does not specify a unit to allow maximum flexibility
> > > > > > > +for multiple device types, but when used for camera sensor drivers it should be
> > > > > > > +expressed in unit of lines whenever possible.
> > > > > >
> > > > > > This part of the documentation applies to both raw and SoC cameras.
> > > > > >
> > > > > > Should the exposure unit be something more user-friendly for SoC cameras?
> > > > >
> > > > > SoC cameras == YUV/RGB sensors ?
> > > > >
> > > > > Are you thinking about using the actual exposure time for YUV/RGB
> > > > > sensors ?
> > > >
> > > > Some devices support both but there are devices that don't natively support
> > > > it, including UVC and Alvium.
> > > >
> > > > I wonder whether we should suggest using the control method that best works
> > > > with device-native units? I.e. if the device natively uses frame length and
> > > > line length, then use blankings + the pixel clock, otherwise
> > > > [gs]_frame_interval?
> > > >
> > >
> > > Are we mixing two things here ? The above documentation block is about
> > > the suggested unit for the exposure control, while [gs]_frame_interval
> > > vs {blankings + pixel_rate} is to control the frame duration ?
> >
> > Oops. The context was apparently garbled in the meantime. X-)
> >
> > I meant using ISO units (i.e. second) in this case.
> >
> > >
> > > > >
> > > > > >
> > > > > > We have two exposure controls now, V4L2_CID_EXPOSURE and
> > > > > > V4L2_CID_EXPOSURE_ABSOLUTE. The former doesn't specity a unit whereas the
> > > > >
> > > > > Apparently only 2 drivers in mainline register V4L2_CID_EXPOSURE_ABSOLUTE
> > > >
> > > > It's not very popular, no. :-) 100 µs is also a long time, I would expect
> > > > to have issues with that large granularity.
> > > >
> > >
> > > is 100 micro-seconds a too large granularity when it comes to exposure
> > > time ??
> >
> > It's a pretty long time in bright lighting conditions. The problem is not a
> > value as such, but the granularity: a change of one has a major relative
> > effect on the exposure time.
> >
> > In practice this value is translated to some number of lines, and the
> > granularity shouldn't be worse than that. I'd therefore make this µs
> > instead.
> >
> 
> Yeah, you're right, the two controls' granularity should be similar.
> 
> I actually wonder if that's enough. C-PHY has a total bandwidth of 5.8Gbps
> if I'm not mistaken, and assuming 12bpp and a line lenght of 4000
> pixels (arbitrary pick) the line time is 8usec. I wonder if we
> shouldn't go for nanoseconds and be done with that.

That limits the maximum time to around 2 s, but I guess one could use
64-bit controls, too. But how about 10 ns?

> 
> > >
> > > > >
> > > > > > latter suggests the unit of 100 µs.
> > > > > >
> > > > > > As exposure is specific to cameras, I think at least a part of this should
> > > > > > make it to the controls documentation. The UVC, for instance, uses
> > > > > > EXPOSURE_ABSOLUTE.
> > > > > >
> > > > > > Could we document V4L2_CID_EXPOSURE is in lines (if possible)?
> > > > >
> > > > > I would indeed be happy with something like "The suggested unit for
> > > > > the control is lines"
> > > >
> > > > Should there be another control for exposure in (µ)s then?
> > > >
> > >
> > > Isn't it V4L2_CID_EXPOSURE_ABSOLUTE ?
> >
> > I guess we could use that but the control name makes no sense, there is
> > also some amount of former use. I'd create a new one instead. At this point
> > it shouldn't really matter for the user space.
> >
> > The existing drivers will continue to also use whatever they're using now
> > (I guess?).
> >
> 
> if we introduce a new control V4L2_CID_EXPOSURE_USEC (name to be
> 
>    V4L2_CID_EXPOSURE in lines
>    V4L2_CID_EXPOSURE_ABSOLUTE in 100usec
>    V4L2_CID_EXPOSURE_USEC in 1usec/nanosecs
> 
> isn't it very confusing ?
> 
> Ideally there should have been V4L2_CID_EXPOSURE_LINES and
> V4L2_CID_EXPOSURE_USEC from the start, but how to get there without
> breaking existing users or duplicating controls is not easy...

I wonder what Laurent and Hans think about this.

I'll form my opinion tomorrow. :-)

> 
> > >
> > > > >
> > > > > >
> > > > > > > +
> > > > > > > +To convert lines into units of time, the total line length (visible and
> > > > > > > +not visible pixels) has to be divided by the pixel rate::
> > > > > > > +
> > > > > > > +        line duration = total line length / pixel rate
> > > > > > > +                      = (image width + horizontal blanking) / pixel rate
> > > > > > > +
> > > > > > > +Camera sensor driver should try whenever possible to distinguish between the
> > > > > > > +analogue and digital gain control functions. Analogue gain is a multiplication
> > > > > > > +factor applied to all color channels on the pixel array before they get
> > > > > > > +converted into the digital domain. It should be made controllable by
> > > > > >
> > > > > > The analogue gain may not be linear. This depends on the sensor. I'd thus
> > > > > > drop the wording related to multiplication factor.
> > > > > >
> > > > >
> > > > > I might have missed why the gain being linear or not has implications
> > > > > on the fact it acts as a multiplication factor for the color
> > > > > channels...
> > > >
> > > > I must have read this as the analogue gain being the control value. Could
> > > > you still add that the analogue gain factor may have a non-linear relation
> > > > to the control value?
> > > >
> > >
> > > Sure!
> > >
> > > Thanks for digging this one out!
> >
> > You're welcome! :-)
> >