Commit 0f807fd607138660d8efcee3b1303ce40c625d4d

Manuel Alfayate Corchete 2020-09-07T22:54:15

kmsdrm: use a black dumb buffer for keeping the PRIMARY PLANE occupied when we destroy the KMS buffers, instead of using the TTY buffer, to avoid flickering.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
323
324
325
326
327
328
329
330
331
332
333
334
335
336
337
338
339
340
341
342
343
344
345
346
347
348
349
350
351
352
353
354
355
356
357
358
359
360
361
362
363
364
365
366
367
368
369
370
371
372
373
374
375
376
377
378
379
380
381
382
383
384
385
386
387
388
389
390
391
392
393
394
395
396
397
398
399
400
401
402
403
404
405
406
407
408
409
410
411
412
413
414
415
416
417
418
419
420
421
422
423
424
425
426
427
428
429
430
431
432
433
434
435
436
437
438
439
440
441
442
443
444
445
446
447
448
449
450
451
452
453
454
455
456
457
458
459
460
461
462
463
464
465
466
467
468
469
470
471
472
473
474
475
476
477
478
479
480
481
482
483
484
485
486
487
488
489
490
491
492
493
494
495
496
497
498
diff --git a/src/video/kmsdrm/SDL_kmsdrmsym.h b/src/video/kmsdrm/SDL_kmsdrmsym.h
index 97df2a2..bc1c41d 100644
--- a/src/video/kmsdrm/SDL_kmsdrmsym.h
+++ b/src/video/kmsdrm/SDL_kmsdrmsym.h
@@ -41,6 +41,7 @@ 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(int,drmIoctl,(int fd, unsigned long request, void *arg))
 SDL_KMSDRM_SYM(drmModeResPtr,drmModeGetResources,(int fd))
 
 SDL_KMSDRM_SYM(int,drmModeAddFB,(int fd, uint32_t width, uint32_t height, uint8_t depth,
diff --git a/src/video/kmsdrm/SDL_kmsdrmvideo.c b/src/video/kmsdrm/SDL_kmsdrmvideo.c
index 49b577f..933b313 100644
--- a/src/video/kmsdrm/SDL_kmsdrmvideo.c
+++ b/src/video/kmsdrm/SDL_kmsdrmvideo.c
@@ -146,6 +146,148 @@ get_driindex(void)
     return -ENOENT;
 }
 
+/**********************/
+/* DUMB BUFFER Block. */
+/**********************/
+
+/* Create a dumb buffer, mmap the dumb buffer and fill it with pixels, */
+/* then create a KMS framebuffer wrapping the dumb buffer.             */
+static dumb_buffer *KMSDRM_CreateDumbBuffer(_THIS)
+{
+    SDL_VideoData *viddata = ((SDL_VideoData *)_this->driverdata);
+    SDL_DisplayData *dispdata = (SDL_DisplayData *)SDL_GetDisplayDriverData(0);
+
+    dumb_buffer *ret = calloc(1, sizeof(*ret));
+    struct drm_mode_create_dumb create;
+    struct drm_mode_map_dumb map;
+    struct drm_mode_destroy_dumb destroy;
+
+    /*
+     * The create ioctl uses the combination of depth and bpp to infer
+     * a format; 24/32 refers to DRM_FORMAT_XRGB8888 as defined in
+     * the drm_fourcc.h header. These arguments are the same as given
+     * to drmModeAddFB, which has since been superseded by
+     * drmModeAddFB2 as the latter takes an explicit format token.
+     *
+     * We only specify these arguments; the driver calculates the
+     * pitch (also known as stride or row length) and total buffer size
+     * for us, also returning us the GEM handle.
+     */
+    create = (struct drm_mode_create_dumb) {
+	.width = dispdata->mode.hdisplay,
+	.height = dispdata->mode.vdisplay,
+	.bpp = 32,
+    };
+
+    if (KMSDRM_drmIoctl(viddata->drm_fd, DRM_IOCTL_MODE_CREATE_DUMB, &create)) {
+	SDL_SetError("failed to create dumb buffer\n");
+	goto err;
+    }
+
+    ret->gem_handles[0] = create.handle;
+    ret->format = DRM_FORMAT_XRGB8888;
+    ret->modifier = DRM_FORMAT_MOD_LINEAR;
+    ret->width = create.width;
+    ret->height = create.height;
+    ret->pitches[0] = create.pitch;
+
+    /*
+     * In order to map the buffer, we call an ioctl specific to the buffer
+     * type, which returns us a fake offset to use with the mmap syscall.
+     * mmap itself then works as you expect.
+     *
+     * Note this means it is not possible to map arbitrary offsets of
+     * buffers without specifically requesting it from the kernel.
+     */
+    map = (struct drm_mode_map_dumb) {
+        .handle = ret->gem_handles[0],
+    };
+    if (KMSDRM_drmIoctl(viddata->drm_fd, DRM_IOCTL_MODE_MAP_DUMB, &map)) {
+        SDL_SetError("failed to get mmap offset for the dumb buffer.");
+        goto err_dumb;
+    }
+
+    ret->dumb.mem = mmap(NULL, create.size, PROT_WRITE, MAP_SHARED,
+                            viddata->drm_fd, map.offset);
+    if (ret->dumb.mem == MAP_FAILED) {
+        SDL_SetError("failed to get mmap offset for the dumb buffer.");
+        goto err_dumb;
+    }
+    ret->dumb.size = create.size;
+
+    return ret;
+
+err_dumb:
+    destroy = (struct drm_mode_destroy_dumb) { .handle = create.handle };
+    KMSDRM_drmIoctl(viddata->drm_fd, DRM_IOCTL_MODE_DESTROY_DUMB, &destroy);
+err:
+    SDL_free(ret);
+    return NULL;
+}
+
+static void
+KMSDRM_DestroyDumbBuffer(_THIS, dumb_buffer *buffer)
+{
+    SDL_VideoData *viddata = ((SDL_VideoData *)_this->driverdata);
+
+    struct drm_mode_destroy_dumb destroy = {
+        .handle = buffer->gem_handles[0],
+    };
+
+    KMSDRM_drmModeRmFB(viddata->drm_fd, buffer->fb_id);
+
+    munmap(buffer->dumb.mem, buffer->dumb.size);
+    KMSDRM_drmIoctl(viddata->drm_fd, DRM_IOCTL_MODE_DESTROY_DUMB, &destroy);
+    free(buffer);
+}
+
+/* Using the CPU mapping, fill the dumb buffer with black pixels. */
+static void
+KMSDRM_FillDumbBuffer(dumb_buffer *buffer)
+{
+    for (unsigned int y = 0; y < buffer->height; y++) {
+	uint32_t *pix = (uint32_t *) ((uint8_t *) buffer->dumb.mem + (y * buffer->pitches[0]));
+	for (unsigned int x = 0; x < buffer->width; x++) {
+	    *pix++ = (0x00000000);
+	}
+    }
+}
+
+static dumb_buffer *KMSDRM_CreateBuffer(_THIS)
+{
+    dumb_buffer *ret;
+    int err;
+
+    SDL_VideoData *viddata = ((SDL_VideoData *)_this->driverdata);
+
+    ret = KMSDRM_CreateDumbBuffer(_this);
+
+    if (!ret)
+        return NULL;
+
+    /*
+     * Wrap our GEM buffer in a KMS framebuffer, so we can then attach it
+     * to a plane. Here's where we get out fb_id!
+     */
+    err = KMSDRM_drmModeAddFB2(viddata->drm_fd, ret->width, ret->height,
+        ret->format, ret->gem_handles, ret->pitches,
+        ret->offsets, &ret->fb_id, 0);
+
+    if (err != 0 || ret->fb_id == 0) {
+        SDL_SetError("Failed AddFB2 on dumb buffer\n");
+        goto err;
+    }
+    return ret;
+
+err:
+    KMSDRM_DestroyDumbBuffer(_this, ret);
+    return NULL;
+}
+
+/***************************/
+/* DUMB BUFFER Block ends. */
+/***************************/
+
 /*********************************/
 /* Atomic helper functions block */
 /*********************************/
@@ -388,7 +530,7 @@ static int get_plane_id(_THIS, uint32_t plane_type)
     return ret;
 }
 
