diff mbox series

[10/11] media: exynos4-is: Prevent duplicate call to media_pipeline_stop

Message ID BN6PR04MB0660DB1C884EE9F9C7D94857A3AE0@BN6PR04MB0660.namprd04.prod.outlook.com (mailing list archive)
State Not Applicable
Headers show
Series media: exynos4-is: Improve support for s5pv210 and parallel ports | expand

Commit Message

Jonathan Bakker April 26, 2020, 2:26 a.m. UTC 
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.

This probably became noticeable after commit 2a2599c66368 ("[media] media:
entity: Catch unbalanced media_pipeline_stop calls") was merged as it
added a WARN.

Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
---
 drivers/media/platform/exynos4-is/fimc-capture.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Tomasz Figa July 7, 2020, 6:44 p.m. UTC | #1 
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()?

Best regards,
Tomasz
Jonathan Bakker July 11, 2020, 6:17 p.m. UTC | #2 
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.

I'm willing to test various options

> Best regards,
> Tomasz
> 

Thanks,
Jonathan
Tomasz Figa July 20, 2020, 1:10 p.m. UTC | #3 
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
Jonathan Bakker July 24, 2020, 11:46 p.m. UTC | #4 
Hi Tomasz,

On 2020-07-20 6:10 a.m., Tomasz Figa wrote:
> 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.

Here's the results of that:

[   69.876823] ------------[ cut here ]------------
[   69.876962] WARNING: CPU: 0 PID: 1566 at drivers/media/mc/mc-entity.c:550 __media_pipeline_stop+0x24/0xfc [mc]
[   69.876976] Modules linked in: s5p_fimc v4l2_fwnode exynos4_is_common videobuf2_dma_contig videobuf2_memops v4l2_mem2mem brcmfmac videobuf2_v4l2 pvrsrvkm_s5pv210_sgx540_120 videobuf2_common hci_uart sha256_generic btbcm libsha256 bluetooth cfg80211 ce147 brcmutil s5ka3dfx ecdh_generic ecc libaes videodev atmel_mxt_ts mc pwm_vibra rtc_max8998
[   69.877182] CPU: 0 PID: 1566 Comm: v4l2-ctl Not tainted 5.7.0-14540-gb1220848c797-dirty #681
[   69.877198] Hardware name: Samsung S5PC110/S5PV210-based board
[   69.877274] [<c010c7c4>] (unwind_backtrace) from [<c010a120>] (show_stack+0x10/0x14)
[   69.877326] [<c010a120>] (show_stack) from [<c0117038>] (__warn+0xbc/0xd4)
[   69.877375] [<c0117038>] (__warn) from [<c01170b0>] (warn_slowpath_fmt+0x60/0xb8)
[   69.877448] [<c01170b0>] (warn_slowpath_fmt) from [<bf010130>] (__media_pipeline_stop+0x24/0xfc [mc])
[   69.877540] [<bf010130>] (__media_pipeline_stop [mc]) from [<bf010228>] (media_pipeline_stop+0x20/0x2c [mc])
[   69.877663] [<bf010228>] (media_pipeline_stop [mc]) from [<bf08fc48>] (fimc_cap_streamoff+0x38/0x48 [s5p_fimc])
[   69.877904] [<bf08fc48>] (fimc_cap_streamoff [s5p_fimc]) from [<bf040bf4>] (__video_do_ioctl+0x220/0x448 [videodev])
[   69.878105] [<bf040bf4>] (__video_do_ioctl [videodev]) from [<bf041600>] (video_usercopy+0x114/0x498 [videodev])
[   69.878234] [<bf041600>] (video_usercopy [videodev]) from [<c0205024>] (ksys_ioctl+0x20c/0xa10)
[   69.878281] [<c0205024>] (ksys_ioctl) from [<c0100060>] (ret_fast_syscall+0x0/0x54)
[   69.878301] Exception stack(0xe50c1fa8 to 0xe50c1ff0)
[   69.878342] 1fa0:                   004ef08d 00539d0c 00000003 40045613 bec1578c 00517d16
[   69.878386] 1fc0: 004ef08d 00539d0c bec188c0 00000036 bec165ac 00000000 b6def320 bec18af8
[   69.878415] 1fe0: 00539ed4 bec15730 004ef0bb b6f84d0a
[   69.878436] ---[ end trace d004ab573a72c329 ]---
[   69.879704] ------------[ cut here ]------------
[   69.879794] WARNING: CPU: 0 PID: 1566 at drivers/media/mc/mc-entity.c:550 __media_pipeline_stop+0x24/0xfc [mc]
[   69.879806] Modules linked in: s5p_fimc v4l2_fwnode exynos4_is_common videobuf2_dma_contig videobuf2_memops v4l2_mem2mem brcmfmac videobuf2_v4l2 pvrsrvkm_s5pv210_sgx540_120 videobuf2_common hci_uart sha256_generic btbcm libsha256 bluetooth cfg80211 ce147 brcmutil s5ka3dfx ecdh_generic ecc libaes videodev atmel_mxt_ts mc pwm_vibra rtc_max8998
[   69.880002] CPU: 0 PID: 1566 Comm: v4l2-ctl Tainted: G        W         5.7.0-14540-gb1220848c797-dirty #681
[   69.880016] Hardware name: Samsung S5PC110/S5PV210-based board
[   69.880071] [<c010c7c4>] (unwind_backtrace) from [<c010a120>] (show_stack+0x10/0x14)
[   69.880115] [<c010a120>] (show_stack) from [<c0117038>] (__warn+0xbc/0xd4)
[   69.880161] [<c0117038>] (__warn) from [<c01170b0>] (warn_slowpath_fmt+0x60/0xb8)
[   69.880231] [<c01170b0>] (warn_slowpath_fmt) from [<bf010130>] (__media_pipeline_stop+0x24/0xfc [mc])
[   69.880318] [<bf010130>] (__media_pipeline_stop [mc]) from [<bf010228>] (media_pipeline_stop+0x20/0x2c [mc])
[   69.880419] [<bf010228>] (media_pipeline_stop [mc]) from [<bf08fc48>] (fimc_cap_streamoff+0x38/0x48 [s5p_fimc])
[   69.880582] [<bf08fc48>] (fimc_cap_streamoff [s5p_fimc]) from [<bf040bf4>] (__video_do_ioctl+0x220/0x448 [videodev])
[   69.880776] [<bf040bf4>] (__video_do_ioctl [videodev]) from [<bf041600>] (video_usercopy+0x114/0x498 [videodev])
[   69.880895] [<bf041600>] (video_usercopy [videodev]) from [<c0205024>] (ksys_ioctl+0x20c/0xa10)
[   69.880939] [<c0205024>] (ksys_ioctl) from [<c0100060>] (ret_fast_syscall+0x0/0x54)
[   69.880958] Exception stack(0xe50c1fa8 to 0xe50c1ff0)
[   69.880997] 1fa0:                   004ef08d bec188c0 00000003 40045613 bec165ac 00517d16
[   69.881040] 1fc0: 004ef08d bec188c0 bec188c0 00000036 bec165ac 00000000 b6def320 bec18af8
[   69.881070] 1fe0: 00539ed4 bec15718 004ef0bb b6f84d0a
[   69.881089] ---[ end trace d004ab573a72c32a ]---
[   69.881102] ------------[ cut here ]------------
[   69.881163] WARNING: CPU: 0 PID: 1566 at drivers/media/mc/mc-entity.c:556 media_pipeline_stop+0x20/0x2c [mc]
[   69.881174] Modules linked in: s5p_fimc v4l2_fwnode exynos4_is_common videobuf2_dma_contig videobuf2_memops v4l2_mem2mem brcmfmac videobuf2_v4l2 pvrsrvkm_s5pv210_sgx540_120 videobuf2_common hci_uart sha256_generic btbcm libsha256 bluetooth cfg80211 ce147 brcmutil s5ka3dfx ecdh_generic ecc libaes videodev atmel_mxt_ts mc pwm_vibra rtc_max8998
[   69.881367] CPU: 0 PID: 1566 Comm: v4l2-ctl Tainted: G        W         5.7.0-14540-gb1220848c797-dirty #681
[   69.881381] Hardware name: Samsung S5PC110/S5PV210-based board
[   69.881424] [<c010c7c4>] (unwind_backtrace) from [<c010a120>] (show_stack+0x10/0x14)
[   69.881465] [<c010a120>] (show_stack) from [<c0117038>] (__warn+0xbc/0xd4)
[   69.881511] [<c0117038>] (__warn) from [<c01170b0>] (warn_slowpath_fmt+0x60/0xb8)
[   69.881580] [<c01170b0>] (warn_slowpath_fmt) from [<bf010228>] (media_pipeline_stop+0x20/0x2c [mc])
[   69.881683] [<bf010228>] (media_pipeline_stop [mc]) from [<bf08fc48>] (fimc_cap_streamoff+0x38/0x48 [s5p_fimc])
[   69.881834] [<bf08fc48>] (fimc_cap_streamoff [s5p_fimc]) from [<bf040bf4>] (__video_do_ioctl+0x220/0x448 [videodev])
[   69.882025] [<bf040bf4>] (__video_do_ioctl [videodev]) from [<bf041600>] (video_usercopy+0x114/0x498 [videodev])
[   69.882246] [<bf041600>] (video_usercopy [videodev]) from [<c0205024>] (ksys_ioctl+0x20c/0xa10)
[   69.882291] [<c0205024>] (ksys_ioctl) from [<c0100060>] (ret_fast_syscall+0x0/0x54)
[   69.882309] Exception stack(0xe50c1fa8 to 0xe50c1ff0)
[   69.882348] 1fa0:                   004ef08d bec188c0 00000003 40045613 bec165ac 00517d16
[   69.882391] 1fc0: 004ef08d bec188c0 bec188c0 00000036 bec165ac 00000000 b6def320 bec18af8
[   69.882420] 1fe0: 00539ed4 bec15718 004ef0bb b6f84d0a
[   69.882439] ---[ end trace d004ab573a72c32b ]---

With the final trace being the original one that I was having.

So it looks to me as if streamoff is being called twice.  Is this a possibility for all drivers
or is there a different bug that I should be trying to track down?  In any event, my patch does
prevent the warning (although my reasoning was wrong as I thought it was being stopped on the
call to release).

Thanks,
Jonathan

> 
> Best regards,
> Tomasz
>
Tomasz Figa July 27, 2020, 1:48 p.m. UTC | #5 
On Sat, Jul 25, 2020 at 1:46 AM Jonathan Bakker <xc-racer2@live.ca> wrote:
>
> Hi Tomasz,
>
> On 2020-07-20 6:10 a.m., Tomasz Figa wrote:
> > 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.
>
> Here's the results of that:
>
> [   69.876823] ------------[ cut here ]------------
> [   69.876962] WARNING: CPU: 0 PID: 1566 at drivers/media/mc/mc-entity.c:550 __media_pipeline_stop+0x24/0xfc [mc]
> [   69.876976] Modules linked in: s5p_fimc v4l2_fwnode exynos4_is_common videobuf2_dma_contig videobuf2_memops v4l2_mem2mem brcmfmac videobuf2_v4l2 pvrsrvkm_s5pv210_sgx540_120 videobuf2_common hci_uart sha256_generic btbcm libsha256 bluetooth cfg80211 ce147 brcmutil s5ka3dfx ecdh_generic ecc libaes videodev atmel_mxt_ts mc pwm_vibra rtc_max8998
> [   69.877182] CPU: 0 PID: 1566 Comm: v4l2-ctl Not tainted 5.7.0-14540-gb1220848c797-dirty #681
> [   69.877198] Hardware name: Samsung S5PC110/S5PV210-based board
> [   69.877274] [<c010c7c4>] (unwind_backtrace) from [<c010a120>] (show_stack+0x10/0x14)
> [   69.877326] [<c010a120>] (show_stack) from [<c0117038>] (__warn+0xbc/0xd4)
> [   69.877375] [<c0117038>] (__warn) from [<c01170b0>] (warn_slowpath_fmt+0x60/0xb8)
> [   69.877448] [<c01170b0>] (warn_slowpath_fmt) from [<bf010130>] (__media_pipeline_stop+0x24/0xfc [mc])
> [   69.877540] [<bf010130>] (__media_pipeline_stop [mc]) from [<bf010228>] (media_pipeline_stop+0x20/0x2c [mc])
> [   69.877663] [<bf010228>] (media_pipeline_stop [mc]) from [<bf08fc48>] (fimc_cap_streamoff+0x38/0x48 [s5p_fimc])
> [   69.877904] [<bf08fc48>] (fimc_cap_streamoff [s5p_fimc]) from [<bf040bf4>] (__video_do_ioctl+0x220/0x448 [videodev])
> [   69.878105] [<bf040bf4>] (__video_do_ioctl [videodev]) from [<bf041600>] (video_usercopy+0x114/0x498 [videodev])
> [   69.878234] [<bf041600>] (video_usercopy [videodev]) from [<c0205024>] (ksys_ioctl+0x20c/0xa10)
> [   69.878281] [<c0205024>] (ksys_ioctl) from [<c0100060>] (ret_fast_syscall+0x0/0x54)
> [   69.878301] Exception stack(0xe50c1fa8 to 0xe50c1ff0)
> [   69.878342] 1fa0:                   004ef08d 00539d0c 00000003 40045613 bec1578c 00517d16
> [   69.878386] 1fc0: 004ef08d 00539d0c bec188c0 00000036 bec165ac 00000000 b6def320 bec18af8
> [   69.878415] 1fe0: 00539ed4 bec15730 004ef0bb b6f84d0a
> [   69.878436] ---[ end trace d004ab573a72c329 ]---
> [   69.879704] ------------[ cut here ]------------
> [   69.879794] WARNING: CPU: 0 PID: 1566 at drivers/media/mc/mc-entity.c:550 __media_pipeline_stop+0x24/0xfc [mc]
> [   69.879806] Modules linked in: s5p_fimc v4l2_fwnode exynos4_is_common videobuf2_dma_contig videobuf2_memops v4l2_mem2mem brcmfmac videobuf2_v4l2 pvrsrvkm_s5pv210_sgx540_120 videobuf2_common hci_uart sha256_generic btbcm libsha256 bluetooth cfg80211 ce147 brcmutil s5ka3dfx ecdh_generic ecc libaes videodev atmel_mxt_ts mc pwm_vibra rtc_max8998
> [   69.880002] CPU: 0 PID: 1566 Comm: v4l2-ctl Tainted: G        W         5.7.0-14540-gb1220848c797-dirty #681
> [   69.880016] Hardware name: Samsung S5PC110/S5PV210-based board
> [   69.880071] [<c010c7c4>] (unwind_backtrace) from [<c010a120>] (show_stack+0x10/0x14)
> [   69.880115] [<c010a120>] (show_stack) from [<c0117038>] (__warn+0xbc/0xd4)
> [   69.880161] [<c0117038>] (__warn) from [<c01170b0>] (warn_slowpath_fmt+0x60/0xb8)
> [   69.880231] [<c01170b0>] (warn_slowpath_fmt) from [<bf010130>] (__media_pipeline_stop+0x24/0xfc [mc])
> [   69.880318] [<bf010130>] (__media_pipeline_stop [mc]) from [<bf010228>] (media_pipeline_stop+0x20/0x2c [mc])
> [   69.880419] [<bf010228>] (media_pipeline_stop [mc]) from [<bf08fc48>] (fimc_cap_streamoff+0x38/0x48 [s5p_fimc])
> [   69.880582] [<bf08fc48>] (fimc_cap_streamoff [s5p_fimc]) from [<bf040bf4>] (__video_do_ioctl+0x220/0x448 [videodev])
> [   69.880776] [<bf040bf4>] (__video_do_ioctl [videodev]) from [<bf041600>] (video_usercopy+0x114/0x498 [videodev])
> [   69.880895] [<bf041600>] (video_usercopy [videodev]) from [<c0205024>] (ksys_ioctl+0x20c/0xa10)
> [   69.880939] [<c0205024>] (ksys_ioctl) from [<c0100060>] (ret_fast_syscall+0x0/0x54)
> [   69.880958] Exception stack(0xe50c1fa8 to 0xe50c1ff0)
> [   69.880997] 1fa0:                   004ef08d bec188c0 00000003 40045613 bec165ac 00517d16
> [   69.881040] 1fc0: 004ef08d bec188c0 bec188c0 00000036 bec165ac 00000000 b6def320 bec18af8
> [   69.881070] 1fe0: 00539ed4 bec15718 004ef0bb b6f84d0a
> [   69.881089] ---[ end trace d004ab573a72c32a ]---
> [   69.881102] ------------[ cut here ]------------
> [   69.881163] WARNING: CPU: 0 PID: 1566 at drivers/media/mc/mc-entity.c:556 media_pipeline_stop+0x20/0x2c [mc]
> [   69.881174] Modules linked in: s5p_fimc v4l2_fwnode exynos4_is_common videobuf2_dma_contig videobuf2_memops v4l2_mem2mem brcmfmac videobuf2_v4l2 pvrsrvkm_s5pv210_sgx540_120 videobuf2_common hci_uart sha256_generic btbcm libsha256 bluetooth cfg80211 ce147 brcmutil s5ka3dfx ecdh_generic ecc libaes videodev atmel_mxt_ts mc pwm_vibra rtc_max8998
> [   69.881367] CPU: 0 PID: 1566 Comm: v4l2-ctl Tainted: G        W         5.7.0-14540-gb1220848c797-dirty #681
> [   69.881381] Hardware name: Samsung S5PC110/S5PV210-based board
> [   69.881424] [<c010c7c4>] (unwind_backtrace) from [<c010a120>] (show_stack+0x10/0x14)
> [   69.881465] [<c010a120>] (show_stack) from [<c0117038>] (__warn+0xbc/0xd4)
> [   69.881511] [<c0117038>] (__warn) from [<c01170b0>] (warn_slowpath_fmt+0x60/0xb8)
> [   69.881580] [<c01170b0>] (warn_slowpath_fmt) from [<bf010228>] (media_pipeline_stop+0x20/0x2c [mc])
> [   69.881683] [<bf010228>] (media_pipeline_stop [mc]) from [<bf08fc48>] (fimc_cap_streamoff+0x38/0x48 [s5p_fimc])
> [   69.881834] [<bf08fc48>] (fimc_cap_streamoff [s5p_fimc]) from [<bf040bf4>] (__video_do_ioctl+0x220/0x448 [videodev])
> [   69.882025] [<bf040bf4>] (__video_do_ioctl [videodev]) from [<bf041600>] (video_usercopy+0x114/0x498 [videodev])
> [   69.882246] [<bf041600>] (video_usercopy [videodev]) from [<c0205024>] (ksys_ioctl+0x20c/0xa10)
> [   69.882291] [<c0205024>] (ksys_ioctl) from [<c0100060>] (ret_fast_syscall+0x0/0x54)
> [   69.882309] Exception stack(0xe50c1fa8 to 0xe50c1ff0)
> [   69.882348] 1fa0:                   004ef08d bec188c0 00000003 40045613 bec165ac 00517d16
> [   69.882391] 1fc0: 004ef08d bec188c0 bec188c0 00000036 bec165ac 00000000 b6def320 bec18af8
> [   69.882420] 1fe0: 00539ed4 bec15718 004ef0bb b6f84d0a
> [   69.882439] ---[ end trace d004ab573a72c32b ]---
>
> With the final trace being the original one that I was having.
>
> So it looks to me as if streamoff is being called twice.  Is this a possibility for all drivers
> or is there a different bug that I should be trying to track down?  In any event, my patch does
> prevent the warning (although my reasoning was wrong as I thought it was being stopped on the
> call to release).

It certainly is possible for the userspace call STREAMOFF twice and
the driver needs to ensure that the second call is essentially a
no-op, as per [1]. FWIW, if the driver defers the stream start/stop
operation entirely to the vb2 .start/stop_streaming callback, then vb2
would handle this automatically.

[1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-streamon.html#description

Best regards,
Tomasz
diff mbox series

Patch

diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c b/drivers/media/platform/exynos4-is/fimc-capture.c
index 95d4a667bffb..d3ef1268da07 100644
--- a/drivers/media/platform/exynos4-is/fimc-capture.c
+++ b/drivers/media/platform/exynos4-is/fimc-capture.c
@@ -1232,8 +1232,11 @@  static int fimc_cap_streamoff(struct file *file, void *priv,
 	if (ret < 0)
 		return ret;
 
-	media_pipeline_stop(&vc->ve.vdev.entity);
-	vc->streaming = false;
+	if (vc->streaming) {
+		media_pipeline_stop(&vc->ve.vdev.entity);
+		vc->streaming = false;
+	}
+
 	return 0;
 }