Hi All, On 30-Jan-25 4:34 PM, Hans de Goede wrote: > Hi Sakari, > > On 27-Jan-25 8:48 AM, sakari.ailus@xxxxxxxxxxxxxxx wrote: >> Hi Hans, >> >> Thanks for your e-mail. >> >> On Wed, Jan 22, 2025 at 06:19:47PM +0100, Hans de Goede wrote: >>> Hi All, >>> >>> Background / analysis: >>> ---------------------- >>> >>> New Intel Meteor Lake based laptops with IPU6 cameras have a new type 0x12 >>> pin defined in the INT3472 sensor companion device which describes >>> the sensor's GPIOs. >>> >>> Based on what I know about this now, this pin seems to be used in at least >>> 3 different scenarios: >>> >>> 1. To power-up / activate some unknown auxiliary IC on HP laptop designs >>> where the sensor is directly connected to the main Meteor Lake SoC for >>> I2C and GPIOs (no USB io-expander used). Example dyndbg int3472 output: >>> >>> HP Spectre x360 2-in-1 Laptop 16t-aa000/8C17, BIOS F.11 08/14/2024 (ov08x40): >>> [ 4.908016] int3472-discrete INT3472:01: unknown \_SB.GPI0 pin 65 active-high >>> [ 4.908019] int3472-discrete INT3472:01: GPIO type 0x12 unknown; the sensor may not work >>> [ 4.908100] int3472-discrete INT3472:01: privacy-led \_SB.GPI0 pin 107 active-high >>> (and lsusb shows no Lattice NX## / Synaptics Sabre USB-io expander) >>> >>> 2. To make the Lattice chip in designs with the Lattice chip is used run >>> the power-on sequence of the sensor which is handled by the Lattice chip >>> firmware itself running the entire power-on/-down sequence when changing >>> the GPIO value. Example dyndbg int3472 output: >>> >>> Dell Latitude 7450 (with patch to map handshake in INT3472) (ov02e10): >>> [ 5.110920] int3472-discrete INT3472:01: Sensor name OVTI02E1:00 >>> [ 5.111908] int3472-discrete INT3472:01: Sensor module id: 'CIFNE24' >>> [ 5.113653] int3472-discrete INT3472:01: handshake \_SB.PC00.XHCI.RHUB.HS05.VGPO pin 0 active-high >>> [ 5.113676] int3472-discrete INT3472:01: privacy-led \_SB.PC00.XHCI.RHUB.HS05.VGPO pin 2 active-high >>> (with 2ac1:20c9 Lattice NX33 USB IO-expander) >> >> The good thing is that there's a datasheet >> <URL:https://mm.digikey.com/Volume0/opasdata/d220001/medias/docus/5726/FPGA-DS-02104-0-92-CrossLink-NX-33-and-CrossLink-NX-33U.pdf> >> so we don't need to guess. >> >> Lattice NX-33 is more than an USB IO expenader. Most likely it acts as a >> CSI-2 RX and CSI-2 TX and does something in between, like IVSC. It should >> have its own driver, even if all it does is toggle GPIOs and sleep a >> little. > > I guess that what you mean is having something similar to: > > drivers/media/pci/intel/ivsc/mei_csi.c > > which just toggles the handshake GPIO on the s_stream() callback ? > > That would require injecting the Lattice NX33 into the media fwnode > graph similar to how we are doing this for the ivsc. But where > the ivsc triggers this on there being an ACPI fwnode, there does > not appear to be any ACPI fwnode for the NX33 at least not > on Alan's laptop where we know the handshake signal is necessary. > > https://github.com/intel/vision-drivers > > suggests that the new chip shows up as an i2c device when it > actually is mitm-ing the sensor and here is "ls /sys/bus/i2c/devices" > output for Alan's laptop with the usbio-i2c driver loaded: > > $ ls -l /sys/bus/i2c/devices > total 0 > lrwxrwxrwx. 1 root root 0 Jan 6 08:46 i2c-0 -> ../../../devices/pci0000:00/0000:00:02.0/i2c-0/ > lrwxrwxrwx. 1 root root 0 Jan 6 08:46 i2c-1 -> ../../../devices/pci0000:00/0000:00:02.0/i2c-1/ > lrwxrwxrwx. 1 root root 0 Jan 6 08:46 i2c-10 -> ../../../devices/pci0000:00/0000:00:02.0/drm/card0/card0-DP-1/i2c-10/ > lrwxrwxrwx. 1 root root 0 Jan 6 08:46 i2c-11 -> ../../../devices/pci0000:00/0000:00:02.0/drm/card0/card0-DP-2/i2c-11/ > lrwxrwxrwx. 1 root root 0 Jan 6 08:46 i2c-12 -> ../../../devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-12/ > lrwxrwxrwx. 1 root root 0 Jan 6 08:46 i2c-13 -> ../../../devices/pci0000:00/0000:00:15.3/i2c_designware.1/i2c-13/ > lrwxrwxrwx. 1 root root 0 Jan 6 08:46 i2c-14 -> ../../../devices/pci0000:00/0000:00:1f.4/i2c-14/ > lrwxrwxrwx. 1 root root 0 Jan 6 08:46 i2c-15 -> ../../../devices/pci0000:00/0000:00:14.0/usb3/3-5/3-5:1.0/usbio-i2c.2.auto/i2c-15/ > lrwxrwxrwx. 1 root root 0 Jan 6 08:46 i2c-16 -> ../../../devices/pci0000:00/0000:00:14.0/usb3/3-5/3-5:1.0/usbio-i2c.3.auto/i2c-16/ > lrwxrwxrwx. 1 root root 0 Jan 6 08:46 i2c-2 -> ../../../devices/pci0000:00/0000:00:02.0/i2c-2/ > lrwxrwxrwx. 1 root root 0 Jan 6 08:46 i2c-3 -> ../../../devices/pci0000:00/0000:00:02.0/i2c-3/ > lrwxrwxrwx. 1 root root 0 Jan 6 08:46 i2c-4 -> ../../../devices/pci0000:00/0000:00:02.0/i2c-4/ > lrwxrwxrwx. 1 root root 0 Jan 6 08:46 i2c-5 -> ../../../devices/pci0000:00/0000:00:02.0/i2c-5/ > lrwxrwxrwx. 1 root root 0 Jan 6 08:46 i2c-6 -> ../../../devices/pci0000:00/0000:00:02.0/i2c-6/ > lrwxrwxrwx. 1 root root 0 Jan 6 08:46 i2c-7 -> ../../../devices/pci0000:00/0000:00:02.0/i2c-7/ > lrwxrwxrwx. 1 root root 0 Jan 6 08:46 i2c-8 -> ../../../devices/pci0000:00/0000:00:02.0/i2c-8/ > lrwxrwxrwx. 1 root root 0 Jan 6 08:46 i2c-9 -> ../../../devices/pci0000:00/0000:00:02.0/drm/card0/card0-eDP-1/i2c-9/ > lrwxrwxrwx. 1 root root 0 Jan 6 08:46 i2c-HIMX1092:00 -> ../../../devices/pci0000:00/0000:00:14.0/usb3/3-5/3-5:1.0/usbio-i2c.2.auto/i2c-15/i2c-HIMX1092:00/ > lrwxrwxrwx. 1 root root 0 Jan 6 08:46 i2c-OVTI02E1:00 -> ../../../devices/pci0000:00/0000:00:14.0/usb3/3-5/3-5:1.0/usbio-i2c.3.auto/i2c-16/i2c-OVTI02E1:00/ > lrwxrwxrwx. 1 root root 0 Jan 6 08:46 i2c-VEN_0488:00 -> ../../../devices/pci0000:00/0000:00:15.3/i2c_designware.1/i2c-13/i2c-VEN_0488:00/ > > Notice there is no i2c device matching the devices expected by > the vision-drivers. > > I have heard that the X1 carbon gen 13 (lunar lake) does have > such an i2c device. > > So I think that the CSI pass-through functionality is not used > on Alan's laptop. > > Also the ivcs needed to be configured for number of CSI lanes + > CSI frequency. That is not the case here. It is a fully opaque > black box and Windows seems to treat it mostly as if it is > not there at all. > > I believe that it would be best to do the same as Windows and > just make sure we toggle the handshake pin and otherwise forget > about the chip being there. In my many years if experience with > x86 hw primarily designed for Windows trying to behave differently > from Windows only causes trouble. > > If we were to try and model this as a separate v4l2-subdev > in the media graph then then the questions become: > > 1. When to inject such a node. The ivsc model of going by > ACPI HID's very likely does not work (or maybe it does and > we simply have not seen hw yet which actually uses the vision > parts? That is what the vision-drivers git repo suggests). > > Going by the presence of a handshake pin in the INT3472 device > is riddled with problems: > > a. It requires digging into the ACPI resources of a device > not owned by the ipu-bridge code. > > b. Given the HP laptop example, likely also Alan's laptop > the presence of a handshake pin on meteor lake laptops does > not seem to guarantee the CSI signals are actually MITM-ed > by a chip, so we will likely create false-positives. > > c. Some previous (alder/raptor lake) HP laptops advertise > a handshake pin but already work since they seem to not use it, > so more false-postive matches. > > 2. Without an ACPI hid we have no struct-device/fwnode > to use as parent for the extra v4l2-subdev, so what do we > use for this ? > > 3. What about models where the fwnode-s for the media graph > come from ACPI MIPI discovery? We skip the ipu-bridge code > there ... > > Combining these issues with the fact that the interface > is now just a single GPIO of which we do not exactly know > what it does, where as with the ivsc we actually needed > to program a CSI2 lane-count + link-freq I do not believe > that modelling this unknown hw controlled by the handshake > pin as an extra node on the media graph is the right thing > to do. > > ATM this feels much more like some power-sequencing thing, > just one more regulator / clk / reset-pin which we need > to set during power-up / -down. > > If in the future we get models where the ACPI HIDs claimed by: > https://github.com/intel/vision-drivers/blob/main/drivers/misc/icvs/intel_cvs.c#L720 > actually show up then we can revisit this. It might make > sense to treat things like the ivsc setup in that case and > then if those too use the type 0x12 GPIO on the INT3472 > device we can use the same code as used by the ipu-bridge > to create the extra media graph nodes to opt out of mapping > type 0x12 to a vdd supply. > > Another solution would be to move even closer to what Windows > does and switch the driver for the INT3472 ACPI device from > mapping pins to gpio-lookups / clks / regulators to it controlling > all the GPIOs listed in the INT3472 _DSM directly from runtime > suspend/resume functions, using a runtime pm device link from > the sensor to ensure that the INT3472 device is suspended / resumed > at the right time. > > This would also solve all of the mapping issues discussed in relation > to the ov7251 driver expecting an "enable" pin. I believe that this > would be the closest mirror of what Windows actually does and thus > is what will work without issues / quirks on the most laptop models. > >> Any thoughts, Bingbu? >> >>> >>> 3. For unknown reason (ACPI table bug? / not actually used) there is >>> a handshake type pin in the INT3472 device on some older HP models with >>> a hi556 sensor connected directly to the Alder Lake or Raptor Lake SoC, >>> see e.g. the dmesg from: https://linux-hardware.org/?probe=a9a2e2ab03 : >>> >>> [ 9.077107] int3472-discrete INT3472:01: reset \_SB.GPI0 pin number mismatch _DSM 141 resource 301 >>> [ 9.077170] int3472-discrete INT3472:01: power-enable \_SB.GPI0 pin number mismatch _DSM 142 resource 302 >>> [ 9.086727] int3472-discrete INT3472:01: GPIO type 0x12 unknown; the sensor may not work >>> >>> which is from a model where it has been confirmed that the FOSS stack >>> works already even though we have no handshake GPIO support yet. >>> >>> Testing has shown that for things to work in scenario 1. and 2. the 0x12 / >>> handshake type pin needs to be driven high, followed by a msleep of 25 ms. >>> >>> The 25 ms was taken from the out of tree drivers which specify this as >>> minimum sleep for the Lattice case. For the HP without Lattice, the default >>> 1 ms post reset delay also is not enough see: >>> https://lore.kernel.org/linux-media/1664997903.107383327.1734874558698.JavaMail.zimbra@xxxxxxxxxx/ >> >> Maybe that 25 ms is required for booting whatever runs on NX-33? > > Yes the comments in the out of tree driver refer to this being > related to the Lattice fw. > >>> This applies to both the HP Spectre x360 16t without Lattice chip and >>> the Dell Latitude 7450 with Lattice chip. >>> >>> I suspect that there might be some micro-controller or something running >>> the power-on sequence in the HP Spectre x360 16t case too, but there it >>> is just a standalone chip responding to the GPIO, not an USB attached chip >>> also offering, e.g. an I2C + GPIO controller like the Lattice chip. >> >> So there's nothing unusual there in terms of USB devices there? >> >> Would you be able to dump the ACPI tables from the HP machine? (I don't >> mind Dell either.) > > I have attached an acpidump from a HP Spectre x360 14-eu0xxx where > the handshake pin appears to be necessary but there is no lattice chip here: > > https://bugzilla.redhat.com/show_bug.cgi?id=2333331 > > Alan can you send Sakari an acpidump of your laptop ? > > <snip> > >> I'll need to dig a bit further, hoping to find out what that special GPIO >> 0x12 is. > > Between this and the pin mapping discussion before I'm more and more > convinced that we are going all wrong about this. > > The INT3472 device seems to purely be a power-sequencing device which > I believe has its own driver separate from the sensor drivers under > Windows and the ACPI _DEP link ensures that it gets powered-up before > the sensor device does. > > We really should use this fw API as intended and simply have > the INT3472 driver directly control the GPIOs from its own runtime > suspend/resume methods and use a runtime-pm link to ensure > the INT3472 device gets runtime-resumed before the sensor device > gets runtime-resumed. > > This requires basically rewriting: > drivers/platform/x86/intel/int3472/discrete.c > but I believe that the end result will be cleaner / smaller and this > should solve most problems surrounding this code once and for all > avoiding us to have to revisit things all the time to work around > the mismatch in expectations between the ACPI tables on Windows > laptops and the devicetree-like setup the sensor drivers expect. One remark about my idea to turn the INT3472 discrete driver into a power-sequencing driver and have it control all the GPIOs + timing itself. Using runtime suspend/resume with runtime-pm device links will not work since the i2c_client does not exist yet when the INT3472 driver binds and adding the links later is troublesome since as soon as the i2c_client is there the sensor driver my bind and that may want to power-up the sensor during probe(). So instead I think we should go with Sakari's suggestion from: https://lore.kernel.org/platform-driver-x86/Z49V5CqEt6H96LvJ@kekkonen.localdomain/ to use a virtual GPIO controller offering a "reset" or "enable" con_id GPIO to the sensor and then do the power-sequencing based on that GPIO being set low/high by the sensor driver. Regards, Hans
