[PATCH] drm/msm/dp: add EDID re-read retry in msm_dp_panel_get_modes()

[PATCH] drm/msm/dp: add EDID re-read retry in msm_dp_panel_get_modes()

[Jens Glathe via B4 Relay]

From: Jens Glathe <jens.glathe@oldschoolsolutions.biz>

drm_edid_read_ddc() can return a structurally valid EDID from which
drm_edid_connector_add_modes() still returns zero modes. This triggers
the error:

  [drm:msm_dp_bridge_get_modes [msm]] *ERROR* failed to get DP sink
   modes, rc=0

even though the link is ready. Since the EDID is only read once, this
error persists and the display comes up with 640x480 resolution.

Add a retry of drm_edid_read_ddc() inside get_modes() when the initial
read produces no usable modes. The bad EDID is freed before retrying
and container_of ensures access to the DDC channel. This directly
addresses the observed "valid but empty/garbage" EDID case on
flaky DP plugs and adapters.

I tested this on a few of my "flaky" type-c to HDMI adapters and hubs,
getting no "retry failed" messages and the desired resolution. Without
the patch most plugs would result in 640x480 external display.

Fixes: [5bea90ad9743d2] "drm/msm/dp: switch to struct drm_edid"

Signed-off-by: Jens Glathe <jens.glathe@oldschoolsolutions.biz>
---
 drivers/gpu/drm/msm/dp/dp_panel.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
