Fixed bug 5140 - KMSDRM: Dynamic vsync toggle does not work

Manuel Alfayate Corchete

The KMSDRM backend was doing things wrong because of some small (but important) misconceptions on how KMS/DRM works: to implement a largely broken non-vsync refresh mechanism, the SwapWindow() function was issuing new pageflips before previous ones had completed, thus causing EBUSY returns, buffer mismanagement, etc... resulting in general breakage on vsync disabling from apps, that would not allow vsync to work again without KMSDRM video re-initialization.
To further clarify, on most DRM drivers async pageflips are NOT working nowadays, so all issued pageflips will complete on next VBLANK, NOT ASAP (calling drmModePageFlip() with the DRM_MODE_PAGE_FLIP_ASYNC flag will return error).

The old code was assuming that can just issue a synchronous (=on VBLANK) pageflip and then pass a 0 timeout to the pull() function so we do not wait for the pageflip event, thinking that this will lead to correct non-vsynced screen updates from the program: That is plain wrong.
Each pageflip has to be waite before issuing a new one, ALWAYS. And if we do not support ASYNC pageflips on the DRM driver level, then we are forced to wait for the next VBLANK. There is no way around it.

I have also added many comments on the KMSDRM code. This is needed for future reference for me or others who may need to look at this code: KMS/DRM terminology regarding what SYNC and ASYNC mean in pageflip terms, and where to do certain things and why, is not trivial. It is not desirable or possible to invest time on researching the same concepts every time there is need to dive into this code. So please leave all these comments in the patch.

src/video/kmsdrm/SDL_kmsdrmopengles.c

