* [PATCH v2] media: v4l2-subdev: Document and enforce .s_stream() requirements
@ 2023-09-18 12:48 Laurent Pinchart
2023-10-24 8:57 ` Geert Uytterhoeven
0 siblings, 1 reply; 3+ messages in thread
From: Laurent Pinchart @ 2023-09-18 12:48 UTC (permalink / raw)
To: linux-media
Cc: Sakari Ailus, Martin Kepplinger, Ricardo Ribalda, Dave Stevenson,
Bingbu Cao, Paul J. Murphy, Daniele Alessandrelli, Tianshu Qiu,
Jimmy Su, Jason Chen, Arec Kao, Shunqian Zheng, Jacopo Mondi,
Mikhail Rudenko
The subdev .s_stream() operation must not be called to start an already
started subdev, or stop an already stopped one. This requirement has
never been formally documented. Fix it, and catch possible offenders
with a WARN_ON() in the call_s_stream() wrapper.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:
- Add WARN_ON() in call_s_stream()
- Fix typo and language in documentation
---
I'm resending this patch individually to avoid spamming the list with
the 56 other patches included in v1. You can find the original series at
https://lore.kernel.org/linux-media/20230914181704.4811-1-laurent.pinchart@ideasonboard.com
---
drivers/media/v4l2-core/v4l2-subdev.c | 17 ++++++++++++++++-
include/media/v4l2-subdev.h | 4 +++-
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index b92348ad61f6..32b7d9cd43e6 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -359,6 +359,18 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
{
int ret;
+ /*
+ * The .s_stream() operation must never be called to start or stop an
+ * already started or stopped subdev. Catch offenders but don't return
+ * an error yet to avoid regressions.
+ *
+ * As .s_stream() is mutually exclusive with the .enable_streams() and
+ * .disable_streams() operation, we can use the enabled_streams field
+ * to store the subdev streaming state.
+ */
+ if (WARN_ON(!!sd->enabled_streams == !!enable))
+ return 0;
+
#if IS_REACHABLE(CONFIG_LEDS_CLASS)
if (!IS_ERR_OR_NULL(sd->privacy_led)) {
if (enable)
@@ -372,9 +384,12 @@ static int call_s_stream(struct v4l2_subdev *sd, int enable)
if (!enable && ret < 0) {
dev_warn(sd->dev, "disabling streaming failed (%d)\n", ret);
- return 0;
+ ret = 0;
}
+ if (!ret)
+ sd->enabled_streams = enable ? BIT(0) : 0;
+
return ret;
}
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index d9fca929c10b..ab2a7ef61d42 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -446,7 +446,9 @@ enum v4l2_subdev_pre_streamon_flags {
* @s_stream: start (enabled == 1) or stop (enabled == 0) streaming on the
* sub-device. Failure on stop will remove any resources acquired in
* streaming start, while the error code is still returned by the driver.
- * Also see call_s_stream wrapper in v4l2-subdev.c.
+ * The caller shall track the subdev state, and shall not start or stop an
+ * already started or stopped subdev. Also see call_s_stream wrapper in
+ * v4l2-subdev.c.
*
* @g_pixelaspect: callback to return the pixelaspect ratio.
*
base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] media: v4l2-subdev: Document and enforce .s_stream() requirements
2023-09-18 12:48 [PATCH v2] media: v4l2-subdev: Document and enforce .s_stream() requirements Laurent Pinchart
@ 2023-10-24 8:57 ` Geert Uytterhoeven
2023-10-24 14:27 ` Laurent Pinchart
0 siblings, 1 reply; 3+ messages in thread
From: Geert Uytterhoeven @ 2023-10-24 8:57 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, Sakari Ailus, Martin Kepplinger, Ricardo Ribalda,
Dave Stevenson, Bingbu Cao, Paul J. Murphy,
Daniele Alessandrelli, Tianshu Qiu, Jimmy Su, Jason Chen,
Arec Kao, Shunqian Zheng, Jacopo Mondi, Mikhail Rudenko,
linux-renesas-soc
Hi Laurent,
On Mon, 18 Sep 2023, Laurent Pinchart wrote:
> The subdev .s_stream() operation must not be called to start an already
> started subdev, or stop an already stopped one. This requirement has
> never been formally documented. Fix it, and catch possible offenders
> with a WARN_ON() in the call_s_stream() wrapper.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v1:
>
> - Add WARN_ON() in call_s_stream()
> - Fix typo and language in documentation
Thanks for your patch, which is now commit 009905ec50433259 ("media:
v4l2-subdev: Document and enforce .s_stream() requirements")
in linux-next/master media/master.
This patch causes the below warning during s2idle/s2ram on Salvator-XS
(R-Car H3 ES2.0) and White-Hawk (R-Car V4H). Koelsch (R-Car M2-W) is not
affected, as its DU does not use the VSP.
Filesystems sync: 0.014 seconds
Freezing user space processes
Freezing user space processes completed (elapsed 0.006 seconds)
OOM killer disabled.
Freezing remaining freezable tasks
Freezing remaining freezable tasks completed (elapsed 0.002 seconds)
------------[ cut here ]------------
WARNING: CPU: 2 PID: 962 at drivers/media/v4l2-core/v4l2-subdev.c:371 call_s_stream+0xd4/0xf0
CPU: 2 PID: 962 Comm: s2idle Not tainted 6.6.0-rc3-arm64-renesas-00068-g009905ec5043 #2302
Hardware name: Renesas Salvator-X 2nd version board based on r8a77951 (DT)
pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : call_s_stream+0xd4/0xf0
lr : vsp1_pipeline_stop+0x10c/0x2a4
sp : ffff800083f635f0
x29: ffff800083f635f0 x28: 0000000000000000 x27: 0000000000000001
x26: 0000000000000000 x25: ffff0004c3375b08 x24: ffff0004c3375880
x23: 0000000000000288 x22: ffff0004c3375b30 x21: 0000000000000000
x20: ffff0004c3370880 x19: ffff0004c33750f0 x18: 0000000000000001
x17: 000000040044ffff x16: 00000034b5503510 x15: 0000000000000028
x14: 0000000000000000 x13: 0000000000000003 x12: 0000000000000000
x11: 0000000000000000 x10: ffff800082063b78 x9 : ffff800081a5abb8
x8 : 00000000e50359b8 x7 : 00000000000000ac x6 : 0000000000002b10
x5 : 0000000000000000 x4 : ffff8000838f8000 x3 : ffff800080932760
x2 : ffff80008096671c x1 : 0000000000000000 x0 : 0000000000000001
Call trace:
call_s_stream+0xd4/0xf0
vsp1_pipeline_stop+0x10c/0x2a4
vsp1_du_setup_lif+0x324/0x468
rcar_du_vsp_disable+0x1c/0x24
rcar_du_crtc_atomic_disable+0x2f8/0x438
disable_outputs+0x22c/0x338
drm_atomic_helper_commit_modeset_disables+0x18/0x44
rcar_du_atomic_commit_tail+0x90/0xd8
commit_tail+0x9c/0x178
drm_atomic_helper_commit+0x18c/0x1a0
drm_atomic_commit+0xa4/0xd8
drm_atomic_helper_disable_all+0x1ec/0x200
drm_atomic_helper_suspend+0xa0/0x208
drm_mode_config_helper_suspend+0x2c/0x70
rcar_du_pm_suspend+0x14/0x1c
platform_pm_suspend+0x28/0x64
dpm_run_callback+0x34/0x90
__device_suspend+0x10c/0x3c4
dpm_suspend+0x140/0x250
dpm_suspend_start+0x78/0x90
suspend_devices_and_enter+0x124/0x570
pm_suspend+0x1ec/0x310
state_store+0x88/0x124
kobj_attr_store+0x14/0x24
sysfs_kf_write+0x48/0x6c
kernfs_fop_write_iter+0x118/0x1a8
vfs_write+0x208/0x30c
ksys_write+0x64/0xec
__arm64_sys_write+0x18/0x20
invoke_syscall+0x44/0x108
el0_svc_common.constprop.0+0x3c/0xdc
do_el0_svc+0x1c/0x24
el0_svc+0x40/0xd4
el0t_64_sync_handler+0x134/0x150
el0t_64_sync+0x14c/0x150
irq event stamp: 19852
hardirqs last enabled at (19851): [<ffff800080dc3234>] _raw_spin_unlock_irqrestore+0x6c/0x70
hardirqs last disabled at (19852): [<ffff800080db3f9c>] el1_dbg+0x20/0x80
softirqs last enabled at (17806): [<ffff800080010618>] __do_softirq+0x430/0x4e0
softirqs last disabled at (17799): [<ffff8000800151e0>] ____do_softirq+0xc/0x14
---[ end trace 0000000000000000 ]---
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] media: v4l2-subdev: Document and enforce .s_stream() requirements
2023-10-24 8:57 ` Geert Uytterhoeven
@ 2023-10-24 14:27 ` Laurent Pinchart
0 siblings, 0 replies; 3+ messages in thread
From: Laurent Pinchart @ 2023-10-24 14:27 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: linux-media, Sakari Ailus, Martin Kepplinger, Ricardo Ribalda,
Dave Stevenson, Bingbu Cao, Paul J. Murphy,
Daniele Alessandrelli, Tianshu Qiu, Jimmy Su, Jason Chen,
Arec Kao, Shunqian Zheng, Jacopo Mondi, Mikhail Rudenko,
linux-renesas-soc
Hi Geert,
On Tue, Oct 24, 2023 at 10:57:26AM +0200, Geert Uytterhoeven wrote:
> Hi Laurent,
>
> On Mon, 18 Sep 2023, Laurent Pinchart wrote:
> > The subdev .s_stream() operation must not be called to start an already
> > started subdev, or stop an already stopped one. This requirement has
> > never been formally documented. Fix it, and catch possible offenders
> > with a WARN_ON() in the call_s_stream() wrapper.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v1:
> >
> > - Add WARN_ON() in call_s_stream()
> > - Fix typo and language in documentation
>
> Thanks for your patch, which is now commit 009905ec50433259 ("media:
> v4l2-subdev: Document and enforce .s_stream() requirements")
> in linux-next/master media/master.
>
> This patch causes the below warning during s2idle/s2ram on Salvator-XS
> (R-Car H3 ES2.0) and White-Hawk (R-Car V4H). Koelsch (R-Car M2-W) is not
> affected, as its DU does not use the VSP.
Oops :-S
https://lore.kernel.org/linux-media/20231024142522.29658-1-laurent.pinchart+renesas@ideasonboard.com/T/#u
should fix it.
> Filesystems sync: 0.014 seconds
> Freezing user space processes
> Freezing user space processes completed (elapsed 0.006 seconds)
> OOM killer disabled.
> Freezing remaining freezable tasks
> Freezing remaining freezable tasks completed (elapsed 0.002 seconds)
> ------------[ cut here ]------------
> WARNING: CPU: 2 PID: 962 at drivers/media/v4l2-core/v4l2-subdev.c:371 call_s_stream+0xd4/0xf0
> CPU: 2 PID: 962 Comm: s2idle Not tainted 6.6.0-rc3-arm64-renesas-00068-g009905ec5043 #2302
> Hardware name: Renesas Salvator-X 2nd version board based on r8a77951 (DT)
> pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> pc : call_s_stream+0xd4/0xf0
> lr : vsp1_pipeline_stop+0x10c/0x2a4
> sp : ffff800083f635f0
> x29: ffff800083f635f0 x28: 0000000000000000 x27: 0000000000000001
> x26: 0000000000000000 x25: ffff0004c3375b08 x24: ffff0004c3375880
> x23: 0000000000000288 x22: ffff0004c3375b30 x21: 0000000000000000
> x20: ffff0004c3370880 x19: ffff0004c33750f0 x18: 0000000000000001
> x17: 000000040044ffff x16: 00000034b5503510 x15: 0000000000000028
> x14: 0000000000000000 x13: 0000000000000003 x12: 0000000000000000
> x11: 0000000000000000 x10: ffff800082063b78 x9 : ffff800081a5abb8
> x8 : 00000000e50359b8 x7 : 00000000000000ac x6 : 0000000000002b10
> x5 : 0000000000000000 x4 : ffff8000838f8000 x3 : ffff800080932760
> x2 : ffff80008096671c x1 : 0000000000000000 x0 : 0000000000000001
> Call trace:
> call_s_stream+0xd4/0xf0
> vsp1_pipeline_stop+0x10c/0x2a4
> vsp1_du_setup_lif+0x324/0x468
> rcar_du_vsp_disable+0x1c/0x24
> rcar_du_crtc_atomic_disable+0x2f8/0x438
> disable_outputs+0x22c/0x338
> drm_atomic_helper_commit_modeset_disables+0x18/0x44
> rcar_du_atomic_commit_tail+0x90/0xd8
> commit_tail+0x9c/0x178
> drm_atomic_helper_commit+0x18c/0x1a0
> drm_atomic_commit+0xa4/0xd8
> drm_atomic_helper_disable_all+0x1ec/0x200
> drm_atomic_helper_suspend+0xa0/0x208
> drm_mode_config_helper_suspend+0x2c/0x70
> rcar_du_pm_suspend+0x14/0x1c
> platform_pm_suspend+0x28/0x64
> dpm_run_callback+0x34/0x90
> __device_suspend+0x10c/0x3c4
> dpm_suspend+0x140/0x250
> dpm_suspend_start+0x78/0x90
> suspend_devices_and_enter+0x124/0x570
> pm_suspend+0x1ec/0x310
> state_store+0x88/0x124
> kobj_attr_store+0x14/0x24
> sysfs_kf_write+0x48/0x6c
> kernfs_fop_write_iter+0x118/0x1a8
> vfs_write+0x208/0x30c
> ksys_write+0x64/0xec
> __arm64_sys_write+0x18/0x20
> invoke_syscall+0x44/0x108
> el0_svc_common.constprop.0+0x3c/0xdc
> do_el0_svc+0x1c/0x24
> el0_svc+0x40/0xd4
> el0t_64_sync_handler+0x134/0x150
> el0t_64_sync+0x14c/0x150
> irq event stamp: 19852
> hardirqs last enabled at (19851): [<ffff800080dc3234>] _raw_spin_unlock_irqrestore+0x6c/0x70
> hardirqs last disabled at (19852): [<ffff800080db3f9c>] el1_dbg+0x20/0x80
> softirqs last enabled at (17806): [<ffff800080010618>] __do_softirq+0x430/0x4e0
> softirqs last disabled at (17799): [<ffff8000800151e0>] ____do_softirq+0xc/0x14
> ---[ end trace 0000000000000000 ]---
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-10-24 14:27 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-18 12:48 [PATCH v2] media: v4l2-subdev: Document and enforce .s_stream() requirements Laurent Pinchart
2023-10-24 8:57 ` Geert Uytterhoeven
2023-10-24 14:27 ` Laurent Pinchart
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).