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: 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	[thread overview]
Message-ID: <review-patch1-20260510190505.4810-2-gilles.risch@gmail.com> (raw)
In-Reply-To: <20260510190505.4810-2-gilles.risch@gmail.com>

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

  parent reply	other threads:[~2026-05-16  5:30 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-05-10 19:05 [PATCH] 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 Gilles Risch
2026-05-16  5:30 ` Claude review: " Claude Code Review Bot
2026-05-16  5:30 ` Claude Code Review Bot [this message]
  -- strict thread matches above, loose matches on Subject: below --
2026-05-10 18:54 [PATCH] " Gilles Risch
2026-05-16  5:26 ` Claude review: " Claude Code Review Bot
2026-05-16  5:26 ` 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-20260510190505.4810-2-gilles.risch@gmail.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