Re: [PATCH v4 1/6] drm/tyr: Use register! macro for GPU_CONTROL

["Gary Guo" <gary@garyguo.net>]

On Fri Apr 3, 2026 at 12:35 AM BST, Deborah Brouwer wrote:
> From: Daniel Almeida <daniel.almeida@collabora.com>
>
> Convert the GPU_CONTROL register definitions to use the `register!` macro.
>
> Using the `register!` macro allows us to replace manual bit masks and
> shifts with typed register and field accessors, which makes the code
> easier to read and avoids errors from bit manipulation.
>
> Acked-by: Boris Brezillon <boris.brezillon@collabora.com>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> Co-developed-by: Deborah Brouwer <deborah.brouwer@collabora.com>
> Signed-off-by: Deborah Brouwer <deborah.brouwer@collabora.com>
> ---
>  drivers/gpu/drm/tyr/driver.rs |  24 +-
>  drivers/gpu/drm/tyr/gpu.rs    | 232 +++++------
>  drivers/gpu/drm/tyr/regs.rs   | 909 +++++++++++++++++++++++++++++++++++++++---
>  3 files changed, 969 insertions(+), 196 deletions(-)
>
> diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
> index 611434641580574ec6b5afa49a8fe79888bb7ace..3ebb5e08bfca342f136e8d365b1d9dcb6cc3dbca 100644
> --- a/drivers/gpu/drm/tyr/driver.rs
> +++ b/drivers/gpu/drm/tyr/driver.rs
> @@ -13,7 +13,10 @@
>      devres::Devres,
>      drm,
>      drm::ioctl,
> -    io::poll,
> +    io::{
> +        poll,
> +        Io, //
> +    },
>      new_mutex,
>      of,
>      platform,
> @@ -33,8 +36,11 @@
>      file::TyrDrmFileData,
>      gem::TyrObject,
>      gpu,
> -    gpu::GpuInfo,
> -    regs, //
> +    gpu::{
> +        gpu_info_log, //
> +        GpuInfo,
> +    },
> +    regs::gpu_control::*, //
>  };
>  
>  pub(crate) type IoMem = kernel::io::mem::IoMem<SZ_2M>;
> @@ -78,11 +84,15 @@ unsafe impl Send for TyrDrmDeviceData {}
>  unsafe impl Sync for TyrDrmDeviceData {}
>  
>  fn issue_soft_reset(dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result {
> -    regs::GPU_CMD.write(dev, iomem, regs::GPU_CMD_SOFT_RESET)?;
> +    let io = (*iomem).access(dev)?;
> +    io.write_reg(GPU_COMMAND::reset(ResetMode::SoftReset));
>  
>      poll::read_poll_timeout(
> -        || regs::GPU_IRQ_RAWSTAT.read(dev, iomem),
> -        |status| *status & regs::GPU_IRQ_RAWSTAT_RESET_COMPLETED != 0,
> +        || {
> +            let io = (*iomem).access(dev)?;
> +            Ok(io.read(GPU_IRQ_RAWSTAT))
> +        },
> +        |status| status.reset_completed(),
>          time::Delta::from_millis(1),
>          time::Delta::from_millis(100),
>      )
> @@ -127,7 +137,7 @@ fn probe(
>          gpu::l2_power_on(pdev.as_ref(), &iomem)?;
>  
>          let gpu_info = GpuInfo::new(pdev.as_ref(), &iomem)?;
> -        gpu_info.log(pdev);
> +        gpu_info_log(pdev.as_ref(), &iomem)?;

This causes all registers to be re-read again for some reason?

Why is the function signature of `gpu_info_log` changing from a method to a
standalone function? The commit message doesn't mention any.

>  
>          let platform: ARef<platform::Device> = pdev.into();
>  
> diff --git a/drivers/gpu/drm/tyr/gpu.rs b/drivers/gpu/drm/tyr/gpu.rs
> index a88775160f981e899e9c9b58debbda33e1b7244d..8ae39137a1d190ef026351d47a6cdd89063ed0fb 100644
> --- a/drivers/gpu/drm/tyr/gpu.rs
> +++ b/drivers/gpu/drm/tyr/gpu.rs
> @@ -5,14 +5,16 @@
>      DerefMut, //
>  };
>  use kernel::{
> -    bits::genmask_u32,
>      device::{
>          Bound,
>          Device, //
>      },
>      devres::Devres,
> -    io::poll,
> -    platform,
> +    io::{
> +        poll,
> +        register::Array,
> +        Io, //
> +    },
>      prelude::*,
>      time::Delta,
>      transmute::AsBytes,
> @@ -21,7 +23,10 @@
>  
>  use crate::{
>      driver::IoMem,
> -    regs, //
> +    regs::{
> +        gpu_control::*,
> +        join_u64, //
> +    }, //
>  };
>  
>  /// Struct containing information that can be queried by userspace. This is read from
> @@ -29,120 +34,55 @@
>  ///
>  /// # Invariants
>  ///
> -/// - The layout of this struct identical to the C `struct drm_panthor_gpu_info`.
> +/// - The layout of this struct is identical to the C `struct drm_panthor_gpu_info`.
>  #[repr(transparent)]
>  #[derive(Clone, Copy)]
>  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)?);
> -        let shader_present =
> -            shader_present | u64::from(regs::GPU_SHADER_PRESENT_HI.read(dev, 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 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 io = (*iomem).access(dev)?;
>  
>          Ok(Self(uapi::drm_panthor_gpu_info {
> -            gpu_id,
> -            gpu_rev,
> -            csf_id,
> -            l2_features,
> -            tiler_features,
> -            mem_features,
> -            mmu_features,
> -            thread_features,
> -            max_threads,
> -            thread_max_workgroup_size,
> -            thread_max_barrier_size,
> -            coherency_features,
> -            // TODO: Add texture_features_{1,2,3}.
> -            texture_features: [texture_features, 0, 0, 0],
> -            as_present,
> +            gpu_id: io.read(GPU_ID).into_raw(),
> +            gpu_rev: io.read(REVIDR).into_raw(),
> +            csf_id: io.read(CSF_ID).into_raw(),
> +            l2_features: io.read(L2_FEATURES).into_raw(),
> +            tiler_features: io.read(TILER_FEATURES).into_raw(),
> +            mem_features: io.read(MEM_FEATURES).into_raw(),
> +            mmu_features: io.read(MMU_FEATURES).into_raw(),
> +            thread_features: io.read(THREAD_FEATURES).into_raw(),
> +            max_threads: io.read(THREAD_MAX_THREADS).into_raw(),
> +            thread_max_workgroup_size: io.read(THREAD_MAX_WORKGROUP_SIZE).into_raw(),
> +            thread_max_barrier_size: io.read(THREAD_MAX_BARRIER_SIZE).into_raw(),
> +            coherency_features: io.read(COHERENCY_FEATURES).into_raw(),
> +            texture_features: [
> +                io.read(TEXTURE_FEATURES::at(0)).supported_formats().get(),
> +                io.read(TEXTURE_FEATURES::at(1)).supported_formats().get(),
> +                io.read(TEXTURE_FEATURES::at(2)).supported_formats().get(),
> +                io.read(TEXTURE_FEATURES::at(3)).supported_formats().get(),
> +            ],
> +            as_present: io.read(AS_PRESENT).into_raw(),
>              selected_coherency: uapi::drm_panthor_gpu_coherency_DRM_PANTHOR_GPU_COHERENCY_NONE,
> -            shader_present,
> -            l2_present,
> -            tiler_present,
> -            core_features,
> +            shader_present: join_u64(
> +                io.read(SHADER_PRESENT_LO).into_raw(),
> +                io.read(SHADER_PRESENT_HI).into_raw(),
> +            ),
> +            l2_present: join_u64(
> +                io.read(L2_PRESENT_LO).into_raw(),
> +                io.read(L2_PRESENT_HI).into_raw(),
> +            ),
> +            tiler_present: join_u64(
> +                io.read(TILER_PRESENT_LO).into_raw(),
> +                io.read(TILER_PRESENT_HI).into_raw(),
> +            ),
> +            core_features: io.read(CORE_FEATURES).into_raw(),
> +            // Padding must be zero.
>              pad: 0,
> +            //GPU_FEATURES register is not available; it was introduced in arch 11.x.
>              gpu_features: 0,
>          }))
>      }
> -
> -    pub(crate) fn log(&self, pdev: &platform::Device) {
> -        let gpu_id = GpuId::from(self.gpu_id);
> -
> -        let model_name = if let Some(model) = GPU_MODELS
> -            .iter()
> -            .find(|&f| f.arch_major == gpu_id.arch_major && f.prod_major == gpu_id.prod_major)
> -        {
> -            model.name
> -        } else {
> -            "unknown"
> -        };
> -
> -        dev_info!(
> -            pdev,
> -            "mali-{} id 0x{:x} major 0x{:x} minor 0x{:x} status 0x{:x}",
> -            model_name,
> -            self.gpu_id >> 16,
> -            gpu_id.ver_major,
> -            gpu_id.ver_minor,
> -            gpu_id.ver_status
> -        );
> -
> -        dev_info!(
> -            pdev,
> -            "Features: L2:{:#x} Tiler:{:#x} Mem:{:#x} MMU:{:#x} AS:{:#x}",
> -            self.l2_features,
> -            self.tiler_features,
> -            self.mem_features,
> -            self.mmu_features,
> -            self.as_present
> -        );
> -
> -        dev_info!(
> -            pdev,
> -            "shader_present=0x{:016x} l2_present=0x{:016x} tiler_present=0x{:016x}",
> -            self.shader_present,
> -            self.l2_present,
> -            self.tiler_present
> -        );
> -    }
> -
> -    /// Returns the number of virtual address bits supported by the GPU.
> -    #[expect(dead_code)]
> -    pub(crate) fn va_bits(&self) -> u32 {
> -        self.mmu_features & genmask_u32(0..=7)
> -    }
> -
> -    /// Returns the number of physical address bits supported by the GPU.
> -    #[expect(dead_code)]
> -    pub(crate) fn pa_bits(&self) -> u32 {
> -        (self.mmu_features >> 8) & genmask_u32(0..=7)
> -    }
>  }
>  
>  impl Deref for GpuInfo {
> @@ -182,38 +122,68 @@ struct GpuModels {
>      prod_major: 7,
>  }];
>  
> -#[allow(dead_code)]
> -pub(crate) struct GpuId {
> -    pub(crate) arch_major: u32,
> -    pub(crate) arch_minor: u32,
> -    pub(crate) arch_rev: u32,
> -    pub(crate) prod_major: u32,
> -    pub(crate) ver_major: u32,
> -    pub(crate) ver_minor: u32,
> -    pub(crate) ver_status: u32,
> -}
> -
> -impl From<u32> for GpuId {
> -    fn from(value: u32) -> Self {
> -        GpuId {
> -            arch_major: (value & genmask_u32(28..=31)) >> 28,
> -            arch_minor: (value & genmask_u32(24..=27)) >> 24,
> -            arch_rev: (value & genmask_u32(20..=23)) >> 20,
> -            prod_major: (value & genmask_u32(16..=19)) >> 16,
> -            ver_major: (value & genmask_u32(12..=15)) >> 12,
> -            ver_minor: (value & genmask_u32(4..=11)) >> 4,
> -            ver_status: value & genmask_u32(0..=3),
> -        }
> -    }
> +pub(crate) fn gpu_info_log(dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result {
> +    let io = (*iomem).access(dev)?;
> +    let gpu_id = io.read(GPU_ID);
> +
> +    let model_name = if let Some(model) = GPU_MODELS.iter().find(|&f| {
> +        f.arch_major == gpu_id.arch_major().get() && f.prod_major == gpu_id.prod_major().get()
> +    }) {
> +        model.name
> +    } else {
> +        "unknown"
> +    };
> +
> +    dev_info!(
> +        dev,
> +        "mali-{} id 0x{:x} major 0x{:x} minor 0x{:x} status 0x{:x}",
> +        model_name,
> +        gpu_id.into_raw() >> 16,
> +        gpu_id.ver_major().get(),
> +        gpu_id.ver_minor().get(),
> +        gpu_id.ver_status().get()
> +    );
> +
> +    dev_info!(
> +        dev,
> +        "Features: L2:{:#x} Tiler:{:#x} Mem:{:#x} MMU:{:#x} AS:{:#x}",
> +        io.read(L2_FEATURES).into_raw(),
> +        io.read(TILER_FEATURES).into_raw(),
> +        io.read(MEM_FEATURES).into_raw(),
> +        io.read(MMU_FEATURES).into_raw(),
> +        io.read(AS_PRESENT).into_raw(),
> +    );

Without the signature change the old code is all accessing from self.

Best,
Gary

> +
> +    dev_info!(
> +        dev,
> +        "shader_present=0x{:016x} l2_present=0x{:016x} tiler_present=0x{:016x}",
> +        join_u64(
> +            io.read(SHADER_PRESENT_LO).into_raw(),
> +            io.read(SHADER_PRESENT_HI).into_raw(),
> +        ),
> +        join_u64(
> +            io.read(L2_PRESENT_LO).into_raw(),
> +            io.read(L2_PRESENT_HI).into_raw(),
> +        ),
> +        join_u64(
> +            io.read(TILER_PRESENT_LO).into_raw(),
> +            io.read(TILER_PRESENT_HI).into_raw(),
> +        ),
> +    );
> +    Ok(())
>  }
>  
> [snip]