From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/tyr: Use register! macro for GPU_CONTROL
Date: Sun, 12 Apr 2026 10:45:08 +1000 [thread overview]
Message-ID: <review-patch1-20260409-b4-tyr-use-register-macro-v5-v5-1-8abfff8a0204@collabora.com> (raw)
In-Reply-To: <20260409-b4-tyr-use-register-macro-v5-v5-1-8abfff8a0204@collabora.com>
Patch Review
This is the largest and most important patch. It defines the entire GPU_CONTROL register page with the `register!` macro, converts all users (`issue_soft_reset`, `l2_power_on`, `GpuInfo::new`, `GpuInfo::log`), adds helper types, and removes the old `GpuId` struct.
**Register offset fix (silent bug fix):**
The old code had:
```rust
pub(crate) const L2_PWRTRANS_HI: Register<0x204> = Register;
```
The new code correctly defines:
```rust
pub(crate) L2_PWRTRANS_HI(u32) @ 0x224 {
```
Offset 0x204 is actually `SHADER_PWRTRANS_HI` (confirmed by panthor's `SHADER_PWRTRANS` at 0x200). This was a copy-paste bug in the original tyr code. This fix should be explicitly mentioned in the commit message rather than being hidden in a large refactor.
**`read_u64_no_tearing` helper:**
```rust
pub(crate) fn read_u64_no_tearing(lo_read: impl Fn() -> u32, hi_read: impl Fn() -> u32) -> u64 {
loop {
let hi1 = hi_read();
let lo = lo_read();
let hi2 = hi_read();
if hi1 == hi2 {
return join_u64(lo, hi1);
}
}
}
```
This is a standard pattern for tear-free 64-bit reads from split registers. The potential infinite loop is fine in practice -- the high half of hardware counters doesn't change faster than software can read it. However, this function is not used anywhere in the series. It's harmless under `#![allow(dead_code)]`, but could be deferred to the patch that actually needs it.
**GPU_COMMAND aliasing pattern:**
```rust
GPU_COMMAND (u32) @ 0x30 {
7:0 command ?=> GpuCommand;
}
GPU_COMMAND_RESET (u32) => GPU_COMMAND {
7:0 command ?=> GpuCommand;
11:8 reset_mode ?=> ResetMode;
}
GPU_COMMAND_FLUSH (u32) => GPU_COMMAND {
7:0 command ?=> GpuCommand;
11:8 l2_flush ?=> FlushMode;
15:12 lsc_flush ?=> FlushMode;
19:16 other_flush ?=> FlushMode;
}
```
This is a clever approach to handle the GPU_COMMAND register's multiple layouts. The aliases share the same MMIO offset via `=> GPU_COMMAND` and the constructor methods on `GPU_COMMAND` (`reset()`, `flush_caches()`, `clear_fault()`) provide a clean API. Well designed.
**Texture features -- fixing a TODO:**
```rust
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(),
],
```
The old code only read `TEXTURE_FEATURES0` and hardcoded `[texture_features, 0, 0, 0]` with a `// TODO: Add texture_features_{1,2,3}` comment. Now all four are read from hardware. Good improvement.
**Minor nit -- comment formatting:**
```rust
//GPU_FEATURES register is not available; it was introduced in arch 11.x.
```
Missing space after `//`. Should be `// GPU_FEATURES register...`.
**Signature change of `GpuInfo::log`:**
```rust
- pub(crate) fn log(&self, pdev: &platform::Device) {
+ pub(crate) fn log(&self, dev: &Device<Bound>) {
```
And the caller is updated:
```rust
- gpu_info.log(pdev);
+ gpu_info.log(pdev.as_ref());
```
This is a reasonable change since `log()` only needs `dev_info!` which works with `Device<Bound>`.
**Removed `va_bits()` and `pa_bits()` methods:**
These were marked `#[expect(dead_code)]` and are now redundant since `MMU_FEATURES` has typed `va_bits` and `pa_bits` fields. Clean removal.
---
Generated by Claude Code Patch Reviewer
next prev parent reply other threads:[~2026-04-12 0:45 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-04-09 17:51 [PATCH v5 0/6] drm/tyr: Use register! macro Deborah Brouwer
2026-04-09 17:51 ` [PATCH v5 1/6] drm/tyr: Use register! macro for GPU_CONTROL Deborah Brouwer
2026-04-12 0:45 ` Claude Code Review Bot [this message]
2026-04-09 17:51 ` [PATCH v5 2/6] drm/tyr: Print GPU_ID without filtering Deborah Brouwer
2026-04-12 0:45 ` Claude review: " Claude Code Review Bot
2026-04-09 17:51 ` [PATCH v5 3/6] drm/tyr: Use register! macro for JOB_CONTROL Deborah Brouwer
2026-04-12 0:45 ` Claude review: " Claude Code Review Bot
2026-04-09 17:51 ` [PATCH v5 4/6] drm/tyr: Use register! macro for MMU_CONTROL Deborah Brouwer
2026-04-12 0:45 ` Claude review: " Claude Code Review Bot
2026-04-09 17:51 ` [PATCH v5 5/6] drm/tyr: Remove custom register struct Deborah Brouwer
2026-04-12 0:45 ` Claude review: " Claude Code Review Bot
2026-04-09 17:51 ` [PATCH v5 6/6] drm/tyr: Add DOORBELL_BLOCK registers Deborah Brouwer
2026-04-12 0:45 ` Claude review: " Claude Code Review Bot
2026-04-12 0:45 ` Claude review: drm/tyr: Use register! macro Claude Code Review Bot
-- strict thread matches above, loose matches on Subject: below --
2026-03-24 0:18 [PATCH v3 00/12] " Deborah Brouwer
2026-03-24 0:18 ` [PATCH v3 01/12] drm/tyr: Use register! macro for GPU_CONTROL Deborah Brouwer
2026-03-24 21:08 ` Claude review: " Claude Code Review Bot
2026-03-11 23:03 [PATCH v2 0/5] drm/tyr: Use register! macro Deborah Brouwer
2026-03-11 23:03 ` [PATCH v2 1/5] drm/tyr: Use register! macro for GPU_CONTROL Deborah Brouwer
2026-03-13 4:38 ` Claude review: " Claude Code Review Bot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=review-patch1-20260409-b4-tyr-use-register-macro-v5-v5-1-8abfff8a0204@collabora.com \
--to=claude-review@example.com \
--cc=dri-devel-reviews@example.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox