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

[RFC] Add VK_EXT_acquire_wl_display #1001

Open
wants to merge 1 commit into
base: master
from

Conversation

@ddevault
Copy link
Contributor

commented Jul 11, 2019

This is my first time putting together a VK extension, so be prepared to find some mistakes herein. I appreciate your patience.

This extension fulfills a similar purpose to VK_EXT_acquire_xlib_display, but for Wayland. It relies on a Wayland extension which is currently under discussion on wayland-devel. The thread there provides more information about the specifics:

https://lists.freedesktop.org/archives/wayland-devel/2019-July/040707.html

I want to avoid creating a Vk extension which depends on an unstable Wayland protocol, as the structure names reflect their unstable nature and I would prefer not to immortalize the unstable version in this extension. Therefore, once consensus is reached on the Wayland protocol, we're thinking about fast-tracking it to stable. At that point, I'll update this patch accordingly.

I've also implemented this extension for Mesa and radv:

https://gitlab.freedesktop.org/mesa/mesa/merge_requests/1509

As well as for Sway and wlroots (Wayland compositor) with these patches:

swaywm/wlroots#1730

swaywm/sway#4289

And I have an xrgears tree as well which takes advantage of this to show a VR scene using these extensions:

https://git.sr.ht/~sircmpwn/xrgears

@CLAassistant

This comment has been minimized.

Copy link

commented Jul 11, 2019

CLA assistant check
All committers have signed the CLA.

@jekstrand jekstrand self-assigned this Jul 24, 2019

@jekstrand

This comment has been minimized.

Copy link

commented Jul 24, 2019

First step here is to submit a separate PR which just reserves a new extension number and doesn't add any real content or spec text. You can see a bunch of examples of that in the XML. That PR will get merged almost immediately. That way you have the extension number reserved while people are reviewing the spec so it won't change out from under you.

@ddevault

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2019

Can do, thanks!

@ddevault

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

Reserving the extension number here: #1006

@swick

This comment has been minimized.

Copy link

commented Aug 13, 2019

Why does the extension take in a wayland object and not the final lease fd? Taking only the fd would allow the extension to work no matter how you acquired the fd. Maybe there are advantages by taking in the wayland objects but I can't find them so I'd appreciate a rationale.

@ddevault

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

Because this extension is designed in a manner consistent with the rest of WSI, including the Wayland bits. Refactoring the entire Vulkan WSI API around a different model is well out of scope for this patch and a fool's errand anyway.

Please leave your grievances on Reddit, this is a professional and technical forum and I am not pleased to be hosting overflow discussions from non-stakeholders stirring up trouble on Reddit here.

@swick

This comment has been minimized.

Copy link

commented Aug 13, 2019

Because this extension is designed in a manner consistent with the rest of WSI, including the Wayland bits.

The point I was making is that it doesn't have to be wayland specific. E.g. VK_EXT_acquire_leased_display with

VkResult vkAcquireLeasedDisplayEXT(VkPhysicalDevice physicalDevice,
                                   int leaseFd,
                                   VkDisplayKHR *displayOut);

I'd like to know if this solution was considered and if so, what's the rationale to prefer the proposed solution.

Refactoring the entire Vulkan WSI API around a different model

Not proposing that. Never did.

Please leave your grievances on Reddit, this is a professional and technical forum and I am not pleased to be hosting overflow discussions from non-stakeholders stirring up trouble on Reddit here.

Stop being condescending.

@ddevault

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

I apologize to the Vulkan membership for the disruption. By way of explanation, I recently wrote a blog post summarizing the work around this feature that took place throughout the ecosystem, and linked to this Vulkan patch there. I had hoped it would serve as a useful reference for understanding the feature. Unfortunately, after trying to deal with bad-faith arguments from Wayland naysayers in the resulting discussions, it served to lead trolls to this forum. I've removed the link from my article to steer off additional noise.

Stop being condescending.

Stop dragging your dead horse around the internet. You knew the answers to your questions before you came here, because I explained them to you in another forum. You've come to disrupt the discussion here and I'll thank you for desisting.

@oddhack

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

Speaking as one of the people who actually is responsible for operating the repository, I'd appreciate it if both of you would step back. Please refer to the Khronos code of conduct at https://www.khronos.org/events/code-of-conduct .

Apparently some design decisions were made that not everyone is on board with. For a vendor extension that's fine if the vendor is going to support it, and for an EXT extension that's fine if multiple implementations are going to support it.

@swick

This comment has been minimized.

Copy link

commented Aug 13, 2019

Apparently some design decisions were made that not everyone is on board with.

Maybe there is a good reason for the design. I'm trying to figure that out but I'm met with this incredible hostility and I'm not getting a clear answer either.

Just here he's implying that I'm asking the questions in "bad-faith", that I'm trying to "disrupt the discussion" (which discussion btw?), that I'm not a professional and not a stakeholder.

@cubanismo
Copy link

left a comment

Good to see this getting picked up. I went and looked up the blog post you referenced while reviewing, and since it helped establish context for me, I hope you don't mind if I link it here:

https://drewdevault.com/2019/08/09/DRM-leasing-and-VR-for-Wayland.html

I've left some comments inline.


include::{generated}/api/structs/VkWaylandLeaseConnectorEXT.txt[]

* pname:pConnectorIn A Wayland zwp_drm_lease_connector_v1 to include in the

This comment has been minimized.

Copy link
@cubanismo

cubanismo Aug 13, 2019

Generally Vulkan struct members aren't decorated with In/Out suffixes.

ename:VK_ERROR_INITIALIZATION_FAILED.

