Re: [PATCH v2 1/5] drm/tyr: Use register! macro for GPU_CONTROL

[Boris Brezillon <boris.brezillon@collabora.com>]

On Wed, 11 Mar 2026 16:03:58 -0700
Deborah Brouwer <deborah.brouwer@collabora.com> wrote:

Just a couple drive-by comments. Didn't go through all the register
definitions to make sure they are correct, but I'm pretty happy with
the transition to the register!() macro overall (maybe at some point we
can even automate the generation through some script...)

> @@ -78,11 +84,17 @@ 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_val(
> +        GPU_COMMAND_RESET::zeroed().with_const_reset_mode::<{ GPU_COMMAND_RESET::SOFT_RESET }>(),

I don't see a .with_[const_]command() here, is there a trick I'm
missing, or are you relying on the fact GPU_COMMAND::RESET is zero
(which works, but is quite confusing). On a side note, it be good if we
could have a mode where fields get auto-initialized for such alias
(more on this below).

> +    );
>  
>      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().get() != 0,
>          time::Delta::from_millis(1),
>          time::Delta::from_millis(100),
>      )

[...]

> @@ -182,37 +105,67 @@ 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"
> +    };
> +
> +    // Create canonical product ID with only arch/product fields, excluding version
> +    // fields. This ensures the same product at different revisions has the same ID.
> +    let id = GPU_ID::zeroed()
> +        .with_arch_major(gpu_id.arch_major())
> +        .with_arch_minor(gpu_id.arch_minor())
> +        .with_arch_rev(gpu_id.arch_rev())
> +        .with_prod_major(gpu_id.prod_major())
> +        .into_raw();
> +
> +    dev_info!(
> +        dev,
> +        "mali-{} id 0x{:x} major 0x{:x} minor 0x{:x} status 0x{:x}",
> +        model_name,
> +        id,

This was previously right-shifted by 16. Now, I'm questioning this
decision to filter out the version fields. I think it'd be better to
print the raw ID directly. We're already extracting and printing the
arch major.minor and version status, but if there's anything else
we want to clearly extract from this raw ID, we could add more.

TLDR; that's probably one place where I think it's not such a bad idea
to diverge from Panthor and print an unmodified GPU_ID. Maybe s/id/raw
GPU ID/ to make the distincting clear. None of this should happen in
this commit though. Either we do that in a preliminary commit that
drops the `>> 16`, or we keep the `>> 16` here, and change that in a
follow-up.

> +        gpu_id.ver_major().get(),
> +        gpu_id.ver_minor().get(),
> +        gpu_id.ver_status().get()
> +    );

[...]

> +
> +    impl GPU_COMMAND {
> +        /// No operation. This is the default value.
> +        pub(crate) const NOP: u32 = 0;
> +        /// Reset the GPU.
> +        pub(crate) const RESET: u32 = 1;
> +        /// Flush caches.
> +        pub(crate) const FLUSH_CACHES: u32 = 4;
> +        /// Clear GPU faults.
> +        pub(crate) const CLEAR_FAULT: u32 = 7;
> +    }
> +
> +    register! {
> +        /// GPU command register in reset mode.
> +        /// Set command to [`GPU_COMMAND::RESET`] to set reset_mode.
> +        pub(crate) GPU_COMMAND_RESET (u32) => GPU_COMMAND {
> +            7:0     command;

Alexandre, dunno how hard it would be to extend this alias syntax to
provide auto-initialization/expected-value of certain fields, like:

	pub(crate) GPU_COMMAND_RESET (u32) => GPU_COMMAND {
            7:0     command <- GPU_COMMAND::RESET;
            11:8    reset_mode;
        }

so that when you instantiate a GPU_COMMAND_RESET, all you have to set is
reset_mode and the command gets set to GPU_COMMAND::RESET for you.
(that's for the write path, for the read path, you'll need some sort of
match to do the re-interpretation anyway).

Just to be clear, I'm not asking for any of that in the current
register!() patchset. It's more a suggestion for a potential future
improvement.

> +            11:8    reset_mode;
> +        }
> +    }