[BUG 0/5] bug reports for atomisp to make it work

[BUG 0/5] bug reports for atomisp to make it work

[Tsuchiya Yuto]

Hi all,

These mails contain RFC patches, which are almost bug report and some
are just bug report, for atomisp to work (again). Tested on Microsoft
Surface 3 (Windows) and Xiaomi Mi Pad 2 (Android model) with v5.15-rc5.
Both are Cherry Trail (ISP2401) devices.

I'm still not used to Linux patch sending flow. Sorry in advance
if there is some weirdness :-) but I did my best.

To try to take a picture, take a look at the series I sent earlier named
("various fixes for atomisp to make it work")

The 1st patch is required to take a picture with atomsip (again):

    [BUG][RFC] media: atomisp: pci: assume run_mode is PREVIEW

The 2nd patch is to avoid kernel warning message:

    [BUG][RFC] media: atomisp: pci: remove dummy_ptr NULL check to avoid
      duplicate active_bo

The 3rd patch is to avoid kernel oops, which is almost required for
using atomisp normally:

    [BUG][RFC] media: atomisp: pci: add NULL check for asd obtained from
      atomisp_video_pipe

The 4th-5th mail is bug reports, no patches for these issues yet:

    [BUG] media: atomisp: `modprobe -r` not working well (dup video4linux,
      ATOMISP_SUBDEV_{0,1})
    [BUG] media: atomisp: atomisp causes touchscreen to stop working on
      Microsoft Surface 3

I added further descriptions at the top of each RFC/BUG mails.

Regards,
Tsuchiya Yuto

Tsuchiya Yuto (5):
  [BUG][RFC] media: atomisp: pci: assume run_mode is PREVIEW
  [BUG][RFC] media: atomisp: pci: remove dummy_ptr NULL check to avoid
    duplicate active_bo
  [BUG][RFC] media: atomisp: pci: add NULL check for asd obtained from
    atomisp_video_pipe
  [BUG] media: atomisp: `modprobe -r` not working well (dup video4linux,
    ATOMISP_SUBDEV_{0,1})
  [BUG] media: atomisp: atomisp causes touchscreen to stop working on
    Microsoft Surface 3

 .../staging/media/atomisp/pci/atomisp_cmd.c   | 73 ++++++++++++++
 .../staging/media/atomisp/pci/atomisp_fops.c  |  6 ++
 .../staging/media/atomisp/pci/atomisp_ioctl.c | 96 ++++++++++++++++++-
 drivers/staging/media/atomisp/pci/hmm/hmm.c   |  4 -
 4 files changed, 174 insertions(+), 5 deletions(-)

-- 
2.33.1

[BUG/RFC PATCH 1/5] [BUG][RFC] media: atomisp: pci: assume run_mode is PREVIEW

[Tsuchiya Yuto]

This is almost a BUG report with a patch to just make atomisp work
again with the current mainline kernel. Thus, prefixed with [BUG][RFC].

RFC:
  1. When looking at the `case CI_MODE_NONE:`, it tries to do something.
     So, it should rather work with CI_MODE_NONE, too? Just I don't know
     how to configure userspace apps to use CI_MODE_NONE ?
  2. How can we re-add the run mode support again without relying on
     the s_parm ?

>8-----------------------------------------------------------------8<

After the commit 8a7c5594c020 ("media: v4l2-ioctl: clear fields in s_parm")
added on v4.18 (backported to v4.9.182 and v4.14.127), the capture mode
flag is cleared (except for V4L2_MODE_HIGHQUALITY).

Due to this, it seems that now we can't use this flag to set atomisp
run_mode. This results in capture not working with the following message
(loaded atomisp driver with dbg_level=1):

	kern  :warn  : [ 3658.776616] ia_css_pipe_get_info: ia_css_stream_create needs to be called before ia_css_[stream/pipe]_get_info
	kern  :err   : [ 3658.776641] atomisp-isp2 0000:00:03.0: get_frame_info 1920x1080 (padded to 0)
	kern  :warn  : [ 3658.776666] atomisp-isp2 0000:00:03.0: Can't set format on ISP. Error -22

So, when we can't detect run mode from the s_parm capturemode
(CI_MODE_NONE), let's assume the run mode is PREVIEW.

Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
---
 drivers/staging/media/atomisp/pci/atomisp_ioctl.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index c8a625667e81..6fc64f0ccc31 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -2638,7 +2638,11 @@ static int atomisp_s_parm(struct file *file, void *fh,
 				asd->high_speed_mode = true;
 		}
 
-		goto out;
+		dev_warn(isp->dev,
+			 "setting run_mode using s_parm capturemode is not supported anymore\n");
+		dev_warn(isp->dev, "assuming run_mode is PREVIEW\n");
+		mode = ATOMISP_RUN_MODE_PREVIEW;
+		break;
 	}
 	case CI_MODE_VIDEO:
 		mode = ATOMISP_RUN_MODE_VIDEO;
-- 
2.33.1

[BUG/RFC PATCH 2/5] [BUG][RFC] media: atomisp: pci: remove dummy_ptr NULL check to avoid duplicate active_bo

[Tsuchiya Yuto]

This is almost a BUG report with a patch to just avoid kernel error
message. Thus, prefixed with [BUG][RFC].

RFC:
  1. Regarding the NULL check in hmm_init(), it was added for the case
     when probe failed [1]. So, need to address that case with the other
     means? But I'm not sure how we can do for the case.
  2. Regarding the WARN_ON() in hmm_free(), there is a patch from Alan
     ("atomisp: hmm gives a bogus warning on unload") [1] but I haven't
     looked into it yet.

[1] https://lore.kernel.org/linux-media/151001141201.77201.10725942741811192730.stgit@alans-desktop/

>8-----------------------------------------------------------------8<

The NULL check for dummy_ptr in hmm_init() [1] results in the following
"hmm_init Failed to create sysfs" error exactly once every
two times on atomisp reload by rmmod/insmod (although atomisp module
loads and works fine regardless of this error):

	kern  :warn  : [  140.230662] sysfs: cannot create duplicate filename '/devices/pci0000:00/0000:00:03.0/active_bo'
	kern  :warn  : [  140.230668] CPU: 1 PID: 2502 Comm: insmod Tainted: G         C OE     5.15.0-rc4-1-surface-mainline #1 b8acf6eb64994414b2e20bad312a7a2c45f748f9
	kern  :warn  : [  140.230675] Hardware name: OEMB OEMB/OEMB, BIOS 1.51116.238 03/09/2015
	kern  :warn  : [  140.230678] Call Trace:
	kern  :warn  : [  140.230687]  dump_stack_lvl+0x46/0x5a
	kern  :warn  : [  140.230702]  sysfs_warn_dup.cold+0x17/0x24
	kern  :warn  : [  140.230710]  sysfs_add_file_mode_ns+0x160/0x170
	kern  :warn  : [  140.230717]  internal_create_group+0x126/0x390
	kern  :warn  : [  140.230723]  hmm_init+0x5c/0x70 [atomisp 7a6a680bf400629363d2a6f58fd10e7299678b99]
	kern  :warn  : [  140.230811]  atomisp_pci_probe.cold+0x1136/0x148e [atomisp 7a6a680bf400629363d2a6f58fd10e7299678b99]
	kern  :warn  : [  140.230875]  local_pci_probe+0x45/0x80
	kern  :warn  : [  140.230882]  ? pci_match_device+0xd7/0x130
	kern  :warn  : [  140.230887]  pci_device_probe+0xfa/0x1b0
	kern  :warn  : [  140.230892]  really_probe+0x1f5/0x3f0
	kern  :warn  : [  140.230899]  __driver_probe_device+0xfe/0x180
	kern  :warn  : [  140.230903]  driver_probe_device+0x1e/0x90
	kern  :warn  : [  140.230908]  __driver_attach+0xc0/0x1c0
	kern  :warn  : [  140.230912]  ? __device_attach_driver+0xe0/0xe0
	kern  :warn  : [  140.230915]  ? __device_attach_driver+0xe0/0xe0
	kern  :warn  : [  140.230919]  bus_for_each_dev+0x89/0xd0
	kern  :warn  : [  140.230924]  bus_add_driver+0x12b/0x1e0
	kern  :warn  : [  140.230929]  driver_register+0x8f/0xe0
	kern  :warn  : [  140.230933]  ? 0xffffffffc153f000
	kern  :warn  : [  140.230937]  do_one_initcall+0x57/0x220
	kern  :warn  : [  140.230945]  do_init_module+0x5c/0x260
	kern  :warn  : [  140.230952]  load_module+0x24bd/0x26a0
	kern  :warn  : [  140.230962]  ? __do_sys_finit_module+0xae/0x110
	kern  :warn  : [  140.230966]  __do_sys_finit_module+0xae/0x110
	kern  :warn  : [  140.230972]  do_syscall_64+0x5c/0x80
	kern  :warn  : [  140.230979]  ? syscall_exit_to_user_mode+0x23/0x40
	kern  :warn  : [  140.230983]  ? do_syscall_64+0x69/0x80
	kern  :warn  : [  140.230988]  ? exc_page_fault+0x72/0x170
	kern  :warn  : [  140.230991]  entry_SYSCALL_64_after_hwframe+0x44/0xae
	kern  :warn  : [  140.230997] RIP: 0033:0x7f7fd5d8718d
	kern  :warn  : [  140.231003] Code: b4 0c 00 0f 05 eb a9 66 0f 1f 44 00 00 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d b3 6c 0c 00 f7 d8 64 89 01 48
	kern  :warn  : [  140.231006] RSP: 002b:00007ffefc25f0e8 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
	kern  :warn  : [  140.231012] RAX: ffffffffffffffda RBX: 000055ac3edcd7f0 RCX: 00007f7fd5d8718d
	kern  :warn  : [  140.231015] RDX: 0000000000000000 RSI: 000055ac3d723270 RDI: 0000000000000003
	kern  :warn  : [  140.231017] RBP: 0000000000000000 R08: 0000000000000000 R09: 00007f7fd5e52380
	kern  :warn  : [  140.231019] R10: 0000000000000003 R11: 0000000000000246 R12: 000055ac3d723270
	kern  :warn  : [  140.231021] R13: 0000000000000000 R14: 000055ac3edd06e0 R15: 0000000000000000
	kern  :err   : [  140.231038] atomisp-isp2 0000:00:03.0: hmm_init Failed to create sysfs

So, remove the NULL check to always call sysfs_remove_group() and fix
the above error.

At this point, atomisp now gives WARN_ON() in hmm_free() [2] on atomisp
reload by rmmod/insmod. So, also remove it.

[1] added on commit
    d9ab83953fa7 ("media: atomisp: don't cause a warn if probe failed")
[2] added on commit
    b83cc378dfc4 ("atomisp: clean up the hmm init/cleanup indirections")

Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
---
 drivers/staging/media/atomisp/pci/hmm/hmm.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm.c b/drivers/staging/media/atomisp/pci/hmm/hmm.c
index 6a5ee4607089..47dc3df3b800 100644
--- a/drivers/staging/media/atomisp/pci/hmm/hmm.c
+++ b/drivers/staging/media/atomisp/pci/hmm/hmm.c
@@ -209,8 +209,6 @@ int hmm_init(void)
 
 void hmm_cleanup(void)
 {
-	if (!dummy_ptr)
-		return;
 	sysfs_remove_group(&atomisp_dev->kobj, atomisp_attribute_group);
 
 	/* free dummy memory first */
@@ -288,8 +286,6 @@ void hmm_free(ia_css_ptr virt)
 
 	dev_dbg(atomisp_dev, "%s: free 0x%08x\n", __func__, virt);
 
-	WARN_ON(!virt);
-
 	bo = hmm_bo_device_search_start(&bo_device, (unsigned int)virt);
 
 	if (!bo) {
-- 
2.33.1

[BUG/RFC PATCH 3/5] [BUG][RFC] media: atomisp: pci: add NULL check for asd obtained from atomisp_video_pipe

[Tsuchiya Yuto]

This is almost a BUG report with RFC patch that just avoids kernel
oopses. Thus, prefixed with [BUG][RFC].

Here is the kernel log after running `v4l2-compliance -d /dev/video4`
with this patch applied:

	kern  :err   : [25507.580392] atomisp-isp2 0000:00:03.0: can't change power state from D3cold to D0 (config space inaccessible)
	kern  :warn  : [25507.592343] isys dma store at addr(0xcd408) val(0)
	kern  :err   : [25507.592995] atomisp-isp2 0000:00:03.0: atomisp_queryctl(): asd is NULL, device is ATOMISP ISP ACC
	kern  :err   : [25507.593685] atomisp-isp2 0000:00:03.0: atomisp_g_input(): asd is NULL, device is ATOMISP ISP ACC
	kern  :err   : [25507.593719] atomisp-isp2 0000:00:03.0: atomisp_g_parm(): asd is NULL, device is ATOMISP ISP ACC
	kern  :err   : [25507.593727] atomisp-isp2 0000:00:03.0: atomisp_queryctl(): asd is NULL, device is ATOMISP ISP ACC
	[omitting 42 same messages]
	kern  :err   : [25507.593976] atomisp-isp2 0000:00:03.0: atomisp_queryctl(): asd is NULL, device is ATOMISP ISP ACC
	kern  :err   : [25507.594191] atomisp-isp2 0000:00:03.0: atomisp_g_input(): asd is NULL, device is ATOMISP ISP ACC
	kern  :err   : [25507.594449] atomisp-isp2 0000:00:03.0: atomisp_queryctl(): asd is NULL, device is ATOMISP ISP ACC
	[omitting 43 same messages]
	kern  :err   : [25507.594756] atomisp-isp2 0000:00:03.0: atomisp_queryctl(): asd is NULL, device is ATOMISP ISP ACC
	kern  :err   : [25507.594779] atomisp-isp2 0000:00:03.0: atomisp_g_ctrl(): asd is NULL, device is ATOMISP ISP ACC
	kern  :err   : [25507.594787] atomisp-isp2 0000:00:03.0: atomisp_s_ctrl(): asd is NULL, device is ATOMISP ISP ACC
	kern  :err   : [25507.594803] atomisp-isp2 0000:00:03.0: atomisp_camera_g_ext_ctrls(): asd is NULL, device is ATOMISP ISP ACC
	kern  :err   : [25507.594880] atomisp-isp2 0000:00:03.0: atomisp_enum_fmt_cap(): asd is NULL, device is ATOMISP ISP ACC
	kern  :err   : [25507.594915] atomisp-isp2 0000:00:03.0: atomisp_g_parm(): asd is NULL, device is ATOMISP ISP ACC
	kern  :err   : [25507.595058] atomisp-isp2 0000:00:03.0: atomisp_try_fmt(): asd is NULL, device is ATOMISP ISP ACC
	kern  :err   : [25507.595089] atomisp-isp2 0000:00:03.0: atomisp_set_fmt(): asd is NULL, device is ATOMISP ISP ACC
	kern  :err   : [25507.595124] atomisp-isp2 0000:00:03.0: atomisp_set_fmt(): asd is NULL, device is ATOMISP ISP ACC
	kern  :err   : [25507.595221] atomisp-isp2 0000:00:03.0: atomisp_set_fmt(): asd is NULL, device is ATOMISP ISP ACC
	kern  :err   : [25507.595241] atomisp-isp2 0000:00:03.0: atomisp_set_fmt(): asd is NULL, device is ATOMISP ISP ACC
	kern  :err   : [25507.601571] atomisp-isp2 0000:00:03.0: can't change power state from D3cold to D0 (config space inaccessible)
	kern  :warn  : [25507.607496] isys dma store at addr(0xcd408) val(0)
	kern  :err   : [25507.608604] atomisp-isp2 0000:00:03.0: atomisp_queryctl(): asd is NULL, device is ATOMISP ISP ACC
	kern  :err   : [25507.611988] atomisp-isp2 0000:00:03.0: can't change power state from D3cold to D0 (config space inaccessible)
	kern  :warn  : [25507.617420] isys dma store at addr(0xcd408) val(0)
	kern  :err   : [25507.618429] atomisp-isp2 0000:00:03.0: atomisp_queryctl(): asd is NULL, device is ATOMISP ISP ACC
	kern  :err   : [25507.618811] atomisp-isp2 0000:00:03.0: atomisp_g_parm(): asd is NULL, device is ATOMISP ISP ACC
	kern  :err   : [25507.622193] atomisp-isp2 0000:00:03.0: can't change power state from D3cold to D0 (config space inaccessible)
	kern  :warn  : [25507.627355] isys dma store at addr(0xcd408) val(0)
	kern  :err   : [25507.628391] atomisp-isp2 0000:00:03.0: atomisp_queryctl(): asd is NULL, device is ATOMISP ISP ACC
	kern  :err   : [25507.631143] atomisp-isp2 0000:00:03.0: can't change power state from D3cold to D0 (config space inaccessible)
	kern  :warn  : [25507.635813] isys dma store at addr(0xcd408) val(0)
	kern  :err   : [25507.636489] atomisp-isp2 0000:00:03.0: atomisp_queryctl(): asd is NULL, device is ATOMISP ISP ACC
	kern  :err   : [25507.636504] atomisp-isp2 0000:00:03.0: atomisp_s_input(): asd is NULL, device is ATOMISP ISP ACC
	kern  :err   : [25507.636516] atomisp-isp2 0000:00:03.0: atomisp_set_fmt(): asd is NULL, device is ATOMISP ISP ACC
	kern  :err   : [25507.639111] atomisp-isp2 0000:00:03.0: can't change power state from D3cold to D0 (config space inaccessible)
	kern  :warn  : [25507.646152] isys dma store at addr(0xcd408) val(0)
	kern  :err   : [25507.646831] atomisp-isp2 0000:00:03.0: atomisp_queryctl(): asd is NULL, device is ATOMISP ISP ACC
	kern  :err   : [25507.646847] atomisp-isp2 0000:00:03.0: atomisp_s_input(): asd is NULL, device is ATOMISP ISP ACC
	kern  :err   : [25507.650079] atomisp-isp2 0000:00:03.0: can't change power state from D3cold to D0 (config space inaccessible)
	kern  :warn  : [25507.657476] isys dma store at addr(0xcd408) val(0)
	kern  :err   : [25507.658741] atomisp-isp2 0000:00:03.0: atomisp_queryctl(): asd is NULL, device is ATOMISP ISP ACC
	kern  :err   : [25507.658759] atomisp-isp2 0000:00:03.0: atomisp_s_input(): asd is NULL, device is ATOMISP ISP ACC
	kern  :err   : [25507.658771] atomisp-isp2 0000:00:03.0: atomisp_set_fmt(): asd is NULL, device is ATOMISP ISP ACC
	kern  :err   : [25507.660959] atomisp-isp2 0000:00:03.0: can't change power state from D3cold to D0 (config space inaccessible)
	kern  :warn  : [25507.666665] isys dma store at addr(0xcd408) val(0)
	kern  :err   : [25507.667397] atomisp-isp2 0000:00:03.0: atomisp_queryctl(): asd is NULL, device is ATOMISP ISP ACC

RFC:
  We should rather look into why the asd is NULL when the opened device
  is ACC node (video4/video10), instead of just avoiding NULL ptr derefs?

>8-----------------------------------------------------------------8<

Currently, NULL pointer dereference can occur on running some userspace
apps such as v4l2-compliance, gst-plugin-scanner, guvcview, cheese, etc.

Some debugging revealed that variable asd obtained from atomisp_video_pipe
(e.g., atomisp_to_video_pipe(vdev)->asd) causes this issue.

More precisely, this issue occurs when (and only when) apps try to do
its work for "ACC node"s, which are /dev/video4 and /dev/video10:

	$ grep . /sys/class/video4linux/video*/name
	/sys/class/video4linux/video0/name:ATOMISP ISP CAPTURE output
	/sys/class/video4linux/video10/name:ATOMISP ISP ACC
	/sys/class/video4linux/video1/name:ATOMISP ISP VIEWFINDER output
	/sys/class/video4linux/video2/name:ATOMISP ISP PREVIEW output
	/sys/class/video4linux/video3/name:ATOMISP ISP VIDEO output
	/sys/class/video4linux/video4/name:ATOMISP ISP ACC
	/sys/class/video4linux/video5/name:ATOMISP ISP MEMORY input
	/sys/class/video4linux/video6/name:ATOMISP ISP CAPTURE output
	/sys/class/video4linux/video7/name:ATOMISP ISP VIEWFINDER output
	/sys/class/video4linux/video8/name:ATOMISP ISP PREVIEW output
	/sys/class/video4linux/video9/name:ATOMISP ISP VIDEO output

For example, here are the commands to reproduce this issue with
v4l2-compliance for device video4 or video10 (ACC node):

        v4l2-compliance -d /dev/video4
        # or
        v4l2-compliance -d /dev/video10

You may also reproduce this issue when you just log in to a GNOME desktop.
On my setup, someone calls gst-plugin-scanner app when logging in to the
desktop and it causes this issue.

The other examples are, just open camera apps such as guvcview, cheese,
etc.

So, add NULL checks for asd obtained from `atomisp_to_video_pipe(vdev)->asd`
to avoid kernel NULL pointer dereference.

In some parts, `pipe->isp` is used instead of `pipe->asd->isp` to avoid
using the asd which is potentially NULL.

Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
---
 .../staging/media/atomisp/pci/atomisp_cmd.c   | 73 +++++++++++++++
 .../staging/media/atomisp/pci/atomisp_fops.c  |  6 ++
 .../staging/media/atomisp/pci/atomisp_ioctl.c | 90 +++++++++++++++++++
 3 files changed, 169 insertions(+)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
index 366161cff560..7206d29ba263 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -1715,6 +1715,12 @@ void atomisp_wdt_refresh_pipe(struct atomisp_video_pipe *pipe,
 {
 	unsigned long next;
 
+	if(!pipe->asd) {
+		dev_err(pipe->isp->dev, "%s(): asd is NULL, device is %s\n",
+			__func__, pipe->vdev.name);
+		return;
+	}
+
 	if (delay != ATOMISP_WDT_KEEP_CURRENT_DELAY)
 		pipe->wdt_duration = delay;
 
@@ -1777,6 +1783,12 @@ void atomisp_wdt_refresh(struct atomisp_sub_device *asd, unsigned int delay)
 /* ISP2401 */
 void atomisp_wdt_stop_pipe(struct atomisp_video_pipe *pipe, bool sync)
 {
+	if(!pipe->asd) {
+		dev_err(pipe->isp->dev, "%s(): asd is NULL, device is %s\n",
+			__func__, pipe->vdev.name);
+		return;
+	}
+
 	if (!atomisp_is_wdt_running(pipe))
 		return;
 
@@ -4109,6 +4121,12 @@ void atomisp_handle_parameter_and_buffer(struct atomisp_video_pipe *pipe)
 	unsigned long irqflags;
 	bool need_to_enqueue_buffer = false;
 
+	if(!asd) {
+		dev_err(pipe->isp->dev, "%s(): asd is NULL, device is %s\n",
+			__func__, pipe->vdev.name);
+		return;
+	}
+
 	if (atomisp_is_vf_pipe(pipe))
 		return;
 
@@ -4196,6 +4214,12 @@ int atomisp_set_parameters(struct video_device *vdev,
 	struct atomisp_css_params *css_param = &asd->params.css_param;
 	int ret;
 
+	if(!asd) {
+		dev_err(pipe->isp->dev, "%s(): asd is NULL, device is %s\n",
+			__func__, vdev->name);
+		return -EINVAL;
+	}
+
 	if (!asd->stream_env[ATOMISP_INPUT_STREAM_GENERAL].stream) {
 		dev_err(asd->isp->dev, "%s: internal error!\n", __func__);
 		return -EINVAL;
@@ -4857,6 +4881,12 @@ int atomisp_try_fmt(struct video_device *vdev, struct v4l2_pix_format *f,
 	int source_pad = atomisp_subdev_source_pad(vdev);
 	int ret;
 
+	if(!asd) {
+		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
+			__func__, vdev->name);
+		return -EINVAL;
+	}
+
 	if (!isp->inputs[asd->input_curr].camera)
 		return -EINVAL;
 
@@ -5198,6 +5228,12 @@ static int atomisp_set_fmt_to_isp(struct video_device *vdev,
 	const struct atomisp_in_fmt_conv *fc;
 	int ret, i;
 
+	if(!asd) {
+		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
+			__func__, vdev->name);
+		return -EINVAL;
+	}
+
 	v4l2_fh_init(&fh.vfh, vdev);
 
 	isp_sink_crop = atomisp_subdev_get_rect(
@@ -5494,6 +5530,7 @@ static int atomisp_set_fmt_to_snr(struct video_device *vdev,
 				  unsigned int dvs_env_w, unsigned int dvs_env_h)
 {
 	struct atomisp_sub_device *asd = atomisp_to_video_pipe(vdev)->asd;
+	struct atomisp_video_pipe *pipe = atomisp_to_video_pipe(vdev);
 	const struct atomisp_format_bridge *format;
 	struct v4l2_subdev_pad_config pad_cfg;
 	struct v4l2_subdev_state pad_state = {
@@ -5512,6 +5549,12 @@ static int atomisp_set_fmt_to_snr(struct video_device *vdev,
 	struct v4l2_subdev_fh fh;
 	int ret;
 
+	if(!asd) {
+		dev_err(pipe->isp->dev, "%s(): asd is NULL, device is %s\n",
+			__func__, vdev->name);
+		return -EINVAL;
+	}
+
 	v4l2_fh_init(&fh.vfh, vdev);
 
 	stream_index = atomisp_source_pad_to_stream_id(asd, source_pad);
@@ -5602,6 +5645,12 @@ int atomisp_set_fmt(struct video_device *vdev, struct v4l2_format *f)
 	struct v4l2_subdev_fh fh;
 	int ret;
 
+	if(!asd) {
+		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
+			__func__, vdev->name);
+		return -EINVAL;
+	}
+
 	if (source_pad >= ATOMISP_SUBDEV_PADS_NUM)
 		return -EINVAL;
 
@@ -6034,6 +6083,12 @@ int atomisp_set_fmt_file(struct video_device *vdev, struct v4l2_format *f)
 	struct v4l2_subdev_fh fh;
 	int ret;
 
+	if(!asd) {
+		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
+			__func__, vdev->name);
+		return -EINVAL;
+	}
+
 	v4l2_fh_init(&fh.vfh, vdev);
 
 	dev_dbg(isp->dev, "setting fmt %ux%u 0x%x for file inject\n",
@@ -6359,6 +6414,12 @@ bool atomisp_is_vf_pipe(struct atomisp_video_pipe *pipe)
 {
 	struct atomisp_sub_device *asd = pipe->asd;
 
+	if(!asd) {
+		dev_err(pipe->isp->dev, "%s(): asd is NULL, device is %s\n",
+			__func__, pipe->vdev.name);
+		return false;
+	}
+
 	if (pipe == &asd->video_out_vf)
 		return true;
 
@@ -6572,6 +6633,12 @@ static int atomisp_get_pipe_id(struct atomisp_video_pipe *pipe)
 {
 	struct atomisp_sub_device *asd = pipe->asd;
 
+	if(!asd) {
+		dev_err(pipe->isp->dev, "%s(): asd is NULL, device is %s\n",
+			__func__, pipe->vdev.name);
+		return -EINVAL;
+	}
+
 	if (ATOMISP_USE_YUVPP(asd)) {
 		return IA_CSS_PIPE_ID_YUVPP;
 	} else if (asd->vfpp->val == ATOMISP_VFPP_DISABLE_SCALER) {
@@ -6609,6 +6676,12 @@ int atomisp_get_invalid_frame_num(struct video_device *vdev,
 	struct ia_css_pipe_info p_info;
 	int ret;
 
+	if(!asd) {
+		dev_err(pipe->isp->dev, "%s(): asd is NULL, device is %s\n",
+			__func__, vdev->name);
+		return -EINVAL;
+	}
+
 	if (asd->isp->inputs[asd->input_curr].camera_caps->
 	    sensor[asd->sensor_curr].stream_num > 1) {
 		/* External ISP */
diff --git a/drivers/staging/media/atomisp/pci/atomisp_fops.c b/drivers/staging/media/atomisp/pci/atomisp_fops.c
index 62535e0f4429..e5cc93b39299 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_fops.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_fops.c
@@ -1183,6 +1183,12 @@ static int atomisp_mmap(struct file *file, struct vm_area_struct *vma)
 	u32 origin_size, new_size;
 	int ret;
 
+	if(!asd) {
+		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
+			__func__, vdev->name);
+		return -EINVAL;
+	}
+
 	if (!(vma->vm_flags & (VM_WRITE | VM_READ)))
 		return -EACCES;
 
diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index 6fc64f0ccc31..01c6eda2c3cc 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -646,6 +646,12 @@ static int atomisp_g_input(struct file *file, void *fh, unsigned int *input)
 	struct atomisp_device *isp = video_get_drvdata(vdev);
 	struct atomisp_sub_device *asd = atomisp_to_video_pipe(vdev)->asd;
 
+	if(!asd) {
+		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
+			__func__, vdev->name);
+		return -EINVAL;
+	}
+
 	rt_mutex_lock(&isp->mutex);
 	*input = asd->input_curr;
 	rt_mutex_unlock(&isp->mutex);
@@ -665,6 +671,12 @@ static int atomisp_s_input(struct file *file, void *fh, unsigned int input)
 	struct v4l2_subdev *motor;
 	int ret;
 
+	if(!asd) {
+		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
+			__func__, vdev->name);
+		return -EINVAL;
+	}
+
 	rt_mutex_lock(&isp->mutex);
 	if (input >= ATOM_ISP_MAX_INPUTS || input >= isp->input_cnt) {
 		dev_dbg(isp->dev, "input_cnt: %d\n", isp->input_cnt);
@@ -765,6 +777,12 @@ static int atomisp_enum_fmt_cap(struct file *file, void *fh,
 	unsigned int i, fi = 0;
 	int rval;
 
+	if(!asd) {
+		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
+			__func__, vdev->name);
+		return -EINVAL;
+	}
+
 	rt_mutex_lock(&isp->mutex);
 	rval = v4l2_subdev_call(isp->inputs[asd->input_curr].camera, pad,
 				enum_mbus_code, NULL, &code);
@@ -1027,6 +1045,12 @@ int __atomisp_reqbufs(struct file *file, void *fh,
 	u16 stream_id = atomisp_source_pad_to_stream_id(asd, source_pad);
 	int ret = 0, i = 0;
 
+	if(!asd) {
+		dev_err(pipe->isp->dev, "%s(): asd is NULL, device is %s\n",
+			__func__, vdev->name);
+		return -EINVAL;
+	}
+
 	if (req->count == 0) {
 		mutex_lock(&pipe->capq.vb_lock);
 		if (!list_empty(&pipe->capq.stream))
@@ -1154,6 +1178,12 @@ static int atomisp_qbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 	u32 pgnr;
 	int ret = 0;
 
+	if(!asd) {
+		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
+			__func__, vdev->name);
+		return -EINVAL;
+	}
+
 	rt_mutex_lock(&isp->mutex);
 	if (isp->isp_fatal_error) {
 		ret = -EIO;
@@ -1389,6 +1419,12 @@ static int atomisp_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 	struct atomisp_device *isp = video_get_drvdata(vdev);
 	int ret = 0;
 
+	if(!asd) {
+		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
+			__func__, vdev->name);
+		return -EINVAL;
+	}
+
 	rt_mutex_lock(&isp->mutex);
 
 	if (isp->isp_fatal_error) {
@@ -1640,6 +1676,12 @@ static int atomisp_streamon(struct file *file, void *fh,
 	int ret = 0;
 	unsigned long irqflags;
 
+	if(!asd) {
+		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
+			__func__, vdev->name);
+		return -EINVAL;
+	}
+
 	dev_dbg(isp->dev, "Start stream on pad %d for asd%d\n",
 		atomisp_subdev_source_pad(vdev), asd->index);
 
@@ -1901,6 +1943,12 @@ int __atomisp_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
 	unsigned long flags;
 	bool first_streamoff = false;
 
+	if(!asd) {
+		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
+			__func__, vdev->name);
+		return -EINVAL;
+	}
+
 	dev_dbg(isp->dev, "Stop stream on pad %d for asd%d\n",
 		atomisp_subdev_source_pad(vdev), asd->index);
 
@@ -2150,6 +2198,12 @@ static int atomisp_g_ctrl(struct file *file, void *fh,
 	struct atomisp_device *isp = video_get_drvdata(vdev);
 	int i, ret = -EINVAL;
 
+	if(!asd) {
+		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
+			__func__, vdev->name);
+		return -EINVAL;
+	}
+
 	for (i = 0; i < ctrls_num; i++) {
 		if (ci_v4l2_controls[i].id == control->id) {
 			ret = 0;
@@ -2229,6 +2283,12 @@ static int atomisp_s_ctrl(struct file *file, void *fh,
 	struct atomisp_device *isp = video_get_drvdata(vdev);
 	int i, ret = -EINVAL;
 
+	if(!asd) {
+		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
+			__func__, vdev->name);
+		return -EINVAL;
+	}
+
 	for (i = 0; i < ctrls_num; i++) {
 		if (ci_v4l2_controls[i].id == control->id) {
 			ret = 0;
@@ -2310,6 +2370,12 @@ static int atomisp_queryctl(struct file *file, void *fh,
 	struct atomisp_sub_device *asd = atomisp_to_video_pipe(vdev)->asd;
 	struct atomisp_device *isp = video_get_drvdata(vdev);
 
+	if(!asd) {
+		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
+			__func__, vdev->name);
+		return -EINVAL;
+	}
+
 	switch (qc->id) {
 	case V4L2_CID_FOCUS_ABSOLUTE:
 	case V4L2_CID_FOCUS_RELATIVE:
@@ -2355,6 +2421,12 @@ static int atomisp_camera_g_ext_ctrls(struct file *file, void *fh,
 	int i;
 	int ret = 0;
 
+	if(!asd) {
+		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
+			__func__, vdev->name);
+		return -EINVAL;
+	}
+
 	if (!IS_ISP2401)
 		motor = isp->inputs[asd->input_curr].motor;
 	else
@@ -2466,6 +2538,12 @@ static int atomisp_camera_s_ext_ctrls(struct file *file, void *fh,
 	int i;
 	int ret = 0;
 
+	if(!asd) {
+		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
+			__func__, vdev->name);
+		return -EINVAL;
+	}
+
 	if (!IS_ISP2401)
 		motor = isp->inputs[asd->input_curr].motor;
 	else
@@ -2591,6 +2669,12 @@ static int atomisp_g_parm(struct file *file, void *fh,
 	struct atomisp_sub_device *asd = atomisp_to_video_pipe(vdev)->asd;
 	struct atomisp_device *isp = video_get_drvdata(vdev);
 
+	if(!asd) {
+		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
+			__func__, vdev->name);
+		return -EINVAL;
+	}
+
 	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) {
 		dev_err(isp->dev, "unsupported v4l2 buf type\n");
 		return -EINVAL;
@@ -2613,6 +2697,12 @@ static int atomisp_s_parm(struct file *file, void *fh,
 	int rval;
 	int fps;
 
+	if(!asd) {
+		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
+			__func__, vdev->name);
+		return -EINVAL;
+	}
+
 	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) {
 		dev_err(isp->dev, "unsupported v4l2 buf type\n");
 		return -EINVAL;
-- 
2.33.1

[BUG 4/5] [BUG] media: atomisp: `modprobe -r` not working well (dup video4linux, ATOMISP_SUBDEV_{0,1})

[Tsuchiya Yuto]

This is just a BUG report and I don't have any patch for this issue.
For now, I use rmmod/insmod to reload atomisp modules.

When I use `modprobe -r` to unload atomisp drivers like the following,

        $ sudo modprobe -r atomisp
        $ sudo modprobe -r atomisp-ar0330
        $ sudo modprobe -r atomisp-ov883x

it will fail to initialize on next load (with both modprobe and insmod).

Steps to reproduce:

  1. boot without denylisting any atomisp modules (or load modules using
     modprobe manually)
  2. unload drivers with `modprobe -r`
  3. try to load drivers (modprobe or insmod)
  4. probe fails saying "duplicate filename [...] video4linux"
     (log available below)

Note:

  1. `modprobe -r` works when booting with denylisting then insmod-ing
  2. rmmod always works without errors

Here is the dmesg log:

        kern  :warn  : [  150.326240] sysfs: cannot create duplicate filename '/devices/pci0000:00/0000:00:03.0/video4linux'
        kern  :warn  : [  150.326246] CPU: 1 PID: 2739 Comm: modprobe Tainted: G         C OE     5.15.0-rc3-1-surface-mainline #1 22942c127770b6216143942411a0f5916a743a60
        kern  :warn  : [  150.326257] Hardware name: OEMB OEMB/OEMB, BIOS 1.51116.238 03/09/2015
        kern  :warn  : [  150.326262] Call Trace:
        kern  :warn  : [  150.326276]  dump_stack_lvl+0x46/0x5a
        kern  :warn  : [  150.326292]  sysfs_warn_dup.cold+0x17/0x24
        kern  :warn  : [  150.326304]  sysfs_create_dir_ns+0xcc/0xe0
        kern  :warn  : [  150.326318]  kobject_add_internal+0xbd/0x2b0
        kern  :warn  : [  150.326330]  kobject_add+0x98/0xd0
        kern  :warn  : [  150.326338]  ? kmem_cache_alloc_trace+0x15c/0x2f0
        kern  :warn  : [  150.326352]  get_device_parent+0x15e/0x1c0
        kern  :warn  : [  150.326365]  device_add+0xd9/0x9a0
        kern  :warn  : [  150.326375]  __video_register_device+0x8ee/0x11f0 [videodev 2ea57a870000384ef8d27d4d70e6e51a68e3f718]
        kern  :warn  : [  150.326420]  atomisp_subdev_register_entities+0x83/0x220 [atomisp 752573cebc128314ca99d40a85c7801ade7015b0]
        kern  :warn  : [  150.326528]  atomisp_pci_probe.cold+0xec3/0x1492 [atomisp 752573cebc128314ca99d40a85c7801ade7015b0]
        kern  :warn  : [  150.326625]  local_pci_probe+0x45/0x80
        kern  :warn  : [  150.326634]  ? pci_match_device+0xd7/0x130
        kern  :warn  : [  150.326640]  pci_device_probe+0xfa/0x1b0
        kern  :warn  : [  150.326650]  really_probe+0x1f5/0x3f0
        kern  :warn  : [  150.326661]  __driver_probe_device+0xfe/0x180
        kern  :warn  : [  150.326667]  driver_probe_device+0x1e/0x90
        kern  :warn  : [  150.326674]  __driver_attach+0xc0/0x1c0
        kern  :warn  : [  150.326680]  ? __device_attach_driver+0xe0/0xe0
        kern  :warn  : [  150.326685]  ? __device_attach_driver+0xe0/0xe0
        kern  :warn  : [  150.326691]  bus_for_each_dev+0x89/0xd0
        kern  :warn  : [  150.326698]  bus_add_driver+0x12b/0x1e0
        kern  :warn  : [  150.326704]  driver_register+0x8f/0xe0
        kern  :warn  : [  150.326712]  ? 0xffffffffc09e6000
        kern  :warn  : [  150.326717]  do_one_initcall+0x57/0x220
        kern  :warn  : [  150.326733]  do_init_module+0x5c/0x260
        kern  :warn  : [  150.326745]  load_module+0x24bd/0x26a0
        kern  :warn  : [  150.326760]  ? __do_sys_finit_module+0xae/0x110
        kern  :warn  : [  150.326766]  __do_sys_finit_module+0xae/0x110
        kern  :warn  : [  150.326777]  do_syscall_64+0x5c/0x80
        kern  :warn  : [  150.326789]  ? __audit_syscall_exit+0x24d/0x2a0
        kern  :warn  : [  150.326796]  ? syscall_exit_to_user_mode+0x23/0x40
        kern  :warn  : [  150.326804]  ? do_syscall_64+0x69/0x80
        kern  :warn  : [  150.326810]  ? do_syscall_64+0x69/0x80
        kern  :warn  : [  150.326816]  entry_SYSCALL_64_after_hwframe+0x44/0xae
        kern  :warn  : [  150.326824] RIP: 0033:0x7f33b9de918d
        kern  :warn  : [  150.326832] Code: b4 0c 00 0f 05 eb a9 66 0f 1f 44 00 00 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d b3 6c 0c 00 f7 d8 64 89 01 48
        kern  :warn  : [  150.326836] RSP: 002b:00007fffce4b44a8 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
        kern  :warn  : [  150.326846] RAX: ffffffffffffffda RBX: 00005643ae549e10 RCX: 00007f33b9de918d
        kern  :warn  : [  150.326851] RDX: 0000000000000000 RSI: 00005643acc65270 RDI: 0000000000000005
        kern  :warn  : [  150.326854] RBP: 0000000000040000 R08: 0000000000000000 R09: 00005643ae549f20
        kern  :warn  : [  150.326858] R10: 0000000000000005 R11: 0000000000000246 R12: 00005643acc65270
        kern  :warn  : [  150.326861] R13: 0000000000000000 R14: 00005643ae549cc0 R15: 00005643ae549e10
        kern  :err   : [  150.326871] kobject_add_internal failed for video4linux with -EEXIST, don't try to register things with the same name in the same directory.
        kern  :err   : [  150.326878] videodev: __video_register_device: device_register failed
        kern  :err   : [  150.326903] atomisp-isp2 0000:00:03.0: atomisp_subdev_register_entities fail
        kern  :err   : [  150.326918] atomisp-isp2 0000:00:03.0: atomisp_register_entities failed (-17)
        kern  :warn  : [  150.328150] atomisp-isp2: probe of 0000:00:03.0 failed with error -17

Later, I found that after unloading atomisp module (by both
rmmod and `modprobe -r`), ATOMISP_SUBDEV_0 and ATOMISP_SUBDEV_1 were left
under /sys/class/video4linux directory:

        # list v4l device names
        $ grep . /sys/class/video4linux/*/name
        /sys/class/video4linux/v4l-subdev7/name:ATOMISP_SUBDEV_0
        /sys/class/video4linux/v4l-subdev8/name:ATOMISP_SUBDEV_1

These files will be added every time I reload atomisp module using
rmmod/insmod:

        /sys/class/video4linux/v4l-subdev7/name:ATOMISP_SUBDEV_0
        /sys/class/video4linux/v4l-subdev8/name:ATOMISP_SUBDEV_1
        /sys/class/video4linux/v4l-subdev9/name:ATOMISP_SUBDEV_0
        /sys/class/video4linux/v4l-subdev10/name:ATOMISP_SUBDEV_1

Note 1:
  Somehow I can't reproduce this issue on Android kernel (using
  Mi Pad 2) and my port of intel-aero atomisp [1].

[1] https://github.com/kitakar5525/linux-kernel/tree/mainline+aero_atomisp_wo_kapi_changes-2021-10-11

Note 2:
  Here is the complete reload script (using rmmod/insmod) I use on
  Surface 3:

	# reload atomisp camera modules
	# somehow `modprobe -r` does not work well
	sudo rmmod atomisp
	sudo rmmod atomisp-ar0330
	sudo rmmod atomisp-ov883x
	sudo rmmod atomisp_gmin_platform

	# load drivers needed for atomisp first for insmod
	# for sensor drivers
	sudo modprobe media # needed for older LTS
	sudo modprobe videodev
	sudo modprobe v4l2_common # needed for older LTS
	sudo modprobe v4l2_async # if using async_register
	# for atomisp pci driver
	sudo modprobe videobuf-core
	sudo modprobe videobuf-vmalloc

	# insmod upstreamed atomisp
	sudo insmod upst/atomisp_gmin_platform.ko
	sudo insmod upst/surface3/atomisp-ar0330.ko
	sudo insmod upst/surface3/atomisp-ov883x.ko
	sudo insmod upst/atomisp.ko dbg_level=1 #dyndbg
-- 
2.33.1

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

[Tsuchiya Yuto]

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?
-- 
2.33.1

Re: [BUG 0/5] bug reports for atomisp to make it work

[Hans de Goede]

Hi All,

Just adding some folks to the Cc.

Regards,

Hans

On 10/17/21 18:23, Tsuchiya Yuto wrote:
> Hi all,
> 
> These mails contain RFC patches, which are almost bug report and some
> are just bug report, for atomisp to work (again). Tested on Microsoft
> Surface 3 (Windows) and Xiaomi Mi Pad 2 (Android model) with v5.15-rc5.
> Both are Cherry Trail (ISP2401) devices.
> 
> I'm still not used to Linux patch sending flow. Sorry in advance
> if there is some weirdness :-) but I did my best.
> 
> To try to take a picture, take a look at the series I sent earlier named
> ("various fixes for atomisp to make it work")
> 
> The 1st patch is required to take a picture with atomsip (again):
> 
>     [BUG][RFC] media: atomisp: pci: assume run_mode is PREVIEW
> 
> The 2nd patch is to avoid kernel warning message:
> 
>     [BUG][RFC] media: atomisp: pci: remove dummy_ptr NULL check to avoid
>       duplicate active_bo
> 
> The 3rd patch is to avoid kernel oops, which is almost required for
> using atomisp normally:
> 
>     [BUG][RFC] media: atomisp: pci: add NULL check for asd obtained from
>       atomisp_video_pipe
> 
> The 4th-5th mail is bug reports, no patches for these issues yet:
> 
>     [BUG] media: atomisp: `modprobe -r` not working well (dup video4linux,
>       ATOMISP_SUBDEV_{0,1})
>     [BUG] media: atomisp: atomisp causes touchscreen to stop working on
>       Microsoft Surface 3
> 
> I added further descriptions at the top of each RFC/BUG mails.
> 
> Regards,
> Tsuchiya Yuto
> 
> Tsuchiya Yuto (5):
>   [BUG][RFC] media: atomisp: pci: assume run_mode is PREVIEW
>   [BUG][RFC] media: atomisp: pci: remove dummy_ptr NULL check to avoid
>     duplicate active_bo
>   [BUG][RFC] media: atomisp: pci: add NULL check for asd obtained from
>     atomisp_video_pipe
>   [BUG] media: atomisp: `modprobe -r` not working well (dup video4linux,
>     ATOMISP_SUBDEV_{0,1})
>   [BUG] media: atomisp: atomisp causes touchscreen to stop working on
>     Microsoft Surface 3
> 
>  .../staging/media/atomisp/pci/atomisp_cmd.c   | 73 ++++++++++++++
>  .../staging/media/atomisp/pci/atomisp_fops.c  |  6 ++
>  .../staging/media/atomisp/pci/atomisp_ioctl.c | 96 ++++++++++++++++++-
>  drivers/staging/media/atomisp/pci/hmm/hmm.c   |  4 -
>  4 files changed, 174 insertions(+), 5 deletions(-)
> 

Re: [BUG 0/5] bug reports for atomisp to make it work

[Andy Shevchenko]

On Mon, Oct 18, 2021 at 10:51 AM Hans de Goede <hdegoede@redhat.com> wrote:

> Just adding some folks to the Cc.

A hint to the newly added folks, there is an archive of the Linux
kernel related emails located on lore.kernel.org. The quite famous
distros already have in their repositories the `b4` tool that helps to
access that archive. So, after installing that tool you may download
the whole thread as a mailbox or as a bundle ready for `git am` just
using the Message-ID value.

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

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.

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

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.

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

Re: [BUG 0/5] bug reports for atomisp to make it work

[Tsuchiya Yuto]

On Mon, 2021-10-18 at 11:10 +0300, Andy Shevchenko wrote:
> On Mon, Oct 18, 2021 at 10:51 AM Hans de Goede <hdegoede@redhat.com> wrote:
> 
> > Just adding some folks to the Cc.
> 
> A hint to the newly added folks, there is an archive of the Linux
> kernel related emails located on lore.kernel.org. The quite famous
> distros already have in their repositories the `b4` tool that helps to
> access that archive. So, after installing that tool you may download
> the whole thread as a mailbox or as a bundle ready for `git am` just
> using the Message-ID value.

Ah sorry, I failed to add people to Cc who I should have definitly
added.

And I might have sent all of the emails to people who I should not
by blindly using `scripts/get_maintainer.pl` for the first bug report
with `--cc-cmd`.

Sorry if that's the case for some of you...

Regards,
Tsuchiya Yuto

Re: [BUG 0/5] bug reports for atomisp to make it work

[Andy Shevchenko]

On Tue, Oct 19, 2021 at 09:58:20PM +0900, Tsuchiya Yuto wrote:
> On Mon, 2021-10-18 at 11:10 +0300, Andy Shevchenko wrote:
> > On Mon, Oct 18, 2021 at 10:51 AM Hans de Goede <hdegoede@redhat.com> wrote:
> > 
> > > Just adding some folks to the Cc.
> > 
> > A hint to the newly added folks, there is an archive of the Linux
> > kernel related emails located on lore.kernel.org. The quite famous
> > distros already have in their repositories the `b4` tool that helps to
> > access that archive. So, after installing that tool you may download
> > the whole thread as a mailbox or as a bundle ready for `git am` just
> > using the Message-ID value.
> 
> Ah sorry, I failed to add people to Cc who I should have definitly
> added.
> 
> And I might have sent all of the emails to people who I should not
> by blindly using `scripts/get_maintainer.pl` for the first bug report
> with `--cc-cmd`.

I have written a script [1] for myself which helps me a lot nowadays.
It tries to be smart enough to include all interested parties. Although,
user may expand the Cc and/or To lists.

[1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh

-- 
With Best Regards,
Andy Shevchenko

Re: [BUG 0/5] bug reports for atomisp to make it work

[Mauro Carvalho Chehab]

Em Mon, 18 Oct 2021 01:23:31 +0900
Tsuchiya Yuto <kitakar@gmail.com> escreveu:

> Hi all,
> 
> These mails contain RFC patches, which are almost bug report and some
> are just bug report, for atomisp to work (again). Tested on Microsoft
> Surface 3 (Windows) and Xiaomi Mi Pad 2 (Android model) with v5.15-rc5.
> Both are Cherry Trail (ISP2401) devices.

Before start looking at the patches, let me check if I got it right:

Should this series be applied after the series you sent earlier[1]?

[1] The series which starts with this one:
	 [PATCH 00/17] various fixes for atomisp to make it work


Regards,
Mauro

Re: [BUG 0/5] bug reports for atomisp to make it work

[Tsuchiya Yuto]

On Wed, 2021-10-20 at 07:48 +0100, Mauro Carvalho Chehab wrote:
> Em Mon, 18 Oct 2021 01:23:31 +0900
> Tsuchiya Yuto <kitakar@gmail.com> escreveu:
> 
> > Hi all,
> > 
> > These mails contain RFC patches, which are almost bug report and some
> > are just bug report, for atomisp to work (again). Tested on Microsoft
> > Surface 3 (Windows) and Xiaomi Mi Pad 2 (Android model) with v5.15-rc5.
> > Both are Cherry Trail (ISP2401) devices.
> 
> Before start looking at the patches, let me check if I got it right:
> 
> Should this series be applied after the series you sent earlier[1]?
> 
> [1] The series which starts with this one:
> 	 [PATCH 00/17] various fixes for atomisp to make it work

Hi, thank you for looking into this.

This series (RFC patches and BUG reports) does not depend on the series
I sent earlier ("various fixes for atomisp to make it work"). So, these
bugs here can be looked into anytime.

Especially, the bug that causes NULL pointer dereference on some setup
("[BUG/RFC PATCH 3/5] [BUG][RFC] media: atomisp: pci: add NULL check for
asd obtained from atomisp_video_pipe") is somewhat a critical issue and
it'd be great if it's fixed in a proper way as soon as possible.

Regards,
Tsuchiya Yuto

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

[Tsuchiya Yuto]

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 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
> 

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

[Hans de Goede]

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 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).

<note going a bit offtopic from atomisp here>

Friday I've resized the Android data partition on my Mi Pad 2 Android,
16G eMMC model and installed Fedora 35 in the free space.

And yesterday I've been poking at the Mi Pad 2 the entire day,
both under Fedora and under the original Android install to figure
out which chips there are and how they are used, etc. This has
diverted me from looking into atomisp2 stuff, but it was fun :)

I've also managed to make the i915 driver work. It still gives one
warning during boot which I need to look into. But it works now.
ATM my i915 fix is just a hack. I plan to turn it into something
which I hope I can get upstream, I'll Cc you (Tsuchiya) on the
upstream submission of the i915 submission.

I've also figured out all the other chips used in the Mi Pad 2
and I believe I should be able to get things battery monitoring
and switching the USB plug between host <-> device mode to work
without too much issues (but it will take some time). This is
all pretty similar to all the special handling which I've already
added to the kernel for the GPD win / pocket devices which also
use the CHT Whiskey Cove PMIC.

Here are my notes about all the non standard chips used in the
Mi Pad 2:

PMIC/charger/fuel-gauge:
-The Type-C connector is used as / wired up as a regular micro-USB connector

-There is a Cherry Trail Whiskey Cove PMIC on the I2C7 i2c_designware ctrl
 -This is used for ID pin detection
 -Charger-type detection does not work though, because the USB-2 data-lines are
  not connected to it
 -The 2 GPIOs which are used to enable an external V5 boost converter for
  Vbus resp Vconn on other designs are both configured as inputs (register value 0x18)
 -The extcon-intel-cht-wc driver should control the USB mux according to the
  ID pin, identically to how the extcon-axp288 code does this
 -The extcon-intel-cht-wc driver should control the Vboost converter in the
  bq25890 charger IC based on the ID pin 

-There is a bq25890 charger hanging from the CHT-WC PMIC charger I2C-bus at addr 0x6a
 -At boot the BIOS clears bit 4 of register 3, disabling charging so the device
  will still be powered from an external supply, but it will not charge!
  Linux needs to fix this up
 -This charger is connected to the USB-2 data-lines and automatically sets its
  input-current-limit based on the detected charger
 -Bit 5 of register 3 controls the Vboost converter for sending 5V to attached
  USB-devices this bit needs to be controller by Linux based on the ID pin
  detection from the PMIC. The BIOS does leave this enabled when booting with
  a USB-device plugged in.

-There is a BQ27520 fuel-gauge at address 0x55 of the I2C1 i2c_designware ctrl

I2C1: addr 0x55 BQ27520 fuel-gauge

I2C2: addr 0x0e unknown
I2C2: addr 0x1b Realtek 5659 codec ? (not detected by i2cdetect)
I2C2: addr 0x2c TI lp855x backlight controller:
 https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/latte-l-oss/drivers/video/backlight/lp855x_bl.c
I2C2: addr 0x34 NXP9890 audio amplifier
I2C2: addr 0x37 NXP9890 audio amplifier
I2C2: addr 0x3e unknown

I2C3: addr 0x30 KTD2026 RGB LED driver, controlling the status LED
 https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/latte-l-oss/drivers/leds/leds-ktd2026.c

I2C4: addr 0x36? OVTI5693 camera sensor
I2C4: addr 0x37 t3ka3 camera sensor

I2C5: addr 0x5a Motor DRV2604 Driver ? the tablet has no haptic feedback motor!
      Also nothing seen here by i2c-detect, probably bogus

I2C6: addr 0x38 FTSC touchscreen

I2C7: PMIC bus

-TPS61158: LED controller for menu keys LEDS, driven by PWM controller, max brightness
 80/255 !!!!
 https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/latte-l-oss/drivers/leds/leds-tps61158.c
 Android behavior: light up menu keys for 5 seconds on any human input:
 -Write a special HID driver for mainline linux to fix the key-events send by the
  touchscreen and to light up the keys for 5 seconds on any HID input reports

-Sensors (accel, als) through hid-ishtp

-Panel 1536x2048 on card0-DSI-1
 https://bugs.freedesktop.org/show_bug.cgi?id=108714

-DSDT: Android: OSID == 0x04, Windows OSID == 0x01

Regards,

Hans




> 
> 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
>>
> 
> 

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

[Tsuchiya Yuto]

On Sun, 2021-10-24 at 10:32 +0200, Hans de Goede wrote:
> Hi,
> 
> [...]
> 
> <note going a bit offtopic from atomisp here>
> 
> Friday I've resized the Android data partition on my Mi Pad 2 Android,
> 16G eMMC model and installed Fedora 35 in the free space.
> 
> And yesterday I've been poking at the Mi Pad 2 the entire day,
> both under Fedora and under the original Android install to figure
> out which chips there are and how they are used, etc. This has
> diverted me from looking into atomisp2 stuff, but it was fun :)
> 
> I've also managed to make the i915 driver work. It still gives one
> warning during boot which I need to look into. But it works now.
> ATM my i915 fix is just a hack. I plan to turn it into something
> which I hope I can get upstream, I'll Cc you (Tsuchiya) on the
> upstream submission of the i915 submission.

Thank you! I just tried your patch and now mipad2 can boot with GPU!
So, I tried if I can reproduce touchscreen issue with atomisp, but
there was no such issue. Touchscreen works regardless of atomisp drivers.
I guess this is maybe a PMIC difference (mipad2/wcove and surface3/ccove).

> I've also figured out all the other chips used in the Mi Pad 2
> and I believe I should be able to get things battery monitoring
> and switching the USB plug between host <-> device mode to work
> without too much issues (but it will take some time). This is
> all pretty similar to all the special handling which I've already
> added to the kernel for the GPD win / pocket devices which also
> use the CHT Whiskey Cove PMIC.

Thanks. I haven't looked into anything other than atomisp yet, so I
can't comment anything but it's really interesting to see how drivers
are developed :-)

> Here are my notes about all the non standard chips used in the
> Mi Pad 2:
> 
> PMIC/charger/fuel-gauge:
> -The Type-C connector is used as / wired up as a regular micro-USB connector
> 
> -There is a Cherry Trail Whiskey Cove PMIC on the I2C7 i2c_designware ctrl
>  -This is used for ID pin detection
>  -Charger-type detection does not work though, because the USB-2 data-lines are
>   not connected to it
>  -The 2 GPIOs which are used to enable an external V5 boost converter for
>   Vbus resp Vconn on other designs are both configured as inputs (register value 0x18)
>  -The extcon-intel-cht-wc driver should control the USB mux according to the
>   ID pin, identically to how the extcon-axp288 code does this
>  -The extcon-intel-cht-wc driver should control the Vboost converter in the
>   bq25890 charger IC based on the ID pin 
> 
> -There is a bq25890 charger hanging from the CHT-WC PMIC charger I2C-bus at addr 0x6a
>  -At boot the BIOS clears bit 4 of register 3, disabling charging so the device
>   will still be powered from an external supply, but it will not charge!
>   Linux needs to fix this up
>  -This charger is connected to the USB-2 data-lines and automatically sets its
>   input-current-limit based on the detected charger
>  -Bit 5 of register 3 controls the Vboost converter for sending 5V to attached
>   USB-devices this bit needs to be controller by Linux based on the ID pin
>   detection from the PMIC. The BIOS does leave this enabled when booting with
>   a USB-device plugged in.
> 
> -There is a BQ27520 fuel-gauge at address 0x55 of the I2C1 i2c_designware ctrl
> 
> I2C1: addr 0x55 BQ27520 fuel-gauge
> 
> I2C2: addr 0x0e unknown
> I2C2: addr 0x1b Realtek 5659 codec ? (not detected by i2cdetect)
> I2C2: addr 0x2c TI lp855x backlight controller:
>  https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/latte-l-oss/drivers/video/backlight/lp855x_bl.c
> I2C2: addr 0x34 NXP9890 audio amplifier
> I2C2: addr 0x37 NXP9890 audio amplifier
> I2C2: addr 0x3e unknown
> 
> I2C3: addr 0x30 KTD2026 RGB LED driver, controlling the status LED
>  https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/latte-l-oss/drivers/leds/leds-ktd2026.c
> 
> I2C4: addr 0x36? OVTI5693 camera sensor
> I2C4: addr 0x37 t3ka3 camera sensor
> 
> I2C5: addr 0x5a Motor DRV2604 Driver ? the tablet has no haptic feedback motor!
>       Also nothing seen here by i2c-detect, probably bogus

This must be a motor for the world-facing camera! I see "DW9761" in DSDT.
Currently, I have no idea if motors are working with the upstreamed
atomisp because there is no userspace driver for Linux that can use
motors.

Regards,
Tsuchiya Yuto

> I2C6: addr 0x38 FTSC touchscreen
> 
> I2C7: PMIC bus
> 
> -TPS61158: LED controller for menu keys LEDS, driven by PWM controller, max brightness
>  80/255 !!!!
>  https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/latte-l-oss/drivers/leds/leds-tps61158.c
>  Android behavior: light up menu keys for 5 seconds on any human input:
>  -Write a special HID driver for mainline linux to fix the key-events send by the
>   touchscreen and to light up the keys for 5 seconds on any HID input reports
> 
> -Sensors (accel, als) through hid-ishtp
> 
> -Panel 1536x2048 on card0-DSI-1
>  https://bugs.freedesktop.org/show_bug.cgi?id=108714
> 
> -DSDT: Android: OSID == 0x04, Windows OSID == 0x01
> 
> Regards,
> 
> Hans

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

[Hans de Goede]

Hi,

On 10/26/21 11:35, Tsuchiya Yuto wrote:
> On Sun, 2021-10-24 at 10:32 +0200, Hans de Goede wrote:
>> Hi,
>>
>> [...]
>>
>> <note going a bit offtopic from atomisp here>
>>
>> Friday I've resized the Android data partition on my Mi Pad 2 Android,
>> 16G eMMC model and installed Fedora 35 in the free space.
>>
>> And yesterday I've been poking at the Mi Pad 2 the entire day,
>> both under Fedora and under the original Android install to figure
>> out which chips there are and how they are used, etc. This has
>> diverted me from looking into atomisp2 stuff, but it was fun :)
>>
>> I've also managed to make the i915 driver work. It still gives one
>> warning during boot which I need to look into. But it works now.
>> ATM my i915 fix is just a hack. I plan to turn it into something
>> which I hope I can get upstream, I'll Cc you (Tsuchiya) on the
>> upstream submission of the i915 submission.
> 
> Thank you! I just tried your patch and now mipad2 can boot with GPU!

I'm happy to hear that it works for you to, is it ok if I add
a "Tested-by: Tsuchiya Yuto <kitakar@gmail.com>" to the next version
of the patch?

> So, I tried if I can reproduce touchscreen issue with atomisp, but
> there was no such issue. Touchscreen works regardless of atomisp drivers.
> I guess this is maybe a PMIC difference (mipad2/wcove and surface3/ccove).

Yeah, I actually bought a 2nd hand Surface 3 this weekend so that
I have the same platforms as you to test on as we work on improving
the atomisp2 driver. I've not run any atomisp tests on either device
yet, but I hope to get around to that soon. Then I should also be
able to see if I can reproduce the touchscreen issue and try to
debug it.

>> I've also figured out all the other chips used in the Mi Pad 2
>> and I believe I should be able to get things battery monitoring
>> and switching the USB plug between host <-> device mode to work
>> without too much issues (but it will take some time). This is
>> all pretty similar to all the special handling which I've already
>> added to the kernel for the GPD win / pocket devices which also
>> use the CHT Whiskey Cove PMIC.
> 
> Thanks. I haven't looked into anything other than atomisp yet, so I
> can't comment anything but it's really interesting to see how drivers
> are developed :-)
> 
>> Here are my notes about all the non standard chips used in the
>> Mi Pad 2:
>>
>> PMIC/charger/fuel-gauge:
>> -The Type-C connector is used as / wired up as a regular micro-USB connector
>>
>> -There is a Cherry Trail Whiskey Cove PMIC on the I2C7 i2c_designware ctrl
>>  -This is used for ID pin detection
>>  -Charger-type detection does not work though, because the USB-2 data-lines are
>>   not connected to it
>>  -The 2 GPIOs which are used to enable an external V5 boost converter for
>>   Vbus resp Vconn on other designs are both configured as inputs (register value 0x18)
>>  -The extcon-intel-cht-wc driver should control the USB mux according to the
>>   ID pin, identically to how the extcon-axp288 code does this
>>  -The extcon-intel-cht-wc driver should control the Vboost converter in the
>>   bq25890 charger IC based on the ID pin 
>>
>> -There is a bq25890 charger hanging from the CHT-WC PMIC charger I2C-bus at addr 0x6a
>>  -At boot the BIOS clears bit 4 of register 3, disabling charging so the device
>>   will still be powered from an external supply, but it will not charge!
>>   Linux needs to fix this up
>>  -This charger is connected to the USB-2 data-lines and automatically sets its
>>   input-current-limit based on the detected charger
>>  -Bit 5 of register 3 controls the Vboost converter for sending 5V to attached
>>   USB-devices this bit needs to be controller by Linux based on the ID pin
>>   detection from the PMIC. The BIOS does leave this enabled when booting with
>>   a USB-device plugged in.
>>
>> -There is a BQ27520 fuel-gauge at address 0x55 of the I2C1 i2c_designware ctrl
>>
>> I2C1: addr 0x55 BQ27520 fuel-gauge
>>
>> I2C2: addr 0x0e unknown
>> I2C2: addr 0x1b Realtek 5659 codec ? (not detected by i2cdetect)
>> I2C2: addr 0x2c TI lp855x backlight controller:
>>  https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/latte-l-oss/drivers/video/backlight/lp855x_bl.c
>> I2C2: addr 0x34 NXP9890 audio amplifier
>> I2C2: addr 0x37 NXP9890 audio amplifier
>> I2C2: addr 0x3e unknown
>>
>> I2C3: addr 0x30 KTD2026 RGB LED driver, controlling the status LED
>>  https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/latte-l-oss/drivers/leds/leds-ktd2026.c
>>
>> I2C4: addr 0x36? OVTI5693 camera sensor
>> I2C4: addr 0x37 t3ka3 camera sensor
>>
>> I2C5: addr 0x5a Motor DRV2604 Driver ? the tablet has no haptic feedback motor!
>>       Also nothing seen here by i2c-detect, probably bogus
> 
> This must be a motor for the world-facing camera! I see "DW9761" in DSDT.

Interesting I checked and the world facing camera indeed has
a variable focus length. But the VCM (Voice Coil Motor) for this likely
sits on the same bus as the sensors.

> Currently, I have no idea if motors are working with the upstreamed
> atomisp because there is no userspace driver for Linux that can use
> motors.

No the code attaching to the DRV2604 ACPI ID really is a motor driver
for haptic feedback, see:
https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/latte-l-oss/drivers/misc/tspdrv/ImmVibeSPI.c

(never mind the SPI in the name it is actually an I2C driver).

I also noticed that the driver is using an enable GPIO, but even
if I drive that high i2cdetect still does not see anything and
AFAIK the Mi Pad 2 does not have haptics / a buzzer motor.

Regards,

Hans


> 
> Regards,
> Tsuchiya Yuto
> 
>> I2C6: addr 0x38 FTSC touchscreen
>>
>> I2C7: PMIC bus
>>
>> -TPS61158: LED controller for menu keys LEDS, driven by PWM controller, max brightness
>>  80/255 !!!!
>>  https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/latte-l-oss/drivers/leds/leds-tps61158.c
>>  Android behavior: light up menu keys for 5 seconds on any human input:
>>  -Write a special HID driver for mainline linux to fix the key-events send by the
>>   touchscreen and to light up the keys for 5 seconds on any HID input reports
>>
>> -Sensors (accel, als) through hid-ishtp
>>
>> -Panel 1536x2048 on card0-DSI-1
>>  https://bugs.freedesktop.org/show_bug.cgi?id=108714
>>
>> -DSDT: Android: OSID == 0x04, Windows OSID == 0x01
>>
>> Regards,
>>
>> Hans
> 
> 

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

[Tsuchiya Yuto]

On Tue, 2021-10-26 at 17:41 +0200, Hans de Goede wrote:
> Hi,
> 
> On 10/26/21 11:35, Tsuchiya Yuto wrote:
> > On Sun, 2021-10-24 at 10:32 +0200, Hans de Goede wrote:
> > > Hi,
> > > 
> > > [...]
> > > 
> > > <note going a bit offtopic from atomisp here>
> > > 
> > > Friday I've resized the Android data partition on my Mi Pad 2 Android,
> > > 16G eMMC model and installed Fedora 35 in the free space.
> > > 
> > > And yesterday I've been poking at the Mi Pad 2 the entire day,
> > > both under Fedora and under the original Android install to figure
> > > out which chips there are and how they are used, etc. This has
> > > diverted me from looking into atomisp2 stuff, but it was fun :)
> > > 
> > > I've also managed to make the i915 driver work. It still gives one
> > > warning during boot which I need to look into. But it works now.
> > > ATM my i915 fix is just a hack. I plan to turn it into something
> > > which I hope I can get upstream, I'll Cc you (Tsuchiya) on the
> > > upstream submission of the i915 submission.
> > 
> > Thank you! I just tried your patch and now mipad2 can boot with GPU!
> 
> I'm happy to hear that it works for you to, is it ok if I add
> a "Tested-by: Tsuchiya Yuto <kitakar@gmail.com>" to the next version
> of the patch?

Yes, no problem.

> > So, I tried if I can reproduce touchscreen issue with atomisp, but
> > there was no such issue. Touchscreen works regardless of atomisp drivers.
> > I guess this is maybe a PMIC difference (mipad2/wcove and surface3/ccove).
> 
> Yeah, I actually bought a 2nd hand Surface 3 this weekend so that
> I have the same platforms as you to test on as we work on improving
> the atomisp2 driver. I've not run any atomisp tests on either device
> yet, but I hope to get around to that soon. Then I should also be
> able to see if I can reproduce the touchscreen issue and try to
> debug it.

Welcome, and really appreciate it :-)

> > > I've also figured out all the other chips used in the Mi Pad 2
> > > and I believe I should be able to get things battery monitoring
> > > and switching the USB plug between host <-> device mode to work
> > > without too much issues (but it will take some time). This is
> > > all pretty similar to all the special handling which I've already
> > > added to the kernel for the GPD win / pocket devices which also
> > > use the CHT Whiskey Cove PMIC.
> > 
> > Thanks. I haven't looked into anything other than atomisp yet, so I
> > can't comment anything but it's really interesting to see how drivers
> > are developed :-)
> > 
> > > Here are my notes about all the non standard chips used in the
> > > Mi Pad 2:
> > > 
> > > PMIC/charger/fuel-gauge:
> > > -The Type-C connector is used as / wired up as a regular micro-USB connector
> > > 
> > > -There is a Cherry Trail Whiskey Cove PMIC on the I2C7 i2c_designware ctrl
> > >  -This is used for ID pin detection
> > >  -Charger-type detection does not work though, because the USB-2 data-lines are
> > >   not connected to it
> > >  -The 2 GPIOs which are used to enable an external V5 boost converter for
> > >   Vbus resp Vconn on other designs are both configured as inputs (register value 0x18)
> > >  -The extcon-intel-cht-wc driver should control the USB mux according to the
> > >   ID pin, identically to how the extcon-axp288 code does this
> > >  -The extcon-intel-cht-wc driver should control the Vboost converter in the
> > >   bq25890 charger IC based on the ID pin 
> > > 
> > > -There is a bq25890 charger hanging from the CHT-WC PMIC charger I2C-bus at addr 0x6a
> > >  -At boot the BIOS clears bit 4 of register 3, disabling charging so the device
> > >   will still be powered from an external supply, but it will not charge!
> > >   Linux needs to fix this up
> > >  -This charger is connected to the USB-2 data-lines and automatically sets its
> > >   input-current-limit based on the detected charger
> > >  -Bit 5 of register 3 controls the Vboost converter for sending 5V to attached
> > >   USB-devices this bit needs to be controller by Linux based on the ID pin
> > >   detection from the PMIC. The BIOS does leave this enabled when booting with
> > >   a USB-device plugged in.
> > > 
> > > -There is a BQ27520 fuel-gauge at address 0x55 of the I2C1 i2c_designware ctrl
> > > 
> > > I2C1: addr 0x55 BQ27520 fuel-gauge
> > > 
> > > I2C2: addr 0x0e unknown
> > > I2C2: addr 0x1b Realtek 5659 codec ? (not detected by i2cdetect)
> > > I2C2: addr 0x2c TI lp855x backlight controller:
> > >  https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/latte-l-oss/drivers/video/backlight/lp855x_bl.c
> > > I2C2: addr 0x34 NXP9890 audio amplifier
> > > I2C2: addr 0x37 NXP9890 audio amplifier
> > > I2C2: addr 0x3e unknown
> > > 
> > > I2C3: addr 0x30 KTD2026 RGB LED driver, controlling the status LED
> > >  https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/latte-l-oss/drivers/leds/leds-ktd2026.c
> > > 
> > > I2C4: addr 0x36? OVTI5693 camera sensor
> > > I2C4: addr 0x37 t3ka3 camera sensor
> > > 
> > > I2C5: addr 0x5a Motor DRV2604 Driver ? the tablet has no haptic feedback motor!
> > >       Also nothing seen here by i2c-detect, probably bogus
> > 
> > This must be a motor for the world-facing camera! I see "DW9761" in DSDT.
> 
> Interesting I checked and the world facing camera indeed has
> a variable focus length. But the VCM (Voice Coil Motor) for this likely
> sits on the same bus as the sensors.
> 
> > Currently, I have no idea if motors are working with the upstreamed
> > atomisp because there is no userspace driver for Linux that can use
> > motors.
> 
> No the code attaching to the DRV2604 ACPI ID really is a motor driver
> for haptic feedback, see:
> https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/latte-l-oss/drivers/misc/tspdrv/ImmVibeSPI.c
> (never mind the SPI in the name it is actually an I2C driver).

Um, I spoke too soon. It may be possible that there is no such device
but just it's written in DSDT. There is one instance. On my mipad2, there
is no S5K4H8 camera but shows up anyway (when `OSID == 0x04`).

Regards,
Tsuchiya Yuto

> I also noticed that the driver is using an enable GPIO, but even
> if I drive that high i2cdetect still does not see anything and
> AFAIK the Mi Pad 2 does not have haptics / a buzzer motor.
> 
> Regards,
> 
> Hans
> 
> 
> > 
> > Regards,
> > Tsuchiya Yuto
> > 
> > > I2C6: addr 0x38 FTSC touchscreen
> > > 
> > > I2C7: PMIC bus
> > > 
> > > -TPS61158: LED controller for menu keys LEDS, driven by PWM controller, max brightness
> > >  80/255 !!!!
> > >  https://github.com/MiCode/Xiaomi_Kernel_OpenSource/blob/latte-l-oss/drivers/leds/leds-tps61158.c
> > >  Android behavior: light up menu keys for 5 seconds on any human input:
> > >  -Write a special HID driver for mainline linux to fix the key-events send by the
> > >   touchscreen and to light up the keys for 5 seconds on any HID input reports
> > > 
> > > -Sensors (accel, als) through hid-ishtp
> > > 
> > > -Panel 1536x2048 on card0-DSI-1
> > >  https://bugs.freedesktop.org/show_bug.cgi?id=108714
> > > 
> > > -DSDT: Android: OSID == 0x04, Windows OSID == 0x01
> > > 
> > > Regards,
> > > 
> > > Hans
> > 
> > 
> 

Re: [BUG/RFC PATCH 3/5] [BUG][RFC] media: atomisp: pci: add NULL check for asd obtained from atomisp_video_pipe

[Dan Carpenter]

On Mon, Oct 18, 2021 at 01:23:34AM +0900, Tsuchiya Yuto wrote:
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> index 366161cff560..7206d29ba263 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> @@ -1715,6 +1715,12 @@ void atomisp_wdt_refresh_pipe(struct atomisp_video_pipe *pipe,
>  {
>  	unsigned long next;
>  
> +	if(!pipe->asd) {

Run your patches through scripts/checkpatch.pl.

> +		dev_err(pipe->isp->dev, "%s(): asd is NULL, device is %s\n",
> +			__func__, pipe->vdev.name);
> +		return;
> +	}

regards,
dan carpenter

Re: [BUG/RFC PATCH 3/5] [BUG][RFC] media: atomisp: pci: add NULL check for asd obtained from atomisp_video_pipe

[Andy Shevchenko]

On Tue, Nov 2, 2021 at 3:10 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Mon, Oct 18, 2021 at 01:23:34AM +0900, Tsuchiya Yuto wrote:

> > +     if(!pipe->asd) {
>
> Run your patches through scripts/checkpatch.pl.

While it's good advice, we are dealing with quite a bad code under
staging, so the requirements may be relaxed.

> > +             dev_err(pipe->isp->dev, "%s(): asd is NULL, device is %s\n",
> > +                     __func__, pipe->vdev.name);
> > +             return;
> > +     }

-- 
With Best Regards,
Andy Shevchenko

Re: [BUG/RFC PATCH 3/5] [BUG][RFC] media: atomisp: pci: add NULL check for asd obtained from atomisp_video_pipe

[Andy Shevchenko]

On Tue, Nov 2, 2021 at 4:44 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Nov 2, 2021 at 3:10 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Mon, Oct 18, 2021 at 01:23:34AM +0900, Tsuchiya Yuto wrote:

...

> > Run your patches through scripts/checkpatch.pl.
>
> While it's good advice, we are dealing with quite a bad code under
> staging, so the requirements may be relaxed.

To be more clear: the goal now is getting it _working_. That's why
this kind of noise is not important _for now_.

-- 
With Best Regards,
Andy Shevchenko

Re: [BUG/RFC PATCH 3/5] [BUG][RFC] media: atomisp: pci: add NULL check for asd obtained from atomisp_video_pipe

[Dan Carpenter]

On Tue, Nov 02, 2021 at 04:45:20PM +0200, Andy Shevchenko wrote:
> On Tue, Nov 2, 2021 at 4:44 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Tue, Nov 2, 2021 at 3:10 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > On Mon, Oct 18, 2021 at 01:23:34AM +0900, Tsuchiya Yuto wrote:
> 
> ...
> 
> > > Run your patches through scripts/checkpatch.pl.
> >
> > While it's good advice, we are dealing with quite a bad code under
> > staging, so the requirements may be relaxed.
> 
> To be more clear: the goal now is getting it _working_. That's why
> this kind of noise is not important _for now_.

If it's a new driver, then we accept all sorts of garbage, that's true.

regards,
dan carpenter

Re: [BUG/RFC PATCH 3/5] [BUG][RFC] media: atomisp: pci: add NULL check for asd obtained from atomisp_video_pipe

[Andy Shevchenko]

On Tue, Nov 02, 2021 at 06:05:23PM +0300, Dan Carpenter wrote:
> On Tue, Nov 02, 2021 at 04:45:20PM +0200, Andy Shevchenko wrote:
> > On Tue, Nov 2, 2021 at 4:44 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Tue, Nov 2, 2021 at 3:10 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > On Mon, Oct 18, 2021 at 01:23:34AM +0900, Tsuchiya Yuto wrote:

...

> > > > Run your patches through scripts/checkpatch.pl.
> > >
> > > While it's good advice, we are dealing with quite a bad code under
> > > staging, so the requirements may be relaxed.
> > 
> > To be more clear: the goal now is getting it _working_. That's why
> > this kind of noise is not important _for now_.
> 
> If it's a new driver, then we accept all sorts of garbage, that's true.

It was in kernel for a while, but never worked (hence anyhow tested)
up to the recent effort made by Tsuchiya.

In any case, as I said, we shall run checkpatch in the future when
we have something working.

-- 
With Best Regards,
Andy Shevchenko

Re: [BUG/RFC PATCH 3/5] [BUG][RFC] media: atomisp: pci: add NULL check for asd obtained from atomisp_video_pipe

[Mauro Carvalho Chehab]

Em Tue, 2 Nov 2021 17:49:17 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> escreveu:

> On Tue, Nov 02, 2021 at 06:05:23PM +0300, Dan Carpenter wrote:
> > On Tue, Nov 02, 2021 at 04:45:20PM +0200, Andy Shevchenko wrote:  
> > > On Tue, Nov 2, 2021 at 4:44 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:  
> > > > On Tue, Nov 2, 2021 at 3:10 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:  
> > > > > On Mon, Oct 18, 2021 at 01:23:34AM +0900, Tsuchiya Yuto wrote:  
> 
> ...
> 
> > > > > Run your patches through scripts/checkpatch.pl.  
> > > >
> > > > While it's good advice, we are dealing with quite a bad code under
> > > > staging, so the requirements may be relaxed.  

FYI, I fixed the checkpatch issue when I applied at media_stage:

	https://git.linuxtv.org/media_stage.git/commit/?id=8a5457b7c7c3b6aa1789b18bbaff9b6a99d74caa

Ok, I could have instead replied to Tsuchiya instead, but, as Andy
pointed, those patches solved longstanding issues at the atomisp
driver. So, I just went ahead and cleaned up the issue ;-)

> > > 
> > > To be more clear: the goal now is getting it _working_. That's why
> > > this kind of noise is not important _for now_.  
> > 
> > If it's a new driver, then we accept all sorts of garbage, that's true.  
> 
> It was in kernel for a while, but never worked (hence anyhow tested)
> up to the recent effort made by Tsuchiya.
> 
> In any case, as I said, we shall run checkpatch in the future when
> we have something working.

Yeah, agreed. The best is to run checkpatch and save some time from
the maintainers.

In any case, as Andy pointed out, this driver still requires major
cleanups everywhere. Yet, our current focus is to make it work with
standard V4L2 apps.

Regards,
Mauro

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

[Hans de Goede]

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
>>
> 
> 

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

[Mauro Carvalho Chehab]

Em Mon, 8 Nov 2021 00:39:38 +0100
Hans de Goede <hdegoede@redhat.com> 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

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

[Hans de Goede]

Hi Mauro,

On 11/8/21 08:41, Mauro Carvalho Chehab wrote:
> Em Mon, 8 Nov 2021 00:39:38 +0100
> Hans de Goede <hdegoede@redhat.com> 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:

Thanks for the hint, but I've not rebased my tree to those latest couple
of patches yet, the same tree does work on the T101HA with /dev/video2 :)

I think this may be a module load ordering issue, I believe that the
sensor drivers need to be loaded before the atomisp driver itself
and on the T101HA we are hitting this "sweet spot", where as on
the surface I was loading the not yet merged sensor drivers manually,
causing them to be loaded later.

I still need to verify this theory, Tsuchiya can you perhaps confirm 
that the modules must be loaded in this order?

Regards,

Hans

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

[Mauro Carvalho Chehab]

Em Mon, 8 Nov 2021 08:55:53 +0100
Hans de Goede <hdegoede@redhat.com> escreveu:

> Hi Mauro,
> 
> On 11/8/21 08:41, Mauro Carvalho Chehab wrote:
> > Em Mon, 8 Nov 2021 00:39:38 +0100
> > Hans de Goede <hdegoede@redhat.com> 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:  
> 
> Thanks for the hint, but I've not rebased my tree to those latest couple
> of patches yet, the same tree does work on the T101HA with /dev/video2 :)
> 
> I think this may be a module load ordering issue, I believe that the
> sensor drivers need to be loaded before the atomisp driver itself
> and on the T101HA we are hitting this "sweet spot", where as on
> the surface I was loading the not yet merged sensor drivers manually,
> causing them to be loaded later.

Could be. The atomisp driver should be using V4L2 async kAPI, in
order to properly register the sensors as their init code finishes
to register.

Right now, the registration logic inside atomisp waits for the first
sensor to be available. So, everything works fine on devices with
just one sensor, like Asus T101, but devices with multiple sensors 
may end having just one of them registered.

Regards,
Mauro

Re: [BUG/RFC PATCH 3/5] [BUG][RFC] media: atomisp: pci: add NULL check for asd obtained from atomisp_video_pipe

[Tsuchiya Yuto]

<removed Alan from Cc as the mail address not reachable>

On Tue, 2021-11-02 at 16:02 +0300, Dan Carpenter wrote:
> On Mon, Oct 18, 2021 at 01:23:34AM +0900, Tsuchiya Yuto wrote:
> > diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> > index 366161cff560..7206d29ba263 100644
> > --- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> > +++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> > @@ -1715,6 +1715,12 @@ void atomisp_wdt_refresh_pipe(struct atomisp_video_pipe *pipe,
> >  {
> >  	unsigned long next;
> >  
> > +	if(!pipe->asd) {
> 
> Run your patches through scripts/checkpatch.pl.

I'm sorry about this. I did checkpatch for another series I sent earlier,
but forgot this series.

Regards,
Tsuchiya Yuto

> > +		dev_err(pipe->isp->dev, "%s(): asd is NULL, device is %s\n",
> > +			__func__, pipe->vdev.name);
> > +		return;
> > +	}
> 
> regards,
> dan carpenter
> 

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

[Tsuchiya Yuto]

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 testimage_@.raw \
                --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

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

[Tsuchiya Yuto]

On Mon, 2021-11-08 at 08:55 +0100, Hans de Goede wrote:
> Hi Mauro,
> 
> On 11/8/21 08:41, Mauro Carvalho Chehab wrote:
> > Em Mon, 8 Nov 2021 00:39:38 +0100
> > Hans de Goede <hdegoede@redhat.com> 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:
> 
> Thanks for the hint, but I've not rebased my tree to those latest couple
> of patches yet, the same tree does work on the T101HA with /dev/video2 :)
> 
> I think this may be a module load ordering issue, I believe that the
> sensor drivers need to be loaded before the atomisp driver itself
> and on the T101HA we are hitting this "sweet spot",

> where as on
> the surface I was loading the not yet merged sensor drivers manually,
> causing them to be loaded later.
> 
> I still need to verify this theory, Tsuchiya can you perhaps confirm 
> that the modules must be loaded in this order?

Sorry I forgot to add a note about a load order in my sensor driver repo
for the case if you insmod external drivers. Yes, you need to load sensor
drivers _before_ the main atomisp driver.

So, here is the script to load sensor drivers in a proper order. Using
rmmod because as I sent a bug report ("[BUG 4/5] [BUG] media: atomisp:
`modprobe -r` not working well (dup video4linux, ATOMISP_SUBDEV_{0,1})"),
`modprobe -r` does not work well for me.

        rmmod atomisp
        insmod atomisp-ar0330.ko
        insmod atomisp-ov883x.ko
        # IIRC, modprobe works but try insmod instead if weird
        modprobe atomisp

Here is the insmod script I use for the record:

        # load drivers needed for atomisp first for insmod
        # for sensor drivers
        sudo modprobe media # needed for older LTS
        sudo modprobe videodev
        sudo modprobe v4l2_common # needed for older LTS
        sudo modprobe v4l2_async # if using async_register
        # for atomisp
        sudo modprobe videobuf-core
        sudo modprobe videobuf-vmalloc

        # load upstreamed atomisp drivers
        sudo insmod upst/atomisp_gmin_platform.ko
        sudo insmod upst/surface3/atomisp-ar0330.ko
        sudo insmod upst/surface3/atomisp-ov883x.ko
        sudo insmod upst/atomisp.ko dbg_level=1 #dyndbg

I'm denylisting all the atomisp drivers and I load these manually
currently to prevent oopses in case I use non-patched kernel.



Btw, this load order issue is indeed another problem even when the sensor
drivers are built as in-tree module. I sometimes see atomisp fails to
register camera device(s) on surface3.

On mipad2, things are worse. It uses regulator driver but sensor drivers
do not wait for the driver to initialize the regulators. This results in
probe failure for sensor drivers.



Regards,
Tsuchiya Yuto

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

[Mauro Carvalho Chehab]

Em Tue, 09 Nov 2021 13:15:07 +0900
Tsuchiya Yuto <kitakar@gmail.com> escreveu:

> 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 testimage_@.raw \
>                 --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)?

I have some hints, based on what changeset e4c5cfe8d96d ("media: atomisp:
 fix VIDIOC_S_FMT logic") is meant to fix:

1. atomisp wastes 16 columns and 16 lines per default (pad_h/pad_w). Those
   are controlled by a modprobe parameter. The entire logic is based on
   the assumption that the sensor drivers will add those extra 16 bytes
   to the formats. Upstreamed sensors (out of staging) won't do the same,
   which could be the cause of problems.
 
   On other words, if you set 1920x1080, atomisp will try to find a
   resolution at the source pad (sensor) with 1936x1096, and will set
   the output pad to 1920x1080.

   I would try:

		$ modprobe atomisp with pad_h=0 pad_w=0

   and see if it would solve the issue. I would expect some artifacts
   at the image borders, though, depending on the pipeline;

2. I got a bug with atomisp-ov2680 when setting resolution. Such
   driver supports two resolutions:

	- 1616x1216
	- 1616x916

   The logic which finds the nearest resolution at the downstream drivers
   were broken, causing the driver to get the wrong resolution if the
   atomisp driver requests a higher resolution. So, it returns 1616x916
   if, for instance, userspace requests 1616x1216 (which is internally
   converted into 1632x1232).

   You could enable some debug info in order to check what resolution
   atomisp is requesting and what the sensor is returning back.

Besides that, the patch also fix two issues that aren't probably related
to the issue you're seeing:

- The driver returns the sensor format, e.g. with pad (+16) added to
  both vertical and horizontal resolutions;
 
- if pixfmt==BG10 (and probably any other bayer format), S_FMT fails.

I hope that helps.

Regards,
Mauro

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

[Mauro Carvalho Chehab]

Em Tue, 09 Nov 2021 13:18:37 +0900
Tsuchiya Yuto <kitakar@gmail.com> escreveu:

> On Mon, 2021-11-08 at 08:55 +0100, Hans de Goede wrote:
> > Hi Mauro,
> > 
> > On 11/8/21 08:41, Mauro Carvalho Chehab wrote:  
> > > Em Mon, 8 Nov 2021 00:39:38 +0100
> > > Hans de Goede <hdegoede@redhat.com> 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:  
> > 
> > Thanks for the hint, but I've not rebased my tree to those latest couple
> > of patches yet, the same tree does work on the T101HA with /dev/video2 :)
> > 
> > I think this may be a module load ordering issue, I believe that the
> > sensor drivers need to be loaded before the atomisp driver itself
> > and on the T101HA we are hitting this "sweet spot",  
> 
> > where as on
> > the surface I was loading the not yet merged sensor drivers manually,
> > causing them to be loaded later.
> > 
> > I still need to verify this theory, Tsuchiya can you perhaps confirm 
> > that the modules must be loaded in this order?  
> 
> Sorry I forgot to add a note about a load order in my sensor driver repo
> for the case if you insmod external drivers. Yes, you need to load sensor
> drivers _before_ the main atomisp driver.
> 
> So, here is the script to load sensor drivers in a proper order. Using
> rmmod because as I sent a bug report ("[BUG 4/5] [BUG] media: atomisp:
> `modprobe -r` not working well (dup video4linux, ATOMISP_SUBDEV_{0,1})"),
> `modprobe -r` does not work well for me.
> 
>         rmmod atomisp
>         insmod atomisp-ar0330.ko
>         insmod atomisp-ov883x.ko
>         # IIRC, modprobe works but try insmod instead if weird
>         modprobe atomisp

Ok, you need to probe first the sensors, because the lack of support
for V4L2 async kAPI at the driver.

See, the way atomisp expects is that it will initialize the
subdevs at:

	atomisp_subdev_probe() function at atomisp_v4l2.

I added a hacky logic there to avoid the need of probing drivers
manually. It makes atomisp driver to wait up to SUBDEV_WAIT_TIMEOUT_MAX_COUNT
for a sensor to be detected, and added a FIXME to remind that this requires
further changes.

Once the first sensor is detected, it waits for 5 * SUBDEV_WAIT_TIMEOUT
for it to finish probing, hoping that everything will be fine after
such time. If the second sensor registers on such time (on devices with
more than one sensor), everything will also be ok, but it seems that
the time there is not enough for the second sensor to register.

The right solution here would be, instead:

1. to return -EPROBE_DEFER, if not all sensors are detected. Some
   care need to be taken doing that, though, in order to avoid
   troubles re-initializing things;
2. to let the sensors register themselves, instead of relying on
   atomisp driver for doing that.

Once the drivers got converted to register themselves as subdevices
without needing atomisp for doing that (nor the ancillary module
atomisp_gmin_platform), the right way would be to use the V4L2 async 
framework, which was designed to ensure that all subdevs are there
during device's init time.

There's a catch, though: the main driver needs to know how many sensors
it will wait to be probed. Right now, the driver doesn't store or
gather this information directly.

Regards,
Mauro

Re: [BUG/RFC PATCH 1/5] [BUG][RFC] media: atomisp: pci: assume run_mode is PREVIEW

[Tsuchiya Yuto]

<Adding back people/list to Cc>

On Fri, 2021-10-29 at 10:02 +0100, Mauro Carvalho Chehab wrote:
> Em Mon, 18 Oct 2021 01:23:32 +0900
> Tsuchiya Yuto <kitakar@gmail.com> escreveu:
> 
> > This is almost a BUG report with a patch to just make atomisp work
> > again with the current mainline kernel. Thus, prefixed with [BUG][RFC].
> > 
> > RFC:
> >   1. When looking at the `case CI_MODE_NONE:`, it tries to do something.
> >      So, it should rather work with CI_MODE_NONE, too? Just I don't know
> >      how to configure userspace apps to use CI_MODE_NONE ?
> >   2. How can we re-add the run mode support again without relying on
> >      the s_parm ?
> > 
> > > 8-----------------------------------------------------------------8<  
> > 
> > After the commit 8a7c5594c020 ("media: v4l2-ioctl: clear fields in s_parm")
> > added on v4.18 (backported to v4.9.182 and v4.14.127), the capture mode
> > flag is cleared (except for V4L2_MODE_HIGHQUALITY).
> > 
> > Due to this, it seems that now we can't use this flag to set atomisp
> > run_mode. This results in capture not working with the following message
> > (loaded atomisp driver with dbg_level=1):
> > 
> > 	kern  :warn  : [ 3658.776616] ia_css_pipe_get_info: ia_css_stream_create needs to be called before ia_css_[stream/pipe]_get_info
> > 	kern  :err   : [ 3658.776641] atomisp-isp2 0000:00:03.0: get_frame_info 1920x1080 (padded to 0)
> > 	kern  :warn  : [ 3658.776666] atomisp-isp2 0000:00:03.0: Can't set format on ISP. Error -22
> > 
> > So, when we can't detect run mode from the s_parm capturemode
> > (CI_MODE_NONE), let's assume the run mode is PREVIEW.
> 
> The run_mode on this driver is related to how camera works on Android.
> On such systems, camera have a preview mode, which is what the user
> views at the camera display. This is usually set to the display
> resolution (or to some other resolution that would allow a high
> fps rate). When the photo button is clicked, it sets the camera to
> the highest resolution mode (by default), which usually means a low
> fps rate.

Thank you for the description. I always wondered why they needed to
introduce run mode stuff.

> This doesn't make much sense on desktops, nor standard Linux apps
> support that.
> 
> Anyway, after looking on this patch, I guess the best is to apply
> this patch instead:
> 
> 	https://lore.kernel.org/linux-media/543e61dd07c90a7d8577b3a94696edc77953b9d8.1635497370.git.mchehab+huawei@kernel.org/T/#u
> 
> As a plus side, it is also one step further to allow generic apps to
> use the atomisp driver.
> 
> Applying it allowed me to do:
> 
> 	$ v4l2grab -D -f 'NV12' -x 1600 -y 1200 -d /dev/video2 -u
> 	$ nvt/raw2pnm -x1600 -y1200 -f NV12 out017.raw out017.pnm
> 	$ feh out017.pnm
> 
> So, I'm dropping patch 1/5 from media_stage, replacing it by the one which
> sets the mode after open().

Sorry for a bit late reply, I checked the capture functionality with the
latest media_stage patches and can confirm capture is still working
regarding this run_mode issue.

So, I agree to replace this my patch with yours. Thanks!

Regards,
Tsuchiya Yuto

> Regards,
> Mauro
> 
> > 
> > Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
> > ---
> >  drivers/staging/media/atomisp/pci/atomisp_ioctl.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> > index c8a625667e81..6fc64f0ccc31 100644
> > --- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> > +++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> > @@ -2638,7 +2638,11 @@ static int atomisp_s_parm(struct file *file, void *fh,
> >  				asd->high_speed_mode = true;
> >  		}
> >  
> > -		goto out;
> > +		dev_warn(isp->dev,
> > +			 "setting run_mode using s_parm capturemode is not supported anymore\n");
> > +		dev_warn(isp->dev, "assuming run_mode is PREVIEW\n");
> > +		mode = ATOMISP_RUN_MODE_PREVIEW;
> > +		break;
> >  	}
> >  	case CI_MODE_VIDEO:
> >  		mode = ATOMISP_RUN_MODE_VIDEO;