From 1c5917437e2f98a05a91e97164ca8d8fb72fa8e5 Mon Sep 17 00:00:00 2001 From: Eric Curtin Date: Thu, 30 Jun 2022 13:46:06 +0100 Subject: [PATCH 1/4] Fix assumption that DRI_DEVNAME begins at 0 The existing logic of the code was to count every possible entry in KMSDRM_DRI_PATH. After this a for loop would start trying to open filename0, filename1, filename2, etc. In recent Linux kernels (say 5.18) with simpledrm, the lowest KMSDRM_DRI_DEVNAME is often /dev/dri/card1, rather than /dev/dri/card0, causing the code to fail once /dev/dri/card0 has failed to open. Running: modprobe foodrm && modprobe bardrm && rmmod foodrm before you try to run an application with SDL KMSDRM would have also made this fail. --- src/video/kmsdrm/SDL_kmsdrmvideo.c | 171 +++++++++++++---------------- 1 file changed, 74 insertions(+), 97 deletions(-) diff --git a/src/video/kmsdrm/SDL_kmsdrmvideo.c b/src/video/kmsdrm/SDL_kmsdrmvideo.c index fd3a3cd63e0..641989ceb3e 100644 --- a/src/video/kmsdrm/SDL_kmsdrmvideo.c +++ b/src/video/kmsdrm/SDL_kmsdrmvideo.c @@ -55,129 +55,106 @@ #ifdef __OpenBSD__ static SDL_bool openbsd69orgreater = SDL_FALSE; #define KMSDRM_DRI_PATH openbsd69orgreater ? "/dev/dri/" : "/dev/" -#define KMSDRM_DRI_DEVFMT openbsd69orgreater ? "%scard%d" : "%sdrm%d" #define KMSDRM_DRI_DEVNAME openbsd69orgreater ? "card" : "drm" -#define KMSDRM_DRI_DEVNAMESIZE openbsd69orgreater ? 4 : 3 -#define KMSDRM_DRI_CARDPATHFMT openbsd69orgreater ? "/dev/dri/card%d" : "/dev/drm%d" #else #define KMSDRM_DRI_PATH "/dev/dri/" -#define KMSDRM_DRI_DEVFMT "%scard%d" #define KMSDRM_DRI_DEVNAME "card" -#define KMSDRM_DRI_DEVNAMESIZE 4 -#define KMSDRM_DRI_CARDPATHFMT "/dev/dri/card%d" #endif +#define KMSDRM_DRI_CARDPATHFMT KMSDRM_DRI_PATH KMSDRM_DRI_DEVNAME "%d" +#define KMSDRM_DRI_DEVNAMESIZE sizeof(KMSDRM_DRI_DEVNAME) - 1 +#define KMSDRM_DRI_PATHSIZE sizeof(KMSDRM_DRI_PATH) - 1 + #ifndef EGL_PLATFORM_GBM_MESA #define EGL_PLATFORM_GBM_MESA 0x31D7 #endif static int -check_modestting(int devindex) +get_driindex(void) { - SDL_bool available = SDL_FALSE; - char device[512]; + int available = -ENOENT; + char device[512] = KMSDRM_DRI_PATH; int drm_fd; int i; - - SDL_snprintf(device, sizeof (device), KMSDRM_DRI_DEVFMT, KMSDRM_DRI_PATH, devindex); - - drm_fd = open(device, O_RDWR | O_CLOEXEC); - if (drm_fd >= 0) { - if (SDL_KMSDRM_LoadSymbols()) { - drmModeRes *resources = KMSDRM_drmModeGetResources(drm_fd); - if (resources) { - SDL_LogDebug(SDL_LOG_CATEGORY_VIDEO, - KMSDRM_DRI_DEVFMT - " connector, encoder and CRTC counts are: %d %d %d", - KMSDRM_DRI_PATH, devindex, - resources->count_connectors, resources->count_encoders, - resources->count_crtcs); - - if (resources->count_connectors > 0 - && resources->count_encoders > 0 - && resources->count_crtcs > 0) - { - available = SDL_FALSE; - for (i = 0; i < resources->count_connectors; i++) { - drmModeConnector *conn = KMSDRM_drmModeGetConnector(drm_fd, - resources->connectors[i]); - - if (!conn) { - continue; - } - - if (conn->connection == DRM_MODE_CONNECTED && conn->count_modes) { - if (SDL_GetHintBoolean(SDL_HINT_KMSDRM_REQUIRE_DRM_MASTER, SDL_TRUE)) { - /* Skip this device if we can't obtain DRM master */ - KMSDRM_drmSetMaster(drm_fd); - if (KMSDRM_drmAuthMagic(drm_fd, 0) == -EACCES) { + int devindex = -1; + DIR *folder = opendir(device); + if (!folder) { + SDL_SetError("Failed to open directory '%s'.\n", device); + return -ENOENT; + } + + memcpy(device + KMSDRM_DRI_PATHSIZE, KMSDRM_DRI_DEVNAME, + KMSDRM_DRI_DEVNAMESIZE); + for (struct dirent *res; (res = readdir(folder));) { + if (!memcmp(res->d_name, KMSDRM_DRI_DEVNAME, KMSDRM_DRI_DEVNAMESIZE)) { + memcpy(device + KMSDRM_DRI_PATHSIZE + KMSDRM_DRI_DEVNAMESIZE, + res->d_name + KMSDRM_DRI_DEVNAMESIZE, 5); + + drm_fd = open(device, O_RDWR | O_CLOEXEC); + if (drm_fd >= 0) { + devindex = SDL_atoi(device + KMSDRM_DRI_PATHSIZE + + KMSDRM_DRI_DEVNAMESIZE); + if (SDL_KMSDRM_LoadSymbols()) { + drmModeRes *resources = KMSDRM_drmModeGetResources(drm_fd); + if (resources) { + SDL_LogDebug(SDL_LOG_CATEGORY_VIDEO, + KMSDRM_DRI_CARDPATHFMT + " connector, encoder and CRTC counts " + "are: %d %d %d", + devindex, resources->count_connectors, + resources->count_encoders, + resources->count_crtcs); + + if (resources->count_connectors > 0 && + resources->count_encoders > 0 && + resources->count_crtcs > 0) { + available = -ENOENT; + for (i = 0; i < resources->count_connectors; i++) { + drmModeConnector *conn = + KMSDRM_drmModeGetConnector( + drm_fd, resources->connectors[i]); + + if (!conn) { continue; } - } - available = SDL_TRUE; - break; - } + if (conn->connection == DRM_MODE_CONNECTED && + conn->count_modes) { + if (SDL_GetHintBoolean( + SDL_HINT_KMSDRM_REQUIRE_DRM_MASTER, + SDL_TRUE)) { + /* Skip this device if we can't obtain + * DRM master */ + KMSDRM_drmSetMaster(drm_fd); + if (KMSDRM_drmAuthMagic(drm_fd, 0) == + -EACCES) { + continue; + } + } + + available = devindex; + break; + } - KMSDRM_drmModeFreeConnector(conn); + KMSDRM_drmModeFreeConnector(conn); + } + } + KMSDRM_drmModeFreeResources(resources); } + SDL_KMSDRM_UnloadSymbols(); } - KMSDRM_drmModeFreeResources(resources); + close(drm_fd); } - SDL_KMSDRM_UnloadSymbols(); - } - close(drm_fd); - } - - return available; -} -static int get_dricount(void) -{ - int devcount = 0; - struct dirent *res; - struct stat sb; - DIR *folder; - - if (!(stat(KMSDRM_DRI_PATH, &sb) == 0 - && S_ISDIR(sb.st_mode))) { - /*printf("The path %s cannot be opened or is not available\n", KMSDRM_DRI_PATH);*/ - return 0; - } - - if (access(KMSDRM_DRI_PATH, F_OK) == -1) { - /*printf("The path %s cannot be opened\n", KMSDRM_DRI_PATH);*/ - return 0; - } - - folder = opendir(KMSDRM_DRI_PATH); - if (folder) { - while ((res = readdir(folder))) { - int len = SDL_strlen(res->d_name); - if (len > KMSDRM_DRI_DEVNAMESIZE && SDL_strncmp(res->d_name, - KMSDRM_DRI_DEVNAME, KMSDRM_DRI_DEVNAMESIZE) == 0) { - devcount++; - } + SDL_LogDebug(SDL_LOG_CATEGORY_VIDEO, + "Failed to open KMSDRM device %s, errno: %d\n", device, + errno); } - closedir(folder); } - return devcount; -} - -static int -get_driindex(void) -{ - const int devcount = get_dricount(); - int i; + closedir(folder); - for (i = 0; i < devcount; i++) { - if (check_modestting(i)) { - return i; - } - } - - return -ENOENT; + return available; } static int From 840bc304d7a80d6b77521fe7d3fb5f3ac1f1fa13 Mon Sep 17 00:00:00 2001 From: Eric Curtin Date: Fri, 1 Jul 2022 00:15:14 +0100 Subject: [PATCH 2/4] Various changes from review - Removed newline and period from SDL error - Explicitely compare memcmp to zero (also changed to SDL_memcmp) - Changed memcpy to strncpy - Less aggressive line wrapping --- src/video/kmsdrm/SDL_kmsdrmvideo.c | 32 ++++++++++++++++-------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/src/video/kmsdrm/SDL_kmsdrmvideo.c b/src/video/kmsdrm/SDL_kmsdrmvideo.c index 641989ceb3e..045ade359a0 100644 --- a/src/video/kmsdrm/SDL_kmsdrmvideo.c +++ b/src/video/kmsdrm/SDL_kmsdrmvideo.c @@ -55,15 +55,17 @@ #ifdef __OpenBSD__ static SDL_bool openbsd69orgreater = SDL_FALSE; #define KMSDRM_DRI_PATH openbsd69orgreater ? "/dev/dri/" : "/dev/" +#define KMSDRM_DRI_PATHSIZE openbsd69orgreater ? 9 : 5 #define KMSDRM_DRI_DEVNAME openbsd69orgreater ? "card" : "drm" +#define KMSDRM_DRI_DEVNAMESIZE openbsd69orgreater ? 4 : 3 #else #define KMSDRM_DRI_PATH "/dev/dri/" +#define KMSDRM_DRI_PATHSIZE 9 #define KMSDRM_DRI_DEVNAME "card" +#define KMSDRM_DRI_DEVNAMESIZE 4 #endif #define KMSDRM_DRI_CARDPATHFMT KMSDRM_DRI_PATH KMSDRM_DRI_DEVNAME "%d" -#define KMSDRM_DRI_DEVNAMESIZE sizeof(KMSDRM_DRI_DEVNAME) - 1 -#define KMSDRM_DRI_PATHSIZE sizeof(KMSDRM_DRI_PATH) - 1 #ifndef EGL_PLATFORM_GBM_MESA #define EGL_PLATFORM_GBM_MESA 0x31D7 @@ -79,16 +81,17 @@ get_driindex(void) int devindex = -1; DIR *folder = opendir(device); if (!folder) { - SDL_SetError("Failed to open directory '%s'.\n", device); + SDL_SetError("Failed to open directory '%s'", device); return -ENOENT; } - memcpy(device + KMSDRM_DRI_PATHSIZE, KMSDRM_DRI_DEVNAME, - KMSDRM_DRI_DEVNAMESIZE); + strncpy(device + KMSDRM_DRI_PATHSIZE, KMSDRM_DRI_DEVNAME, + KMSDRM_DRI_DEVNAMESIZE + 1); for (struct dirent *res; (res = readdir(folder));) { - if (!memcmp(res->d_name, KMSDRM_DRI_DEVNAME, KMSDRM_DRI_DEVNAMESIZE)) { - memcpy(device + KMSDRM_DRI_PATHSIZE + KMSDRM_DRI_DEVNAMESIZE, - res->d_name + KMSDRM_DRI_DEVNAMESIZE, 5); + if (SDL_memcmp(res->d_name, KMSDRM_DRI_DEVNAME, + KMSDRM_DRI_DEVNAMESIZE) == 0) { + strncpy(device + KMSDRM_DRI_PATHSIZE + KMSDRM_DRI_DEVNAMESIZE, + res->d_name + KMSDRM_DRI_DEVNAMESIZE, 6); drm_fd = open(device, O_RDWR | O_CLOEXEC); if (drm_fd >= 0) { @@ -97,13 +100,12 @@ get_driindex(void) if (SDL_KMSDRM_LoadSymbols()) { drmModeRes *resources = KMSDRM_drmModeGetResources(drm_fd); if (resources) { - SDL_LogDebug(SDL_LOG_CATEGORY_VIDEO, - KMSDRM_DRI_CARDPATHFMT - " connector, encoder and CRTC counts " - "are: %d %d %d", - devindex, resources->count_connectors, - resources->count_encoders, - resources->count_crtcs); + SDL_LogDebug( + SDL_LOG_CATEGORY_VIDEO, + KMSDRM_DRI_CARDPATHFMT + " connector, encoder and CRTC counts are: %d %d %d", + devindex, resources->count_connectors, + resources->count_encoders, resources->count_crtcs); if (resources->count_connectors > 0 && resources->count_encoders > 0 && From e8894ba9371354e01e195dd4e48b561f193374a4 Mon Sep 17 00:00:00 2001 From: Eric Curtin Date: Wed, 6 Jul 2022 12:01:32 +0100 Subject: [PATCH 3/4] Various changes from review - strncpy to SDL_strlcpy - removed size hardcodings for KMSDRM_DRI_PATHSIZE and KMSDRM_DRI_DEVNAMESIZE - made all KMSDRM_DRI defines, run-time variables to reduce bugs caused by these defines being more build-time on Linux and more run-rime on OpenBSD - renamed openbsd69orgreater variable to moderndri - altered comment from "if on OpenBSD" to add difference in 6.9 --- src/video/kmsdrm/SDL_kmsdrmvideo.c | 73 ++++++++++++++++++------------ 1 file changed, 44 insertions(+), 29 deletions(-) diff --git a/src/video/kmsdrm/SDL_kmsdrmvideo.c b/src/video/kmsdrm/SDL_kmsdrmvideo.c index 045ade359a0..d85e93ebc93 100644 --- a/src/video/kmsdrm/SDL_kmsdrmvideo.c +++ b/src/video/kmsdrm/SDL_kmsdrmvideo.c @@ -53,19 +53,16 @@ #include #ifdef __OpenBSD__ -static SDL_bool openbsd69orgreater = SDL_FALSE; -#define KMSDRM_DRI_PATH openbsd69orgreater ? "/dev/dri/" : "/dev/" -#define KMSDRM_DRI_PATHSIZE openbsd69orgreater ? 9 : 5 -#define KMSDRM_DRI_DEVNAME openbsd69orgreater ? "card" : "drm" -#define KMSDRM_DRI_DEVNAMESIZE openbsd69orgreater ? 4 : 3 +static SDL_bool moderndri = SDL_FALSE; #else -#define KMSDRM_DRI_PATH "/dev/dri/" -#define KMSDRM_DRI_PATHSIZE 9 -#define KMSDRM_DRI_DEVNAME "card" -#define KMSDRM_DRI_DEVNAMESIZE 4 +static SDL_bool moderndri = SDL_TRUE; #endif -#define KMSDRM_DRI_CARDPATHFMT KMSDRM_DRI_PATH KMSDRM_DRI_DEVNAME "%d" +static char kmsdrm_dri_path[16] = ""; +static int kmsdrm_dri_pathsize = 0; +static char kmsdrm_dri_devname[8] = ""; +static int kmsdrm_dri_devnamesize = 0; +static char kmsdrm_dri_cardpath[32] = ""; #ifndef EGL_PLATFORM_GBM_MESA #define EGL_PLATFORM_GBM_MESA 0x31D7 @@ -75,37 +72,40 @@ static int get_driindex(void) { int available = -ENOENT; - char device[512] = KMSDRM_DRI_PATH; + char device[32]; int drm_fd; int i; int devindex = -1; - DIR *folder = opendir(device); + DIR *folder; + + SDL_strlcpy(device, kmsdrm_dri_path, kmsdrm_dri_pathsize + 1); + folder = opendir(device); if (!folder) { SDL_SetError("Failed to open directory '%s'", device); return -ENOENT; } - strncpy(device + KMSDRM_DRI_PATHSIZE, KMSDRM_DRI_DEVNAME, - KMSDRM_DRI_DEVNAMESIZE + 1); + SDL_strlcpy(device + kmsdrm_dri_pathsize, kmsdrm_dri_devname, + kmsdrm_dri_devnamesize + 1); for (struct dirent *res; (res = readdir(folder));) { - if (SDL_memcmp(res->d_name, KMSDRM_DRI_DEVNAME, - KMSDRM_DRI_DEVNAMESIZE) == 0) { - strncpy(device + KMSDRM_DRI_PATHSIZE + KMSDRM_DRI_DEVNAMESIZE, - res->d_name + KMSDRM_DRI_DEVNAMESIZE, 6); + if (SDL_memcmp(res->d_name, kmsdrm_dri_devname, + kmsdrm_dri_devnamesize) == 0) { + SDL_strlcpy(device + kmsdrm_dri_pathsize + kmsdrm_dri_devnamesize, + res->d_name + kmsdrm_dri_devnamesize, 6); drm_fd = open(device, O_RDWR | O_CLOEXEC); if (drm_fd >= 0) { - devindex = SDL_atoi(device + KMSDRM_DRI_PATHSIZE + - KMSDRM_DRI_DEVNAMESIZE); + devindex = SDL_atoi(device + kmsdrm_dri_pathsize + + kmsdrm_dri_devnamesize); if (SDL_KMSDRM_LoadSymbols()) { drmModeRes *resources = KMSDRM_drmModeGetResources(drm_fd); if (resources) { - SDL_LogDebug( - SDL_LOG_CATEGORY_VIDEO, - KMSDRM_DRI_CARDPATHFMT - " connector, encoder and CRTC counts are: %d %d %d", - devindex, resources->count_connectors, - resources->count_encoders, resources->count_crtcs); + SDL_LogDebug(SDL_LOG_CATEGORY_VIDEO, + "%s%d connector, encoder and CRTC counts are: %d %d %d", + kmsdrm_dri_cardpath, devindex, + resources->count_connectors, + resources->count_encoders, + resources->count_crtcs); if (resources->count_connectors > 0 && resources->count_encoders > 0 && @@ -172,10 +172,24 @@ KMSDRM_Available(void) if (!(uname(&nameofsystem) < 0)) { releaseversion = SDL_atof(nameofsystem.release); if (releaseversion >= 6.9) { - openbsd69orgreater = SDL_TRUE; + moderndri = SDL_TRUE; } } #endif + + if (moderndri) { + SDL_strlcpy(kmsdrm_dri_path, "/dev/dri/", sizeof("/dev/dri/") + 1); + SDL_strlcpy(kmsdrm_dri_devname, "card", sizeof("card") + 1); + } else { + SDL_strlcpy(kmsdrm_dri_path, "/dev/", sizeof("/dev/") + 1); + SDL_strlcpy(kmsdrm_dri_devname, "drm", sizeof("drm") + 1); + } + + kmsdrm_dri_pathsize = SDL_strlen(kmsdrm_dri_path); + kmsdrm_dri_devnamesize = SDL_strlen(kmsdrm_dri_devname); + SDL_snprintf(kmsdrm_dri_cardpath, sizeof(kmsdrm_dri_cardpath), "%s%s", + kmsdrm_dri_path, kmsdrm_dri_devname); + ret = get_driindex(); if (ret >= 0) return 1; @@ -711,8 +725,9 @@ KMSDRM_InitDisplays (_THIS) { int ret = 0; int i; - /* Open /dev/dri/cardNN (/dev/drmN if on OpenBSD) */ - SDL_snprintf(viddata->devpath, sizeof(viddata->devpath), KMSDRM_DRI_CARDPATHFMT, viddata->devindex); + /* Open /dev/dri/cardNN (/dev/drmN if on OpenBSD version less than 6.9) */ + SDL_snprintf(viddata->devpath, sizeof(viddata->devpath), "%s%d", + kmsdrm_dri_cardpath, viddata->devindex); SDL_LogDebug(SDL_LOG_CATEGORY_VIDEO, "Opening device %s", viddata->devpath); viddata->drm_fd = open(viddata->devpath, O_RDWR | O_CLOEXEC); From 7e61a812465df3c1e29bab4acafcd086d7a813ba Mon Sep 17 00:00:00 2001 From: Eric Curtin Date: Wed, 6 Jul 2022 16:06:32 +0100 Subject: [PATCH 4/4] Various changes from review - Use max size of destination, rather than max size of source - Less hardcodings --- src/video/kmsdrm/SDL_kmsdrmvideo.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/src/video/kmsdrm/SDL_kmsdrmvideo.c b/src/video/kmsdrm/SDL_kmsdrmvideo.c index d85e93ebc93..07a15ae6718 100644 --- a/src/video/kmsdrm/SDL_kmsdrmvideo.c +++ b/src/video/kmsdrm/SDL_kmsdrmvideo.c @@ -58,11 +58,11 @@ static SDL_bool moderndri = SDL_FALSE; static SDL_bool moderndri = SDL_TRUE; #endif -static char kmsdrm_dri_path[16] = ""; +static char kmsdrm_dri_path[16]; static int kmsdrm_dri_pathsize = 0; -static char kmsdrm_dri_devname[8] = ""; +static char kmsdrm_dri_devname[8]; static int kmsdrm_dri_devnamesize = 0; -static char kmsdrm_dri_cardpath[32] = ""; +static char kmsdrm_dri_cardpath[32]; #ifndef EGL_PLATFORM_GBM_MESA #define EGL_PLATFORM_GBM_MESA 0x31D7 @@ -72,13 +72,13 @@ static int get_driindex(void) { int available = -ENOENT; - char device[32]; + char device[sizeof(kmsdrm_dri_cardpath)]; int drm_fd; int i; int devindex = -1; DIR *folder; - SDL_strlcpy(device, kmsdrm_dri_path, kmsdrm_dri_pathsize + 1); + SDL_strlcpy(device, kmsdrm_dri_path, sizeof(device)); folder = opendir(device); if (!folder) { SDL_SetError("Failed to open directory '%s'", device); @@ -86,12 +86,14 @@ get_driindex(void) } SDL_strlcpy(device + kmsdrm_dri_pathsize, kmsdrm_dri_devname, - kmsdrm_dri_devnamesize + 1); + sizeof(device) - kmsdrm_dri_devnamesize); for (struct dirent *res; (res = readdir(folder));) { if (SDL_memcmp(res->d_name, kmsdrm_dri_devname, kmsdrm_dri_devnamesize) == 0) { SDL_strlcpy(device + kmsdrm_dri_pathsize + kmsdrm_dri_devnamesize, - res->d_name + kmsdrm_dri_devnamesize, 6); + res->d_name + kmsdrm_dri_devnamesize, + sizeof(device) - kmsdrm_dri_pathsize - + kmsdrm_dri_devnamesize); drm_fd = open(device, O_RDWR | O_CLOEXEC); if (drm_fd >= 0) { @@ -178,11 +180,11 @@ KMSDRM_Available(void) #endif if (moderndri) { - SDL_strlcpy(kmsdrm_dri_path, "/dev/dri/", sizeof("/dev/dri/") + 1); - SDL_strlcpy(kmsdrm_dri_devname, "card", sizeof("card") + 1); + SDL_strlcpy(kmsdrm_dri_path, "/dev/dri/", sizeof(kmsdrm_dri_path)); + SDL_strlcpy(kmsdrm_dri_devname, "card", sizeof(kmsdrm_dri_devname)); } else { - SDL_strlcpy(kmsdrm_dri_path, "/dev/", sizeof("/dev/") + 1); - SDL_strlcpy(kmsdrm_dri_devname, "drm", sizeof("drm") + 1); + SDL_strlcpy(kmsdrm_dri_path, "/dev/", sizeof(kmsdrm_dri_path)); + SDL_strlcpy(kmsdrm_dri_devname, "drm", sizeof(kmsdrm_dri_devname)); } kmsdrm_dri_pathsize = SDL_strlen(kmsdrm_dri_path);