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

Wayland support #4075

Closed
wants to merge 6 commits into from
Closed

Wayland support #4075

wants to merge 6 commits into from

Conversation

bmwalters
Copy link

These patches add support for running PCSX2 natively under Wayland on Linux. The launcher script no longer forces GDK_BACKEND=x11.

The main changes are as follows (also see the individual commits):

  • Implemented the previously-stubbed GSWndEGL_Wayland
    • The bulk of the changes are for supporting the deprecated GSopen (GSopen1) interface which creates a standalone window.
    • The implementation of AttachNativeWindow expects a wl_egl_window to be passed to GS from the GUI. This is created in GSFrame in order to keep a reference to the wl_egl_window around to support resizing it in the wx OnResize.
  • Updated PAD/Linux to only call into X11 libraries when GS is using an X11 window.

I have successfully used these changes when running the Sway compositor, with rendering, resizing, keyboard input, and gamepad input working as expected.

Copy link
Member

@GovanifY GovanifY left a comment

Choose a reason for hiding this comment

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

Hi, sway user there, thanks a lot for the PR!

I did not find any big issues with your sway implementation, mostly superfluous stylistic and implementation changes.
Light testing also proved to work well.

Thanks for the work!!

@@ -154,6 +157,11 @@ if(UNIX)
include_directories(${GTK2_INCLUDE_DIRS})
endif()

if(WAYLAND_FOUND)
Copy link
Member

Choose a reason for hiding this comment

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

&& WAYLAND_PROTOCOLS_FOUND maybe?

Copy link
Author

Choose a reason for hiding this comment

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

WAYLAND_INCLUDE_DIRS and WAYLAND_DEFINITIONS are actually unrelated to wayland-protocols.

The only current usage of wayland-protocols is in the implementation of the old (deprecated?) GSopen (aka GSopen1) API in which the GS plugin creates its own native window instead of taking over a window given to it by core. It uses the xdg-shell protocol from wayland-protocols to handle setting the window title, and as currently unimplemented, resizing. (I focused my development efforts on the non-deprecated GSopen2 API).

So I think that we should only condition on wayland-protocols in the cmake code that uses it directly (only the WAYLAND_ADD_PROTOCOL_CLIENT macro's usage(s)).

I'm a cmake noob so I don't know how to get the best error message in case a dev tries to build without having wayland-protocols. Would adding an if(WAYLAND_PROTOCOLS_FOUND) here be the best way to do it? Or would it generate hard-to-understand error messages since the .c and .h protocol files would not be generated?
https://github.com/PCSX2/pcsx2/pull/4075/files#diff-273e7a9b69994f653862c746a10ed357c053b6807756d7878de4c06879f2a1adR225

Copy link
Member

Choose a reason for hiding this comment

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

your "FindX" stuff should simply error out if a dependency isn't found, and you'll add a check required dep function in the general cmake file.

GSOpen1 will eventually be completely removed so supporting it isn't necessarily necessary, indeed. I don't think we even use it anymore.

Copy link
Member

Choose a reason for hiding this comment

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

We do use GSOpen1 for GSDumpGUI

@@ -0,0 +1,13 @@
#pragma once
Copy link
Member

Choose a reason for hiding this comment

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

Why in common/? We won't reuse it outside of the displaying system, will we?

Copy link
Author

Choose a reason for hiding this comment

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

It's referenced by gui as you mention and also once by PAD (since PAD has per-display-system input polling). It will also be used by GSdx once it's merged (the struct is currently duplicated into GSdx code).

Copy link
Member

Choose a reason for hiding this comment

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

PAD is currently a part of the core as much as GUI is. common is used when libraries are used in plugins && core && utilities or any combination of it. Eventually this will be a part of the frontend part of the code but that's a bit far ahead, so you can move it into gui for now

// Inspect the provided display handle to check if it is Wayland or X11.
// The Wayland GS display handle happens to set pDsp[1] to NULL, so check it.
bool is_wayland_display = display_handle == nullptr
|| ((void **)display_handle)[1] == nullptr;
Copy link
Member

Choose a reason for hiding this comment

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

Why not use a struct there? Would be more appropriate

Copy link
Author

Choose a reason for hiding this comment

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

I used a struct for this in PAD but didn't want to duplicate the code into GSdx since it'll be merged. After the GSdx merge a ton of indirection can be removed since we won't need to marshal window handles through the pDsp pointer anymore.

Copy link
Member

Choose a reason for hiding this comment

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

You can always duplicate the struct for now and I'll remove it when I'll do the merge, this way I don't end up forgetting about it and casting structures

@GovanifY GovanifY self-assigned this Jan 5, 2021
Copy link
Author

@bmwalters bmwalters 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 the review!

One other comment I wanted to make is that you'll notice resizing isn't frame-perfect right now (game content clips outside the GS window while dragging to resize). This will become a lot easier to fix once GSdx is merged.

Explanation under https://ppaalanen.blogspot.com/2013/11/sub-surfaces-now.html heading "Independent application sub-modules" explains the coordination between surface and subsurface owners to get frame-perfect resize.

if (GDK_IS_WAYLAND_WINDOW(draw_window))
{
pDsp[0] = (uptr)gsFrame->GetPluginDisplayPropertiesWaylandEGL();
pDsp[1] = (uptr)NULL;
Copy link
Author

Choose a reason for hiding this comment

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

NULL and nullptr both require a cast to uptr since uptr is an integer size type (size of a pointer) and not a pointer type.

#else
pDsp[0] = (uptr)gsFrame->GetViewport()->GetHandle();
pDsp[1] = NULL;
#endif
pDsp[1] = (uptr)NULL;
Copy link
Author

Choose a reason for hiding this comment

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

NULL and nullptr both require a cast to uptr since uptr is an integer size type (size of a pointer) and not a pointer type.

// Inspect the provided display handle to check if it is Wayland or X11.
// The Wayland GS display handle happens to set pDsp[1] to NULL, so check it.
bool is_wayland_display = display_handle == nullptr
|| ((void **)display_handle)[1] == nullptr;
Copy link
Author

Choose a reason for hiding this comment

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

I used a struct for this in PAD but didn't want to duplicate the code into GSdx since it'll be merged. After the GSdx merge a ton of indirection can be removed since we won't need to marshal window handles through the pDsp pointer anymore.

wl_egl_window creation is in GSFrame for resize support.
This case should be covered by WXK_ALT anyway.

wxGetKeyState on Wayland only supports modifier keys:
WXK_ALT, WXK_CONTROL, and WXK_SHIFT.

See wxWidgets/wxWidgets@9806582
This allows Wayland to be the default in supported environments.
@sjnewbury
Copy link

There seems to be a problem with scaling in High DPI (2x), at least in Sway.

@fantesykikachu
Copy link

This should probably be closed in favor of #4905.

@bmwalters bmwalters closed this Oct 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GS plugin crash -- "EGL: failed to get a window surface" Wayland Support
7 participants