[libcamera-devel] Firmware (devicetree/ACPI interface) for marking camera sensors being on the front/back of a device

[Hans de Goede]

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:
>> On 1/16/22 23:20, Laurent Pinchart wrote:
>>> On Sun, Jan 16, 2022 at 10:43:25PM +0100, Hans de Goede wrote:
>>>> 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.

>>> 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 ?

>> 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