[PATCH 0/2] Documentation: v4l: more camera sensor doc

[PATCH 0/2] Documentation: v4l: more camera sensor doc

[Jacopo Mondi]

Two small addictions to camera sensor documentation to reflect what has
been discussed during my presentation at EOSS last week.

Thanks
   j

Jacopo Mondi (2):
  Documentation: v4l: Flip handling for RAW sensors
  Documentation: v4l: Exposure/gain for camera sensor

 .../driver-api/media/camera-sensor.rst        | 35 +++++++++++++++++++
 1 file changed, 35 insertions(+)

--
2.40.1

[PATCH 1/2] Documentation: v4l: Flip handling for RAW sensors

[Jacopo Mondi]

Document the requirement of notifying to userspace the possible
re-ordering of the color sample components when a vertical or horizontal
flip is applied to a RAW camera sensor.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 Documentation/driver-api/media/camera-sensor.rst | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
index 93f4f2536c25..ee4a7fe5f72a 100644
--- a/Documentation/driver-api/media/camera-sensor.rst
+++ b/Documentation/driver-api/media/camera-sensor.rst
@@ -173,3 +173,19 @@ V4L2_CID_VFLIP controls with the values programmed by the register sequences.
 The default values of these controls shall be 0 (disabled). Especially these
 controls shall not be inverted, independently of the sensor's mounting
 rotation.
+
+Flip handling for raw camera sensors
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Applying vertical and horizontal flips on raw camera sensors inverts the color
+sample reading direction on the sensor's pixel array. This causes the
+re-ordering of the color samples on the sensor's output frame. As an example, a
+raw camera sensor with a Bayer pattern color filter array and a native RGGB
+Bayer order will produce frames with GRBG component ordering when a vertical
+flip is applied. Camera sensor drivers where inverting the reading order
+direction causes a re-ordering of the color components are requested to register
+the ``V4L2_CID_VFLIP`` and ``V4L2_CID_HFLIP`` controls with the
+``V4L2_CTRL_FLAG_MODIFY_LAYOUT`` flag enabled to notify userspace that enabling
+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.
-- 
2.40.1

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

[Jacopo Mondi]

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

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 .../driver-api/media/camera-sensor.rst        | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
index ee4a7fe5f72a..dfe8f35aecea 100644
--- a/Documentation/driver-api/media/camera-sensor.rst
+++ b/Documentation/driver-api/media/camera-sensor.rst
@@ -189,3 +189,22 @@ 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 allows 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 driver it should be
+expressed in unit of lines whenever possible.
+
+Camera sensor driver should try whenever possible to distinguish between the
+analogue and digital gain control functions. Analogue gain is a multiplier
+factor applied to all color channels on the pixel array before they get
+converted in the digital domain. It should be be made controllable by
+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
-- 
2.40.1

Re: [PATCH 1/2] Documentation: v4l: Flip handling for RAW sensors

[Dave Stevenson]

Hi Jacopo

Thanks for adding documentation.
Sorry I couldn't be at your presentation, but I'll find the slides
(and recording if there is one).

On Mon, 3 Jul 2023 at 21:29, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
>
> Document the requirement of notifying to userspace the possible
> re-ordering of the color sample components when a vertical or horizontal
> flip is applied to a RAW camera sensor.
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  Documentation/driver-api/media/camera-sensor.rst | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
> index 93f4f2536c25..ee4a7fe5f72a 100644
> --- a/Documentation/driver-api/media/camera-sensor.rst
> +++ b/Documentation/driver-api/media/camera-sensor.rst
> @@ -173,3 +173,19 @@ V4L2_CID_VFLIP controls with the values programmed by the register sequences.
>  The default values of these controls shall be 0 (disabled). Especially these
>  controls shall not be inverted, independently of the sensor's mounting
>  rotation.
> +
> +Flip handling for raw camera sensors
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Possibly "for colour raw camera sensors".
Mono sensors are still raw in that they need processing for black
level, lens shading, etc, but they won't change the colour ordering.

> +
> +Applying vertical and horizontal flips on raw camera sensors inverts the color
> +sample reading direction on the sensor's pixel array. This causes the
> +re-ordering of the color samples on the sensor's output frame.

This *may* cause the re-ordering....

Not all sensors do. Some shift the readout by one line/column to keep
the Bayer order the same, and technically should update the selection.
OnSemi sensors in particular seem to do this, as do the Sony
IMX327/290/462 family.

> As an example, a
> +raw camera sensor with a Bayer pattern color filter array and a native RGGB
> +Bayer order will produce frames with GRBG component ordering when a vertical
> +flip is applied.

Vertical flip of RGGB would be GBRG as the RG and GB get swapped, not
GRBG (which would be horizontal flip).

  Dave

> Camera sensor drivers where inverting the reading order
> +direction causes a re-ordering of the color components are requested to register
> +the ``V4L2_CID_VFLIP`` and ``V4L2_CID_HFLIP`` controls with the
> +``V4L2_CTRL_FLAG_MODIFY_LAYOUT`` flag enabled to notify userspace that enabling
> +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.
> --
> 2.40.1
>

Re: [PATCH 2/2] Documentation: v4l: Exposure/gain for camera sensor

[Dave Stevenson]

Hi Jacopo

On Mon, 3 Jul 2023 at 21:29, Jacopo Mondi <jacopo.mondi@ideasonboard.com> 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>
> ---
>  .../driver-api/media/camera-sensor.rst        | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
> index ee4a7fe5f72a..dfe8f35aecea 100644
> --- a/Documentation/driver-api/media/camera-sensor.rst
> +++ b/Documentation/driver-api/media/camera-sensor.rst
> @@ -189,3 +189,22 @@ 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 allows applications to control the image exposure

s/allows/allow

> +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 driver it should be

s/driver/drivers

> +expressed in unit of lines whenever possible.

s/unit/units

Possibly clarify that lines can be converted into units of time by
using V4L2_CID_PIXEL_RATE and (image width + V4L2_CID_HBLANK).

> +
> +Camera sensor driver should try whenever possible to distinguish between the
> +analogue and digital gain control functions. Analogue gain is a multiplier
> +factor applied to all color channels on the pixel array before they get
> +converted in the digital domain. It should be be made controllable by
> +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

Something appears to be truncated here.

  Dave

