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

GPU driver for SDL_render #192

Merged
merged 37 commits into from
Aug 29, 2024

Conversation

Akaricchi
Copy link

@Akaricchi Akaricchi commented Aug 17, 2024

A basic GPU driver for SDL_render. Mostly based on the GL driver with some reference to the Vulkan one.

I threw most of the stuff from the test directory at it and it all seems to work, including testautomation. This is by no means comprehensive and I have yet to test it with a more serious program (probably through the sdl2-compat layer), or on anything that isn't Vulkan on Linux.

Some known issues:

  • YUV support is missing. The implementation would probably have to be similar to that of the GL backend. The Vulkan backend uses some fancy extensions for this.
  • All the colorspace conversion stuff found in the Vulkan backend is missing.
  • Presentation code could be improved/stolen from FNA3D as per @thatcosmonaut's suggestion.
  • I've made an effort to support all GPU backends, but this is untested. D3D11 and D3D12 are supported with DXBC shaders (no DXIL), and Metal with text-form MSL. The shaders are rebuildable on any OS; DXBC output was tested with https://github.com/mozilla/fxc2 on Linux.
  • Because we have to support synchronous readback for ReadPixels, everything is rendered into a faux-backbuffer and copied into the swapchain at present time. The Vulkan backend doesn't need this workaround.
  • A few random TODOs and FIXMEs throughout the code.
  • The code is only known to compile with GCC and Clang, and will probably have to be disfigured by someone who cares, in order to compile with VS and conform to whatever prehistoric version of C that SDL follows. I do hate myself and love pain, but not enough to maintain that, sorry. Thank me later for not using designated initializers absolutely everywhere.

Closes #12

Copy link
Collaborator

@flibitijibibo flibitijibibo left a comment

Choose a reason for hiding this comment

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

At a high level this looks great so far - we may change the shader pipeline to be more like testgpu but the core is what's most important and it's in good shape. I may defer to @icculus for some of the FIXMEs/TODOs since a lot of it depends on what SDL_Render prefers at a high level, and I'm not as familiar with that as I should be.

SDL_GpuShaderFormat format;
} GPU_ShaderModuleSource;

// FIXME Please fix this in the build system!
Copy link
Collaborator

Choose a reason for hiding this comment

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

These will have to be #ifdef rather than just #if, since that's how cmakedefines come out https://github.com/thatcosmonaut/SDL/blob/gpu/include/build_config/SDL_build_config.h.cmake#L447

Copy link
Author

@Akaricchi Akaricchi Aug 17, 2024

Choose a reason for hiding this comment

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

Looks like SDL defines a bunch of undefined macros as 0s here, at least for the render backends: https://github.com/libsdl-org/SDL/blob/a7bed810b3499d157655ae6dcc7cd5e8a34fa549/src/SDL_internal.h#L194

GPU also uses #if checks on its backend defines, which is technically not correct (clang throws a warning).

Maybe we could do something similar for GPU backends. I like them being 1/0 macros because it makes the implementation of GPU_FillSupportedShaderFormats neat, but it's not a big deal.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, good catch - we can do the same for GPU.

@Akaricchi
Copy link
Author

I'm personally not a fan of the testgpu shader builder because it requires running on at least two different proprietary operating systems in order to rebuild all the shaders. I tried to make it reasonably portable, but it's your call.

Copy link
Collaborator

@flibitijibibo flibitijibibo left a comment

Choose a reason for hiding this comment

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

Latest should be good enough for the first round of Beta reviews - they may complain about util.h and pipeline.c/h but that's an easy fix if it comes up.

Copy link
Owner

@thatcosmonaut thatcosmonaut left a comment

Choose a reason for hiding this comment

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

Thanks for doing this. There's some significant performance issues here that we should address before ship but the SDL_render tests are functional and the busywork is done, so that's good enough for now. Will squash and then commit a patch on top that fixes a sampler creation issue and notes some performance FIXMEs.

@thatcosmonaut thatcosmonaut merged commit 80dad09 into thatcosmonaut:gpu_next Aug 29, 2024
39 checks passed
@icculus
Copy link

icculus commented Aug 29, 2024

Yeah, I'm super-thrilled someone just jumped in to work on this, as it's going to be quite important, so thanks for that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants