diff mbox series

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

Message ID 20230703202910.31142-2-jacopo.mondi@ideasonboard.com (mailing list archive)
State New, archived
Headers show
Series Documentation: v4l: more camera sensor doc | expand

Commit Message

Jacopo Mondi July 3, 2023, 8:29 p.m. UTC
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(+)

Comments

Dave Stevenson July 4, 2023, 9:58 a.m. UTC | #1
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
>
Jacopo Mondi July 4, 2023, 10:24 a.m. UTC | #2
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
> >
Dave Stevenson July 4, 2023, 11:03 a.m. UTC | #3
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
> > >
Laurent Pinchart July 4, 2023, 2:56 p.m. UTC | #4
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.
Jacopo Mondi July 4, 2023, 3:02 p.m. UTC | #5
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
Dave Stevenson July 4, 2023, 3:51 p.m. UTC | #6
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
diff mbox series

Patch

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.