(no subject)

[Jacopo Mondi]

Subject: [PATCH 0/4] media: docs: Document pixel array properties

Hans' patch "[PATCH] imx219: selection compliance fixes" sparkled a discussion
on how the V4L2 selection targets have to be used in order to access an
image sensor pixel array properties.

The discussion shown how much under-specified that part was, so this is
an attempt to provide a bit documentation for this.

My feeling is that we're hijacking the existing targets for this use case
and we should probably define new ones, considering how few users we have in
mainline of them at the moment.

On top Hans' patch with reworded commit message and minor updates.

For reference, we're using the V4L2 selection targets in libcamera to retrieve
the sensor pixel array properties to be reported to applications for
calibration purposes. The currently defined pixel properties for libcamera
are available here:
https://git.linuxtv.org/libcamera.git/tree/src/libcamera/property_ids.yaml#n390

Thanks
   j

Hans Verkuil (1):
  media: i2c: imx219: Selection compliance fixes

Jacopo Mondi (3):
  media: docs: Describe pixel array properties
  media: docs: Describe targets for sensor properties
  media: docs: USe SUBDEV_G_SELECTION for sensor properties

 .../userspace-api/media/v4l/dev-subdev.rst    | 85 +++++++++++++++++++
 .../media/v4l/v4l2-selection-targets.rst      | 49 +++++++++++
 .../media/v4l/vidioc-subdev-g-selection.rst   |  4 +
 drivers/media/i2c/imx219.c                    | 17 ++--
 4 files changed, 147 insertions(+), 8 deletions(-)

--
2.27.0

[PATCH 1/4] media: docs: Describe pixel array properties

[Jacopo Mondi]

The V4L2 selection API are also used to access the pixel array
properties of an image sensor, such as the size and position of active
pixels and the cropped area of the pixel matrix used to produce images.

Currently no clear definition of the different areas that compose an
image sensor pixel array matrix is provided in the specification, and
the actual meaning of each selection target when applied to an image
sensor was not provided.

Provide in the sub-device documentation the definition of the pixel
matrix properties and the selection target associated to each of them.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 .../userspace-api/media/v4l/dev-subdev.rst    | 81 +++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
index 134d2fb909fa4..c47861dff9b9b 100644
--- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
+++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
@@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.
 ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round
 the image size either up or down. :ref:`v4l2-selection-flags`
 
+.. _v4l2-subdev-pixel-array-properties:
+
+Selection targets for image sensors properties
+----------------------------------------------
+
+The V4L2 selection API can be used on sub-devices that represent an image
+sensor to retrieve the sensor's pixel array matrix properties by using the
+:ref:`selection <VIDIOC_SUBDEV_G_SELECTION>` ioctls.
+
+Sub-device drivers for image sensor usually register a single source pad, but in
+the case they expose more, the pixel array properties can be accessed from
+any of them.
+
+The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,
+``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve
+the immutable properties of the several different areas that compose the sensor
+pixel array matrix. Each area describes a rectangle of logically adjacent pixel
+units. The logical disposition of pixels is defined by the sensor read-out
+starting point and direction, and may differ from the physical disposition of
+the pixel units in the pixel array matrix.
+
+Each pixel matrix portion is contained in a larger rectangle, with the most
+largest being the one that describes the pixel matrix physical size. This
+defines a hierarchical positional system, where each rectangle is defined
+relatively to the largest available one among the ones exposed by the
+sub-device driver. Each selection target and the associated pixel array portion
+it represents are below presented in order from the largest to the smallest one.
+
+Pixel array physical size
+^^^^^^^^^^^^^^^^^^^^^^^^^
+
+The image sensor chip is composed by a number of physical pixels, not all of
+them readable by the application processor. Invalid or unreadable lines might
+not be transmitted on the data bus at all, or in case on CSI-2 capable sensors
+they might be tagged with an invalid data type (DT) so that the receiver
+automatically discard them. The size of the whole pixel matrix area is
+retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner
+defined as position (0, 0). All the other selection targets are defined
+relatively to this, larger, rectangle. The rectangle returned by
+V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it
+does not change at run-time and cannot be modified from userspace.
+
+Pixel array readable area
+^^^^^^^^^^^^^^^^^^^^^^^^^
+
+The V4L2_SEL_TGT_CROP_BOUNDS targets returns size and position of the readable
+area of the pixel array matrix, including pixels with valid image data and pixel
+used for calibration purposes, such as optical black pixels. It is not unlikely
+that valid pixels and optical black pixels are surrounded by non-readable rows
+and columns of pixels. Those does not concur in the definition of the
+V4L2_SEL_TGT_CROP_BOUNDS rectangle. The rectangle returned by
+V4L2_SEL_TGT_CROP_BOUNDS describes an immutable property of the image sensor, it
+does not change at run-time and cannot be modified from userspace.
+
+Pixel array active area
+^^^^^^^^^^^^^^^^^^^^^^^
+
+The portion of the pixel array which contains valid image data is defined as the
+active area of the pixel matrix. The active pixel array is is accessed by mean
+of the V4L2_SEL_TGT_CROP_DEFAULT target, and is contained in the larger
+V4L2_SEL_TGT_CROP_BOUNDS rectangle. It represents the largest possible frame
+resolution the sensor can produce and defines the dimension of the full
+field-of-view. The rectangle returned by V4L2_SEL_TGT_CROP_BOUNDS describes an
+immutable property of the image sensor, it does not change at run-time and
+cannot be modified from userspace.
+
+Analog crop rectangle
+^^^^^^^^^^^^^^^^^^^^^
+
+The sensor driver might decide, in order to adjust the image resolution to best
+match the one requested by applications, to only process a part of the active
+pixel array matrix. The selected area is read-out and processed by the image
+sensor on-board ISP in order to produce images of the desired size and
+resolution while possible maintaing the largest possible field-of-view. The
+cropped portion of the pixel array which is used to produce images is returned
+by the V4L2_SEL_TGT_CROP target and represent the only information that can
+change at runtime as it depends on the currently configured sensor mode and
+desired image resolution. If the sub-device driver supports that, userspace
+can set the analog crop rectangle to select which portion of the pixel array
+to read out.
+
 
 Types of selection targets
 --------------------------
-- 
2.27.0

[PATCH 2/4] media: docs: Describe targets for sensor properties

[Jacopo Mondi]

Provide a table to describe how the V4L2 selection targets can be used
to access an image sensor pixel array properties.

Reference the table in the sub-device documentation.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 .../userspace-api/media/v4l/dev-subdev.rst    |  4 ++
 .../media/v4l/v4l2-selection-targets.rst      | 49 +++++++++++++++++++
 2 files changed, 53 insertions(+)

diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
index c47861dff9b9b..2f7da3832f458 100644
--- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
+++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
@@ -467,6 +467,10 @@ desired image resolution. If the sub-device driver supports that, userspace
 can set the analog crop rectangle to select which portion of the pixel array
 to read out.
 
+A description of each of the above mentioned targets when used to access the
+image sensor pixel array properties is provided by
+:ref:`v4l2-selection-targets-image-sensor-table`
+
 
 Types of selection targets
 --------------------------
diff --git a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
index 69f500093aa2a..632e6448b784e 100644
--- a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
+++ b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
@@ -76,3 +76,52 @@ of the two interfaces they are used.
 	modified by hardware.
       - Yes
       - No
+
+
+.. _v4l2-selection-targets-image-sensor-table:
+
+********************************************
+Selection Targets For Pixel Array Properties
+********************************************
+
+The V4L2 selection API can be used to retrieve the size and disposition of the
+pixel units that compose and image sensor pixel matrix when applied to a video
+sub-device that represents an image sensor.
+
+A description of the properties associated with each of the sensor pixel array
+areas is provided by the :ref:`v4l2-subdev-pixel-array-properties` section.
+
+.. tabularcolumns:: |p{6.0cm}|p{1.4cm}|p{7.4cm}|p(1.4cm)|
+
+.. flat-table:: Selection target definitions
+    :header-rows:  1
+    :stub-columns: 0
+
+    * - Target name
+      - id
+      - Definition
+      - Read/Write
+    * - ``V4L2_SEL_TGT_CROP``
+      - 0x0000
+      - The analog crop rectangle. Represents the portion of the active pixel
+        array which is processed to produce images.
+      - RW
+    * - ``V4L2_SEL_TGT_CROP_DEFAULT``
+      - 0x0001
+      - The active pixel array rectangle. It includes only active pixels and
+        excludes other ones such as optical black pixels. Its width and height
+        represent the maximum image resolution an image sensor can produce.
+      - RO
+    * - ``V4L2_SEL_TGT_CROP_BOUNDS``
+      - 0x0002
+      - The readable portion of the physical pixel array matrix. It includes
+        pixels that contains valid image data and calibration pixels such as the
+        optical black ones.
+      - RO
+    * - ``V4L2_SEL_TGT_NATIVE_SIZE``
+      - 0x0003
+      - The physical pixel array size, including readable and not readable
+        pixels. As pixels that cannot be read from application processor are not
+        relevant for calibration purposes, this rectangle is useful to calculate
+        the physical properties of the image sensor.
+      - RO
-- 
2.27.0

[PATCH 3/4] media: docs: USe SUBDEV_G_SELECTION for sensor properties

[Jacopo Mondi]

Add a small note to the VIDIOC_SUBDEV_G_SELECTION IOCTL documentation
to report that the API can be used to access an image sensor pixel array
properties.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 .../userspace-api/media/v4l/vidioc-subdev-g-selection.rst     | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-selection.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-selection.rst
index 06c9553ac48f5..05539f5deace2 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-selection.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-selection.rst
@@ -46,6 +46,10 @@ The selections are used to configure various image processing
 functionality performed by the subdevs which affect the image size. This
 currently includes cropping, scaling and composition.
 
+This API can also be used to retrieve immutable properties of the device
+represented by the subdev, such as the pixel matrix properties of an image
+sensor.
+
 The selection API replaces
 :ref:`the old subdev crop API <VIDIOC_SUBDEV_G_CROP>`. All the
 function of the crop API, and more, are supported by the selections API.
-- 
2.27.0

[PATCH 4/4] media: i2c: imx219: Selection compliance fixes

[Jacopo Mondi]

From: Hans Verkuil <hverkuil@xs4all.nl>

To comply with the intended usage of the V4L2 selection target when
used to retrieve a sensor image properties, adjust the rectangles
returned by the imx219 driver.

The top/left crop coordinates of the TGT_CROP rectangle were set to
(0, 0) instead of (8, 8) which is the offset from the larger physical
pixel array rectangle. This was also a mismatch with the default values
crop rectangle value, so this is corrected. Found with v4l2-compliance.

While at it, add V4L2_SEL_TGT_CROP_BOUNDS support: CROP_DEFAULT and
CROP_BOUNDS have the same size as the non-active pixels are not readable
using the selection API. Found with v4l2-compliance.

