From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/edid: parse panel type from DisplayID 2.x Display Parameters block
Date: Sat, 16 May 2026 11:17:30 +1000
Message-ID: <review-patch1-20260514065606.1151834-2-chen-yu.chen@amd.com>
In-Reply-To: <20260514065606.1151834-2-chen-yu.chen@amd.com>
References: <20260514065606.1151834-1-chen-yu.chen@amd.com> <20260514065606.1151834-2-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

**Struct layout and minimum length check =E2=80=94 looks correct.**

The `displayid_display_params_block` struct places `device_tech_byte` at pa=
yload offset 27 (absolute block offset 0x1E), matching the spec reference. =
The `DISPLAYID_DISPLAY_PARAMS_MIN_LEN` correctly computes 29 (the struct pa=
yload excluding the `displayid_block` header), ensuring `device_tech_byte` =
is accessible before it's read:

```c
if (block->num_bytes < DISPLAYID_DISPLAY_PARAMS_MIN_LEN) {
    ...
    continue;
}
```

**Private field access in `displayid_iter`.**

The patch accesses `iter.section` directly:

```c
const u8 *section =3D NULL;
...
if (section !=3D iter.section) {
    ...
    section =3D iter.section;
}
```

The `displayid_iter` struct is annotated with `Do not access directly, this=
 is private.` While `drm_edid.c` is core DRM code and uses the internal hea=
der, this still violates the stated API contract. Consider adding an access=
or like `displayid_section()` alongside the existing `displayid_version()` =
and `displayid_primary_use()`. Alternatively, a boolean `displayid_is_v2_he=
admount()` helper that encapsulates the VR/AR check might be cleaner, so th=
e section-tracking remains internal to the iterator implementation.

**Behavior change from removing the `break` =E2=80=94 intentional but worth=
 noting in the commit message.**

The old code:
```c
displayid_iter_for_each(block, &iter) {
    // log + VR/AR check
    break;  // only first block of first section
}
```

The new code iterates **all blocks across all sections**. This is required =
to find the Display Parameters block, and the VR/AR detection is correctly =
refactored to fire once per section via the `section` pointer guard. Howeve=
r, the commit message doesn't mention this change in iteration scope. It wo=
uld be helpful to note that the function now processes all DisplayID sectio=
ns/blocks, not just the first.

**Minor: magic numbers in the switch.**

```c
case 1: /* LCD */
    info->did_panel_type =3D DRM_MODE_PANEL_TYPE_LCD;
    break;
case 2: /* OLED */
    info->did_panel_type =3D DRM_MODE_PANEL_TYPE_OLED;
    break;
```

These match the spec (Display Device Technology: 001b =3D LCD, 010b =3D OLE=
D). The comments are sufficient, but named constants in the header (e.g. `D=
ISPLAYID_DEVICE_TECH_LCD =3D 1`, `DISPLAYID_DEVICE_TECH_OLED =3D 2`) would =
be slightly more self-documenting and consistent with the kernel style for =
spec-derived values.

**Missing update to `drm_panel_type_enum_list`.**

The new `DRM_MODE_PANEL_TYPE_LCD` value is added to `drm_mode.h` but the en=
um registration in `drm_connector.c` is not updated:

```c
static const struct drm_prop_enum_list drm_panel_type_enum_list[] =3D {
    { DRM_MODE_PANEL_TYPE_UNKNOWN, "unknown" },
    { DRM_MODE_PANEL_TYPE_OLED, "OLED" },
    // missing: { DRM_MODE_PANEL_TYPE_LCD, "LCD" },
};
```

Without this, `DRM_MODE_PANEL_TYPE_LCD` cannot actually be used as a proper=
ty value. This should be part of this patch (since this is where the UAPI c=
onstant is introduced).

**Overall:** Structurally solid parsing code, but needs the enum list updat=
e and preferably an accessor for `iter.section`.

---

---
Generated by Claude Code Patch Reviewer