* Re: [PATCH] drm/nouveau: handle same-fb page flips
[not found] ` <20121005193759.GA8531-OI9uyE9O0yo@public.gmane.org>
@ 2012-10-05 19:53 ` Marcin Slusarz
2012-10-06 7:49 ` Pekka Paalanen
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Marcin Slusarz @ 2012-10-05 19:53 UTC (permalink / raw)
To: Ben Skeggs; +Cc: nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
On Fri, Oct 05, 2012 at 09:37:59PM +0200, Marcin Slusarz wrote:
> It's questionable use case, but weston/wayland already relies on this behaviour,
Important detail: weston does it only once per session.
> 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.
> Without it, process invoking such page flip hangs in unkillable state, trying
> to reserve the same buffer twice.
>
> Signed-off-by: Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> drivers/gpu/drm/nouveau/nouveau_display.c | 20 +++++++++++++++-----
> 1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
> index 61f370d..a52cfd3 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_display.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
> @@ -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;
> --
> 1.7.12
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] drm/nouveau: handle same-fb page flips
[not found] ` <20121005193759.GA8531-OI9uyE9O0yo@public.gmane.org>
2012-10-05 19:53 ` Marcin Slusarz
@ 2012-10-06 7:49 ` Pekka Paalanen
2012-10-16 21:40 ` Marcin Slusarz
2012-10-17 4:30 ` Ben Skeggs
3 siblings, 0 replies; 6+ messages in thread
From: Pekka Paalanen @ 2012-10-06 7:49 UTC (permalink / raw)
To: Marcin Slusarz; +Cc: nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
On Fri, 5 Oct 2012 21:37:59 +0200
Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 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 <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
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.
--
Pekka Paalanen
http://www.iki.fi/pq/
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] drm/nouveau: handle same-fb page flips
[not found] ` <20121005193759.GA8531-OI9uyE9O0yo@public.gmane.org>
2012-10-05 19:53 ` Marcin Slusarz
2012-10-06 7:49 ` Pekka Paalanen
@ 2012-10-16 21:40 ` Marcin Slusarz
2012-10-17 4:30 ` Ben Skeggs
3 siblings, 0 replies; 6+ messages in thread
From: Marcin Slusarz @ 2012-10-16 21:40 UTC (permalink / raw)
To: Ben Skeggs; +Cc: nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
On Fri, Oct 05, 2012 at 09:37:59PM +0200, Marcin Slusarz wrote:
> 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 <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> drivers/gpu/drm/nouveau/nouveau_display.c | 20 +++++++++++++++-----
> 1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
> index 61f370d..a52cfd3 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_display.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
> @@ -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;
> --
ping
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] drm/nouveau: handle same-fb page flips
[not found] ` <20121005193759.GA8531-OI9uyE9O0yo@public.gmane.org>
` (2 preceding siblings ...)
2012-10-16 21:40 ` Marcin Slusarz
@ 2012-10-17 4:30 ` Ben Skeggs
[not found] ` <20121017043056.GA4049-yqdYmcOkqV0XGNroddHbYwC/G2K4zDHf@public.gmane.org>
3 siblings, 1 reply; 6+ messages in thread
From: Ben Skeggs @ 2012-10-17 4:30 UTC (permalink / raw)
To: Marcin Slusarz; +Cc: nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
On Fri, Oct 05, 2012 at 09:37:59PM +0200, Marcin Slusarz wrote:
> 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 <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> drivers/gpu/drm/nouveau/nouveau_display.c | 20 +++++++++++++++-----
> 1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
> index 61f370d..a52cfd3 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_display.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
> @@ -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));
> + }
> +
The patch looks alright, except I think we should just drop this hunk,
no other driver reports it either.
Thoughts?
Ben.
> s = kzalloc(sizeof(*s), GFP_KERNEL);
> if (!s)
> return -ENOMEM;
> --
> 1.7.12
>
^ permalink raw reply [flat|nested] 6+ messages in thread