Add DRM sources to sfml-window #2259

ChrisThrasher · 2022-10-24T00:56:45Z

Description

This moves the 3rd party DRM sources into SFML itself since we need to regularly modify these files. Note that we can't immediately start building this as C++ code since it uses C features or extensions that don't work in C++. That transition will require some extra work.

Addresses #2252

While I was working on this I realized there was no cache variable for SFML_USE_DRM so I added that. This means you can edit the CMakeCache to enable or disable this setting without having to reconfigure the project.

How far do we want to take this? Do we want to refactor this as C++ code and do some modest style fixes in the same PR that moves the code or do we tackle this in smaller pieces?

EDT: I expanded the scope of this PR to also include all of the refactoring required to make this code match the rest of SFML.

ChrisThrasher · 2022-10-24T01:03:51Z

Here's a sneak peak of the errors we get when trying to build this C code as C++.

/home/thrasher/Projects/sfml/src/SFML/Window/DRM/Common.cpp: In function ‘void drm_fb_destroy_callback(gbm_bo*, void*)’:
/home/thrasher/Projects/sfml/src/SFML/Window/DRM/Common.cpp:46:29: error: invalid conversion from ‘void*’ to ‘drm_fb*’ [-fpermissive]
   46 |         struct drm_fb *fb = data;
      |                             ^~~~
      |                             |
      |                             void*
/home/thrasher/Projects/sfml/src/SFML/Window/DRM/Common.cpp: In function ‘drm_fb* drm_fb_get_from_bo(gbm_bo*)’:
/home/thrasher/Projects/sfml/src/SFML/Window/DRM/Common.cpp:57:49: error: invalid conversion from ‘void*’ to ‘drm_fb*’ [-fpermissive]
   57 |         struct drm_fb *fb = gbm_bo_get_user_data(bo);
      |                             ~~~~~~~~~~~~~~~~~~~~^~~~
      |                                                 |
      |                                                 void*
/home/thrasher/Projects/sfml/src/SFML/Window/DRM/Common.cpp:66:20: error: invalid conversion from ‘void*’ to ‘drm_fb*’ [-fpermissive]
   66 |         fb = calloc(1, sizeof *fb);
      |              ~~~~~~^~~~~~~~~~~~~~~
      |                    |
      |                    void*
/home/thrasher/Projects/sfml/src/SFML/Window/DRM/Common.cpp:103:79: error: ISO C++ forbids compound-literals [-Werror=pedantic]
  103 |                 memcpy(handles, (uint32_t [4]){gbm_bo_get_handle(bo).u32,0,0,0}, 16);
      |                                                                               ^
/home/thrasher/Projects/sfml/src/SFML/Window/DRM/Common.cpp:103:47: error: taking address of temporary array
  103 |                 memcpy(handles, (uint32_t [4]){gbm_bo_get_handle(bo).u32,0,0,0}, 16);
      |                                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/thrasher/Projects/sfml/src/SFML/Window/DRM/Common.cpp:104:75: error: ISO C++ forbids compound-literals [-Werror=pedantic]
  104 |                 memcpy(strides, (uint32_t [4]){gbm_bo_get_stride(bo),0,0,0}, 16);
      |                                                                           ^
/home/thrasher/Projects/sfml/src/SFML/Window/DRM/Common.cpp:104:47: error: taking address of temporary array
  104 |                 memcpy(strides, (uint32_t [4]){gbm_bo_get_stride(bo),0,0,0}, 16);
      |                                               ^~~~~~~~~~~~~~~

Fixing the compound literals is easy if we could use C++11's std::array but will be a bit harder when restricted to C++03. We also need to do some style fixes like replacing all the tabs with spaces. Much of this won't be hard, we just need to agree on what exactly we want to change.

ChrisThrasher · 2022-10-24T07:03:35Z

I expanded the scope of this PR to also include all of the refactoring required to make this code match the rest of SFML.

ChrisThrasher · 2022-10-24T07:13:41Z

@substring @oomek Can you try this out and make sure I didn't break anything? Thanks!

oomek · 2022-10-25T11:01:19Z

@ChrisThrasher Sorry, I'm a bit preoccupied by the bathroom renovation, I'll check as soon as I can. Thank you very much for getting your hands dirty.

substring · 2022-10-26T16:29:10Z

We've stumbled upon some very rare cases where that part would fail

SFML/src/SFML/Window/DRM/DRMContext.cpp

