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

extensions: Add EGL_EXT_platform_xcb. #112

Merged
merged 3 commits into from Oct 21, 2020
Merged

Conversation

yshui
Copy link
Contributor

@yshui yshui commented Aug 28, 2020

This enables GL applications to be written without any involvement of Xlib.

This is my first attempt ever at writing an EGL extension, I don't expect to have done everything right the first try. I hope I can get as much help as possible.

@fooishbar @evelikov @1ace suggestions?

Co-authored-by: Yuxuan Shui <yshuiv7@gmail.com>
@stonesthrow
Copy link
Contributor

@yshui Thank you for your PR. I will try to get some xcb reviewers for feedback.

@fooishbar
Copy link
Contributor

Text looks good to me.

@stonesthrow
Copy link
Contributor

@oddhack Please check this for proper reservation of tokens.

@stonesthrow
Copy link
Contributor

This looks good to me. It would be good to get a few more thumbs up from xcb users to represent EXT mutivendor extension.

@fooishbar
Copy link
Contributor

cc @cubanismo

@caramelli
Copy link
Contributor

caramelli commented Sep 16, 2020

Thank you for the proposal of this extension, I have been using it for many years (you can take a look at the yagears project https://github.com/caramelli/yagears which illustrates the use of EGL with XCB). But I never took the time to submit it to the Khronos Group ...
In addition, it gives consistency with the Vulkan WSI layer which supports both Xlib (VK_KHR_xlib_surface) and XCB (VK_KHR_xcb_surface)

Just note that eglplatform.h needs to be updated with something like this:

#elif defined(__unix__) || defined(USE_X11) || defined(USE_XCB)

#if defined(USE_XCB)

#include <xcb/xcb.h>

typedef xcb_connection_t *EGLNativeDisplayType;
typedef xcb_pixmap_t EGLNativePixmapType;
typedef xcb_window_t EGLNativeWindowType;

#else

/* X11 (tentative) */
#include <X11/Xlib.h>
#include <X11/Xutil.h>

typedef Display *EGLNativeDisplayType;
typedef Pixmap EGLNativePixmapType;
typedef Window EGLNativeWindowType;

#endif

Nicolas Caramelli

@yshui
Copy link
Contributor Author

yshui commented Sep 17, 2020

@caramelli

Just note that eglplatform.h needs to be updated with something like this:

No, xcb_connection_t * is not going to be accepted by eglGetDisplay

@caramelli
Copy link
Contributor

I don't understand why: eglGetDisplay accepts a NativeDisplayType
eglplatform.h should normally provide a way to define EGLNativeDisplayType, EGLNativePixmapType, EGLNativeWindowType for the XCB platform.

@yshui
Copy link
Contributor Author

yshui commented Sep 17, 2020

@caramelli To not complicate things.

eglGetDisplay doesn't know what is given to it, so it has to use less than elegant heuristics to guess the type of the NativeDisplay. Adding a new type will make it more complicated.

eglGetDisplay accepts a NativeDisplayType

xcb_connection_t is not NativeDisplayType

@caramelli
Copy link
Contributor

Yes, using EXT_platform_base with eglGetPlatformDisplayEXT () does not require knowing the native display type (compared to eglGetDisplay) because it is a "void *": EGL_EXT_platform_xcb will be used like that!
But when using a platform that only has XCB (for example on an embedded system), the build fails with this error: "Platform not recognized" (unless you set the flag EGL_NO_X11 ...)
So I think it makes sense to have a way to define EGLNativeDisplayType, EGLNativePixmapType, EGLNativeWindowType for XCB, it's also consistent with other platforms and EGL history.
But this is only my feedback and to share my point of view.
Anyway, thank you again for this extension proposal!

Nicolas Caramelli

@yshui
Copy link
Contributor Author

yshui commented Sep 27, 2020

Since the feedback is positive, I removed the draft status.

@yshui yshui marked this pull request as ready for review September 27, 2020 01:07
@stonesthrow
Copy link
Contributor

If we can get a couple reviews to represent the xcb community for EXT then it seems it would be good for merge.

@fooishbar
Copy link
Contributor

I'm more ex-11 than X11, but at least from a Mesa winsys point of view I'm perfectly happy with this extension after the semantics around Screen specification. Thanks @yshui!

extensions/EXT/EGL_EXT_platform_xcb.txt Outdated Show resolved Hide resolved
extensions/EXT/EGL_EXT_platform_xcb.txt Outdated Show resolved Hide resolved
must be a valid screen on the display connection. If the attribute's value
is not a valid screen, then an EGL_BAD_ATTRIBUTE error is generated.

[fn1] The method by which EGL creates a connection to the default X11
Copy link
Contributor

Choose a reason for hiding this comment

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

There may be a subtle interaction with EGL_NV_native_query here: Whatever method EGL uses, it must be capable of generating an xcb_connection_t to return to the app form it if EGL_NV_native_query is supported.

Copy link
Contributor Author

@yshui yshui Oct 13, 2020

Choose a reason for hiding this comment

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

This is the same language used in EXT_platform_x11 too.

Mesa stores a xcb_connection_t * so this won't be a problem for them. If other drivers don't do this, they probably have to choose between not supporting EXT_platform_xcb or not supporting NV_native_query.

Oh wait, this extension doesn't define a EGLNativeDisplayType, which means NV_native_query have to create a Xlib Display from a xcb_connection_t.

I think it's probably best to update NV_native_query to say, the native display can only be retrieved if a EGLNativeDisplayType is used to create the EGLDisplay.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not clear why that is. The native display type is a property of the EGLDisplay. NV_native_query implicitly returns an object of the type corresponding to the display's native display type, right?

I agree this is likely an issue in EXT_platform_x11 as well, but I think simply noting an interaction as you originally defined above (If you do something crazy, you can't support both extensions) in the "dependencies" section would be fine to resolve this, and then we could backport that to platform_x11.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NV_native_query explicitly specifies that the function would return a EGLNativeDisplayType, which is the type passed to eglGetDisplay. This extension doesn't define a EGLNativeDisplayType.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is a rough edge, but I think in practice EGLNativeDisplayType will always be a pointer, and is effectively void* in any implementation that supports multiple platforms, so very pedantic C implementations (are there any C implementations where pointer types actually differ besides between function pointers and data pointers?) aside, I would assume it's just a matter of casting EGLNativeDisplayType back to the appropriate type for your platform.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can take this interpretation. So I guess this is a non-issue then?

Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty much. I think it's worth mentioning as an interaction issue (using text similar to suggested above), but if you disagree, not a big deal. It's a pretty dark corner case.

extensions/EXT/EGL_EXT_platform_xcb.txt Outdated Show resolved Hide resolved
extensions/EXT/EGL_EXT_platform_xcb.txt Outdated Show resolved Hide resolved
@yshui
Copy link
Contributor Author

yshui commented Oct 13, 2020

Screen selection issue is addressed.

I still need to look into EGL_NV_native_query...

Copy link
Contributor

@cubanismo cubanismo left a comment

Choose a reason for hiding this comment

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

Approved from my POV, with or without a mention of the NV_native_query interaction.

@stonesthrow stonesthrow self-requested a review October 19, 2020 15:21
@stonesthrow stonesthrow added this to the Approved to Merge milestone Oct 19, 2020
@oddhack oddhack merged commit 187bd7f into KhronosGroup:master Oct 21, 2020
XZiar pushed a commit to XZiar/EGL-Registry that referenced this pull request Jan 22, 2022
* extensions: Add EGL_EXT_platform_xcb.

Co-authored-by: Yuxuan Shui <yshuiv7@gmail.com>

* Mark EXT_platform_xcb complete

* EGL_EXT_platform_xcb: clarify screen selection.
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.

None yet

7 participants