Fixed bug 5231 - Fix for hardware cursor: size and alpha-premultiplication.

Manuel Alfayate Corchete

I noticed pt2-clone had problems with it's optional hardware mouse on the KMSDRM backend: cursor had a transparent block around it.
So I was investigating and it seems that a GBM cursor needs it's pixels to be alpha-premultiplied instead of straight-alpha.
A
lso, I was previously relying on "manual testing" for the cursor size, but it's far better to use whatever the DRM driver recommends via drmGetCap(): any working driver should make a size recommendation via drmGetCap(), so that's what we use now. I took this decision because I found out that the AMDGPU driver reported working cursor sizes that would appear garbled on screen, and only the recommended cursor size works.

src/video/kmsdrm/SDL_kmsdrmmouse.c

@@ -26,6 +26,7 @@
 #include "SDL_kmsdrmvideo.h"
 #include "SDL_kmsdrmmouse.h"
 #include "SDL_kmsdrmdyn.h"
+#include "SDL_assert.h"
 
 #include "../../events/SDL_mouse_c.h"
 #include "../../events/default_cursor.h"
@@ -38,133 +39,59 @@ static void KMSDRM_FreeCursor(SDL_Cursor * cursor);
 static void KMSDRM_WarpMouse(SDL_Window * window, int x, int y);
 static int KMSDRM_WarpMouseGlobal(int x, int y);
 
+/* Converts a pixel from straight-alpha [AA, RR, GG, BB], which the SDL cursor surface has,
+   to premultiplied-alpha [AA. AA*RR, AA*GG, AA*BB].
+   These multiplications have to be done with floats instead of uint32_t's, and the resulting values have 
+   to be converted to be relative to the 0-255 interval, where 255 is 1.00 and anything between 0 and 255 is 0.xx. */
+void alpha_premultiply_ARGB8888 (uint32_t *pixel) {
+
+    uint32_t A, R, G, B;
+
+    /* Component bytes extraction. */
+    A = (*pixel >> (3 << 3)) & 0xFF;
+    R = (*pixel >> (2 << 3)) & 0xFF;
+    G = (*pixel >> (1 << 3)) & 0xFF;
+    B = (*pixel >> (0 << 3)) & 0xFF;
+
+    /* Alpha pre-multiplication of each component. */
+    R = (float)A * ((float)R /255);
+    G = (float)A * ((float)G /255);
+    B = (float)A * ((float)B /255);
+
+    /* ARGB8888 pixel recomposition. */
+    (*pixel) = (((uint32_t)A << 24) | ((uint32_t)R << 16) | ((uint32_t)G << 8)) | ((uint32_t)B << 0);
+}
+
+
 static SDL_Cursor *
 KMSDRM_CreateDefaultCursor(void)
 {
     return SDL_CreateCursor(default_cdata, default_cmask, DEFAULT_CWIDTH, DEFAULT_CHEIGHT, DEFAULT_CHOTX, DEFAULT_CHOTY);
 }
 