If the resulting DRM lease does not include a slink:VkDisplayKHR corresponding
to pname:pConnectorIn and more than one connector was requested (meaning

This comment has been minimized.

Copy link
@cubanismo

cubanismo Aug 13, 2019

Seems like something is missing here. Was this sentence/paragraph included intentionally?

to pname:pConnectorIn and more than one connector was requested (meaning

If there is no slink:VkDisplayKHR corresponding to pname:pConnectorIn on
pname:physicalDevice, dlink:VK_NULL_HANDLE must: be returned in pname:pDisplay.

This comment has been minimized.

Copy link
@cubanismo

cubanismo Aug 13, 2019

Does this indicate a failure? I'm not sure how to resolve this with the INITIALIZATION_FAILED error describe above.

* pname:physicalDevice The physical device the display is on.
* pname:display A wl_display connected to the Wayland compositor that
currently owns pname:display.
* pname:manager A zwp_drm_lease_manager_v1 connected to the Wayland compositor

This comment has been minimized.

Copy link
@cubanismo

cubanismo Aug 13, 2019

The corresponding Xlib Vulkan extension was purposely written in a manner that avoided tying it to DRM or other vendor-specific drivers. That's why we didn't just include a lease FD in the WSI extension there as Sebastian is suggesting here (Though that would have worked fine for NV and DRM drivers), and it makes me wonder if including DRM-specific wayland protocol structs in this extension is the right direction. For Wayland, both NV and Mesa stacks use DRM for display enumeration purposes right now, but it's been pointed out to me, and I've pointed out before that one could theoretically write a Wayland implementation entirely on top of Vulkan D2D, OpenWF, etc. Hence, I'd prefer not to bake current de-facto standards/assumptions into the Vulkan spec if possible.

As a straw-man for discussion, would it make sense to instantiate the leasing manager object inside the Vulkan WSI implementation instead and reference the displays directly via their Vulkan handles at the API level, or is that problematic? FWIW, the additional "translate RandR to VkDisplayKHR" entry point fell out of a similar design decision on the X11 side.

This comment has been minimized.

Copy link
@pH5

pH5 Aug 13, 2019

Picking up the straw-man, I see how the WSI could try to open a Wayland connection and enumerate the leasable connectors internally, and present them via vkGetPhysicalDeviceDisplayProperties2KHR, without any Wayland specific extension. Whether or not that would be in the spirit of the Vulkan API, I don't know.

One issue with the display enumeration API is that VkDisplayProperties2KHR only provides a poorly specified displayName ("Generally, this will be the name provided by the display's EDID") to identify the display. It would be very useful to be able to match on specific EDID properties such as vendor and product id, or even serial number, to find a specific VR headset.

Another issue is that it is unclear at which point the WSI would actually request the lease. The KHR_display extension itself doesn't provide any kind of Acquire/Release functionality.

And I think the Vulkan model of enumerating planes upfront doesn't fit well with DRM leasing, as the lessee currently has no say in the matter and only is made aware of the available planes after the lease has been issued.

This comment has been minimized.

Copy link
@ddevault

ddevault Aug 13, 2019

Author Contributor

The Wayland protocol provides an out of band way to get things like the EDID for doing display selection. Splitting the "RandR -> VkDisplayKHR" step into a separate one has actually already caused us headaches implementing this for Xwayland. The design of all of these subsystems - DRM, Wayland, etc - does not lend itself well to a design which does this in two steps, at least for Wayland in particular.

This comment has been minimized.

Copy link
@swick

swick Aug 13, 2019

Thanks @cubanismo for providing the context and rationale.

So one problem I have with this argument is that there is no other "leasing" mechanism on linux that I'm aware of but there is multiple windowing systems and we're already at the second extension here. I can easily see the proposed wayland extension not getting adopted (mainly due to missing authentication support, I can expand on that if wanted) but e.g. a dbus interface. Is there any realistic reason why someone would support a "leasing" mechanism that's not based on DRM? Would that ever hit mainline? If it doesn't I really don't care. Either way, if someone invents another "leasing" mechanism they could also invent a new vk extension.

To recap: there already are different ways to get a lease fd but there only is one leasing mechanism.

The staw-man would not allow for e.g. receiving the fd via dbus, it only abstracts away the specific wayland protocol so I don't see much value in that. All solutions seem to have a draw-back.

I'd prefer not to bake current de-facto standards/assumptions into the Vulkan spec if possible

I don't think that's a fair argument. If we explicitly say in the extension that the fd is a lease fd acquired from kms/drm then it's not an assumption anymore.

In the same spirit I could argue that the xlib acquire extension bakes de-facto standards into the Vulkan spec or that this wayland proposal bakes in a not-even-de-facto standard. drm/kms leasing is at least a de-facto standard.

I hope this makes my concerns understandable.

This comment has been minimized.

Copy link
@ddevault

ddevault Aug 13, 2019

Author Contributor

Whether or not this protocol will be adopted is out of scope for here, we can discuss the merits of the protocol on the mailing list. It has the explicit support of several stakeholders. This patch is not suitable for merge before it has been standardized anyway, as has already been outlined in the original post.

There is no possibility whatsoever of a dbus protocol being preferred here. There will be a hard nack and a bloody fight over that, but thankfully the only person asking for it is someone with no horse in the race.

This comment has been minimized.

Copy link
@ddevault

ddevault Aug 13, 2019

Author Contributor

Your preference has no bearing on the matter. You're incorrect and I don't care to further argue with you about the security of Wayland - this is not the place.

I would really appreciate it if you stopped derailing this discussion.

This comment has been minimized.

Copy link
@swick

swick Aug 13, 2019

You're just trolling me at this point. There is a CoC, behave appropriately.

This comment has been minimized.

Copy link
@cubanismo

cubanismo Aug 14, 2019

Picking up the straw-man, I see how the WSI could try to open a Wayland connection and enumerate the leasable connectors internally, and present them via vkGetPhysicalDeviceDisplayProperties2KHR, without any Wayland specific extension.

See below. All the window-system integration (WSI) extensions were designed to avoid drivers creating internal secret connections to the windowing systems, or even using app-provided connections outside of the scope of the APIs that take those native objects as parameters, directly or indirectly via a VkSurface reference. Everything that isn't WSI isn't supposed to even know a window system exists. If it doesn't reference a native window system directly, via a VkSurfaceKHR object directly, or via a swapchain derived from a VkSurfaceKHR object, it's intended to be window-system agnostic, and be implemented on top of client-side driver functionality (E.g., ioctls on device nodes, sysfs inspection, etc.) only.

One issue with the display enumeration API is that VkDisplayProperties2KHR only provides a poorly specified displayName ("Generally, this will be the name provided by the display's EDID") to identify the display. It would be very useful to be able to match on specific EDID properties such as vendor and product id, or even serial number, to find a specific VR headset.

The core display enumeration API is indeed extremely minimalist on purpose. It is, however, trivially extensible. If you'd like to add an extension that returns other properties of displays, I think that would be a great idea.

Another issue is that it is unclear at which point the WSI would actually request the lease. The KHR_display extension itself doesn't provide any kind of Acquire/Release functionality.

Right. Displays aren't intended to be enumerated from a window system. They're supposed to be enumerated from the device driver directly. There has been much debate over whether this meshes well with DRM's by-design inability to enumerate displays associated with a render node, whether an application should even care about the physical displays associated with a render node/VkPhysicalDevice Vs. the displays available to it via a window system, etc.

The current design of KHR_display and the derivative leasing mechanisms is that the display leases are needed only to set a mode and present to a display, not to enumerate its properties. However, I'd be happy to discuss any proposals that make this play better with DRM semantics. I don't have a proposal of my own here at the moment.

And I think the Vulkan model of enumerating planes upfront doesn't fit well with DRM leasing, as the lessee currently has no say in the matter and only is made aware of the available planes after the lease has been issued.

That's an interesting note, and ties into the above response somewhat. Always happy to hear suggestions to improve the Vulkan display APIs.

The Wayland protocol provides an out of band way to get things like the EDID for doing display selection. Splitting the "RandR -> VkDisplayKHR" step into a separate one has actually already caused us headaches implementing this for Xwayland. The design of all of these subsystems - DRM, Wayland, etc - does not lend itself well to a design which does this in two steps, at least for Wayland in particular.

I don't fully follow the complications in the linked issue. Is the issue that Xwayland provides inaccurate modes for the connector and hence the application can't reliably build up its mode list prior to acquiring the lease? Why isn't this considered a bug in Xwayland? I recall there was considerable debate over Xrandr behavior in Xwayland environments, but I admit I didn't read most of it. Perhaps I'm kicking dead horses.

However, I don't think the VkDisplayKHR-on-Xrandr-on-Xwayland path nor the new lease extension you're designing here need to be confined to Xwayland's RandR behavior. You're defining custom wayland protocol already. Presumably that protocol could be used to tunnel an accurate mode list, EDID, etc. back to the Vulkan implementation. Xwayland could use this path rather than its usual RandR implementation when sending modes to the Vulkan driver as opposed to general RandR client applications.

BTW, I don't know how valid the concerns in the other issue about current behavior being baked into games are. From my understanding, this behavior is only baked into VR intermediary libraries like the SteamVR runtime, which are likely much more easily updated. However, this is speculation. We'd have to go check the code/ask the platform owners. I for one wouldn't be opposed to a larger re-design if there's a more suitable API for leasing, assuming the users would be on board.

To recap: there already are different ways to get a lease fd but there only is one leasing mechanism.

There are two. NVIDIA has a different leasing mechanism than DRM. It still uses FDs to represent the lease. For Wayland, it's not clear yet, but we'd likely use the DRM leasing mechanism even in the NVIDIA proprietary driver, so we're interested in the semantics of both. How we behave in a hypothetical future where our X DDX is running in an Xwayland instance without direct access to our display hardware, but where we still want to maintain compatibility with applications that rely on our prior X DDX behavior is also an interesting line of thought.

In general, I'm rarely swayed by any reasoning of the form "If it doesn't exist now, on Linux, let's pretend it never will, anywhere," especially when it comes to designing extensible APIs that have a habit of being baked into applications that are difficult to update. I noted above that we may still have a window of time to modify the existing Vulkan leasing APIs without major repercussion if there's sufficient need, but that window could close at any time, so I'd rather not rely on it as a design principle.

This comment has been minimized.

Copy link
@swick

swick Aug 14, 2019

In general, I'm rarely swayed by any reasoning of the form "If it doesn't exist now, on Linux, let's pretend it never will, anywhere,"

The same argument is true for yet another way to receive a lease fd. The question here boils down to this: do we want to abstract away to way your receive the fd or do we want to abstract away what you receive.

Both could change and both already seem to have two answers (x11 vs wayland and drm fd vs nvidia fd).

This comment has been minimized.

Copy link
@pH5

pH5 Aug 24, 2019

See below. All the window-system integration (WSI) extensions were designed to avoid drivers creating internal secret connections to the windowing systems, or even using app-provided connections outside of the scope of the APIs that take those native objects as parameters, directly or indirectly via a VkSurface reference. Everything that isn't WSI isn't supposed to even know a window system exists.

Thank you for explaining the intention in detail, I never got that just from reading the spec. Assuming that indeed the only way to get a wl_display into Vulkan as intended would be via vkCreateWaylandSurfaceKHR, it seems weird then that an application that doesn't want to render into a Wayland surface at all would have to create an otherwise unused Wayland surface, just to be able to request a lease from the compositor.

If it doesn't reference a native window system directly, via a VkSurfaceKHR object directly, or via a swapchain derived from a VkSurfaceKHR object, it's intended to be window-system agnostic, and be implemented on top of client-side driver functionality (E.g., ioctls on device nodes, sysfs inspection, etc.) only.

So the idea is that vkGetPhysicalDeviceDisplayPropertiesKHR lists all displays, even those that can't be used (because the compositor won't lease them to us)? Similarly, since it is the Wayland compositor's decision which planes to lease to us, vkGetDisplayPlaneSupportedDisplaysKHR can't possibly know which hardware planes are available before the lease is requested.

One issue with the display enumeration API is that VkDisplayProperties2KHR only provides a poorly specified displayName ("Generally, this will be the name provided by the display's EDID") to identify the display. It would be very useful to be able to match on specific EDID properties such as vendor and product id, or even serial number, to find a specific VR headset.

The core display enumeration API is indeed extremely minimalist on purpose. It is, however, trivially extensible. If you'd like to add an extension that returns other properties of displays, I think that would be a great idea.

Ok, I see this is not an issue / out of scope in this context.

Another issue is that it is unclear at which point the WSI would actually request the lease. The KHR_display extension itself doesn't provide any kind of Acquire/Release functionality.

Right. Displays aren't intended to be enumerated from a window system. They're supposed to be enumerated from the device driver directly.

Say the VkDisplayKHR are enumerated from a DRM modesetting node if available. Given such a display, which is not leased yet, at what point should the actual lease be requested? When creating the display surface? When creating the swapchain? When presenting to it for the first time, as that's probably what triggers the modeset?

There has been much debate over whether this meshes well with DRM's by-design inability to enumerate displays associated with a render node, whether an application should even care about the physical displays associated with a render node/VkPhysicalDevice Vs. the displays available to it via a window system, etc.

Good point, what if the application does not have access to a modesetting node at all until it is provided with one via the leasing mechanism?

The current design of KHR_display and the derivative leasing mechanisms is that the display leases are needed only to set a mode and present to a display, not to enumerate its properties. However, I'd be happy to discuss any proposals that make this play better with DRM semantics. I don't have a proposal of my own here at the moment.

How about an extension structure to VkDisplaySurfaceCreateInfoKHR that allows to pass a Wayland VkSurfaceKHR (which internally provides the wl_display) to request the lease from?

@krOoze
Copy link
Contributor

left a comment

Some (mostly style) notes:

*Contributors*::
- Drew DeVault, Status Holdings, Ltd.

This extension allows an application to take exclusive control on a display

This comment has been minimized.

Copy link
@krOoze

krOoze Aug 13, 2019

Contributor
Suggested change
This extension allows an application to take exclusive control on a display
This extension allows an application to take exclusive control of a display

This extension allows an application to take exclusive control on a display
currently associated with a Wayland compositor.
When control is acquired, the display will be deassociated from the Wayland

This comment has been minimized.

Copy link
@krOoze

krOoze Aug 13, 2019

Contributor

"unassociated"?

This comment has been minimized.

Copy link
@rpavlik

rpavlik Aug 13, 2019

dis-associated?

This comment has been minimized.

Copy link
@krOoze

krOoze Aug 13, 2019

Contributor

Sounds like a medical condition. :D
More common spelling is "dissociated", I think.

This extension allows an application to take exclusive control on a display
currently associated with a Wayland compositor.
When control is acquired, the display will be deassociated from the Wayland
compositor until control is released or the specified display connection is

This comment has been minimized.

Copy link
@krOoze

krOoze Aug 13, 2019

Contributor
Suggested change
compositor until control is released or the specified display connection is
compositor until the control is released or the specified display connection is

include::{generated}/api/protos/vkAcquireWaylandDisplayEXT.txt[]

* pname:physicalDevice The physical device the display is on.

This comment has been minimized.

Copy link
@krOoze

krOoze Aug 13, 2019

Contributor

The parameter typically is part of the sentence in the descriptions, e.g.

pname:physicalDevice is the physical device the display is on.

Also typically things are spelled out in full, e.g.

pname:physicalDevice is the handle to the physical device the display is on.

Same below.

include::{generated}/api/protos/vkAcquireWaylandDisplayEXT.txt[]

* pname:physicalDevice The physical device the display is on.
* pname:display A wl_display connected to the Wayland compositor that

This comment has been minimized.

Copy link
@krOoze

krOoze Aug 13, 2019

Contributor

Foreign API is still marked up, e.g. code:wl_display. Same below.

This comment has been minimized.

Copy link
@rpavlik

rpavlik Aug 13, 2019

typically this will be sname:wl_display for a structure-ish type, but it's probably not critical. (Just don't use the *link: markup macros, those are for internally-defined API only.)