Fixes: e6d4ef7d58aa7 ("media: i2c: imx219: Implement get_selection")
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
[reword commit message, use macros for pixel offsets]
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/imx219.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 60b23d113fc56..ff48a95b448b1 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -473,8 +473,8 @@ static const struct imx219_mode supported_modes[] = {
 		.width = 3280,
 		.height = 2464,
 		.crop = {
-			.left = 0,
-			.top = 0,
+			.left = IMX219_PIXEL_ARRAY_LEFT,
+			.top = IMX219_PIXEL_ARRAY_TOP,
 			.width = 3280,
 			.height = 2464
 		},
@@ -489,8 +489,8 @@ static const struct imx219_mode supported_modes[] = {
 		.width = 1920,
 		.height = 1080,
 		.crop = {
-			.left = 680,
-			.top = 692,
+			.left = 688,
+			.top = 700,
 			.width = 1920,
 			.height = 1080
 		},
@@ -505,8 +505,8 @@ static const struct imx219_mode supported_modes[] = {
 		.width = 1640,
 		.height = 1232,
 		.crop = {
-			.left = 0,
-			.top = 0,
+			.left = IMX219_PIXEL_ARRAY_LEFT,
+			.top = IMX219_PIXEL_ARRAY_TOP,
 			.width = 3280,
 			.height = 2464
 		},
@@ -521,8 +521,8 @@ static const struct imx219_mode supported_modes[] = {
 		.width = 640,
 		.height = 480,
 		.crop = {
-			.left = 1000,
-			.top = 752,
+			.left = 1008,
+			.top = 760,
 			.width = 1280,
 			.height = 960
 		},
@@ -1045,6 +1045,7 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
 		return 0;
 
 	case V4L2_SEL_TGT_CROP_DEFAULT:
+	case V4L2_SEL_TGT_CROP_BOUNDS:
 		sel->r.top = IMX219_PIXEL_ARRAY_TOP;
 		sel->r.left = IMX219_PIXEL_ARRAY_LEFT;
 		sel->r.width = IMX219_PIXEL_ARRAY_WIDTH;
-- 
2.27.0

Re: [PATCH 1/4] media: docs: Describe pixel array properties

[Sakari Ailus]

Hi Jacopo,

Thanks for the patchset.

This improves selection documentation quite a bit. Please see my comments
below.

On Wed, Aug 05, 2020 at 12:57:18PM +0200, Jacopo Mondi wrote:
> The V4L2 selection API are also used to access the pixel array
> properties of an image sensor, such as the size and position of active
> pixels and the cropped area of the pixel matrix used to produce images.
> 
> Currently no clear definition of the different areas that compose an
> image sensor pixel array matrix is provided in the specification, and
> the actual meaning of each selection target when applied to an image
> sensor was not provided.
> 
> Provide in the sub-device documentation the definition of the pixel
> matrix properties and the selection target associated to each of them.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  .../userspace-api/media/v4l/dev-subdev.rst    | 81 +++++++++++++++++++
>  1 file changed, 81 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> index 134d2fb909fa4..c47861dff9b9b 100644
> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.
>  ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round
>  the image size either up or down. :ref:`v4l2-selection-flags`
>  
> +.. _v4l2-subdev-pixel-array-properties:
> +
> +Selection targets for image sensors properties
> +----------------------------------------------
> +
> +The V4L2 selection API can be used on sub-devices that represent an image
> +sensor to retrieve the sensor's pixel array matrix properties by using the
> +:ref:`selection <VIDIOC_SUBDEV_G_SELECTION>` ioctls.
> +
> +Sub-device drivers for image sensor usually register a single source pad, but in
> +the case they expose more, the pixel array properties can be accessed from
> +any of them.

Is this a hypothetical case or are there examples?

Also note that camera sensor drivers may expose more than one sub-devices,
only one of which represents the pixel array.

> +
> +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,
> +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve
> +the immutable properties of the several different areas that compose the sensor
> +pixel array matrix. Each area describes a rectangle of logically adjacent pixel
> +units. The logical disposition of pixels is defined by the sensor read-out
> +starting point and direction, and may differ from the physical disposition of
> +the pixel units in the pixel array matrix.
> +
> +Each pixel matrix portion is contained in a larger rectangle, with the most

s/larger\K/ or equal/

s/most//

> +largest being the one that describes the pixel matrix physical size. This
> +defines a hierarchical positional system, where each rectangle is defined

s/,//

> +relatively to the largest available one among the ones exposed by the
> +sub-device driver. Each selection target and the associated pixel array portion
> +it represents are below presented in order from the largest to the smallest one.
> +
> +Pixel array physical size
> +^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +The image sensor chip is composed by a number of physical pixels, not all of
> +them readable by the application processor. Invalid or unreadable lines might
> +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors
> +they might be tagged with an invalid data type (DT) so that the receiver
> +automatically discard them. The size of the whole pixel matrix area is
> +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner
> +defined as position (0, 0). All the other selection targets are defined
> +relatively to this, larger, rectangle. The rectangle returned by
> +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it
> +does not change at run-time and cannot be modified from userspace.
> +
> +Pixel array readable area
> +^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +The V4L2_SEL_TGT_CROP_BOUNDS targets returns size and position of the readable
> +area of the pixel array matrix, including pixels with valid image data and pixel
> +used for calibration purposes, such as optical black pixels. It is not unlikely
> +that valid pixels and optical black pixels are surrounded by non-readable rows
> +and columns of pixels. Those does not concur in the definition of the

How about: "Only pixels that can be read out are included in the
V4L2_SEL_TGT_CROP_BOUNDS rectangle."?

> +V4L2_SEL_TGT_CROP_BOUNDS rectangle. The rectangle returned by
> +V4L2_SEL_TGT_CROP_BOUNDS describes an immutable property of the image sensor, it
> +does not change at run-time and cannot be modified from userspace.
> +
> +Pixel array active area
> +^^^^^^^^^^^^^^^^^^^^^^^
> +
> +The portion of the pixel array which contains valid image data is defined as the
> +active area of the pixel matrix. The active pixel array is is accessed by mean

s/accessed/described/

Another word than "active" here would be great as we already have active
and try contexts for selections.

> +of the V4L2_SEL_TGT_CROP_DEFAULT target, and is contained in the larger

s/the larger//

> +V4L2_SEL_TGT_CROP_BOUNDS rectangle. It represents the largest possible frame
> +resolution the sensor can produce and defines the dimension of the full

s/resolution/size/

> +field-of-view. The rectangle returned by V4L2_SEL_TGT_CROP_BOUNDS describes an

s/-/ /g

> +immutable property of the image sensor, it does not change at run-time and
> +cannot be modified from userspace.
> +
> +Analog crop rectangle
> +^^^^^^^^^^^^^^^^^^^^^
> +
> +The sensor driver might decide, in order to adjust the image resolution to best
> +match the one requested by applications, to only process a part of the active

s/, to\K/ instruct the hardware to/

> +pixel array matrix. The selected area is read-out and processed by the image
> +sensor on-board ISP in order to produce images of the desired size and
> +resolution while possible maintaing the largest possible field-of-view. The

s/size\K[^.]?*\./m

> +cropped portion of the pixel array which is used to produce images is returned

s/produce/read out/
s/returned/configured/

> +by the V4L2_SEL_TGT_CROP target and represent the only information that can

s/by/using/

I'd leave out the rest of the sentence after "target" above.

> +change at runtime as it depends on the currently configured sensor mode and
> +desired image resolution. If the sub-device driver supports that, userspace
> +can set the analog crop rectangle to select which portion of the pixel array
> +to read out.

How about instead:

Register list based drivers generally do not allow setting analogue crop
rectangles.

> +
>  
>  Types of selection targets
>  --------------------------

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 1/4] media: docs: Describe pixel array properties

[Hans Verkuil]

Hi Jacopo,

Some review comments below:

On 05/08/2020 12:57, Jacopo Mondi wrote:
> The V4L2 selection API are also used to access the pixel array

are -> is

> properties of an image sensor, such as the size and position of active
> pixels and the cropped area of the pixel matrix used to produce images.
> 
> Currently no clear definition of the different areas that compose an
> image sensor pixel array matrix is provided in the specification, and
> the actual meaning of each selection target when applied to an image
> sensor was not provided.
> 
> Provide in the sub-device documentation the definition of the pixel
> matrix properties and the selection target associated to each of them.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  .../userspace-api/media/v4l/dev-subdev.rst    | 81 +++++++++++++++++++
>  1 file changed, 81 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> index 134d2fb909fa4..c47861dff9b9b 100644
> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.
>  ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round
>  the image size either up or down. :ref:`v4l2-selection-flags`
>  
> +.. _v4l2-subdev-pixel-array-properties:
> +
> +Selection targets for image sensors properties
> +----------------------------------------------
> +
> +The V4L2 selection API can be used on sub-devices that represent an image
> +sensor to retrieve the sensor's pixel array matrix properties by using the
> +:ref:`selection <VIDIOC_SUBDEV_G_SELECTION>` ioctls.
> +
> +Sub-device drivers for image sensor usually register a single source pad, but in
> +the case they expose more, the pixel array properties can be accessed from
> +any of them.
> +
> +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,

V4L2_SEL_TGT_NATIVE -> V4L2_SEL_TGT_NATIVE_SIZE

(same mistake is made elsewhere).

> +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve
> +the immutable properties of the several different areas that compose the sensor
> +pixel array matrix. Each area describes a rectangle of logically adjacent pixel
> +units. The logical disposition of pixels is defined by the sensor read-out
> +starting point and direction, and may differ from the physical disposition of
> +the pixel units in the pixel array matrix.
> +
> +Each pixel matrix portion is contained in a larger rectangle, with the most
> +largest being the one that describes the pixel matrix physical size. This
> +defines a hierarchical positional system, where each rectangle is defined
> +relatively to the largest available one among the ones exposed by the
> +sub-device driver. Each selection target and the associated pixel array portion
> +it represents are below presented in order from the largest to the smallest one.
> +
> +Pixel array physical size
> +^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +The image sensor chip is composed by a number of physical pixels, not all of
> +them readable by the application processor. Invalid or unreadable lines might
> +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors
> +they might be tagged with an invalid data type (DT) so that the receiver
> +automatically discard them. The size of the whole pixel matrix area is
> +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner
> +defined as position (0, 0). All the other selection targets are defined
> +relatively to this, larger, rectangle. The rectangle returned by
> +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it
> +does not change at run-time and cannot be modified from userspace.

It is a good idea to mention that if there are no invalid or unreadable pixels/lines,
then V4L2_SEL_TGT_NATIVE_SIZE == V4L2_SEL_TGT_CROP_BOUNDS.

> +
> +Pixel array readable area
> +^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +The V4L2_SEL_TGT_CROP_BOUNDS targets returns size and position of the readable
> +area of the pixel array matrix, including pixels with valid image data and pixel
> +used for calibration purposes, such as optical black pixels. It is not unlikely
> +that valid pixels and optical black pixels are surrounded by non-readable rows
> +and columns of pixels. Those does not concur in the definition of the
> +V4L2_SEL_TGT_CROP_BOUNDS rectangle. The rectangle returned by
> +V4L2_SEL_TGT_CROP_BOUNDS describes an immutable property of the image sensor, it
> +does not change at run-time and cannot be modified from userspace.

Mention that BOUNDS is enclosed by NATIVE_SIZE.

> +
> +Pixel array active area
> +^^^^^^^^^^^^^^^^^^^^^^^
> +
> +The portion of the pixel array which contains valid image data is defined as the
> +active area of the pixel matrix. The active pixel array is is accessed by mean

mean -> means

> +of the V4L2_SEL_TGT_CROP_DEFAULT target, and is contained in the larger
> +V4L2_SEL_TGT_CROP_BOUNDS rectangle. It represents the largest possible frame
> +resolution the sensor can produce and defines the dimension of the full
> +field-of-view. The rectangle returned by V4L2_SEL_TGT_CROP_BOUNDS describes an

BOUNDS -> DEFAULT

> +immutable property of the image sensor, it does not change at run-time and
> +cannot be modified from userspace.

Mention that CROP_DEFAULT is enclosed by CROP_BOUNDS

> +
> +Analog crop rectangle

Why analog? It's just the crop rectangle, nothing analog about it.

> +^^^^^^^^^^^^^^^^^^^^^
> +
> +The sensor driver might decide, in order to adjust the image resolution to best
> +match the one requested by applications, to only process a part of the active
> +pixel array matrix. The selected area is read-out and processed by the image
> +sensor on-board ISP in order to produce images of the desired size and
> +resolution while possible maintaing the largest possible field-of-view. The

maintaing -> maintaining

Actually, I'd drop 'while possible maintaing the largest possible field-of-view'
entirely. It doesn't make much sense.

> +cropped portion of the pixel array which is used to produce images is returned
> +by the V4L2_SEL_TGT_CROP target and represent the only information that can

represent -> represents

> +change at runtime as it depends on the currently configured sensor mode and
> +desired image resolution. If the sub-device driver supports that, userspace
> +can set the analog crop rectangle to select which portion of the pixel array

s/analog//

> +to read out.

Mention that CROP is enclosed by CROP_BOUNDS and defaults to CROP_DEFAULT.

Make a note that CROP can also be used to obtain optical black pixels.

> +
>  
>  Types of selection targets
>  --------------------------
> 

Regards,

	Hans

Re: [PATCH 2/4] media: docs: Describe targets for sensor properties

[Hans Verkuil]

On 05/08/2020 12:57, Jacopo Mondi wrote:
> Provide a table to describe how the V4L2 selection targets can be used
> to access an image sensor pixel array properties.
> 
> Reference the table in the sub-device documentation.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  .../userspace-api/media/v4l/dev-subdev.rst    |  4 ++
>  .../media/v4l/v4l2-selection-targets.rst      | 49 +++++++++++++++++++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> index c47861dff9b9b..2f7da3832f458 100644
> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> @@ -467,6 +467,10 @@ desired image resolution. If the sub-device driver supports that, userspace
>  can set the analog crop rectangle to select which portion of the pixel array
>  to read out.
>  
> +A description of each of the above mentioned targets when used to access the
> +image sensor pixel array properties is provided by
> +:ref:`v4l2-selection-targets-image-sensor-table`
> +
>  
>  Types of selection targets
>  --------------------------
> diff --git a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
> index 69f500093aa2a..632e6448b784e 100644
> --- a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
> +++ b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
> @@ -76,3 +76,52 @@ of the two interfaces they are used.
>  	modified by hardware.
>        - Yes
>        - No
> +
> +
> +.. _v4l2-selection-targets-image-sensor-table:
> +
> +********************************************
> +Selection Targets For Pixel Array Properties
> +********************************************
> +
> +The V4L2 selection API can be used to retrieve the size and disposition of the
> +pixel units that compose and image sensor pixel matrix when applied to a video
> +sub-device that represents an image sensor.
> +
> +A description of the properties associated with each of the sensor pixel array
> +areas is provided by the :ref:`v4l2-subdev-pixel-array-properties` section.
> +
> +.. tabularcolumns:: |p{6.0cm}|p{1.4cm}|p{7.4cm}|p(1.4cm)|
> +
> +.. flat-table:: Selection target definitions
> +    :header-rows:  1
> +    :stub-columns: 0
> +
> +    * - Target name
> +      - id
> +      - Definition
> +      - Read/Write
> +    * - ``V4L2_SEL_TGT_CROP``
> +      - 0x0000
> +      - The analog crop rectangle. Represents the portion of the active pixel
> +        array which is processed to produce images.
> +      - RW
> +    * - ``V4L2_SEL_TGT_CROP_DEFAULT``
> +      - 0x0001
> +      - The active pixel array rectangle. It includes only active pixels and
> +        excludes other ones such as optical black pixels. Its width and height
> +        represent the maximum image resolution an image sensor can produce.
> +      - RO
> +    * - ``V4L2_SEL_TGT_CROP_BOUNDS``
> +      - 0x0002
> +      - The readable portion of the physical pixel array matrix. It includes
> +        pixels that contains valid image data and calibration pixels such as the
> +        optical black ones.
> +      - RO
> +    * - ``V4L2_SEL_TGT_NATIVE_SIZE``
> +      - 0x0003
> +      - The physical pixel array size, including readable and not readable
> +        pixels. As pixels that cannot be read from application processor are not
> +        relevant for calibration purposes, this rectangle is useful to calculate
> +        the physical properties of the image sensor.
> +      - RO
> 

Hmm, this basically just duplicates the previous patch.

I think you are documenting things at the wrong place. What you documented in the
previous patch really belongs here since it is shared between the subdev API and the
regular V4L2 API. And in dev-subdev.rst you then refer to here.

Frankly, the selection API documentation is a mess. It's spread out over various sections:
The VIDIOC_G/S_SELECTION and VIDIOC_SUBDEV_G/S_SELECTION descriptions in the Function Reference,
section 8 ("Common definitions for V4L2 and V4L2 subdev interfaces"), 1.25 "Cropping, composing
and scaling – the SELECTION API" and 4.13.3.2-4.13.3.3 "Selections: cropping, scaling and composition".

In my view section 8 should be moved to section 1.25.2. Ideally 1.25 should be rewritten for both
the V4L2 and V4L2 subdev APIs, but that's a lot of work.

I would suggest that you add a first patch that moves section 8 to 1.25.2. Note that I don't like
the tables for the selection targets and flags: the 'Valid for V4L2/V4L2 subdevs' columns should
be removed and it should either be mentioned in the definition if a target/flag is invalid for
an API, or it should be put in a separate table.

And in 1.25.2 perhaps a new picture can be created for the specific sensor use-case that includes
the NATIVE_SIZE target.

Another pet peeve of mine is that section 8 splits the selection targets and flags into separate
subsections. I'd just keep it in one section.

Regards,

	Hans

Re: [PATCH 3/4] media: docs: USe SUBDEV_G_SELECTION for sensor properties

[Hans Verkuil]

On 05/08/2020 12:57, Jacopo Mondi wrote:
> Add a small note to the VIDIOC_SUBDEV_G_SELECTION IOCTL documentation
> to report that the API can be used to access an image sensor pixel array

an -> the

> properties.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  .../userspace-api/media/v4l/vidioc-subdev-g-selection.rst     | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-selection.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-selection.rst
> index 06c9553ac48f5..05539f5deace2 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-selection.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-selection.rst
> @@ -46,6 +46,10 @@ The selections are used to configure various image processing
>  functionality performed by the subdevs which affect the image size. This
>  currently includes cropping, scaling and composition.
>  
> +This API can also be used to retrieve immutable properties of the device
> +represented by the subdev, such as the pixel matrix properties of an image
> +sensor.
> +
>  The selection API replaces
>  :ref:`the old subdev crop API <VIDIOC_SUBDEV_G_CROP>`. All the
>  function of the crop API, and more, are supported by the selections API.
> 

Re: [PATCH 1/4] media: docs: Describe pixel array properties

[Jacopo Mondi]

Hi Hans,

On Thu, Aug 06, 2020 at 10:05:37AM +0200, Hans Verkuil wrote:
> Hi Jacopo,
>
> Some review comments below:
>
> On 05/08/2020 12:57, Jacopo Mondi wrote:
> > The V4L2 selection API are also used to access the pixel array
>
> are -> is
>
> > properties of an image sensor, such as the size and position of active
> > pixels and the cropped area of the pixel matrix used to produce images.
> >
> > Currently no clear definition of the different areas that compose an
> > image sensor pixel array matrix is provided in the specification, and
> > the actual meaning of each selection target when applied to an image
> > sensor was not provided.
> >
> > Provide in the sub-device documentation the definition of the pixel
> > matrix properties and the selection target associated to each of them.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  .../userspace-api/media/v4l/dev-subdev.rst    | 81 +++++++++++++++++++
> >  1 file changed, 81 insertions(+)
> >
> > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > index 134d2fb909fa4..c47861dff9b9b 100644
> > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.
> >  ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round
> >  the image size either up or down. :ref:`v4l2-selection-flags`
> >
> > +.. _v4l2-subdev-pixel-array-properties:
> > +
> > +Selection targets for image sensors properties
> > +----------------------------------------------
> > +
> > +The V4L2 selection API can be used on sub-devices that represent an image
> > +sensor to retrieve the sensor's pixel array matrix properties by using the
> > +:ref:`selection <VIDIOC_SUBDEV_G_SELECTION>` ioctls.
> > +
> > +Sub-device drivers for image sensor usually register a single source pad, but in
> > +the case they expose more, the pixel array properties can be accessed from
> > +any of them.
> > +
> > +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,
>
> V4L2_SEL_TGT_NATIVE -> V4L2_SEL_TGT_NATIVE_SIZE
>
> (same mistake is made elsewhere).

Ah ups, I used TGT_NATIVE consistently, seems like I thought that was
the right name

>
> > +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve
> > +the immutable properties of the several different areas that compose the sensor
> > +pixel array matrix. Each area describes a rectangle of logically adjacent pixel
> > +units. The logical disposition of pixels is defined by the sensor read-out
> > +starting point and direction, and may differ from the physical disposition of
> > +the pixel units in the pixel array matrix.
> > +
> > +Each pixel matrix portion is contained in a larger rectangle, with the most
> > +largest being the one that describes the pixel matrix physical size. This
> > +defines a hierarchical positional system, where each rectangle is defined
> > +relatively to the largest available one among the ones exposed by the
> > +sub-device driver. Each selection target and the associated pixel array portion
> > +it represents are below presented in order from the largest to the smallest one.
> > +
> > +Pixel array physical size
> > +^^^^^^^^^^^^^^^^^^^^^^^^^
> > +
> > +The image sensor chip is composed by a number of physical pixels, not all of
> > +them readable by the application processor. Invalid or unreadable lines might
> > +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors
> > +they might be tagged with an invalid data type (DT) so that the receiver
> > +automatically discard them. The size of the whole pixel matrix area is
> > +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner
> > +defined as position (0, 0). All the other selection targets are defined
> > +relatively to this, larger, rectangle. The rectangle returned by
> > +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it
> > +does not change at run-time and cannot be modified from userspace.
>
> It is a good idea to mention that if there are no invalid or unreadable pixels/lines,
> then V4L2_SEL_TGT_NATIVE_SIZE == V4L2_SEL_TGT_CROP_BOUNDS.

Yes it is! I'll add it here

>
> > +
> > +Pixel array readable area
> > +^^^^^^^^^^^^^^^^^^^^^^^^^
> > +
> > +The V4L2_SEL_TGT_CROP_BOUNDS targets returns size and position of the readable
> > +area of the pixel array matrix, including pixels with valid image data and pixel
> > +used for calibration purposes, such as optical black pixels. It is not unlikely
> > +that valid pixels and optical black pixels are surrounded by non-readable rows
> > +and columns of pixels. Those does not concur in the definition of the
> > +V4L2_SEL_TGT_CROP_BOUNDS rectangle. The rectangle returned by
> > +V4L2_SEL_TGT_CROP_BOUNDS describes an immutable property of the image sensor, it
> > +does not change at run-time and cannot be modified from userspace.
>
> Mention that BOUNDS is enclosed by NATIVE_SIZE.
>

I tried to express that in the intro section with

"Each pixel matrix portion is contained in a larger rectangle, with the most
largest being the one that describes the pixel matrix physical size."

But I guess it's worth to express that for each target!

> > +
> > +Pixel array active area
> > +^^^^^^^^^^^^^^^^^^^^^^^
> > +
> > +The portion of the pixel array which contains valid image data is defined as the
> > +active area of the pixel matrix. The active pixel array is is accessed by mean
>
> mean -> means
>
> > +of the V4L2_SEL_TGT_CROP_DEFAULT target, and is contained in the larger
> > +V4L2_SEL_TGT_CROP_BOUNDS rectangle. It represents the largest possible frame
> > +resolution the sensor can produce and defines the dimension of the full
> > +field-of-view. The rectangle returned by V4L2_SEL_TGT_CROP_BOUNDS describes an
>
> BOUNDS -> DEFAULT
>

ups

> > +immutable property of the image sensor, it does not change at run-time and
> > +cannot be modified from userspace.
>
> Mention that CROP_DEFAULT is enclosed by CROP_BOUNDS
>
> > +
> > +Analog crop rectangle
>
> Why analog? It's just the crop rectangle, nothing analog about it.
>

We used the 'analogCrop' term in libcamera to differentiate the
cropping which happens on the sensor pixel array matrix to select the
region to process and produce image from. Sensor with an on-board
scaler can perform other cropping steps to implement, in example digital
zoom, so we expect to have a 'digital crop' phase as well. RAW
sensors, in example, will only have an analogCrop rectangle.

Quoting the libcamera definition of analog crop:

 * horizontal and vertical sizes define the portion of the pixel array which
 * is read-out and provided to the sensor's internal processing pipeline, before
 * any pixel sub-sampling method, such as pixel binning, skipping and averaging
 * take place.

should I keep it or remove it ?

> > +^^^^^^^^^^^^^^^^^^^^^
> > +
> > +The sensor driver might decide, in order to adjust the image resolution to best
> > +match the one requested by applications, to only process a part of the active
> > +pixel array matrix. The selected area is read-out and processed by the image
> > +sensor on-board ISP in order to produce images of the desired size and
> > +resolution while possible maintaing the largest possible field-of-view. The
>
> maintaing -> maintaining
>
> Actually, I'd drop 'while possible maintaing the largest possible field-of-view'
> entirely. It doesn't make much sense.

Ack

>
> > +cropped portion of the pixel array which is used to produce images is returned
> > +by the V4L2_SEL_TGT_CROP target and represent the only information that can
>
> represent -> represents
>
> > +change at runtime as it depends on the currently configured sensor mode and
> > +desired image resolution. If the sub-device driver supports that, userspace
> > +can set the analog crop rectangle to select which portion of the pixel array
>
> s/analog//
>
> > +to read out.
>
> Mention that CROP is enclosed by CROP_BOUNDS and defaults to CROP_DEFAULT.
>
> Make a note that CROP can also be used to obtain optical black pixels.
>

What about:

+desired image resolution. If the sub-device driver supports that, userspace
+can set the analog crop rectangle to select which portion of the pixel array
+to read out including, if supported, optical black pixels.

?

Thanks
  j

> > +
> >
> >  Types of selection targets
> >  --------------------------
> >
>
> Regards,
>
> 	Hans

Re: [PATCH 1/4] media: docs: Describe pixel array properties

[Hans Verkuil]

On 06/08/2020 11:50, Jacopo Mondi wrote:
> Hi Hans,
> 
> On Thu, Aug 06, 2020 at 10:05:37AM +0200, Hans Verkuil wrote:
>> Hi Jacopo,
>>
>> Some review comments below:
>>
>> On 05/08/2020 12:57, Jacopo Mondi wrote:
>>> The V4L2 selection API are also used to access the pixel array
>>
>> are -> is
>>
>>> properties of an image sensor, such as the size and position of active
>>> pixels and the cropped area of the pixel matrix used to produce images.
>>>
>>> Currently no clear definition of the different areas that compose an
>>> image sensor pixel array matrix is provided in the specification, and
>>> the actual meaning of each selection target when applied to an image
>>> sensor was not provided.
>>>
>>> Provide in the sub-device documentation the definition of the pixel
>>> matrix properties and the selection target associated to each of them.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>> ---
>>>  .../userspace-api/media/v4l/dev-subdev.rst    | 81 +++++++++++++++++++
>>>  1 file changed, 81 insertions(+)
>>>
>>> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
>>> index 134d2fb909fa4..c47861dff9b9b 100644
>>> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
>>> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
>>> @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.
>>>  ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round
>>>  the image size either up or down. :ref:`v4l2-selection-flags`
>>>
>>> +.. _v4l2-subdev-pixel-array-properties:
>>> +
>>> +Selection targets for image sensors properties
>>> +----------------------------------------------
>>> +
>>> +The V4L2 selection API can be used on sub-devices that represent an image
>>> +sensor to retrieve the sensor's pixel array matrix properties by using the
>>> +:ref:`selection <VIDIOC_SUBDEV_G_SELECTION>` ioctls.
>>> +
>>> +Sub-device drivers for image sensor usually register a single source pad, but in
>>> +the case they expose more, the pixel array properties can be accessed from
>>> +any of them.
>>> +
>>> +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,
>>
>> V4L2_SEL_TGT_NATIVE -> V4L2_SEL_TGT_NATIVE_SIZE
>>
>> (same mistake is made elsewhere).
> 
> Ah ups, I used TGT_NATIVE consistently, seems like I thought that was
> the right name
> 
>>
>>> +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve
>>> +the immutable properties of the several different areas that compose the sensor
>>> +pixel array matrix. Each area describes a rectangle of logically adjacent pixel
>>> +units. The logical disposition of pixels is defined by the sensor read-out
>>> +starting point and direction, and may differ from the physical disposition of
>>> +the pixel units in the pixel array matrix.
>>> +
>>> +Each pixel matrix portion is contained in a larger rectangle, with the most
>>> +largest being the one that describes the pixel matrix physical size. This
>>> +defines a hierarchical positional system, where each rectangle is defined
>>> +relatively to the largest available one among the ones exposed by the
>>> +sub-device driver. Each selection target and the associated pixel array portion
>>> +it represents are below presented in order from the largest to the smallest one.
>>> +
>>> +Pixel array physical size
>>> +^^^^^^^^^^^^^^^^^^^^^^^^^
>>> +
>>> +The image sensor chip is composed by a number of physical pixels, not all of
>>> +them readable by the application processor. Invalid or unreadable lines might
>>> +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors
>>> +they might be tagged with an invalid data type (DT) so that the receiver
>>> +automatically discard them. The size of the whole pixel matrix area is
>>> +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner
>>> +defined as position (0, 0). All the other selection targets are defined
>>> +relatively to this, larger, rectangle. The rectangle returned by
>>> +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it
>>> +does not change at run-time and cannot be modified from userspace.
>>
>> It is a good idea to mention that if there are no invalid or unreadable pixels/lines,
>> then V4L2_SEL_TGT_NATIVE_SIZE == V4L2_SEL_TGT_CROP_BOUNDS.
> 
> Yes it is! I'll add it here
> 
>>
>>> +
>>> +Pixel array readable area
>>> +^^^^^^^^^^^^^^^^^^^^^^^^^
>>> +
>>> +The V4L2_SEL_TGT_CROP_BOUNDS targets returns size and position of the readable
>>> +area of the pixel array matrix, including pixels with valid image data and pixel
>>> +used for calibration purposes, such as optical black pixels. It is not unlikely
>>> +that valid pixels and optical black pixels are surrounded by non-readable rows
>>> +and columns of pixels. Those does not concur in the definition of the
>>> +V4L2_SEL_TGT_CROP_BOUNDS rectangle. The rectangle returned by
>>> +V4L2_SEL_TGT_CROP_BOUNDS describes an immutable property of the image sensor, it
>>> +does not change at run-time and cannot be modified from userspace.
>>
>> Mention that BOUNDS is enclosed by NATIVE_SIZE.
>>
> 
> I tried to express that in the intro section with
> 
> "Each pixel matrix portion is contained in a larger rectangle, with the most
> largest being the one that describes the pixel matrix physical size."
> 
> But I guess it's worth to express that for each target!
> 
>>> +
>>> +Pixel array active area
>>> +^^^^^^^^^^^^^^^^^^^^^^^
>>> +
>>> +The portion of the pixel array which contains valid image data is defined as the
>>> +active area of the pixel matrix. The active pixel array is is accessed by mean
>>
>> mean -> means
>>
>>> +of the V4L2_SEL_TGT_CROP_DEFAULT target, and is contained in the larger
>>> +V4L2_SEL_TGT_CROP_BOUNDS rectangle. It represents the largest possible frame
>>> +resolution the sensor can produce and defines the dimension of the full
>>> +field-of-view. The rectangle returned by V4L2_SEL_TGT_CROP_BOUNDS describes an
>>
>> BOUNDS -> DEFAULT
>>
> 
> ups
> 
>>> +immutable property of the image sensor, it does not change at run-time and
>>> +cannot be modified from userspace.
>>
>> Mention that CROP_DEFAULT is enclosed by CROP_BOUNDS
>>
>>> +
>>> +Analog crop rectangle
>>
>> Why analog? It's just the crop rectangle, nothing analog about it.
>>
> 
> We used the 'analogCrop' term in libcamera to differentiate the
> cropping which happens on the sensor pixel array matrix to select the
> region to process and produce image from. Sensor with an on-board
> scaler can perform other cropping steps to implement, in example digital
> zoom, so we expect to have a 'digital crop' phase as well. RAW
> sensors, in example, will only have an analogCrop rectangle.
> 
> Quoting the libcamera definition of analog crop:
> 
>  * horizontal and vertical sizes define the portion of the pixel array which
>  * is read-out and provided to the sensor's internal processing pipeline, before
>  * any pixel sub-sampling method, such as pixel binning, skipping and averaging
>  * take place.
> 
> should I keep it or remove it ?

It's a very confusing term. Especially since this API can also be used with analog
video capture devices (Composite/S-Video) where the video signal actually is analog.

In the V4L2 API there is no such thing as 'analog crop', so please remove it.

> 
>>> +^^^^^^^^^^^^^^^^^^^^^
>>> +
>>> +The sensor driver might decide, in order to adjust the image resolution to best
>>> +match the one requested by applications, to only process a part of the active
>>> +pixel array matrix. The selected area is read-out and processed by the image
>>> +sensor on-board ISP in order to produce images of the desired size and
>>> +resolution while possible maintaing the largest possible field-of-view. The
>>
>> maintaing -> maintaining
>>
>> Actually, I'd drop 'while possible maintaing the largest possible field-of-view'
>> entirely. It doesn't make much sense.
> 
> Ack
> 
>>
>>> +cropped portion of the pixel array which is used to produce images is returned
>>> +by the V4L2_SEL_TGT_CROP target and represent the only information that can
>>
>> represent -> represents
>>
>>> +change at runtime as it depends on the currently configured sensor mode and
>>> +desired image resolution. If the sub-device driver supports that, userspace
>>> +can set the analog crop rectangle to select which portion of the pixel array
>>
>> s/analog//
>>
>>> +to read out.
>>
>> Mention that CROP is enclosed by CROP_BOUNDS and defaults to CROP_DEFAULT.
>>
>> Make a note that CROP can also be used to obtain optical black pixels.
>>
> 
> What about:
> 
> +desired image resolution. If the sub-device driver supports that, userspace
> +can set the analog crop rectangle to select which portion of the pixel array
> +to read out including, if supported, optical black pixels.

Hmm, that's a bit awkward. How about:

+desired image resolution. If supported by the sub-device driver, userspace
+can set the crop rectangle to select which portion of the pixel array
+to read out. This may include optical black pixels if those are part of
+V4L2_SEL_TGT_CROP_BOUNDS.

Regards,

	Hans

> 
> ?
> 
> Thanks
>   j
> 
>>> +
>>>
>>>  Types of selection targets
>>>  --------------------------
>>>
>>
>> Regards,
>>
>> 	Hans

Re: [PATCH 2/4] media: docs: Describe targets for sensor properties

[Jacopo Mondi]

Hi Hans,

On Thu, Aug 06, 2020 at 10:45:17AM +0200, Hans Verkuil wrote:
> On 05/08/2020 12:57, Jacopo Mondi wrote:
> > Provide a table to describe how the V4L2 selection targets can be used
> > to access an image sensor pixel array properties.
> >
> > Reference the table in the sub-device documentation.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  .../userspace-api/media/v4l/dev-subdev.rst    |  4 ++
> >  .../media/v4l/v4l2-selection-targets.rst      | 49 +++++++++++++++++++
> >  2 files changed, 53 insertions(+)
> >
> > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > index c47861dff9b9b..2f7da3832f458 100644
> > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > @@ -467,6 +467,10 @@ desired image resolution. If the sub-device driver supports that, userspace
> >  can set the analog crop rectangle to select which portion of the pixel array
> >  to read out.
> >
> > +A description of each of the above mentioned targets when used to access the
> > +image sensor pixel array properties is provided by
> > +:ref:`v4l2-selection-targets-image-sensor-table`
> > +
> >
> >  Types of selection targets
> >  --------------------------
> > diff --git a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
> > index 69f500093aa2a..632e6448b784e 100644
> > --- a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
> > +++ b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
> > @@ -76,3 +76,52 @@ of the two interfaces they are used.
> >  	modified by hardware.
> >        - Yes
> >        - No
> > +
> > +
> > +.. _v4l2-selection-targets-image-sensor-table:
> > +
> > +********************************************
> > +Selection Targets For Pixel Array Properties
> > +********************************************
> > +
> > +The V4L2 selection API can be used to retrieve the size and disposition of the
> > +pixel units that compose and image sensor pixel matrix when applied to a video
> > +sub-device that represents an image sensor.
> > +
> > +A description of the properties associated with each of the sensor pixel array
> > +areas is provided by the :ref:`v4l2-subdev-pixel-array-properties` section.
> > +
> > +.. tabularcolumns:: |p{6.0cm}|p{1.4cm}|p{7.4cm}|p(1.4cm)|
> > +
> > +.. flat-table:: Selection target definitions
> > +    :header-rows:  1
> > +    :stub-columns: 0
> > +
> > +    * - Target name
> > +      - id
> > +      - Definition
> > +      - Read/Write
> > +    * - ``V4L2_SEL_TGT_CROP``
> > +      - 0x0000
> > +      - The analog crop rectangle. Represents the portion of the active pixel
> > +        array which is processed to produce images.
> > +      - RW
> > +    * - ``V4L2_SEL_TGT_CROP_DEFAULT``
> > +      - 0x0001
> > +      - The active pixel array rectangle. It includes only active pixels and
> > +        excludes other ones such as optical black pixels. Its width and height
> > +        represent the maximum image resolution an image sensor can produce.
> > +      - RO
> > +    * - ``V4L2_SEL_TGT_CROP_BOUNDS``
> > +      - 0x0002
> > +      - The readable portion of the physical pixel array matrix. It includes
> > +        pixels that contains valid image data and calibration pixels such as the
> > +        optical black ones.
> > +      - RO
> > +    * - ``V4L2_SEL_TGT_NATIVE_SIZE``
> > +      - 0x0003
> > +      - The physical pixel array size, including readable and not readable
> > +        pixels. As pixels that cannot be read from application processor are not
> > +        relevant for calibration purposes, this rectangle is useful to calculate
> > +        the physical properties of the image sensor.
> > +      - RO
> >
>
> Hmm, this basically just duplicates the previous patch.
>
> I think you are documenting things at the wrong place. What you documented in the
> previous patch really belongs here since it is shared between the subdev API and the
> regular V4L2 API. And in dev-subdev.rst you then refer to here.

I originally had it here, but then I moved to dev-subdev as an image
sensor will always be represented as a video sub-device, doen't it ?

>
> Frankly, the selection API documentation is a mess. It's spread out over various sections:

I know :(

> The VIDIOC_G/S_SELECTION and VIDIOC_SUBDEV_G/S_SELECTION descriptions in the Function Reference,
> section 8 ("Common definitions for V4L2 and V4L2 subdev interfaces"), 1.25 "Cropping, composing
> and scaling – the SELECTION API" and 4.13.3.2-4.13.3.3 "Selections: cropping, scaling and composition".
>
> In my view section 8 should be moved to section 1.25.2. Ideally 1.25 should be rewritten for both
> the V4L2 and V4L2 subdev APIs, but that's a lot of work.

That's a lot of work indeed. But it could be split as

1) Augment the 1.25.1 section with a mention of subdev, maybe using
   pieces of 4.13.3.2
2) Move what's in section 8 to be 1.25.x
   Actually merging 1.25.2 and 8.1.1, splitting 1.25.2 in a device and
   sub-device section
>
> I would suggest that you add a first patch that moves section 8 to 1.25.2. Note that I don't like
> the tables for the selection targets and flags: the 'Valid for V4L2/V4L2 subdevs' columns should
> be removed and it should either be mentioned in the definition if a target/flag is invalid for
> an API, or it should be put in a separate table.

Two tables seems about right

>
> And in 1.25.2 perhaps a new picture can be created for the specific sensor use-case that includes
> the NATIVE_SIZE target.

I know 'image sensor' is not an API concept like device and subdevice,
but if we split 1.25.2 in 'video device' and 'subdevice' parts, a
section for 'image sensors' with what I've now put in section 4.13.3.3
(the description of the sensor array areas) could fit there ?

Seems like a long re-work. Can the imx219 patch be broke out and
fast-tracked ?

Thanks
  j

>
> Another pet peeve of mine is that section 8 splits the selection targets and flags into separate
> subsections. I'd just keep it in one section.
>
> Regards,
>
> 	Hans

Re: [PATCH 2/4] media: docs: Describe targets for sensor properties

[Hans Verkuil]

On 06/08/2020 12:08, Jacopo Mondi wrote:
> Hi Hans,
> 
> On Thu, Aug 06, 2020 at 10:45:17AM +0200, Hans Verkuil wrote:
>> On 05/08/2020 12:57, Jacopo Mondi wrote:
>>> Provide a table to describe how the V4L2 selection targets can be used
>>> to access an image sensor pixel array properties.
>>>
>>> Reference the table in the sub-device documentation.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>> ---
>>>  .../userspace-api/media/v4l/dev-subdev.rst    |  4 ++
>>>  .../media/v4l/v4l2-selection-targets.rst      | 49 +++++++++++++++++++
>>>  2 files changed, 53 insertions(+)
>>>
>>> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
>>> index c47861dff9b9b..2f7da3832f458 100644
>>> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
>>> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
>>> @@ -467,6 +467,10 @@ desired image resolution. If the sub-device driver supports that, userspace
>>>  can set the analog crop rectangle to select which portion of the pixel array
>>>  to read out.
>>>
>>> +A description of each of the above mentioned targets when used to access the
>>> +image sensor pixel array properties is provided by
>>> +:ref:`v4l2-selection-targets-image-sensor-table`
>>> +
>>>
>>>  Types of selection targets
>>>  --------------------------
>>> diff --git a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
>>> index 69f500093aa2a..632e6448b784e 100644
>>> --- a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
>>> +++ b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
>>> @@ -76,3 +76,52 @@ of the two interfaces they are used.
>>>  	modified by hardware.
>>>        - Yes
>>>        - No
>>> +
>>> +
>>> +.. _v4l2-selection-targets-image-sensor-table:
>>> +
>>> +********************************************
>>> +Selection Targets For Pixel Array Properties
>>> +********************************************
>>> +
>>> +The V4L2 selection API can be used to retrieve the size and disposition of the
>>> +pixel units that compose and image sensor pixel matrix when applied to a video
>>> +sub-device that represents an image sensor.
>>> +
>>> +A description of the properties associated with each of the sensor pixel array
>>> +areas is provided by the :ref:`v4l2-subdev-pixel-array-properties` section.
>>> +
>>> +.. tabularcolumns:: |p{6.0cm}|p{1.4cm}|p{7.4cm}|p(1.4cm)|
>>> +
>>> +.. flat-table:: Selection target definitions
>>> +    :header-rows:  1
>>> +    :stub-columns: 0
>>> +
>>> +    * - Target name
>>> +      - id
>>> +      - Definition
>>> +      - Read/Write
>>> +    * - ``V4L2_SEL_TGT_CROP``
>>> +      - 0x0000
>>> +      - The analog crop rectangle. Represents the portion of the active pixel
>>> +        array which is processed to produce images.
>>> +      - RW
>>> +    * - ``V4L2_SEL_TGT_CROP_DEFAULT``
>>> +      - 0x0001
>>> +      - The active pixel array rectangle. It includes only active pixels and
>>> +        excludes other ones such as optical black pixels. Its width and height
>>> +        represent the maximum image resolution an image sensor can produce.
>>> +      - RO
>>> +    * - ``V4L2_SEL_TGT_CROP_BOUNDS``
>>> +      - 0x0002
>>> +      - The readable portion of the physical pixel array matrix. It includes
>>> +        pixels that contains valid image data and calibration pixels such as the
>>> +        optical black ones.
>>> +      - RO
>>> +    * - ``V4L2_SEL_TGT_NATIVE_SIZE``
>>> +      - 0x0003
>>> +      - The physical pixel array size, including readable and not readable
>>> +        pixels. As pixels that cannot be read from application processor are not
>>> +        relevant for calibration purposes, this rectangle is useful to calculate
>>> +        the physical properties of the image sensor.
>>> +      - RO
>>>
>>
>> Hmm, this basically just duplicates the previous patch.
>>
>> I think you are documenting things at the wrong place. What you documented in the
>> previous patch really belongs here since it is shared between the subdev API and the
>> regular V4L2 API. And in dev-subdev.rst you then refer to here.
> 
> I originally had it here, but then I moved to dev-subdev as an image
> sensor will always be represented as a video sub-device, doen't it ?

No. Some camera drivers are V4L2 only, most notably uvc. Also there are several simple
platform drivers that don't use the subdev API.

> 
>>
>> Frankly, the selection API documentation is a mess. It's spread out over various sections:
> 
> I know :(
> 
>> The VIDIOC_G/S_SELECTION and VIDIOC_SUBDEV_G/S_SELECTION descriptions in the Function Reference,
>> section 8 ("Common definitions for V4L2 and V4L2 subdev interfaces"), 1.25 "Cropping, composing
>> and scaling – the SELECTION API" and 4.13.3.2-4.13.3.3 "Selections: cropping, scaling and composition".
>>
>> In my view section 8 should be moved to section 1.25.2. Ideally 1.25 should be rewritten for both
>> the V4L2 and V4L2 subdev APIs, but that's a lot of work.
> 
> That's a lot of work indeed. But it could be split as
> 
> 1) Augment the 1.25.1 section with a mention of subdev, maybe using
>    pieces of 4.13.3.2
> 2) Move what's in section 8 to be 1.25.x
>    Actually merging 1.25.2 and 8.1.1, splitting 1.25.2 in a device and
>    sub-device section
>>
>> I would suggest that you add a first patch that moves section 8 to 1.25.2. Note that I don't like
>> the tables for the selection targets and flags: the 'Valid for V4L2/V4L2 subdevs' columns should
>> be removed and it should either be mentioned in the definition if a target/flag is invalid for
>> an API, or it should be put in a separate table.
> 
> Two tables seems about right
> 
>>
>> And in 1.25.2 perhaps a new picture can be created for the specific sensor use-case that includes
>> the NATIVE_SIZE target.
> 
> I know 'image sensor' is not an API concept like device and subdevice,
> but if we split 1.25.2 in 'video device' and 'subdevice' parts, a
> section for 'image sensors' with what I've now put in section 4.13.3.3
> (the description of the sensor array areas) could fit there ?

I think so.

> 
> Seems like a long re-work. Can the imx219 patch be broke out and
> fast-tracked ?

Yes.

Regards,

	Hans

> 
> Thanks
>   j
> 
>>
>> Another pet peeve of mine is that section 8 splits the selection targets and flags into separate
>> subsections. I'd just keep it in one section.
>>
>> Regards,
>>
>> 	Hans

Re: [PATCH 2/4] media: docs: Describe targets for sensor properties

[Jacopo Mondi]

Hi Hans,

On Thu, Aug 06, 2020 at 12:15:49PM +0200, Hans Verkuil wrote:
> On 06/08/2020 12:08, Jacopo Mondi wrote:
> > Hi Hans,
> >
> > On Thu, Aug 06, 2020 at 10:45:17AM +0200, Hans Verkuil wrote:
> >> On 05/08/2020 12:57, Jacopo Mondi wrote:
> >>> Provide a table to describe how the V4L2 selection targets can be used
> >>> to access an image sensor pixel array properties.
> >>>
> >>> Reference the table in the sub-device documentation.
> >>>
> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>> ---
> >>>  .../userspace-api/media/v4l/dev-subdev.rst    |  4 ++
> >>>  .../media/v4l/v4l2-selection-targets.rst      | 49 +++++++++++++++++++
> >>>  2 files changed, 53 insertions(+)
> >>>
> >>> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> >>> index c47861dff9b9b..2f7da3832f458 100644
> >>> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
> >>> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> >>> @@ -467,6 +467,10 @@ desired image resolution. If the sub-device driver supports that, userspace
> >>>  can set the analog crop rectangle to select which portion of the pixel array
> >>>  to read out.
> >>>
> >>> +A description of each of the above mentioned targets when used to access the
> >>> +image sensor pixel array properties is provided by
> >>> +:ref:`v4l2-selection-targets-image-sensor-table`
> >>> +
> >>>
> >>>  Types of selection targets
> >>>  --------------------------
> >>> diff --git a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
> >>> index 69f500093aa2a..632e6448b784e 100644
> >>> --- a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
> >>> +++ b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
> >>> @@ -76,3 +76,52 @@ of the two interfaces they are used.
> >>>  	modified by hardware.
> >>>        - Yes
> >>>        - No
> >>> +
> >>> +
> >>> +.. _v4l2-selection-targets-image-sensor-table:
> >>> +
> >>> +********************************************
> >>> +Selection Targets For Pixel Array Properties
> >>> +********************************************
> >>> +
> >>> +The V4L2 selection API can be used to retrieve the size and disposition of the
> >>> +pixel units that compose and image sensor pixel matrix when applied to a video
> >>> +sub-device that represents an image sensor.
> >>> +
> >>> +A description of the properties associated with each of the sensor pixel array
> >>> +areas is provided by the :ref:`v4l2-subdev-pixel-array-properties` section.
> >>> +
> >>> +.. tabularcolumns:: |p{6.0cm}|p{1.4cm}|p{7.4cm}|p(1.4cm)|
> >>> +
> >>> +.. flat-table:: Selection target definitions
> >>> +    :header-rows:  1
> >>> +    :stub-columns: 0
> >>> +
> >>> +    * - Target name
> >>> +      - id
> >>> +      - Definition
> >>> +      - Read/Write
> >>> +    * - ``V4L2_SEL_TGT_CROP``
> >>> +      - 0x0000
> >>> +      - The analog crop rectangle. Represents the portion of the active pixel
> >>> +        array which is processed to produce images.
> >>> +      - RW
> >>> +    * - ``V4L2_SEL_TGT_CROP_DEFAULT``
> >>> +      - 0x0001
> >>> +      - The active pixel array rectangle. It includes only active pixels and
> >>> +        excludes other ones such as optical black pixels. Its width and height
> >>> +        represent the maximum image resolution an image sensor can produce.
> >>> +      - RO
> >>> +    * - ``V4L2_SEL_TGT_CROP_BOUNDS``
> >>> +      - 0x0002
> >>> +      - The readable portion of the physical pixel array matrix. It includes
> >>> +        pixels that contains valid image data and calibration pixels such as the
> >>> +        optical black ones.
> >>> +      - RO
> >>> +    * - ``V4L2_SEL_TGT_NATIVE_SIZE``
> >>> +      - 0x0003
> >>> +      - The physical pixel array size, including readable and not readable
> >>> +        pixels. As pixels that cannot be read from application processor are not
> >>> +        relevant for calibration purposes, this rectangle is useful to calculate
> >>> +        the physical properties of the image sensor.
> >>> +      - RO
> >>>
> >>
> >> Hmm, this basically just duplicates the previous patch.
> >>
> >> I think you are documenting things at the wrong place. What you documented in the
> >> previous patch really belongs here since it is shared between the subdev API and the
> >> regular V4L2 API. And in dev-subdev.rst you then refer to here.
> >
> > I originally had it here, but then I moved to dev-subdev as an image
> > sensor will always be represented as a video sub-device, doen't it ?
>
> No. Some camera drivers are V4L2 only, most notably uvc. Also there are several simple
> platform drivers that don't use the subdev API.

Do we expect to be able to retrieve sensor array properties from video
device nodes which represents, in my understanding a DMA engine that
writes data to memory ? As I see it, not subdev for the image sensor,
no pixel array properties. How can these be exposed by a video device
which abstracts the full capture pipeline ?

Re: [PATCH 1/4] media: docs: Describe pixel array properties

[Sakari Ailus]

Hi Hans,

On Thu, Aug 06, 2020 at 11:58:31AM +0200, Hans Verkuil wrote:
> On 06/08/2020 11:50, Jacopo Mondi wrote:
> > Hi Hans,
> > 
> > On Thu, Aug 06, 2020 at 10:05:37AM +0200, Hans Verkuil wrote:
> >> Hi Jacopo,
> >>
> >> Some review comments below:
> >>
> >> On 05/08/2020 12:57, Jacopo Mondi wrote:
> >>> +Analog crop rectangle
> >>
> >> Why analog? It's just the crop rectangle, nothing analog about it.
> >>
> > 
> > We used the 'analogCrop' term in libcamera to differentiate the
> > cropping which happens on the sensor pixel array matrix to select the
> > region to process and produce image from. Sensor with an on-board
> > scaler can perform other cropping steps to implement, in example digital
> > zoom, so we expect to have a 'digital crop' phase as well. RAW
> > sensors, in example, will only have an analogCrop rectangle.
> > 
> > Quoting the libcamera definition of analog crop:
> > 
> >  * horizontal and vertical sizes define the portion of the pixel array which
> >  * is read-out and provided to the sensor's internal processing pipeline, before
> >  * any pixel sub-sampling method, such as pixel binning, skipping and averaging
> >  * take place.
> > 
> > should I keep it or remove it ?
> 
> It's a very confusing term. Especially since this API can also be used with analog
> video capture devices (Composite/S-Video) where the video signal actually is analog.
> 
> In the V4L2 API there is no such thing as 'analog crop', so please remove it.

There isn't in the V4L2 API but I don't see why we couldn't document it
here. Analogue crop is an established term related to raw (perhaps others,
too?) camera sensors which describes cropping that is implemented by not
reading parts of the pixel array.

As this documentation only applies to camera sensors, I think it's entirely
appropriate to document this here, and using that term.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 2/4] media: docs: Describe targets for sensor properties