> --
> 2.40.1
>

Re: [PATCH 1/2] Documentation: v4l: Flip handling for RAW sensors

[Jacopo Mondi]

Hi Dave,
   thanks for the comments

On Tue, Jul 04, 2023 at 10:58:11AM +0100, Dave Stevenson wrote:
> Hi Jacopo
>
> Thanks for adding documentation.
> Sorry I couldn't be at your presentation, but I'll find the slides
> (and recording if there is one).
>

videos and slides should be already available for attendees. If not I
can send you the slide deck, but trust me, there is nothing new for
you there.

> On Mon, 3 Jul 2023 at 21:29, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
> >
> > Document the requirement of notifying to userspace the possible
> > re-ordering of the color sample components when a vertical or horizontal
> > flip is applied to a RAW camera sensor.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  Documentation/driver-api/media/camera-sensor.rst | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
> > index 93f4f2536c25..ee4a7fe5f72a 100644
> > --- a/Documentation/driver-api/media/camera-sensor.rst
> > +++ b/Documentation/driver-api/media/camera-sensor.rst
> > @@ -173,3 +173,19 @@ V4L2_CID_VFLIP controls with the values programmed by the register sequences.
> >  The default values of these controls shall be 0 (disabled). Especially these
> >  controls shall not be inverted, independently of the sensor's mounting
> >  rotation.
> > +
> > +Flip handling for raw camera sensors
> > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Possibly "for colour raw camera sensors".
> Mono sensors are still raw in that they need processing for black
> level, lens shading, etc, but they won't change the colour ordering.
>

Thanks, good point

> > +
> > +Applying vertical and horizontal flips on raw camera sensors inverts the color
> > +sample reading direction on the sensor's pixel array. This causes the
> > +re-ordering of the color samples on the sensor's output frame.
>
> This *may* cause the re-ordering....
>
> Not all sensors do. Some shift the readout by one line/column to keep
> the Bayer order the same, and technically should update the selection.
> OnSemi sensors in particular seem to do this, as do the Sony
> IMX327/290/462 family.
>

Is it the driver doing the shift or is it the sensor automatically
adjusting ?

> > As an example, a
> > +raw camera sensor with a Bayer pattern color filter array and a native RGGB
> > +Bayer order will produce frames with GRBG component ordering when a vertical
> > +flip is applied.
>
> Vertical flip of RGGB would be GBRG as the RG and GB get swapped, not
> GRBG (which would be horizontal flip).

I clearly mean horizontal sorry. I'll fix.

Thanks
   j

>
>   Dave
>
> > Camera sensor drivers where inverting the reading order
> > +direction causes a re-ordering of the color components are requested to register
> > +the ``V4L2_CID_VFLIP`` and ``V4L2_CID_HFLIP`` controls with the
> > +``V4L2_CTRL_FLAG_MODIFY_LAYOUT`` flag enabled to notify userspace that enabling
> > +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.
> > --
> > 2.40.1
> >

Re: [PATCH 2/2] Documentation: v4l: Exposure/gain for camera sensor

[Jacopo Mondi]

Hi Dave

On Tue, Jul 04, 2023 at 11:05:42AM +0100, Dave Stevenson wrote:
> Hi Jacopo
>
> On Mon, 3 Jul 2023 at 21:29, Jacopo Mondi <jacopo.mondi@ideasonboard.com> 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>
> > ---
> >  .../driver-api/media/camera-sensor.rst        | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
> > index ee4a7fe5f72a..dfe8f35aecea 100644
> > --- a/Documentation/driver-api/media/camera-sensor.rst
> > +++ b/Documentation/driver-api/media/camera-sensor.rst
> > @@ -189,3 +189,22 @@ 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 allows applications to control the image exposure
>
> s/allows/allow
>
> > +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 driver it should be
>
> s/driver/drivers
>
> > +expressed in unit of lines whenever possible.
>
> s/unit/units
>
> Possibly clarify that lines can be converted into units of time by
> using V4L2_CID_PIXEL_RATE and (image width + V4L2_CID_HBLANK).
>

I think this might be useful yes. A few paragraph above the frame
duration calculation formula is expressed as well, so I guess adding
one for this is helpful too

> > +
> > +Camera sensor driver should try whenever possible to distinguish between the
> > +analogue and digital gain control functions. Analogue gain is a multiplier
> > +factor applied to all color channels on the pixel array before they get
> > +converted in the digital domain. It should be be made controllable by
> > +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
>
> Something appears to be truncated here.

Clearly a bad rebase. What I meant (I lost the original version) was

Camera sensor drivers are discouraged from using ``V4L2_CID_GAIN``
which doesn't allow to control the analogue and digital gain
components individually.

How does this sound ?

>
>   Dave
>
> > --
> > 2.40.1
> >

Re: [PATCH 1/2] Documentation: v4l: Flip handling for RAW sensors

[Dave Stevenson]

On Tue, 4 Jul 2023 at 11:24, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Dave,
>    thanks for the comments
>
> On Tue, Jul 04, 2023 at 10:58:11AM +0100, Dave Stevenson wrote:
> > Hi Jacopo
> >
> > Thanks for adding documentation.
> > Sorry I couldn't be at your presentation, but I'll find the slides
> > (and recording if there is one).
> >
>
> videos and slides should be already available for attendees. If not I
> can send you the slide deck, but trust me, there is nothing new for
> you there.
>
> > On Mon, 3 Jul 2023 at 21:29, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
> > >
> > > Document the requirement of notifying to userspace the possible
> > > re-ordering of the color sample components when a vertical or horizontal
> > > flip is applied to a RAW camera sensor.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > ---
> > >  Documentation/driver-api/media/camera-sensor.rst | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > >
> > > diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
> > > index 93f4f2536c25..ee4a7fe5f72a 100644
> > > --- a/Documentation/driver-api/media/camera-sensor.rst
> > > +++ b/Documentation/driver-api/media/camera-sensor.rst
> > > @@ -173,3 +173,19 @@ V4L2_CID_VFLIP controls with the values programmed by the register sequences.
> > >  The default values of these controls shall be 0 (disabled). Especially these
> > >  controls shall not be inverted, independently of the sensor's mounting
> > >  rotation.
> > > +
> > > +Flip handling for raw camera sensors
> > > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Possibly "for colour raw camera sensors".
> > Mono sensors are still raw in that they need processing for black
> > level, lens shading, etc, but they won't change the colour ordering.
> >
>
> Thanks, good point
>
> > > +
> > > +Applying vertical and horizontal flips on raw camera sensors inverts the color
> > > +sample reading direction on the sensor's pixel array. This causes the
> > > +re-ordering of the color samples on the sensor's output frame.
> >
> > This *may* cause the re-ordering....
> >
> > Not all sensors do. Some shift the readout by one line/column to keep
> > the Bayer order the same, and technically should update the selection.
> > OnSemi sensors in particular seem to do this, as do the Sony
> > IMX327/290/462 family.
> >
>
> Is it the driver doing the shift or is it the sensor automatically
> adjusting ?

The sensor does it.

For IMX290, the array is defined horizontally as:
4 ignored pixels
8 pixels margin for colour processing
1920 recording pixel area
9 pixels margin for colour processing
4 ignored pixels
(3 dummy pixels)
Ignoring the dummy ones, that gives a width of 1945 pixels.

Vertically is similar with 1109 pixels total (9 colour margin / 1080
active / 8 colour margin).

That means it is a red pixel in all 4 corners of the full 1945x1109
array, and it keeps the 8 pixels margin on the left of the image read
out in all circumstances, and 8 at the top too so that readout always
follows that.
It's slightly weird as a sensor in that it has presets for 1080p and
720p, as well as a windowed mode where you can arbitrarily crop from
the whole array. The driver only uses the two presets, so exactly what
is going on is partly reverse engineering.

With regard OnSemi, we asked them about it several years ago, and the
response was that there was no way to change that behaviour for the
sensor we were looking at.
From that datasheet:
"While the sensor’s format is W × H, the additional active columns and
active rows are included for use when horizontal or vertical mirrored
readout is enabled, to allow readout to start on the same pixel. The
pixel adjustment is always performed for monochrome or color versions"
Definition of the VERT_FLIP bit
"1: Readout is flipped (mirrored) vertically so that the row specified
by y_addr_end_ (+1) is read out of the sensor first"
and HORIZ_MIRROR bit
"1: Readout is mirrored horizontally so that the column specified by
x_addr_end_ (+1) is read out of the sensor first."

Having just checked a different OnSemi datasheet, that doesn't state
that it alters the pixel read out first when flipped. It possibly
depends on the target market for the sensor, and how configurable they
anticipate the ISPs to be.

  Dave

> > > As an example, a
> > > +raw camera sensor with a Bayer pattern color filter array and a native RGGB
> > > +Bayer order will produce frames with GRBG component ordering when a vertical
> > > +flip is applied.
> >
> > Vertical flip of RGGB would be GBRG as the RG and GB get swapped, not
> > GRBG (which would be horizontal flip).
>
> I clearly mean horizontal sorry. I'll fix.
>
> Thanks
>    j
>
> >
> >   Dave
> >
> > > Camera sensor drivers where inverting the reading order
> > > +direction causes a re-ordering of the color components are requested to register
> > > +the ``V4L2_CID_VFLIP`` and ``V4L2_CID_HFLIP`` controls with the
> > > +``V4L2_CTRL_FLAG_MODIFY_LAYOUT`` flag enabled to notify userspace that enabling
> > > +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.
> > > --
> > > 2.40.1
> > >

Re: [PATCH 2/2] Documentation: v4l: Exposure/gain for camera sensor

[Dave Stevenson]

On Tue, 4 Jul 2023 at 11:28, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Dave
>
> On Tue, Jul 04, 2023 at 11:05:42AM +0100, Dave Stevenson wrote:
> > Hi Jacopo
> >
> > On Mon, 3 Jul 2023 at 21:29, Jacopo Mondi <jacopo.mondi@ideasonboard.com> 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>
> > > ---
> > >  .../driver-api/media/camera-sensor.rst        | 19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > >
> > > diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
> > > index ee4a7fe5f72a..dfe8f35aecea 100644
> > > --- a/Documentation/driver-api/media/camera-sensor.rst
> > > +++ b/Documentation/driver-api/media/camera-sensor.rst
> > > @@ -189,3 +189,22 @@ 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 allows applications to control the image exposure
> >
> > s/allows/allow
> >
> > > +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 driver it should be
> >
> > s/driver/drivers
> >
> > > +expressed in unit of lines whenever possible.
> >
> > s/unit/units
> >
> > Possibly clarify that lines can be converted into units of time by
> > using V4L2_CID_PIXEL_RATE and (image width + V4L2_CID_HBLANK).
> >
>
> I think this might be useful yes. A few paragraph above the frame
> duration calculation formula is expressed as well, so I guess adding
> one for this is helpful too

Possibly just reference that paragraph then for computing the line period then.

> > > +
> > > +Camera sensor driver should try whenever possible to distinguish between the
> > > +analogue and digital gain control functions. Analogue gain is a multiplier
> > > +factor applied to all color channels on the pixel array before they get
> > > +converted in the digital domain. It should be be made controllable by
> > > +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
> >
> > Something appears to be truncated here.
>
> Clearly a bad rebase. What I meant (I lost the original version) was
>
> Camera sensor drivers are discouraged from using ``V4L2_CID_GAIN``
> which doesn't allow to control the analogue and digital gain

as it doesn't allow differentiation of analogue vs digital gain

> components individually.
>
> How does this sound ?

Otherwise it sounds fine.

  Dave

Re: [PATCH 2/2] Documentation: v4l: Exposure/gain for camera sensor

[Tommaso Merciai]

Hi Jacopo,

On Mon, Jul 03, 2023 at 10:29:10PM +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>
> ---
>  .../driver-api/media/camera-sensor.rst        | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
> index ee4a7fe5f72a..dfe8f35aecea 100644
> --- a/Documentation/driver-api/media/camera-sensor.rst
> +++ b/Documentation/driver-api/media/camera-sensor.rst
> @@ -189,3 +189,22 @@ 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 allows 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 driver it should be

> +expressed in unit of lines whenever possible.

Same comment here. 

