search

akobel / linux-thinkpad-x1-tablet

2 stars 0 forks source link

Camera support #1

Open akobel opened 2 years ago

akobel commented 2 years ago

Continued from https://github.com/linux-surface/linux-surface/issues/91, where similar work has been done for the camera of Surface Go 2; let's hope that migrating that effort to the X1 tablet is feasible...

(more precisely, coming over from this comment)

markum commented 2 years ago

Thanks for starting this. I assume I not need to create the DSDT for the same device and can cease my efforts go get it?

akobel commented 2 years ago

Correct, I think. But on Linux, getting them is fairly easy; it could be worth to double-check whether they are identical: acpidump -b from acpica and iasl -d *.dat to disassemble.

djrscally commented 2 years ago

Howdy.

OK, you guys have an annoying DSDT like the Surface Go line, where most of the settings are defined by values read through OpRegions instead of hardcoded. Unfortunately that means the DSDT is not readable, and the right values need to be discovered at runtime. Fortunately, one of the other devs already wrote something to let you do that.

Can you please build and load this module, and share the output (which will be written to dmesg)?

In terms of what will need to be done, it depends on the output of that module. If it says the PMICs are discrete type we just need to make the sensor drivers work. If they're the TPS68470 type we'll need to do some reverse engineering on Windows to sniff the right voltage settings for the PMIC to make sure we don't damage your devices. I can do the sensor drivers but you guys would have to test for me. The reverse engineering you'll need to do, but I can talk you through it if you need.

akobel commented 2 years ago

Thanks @djrscally already! Testing and some amount of debugging I can offer, but my expertise in this are of reverse engineering is pretty low; not sure about @markum.

For the dumps: sure thing, see https://github.com/akobel/linux-thinkpad-x1-tablet/tree/main/x1-tablet-gen2/intel_ipu_dump Note: this is on the Arch zen kernel (near-vanilla with only few tweaks for desktop usage); I can reproduce on linux-surface if necessary.

markum commented 2 years ago

Unfortunately I am not very qualified in this field. But I can ask a friend to help me.

djrscally commented 2 years ago

Hey. Thanks, so that confirms your PMIC is a physical TPS68470, so we do need to do some reverse engineering to get the right settings. This requires you to boot windows, does either of you have it set to dual boot by any chance?

On the sensor drivers front the OV2470 is well supported already and might only need a one line change to be useable. The OV8858 has an atomisp driver which would be adaptable with some effort.

On Sun, 27 Feb 2022, 23:46 markum, @.***> wrote:

Unfortunately I am not very qualified in this field. But I can ask a friend to help me.

— Reply to this email directly, view it on GitHub https://github.com/akobel/linux-thinkpad-x1-tablet/issues/1#issuecomment-1053726483, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABDBE2YWALHFSKP6JWQM3KLU5KZVPANCNFSM5PPEUDAQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

akobel commented 2 years ago

This requires you to boot windows, does either of you have it set to dual boot by any chance?

Yep, can do. (Note to myself: next time before you upgrade to Win11, make sure you have a backup to revert, not just because it's so horribly slow.)

On the sensor drivers front the OV2470 is well supported already and might only need a one line change to be useable. The OV8858 has an atomisp driver which would be adaptable with some effort.

Sounds great! I would have expected problems/significant effort for the 8858; if that could be made work, even better. For the time being, anything is an improvement, and it seems that you have the steps to initiate the cameras freshly in your head... :-)

djrscally commented 2 years ago

Yep, can do.

Excellent. How do you feel trying the reverse engineering steps then? It looks complicated but isn't really so bad - I can help get the DSDT entry written up and from there it's just getting the compiler installed and running a bunch of commands in the right order. Fair warning though: there's a very slight risk of messing the Windows install up a bit. I've never done it, but it is apparently a potential problem.

Sounds great! I would have expected problems/significant effort for the 8858; if that could be made work, even better. For the time being, anything is an improvement, and it seems that you have the steps to initiate the cameras freshly in your head... :-)

Yup still actively tackling the surface ones so we're kinda used to this now.

akobel commented 2 years ago

I'm struggling with the very first steps already, unfortunately.

First, on Linux, cause that's what I have currently booted: iasl is unable to process its own output. iasl dsdt.dsl (see here) gives the following output: iasl.log iasl *.dsl doesn't help, either.

I did not investigate the error itself yet, but will try to do so. Problem report on Windows will follow after a reboot.

akobel commented 2 years ago

Situation on Windows:

asl.exe /tab=DSDT produces an DSDT.ASL, but with the following error message (see asl-read-DSDT.log):

asl_ERR: UnAsmSuperName: invalid SuperName - 0x0a

asl.exe DSDT.ASL afterwards complains about some invalid names with funny characters (encoding issue?), but then seems to find an actual error: see asl-compile-DSDT.log...

djrscally commented 2 years ago

I'm struggling with the very first steps already, unfortunately.

First, on Linux, cause that's what I have currently booted: iasl is unable to process its own output. iasl dsdt.dsl (see here) gives the following output: iasl.log iasl *.dsl doesn't help, either.

I did not investigate the error itself yet, but will try to do so. Problem report on Windows will follow after a reboot.

I never understand how they manage to include a .dsl that won't compile. The windows tool is far worse than iasl at this; I actually make the edits in Linux, compile using iasl and then simply transfer the compiled .aml to windows to load.

Anyway; you have a ton of noise there, all that matters are the errors. It's complaining about this section:


    Name (SS1, 0x00)
    Name (SS2, 0x00)
    Name (SS3, One)
    One
    Name (SS4, One)
    One
    OperationRegion (GNVS, SystemMemory, 0xBFF4F000, 0x072C)
    Field (GNVS, AnyAcc, Lock, Preserve

Just CTRL-F for SS4 to find that bit. Delete those standalone "One" after SS3 and SS4, and subsequent to that it should compile (at least it is doing for me)

EDIT: My advice: stick to iasl for compiling the dsdt and just use asl on Windows to load it. The MS tool seems much worse than Intel's

akobel commented 2 years ago

Okay, that helps. Compilation works now, trying to progress from here. One question: the DSDT needs to be loaded on Windows; so I assume it's not persistently flashed to the device, but only temporary until next power cycle, right?

djrscally commented 2 years ago

Okay, that helps. Compilation works now, trying to progress from here. One question: the DSDT needs to be loaded on Windows; so I assume it's not persistently flashed to the device, but only temporary until next power cycle, right?

No that's not right; it's stored in the registry and lasts until it's replaced with a new version or you go and clear out the registry keys (which will make it restore to default). You need to increase the version number each time you load a new DSDT on Windows, or it will ignore the new file:

DefinitionBlock ("", "DSDT", 2, "LENOVO", "SKL     ", 0x00000000)

The last field in the definition block is the version number

akobel commented 2 years ago

Hehe, okay, makes sense. And that's soothing, cause...

IMG_20220302_121609

Will set me back a bit, but I'll recover. :)

djrscally commented 2 years ago

Ah-ha! Never mind - didn't want that OS anyway; here's recovery steps: https://github.com/bentiss/SimplePeripheralBusProbe#what-if-the-dsdt-is-wrong-and-i-get-the-blue-screen-acpi_bios_error-at-boot

akobel commented 2 years ago

Hm, the good news is that the recovery works like a charm. The bad news is that even without any modification, I get the same message. That is, just removing the two One lines, incrementing the version number, iasl dsdt.dsl and installing the patched aml via asl.exe /loadtable...

djrscally commented 2 years ago

Hm, the good news is that the recovery works like a charm. The bad news is that even without any modification, I get the same message. That is, just removing the two One lines, incrementing the version number, iasl dsdt.dsl and installing the patched aml via asl.exe /loadtable...

Ah; that is indeed suboptimal yeah. Maybe try the whole Windows asl compiling thing after all and see if that does a better job...

The alternative is scouring the internet for leaked schematics that will give us the connections and voltage settings instead.

akobel commented 2 years ago

Ah; that is indeed suboptimal yeah. Maybe try the whole Windows asl compiling thing after all and see if that does a better job...

Will do. But ...

The alternative is scouring the internet for leaked schematics that will give us the connections and voltage settings instead.

... what exactly are we after here? Just voltages, or something else, too? For the OV8858, I found a datasheet which might help? - From page 23:

A datasheet for the TPS68470 is also available here.

Unfortunately, no luck searching for OV2740 apart from a product brief yet, and this would likely be the most interesting one for a start...

djrscally commented 2 years ago

... what exactly are we after here? Just voltages, or something else, too? For the OV8858, I found a datasheet which might help? - From page 23:

* AVDD is 2.6~3.0V for sensor analog power (clean), 2.8V is recommended

* DOVDD is 1.7~3.0V for sensor digital IO power (clean). 1.8V is recommended

* DVDD should be 1.27V for internal regulator. If using external, prefer 1.11~1.30V for DVDD

A datasheet for the TPS68470 is also available here.

The datasheets don't help us here. The TPS68470 has 10 regulator lines, any 3 of which could be configured to supply anything from .875V to over 5V IIRC, and we have no idea which line supplies which voltage to which sensor input. Additionally there will almost certainly be some GPIO's that need to be toggled to send power on / reset signals to the sensor, and they probably come from the TPS68470 too - it has ten of those too so we need to figure out which drive which sensor.

So the datasheets are helpful (I have the TPS68470 one, the ov8858 one we'll need to get a driver running) but we need either the reverse engineering or schematics too.

Unfortunately, no luck searching for OV2740 apart from a product brief yet, and this would likely be the most interesting one for a start...

Don't need that one anyway as the driver that's already upstream looks fine to me, will need one or two very minor edits but nothing that we'd need the datasheet for.

akobel commented 2 years ago

First achievements: Loading an AML compiled with iasl version 20160527 worked better, apparently. Here is a precompiled binary distribution for Windows, too.

> ./I2cTestTool.exe -list
  FriendlyName DeviceId
          I2C2 \\?\ACPI#MSFT8000#1#{a11ee3c6-8421-4202-a3e7-b91ff90188e4}\I2C2

I'd assume I2C2 is the correct bus, since the DSDT has

    Scope (\_SB.PCI0.I2C2)
    {
        Device (PMIC)
        {
            Name (_ADR, Zero)  // _ADR: Address
            Name (_HID, "INT3472")  // _HID: Hardware ID
            Name (_CID, "INT3472")  // _CID: Compatible ID
            Name (_DDN, "PMIC-CRDG2")  // _DDN: DOS Device Name
            Name (_UID, "0")  // _UID: Unique ID
            Name (_PLD, Package (0x01)  // _PLD: Physical Location of Device

and this should be the camera power management module, right?

djrscally commented 2 years ago

Nice one, well done. That's the PMIC yes; the entry gives its I2C address further down:

            Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
            {
                Name (SBUF, ResourceTemplate ()
                {
                    I2cSerialBusV2 (0x004C, ControllerInitiated, 0x00061A80,
                        AddressingMode7Bit, "\\_SB.PCI0.I2C2",
                        0x00, ResourceConsumer, , Exclusive,
                        )
                })
                Return (SBUF) /* \_SB_.PCI0.I2C2.PMIC._CRS.SBUF */
            }

So you should be able to connect with I2CTestTool 0x4c I2C2, and then subsequently communicate by running writeread {ff} 1 which means "write the byte 0xff and then read 1 byte". The first byte you read sets the PMICs pointer to that address, 0xff being the register containing the Chip's ID. The output should say "21".

edit:

assuming the above works, best thing to do is simply dump out all the registers for the PMIC three times:

  1. On boot, with no cameras running
  2. With the front camera running
  3. With the back camera running

That combination will allow us to identify which bits are being set for each camera. The PMIC does a lot of stuff actually; the regulators of course but also provides the clock for the sensors, GPIO lines and at least on the Surface Go2 drives the privacy and IR LEDs too, so there's probably a lot to learn.

You can literally just do writeread {00} 256 and dump the whole chip in one go.

akobel commented 2 years ago

Strange. On 0x4c I only get "The transfer failed for an unknown reason". On 0x4d however, I can communicate, and I see reactions depending on whether the camera is connected, see below. Note: writeread doesn't work for me, but write followed by read does.

Dumps:

No camera:

00000000:  00  20  02  00  00  00  01  03    02  11  aa  20  01  d0  00  00
00000010:  05  00  00  00  01  08  01  08    01  08  8a  08  8a  08  c2  08
00000020:  c2  08  00  00  00  00  00  00    00  00  0a  00  00  00  00  00
00000030:  07  38  00  00  00  00  00  00    00  00  00  00  6d  13  34  34
00000040:  34  6d  0c  00  00  00  00  00    04  00  00  00  00  00  00  00
00000050:  00  00  00  00  00  00  00  00    00  00  00  00  00  00  00  00
*
000000f0:  00  00  00  00  00  00  00  00    00  00  00  00  00  00  00  21

Front camera:

00000000:  00  20 [d2] 00  00  00  01  03    02  11  aa  20  01 [d7] 00 [0a]
00000010:  05  00  00  00  01  08  01  08    01  08  8a  08  8a  08  c2  08
00000020:  c2  08  00  00  00  00  00 [70]  [c0] 00  0a  00  00  00  00  00
00000030:  07  38  00  00  00  00  00  00    00  00  00  00  6d  13  34  34
00000040:  34  6d  0c [03] 00 [01][01] 00    04  00  00  00  00  00  00  00
00000050:  00  00  00  00  00  00  00  00    00  00  00  00  00  00  00  00
*
000000f0:  00  00  00  00  00  00  00  00    00  00  00  00  00  00  00  21

Rear camera:

00000000:  00  20 [1f] 00  00  00  01  03    02  11  aa  20  01 [d7] 00 [0a]
00000010:  05  00  00  00  01  08  01  08    01  08  8a  08  8a  08  c2  08
00000020:  c2  08 [05] 00  00  00  00  00   [04] 00  0a  00  00  00  00  00
00000030:  07  38  00  00  00  00  00  00    00  00  00  00  6d  13  34  34
00000040:  34  6d  0c [03][01] 00  00 [01]  [05] 00  00  00  00  00  00  00
00000050:  00  00  00  00  00  00  00  00    00  00  00  00  00  00  00  00
*
000000f0:  00  00  00  00  00  00  00  00    00  00  00  00  00  00  00  21

Changes over camera off are marked with [ ]. (Lines with * are only zeros.)

djrscally commented 2 years ago

So front camera:

0x02, 0x07 and 0x09 are status and clock settings 0x27 is a status checker 0x28 set to c0 is a bit weird, it says ILED_B is the indicator LED line but also that it failed. That's inaccurate on my Go2, does the LED come on ok? 0x43 turns on the S_IO 0x45 and 0x46 turn on VAUX 1 and 2. Thsese are the voltage regulators driving that camera. 0x3d is the value for VAUX1 and looks like it's set to 1.213V and so on.

I have to stop now, but I'll decipher the rest over the weekend (unless you beat me to it) and I can make a patch adding support for the OV2740 camera for you to test.

djrscally commented 2 years ago

Sorry; I need the DMI vendor and product name for your device; can you give me sudo dmidecode -t1?

akobel commented 2 years ago

Sure, that'd be

Handle 0x000C, DMI type 1, 27 bytes
System Information
    Manufacturer: LENOVO
    Product Name: 20JCS00C00
    Version: ThinkPad X1 Tablet Gen 2
    Serial Number: XXXXXXXX
    UUID: XXXXXXXX
    Wake-up Type: Power Switch
    SKU Number: LENOVO_MT_20JC_BU_Think_FM_ThinkPad X1 Tablet Gen 2
    Family: ThinkPad X1 Tablet Gen 2

There's also a 20JB variant (identical concerning the camera AFAIK, but without LTE or with other minor differences), but I don't have one available to confirm for sure. I don't know if that would warrant filtering for version or family instead of product name, if this is even possible at this stage.

akobel commented 2 years ago

Also trying to follow, by the way. You got the meanings of the entries in the I2cTestTool dump from the register map in the TPS68470 datasheet, right? Starts to make sense to me.

0x28 set to c0 is a bit weird, it says ILED_B is the indicator LED line but also that it failed. That's inaccurate on my Go2, does the LED come on ok?

Yes, it does. Datasheet says bits 2 and 6 are set for ILED_A/B enabled (A with 16mA, B with 2mA with bits[5:4]=00), and bits 3 and 7 are unset for failure mode "open"; which apparently means "no failure"?

0x3d is the value for VAUX1 and looks like it's set to 1.213V and so on.

From what I can tell, voltages are

That'd be nearly identical to the Surface Go settings that are already in tps68470_board_data.c; I tried to add a near-verbatim copy for the X1 Tablet, but only got it to

int3472-tps68470 i2c-INT3472:05: INT3472 seems to have no dependents.

I hope I can free some more time tomorrow for a bit of investigation...

djrscally commented 2 years ago

Also trying to follow, by the way. You got the meanings of the entries in the I2cTestTool dump from the register map in the TPS68470 datasheet, right? Starts to make sense to me.

Exactly.

Yes, it does. Datasheet says bits 2 and 6 are set for ILED_A/B enabled (A with 16mA, B with 2mA with bits[5:4]=00), and bits 3 and 7 are unset for failure mode "open"; which apparently means "no failure"?

Yes, but 0xc0 is bits 7 and 6 set, so ILED_B enabled and failure mode shorted...which doesn't really fit the fact that it works fine. It's the same on my Go2

I hope I can free some more time tomorrow for a bit of investigation...

That's the driver for the PMIC. It's complaining that it doesn't find any ACPI devices (I.E. Linux's representation of the Device (LNK0) and Device (LNK1) from the DSDT table) declaring themselves as dependent on it in their _DEP entry, but according to your DSDT dump they both do...bit weird that one; acpi device creation should be finished by the time this driver probes so I'd expect them to be available. You're building a custom kernel are you? Have you configured that driver as built-in if so?

There might be an additional problem here in that the PMIC driver as written only expects a single sensor to be dependent on it, so it only fetches the first sensor and ignores any others. That won't work if the OV2470 turns out to be the second one. I patched that recently so it'll handle multiple consuming sensors but it's not upstream anywhere yet...if you get past the "no dependents" problem and later find the OV2470 fails probe for missing clock / GPIOs, this will be why.

markum commented 2 years ago

Sorry; I need the DMI vendor and product name for your device; can you give me sudo dmidecode -t1?

dmidecode 3.2

Getting SMBIOS data from sysfs. SMBIOS 3.0.0 present.

Handle 0x000B, DMI type 1, 27 bytes System Information Manufacturer: LENOVO Product Name: 20JBCTO1WW Version: ThinkPad X1 Tablet Gen 2 Serial Number: XXXXXXXXX UUID: XXXXXXXXXXXXXXX Wake-up Type: Power Switch SKU Number: LENOVO_MT_20JB_BU_Think_FM_ThinkPad Family: ThinkPad

markum commented 2 years ago

Here is the list of model variants of the thinkpad x1 tablet gen 2 which should all have the same camera model: https://psref.lenovo.com/Product/Think_Tablets/ThinkPad_X1_Tablet_2nd_Gen I am not sure, but I believe it is the same for gen 1: https://psref.lenovo.com/Product/Think_Tablets/ThinkPad_X1_Tablet

akobel commented 2 years ago

Yes, it does. Datasheet says bits 2 and 6 are set for ILED_A/B enabled (A with 16mA, B with 2mA with bits[5:4]=00), and bits 3 and 7 are unset for failure mode "open"; which apparently means "no failure"?

Yes, but 0xc0 is bits 7 and 6 set, so ILED_B enabled and failure mode shorted...which doesn't really fit the fact that it works fine. It's the same on my Go2

Ah, 0xc0 is not just bit 6, yeah... Right, seems fishy.

That's the driver for the PMIC. It's complaining that it doesn't find any ACPI devices (I.E. Linux's representation of the Device (LNK0) and Device (LNK1) from the DSDT table) declaring themselves as dependent on it in their _DEP entry, but according to your DSDT dump they both do...bit weird that one; acpi device creation should be finished by the time this driver probes so I'd expect them to be available. You're building a custom kernel are you? Have you configured that driver as built-in if so?

I'm using the linux-surface kernel for Arch for this tinkering, so I'm based on v5.16.11. All relevant (as far as I can tell) drivers are built as modules.

There might be an additional problem here in that the PMIC driver as written only expects a single sensor to be dependent on it, so it only fetches the first sensor and ignores any others. That won't work if the OV2470 turns out to be the second one. I patched that recently so it'll handle multiple consuming sensors but it's not upstream anywhere yet...if you get past the "no dependents" problem and later find the OV2470 fails probe for missing clock / GPIOs, this will be why.

Sounds plausible. I gave it a shot, but your patch doesn't cleanly apply over neither 5.16.11 nor 5.16.11 with the linux-surface patches applied (kind of expected). Is the baseline for this patch publicly available? (Hans' branch perhaps?)

djrscally commented 2 years ago

I'm pretty sure it's platform-drivers-x86/for-next that I based it on.

Not sure why else it might not be finding the acpi device for the sensor...I'll have a think on that one

On Sun, 6 Mar 2022, 12:27 Alexander Kobel, @.***> wrote:

Yes, it does. Datasheet says bits 2 and 6 are set for ILED_A/B enabled (A with 16mA, B with 2mA with bits[5:4]=00), and bits 3 and 7 are unset for failure mode "open"; which apparently means "no failure"?

Yes, but 0xc0 is bits 7 and 6 set, so ILED_B enabled and failure mode shorted...which doesn't really fit the fact that it works fine. It's the same on my Go2

Ah, 0xc0 is not just bit 6, yeah... Right, seems fishy.

That's the driver for the PMIC. It's complaining that it doesn't find any ACPI devices (I.E. Linux's representation of the Device (LNK0) and Device (LNK1) from the DSDT table) declaring themselves as dependent on it in their _DEP entry, but according to your DSDT dump https://github.com/akobel/linux-thinkpad-x1-tablet/blob/main/x1-tablet-gen2/intel_ipu_dump/result_20JCS00C00_LENOVO_MT_20JC_BU_Think_FM_ThinkPad_X1_Tablet_Gen_2%40v1.1.md they both do...bit weird that one; acpi device creation should be finished by the time this driver probes so I'd expect them to be available. You're building a custom kernel are you? Have you configured that driver as built-in if so?

I'm using the linux-surface kernel for Arch https://github.com/linux-surface/linux-surface/tree/master/pkg/arch/kernel for this tinkering, so I'm based on v5.16.11. All relevant (as far as I can tell) drivers are built as modules.

There might be an additional problem here in that the PMIC driver as written only expects a single sensor to be dependent on it, so it only fetches the first sensor and ignores any others. That won't work if the OV2470 turns out to be the second one. I patched that recently @.***/T/#m105a926d6b2fc0e6b7c3e0d9c8d5aa975beb0989> so it'll handle multiple consuming sensors but it's not upstream anywhere yet...if you get past the "no dependents" problem and later find the OV2470 fails probe for missing clock / GPIOs, this will be why.

Sounds plausible. I gave it a shot, but your patch doesn't cleanly apply over neither 5.16.11 nor 5.16.11 with the linux-surface patches applied (kind of expected). Is the baseline for this patch publicly available? (Hans' branch perhaps?)

