This bug report was migrated from our old Bugzilla tracker.

These attachments are available in the static archive:

• KMSDRM dynamic modeset patch (kmsdrm_dynamic_modeset.patch, text/plain, 2020-02-09 06:07:58 +0000, 53286 bytes)
• Fix regression causing joystick events not being generated when using the KMSDRM driver (KMSDRM_FOCUS.patch, text/plain, 2020-03-06 00:53:46 +0000, 469 bytes)

Reported in version: HG 2.1
Reported for operating system, platform: Linux, x86_64

Comments on the original bug report:

On 2020-02-05 01:58:14 +0000, Anthony Pesch wrote:

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.

On 2020-02-07 06:48:35 +0000, Anthony Pesch wrote:

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).

On 2020-02-07 19:57:05 +0000, Sam Lantinga wrote:

These changes sound good. Can you attach a single patch that applies cleanly to the code in Mercurial?

Thanks!

On 2020-02-09 06:07:58 +0000, Anthony Pesch wrote:

Created attachment 4198
KMSDRM dynamic modeset patch

On 2020-02-09 19:44:59 +0000, Sam Lantinga wrote:

Looks good, thanks!
https://hg.libsdl.org/SDL/rev/045f218436fe

Feel free to open additional bugs with further refinements.

On 2020-02-09 20:59:30 +0000, Anthony Pesch wrote:

Will do, thanks for taking the time to review.

On 2020-02-10 04:21:07 +0000, Sam Lantinga wrote:

You're welcome!

On 2020-03-05 18:48:30 +0000, wrote:

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.

On 2020-03-05 19:24:10 +0000, Anthony Pesch wrote:

Hi,

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

This change removed the forced window focus change on creation (inolen/SDL-mirror@3534cb3) 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.

On 2020-03-06 00:31:05 +0000, wrote:

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.

On 2020-03-06 00:53:46 +0000, Anthony Pesch wrote:

Created attachment 4237
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!.

On 2020-03-07 16:48:26 +0000, Sam Lantinga wrote:

Fixed, thanks!
https://hg.libsdl.org/SDL/rev/3ff45857428d