kmsdrm: Buffer management refactoring. Fixes for compatibility with more video drivers.

src/video/kmsdrm/SDL_kmsdrmmouse.c

@@ -263,6 +263,7 @@ KMSDRM_ShowCursor(SDL_Cursor * cursor)
             if (dispdata && dispdata->cursor_plane) {
                 info.plane = dispdata->cursor_plane; /* The rest of the members are zeroed. */
                 ret = drm_atomic_set_plane_props(&info); 
+                //drm_atomic_commit(display->device, SDL_TRUE);
                 if (ret) {
                     SDL_SetError("Could not hide current cursor.");
                     return ret;
@@ -313,6 +314,7 @@ KMSDRM_ShowCursor(SDL_Cursor * cursor)
     info.crtc_h = curdata->h; 
 
     ret = drm_atomic_set_plane_props(&info);
+    //drm_atomic_commit(display->device, SDL_TRUE);
 
     if (ret) {
         SDL_SetError("KMSDRM_ShowCursor failed.");
@@ -341,7 +343,6 @@ KMSDRM_FreeCursor(SDL_Cursor * cursor)
 
     if (cursor) {
         curdata = (KMSDRM_CursorData *) cursor->driverdata;
-        video = curdata->video;
         if (video && curdata->bo && curdata->plane) {
             info.plane = curdata->plane; /* The other members are zeroed. */
 	    drm_atomic_set_plane_props(&info);
@@ -381,7 +382,6 @@ KMSDRM_WarpMouseGlobal(int x, int y)
 	    int ret;
 
             ret = drm_atomic_movecursor(curdata, x, y);
-            //ret = drm_atomic_commit(curdata->video, SDL_TRUE);
 
 	    if (ret) {
 		SDL_SetError("drm_atomic_movecursor() failed.");
@@ -449,7 +449,6 @@ KMSDRM_MoveCursor(SDL_Cursor * cursor)
            cursor movement request, but it cripples the movement to 30FPS, so a future solution
            is needed. SDLPoP "QUIT?" menu is an example of this situation. */
         ret = drm_atomic_movecursor(curdata, mouse->x, mouse->y);
-        //ret = drm_atomic_commit(curdata->video, SDL_TRUE);
 
 	if (ret) {
 	    SDL_SetError("drm_atomic_movecursor() failed.");

src/video/kmsdrm/SDL_kmsdrmopengles.c

@@ -82,6 +82,23 @@ KMSDRM_GLES_SwapWindow(_THIS, SDL_Window * window)
     KMSDRM_PlaneInfo info = {0};
     int ret;
 
+    /* Do we have a pending old surfaces destruction? */
+    if (dispdata->destroy_surfaces_pending == SDL_TRUE) {
+
+        /* Take note that we have not pending old surfaces destruction.
+           Do it ASAP, DON'T GO into SwapWindowDB() with it enabled or
+           we will enter recursivity! */
+        dispdata->destroy_surfaces_pending = SDL_FALSE;
+
+        /* Do blocking pageflip, to be sure it's atomic commit is completed
+           before destroying the old surfaces and buffers. */
+        KMSDRM_GLES_SwapWindowDB(_this, window);
+
+        KMSDRM_DestroyOldSurfaces(_this);
+
+        return 0;
+    }
+
     /*************************************************************************/
     /* Block for telling KMS to wait for GPU rendering of the current frame  */
     /* before applying the KMS changes requested in the atomic ioctl.        */
@@ -181,10 +198,12 @@ KMSDRM_GLES_SwapWindowDB(_THIS, SDL_Window * window)
     KMSDRM_PlaneInfo info = {0};
     int ret;
 
-    /* In double-buffer mode, atomic commit will always be synchronous/blocking (ie: won't return until
-       the requested changes are done).
-       Also, there's no need to fence KMS or the GPU, because we won't be entering game loop again
-       (hence not building or executing a new cmdstring) until pageflip is done. */
+    /****************************************************************************************************/
+    /* In double-buffer mode, atomic commit will always be synchronous/blocking (ie: won't return until */
+    /* the requested changes are really done).                                                          */
+    /* Also, there's no need to fence KMS or the GPU, because we won't be entering game loop again      */
+    /* (hence not building or executing a new cmdstring) until pageflip is done.                        */
+    /****************************************************************************************************/ 
 
     /* Mark, at EGL level, the buffer that we want to become the new front buffer.
        However, it won't really happen until we request a pageflip at the KMS level and it completes. */
@@ -231,6 +250,14 @@ KMSDRM_GLES_SwapWindowDB(_THIS, SDL_Window * window)
     /* Take note of current front buffer, so we can free it next time we come here. */
     windata->bo = windata->next_bo;
 
+    /* Do we have a pending old surfaces destruction? */
+    if (dispdata->destroy_surfaces_pending == SDL_TRUE) {
+        /* We have just done a blocking pageflip to the new buffers already,
+           so just do what you are here for... */
+        KMSDRM_DestroyOldSurfaces(_this);
+        dispdata->destroy_surfaces_pending = SDL_FALSE;
+    }
+
     return ret;
 }
 

src/video/kmsdrm/SDL_kmsdrmvideo.c

@@ -149,7 +149,6 @@ get_driindex(void)
 
 #define VOID2U64(x) ((uint64_t)(unsigned long)(x))
 
-#if 0
 static int add_connector_property(drmModeAtomicReq *req, struct connector *connector,
 					const char *name, uint64_t value)
 {
@@ -170,7 +169,6 @@ static int add_connector_property(drmModeAtomicReq *req, struct connector *conne
 
     return KMSDRM_drmModeAtomicAddProperty(req, connector->connector->connector_id, prop_id, value);
 }
-#endif
 
 static int add_crtc_property(drmModeAtomicReq *req, struct crtc *crtc,
 				const char *name, uint64_t value)
@@ -526,7 +524,7 @@ int drm_atomic_commit(_THIS, SDL_bool blocking)
     if (ret) {
         SDL_SetError("Atomic commit failed, returned %d.", ret);
         /* Uncomment this for fast-debugging */
-        //printf("ATOMIC COMMIT FAILED: %d.\n", ret);
+        printf("ATOMIC COMMIT FAILED: %d.\n", ret);
 	goto out;
     }
 
@@ -761,44 +759,64 @@ KMSDRM_FBFromBO(_THIS, struct gbm_bo *bo)
 /* _this is a SDL_VideoDevice *                                              */
 /*****************************************************************************/
 
+/* Just backup the GBM/EGL surfaces pinters, and buffers pointers,
+   because we are not destroying them until we get a buffer of the new GBM
+   surface so we can set the FB_ID of the PRIMARY plane to it.
+   That will happen in SwapWindow(), when we call lock_front_buffer()
+   on the new GBM surface, so that's where we can destroy the old buffers.
+   THE IDEA IS THAT THE PRIMARY PLANE IS NEVER LEFT WITHOUT A VALID BUFFER
+   TO READ, AND ONLY SET IT'S FB_ID/CRTC_ID PROPS TO 0 WHEN PROGRAM IS QUITTING.*/
 static void
-KMSDRM_DestroySurfaces(_THIS, SDL_Window * window)
+KMSDRM_SetPendingSurfacesDestruction(_THIS, SDL_Window * window)
 {
     SDL_WindowData *windata = (SDL_WindowData *)window->driverdata;
     SDL_DisplayData *dispdata = (SDL_DisplayData *) SDL_GetDisplayForWindow(window)->driverdata;
 
-    /* CAUTION: Before destroying the GBM ane EGL surfaces, we must disconnect
-       the display plane from the GBM surface buffer it's reading by setting 
-       it's CRTC_ID and FB_ID props to 0.
-    */
-    KMSDRM_PlaneInfo info = {0};
-    info.plane = dispdata->display_plane;
+    /*******************************************************************************/
+    /* Backup the pointers to the GBM/EGL surfaces and buffers we want to destroy, */
+    /* so we can destroy them later, even after destroying the SDL_Window.         */
+    /* DO NOT store the old ponters in windata, because it's freed when the window */
+    /* is destroyed.                                                               */
+    /*******************************************************************************/
+
+    dispdata->old_gs = windata->gs;
+    dispdata->old_bo = windata->bo;
+    dispdata->old_next_bo = windata->next_bo;
+    dispdata->old_egl_surface = windata->egl_surface;
+
+    /* Take note that we have yet to destroy the old surfaces.
+       We will do so in SwapWindow(), once we have the new
+       surfaces and buffers available for the PRIMARY plane. */
+    dispdata->destroy_surfaces_pending = SDL_TRUE;
+}
 
-    drm_atomic_set_plane_props(&info);
-    drm_atomic_commit(_this, SDL_TRUE);
+void
+KMSDRM_DestroyOldSurfaces(_THIS)
+{
+    SDL_DisplayData *dispdata = (SDL_DisplayData *)SDL_GetDisplayDriverData(0);
 
-    if (windata->bo) {
-       KMSDRM_gbm_surface_release_buffer(windata->gs, windata->bo);
-        windata->bo = NULL;
+#if SDL_VIDEO_OPENGL_EGL
+    /* Destroy the old EGL surface. */
+    if (dispdata->old_egl_surface != EGL_NO_SURFACE) {
+        SDL_EGL_DestroySurface(_this, dispdata->old_egl_surface);
+        dispdata->old_egl_surface = EGL_NO_SURFACE;
     }
+#endif
 
-    if (windata->next_bo) {
-        KMSDRM_gbm_surface_release_buffer(windata->gs, windata->next_bo);
-        windata->next_bo = NULL;
+    /* Destroy the old GBM surface and buffers. */
+    if (dispdata->old_bo) {
+       KMSDRM_gbm_surface_release_buffer(dispdata->old_gs, dispdata->old_bo);
+	dispdata->old_bo = NULL;
     }
 
-#if SDL_VIDEO_OPENGL_EGL
-    SDL_EGL_MakeCurrent(_this, EGL_NO_SURFACE, EGL_NO_CONTEXT);
-
-    if (windata->egl_surface != EGL_NO_SURFACE) {
-        SDL_EGL_DestroySurface(_this, windata->egl_surface);
-        windata->egl_surface = EGL_NO_SURFACE;
+    if (dispdata->old_next_bo) {
+	KMSDRM_gbm_surface_release_buffer(dispdata->old_gs, dispdata->old_next_bo);
+	dispdata->old_next_bo = NULL;
     }
-#endif
 
-    if (windata->gs) {
-        KMSDRM_gbm_surface_destroy(windata->gs);
-        windata->gs = NULL;
+    if (dispdata->old_gs) {
+	KMSDRM_gbm_surface_destroy(dispdata->old_gs);
+	dispdata->old_gs = NULL;
     }
 }
 
@@ -814,20 +832,13 @@ KMSDRM_CreateSurfaces(_THIS, SDL_Window * window)
     Uint32 surface_flags = GBM_BO_USE_SCANOUT | GBM_BO_USE_RENDERING;
 #if SDL_VIDEO_OPENGL_EGL
     EGLContext egl_context;
+    SDL_EGL_SetRequiredVisualId(_this, surface_fmt);
+    egl_context = (EGLContext)SDL_GL_GetCurrentContext();
 #endif
-
-    /* Always try to destroy previous surfaces before creating new ones. */
-    KMSDRM_DestroySurfaces(_this, window);
-
     if (!KMSDRM_gbm_device_is_format_supported(viddata->gbm_dev, surface_fmt, surface_flags)) {
         SDL_LogWarn(SDL_LOG_CATEGORY_VIDEO, "GBM surface format not supported. Trying anyway.");
     }
 
-#if SDL_VIDEO_OPENGL_EGL
-    SDL_EGL_SetRequiredVisualId(_this, surface_fmt);
-    egl_context = (EGLContext)SDL_GL_GetCurrentContext();
-#endif
-
     windata->gs = KMSDRM_gbm_surface_create(viddata->gbm_dev, width, height, surface_fmt, surface_flags);
 
     if (!windata->gs) {
@@ -1123,7 +1134,72 @@ KMSDRM_VideoQuit(_THIS)
 {
     SDL_VideoData *viddata = ((SDL_VideoData *)_this->driverdata);
     SDL_DisplayData *dispdata = (SDL_DisplayData *)SDL_GetDisplayDriverData(0);
-    SDL_LogDebug(SDL_LOG_CATEGORY_VIDEO, "KMSDRM_VideoQuit()");
+    KMSDRM_PlaneInfo plane_info = {0};
+
+    /*******************************************************************************/
+    /* BLOCK 1                                                                     */
+    /* Clear Old GBM surface and KMS buffers.                                      */
+    /* We have to set the FB_ID and CRTC_ID props of the PRIMARY PLANE to ZERO     */
+    /* before destroying the GBM buffers it's reading (SDL internals have already  */
+    /* run the DestroyWindow()->SetPendingSurfacesDestruction()() sequence,        */
+    /* so we have pending the destruction of the GBM/EGL surfaces and GBM buffers).*/
+    /* But there can not be an ACTIVE CRTC without a PLANE, so we also have to     */
+    /* DEACTIVATE the CRTC, and dettach the CONNECTORs from the CRTC, all in the   */
+    /* same atomic commit in which we zero the primary plane FB_ID and CRTC_ID.    */
+    /*******************************************************************************/
+
+    /* Do we have a set of changes already in the making? If not, allocate a new one. */
+    if (!dispdata->atomic_req)
+        dispdata->atomic_req = KMSDRM_drmModeAtomicAlloc();
+#define AMDGPU  /* Use this for now, for greater compatibility! */
+#ifdef AMDGPU
+
+    /* AMDGPU won't let FBCON takeover without this. The problem is
+       that crtc->buffer_id is not guaranteed to be there... */
+    plane_info.plane = dispdata->display_plane;
+    plane_info.crtc_id = dispdata->crtc->crtc->crtc_id;
+    plane_info.fb_id = dispdata->crtc->crtc->buffer_id;
+    plane_info.src_w = dispdata->mode.hdisplay;
+    plane_info.src_h = dispdata->mode.vdisplay;
+    plane_info.crtc_w = dispdata->mode.hdisplay;
+    plane_info.crtc_h = dispdata->mode.vdisplay;
+
+#else /* This is the correct way, but wait until more chips support FB_ID/CRTC_ID to 0. */
+
+    /* This is the *right* thing to do: dettach the CONNECTOR from the CRTC,
+       deactivate the CRTC, and set the FB_ID and CRTC_ID props of the PRIMARY PLANE
+       to 0. All this is needed because there can be no active CRTC with no plane.
+       All this is done in a single blocking atomic commit before destroying the buffers
+       that the PRIMARY PLANE is reading. */
+    if (add_connector_property(dispdata->atomic_req, dispdata->connector , "CRTC_ID", 0) < 0)
+        SDL_SetError("Failed to set CONNECTOR prop CRTC_ID to zero before buffer destruction");
+
+    if (add_crtc_property(dispdata->atomic_req, dispdata->crtc , "ACTIVE", 0) < 0)
+        SDL_SetError("Failed to set CRTC prop ACTIVE to zero before buffer destruction");
+
+    /* Since we initialize plane_info to all zeros,  ALL PRIMARY PLANE props are set to 0 with this,
+       including FB_ID and CRTC_ID. Not all drivers like FB_ID and CRTC_ID to 0, but they *should*. */
+    plane_info.plane = dispdata->display_plane;
+
+#endif
+
+    drm_atomic_set_plane_props(&plane_info);
+
+    /* ... And finally issue blocking ATOMIC commit before destroying the old buffers.
+       We get here with this destruction still pending if the program calls SDL_DestroyWindow(),
+       which in turn calls KMSDRM_SetPendingSurfacesDestruction(), and then quits instead of
+       creating a new SDL_Window, thus ending here. */
+
+    drm_atomic_commit(_this, SDL_TRUE);
+
+    if (dispdata->destroy_surfaces_pending) {
+        KMSDRM_DestroyOldSurfaces(_this);
+        dispdata->destroy_surfaces_pending = SDL_FALSE;
+    }
+
+    /************************/
+    /* BLOCK 1 ENDS.        */
+    /************************/
 
     /* Clear out the window list */
     SDL_free(viddata->windows);
@@ -1169,7 +1245,7 @@ KMSDRM_VideoQuit(_THIS)
     /* Free cursor plane (if still not freed) */
     free_plane(&dispdata->cursor_plane);
 
-    /* Destroy GBM device. GBM surface is destroyed by DestroySurfaces(). */
+    /* Destroy GBM device. GBM surface is destroyed by DestroyOldSurfaces(). */
     if (viddata->gbm_dev) {
         KMSDRM_gbm_device_destroy(viddata->gbm_dev);
         viddata->gbm_dev = NULL;
@@ -1293,7 +1369,7 @@ KMSDRM_DestroyWindow(_THIS, SDL_Window * window)
         }
     }
 
-    KMSDRM_DestroySurfaces(_this, window);
+    KMSDRM_SetPendingSurfacesDestruction(_this, window);
 
     window->driverdata = NULL;
 

src/video/kmsdrm/SDL_kmsdrmvideo.h

@@ -78,8 +78,8 @@ typedef struct SDL_DisplayData
     uint32_t atomic_flags;
 
     /* All changes will be requested via this one and only atomic request,
-       that will be sent to the kernel in the one and only atomic_commit() call
-       that takes place in SwapWindow(). */
+       that will be sent to the kernel in the one and only atomic_commit()
+       call that takes place in SwapWindow(). */
     drmModeAtomicReq *atomic_req;
     struct plane *display_plane;
     struct plane *cursor_plane;
@@ -94,6 +94,19 @@ typedef struct SDL_DisplayData
 
     EGLSyncKHR gpu_fence; /* Signaled when GPU rendering is done. */
 
+    /* Backup pointers of the GBM surfaces and buffers to be deleted after
+       SDL_Window destruction, since SDL_Window destruction causes it's 
+       driverdata pointer (windata) to be destroyed, so we have to 
+       keep them here instead. */
+    struct gbm_surface *old_gs;
+    struct gbm_bo *old_bo;
+    struct gbm_bo *old_next_bo;
+#if SDL_VIDEO_OPENGL_EGL
+    EGLSurface old_egl_surface;
+#endif
+
+    SDL_bool destroy_surfaces_pending;
+
 } SDL_DisplayData;
 
 /* Driverdata info that gives KMSDRM-side support and substance to the SDL_Window. */
@@ -102,8 +115,8 @@ typedef struct SDL_WindowData
     SDL_VideoData *viddata;
     /* SDL internals expect EGL surface to be here, and in KMSDRM the GBM surface is
        what supports the EGL surface on the driver side, so all these surfaces and buffers
-       are expected to be here, in the struct pointed by SDL_Window driverdata pointer: this one.
-       So don't try to move these to dispdata!  */
+       are expected to be here, in the struct pointed by SDL_Window driverdata pointer:
+       this one. So don't try to move these to dispdata!  */
     struct gbm_surface *gs;
     struct gbm_bo *bo;
     struct gbm_bo *next_bo;
@@ -138,6 +151,7 @@ typedef struct KMSDRM_PlaneInfo
 
 /* Helper functions */
 int KMSDRM_CreateSurfaces(_THIS, SDL_Window * window);
+void KMSDRM_DestroyOldSurfaces(_THIS);
 KMSDRM_FBInfo *KMSDRM_FBFromBO(_THIS, struct gbm_bo *bo);
 
 /* Atomic functions that are used from SDL_kmsdrmopengles.c and SDL_kmsdrmmouse.c */