— Reply to this email directly, view it on GitHub https://github.com/akobel/linux-thinkpad-x1-tablet/issues/1#issuecomment-1059953733, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABDBE25WQYQRWGJQTKNA3RLU6SQDDANCNFSM5PPEUDAQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

akobel commented 2 years ago

Okay, will try to go from there later in the evening or tomorrow. Thanks!

akobel commented 2 years ago

Okay, some new errors. I based on platform-drivers-x86/for-next and applied your patchset as well as this patch attempt by myself. Perhaps you can sanity-check this one?

Output is here: journal-1st.log

Things that look good:

kernel: int3472-tps68470 i2c-INT3472:05: TPS68470 REVID: 0x21
kernel: CORE: Bringing 900000uV into 1200000-1200000uV
kernel: ANA: Bringing 875000uV into 2815200-2815200uV
kernel: VCM: Bringing 875000uV into 2815200-2815200uV
kernel: AUX1: Bringing 875000uV into 2815200-2815200uV
kernel: AUX2: Bringing 875000uV into 1800600-1800600uV
kernel: ipu3-cio2 0000:00:14.3: Found supported sensor INT3474:01
kernel: ipu3-cio2 0000:00:14.3: Connected 1 cameras
kernel: ipu3-cio2 0000:00:14.3: enabling device (0000 -> 0002)
kernel: ipu3-cio2 0000:00:14.3: device 0x9d32 (rev: 0x1)

Things that don't look quite as good:

kernel: RIP: 0010:ov2740_read_reg.constprop.0+0x5b/0xf0 [ov2740]

and the related calltrace from ov2740_identify_module.

Things that I couldn't find out, among plenty others:

djrscally commented 2 years ago

Okay, some new errors. I based on platform-drivers-x86/for-next and applied your patchset as well as this patch attempt by myself. Perhaps you can sanity-check this one?

Good job! Faster than me, I was half way through rebasing. Let me take a look.

EDIT:

OK. The 2740 driver needs more work than I thought; it's currently relying on on ACPI power on / off methods, which isn't applicable in our case because of Intel's crappy ACPI table. We'll need to add a bunch of regulator_get()/regulator_enable() calls and ditto interfaces to the clock API

Output is here: journal-1st.log

Well you should never get an oops, so the driver that oopsed at least has a bug in it, and maybe some others. Scatter calls like this:

pr_info("%s(): %d\n", __func__, 0);

throughout ov2740_read_reg(). increasing the 0 each time - that way you can boot again and check where exactly the oops comes from. Can't actually see any const variables in there so not sure why it's happened.

What is device 0x9d32?

That's the PCI ID of the CIO2 device. The camera sensor pours data through an electrical bus following a spec known as the MIPI CSI-2 specification. The sensor is a CSI-2 transmitter, the CIO2 is a CSI-2 receiver. That device then loads the data into memory, and passes pointers to that memory to the image processing unit.

IIUC, TPS68470 has 7 GPIOs. How come that for Surface Go, GPIOs 7 and 9 seem to be addressed?

It has 10, but three are named. Section 8.3.4 of the datasheet:

The TPS68470 has three dedicated discrete signals (S_ENABLE, S_IDLE and S_RESETN) to support an Image sensor.

akobel commented 2 years ago

Output is here: journal-1st.log

Well you should never get an oops, so the driver that oopsed at least has a bug in it, and maybe some others. Scatter calls like this:

pr_info("%s(): %d\n", __func__, 0);

throughout ov2740_read_reg(). increasing the 0 each time - that way you can boot again and check where exactly the oops comes from. Can't actually see any const variables in there so not sure why it's happened.

Turns out that struct i2c_client *client = v4l2_get_subdevdata(&ov2740->sd); evaluates to 0 in the very first line. I guess the compiler is clever enough to notice that client is const in this function, that would explain the message. Perhaps a check against 0 would be in order; but I assume it's a rule that in this case the function shouldn't be called in the first place?

Too tired to go deeper in the stack today.

What is device 0x9d32?

That's the PCI ID of the CIO2 device. [...]

Ah, makes sense, the TPS68470 only is for power management.

IIUC, TPS68470 has 7 GPIOs. How come that for Surface Go, GPIOs 7 and 9 seem to be addressed?

It has 10, but three are named. Section 8.3.4 of the datasheet:

The TPS68470 has three dedicated discrete signals (S_ENABLE, S_IDLE and S_RESETN) to support an Image sensor.

Ah, thanks. So at least those are well-known, good.

djrscally commented 2 years ago

Might need a datasheet after all for the ov2740. The problem is that the driver as written expects ACPI to handle all the power on / off stuff, which comprises three things primarily:

  1. Clock (this is easy)
  2. Regulators - we need to know the voltage ranges each input expects
  3. GPIOs, need to know the polarity that these expect. Also, it looks like there's three GPIOs being toggled when the front camera is on - lines 4, 5 and 6. I've only seen 2 before so I want to confirm with the datasheet whether it even has 3 inputs. If not, possibly one of them does something else.

EDIT: The product brief you found shows there's three GPIOs actually, XSHUTDN, XSHUTDN2 and PWDNB. It also shows one of the regulators as having a typical input of 2.8V though which we don't have, so still ideally need to see the range really to make sure what the PMIC is configuring is valid...and also that the power-on sequence doesn't mandate any specific ordering.

Also, from your patch you have AUX1 set to 2815200 uV, I make it 1213200uV; which of us is wrong?

akobel commented 2 years ago

Might need a datasheet after all for the ov2740. The problem is that the driver as written expects ACPI to handle all the power on / off stuff [...]

Might be a tough one, but I'll try to find something. Google alone is not enough here, apparently.

EDIT: The product brief you found shows there's three GPIOs actually, XSHUTDN, XSHUTDN2 and PWDNB. It also shows one of the regulators as having a typical input of 2.8V though which we don't have, so still ideally need to see the range really to make sure what the PMIC is configuring is valid...and also that the power-on sequence doesn't mandate any specific ordering.

Concerning power-on sequence: I compared the OV2740 and OV8856 drivers to debug the null pointer issue, and realized that ov2740 = devm_kzalloc(&client->dev, sizeof(*ov2740), GFP_KERNEL); should probably go before ov2740_check_hwcfg in ov2740_probe. With that change, I get

ov2740 i2c-INT3474:01: failed to find sensor: -121
ov2740: probe of i2c-INT3474:01 failed with error -121

instead of oops. But OV8856 also has separate power on and power off functions called in probe - I guess something like that is missing?

Also, from your patch you have AUX1 set to 2815200 uV, I make it 1213200uV; which of us is wrong?

Probably me - I guess this was a leftover from copy-paste rather than serious consideration...

djrscally commented 2 years ago

With that change, I get

ov2740 i2c-INT3474:01: failed to find sensor: -121 ov2740: probe of i2c-INT3474:01 failed with error -121

instead of oops. But OV8856 also has separate power on and power off functions called in probe - I guess something like that is missing?

That change really shouldn't have fixed that problem, but whatever works lol. We do indeed need separate power on/off functions in probe (and ideally linked to pm_runtime), I'm just working on that now. I can make some guesses without the datasheet and see if it works, but no guarantees.

markum commented 2 years ago

I am sorry, that I kind of dropped out of this thread. If there is any way, I can support these efforts, please let me know. I am not so sophisticated in doing like "compile this", but if there are kind of more or less precise instructions, I can do. In general I am familiar with the command line and have understanding of Linux which I am using since 2003. I got the libcamera stuff working with my Surface Go.

djrscally commented 2 years ago

No worries. I've been working on a patch series that should hopefully sort the OV2740, should be able to share it this evening some time - if you could build and test that that would be really helpful, I can give instructions how to do it (it's pretty easy)

On Sun, 13 Mar 2022, 12:50 markum, @.***> wrote:

I am sorry, that I kind of dropped out of this thread. If there is any way, I can support these efforts, please let me know. I am not so sophisticated in doing like "compile this", but if there are kind of more or less precise instructions, I can do. In general I am familiar with the command line and have understanding of Linux which I am using since 2003. I got the libcamera stuff working with my Surface Go.

— Reply to this email directly, view it on GitHub https://github.com/akobel/linux-thinkpad-x1-tablet/issues/1#issuecomment-1066094580, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABDBE23J4TUCI7EM6EJXK53U7XQAJANCNFSM5PPEUDAQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

