Add connector_id to DrmWindowHandle #170

morr0ne · 2024-07-05T19:23:42Z

This pull request introduces a new field to the DRM display handle to specify the connector id.

Rationale:
Without a connector, the file descriptor may point to a DRM device that lacks any connected displays, rendering it non-functional and arguably not a valid handle. By adding the connector_id field, we ensure that the display handle accurately represents a connected and usable display.

Lokathor · 2024-07-05T19:30:14Z

Changing the signature of new is a breaking change, which we wouldn't want to do here. Adding the public field to a non-exhaustive struct is fine though.

Can we just add the field, as well as a doc comment if necessary, but not change any method signatures?

kchibisov · 2024-07-05T19:34:34Z

I don't think that connector should go there, because connector is display and I don't think any one stops you from drawing into device without connectors attached and e.g. copy to other GPU which has connectors attached?

Also, if multiples are present, which connector should be used?

morr0ne · 2024-07-05T19:48:28Z

I don't think that connector should go there, because connector is display and I don't think any one stops you from drawing into device without connectors attached and e.g. copy to other GPU which has connectors attached?

That's exactly why it should go here. If you want to draw to a display the only way is to get a connector otherwise you have no way to create something to draw too. If you want to copy to another gpu then you should use the Handle of that gpu. Without the connector there is no handle to a display because there is no display.

Also, if multiples are present, which connector should be used?

That is up to the provider of the handle to decide.

To ensure we handle cases where programs rely on the device without a connector then we could specify the connect_id as an Option. This would also fix the problem mentioned above since we could just pass None when calling new thus making this a non breaking change.

morr0ne · 2024-07-05T19:51:19Z

I also realized that instead of a u32 I should have used a NonZeroU32. Going with the above would also mean we get a nicely aligned layout since the size of Option<NonZeroU32> is guaranteed to be the same as u32

kchibisov · 2024-07-05T19:58:17Z

But you can have lots of connectors per once device so it doesn't make much sense, and Display handle is not for drawing it's for getting a some kind of device. Your connector is also hotplug, while device is generally not .

connector is also not required to create any drawing setups with OpenGL/Vulkan, etc, because you draw into Window.

So if anywhere connector could be an optional field on DrmWindowHandle, but then drm APIs accept array of connectors IIRC.

morr0ne · 2024-07-05T20:06:27Z

It appears I misunderstood the difference between the display and the window handle. Given this clarification, I agree that placing the connector in the window handle is more logical.

To provide additional context, this change is necessary for adding support to wgpu. Specifically, Vulkan requires a connector_id when creating the surface. You can see how this is used here: gfx-rs/wgpu#5908

kchibisov · 2024-07-05T20:10:47Z

I'd say just move it to the Window and make it Option, should be fine after that.

I personally always used Gbm or drm atomic APIs, so can't say much about DrmWindowHandle, probably Vulkan will be the only consumer of it and if it's a single connector it's fine...

kchibisov · 2024-07-05T20:21:01Z

src/unix.rs

