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

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(-)

src/nouveau_dri2.c +0 -0

index 588735f..3d8d22f 100644

Quoted 17 lines · 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(-)

src/nouveau_dri2.c +0 -0

index 588735f..3d8d22f 100644

src/nouveau_dri2.c nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame, · line 332

Quoted 6 lines

src/nouveau_dri2.c +2 -1

@@ -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.

Quoted 28 lines · Francisco Jerez

Quoted 17 lines · 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(-)

src/nouveau_dri2.c +0 -0

index 588735f..3d8d22f 100644

src/nouveau_dri2.c nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame, · line 332

Quoted 6 lines

src/nouveau_dri2.c +2 -1

@@ -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,

Quoted 3 lines

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 <[email protected]>
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(-)

src/nouveau_dri2.c +19 -10

index 588735f..86039ef 100644
@@ -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);

Quoted 33 lines · Marcin Slusarz

Quoted 31 lines · Francisco Jerez

Quoted 17 lines · 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(-)

src/nouveau_dri2.c +0 -0

index 588735f..3d8d22f 100644

src/nouveau_dri2.c nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame, · line 332

Quoted 6 lines

src/nouveau_dri2.c +2 -1

@@ -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.

Quoted 3 lines

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.