From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/amd/display: use DisplayID panel type in dm_set_panel_type Date: Mon, 25 May 2026 22:19:21 +1000 Message-ID: In-Reply-To: <20260520021432.1301326-4-chen-yu.chen@amd.com> References: <20260520021432.1301326-1-chen-yu.chen@amd.com> <20260520021432.1301326-4-chen-yu.chen@amd.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 **Purpose:** Wires up `did_panel_type` into amdgpu_dm's detection priority = chain. **Priority placement:** The new block is inserted correctly =E2=80=94 after= the VSDB and DPCD checks, before the Samsung luminance heuristic: ```c + /* If VSDB and DPCD didn't determine panel type, check DID */ + if (link->panel_type =3D=3D PANEL_TYPE_NONE) { + if (display_info->did_panel_type =3D=3D DRM_MODE_PANEL_TYPE_LCD) + link->panel_type =3D PANEL_TYPE_LCD; + else if (display_info->did_panel_type =3D=3D DRM_MODE_PANEL_TYPE_OLED) + link->panel_type =3D PANEL_TYPE_OLED; + } ``` **Effectiveness concern:** The existing end-of-function code only exposes O= LED to userspace: ```c if (link->panel_type =3D=3D PANEL_TYPE_OLED) drm_object_property_set_value(..., DRM_MODE_PANEL_TYPE_OLED); else drm_object_property_set_value(..., DRM_MODE_PANEL_TYPE_UNKNOWN); ``` So `PANEL_TYPE_LCD` detected via DisplayID would internally set `link->pane= l_type =3D PANEL_TYPE_LCD` (which is `1` in `dc_types.h`), but the userspac= e property would still read "unknown". Currently no DC code checks `panel_t= ype =3D=3D PANEL_TYPE_LCD` anywhere, so this detection has no observable ef= fect unless there are downstream consumers added later. This may be intenti= onal (laying groundwork), but it should be documented or the property-setti= ng code should be updated to also handle LCD. **Cross-domain constant mixing:** The code mixes UAPI constants (`DRM_MODE_= PANEL_TYPE_LCD`) with internal DC constants (`PANEL_TYPE_LCD`). This works = because they're independent enums, but it's somewhat unusual to compare a `= drm_display_info` field set with UAPI values against those UAPI values and = then assign internal DC enum values. This is fine functionally but worth no= ting for maintainability =E2=80=94 a mapping helper could improve clarity. **Overall for Patch 3:** The TODO removal is welcome and the fallback inser= tion point is correct. The main gap is that LCD detection currently has no = visible effect. --- Generated by Claude Code Patch Reviewer