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/msm/dp: Add MST support for MSM chipsets Date: Sun, 12 Apr 2026 10:16:11 +1000 Message-ID: In-Reply-To: <20260410-msm-dp-mst-v4-0-b20518dea8de@oss.qualcomm.com> References: <20260410-msm-dp-mst-v4-0-b20518dea8de@oss.qualcomm.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Overall Series Review Subject: drm/msm/dp: Add MST support for MSM chipsets Author: Yongxing Mou Patches: 66 Reviewed: 2026-04-12T10:16:11.494271 --- This 39-patch series adds Multi-Stream Transport (MST) support to the Qualc= omm MSM DP driver, enabling a single DisplayPort controller to drive up to = 4 independent displays. The series is structured as: prerequisite fixes (pa= tches 1-4), DP driver refactoring to support multi-stream abstractions (pat= ches 5-28), sink-side MST initialization (patch 29), new MST panel/controll= er helpers (patches 30-32), and the core MST DRM bridge module (patches 33-= 39). **Architecture**: The approach is sound =E2=80=94 the series correctly uses= `drm_dp_mst_topology_mgr` for topology management, `drm_private_obj` for p= er-bridge atomic state, and the standard part1/part2 payload allocation flo= w. The separation of link-level operations (`prepare`/`unprepare`) from str= eam-level operations (`enable`/`disable`) is well-designed for MST where mu= ltiple streams share a single trained link. **Concerns**: 1. **Bug in patch 30**: `IS_ERR(dp->panel)` checks the wrong variable =E2= =80=94 should be `IS_ERR(dp_panel)`. This means a failed panel allocation w= ill dereference an error pointer. 2. **State access issue in patch 34**: `atomic_check` uses `to_drm_dp_mst_t= opology_state(mst->mst_mgr.base.state)` which reads the *current committed*= state rather than the new atomic state being validated. Should use `drm_at= omic_get_mst_topology_state()` to get the state from the atomic transaction. 3. **Memory leak in patch 01**: `drm_edid_read_ddc()` result is never freed= , and the old cleanup path (`drm_edid_free`) was removed. 4. **Hardcoded 100ms sleep in patch 29** for clearing MST state is excessiv= e and not tied to any spec requirement. 5. **Missing error propagation**: Several functions (e.g. `msm_dp_mst_bridg= e_atomic_check`) compute `slots` from `drm_dp_atomic_find_time_slots()` but= never check if `slots < 0` before returning success. 6. **Series ordering**: Some patches could be squashed (e.g., patches 13-14= both touch stream_id) and the series is long enough that reviewability suf= fers. The 39-patch count could be reduced. Overall: The series needs at least the IS_ERR bug fix and the atomic_check = state access fix before it can be merged. The architecture and general appr= oach are correct. --- --- Generated by Claude Code Patch Reviewer