uterm: drm: share mode-setting between dumb+drm

The modesetting code of both DRM backends is almost the same so share it.
This also works around several race-conditions in some DRM drivers. In
particular, we cannot call drmModeSetCrtc while a drmModePageFlip is still
pending. Therefore, we wait for page-flips to complete before performing
an immediate mode-set.

This requires us to handle page-flip events synchronously so we can wait
for kernel page-flip events but the user-space bottom-half is executed in
an idle-handler.

Note that the chance that we have to wait for a page-flip to complete is
pretty small. In fact, without hacking the code to do fast page-flips, I
couldn't get kmscon to run into this condition.

Last but not least, this patch also makes the dumb-backend support
immediate page-flips like the DRM backend did all the time.

Signed-off-by: David Herrmann <dh.herrmann@googlemail.com>

Makefile.am

@@ -230,6 +230,7 @@ libuterm_la_SOURCES = \
 	$(SHL_DLIST) \
 	$(SHL_HOOK) \
 	$(SHL_MISC) \
+	$(SHL_TIMER) \
 	src/uterm_input.h \
 	src/uterm_monitor.h \
 	src/uterm_video.h \

src/uterm_drm_shared.c

@@ -29,6 +29,7 @@
 
 #include <errno.h>
 #include <fcntl.h>
+#include <poll.h>
 #include <stdbool.h>
 #include <stdlib.h>
 #include <string.h>
@@ -36,6 +37,7 @@
 #include <xf86drm.h>
 #include <xf86drmMode.h>
 #include "log.h"
+#include "shl_timer.h"
 #include "uterm_drm_shared_internal.h"
 #include "uterm_video.h"
 #include "uterm_video_internal.h"
@@ -265,6 +267,8 @@ void uterm_drm_display_deactivate(struct uterm_display *disp, int fd)
 {
 	struct uterm_drm_display *ddrm = disp->data;
 
+	uterm_drm_display_wait_pflip(disp);
+
 	if (ddrm->saved_crtc) {
 		if (disp->video->flags & VIDEO_AWAKE) {
 			drmModeSetCrtc(fd, ddrm->saved_crtc->crtc_id,
@@ -279,6 +283,7 @@ void uterm_drm_display_deactivate(struct uterm_display *disp, int fd)
 	}
 
 	ddrm->crtc_id = 0;
+	disp->flags &= ~(DISPLAY_VSYNC | DISPLAY_ONLINE | DISPLAY_PFLIP);
 }
 
 int uterm_drm_display_set_dpms(struct uterm_display *disp, int state)
@@ -298,19 +303,141 @@ int uterm_drm_display_set_dpms(struct uterm_display *disp, int state)
 	return 0;
 }
 
-static void event(struct ev_fd *fd, int mask, void *data)
+int uterm_drm_display_wait_pflip(struct uterm_display *disp)
+{
+	struct uterm_video *video = disp->video;
+	int ret;
+	unsigned int timeout = 1000; /* 1s */
+
+	if ((disp->flags & DISPLAY_PFLIP) || !(disp->flags & DISPLAY_VSYNC))
+		return 0;
+
+	do {
+		ret = uterm_drm_video_wait_pflip(video, &timeout);
+		if (ret < 1)
+			break;
+		else if ((disp->flags & DISPLAY_PFLIP))
+			break;
+	} while (timeout > 0);
+
+	if (ret < 0)
+		return ret;
+	if (ret == 0 || !timeout) {
+		log_warning("timeout waiting for page-flip on display %p",
+			    disp);
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
+int uterm_drm_display_swap(struct uterm_display *disp, uint32_t fb,
+			   bool immediate)
+{
+	struct uterm_drm_display *ddrm = disp->data;
+	struct uterm_video *video = disp->video;
+	struct uterm_drm_video *vdrm = video->data;
+	int ret;
+	drmModeModeInfo *mode;
+
+	if (disp->dpms != UTERM_DPMS_ON)
+		return -EINVAL;
+
+	if (immediate) {
+		ret = uterm_drm_display_wait_pflip(disp);
+		if (ret)
+			return ret;
+
+		mode = uterm_drm_mode_get_info(disp->current_mode);
+		ret = drmModeSetCrtc(vdrm->fd, ddrm->crtc_id, fb, 0, 0,
+				     &ddrm->conn_id, 1, mode);
+		if (ret) {
+			log_error("cannot set DRM-CRTC (%d): %m", errno);
+			return -EFAULT;
+		}
+	} else {
+		if ((disp->flags & DISPLAY_VSYNC))
+			return -EBUSY;
+
+		ret = drmModePageFlip(vdrm->fd, ddrm->crtc_id, fb,
+				      DRM_MODE_PAGE_FLIP_EVENT, disp);
+		if (ret) {
+			log_error("cannot page-flip on DRM-CRTC (%d): %m",
+				  errno);
+			return -EFAULT;
+		}
+
+		uterm_display_ref(disp);
+		disp->flags |= DISPLAY_VSYNC;
+	}
+
+	return 0;
+}
+
+static void uterm_drm_display_pflip(struct uterm_display *disp)
+{
+	struct uterm_drm_video *vdrm = disp->video->data;
+
+	disp->flags &= ~(DISPLAY_PFLIP | DISPLAY_VSYNC);
+	if (vdrm->page_flip)
+		vdrm->page_flip(disp);
+
+	DISPLAY_CB(disp, UTERM_PAGE_FLIP);
+}
+
+static void display_event(int fd, unsigned int frame, unsigned int sec,
+			  unsigned int usec, void *data)
+{
+	struct uterm_display *disp = data;
+
+	if (disp->video && (disp->flags & DISPLAY_VSYNC))
+		disp->flags |= DISPLAY_PFLIP;
+
+	uterm_display_unref(disp);
+}
+
+static int uterm_drm_video_read_events(struct uterm_video *video)
+{
+	struct uterm_drm_video *vdrm = video->data;
+	drmEventContext ev;
+	int ret;
+
+	/* TODO: DRM subsystem does not support non-blocking reads and it also
+	 * doesn't return 0/-1 if the device is dead. This can lead to serious
+	 * deadlocks in userspace if we read() after a device was unplugged. Fix
+	 * this upstream and then make this code actually loop. */
+	memset(&ev, 0, sizeof(ev));
+	ev.version = DRM_EVENT_CONTEXT_VERSION;
+	ev.page_flip_handler = display_event;
+	errno = 0;
+	ret = drmHandleEvent(vdrm->fd, &ev);
+
+	if (ret < 0 && errno != EAGAIN)
+		return -EFAULT;
+
+	return 0;
+}
+
+static void do_pflips(struct ev_eloop *eloop, void *unused, void *data)
+{
+	struct uterm_video *video = data;
+	struct uterm_display *disp;
+	struct shl_dlist *iter;
+
+	shl_dlist_for_each(iter, &video->displays) {
+		disp = shl_dlist_entry(iter, struct uterm_display, list);
+		if ((disp->flags & DISPLAY_PFLIP))
+			uterm_drm_display_pflip(disp);
+	}
+}
+
+static void io_event(struct ev_fd *fd, int mask, void *data)
 {
 	struct uterm_video *video = data;
 	struct uterm_drm_video *vdrm = video->data;
-	drmEventContext ev;
-
-	/* TODO: run this in a loop with O_NONBLOCK */
-	if (mask & EV_READABLE) {
-		memset(&ev, 0, sizeof(ev));
-		ev.version = DRM_EVENT_CONTEXT_VERSION;
-		ev.page_flip_handler = vdrm->page_flip;
-		drmHandleEvent(vdrm->fd, &ev);
-	}
+	struct uterm_display *disp;
+	struct shl_dlist *iter;
+	int ret;
 
 	/* TODO: forward HUP to caller */
 	if (mask & (EV_HUP | EV_ERR)) {
@@ -319,6 +446,19 @@ static void event(struct ev_fd *fd, int mask, void *data)
 		vdrm->efd = NULL;
 		return;
 	}
+
+	if (!(mask & EV_READABLE))
+		return;
+
+	ret = uterm_drm_video_read_events(video);
+	if (ret)
+		return;
+
+	shl_dlist_for_each(iter, &video->displays) {
+		disp = shl_dlist_entry(iter, struct uterm_display, list);
+		if ((disp->flags & DISPLAY_PFLIP))
+			uterm_drm_display_pflip(disp);
+	}
 }
 
 int uterm_drm_video_init(struct uterm_video *video, const char *node,
@@ -337,7 +477,7 @@ int uterm_drm_video_init(struct uterm_video *video, const char *node,
 	vdrm->data = data;
 	vdrm->page_flip = pflip;
 
-	vdrm->fd = open(node, O_RDWR | O_CLOEXEC);
+	vdrm->fd = open(node, O_RDWR | O_CLOEXEC | O_NONBLOCK);
 	if (vdrm->fd < 0) {
 		log_err("cannot open drm device %s (%d): %m", node, errno);
 		ret = -EFAULT;
@@ -347,13 +487,19 @@ int uterm_drm_video_init(struct uterm_video *video, const char *node,
 	drmDropMaster(vdrm->fd);
 
 	ret = ev_eloop_new_fd(video->eloop, &vdrm->efd, vdrm->fd, EV_READABLE,
-			      event, video);
+			      io_event, video);
 	if (ret)
 		goto err_close;
 
+	ret = shl_timer_new(&vdrm->timer);
+	if (ret)
+		goto err_fd;
+
 	video->flags |= VIDEO_HOTPLUG;
 	return 0;
 
+err_fd:
+	ev_eloop_rm_fd(vdrm->efd);
 err_close:
 	close(vdrm->fd);
 err_free:
@@ -365,6 +511,8 @@ void uterm_drm_video_destroy(struct uterm_video *video)
 {
 	struct uterm_drm_video *vdrm = video->data;
 
+	ev_eloop_unregister_idle_cb(video->eloop, do_pflips, video, EV_SINGLE);
+	shl_timer_free(vdrm->timer);
 	ev_eloop_rm_fd(vdrm->efd);
 	close(vdrm->fd);
 	free(video->data);
@@ -566,3 +714,49 @@ int uterm_drm_video_poll(struct uterm_video *video,
 	video->flags |= VIDEO_HOTPLUG;
 	return uterm_drm_video_hotplug(video, ops, false);
 }
+
+/* Waits for events on DRM fd for \mtimeout milliseconds and returns 0 if the
+ * timeout expired, -ERR on errors and 1 if a page-flip event has been read.
+ * \mtimeout is adjusted to the remaining time. */
+int uterm_drm_video_wait_pflip(struct uterm_video *video,
+			       unsigned int *mtimeout)
+{
+	struct uterm_drm_video *vdrm = video->data;
+	struct pollfd pfd;
+	int ret;
+	uint64_t elapsed;
+
+	shl_timer_start(vdrm->timer);
+
+	memset(&pfd, 0, sizeof(pfd));
+	pfd.fd = vdrm->fd;
+	pfd.events = POLLIN;
+
+	log_debug("waiting for pageflip on %p", video);
+	ret = poll(&pfd, 1, *mtimeout);
+
+	elapsed = shl_timer_stop(vdrm->timer);
+	*mtimeout = *mtimeout - (elapsed / 1000 + 1);
+
+	if (ret < 0) {
+		log_error("poll() failed on DRM fd (%d): %m", errno);
+		return -EFAULT;
+	} else if (!ret) {
+		log_warning("timeout waiting for page-flip on %p", video);
+		return 0;
+	} else if ((pfd.revents & POLLIN)) {
+		ret = uterm_drm_video_read_events(video);
+		if (ret)
+			return ret;
+
+		ret = ev_eloop_register_idle_cb(video->eloop, do_pflips,
+						video, EV_ONESHOT | EV_SINGLE);
+		if (ret)
+			return ret;
+
+		return 1;
+	} else {
+		log_debug("poll() HUP/ERR on DRM fd (%d)", pfd.revents);
+		return -EFAULT;
+	}
+}

src/uterm_drm_shared_internal.h

@@ -32,6 +32,7 @@
 #include <xf86drm.h>
 #include <xf86drmMode.h>
 #include "eloop.h"
+#include "shl_timer.h"
 #include "uterm_video.h"
 #include "uterm_video_internal.h"
 
@@ -76,6 +77,9 @@ void uterm_drm_display_destroy(struct uterm_display *disp);
 int uterm_drm_display_activate(struct uterm_display *disp, int fd);
 void uterm_drm_display_deactivate(struct uterm_display *disp, int fd);
 int uterm_drm_display_set_dpms(struct uterm_display *disp, int state);
+int uterm_drm_display_wait_pflip(struct uterm_display *disp);
+int uterm_drm_display_swap(struct uterm_display *disp, uint32_t fb,
+			   bool immediate);
 
 static inline void *uterm_drm_display_get_data(struct uterm_display *disp)
 {
@@ -86,15 +90,14 @@ static inline void *uterm_drm_display_get_data(struct uterm_display *disp)
 
 /* drm video */
 
-typedef void (*uterm_drm_page_flip_t) (int fd, unsigned int frame,
-				       unsigned int sec, unsigned int usec,
-				       void *data);
+typedef void (*uterm_drm_page_flip_t) (struct uterm_display *disp);
 
 struct uterm_drm_video {
 	int fd;
 	struct ev_fd *efd;
 	uterm_drm_page_flip_t page_flip;
 	void *data;
+	struct shl_timer *timer;
 };
 
 int uterm_drm_video_init(struct uterm_video *video, const char *node,
@@ -109,6 +112,8 @@ int uterm_drm_video_wake_up(struct uterm_video *video,
 void uterm_drm_video_sleep(struct uterm_video *video);
 int uterm_drm_video_poll(struct uterm_video *video,
 			 const struct display_ops *ops);
+int uterm_drm_video_wait_pflip(struct uterm_video *video,
+			       unsigned int *mtimeout);
 
 static inline void *uterm_drm_video_get_data(struct uterm_video *video)
 {

src/uterm_video_drm.c

@@ -64,7 +64,6 @@ struct uterm_drm3d_display {
 	EGLSurface surface;
 	struct uterm_drm3d_rb *current;
 	struct uterm_drm3d_rb *next;
-	unsigned int ignore_flips;
 };
 
 struct uterm_drm3d_video {
@@ -304,7 +303,6 @@ static void display_deactivate(struct uterm_display *disp)
 
 	gbm_surface_destroy(d3d->gbm);
 	disp->current_mode = NULL;
-	disp->flags &= ~(DISPLAY_ONLINE | DISPLAY_VSYNC);
 }
 
 static int display_use(struct uterm_display *disp)
@@ -328,46 +326,12 @@ static int swap_display(struct uterm_display *disp, bool immediate)
 	struct gbm_bo *bo;
 	struct uterm_drm3d_rb *rb;
 	struct uterm_drm3d_display *d3d = uterm_drm_display_get_data(disp);
-	struct uterm_drm_display *ddrm = disp->data;
-	struct uterm_drm3d_video *v3d;
-	struct uterm_drm_video *vdrm;
+	struct uterm_video *video = disp->video;
+	struct uterm_drm3d_video *v3d = uterm_drm_video_get_data(video);
 
-	if (disp->dpms != UTERM_DPMS_ON)
-		return -EINVAL;
-	if (!immediate &&
-	    ((disp->flags & DISPLAY_VSYNC) || d3d->ignore_flips))
+	if (!gbm_surface_has_free_buffers(d3d->gbm))
 		return -EBUSY;
 
-	vdrm = disp->video->data;
-	v3d = uterm_drm_video_get_data(disp->video);
-	/* TODO: immediate page-flips are somewhat buggy and can cause
-	 * dead-locks in the kernel. This is being worked on and will hopefully
-	 * be fixed soon. However, until then, we prevent immediate page-flips
-	 * if there is another vsync'ed flip pending and print a warning
-	 * instead. If this is fixed, simply remove this warning and everything
-	 * should work. */
-	if (disp->flags & DISPLAY_VSYNC) {
-		log_warning("immediate page-flip canceled as another page-flip is pending");
-		return 0;
-	}
-
-	if (!gbm_surface_has_free_buffers(d3d->gbm)) {
-		if (d3d->next) {
-			log_debug("no free buffer, releasing next-buffer");
-			gbm_surface_release_buffer(d3d->gbm, d3d->next->bo);
-			d3d->next = NULL;
-		} else if (d3d->current) {
-			log_debug("no free buffer, releasing current-buffer");
-			gbm_surface_release_buffer(d3d->gbm, d3d->current->bo);
-			d3d->current = NULL;
-		}
-
-		if (!gbm_surface_has_free_buffers(d3d->gbm)) {
-			log_warning("gbm ran out of free buffers");
-			return -EFAULT;
-		}
-	}
-
 	if (!eglSwapBuffers(v3d->disp, d3d->surface)) {
 		log_error("cannot swap EGL buffers (%d): %m", errno);
 		return -EFAULT;
@@ -386,43 +350,23 @@ static int swap_display(struct uterm_display *disp, bool immediate)
 		return -EFAULT;
 	}
 
+	ret = uterm_drm_display_swap(disp, rb->fb, immediate);
+	if (ret) {
+		gbm_surface_release_buffer(d3d->gbm, bo);
+		return ret;
+	}
+
+	if (d3d->next) {
+		gbm_surface_release_buffer(d3d->gbm, d3d->next->bo);
+		d3d->next = NULL;
+	}
+
 	if (immediate) {
-		ret = drmModeSetCrtc(vdrm->fd, ddrm->crtc_id,
-				     rb->fb, 0, 0, &ddrm->conn_id, 1,
-				     uterm_drm_mode_get_info(disp->current_mode));
-		if (ret) {
-			log_err("cannot set drm-crtc");
-			gbm_surface_release_buffer(d3d->gbm, bo);
-			return -EFAULT;
-		}
-
-		if (d3d->current) {
+		if (d3d->current)
 			gbm_surface_release_buffer(d3d->gbm, d3d->current->bo);
-			d3d->current = NULL;
-		}
-		if (d3d->next) {
-			gbm_surface_release_buffer(d3d->gbm, d3d->next->bo);
-			d3d->next = NULL;
-		}
 		d3d->current = rb;
-
-		if (disp->flags & DISPLAY_VSYNC) {
-			disp->flags &= ~DISPLAY_VSYNC;
-			d3d->ignore_flips++;
-			DISPLAY_CB(disp, UTERM_PAGE_FLIP);
-		}
 	} else {
-		ret = drmModePageFlip(vdrm->fd, ddrm->crtc_id,
-				      rb->fb, DRM_MODE_PAGE_FLIP_EVENT, disp);
-		if (ret) {
-			log_warn("page-flip failed %d %d", ret, errno);
-			gbm_surface_release_buffer(d3d->gbm, bo);
-			return -EFAULT;
-		}
-
 		d3d->next = rb;
-		uterm_display_ref(disp);
-		disp->flags |= DISPLAY_VSYNC;
 	}
 
 	return 0;
@@ -913,27 +857,17 @@ static void show_displays(struct uterm_video *video)
 	}
 }
 
-static void page_flip_handler(int fd, unsigned int frame, unsigned int sec,
-			      unsigned int usec, void *data)
+static void page_flip_handler(struct uterm_display *disp)
 {
-	struct uterm_display *disp = data;
 	struct uterm_drm3d_display *d3d = uterm_drm_display_get_data(disp);
 
-	if (d3d->ignore_flips) {
-		--d3d->ignore_flips;
-	} else if (disp->flags & DISPLAY_VSYNC) {
-		disp->flags &= ~DISPLAY_VSYNC;
-		if (d3d->next) {
-			if (d3d->current)
-				gbm_surface_release_buffer(d3d->gbm,
-							   d3d->current->bo);
-			d3d->current = d3d->next;
-			d3d->next = NULL;
-		}
-		DISPLAY_CB(disp, UTERM_PAGE_FLIP);
+	if (d3d->next) {
+		if (d3d->current)
+			gbm_surface_release_buffer(d3d->gbm,
+						   d3d->current->bo);
+		d3d->current = d3d->next;
+		d3d->next = NULL;
 	}
-
-	uterm_display_unref(disp);
 }
 
 static int video_init(struct uterm_video *video, const char *node)

src/uterm_video_dumb.c

@@ -238,33 +238,25 @@ static void display_deactivate(struct uterm_display *disp)
 	destroy_rb(disp, &d2d->rb[1]);
 	destroy_rb(disp, &d2d->rb[0]);
 	disp->current_mode = NULL;
-	disp->flags &= ~(DISPLAY_ONLINE | DISPLAY_VSYNC);
+}
+
+static int swap_display(struct uterm_display *disp, bool immediate)
+{
+	int ret, rb;
+	struct uterm_drm2d_display *d2d = uterm_drm_display_get_data(disp);
+
+	rb = d2d->current_rb ^ 1;
+	ret = uterm_drm_display_swap(disp, d2d->rb[rb].fb, immediate);
+	if (ret)
+		return ret;
+
+	d2d->current_rb = rb;
+	return 0;
 }
 
 static int display_swap(struct uterm_display *disp)
 {
-	int ret;
-	struct uterm_drm_display *ddrm = disp->data;
-	struct uterm_drm2d_display *d2d = uterm_drm_display_get_data(disp);
-	struct uterm_drm_video *vdrm;
-
-	if (disp->dpms != UTERM_DPMS_ON)
-		return -EINVAL;
-
-	vdrm = disp->video->data;
-	errno = 0;
-	d2d->current_rb ^= 1;
-	ret = drmModePageFlip(vdrm->fd, ddrm->crtc_id,
-			      d2d->rb[d2d->current_rb].fb,
-			      DRM_MODE_PAGE_FLIP_EVENT, disp);
-	if (ret) {
-		log_warn("page-flip failed %d %d", ret, errno);
-		return -EFAULT;
-	}
-	uterm_display_ref(disp);
-	disp->flags |= DISPLAY_VSYNC;
-
-	return 0;
+	return swap_display(disp, false);
 }
 
 static int display_blit(struct uterm_display *disp,
@@ -448,12 +440,9 @@ static const struct display_ops dumb_display_ops = {
 
 static void show_displays(struct uterm_video *video)
 {
-	int ret;
 	struct uterm_display *iter;
-	struct uterm_drm_display *ddrm;
 	struct uterm_drm2d_display *d2d;
 	struct uterm_drm2d_rb *rb;
-	struct uterm_drm_video *vdrm = video->data;
 	struct shl_dlist *i;
 
 	if (!video_is_awake(video))
@@ -467,40 +456,26 @@ static void show_displays(struct uterm_video *video)
 		if (iter->dpms != UTERM_DPMS_ON)
 			continue;
 
-		ddrm = iter->data;
+		/* We use double-buffering so there might be no free back-buffer
+		 * here. Hence, draw into the current (pending) front-buffer and
+		 * wait for possible page-flips to complete. This might cause
+		 * tearing but that's acceptable as this is only called during
+		 * wakeup/sleep. */
+
 		d2d = uterm_drm_display_get_data(iter);
 		rb = &d2d->rb[d2d->current_rb];
-
 		memset(rb->map, 0, rb->size);
-		ret = drmModeSetCrtc(vdrm->fd, ddrm->crtc_id,
-				     rb->fb, 0, 0, &ddrm->conn_id, 1,
-				     uterm_drm_mode_get_info(iter->current_mode));
-		if (ret) {
-			log_err("cannot set drm-crtc on display %p", iter);
-			continue;
-		}
+		uterm_drm_display_wait_pflip(iter);
 	}
 }
 
-static void page_flip_handler(int fd, unsigned int frame, unsigned int sec,
-			      unsigned int usec, void *data)
-{
-	struct uterm_display *disp = data;
-
-	if (disp->flags & DISPLAY_VSYNC) {
-		disp->flags &= ~DISPLAY_VSYNC;
-		DISPLAY_CB(disp, UTERM_PAGE_FLIP);
-	}
-	uterm_display_unref(disp);
-}
-
 static int video_init(struct uterm_video *video, const char *node)
 {
 	int ret;
 	uint64_t has_dumb;
 	struct uterm_drm_video *vdrm;
 
-	ret = uterm_drm_video_init(video, node, page_flip_handler, NULL);
+	ret = uterm_drm_video_init(video, node, NULL, NULL);
 	if (ret)
 		return ret;
 	vdrm = video->data;

src/uterm_video_internal.h

@@ -104,6 +104,7 @@ void uterm_mode_unbind(struct uterm_mode *mode);
 #define DISPLAY_OPEN		0x08
 #define DISPLAY_DBUF		0x10
 #define DISPLAY_DITHERING	0x20
+#define DISPLAY_PFLIP		0x40
 
 struct uterm_display {
 	struct shl_dlist list;