-/* Setup cursor plane and it's props. */
+/* Setup a plane and it's props. */
 int
 setup_plane(_THIS, struct plane **plane, uint32_t plane_type)
 {
@@ -516,7 +658,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;
     }
 
@@ -772,16 +914,9 @@ KMSDRM_DestroySurfaces(_THIS, SDL_Window *window)
     /* it's using.                                                      */
     /********************************************************************/
 
-#if AMDGPU_COMPAT
-    /************************************************************************/
-    /*  We can't do the usual CRTC_ID+FB_ID to 0 with AMDGPU, because       */
-    /*  the driver never recovers from the CONNECTOR and CRTC disconnection.*/
-    /*  The with this solution 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.fb_id = dispdata->dumb_buffer->fb_id;
     plane_info.src_w = dispdata->mode.hdisplay;
     plane_info.src_h = dispdata->mode.vdisplay;
     plane_info.crtc_w = dispdata->mode.hdisplay;
@@ -793,32 +928,6 @@ KMSDRM_DestroySurfaces(_THIS, SDL_Window *window)
     if (drm_atomic_commit(_this, SDL_TRUE)) {
         SDL_SetError("Failed to issue atomic commit on DestroyWindow().");
     }
-#else
-    /*********************************************************************************/
-    /* Disconnect the CONNECTOR from the CRTC (several connectors can read a CRTC),  */
-    /* deactivate the CRTC, and set the PRIMARY PLANE props CRTC_ID and FB_ID to 0.  */
-    /* We have to do this before setting the PLANE CRTC_ID and FB_ID to 0 because    */
-    /* there can be no active CRTC without an active PLANE.                          */
-    /* We can leave all like this if we are exiting the program after the window     */
-    /* destruction, or things will be re-connected again on SwapWindow(), if needed. */
-    /*********************************************************************************/
-    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 yet. */
-    plane_info.plane = dispdata->display_plane;
-
-    drm_atomic_set_plane_props(&plane_info);
-
-    /* Issue blocking atomic commit. */    
-    if (drm_atomic_commit(_this, SDL_TRUE)) {
-        SDL_SetError("Failed to issue atomic commit on window surfaces and buffers destruction.");
-    }
-#endif
 
     /****************************************************************************/
     /* BLOCK 2: We can finally destroy the window GBM and EGL surfaces, and     */
@@ -1006,6 +1115,7 @@ KMSDRM_VideoInit(_THIS)
     dispdata->kms_fence = NULL;
     dispdata->gpu_fence = NULL;
     dispdata->kms_out_fence_fd = -1;
+    dispdata->dumb_buffer = NULL;
 
     if (!dispdata) {
         return SDL_OutOfMemory();
@@ -1185,7 +1295,6 @@ KMSDRM_VideoInit(_THIS)
     if (ret) {
         ret = SDL_SetError("can't find suitable display plane.");
         goto cleanup;
-
     }
 
     /* Get CRTC properties */
@@ -1212,6 +1321,17 @@ KMSDRM_VideoInit(_THIS)
         dispdata->connector->props->props[i]);
     }
 
+    /* Create aux dumb buffer. It's only useful to keep the PRIMARY PLANE occupied
+       when we destroy the GBM surface and it's KMS buffers, so not being able to
+       create it is not fatal. */
+    dispdata->dumb_buffer = KMSDRM_CreateBuffer(_this);
+    if (!dispdata->dumb_buffer) {
+        ret = SDL_SetError("can't find suitable display plane.");
+    } else {
+        /* Fill the dumb buffer with black pixels. */
+        KMSDRM_FillDumbBuffer(dispdata->dumb_buffer);
+    }
+
     /*********************/
     /* Atomic block ends */
     /*********************/
