Add function to enumerate devices #208
Conversation
Co-authored-by: John Nunley <[email protected]>
Co-authored-by: Marijn Suijten <[email protected]>
|
BUMP. @MarijnS95 @notgull Can you guys give another look in this PR and approve/merge if its good? |
|
Checking once again if there any update on this. |
src/node/mod.rs
Outdated
| @@ -380,3 +380,12 @@ pub fn dev_path(dev: dev_t, ty: NodeType) -> io::Result<PathBuf> { | |||
| ), | |||
| )) | |||
| } | |||
|
|
|||
| /// Returns an iterator with all DRM Nodes we managed to find. There might be duplicates. | |||
There was a problem hiding this comment.
Nit: not sure about the wording "we managed to find". We might want to be more explicit about where/how we found them instead, and plainly state that these are "all the DRM nodes" currently on the system (putting trust in our implementation).
There was a problem hiding this comment.
I dropped the "we managed to find" part.
Shall I expand the description to mention the exact directory and how we find the devices?
src/node/mod.rs
Outdated
| use std::io; | ||
| use std::os::unix::io::AsFd; | ||
| use std::path::{Path, PathBuf}; | ||
| use std::{fs, io}; |
There was a problem hiding this comment.
We should probably merge all module imports, or move fmt::{self here and flatten that a bit.
There was a problem hiding this comment.
I moved fmt down to this import line. Please take another look.
There was a problem hiding this comment.
I think I should have been more elaborate here (but wasn't, since it's not related to this PR): personally I don't like importing both a module and some of its items. In this case fmt is only used in the impl blocks for fmt::Result, but Debug/Display/Formatter are globally imported.
If that's to disambiguate the Result from the prelude, my personal preference is to import only fmt and qualify fmt::Debug/fmt::Display/fmt::Formatter :)
| pub fn devices() -> io::Result<impl Iterator<Item = DrmNode>> { | ||
| let result = fs::read_dir(DRM_DIR_NAME)? | ||
| .filter_map(|entry| entry.ok()) | ||
| .filter_map(|entry| DrmNode::from_path(entry.path()).ok()); |
There was a problem hiding this comment.
Nit: I'd only filter out from_path() == Err(CreateDrmNodeError::NotDrmNode), and bubble up all other io::Errors to the user.
This would quite literally look like fs::read_dir() that returns an outer and inner io error: -> io::Result<impl Iterator<Item = io::Result<DrmNode>>>.
And maybe all entry.ok()s should translate into that as well? Though OTOH, we cannot be sure if the entry we tried to visit was even DRM node in the first place, making things confusing (equally confusing as missing DRM nodes because we were not able/allowed to visit that entry for some reason).
There was a problem hiding this comment.
libdrm's drmGetDevice2() doesn't seem to bubble-up IO errors (here I guess) after it's reading the directory, so I'm inclined to also not bubble those up, so we remain closer to their implementation.
Of course that is my personal taste and I'm fine to change to your suggested approach if that is desired.
There was a problem hiding this comment.
We don't need to necessarily be bit-for-bit identical to what drmGetDeviceS2() does, since the underlying process_device() returns -1 both for errors as well as detecting it's not a DRM node.
If we do such a thing we do have to be careful indeed; it'd be a bummer to bail on an io::Error from stat() when reading a file that isn't a DRM device in the first place (i.e. on openbsd that reads all of /dev). On the other hand, it might be "annoying" to not be alerted of certain IO failures (possibly permission denied?) for paths that are DRM devices that won't get returned.
This is why I suggested returning nested errors so that the caller can decide whether to skip them or not, but I'd like to leave it to the other drm-rs maintainers to make a decision.
If anything it might be useful to look at existing users of drm-rs and see if they can benefit from this addition; and if so how they currently implement the looping.
There was a problem hiding this comment.
Oh also, looking at the examples and I specifically just opened #218, how about updating all the examples in this repo/crate to use this function in favour of the hardcoded /dev/dri/card0? I had to hack up a bunch of examples previously when using an Intel GPU which seemed to start probing at /dev/dri/card1.
There was a problem hiding this comment.
I had another look at the examples and realized that in libdrm we open the devices and thus have a file descriptor ready. In this library devices aren't opened and no node/file path information is stored, so even if we are able to list all devices, we can't do much with them, as we don't know how to open them.
I guess it might make sense to extend the current implementation to also include at least the path of the device, otherwise it won't be very useful for the listing.
Adds a basic DRM Listing functionality.
Please note that, different from libDRM, we don't process the devices to include bus information or anything else. We also don't de-duplicate devices like libDRM does, so duplicates might occur.
This change addresses #207