[PATCH v8 0/8] drm/amd/display: more drm_edid to AMD display driver (partial)

[PATCH v8 0/8] drm/amd/display: more drm_edid to AMD display driver (partial)

[Melissa Wen]

Hi,

This is a reduced version of `drm/amd/display: more drm_edid to AMD
display driver` [1] sent a few months ago only with the less invasive
changes, i.e., those changes that don't affect DC. This partial focus on
using more drm_edid helpers instead of raw EDID helpers. Most patches
here are already reviewed by someone and I think they can be merged
without major concerns. They can also prevent new initiatives of keeping
parsing raw EDID as driver specific code, which can make harder for us
to remove `drm_edid_raw()`. They should focus on moving raw EDID
handling to the DRM common code instead.

Regarding code changes from previous version, here I added some r-b tags
(from Mario and Timur), fixed commit message syntax (Mario) and
centralized error handling (Timur). I'll follow up the changes in DC to
accept the Linux/DRM opaque object in a separate series since those look
more sensitive.

[1] https://lore.kernel.org/amd-gfx/20251106165536.161662-1-mwen@igalia.com/

BR,

Melissa

Melissa Wen (8):
  drm/amd/display: make sure drm_edid stored in aconnector doesn't leak
  drm/amd/display: start using drm_edid helpers to parse EDID caps
  drm/amd/display: use drm_edid_product_id for parsing EDID product info
  drm/amd/display: use drm_edid helper to set analog EDID caps
  drm/edid: introduce a helper that gets monitor name from drm_edid
  drm/amd/display: get panel id with drm_edid helper
  drm/amd/display: get SAD from drm_eld when parsing EDID caps
  drm/amd/display: get SADB from drm_eld when parsing EDID caps

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  2 +
 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 84 +++++++++----------
 drivers/gpu/drm/bridge/sil-sii8620.c          |  2 +-
 drivers/gpu/drm/display/drm_dp_mst_topology.c |  2 +-
 drivers/gpu/drm/drm_edid.c                    | 30 +++++--
 include/drm/drm_edid.h                        |  7 +-
 6 files changed, 69 insertions(+), 58 deletions(-)

-- 
2.51.0

[PATCH v8 1/8] drm/amd/display: make sure drm_edid stored in aconnector doesn't leak

[Melissa Wen]

Make sure the drm_edid container stored in aconnector is freed when
destroying the aconnector.

Fixes: 48edb2a4256e ("drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid")
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 50a10b4fbb3f..845069c9ce85 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -7729,6 +7729,8 @@ static void amdgpu_dm_connector_destroy(struct drm_connector *connector)
 		dc_sink_release(aconnector->dc_sink);
 	aconnector->dc_sink = NULL;
 
+	drm_edid_free(aconnector->drm_edid);
+
 	drm_dp_cec_unregister_connector(&aconnector->dm_dp_aux.aux);
 	drm_connector_unregister(connector);
 	drm_connector_cleanup(connector);
-- 
2.51.0

[PATCH v8 2/8] drm/amd/display: start using drm_edid helpers to parse EDID caps

[Melissa Wen]

Groundwork that allocates a temporary drm_edid from raw edid to take
advantage of DRM common-code helpers instead of driver-specific code.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---

v8:
- centralize cleanup and return to a single place (Timur)
---
 .../drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c  | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index 7d0ecce6b034..ef7f27ce4b50 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -110,18 +110,21 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
 	struct drm_connector *connector = &aconnector->base;
 	struct drm_device *dev = connector->dev;
 	struct edid *edid_buf = edid ? (struct edid *) edid->raw_edid : NULL;
+	const struct drm_edid *drm_edid;
 	struct cea_sad *sads;
 	int sad_count = -1;
 	int sadb_count = -1;
 	int i = 0;
 	uint8_t *sadb = NULL;
-
 	enum dc_edid_status result = EDID_OK;
 
+
 	if (!edid_caps || !edid)
 		return EDID_BAD_INPUT;
 
-	if (!drm_edid_is_valid(edid_buf))
+	drm_edid = drm_edid_alloc(edid_buf, EDID_LENGTH * (edid_buf->extensions + 1));
+
+	if (!drm_edid_valid(drm_edid))
 		result = EDID_BAD_CHECKSUM;
 
 	edid_caps->manufacturer_id = (uint16_t) edid_buf->mfg_id[0] |
@@ -145,8 +148,9 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
 	apply_edid_quirks(dev, edid_buf, edid_caps);
 
 	sad_count = drm_edid_to_sad((struct edid *) edid->raw_edid, &sads);
