From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: The Apple iMac11, 1 (late 2009) has an integrated ATI Mobility Radeon HD 4850. This machine suffers from a similar problem as the iMac10, 1 (late 2009) and the iMac11, 2 (mid 2010). This small patch fixes the issue on this machine.
Date: Sat, 16 May 2026 15:30:58 +1000
Message-ID: <review-patch1-20260510190505.4810-2-gilles.risch@gmail.com>
In-Reply-To: <20260510190505.4810-2-gilles.risch@gmail.com>
References: <20260510190505.4810-2-gilles.risch@gmail.com> <20260510190505.4810-2-gilles.risch@gmail.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

**Subject line (major issue):**
The subject line contains the entire patch description rather than a concise summary. It should follow the kernel convention:
```
drm/radeon: Fix dark eDP panel on Apple iMac11,1
```
The current multi-line subject wraps badly in `git log` and mailing list archives. The explanation belongs in the commit message body.

**Commit message body (needs work):**
The body only says:
```
Fixes freedesktop issue 164
Link: https://gitlab.freedesktop.org/xorg/driver/xf86-video-ati/-/issues/164
```
It should explain:
- What the technical root cause is (RV770 excluded from `ASIC_IS_DCE32` due to enum ordering)
- Why the PLL and encoder changes are correct for RV770
- What testing was performed
- The `Link:` tag should use the kernel format with the kernel bugzilla or `Closes:` tag for the gitlab issue

**New macro definition (`radeon.h`):**
```c
#define ASIC_IS_DCE31(rdev) ((rdev->family >= CHIP_RV770))
#define ASIC_IS_DCE32(rdev) ((rdev->family >= CHIP_RV730))
```

The chip family enum has `CHIP_RV770 = 35` before `CHIP_RV730 = 36`, so `ASIC_IS_DCE32` currently **excludes** RV770. The new `ASIC_IS_DCE31` adds only RV770 to the set.

However, RV770 is generally understood to be a DCE 3.2 part (same display engine generation as RV730/RV710/RV740). The existing `ASIC_IS_DCE32` appears to have a longstanding bug excluding it. An alternative approach worth considering: fix `ASIC_IS_DCE32` itself to use `>= CHIP_RV770`. That would also fix the remaining `ASIC_IS_DCE32` call sites in `r600.c` (interrupt handling, clock setup) and `radeon_audio.c` that likely also incorrectly exclude RV770. If there's a reason RV770 genuinely has a different DCE revision, the commit message needs to explain that.

**PLL changes (`atombios_crtc.c`) -- regression risk:**
```c
if (ASIC_IS_DCE31(rdev) && mode->clock > 200000)  /* range limits??? */
    radeon_crtc->pll_flags |= RADEON_PLL_PREFER_HIGH_FB_DIV;
```
and:
```c
if (ASIC_IS_DCE31(rdev) && mode->clock > 165000)
    radeon_crtc->pll_flags |= RADEON_PLL_USE_FRAC_FB_DIV;
```
These changes affect **all** RV770 systems (Radeon HD 4850/4870 cards in regular PCs), not just Apple iMacs. Previously, RV770 would take the `else` branch at line 585 and get `RADEON_PLL_PREFER_LOW_REF_DIV`, and would not get `RADEON_PLL_USE_FRAC_FB_DIV` for high clocks. The commit message does not acknowledge this broader behavioral change. While RV770 and RV730 share the same PLL hardware, this still warrants explicit justification and testing on non-Apple RV770 systems.

Note that line 588 already uses `CHIP_RV770` as a direct boundary:
```c
if (rdev->family < CHIP_RV770)
    radeon_crtc->pll_flags |= RADEON_PLL_PREFER_MINM_OVER_MAXP;
```
This is consistent with the change direction, but it also suggests that the original `ASIC_IS_DCE32` at this call site was always wrong for RV770.

**Encoder selection change (`atombios_encoders.c`) -- correct but impacts all RV770:**
```c
if (ASIC_IS_DCE31(rdev)) {
    if (dmi_match(DMI_PRODUCT_NAME, "iMac10,1") ||
        dmi_match(DMI_PRODUCT_NAME, "iMac11,1") ||
        dmi_match(DMI_PRODUCT_NAME, "iMac11,2"))
        enc_idx = (dig->linkb) ? 1 : 0;
    else
        enc_idx = radeon_crtc->crtc_id;
    goto assigned;
}
```
Adding iMac11,1 to the DMI match list is straightforward and correct. However, all RV770 systems now take this `ASIC_IS_DCE31` path (`enc_idx = radeon_crtc->crtc_id`) instead of falling through to the DCE3 path (lines 2141-2165), which has different encoder-conflict resolution logic. The comment says "any encoder can drive any block" for these chips, which is likely correct for RV770, but this behavioral change should be documented.

**Summary of requested changes:**
1. Fix the subject line to follow `drm/radeon: <concise summary>` format
2. Write a proper commit message explaining the root cause, what changed, and the broader impact on RV770 systems
3. Either justify why `ASIC_IS_DCE31` is the correct version name for RV770 (not DCE 3.2), or consider fixing `ASIC_IS_DCE32` directly
4. Acknowledge and justify the PLL/encoder behavioral changes for all RV770 systems, not just Apple iMacs

---
Generated by Claude Code Patch Reviewer