@@ -1259,13 +1379,80 @@ KMSDRM_VideoQuit(_THIS)
 {
     SDL_VideoData *viddata = ((SDL_VideoData *)_this->driverdata);
     SDL_DisplayData *dispdata = (SDL_DisplayData *)SDL_GetDisplayDriverData(0);
+    KMSDRM_PlaneInfo plane_info = {0};
+
+    /*****************************************************************/
+    /*                                                               */
+    /* BLOCK to safely destroy the DUMB BUFFER.                      */
+    /*                                                               */
+    /* Since the program should already have called DestroyWindow()  */
+    /* on all the windows by now, there's no need to destroy the     */
+    /* GBM/EGL surfaces and buffers of the windows here: they have   */
+    /* already been destroyed, and the PRIMARY PLANE is using the    */
+    /* DUMB BUFFER. BUT the DUMB BUFFER we use to keep the PRIMARY   */
+    /* PLANE occupied when we do DestroySurfaces calls is going to   */
+    /* be destroyed one way or another when the program quits, so    */
+    /* to avoid the kernel disabling the CRTC when it detects the    */
+    /* deletion of a buffer that IS IN USE BY THE PRIMARY PLANE,     */
+    /* we do one of these:                                           */
+    /*                                                               */
+    /* -In AMDGPU, where manually disabling the CRTC and             */
+    /*  disconnecting the CONNECTOR from the CRTC is an              */
+    /*  unrecoverable situation, so we point the PRIMARY PLANE to    */ 
+    /*  the original TTY buffer (not guaranteed to be there for us!) */
+    /*  and then destroy the DUMB BUFFER).                           */
+    /*                                                               */
+    /* -In other drivers, we disconnect the CONNECTOR from the CRTC  */
+    /*  (remember: several connectors can read a CRTC), deactivate   */
+    /*  the CRTC, and set the PRIMARY PLANE props CRTC_ID and FB_ID  */
+    /*  to 0. Then we destroy the DUMB BUFFER.                       */
+    /*  We can leave all like this if we are exiting the program:    */
+    /*  FBCON or whatever will reconfigure things as it needs.       */
+    /*                                                               */
+    /*****************************************************************/
+
+#if AMDGPU_COMPAT
 
-    /****************************************************************/
-    /* Since the program should already have called DestroyWindow() */
-    /* on all the windows by now, there's no need to destroy the    */
-    /* GBM/EGL surfaces and buffers of the windows here: they have  */
-    /* already been destroyed.                                      */
-    /****************************************************************/
+    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;
+
+    drm_atomic_set_plane_props(&plane_info);
+
+#else
+
+    /*********************************************************************************/
+   /*********************************************************************************/
+    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 yet. */
+    plane_info.plane = dispdata->display_plane;
+
+    drm_atomic_set_plane_props(&plane_info);
+
+#endif
+
+    /* Issue blocking atomic commit. */    
+    if (drm_atomic_commit(_this, SDL_TRUE)) {
+        SDL_SetError("Failed to issue atomic commit on DestroyWindow().");
+    }
+
+    /* Destroy the DUMB buffer, now that it's not being
+       used anymore by the PRIMARY PLANE. */
+    KMSDRM_DestroyDumbBuffer(_this, dispdata->dumb_buffer);
+
+    /***************/
+    /* BLOCK ENDS. */
+    /***************/
 
     /* Clear out the window list */
     SDL_free(viddata->windows);
@@ -1338,7 +1525,6 @@ KMSDRM_GetDisplayModes(_THIS, SDL_VideoDisplay * display)
 }
 #endif
 