-	if (sad_count <= 0)
-		return result;
+	if (sad_count <= 0) {
+		goto cleanup;
+	}
 
 	edid_caps->audio_mode_count = min(sad_count, DC_MAX_AUDIO_DESC_COUNT);
 	for (i = 0; i < edid_caps->audio_mode_count; ++i) {
@@ -173,6 +177,8 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
 	kfree(sads);
 	kfree(sadb);
 
+cleanup:
+	drm_edid_free(drm_edid);
 	return result;
 }
 
-- 
2.51.0

[PATCH v8 3/8] drm/amd/display: use drm_edid_product_id for parsing EDID product info

[Melissa Wen]

commit 3ddbd345539e ("drm/edid: add drm_edid_get_product_id()")
introduced drm_edid_product_id. Use this to get debug info from
drm_edid instead of directly parsing the raw EDID.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---

v5:
- replace series url to commit hash (Mario)

v8:
- fix syntax in the commit message (Mario)
---
 .../drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index ef7f27ce4b50..93db85543440 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -111,6 +111,7 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
 	struct drm_device *dev = connector->dev;
 	struct edid *edid_buf = edid ? (struct edid *) edid->raw_edid : NULL;
 	const struct drm_edid *drm_edid;
+	struct drm_edid_product_id product_id;
 	struct cea_sad *sads;
 	int sad_count = -1;
 	int sadb_count = -1;
@@ -127,13 +128,13 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
 	if (!drm_edid_valid(drm_edid))
 		result = EDID_BAD_CHECKSUM;
 
-	edid_caps->manufacturer_id = (uint16_t) edid_buf->mfg_id[0] |
-					((uint16_t) edid_buf->mfg_id[1])<<8;
-	edid_caps->product_id = (uint16_t) edid_buf->prod_code[0] |
-					((uint16_t) edid_buf->prod_code[1])<<8;
-	edid_caps->serial_number = edid_buf->serial;
-	edid_caps->manufacture_week = edid_buf->mfg_week;
-	edid_caps->manufacture_year = edid_buf->mfg_year;
+	drm_edid_get_product_id(drm_edid, &product_id);
+
+	edid_caps->manufacturer_id = product_id.manufacturer_name;
+	edid_caps->product_id = le16_to_cpu(product_id.product_code);
+	edid_caps->serial_number = le32_to_cpu(product_id.serial_number);
+	edid_caps->manufacture_week = product_id.week_of_manufacture;
+	edid_caps->manufacture_year = product_id.year_of_manufacture;
 	edid_caps->analog = !(edid_buf->input & DRM_EDID_INPUT_DIGITAL);
 
 	drm_edid_get_monitor_name(edid_buf,
-- 
2.51.0

[PATCH v8 4/8] drm/amd/display: use drm_edid helper to set analog EDID caps

[Melissa Wen]

Use drm_edid_is_digital helper instead of open-coded mask.

Reviewed-by: Timur Kristóf <timur.kristof@gmail.com>
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index 93db85543440..e553b9cbe179 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -135,7 +135,7 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
 	edid_caps->serial_number = le32_to_cpu(product_id.serial_number);
 	edid_caps->manufacture_week = product_id.week_of_manufacture;
 	edid_caps->manufacture_year = product_id.year_of_manufacture;
-	edid_caps->analog = !(edid_buf->input & DRM_EDID_INPUT_DIGITAL);
+	edid_caps->analog = !drm_edid_is_digital(drm_edid);
 
 	drm_edid_get_monitor_name(edid_buf,
 				  edid_caps->display_name,
-- 
2.51.0

[PATCH v8 5/8] drm/edid: introduce a helper that gets monitor name from drm_edid

[Melissa Wen]

Original drm_edid_get_monitor_name encapsulates raw edid in drm_edid and
then call get_monitor_name. AMD still stores the display name for
debugging, but it is migrating to drm_edid, on the other hand,
drm_dp_mst_topology and sil-sii8620 still use the raw edid version.

Split drm_edid_get_monitor_name into two helpers, one that gets monitor
name from raw edid and another from drm_edid. It's temporary and the raw
edid version should be removed later.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---

v3:
- kernel-doc and commit msg mentionind raw edid stuff is deprecated (jani)
- use drm_edid_legacy_init instead of open coded (jani)
- move drm_edid new func declaration to its section (jani)
---
 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  2 +-
 drivers/gpu/drm/bridge/sil-sii8620.c          |  2 +-
 drivers/gpu/drm/display/drm_dp_mst_topology.c |  2 +-
 drivers/gpu/drm/drm_edid.c                    | 30 +++++++++++++------
 include/drm/drm_edid.h                        |  7 +++--
 5 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index e553b9cbe179..6a017e9a7bd9 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -137,7 +137,7 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
 	edid_caps->manufacture_year = product_id.year_of_manufacture;
 	edid_caps->analog = !drm_edid_is_digital(drm_edid);
 
-	drm_edid_get_monitor_name(edid_buf,
+	drm_edid_get_monitor_name(drm_edid,
 				  edid_caps->display_name,
 				  AUDIO_INFO_DISPLAY_NAME_SIZE_IN_CHARS);
 
diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
index 9e48ad39e1cc..891d904a7174 100644
--- a/drivers/gpu/drm/bridge/sil-sii8620.c
+++ b/drivers/gpu/drm/bridge/sil-sii8620.c
@@ -505,7 +505,7 @@ static void sii8620_identify_sink(struct sii8620 *ctx)
 	else
 		ctx->sink_type = SINK_DVI;
 
-	drm_edid_get_monitor_name(ctx->edid, sink_name, ARRAY_SIZE(sink_name));
+	drm_edid_raw_get_monitor_name(ctx->edid, sink_name, ARRAY_SIZE(sink_name));
 
 	dev_info(dev, "detected sink(type: %s): %s\n",
 		 sink_str[ctx->sink_type], sink_name);
diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 64e5c176d5cc..14e916bf88d0 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -4892,7 +4892,7 @@ static void fetch_monitor_name(struct drm_dp_mst_topology_mgr *mgr,
 	struct edid *mst_edid;
 
 	mst_edid = drm_dp_mst_get_edid(port->connector, mgr, port);
-	drm_edid_get_monitor_name(mst_edid, name, namelen);
+	drm_edid_raw_get_monitor_name(mst_edid, name, namelen);
 	kfree(mst_edid);
 }
 
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 26bb7710a462..78d676e8ab35 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -5600,27 +5600,23 @@ static int get_monitor_name(const struct drm_edid *drm_edid, char name[13])
 }
 
 /**
- * drm_edid_get_monitor_name - fetch the monitor name from the edid
- * @edid: monitor EDID information
+ * drm_edid_get_monitor_name - fetch the monitor name from the drm_edid
+ * @drm_edid: EDID
  * @name: pointer to a character array to hold the name of the monitor
  * @bufsize: The size of the name buffer (should be at least 14 chars.)
  *
  */
-void drm_edid_get_monitor_name(const struct edid *edid, char *name, int bufsize)
+void drm_edid_get_monitor_name(const struct drm_edid *drm_edid, char *name, int bufsize)
 {
 	int name_length = 0;
 
 	if (bufsize <= 0)
 		return;
 
-	if (edid) {
+	if (drm_edid->edid) {
 		char buf[13];
-		struct drm_edid drm_edid = {
-			.edid = edid,
-			.size = edid_size(edid),
-		};
 
-		name_length = min(get_monitor_name(&drm_edid, buf), bufsize - 1);
+		name_length = min(get_monitor_name(drm_edid, buf), bufsize - 1);
 		memcpy(name, buf, name_length);
 	}
 
@@ -5628,6 +5624,22 @@ void drm_edid_get_monitor_name(const struct edid *edid, char *name, int bufsize)
 }
 EXPORT_SYMBOL(drm_edid_get_monitor_name);
 
+/**
+ * drm_edid_raw_get_monitor_name - fetch the monitor name from raw edid
+ * @edid: monitor EDID information
+ * @name: pointer to a character array to hold the name of the monitor
+ * @bufsize: The size of the name buffer (should be at least 14 chars.)
+ *
+ * This function is deprecated. Use drm_edid_get_monitor_name() instead.
+ */
+void drm_edid_raw_get_monitor_name(const struct edid *edid, char *name, int bufsize)
+{
+	struct drm_edid drm_edid;
+
+	drm_edid_get_monitor_name(drm_edid_legacy_init(&drm_edid, edid), name, bufsize);
+}
+EXPORT_SYMBOL(drm_edid_raw_get_monitor_name);
+
 static void clear_eld(struct drm_connector *connector)
 {
 	mutex_lock(&connector->eld_mutex);
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 04f7a7f1f108..2aeba6f10e6b 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -454,8 +454,8 @@ int drm_add_modes_noedid(struct drm_connector *connector,
 
 int drm_edid_header_is_valid(const void *edid);
 bool drm_edid_is_valid(struct edid *edid);
-void drm_edid_get_monitor_name(const struct edid *edid, char *name,
-			       int buflen);
+void drm_edid_raw_get_monitor_name(const struct edid *edid, char *name,
+				   int bufsize);
 struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
 					   int hsize, int vsize, int fresh,
 					   bool rb);
@@ -490,5 +490,8 @@ u32 drm_edid_get_panel_id(const struct drm_edid *drm_edid);
 bool drm_edid_match(const struct drm_edid *drm_edid,
 		    const struct drm_edid_ident *ident);
 bool drm_edid_has_quirk(struct drm_connector *connector, enum drm_edid_quirk quirk);
+void drm_edid_get_monitor_name(const struct drm_edid *drm_edid,
+			       char *name,
+			       int bufsize);
 
 #endif /* __DRM_EDID_H__ */
-- 
2.51.0

[PATCH v8 6/8] drm/amd/display: get panel id with drm_edid helper

[Melissa Wen]

Instead of using driver-specific code, use DRM helpers.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Reviewed-by: Timur Kristóf <timur.kristof@gmail.com>
Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index 6a017e9a7bd9..7cef16ed2eb9 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -49,16 +49,11 @@
 #include "ddc_service_types.h"
 #include "clk_mgr.h"
 
-static u32 edid_extract_panel_id(struct edid *edid)
+static void apply_edid_quirks(struct drm_device *dev,
+			      const struct drm_edid *drm_edid,
+			      struct dc_edid_caps *edid_caps)
 {
-	return (u32)edid->mfg_id[0] << 24   |
-	       (u32)edid->mfg_id[1] << 16   |
-	       (u32)EDID_PRODUCT_ID(edid);
-}
-
-static void apply_edid_quirks(struct drm_device *dev, struct edid *edid, struct dc_edid_caps *edid_caps)
-{
-	uint32_t panel_id = edid_extract_panel_id(edid);
+	uint32_t panel_id = drm_edid_get_panel_id(drm_edid);
 
 	switch (panel_id) {
 	/* Workaround for monitors that need a delay after detecting the link */
@@ -146,7 +141,7 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
 	if (edid_caps->edid_hdmi)
 		populate_hdmi_info_from_connector(&connector->display_info.hdmi, edid_caps);
 
-	apply_edid_quirks(dev, edid_buf, edid_caps);
+	apply_edid_quirks(dev, drm_edid, edid_caps);
 
 	sad_count = drm_edid_to_sad((struct edid *) edid->raw_edid, &sads);
 	if (sad_count <= 0) {
-- 
2.51.0

[PATCH v8 7/8] drm/amd/display: get SAD from drm_eld when parsing EDID caps

[Melissa Wen]

drm_edid_connector_update() updates display info, filling ELD with audio
info from Short-Audio Descriptors in the last step of
update_dislay_info(). Our goal is stopping using raw edid, so we can
extract SAD from drm_eld instead of access raw edid to get audio caps.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Reviewed-by: Timur Kristóf <timur.kristof@gmail.com>
Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 22 ++++++++++---------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index 7cef16ed2eb9..519b80f43b90 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -34,6 +34,7 @@
 #include <drm/drm_probe_helper.h>
 #include <drm/amdgpu_drm.h>
 #include <drm/drm_edid.h>
+#include <drm/drm_eld.h>
 #include <drm/drm_fixed.h>
 
 #include "dm_services.h"
@@ -107,9 +108,7 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
 	struct edid *edid_buf = edid ? (struct edid *) edid->raw_edid : NULL;
 	const struct drm_edid *drm_edid;
 	struct drm_edid_product_id product_id;
-	struct cea_sad *sads;
-	int sad_count = -1;
-	int sadb_count = -1;
+	int sad_count, sadb_count;
 	int i = 0;
 	uint8_t *sadb = NULL;
 	enum dc_edid_status result = EDID_OK;
@@ -123,6 +122,7 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
 	if (!drm_edid_valid(drm_edid))
 		result = EDID_BAD_CHECKSUM;
 
+	drm_edid_connector_update(connector, drm_edid);
 	drm_edid_get_product_id(drm_edid, &product_id);
 
 	edid_caps->manufacturer_id = product_id.manufacturer_name;
@@ -143,19 +143,22 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
 
 	apply_edid_quirks(dev, drm_edid, edid_caps);
 
-	sad_count = drm_edid_to_sad((struct edid *) edid->raw_edid, &sads);
+	sad_count = drm_eld_sad_count(connector->eld);
 	if (sad_count <= 0) {
 		goto cleanup;
 	}
 
 	edid_caps->audio_mode_count = min(sad_count, DC_MAX_AUDIO_DESC_COUNT);
 	for (i = 0; i < edid_caps->audio_mode_count; ++i) {
-		struct cea_sad *sad = &sads[i];
+		struct cea_sad sad;
 
-		edid_caps->audio_modes[i].format_code = sad->format;
-		edid_caps->audio_modes[i].channel_count = sad->channels + 1;
-		edid_caps->audio_modes[i].sample_rate = sad->freq;
-		edid_caps->audio_modes[i].sample_size = sad->byte2;
+		if (drm_eld_sad_get(connector->eld, i, &sad) < 0)
+			continue;
+
+		edid_caps->audio_modes[i].format_code = sad.format;
+		edid_caps->audio_modes[i].channel_count = sad.channels + 1;
+		edid_caps->audio_modes[i].sample_rate = sad.freq;
+		edid_caps->audio_modes[i].sample_size = sad.byte2;
 	}
 
 	sadb_count = drm_edid_to_speaker_allocation((struct edid *) edid->raw_edid, &sadb);
@@ -170,7 +173,6 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
 	else
 		edid_caps->speaker_flags = DEFAULT_SPEAKER_LOCATION;
 
-	kfree(sads);
 	kfree(sadb);
 
 cleanup:
-- 
2.51.0

[PATCH v8 8/8] drm/amd/display: get SADB from drm_eld when parsing EDID caps

[Melissa Wen]

drm_edid_connector_update() updates display info, filling ELD with
speaker allocation data in the last step of update_dislay_info(). Our
goal is stopping using raw edid, so we can extract SADB from drm_eld
instead of access raw edid to get audio caps.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c    | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index 519b80f43b90..4f0e8e46f0bd 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -108,9 +108,8 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
 	struct edid *edid_buf = edid ? (struct edid *) edid->raw_edid : NULL;
 	const struct drm_edid *drm_edid;
 	struct drm_edid_product_id product_id;
-	int sad_count, sadb_count;
+	int sad_count;
 	int i = 0;
-	uint8_t *sadb = NULL;
 	enum dc_edid_status result = EDID_OK;
 
 
@@ -161,20 +160,11 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
 		edid_caps->audio_modes[i].sample_size = sad.byte2;
 	}
 
-	sadb_count = drm_edid_to_speaker_allocation((struct edid *) edid->raw_edid, &sadb);
-
-	if (sadb_count < 0) {
-		DRM_ERROR("Couldn't read Speaker Allocation Data Block: %d\n", sadb_count);
-		sadb_count = 0;
-	}
-
-	if (sadb_count)
-		edid_caps->speaker_flags = sadb[0];
+	if (connector->eld[DRM_ELD_SPEAKER])
+		edid_caps->speaker_flags = connector->eld[DRM_ELD_SPEAKER];
 	else
 		edid_caps->speaker_flags = DEFAULT_SPEAKER_LOCATION;
 
-	kfree(sadb);
-
 cleanup:
 	drm_edid_free(drm_edid);
 	return result;
-- 
2.51.0

Re: [PATCH v8 0/8] drm/amd/display: more drm_edid to AMD display driver (partial)

[Timur Kristóf]

On Wednesday, February 25, 2026 1:04:29 PM Central European Standard Time 
Melissa Wen wrote:
> Hi,
> 
> This is a reduced version of `drm/amd/display: more drm_edid to AMD
> display driver` [1] sent a few months ago only with the less invasive
> changes, i.e., those changes that don't affect DC. This partial focus on
> using more drm_edid helpers instead of raw EDID helpers. Most patches
> here are already reviewed by someone and I think they can be merged
> without major concerns. They can also prevent new initiatives of keeping
> parsing raw EDID as driver specific code, which can make harder for us
> to remove `drm_edid_raw()`. They should focus on moving raw EDID
> handling to the DRM common code instead.
> 
> Regarding code changes from previous version, here I added some r-b tags
> (from Mario and Timur), fixed commit message syntax (Mario) and
> centralized error handling (Timur). I'll follow up the changes in DC to
> accept the Linux/DRM opaque object in a separate series since those look
> more sensitive.
> 
> [1] https://lore.kernel.org/amd-gfx/20251106165536.161662-1-mwen@igalia.com/
> 
> BR,
> 
> Melissa
> 

Nice work, Melissa.

Reviewed-by: Timur Kristóf <timur.kristof@gmail.com>
For this series.

Re: [PATCH v8 0/8] drm/amd/display: more drm_edid to AMD display driver (partial)

[Leo Li]

On 2026-02-27 03:52, Timur Kristóf wrote:
> On Wednesday, February 25, 2026 1:04:29 PM Central European Standard Time 
> Melissa Wen wrote:
>> Hi,
>>
>> This is a reduced version of `drm/amd/display: more drm_edid to AMD
>> display driver` [1] sent a few months ago only with the less invasive
>> changes, i.e., those changes that don't affect DC. This partial focus on
>> using more drm_edid helpers instead of raw EDID helpers. Most patches
>> here are already reviewed by someone and I think they can be merged
>> without major concerns. They can also prevent new initiatives of keeping
>> parsing raw EDID as driver specific code, which can make harder for us
>> to remove `drm_edid_raw()`. They should focus on moving raw EDID
>> handling to the DRM common code instead.
>>
>> Regarding code changes from previous version, here I added some r-b tags
>> (from Mario and Timur), fixed commit message syntax (Mario) and
>> centralized error handling (Timur). I'll follow up the changes in DC to
>> accept the Linux/DRM opaque object in a separate series since those look
>> more sensitive.
>>
>> [1] https://lore.kernel.org/amd-gfx/20251106165536.161662-1-mwen@igalia.com/
>>
>> BR,
>>
>> Melissa
>>
> 
> Nice work, Melissa.
> 
> Reviewed-by: Timur Kristóf <timur.kristof@gmail.com>
> For this series.

Thanks for your patience Melissa, series is also
Reviewed-by: Leo Li <sunpeng.li@amd.com>

We'll include it in this week's testing.

- Leo
> 
> 

Re: [PATCH v8 0/8] drm/amd/display: more drm_edid to AMD display driver (partial)

[Melissa Wen]

On 31/03/2026 12:45, Leo Li wrote:
>
> On 2026-02-27 03:52, Timur Kristóf wrote:
>> On Wednesday, February 25, 2026 1:04:29 PM Central European Standard Time
>> Melissa Wen wrote:
>>> Hi,
>>>
>>> This is a reduced version of `drm/amd/display: more drm_edid to AMD
>>> display driver` [1] sent a few months ago only with the less invasive
>>> changes, i.e., those changes that don't affect DC. This partial focus on
>>> using more drm_edid helpers instead of raw EDID helpers. Most patches
>>> here are already reviewed by someone and I think they can be merged
>>> without major concerns. They can also prevent new initiatives of keeping
>>> parsing raw EDID as driver specific code, which can make harder for us
>>> to remove `drm_edid_raw()`. They should focus on moving raw EDID
>>> handling to the DRM common code instead.
>>>
>>> Regarding code changes from previous version, here I added some r-b tags
>>> (from Mario and Timur), fixed commit message syntax (Mario) and
>>> centralized error handling (Timur). I'll follow up the changes in DC to
>>> accept the Linux/DRM opaque object in a separate series since those look
>>> more sensitive.
>>>
>>> [1] https://lore.kernel.org/amd-gfx/20251106165536.161662-1-mwen@igalia.com/
>>>
>>> BR,
>>>
>>> Melissa
>>>
>> Nice work, Melissa.
>>
>> Reviewed-by: Timur Kristóf <timur.kristof@gmail.com>
>> For this series.
> Thanks for your patience Melissa, series is also
> Reviewed-by: Leo Li <sunpeng.li@amd.com>
>
> We'll include it in this week's testing.

Hi Leo,

In the end, was this series applied to AMD's branch or do I need to 
rebase it?

Melissa

>
> - Leo
>>

Re: [PATCH v8 0/8] drm/amd/display: more drm_edid to AMD display driver (partial)

[Leo Li]

On 2026-04-13 13:51, Melissa Wen wrote:
> 
> 
> On 31/03/2026 12:45, Leo Li wrote:
>>
>> On 2026-02-27 03:52, Timur Kristóf wrote:
>>> On Wednesday, February 25, 2026 1:04:29 PM Central European Standard Time
>>> Melissa Wen wrote:
>>>> Hi,
>>>>
>>>> This is a reduced version of `drm/amd/display: more drm_edid to AMD
>>>> display driver` [1] sent a few months ago only with the less invasive
>>>> changes, i.e., those changes that don't affect DC. This partial focus on
>>>> using more drm_edid helpers instead of raw EDID helpers. Most patches
>>>> here are already reviewed by someone and I think they can be merged
>>>> without major concerns. They can also prevent new initiatives of keeping
>>>> parsing raw EDID as driver specific code, which can make harder for us
>>>> to remove `drm_edid_raw()`. They should focus on moving raw EDID
>>>> handling to the DRM common code instead.
>>>>
>>>> Regarding code changes from previous version, here I added some r-b tags
>>>> (from Mario and Timur), fixed commit message syntax (Mario) and
>>>> centralized error handling (Timur). I'll follow up the changes in DC to
>>>> accept the Linux/DRM opaque object in a separate series since those look
>>>> more sensitive.
>>>>
>>>> [1] https://lore.kernel.org/amd-gfx/20251106165536.161662-1-mwen@igalia.com/
>>>>
>>>> BR,
>>>>
>>>> Melissa
>>>>
>>> Nice work, Melissa.
>>>
>>> Reviewed-by: Timur Kristóf <timur.kristof@gmail.com>
>>> For this series.
>> Thanks for your patience Melissa, series is also
>> Reviewed-by: Leo Li <sunpeng.li@amd.com>
>>
>> We'll include it in this week's testing.
> 
> Hi Leo,
> 
> In the end, was this series applied to AMD's branch or do I need to rebase it?
> 
> Melissa

Hi Melissa,

During testing, it seems we're failing drm_edid_valid() in this callpath:

    dm_helpers_read_local_edid > dm_helpers_parse_edid_caps > drm_edid_valid.

I'm 90% sure it's because recreating the drm_edid from dc_edid ignores the
HF-EEODB (HDMI Forum EDID Extension Override Data Block), which can override the
number of extensions dictated by the base EDID. However, drm_edid_valid() does
consider the HF-EEODB, causing this line(*) to return false. Commenting it out
"fixes" the issue

(*)https://elixir.bootlin.com/linux/v7.0/source/drivers/gpu/drm/drm_edid.c#L2076

This whole drm_edid to dc_edid conversion in dm_helpers_read_local_edid(), then
immediately back to drm_edid in parse_edid_caps(), is really not nice... We lose
the HF-EEODB-aware size by creating dc_edid based on the EDID-advertised
extension count. It should work if drm_edid is simply passed straight through.

The challenge is dm_helpers* are DC's interface back to DM, and there is another
OS that depends on it. Plumbing drm_edid through DC would be quite challenging.

Alternatively, It should work if we carry over the drm_edid.size value to
dc_edid.length. Do you know the motivation behind keeping drm_edid->size
private? Would it be a good idea to implement a drm_edid_get_size()?

Thanks,
Leo

> 
>>
>> - Leo
>>>
> 

Re: [PATCH v8 0/8] drm/amd/display: more drm_edid to AMD display driver (partial)

[Jani Nikula]

On Tue, 14 Apr 2026, Leo Li <sunpeng.li@amd.com> wrote:
> On 2026-04-13 13:51, Melissa Wen wrote:
>> 
>> 
>> On 31/03/2026 12:45, Leo Li wrote:
>>>
>>> On 2026-02-27 03:52, Timur Kristóf wrote:
>>>> On Wednesday, February 25, 2026 1:04:29 PM Central European Standard Time
>>>> Melissa Wen wrote:
>>>>> Hi,
>>>>>
>>>>> This is a reduced version of `drm/amd/display: more drm_edid to AMD
>>>>> display driver` [1] sent a few months ago only with the less invasive
>>>>> changes, i.e., those changes that don't affect DC. This partial focus on
>>>>> using more drm_edid helpers instead of raw EDID helpers. Most patches
>>>>> here are already reviewed by someone and I think they can be merged
>>>>> without major concerns. They can also prevent new initiatives of keeping
>>>>> parsing raw EDID as driver specific code, which can make harder for us
>>>>> to remove `drm_edid_raw()`. They should focus on moving raw EDID
>>>>> handling to the DRM common code instead.
>>>>>
>>>>> Regarding code changes from previous version, here I added some r-b tags
>>>>> (from Mario and Timur), fixed commit message syntax (Mario) and
>>>>> centralized error handling (Timur). I'll follow up the changes in DC to
>>>>> accept the Linux/DRM opaque object in a separate series since those look
>>>>> more sensitive.
>>>>>
>>>>> [1] https://lore.kernel.org/amd-gfx/20251106165536.161662-1-mwen@igalia.com/
>>>>>
>>>>> BR,
>>>>>
>>>>> Melissa
>>>>>
>>>> Nice work, Melissa.
>>>>
>>>> Reviewed-by: Timur Kristóf <timur.kristof@gmail.com>
>>>> For this series.
>>> Thanks for your patience Melissa, series is also
>>> Reviewed-by: Leo Li <sunpeng.li@amd.com>
>>>
>>> We'll include it in this week's testing.
>> 
>> Hi Leo,
>> 
>> In the end, was this series applied to AMD's branch or do I need to rebase it?
>> 
>> Melissa
>
> Hi Melissa,
>
> During testing, it seems we're failing drm_edid_valid() in this callpath:
>
>     dm_helpers_read_local_edid > dm_helpers_parse_edid_caps > drm_edid_valid.
>
> I'm 90% sure it's because recreating the drm_edid from dc_edid ignores the
> HF-EEODB (HDMI Forum EDID Extension Override Data Block), which can override the
> number of extensions dictated by the base EDID. However, drm_edid_valid() does
> consider the HF-EEODB, causing this line(*) to return false. Commenting it out
> "fixes" the issue
>
> (*)https://elixir.bootlin.com/linux/v7.0/source/drivers/gpu/drm/drm_edid.c#L2076
>
> This whole drm_edid to dc_edid conversion in dm_helpers_read_local_edid(), then
> immediately back to drm_edid in parse_edid_caps(), is really not nice... We lose
> the HF-EEODB-aware size by creating dc_edid based on the EDID-advertised
> extension count. It should work if drm_edid is simply passed straight through.
>
> The challenge is dm_helpers* are DC's interface back to DM, and there is another
> OS that depends on it. Plumbing drm_edid through DC would be quite challenging.

I find this to be in stark contrast with stable-api-nonsense.rst [1].
Why do we have internal kernel code that we can't change based on the
*outside* needs even if it makes total sense from the kernel POV? Nobody
outside of AMD has any idea of the constraints or how they apply.

[1] https://docs.kernel.org/process/stable-api-nonsense.html

> Alternatively, It should work if we carry over the drm_edid.size value to
> dc_edid.length. Do you know the motivation behind keeping drm_edid->size
> private? Would it be a good idea to implement a drm_edid_get_size()?

struct drm_edid is opaque specifically to prevent drivers from trying to
parse EDID directly or trying to calculate the EDID size. I did this to
enforce centralized handling of HF-EEODB. With the old struct edid,
there used to be huge amounts of code computing EDID size, and you
couldn't be sure if whoever allocated it was aware of HF-EEODB or
not. struct drm_edid dissociates the EDID from the allocated buffer
size.

And, really, all EDID parsing should happen in drm_edid.c the same way
for all drivers, and making struct drm_edid opaque is pretty much the
only way to block quick hacks from proliferating.

BR,
Jani.

-- 
Jani Nikula, Intel

Re: [PATCH v8 0/8] drm/amd/display: more drm_edid to AMD display driver (partial)

[Melissa Wen]

On 14/04/2026 20:17, Leo Li wrote:
>
> On 2026-04-13 13:51, Melissa Wen wrote:
>>
>> On 31/03/2026 12:45, Leo Li wrote:
>>> On 2026-02-27 03:52, Timur Kristóf wrote:
>>>> On Wednesday, February 25, 2026 1:04:29 PM Central European Standard Time
>>>> Melissa Wen wrote:
>>>>> Hi,
>>>>>
>>>>> This is a reduced version of `drm/amd/display: more drm_edid to AMD
>>>>> display driver` [1] sent a few months ago only with the less invasive
>>>>> changes, i.e., those changes that don't affect DC. This partial focus on
>>>>> using more drm_edid helpers instead of raw EDID helpers. Most patches
>>>>> here are already reviewed by someone and I think they can be merged
>>>>> without major concerns. They can also prevent new initiatives of keeping
>>>>> parsing raw EDID as driver specific code, which can make harder for us
>>>>> to remove `drm_edid_raw()`. They should focus on moving raw EDID
>>>>> handling to the DRM common code instead.
>>>>>
>>>>> Regarding code changes from previous version, here I added some r-b tags
>>>>> (from Mario and Timur), fixed commit message syntax (Mario) and
>>>>> centralized error handling (Timur). I'll follow up the changes in DC to
>>>>> accept the Linux/DRM opaque object in a separate series since those look
>>>>> more sensitive.
>>>>>
>>>>> [1] https://lore.kernel.org/amd-gfx/20251106165536.161662-1-mwen@igalia.com/
>>>>>
>>>>> BR,
>>>>>
>>>>> Melissa
>>>>>
>>>> Nice work, Melissa.
>>>>
>>>> Reviewed-by: Timur Kristóf <timur.kristof@gmail.com>
>>>> For this series.
>>> Thanks for your patience Melissa, series is also
>>> Reviewed-by: Leo Li <sunpeng.li@amd.com>
>>>
>>> We'll include it in this week's testing.
>> Hi Leo,
>>
>> In the end, was this series applied to AMD's branch or do I need to rebase it?
>>
>> Melissa
> Hi Melissa,
>
> During testing, it seems we're failing drm_edid_valid() in this callpath:
>
>      dm_helpers_read_local_edid > dm_helpers_parse_edid_caps > drm_edid_valid.
>
> I'm 90% sure it's because recreating the drm_edid from dc_edid ignores the
> HF-EEODB (HDMI Forum EDID Extension Override Data Block), which can override the
> number of extensions dictated by the base EDID. However, drm_edid_valid() does
> consider the HF-EEODB, causing this line(*) to return false. Commenting it out
> "fixes" the issue
>
> (*)https://elixir.bootlin.com/linux/v7.0/source/drivers/gpu/drm/drm_edid.c#L2076
>
> This whole drm_edid to dc_edid conversion in dm_helpers_read_local_edid(), then
> immediately back to drm_edid in parse_edid_caps(), is really not nice... We lose
> the HF-EEODB-aware size by creating dc_edid based on the EDID-advertised
> extension count. It should work if drm_edid is simply passed straight through.
Hi Leo,

Thanks for testing and debugging the issue.
Can you share a reproducer, IGT test or setup to trigger this failure?
Anyway, I'll try to come up with another approach for this...

Also, I'm confident that we can avoid these ugly conversions with my 
second half migration series [1] plus the series to move AMD VSDB 
parsing in drm_edid [2].

[1] https://lore.kernel.org/amd-gfx/20251106165536.161662-1-mwen@igalia.com/
[2] 
https://lore.kernel.org/dri-devel/20260327082342.1286878-1-chen-yu.chen@amd.com/

BR,

Melissa
>
> The challenge is dm_helpers* are DC's interface back to DM, and there is another
> OS that depends on it. Plumbing drm_edid through DC would be quite challenging.
>
> Alternatively, It should work if we carry over the drm_edid.size value to
> dc_edid.length. Do you know the motivation behind keeping drm_edid->size
> private? Would it be a good idea to implement a drm_edid_get_size()?
>
> Thanks,
> Leo
>
>>> - Leo