[PATCH NEXT 0/2] drm/radeon/radeon_connectors: use struct drm_edid

[PATCH NEXT 0/2] drm/radeon/radeon_connectors: use struct drm_edid

[Joshua Peisach]

This is the equivalent of what was done with amdgpu[1], using struct
drm_edid instead of struct edid.

The radeon_connector_free_edid function can also be removed since we
can call drm_edid_free (unlike last time, I made sure to set the pointer
to NULL afterwards).

[1]: https://lore.kernel.org/amd-gfx/20260303211823.76631-1-jpeisach@ubuntu.com/

Joshua Peisach (2):
  drm/radeon/radeon_connectors: use struct drm_edid instead of struct
    edid
  drm/radeon/radeon_connectors: remove radeon_connector_free_edid

 drivers/gpu/drm/radeon/radeon_audio.c      |  4 +-
 drivers/gpu/drm/radeon/radeon_combios.c    |  4 +-
 drivers/gpu/drm/radeon/radeon_connectors.c | 60 +++++++++++-----------
 drivers/gpu/drm/radeon/radeon_mode.h       |  4 +-
 4 files changed, 36 insertions(+), 36 deletions(-)

-- 
2.53.0

[PATCH NEXT 1/2] drm/radeon/radeon_connectors: use struct drm_edid instead of struct edid

[Joshua Peisach]

This was done with amdgpu, just bringing the same patch to radeon.

The goal of this is to stop using the deprecated edid functions,
specifically drm_connector_update_edid_property. Switch to struct
drm_edid and the appropriate function replacements for the new type.

Also, for audio, use the raw edid for SADB allocations and for
equivalent drm_edid_is_digital expressions.

Signed-off-by: Joshua Peisach <jpeisach@ubuntu.com>
---
 drivers/gpu/drm/radeon/radeon_audio.c      |  4 +--
 drivers/gpu/drm/radeon/radeon_combios.c    |  4 +--
 drivers/gpu/drm/radeon/radeon_connectors.c | 30 +++++++++++-----------
 drivers/gpu/drm/radeon/radeon_mode.h       |  4 +--
 4 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_audio.c b/drivers/gpu/drm/radeon/radeon_audio.c
index 8d64ba18572e..52d0049738b1 100644
--- a/drivers/gpu/drm/radeon/radeon_audio.c
+++ b/drivers/gpu/drm/radeon/radeon_audio.c
@@ -311,7 +311,7 @@ static void radeon_audio_write_sad_regs(struct drm_encoder *encoder)
 	if (!connector)
 		return;
 
-	sad_count = drm_edid_to_sad(radeon_connector->edid, &sads);
+	sad_count = drm_edid_to_sad(drm_edid_raw(radeon_connector->edid), &sads);
 	if (sad_count < 0)
 		DRM_ERROR("Couldn't read SADs: %d\n", sad_count);
 	if (sad_count <= 0)
@@ -335,7 +335,7 @@ static void radeon_audio_write_speaker_allocation(struct drm_encoder *encoder)
 	if (!connector)
 		return;
 
-	sad_count = drm_edid_to_speaker_allocation(radeon_connector->edid, &sadb);
+	sad_count = drm_edid_to_speaker_allocation(drm_edid_raw(radeon_connector->edid), &sadb);
 	if (sad_count < 0) {
 		DRM_DEBUG("Couldn't read Speaker Allocation Data Block: %d\n",
 			  sad_count);
diff --git a/drivers/gpu/drm/radeon/radeon_combios.c b/drivers/gpu/drm/radeon/radeon_combios.c
index df8d7f56b028..f4947acd0419 100644
--- a/drivers/gpu/drm/radeon/radeon_combios.c
+++ b/drivers/gpu/drm/radeon/radeon_combios.c
@@ -390,10 +390,10 @@ bool radeon_combios_check_hardcoded_edid(struct radeon_device *rdev)
 }
 
 /* this is used for atom LCDs as well */
