2021-11-08 06:02:45

by Hans de Goede

[permalink] [raw]
Subject: Re: [BUG 5/5] [BUG] media: atomisp: atomisp causes touchscreen to stop working on Microsoft Surface 3

Hi,

On 10/21/21 11:52, Tsuchiya Yuto wrote:
> Thank you for your comment :-)
>
> First, I need to correct what I said in the previous mail. I later found
> that loading only "atomisp" (as well as its dependency,
> atomisp_gmin_platform) does not cause this issue.
>
> What causes this issue is rather, loading sensor drivers (as well as its
> dependency, atomisp_gmin_platform).
>
> These sensor drivers for surface3 are both not upstream, but I made them
> as similar as possible to the upstreamed ones. So, I guess this issue
> can still be reproducible on some other devices.

I've run some test on my own surface3 and the problem is the writing
of 0x62 (which becomes just 0x02) to the 0x57 register of the PMIC,
writing 0x00 to that after loading the sensor driver makes things work
again.

I have not had time to investigate this further.

I used media-staging + your sensor drivers from:
https://github.com/kitakar5525/surface3-atomisp-cameras.git

Which was enough to figure this out, but I've not actually gotten
either of the cameras working :| I get:

[user@fedora nvt]$ ./atomisp-test.sh
p0: OPEN video device `/dev/video2'
p0: VIDIOC_S_INPUT <- 1
p0: ATOMISP_IOC_S_EXPOSURE integration_time={30000,30000} gain={30000,30000}
p0: ./v4l2n: ATOMISP_IOC_S_EXPOSURE failed on fd 3: Inappropriate ioctl for device (25)
p0: CLOSED video device

No matter which value I pass for VIDIOC_S_INPUT (tried 0 and 1) any ideas?

Perhaps I need to load the modules in a certain order, e.g. load the
sensor drivers before the main atomisp driver ?

Regards,

Hans





>
> I can't (easily) try touchscreen on mipad2 because it won't boot without
> nomodeset and somehow GNOME won't start if it's using nomodeset (on Arch
> Linux).
>
> On Mon, 2021-10-18 at 10:30 +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 10/17/21 18:23, Tsuchiya Yuto wrote:
>>> Touchscreen input works fine before loading atomisp driver on Surface 3.
>>>
>>> However, after loading atomisp driver, touchscreen works only when
>>> capturing images. This sounds like atomisp turns off something needed
>>> for touchscreen when atomisp is idle.
>>>
>>> There is no useful kernel log. Just the touchscreen stops working
>>> with no log.
>>>
>>> I'll update if I find something further. First of all, can someone
>>> reproduce this issue on the other devices?
>>
>> My first bet would be some regulator getting turned off.
>>
>> What you can do is:
>>
>> 1. ls -l /dev/bus/i2c/devices
>>
>> And then write down the number of the i2c-bus to which the
>> CRC PMIC is connected, lets say it is number "4". Then:
>>
>> 2. Before loading the atomisp drivers run:
>> "sudo i2cdump -y -f 4 0x6e > crc-regs-good"
>>
>> 3. After loading the atomisp drivers run:
>> "sudo i2cdump -y -f 4 0x6e > crc-regs-bad
>>
>> 4. "diff -u crc-regs-good crc-regs-bad"
>>
>> And see what changed.
>
> Here is the diff. The "good" one is before loading sensor driver, the
> "bad" one is from after loading sensor driver:
>
> $ diff -u crc-regs-good crc-regs-bad
> --- crc-regs-good 2021-10-21 18:04:57.853396866 +0900
> +++ crc-regs-bad 2021-10-21 18:06:13.755738054 +0900
> @@ -4,14 +4,14 @@
> 20: 00 00 01 c8 68 07 0a 10 10 01 1f 10 10 10 10 10 ..??h???????????
> 30: 10 10 00 20 21 20 20 20 20 20 00 2a 1c 1c 14 10 ??. ! .*????
> 40: 10 10 10 28 20 20 28 2e 2f 20 20 83 00 00 4c 00 ???( (./ ?..L.
> -50: 00 01 01 01 00 00 60 00 60 00 00 02 02 03 60 60 .???..`.`..???``
> +50: 00 01 01 01 00 00 60 02 60 00 00 02 02 62 60 60 .???..`?`..??b``
> 60: 60 01 03 00 00 00 00 00 00 00 30 04 00 00 00 00 `??.......0?....
> -70: 00 03 00 00 02 7b 02 6c 02 71 02 55 02 7c 02 9d .?..?{?l?q?U?|??
> +70: 00 03 00 00 02 7b 02 6d 02 73 02 58 02 7f 02 9e .?..?{?m?s?X????
> 80: 00 00 00 00 00 00 00 00 00 00 00 02 08 00 0b 02 ...........??.??
> 90: 3f 07 00 00 00 00 4c 00 4e 00 00 4c 00 23 01 b4 ??....L.N..L.#??
> a0: 4c 00 4e 00 00 3d ca 6a f0 00 00 3d ca 6a f0 00 L.N..=?j?..=?j?.
> b0: 00 7e 2a ff 02 04 06 00 00 00 00 00 00 20 00 00 .~*.???...... ..
> c0: 00 00 00 cd 08 00 00 4c 00 00 00 4c 00 00 00 3d ...??..L...L...=
> -d0: 97 00 00 3d 97 00 00 fe 17 00 ff 02 01 07 94 03 ?..=?..??..?????
> -e0: 9a 00 27 00 00 00 00 00 00 00 00 00 00 00 00 00 ?.'.............
> +d0: 97 00 00 3d 97 00 00 fe 17 00 ff 02 01 07 ec 03 ?..=?..??..?????
> +e0: 96 00 21 00 00 00 00 00 00 00 00 00 00 00 00 00 ?.!.............
> f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>
> Note that lines "70:" and "d0:"/"e0:" change over time. So, the actual
> change caused by loading sensor driver is line "50:"
>
> -50: 00 01 01 01 00 00 60 00 60 00 00 02 02 03 60 60 .???..`.`..???``
> +50: 00 01 01 01 00 00 60 02 60 00 00 02 02 62 60 60 .???..`?`..??b``
>
> and in atomisp_gmin_platform.c file,
>
> /* CRYSTAL COVE PMIC register set */
> #define CRYSTAL_1P8V_REG 0x57
> #define CRYSTAL_2P8V_REG 0x5d
> #define CRYSTAL_ON 0x63
> #define CRYSTAL_OFF 0x62
>
> indeed we're poking at 0x57 and 0x5d. So, maybe this issue is caused by
> regulators? I tried what would happen if I kept sensor power on before
> in sensor drivers, but there was no effect. But I feel I need to look
> into it again further.
>
>> There are 2 possible root causes here:
>>
>> 1. Some regulator is shared between the cameras and
>> touchscreen
>>
>> 2. The crc code in atomisp which you are using is
>> poking registers assuming the Bay Trail version of
>> the Crystal Cove PMIC (aka CRC PMIC) but your
>> Surface 3 has the Cherry Trail version and we know
>> that the regulators are add different register
>> addresses there, see the comment in:
>> drivers/acpi/pmic/intel_pmic_chtcrc.c
>> so it is possible that the atomisp code is simply
>> poking the wrong register for one of the regulators
>
> According to this Android kernel commit [1], the address for 1P8V and
> 2P8V are updated to the CRC+ ones (the upstreamed atomisp already has
> this change)
>
> [1] https://github.com/MiCode/Xiaomi_Kernel_OpenSource/commit/2f8221ba9a3770aed1ecfad2d04db61b95f30394
> ("update PMIC v1p8/v2p8 address")
>
>> I also wonder if this goes away if you do the
>>
>> hrv = 0x03;
>>
>> Hack inside drivers/mfd/intel_soc_pmic_core.c ?
>>
>> Without that we end up using the wrong PMIC
>> OpRegion driver which also uses the wrong
>> regulator addresses.
>
> I'm now using cht one with your patch, but the situation is the same
> as before.
>
> Regards,
> Tsuchiya Yuto
>
>> Regards,
>>
>> Hans
>>
>>
>> p.s.
>>
>> Here are the 2 different regulator drivers the
>> comment in drivers/acpi/pmic/intel_pmic_chtcrc.c
>> refers to:
>>
>> https://github.com/Zenfone2-Dev/android_kernel_asus_moorefield-1/blob/stock/drivers/regulator/pmic_crystal_cove.c
>> https://github.com/Zenfone2-Dev/android_kernel_asus_moorefield-1/blob/stock/drivers/regulator/pmic_crystal_cove_plus.c
>>
>
>


2021-11-08 11:26:53

by Mauro Carvalho Chehab

[permalink] [raw]
Subject: Re: [BUG 5/5] [BUG] media: atomisp: atomisp causes touchscreen to stop working on Microsoft Surface 3

Em Mon, 8 Nov 2021 00:39:38 +0100
Hans de Goede <[email protected]> escreveu:

> Hi,
>
> On 10/21/21 11:52, Tsuchiya Yuto wrote:
> > Thank you for your comment :-)
> >
> > First, I need to correct what I said in the previous mail. I later found
> > that loading only "atomisp" (as well as its dependency,
> > atomisp_gmin_platform) does not cause this issue.
> >
> > What causes this issue is rather, loading sensor drivers (as well as its
> > dependency, atomisp_gmin_platform).
> >
> > These sensor drivers for surface3 are both not upstream, but I made them
> > as similar as possible to the upstreamed ones. So, I guess this issue
> > can still be reproducible on some other devices.
>
> I've run some test on my own surface3 and the problem is the writing
> of 0x62 (which becomes just 0x02) to the 0x57 register of the PMIC,
> writing 0x00 to that after loading the sensor driver makes things work
> again.
>
> I have not had time to investigate this further.
>
> I used media-staging + your sensor drivers from:
> https://github.com/kitakar5525/surface3-atomisp-cameras.git
>
> Which was enough to figure this out, but I've not actually gotten
> either of the cameras working :| I get:
>
> [user@fedora nvt]$ ./atomisp-test.sh
> p0: OPEN video device `/dev/video2'

After the patch that moved the output preview to be the first one,
you should probably use /dev/video0 here:


$ v4l2-ctl -D -d /dev/video0|grep Name
Name : ATOMISP ISP PREVIEW output

$ v4l2-ctl -D -d /dev/video2|grep Name
Name : ATOMISP ISP VIEWFINDER output


On Asus T101HA, I'm also getting this if I use /dev/video2 with nvt:

ioctl(3, _IOC(_IOC_WRITE, 0x76, 0xe0, 0x1f0), 0x7ffcf08fe030) = -1 EINVAL (Argumento inválido)
p0: ./v4l2n: ATOMISP_IOC_S_PARAMETERS failed on fd 3: Invalid argument (22)
p0: CLOSED video device

Regards,
Mauro

2021-11-09 04:15:15

by Tsuchiya Yuto

[permalink] [raw]
Subject: Re: [BUG 5/5] [BUG] media: atomisp: atomisp causes touchscreen to stop working on Microsoft Surface 3

On Mon, 2021-11-08 at 00:39 +0100, Hans de Goede wrote:
> Hi,
>
> On 10/21/21 11:52, Tsuchiya Yuto wrote:
> > Thank you for your comment :-)
> >
> > First, I need to correct what I said in the previous mail. I later found
> > that loading only "atomisp" (as well as its dependency,
> > atomisp_gmin_platform) does not cause this issue.
> >
> > What causes this issue is rather, loading sensor drivers (as well as its
> > dependency, atomisp_gmin_platform).
> >
> > These sensor drivers for surface3 are both not upstream, but I made them
> > as similar as possible to the upstreamed ones. So, I guess this issue
> > can still be reproducible on some other devices.
>
> I've run some test on my own surface3 and the problem is the writing
> of 0x62 (which becomes just 0x02) to the 0x57 register of the PMIC,
> writing 0x00 to that after loading the sensor driver makes things work
> again.
>
> I have not had time to investigate this further.
>
> I used media-staging + your sensor drivers from:
> https://github.com/kitakar5525/surface3-atomisp-cameras.git
>
> Which was enough to figure this out, but I've not actually gotten
> either of the cameras working :| I get:
>
> [user@fedora nvt]$ ./atomisp-test.sh
> p0: OPEN video device `/dev/video2'
> p0: VIDIOC_S_INPUT <- 1
> p0: ATOMISP_IOC_S_EXPOSURE integration_time={30000,30000} gain={30000,30000}
> p0: ./v4l2n: ATOMISP_IOC_S_EXPOSURE failed on fd 3: Inappropriate ioctl for device (25)
> p0: CLOSED video device
>
> No matter which value I pass for VIDIOC_S_INPUT (tried 0 and 1) any ideas?

I also tried with the latest media-staging patches, and turned out that
somehow I need to revert this commit ("media: atomisp: fix VIDIOC_S_FMT
logic"). If you applied this patch, reverting this for now should make
the world-facing camera (ov8835) work.

Quick test revealed that upstreamed ov5693 driver is also affected this
(confirmed on mipad2) with the following log:

$ sudo dmesg -xw
kern :err : [ 840.165422] atomisp-isp2 0000:00:03.0: can't change power state from D3cold to D0 (config space inaccessible)
kern :warn : [ 840.171126] isys dma store at addr(0xcd408) val(0)
kern :info : [ 840.171890] ov5693_s_power: on 1
kern :info : [ 840.220418] ov5693_init
kern :warn : [ 840.321579] CPU: 3 PID: 3114 Comm: v4l2n Tainted: G WC OE 5.15.0-1-surface-mainline #4 a88d9b28206d4c7ef4fe4f41076a231501cdd2c8
kern :warn : [ 840.321613] Hardware name: Xiaomi Inc Mipad2/Mipad, BIOS MIPad-P4.X64.0043.R03.1603071414 03/07/2016
kern :warn : [ 840.321622] Call Trace:
kern :warn : [ 840.321641] dump_stack_lvl+0x46/0x62
kern :warn : [ 840.321678] ia_css_binary_find+0xa7d/0xd10 [atomisp f8192e88d06518afeafd868d132019e1c605ae55]
kern :warn : [ 840.321959] load_preview_binaries+0x41f/0x4d0 [atomisp f8192e88d06518afeafd868d132019e1c605ae55]
kern :warn : [ 840.322216] ia_css_stream_create+0xd98/0x17c0 [atomisp f8192e88d06518afeafd868d132019e1c605ae55]
kern :warn : [ 840.322467] __create_streams+0x264/0xd80 [atomisp f8192e88d06518afeafd868d132019e1c605ae55]
kern :warn : [ 840.322694] __get_frame_info+0xc0/0x320 [atomisp f8192e88d06518afeafd868d132019e1c605ae55]
kern :warn : [ 840.322931] ? atomisp_css_video_get_output_frame_info+0x80/0x80 [atomisp f8192e88d06518afeafd868d132019e1c605ae55]
kern :warn : [ 840.323157] atomisp_set_fmt+0x121c/0x1cc0 [atomisp f8192e88d06518afeafd868d132019e1c605ae55]
kern :warn : [ 840.323377] ? newidle_balance+0x138/0x430
kern :warn : [ 840.323396] ? atomisp_css_copy_get_output_frame_info+0x20/0x20 [atomisp f8192e88d06518afeafd868d132019e1c605ae55]
kern :warn : [ 840.323663] atomisp_s_fmt_cap+0x40/0x70 [atomisp f8192e88d06518afeafd868d132019e1c605ae55]
kern :warn : [ 840.323898] v4l_s_fmt+0x32a/0x5d0 [videodev dd91554b6b7a8a8394b47bdd48fb7420233a4650]
kern :warn : [ 840.324003] __video_do_ioctl+0x3c5/0x400 [videodev dd91554b6b7a8a8394b47bdd48fb7420233a4650]
kern :warn : [ 840.324108] video_usercopy+0x151/0x780 [videodev dd91554b6b7a8a8394b47bdd48fb7420233a4650]
kern :warn : [ 840.324205] ? v4l_print_control+0x20/0x20 [videodev dd91554b6b7a8a8394b47bdd48fb7420233a4650]
kern :warn : [ 840.324308] v4l2_ioctl+0x48/0x60 [videodev dd91554b6b7a8a8394b47bdd48fb7420233a4650]
kern :warn : [ 840.324401] __x64_sys_ioctl+0x8e/0xd0
kern :warn : [ 840.324426] do_syscall_64+0x5c/0x90
kern :warn : [ 840.324450] ? do_syscall_64+0x69/0x90
kern :warn : [ 840.324466] ? ksys_write+0x67/0xf0
kern :warn : [ 840.324485] ? syscall_exit_to_user_mode+0x23/0x50
kern :warn : [ 840.324502] ? do_syscall_64+0x69/0x90
kern :warn : [ 840.324519] ? exc_page_fault+0x72/0x180
kern :warn : [ 840.324533] entry_SYSCALL_64_after_hwframe+0x44/0xae
kern :warn : [ 840.324554] RIP: 0033:0x46c08b
kern :warn : [ 840.324572] Code: 5c c3 0f 1f 44 00 00 31 ff e8 91 7a 02 00 4c 8b 25 e2 30 0a 00 85 c0 79 90 eb ab 0f 1f 40 00 f3 0f 1e fa b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3
48 c7 c1 c0 ff ff ff f7 d8 64 89 01 48
kern :warn : [ 840.324584] RSP: 002b:00007ffcd87d0378 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
kern :warn : [ 840.324606] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 000000000046c08b
kern :warn : [ 840.324616] RDX: 00007ffcd87d03e0 RSI: ffffffffc0d05605 RDI: 0000000000000003
kern :warn : [ 840.324625] RBP: 00007ffcd87d03b0 R08: 0000000000000000 R09: 00007ffcd87d0100
kern :warn : [ 840.324635] R10: 000000003231564e R11: 0000000000000246 R12: 000000000040cef0
kern :warn : [ 840.324644] R13: 0000000000000000 R14: 00000000004e5018 R15: 0000000000400580
kern :warn : [ 840.324661] ? perf_trace_rdev_set_default_beacon_key+0x225/0x230 [cfg80211 0c5445915bd6781bf918218ab74f6ed610236fa6]
kern :err : [ 840.325998] atomisp-isp2 0000:00:03.0: can't create streams
kern :err : [ 840.326028] atomisp-isp2 0000:00:03.0: __get_frame_info 2560x1440 (padded to 0) returned -22
kern :warn : [ 840.326045] atomisp-isp2 0000:00:03.0: Can't set format on ISP. Error -22
kern :info : [ 840.326177] ov5693_s_power: on 0

# output from intel-nvt
$ ./v4l2n -o [email protected] \
--device /dev/video0 \
--input 0 \
--exposure=100000,100000,100000,100000 \
--parm type=1,capturemode=CI_MODE_PREVIEW \
--fmt type=1,width=1920,height=1080,pixelformat=NV12 \
--reqbufs count=2,memory=USERPTR \
--parameters=wb_config.r=32768,wb_config.gr=21043,wb_config.gb=21043,wb_config.b=30863 \
--capture=2 \

p0: OPEN video device `/dev/video0'
p0: VIDIOC_S_INPUT <- 0
p0: ATOMISP_IOC_S_EXPOSURE integration_time={100000,100000} gain={100000,100000}
p0: VIDIOC_S_PARM
p0: : type: VIDEO_CAPTURE [1]
p0: : capability: 0
p0: : capturemode: CI_MODE_PREVIEW [0x00008000]
p0: : timeperframe: 0/0
p0: : extendedmode: 0
p0: : readbuffers: 0
p0: VIDIOC_S_FMT
p0: ./v4l2n: VIDIOC_S_FMT failed on fd 3: Invalid argument (22)
p0: CLOSED video device

But somehow the world-facing camera (t4ka3) on mipad2 (which I ported
from Android kernel, non-upstream) is still working. So, I guess there
are issues on some sensor drivers side?

Mauro: do you know what could be a issue on sensor drivers? (especially
for the upstreamed ov5693)?

For the user-facing camera (ar0330), I haven't added this note anywhere,
exposure is not implemented yet. And in this case, if I try to set
exposure values using intel-nvt, atomisp stops working. Your above nvt
log shows this case. If you remove the `--exposure` option, you should
get a black image at least (yes, somehow not working yet).



Regards,
Tsuchiya Yuto