[Hans Verkuil]

On 06/08/2020 14:45, Jacopo Mondi wrote:
> Hi Hans,
> 
> On Thu, Aug 06, 2020 at 12:15:49PM +0200, Hans Verkuil wrote:
>> On 06/08/2020 12:08, Jacopo Mondi wrote:
>>> Hi Hans,
>>>
>>> On Thu, Aug 06, 2020 at 10:45:17AM +0200, Hans Verkuil wrote:
>>>> On 05/08/2020 12:57, Jacopo Mondi wrote:
>>>>> Provide a table to describe how the V4L2 selection targets can be used
>>>>> to access an image sensor pixel array properties.
>>>>>
>>>>> Reference the table in the sub-device documentation.
>>>>>
>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>>>> ---
>>>>>  .../userspace-api/media/v4l/dev-subdev.rst    |  4 ++
>>>>>  .../media/v4l/v4l2-selection-targets.rst      | 49 +++++++++++++++++++
>>>>>  2 files changed, 53 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
>>>>> index c47861dff9b9b..2f7da3832f458 100644
>>>>> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
>>>>> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
>>>>> @@ -467,6 +467,10 @@ desired image resolution. If the sub-device driver supports that, userspace
>>>>>  can set the analog crop rectangle to select which portion of the pixel array
>>>>>  to read out.
>>>>>
>>>>> +A description of each of the above mentioned targets when used to access the
>>>>> +image sensor pixel array properties is provided by
>>>>> +:ref:`v4l2-selection-targets-image-sensor-table`
>>>>> +
>>>>>
>>>>>  Types of selection targets
>>>>>  --------------------------
>>>>> diff --git a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
>>>>> index 69f500093aa2a..632e6448b784e 100644
>>>>> --- a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
>>>>> +++ b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
>>>>> @@ -76,3 +76,52 @@ of the two interfaces they are used.
>>>>>  	modified by hardware.
>>>>>        - Yes
>>>>>        - No
>>>>> +
>>>>> +
>>>>> +.. _v4l2-selection-targets-image-sensor-table:
>>>>> +
>>>>> +********************************************
>>>>> +Selection Targets For Pixel Array Properties
>>>>> +********************************************
>>>>> +
>>>>> +The V4L2 selection API can be used to retrieve the size and disposition of the
>>>>> +pixel units that compose and image sensor pixel matrix when applied to a video
>>>>> +sub-device that represents an image sensor.
>>>>> +
>>>>> +A description of the properties associated with each of the sensor pixel array
>>>>> +areas is provided by the :ref:`v4l2-subdev-pixel-array-properties` section.
>>>>> +
>>>>> +.. tabularcolumns:: |p{6.0cm}|p{1.4cm}|p{7.4cm}|p(1.4cm)|
>>>>> +
>>>>> +.. flat-table:: Selection target definitions
>>>>> +    :header-rows:  1
>>>>> +    :stub-columns: 0
>>>>> +
>>>>> +    * - Target name
>>>>> +      - id
>>>>> +      - Definition
>>>>> +      - Read/Write
>>>>> +    * - ``V4L2_SEL_TGT_CROP``
>>>>> +      - 0x0000
>>>>> +      - The analog crop rectangle. Represents the portion of the active pixel
>>>>> +        array which is processed to produce images.
>>>>> +      - RW
>>>>> +    * - ``V4L2_SEL_TGT_CROP_DEFAULT``
>>>>> +      - 0x0001
>>>>> +      - The active pixel array rectangle. It includes only active pixels and
>>>>> +        excludes other ones such as optical black pixels. Its width and height
>>>>> +        represent the maximum image resolution an image sensor can produce.
>>>>> +      - RO
>>>>> +    * - ``V4L2_SEL_TGT_CROP_BOUNDS``
>>>>> +      - 0x0002
>>>>> +      - The readable portion of the physical pixel array matrix. It includes
>>>>> +        pixels that contains valid image data and calibration pixels such as the
>>>>> +        optical black ones.
>>>>> +      - RO
>>>>> +    * - ``V4L2_SEL_TGT_NATIVE_SIZE``
>>>>> +      - 0x0003
>>>>> +      - The physical pixel array size, including readable and not readable
>>>>> +        pixels. As pixels that cannot be read from application processor are not
>>>>> +        relevant for calibration purposes, this rectangle is useful to calculate
>>>>> +        the physical properties of the image sensor.
>>>>> +      - RO
>>>>>
>>>>
>>>> Hmm, this basically just duplicates the previous patch.
>>>>
>>>> I think you are documenting things at the wrong place. What you documented in the
>>>> previous patch really belongs here since it is shared between the subdev API and the
>>>> regular V4L2 API. And in dev-subdev.rst you then refer to here.
>>>
>>> I originally had it here, but then I moved to dev-subdev as an image
>>> sensor will always be represented as a video sub-device, doen't it ?
>>
>> No. Some camera drivers are V4L2 only, most notably uvc. Also there are several simple
>> platform drivers that don't use the subdev API.
> 
> Do we expect to be able to retrieve sensor array properties from video
> device nodes which represents, in my understanding a DMA engine that
> writes data to memory ? As I see it, not subdev for the image sensor,
> no pixel array properties. How can these be exposed by a video device
> which abstracts the full capture pipeline ?

They will typically ask the subdev driver. The vidioc_g_selection op
implementation will in turn call the get_selection op of the sensor subdev
driver and pass that information back to userspace.

There is nothing subdev specific to this API.

Regards,

	Hans

Re: [PATCH 1/4] media: docs: Describe pixel array properties

[Hans Verkuil]

On 06/08/2020 14:54, Sakari Ailus wrote:
> Hi Hans,
> 
> On Thu, Aug 06, 2020 at 11:58:31AM +0200, Hans Verkuil wrote:
>> On 06/08/2020 11:50, Jacopo Mondi wrote:
>>> Hi Hans,
>>>
>>> On Thu, Aug 06, 2020 at 10:05:37AM +0200, Hans Verkuil wrote:
>>>> Hi Jacopo,
>>>>
>>>> Some review comments below:
>>>>
>>>> On 05/08/2020 12:57, Jacopo Mondi wrote:
>>>>> +Analog crop rectangle
>>>>
>>>> Why analog? It's just the crop rectangle, nothing analog about it.
>>>>
>>>
>>> We used the 'analogCrop' term in libcamera to differentiate the
>>> cropping which happens on the sensor pixel array matrix to select the
>>> region to process and produce image from. Sensor with an on-board
>>> scaler can perform other cropping steps to implement, in example digital
>>> zoom, so we expect to have a 'digital crop' phase as well. RAW
>>> sensors, in example, will only have an analogCrop rectangle.
>>>
>>> Quoting the libcamera definition of analog crop:
>>>
>>>  * horizontal and vertical sizes define the portion of the pixel array which
>>>  * is read-out and provided to the sensor's internal processing pipeline, before
>>>  * any pixel sub-sampling method, such as pixel binning, skipping and averaging
>>>  * take place.
>>>
>>> should I keep it or remove it ?
>>
>> It's a very confusing term. Especially since this API can also be used with analog
>> video capture devices (Composite/S-Video) where the video signal actually is analog.
>>
>> In the V4L2 API there is no such thing as 'analog crop', so please remove it.
> 
> There isn't in the V4L2 API but I don't see why we couldn't document it
> here. Analogue crop is an established term related to raw (perhaps others,
> too?) camera sensors which describes cropping that is implemented by not
> reading parts of the pixel array.
> 
> As this documentation only applies to camera sensors, I think it's entirely
> appropriate to document this here, and using that term.
> 

It's always been called just 'crop' in the V4L2 API, so renaming it suddenly
to something else is IMHO confusing. What you can do, however, is that in the
description of the "crop rectangle" you mention that "it is also known as
"analog crop" in the context of camera sensors.

With perhaps some more extensive explanation of the term.

Regards,

	Hans

Re: [PATCH 2/4] media: docs: Describe targets for sensor properties

[Jacopo Mondi]

Hi Hans,
   I'm sorry, I don't want to insist but..

On Thu, Aug 06, 2020 at 03:15:54PM +0200, Hans Verkuil wrote:
> On 06/08/2020 14:45, Jacopo Mondi wrote:
> > Hi Hans,
> >
> > On Thu, Aug 06, 2020 at 12:15:49PM +0200, Hans Verkuil wrote:
> >> On 06/08/2020 12:08, Jacopo Mondi wrote:
> >>> Hi Hans,
> >>>
> >>> On Thu, Aug 06, 2020 at 10:45:17AM +0200, Hans Verkuil wrote:
> >>>> On 05/08/2020 12:57, Jacopo Mondi wrote:
> >>>>> Provide a table to describe how the V4L2 selection targets can be used
> >>>>> to access an image sensor pixel array properties.
> >>>>>
> >>>>> Reference the table in the sub-device documentation.
> >>>>>
> >>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>>>> ---
> >>>>>  .../userspace-api/media/v4l/dev-subdev.rst    |  4 ++
> >>>>>  .../media/v4l/v4l2-selection-targets.rst      | 49 +++++++++++++++++++
> >>>>>  2 files changed, 53 insertions(+)
> >>>>>
> >>>>> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> >>>>> index c47861dff9b9b..2f7da3832f458 100644
> >>>>> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
> >>>>> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> >>>>> @@ -467,6 +467,10 @@ desired image resolution. If the sub-device driver supports that, userspace
> >>>>>  can set the analog crop rectangle to select which portion of the pixel array
> >>>>>  to read out.
> >>>>>
> >>>>> +A description of each of the above mentioned targets when used to access the
> >>>>> +image sensor pixel array properties is provided by
> >>>>> +:ref:`v4l2-selection-targets-image-sensor-table`
> >>>>> +
> >>>>>
> >>>>>  Types of selection targets
> >>>>>  --------------------------
> >>>>> diff --git a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
> >>>>> index 69f500093aa2a..632e6448b784e 100644
> >>>>> --- a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
> >>>>> +++ b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
> >>>>> @@ -76,3 +76,52 @@ of the two interfaces they are used.
> >>>>>  	modified by hardware.
> >>>>>        - Yes
> >>>>>        - No
> >>>>> +
> >>>>> +
> >>>>> +.. _v4l2-selection-targets-image-sensor-table:
> >>>>> +
> >>>>> +********************************************
> >>>>> +Selection Targets For Pixel Array Properties
> >>>>> +********************************************
> >>>>> +
> >>>>> +The V4L2 selection API can be used to retrieve the size and disposition of the
> >>>>> +pixel units that compose and image sensor pixel matrix when applied to a video
> >>>>> +sub-device that represents an image sensor.
> >>>>> +
> >>>>> +A description of the properties associated with each of the sensor pixel array
> >>>>> +areas is provided by the :ref:`v4l2-subdev-pixel-array-properties` section.
> >>>>> +
> >>>>> +.. tabularcolumns:: |p{6.0cm}|p{1.4cm}|p{7.4cm}|p(1.4cm)|
> >>>>> +
> >>>>> +.. flat-table:: Selection target definitions
> >>>>> +    :header-rows:  1
> >>>>> +    :stub-columns: 0
> >>>>> +
> >>>>> +    * - Target name
> >>>>> +      - id
> >>>>> +      - Definition
> >>>>> +      - Read/Write
> >>>>> +    * - ``V4L2_SEL_TGT_CROP``
> >>>>> +      - 0x0000
> >>>>> +      - The analog crop rectangle. Represents the portion of the active pixel
> >>>>> +        array which is processed to produce images.
> >>>>> +      - RW
> >>>>> +    * - ``V4L2_SEL_TGT_CROP_DEFAULT``
> >>>>> +      - 0x0001
> >>>>> +      - The active pixel array rectangle. It includes only active pixels and
> >>>>> +        excludes other ones such as optical black pixels. Its width and height
> >>>>> +        represent the maximum image resolution an image sensor can produce.
> >>>>> +      - RO
> >>>>> +    * - ``V4L2_SEL_TGT_CROP_BOUNDS``
> >>>>> +      - 0x0002
> >>>>> +      - The readable portion of the physical pixel array matrix. It includes
> >>>>> +        pixels that contains valid image data and calibration pixels such as the
> >>>>> +        optical black ones.
> >>>>> +      - RO
> >>>>> +    * - ``V4L2_SEL_TGT_NATIVE_SIZE``
> >>>>> +      - 0x0003
> >>>>> +      - The physical pixel array size, including readable and not readable
> >>>>> +        pixels. As pixels that cannot be read from application processor are not
> >>>>> +        relevant for calibration purposes, this rectangle is useful to calculate
> >>>>> +        the physical properties of the image sensor.
> >>>>> +      - RO
> >>>>>
> >>>>
> >>>> Hmm, this basically just duplicates the previous patch.
> >>>>
> >>>> I think you are documenting things at the wrong place. What you documented in the
> >>>> previous patch really belongs here since it is shared between the subdev API and the
> >>>> regular V4L2 API. And in dev-subdev.rst you then refer to here.
> >>>
> >>> I originally had it here, but then I moved to dev-subdev as an image
> >>> sensor will always be represented as a video sub-device, doen't it ?
> >>
> >> No. Some camera drivers are V4L2 only, most notably uvc. Also there are several simple
> >> platform drivers that don't use the subdev API.
> >
> > Do we expect to be able to retrieve sensor array properties from video
> > device nodes which represents, in my understanding a DMA engine that
> > writes data to memory ? As I see it, not subdev for the image sensor,
> > no pixel array properties. How can these be exposed by a video device
> > which abstracts the full capture pipeline ?
>
> They will typically ask the subdev driver. The vidioc_g_selection op
> implementation will in turn call the get_selection op of the sensor subdev
> driver and pass that information back to userspace.

How do we know that the any target reports the actual sensor
properties and not some other limitation introduced by processing
components down the pipeline, as everything sits behind a single video
device node ? In example, the default crop rectangle might very well depend
on the receiver's side decision to crop the frames received from the
sensor to maximize the FOV or whatnot. How do we know which 'cropping
point' is reported ?

I hardly see a vidio_g_selection() being ideal to report properties of
the camera sensor such as the pixel array ones. I still feel we're
hijacking that API for that purpose and I would be glad to have
dedicated targets for image sensors. This would promote 'image
sensors' as first class citizens of the API like devices and
sub-devices, and I'm not sure this is desirable.

>
> There is nothing subdev specific to this API.
>
> Regards,
>
> 	Hans

Re: [PATCH 2/4] media: docs: Describe targets for sensor properties

[Hans Verkuil]

On 06/08/2020 15:36, Jacopo Mondi wrote:
> Hi Hans,
>    I'm sorry, I don't want to insist but..
> 
> On Thu, Aug 06, 2020 at 03:15:54PM +0200, Hans Verkuil wrote:
>> On 06/08/2020 14:45, Jacopo Mondi wrote:
>>> Hi Hans,
>>>
>>> On Thu, Aug 06, 2020 at 12:15:49PM +0200, Hans Verkuil wrote:
>>>> On 06/08/2020 12:08, Jacopo Mondi wrote:
>>>>> Hi Hans,
>>>>>
>>>>> On Thu, Aug 06, 2020 at 10:45:17AM +0200, Hans Verkuil wrote:
>>>>>> On 05/08/2020 12:57, Jacopo Mondi wrote:
>>>>>>> Provide a table to describe how the V4L2 selection targets can be used
>>>>>>> to access an image sensor pixel array properties.
>>>>>>>
>>>>>>> Reference the table in the sub-device documentation.
>>>>>>>
>>>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>>>>>> ---
>>>>>>>  .../userspace-api/media/v4l/dev-subdev.rst    |  4 ++
>>>>>>>  .../media/v4l/v4l2-selection-targets.rst      | 49 +++++++++++++++++++
>>>>>>>  2 files changed, 53 insertions(+)
>>>>>>>
>>>>>>> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
>>>>>>> index c47861dff9b9b..2f7da3832f458 100644
>>>>>>> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
>>>>>>> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
>>>>>>> @@ -467,6 +467,10 @@ desired image resolution. If the sub-device driver supports that, userspace
>>>>>>>  can set the analog crop rectangle to select which portion of the pixel array
>>>>>>>  to read out.
>>>>>>>
>>>>>>> +A description of each of the above mentioned targets when used to access the
>>>>>>> +image sensor pixel array properties is provided by
>>>>>>> +:ref:`v4l2-selection-targets-image-sensor-table`
>>>>>>> +
>>>>>>>
>>>>>>>  Types of selection targets
>>>>>>>  --------------------------
>>>>>>> diff --git a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
>>>>>>> index 69f500093aa2a..632e6448b784e 100644
>>>>>>> --- a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
>>>>>>> +++ b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
>>>>>>> @@ -76,3 +76,52 @@ of the two interfaces they are used.
>>>>>>>  	modified by hardware.
>>>>>>>        - Yes
>>>>>>>        - No
>>>>>>> +
>>>>>>> +
>>>>>>> +.. _v4l2-selection-targets-image-sensor-table:
>>>>>>> +
>>>>>>> +********************************************
>>>>>>> +Selection Targets For Pixel Array Properties
>>>>>>> +********************************************
>>>>>>> +
>>>>>>> +The V4L2 selection API can be used to retrieve the size and disposition of the
>>>>>>> +pixel units that compose and image sensor pixel matrix when applied to a video
>>>>>>> +sub-device that represents an image sensor.
>>>>>>> +
>>>>>>> +A description of the properties associated with each of the sensor pixel array
>>>>>>> +areas is provided by the :ref:`v4l2-subdev-pixel-array-properties` section.
>>>>>>> +
>>>>>>> +.. tabularcolumns:: |p{6.0cm}|p{1.4cm}|p{7.4cm}|p(1.4cm)|
>>>>>>> +
>>>>>>> +.. flat-table:: Selection target definitions
>>>>>>> +    :header-rows:  1
>>>>>>> +    :stub-columns: 0
>>>>>>> +
>>>>>>> +    * - Target name
>>>>>>> +      - id
>>>>>>> +      - Definition
>>>>>>> +      - Read/Write
>>>>>>> +    * - ``V4L2_SEL_TGT_CROP``
>>>>>>> +      - 0x0000
>>>>>>> +      - The analog crop rectangle. Represents the portion of the active pixel
>>>>>>> +        array which is processed to produce images.
>>>>>>> +      - RW
>>>>>>> +    * - ``V4L2_SEL_TGT_CROP_DEFAULT``
>>>>>>> +      - 0x0001
>>>>>>> +      - The active pixel array rectangle. It includes only active pixels and
>>>>>>> +        excludes other ones such as optical black pixels. Its width and height
>>>>>>> +        represent the maximum image resolution an image sensor can produce.
>>>>>>> +      - RO
>>>>>>> +    * - ``V4L2_SEL_TGT_CROP_BOUNDS``
>>>>>>> +      - 0x0002
>>>>>>> +      - The readable portion of the physical pixel array matrix. It includes
>>>>>>> +        pixels that contains valid image data and calibration pixels such as the
>>>>>>> +        optical black ones.
>>>>>>> +      - RO
>>>>>>> +    * - ``V4L2_SEL_TGT_NATIVE_SIZE``
>>>>>>> +      - 0x0003
>>>>>>> +      - The physical pixel array size, including readable and not readable
>>>>>>> +        pixels. As pixels that cannot be read from application processor are not
>>>>>>> +        relevant for calibration purposes, this rectangle is useful to calculate
>>>>>>> +        the physical properties of the image sensor.
>>>>>>> +      - RO
>>>>>>>
>>>>>>
>>>>>> Hmm, this basically just duplicates the previous patch.
>>>>>>
>>>>>> I think you are documenting things at the wrong place. What you documented in the
>>>>>> previous patch really belongs here since it is shared between the subdev API and the
>>>>>> regular V4L2 API. And in dev-subdev.rst you then refer to here.
>>>>>
>>>>> I originally had it here, but then I moved to dev-subdev as an image
>>>>> sensor will always be represented as a video sub-device, doen't it ?
>>>>
>>>> No. Some camera drivers are V4L2 only, most notably uvc. Also there are several simple
>>>> platform drivers that don't use the subdev API.
>>>
>>> Do we expect to be able to retrieve sensor array properties from video
>>> device nodes which represents, in my understanding a DMA engine that
>>> writes data to memory ? As I see it, not subdev for the image sensor,
>>> no pixel array properties. How can these be exposed by a video device
>>> which abstracts the full capture pipeline ?
>>
>> They will typically ask the subdev driver. The vidioc_g_selection op
>> implementation will in turn call the get_selection op of the sensor subdev
>> driver and pass that information back to userspace.
> 
> How do we know that the any target reports the actual sensor
> properties and not some other limitation introduced by processing
> components down the pipeline, as everything sits behind a single video
> device node ? In example, the default crop rectangle might very well depend
> on the receiver's side decision to crop the frames received from the
> sensor to maximize the FOV or whatnot. How do we know which 'cropping
> point' is reported ?

Why would you care? Would you do anything different in userspace if a driver
would modify these rectangles before passing it on to userspace?

> I hardly see a vidio_g_selection() being ideal to report properties of
> the camera sensor such as the pixel array ones. I still feel we're
> hijacking that API for that purpose and I would be glad to have
> dedicated targets for image sensors. This would promote 'image
> sensors' as first class citizens of the API like devices and
> sub-devices, and I'm not sure this is desirable.

Sorry, but the selection API (and its CROP predecessor) has been in use
for sensors and webcam drivers since pretty much the beginning of V4L1.

I'm not sure what it is that you think is problematical with the current
API.

Regards,

	Hans

> 
>>
>> There is nothing subdev specific to this API.
>>
>> Regards,
>>
>> 	Hans

Re: [PATCH 2/4] media: docs: Describe targets for sensor properties

[Jacopo Mondi]

Hi Hans,

On Thu, Aug 06, 2020 at 05:32:50PM +0200, Hans Verkuil wrote:
> On 06/08/2020 15:36, Jacopo Mondi wrote:
> > Hi Hans,
> >    I'm sorry, I don't want to insist but..
> >
> > On Thu, Aug 06, 2020 at 03:15:54PM +0200, Hans Verkuil wrote:
> >> On 06/08/2020 14:45, Jacopo Mondi wrote:
> >>> Hi Hans,
> >>>
> >>> On Thu, Aug 06, 2020 at 12:15:49PM +0200, Hans Verkuil wrote:
> >>>> On 06/08/2020 12:08, Jacopo Mondi wrote:
> >>>>> Hi Hans,
> >>>>>
> >>>>> On Thu, Aug 06, 2020 at 10:45:17AM +0200, Hans Verkuil wrote:
> >>>>>> On 05/08/2020 12:57, Jacopo Mondi wrote:
> >>>>>>> Provide a table to describe how the V4L2 selection targets can be used
> >>>>>>> to access an image sensor pixel array properties.
> >>>>>>>
> >>>>>>> Reference the table in the sub-device documentation.
> >>>>>>>
> >>>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>>>>>> ---
> >>>>>>>  .../userspace-api/media/v4l/dev-subdev.rst    |  4 ++
> >>>>>>>  .../media/v4l/v4l2-selection-targets.rst      | 49 +++++++++++++++++++
> >>>>>>>  2 files changed, 53 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> >>>>>>> index c47861dff9b9b..2f7da3832f458 100644
> >>>>>>> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
> >>>>>>> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> >>>>>>> @@ -467,6 +467,10 @@ desired image resolution. If the sub-device driver supports that, userspace
> >>>>>>>  can set the analog crop rectangle to select which portion of the pixel array
> >>>>>>>  to read out.
> >>>>>>>
> >>>>>>> +A description of each of the above mentioned targets when used to access the
> >>>>>>> +image sensor pixel array properties is provided by
> >>>>>>> +:ref:`v4l2-selection-targets-image-sensor-table`
> >>>>>>> +
> >>>>>>>
> >>>>>>>  Types of selection targets
> >>>>>>>  --------------------------
> >>>>>>> diff --git a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
> >>>>>>> index 69f500093aa2a..632e6448b784e 100644
> >>>>>>> --- a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
> >>>>>>> +++ b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
> >>>>>>> @@ -76,3 +76,52 @@ of the two interfaces they are used.
> >>>>>>>  	modified by hardware.
> >>>>>>>        - Yes
> >>>>>>>        - No
> >>>>>>> +
> >>>>>>> +
> >>>>>>> +.. _v4l2-selection-targets-image-sensor-table:
> >>>>>>> +
> >>>>>>> +********************************************
> >>>>>>> +Selection Targets For Pixel Array Properties
> >>>>>>> +********************************************
> >>>>>>> +
> >>>>>>> +The V4L2 selection API can be used to retrieve the size and disposition of the
> >>>>>>> +pixel units that compose and image sensor pixel matrix when applied to a video
> >>>>>>> +sub-device that represents an image sensor.
> >>>>>>> +
> >>>>>>> +A description of the properties associated with each of the sensor pixel array
> >>>>>>> +areas is provided by the :ref:`v4l2-subdev-pixel-array-properties` section.
> >>>>>>> +
> >>>>>>> +.. tabularcolumns:: |p{6.0cm}|p{1.4cm}|p{7.4cm}|p(1.4cm)|
> >>>>>>> +
> >>>>>>> +.. flat-table:: Selection target definitions
> >>>>>>> +    :header-rows:  1
> >>>>>>> +    :stub-columns: 0
> >>>>>>> +
> >>>>>>> +    * - Target name
> >>>>>>> +      - id
> >>>>>>> +      - Definition
> >>>>>>> +      - Read/Write
> >>>>>>> +    * - ``V4L2_SEL_TGT_CROP``
> >>>>>>> +      - 0x0000
> >>>>>>> +      - The analog crop rectangle. Represents the portion of the active pixel
> >>>>>>> +        array which is processed to produce images.
> >>>>>>> +      - RW
> >>>>>>> +    * - ``V4L2_SEL_TGT_CROP_DEFAULT``
> >>>>>>> +      - 0x0001
> >>>>>>> +      - The active pixel array rectangle. It includes only active pixels and
> >>>>>>> +        excludes other ones such as optical black pixels. Its width and height
> >>>>>>> +        represent the maximum image resolution an image sensor can produce.
> >>>>>>> +      - RO
> >>>>>>> +    * - ``V4L2_SEL_TGT_CROP_BOUNDS``
> >>>>>>> +      - 0x0002
> >>>>>>> +      - The readable portion of the physical pixel array matrix. It includes
> >>>>>>> +        pixels that contains valid image data and calibration pixels such as the
> >>>>>>> +        optical black ones.
> >>>>>>> +      - RO
> >>>>>>> +    * - ``V4L2_SEL_TGT_NATIVE_SIZE``
> >>>>>>> +      - 0x0003
> >>>>>>> +      - The physical pixel array size, including readable and not readable
> >>>>>>> +        pixels. As pixels that cannot be read from application processor are not
> >>>>>>> +        relevant for calibration purposes, this rectangle is useful to calculate
> >>>>>>> +        the physical properties of the image sensor.
> >>>>>>> +      - RO
> >>>>>>>
> >>>>>>
> >>>>>> Hmm, this basically just duplicates the previous patch.
> >>>>>>
> >>>>>> I think you are documenting things at the wrong place. What you documented in the
> >>>>>> previous patch really belongs here since it is shared between the subdev API and the
> >>>>>> regular V4L2 API. And in dev-subdev.rst you then refer to here.
> >>>>>
> >>>>> I originally had it here, but then I moved to dev-subdev as an image
> >>>>> sensor will always be represented as a video sub-device, doen't it ?
> >>>>
> >>>> No. Some camera drivers are V4L2 only, most notably uvc. Also there are several simple
> >>>> platform drivers that don't use the subdev API.
> >>>
> >>> Do we expect to be able to retrieve sensor array properties from video
> >>> device nodes which represents, in my understanding a DMA engine that
> >>> writes data to memory ? As I see it, not subdev for the image sensor,
> >>> no pixel array properties. How can these be exposed by a video device
> >>> which abstracts the full capture pipeline ?
> >>
> >> They will typically ask the subdev driver. The vidioc_g_selection op
> >> implementation will in turn call the get_selection op of the sensor subdev
> >> driver and pass that information back to userspace.
> >
> > How do we know that the any target reports the actual sensor
> > properties and not some other limitation introduced by processing
> > components down the pipeline, as everything sits behind a single video
> > device node ? In example, the default crop rectangle might very well depend
> > on the receiver's side decision to crop the frames received from the
> > sensor to maximize the FOV or whatnot. How do we know which 'cropping
> > point' is reported ?
>
> Why would you care? Would you do anything different in userspace if a driver
> would modify these rectangles before passing it on to userspace?
>

