Hi Laurent,

On 1/17/22 16:35, Laurent Pinchart wrote:

Hi Hans,

(CC'ing Sakari)

On Mon, Jan 17, 2022 at 10:26:54AM +0100, Hans de Goede wrote:

quoted

On 1/16/22 23:20, Laurent Pinchart wrote:

quoted

On Sun, Jan 16, 2022 at 10:43:25PM +0100, Hans de Goede wrote:

quoted

Hi All,

IIRC there was some discussion about $subject a while ago,
esp. being pushed by the ChromeOS folks (IIRC). If you know what
I'm talking about, please add relevant folks to the Cc.

While doing some work on atomisp support I noticed that the
ACPI device fwnode-s describing the sensors have an ACPI _PLD
method, which is a standardized ACPI method to retreive an
package (ACPI for struct) describing the location of things
like USB ports; and in this case of the camera sensors.

And upon checking the Surface Go DSDT the sensors there seem to
have the _PLD bits to. And in both cases at least the following
PLD field (bits 67-69) seems to contain valid and relevant info,
quoting from the ACPI spec 6.2 version, page 329:

"""
Panel: Describes which panel surface of the system’s housing
the device connection point resides on:
0 – Top
1 – Bottom
2 – Left
3 – Right
4 – Front
5 – Back
6 – Unknown
"""

This seems to be consistently set to 4 or 5 for the _PLD method
of the sensor ACPI nodes which I checked.

So rather then defining a new devicetree property for this and
embedding that inside the ACPI tables, IMHO it would be best if
the ChromeOS devices would use the standardized _PLD ACPI method
for this too.

I have no specific objection to this, given that the _PLD is
standardized. In your experience, is the rotation also populated
correctly ? That's important information too.

That is a good question, so I just checked what the IPU3 does and
it uses a field in the SSDB ACPI package for this.

And I'm not sure that the _PLD is the right place for this, the _PLD
is about how the ACPI object appears to the user, so that
the operating-system can describe e.g. external connectors in
a dialog box using this info. E.g. the _PLD also contains information
about the color of the connector.

And an upside-down sensor, does not look upside-down to the user when
looking at it from the outside of the device. So based on this reading
of the spec I don't expect the rotation field to contain what we are
looking for (as is shown by the IPU3 driver using a SSDB field for this).

Your interpretation of the _PLD makes sense, but that's also the problem
with ACPI: there are many interpretation that make sense (at least to
the OEMs), leading to lack of standardization in practice :-S I suppose
I'm a bit concerned that the _PLD rotation, defined in the spec, would
be interpreted by some OEMs as being the right place to expose this
information, while others would use a different mechanism. I don't care
much about where the information ends up being stored, but I care about
avoiding proliferation of different solutions as much as possible.

The DSDTs / _PLDs which I have checked seem to all set the rotation
field to plain 0 / 0 degrees. But I have only checked atomisp2 / ipu3
based DSDTs. There are indeed no guarantees that some vendor
will not use the _PLD rotation, but this does seem somewhat unlikely,
the rotation field is described as: "Rotates the Shape" note the
captical S, so this seems to refer to the "Shape" field (Round / Oval /
Square / Rectangle) which gives the shape of the object as observed by
the user.

One think I do *not* want to see is all sensor drivers being poluted
with OEM-specific code because an OEM has decided to store data using a
custom representation.

Ack, ATM for the IPU3 the _PLD back/front parsing is done in the
bridge driver. If this becomes a common pattern we may need to offer
a helper for bridge drivers, or maybe even move it to the core.

One question here is if we should try to fix this at the ACPI level, to
avoid every vendor coming up with its own SSDB-like solution, or leave
it as-is. In the latter case, I wouldn't be surprised if some OEMs would
end up not populating the location correctly in the _PLD. 

Reading the ACPI spec a second time I'm convinced that the rotation
really does not belong in the _PLD. So to fix this at the ACPI level
would me doing some proposal there with some camera sensor specific
standardized ACPI fields/methods.

quoted

quoted

It we go in that direction, we should try to push OEMs to also populate
the vertical offset and horizontal offset fields, as I expect it to
become useful when multiple cameras are present in the same location.

That is a good point.

Note that my main reason for advertising using _PLD for the front/back
info is that that is what is already being used in Windows laptops
ACPI tables, so we need support for it regardless.

Is the information available in the SSDB too, or only in the _PLD ?

The front/back info is only available in the _PLD.

In
the latter case, what should we do for systems that don't populate the
_PLD correctly ?

Currently for unknown _PLD values the IPU3 driver sets
V4L2_FWNODE_ORIENTATION_EXTERNAL, which I guess is the same as not
setting any value at all ?

quoted

Actually your rotation question made me wonder what we are doing
for IPU3 here and we already have code parsing the _PLD in
drivers/media/pci/intel/ipu3/cio2-bridge.c to set
V4L2_FWNODE_ORIENTATION_FRONT / V4L2_FWNODE_ORIENTATION_BACK :)

Since at least atomisp2 is going to need this to we probably need to
factor this out into some shared helper.

I wonder if the atomisp2 driver will ever get out of staging. There's so
much work to be done there.

I would be more then happy if we can get it mostly functional, ever
getting it out of staging indeed is unlikely.

Regards,

Hans