Can you add formula/references about this point I think you are referring on "tline" units (maybe I'm completely wrong :) ),
but to be honest checking also the some sensors datasheet I don't find to much infos about this units.
Would be really helpfull to add some details on this point.

> +
> +Camera sensor driver should try whenever possible to distinguish between the
> +analogue and digital gain control functions. Analogue gain is a multiplier
> +factor applied to all color channels on the pixel array before they get
> +converted in the digital domain. It should be be made controllable by
> +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
> -- 

This part looks good to me.

Thanks,
Tommaso

> 2.40.1
> 

Re: [PATCH 2/2] Documentation: v4l: Exposure/gain for camera sensor

[Jacopo Mondi]

Hi Tommaso

On Tue, Jul 04, 2023 at 03:51:43PM +0200, Tommaso Merciai wrote:
> Hi Jacopo,
>
> On Mon, Jul 03, 2023 at 10:29:10PM +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>
> > ---
> >  .../driver-api/media/camera-sensor.rst        | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
> > index ee4a7fe5f72a..dfe8f35aecea 100644
> > --- a/Documentation/driver-api/media/camera-sensor.rst
> > +++ b/Documentation/driver-api/media/camera-sensor.rst
> > @@ -189,3 +189,22 @@ 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 allows 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 driver it should be
>
> > +expressed in unit of lines whenever possible.
>
> Same comment here.

I might have missed what other comment you are referring to :)

>
> Can you add formula/references about this point I think you are referring on "tline" units (maybe I'm completely wrong :) ),

Is "tline" the line duration ? If that's the case then no, I am
referring to the number of lines, not their duration.

> but to be honest checking also the some sensors datasheet I don't find to much infos about this units.
> Would be really helpfull to add some details on this point.
>
> > +
> > +Camera sensor driver should try whenever possible to distinguish between the
> > +analogue and digital gain control functions. Analogue gain is a multiplier
> > +factor applied to all color channels on the pixel array before they get
> > +converted in the digital domain. It should be be made controllable by
> > +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
> > --
>
> This part looks good to me.
>
> Thanks,
> Tommaso
>
> > 2.40.1
> >

Re: [PATCH 2/2] Documentation: v4l: Exposure/gain for camera sensor

[Tommaso Merciai]

Hi Jacopo,

On Tue, Jul 04, 2023 at 04:05:44PM +0200, Jacopo Mondi wrote:
> Hi Tommaso
> 
> On Tue, Jul 04, 2023 at 03:51:43PM +0200, Tommaso Merciai wrote:
> > Hi Jacopo,
> >
> > On Mon, Jul 03, 2023 at 10:29:10PM +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>
> > > ---
> > >  .../driver-api/media/camera-sensor.rst        | 19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > >
> > > diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
> > > index ee4a7fe5f72a..dfe8f35aecea 100644
> > > --- a/Documentation/driver-api/media/camera-sensor.rst
> > > +++ b/Documentation/driver-api/media/camera-sensor.rst
> > > @@ -189,3 +189,22 @@ 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 allows 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 driver it should be
> >
> > > +expressed in unit of lines whenever possible.
> >
> > Same comment here.
> 
> I might have missed what other comment you are referring to :)

Sorry, I'm referring to your comment:

"I think this might be useful yes. A few paragraph above the frame
duration calculation formula is expressed as well, so I guess adding
one for this is helpful too"

> 
> >
> > Can you add formula/references about this point I think you are referring on "tline" units (maybe I'm completely wrong :) ),
> 
> Is "tline" the line duration ? If that's the case then no, I am
> referring to the number of lines, not their duration.

Ok. Thanks, need to find some docs regarding this units :'(
For this I think having some good reference/formula here would help users to find the
corrispective value into the sensor datasheet.

I just proved this to you :)

Regards,
Tommaso

> 
> > but to be honest checking also the some sensors datasheet I don't find to much infos about this units.
> > Would be really helpfull to add some details on this point.
> >
> > > +
> > > +Camera sensor driver should try whenever possible to distinguish between the
> > > +analogue and digital gain control functions. Analogue gain is a multiplier
> > > +factor applied to all color channels on the pixel array before they get
> > > +converted in the digital domain. It should be be made controllable by
> > > +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
> > > --
> >
> > This part looks good to me.
> >
> > Thanks,
> > Tommaso
> >
> > > 2.40.1
> > >

Re: [PATCH 1/2] Documentation: v4l: Flip handling for RAW sensors

[Laurent Pinchart]

On Tue, Jul 04, 2023 at 12:03:00PM +0100, Dave Stevenson wrote:
> On Tue, 4 Jul 2023 at 11:24, Jacopo Mondi wrote:
> > On Tue, Jul 04, 2023 at 10:58:11AM +0100, Dave Stevenson wrote:
> > > Hi Jacopo
> > >
> > > Thanks for adding documentation.
> > > Sorry I couldn't be at your presentation, but I'll find the slides
> > > (and recording if there is one).
> >
> > videos and slides should be already available for attendees. If not I
> > can send you the slide deck, but trust me, there is nothing new for
> > you there.
> >
> > > On Mon, 3 Jul 2023 at 21:29, Jacopo Mondi wrote:
> > > >
> > > > Document the requirement of notifying to userspace the possible
> > > > re-ordering of the color sample components when a vertical or horizontal
> > > > flip is applied to a RAW camera sensor.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > ---
> > > >  Documentation/driver-api/media/camera-sensor.rst | 16 ++++++++++++++++
> > > >  1 file changed, 16 insertions(+)
> > > >
> > > > diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
> > > > index 93f4f2536c25..ee4a7fe5f72a 100644
> > > > --- a/Documentation/driver-api/media/camera-sensor.rst
> > > > +++ b/Documentation/driver-api/media/camera-sensor.rst
> > > > @@ -173,3 +173,19 @@ V4L2_CID_VFLIP controls with the values programmed by the register sequences.
> > > >  The default values of these controls shall be 0 (disabled). Especially these
> > > >  controls shall not be inverted, independently of the sensor's mounting
> > > >  rotation.
> > > > +
> > > > +Flip handling for raw camera sensors
> > > > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >
> > > Possibly "for colour raw camera sensors".
> > > Mono sensors are still raw in that they need processing for black
> > > level, lens shading, etc, but they won't change the colour ordering.
> >
> > Thanks, good point
> >
> > > > +
> > > > +Applying vertical and horizontal flips on raw camera sensors inverts the color
> > > > +sample reading direction on the sensor's pixel array. This causes the
> > > > +re-ordering of the color samples on the sensor's output frame.
> > >
> > > This *may* cause the re-ordering....
> > >
> > > Not all sensors do. Some shift the readout by one line/column to keep
> > > the Bayer order the same, and technically should update the selection.
> > > OnSemi sensors in particular seem to do this, as do the Sony
> > > IMX327/290/462 family.
> >
> > Is it the driver doing the shift or is it the sensor automatically
> > adjusting ?
> 
> The sensor does it.

