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/msm/dpu: Add support for Eliza SoC
Date: Wed, 04 Mar 2026 07:15:20 +1000
Message-ID: <review-patch7-20260303-drm-display-eliza-v1-7-814121dbb2bf@oss.qualcomm.com>
In-Reply-To: <20260303-drm-display-eliza-v1-7-814121dbb2bf@oss.qualcomm.com>
References: <20260303-drm-display-eliza-v1-0-814121dbb2bf@oss.qualcomm.com> <20260303-drm-display-eliza-v1-7-814121dbb2bf@oss.qualcomm.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

This is the main catalog patch adding `dpu_12_4_eliza.h` (365 lines). It defines the DPU v12.4 hardware blocks. Observations:

**Bug -- `.cwb` array mismatch:**
```c
.cwb_count = ARRAY_SIZE(eliza_cwb),
.cwb = sm8650_cwb,
```
The local `eliza_cwb[]` array is defined with 4 entries having `.len = 0x20`, but the config struct points to `sm8650_cwb` whose entries have `.len = 0x8`. This means the `eliza_cwb` array data is defined but never actually used. This is a pre-existing pattern -- SM8750, Glymur, and Kaanapali catalogs all exhibit the same mismatch (they all define local CWB arrays but point `.cwb` at `sm8650_cwb`). This should ideally be `.cwb = eliza_cwb` to use the correct register lengths. **However**, since this mirrors existing upstream code, this might be intentional or a known issue being tracked separately. Worth a comment from the maintainers.

**DSC with unpaired dce_1:**
```c
static const struct dpu_dsc_cfg eliza_dsc[] = {
    { .name = "dce_0_0", .id = DSC_0, .base = 0x80000, ... .sblk = &sm8750_dsc_sblk_0, },
    { .name = "dce_0_1", .id = DSC_1, .base = 0x80000, ... .sblk = &sm8750_dsc_sblk_1, },
    { .name = "dce_1_0", .id = DSC_2, .base = 0x81000, ... .sblk = &sm8750_dsc_sblk_0, },
};
```
DCE 0 has both hard slices (DSC_0 + DSC_1) but DCE 1 only has one hard slice (DSC_2 without DSC_3). The comment says "Each display compression engine (DCE) contains dual hard slice DSC encoders," yet dce_1 only has one. If this matches the actual hardware, the comment inherited from SM8750 is now slightly misleading. If the hardware actually has the second slice, it should be included.

**Unverified MDP base/len:**
```c
static const struct dpu_mdp_cfg eliza_mdp = {
    .name = "top_0",
    .base = 0, .len = 0x494, // not verified
```
The `// not verified` comment is honest, but for a production patch this should ideally be verified before merging.

**Perf data uses sc7180 QoS tables:**
```c
.qos_lut_tbl = {
    {.nentry = ARRAY_SIZE(sc7180_qos_linear),
     .entries = sc7180_qos_linear },
    ...
    /* TODO: macrotile-qseed is different from macrotile */
},
```
The TODO comment and reuse of sc7180 QoS tables is consistent with what SM8750 does, so this is fine for initial bringup.

**LM_3 has no DSPP:**
```c
{ .name = "lm_3", .id = LM_3,
  .base = 0x47000, .len = 0x400,
  ...
  .pingpong = PINGPONG_3,
},  // no .dspp field
```
While this is consistent with the cover letter saying DSPP3 was removed (only DSPP 0-2 remain), LM_3 having no DSPP assignment will limit its functionality. This appears intentional.

The remaining catalog additions (`dpu_hw_catalog.c`, `dpu_hw_catalog.h`, `dpu_kms.c`) are mechanical one-liner additions that look correct.

---
Generated by Claude Code Patch Reviewer