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

Vulkan DRM/KMS support #13222

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Vulkan DRM/KMS support #13222

wants to merge 1 commit into from

Conversation

camdenorrb
Copy link

@camdenorrb camdenorrb commented Dec 14, 2024

Utilizes vkCreateDisplayPlaneSurfaceKHR to create a DRM/KMS level surface.

This allows you to avoid dependency on X11, Wayland and other window systems

@camdenorrb
Copy link
Author

First time contributing, not sure if those ci's are normal. Will look later

@JosJuice
Copy link
Member

No, those results are unexpected.

@camdenorrb
Copy link
Author

camdenorrb commented Dec 15, 2024

No, those results are unexpected.

Oh, is it literally empty, lol....
Welp, that's fun

@camdenorrb
Copy link
Author

Probably has to do with it being on by default, will fix tomorrow

@mitaclaw
Copy link
Contributor

@dolphin-emu-bot rebuild

@camdenorrb
Copy link
Author

I think everything should be fixed next run

@dreamsyntax
Copy link
Member

Gave this PR a try to see if it changes the dGPU flatpak error since KHR fails, but no change. Unrelated.

@camdenorrb
Copy link
Author

Gave this PR a try to see if it changes the dGPU flatpak error since KHR fails, but no change. Unrelated.

Sorry, wish it helped yah

@dreamsyntax
Copy link
Member

I think everything should be fixed next run

You can run the Tools/lint.sh script locally to catch the linter.
It's still failing. I can re-run when its ready.
Also please squash any superfluous commits

@camdenorrb
Copy link
Author

@dreamsyntax

I think everything should be fixed next run

You can run the Tools/lint.sh script locally to catch the linter. It's still failing. I can re-run when its ready. Also please squash any superfluous commits

I believe it should be good now, was using that lint script incorrectly.
Also squashed all the commits

@camdenorrb
Copy link
Author

Dolphin ci hates me 😕

@BhaaLseN
Copy link
Member

ogl-lin-mesa looks like a configuration issue to me, since it's running Vulkan instead of OpenGL. vk-lin-mesa though, that's missing extensions. It tries to load the DRM extensions, and fails.

@camdenorrb
Copy link
Author

Thank you @BhaaLseN

Copy link
Member

@BhaaLseN BhaaLseN left a comment

Choose a reason for hiding this comment

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

I'm not really the best choice for reviewing graphics code, but it looks pretty good style-wise. Just that one duplicate line that feels off to me.

Source/Core/VideoBackends/Vulkan/VKSwapChain.cpp Outdated Show resolved Hide resolved
@dreamsyntax
Copy link
Member

@camdenorrb squash the comment removal commit and you should be good to merge assuming other graphics approvals

@camdenorrb
Copy link
Author

@dreamsyntax done, not sure how to get graphics approvals

Comment on lines -60 to +63
PRIVATE
${CMAKE_SOURCE_DIR}/Externals/Vulkan-Headers/include
${CMAKE_SOURCE_DIR}/Externals/VulkanMemoryAllocator/include
${CMAKE_SOURCE_DIR}/Externals/libadrenotools/include
PRIVATE
${CMAKE_SOURCE_DIR}/Externals/Vulkan-Headers/include
${CMAKE_SOURCE_DIR}/Externals/VulkanMemoryAllocator/include
${CMAKE_SOURCE_DIR}/Externals/libadrenotools/include
Copy link
Member

Choose a reason for hiding this comment

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

I don't disagree with this formatting change, but since there are no other changes besides formatting I'm not sure it should be in this PR.
Nitpick though, leave it or not. I have no preference, but a graphics reviewer might.

@@ -13,6 +13,7 @@ enum class WindowSystemType
Wayland,
FBDev,
Haiku,
DRM,
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why this is highlighted in blue, its not a C++ keyword. Anyone know?

Copy link
Author

Choose a reason for hiding this comment

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

Probably just a syntax highlighting glitch

@@ -36,6 +36,7 @@ static void ResetVulkanLibraryFunctionPointers()
#define VULKAN_INSTANCE_ENTRY_POINT(name, required) name = nullptr;
#define VULKAN_DEVICE_ENTRY_POINT(name, required) name = nullptr;
#include "VideoBackends/Vulkan/VulkanEntryPoints.inl"

Copy link
Member

Choose a reason for hiding this comment

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

Same comment as my other, there are no changes to VulkanLoader.cpp other than formatting.

Comment on lines +44 to +45
// TODO: Is this sleep appropriate?
std::this_thread::sleep_for(std::chrono::milliseconds(1));
Copy link
Member

Choose a reason for hiding this comment

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

I am interested to hear those more experienced with Dolphin who can weigh on for this.
Once finalized I can do a bunch of user testing of this branch.

Copy link
Author

Choose a reason for hiding this comment

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

This is just copied and pasted from a different platform

@camdenorrb
Copy link
Author

@mitaclaw any thoughts on this PR?

@dreamsyntax
Copy link
Member

dreamsyntax commented Jan 8, 2025

@camdenorrb am I understanding correctly this makes the below:
#11257
#8727

Obsolete?
Oh wait, no OpenGL and SR would still not work.

@camdenorrb
Copy link
Author

@camdenorrb am I understanding correctly this makes the below:

#11257

#8727

Obsolete?

Oh wait, no OpenGL and SR would still not work.

@dreamsyntax those both look wayland specific, so shouldn't make it obsolete

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

Successfully merging this pull request may close these issues.

5 participants