Re: [PATCH REF 24/24] gpu: drm: tyr: use HRT lifetime for IoMem

Re: [PATCH REF 24/24] gpu: drm: tyr: use HRT lifetime for IoMem

[Deborah Brouwer]

On Tue, Apr 28, 2026 at 12:11:22AM +0200, Danilo Krummrich wrote:
> Take advantage of the lifetime-parameterized IoMem<'a> to use the
> memory mapping directly during probe, eliminating the Arc<Devres<IoMem>>
> indirection.
> 
> Since the IoMem is only used during probe, this also simplifies
> Register::read/write to be infallible -- the Devres access check is no
> longer needed, so reads return u32 directly and writes return ().

Hi Danilo,
Is the intended model that DRM drivers keep lifetime-bound resources such as
IoMem<'bound> only in platform drvdata and access them via Device::drvdata_borrow()?

Or is the expectation that drm::Driver should also have a lifetime-parameterized
Data associated type?

The reason I ask is that Tyr currently stores an MMIO handle in several areas,
(firmware, MMU/address-space management, and IRQ handling)  and it does register
accesses directly. See what we're trying to do:
https://lore.kernel.org/rust-for-linux/20260424-b4-fw-boot-v4-v4-0-a5d91050789d@collabora.com/

Moving IoMem<'bound> only into platform drvdata would require a big refactor
to thread &IoMem<'_> through those paths or fetch it from drvdata at each hardware
access site. So, I wanted to clarify the plan first before I start this work.

Thanks,
Deborah

> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
> Not yet updated to Tyr using the register!() macro, but probably good enough for
> reference.
> ---
>  drivers/gpu/drm/tyr/driver.rs | 14 ++++----
>  drivers/gpu/drm/tyr/gpu.rs    | 62 +++++++++++++++++------------------
>  drivers/gpu/drm/tyr/regs.rs   | 21 +++---------
>  3 files changed, 41 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
> index eaa84efdfdf7..d305ad433e03 100644
> --- a/drivers/gpu/drm/tyr/driver.rs
> +++ b/drivers/gpu/drm/tyr/driver.rs
> @@ -10,7 +10,6 @@
>          Core,
>          Device, //
>      },
> -    devres::Devres,
>      drm,
>      drm::ioctl,
>      io::poll,
> @@ -23,7 +22,6 @@
>      sizes::SZ_2M,
>      sync::{
>          aref::ARef,
> -        Arc,
>          Mutex, //
>      },
>      time, //
> @@ -37,7 +35,7 @@
>      regs, //
>  };
>  
> -pub(crate) type IoMem = kernel::io::mem::IoMem<'static, SZ_2M>;
> +pub(crate) type IoMem = kernel::io::Mmio<SZ_2M>;
>  
>  pub(crate) struct TyrDrmDriver;
>  
> @@ -65,11 +63,11 @@ pub(crate) struct TyrDrmDeviceData {
>      pub(crate) gpu_info: GpuInfo,
>  }
>  
> -fn issue_soft_reset(dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result {
> -    regs::GPU_CMD.write(dev, iomem, regs::GPU_CMD_SOFT_RESET)?;
> +fn issue_soft_reset(dev: &Device<Bound>, iomem: &IoMem) -> Result {
> +    regs::GPU_CMD.write(iomem, regs::GPU_CMD_SOFT_RESET);
>  
>      poll::read_poll_timeout(
> -        || regs::GPU_IRQ_RAWSTAT.read(dev, iomem),
> +        || Ok(regs::GPU_IRQ_RAWSTAT.read(iomem)),
>          |status| *status & regs::GPU_IRQ_RAWSTAT_RESET_COMPLETED != 0,
>          time::Delta::from_millis(1),
>          time::Delta::from_millis(100),
> @@ -109,12 +107,12 @@ fn probe(
>          let sram_regulator = Regulator::<regulator::Enabled>::get(pdev.as_ref(), c"sram")?;
>  
>          let request = pdev.io_request_by_index(0).ok_or(ENODEV)?;
> -        let iomem = Arc::new(request.iomap_sized::<SZ_2M>()?.into_devres()?, GFP_KERNEL)?;
> +        let iomem = request.iomap_sized::<SZ_2M>()?;
>  
>          issue_soft_reset(pdev.as_ref(), &iomem)?;
>          gpu::l2_power_on(pdev.as_ref(), &iomem)?;
>  
> -        let gpu_info = GpuInfo::new(pdev.as_ref(), &iomem)?;
> +        let gpu_info = GpuInfo::new(&iomem);
>          gpu_info.log(pdev);
>  
>          let platform: ARef<platform::Device> = pdev.into();
> diff --git a/drivers/gpu/drm/tyr/gpu.rs b/drivers/gpu/drm/tyr/gpu.rs
> index a88775160f98..bb0473c85bf7 100644
> --- a/drivers/gpu/drm/tyr/gpu.rs
> +++ b/drivers/gpu/drm/tyr/gpu.rs
> @@ -10,7 +10,6 @@
>          Bound,
>          Device, //
>      },
> -    devres::Devres,
>      io::poll,
>      platform,
>      prelude::*,
> @@ -35,37 +34,36 @@
>  pub(crate) struct GpuInfo(pub(crate) uapi::drm_panthor_gpu_info);
>  
>  impl GpuInfo {
> -    pub(crate) fn new(dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result<Self> {
> -        let gpu_id = regs::GPU_ID.read(dev, iomem)?;
> -        let csf_id = regs::GPU_CSF_ID.read(dev, iomem)?;
> -        let gpu_rev = regs::GPU_REVID.read(dev, iomem)?;
> -        let core_features = regs::GPU_CORE_FEATURES.read(dev, iomem)?;
> -        let l2_features = regs::GPU_L2_FEATURES.read(dev, iomem)?;
> -        let tiler_features = regs::GPU_TILER_FEATURES.read(dev, iomem)?;
> -        let mem_features = regs::GPU_MEM_FEATURES.read(dev, iomem)?;
> -        let mmu_features = regs::GPU_MMU_FEATURES.read(dev, iomem)?;
> -        let thread_features = regs::GPU_THREAD_FEATURES.read(dev, iomem)?;
> -        let max_threads = regs::GPU_THREAD_MAX_THREADS.read(dev, iomem)?;
> -        let thread_max_workgroup_size = regs::GPU_THREAD_MAX_WORKGROUP_SIZE.read(dev, iomem)?;
> -        let thread_max_barrier_size = regs::GPU_THREAD_MAX_BARRIER_SIZE.read(dev, iomem)?;
> -        let coherency_features = regs::GPU_COHERENCY_FEATURES.read(dev, iomem)?;
> -
> -        let texture_features = regs::GPU_TEXTURE_FEATURES0.read(dev, iomem)?;
> -
> -        let as_present = regs::GPU_AS_PRESENT.read(dev, iomem)?;
> -
> -        let shader_present = u64::from(regs::GPU_SHADER_PRESENT_LO.read(dev, iomem)?);
> +    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);
> +        let core_features = regs::GPU_CORE_FEATURES.read(iomem);
> +        let l2_features = regs::GPU_L2_FEATURES.read(iomem);
> +        let tiler_features = regs::GPU_TILER_FEATURES.read(iomem);
> +        let mem_features = regs::GPU_MEM_FEATURES.read(iomem);
> +        let mmu_features = regs::GPU_MMU_FEATURES.read(iomem);
> +        let thread_features = regs::GPU_THREAD_FEATURES.read(iomem);
> +        let max_threads = regs::GPU_THREAD_MAX_THREADS.read(iomem);
> +        let thread_max_workgroup_size = regs::GPU_THREAD_MAX_WORKGROUP_SIZE.read(iomem);
> +        let thread_max_barrier_size = regs::GPU_THREAD_MAX_BARRIER_SIZE.read(iomem);
> +        let coherency_features = regs::GPU_COHERENCY_FEATURES.read(iomem);
> +
> +        let texture_features = regs::GPU_TEXTURE_FEATURES0.read(iomem);
> +
> +        let as_present = regs::GPU_AS_PRESENT.read(iomem);
> +
> +        let shader_present = u64::from(regs::GPU_SHADER_PRESENT_LO.read(iomem));
>          let shader_present =
> -            shader_present | u64::from(regs::GPU_SHADER_PRESENT_HI.read(dev, iomem)?) << 32;
> +            shader_present | u64::from(regs::GPU_SHADER_PRESENT_HI.read(iomem)) << 32;
>  
> -        let tiler_present = u64::from(regs::GPU_TILER_PRESENT_LO.read(dev, iomem)?);
> -        let tiler_present =
> -            tiler_present | u64::from(regs::GPU_TILER_PRESENT_HI.read(dev, iomem)?) << 32;
> +        let tiler_present = u64::from(regs::GPU_TILER_PRESENT_LO.read(iomem));
> +        let tiler_present = tiler_present | u64::from(regs::GPU_TILER_PRESENT_HI.read(iomem)) << 32;
>  
> -        let l2_present = u64::from(regs::GPU_L2_PRESENT_LO.read(dev, iomem)?);
> -        let l2_present = l2_present | u64::from(regs::GPU_L2_PRESENT_HI.read(dev, iomem)?) << 32;
> +        let l2_present = u64::from(regs::GPU_L2_PRESENT_LO.read(iomem));
> +        let l2_present = l2_present | u64::from(regs::GPU_L2_PRESENT_HI.read(iomem)) << 32;
>  
> -        Ok(Self(uapi::drm_panthor_gpu_info {
> +        Self(uapi::drm_panthor_gpu_info {
>              gpu_id,
>              gpu_rev,
>              csf_id,
> @@ -88,7 +86,7 @@ pub(crate) fn new(dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result<Self> {
>              core_features,
>              pad: 0,
>              gpu_features: 0,
> -        }))
> +        })
>      }
>  
>      pub(crate) fn log(&self, pdev: &platform::Device) {
> @@ -208,11 +206,11 @@ fn from(value: u32) -> Self {
>  }
>  
>  /// Powers on the l2 block.
> -pub(crate) fn l2_power_on(dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result {
> -    regs::L2_PWRON_LO.write(dev, iomem, 1)?;
> +pub(crate) fn l2_power_on(dev: &Device<Bound>, iomem: &IoMem) -> Result {
> +    regs::L2_PWRON_LO.write(iomem, 1);
>  
>      poll::read_poll_timeout(
> -        || regs::L2_READY_LO.read(dev, iomem),
> +        || Ok(regs::L2_READY_LO.read(iomem)),
>          |status| *status == 1,
>          Delta::from_millis(1),
>          Delta::from_millis(100),
> diff --git a/drivers/gpu/drm/tyr/regs.rs b/drivers/gpu/drm/tyr/regs.rs
> index 611870c2e6af..0881b3812afd 100644
> --- a/drivers/gpu/drm/tyr/regs.rs
> +++ b/drivers/gpu/drm/tyr/regs.rs
> @@ -7,16 +7,7 @@
>  // does.
>  #![allow(dead_code)]
>  
> -use kernel::{
> -    bits::bit_u32,
> -    device::{
> -        Bound,
> -        Device, //
> -    },
> -    devres::Devres,
> -    io::Io,
> -    prelude::*, //
> -};
> +use kernel::{bits::bit_u32, io::Io};
>  
>  use crate::driver::IoMem;
>  
> @@ -29,15 +20,13 @@
>  
>  impl<const OFFSET: usize> Register<OFFSET> {
>      #[inline]
> -    pub(crate) fn read(&self, dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result<u32> {
> -        let value = (*iomem).access(dev)?.read32(OFFSET);
> -        Ok(value)
> +    pub(crate) fn read(&self, iomem: &IoMem) -> u32 {
> +        iomem.read32(OFFSET)
>      }
>  
>      #[inline]
> -    pub(crate) fn write(&self, dev: &Device<Bound>, iomem: &Devres<IoMem>, value: u32) -> Result {
> -        (*iomem).access(dev)?.write32(value, OFFSET);
> -        Ok(())
> +    pub(crate) fn write(&self, iomem: &IoMem, value: u32) {
> +        iomem.write32(value, OFFSET);
>      }
>  }
>  
> -- 
> 2.54.0
> 

Re: [PATCH REF 24/24] gpu: drm: tyr: use HRT lifetime for IoMem

[Danilo Krummrich]

On Wed May 6, 2026 at 12:56 AM CEST, Deborah Brouwer wrote:
> Is the intended model that DRM drivers keep lifetime-bound resources such as
> IoMem<'bound> only in platform drvdata and access them via Device::drvdata_borrow()?

No, this method is unsafe and intended for busses only, drivers should never use
it directly.

> Or is the expectation that drm::Driver should also have a lifetime-parameterized
> Data associated type?

Exactly, the plan is to have lifetime-parameterized private data on the
drm::Registration, which is available in IOCTLs. The cover letter mentions that
briefly:

    A follow-up series extends this to class device registrations, starting with
    DRM, so that class device callbacks (IOCTLs, etc.) can safely access device
    resources through the separate registration data bound to the registration's
    lifetime without Devres indirection.

This becomes possible as I also have patches that protect DRM IOCTLs through the
drm::UnbindGuard (which is just drm_dev_enter() and drm_dev_exit()). Together
with drm_dev_unplug() being called when the drm::Registration is dropped all
IOCTLs are guaranteed to provide a bound scope of the underlying bus device and
hence the device resources in the DRM registration data availble through the
IOCTLs.

I have all those patches ready and I plan to send them this week.

> The reason I ask is that Tyr currently stores an MMIO handle in several areas,
> (firmware, MMU/address-space management, and IRQ handling)  and it does register
> accesses directly. See what we're trying to do:
> https://lore.kernel.org/rust-for-linux/20260424-b4-fw-boot-v4-v4-0-a5d91050789d@collabora.com/

Note that you are not blocked by this, you can always move IoMem<'_> into a
Devres through IoMem::into_devres() and use it without lifetime bounds. But of
course the goal is to make this unnecessary.

> Moving IoMem<'bound> only into platform drvdata would require a big refactor
> to thread &IoMem<'_> through those paths or fetch it from drvdata at each hardware
> access site. So, I wanted to clarify the plan first before I start this work.

No worries, that'd be the exact opposite of what we want to achieve. :)