-#if 1
 /* We are NOT really changing the physical display mode, but using
 the PRIMARY PLANE and CRTC to scale as we please. But we need that SDL
 has knowledge of the video modes we are going to use for fullscreen
@@ -1370,7 +1556,6 @@ KMSDRM_GetDisplayModes(_THIS, SDL_VideoDisplay * display)
         }
     }
 }
-#endif
 
 int
 KMSDRM_SetDisplayMode(_THIS, SDL_VideoDisplay * display, SDL_DisplayMode * mode)
diff --git a/src/video/kmsdrm/SDL_kmsdrmvideo.h b/src/video/kmsdrm/SDL_kmsdrmvideo.h
index ab4ea1e..535ee6e 100644
--- a/src/video/kmsdrm/SDL_kmsdrmvideo.h
+++ b/src/video/kmsdrm/SDL_kmsdrmvideo.h
@@ -31,6 +31,7 @@
 #include <unistd.h>
 #include <xf86drm.h>
 #include <xf86drmMode.h>
+
 #include <gbm.h>
 #include <assert.h>
 #if SDL_VIDEO_OPENGL_EGL
@@ -38,10 +39,47 @@
 #include <EGL/eglext.h>
 #endif
 
-/* Driverdata pointers are void struct* used to store backend-specific variables
-   and info that supports the SDL-side structs like SDL Display Devices, SDL_Windows...
-   which need to be "supported" with backend-side info and mechanisms to work. */ 
+/* Headers related to dumb buffer creation. */
+#include <drm_fourcc.h>
+#include <sys/mman.h>
+
+/**********************/
+/* DUMB BUFFER Block. */
+/**********************/
+
+typedef struct dumb_buffer {
+      
+    /* The GEM handle for this buffer, returned by the creation ioctl. */
+    uint32_t gem_handles[4];
+
+    /* The framebuffer ID which is passed to KMS to display. */
+    uint32_t fb_id;
+
+    uint32_t format;
+    uint64_t modifier;
 
+    /* Parameters for our memory-mapped image. */
+    struct {
+	uint32_t *mem;
+	unsigned int size;
+    } dumb;
+
+    unsigned int width;
+    unsigned int height;
+    unsigned int pitches[4]; /* in bytes */
+    unsigned int offsets[4]; /* in bytes */
+
+} dumb_buffer;
+
+/***************************/
+/* DUMB BUFFER Block ends. */
+/***************************/
+
+/****************************************************************************************/
+/* Driverdata pointers are void struct* used to store backend-specific variables        */
+/* and info that supports the SDL-side structs like SDL Display Devices, SDL_Windows... */
+/* which need to be "supported" with backend-side info and mechanisms to work.          */ 
+/****************************************************************************************/
 
 typedef struct SDL_VideoData
 {
@@ -54,23 +92,23 @@ typedef struct SDL_VideoData
     unsigned int num_windows;
 } SDL_VideoData;
 
-struct plane {
-	drmModePlane *plane;
-	drmModeObjectProperties *props;
-	drmModePropertyRes **props_info;
-};
+typedef struct plane {
+    drmModePlane *plane;
+    drmModeObjectProperties *props;
+    drmModePropertyRes **props_info;
+} plane;
 
-struct crtc {
-	drmModeCrtc *crtc;
-	drmModeObjectProperties *props;
-	drmModePropertyRes **props_info;
-};
+typedef struct crtc {
+    drmModeCrtc *crtc;
+    drmModeObjectProperties *props;
+    drmModePropertyRes **props_info;
+} crtc;
 
-struct connector {
-	drmModeConnector *connector;
-	drmModeObjectProperties *props;
-	drmModePropertyRes **props_info;
-};
+typedef struct connector {
+    drmModeConnector *connector;
+    drmModeObjectProperties *props;
+    drmModePropertyRes **props_info;
+} connector;
 
 /* More general driverdata info that gives support and substance to the SDL_Display. */
 typedef struct SDL_DisplayData
@@ -82,10 +120,10 @@ typedef struct SDL_DisplayData
        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;
-    struct crtc *crtc;
-    struct connector *connector;
+    plane *display_plane;
+    plane *cursor_plane;
+    crtc *crtc;
+    connector *connector;
 
     int kms_in_fence_fd;
     int kms_out_fence_fd;
@@ -101,6 +139,9 @@ typedef struct SDL_DisplayData
 
     SDL_bool destroy_surfaces_pending;
 
+    dumb_buffer *dumb_buffer; /* Aux dumb buffer to keep the PRIMARY PLANE
+                                 entertained with when we destroy GBM surface. */
+
 } SDL_DisplayData;
 
 /* Driverdata info that gives KMSDRM-side support and substance to the SDL_Window. */