drm/nouveau: handle same-fb page flips
Subsystems (3)
PATCH 1/1 drm/nouveau: handle same-fb page flips
It's questionable use case, but weston/wayland already relies on this behaviour, and other drivers don't care about it, so it's a matter of compatibility. Without it, process invoking such page flip hangs in unkillable state, trying to reserve the same buffer twice.
--- drivers/gpu/drm/nouveau/nouveau_display.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)
drivers/gpu/drm/nouveau/nouveau_display.c +15 -5
@@ -530,9 +530,11 @@ nouveau_page_flip_reserve(struct nouveau_bo *old_bo,if (ret) goto fail; - ret =ttm_bo_reserve (&old_bo->bo, false, false, false,0 ); -if (ret) - goto fail_unreserve; +if (likely (old_bo != new_bo)) { + ret =ttm_bo_reserve (&old_bo->bo, false, false, false,0 ); +if (ret) + goto fail_unreserve; + }return 0 ;
@@ -551,8 +553,10 @@ nouveau_page_flip_unreserve(struct nouveau_bo *old_bo,nouveau_bo_fence (new_bo, fence);ttm_bo_unreserve (&new_bo->bo); -nouveau_bo_fence (old_bo, fence); -ttm_bo_unreserve (&old_bo->bo); +if (likely (old_bo != new_bo)) { +nouveau_bo_fence (old_bo, fence); +ttm_bo_unreserve (&old_bo->bo); + }nouveau_bo_unpin (old_bo); }
@@ -624,6 +628,12 @@ nouveau_crtc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb,if (!drm->channel)return -ENODEV ; +if (unlikely (old_bo == new_bo)) { +char name[sizeof (current->comm)]; +pr_debug_ratelimited ("'%s': useless page flip invoked\n" , +get_task_comm (name, current)); + } + s =kzalloc (sizeof (*s),GFP_KERNEL );if (!s)return -ENOMEM ;
Signature · 1 line
1.7.12
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 1 line · Marcin Slusarz
It's questionable use case, but weston/wayland already relies on this behaviour,
Important detail: weston does it only once per session.
Quoted 1 line
and other drivers don't care about it, so it's a matter of compatibility.
I looked into weston sources and produced a patch, but it's quite ugly.
Quoted 56 lines
Without it, process invoking such page flip hangs in unkillable state, trying to reserve the same buffer twice.
Signed-off-by Marcin Slusarz <[email protected]>--- drivers/gpu/drm/nouveau/nouveau_display.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)drivers/gpu/drm/nouveau/nouveau_display.c +15
-5@@ -530,9 +530,11 @@ nouveau_page_flip_reserve(struct nouveau_bo *old_bo,if (ret) goto fail; - ret =ttm_bo_reserve (&old_bo->bo, false, false, false,0 ); -if (ret) - goto fail_unreserve; +if (likely (old_bo != new_bo)) { + ret =ttm_bo_reserve (&old_bo->bo, false, false, false,0 ); +if (ret) + goto fail_unreserve; + }return 0 ;@@ -551,8 +553,10 @@ nouveau_page_flip_unreserve(struct nouveau_bo *old_bo,nouveau_bo_fence (new_bo, fence);ttm_bo_unreserve (&new_bo->bo); -nouveau_bo_fence (old_bo, fence); -ttm_bo_unreserve (&old_bo->bo); +if (likely (old_bo != new_bo)) { +nouveau_bo_fence (old_bo, fence); +ttm_bo_unreserve (&old_bo->bo); + }nouveau_bo_unpin (old_bo); }@@ -624,6 +628,12 @@ nouveau_crtc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb,if (!drm->channel)return -ENODEV ; +if (unlikely (old_bo == new_bo)) { +char name[sizeof (current->comm)]; +pr_debug_ratelimited ("'%s': useless page flip invoked\n" , +get_task_comm (name, current)); + } + s =kzalloc (sizeof (*s),GFP_KERNEL );if (!s)return -ENOMEM ;Signature · 1 line
1.7.12
Quoted 6 lines · On Fri, 5 Oct 2012 21:37:59 +0200 Marcin Slusarz…
It's questionable use case, but weston/wayland already relies on this behaviour, and other drivers don't care about it, so it's a matter of compatibility. Without it, process invoking such page flip hangs in unkillable state, trying to reserve the same buffer twice.
Signed-off-by Marcin Slusarz <[email protected]>
I'd say a process going into unkillable state for something like this is already a bug in itself.
I'm very happy if this makes Weston run on Nouveau, we've had many unhappy users so far.
Signature · 2 lines
Pekka Paalanen http://www.iki.fi/pq/
Quoted 46 lines · Marcin Slusarz
It's questionable use case, but weston/wayland already relies on this behaviour, and other drivers don't care about it, so it's a matter of compatibility. Without it, process invoking such page flip hangs in unkillable state, trying to reserve the same buffer twice.
Signed-off-by Marcin Slusarz <[email protected]>--- drivers/gpu/drm/nouveau/nouveau_display.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)drivers/gpu/drm/nouveau/nouveau_display.c +9
-5@@ -530,9 +530,11 @@ nouveau_page_flip_reserve(struct nouveau_bo *old_bo,if (ret) goto fail; - ret =ttm_bo_reserve (&old_bo->bo, false, false, false,0 ); -if (ret) - goto fail_unreserve; +if (likely (old_bo != new_bo)) { + ret =ttm_bo_reserve (&old_bo->bo, false, false, false,0 ); +if (ret) + goto fail_unreserve; + }return 0 ;@@ -551,8 +553,10 @@ nouveau_page_flip_unreserve(struct nouveau_bo *old_bo,nouveau_bo_fence (new_bo, fence);ttm_bo_unreserve (&new_bo->bo); -nouveau_bo_fence (old_bo, fence); -ttm_bo_unreserve (&old_bo->bo); +if (likely (old_bo != new_bo)) { +nouveau_bo_fence (old_bo, fence); +ttm_bo_unreserve (&old_bo->bo); + }nouveau_bo_unpin (old_bo); }@@ -624,6 +628,12 @@ nouveau_crtc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb,if (!drm->channel)return -ENODEV ;
drivers/gpu/drm/nouveau/nouveau_display.c nouveau_crtc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb, · line 639
Quoted 10 lines
drivers/gpu/drm/nouveau/nouveau_display.c +6
-0+if (unlikely (old_bo == new_bo)) { +char name[sizeof (current->comm)]; +pr_debug_ratelimited ("'%s': useless page flip invoked\n" , +get_task_comm (name, current)); + } + s =kzalloc (sizeof (*s),GFP_KERNEL );if (!s)return -ENOMEM ;
ping
Quoted 43 lines · Marcin Slusarz
It's questionable use case, but weston/wayland already relies on this behaviour, and other drivers don't care about it, so it's a matter of compatibility. Without it, process invoking such page flip hangs in unkillable state, trying to reserve the same buffer twice.
Signed-off-by Marcin Slusarz <[email protected]>--- drivers/gpu/drm/nouveau/nouveau_display.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)drivers/gpu/drm/nouveau/nouveau_display.c +9
-5@@ -530,9 +530,11 @@ nouveau_page_flip_reserve(struct nouveau_bo *old_bo,if (ret) goto fail; - ret =ttm_bo_reserve (&old_bo->bo, false, false, false,0 ); -if (ret) - goto fail_unreserve; +if (likely (old_bo != new_bo)) { + ret =ttm_bo_reserve (&old_bo->bo, false, false, false,0 ); +if (ret) + goto fail_unreserve; + }return 0 ;@@ -551,8 +553,10 @@ nouveau_page_flip_unreserve(struct nouveau_bo *old_bo,nouveau_bo_fence (new_bo, fence);ttm_bo_unreserve (&new_bo->bo); -nouveau_bo_fence (old_bo, fence); -ttm_bo_unreserve (&old_bo->bo); +if (likely (old_bo != new_bo)) { +nouveau_bo_fence (old_bo, fence); +ttm_bo_unreserve (&old_bo->bo); + }nouveau_bo_unpin (old_bo); }
drivers/gpu/drm/nouveau/nouveau_display.c nouveau_crtc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb, · line 636
Quoted 9 lines
drivers/gpu/drm/nouveau/nouveau_display.c +6
-0@@ -624,6 +628,12 @@ nouveau_crtc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb,if (!drm->channel)return -ENODEV ; +if (unlikely (old_bo == new_bo)) { +char name[sizeof (current->comm)]; +pr_debug_ratelimited ("'%s': useless page flip invoked\n" , +get_task_comm (name, current)); + } +
The patch looks alright, except I think we should just drop this hunk, no other driver reports it either.
Thoughts?
Ben.
Quoted 6 lines
drivers/gpu/drm/nouveau/nouveau_display.c +0
-0s =kzalloc (sizeof (*s),GFP_KERNEL );if (!s)return -ENOMEM ;Signature · 1 line
1.7.12
A message that is not in the archive (sent off-list or not yet received): 20121017043056.GA4049-yqdYmcOkqV0XGNroddHbYwC/[email protected]
17 paź 2012 06:31, "Ben Skeggs" <[email protected]> napisał(a):
Quoted 3 lines · Marcin Slusarz
Quoted 1 line
It's questionable use case, but weston/wayland already relies on this
behaviour,
Quoted 1 line
Quoted 1 line
and other drivers don't care about it, so it's a matter of
compatibility.
Quoted 1 line
Quoted 1 line
Without it, process invoking such page flip hangs in unkillable state,
trying
Quoted 8 lines
Review
Quoted 8 lines
to reserve the same buffer twice.
Signed-off-by Marcin Slusarz <[email protected]>--- drivers/gpu/drm/nouveau/nouveau_display.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)(quoted diff) +0
-0
b/drivers/gpu/drm/nouveau/nouveau_display.c
Quoted 4 lines
drivers/gpu/drm/nouveau/nouveau_display.c nouveau_page_flip_reserve(struct nouveau_bo
Quoted 4 lines
index 61f370d..a52cfd3 100644
drivers/gpu/drm/nouveau/nouveau_display.c +0
-0@@ -530,9 +530,11 @@ nouveau_page_flip_reserve(struct nouveau_bo
*old_bo,
Quoted 12 lines
Quoted 12 lines
drivers/gpu/drm/nouveau/nouveau_display.c +5
-3if (ret) goto fail; - ret =ttm_bo_reserve (&old_bo->bo, false, false, false,0 ); -if (ret) - goto fail_unreserve; +if (likely (old_bo != new_bo)) { + ret =ttm_bo_reserve (&old_bo->bo, false, false, false,0 ); +if (ret) + goto fail_unreserve; + }
Quoted 3 lines
drivers/gpu/drm/nouveau/nouveau_display.c nouveau_page_flip_unreserve(struct nouveau_bo
Quoted 3 lines
drivers/gpu/drm/nouveau/nouveau_display.c +0
-0return 0 ;@@ -551,8 +553,10 @@ nouveau_page_flip_unreserve(struct nouveau_bo
*old_bo,
Quoted 10 lines
Quoted 10 lines
drivers/gpu/drm/nouveau/nouveau_display.c +4
-2nouveau_bo_fence (new_bo, fence);ttm_bo_unreserve (&new_bo->bo); -nouveau_bo_fence (old_bo, fence); -ttm_bo_unreserve (&old_bo->bo); +if (likely (old_bo != new_bo)) { +nouveau_bo_fence (old_bo, fence); +ttm_bo_unreserve (&old_bo->bo); + }
Quoted 3 lines
drivers/gpu/drm/nouveau/nouveau_display.c nouveau_crtc_page_flip(struct drm_crtc *crtc,
Quoted 3 lines
drivers/gpu/drm/nouveau/nouveau_display.c +0
-0nouveau_bo_unpin (old_bo); }@@ -624,6 +628,12 @@ nouveau_crtc_page_flip(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
Quoted 9 lines
drivers/gpu/drm/nouveau/nouveau_display.c nouveau_crtc_page_flip(struct drm_crtc *crtc, struct drm_framebuffer *fb, · line 636
Quoted 9 lines
drivers/gpu/drm/nouveau/nouveau_display.c +6
-0if (!drm->channel)return -ENODEV ; +if (unlikely (old_bo == new_bo)) { +char name[sizeof (current->comm)]; +pr_debug_ratelimited ("'%s': useless page flip invoked\n" , +get_task_comm (name, current)); + } +
Quoted 4 lines
The patch looks alright, except I think we should just drop this hunk, no other driver reports it either.
Thoughts?
It will be easier to debug buggy clients with this code in. With multiple wayland compositors it's possible some of them might be buggy... But I don't feel strongly about it. You can delete this hunk when applying.
Marcin
_______________________________________________
Nouveau mailing list [email protected]
http://lists.freedesktop.org/mailman/listinfo/nouveau