Both cases exist, some drivers do the adjustments internally. That's not
something I would recommend, but it was implemented to allow changing
the flip controls during streaming as we lacked (and still lack) a
solution to this problem.

There are also sensors that make this behaviour configurable, with a
register bit to indicate if the crop rectangle should be adjusted
automatically.

While moving the crop rectangle by one pixel in a large resolution
sensor could seem difficult to notice, it can have very noticeable
effects. For instance, defective pixel correction that operates on a
list of known bad pixels will require adjustement.

> For IMX290, the array is defined horizontally as:
> 4 ignored pixels
> 8 pixels margin for colour processing
> 1920 recording pixel area
> 9 pixels margin for colour processing
> 4 ignored pixels
> (3 dummy pixels)

Do you know what those dummy pixels are, and how they differ from the
ignored pixels that are not supposed to be read out either ?

> Ignoring the dummy ones, that gives a width of 1945 pixels.
> 
> Vertically is similar with 1109 pixels total (9 colour margin / 1080
> active / 8 colour margin).
> 
> That means it is a red pixel in all 4 corners of the full 1945x1109
> array, and it keeps the 8 pixels margin on the left of the image read
> out in all circumstances, and 8 at the top too so that readout always
> follows that.
> It's slightly weird as a sensor in that it has presets for 1080p and
> 720p, as well as a windowed mode where you can arbitrarily crop from
> the whole array. The driver only uses the two presets, so exactly what
> is going on is partly reverse engineering.
> 
> With regard OnSemi, we asked them about it several years ago, and the
> response was that there was no way to change that behaviour for the
> sensor we were looking at.
> From that datasheet:
> "While the sensor’s format is W × H, the additional active columns and
> active rows are included for use when horizontal or vertical mirrored
> readout is enabled, to allow readout to start on the same pixel. The
> pixel adjustment is always performed for monochrome or color versions"
> Definition of the VERT_FLIP bit
> "1: Readout is flipped (mirrored) vertically so that the row specified
> by y_addr_end_ (+1) is read out of the sensor first"
> and HORIZ_MIRROR bit
> "1: Readout is mirrored horizontally so that the column specified by
> x_addr_end_ (+1) is read out of the sensor first."
> 
> Having just checked a different OnSemi datasheet, that doesn't state
> that it alters the pixel read out first when flipped. It possibly
> depends on the target market for the sensor, and how configurable they
> anticipate the ISPs to be.
> 
> > > > As an example, a
> > > > +raw camera sensor with a Bayer pattern color filter array and a native RGGB
> > > > +Bayer order will produce frames with GRBG component ordering when a vertical
> > > > +flip is applied.
> > >
> > > Vertical flip of RGGB would be GBRG as the RG and GB get swapped, not
> > > GRBG (which would be horizontal flip).
> >
> > I clearly mean horizontal sorry. I'll fix.
> >
> > > > Camera sensor drivers where inverting the reading order
> > > > +direction causes a re-ordering of the color components are requested to register
> > > > +the ``V4L2_CID_VFLIP`` and ``V4L2_CID_HFLIP`` controls with the
> > > > +``V4L2_CTRL_FLAG_MODIFY_LAYOUT`` flag enabled to notify userspace that enabling
> > > > +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.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] Documentation: v4l: Exposure/gain for camera sensor

[Jacopo Mondi]

Hi Tommaso

On Tue, Jul 04, 2023 at 04:30:09PM +0200, Tommaso Merciai wrote:
> Hi Jacopo,
>
> On Tue, Jul 04, 2023 at 04:05:44PM +0200, Jacopo Mondi wrote:
> > Hi Tommaso
> >
> > On Tue, Jul 04, 2023 at 03:51:43PM +0200, Tommaso Merciai wrote:
> > > Hi Jacopo,
> > >
> > > On Mon, Jul 03, 2023 at 10:29:10PM +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>
> > > > ---
> > > >  .../driver-api/media/camera-sensor.rst        | 19 +++++++++++++++++++
> > > >  1 file changed, 19 insertions(+)
> > > >
> > > > diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
> > > > index ee4a7fe5f72a..dfe8f35aecea 100644
> > > > --- a/Documentation/driver-api/media/camera-sensor.rst
> > > > +++ b/Documentation/driver-api/media/camera-sensor.rst
> > > > @@ -189,3 +189,22 @@ 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 allows 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 driver it should be
> > >
> > > > +expressed in unit of lines whenever possible.
> > >
> > > Same comment here.
> >
> > I might have missed what other comment you are referring to :)
>
> Sorry, I'm referring to your comment:
>
> "I think this might be useful yes. A few paragraph above the frame
> duration calculation formula is expressed as well, so I guess adding
> one for this is helpful too"
>

Ah ok, see v2 for that

> >
> > >
> > > Can you add formula/references about this point I think you are referring on "tline" units (maybe I'm completely wrong :) ),
> >
> > Is "tline" the line duration ? If that's the case then no, I am
> > referring to the number of lines, not their duration.
>
> Ok. Thanks, need to find some docs regarding this units :'(
> For this I think having some good reference/formula here would help users to find the
> corrispective value into the sensor datasheet.
>

I'm still not sure if you're talking about the formula to convert from
a time duration to the number of lines as Dave suggested (which I
added in v2) or a formula to convert from the number of lines provided
by userspace as the value of the V4L2_CID_EXPOSURE control to the
actual value to be set in the sensor's registers that control the
exposure time. If you're referring to the latter I'm afraid this is
device specific and putting any example here might actually be
mis-leading. As far as I can tell the sensors I had dealt with,
internally represents the exposure control in number of lines or
fractions of lines. Doing the conversion in the driver is usually
trivial (I'm sure there are devices where this is less trivial of
course).

Did I get your question right or am I still missing something ?

> I just proved this to you :)
>
> Regards,
> Tommaso
>
> >
> > > but to be honest checking also the some sensors datasheet I don't find to much infos about this units.
> > > Would be really helpfull to add some details on this point.
> > >
> > > > +
> > > > +Camera sensor driver should try whenever possible to distinguish between the
> > > > +analogue and digital gain control functions. Analogue gain is a multiplier
> > > > +factor applied to all color channels on the pixel array before they get
> > > > +converted in the digital domain. It should be be made controllable by
> > > > +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
> > > > --
> > >
> > > This part looks good to me.
> > >
> > > Thanks,
> > > Tommaso
> > >
> > > > 2.40.1
> > > >