@@ -239,6 +239,7 @@ impl DrmDisplayHandle {
 pub struct DrmWindowHandle {
    /// The primary drm plane handle.
    pub plane: u32,
+    pub connector_id: Option<NonZeroU32>,


Should document what is it.

Also ideally with links to other documentation explaining it, if such docs exist.

kchibisov

Also need a CHANGELOG entry.

madsmtm · 2024-07-10T14:45:31Z

src/unix.rs

@@ -255,7 +257,10 @@ impl DrmWindowHandle {
    /// let handle = DrmWindowHandle::new(plane);


Please update this example to also set the connector_id

What I mean here is something like:

Suggested change

/// let handle = DrmWindowHandle::new(plane);

/// # connector_id = unsafe { NonZeroU32::new_unchecked(1) };

/// let mut handle = DrmWindowHandle::new(plane);

/// handle.connector_id = Some(connector_id);

kchibisov

From my side it's good. Docs are indirectly linked via the description of the struct itself since it's all on the DRM docs, as well as plane stuff.

madsmtm · 2024-07-15T15:46:16Z

src/unix.rs

+    /// # connector_id = unsafe { NonZeroU32::new_unchecked(1) };
+    /// let handle = DrmWindowHandle::new_with_connector_id(plane, connector_id);
+    /// ```
+    pub fn new_with_connector_id(plane: u32, connector_id: NonZeroU32) -> Self {


Nit: This doesn't really match what's exposed for the other platforms, there, the user would just do:

let mut handle = DrmWindowHandle::new(...); handle.connector_id = Some(...);

But perhaps this approach is indeed better? In any case, might make sense to not do such a change in this PR, as it's somewhat inconsistent with the other platforms.

Using the _with_* suffix aligns better with rust's coding guidelines, promoting more idiomatic code. If there is a consensus that this requires further discussion, I can amend the changes and open a separate pr. However, I believe this approach is the most appropriate for exposing the api and that similar constructors should be considered for other platforms as well.

Implementing these constructors will not compromise backwards compatibility, as the only scenario requiring their removal would be the elimination of the corresponding fields. In such a case, a breaking change would be inevitable regardless.

Furthermore, implementing a separate constructor prevents unnecessary mutability of the created handle, which typically does not require any kind of modification.

I'm generally against constructors like that in this crate because they are prone to factorial growth. So if you want to add one more optional field you'd need to add permutations, since you don't generally require all the things to be present.

Unless you just stuck constructors with Option but I don't see a point in it given that you can just build with WindowHandle { } syntax because it's a pod with all fields being pub.

I'm also not that much in love with ::new methods, but it's more of a shorthand for essential stuff, I guess.

The primary justification for introducing a new constructor is that it is not feasible to manually instantiate a handle due to the struct being marked as non_exhaustive. This is not merely a matter of convenience or shorthand. Structs marked as non_exhaustive cannot be instantiated outside of their originating crate. A dedicated function to construct one is mandatory.

They can as long as they have Default but I forgot that we don't have it because we can not really provide Default. I still don't like the constructor because factorial growth.

The proper solution would be to only have one constructor that takes all the parameters but that would break backwards compatibility and require a version bump. My understanding was that we should avoid that if possible.

The connector is optional though, since not all APIs require it and some APIs may require a list of connectors.

The connector is optional though, since not all APIs require it and some APIs may require a list of connectors.

Which perfectly translates into the new constructor. A new constructor like this would only be necessary for an optional field.

I am also not aware of any API that requires a least of connectors that would also consume a window handle. Do you have any examples? If that's the case then maybe the signature of field should be reconsidered. My main use case for this is implementing a drm backend for wgpu which afaik would require only a specific connector, at least for the vulkan backend.

Do you have any examples? If that's the case then maybe the signature of field should be reconsidered. My main use case for this is implementing a drm backend for wgpu which afaik would require only a specific connector, at least for the vulkan backend.

I know that drm APIs accept an array IIRC, at least smithay uses arrays of connectors a lot. Though, they unlikely to use the raw-window-handle for that typo of things and if Vulkan needs just a single connector than it's fine the way it is.

In general I won't block on adding a constructor, it's just I don't really like it, though, I'm not sure there will be more fields to add.

I'd like @Lokathor's opinion - but as stated in my first comment, this is very much nitpicking ;)

morr0ne · 2024-08-10T00:30:36Z

Could you please provide an update on the possibility of merging this PR?

Lokathor · 2024-08-10T00:33:21Z

Ah, sorry this one slipped out of my attention.

I will double check later but in general we probably want as many optional fields to be supported as possible so this is likely to be approved.

morr0ne · 2024-08-10T22:47:13Z

Based on the discussion in this comment, I am reconsidering whether this patch is the most effective approach to achieve my final objective. I'm currently unsure how to mark the pull request as not ready for merging on my end, but for now, it should be treated as such.

MarijnS95 · 2024-08-10T22:47:25Z

To provide additional context, this change is necessary for adding support to wgpu. Specifically, Vulkan requires a connector_id when creating the surface. You can see how this is used here: gfx-rs/wgpu#5908

To relay some of my comment over at that WGPU PR, Vulkan doesn't specifically require the connector ID especially because VK_KHR_display is DRM-agnostic. There just happen to be a few more Vulkan extensions that make it slightly easier to get/identify a VkDisplayKHR instance given a connector ID, but it depends on the driver if that's even supported (and the Vulkan API has other, DRM-agnostic means to query VkDisplayKHRs, so I would in general be careful when tying the two concepts together).

Because the way you plan to use this now seems to rely on the receiver of the DrmWindowHandle doing the modeset (this is what VK_KHR_display has to do internally on DRM platforms), the caller also has to somehow specify which mode and pipe/plane to use. Is the mode this something that should be captured in DrmWindowHandle as well?
Unfortunately the VK_EXT_acquire_drm_display extension used to map a connector ID to a VkDisplayKHR doesn't also provide a way to map a DRM CRTC/plane to its Vulkan representation (if the driver didn't abstract this concept away in the first place) - so the DrmWindowHandle::plane field provided by raw-window-handle has to be ignored here.

As a side-note, regardless of how we continue, let's replace DrmDisplayHandle with DrmWindowHandle in the title :)

Lokathor · 2024-08-11T01:15:25Z

@morr0ne there's some sort of "draft" status a PR can be set to, but that's not necessary, we can just hold off on a merge until you're confident about the direction of things

morr0ne added 2 commits July 4, 2024 17:10

Add connector_id to DrmDisplayHandle

b598cf5

Fix DrmDisplayHandle::new doc-test

332ee85

Move connector_id to DrmWindowHandle

2c43a16

kchibisov reviewed Jul 5, 2024

View reviewed changes

kchibisov requested changes Jul 5, 2024

View reviewed changes

morr0ne added 2 commits July 5, 2024 22:28

Add doc comment for connector_id

017263b

Update changelog

1eb46ad

morr0ne mentioned this pull request Jul 6, 2024

Initial Support for DRM interface in vulkan backend gfx-rs/wgpu#5908

Draft

morr0ne requested a review from kchibisov July 10, 2024 13:13

madsmtm reviewed Jul 10, 2024

View reviewed changes

kchibisov approved these changes Jul 11, 2024

View reviewed changes

morr0ne added 2 commits July 13, 2024 21:33

Add new_with_connector_id constructor to DrmWindowHandle

e51370e

Fix type in new_with_connector_id doctest

10eb23b

madsmtm reviewed Jul 15, 2024

View reviewed changes

morr0ne changed the title ~~Add connector_id to DrmDisplayHandle~~ Add connector_id to DrmWindowHandle Aug 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add connector_id to DrmWindowHandle #170

Add connector_id to DrmWindowHandle #170

morr0ne commented Jul 5, 2024

Lokathor commented Jul 5, 2024

kchibisov commented Jul 5, 2024

morr0ne commented Jul 5, 2024

morr0ne commented Jul 5, 2024

kchibisov commented Jul 5, 2024 •

edited

Loading

morr0ne commented Jul 5, 2024

kchibisov commented Jul 5, 2024

kchibisov Jul 5, 2024

madsmtm Jul 10, 2024

kchibisov left a comment

madsmtm Jul 10, 2024

madsmtm Jul 15, 2024

kchibisov left a comment

madsmtm Jul 15, 2024 •

edited

Loading

morr0ne Jul 15, 2024 •

edited

Loading

morr0ne Jul 15, 2024

kchibisov Jul 15, 2024

morr0ne Jul 15, 2024

morr0ne Jul 15, 2024

kchibisov Jul 15, 2024

morr0ne Jul 15, 2024

kchibisov Jul 15, 2024

madsmtm Jul 16, 2024 •

edited

Loading

morr0ne commented Aug 10, 2024

Lokathor commented Aug 10, 2024

morr0ne commented Aug 10, 2024

MarijnS95 commented Aug 10, 2024 •

edited

Loading

Lokathor commented Aug 11, 2024

		@@ -255,7 +257,10 @@ impl DrmWindowHandle {
		/// let handle = DrmWindowHandle::new(plane);

Add connector_id to DrmWindowHandle #170

Are you sure you want to change the base?

Add connector_id to DrmWindowHandle #170

Conversation

morr0ne commented Jul 5, 2024

Lokathor commented Jul 5, 2024

kchibisov commented Jul 5, 2024

morr0ne commented Jul 5, 2024

morr0ne commented Jul 5, 2024

kchibisov commented Jul 5, 2024 • edited Loading

morr0ne commented Jul 5, 2024

kchibisov commented Jul 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchibisov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchibisov left a comment

Choose a reason for hiding this comment

madsmtm Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

morr0ne Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madsmtm Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

morr0ne commented Aug 10, 2024

Lokathor commented Aug 10, 2024

morr0ne commented Aug 10, 2024

MarijnS95 commented Aug 10, 2024 • edited Loading

Lokathor commented Aug 11, 2024

kchibisov commented Jul 5, 2024 •

edited

Loading

madsmtm Jul 15, 2024 •

edited

Loading

morr0ne Jul 15, 2024 •

edited

Loading

madsmtm Jul 16, 2024 •

edited

Loading

MarijnS95 commented Aug 10, 2024 •

edited

Loading