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 EDID re-read retry in msm_dp_panel_get_modes() Date: Thu, 04 Jun 2026 14:42:15 +1000 Message-ID: In-Reply-To: <20260531-drm_plug_flaky_edid-v1-1-2708c16dce97@oldschoolsolutions.biz> References: <20260531-drm_plug_flaky_edid-v1-1-2708c16dce97@oldschoolsolutions.biz> <20260531-drm_plug_flaky_edid-v1-1-2708c16dce97@oldschoolsolutions.biz> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **1. Wrong location for the retry =E2=80=94 should be in `msm_dp_panel_read= _sink_caps()`** The EDID is originally read in `msm_dp_panel_read_sink_caps()` (dp_panel.c:= 277-281) during HPD handling. That function already does the `drm_edid_read= _ddc()` + `drm_edid_connector_update()` sequence. Putting the retry in `get= _modes()` means: - The retry is architecturally in the wrong layer =E2=80=94 `get_modes()` i= s a DRM bridge callback called during connector probing, potentially called= multiple times. It should be a simple consumer of already-read EDID data. - The fix could be called by userspace via repeated `GetConnector` ioctls, = each time sleeping up to 125ms (5 =C3=97 25ms) in the bridge's `get_modes` = callback. - If the problem is that `drm_edid_read_ddc()` returns a bad EDID, the natu= ral place to retry is right after the initial read in `read_sink_caps()`. **2. Missing mode count check on re-read** After the retry loop successfully reads a new EDID, the code falls through = to: ```c drm_edid_connector_update(connector, msm_dp_panel->drm_edid); if (msm_dp_panel->drm_edid) return drm_edid_connector_add_modes(connector); ``` But the whole point of this patch is that `drm_edid_read_ddc()` can return = a structurally valid EDID that yields zero modes from `drm_edid_connector_a= dd_modes()`. The retry loop only checks whether `drm_edid_read_ddc()` retur= ned non-NULL =E2=80=94 it never checks whether the re-read EDID actually pr= oduces modes. So you can retry 5 times, get the same "valid but useless" ED= ID each time, break out of the loop on the first iteration, and still get z= ero modes. The retry doesn't fix the scenario described in the commit messa= ge. The retry loop should be: ``` for each retry: re-read EDID if (EDID is non-NULL) { update connector modes =3D add_modes() if (modes > 0) return modes; // else free the bad EDID and try again } ``` **3. Initial `drm_edid_connector_add_modes()` called without prior `drm_edi= d_connector_update()`** At the top of the patched function: ```c if (msm_dp_panel->drm_edid) { modes =3D drm_edid_connector_add_modes(connector); if (modes > 0) return modes; drm_edid_free(msm_dp_panel->drm_edid); msm_dp_panel->drm_edid =3D NULL; } ``` The original code (line 327-328) just does `return drm_edid_connector_add_m= odes(connector)` =E2=80=94 the `drm_edid_connector_update()` was already ca= lled in `read_sink_caps()`. This new code correctly relies on that earlier = update. However, when the `drm_edid_free()` path is taken and a re-read suc= ceeds, `drm_edid_connector_update()` is properly called on line 157. This p= art is okay, but worth noting that the first `add_modes()` call relies on c= onnector state set up elsewhere. **4. `drm_edid_free()` called on pointer from `msm_dp_panel` but this is a = `const struct drm_edid *`** Looking at the existing code, `msm_dp_panel->drm_edid` is used with `drm_ed= id_free()` already (line 277, 755), so this matches the existing pattern. H= owever, note that after freeing: ```c drm_edid_free(msm_dp_panel->drm_edid); msm_dp_panel->drm_edid =3D NULL; ``` ...if all 5 retries fail (all return NULL), we fall through to: ```c drm_edid_connector_update(connector, msm_dp_panel->drm_edid); ``` This calls `drm_edid_connector_update(connector, NULL)` which will clear th= e connector's EDID info. This is actually reasonable behavior, but combined= with the original code's `return 0` fallthrough (when `drm_edid` is NULL),= the caller `msm_dp_bridge_get_modes()` at dp_drm.c:87 treats `rc <=3D 0` a= s an error and logs the "failed to get DP sink modes" error anyway. So the = retry doesn't suppress the error message from the caller if it ultimately f= ails. **5. Sleeping in `get_modes()` callback** ```c usleep_range(20000, 25000); ``` The `get_modes` bridge callback can be called from the DRM core's connector= probing path. Sleeping 20-25ms per retry (up to ~125ms total) may be accep= table, but it's worth questioning whether this is called with any locks hel= d (e.g., `connector->dev->mode_config.mutex`). The `usleep_range` is fine f= or process context but the concern is holding a mutex for 125ms while doing= I2C retries. **6. Magic numbers** The retry count (5) and sleep duration (20000-25000 =C2=B5s) are unexplaine= d. What is the rationale for 5 retries? What is the minimum delay needed fo= r the adapter to settle? These should at minimum be named constants with a = brief comment, and ideally justified in the commit message. **7. Fixes tag format** ``` Fixes: [5bea90ad9743d2] "drm/msm/dp: switch to struct drm_edid" ``` The Fixes tag has the commit hash in brackets, which is not the standard ke= rnel format. It should be: ``` Fixes: 5bea90ad9743 ("drm/msm/dp: switch to struct drm_edid") ``` Also, this Fixes tag claims the bug was introduced by the `struct drm_edid`= conversion, but the underlying issue (flaky adapters producing bad EDIDs) = likely existed before that conversion too. The conversion just changed the = mechanism =E2=80=94 the single-read-no-retry behavior predates it. This Fix= es tag would inappropriately limit the scope of stable backports. **Summary:** The patch addresses a real problem but the retry logic is in t= he wrong function and is incomplete =E2=80=94 it doesn't actually verify th= e re-read EDID produces modes, which was the entire point. It should be rew= orked to either (a) move the retry into `msm_dp_panel_read_sink_caps()` wit= h a mode-count check there, or (b) at minimum fix the retry loop in `get_mo= des()` to verify the retried EDID actually yields modes before accepting it. --- Generated by Claude Code Patch Reviewer