[PATCH 0/2] media: amd: isp4: two fixes found on the ROG Flow Z13

[PATCH 0/2] media: amd: isp4: two fixes found on the ROG Flow Z13

[Seifelden Abdelmonem]

These are two small fixes for the AMD ISP4 capture driver. I found
them while bringing up the OV13B10 rear camera of the ASUS ROG Flow Z13
(GZ302EA, ACPI ID OMNI13B1), which is discussed in [1]. Both fixes are
independent of that sensor and apply to the driver as merged.

Patch 1 holds buf_list_lock while start_streaming hands the queued
buffers to the firmware. The firmware response thread can remove
completed buffers from the same list during that walk; I hit the
resulting list corruption as an oops.

Patch 2 makes completed buffers report V4L2_FIELD_NONE instead of
V4L2_FIELD_ANY, which v4l2-compliance rejects.

The two patches are independent of each other and apply in any order.

Testing: the GZ302 camera is not supported upstream, so I applied the
patches to an out-of-tree build of this driver that streams on that
machine (NV12 1920x1080, about 30 fps). The stock linux-firmware image
does not start this sensor; on my machine the camera runs with ISP
firmware and sensor setup data from my own Windows driver
installation, read from the Windows partition of that machine. I do
not redistribute any of it.
- 6 stream starts with patch 1 alone and 6 with both patches (each
  time 5 captures of 90 frames plus the v4l2-compliance run); no oops,
  hung task or WARN.
- With both patches, s2idle suspend and hibernation with the camera
  idle: capture works before and after.
- v4l2-compliance -s: 52 of 58 without the patches, the same 52 with
  patch 1, 53 with both.
- W=1 build (clang) on media-committers next. Not run: lockdep,
  sparse, smatch.

Known issues, not addressed by this series:
- The remaining 5 v4l2-compliance failures all stop at
  v4l2-test-buffers.cpp:1651: CREATE_BUFS with half the sizeimage after
  REQBUFS is expected to fail with EINVAL and does not. A likely cause,
  not yet confirmed: isp4vid_qops_queue_setup() writes the requested
  size back into isp_vdev->format.sizeimage, so a larger CREATE_BUFS
  earlier in the test raises the size that later requests are checked
  against.
- A stream does not survive system sleep. Start a stream, then
  "rtcwake -m mem -s 20": after resume no further frame arrives, and
  stopping the stream logs "fail to disable stream" and "fail to stop
  stream". The same happens without these patches; the driver has no
  system-sleep callbacks. On the GZ302 the command timeouts return; the
  stop-path hang reported in [1] may be related. This should be
  reproducible on the HP ZBook Ultra G1a, and I can test a fix on the
  GZ302.
- Hibernating while a stream is running hung the GZ302 once, during
  hibernation, and needed a forced power-off. That run used both
  patches and I did not repeat it without them, so I cannot say yet
  whether it predates them; neither patch touches the sleep path.

AI use: Claude (Claude Code) helped me analyse the code, write the
changes and the changelogs, and run the checks. I reviewed the patches
and ran the hardware tests.

[1] https://lore.kernel.org/all/lWYl1YJwDSh-q2Ew2f8b0BweQ4V-SUnPMSMurU5gT1LkqVdrmWFunj0sSgjC7m66bORBAD_jRCFTt1_ZOkotOMukY46rI4ayYbIfPQq4E9U=@proton.me/

Seifelden Abdelmonem (2):
  media: amd: isp4: hold buf_list_lock while sending buffers at stream
    start
  media: amd: isp4: return capture buffers with V4L2_FIELD_NONE

 drivers/media/platform/amd/isp4/isp4_video.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)


base-commit: d7e7bca7169873b46e857c025c10f3336c1258d8
-- 
2.56.0

[PATCH 1/2] media: amd: isp4: hold buf_list_lock while sending buffers at stream start

[Seifelden Abdelmonem]

isp4vid_qops_start_streaming() walks isp_vdev->buf_list without
buf_list_lock and passes each buffer to isp4sd_ioc_send_img_buf().
Every other access to buf_list holds that lock.

vb2 serialises the walk against buf_queue(), but not against the
firmware response thread. Once ISP4SD_MIN_BUF_CNT_BEF_START_STREAM
buffers have been sent, isp4sd_ioc_send_img_buf() also sends
START_STREAM, so the firmware can complete buffers while the walk is
still running. isp4vid_handle_frame_done() then list_del()s the
completed buffer from buf_list. If that buffer is the current entry of
the walk, list_for_each_entry() follows the poisoned next pointer:

  Oops: general protection fault, maybe for address 0xffff8e6944d97060
  RIP: 0010:isp4if_alloc_buffer_node+0x37/0x50 [amd_isp4_capture]
  RBX: dead0000000000b8 ... RSI: dead0000000000b8
  Call Trace:
   isp4sd_ioc_send_img_buf
   isp4vid_qops_start_streaming
   vb2_start_streaming
   vb2_core_streamon

The oopsing task holds the queue lock, so the video device can no
longer be closed and the module cannot be unloaded until reboot.

This was hit on a board that is not supported upstream yet, with an
out-of-tree change that made one isp4sd_ioc_send_img_buf() call take
over a second. With the stock driver the window is much shorter, but
the list is still modified concurrently without the lock.

Hold buf_list_lock across the walk. This adds the lock order
buf_list_lock -> ops_mutex. No path takes them in the reverse order:
isp4vid_handle_frame_done() runs in the response thread without
ops_mutex. isp4sd_ioc_send_img_buf() only queues commands and does not
wait for a firmware response, so the response thread is at most
delayed until the initial buffers have been sent.

Fixes: 2ccf48af2270 ("media: platform: amd: isp4 video node and buffers handling added")
Signed-off-by: Seifelden Abdelmonem <saif.elden.hatim@gmail.com>
---
Testing and AI use: see the cover letter.

 drivers/media/platform/amd/isp4/isp4_video.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/amd/isp4/isp4_video.c b/drivers/media/platform/amd/isp4/isp4_video.c
index 856a2a0b4..58ea8d6ac 100644
--- a/drivers/media/platform/amd/isp4/isp4_video.c
+++ b/drivers/media/platform/amd/isp4/isp4_video.c
@@ -567,9 +567,11 @@ static int isp4vid_qops_start_streaming(struct vb2_queue *vq,
 		}
 	}
 
-	list_for_each_entry(isp4vid_buf, &isp_vdev->buf_list, list)
-		isp4sd_ioc_send_img_buf(isp_vdev->isp_sdev,
-					&isp4vid_buf->img_buf);
+	scoped_guard(mutex, &isp_vdev->buf_list_lock) {
+		list_for_each_entry(isp4vid_buf, &isp_vdev->buf_list, list)
+			isp4sd_ioc_send_img_buf(isp_vdev->isp_sdev,
+						&isp4vid_buf->img_buf);
+	}
 
 	isp_vdev->stream_started = true;
 
-- 
2.56.0

[PATCH 2/2] media: amd: isp4: return capture buffers with V4L2_FIELD_NONE

[Seifelden Abdelmonem]

isp4vid_handle_frame_done() marks every completed buffer as
V4L2_FIELD_ANY. V4L2_FIELD_ANY is only valid as a request from the
application; a dequeued buffer must report the field order of the
image it contains. v4l2-compliance therefore fails the first MMAP
streaming test:

  fail: v4l2-test-buffers.cpp(419): g_field() == V4L2_FIELD_ANY
  fail: v4l2-test-buffers.cpp(1280): buf.check(q, last_seq)
  fail: v4l2-test-buffers.cpp(1735): captureBufs(...)
  test MMAP (no poll, REQBUFS): FAIL

The ISP only produces progressive frames, and the format ioctls already
report V4L2_FIELD_NONE (isp4vid_fmt_default). Use V4L2_FIELD_NONE for
the returned buffers as well. With this change that test passes.

Fixes: 2ccf48af2270 ("media: platform: amd: isp4 video node and buffers handling added")
Signed-off-by: Seifelden Abdelmonem <saif.elden.hatim@gmail.com>
---
Testing and AI use: see the cover letter.

 drivers/media/platform/amd/isp4/isp4_video.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/amd/isp4/isp4_video.c b/drivers/media/platform/amd/isp4/isp4_video.c
index 58ea8d6ac..b8af3c766 100644
--- a/drivers/media/platform/amd/isp4/isp4_video.c
+++ b/drivers/media/platform/amd/isp4/isp4_video.c
@@ -72,7 +72,7 @@ void isp4vid_handle_frame_done(struct isp4vid_dev *isp_vdev,
 	/* Fill the buffer */
 	isp4vid_buf->vb2.vb2_buf.timestamp = ktime_get_ns();
 	isp4vid_buf->vb2.sequence = isp_vdev->sequence++;
-	isp4vid_buf->vb2.field = V4L2_FIELD_ANY;
+	isp4vid_buf->vb2.field = V4L2_FIELD_NONE;
 
 	vb2_set_plane_payload(&isp4vid_buf->vb2.vb2_buf,
 			      0, isp_vdev->format.sizeimage);
-- 
2.56.0