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

EGL: Implement interfaces for EGL_EXT_device_drm #1691

Merged
merged 1 commit into from
Sep 10, 2024
Merged

Conversation

MarijnS95
Copy link
Member

This extension (together with the optional EGL_EXT_device_drm_render_node extension) allows querying the DRM primary and render device node filenames from EGLDevice, to help associating them with DRM devices.

Additionally a platform display can be created with a DRM file descriptor with master permissions, but this is only useful in situations where EGL might fail to open a file descriptor itself or will fail to acquire master permissions (which are only required when the implementation needs to "modify output attributes": but no behaviour is defined by this extension that requires this).

This file descriptor will be duplicated by the implementation if it needs to use it beyond the call to eglGetPlatformDisplayEXT(), and does not need to be kept alive by the caller.

Note that EGL_EXT_output_drm is omitted for now, because it relies on the equally unimplemented EGL_EXT_output_base extension (neither of which are available on my hardware for testing).

  • Tested on all platforms changed
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality

glutin/src/api/egl/device.rs Outdated Show resolved Hide resolved
glutin/src/api/egl/device.rs Outdated Show resolved Hide resolved
glutin_examples/Cargo.toml Outdated Show resolved Hide resolved
@MarijnS95 MarijnS95 force-pushed the device-drm branch 2 times, most recently from 7b995a5 to 9cd6d4c Compare September 4, 2024 09:39
// SAFETY: We pass a valid EGLDevice pointer, and validated that the enum name
// is valid because the extension is present.
unsafe { Self::query_string(self.raw_device(), EGL_DRM_RENDER_NODE_PATH_EXT) }
.map(PathBuf::from)
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we let query_string return a &'static CStr or &'static OsStr since eglQueryDeviceStringEXT() is guaranteed to return a static string (which would disappear if the implementation library is unloaded...)?

Copy link
Member

Choose a reason for hiding this comment

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

Well, you can not unload the library and keep using anything created by glutin, since we never unload.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point (but the user could hold on to the 'static str). Perhaps I'll refactor it to return '_ for both query_string() and &Path here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done now, it's returning a lifetime constrained by &self.

By the way, for Device::from_ptr() is there a reason we pass a borrow of EGL when self::query_string() also borrows that global already?

Copy link
Member Author

Choose a reason for hiding this comment

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

Or right, since we have static EGL: Lazy this could actually be 'static?

Copy link
Member Author

Choose a reason for hiding this comment

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

We already did this for extensions_from_ptr() anyway :)

@MarijnS95
Copy link
Member Author

MarijnS95 commented Sep 4, 2024

Note that EGL_EXT_output_drm is omitted for now, because it relies on the equally unimplemented EGL_EXT_output_base extension (neither of which are available on my hardware for testing).

The output extensions only appear to be implemented on Nvidia drivers (and my laptop unfortunately has no outputs connected on the dedicated GPU), and don't seem to be used for anything else than creating EGLStreams which is a completely separate feature I don't plan on building in glutin, so I'll skip these extensions too.

Copy link
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

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

From what I can see, it doesn't have any breaking changes, since static is more broad lifetime here?

@MarijnS95

This comment was marked as resolved.

@kchibisov

This comment was marked as resolved.

@kchibisov
Copy link
Member

Also, we don't really have to add &'static str, since &str is more forward compat.

@MarijnS95

This comment was marked as resolved.

@MarijnS95
Copy link
Member Author

Also, we don't really have to add &'static str, since &str is more forward compat.

That is true, do you think we'll ever unload or replace the library?

@kchibisov
Copy link
Member

That is true, do you think we'll ever unload or replace the library?

I'm not sure about this, probably not, but having &'static str is more restricting to what we can do, so, maybe changing back to regular &str is better?

Like if someone has a need for &'static there they could probably request it, though, we do have 'static inside the extensions HashMap already, so that's why I don't really mind changing to 'static other stuff that behaves like that.

@MarijnS95
Copy link
Member Author

MarijnS95 commented Sep 9, 2024

More restrictive to what we can do, but less restrictive to what the caller can do with it once we've given a borrow to them. So yeah, I think having these names behave the same as the string borrows would be nice/consistent?

This extension (together with the optional
`EGL_EXT_device_drm_render_node` extension) allows querying the DRM
primary and render device node filenames from `EGLDevice`, to help
associating them with DRM devices.

Additionally a platform display _can_ be created with a DRM file
descriptor with master permissions, but this is only useful in
situations where EGL might fail to open a file descriptor itself or will
fail to acquire master permissions (which are only required when the
implementation needs to "modify output attributes": but no behaviour is
defined by this extension that requires this).

This file descriptor will be duplicated by the implementation if it
needs to use it beyond the call to `eglGetPlatformDisplayEXT()`, and
does not need to be kept alive by the caller.

Note that `EGL_EXT_output_drm` is omitted for now, because it relies on
the equally unimplemented `EGL_EXT_output_base` extension.
@kchibisov kchibisov merged commit f27377a into master Sep 10, 2024
43 checks passed
@kchibisov kchibisov deleted the device-drm branch September 10, 2024 15:27
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.

2 participants