Re: [PATCH 1/2] Documentation: v4l: Flip handling for RAW sensors

[Jacopo Mondi]

Hi Laurent

On Tue, Jul 04, 2023 at 05:56:16PM +0300, Laurent Pinchart wrote:
> On Tue, Jul 04, 2023 at 12:03:00PM +0100, Dave Stevenson wrote:
> > On Tue, 4 Jul 2023 at 11:24, Jacopo Mondi wrote:
> > > On Tue, Jul 04, 2023 at 10:58:11AM +0100, Dave Stevenson wrote:
> > > > Hi Jacopo
> > > >
> > > > Thanks for adding documentation.
> > > > Sorry I couldn't be at your presentation, but I'll find the slides
> > > > (and recording if there is one).
> > >
> > > videos and slides should be already available for attendees. If not I
> > > can send you the slide deck, but trust me, there is nothing new for
> > > you there.
> > >
> > > > On Mon, 3 Jul 2023 at 21:29, Jacopo Mondi wrote:
> > > > >
> > > > > Document the requirement of notifying to userspace the possible
> > > > > re-ordering of the color sample components when a vertical or horizontal
> > > > > flip is applied to a RAW camera sensor.
> > > > >
> > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > > ---
> > > > >  Documentation/driver-api/media/camera-sensor.rst | 16 ++++++++++++++++
> > > > >  1 file changed, 16 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
> > > > > index 93f4f2536c25..ee4a7fe5f72a 100644
> > > > > --- a/Documentation/driver-api/media/camera-sensor.rst
> > > > > +++ b/Documentation/driver-api/media/camera-sensor.rst
> > > > > @@ -173,3 +173,19 @@ V4L2_CID_VFLIP controls with the values programmed by the register sequences.
> > > > >  The default values of these controls shall be 0 (disabled). Especially these
> > > > >  controls shall not be inverted, independently of the sensor's mounting
> > > > >  rotation.
> > > > > +
> > > > > +Flip handling for raw camera sensors
> > > > > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > >
> > > > Possibly "for colour raw camera sensors".
> > > > Mono sensors are still raw in that they need processing for black
> > > > level, lens shading, etc, but they won't change the colour ordering.
> > >
> > > Thanks, good point
> > >
> > > > > +
> > > > > +Applying vertical and horizontal flips on raw camera sensors inverts the color
> > > > > +sample reading direction on the sensor's pixel array. This causes the
> > > > > +re-ordering of the color samples on the sensor's output frame.
> > > >
> > > > This *may* cause the re-ordering....
> > > >
> > > > Not all sensors do. Some shift the readout by one line/column to keep
> > > > the Bayer order the same, and technically should update the selection.
> > > > OnSemi sensors in particular seem to do this, as do the Sony
> > > > IMX327/290/462 family.
> > >
> > > Is it the driver doing the shift or is it the sensor automatically
> > > adjusting ?
> >
> > The sensor does it.
>
> Both cases exist, some drivers do the adjustments internally. That's not
> something I would recommend, but it was implemented to allow changing
> the flip controls during streaming as we lacked (and still lack) a
> solution to this problem.

Ah right! Should I state that for this reasons sensor's that are
affected by color pattern reordering should disable changing the value
of flips during streaming ?

Thanks
   j

>
> There are also sensors that make this behaviour configurable, with a
> register bit to indicate if the crop rectangle should be adjusted
> automatically.
>
> While moving the crop rectangle by one pixel in a large resolution
> sensor could seem difficult to notice, it can have very noticeable
> effects. For instance, defective pixel correction that operates on a
> list of known bad pixels will require adjustement.
>
> > For IMX290, the array is defined horizontally as:
> > 4 ignored pixels
> > 8 pixels margin for colour processing
> > 1920 recording pixel area
> > 9 pixels margin for colour processing
> > 4 ignored pixels
> > (3 dummy pixels)
>
> Do you know what those dummy pixels are, and how they differ from the
> ignored pixels that are not supposed to be read out either ?
>
> > Ignoring the dummy ones, that gives a width of 1945 pixels.
> >
> > Vertically is similar with 1109 pixels total (9 colour margin / 1080
> > active / 8 colour margin).
> >
> > That means it is a red pixel in all 4 corners of the full 1945x1109
> > array, and it keeps the 8 pixels margin on the left of the image read
> > out in all circumstances, and 8 at the top too so that readout always
> > follows that.
> > It's slightly weird as a sensor in that it has presets for 1080p and
> > 720p, as well as a windowed mode where you can arbitrarily crop from
> > the whole array. The driver only uses the two presets, so exactly what
> > is going on is partly reverse engineering.
> >
> > With regard OnSemi, we asked them about it several years ago, and the
> > response was that there was no way to change that behaviour for the
> > sensor we were looking at.
> > From that datasheet:
> > "While the sensor’s format is W × H, the additional active columns and
> > active rows are included for use when horizontal or vertical mirrored
> > readout is enabled, to allow readout to start on the same pixel. The
> > pixel adjustment is always performed for monochrome or color versions"
> > Definition of the VERT_FLIP bit
> > "1: Readout is flipped (mirrored) vertically so that the row specified
> > by y_addr_end_ (+1) is read out of the sensor first"
> > and HORIZ_MIRROR bit
> > "1: Readout is mirrored horizontally so that the column specified by
> > x_addr_end_ (+1) is read out of the sensor first."
> >
> > Having just checked a different OnSemi datasheet, that doesn't state
> > that it alters the pixel read out first when flipped. It possibly
> > depends on the target market for the sensor, and how configurable they
> > anticipate the ISPs to be.
> >
> > > > > As an example, a
> > > > > +raw camera sensor with a Bayer pattern color filter array and a native RGGB
> > > > > +Bayer order will produce frames with GRBG component ordering when a vertical
> > > > > +flip is applied.
> > > >
> > > > Vertical flip of RGGB would be GBRG as the RG and GB get swapped, not
> > > > GRBG (which would be horizontal flip).
> > >
> > > I clearly mean horizontal sorry. I'll fix.
> > >
> > > > > Camera sensor drivers where inverting the reading order
> > > > > +direction causes a re-ordering of the color components are requested to register
> > > > > +the ``V4L2_CID_VFLIP`` and ``V4L2_CID_HFLIP`` controls with the
> > > > > +``V4L2_CTRL_FLAG_MODIFY_LAYOUT`` flag enabled to notify userspace that enabling
> > > > > +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.
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH 1/2] Documentation: v4l: Flip handling for RAW sensors