djrscally commented 2 years ago

OK, longer than hoped for, but this tree should work: https://github.com/djrscally/kernel/commits/thinkpad-x1

I will undoubtedly have screwed something up...but I can't test it so I don't know what that is yet :). Assuming it doesn't cause an oops at all, then if the driver doesn't probe my guess is it will be something to do with the power on sequence. In particular I'm unsure about the GPIO's; I'm toggling all three but it might be that we only need to do two of them...some experimentation might be needed

The lack of a 2.8V regulator line still bothers me actually because there really should be one according to the datasheet. AVDD is called out as being 2.6V to 3V and we don't have that according to the register dumps you got. It might be worth double checking the AUX1VAL and AUX2VAL registers on Windows again...but I think it would be safe to test anyway as we're setting a voltage under what the datasheet prescribes which ought to be safe.

akobel commented 2 years ago

Thanks @djrscally!

No dice for me with the first attempt of just compiling the latest additions as modules and plugging into my previous test kernel (NULL pointer dereference in skl_int3472_tps68470_probe). Will start with a clean slate and compile directly your tree now, after that I'll report again and try actual debugging.

markum commented 2 years ago

OK, longer than hoped for, but this tree should work: https://github.com/djrscally/kernel/commits/thinkpad-x1

I will undoubtedly have screwed something up...but I can't test it so I don't know what that is yet :). Assuming it doesn't cause an oops at all, then if the driver doesn't probe my guess is it will be something to do with the power on sequence. In particular I'm unsure about the GPIO's; I'm toggling all three but it might be that we only need to do two of them...some experimentation might be needed

The lack of a 2.8V regulator line still bothers me actually because there really should be one according to the datasheet. AVDD is called out as being 2.6V to 3V and we don't have that according to the register dumps you got. It might be worth double checking the AUX1VAL and AUX2VAL registers on Windows again...but I think it would be safe to test anyway as we're setting a voltage under what the datasheet prescribes which ought to be safe.

Sounds great. How would I proceed to compile this with an Ubuntubased system (20.04, KDE Neon)?

djrscally commented 2 years ago

OK, longer than hoped for, but this tree should work: https://github.com/djrscally/kernel/commits/thinkpad-x1 I will undoubtedly have screwed something up...but I can't test it so I don't know what that is yet :). Assuming it doesn't cause an oops at all, then if the driver doesn't probe my guess is it will be something to do with the power on sequence. In particular I'm unsure about the GPIO's; I'm toggling all three but it might be that we only need to do two of them...some experimentation might be needed The lack of a 2.8V regulator line still bothers me actually because there really should be one according to the datasheet. AVDD is called out as being 2.6V to 3V and we don't have that according to the register dumps you got. It might be worth double checking the AUX1VAL and AUX2VAL registers on Windows again...but I think it would be safe to test anyway as we're setting a voltage under what the datasheet prescribes which ought to be safe.

Sounds great. How would I proceed to compile this with an Ubuntubased system (20.04, KDE Neon)?

You should:

  1. Install all the things (compilers and whatnot): sudo apt-get install build-essential libncurses-dev bison flex libssl-dev libelf-dev
  2. Git clone that repo and check out thinkpad-x1
  3. cd into the repo
  4. Copy surface-5.16.config into the repo and call it .config (note the period)
  5. Run make menuconfig and make sure that the ov2740, cio2-bridge and anything called TPS68470 are enabled (use / to search)
  6. Run make -j<number of cores> bindeb-pkg. Replace with the number of threads your CPU can run.
  7. Make some tea...
  8. Run sudo dpkg -i ../linux*.deb

That basically builds the kernel into .deb files and installs them using Ubuntu's package infrastructure. There might be an easier way to do the installation to be honest, but this is what I always do (mostly as I compile on my PC and just scp the .debs to the laptops so it doesn't take so long).

let me know if something didn't make sense there

akobel commented 2 years ago

First sanity check: This should change absolutely nothing for you until you change 20JCS00C00 to 20JBCTO1WW in drivers/platform/x86/intel/int3472/tps68470_board_data.c line 241.

djrscally commented 2 years ago

Just a thought; you'll need to patch libcamera a bit to enable support for the ov2740; this series should do it: https://lists.libcamera.org/pipermail/libcamera-devel/2022-March/029265.html (I posted to the libcamera mailing list so hopefully make it upstream there soon)

akobel commented 2 years ago

Huh. Debugged a little bit: the NULL pointer is not in skl_int3472_tps68470_probe, but in int3472_tps68470_get_board_data: board_data is NULL in the second iteration. If I replace the while condition by match && match->driver_data, no faults, but:

int3472-tps68470 i2c-INT3472:05: No board-data found for this laptop/tablet model

Q: What's the reason that for this model, there seems to be no CORE/ANA settings? IIUC, at this point, we need to try and supply the camera with power to make it even detectable, then the power up sequence comes later in ov2740's probe, correct?

djrscally commented 2 years ago

Huh. Debugged a little bit: the NULL pointer is not in skl_int3472_tps68470_probe, but in int3472_tps68470_get_board_data: board_data is NULL in the second iteration. If I replace the while condition by match && match->driver_data, no faults, but:

int3472-tps68470 i2c-INT3472:05: No board-data found for this laptop/tablet model

