Hello Shimoda-san,
On Tue, Jan 31, 2023 at 06:38:56AM +0000, Yoshihiro Shimoda wrote:
> Hi Laurent-san,
>
> > From: Laurent Pinchart, Sent: Saturday, January 21, 2023 5:44 AM
> >
> > The vsp1 driver uses the vb2_is_streaming() function in its .buf_queue()
> > handler to check if the .start_streaming() operation has been called,
> > and decide whether to just add the buffer to an internal queue, or also
> > trigger a hardware run. vb2_is_streaming() relies on the vb2_queue
> > structure's streaming field, which used to be set only after calling the
> > .start_streaming() operation.
> >
> > Commit a10b21532574 ("media: vb2: add (un)prepare_streaming queue ops")
> > changed this, setting the .streaming field in vb2_core_streamon() before
> > enqueuing buffers to the driver and calling .start_streaming(). This
> > broke the vsp1 driver which now believes that .start_streaming() has
> > been called when it hasn't, leading to a crash:
> >
> > [ 881.058705] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000020
> > [ 881.067495] Mem abort info:
> > [ 881.070290] ESR = 0x0000000096000006
> > [ 881.074042] EC = 0x25: DABT (current EL), IL = 32 bits
> > [ 881.079358] SET = 0, FnV = 0
> > [ 881.082414] EA = 0, S1PTW = 0
> > [ 881.085558] FSC = 0x06: level 2 translation fault
> > [ 881.090439] Data abort info:
> > [ 881.093320] ISV = 0, ISS = 0x00000006
> > [ 881.097157] CM = 0, WnR = 0
> > [ 881.100126] user pgtable: 4k pages, 48-bit VAs, pgdp=000000004fa51000
> > [ 881.106573] [0000000000000020] pgd=080000004f36e003, p4d=080000004f36e003, pud=080000004f7ec003,
> > pmd=0000000000000000
> > [ 881.117217] Internal error: Oops: 0000000096000006 [#1] PREEMPT SMP
> > [ 881.123494] Modules linked in: rcar_fdp1 v4l2_mem2mem
> > [ 881.128572] CPU: 0 PID: 1271 Comm: yavta Tainted: G B 6.2.0-rc1-00023-g6c94e2e99343 #556
> > [ 881.138061] Hardware name: Renesas Salvator-X 2nd version board based on r8a77965 (DT)
> > [ 881.145981] pstate: 400000c5 (nZcv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > [ 881.152951] pc : vsp1_dl_list_add_body+0xa8/0xe0
> > [ 881.157580] lr : vsp1_dl_list_add_body+0x34/0xe0
> > [ 881.162206] sp : ffff80000c267710
> > [ 881.165522] x29: ffff80000c267710 x28: ffff000010938ae8 x27: ffff000013a8dd98
> > [ 881.172683] x26: ffff000010938098 x25: ffff000013a8dc00 x24: ffff000010ed6ba8
> > [ 881.179841] x23: ffff00000faa4000 x22: 0000000000000000 x21: 0000000000000020
> > [ 881.186998] x20: ffff00000faa4000 x19: 0000000000000000 x18: 0000000000000000
> > [ 881.194154] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
> > [ 881.201309] x14: 0000000000000000 x13: 746e696174206c65 x12: ffff70000157043d
> > [ 881.208465] x11: 1ffff0000157043c x10: ffff70000157043c x9 : dfff800000000000
> > [ 881.215622] x8 : ffff80000ab821e7 x7 : 00008ffffea8fbc4 x6 : 0000000000000001
> > [ 881.222779] x5 : ffff80000ab821e0 x4 : ffff70000157043d x3 : 0000000000000020
> > [ 881.229936] x2 : 0000000000000020 x1 : ffff00000e4f6400 x0 : 0000000000000000
> > [ 881.237092] Call trace:
> > [ 881.239542] vsp1_dl_list_add_body+0xa8/0xe0
> > [ 881.243822] vsp1_video_pipeline_run+0x270/0x2a0
> > [ 881.248449] vsp1_video_buffer_queue+0x1c0/0x1d0
> > [ 881.253076] __enqueue_in_driver+0xbc/0x260
> > [ 881.257269] vb2_start_streaming+0x48/0x200
> > [ 881.261461] vb2_core_streamon+0x13c/0x280
> > [ 881.265565] vb2_streamon+0x3c/0x90
> > [ 881.269064] vsp1_video_streamon+0x2fc/0x3e0
> > [ 881.273344] v4l_streamon+0x50/0x70
> > [ 881.276844] __video_do_ioctl+0x2bc/0x5d0
> > [ 881.280861] video_usercopy+0x2a8/0xc80
> > [ 881.284704] video_ioctl2+0x20/0x40
> > [ 881.288201] v4l2_ioctl+0xa4/0xc0
> > [ 881.291525] __arm64_sys_ioctl+0xe8/0x110
> > [ 881.295543] invoke_syscall+0x68/0x190
> > [ 881.299303] el0_svc_common.constprop.0+0x88/0x170
> > [ 881.304105] do_el0_svc+0x4c/0xf0
> > [ 881.307430] el0_svc+0x4c/0xa0
> > [ 881.310494] el0t_64_sync_handler+0xbc/0x140
> > [ 881.314773] el0t_64_sync+0x190/0x194
> > [ 881.318450] Code: d50323bf d65f03c0 91008263 f9800071 (885f7c60)
> > [ 881.324551] ---[ end trace 0000000000000000 ]---
> > [ 881.329173] note: yavta[1271] exited with preempt_count 1
> >
> > A different regression report sent to the linux-media mailing list ([1])
> > was answered with a claim that the vb2_is_streaming() function has never
> > been meant for this purpose. The document of the function, as well as of
> > the struct vb2_queue streaming field, is sparse, so this claim may be
> > hard to verify.
> >
> > The information needed by the vsp1 driver to decide how to process
> > queued buffers is also available from the vb2_start_streaming_called()
> > function. Use it instead of vb2_is_streaming() to fix the problem.
> >
> > [1]
> <snip the URL>
> >
> > Fixes: a10b21532574 ("media: vb2: add (un)prepare_streaming queue ops")
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
>
> My team member tested this patch and confirmed that this patch fixed the issue.
> So,
>
> Tested-by: Duy Nguyen <duy.nguyen.rh@xxxxxxxxxxx>
Thank you.
A different fix has been merged in the media tree for v6.2:
commit a1550700629f30c5bd554161524f14f14600d554
Author: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
Date: Wed Dec 28 18:02:27 2022 +0100
media: v4l2-ctrls-api.c: move ctrl->is_new = 1 to the correct line
The patch that fixed string control support somehow got mangled when it was
merged in mainline: the added line ended up in the wrong place.
Fix this.
Fixes: 73278d483378 ("media: v4l2-ctrls-api.c: add back dropped ctrl->is_new = 1")
Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
This patch still makes sense, but it's not needed as a fix anymore, so
I'll submit it for v6.4 instead.
> > ---
> > Hans, I think many drivers may be affected by a10b21532574, and it would
> > be difficult to test them all in time for the v6.2 release. Maybe the
> > original behaviour of vb2_is_streaming() could be restored (I haven't
> > checked), or maybe the commit should be reverted to give more time to
> > fix the issue correctly.
> >
> > In the meantime, this patch should be merged as a v6.2 fix, as I think
> > it goes in the right direction in any case.
> > ---
> > drivers/media/platform/renesas/vsp1/vsp1_video.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_video.c b/drivers/media/platform/renesas/vsp1/vsp1_video.c
> > index 544012fd1fe9..3664c87e4afb 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_video.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_video.c
> > @@ -776,7 +776,7 @@ static void vsp1_video_buffer_queue(struct vb2_buffer *vb)
> > video->rwpf->mem = buf->mem;
> > pipe->buffers_ready |= 1 << video->pipe_index;
> >
> > - if (vb2_is_streaming(&video->queue) &&
> > + if (vb2_start_streaming_called(&video->queue) &&
> > vsp1_pipeline_ready(pipe))
> > vsp1_video_pipeline_run(pipe);
> >
> >
> > base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
--
Regards,
Laurent Pinchart
