Re: [PATCH] media: amd: isp4: fix self-deadlock in isp4sd_start_stream() error path

[Guido Kraemer]

Hi Caspar,

I hit this deadlock regularly on an HP ZBook Ultra G1a (Arch 7.2.6 and
CachyOS 7.2.7), usually when Chromium restarted the camera during a
video call. After "fail to get out prop" / "fail to setup output", the
streaming thread hung and every later user of /dev/video0 blocked in
vb2_fop_release until reboot:

  INFO: task ThreadPoolSingl:45879 blocked for more than 122 seconds.
  Call Trace:
   isp4sd_stop_stream.isra.0+0x52/0x200 [amd_isp4_capture]
   isp4sd_enable_streams.cold+0x60/0x2ad [amd_isp4_capture]
   v4l2_subdev_enable_streams+0x27f/0x400 [videodev]
   isp4vid_qops_start_streaming+0x114/0x190 [amd_isp4_capture]
   vb2_start_streaming+0x67/0x130 [videobuf2_common]
   vb2_core_streamon+0x98/0xf0 [videobuf2_common]

Tested on the G1a with v7.3.1 plus this patch and Seifelden's two isp4
fixes, with your sequence: v4l2-ctl --set-fmt-video=1920x1080 NV12
--stream-mmap, then the same without --set-fmt-video, then the first
command again. The second run fails with "fail to get out prop" /
"fail to setup output" and VIDIOC_STREAMON returns -EINVAL, v4l2-ctl
exits, and no task is left blocked. The third run starts and stops the
stream normally. Zoom also opened and closed the camera about fifteen
times without a hang.

(On this machine the camera currently gets no frames for an unrelated
reason, the firmware rejects SET_STREAM_CONFIG; I'm reporting that in
a separate thread. The start and stop paths run all the same.)

Tested-by: Guido Kraemer <guido.kraemer@xxxxxxxxxxxxxx>

Thanks,
Guido


On 10/28/26 15:05, Caspar Adriani wrote:
> isp4sd_start_stream() takes ops_mutex with guard() and, when
> isp4sd_init_stream() or isp4sd_setup_output() fails, jumps to
> err_stop_stream and calls isp4sd_stop_stream(). That function takes
> ops_mutex again, so the task blocks on a mutex it already holds. Every
> later open, close or ioctl on the device then blocks behind it, and
> only a reboot recovers:
>
>    INFO: task .v4l2-ctl-wrapp:38440 blocked for more than 122 seconds.
>     isp4sd_stop_stream.isra.0+0x43/0x1d0 [amd_isp4_capture]
>     isp4sd_enable_streams.cold+0x8b/0x26c [amd_isp4_capture]
>     v4l2_subdev_enable_streams+0x1ff/0x360 [videodev]
>     isp4vid_qops_start_streaming+0xf2/0x160 [amd_isp4_capture]
>    INFO: task .v4l2-ctl-wrapp:38440 is blocked on a mutex likely owned
>    by task .v4l2-ctl-wrapp:38440.
>
> One way to reach the error path: start and stop a stream, then start
> it again without setting the format. isp4sd_uninit_stream() clears the
> subdev pad format on stop, so isp4sd_get_str_out_prop() fails on the
> second start with "fail to get out prop" and "fail to setup output".
>
> Move the body of isp4sd_stop_stream() into __isp4sd_stop_stream(),
> which expects ops_mutex to be held, and call it from the error path.
>
> Fixes: 4e5e7a7ddb4a ("media: platform: amd: isp4 subdev and firmware loading handling added")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Caspar Adriani <caspar.adriani@xxxxxxxxx>
> ---
> Tested on an ASUS ROG Flow Z13 (GZ302EA) on v7.3, with a local,
> not-for-upstream change that adds its OMNI13B1 sensor to the ISP4 ACPI
> tables so the capture device is created. The stock firmware does not
> support that sensor, so no frames arrive, but the start and stop paths
> run. Sequence: v4l2-ctl --set-fmt-video=1920x1080 NV12 --stream-mmap,
> stopped after 10 s; the same without --set-fmt-video; then the first
> command again.
>
> Without this patch (v7.2.8, same setup) this failure hung the task as
> shown above. With it, VIDIOC_STREAMON returns -EINVAL after "fail to
> get out prop" / "fail to setup output", v4l2-ctl exits, no task is left
> blocked, and the third run starts and stops the stream normally.
> Not run: lockdep (CONFIG_PROVE_LOCKING is off in this kernel).
> ---
>   drivers/media/platform/amd/isp4/isp4_subdev.c | 15 +++++++++++----
>   1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/platform/amd/isp4/isp4_subdev.c b/drivers/media/platform/amd/isp4/isp4_subdev.c
> index a108d90ace..08512f060a 100644
> --- a/drivers/media/platform/amd/isp4/isp4_subdev.c
> +++ b/drivers/media/platform/amd/isp4/isp4_subdev.c
> @@ -745,15 +745,15 @@ int isp4sd_pwron_and_init(struct v4l2_subdev *sd)
>   	return -EINVAL;
>   }
>   
> -static int isp4sd_stop_stream(struct isp4_subdev *isp_subdev,
> -			      struct v4l2_subdev_state *state, u32 pad)
> +static void __isp4sd_stop_stream(struct isp4_subdev *isp_subdev,
> +				 struct v4l2_subdev_state *state, u32 pad)
>   {
>   	struct isp4sd_sensor_info *sensor_info = &isp_subdev->sensor_info;
>   	struct isp4sd_output_info *output_info = &sensor_info->output_info;
>   	struct isp4_interface *ispif = &isp_subdev->ispif;
>   	struct device *dev = isp_subdev->dev;
>   
> -	guard(mutex)(&isp_subdev->ops_mutex);
> +	lockdep_assert_held(&isp_subdev->ops_mutex);
>   	dev_dbg(dev, "status %i\n", output_info->start_status);
>   
>   	if (output_info->start_status == ISP4SD_START_STATUS_STARTED) {
> @@ -786,6 +786,13 @@ static int isp4sd_stop_stream(struct isp4_subdev *isp_subdev,
>   	}
>   
>   	isp4sd_uninit_stream(isp_subdev, state, pad);
> +}
> +
> +static int isp4sd_stop_stream(struct isp4_subdev *isp_subdev,
> +			      struct v4l2_subdev_state *state, u32 pad)
> +{
> +	guard(mutex)(&isp_subdev->ops_mutex);
> +	__isp4sd_stop_stream(isp_subdev, state, pad);
>   
>   	/*
>   	 * Return success to ensure the stop process proceeds,
> @@ -836,7 +843,7 @@ static int isp4sd_start_stream(struct isp4_subdev *isp_subdev,
>   	return 0;
>   
>   err_stop_stream:
> -	isp4sd_stop_stream(isp_subdev, state, pad);
> +	__isp4sd_stop_stream(isp_subdev, state, pad);
>   	return ret;
>   }
>   
>
> ---
> base-commit: 65ba8f34ec886ec72b2c07ddc8cf3d815c32869d
> change-id: 20261028-isp4-start-stream-deadlock-6c33a97d3297
>
> Best regards,
> --
> Caspar Adriani <caspar.adriani@xxxxxxxxx>