We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 4966

Summary: [patch] KMSDRM: Add dynamic modeset support
Product: SDL Reporter: Anthony Pesch <inolen>
Component: videoAssignee: Sam Lantinga <slouken>
Status: RESOLVED FIXED QA Contact: Sam Lantinga <slouken>
Severity: normal    
Priority: P2 CC: shantic
Version: HG 2.1   
Hardware: x86_64   
OS: Linux   
Attachments: KMSDRM dynamic modeset patch
Fix regression causing joystick events not being generated when using the KMSDRM driver

Description Anthony Pesch 2020-02-05 01:58:14 UTC 
Hi,

First off, apologies in advance, this patch set has _way_ more noise than is required. I'd never touched this code before and started doing various house keeping while hacking on it as part of improving my understanding. I've rebased and chopped it up pretty substantially to hopefully ease reviewing.

The major parts:

* Remove triple buffering support. As far as I can tell, this goes against the libdrm API; the EGL implementations themselves control the buffering. Removing it isn't absolutely necessary as it seemingly works on the Pi at least, but I noticed this while doing my work and explained my reasoning in the commit.

* Replace the crtc_ready logic which allocates an extra bo to perform the initial CRTC configuration (which is required before calling drmModePageFlip) with a call to drmModeSetCrtc after the front and back buffers are allocated, avoiding this allocation.

* Standardized the SDL_*Data variable names and null checks to improve readability. Given that there were duplicate fields in each SDL_*Data structure, having generic names such as "data" at times was very confusing.

* Removed unused fields from the SDL_*Data structures and moves all display related fields out of SDL_VideoData and into SDL_DisplayData. Not required since the code only supports a single display right now, but this was helpful in reading and understanding the code initially.

* Implement KMSDRM_GetDisplayModes / KMSDRM_SetDisplayMode to provide dynamic modeset support.

If any of the housekeeping is too vain just let me know and I can rebase the changes out. Rather than attaching each patch I'v pushed the changes to https://github.com/inolen/SDL-mirror/commits/kms.

These changes have been tested on a Raspberry Pi 4 and a Dell XPS laptop with an HD 520.
Comment 1 Anthony Pesch 2020-02-07 06:48:35 UTC 
As an update, I went back over the triple buffer changes and left them in. I didn't entirely get the code originally, I had just seen it calling KMSDRM_gbm_surface_lock_front_buffer twice for a single swap and had removed it because I was paranoid of bugs stemming from it while working on the modeset changes.

I've made a few small changes to the logic that had thrown me off originally and rebased the changes:
* The condition wrapping the call to release buffer was incorrect.
* The first call to KMSDRM_gbm_surface_lock_front_buffer has been removed. I don't understand why it existed.
* Added additional comments describing what was going on in the code (as it does fix the buffer release pattern of the original code before it).
Comment 2 Sam Lantinga 2020-02-07 19:57:05 UTC 
These changes sound good. Can you attach a single patch that applies cleanly to the code in Mercurial?

Thanks!
Comment 3 Anthony Pesch 2020-02-09 06:07:58 UTC 
Created attachment 4198 [details]
KMSDRM dynamic modeset patch
Comment 4 Sam Lantinga 2020-02-09 19:44:59 UTC 
Looks good, thanks!
https://hg.libsdl.org/SDL/rev/045f218436fe

Feel free to open additional bugs with further refinements.
Comment 5 Anthony Pesch 2020-02-09 20:59:30 UTC 
Will do, thanks for taking the time to review.
Comment 6 Sam Lantinga 2020-02-10 04:21:07 UTC 
You're welcome!
Comment 7 shantic 2020-03-05 18:48:30 UTC 
Is there any reason this patch would break gamepad support?

I am using SDL on the Odroid Go Advance, if I compile SDL after this commit the gamepad does not work on any app that uses SDL, if I compile a version before this commit, or if I revert it from the latest commit, gamepad works perfectly fine. I see no reason why this would happen, but it does.
Comment 8 Anthony Pesch 2020-03-05 19:24:10 UTC 
Hi,

I was just communicating with one of the Retropie developers regarding this.

This change removed the forced window focus change on creation (https://github.com/inolen/SDL-mirror/commit/3534cb3793f4744509f020f1267f510ec7099366) as part of the change no longer assumes there's only a single window being created. This was perhaps an over-aggressive removal.

Due to that change, joystick events are only received if SDL_SetKeyboardFocus is called explicitly, or if the app has specified SDL_HINT_JOYSTICK_ALLOW_BACKGROUND_EVENTS.

I think that part of my change should be reverted to continue setting mouse / keyboard focus to the window being created. If SDL_WINDOW_INPUT_FOCUS is to be used as an input flag the code could be conditional, but that would still leave existing software broken.
Comment 9 shantic 2020-03-06 00:31:05 UTC 
Re-adding those lines worked, thanks!

(In reply to Anthony Pesch from comment #8)
> Hi,
> 
> I was just communicating with one of the Retropie developers regarding this.
> 
> This change removed the forced window focus change on creation
> (https://github.com/inolen/SDL-mirror/commit/
> 3534cb3793f4744509f020f1267f510ec7099366) as part of the change no longer
> assumes there's only a single window being created. This was perhaps an
> over-aggressive removal.
> 
> Due to that change, joystick events are only received if
> SDL_SetKeyboardFocus is called explicitly, or if the app has specified
> SDL_HINT_JOYSTICK_ALLOW_BACKGROUND_EVENTS.
> 
> I think that part of my change should be reverted to continue setting mouse
> / keyboard focus to the window being created. If SDL_WINDOW_INPUT_FOCUS is
> to be used as an input flag the code could be conditional, but that would
> still leave existing software broken.
Comment 10 Anthony Pesch 2020-03-06 00:53:46 UTC 
Created attachment 4237 [details]
Fix regression causing joystick events not being generated when using the KMSDRM driver

Would suggest re-applying the attached patch. The bit got removed partially based on its comment (/* One window, it always has focus */) while I was adding the code to track each window. I didn't notice the regression as I have SDL_HINT_JOYSTICK_ALLOW_BACKGROUND_EVENTS set.

Sorry about that!.
Comment 11 Sam Lantinga 2020-03-07 16:48:26 UTC 
Fixed, thanks!
https://hg.libsdl.org/SDL/rev/3ff45857428d