Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kmsdrm: Restore atomic support. #11511

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

icculus
Copy link
Collaborator

@icculus icculus commented Nov 21, 2024

This is an attempt to restore the ATOMIC support to the kmsdrm backend.

This was done by merging @vanfanel's final atomic work from SDL2 into the latest from SDL3. This wasn't a trivial effort, as a lot had changed in both kmsdrm specifically and SDL3 generally, so I expect there are issues to resolve in this PR still. It's at the "it compiles" stage; I'll be trying it on a Raspberry Pi 5 soon and fixing obvious issues that pop up.

There isn't a separate atomic and non-atomic version of the kmsdrm backend in this PR; it's now one codebase that will decide to use the atomic features if it can set the DRM_CLIENT_CAP_ATOMIC and DRM_CLIENT_CAP_UNIVERSAL_PLANES client capabilities. If not, it'll use the "legacy" codepaths. Before merging, we should probably add an SDL hint to let people turn off atomic support even if the system would otherwise support it, as a failsafe.

Note a few FIXMEs in this PR; these were pieces I couldn't decide how to correctly adapt, but I made a reasonable attempt anyhow.

Reference Issue #4984.

@icculus icculus added this to the 3.2.0 milestone Nov 21, 2024
/* frame.(KMS will have to wait on it before doing a pageflip.) */
/******************************************************************/
dispdata->gpu_fence = create_fence(_this, EGL_NO_NATIVE_FENCE_FD_ANDROID);
SDL_assert(dispdata->gpu_fence);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the preceding comment:

Also, DON'T remove the asserts: if a fence-related call fails, it's better that program exits immediately, or we could leave KMS waiting for a failed/missing fence forever.

this should be SDL_assert_always(), or the asserts will be compiled out in release builds.

Suggested change
SDL_assert(dispdata->gpu_fence);
SDL_assert_always(dispdata->gpu_fence);

dispdata->kms_in_fence_fd = _this->egl_data->eglDupNativeFenceFDANDROID (_this->egl_data->egl_display, dispdata->gpu_fence);

_this->egl_data->eglDestroySyncKHR(_this->egl_data->egl_display, dispdata->gpu_fence);
SDL_assert(dispdata->kms_in_fence_fd != -1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here:

Suggested change
SDL_assert(dispdata->kms_in_fence_fd != -1);
SDL_assert_always(dispdata->kms_in_fence_fd != -1);

src/video/kmsdrm/SDL_kmsdrmopengles.c Outdated Show resolved Hide resolved
/* KMS-side FENCE OBJECT so we can use use it to fence the GPU. */
/****************************************************************/
dispdata->kms_fence = create_fence(_this, dispdata->kms_out_fence_fd);
SDL_assert(dispdata->kms_fence);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more time:

Suggested change
SDL_assert(dispdata->kms_fence);
SDL_assert_always(dispdata->kms_fence);

@icculus
Copy link
Collaborator Author

icculus commented Dec 11, 2024

I'm not sure we should get into the business of promising to crash the entire process if something goes wrong, by adding assert_always to this.

@slouken
Copy link
Collaborator

slouken commented Dec 11, 2024

I'm not sure we should get into the business of promising to crash the entire process if something goes wrong, by adding assert_always to this.

Agreed. Gamers get very unhappy if they lose several hours progress without getting a chance to save. :)

@icculus
Copy link
Collaborator Author

icculus commented Dec 16, 2024

Tried this out on a raspberry pi, and it mostly works. Here's the current TODO list:

  • It crashes because it does this in several places...

    SDL_DisplayData *dispdata = (SDL_DisplayData *)SDL_GetDisplayDriverData(0);

    ...and this results in a NULL dispdata. I assume this function changed from an index to an instance ID, and hardcoding a 1 instead of a zero makes this work, since that's the instance ID that I've got. But I assume this also comes from a time where you'd only have one display in the kmsdrm backend, so this probably needs a bit of a refactor in any case to not have a hardcoded value.

  • Running things like testsprite and testaudio are only rendering a tiny window to maybe a quarter of the screen, in the top right. Something failed to scale? Something failed to change resolutions? I'm not sure. This definitely takes the whole television screen in main, so it's a regression.

  • The mouse cursor on my 1080p TV is microscopic. Might be that it was intended to scale and didn't?

These are obviously important issues to resolve, but I was surprised that it was otherwise rendering stuff on the (almost) first attempt. More work to be done still, though.

@slouken
Copy link
Collaborator

slouken commented Dec 16, 2024

Tried this out on a raspberry pi, and it mostly works. Here's the current TODO list:

  • It crashes because it does this in several places...
    SDL_DisplayData *dispdata = (SDL_DisplayData *)SDL_GetDisplayDriverData(0);

As you noted, we probably want to get the correct display data. If we have a window, you can use SDL_GetDisplayDriverDataForWindow(). Otherwise if you really want the first display, you can use SDL_GetDisplayDriverData(SDL_GetPrimaryDisplay()).

@ccawley2011
Copy link
Contributor

ccawley2011 commented Dec 27, 2024

  • Running things like testsprite and testaudio are only rendering a tiny window to maybe a quarter of the screen, in the top right. Something failed to scale? Something failed to change resolutions? I'm not sure. This definitely takes the whole television screen in main, so it's a regression.

Setting VIDEO_DEVICE_CAPS_FULLSCREEN_ONLY for the kmsdrm backend might help here, but that does assume that it doesn't need to support multiple windows on a single display.

@slouken slouken modified the milestones: 3.2.0, 3.4.0 Jan 10, 2025
@slouken slouken added the early in milestone This change should be made early in the milestone for additional testing label Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
early in milestone This change should be made early in the milestone for additional testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants