[PATCH v9 00/11] media: report camera sensor properties

[PATCH v9 00/11] media: report camera sensor properties

[Jacopo Mondi]

Rebased on latest media master as the documentation has moved since my
last v8.

Hans, is this ready to be collected in your opinion ?

Jacopo Mondi (11):
  dt-bindings: video-interfaces: Document 'location' property
  media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_LOCATION
  dt-bindings: video-interface: Replace 'rotation' description
  media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_ROTATION
  media: v4l2-ctrls: Add camera location and rotation
  media: v4l2-fwnode: Add helper to parse device properties
  include: v4l2-ctrl: Sort forward declarations
  media: v4l2-ctrls: Sort includes alphabetically
  media: v4l2-ctrls: Add helper to register properties
  media: i2c: ov5670: Parse and register properties
  media: i2c: ov13858: Parse and register properties

v8->v9:
- Rebased on media master which has moved media documentation

v7->v8:
- Add Rob's ack to 03/11
- Address Hans typographical comments in 03/11

 .../bindings/media/video-interfaces.txt       | 372 +++++++++++++++++-
 .../media/v4l/ext-ctrls-camera.rst            | 153 +++++++
 drivers/media/i2c/ov13858.c                   |  11 +
 drivers/media/i2c/ov5670.c                    |  12 +
 drivers/media/v4l2-core/v4l2-ctrls.c          |  52 ++-
 drivers/media/v4l2-core/v4l2-fwnode.c         |  42 ++
 include/media/v4l2-ctrls.h                    |  34 +-
 include/media/v4l2-fwnode.h                   |  47 +++
 include/uapi/linux/v4l2-controls.h            |   7 +
 9 files changed, 718 insertions(+), 12 deletions(-)

--
2.26.1

[PATCH v9 01/11] dt-bindings: video-interfaces: Document 'location' property

[Jacopo Mondi]

Add the 'location' device property, used to specify a device mounting
position. The property is particularly meaningful for mobile devices
with a well defined usage orientation.

Reviewed-by: Rob Herring <robh@kernel.org>
Acked-by: Tomasz Figa <tfiga@chromium.org>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 .../devicetree/bindings/media/video-interfaces.txt    | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
index f884ada0bffc..1211bdf80722 100644
--- a/Documentation/devicetree/bindings/media/video-interfaces.txt
+++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
@@ -89,6 +89,17 @@ Optional properties
   but a number of degrees counter clockwise. Typical values are 0 and 180
   (upside down).
 
+- location: The mount location of a device (typically an image sensor or a flash
+  LED) expressed as a position relative to the usage orientation of the system
+  where the device is installed on.
+  Possible values are:
+  0 - Front. The device is mounted on the front facing side of the system.
+  For mobile devices such as smartphones, tablets and laptops the front side is
+  the user facing side.
+  1 - Back. The device is mounted on the back side of the system, which is
+  defined as the opposite side of the front facing one.
+  2 - External. The device is not attached directly to the system but is
+  attached in a way that allows it to move freely.
 
 Optional endpoint properties
 ----------------------------
-- 
2.26.1

[PATCH v9 02/11] media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_LOCATION

[Jacopo Mondi]

Add documentation for the V4L2_CID_CAMERA_SENSOR_LOCATION camera
control. The newly added read-only control reports the camera device
mounting position.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 .../media/v4l/ext-ctrls-camera.rst            | 32 +++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
index e39f84d2447f..01a9042d53a6 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
@@ -510,6 +510,38 @@ enum v4l2_scene_mode -
     value down. A value of zero stops the motion if one is in progress
     and has no effect otherwise.
 
+``V4L2_CID_CAMERA_SENSOR_LOCATION (integer)``
+    This read-only control describes the camera sensor location by reporting
+    its mounting position on the device where the camera is installed. The
+    control value is constant and not modifiable by software. This control is
+    particularly meaningful for devices which have a well defined orientation,
+    such as phones, laptops and portable devices since the camera location is
+    expressed as a position relative to the device's intended usage orientation.
+    For example, a camera sensor installed on the user-facing side of a phone,
+    a tablet or a laptop device is said to be installed in the
+    ``V4L2_LOCATION_FRONT`` location while camera sensors installed on the side
+    opposite the front one are said to be installed in the
+    ``V4L2_LOCATION_BACK`` location. Camera sensors not directly attached to
+    the device or attached in a way that allows them to move freely, such as
+    webcams and digital cameras, are said to have the ``V4L2_LOCATION_EXTERNAL``
+    location.
+
+
+
+.. flat-table::
+    :header-rows:  0
+    :stub-columns: 0
+
+    * - ``V4L2_LOCATION_FRONT``
+      - The camera sensor is located on the front side of the device.
+    * - ``V4L2_LOCATION_BACK``
+      - The camera sensor is located on the back side of the device.
+    * - ``V4L2_LOCATION_EXTERNAL``
+      - The camera sensor is not directly attached to the device and is
+        freely movable.
+
+
+
 .. [#f1]
    This control may be changed to a menu control in the future, if more
    options are required.
-- 
2.26.1

Re: [PATCH v9 02/11] media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_LOCATION

[Mauro Carvalho Chehab]

Em Fri, 17 Apr 2020 14:41:01 +0200
Jacopo Mondi <jacopo@jmondi.org> escreveu:

> Add documentation for the V4L2_CID_CAMERA_SENSOR_LOCATION camera
> control. The newly added read-only control reports the camera device
> mounting position.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  .../media/v4l/ext-ctrls-camera.rst            | 32 +++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> index e39f84d2447f..01a9042d53a6 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> @@ -510,6 +510,38 @@ enum v4l2_scene_mode -
>      value down. A value of zero stops the motion if one is in progress
>      and has no effect otherwise.
>  
> +``V4L2_CID_CAMERA_SENSOR_LOCATION (integer)``
> +    This read-only control describes the camera sensor location by reporting
> +    its mounting position on the device where the camera is installed. The
> +    control value is constant and not modifiable by software. This control is
> +    particularly meaningful for devices which have a well defined orientation,
> +    such as phones, laptops and portable devices since the camera location is
> +    expressed as a position relative to the device's intended usage orientation.
> +    For example, a camera sensor installed on the user-facing side of a phone,
> +    a tablet or a laptop device is said to be installed in the
> +    ``V4L2_LOCATION_FRONT`` location while camera sensors installed on the side
> +    opposite the front one are said to be installed in the
> +    ``V4L2_LOCATION_BACK`` location. Camera sensors not directly attached to
> +    the device or attached in a way that allows them to move freely, such as
> +    webcams and digital cameras, are said to have the ``V4L2_LOCATION_EXTERNAL``
> +    location.
> +
> +
> +
> +.. flat-table::
> +    :header-rows:  0
> +    :stub-columns: 0
> +
> +    * - ``
``
> +      - The camera sensor is located on the front side of the device.
> +    * - ``V4L2_LOCATION_BACK``
> +      - The camera sensor is located on the back side of the device.
> +    * - ``V4L2_LOCATION_EXTERNAL``
> +      - The camera sensor is not directly attached to the device and is
> +        freely movable.

I guess I mentioned this already, but IMHO this ioctl is somewhat flawed,
for two reasons:

1) newer devices may all top of the line mobile devices now are coming
   with multiple camera sensors at the same side. So, just saying that
   the location is front or back is not enough. A location syscall would
   need have something more to better identify the location.
   It probably doesn't need to be something fancy, but, at least, on a
   device with 3 back sensors, I would call them as:

	V4L2_LOCATION_BACK_1
	V4L2_LOCATION_BACK_2
	V4L2_LOCATION_BACK_3

   And add some comment at the control documentation that would allow to
   uniquely number the other ones, like:

	"when multiple sensors are present at the same side, sensors
	 will be numbered considering the ``(x,y)`` coordinates of the center
	 of each sensor, starting from the topmost, leftmost position.

	 She first sensor will be the topmost sensor column at the leftmost
	 side. The other sensors that will have the same ``y`` coordinate,
	 counting from the left to the right, then increment the ``y`` and
	 parse the next column again until all sensors are numbered."

2) There are also some devices that has a movable sensor, that can either
   be taking a picture from the front or from the back, like those:

	https://www.youtube.com/watch?v=br6G7MrkRUc

   On such case, the control should not be read-only, as one may need to
   change this control in order to select if a sensor would either be on
   FRONT or on BACK position.

   For such kind of sensors (when we start supporting them), we could 
   for example call them like:

	V4L2_LOCATION_MOVABLE_IN_BACK_POSITION_1
	V4L2_LOCATION_MOVABLE_IN_BACK_POSITION_2
	V4L2_LOCATION_MOVABLE_IN_FRONT_POSITION_1
	V4L2_LOCATION_MOVABLE_IN_FRONT_POSITION_2

   And add rename the other definitions to:

	V4L2_LOCATION_FIXED_FRONT_1
	V4L2_LOCATION_FIXED_BACK_1

Ok, nobody has yet attempted to upstream code for such devices,
so, we, for now, we don't need to add more than those 3 types.

But, IMO, we would change the sensors description in a way that it
would be easier to add support for more than one sensor per location
in the future, like:

	* - ``V4L2_LOCATION_FIXED_FRONT_1``
          - The camera sensor is fixed, being the first sensor
	    located on the front side of the device.
	* - ``V4L2_LOCATION_FIXED_BACK_1``
	  - The camera sensor is fixed, being the first sensor
	    located on the back side of the device.
	* - ``V4L2_LOCATION_EXTERNAL``
          - The camera sensor is not directly attached to the device
	    and is freely movable.

	.. note:: Please submit a patch upstream if you need to have
		  more than one sensor either at front or back position.

This would make a lot easier when someone upstream patches requiring 
to locate more than one sensor location, or to support flipping
sensors.

Thanks,
Mauro

Re: [PATCH v9 02/11] media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_LOCATION

[Hans Verkuil]

On 05/05/2020 14:02, Mauro Carvalho Chehab wrote:
> Em Fri, 17 Apr 2020 14:41:01 +0200
> Jacopo Mondi <jacopo@jmondi.org> escreveu:
> 
>> Add documentation for the V4L2_CID_CAMERA_SENSOR_LOCATION camera
>> control. The newly added read-only control reports the camera device
>> mounting position.
>>
>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>> ---
>>  .../media/v4l/ext-ctrls-camera.rst            | 32 +++++++++++++++++++
>>  1 file changed, 32 insertions(+)
>>
>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> index e39f84d2447f..01a9042d53a6 100644
>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> @@ -510,6 +510,38 @@ enum v4l2_scene_mode -
>>      value down. A value of zero stops the motion if one is in progress
>>      and has no effect otherwise.
>>  
>> +``V4L2_CID_CAMERA_SENSOR_LOCATION (integer)``
>> +    This read-only control describes the camera sensor location by reporting
>> +    its mounting position on the device where the camera is installed. The
>> +    control value is constant and not modifiable by software. This control is
>> +    particularly meaningful for devices which have a well defined orientation,
>> +    such as phones, laptops and portable devices since the camera location is
>> +    expressed as a position relative to the device's intended usage orientation.
>> +    For example, a camera sensor installed on the user-facing side of a phone,
>> +    a tablet or a laptop device is said to be installed in the
>> +    ``V4L2_LOCATION_FRONT`` location while camera sensors installed on the side
>> +    opposite the front one are said to be installed in the
>> +    ``V4L2_LOCATION_BACK`` location. Camera sensors not directly attached to
>> +    the device or attached in a way that allows them to move freely, such as
>> +    webcams and digital cameras, are said to have the ``V4L2_LOCATION_EXTERNAL``
>> +    location.
>> +
>> +
>> +
>> +.. flat-table::
>> +    :header-rows:  0
>> +    :stub-columns: 0
>> +
>> +    * - ``
> ``
>> +      - The camera sensor is located on the front side of the device.
>> +    * - ``V4L2_LOCATION_BACK``
>> +      - The camera sensor is located on the back side of the device.
>> +    * - ``V4L2_LOCATION_EXTERNAL``
>> +      - The camera sensor is not directly attached to the device and is
>> +        freely movable.
> 
> I guess I mentioned this already, but IMHO this ioctl is somewhat flawed,
> for two reasons:
> 
> 1) newer devices may all top of the line mobile devices now are coming
>    with multiple camera sensors at the same side. So, just saying that
>    the location is front or back is not enough. A location syscall would
>    need have something more to better identify the location.
>    It probably doesn't need to be something fancy, but, at least, on a
>    device with 3 back sensors, I would call them as:
> 
> 	V4L2_LOCATION_BACK_1
> 	V4L2_LOCATION_BACK_2
> 	V4L2_LOCATION_BACK_3
> 
>    And add some comment at the control documentation that would allow to
>    uniquely number the other ones, like:
> 
> 	"when multiple sensors are present at the same side, sensors
> 	 will be numbered considering the ``(x,y)`` coordinates of the center
> 	 of each sensor, starting from the topmost, leftmost position.
> 
> 	 She first sensor will be the topmost sensor column at the leftmost
> 	 side. The other sensors that will have the same ``y`` coordinate,
> 	 counting from the left to the right, then increment the ``y`` and
> 	 parse the next column again until all sensors are numbered."

I think this isn't a good idea. In most cases you do not care about this.

And if you do care about this, then wouldn't it be better to do that through
a new control where you provide the precise coordinates in e.g. mm?

BACK_1/2/3 really doesn't tell you anything other than that there are three
sensors on the back, but we knew that already.

If we need support for the precise location in the future, then let's do that
right and not try to shoehorn into something that wasn't meant for it.

> 
> 2) There are also some devices that has a movable sensor, that can either
>    be taking a picture from the front or from the back, like those:
> 
> 	https://www.youtube.com/watch?v=br6G7MrkRUc
> 
>    On such case, the control should not be read-only, as one may need to
>    change this control in order to select if a sensor would either be on
>    FRONT or on BACK position.
> 
>    For such kind of sensors (when we start supporting them), we could 
>    for example call them like:
> 
> 	V4L2_LOCATION_MOVABLE_IN_BACK_POSITION_1
> 	V4L2_LOCATION_MOVABLE_IN_BACK_POSITION_2
> 	V4L2_LOCATION_MOVABLE_IN_FRONT_POSITION_1
> 	V4L2_LOCATION_MOVABLE_IN_FRONT_POSITION_2

I don't like this. If the driver can tell when the position changes, then it
can update the control's value (it's still read-only because userspace
can't write to it, but that doesn't mean it can't be updated). So there is
no need to call it 'MOVABLE', you just report the correct location. And with
QUERYMENU you can tell that it is movable since multiple possible locations
are reported (BACK and FRONT in this example). If it is fixed, then QUERYMENU
will report only a single location.

This might have some consequences for the DT bindings, though. Not sure
how to represent this there.

If the driver cannot tell what the position is, then it makes no sense for
the driver to expose this location control since it clearly is something that
has to be hardcoded in userspace. I.e., there is no point for userspace to
write to the control and then read back what it wrote :-)

So I disagree that there is a need for FIXED vs MOVABLE, this can be
represented nicely with the current proposal.

Regards,

	Hans

> 
>    And add rename the other definitions to:
> 
> 	V4L2_LOCATION_FIXED_FRONT_1
> 	V4L2_LOCATION_FIXED_BACK_1
> 
> Ok, nobody has yet attempted to upstream code for such devices,
> so, we, for now, we don't need to add more than those 3 types.
> 
> But, IMO, we would change the sensors description in a way that it
> would be easier to add support for more than one sensor per location
> in the future, like:
> 
> 	* - ``V4L2_LOCATION_FIXED_FRONT_1``
>           - The camera sensor is fixed, being the first sensor
> 	    located on the front side of the device.
> 	* - ``V4L2_LOCATION_FIXED_BACK_1``
> 	  - The camera sensor is fixed, being the first sensor
> 	    located on the back side of the device.
> 	* - ``V4L2_LOCATION_EXTERNAL``
>           - The camera sensor is not directly attached to the device
> 	    and is freely movable.
> 
> 	.. note:: Please submit a patch upstream if you need to have
> 		  more than one sensor either at front or back position.
> 
> This would make a lot easier when someone upstream patches requiring 
> to locate more than one sensor location, or to support flipping
> sensors.
> 
> Thanks,
> Mauro
> 

Re: [PATCH v9 02/11] media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_LOCATION

[Mauro Carvalho Chehab]

Em Tue, 5 May 2020 14:21:38 +0200
Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:

> On 05/05/2020 14:02, Mauro Carvalho Chehab wrote:
> > Em Fri, 17 Apr 2020 14:41:01 +0200
> > Jacopo Mondi <jacopo@jmondi.org> escreveu:
> >   
> >> Add documentation for the V4L2_CID_CAMERA_SENSOR_LOCATION camera
> >> control. The newly added read-only control reports the camera device
> >> mounting position.
> >>
> >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >> ---
> >>  .../media/v4l/ext-ctrls-camera.rst            | 32 +++++++++++++++++++
> >>  1 file changed, 32 insertions(+)
> >>
> >> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> >> index e39f84d2447f..01a9042d53a6 100644
> >> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> >> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> >> @@ -510,6 +510,38 @@ enum v4l2_scene_mode -
> >>      value down. A value of zero stops the motion if one is in progress
> >>      and has no effect otherwise.
> >>  
> >> +``V4L2_CID_CAMERA_SENSOR_LOCATION (integer)``
> >> +    This read-only control describes the camera sensor location by reporting
> >> +    its mounting position on the device where the camera is installed. The
> >> +    control value is constant and not modifiable by software. This control is
> >> +    particularly meaningful for devices which have a well defined orientation,
> >> +    such as phones, laptops and portable devices since the camera location is
> >> +    expressed as a position relative to the device's intended usage orientation.
> >> +    For example, a camera sensor installed on the user-facing side of a phone,
> >> +    a tablet or a laptop device is said to be installed in the
> >> +    ``V4L2_LOCATION_FRONT`` location while camera sensors installed on the side
> >> +    opposite the front one are said to be installed in the
> >> +    ``V4L2_LOCATION_BACK`` location. Camera sensors not directly attached to
> >> +    the device or attached in a way that allows them to move freely, such as
> >> +    webcams and digital cameras, are said to have the ``V4L2_LOCATION_EXTERNAL``
> >> +    location.
> >> +
> >> +
> >> +
> >> +.. flat-table::
> >> +    :header-rows:  0
> >> +    :stub-columns: 0
> >> +
> >> +    * - ``  
> > ``  
> >> +      - The camera sensor is located on the front side of the device.
> >> +    * - ``V4L2_LOCATION_BACK``
> >> +      - The camera sensor is located on the back side of the device.
> >> +    * - ``V4L2_LOCATION_EXTERNAL``
> >> +      - The camera sensor is not directly attached to the device and is
> >> +        freely movable.  
> > 
> > I guess I mentioned this already, but IMHO this ioctl is somewhat flawed,
> > for two reasons:
> > 
> > 1) newer devices may all top of the line mobile devices now are coming
> >    with multiple camera sensors at the same side. So, just saying that
> >    the location is front or back is not enough. A location syscall would
> >    need have something more to better identify the location.
> >    It probably doesn't need to be something fancy, but, at least, on a
> >    device with 3 back sensors, I would call them as:
> > 
> > 	V4L2_LOCATION_BACK_1
> > 	V4L2_LOCATION_BACK_2
> > 	V4L2_LOCATION_BACK_3
> > 
> >    And add some comment at the control documentation that would allow to
> >    uniquely number the other ones, like:
> > 
> > 	"when multiple sensors are present at the same side, sensors
> > 	 will be numbered considering the ``(x,y)`` coordinates of the center
> > 	 of each sensor, starting from the topmost, leftmost position.
> > 
> > 	 She first sensor will be the topmost sensor column at the leftmost
> > 	 side. The other sensors that will have the same ``y`` coordinate,
> > 	 counting from the left to the right, then increment the ``y`` and
> > 	 parse the next column again until all sensors are numbered."  
> 
> I think this isn't a good idea. In most cases you do not care about this.