Yes it indeed makes a difference, particularly for application dealing
with RAW sensors and for IPS tuning algorithms that need to access the
sensor pixel matrix sizes to calculate, in example, the FOV ratio, or
might want to access black pixels for calibration purposes.

Now, I'm not an expert on that part, but in example I see the
RaspberryPi 3A algorthms in libcamera using the ratio between the
active pixel array size and what I referred to as 'analogCrop' to
calculate the lens shading correction maps
https://git.linuxtv.org/libcamera.git/tree/src/ipa/raspberrypi/controller/rpi/alsc.cpp#n185

I can imagine there are other relevant image tuning algorithms that
needs to access the sensor characteristics precisely. Knowing that
what I'm getting describes the sensor properties is relevant. That's
why I hardly see this happening going through a single device node.

> > I hardly see a vidio_g_selection() being ideal to report properties of
> > the camera sensor such as the pixel array ones. I still feel we're
> > hijacking that API for that purpose and I would be glad to have
> > dedicated targets for image sensors. This would promote 'image
> > sensors' as first class citizens of the API like devices and
> > sub-devices, and I'm not sure this is desirable.
>
> Sorry, but the selection API (and its CROP predecessor) has been in use
> for sensors and webcam drivers since pretty much the beginning of V4L1.
>
> I'm not sure what it is that you think is problematical with the current
> API.
>

I'm not proposing to kill that API, I just think the existing targets
fall short or are under-specified when applied to RAW image sensor,
that's it :)

Thanks
  j

> Regards,
>
> 	Hans
>
> >
> >>
> >> There is nothing subdev specific to this API.
> >>
> >> Regards,
> >>
> >> 	Hans
>

Re: your mail

[Laurent Pinchart]

Hi Jacopo,

On Wed, Aug 05, 2020 at 12:57:17PM +0200, Jacopo Mondi wrote:
> Subject: [PATCH 0/4] media: docs: Document pixel array properties
> 
> Hans' patch "[PATCH] imx219: selection compliance fixes" sparkled a discussion
> on how the V4L2 selection targets have to be used in order to access an
> image sensor pixel array properties.
> 
> The discussion shown how much under-specified that part was, so this is
> an attempt to provide a bit documentation for this.
> 
> My feeling is that we're hijacking the existing targets for this use case
> and we should probably define new ones, considering how few users we have in
> mainline of them at the moment.

Do you mean you think we should create new targets instead of reusing
the existing ones as you do in this series ? I don't see anything really
wrong in reusing the existing targets, provided we document them
properly, and it's not too much of a hack (that is, the documented
purpose reasonably matches the target name), but maybe I'm missing the
issue.

> On top Hans' patch with reworded commit message and minor updates.
> 
> For reference, we're using the V4L2 selection targets in libcamera to retrieve
> the sensor pixel array properties to be reported to applications for
> calibration purposes. The currently defined pixel properties for libcamera
> are available here:
> https://git.linuxtv.org/libcamera.git/tree/src/libcamera/property_ids.yaml#n390
> 
> Thanks
>    j
> 
> Hans Verkuil (1):
>   media: i2c: imx219: Selection compliance fixes
> 
> Jacopo Mondi (3):
>   media: docs: Describe pixel array properties
>   media: docs: Describe targets for sensor properties
>   media: docs: USe SUBDEV_G_SELECTION for sensor properties
> 
>  .../userspace-api/media/v4l/dev-subdev.rst    | 85 +++++++++++++++++++
>  .../media/v4l/v4l2-selection-targets.rst      | 49 +++++++++++
>  .../media/v4l/vidioc-subdev-g-selection.rst   |  4 +
>  drivers/media/i2c/imx219.c                    | 17 ++--
>  4 files changed, 147 insertions(+), 8 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/4] media: docs: Describe pixel array properties

[Laurent Pinchart]

Hi Jacopo, Hans,

On Thu, Aug 06, 2020 at 11:58:31AM +0200, Hans Verkuil wrote:
> On 06/08/2020 11:50, Jacopo Mondi wrote:
> > On Thu, Aug 06, 2020 at 10:05:37AM +0200, Hans Verkuil wrote:
> >> On 05/08/2020 12:57, Jacopo Mondi wrote:
> >>> The V4L2 selection API are also used to access the pixel array
> >>
> >> are -> is
> >>
> >>> properties of an image sensor, such as the size and position of active
> >>> pixels and the cropped area of the pixel matrix used to produce images.
> >>>
> >>> Currently no clear definition of the different areas that compose an
> >>> image sensor pixel array matrix is provided in the specification, and
> >>> the actual meaning of each selection target when applied to an image
> >>> sensor was not provided.
> >>>
> >>> Provide in the sub-device documentation the definition of the pixel
> >>> matrix properties and the selection target associated to each of them.
> >>>
> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>> ---
> >>>  .../userspace-api/media/v4l/dev-subdev.rst    | 81 +++++++++++++++++++
> >>>  1 file changed, 81 insertions(+)
> >>>
> >>> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> >>> index 134d2fb909fa4..c47861dff9b9b 100644
> >>> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
> >>> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> >>> @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.
> >>>  ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round
> >>>  the image size either up or down. :ref:`v4l2-selection-flags`
> >>>
> >>> +.. _v4l2-subdev-pixel-array-properties:
> >>> +
> >>> +Selection targets for image sensors properties

Maybe "Selection targets for image sensors", and renaming the reference
to v4l2-subdev-selections-image-sensors ? This section is about how
selection rectangles are used for sensors, right ?

> >>> +----------------------------------------------

I'd move this further down, after "Types of selection targets", as that
section contains generic information that applies to sensors too.

> >>> +
> >>> +The V4L2 selection API can be used on sub-devices that represent an image
> >>> +sensor to retrieve the sensor's pixel array matrix properties by using the
> >>> +:ref:`selection <VIDIOC_SUBDEV_G_SELECTION>` ioctls.
> >>> +
> >>> +Sub-device drivers for image sensor usually register a single source pad, but in
> >>> +the case they expose more, the pixel array properties can be accessed from
> >>> +any of them.

Is this right ? I don't think the V4L2_SEL_TGT_CROP rectangle, for
instance, can be accessed from any source pad indifferently. Do we have
sensor drivers that create multiple source pads in a subdev today ? If
not I'd suggest dropping this, and adding it later if needed (when we'll
have a better idea of how that would work).

I think what should be explained here, as also mentioned by Sakari, is
that camera sensors can be exposed as multiple subdevs. The text below
is related to the pixel array, which is always the first subdev, with
one source pad and no sink pad. The other subdevs, modelling additional
processing blocks in the sensor, may use the selection API, but that's
out of scope for this patch.

As we'll also need to document how other subdevs use the selection API,
as well as how sensors are usually handled, would it make sense to move
this to a separate file ? Sakari has proposed in [1] to create a new
Documentation/driver-api/media/camera-sensor.rst file. It would make
sense to centralize all sensor information there. This doesn't mean this
series should depend on Sakari's patch, we can handle merge conflicts
depending on what gets merged first.

[1] https://lore.kernel.org/linux-media/20200730162040.15560-1-sakari.ailus@linux.intel.com/

> >>> +
> >>> +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,
> >>
> >> V4L2_SEL_TGT_NATIVE -> V4L2_SEL_TGT_NATIVE_SIZE
> >>
> >> (same mistake is made elsewhere).
> > 
> > Ah ups, I used TGT_NATIVE consistently, seems like I thought that was
> > the right name
> > 
> >>> +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve
> >>> +the immutable properties of the several different areas that compose the sensor
> >>> +pixel array matrix. Each area describes a rectangle of logically adjacent pixel

V4L2_TGT_CROP isn't immutable, is it ?

> >>> +units. The logical disposition of pixels is defined by the sensor read-out
> >>> +starting point and direction, and may differ from the physical disposition of
> >>> +the pixel units in the pixel array matrix.
> >>> +
> >>> +Each pixel matrix portion is contained in a larger rectangle, with the most
> >>> +largest being the one that describes the pixel matrix physical size. This
> >>> +defines a hierarchical positional system, where each rectangle is defined
> >>> +relatively to the largest available one among the ones exposed by the
> >>> +sub-device driver. Each selection target and the associated pixel array portion
> >>> +it represents are below presented in order from the largest to the smallest one.

I find this quite confusing. As Hans suggested, I think each target
should define its boundaries. I'd drop this paragraph completely, as you
already explain below that all rectangles are defined relatively to
V4L2_SEL_TGT_NATIVE_SIZE.

> >>> +
> >>> +Pixel array physical size
> >>> +^^^^^^^^^^^^^^^^^^^^^^^^^
> >>> +
> >>> +The image sensor chip is composed by a number of physical pixels, not all of
> >>> +them readable by the application processor. Invalid or unreadable lines might
> >>> +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors
> >>> +they might be tagged with an invalid data type (DT) so that the receiver
> >>> +automatically discard them.

"might" is a bit weak for unreadable lines, there's no way they can be
transmitted if they can't be read :-)

One way to generalize this a bit would be to explain, after the first
sentence, that not all pixels may be read by the sensor, that among the
pixels that are read invalid ones may not be transmitted on the bus, and
that among transmitted pixels not all of them may be possible to capture
on the receiver side. For instance, invalid lines may be transmitted as
part of the vertical blanking on parallel buses, or tagged as blanking
data or null data on CSI-2 buses. Most receivers are not able to capture
either.

(On a side note, strictly speaking, a CSI-2 receiver that would be able
to capture null or blanking packets wouldn't be compliant with the CSI-2
spec.)

> >>> The size of the whole pixel matrix area is
> >>> +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner
> >>> +defined as position (0, 0). All the other selection targets are defined
> >>> +relatively to this, larger, rectangle. The rectangle returned by

s/, larger,/

> >>> +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it
> >>> +does not change at run-time and cannot be modified from userspace.
> >>
> >> It is a good idea to mention that if there are no invalid or unreadable pixels/lines,
> >> then V4L2_SEL_TGT_NATIVE_SIZE == V4L2_SEL_TGT_CROP_BOUNDS.
> > 
> > Yes it is! I'll add it here

Should it be added below instead, where you define
V4L2_SEL_TGT_CROP_BOUNDS ? It's best to avoid mentioning something that
isn't defined yet when possible.

> >>> +
> >>> +Pixel array readable area
> >>> +^^^^^^^^^^^^^^^^^^^^^^^^^
> >>> +
> >>> +The V4L2_SEL_TGT_CROP_BOUNDS targets returns size and position of the readable
> >>> +area of the pixel array matrix, including pixels with valid image data and pixel
> >>> +used for calibration purposes, such as optical black pixels. It is not unlikely

s/not unlikely/likely ? Or just "common".

> >>> +that valid pixels and optical black pixels are surrounded by non-readable rows
> >>> +and columns of pixels. Those does not concur in the definition of the

s/does/do/

I'm not sure "concur" is the right word. Did you mean "those are not
part of the V4L2_SEL_TGT_CROP_BOUNDS rectangle" ?

> >>> +V4L2_SEL_TGT_CROP_BOUNDS rectangle. The rectangle returned by
> >>> +V4L2_SEL_TGT_CROP_BOUNDS describes an immutable property of the image sensor, it
> >>> +does not change at run-time and cannot be modified from userspace.
> >>
> >> Mention that BOUNDS is enclosed by NATIVE_SIZE.
> > 
> > I tried to express that in the intro section with
> > 
> > "Each pixel matrix portion is contained in a larger rectangle, with the most
> > largest being the one that describes the pixel matrix physical size."
> > 
> > But I guess it's worth to express that for each target!
> > 
> >>> +
> >>> +Pixel array active area
> >>> +^^^^^^^^^^^^^^^^^^^^^^^
> >>> +
> >>> +The portion of the pixel array which contains valid image data is defined as the
> >>> +active area of the pixel matrix. The active pixel array is is accessed by mean
> >>

s/is is/is/

> >> mean -> means
> >>
> >>> +of the V4L2_SEL_TGT_CROP_DEFAULT target, and is contained in the larger
> >>> +V4L2_SEL_TGT_CROP_BOUNDS rectangle. It represents the largest possible frame
> >>> +resolution the sensor can produce and defines the dimension of the full
> >>> +field-of-view. The rectangle returned by V4L2_SEL_TGT_CROP_BOUNDS describes an
> >>
> >> BOUNDS -> DEFAULT
> > 
> > ups
> > 
> >>> +immutable property of the image sensor, it does not change at run-time and
> >>> +cannot be modified from userspace.
> >>
> >> Mention that CROP_DEFAULT is enclosed by CROP_BOUNDS
> >>
> >>> +
> >>> +Analog crop rectangle
> >>
> >> Why analog? It's just the crop rectangle, nothing analog about it.
> > 
> > We used the 'analogCrop' term in libcamera to differentiate the
> > cropping which happens on the sensor pixel array matrix to select the
> > region to process and produce image from. Sensor with an on-board
> > scaler can perform other cropping steps to implement, in example digital
> > zoom, so we expect to have a 'digital crop' phase as well. RAW
> > sensors, in example, will only have an analogCrop rectangle.
> > 
> > Quoting the libcamera definition of analog crop:
> > 
> >  * horizontal and vertical sizes define the portion of the pixel array which
> >  * is read-out and provided to the sensor's internal processing pipeline, before
> >  * any pixel sub-sampling method, such as pixel binning, skipping and averaging
> >  * take place.
> > 
> > should I keep it or remove it ?
> 
> It's a very confusing term. Especially since this API can also be used with analog
> video capture devices (Composite/S-Video) where the video signal actually is analog.
> 
> In the V4L2 API there is no such thing as 'analog crop', so please remove it.

Jacopo is right, sensors usually perform cropping in the analog domain
(by not reading out all pixels from the pixel array), and also support
cropping in later stages, after binning/skipping, and after further
scaling. Note that all of these crop operations are optional. Although
not common, it's also not unconceivable that a sensor wouldn't support
cropping at all.

This being said, it only makes sense to talk about analog crop when
multiple crop operations are performed, and thus in the context of the
whole sensor, with multiple subdevs. If we explain this, as proposed
above, and make it clear that the usage of the selection rectangles
defined here applies to the pixel array only, we can drop the "analog"
term, and just talk about cropping in the pixel array.

> >>> +^^^^^^^^^^^^^^^^^^^^^
> >>> +
> >>> +The sensor driver might decide, in order to adjust the image resolution to best
> >>> +match the one requested by applications, to only process a part of the active
> >>> +pixel array matrix.

I don't think that's the right approach. With MC-based devices, the
philosophy is to expose all configuration parameters to userspace. It's
not about sensor drivers making decisions, but about userspace deciding
to crop at the pixel array level.

This being said, I'm aware the decision is made by drivers when they're
mode-based. Please see below for that.

> >>> The selected area is read-out and processed by the image
> >>> +sensor on-board ISP in order to produce images of the desired size and
> >>> +resolution while possible maintaing the largest possible field-of-view. The
> >>
> >> maintaing -> maintaining
> >>
> >> Actually, I'd drop 'while possible maintaing the largest possible field-of-view'
> >> entirely. It doesn't make much sense.
> > 
> > Ack

In general, in this section, as we're documenting the pixel array, let's
not talk about the ISP.

> >>> +cropped portion of the pixel array which is used to produce images is returned
> >>> +by the V4L2_SEL_TGT_CROP target and represent the only information that can
> >>
> >> represent -> represents
> >>
> >>> +change at runtime as it depends on the currently configured sensor mode and
> >>> +desired image resolution. If the sub-device driver supports that, userspace
> >>> +can set the analog crop rectangle to select which portion of the pixel array
> >>
> >> s/analog//
> >>
> >>> +to read out.

I think it's better to focus on the best case, and document usage of
crop rectangles in general first, for drivers that expose full
configurability of the sensor. A separate section should then then make
a note of how mode-based drivers differ, which is mostly in the
V4L2_SEL_TGT_CROP target being read-only, and on the single subdev
hiding all the processing steps, with the crop target thus being the
result of all cropping operations, analog and digital.

Sakari's patch has a bit of information about this, it may be useful to
reuse it or integrate with it somehow.

> >> Mention that CROP is enclosed by CROP_BOUNDS and defaults to CROP_DEFAULT.
> >>
> >> Make a note that CROP can also be used to obtain optical black pixels.
> > 
> > What about:
> > 
> > +desired image resolution. If the sub-device driver supports that, userspace
> > +can set the analog crop rectangle to select which portion of the pixel array
> > +to read out including, if supported, optical black pixels.
> 
> Hmm, that's a bit awkward. How about:
> 
> +desired image resolution. If supported by the sub-device driver, userspace
> +can set the crop rectangle to select which portion of the pixel array
> +to read out. This may include optical black pixels if those are part of
> +V4L2_SEL_TGT_CROP_BOUNDS.
>
> >>> +
> >>>
> >>>  Types of selection targets
> >>>  --------------------------

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/4] media: docs: Describe targets for sensor properties

[Laurent Pinchart]

Hi Jacopo and Hans,

On Thu, Aug 06, 2020 at 10:45:17AM +0200, Hans Verkuil wrote:
> On 05/08/2020 12:57, Jacopo Mondi wrote:
> > Provide a table to describe how the V4L2 selection targets can be used
> > to access an image sensor pixel array properties.
> > 
> > Reference the table in the sub-device documentation.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  .../userspace-api/media/v4l/dev-subdev.rst    |  4 ++
> >  .../media/v4l/v4l2-selection-targets.rst      | 49 +++++++++++++++++++
> >  2 files changed, 53 insertions(+)
> > 
> > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > index c47861dff9b9b..2f7da3832f458 100644
> > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > @@ -467,6 +467,10 @@ desired image resolution. If the sub-device driver supports that, userspace
> >  can set the analog crop rectangle to select which portion of the pixel array
> >  to read out.
> >  
> > +A description of each of the above mentioned targets when used to access the
> > +image sensor pixel array properties is provided by
> > +:ref:`v4l2-selection-targets-image-sensor-table`
> > +
> >  
> >  Types of selection targets
> >  --------------------------
> > diff --git a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
> > index 69f500093aa2a..632e6448b784e 100644
> > --- a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
> > +++ b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
> > @@ -76,3 +76,52 @@ of the two interfaces they are used.
> >  	modified by hardware.
> >        - Yes
> >        - No
> > +
> > +
> > +.. _v4l2-selection-targets-image-sensor-table:
> > +
> > +********************************************
> > +Selection Targets For Pixel Array Properties
> > +********************************************
> > +
> > +The V4L2 selection API can be used to retrieve the size and disposition of the
> > +pixel units that compose and image sensor pixel matrix when applied to a video
> > +sub-device that represents an image sensor.
> > +
> > +A description of the properties associated with each of the sensor pixel array
> > +areas is provided by the :ref:`v4l2-subdev-pixel-array-properties` section.
> > +
> > +.. tabularcolumns:: |p{6.0cm}|p{1.4cm}|p{7.4cm}|p(1.4cm)|
> > +
> > +.. flat-table:: Selection target definitions
> > +    :header-rows:  1
> > +    :stub-columns: 0
> > +
> > +    * - Target name
> > +      - id
> > +      - Definition
> > +      - Read/Write
> > +    * - ``V4L2_SEL_TGT_CROP``
> > +      - 0x0000
> > +      - The analog crop rectangle. Represents the portion of the active pixel
> > +        array which is processed to produce images.
> > +      - RW
> > +    * - ``V4L2_SEL_TGT_CROP_DEFAULT``
> > +      - 0x0001
> > +      - The active pixel array rectangle. It includes only active pixels and
> > +        excludes other ones such as optical black pixels. Its width and height
> > +        represent the maximum image resolution an image sensor can produce.
> > +      - RO
> > +    * - ``V4L2_SEL_TGT_CROP_BOUNDS``
> > +      - 0x0002
> > +      - The readable portion of the physical pixel array matrix. It includes
> > +        pixels that contains valid image data and calibration pixels such as the
> > +        optical black ones.
> > +      - RO
> > +    * - ``V4L2_SEL_TGT_NATIVE_SIZE``
> > +      - 0x0003
> > +      - The physical pixel array size, including readable and not readable
> > +        pixels. As pixels that cannot be read from application processor are not
> > +        relevant for calibration purposes, this rectangle is useful to calculate
> > +        the physical properties of the image sensor.
> > +      - RO
> > 
> 
> Hmm, this basically just duplicates the previous patch.
> 
> I think you are documenting things at the wrong place. What you documented in the
> previous patch really belongs here since it is shared between the subdev API and the
> regular V4L2 API. And in dev-subdev.rst you then refer to here.
> 
> Frankly, the selection API documentation is a mess. It's spread out over various sections:
> The VIDIOC_G/S_SELECTION and VIDIOC_SUBDEV_G/S_SELECTION descriptions in the Function Reference,
> section 8 ("Common definitions for V4L2 and V4L2 subdev interfaces"), 1.25 "Cropping, composing
> and scaling – the SELECTION API" and 4.13.3.2-4.13.3.3 "Selections: cropping, scaling and composition".
> 
> In my view section 8 should be moved to section 1.25.2.

Agreed.

> Ideally 1.25 should be rewritten for both
> the V4L2 and V4L2 subdev APIs, but that's a lot of work.

Agreed too.

> I would suggest that you add a first patch that moves section 8 to 1.25.2. Note that I don't like
> the tables for the selection targets and flags: the 'Valid for V4L2/V4L2 subdevs' columns should
> be removed and it should either be mentioned in the definition if a target/flag is invalid for
> an API, or it should be put in a separate table.

Should "1.25.3.1. Configuration of video capture" be moved to "4.1.
Video Capture Interface", and "1.25.3.2. Configuration of video output"
to "4.3. Video Output Interface" ?

I'm not sure where patch 1/4 should be moved to though. As I mentioned
in the review, I think it should create a section specific to camera
sensors. "4.13. Sub-device Interface" isn't a very good candidate as it
describes the interface, it shouldn't document how particular classes of
subdevs are to be handled. Maybe a new top-level section in "Part I -
Video for Linux API" to describe different types of sub-devices ?

> And in 1.25.2 perhaps a new picture can be created for the specific sensor use-case that includes
> the NATIVE_SIZE target.
> 
> Another pet peeve of mine is that section 8 splits the selection targets and flags into separate
> subsections. I'd just keep it in one section.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/4] media: docs: Describe targets for sensor properties

[Laurent Pinchart]

Hi Jacopo and Hans,

On Thu, Aug 06, 2020 at 06:11:56PM +0200, Jacopo Mondi wrote:
> On Thu, Aug 06, 2020 at 05:32:50PM +0200, Hans Verkuil wrote:
> > On 06/08/2020 15:36, Jacopo Mondi wrote:
> >> On Thu, Aug 06, 2020 at 03:15:54PM +0200, Hans Verkuil wrote:
> >>> On 06/08/2020 14:45, Jacopo Mondi wrote:
> >>>> On Thu, Aug 06, 2020 at 12:15:49PM +0200, Hans Verkuil wrote:
> >>>>> On 06/08/2020 12:08, Jacopo Mondi wrote:
> >>>>>> On Thu, Aug 06, 2020 at 10:45:17AM +0200, Hans Verkuil wrote:
> >>>>>>> On 05/08/2020 12:57, Jacopo Mondi wrote:
> >>>>>>>> Provide a table to describe how the V4L2 selection targets can be used
> >>>>>>>> to access an image sensor pixel array properties.
> >>>>>>>>
> >>>>>>>> Reference the table in the sub-device documentation.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>>>>>>> ---
> >>>>>>>>  .../userspace-api/media/v4l/dev-subdev.rst    |  4 ++
> >>>>>>>>  .../media/v4l/v4l2-selection-targets.rst      | 49 +++++++++++++++++++
> >>>>>>>>  2 files changed, 53 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> >>>>>>>> index c47861dff9b9b..2f7da3832f458 100644
> >>>>>>>> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
> >>>>>>>> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> >>>>>>>> @@ -467,6 +467,10 @@ desired image resolution. If the sub-device driver supports that, userspace
> >>>>>>>>  can set the analog crop rectangle to select which portion of the pixel array
> >>>>>>>>  to read out.
> >>>>>>>>
> >>>>>>>> +A description of each of the above mentioned targets when used to access the
> >>>>>>>> +image sensor pixel array properties is provided by
> >>>>>>>> +:ref:`v4l2-selection-targets-image-sensor-table`
> >>>>>>>> +
> >>>>>>>>
> >>>>>>>>  Types of selection targets
> >>>>>>>>  --------------------------
> >>>>>>>> diff --git a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
> >>>>>>>> index 69f500093aa2a..632e6448b784e 100644
> >>>>>>>> --- a/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
> >>>>>>>> +++ b/Documentation/userspace-api/media/v4l/v4l2-selection-targets.rst
> >>>>>>>> @@ -76,3 +76,52 @@ of the two interfaces they are used.
> >>>>>>>>  	modified by hardware.
> >>>>>>>>        - Yes
> >>>>>>>>        - No
> >>>>>>>> +
> >>>>>>>> +
> >>>>>>>> +.. _v4l2-selection-targets-image-sensor-table:
> >>>>>>>> +
> >>>>>>>> +********************************************
> >>>>>>>> +Selection Targets For Pixel Array Properties
> >>>>>>>> +********************************************
> >>>>>>>> +
> >>>>>>>> +The V4L2 selection API can be used to retrieve the size and disposition of the
> >>>>>>>> +pixel units that compose and image sensor pixel matrix when applied to a video
> >>>>>>>> +sub-device that represents an image sensor.
> >>>>>>>> +
> >>>>>>>> +A description of the properties associated with each of the sensor pixel array
> >>>>>>>> +areas is provided by the :ref:`v4l2-subdev-pixel-array-properties` section.
> >>>>>>>> +
> >>>>>>>> +.. tabularcolumns:: |p{6.0cm}|p{1.4cm}|p{7.4cm}|p(1.4cm)|
> >>>>>>>> +
> >>>>>>>> +.. flat-table:: Selection target definitions
> >>>>>>>> +    :header-rows:  1
> >>>>>>>> +    :stub-columns: 0
> >>>>>>>> +
> >>>>>>>> +    * - Target name
> >>>>>>>> +      - id
> >>>>>>>> +      - Definition
> >>>>>>>> +      - Read/Write
> >>>>>>>> +    * - ``V4L2_SEL_TGT_CROP``
> >>>>>>>> +      - 0x0000
> >>>>>>>> +      - The analog crop rectangle. Represents the portion of the active pixel
> >>>>>>>> +        array which is processed to produce images.
> >>>>>>>> +      - RW
> >>>>>>>> +    * - ``V4L2_SEL_TGT_CROP_DEFAULT``
> >>>>>>>> +      - 0x0001
> >>>>>>>> +      - The active pixel array rectangle. It includes only active pixels and
> >>>>>>>> +        excludes other ones such as optical black pixels. Its width and height
> >>>>>>>> +        represent the maximum image resolution an image sensor can produce.
> >>>>>>>> +      - RO
> >>>>>>>> +    * - ``V4L2_SEL_TGT_CROP_BOUNDS``
> >>>>>>>> +      - 0x0002
> >>>>>>>> +      - The readable portion of the physical pixel array matrix. It includes
> >>>>>>>> +        pixels that contains valid image data and calibration pixels such as the
> >>>>>>>> +        optical black ones.
> >>>>>>>> +      - RO
> >>>>>>>> +    * - ``V4L2_SEL_TGT_NATIVE_SIZE``
> >>>>>>>> +      - 0x0003
> >>>>>>>> +      - The physical pixel array size, including readable and not readable
> >>>>>>>> +        pixels. As pixels that cannot be read from application processor are not
> >>>>>>>> +        relevant for calibration purposes, this rectangle is useful to calculate
> >>>>>>>> +        the physical properties of the image sensor.
> >>>>>>>> +      - RO
> >>>>>>>>
> >>>>>>>
> >>>>>>> Hmm, this basically just duplicates the previous patch.
> >>>>>>>
> >>>>>>> I think you are documenting things at the wrong place. What you documented in the
> >>>>>>> previous patch really belongs here since it is shared between the subdev API and the
> >>>>>>> regular V4L2 API. And in dev-subdev.rst you then refer to here.
> >>>>>>
> >>>>>> I originally had it here, but then I moved to dev-subdev as an image
> >>>>>> sensor will always be represented as a video sub-device, doen't it ?
> >>>>>
> >>>>> No. Some camera drivers are V4L2 only, most notably uvc. Also there are several simple
> >>>>> platform drivers that don't use the subdev API.
> >>>>
> >>>> Do we expect to be able to retrieve sensor array properties from video
> >>>> device nodes which represents, in my understanding a DMA engine that
> >>>> writes data to memory ? As I see it, not subdev for the image sensor,
> >>>> no pixel array properties. How can these be exposed by a video device
> >>>> which abstracts the full capture pipeline ?
> >>>
> >>> They will typically ask the subdev driver. The vidioc_g_selection op
> >>> implementation will in turn call the get_selection op of the sensor subdev
> >>> driver and pass that information back to userspace.
> >>
> >> How do we know that the any target reports the actual sensor
> >> properties and not some other limitation introduced by processing
> >> components down the pipeline, as everything sits behind a single video
> >> device node ? In example, the default crop rectangle might very well depend
> >> on the receiver's side decision to crop the frames received from the
> >> sensor to maximize the FOV or whatnot. How do we know which 'cropping
> >> point' is reported ?
> >
> > Why would you care? Would you do anything different in userspace if a driver
> > would modify these rectangles before passing it on to userspace?
>
> Yes it indeed makes a difference, particularly for application dealing
> with RAW sensors and for IPS tuning algorithms that need to access the
> sensor pixel matrix sizes to calculate, in example, the FOV ratio, or
> might want to access black pixels for calibration purposes.
> 
> Now, I'm not an expert on that part, but in example I see the
> RaspberryPi 3A algorthms in libcamera using the ratio between the
> active pixel array size and what I referred to as 'analogCrop' to
> calculate the lens shading correction maps
> https://git.linuxtv.org/libcamera.git/tree/src/ipa/raspberrypi/controller/rpi/alsc.cpp#n185
> 
> I can imagine there are other relevant image tuning algorithms that
> needs to access the sensor characteristics precisely. Knowing that
> what I'm getting describes the sensor properties is relevant. That's
> why I hardly see this happening going through a single device node.
> 
> >> I hardly see a vidio_g_selection() being ideal to report properties of
> >> the camera sensor such as the pixel array ones. I still feel we're
> >> hijacking that API for that purpose and I would be glad to have
> >> dedicated targets for image sensors. This would promote 'image
> >> sensors' as first class citizens of the API like devices and
> >> sub-devices, and I'm not sure this is desirable.
> >
> > Sorry, but the selection API (and its CROP predecessor) has been in use
> > for sensors and webcam drivers since pretty much the beginning of V4L1.
> >
> > I'm not sure what it is that you think is problematical with the current
> > API.
> 
> I'm not proposing to kill that API, I just think the existing targets
> fall short or are under-specified when applied to RAW image sensor,
> that's it :)

