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
  2026-05-16  5:26 ` Claude review: " Claude Code Review Bot
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ 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] 12+ 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 18:54 [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:26 ` Claude Code Review Bot
  2026-05-16  5:26 ` Claude Code Review Bot
  2026-05-16  9:24 ` [PATCH v2] drm/radeon: fix internal display on iMac11, 1 (RV770/DCE3.1) Gilles Risch
  2 siblings, 0 replies; 12+ messages in thread
From: Claude Code Review Bot @ 2026-05-16  5:26 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:26:36.767296

---

This is a single patch fixing display issues on the Apple iMac11,1 (late 2009, ATI Mobility Radeon HD 4850 / RV770). The core problem being fixed is real: the RV770 chip is DCE 3.2 hardware, but the existing `ASIC_IS_DCE32` macro (`>= CHIP_RV730`) accidentally excludes RV770 because `CHIP_RV770` precedes `CHIP_RV730` in the `radeon_family` enum. This causes PLL misconfiguration and wrong encoder selection on RV770-based Apple machines.

However, the patch has **significant issues** with its approach, naming, commit message formatting, and missing sign-off. It should not be applied in its current form.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 12+ 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 18:54 [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:26 ` Claude review: " Claude Code Review Bot
@ 2026-05-16  5:26 ` Claude Code Review Bot
  2026-05-16  9:24 ` [PATCH v2] drm/radeon: fix internal display on iMac11, 1 (RV770/DCE3.1) Gilles Risch
  2 siblings, 0 replies; 12+ messages in thread
From: Claude Code Review Bot @ 2026-05-16  5:26 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Commit message formatting issues:**

- The subject line contains the entire commit description, which is against kernel patch conventions. It should be a short imperative line like `drm/radeon: Fix display on iMac11,1 with HD 4850` with details in the body.
- **Missing `Signed-off-by:` line** — required by the kernel's Developer Certificate of Origin policy. The patch cannot be accepted without it.
- The `Fixes:` tag format is wrong. It references a freedesktop.org xf86-video-ati issue, not a kernel commit. If a kernel commit introduced the regression, that commit hash should be cited with the standard `Fixes: <hash> ("subject")` format.

**Incorrect macro definition — the central problem:**

```c
#define ASIC_IS_DCE31(rdev) ((rdev->family >= CHIP_RV770))
#define ASIC_IS_DCE32(rdev) ((rdev->family >= CHIP_RV730))
```

From `radeon_family.h`, the enum ordering is:
```
CHIP_RS780  = 72  (DCE 3.1)
CHIP_RS880  = 73  (DCE 3.1)
CHIP_RV770  = 74  (DCE 3.2)
CHIP_RV730  = 75  (DCE 3.2)
```

The new `ASIC_IS_DCE31` defined as `>= CHIP_RV770` does **not** match any actual DCE 3.1 hardware (RS780, RS880). It only matches DCE 3.2 hardware — it is effectively a corrected version of `ASIC_IS_DCE32`. The naming is misleading and will confuse future maintainers.

The real bug is that `ASIC_IS_DCE32` itself is wrong: it should be `>= CHIP_RV770` instead of `>= CHIP_RV730`, since RV770 is DCE 3.2 hardware. The correct fix is to fix `ASIC_IS_DCE32` directly, not introduce a new macro with a confusing name. This would also fix the other ~7 callsites in `r600.c` and `radeon_audio.c` that currently exclude RV770 from DCE 3.2 logic.

**PLL changes are broader than necessary:**

```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, not just Apple iMac11,1. Since RV770 genuinely is DCE 3.2 hardware, these PLL flags arguably should apply to it — but this represents a behavior change for all RV770 users and should be explicitly acknowledged and tested. It's worth noting that line 588 already uses `rdev->family < CHIP_RV770` as a boundary, suggesting the code already knows RV770 belongs with the newer chips.

**Encoder selection change is correctly scoped:**

```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;
```

This part is fine — the DMI matching gates the Apple-specific quirk so only the listed machines are affected. Adding `iMac11,1` here is the right thing to do. The comment update is also appropriate.

**Recommended approach for a v2:**

1. Fix `ASIC_IS_DCE32` to be `>= CHIP_RV770` instead of creating a new `ASIC_IS_DCE31` macro. This fixes the root cause (RV770 being wrongly excluded from DCE 3.2) and cleans up all callsites at once.
2. Add the `iMac11,1` DMI match in `atombios_encoders.c` — this is unambiguously correct.
3. Use a proper short subject line: `drm/radeon: Fix display on Apple iMac11,1 (HD 4850/RV770)`.
4. Add `Signed-off-by:`.
5. Write a proper commit message explaining that `ASIC_IS_DCE32` incorrectly excluded RV770 due to the enum ordering.

---
Generated by Claude Code Patch Reviewer

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

* [PATCH v2] drm/radeon: fix internal display on iMac11, 1 (RV770/DCE3.1)
  2026-05-10 18:54 [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:26 ` Claude review: " Claude Code Review Bot
  2026-05-16  5:26 ` Claude Code Review Bot
@ 2026-05-16  9:24 ` Gilles Risch
  2026-05-16 18:09   ` Lukas Wunner
                     ` (2 more replies)
  2 siblings, 3 replies; 12+ messages in thread
From: Gilles Risch @ 2026-05-16  9:24 UTC (permalink / raw)
  To: alexander.deucher; +Cc: amd-gfx, dri-devel, Gilles Risch

The Apple iMac11,1 (late 2009) has an integrated ATI Mobility Radeon
HD 4850 (RV770/M98L) with a 2560x1440 internal panel connected via an
internal DisplayPort path. This machine suffers from a similar problem
as the iMac10,1 (late 2009) and the iMac11,2 (mid 2010). Without this
fix the display stays dark under KMS. Two issues are addressed:

1. The RV770 implements DCE3.1 and not DCE3.2. ASIC_IS_DCE32() starts at
   CHIP_RV730 which is newer than RV770, so the RV770 never matched the
   DCE3.2 PLL and encoder logic. Introduce ASIC_IS_DCE31() starting at
   CHIP_RV770 to fix this.

2. Apple routed the internal display through Link B of the DIG encoder
   instead of Link A, as observed in the kernel display connector log.
   The same quirk already exists for iMac10,1 and iMac11,2 - iMac11,1
   was simply missing from the list.

Note: resume from suspend still results in a dark screen as the DP
re-driver chips on the mainboard lose their state during power-off.
This will be addressed in a follow-up patch.

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>
---
v2: No code changes. Fixed commit message:
    - Added drm/radeon: prefix to subject line
    - Added empty line between subject and body
    - Added technical description of the changes

 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 2fc0334e0d6c..3c6d332739e3 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 5cfd8fcfa5e8..4e984973c043 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 527b9d19d730..6b7c0abe49fb 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] 12+ messages in thread

* Re: [PATCH v2] drm/radeon: fix internal display on iMac11, 1 (RV770/DCE3.1)
  2026-05-16  9:24 ` [PATCH v2] drm/radeon: fix internal display on iMac11, 1 (RV770/DCE3.1) Gilles Risch
@ 2026-05-16 18:09   ` Lukas Wunner
  2026-05-17 13:53     ` Gilles Risch
  2026-05-18  6:36     ` Claude review: " Claude Code Review Bot
  2026-05-16 18:52   ` [PATCH] drm/radeon: fix eDP resume from suspend on iMac11, 1 / DCE3.1 systems Gilles Risch
  2026-05-18  6:57   ` Claude review: drm/radeon: fix internal display on iMac11, 1 (RV770/DCE3.1) Claude Code Review Bot
  2 siblings, 2 replies; 12+ messages in thread
From: Lukas Wunner @ 2026-05-16 18:09 UTC (permalink / raw)
  To: Gilles Risch; +Cc: alexander.deucher, amd-gfx, dri-devel

On Sat, May 16, 2026 at 11:24:21AM +0200, Gilles Risch wrote:
> The Apple iMac11,1 (late 2009) has an integrated ATI Mobility Radeon
> HD 4850 (RV770/M98L) with a 2560x1440 internal panel connected via an
> internal DisplayPort path. This machine suffers from a similar problem
> as the iMac10,1 (late 2009) and the iMac11,2 (mid 2010). Without this
> fix the display stays dark under KMS. Two issues are addressed:
> 
> 1. The RV770 implements DCE3.1 and not DCE3.2. ASIC_IS_DCE32() starts at
>    CHIP_RV730 which is newer than RV770, so the RV770 never matched the
>    DCE3.2 PLL and encoder logic. Introduce ASIC_IS_DCE31() starting at
>    CHIP_RV770 to fix this.
> 
> 2. Apple routed the internal display through Link B of the DIG encoder
>    instead of Link A, as observed in the kernel display connector log.
>    The same quirk already exists for iMac10,1 and iMac11,2 - iMac11,1
>    was simply missing from the list.

Not an expert in the radeon driver or AMD chip revisions, but if it
helps improve support for older Macs I'll be happy to offer my:

Reviewed-by: Lukas Wunner <lukas@wunner.de>

It's not immediately obvious that CHIP_RV770 precedes CHIP_RV730
in the enum and that hence ASIC_IS_DCE31() subsumes ASIC_IS_DCE32().

Thanks,

Lukas

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

* [PATCH] drm/radeon: fix eDP resume from suspend on iMac11, 1 / DCE3.1 systems
  2026-05-16  9:24 ` [PATCH v2] drm/radeon: fix internal display on iMac11, 1 (RV770/DCE3.1) Gilles Risch
  2026-05-16 18:09   ` Lukas Wunner
@ 2026-05-16 18:52   ` Gilles Risch
  2026-05-17  3:28     ` kernel test robot
  2026-05-17  5:12     ` kernel test robot
  2026-05-18  6:57   ` Claude review: drm/radeon: fix internal display on iMac11, 1 (RV770/DCE3.1) Claude Code Review Bot
  2 siblings, 2 replies; 12+ messages in thread
From: Gilles Risch @ 2026-05-16 18:52 UTC (permalink / raw)
  To: alexander.deucher; +Cc: amd-gfx, dri-devel, Gilles Risch

After suspend/resume the internal eDP display on iMac11,1 (and
potentially other DCE3.1 systems) stays dark because
atombios_set_edp_panel_power() skips panel power control for
anything older than DCE4.

Fix this by:
- Extending atombios_set_edp_panel_power() to also handle DCE3.1
  by changing the !ASIC_IS_DCE4() guard to !ASIC_IS_DCE31().
  HPD polling works correctly on DCE3.1 (verified: HPD is asserted
  at iteration 0 on iMac11,1).
- Issuing ATOM_ENCODER_CMD_DP_VIDEO_ON/OFF for DCE3.1 in addition
  to DCE4+.

Tested on iMac11,1 (Mobility Radeon HD 4850, RV770/DCE3.1).

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

diff --git a/drivers/gpu/drm/radeon/atombios_encoders.c b/drivers/gpu/drm/radeon/atombios_encoders.c
index 4e984973c043..b62fd713efcf 100644
--- a/drivers/gpu/drm/radeon/atombios_encoders.c
+++ b/drivers/gpu/drm/radeon/atombios_encoders.c
@@ -1383,7 +1383,7 @@ atombios_set_edp_panel_power(struct drm_connector *connector, int action)
 	if (connector->connector_type != DRM_MODE_CONNECTOR_eDP)
 		goto done;
 
-	if (!ASIC_IS_DCE4(rdev))
+	if (!ASIC_IS_DCE31(rdev))
 		goto done;
 
 	if ((action != ATOM_TRANSMITTER_ACTION_POWER_ON) &&
@@ -1707,7 +1707,7 @@ radeon_atom_encoder_dpms_dig(struct drm_encoder *encoder, int mode)
 		if (ENCODER_MODE_IS_DP(atombios_get_encoder_mode(encoder)) && connector) {
 			/* DP_SET_POWER_D0 is set in radeon_dp_link_train */
 			radeon_dp_link_train(encoder, connector);
-			if (ASIC_IS_DCE4(rdev))
+			if (ASIC_IS_DCE4(rdev) || ASIC_IS_DCE31(rdev))
 				atombios_dig_encoder_setup(encoder, ATOM_ENCODER_CMD_DP_VIDEO_ON, 0);
 		}
 		if (radeon_encoder->devices & (ATOM_DEVICE_LCD_SUPPORT)) {
@@ -1724,7 +1724,7 @@ radeon_atom_encoder_dpms_dig(struct drm_encoder *encoder, int mode)
 	case DRM_MODE_DPMS_SUSPEND:
 	case DRM_MODE_DPMS_OFF:
 
-		if (ASIC_IS_DCE4(rdev)) {
+		if (ASIC_IS_DCE4(rdev) || ASIC_IS_DCE31(rdev)) {
 			if (ENCODER_MODE_IS_DP(atombios_get_encoder_mode(encoder)) && connector)
 				atombios_dig_encoder_setup(encoder, ATOM_ENCODER_CMD_DP_VIDEO_OFF, 0);
 		}
-- 
2.47.3


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

* Re: [PATCH] drm/radeon: fix eDP resume from suspend on iMac11, 1 / DCE3.1 systems
  2026-05-16 18:52   ` [PATCH] drm/radeon: fix eDP resume from suspend on iMac11, 1 / DCE3.1 systems Gilles Risch
@ 2026-05-17  3:28     ` kernel test robot
  2026-05-17  5:12     ` kernel test robot
  1 sibling, 0 replies; 12+ messages in thread
From: kernel test robot @ 2026-05-17  3:28 UTC (permalink / raw)
  To: Gilles Risch, alexander.deucher
  Cc: llvm, oe-kbuild-all, amd-gfx, dri-devel, Gilles Risch

Hi Gilles,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-misc/drm-misc-next]
[also build test ERROR on linus/master v7.1-rc3 next-20260508]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Gilles-Risch/drm-radeon-fix-eDP-resume-from-suspend-on-iMac11-1-DCE3-1-systems/20260517-025343
base:   https://gitlab.freedesktop.org/drm/misc/kernel.git drm-misc-next
patch link:    https://lore.kernel.org/r/20260516185226.3005-1-gilles.risch%40gmail.com
patch subject: [PATCH] drm/radeon: fix eDP resume from suspend on iMac11, 1 / DCE3.1 systems
config: riscv-randconfig-001-20260517 (https://download.01.org/0day-ci/archive/20260517/202605171126.1g7CRkTZ-lkp@intel.com/config)
compiler: clang version 23.0.0git (https://github.com/llvm/llvm-project 5bac06718f502014fade905512f1d26d578a18f3)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260517/202605171126.1g7CRkTZ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202605171126.1g7CRkTZ-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/radeon/atombios_encoders.c:1386:7: error: call to undeclared function 'ASIC_IS_DCE31'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    1386 |         if (!ASIC_IS_DCE31(rdev))
         |              ^
   drivers/gpu/drm/radeon/atombios_encoders.c:1710:30: error: call to undeclared function 'ASIC_IS_DCE31'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    1710 |                         if (ASIC_IS_DCE4(rdev) || ASIC_IS_DCE31(rdev))
         |                                                   ^
   drivers/gpu/drm/radeon/atombios_encoders.c:1727:29: error: call to undeclared function 'ASIC_IS_DCE31'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    1727 |                 if (ASIC_IS_DCE4(rdev) || ASIC_IS_DCE31(rdev)) {
         |                                           ^
   3 errors generated.


vim +/ASIC_IS_DCE31 +1386 drivers/gpu/drm/radeon/atombios_encoders.c

  1372	
  1373	bool
  1374	atombios_set_edp_panel_power(struct drm_connector *connector, int action)
  1375	{
  1376		struct radeon_connector *radeon_connector = to_radeon_connector(connector);
  1377		struct drm_device *dev = radeon_connector->base.dev;
  1378		struct radeon_device *rdev = dev->dev_private;
  1379		union dig_transmitter_control args;
  1380		int index = GetIndexIntoMasterTable(COMMAND, UNIPHYTransmitterControl);
  1381		uint8_t frev, crev;
  1382	
  1383		if (connector->connector_type != DRM_MODE_CONNECTOR_eDP)
  1384			goto done;
  1385	
> 1386		if (!ASIC_IS_DCE31(rdev))
  1387			goto done;
  1388	
  1389		if ((action != ATOM_TRANSMITTER_ACTION_POWER_ON) &&
  1390		    (action != ATOM_TRANSMITTER_ACTION_POWER_OFF))
  1391			goto done;
  1392	
  1393		if (!atom_parse_cmd_header(rdev->mode_info.atom_context, index, &frev, &crev))
  1394			goto done;
  1395	
  1396		memset(&args, 0, sizeof(args));
  1397	
  1398		args.v1.ucAction = action;
  1399	
  1400		atom_execute_table(rdev->mode_info.atom_context, index, (uint32_t *)&args, sizeof(args));
  1401	
  1402		/* wait for the panel to power up */
  1403		if (action == ATOM_TRANSMITTER_ACTION_POWER_ON) {
  1404			int i;
  1405	
  1406			for (i = 0; i < 300; i++) {
  1407				if (radeon_hpd_sense(rdev, radeon_connector->hpd.hpd))
  1408					return true;
  1409				mdelay(1);
  1410			}
  1411			return false;
  1412		}
  1413	done:
  1414		return true;
  1415	}
  1416	

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

* Re: [PATCH] drm/radeon: fix eDP resume from suspend on iMac11, 1 / DCE3.1 systems
  2026-05-16 18:52   ` [PATCH] drm/radeon: fix eDP resume from suspend on iMac11, 1 / DCE3.1 systems Gilles Risch
  2026-05-17  3:28     ` kernel test robot
@ 2026-05-17  5:12     ` kernel test robot
  1 sibling, 0 replies; 12+ messages in thread
From: kernel test robot @ 2026-05-17  5:12 UTC (permalink / raw)
  To: Gilles Risch, alexander.deucher
  Cc: oe-kbuild-all, amd-gfx, dri-devel, Gilles Risch

Hi Gilles,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-misc/drm-misc-next]
[also build test ERROR on linus/master v7.1-rc3 next-20260508]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Gilles-Risch/drm-radeon-fix-eDP-resume-from-suspend-on-iMac11-1-DCE3-1-systems/20260517-025343
base:   https://gitlab.freedesktop.org/drm/misc/kernel.git drm-misc-next
patch link:    https://lore.kernel.org/r/20260516185226.3005-1-gilles.risch%40gmail.com
patch subject: [PATCH] drm/radeon: fix eDP resume from suspend on iMac11, 1 / DCE3.1 systems
config: riscv-randconfig-002-20260517 (https://download.01.org/0day-ci/archive/20260517/202605171301.73YzX9wA-lkp@intel.com/config)
compiler: riscv32-linux-gcc (GCC) 15.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260517/202605171301.73YzX9wA-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202605171301.73YzX9wA-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/gpu/drm/radeon/atombios_encoders.c: In function 'atombios_set_edp_panel_power':
>> drivers/gpu/drm/radeon/atombios_encoders.c:1386:14: error: implicit declaration of function 'ASIC_IS_DCE31'; did you mean 'ASIC_IS_DCE3'? [-Wimplicit-function-declaration]
    1386 |         if (!ASIC_IS_DCE31(rdev))
         |              ^~~~~~~~~~~~~
         |              ASIC_IS_DCE3


vim +1386 drivers/gpu/drm/radeon/atombios_encoders.c

  1372	
  1373	bool
  1374	atombios_set_edp_panel_power(struct drm_connector *connector, int action)
  1375	{
  1376		struct radeon_connector *radeon_connector = to_radeon_connector(connector);
  1377		struct drm_device *dev = radeon_connector->base.dev;
  1378		struct radeon_device *rdev = dev->dev_private;
  1379		union dig_transmitter_control args;
  1380		int index = GetIndexIntoMasterTable(COMMAND, UNIPHYTransmitterControl);
  1381		uint8_t frev, crev;
  1382	
  1383		if (connector->connector_type != DRM_MODE_CONNECTOR_eDP)
  1384			goto done;
  1385	
> 1386		if (!ASIC_IS_DCE31(rdev))
  1387			goto done;
  1388	
  1389		if ((action != ATOM_TRANSMITTER_ACTION_POWER_ON) &&
  1390		    (action != ATOM_TRANSMITTER_ACTION_POWER_OFF))
  1391			goto done;
  1392	
  1393		if (!atom_parse_cmd_header(rdev->mode_info.atom_context, index, &frev, &crev))
  1394			goto done;
  1395	
  1396		memset(&args, 0, sizeof(args));
  1397	
  1398		args.v1.ucAction = action;
  1399	
  1400		atom_execute_table(rdev->mode_info.atom_context, index, (uint32_t *)&args, sizeof(args));
  1401	
  1402		/* wait for the panel to power up */
  1403		if (action == ATOM_TRANSMITTER_ACTION_POWER_ON) {
  1404			int i;
  1405	
  1406			for (i = 0; i < 300; i++) {
  1407				if (radeon_hpd_sense(rdev, radeon_connector->hpd.hpd))
  1408					return true;
  1409				mdelay(1);
  1410			}
  1411			return false;
  1412		}
  1413	done:
  1414		return true;
  1415	}
  1416	

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

* Re: [PATCH v2] drm/radeon: fix internal display on iMac11, 1 (RV770/DCE3.1)
  2026-05-16 18:09   ` Lukas Wunner
@ 2026-05-17 13:53     ` Gilles Risch
  2026-05-17 14:05       ` Lukas Wunner
  2026-05-18  6:36     ` Claude review: " Claude Code Review Bot
  1 sibling, 1 reply; 12+ messages in thread
From: Gilles Risch @ 2026-05-17 13:53 UTC (permalink / raw)
  To: Lukas Wunner; +Cc: alexander.deucher, amd-gfx, dri-devel

Good afternoon Lukas,

thanks for the review.

The initial launch date can either be verified in the TeraScale 
architecture comparison table on Wikipedia [1] or directly in the kernel 
source at drivers/gpu/drm/radeon/radeon_family.h where CHIP_RV770 (line 
74) precedes CHIP_RV730 (line 75).

The test robot mentions that it cannot find ASIC_IS_DCE31(); this is 
because both patches build on one another. Should I submit both patches 
as a series?

Kind regards,
Gilles

On 16.05.26 20:09, Lukas Wunner wrote:
> On Sat, May 16, 2026 at 11:24:21AM +0200, Gilles Risch wrote:
>> The Apple iMac11,1 (late 2009) has an integrated ATI Mobility Radeon
>> HD 4850 (RV770/M98L) with a 2560x1440 internal panel connected via an
>> internal DisplayPort path. This machine suffers from a similar problem
>> as the iMac10,1 (late 2009) and the iMac11,2 (mid 2010). Without this
>> fix the display stays dark under KMS. Two issues are addressed:
>>
>> 1. The RV770 implements DCE3.1 and not DCE3.2. ASIC_IS_DCE32() starts at
>>     CHIP_RV730 which is newer than RV770, so the RV770 never matched the
>>     DCE3.2 PLL and encoder logic. Introduce ASIC_IS_DCE31() starting at
>>     CHIP_RV770 to fix this.
>>
>> 2. Apple routed the internal display through Link B of the DIG encoder
>>     instead of Link A, as observed in the kernel display connector log.
>>     The same quirk already exists for iMac10,1 and iMac11,2 - iMac11,1
>>     was simply missing from the list.
> Not an expert in the radeon driver or AMD chip revisions, but if it
> helps improve support for older Macs I'll be happy to offer my:
>
> Reviewed-by: Lukas Wunner <lukas@wunner.de>
>
> It's not immediately obvious that CHIP_RV770 precedes CHIP_RV730
> in the enum and that hence ASIC_IS_DCE31() subsumes ASIC_IS_DCE32().
>
> Thanks,
>
> Lukas



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

* Re: [PATCH v2] drm/radeon: fix internal display on iMac11, 1 (RV770/DCE3.1)
  2026-05-17 13:53     ` Gilles Risch
@ 2026-05-17 14:05       ` Lukas Wunner
  0 siblings, 0 replies; 12+ messages in thread
From: Lukas Wunner @ 2026-05-17 14:05 UTC (permalink / raw)
  To: Gilles Risch; +Cc: alexander.deucher, amd-gfx, dri-devel

On Sun, May 17, 2026 at 03:53:51PM +0200, Gilles Risch wrote:
> The test robot mentions that it cannot find ASIC_IS_DCE31(); this is because
> both patches build on one another. Should I submit both patches as a series?

Yes, either that or wait for the first patch to be applied and show up in
the drm-next repo.  Then 0-day should not complain.

Usually people are annoyed if patches are resubmitted in short cadence,
so the polite thing to do is wait for 1 or 2 weeks before respinning
so that people get a chance to provide further comments.

Thanks,

Lukas

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

* Claude review: Re: [PATCH v2] drm/radeon: fix internal display on iMac11, 1 (RV770/DCE3.1)
  2026-05-16 18:09   ` Lukas Wunner
  2026-05-17 13:53     ` Gilles Risch
@ 2026-05-18  6:36     ` Claude Code Review Bot
  1 sibling, 0 replies; 12+ messages in thread
From: Claude Code Review Bot @ 2026-05-18  6:36 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Correctness: Mostly good, with a logical redundancy.**

**Panel power guard in atombios_set_edp_panel_power():**

```c
-	if (!ASIC_IS_DCE4(rdev))
+	if (!ASIC_IS_DCE31(rdev))
 		goto done;
```

This is the key fix — without it, the eDP panel power-on sequence is skipped entirely for DCE3.1. The connector type check (`!= DRM_MODE_CONNECTOR_eDP`) above this line already gates entry, so extending to DCE3.1 won't affect non-eDP systems. The author reports HPD polling works correctly on iMac11,1.

**DP video on/off changes:**

```c
-			if (ASIC_IS_DCE4(rdev))
+			if (ASIC_IS_DCE4(rdev) || ASIC_IS_DCE31(rdev))
 				atombios_dig_encoder_setup(encoder, ATOM_ENCODER_CMD_DP_VIDEO_ON, 0);
```

```c
-		if (ASIC_IS_DCE4(rdev)) {
+		if (ASIC_IS_DCE4(rdev) || ASIC_IS_DCE31(rdev)) {
```

**Issue: Logical redundancy.** Since `ASIC_IS_DCE31()` is `>= CHIP_RV770` and `ASIC_IS_DCE4()` is `>= CHIP_CEDAR`, and `CHIP_CEDAR > CHIP_RV770` in the enum, every chip matching DCE4 also matches DCE31. Therefore `ASIC_IS_DCE4(rdev) || ASIC_IS_DCE31(rdev)` simplifies to just `ASIC_IS_DCE31(rdev)`. The `ASIC_IS_DCE4` term is redundant.

However, this has a subtle semantic consequence: the original code only sent `DP_VIDEO_ON`/`DP_VIDEO_OFF` for DCE4+. By changing to `ASIC_IS_DCE31(rdev)` (even with the redundant `||`), this now also sends these commands for **DCE3.2** hardware (RV730, RV710, RV740) — not just RV770. The author should confirm this is intentional and safe for DCE3.2 systems, or narrow the condition if only RV770 needs it.

**Other issues:**

1. **Missing `Fixes:` tag** — same as patch 2.

2. **Missing `Signed-off-by` chain context.** This patch depends on patch v2 (it uses `ASIC_IS_DCE31` introduced there and its `diff --git` index line `4e984973c043` matches the post-v2 state). The dependency should be explicitly stated via an `In-Reply-To:` to the v2 patch or a cover letter, and the two should be submitted as a proper series (`[PATCH v2 1/2]` and `[PATCH v2 2/2]`).

3. **The `ATOM_ENCODER_CMD_DP_VIDEO_ON/OFF` on DCE3.1** — the author should confirm the ATOM BIOS table on RV770 actually implements these encoder commands. If the VBIOS table doesn't handle them, `atom_execute_table()` might silently succeed or could misbehave. The author says it's tested on iMac11,1, which is good, but broader DCE3.1/3.2 implications should be considered.

---
Generated by Claude Code Patch Reviewer

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

* Claude review: drm/radeon: fix internal display on iMac11, 1 (RV770/DCE3.1)
  2026-05-16  9:24 ` [PATCH v2] drm/radeon: fix internal display on iMac11, 1 (RV770/DCE3.1) Gilles Risch
  2026-05-16 18:09   ` Lukas Wunner
  2026-05-16 18:52   ` [PATCH] drm/radeon: fix eDP resume from suspend on iMac11, 1 / DCE3.1 systems Gilles Risch
@ 2026-05-18  6:57   ` Claude Code Review Bot
  2 siblings, 0 replies; 12+ messages in thread
From: Claude Code Review Bot @ 2026-05-18  6:57 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Correctness: Looks correct.** The new macro is well-placed and the logic follows the existing DCE versioning scheme.

**Issue 1 — Missing `Fixes:` tag:** The commit message references "Fixes freedesktop issue 164" in prose, but does not include a kernel-style `Fixes:` tag pointing to the commit that introduced the regression (or the original DCE3.2 logic that omitted RV770). If this is a fix for existing behavior, it should have:
```
Fixes: <sha1> ("drm/radeon: ...")
Cc: stable@vger.kernel.org
```
If there's no single commit to blame (the logic was always wrong), a `Cc: stable` is still appropriate since this is a hardware enablement fix for a longstanding bug.

**Issue 2 — PLL flag changes affect all RV770 systems, not just iMac:**

```c
-		if (ASIC_IS_DCE32(rdev) && mode->clock > 200000)
+		if (ASIC_IS_DCE31(rdev) && mode->clock > 200000)
```

This change means *every* RV770-based system (not just the iMac11,1) will now get `RADEON_PLL_PREFER_HIGH_FB_DIV` for high clocks and `RADEON_PLL_USE_FRAC_FB_DIV` for clocks > 165000. While this is likely the correct behavior for DCE3.1 hardware in general, the commit message frames this purely as an iMac fix. The author should confirm this is intentional for all RV770 cards, or if it should be gated behind a DMI check. Given that line 588 already has `if (rdev->family < CHIP_RV770)` as a boundary, this is likely fine — but it should be stated explicitly in the commit message.

**Issue 3 — Audit of remaining `ASIC_IS_DCE32` call sites:** There are additional `ASIC_IS_DCE32()` users in `r600.c` (interrupt handling, HDMI, HPD) and `radeon_audio.c`. The author should confirm these do not also need the DCE3.1 treatment. If they are truly DCE3.2-specific features, the commit message should note this.

**Minor — Subject line wrapping:** The subject line appears wrapped across multiple header lines in the mbox (`Subject: [PATCH v2] drm/radeon: fix internal display on iMac11,\n 1 (RV770/DCE3.1)`). This may be an email client artifact, but the author should verify `git log --oneline` shows it correctly.

---

---
Generated by Claude Code Patch Reviewer

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

end of thread, other threads:[~2026-05-18  6:57 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-10 18:54 [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:26 ` Claude review: " Claude Code Review Bot
2026-05-16  5:26 ` Claude Code Review Bot
2026-05-16  9:24 ` [PATCH v2] drm/radeon: fix internal display on iMac11, 1 (RV770/DCE3.1) Gilles Risch
2026-05-16 18:09   ` Lukas Wunner
2026-05-17 13:53     ` Gilles Risch
2026-05-17 14:05       ` Lukas Wunner
2026-05-18  6:36     ` Claude review: " Claude Code Review Bot
2026-05-16 18:52   ` [PATCH] drm/radeon: fix eDP resume from suspend on iMac11, 1 / DCE3.1 systems Gilles Risch
2026-05-17  3:28     ` kernel test robot
2026-05-17  5:12     ` kernel test robot
2026-05-18  6:57   ` Claude review: drm/radeon: fix internal display on iMac11, 1 (RV770/DCE3.1) Claude Code Review Bot

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