From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: Re: [PATCH] drm/radeon: fix eDP resume from suspend on iMac11, 1 / DCE3.1 systems Date: Thu, 04 Jun 2026 13:37:52 +1000 Message-ID: In-Reply-To: <202605171126.1g7CRkTZ-lkp@intel.com> References: <20260518211409.4868-3-gilles.risch@gmail.com> <202605171126.1g7CRkTZ-lkp@intel.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review Superseded by v5. Combined everything into a single patch. Still introduces= `ASIC_IS_DCE31()` in `radeon.h` but only uses it in the encoder picker =E2= =80=94 other uses are `dmi_match()`. ### PATCH 6 (LATEST): [PATCH v5] drm/radeon: fix internal display on iMac11= ,1 (RV770/DCE3.1) This is the version that matters. Three changes across two files: **1. atombios_crtc.c =E2=80=94 PLL fractional feedback divider quirk** ```c +#include ``` Correct =E2=80=94 needed for `dmi_match()`, not previously included in this= file. ```c - if (ASIC_IS_DCE32(rdev) && mode->clock > 165000) + if ((ASIC_IS_DCE32(rdev) || dmi_match(DMI_PRODUCT_NAME, "iMac11,1")) + && mode->clock > 165000) radeon_crtc->pll_flags |=3D RADEON_PLL_USE_FRAC_FB_DIV; ``` This enables the fractional feedback divider for high-clock modes on the iM= ac11,1's 2560x1440 panel. The 165000 kHz threshold corresponds to single-li= nk DVI limits. This is correct =E2=80=94 the iMac11,1's RV770 (DCE3.1) was = excluded from the DCE3.2 check because `CHIP_RV770 < CHIP_RV730` in the enu= m. Note: the other DCE32 check at line ~583 (`ASIC_IS_DCE32(rdev) && mode->clo= ck > 200000` for `RADEON_PLL_PREFER_HIGH_FB_DIV`) is intentionally left unt= ouched. This seems reasonable if the iMac11,1 works without the high-FB-div= preference. **2. atombios_encoders.c =E2=80=94 DP_VIDEO_ON quirk** ```c - if (ASIC_IS_DCE4(rdev)) + if (ASIC_IS_DCE4(rdev) || dmi_match(DMI_PRODUCT_NAME, "iMac11,1")) atombios_dig_encoder_setup(encoder, ATOM_ENCODER_CMD_DP_VIDEO_ON, 0); ``` Enables the DP video-on command for the iMac11,1. Reasonable for initial di= splay bring-up. **Concern: DP_VIDEO_OFF not patched.** The corresponding `DPMS_OFF` path at= line 1727: ```c if (ASIC_IS_DCE4(rdev)) { if (ENCODER_MODE_IS_DP(atombios_get_encoder_mode(encoder)) && connector) atombios_dig_encoder_setup(encoder, ATOM_ENCODER_CMD_DP_VIDEO_OFF, 0); } ``` is not updated with the same `dmi_match()`. In earlier versions (v3 2/2), b= oth ON and OFF were patched. If the encoder needs an explicit VIDEO_ON to l= ight up, symmetry suggests it should also get VIDEO_OFF when powering down,= otherwise the encoder may be left in an inconsistent state on DPMS off or = suspend. The author should clarify whether omitting the OFF path is intenti= onal or an oversight. **3. atombios_encoders.c =E2=80=94 Encoder picker restructure** ```c - if (ASIC_IS_DCE32(rdev)) { - if (dmi_match(DMI_PRODUCT_NAME, "iMac10,1") || - dmi_match(DMI_PRODUCT_NAME, "iMac11,2")) - enc_idx =3D (dig->linkb) ? 1 : 0; - else - enc_idx =3D radeon_crtc->crtc_id; - + if (dmi_match(DMI_PRODUCT_NAME, "iMac10,1") || + dmi_match(DMI_PRODUCT_NAME, "iMac11,1") || + dmi_match(DMI_PRODUCT_NAME, "iMac11,2")) { + enc_idx =3D (dig->linkb) ? 1 : 0; goto assigned; } + + /* on DCE32 and encoder can driver any block so just crtc id */ + if (ASIC_IS_DCE32(rdev)) { + enc_idx =3D radeon_crtc->crtc_id; + goto assigned; + } ``` This restructuring is the cleanest part of the patch. It: - Separates the Apple DMI quirk from the DCE version check - Adds iMac11,1 to the quirk list - Preserves identical behavior for all existing paths: - Apple iMac10,1/iMac11,2 (DCE3.2): still gets linkb (now matched by DMI = before the DCE32 check) - Non-Apple DCE3.2: still gets `crtc_id` - iMac11,1 (DCE3.1): **now correctly gets linkb** (previously fell throug= h to DCE3 LVTMA logic) - Non-Apple DCE3.1: unchanged (DMI fails, DCE32 fails, falls to DCE3 logi= c) **Minor issue: typo in comment.** The new comment reads: ```c /* on DCE32 and encoder can driver any block so just crtc id */ ``` Should be: `/* on DCE32 any encoder can drive any block so just use crtc id= */` **Other observations:** - **Missing Fixes/Cc-stable tags**: This fixes a real dark-display regressi= on. A `Cc: stable@vger.kernel.org` tag would be appropriate. A `Fixes:` tag= pointing to the commit that introduced the DCE32 encoder picker logic woul= d help stable backports. - **No `atombios_set_edp_panel_power()` change**: The v5 does not extend th= e eDP panel power function to iMac11,1. The earlier v3 series included this= as patch 2/2 for suspend/resume. The commit message no longer mentions sus= pend/resume, so this is apparently deferred. This is a reasonable scoping d= ecision =E2=80=94 fix initial display first, tackle suspend/resume separate= ly. - **The ASIC_IS_DCE31 macro is no longer added**: v5 dropped this entirely = in favor of targeted `dmi_match()` calls, which is the safer approach for a= legacy driver where broad DCE3.1 changes could regress other hardware. **Summary**: v5 is a well-targeted, low-risk quirk fix. The encoder picker = restructure is clean and correct. Two items need attention before merge: th= e comment typo, and clarification on whether the missing `DP_VIDEO_OFF` dmi= _match is intentional. --- Generated by Claude Code Patch Reviewer