There's a bit difference between what is exposed by a V4L2 video capture
device in video-node-centric designs, and by a camera sensor subdev in
MC-centric designs.

In the first case, the video node exposes a logical view of the device,
with the ability to scale and crop if supported, but without any control
on how and where those operations are performed.  When userspace wants
to crop the image, a USB webcam, an embedded system camera (for lack of
a better term, I mean here a camera sensor and a receiver, both being
controllable by Linux) or an analog capture device are exposed
similarly, without applications having to care much about the device
type (there are of course device-specific details exposed, such as the
analog or digital TV standards, but that's separate).

Note that in the USB case, it's common for UVC webcams to expose a set
of resolutions, and use a combination of scaling and cropping to achieve
them, without reporting to the host how much cropping is applied. The
UVC standard allows webcams to crop internally, and doesn't expose any
cropping control, so the UVC driver doesn't implement control of the
crop rectangle. This is an extreme case that shows how userspace has
little control of the internal configuration of the device. Reading
optical black pixels is clearly out of the picture with this API, the
video-node-centric device is really a simple, high-level view of a
logical device. It's good enough for many devices.

In the MC-centric case, applications need full control of all processing
steps, and that's where getting detailed information about the crop
rectangles, and the ability to control them all (including up to 3
different stages of cropping in SMIA++ devices for instance) is
important. As Jacopo explained above, the lens shading correction map is
calculated based on how much cropping has been applied. Here we're
talking about specialized userspace, with the main user being
libcamera. libcamera abstracts all this, and exposes a simpler, logical
view of the device to applications. In effect, it turns an MC-centric
kernel device into a simpler logical device (but with extra features
compared to video-node-centric devices, such as a request-based API, and
the ability to have some more control over the ISP).

For this kind of userspace, the V4L2 video API was not enough, and we
have introduced the MC API and the V4L2 subdev API. The lack of a
userspace stack on top of that, a long term issue that we're fixing now,
has prevented us to see the still numerous shortcomings of our kernel
APIs. Vendors who have been using MC-centric drivers to implement
proprietary camera stacks have thus resorted to either out-of-tree
API extentions or abuses of the existing interfaces. For instance, I've
seen out-of-tree drivers using the v4l2_streamparm.capture.capturemode
field returned by VIDIOC_G_PARM to retrieve the resolution and crop
configuration of the camera sensor by indexing in a table of modes in
userspace. There are lots of examples of horrible code, which shows the
need not only for API extensions, but for standardizing through
documentation how the existing APIS are used.

This is what Jacopo is trying to address with this series. I however
don't think we need to define extra selection targets (but I'm open to
considering that if there are good reasons I'm not aware of), but we
very much need to describe in details how the selection targets apply to
camera sensor subdevs. This is separate from the V4L2 video API, we may
also need better descriptions there, but that should be handled
separately. The base concepts and the targets are shared between the two
cases, but that's where it stops.

> >>> There is nothing subdev specific to this API.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/4] media: docs: Describe pixel array properties

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Aug 05, 2020 at 12:57:18PM +0200, Jacopo Mondi wrote:
> The V4L2 selection API are also used to access the pixel array
> properties of an image sensor, such as the size and position of active
> pixels and the cropped area of the pixel matrix used to produce images.
> 
> Currently no clear definition of the different areas that compose an
> image sensor pixel array matrix is provided in the specification, and
> the actual meaning of each selection target when applied to an image
> sensor was not provided.
> 
> Provide in the sub-device documentation the definition of the pixel
> matrix properties and the selection target associated to each of them.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  .../userspace-api/media/v4l/dev-subdev.rst    | 81 +++++++++++++++++++
>  1 file changed, 81 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> index 134d2fb909fa4..c47861dff9b9b 100644
> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.
>  ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round
>  the image size either up or down. :ref:`v4l2-selection-flags`
>  
> +.. _v4l2-subdev-pixel-array-properties:
> +
> +Selection targets for image sensors properties
> +----------------------------------------------
> +
> +The V4L2 selection API can be used on sub-devices that represent an image
> +sensor to retrieve the sensor's pixel array matrix properties by using the
> +:ref:`selection <VIDIOC_SUBDEV_G_SELECTION>` ioctls.
> +
> +Sub-device drivers for image sensor usually register a single source pad, but in
> +the case they expose more, the pixel array properties can be accessed from
> +any of them.
> +
> +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,
> +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve
> +the immutable properties of the several different areas that compose the sensor
> +pixel array matrix. Each area describes a rectangle of logically adjacent pixel
> +units. The logical disposition of pixels is defined by the sensor read-out
> +starting point and direction, and may differ from the physical disposition of
> +the pixel units in the pixel array matrix.
> +
> +Each pixel matrix portion is contained in a larger rectangle, with the most
> +largest being the one that describes the pixel matrix physical size. This
> +defines a hierarchical positional system, where each rectangle is defined
> +relatively to the largest available one among the ones exposed by the
> +sub-device driver. Each selection target and the associated pixel array portion
> +it represents are below presented in order from the largest to the smallest one.
> +
> +Pixel array physical size
> +^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +The image sensor chip is composed by a number of physical pixels, not all of
> +them readable by the application processor. Invalid or unreadable lines might
> +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors
> +they might be tagged with an invalid data type (DT) so that the receiver
> +automatically discard them. The size of the whole pixel matrix area is
> +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner
> +defined as position (0, 0). All the other selection targets are defined
> +relatively to this, larger, rectangle. The rectangle returned by
> +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it
> +does not change at run-time and cannot be modified from userspace.

As I think I've mentioned previously (not sure if it was by e-mail or on
IRC), we could also decide to set V4L2_SEL_TGT_NATIVE_SIZE ==
V4L2_SEL_TGT_CROP_BOUNDS by ignoring the non-readable pixels completely.
What's the advantage of exposing them in the API, when the sensors
doesn't provide them to the rest of the pipeline ?

> +Pixel array readable area
> +^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +The V4L2_SEL_TGT_CROP_BOUNDS targets returns size and position of the readable
> +area of the pixel array matrix, including pixels with valid image data and pixel
> +used for calibration purposes, such as optical black pixels. It is not unlikely
> +that valid pixels and optical black pixels are surrounded by non-readable rows
> +and columns of pixels. Those does not concur in the definition of the
> +V4L2_SEL_TGT_CROP_BOUNDS rectangle. The rectangle returned by
> +V4L2_SEL_TGT_CROP_BOUNDS describes an immutable property of the image sensor, it
> +does not change at run-time and cannot be modified from userspace.
> +
> +Pixel array active area
> +^^^^^^^^^^^^^^^^^^^^^^^
> +
> +The portion of the pixel array which contains valid image data is defined as the
> +active area of the pixel matrix. The active pixel array is is accessed by mean
> +of the V4L2_SEL_TGT_CROP_DEFAULT target, and is contained in the larger
> +V4L2_SEL_TGT_CROP_BOUNDS rectangle. It represents the largest possible frame
> +resolution the sensor can produce and defines the dimension of the full
> +field-of-view. The rectangle returned by V4L2_SEL_TGT_CROP_BOUNDS describes an
> +immutable property of the image sensor, it does not change at run-time and
> +cannot be modified from userspace.
> +
> +Analog crop rectangle
> +^^^^^^^^^^^^^^^^^^^^^
> +
> +The sensor driver might decide, in order to adjust the image resolution to best
> +match the one requested by applications, to only process a part of the active
> +pixel array matrix. The selected area is read-out and processed by the image
> +sensor on-board ISP in order to produce images of the desired size and
> +resolution while possible maintaing the largest possible field-of-view. The
> +cropped portion of the pixel array which is used to produce images is returned
> +by the V4L2_SEL_TGT_CROP target and represent the only information that can
> +change at runtime as it depends on the currently configured sensor mode and
> +desired image resolution. If the sub-device driver supports that, userspace
> +can set the analog crop rectangle to select which portion of the pixel array
> +to read out.
> +
>  
>  Types of selection targets
>  --------------------------

-- 
Regards,

Laurent Pinchart

Re: your mail

[Jacopo Mondi]

Hi Laurent,

On Sun, Aug 09, 2020 at 06:53:11PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Wed, Aug 05, 2020 at 12:57:17PM +0200, Jacopo Mondi wrote:
> > Subject: [PATCH 0/4] media: docs: Document pixel array properties
> >
> > Hans' patch "[PATCH] imx219: selection compliance fixes" sparkled a discussion
> > on how the V4L2 selection targets have to be used in order to access an
> > image sensor pixel array properties.
> >
> > The discussion shown how much under-specified that part was, so this is
> > an attempt to provide a bit documentation for this.
> >
> > My feeling is that we're hijacking the existing targets for this use case
> > and we should probably define new ones, considering how few users we have in
> > mainline of them at the moment.
>
> Do you mean you think we should create new targets instead of reusing
> the existing ones as you do in this series ? I don't see anything really
> wrong in reusing the existing targets, provided we document them
> properly, and it's not too much of a hack (that is, the documented
> purpose reasonably matches the target name), but maybe I'm missing the
> issue.

Yes, that's what I meant.

To me, if you have something and you need to document it with "if
applied to X it means Y, if applied to Z it means W etcetc" feels like
we're abusing it. Having sensor's specific targets would make clear
what a target represents without the ambiguities of defining the
symbol semantic depending on the device it applies to (which means
userspace would need to know what kind of device it's dealing with
precisely).

That said, my preference would be using a different API, but see my
reply to your other email for that.

Thanks
  j

>
> > On top Hans' patch with reworded commit message and minor updates.
> >
> > For reference, we're using the V4L2 selection targets in libcamera to retrieve
> > the sensor pixel array properties to be reported to applications for
> > calibration purposes. The currently defined pixel properties for libcamera
> > are available here:
> > https://git.linuxtv.org/libcamera.git/tree/src/libcamera/property_ids.yaml#n390
> >
> > Thanks
> >    j
> >
> > Hans Verkuil (1):
> >   media: i2c: imx219: Selection compliance fixes
> >
> > Jacopo Mondi (3):
> >   media: docs: Describe pixel array properties
> >   media: docs: Describe targets for sensor properties
> >   media: docs: USe SUBDEV_G_SELECTION for sensor properties
> >
> >  .../userspace-api/media/v4l/dev-subdev.rst    | 85 +++++++++++++++++++
> >  .../media/v4l/v4l2-selection-targets.rst      | 49 +++++++++++
> >  .../media/v4l/vidioc-subdev-g-selection.rst   |  4 +
> >  drivers/media/i2c/imx219.c                    | 17 ++--
> >  4 files changed, 147 insertions(+), 8 deletions(-)
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH 1/4] media: docs: Describe pixel array properties

[Jacopo Mondi]

Hi Sakari,
    thanks for feedback.

On Thu, Aug 06, 2020 at 01:24:47AM +0300, Sakari Ailus wrote:
> Hi Jacopo,
>
> Thanks for the patchset.
>
> This improves selection documentation quite a bit. Please see my comments
> below.
>

Thanks. Laurent just mentioned your effort on documenting sensor
driver drivers, I would be happy to somehow merge the two efforts.

> On Wed, Aug 05, 2020 at 12:57:18PM +0200, Jacopo Mondi wrote:
> > The V4L2 selection API are also used to access the pixel array
> > properties of an image sensor, such as the size and position of active
> > pixels and the cropped area of the pixel matrix used to produce images.
> >
> > Currently no clear definition of the different areas that compose an
> > image sensor pixel array matrix is provided in the specification, and
> > the actual meaning of each selection target when applied to an image
> > sensor was not provided.
> >
> > Provide in the sub-device documentation the definition of the pixel
> > matrix properties and the selection target associated to each of them.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  .../userspace-api/media/v4l/dev-subdev.rst    | 81 +++++++++++++++++++
> >  1 file changed, 81 insertions(+)
> >
> > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > index 134d2fb909fa4..c47861dff9b9b 100644
> > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.
> >  ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round
> >  the image size either up or down. :ref:`v4l2-selection-flags`
> >
> > +.. _v4l2-subdev-pixel-array-properties:
> > +
> > +Selection targets for image sensors properties
> > +----------------------------------------------
> > +
> > +The V4L2 selection API can be used on sub-devices that represent an image
> > +sensor to retrieve the sensor's pixel array matrix properties by using the
> > +:ref:`selection <VIDIOC_SUBDEV_G_SELECTION>` ioctls.
> > +
> > +Sub-device drivers for image sensor usually register a single source pad, but in
> > +the case they expose more, the pixel array properties can be accessed from
> > +any of them.
>
> Is this a hypothetical case or are there examples?
>
> Also note that camera sensor drivers may expose more than one sub-devices,
> only one of which represents the pixel array.
>

Yes, I just tried to mention that as a possible use case, but I admit
I might be a bit confused about that. I would, as suggested by
Laurent, drop this part and add it back when we'll have a case at
hands.

> > +
> > +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,
> > +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve
> > +the immutable properties of the several different areas that compose the sensor
> > +pixel array matrix. Each area describes a rectangle of logically adjacent pixel
> > +units. The logical disposition of pixels is defined by the sensor read-out
> > +starting point and direction, and may differ from the physical disposition of
> > +the pixel units in the pixel array matrix.
> > +
> > +Each pixel matrix portion is contained in a larger rectangle, with the most
>
> s/larger\K/ or equal/
>
> s/most//
>
> > +largest being the one that describes the pixel matrix physical size. This
> > +defines a hierarchical positional system, where each rectangle is defined
>
> s/,//
>

Ack on both sections.

> > +relatively to the largest available one among the ones exposed by the
> > +sub-device driver. Each selection target and the associated pixel array portion
> > +it represents are below presented in order from the largest to the smallest one.
> > +
> > +Pixel array physical size
> > +^^^^^^^^^^^^^^^^^^^^^^^^^
> > +
> > +The image sensor chip is composed by a number of physical pixels, not all of
> > +them readable by the application processor. Invalid or unreadable lines might
> > +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors
> > +they might be tagged with an invalid data type (DT) so that the receiver
> > +automatically discard them. The size of the whole pixel matrix area is
> > +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner
> > +defined as position (0, 0). All the other selection targets are defined
> > +relatively to this, larger, rectangle. The rectangle returned by
> > +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it
> > +does not change at run-time and cannot be modified from userspace.
> > +
> > +Pixel array readable area
> > +^^^^^^^^^^^^^^^^^^^^^^^^^
> > +
> > +The V4L2_SEL_TGT_CROP_BOUNDS targets returns size and position of the readable
> > +area of the pixel array matrix, including pixels with valid image data and pixel
> > +used for calibration purposes, such as optical black pixels. It is not unlikely
> > +that valid pixels and optical black pixels are surrounded by non-readable rows
> > +and columns of pixels. Those does not concur in the definition of the
>
> How about: "Only pixels that can be read out are included in the
> V4L2_SEL_TGT_CROP_BOUNDS rectangle."?
>

I'll more extensively reply on this to laurent's comment on the "Pixel
array physical size" section, as it also address this comment you have
here. There's a SMIA++ question for you there :)

> > +V4L2_SEL_TGT_CROP_BOUNDS rectangle. The rectangle returned by
> > +V4L2_SEL_TGT_CROP_BOUNDS describes an immutable property of the image sensor, it
> > +does not change at run-time and cannot be modified from userspace.
> > +
> > +Pixel array active area
> > +^^^^^^^^^^^^^^^^^^^^^^^
> > +
> > +The portion of the pixel array which contains valid image data is defined as the
> > +active area of the pixel matrix. The active pixel array is is accessed by mean
>
> s/accessed/described/
>
> Another word than "active" here would be great as we already have active
> and try contexts for selections.
>

I feel like we would need to define a stricter gloassary as well.
To replace active I would usually say "Valid for image capture",
"pixel that contain valid image data". But they're both longer

> > +of the V4L2_SEL_TGT_CROP_DEFAULT target, and is contained in the larger
>
> s/the larger//
>
> > +V4L2_SEL_TGT_CROP_BOUNDS rectangle. It represents the largest possible frame
> > +resolution the sensor can produce and defines the dimension of the full
>
> s/resolution/size/
>
> > +field-of-view. The rectangle returned by V4L2_SEL_TGT_CROP_BOUNDS describes an
>
> s/-/ /g
>
> > +immutable property of the image sensor, it does not change at run-time and
> > +cannot be modified from userspace.
> > +
> > +Analog crop rectangle
> > +^^^^^^^^^^^^^^^^^^^^^
> > +
> > +The sensor driver might decide, in order to adjust the image resolution to best
> > +match the one requested by applications, to only process a part of the active
>
> s/, to\K/ instruct the hardware to/
>
> > +pixel array matrix. The selected area is read-out and processed by the image
> > +sensor on-board ISP in order to produce images of the desired size and
> > +resolution while possible maintaing the largest possible field-of-view. The
>
> s/size\K[^.]?*\./m
>
> > +cropped portion of the pixel array which is used to produce images is returned
>
> s/produce/read out/
> s/returned/configured/
>
> > +by the V4L2_SEL_TGT_CROP target and represent the only information that can
>
> s/by/using/
>
> I'd leave out the rest of the sentence after "target" above.

I added this as I initially listed CROP in the immutable targets, and
I was trying to specify it's actually not here. I'll remove the first
ambiguity then I could drop this patch.

>
> > +change at runtime as it depends on the currently configured sensor mode and
> > +desired image resolution. If the sub-device driver supports that, userspace
> > +can set the analog crop rectangle to select which portion of the pixel array
> > +to read out.
>
> How about instead:
>
> Register list based drivers generally do not allow setting analogue crop
> rectangles.
>

I could do that. I'm always a bit uncertain on mentioning 'register
list drivers', 'mode-based drivers' etc as we don't have a real
definition of them in the documentation..

Thanks
  j


> > +
> >
> >  Types of selection targets
> >  --------------------------
>
> --
> Kind regards,
>
> Sakari Ailus

Re: [PATCH 1/4] media: docs: Describe pixel array properties

[Jacopo Mondi]

Hi Sakari, Laurent
    tanks for comments.


On Sun, Aug 09, 2020 at 08:17:57PM +0300, Laurent Pinchart wrote:
> Hi Jacopo, Hans,
>
> On Thu, Aug 06, 2020 at 11:58:31AM +0200, Hans Verkuil wrote:
> > On 06/08/2020 11:50, Jacopo Mondi wrote:
> > > On Thu, Aug 06, 2020 at 10:05:37AM +0200, Hans Verkuil wrote:
> > >> On 05/08/2020 12:57, Jacopo Mondi wrote:
> > >>> The V4L2 selection API are also used to access the pixel array
> > >>
> > >> are -> is
> > >>
> > >>> properties of an image sensor, such as the size and position of active
> > >>> pixels and the cropped area of the pixel matrix used to produce images.
> > >>>
> > >>> Currently no clear definition of the different areas that compose an
> > >>> image sensor pixel array matrix is provided in the specification, and
> > >>> the actual meaning of each selection target when applied to an image
> > >>> sensor was not provided.
> > >>>
> > >>> Provide in the sub-device documentation the definition of the pixel
> > >>> matrix properties and the selection target associated to each of them.
> > >>>
> > >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > >>> ---
> > >>>  .../userspace-api/media/v4l/dev-subdev.rst    | 81 +++++++++++++++++++
> > >>>  1 file changed, 81 insertions(+)
> > >>>
> > >>> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > >>> index 134d2fb909fa4..c47861dff9b9b 100644
> > >>> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > >>> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > >>> @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.
> > >>>  ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round
> > >>>  the image size either up or down. :ref:`v4l2-selection-flags`
> > >>>
> > >>> +.. _v4l2-subdev-pixel-array-properties:
> > >>> +
> > >>> +Selection targets for image sensors properties
>
> Maybe "Selection targets for image sensors", and renaming the reference
> to v4l2-subdev-selections-image-sensors ? This section is about how
> selection rectangles are used for sensors, right ?
>
> > >>> +----------------------------------------------
>
> I'd move this further down, after "Types of selection targets", as that
> section contains generic information that applies to sensors too.

Ack on both points.

>
> > >>> +
> > >>> +The V4L2 selection API can be used on sub-devices that represent an image
> > >>> +sensor to retrieve the sensor's pixel array matrix properties by using the
> > >>> +:ref:`selection <VIDIOC_SUBDEV_G_SELECTION>` ioctls.
> > >>> +
> > >>> +Sub-device drivers for image sensor usually register a single source pad, but in
> > >>> +the case they expose more, the pixel array properties can be accessed from
> > >>> +any of them.
>
> Is this right ? I don't think the V4L2_SEL_TGT_CROP rectangle, for
> instance, can be accessed from any source pad indifferently. Do we have
> sensor drivers that create multiple source pads in a subdev today ? If
> not I'd suggest dropping this, and adding it later if needed (when we'll
> have a better idea of how that would work).

Yes, this was meant to cover cases which I still have not a clear idea
about but I suspect people might have question about looking at their
drivers. I'm totally fine adding it later when required.

>
> I think what should be explained here, as also mentioned by Sakari, is
> that camera sensors can be exposed as multiple subdevs. The text below
> is related to the pixel array, which is always the first subdev, with
> one source pad and no sink pad. The other subdevs, modelling additional
> processing blocks in the sensor, may use the selection API, but that's
> out of scope for this patch.
>
> As we'll also need to document how other subdevs use the selection API,
> as well as how sensors are usually handled, would it make sense to move
> this to a separate file ? Sakari has proposed in [1] to create a new
> Documentation/driver-api/media/camera-sensor.rst file. It would make
> sense to centralize all sensor information there. This doesn't mean this
> series should depend on Sakari's patch, we can handle merge conflicts
> depending on what gets merged first.

I totally missed that one but I equally totally welcome that change. I
would be happy to rebase this work on top of Sakari's patch which I
will soon give a read to.

>
> [1] https://lore.kernel.org/linux-media/20200730162040.15560-1-sakari.ailus@linux.intel.com/
>
> > >>> +
> > >>> +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,
> > >>
> > >> V4L2_SEL_TGT_NATIVE -> V4L2_SEL_TGT_NATIVE_SIZE
> > >>
> > >> (same mistake is made elsewhere).
> > >
> > > Ah ups, I used TGT_NATIVE consistently, seems like I thought that was
> > > the right name
> > >
> > >>> +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve
> > >>> +the immutable properties of the several different areas that compose the sensor
> > >>> +pixel array matrix. Each area describes a rectangle of logically adjacent pixel
>
> V4L2_TGT_CROP isn't immutable, is it ?
>

Right, I noticed that, but didn't want to be too heavy with createing
a special section for CROP. But you're right, it's not immutable so it
should not be mentioned here.

> > >>> +units. The logical disposition of pixels is defined by the sensor read-out
> > >>> +starting point and direction, and may differ from the physical disposition of
> > >>> +the pixel units in the pixel array matrix.
> > >>> +
> > >>> +Each pixel matrix portion is contained in a larger rectangle, with the most
> > >>> +largest being the one that describes the pixel matrix physical size. This
> > >>> +defines a hierarchical positional system, where each rectangle is defined
> > >>> +relatively to the largest available one among the ones exposed by the
> > >>> +sub-device driver. Each selection target and the associated pixel array portion
> > >>> +it represents are below presented in order from the largest to the smallest one.
>
> I find this quite confusing. As Hans suggested, I think each target
> should define its boundaries. I'd drop this paragraph completely, as you
> already explain below that all rectangles are defined relatively to
> V4L2_SEL_TGT_NATIVE_SIZE.
>

Ack.

> > >>> +
> > >>> +Pixel array physical size
> > >>> +^^^^^^^^^^^^^^^^^^^^^^^^^
> > >>> +
> > >>> +The image sensor chip is composed by a number of physical pixels, not all of
> > >>> +them readable by the application processor. Invalid or unreadable lines might
> > >>> +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors
> > >>> +they might be tagged with an invalid data type (DT) so that the receiver
> > >>> +automatically discard them.
>
> "might" is a bit weak for unreadable lines, there's no way they can be
> transmitted if they can't be read :-)

This paragraph reflects my confusion on the subject. My interpretation
is that for CSI-2 sensors, you cannot crop a black pixels area and
capture just them. At the contrary, the black pixels are sent on the
bus (maybe that can be optionally enabled/disabled) tagged with a
special DT and interleaved with image data and you have to instruct
your receiver to discard or accept that DT, and have black pixels
captured to a separate memory area, or at buffer end (that depends on
the receiver's architecture I guess). At the same time, I won't be
surprised if some sensor's allow you to explicitly crop on black
pixels areas and only put them on the bus. Knowing how the SMIA++
standard handles that part might help establishing an expected
behaviour (I really, really, wish we had as a community any leverage
to influence sensor manufacturer towards a standardized behaviour, it
would be time for the industry to do so. </wishful thinking>).

If that's correct, I wonder how would that possibly work with parallel
sensors, where you cannot tag data on the bus. I there assume you have
to explicitly select the black region to capture.

I there tried to, confusingly, express that different behaviour and
stay as much as possible generic not to rule out any existing case.

>
> One way to generalize this a bit would be to explain, after the first
> sentence, that not all pixels may be read by the sensor, that among the
> pixels that are read invalid ones may not be transmitted on the bus, and
> that among transmitted pixels not all of them may be possible to capture
> on the receiver side. For instance, invalid lines may be transmitted as
> part of the vertical blanking on parallel buses, or tagged as blanking
> data or null data on CSI-2 buses. Most receivers are not able to capture
> either.
>
> (On a side note, strictly speaking, a CSI-2 receiver that would be able
> to capture null or blanking packets wouldn't be compliant with the CSI-2
> spec.)
>
> > >>> The size of the whole pixel matrix area is
> > >>> +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner
> > >>> +defined as position (0, 0). All the other selection targets are defined
> > >>> +relatively to this, larger, rectangle. The rectangle returned by
>
> s/, larger,/
>
> > >>> +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it
> > >>> +does not change at run-time and cannot be modified from userspace.
> > >>
> > >> It is a good idea to mention that if there are no invalid or unreadable pixels/lines,
> > >> then V4L2_SEL_TGT_NATIVE_SIZE == V4L2_SEL_TGT_CROP_BOUNDS.
> > >
> > > Yes it is! I'll add it here
>
> Should it be added below instead, where you define
> V4L2_SEL_TGT_CROP_BOUNDS ? It's best to avoid mentioning something that
> isn't defined yet when possible.
>

I have a question for Sakari on this target, but I'll deflect it to
the reply to your comment  on patch 1/4.

> > >>> +
> > >>> +Pixel array readable area
> > >>> +^^^^^^^^^^^^^^^^^^^^^^^^^
> > >>> +
> > >>> +The V4L2_SEL_TGT_CROP_BOUNDS targets returns size and position of the readable
> > >>> +area of the pixel array matrix, including pixels with valid image data and pixel
> > >>> +used for calibration purposes, such as optical black pixels. It is not unlikely
>
> s/not unlikely/likely ? Or just "common".
>
> > >>> +that valid pixels and optical black pixels are surrounded by non-readable rows
> > >>> +and columns of pixels. Those does not concur in the definition of the
>
> s/does/do/
>
> I'm not sure "concur" is the right word. Did you mean "those are not
> part of the V4L2_SEL_TGT_CROP_BOUNDS rectangle" ?

Yes, I meant they should not be counted in the definition of the BOUND
rectangle sizes.

>
> > >>> +V4L2_SEL_TGT_CROP_BOUNDS rectangle. The rectangle returned by
> > >>> +V4L2_SEL_TGT_CROP_BOUNDS describes an immutable property of the image sensor, it
> > >>> +does not change at run-time and cannot be modified from userspace.
> > >>
> > >> Mention that BOUNDS is enclosed by NATIVE_SIZE.
> > >
> > > I tried to express that in the intro section with
> > >
> > > "Each pixel matrix portion is contained in a larger rectangle, with the most
> > > largest being the one that describes the pixel matrix physical size."
> > >
> > > But I guess it's worth to express that for each target!
> > >
> > >>> +
> > >>> +Pixel array active area
> > >>> +^^^^^^^^^^^^^^^^^^^^^^^
> > >>> +
> > >>> +The portion of the pixel array which contains valid image data is defined as the
> > >>> +active area of the pixel matrix. The active pixel array is is accessed by mean
> > >>
>
> s/is is/is/
>
> > >> mean -> means
> > >>
> > >>> +of the V4L2_SEL_TGT_CROP_DEFAULT target, and is contained in the larger
> > >>> +V4L2_SEL_TGT_CROP_BOUNDS rectangle. It represents the largest possible frame
> > >>> +resolution the sensor can produce and defines the dimension of the full
> > >>> +field-of-view. The rectangle returned by V4L2_SEL_TGT_CROP_BOUNDS describes an
> > >>
> > >> BOUNDS -> DEFAULT
> > >
> > > ups
> > >
> > >>> +immutable property of the image sensor, it does not change at run-time and
> > >>> +cannot be modified from userspace.
> > >>
> > >> Mention that CROP_DEFAULT is enclosed by CROP_BOUNDS
> > >>
> > >>> +
> > >>> +Analog crop rectangle
> > >>
> > >> Why analog? It's just the crop rectangle, nothing analog about it.
> > >
> > > We used the 'analogCrop' term in libcamera to differentiate the
> > > cropping which happens on the sensor pixel array matrix to select the
> > > region to process and produce image from. Sensor with an on-board
> > > scaler can perform other cropping steps to implement, in example digital
> > > zoom, so we expect to have a 'digital crop' phase as well. RAW
> > > sensors, in example, will only have an analogCrop rectangle.
> > >
> > > Quoting the libcamera definition of analog crop:
> > >
> > >  * horizontal and vertical sizes define the portion of the pixel array which
> > >  * is read-out and provided to the sensor's internal processing pipeline, before
> > >  * any pixel sub-sampling method, such as pixel binning, skipping and averaging
> > >  * take place.
> > >
> > > should I keep it or remove it ?
> >
> > It's a very confusing term. Especially since this API can also be used with analog
> > video capture devices (Composite/S-Video) where the video signal actually is analog.
> >
> > In the V4L2 API there is no such thing as 'analog crop', so please remove it.
>
> Jacopo is right, sensors usually perform cropping in the analog domain
> (by not reading out all pixels from the pixel array), and also support
> cropping in later stages, after binning/skipping, and after further
> scaling. Note that all of these crop operations are optional. Although
> not common, it's also not unconceivable that a sensor wouldn't support
> cropping at all.
>
> This being said, it only makes sense to talk about analog crop when
> multiple crop operations are performed, and thus in the context of the
> whole sensor, with multiple subdevs. If we explain this, as proposed
> above, and make it clear that the usage of the selection rectangles
> defined here applies to the pixel array only, we can drop the "analog"
> term, and just talk about cropping in the pixel array.
>

It's fine as long as it removes any ambiguity.

> > >>> +^^^^^^^^^^^^^^^^^^^^^
> > >>> +
> > >>> +The sensor driver might decide, in order to adjust the image resolution to best
> > >>> +match the one requested by applications, to only process a part of the active
> > >>> +pixel array matrix.
>
> I don't think that's the right approach. With MC-based devices, the
> philosophy is to expose all configuration parameters to userspace. It's
> not about sensor drivers making decisions, but about userspace deciding
> to crop at the pixel array level.
>
> This being said, I'm aware the decision is made by drivers when they're
> mode-based. Please see below for that.

Correct, and I should now be used enough to the 'userspace drives'
approach to remember about documenting it :)

