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