-struct edid *
+const struct drm_edid *
 radeon_bios_get_hardcoded_edid(struct radeon_device *rdev)
 {
-	return drm_edid_duplicate(drm_edid_raw(rdev->mode_info.bios_hardcoded_edid));
+	return drm_edid_dup(rdev->mode_info.bios_hardcoded_edid);
 }
 
 static struct radeon_i2c_bus_rec combios_setup_i2c_bus(struct radeon_device *rdev,
diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c
index 012d8b2295b8..d3a2efbcfd0d 100644
--- a/drivers/gpu/drm/radeon/radeon_connectors.c
+++ b/drivers/gpu/drm/radeon/radeon_connectors.c
@@ -271,8 +271,8 @@ static void radeon_connector_get_edid(struct drm_connector *connector)
 	if ((radeon_connector_encoder_get_dp_bridge_encoder_id(connector) !=
 	     ENCODER_OBJECT_ID_NONE) &&
 	    radeon_connector->ddc_bus->has_aux) {
-		radeon_connector->edid = drm_get_edid(connector,
-						      &radeon_connector->ddc_bus->aux.ddc);
+		radeon_connector->edid = drm_edid_read_ddc(connector,
+							   &radeon_connector->ddc_bus->aux.ddc);
 	} else if ((connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort) ||
 		   (connector->connector_type == DRM_MODE_CONNECTOR_eDP)) {
 		struct radeon_connector_atom_dig *dig = radeon_connector->con_priv;
@@ -280,19 +280,19 @@ static void radeon_connector_get_edid(struct drm_connector *connector)
 		if ((dig->dp_sink_type == CONNECTOR_OBJECT_ID_DISPLAYPORT ||
 		     dig->dp_sink_type == CONNECTOR_OBJECT_ID_eDP) &&
 		    radeon_connector->ddc_bus->has_aux)
-			radeon_connector->edid = drm_get_edid(&radeon_connector->base,
-							      &radeon_connector->ddc_bus->aux.ddc);
+			radeon_connector->edid = drm_edid_read_ddc(&radeon_connector->base,
+								   &radeon_connector->ddc_bus->aux.ddc);
 		else if (radeon_connector->ddc_bus)
-			radeon_connector->edid = drm_get_edid(&radeon_connector->base,
-							      &radeon_connector->ddc_bus->adapter);
+			radeon_connector->edid = drm_edid_read_ddc(&radeon_connector->base,
+								   &radeon_connector->ddc_bus->adapter);
 	} else if (vga_switcheroo_handler_flags() & VGA_SWITCHEROO_CAN_SWITCH_DDC &&
 		   connector->connector_type == DRM_MODE_CONNECTOR_LVDS &&
 		   radeon_connector->ddc_bus) {
-		radeon_connector->edid = drm_get_edid_switcheroo(&radeon_connector->base,
-								 &radeon_connector->ddc_bus->adapter);
+		radeon_connector->edid = drm_edid_read_switcheroo(&radeon_connector->base,
+								  &radeon_connector->ddc_bus->adapter);
 	} else if (radeon_connector->ddc_bus) {
-		radeon_connector->edid = drm_get_edid(&radeon_connector->base,
-						      &radeon_connector->ddc_bus->adapter);
+		radeon_connector->edid = drm_edid_read_ddc(&radeon_connector->base,
+							   &radeon_connector->ddc_bus->adapter);
 	}
 
 	if (!radeon_connector->edid) {
@@ -328,11 +328,11 @@ static int radeon_ddc_get_modes(struct drm_connector *connector)
 	int ret;
 
 	if (radeon_connector->edid) {
-		drm_connector_update_edid_property(connector, radeon_connector->edid);
-		ret = drm_add_edid_modes(connector, radeon_connector->edid);
+		drm_edid_connector_update(connector, radeon_connector->edid);
+		ret = drm_edid_connector_add_modes(connector);
 		return ret;
 	}
-	drm_connector_update_edid_property(connector, NULL);
+	drm_edid_connector_update(connector, NULL);
 	return 0;
 }
 
@@ -1016,7 +1016,7 @@ radeon_vga_detect(struct drm_connector *connector, bool force)
 			ret = connector_status_connected;
 		} else {
 			radeon_connector->use_digital =
-				!!(radeon_connector->edid->input & DRM_EDID_INPUT_DIGITAL);
+				drm_edid_is_digital(radeon_connector->edid);
 
 			/* some oems have boards with separate digital and analog connectors
 			 * with a shared ddc line (often vga + hdmi)
@@ -1271,7 +1271,7 @@ radeon_dvi_detect(struct drm_connector *connector, bool force)
 			}
 		} else {
 			radeon_connector->use_digital =
-				!!(radeon_connector->edid->input & DRM_EDID_INPUT_DIGITAL);
+				drm_edid_is_digital(radeon_connector->edid);
 
 			/* some oems have boards with separate digital and analog connectors
 			 * with a shared ddc line (often vga + hdmi)
diff --git a/drivers/gpu/drm/radeon/radeon_mode.h b/drivers/gpu/drm/radeon/radeon_mode.h
index ae1ecdc2e189..031a3bf6fe0a 100644
--- a/drivers/gpu/drm/radeon/radeon_mode.h
+++ b/drivers/gpu/drm/radeon/radeon_mode.h
@@ -525,7 +525,7 @@ struct radeon_connector {
 	bool use_digital;
 	/* we need to mind the EDID between detect
 	   and get modes due to analog/digital/tvencoder */
-	struct edid *edid;
+	const struct drm_edid *edid;
 	void *con_priv;
 	bool dac_load_detect;
 	bool detected_by_load; /* if the connection status was determined by load */
@@ -839,7 +839,7 @@ radeon_get_crtc_scanout_position(struct drm_crtc *crtc, bool in_vblank_irq,
 				 const struct drm_display_mode *mode);
 
 extern bool radeon_combios_check_hardcoded_edid(struct radeon_device *rdev);
-extern struct edid *
+extern const struct drm_edid *
 radeon_bios_get_hardcoded_edid(struct radeon_device *rdev);
 extern bool radeon_atom_get_clock_info(struct drm_device *dev);
 extern bool radeon_combios_get_clock_info(struct drm_device *dev);
-- 
2.53.0

Re: [PATCH NEXT 1/2] drm/radeon/radeon_connectors: use struct drm_edid instead of struct edid

[Joshua Peisach]

On Sat May 23, 2026 at 10:27 AM EDT, Joshua Peisach wrote:
> @@ -280,19 +280,19 @@ static void radeon_connector_get_edid(struct drm_connector *connector)
>  		if ((dig->dp_sink_type == CONNECTOR_OBJECT_ID_DISPLAYPORT ||
>  		     dig->dp_sink_type == CONNECTOR_OBJECT_ID_eDP) &&
>  		    radeon_connector->ddc_bus->has_aux)
> -			radeon_connector->edid = drm_get_edid(&radeon_connector->base,
> -							      &radeon_connector->ddc_bus->aux.ddc);
> +			radeon_connector->edid = drm_edid_read_ddc(&radeon_connector->base,
> +								   &radeon_connector->ddc_bus->aux.ddc);
>  		else if (radeon_connector->ddc_bus)
> -			radeon_connector->edid = drm_get_edid(&radeon_connector->base,
> -							      &radeon_connector->ddc_bus->adapter);
> +			radeon_connector->edid = drm_edid_read_ddc(&radeon_connector->base,
> +								   &radeon_connector->ddc_bus->adapter);
>  	} else if (vga_switcheroo_handler_flags() & VGA_SWITCHEROO_CAN_SWITCH_DDC &&
>  		   connector->connector_type == DRM_MODE_CONNECTOR_LVDS &&
>  		   radeon_connector->ddc_bus) {
> -		radeon_connector->edid = drm_get_edid_switcheroo(&radeon_connector->base,
> -								 &radeon_connector->ddc_bus->adapter);
> +		radeon_connector->edid = drm_edid_read_switcheroo(&radeon_connector->base,
> +								  &radeon_connector->ddc_bus->adapter);
>  	} else if (radeon_connector->ddc_bus) {
> -		radeon_connector->edid = drm_get_edid(&radeon_connector->base,
> -						      &radeon_connector->ddc_bus->adapter);
> +		radeon_connector->edid = drm_edid_read_ddc(&radeon_connector->base,
> +							   &radeon_connector->ddc_bus->adapter);
>  	}
>  