>
> > >>> The selected area is read-out and processed by the image
> > >>> +sensor on-board ISP in order to produce images of the desired size and
> > >>> +resolution while possible maintaing the largest possible field-of-view. The
> > >>
> > >> maintaing -> maintaining
> > >>
> > >> Actually, I'd drop 'while possible maintaing the largest possible field-of-view'
> > >> entirely. It doesn't make much sense.
> > >
> > > Ack
>
> In general, in this section, as we're documenting the pixel array, let's
> not talk about the ISP.
>
> > >>> +cropped portion of the pixel array which is used to produce images is returned
> > >>> +by the V4L2_SEL_TGT_CROP target and represent the only information that can
> > >>
> > >> represent -> represents
> > >>
> > >>> +change at runtime as it depends on the currently configured sensor mode and
> > >>> +desired image resolution. If the sub-device driver supports that, userspace
> > >>> +can set the analog crop rectangle to select which portion of the pixel array
> > >>
> > >> s/analog//
> > >>
> > >>> +to read out.
>
> I think it's better to focus on the best case, and document usage of
> crop rectangles in general first, for drivers that expose full
> configurability of the sensor. A separate section should then then make
> a note of how mode-based drivers differ, which is mostly in the
> V4L2_SEL_TGT_CROP target being read-only, and on the single subdev
> hiding all the processing steps, with the crop target thus being the
> result of all cropping operations, analog and digital.
>
> Sakari's patch has a bit of information about this, it may be useful to
> reuse it or integrate with it somehow.
>

I'll try to see how the two parts can be piled one on top of the
other.

Thanks
  j


> > >> Mention that CROP is enclosed by CROP_BOUNDS and defaults to CROP_DEFAULT.
> > >>
> > >> Make a note that CROP can also be used to obtain optical black pixels.
> > >
> > > What about:
> > >
> > > +desired image resolution. If the sub-device driver supports that, userspace
> > > +can set the analog crop rectangle to select which portion of the pixel array
> > > +to read out including, if supported, optical black pixels.
> >
> > Hmm, that's a bit awkward. How about:
> >
> > +desired image resolution. If supported by the sub-device driver, userspace
> > +can set the crop rectangle to select which portion of the pixel array
> > +to read out. This may include optical black pixels if those are part of
> > +V4L2_SEL_TGT_CROP_BOUNDS.
> >
> > >>> +
> > >>>
> > >>>  Types of selection targets
> > >>>  --------------------------
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH 1/4] media: docs: Describe pixel array properties

[Jacopo Mondi]

On Sun, Aug 09, 2020 at 08:58:21PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Aug 05, 2020 at 12:57:18PM +0200, Jacopo Mondi wrote:
> > The V4L2 selection API are also used to access the pixel array
> > properties of an image sensor, such as the size and position of active
> > pixels and the cropped area of the pixel matrix used to produce images.
> >
> > Currently no clear definition of the different areas that compose an
> > image sensor pixel array matrix is provided in the specification, and
> > the actual meaning of each selection target when applied to an image
> > sensor was not provided.
> >
> > Provide in the sub-device documentation the definition of the pixel
> > matrix properties and the selection target associated to each of them.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  .../userspace-api/media/v4l/dev-subdev.rst    | 81 +++++++++++++++++++
> >  1 file changed, 81 insertions(+)
> >
> > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > index 134d2fb909fa4..c47861dff9b9b 100644
> > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.
> >  ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round
> >  the image size either up or down. :ref:`v4l2-selection-flags`
> >
> > +.. _v4l2-subdev-pixel-array-properties:
> > +
> > +Selection targets for image sensors properties
> > +----------------------------------------------
> > +
> > +The V4L2 selection API can be used on sub-devices that represent an image
> > +sensor to retrieve the sensor's pixel array matrix properties by using the
> > +:ref:`selection <VIDIOC_SUBDEV_G_SELECTION>` ioctls.
> > +
> > +Sub-device drivers for image sensor usually register a single source pad, but in
> > +the case they expose more, the pixel array properties can be accessed from
> > +any of them.
> > +
> > +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,
> > +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve
> > +the immutable properties of the several different areas that compose the sensor
> > +pixel array matrix. Each area describes a rectangle of logically adjacent pixel
> > +units. The logical disposition of pixels is defined by the sensor read-out
> > +starting point and direction, and may differ from the physical disposition of
> > +the pixel units in the pixel array matrix.
> > +
> > +Each pixel matrix portion is contained in a larger rectangle, with the most
> > +largest being the one that describes the pixel matrix physical size. This
> > +defines a hierarchical positional system, where each rectangle is defined
> > +relatively to the largest available one among the ones exposed by the
> > +sub-device driver. Each selection target and the associated pixel array portion
> > +it represents are below presented in order from the largest to the smallest one.
> > +
> > +Pixel array physical size
> > +^^^^^^^^^^^^^^^^^^^^^^^^^
> > +
> > +The image sensor chip is composed by a number of physical pixels, not all of
> > +them readable by the application processor. Invalid or unreadable lines might
> > +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors
> > +they might be tagged with an invalid data type (DT) so that the receiver
> > +automatically discard them. The size of the whole pixel matrix area is
> > +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner
> > +defined as position (0, 0). All the other selection targets are defined
> > +relatively to this, larger, rectangle. The rectangle returned by
> > +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it
> > +does not change at run-time and cannot be modified from userspace.
>
> As I think I've mentioned previously (not sure if it was by e-mail or on
> IRC), we could also decide to set V4L2_SEL_TGT_NATIVE_SIZE ==
> V4L2_SEL_TGT_CROP_BOUNDS by ignoring the non-readable pixels completely.
> What's the advantage of exposing them in the API, when the sensors
> doesn't provide them to the rest of the pipeline ?
>

I don't know :) I'm also  bit confused on what's the purpose of
NATIVE, this commit seems to suggest it was meant to replace
CROP_BOUNDS, but I'm not sure about that.

commit b518d86609cc066b626120fe6ec6fe3a4ccfcd54
Author: Sakari Ailus <sakari.ailus@linux.intel.com>
Date:   Thu Nov 6 16:54:33 2014 -0300

    [media] smiapp: Support V4L2_SEL_TGT_NATIVE_SIZE

    Add support for selection target V4L2_SEL_TGT_NATIVE_SIZE. It is equivalent
    of what V4L2_SEL_TGT_CROP_BOUNDS used to be. Support for
    V4L2_SEL_TGT_CROP_BOUNDS is still supported by the driver as a compatibility
    interface.

Sakari, do you recall if that's was the original plan ?

Thanks
  j

> > +Pixel array readable area
> > +^^^^^^^^^^^^^^^^^^^^^^^^^
> > +
> > +The V4L2_SEL_TGT_CROP_BOUNDS targets returns size and position of the readable
> > +area of the pixel array matrix, including pixels with valid image data and pixel
> > +used for calibration purposes, such as optical black pixels. It is not unlikely
> > +that valid pixels and optical black pixels are surrounded by non-readable rows
> > +and columns of pixels. Those does not concur in the definition of the
> > +V4L2_SEL_TGT_CROP_BOUNDS rectangle. The rectangle returned by
> > +V4L2_SEL_TGT_CROP_BOUNDS describes an immutable property of the image sensor, it
> > +does not change at run-time and cannot be modified from userspace.
> > +
> > +Pixel array active area
> > +^^^^^^^^^^^^^^^^^^^^^^^
> > +
> > +The portion of the pixel array which contains valid image data is defined as the
> > +active area of the pixel matrix. The active pixel array is is accessed by mean
> > +of the V4L2_SEL_TGT_CROP_DEFAULT target, and is contained in the larger
> > +V4L2_SEL_TGT_CROP_BOUNDS rectangle. It represents the largest possible frame
> > +resolution the sensor can produce and defines the dimension of the full
> > +field-of-view. The rectangle returned by V4L2_SEL_TGT_CROP_BOUNDS describes an
> > +immutable property of the image sensor, it does not change at run-time and
> > +cannot be modified from userspace.
> > +
> > +Analog crop rectangle
> > +^^^^^^^^^^^^^^^^^^^^^
> > +
> > +The sensor driver might decide, in order to adjust the image resolution to best
> > +match the one requested by applications, to only process a part of the active
> > +pixel array matrix. The selected area is read-out and processed by the image
> > +sensor on-board ISP in order to produce images of the desired size and
> > +resolution while possible maintaing the largest possible field-of-view. The
> > +cropped portion of the pixel array which is used to produce images is returned
> > +by the V4L2_SEL_TGT_CROP target and represent the only information that can
> > +change at runtime as it depends on the currently configured sensor mode and
> > +desired image resolution. If the sub-device driver supports that, userspace
> > +can set the analog crop rectangle to select which portion of the pixel array
> > +to read out.
> > +
> >
> >  Types of selection targets
> >  --------------------------
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH 1/4] media: docs: Describe pixel array properties

[Sakari Ailus]

On Thu, Aug 06, 2020 at 03:22:34PM +0200, Hans Verkuil wrote:
> On 06/08/2020 14:54, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Thu, Aug 06, 2020 at 11:58:31AM +0200, Hans Verkuil wrote:
> >> On 06/08/2020 11:50, Jacopo Mondi wrote:
> >>> Hi Hans,
> >>>
> >>> On Thu, Aug 06, 2020 at 10:05:37AM +0200, Hans Verkuil wrote:
> >>>> Hi Jacopo,
> >>>>
> >>>> Some review comments below:
> >>>>
> >>>> On 05/08/2020 12:57, Jacopo Mondi wrote:
> >>>>> +Analog crop rectangle
> >>>>
> >>>> Why analog? It's just the crop rectangle, nothing analog about it.
> >>>>
> >>>
> >>> We used the 'analogCrop' term in libcamera to differentiate the
> >>> cropping which happens on the sensor pixel array matrix to select the
> >>> region to process and produce image from. Sensor with an on-board
> >>> scaler can perform other cropping steps to implement, in example digital
> >>> zoom, so we expect to have a 'digital crop' phase as well. RAW
> >>> sensors, in example, will only have an analogCrop rectangle.
> >>>
> >>> Quoting the libcamera definition of analog crop:
> >>>
> >>>  * horizontal and vertical sizes define the portion of the pixel array which
> >>>  * is read-out and provided to the sensor's internal processing pipeline, before
> >>>  * any pixel sub-sampling method, such as pixel binning, skipping and averaging
> >>>  * take place.
> >>>
> >>> should I keep it or remove it ?
> >>
> >> It's a very confusing term. Especially since this API can also be used with analog
> >> video capture devices (Composite/S-Video) where the video signal actually is analog.
> >>
> >> In the V4L2 API there is no such thing as 'analog crop', so please remove it.
> > 
> > There isn't in the V4L2 API but I don't see why we couldn't document it
> > here. Analogue crop is an established term related to raw (perhaps others,
> > too?) camera sensors which describes cropping that is implemented by not
> > reading parts of the pixel array.
> > 
> > As this documentation only applies to camera sensors, I think it's entirely
> > appropriate to document this here, and using that term.
> > 
> 
> It's always been called just 'crop' in the V4L2 API, so renaming it suddenly
> to something else is IMHO confusing. What you can do, however, is that in the

This has been actually implemented a decade ago but it seems the
documentation has either never been there or has disappeared.

Most drivers hide this as they work on the frame interval and output size
alone, ignoring the rest. Despite that, generally camera sensors do have
both analogue and digital cropping capabilities with differing features
(granularity and dependency to frame interval).

> description of the "crop rectangle" you mention that "it is also known as
> "analog crop" in the context of camera sensors.

Just saying it's a crop rectangle isn't exactly wrong but it's incomplete.
The frame interval calculation requires that information so this should be
more than just a side note.

> 
> With perhaps some more extensive explanation of the term.

-- 
Sakari Ailus

Re: [PATCH 1/4] media: docs: Describe pixel array properties

[Sakari Ailus]

Hi Jacopo,

On Mon, Aug 10, 2020 at 10:17:57AM +0200, Jacopo Mondi wrote:
> On Sun, Aug 09, 2020 at 08:58:21PM +0300, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > On Wed, Aug 05, 2020 at 12:57:18PM +0200, Jacopo Mondi wrote:
> > > The V4L2 selection API are also used to access the pixel array
> > > properties of an image sensor, such as the size and position of active
> > > pixels and the cropped area of the pixel matrix used to produce images.
> > >
> > > Currently no clear definition of the different areas that compose an
> > > image sensor pixel array matrix is provided in the specification, and
> > > the actual meaning of each selection target when applied to an image
> > > sensor was not provided.
> > >
> > > Provide in the sub-device documentation the definition of the pixel
> > > matrix properties and the selection target associated to each of them.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  .../userspace-api/media/v4l/dev-subdev.rst    | 81 +++++++++++++++++++
> > >  1 file changed, 81 insertions(+)
> > >
> > > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > index 134d2fb909fa4..c47861dff9b9b 100644
> > > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.
> > >  ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round
> > >  the image size either up or down. :ref:`v4l2-selection-flags`
> > >
> > > +.. _v4l2-subdev-pixel-array-properties:
> > > +
> > > +Selection targets for image sensors properties
> > > +----------------------------------------------
> > > +
> > > +The V4L2 selection API can be used on sub-devices that represent an image
> > > +sensor to retrieve the sensor's pixel array matrix properties by using the
> > > +:ref:`selection <VIDIOC_SUBDEV_G_SELECTION>` ioctls.
> > > +
> > > +Sub-device drivers for image sensor usually register a single source pad, but in
> > > +the case they expose more, the pixel array properties can be accessed from
> > > +any of them.
> > > +
> > > +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,
> > > +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve
> > > +the immutable properties of the several different areas that compose the sensor
> > > +pixel array matrix. Each area describes a rectangle of logically adjacent pixel
> > > +units. The logical disposition of pixels is defined by the sensor read-out
> > > +starting point and direction, and may differ from the physical disposition of
> > > +the pixel units in the pixel array matrix.
> > > +
> > > +Each pixel matrix portion is contained in a larger rectangle, with the most
> > > +largest being the one that describes the pixel matrix physical size. This
> > > +defines a hierarchical positional system, where each rectangle is defined
> > > +relatively to the largest available one among the ones exposed by the
> > > +sub-device driver. Each selection target and the associated pixel array portion
> > > +it represents are below presented in order from the largest to the smallest one.
> > > +
> > > +Pixel array physical size
> > > +^^^^^^^^^^^^^^^^^^^^^^^^^
> > > +
> > > +The image sensor chip is composed by a number of physical pixels, not all of
> > > +them readable by the application processor. Invalid or unreadable lines might
> > > +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors
> > > +they might be tagged with an invalid data type (DT) so that the receiver
> > > +automatically discard them. The size of the whole pixel matrix area is
> > > +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner
> > > +defined as position (0, 0). All the other selection targets are defined
> > > +relatively to this, larger, rectangle. The rectangle returned by
> > > +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it
> > > +does not change at run-time and cannot be modified from userspace.
> >
> > As I think I've mentioned previously (not sure if it was by e-mail or on
> > IRC), we could also decide to set V4L2_SEL_TGT_NATIVE_SIZE ==
> > V4L2_SEL_TGT_CROP_BOUNDS by ignoring the non-readable pixels completely.
> > What's the advantage of exposing them in the API, when the sensors
> > doesn't provide them to the rest of the pipeline ?
> >
> 
> I don't know :) I'm also  bit confused on what's the purpose of
> NATIVE, this commit seems to suggest it was meant to replace
> CROP_BOUNDS, but I'm not sure about that.
> 
> commit b518d86609cc066b626120fe6ec6fe3a4ccfcd54
> Author: Sakari Ailus <sakari.ailus@linux.intel.com>
> Date:   Thu Nov 6 16:54:33 2014 -0300
> 
>     [media] smiapp: Support V4L2_SEL_TGT_NATIVE_SIZE
> 
>     Add support for selection target V4L2_SEL_TGT_NATIVE_SIZE. It is equivalent
>     of what V4L2_SEL_TGT_CROP_BOUNDS used to be. Support for
>     V4L2_SEL_TGT_CROP_BOUNDS is still supported by the driver as a compatibility
>     interface.
> 
> Sakari, do you recall if that's was the original plan ?

That was to denote the size of the pixel array indeed. We didn't discuss
dark or invalid pixels at the time.

So this was just there to tell that it's the pixel array you're cropping
from.

But as long as it's API-wise compatible, I don't think anything prevents
re-purposing this to include other areas. The documentation (AFAIR) does
not say this has to be the same as the crop bounds rectangle.

-- 
Regards,

Sakari Ailus

Re: your mail

[Laurent Pinchart]

Hi Jacopo,

On Mon, Aug 10, 2020 at 09:28:14AM +0200, Jacopo Mondi wrote:
> On Sun, Aug 09, 2020 at 06:53:11PM +0300, Laurent Pinchart wrote:
> > On Wed, Aug 05, 2020 at 12:57:17PM +0200, Jacopo Mondi wrote:
> > > Subject: [PATCH 0/4] media: docs: Document pixel array properties
> > >
> > > Hans' patch "[PATCH] imx219: selection compliance fixes" sparkled a discussion
> > > on how the V4L2 selection targets have to be used in order to access an
> > > image sensor pixel array properties.
> > >
> > > The discussion shown how much under-specified that part was, so this is
> > > an attempt to provide a bit documentation for this.
> > >
> > > My feeling is that we're hijacking the existing targets for this use case
> > > and we should probably define new ones, considering how few users we have in
> > > mainline of them at the moment.
> >
> > Do you mean you think we should create new targets instead of reusing
> > the existing ones as you do in this series ? I don't see anything really
> > wrong in reusing the existing targets, provided we document them
> > properly, and it's not too much of a hack (that is, the documented
> > purpose reasonably matches the target name), but maybe I'm missing the
> > issue.
> 
> Yes, that's what I meant.
> 
> To me, if you have something and you need to document it with "if
> applied to X it means Y, if applied to Z it means W etcetc" feels like
> we're abusing it.

I don't really agree, I think this is common, and I don't really see a
problem. Lots of V4L2 ioctls (or the structures they use) document how
they apply to capture and output devices for instance, and I don't think
that splitting ioctls into _CAPTURE and _OUTPUT variants would help
much.

If you were using a COMPOSE rectangle to specify a crop operation when
used with camera sensors it would of course be an API abuse, but as long
as CROP is crop, I see nothing wrong in differences in details of how
the rectangles apply to different types of devices.

> Having sensor's specific targets would make clear
> what a target represents without the ambiguities of defining the
> symbol semantic depending on the device it applies to (which means
> userspace would need to know what kind of device it's dealing with
> precisely).

Userspace would always need that, in order to pick the applicable
target.

> That said, my preference would be using a different API, but see my
> reply to your other email for that.
> 
> > > On top Hans' patch with reworded commit message and minor updates.
> > >
> > > For reference, we're using the V4L2 selection targets in libcamera to retrieve
> > > the sensor pixel array properties to be reported to applications for
> > > calibration purposes. The currently defined pixel properties for libcamera
> > > are available here:
> > > https://git.linuxtv.org/libcamera.git/tree/src/libcamera/property_ids.yaml#n390
> > >
> > > Thanks
> > >    j
> > >
> > > Hans Verkuil (1):
> > >   media: i2c: imx219: Selection compliance fixes
> > >
> > > Jacopo Mondi (3):
> > >   media: docs: Describe pixel array properties
> > >   media: docs: Describe targets for sensor properties
> > >   media: docs: USe SUBDEV_G_SELECTION for sensor properties
> > >
> > >  .../userspace-api/media/v4l/dev-subdev.rst    | 85 +++++++++++++++++++
> > >  .../media/v4l/v4l2-selection-targets.rst      | 49 +++++++++++
> > >  .../media/v4l/vidioc-subdev-g-selection.rst   |  4 +
> > >  drivers/media/i2c/imx219.c                    | 17 ++--
> > >  4 files changed, 147 insertions(+), 8 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/4] media: docs: Describe pixel array properties

[Laurent Pinchart]

Hi Sakari,

On Tue, Aug 18, 2020 at 11:17:43AM +0300, Sakari Ailus wrote:
> On Mon, Aug 10, 2020 at 10:17:57AM +0200, Jacopo Mondi wrote:
> > On Sun, Aug 09, 2020 at 08:58:21PM +0300, Laurent Pinchart wrote:
> > > On Wed, Aug 05, 2020 at 12:57:18PM +0200, Jacopo Mondi wrote:
> > > > The V4L2 selection API are also used to access the pixel array
> > > > properties of an image sensor, such as the size and position of active
> > > > pixels and the cropped area of the pixel matrix used to produce images.
> > > >
> > > > Currently no clear definition of the different areas that compose an
> > > > image sensor pixel array matrix is provided in the specification, and
> > > > the actual meaning of each selection target when applied to an image
> > > > sensor was not provided.
> > > >
> > > > Provide in the sub-device documentation the definition of the pixel
> > > > matrix properties and the selection target associated to each of them.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > ---
> > > >  .../userspace-api/media/v4l/dev-subdev.rst    | 81 +++++++++++++++++++
> > > >  1 file changed, 81 insertions(+)
> > > >
> > > > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > > index 134d2fb909fa4..c47861dff9b9b 100644
> > > > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > > @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.
> > > >  ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round
> > > >  the image size either up or down. :ref:`v4l2-selection-flags`
> > > >
> > > > +.. _v4l2-subdev-pixel-array-properties:
> > > > +
> > > > +Selection targets for image sensors properties
> > > > +----------------------------------------------
> > > > +
> > > > +The V4L2 selection API can be used on sub-devices that represent an image
> > > > +sensor to retrieve the sensor's pixel array matrix properties by using the
> > > > +:ref:`selection <VIDIOC_SUBDEV_G_SELECTION>` ioctls.
> > > > +
> > > > +Sub-device drivers for image sensor usually register a single source pad, but in
> > > > +the case they expose more, the pixel array properties can be accessed from
> > > > +any of them.
> > > > +
> > > > +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,
> > > > +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve
> > > > +the immutable properties of the several different areas that compose the sensor
> > > > +pixel array matrix. Each area describes a rectangle of logically adjacent pixel
> > > > +units. The logical disposition of pixels is defined by the sensor read-out
> > > > +starting point and direction, and may differ from the physical disposition of
> > > > +the pixel units in the pixel array matrix.
> > > > +
> > > > +Each pixel matrix portion is contained in a larger rectangle, with the most
> > > > +largest being the one that describes the pixel matrix physical size. This
> > > > +defines a hierarchical positional system, where each rectangle is defined
> > > > +relatively to the largest available one among the ones exposed by the
> > > > +sub-device driver. Each selection target and the associated pixel array portion
> > > > +it represents are below presented in order from the largest to the smallest one.
> > > > +
> > > > +Pixel array physical size
> > > > +^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > +
> > > > +The image sensor chip is composed by a number of physical pixels, not all of
> > > > +them readable by the application processor. Invalid or unreadable lines might
> > > > +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors
> > > > +they might be tagged with an invalid data type (DT) so that the receiver
> > > > +automatically discard them. The size of the whole pixel matrix area is
> > > > +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner
> > > > +defined as position (0, 0). All the other selection targets are defined
> > > > +relatively to this, larger, rectangle. The rectangle returned by
> > > > +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it
> > > > +does not change at run-time and cannot be modified from userspace.
> > >
> > > As I think I've mentioned previously (not sure if it was by e-mail or on
> > > IRC), we could also decide to set V4L2_SEL_TGT_NATIVE_SIZE ==
> > > V4L2_SEL_TGT_CROP_BOUNDS by ignoring the non-readable pixels completely.
> > > What's the advantage of exposing them in the API, when the sensors
> > > doesn't provide them to the rest of the pipeline ?
> > 
> > I don't know :) I'm also  bit confused on what's the purpose of
> > NATIVE, this commit seems to suggest it was meant to replace
> > CROP_BOUNDS, but I'm not sure about that.
> > 
> > commit b518d86609cc066b626120fe6ec6fe3a4ccfcd54
> > Author: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Date:   Thu Nov 6 16:54:33 2014 -0300
> > 
> >     [media] smiapp: Support V4L2_SEL_TGT_NATIVE_SIZE
> > 
> >     Add support for selection target V4L2_SEL_TGT_NATIVE_SIZE. It is equivalent
> >     of what V4L2_SEL_TGT_CROP_BOUNDS used to be. Support for
> >     V4L2_SEL_TGT_CROP_BOUNDS is still supported by the driver as a compatibility
> >     interface.
> > 
> > Sakari, do you recall if that's was the original plan ?
> 
> That was to denote the size of the pixel array indeed. We didn't discuss
> dark or invalid pixels at the time.
> 
> So this was just there to tell that it's the pixel array you're cropping
> from.
> 
> But as long as it's API-wise compatible, I don't think anything prevents
> re-purposing this to include other areas. The documentation (AFAIR) does
> not say this has to be the same as the crop bounds rectangle.

What do you think would be best ? Should we include the non-readable
pixels in V4L2_SEL_TGT_NATIVE_SIZE, with V4L2_SEL_TGT_CROP_BOUNDS then
being strictly smaller, or drop them completely from the API, with
V4L2_SEL_TGT_CROP_BOUNDS being equal to V4L2_SEL_TGT_NATIVE_SIZE ? It
may be that we have to allow both to support existing drivers, but we
should pick one of the two options and make it mandatory for new
drivers.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/4] media: docs: Describe pixel array properties

[Laurent Pinchart]

Hi Jacopo,

