[PATCH] nouveau/dri2: don't try to page flip pixmaps

[PATCH] nouveau/dri2: don't try to page flip pixmaps

[Marcin Slusarz]

Port of commit ae45d7e6d8e6844cd4586c9ee97c21b257fa788f in xf86-video-ati.

Fixes https://bugs.freedesktop.org/show_bug.cgi?id=49351

(Additionally, don't try to pageflip if user disabled it in xorg.conf.
Currently this change is a no-op, because can_exchange returns true only when
page flipping is enabled, but commit 169512fbe91f0671a90dfee5e280357f0a4ef701 -
which changed can_exchange behavior - is due to be reverted)
---
 src/nouveau_dri2.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/src/nouveau_dri2.c b/src/nouveau_dri2.c
index 588735f..3d8d22f 100644
--- a/src/nouveau_dri2.c
+++ b/src/nouveau_dri2.c
@@ -328,7 +328,8 @@ nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame,
 		type = DRI2_EXCHANGE_COMPLETE;
 		DamageRegionAppend(draw, &reg);
 
-		if (DRI2CanFlip(draw)) {
+		if (DRI2CanFlip(draw) && pNv->has_pageflip &&
+				draw->type == DRAWABLE_WINDOW) {
 			type = DRI2_FLIP_COMPLETE;
 			ret = drmmode_page_flip(draw, src_pix,
 						violate_oml(draw) ? NULL : s,
-- 
1.7.8.5

Re: [PATCH] nouveau/dri2: don't try to page flip pixmaps

[Francisco Jerez]

[-- Attachment #1.1.1: Type: text/plain, Size: 1404 bytes --]

Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:

> Port of commit ae45d7e6d8e6844cd4586c9ee97c21b257fa788f in xf86-video-ati.
>
> Fixes https://bugs.freedesktop.org/show_bug.cgi?id=49351
>
> (Additionally, don't try to pageflip if user disabled it in xorg.conf.
> Currently this change is a no-op, because can_exchange returns true only when
> page flipping is enabled, but commit 169512fbe91f0671a90dfee5e280357f0a4ef701 -
> which changed can_exchange behavior - is due to be reverted)
> ---
>  src/nouveau_dri2.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/src/nouveau_dri2.c b/src/nouveau_dri2.c
> index 588735f..3d8d22f 100644
> --- a/src/nouveau_dri2.c
> +++ b/src/nouveau_dri2.c
> @@ -328,7 +328,8 @@ nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame,
>  		type = DRI2_EXCHANGE_COMPLETE;
>  		DamageRegionAppend(draw, &reg);
>  
> -		if (DRI2CanFlip(draw)) {
> +		if (DRI2CanFlip(draw) && pNv->has_pageflip &&
> +				draw->type == DRAWABLE_WINDOW) {

Hey,

How about 'if (nouveau_exa_pixmap_is_onscreen(dst_pix)) {...'?  We
should really never get to that point unless we know for sure that we
can either flip or exchange, so the 'has_pageflip' check is redundant.

>  			type = DRI2_FLIP_COMPLETE;
>  			ret = drmmode_page_flip(draw, src_pix,
>  						violate_oml(draw) ? NULL : s,

[-- Attachment #1.2: Type: application/pgp-signature, Size: 229 bytes --]

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

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

Re: [PATCH] nouveau/dri2: don't try to page flip pixmaps

[Marcin Slusarz]

On Thu, May 03, 2012 at 03:15:51PM +0200, Francisco Jerez wrote:
> Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
> 
> > Port of commit ae45d7e6d8e6844cd4586c9ee97c21b257fa788f in xf86-video-ati.
> >
> > Fixes https://bugs.freedesktop.org/show_bug.cgi?id=49351
> >
> > (Additionally, don't try to pageflip if user disabled it in xorg.conf.
> > Currently this change is a no-op, because can_exchange returns true only when
> > page flipping is enabled, but commit 169512fbe91f0671a90dfee5e280357f0a4ef701 -
> > which changed can_exchange behavior - is due to be reverted)
> > ---
> >  src/nouveau_dri2.c |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/src/nouveau_dri2.c b/src/nouveau_dri2.c
> > index 588735f..3d8d22f 100644
> > --- a/src/nouveau_dri2.c
> > +++ b/src/nouveau_dri2.c
> > @@ -328,7 +328,8 @@ nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame,
> >  		type = DRI2_EXCHANGE_COMPLETE;
> >  		DamageRegionAppend(draw, &reg);
> >  
> > -		if (DRI2CanFlip(draw)) {
> > +		if (DRI2CanFlip(draw) && pNv->has_pageflip &&
> > +				draw->type == DRAWABLE_WINDOW) {
> 
> Hey,
> 
> How about 'if (nouveau_exa_pixmap_is_onscreen(dst_pix)) {...'?  We
> should really never get to that point unless we know for sure that we
> can either flip or exchange, so the 'has_pageflip' check is redundant.

I see your point, but it's all non obvious. How about this patch instead?
:)

---
From: Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Subject: [PATCH] dri2: don't try to page flip pixmaps

Based on commit ae45d7e6d8e6844cd4586c9ee97c21b257fa788f in xf86-video-ati.

Fixes https://bugs.freedesktop.org/show_bug.cgi?id=49351
---
 src/nouveau_dri2.c |   31 ++++++++++++++++++++-----------
 1 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/src/nouveau_dri2.c b/src/nouveau_dri2.c
index 588735f..86039ef 100644
--- a/src/nouveau_dri2.c
+++ b/src/nouveau_dri2.c
@@ -178,12 +178,14 @@ update_front(DrawablePtr draw, DRI2BufferPtr front)
 }
 
 static Bool
-can_exchange(DrawablePtr draw, PixmapPtr dst_pix, PixmapPtr src_pix)
+can_exchange_or_flip(DrawablePtr draw, PixmapPtr dst_pix, PixmapPtr src_pix,
+		Bool *flip)
 {
 	ScrnInfoPtr scrn = xf86Screens[draw->pScreen->myNum];
 	xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
 	NVPtr pNv = NVPTR(scrn);
 	int i;
+	Bool dst_is_screen;
 
 	for (i = 0; i < xf86_config->num_crtc; i++) {
 		xf86CrtcPtr crtc = xf86_config->crtc[i];
@@ -192,11 +194,17 @@ can_exchange(DrawablePtr draw, PixmapPtr dst_pix, PixmapPtr src_pix)
 
 	}
 
-	return ((DRI2CanFlip(draw) && pNv->has_pageflip)) &&
-		dst_pix->drawable.width == src_pix->drawable.width &&
-		dst_pix->drawable.height == src_pix->drawable.height &&
-		dst_pix->drawable.bitsPerPixel == src_pix->drawable.bitsPerPixel &&
-		dst_pix->devKind == src_pix->devKind;
+	if (dst_pix->drawable.width != src_pix->drawable.width ||
+			dst_pix->drawable.height != src_pix->drawable.height ||
+			dst_pix->drawable.bitsPerPixel != src_pix->drawable.bitsPerPixel ||
+			dst_pix->devKind != src_pix->devKind)
+		return FALSE;
+
+	dst_is_screen = nouveau_exa_pixmap_is_onscreen(dst_pix);
+	*flip = dst_is_screen && pNv->has_pageflip && DRI2CanFlip(draw);
+
+	/* Exchange path is currently disabled, see fdo bug 35930 */
+	return *flip /*|| !dst_is_screen*/;
 }
 
 static Bool
@@ -280,7 +288,7 @@ nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame,
 	struct nouveau_pushbuf *push = pNv->pushbuf;
 	RegionRec reg;
 	int type, ret;
-	Bool front_updated;
+	Bool front_updated, flip;
 
 	REGION_INIT(0, &reg, (&(BoxRec){ 0, 0, draw->width, draw->height }), 0);
 	REGION_TRANSLATE(0, &reg, draw->x, draw->y);
@@ -324,17 +332,18 @@ nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame,
 		nouveau_pushbuf_kick(push, push->channel);
 	}
 
-	if (front_updated && can_exchange(draw, dst_pix, src_pix)) {
-		type = DRI2_EXCHANGE_COMPLETE;
+	if (front_updated && can_exchange_or_flip(draw, dst_pix, src_pix, &flip)) {
 		DamageRegionAppend(draw, &reg);
 
-		if (DRI2CanFlip(draw)) {
+		if (flip) {
 			type = DRI2_FLIP_COMPLETE;
 			ret = drmmode_page_flip(draw, src_pix,
 						violate_oml(draw) ? NULL : s,
 						ref_crtc_hw_id);
 			if (!ret)
 				goto out;
+		} else {
+			type = DRI2_EXCHANGE_COMPLETE;
 		}
 
 		SWAP(s->dst->name, s->src->name);
@@ -343,7 +352,7 @@ nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame,
 		DamageRegionProcessPending(draw);
 
 		/* If it is a page flip, finish it in the flip event handler. */
-		if ((type == DRI2_FLIP_COMPLETE) && !violate_oml(draw))
+		if (flip && !violate_oml(draw))
 			return;
 	} else {
 		type = DRI2_BLIT_COMPLETE;
-- 
1.7.8.5

Re: [PATCH] nouveau/dri2: don't try to page flip pixmaps

[Francisco Jerez]

[-- Attachment #1.1.1: Type: text/plain, Size: 5713 bytes --]

Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:

> On Thu, May 03, 2012 at 03:15:51PM +0200, Francisco Jerez wrote:
>> Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
>> 
>> > Port of commit ae45d7e6d8e6844cd4586c9ee97c21b257fa788f in xf86-video-ati.
>> >
>> > Fixes https://bugs.freedesktop.org/show_bug.cgi?id=49351
>> >
>> > (Additionally, don't try to pageflip if user disabled it in xorg.conf.
>> > Currently this change is a no-op, because can_exchange returns true only when
>> > page flipping is enabled, but commit 169512fbe91f0671a90dfee5e280357f0a4ef701 -
>> > which changed can_exchange behavior - is due to be reverted)
>> > ---
>> >  src/nouveau_dri2.c |    3 ++-
>> >  1 files changed, 2 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/src/nouveau_dri2.c b/src/nouveau_dri2.c
>> > index 588735f..3d8d22f 100644
>> > --- a/src/nouveau_dri2.c
>> > +++ b/src/nouveau_dri2.c
>> > @@ -328,7 +328,8 @@ nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame,
>> >  		type = DRI2_EXCHANGE_COMPLETE;
>> >  		DamageRegionAppend(draw, &reg);
>> >  
>> > -		if (DRI2CanFlip(draw)) {
>> > +		if (DRI2CanFlip(draw) && pNv->has_pageflip &&
>> > +				draw->type == DRAWABLE_WINDOW) {
>> 
>> Hey,
>> 
>> How about 'if (nouveau_exa_pixmap_is_onscreen(dst_pix)) {...'?  We
>> should really never get to that point unless we know for sure that we
>> can either flip or exchange, so the 'has_pageflip' check is redundant.
>
> I see your point, but it's all non obvious. How about this patch instead?
> :)
>
I guess the confusion stems from the meaning of "exchange": Do you
exchange as well as you flip or does one exclude the other?  This code
only (used to) make sense if you read it the first way, i.e. as if
"flip" were the subset of "exchange" you use with on-screen windows. :P

That said, I'm OK with changing the name of "can_exchange" to
"can_exchange_or_flip" if that makes its meaning clearer.  TBH the rest
of the changes in this patch don't really look to me like they're making
anything easier to understand.  That's quite a personal matter though.

> ---
> From: Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Subject: [PATCH] dri2: don't try to page flip pixmaps
>
> Based on commit ae45d7e6d8e6844cd4586c9ee97c21b257fa788f in xf86-video-ati.
>
> Fixes https://bugs.freedesktop.org/show_bug.cgi?id=49351
> ---
>  src/nouveau_dri2.c |   31 ++++++++++++++++++++-----------
>  1 files changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/src/nouveau_dri2.c b/src/nouveau_dri2.c
> index 588735f..86039ef 100644
> --- a/src/nouveau_dri2.c
> +++ b/src/nouveau_dri2.c
> @@ -178,12 +178,14 @@ update_front(DrawablePtr draw, DRI2BufferPtr front)
>  }
>  
>  static Bool
> -can_exchange(DrawablePtr draw, PixmapPtr dst_pix, PixmapPtr src_pix)
> +can_exchange_or_flip(DrawablePtr draw, PixmapPtr dst_pix, PixmapPtr src_pix,
> +		Bool *flip)
>  {
>  	ScrnInfoPtr scrn = xf86Screens[draw->pScreen->myNum];
>  	xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
>  	NVPtr pNv = NVPTR(scrn);
>  	int i;
> +	Bool dst_is_screen;
>  
>  	for (i = 0; i < xf86_config->num_crtc; i++) {
>  		xf86CrtcPtr crtc = xf86_config->crtc[i];
> @@ -192,11 +194,17 @@ can_exchange(DrawablePtr draw, PixmapPtr dst_pix, PixmapPtr src_pix)
>  
>  	}
>  
> -	return ((DRI2CanFlip(draw) && pNv->has_pageflip)) &&
> -		dst_pix->drawable.width == src_pix->drawable.width &&
> -		dst_pix->drawable.height == src_pix->drawable.height &&
> -		dst_pix->drawable.bitsPerPixel == src_pix->drawable.bitsPerPixel &&
> -		dst_pix->devKind == src_pix->devKind;
> +	if (dst_pix->drawable.width != src_pix->drawable.width ||
> +			dst_pix->drawable.height != src_pix->drawable.height ||
> +			dst_pix->drawable.bitsPerPixel != src_pix->drawable.bitsPerPixel ||
> +			dst_pix->devKind != src_pix->devKind)
> +		return FALSE;
> +
> +	dst_is_screen = nouveau_exa_pixmap_is_onscreen(dst_pix);
> +	*flip = dst_is_screen && pNv->has_pageflip && DRI2CanFlip(draw);
> +
> +	/* Exchange path is currently disabled, see fdo bug 35930 */
> +	return *flip /*|| !dst_is_screen*/;
>  }
>  
>  static Bool
> @@ -280,7 +288,7 @@ nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame,
>  	struct nouveau_pushbuf *push = pNv->pushbuf;
>  	RegionRec reg;
>  	int type, ret;
> -	Bool front_updated;
> +	Bool front_updated, flip;
>  
>  	REGION_INIT(0, &reg, (&(BoxRec){ 0, 0, draw->width, draw->height }), 0);
>  	REGION_TRANSLATE(0, &reg, draw->x, draw->y);
> @@ -324,17 +332,18 @@ nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame,
>  		nouveau_pushbuf_kick(push, push->channel);
>  	}
>  
> -	if (front_updated && can_exchange(draw, dst_pix, src_pix)) {
> -		type = DRI2_EXCHANGE_COMPLETE;
> +	if (front_updated && can_exchange_or_flip(draw, dst_pix, src_pix, &flip)) {
>  		DamageRegionAppend(draw, &reg);
>  
> -		if (DRI2CanFlip(draw)) {
> +		if (flip) {
>  			type = DRI2_FLIP_COMPLETE;
>  			ret = drmmode_page_flip(draw, src_pix,
>  						violate_oml(draw) ? NULL : s,
>  						ref_crtc_hw_id);
>  			if (!ret)
>  				goto out;
> +		} else {
> +			type = DRI2_EXCHANGE_COMPLETE;
>  		}
>  
>  		SWAP(s->dst->name, s->src->name);
> @@ -343,7 +352,7 @@ nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame,
>  		DamageRegionProcessPending(draw);
>  
>  		/* If it is a page flip, finish it in the flip event handler. */
> -		if ((type == DRI2_FLIP_COMPLETE) && !violate_oml(draw))
> +		if (flip && !violate_oml(draw))
>  			return;
>  	} else {
>  		type = DRI2_BLIT_COMPLETE;

[-- Attachment #1.2: Type: application/pgp-signature, Size: 229 bytes --]

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

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

Re: [PATCH] nouveau/dri2: don't try to page flip pixmaps

[Marcin Slusarz]

On Fri, May 04, 2012 at 12:31:04AM +0200, Francisco Jerez wrote:
> Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
> 
> > On Thu, May 03, 2012 at 03:15:51PM +0200, Francisco Jerez wrote:
> >> Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
> >> 
> >> > Port of commit ae45d7e6d8e6844cd4586c9ee97c21b257fa788f in xf86-video-ati.
> >> >
> >> > Fixes https://bugs.freedesktop.org/show_bug.cgi?id=49351
> >> >
> >> > (Additionally, don't try to pageflip if user disabled it in xorg.conf.
> >> > Currently this change is a no-op, because can_exchange returns true only when
> >> > page flipping is enabled, but commit 169512fbe91f0671a90dfee5e280357f0a4ef701 -
> >> > which changed can_exchange behavior - is due to be reverted)
> >> > ---
> >> >  src/nouveau_dri2.c |    3 ++-
> >> >  1 files changed, 2 insertions(+), 1 deletions(-)
> >> >
> >> > diff --git a/src/nouveau_dri2.c b/src/nouveau_dri2.c
> >> > index 588735f..3d8d22f 100644
> >> > --- a/src/nouveau_dri2.c
> >> > +++ b/src/nouveau_dri2.c
> >> > @@ -328,7 +328,8 @@ nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame,
> >> >  		type = DRI2_EXCHANGE_COMPLETE;
> >> >  		DamageRegionAppend(draw, &reg);
> >> >  
> >> > -		if (DRI2CanFlip(draw)) {
> >> > +		if (DRI2CanFlip(draw) && pNv->has_pageflip &&
> >> > +				draw->type == DRAWABLE_WINDOW) {
> >> 
> >> Hey,
> >> 
> >> How about 'if (nouveau_exa_pixmap_is_onscreen(dst_pix)) {...'?  We
> >> should really never get to that point unless we know for sure that we
> >> can either flip or exchange, so the 'has_pageflip' check is redundant.
> >
> > I see your point, but it's all non obvious. How about this patch instead?
> > :)
> >
> I guess the confusion stems from the meaning of "exchange": Do you
> exchange as well as you flip or does one exclude the other?  This code
> only (used to) make sense if you read it the first way, i.e. as if
> "flip" were the subset of "exchange" you use with on-screen windows. :P

Heh, that makes sense now.

> That said, I'm OK with changing the name of "can_exchange" to
> "can_exchange_or_flip" if that makes its meaning clearer.  TBH the rest
> of the changes in this patch don't really look to me like they're making
> anything easier to understand.  That's quite a personal matter though.

I wanted to take the decision about pageflipping or not in one place instead
of two.

However, I do not feel strongly about which way it's going to be fixed. Both
are technically correct. Feel free to commit your version.

Marcin