Question: For all of these, should we actually call with connector
instead of &radeon_connector->base?

Claude review: drm/radeon/radeon_connectors: use struct drm_edid instead of struct edid

[Claude Code Review Bot]

Patch Review

**Positive aspects:**
- The field type change in `radeon_mode.h` from `struct edid *edid` to `const struct drm_edid *edid` is correct and matches the return type of `drm_edid_read_ddc()` / `drm_edid_read_switcheroo()`.
- The `drm_edid_connector_update()` + `drm_edid_connector_add_modes()` replacement is the correct modern pattern.
- The `drm_edid_is_digital()` replacement for `!!(radeon_connector->edid->input & DRM_EDID_INPUT_DIGITAL)` is correct — `drm_edid_is_digital()` takes `const struct drm_edid *`.
- The `radeon_bios_get_hardcoded_edid()` change from `drm_edid_duplicate(drm_edid_raw(...))` to `drm_edid_dup(...)` is a clean simplification.

**Issue 1 — `radeon_connector_free_edid` left using `kfree` on `struct drm_edid`:**

After patch 1, the field type changes to `const struct drm_edid *`, but `radeon_connector_free_edid()` still calls `kfree()` on it:

```c
static void radeon_connector_free_edid(struct drm_connector *connector)
{
	struct radeon_connector *radeon_connector = to_radeon_connector(connector);

	kfree(radeon_connector->edid);
	radeon_connector->edid = NULL;
}
```