On Mon, Aug 10, 2020 at 10:14:37AM +0200, Jacopo Mondi wrote:
> On Sun, Aug 09, 2020 at 08:17:57PM +0300, Laurent Pinchart wrote:
> > On Thu, Aug 06, 2020 at 11:58:31AM +0200, Hans Verkuil wrote:
> > > On 06/08/2020 11:50, Jacopo Mondi wrote:
> > > > On Thu, Aug 06, 2020 at 10:05:37AM +0200, Hans Verkuil wrote:
> > > >> On 05/08/2020 12:57, Jacopo Mondi wrote:
> > > >>> The V4L2 selection API are also used to access the pixel array
> > > >>
> > > >> are -> is
> > > >>
> > > >>> properties of an image sensor, such as the size and position of active
> > > >>> pixels and the cropped area of the pixel matrix used to produce images.
> > > >>>
> > > >>> Currently no clear definition of the different areas that compose an
> > > >>> image sensor pixel array matrix is provided in the specification, and
> > > >>> the actual meaning of each selection target when applied to an image
> > > >>> sensor was not provided.
> > > >>>
> > > >>> Provide in the sub-device documentation the definition of the pixel
> > > >>> matrix properties and the selection target associated to each of them.
> > > >>>
> > > >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > >>> ---
> > > >>>  .../userspace-api/media/v4l/dev-subdev.rst    | 81 +++++++++++++++++++
> > > >>>  1 file changed, 81 insertions(+)
> > > >>>
> > > >>> diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > >>> index 134d2fb909fa4..c47861dff9b9b 100644
> > > >>> --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > >>> +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > >>> @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.
> > > >>>  ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round
> > > >>>  the image size either up or down. :ref:`v4l2-selection-flags`
> > > >>>
> > > >>> +.. _v4l2-subdev-pixel-array-properties:
> > > >>> +
> > > >>> +Selection targets for image sensors properties
> >
> > Maybe "Selection targets for image sensors", and renaming the reference
> > to v4l2-subdev-selections-image-sensors ? This section is about how
> > selection rectangles are used for sensors, right ?
> >
> > > >>> +----------------------------------------------
> >
> > I'd move this further down, after "Types of selection targets", as that
> > section contains generic information that applies to sensors too.
> 
> Ack on both points.
> 
> > > >>> +
> > > >>> +The V4L2 selection API can be used on sub-devices that represent an image
> > > >>> +sensor to retrieve the sensor's pixel array matrix properties by using the
> > > >>> +:ref:`selection <VIDIOC_SUBDEV_G_SELECTION>` ioctls.
> > > >>> +
> > > >>> +Sub-device drivers for image sensor usually register a single source pad, but in
> > > >>> +the case they expose more, the pixel array properties can be accessed from
> > > >>> +any of them.
> >
> > Is this right ? I don't think the V4L2_SEL_TGT_CROP rectangle, for
> > instance, can be accessed from any source pad indifferently. Do we have
> > sensor drivers that create multiple source pads in a subdev today ? If
> > not I'd suggest dropping this, and adding it later if needed (when we'll
> > have a better idea of how that would work).
> 
> Yes, this was meant to cover cases which I still have not a clear idea
> about but I suspect people might have question about looking at their
> drivers. I'm totally fine adding it later when required.
> 
> > I think what should be explained here, as also mentioned by Sakari, is
> > that camera sensors can be exposed as multiple subdevs. The text below
> > is related to the pixel array, which is always the first subdev, with
> > one source pad and no sink pad. The other subdevs, modelling additional
> > processing blocks in the sensor, may use the selection API, but that's
> > out of scope for this patch.
> >
> > As we'll also need to document how other subdevs use the selection API,
> > as well as how sensors are usually handled, would it make sense to move
> > this to a separate file ? Sakari has proposed in [1] to create a new
> > Documentation/driver-api/media/camera-sensor.rst file. It would make
> > sense to centralize all sensor information there. This doesn't mean this
> > series should depend on Sakari's patch, we can handle merge conflicts
> > depending on what gets merged first.
> 
> I totally missed that one but I equally totally welcome that change. I
> would be happy to rebase this work on top of Sakari's patch which I
> will soon give a read to.
> 
> >
> > [1] https://lore.kernel.org/linux-media/20200730162040.15560-1-sakari.ailus@linux.intel.com/
> >
> > > >>> +
> > > >>> +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,
> > > >>
> > > >> V4L2_SEL_TGT_NATIVE -> V4L2_SEL_TGT_NATIVE_SIZE
> > > >>
> > > >> (same mistake is made elsewhere).
> > > >
> > > > Ah ups, I used TGT_NATIVE consistently, seems like I thought that was
> > > > the right name
> > > >
> > > >>> +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve
> > > >>> +the immutable properties of the several different areas that compose the sensor
> > > >>> +pixel array matrix. Each area describes a rectangle of logically adjacent pixel
> >
> > V4L2_TGT_CROP isn't immutable, is it ?
> >
> 
> Right, I noticed that, but didn't want to be too heavy with createing
> a special section for CROP. But you're right, it's not immutable so it
> should not be mentioned here.
> 
> > > >>> +units. The logical disposition of pixels is defined by the sensor read-out
> > > >>> +starting point and direction, and may differ from the physical disposition of
> > > >>> +the pixel units in the pixel array matrix.
> > > >>> +
> > > >>> +Each pixel matrix portion is contained in a larger rectangle, with the most
> > > >>> +largest being the one that describes the pixel matrix physical size. This
> > > >>> +defines a hierarchical positional system, where each rectangle is defined
> > > >>> +relatively to the largest available one among the ones exposed by the
> > > >>> +sub-device driver. Each selection target and the associated pixel array portion
> > > >>> +it represents are below presented in order from the largest to the smallest one.
> >
> > I find this quite confusing. As Hans suggested, I think each target
> > should define its boundaries. I'd drop this paragraph completely, as you
> > already explain below that all rectangles are defined relatively to
> > V4L2_SEL_TGT_NATIVE_SIZE.
> 
> Ack.
> 
> > > >>> +
> > > >>> +Pixel array physical size
> > > >>> +^^^^^^^^^^^^^^^^^^^^^^^^^
> > > >>> +
> > > >>> +The image sensor chip is composed by a number of physical pixels, not all of
> > > >>> +them readable by the application processor. Invalid or unreadable lines might
> > > >>> +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors
> > > >>> +they might be tagged with an invalid data type (DT) so that the receiver
> > > >>> +automatically discard them.
> >
> > "might" is a bit weak for unreadable lines, there's no way they can be
> > transmitted if they can't be read :-)
> 
> This paragraph reflects my confusion on the subject. My interpretation
> is that for CSI-2 sensors, you cannot crop a black pixels area and
> capture just them. At the contrary, the black pixels are sent on the
> bus (maybe that can be optionally enabled/disabled) tagged with a
> special DT and interleaved with image data and you have to instruct
> your receiver to discard or accept that DT, and have black pixels
> captured to a separate memory area, or at buffer end (that depends on
> the receiver's architecture I guess).

This is all sensor specific I'm afraid :-( It's entirely conceivable
that a sensor would implement a single crop rectangle that can span the
black pixels, while another sensor could restrict cropping to the
effective area, and enable/disable the black pixels output through a
separate configuration. The black pixels lines will however very likely
be cropped horizontally the same way as the visible pixels, as
variable-length lines isn't widely supported on the receiver side, but
some sensors could implement more options.

> At the same time, I won't be
> surprised if some sensor's allow you to explicitly crop on black
> pixels areas and only put them on the bus. Knowing how the SMIA++
> standard handles that part might help establishing an expected
> behaviour (I really, really, wish we had as a community any leverage
> to influence sensor manufacturer towards a standardized behaviour, it
> would be time for the industry to do so. </wishful thinking>).

MIPI CCS ? Still, not all vendors will implement that, sometimes for
good reasons, mostly probably just "because".

> If that's correct, I wonder how would that possibly work with parallel
> sensors, where you cannot tag data on the bus. I there assume you have
> to explicitly select the black region to capture.

From a sensor point of view it makes no difference, it's only from a
receiver point of view that the situation may get more complex as you
can't filter on the data type.

> I there tried to, confusingly, express that different behaviour and
> stay as much as possible generic not to rule out any existing case.
> 
> > One way to generalize this a bit would be to explain, after the first
> > sentence, that not all pixels may be read by the sensor, that among the
> > pixels that are read invalid ones may not be transmitted on the bus, and
> > that among transmitted pixels not all of them may be possible to capture
> > on the receiver side. For instance, invalid lines may be transmitted as
> > part of the vertical blanking on parallel buses, or tagged as blanking
> > data or null data on CSI-2 buses. Most receivers are not able to capture
> > either.
> >
> > (On a side note, strictly speaking, a CSI-2 receiver that would be able
> > to capture null or blanking packets wouldn't be compliant with the CSI-2
> > spec.)
> >
> > > >>> The size of the whole pixel matrix area is
> > > >>> +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner
> > > >>> +defined as position (0, 0). All the other selection targets are defined
> > > >>> +relatively to this, larger, rectangle. The rectangle returned by
> >
> > s/, larger,/
> >
> > > >>> +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it
> > > >>> +does not change at run-time and cannot be modified from userspace.
> > > >>
> > > >> It is a good idea to mention that if there are no invalid or unreadable pixels/lines,
> > > >> then V4L2_SEL_TGT_NATIVE_SIZE == V4L2_SEL_TGT_CROP_BOUNDS.
> > > >
> > > > Yes it is! I'll add it here
> >
> > Should it be added below instead, where you define
> > V4L2_SEL_TGT_CROP_BOUNDS ? It's best to avoid mentioning something that
> > isn't defined yet when possible.
> 
> I have a question for Sakari on this target, but I'll deflect it to
> the reply to your comment  on patch 1/4.
> 
> > > >>> +
> > > >>> +Pixel array readable area
> > > >>> +^^^^^^^^^^^^^^^^^^^^^^^^^
> > > >>> +
> > > >>> +The V4L2_SEL_TGT_CROP_BOUNDS targets returns size and position of the readable
> > > >>> +area of the pixel array matrix, including pixels with valid image data and pixel
> > > >>> +used for calibration purposes, such as optical black pixels. It is not unlikely
> >
> > s/not unlikely/likely ? Or just "common".
> >
> > > >>> +that valid pixels and optical black pixels are surrounded by non-readable rows
> > > >>> +and columns of pixels. Those does not concur in the definition of the
> >
> > s/does/do/
> >
> > I'm not sure "concur" is the right word. Did you mean "those are not
> > part of the V4L2_SEL_TGT_CROP_BOUNDS rectangle" ?
> 
> Yes, I meant they should not be counted in the definition of the BOUND
> rectangle sizes.
> 
> > > >>> +V4L2_SEL_TGT_CROP_BOUNDS rectangle. The rectangle returned by
> > > >>> +V4L2_SEL_TGT_CROP_BOUNDS describes an immutable property of the image sensor, it
> > > >>> +does not change at run-time and cannot be modified from userspace.
> > > >>
> > > >> Mention that BOUNDS is enclosed by NATIVE_SIZE.
> > > >
> > > > I tried to express that in the intro section with
> > > >
> > > > "Each pixel matrix portion is contained in a larger rectangle, with the most
> > > > largest being the one that describes the pixel matrix physical size."
> > > >
> > > > But I guess it's worth to express that for each target!
> > > >
> > > >>> +
> > > >>> +Pixel array active area
> > > >>> +^^^^^^^^^^^^^^^^^^^^^^^
> > > >>> +
> > > >>> +The portion of the pixel array which contains valid image data is defined as the
> > > >>> +active area of the pixel matrix. The active pixel array is is accessed by mean
> > > >>
> >
> > s/is is/is/
> >
> > > >> mean -> means
> > > >>
> > > >>> +of the V4L2_SEL_TGT_CROP_DEFAULT target, and is contained in the larger
> > > >>> +V4L2_SEL_TGT_CROP_BOUNDS rectangle. It represents the largest possible frame
> > > >>> +resolution the sensor can produce and defines the dimension of the full
> > > >>> +field-of-view. The rectangle returned by V4L2_SEL_TGT_CROP_BOUNDS describes an
> > > >>
> > > >> BOUNDS -> DEFAULT
> > > >
> > > > ups
> > > >
> > > >>> +immutable property of the image sensor, it does not change at run-time and
> > > >>> +cannot be modified from userspace.
> > > >>
> > > >> Mention that CROP_DEFAULT is enclosed by CROP_BOUNDS
> > > >>
> > > >>> +
> > > >>> +Analog crop rectangle
> > > >>
> > > >> Why analog? It's just the crop rectangle, nothing analog about it.
> > > >
> > > > We used the 'analogCrop' term in libcamera to differentiate the
> > > > cropping which happens on the sensor pixel array matrix to select the
> > > > region to process and produce image from. Sensor with an on-board
> > > > scaler can perform other cropping steps to implement, in example digital
> > > > zoom, so we expect to have a 'digital crop' phase as well. RAW
> > > > sensors, in example, will only have an analogCrop rectangle.
> > > >
> > > > Quoting the libcamera definition of analog crop:
> > > >
> > > >  * horizontal and vertical sizes define the portion of the pixel array which
> > > >  * is read-out and provided to the sensor's internal processing pipeline, before
> > > >  * any pixel sub-sampling method, such as pixel binning, skipping and averaging
> > > >  * take place.
> > > >
> > > > should I keep it or remove it ?
> > >
> > > It's a very confusing term. Especially since this API can also be used with analog
> > > video capture devices (Composite/S-Video) where the video signal actually is analog.
> > >
> > > In the V4L2 API there is no such thing as 'analog crop', so please remove it.
> >
> > Jacopo is right, sensors usually perform cropping in the analog domain
> > (by not reading out all pixels from the pixel array), and also support
> > cropping in later stages, after binning/skipping, and after further
> > scaling. Note that all of these crop operations are optional. Although
> > not common, it's also not unconceivable that a sensor wouldn't support
> > cropping at all.
> >
> > This being said, it only makes sense to talk about analog crop when
> > multiple crop operations are performed, and thus in the context of the
> > whole sensor, with multiple subdevs. If we explain this, as proposed
> > above, and make it clear that the usage of the selection rectangles
> > defined here applies to the pixel array only, we can drop the "analog"
> > term, and just talk about cropping in the pixel array.
> 
> It's fine as long as it removes any ambiguity.
> 
> > > >>> +^^^^^^^^^^^^^^^^^^^^^
> > > >>> +
> > > >>> +The sensor driver might decide, in order to adjust the image resolution to best
> > > >>> +match the one requested by applications, to only process a part of the active
> > > >>> +pixel array matrix.
> >
> > I don't think that's the right approach. With MC-based devices, the
> > philosophy is to expose all configuration parameters to userspace. It's
> > not about sensor drivers making decisions, but about userspace deciding
> > to crop at the pixel array level.
> >
> > This being said, I'm aware the decision is made by drivers when they're
> > mode-based. Please see below for that.
> 
> Correct, and I should now be used enough to the 'userspace drives'
> approach to remember about documenting it :)
> 
> > > >>> The selected area is read-out and processed by the image
> > > >>> +sensor on-board ISP in order to produce images of the desired size and
> > > >>> +resolution while possible maintaing the largest possible field-of-view. The
> > > >>
> > > >> maintaing -> maintaining
> > > >>
> > > >> Actually, I'd drop 'while possible maintaing the largest possible field-of-view'
> > > >> entirely. It doesn't make much sense.
> > > >
> > > > Ack
> >
> > In general, in this section, as we're documenting the pixel array, let's
> > not talk about the ISP.
> >
> > > >>> +cropped portion of the pixel array which is used to produce images is returned
> > > >>> +by the V4L2_SEL_TGT_CROP target and represent the only information that can
> > > >>
> > > >> represent -> represents
> > > >>
> > > >>> +change at runtime as it depends on the currently configured sensor mode and
> > > >>> +desired image resolution. If the sub-device driver supports that, userspace
> > > >>> +can set the analog crop rectangle to select which portion of the pixel array
> > > >>
> > > >> s/analog//
> > > >>
> > > >>> +to read out.
> >
> > I think it's better to focus on the best case, and document usage of
> > crop rectangles in general first, for drivers that expose full
> > configurability of the sensor. A separate section should then then make
> > a note of how mode-based drivers differ, which is mostly in the
> > V4L2_SEL_TGT_CROP target being read-only, and on the single subdev
> > hiding all the processing steps, with the crop target thus being the
> > result of all cropping operations, analog and digital.
> >
> > Sakari's patch has a bit of information about this, it may be useful to
> > reuse it or integrate with it somehow.
> 
> I'll try to see how the two parts can be piled one on top of the
> other.
> 
> > > >> Mention that CROP is enclosed by CROP_BOUNDS and defaults to CROP_DEFAULT.
> > > >>
> > > >> Make a note that CROP can also be used to obtain optical black pixels.
> > > >
> > > > What about:
> > > >
> > > > +desired image resolution. If the sub-device driver supports that, userspace
> > > > +can set the analog crop rectangle to select which portion of the pixel array
> > > > +to read out including, if supported, optical black pixels.
> > >
> > > Hmm, that's a bit awkward. How about:
> > >
> > > +desired image resolution. If supported by the sub-device driver, userspace
> > > +can set the crop rectangle to select which portion of the pixel array
> > > +to read out. This may include optical black pixels if those are part of
> > > +V4L2_SEL_TGT_CROP_BOUNDS.
> > >
> > > >>> +
> > > >>>
> > > >>>  Types of selection targets
> > > >>>  --------------------------

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/4] media: docs: Describe pixel array properties

[Sakari Ailus]

Hi Laurent,

On Wed, Aug 19, 2020 at 04:06:23AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Tue, Aug 18, 2020 at 11:17:43AM +0300, Sakari Ailus wrote:
> > On Mon, Aug 10, 2020 at 10:17:57AM +0200, Jacopo Mondi wrote:
> > > On Sun, Aug 09, 2020 at 08:58:21PM +0300, Laurent Pinchart wrote:
> > > > On Wed, Aug 05, 2020 at 12:57:18PM +0200, Jacopo Mondi wrote:
> > > > > The V4L2 selection API are also used to access the pixel array
> > > > > properties of an image sensor, such as the size and position of active
> > > > > pixels and the cropped area of the pixel matrix used to produce images.
> > > > >
> > > > > Currently no clear definition of the different areas that compose an
> > > > > image sensor pixel array matrix is provided in the specification, and
> > > > > the actual meaning of each selection target when applied to an image
> > > > > sensor was not provided.
> > > > >
> > > > > Provide in the sub-device documentation the definition of the pixel
> > > > > matrix properties and the selection target associated to each of them.
> > > > >
> > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > > ---
> > > > >  .../userspace-api/media/v4l/dev-subdev.rst    | 81 +++++++++++++++++++
> > > > >  1 file changed, 81 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > > > index 134d2fb909fa4..c47861dff9b9b 100644
> > > > > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > > > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > > > @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.
> > > > >  ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round
> > > > >  the image size either up or down. :ref:`v4l2-selection-flags`
> > > > >
> > > > > +.. _v4l2-subdev-pixel-array-properties:
> > > > > +
> > > > > +Selection targets for image sensors properties
> > > > > +----------------------------------------------
> > > > > +
> > > > > +The V4L2 selection API can be used on sub-devices that represent an image
> > > > > +sensor to retrieve the sensor's pixel array matrix properties by using the
> > > > > +:ref:`selection <VIDIOC_SUBDEV_G_SELECTION>` ioctls.
> > > > > +
> > > > > +Sub-device drivers for image sensor usually register a single source pad, but in
> > > > > +the case they expose more, the pixel array properties can be accessed from
> > > > > +any of them.
> > > > > +
> > > > > +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,
> > > > > +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve
> > > > > +the immutable properties of the several different areas that compose the sensor
> > > > > +pixel array matrix. Each area describes a rectangle of logically adjacent pixel
> > > > > +units. The logical disposition of pixels is defined by the sensor read-out
> > > > > +starting point and direction, and may differ from the physical disposition of
> > > > > +the pixel units in the pixel array matrix.
> > > > > +
> > > > > +Each pixel matrix portion is contained in a larger rectangle, with the most
> > > > > +largest being the one that describes the pixel matrix physical size. This
> > > > > +defines a hierarchical positional system, where each rectangle is defined
> > > > > +relatively to the largest available one among the ones exposed by the
> > > > > +sub-device driver. Each selection target and the associated pixel array portion
> > > > > +it represents are below presented in order from the largest to the smallest one.
> > > > > +
> > > > > +Pixel array physical size
> > > > > +^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > > +
> > > > > +The image sensor chip is composed by a number of physical pixels, not all of
> > > > > +them readable by the application processor. Invalid or unreadable lines might
> > > > > +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors
> > > > > +they might be tagged with an invalid data type (DT) so that the receiver
> > > > > +automatically discard them. The size of the whole pixel matrix area is
> > > > > +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner
> > > > > +defined as position (0, 0). All the other selection targets are defined
> > > > > +relatively to this, larger, rectangle. The rectangle returned by
> > > > > +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it
> > > > > +does not change at run-time and cannot be modified from userspace.
> > > >
> > > > As I think I've mentioned previously (not sure if it was by e-mail or on
> > > > IRC), we could also decide to set V4L2_SEL_TGT_NATIVE_SIZE ==
> > > > V4L2_SEL_TGT_CROP_BOUNDS by ignoring the non-readable pixels completely.
> > > > What's the advantage of exposing them in the API, when the sensors
> > > > doesn't provide them to the rest of the pipeline ?
> > > 
> > > I don't know :) I'm also  bit confused on what's the purpose of
> > > NATIVE, this commit seems to suggest it was meant to replace
> > > CROP_BOUNDS, but I'm not sure about that.
> > > 
> > > commit b518d86609cc066b626120fe6ec6fe3a4ccfcd54
> > > Author: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > Date:   Thu Nov 6 16:54:33 2014 -0300
> > > 
> > >     [media] smiapp: Support V4L2_SEL_TGT_NATIVE_SIZE
> > > 
> > >     Add support for selection target V4L2_SEL_TGT_NATIVE_SIZE. It is equivalent
> > >     of what V4L2_SEL_TGT_CROP_BOUNDS used to be. Support for
> > >     V4L2_SEL_TGT_CROP_BOUNDS is still supported by the driver as a compatibility
> > >     interface.
> > > 
> > > Sakari, do you recall if that's was the original plan ?
> > 
> > That was to denote the size of the pixel array indeed. We didn't discuss
> > dark or invalid pixels at the time.
> > 
> > So this was just there to tell that it's the pixel array you're cropping
> > from.
> > 
> > But as long as it's API-wise compatible, I don't think anything prevents
> > re-purposing this to include other areas. The documentation (AFAIR) does
> > not say this has to be the same as the crop bounds rectangle.
> 
> What do you think would be best ? Should we include the non-readable
> pixels in V4L2_SEL_TGT_NATIVE_SIZE, with V4L2_SEL_TGT_CROP_BOUNDS then
> being strictly smaller, or drop them completely from the API, with
> V4L2_SEL_TGT_CROP_BOUNDS being equal to V4L2_SEL_TGT_NATIVE_SIZE ? It
> may be that we have to allow both to support existing drivers, but we
> should pick one of the two options and make it mandatory for new
> drivers.

That's a very good question.

What would be the purpose of adding pixels that cannot be read? I assume
they would not affect sensor timing either in that case, so there would be
no difference whether they are there or not. The crop bounds should contain
everything whereas for the default crop should reflect the area of the
visible pixels.

I guess in theory the visible pixels could not be cropped by the sensor in
analogue cropping step, so it might be worth having a separate rectangle
for those, too.

There could also be sensors that read the dark pixels internally and use
them somehow without sending their data out. don't think we should try to
model that though as it's likely entirely internal to the sensor in that
case.

-- 
Sakari Ailus

Re: [PATCH 1/4] media: docs: Describe pixel array properties

[Laurent Pinchart]

Hi Sakari,

On Wed, Aug 19, 2020 at 01:20:00PM +0300, Sakari Ailus wrote:
> On Wed, Aug 19, 2020 at 04:06:23AM +0300, Laurent Pinchart wrote:
> > On Tue, Aug 18, 2020 at 11:17:43AM +0300, Sakari Ailus wrote:
> > > On Mon, Aug 10, 2020 at 10:17:57AM +0200, Jacopo Mondi wrote:
> > > > On Sun, Aug 09, 2020 at 08:58:21PM +0300, Laurent Pinchart wrote:
> > > > > On Wed, Aug 05, 2020 at 12:57:18PM +0200, Jacopo Mondi wrote:
> > > > > > The V4L2 selection API are also used to access the pixel array
> > > > > > properties of an image sensor, such as the size and position of active
> > > > > > pixels and the cropped area of the pixel matrix used to produce images.
> > > > > >
> > > > > > Currently no clear definition of the different areas that compose an
> > > > > > image sensor pixel array matrix is provided in the specification, and
> > > > > > the actual meaning of each selection target when applied to an image
> > > > > > sensor was not provided.
> > > > > >
> > > > > > Provide in the sub-device documentation the definition of the pixel
> > > > > > matrix properties and the selection target associated to each of them.
> > > > > >
> > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > > > ---
> > > > > >  .../userspace-api/media/v4l/dev-subdev.rst    | 81 +++++++++++++++++++
> > > > > >  1 file changed, 81 insertions(+)
> > > > > >
> > > > > > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > > > > index 134d2fb909fa4..c47861dff9b9b 100644
> > > > > > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > > > > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > > > > @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.
> > > > > >  ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round
> > > > > >  the image size either up or down. :ref:`v4l2-selection-flags`
> > > > > >
> > > > > > +.. _v4l2-subdev-pixel-array-properties:
> > > > > > +
> > > > > > +Selection targets for image sensors properties
> > > > > > +----------------------------------------------
> > > > > > +
> > > > > > +The V4L2 selection API can be used on sub-devices that represent an image
> > > > > > +sensor to retrieve the sensor's pixel array matrix properties by using the
> > > > > > +:ref:`selection <VIDIOC_SUBDEV_G_SELECTION>` ioctls.
> > > > > > +
> > > > > > +Sub-device drivers for image sensor usually register a single source pad, but in
> > > > > > +the case they expose more, the pixel array properties can be accessed from
> > > > > > +any of them.
> > > > > > +
> > > > > > +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,
> > > > > > +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve
> > > > > > +the immutable properties of the several different areas that compose the sensor
> > > > > > +pixel array matrix. Each area describes a rectangle of logically adjacent pixel
> > > > > > +units. The logical disposition of pixels is defined by the sensor read-out
> > > > > > +starting point and direction, and may differ from the physical disposition of
> > > > > > +the pixel units in the pixel array matrix.
> > > > > > +
> > > > > > +Each pixel matrix portion is contained in a larger rectangle, with the most
> > > > > > +largest being the one that describes the pixel matrix physical size. This
> > > > > > +defines a hierarchical positional system, where each rectangle is defined
> > > > > > +relatively to the largest available one among the ones exposed by the
> > > > > > +sub-device driver. Each selection target and the associated pixel array portion
> > > > > > +it represents are below presented in order from the largest to the smallest one.
> > > > > > +
> > > > > > +Pixel array physical size
> > > > > > +^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > > > +
> > > > > > +The image sensor chip is composed by a number of physical pixels, not all of
> > > > > > +them readable by the application processor. Invalid or unreadable lines might
> > > > > > +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors
> > > > > > +they might be tagged with an invalid data type (DT) so that the receiver
> > > > > > +automatically discard them. The size of the whole pixel matrix area is
> > > > > > +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner
> > > > > > +defined as position (0, 0). All the other selection targets are defined
> > > > > > +relatively to this, larger, rectangle. The rectangle returned by
> > > > > > +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it
> > > > > > +does not change at run-time and cannot be modified from userspace.
> > > > >
> > > > > As I think I've mentioned previously (not sure if it was by e-mail or on
> > > > > IRC), we could also decide to set V4L2_SEL_TGT_NATIVE_SIZE ==
> > > > > V4L2_SEL_TGT_CROP_BOUNDS by ignoring the non-readable pixels completely.
> > > > > What's the advantage of exposing them in the API, when the sensors
> > > > > doesn't provide them to the rest of the pipeline ?
> > > > 
> > > > I don't know :) I'm also  bit confused on what's the purpose of
> > > > NATIVE, this commit seems to suggest it was meant to replace
> > > > CROP_BOUNDS, but I'm not sure about that.
> > > > 
> > > > commit b518d86609cc066b626120fe6ec6fe3a4ccfcd54
> > > > Author: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > Date:   Thu Nov 6 16:54:33 2014 -0300
> > > > 
> > > >     [media] smiapp: Support V4L2_SEL_TGT_NATIVE_SIZE
> > > > 
> > > >     Add support for selection target V4L2_SEL_TGT_NATIVE_SIZE. It is equivalent
> > > >     of what V4L2_SEL_TGT_CROP_BOUNDS used to be. Support for
> > > >     V4L2_SEL_TGT_CROP_BOUNDS is still supported by the driver as a compatibility
> > > >     interface.
> > > > 
> > > > Sakari, do you recall if that's was the original plan ?
> > > 
> > > That was to denote the size of the pixel array indeed. We didn't discuss
> > > dark or invalid pixels at the time.
> > > 
> > > So this was just there to tell that it's the pixel array you're cropping
> > > from.
> > > 
> > > But as long as it's API-wise compatible, I don't think anything prevents
> > > re-purposing this to include other areas. The documentation (AFAIR) does
> > > not say this has to be the same as the crop bounds rectangle.
> > 
> > What do you think would be best ? Should we include the non-readable
> > pixels in V4L2_SEL_TGT_NATIVE_SIZE, with V4L2_SEL_TGT_CROP_BOUNDS then
> > being strictly smaller, or drop them completely from the API, with
> > V4L2_SEL_TGT_CROP_BOUNDS being equal to V4L2_SEL_TGT_NATIVE_SIZE ? It
> > may be that we have to allow both to support existing drivers, but we
> > should pick one of the two options and make it mandatory for new
> > drivers.
> 
> That's a very good question.
> 
> What would be the purpose of adding pixels that cannot be read? I assume
> they would not affect sensor timing either in that case, so there would be
> no difference whether they are there or not.

Timings is a good point, could there be sensors that read those pixels
but don't send them out ? Maybe to avoid edge effects ? That would be
accounted for in the H/V blank though, wouldn't it ?

> The crop bounds should contain
> everything whereas for the default crop should reflect the area of the
> visible pixels.

I believe there are sensors that have all pixels visible, but recommend
not using edge pixels as they are affected by edge effects, even if
those pixels can be read out and transferred. In that case
V4L2_SEL_TGT_CROP_BOUNDS should include the edge pixels, but maybe
V4L2_SEL_TGT_CROP_DEFAULT shouldn't ?

> I guess in theory the visible pixels could not be cropped by the sensor in
> analogue cropping step, so it might be worth having a separate rectangle
> for those, too.

I'm not sure to follow you here.

> There could also be sensors that read the dark pixels internally and use
> them somehow without sending their data out. don't think we should try to
> model that though as it's likely entirely internal to the sensor in that
> case.

Agreed.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/4] media: docs: Describe pixel array properties

[Sakari Ailus]

Hi Laurent,