index 6bb021820d7c5..4f27c8129b0ef 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -327,11 +327,39 @@ u32 msm_dp_panel_get_mode_bpp(struct msm_dp_panel *msm_dp_panel,
 int msm_dp_panel_get_modes(struct msm_dp_panel *msm_dp_panel,
 	struct drm_connector *connector)
 {
+	int modes;
+	int retry;
+	struct msm_dp_panel_private *panel;
+
 	if (!msm_dp_panel) {
 		DRM_ERROR("invalid input\n");
 		return -EINVAL;
 	}
 
+	panel = container_of(msm_dp_panel, struct msm_dp_panel_private, msm_dp_panel);
+
+	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;
+	}
+
+	for (retry = 0; retry < 5; retry++) {
+		usleep_range(20000, 25000);
+		msm_dp_panel->drm_edid =
+			drm_edid_read_ddc(connector, &panel->aux->ddc);
+		if (msm_dp_panel->drm_edid)
+			break;
+
+		drm_dbg_dp(connector->dev,
+			   "get_modes re-read attempt %d/5 failed\n",
+			   retry + 1);
+	}
+
+	drm_edid_connector_update(connector, msm_dp_panel->drm_edid);
 	if (msm_dp_panel->drm_edid)
 		return drm_edid_connector_add_modes(connector);
 

---
base-commit: 7da7f07112610a520567421dd2ffcb51beaefbcc
change-id: 20260531-drm_plug_flaky_edid-cc7743f6f909

Best regards,
-- 
Jens Glathe <jens.glathe@oldschoolsolutions.biz>

Re: [PATCH] drm/msm/dp: add EDID re-read retry in msm_dp_panel_get_modes()

[Jani Nikula]

On Sun, 31 May 2026, Jens Glathe via B4 Relay <devnull+jens.glathe.oldschoolsolutions.biz@kernel.org> wrote:
> From: Jens Glathe <jens.glathe@oldschoolsolutions.biz>
>
> drm_edid_read_ddc() can return a structurally valid EDID from which
> drm_edid_connector_add_modes() still returns zero modes. This triggers
> the error:

What are the EDID contents in that case?

Where does it originate from? A valid but "not what you wanted" EDID
doesn't show up from thin air.

Perhaps it's the hub/adapter replying with some placeholder EDID instead
of the actual sink EDID. Maybe it's the timing and/or ordering.

Feels like root causing would be in order before adding hacks like this
that are notoriously difficult to remove in the future, and will easily
block future progress.


BR,
Jani.


>
>   [drm:msm_dp_bridge_get_modes [msm]] *ERROR* failed to get DP sink
>    modes, rc=0
>
> even though the link is ready. Since the EDID is only read once, this
> error persists and the display comes up with 640x480 resolution.
>
> Add a retry of drm_edid_read_ddc() inside get_modes() when the initial
> read produces no usable modes. The bad EDID is freed before retrying
> and container_of ensures access to the DDC channel. This directly
> addresses the observed "valid but empty/garbage" EDID case on
> flaky DP plugs and adapters.
>
> I tested this on a few of my "flaky" type-c to HDMI adapters and hubs,
> getting no "retry failed" messages and the desired resolution. Without
> the patch most plugs would result in 640x480 external display.
>
> Fixes: [5bea90ad9743d2] "drm/msm/dp: switch to struct drm_edid"
>
> Signed-off-by: Jens Glathe <jens.glathe@oldschoolsolutions.biz>
> ---
>  drivers/gpu/drm/msm/dp/dp_panel.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
> index 6bb021820d7c5..4f27c8129b0ef 100644
> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
> @@ -327,11 +327,39 @@ u32 msm_dp_panel_get_mode_bpp(struct msm_dp_panel *msm_dp_panel,
>  int msm_dp_panel_get_modes(struct msm_dp_panel *msm_dp_panel,
>  	struct drm_connector *connector)
>  {
> +	int modes;
> +	int retry;
> +	struct msm_dp_panel_private *panel;
> +
>  	if (!msm_dp_panel) {
>  		DRM_ERROR("invalid input\n");
>  		return -EINVAL;
>  	}
>  
> +	panel = container_of(msm_dp_panel, struct msm_dp_panel_private, msm_dp_panel);
> +
> +	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;
> +	}
> +
> +	for (retry = 0; retry < 5; retry++) {
> +		usleep_range(20000, 25000);
> +		msm_dp_panel->drm_edid =
> +			drm_edid_read_ddc(connector, &panel->aux->ddc);
> +		if (msm_dp_panel->drm_edid)
> +			break;
> +
> +		drm_dbg_dp(connector->dev,
> +			   "get_modes re-read attempt %d/5 failed\n",
> +			   retry + 1);
> +	}
> +
> +	drm_edid_connector_update(connector, msm_dp_panel->drm_edid);
>  	if (msm_dp_panel->drm_edid)
>  		return drm_edid_connector_add_modes(connector);
>  
>
> ---
> base-commit: 7da7f07112610a520567421dd2ffcb51beaefbcc
> change-id: 20260531-drm_plug_flaky_edid-cc7743f6f909
>
> Best regards,

-- 
Jani Nikula, Intel

Re: [PATCH] drm/msm/dp: add EDID re-read retry in msm_dp_panel_get_modes()

[Jani Nikula]

On Sun, 31 May 2026, Jens Glathe via B4 Relay <devnull+jens.glathe.oldschoolsolutions.biz@kernel.org> wrote:
> From: Jens Glathe <jens.glathe@oldschoolsolutions.biz>
>
> drm_edid_read_ddc() can return a structurally valid EDID from which
> drm_edid_connector_add_modes() still returns zero modes. This triggers
> the error:
>
>   [drm:msm_dp_bridge_get_modes [msm]] *ERROR* failed to get DP sink
>    modes, rc=0
>
> even though the link is ready. Since the EDID is only read once, this
> error persists and the display comes up with 640x480 resolution.
>
> Add a retry of drm_edid_read_ddc() inside get_modes() when the initial
> read produces no usable modes. The bad EDID is freed before retrying
> and container_of ensures access to the DDC channel. This directly
> addresses the observed "valid but empty/garbage" EDID case on
> flaky DP plugs and adapters.
>
> I tested this on a few of my "flaky" type-c to HDMI adapters and hubs,
> getting no "retry failed" messages and the desired resolution. Without
> the patch most plugs would result in 640x480 external display.
>
> Fixes: [5bea90ad9743d2] "drm/msm/dp: switch to struct drm_edid"

Did you actually bisect to this commit? It's a bit of a surprise.

Side note, the Fixes: trailer has a strict format. Please look at git
logs for what it should be like.

>
> Signed-off-by: Jens Glathe <jens.glathe@oldschoolsolutions.biz>
> ---
>  drivers/gpu/drm/msm/dp/dp_panel.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
> index 6bb021820d7c5..4f27c8129b0ef 100644
> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
> @@ -327,11 +327,39 @@ u32 msm_dp_panel_get_mode_bpp(struct msm_dp_panel *msm_dp_panel,
>  int msm_dp_panel_get_modes(struct msm_dp_panel *msm_dp_panel,
>  	struct drm_connector *connector)
>  {
> +	int modes;
> +	int retry;
> +	struct msm_dp_panel_private *panel;
> +
>  	if (!msm_dp_panel) {
>  		DRM_ERROR("invalid input\n");
>  		return -EINVAL;
>  	}
>  
> +	panel = container_of(msm_dp_panel, struct msm_dp_panel_private, msm_dp_panel);
> +
> +	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;
> +	}
> +
> +	for (retry = 0; retry < 5; retry++) {
> +		usleep_range(20000, 25000);
> +		msm_dp_panel->drm_edid =
> +			drm_edid_read_ddc(connector, &panel->aux->ddc);
> +		if (msm_dp_panel->drm_edid)
> +			break;
> +
> +		drm_dbg_dp(connector->dev,
> +			   "get_modes re-read attempt %d/5 failed\n",
> +			   retry + 1);
> +	}
> +
> +	drm_edid_connector_update(connector, msm_dp_panel->drm_edid);
>  	if (msm_dp_panel->drm_edid)
>  		return drm_edid_connector_add_modes(connector);
>  
>
> ---
> base-commit: 7da7f07112610a520567421dd2ffcb51beaefbcc
> change-id: 20260531-drm_plug_flaky_edid-cc7743f6f909
>
> Best regards,

-- 
Jani Nikula, Intel

Re: [PATCH] drm/msm/dp: add EDID re-read retry in msm_dp_panel_get_modes()

[Jens Glathe]

On 01.06.26 12:57, Jani Nikula wrote:
> Did you actually bisect to this commit? It's a bit of a surprise.
Hi Jani,

haha yes, sort of. I did a few weeks of bisecting for a different issue 
[1] and this error message came up increasingly, so after resolving (or 
so) the first one I took a shot at it. The commit was the result of git 
blame for the line that the retry was programmed around. And as v2 
shows, it was the right intuition.

[1]: 
https://lore.kernel.org/all/20260530-typc-mux-modeset-v1-1-64b0281e2cd6@oldschoolsolutions.biz/

with best regards

Jens

Re: [PATCH] drm/msm/dp: add EDID re-read retry in msm_dp_panel_get_modes()

[Dmitry Baryshkov]

On Sun, May 31, 2026 at 11:16:51PM +0200, Jens Glathe via B4 Relay wrote:
> From: Jens Glathe <jens.glathe@oldschoolsolutions.biz>
> 
> drm_edid_read_ddc() can return a structurally valid EDID from which

When?

> drm_edid_connector_add_modes() still returns zero modes. This triggers
> the error:

Please at least add EDID contents.

> 
>   [drm:msm_dp_bridge_get_modes [msm]] *ERROR* failed to get DP sink
>    modes, rc=0
> 
> even though the link is ready. Since the EDID is only read once, this
> error persists and the display comes up with 640x480 resolution.

Please check with the current next tree, we have reworked HPD handling a
bit. Also, it might be the case that the dongle sends a placeholder
first and then sends a HPD or IRQ_HPD event. Please check if that's the
case.

> 
> Add a retry of drm_edid_read_ddc() inside get_modes() when the initial
> read produces no usable modes. The bad EDID is freed before retrying
> and container_of ensures access to the DDC channel. This directly
> addresses the observed "valid but empty/garbage" EDID case on
> flaky DP plugs and adapters.
> 
> I tested this on a few of my "flaky" type-c to HDMI adapters and hubs,
> getting no "retry failed" messages and the desired resolution. Without
> the patch most plugs would result in 640x480 external display.
> 
> Fixes: [5bea90ad9743d2] "drm/msm/dp: switch to struct drm_edid"

Hard to believe.

> 
> Signed-off-by: Jens Glathe <jens.glathe@oldschoolsolutions.biz>
> ---
>  drivers/gpu/drm/msm/dp/dp_panel.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 

-- 
With best wishes
Dmitry

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

[Claude Code Review Bot]

Overall Series Review

Subject: drm/msm/dp: add EDID re-read retry in msm_dp_panel_get_modes()
Author: Jens Glathe via B4 Relay
 <devnull+jens.glathe.oldschoolsolutions.biz@kernel.org>
Patches: 5
Reviewed: 2026-06-04T14:42:15.755995

---

This is a single patch that adds EDID re-read retry logic to `msm_dp_panel_get_modes()` to handle the case where the initial EDID read (done in `msm_dp_panel_read_sink_caps()` during HPD) returns a structurally valid but content-deficient EDID that yields zero modes.

The problem being addressed is real — flaky USB-C/DP adapters can produce garbage EDIDs, and retrying the read is a reasonable workaround. However, the implementation has several significant issues with correctness, placement, and missing checks.

**Verdict: Needs rework.** The retry logic belongs in `msm_dp_panel_read_sink_caps()` where the EDID is originally read, not in `get_modes()`. The current patch also has bugs around the zero-modes-from-re-read path and introduces sleeping in a callback context that may not be appropriate.

---

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

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