Re: [PATCH v2 01/10] media: dt-bindings: Document 'location' property

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Jacopo,

On Mon, Sep 02, 2019 at 09:48:30PM +0200, Jacopo Mondi wrote:
> On Mon, Sep 02, 2019 at 07:49:37PM +0300, Laurent Pinchart wrote:
> > On Mon, Sep 02, 2019 at 06:40:31PM +0200, Jacopo Mondi wrote:
> >> On Mon, Sep 02, 2019 at 02:38:40PM +0100, Rob Herring wrote:
> >>> On Thu, Aug 29, 2019 at 02:46:40PM +0200, Jacopo Mondi wrote:
> >>>> On Tue, Aug 27, 2019 at 03:21:26PM +0300, Laurent Pinchart wrote:
> >>>>> On Tue, Aug 27, 2019 at 11:23:27AM +0200, Jacopo Mondi wrote:
> >>>>>> Add the 'location' device property, used to specify the camera device
> >>>>>> mounting position. The property is particularly meaningful for mobile
> >>>>>> devices with a well defined usage orientation.
> >>>>>>
> >>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>>>>> ---
> >>>>>>  .../devicetree/bindings/media/video-interfaces.txt     | 10 ++++++++++
> >>>>>>  1 file changed, 10 insertions(+)
> >>>>>>
> >>>>>> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> >>>>>> index f884ada0bffc..865f4142f432 100644
> >>>>>> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> >>>>>> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> >>>>>> @@ -89,6 +89,16 @@ Optional properties
> >>>>>>    but a number of degrees counter clockwise. Typical values are 0 and 180
> >>>>>>    (upside down).
> >>>>>>
> >>>>>> +- location: The camera sensor mounting location, expressed as a position
> >>>>>> +  relative to the usage orientation of the device the sensor is installed on.
> >>>>>
> >>>>> DT bindings being ABIs, we need to be precise and thorough there. One
> >>>>> particular point that bothers me is that the property is named location,
> >>>>> and its description refers to camera sensor mounting location.
> >>>>>
> >>>>> I see two options to fix this. One of them is to rename the property to
> >>>>> camera-location, but that would limit its future usage for other types
> >>>>> of devices. The other one is to document the property as applying to a
> >>>>> "device" instead of a "camera sensor", and add one sentence stating that
> >>>>> this property is valid for camera sensors only.
> >>>>>
> >>>>> This will require finding another name for the device that the device is
> >>>>> mounted on though, as using device twice would be very confusing.
> >>>>
> >>>> I had the same concern, but I cannot find another term to convey
> >>>> this... suggestions?
> >>>
> >>> For accelerometers and/or gyroscopes, we already have 'mount-matrix'.
> >>
> >> Pardon my ignorance, but I could not find it documented. Some binding
> >> files refers to an iio/mount-matrix.txt file which I cannot find. Has
> >> it been removed? Anyway, some individual bindings report examples of
> >> mount matrices (ie
> >> Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt)
> >>
> >>> That would replace 'rotation'. Maybe we can do something similar here?
> >>
> >> I think 'rotation' simply expressed as degrees is fine here, our
> >> problem was to find a term that would make it possible to specify that
> >> rotation/location are applied to a 'device' mounted on a 'device'.
> >> Sakari suggested on irc to use 'system' in place of the second
> >> 'device' and that seems to work well to me.
> >>
> >> Or were you suggesting to use a construct similar to mount-matrix for
> >> a purpose I didn't get :) ?
> >
> > mount-matrix would allow exposing the rotation in a more generic way,
> > and if there are standard DT bindings, I think this would make sense,
> > especially given that we could have device with camera sensors not
> > mounted parallel to the device side.
> 
> After reading this entry you pointed me to
> https://lore.kernel.org/patchwork/patch/1044786/
> I understand why Rob suggested that, but it seems a bit far fetched to
> me... The devices that could make use of 'mount-matrix', such as the
> ones IIO deal with, sample data in a 3D space, and to compensate their
> mounting displacement a rotation matrix is required.
> 
> Do you see use cases for compensating mounting displacement in a 3-d
> space for a camera ? I'm not saying it's not possible, but seems -a
> bit- far fetched.

360° cameras come to mind, they can use multiple sensors mounted with
different angles.

> If we consider cameras as always mounted parallel to the device used
> to take picturea, the correction you could apply to the possible mounting
> displacements are just rotations along the origin (or the z axis you want
> to consider a 3 dimensional space) and requiring to supply a 2-dimensions
> rotation matrix (with only a few combinations of 0, 1 and -1, if we only
> accept multiples of 90 degrees as we agreed) it seems a bit an overkill.
> 
> True, we could also handle image flipping, not just rotation with a rotation
> matrix but an image sensor mounted flipped upside down kind of defeat its
> purpose, doesn't it ? (it would point to the inside of the device :)
> 
> When we'll have devices with movable sensors like the one Pavel
> linked, rotation matrices could be used to report the current camera
> position maybe, but to represent a mounting displacement compensation,
> for devices working in a 2 dimensional space and with fixed rotations
> of 90 degrees multiples it seems an overkill to ask to developers to
> me.
> 
> That said, there are devices out there which do things I cannot even
> imagine, and maybe could actually compensate rotations in the 3D
> space, sample 3D point maps or other advanced things. Indeed they
> could use a rotation matrix if they need to, but the purpose of
> 'rotation' is much more humble and even if it could be represented
> with 'mount-matrix' it seems to me a non justifiable effort.

Most devices will only need to report a 0, 90, 180 or 270 rotation angle
along an axis perpendicular to the device. For those, mount-matrix is
indeed a bit overkill, and a rotation property would be enough. However,
when we will need to express more than those simple configurations,
mount-matrix will become needed, and all of a sudden we will have
multiple ways to express the same information. That's what I would like
to avoid by going for mount-matrix already.

-- 
Regards,

Laurent Pinchart