Add DRM node abstraction

[chrisduerr]

This copies the DRM node abstraction from Smithay almost 1:1, to make it easier for other consumers to reuse this code.

[MarijnS95]

Nice stuff! Something like this would also be great to start using in the examples, as we hardcode card0 which even on Linux don't fare well when a GPU probes as card1.

One thing I don't follow is why - at least for Linux, I don't know anything about FreeBSD - certain implementations are special-cased. For at least Linux I believe the common implementation should be valid?

[chrisduerr]

@MarijnS95 Is there anything still missing from this? Considering that this code was already part of the Smithay project before this PR, I was hoping this wouldn't take weeks to merge. If you're not interested in shipping this please let me know so I don't have to wait for this PR and can instead implement this myself in my application.

[MarijnS95]

@chrisduerr I am not at all a maintainer of this crate and won't have any say in what gets merged nor when (and you'll either way have to "wait" for a release). I hope you've been seeing this review as constructive rather than unnecessary push-back.

Let's see if I can catch back up to where we left off, after leaving town since posting my last review.

[MarijnS95]

Looking nice! I pretty much only have straightforward suggestions to make the documentation in this module more consistent with itself, but the implementations look sensible now!

[MarijnS95]

Just checking, this should probably be:

Suggested change

	#[cfg(target_os = "openbsd")]
	#[cfg(not(any(target_os = "linux", target_os = "freebsd")))]

To match the #else in libdrm.

[chrisduerr]

We're not libdrm there's no reason why we'd try to match it. New platforms should be tested before accepting an existing impl as correct.

[MarijnS95]

Then how about doing the same for is_device_drm()?

[Drakulix]

Would apprechiate consistency here either way. Either all special cased methods should be available on a new platform or none. And I tend to agree that new platforms should be explicitly added.

[chrisduerr]

I wasn't aware that we already used this in other places. Consistency with other code in the module makes sense to me.

[chrisduerr]

I hope you've been seeing this review as constructive rather than unnecessary push-back.

Just for the record, I do not see this review as constructive. I didn't even write any of the code in Smithay and much of it was maybe not ideal, but fine as-is. Changing things around from one style to another in the same repository is just pointless code churn that risks introducing new bugs with no benefit.

Don't get me wrong I agree with many of your points, but effectively you've just stalled out what should be a "free win" for everyone into a weeks-long back and forth. For the future I would recommend just making all your suggestions in one review and then getting things resolved quickly while you are still willing to review the PR. If you don't have the time to take another look, just dismiss the review and move on, the code is fine as-is anyway.

[MarijnS95]

Changing things around from one style to another in the same repository is just pointless code churn that risks introducing new bugs with no benefit.

I disagree here: the fact that code was once part of a different repository under the same organization doesn't suddenly mean that it's ready for this public crate, as is evident from all the silly and unnecessary inconsistencies that I had to point out. As a user of just the drm-rs crate that's all I care about, not the rest of the Smithay organizations nor codebases.
I've spent quite some time and effort in the past to start cleaning up the style and fixing lacking doc-comments in drm-rs - though eventually bailed on this massive yet thankless effort.

It'd be unfortunate to have to add this module to that effort, though on the other hand is a valid way to speed up merging of a new (and apparently desperately needed?) feature, as long as there is commitment to fix i.e. documentation comments in the very near future (by the reviewer that requested them).

you've just stalled out what should be a "free win" for everyone into a weeks-long back and forth.

Sounds like you missed my previous comment. I'm not a maintainer here and wouldn't in any way have been able to help you speed up merging - not to mention pushing out a new release. Given that no maintainer has shown up to say something along the lines of "dismiss/ignore his review comments" they're either unavailable or waiting for my review to pan out?

For the future I would recommend just making all your suggestions in one review and then getting things resolved quickly

This comment pops up far too often. We're all human, and I don't see everything during the first round of review, or find that my review comments caused a misunderstanding when they are addressed.

just dismiss the review and move on

Notice that I already approved your PR in the second round, with only minor inconsistencies pointed out (i.e. drm being capitalized only half the time in doc-comments).

Please remove the re-request for review if you don't care about it. Permissions make me unable to unassign myself 😞

the code is fine as-is anyway.

Says the submitter 😬

[Drakulix]

As a maintainer, I feel like some comments on the situation are necessary (though I apprechiate the rather honest, but at least respectful discourse so far).

Changing things around from one style to another in the same repository is just pointless code churn that risks introducing new bugs with no benefit.

I don't think most of the review changes (especially consistency / style nits) are posing much risk here.

Don't get me wrong I agree with many of your points, but effectively you've just stalled out what should be a "free win" for everyone into a weeks-long back and forth.

I don't think we necessarily needed this "free win". The code in smithay works and will until whenever this PR is resolved. smithay doesn't hugely benefit from this and drm-rs is a different crate with a bunch of different properties and demands (different maintainers, different regards to api stability, etc).

The reason this was merged into smithay in it's current state wasn't, that it was deemed good code. It was deemed better than what we had previously, which is generally a quite low bar to clear and mostly why smithay is much more active than drm-rs is. Both approaches make sense in regard to the particular project they are employed within, but they don't translate from one to the other just because of the shared organisation.

However I do also see, that this causes friction and dissatisfaction with the review process and obviously I do welcome these changes quite a bit. I think DrmNode is a great fit for drm-rs. New code should clear a certain bar of consistency and code quality, but the way I see it @chrisduerr has done his due diligence on the matter.

On specifically resolving this PR: I feel like the only comments that still need to be addressed before merging are those that affect the public api, which mostly comes down to consistency of #[cfg]-annotations.