This comment has been minimized.

Copy link
@krOoze

krOoze Aug 13, 2019

Contributor

Nope, sname is for Vulkan types.
Nothing is critical, exept death and taxes.

See this

pname:display must: point to a valid Wayland code:wl_display.

This comment has been minimized.

Copy link
@rpavlik

rpavlik Aug 14, 2019

Ah, thanks for the clarification. The usage isn't entirely consistent, and differs slightly from the OpenXR usage (my "native" WG).

This comment has been minimized.

Copy link
@krOoze

krOoze Aug 14, 2019

Contributor

Seems like we need a specification for making specifications :p

This comment has been minimized.

Copy link
@rpavlik

rpavlik Aug 14, 2019

Well, there's the style guide, but the curse of such documents is that it's typically always slightly out of date. This is especially true for OpenXR which started with a Vulkan fork and isn't quite updated yet (and thus isn't fully public yet)

ename:VK_ERROR_INITIALIZATION_FAILED.

If the resulting DRM lease does not include a slink:VkDisplayKHR corresponding
to pname:pConnectorIn and more than one connector was requested (meaning

This comment has been minimized.

Copy link
@krOoze

krOoze Aug 13, 2019

Contributor

broken sentence. You probably forgot to finish some thought.

leased by the Wayland compositor to the appropriate pname:pConnectorIn. If no
match is found and the result would be ambiguous, pname:pDisplayOut is set to
dlink:VK_NULL_HANDLE, the lease is terminated, and
flink:VkAcquireWaylandDisplayEXT must: return

This comment has been minimized.

Copy link
@krOoze

krOoze Aug 13, 2019

Contributor

wrong function name (capital "V")

This comment has been minimized.

Copy link
@rpavlik

rpavlik Aug 13, 2019

This was also caught by scripts/check_spec_links.py which is worth running in the process. (It's run in the Khronos private CI)

to pname:pConnectorIn and more than one connector was requested (meaning

If there is no slink:VkDisplayKHR corresponding to pname:pConnectorIn on
pname:physicalDevice, dlink:VK_NULL_HANDLE must: be returned in pname:pDisplay.

This comment has been minimized.

Copy link
@krOoze

krOoze Aug 13, 2019

Contributor

You already said this above, I think.

// Forward declared for VK_EXT_acquire_wl_display
struct zwp_drm_lease_manager_v1;
struct zwp_drm_lease_connector_v1;
#endif // VK_USE_PLATFORM_WAYLAND_KHR

This comment has been minimized.

Copy link
@krOoze

krOoze Aug 13, 2019

Contributor

Hmm, so far we have been including stuff directly with headers. Though TBH, I would prefer your way, instead of dragging in unnecessary header dependencies.

This comment has been minimized.

Copy link
@cubanismo

cubanismo Aug 14, 2019

Yes, I prefer forward declarations as well when feasible.

This comment has been minimized.

Copy link
@krOoze

krOoze Aug 14, 2019

Contributor

Nonetheless this kinda circumvents the vk.xml autogenerating system.

This comment has been minimized.

Copy link
@rpavlik

rpavlik Aug 14, 2019

If there's interest in adding features for forward-declaration of platform types, I'd be interested too on the OpenXR side. I am pretty sure it actually can be done: I think I managed to get an include added without changing any code, just xml.

This comment has been minimized.

Copy link
@krOoze

krOoze Aug 14, 2019

Contributor

It has been "solved" a bit here, when I complained about dragging in the whole monolithic windows.h. The solution arrived at here is that vulkan.h is supposed to just include everything (using VK_USE_PLATFORM_*), and if you want to control includes you are supposed to be including the vulkan_*.h stuff directly.

Anyway we are offtopicing a bit here. I suggest you start an Issue.
A precedent is to create a new platform (e.g. as was done with VK_USE_PLATFORM_XLIB_XRANDR_EXT). AIS forward declarations are fine with me, then again this is not my repo, and my opinion does not ultimately matter.

This comment has been minimized.

Copy link
@cubanismo

cubanismo Aug 15, 2019

I don't have a strong opinion, but I recall the way I did it with XLIB_XRANDR caused some problems as well. I don't remember exactly why.

In general, there was a lot of time pressure behind the VR-related extensions, and we didn't have as much time for discussion and especially finessing of these infrastructure/backend details as I would have liked, so stick to first principles when deciding whether to carry their design tropes forward.

<param>struct <type>wl_display</type>* <name>display</name></param>
<param>struct <type>zwp_drm_lease_manager_v1</type>* <name>manager</name></param>
<param><type>int</type> <name>nConnectors</name></param>
<param><type>VkWaylandLeaseConnectorEXT</type>* <name>pConnectors</name></param>

This comment has been minimized.

Copy link
@krOoze

krOoze Aug 13, 2019

Contributor

We use uint32_t type for array length.
Also count variable would be named like connectorCount.
Also missing len="queueCount".

@rpavlik
Copy link

left a comment

As a bit of a tooling nut, I tried running "check spec links" on this and found some issues: some duplicated from the manual reviews, some that had slipped thru.

leased by the Wayland compositor to the appropriate pname:pConnectorIn. If no
match is found and the result would be ambiguous, pname:pDisplayOut is set to
dlink:VK_NULL_HANDLE, the lease is terminated, and
flink:VkAcquireWaylandDisplayEXT must: return

This comment has been minimized.

Copy link
@rpavlik

rpavlik Aug 13, 2019

This was also caught by scripts/check_spec_links.py which is worth running in the process. (It's run in the Khronos private CI)


The sname:VkWaylandLeaseConnectorEXT structure is defined as:

include::{generated}/api/structs/VkWaylandLeaseConnectorEXT.txt[]

This comment has been minimized.

Copy link
@rpavlik

rpavlik Aug 13, 2019

You have an opening of a refpage for VkWaylandLeaseConnectorEXT, and the generated API include, but no generated validity include. script/check_spec_links.py chapters/VK_EXT_acquire_wl_display/acquire_wl_display.txt appendices/VK_EXT_acquire_wl_display.txt (which I recommend running) reports:

chapters/VK_EXT_acquire_wl_display/acquire_wl_display.txt:47:0: warning: Saw /api/ include for VkWaylandLeaseConnectorEXT, but no matching /validity/ include (-Wmissing_validity_include)
                                                                          Expected a line with include::{generated}/validity/structs/VkWaylandLeaseConnectorEXT.txt[]

include::{generated}/api/structs/VkWaylandLeaseConnectorEXT.txt[]
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

=== New Structures

* flink:VkWaylandLeaseConnectorEXT

This comment has been minimized.

Copy link
@rpavlik

rpavlik Aug 13, 2019

Structures should be marked with slink: - caught by check_spec_links.

@ddevault

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

With respect to explicitly mentioning DRM here, I've been thinking over it and I don't have a great suggestion for an alternative. I'm open to suggestions.

The problem is that the Wayland protocol definitely needs to mention DRM, because its purpose is to hand a file descriptor to the client and the client needs to know that once it receives it, it's expected to do DRM ioctls on it. There are advantages to having the client do the bulk of the negotiation here, for example, it allows them to seek out the EDID they want. They also already have the Wayland display opened and with this design we can avoid doing a bunch of nonsense in the implementation with multiple event queues and so on, like Mesa has to do for wayland-egl today.

Creating a VkDisplayKHR earlier to avoid this isn't possible because a lot of the information available from a VkDisplayKHR would not be available in that context (and proposals to change the Wayland protocol to accommodate for such a design were found to be untenable), and updating all of those things to optionally fail would be a breaking change. We ended up doing something for this for xserver but it was a big hack that no one is pleased with, and we settled on it because xserver is considered a legacy system - and we'd rather not make such concessions for anything we intend to support into the future, such as Wayland or Vulkan.

This protocol is optional from all parties - the client doesn't have to use the Vk extension, the Vk implementation doesn't have to provide it, the compositor doesn't have to offer it - so it won't prevent additional Wayland implementations which are not DRM-based from supporting Vulkan (though they won't support this particular feature).

Additionally, DRM has been around for decades now. I don't think we're going to see it left behind for another system any time soon, and future versions of DRM will still be fundamentally similar in the respects important here - that this process will involve transferring a file descriptor to the client. And even if it does become stale, what of it? Extensions become stale all the time. All of the X11 WSI extensions, for example, will slowly become obsolete as Wayland takes over.

@swick

This comment has been minimized.

Copy link

commented Aug 14, 2019

Well, at least I agree with drew's opinion that the dependency on DRM is fine. I still don't get why there is a dependency on the specific wayland protocol when it would work just fine without it (especially considering that the wayland protocol itself is tied to DRM so the vk ext also depends on DRM already).

@cubanismo

This comment has been minimized.

Copy link

commented Aug 14, 2019

Yeah, I'm not opposed to an explicit DRM dependency if the underlying usage requires it. Let's bottom out on the design for the actual leasing, and I think the DRM dependency, or lack thereof, will fall out naturally from that.

My above inline reply/suggestions probably relate to this:

(and proposals to change the Wayland protocol to accommodate for such a design were found to be untenable)

I'd be curious to hear what those proposals were, and why they didn't work out.

All of the X11 WSI extensions, for example, will slowly become obsolete as Wayland takes over.

They may be deprecated, but I don't know about obsolete. I'll probably be playing my abandonware X11-only games on top of Xwayland for years to come. To move forward, there unfortunately usually has to be a backwards compatibility path. In my experience, keeping things abstracted where possible generally makes it easier to layer them on top of something new down the line, and hence in fact makes it easier to drop old stuff and move on to something better. I agree DRM is likely here to stay. However, the DRM leasing mechanism is still relatively new. Perhaps it will be adjusted or replaced. Perhaps that means a new set of Wayland and Vulkan extensions will be needed, but it doesn't necessarily need to be that way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.