Re: [PATCH 2/2] media: video-mux: update driver to active state
From: Jacopo Mondi
Date: Wed Aug 02 2023 - 09:15:43 EST
Hello
On Wed, May 24, 2023 at 03:29:25PM +0200, Philipp Zabel wrote:
> Drop the open coded pad format array, use subdev active state instead.
>
> Signed-off-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
I get a WARN with this patch applied when testing iMX6Q with OV5640
media-ctl --links ''\''imx6-mipi-csi2'\'':1->'\''ipu1_csi0_mux'\'':0[1]'
[ 85.377504]
[ 85.379019] ======================================================
[ 85.385208] WARNING: possible circular locking dependency detected
[ 85.391397] 6.4.0-rc5-00109-gd716bd480968 #2 Not tainted
[ 85.396719] ------------------------------------------------------
[ 85.402905] media-ctl/199 is trying to acquire lock:
[ 85.407879] c397d6b8 (video_mux:448:sd->active_state->lock){+.+.}-{3:3}, at: video_mux_link_setup+0xac/0x260
[ 85.417770]
[ 85.417770] but task is already holding lock:
[ 85.423609] c2fcb780 (&vmux->lock){+.+.}-{3:3}, at: video_mux_link_setup+0x48/0x260
[ 85.431305]
[ 85.431305] which lock already depends on the new lock.
[ 85.431305]
[ 85.439489]
[ 85.439489] the existing dependency chain (in reverse order) is:
[ 85.446978]
[ 85.446978] -> #1 (&vmux->lock){+.+.}-{3:3}:
[ 85.452754] __mutex_lock+0x84/0x8c4
[ 85.456875] mutex_lock_nested+0x1c/0x24
[ 85.461336] video_mux_init_cfg+0x1c/0xe0
[ 85.465883] __v4l2_subdev_state_alloc+0xbc/0x138
[ 85.471135] __v4l2_subdev_init_finalize+0xc/0x20
[ 85.476379] video_mux_probe+0x190/0x320
[ 85.480838] platform_probe+0x58/0xb0
[ 85.485036] really_probe+0xd8/0x3fc
[ 85.489152] __driver_probe_device+0x94/0x1f0
[ 85.494047] driver_probe_device+0x2c/0xc4
[ 85.498679] __device_attach_driver+0xa4/0x11c
[ 85.503663] bus_for_each_drv+0x84/0xdc
[ 85.508036] __device_attach+0xa8/0x1dc
[ 85.512409] bus_probe_device+0x8c/0x90
[ 85.516782] deferred_probe_work_func+0x88/0xd4
[ 85.521850] process_one_work+0x2a4/0x6a0
[ 85.526399] worker_thread+0x28/0x4ac
[ 85.530596] kthread+0xf4/0x114
[ 85.534272] ret_from_fork+0x14/0x28
[ 85.538382]
[ 85.538382] -> #0 (video_mux:448:sd->active_state->lock){+.+.}-{3:3}:
[ 85.546328] __lock_acquire+0x14b8/0x28e8
[ 85.550873] lock_acquire.part.0+0xb4/0x250
[ 85.555591] __mutex_lock+0x84/0x8c4
[ 85.559704] mutex_lock_nested+0x1c/0x24
[ 85.564163] video_mux_link_setup+0xac/0x260
[ 85.568969] __media_entity_setup_link+0x14c/0x1b4
[ 85.574305] media_device_ioctl+0x158/0x190
[ 85.579027] sys_ioctl+0x4a8/0xee8
[ 85.582971] ret_fast_syscall+0x0/0x1c
[ 85.587253]
[ 85.587253] other info that might help us debug this:
[ 85.587253]
[ 85.595263] Possible unsafe locking scenario:
[ 85.595263]
[ 85.601186] CPU0 CPU1
[ 85.605722] ---- ----
[ 85.610258] lock(&vmux->lock);
[ 85.613501] lock(video_mux:448:sd->active_state->lock);
[ 85.621434] lock(&vmux->lock);
[ 85.627195] lock(video_mux:448:sd->active_state->lock);
[ 85.632609]
[ 85.632609] *** DEADLOCK ***
[ 85.632609]
[ 85.638535] 2 locks held by media-ctl/199:
[ 85.642640] #0: c3856174 (&mdev->graph_mutex){+.+.}-{3:3}, at: media_device_ioctl+0x148/0x190
[ 85.651299] #1: c2fcb780 (&vmux->lock){+.+.}-{3:3}, at: video_mux_link_setup+0x48/0x260
[ 85.659434]
[ 85.659434] stack backtrace:
[ 85.663800] CPU: 1 PID: 199 Comm: media-ctl Not tainted 6.4.0-rc5-00109-gd716bd480968 #2
[ 85.671904] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[ 85.678445] unwind_backtrace from show_stack+0x10/0x14
[ 85.683700] show_stack from dump_stack_lvl+0x60/0x90
[ 85.688769] dump_stack_lvl from check_noncircular+0xf4/0x164
[ 85.694547] check_noncircular from __lock_acquire+0x14b8/0x28e8
[ 85.700576] __lock_acquire from lock_acquire.part.0+0xb4/0x250
[ 85.706510] lock_acquire.part.0 from __mutex_lock+0x84/0x8c4
[ 85.712279] __mutex_lock from mutex_lock_nested+0x1c/0x24
[ 85.717791] mutex_lock_nested from video_mux_link_setup+0xac/0x260
[ 85.724085] video_mux_link_setup from __media_entity_setup_link+0x14c/0x1b4
[ 85.731164] __media_entity_setup_link from media_device_ioctl+0x158/0x190
[ 85.738063] media_device_ioctl from sys_ioctl+0x4a8/0xee8
[ 85.743577] sys_ioctl from ret_fast_syscall+0x0/0x1c
[ 85.748649] Exception stack(0xe0f41fa8 to 0xe0f41ff0)
[ 85.753716] 1fa0: 00e233b8 00000079 00000003 c0347c03 bef629d0 00000001
[ 85.761908] 1fc0: 00e233b8 00000079 00e21150 00000036 bef629d0 00e23230 00000001 00000000
[ 85.770096] 1fe0: 004d9f5c bef629cc 004c37c8 b6e79f4c
Please note that in my tree I have reverted
"media: v4l2-async: Use endpoints in __v4l2_async_nf_add_fwnode_remote()"
to have the imx6 media graph to probe correctly, but I presume this is
not related to the error here above.
Thanks
j
> ---
> drivers/media/platform/video-mux.c | 78 ++++++++++++--------------------------
> 1 file changed, 24 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
> index 4fe31386afc7..6d273abfe16c 100644
> --- a/drivers/media/platform/video-mux.c
> +++ b/drivers/media/platform/video-mux.c
> @@ -24,7 +24,6 @@ struct video_mux {
> struct v4l2_subdev subdev;
> struct v4l2_async_notifier notifier;
> struct media_pad *pads;
> - struct v4l2_mbus_framefmt *format_mbus;
> struct mux_control *mux;
> struct mutex lock;
> int active;
> @@ -71,6 +70,9 @@ static int video_mux_link_setup(struct media_entity *entity,
> mutex_lock(&vmux->lock);
>
> if (flags & MEDIA_LNK_FL_ENABLED) {
> + struct v4l2_subdev_state *sd_state;
> + struct v4l2_mbus_framefmt *source_mbusformat;
> +
> if (vmux->active == local->index)
> goto out;
>
> @@ -86,7 +88,12 @@ static int video_mux_link_setup(struct media_entity *entity,
> vmux->active = local->index;
>
> /* Propagate the active format to the source */
> - vmux->format_mbus[source_pad] = vmux->format_mbus[vmux->active];
> + sd_state = v4l2_subdev_lock_and_get_active_state(sd);
> + source_mbusformat = v4l2_subdev_get_pad_format(sd, sd_state,
> + source_pad);
> + *source_mbusformat = *v4l2_subdev_get_pad_format(sd, sd_state,
> + vmux->active);
> + v4l2_subdev_unlock_state(sd_state);
> } else {
> if (vmux->active != local->index)
> goto out;
> @@ -138,40 +145,6 @@ static const struct v4l2_subdev_video_ops video_mux_subdev_video_ops = {
> .s_stream = video_mux_s_stream,
> };
>
> -static struct v4l2_mbus_framefmt *
> -__video_mux_get_pad_format(struct v4l2_subdev *sd,
> - struct v4l2_subdev_state *sd_state,
> - unsigned int pad, u32 which)
> -{
> - struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> -
> - switch (which) {
> - case V4L2_SUBDEV_FORMAT_TRY:
> - return v4l2_subdev_get_try_format(sd, sd_state, pad);
> - case V4L2_SUBDEV_FORMAT_ACTIVE:
> - return &vmux->format_mbus[pad];
> - default:
> - return NULL;
> - }
> -}
> -
> -static int video_mux_get_format(struct v4l2_subdev *sd,
> - struct v4l2_subdev_state *sd_state,
> - struct v4l2_subdev_format *sdformat)
> -{
> - struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> -
> - mutex_lock(&vmux->lock);
> -
> - sdformat->format = *__video_mux_get_pad_format(sd, sd_state,
> - sdformat->pad,
> - sdformat->which);
> -
> - mutex_unlock(&vmux->lock);
> -
> - return 0;
> -}
> -
> static int video_mux_set_format(struct v4l2_subdev *sd,
> struct v4l2_subdev_state *sd_state,
> struct v4l2_subdev_format *sdformat)
> @@ -181,14 +154,11 @@ static int video_mux_set_format(struct v4l2_subdev *sd,
> struct media_pad *pad = &vmux->pads[sdformat->pad];
> u16 source_pad = sd->entity.num_pads - 1;
>
> - mbusformat = __video_mux_get_pad_format(sd, sd_state, sdformat->pad,
> - sdformat->which);
> + mbusformat = v4l2_subdev_get_pad_format(sd, sd_state, sdformat->pad);
> if (!mbusformat)
> return -EINVAL;
>
> - source_mbusformat = __video_mux_get_pad_format(sd, sd_state,
> - source_pad,
> - sdformat->which);
> + source_mbusformat = v4l2_subdev_get_pad_format(sd, sd_state, source_pad);
> if (!source_mbusformat)
> return -EINVAL;
>
> @@ -298,7 +268,8 @@ static int video_mux_set_format(struct v4l2_subdev *sd,
>
> /* Source pad mirrors active sink pad, no limitations on sink pads */
> if ((pad->flags & MEDIA_PAD_FL_SOURCE) && vmux->active >= 0)
> - sdformat->format = vmux->format_mbus[vmux->active];
> + sdformat->format = *v4l2_subdev_get_pad_format(sd, sd_state,
> + vmux->active);
>
> *mbusformat = sdformat->format;
>
> @@ -321,7 +292,7 @@ static int video_mux_init_cfg(struct v4l2_subdev *sd,
> mutex_lock(&vmux->lock);
>
> for (i = 0; i < sd->entity.num_pads; i++) {
> - mbusformat = v4l2_subdev_get_try_format(sd, sd_state, i);
> + mbusformat = v4l2_subdev_get_pad_format(sd, sd_state, i);
> *mbusformat = video_mux_format_mbus_default;
> }
>
> @@ -332,7 +303,7 @@ static int video_mux_init_cfg(struct v4l2_subdev *sd,
>
> static const struct v4l2_subdev_pad_ops video_mux_pad_ops = {
> .init_cfg = video_mux_init_cfg,
> - .get_fmt = video_mux_get_format,
> + .get_fmt = v4l2_subdev_get_fmt,
> .set_fmt = video_mux_set_format,
> };
>
> @@ -462,17 +433,9 @@ static int video_mux_probe(struct platform_device *pdev)
> if (!vmux->pads)
> return -ENOMEM;
>
> - vmux->format_mbus = devm_kcalloc(dev, num_pads,
> - sizeof(*vmux->format_mbus),
> - GFP_KERNEL);
> - if (!vmux->format_mbus)
> - return -ENOMEM;
> -
> - for (i = 0; i < num_pads; i++) {
> + for (i = 0; i < num_pads; i++)
> vmux->pads[i].flags = (i < num_pads - 1) ? MEDIA_PAD_FL_SINK
> : MEDIA_PAD_FL_SOURCE;
> - vmux->format_mbus[i] = video_mux_format_mbus_default;
> - }
>
> vmux->subdev.entity.function = MEDIA_ENT_F_VID_MUX;
> ret = media_entity_pads_init(&vmux->subdev.entity, num_pads,
> @@ -482,12 +445,18 @@ static int video_mux_probe(struct platform_device *pdev)
>
> vmux->subdev.entity.ops = &video_mux_ops;
>
> + ret = v4l2_subdev_init_finalize(&vmux->subdev);
> + if (ret < 0)
> + goto err_entity_cleanup;
> +
> ret = video_mux_async_register(vmux, num_pads - 1);
> if (ret)
> - goto err_entity_cleanup;
> + goto err_subdev_cleanup;
>
> return 0;
>
> +err_subdev_cleanup:
> + v4l2_subdev_cleanup(&vmux->subdev);
> err_entity_cleanup:
> media_entity_cleanup(&vmux->subdev.entity);
> return ret;
> @@ -501,6 +470,7 @@ static void video_mux_remove(struct platform_device *pdev)
> v4l2_async_nf_unregister(&vmux->notifier);
> v4l2_async_nf_cleanup(&vmux->notifier);
> v4l2_async_unregister_subdev(sd);
> + v4l2_subdev_cleanup(sd);
> media_entity_cleanup(&sd->entity);
> }
>
>
> --
> 2.39.2