Update ov5693 and intel-skl-int3472 drivers

[djrscally]

This PR updates the two main in-progress camera modules.

The intel-skl-int3472 driver actually has little functional change, because I'd already merged the main functional updates separately. The most major change is that the indicator-led will no longer be exposed to the sensor drivers and instead is simply triggered along with the clock enable pin.

The ov5693 driver has been extensively cleaned up (I will be upstreaming this soon, possibly tomorrow...or atleast submitting a v1 that will probably elicit a lot of feedback!). Main things to point out:

1. I reduced the modes to 3 (2592x1944, 1920x1080 and 1280x720), primarily I can't see any reason to have more than that. All the modes have full field of view, through either scaling or binning. Defining modes now echoes the ov8865/ov5648 driver, I.E. you can use raw values in a struct instead of incomprehensible register values, so adding more if desired should be easy.
2. All mandatory, recommended and optional controls for libcamera are now included, meaning you should receive no errors or warnings running qcam
3. Added a test pattern control...quite fun to turn it on and see the snow.
4. .probe() now checks for the cio2-bridge created endpoint and returns -EPROBE_DEFER if it's not present; this is necessary to ensure @fabwu's rotation and orientation controls patch works reliably.

There's one remaining message in dmesg to this effect:

use of bytesused == 0 is deprecated and will be removed in the future, use the acutal size instead

This comes from the V4L2 framework, but I haven't tracked down the cause yet.

[djrscally]

@qzed thanks for the comments - much appreciated.

Some testing results: Before updating libcamera, it works as before, i.e. the image is flipped and has a green tint. After updating libcamera, the image is now correctly rotated and now has a blue tint. Neither version seems to have any complaints wrt. missing controls or other things.

Hm, weird. I don't get a blue tint, I'll pull libcamera master and see what that does.

Apart from that, the LED still blinks on startup and I do still get some occasional errors in dmesg, although I can't see any real issues:

[  955.357019] ipu3-cio2 0000:00:14.3: CSI-2 receiver port 1: incomplete long packet detected
[  955.390497] ipu3-cio2 0000:00:14.3: port 1 error PKT2SHORT
[  955.390501] ipu3-cio2 0000:00:14.3: CSI-2 receiver port 1: frame sync error

Otherwise small things mostly, looks good overall. What's the status on the LED issue?

Alas, with the move to run the LED at the same time as the clock @fabwu's fix for that no longer applies...I'm sure I remember Sakari fixing it somehow on the mailing lists recently, I'll hunt that down when I get time. Or do you mean the chap from reddit who's LED came on permanently? I got him to send me dmesg output, but nothing in there looks unusual. My best guess at the moment is some kind of pm_runtime jiggery-pokery, like maybe it's not suspending properly or something...not sure yet.

I'd really like those CSI-2 errors to not be there...any chance you can try the make clean && make thing that managed to get it working last time at some point?

@fabwu Do you want to look over this too?

[qzed]

Hm, weird. I don't get a blue tint, I'll pull libcamera master and see what that does.

Happened after I've updated to https://github.com/kbingham/libcamera/commit/de1b994bbaa4e34dc39c997d99f47026e7cddcfe today. Pretty sure that change is not related to the kernel driver.

Alas, with the move to run the LED at the same time as the clock @fabwu's fix for that no longer applies...I'm sure I remember Sakari fixing it somehow on the mailing lists recently, I'll hunt that down when I get time. Or do you mean the chap from reddit who's LED came on permanently? I got him to send me dmesg output, but nothing in there looks unusual. My best guess at the moment is some kind of pm_runtime jiggery-pokery, like maybe it's not suspending properly or something...not sure yet.

I actually meant the reddit issue. Thanks! I think you're probably right with the PM guess, I think that's the only thing that changed in that range with regards to the LED.

I'd really like those CSI-2 errors to not be there...any chance you can try the make clean && make thing that managed to get it working last time at some point?

I can try, but might take a bit until I get around to that. I'm currently busy bisecting some GPIO issue on v5.12-rc2... gpiod_get() fails with ENOENT for some surface drivers (hotplug and aggregator).

Edit: That didn't get rid of the messages last time around though and I have the feeling that they aren't directly related to any drivers in this PR. It kinda looks like maybe one of the initial transmissions via CIO2 after starting up qcam is somehow faulty. By the wording, I'd kinda expect the messages to be in that part of the code, decoding those messages. And they don't repeat after it started capturing, so it doesn't seem like it's all too critical.

[djrscally]

Hm, weird. I don't get a blue tint, I'll pull libcamera master and see what that does.

Happened after I've updated to kbingham/libcamera@de1b994 today. Pretty sure that change is not related to the kernel driver.

Alas, with the move to run the LED at the same time as the clock @fabwu's fix for that no longer applies...I'm sure I remember Sakari fixing it somehow on the mailing lists recently, I'll hunt that down when I get time. Or do you mean the chap from reddit who's LED came on permanently? I got him to send me dmesg output, but nothing in there looks unusual. My best guess at the moment is some kind of pm_runtime jiggery-pokery, like maybe it's not suspending properly or something...not sure yet.