Lines 107 to 118 in 4134e35

    
           /* Avoid a modeswitch if possible */ 
        
           if (drmNode.mode != &drmNode.originalCrtc->mode) 
        
               drmModeSetCrtc(drmNode.fd, 
        
                              drmNode.originalCrtc->crtc_id, 
        
                              drmNode.originalCrtc->buffer_id, 
        
                              drmNode.originalCrtc->x, 
        
                              drmNode.originalCrtc->y, 
        
                              &drmNode.connectorId, 
        
                              1, 
        
                              &drmNode.originalCrtc->mode); 
        
           else if (getenv("SFML_DRM_DEBUG")) 
        
               printf("DRM keeping the same mode since using the original one\n");

Can you just keep L109->116 from that block ? Let's always trigger the drmModeSetCrtc, whichever the situation.

Beside that case, tests were good so far. I'll let @oomek comment further if he wishes

ChrisThrasher · 2022-10-26T18:37:45Z

We've stumbled upon some very rare cases where that part would fail

SFML/src/SFML/Window/DRM/DRMContext.cpp

Lines 107 to 118 in 4134e35

/* Avoid a modeswitch if possible */

if (drmNode.mode != &drmNode.originalCrtc->mode)

drmModeSetCrtc(drmNode.fd,

drmNode.originalCrtc->crtc_id,

drmNode.originalCrtc->buffer_id,

drmNode.originalCrtc->x,

drmNode.originalCrtc->y,

&drmNode.connectorId,

1,

&drmNode.originalCrtc->mode);

else if (getenv("SFML_DRM_DEBUG"))

printf("DRM keeping the same mode since using the original one\n");

Can you just keep L109->116 from that block ? Let's always trigger the drmModeSetCrtc, whichever the situation.

Beside that case, tests were good so far. I'll let @oomek comment further if he wishes

Is this a bug I introduced in this PR?

EDIT: I pushed an additional commit for this change. Let me know if it helps.

oomek · 2022-10-26T21:15:09Z

Is this a bug I introduced in this PR?

It's not strictly a bug, but an optimization added by @substring that does not play nicely when the drm app is launched from linux session in VirtualBox. It resulted in a black screen, no console visible upon exiting the app, so it's better to remove it until further investigation. Thanks for the commit reverting it, and your overall involvement in SFML-ifying the common drm functions. I had a chance to test it this evening and all seems to be working as expected.

CMakeLists.txt

src/SFML/Window/DRM/DRMContext.hpp

src/SFML/Window/DRM/DRMContext.cpp

Bambo-Borris

Feels like we're at diminishing returns style improvement wise. I can't see any obvious issues so I reckon it's ready

ChrisThrasher · 2022-11-21T22:01:03Z

@substring @oomek I made substantial changes since I last pinged you about this PR but have since finished everything I wanted to do. If possible I'd really appreciate you give this a review and test it out on your end so we can get this merged. I think this will help speed up all future work on DRM code. Thanks!

oomek · 2022-11-24T20:11:52Z

Just tested. All looking good. Sorry for the late response.

substring · 2022-11-26T19:35:34Z

LGTM

src/SFML/Window/DRM/DRMContext.cpp

eXpl0it3r

Given that others having given an approval this is basically good to go, just three small thingies 😄

src/SFML/Window/DRM/DRMContext.cpp

eXpl0it3r · 2022-12-12T23:05:39Z

src/SFML/Window/DRM/DRMContext.cpp

+            return -1;
+        }
+
+        int fd = -1;


I vote for int fileDescriptor

eXpl0it3r · 2022-12-12T23:09:36Z

src/SFML/Window/DRM/DRMContext.cpp

+        for (int i = 0; i < numDevices; ++i)
+        {
+            drmDevicePtr device = devices[i];
+            int ret;


returnCode or just result

ChrisThrasher changed the title ~~Move drm sources~~ Move DRM sources Oct 24, 2022

ChrisThrasher changed the title ~~Move DRM sources~~ Move DRM sources into sfml-window Oct 24, 2022

ChrisThrasher mentioned this pull request Oct 24, 2022

Incorporate 3rd party drm-common.[h|c] files into SFML #2252

Closed

ChrisThrasher added m:sfml-window p:linux m:config labels Oct 24, 2022

ChrisThrasher force-pushed the move_drm_sources branch 2 times, most recently from d2d07e0 to c43e9e3 Compare October 25, 2022 16:19

ChrisThrasher marked this pull request as ready for review October 25, 2022 16:21

ChrisThrasher force-pushed the move_drm_sources branch from c43e9e3 to 4134e35 Compare October 25, 2022 18:17

ChrisThrasher force-pushed the move_drm_sources branch 3 times, most recently from ba0e928 to 78555c0 Compare October 26, 2022 18:48

ChrisThrasher changed the title ~~Move DRM sources into sfml-window~~ Add DRM sources to sfml-window Oct 26, 2022

ChrisThrasher force-pushed the move_drm_sources branch 2 times, most recently from ae3f710 to 8931f17 Compare October 26, 2022 19:15

oomek approved these changes Oct 26, 2022

View reviewed changes

ChrisThrasher force-pushed the move_drm_sources branch 2 times, most recently from e4ac09d to 972279f Compare October 27, 2022 16:38

ChrisThrasher requested review from binary1248, Bromeon, eXpl0it3r and vittorioromeo November 4, 2022 18:32

eXpl0it3r added s:accepted and removed s:undecided labels Nov 6, 2022

eXpl0it3r added this to Discussion in SFML 2.6.0 via automation Nov 6, 2022

eXpl0it3r added this to the 2.6 milestone Nov 6, 2022

ChrisThrasher force-pushed the move_drm_sources branch from 7c82048 to bf8fb77 Compare November 15, 2022 22:20

kimci86 reviewed Nov 17, 2022

View reviewed changes

ChrisThrasher force-pushed the move_drm_sources branch 2 times, most recently from 3da585d to e9964f5 Compare November 18, 2022 04:20

SFML deleted a comment from ChrisThrasher Nov 18, 2022

Bambo-Borris approved these changes Nov 21, 2022

View reviewed changes

oomek approved these changes Nov 24, 2022

View reviewed changes

eXpl0it3r moved this from Discussion to Review & Testing in SFML 2.6.0 Nov 26, 2022

eXpl0it3r reviewed Nov 26, 2022

View reviewed changes

src/SFML/Window/DRM/DRMContext.cpp Show resolved Hide resolved

eXpl0it3r reviewed Nov 26, 2022

View reviewed changes

src/SFML/Window/DRM/DRMContext.cpp Show resolved Hide resolved

eXpl0it3r requested changes Dec 12, 2022

View reviewed changes

ChrisThrasher added 3 commits December 12, 2022 21:38

Add option for enabling DRM

a08c111

Add DRM utilities to sfml-window

dbf3396

Always trigger drmModeSetCrtc

cbcdf46

ChrisThrasher force-pushed the move_drm_sources branch from e9964f5 to cbcdf46 Compare December 13, 2022 04:44

eXpl0it3r approved these changes Dec 13, 2022

View reviewed changes

SFML 2.6.0 automation moved this from Review & Testing to Ready Dec 13, 2022

ChrisThrasher merged commit ed6d944 into SFML:2.6.x Dec 14, 2022

SFML 2.6.0 automation moved this from Ready to Done Dec 14, 2022

ChrisThrasher deleted the move_drm_sources branch December 14, 2022 04:52

ChrisThrasher mentioned this pull request Dec 14, 2022

Feature/backmerge #2296

Merged

dogunbound mentioned this pull request May 17, 2023

Pull request for 2.6.x CSFML SFML/CSFML#186

Merged

ChrisThrasher mentioned this pull request Jun 23, 2023

Implement remaining SFML 2.6.0 changes SFML/CSFML#187

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DRM sources to sfml-window #2259

Add DRM sources to sfml-window #2259

ChrisThrasher commented Oct 24, 2022 •

edited

ChrisThrasher commented Oct 24, 2022

ChrisThrasher commented Oct 24, 2022

ChrisThrasher commented Oct 24, 2022

oomek commented Oct 25, 2022

substring commented Oct 26, 2022

ChrisThrasher commented Oct 26, 2022 •

edited

oomek commented Oct 26, 2022 •

edited

Bambo-Borris left a comment

ChrisThrasher commented Nov 21, 2022

oomek commented Nov 24, 2022

substring commented Nov 26, 2022

eXpl0it3r left a comment •

edited

eXpl0it3r Dec 12, 2022

eXpl0it3r Dec 12, 2022

Add DRM sources to sfml-window #2259

Add DRM sources to sfml-window #2259

Conversation

ChrisThrasher commented Oct 24, 2022 • edited

Description

ChrisThrasher commented Oct 24, 2022

ChrisThrasher commented Oct 24, 2022

ChrisThrasher commented Oct 24, 2022

oomek commented Oct 25, 2022

substring commented Oct 26, 2022

ChrisThrasher commented Oct 26, 2022 • edited

oomek commented Oct 26, 2022 • edited

Bambo-Borris left a comment

Choose a reason for hiding this comment

ChrisThrasher commented Nov 21, 2022

oomek commented Nov 24, 2022

substring commented Nov 26, 2022

eXpl0it3r left a comment • edited

Choose a reason for hiding this comment

eXpl0it3r Dec 12, 2022

Choose a reason for hiding this comment

eXpl0it3r Dec 12, 2022

Choose a reason for hiding this comment

ChrisThrasher commented Oct 24, 2022 •

edited

ChrisThrasher commented Oct 26, 2022 •

edited

oomek commented Oct 26, 2022 •

edited

eXpl0it3r left a comment •

edited