| Message ID | 20250313160220.6410-2-sergeantsagara@protonmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Initial work for Rust abstraction for HID device driver development | expand |
Hi, [quick reply because I am completely under the water for the next 2 weeks] On Mar 13 2025, Rahul Rameshbabu wrote: > Hello, > > I am a hobbyist developer who has been working on a project to create a new Rust > HID device driver and the needed core abstractions for writing more HID device > drivers in Rust. My goal is to support the USB Monitor Control Class needed for > functionality such as backlight control for monitors like the Apple Studio > Display and Apple Pro Display XDR. A new backlight API will be required to > support multiple backlight instances and will be mapped per DRM connector. The > current backlight API is designed around the assumption of only a single > internal panel being present. I am currently working on making this new API for > DRM in parallel to my work on the HID side of the stack for supporting these > displays. Thanks a lot for this work, though I wonder if your goal is not too big, too far from the HID point of view. HID is simple, and there is only a few bindings that you would need to be able to make "simple" HID drivers. My assumption would be to introduce the binding with a functional but small driver (like one that just changes the report descriptor, or does a sime raw event processing). Then we can look at integrating with the DRM interface. Though it's up to you to decide how you want to play ;) > > https://binary-eater.github.io/tags/usb-monitor-control/ > > Julius Zint had attempted to do so a year ago with a C HID driver but was gated > by the lack of an appropriate backlight API for external displays. I asked him > for permission to do the work need in Rust and plan to accredit him for the HID > report handling for backlight in the USB Monitor Control Class standard. > > https://lore.kernel.org/lkml/f95da7ff-06dd-2c0e-d563-7e5ad61c3bcc@redhat.com/ > > I was hoping to get initial feedback on this work to make sure I am on the right > path for making a Rust HID abstraction that would be acceptable upstream. The > patches compile with WERROR being disabled. This is necessary since Rust treats > missing documentation comments as warnings (which is a good thing). I also need > to go in and add more SAFETY comments. K, I'll give you my opinion in the patches as the HID co-maintainer. I do have a very little rust experience, but this is my first in kernel, so I hope the more experience rust people here will chime in as well. Cheers, Benjamin > > Thanks, > Rahul Rameshbabu > > Rahul Rameshbabu (3): > rust: core abstractions for HID drivers > rust: hid: USB Monitor Control Class driver > rust: hid: demo the core abstractions for probe and remove > > drivers/hid/Kconfig | 16 ++ > drivers/hid/Makefile | 1 + > drivers/hid/hid_monitor_control.rs | 42 +++++ > rust/bindings/bindings_helper.h | 1 + > rust/kernel/hid.rs | 245 +++++++++++++++++++++++++++++ > rust/kernel/lib.rs | 2 + > 6 files changed, 307 insertions(+) > create mode 100644 drivers/hid/hid_monitor_control.rs > create mode 100644 rust/kernel/hid.rs > > -- > 2.47.2 > >
On Thu Mar 13, 2025 at 5:02 PM CET, Rahul Rameshbabu wrote: > Hello, > > I am a hobbyist developer who has been working on a project to create a new Rust > HID device driver and the needed core abstractions for writing more HID device > drivers in Rust. My goal is to support the USB Monitor Control Class needed for > functionality such as backlight control for monitors like the Apple Studio > Display and Apple Pro Display XDR. A new backlight API will be required to > support multiple backlight instances and will be mapped per DRM connector. The > current backlight API is designed around the assumption of only a single > internal panel being present. I am currently working on making this new API for > DRM in parallel to my work on the HID side of the stack for supporting these > displays. > > https://binary-eater.github.io/tags/usb-monitor-control/ > > Julius Zint had attempted to do so a year ago with a C HID driver but was gated > by the lack of an appropriate backlight API for external displays. I asked him > for permission to do the work need in Rust and plan to accredit him for the HID > report handling for backlight in the USB Monitor Control Class standard. > > https://lore.kernel.org/lkml/f95da7ff-06dd-2c0e-d563-7e5ad61c3bcc@redhat.com/ > > I was hoping to get initial feedback on this work to make sure I am on the right > path for making a Rust HID abstraction that would be acceptable upstream. The > patches compile with WERROR being disabled. This is necessary since Rust treats > missing documentation comments as warnings (which is a good thing). I also need > to go in and add more SAFETY comments. > > Thanks, > Rahul Rameshbabu > > Rahul Rameshbabu (3): > rust: core abstractions for HID drivers > rust: hid: USB Monitor Control Class driver > rust: hid: demo the core abstractions for probe and remove > > drivers/hid/Kconfig | 16 ++ > drivers/hid/Makefile | 1 + > drivers/hid/hid_monitor_control.rs | 42 +++++ > rust/bindings/bindings_helper.h | 1 + > rust/kernel/hid.rs | 245 +++++++++++++++++++++++++++++ > rust/kernel/lib.rs | 2 + > 6 files changed, 307 insertions(+) > create mode 100644 drivers/hid/hid_monitor_control.rs > create mode 100644 rust/kernel/hid.rs I have taken a very quick look and haven't seen any big problems, there are some minor things, but not worth mentioning for an RFC. --- Cheers, Benno
On Thu, 13 Mar, 2025 17:31:53 +0100 "Benjamin Tissoires" <bentiss@kernel.org> wrote: > Hi, > > [quick reply because I am completely under the water for the next 2 > weeks] > > On Mar 13 2025, Rahul Rameshbabu wrote: >> Hello, >> >> I am a hobbyist developer who has been working on a project to create a new Rust >> HID device driver and the needed core abstractions for writing more HID device >> drivers in Rust. My goal is to support the USB Monitor Control Class needed for >> functionality such as backlight control for monitors like the Apple Studio >> Display and Apple Pro Display XDR. A new backlight API will be required to >> support multiple backlight instances and will be mapped per DRM connector. The >> current backlight API is designed around the assumption of only a single >> internal panel being present. I am currently working on making this new API for >> DRM in parallel to my work on the HID side of the stack for supporting these >> displays. > > Thanks a lot for this work, though I wonder if your goal is not too big, > too far from the HID point of view. HID is simple, and there is only a > few bindings that you would need to be able to make "simple" HID > drivers. > > My assumption would be to introduce the binding with a functional but > small driver (like one that just changes the report descriptor, or does > a sime raw event processing). Then we can look at integrating with the > DRM interface. > > Though it's up to you to decide how you want to play ;) > Thanks for the suggestion, Benjamin! I think its a great suggestion for getting the Rust abstractions for HID cleaned up and integrated before taking on this more herculian challenge. I think it would be ideal to maybe do the following? 1. Focus on the core Rust HID abstractions first to get something merged for supporting "simple" HID drivers. 2. Build a reference driver to validate the abstrations support "simple" HID devices. - https://rust-for-linux.com/rust-reference-drivers 3. After getting the first two steps worked out, continue pursuing monitor control as a Rust driver and work on the needed changes for DRM connector backlight control API. Luckily, it seems to me that parts of 3 can be done in parallel to 1 and 2. However, the advantage of this is that if the Rust HID abstration can support "simple" HID drivers initially, it has a path to get merged and iterated upon for supporting more complex drivers. You mention this in the third patch in this series, but it would probably be good to find a simple device that requires a report fixup or some processing in .raw_event or .event callback. Making a reference driver for the Glorious PC Gaming Race mice seems like a good start. My only concern is the lack of complexity in the C driver not needing a .remove callback implementation. I wanted to have an example where architecting what device removal with Rust semantics would look like. I'll get into more details where you bring this up in third patch in this series. Thanks for the feedback, Rahul Rameshbabu >> >> https://binary-eater.github.io/tags/usb-monitor-control/ >> >> Julius Zint had attempted to do so a year ago with a C HID driver but was gated >> by the lack of an appropriate backlight API for external displays. I asked him >> for permission to do the work need in Rust and plan to accredit him for the HID >> report handling for backlight in the USB Monitor Control Class standard. >> >> https://lore.kernel.org/lkml/f95da7ff-06dd-2c0e-d563-7e5ad61c3bcc@redhat.com/ >> >> I was hoping to get initial feedback on this work to make sure I am on the right >> path for making a Rust HID abstraction that would be acceptable upstream. The >> patches compile with WERROR being disabled. This is necessary since Rust treats >> missing documentation comments as warnings (which is a good thing). I also need >> to go in and add more SAFETY comments. > > K, I'll give you my opinion in the patches as the HID co-maintainer. I > do have a very little rust experience, but this is my first in kernel, > so I hope the more experience rust people here will chime in as well. > > Cheers, > Benjamin > >> >> Thanks, >> Rahul Rameshbabu >> >> Rahul Rameshbabu (3): >> rust: core abstractions for HID drivers >> rust: hid: USB Monitor Control Class driver >> rust: hid: demo the core abstractions for probe and remove >> >> drivers/hid/Kconfig | 16 ++ >> drivers/hid/Makefile | 1 + >> drivers/hid/hid_monitor_control.rs | 42 +++++ >> rust/bindings/bindings_helper.h | 1 + >> rust/kernel/hid.rs | 245 +++++++++++++++++++++++++++++ >> rust/kernel/lib.rs | 2 + >> 6 files changed, 307 insertions(+) >> create mode 100644 drivers/hid/hid_monitor_control.rs >> create mode 100644 rust/kernel/hid.rs >> >> -- >> 2.47.2 >> >>