True, because on most cases, the userspace is hardcoded to open, let's say,
video0 for the front sensor or video1 for the back sensor.

This control only makes sense if the userspace is generic enough to accept
sensors on different positions, identifying them at runtime.

With the current proposal, userspace can only work with 2 sensors, as, if
there's a third sensor, userspace won't know how to pick the right one.

For instance, let's assume a car with 4 sensors, one on each side of
the car (right, front); (left, front); (right; back); (left; back).

With the current proposal, userspace can't do anything if it wants
to identify the (right, back) camera.

> And if you do care about this, then wouldn't it be better to do that through
> a new control where you provide the precise coordinates in e.g. mm?
> 
> BACK_1/2/3 really doesn't tell you anything other than that there are three
> sensors on the back, but we knew that already.

No, if we define some criteria about how sensors should be accounted for
(something similar to the text I drafted), the location will be defined.

With the above text, for example, a device with 3 sensors horizontally
aligned, the arrangement will be:

- sensor 1 is on the left;
- sensor 2 in the middle;
- sensor 3 is on the right.

Ok, I agree that writing a text with such criteria sucks, and maybe
just numbering from 1 to n may not be the best thing to do. Yet,
adding coordinates in mm would be just too much information, IMHO.

> If we need support for the precise location in the future, then let's do that
> right and not try to shoehorn into something that wasn't meant for it.

Assuming that all the problems we have so far are to support devices with
2 sensors, by the time we add support for a third sensor, we'll end having
a new ioctl for the same thing: to specify the sensors locations.

We know the drill: having two controls for the same thing makes userspace
more complex and will require backward-compatibility code at the kernel
and at userspace. That's what I want to avoid.

I'm open to other suggestions that won't limit the usage of this control
for devices with just (up to) two sensors.

> 
> > 
> > 2) There are also some devices that has a movable sensor, that can either
> >    be taking a picture from the front or from the back, like those:
> > 
> > 	https://www.youtube.com/watch?v=br6G7MrkRUc
> > 
> >    On such case, the control should not be read-only, as one may need to
> >    change this control in order to select if a sensor would either be on
> >    FRONT or on BACK position.
> > 
> >    For such kind of sensors (when we start supporting them), we could 
> >    for example call them like:
> > 
> > 	V4L2_LOCATION_MOVABLE_IN_BACK_POSITION_1
> > 	V4L2_LOCATION_MOVABLE_IN_BACK_POSITION_2
> > 	V4L2_LOCATION_MOVABLE_IN_FRONT_POSITION_1
> > 	V4L2_LOCATION_MOVABLE_IN_FRONT_POSITION_2  
> 
> I don't like this. If the driver can tell when the position changes, then it
> can update the control's value (it's still read-only because userspace
> can't write to it, but that doesn't mean it can't be updated).

Why userspace can't set it? I mean, if the camera is movable, it
should be up to the application to select the sensor between FRONT
and BACK.

Btw, this is a case where I clearly see value on this ioctl: all cameras
with flippable sensors need a control to switch the sensor's position,
even if the sensor device is hardcoded on some application.

> So there is
> no need to call it 'MOVABLE', you just report the correct location. And with
> QUERYMENU you can tell that it is movable since multiple possible locations
> are reported (BACK and FRONT in this example). If it is fixed, then QUERYMENU
> will report only a single location.
> 
> This might have some consequences for the DT bindings, though. Not sure
> how to represent this there.

I guess DT should contain the default value when the device is turned
off. 

> If the driver cannot tell what the position is, then it makes no sense for
> the driver to expose this location control since it clearly is something that
> has to be hardcoded in userspace. I.e., there is no point for userspace to
> write to the control and then read back what it wrote :-)

Actually there is. When you command a device to switch position, it may
take some time to move the sensor, and such operation may even fail.

So, reading back the position is probably mandatory.

That reminds that it may actually have a third position, to warn
that the sensor was blocked.

Also, some flip sensors may have another position (a "closed"
position).

> So I disagree that there is a need for FIXED vs MOVABLE, this can be
> represented nicely with the current proposal.

Thanks,
Mauro

Re: [PATCH v9 02/11] media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_LOCATION

[Hans Verkuil]

On 05/05/2020 16:58, Mauro Carvalho Chehab wrote:
> Em Tue, 5 May 2020 14:21:38 +0200
> Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:
> 
>> On 05/05/2020 14:02, Mauro Carvalho Chehab wrote:
>>> Em Fri, 17 Apr 2020 14:41:01 +0200
>>> Jacopo Mondi <jacopo@jmondi.org> escreveu:
>>>   
>>>> Add documentation for the V4L2_CID_CAMERA_SENSOR_LOCATION camera
>>>> control. The newly added read-only control reports the camera device
>>>> mounting position.
>>>>
>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>>> ---
>>>>  .../media/v4l/ext-ctrls-camera.rst            | 32 +++++++++++++++++++
>>>>  1 file changed, 32 insertions(+)
>>>>
>>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>>>> index e39f84d2447f..01a9042d53a6 100644
>>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>>>> @@ -510,6 +510,38 @@ enum v4l2_scene_mode -
>>>>      value down. A value of zero stops the motion if one is in progress
>>>>      and has no effect otherwise.
>>>>  
>>>> +``V4L2_CID_CAMERA_SENSOR_LOCATION (integer)``
>>>> +    This read-only control describes the camera sensor location by reporting
>>>> +    its mounting position on the device where the camera is installed. The
>>>> +    control value is constant and not modifiable by software. This control is
>>>> +    particularly meaningful for devices which have a well defined orientation,
>>>> +    such as phones, laptops and portable devices since the camera location is
>>>> +    expressed as a position relative to the device's intended usage orientation.
>>>> +    For example, a camera sensor installed on the user-facing side of a phone,
>>>> +    a tablet or a laptop device is said to be installed in the
>>>> +    ``V4L2_LOCATION_FRONT`` location while camera sensors installed on the side
>>>> +    opposite the front one are said to be installed in the
>>>> +    ``V4L2_LOCATION_BACK`` location. Camera sensors not directly attached to
>>>> +    the device or attached in a way that allows them to move freely, such as
>>>> +    webcams and digital cameras, are said to have the ``V4L2_LOCATION_EXTERNAL``
>>>> +    location.
>>>> +
>>>> +
>>>> +
>>>> +.. flat-table::
>>>> +    :header-rows:  0
>>>> +    :stub-columns: 0
>>>> +
>>>> +    * - ``  
>>> ``  
>>>> +      - The camera sensor is located on the front side of the device.
>>>> +    * - ``V4L2_LOCATION_BACK``
>>>> +      - The camera sensor is located on the back side of the device.
>>>> +    * - ``V4L2_LOCATION_EXTERNAL``
>>>> +      - The camera sensor is not directly attached to the device and is
>>>> +        freely movable.  
>>>
>>> I guess I mentioned this already, but IMHO this ioctl is somewhat flawed,
>>> for two reasons:
>>>
>>> 1) newer devices may all top of the line mobile devices now are coming
>>>    with multiple camera sensors at the same side. So, just saying that
>>>    the location is front or back is not enough. A location syscall would
>>>    need have something more to better identify the location.
>>>    It probably doesn't need to be something fancy, but, at least, on a
>>>    device with 3 back sensors, I would call them as:
>>>
>>> 	V4L2_LOCATION_BACK_1
>>> 	V4L2_LOCATION_BACK_2
>>> 	V4L2_LOCATION_BACK_3
>>>
>>>    And add some comment at the control documentation that would allow to
>>>    uniquely number the other ones, like:
>>>
>>> 	"when multiple sensors are present at the same side, sensors
>>> 	 will be numbered considering the ``(x,y)`` coordinates of the center
>>> 	 of each sensor, starting from the topmost, leftmost position.
>>>
>>> 	 She first sensor will be the topmost sensor column at the leftmost
>>> 	 side. The other sensors that will have the same ``y`` coordinate,
>>> 	 counting from the left to the right, then increment the ``y`` and
>>> 	 parse the next column again until all sensors are numbered."  
>>
>> I think this isn't a good idea. In most cases you do not care about this.
> 
> True, because on most cases, the userspace is hardcoded to open, let's say,
> video0 for the front sensor or video1 for the back sensor.
> 
> This control only makes sense if the userspace is generic enough to accept
> sensors on different positions, identifying them at runtime.
> 
> With the current proposal, userspace can only work with 2 sensors, as, if
> there's a third sensor, userspace won't know how to pick the right one.
> 
> For instance, let's assume a car with 4 sensors, one on each side of
> the car (right, front); (left, front); (right; back); (left; back).
> 
> With the current proposal, userspace can't do anything if it wants
> to identify the (right, back) camera.
> 
>> And if you do care about this, then wouldn't it be better to do that through
>> a new control where you provide the precise coordinates in e.g. mm?
>>
>> BACK_1/2/3 really doesn't tell you anything other than that there are three
>> sensors on the back, but we knew that already.
> 
> No, if we define some criteria about how sensors should be accounted for
> (something similar to the text I drafted), the location will be defined.
> 
> With the above text, for example, a device with 3 sensors horizontally
> aligned, the arrangement will be:
> 
> - sensor 1 is on the left;
> - sensor 2 in the middle;
> - sensor 3 is on the right.

Or sensor 2 is below sensor 1 and sensor 3 is to the right of sensor 1.
It's meaningless information. If you want to specify the location, then
be precise. Especially for stereoscopic sensors (left and right) it is
good to know the exact distance between the sensors. Just calling them
'1' and '2' is not enough.

For sensors you want to know the plane (back/front) and where they are
on that plane (in the case of more than one sensor). That's separate
information that's only needed in the case of more than one sensor.

> 
> Ok, I agree that writing a text with such criteria sucks, and maybe
> just numbering from 1 to n may not be the best thing to do. Yet,
> adding coordinates in mm would be just too much information, IMHO.

Why? Just numbering them makes no sense, it's useless information.

> 
>> If we need support for the precise location in the future, then let's do that
>> right and not try to shoehorn into something that wasn't meant for it.
> 
> Assuming that all the problems we have so far are to support devices with
> 2 sensors, by the time we add support for a third sensor, we'll end having
> a new ioctl for the same thing: to specify the sensors locations.

It's just a control, nothing more.

In most cases all you need to know is if it is a front or back sensor. In
some cases you need to know more: e.g. my Samsung Note 10+ has three sensors
on the back in a vertical row (wide, telephoto, ultrawide), and two sensors
for 3D to the right of them. For those last two you need to know the exact
position relative to one another. For the other sensors all you need to know
is that they are back sensors.

> 
> We know the drill: having two controls for the same thing makes userspace
> more complex and will require backward-compatibility code at the kernel
> and at userspace. That's what I want to avoid.
> 
> I'm open to other suggestions that won't limit the usage of this control
> for devices with just (up to) two sensors.

What backward compatibility code are you talking about? I honestly don't see
the problem here.

> 
>>
>>>
>>> 2) There are also some devices that has a movable sensor, that can either
>>>    be taking a picture from the front or from the back, like those:
>>>
>>> 	https://www.youtube.com/watch?v=br6G7MrkRUc
>>>
>>>    On such case, the control should not be read-only, as one may need to
>>>    change this control in order to select if a sensor would either be on
>>>    FRONT or on BACK position.
>>>
>>>    For such kind of sensors (when we start supporting them), we could 
>>>    for example call them like:
>>>
>>> 	V4L2_LOCATION_MOVABLE_IN_BACK_POSITION_1
>>> 	V4L2_LOCATION_MOVABLE_IN_BACK_POSITION_2
>>> 	V4L2_LOCATION_MOVABLE_IN_FRONT_POSITION_1
>>> 	V4L2_LOCATION_MOVABLE_IN_FRONT_POSITION_2  
>>
>> I don't like this. If the driver can tell when the position changes, then it
>> can update the control's value (it's still read-only because userspace
>> can't write to it, but that doesn't mean it can't be updated).
> 
> Why userspace can't set it? I mean, if the camera is movable, it
> should be up to the application to select the sensor between FRONT
> and BACK.

Ah, right. If you can command the camera to flip from back to front using
a button or something, then yes, it can be writable. Sorry, didn't think of
that. I was thinking that the user would manually move the camera and the
new position would be detected by the driver and reported in the location
control.

In any case, if the location control can be set through the driver by setting
this control, then just drop the READ_ONLY flag. If the control is writable,
then the sensor is movable. Just document this and we're done.

You are making this much more complicated than it need to be IMHO.

> 
> Btw, this is a case where I clearly see value on this ioctl: all cameras

It's a *control*, not a new ioctl.

> with flippable sensors need a control to switch the sensor's position,
> even if the sensor device is hardcoded on some application.
> 
>> So there is
>> no need to call it 'MOVABLE', you just report the correct location. And with
>> QUERYMENU you can tell that it is movable since multiple possible locations
>> are reported (BACK and FRONT in this example). If it is fixed, then QUERYMENU
>> will report only a single location.
>>
>> This might have some consequences for the DT bindings, though. Not sure
>> how to represent this there.
> 
> I guess DT should contain the default value when the device is turned
> off. 
> 
>> If the driver cannot tell what the position is, then it makes no sense for
>> the driver to expose this location control since it clearly is something that
>> has to be hardcoded in userspace. I.e., there is no point for userspace to
>> write to the control and then read back what it wrote :-)
> 
> Actually there is. When you command a device to switch position, it may
> take some time to move the sensor, and such operation may even fail.

Yeah, I forgot about that option.

> 
> So, reading back the position is probably mandatory.

Well, it's a control, so that's standard.

> 
> That reminds that it may actually have a third position, to warn
> that the sensor was blocked.
> 
> Also, some flip sensors may have another position (a "closed"
> position).

It's certainly possible that we need to add new positions to support the
various states of such a movable sensor. But that's no problem. It's just
a menu control, adding new positions is cheap and easy.

I stand by what I said, except that I agree that this control can be
writable in some circumstances, and that should be documented.

I strongly disagree with the notion of BACK_1/2/3 and FRONT_1/2/3: it adds
no meaningful information. If you have multiple sensors and in order to use
them the application needs to know the relative positions (most likely for
3D sensors), then provide the precise position. The unit for that should
probably be micrometer since millimeter is most likely not precise enough
(at least looking at the depth sensors on my camera).

Regards,

	Hans

> 
>> So I disagree that there is a need for FIXED vs MOVABLE, this can be
>> represented nicely with the current proposal.
> 
> Thanks,
> Mauro
> 

Re: [PATCH v9 02/11] media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_LOCATION

[Mauro Carvalho Chehab]

Em Wed, 6 May 2020 10:04:39 +0200
Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:

