Should drm::Device should have a default implementation or be demoted to a struct? about drm-rs HOT 9 CLOSED

Originally Device was implemented as a struct wrapping around a RawFd. However, there needed to be a mechanism to close the file descriptor when the Device goes out of scope, requiring a custom Drop trait. But if the same RawFd was used elsewhere in the user's code, then we can't just close it ourselves, which means the user would need to use unsafe code (which I wanted to avoid).

Wrapping it around a File worked for most cases that opened /dev/dri/* directly, but surprisingly many C applications were preferring to open it using either dbus or getting the file descriptor over a socket. You also lost any other traits/functionality you may want from File.

Eventually, I found more and more issues while looking at how various display managers and desktop environments were working, and ended up with this over-engineered abomination:

pub struct Device<T, U> where T: Borrow<U>, U: AsRawFd {
    inner: T
}

As beautiful as it was, some friends of mine convinced me to scrap the idea of a wrapper and make it a trait to just let the user figure out how they want to use it.

It would be simple enough to include a basic Card wrapper for the less complex use-cases. On the other hand we could use a blanket implementation like impl<T> Device for T where T: AsRawFd so that you can directly use File and avoid requiring a wrapper at all.

What would you prefer?

from drm-rs.

Originally Device was implemented as a struct wrapping around a RawFd. However, there needed to be a mechanism to close the file descriptor when the Device goes out of scope, requiring a custom Drop trait. But if the same RawFd was used elsewhere in the user's code, then we can't just close it ourselves, which means the user would need to use unsafe code (which I wanted to avoid).

This is the main reason that the Trait-approach is a godsend for smithay.

Wrapping it around a File worked for most cases that opened /dev/dri/* directly, but surprisingly many C applications were preferring to open it using either dbus or getting the file descriptor over a socket. You also lost any other traits/functionality you may want from File.

This happens a lot in compositors and close semantics are all over the place. Sometime we need to close the file descriptor, sometimes another process does this for us and sometimes we need multiple references of the same file. Tracking all those cases requires a lot of flexibility and having to use anything more then a RawFd (maybe even with its own Drop implementation, I would need to work around), would make things way more difficult for smithay.

Eventually, I found more and more issues while looking at how various display managers and desktop environments were working, and ended up with this over-engineered abomination:

pub struct Device<T, U> where T: Borrow<U>, U: AsRawFd {
    inner: T
}

As beautiful as it was, some friends of mine convinced me to scrap the idea of a wrapper and make it a trait to just let the user figure out how they want to use it.

A huge thanks to that friend.

It would be simple enough to include a basic Card wrapper for the less complex use-cases. On the other hand we could use a blanket implementation like impl<T> Device for T where T: AsRawFd so that you can directly use File and avoid requiring a wrapper at all.

What would you prefer?

The latter solution clashes with some use-cases in smithay. I could work around this, but I would strongly prefer the first approach, so I do not need to add more indirection in smithay. Not every file descriptor is a valid drm-device, but I deal with a lot of different structs implementing AsRawFd in smithay. As a result a blanket implementation does also not make much sense from a logical point of view.

from drm-rs.

Those are pretty good excuses for not having a default implementation? Do you see display managers the primary use-case for this crate? If that's the case, then I will admit to being wrong about the 99% case just wanting to own the display for the life of the process.

My thinking was definitely biased towards embedded use-cases like kiosks, touch-screen interfaces, retro game consoles, etc. In my case I'm specifically looking to get graphics onscreen as quickly as possible after boot (a custom dashboard for an automotive project).

from drm-rs.

I think I would also favor the first option of just having a standard Card implementation for those that want it.

from drm-rs.

Do you see display managers the primary use-case for this crate? If that's the case, then I will admit to being wrong about the 99% case just wanting to own the display for the life of the process.

That's difficult to determine. Display managers and desktop environments are definitely the most complex use cases for this crate. They need to be able to dynamically switch modes throughout the session, share graphics buffers with multiple process', and work across a large number of different graphics cards. Not to mention the new 'syncobj' and 'leasing` parts that the kernels have recently implemented.

Those requirements make it a bit difficult to provide default simple solutions. Instead of trying to guess how people are going to use it, I just made the goal of this crate to provide a safe, direct interface to the DRM sybsystem with minimal overhead (even more minimal than libdrm :P).

I will likely include some sort of prelude module that re-exports the traits and provides a simple, concrete implementation of them. Until then, the examples provide a really simple Card structure that you can paste into your code. If there's an issue with it, let me know.

from drm-rs.

Originally Device was implemented as a struct wrapping around a RawFd. However, there needed to be a mechanism to close the file descriptor when the Device goes out of scope, requiring a custom Drop trait. But if the same RawFd was used elsewhere in the user's code, then we can't just close it ourselves, which means the user would need to use unsafe code (which I wanted to avoid).

I wonder if the issue is that RawFd doesn't encode ownership, and if the right answer would be to add the notion of a a "scoped file descriptor", which would be either owned (implementing Drop), or borrowed (not implementing drop), but both themselves implementing AsRawFd. Then the struct itself can simply be a wrapper around an AsRawFd. Like with the type system in general, it should be possible to temporarily borrow and OwnedFd, and it should also be possible permanently move an OwnedFd to some external place. Meanwhile, if we're receiving and file descriptor from an external process, we simply need to choose the right type.

As I write this I'm wondering about the case for a WeakFd?

from drm-rs.

File is a basically an owned file descriptor. When a File drops it closes the underlying file descriptor.

You can get around that with IntoRawFd, which turns the File into a RawFd without closing it. Then there's also FromRawFd, which takes a RawFd and creates an owned File. However, the latter is unsafe as it can't assume it's the sole owner of the underlying file descriptor.

However, I'm not quite sure what the issue you're trying to solve here is. It sounds like you dislike the design pattern used, but I don't see any scenario that it fails to perform well in.

from drm-rs.

However, I'm not quite sure what the issue you're trying to solve here is. It sounds like you dislike the design pattern used, but I don't see any scenario that it fails to perform well in.

Well at this point I've kinda gotten over it. What is it about pub struct Device<T, U> where T: Borrow<U>, U: AsRawFd that people find so painful? Coming from the outside, and being a bit new to rust, that's roughly what I expected to see.

from drm-rs.

Well at this point I've kinda gotten over it. What is it about pub struct Device<T, U> where T: Borrow<U>, U: AsRawFd that people find so painful? Coming from the outside, and being a bit new to rust, that's roughly what I expected to see.

In terms of how the code works? Nothing. But nesting generics blows up function signatures and requires even more generics if you have an object that needs to reference one. Makes writing clean code difficult, and doesn't provide any extra safety guarantees compared to the trait-based method.

from drm-rs.