huh ... what's the output of ls /sys/bus/i2c/devices ?

Q: What's the reason that for this model, there seems to be no CORE/ANA settings? IIUC, at this point, we need to try and supply the camera with power to make it even detectable, then the power up sequence comes later in ov2740's probe, correct?

The CORE/ANA supplies are powering the other camera, so I didn't bother including them yet since we don't have a driver for it anyway.

akobel commented 2 years ago

huh ... what's the output of ls /sys/bus/i2c/devices ?

% ls -l /sys/bus/i2c/devices                 
total 0
lrwxrwxrwx 1 root root 0 Mar 16 23:53 i2c-0 -> ../../../devices/pci0000:00/0000:00:02.0/i2c-0
lrwxrwxrwx 1 root root 0 Mar 16 23:53 i2c-1 -> ../../../devices/pci0000:00/0000:00:02.0/i2c-1
lrwxrwxrwx 1 root root 0 Mar 16 23:53 i2c-2 -> ../../../devices/pci0000:00/0000:00:02.0/i2c-2
lrwxrwxrwx 1 root root 0 Mar 16 23:53 i2c-3 -> ../../../devices/pci0000:00/0000:00:02.0/drm/card0/card0-eDP-1/i2c-3
lrwxrwxrwx 1 root root 0 Mar 16 23:53 i2c-4 -> ../../../devices/pci0000:00/0000:00:02.0/drm/card0/card0-DP-1/i2c-4
lrwxrwxrwx 1 root root 0 Mar 16 23:53 i2c-5 -> ../../../devices/pci0000:00/0000:00:02.0/drm/card0/card0-DP-2/i2c-5
lrwxrwxrwx 1 root root 0 Mar 16 23:54 i2c-6 -> ../../../devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-6
lrwxrwxrwx 1 root root 0 Mar 16 23:54 i2c-7 -> ../../../devices/pci0000:00/0000:00:15.2/i2c_designware.1/i2c-7
lrwxrwxrwx 1 root root 0 Mar 16 23:54 i2c-8 -> ../../../devices/pci0000:00/0000:00:15.3/i2c_designware.2/i2c-8
lrwxrwxrwx 1 root root 0 Mar 16 23:53 i2c-9 -> ../../../devices/pci0000:00/0000:00:1f.4/i2c-9
lrwxrwxrwx 1 root root 0 Mar 16 23:53 i2c-INT3472:05 -> ../../../devices/pci0000:00/0000:00:15.2/i2c_designware.1/i2c-7/i2c-INT3472:05
lrwxrwxrwx 1 root root 0 Mar 16 23:54 i2c-WCOM5116:00 -> ../../../devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-6/i2c-WCOM5116:00

And, FWIW,

% grep -H . /sys/**/INT347*:*/path  
/sys/bus/acpi/devices/INT3470:00/path:\_SB_.SKC0
/sys/bus/acpi/devices/INT3471:00/path:\_SB_.PCI0.I2C2.CAM0
/sys/bus/acpi/devices/INT3472:00/path:\_SB_.PCI0.I2C2.PMIC
/sys/bus/acpi/devices/INT3472:01/path:\_SB_.PCI0.DSC0
/sys/bus/acpi/devices/INT3472:02/path:\_SB_.PCI0.DSC1
/sys/bus/acpi/devices/INT3472:03/path:\_SB_.PCI0.DSC2
/sys/bus/acpi/devices/INT3472:04/path:\_SB_.PCI0.DSC3
/sys/bus/acpi/devices/INT3472:05/path:\_SB_.PCI0.CLP0
/sys/bus/acpi/devices/INT3472:06/path:\_SB_.PCI0.CLP1
/sys/bus/acpi/devices/INT3472:07/path:\_SB_.PCI0.CLP2
/sys/bus/acpi/devices/INT3472:08/path:\_SB_.PCI0.CLP3
/sys/bus/acpi/devices/INT3474:00/path:\_SB_.PCI0.I2C4.CAM1
/sys/bus/acpi/devices/INT3474:01/path:\_SB_.PCI0.LNK2
/sys/bus/acpi/devices/INT3477:00/path:\_SB_.PCI0.LNK0
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/INT3470:00/path:\_SB_.SKC0
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:7b/INT3471:00/path:\_SB_.PCI0.I2C2.CAM0
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:7b/INT3472:00/path:\_SB_.PCI0.I2C2.PMIC
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT3446:00/INT3474:00/path:\_SB_.PCI0.I2C4.CAM1
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT3472:01/path:\_SB_.PCI0.DSC0
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT3472:02/path:\_SB_.PCI0.DSC1
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT3472:03/path:\_SB_.PCI0.DSC2
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT3472:04/path:\_SB_.PCI0.DSC3
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT3472:05/path:\_SB_.PCI0.CLP0
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT3472:06/path:\_SB_.PCI0.CLP1
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT3472:07/path:\_SB_.PCI0.CLP2
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT3472:08/path:\_SB_.PCI0.CLP3
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT3474:01/path:\_SB_.PCI0.LNK2
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/INT3477:00/path:\_SB_.PCI0.LNK0

The CORE/ANA supplies are powering the other camera, so I didn't bother including them yet since we don't have a driver for it anyway.

Ah, makes sense. Thanks.

djrscally commented 2 years ago

Is it just me or am I not seeing the i2c-INT3474:01 entry for the camera there?