drivers/gpu/drm/nova/driver.rs | 14 +++- drivers/gpu/drm/nova/file.rs | 8 ++ drivers/gpu/drm/tyr/driver.rs | 12 ++- drivers/gpu/drm/tyr/file.rs | 4 + rust/kernel/drm/device.rs | 134 ++++++++++++++++++++++++++++++++- rust/kernel/drm/driver.rs | 100 +++++++++++++++++++----- rust/kernel/drm/ioctl.rs | 17 ++++- rust/kernel/drm/mod.rs | 1 + 8 files changed, 258 insertions(+), 32 deletions(-)
DRM ioctls run in process context without any guarantee that the parent bus device is still bound. This series solves the problem by introducing UnbindGuard -- a guard representing a drm_dev_enter/exit SRCU critical section that proves the parent bus device is bound for the lifetime of the guard. On top of that, add RegistrationData as a ForLt associated type on drm::Driver, allowing drivers to store data whose lifetime is tied to the parent bus device binding scope. The data is allocated in Registration::new(), lifetime-erased to 'static for storage, and made accessible through UnbindGuard::registration_data() with the lifetime shortened back via ForLt::cast_ref. Also update the ioctl dispatch macro to wrap every handler in an UnbindGuard, returning ENODEV if the device has been unplugged, and pass both the bound parent bus device and the registration data as arguments to handlers. This series is based on [1], as well as Lyudes drm::DeviceContext work; a branch with all patches can be found in [2]. [1] https://lore.kernel.org/driver-core/20260506220012.855173-1-dakr@kernel.org/ [2] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=drm-lifetime Danilo Krummrich (6): rust: drm: Add Driver::ParentDevice associated type rust: drm: Add UnbindGuard for drm_dev_enter/exit critical sections rust: drm: Add RegistrationData to drm::Driver rust: drm: Wrap ioctl dispatch in UnbindGuard rust: drm: Pass registration data to ioctl handlers rust: drm: Pass bound parent device to ioctl handlers drivers/gpu/drm/nova/driver.rs | 14 +++- drivers/gpu/drm/nova/file.rs | 8 ++ drivers/gpu/drm/tyr/driver.rs | 12 ++- drivers/gpu/drm/tyr/file.rs | 4 + rust/kernel/drm/device.rs | 134 ++++++++++++++++++++++++++++++++- rust/kernel/drm/driver.rs | 100 +++++++++++++++++++----- rust/kernel/drm/ioctl.rs | 17 ++++- rust/kernel/drm/mod.rs | 1 + 8 files changed, 258 insertions(+), 32 deletions(-) -- 2.54.0
On Thu May 7, 2026 at 12:05 AM CEST, Danilo Krummrich wrote:
> Danilo Krummrich (6):
> rust: drm: Add Driver::ParentDevice associated type
> rust: drm: Add UnbindGuard for drm_dev_enter/exit critical sections
> rust: drm: Add RegistrationData to drm::Driver
> rust: drm: Wrap ioctl dispatch in UnbindGuard
> rust: drm: Pass registration data to ioctl handlers
> rust: drm: Pass bound parent device to ioctl handlers
Deborah, following up on your question in the other thread, this is roughly how
you'd use this in Tyr.
diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
index 7ac3707823b6..447e53c0eecf 100644
--- a/drivers/gpu/drm/tyr/driver.rs
+++ b/drivers/gpu/drm/tyr/driver.rs
@@ -37,13 +37,17 @@
regs, //
};
-pub(crate) type IoMem = kernel::io::Mmio<SZ_2M>;
+pub(crate) type IoMem<'bound> = kernel::io::mem::IoMem<'bound, SZ_2M>;
pub(crate) struct TyrDrmDriver;
/// Convenience type alias for the DRM device type for this driver.
pub(crate) type TyrDrmDevice<Ctx = drm::Registered> = drm::Device<TyrDrmDriver, Ctx>;
+pub(crate) struct RegData<'bound> {
+ pub(crate) iomem: IoMem<'bound>,
+}
+
#[pin_data(PinnedDrop)]
pub(crate) struct TyrPlatformDriverData {
_device: ARef<TyrDrmDevice>,
@@ -65,7 +69,7 @@ pub(crate) struct TyrDrmDeviceData {
pub(crate) gpu_info: GpuInfo,
}
-fn issue_soft_reset(dev: &Device<Bound>, iomem: &IoMem) -> Result {
+fn issue_soft_reset(dev: &Device<Bound>, iomem: &IoMem<'_>) -> Result {
regs::GPU_CMD.write(iomem, regs::GPU_CMD_SOFT_RESET);
poll::read_poll_timeout(
@@ -133,8 +137,10 @@ fn probe(
gpu_info,
});
+ let reg_data = RegData { iomem };
+
let tdev = drm::UnregisteredDevice::<TyrDrmDriver>::new(pdev, data)?;
- let tdev = drm::driver::Registration::new_foreign_owned(tdev, pdev.as_ref(), (), 0)?;
+ let tdev = drm::driver::Registration::new_foreign_owned(tdev, pdev.as_ref(), reg_data, 0)?;
let driver = TyrPlatformDriverData {
_device: tdev.into(),
@@ -176,7 +182,7 @@ fn drop(self: Pin<&mut Self>) {
#[vtable]
impl drm::Driver for TyrDrmDriver {
type Data = TyrDrmDeviceData;
- type RegistrationData = ForLt!(());
+ type RegistrationData = ForLt!(for<'a> RegData<'a>);
type File = TyrDrmFileData;
type Object<R: drm::DeviceContext> = drm::gem::Object<TyrObject, R>;
type ParentDevice<Ctx: DeviceContext> = platform::Device<Ctx>;
diff --git a/drivers/gpu/drm/tyr/file.rs b/drivers/gpu/drm/tyr/file.rs
index 9f53da7633ab..30efdf924cc2 100644
--- a/drivers/gpu/drm/tyr/file.rs
+++ b/drivers/gpu/drm/tyr/file.rs
@@ -10,6 +10,7 @@
};
use crate::driver::{
+ RegData,
TyrDrmDevice,
TyrDrmDriver, //
};
@@ -32,10 +33,12 @@ impl TyrDrmFileData {
pub(crate) fn dev_query(
ddev: &TyrDrmDevice,
_pdev: &platform::Device<device::Bound>,
- _reg_data: &(),
+ reg_data: &RegData<'_>,
devquery: &mut uapi::drm_panthor_dev_query,
_file: &TyrDrmFile,
) -> Result<u32> {
+ let _iomem = reg_data.iomem;
+
if devquery.pointer == 0 {
match devquery.type_ {
uapi::drm_panthor_dev_query_type_DRM_PANTHOR_DEV_QUERY_GPU_INFO => {
diff --git a/drivers/gpu/drm/tyr/gpu.rs b/drivers/gpu/drm/tyr/gpu.rs
index bb0473c85bf7..f169fb1ccc03 100644
--- a/drivers/gpu/drm/tyr/gpu.rs
+++ b/drivers/gpu/drm/tyr/gpu.rs
@@ -34,7 +34,7 @@
pub(crate) struct GpuInfo(pub(crate) uapi::drm_panthor_gpu_info);
impl GpuInfo {
- pub(crate) fn new(iomem: &IoMem) -> Self {
+ pub(crate) fn new(iomem: &IoMem<'_>) -> Self {
let gpu_id = regs::GPU_ID.read(iomem);
let csf_id = regs::GPU_CSF_ID.read(iomem);
let gpu_rev = regs::GPU_REVID.read(iomem);
@@ -206,7 +206,7 @@ fn from(value: u32) -> Self {
}
/// Powers on the l2 block.
-pub(crate) fn l2_power_on(dev: &Device<Bound>, iomem: &IoMem) -> Result {
+pub(crate) fn l2_power_on(dev: &Device<Bound>, iomem: &IoMem<'_>) -> Result {
regs::L2_PWRON_LO.write(iomem, 1);
poll::read_poll_timeout(
diff --git a/drivers/gpu/drm/tyr/regs.rs b/drivers/gpu/drm/tyr/regs.rs
index 0881b3812afd..2b0da6019512 100644
--- a/drivers/gpu/drm/tyr/regs.rs
+++ b/drivers/gpu/drm/tyr/regs.rs
@@ -20,12 +20,12 @@
impl<const OFFSET: usize> Register<OFFSET> {
#[inline]
- pub(crate) fn read(&self, iomem: &IoMem) -> u32 {
+ pub(crate) fn read(&self, iomem: &IoMem<'_>) -> u32 {
iomem.read32(OFFSET)
}
#[inline]
- pub(crate) fn write(&self, iomem: &IoMem, value: u32) {
+ pub(crate) fn write(&self, iomem: &IoMem<'_>, value: u32) {
iomem.write32(value, OFFSET);
}
}
On Thu, May 07, 2026 at 11:38:37AM +0200, Danilo Krummrich wrote:
> On Thu May 7, 2026 at 12:05 AM CEST, Danilo Krummrich wrote:
> > Danilo Krummrich (6):
> > rust: drm: Add Driver::ParentDevice associated type
> > rust: drm: Add UnbindGuard for drm_dev_enter/exit critical sections
> > rust: drm: Add RegistrationData to drm::Driver
> > rust: drm: Wrap ioctl dispatch in UnbindGuard
> > rust: drm: Pass registration data to ioctl handlers
> > rust: drm: Pass bound parent device to ioctl handlers
>
> Deborah, following up on your question in the other thread, this is roughly how
> you'd use this in Tyr.
Hi Danilo,
Thanks for this advice, I've been working on applying these changes
to Tyr's MCU/firmware boot series. One problem i've had is that we used
to be able to get an unregistered device without providing the drm::Driver
type Data like this:
let unreg_dev = drm::UnregisteredDevice::<TyrDrmDriver>::new(pdev)?;
Then Tyr used that unregistered device to create a gem::shmem::Object
and initialize a gpuvm tree which we used to map the shared memory
to load the firmware.
Has the design changed so that the drm::Driver type Data must be
provided now to get the unregistered devices? Like this?
let unreg_dev = drm::UnregisteredDevice::<TyrDrmDriver>::new(pdev, data)?;
Just checking that this is what we need to work with? If so, I was
thinking of just simplifying the firmware boot to use a small coherent
dma allocation instead like this:
https://gitlab.freedesktop.org/dbrouwer/linux/-/merge_requests/3/diffs?commit_id=39be9140e22c6252317082a1424a66c35dc004b3
Could you let me know if this aligns with your driver core changes?
Deborah
>
> diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
> index 7ac3707823b6..447e53c0eecf 100644
> --- a/drivers/gpu/drm/tyr/driver.rs
> +++ b/drivers/gpu/drm/tyr/driver.rs
> @@ -37,13 +37,17 @@
> regs, //
> };
>
> -pub(crate) type IoMem = kernel::io::Mmio<SZ_2M>;
> +pub(crate) type IoMem<'bound> = kernel::io::mem::IoMem<'bound, SZ_2M>;
>
> pub(crate) struct TyrDrmDriver;
>
> /// Convenience type alias for the DRM device type for this driver.
> pub(crate) type TyrDrmDevice<Ctx = drm::Registered> = drm::Device<TyrDrmDriver, Ctx>;
>
> +pub(crate) struct RegData<'bound> {
> + pub(crate) iomem: IoMem<'bound>,
> +}
> +
> #[pin_data(PinnedDrop)]
> pub(crate) struct TyrPlatformDriverData {
> _device: ARef<TyrDrmDevice>,
> @@ -65,7 +69,7 @@ pub(crate) struct TyrDrmDeviceData {
> pub(crate) gpu_info: GpuInfo,
> }
>
> -fn issue_soft_reset(dev: &Device<Bound>, iomem: &IoMem) -> Result {
> +fn issue_soft_reset(dev: &Device<Bound>, iomem: &IoMem<'_>) -> Result {
> regs::GPU_CMD.write(iomem, regs::GPU_CMD_SOFT_RESET);
>
> poll::read_poll_timeout(
> @@ -133,8 +137,10 @@ fn probe(
> gpu_info,
> });
>
> + let reg_data = RegData { iomem };
> +
> let tdev = drm::UnregisteredDevice::<TyrDrmDriver>::new(pdev, data)?;
> - let tdev = drm::driver::Registration::new_foreign_owned(tdev, pdev.as_ref(), (), 0)?;
> + let tdev = drm::driver::Registration::new_foreign_owned(tdev, pdev.as_ref(), reg_data, 0)?;
>
> let driver = TyrPlatformDriverData {
> _device: tdev.into(),
> @@ -176,7 +182,7 @@ fn drop(self: Pin<&mut Self>) {
> #[vtable]
> impl drm::Driver for TyrDrmDriver {
> type Data = TyrDrmDeviceData;
> - type RegistrationData = ForLt!(());
> + type RegistrationData = ForLt!(for<'a> RegData<'a>);
> type File = TyrDrmFileData;
> type Object<R: drm::DeviceContext> = drm::gem::Object<TyrObject, R>;
> type ParentDevice<Ctx: DeviceContext> = platform::Device<Ctx>;
> diff --git a/drivers/gpu/drm/tyr/file.rs b/drivers/gpu/drm/tyr/file.rs
> index 9f53da7633ab..30efdf924cc2 100644
> --- a/drivers/gpu/drm/tyr/file.rs
> +++ b/drivers/gpu/drm/tyr/file.rs
> @@ -10,6 +10,7 @@
> };
>
> use crate::driver::{
> + RegData,
> TyrDrmDevice,
> TyrDrmDriver, //
> };
> @@ -32,10 +33,12 @@ impl TyrDrmFileData {
> pub(crate) fn dev_query(
> ddev: &TyrDrmDevice,
> _pdev: &platform::Device<device::Bound>,
> - _reg_data: &(),
> + reg_data: &RegData<'_>,
> devquery: &mut uapi::drm_panthor_dev_query,
> _file: &TyrDrmFile,
> ) -> Result<u32> {
> + let _iomem = reg_data.iomem;
> +
> if devquery.pointer == 0 {
> match devquery.type_ {
> uapi::drm_panthor_dev_query_type_DRM_PANTHOR_DEV_QUERY_GPU_INFO => {
> diff --git a/drivers/gpu/drm/tyr/gpu.rs b/drivers/gpu/drm/tyr/gpu.rs
> index bb0473c85bf7..f169fb1ccc03 100644
> --- a/drivers/gpu/drm/tyr/gpu.rs
> +++ b/drivers/gpu/drm/tyr/gpu.rs
> @@ -34,7 +34,7 @@
> pub(crate) struct GpuInfo(pub(crate) uapi::drm_panthor_gpu_info);
>
> impl GpuInfo {
> - pub(crate) fn new(iomem: &IoMem) -> Self {
> + pub(crate) fn new(iomem: &IoMem<'_>) -> Self {
> let gpu_id = regs::GPU_ID.read(iomem);
> let csf_id = regs::GPU_CSF_ID.read(iomem);
> let gpu_rev = regs::GPU_REVID.read(iomem);
> @@ -206,7 +206,7 @@ fn from(value: u32) -> Self {
> }
>
> /// Powers on the l2 block.
> -pub(crate) fn l2_power_on(dev: &Device<Bound>, iomem: &IoMem) -> Result {
> +pub(crate) fn l2_power_on(dev: &Device<Bound>, iomem: &IoMem<'_>) -> Result {
> regs::L2_PWRON_LO.write(iomem, 1);
>
> poll::read_poll_timeout(
> diff --git a/drivers/gpu/drm/tyr/regs.rs b/drivers/gpu/drm/tyr/regs.rs
> index 0881b3812afd..2b0da6019512 100644
> --- a/drivers/gpu/drm/tyr/regs.rs
> +++ b/drivers/gpu/drm/tyr/regs.rs
> @@ -20,12 +20,12 @@
>
> impl<const OFFSET: usize> Register<OFFSET> {
> #[inline]
> - pub(crate) fn read(&self, iomem: &IoMem) -> u32 {
> + pub(crate) fn read(&self, iomem: &IoMem<'_>) -> u32 {
> iomem.read32(OFFSET)
> }
>
> #[inline]
> - pub(crate) fn write(&self, iomem: &IoMem, value: u32) {
> + pub(crate) fn write(&self, iomem: &IoMem<'_>, value: u32) {
> iomem.write32(value, OFFSET);
> }
> }
On Thu May 14, 2026 at 8:59 PM CEST, Deborah Brouwer wrote: > let unreg_dev = drm::UnregisteredDevice::<TyrDrmDriver>::new(pdev, data)?; You shouldn't need this anymore as the drm::Registration itself has private data now that is bound to the lifetime of the underlying bus device, which should be the correct lifetime for juggling the GPU page tables.
On Thu, 14 May 2026 21:07:11 +0200 "Danilo Krummrich" <dakr@kernel.org> wrote: > On Thu May 14, 2026 at 8:59 PM CEST, Deborah Brouwer wrote: > > let unreg_dev = drm::UnregisteredDevice::<TyrDrmDriver>::new(pdev, data)?; > > You shouldn't need this anymore as the drm::Registration itself has private data > now that is bound to the lifetime of the underlying bus device, which should be > the correct lifetime for juggling the GPU page tables. The problem we have is that, to initialize some of the sub-components of the driver, we need to be able to allocate GEM objects before the DRM device is exposed (registered), and because the data we want to attach to the final registration contains these sub-components, we need to defer the data assignment to the registration step (which was allowed by [1], but apparently this was dropped from the latest version of the series for some reason). [1]https://lore.kernel.org/dri-devel/20260320233645.950190-4-lyude@redhat.com/
On Tue May 19, 2026 at 8:59 PM CEST, Boris Brezillon wrote: > On Thu, 14 May 2026 21:07:11 +0200 > "Danilo Krummrich" <dakr@kernel.org> wrote: > >> On Thu May 14, 2026 at 8:59 PM CEST, Deborah Brouwer wrote: >> > let unreg_dev = drm::UnregisteredDevice::<TyrDrmDriver>::new(pdev, data)?; >> >> You shouldn't need this anymore as the drm::Registration itself has private data >> now that is bound to the lifetime of the underlying bus device, which should be >> the correct lifetime for juggling the GPU page tables. > > The problem we have is that, to initialize some of the sub-components > of the driver, we need to be able to allocate GEM objects before the DRM > device is exposed (registered), and because the data we want to attach > to the final registration contains these sub-components, we need to > defer the data assignment to the registration step (which was allowed > by [1], but apparently this was dropped from the latest version of > the series for some reason). I'm perfectly aware, what I'm saying above is that you probably want to store those GEM objects (that you can create with the previously allocated drm::Device) in the drm::Registration data. This fulfills this requirement *and* ties the lifetime of those GEM objects to the lifetime of the underlying bus device being bound to the driver, which is exactly what you want for GEM objects that contain the device page tables.
On Tue, 19 May 2026 21:28:17 +0200 "Danilo Krummrich" <dakr@kernel.org> wrote: > On Tue May 19, 2026 at 8:59 PM CEST, Boris Brezillon wrote: > > On Thu, 14 May 2026 21:07:11 +0200 > > "Danilo Krummrich" <dakr@kernel.org> wrote: > > > >> On Thu May 14, 2026 at 8:59 PM CEST, Deborah Brouwer wrote: > >> > let unreg_dev = drm::UnregisteredDevice::<TyrDrmDriver>::new(pdev, data)?; > >> > >> You shouldn't need this anymore as the drm::Registration itself has private data > >> now that is bound to the lifetime of the underlying bus device, which should be > >> the correct lifetime for juggling the GPU page tables. > > > > The problem we have is that, to initialize some of the sub-components > > of the driver, we need to be able to allocate GEM objects before the DRM > > device is exposed (registered), and because the data we want to attach > > to the final registration contains these sub-components, we need to > > defer the data assignment to the registration step (which was allowed > > by [1], but apparently this was dropped from the latest version of > > the series for some reason). > > I'm perfectly aware, what I'm saying above is that you probably want to store > those GEM objects (that you can create with the previously allocated > drm::Device) in the drm::Registration data. This fulfills this requirement *and* > ties the lifetime of those GEM objects to the lifetime of the underlying bus > device being bound to the driver, which is exactly what you want for GEM objects > that contain the device page tables. Yeah, I don't think the problem Deborah was mentioning was about not being able to allocate GEMs early on, or making sure those GEMs don't outlive the bus-device. Oh, and BTW, we don't use GEMs for the page tables themselves, those still go through regular page allocation and are entirely handled by the io-pgtable-arm.c logic. We need early GEM allocation to populate FW sections, and to map those to the VM attached to the FW we need GPUVM too (because the FW also lives behind the GPU MMU and has its own page table that's populated by the driver). What I think Deborah was pointing out is the chicken-and-egg issue, where the registration data is only fully populated after all sub-components of the drivers have been initialized, and one of these sub-components (the FW) needs to allocate GEMs and map them to its VM. So, GEM allocation and GPUVM are required are required early on, and both want a drm::Device of some sort (Unregistered or Registered). So the question is, are you happy with having [1] applied before/after your patchset to address this chicken-and-egg issue I mentioned, or are you proposing something else (Option<Subcomponent> so that we start with all sub-components set to None and progressively populate those, which I remember Daniel was strongly opposed to)? [1]https://lore.kernel.org/dri-devel/20260320233645.950190-4-lyude@redhat.com/
On Wed May 20, 2026 at 8:38 AM CEST, Boris Brezillon wrote:
> So, GEM allocation and GPUVM are required are required early on, and
> both want a drm::Device of some sort (Unregistered or Registered). So
> the question is, are you happy with having [1] applied before/after
> your patchset to address this chicken-and-egg issue I mentioned,
Yes, this is exactly what I had in mind for all of my replies, but I think I
only implied my key point rather than making it explicit, so let me do that now.
What I'm saying is that we now have all DRM ioctls behind the SRCU guard, which
means that the drm::Registration data is always available from DRM ioctls.
Now, with this precondition, the real question is whether there are any
resources left that should be bound to the lifetime of the DRM device itself,
which I don't think is the case.
After device unbind nothing is able to call back into the driver anymore, so
there is no reason to keep the corresponding data alive within the driver's
private data.
There may be resources that are kept alive through a separate reference count
while userspace still has open file descriptors, but they are separate reference
counts and should be independent from the driver's private data.
IWO, the fact that we guard the ioctls makes the whole class of problems go away
that we inherit from having to keep the driver's private data alive until after
remvoe() -- the DRM device private data becomes obsolete and we should probably
evene remove it.
Instead, we should use the drm::Registration private data for everything.
With this, your chicken-and-egg issue is already solved; the DRM device is
available, you can populate all the resources you need (GEM, GPUVM, etc.) and
then pass it into drm::Registration::new().
In terms of making any of the resources you store in the drm::Registration data
available in the bus device private data, it becomes the same pattern as for
everything else, i.e. you need shared ownership.
So, let's say you need IoMem<'bound> in both the drm::Registration data and in
the bus device private data, then you currently need Arc<IoMem<'bound>> in both
of them, which is fine, but I think we can do better.
Eventually, once we have landed the series I currently have in flight and once
Gary finished his self-referencial support in pin-init, I think we can even
avoid the reference count. Here is an example.
struct TyrPlatformDriverData<'bound> {
_device: ARef<TyrDrmDevice>,
_reg: drm::Registration<'bound, TyrDrmData<'_>>,
iomem: IoMem<'bound>,
}
struct TyrDrmData<'bound> {
iomem: &'bound IoMem<'bound>,
}
fn probe() -> ... {
try_pin_init!(TyrPlatformDriverData {
_device: ...,
iomem <- ...,
_reg <- drm::Registration::new(..., TyrDrmData { iomem: &iomem }),
})
}
I don't know how the pin-init self-referencial stuff will exactly turn out, but
this is pretty much what I hope we can evolve this into.
(And sorry again, now that I spelled this all out, having that left implicit in
my first reply was obviously unreasonable. :)
> or are you proposing something else (Option<Subcomponent> so that we start
> with all sub-components set to None and progressively populate those, which I
> remember Daniel was strongly opposed to)?
No, I am strongly opposed to do that (as well).
On Wed, 20 May 2026 12:55:40 +0200
"Danilo Krummrich" <dakr@kernel.org> wrote:
> On Wed May 20, 2026 at 8:38 AM CEST, Boris Brezillon wrote:
> > So, GEM allocation and GPUVM are required are required early on, and
> > both want a drm::Device of some sort (Unregistered or Registered). So
> > the question is, are you happy with having [1] applied before/after
> > your patchset to address this chicken-and-egg issue I mentioned,
>
> Yes, this is exactly what I had in mind for all of my replies, but I think I
> only implied my key point rather than making it explicit, so let me do that now.
>
> What I'm saying is that we now have all DRM ioctls behind the SRCU guard, which
> means that the drm::Registration data is always available from DRM ioctls.
>
> Now, with this precondition, the real question is whether there are any
> resources left that should be bound to the lifetime of the DRM device itself,
> which I don't think is the case.
>
> After device unbind nothing is able to call back into the driver anymore, so
> there is no reason to keep the corresponding data alive within the driver's
> private data.
>
> There may be resources that are kept alive through a separate reference count
> while userspace still has open file descriptors, but they are separate reference
> counts and should be independent from the driver's private data.
>
> IWO, the fact that we guard the ioctls makes the whole class of problems go away
> that we inherit from having to keep the driver's private data alive until after
> remvoe() -- the DRM device private data becomes obsolete and we should probably
> evene remove it.
>
> Instead, we should use the drm::Registration private data for everything.
>
> With this, your chicken-and-egg issue is already solved; the DRM device is
> available, you can populate all the resources you need (GEM, GPUVM, etc.) and
> then pass it into drm::Registration::new().
Okay, it starts to click. So ioctls get passed the drm::Registration,
which has both a device and some driver specific data (I guess it's
even hidden behind the File abstraction). During early probe, we'd still
be able to pass an Unregistered variant of the device, and then our
current TyrDrmDeviceData would be attached to the Registration object
when Registration::new_foreign_owned() is called.
This probably imply changing the places where we currently access our
TyrDrmDeviceData through the drm::Device, but hopefully there's not
many of those cases.
>
> In terms of making any of the resources you store in the drm::Registration data
> available in the bus device private data, it becomes the same pattern as for
> everything else, i.e. you need shared ownership.
Yep, that we can solve.
>
> So, let's say you need IoMem<'bound> in both the drm::Registration data and in
> the bus device private data, then you currently need Arc<IoMem<'bound>> in both
> of them, which is fine, but I think we can do better.
>
> Eventually, once we have landed the series I currently have in flight and once
> Gary finished his self-referencial support in pin-init, I think we can even
> avoid the reference count. Here is an example.
>
> struct TyrPlatformDriverData<'bound> {
> _device: ARef<TyrDrmDevice>,
> _reg: drm::Registration<'bound, TyrDrmData<'_>>,
> iomem: IoMem<'bound>,
> }
>
> struct TyrDrmData<'bound> {
> iomem: &'bound IoMem<'bound>,
> }
>
> fn probe() -> ... {
> try_pin_init!(TyrPlatformDriverData {
> _device: ...,
> iomem <- ...,
> _reg <- drm::Registration::new(..., TyrDrmData { iomem: &iomem }),
> })
> }
>
> I don't know how the pin-init self-referencial stuff will exactly turn out, but
> this is pretty much what I hope we can evolve this into.
Yep, makes sense.
>
> (And sorry again, now that I spelled this all out, having that left implicit in
> my first reply was obviously unreasonable. :)
No worries, that's the usual back-and-forth happening during reviews,
and it's all fine.
>
> > or are you proposing something else (Option<Subcomponent> so that we start
> > with all sub-components set to None and progressively populate those, which I
> > remember Daniel was strongly opposed to)?
>
> No, I am strongly opposed to do that (as well).
Got it.
On Tue, 2026-05-19 at 20:59 +0200, Boris Brezillon wrote: > On Thu, 14 May 2026 21:07:11 +0200 > "Danilo Krummrich" <dakr@kernel.org> wrote: > > > On Thu May 14, 2026 at 8:59 PM CEST, Deborah Brouwer wrote: > > > let unreg_dev = > > > drm::UnregisteredDevice::<TyrDrmDriver>::new(pdev, data)?; > > > > You shouldn't need this anymore as the drm::Registration itself has > > private data > > now that is bound to the lifetime of the underlying bus device, > > which should be > > the correct lifetime for juggling the GPU page tables. > > The problem we have is that, to initialize some of the sub-components > of the driver, we need to be able to allocate GEM objects before the > DRM > device is exposed (registered), and because the data we want to > attach > to the final registration contains these sub-components, we need to > defer the data assignment to the registration step (which was allowed > by [1], but apparently this was dropped from the latest version of > the series for some reason). > > [1] > https://lore.kernel.org/dri-devel/20260320233645.950190-4-lyude@redha > t.com/ I dropped it because when talking with Danilo I had the impression that we weren't going to need this (and I thought we explicitly had mentioned it when talking with eachother). But I'm always happy to add it back
© 2016 - 2026 Red Hat, Inc.