On Wed, Aug 19, 2020 at 03:38:40PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Wed, Aug 19, 2020 at 01:20:00PM +0300, Sakari Ailus wrote:
> > On Wed, Aug 19, 2020 at 04:06:23AM +0300, Laurent Pinchart wrote:
> > > On Tue, Aug 18, 2020 at 11:17:43AM +0300, Sakari Ailus wrote:
> > > > On Mon, Aug 10, 2020 at 10:17:57AM +0200, Jacopo Mondi wrote:
> > > > > On Sun, Aug 09, 2020 at 08:58:21PM +0300, Laurent Pinchart wrote:
> > > > > > On Wed, Aug 05, 2020 at 12:57:18PM +0200, Jacopo Mondi wrote:
> > > > > > > The V4L2 selection API are also used to access the pixel array
> > > > > > > properties of an image sensor, such as the size and position of active
> > > > > > > pixels and the cropped area of the pixel matrix used to produce images.
> > > > > > >
> > > > > > > Currently no clear definition of the different areas that compose an
> > > > > > > image sensor pixel array matrix is provided in the specification, and
> > > > > > > the actual meaning of each selection target when applied to an image
> > > > > > > sensor was not provided.
> > > > > > >
> > > > > > > Provide in the sub-device documentation the definition of the pixel
> > > > > > > matrix properties and the selection target associated to each of them.
> > > > > > >
> > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > > > > ---
> > > > > > >  .../userspace-api/media/v4l/dev-subdev.rst    | 81 +++++++++++++++++++
> > > > > > >  1 file changed, 81 insertions(+)
> > > > > > >
> > > > > > > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > > > > > index 134d2fb909fa4..c47861dff9b9b 100644
> > > > > > > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > > > > > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > > > > > @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.
> > > > > > >  ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round
> > > > > > >  the image size either up or down. :ref:`v4l2-selection-flags`
> > > > > > >
> > > > > > > +.. _v4l2-subdev-pixel-array-properties:
> > > > > > > +
> > > > > > > +Selection targets for image sensors properties
> > > > > > > +----------------------------------------------
> > > > > > > +
> > > > > > > +The V4L2 selection API can be used on sub-devices that represent an image
> > > > > > > +sensor to retrieve the sensor's pixel array matrix properties by using the
> > > > > > > +:ref:`selection <VIDIOC_SUBDEV_G_SELECTION>` ioctls.
> > > > > > > +
> > > > > > > +Sub-device drivers for image sensor usually register a single source pad, but in
> > > > > > > +the case they expose more, the pixel array properties can be accessed from
> > > > > > > +any of them.
> > > > > > > +
> > > > > > > +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,
> > > > > > > +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve
> > > > > > > +the immutable properties of the several different areas that compose the sensor
> > > > > > > +pixel array matrix. Each area describes a rectangle of logically adjacent pixel
> > > > > > > +units. The logical disposition of pixels is defined by the sensor read-out
> > > > > > > +starting point and direction, and may differ from the physical disposition of
> > > > > > > +the pixel units in the pixel array matrix.
> > > > > > > +
> > > > > > > +Each pixel matrix portion is contained in a larger rectangle, with the most
> > > > > > > +largest being the one that describes the pixel matrix physical size. This
> > > > > > > +defines a hierarchical positional system, where each rectangle is defined
> > > > > > > +relatively to the largest available one among the ones exposed by the
> > > > > > > +sub-device driver. Each selection target and the associated pixel array portion
> > > > > > > +it represents are below presented in order from the largest to the smallest one.
> > > > > > > +
> > > > > > > +Pixel array physical size
> > > > > > > +^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > > > > +
> > > > > > > +The image sensor chip is composed by a number of physical pixels, not all of
> > > > > > > +them readable by the application processor. Invalid or unreadable lines might
> > > > > > > +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors
> > > > > > > +they might be tagged with an invalid data type (DT) so that the receiver
> > > > > > > +automatically discard them. The size of the whole pixel matrix area is
> > > > > > > +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner
> > > > > > > +defined as position (0, 0). All the other selection targets are defined
> > > > > > > +relatively to this, larger, rectangle. The rectangle returned by
> > > > > > > +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it
> > > > > > > +does not change at run-time and cannot be modified from userspace.
> > > > > >
> > > > > > As I think I've mentioned previously (not sure if it was by e-mail or on
> > > > > > IRC), we could also decide to set V4L2_SEL_TGT_NATIVE_SIZE ==
> > > > > > V4L2_SEL_TGT_CROP_BOUNDS by ignoring the non-readable pixels completely.
> > > > > > What's the advantage of exposing them in the API, when the sensors
> > > > > > doesn't provide them to the rest of the pipeline ?
> > > > > 
> > > > > I don't know :) I'm also  bit confused on what's the purpose of
> > > > > NATIVE, this commit seems to suggest it was meant to replace
> > > > > CROP_BOUNDS, but I'm not sure about that.
> > > > > 
> > > > > commit b518d86609cc066b626120fe6ec6fe3a4ccfcd54
> > > > > Author: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > Date:   Thu Nov 6 16:54:33 2014 -0300
> > > > > 
> > > > >     [media] smiapp: Support V4L2_SEL_TGT_NATIVE_SIZE
> > > > > 
> > > > >     Add support for selection target V4L2_SEL_TGT_NATIVE_SIZE. It is equivalent
> > > > >     of what V4L2_SEL_TGT_CROP_BOUNDS used to be. Support for
> > > > >     V4L2_SEL_TGT_CROP_BOUNDS is still supported by the driver as a compatibility
> > > > >     interface.
> > > > > 
> > > > > Sakari, do you recall if that's was the original plan ?
> > > > 
> > > > That was to denote the size of the pixel array indeed. We didn't discuss
> > > > dark or invalid pixels at the time.
> > > > 
> > > > So this was just there to tell that it's the pixel array you're cropping
> > > > from.
> > > > 
> > > > But as long as it's API-wise compatible, I don't think anything prevents
> > > > re-purposing this to include other areas. The documentation (AFAIR) does
> > > > not say this has to be the same as the crop bounds rectangle.
> > > 
> > > What do you think would be best ? Should we include the non-readable
> > > pixels in V4L2_SEL_TGT_NATIVE_SIZE, with V4L2_SEL_TGT_CROP_BOUNDS then
> > > being strictly smaller, or drop them completely from the API, with
> > > V4L2_SEL_TGT_CROP_BOUNDS being equal to V4L2_SEL_TGT_NATIVE_SIZE ? It
> > > may be that we have to allow both to support existing drivers, but we
> > > should pick one of the two options and make it mandatory for new
> > > drivers.
> > 
> > That's a very good question.
> > 
> > What would be the purpose of adding pixels that cannot be read? I assume
> > they would not affect sensor timing either in that case, so there would be
> > no difference whether they are there or not.
> 
> Timings is a good point, could there be sensors that read those pixels
> but don't send them out ? Maybe to avoid edge effects ? That would be
> accounted for in the H/V blank though, wouldn't it ?

I guess we could ignore it, as it takes place during what is indeed
considered as blanking.

> 
> > The crop bounds should contain
> > everything whereas for the default crop should reflect the area of the
> > visible pixels.
> 
> I believe there are sensors that have all pixels visible, but recommend
> not using edge pixels as they are affected by edge effects, even if
> those pixels can be read out and transferred. In that case
> V4L2_SEL_TGT_CROP_BOUNDS should include the edge pixels, but maybe
> V4L2_SEL_TGT_CROP_DEFAULT shouldn't ?

I guess so. But in practice I wonder if there are such implementations.

> 
> > I guess in theory the visible pixels could not be cropped by the sensor in
> > analogue cropping step, so it might be worth having a separate rectangle
> > for those, too.
> 
> I'm not sure to follow you here.

I'm saying the sensor hardware could in theory be unable to read only the
visible pixels.

-- 
Regards,

Sakari Ailus

Re: [PATCH 4/4] media: i2c: imx219: Selection compliance fixes

[Jacopo Mondi]

Hi Hans,

On Wed, Aug 05, 2020 at 12:57:21PM +0200, Jacopo Mondi wrote:
> From: Hans Verkuil <hverkuil@xs4all.nl>
>
> To comply with the intended usage of the V4L2 selection target when
> used to retrieve a sensor image properties, adjust the rectangles
> returned by the imx219 driver.
>
> The top/left crop coordinates of the TGT_CROP rectangle were set to
> (0, 0) instead of (8, 8) which is the offset from the larger physical
> pixel array rectangle. This was also a mismatch with the default values
> crop rectangle value, so this is corrected. Found with v4l2-compliance.
>
> While at it, add V4L2_SEL_TGT_CROP_BOUNDS support: CROP_DEFAULT and
> CROP_BOUNDS have the same size as the non-active pixels are not readable
> using the selection API. Found with v4l2-compliance.
>
> Fixes: e6d4ef7d58aa7 ("media: i2c: imx219: Implement get_selection")
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> [reword commit message, use macros for pixel offsets]
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Has this fallen to the cracks ? It was part of a larger series
(abandoned for now) but I think this patch has merits and can be broke
out.

If that's the case I'll fix ov5647 which I have in the pipe which
suffer from the same issue as this one and update the libcamera side
that uses this information.

Thanks
  j

> ---
>  drivers/media/i2c/imx219.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 60b23d113fc56..ff48a95b448b1 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -473,8 +473,8 @@ static const struct imx219_mode supported_modes[] = {
>  		.width = 3280,
>  		.height = 2464,
>  		.crop = {
> -			.left = 0,
> -			.top = 0,
> +			.left = IMX219_PIXEL_ARRAY_LEFT,
> +			.top = IMX219_PIXEL_ARRAY_TOP,
>  			.width = 3280,
>  			.height = 2464
>  		},
> @@ -489,8 +489,8 @@ static const struct imx219_mode supported_modes[] = {
>  		.width = 1920,
>  		.height = 1080,
>  		.crop = {
> -			.left = 680,
> -			.top = 692,
> +			.left = 688,
> +			.top = 700,
>  			.width = 1920,
>  			.height = 1080
>  		},
> @@ -505,8 +505,8 @@ static const struct imx219_mode supported_modes[] = {
>  		.width = 1640,
>  		.height = 1232,
>  		.crop = {
> -			.left = 0,
> -			.top = 0,
> +			.left = IMX219_PIXEL_ARRAY_LEFT,
> +			.top = IMX219_PIXEL_ARRAY_TOP,
>  			.width = 3280,
>  			.height = 2464
>  		},
> @@ -521,8 +521,8 @@ static const struct imx219_mode supported_modes[] = {
>  		.width = 640,
>  		.height = 480,
>  		.crop = {
> -			.left = 1000,
> -			.top = 752,
> +			.left = 1008,
> +			.top = 760,
>  			.width = 1280,
>  			.height = 960
>  		},
> @@ -1045,6 +1045,7 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
>  		return 0;
>
>  	case V4L2_SEL_TGT_CROP_DEFAULT:
> +	case V4L2_SEL_TGT_CROP_BOUNDS:
>  		sel->r.top = IMX219_PIXEL_ARRAY_TOP;
>  		sel->r.left = IMX219_PIXEL_ARRAY_LEFT;
>  		sel->r.width = IMX219_PIXEL_ARRAY_WIDTH;
> --
> 2.27.0
>

Re: [libcamera-devel] [PATCH 4/4] media: i2c: imx219: Selection compliance fixes

[Jacopo Mondi]

Hi Hans,

On Fri, Nov 06, 2020 at 01:33:06PM +0100, Jacopo Mondi wrote:
> Hi Hans,
>
> On Wed, Aug 05, 2020 at 12:57:21PM +0200, Jacopo Mondi wrote:
> > From: Hans Verkuil <hverkuil@xs4all.nl>
> >
> > To comply with the intended usage of the V4L2 selection target when
> > used to retrieve a sensor image properties, adjust the rectangles
> > returned by the imx219 driver.
> >
> > The top/left crop coordinates of the TGT_CROP rectangle were set to
> > (0, 0) instead of (8, 8) which is the offset from the larger physical
> > pixel array rectangle. This was also a mismatch with the default values
> > crop rectangle value, so this is corrected. Found with v4l2-compliance.
> >
> > While at it, add V4L2_SEL_TGT_CROP_BOUNDS support: CROP_DEFAULT and
> > CROP_BOUNDS have the same size as the non-active pixels are not readable
> > using the selection API. Found with v4l2-compliance.
> >
> > Fixes: e6d4ef7d58aa7 ("media: i2c: imx219: Implement get_selection")
> > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > [reword commit message, use macros for pixel offsets]
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Has this fallen to the cracks ? It was part of a larger series
> (abandoned for now) but I think this patch has merits and can be broke
> out.
>
> If that's the case I'll fix ov5647 which I have in the pipe which
> suffer from the same issue as this one and update the libcamera side
> that uses this information.

Gentle ping

>
> Thanks
>   j
>
> > ---
> >  drivers/media/i2c/imx219.c | 17 +++++++++--------
> >  1 file changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index 60b23d113fc56..ff48a95b448b1 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -473,8 +473,8 @@ static const struct imx219_mode supported_modes[] = {
> >  		.width = 3280,
> >  		.height = 2464,
> >  		.crop = {
> > -			.left = 0,
> > -			.top = 0,
> > +			.left = IMX219_PIXEL_ARRAY_LEFT,
> > +			.top = IMX219_PIXEL_ARRAY_TOP,
> >  			.width = 3280,
> >  			.height = 2464
> >  		},
> > @@ -489,8 +489,8 @@ static const struct imx219_mode supported_modes[] = {
> >  		.width = 1920,
> >  		.height = 1080,
> >  		.crop = {
> > -			.left = 680,
> > -			.top = 692,
> > +			.left = 688,
> > +			.top = 700,
> >  			.width = 1920,
> >  			.height = 1080
> >  		},
> > @@ -505,8 +505,8 @@ static const struct imx219_mode supported_modes[] = {
> >  		.width = 1640,
> >  		.height = 1232,
> >  		.crop = {
> > -			.left = 0,
> > -			.top = 0,
> > +			.left = IMX219_PIXEL_ARRAY_LEFT,
> > +			.top = IMX219_PIXEL_ARRAY_TOP,
> >  			.width = 3280,
> >  			.height = 2464
> >  		},
> > @@ -521,8 +521,8 @@ static const struct imx219_mode supported_modes[] = {
> >  		.width = 640,
> >  		.height = 480,
> >  		.crop = {
> > -			.left = 1000,
> > -			.top = 752,
> > +			.left = 1008,
> > +			.top = 760,
> >  			.width = 1280,
> >  			.height = 960
> >  		},
> > @@ -1045,6 +1045,7 @@ static int imx219_get_selection(struct v4l2_subdev *sd,
> >  		return 0;
> >
> >  	case V4L2_SEL_TGT_CROP_DEFAULT:
> > +	case V4L2_SEL_TGT_CROP_BOUNDS:
> >  		sel->r.top = IMX219_PIXEL_ARRAY_TOP;
> >  		sel->r.left = IMX219_PIXEL_ARRAY_LEFT;
> >  		sel->r.width = IMX219_PIXEL_ARRAY_WIDTH;
> > --
> > 2.27.0
> >
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Re: [PATCH 1/4] media: docs: Describe pixel array properties

[Laurent Pinchart]

Hi Sakari,

On Thu, Aug 20, 2020 at 06:16:04PM +0300, Sakari Ailus wrote:
> On Wed, Aug 19, 2020 at 03:38:40PM +0300, Laurent Pinchart wrote:
> > On Wed, Aug 19, 2020 at 01:20:00PM +0300, Sakari Ailus wrote:
> > > On Wed, Aug 19, 2020 at 04:06:23AM +0300, Laurent Pinchart wrote:
> > > > On Tue, Aug 18, 2020 at 11:17:43AM +0300, Sakari Ailus wrote:
> > > > > On Mon, Aug 10, 2020 at 10:17:57AM +0200, Jacopo Mondi wrote:
> > > > > > On Sun, Aug 09, 2020 at 08:58:21PM +0300, Laurent Pinchart wrote:
> > > > > > > On Wed, Aug 05, 2020 at 12:57:18PM +0200, Jacopo Mondi wrote:
> > > > > > > > The V4L2 selection API are also used to access the pixel array
> > > > > > > > properties of an image sensor, such as the size and position of active
> > > > > > > > pixels and the cropped area of the pixel matrix used to produce images.
> > > > > > > >
> > > > > > > > Currently no clear definition of the different areas that compose an
> > > > > > > > image sensor pixel array matrix is provided in the specification, and
> > > > > > > > the actual meaning of each selection target when applied to an image
> > > > > > > > sensor was not provided.
> > > > > > > >
> > > > > > > > Provide in the sub-device documentation the definition of the pixel
> > > > > > > > matrix properties and the selection target associated to each of them.
> > > > > > > >
> > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > > > > > ---
> > > > > > > >  .../userspace-api/media/v4l/dev-subdev.rst    | 81 +++++++++++++++++++
> > > > > > > >  1 file changed, 81 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > > > > > > index 134d2fb909fa4..c47861dff9b9b 100644
> > > > > > > > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > > > > > > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > > > > > > @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.
> > > > > > > >  ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round
> > > > > > > >  the image size either up or down. :ref:`v4l2-selection-flags`
> > > > > > > >
> > > > > > > > +.. _v4l2-subdev-pixel-array-properties:
> > > > > > > > +
> > > > > > > > +Selection targets for image sensors properties
> > > > > > > > +----------------------------------------------
> > > > > > > > +
> > > > > > > > +The V4L2 selection API can be used on sub-devices that represent an image
> > > > > > > > +sensor to retrieve the sensor's pixel array matrix properties by using the
> > > > > > > > +:ref:`selection <VIDIOC_SUBDEV_G_SELECTION>` ioctls.
> > > > > > > > +
> > > > > > > > +Sub-device drivers for image sensor usually register a single source pad, but in
> > > > > > > > +the case they expose more, the pixel array properties can be accessed from
> > > > > > > > +any of them.
> > > > > > > > +
> > > > > > > > +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,
> > > > > > > > +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve
> > > > > > > > +the immutable properties of the several different areas that compose the sensor
> > > > > > > > +pixel array matrix. Each area describes a rectangle of logically adjacent pixel
> > > > > > > > +units. The logical disposition of pixels is defined by the sensor read-out
> > > > > > > > +starting point and direction, and may differ from the physical disposition of
> > > > > > > > +the pixel units in the pixel array matrix.
> > > > > > > > +
> > > > > > > > +Each pixel matrix portion is contained in a larger rectangle, with the most
> > > > > > > > +largest being the one that describes the pixel matrix physical size. This
> > > > > > > > +defines a hierarchical positional system, where each rectangle is defined
> > > > > > > > +relatively to the largest available one among the ones exposed by the
> > > > > > > > +sub-device driver. Each selection target and the associated pixel array portion
> > > > > > > > +it represents are below presented in order from the largest to the smallest one.
> > > > > > > > +
> > > > > > > > +Pixel array physical size
> > > > > > > > +^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > > > > > +
> > > > > > > > +The image sensor chip is composed by a number of physical pixels, not all of
> > > > > > > > +them readable by the application processor. Invalid or unreadable lines might
> > > > > > > > +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors
> > > > > > > > +they might be tagged with an invalid data type (DT) so that the receiver
> > > > > > > > +automatically discard them. The size of the whole pixel matrix area is
> > > > > > > > +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner
> > > > > > > > +defined as position (0, 0). All the other selection targets are defined
> > > > > > > > +relatively to this, larger, rectangle. The rectangle returned by
> > > > > > > > +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it
> > > > > > > > +does not change at run-time and cannot be modified from userspace.
> > > > > > >
> > > > > > > As I think I've mentioned previously (not sure if it was by e-mail or on
> > > > > > > IRC), we could also decide to set V4L2_SEL_TGT_NATIVE_SIZE ==
> > > > > > > V4L2_SEL_TGT_CROP_BOUNDS by ignoring the non-readable pixels completely.
> > > > > > > What's the advantage of exposing them in the API, when the sensors
> > > > > > > doesn't provide them to the rest of the pipeline ?
> > > > > > 
> > > > > > I don't know :) I'm also  bit confused on what's the purpose of
> > > > > > NATIVE, this commit seems to suggest it was meant to replace
> > > > > > CROP_BOUNDS, but I'm not sure about that.
> > > > > > 
> > > > > > commit b518d86609cc066b626120fe6ec6fe3a4ccfcd54
> > > > > > Author: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > Date:   Thu Nov 6 16:54:33 2014 -0300
> > > > > > 
> > > > > >     [media] smiapp: Support V4L2_SEL_TGT_NATIVE_SIZE
> > > > > > 
> > > > > >     Add support for selection target V4L2_SEL_TGT_NATIVE_SIZE. It is equivalent
> > > > > >     of what V4L2_SEL_TGT_CROP_BOUNDS used to be. Support for
> > > > > >     V4L2_SEL_TGT_CROP_BOUNDS is still supported by the driver as a compatibility
> > > > > >     interface.
> > > > > > 
> > > > > > Sakari, do you recall if that's was the original plan ?
> > > > > 
> > > > > That was to denote the size of the pixel array indeed. We didn't discuss
> > > > > dark or invalid pixels at the time.
> > > > > 
> > > > > So this was just there to tell that it's the pixel array you're cropping
> > > > > from.
> > > > > 
> > > > > But as long as it's API-wise compatible, I don't think anything prevents
> > > > > re-purposing this to include other areas. The documentation (AFAIR) does
> > > > > not say this has to be the same as the crop bounds rectangle.
> > > > 
> > > > What do you think would be best ? Should we include the non-readable
> > > > pixels in V4L2_SEL_TGT_NATIVE_SIZE, with V4L2_SEL_TGT_CROP_BOUNDS then
> > > > being strictly smaller, or drop them completely from the API, with
> > > > V4L2_SEL_TGT_CROP_BOUNDS being equal to V4L2_SEL_TGT_NATIVE_SIZE ? It
> > > > may be that we have to allow both to support existing drivers, but we
> > > > should pick one of the two options and make it mandatory for new
> > > > drivers.
> > > 
> > > That's a very good question.
> > > 
> > > What would be the purpose of adding pixels that cannot be read? I assume
> > > they would not affect sensor timing either in that case, so there would be
> > > no difference whether they are there or not.
> > 
> > Timings is a good point, could there be sensors that read those pixels
> > but don't send them out ? Maybe to avoid edge effects ? That would be
> > accounted for in the H/V blank though, wouldn't it ?
> 
> I guess we could ignore it, as it takes place during what is indeed
> considered as blanking.

Makes sense.

> > > The crop bounds should contain
> > > everything whereas for the default crop should reflect the area of the
> > > visible pixels.
> > 
> > I believe there are sensors that have all pixels visible, but recommend
> > not using edge pixels as they are affected by edge effects, even if
> > those pixels can be read out and transferred. In that case
> > V4L2_SEL_TGT_CROP_BOUNDS should include the edge pixels, but maybe
> > V4L2_SEL_TGT_CROP_DEFAULT shouldn't ?
> 
> I guess so. But in practice I wonder if there are such implementations.

I think it's actually quite common, sensors often have visible pixels on
the edges that are not counted in the nominal sensor resolution, but are
still commonly read out and consumed by the demosaicing operation.
Ideally we should report both the nominal active array (the pixels
guaranteed by the manufacturer to be good), and the potentially larger
exposed pixels array that include margins of potentially lower quality.

> > > I guess in theory the visible pixels could not be cropped by the sensor in
> > > analogue cropping step, so it might be worth having a separate rectangle
> > > for those, too.
> > 
> > I'm not sure to follow you here.
> 
> I'm saying the sensor hardware could in theory be unable to read only the
> visible pixels.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/4] media: docs: Describe pixel array properties

[Sakari Ailus]

Hi Laurent,

On Thu, Nov 26, 2020 at 03:09:43PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Thu, Aug 20, 2020 at 06:16:04PM +0300, Sakari Ailus wrote:
> > On Wed, Aug 19, 2020 at 03:38:40PM +0300, Laurent Pinchart wrote:
> > > On Wed, Aug 19, 2020 at 01:20:00PM +0300, Sakari Ailus wrote:
> > > > On Wed, Aug 19, 2020 at 04:06:23AM +0300, Laurent Pinchart wrote:
> > > > > On Tue, Aug 18, 2020 at 11:17:43AM +0300, Sakari Ailus wrote:
> > > > > > On Mon, Aug 10, 2020 at 10:17:57AM +0200, Jacopo Mondi wrote:
> > > > > > > On Sun, Aug 09, 2020 at 08:58:21PM +0300, Laurent Pinchart wrote:
> > > > > > > > On Wed, Aug 05, 2020 at 12:57:18PM +0200, Jacopo Mondi wrote:
> > > > > > > > > The V4L2 selection API are also used to access the pixel array
> > > > > > > > > properties of an image sensor, such as the size and position of active
> > > > > > > > > pixels and the cropped area of the pixel matrix used to produce images.
> > > > > > > > >
> > > > > > > > > Currently no clear definition of the different areas that compose an
> > > > > > > > > image sensor pixel array matrix is provided in the specification, and
> > > > > > > > > the actual meaning of each selection target when applied to an image
> > > > > > > > > sensor was not provided.
> > > > > > > > >
> > > > > > > > > Provide in the sub-device documentation the definition of the pixel
> > > > > > > > > matrix properties and the selection target associated to each of them.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > > > > > > ---
> > > > > > > > >  .../userspace-api/media/v4l/dev-subdev.rst    | 81 +++++++++++++++++++
> > > > > > > > >  1 file changed, 81 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > > > > > > > index 134d2fb909fa4..c47861dff9b9b 100644
> > > > > > > > > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > > > > > > > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
> > > > > > > > > @@ -386,6 +386,87 @@ requests on all selection targets, unless specifically told otherwise.
> > > > > > > > >  ``V4L2_SEL_FLAG_GE`` and ``V4L2_SEL_FLAG_LE`` flags may be used to round
> > > > > > > > >  the image size either up or down. :ref:`v4l2-selection-flags`
> > > > > > > > >
> > > > > > > > > +.. _v4l2-subdev-pixel-array-properties:
> > > > > > > > > +
> > > > > > > > > +Selection targets for image sensors properties
> > > > > > > > > +----------------------------------------------
> > > > > > > > > +
> > > > > > > > > +The V4L2 selection API can be used on sub-devices that represent an image
> > > > > > > > > +sensor to retrieve the sensor's pixel array matrix properties by using the
> > > > > > > > > +:ref:`selection <VIDIOC_SUBDEV_G_SELECTION>` ioctls.
> > > > > > > > > +
> > > > > > > > > +Sub-device drivers for image sensor usually register a single source pad, but in
> > > > > > > > > +the case they expose more, the pixel array properties can be accessed from
> > > > > > > > > +any of them.
> > > > > > > > > +
> > > > > > > > > +The ``V4L2_SEL_TGT_NATIVE``, ``V4L2_SEL_TGT_CROP_BOUNDS``,
> > > > > > > > > +``V4L2_SEL_TGT_CROP_DEFAULT`` and ``V4L2_TGT_CROP`` targets are used to retrieve
> > > > > > > > > +the immutable properties of the several different areas that compose the sensor
> > > > > > > > > +pixel array matrix. Each area describes a rectangle of logically adjacent pixel
> > > > > > > > > +units. The logical disposition of pixels is defined by the sensor read-out
> > > > > > > > > +starting point and direction, and may differ from the physical disposition of
> > > > > > > > > +the pixel units in the pixel array matrix.
> > > > > > > > > +
> > > > > > > > > +Each pixel matrix portion is contained in a larger rectangle, with the most
> > > > > > > > > +largest being the one that describes the pixel matrix physical size. This
> > > > > > > > > +defines a hierarchical positional system, where each rectangle is defined
> > > > > > > > > +relatively to the largest available one among the ones exposed by the
> > > > > > > > > +sub-device driver. Each selection target and the associated pixel array portion
> > > > > > > > > +it represents are below presented in order from the largest to the smallest one.
> > > > > > > > > +
> > > > > > > > > +Pixel array physical size
> > > > > > > > > +^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > > > > > > +
> > > > > > > > > +The image sensor chip is composed by a number of physical pixels, not all of
> > > > > > > > > +them readable by the application processor. Invalid or unreadable lines might
> > > > > > > > > +not be transmitted on the data bus at all, or in case on CSI-2 capable sensors
> > > > > > > > > +they might be tagged with an invalid data type (DT) so that the receiver
> > > > > > > > > +automatically discard them. The size of the whole pixel matrix area is
> > > > > > > > > +retrieved using the V4L2_SEL_TGT_NATIVE target, which has its top-left corner
> > > > > > > > > +defined as position (0, 0). All the other selection targets are defined
> > > > > > > > > +relatively to this, larger, rectangle. The rectangle returned by
> > > > > > > > > +V4L2_SEL_TGT_NATIVE describes an immutable property of the image sensor, it
> > > > > > > > > +does not change at run-time and cannot be modified from userspace.
> > > > > > > >
> > > > > > > > As I think I've mentioned previously (not sure if it was by e-mail or on
> > > > > > > > IRC), we could also decide to set V4L2_SEL_TGT_NATIVE_SIZE ==
> > > > > > > > V4L2_SEL_TGT_CROP_BOUNDS by ignoring the non-readable pixels completely.
> > > > > > > > What's the advantage of exposing them in the API, when the sensors
> > > > > > > > doesn't provide them to the rest of the pipeline ?
> > > > > > > 
> > > > > > > I don't know :) I'm also  bit confused on what's the purpose of
> > > > > > > NATIVE, this commit seems to suggest it was meant to replace
> > > > > > > CROP_BOUNDS, but I'm not sure about that.
> > > > > > > 
> > > > > > > commit b518d86609cc066b626120fe6ec6fe3a4ccfcd54
> > > > > > > Author: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > > Date:   Thu Nov 6 16:54:33 2014 -0300
> > > > > > > 
> > > > > > >     [media] smiapp: Support V4L2_SEL_TGT_NATIVE_SIZE
> > > > > > > 
> > > > > > >     Add support for selection target V4L2_SEL_TGT_NATIVE_SIZE. It is equivalent
> > > > > > >     of what V4L2_SEL_TGT_CROP_BOUNDS used to be. Support for
> > > > > > >     V4L2_SEL_TGT_CROP_BOUNDS is still supported by the driver as a compatibility
> > > > > > >     interface.
> > > > > > > 
> > > > > > > Sakari, do you recall if that's was the original plan ?
> > > > > > 
> > > > > > That was to denote the size of the pixel array indeed. We didn't discuss
> > > > > > dark or invalid pixels at the time.
> > > > > > 
> > > > > > So this was just there to tell that it's the pixel array you're cropping
> > > > > > from.
> > > > > > 
> > > > > > But as long as it's API-wise compatible, I don't think anything prevents
> > > > > > re-purposing this to include other areas. The documentation (AFAIR) does
> > > > > > not say this has to be the same as the crop bounds rectangle.
> > > > > 
> > > > > What do you think would be best ? Should we include the non-readable
> > > > > pixels in V4L2_SEL_TGT_NATIVE_SIZE, with V4L2_SEL_TGT_CROP_BOUNDS then
> > > > > being strictly smaller, or drop them completely from the API, with
> > > > > V4L2_SEL_TGT_CROP_BOUNDS being equal to V4L2_SEL_TGT_NATIVE_SIZE ? It
> > > > > may be that we have to allow both to support existing drivers, but we
> > > > > should pick one of the two options and make it mandatory for new
> > > > > drivers.
> > > > 
> > > > That's a very good question.
> > > > 
> > > > What would be the purpose of adding pixels that cannot be read? I assume
> > > > they would not affect sensor timing either in that case, so there would be
> > > > no difference whether they are there or not.
> > > 
> > > Timings is a good point, could there be sensors that read those pixels
> > > but don't send them out ? Maybe to avoid edge effects ? That would be
> > > accounted for in the H/V blank though, wouldn't it ?
> > 
> > I guess we could ignore it, as it takes place during what is indeed
> > considered as blanking.
> 
> Makes sense.
> 
> > > > The crop bounds should contain
> > > > everything whereas for the default crop should reflect the area of the
> > > > visible pixels.
> > > 
> > > I believe there are sensors that have all pixels visible, but recommend
> > > not using edge pixels as they are affected by edge effects, even if
> > > those pixels can be read out and transferred. In that case
> > > V4L2_SEL_TGT_CROP_BOUNDS should include the edge pixels, but maybe
> > > V4L2_SEL_TGT_CROP_DEFAULT shouldn't ?
> > 
> > I guess so. But in practice I wonder if there are such implementations.
> 
> I think it's actually quite common, sensors often have visible pixels on
> the edges that are not counted in the nominal sensor resolution, but are
> still commonly read out and consumed by the demosaicing operation.
> Ideally we should report both the nominal active array (the pixels
> guaranteed by the manufacturer to be good), and the potentially larger
> exposed pixels array that include margins of potentially lower quality.

Yes, I think so, too.

But I do also think we'll need a new target for the purpose; this is really
about telling the pixels inside the area are of good quality, and it's
unrelated to cropping.

I wonder what to call it though. V4L2_SEL_TGT_PIXEL_PRETTY? :-)

> 
> > > > I guess in theory the visible pixels could not be cropped by the sensor in
> > > > analogue cropping step, so it might be worth having a separate rectangle
> > > > for those, too.
> > > 
> > > I'm not sure to follow you here.
> > 
> > I'm saying the sensor hardware could in theory be unable to read only the
> > visible pixels.
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Sakari Ailus