[PATCH v1] drm/tyr: gpu: fix GpuInfo::log model/version decoding

[PATCH v1] drm/tyr: gpu: fix GpuInfo::log model/version decoding

[Onur Özkan]

GpuInfo::log() was decoding GPU_ID like this:

  major = (self.gpu_id >> 16) & 0xff;
  minor = (self.gpu_id >> 8) & 0xff;
  status = self.gpu_id & 0xff;

That does not match the Mali GPU_ID layout and mixes unrelated
fields. Due to that, model detection becomes `mali-unknown` on
rk3588s which is wrong.

We can already get all the version information with a single
GpuId::from call (less code and cleaner), so this patch uses it.

Also renamed `GpuModels` fields from `major/minor` to
`arch_major/prod_major` to reflect their real meaning.

This change was tested on Orange Pi 5 (rk3588s) board and the
results are as follows:

Before this change:

$ dmesg | grep 'tyr'
[   19.698338] tyr fb000000.gpu: mali-unknown id 0xa867 major 0x67 minor 0x0 status 0x5
[   19.699050] tyr fb000000.gpu: Features: L2:0x7120306 Tiler:0x809 Mem:0x301 MMU:0x2830 AS:0xff
[   19.699817] tyr fb000000.gpu: shader_present=0x0000000000050005 l2_present=0x0000000000000001 tiler_present=0x0000000000000001
[   19.702493] tyr fb000000.gpu: Tyr initialized correctly.

After this change:

$ dmesg | grep 'tyr'
[   19.591692] tyr fb000000.gpu: mali-g610 id 0xa867 major 0x0 minor 0x0 status 0x5
[   19.592374] tyr fb000000.gpu: Features: L2:0x7120306 Tiler:0x809 Mem:0x301 MMU:0x2830 AS:0xff
[   19.593141] tyr fb000000.gpu: shader_present=0x0000000000050005 l2_present=0x0000000000000001 tiler_present=0x0000000000000001
[   19.595831] tyr fb000000.gpu: Tyr initialized correctly.

Signed-off-by: Onur Özkan <work@onurozkan.dev>
---
 drivers/gpu/drm/tyr/gpu.rs | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/tyr/gpu.rs b/drivers/gpu/drm/tyr/gpu.rs
