From d43e666eedfb711584f5b2634bfb078957e6c8a9 Mon Sep 17 00:00:00 2001 From: Manuel Alfayate Corchete Date: Tue, 25 Aug 2020 04:05:36 +0200 Subject: [PATCH] kmsdrm: Buffer management refactoring. Fixes for compatibility with more video drivers. --- src/video/kmsdrm/SDL_kmsdrmmouse.c | 5 +- src/video/kmsdrm/SDL_kmsdrmopengles.c | 35 +++++- src/video/kmsdrm/SDL_kmsdrmvideo.c | 158 +++++++++++++++++++------- src/video/kmsdrm/SDL_kmsdrmvideo.h | 22 +++- 4 files changed, 168 insertions(+), 52 deletions(-) diff --git a/src/video/kmsdrm/SDL_kmsdrmmouse.c b/src/video/kmsdrm/SDL_kmsdrmmouse.c index dc9e37e59..2c6c5defe 100644 --- a/src/video/kmsdrm/SDL_kmsdrmmouse.c +++ b/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."); diff --git a/src/video/kmsdrm/SDL_kmsdrmopengles.c b/src/video/kmsdrm/SDL_kmsdrmopengles.c index 5588dbf4b..d79608cf4 100644 --- a/src/video/kmsdrm/SDL_kmsdrmopengles.c +++ b/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; } diff --git a/src/video/kmsdrm/SDL_kmsdrmvideo.c b/src/video/kmsdrm/SDL_kmsdrmvideo.c index 768b9affd..00b954ae3 100644 --- a/src/video/kmsdrm/SDL_kmsdrmvideo.c +++ b/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. */ + /*******************************************************************************/ - drm_atomic_set_plane_props(&info); - drm_atomic_commit(_this, SDL_TRUE); + dispdata->old_gs = windata->gs; + dispdata->old_bo = windata->bo; + dispdata->old_next_bo = windata->next_bo; + dispdata->old_egl_surface = windata->egl_surface; - if (windata->bo) { - KMSDRM_gbm_surface_release_buffer(windata->gs, windata->bo); - windata->bo = NULL; - } + /* 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; +} - if (windata->next_bo) { - KMSDRM_gbm_surface_release_buffer(windata->gs, windata->next_bo); - windata->next_bo = NULL; - } +void +KMSDRM_DestroyOldSurfaces(_THIS) +{ + SDL_DisplayData *dispdata = (SDL_DisplayData *)SDL_GetDisplayDriverData(0); #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; + /* 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->gs) { - KMSDRM_gbm_surface_destroy(windata->gs); - windata->gs = 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 (dispdata->old_next_bo) { + KMSDRM_gbm_surface_release_buffer(dispdata->old_gs, dispdata->old_next_bo); + dispdata->old_next_bo = NULL; + } + + if (dispdata->old_gs) { + KMSDRM_gbm_surface_destroy(dispdata->old_gs); + dispdata->old_gs = NULL; } } @@ -814,19 +832,12 @@ 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; -#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 + 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."); + } windata->gs = KMSDRM_gbm_surface_create(viddata->gbm_dev, width, height, surface_fmt, surface_flags); @@ -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; diff --git a/src/video/kmsdrm/SDL_kmsdrmvideo.h b/src/video/kmsdrm/SDL_kmsdrmvideo.h index c4c3a4006..102c2fdd2 100644 --- a/src/video/kmsdrm/SDL_kmsdrmvideo.h +++ b/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 */