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