index 6c582910dd5d..da97844efbec 100644
--- a/drivers/gpu/drm/tyr/gpu.rs
+++ b/drivers/gpu/drm/tyr/gpu.rs
@@ -99,13 +99,11 @@ pub(crate) fn new(dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result<Self> {
     }
 
     pub(crate) fn log(&self, pdev: &platform::Device) {
-        let major = (self.gpu_id >> 16) & 0xff;
-        let minor = (self.gpu_id >> 8) & 0xff;
-        let status = self.gpu_id & 0xff;
+        let gpu_id = GpuId::from(self.gpu_id);
 
         let model_name = if let Some(model) = GPU_MODELS
             .iter()
-            .find(|&f| f.major == major && f.minor == minor)
+            .find(|&f| f.arch_major == gpu_id.arch_major && f.prod_major == gpu_id.prod_major)
         {
             model.name
         } else {
@@ -117,9 +115,9 @@ pub(crate) fn log(&self, pdev: &platform::Device) {
             "mali-{} id 0x{:x} major 0x{:x} minor 0x{:x} status 0x{:x}",
             model_name,
             self.gpu_id >> 16,
-            major,
-            minor,
-            status
+            gpu_id.ver_major,
+            gpu_id.ver_minor,
+            gpu_id.ver_status
         );
 
         dev_info!(
@@ -167,14 +165,14 @@ unsafe impl AsBytes for GpuInfo {}
 
 struct GpuModels {
     name: &'static str,
-    major: u32,
-    minor: u32,
+    arch_major: u32,
+    prod_major: u32,
 }
 
 const GPU_MODELS: [GpuModels; 1] = [GpuModels {
     name: "g610",
-    major: 10,
-    minor: 7,
+    arch_major: 10,
+    prod_major: 7,
 }];
 
 #[allow(dead_code)]
-- 
2.51.2

Claude review: drm/tyr: gpu: fix GpuInfo::log model/version decoding

[Claude Code Review Bot]

Overall Series Review

Subject: drm/tyr: gpu: fix GpuInfo::log model/version decoding
Author: =?UTF-8?q?Onur=20=C3=96zkan?= <work@onurozkan.dev>
Patches: 1
Reviewed: 2026-02-11T16:17:51.416707

---

This is a single-patch series that fixes GPU model detection in the DRM Tyr driver (a Rust-based Mali GPU driver). The patch addresses a fundamental bug where GPU_ID register bits were being incorrectly decoded, leading to wrong model identification ("mali-unknown" instead of "mali-g610" on RK3588S).

**Strengths:**
- Fixes a real bug with clear before/after evidence from hardware testing
- Improves code clarity by using existing GpuId::from() helper instead of manual bit manipulation
- Renames struct fields to accurately reflect their semantic meaning (arch_major/prod_major vs major/minor)
- Tested on real hardware (Orange Pi 5 with RK3588S)

**Concerns:**
- The commit message could better explain the Mali GPU_ID register layout
- Missing information about what GpuId::from() actually does and where it's defined
- Unclear if GPU_MODELS array needs expansion for other Mali GPUs
- The debug output still shows "major 0x0 minor 0x0" which might be confusing

---
Generated by Claude Code Patch Reviewer

Claude review: drm/tyr: gpu: fix GpuInfo::log model/version decoding

[Claude Code Review Bot]

Patch Review

#### Commit Message Analysis

The commit message adequately describes the problem and solution, but could be improved:

**Good points:**
- Clearly shows the incorrect bit extraction logic
- Provides concrete before/after test results
- Explains the impact (wrong model detection)

**Missing information:**
- What is the correct Mali GPU_ID register layout? The commit says "does not match the Mali GPU_ID layout" but doesn't explain what the correct layout should be
- Where is GpuId::from() defined and what does it do?
- Are there other Mali GPUs that need entries in GPU_MODELS?

#### Technical Review

**Issue 1: Incorrect register decoding**

```rust
-        let major = (self.gpu_id >> 16) & 0xff;
-        let minor = (self.gpu_id >> 8) & 0xff;
-        let status = self.gpu_id & 0xff;
+        let gpu_id = GpuId::from(self.gpu_id);
```

The old code was extracting:
- Bits [23:16] as "major"
- Bits [15:8] as "minor"  
- Bits [7:0] as "status"

But according to Mali GPU_ID register specification, this appears to be mixing architectural version fields with product version fields. The fix delegates to `GpuId::from()` which presumably does the correct decoding.

**Question:** Where is `GpuId::from()` implementation? This needs to be visible to verify correctness. The patch assumes this function exists and works correctly, but doesn't show its implementation or reference documentation.

**Issue 2: Model matching logic**

```rust
         let model_name = if let Some(model) = GPU_MODELS
             .iter()
-            .find(|&f| f.major == major && f.minor == minor)
+            .find(|&f| f.arch_major == gpu_id.arch_major && f.prod_major == gpu_id.prod_major)
```

The old code was comparing:
- `major` (bits [23:16] of GPU_ID) with model.major (10)
- `minor` (bits [15:8] of GPU_ID) with model.minor (7)

For GPU_ID = 0xa867:
- Old major = 0x67 = 103
- Old minor = 0x0 = 0
- These didn't match (10, 7), hence "mali-unknown"

The new code presumably extracts the architecture and product major versions correctly from GpuId::from(), which should return arch_major=10 and prod_major=7 for a Mali-G610.

**Concern:** Without seeing the GpuId structure definition, I cannot verify this is correct. What are the actual bit positions for arch_major and prod_major in the Mali GPU_ID register?

**Issue 3: Debug output change**

```rust
             "mali-{} id 0x{:x} major 0x{:x} minor 0x{:x} status 0x{:x}",
             model_name,
             self.gpu_id >> 16,
-            major,
-            minor,
-            status
+            gpu_id.ver_major,
+            gpu_id.ver_minor,
+            gpu_id.ver_status
```

The test output shows:
```
Before: mali-unknown id 0xa867 major 0x67 minor 0x0 status 0x5
After:  mali-g610 id 0xa867 major 0x0 minor 0x0 status 0x5
```

**Problem:** The "after" output shows "major 0x0 minor 0x0" which seems odd. Are ver_major and ver_minor really both 0? This might be correct if the Mali-G610 GPU_ID register has version fields set to 0, but it looks suspicious and could confuse users.

**Question:** What do ver_major and ver_minor actually represent? Are they revision/stepping numbers that happen to be 0 on this chip?

**Issue 4: Field renaming**

```rust
 struct GpuModels {
     name: &'static str,
-    major: u32,
-    minor: u32,
+    arch_major: u32,
+    prod_major: u32,
 }
```

This is a good improvement for clarity. The new names better reflect that these are architecture and product identifiers, not version numbers.

**Issue 5: GPU_MODELS constant values**

```rust
 const GPU_MODELS: [GpuModels; 1] = [GpuModels {
     name: "g610",
-    major: 10,
-    minor: 7,
+    arch_major: 10,
+    prod_major: 7,
 }];
```

The values (10, 7) remain unchanged, which makes sense - they were the correct target values, just being compared against the wrong extracted fields.

**Concern:** The GPU_MODELS array only has one entry (Mali-G610). Are other Mali GPUs supported by this driver? Do they need entries? The array size annotation `[GpuModels; 1]` suggests this driver only supports G610, but this should be clarified.

#### Missing Context

To properly review this patch, I would need to see:

1. **GpuId struct definition** - What fields does it have? What are their types?
2. **GpuId::from() implementation** - How does it extract arch_major, prod_major, ver_major, ver_minor, ver_status from the raw GPU_ID register?
3. **Mali GPU_ID register specification** - What is the actual bit layout according to ARM documentation?
4. **Driver scope** - Does this driver support other Mali GPUs beyond G610?

#### Testing Evidence

The testing is solid:
- Real hardware (Orange Pi 5 with RK3588S)
- Clear before/after dmesg output
- Correct model detection (g610 vs unknown)

However:
- Only tested on one GPU model (G610)
- No testing on other boards/GPUs if they exist
- No verification that the version fields (0x0, 0x0, 0x5) are actually correct

#### Code Quality

**Positive:**
- Reduces code by using existing helper function
- More semantic field names
- Cleaner, more maintainable

**Negative:**
- Adds dependency on GpuId type without showing it in the patch
- The ver_major/ver_minor output being 0x0 needs explanation

#### Potential Issues

1. **ABI compatibility:** Does the changed debug output format affect any userspace parsing? Probably not for kernel debug messages, but worth considering.

2. **Completeness:** If other Mali GPUs exist in the wild, they'll still show "mali-unknown". Is this acceptable?

3. **Documentation:** The code would benefit from comments explaining the GPU_ID register layout, especially since the previous developer clearly misunderstood it.

#### Recommendations

1. **Add a comment** explaining the Mali GPU_ID register bit layout above the GpuId::from() call
2. **Verify GpuId::from()** - ensure its implementation matches ARM's Mali GPU_ID specification
3. **Consider adding a comment** in GPU_MODELS explaining how to find arch_major/prod_major values for new GPU models
4. **Explain in commit message** why ver_major and ver_minor are 0x0 (if that's expected)
5. **Consider testing** on other Mali GPUs if this driver supports them

#### Verdict

**The fix appears correct** based on the evidence provided (correct model detection on real hardware), but the review is limited without seeing:
- GpuId struct definition and its From implementation
- Mali GPU_ID register documentation reference

**Recommendation:** Request the submitter to:
1. Either show the GpuId implementation in the patch or reference where it's defined
2. Add a comment explaining the GPU_ID register layout
3. Clarify if ver_major=0x0, ver_minor=0x0 is expected for this GPU

If GpuId::from() correctly implements the Mali GPU_ID register decoding according to ARM specifications, then:

**Reviewed-by: [conditional on verifying GpuId implementation]**

The core fix (using GpuId::from() and matching on arch_major/prod_major) is the right approach and solves the real bug. The field renaming improves code clarity.

---
Generated by Claude Code Patch Reviewer

Re: [PATCH v1] drm/tyr: gpu: fix GpuInfo::log model/version decoding

[孙科]

Verified on my NanoPi R6C; the GPU model is now correctly detected and
printed as follows:

root@nanopi-r6s:~# dmesg | grep tyr
[    1.894322] tyr fb000000.gpu: supply sram not found, using dummy regulator
[    1.894527] tyr fb000000.gpu: mali-g610 id 0xa867 major 0x0 minor
0x0 status 0x5
[    1.894534] tyr fb000000.gpu: Features: L2:0x7120306 Tiler:0x809
Mem:0x301 MMU:0x2830 AS:0xff
[    1.894539] tyr fb000000.gpu: shader_present=0x0000000000050005
l2_present=0x0000000000000001 tiler_present=0x0000000000000001
[    1.895170] tyr fb000000.gpu: Tyr initialized correctly.

Tested-by: Alvin Sun <sk.alvin.x@gmail.com>

Best regards,
Alvin

On Wed, Feb 11, 2026 at 2:45 AM Onur Özkan <work@onurozkan.dev> wrote:
>
> GpuInfo::log() was decoding GPU_ID like this:
>
>   major = (self.gpu_id >> 16) & 0xff;
>   minor = (self.gpu_id >> 8) & 0xff;
>   status = self.gpu_id & 0xff;
>
> That does not match the Mali GPU_ID layout and mixes unrelated
> fields. Due to that, model detection becomes `mali-unknown` on
> rk3588s which is wrong.
>
> We can already get all the version information with a single
> GpuId::from call (less code and cleaner), so this patch uses it.
>
> Also renamed `GpuModels` fields from `major/minor` to
> `arch_major/prod_major` to reflect their real meaning.
>
> This change was tested on Orange Pi 5 (rk3588s) board and the
> results are as follows:
>
> Before this change:
>
> $ dmesg | grep 'tyr'
> [   19.698338] tyr fb000000.gpu: mali-unknown id 0xa867 major 0x67 minor 0x0 status 0x5
> [   19.699050] tyr fb000000.gpu: Features: L2:0x7120306 Tiler:0x809 Mem:0x301 MMU:0x2830 AS:0xff
> [   19.699817] tyr fb000000.gpu: shader_present=0x0000000000050005 l2_present=0x0000000000000001 tiler_present=0x0000000000000001
> [   19.702493] tyr fb000000.gpu: Tyr initialized correctly.
>
> After this change:
>
> $ dmesg | grep 'tyr'
> [   19.591692] tyr fb000000.gpu: mali-g610 id 0xa867 major 0x0 minor 0x0 status 0x5
> [   19.592374] tyr fb000000.gpu: Features: L2:0x7120306 Tiler:0x809 Mem:0x301 MMU:0x2830 AS:0xff
> [   19.593141] tyr fb000000.gpu: shader_present=0x0000000000050005 l2_present=0x0000000000000001 tiler_present=0x0000000000000001
> [   19.595831] tyr fb000000.gpu: Tyr initialized correctly.
>
> Signed-off-by: Onur Özkan <work@onurozkan.dev>
> ---
>  drivers/gpu/drm/tyr/gpu.rs | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/tyr/gpu.rs b/drivers/gpu/drm/tyr/gpu.rs
> index 6c582910dd5d..da97844efbec 100644
> --- a/drivers/gpu/drm/tyr/gpu.rs
> +++ b/drivers/gpu/drm/tyr/gpu.rs
> @@ -99,13 +99,11 @@ pub(crate) fn new(dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result<Self> {
>      }
>
>      pub(crate) fn log(&self, pdev: &platform::Device) {
> -        let major = (self.gpu_id >> 16) & 0xff;
> -        let minor = (self.gpu_id >> 8) & 0xff;
> -        let status = self.gpu_id & 0xff;
> +        let gpu_id = GpuId::from(self.gpu_id);
>
>          let model_name = if let Some(model) = GPU_MODELS
>              .iter()
> -            .find(|&f| f.major == major && f.minor == minor)
> +            .find(|&f| f.arch_major == gpu_id.arch_major && f.prod_major == gpu_id.prod_major)
>          {
>              model.name
>          } else {
> @@ -117,9 +115,9 @@ pub(crate) fn log(&self, pdev: &platform::Device) {
>              "mali-{} id 0x{:x} major 0x{:x} minor 0x{:x} status 0x{:x}",
>              model_name,
>              self.gpu_id >> 16,
> -            major,
> -            minor,
> -            status
> +            gpu_id.ver_major,
> +            gpu_id.ver_minor,
> +            gpu_id.ver_status
>          );
>
>          dev_info!(
> @@ -167,14 +165,14 @@ unsafe impl AsBytes for GpuInfo {}
>
>  struct GpuModels {
>      name: &'static str,
> -    major: u32,
> -    minor: u32,
> +    arch_major: u32,
> +    prod_major: u32,
>  }
>
>  const GPU_MODELS: [GpuModels; 1] = [GpuModels {
>      name: "g610",
> -    major: 10,
> -    minor: 7,
> +    arch_major: 10,
> +    prod_major: 7,
>  }];
>
>  #[allow(dead_code)]
> --
> 2.51.2
>
>

Re: [PATCH v1] drm/tyr: gpu: fix GpuInfo::log model/version decoding

[Boris Brezillon]

On Tue, 10 Feb 2026 21:38:12 +0300
Onur Özkan <work@onurozkan.dev> wrote:

> GpuInfo::log() was decoding GPU_ID like this:
> 
>   major = (self.gpu_id >> 16) & 0xff;
>   minor = (self.gpu_id >> 8) & 0xff;
>   status = self.gpu_id & 0xff;
> 
> That does not match the Mali GPU_ID layout and mixes unrelated
> fields. Due to that, model detection becomes `mali-unknown` on
> rk3588s which is wrong.
> 
> We can already get all the version information with a single
> GpuId::from call (less code and cleaner), so this patch uses it.
> 
> Also renamed `GpuModels` fields from `major/minor` to
> `arch_major/prod_major` to reflect their real meaning.
> 
> This change was tested on Orange Pi 5 (rk3588s) board and the
> results are as follows:
> 
> Before this change:
> 
> $ dmesg | grep 'tyr'
> [   19.698338] tyr fb000000.gpu: mali-unknown id 0xa867 major 0x67 minor 0x0 status 0x5
> [   19.699050] tyr fb000000.gpu: Features: L2:0x7120306 Tiler:0x809 Mem:0x301 MMU:0x2830 AS:0xff
> [   19.699817] tyr fb000000.gpu: shader_present=0x0000000000050005 l2_present=0x0000000000000001 tiler_present=0x0000000000000001
> [   19.702493] tyr fb000000.gpu: Tyr initialized correctly.
> 
> After this change:
> 
> $ dmesg | grep 'tyr'
> [   19.591692] tyr fb000000.gpu: mali-g610 id 0xa867 major 0x0 minor 0x0 status 0x5
> [   19.592374] tyr fb000000.gpu: Features: L2:0x7120306 Tiler:0x809 Mem:0x301 MMU:0x2830 AS:0xff
> [   19.593141] tyr fb000000.gpu: shader_present=0x0000000000050005 l2_present=0x0000000000000001 tiler_present=0x0000000000000001
> [   19.595831] tyr fb000000.gpu: Tyr initialized correctly.
> 
> Signed-off-by: Onur Özkan <work@onurozkan.dev>

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> ---
>  drivers/gpu/drm/tyr/gpu.rs | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tyr/gpu.rs b/drivers/gpu/drm/tyr/gpu.rs
> index 6c582910dd5d..da97844efbec 100644
> --- a/drivers/gpu/drm/tyr/gpu.rs
> +++ b/drivers/gpu/drm/tyr/gpu.rs
> @@ -99,13 +99,11 @@ pub(crate) fn new(dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result<Self> {
>      }
>  
>      pub(crate) fn log(&self, pdev: &platform::Device) {
> -        let major = (self.gpu_id >> 16) & 0xff;
> -        let minor = (self.gpu_id >> 8) & 0xff;
> -        let status = self.gpu_id & 0xff;
> +        let gpu_id = GpuId::from(self.gpu_id);
>  
>          let model_name = if let Some(model) = GPU_MODELS
>              .iter()
> -            .find(|&f| f.major == major && f.minor == minor)
> +            .find(|&f| f.arch_major == gpu_id.arch_major && f.prod_major == gpu_id.prod_major)
>          {
>              model.name
>          } else {
> @@ -117,9 +115,9 @@ pub(crate) fn log(&self, pdev: &platform::Device) {
>              "mali-{} id 0x{:x} major 0x{:x} minor 0x{:x} status 0x{:x}",
>              model_name,
>              self.gpu_id >> 16,
> -            major,
> -            minor,
> -            status
> +            gpu_id.ver_major,
> +            gpu_id.ver_minor,
> +            gpu_id.ver_status
>          );
>  
>          dev_info!(
> @@ -167,14 +165,14 @@ unsafe impl AsBytes for GpuInfo {}
>  
>  struct GpuModels {
>      name: &'static str,
> -    major: u32,
> -    minor: u32,
> +    arch_major: u32,
> +    prod_major: u32,
>  }
>  
>  const GPU_MODELS: [GpuModels; 1] = [GpuModels {
>      name: "g610",
> -    major: 10,
> -    minor: 7,
> +    arch_major: 10,
> +    prod_major: 7,
>  }];
>  
>  #[allow(dead_code)]