@@ -61,82 +61,169 @@ KMSDRM_GLES_SwapWindow(_THIS, SDL_Window * window) {
     SDL_DisplayData *dispdata = (SDL_DisplayData *) SDL_GetDisplayForWindow(window)->driverdata;
     SDL_VideoData *viddata = ((SDL_VideoData *)_this->driverdata);
     KMSDRM_FBInfo *fb_info;
-    int ret, timeout;
+    SDL_bool crtc_setup_pending = SDL_FALSE;
+
+    /* ALWAYS wait for each pageflip to complete before issuing another, vsync or not,
+       or drmModePageFlip() will start returning EBUSY if there are pending pageflips.
+
+       To disable vsync in games, it would be needed to issue async pageflips,
+       and then wait for each pageflip to complete. Since async pageflips complete ASAP 
+       instead of VBLANK, thats how non-vsync screen updates should wok.
+
+       BUT Async pageflips do not work right now because calling drmModePageFlip() with the
+       DRM_MODE_PAGE_FLIP_ASYNC flag returns error on every driver I have tried.
+
+       So, for now, only do vsynced updates: _this->egl_data->egl_swapinterval is
+       ignored for now, it makes no sense to use it until async pageflips work on drm drivers. */
 
     /* Recreate the GBM / EGL surfaces if the display mode has changed */
     if (windata->egl_surface_dirty) {
         KMSDRM_CreateSurfaces(_this, window);
+        /* Do this later, when a fb_id is obtained. */
+        crtc_setup_pending = SDL_TRUE;
     }
 
-    /* Wait for confirmation that the next front buffer has been flipped, at which
-       point the previous front buffer can be released */
-    timeout = 0;
-    if (_this->egl_data->egl_swapinterval == 1) {
-        timeout = -1;
-    }
-    if (!KMSDRM_WaitPageFlip(_this, windata, timeout)) {
-        return 0;
-    }
+    if (windata->double_buffer) {
+        /* Use a double buffering scheme, independently of the number of buffers that the GBM surface has,
+           (number of buffers on the GBM surface depends on the implementation).
+           Double buffering (instead of triple) is achieved by waiting for synchronous pageflip to complete
+           inmediately after the pageflip is issued. That way, in the end of this function only two buffers
+           are needed: a buffer that is available to be picked again by EGL as a backbuffer to draw on it,
+           and the new front buffer that has just been set up.
+
+           Since programmer has no control over the number of buffers of the GBM surface, wait for pageflip
+           is done inmediately after issuing pageflip, and so a double-buffer scheme is achieved. */
+
+        /* Ask EGL to mark the current back buffer to become the next front buffer. 
+           That will happen when a pageflip is issued, and the next vsync arrives (sync flip)
+           or ASAP (async flip). */
+        if (!(_this->egl_data->eglSwapBuffers(_this->egl_data->egl_display, windata->egl_surface))) {
+            SDL_LogError(SDL_LOG_CATEGORY_VIDEO, "eglSwapBuffers failed.");
+            return 0;
+        }
 
-    /* Release the previous front buffer */
-    if (windata->curr_bo) {
-        KMSDRM_gbm_surface_release_buffer(windata->gs, windata->curr_bo);
-        /* SDL_LogDebug(SDL_LOG_CATEGORY_VIDEO, "Released GBM surface %p", (void *)windata->curr_bo); */
-        windata->curr_bo = NULL;
-    }
+        /* Get a handler to the buffer that is marked to become the next front buffer, and lock it
+           so it can not be chosen by EGL as a back buffer. */
+        windata->next_bo = KMSDRM_gbm_surface_lock_front_buffer(windata->gs);
+        if (!windata->next_bo) {
+            SDL_LogError(SDL_LOG_CATEGORY_VIDEO, "Could not lock GBM surface front buffer");
+            return 0;
+        /* } else {
+            SDL_LogDebug(SDL_LOG_CATEGORY_VIDEO, "Locked GBM surface %p", (void *)windata->next_bo); */
+        }
 
-    windata->curr_bo = windata->next_bo;
+        /* Issue synchronous pageflip: drmModePageFlip() NEVER blocks, synchronous here means that it
+           will be done on next VBLANK, not ASAP. And return to program loop inmediately. */
 
-    /* Make the current back buffer the next front buffer */
-    if (!(_this->egl_data->eglSwapBuffers(_this->egl_data->egl_display, windata->egl_surface))) {
-        SDL_LogError(SDL_LOG_CATEGORY_VIDEO, "eglSwapBuffers failed.");
-        return 0;
-    }
+        fb_info = KMSDRM_FBFromBO(_this, windata->next_bo);
+        if (!fb_info) {
+            return 0;
+        }
 
-    /* Lock the next front buffer so it can't be allocated as a back buffer */
-    windata->next_bo = KMSDRM_gbm_surface_lock_front_buffer(windata->gs);
-    if (!windata->next_bo) {
-        SDL_LogError(SDL_LOG_CATEGORY_VIDEO, "Could not lock GBM surface front buffer");
-        return 0;
-    /* } else {
-        SDL_LogDebug(SDL_LOG_CATEGORY_VIDEO, "Locked GBM surface %p", (void *)windata->next_bo); */
-    }
+        /* When needed, this is done once we have the needed fb_id, not before. */
+        if (crtc_setup_pending) {
+	    if (KMSDRM_drmModeSetCrtc(viddata->drm_fd, dispdata->crtc_id, fb_info->fb_id, 0,
+					0, &dispdata->conn->connector_id, 1, &dispdata->mode)) {
+		SDL_LogError(SDL_LOG_CATEGORY_VIDEO, "Could not configure CRTC on video mode setting.");
+	    }
+            crtc_setup_pending = SDL_FALSE;
+        }
 
-    fb_info = KMSDRM_FBFromBO(_this, windata->next_bo);
-    if (!fb_info) {
-        return 0;
-    }
+        if (!KMSDRM_drmModePageFlip(viddata->drm_fd, dispdata->crtc_id, fb_info->fb_id,
+                                    DRM_MODE_PAGE_FLIP_EVENT, &windata->waiting_for_flip)) {
+            windata->waiting_for_flip = SDL_TRUE;
+        } else {
+            SDL_LogError(SDL_LOG_CATEGORY_VIDEO, "Could not issue pageflip");
+        }
 
-    if (!windata->curr_bo) {
-        /* On the first swap, immediately present the new front buffer. Before
-           drmModePageFlip can be used the CRTC has to be configured to use
-           the current connector and mode with drmModeSetCrtc */
-        ret = KMSDRM_drmModeSetCrtc(viddata->drm_fd, dispdata->crtc_id, fb_info->fb_id, 0,
-                                    0, &dispdata->conn->connector_id, 1, &dispdata->mode);
+        /* Since issued pageflips are always synchronous (ASYNC dont currently work), these pageflips
+           will happen at next vsync, so in practice waiting for vsync is being done here. */ 
+        if (!KMSDRM_WaitPageFlip(_this, windata, -1)) {
+            SDL_LogError(SDL_LOG_CATEGORY_VIDEO, "Error waiting for pageflip event");
+            return 0;
+        }
 
-        if (ret) {
-          SDL_LogError(SDL_LOG_CATEGORY_VIDEO, "Could not configure CRTC");
+        /* Return the previous front buffer to the available buffer pool of the GBM surface,
+           so it can be chosen again by EGL as the back buffer for drawing into it. */
+        if (windata->curr_bo) {
+            KMSDRM_gbm_surface_release_buffer(windata->gs, windata->curr_bo);
+            /* SDL_LogDebug(SDL_LOG_CATEGORY_VIDEO, "Released GBM surface buffer %p", (void *)windata->curr_bo); */
+            windata->curr_bo = NULL;
         }
+
+        /* Take note of the current front buffer, so it can be freed next time this function is called. */
+        windata->curr_bo = windata->next_bo;
     } else {
-        /* On subsequent swaps, queue the new front buffer to be flipped during
-           the next vertical blank */
-        ret = KMSDRM_drmModePageFlip(viddata->drm_fd, dispdata->crtc_id, fb_info->fb_id,
-                                     DRM_MODE_PAGE_FLIP_EVENT, &windata->waiting_for_flip);
-        /* SDL_LogDebug(SDL_LOG_CATEGORY_VIDEO, "drmModePageFlip(%d, %u, %u, DRM_MODE_PAGE_FLIP_EVENT, &windata->waiting_for_flip)",
-            viddata->drm_fd, displaydata->crtc_id, fb_info->fb_id); */
-
-        if (_this->egl_data->egl_swapinterval == 1) {
-            if (ret == 0) {
-                windata->waiting_for_flip = SDL_TRUE;
-            } else {
-                SDL_LogError(SDL_LOG_CATEGORY_VIDEO, "Could not queue pageflip: %d", ret);
-            }
+        /* Triple buffering requires waiting for last pageflip upon entering instead of waiting at the end,
+           and issuing the next pageflip at the end, thus allowing the program loop to run 
+           while the issued pageflip arrives (at next VBLANK, since ONLY synchronous pageflips are possible).
+           In a game context, this means that the player can be doing inputs before seeing the last
+           completed frame, causing "input lag" that is known to plage other APIs and backends.
+           Triple buffering requires the use of three different buffers at the end of this function:
+           1- the front buffer which is on screen,
+           2- the back buffer wich is ready to be flipped (a pageflip has been issued on it, which has yet to complete)
+           3- a third buffer that can be used by EGL to draw while the previously issued pageflip arrives
+              (should not put back the previous front buffer into the free buffers pool of the
+              GBM surface until that happens).
+           If the implementation only has two buffers for the GBM surface, this would behave like a double buffer.
+        */
+        
+        /* Wait for previously issued pageflip to complete. */
+        if (!KMSDRM_WaitPageFlip(_this, windata, -1)) {
+            SDL_LogError(SDL_LOG_CATEGORY_VIDEO, "Error waiting for pageflip event");
+            return 0;
+        }
+
+        /* Free the previous front buffer so EGL can pick it again as back buffer.*/
+        if (windata->curr_bo) {
+            KMSDRM_gbm_surface_release_buffer(windata->gs, windata->curr_bo);
+            /* SDL_LogDebug(SDL_LOG_CATEGORY_VIDEO, "Released GBM surface buffer %p", (void *)windata->curr_bo); */
+            windata->curr_bo = NULL;
+        }
+
+        /* Ask EGL to mark the current back buffer to become the next front buffer. 
+           That will happen when a pageflip is issued, and the next vsync arrives (sync flip)
+           or ASAP (async flip). */
+        if (!(_this->egl_data->eglSwapBuffers(_this->egl_data->egl_display, windata->egl_surface))) {
+            SDL_LogError(SDL_LOG_CATEGORY_VIDEO, "eglSwapBuffers failed.");
+            return 0;
         }
 
-        /* Wait immediately for vsync (as if we only had two buffers), for low input-lag scenarios.
-           Run your SDL2 program with "SDL_KMSDRM_DOUBLE_BUFFER=1 <program_name>" to enable this. */
-        if (_this->egl_data->egl_swapinterval == 1 && windata->double_buffer) {
-            KMSDRM_WaitPageFlip(_this, windata, -1);
+        /* Take note of the current front buffer, so it can be freed next time this function is called. */
+        windata->curr_bo = windata->next_bo;
+
+        /* Get a handler to the buffer that is marked to become the next front buffer, and lock it
+           so it can not be chosen by EGL as a back buffer. */
+        windata->next_bo = KMSDRM_gbm_surface_lock_front_buffer(windata->gs);
+        if (!windata->next_bo) {
+             SDL_LogError(SDL_LOG_CATEGORY_VIDEO, "Could not lock GBM surface front buffer");
+             return 0;
+	/* } else {
+             SDL_LogDebug(SDL_LOG_CATEGORY_VIDEO, "Locked GBM surface %p", (void *)windata->next_bo); */
+        }
+
+        /* Issue synchronous pageflip: drmModePageFlip() NEVER blocks, synchronous here means that it
+           will be done on next VBLANK, not ASAP. And return to program loop inmediately. */
+        fb_info = KMSDRM_FBFromBO(_this, windata->next_bo);
+        if (!fb_info) {
+            return 0;
+        }
+
+        /* When needed, this is done once we have the needed fb_id, not before. */
+        if (crtc_setup_pending) {
+	    if (KMSDRM_drmModeSetCrtc(viddata->drm_fd, dispdata->crtc_id, fb_info->fb_id, 0,
+					0, &dispdata->conn->connector_id, 1, &dispdata->mode)) {
+		SDL_LogError(SDL_LOG_CATEGORY_VIDEO, "Could not configure CRTC on video mode setting.");
+	    }
+            crtc_setup_pending = SDL_FALSE;
+        }
+
+
+        if (!KMSDRM_drmModePageFlip(viddata->drm_fd, dispdata->crtc_id, fb_info->fb_id,
+                                    DRM_MODE_PAGE_FLIP_EVENT, &windata->waiting_for_flip)) {
+            windata->waiting_for_flip = SDL_TRUE;
+        } else {
+            SDL_LogError(SDL_LOG_CATEGORY_VIDEO, "Could not issue pageflip");
         }
     }
 

src/video/kmsdrm/SDL_kmsdrmvideo.c

@@ -188,7 +188,7 @@ KMSDRM_CreateDevice(int devindex)
 
     device->driverdata = viddata;
 
-    /* Setup all functions which we can handle */
+    /* Setup all functions that can be handled from this backend. */
     device->VideoInit = KMSDRM_VideoInit;
     device->VideoQuit = KMSDRM_VideoQuit;
     device->GetDisplayModes = KMSDRM_GetDisplayModes;
@@ -303,6 +303,12 @@ KMSDRM_FBFromBO(_THIS, struct gbm_bo *bo)
 static void
 KMSDRM_FlipHandler(int fd, unsigned int frame, unsigned int sec, unsigned int usec, void *data)
 {
+    /* If the data pointer received here is the same passed as the user_data in drmModePageFlip()
+       then this is the event handler for the pageflip that was issued on drmPageFlip(): got here 
+       because of that precise page flip, the while loop gets broken here because of the right event.
+       This knowledge will allow handling different issued pageflips if sometime in the future 
+       managing different CRTCs in SDL2 is needed, for example (synchronous pageflips happen on vblank 
+       and vblank is a CRTC thing). */
     *((SDL_bool *) data) = SDL_FALSE;
 }
 
@@ -331,8 +337,12 @@ KMSDRM_WaitPageFlip(_THIS, SDL_WindowData *windata, int timeout) {
             return SDL_FALSE;
         }
 
+        /* Is the fd readable? Thats enough to call drmHandleEvent() on it. */
         if (pfd.revents & POLLIN) {
-            /* Page flip? If so, drmHandleEvent will unset windata->waiting_for_flip */
+            /* Page flip? ONLY if the event that made the fd readable (=POLLIN state)
+               is a page flip, will drmHandleEvent call page_flip_handler, which will break the loop.
+               The drmHandleEvent() and subsequent page_flip_handler calls are both synchronous (blocking),
+               nothing runs on a different thread, so no need to protect waiting_for_flip access with mutexes. */
             KMSDRM_drmHandleEvent(viddata->drm_fd, &ev);
         } else {
             /* Timed out and page flip didn't happen */
@@ -776,7 +786,7 @@ KMSDRM_CreateWindow(_THIS, SDL_Window * window)
     /* Maybe you didn't ask for a fullscreen OpenGL window, but that's what you get */
     window->flags |= (SDL_WINDOW_FULLSCREEN | SDL_WINDOW_OPENGL);
 
-    /* In case we want low-latency, double-buffer video, we take note here */
+    /* In case low-latency is wanted, double-buffered video will be used. We take note here */
     windata->double_buffer = SDL_FALSE;
 
     if (SDL_GetHintBoolean(SDL_HINT_VIDEO_DOUBLE_BUFFER, SDL_FALSE)) {