[Dave Stevenson]

Hi Laurent

On Tue, 4 Jul 2023 at 15:56, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Tue, Jul 04, 2023 at 12:03:00PM +0100, Dave Stevenson wrote:
> > On Tue, 4 Jul 2023 at 11:24, Jacopo Mondi wrote:
> > > On Tue, Jul 04, 2023 at 10:58:11AM +0100, Dave Stevenson wrote:
> > > > Hi Jacopo
> > > >
> > > > Thanks for adding documentation.
> > > > Sorry I couldn't be at your presentation, but I'll find the slides
> > > > (and recording if there is one).
> > >
> > > videos and slides should be already available for attendees. If not I
> > > can send you the slide deck, but trust me, there is nothing new for
> > > you there.
> > >
> > > > On Mon, 3 Jul 2023 at 21:29, Jacopo Mondi wrote:
> > > > >
> > > > > Document the requirement of notifying to userspace the possible
> > > > > re-ordering of the color sample components when a vertical or horizontal
> > > > > flip is applied to a RAW camera sensor.
> > > > >
> > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > > ---
> > > > >  Documentation/driver-api/media/camera-sensor.rst | 16 ++++++++++++++++
> > > > >  1 file changed, 16 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
> > > > > index 93f4f2536c25..ee4a7fe5f72a 100644
> > > > > --- a/Documentation/driver-api/media/camera-sensor.rst
> > > > > +++ b/Documentation/driver-api/media/camera-sensor.rst
> > > > > @@ -173,3 +173,19 @@ V4L2_CID_VFLIP controls with the values programmed by the register sequences.
> > > > >  The default values of these controls shall be 0 (disabled). Especially these
> > > > >  controls shall not be inverted, independently of the sensor's mounting
> > > > >  rotation.
> > > > > +
> > > > > +Flip handling for raw camera sensors
> > > > > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > >
> > > > Possibly "for colour raw camera sensors".
> > > > Mono sensors are still raw in that they need processing for black
> > > > level, lens shading, etc, but they won't change the colour ordering.
> > >
> > > Thanks, good point
> > >
> > > > > +
> > > > > +Applying vertical and horizontal flips on raw camera sensors inverts the color
> > > > > +sample reading direction on the sensor's pixel array. This causes the
> > > > > +re-ordering of the color samples on the sensor's output frame.
> > > >
> > > > This *may* cause the re-ordering....
> > > >
> > > > Not all sensors do. Some shift the readout by one line/column to keep
> > > > the Bayer order the same, and technically should update the selection.
> > > > OnSemi sensors in particular seem to do this, as do the Sony
> > > > IMX327/290/462 family.
> > >
> > > Is it the driver doing the shift or is it the sensor automatically
> > > adjusting ?
> >
> > The sensor does it.
>
> Both cases exist, some drivers do the adjustments internally. That's not
> something I would recommend, but it was implemented to allow changing
> the flip controls during streaming as we lacked (and still lack) a
> solution to this problem.

I'll believe you, but had never noticed it in a mainline driver.

> There are also sensors that make this behaviour configurable, with a
> register bit to indicate if the crop rectangle should be adjusted
> automatically.
>
> While moving the crop rectangle by one pixel in a large resolution
> sensor could seem difficult to notice, it can have very noticeable
> effects. For instance, defective pixel correction that operates on a
> list of known bad pixels will require adjustement.
>
> > For IMX290, the array is defined horizontally as:
> > 4 ignored pixels
> > 8 pixels margin for colour processing
> > 1920 recording pixel area
> > 9 pixels margin for colour processing
> > 4 ignored pixels
> > (3 dummy pixels)
>
> Do you know what those dummy pixels are, and how they differ from the
> ignored pixels that are not supposed to be read out either ?

Going off topic for this documentation thread, but c'est la vie.

I'm assuming you have a datasheet for IMX290 or 327 as you've
previously worked on the driver.

"Drive Timing Chart for Full HD 1080p mode (CSI-2 serial output)" on
(for me) page 62 says that there are1948 pixels are being produced per
line - 4 side ignored area, 8 margin for colour processing, 1920
active, 9 margin for colour processing, 4 side ignored area, and 3
dummy. The dummy ones are not numbered, so presumably aren't actually
read from the pixel array.
Vertically it says you're getting lines 3-12 (optical black), and then
13-1109 (8 + 1080 + 9) as active lines.

The Drive Timing Chart for the 720p mode (page 76) says that it's
sending pixels 321 to 1625 (inclusive) per line as 4 ignored, 8
margin, 1280 active, 9 margin, 4 ignored, and then 3 dummy area
samples (again not numbered).
Vertically it says you're getting lines 3-6 (optical black), and then
197-925 as active lines.

Looking again at that diagram, it lists out exactly which pixels/lines
are sent in normal and inverted modes, so the shift-by-1 is explicitly
stated. The OB lines always stay at the top of the image.
OB data appears to be sent with data type 0x37, so at least for Unicam
it won't end up in the image buffer.

Further comparison between the datasheet and driver show that for
1080p the datasheet intends X_OUT_SIZE = 0x79c (1948) and Y_OUT_SIZE =
0x449 (1097), and for the 720p mode 0x51c (1308) x 0x2d9 (729). The
driver is setting those to 1920x1080 and 1280x720 respectively,
therefore the output FOV may well not be that which is expected. I
seem to recall making a comment along those lines when I upstreamed
the last load of patches for imx290, but have never had the time to
fully investigate.

  Dave

> > Ignoring the dummy ones, that gives a width of 1945 pixels.
> >
> > Vertically is similar with 1109 pixels total (9 colour margin / 1080
> > active / 8 colour margin).
> >
> > That means it is a red pixel in all 4 corners of the full 1945x1109
> > array, and it keeps the 8 pixels margin on the left of the image read
> > out in all circumstances, and 8 at the top too so that readout always
> > follows that.
> > It's slightly weird as a sensor in that it has presets for 1080p and
> > 720p, as well as a windowed mode where you can arbitrarily crop from
> > the whole array. The driver only uses the two presets, so exactly what
> > is going on is partly reverse engineering.
> >
> > With regard OnSemi, we asked them about it several years ago, and the
> > response was that there was no way to change that behaviour for the
> > sensor we were looking at.
> > From that datasheet:
> > "While the sensor’s format is W × H, the additional active columns and
> > active rows are included for use when horizontal or vertical mirrored
> > readout is enabled, to allow readout to start on the same pixel. The
> > pixel adjustment is always performed for monochrome or color versions"
> > Definition of the VERT_FLIP bit
> > "1: Readout is flipped (mirrored) vertically so that the row specified
> > by y_addr_end_ (+1) is read out of the sensor first"
> > and HORIZ_MIRROR bit
> > "1: Readout is mirrored horizontally so that the column specified by
> > x_addr_end_ (+1) is read out of the sensor first."
> >
> > Having just checked a different OnSemi datasheet, that doesn't state
> > that it alters the pixel read out first when flipped. It possibly
> > depends on the target market for the sensor, and how configurable they
> > anticipate the ISPs to be.
> >
> > > > > As an example, a
> > > > > +raw camera sensor with a Bayer pattern color filter array and a native RGGB
> > > > > +Bayer order will produce frames with GRBG component ordering when a vertical
> > > > > +flip is applied.
> > > >
> > > > Vertical flip of RGGB would be GBRG as the RG and GB get swapped, not
> > > > GRBG (which would be horizontal flip).
> > >
> > > I clearly mean horizontal sorry. I'll fix.
> > >
> > > > > Camera sensor drivers where inverting the reading order
> > > > > +direction causes a re-ordering of the color components are requested to register
> > > > > +the ``V4L2_CID_VFLIP`` and ``V4L2_CID_HFLIP`` controls with the
> > > > > +``V4L2_CTRL_FLAG_MODIFY_LAYOUT`` flag enabled to notify userspace that enabling
> > > > > +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.
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH 2/2] Documentation: v4l: Exposure/gain for camera sensor

[Tommaso Merciai]

Hi Jacopo,

On Tue, Jul 04, 2023 at 05:00:24PM +0200, Jacopo Mondi wrote:
> Hi Tommaso
> 
> On Tue, Jul 04, 2023 at 04:30:09PM +0200, Tommaso Merciai wrote:
> > Hi Jacopo,
> >
> > On Tue, Jul 04, 2023 at 04:05:44PM +0200, Jacopo Mondi wrote:
> > > Hi Tommaso
> > >
> > > On Tue, Jul 04, 2023 at 03:51:43PM +0200, Tommaso Merciai wrote:
> > > > Hi Jacopo,
> > > >
> > > > On Mon, Jul 03, 2023 at 10:29:10PM +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>
> > > > > ---
> > > > >  .../driver-api/media/camera-sensor.rst        | 19 +++++++++++++++++++
> > > > >  1 file changed, 19 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst
> > > > > index ee4a7fe5f72a..dfe8f35aecea 100644
> > > > > --- a/Documentation/driver-api/media/camera-sensor.rst
> > > > > +++ b/Documentation/driver-api/media/camera-sensor.rst
> > > > > @@ -189,3 +189,22 @@ 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 allows 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 driver it should be
> > > >
> > > > > +expressed in unit of lines whenever possible.
> > > >
> > > > Same comment here.
> > >
> > > I might have missed what other comment you are referring to :)
> >
> > Sorry, I'm referring to your comment:
> >
> > "I think this might be useful yes. A few paragraph above the frame
> > duration calculation formula is expressed as well, so I guess adding
> > one for this is helpful too"
> >
> 
> Ah ok, see v2 for that
> 
> > >
> > > >
> > > > Can you add formula/references about this point I think you are referring on "tline" units (maybe I'm completely wrong :) ),
> > >
> > > Is "tline" the line duration ? If that's the case then no, I am
> > > referring to the number of lines, not their duration.
> >
> > Ok. Thanks, need to find some docs regarding this units :'(
> > For this I think having some good reference/formula here would help users to find the
> > corrispective value into the sensor datasheet.
> >
> 

> I'm still not sure if you're talking about the formula to convert from
> a time duration to the number of lines as Dave suggested (which I
> added in v2) 

This one. I checked v2 thanks for the clarification!

Regards,
Tommaso


> or a formula to convert from the number of lines provided
> by userspace as the value of the V4L2_CID_EXPOSURE control to the
> actual value to be set in the sensor's registers that control the
> exposure time. If you're referring to the latter I'm afraid this is
> device specific and putting any example here might actually be
> mis-leading. As far as I can tell the sensors I had dealt with,
> internally represents the exposure control in number of lines or
> fractions of lines. Doing the conversion in the driver is usually
> trivial (I'm sure there are devices where this is less trivial of
> course).
> 
> Did I get your question right or am I still missing something ?
> 
> > I just proved this to you :)
> >
> > Regards,
> > Tommaso
> >
> > >
> > > > but to be honest checking also the some sensors datasheet I don't find to much infos about this units.
> > > > Would be really helpfull to add some details on this point.
> > > >
> > > > > +
> > > > > +Camera sensor driver should try whenever possible to distinguish between the
> > > > > +analogue and digital gain control functions. Analogue gain is a multiplier
> > > > > +factor applied to all color channels on the pixel array before they get
> > > > > +converted in the digital domain. It should be be made controllable by
> > > > > +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
> > > > > --
> > > >
> > > > This part looks good to me.
> > > >
> > > > Thanks,
> > > > Tommaso
> > > >
> > > > > 2.40.1
> > > > >