From ead3c406a25c9196372cc5ce0118dacd9174ea3f Mon Sep 17 00:00:00 2001 From: Manuel Alfayate Corchete Date: Wed, 13 Jan 2021 20:11:01 +0100 Subject: [PATCH] [KMS/DRM] Refactor, improve and re-comment async pageflips code. --- src/video/kmsdrm/SDL_kmsdrmopengles.c | 23 ++++++++------ src/video/kmsdrm/SDL_kmsdrmvideo.c | 46 ++++++++++++++++++++------- 2 files changed, 48 insertions(+), 21 deletions(-) diff --git a/src/video/kmsdrm/SDL_kmsdrmopengles.c b/src/video/kmsdrm/SDL_kmsdrmopengles.c index 888190707..d9e14b037 100644 --- a/src/video/kmsdrm/SDL_kmsdrmopengles.c +++ b/src/video/kmsdrm/SDL_kmsdrmopengles.c @@ -154,13 +154,14 @@ KMSDRM_GLES_SwapWindow(_THIS, SDL_Window * window) { /* Issue pageflip on the next front buffer. Remember: drmModePageFlip() never blocks, it just issues the flip, - which will be done during the next vblank. - Since it will return EBUSY if we call it again without having - completed the last issued flip, we must pass the + which will be done during the next vblank, or immediately if + we pass the DRM_MODE_PAGE_FLIP_ASYNC flag. + Since calling drmModePageFlip() will return EBUSY if we call it + without having completed the last issued flip, we must pass the DRM_MODE_PAGE_FLIP_ASYNC if we don't block on EGL (egl_swapinterval = 0). - That makes it flip immediately, without waiting for the next vblank, - so even if we don't block on EGL, it will have flipped when we - get back here. */ + That makes it flip immediately, without waiting for the next vblank + to do so, so even if we don't block on EGL, the flip will have completed + when we get here again. */ if (_this->egl_data->egl_swapinterval == 0) { flip_flags |= DRM_MODE_PAGE_FLIP_ASYNC; @@ -173,11 +174,15 @@ KMSDRM_GLES_SwapWindow(_THIS, SDL_Window * window) { windata->waiting_for_flip = SDL_TRUE; } else { SDL_LogError(SDL_LOG_CATEGORY_VIDEO, "Could not queue pageflip: %d", ret); - printf("Could not queue pageflip: %s\n", strerror(errno)); } - /* If we are in double-buffer mode, wait immediately for vsync - (as if we only had two buffers), + /* Wait immediately for vsync (as if we only had two buffers). + Even if we are already doing a WaitPageFlip at the begining of this + function, this is NOT redundant because here we wait immediately + after submitting the image to the screen, reducing lag, and if + we have waited here, there won't be a pending pageflip so the + WaitPageFlip at the beggining of this function will be a no-op. + Just leave it here and don't worry. Run your SDL2 program with "SDL_KMSDRM_DOUBLE_BUFFER=1 " to enable this. */ if (_this->egl_data->egl_swapinterval == 1 && windata->double_buffer) { diff --git a/src/video/kmsdrm/SDL_kmsdrmvideo.c b/src/video/kmsdrm/SDL_kmsdrmvideo.c index b47e90f3d..821e7e71e 100644 --- a/src/video/kmsdrm/SDL_kmsdrmvideo.c +++ b/src/video/kmsdrm/SDL_kmsdrmvideo.c @@ -45,8 +45,9 @@ #include "SDL_kmsdrmvulkan.h" #include #include -#include #include +#include +#include #ifdef __OpenBSD__ #define KMSDRM_DRI_PATH "/dev/" @@ -340,42 +341,63 @@ KMSDRM_FlipHandler(int fd, unsigned int frame, unsigned int sec, unsigned int us SDL_bool KMSDRM_WaitPageFlip(_THIS, SDL_WindowData *windata) { + SDL_VideoData *viddata = ((SDL_VideoData *)_this->driverdata); drmEventContext ev = {0}; struct pollfd pfd = {0}; - /* If the pageflip hasn't completed after 10 seconds, it nevel will. */ - uint32_t timeout = 10000; - + ev.version = DRM_EVENT_CONTEXT_VERSION; ev.page_flip_handler = KMSDRM_FlipHandler; pfd.fd = viddata->drm_fd; pfd.events = POLLIN; + /* Stay on loop until we get the desired event + We need the while the loop because we could be in a situation where: + -We get events on the FD in time, thus not on exiting on return number 1. + -These events are not errors, thus not exiting on return number 2. + -These events are of POLLIN type, thus not exiting on return number 3, + but if the event is not the pageflip we are waiting for, we arrive at the end + of the loop and do loop re-entry, hoping the next event will be the pageflip. + + So we could erroneously exit the function without the pageflip event to arrive + if it wasn't for the while loop! + + Vblank events, for example, hit the FD and they are POLLIN events too (POLLIN + means "there's data to read on the FD"), but they are not the pageflip event + we are waiting for, so the drmEventHandle() doesn't run the flip handler, and + since waiting_for_flip is set on the pageflip handle, it's not set and we stay + on the loop. + */ while (windata->waiting_for_flip) { - pfd.revents = 0; + + pfd.revents = 0; /* poll() waits for events arriving on the FD, and returns < 0 if timeout - passes with no events. */ - if (poll(&pfd, 1, timeout) < 0) { + passes with no events. + We wait forever (timeout = -1), but even if we DO get an event, + we have yet to see if it's of the required type. */ + if (poll(&pfd, 1, -1) < 0) { SDL_LogError(SDL_LOG_CATEGORY_VIDEO, "DRM poll error"); - return SDL_FALSE; + return SDL_FALSE; /* Return number 1. */ } if (pfd.revents & (POLLHUP | POLLERR)) { /* An event arrived on the FD in time, but it's an error. */ SDL_LogError(SDL_LOG_CATEGORY_VIDEO, "DRM poll hup or error"); - return SDL_FALSE; + return SDL_FALSE; /* Return number 2. */ } if (pfd.revents & POLLIN) { /* There is data to read on the FD! - Is the event a pageflip? If so, drmHandleEvent will - unset windata->waiting_for_flip */ + Is the event a pageflip? We know it is a pageflip if it matches the + event we are passing in &ev. If it is, drmHandleEvent() will unset + windata->waiting_for_flip and we will get out of the "while" loop. + If it's not, we keep iterating on the loop. */ KMSDRM_drmHandleEvent(viddata->drm_fd, &ev); } else { SDL_LogDebug(SDL_LOG_CATEGORY_VIDEO, "Dropping frame while waiting_for_flip"); - return SDL_FALSE; + return SDL_FALSE; /* Return number 3. */ } }