I actually meant the reddit issue. Thanks! I think you're probably right with the PM guess, I think that's the only thing that changed in that range with regards to the LED.

Yeah...I'll get him to ramp up the kernel log level and try again.

I'd really like those CSI-2 errors to not be there...any chance you can try the make clean && make thing that managed to get it working last time at some point?

I can try, but might take a bit until I get around to that. I'm currently busy bisecting some GPIO issue on v5.12-rc2... gpiod_get() fails with ENOENT for some surface drivers (hotplug and aggregator).

Edit: That didn't get rid of the messages last time around though and I have the feeling that they aren't directly related to any drivers in this PR. It kinda looks like maybe one of the initial transmissions via CIO2 after starting up qcam is somehow faulty. By the wording, I'd kinda expect the messages to be in that part of the code, decoding those messages. And they don't repeat after it started capturing, so it doesn't seem like it's all too critical.

Ah, sorry. Must have misunderstood before. Did they ever go away from the last PR then, or where they always there? I was under the impression they'd gone. It could be something in libcamera or qcam too I suppose yes.

[qzed]

Ah, sorry. Must have misunderstood before. Did they ever go away from the last PR then, or where they always there? I was under the impression they'd gone. It could be something in libcamera or qcam too I suppose yes.

I didn't test that much, but I'm fairly certain now that they've always been there. Before and after the last PR. Usually the first time starting qcam works without messages, but repeated starts seem to cause them. Thinking of it, could it be some kind of buffer that doesn't get cleared when closing and is read in the next time?

[fabwu]

I tried a make clean && make and get this error:

ld: fs/efivarfs/file.o: in function `acpi_get_gpiod':
file.c:(.text+0x430): multiple definition of `acpi_get_gpiod'; fs/efivarfs/inode.o:inode.c:(.text+0x40): first defined here
ld: fs/efivarfs/super.o: in function `acpi_get_gpiod':
super.c:(.text+0x510): multiple definition of `acpi_get_gpiod'; fs/efivarfs/inode.o:inode.c:(.text+0x40): first defined here
make[2]: *** [scripts/Makefile.build:430: fs/efivarfs/efivarfs.o] Error 1
make[1]: *** [scripts/Makefile.build:496: fs/efivarfs] Error 2
make: *** [Makefile:1800: fs] Error 2

@djrscally Have you ever tried to build your changes on a clean kernel?

[djrscally]

I tried a make clean && make and get this error:

ld: fs/efivarfs/file.o: in function `acpi_get_gpiod':
file.c:(.text+0x430): multiple definition of `acpi_get_gpiod'; fs/efivarfs/inode.o:inode.c:(.text+0x40): first defined here
ld: fs/efivarfs/super.o: in function `acpi_get_gpiod':
super.c:(.text+0x510): multiple definition of `acpi_get_gpiod'; fs/efivarfs/inode.o:inode.c:(.text+0x40): first defined here
make[2]: *** [scripts/Makefile.build:430: fs/efivarfs/efivarfs.o] Error 1
make[1]: *** [scripts/Makefile.build:496: fs/efivarfs] Error 2
make: *** [Makefile:1800: fs] Error 2

@djrscally Have you ever tried to build your changes on a clean kernel?

Oh shoot! I haven't, no. Sorry...didn't realise that could result in missed issues like this. I'll do that now and fix what crops up.

[qzed]

Hmm, I thought I did clean-build this. I usually run make distclean and ./scripts/kconfig/merge_config.sh ... if I check out any new branches.

[djrscally]

Yeah, actually I did make clean && make now, and it's built without error for me.

[fabwu]

Mmh that's strange I'll try make distclean maybe that helps.

[qzed]

@djrscally I've added a new v5.12 based branch (v5.12-surface-devel) with the camera commits taken from your latest kernel submissions (v2 "Introduce intel_skl_int3472 driver" and v2 "Add support for OV5693", plus some commits from @fabwu from v5.11-surface-devel, the rest should already be upstream). I think it's best if you open a new PR and base any updates on that.

Also feel free to have a look over the camera related commits on that branch and let me know if it's missing anything (front camera works as far as I can tell, does have a purple tint though).

[djrscally]

@qzed thanks! Sorry, kinda abandoned this, time constraints lately. I'll get back to it asap :)

[qzed]

No worries.

[fabwu]

Thanks for merging! I saw some of the surface patches are upstream that's awesome.

I added on tiny PR https://github.com/linux-surface/kernel/pull/95 just to make sure everything is working.

Quick question: I sent a patch a month ago and haven't yet received an answer. Should I resend it or is it too early? Or maybe write directly to the person who's responsible for it?

[djrscally]

Quick question: I sent a patch a month ago and haven't yet received an answer. Should I resend it or is it too early? Or maybe write directly to the person who's responsible for it?

Yeah I was just starting to think about chasing them on that - I would reply to Rafael's mail asking erik to pick up the 1/2 patch for ACPICA and ask if anything else needs doing...and maybe just a general ping on the 2/2 asking for any more comments, or for it to be picked up - it'll probably be Sakari who has to pick it anyway so he should see that.