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 v2] drm/radeon: fix internal display on iMac11, 1 (RV770/DCE3.1) Date: Mon, 18 May 2026 16:36:04 +1000 Message-ID: In-Reply-To: References: <20260510185426.4264-1-gilles.risch@gmail.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 **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 =E2=80=94 without it, the eDP panel power-on sequence i= s skipped entirely for DCE3.1. The connector type check (`!=3D DRM_MODE_CON= NECTOR_eDP`) above this line already gates entry, so extending to DCE3.1 wo= n'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 `>=3D CHIP_RV770`= and `ASIC_IS_DCE4()` is `>=3D CHIP_CEDAR`, and `CHIP_CEDAR > CHIP_RV770` i= n 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 sen= t `DP_VIDEO_ON`/`DP_VIDEO_OFF` for DCE4+. By changing to `ASIC_IS_DCE31(rde= v)` (even with the redundant `||`), this now also sends these commands for = **DCE3.2** hardware (RV730, RV710, RV740) =E2=80=94 not just RV770. The aut= hor should confirm this is intentional and safe for DCE3.2 systems, or narr= ow the condition if only RV770 needs it. **Other issues:** 1. **Missing `Fixes:` tag** =E2=80=94 same as patch 2. 2. **Missing `Signed-off-by` chain context.** This patch depends on patch v= 2 (it uses `ASIC_IS_DCE31` introduced there and its `diff --git` index line= `4e984973c043` matches the post-v2 state). The dependency should be explic= itly stated via an `In-Reply-To:` to the v2 patch or a cover letter, and th= e 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** =E2=80=94 the autho= r should confirm the ATOM BIOS table on RV770 actually implements these enc= oder commands. If the VBIOS table doesn't handle them, `atom_execute_table(= )` might silently succeed or could misbehave. The author says it's tested o= n iMac11,1, which is good, but broader DCE3.1/3.2 implications should be co= nsidered. --- Generated by Claude Code Patch Reviewer