public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [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.
@ 2026-05-10 18:54 Gilles Risch
  0 siblings, 0 replies; 4+ messages in thread
From: Gilles Risch @ 2026-05-10 18:54 UTC (permalink / raw)
  To: alexander.deucher; +Cc: dri-devel, amd-gfx, Gilles Risch

Fixes freedesktop issue 164
Link: https://gitlab.freedesktop.org/xorg/driver/xf86-video-ati/-/issues/164
---
 drivers/gpu/drm/radeon/atombios_crtc.c     | 4 ++--
 drivers/gpu/drm/radeon/atombios_encoders.c | 9 +++++----
 drivers/gpu/drm/radeon/radeon.h            | 1 +
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c b/drivers/gpu/drm/radeon/atombios_crtc.c
index 2fc0334e0..3c6d33273 100644
--- a/drivers/gpu/drm/radeon/atombios_crtc.c
+++ b/drivers/gpu/drm/radeon/atombios_crtc.c
@@ -580,7 +580,7 @@ static u32 atombios_adjust_pll(struct drm_crtc *crtc,
 			radeon_crtc->pll_flags |= (/*RADEON_PLL_USE_FRAC_FB_DIV |*/
 				RADEON_PLL_PREFER_CLOSEST_LOWER);
 
-		if (ASIC_IS_DCE32(rdev) && mode->clock > 200000)	/* range limits??? */
+		if (ASIC_IS_DCE31(rdev) && mode->clock > 200000)	/* range limits??? */
 			radeon_crtc->pll_flags |= RADEON_PLL_PREFER_HIGH_FB_DIV;
 		else
 			radeon_crtc->pll_flags |= RADEON_PLL_PREFER_LOW_REF_DIV;
@@ -594,7 +594,7 @@ static u32 atombios_adjust_pll(struct drm_crtc *crtc,
 		if (((rdev->family == CHIP_RS780) || (rdev->family == CHIP_RS880))
 		    && !radeon_crtc->ss_enabled)
 			radeon_crtc->pll_flags |= RADEON_PLL_USE_FRAC_FB_DIV;
-		if (ASIC_IS_DCE32(rdev) && mode->clock > 165000)
+		if (ASIC_IS_DCE31(rdev) && mode->clock > 165000)
 			radeon_crtc->pll_flags |= RADEON_PLL_USE_FRAC_FB_DIV;
 	} else {
 		radeon_crtc->pll_flags |= RADEON_PLL_LEGACY;
diff --git a/drivers/gpu/drm/radeon/atombios_encoders.c b/drivers/gpu/drm/radeon/atombios_encoders.c
index 5cfd8fcfa..4e984973c 100644
--- a/drivers/gpu/drm/radeon/atombios_encoders.c
+++ b/drivers/gpu/drm/radeon/atombios_encoders.c
@@ -2123,12 +2123,13 @@ int radeon_atom_pick_dig_encoder(struct drm_encoder *encoder, int fe_idx)
 	}
 
 	/*
-	 * On DCE32 any encoder can drive any block so usually just use crtc id,
-	 * but Apple thinks different at least on iMac10,1 and iMac11,2, so there use linkb,
-	 * otherwise the internal eDP panel will stay dark.
+	 * On DCE31 and DCE32 any encoder can drive any block so usually just use crtc id,
+	 * but Apple thinks different at least on iMac10,1, iMac11,1 and iMac11,2,
+	 * so there use linkb, otherwise the internal eDP panel will stay dark.
 	 */
-	if (ASIC_IS_DCE32(rdev)) {
+	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
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index 527b9d19d..6b7c0abe4 100644
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -2625,6 +2625,7 @@ void r100_pll_errata_after_index(struct radeon_device *rdev);
 			    (rdev->family == CHIP_RS740)  ||	\
 			    (rdev->family >= CHIP_R600))
 #define ASIC_IS_DCE3(rdev) ((rdev->family >= CHIP_RV620))
+#define ASIC_IS_DCE31(rdev) ((rdev->family >= CHIP_RV770))
 #define ASIC_IS_DCE32(rdev) ((rdev->family >= CHIP_RV730))
 #define ASIC_IS_DCE4(rdev) ((rdev->family >= CHIP_CEDAR))
 #define ASIC_IS_DCE41(rdev) ((rdev->family >= CHIP_PALM) && \
-- 
2.47.3


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [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.
@ 2026-05-10 19:05 Gilles Risch
  2026-05-16  5:30 ` Claude review: " Claude Code Review Bot
  2026-05-16  5:30 ` Claude Code Review Bot
  0 siblings, 2 replies; 4+ messages in thread
From: Gilles Risch @ 2026-05-10 19:05 UTC (permalink / raw)
  To: alexander.deucher; +Cc: dri-devel, amd-gfx, Gilles Risch

Fixes freedesktop issue 164
Link: https://gitlab.freedesktop.org/xorg/driver/xf86-video-ati/-/issues/164

Signed-off-by: Gilles Risch <gilles.risch@gmail.com>
---
 drivers/gpu/drm/radeon/atombios_crtc.c     | 4 ++--
 drivers/gpu/drm/radeon/atombios_encoders.c | 9 +++++----
 drivers/gpu/drm/radeon/radeon.h            | 1 +
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c b/drivers/gpu/drm/radeon/atombios_crtc.c
index 2fc0334e0..3c6d33273 100644
--- a/drivers/gpu/drm/radeon/atombios_crtc.c
+++ b/drivers/gpu/drm/radeon/atombios_crtc.c
@@ -580,7 +580,7 @@ static u32 atombios_adjust_pll(struct drm_crtc *crtc,
 			radeon_crtc->pll_flags |= (/*RADEON_PLL_USE_FRAC_FB_DIV |*/
 				RADEON_PLL_PREFER_CLOSEST_LOWER);
 
-		if (ASIC_IS_DCE32(rdev) && mode->clock > 200000)	/* range limits??? */
+		if (ASIC_IS_DCE31(rdev) && mode->clock > 200000)	/* range limits??? */
 			radeon_crtc->pll_flags |= RADEON_PLL_PREFER_HIGH_FB_DIV;
 		else
 			radeon_crtc->pll_flags |= RADEON_PLL_PREFER_LOW_REF_DIV;
@@ -594,7 +594,7 @@ static u32 atombios_adjust_pll(struct drm_crtc *crtc,
 		if (((rdev->family == CHIP_RS780) || (rdev->family == CHIP_RS880))
 		    && !radeon_crtc->ss_enabled)
 			radeon_crtc->pll_flags |= RADEON_PLL_USE_FRAC_FB_DIV;
-		if (ASIC_IS_DCE32(rdev) && mode->clock > 165000)
+		if (ASIC_IS_DCE31(rdev) && mode->clock > 165000)
 			radeon_crtc->pll_flags |= RADEON_PLL_USE_FRAC_FB_DIV;
 	} else {
 		radeon_crtc->pll_flags |= RADEON_PLL_LEGACY;
diff --git a/drivers/gpu/drm/radeon/atombios_encoders.c b/drivers/gpu/drm/radeon/atombios_encoders.c
index 5cfd8fcfa..4e984973c 100644
--- a/drivers/gpu/drm/radeon/atombios_encoders.c
+++ b/drivers/gpu/drm/radeon/atombios_encoders.c
@@ -2123,12 +2123,13 @@ int radeon_atom_pick_dig_encoder(struct drm_encoder *encoder, int fe_idx)
 	}
 
 	/*
-	 * On DCE32 any encoder can drive any block so usually just use crtc id,
-	 * but Apple thinks different at least on iMac10,1 and iMac11,2, so there use linkb,
-	 * otherwise the internal eDP panel will stay dark.
+	 * On DCE31 and DCE32 any encoder can drive any block so usually just use crtc id,
+	 * but Apple thinks different at least on iMac10,1, iMac11,1 and iMac11,2,
+	 * so there use linkb, otherwise the internal eDP panel will stay dark.
 	 */
-	if (ASIC_IS_DCE32(rdev)) {
+	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
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index 527b9d19d..6b7c0abe4 100644
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -2625,6 +2625,7 @@ void r100_pll_errata_after_index(struct radeon_device *rdev);
 			    (rdev->family == CHIP_RS740)  ||	\
 			    (rdev->family >= CHIP_R600))
 #define ASIC_IS_DCE3(rdev) ((rdev->family >= CHIP_RV620))
+#define ASIC_IS_DCE31(rdev) ((rdev->family >= CHIP_RV770))
 #define ASIC_IS_DCE32(rdev) ((rdev->family >= CHIP_RV730))
 #define ASIC_IS_DCE4(rdev) ((rdev->family >= CHIP_CEDAR))
 #define ASIC_IS_DCE41(rdev) ((rdev->family >= CHIP_PALM) && \
-- 
2.47.3


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* 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.
  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 Code Review Bot
  2026-05-16  5:30 ` Claude Code Review Bot
  1 sibling, 0 replies; 4+ messages in thread
From: Claude Code Review Bot @ 2026-05-16  5:30 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: 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.
Author: Gilles Risch <gilles.risch@gmail.com>
Patches: 1
Reviewed: 2026-05-16T15:30:57.705528

---

This is a single-patch series that fixes a dark internal eDP panel on the Apple iMac11,1 (late 2009, ATI Mobility Radeon HD 4850 / RV770). The approach introduces a new `ASIC_IS_DCE31` macro to include the RV770 chip in code paths that previously only covered DCE 3.2 (RV730 and later), and adds the iMac11,1 to an existing Apple DMI quirk list for encoder selection.

The fix addresses a real hardware issue and the core logic is sound, but there are several problems with the submission that need addressing before it can be accepted:

1. **Severely malformed subject line** -- the entire patch description has been placed in the Subject header, violating standard kernel patch formatting.
2. **DCE version naming is questionable** -- historical AMD documentation associates DCE 3.1 with the RS780/RS880 IGPs, not RV770. The RV770 is widely understood to be DCE 3.2, and the existing `ASIC_IS_DCE32` macro appears to have a longstanding off-by-one in the chip enum that excludes it. Creating a new macro with a debatable name rather than fixing the existing one warrants discussion.
3. **PLL behavior changes affect all RV770 systems**, not just Apple iMacs -- this is not called out in the commit message and represents a regression risk.
4. **Thin commit message** with no technical explanation of the root cause.

---

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 4+ messages in thread

* 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.
  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
  1 sibling, 0 replies; 4+ messages in thread
From: Claude Code Review Bot @ 2026-05-16  5:30 UTC (permalink / raw)
  To: dri-devel-reviews

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-05-16  5:30 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
  -- strict thread matches above, loose matches on Subject: below --
2026-05-10 18:54 [PATCH] " Gilles Risch

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox