[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

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

[Bin Du]

Reviewed-by: Bin Du <bin.du@amd.com>

On 10/27/2026 2:24 AM, Seifelden Abdelmonem wrote:
> 
> 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

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

[Bin Du]

Reviewed-by: Bin Du <bin.du@amd.com>

On 10/27/2026 2:24 AM, Seifelden Abdelmonem wrote:
> 
> 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

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

[Nirujogi, Pratap]

On 10/26/2026 2:24 PM, Seifelden Abdelmonem wrote:
> [You don't often get email from saif.elden.hatim@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> 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.
> 
Reviewed-by: Pratap Nirujogi <pratap.nirujogi@amd.com>

Thanks for submitting the fix. It appears we were fortunate to have 
passed the tests, as isp4sd_ioc_send_img_buf() never took this long to 
reproduce the issue while the stream is running. I’m curious about the 
sequence you are using to reproduce this issue. Would you mind sharing 
the details of the sequence and steps involved?

Thanks,
Pratap

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

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

[Nirujogi, Pratap]

On 10/26/2026 2:24 PM, Seifelden Abdelmonem wrote:
> [You don't often get email from saif.elden.hatim@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> 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>
> ---
Reviewed-by: Pratap Nirujogi <pratap.nirujogi@amd.com>

Thanks,
Pratap


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