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>
Best regards,
Yoshihiro Shimoda
> ---
> 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
