[PATCH 5/8] rust: drm: add DRM driver registration

[Maxime Ripard]

On Wed, Mar 26, 2025 at 11:46:54AM +0100, Danilo Krummrich wrote:
> On Wed, Mar 26, 2025 at 10:24:43AM +0100, Maxime Ripard wrote:
> > Hi,
> > 
> > On Wed, Mar 26, 2025 at 12:54:32AM +0100, Danilo Krummrich wrote:
> > > Implement the DRM driver `Registration`.
> > > 
> > > The `Registration` structure is responsible to register and unregister a
> > > DRM driver. It makes use of the `Devres` container in order to allow the
> > > `Registration` to be owned by devres, such that it is automatically
> > > dropped (and the DRM driver unregistered) once the parent device is
> > > unbound.
> > 
> > The code looks correct, but the wording is confusing to me.
> > drm_dev_unregister does indeed unregister the device, but it's not freed
> > until the last reference is dropped, so it's not really "dropped once
> > the parent device is unbound", the reference is, and it's not active
> > anymore.
> 
> The above wording is related to the Registration structure itself, i.e. the
> Registration is dropped, but not the the DRM device itself. However, if the
> Registration had the last reference to the DRM device, then of course it's
> freed.

Ok, my bad then :)

> > > +impl<T: Driver> Drop for Registration<T> {
> > > +    /// Removes the registration from the kernel if it has completed successfully before.
> > > +    fn drop(&mut self) {
> > > +        // SAFETY: Safe by the invariant of `ARef<drm::Device<T>>`. The existence of this
> > > +        // `Registration` also guarantees the this `drm::Device` is actually registered.
> > > +        unsafe { bindings::drm_dev_unregister(self.0.as_raw()) };
> > > +    }
> > > +}
> > 
> > drm_dev_unregister also have an hotplug-aware variant in
> > drm_dev_unplug(). However, most devices are hotpluggable, even if only
> > through sysfs. So drm_dev_unplug() is generally a better option. Should
> > we use it here, and / or should we provide multiple options still?
> > 
> > Another thing worth mentioning I think is that drm_dev_unplug() works by
> > setting a flag, and drivers are expected to check that their access to
> > device-bound resources (so registers mapping, clocks, regulators, etc.)
> > are still there through drm_dev_enter/drm_dev_exit. It's pretty fragile
> > overall, so I wonder if it's something we could abstract away in Rust.
> 
> DRM should not have to take care of the lifetime of device resources of the
> parent device. This is the responsibility of the driver core abstractions.
>
> At least for the device resources we directly give out to drivers, it has to be,
> since otherwise it would mean that the driver core abstraction is unsound.
> Drivers could otherwise extend the lifetime of device resources arbitrarily.

Sure.

> For instance, I/O memory is only ever given out by bus abstractions embedded in
> a Devres container (e.g. Devres<pci::Bar>). Once the parent device is unbound
> the pci::Bar within the Devres container won't be accessible any more and will
> be dropped internally. So, that's nothing DRM has to take care of.
> 
> However, there are cases where it's better to let subsystem abstractions manage
> the lifetime of device resources, (e.g. DMA mappings). The relevant thing is,
> that we never hand out device resources to a driver in a way that the driver can
> extend their lifetime arbitrarily.

I was talking about the opposite though: when the driver is still around
but the device (and its resources) aren't anymore.

It makes total sense to tie the lifetime of the device resources to the
device. However, the DRM device and driver will far outlive the device
it was bound to so it needs to deal with that kind of degraded "the DRM
driver can still be called by userspace, but it doesn't have a device
anymore" scenario. That's what I was talking about.

Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20250328/a7fcf7d2/attachment-0001.sig>