* [PATCH v2 1/2] media: nxp: imx8-isi: Fix streaming cleanup on release
2025-08-13 21:24 [PATCH v2 0/2] media: nxp: imx8-isi: fix streaming cleanup on release Laurent Pinchart
@ 2025-08-13 21:24 ` Laurent Pinchart
2025-08-13 22:38 ` Richard Leitner
2025-08-13 21:24 ` [PATCH v2 2/2] media: nxp: imx8-isi: Drop mxc_isi_video.is_streaming field Laurent Pinchart
2025-08-13 21:46 ` [PATCH v2 0/2] media: nxp: imx8-isi: fix streaming cleanup on release Richard Leitner
2 siblings, 1 reply; 7+ messages in thread
From: Laurent Pinchart @ 2025-08-13 21:24 UTC (permalink / raw)
To: linux-media
Cc: Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Richard Leitner, imx
From: Richard Leitner <richard.leitner@linux.dev>
The current implementation unconditionally calls
mxc_isi_video_cleanup_streaming() in mxc_isi_video_release(). This can
lead to situations where any release call (like from a simple
"v4l2-ctl -l") may release a currently streaming queue when called on
such a device.
This is reproducible on an i.MX8MP board by streaming from an ISI
capture device using gstreamer:
gst-launch-1.0 -v v4l2src device=/dev/videoX ! \
video/x-raw,format=GRAY8,width=1280,height=800,framerate=1/120 ! \
fakesink
While this stream is running, querying the caps of the same device
provokes the error state:
v4l2-ctl -l -d /dev/videoX
This results in the following trace:
[ 155.452152] ------------[ cut here ]------------
[ 155.452163] WARNING: CPU: 0 PID: 1708 at drivers/media/platform/nxp/imx8-isi/imx8-isi-pipe.c:713 mxc_isi_pipe_irq_handler+0x19c/0x1b0 [imx8_isi]
[ 157.004248] Modules linked in: cfg80211 rpmsg_ctrl rpmsg_char rpmsg_tty virtio_rpmsg_bus rpmsg_ns rpmsg_core rfkill nft_ct nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 nf_tables mcp251x6
[ 157.053499] CPU: 0 UID: 0 PID: 1708 Comm: python3 Not tainted 6.15.4-00114-g1f61ca5cad76 #1 PREEMPT
[ 157.064369] Hardware name: imx8mp_board_01 (DT)
[ 157.068205] pstate: 400000c5 (nZcv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 157.075169] pc : mxc_isi_pipe_irq_handler+0x19c/0x1b0 [imx8_isi]
[ 157.081195] lr : mxc_isi_pipe_irq_handler+0x38/0x1b0 [imx8_isi]
[ 157.087126] sp : ffff800080003ee0
[ 157.090438] x29: ffff800080003ee0 x28: ffff0000c3688000 x27: 0000000000000000
[ 157.097580] x26: 0000000000000000 x25: ffff0000c1e7ac00 x24: ffff800081b5ad50
[ 157.104723] x23: 00000000000000d1 x22: 0000000000000000 x21: ffff0000c25e4000
[ 157.111866] x20: 0000000060000200 x19: ffff80007a0608d0 x18: 0000000000000000
[ 157.119008] x17: ffff80006a4e3000 x16: ffff800080000000 x15: 0000000000000000
[ 157.126146] x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000
[ 157.133287] x11: 0000000000000040 x10: ffff0000c01445f0 x9 : ffff80007a053a38
[ 157.140425] x8 : ffff0000c04004b8 x7 : 0000000000000000 x6 : 0000000000000000
[ 157.147567] x5 : ffff0000c0400490 x4 : ffff80006a4e3000 x3 : ffff0000c25e4000
[ 157.154706] x2 : 0000000000000000 x1 : ffff8000825c0014 x0 : 0000000060000200
[ 157.161850] Call trace:
[ 157.164296] mxc_isi_pipe_irq_handler+0x19c/0x1b0 [imx8_isi] (P)
[ 157.170319] __handle_irq_event_percpu+0x58/0x218
[ 157.175029] handle_irq_event+0x54/0xb8
[ 157.178867] handle_fasteoi_irq+0xac/0x248
[ 157.182968] handle_irq_desc+0x48/0x68
[ 157.186723] generic_handle_domain_irq+0x24/0x38
[ 157.191346] gic_handle_irq+0x54/0x120
[ 157.195098] call_on_irq_stack+0x24/0x30
[ 157.199027] do_interrupt_handler+0x88/0x98
[ 157.203212] el0_interrupt+0x44/0xc0
[ 157.206792] __el0_irq_handler_common+0x18/0x28
[ 157.211328] el0t_64_irq_handler+0x10/0x20
[ 157.215429] el0t_64_irq+0x198/0x1a0
[ 157.219009] ---[ end trace 0000000000000000 ]---
Address this issue by moving the streaming preparation and cleanup to
the vb2 .prepare_streaming() and .unprepare_streaming() operations. This
also simplifies the driver by allowing direct usage of the
vb2_ioctl_streamon() and vb2_ioctl_streamoff() helpers, and removal of
the manual cleanup from mxc_isi_video_release().
Signed-off-by: Richard Leitner <richard.leitner@linux.dev>
Co-developed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
.../platform/nxp/imx8-isi/imx8-isi-video.c | 156 +++++++-----------
1 file changed, 58 insertions(+), 98 deletions(-)
diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-video.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-video.c
index 8654150728a8..042b554d2775 100644
--- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-video.c
+++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-video.c
@@ -937,6 +937,49 @@ static void mxc_isi_video_init_channel(struct mxc_isi_video *video)
mxc_isi_channel_set_output_format(pipe, video->fmtinfo, &video->pix);
}
+static int mxc_isi_vb2_prepare_streaming(struct vb2_queue *q)
+{
+ struct mxc_isi_video *video = vb2_get_drv_priv(q);
+ struct media_device *mdev = &video->pipe->isi->media_dev;
+ struct media_pipeline *pipe;
+ int ret;
+
+ /* Get a pipeline for the video node and start it. */
+ scoped_guard(mutex, &mdev->graph_mutex) {
+ ret = mxc_isi_pipe_acquire(video->pipe,
+ &mxc_isi_video_frame_write_done);
+ if (ret)
+ return ret;
+
+ pipe = media_entity_pipeline(&video->vdev.entity)
+ ? : &video->pipe->pipe;
+
+ ret = __video_device_pipeline_start(&video->vdev, pipe);
+ if (ret)
+ goto err_release;
+ }
+
+ /* Verify that the video format matches the output of the subdev. */
+ ret = mxc_isi_video_validate_format(video);
+ if (ret)
+ goto err_stop;
+
+ /* Allocate buffers for discard operation. */
+ ret = mxc_isi_video_alloc_discard_buffers(video);
+ if (ret)
+ goto err_stop;
+
+ video->is_streaming = true;
+
+ return 0;
+
+err_stop:
+ video_device_pipeline_stop(&video->vdev);
+err_release:
+ mxc_isi_pipe_release(video->pipe);
+ return ret;
+}
+
static int mxc_isi_vb2_start_streaming(struct vb2_queue *q, unsigned int count)
{
struct mxc_isi_video *video = vb2_get_drv_priv(q);
@@ -985,13 +1028,26 @@ static void mxc_isi_vb2_stop_streaming(struct vb2_queue *q)
mxc_isi_video_return_buffers(video, VB2_BUF_STATE_ERROR);
}
+static void mxc_isi_vb2_unprepare_streaming(struct vb2_queue *q)
+{
+ struct mxc_isi_video *video = vb2_get_drv_priv(q);
+
+ mxc_isi_video_free_discard_buffers(video);
+ video_device_pipeline_stop(&video->vdev);
+ mxc_isi_pipe_release(video->pipe);
+
+ video->is_streaming = false;
+}
+
static const struct vb2_ops mxc_isi_vb2_qops = {
.queue_setup = mxc_isi_vb2_queue_setup,
.buf_init = mxc_isi_vb2_buffer_init,
.buf_prepare = mxc_isi_vb2_buffer_prepare,
.buf_queue = mxc_isi_vb2_buffer_queue,
+ .prepare_streaming = mxc_isi_vb2_prepare_streaming,
.start_streaming = mxc_isi_vb2_start_streaming,
.stop_streaming = mxc_isi_vb2_stop_streaming,
+ .unprepare_streaming = mxc_isi_vb2_unprepare_streaming,
};
/* -----------------------------------------------------------------------------
@@ -1145,97 +1201,6 @@ static int mxc_isi_video_s_fmt(struct file *file, void *priv,
return 0;
}
-static int mxc_isi_video_streamon(struct file *file, void *priv,
- enum v4l2_buf_type type)
-{
- struct mxc_isi_video *video = video_drvdata(file);
- struct media_device *mdev = &video->pipe->isi->media_dev;
- struct media_pipeline *pipe;
- int ret;
-
- if (vb2_queue_is_busy(&video->vb2_q, file))
- return -EBUSY;
-
- /*
- * Get a pipeline for the video node and start it. This must be done
- * here and not in the queue .start_streaming() handler, so that
- * pipeline start errors can be reported from VIDIOC_STREAMON and not
- * delayed until subsequent VIDIOC_QBUF calls.
- */
- mutex_lock(&mdev->graph_mutex);
-
- ret = mxc_isi_pipe_acquire(video->pipe, &mxc_isi_video_frame_write_done);
- if (ret) {
- mutex_unlock(&mdev->graph_mutex);
- return ret;
- }
-
- pipe = media_entity_pipeline(&video->vdev.entity) ? : &video->pipe->pipe;
-
- ret = __video_device_pipeline_start(&video->vdev, pipe);
- if (ret) {
- mutex_unlock(&mdev->graph_mutex);
- goto err_release;
- }
-
- mutex_unlock(&mdev->graph_mutex);
-
- /* Verify that the video format matches the output of the subdev. */
- ret = mxc_isi_video_validate_format(video);
- if (ret)
- goto err_stop;
-
- /* Allocate buffers for discard operation. */
- ret = mxc_isi_video_alloc_discard_buffers(video);
- if (ret)
- goto err_stop;
-
- ret = vb2_streamon(&video->vb2_q, type);
- if (ret)
- goto err_free;
-
- video->is_streaming = true;
-
- return 0;
-
-err_free:
- mxc_isi_video_free_discard_buffers(video);
-err_stop:
- video_device_pipeline_stop(&video->vdev);
-err_release:
- mxc_isi_pipe_release(video->pipe);
- return ret;
-}
-
-static void mxc_isi_video_cleanup_streaming(struct mxc_isi_video *video)
-{
- lockdep_assert_held(&video->lock);
-
- if (!video->is_streaming)
- return;
-
- mxc_isi_video_free_discard_buffers(video);
- video_device_pipeline_stop(&video->vdev);
- mxc_isi_pipe_release(video->pipe);
-
- video->is_streaming = false;
-}
-
-static int mxc_isi_video_streamoff(struct file *file, void *priv,
- enum v4l2_buf_type type)
-{
- struct mxc_isi_video *video = video_drvdata(file);
- int ret;
-
- ret = vb2_ioctl_streamoff(file, priv, type);
- if (ret)
- return ret;
-
- mxc_isi_video_cleanup_streaming(video);
-
- return 0;
-}
-
static int mxc_isi_video_enum_framesizes(struct file *file, void *priv,
struct v4l2_frmsizeenum *fsize)
{
@@ -1291,9 +1256,8 @@ static const struct v4l2_ioctl_ops mxc_isi_video_ioctl_ops = {
.vidioc_expbuf = vb2_ioctl_expbuf,
.vidioc_prepare_buf = vb2_ioctl_prepare_buf,
.vidioc_create_bufs = vb2_ioctl_create_bufs,
-
- .vidioc_streamon = mxc_isi_video_streamon,
- .vidioc_streamoff = mxc_isi_video_streamoff,
+ .vidioc_streamon = vb2_ioctl_streamon,
+ .vidioc_streamoff = vb2_ioctl_streamoff,
.vidioc_enum_framesizes = mxc_isi_video_enum_framesizes,
@@ -1332,10 +1296,6 @@ static int mxc_isi_video_release(struct file *file)
if (ret)
dev_err(video->pipe->isi->dev, "%s fail\n", __func__);
- mutex_lock(&video->lock);
- mxc_isi_video_cleanup_streaming(video);
- mutex_unlock(&video->lock);
-
pm_runtime_put(video->pipe->isi->dev);
return ret;
}
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v2 2/2] media: nxp: imx8-isi: Drop mxc_isi_video.is_streaming field
2025-08-13 21:24 [PATCH v2 0/2] media: nxp: imx8-isi: fix streaming cleanup on release Laurent Pinchart
2025-08-13 21:24 ` [PATCH v2 1/2] media: nxp: imx8-isi: Fix " Laurent Pinchart
@ 2025-08-13 21:24 ` Laurent Pinchart
2025-08-13 22:40 ` Richard Leitner
2025-08-13 21:46 ` [PATCH v2 0/2] media: nxp: imx8-isi: fix streaming cleanup on release Richard Leitner
2 siblings, 1 reply; 7+ messages in thread
From: Laurent Pinchart @ 2025-08-13 21:24 UTC (permalink / raw)
To: linux-media
Cc: Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Richard Leitner, imx
The mxc_isi_video.is_streaming field is used to track the streaming
status of the video device. The same information is also tracked by the
videobuf2 queue. Drop the is_streaming field, and check the queue
streaming status instead.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h | 3 +--
drivers/media/platform/nxp/imx8-isi/imx8-isi-video.c | 8 ++------
2 files changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
index 206995bedca4..d86f5ede0c0e 100644
--- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
+++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
@@ -202,9 +202,8 @@ struct mxc_isi_video {
struct video_device vdev;
struct media_pad pad;
- /* Protects is_streaming, and the vdev and vb2_q operations */
+ /* Protects the vdev and vb2_q operations */
struct mutex lock;
- bool is_streaming;
struct v4l2_pix_format_mplane pix;
const struct mxc_isi_format_info *fmtinfo;
diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-video.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-video.c
index 042b554d2775..13682bf6e9f8 100644
--- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-video.c
+++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-video.c
@@ -969,8 +969,6 @@ static int mxc_isi_vb2_prepare_streaming(struct vb2_queue *q)
if (ret)
goto err_stop;
- video->is_streaming = true;
-
return 0;
err_stop:
@@ -1035,8 +1033,6 @@ static void mxc_isi_vb2_unprepare_streaming(struct vb2_queue *q)
mxc_isi_video_free_discard_buffers(video);
video_device_pipeline_stop(&video->vdev);
mxc_isi_pipe_release(video->pipe);
-
- video->is_streaming = false;
}
static const struct vb2_ops mxc_isi_vb2_qops = {
@@ -1317,7 +1313,7 @@ void mxc_isi_video_suspend(struct mxc_isi_pipe *pipe)
{
struct mxc_isi_video *video = &pipe->video;
- if (!video->is_streaming)
+ if (!vb2_is_streaming(&video->vb2_q))
return;
mxc_isi_pipe_disable(pipe);
@@ -1348,7 +1344,7 @@ int mxc_isi_video_resume(struct mxc_isi_pipe *pipe)
{
struct mxc_isi_video *video = &pipe->video;
- if (!video->is_streaming)
+ if (!vb2_is_streaming(&video->vb2_q))
return 0;
mxc_isi_video_init_channel(video);
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2 0/2] media: nxp: imx8-isi: fix streaming cleanup on release
2025-08-13 21:24 [PATCH v2 0/2] media: nxp: imx8-isi: fix streaming cleanup on release Laurent Pinchart
2025-08-13 21:24 ` [PATCH v2 1/2] media: nxp: imx8-isi: Fix " Laurent Pinchart
2025-08-13 21:24 ` [PATCH v2 2/2] media: nxp: imx8-isi: Drop mxc_isi_video.is_streaming field Laurent Pinchart
@ 2025-08-13 21:46 ` Richard Leitner
2025-08-13 21:49 ` Laurent Pinchart
2 siblings, 1 reply; 7+ messages in thread
From: Richard Leitner @ 2025-08-13 21:46 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team,
Fabio Estevam, imx
Hi Laurent,
On Thu, Aug 14, 2025 at 12:24:49AM +0300, Laurent Pinchart wrote:
> Hello,
>
> This patch series fixes an issue initially reported by Richard in [1],
> with a proposed fix. I've recommended an alternative approach, and gave
> it a try.
Thanks for reviewing that patch of mine [1].
And thank you for providing this alternative approach. It looks way
cleaner now :-) I previously tought of a similar approach as yours,
but then opted for the "less invasive" one. So again what learned for
me. Thanks!
As I've never had such a situation and wasn't able to find something in
the docs [2] about it: Do you need/want a Reviewed or Tested-By tag on
patch 1/2 of this series? (Altough there's also a SoB from me before
your Co-Developed tag)
[2] https://www.kernel.org/doc/html/latest/process/submitting-patches.html
regards;rl
>
> Patch 1/2 fixes streaming cleanup on release. Patch 2/2 then cleans up
> the driver a bit by dropping an unneeded structure field.
>
> The changes have been tested by opening and closing the video device
> while streaming in another process. I have also tested suspend/resume
> during streaming.
>
> [1] https://lore.kernel.org/linux-media/20250709-imx8-isi-release-fix-v1-1-c47c659ce1a6@linux.dev/
>
> Laurent Pinchart (1):
> media: nxp: imx8-isi: Drop mxc_isi_video.is_streaming field
>
> Richard Leitner (1):
> media: nxp: imx8-isi: Fix streaming cleanup on release
>
> .../platform/nxp/imx8-isi/imx8-isi-core.h | 3 +-
> .../platform/nxp/imx8-isi/imx8-isi-video.c | 156 +++++++-----------
> 2 files changed, 57 insertions(+), 102 deletions(-)
>
>
> base-commit: 078f1a7eb48eef9b3cb78bcd2254356f3a332358
> --
> Regards,
>
> Laurent Pinchart
>
^ permalink raw reply [flat|nested] 7+ messages in thread