-/* Evaluate if a given cursor size is supported or not. Notably, current Intel gfx only support 64x64 and up. */
+/* Create a GBM cursor from a surface, which means creating a hardware cursor.
-static SDL_bool
+   Most programs use software cursors, but protracker-clone for example uses
-KMSDRM_IsCursorSizeSupported (int w, int h, uint32_t bo_format) {
+   an optional hardware cursor. */
-
-    SDL_VideoDevice *dev = SDL_GetVideoDevice();
-    SDL_VideoData *viddata = ((SDL_VideoData *)dev->driverdata);
-    SDL_DisplayData *dispdata = (SDL_DisplayData *)SDL_GetDisplayDriverData(0);
-
-    int ret;
-    uint32_t bo_handle;
-    struct gbm_bo *bo = KMSDRM_gbm_bo_create(viddata->gbm, w, h, bo_format,
-                                       GBM_BO_USE_CURSOR | GBM_BO_USE_WRITE);
-
-    if (!bo) {
-        SDL_SetError("Could not create GBM cursor BO width size %dx%d for size testing", w, h);
-        goto cleanup;
-    }
-
-    bo_handle = KMSDRM_gbm_bo_get_handle(bo).u32;
-    ret = KMSDRM_drmModeSetCursor(viddata->drm_fd, dispdata->crtc_id, bo_handle, w, h);
-
-    if (ret) {
-        goto cleanup;
-    }
-    else {
-        KMSDRM_gbm_bo_destroy(bo);
-        return SDL_TRUE;
-    }
-
-cleanup:
-    if (bo) {
-        KMSDRM_gbm_bo_destroy(bo);
-    }
-    return SDL_FALSE;
-}
-
-/* Create a cursor from a surface */
 static SDL_Cursor *
 KMSDRM_CreateCursor(SDL_Surface * surface, int hot_x, int hot_y)
 {
     SDL_VideoDevice *dev = SDL_GetVideoDevice();
     SDL_VideoData *viddata = ((SDL_VideoData *)dev->driverdata);
-    SDL_PixelFormat *pixlfmt = surface->format;
     KMSDRM_CursorData *curdata;
     SDL_Cursor *cursor;
-    SDL_bool cursor_supported = SDL_FALSE;
+    uint64_t usable_cursor_w, usable_cursor_h;
-    int i, ret, usable_cursor_w, usable_cursor_h;
+    uint32_t bo_stride, pixel;
-    uint32_t bo_format, bo_stride;
+    uint32_t *buffer = NULL;
-    char *buffer = NULL;
     size_t bufsize;
 
-    switch(pixlfmt->format) {
+    /* All code below assumes ARGB8888 format for the cursor surface, like other backends do.
-    case SDL_PIXELFORMAT_RGB332:
+       Also, the GBM BO pixels have to be alpha-premultiplied, but the SDL surface we receive has
-        bo_format = GBM_FORMAT_RGB332;
+       straight-alpha pixels, so we always have to convert. */ 
-        break;
+    SDL_assert(surface->format->format == SDL_PIXELFORMAT_ARGB8888);
-    case SDL_PIXELFORMAT_ARGB4444:
+    SDL_assert(surface->pitch == surface->w * 4);
-        bo_format = GBM_FORMAT_ARGB4444;
-        break;
-    case SDL_PIXELFORMAT_RGBA4444:
-        bo_format = GBM_FORMAT_RGBA4444;
-        break;
-    case SDL_PIXELFORMAT_ABGR4444:
-        bo_format = GBM_FORMAT_ABGR4444;
-        break;
-    case SDL_PIXELFORMAT_BGRA4444:
-        bo_format = GBM_FORMAT_BGRA4444;
-        break;
-    case SDL_PIXELFORMAT_ARGB1555:
-        bo_format = GBM_FORMAT_ARGB1555;
-        break;
-    case SDL_PIXELFORMAT_RGBA5551:
-        bo_format = GBM_FORMAT_RGBA5551;
-        break;
-    case SDL_PIXELFORMAT_ABGR1555:
-        bo_format = GBM_FORMAT_ABGR1555;
-        break;
-    case SDL_PIXELFORMAT_BGRA5551:
-        bo_format = GBM_FORMAT_BGRA5551;
-        break;
-    case SDL_PIXELFORMAT_RGB565:
-        bo_format = GBM_FORMAT_RGB565;
-        break;
-    case SDL_PIXELFORMAT_BGR565:
-        bo_format = GBM_FORMAT_BGR565;
-        break;
-    case SDL_PIXELFORMAT_RGB888:
-    case SDL_PIXELFORMAT_RGB24:
-        bo_format = GBM_FORMAT_RGB888;
-        break;
-    case SDL_PIXELFORMAT_BGR888:
-    case SDL_PIXELFORMAT_BGR24:
-        bo_format = GBM_FORMAT_BGR888;
-        break;
-    case SDL_PIXELFORMAT_RGBX8888:
-        bo_format = GBM_FORMAT_RGBX8888;
-        break;
-    case SDL_PIXELFORMAT_BGRX8888:
-        bo_format = GBM_FORMAT_BGRX8888;
-        break;
-    case SDL_PIXELFORMAT_ARGB8888:
-        bo_format = GBM_FORMAT_ARGB8888;
-        break;
-    case SDL_PIXELFORMAT_RGBA8888:
-        bo_format = GBM_FORMAT_RGBA8888;
-        break;
-    case SDL_PIXELFORMAT_ABGR8888:
-        bo_format = GBM_FORMAT_ABGR8888;
-        break;
-    case SDL_PIXELFORMAT_BGRA8888:
-        bo_format = GBM_FORMAT_BGRA8888;
-        break;
-    case SDL_PIXELFORMAT_ARGB2101010:
-        bo_format = GBM_FORMAT_ARGB2101010;
-        break;
-    default:
-        SDL_SetError("Unsupported pixel format for cursor");
-        return NULL;
-    }
 
-    if (!KMSDRM_gbm_device_is_format_supported(viddata->gbm, bo_format, GBM_BO_USE_CURSOR | GBM_BO_USE_WRITE)) {
+    if (!KMSDRM_gbm_device_is_format_supported(viddata->gbm, GBM_FORMAT_ARGB8888, GBM_BO_USE_CURSOR | GBM_BO_USE_WRITE)) {
-        SDL_SetError("Unsupported pixel format for cursor");
+        printf("Unsupported pixel format for cursor\n");
         return NULL;
     }
 
@@ -180,25 +107,15 @@ KMSDRM_CreateCursor(SDL_Surface * surface, int hot_x, int hot_y)
         return NULL;
     }
 
-    /* We have to know beforehand if a cursor with the same size as the surface is supported.
+    /* Find out what GBM cursor size is recommended by the driver. */
-     * If it's not, we have to find an usable cursor size and use an intermediate and clean buffer.
+    if (KMSDRM_drmGetCap(viddata->drm_fd, DRM_CAP_CURSOR_WIDTH,  &usable_cursor_w) ||
-     * If we can't find a cursor size supported by the hardware, we won't go on trying to 
+        KMSDRM_drmGetCap(viddata->drm_fd, DRM_CAP_CURSOR_HEIGHT, &usable_cursor_h)) {
-     * call SDL_SetCursor() later. */
+            SDL_SetError("Could not get the recommended GBM cursor size");
-
+            goto cleanup;
-    usable_cursor_w = surface->w;
-    usable_cursor_h = surface->h;
-
-    while (usable_cursor_w <= MAX_CURSOR_W && usable_cursor_h <= MAX_CURSOR_H) { 
-        if (KMSDRM_IsCursorSizeSupported(usable_cursor_w, usable_cursor_h, bo_format)) {
-            cursor_supported = SDL_TRUE;
-            break;
-        }
-        usable_cursor_w += usable_cursor_w;
-        usable_cursor_h += usable_cursor_h;
     }
 
-    if (!cursor_supported) {
+    if (usable_cursor_w == 0 || usable_cursor_w == 0) {
-        SDL_SetError("Could not find a cursor size supported by the kernel driver");
+            SDL_SetError("Could not get an usable GBM cursor size");
             goto cleanup;
     }
 
@@ -207,7 +124,7 @@ KMSDRM_CreateCursor(SDL_Surface * surface, int hot_x, int hot_y)
     curdata->w = usable_cursor_w;
     curdata->h = usable_cursor_h;
 
-    curdata->bo = KMSDRM_gbm_bo_create(viddata->gbm, usable_cursor_w, usable_cursor_h, bo_format,
+    curdata->bo = KMSDRM_gbm_bo_create(viddata->gbm, usable_cursor_w, usable_cursor_h, GBM_FORMAT_ARGB8888,
                                        GBM_BO_USE_CURSOR | GBM_BO_USE_WRITE);
 
     if (!curdata->bo) {
@@ -218,9 +135,10 @@ KMSDRM_CreateCursor(SDL_Surface * surface, int hot_x, int hot_y)
     bo_stride = KMSDRM_gbm_bo_get_stride(curdata->bo);
     bufsize = bo_stride * curdata->h;
 
-    if (surface->pitch != bo_stride) {
+    /* Always use a temp buffer: it serves the purpose of storing the alpha-premultiplied pixels (so
-        /* pitch doesn't match stride, must be copied to temp buffer  */
+       we can copy them to the gbm BO with a single gb_bo_write() call), and also copying from the
-        buffer = SDL_malloc(bufsize);
+       SDL surface, line by line, to a gbm BO with different pitch. */
+    buffer = (uint32_t*)SDL_malloc(bufsize);
     if (!buffer) {
 	SDL_OutOfMemory();
 	goto cleanup;
@@ -233,14 +151,16 @@ KMSDRM_CreateCursor(SDL_Surface * surface, int hot_x, int hot_y)
 	}
     }
 
-        /* Clean the whole temporary buffer */
+    /* Clean the whole temporary buffer. */
     SDL_memset(buffer, 0x00, bo_stride * curdata->h);
 
-        /* Copy to temporary buffer */
+    /* Copy from SDL surface to buffer, pre-multiplying by alpha each pixel as we go. */
-        for (i = 0; i < surface->h; i++) {
+    for (int i = 0; i < surface->h; i++) {
-            SDL_memcpy(buffer + (i * bo_stride),
+        for (int j = 0; j < surface->w; j++) {
-                       ((char *)surface->pixels) + (i * surface->pitch),
+            pixel = ((uint32_t*)surface->pixels)[i * surface->w + j];
-                       surface->w * pixlfmt->BytesPerPixel);
+            alpha_premultiply_ARGB8888 (&pixel);
+	    SDL_memcpy(buffer + (i * curdata->w)  + j, &pixel, 4);
+        }
     }
 
     if (SDL_MUSTLOCK(surface)) {
@@ -255,26 +175,6 @@ KMSDRM_CreateCursor(SDL_Surface * surface, int hot_x, int hot_y)
     /* Free temporary buffer */
     SDL_free(buffer);
     buffer = NULL;
-    } else {
-        /* surface matches BO format */
-        if (SDL_MUSTLOCK(surface)) {
-            if (SDL_LockSurface(surface) < 0) {
-                /* Could not lock surface */
-                goto cleanup;
-            }
-        }
-
-        ret = KMSDRM_gbm_bo_write(curdata->bo, surface->pixels, bufsize);
-
-        if (SDL_MUSTLOCK(surface)) {
-            SDL_UnlockSurface(surface);
-        }
-
-        if (ret) {
-            SDL_SetError("Could not write to GBM cursor BO");
-            goto cleanup;
-        }
-    }
 
     cursor->driverdata = curdata;
 

src/video/kmsdrm/SDL_kmsdrmsym.h

@@ -40,6 +40,7 @@ SDL_KMSDRM_SYM(void,drmModeFreeFB,(drmModeFBPtr ptr))
 SDL_KMSDRM_SYM(void,drmModeFreeCrtc,(drmModeCrtcPtr ptr))
 SDL_KMSDRM_SYM(void,drmModeFreeConnector,(drmModeConnectorPtr ptr))
 SDL_KMSDRM_SYM(void,drmModeFreeEncoder,(drmModeEncoderPtr ptr))
+SDL_KMSDRM_SYM(int,drmGetCap,(int fd, uint64_t capability, uint64_t *value))
 SDL_KMSDRM_SYM(drmModeResPtr,drmModeGetResources,(int fd))
 SDL_KMSDRM_SYM(int,drmModeAddFB,(int fd, uint32_t width, uint32_t height, uint8_t depth,
                                  uint8_t bpp, uint32_t pitch, uint32_t bo_handle,