cameras: ov5693: make the IPU6 Surface front cameras stream (Pro 8/9, Go 4) - #2171
cameras: ov5693: make the IPU6 Surface front cameras stream (Pro 8/9, Go 4)#2171naeemarsalan wants to merge 2 commits into
Conversation
…on IPU6 Add a commit to the cameras patchset that writes MIPI_CTRL00 (0x4800)=0x2d before stream-on in the ov5693 driver. Without it the Surface Pro 8 front camera (ov5693 on IPU6) powers up and 'streams' but the IPU6 D-PHY receives no CSI-2 data and capture times out. The same driver/sensor works on the IPU3 Surface devices with the 0x00 default, so the write is currently unconditional and wants checking on those before being relied upon there. Reverse-engineered from the SP8 Windows ov5693.sys; verified on a Surface Pro 8 (kernel 6.19). See the patch commit message for details. Link: linux-surface#1893 Signed-off-by: Arsalan Naeem <naeemarsalan@gmail.com>
|
For anyone who wants to apply this on a Surface Pro 8 before it lands here, I wrote up a step-by-step guide (build + MOK-sign the patched 📄 https://gist.github.com/naeemarsalan/3194a838a7bfb66671bc90d6de6734dd Same honesty caveats as the PR: reverse-engineered from the Windows driver, verified only on SP8/IPU6 (kernel 6.19.8), not tested on the IPU3 Surface devices that share this driver. Feedback very welcome. |
|
Thanks for working on this patch! I tested it on my Microsoft Surface Go 4, and it looks like the front OV5693 camera now streams successfully here as well. My environment:
Before applying the patch, the camera was detected, but capture did not actually start receiving frames: dmesg showed: After applying the Then I still saw a few CSI warnings in dmesg: But unlike before, capture no longer hangs at stream start, and frames are received at around 28.6 fps. So from my test, this patch also seems to fix the front OV5693 streaming issue on the Surface Go 4. Thanks again! |
|
Here are the full logs:
Full logs
Full logsNote: the “failed to open file ~/...” messages seem to be caused by my output path using |
|
Also confirmed working on the Surface Pro 9 (IPU6), but with one difference: my Pro 9 enumerates the sensor as OVTI5693, not INT33BE. Mainline ov5693.c only matches INT33BE, so on the Pro 9 the driver doesn't bind at all until that HID is added, so the MIPI_CTRL00 write alone isn't enough here. (The Go 4 logs above from @Fugu0141 show it binding as i2c-INT33BE:00, which is why the register write was sufficient there.) Two ways to handle the Pro 9: add {"OVTI5693"} to ov5693_acpi_match[], or a modprobe alias (alias acpi*:OVTI5693:* ov5693). Might be worth folding the HID into this PR so IPU6 Surfaces that use the OVTI5693 ID work end-to-end. Happy to send a patch for that part. |
|
@naeemarsalan, since the OVTI5693 HID is separate from your MIPI_CTRL00 change, how would you like to handle it? I could add {"OVTI5693"} to your patch here, so IPU6 Surfaces that use that HID (Pro 9) work end-to-end in one go, or send it as a follow-up patch on top of yours, crediting this PR. Either's fine by me, I don't want to step on your work. I've got it tested and ready on a Pro 9 (kernel 6.19). Lmk which you'd prefer. |
|
@femito1 Hey! Happy you got it working, yea can do what ever you like! Which ever is easier, I'd be happy to add it! Thanks |
|
Awesome, thanks! Easiest for right now is probably to just fold the HID into your patch here so Pro 9 users get it in the linux-surface kernel. It's a one-liner in the That's what the Pro 9 enumerates as (OVTI5693 instead of INT33BE), so with this + your MIPI_CTRL00 write the front camera binds and streams end-to-end on IPU6. Tested on Pro 9, kernel 6.19. Separately I'm going to try sending the HID bits upstream to linux-media (it also needs a matching entry in ipu-bridge.c's supported-sensors list to enumerate on a stock kernel). I'll keep your MIPI_CTRL00 work credited, happy to coordinate if you want to send that part too. Thanks for putting this together! |
…patch The MIPI_CTRL00=0x2d fix has been confirmed on the Surface Go 4 (ov5693 at INT33BE, by Fugu0141) and the Surface Pro 9 (ov5693 at OVTI5693, by femito1) in the PR thread. The Pro 9's OVTI5693 ACPI HID is already added by the 'Add camera support for Surface Pro 9' patch earlier in this patchset (from linux-surface#1867), so no match-table change is needed; update the patch subject, commit message, in-code comment and Link tags to record the broader IPU6 scope instead. Link: linux-surface#2171 (comment) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@femito1 Thanks for testing and for the offer! Before folding the one-liner in I double-checked the patchset, and it turns out
So on a linux-surface kernel your Pro 9 binds without any extra change — I'm guessing you were testing mainline/stock + this patch, where the HID really is missing (I checked v6.19: neither What I updated instead (just pushed):
Sending the HID bits upstream to linux-media sounds great — that's exactly where they're missing, and yes, both the fyi: Claude generated this comment so I could be wrong, I think its look |
|
Confirmed this patch also fixes the OV5693 front camera on a Microsoft Surface Pro 7+. Tested setup:
After applying the Before the patch, the sensor was detected but the IPU6 receiver did not receive frames. The rear OV8865 on the same Surface Pro 7+ is also now working after enabling its additional |
|
Hi @naeemarsalan , Following up on the MIPI_CTRL00 (0x4800) write from PR#2171. Two things: some real test data, and a question about how we get it upstream together. Background: my OVTI5693 HID patch is now a v2 two-patch series on linux-media (Dan Scally's Reviewed-by carried over). It only adds the ACPI HID and is independent of the register write. On that thread, Sakari Ailus asked whether the 0x4800 value could be something safer/more minimal than 0x2d, since the OV5693 is also used on IPU3 (CIO2) and a Rockchip board and he doesn't want to regress those. I have the Surface Pro 9 (IPU6) hardware, so I characterized the register properly:
Bit meanings are confirmed by the sibling kernel drivers and the datasheets: ov5647.c and ov5648.c define bit 5 = CLOCK_LANE_GATE / CLK_LANE_AUTOGATE and bit 2 = LP11-idle; ov5640.c actually sets 0x4800 = 0x24 with the comment "[5] Gate clock when no packets, [2] MIPI bus in LP11 when no packets"; and the OV5640 / OV5645 datasheets document the same bit-5 = clock-lane-gate meaning. (There's no public OV5693 datasheet with a register table, so this leans on the sibling parts, which share the register block.) My suggestion for the patch value is 0x24 rather than 0x2d: bit 5 (the bit IPU6 actually needs) + bit 2 (LP11 idle, which is what Sakari asked about). It's the documented ov5640 value, the easiest to justify in a commit message, and the safest bet for the shared IPU3/Rockchip users since every OV sibling sets bit 5 on all platforms. 0x2d works too, but it also sets bit 3 (lane-2 select, not relevant to this 1-lane config) and bit 0 (undocumented). Your call, if 0x2d is what you've actually validated across Pro 8 / Go 4, that's reason to keep it; we'd just want to note in the commit message that bit 5 is the decisive bit. (I've only tested 0x24 on SP9/IPU6, not on IPU3 or Rockchip, so I can't claim it cross-platform) The register write is originally your work, and the bit characterization + the 0x24 recommendation are mine. That feels like a co-authored patch to me, and I'd rather do it that way than either of us taking sole credit. I can't add your Signed-off-by or a Co-developed-by tag without your OK, so how would you like to do it? A few options: (a) You send it, listed as author; I'm added as Co-developed-by: Fernando Rimoli fernandorimoli11@gmail.com + my Signed-off-by. Whichever way, it should Link back to PR#2171, and go to the same recipients as my series (get_maintainer.pl on drivers/media/i2c/ov5693.c -> Dan Scally, Sakari Ailus, linux-media, cc Mauro + linux-kernel). If you'd like me to send it I'll need the exact name + email you want on your Signed-off-by, and I'll share the full write-up so you can check it before it goes out. Let me know which you prefer and I'll get it moving. Cheers, |
|
I tested the proposed Test environment
Results with
|
|
@femito1 Hey! Option (b) sounds good, go for it! For the sign-off: Arsalan Naeem naeemarsalan@gmail.com Honestly I'm not a kernel dev, I just had a busted camera on my Pro 8 and got lucky figuring out the register with a lot of LLM help. You did the real digging on the bits so happy for you to take it from here. 0x24 is fine by me too. Thanks! |
|
This pull request help to me! This work in |
Additional Surface Go 4 validationI completed a more detailed end-to-end test of the Environment
ResultsAfter a full shutdown and cold boot, with
I also captured 10 valid 640×480 PPM images and visually confirmed that they contained a real image from the front camera. This provides stronger evidence than device detection or buffer activity alone: the patched OV5693 produced valid image data on Surface Go 4. Application-level resultAfter restarting WirePlumber, both cameras appeared through PipeWire as: Both front OV5693 and rear OV8865 then displayed live video in GNOME Camera. The WirePlumber restart was needed because its initial startup probe failed to open Detailed report: Remaining issues are separate from this stream fix:
|
Surface Pro 8: bit sweep of MIPI_CTRL00 — only bit 5 is requiredIndependent confirmation on a Surface Pro 8, plus data on the "can Short version: Environment
Method: no rebuild neededRather than building and MOK-signing a patched module per candidate value, I wrote sudo modprobe i2c-dev
# start a capture; it hangs with no frames
cam -c2 --capture=30 &
sleep 7
# poke the value under test
sudo i2ctransfer -f -y 15 w3@0x36 0x48 0x00 0x20
# frames start immediately if the value worksEach test is an independent stream: the sensor powers down between captures (I²C times out when idle), so Results
Before the write: sensor powered and healthy (chip ID SuggestionSince the concern is regressing IPU3/CIO2 and Rockchip users, the most conservative possible change is to set only bit 5 and preserve whatever else the platform left in the register, using the read-modify-write helper this driver already uses elsewhere: /* MIPI control */
#define OV5693_MIPI_CTRL00_REG CCI_REG8(0x4800)
#define OV5693_MIPI_CTRL00_CLK_GATE BIT(5)
...
if (enable)
cci_update_bits(ov5693->regmap, OV5693_MIPI_CTRL00_REG,
OV5693_MIPI_CTRL00_CLK_GATE,
OV5693_MIPI_CTRL00_CLK_GATE, &ret);That touches one bit instead of overwriting all eight, which should be an easier sell than either Caveats
Root-caused independently before finding this PR (I hit the same dead camera and didn't search for an open PR. So the LLM worked back from |
What
Adds one commit to the
cameraspatchset (patches/6.19/0013-cameras.patch) that programsMIPI_CTRL00(register0x4800)= 0x2dbefore stream-on in theov5693driver. With this, the front camera streams on IPU6 on:ov5693atINT33BE) — verified by me, ~28 fps via libcamera, real image out (the SP8 case in Surface Pro 8 camera support #1893)ov5693atINT33BE) — confirmed by @Fugu0141 (comment), ~28.6 fps (Surface Go 4 IPU6 camera status: rear OV8865 detected/partially streams, front OV5693 LED only #2154)ov5693atOVTI5693) — confirmed by @femito1 (comment) (front-camera part of Camera ov5693 driver kernel module on Ubuntu 20.04, 6.17.1-surface-2 and Surface Pro 9 #1892)No ACPI match-table change is needed for the Pro 9 in this repo: the
OVTI5693HID is already added by the "Add camera support for Surface Pro 9" patch earlier in this same patchset (#1867, shipped in every 6.19 release since6.19.7-1) — both inov5693_acpi_match[]and inipu-bridge.c. Mainline v6.19 has neither, so the HID does still need to go to linux-media separately (@femito1 is planning to send that).Why it was broken
Mainline
ov5693.cnever writesMIPI_CTRL00, leaving it at the0x00power-on default. That default is fine on the IPU3 Surface devices (Pro 5/6/7), where this sensor already works. On the IPU6 devices the same sensor is wired to an IPU6, and with0x4800 == 0x00the D-PHY never locks: the sensor reports streaming, the PHY powers up, but the receiver gets zero CSI-2 packets (no SOT/CRC errors at all) and capture ends instream stop time out. Writing0x2dconfigures the clock-lane behaviour the IPU6 expects and frames flow.How it was found
Reverse-engineered from the Surface Pro 8 Windows driver (
ov5693.sysregister table) —0x2dis the value Windows programs. Diffing the Windows init against mainline madeMIPI_CTRL00the obvious omission, and setting it alone fixes streaming.Honest caveats (feedback very welcome)
I want to be upfront — parts of this were experimental / AI-assisted and I'd really appreciate review:
0x00default today. It should be confirmed there before merge — or gated if it regresses them.0x2dbit meaning is taken from the vendor driver, not a datasheet — I know empirically it works, not the precise semantics of every bit.0x4806/0x4816/0x4831/0x4d00/0x4d01);0x2dalone is sufficient to stream so I left them out, but they (or an IPU6-side watermark tweak) may clean up the overflow.patches/6.19/. Happy to reformat, split, target thelinux-surface/kerneltree instead, or propagate to other version dirs — whatever you prefer.Refs
ov5693on IPU6 (confirmed in-thread)ov5693(confirmed in-thread; binds via theOVTI5693HID already in this patchset from Add camera support for Surface Pro 9 #1867)Verified on: Surface Pro 8,
kernel-surface 6.19.8-3, Fedora 44, libcamera 0.7.1 — plus the Go 4 and Pro 9 confirmations linked above.