The doc comments are fine in their current state and can be polished up later, if somebody feels like doing that. I'll give the code a second review round before merging, but it looks correct and style discussions around format! vs PathBuf::join or the like don't seem very productive to me at this point either. Again everybody is free to clean this up, if they feel like it, but this is nothing that needs to block merging at this point imo.

[MarijnS95]

and style discussions around format! vs PathBuf::join or the like don't seem very productive to me at this point either.

It looks like all such discussions have already been resolved in the weeks prior, in particular the semantic difference between .join(a).join(b) vs format!("{}{}", a, b) (the latter has no path separator). But now that you bring it up, it does look like let path = Path::new("/dev/dri").join(&*name); could be a PathBuf::from(format!("/dev/dri/{name}")) to be consistent with the other two implementations of fn dev_path() 😁

---

Other than that, I don't want to say that the original code from Smithay was bad or anything like that. Just that we now have to opportunity to go over it once more and pick out little inconsistencies which tend to be easier to clear up directly when new code is introduced.

[chrisduerr]

I think everything that needed to be said here has been said, but maybe just a few closing thoughts from me:

you've just stalled out what should be a "free win" for everyone into a weeks-long back and forth.

I'm not a maintainer here and wouldn't in any way have been able to help you speed up merging [...].
Given that no maintainer has shown up [...], they're either unavailable or waiting for my review to pan out?

If I have a frequent contributor submit a review for a PR as a maintainer, I think it's logical to take a step back and wait for these things to be resolved. So if that review slows down, that can obviously still impact the time it takes to merge things.

the code is fine as-is anyway.

Says the submitter 😬

To be fair I'm submitter, not author. The original code was completely unchanged. I do think that the quality of the original code was bad, but my goal was obviously to get this available in a reasonable timeframe so I didn't want to open the whole can of worms that a rewrite can bring (as you can see here).

I don't think we necessarily needed this "free win". The code in smithay works and will until whenever this PR is resolved.

It's a free win for me, because I also want to use this code. 😄

Generally speaking I don't think the contents of the review were a particularly big issue. Most of the comments are valid and I'd only consider some minor stuff as too nitpicky in the context of not having authored this code myself.

My main problem is that there was no review for 2 weeks. You're obviously free to work or not whenever you feel like it and you quickly made another review once I pinged you, but if you're unavailable and not the one responsible for merging anyway, it's very unfortunate to stall out on inactivity due to that.

[MarijnS95]

so I didn't want to open the whole can of worms that a rewrite can bring (as you can see here).

A complete rewrite wasn't and isn't needed at all, just many smaller touch-ups, clarifications, and comment improvements is all.

My main problem is that there was no review for 2 weeks. You're obviously free to work or not whenever you feel like it and you quickly made another review once I pinged you, but if you're unavailable and not the one responsible for merging anyway, it's very unfortunate to stall out on inactivity due to that.

I don't think maintainers should have to be waiting for "unrelated contributors" to re-review a PR, unless there has been an explicit agreement that they will be furthering it and want to stay in the loop, as well as have some final say before the merge button is clicked.

The thing about reviewing like that is that there is no obligation for me to do anything, nor in any specific time frame. At the same time that means there is no guarantee that I won't miss out on future changes and pushes before a maintainer deems it ready and merges: that's the point of not having any responsibility as a non-maintainer 😬

---

And personally I don't think 2 weeks is anything unreasonable. This PR wasn't opened with a time frame or dependency requirement in mind: to quote, it's just a nicety "to make it easier for other consumers to reuse this code". I have a lot of "this would be nice to have" PRs open (the only two other open PRs on this repository currently are good examples) that sat stale for way longer, and I might rebase or bump them every once in a while when I get back to a project. I'd love for that list to be shorter, but that's both down to maintainer/community time, and my own time to keep following up. At least there's visibility that something exists.

[Drakulix]

My main problem is that there was no review for 2 weeks. You're obviously free to work or not whenever you feel like it and you quickly made another review once I pinged you, but if you're unavailable and not the one responsible for merging anyway, it's very unfortunate to stall out on inactivity due to that.

Yeah sorry, I had like a major sickness period followed by some very much needed holidays in the last two weeks, and this just got lost in the mountains of github notifications I was slowly catching up on at the end of last week.

Which is why I very much appreciated the ping on the matter. In general, if I am not answering, it usually means I am actually not available, given I very rarely postpone these kind of things. But I am now also very aware I could have communicated this better.

[Drakulix]

Thanks a bunch for working on this. LGTM!

[chrisduerr]

@Drakulix Seems like the latest unicode-width doesn't build. Do you want me to fix that in this PR or can you merge without clippy?

[Drakulix]

@Drakulix Seems like the latest unicode-width doesn't build. Do you want me to fix that in this PR or can you merge without clippy?

I would like to have a working state on master, so I am not going to merge this before that is fixed. But if you don't feel like taking care of this, I'll just do that sometime tomorrow, no worries.

[ids1024]

I guess the main downside to merging this in drm-rs rather than Smithay is that drm-rs generally should have a more stable API. But I think this part of Smithay hasn't really had many API changes. So it seems quite reasonable to move here.

Otherwise it definitely seems good to have this in drm-rs. And rather expected, since it's functionality that is also found in libdrm.

[chrisduerr]

I would like to have a working state on master, so I am not going to merge this before that is fixed. But if you don't feel like taking care of this, I'll just do that sometime tomorrow, no worries.

Just to be clear, master is not in a working state currently, this PR changes nothing about that. I'll look into it later today.

[chrisduerr]

I've decided to bump rust-version everywhere, since CI does not distinguish between per-crate Rust versions. It shouldn't make a big difference and this way one can be certain that the MSRV is actually correct.