* [PATCH] drm/msm: fix splat when userspace is killed with pending atomic update
@ 2017-04-28 18:05 Rob Clark
2017-05-02 9:01 ` Daniel Vetter
0 siblings, 1 reply; 3+ messages in thread
From: Rob Clark @ 2017-04-28 18:05 UTC (permalink / raw)
To: dri-devel; +Cc: Stanimir Varbanov
The ->preclose() hook is a good place to block for pending atomic
updates. We can't do this in ->postclose(), as it needs to happen
before drm_fb_release(). Otherwise, since we have already swapped
state (in the case of a non-blocking atomic update), this means that
the plane_state->fb will be released and cleared before we wait for
fences from the atomic-commit wq.
There are probably more complex solutions possible. But since already
scheduled atomic update, possibly blocking on already scheduled gpu/etc
fences, will complete eventually (assuming nothing catches fire), so
the sanest thing seems to be just block until already scheduled atomic
updates complete before tearing things down.
Fixes:
WARNING: CPU: 1 PID: 69 at ../drivers/gpu/drm/drm_atomic_helper.c:1061 drm_atomic_helper_wait_for_fences+0xe0/0xf8
Modules linked in:
CPU: 1 PID: 69 Comm: kworker/1:1 Tainted: G W 4.11.0-rc8+ #1187
Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT)
Workqueue: events drm_mode_rmfb_work_fn
task: ffffffc036560d00 task.stack: ffffffc036550000
PC is at drm_atomic_helper_wait_for_fences+0xe0/0xf8
LR is at complete_commit.isra.1+0x44/0x1c0
pc : [<ffffff80084f6040>] lr : [<ffffff800854176c>] pstate: 20000145
sp : ffffffc036553b60
x29: ffffffc036553b60 x28: ffffffc0264e6a00
x27: ffffffc035659000 x26: 0000000000000000
x25: ffffffc0240e8000 x24: 0000000000000038
x23: 0000000000000000 x22: ffffff800858f200
x21: ffffffc0240e8000 x20: ffffffc02f56a800
x19: 0000000000000000 x18: 0000000000000000
x17: 0000000000000000 x16: 0000000000000000
x15: 0000000000000000 x14: ffffffc00a192700
x13: 0000000000000004 x12: 0000000000000000
x11: ffffff80089a1690 x10: 00000000000008f0
x9 : ffffffc036553b20 x8 : ffffffc036561650
x7 : ffffffc03fe6cb40 x6 : 0000000000000000
x5 : 0000000000000001 x4 : 0000000000000002
x3 : ffffffc035659000 x2 : ffffffc0240e8c80
x1 : 0000000000000000 x0 : ffffffc02adbe588
---[ end trace 13aeec77c3fb55e2 ]---
Call trace:
Exception stack(0xffffffc036553990 to 0xffffffc036553ac0)
3980: 0000000000000000 0000008000000000
39a0: ffffffc036553b60 ffffff80084f6040 0000000000004ff0 0000000000000038
39c0: ffffffc0365539d0 ffffff800857e098 ffffffc036553a00 ffffff800857e1b0
39e0: ffffffc036553a10 ffffff800857c554 ffffffc0365e8400 ffffffc0365e8400
3a00: ffffffc036553a20 ffffff8008103358 000000000001aad7 ffffff800851b72c
3a20: ffffffc036553a50 ffffff80080e9228 ffffffc02adbe588 0000000000000000
3a40: ffffffc0240e8c80 ffffffc035659000 0000000000000002 0000000000000001
3a60: 0000000000000000 ffffffc03fe6cb40 ffffffc036561650 ffffffc036553b20
3a80: 00000000000008f0 ffffff80089a1690 0000000000000000 0000000000000004
3aa0: ffffffc00a192700 0000000000000000 0000000000000000 0000000000000000
[<ffffff80084f6040>] drm_atomic_helper_wait_for_fences+0xe0/0xf8
[<ffffff800854176c>] complete_commit.isra.1+0x44/0x1c0
[<ffffff8008541c64>] msm_atomic_commit+0x32c/0x350
[<ffffff8008516230>] drm_atomic_commit+0x50/0x60
[<ffffff8008517548>] drm_atomic_remove_fb+0x158/0x250
[<ffffff80085186d0>] drm_framebuffer_remove+0x50/0x158
[<ffffff8008518818>] drm_mode_rmfb_work_fn+0x40/0x58
[<ffffff80080d5668>] process_one_work+0x1d0/0x378
[<ffffff80080d5a54>] worker_thread+0x244/0x488
[<ffffff80080db7fc>] kthread+0xfc/0x128
[<ffffff8008082ec0>] ret_from_fork+0x10/0x50
Reported-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
Signed-off-by: Rob Clark <robdclark@gmail.com>
---
The hunk that removes the comment about ->preclose() included in this
patch to challenge the assumption that ->preclose() shouldn't exist ;-)
drivers/gpu/drm/msm/msm_drv.c | 10 +++++++++-
include/drm/drm_drv.h | 3 ---
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 87b5695..c8763b9 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -540,6 +540,13 @@ static int msm_open(struct drm_device *dev, struct drm_file *file)
return 0;
}
+static void msm_preclose(struct drm_device *dev, struct drm_file *file)
+{
+ struct msm_drm_private *priv = dev->dev_private;
+
+ flush_workqueue(priv->atomic_wq);
+}
+
static void msm_postclose(struct drm_device *dev, struct drm_file *file)
{
struct msm_drm_private *priv = dev->dev_private;
@@ -813,7 +820,8 @@ static struct drm_driver msm_driver = {
DRIVER_ATOMIC |
DRIVER_MODESET,
.open = msm_open,
- .postclose = msm_postclose,
+ .preclose = msm_preclose,
+ .postclose = msm_postclose,
.lastclose = msm_lastclose,
.irq_handler = msm_irq,
.irq_preinstall = msm_irq_preinstall,
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index 53b9832..a045ada 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -114,9 +114,6 @@ struct drm_driver {
* one &struct drm_file (see &drm_file.is_master and &drm_device.master)
* there should never be a need to tear down any modeset related
* resources in this callback. Doing so would be a driver design bug.
- *
- * FIXME: It is not really clear why there's both @preclose and
- * @postclose. Without a really good reason, use @postclose only.
*/
void (*preclose) (struct drm_device *, struct drm_file *file_priv);
--
2.9.3
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] drm/msm: fix splat when userspace is killed with pending atomic update
2017-04-28 18:05 [PATCH] drm/msm: fix splat when userspace is killed with pending atomic update Rob Clark
@ 2017-05-02 9:01 ` Daniel Vetter
2017-05-03 14:01 ` Rob Clark
0 siblings, 1 reply; 3+ messages in thread
From: Daniel Vetter @ 2017-05-02 9:01 UTC (permalink / raw)
To: Rob Clark; +Cc: Stanimir Varbanov, dri-devel
On Fri, Apr 28, 2017 at 8:05 PM, Rob Clark <robdclark@gmail.com> wrote:
> The ->preclose() hook is a good place to block for pending atomic
> updates. We can't do this in ->postclose(), as it needs to happen
> before drm_fb_release(). Otherwise, since we have already swapped
> state (in the case of a non-blocking atomic update), this means that
> the plane_state->fb will be released and cleared before we wait for
> fences from the atomic-commit wq.
>
> There are probably more complex solutions possible. But since already
> scheduled atomic update, possibly blocking on already scheduled gpu/etc
> fences, will complete eventually (assuming nothing catches fire), so
> the sanest thing seems to be just block until already scheduled atomic
> updates complete before tearing things down.
>
> Fixes:
>
> WARNING: CPU: 1 PID: 69 at ../drivers/gpu/drm/drm_atomic_helper.c:1061 drm_atomic_helper_wait_for_fences+0xe0/0xf8
> Modules linked in:
>
> CPU: 1 PID: 69 Comm: kworker/1:1 Tainted: G W 4.11.0-rc8+ #1187
> Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT)
> Workqueue: events drm_mode_rmfb_work_fn
> task: ffffffc036560d00 task.stack: ffffffc036550000
> PC is at drm_atomic_helper_wait_for_fences+0xe0/0xf8
> LR is at complete_commit.isra.1+0x44/0x1c0
> pc : [<ffffff80084f6040>] lr : [<ffffff800854176c>] pstate: 20000145
> sp : ffffffc036553b60
> x29: ffffffc036553b60 x28: ffffffc0264e6a00
> x27: ffffffc035659000 x26: 0000000000000000
> x25: ffffffc0240e8000 x24: 0000000000000038
> x23: 0000000000000000 x22: ffffff800858f200
> x21: ffffffc0240e8000 x20: ffffffc02f56a800
> x19: 0000000000000000 x18: 0000000000000000
> x17: 0000000000000000 x16: 0000000000000000
> x15: 0000000000000000 x14: ffffffc00a192700
> x13: 0000000000000004 x12: 0000000000000000
> x11: ffffff80089a1690 x10: 00000000000008f0
> x9 : ffffffc036553b20 x8 : ffffffc036561650
> x7 : ffffffc03fe6cb40 x6 : 0000000000000000
> x5 : 0000000000000001 x4 : 0000000000000002
> x3 : ffffffc035659000 x2 : ffffffc0240e8c80
> x1 : 0000000000000000 x0 : ffffffc02adbe588
>
> ---[ end trace 13aeec77c3fb55e2 ]---
> Call trace:
> Exception stack(0xffffffc036553990 to 0xffffffc036553ac0)
> 3980: 0000000000000000 0000008000000000
> 39a0: ffffffc036553b60 ffffff80084f6040 0000000000004ff0 0000000000000038
> 39c0: ffffffc0365539d0 ffffff800857e098 ffffffc036553a00 ffffff800857e1b0
> 39e0: ffffffc036553a10 ffffff800857c554 ffffffc0365e8400 ffffffc0365e8400
> 3a00: ffffffc036553a20 ffffff8008103358 000000000001aad7 ffffff800851b72c
> 3a20: ffffffc036553a50 ffffff80080e9228 ffffffc02adbe588 0000000000000000
> 3a40: ffffffc0240e8c80 ffffffc035659000 0000000000000002 0000000000000001
> 3a60: 0000000000000000 ffffffc03fe6cb40 ffffffc036561650 ffffffc036553b20
> 3a80: 00000000000008f0 ffffff80089a1690 0000000000000000 0000000000000004
> 3aa0: ffffffc00a192700 0000000000000000 0000000000000000 0000000000000000
> [<ffffff80084f6040>] drm_atomic_helper_wait_for_fences+0xe0/0xf8
> [<ffffff800854176c>] complete_commit.isra.1+0x44/0x1c0
> [<ffffff8008541c64>] msm_atomic_commit+0x32c/0x350
> [<ffffff8008516230>] drm_atomic_commit+0x50/0x60
> [<ffffff8008517548>] drm_atomic_remove_fb+0x158/0x250
> [<ffffff80085186d0>] drm_framebuffer_remove+0x50/0x158
> [<ffffff8008518818>] drm_mode_rmfb_work_fn+0x40/0x58
> [<ffffff80080d5668>] process_one_work+0x1d0/0x378
> [<ffffff80080d5a54>] worker_thread+0x244/0x488
> [<ffffff80080db7fc>] kthread+0xfc/0x128
> [<ffffff8008082ec0>] ret_from_fork+0x10/0x50
>
> Reported-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
> The hunk that removes the comment about ->preclose() included in this
> patch to challenge the assumption that ->preclose() shouldn't exist ;-)
And I'm going to challenge your patch here. Both fences and
framebuffers and atomic commits are refcounted. If you go boom on them
when userspace closes the fd, you have a refcount bug. We don't fix
those by flusing stuff :-)
Please add a pair of get/put() calls at the right place instead.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] drm/msm: fix splat when userspace is killed with pending atomic update
2017-05-02 9:01 ` Daniel Vetter
@ 2017-05-03 14:01 ` Rob Clark
0 siblings, 0 replies; 3+ messages in thread
From: Rob Clark @ 2017-05-03 14:01 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Stanimir Varbanov, dri-devel
On Tue, May 2, 2017 at 5:01 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Apr 28, 2017 at 8:05 PM, Rob Clark <robdclark@gmail.com> wrote:
>> The ->preclose() hook is a good place to block for pending atomic
>> updates. We can't do this in ->postclose(), as it needs to happen
>> before drm_fb_release(). Otherwise, since we have already swapped
>> state (in the case of a non-blocking atomic update), this means that
>> the plane_state->fb will be released and cleared before we wait for
>> fences from the atomic-commit wq.
>>
>> There are probably more complex solutions possible. But since already
>> scheduled atomic update, possibly blocking on already scheduled gpu/etc
>> fences, will complete eventually (assuming nothing catches fire), so
>> the sanest thing seems to be just block until already scheduled atomic
>> updates complete before tearing things down.
>>
>> Fixes:
>>
>> WARNING: CPU: 1 PID: 69 at ../drivers/gpu/drm/drm_atomic_helper.c:1061 drm_atomic_helper_wait_for_fences+0xe0/0xf8
>> Modules linked in:
>>
>> CPU: 1 PID: 69 Comm: kworker/1:1 Tainted: G W 4.11.0-rc8+ #1187
>> Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT)
>> Workqueue: events drm_mode_rmfb_work_fn
>> task: ffffffc036560d00 task.stack: ffffffc036550000
>> PC is at drm_atomic_helper_wait_for_fences+0xe0/0xf8
>> LR is at complete_commit.isra.1+0x44/0x1c0
>> pc : [<ffffff80084f6040>] lr : [<ffffff800854176c>] pstate: 20000145
>> sp : ffffffc036553b60
>> x29: ffffffc036553b60 x28: ffffffc0264e6a00
>> x27: ffffffc035659000 x26: 0000000000000000
>> x25: ffffffc0240e8000 x24: 0000000000000038
>> x23: 0000000000000000 x22: ffffff800858f200
>> x21: ffffffc0240e8000 x20: ffffffc02f56a800
>> x19: 0000000000000000 x18: 0000000000000000
>> x17: 0000000000000000 x16: 0000000000000000
>> x15: 0000000000000000 x14: ffffffc00a192700
>> x13: 0000000000000004 x12: 0000000000000000
>> x11: ffffff80089a1690 x10: 00000000000008f0
>> x9 : ffffffc036553b20 x8 : ffffffc036561650
>> x7 : ffffffc03fe6cb40 x6 : 0000000000000000
>> x5 : 0000000000000001 x4 : 0000000000000002
>> x3 : ffffffc035659000 x2 : ffffffc0240e8c80
>> x1 : 0000000000000000 x0 : ffffffc02adbe588
>>
>> ---[ end trace 13aeec77c3fb55e2 ]---
>> Call trace:
>> Exception stack(0xffffffc036553990 to 0xffffffc036553ac0)
>> 3980: 0000000000000000 0000008000000000
>> 39a0: ffffffc036553b60 ffffff80084f6040 0000000000004ff0 0000000000000038
>> 39c0: ffffffc0365539d0 ffffff800857e098 ffffffc036553a00 ffffff800857e1b0
>> 39e0: ffffffc036553a10 ffffff800857c554 ffffffc0365e8400 ffffffc0365e8400
>> 3a00: ffffffc036553a20 ffffff8008103358 000000000001aad7 ffffff800851b72c
>> 3a20: ffffffc036553a50 ffffff80080e9228 ffffffc02adbe588 0000000000000000
>> 3a40: ffffffc0240e8c80 ffffffc035659000 0000000000000002 0000000000000001
>> 3a60: 0000000000000000 ffffffc03fe6cb40 ffffffc036561650 ffffffc036553b20
>> 3a80: 00000000000008f0 ffffff80089a1690 0000000000000000 0000000000000004
>> 3aa0: ffffffc00a192700 0000000000000000 0000000000000000 0000000000000000
>> [<ffffff80084f6040>] drm_atomic_helper_wait_for_fences+0xe0/0xf8
>> [<ffffff800854176c>] complete_commit.isra.1+0x44/0x1c0
>> [<ffffff8008541c64>] msm_atomic_commit+0x32c/0x350
>> [<ffffff8008516230>] drm_atomic_commit+0x50/0x60
>> [<ffffff8008517548>] drm_atomic_remove_fb+0x158/0x250
>> [<ffffff80085186d0>] drm_framebuffer_remove+0x50/0x158
>> [<ffffff8008518818>] drm_mode_rmfb_work_fn+0x40/0x58
>> [<ffffff80080d5668>] process_one_work+0x1d0/0x378
>> [<ffffff80080d5a54>] worker_thread+0x244/0x488
>> [<ffffff80080db7fc>] kthread+0xfc/0x128
>> [<ffffff8008082ec0>] ret_from_fork+0x10/0x50
>>
>> Reported-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>> ---
>> The hunk that removes the comment about ->preclose() included in this
>> patch to challenge the assumption that ->preclose() shouldn't exist ;-)
>
> And I'm going to challenge your patch here. Both fences and
> framebuffers and atomic commits are refcounted. If you go boom on them
> when userspace closes the fd, you have a refcount bug. We don't fix
> those by flusing stuff :-)
So, it isn't a refcount'ing but, but something much funnier..
It seems that mdp5 had custom plane state with it's own dup_state fxn,
pre-dating the addition of
__drm_atomic_helper_plane_duplicate_state(), and when the helper was
introduced it wasn't retrofitted. Which was all good until the fence
ptr is added to base plane_state struct. So this means that
plane_state->fence was getting copied over into the duplicated
plane_state.
So the atomic rmfb code would sometimes manage to copy the fence ptr
if there is another pending update which had already swapped state but
not yet committed.
BR,
-R
> Please add a pair of get/put() calls at the right place instead.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-05-03 14:01 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-04-28 18:05 [PATCH] drm/msm: fix splat when userspace is killed with pending atomic update Rob Clark
2017-05-02 9:01 ` Daniel Vetter
2017-05-03 14:01 ` Rob Clark
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox