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

[Deborah Brouwer <deborah.brouwer@collabora.com>]

On Fri, Mar 13, 2026 at 03:29:03PM -0300, Daniel Almeida wrote:
> 
> >> @@ -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.
> 
> IIRC, Onur recently cleaned this up, hasn’t he?

Onur's patch fixes the model name detection and it's in drm-rust-next:
289cf6f91459 drm/tyr: gpu: fix GpuInfo::log model/version decoding

But it doesn't touch the generation of the id. So, in v3 I'll add a
separate commit to print the GPU_ID without filtering.

> 
> > 
> >> +        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;
> >        }
> > 
> 
> +1 to the syntax above. This looks quite ergonomic IMHO.
> 
> > 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;
> >> +        }
> >> +    }
> 
> — Daniel