lkml.org 
[lkml]   [2020]   [Jul]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 10/11] media: exynos4-is: Prevent duplicate call to media_pipeline_stop
    On Sat, Jul 11, 2020 at 8:17 PM Jonathan Bakker <xc-racer2@live.ca> wrote:
    >
    > Hi Tomasz,
    >
    > On 2020-07-07 11:44 a.m., Tomasz Figa wrote:
    > > Hi Jonathan,
    > >
    > > On Sat, Apr 25, 2020 at 07:26:49PM -0700, Jonathan Bakker wrote:
    > >> media_pipeline_stop can be called from both release and streamoff,
    > >> so make sure they're both protected under the streaming flag and
    > >> not just one of them.
    > >
    > > First of all, thanks for the patch.
    > >
    > > Shouldn't it be that release calls streamoff, so that only streamoff
    > > is supposed to have the call to media_pipeline_stop()?
    > >
    >
    > I can't say that I understand the whole media subsystem enough to know :)
    > Since media_pipeline_start is called in streamon, it makes sense that streamoff
    > should have the media_pipeline_stop call. However, even after removing the call
    > in fimc_capture_release I'm still getting a backtrace such as
    >
    > [ 73.843117] ------------[ cut here ]------------
    > [ 73.843251] WARNING: CPU: 0 PID: 1575 at drivers/media/mc/mc-entity.c:554 media_pipeline_stop+0x20/0x2c [mc]
    > [ 73.843265] Modules linked in: s5p_fimc v4l2_fwnode exynos4_is_common videobuf2_dma_contig pvrsrvkm_s5pv210_sgx540_120 videobuf2_memops v4l2_mem2mem brcmfmac videobuf2_v4l2 videobuf2_common hci_uart sha256_generic libsha256 btbcm bluetooth cfg80211 brcmutil ecdh_generic ecc ce147 libaes s5ka3dfx videodev atmel_mxt_ts mc pwm_vibra rtc_max8998
    > [ 73.843471] CPU: 0 PID: 1575 Comm: v4l2-ctl Not tainted 5.7.0-14534-g2b33418b254e-dirty #669
    > [ 73.843487] Hardware name: Samsung S5PC110/S5PV210-based board
    > [ 73.843562] [<c010c7c4>] (unwind_backtrace) from [<c010a120>] (show_stack+0x10/0x14)
    > [ 73.843613] [<c010a120>] (show_stack) from [<c0117038>] (__warn+0xbc/0xd4)
    > [ 73.843661] [<c0117038>] (__warn) from [<c01170b0>] (warn_slowpath_fmt+0x60/0xb8)
    > [ 73.843734] [<c01170b0>] (warn_slowpath_fmt) from [<bf00c20c>] (media_pipeline_stop+0x20/0x2c [mc])
    > [ 73.843867] [<bf00c20c>] (media_pipeline_stop [mc]) from [<bf145c48>] (fimc_cap_streamoff+0x38/0x48 [s5p_fimc])
    > [ 73.844109] [<bf145c48>] (fimc_cap_streamoff [s5p_fimc]) from [<bf03cbf4>] (__video_do_ioctl+0x220/0x448 [videodev])
    > [ 73.844308] [<bf03cbf4>] (__video_do_ioctl [videodev]) from [<bf03d600>] (video_usercopy+0x114/0x498 [videodev])
    > [ 73.844438] [<bf03d600>] (video_usercopy [videodev]) from [<c0205024>] (ksys_ioctl+0x20c/0xa10)
    > [ 73.844484] [<c0205024>] (ksys_ioctl) from [<c0100060>] (ret_fast_syscall+0x0/0x54)
    > [ 73.844505] Exception stack(0xe5083fa8 to 0xe5083ff0)
    > [ 73.844546] 3fa0: 0049908d bef8f8c0 00000003 40045613 bef8d5ac 004c1d16
    > [ 73.844590] 3fc0: 0049908d bef8f8c0 bef8f8c0 00000036 bef8d5ac 00000000 b6d6b320 bef8faf8
    > [ 73.844620] 3fe0: 004e3ed4 bef8c718 004990bb b6f00d0a
    > [ 73.844642] ---[ end trace e6a4a8b2f20addd4 ]---
    >
    > The command I'm using for testing is
    >
    > v4l2-ctl --verbose -d 1 --stream-mmap=3 --stream-skip=2 --stream-to=./test.yuv --stream-count=1
    >
    > Since I noticed that the streaming flag was being checked fimc_capture_release
    > but not in fimc_cap_streamoff, I assumed that it was simply a missed check. Comparing
    > with other drivers, they seem to call media_pipeline_stop in their vb2_ops stop_streaming
    > callback.

    vb2 does a lot of state handling internally and makes sure that driver
    ops are not called when unnecessary, preventing double calls for
    example. I suppose it could be a better place to stop the pipeline
    indeed. However, ...

    >
    > I'm willing to test various options
    >

    I think it could make sense to add something like WARN_ON(1) inside
    media_pipeline_stop() and then check where the first call came from.

    Best regards,
    Tomasz

    \
     
     \ /
      Last update: 2020-07-20 15:11    [W:3.290 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site