public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/amdgpu: Unify vm_fault_interrupt_state logic for gmc v10 and v12_0
Date: Mon, 25 May 2026 20:48:58 +1000	[thread overview]
Message-ID: <review-patch1-20260520224217.17615-1-ryan.oliveiraf@usp.br> (raw)
In-Reply-To: <20260520224217.17615-1-ryan.oliveiraf@usp.br>

Patch Review

**Correctness: Good.** The extracted function is a faithful copy of the code removed from both files. The `.set` callback pointer swap is correct.

**Issue 1 — Co-developed-by / Signed-off-by ordering (must fix)**

The current tag ordering is:
```
Signed-off-by: Ryan Oliveira <ryan.oliveiraf@usp.br>
Co-developed-by: Kaiky Cintra <kaikycintra@usp.br>
Signed-off-by: Kaiky Cintra <kaikycintra@usp.br>
```

Per `Documentation/process/submitting-patches.rst`, the submitter's (From: author's) `Signed-off-by` must be last, and each `Co-developed-by` must be immediately followed by the corresponding `Signed-off-by`. The correct ordering is:

```
Co-developed-by: Kaiky Cintra <kaikycintra@usp.br>
Signed-off-by: Kaiky Cintra <kaikycintra@usp.br>
Signed-off-by: Ryan Oliveira <ryan.oliveiraf@usp.br>
```

**Issue 2 — Extra blank line (nit)**

There's a double blank line between the new function and `amdgpu_gmc_ras_sw_init`:

```c
	return 0;
}


int amdgpu_gmc_ras_sw_init(struct amdgpu_device *adev)
```

Kernel style uses a single blank line between functions.

**Issue 3 — gmc_v11_0 has a near-identical copy that could also be consolidated (suggestion)**

`gmc_v11_0.c` has an almost identical implementation, differing only in the `DISABLE` case where it adds an extra condition:

```c
// gmc_v11_0 DISABLE case:
if (!adev->in_s0ix && (adev->in_runpm || adev->in_suspend ||
                       amdgpu_in_reset(adev)))
    amdgpu_gmc_set_vm_fault_masks(adev, AMDGPU_GFXHUB(0), false);

// gmc_v10_0 / gmc_v12_0 (and the new shared helper) DISABLE case:
if (!adev->in_s0ix)
    amdgpu_gmc_set_vm_fault_masks(adev, AMDGPU_GFXHUB(0), false);
```

The ENABLE paths are identical across all three. This isn't a blocker for this patch, but the commit message could briefly note why gmc_v11_0 was intentionally excluded (different DISABLE logic) so future readers don't wonder. It's also worth considering whether gmc_v11_0 could be consolidated with a flag or callback parameter, but that's a separate patch.

Note: `gmc_v12_1`, `gmc_v9_0`, and the older versions (v6/v7/v8) all have structurally different implementations (register-level iteration, different hub handling), so they are correctly left untouched.

**Summary:** Correct and useful deduplication. Fix the SoB ordering and the extra blank line, and optionally note why gmc_v11_0 was left out.

---
Generated by Claude Code Patch Reviewer

      parent reply	other threads:[~2026-05-25 10:48 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-05-20 22:42 [PATCH] drm/amdgpu: Unify vm_fault_interrupt_state logic for gmc v10 and v12_0 Ryan Oliveira
2026-05-25 10:48 ` Claude review: " Claude Code Review Bot
2026-05-25 10:48 ` Claude Code Review Bot [this message]

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-20260520224217.17615-1-ryan.oliveiraf@usp.br \
    --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