[Patches][nouveau/ddx]: Improvements to bufferswap implementation and timestamping

[Patches][nouveau/ddx]: Improvements to bufferswap implementation and timestamping

[Mario Kleiner]

Hi,

here a set of patches against the nouveau-ddx. This is an extended and
revised set, based on Francisco Jerez feedback from autumn last year.

[1/9] Makes pageflipping work again on X-Server 1.12rc. It apparently stopped
working somewhere around Xorg 1.11+.

[2/9] Implements handling of pageflip completion events from the kernel.
Francisco Jerez argument against including it was that the x-server didn't
have a swaplimit api, so this couldn't be applied at the same time as the
pseudo triple-buffering hack which is in place in the current ddx.
Now we have the swaplimit api in 1.12, so this problem should be solved.

[3/9] Makes use of the swaplimit api. A new xorg.conf option "SwapLimit"
allows to select a swaplimit of 1 or 2 for double-buffering or
triple-buffering. It defaults to 2 for Xorg 1.12+ and 1 for older servers.
This way, on 1.12+ nouveau retains the kind of triple buffering behaviour it
currently has, but swap completion timestamping (OML_sync_control,
INTEL_swap_events, and client swap throttling) works conforming to
the specs. On older servers it removes triple-buffering but makes
nouveau conform to the specs. This is important for apps that need
precise and reliable swap scheduling and timestamping.

[4/9] A bug fix against corrupted desktop when switching from redirected
to non-redirected fullscreen windows under a desktop compositor.
Fixes FDO bug #35452.

[5/9] Some fixes to swap scheduling, revised according to Francisco's
review.

[6/9] Fixes swap throttling for non-fullscreen windows under a desktop
compositor. Split into a separate patch according to Francisco's
feedback.

[7/9] An attempt to provide more sane swap completion events for non-
fullscreen windows. This is a bit of cheating, as it delivers the
events for the earliest point in time one would expect the swap
to complete, assuming only a lightly loaded gpu. Real completion
could be later. I think this is an "improvement" because the current
implementation delivers swap complete events with timestamps and
counts that signal swap completion before the client even requested
the swap.

[8/9] This one adds Francisco's original triple-buffering hack back
for Xorg 1.11 and earlier servers if the xorg.conf option requests
a swaplimit > 1. Users can choose between "triple-buffering" but
broken timestamping or double-buffering with sane timestamping.

[9/9] Fixes a corner case which could cause the ddx to segfault
with its current triple-buffering implementation.

I've tested these on single-display and dual-display setups,
with/without compositor, with/without redirection and checked
the robustness and precision of the timestamps with special
measurement equipment, on XOrg 1.12-rc2 and 1.10. They work
pretty well for me and finally make nouveau very useable for
the kind of scientific applications that require precise
swap scheduling and timestamping, so i'd love to see them
reviewed and hopefully included into the ddx soon.

A couple of things are a bit of hacks:

[3/9] I think the setup of a default swap limit would be
better done in the x-server itself instead of the ddx. The
setup code is a bit awkward, hijacking the ddx->CreateBuffer
function to apply the swaplimit. A DRI2GetSwapLimit() function
is also missing from the server, which could help in the
future to track swaplimit changes by other clients than the ddx
itself. Unfortunately it is too late for the 1.12 release to do
this.

[8/9] Don't know if this is still wanted/needed or not?

[9/9] It fixes the problem and doesn't affect performance, but is
somewhat of a hack. I don't know how to do better, maybe somebody
else has a better solution?

Thanks,
-mario

[PATCH 1/9] dri2: Fix can_exchange() to allow page-flipping on new servers.

[Mario Kleiner]

can_exchange() fails on at least Xorg 1.12+. This fixes
it in the same way it was fixed in the ati & intel ddx.

Signed-off-by: Mario Kleiner <mario.kleiner-TdbV1Z3I5XE0NhjG498hmQ@public.gmane.org>
---
 src/nouveau_dri2.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/nouveau_dri2.c b/src/nouveau_dri2.c
index 3aa5ec5..5b62425 100644
--- a/src/nouveau_dri2.c
+++ b/src/nouveau_dri2.c
@@ -160,7 +160,7 @@ 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.depth == src_pix->drawable.depth &&
+		dst_pix->drawable.bitsPerPixel == src_pix->drawable.bitsPerPixel &&
 		dst_pix->devKind == src_pix->devKind;
 }
 
-- 
1.7.5.4

[PATCH 2/9] dri2: Implement handling of pageflip completion events.

[Mario Kleiner]

Requests pageflip completion events from the kernel.
Implements pageflip completion handler to finalize
and timestamp swaps.

Completion handler includes a consistency check, and
disambiguation if multiple crtc's are involved in a
pageflip (e.g., clone mode, extendend desktop). Only
the timestamp of the crtc whose vblank event initially
triggered the swap is used, but handler waits for flip
completion on all involved crtc's before completing the
swap and releasing the old framebuffer.

This code is almost identical to the code used in the
ati/radeon ddx and intel ddx.

Signed-off-by: Mario Kleiner <mario.kleiner-TdbV1Z3I5XE0NhjG498hmQ@public.gmane.org>
---
 src/drmmode_display.c |  107 +++++++++++++++++++++++++++++++++++++++++++++++--
 src/nouveau_dri2.c    |   89 ++++++++++++++++++++++++++++++++++++++--
 src/nv_proto.h        |    5 ++-
 3 files changed, 191 insertions(+), 10 deletions(-)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c
index 75ef6dd..9e15c29 100644
--- a/src/drmmode_display.c
+++ b/src/drmmode_display.c
@@ -83,6 +83,21 @@ typedef struct {
     drmmode_prop_ptr props;
 } drmmode_output_private_rec, *drmmode_output_private_ptr;
 
+typedef struct {
+    drmmode_ptr drmmode;
+    unsigned old_fb_id;
+    int flip_count;
+    void *event_data;
+    unsigned int fe_frame;
+    unsigned int fe_tv_sec;
+    unsigned int fe_tv_usec;
+} drmmode_flipdata_rec, *drmmode_flipdata_ptr;
+
+typedef struct {
+    drmmode_flipdata_ptr flipdata;
+    Bool dispatch_me;
+} drmmode_flipevtcarrier_rec, *drmmode_flipevtcarrier_ptr;
+
 static void drmmode_output_dpms(xf86OutputPtr output, int mode);
 
 static drmmode_ptr
@@ -1245,13 +1260,17 @@ drmmode_cursor_init(ScreenPtr pScreen)
 }
 
 Bool
-drmmode_page_flip(DrawablePtr draw, PixmapPtr back, void *priv)
+drmmode_page_flip(DrawablePtr draw, PixmapPtr back, void *priv,
+		  unsigned int ref_crtc_hw_id)
 {
 	ScrnInfoPtr scrn = xf86Screens[draw->pScreen->myNum];
 	xf86CrtcConfigPtr config = XF86_CRTC_CONFIG_PTR(scrn);
 	drmmode_crtc_private_ptr crtc = config->crtc[0]->driver_private;
 	drmmode_ptr mode = crtc->drmmode;
 	int ret, i, old_fb_id;
+	int emitted = 0;
+	drmmode_flipdata_ptr flipdata;
+	drmmode_flipevtcarrier_ptr flipcarrier;
 
 	old_fb_id = mode->fb_id;
 	ret = drmModeAddFB(mode->fd, scrn->virtualX, scrn->virtualY,
@@ -1264,24 +1283,64 @@ drmmode_page_flip(DrawablePtr draw, PixmapPtr back, void *priv)
 		return FALSE;
 	}
 
+	flipdata = calloc(1, sizeof(drmmode_flipdata_rec));
+	if (!flipdata) {
+		xf86DrvMsg(scrn->scrnIndex, X_WARNING,
+		"flip queue: data alloc failed.\n");
+		goto error_undo;
+	}
+
+	flipdata->event_data = priv;
+	flipdata->drmmode = mode;
+
 	for (i = 0; i < config->num_crtc; i++) {
 		crtc = config->crtc[i]->driver_private;
 
 		if (!config->crtc[i]->enabled)
 			continue;
 
+		flipdata->flip_count++;
+
+		flipcarrier = calloc(1, sizeof(drmmode_flipevtcarrier_rec));
+		if (!flipcarrier) {
+			xf86DrvMsg(scrn->scrnIndex, X_WARNING,
+				   "flip queue: carrier alloc failed.\n");
+			if (emitted == 0)
+				free(flipdata);
+			goto error_undo;
+		}
+
+		/* Only the reference crtc will finally deliver its page flip
+		 * completion event. All other crtc's events will be discarded.
+		 */
+		flipcarrier->dispatch_me = ((1 << i) == ref_crtc_hw_id);
+		flipcarrier->flipdata = flipdata;
+
 		ret = drmModePageFlip(mode->fd, crtc->mode_crtc->crtc_id,
-				      mode->fb_id, 0, priv);
+				      mode->fb_id, DRM_MODE_PAGE_FLIP_EVENT,
+				      flipcarrier);
 		if (ret) {
 			xf86DrvMsg(scrn->scrnIndex, X_WARNING,
 				   "flip queue failed: %s\n", strerror(errno));
-			return FALSE;
+
+			free(flipcarrier);
+			if (emitted == 0)
+				free(flipdata);
+			goto error_undo;
 		}
+
+		emitted++;
 	}
 
-	drmModeRmFB(mode->fd, old_fb_id);
+	/* Will release old fb after all crtc's completed flip. */
+	flipdata->old_fb_id = old_fb_id;
 
 	return TRUE;
+
+error_undo:
+	drmModeRmFB(mode->fd, mode->fb_id);
+	mode->fb_id = old_fb_id;
+	return FALSE;
 }
 
 #ifdef HAVE_LIBUDEV
@@ -1347,6 +1406,42 @@ drmmode_uevent_fini(ScrnInfoPtr scrn)
 }
 
 static void
+drmmode_flip_handler(int fd, unsigned int frame, unsigned int tv_sec,
+		     unsigned int tv_usec, void *event_data)
+{
+	drmmode_flipevtcarrier_ptr flipcarrier = event_data;
+	drmmode_flipdata_ptr flipdata = flipcarrier->flipdata;
+	drmmode_ptr drmmode = flipdata->drmmode;
+
+	/* Is this the event whose info shall be delivered to higher level? */
+	if (flipcarrier->dispatch_me) {
+		/* Yes: Cache msc, ust for later delivery. */
+		flipdata->fe_frame = frame;
+		flipdata->fe_tv_sec = tv_sec;
+		flipdata->fe_tv_usec = tv_usec;
+	}
+	free(flipcarrier);
+
+	/* Last crtc completed flip? */
+	flipdata->flip_count--;
+	if (flipdata->flip_count > 0)
+		return;
+
+	/* Release framebuffer */
+	drmModeRmFB(drmmode->fd, flipdata->old_fb_id);
+
+	if (flipdata->event_data == NULL) {
+		free(flipdata);
+		return;
+	}
+
+	/* Deliver cached msc, ust from reference crtc to flip event handler */
+	nouveau_dri2_flip_event_handler(flipdata->fe_frame, flipdata->fe_tv_sec,
+					flipdata->fe_tv_usec, flipdata->event_data);
+	free(flipdata);
+}
+
+static void
 drmmode_wakeup_handler(pointer data, int err, pointer p)
 {
 	ScrnInfoPtr scrn = data;
@@ -1376,6 +1471,10 @@ drmmode_screen_init(ScreenPtr pScreen)
 	/* Plug in a vblank event handler */
 	drmmode->event_context.version = DRM_EVENT_CONTEXT_VERSION;
 	drmmode->event_context.vblank_handler = nouveau_dri2_vblank_handler;
+
+	/* Plug in a pageflip completion event handler */
+	drmmode->event_context.page_flip_handler = drmmode_flip_handler;
+
 	AddGeneralSocket(drmmode->fd);
 
 	/* Register a wakeup handler to get informed on DRM events */
diff --git a/src/nouveau_dri2.c b/src/nouveau_dri2.c
index 5b62425..acef08a 100644
--- a/src/nouveau_dri2.c
+++ b/src/nouveau_dri2.c
@@ -140,6 +140,7 @@ struct nouveau_dri2_vblank_state {
 	DRI2BufferPtr src;
 	DRI2SwapEventPtr func;
 	void *data;
+	unsigned int frame;
 };
 
 static Bool
@@ -225,6 +226,18 @@ nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame,
 	REGION_INIT(0, &reg, (&(BoxRec){ 0, 0, draw->width, draw->height }), 0);
 	REGION_TRANSLATE(0, &reg, draw->x, draw->y);
 
+	/* Main crtc for this drawable shall finally deliver pageflip event. */
+	unsigned int ref_crtc_hw_id = nv_window_belongs_to_crtc(scrn, draw->x,
+								draw->y,
+								draw->width,
+								draw->height);
+
+	/* Whenever first crtc is involved, choose it as reference, as
+	 * its vblank event triggered this swap.
+	 */
+	if (ref_crtc_hw_id & 1)
+		ref_crtc_hw_id = 1;
+
 	/* Throttle on the previous frame before swapping */
 	nouveau_bo_map(dst_bo, NOUVEAU_BO_RD);
 	nouveau_bo_unmap(dst_bo);
@@ -249,7 +262,7 @@ nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame,
 
 		if (DRI2CanFlip(draw)) {
 			type = DRI2_FLIP_COMPLETE;
-			ret = drmmode_page_flip(draw, src_pix, s);
+			ret = drmmode_page_flip(draw, src_pix, s, ref_crtc_hw_id);
 			if (!ret)
 				goto out;
 		}
@@ -258,6 +271,10 @@ nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame,
 		SWAP(nouveau_pixmap(dst_pix)->bo, nouveau_pixmap(src_pix)->bo);
 
 		DamageRegionProcessPending(draw);
+
+		/* If it is a page flip, finish it in the flip event handler. */
+		if (type == DRI2_FLIP_COMPLETE)
+			return;
 	} else {
 		type = DRI2_BLIT_COMPLETE;
 
@@ -292,7 +309,7 @@ nouveau_dri2_schedule_swap(ClientPtr client, DrawablePtr draw,
 			   DRI2SwapEventPtr func, void *data)
 {
 	struct nouveau_dri2_vblank_state *s;
-	CARD64 current_msc;
+	CARD64 current_msc, expect_msc;
 	int ret;
 
 	/* Initialize a swap structure */
@@ -301,7 +318,7 @@ nouveau_dri2_schedule_swap(ClientPtr client, DrawablePtr draw,
 		return FALSE;
 
 	*s = (struct nouveau_dri2_vblank_state)
-		{ SWAP, client, draw->id, dst, src, func, data };
+		{ SWAP, client, draw->id, dst, src, func, data, 0 };
 
 	if (can_sync_to_vblank(draw)) {
 		/* Get current sequence */
@@ -319,10 +336,10 @@ nouveau_dri2_schedule_swap(ClientPtr client, DrawablePtr draw,
 		ret = nouveau_wait_vblank(draw, DRM_VBLANK_ABSOLUTE |
 					  DRM_VBLANK_EVENT,
 					  max(current_msc, *target_msc - 1),
-					  NULL, NULL, s);
+					  &expect_msc, NULL, s);
 		if (ret)
 			goto fail;
-
+		s->frame = (unsigned int) expect_msc & 0xffffffff;
 	} else {
 		/* We can't/don't want to sync to vblank, just swap. */
 		nouveau_dri2_finish_swap(draw, 0, 0, 0, s);
@@ -426,6 +443,68 @@ nouveau_dri2_vblank_handler(int fd, unsigned int frame,
 	}
 }
 
+void
+nouveau_dri2_flip_event_handler(unsigned int frame, unsigned int tv_sec,
+				unsigned int tv_usec, void *event_data)
+{
+	struct nouveau_dri2_vblank_state *flip = event_data;
+	DrawablePtr draw;
+	ScreenPtr screen;
+	ScrnInfoPtr scrn;
+	int status;
+	PixmapPtr pixmap;
+
+	status = dixLookupDrawable(&draw, flip->draw, serverClient,
+				   M_ANY, DixWriteAccess);
+	if (status != Success) {
+		free(flip);
+		return;
+	}
+
+	screen = draw->pScreen;
+	scrn = xf86Screens[screen->myNum];
+
+	pixmap = screen->GetScreenPixmap(screen);
+	xf86DrvMsgVerb(scrn->scrnIndex, X_INFO, 4,
+		       "%s: flipevent : width %d x height %d : msc %d : ust = %d.%06d\n",
+		       __func__, pixmap->drawable.width, pixmap->drawable.height,
+		       frame, tv_sec, tv_usec);
+
+	/* We assume our flips arrive in order, so we don't check the frame */
+	switch (flip->action) {
+	case SWAP:
+		/* Check for too small vblank count of pageflip completion,
+		 * taking wraparound into account. This usually means some
+		 * defective kms pageflip completion, causing wrong (msc, ust)
+		 * return values and possible visual corruption.
+		 * Skip test for frame == 0, as this is a valid constant value
+		 * reported by all Linux kernels at least up to Linux 3.0.
+		 */
+		if ((frame != 0) &&
+		    (frame < flip->frame) && (flip->frame - frame < 5)) {
+			xf86DrvMsg(scrn->scrnIndex, X_WARNING,
+				   "%s: Pageflip has impossible msc %d < target_msc %d\n",
+				   __func__, frame, flip->frame);
+			/* All-Zero values signal failure of (msc, ust)
+			 * timestamping to client.
+			 */
+			frame = tv_sec = tv_usec = 0;
+		}
+
+		DRI2SwapComplete(flip->client, draw, frame, tv_sec, tv_usec,
+				 DRI2_FLIP_COMPLETE, flip->func,
+				 flip->data);
+		break;
+	default:
+		xf86DrvMsg(scrn->scrnIndex, X_WARNING,
+			   "%s: unknown vblank event received\n", __func__);
+		/* Unknown type */
+		break;
+	}
+
+	free(flip);
+}
+
 Bool
 nouveau_dri2_init(ScreenPtr pScreen)
 {
diff --git a/src/nv_proto.h b/src/nv_proto.h
index 0b8e513..8bf2fc1 100644
--- a/src/nv_proto.h
+++ b/src/nv_proto.h
@@ -7,7 +7,8 @@ void drmmode_adjust_frame(ScrnInfoPtr pScrn, int x, int y, int flags);
 void drmmode_remove_fb(ScrnInfoPtr pScrn);
 Bool drmmode_cursor_init(ScreenPtr pScreen);
 void drmmode_fbcon_copy(ScreenPtr pScreen);
-Bool drmmode_page_flip(DrawablePtr draw, PixmapPtr back, void *priv);
+Bool drmmode_page_flip(DrawablePtr draw, PixmapPtr back, void *priv,
+		       unsigned int ref_crtc_hw_id);
 void drmmode_screen_init(ScreenPtr pScreen);
 void drmmode_screen_fini(ScreenPtr pScreen);
 
@@ -26,6 +27,8 @@ Bool nouveau_allocate_surface(ScrnInfoPtr scrn, int width, int height,
 void nouveau_dri2_vblank_handler(int fd, unsigned int frame,
 				 unsigned int tv_sec, unsigned int tv_usec,
 				 void *event_data);
+void nouveau_dri2_flip_event_handler(unsigned int frame, unsigned int tv_sec,
+				     unsigned int tv_usec, void *event_data);
 Bool nouveau_dri2_init(ScreenPtr pScreen);
 void nouveau_dri2_fini(ScreenPtr pScreen);
 
-- 
1.7.5.4

[PATCH 3/9] dri2: Add support for DRI2SwapLimit() API.

[Mario Kleiner]

Uses the new DRI2SwapLimit() API of X-Server 1.12+
to allow to change the maximum number of pending
swaps on a drawable before the OpenGL client is
throttled by the server.

The new optional xorg.conf parameter "SwapLimit"
allows to select a new swap limit >= 1. The default
swap limit is 2 for triple-buffering on XOrg 1.12+,
1 for double-buffering on older servers, as we can't
change the swap limit there.

Signed-off-by: Mario Kleiner <mario.kleiner-TdbV1Z3I5XE0NhjG498hmQ@public.gmane.org>
---
 man/nouveau.man    |   11 +++++++++++
 src/nouveau_dri2.c |   29 +++++++++++++++++++++++++++--
 src/nv_const.h     |    2 ++
 src/nv_driver.c    |   34 ++++++++++++++++++++++++++++++++++
 src/nv_type.h      |    3 +++
 5 files changed, 77 insertions(+), 2 deletions(-)

diff --git a/man/nouveau.man b/man/nouveau.man
index dd9d938..59f6c1a 100644
--- a/man/nouveau.man
+++ b/man/nouveau.man
@@ -93,6 +93,17 @@ will assign xrandr outputs LVDS and VGA-0 to this instance of the driver.
 .TP
 .BI "Option \*qPageFlip\*q \*q" boolean \*q
 Enable DRI2 page flipping. Default: on.
+.TP
+.BI "Option \*qSwapLimit\*q \*q" integer \*q
+Set maximum allowed number of pending OpenGL double-buffer swaps for
+a drawable before a client is blocked.
+.br
+A value of 1 corresponds to double-buffering. A value of 2 corresponds
+to triple-buffering. Higher values may allow higher framerate, but also
+increase lag for interactive applications, e.g., games. Nouveau currently
+supports a maximum value of 2 on XOrg 1.12+ and a maximum of 1 on older servers.
+.br
+Default: 2 for XOrg 1.12+, 1 for older servers.
 .SH "SEE ALSO"
 __xservername__(__appmansuffix__), __xconfigfile__(__filemansuffix__), Xserver(__appmansuffix__), X(__miscmansuffix__)
 .SH AUTHORS
diff --git a/src/nouveau_dri2.c b/src/nouveau_dri2.c
index acef08a..2908e56 100644
--- a/src/nouveau_dri2.c
+++ b/src/nouveau_dri2.c
@@ -42,6 +42,11 @@ nouveau_dri2_create_buffer(DrawablePtr pDraw, unsigned int attachment,
 		} else {
 			WindowPtr pwin = (WindowPtr)pDraw;
 			ppix = pScreen->GetWindowPixmap(pwin);
+
+#if DRI2INFOREC_VERSION >= 6
+			/* Set initial swap limit on drawable. */
+			DRI2SwapLimit(pDraw, pNv->swap_limit);
+#endif
 		}
 
 		ppix->refcnt++;
@@ -208,6 +213,20 @@ nouveau_wait_vblank(DrawablePtr draw, int type, CARD64 msc,
 	return 0;
 }
 
+#if DRI2INFOREC_VERSION >= 6
+static Bool
+nouveau_dri2_swap_limit_validate(DrawablePtr draw, int swap_limit)
+{
+	ScrnInfoPtr scrn = xf86Screens[draw->pScreen->myNum];
+	NVPtr pNv = NVPTR(scrn);
+
+	if ((swap_limit < 1 ) || (swap_limit > pNv->max_swap_limit))
+		return FALSE;
+
+	return TRUE;
+}
+#endif
+
 static void
 nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame,
 			 unsigned int tv_sec, unsigned int tv_usec,
@@ -293,8 +312,10 @@ nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame,
 	 * not, to prevent it from blocking the client on the next
 	 * GetBuffers request (and let the client do triple-buffering).
 	 *
-	 * XXX - The DRI2SwapLimit() API will allow us to move this to
-	 *	 the flip handler with no FPS hit.
+	 * XXX - The DRI2SwapLimit() API allowed us to move this to
+	 *	 the flip handler with no FPS hit for page flipped swaps.
+	 *       It is still needed for copy swaps as we lack a method
+	 *       to detect true swap completion for DRI2_BLIT_COMPLETE.
 	 */
 	DRI2SwapComplete(s->client, draw, frame, tv_sec, tv_usec,
 			 type, s->func, s->data);
@@ -534,6 +555,10 @@ nouveau_dri2_init(ScreenPtr pScreen)
 	dri2.ScheduleWaitMSC = nouveau_dri2_schedule_wait;
 	dri2.GetMSC = nouveau_dri2_get_msc;
 
+#if DRI2INFOREC_VERSION >= 6
+	dri2.SwapLimitValidate = nouveau_dri2_swap_limit_validate;
+#endif
+
 	return DRI2ScreenInit(pScreen, &dri2);
 }
 
diff --git a/src/nv_const.h b/src/nv_const.h
index a27a951..5c232d4 100644
--- a/src/nv_const.h
+++ b/src/nv_const.h
@@ -15,6 +15,7 @@ typedef enum {
     OPTION_GLX_VBLANK,
     OPTION_ZAPHOD_HEADS,
     OPTION_PAGE_FLIP,
+    OPTION_SWAP_LIMIT,
 } NVOpts;
 
 
@@ -28,6 +29,7 @@ static const OptionInfoRec NVOptions[] = {
     { OPTION_GLX_VBLANK,	"GLXVBlank",	OPTV_BOOLEAN,	{0}, FALSE },
     { OPTION_ZAPHOD_HEADS,	"ZaphodHeads",	OPTV_STRING,	{0}, FALSE },
     { OPTION_PAGE_FLIP,		"PageFlip",	OPTV_BOOLEAN,	{0}, FALSE },
+    { OPTION_SWAP_LIMIT,	"SwapLimit",	OPTV_INTEGER,	{0}, FALSE },
     { -1,                       NULL,           OPTV_NONE,      {0}, FALSE }
 };
 
diff --git a/src/nv_driver.c b/src/nv_driver.c
index 87ef2c4..5def531 100644
--- a/src/nv_driver.c
+++ b/src/nv_driver.c
@@ -31,6 +31,9 @@
 #include "xf86drm.h"
 #include "xf86drmMode.h"
 #include "nouveau_drm.h"
+#ifdef DRI2
+#include "dri2.h"
+#endif
 
 /*
  * Forward definitions for the functions that make up the driver.
@@ -847,6 +850,37 @@ NVPreInit(ScrnInfoPtr pScrn, int flags)
 		(((pScrn->mask.blue >> pScrn->offset.blue) - 1) << pScrn->offset.blue);
 	}
 
+	/* Limit to max 2 pending swaps - we can't handle more than triple-buffering: */
+	pNv->max_swap_limit = 2;
+
+	if(xf86GetOptValInteger(pNv->Options, OPTION_SWAP_LIMIT, &(pNv->swap_limit))) {
+		if (pNv->swap_limit < 1)
+			pNv->swap_limit = 1;
+
+		if (pNv->swap_limit > pNv->max_swap_limit)
+			pNv->swap_limit = pNv->max_swap_limit;
+
+		reason = "";
+		from = X_CONFIG;
+
+		if (DRI2INFOREC_VERSION < 6) {
+			/* No swap limit api in server. Stick to server default of 1. */
+			pNv->swap_limit = 1;
+			from = X_DEFAULT;
+			reason = ": This X-Server only supports a swap limit of 1.";
+		}
+	} else {
+		/* Driver default: Double buffering on old servers, triple-buffering
+		 * on Xorg 1.12+.
+		 */
+		pNv->swap_limit = (DRI2INFOREC_VERSION < 6) ? 1 : 2;
+		reason = "";
+		from = X_DEFAULT;
+	}
+
+	xf86DrvMsg(pScrn->scrnIndex, from, "Swap limit set to %d [Max allowed %d]%s\n",
+		   pNv->swap_limit, pNv->max_swap_limit, reason);
+
 	ret = drmmode_pre_init(pScrn, nouveau_device(pNv->dev)->fd,
 			       pScrn->bitsPerPixel >> 3);
 	if (ret == FALSE)
diff --git a/src/nv_type.h b/src/nv_type.h
index 4204556..73328fc 100644
--- a/src/nv_type.h
+++ b/src/nv_type.h
@@ -53,6 +53,9 @@ typedef struct _NVRec {
     Bool		tiled_scanout;
     Bool		glx_vblank;
     Bool		has_pageflip;
+    int 		swap_limit;
+    int 		max_swap_limit;
+
     ScreenBlockHandlerProcPtr BlockHandler;
     CreateScreenResourcesProcPtr CreateScreenResources;
     CloseScreenProcPtr  CloseScreen;
-- 
1.7.5.4

[PATCH 4/9] dri2: Update front buffer pixmap and name before exchanging buffers

[Mario Kleiner]

Buffer exchange assumes that the front buffer pixmap and name
information is accurate. That may not be the case eg. if the window
has been (un)redirected since the buffer was created.

This is a translation to nouveau of a fix that was originally developed
by Ville Syrjala <syrjala-ORSVBvAovxo@public.gmane.org> for the ati/radeon ddx to fix the
same bug there.

See thread at:

http://lists.x.org/archives/xorg-devel/2011-May/021908.html

Fixes FDO bug #35452.

Signed-off-by: Mario Kleiner <mario.kleiner-TdbV1Z3I5XE0NhjG498hmQ@public.gmane.org>
---
 src/nouveau_dri2.c |   45 ++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/src/nouveau_dri2.c b/src/nouveau_dri2.c
index 2908e56..8608678 100644
--- a/src/nouveau_dri2.c
+++ b/src/nouveau_dri2.c
@@ -149,6 +149,35 @@ struct nouveau_dri2_vblank_state {
 };
 
 static Bool
+update_front(DrawablePtr draw, DRI2BufferPtr front)
+{
+	int r;
+	PixmapPtr pixmap;
+	struct nouveau_dri2_buffer *nvbuf = nouveau_dri2_buffer(front);
+
+	if (draw->type == DRAWABLE_PIXMAP)
+		pixmap = (PixmapPtr)draw;
+	else
+		pixmap = (*draw->pScreen->GetWindowPixmap)((WindowPtr)draw);
+
+	pixmap->refcnt++;
+
+	exaMoveInPixmap(pixmap);
+	r = nouveau_bo_handle_get(nouveau_pixmap_bo(pixmap), &front->name);
+	if (r) {
+		(*draw->pScreen->DestroyPixmap)(pixmap);
+		return FALSE;
+	}
+
+	(*draw->pScreen->DestroyPixmap)(nvbuf->ppix);
+	front->pitch = pixmap->devKind;
+	front->cpp = pixmap->drawable.bitsPerPixel / 8;
+	nvbuf->ppix = pixmap;
+
+	return TRUE;
+}
+
+static Bool
 can_exchange(DrawablePtr draw, PixmapPtr dst_pix, PixmapPtr src_pix)
 {
 	ScrnInfoPtr scrn = xf86Screens[draw->pScreen->myNum];
@@ -234,13 +263,14 @@ nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame,
 {
 	ScrnInfoPtr scrn = xf86Screens[draw->pScreen->myNum];
 	NVPtr pNv = NVPTR(scrn);
-	PixmapPtr dst_pix = nouveau_dri2_buffer(s->dst)->ppix;
+	PixmapPtr dst_pix;
 	PixmapPtr src_pix = nouveau_dri2_buffer(s->src)->ppix;
-	struct nouveau_bo *dst_bo = nouveau_pixmap_bo(dst_pix);
+	struct nouveau_bo *dst_bo;
 	struct nouveau_bo *src_bo = nouveau_pixmap_bo(src_pix);
 	struct nouveau_channel *chan = pNv->chan;
 	RegionRec reg;
 	int type, ret;
+	Bool front_updated;
 
 	REGION_INIT(0, &reg, (&(BoxRec){ 0, 0, draw->width, draw->height }), 0);
 	REGION_TRANSLATE(0, &reg, draw->x, draw->y);
@@ -257,6 +287,15 @@ nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame,
 	if (ref_crtc_hw_id & 1)
 		ref_crtc_hw_id = 1;
 
+	/* Update frontbuffer pixmap and name: Could have changed due to
+	 * window (un)redirection as part of compositing.
+	 */
+	front_updated = update_front(draw, s->dst);
+
+	/* Assign frontbuffer pixmap, after update in update_front() */
+	dst_pix = nouveau_dri2_buffer(s->dst)->ppix;
+	dst_bo = nouveau_pixmap_bo(dst_pix);
+
 	/* Throttle on the previous frame before swapping */
 	nouveau_bo_map(dst_bo, NOUVEAU_BO_RD);
 	nouveau_bo_unmap(dst_bo);
@@ -275,7 +314,7 @@ nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame,
 		FIRE_RING(chan);
 	}
 
-	if (can_exchange(draw, dst_pix, src_pix)) {
+	if (front_updated && can_exchange(draw, dst_pix, src_pix)) {
 		type = DRI2_EXCHANGE_COMPLETE;
 		DamageRegionAppend(draw, &reg);
 
-- 
1.7.5.4

[PATCH 5/9] dri2: Fixes to swap scheduling.

[Mario Kleiner]

Fix some small off-by-one errors and a mismatch
between 32 bit kernel interfaces for vblank count
and 64 bit dri2 interfaces for target_msc et al.

Return corrected target_msc to swap scheduling in
x-server.

A revised version of the patch discussed here:
http://lists.freedesktop.org/archives/nouveau/2011-September/009143.html

Signed-off-by: Mario Kleiner <mario.kleiner-TdbV1Z3I5XE0NhjG498hmQ@public.gmane.org>
---
 src/nouveau_dri2.c |   23 +++++++++++++++++++++--
 1 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/src/nouveau_dri2.c b/src/nouveau_dri2.c
index 8608678..719b3bb 100644
--- a/src/nouveau_dri2.c
+++ b/src/nouveau_dri2.c
@@ -387,11 +387,22 @@ nouveau_dri2_schedule_swap(ClientPtr client, DrawablePtr draw,
 		if (ret)
 			goto fail;
 
+		/* Truncate to match kernel interfaces; means occasional overflow
+		 * misses, but that's generally not a big deal.
+		 */
+		*target_msc &= 0xffffffff;
+		divisor &= 0xffffffff;
+		remainder &= 0xffffffff;
+
 		/* Calculate a swap target if we don't have one */
 		if (current_msc >= *target_msc && divisor)
 			*target_msc = current_msc + divisor
 				- (current_msc - remainder) % divisor;
 
+		/* Avoid underflow of unsigned value below */
+		if (*target_msc == 0)
+			*target_msc = 1;
+
 		/* Request a vblank event one frame before the target */
 		ret = nouveau_wait_vblank(draw, DRM_VBLANK_ABSOLUTE |
 					  DRM_VBLANK_EVENT,
@@ -399,7 +410,8 @@ nouveau_dri2_schedule_swap(ClientPtr client, DrawablePtr draw,
 					  &expect_msc, NULL, s);
 		if (ret)
 			goto fail;
-		s->frame = (unsigned int) expect_msc & 0xffffffff;
+		s->frame = 1 + ((unsigned int) expect_msc & 0xffffffff);
+		*target_msc = 1 + expect_msc;
 	} else {
 		/* We can't/don't want to sync to vblank, just swap. */
 		nouveau_dri2_finish_swap(draw, 0, 0, 0, s);
@@ -420,6 +432,13 @@ nouveau_dri2_schedule_wait(ClientPtr client, DrawablePtr draw,
 	CARD64 current_msc;
 	int ret;
 
+	/* Truncate to match kernel interfaces; means occasional overflow
+	 * misses, but that's generally not a big deal.
+	 */
+	target_msc &= 0xffffffff;
+	divisor &= 0xffffffff;
+	remainder &= 0xffffffff;
+
 	if (!can_sync_to_vblank(draw)) {
 		DRI2WaitMSCComplete(client, draw, target_msc, 0, 0);
 		return TRUE;
@@ -439,7 +458,7 @@ nouveau_dri2_schedule_wait(ClientPtr client, DrawablePtr draw,
 		goto fail;
 
 	/* Calculate a wait target if we don't have one */
-	if (current_msc > target_msc && divisor)
+	if (current_msc >= target_msc && divisor)
 		target_msc = current_msc + divisor
 			- (current_msc - remainder) % divisor;
 
-- 
1.7.5.4

[PATCH 6/9] dri2: Allow vblank controlled swaps for redirected windows. Part I

[Mario Kleiner]

Make sure that swaps for redirected windows under a
compositor are still scheduled via vblank events, to
avoid effects like 2900 fps swaps under a compositor.

See discussion with Francisco Jerez at:

http://lists.freedesktop.org/archives/nouveau/2011-September/009278.html
http://lists.freedesktop.org/archives/nouveau/2011-September/009292.html

This is part I of the agreed upon band-aid, in a separate patch.

It allows to use vblank related functions on redirected
windows and thereby fixes functions from sgi_sync_control
and oml_sync_control extension, e.g., glXWaitForMscOML(),
glXGetSyncValuesOML(), glXWaitVideoSyncSGI, ...

Signed-off-by: Mario Kleiner <mario.kleiner-TdbV1Z3I5XE0NhjG498hmQ@public.gmane.org>
---
 src/nouveau_dri2.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/src/nouveau_dri2.c b/src/nouveau_dri2.c
index 719b3bb..6a0800c 100644
--- a/src/nouveau_dri2.c
+++ b/src/nouveau_dri2.c
@@ -204,10 +204,8 @@ can_sync_to_vblank(DrawablePtr draw)
 {
 	ScrnInfoPtr scrn = xf86Screens[draw->pScreen->myNum];
 	NVPtr pNv = NVPTR(scrn);
-	PixmapPtr pix = NVGetDrawablePixmap(draw);
 
 	return pNv->glx_vblank &&
-		nouveau_exa_pixmap_is_onscreen(pix) &&
 		nv_window_belongs_to_crtc(scrn, draw->x, draw->y,
 					  draw->width, draw->height);
 }
-- 
1.7.5.4

[PATCH 7/9] dri2: Allow vblank controlled swaps for redirected windows. Part II

[Mario Kleiner]

This part implements proper throttling for clients. For
vblank synchronized blits, it defers DRI2SwapComplete()
until 1 vblank after the framebuffer blit is submitted to
the gpu.

Rationale:

For unredirected windows, this is the earliest time the
"blit swap" can complete, as blits are submitted one vblank
before the target vblank and synchronized with vblank in the
gpu. This makes swap completion timestamps at least reasonable.

For redirected windows, the compositor will probably pick
up the "blit swapped" frontbuffer pixmap of the window quickly,
but defer its own recomposition to the next vblank, at least
if sync to vblank for the compositor is on.

Signed-off-by: Mario Kleiner <mario.kleiner-TdbV1Z3I5XE0NhjG498hmQ@public.gmane.org>
---
 src/nouveau_dri2.c |   32 +++++++++++++++++++++++++++++---
 1 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/src/nouveau_dri2.c b/src/nouveau_dri2.c
index 6a0800c..fdc5148 100644
--- a/src/nouveau_dri2.c
+++ b/src/nouveau_dri2.c
@@ -135,6 +135,7 @@ nouveau_dri2_copy_region(DrawablePtr pDraw, RegionPtr pRegion,
 struct nouveau_dri2_vblank_state {
 	enum {
 		SWAP,
+		BLIT,
 		WAIT
 	} action;
 
@@ -342,6 +343,22 @@ nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame,
 
 		REGION_TRANSLATE(0, &reg, -draw->x, -draw->y);
 		nouveau_dri2_copy_region(draw, &reg, s->dst, s->src);
+
+		if (can_sync_to_vblank(draw)) {
+			/* Request a vblank event one vblank from now, the most
+			 * likely (optimistic?) time a direct framebuffer blit
+			 * will complete or a desktop compositor will update its
+			 * screen. This defers DRI2SwapComplete() to the earliest
+			 * likely time of real swap completion.
+			 */
+			s->action = BLIT;
+			ret = nouveau_wait_vblank(draw, DRM_VBLANK_EVENT |
+						  DRM_VBLANK_RELATIVE, 1,
+						  NULL, NULL, s);
+			/* Done, if success. Otherwise use fallback below. */
+			if (!ret)
+				return;
+		}
 	}
 
 	/*
@@ -351,8 +368,9 @@ nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame,
 	 *
 	 * XXX - The DRI2SwapLimit() API allowed us to move this to
 	 *	 the flip handler with no FPS hit for page flipped swaps.
-	 *       It is still needed for copy swaps as we lack a method
-	 *       to detect true swap completion for DRI2_BLIT_COMPLETE.
+	 *       It is still needed as a fallback for some copy swaps as
+	 *       we lack a method to detect true swap completion for
+	 *       DRI2_BLIT_COMPLETE.
 	 */
 	DRI2SwapComplete(s->client, draw, frame, tv_sec, tv_usec,
 			 type, s->func, s->data);
@@ -505,8 +523,10 @@ nouveau_dri2_vblank_handler(int fd, unsigned int frame,
 
 	ret = dixLookupDrawable(&draw, s->draw, serverClient,
 				M_ANY, DixWriteAccess);
-	if (ret)
+	if (ret) {
+		free(s);
 		return;
+	}
 
 	switch (s->action) {
 	case SWAP:
@@ -517,6 +537,12 @@ nouveau_dri2_vblank_handler(int fd, unsigned int frame,
 		DRI2WaitMSCComplete(s->client, draw, frame, tv_sec, tv_usec);
 		free(s);
 		break;
+
+	case BLIT:
+		DRI2SwapComplete(s->client, draw, frame, tv_sec, tv_usec,
+				 DRI2_BLIT_COMPLETE, s->func, s->data);
+		free(s);
+		break;
 	}
 }
 
-- 
1.7.5.4

[PATCH 8/9] dri2: Reimplement hack for triple-buffering on old X-Servers.

[Mario Kleiner]

X-Servers before 1.12.0 don't have the DRI2SwapLimit()
API. On these, default to a swaplimit of 1 - double-buffering.

This patch implements support for swap limit of 2,
triple-buffering, on old x-servers via Francisco Jerez
previous hack:

Return DRI2SwapComplete() before the swap has completed,
so clients don't get blocked on the pending swap. This
allows for a "triple-buffering look-alike" behaviour, but
breaks the swap scheduling and timestamping defined
in the OML_sync_control spec, so applications which
rely on conformant behaviour will break with a swap
limit of 2 on pre 1.12.0 x-servers.

Signed-off-by: Mario Kleiner <mario.kleiner-TdbV1Z3I5XE0NhjG498hmQ@public.gmane.org>
---
 man/nouveau.man    |    6 +++++-
 src/nouveau_dri2.c |   32 +++++++++++++++++++++++++++++---
 src/nv_driver.c    |   11 ++++++-----
 3 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/man/nouveau.man b/man/nouveau.man
index 59f6c1a..7c72907 100644
--- a/man/nouveau.man
+++ b/man/nouveau.man
@@ -101,7 +101,11 @@ a drawable before a client is blocked.
 A value of 1 corresponds to double-buffering. A value of 2 corresponds
 to triple-buffering. Higher values may allow higher framerate, but also
 increase lag for interactive applications, e.g., games. Nouveau currently
-supports a maximum value of 2 on XOrg 1.12+ and a maximum of 1 on older servers.
+reliably supports a maximum value of 2 on XOrg 1.12+. A maximum setting of 2
+on older x-servers is allowed, but it will break conformance with the
+OpenML OML_sync_control specification and will cause failure of software
+that relies on correct presentation timing behaviour as defined in that
+specification.
 .br
 Default: 2 for XOrg 1.12+, 1 for older servers.
 .SH "SEE ALSO"
diff --git a/src/nouveau_dri2.c b/src/nouveau_dri2.c
index fdc5148..f0c7fec 100644
--- a/src/nouveau_dri2.c
+++ b/src/nouveau_dri2.c
@@ -255,6 +255,18 @@ nouveau_dri2_swap_limit_validate(DrawablePtr draw, int swap_limit)
 }
 #endif
 
+/* Shall we intentionally violate the OML_sync_control spec to
+ * get some sort of triple-buffering behaviour on a pre 1.12.0
+ * x-server?
+ */
+static Bool violate_oml(DrawablePtr draw)
+{
+	ScrnInfoPtr scrn = xf86Screens[draw->pScreen->myNum];
+	NVPtr pNv = NVPTR(scrn);
+
+	return (DRI2INFOREC_VERSION < 6) && (pNv->swap_limit > 1);
+}
+
 static void
 nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame,
 			 unsigned int tv_sec, unsigned int tv_usec,
@@ -319,7 +331,9 @@ nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame,
 
 		if (DRI2CanFlip(draw)) {
 			type = DRI2_FLIP_COMPLETE;
-			ret = drmmode_page_flip(draw, src_pix, s, ref_crtc_hw_id);
+			ret = drmmode_page_flip(draw, src_pix,
+						violate_oml(draw) ? NULL : s,
+						ref_crtc_hw_id);
 			if (!ret)
 				goto out;
 		}
@@ -330,7 +344,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)
+		if ((type == DRI2_FLIP_COMPLETE) && !violate_oml(draw))
 			return;
 	} else {
 		type = DRI2_BLIT_COMPLETE;
@@ -344,7 +358,7 @@ nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame,
 		REGION_TRANSLATE(0, &reg, -draw->x, -draw->y);
 		nouveau_dri2_copy_region(draw, &reg, s->dst, s->src);
 
-		if (can_sync_to_vblank(draw)) {
+		if (can_sync_to_vblank(draw) && !violate_oml(draw)) {
 			/* Request a vblank event one vblank from now, the most
 			 * likely (optimistic?) time a direct framebuffer blit
 			 * will complete or a desktop compositor will update its
@@ -361,6 +375,14 @@ nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame,
 		}
 	}
 
+	/* Special triple-buffering hack for old pre 1.12.0 x-servers used? */
+	if (violate_oml(draw)) {
+		/* Signal to client that swap completion timestamps and counts
+		 * are invalid - they violate the specification.
+		 */
+		frame = tv_sec = tv_usec = 0;
+	}
+
 	/*
 	 * Tell the X server buffers are already swapped even if they're
 	 * not, to prevent it from blocking the client on the next
@@ -371,6 +393,10 @@ nouveau_dri2_finish_swap(DrawablePtr draw, unsigned int frame,
 	 *       It is still needed as a fallback for some copy swaps as
 	 *       we lack a method to detect true swap completion for
 	 *       DRI2_BLIT_COMPLETE.
+	 *
+	 *       It is also used if triple-buffering is requested on
+	 *       old x-servers which don't support the DRI2SwapLimit()
+	 *       function.
 	 */
 	DRI2SwapComplete(s->client, draw, frame, tv_sec, tv_usec,
 			 type, s->func, s->data);
diff --git a/src/nv_driver.c b/src/nv_driver.c
index 5def531..7512464 100644
--- a/src/nv_driver.c
+++ b/src/nv_driver.c
@@ -863,11 +863,12 @@ NVPreInit(ScrnInfoPtr pScrn, int flags)
 		reason = "";
 		from = X_CONFIG;
 
-		if (DRI2INFOREC_VERSION < 6) {
-			/* No swap limit api in server. Stick to server default of 1. */
-			pNv->swap_limit = 1;
-			from = X_DEFAULT;
-			reason = ": This X-Server only supports a swap limit of 1.";
+		if ((DRI2INFOREC_VERSION < 6) && (pNv->swap_limit > 1)) {
+			/* No swap limit api in server. A value > 1 requires use
+			 * of problematic hacks.
+			 */
+			from = X_WARNING;
+			reason = ": Caution: Use of this swap limit > 1 violates OML_sync_control spec on this X-Server!\n";
 		}
 	} else {
 		/* Driver default: Double buffering on old servers, triple-buffering
-- 
1.7.5.4

[PATCH 9/9] dri2: Fix corner case crash for swaplimit > 1

[Mario Kleiner]

If a swaplimit > 1 is set on a server which
supports the swaplimit api (XOrg 1.12.0+),
the following can happen:

1. Client calls glXSwapBuffersMscOML() with a
   swap target > 1 vblank in the future, or a
   client calls glXSwapbuffers() while the swap
   interval is set to > 1 (unusual but possible).

2. nouveau_dri2_finish_swap() is therefore called
   only at the target vblank, instead of immediately.

3. Because of the deferred execution of
   nouveu_dri2_finish_swap(), the OpenGL client
   can call x-servers DRI2GetBuffersWithFormat()
   before nouveau_dri2_finish_swap() executes and
   it deletes pixmaps that would be needed by
   nouveau_dri2_finish_swap() --> Segfault --> Crash.

Prevent this: When a swap is scheduled into the
future, we temporarily reduce the swaplimit to 1
until nouveau_dri2_finish_swap() is done, then
restore it to its original value. This throttles
the client inside the server in DRI2ThrottleClient()
before it can call the evil DRI2GetbuffersWithFormat().

The client will still be released one video refresh
interval before swap completion, so there is still
some potential win.

This doesn't affect the common case of swapping at
the next vblank, where this throttling is not needed
or done.

Signed-off-by: Mario Kleiner <mario.kleiner-TdbV1Z3I5XE0NhjG498hmQ@public.gmane.org>
---
 src/nouveau_dri2.c |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/src/nouveau_dri2.c b/src/nouveau_dri2.c
index f0c7fec..7878a5a 100644
--- a/src/nouveau_dri2.c
+++ b/src/nouveau_dri2.c
@@ -445,6 +445,26 @@ nouveau_dri2_schedule_swap(ClientPtr client, DrawablePtr draw,
 		if (*target_msc == 0)
 			*target_msc = 1;
 
+#if DRI2INFOREC_VERSION >= 6
+		/* Is this a swap in the future, ie. the vblank event will
+		 * not be immediately dispatched, but only at a future vblank?
+		 * If so, we need to temporarily lower the swaplimit to 1, so
+		 * that DRI2GetBuffersWithFormat() requests from the client get
+		 * deferred in the x-server until the vblank event has been
+		 * dispatched to us and nouveau_dri2_finish_swap() is done. If
+		 * we wouldn't do this, DRI2GetBuffersWithFormat() would operate
+		 * on wrong (pre-swap) buffers, and cause a segfault later on in
+		 * nouveau_dri2_finish_swap(). Our vblank event handler restores
+		 * the old swaplimit immediately after nouveau_dri2_finish_swap()
+		 * is done, so we still get 1 video refresh cycle worth of
+		 * triple-buffering. For a swap at next vblank, dispatch of the
+		 * vblank event happens immediately, so there isn't any need
+		 * for this lowered swaplimit.
+		 */
+		if (current_msc < *target_msc - 1)
+			DRI2SwapLimit(draw, 1);
+#endif
+
 		/* Request a vblank event one frame before the target */
 		ret = nouveau_wait_vblank(draw, DRM_VBLANK_ABSOLUTE |
 					  DRM_VBLANK_EVENT,
@@ -557,6 +577,12 @@ nouveau_dri2_vblank_handler(int fd, unsigned int frame,
 	switch (s->action) {
 	case SWAP:
 		nouveau_dri2_finish_swap(draw, frame, tv_sec, tv_usec, s);
+#if DRI2INFOREC_VERSION >= 6
+		/* Restore real swap limit on drawable, now that it is safe. */
+		ScrnInfoPtr scrn = xf86Screens[draw->pScreen->myNum];
+		DRI2SwapLimit(draw, NVPTR(scrn)->swap_limit);
+#endif
+
 		break;
 
 	case WAIT:
-- 
1.7.5.4

Re: [PATCH 9/9] dri2: Fix corner case crash for swaplimit > 1

[Michel Dänzer]

On Don, 2012-02-16 at 00:45 +0100, Mario Kleiner wrote: 
> If a swaplimit > 1 is set on a server which
> supports the swaplimit api (XOrg 1.12.0+),
> the following can happen:
> 
> 1. Client calls glXSwapBuffersMscOML() with a
>    swap target > 1 vblank in the future, or a
>    client calls glXSwapbuffers() while the swap
>    interval is set to > 1 (unusual but possible).
> 
> 2. nouveau_dri2_finish_swap() is therefore called
>    only at the target vblank, instead of immediately.
> 
> 3. Because of the deferred execution of
>    nouveu_dri2_finish_swap(), the OpenGL client
>    can call x-servers DRI2GetBuffersWithFormat()
>    before nouveau_dri2_finish_swap() executes and
>    it deletes pixmaps that would be needed by
>    nouveau_dri2_finish_swap() --> Segfault --> Crash.

Pixmaps are reference counted, so it should be possible to fix this via
proper reference counting.


-- 
Earthling Michel Dänzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 1/9] dri2: Fix can_exchange() to allow page-flipping on new servers.

[Michel Dänzer]

On Don, 2012-02-16 at 00:45 +0100, Mario Kleiner wrote: 
> can_exchange() fails on at least Xorg 1.12+. This fixes
> it in the same way it was fixed in the ati & intel ddx.
> 
> Signed-off-by: Mario Kleiner <mario.kleiner@tuebingen.mpg.de>
> ---
>  src/nouveau_dri2.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/src/nouveau_dri2.c b/src/nouveau_dri2.c
> index 3aa5ec5..5b62425 100644
> --- a/src/nouveau_dri2.c
> +++ b/src/nouveau_dri2.c
> @@ -160,7 +160,7 @@ 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.depth == src_pix->drawable.depth &&
> +		dst_pix->drawable.bitsPerPixel == src_pix->drawable.bitsPerPixel &&
>  		dst_pix->devKind == src_pix->devKind;
>  }
>  

Actually, it seems like the pixmap depths really should match, otherwise
one could end up with the front pixmap depth not matching the window
depth. Not sure that's a real problem right now, but it seems wonky at
least...

Have you investigated why the depths don't match?


-- 
Earthling Michel Dänzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer
_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH 1/9] dri2: Fix can_exchange() to allow page-flipping on new servers.

[Mario Kleiner]

On 02/16/2012 11:04 AM, Michel Dänzer wrote:
> On Don, 2012-02-16 at 00:45 +0100, Mario Kleiner wrote:
>> can_exchange() fails on at least Xorg 1.12+. This fixes
>> it in the same way it was fixed in the ati&  intel ddx.
>>
>> Signed-off-by: Mario Kleiner<mario.kleiner@tuebingen.mpg.de>
>> ---
>>   src/nouveau_dri2.c |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/src/nouveau_dri2.c b/src/nouveau_dri2.c
>> index 3aa5ec5..5b62425 100644
>> --- a/src/nouveau_dri2.c
>> +++ b/src/nouveau_dri2.c
>> @@ -160,7 +160,7 @@ 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.depth == src_pix->drawable.depth&&
>> +		dst_pix->drawable.bitsPerPixel == src_pix->drawable.bitsPerPixel&&
>>   		dst_pix->devKind == src_pix->devKind;
>>   }
>>
>
> Actually, it seems like the pixmap depths really should match, otherwise
> one could end up with the front pixmap depth not matching the window
> depth. Not sure that's a real problem right now, but it seems wonky at
> least...
>
> Have you investigated why the depths don't match?
>
>

Depends on the meaning of "investigated": One of the pixmaps has depth 
24 bits (the pixmap of the root window) the other 32 bits (as requested 
from the client via DRI2GetBuffersWithFormat for RGBA8 visuals). Both 
have 32 bpp. I checked what the intel and ati ddx do. The ati ddx always 
checked for matching drawable.bitsPerPixel since kms pageflip support 
was implemented. The intel ddx does the same, but the code and comments 
suggests they tried both and checking for matching depths probably 
didn't work:

cd xorg/drivers/xf86-video-intel/
git log -p e2615cdeef078dbd2e834b68c437f098a92b941d

So everybody does it like this currently, and it seems to work.

I also just tried setting DefaultDepth to 30 bits on a NV50 card which 
supports this. xdpyinfo confirms a screen depth of 30 bit == 10 bits per 
red/green/blue channel. glxinfo shows that only RGBA8 visuals are 
supported. Here we have a mismatch of depths 30 vs. 24 and the driver 
reverts correctly from page flipping to copy swaps, although i can't see 
immediately what causes it to correctly switch back to copy swaps.

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

Re: [PATCH 9/9] dri2: Fix corner case crash for swaplimit > 1

[Mario Kleiner]

On 02/16/2012 10:46 AM, Michel Dänzer wrote:
> On Don, 2012-02-16 at 00:45 +0100, Mario Kleiner wrote:
>> If a swaplimit>  1 is set on a server which
>> supports the swaplimit api (XOrg 1.12.0+),
>> the following can happen:
>>
>> 1. Client calls glXSwapBuffersMscOML() with a
>>     swap target>  1 vblank in the future, or a
>>     client calls glXSwapbuffers() while the swap
>>     interval is set to>  1 (unusual but possible).
>>
>> 2. nouveau_dri2_finish_swap() is therefore called
>>     only at the target vblank, instead of immediately.
>>
>> 3. Because of the deferred execution of
>>     nouveu_dri2_finish_swap(), the OpenGL client
>>     can call x-servers DRI2GetBuffersWithFormat()
>>     before nouveau_dri2_finish_swap() executes and
>>     it deletes pixmaps that would be needed by
>>     nouveau_dri2_finish_swap() -->  Segfault -->  Crash.
>
> Pixmaps are reference counted, so it should be possible to fix this via
> proper reference counting.
>
>

Thanks for the tip. I'll try that.

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

Re: [PATCH 1/9] dri2: Fix can_exchange() to allow page-flipping on new servers.

[Michel Dänzer]

On Mon, 2012-02-20 at 05:59 +0100, Mario Kleiner wrote: 
> On 02/16/2012 11:04 AM, Michel Dänzer wrote:
> > On Don, 2012-02-16 at 00:45 +0100, Mario Kleiner wrote:
> >> can_exchange() fails on at least Xorg 1.12+. This fixes
> >> it in the same way it was fixed in the ati&  intel ddx.
> >>
> >> Signed-off-by: Mario Kleiner<mario.kleiner@tuebingen.mpg.de>
> >> ---
> >>   src/nouveau_dri2.c |    2 +-
> >>   1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/src/nouveau_dri2.c b/src/nouveau_dri2.c
> >> index 3aa5ec5..5b62425 100644
> >> --- a/src/nouveau_dri2.c
> >> +++ b/src/nouveau_dri2.c
> >> @@ -160,7 +160,7 @@ 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.depth == src_pix->drawable.depth&&
> >> +		dst_pix->drawable.bitsPerPixel == src_pix->drawable.bitsPerPixel&&
> >>   		dst_pix->devKind == src_pix->devKind;
> >>   }
> >>
> >
> > Actually, it seems like the pixmap depths really should match, otherwise
> > one could end up with the front pixmap depth not matching the window
> > depth. Not sure that's a real problem right now, but it seems wonky at
> > least...
> >
> > Have you investigated why the depths don't match?
> 
> Depends on the meaning of "investigated": One of the pixmaps has depth 
> 24 bits (the pixmap of the root window) the other 32 bits (as requested 
> from the client via DRI2GetBuffersWithFormat for RGBA8 visuals). Both 
> have 32 bpp. I checked what the intel and ati ddx do. The ati ddx always 
> checked for matching drawable.bitsPerPixel since kms pageflip support 
> was implemented. The intel ddx does the same, but the code and comments 
> suggests they tried both and checking for matching depths probably 
> didn't work:
> 
> cd xorg/drivers/xf86-video-intel/
> git log -p e2615cdeef078dbd2e834b68c437f098a92b941d
> 
> So everybody does it like this currently, and it seems to work.

Right. I must have been incorrectly thinking of flipping as changing the
window pixmap.

I still have some doubts — e.g. why is an RGBA visual chosen for a
fullscreen window, and does this really have anything to do with changes
in xserver 1.12, or rather in Mesa — but I think the change itself is
okay.


-- 
Earthling Michel Dänzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer
_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH 1/9] dri2: Fix can_exchange() to allow page-flipping on new servers.

[Mario Kleiner]

On 02/20/2012 11:27 AM, Michel Dänzer wrote:
> On Mon, 2012-02-20 at 05:59 +0100, Mario Kleiner wrote:
>> On 02/16/2012 11:04 AM, Michel Dänzer wrote:
>>> On Don, 2012-02-16 at 00:45 +0100, Mario Kleiner wrote:
>>>> can_exchange() fails on at least Xorg 1.12+. This fixes
>>>> it in the same way it was fixed in the ati&   intel ddx.
>>>>
>>>> Signed-off-by: Mario Kleiner<mario.kleiner@tuebingen.mpg.de>
>>>> ---
>>>>    src/nouveau_dri2.c |    2 +-
>>>>    1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/src/nouveau_dri2.c b/src/nouveau_dri2.c
>>>> index 3aa5ec5..5b62425 100644
>>>> --- a/src/nouveau_dri2.c
>>>> +++ b/src/nouveau_dri2.c
>>>> @@ -160,7 +160,7 @@ 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.depth == src_pix->drawable.depth&&
>>>> +		dst_pix->drawable.bitsPerPixel == src_pix->drawable.bitsPerPixel&&
>>>>    		dst_pix->devKind == src_pix->devKind;
>>>>    }
>>>>
>>>
>>> Actually, it seems like the pixmap depths really should match, otherwise
>>> one could end up with the front pixmap depth not matching the window
>>> depth. Not sure that's a real problem right now, but it seems wonky at
>>> least...
>>>
>>> Have you investigated why the depths don't match?
>>
>> Depends on the meaning of "investigated": One of the pixmaps has depth
>> 24 bits (the pixmap of the root window) the other 32 bits (as requested
>> from the client via DRI2GetBuffersWithFormat for RGBA8 visuals). Both
>> have 32 bpp. I checked what the intel and ati ddx do. The ati ddx always
>> checked for matching drawable.bitsPerPixel since kms pageflip support
>> was implemented. The intel ddx does the same, but the code and comments
>> suggests they tried both and checking for matching depths probably
>> didn't work:
>>
>> cd xorg/drivers/xf86-video-intel/
>> git log -p e2615cdeef078dbd2e834b68c437f098a92b941d
>>
>> So everybody does it like this currently, and it seems to work.
>
> Right. I must have been incorrectly thinking of flipping as changing the
> window pixmap.
>
> I still have some doubts — e.g. why is an RGBA visual chosen for a
> fullscreen window, and does this really have anything to do with changes
> in xserver 1.12, or rather in Mesa — but I think the change itself is
> okay.
>

You are right, it is not the xserver, but more likely Mesa. I tried the 
patch again with some debug output against xserver 1.10.4 (Ubuntu 11.10 
standard x-server) and the same happens. The FRONT_LEFT buffer is taken 
from the windows pixmap, which is depth 24, the BACK_LEFT buffer is 
allocated as a pixmap of 32 bits. So now i'm surprised it ever worked 
before...

Original testing of this nouveau patchset was around August 2011, since 
then i've also updated mesa to 7.11 as part of a distribution upgrade 
from Ubuntu 11.04 -> 11.10.

Mesa requests 32 bit buffers as part of the BACK_LEFT attachment in 
DRI2GetBuffersWithFormat(). As far as i could trace it back, the code in 
src/gallium/state_trackers/dri/drm/dri2.c, dri2_drawable_get_buffers() 
mapped the PIPE_FORMAT_B8G8R8A8_UNORM (as requested by my toolkit) and 
also PIPE_FORMAT_B8G8R8X8_UNORM (from glxgears) to a DRI2 buffer format 
of 32 bits and it seems that that is used as .depth for the pixmaps 
drawable as well.

There's this related commit in mesa:

git log -p 576161289df68eedade591fbca4013329c9e5ded

which changed the logic a bit after Mesa 7.12. Also this one:

git log -p c72d7df16879e3210946ba92a7edc823815b6f16

So to support page flipping on different mesa versions i guess 
can_exchange needs to be lenient and check for the matching bitsPerPixel 
instead.

I will change the commit message and title of this patch to be less 
misleading about the cause of the problem.

I wonder if the depth field is still a good way to describe the buffer 
format, especially with things like depth 30 RGB10 scanout.

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

Re: [Patches][nouveau/ddx]: Improvements to bufferswap implementation and timestamping

[Ben Skeggs]

On Thu, 2012-02-16 at 00:45 +0100, Mario Kleiner wrote:
> Hi,
Hey Mario,

What's your plan with this patchset?  Do you intend on taking Michel's
comments into account?

CC'ing Francisco as he had some comments on IRC.  I'd like to get this
all sorted and merged so I can rebase the ported-to-new-libdrm ddx on
top of it all and get that ready.

Ben.

> 
> here a set of patches against the nouveau-ddx. This is an extended and
> revised set, based on Francisco Jerez feedback from autumn last year.
> 
> [1/9] Makes pageflipping work again on X-Server 1.12rc. It apparently stopped
> working somewhere around Xorg 1.11+.
> 
> [2/9] Implements handling of pageflip completion events from the kernel.
> Francisco Jerez argument against including it was that the x-server didn't
> have a swaplimit api, so this couldn't be applied at the same time as the
> pseudo triple-buffering hack which is in place in the current ddx.
> Now we have the swaplimit api in 1.12, so this problem should be solved.
> 
> [3/9] Makes use of the swaplimit api. A new xorg.conf option "SwapLimit"
> allows to select a swaplimit of 1 or 2 for double-buffering or
> triple-buffering. It defaults to 2 for Xorg 1.12+ and 1 for older servers.
> This way, on 1.12+ nouveau retains the kind of triple buffering behaviour it
> currently has, but swap completion timestamping (OML_sync_control,
> INTEL_swap_events, and client swap throttling) works conforming to
> the specs. On older servers it removes triple-buffering but makes
> nouveau conform to the specs. This is important for apps that need
> precise and reliable swap scheduling and timestamping.
> 
> [4/9] A bug fix against corrupted desktop when switching from redirected
> to non-redirected fullscreen windows under a desktop compositor.
> Fixes FDO bug #35452.
> 
> [5/9] Some fixes to swap scheduling, revised according to Francisco's
> review.
> 
> [6/9] Fixes swap throttling for non-fullscreen windows under a desktop
> compositor. Split into a separate patch according to Francisco's
> feedback.
> 
> [7/9] An attempt to provide more sane swap completion events for non-
> fullscreen windows. This is a bit of cheating, as it delivers the
> events for the earliest point in time one would expect the swap
> to complete, assuming only a lightly loaded gpu. Real completion
> could be later. I think this is an "improvement" because the current
> implementation delivers swap complete events with timestamps and
> counts that signal swap completion before the client even requested
> the swap.
> 
> [8/9] This one adds Francisco's original triple-buffering hack back
> for Xorg 1.11 and earlier servers if the xorg.conf option requests
> a swaplimit > 1. Users can choose between "triple-buffering" but
> broken timestamping or double-buffering with sane timestamping.
> 
> [9/9] Fixes a corner case which could cause the ddx to segfault
> with its current triple-buffering implementation.
> 
> I've tested these on single-display and dual-display setups,
> with/without compositor, with/without redirection and checked
> the robustness and precision of the timestamps with special
> measurement equipment, on XOrg 1.12-rc2 and 1.10. They work
> pretty well for me and finally make nouveau very useable for
> the kind of scientific applications that require precise
> swap scheduling and timestamping, so i'd love to see them
> reviewed and hopefully included into the ddx soon.
> 
> A couple of things are a bit of hacks:
> 
> [3/9] I think the setup of a default swap limit would be
> better done in the x-server itself instead of the ddx. The
> setup code is a bit awkward, hijacking the ddx->CreateBuffer
> function to apply the swaplimit. A DRI2GetSwapLimit() function
> is also missing from the server, which could help in the
> future to track swaplimit changes by other clients than the ddx
> itself. Unfortunately it is too late for the 1.12 release to do
> this.
> 
> [8/9] Don't know if this is still wanted/needed or not?
> 
> [9/9] It fixes the problem and doesn't affect performance, but is
> somewhat of a hack. I don't know how to do better, maybe somebody
> else has a better solution?
> 
> Thanks,
> -mario
> 
> _______________________________________________
> Nouveau mailing list
> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Patches][nouveau/ddx]: Improvements to bufferswap implementation and timestamping

[Mario Kleiner]

On 02/29/2012 08:17 AM, Ben Skeggs wrote:
> On Thu, 2012-02-16 at 00:45 +0100, Mario Kleiner wrote:
>> Hi,
> Hey Mario,
>
> What's your plan with this patchset?  Do you intend on taking Michel's
> comments into account?
>
> CC'ing Francisco as he had some comments on IRC.  I'd like to get this
> all sorted and merged so I can rebase the ported-to-new-libdrm ddx on
> top of it all and get that ready.
>

Hi Ben,

just sent out two updated patches wrt. to Michels feedback.

[1/10] Replaces the original [1/9] patch. The code is the same, but the 
commit message is changed to be less misleading about the cause of the 
problem. See the e-mail i think i sent about a week ago.

[10/10] Just for demonstration what i tried, not for application. The 
old [9/9] patch is still the best i can do at the moment. Following
Michels proposal, with 10/10 i added refcounting to nouveau_dri2_buffer 
objects and keep a reference while the swap is still pending. This 
prevents deletion of the buffer by the x-server and thereby prevents the 
server crash.

Problem: Now we have a display that doesn't crash, but flickers horribly 
while pageflipping with deferred swaps. I think it is because we still 
only have two buffers for double-buffering but pretend we're doing 
triple-buffering, but i could be totally wrong about the cause of the 
problem. The original patch [9/9] throttles the client in the special 
case where the pseudo-triplebuffering would result in flicker or crash, 
so it fixes the problem although in a rather hackish way.

Not sure if this can be done cleanly without a real n-buffering 
implementation in the x-server?

I think other than that there were no further comments and Francisco's 
original feedback from last year is already implemented in the patchset.

More feedback etc. welcome, but i usually can only work on this part 
time on weekends if it needs further improvements before merging.

Can the kms kernel patches be already merged? I think they are in a good 
shape now.

Thanks,
-mario


> Ben.
>
>>
>> here a set of patches against the nouveau-ddx. This is an extended and
>> revised set, based on Francisco Jerez feedback from autumn last year.
>>
>> [1/9] Makes pageflipping work again on X-Server 1.12rc. It apparently stopped
>> working somewhere around Xorg 1.11+.
>>
>> [2/9] Implements handling of pageflip completion events from the kernel.
>> Francisco Jerez argument against including it was that the x-server didn't
>> have a swaplimit api, so this couldn't be applied at the same time as the
>> pseudo triple-buffering hack which is in place in the current ddx.
>> Now we have the swaplimit api in 1.12, so this problem should be solved.
>>
>> [3/9] Makes use of the swaplimit api. A new xorg.conf option "SwapLimit"
>> allows to select a swaplimit of 1 or 2 for double-buffering or
>> triple-buffering. It defaults to 2 for Xorg 1.12+ and 1 for older servers.
>> This way, on 1.12+ nouveau retains the kind of triple buffering behaviour it
>> currently has, but swap completion timestamping (OML_sync_control,
>> INTEL_swap_events, and client swap throttling) works conforming to
>> the specs. On older servers it removes triple-buffering but makes
>> nouveau conform to the specs. This is important for apps that need
>> precise and reliable swap scheduling and timestamping.
>>
>> [4/9] A bug fix against corrupted desktop when switching from redirected
>> to non-redirected fullscreen windows under a desktop compositor.
>> Fixes FDO bug #35452.
>>
>> [5/9] Some fixes to swap scheduling, revised according to Francisco's
>> review.
>>
>> [6/9] Fixes swap throttling for non-fullscreen windows under a desktop
>> compositor. Split into a separate patch according to Francisco's
>> feedback.
>>
>> [7/9] An attempt to provide more sane swap completion events for non-
>> fullscreen windows. This is a bit of cheating, as it delivers the
>> events for the earliest point in time one would expect the swap
>> to complete, assuming only a lightly loaded gpu. Real completion
>> could be later. I think this is an "improvement" because the current
>> implementation delivers swap complete events with timestamps and
>> counts that signal swap completion before the client even requested
>> the swap.
>>
>> [8/9] This one adds Francisco's original triple-buffering hack back
>> for Xorg 1.11 and earlier servers if the xorg.conf option requests
>> a swaplimit>  1. Users can choose between "triple-buffering" but
>> broken timestamping or double-buffering with sane timestamping.
>>
>> [9/9] Fixes a corner case which could cause the ddx to segfault
>> with its current triple-buffering implementation.
>>
>> I've tested these on single-display and dual-display setups,
>> with/without compositor, with/without redirection and checked
>> the robustness and precision of the timestamps with special
>> measurement equipment, on XOrg 1.12-rc2 and 1.10. They work
>> pretty well for me and finally make nouveau very useable for
>> the kind of scientific applications that require precise
>> swap scheduling and timestamping, so i'd love to see them
>> reviewed and hopefully included into the ddx soon.
>>
>> A couple of things are a bit of hacks:
>>
>> [3/9] I think the setup of a default swap limit would be
>> better done in the x-server itself instead of the ddx. The
>> setup code is a bit awkward, hijacking the ddx->CreateBuffer
>> function to apply the swaplimit. A DRI2GetSwapLimit() function
>> is also missing from the server, which could help in the
>> future to track swaplimit changes by other clients than the ddx
>> itself. Unfortunately it is too late for the 1.12 release to do
>> this.
>>
>> [8/9] Don't know if this is still wanted/needed or not?
>>
>> [9/9] It fixes the problem and doesn't affect performance, but is
>> somewhat of a hack. I don't know how to do better, maybe somebody
>> else has a better solution?
>>
>> Thanks,
>> -mario
>>
>> _______________________________________________
>> Nouveau mailing list
>> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>> http://lists.freedesktop.org/mailman/listinfo/nouveau
>
>

Re: [PATCH 9/9] dri2: Fix corner case crash for swaplimit > 1

[Anssi Hannula]

Hi!

On 16.02.2012 02:45, Mario Kleiner wrote:
> If a swaplimit > 1 is set on a server which
> supports the swaplimit api (XOrg 1.12.0+),
> the following can happen:
>
> 1. Client calls glXSwapBuffersMscOML() with a
>    swap target > 1 vblank in the future, or a
>    client calls glXSwapbuffers() while the swap
>    interval is set to > 1 (unusual but possible).
>
> 2. nouveau_dri2_finish_swap() is therefore called
>    only at the target vblank, instead of immediately.
>
> 3. Because of the deferred execution of
>    nouveu_dri2_finish_swap(), the OpenGL client
>    can call x-servers DRI2GetBuffersWithFormat()
>    before nouveau_dri2_finish_swap() executes and
>    it deletes pixmaps that would be needed by
>    nouveau_dri2_finish_swap() --> Segfault --> Crash.
>
> Prevent this: When a swap is scheduled into the
> future, we temporarily reduce the swaplimit to 1
> until nouveau_dri2_finish_swap() is done, then
> restore it to its original value. This throttles
> the client inside the server in DRI2ThrottleClient()
> before it can call the evil DRI2GetbuffersWithFormat().
>
> The client will still be released one video refresh
> interval before swap completion, so there is still
> some potential win.
>
> This doesn't affect the common case of swapping at
> the next vblank, where this throttling is not needed
> or done.


After upgrading my system to X server 1.12.3, I've started to
experience some crashes/freezes related to apparent memory
corruption.
Debugging with valgrind and gdb, it seems to me like the "evil"
DRI2GetbuffersWithFormat() can be called before swap even when
target_msc == current_msc + 1. This results in writes+reads to
freed memory when the vblank event is finally being handled.

I'm not an Xorg/drm expert, but with a glance at the code I
didn't see anything that would/should prevent
DRI2GetbuffersWithFormat() from being called before the vblank
event is handled. Even if the event is immediately sent by the
kernel, isn't it possible that the X server services some other
requests before the event is dispatched?

Am I missing something, or is that a bug?

In any case, I'm running ddx 1.0.1, xserver 1.12.3, libdrm 2.4.37,
kernel 3.4.4, and I have Option "GLXVBlank" "true".

Here's one of the invalid write errors:

==24537== Invalid write of size 4
==24537==    at 0x8D45029: nouveau_bo_name_get (nouveau.c:426)
==24537==    by 0x8B1EFFD: nouveau_dri2_finish_swap 
(nouveau_dri2.c:173)
==24537==    by 0x8B1F65F: nouveau_dri2_vblank_handler 
(nouveau_dri2.c:598)
==24537==    by 0x8707BA0: drmHandleEvent (xf86drmMode.c:808)
==24537==    by 0x8B37BC5: drmmode_wakeup_handler 
(drmmode_display.c:1447)
==24537==    by 0x43EA15: WakeupHandler (dixutils.c:421)
==24537==    by 0x5D7429: WaitForSomething (WaitFor.c:224)
==24537==    by 0x430B8B: Dispatch (dispatch.c:357)
==24537==    by 0x421D6C: main (main.c:288)
==24537==  Address 0xdd7c6d4 is 4 bytes inside a block of size 40 
free'd
==24537==    at 0x4C25A9E: free (in 
/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==24537==    by 0x890E5A4: update_dri2_drawable_buffers (dri2.c:415)
==24537==    by 0x890E97D: do_get_buffers (dri2.c:525)
==24537==    by 0x890EB60: DRI2GetBuffersWithFormat (dri2.c:582)
==24537==    by 0x8910A7E: ProcDRI2GetBuffersWithFormat (dri2ext.c:299)
==24537==    by 0x8911256: ProcDRI2Dispatch (dri2ext.c:564)
==24537==    by 0x430DDF: Dispatch (dispatch.c:428)
==24537==    by 0x421D6C: main (main.c:288)

I stored some additional data regarding the vblank event request in
nouveau_dri2_vblank_state, so that I could retrieve the information
at invalid write time:
(gdb) print *(struct nouveau_dri2_vblank_state*) event_data
$1 = {action = SWAP, client = 0x7740b20, draw = 37748775, dst = 
0xdd7c6d0, src = 0xbd1f5a0, func = 0x8910bf2 <DRI2SwapEvent>, data = 
0x99247b0, frame = 302185, current_msc = 302184, target_msc = 302185, 
remainder = 0, divisor = 0, hit = 0}

As you can see, in this event target_msc was current_msc + 1, but
still DRI2GetBuffersWithFormat() apparently managed to get called
first (and since swap limit was not altered, the call was not
throttled).


> Signed-off-by: Mario Kleiner <mario.kleiner-TdbV1Z3I5XE0NhjG498hmQ@public.gmane.org>
> ---
>  src/nouveau_dri2.c |   26 ++++++++++++++++++++++++++
>  1 files changed, 26 insertions(+), 0 deletions(-)
>
> diff --git a/src/nouveau_dri2.c b/src/nouveau_dri2.c
> index f0c7fec..7878a5a 100644
> --- a/src/nouveau_dri2.c
> +++ b/src/nouveau_dri2.c
> @@ -445,6 +445,26 @@ nouveau_dri2_schedule_swap(ClientPtr client,
> DrawablePtr draw,
>  		if (*target_msc == 0)
>  			*target_msc = 1;
>
> +#if DRI2INFOREC_VERSION >= 6
> +		/* Is this a swap in the future, ie. the vblank event will
> +		 * not be immediately dispatched, but only at a future vblank?
> +		 * If so, we need to temporarily lower the swaplimit to 1, so
> +		 * that DRI2GetBuffersWithFormat() requests from the client get
> +		 * deferred in the x-server until the vblank event has been
> +		 * dispatched to us and nouveau_dri2_finish_swap() is done. If
> +		 * we wouldn't do this, DRI2GetBuffersWithFormat() would operate
> +		 * on wrong (pre-swap) buffers, and cause a segfault later on in
> +		 * nouveau_dri2_finish_swap(). Our vblank event handler restores
> +		 * the old swaplimit immediately after nouveau_dri2_finish_swap()
> +		 * is done, so we still get 1 video refresh cycle worth of
> +		 * triple-buffering. For a swap at next vblank, dispatch of the
> +		 * vblank event happens immediately, so there isn't any need
> +		 * for this lowered swaplimit.
> +		 */
> +		if (current_msc < *target_msc - 1)
> +			DRI2SwapLimit(draw, 1);
> +#endif
> +
>  		/* Request a vblank event one frame before the target */
>  		ret = nouveau_wait_vblank(draw, DRM_VBLANK_ABSOLUTE |
>  					  DRM_VBLANK_EVENT,
> @@ -557,6 +577,12 @@ nouveau_dri2_vblank_handler(int fd, unsigned int 
> frame,
>  	switch (s->action) {
>  	case SWAP:
>  		nouveau_dri2_finish_swap(draw, frame, tv_sec, tv_usec, s);
> +#if DRI2INFOREC_VERSION >= 6
> +		/* Restore real swap limit on drawable, now that it is safe. */
> +		ScrnInfoPtr scrn = xf86Screens[draw->pScreen->myNum];
> +		DRI2SwapLimit(draw, NVPTR(scrn)->swap_limit);
> +#endif
> +
>  		break;
>
>  	case WAIT:

-- 
Anssi Hannula

Re: [PATCH 9/9] dri2: Fix corner case crash for swaplimit > 1

[Anssi Hannula]

12.07.2012 22:39, Anssi Hannula kirjoitti:
> Hi!
> 
> On 16.02.2012 02:45, Mario Kleiner wrote:
>> If a swaplimit > 1 is set on a server which
>> supports the swaplimit api (XOrg 1.12.0+),
>> the following can happen:
>>
>> 1. Client calls glXSwapBuffersMscOML() with a
>>    swap target > 1 vblank in the future, or a
>>    client calls glXSwapbuffers() while the swap
>>    interval is set to > 1 (unusual but possible).
>>
>> 2. nouveau_dri2_finish_swap() is therefore called
>>    only at the target vblank, instead of immediately.
>>
>> 3. Because of the deferred execution of
>>    nouveu_dri2_finish_swap(), the OpenGL client
>>    can call x-servers DRI2GetBuffersWithFormat()
>>    before nouveau_dri2_finish_swap() executes and
>>    it deletes pixmaps that would be needed by
>>    nouveau_dri2_finish_swap() --> Segfault --> Crash.
>>
>> Prevent this: When a swap is scheduled into the
>> future, we temporarily reduce the swaplimit to 1
>> until nouveau_dri2_finish_swap() is done, then
>> restore it to its original value. This throttles
>> the client inside the server in DRI2ThrottleClient()
>> before it can call the evil DRI2GetbuffersWithFormat().
>>
>> The client will still be released one video refresh
>> interval before swap completion, so there is still
>> some potential win.
>>
>> This doesn't affect the common case of swapping at
>> the next vblank, where this throttling is not needed
>> or done.
> 
> 
> After upgrading my system to X server 1.12.3, I've started to
> experience some crashes/freezes related to apparent memory
> corruption.
> Debugging with valgrind and gdb, it seems to me like the "evil"
> DRI2GetbuffersWithFormat() can be called before swap even when
> target_msc == current_msc + 1. This results in writes+reads to
> freed memory when the vblank event is finally being handled.
> 
> I'm not an Xorg/drm expert, but with a glance at the code I
> didn't see anything that would/should prevent
> DRI2GetbuffersWithFormat() from being called before the vblank
> event is handled. Even if the event is immediately sent by the
> kernel, isn't it possible that the X server services some other
> requests before the event is dispatched?
> 
> Am I missing something, or is that a bug?
> 
> In any case, I'm running ddx 1.0.1, xserver 1.12.3, libdrm 2.4.37,
> kernel 3.4.4, and I have Option "GLXVBlank" "true".
> 
> Here's one of the invalid write errors:
> 
> ==24537== Invalid write of size 4
> ==24537==    at 0x8D45029: nouveau_bo_name_get (nouveau.c:426)
> ==24537==    by 0x8B1EFFD: nouveau_dri2_finish_swap (nouveau_dri2.c:173)
> ==24537==    by 0x8B1F65F: nouveau_dri2_vblank_handler (nouveau_dri2.c:598)
> ==24537==    by 0x8707BA0: drmHandleEvent (xf86drmMode.c:808)
> ==24537==    by 0x8B37BC5: drmmode_wakeup_handler (drmmode_display.c:1447)
> ==24537==    by 0x43EA15: WakeupHandler (dixutils.c:421)
> ==24537==    by 0x5D7429: WaitForSomething (WaitFor.c:224)
> ==24537==    by 0x430B8B: Dispatch (dispatch.c:357)
> ==24537==    by 0x421D6C: main (main.c:288)
> ==24537==  Address 0xdd7c6d4 is 4 bytes inside a block of size 40 free'd
> ==24537==    at 0x4C25A9E: free (in
> /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==24537==    by 0x890E5A4: update_dri2_drawable_buffers (dri2.c:415)
> ==24537==    by 0x890E97D: do_get_buffers (dri2.c:525)
> ==24537==    by 0x890EB60: DRI2GetBuffersWithFormat (dri2.c:582)
> ==24537==    by 0x8910A7E: ProcDRI2GetBuffersWithFormat (dri2ext.c:299)
> ==24537==    by 0x8911256: ProcDRI2Dispatch (dri2ext.c:564)
> ==24537==    by 0x430DDF: Dispatch (dispatch.c:428)
> ==24537==    by 0x421D6C: main (main.c:288)
> 
> I stored some additional data regarding the vblank event request in
> nouveau_dri2_vblank_state, so that I could retrieve the information
> at invalid write time:
> (gdb) print *(struct nouveau_dri2_vblank_state*) event_data
> $1 = {action = SWAP, client = 0x7740b20, draw = 37748775, dst =
> 0xdd7c6d0, src = 0xbd1f5a0, func = 0x8910bf2 <DRI2SwapEvent>, data =
> 0x99247b0, frame = 302185, current_msc = 302184, target_msc = 302185,
> remainder = 0, divisor = 0, hit = 0}
> 
> As you can see, in this event target_msc was current_msc + 1, but
> still DRI2GetBuffersWithFormat() apparently managed to get called
> first (and since swap limit was not altered, the call was not
> throttled).

I've workarounded this locally for now with:
Option "SwapLimit" "1"

But I guess something should still be done about this memory corruption
issue...

> 
>> Signed-off-by: Mario Kleiner <mario.kleiner-TdbV1Z3I5XE0NhjG498hmQ@public.gmane.org>
>> ---
>>  src/nouveau_dri2.c |   26 ++++++++++++++++++++++++++
>>  1 files changed, 26 insertions(+), 0 deletions(-)
>>
>> diff --git a/src/nouveau_dri2.c b/src/nouveau_dri2.c
>> index f0c7fec..7878a5a 100644
>> --- a/src/nouveau_dri2.c
>> +++ b/src/nouveau_dri2.c
>> @@ -445,6 +445,26 @@ nouveau_dri2_schedule_swap(ClientPtr client,
>> DrawablePtr draw,
>>          if (*target_msc == 0)
>>              *target_msc = 1;
>>
>> +#if DRI2INFOREC_VERSION >= 6
>> +        /* Is this a swap in the future, ie. the vblank event will
>> +         * not be immediately dispatched, but only at a future vblank?
>> +         * If so, we need to temporarily lower the swaplimit to 1, so
>> +         * that DRI2GetBuffersWithFormat() requests from the client get
>> +         * deferred in the x-server until the vblank event has been
>> +         * dispatched to us and nouveau_dri2_finish_swap() is done. If
>> +         * we wouldn't do this, DRI2GetBuffersWithFormat() would operate
>> +         * on wrong (pre-swap) buffers, and cause a segfault later on in
>> +         * nouveau_dri2_finish_swap(). Our vblank event handler restores
>> +         * the old swaplimit immediately after
>> nouveau_dri2_finish_swap()
>> +         * is done, so we still get 1 video refresh cycle worth of
>> +         * triple-buffering. For a swap at next vblank, dispatch of the
>> +         * vblank event happens immediately, so there isn't any need
>> +         * for this lowered swaplimit.
>> +         */
>> +        if (current_msc < *target_msc - 1)
>> +            DRI2SwapLimit(draw, 1);
>> +#endif
>> +
>>          /* Request a vblank event one frame before the target */
>>          ret = nouveau_wait_vblank(draw, DRM_VBLANK_ABSOLUTE |
>>                        DRM_VBLANK_EVENT,
>> @@ -557,6 +577,12 @@ nouveau_dri2_vblank_handler(int fd, unsigned int
>> frame,
>>      switch (s->action) {
>>      case SWAP:
>>          nouveau_dri2_finish_swap(draw, frame, tv_sec, tv_usec, s);
>> +#if DRI2INFOREC_VERSION >= 6
>> +        /* Restore real swap limit on drawable, now that it is safe. */
>> +        ScrnInfoPtr scrn = xf86Screens[draw->pScreen->myNum];
>> +        DRI2SwapLimit(draw, NVPTR(scrn)->swap_limit);
>> +#endif
>> +
>>          break;
>>
>>      case WAIT:
> 



-- 
Anssi Hannula