> On 05/05/2020 16:58, Mauro Carvalho Chehab wrote:
> > Em Tue, 5 May 2020 14:21:38 +0200
> > Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:
> >   
> >> On 05/05/2020 14:02, Mauro Carvalho Chehab wrote:  
> >>> Em Fri, 17 Apr 2020 14:41:01 +0200
> >>> Jacopo Mondi <jacopo@jmondi.org> escreveu:
> >>>     
> >>>> Add documentation for the V4L2_CID_CAMERA_SENSOR_LOCATION camera
> >>>> control. The newly added read-only control reports the camera device
> >>>> mounting position.
> >>>>
> >>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>>> ---
> >>>>  .../media/v4l/ext-ctrls-camera.rst            | 32 +++++++++++++++++++
> >>>>  1 file changed, 32 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> >>>> index e39f84d2447f..01a9042d53a6 100644
> >>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> >>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> >>>> @@ -510,6 +510,38 @@ enum v4l2_scene_mode -
> >>>>      value down. A value of zero stops the motion if one is in progress
> >>>>      and has no effect otherwise.
> >>>>  
> >>>> +``V4L2_CID_CAMERA_SENSOR_LOCATION (integer)``
> >>>> +    This read-only control describes the camera sensor location by reporting
> >>>> +    its mounting position on the device where the camera is installed. The
> >>>> +    control value is constant and not modifiable by software. This control is
> >>>> +    particularly meaningful for devices which have a well defined orientation,
> >>>> +    such as phones, laptops and portable devices since the camera location is
> >>>> +    expressed as a position relative to the device's intended usage orientation.
> >>>> +    For example, a camera sensor installed on the user-facing side of a phone,
> >>>> +    a tablet or a laptop device is said to be installed in the
> >>>> +    ``V4L2_LOCATION_FRONT`` location while camera sensors installed on the side
> >>>> +    opposite the front one are said to be installed in the
> >>>> +    ``V4L2_LOCATION_BACK`` location. Camera sensors not directly attached to
> >>>> +    the device or attached in a way that allows them to move freely, such as
> >>>> +    webcams and digital cameras, are said to have the ``V4L2_LOCATION_EXTERNAL``
> >>>> +    location.
> >>>> +
> >>>> +
> >>>> +
> >>>> +.. flat-table::
> >>>> +    :header-rows:  0
> >>>> +    :stub-columns: 0
> >>>> +
> >>>> +    * - ``    
> >>> ``    
> >>>> +      - The camera sensor is located on the front side of the device.
> >>>> +    * - ``V4L2_LOCATION_BACK``
> >>>> +      - The camera sensor is located on the back side of the device.
> >>>> +    * - ``V4L2_LOCATION_EXTERNAL``
> >>>> +      - The camera sensor is not directly attached to the device and is
> >>>> +        freely movable.    
> >>>
> >>> I guess I mentioned this already, but IMHO this ioctl is somewhat flawed,
> >>> for two reasons:
> >>>
> >>> 1) newer devices may all top of the line mobile devices now are coming
> >>>    with multiple camera sensors at the same side. So, just saying that
> >>>    the location is front or back is not enough. A location syscall would
> >>>    need have something more to better identify the location.
> >>>    It probably doesn't need to be something fancy, but, at least, on a
> >>>    device with 3 back sensors, I would call them as:
> >>>
> >>> 	V4L2_LOCATION_BACK_1
> >>> 	V4L2_LOCATION_BACK_2
> >>> 	V4L2_LOCATION_BACK_3
> >>>
> >>>    And add some comment at the control documentation that would allow to
> >>>    uniquely number the other ones, like:
> >>>
> >>> 	"when multiple sensors are present at the same side, sensors
> >>> 	 will be numbered considering the ``(x,y)`` coordinates of the center
> >>> 	 of each sensor, starting from the topmost, leftmost position.
> >>>
> >>> 	 She first sensor will be the topmost sensor column at the leftmost
> >>> 	 side. The other sensors that will have the same ``y`` coordinate,
> >>> 	 counting from the left to the right, then increment the ``y`` and
> >>> 	 parse the next column again until all sensors are numbered."    
> >>
> >> I think this isn't a good idea. In most cases you do not care about this.  
> > 
> > True, because on most cases, the userspace is hardcoded to open, let's say,
> > video0 for the front sensor or video1 for the back sensor.
> > 
> > This control only makes sense if the userspace is generic enough to accept
> > sensors on different positions, identifying them at runtime.
> > 
> > With the current proposal, userspace can only work with 2 sensors, as, if
> > there's a third sensor, userspace won't know how to pick the right one.
> > 
> > For instance, let's assume a car with 4 sensors, one on each side of
> > the car (right, front); (left, front); (right; back); (left; back).
> > 
> > With the current proposal, userspace can't do anything if it wants
> > to identify the (right, back) camera.
> >   
> >> And if you do care about this, then wouldn't it be better to do that through
> >> a new control where you provide the precise coordinates in e.g. mm?
> >>
> >> BACK_1/2/3 really doesn't tell you anything other than that there are three
> >> sensors on the back, but we knew that already.  
> > 
> > No, if we define some criteria about how sensors should be accounted for
> > (something similar to the text I drafted), the location will be defined.
> > 
> > With the above text, for example, a device with 3 sensors horizontally
> > aligned, the arrangement will be:
> > 
> > - sensor 1 is on the left;
> > - sensor 2 in the middle;
> > - sensor 3 is on the right.  
> 
> Or sensor 2 is below sensor 1 and sensor 3 is to the right of sensor 1.
> It's meaningless information. If you want to specify the location, then
> be precise. Especially for stereoscopic sensors (left and right) it is
> good to know the exact distance between the sensors. Just calling them
> '1' and '2' is not enough.
> 
> For sensors you want to know the plane (back/front) and where they are
> on that plane (in the case of more than one sensor). That's separate
> information that's only needed in the case of more than one sensor.
> 
> > 
> > Ok, I agree that writing a text with such criteria sucks, and maybe
> > just numbering from 1 to n may not be the best thing to do. Yet,
> > adding coordinates in mm would be just too much information, IMHO.  
> 
> Why? Just numbering them makes no sense, it's useless information.
> 
> >   
> >> If we need support for the precise location in the future, then let's do that
> >> right and not try to shoehorn into something that wasn't meant for it.  
> > 
> > Assuming that all the problems we have so far are to support devices with
> > 2 sensors, by the time we add support for a third sensor, we'll end having
> > a new ioctl for the same thing: to specify the sensors locations.  
> 
> It's just a control, nothing more.
> 
> In most cases all you need to know is if it is a front or back sensor. In
> some cases you need to know more: e.g. my Samsung Note 10+ has three sensors
> on the back in a vertical row (wide, telephoto, ultrawide), and two sensors
> for 3D to the right of them. For those last two you need to know the exact
> position relative to one another. For the other sensors all you need to know
> is that they are back sensors.
> 
> > 
> > We know the drill: having two controls for the same thing makes userspace
> > more complex and will require backward-compatibility code at the kernel
> > and at userspace. That's what I want to avoid.
> > 
> > I'm open to other suggestions that won't limit the usage of this control
> > for devices with just (up to) two sensors.  
> 
> What backward compatibility code are you talking about? I honestly don't see
> the problem here.

Right now, we're adding an API that assumes that the video node may have
only up to 2 sensors, and that would cover just one small subset of usecases
(see more below). If it has anything more than that, this control won't help.

One day (probably soon enough, as there are several devices with more than
two sensors already), we'll end adding a proper support for it, and this 
control will become obsoleted, requiring us to think about backward
compatibility issues when this control become deprecated.

That's why I prefer spending some time finding a better way to report it,
avoiding the need of having to do some deprecation logic anytime soon.

> >>>
> >>> 2) There are also some devices that has a movable sensor, that can either
> >>>    be taking a picture from the front or from the back, like those:
> >>>
> >>> 	https://www.youtube.com/watch?v=br6G7MrkRUc
> >>>
> >>>    On such case, the control should not be read-only, as one may need to
> >>>    change this control in order to select if a sensor would either be on
> >>>    FRONT or on BACK position.
> >>>
> >>>    For such kind of sensors (when we start supporting them), we could 
> >>>    for example call them like:
> >>>
> >>> 	V4L2_LOCATION_MOVABLE_IN_BACK_POSITION_1
> >>> 	V4L2_LOCATION_MOVABLE_IN_BACK_POSITION_2
> >>> 	V4L2_LOCATION_MOVABLE_IN_FRONT_POSITION_1
> >>> 	V4L2_LOCATION_MOVABLE_IN_FRONT_POSITION_2    
> >>
> >> I don't like this. If the driver can tell when the position changes, then it
> >> can update the control's value (it's still read-only because userspace
> >> can't write to it, but that doesn't mean it can't be updated).  
> > 
> > Why userspace can't set it? I mean, if the camera is movable, it
> > should be up to the application to select the sensor between FRONT
> > and BACK.  
> 
> Ah, right. If you can command the camera to flip from back to front using
> a button or something, then yes, it can be writable. Sorry, didn't think of
> that. I was thinking that the user would manually move the camera and the
> new position would be detected by the driver and reported in the location
> control.
> 
> In any case, if the location control can be set through the driver by setting
> this control, then just drop the READ_ONLY flag. If the control is writable,
> then the sensor is movable. Just document this and we're done.

Works for me.

> You are making this much more complicated than it need to be IMHO.
> 
> > 
> > Btw, this is a case where I clearly see value on this ioctl: all cameras  
> 
> It's a *control*, not a new ioctl.
> 
> > with flippable sensors need a control to switch the sensor's position,
> > even if the sensor device is hardcoded on some application.
> >   
> >> So there is
> >> no need to call it 'MOVABLE', you just report the correct location. And with
> >> QUERYMENU you can tell that it is movable since multiple possible locations
> >> are reported (BACK and FRONT in this example). If it is fixed, then QUERYMENU
> >> will report only a single location.
> >>
> >> This might have some consequences for the DT bindings, though. Not sure
> >> how to represent this there.  
> > 
> > I guess DT should contain the default value when the device is turned
> > off. 
> >   
> >> If the driver cannot tell what the position is, then it makes no sense for
> >> the driver to expose this location control since it clearly is something that
> >> has to be hardcoded in userspace. I.e., there is no point for userspace to
> >> write to the control and then read back what it wrote :-)  
> > 
> > Actually there is. When you command a device to switch position, it may
> > take some time to move the sensor, and such operation may even fail.  
> 
> Yeah, I forgot about that option.
> 
> > 
> > So, reading back the position is probably mandatory.  
> 
> Well, it's a control, so that's standard.
> 
> > 
> > That reminds that it may actually have a third position, to warn
> > that the sensor was blocked.
> > 
> > Also, some flip sensors may have another position (a "closed"
> > position).  
> 
> It's certainly possible that we need to add new positions to support the
> various states of such a movable sensor. But that's no problem. It's just
> a menu control, adding new positions is cheap and easy.
> 
> I stand by what I said, except that I agree that this control can be
> writable in some circumstances, and that should be documented
> 
> I strongly disagree with the notion of BACK_1/2/3 and FRONT_1/2/3: it adds
> no meaningful information. 

Ok, but if this control would just say where a sensor is mounted
(front, back or unknown/external), naming it as "LOCATION" seems too
ambitious ;-)

What it is actually trying to report is the angle of the sensor, with
regards to the front position, adding currently two possible angles:
0 degrees (front) or 180 degrees (back).

So, I would call it, instead, as V4L2_CID_CAMERA_VIEWING_ANGLE
(or something similar to it).

Having just two pre-defined angles (front/back) only works fine on
devices like cell-phones or tablets, where the sensor cannot be
on some other angle.

If you mount cameras on a larger device, like a car, you may have
some cameras mounted with different angles, for example, the front
cameras could have been mounted with -45, 0 and 45 degrees, in order
to cover a larger region.

So, if that would be ok for you, I can live with a

V4L2_CID_CAMERA_VIEWING_ANGLE (or some similar name) that will
specify the angle where the sensor is mounted (for fixed sensors),
or the current angle, in case of movable ones, being RO for fixed
sensors and RW for movable ones.

Let's postpone discussions for a LOCATION control once this
would be needed by some driver.

> If you have multiple sensors and in order to use
> them the application needs to know the relative positions (most likely for
> 3D sensors), then provide the precise position. The unit for that should
> probably be micrometer since millimeter is most likely not precise enough
> (at least looking at the depth sensors on my camera).

I can see two different types of usage for a real localization control:

1) 3D sensors - for that, micrometer is probably a better measure;
2) multiple sensors, each covering a different view. That could be,
   for example, a back camera on the left side or on at the right side
   of a car. It could also be several sensors at the same side on a long
   product inspection line.

For (2), the distance between sensors could be several meters. So,
perhaps we'll need to either add two different LOCATION controls,
one for 3D and another one for multiple 2D sensors spread to cover
a larger region.

In any case, let's postpone any further discussions about that until
when we have someone needing it.


Thanks,
Mauro

Re: [PATCH v9 02/11] media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_LOCATION

[Jacopo Mondi]

Hello,

On Wed, May 06, 2020 at 11:39:09AM +0200, Mauro Carvalho Chehab wrote:
> Em Wed, 6 May 2020 10:04:39 +0200
> Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:
>
> > On 05/05/2020 16:58, Mauro Carvalho Chehab wrote:
> > > Em Tue, 5 May 2020 14:21:38 +0200
> > > Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:
> > >
> > >> On 05/05/2020 14:02, Mauro Carvalho Chehab wrote:
> > >>> Em Fri, 17 Apr 2020 14:41:01 +0200
> > >>> Jacopo Mondi <jacopo@jmondi.org> escreveu:
> > >>>
> > >>>> Add documentation for the V4L2_CID_CAMERA_SENSOR_LOCATION camera
> > >>>> control. The newly added read-only control reports the camera device
> > >>>> mounting position.
> > >>>>
> > >>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > >>>> ---
> > >>>>  .../media/v4l/ext-ctrls-camera.rst            | 32 +++++++++++++++++++
> > >>>>  1 file changed, 32 insertions(+)
> > >>>>
> > >>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > >>>> index e39f84d2447f..01a9042d53a6 100644
> > >>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > >>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > >>>> @@ -510,6 +510,38 @@ enum v4l2_scene_mode -
> > >>>>      value down. A value of zero stops the motion if one is in progress
> > >>>>      and has no effect otherwise.
> > >>>>
> > >>>> +``V4L2_CID_CAMERA_SENSOR_LOCATION (integer)``
> > >>>> +    This read-only control describes the camera sensor location by reporting
> > >>>> +    its mounting position on the device where the camera is installed. The
> > >>>> +    control value is constant and not modifiable by software. This control is
> > >>>> +    particularly meaningful for devices which have a well defined orientation,
> > >>>> +    such as phones, laptops and portable devices since the camera location is
> > >>>> +    expressed as a position relative to the device's intended usage orientation.
> > >>>> +    For example, a camera sensor installed on the user-facing side of a phone,
> > >>>> +    a tablet or a laptop device is said to be installed in the
> > >>>> +    ``V4L2_LOCATION_FRONT`` location while camera sensors installed on the side
> > >>>> +    opposite the front one are said to be installed in the
> > >>>> +    ``V4L2_LOCATION_BACK`` location. Camera sensors not directly attached to
> > >>>> +    the device or attached in a way that allows them to move freely, such as
> > >>>> +    webcams and digital cameras, are said to have the ``V4L2_LOCATION_EXTERNAL``
> > >>>> +    location.
> > >>>> +
> > >>>> +
> > >>>> +
> > >>>> +.. flat-table::
> > >>>> +    :header-rows:  0
> > >>>> +    :stub-columns: 0
> > >>>> +
> > >>>> +    * - ``
> > >>> ``
> > >>>> +      - The camera sensor is located on the front side of the device.
> > >>>> +    * - ``V4L2_LOCATION_BACK``
> > >>>> +      - The camera sensor is located on the back side of the device.
> > >>>> +    * - ``V4L2_LOCATION_EXTERNAL``
> > >>>> +      - The camera sensor is not directly attached to the device and is
> > >>>> +        freely movable.
> > >>>
> > >>> I guess I mentioned this already, but IMHO this ioctl is somewhat flawed,
> > >>> for two reasons:
> > >>>
> > >>> 1) newer devices may all top of the line mobile devices now are coming
> > >>>    with multiple camera sensors at the same side. So, just saying that
> > >>>    the location is front or back is not enough. A location syscall would
> > >>>    need have something more to better identify the location.
> > >>>    It probably doesn't need to be something fancy, but, at least, on a
> > >>>    device with 3 back sensors, I would call them as:
> > >>>
> > >>> 	V4L2_LOCATION_BACK_1
> > >>> 	V4L2_LOCATION_BACK_2
> > >>> 	V4L2_LOCATION_BACK_3
> > >>>
> > >>>    And add some comment at the control documentation that would allow to
> > >>>    uniquely number the other ones, like:
> > >>>
> > >>> 	"when multiple sensors are present at the same side, sensors
> > >>> 	 will be numbered considering the ``(x,y)`` coordinates of the center
> > >>> 	 of each sensor, starting from the topmost, leftmost position.
> > >>>
> > >>> 	 She first sensor will be the topmost sensor column at the leftmost
> > >>> 	 side. The other sensors that will have the same ``y`` coordinate,
> > >>> 	 counting from the left to the right, then increment the ``y`` and
> > >>> 	 parse the next column again until all sensors are numbered."
> > >>
> > >> I think this isn't a good idea. In most cases you do not care about this.
> > >
> > > True, because on most cases, the userspace is hardcoded to open, let's say,
> > > video0 for the front sensor or video1 for the back sensor.
> > >
> > > This control only makes sense if the userspace is generic enough to accept
> > > sensors on different positions, identifying them at runtime.
> > >
> > > With the current proposal, userspace can only work with 2 sensors, as, if
> > > there's a third sensor, userspace won't know how to pick the right one.
> > >
> > > For instance, let's assume a car with 4 sensors, one on each side of
> > > the car (right, front); (left, front); (right; back); (left; back).
> > >
> > > With the current proposal, userspace can't do anything if it wants
> > > to identify the (right, back) camera.
> > >
> > >> And if you do care about this, then wouldn't it be better to do that through
> > >> a new control where you provide the precise coordinates in e.g. mm?
> > >>
> > >> BACK_1/2/3 really doesn't tell you anything other than that there are three
> > >> sensors on the back, but we knew that already.
> > >
> > > No, if we define some criteria about how sensors should be accounted for
> > > (something similar to the text I drafted), the location will be defined.
> > >
> > > With the above text, for example, a device with 3 sensors horizontally
> > > aligned, the arrangement will be:
> > >
> > > - sensor 1 is on the left;
> > > - sensor 2 in the middle;
> > > - sensor 3 is on the right.
> >
> > Or sensor 2 is below sensor 1 and sensor 3 is to the right of sensor 1.
> > It's meaningless information. If you want to specify the location, then
> > be precise. Especially for stereoscopic sensors (left and right) it is
> > good to know the exact distance between the sensors. Just calling them
> > '1' and '2' is not enough.
> >
> > For sensors you want to know the plane (back/front) and where they are
> > on that plane (in the case of more than one sensor). That's separate
> > information that's only needed in the case of more than one sensor.
> >
> > >
> > > Ok, I agree that writing a text with such criteria sucks, and maybe
> > > just numbering from 1 to n may not be the best thing to do. Yet,
> > > adding coordinates in mm would be just too much information, IMHO.
> >
> > Why? Just numbering them makes no sense, it's useless information.
> >
> > >
> > >> If we need support for the precise location in the future, then let's do that
> > >> right and not try to shoehorn into something that wasn't meant for it.
> > >
> > > Assuming that all the problems we have so far are to support devices with
> > > 2 sensors, by the time we add support for a third sensor, we'll end having
> > > a new ioctl for the same thing: to specify the sensors locations.
> >
> > It's just a control, nothing more.
> >
> > In most cases all you need to know is if it is a front or back sensor. In
> > some cases you need to know more: e.g. my Samsung Note 10+ has three sensors
> > on the back in a vertical row (wide, telephoto, ultrawide), and two sensors
> > for 3D to the right of them. For those last two you need to know the exact
> > position relative to one another. For the other sensors all you need to know
> > is that they are back sensors.
> >
> > >
> > > We know the drill: having two controls for the same thing makes userspace
> > > more complex and will require backward-compatibility code at the kernel
> > > and at userspace. That's what I want to avoid.
> > >
> > > I'm open to other suggestions that won't limit the usage of this control
> > > for devices with just (up to) two sensors.
> >
> > What backward compatibility code are you talking about? I honestly don't see
> > the problem here.
>
> Right now, we're adding an API that assumes that the video node may have
> only up to 2 sensors, and that would cover just one small subset of usecases
> (see more below). If it has anything more than that, this control won't help.

