Camera support #1
Comments
|
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? |
|
Correct, I think. But on Linux, getting them is fairly easy; it could be worth to double-check whether they are identical: |
|
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. |
|
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 |
|
Unfortunately I am not very qualified in this field. But I can ask a friend to help me. |
|
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
<#1 (comment)>,
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:
***@***.***>
|
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.)
Sounds great! I would have expected problems/significant effort for the 8858; if that could be made work, even better. |
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.
Yup still actively tackling the surface ones so we're kinda used to this now. |
|
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. I did not investigate the error itself yet, but will try to do so. Problem report on Windows will follow after a reboot. |
|
Situation on Windows:
|
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: 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 |
|
Okay, that helps. Compilation works now, trying to progress from here. |
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: The last field in the definition block is the version number |
|
Hehe, okay, makes sense. And that's soothing, cause...
Will set me back a bit, but I'll recover. :) |
|
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 |
|
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. |
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. |
Will do. But ...
... what exactly are we after here? Just voltages, or something else, too?
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... |
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.
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. |
|
First achievements: Loading an AML compiled with iasl version 20160527 worked better, apparently. Here is a precompiled binary distribution for Windows, too. I'd assume I2C2 is the correct bus, since the DSDT has and this should be the camera power management module, right? |
|
Nice one, well done. That's the PMIC yes; the entry gives its I2C address further down: So you should be able to connect with edit: assuming the above works, best thing to do is simply dump out all the registers for the PMIC three times:
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 |
|
Strange. On Dumps: No camera: Front camera: Rear camera: Changes over camera off are marked with |
|
So front camera: 0x02, 0x07 and 0x09 are status and clock settings 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. |
|
Sorry; I need the DMI vendor and product name for your device; can you give me |
|
Sure, that'd be 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. |
|
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.
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"?
From what I can tell, voltages are
That'd be nearly identical to the Surface Go settings that are already in I hope I can free some more time tomorrow for a bit of investigation... |
Exactly.
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
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. |
dmidecode 3.2Getting SMBIOS data from sysfs. Handle 0x000B, DMI type 1, 27 bytes |
|
Here is the list of model variants of the thinkpad x1 tablet gen 2 which should all have the same camera model: |
|
Sorry for the silence; finally could manage to try a couple of things. First, some (probably) simple patches over Dan's branch, to bring the Gen 1 and Gen 2 sensor activation on solid ground: e0daada and 88bccfe. Second, with the minimal diff that leads to somehow useful output on my machine (sorry guys, I lost track of what the current best bet is): (Pinkish output, With this, I used the Instead, I receive: First observation: I assume the color coding is messed up; I had some interim solution with better alignment, but I lost it somewhere during testing, and there it looked like the BG/GR was rather interpreted as GBGR or something; I assume this is what we hope could be fixed with horizontal and vertical flipping? Second observation: the image has size 1950x1092, while I'd expect 1920x1080 (nominal camera resolution) or perhaps 1936x1112 or 1936x1116 (physical array size according to the datasheet, depending whether you believe more the sketch of the array or the claimed 20 rows of black, which can probably be used for black level compensation or whatever), or at least 1932x1092 (description of the mode_reg line in ov2740.c). Which brings me to my next open question: why the funny 1932x1092? Shouldn't it be 1920x1080? Or, if cropping is disabled and the entire sensor array is used, 1936x1096? And does it even make sense that we mess around with the TIMING_HTS and TIMING_VTS values (0x380c to 0x380f), and perhaps even the OUTPUT_SIZE values? Are they overwritten in |
|
Ah, at least I can explain the 1950: ipu3-unpack and ipu3-capture pad to the next multiple of 50, IIUC. Cropping the black border (and cutting off the bit of noise at the bottom) makes the output 1933 pixels wide, which is sufficiently close to 1932 that I assume that 1932 pixels per row are actually read... |
|
58b5201 does wonders for my complexion. :) |
Oh good spot! I was stuck on the the width and flip thing screwing with the bayer ordering somehow (you are right that this can cause the same effect. Libcamera needs to know the sensor's orientation and have control over the flipping of the image so that it can calculate and apply a correction). Is that a mistake we introduced, or was it always there? Do you still need to tweak 0x3805 to make it work after that change?
That's just the size that the driver had already defined when we started working on it; I don't know why they chose that size either.
Actually, setting 0x3805 = 0x8f makes the output 1933 pixels wide. The mode definition says the width should be 1932, but we're telling the sensor to output 1933. That's why it is confusing me that that change "works"...because as far as I understand things, it's wrong.
Output size if set during ov2740_start_streaming() as part of the array of register values. HTS and VTS are controlled by the HBLANK and VBLANK controls, so will be configured to some defaults during ov2740_start_streaming() too but then can be changed at any point after that. Those will affect the framerate though, not the size of the output image. Sorry for going quiet here the last week or so also, had a bunch of other things to work on. I think we're pretty close with this now though right? We could get it cleaned up and raise a patch series. |
For me, tweaking 0x3805 is still needed. With your patch, the image is green instead of pink. Flipping does not work (as reported here); if i add this:
(as suggested here) to make flipping work again, vertical_flip=1 turns the image pink again, so pink and green are basically exchanged. |
It's been there ever since. Not sure which driver this was forked from; but somehow I feel that this was either meant for a platform where significant work has been applied in user space, or it wasn't really tested.
Yes. [...] So, regarding register, mode, and crop setting: I got sick and tired of rebooting (which always takes me ages, thanks to Bitlocker being overly picky about changing default boot targets, plus disk encryption wanting a password, plus boot loader/"enter BIOS" hooks being somewhat unreliable on this machine.) So I spent some time offline with one of my crudest hacks ever, to avoid reboots: x1-tablet-gen2/0004-media-i2c-ov2740-Add-dynamic-changes-of-mode-crop-re.patch, and I share it in case it makes someone's future testing easier. In short: this allows to you define three files, which are dynamically loaded into the driver upon powerup of the device.
and accepts hex (prefix 0x) or decimal values, and ignores unparsable lines (especially ones starting with
and in that order. Ugly hack, but it does the job. And I tested a number of settings, and eventually ended up with something vaguely usable - no errors anymore in dmesg, and it's values that seem to make some sense to me: Reg: Crop: Mode: Framerate is still stuck at 15 FPS, but apart from that it looks pretty okay.
Same here. And right, I think so, too; would be nice to at least get this to a baseline where we can suggest other interested parties to give it a shot without need to do custom patching upon the tree. I might have found a victim in real life yesterday. ;-) |
|
Meh. Except that it doesn't work with those values at the initial boot-up. Something about HTS and VTS is messed up, and the device doesn't probe properly. Coincidentally, the values I used in the reg and mode sections don't match - my bad, I'll investigate. Actually using it after a successful initial detection is apparently more robust; the actual HTS/VTS setting during use and the interpretation of the blanking periods has some more involved logic than just fixed register values. However, I've got to admit that I don't really understand what the HTS and VTS values are. My Google-Fu for "MIPI SCLK HTS VTS" claims that Finally, the driver currently has What is MCLK? And where do the 360 MHz come from? The datasheet claims 720 Mbps/lane and a 720 MHz MIPI serial clock, and I can't find any mention of 360 in the sheet... |
Ah, ok - good luck! The values you gave looked ok at a quick glance, though I confess that starting the window at 0,0 instead of in the centre causes me pain :P
Columns/Lines of pixels. MCLK is the sync clock, LINK_FREQ is the "bandwidth"
This is correct
MCLK is the 19.2MHz reference clock being provided to the camera sensor by something else in the device, in your case it's part of the TPS68470 stuff we looked at ages ago. The 360 MHz is the rate of the transfer between the camera sensor and the CSI-2 receiver; the data captured by the sensor is transmitted electrically along a bus to the receiver (controlled by the ipu3-cio2 driver) which loads it into memory and send it to the image processing unit. The datasheet is probably giving the maximum rate, but my understanding is it just needs to be sufficient to carry the data that the sensor puts out, which looks something like this: link frequency = (pixel rate * bits per sample) / nr of lanes / 2 pixel rate being the number of pixels in an image multiplied by the frame rate you need to support. Bits per sample is the number of bits of data it takes to transmit a single pixel (which is encoded in the media bus format code MEDIA_BUS_FMT_SBGGR10_1X10 - the number after the X is the bits per sample). Nr of lanes is the number of data lanes; the MIPI interface allows for up to four data lanes to run concurrently, though in your case there's only 2. Finally, divided by 2 because the format is double data rate. So for the only mode the ov2740 has that's ((1932 * 1092 * 60) * 10) / 2 / 2 = 316461600 (assuming it is 60fps, the datasheet says it should be). The actual link frequency that gets set depends on the capabilities of the sensor's PLL, but as far as I understand things is ideally selected by the vendor to reduce EM interference with other components in the platform. In the cio2-bridge code we mimic firmware setting a link frequency for our devices: static const struct cio2_sensor_config cio2_supported_sensors[] = {
/* Omnivision OV5693 */
CIO2_SENSOR_CONFIG("INT33BE", 1, 419200000),
/* Omnivision OV8865 */
CIO2_SENSOR_CONFIG("INT347A", 1, 360000000),
/* Omnivision OV7251 */
CIO2_SENSOR_CONFIG("INT347E", 1, 319200000),
/* Omnivision OV2680 */
CIO2_SENSOR_CONFIG("OVTI2680", 0),
};and drivers compare the link frequency that's defined in firmware with the values that they have been built to support, and will fail out if the firmware asks for a link freq that they can't do. My experience so far is (as with the first and 3rd entry in that list above) they're setting some value plus the MCLK, so if you scraped the PLL registers whilst running windows you might find it was running at 319.2MHz there. We could add support for that frequency to the driver too.
VTS and HTS, minus the height and width of the image that's output, define "blanking" time which is an idle period during which no data is produced. You can increase the blanking to lower frame rate without otherwise changing the output format |
Thanks once again for a verbose explanation, really appreciated. I'll try and digest at due time. ;-) Just one minor note: wouldn't a factor 3 or 4 come in for RGB or BGGR? That has already puzzled me - I cannot link the sizes of the .bin and .raw output of the ipu3-capture.sh script (2735616 and 4274400 bytes, with my current settings) to any reasonable multiple of (1936 or 1920) and (1096 or 1080)... With the script saying and looking at https://www.kernel.org/doc/html/v4.15/media/uapi/v4l/subdev-formats.html#bayer-formats, I'd expect 19361096(10/8)*4 = 10368000 bytes per frame. On the other hand, the output goes on with which matches the .bin size? (And 2735616 = 1096*2496; but where the heck does the 2496 come from?) |
Er, I don't see that it would. Why do you think so?
Stride is the number of bytes needed to store a full row with some configurable padding in libcamera. The format the data is output from the sensor to the CSI-2 receiver is actually a packed one - 25 pixels into each 32 bytes, with 6 bytes of padding. 1936 / 25 = 77.44, which in that format becomes (77 * 32) + 32 = 2496 bytes. The ipu3-capture script unpacks that format into one that can be converted easily into a viewable format. |
|
Is there anything new, which I can try/test? I am a little limited with what I can do, but before this thread goes dead, I will try new things out :-) |
|
Good point, got side-tracked a little bit. But my camera actually works now, as long as I use the right kernel... ;-) @djrscally Any plans how we should proceed here? Do you want to, uhm, foreport (as opposed to backport?) your branch onto 5.18 / master before we go any further? Do you think this is in a state where you'd feel comfortable to send a patch to Hans or the ACPI maintainers? As far as I can see, the change that is still missing upstream is 1677773acbcc6bbd5e75223db0dae6f4a8d9bdd6. Most likely, everything else (int3472 stuff, board data, multiple consumers etc., and ov2740 stuff) should be pretty much isolated from the rest of the kernel, right? In particular, I have a working setting for the camera that I can dig out from my sources. It's not fully warning-free, but it works overall without hickups, and decodes color correctly; I assume that's a good starting point. |
(snip)
Ah, because I was mistaken as always by being idealistic about marketing figures. I thought that 1936x1096 pixels means 1936x1096 full-color sensors, each a 2x2-array of BG/GR subpixels. But your explanation made it clear that it's actually just 1936x1096 monochromatic pixels, half of which are green and a quarter each are blue and red; and the overall color information is interpolated in the debayering stage. That plus the packed format makes sense; I should have RTFM with more attention. |
Er there'll be some cleanup to do and I think that my bare branch still doesn't actually work as I have the settings wrong...can you link the tree you've got working and I'll cross reference them and make sure I get the settings right, rebase onto media-master, tidy, and then we can send a patchset to linux-media.
That whole multiple consumers series actually needs independently of this to enable the IR camera on my surface go2. I totally forgot that Rafael asked for a (fairly minor) change that I haven't done, so it's waiting for me. I'll fix that and send it. Otherwise yes everything else should be pretty standalone
Automatic gain on the sensor side you mean? |
Changes over Lenovo's ThinkPad X1 Tablet Gen 2: - INT3472:05 -> INT3472:04 - INT3474:01 -> INT3474:00 as reported in akobel/linux-thinkpad-x1-tablet#1 (comment) Signed-off-by: Alexander Kobel <[email protected]>
Right. akobel/linux-djrscally@9b096f4 should be reasonable. TL;DR: 30Hz at 1920x1080 works. First stream is hit-and-miss, second and subsequent streams are steady. Edit: Not sure whether it's just coincidence, but the first attempt at streaming seems to freeze after some 20 seconds or so if I start very early after boot (E.g., to test these driver settings). It's stable if I start later, like, several minutes after booting. That's the only thing that really annoys me for publishing, but I couldn't find a fully solid version without freezes (or I accidentally removed it while I was looking for the right color and couldn't find it again). Perhaps someone else can test or tweak until it's reliable; everything else (gain, FPS, HDR, lens correction, different resolutions/modes) is not that crucial for very basic (webcam) use.
Yes, but I realized that there is a corresponding v4l knob, right? So I think the control is actually supported, it's just that qcam doesn't try to use it. But perhaps there's also automatic support, who knows. |
|
@djrscally Lost track again, sorry. Are there any news and/or plans from your side? I personally didn't spend any time on that one since June, but if I know the route to go, I can take over a couple of steps. If I read the recent kernel logs correctly, the multi-consumer patch is in the 6.1 release candidate, right? |
|
@djrscally Hey Dan, got back to that one again. I successfully ran a test with fairly minimal modifications on a recent 6.2.6 kernel (Arch's build). All I had to do was to 1. apply from your djrscally/thinkpad-x1 branch
and 3. copy over the results on plus additional changes regarding fixing the Bayer format and some mode setting adjustments. Now, IIUC, patches towards ov2740 shall be based on the linuxtv media_tree master. There are some other mostly unrelated changes on that branch; some are cosmetic, some others small detail fixes, but all shall be kept for sure. Your patches don't cleanly apply. I'm unsure on how to proceed. I would agree to try and rebase your patches, but first I'm not quite sure whether the patches on int3472 go to the same branch (probably not?), those on ipu3-cio2 go to the same maintainers (probably not, and there's no base tree given - is it Linus' tree then?), and whether all can be submitted at the same time. So, long story short: would you be able/willing to revisit the patches mentioned above, authored by you, and rebase them to the proper branch, so that I can add the few things required to get some useful output? It's really "around the corner" now, I think. |
|
@akobel Hey dude - I'm sorry this one keeps slipping off my radar!
No those should be based on platform-drivers-x86/for-next and sent to the platform-driver-x86 mailing list.
Those go to linux media (so based on media_tree next)...but I'd be the one looking at them anyway.
It would depend on the scope of your changes; you can always add a Co-developed-by (which should be followed by a Signed-off-by-akobel) to have both authors on there.
Batches to each mailing list; so all the ov2740 ones and the ipu3-cio2 one to linux media in one series, but the platform/x86 one separately.
Yes, absolutely - and I'm sorry that I didn't do it already. I'll try to get it done over the weekend. |
@djrscally No worries, thanks for getting back to us!
Okay, that's pretty much what I expected.
Cool. My main concern was to submit patches (whether mildly changed or not) without your confirmation / LGTM. After all, I wouldn't claim to have written them, but also don't want to put your name under something that I could potentially mess up. ;-) Most of the adjustments should be trivial, but there's actually a few that might be trivial to merge for you, but aren't fully obvious for me (regarding powerup/powerdown/autosuspend). Anyway, if you submit your part, please cc me; I'll be happy to test and work on the remaining adjustments (timing/cropping) that will be cumbersome for you without access to the hardware. |
Yes this part is always annoying. Having to account for both acpi and devicetree firmware (which work completely differently) and for our hacks which are kinda half way between the two makes it a bit rubbish. The Intel guys have got some work going on to make all that much much easier though.
So I rebased my thinkpad-x1 branch onto media master and pushed it here. It's not got the board data patch that I made since you had to change it; you should be able to drop yours in, but rebase so that it's at the front (I.E. directly after eeac8ede1755 ("media/master, media-master) Linux 6.3-rc2")) to make it easier to output the series for linux media. Do you want to add your changes on top and make sure everything's working and then we can look to post them? Separately would be fine, but you could also just send the lot together if you like and I'll pick up any review comments that people make for my patches. Up to you. |
Thanks, I'll have a look; tomorrow or the day after, I guess.
Yes, I think it makes sense if I try and add my bits and pieces; IIRC, they are required to get a picture on the first attempt. If someone can test on their hardware, it'd be nice if they only have to apply one self-contained patch set.
That one I don't really care about. Let me first prepare the branch, then we'll see. |
|
Hi again, So, I went for building the entire kernel from your branch. That didn't work out for me; my usual "keep it simple, stupid"-approach of taking the Arch build script for a kernel package (to rule out any misconfigurations on my end) gave me some (essentially unrelated) errors that I don't quite remember right now anymore, sorry. No further investigation done yet, sorry... Try to give it some more time asap. |
|
@akobel it's a bug that has cropped up after some other patches in 6.3 - I'm actually trying to figure it out now already, but for now if you run 6.2 instead you'll avoid this error. |
|
Thanks for the hint; I think I had build problems on 6.2, though. That's one of the reasons for the procrastination; I'll check again. |
Well I got to the bottom of the problem, so you could also apply this fix: ---
drivers/media/v4l2-core/v4l2-subdev.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index b10045c02f43..21c9860ce43a 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -1236,6 +1236,10 @@ int v4l2_subdev_link_validate(struct media_link *link)
struct v4l2_subdev_state *source_state, *sink_state;
int ret;
+ if (!is_media_entity_v4l2_subdev(link->source->entity) ||
+ !is_media_entity_v4l2_subdev(link->sink->entity))
+ return 0;
+
sink_sd = media_entity_to_v4l2_subdev(link->sink->entity);
source_sd = media_entity_to_v4l2_subdev(link->source->entity);
--
2.34.1And then it should work fine on 6.3 again |
|
Finally. Overall set of patches is: patches-over-a18dd1c42508.tar.gz |
|
@akobel no need to apologise! I'm just catching up on things (been at a conference and prepping for that for the last two weeks so I have a lot to get through) and then I'll rebase the set onto linux-media for-next and we can post them - there are indeed upstream changes that need to be taken into account. There's a couple of comments I'd have out the gate though that we'll need to fix:
|
Cool. I'll have another look then, and prepare for the following in the meanwhile (but I have a busy week ahead, hence unlikely that I can do it before mid-month).
Sure, no problem (can add it myself, though; will anyways post you a new patchset, I guess, with the changes below).
Hm, okay. The problem here is that the original definition never worked for me. For admittedly unclear reasons; it's not that it was invalid per se, as far as I can tell from the datasheet. In any case, the mode name (mode_1932x1092) would need an adjustment. Can do that. An additional complication might be that, according to my experiments, the register settings (especially timing control) need to be set to 1936x1096 (native size), and only then the cropping comes into effect. But OV2740_ACTIVE_START_TOP/LEFT are so large that 1932x1092 cannot be achieved with that crop window. So perhaps I should try and get rid of those macros predefined, and then try to adapt ov2740_get_selection appropriately.
Reason was that I had to replace it at several locations (easy to grep, but still); so I reckoned it'd be good to only have one central definition for that. But I can change that, of course. |
Continued from linux-surface/linux-surface#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)
The text was updated successfully, but these errors were encountered: