From 5604a0a32586518ecef2726da24a55f6f8abffc2 Mon Sep 17 00:00:00 2001 From: David Herrmann Date: Sun, 13 Jan 2013 15:26:56 +0100 Subject: [PATCH] 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 --- Makefile.am | 1 + src/uterm_drm_shared.c | 218 ++++++++++++++++++++++++++++++-- src/uterm_drm_shared_internal.h | 11 +- src/uterm_video_drm.c | 110 ++++------------ src/uterm_video_dumb.c | 71 ++++------- src/uterm_video_internal.h | 1 + 6 files changed, 261 insertions(+), 151 deletions(-) diff --git a/Makefile.am b/Makefile.am index a18498b..f64d51a 100644 --- a/Makefile.am +++ b/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 \ diff --git a/src/uterm_drm_shared.c b/src/uterm_drm_shared.c index d97f139..9071b9c 100644 --- a/src/uterm_drm_shared.c +++ b/src/uterm_drm_shared.c @@ -29,6 +29,7 @@ #include #include +#include #include #include #include @@ -36,6 +37,7 @@ #include #include #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; + } +} diff --git a/src/uterm_drm_shared_internal.h b/src/uterm_drm_shared_internal.h index fc92b6f..824fbdc 100644 --- a/src/uterm_drm_shared_internal.h +++ b/src/uterm_drm_shared_internal.h @@ -32,6 +32,7 @@ #include #include #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) { diff --git a/src/uterm_video_drm.c b/src/uterm_video_drm.c index f7dc080..758883c 100644 --- a/src/uterm_video_drm.c +++ b/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) diff --git a/src/uterm_video_dumb.c b/src/uterm_video_dumb.c index 032ce52..c4b69e5 100644 --- a/src/uterm_video_dumb.c +++ b/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; diff --git a/src/uterm_video_internal.h b/src/uterm_video_internal.h index 017acb3..d52c0cd 100644 --- a/src/uterm_video_internal.h +++ b/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;