All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/nouveau: handle same-fb page flips
@ 2012-10-05 19:37 Marcin Slusarz
       [not found] ` <20121005193759.GA8531-OI9uyE9O0yo@public.gmane.org>
  0 siblings, 1 reply; 6+ messages in thread
From: Marcin Slusarz @ 2012-10-05 19:37 UTC (permalink / raw)
  To: Ben Skeggs; +Cc: nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW

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;
-- 
1.7.12

^ permalink raw reply related	[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
                     ` (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
* Re: [PATCH] drm/nouveau: handle same-fb page flips
       [not found]     ` <20121017043056.GA4049-yqdYmcOkqV0XGNroddHbYwC/G2K4zDHf@public.gmane.org>
@ 2012-10-17  5:51       ` Marcin Ślusarz
  0 siblings, 0 replies; 6+ messages in thread
From: Marcin Ślusarz @ 2012-10-17  5:51 UTC (permalink / raw)
  To: Ben Skeggs; +Cc: nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW


[-- Attachment #1.1: Type: text/plain, Size: 2706 bytes --]

17 paź 2012 06:31, "Ben Skeggs" <skeggsb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> napisał(a):
>
> 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?

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

[-- Attachment #1.2: Type: text/html, Size: 3600 bytes --]

[-- Attachment #2: Type: text/plain, Size: 181 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-10-17  5:51 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-10-05 19:37 [PATCH] drm/nouveau: handle same-fb page flips Marcin Slusarz
     [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
     [not found]     ` <20121017043056.GA4049-yqdYmcOkqV0XGNroddHbYwC/G2K4zDHf@public.gmane.org>
2012-10-17  5:51       ` Marcin Ślusarz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.