I don't agree the number of sensor is limited to 2. This property does
not identify sensors, it describes one more thing that userspace might
use to filter cameras. I was actually suprised nothing like this
existed in Linux when I started looking into this issue, as this seems
to me quite basic information that a generic enough userspace
application would like to be able to retrieve.

TL;DR: you can describe as many BACK cameras you want, the 'location'
gives you -one- filtering criteria more, that's it.

>
> One day (probably soon enough, as there are several devices with more than
> two sensors already), we'll end adding a proper support for it, and this
> control will become obsoleted, requiring us to think about backward
> compatibility issues when this control become deprecated.
>
> That's why I prefer spending some time finding a better way to report it,
> avoiding the need of having to do some deprecation logic anytime soon.
>

As said and discussed during the review of this series, a 3-d rotation
matrix is probably the right direction. I refrained from taking that
path because:
1) 99% of devices are interested in reporting FRONT/BACK or some
specialization of those. Asking dt to provide a 9 entries matrix to
say "FRONT" seemed an overkill.
2) There is no consensus on how the reference plane should be defined,
given the wide range of devices that we target. This is a separate
discussion on itself, and given it took 6 months to get to the point
of considering these basic properties, I'm a bit skeptical such a
discussion would have taken less than that.

> > >>>
> > >>> 2) There are also some devices that has a movable sensor, that can either
> > >>>    be taking a picture from the front or from the back, like those:
> > >>>
> > >>> 	https://www.youtube.com/watch?v=br6G7MrkRUc
> > >>>
> > >>>    On such case, the control should not be read-only, as one may need to
> > >>>    change this control in order to select if a sensor would either be on
> > >>>    FRONT or on BACK position.
> > >>>
> > >>>    For such kind of sensors (when we start supporting them), we could
> > >>>    for example call them like:
> > >>>
> > >>> 	V4L2_LOCATION_MOVABLE_IN_BACK_POSITION_1
> > >>> 	V4L2_LOCATION_MOVABLE_IN_BACK_POSITION_2
> > >>> 	V4L2_LOCATION_MOVABLE_IN_FRONT_POSITION_1
> > >>> 	V4L2_LOCATION_MOVABLE_IN_FRONT_POSITION_2
> > >>
> > >> I don't like this. If the driver can tell when the position changes, then it
> > >> can update the control's value (it's still read-only because userspace
> > >> can't write to it, but that doesn't mean it can't be updated).
> > >
> > > Why userspace can't set it? I mean, if the camera is movable, it
> > > should be up to the application to select the sensor between FRONT
> > > and BACK.
> >
> > Ah, right. If you can command the camera to flip from back to front using
> > a button or something, then yes, it can be writable. Sorry, didn't think of
> > that. I was thinking that the user would manually move the camera and the
> > new position would be detected by the driver and reported in the location
> > control.
> >
> > In any case, if the location control can be set through the driver by setting
> > this control, then just drop the READ_ONLY flag. If the control is writable,
> > then the sensor is movable. Just document this and we're done.
>
> Works for me.
>

This makes sense, it will impact bindings in the sense that it now
becomes possible to specify several locations to which select from,
which will become the items of the menu control (with some rule that
says "the first is the default" or such). If more than one location is
specified the control is RW, RO otherwise.

> > You are making this much more complicated than it need to be IMHO.
> >
> > >
> > > Btw, this is a case where I clearly see value on this ioctl: all cameras
> >
> > It's a *control*, not a new ioctl.
> >
> > > with flippable sensors need a control to switch the sensor's position,
> > > even if the sensor device is hardcoded on some application.
> > >
> > >> So there is
> > >> no need to call it 'MOVABLE', you just report the correct location. And with
> > >> QUERYMENU you can tell that it is movable since multiple possible locations
> > >> are reported (BACK and FRONT in this example). If it is fixed, then QUERYMENU
> > >> will report only a single location.
> > >>
> > >> This might have some consequences for the DT bindings, though. Not sure
> > >> how to represent this there.
> > >
> > > I guess DT should contain the default value when the device is turned
> > > off.
> > >
> > >> If the driver cannot tell what the position is, then it makes no sense for
> > >> the driver to expose this location control since it clearly is something that
> > >> has to be hardcoded in userspace. I.e., there is no point for userspace to
> > >> write to the control and then read back what it wrote :-)
> > >
> > > Actually there is. When you command a device to switch position, it may
> > > take some time to move the sensor, and such operation may even fail.
> >
> > Yeah, I forgot about that option.
> >
> > >
> > > So, reading back the position is probably mandatory.
> >
> > Well, it's a control, so that's standard.
> >
> > >
> > > That reminds that it may actually have a third position, to warn
> > > that the sensor was blocked.
> > >
> > > Also, some flip sensors may have another position (a "closed"
> > > position).
> >
> > It's certainly possible that we need to add new positions to support the
> > various states of such a movable sensor. But that's no problem. It's just
> > a menu control, adding new positions is cheap and easy.
> >
> > I stand by what I said, except that I agree that this control can be
> > writable in some circumstances, and that should be documented
> >
> > I strongly disagree with the notion of BACK_1/2/3 and FRONT_1/2/3: it adds
> > no meaningful information.
>
> Ok, but if this control would just say where a sensor is mounted
> (front, back or unknown/external), naming it as "LOCATION" seems too
> ambitious ;-)
>
> What it is actually trying to report is the angle of the sensor, with
> regards to the front position, adding currently two possible angles:
> 0 degrees (front) or 180 degrees (back).
>
> So, I would call it, instead, as V4L2_CID_CAMERA_VIEWING_ANGLE
> (or something similar to it).

_ORIENTATION might be the right word, I'm fine to reserve _LOCATION
for a more precise property if that helps moving forward.

>
> Having just two pre-defined angles (front/back) only works fine on
> devices like cell-phones or tablets, where the sensor cannot be
> on some other angle.
>
> If you mount cameras on a larger device, like a car, you may have
> some cameras mounted with different angles, for example, the front
> cameras could have been mounted with -45, 0 and 45 degrees, in order
> to cover a larger region.

I considered that case, but I expect those very specific usages to be
covered by downstream extensions of the property supported values. I
wish we had a .dts to describe a car in mainlien, but I would be happy
enough to provide a standard mechanism for people to use downstream
eventually, instead of adding custom properties, or taking shortcuts
like it mostly happens today.

>
> So, if that would be ok for you, I can live with a
>
> V4L2_CID_CAMERA_VIEWING_ANGLE (or some similar name) that will
> specify the angle where the sensor is mounted (for fixed sensors),
> or the current angle, in case of movable ones, being RO for fixed
> sensors and RW for movable ones.
>
> Let's postpone discussions for a LOCATION control once this
> would be needed by some driver.

Would V4L2_CID_CAMERA_ORIENTATION work ?

I could:
1) rename dt-proeprty and control to use orientation
2) specify multiple locations could be entered, the first one being
the "default" (eg. device turned off) location
3) make am RW control if multiple entries have been specified, a RO
otherwise.

Ack ?

Thanks
   j

Re: [PATCH v9 02/11] media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_LOCATION

[Mauro Carvalho Chehab]

Em Wed, 6 May 2020 13:07:30 +0200
Jacopo Mondi <jacopo@jmondi.org> escreveu:

> > So, if that would be ok for you, I can live with a
> >
> > V4L2_CID_CAMERA_VIEWING_ANGLE (or some similar name) that will
> > specify the angle where the sensor is mounted (for fixed sensors),
> > or the current angle, in case of movable ones, being RO for fixed
> > sensors and RW for movable ones.
> >
> > Let's postpone discussions for a LOCATION control once this
> > would be needed by some driver.  
> 
> Would V4L2_CID_CAMERA_ORIENTATION work ?

Yeah, either V4L2_CID_CAMERA_ORIENTATION or CID_LENS_FACING would
equally work (although I would prefer the one with a shorter name).

> 
> I could:
> 1) rename dt-proeprty and control to use orientation
> 2) specify multiple locations could be entered, the first one being
> the "default" (eg. device turned off) location
> 3) make am RW control if multiple entries have been specified, a RO
> otherwise.
> 
> Ack ?

Yeah, that would work for me. 

Thanks,
Mauro

Re: [PATCH v9 02/11] media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_LOCATION

[Jacopo Mondi]

Hi Mauro,

On Wed, May 06, 2020 at 01:28:47PM +0200, Mauro Carvalho Chehab wrote:
> Em Wed, 6 May 2020 13:07:30 +0200
> Jacopo Mondi <jacopo@jmondi.org> escreveu:
>
> > > So, if that would be ok for you, I can live with a
> > >
> > > V4L2_CID_CAMERA_VIEWING_ANGLE (or some similar name) that will
> > > specify the angle where the sensor is mounted (for fixed sensors),
> > > or the current angle, in case of movable ones, being RO for fixed
> > > sensors and RW for movable ones.
> > >
> > > Let's postpone discussions for a LOCATION control once this
> > > would be needed by some driver.
> >
> > Would V4L2_CID_CAMERA_ORIENTATION work ?
>
> Yeah, either V4L2_CID_CAMERA_ORIENTATION or CID_LENS_FACING would
> equally work (although I would prefer the one with a shorter name).
>

Yeah, CID_LENS_FACING is nice and shorter, but I would refrain from
polluting the LENS_ namespace, this control applies to the whole camera
module, so I would keep it in the CAMERA_ namespace... And
'orientation' gives a nice match with the DT property, which I would
not call 'facing' or 'facing_side' as 'orientation' seems more
appropriate as a dt-property name to me..

> >
> > I could:
> > 1) rename dt-proeprty and control to use orientation
> > 2) specify multiple locations could be entered, the first one being
> > the "default" (eg. device turned off) location
> > 3) make am RW control if multiple entries have been specified, a RO
> > otherwise.
> >
> > Ack ?
>
> Yeah, that would work for me.
>
> Thanks,
> Mauro

Re: [PATCH v9 02/11] media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_LOCATION

[Mauro Carvalho Chehab]

Em Thu, 7 May 2020 14:36:49 +0200
Jacopo Mondi <jacopo@jmondi.org> escreveu:

> Hi Mauro,
> 
> On Wed, May 06, 2020 at 01:28:47PM +0200, Mauro Carvalho Chehab wrote:
> > Em Wed, 6 May 2020 13:07:30 +0200
> > Jacopo Mondi <jacopo@jmondi.org> escreveu:
> >  
> > > > So, if that would be ok for you, I can live with a
> > > >
> > > > V4L2_CID_CAMERA_VIEWING_ANGLE (or some similar name) that will
> > > > specify the angle where the sensor is mounted (for fixed sensors),
> > > > or the current angle, in case of movable ones, being RO for fixed
> > > > sensors and RW for movable ones.
> > > >
> > > > Let's postpone discussions for a LOCATION control once this
> > > > would be needed by some driver.  
> > >
> > > Would V4L2_CID_CAMERA_ORIENTATION work ?  
> >
> > Yeah, either V4L2_CID_CAMERA_ORIENTATION or CID_LENS_FACING would
> > equally work (although I would prefer the one with a shorter name).
> >  
> 
> Yeah, CID_LENS_FACING is nice and shorter, but I would refrain from
> polluting the LENS_ namespace, this control applies to the whole camera
> module, so I would keep it in the CAMERA_ namespace... And
> 'orientation' gives a nice match with the DT property, which I would
> not call 'facing' or 'facing_side' as 'orientation' seems more
> appropriate as a dt-property name to me..

Ok. V4L2_CID_CAMERA_ORIENTATION works for me.

Thanks,
Mauro

Re: [PATCH v9 02/11] media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_LOCATION

[Hans Verkuil]

On 07/05/2020 16:05, Mauro Carvalho Chehab wrote:
> Em Thu, 7 May 2020 14:36:49 +0200
> Jacopo Mondi <jacopo@jmondi.org> escreveu:
> 
>> Hi Mauro,
>>
>> On Wed, May 06, 2020 at 01:28:47PM +0200, Mauro Carvalho Chehab wrote:
>>> Em Wed, 6 May 2020 13:07:30 +0200
>>> Jacopo Mondi <jacopo@jmondi.org> escreveu:
>>>  
>>>>> So, if that would be ok for you, I can live with a
>>>>>
>>>>> V4L2_CID_CAMERA_VIEWING_ANGLE (or some similar name) that will
>>>>> specify the angle where the sensor is mounted (for fixed sensors),
>>>>> or the current angle, in case of movable ones, being RO for fixed
>>>>> sensors and RW for movable ones.
>>>>>
>>>>> Let's postpone discussions for a LOCATION control once this
>>>>> would be needed by some driver.  
>>>>
>>>> Would V4L2_CID_CAMERA_ORIENTATION work ?  
>>>
>>> Yeah, either V4L2_CID_CAMERA_ORIENTATION or CID_LENS_FACING would
>>> equally work (although I would prefer the one with a shorter name).
>>>  
>>
>> Yeah, CID_LENS_FACING is nice and shorter, but I would refrain from
>> polluting the LENS_ namespace, this control applies to the whole camera
>> module, so I would keep it in the CAMERA_ namespace... And
>> 'orientation' gives a nice match with the DT property, which I would
>> not call 'facing' or 'facing_side' as 'orientation' seems more
>> appropriate as a dt-property name to me..
> 
> Ok. V4L2_CID_CAMERA_ORIENTATION works for me.

For me as well.

Regards,

	Hans

Re: [PATCH v9 02/11] media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_LOCATION

[Laurent Pinchart]

Hi Jacopo,

On Wed, May 06, 2020 at 01:07:30PM +0200, Jacopo Mondi wrote:
> On Wed, May 06, 2020 at 11:39:09AM +0200, Mauro Carvalho Chehab wrote:
> > Em Wed, 6 May 2020 10:04:39 +0200 Hans Verkuil escreveu:
> >> On 05/05/2020 16:58, Mauro Carvalho Chehab wrote:
> >>> Em Tue, 5 May 2020 14:21:38 +0200 Hans Verkuil escreveu:
> >>>> On 05/05/2020 14:02, Mauro Carvalho Chehab wrote:
> >>>>> Em Fri, 17 Apr 2020 14:41:01 +0200 Jacopo Mondi escreveu:
> >>>>>> Add documentation for the V4L2_CID_CAMERA_SENSOR_LOCATION camera
> >>>>>> control. The newly added read-only control reports the camera device
> >>>>>> mounting position.
> >>>>>>
> >>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>>>>> ---
> >>>>>>  .../media/v4l/ext-ctrls-camera.rst            | 32 +++++++++++++++++++
> >>>>>>  1 file changed, 32 insertions(+)
> >>>>>>
> >>>>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> >>>>>> index e39f84d2447f..01a9042d53a6 100644
> >>>>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> >>>>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> >>>>>> @@ -510,6 +510,38 @@ enum v4l2_scene_mode -
> >>>>>>      value down. A value of zero stops the motion if one is in progress
> >>>>>>      and has no effect otherwise.
> >>>>>>
> >>>>>> +``V4L2_CID_CAMERA_SENSOR_LOCATION (integer)``
> >>>>>> +    This read-only control describes the camera sensor location by reporting
> >>>>>> +    its mounting position on the device where the camera is installed. The
> >>>>>> +    control value is constant and not modifiable by software. This control is
> >>>>>> +    particularly meaningful for devices which have a well defined orientation,
> >>>>>> +    such as phones, laptops and portable devices since the camera location is
> >>>>>> +    expressed as a position relative to the device's intended usage orientation.
> >>>>>> +    For example, a camera sensor installed on the user-facing side of a phone,
> >>>>>> +    a tablet or a laptop device is said to be installed in the
> >>>>>> +    ``V4L2_LOCATION_FRONT`` location while camera sensors installed on the side
> >>>>>> +    opposite the front one are said to be installed in the
> >>>>>> +    ``V4L2_LOCATION_BACK`` location. Camera sensors not directly attached to
> >>>>>> +    the device or attached in a way that allows them to move freely, such as
> >>>>>> +    webcams and digital cameras, are said to have the ``V4L2_LOCATION_EXTERNAL``
> >>>>>> +    location.
> >>>>>> +
> >>>>>> +
> >>>>>> +
> >>>>>> +.. flat-table::
> >>>>>> +    :header-rows:  0
> >>>>>> +    :stub-columns: 0
> >>>>>> +
> >>>>>> +    * - ``
> >>>>> ``
> >>>>>> +      - The camera sensor is located on the front side of the device.
> >>>>>> +    * - ``V4L2_LOCATION_BACK``
> >>>>>> +      - The camera sensor is located on the back side of the device.
> >>>>>> +    * - ``V4L2_LOCATION_EXTERNAL``
> >>>>>> +      - The camera sensor is not directly attached to the device and is
> >>>>>> +        freely movable.
> >>>>>
> >>>>> I guess I mentioned this already, but IMHO this ioctl is somewhat flawed,
> >>>>> for two reasons:
> >>>>>
> >>>>> 1) newer devices may all top of the line mobile devices now are coming
> >>>>>    with multiple camera sensors at the same side. So, just saying that
> >>>>>    the location is front or back is not enough. A location syscall would
> >>>>>    need have something more to better identify the location.
> >>>>>    It probably doesn't need to be something fancy, but, at least, on a
> >>>>>    device with 3 back sensors, I would call them as:
> >>>>>
> >>>>> 	V4L2_LOCATION_BACK_1
> >>>>> 	V4L2_LOCATION_BACK_2
> >>>>> 	V4L2_LOCATION_BACK_3
> >>>>>
> >>>>>    And add some comment at the control documentation that would allow to
> >>>>>    uniquely number the other ones, like:
> >>>>>
> >>>>> 	"when multiple sensors are present at the same side, sensors
> >>>>> 	 will be numbered considering the ``(x,y)`` coordinates of the center
> >>>>> 	 of each sensor, starting from the topmost, leftmost position.
> >>>>>
> >>>>> 	 She first sensor will be the topmost sensor column at the leftmost
> >>>>> 	 side. The other sensors that will have the same ``y`` coordinate,
> >>>>> 	 counting from the left to the right, then increment the ``y`` and
> >>>>> 	 parse the next column again until all sensors are numbered."
> >>>>
> >>>> I think this isn't a good idea. In most cases you do not care about this.
> >>>
> >>> True, because on most cases, the userspace is hardcoded to open, let's say,
> >>> video0 for the front sensor or video1 for the back sensor.
> >>>
> >>> This control only makes sense if the userspace is generic enough to accept
> >>> sensors on different positions, identifying them at runtime.
> >>>
> >>> With the current proposal, userspace can only work with 2 sensors, as, if
> >>> there's a third sensor, userspace won't know how to pick the right one.
> >>>
> >>> For instance, let's assume a car with 4 sensors, one on each side of
> >>> the car (right, front); (left, front); (right; back); (left; back).
> >>>
> >>> With the current proposal, userspace can't do anything if it wants
> >>> to identify the (right, back) camera.
> >>>
> >>>> And if you do care about this, then wouldn't it be better to do that through
> >>>> a new control where you provide the precise coordinates in e.g. mm?
> >>>>
> >>>> BACK_1/2/3 really doesn't tell you anything other than that there are three
> >>>> sensors on the back, but we knew that already.
> >>>
> >>> No, if we define some criteria about how sensors should be accounted for
> >>> (something similar to the text I drafted), the location will be defined.
> >>>
> >>> With the above text, for example, a device with 3 sensors horizontally
> >>> aligned, the arrangement will be:
> >>>
> >>> - sensor 1 is on the left;
> >>> - sensor 2 in the middle;
> >>> - sensor 3 is on the right.
> >>
> >> Or sensor 2 is below sensor 1 and sensor 3 is to the right of sensor 1.
> >> It's meaningless information. If you want to specify the location, then
> >> be precise. Especially for stereoscopic sensors (left and right) it is
> >> good to know the exact distance between the sensors. Just calling them
> >> '1' and '2' is not enough.
> >>
> >> For sensors you want to know the plane (back/front) and where they are
> >> on that plane (in the case of more than one sensor). That's separate
> >> information that's only needed in the case of more than one sensor.
> >>
> >>>
> >>> Ok, I agree that writing a text with such criteria sucks, and maybe
> >>> just numbering from 1 to n may not be the best thing to do. Yet,
> >>> adding coordinates in mm would be just too much information, IMHO.
> >>
> >> Why? Just numbering them makes no sense, it's useless information.
> >>
> >>>> If we need support for the precise location in the future, then let's do that
> >>>> right and not try to shoehorn into something that wasn't meant for it.
> >>>
> >>> Assuming that all the problems we have so far are to support devices with
> >>> 2 sensors, by the time we add support for a third sensor, we'll end having
> >>> a new ioctl for the same thing: to specify the sensors locations.
> >>
> >> It's just a control, nothing more.
> >>
> >> In most cases all you need to know is if it is a front or back sensor. In
> >> some cases you need to know more: e.g. my Samsung Note 10+ has three sensors
> >> on the back in a vertical row (wide, telephoto, ultrawide), and two sensors
> >> for 3D to the right of them. For those last two you need to know the exact
> >> position relative to one another. For the other sensors all you need to know
> >> is that they are back sensors.
> >>
> >>> We know the drill: having two controls for the same thing makes userspace
> >>> more complex and will require backward-compatibility code at the kernel
> >>> and at userspace. That's what I want to avoid.
> >>>
> >>> I'm open to other suggestions that won't limit the usage of this control
> >>> for devices with just (up to) two sensors.
> >>
> >> What backward compatibility code are you talking about? I honestly don't see
> >> the problem here.
> >
> > Right now, we're adding an API that assumes that the video node may have
> > only up to 2 sensors, and that would cover just one small subset of usecases
> > (see more below). If it has anything more than that, this control won't help.
> 
> I don't agree the number of sensor is limited to 2. This property does
> not identify sensors, it describes one more thing that userspace might
> use to filter cameras. I was actually suprised nothing like this
> existed in Linux when I started looking into this issue, as this seems
> to me quite basic information that a generic enough userspace
> application would like to be able to retrieve.
> 
> TL;DR: you can describe as many BACK cameras you want, the 'location'
> gives you -one- filtering criteria more, that's it.
> 
> > One day (probably soon enough, as there are several devices with more than
> > two sensors already), we'll end adding a proper support for it, and this
> > control will become obsoleted, requiring us to think about backward
> > compatibility issues when this control become deprecated.
> >
> > That's why I prefer spending some time finding a better way to report it,
> > avoiding the need of having to do some deprecation logic anytime soon.
> 
> As said and discussed during the review of this series, a 3-d rotation
> matrix is probably the right direction. I refrained from taking that
> path because:
> 1) 99% of devices are interested in reporting FRONT/BACK or some
> specialization of those. Asking dt to provide a 9 entries matrix to
> say "FRONT" seemed an overkill.
> 2) There is no consensus on how the reference plane should be defined,
> given the wide range of devices that we target. This is a separate
> discussion on itself, and given it took 6 months to get to the point
> of considering these basic properties, I'm a bit skeptical such a
> discussion would have taken less than that.
> 
> >>>>>
> >>>>> 2) There are also some devices that has a movable sensor, that can either
> >>>>>    be taking a picture from the front or from the back, like those:
> >>>>>
> >>>>> 	https://www.youtube.com/watch?v=br6G7MrkRUc
> >>>>>
> >>>>>    On such case, the control should not be read-only, as one may need to
> >>>>>    change this control in order to select if a sensor would either be on
> >>>>>    FRONT or on BACK position.
> >>>>>
> >>>>>    For such kind of sensors (when we start supporting them), we could
> >>>>>    for example call them like:
> >>>>>
> >>>>> 	V4L2_LOCATION_MOVABLE_IN_BACK_POSITION_1
> >>>>> 	V4L2_LOCATION_MOVABLE_IN_BACK_POSITION_2
> >>>>> 	V4L2_LOCATION_MOVABLE_IN_FRONT_POSITION_1
> >>>>> 	V4L2_LOCATION_MOVABLE_IN_FRONT_POSITION_2
> >>>>
> >>>> I don't like this. If the driver can tell when the position changes, then it
> >>>> can update the control's value (it's still read-only because userspace
> >>>> can't write to it, but that doesn't mean it can't be updated).
> >>>
> >>> Why userspace can't set it? I mean, if the camera is movable, it
> >>> should be up to the application to select the sensor between FRONT
> >>> and BACK.
> >>
> >> Ah, right. If you can command the camera to flip from back to front using
> >> a button or something, then yes, it can be writable. Sorry, didn't think of
> >> that. I was thinking that the user would manually move the camera and the
> >> new position would be detected by the driver and reported in the location
> >> control.
> >>
> >> In any case, if the location control can be set through the driver by setting
> >> this control, then just drop the READ_ONLY flag. If the control is writable,
> >> then the sensor is movable. Just document this and we're done.
> >
> > Works for me.
> 
> This makes sense, it will impact bindings in the sense that it now
> becomes possible to specify several locations to which select from,
> which will become the items of the menu control (with some rule that
> says "the first is the default" or such). If more than one location is
> specified the control is RW, RO otherwise.
> 
> >> You are making this much more complicated than it need to be IMHO.
> >>
> >>> Btw, this is a case where I clearly see value on this ioctl: all cameras
> >>
> >> It's a *control*, not a new ioctl.
> >>
> >>> with flippable sensors need a control to switch the sensor's position,
> >>> even if the sensor device is hardcoded on some application.
> >>>
> >>>> So there is
> >>>> no need to call it 'MOVABLE', you just report the correct location. And with
> >>>> QUERYMENU you can tell that it is movable since multiple possible locations
> >>>> are reported (BACK and FRONT in this example). If it is fixed, then QUERYMENU
> >>>> will report only a single location.
> >>>>
> >>>> This might have some consequences for the DT bindings, though. Not sure
> >>>> how to represent this there.
> >>>
> >>> I guess DT should contain the default value when the device is turned
> >>> off.
> >>>
> >>>> If the driver cannot tell what the position is, then it makes no sense for
> >>>> the driver to expose this location control since it clearly is something that
> >>>> has to be hardcoded in userspace. I.e., there is no point for userspace to
> >>>> write to the control and then read back what it wrote :-)
> >>>
> >>> Actually there is. When you command a device to switch position, it may
> >>> take some time to move the sensor, and such operation may even fail.
> >>
> >> Yeah, I forgot about that option.
> >>
> >>>
> >>> So, reading back the position is probably mandatory.
> >>
> >> Well, it's a control, so that's standard.
> >>
> >>>
> >>> That reminds that it may actually have a third position, to warn
> >>> that the sensor was blocked.
> >>>
> >>> Also, some flip sensors may have another position (a "closed"
> >>> position).
> >>
> >> It's certainly possible that we need to add new positions to support the
> >> various states of such a movable sensor. But that's no problem. It's just
> >> a menu control, adding new positions is cheap and easy.
> >>
> >> I stand by what I said, except that I agree that this control can be
> >> writable in some circumstances, and that should be documented
> >>
> >> I strongly disagree with the notion of BACK_1/2/3 and FRONT_1/2/3: it adds
> >> no meaningful information.
> >
> > Ok, but if this control would just say where a sensor is mounted
> > (front, back or unknown/external), naming it as "LOCATION" seems too
> > ambitious ;-)
> >
> > What it is actually trying to report is the angle of the sensor, with
> > regards to the front position, adding currently two possible angles:
> > 0 degrees (front) or 180 degrees (back).
> >
> > So, I would call it, instead, as V4L2_CID_CAMERA_VIEWING_ANGLE
> > (or something similar to it).
> 
> _ORIENTATION might be the right word, I'm fine to reserve _LOCATION
> for a more precise property if that helps moving forward.
> 
> > Having just two pre-defined angles (front/back) only works fine on
> > devices like cell-phones or tablets, where the sensor cannot be
> > on some other angle.
> >
> > If you mount cameras on a larger device, like a car, you may have
> > some cameras mounted with different angles, for example, the front
> > cameras could have been mounted with -45, 0 and 45 degrees, in order
> > to cover a larger region.
> 
> I considered that case, but I expect those very specific usages to be
> covered by downstream extensions of the property supported values. I
> wish we had a .dts to describe a car in mainlien, but I would be happy
> enough to provide a standard mechanism for people to use downstream
> eventually, instead of adding custom properties, or taking shortcuts
> like it mostly happens today.
> 
> > So, if that would be ok for you, I can live with a
> >
> > V4L2_CID_CAMERA_VIEWING_ANGLE (or some similar name) that will
> > specify the angle where the sensor is mounted (for fixed sensors),
> > or the current angle, in case of movable ones, being RO for fixed
> > sensors and RW for movable ones.
> >
> > Let's postpone discussions for a LOCATION control once this
> > would be needed by some driver.
> 
> Would V4L2_CID_CAMERA_ORIENTATION work ?
> 
> I could:
> 1) rename dt-proeprty and control to use orientation
> 2) specify multiple locations could be entered, the first one being
> the "default" (eg. device turned off) location
> 3) make am RW control if multiple entries have been specified, a RO
> otherwise.

I would refrain from doing 2) and 3) at this point. We have no idea how
we will control those devices, as we haven't worked with them, and we
don't know whether flipping the camera could be done through the V4L2
subsystem or would need to involve other APIs. Designing APIs that can't
be tested has so far not been a great success. It's easy to specify the
DT property as a single value and the control as read-only and ease
those restrictions later, it will be more difficult to start with the
read-write case and then change it to something else if we realize it
was a bad idea.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v9 02/11] media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_LOCATION

[Jacopo Mondi]

Hi Laurent,

On Wed, May 06, 2020 at 06:47:41PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Wed, May 06, 2020 at 01:07:30PM +0200, Jacopo Mondi wrote:
> > On Wed, May 06, 2020 at 11:39:09AM +0200, Mauro Carvalho Chehab wrote:
> > > Em Wed, 6 May 2020 10:04:39 +0200 Hans Verkuil escreveu:
> > >> On 05/05/2020 16:58, Mauro Carvalho Chehab wrote:
> > >>> Em Tue, 5 May 2020 14:21:38 +0200 Hans Verkuil escreveu:
> > >>>> On 05/05/2020 14:02, Mauro Carvalho Chehab wrote:
> > >>>>> Em Fri, 17 Apr 2020 14:41:01 +0200 Jacopo Mondi escreveu:
> > >>>>>> Add documentation for the V4L2_CID_CAMERA_SENSOR_LOCATION camera
> > >>>>>> control. The newly added read-only control reports the camera device
> > >>>>>> mounting position.
> > >>>>>>
> > >>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > >>>>>> ---
> > >>>>>>  .../media/v4l/ext-ctrls-camera.rst            | 32 +++++++++++++++++++
> > >>>>>>  1 file changed, 32 insertions(+)
> > >>>>>>
> > >>>>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > >>>>>> index e39f84d2447f..01a9042d53a6 100644
> > >>>>>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > >>>>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > >>>>>> @@ -510,6 +510,38 @@ enum v4l2_scene_mode -
> > >>>>>>      value down. A value of zero stops the motion if one is in progress
> > >>>>>>      and has no effect otherwise.
> > >>>>>>
> > >>>>>> +``V4L2_CID_CAMERA_SENSOR_LOCATION (integer)``
> > >>>>>> +    This read-only control describes the camera sensor location by reporting
> > >>>>>> +    its mounting position on the device where the camera is installed. The
> > >>>>>> +    control value is constant and not modifiable by software. This control is
> > >>>>>> +    particularly meaningful for devices which have a well defined orientation,
> > >>>>>> +    such as phones, laptops and portable devices since the camera location is
> > >>>>>> +    expressed as a position relative to the device's intended usage orientation.
> > >>>>>> +    For example, a camera sensor installed on the user-facing side of a phone,
> > >>>>>> +    a tablet or a laptop device is said to be installed in the
> > >>>>>> +    ``V4L2_LOCATION_FRONT`` location while camera sensors installed on the side
> > >>>>>> +    opposite the front one are said to be installed in the
> > >>>>>> +    ``V4L2_LOCATION_BACK`` location. Camera sensors not directly attached to
> > >>>>>> +    the device or attached in a way that allows them to move freely, such as
> > >>>>>> +    webcams and digital cameras, are said to have the ``V4L2_LOCATION_EXTERNAL``
> > >>>>>> +    location.
> > >>>>>> +
> > >>>>>> +
> > >>>>>> +
> > >>>>>> +.. flat-table::
> > >>>>>> +    :header-rows:  0
> > >>>>>> +    :stub-columns: 0
> > >>>>>> +
> > >>>>>> +    * - ``
> > >>>>> ``
> > >>>>>> +      - The camera sensor is located on the front side of the device.
> > >>>>>> +    * - ``V4L2_LOCATION_BACK``
> > >>>>>> +      - The camera sensor is located on the back side of the device.
> > >>>>>> +    * - ``V4L2_LOCATION_EXTERNAL``
> > >>>>>> +      - The camera sensor is not directly attached to the device and is
> > >>>>>> +        freely movable.
> > >>>>>
> > >>>>> I guess I mentioned this already, but IMHO this ioctl is somewhat flawed,
> > >>>>> for two reasons:
> > >>>>>
> > >>>>> 1) newer devices may all top of the line mobile devices now are coming
> > >>>>>    with multiple camera sensors at the same side. So, just saying that
> > >>>>>    the location is front or back is not enough. A location syscall would
> > >>>>>    need have something more to better identify the location.
> > >>>>>    It probably doesn't need to be something fancy, but, at least, on a
> > >>>>>    device with 3 back sensors, I would call them as:
> > >>>>>
> > >>>>> 	V4L2_LOCATION_BACK_1
> > >>>>> 	V4L2_LOCATION_BACK_2
> > >>>>> 	V4L2_LOCATION_BACK_3
> > >>>>>
> > >>>>>    And add some comment at the control documentation that would allow to
> > >>>>>    uniquely number the other ones, like:
> > >>>>>
> > >>>>> 	"when multiple sensors are present at the same side, sensors
> > >>>>> 	 will be numbered considering the ``(x,y)`` coordinates of the center
> > >>>>> 	 of each sensor, starting from the topmost, leftmost position.
> > >>>>>
> > >>>>> 	 She first sensor will be the topmost sensor column at the leftmost
> > >>>>> 	 side. The other sensors that will have the same ``y`` coordinate,
> > >>>>> 	 counting from the left to the right, then increment the ``y`` and
> > >>>>> 	 parse the next column again until all sensors are numbered."
> > >>>>
> > >>>> I think this isn't a good idea. In most cases you do not care about this.
> > >>>
> > >>> True, because on most cases, the userspace is hardcoded to open, let's say,
> > >>> video0 for the front sensor or video1 for the back sensor.
> > >>>
> > >>> This control only makes sense if the userspace is generic enough to accept
> > >>> sensors on different positions, identifying them at runtime.
> > >>>
> > >>> With the current proposal, userspace can only work with 2 sensors, as, if
> > >>> there's a third sensor, userspace won't know how to pick the right one.
> > >>>
> > >>> For instance, let's assume a car with 4 sensors, one on each side of
> > >>> the car (right, front); (left, front); (right; back); (left; back).
> > >>>
> > >>> With the current proposal, userspace can't do anything if it wants
> > >>> to identify the (right, back) camera.
> > >>>
> > >>>> And if you do care about this, then wouldn't it be better to do that through
> > >>>> a new control where you provide the precise coordinates in e.g. mm?
> > >>>>
> > >>>> BACK_1/2/3 really doesn't tell you anything other than that there are three
> > >>>> sensors on the back, but we knew that already.
> > >>>
> > >>> No, if we define some criteria about how sensors should be accounted for
> > >>> (something similar to the text I drafted), the location will be defined.
> > >>>
> > >>> With the above text, for example, a device with 3 sensors horizontally
> > >>> aligned, the arrangement will be:
> > >>>
> > >>> - sensor 1 is on the left;
> > >>> - sensor 2 in the middle;
> > >>> - sensor 3 is on the right.
> > >>
> > >> Or sensor 2 is below sensor 1 and sensor 3 is to the right of sensor 1.
> > >> It's meaningless information. If you want to specify the location, then
> > >> be precise. Especially for stereoscopic sensors (left and right) it is
> > >> good to know the exact distance between the sensors. Just calling them
> > >> '1' and '2' is not enough.
> > >>
> > >> For sensors you want to know the plane (back/front) and where they are
> > >> on that plane (in the case of more than one sensor). That's separate
> > >> information that's only needed in the case of more than one sensor.
> > >>
> > >>>
> > >>> Ok, I agree that writing a text with such criteria sucks, and maybe
> > >>> just numbering from 1 to n may not be the best thing to do. Yet,
> > >>> adding coordinates in mm would be just too much information, IMHO.
> > >>
> > >> Why? Just numbering them makes no sense, it's useless information.
> > >>
> > >>>> If we need support for the precise location in the future, then let's do that
> > >>>> right and not try to shoehorn into something that wasn't meant for it.
> > >>>
> > >>> Assuming that all the problems we have so far are to support devices with
> > >>> 2 sensors, by the time we add support for a third sensor, we'll end having
> > >>> a new ioctl for the same thing: to specify the sensors locations.
> > >>
> > >> It's just a control, nothing more.
> > >>
> > >> In most cases all you need to know is if it is a front or back sensor. In
> > >> some cases you need to know more: e.g. my Samsung Note 10+ has three sensors
> > >> on the back in a vertical row (wide, telephoto, ultrawide), and two sensors
> > >> for 3D to the right of them. For those last two you need to know the exact
> > >> position relative to one another. For the other sensors all you need to know
> > >> is that they are back sensors.
> > >>
> > >>> We know the drill: having two controls for the same thing makes userspace
> > >>> more complex and will require backward-compatibility code at the kernel
> > >>> and at userspace. That's what I want to avoid.
> > >>>
> > >>> I'm open to other suggestions that won't limit the usage of this control
> > >>> for devices with just (up to) two sensors.
> > >>
> > >> What backward compatibility code are you talking about? I honestly don't see
> > >> the problem here.
> > >
> > > Right now, we're adding an API that assumes that the video node may have
> > > only up to 2 sensors, and that would cover just one small subset of usecases
> > > (see more below). If it has anything more than that, this control won't help.
> >
> > I don't agree the number of sensor is limited to 2. This property does
> > not identify sensors, it describes one more thing that userspace might
> > use to filter cameras. I was actually suprised nothing like this
> > existed in Linux when I started looking into this issue, as this seems
> > to me quite basic information that a generic enough userspace
> > application would like to be able to retrieve.
> >
> > TL;DR: you can describe as many BACK cameras you want, the 'location'
> > gives you -one- filtering criteria more, that's it.
> >
> > > One day (probably soon enough, as there are several devices with more than
> > > two sensors already), we'll end adding a proper support for it, and this
> > > control will become obsoleted, requiring us to think about backward
> > > compatibility issues when this control become deprecated.
> > >
> > > That's why I prefer spending some time finding a better way to report it,
> > > avoiding the need of having to do some deprecation logic anytime soon.
> >
> > As said and discussed during the review of this series, a 3-d rotation
> > matrix is probably the right direction. I refrained from taking that
> > path because:
> > 1) 99% of devices are interested in reporting FRONT/BACK or some
> > specialization of those. Asking dt to provide a 9 entries matrix to
> > say "FRONT" seemed an overkill.
> > 2) There is no consensus on how the reference plane should be defined,
> > given the wide range of devices that we target. This is a separate
> > discussion on itself, and given it took 6 months to get to the point
> > of considering these basic properties, I'm a bit skeptical such a
> > discussion would have taken less than that.
> >
> > >>>>>
> > >>>>> 2) There are also some devices that has a movable sensor, that can either
> > >>>>>    be taking a picture from the front or from the back, like those:
> > >>>>>
> > >>>>> 	https://www.youtube.com/watch?v=br6G7MrkRUc
> > >>>>>
> > >>>>>    On such case, the control should not be read-only, as one may need to
> > >>>>>    change this control in order to select if a sensor would either be on
> > >>>>>    FRONT or on BACK position.
> > >>>>>
> > >>>>>    For such kind of sensors (when we start supporting them), we could
> > >>>>>    for example call them like:
> > >>>>>
> > >>>>> 	V4L2_LOCATION_MOVABLE_IN_BACK_POSITION_1
> > >>>>> 	V4L2_LOCATION_MOVABLE_IN_BACK_POSITION_2
> > >>>>> 	V4L2_LOCATION_MOVABLE_IN_FRONT_POSITION_1
> > >>>>> 	V4L2_LOCATION_MOVABLE_IN_FRONT_POSITION_2
> > >>>>
> > >>>> I don't like this. If the driver can tell when the position changes, then it
> > >>>> can update the control's value (it's still read-only because userspace
> > >>>> can't write to it, but that doesn't mean it can't be updated).
> > >>>
> > >>> Why userspace can't set it? I mean, if the camera is movable, it
> > >>> should be up to the application to select the sensor between FRONT
> > >>> and BACK.
> > >>
> > >> Ah, right. If you can command the camera to flip from back to front using
> > >> a button or something, then yes, it can be writable. Sorry, didn't think of
> > >> that. I was thinking that the user would manually move the camera and the
> > >> new position would be detected by the driver and reported in the location
> > >> control.
> > >>
> > >> In any case, if the location control can be set through the driver by setting
> > >> this control, then just drop the READ_ONLY flag. If the control is writable,
> > >> then the sensor is movable. Just document this and we're done.
> > >
> > > Works for me.
> >
> > This makes sense, it will impact bindings in the sense that it now
> > becomes possible to specify several locations to which select from,
> > which will become the items of the menu control (with some rule that
> > says "the first is the default" or such). If more than one location is
> > specified the control is RW, RO otherwise.
> >
> > >> You are making this much more complicated than it need to be IMHO.
> > >>
> > >>> Btw, this is a case where I clearly see value on this ioctl: all cameras
> > >>
> > >> It's a *control*, not a new ioctl.
> > >>
> > >>> with flippable sensors need a control to switch the sensor's position,
> > >>> even if the sensor device is hardcoded on some application.
> > >>>
> > >>>> So there is
> > >>>> no need to call it 'MOVABLE', you just report the correct location. And with
> > >>>> QUERYMENU you can tell that it is movable since multiple possible locations
> > >>>> are reported (BACK and FRONT in this example). If it is fixed, then QUERYMENU
> > >>>> will report only a single location.
> > >>>>
> > >>>> This might have some consequences for the DT bindings, though. Not sure
> > >>>> how to represent this there.
> > >>>
> > >>> I guess DT should contain the default value when the device is turned
> > >>> off.
> > >>>
> > >>>> If the driver cannot tell what the position is, then it makes no sense for
> > >>>> the driver to expose this location control since it clearly is something that
> > >>>> has to be hardcoded in userspace. I.e., there is no point for userspace to
> > >>>> write to the control and then read back what it wrote :-)
> > >>>
> > >>> Actually there is. When you command a device to switch position, it may
> > >>> take some time to move the sensor, and such operation may even fail.
> > >>
> > >> Yeah, I forgot about that option.
> > >>
> > >>>
> > >>> So, reading back the position is probably mandatory.
> > >>
> > >> Well, it's a control, so that's standard.
> > >>
> > >>>
> > >>> That reminds that it may actually have a third position, to warn
> > >>> that the sensor was blocked.
> > >>>
> > >>> Also, some flip sensors may have another position (a "closed"
> > >>> position).
> > >>
> > >> It's certainly possible that we need to add new positions to support the
> > >> various states of such a movable sensor. But that's no problem. It's just
> > >> a menu control, adding new positions is cheap and easy.
> > >>
> > >> I stand by what I said, except that I agree that this control can be
> > >> writable in some circumstances, and that should be documented
> > >>
> > >> I strongly disagree with the notion of BACK_1/2/3 and FRONT_1/2/3: it adds
> > >> no meaningful information.
> > >
> > > Ok, but if this control would just say where a sensor is mounted
> > > (front, back or unknown/external), naming it as "LOCATION" seems too
> > > ambitious ;-)
> > >
> > > What it is actually trying to report is the angle of the sensor, with
> > > regards to the front position, adding currently two possible angles:
> > > 0 degrees (front) or 180 degrees (back).
> > >
> > > So, I would call it, instead, as V4L2_CID_CAMERA_VIEWING_ANGLE
> > > (or something similar to it).
> >
> > _ORIENTATION might be the right word, I'm fine to reserve _LOCATION
> > for a more precise property if that helps moving forward.
> >
> > > Having just two pre-defined angles (front/back) only works fine on
> > > devices like cell-phones or tablets, where the sensor cannot be
> > > on some other angle.
> > >
> > > If you mount cameras on a larger device, like a car, you may have
> > > some cameras mounted with different angles, for example, the front
> > > cameras could have been mounted with -45, 0 and 45 degrees, in order
> > > to cover a larger region.
> >
> > I considered that case, but I expect those very specific usages to be
> > covered by downstream extensions of the property supported values. I
> > wish we had a .dts to describe a car in mainlien, but I would be happy
> > enough to provide a standard mechanism for people to use downstream
> > eventually, instead of adding custom properties, or taking shortcuts
> > like it mostly happens today.
> >
> > > So, if that would be ok for you, I can live with a
> > >
> > > V4L2_CID_CAMERA_VIEWING_ANGLE (or some similar name) that will
> > > specify the angle where the sensor is mounted (for fixed sensors),
> > > or the current angle, in case of movable ones, being RO for fixed
> > > sensors and RW for movable ones.
> > >
> > > Let's postpone discussions for a LOCATION control once this
> > > would be needed by some driver.
> >
> > Would V4L2_CID_CAMERA_ORIENTATION work ?
> >
> > I could:
> > 1) rename dt-proeprty and control to use orientation
> > 2) specify multiple locations could be entered, the first one being
> > the "default" (eg. device turned off) location
> > 3) make am RW control if multiple entries have been specified, a RO
> > otherwise.
>
> I would refrain from doing 2) and 3) at this point. We have no idea how
> we will control those devices, as we haven't worked with them, and we
> don't know whether flipping the camera could be done through the V4L2
> subsystem or would need to involve other APIs. Designing APIs that can't
> be tested has so far not been a great success. It's easy to specify the
> DT property as a single value and the control as read-only and ease
> those restrictions later, it will be more difficult to start with the
> read-write case and then change it to something else if we realize it
> was a bad idea.
>

Sure we don't have use cases at hand.. I'm fine post-poning then.
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v9 02/11] media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_LOCATION

[Jacopo Mondi]

Hello,

On Tue, May 05, 2020 at 02:21:38PM +0200, Hans Verkuil wrote:
> On 05/05/2020 14:02, Mauro Carvalho Chehab wrote:
> > Em Fri, 17 Apr 2020 14:41:01 +0200
> > Jacopo Mondi <jacopo@jmondi.org> escreveu:
> >
> >> Add documentation for the V4L2_CID_CAMERA_SENSOR_LOCATION camera
> >> control. The newly added read-only control reports the camera device
> >> mounting position.
> >>
> >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >> ---
> >>  .../media/v4l/ext-ctrls-camera.rst            | 32 +++++++++++++++++++
> >>  1 file changed, 32 insertions(+)
> >>
> >> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> >> index e39f84d2447f..01a9042d53a6 100644
> >> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> >> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> >> @@ -510,6 +510,38 @@ enum v4l2_scene_mode -
> >>      value down. A value of zero stops the motion if one is in progress
> >>      and has no effect otherwise.
> >>
> >> +``V4L2_CID_CAMERA_SENSOR_LOCATION (integer)``
> >> +    This read-only control describes the camera sensor location by reporting
> >> +    its mounting position on the device where the camera is installed. The
> >> +    control value is constant and not modifiable by software. This control is
> >> +    particularly meaningful for devices which have a well defined orientation,
> >> +    such as phones, laptops and portable devices since the camera location is
> >> +    expressed as a position relative to the device's intended usage orientation.
> >> +    For example, a camera sensor installed on the user-facing side of a phone,
> >> +    a tablet or a laptop device is said to be installed in the
> >> +    ``V4L2_LOCATION_FRONT`` location while camera sensors installed on the side
> >> +    opposite the front one are said to be installed in the
> >> +    ``V4L2_LOCATION_BACK`` location. Camera sensors not directly attached to
> >> +    the device or attached in a way that allows them to move freely, such as
> >> +    webcams and digital cameras, are said to have the ``V4L2_LOCATION_EXTERNAL``
> >> +    location.
> >> +
> >> +
> >> +
> >> +.. flat-table::
> >> +    :header-rows:  0
> >> +    :stub-columns: 0
> >> +
> >> +    * - ``
> > ``
> >> +      - The camera sensor is located on the front side of the device.
> >> +    * - ``V4L2_LOCATION_BACK``
> >> +      - The camera sensor is located on the back side of the device.
> >> +    * - ``V4L2_LOCATION_EXTERNAL``
> >> +      - The camera sensor is not directly attached to the device and is
> >> +        freely movable.
> >
> > I guess I mentioned this already, but IMHO this ioctl is somewhat flawed,
> > for two reasons:
> >
> > 1) newer devices may all top of the line mobile devices now are coming
> >    with multiple camera sensors at the same side. So, just saying that
> >    the location is front or back is not enough. A location syscall would
> >    need have something more to better identify the location.
> >    It probably doesn't need to be something fancy, but, at least, on a
> >    device with 3 back sensors, I would call them as:
> >
> > 	V4L2_LOCATION_BACK_1
> > 	V4L2_LOCATION_BACK_2
> > 	V4L2_LOCATION_BACK_3
> >
> >    And add some comment at the control documentation that would allow to
> >    uniquely number the other ones, like:
> >
> > 	"when multiple sensors are present at the same side, sensors
> > 	 will be numbered considering the ``(x,y)`` coordinates of the center
> > 	 of each sensor, starting from the topmost, leftmost position.
> >
> > 	 She first sensor will be the topmost sensor column at the leftmost
> > 	 side. The other sensors that will have the same ``y`` coordinate,
> > 	 counting from the left to the right, then increment the ``y`` and
> > 	 parse the next column again until all sensors are numbered."
>
> I think this isn't a good idea. In most cases you do not care about this.
>
> And if you do care about this, then wouldn't it be better to do that through
> a new control where you provide the precise coordinates in e.g. mm?
>
> BACK_1/2/3 really doesn't tell you anything other than that there are three
> sensors on the back, but we knew that already.
>
> If we need support for the precise location in the future, then let's do that
> right and not try to shoehorn into something that wasn't meant for it.

I think the best move forward to describe movable cameras and such
would be to provide a 3D rotation matrix, along the lines of what iio
has in the 'mount-matrix' property as suggested by Rob and Laurent in
the review of the series.

Before going the 'easy' way with this proeprty that just allow to
enumerate fixed locations I considered the idea, but we're still
missing a unique definition for the device usage orientation that the
rotation matrix would be defined for.

This property implements a mechanism that covers most of devices out
there and all devices in mainline. The properties defined here are the
most basic ones, and could be combined and expanded to provide more
precise definition is someone needs to do so (expecially downstream),
but the important part is that the mechanism to retrieve the
information is in place.

>
> >
> > 2) There are also some devices that has a movable sensor, that can either
> >    be taking a picture from the front or from the back, like those:
> >
> > 	https://www.youtube.com/watch?v=br6G7MrkRUc
> >
> >    On such case, the control should not be read-only, as one may need to
> >    change this control in order to select if a sensor would either be on
> >    FRONT or on BACK position.
> >
> >    For such kind of sensors (when we start supporting them), we could
> >    for example call them like:
> >
> > 	V4L2_LOCATION_MOVABLE_IN_BACK_POSITION_1
> > 	V4L2_LOCATION_MOVABLE_IN_BACK_POSITION_2
> > 	V4L2_LOCATION_MOVABLE_IN_FRONT_POSITION_1
> > 	V4L2_LOCATION_MOVABLE_IN_FRONT_POSITION_2
>
> I don't like this. If the driver can tell when the position changes, then it
> can update the control's value (it's still read-only because userspace
> can't write to it, but that doesn't mean it can't be updated). So there is

Yes, the control is read-only as userspace cannot modify it, but
drivers could update it after having gathered its value from firmware.

> no need to call it 'MOVABLE', you just report the correct location. And with
> QUERYMENU you can tell that it is movable since multiple possible locations
> are reported (BACK and FRONT in this example). If it is fixed, then QUERYMENU
> will report only a single location.
>
> This might have some consequences for the DT bindings, though. Not sure
> how to represent this there.
>
> If the driver cannot tell what the position is, then it makes no sense for
> the driver to expose this location control since it clearly is something that
> has to be hardcoded in userspace. I.e., there is no point for userspace to
> write to the control and then read back what it wrote :-)
>
> So I disagree that there is a need for FIXED vs MOVABLE, this can be
> represented nicely with the current proposal.

Just to add that enumerating several BACK_x (or FRONT_x) location
would not help at all userspace, which should be again capable of
recognizing what '_x' conveys.

What userspace could be interested in is something like "find
all BACK cameras" then "use the one with the highest resolution". The
here presented property allows filtering cameras in the system, but
should not try to uniquely identify them, as userspace would most
likely combine several filtering criteria to get to the "right" camera.

Thanks
   j

>
> Regards,
>
> 	Hans
>
> >
> >    And add rename the other definitions to:
> >
> > 	V4L2_LOCATION_FIXED_FRONT_1
> > 	V4L2_LOCATION_FIXED_BACK_1
> >
> > Ok, nobody has yet attempted to upstream code for such devices,
> > so, we, for now, we don't need to add more than those 3 types.
> >
> > But, IMO, we would change the sensors description in a way that it
> > would be easier to add support for more than one sensor per location
> > in the future, like:
> >
> > 	* - ``V4L2_LOCATION_FIXED_FRONT_1``
> >           - The camera sensor is fixed, being the first sensor
> > 	    located on the front side of the device.
> > 	* - ``V4L2_LOCATION_FIXED_BACK_1``
> > 	  - The camera sensor is fixed, being the first sensor
> > 	    located on the back side of the device.
> > 	* - ``V4L2_LOCATION_EXTERNAL``
> >           - The camera sensor is not directly attached to the device
> > 	    and is freely movable.
> >
> > 	.. note:: Please submit a patch upstream if you need to have
> > 		  more than one sensor either at front or back position.
> >
> > This would make a lot easier when someone upstream patches requiring
> > to locate more than one sensor location, or to support flipping
> > sensors.
> >
> > Thanks,
> > Mauro
> >
>

Re: [PATCH v9 02/11] media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_LOCATION

[Mauro Carvalho Chehab]

Em Wed, 6 May 2020 12:47:23 +0200
Jacopo Mondi <jacopo@jmondi.org> escreveu:

> Hello,
> 
> On Tue, May 05, 2020 at 02:21:38PM +0200, Hans Verkuil wrote:
> > On 05/05/2020 14:02, Mauro Carvalho Chehab wrote:  
> > > Em Fri, 17 Apr 2020 14:41:01 +0200
> > > Jacopo Mondi <jacopo@jmondi.org> escreveu:
> > >  
> > >> Add documentation for the V4L2_CID_CAMERA_SENSOR_LOCATION camera
> > >> control. The newly added read-only control reports the camera device
> > >> mounting position.
> > >>
> > >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > >> ---
> > >>  .../media/v4l/ext-ctrls-camera.rst            | 32 +++++++++++++++++++
> > >>  1 file changed, 32 insertions(+)
> > >>
> > >> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > >> index e39f84d2447f..01a9042d53a6 100644
> > >> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > >> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > >> @@ -510,6 +510,38 @@ enum v4l2_scene_mode -
> > >>      value down. A value of zero stops the motion if one is in progress
> > >>      and has no effect otherwise.
> > >>
> > >> +``V4L2_CID_CAMERA_SENSOR_LOCATION (integer)``
> > >> +    This read-only control describes the camera sensor location by reporting
> > >> +    its mounting position on the device where the camera is installed. The
> > >> +    control value is constant and not modifiable by software. This control is
> > >> +    particularly meaningful for devices which have a well defined orientation,
> > >> +    such as phones, laptops and portable devices since the camera location is
> > >> +    expressed as a position relative to the device's intended usage orientation.
> > >> +    For example, a camera sensor installed on the user-facing side of a phone,
> > >> +    a tablet or a laptop device is said to be installed in the
> > >> +    ``V4L2_LOCATION_FRONT`` location while camera sensors installed on the side
> > >> +    opposite the front one are said to be installed in the
> > >> +    ``V4L2_LOCATION_BACK`` location. Camera sensors not directly attached to
> > >> +    the device or attached in a way that allows them to move freely, such as
> > >> +    webcams and digital cameras, are said to have the ``V4L2_LOCATION_EXTERNAL``
> > >> +    location.
> > >> +
> > >> +
> > >> +
> > >> +.. flat-table::
> > >> +    :header-rows:  0
> > >> +    :stub-columns: 0
> > >> +
> > >> +    * - ``  
> > > ``  
> > >> +      - The camera sensor is located on the front side of the device.
> > >> +    * - ``V4L2_LOCATION_BACK``
> > >> +      - The camera sensor is located on the back side of the device.
> > >> +    * - ``V4L2_LOCATION_EXTERNAL``
> > >> +      - The camera sensor is not directly attached to the device and is
> > >> +        freely movable.  
> > >
> > > I guess I mentioned this already, but IMHO this ioctl is somewhat flawed,
> > > for two reasons:
> > >
> > > 1) newer devices may all top of the line mobile devices now are coming
> > >    with multiple camera sensors at the same side. So, just saying that
> > >    the location is front or back is not enough. A location syscall would
> > >    need have something more to better identify the location.
> > >    It probably doesn't need to be something fancy, but, at least, on a
> > >    device with 3 back sensors, I would call them as:
> > >
> > > 	V4L2_LOCATION_BACK_1
> > > 	V4L2_LOCATION_BACK_2
> > > 	V4L2_LOCATION_BACK_3
> > >
> > >    And add some comment at the control documentation that would allow to
> > >    uniquely number the other ones, like:
> > >
> > > 	"when multiple sensors are present at the same side, sensors
> > > 	 will be numbered considering the ``(x,y)`` coordinates of the center
> > > 	 of each sensor, starting from the topmost, leftmost position.
> > >
> > > 	 She first sensor will be the topmost sensor column at the leftmost
> > > 	 side. The other sensors that will have the same ``y`` coordinate,
> > > 	 counting from the left to the right, then increment the ``y`` and
> > > 	 parse the next column again until all sensors are numbered."  
> >
> > I think this isn't a good idea. In most cases you do not care about this.
> >
> > And if you do care about this, then wouldn't it be better to do that through
> > a new control where you provide the precise coordinates in e.g. mm?
> >
> > BACK_1/2/3 really doesn't tell you anything other than that there are three
> > sensors on the back, but we knew that already.
> >
> > If we need support for the precise location in the future, then let's do that
> > right and not try to shoehorn into something that wasn't meant for it.  
> 
> I think the best move forward to describe movable cameras and such
> would be to provide a 3D rotation matrix, along the lines of what iio
> has in the 'mount-matrix' property as suggested by Rob and Laurent in
> the review of the series.
> 
> Before going the 'easy' way with this proeprty that just allow to
> enumerate fixed locations I considered the idea, but we're still
> missing a unique definition for the device usage orientation that the
> rotation matrix would be defined for.
> 
> This property implements a mechanism that covers most of devices out
> there and all devices in mainline. The properties defined here are the
> most basic ones, and could be combined and expanded to provide more
> precise definition is someone needs to do so (expecially downstream),
> but the important part is that the mechanism to retrieve the
> information is in place.

I had some discussions with Laurent about that.

Yeah, a 3D rotation matrix could work. Another option would be to
name this as CID_LENS_FACING, use about the same definition as on
Android:

 https://jmondi.org/android_metadata_tags/docs.html#static_android.lens.poseTranslation

The definition there is arguable (as some devices may have back
screens nowadays), but a name like that is what this control
really does, as it doesn't neither provide a rotation matrix
nor a camera location.

Starting with a "read-only" control sound OK to me, but I would
add some note about flippable changes that can be changed in
runtime between back/front position.

Something like:

.. note:

	Sensors that could have it side flipped is currently out
	of the scope of this control. Some changes on the behavior
	of this control may change when support for such kind of
	devices would be added upstream.

Thanks,
Mauro

[PATCH v9 03/11] dt-bindings: video-interface: Replace 'rotation' description

[Jacopo Mondi]

Replace the 'rotation' property description by providing a definition
relative to the camera sensor pixel array coordinate system and the
captured scene.

Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 .../bindings/media/video-interfaces.txt       | 359 +++++++++++++++++-
 1 file changed, 356 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
index 1211bdf80722..6208d76a4f1e 100644
--- a/Documentation/devicetree/bindings/media/video-interfaces.txt
+++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
@@ -85,9 +85,362 @@ Optional properties
 
 - lens-focus: A phandle to the node of the focus lens controller.
 
-- rotation: The device, typically an image sensor, is not mounted upright,
-  but a number of degrees counter clockwise. Typical values are 0 and 180
-  (upside down).
+- rotation: The camera rotation is expressed as the angular difference in
+  degrees between two reference systems, one relative to the camera module, and
+  one defined on the external world scene to be captured when projected on the
+  image sensor pixel array.
+
+  A camera sensor has a 2-dimensional reference system 'Rc' defined by
+  its pixel array read-out order. The origin is set to the first pixel
+  being read out, the X-axis points along the column read-out direction
+  towards the last columns, and the Y-axis along the row read-out
+  direction towards the last row.
+
+  A typical example for a sensor with a 2592x1944 pixel array matrix
+  observed from the front is:
+
+              2591       X-axis          0
+                <------------------------+ 0
+                .......... ... ..........!
+                .......... ... ..........! Y-axis
+                           ...           !
+                .......... ... ..........!
+                .......... ... ..........! 1943
+                                         V
+
+  The external world scene reference system 'Rs' is a 2-dimensional
+  reference system on the focal plane of the camera module. The origin is
+  placed on the top-left corner of the visible scene, the X-axis points
+  towards the right, and the Y-axis points towards the bottom of the
+  scene. The top, bottom, left and right directions are intentionally not
+  defined and depend on the environment in which the camera is used.
+
+  A typical example of a (very common) picture of a shark swimming from
+  left to right, as seen from the camera, is:
+
+               0               X-axis
+             0 +------------------------------------->
+               !
+               !
+               !
+               !           |\____)\___
+               !           ) _____  __`<
+               !           |/     )/
+               !
+               !
+               !
+               V
+             Y-axis
+
+  with the reference system 'Rs' placed on the camera focal plane:
+
+                                  ¸.·˙!
+                              ¸.·˙    !
+                  _       ¸.·˙        !
+               +-/ \-+¸.·˙            !
+               | (o) |                ! Camera focal plane
+               +-----+˙·.¸            !
+                          ˙·.¸        !
+                              ˙·.¸    !
+                                  ˙·.¸!
+
+  When projected on the sensor's pixel array, the image and the associated
+  reference system 'Rs' are typically (but not always) inverted, due to
+  the camera module's lens optical inversion effect.
+
+  Assuming the above represented scene of the swimming shark, the lens
+  inversion projects the scene and its reference system onto the sensor
+  pixel array, seen from the front of the camera sensor, as follows:
+
+            Y-axis
+               ^
+               !
+               !
+               !
+               !            |\_____)\__
+               !            ) ____  ___.<
+               !            |/    )/
+               !
+               !
+               !
+             0 +------------------------------------->
+               0               X-axis
+
+  Note the shark being upside-down.
+
+  The resulting projected reference system is named 'Rp'.
+
+  The camera rotation property is then defined as the angular difference
+  in the counter-clockwise direction between the camera reference system
+  'Rc' and the projected scene reference system 'Rp'. It is expressed in
+  degrees as a number in the range [0, 360[.
+
+  Examples
+
+  0 degrees camera rotation:
+
+
+                    Y-Rp
+                     ^
+              Y-Rc   !
+               ^     !
+               !     !
+               !     !
+               !     !
+               !     !
+               !     !
+               !     !
+               !     !
+               !   0 +------------------------------------->
+               !     0               X-Rp
+             0 +------------------------------------->
+               0               X-Rc
+
+
+                                X-Rc                0
+               <------------------------------------+ 0
+                           X-Rp                 0   !
+           <------------------------------------+ 0 !
+                                                !   !
+                                                !   !
+                                                !   !
+                                                !   !
+                                                !   !
+                                                !   !
+                                                !   !
+                                                !   V
+                                                !  Y-Rc
+                                                V
+                                               Y-Rp
+
+  90 degrees camera rotation:
+
+               0        Y-Rc
+             0 +-------------------->
+               !   Y-Rp
+               !    ^
+               !    !
+               !    !
+               !    !
+               !    !
+               !    !
+               !    !
+               !    !
+               !    !
+               !    !
+               !  0 +------------------------------------->
+               !    0              X-Rp
+               !
+               !
+               !
+               !
+               V
+              X-Rc
+
+  180 degrees camera rotation:
+
+                                            0
+       <------------------------------------+ 0
+                        X-Rc                !
+              Y-Rp                          !
+               ^                            !
+               !                            !
+               !                            !
+               !                            !
+               !                            !
+               !                            !
+               !                            !
+               !                            V
+               !                           Y-Rc
+             0 +------------------------------------->
+               0              X-Rp
+
+  270 degrees camera rotation:
+
+               0        Y-Rc
+             0 +-------------------->
+               !                                        0
+               !    <-----------------------------------+ 0
+               !                    X-Rp                !
+               !                                        !
+               !                                        !
+               !                                        !
+               !                                        !
+               !                                        !
+               !                                        !
+               !                                        !
+               !                                        !
+               !                                        V
+               !                                       Y-Rp
+               !
+               !
+               !
+               !
+               V
+              X-Rc
+
+
+  Example one - Webcam
+
+  A camera module installed on the user facing part of a laptop screen
+  casing used for video calls. The captured images are meant to be
+  displayed in landscape mode (width > height) on the laptop screen.
+
+  The camera is typically mounted upside-down to compensate the lens
+  optical inversion effect:
+
+                    Y-Rp
+              Y-Rc   ^
+               ^     !
+               !     !
+               !     !       |\_____)\__
+               !     !       ) ____  ___.<
+               !     !       |/    )/
+               !     !
+               !     !
+               !     !
+               !   0 +------------------------------------->
+               !     0           X-Rp
+             0 +------------------------------------->
+               0            X-Rc
+
+  The two reference systems are aligned, the resulting camera rotation is
+  0 degrees, no rotation correction needs to be applied to the resulting
+  image once captured to memory buffers to correctly display it to users:
+
+               +--------------------------------------+
+               !                                      !
+               !                                      !
+               !                                      !
+               !             |\____)\___              !
+               !             ) _____  __`<            !
+               !             |/     )/                !
+               !                                      !
+               !                                      !
+               !                                      !
+               +--------------------------------------+
+
+  If the camera sensor is not mounted upside-down to compensate for the
+  lens optical inversion, the two reference systems will not be aligned,
+  with 'Rp' being rotated 180 degrees relatively to 'Rc':
+
+
+                        X-Rc                0
+       <------------------------------------+ 0
+                                            !
+              Y-Rp                          !
+               ^                            !
+               !                            !
+               !       |\_____)\__          !
+               !       ) ____  ___.<        !
+               !       |/    )/             !
+               !                            !
+               !                            !
+               !                            V
+               !                           Y-Rc
+             0 +------------------------------------->
+               0            X-Rp
+
+  The image once captured to memory will then be rotated by 180 degrees:
+
+               +--------------------------------------+
+               !                                      !
+               !                                      !
+               !                                      !
+               !              __/(_____/|             !
+               !            >.___  ____ (             !
+               !                 \(    \|             !
+               !                                      !
+               !                                      !
+               !                                      !
+               +--------------------------------------+
+
+  A software rotation correction of 180 degrees should be applied to
+  correctly display the image:
+
+               +--------------------------------------+
+               !                                      !
+               !                                      !
+               !                                      !
+               !             |\____)\___              !
+               !             ) _____  __`<            !
+               !             |/     )/                !
+               !                                      !
+               !                                      !
+               !                                      !
+               +--------------------------------------+
+
+  Example two - Phone camera
+
+  A camera installed on the back side of a mobile device facing away from
+  the user. The captured images are meant to be displayed in portrait mode
+  (height > width) to match the device screen orientation and the device
+  usage orientation used when taking the picture.
+
+  The camera sensor is typically mounted with its pixel array longer side
+  aligned to the device longer side, upside-down mounted to compensate for
+  the lens optical inversion effect:
+
+               0        Y-Rc
+             0 +-------------------->
+               !   Y-Rp
+               !    ^
+               !    !
+               !    !
+               !    !
+               !    !            |\_____)\__
+               !    !            ) ____  ___.<
+               !    !            |/    )/
+               !    !
+               !    !
+               !    !
+               !  0 +------------------------------------->
+               !    0                X-Rp
+               !
+               !
+               !
+               !
+               V
+              X-Rc
+
+  The two reference systems are not aligned and the 'Rp' reference
+  system is rotated by 90 degrees in the counter-clockwise direction
+  relatively to the 'Rc' reference system.
+
+  The image once captured to memory will be rotated:
+
+               +-------------------------------------+
+               |                 _ _                 |
+               |                \   /                |
+               |                 | |                 |
+               |                 | |                 |
+               |                 |  >                |
+               |                <  |                 |
+               |                 | |                 |
+               |                   .                 |
+               |                  V                  |
+               +-------------------------------------+
+
+  A correction of 90 degrees in counter-clockwise direction has to be
+  applied to correctly display the image in portrait mode on the device
+  screen:
+
+                        +--------------------+
+                        |                    |
+                        |                    |
+                        |                    |
+                        |                    |
+                        |                    |
+                        |                    |
+                        |   |\____)\___      |
+                        |   ) _____  __`<    |
+                        |   |/     )/        |
+                        |                    |
+                        |                    |
+                        |                    |
+                        |                    |
+                        |                    |
+                        +--------------------+
 
 - location: The mount location of a device (typically an image sensor or a flash
   LED) expressed as a position relative to the usage orientation of the system
-- 
2.26.1

[PATCH v9 04/11] media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_ROTATION

[Jacopo Mondi]

Add documentation for the V4L2_CID_CAMERA_SENSOR_ROTATION camera
control. The newly added read-only control reports the rotation
correction to be applied to images before displaying them to the user.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 .../media/v4l/ext-ctrls-camera.rst            | 121 ++++++++++++++++++
 1 file changed, 121 insertions(+)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
index 01a9042d53a6..140f985c7f06 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
@@ -542,6 +542,127 @@ enum v4l2_scene_mode -
 
 
 
+``V4L2_CID_CAMERA_SENSOR_ROTATION (integer)``
+    This read-only control describes the rotation correction in degrees in the
+    counter-clockwise direction to be applied to the captured images once
+    captured to memory to compensate for the camera sensor mounting rotation.
+
+    For a precise definition of the sensor mounting rotation refer to the
+    extensive description of the 'rotation' properties in the device tree
+    bindings file 'video-interfaces.txt'.
+
+    A few examples are below reported, using a shark swimming from left to
+    right in front of the user as the example scene to capture. ::
+
+                 0               X-axis
+               0 +------------------------------------->
+                 !
+                 !
+                 !
+                 !           |\____)\___
+                 !           ) _____  __`<
+                 !           |/     )/
+                 !
+                 !
+                 !
+                 V
+               Y-axis
+
+    Example one - Webcam
+
+    Assuming you can bring your laptop with you while swimming with sharks,
+    the camera module of the laptop is installed on the user facing part of a
+    laptop screen casing, and is typically used for video calls. The captured
+    images are meant to be displayed in landscape mode (width > height) on the
+    laptop screen.
+
+    The camera is typically mounted upside-down to compensate the lens optical
+    inversion effect. In this case the value of the
+    V4L2_CID_CAMERA_SENSOR_ROTATION control is 0, no rotation is required to
+    display images correctly to the user.
+
+    If the camera sensor is not mounted upside-down it is required to compensate
+    the lens optical inversion effect and the value of the
+    V4L2_CID_CAMERA_SENSOR_ROTATION control is 180 degrees, as images will
+    result rotated when captured to memory. ::
+
+                 +--------------------------------------+
+                 !                                      !
+                 !                                      !
+                 !                                      !
+                 !              __/(_____/|             !
+                 !            >.___  ____ (             !
+                 !                 \(    \|             !
+                 !                                      !
+                 !                                      !
+                 !                                      !
+                 +--------------------------------------+
+
+    A software rotation correction of 180 degrees has to be applied to correctly
+    display the image on the user screen. ::
+
+                 +--------------------------------------+
+                 !                                      !
+                 !                                      !
+                 !                                      !
+                 !             |\____)\___              !
+                 !             ) _____  __`<            !
+                 !             |/     )/                !
+                 !                                      !
+                 !                                      !
+                 !                                      !
+                 +--------------------------------------+
+
+    Example two - Phone camera
+
+    It is more handy to go and swim with sharks with only your mobile phone
+    with you and take pictures with the camera that is installed on the back
+    side of the device, facing away from the user. The captured images are meant
+    to be displayed in portrait mode (height > width) to match the device screen
+    orientation and the device usage orientation used when taking the picture.
+
+    The camera sensor is typically mounted with its pixel array longer side
+    aligned to the device longer side, upside-down mounted to compensate for
+    the lens optical inversion effect.
+
+    The images once captured to memory will be rotated and the value of the
+    V4L2_CID_CAMERA_SENSOR_ROTATION will report a 90 degree rotation. ::
+
+
+                 +-------------------------------------+
+                 |                 _ _                 |
+                 |                \   /                |
+                 |                 | |                 |
+                 |                 | |                 |
+                 |                 |  >                |
+                 |                <  |                 |
+                 |                 | |                 |
+                 |                   .                 |
+                 |                  V                  |
+                 +-------------------------------------+
+
+    A correction of 90 degrees in counter-clockwise direction has to be
+    applied to correctly display the image in portrait mode on the device
+    screen. ::
+
+                          +--------------------+
+                          |                    |
+                          |                    |
+                          |                    |
+                          |                    |
+                          |                    |
+                          |                    |
+                          |   |\____)\___      |
+                          |   ) _____  __`<    |
+                          |   |/     )/        |
+                          |                    |
+                          |                    |
+                          |                    |
+                          |                    |
+                          |                    |
+                          +--------------------+
+
+
 .. [#f1]
    This control may be changed to a menu control in the future, if more
    options are required.
-- 
2.26.1

[PATCH v9 05/11] media: v4l2-ctrls: Add camera location and rotation

[Jacopo Mondi]

Add support for the newly defined V4L2_CID_CAMERA_SENSOR_LOCATION
and V4L2_CID_CAMERA_SENSOR_ROTATION read-only controls used to report
the camera device mounting position and orientation respectively.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 4 ++++
 include/uapi/linux/v4l2-controls.h   | 7 +++++++
 2 files changed, 11 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 93d33d1db4e8..fdb1007212a7 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1015,6 +1015,8 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_PAN_SPEED:		return "Pan, Speed";
 	case V4L2_CID_TILT_SPEED:		return "Tilt, Speed";
 	case V4L2_CID_UNIT_CELL_SIZE:		return "Unit Cell Size";
+	case V4L2_CID_CAMERA_SENSOR_LOCATION:	return "Camera Sensor Location";
+	case V4L2_CID_CAMERA_SENSOR_ROTATION:	return "Camera Sensor Rotation";
 
 	/* FM Radio Modulator controls */
 	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
@@ -1341,6 +1343,8 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 		break;
 	case V4L2_CID_MIN_BUFFERS_FOR_CAPTURE:
 	case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT:
+	case V4L2_CID_CAMERA_SENSOR_LOCATION:
+	case V4L2_CID_CAMERA_SENSOR_ROTATION:
 		*type = V4L2_CTRL_TYPE_INTEGER;
 		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
 		break;
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 1a58d7cc4ccc..3a2d8c2ad23b 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -918,6 +918,13 @@ enum v4l2_auto_focus_range {
 #define V4L2_CID_PAN_SPEED			(V4L2_CID_CAMERA_CLASS_BASE+32)
 #define V4L2_CID_TILT_SPEED			(V4L2_CID_CAMERA_CLASS_BASE+33)
 
+#define V4L2_CID_CAMERA_SENSOR_LOCATION		(V4L2_CID_CAMERA_CLASS_BASE+34)
+#define V4L2_LOCATION_FRONT			0
+#define V4L2_LOCATION_BACK			1
+#define V4L2_LOCATION_EXTERNAL			2
+
+#define V4L2_CID_CAMERA_SENSOR_ROTATION		(V4L2_CID_CAMERA_CLASS_BASE+35)
+
 /* FM Modulator class control IDs */
 
 #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
-- 
2.26.1

[PATCH v9 06/11] media: v4l2-fwnode: Add helper to parse device properties

[Jacopo Mondi]

Add an helper function to parse common device properties in the same
way as v4l2_fwnode_endpoint_parse() parses common endpoint properties.

Parse the 'rotation' and 'location' properties from the firmware
interface.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/v4l2-core/v4l2-fwnode.c | 42 ++++++++++++++++++++++++
 include/media/v4l2-fwnode.h           | 47 +++++++++++++++++++++++++++
 2 files changed, 89 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index 97f0f8b23b5d..1e074e1960a0 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -756,6 +756,48 @@ int v4l2_fwnode_connector_add_link(struct fwnode_handle *fwnode,
 }
 EXPORT_SYMBOL_GPL(v4l2_fwnode_connector_add_link);
 
+int v4l2_fwnode_device_parse(struct device *dev,
+			     struct v4l2_fwnode_device_properties *props)
+{
+	struct fwnode_handle *fwnode = dev_fwnode(dev);
+	u32 val;
+	int ret;
+
+	memset(props, 0, sizeof(*props));
+
+	props->location = V4L2_FWNODE_PROPERTY_UNSET;
+	ret = fwnode_property_read_u32(fwnode, "location", &val);
+	if (!ret) {
+		switch (val) {
+		case V4L2_FWNODE_LOCATION_FRONT:
+		case V4L2_FWNODE_LOCATION_BACK:
+		case V4L2_FWNODE_LOCATION_EXTERNAL:
+			break;
+		default:
+			dev_warn(dev, "Unsupported device location: %u\n", val);
+			return -EINVAL;
+		}
+
+		props->location = val;
+		dev_dbg(dev, "device location: %u\n", val);
+	}
+
+	props->rotation = V4L2_FWNODE_PROPERTY_UNSET;
+	ret = fwnode_property_read_u32(fwnode, "rotation", &val);
+	if (!ret) {
+		if (val >= 360 || val % 90) {
+			dev_warn(dev, "Unsupported device rotation: %u\n", val);
+			return -EINVAL;
+		}
+
+		props->rotation = val;
+		dev_dbg(dev, "device rotation: %u\n", val);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_fwnode_device_parse);
+
 static int
 v4l2_async_notifier_fwnode_parse_endpoint(struct device *dev,
 					  struct v4l2_async_notifier *notifier,
diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
index dd82d6d9764e..b37d0be8b28d 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -109,6 +109,36 @@ struct v4l2_fwnode_endpoint {
 	unsigned int nr_of_link_frequencies;
 };
 
+/**
+ * v4l2_fwnode_location - identify a non initialized property.
+ *
+ * All properties in &struct v4l2_fwnode_device_properties are initialized
+ * to this value.
+ */
+#define V4L2_FWNODE_PROPERTY_UNSET   (-1U)
+
+/**
+ * enum v4l2_fwnode_location - possible device locations
+ * @V4L2_FWNODE_LOCATION_FRONT: device installed on the front side
+ * @V4L2_FWNODE_LOCATION_BACK: device installed on the back side
+ * @V4L2_FWNODE_LOCATION_EXTERNAL: device externally located
+ */
+enum v4l2_fwnode_location {
+	V4L2_FWNODE_LOCATION_FRONT,
+	V4L2_FWNODE_LOCATION_BACK,
+	V4L2_FWNODE_LOCATION_EXTERNAL
+};
+
+/**
+ * struct v4l2_fwnode_device_properties - fwnode device properties
+ * @location: device location. See &enum v4l2_fwnode_location
+ * @rotation: device rotation
+ */
+struct v4l2_fwnode_device_properties {
+	enum v4l2_fwnode_location location;
+	unsigned int rotation;
+};
+
 /**
  * struct v4l2_fwnode_link - a link between two endpoints
  * @local_node: pointer to device_node of this endpoint
@@ -354,6 +384,23 @@ int v4l2_fwnode_connector_parse(struct fwnode_handle *fwnode,
 int v4l2_fwnode_connector_add_link(struct fwnode_handle *fwnode,
 				   struct v4l2_fwnode_connector *connector);
 
+/**
+ * v4l2_fwnode_device_parse() - parse fwnode device properties
+ * @dev: pointer to &struct device
+ * @props: pointer to &struct v4l2_fwnode_device_properties where to store the
+ *	   parsed properties values
+ *
+ * This function parses and validates the V4L2 fwnode device properties from the
+ * firmware interface, and fills the @struct v4l2_fwnode_device_properties
+ * provided by the caller.
+ *
+ * Return:
+ *	% 0 on success
+ *	%-EINVAL if a parsed property value is not valid
+ */
+int v4l2_fwnode_device_parse(struct device *dev,
+			     struct v4l2_fwnode_device_properties *props);
+
 /**
  * typedef parse_endpoint_func - Driver's callback function to be called on
  *	each V4L2 fwnode endpoint.
-- 
2.26.1

[PATCH v9 07/11] include: v4l2-ctrl: Sort forward declarations

[Jacopo Mondi]

Before adding a new forward declaration to the v4l2-ctrls.h header file,
sort the existing ones alphabetically.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/media/v4l2-ctrls.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 7db9e719a583..cf59abafb0d9 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -25,14 +25,14 @@
 
 /* forward references */
 struct file;
+struct poll_table_struct;
+struct v4l2_ctrl;
 struct v4l2_ctrl_handler;
 struct v4l2_ctrl_helper;
-struct v4l2_ctrl;
-struct video_device;
+struct v4l2_fh;
 struct v4l2_subdev;
 struct v4l2_subscribed_event;
-struct v4l2_fh;
-struct poll_table_struct;
+struct video_device;
 
 /**
  * union v4l2_ctrl_ptr - A pointer to a control value.
-- 
2.26.1

[PATCH v9 08/11] media: v4l2-ctrls: Sort includes alphabetically

[Jacopo Mondi]

Before adding a new include directive, sort the existing ones in
alphabetical order.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index fdb1007212a7..03029f92f8f3 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -9,14 +9,14 @@
 #define pr_fmt(fmt) "v4l2-ctrls: " fmt
 
 #include <linux/ctype.h>
+#include <linux/export.h>
 #include <linux/mm.h>
 #include <linux/slab.h>
-#include <linux/export.h>
-#include <media/v4l2-ioctl.h>
-#include <media/v4l2-device.h>
 #include <media/v4l2-ctrls.h>
-#include <media/v4l2-event.h>
 #include <media/v4l2-dev.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-event.h>
+#include <media/v4l2-ioctl.h>
 
 #define dprintk(vdev, fmt, arg...) do {					\
 	if (!WARN_ON(!(vdev)) && ((vdev)->dev_debug & V4L2_DEV_DEBUG_CTRL)) \
-- 
2.26.1

[PATCH v9 09/11] media: v4l2-ctrls: Add helper to register properties

[Jacopo Mondi]

Add an helper function to v4l2-ctrls to register controls associated
with a device property.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 40 ++++++++++++++++++++++++++++
 include/media/v4l2-ctrls.h           | 26 ++++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 03029f92f8f3..539fa9e71c0e 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -16,6 +16,7 @@
 #include <media/v4l2-dev.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-event.h>
+#include <media/v4l2-fwnode.h>
 #include <media/v4l2-ioctl.h>
 
 #define dprintk(vdev, fmt, arg...) do {					\
@@ -4601,3 +4602,42 @@ __poll_t v4l2_ctrl_poll(struct file *file, struct poll_table_struct *wait)
 	return 0;
 }
 EXPORT_SYMBOL(v4l2_ctrl_poll);
+
+int v4l2_ctrl_new_fwnode_properties(struct v4l2_ctrl_handler *hdl,
+				    const struct v4l2_ctrl_ops *ctrl_ops,
+				    const struct v4l2_fwnode_device_properties *p)
+{
+	if (p->location != V4L2_FWNODE_PROPERTY_UNSET) {
+		u32 location_ctrl;
+
+		switch (p->location) {
+		case V4L2_FWNODE_LOCATION_FRONT:
+			location_ctrl = V4L2_LOCATION_FRONT;
+			break;
+		case V4L2_FWNODE_LOCATION_BACK:
+			location_ctrl = V4L2_LOCATION_BACK;
+			break;
+		case V4L2_FWNODE_LOCATION_EXTERNAL:
+			location_ctrl = V4L2_LOCATION_EXTERNAL;
+			break;
+		default:
+			return -EINVAL;
+		}
+		if (!v4l2_ctrl_new_std(hdl, ctrl_ops,
+				       V4L2_CID_CAMERA_SENSOR_LOCATION,
+				       location_ctrl, location_ctrl, 1,
+				       location_ctrl))
+			return hdl->error;
+	}
+
+	if (p->rotation != V4L2_FWNODE_PROPERTY_UNSET) {
+		if (!v4l2_ctrl_new_std(hdl, ctrl_ops,
+				       V4L2_CID_CAMERA_SENSOR_ROTATION,
+				       p->rotation, p->rotation, 1,
+				       p->rotation))
+			return hdl->error;
+	}
+
+	return hdl->error;
+}
+EXPORT_SYMBOL(v4l2_ctrl_new_fwnode_properties);
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index cf59abafb0d9..409c800ab1f5 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -30,6 +30,7 @@ struct v4l2_ctrl;
 struct v4l2_ctrl_handler;
 struct v4l2_ctrl_helper;
 struct v4l2_fh;
+struct v4l2_fwnode_device_properties;
 struct v4l2_subdev;
 struct v4l2_subscribed_event;
 struct video_device;
@@ -1417,4 +1418,29 @@ int v4l2_ctrl_subdev_subscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
  */
 int v4l2_ctrl_subdev_log_status(struct v4l2_subdev *sd);
 
+/**
+ * v4l2_ctrl_new_fwnode_properties() - Register controls for the device
+ *				       properties
+ *
+ * @hdl: pointer to &struct v4l2_ctrl_handler to register controls on
+ * @ctrl_ops: pointer to &struct v4l2_ctrl_ops to register controls with
+ * @p: pointer to &struct v4l2_fwnode_device_properties
+ *
+ * This function registers controls associated to device properties, using the
+ * property values contained in @p parameter, if the property has been set to
+ * a value.
+ *
+ * Currently the following v4l2 controls are parsed and registered:
+ * - V4L2_CID_CAMERA_SENSOR_LOCATION;
+ * - V4L2_CID_CAMERA_SENSOR_ROTATION;
+ *
+ * Controls already registered by the caller with the @hdl control handler are
+ * not overwritten. Callers should register the controls they want to handle
+ * themselves before calling this function.
+ *
+ * Return: 0 on success, a negative error code on failure.
+ */
+int v4l2_ctrl_new_fwnode_properties(struct v4l2_ctrl_handler *hdl,
+				    const struct v4l2_ctrl_ops *ctrl_ops,
+				    const struct v4l2_fwnode_device_properties *p);
 #endif
-- 
2.26.1

[PATCH v9 10/11] media: i2c: ov5670: Parse and register properties

[Jacopo Mondi]

Parse device properties and register controls for them using the newly
introduced helpers.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov5670.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index 041fcbb4eebd..f118d44b0889 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -7,6 +7,7 @@
 #include <linux/pm_runtime.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
+#include <media/v4l2-fwnode.h>
 
 #define OV5670_REG_CHIP_ID		0x300a
 #define OV5670_CHIP_ID			0x005670
@@ -2059,6 +2060,8 @@ static const struct v4l2_ctrl_ops ov5670_ctrl_ops = {
 /* Initialize control handlers */
 static int ov5670_init_controls(struct ov5670 *ov5670)
 {
+	struct i2c_client *client = v4l2_get_subdevdata(&ov5670->sd);
+	struct v4l2_fwnode_device_properties props;
 	struct v4l2_ctrl_handler *ctrl_hdlr;
 	s64 vblank_max;
 	s64 vblank_def;
@@ -2129,6 +2132,15 @@ static int ov5670_init_controls(struct ov5670 *ov5670)
 		goto error;
 	}
 
+	ret = v4l2_fwnode_device_parse(&client->dev, &props);
+	if (ret)
+		return ret;
+
+	ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &ov5670_ctrl_ops,
+					      &props);
+	if (ret)
+		return ret;
+
 	ov5670->sd.ctrl_handler = ctrl_hdlr;
 
 	return 0;
-- 
2.26.1

[PATCH v9 11/11] media: i2c: ov13858: Parse and register properties

[Jacopo Mondi]

Parse device properties and register controls for them using the newly
introduced helpers.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov13858.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/media/i2c/ov13858.c b/drivers/media/i2c/ov13858.c
index aac6f77afa0f..2ef5fb5cf519 100644
--- a/drivers/media/i2c/ov13858.c
+++ b/drivers/media/i2c/ov13858.c
@@ -7,6 +7,7 @@
 #include <linux/pm_runtime.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
+#include <media/v4l2-fwnode.h>
 
 #define OV13858_REG_VALUE_08BIT		1
 #define OV13858_REG_VALUE_16BIT		2
@@ -1589,6 +1590,7 @@ static const struct v4l2_subdev_internal_ops ov13858_internal_ops = {
 static int ov13858_init_controls(struct ov13858 *ov13858)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&ov13858->sd);
+	struct v4l2_fwnode_device_properties props;
 	struct v4l2_ctrl_handler *ctrl_hdlr;
 	s64 exposure_max;
 	s64 vblank_def;
@@ -1666,6 +1668,15 @@ static int ov13858_init_controls(struct ov13858 *ov13858)
 		goto error;
 	}
 
+	ret = v4l2_fwnode_device_parse(&client->dev, &props);
+	if (ret)
+		return ret;
+
+	ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &ov13858_ctrl_ops,
+					      &props);
+	if (ret)
+		return ret;
+
 	ov13858->sd.ctrl_handler = ctrl_hdlr;
 
 	return 0;
-- 
2.26.1