After patch 1, `radeon_connector->edid` is now a `const struct drm_edid *`, but `kfree()` is still used instead of `drm_edid_free()`. This is a **correctness bug between patches** — `struct drm_edid` is allocated by the `drm_edid_read_*` functions and contains an internal `const struct edid *edid` pointer that also needs freeing. Simply calling `kfree()` on the outer `struct drm_edid` will leak the inner EDID data. The tree must compile and be correct at every patch boundary, so patch 1 should update `radeon_connector_free_edid` to call `drm_edid_free()` or the two patches should be squashed.

Additionally, `kfree()` on a `const` pointer will generate a compiler warning/error about discarding the `const` qualifier.

**Issue 2 — `drm_edid_to_sad` / `drm_edid_to_speaker_allocation` take `const struct edid *`:**

The audio code changes are correct. These functions take `const struct edid *`, so using `drm_edid_raw()` to extract the raw EDID is the right approach:

```c
sad_count = drm_edid_to_sad(drm_edid_raw(radeon_connector->edid), &sads);
sad_count = drm_edid_to_speaker_allocation(drm_edid_raw(radeon_connector->edid), &sadb);
```

No issue here, just confirming it's correct.

**Issue 3 — `drm_edid_connector_update` return value ignored:**

In `radeon_ddc_get_modes`, the return value of `drm_edid_connector_update()` is silently discarded:

```c
	if (radeon_connector->edid) {
		drm_edid_connector_update(connector, radeon_connector->edid);
		ret = drm_edid_connector_add_modes(connector);
		return ret;
	}
	drm_edid_connector_update(connector, NULL);
```

The old `drm_connector_update_edid_property()` also returned int and was also ignored, so this is pre-existing behavior and not a regression. Just noting it.

**Nit — `drm_edid_read_ddc` vs `drm_edid_read`:**

For the non-DP, non-switcheroo, plain i2c adapter case, `drm_edid_read()` could be used instead of `drm_edid_read_ddc()` since `drm_edid_read()` is just `drm_edid_read_ddc(connector, connector->ddc)`. However, radeon sets the ddc_bus separately from the connector's `ddc` field, so using `drm_edid_read_ddc` with the explicit adapter is correct here.

---

---
Generated by Claude Code Patch Reviewer

[PATCH NEXT 2/2] drm/radeon/radeon_connectors: remove radeon_connector_free_edid

[Joshua Peisach]

Since we are using struct drm_edid, we can call drm_edid_free directly.
Also make sure to set the pointer to NULL afterwards.

Signed-off-by: Joshua Peisach <jpeisach@ubuntu.com>
---
 drivers/gpu/drm/radeon/radeon_connectors.c | 30 +++++++++++-----------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c
index d3a2efbcfd0d..da678f41823d 100644
--- a/drivers/gpu/drm/radeon/radeon_connectors.c
+++ b/drivers/gpu/drm/radeon/radeon_connectors.c
@@ -314,14 +314,6 @@ static void radeon_connector_get_edid(struct drm_connector *connector)
 	}
 }
 
-static void radeon_connector_free_edid(struct drm_connector *connector)
-{
-	struct radeon_connector *radeon_connector = to_radeon_connector(connector);
-
-	kfree(radeon_connector->edid);
-	radeon_connector->edid = NULL;
-}
-
 static int radeon_ddc_get_modes(struct drm_connector *connector)
 {
 	struct radeon_connector *radeon_connector = to_radeon_connector(connector);
@@ -895,7 +887,9 @@ static void radeon_connector_destroy(struct drm_connector *connector)
 {
 	struct radeon_connector *radeon_connector = to_radeon_connector(connector);
 
-	radeon_connector_free_edid(connector);
+	drm_edid_free(radeon_connector->edid);
+	radeon_connector->edid = NULL;
+
 	kfree(radeon_connector->con_priv);
 	drm_connector_unregister(connector);
 	drm_connector_cleanup(connector);
@@ -1007,7 +1001,8 @@ radeon_vga_detect(struct drm_connector *connector, bool force)
 		dret = radeon_ddc_probe(radeon_connector, false);
 	if (dret) {
 		radeon_connector->detected_by_load = false;
-		radeon_connector_free_edid(connector);
+		drm_edid_free(radeon_connector->edid);
+		radeon_connector->edid = NULL;
 		radeon_connector_get_edid(connector);
 
 		if (!radeon_connector->edid) {
@@ -1022,7 +1017,8 @@ radeon_vga_detect(struct drm_connector *connector, bool force)
 			 * with a shared ddc line (often vga + hdmi)
 			 */
 			if (radeon_connector->use_digital && radeon_connector->shared_ddc) {
-				radeon_connector_free_edid(connector);
+				drm_edid_free(radeon_connector->edid);
+				radeon_connector->edid = NULL;
 				ret = connector_status_disconnected;
 			} else {
 				ret = connector_status_connected;
@@ -1251,7 +1247,8 @@ radeon_dvi_detect(struct drm_connector *connector, bool force)
 	}
 	if (dret) {
 		radeon_connector->detected_by_load = false;
-		radeon_connector_free_edid(connector);
+		drm_edid_free(radeon_connector->edid);
+		radeon_connector->edid = NULL;
 		radeon_connector_get_edid(connector);
 
 		if (!radeon_connector->edid) {
@@ -1277,7 +1274,8 @@ radeon_dvi_detect(struct drm_connector *connector, bool force)
 			 * with a shared ddc line (often vga + hdmi)
 			 */
 			if ((!radeon_connector->use_digital) && radeon_connector->shared_ddc) {
-				radeon_connector_free_edid(connector);
+				drm_edid_free(radeon_connector->edid);
+				radeon_connector->edid = NULL;
 				ret = connector_status_disconnected;
 			} else {
 				ret = connector_status_connected;
@@ -1301,7 +1299,8 @@ radeon_dvi_detect(struct drm_connector *connector, bool force)
 						if (list_connector->connector_type != DRM_MODE_CONNECTOR_VGA) {
 							/* hpd is our only option in this case */
 							if (!radeon_hpd_sense(rdev, radeon_connector->hpd.hpd)) {
-								radeon_connector_free_edid(connector);
+								drm_edid_free(radeon_connector->edid);
+								radeon_connector->edid = NULL;
 								ret = connector_status_disconnected;
 							}
 						}
@@ -1635,7 +1634,8 @@ radeon_dp_detect(struct drm_connector *connector, bool force)
 		goto out;
 	}
 
-	radeon_connector_free_edid(connector);
+	drm_edid_free(radeon_connector->edid);
+	radeon_connector->edid = NULL;
 
 	if ((connector->connector_type == DRM_MODE_CONNECTOR_eDP) ||
 	    (connector->connector_type == DRM_MODE_CONNECTOR_LVDS)) {
-- 
2.53.0

Claude review: drm/radeon/radeon_connectors: remove radeon_connector_free_edid

[Claude Code Review Bot]

Patch Review

**Approach:** Removes the `radeon_connector_free_edid()` helper and replaces all call sites with inline `drm_edid_free()` + NULL assignment. This is the right thing to do since `drm_edid_free()` is a standard API function.

**The transformation is mechanical and correct** — every call site is replaced with the same two-line pattern:

```c
drm_edid_free(radeon_connector->edid);
radeon_connector->edid = NULL;
```

**Minor observation:** The NULL assignment after `drm_edid_free()` is good practice but is technically unnecessary in the `radeon_connector_destroy()` path since the connector struct itself is about to be freed. Not worth changing, just noting.

**Ordering concern:** As mentioned above, this patch fixes the correctness problem created by patch 1 (where `kfree` was being called on `struct drm_edid *`). The series would be cleaner if either: (a) patch 1 updates `radeon_connector_free_edid` to use `drm_edid_free()` internally, or (b) the two patches are squashed. As submitted, bisecting to the state after only patch 1 produces broken code.

---

**Summary:** The API migration is correctly done and the final state after both patches is correct. The main concern is the bisectability issue: after patch 1 alone, `kfree()` is called on `const struct drm_edid *`, which is both a memory leak (inner edid not freed) and a const-correctness violation. Recommend either squashing or fixing the `radeon_connector_free_edid` body in patch 1.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/radeon/radeon_connectors: use struct drm_edid

[Claude Code Review Bot]

Overall Series Review

Subject: drm/radeon/radeon_connectors: use struct drm_edid
Author: Joshua Peisach <jpeisach@ubuntu.com>
Patches: 4
Reviewed: 2026-05-25T17:37:44.687299

---

This is a 2-patch series converting the radeon connector code from the deprecated `struct edid` / `drm_get_edid()` / `drm_connector_update_edid_property()` API to the newer `struct drm_edid` / `drm_edid_read_ddc()` / `drm_edid_connector_update()` API. This mirrors an equivalent conversion already done for amdgpu.

The overall approach is correct and the API migration is sound. The new functions exist in the kernel tree with the expected signatures. The series is logically split: patch 1 does the type/API conversion, patch 2 inlines and removes the now-trivial `radeon_connector_free_edid` helper. However, there are issues with the ordering of patches in the mbox (patch 2/2 appears before patch 1/2), and there are a few correctness concerns worth raising.

**Verdict: Needs minor revision** — the series is on the right track but has a few issues to address.

---

---
Generated by Claude Code Patch Reviewer