Claude review: drm/msm/dp: add EDID re-read retry in msm_dp_panel_get_modes()

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**1. Wrong location for the retry — 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 — `get_modes()` is 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 × 25ms) in the bridge's `get_modes` callback.
- If the problem is that `drm_edid_read_ddc()` returns a bad EDID, the natural 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_add_modes()`. The retry loop only checks whether `drm_edid_read_ddc()` returned non-NULL — it never checks whether the re-read EDID actually produces modes. So you can retry 5 times, get the same "valid but useless" EDID each time, break out of the loop on the first iteration, and still get zero modes. The retry doesn't fix the scenario described in the commit message.

The retry loop should be:

```
for each retry:
    re-read EDID
    if (EDID is non-NULL) {
        update connector
        modes = 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_edid_connector_update()`**

At the top of the patched function:

```c
if (msm_dp_panel->drm_edid) {
    modes = drm_edid_connector_add_modes(connector);
    if (modes > 0)
        return modes;

    drm_edid_free(msm_dp_panel->drm_edid);
    msm_dp_panel->drm_edid = NULL;
}
```

The original code (line 327-328) just does `return drm_edid_connector_add_modes(connector)` — the `drm_edid_connector_update()` was already called 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 succeeds, `drm_edid_connector_update()` is properly called on line 157. This part is okay, but worth noting that the first `add_modes()` call relies on connector 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_edid_free()` already (line 277, 755), so this matches the existing pattern. However, note that after freeing:

```c
drm_edid_free(msm_dp_panel->drm_edid);
msm_dp_panel->drm_edid = 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 the 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 <= 0` as 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 fails.

**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 acceptable, but it's worth questioning whether this is called with any locks held (e.g., `connector->dev->mode_config.mutex`). The `usleep_range` is fine for 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 µs) are unexplained. What is the rationale for 5 retries? What is the minimum delay needed for 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 kernel 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 — the single-read-no-retry behavior predates it. This Fixes tag would inappropriately limit the scope of stable backports.

**Summary:** The patch addresses a real problem but the retry logic is in the wrong function and is incomplete — it doesn't actually verify the re-read EDID produces modes, which was the entire point. It should be reworked to either (a) move the retry into `msm_dp_panel_read_sink_caps()` with a mode-count check there, or (b) at minimum fix the retry loop in `get_modes()` to verify the retried EDID actually yields modes before accepting it.

---
Generated by Claude Code Patch Reviewer