[PATCH v4 00/14] drm/amd/display: more drm_edid to AMD display driver

[PATCH v4 00/14] drm/amd/display: more drm_edid to AMD display driver

[Melissa Wen]

Hi,

Siqueira and I have been working on a solution to reduce the usage of
drm_edid_raw in the AMD display driver, since the current guideline in
the DRM subsystem is to stop handling raw edid data in driver-specific
implementation and use opaque `drm_edid` object with common-code
helpers.

In short, this series approaches the issue of maintaining DC as an
OS-agnostic component by creating a mid layer to isolate `drm_edid`
helpers that need to be called in the DC code, while allowing other OSes
to implement their specific implementation. This is an extension of [1].

- Patch 1 addresses a possible leak added by previous migration to
  drm_edid.
- Patch 2 allocates a temporary drm_edid from raw edid for parsing.
- Patches 3-7 use common-code, drm_edid helpers to parse edid
  capabilities instead of driver-specific solutions. For this, patch 4
  introduces a new helper that gets monitor name from drm_edid.
- Patches 8-9 are groundwork to reduce the noise of Linux/DRM specific
  code in the DC shared code
- Patch 10 creates a mid layer to make DC embraces different ways of
  handling EDID by platforms.
- Patch 11 move open-coded management of raw EDID data to the mid
  layer created before.
- Patch 12 introduces a helper that compares EDIDs from two drm_edids.
- Patch 13 adds drm_edid to dc_sink struct and a mid-layer helper to
  free `drm_edid`.
- Patch 14 switch dc_edid to drm_edid across the driver in a way that
  the DC shared code is little affected by Linux specific stuff.

[v1] https://lore.kernel.org/dri-devel/20250411201333.151335-1-mwen@igalia.com/
Changes:
- fix broken approach to get monitor name from eld (Jani)
  - I introduced a new helper that gets monitor name from drm_edid
- rename drm_edid_eq to drm_edid_eq_buf and doc fixes (Jani)
- add NULL edid checks (Jani)
- fix mishandling of product_id.manufacturer_name (Michel)
  - I directly set it to manufacturer_id since sparse didn't complain.
- add Mario's r-b in the first fix patch and fix commit msg typo.

[v2] https://lore.kernel.org/dri-devel/20250507001712.120215-1-mwen@igalia.com/
Changes:
- kernel-doc and commit msg fixes (Jani)
- use drm_edid_legacy_init instead of open coded (Jani)
- place drm_edid new func into the right section (Jani)
- paramenter names fix (Jani)
- add Jani's r-b to the patch 12
- remove unnecessary include (Jani)
- call dc_edid_sink_edid_free in link_detection, instead of drm_edid_free
- rebase on top of asdn

[v3] https://lore.kernel.org/dri-devel/20250514202130.291324-1-mwen@igalia.com/
Changes:
- rebase to asdn
- some kernel-doc fixes
- move some changes to the right commit

---

There are three specific points where we still use drm_edid_raw() in the
driver:
1. raw edid data for write EDID checksum in DP_TEST_EDID_CHECKSUM via
   drm_dp_dpcd_write(), that AFAIK there is no common code solution yet;
2. open-coded connectivity log for dc link detection, that maybe can be
   moved to drm (?);
3. open-coded parser that I suspect is a lot of duplicated code, but
   needs careful examining.

I suggest to address those points in a next phase for regression control.

Finally, I'm not sure amdgpu_dm/dc_edid is the right place and file name
for the mid-layer. Suggestions?

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

Let me know yours thoughts!

Melissa

Melissa Wen (12):
  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/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
  drm/amd/display: simplify dm_helpers_parse_edid_caps signature
  drm/amd/display: change DC functions to accept private types for edid
  drm/edid: introduce a helper that compares edid data from two drm_edid
  drm/amd/display: add drm_edid to dc_sink
  drm/amd/display: move dc_sink from dc_edid to drm_edid

Rodrigo Siqueira (2):
  drm/amd/display: add a mid-layer file to handle EDID in DC
  drm/amd/display: create a function to fill dc_sink with edid data

 .../gpu/drm/amd/display/amdgpu_dm/Makefile    |   1 +
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  33 +++---
 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 102 +++++++-----------
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c   |  21 ++--
 .../gpu/drm/amd/display/amdgpu_dm/dc_edid.c   |  33 ++++++
 .../gpu/drm/amd/display/amdgpu_dm/dc_edid.h   |  15 +++
 .../drm/amd/display/dc/core/dc_link_exports.c |   9 +-
 drivers/gpu/drm/amd/display/dc/core/dc_sink.c |   3 +
 drivers/gpu/drm/amd/display/dc/dc.h           |  10 +-
 drivers/gpu/drm/amd/display/dc/dm_helpers.h   |   7 +-
 drivers/gpu/drm/amd/display/dc/inc/link.h     |   9 +-
 .../drm/amd/display/dc/link/link_detection.c  |  30 ++----
 .../drm/amd/display/dc/link/link_detection.h  |   9 +-
 drivers/gpu/drm/bridge/sil-sii8620.c          |   2 +-
 drivers/gpu/drm/display/drm_dp_mst_topology.c |   2 +-
 drivers/gpu/drm/drm_edid.c                    |  54 +++++++---
 include/drm/drm_edid.h                        |  10 +-
 17 files changed, 191 insertions(+), 159 deletions(-)
 create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
 create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h

-- 
2.47.2

[PATCH v4 01/14] 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: 48edb2a4 ("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 78816712afbb..2f806bc6322b 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -7252,6 +7252,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.47.2

[PATCH v4 02/14] 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>
---
 .../drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c    | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 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 d4395b92fb85..5543780f1024 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,18 +108,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] |
@@ -139,8 +142,10 @@ 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)
+	if (sad_count <= 0) {
+		drm_edid_free(drm_edid);
 		return result;
+	}
 
 	edid_caps->audio_mode_count = min(sad_count, DC_MAX_AUDIO_DESC_COUNT);
 	for (i = 0; i < edid_caps->audio_mode_count; ++i) {
@@ -166,6 +171,7 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
 
 	kfree(sads);
 	kfree(sadb);
+	drm_edid_free(drm_edid);
 
 	return result;
 }
-- 
2.47.2

[PATCH v4 03/14] drm/amd/display: use drm_edid_product_id for parsing EDID product info

[Melissa Wen]

Since [1], we can use drm_edid_product_id to get debug info from
drm_edid instead of directly parsing EDID.

Link: https://lore.kernel.org/dri-devel/cover.1712655867.git.jani.nikula@intel.com/ [1]
Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../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 5543780f1024..b1085f1195f7 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
@@ -109,6 +109,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;
@@ -125,13 +126,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;
 
 	drm_edid_get_monitor_name(edid_buf,
 				  edid_caps->display_name,
-- 
2.47.2

[PATCH v4 04/14] 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.

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)

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../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                        |  8 +++--
 5 files changed, 29 insertions(+), 15 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 b1085f1195f7..514da4d5d300 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
@@ -134,7 +134,7 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
 	edid_caps->manufacture_week = product_id.week_of_manufacture;
 	edid_caps->manufacture_year = product_id.year_of_manufacture;
 
-	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 28a2e1ee04b2..c2d60b9c28fd 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 3a1f1ffc7b55..b17a602516ee 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -4896,7 +4896,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 13bc4c290b17..d5772a3d27f1 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -5575,27 +5575,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);
 	}
 
@@ -5603,6 +5599,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 eaac5e665892..960592167486 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -441,8 +441,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);
@@ -476,5 +476,7 @@ void drm_edid_print_product_id(struct drm_printer *p,
 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);
-
+void drm_edid_get_monitor_name(const struct drm_edid *drm_edid,
+			       char *name,
+			       int bufsize);
 #endif /* __DRM_EDID_H__ */
-- 
2.47.2

[PATCH v4 05/14] drm/amd/display: get panel id with drm_edid helper

[Melissa Wen]

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

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 514da4d5d300..760da13612b9 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
@@ -48,16 +48,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 */
@@ -140,7 +135,7 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
 
 	edid_caps->edid_hdmi = connector->display_info.is_hdmi;
 
-	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.47.2

[PATCH v4 06/14] 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.

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 760da13612b9..10af9db47c39 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
@@ -33,6 +33,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"
@@ -105,9 +106,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;
@@ -121,6 +120,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;
@@ -137,7 +137,7 @@ 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) {
 		drm_edid_free(drm_edid);
 		return result;
@@ -145,12 +145,15 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
 
 	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);
@@ -165,7 +168,6 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
 	else
 		edid_caps->speaker_flags = DEFAULT_SPEAKER_LOCATION;
 
-	kfree(sads);
 	kfree(sadb);
 	drm_edid_free(drm_edid);
 
-- 
2.47.2

[PATCH v4 07/14] 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.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 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 10af9db47c39..e7cfbee6c67f 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
@@ -106,9 +106,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;
 
 
@@ -156,19 +155,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);
 	drm_edid_free(drm_edid);
 
 	return result;
-- 
2.47.2

[PATCH v4 08/14] drm/amd/display: simplify dm_helpers_parse_edid_caps signature

[Melissa Wen]

Pass dc_sink to dm_helpers_parse_edid_caps(), since it already contains
edid info. It's a groundwork to get rid of raw edid stored as dc_edid.

v4:
- fix kernel-doc

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  5 +---
 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 23 ++++++++-----------
 drivers/gpu/drm/amd/display/dc/dm_helpers.h   |  7 ++----
 .../drm/amd/display/dc/link/link_detection.c  |  5 +---
 4 files changed, 13 insertions(+), 27 deletions(-)

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 2f806bc6322b..c7efeb9f38b6 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -7384,10 +7384,7 @@ static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector)
 		memset(&dc_em_sink->edid_caps, 0, sizeof(struct dc_edid_caps));
 		memmove(dc_em_sink->dc_edid.raw_edid, edid,
 			(edid->extensions + 1) * EDID_LENGTH);
-		dm_helpers_parse_edid_caps(
-			dc_link,
-			&dc_em_sink->dc_edid,
-			&dc_em_sink->edid_caps);
+		dm_helpers_parse_edid_caps(dc_link, dc_em_sink);
 	}
 }
 
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 e7cfbee6c67f..abfce44dcee7 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
@@ -89,29 +89,27 @@ static void apply_edid_quirks(struct drm_device *dev,
 /**
  * dm_helpers_parse_edid_caps() - Parse edid caps
  *
- * @link: current detected link
- * @edid:	[in] pointer to edid
- * @edid_caps:	[in] pointer to edid caps
+ * @link: current detected link (connector)
+ * @sink: current detected sink (display)
  *
  * Return: void
  */
-enum dc_edid_status dm_helpers_parse_edid_caps(
-		struct dc_link *link,
-		const struct dc_edid *edid,
-		struct dc_edid_caps *edid_caps)
+enum dc_edid_status dm_helpers_parse_edid_caps(struct dc_link *link,
+					       struct dc_sink *sink)
 {
 	struct amdgpu_dm_connector *aconnector = link->priv;
 	struct drm_connector *connector = &aconnector->base;
 	struct drm_device *dev = connector->dev;
-	struct edid *edid_buf = edid ? (struct edid *) edid->raw_edid : NULL;
+	struct edid *edid_buf;
 	const struct drm_edid *drm_edid;
 	struct drm_edid_product_id product_id;
+	struct dc_edid_caps *edid_caps = &sink->edid_caps;
 	int sad_count;
 	int i = 0;
 	enum dc_edid_status result = EDID_OK;
 
-
-	if (!edid_caps || !edid)
+	edid_buf = (struct edid *) &sink->dc_edid.raw_edid;
+	if (!edid_caps || !edid_buf)
 		return EDID_BAD_INPUT;
 
 	drm_edid = drm_edid_alloc(edid_buf, EDID_LENGTH * (edid_buf->extensions + 1));
@@ -1030,10 +1028,7 @@ enum dc_edid_status dm_helpers_read_local_edid(
 		/* We don't need the original edid anymore */
 		drm_edid_free(drm_edid);
 
-		edid_status = dm_helpers_parse_edid_caps(
-						link,
-						&sink->dc_edid,
-						&sink->edid_caps);
+		edid_status = dm_helpers_parse_edid_caps(link, sink);
 
 	} while (edid_status == EDID_BAD_CHECKSUM && --retry > 0);
 
diff --git a/drivers/gpu/drm/amd/display/dc/dm_helpers.h b/drivers/gpu/drm/amd/display/dc/dm_helpers.h
index 9d160b39e8c5..ce6a70368bd0 100644
--- a/drivers/gpu/drm/amd/display/dc/dm_helpers.h
+++ b/drivers/gpu/drm/amd/display/dc/dm_helpers.h
@@ -59,11 +59,8 @@ void dm_helpers_free_gpu_mem(
 		enum dc_gpu_mem_alloc_type type,
 		void *pvMem);
 
-enum dc_edid_status dm_helpers_parse_edid_caps(
-	struct dc_link *link,
-	const struct dc_edid *edid,
-	struct dc_edid_caps *edid_caps);
-
+enum dc_edid_status dm_helpers_parse_edid_caps(struct dc_link *link,
+					       struct dc_sink *sink);
 
 /*
  * Update DP branch info
diff --git a/drivers/gpu/drm/amd/display/dc/link/link_detection.c b/drivers/gpu/drm/amd/display/dc/link/link_detection.c
index 9655e6fa53a4..5ac361c59371 100644
--- a/drivers/gpu/drm/amd/display/dc/link/link_detection.c
+++ b/drivers/gpu/drm/amd/display/dc/link/link_detection.c
@@ -1420,10 +1420,7 @@ struct dc_sink *link_add_remote_sink(
 			dc_sink))
 		goto fail_add_sink;
 
-	edid_status = dm_helpers_parse_edid_caps(
-			link,
-			&dc_sink->dc_edid,
-			&dc_sink->edid_caps);
+	edid_status = dm_helpers_parse_edid_caps(link, dc_sink);
 
 	/*
 	 * Treat device as no EDID device if EDID
-- 
2.47.2

[PATCH v4 09/14] drm/amd/display: change DC functions to accept private types for edid

[Melissa Wen]

There is an opaque obj in Linux/DRM to encapsulate edid data as
`drm_edid`. This obj isn't present in other platforms but we need to
pass it through DC when adding sink. To pass this data without
compromise the independence of DC code, make some DC functions accept
edid data as private options.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/amd/display/dc/core/dc_link_exports.c | 9 ++++-----
 drivers/gpu/drm/amd/display/dc/dc.h                   | 9 ++++-----
 drivers/gpu/drm/amd/display/dc/inc/link.h             | 9 ++++-----
 drivers/gpu/drm/amd/display/dc/link/link_detection.c  | 4 ++--
 drivers/gpu/drm/amd/display/dc/link/link_detection.h  | 9 ++++-----
 5 files changed, 18 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link_exports.c b/drivers/gpu/drm/amd/display/dc/core/dc_link_exports.c
index 71e15da4bb69..b6f03ac16cad 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_link_exports.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_link_exports.c
@@ -278,11 +278,10 @@ unsigned int dc_dp_trace_get_link_loss_count(struct dc_link *link)
 	return link->dc->link_srv->dp_trace_get_link_loss_count(link);
 }
 
-struct dc_sink *dc_link_add_remote_sink(
-		struct dc_link *link,
-		const uint8_t *edid,
-		int len,
-		struct dc_sink_init_data *init_data)
+struct dc_sink *dc_link_add_remote_sink(struct dc_link *link,
+					const void *edid,
+					int len,
+					struct dc_sink_init_data *init_data)
 {
 	return link->dc->link_srv->add_remote_sink(link, edid, len, init_data);
 }
diff --git a/drivers/gpu/drm/amd/display/dc/dc.h b/drivers/gpu/drm/amd/display/dc/dc.h
index 4984700b5f1b..86feef038de6 100644
--- a/drivers/gpu/drm/amd/display/dc/dc.h
+++ b/drivers/gpu/drm/amd/display/dc/dc.h
@@ -1911,11 +1911,10 @@ struct dc_sink_init_data;
  * @len - size of the edid in byte
  * @init_data -
  */
-struct dc_sink *dc_link_add_remote_sink(
-		struct dc_link *dc_link,
-		const uint8_t *edid,
-		int len,
-		struct dc_sink_init_data *init_data);
+struct dc_sink *dc_link_add_remote_sink(struct dc_link *dc_link,
+					const void *edid,
+					int len,
+					struct dc_sink_init_data *init_data);
 
 /* Remove remote sink from a link with dc_connection_mst_branch connection type.
  * @link - link the sink should be removed from
diff --git a/drivers/gpu/drm/amd/display/dc/inc/link.h b/drivers/gpu/drm/amd/display/dc/inc/link.h
index 7d16351bba99..5b30d6e6bea1 100644
--- a/drivers/gpu/drm/amd/display/dc/inc/link.h
+++ b/drivers/gpu/drm/amd/display/dc/inc/link.h
@@ -107,11 +107,10 @@ struct link_service {
 	bool (*detect_link)(struct dc_link *link, enum dc_detect_reason reason);
 	bool (*detect_connection_type)(struct dc_link *link,
 			enum dc_connection_type *type);
-	struct dc_sink *(*add_remote_sink)(
-			struct dc_link *link,
-			const uint8_t *edid,
-			int len,
-			struct dc_sink_init_data *init_data);
+	struct dc_sink *(*add_remote_sink)(struct dc_link *link,
+					   const void *edid,
+					   int len,
+					   struct dc_sink_init_data *init_data);
 	void (*remove_remote_sink)(struct dc_link *link, struct dc_sink *sink);
 	bool (*get_hpd_state)(struct dc_link *link);
 	struct gpio *(*get_hpd_gpio)(struct dc_bios *dcb,
diff --git a/drivers/gpu/drm/amd/display/dc/link/link_detection.c b/drivers/gpu/drm/amd/display/dc/link/link_detection.c
index 5ac361c59371..863c24fe1117 100644
--- a/drivers/gpu/drm/amd/display/dc/link/link_detection.c
+++ b/drivers/gpu/drm/amd/display/dc/link/link_detection.c
@@ -1385,7 +1385,7 @@ static bool link_add_remote_sink_helper(struct dc_link *dc_link, struct dc_sink
 
 struct dc_sink *link_add_remote_sink(
 		struct dc_link *link,
-		const uint8_t *edid,
+		const void *edid,
 		int len,
 		struct dc_sink_init_data *init_data)
 {
@@ -1412,7 +1412,7 @@ struct dc_sink *link_add_remote_sink(
 	if (!dc_sink)
 		return NULL;
 
-	memmove(dc_sink->dc_edid.raw_edid, edid, len);
+	memmove(dc_sink->dc_edid.raw_edid, (const uint8_t *) edid, len);
 	dc_sink->dc_edid.length = len;
 
 	if (!link_add_remote_sink_helper(
diff --git a/drivers/gpu/drm/amd/display/dc/link/link_detection.h b/drivers/gpu/drm/amd/display/dc/link/link_detection.h
index 7da05078721e..9cd3aa36c7d8 100644
--- a/drivers/gpu/drm/amd/display/dc/link/link_detection.h
+++ b/drivers/gpu/drm/amd/display/dc/link/link_detection.h
@@ -29,11 +29,10 @@
 bool link_detect(struct dc_link *link, enum dc_detect_reason reason);
 bool link_detect_connection_type(struct dc_link *link,
 		enum dc_connection_type *type);
-struct dc_sink *link_add_remote_sink(
-		struct dc_link *link,
-		const uint8_t *edid,
-		int len,
-		struct dc_sink_init_data *init_data);
+struct dc_sink *link_add_remote_sink(struct dc_link *link,
+				     const void *edid,
+				     int len,
+				     struct dc_sink_init_data *init_data);
 void link_remove_remote_sink(struct dc_link *link, struct dc_sink *sink);
 bool link_reset_cur_dp_mst_topology(struct dc_link *link);
 const struct dc_link_status *link_get_status(const struct dc_link *link);
-- 
2.47.2

[PATCH v4 10/14] drm/amd/display: add a mid-layer file to handle EDID in DC

[Melissa Wen]

From: Rodrigo Siqueira <siqueira@igalia.com>

Since DC is a shared code, this commit introduces a new file to work as
a mid-layer in DC for the edid manipulation.

v3:
- rebase on top of asdn

Signed-off-by: Rodrigo Siqueira <siqueira@igalia.com>
Co-developed-by: Melissa Wen <mwen@igalia.com>
Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/Makefile    |  1 +
 .../gpu/drm/amd/display/amdgpu_dm/dc_edid.c   | 19 +++++++++++++++++++
 .../gpu/drm/amd/display/amdgpu_dm/dc_edid.h   | 11 +++++++++++
 .../drm/amd/display/dc/link/link_detection.c  | 17 +++--------------
 4 files changed, 34 insertions(+), 14 deletions(-)
 create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
 create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile b/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
index 7329b8cc2576..09cb94d8e0e4 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
@@ -39,6 +39,7 @@ AMDGPUDM = \
 	amdgpu_dm_psr.o \
 	amdgpu_dm_replay.o \
 	amdgpu_dm_quirks.o \
+	dc_edid.o \
 	amdgpu_dm_wb.o
 
 ifdef CONFIG_DRM_AMD_DC_FP
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
new file mode 100644
index 000000000000..fab873b091f5
--- /dev/null
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: MIT
+#include "amdgpu_dm/dc_edid.h"
+#include "dc.h"
+
+bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
+			  struct dc_sink *current_sink)
+{
+	struct dc_edid *old_edid = &prev_sink->dc_edid;
+	struct dc_edid *new_edid = &current_sink->dc_edid;
+
+       if (old_edid->length != new_edid->length)
+               return false;
+
+       if (new_edid->length == 0)
+               return false;
+
+       return (memcmp(old_edid->raw_edid,
+                      new_edid->raw_edid, new_edid->length) == 0);
+}
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
new file mode 100644
index 000000000000..7e3b1177bc8a
--- /dev/null
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: MIT */
+
+#ifndef __DC_EDID_H__
+#define __DC_EDID_H__
+
+#include "dc.h"
+
+bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
+			  struct dc_sink *current_sink);
+
+#endif /* __DC_EDID_H__ */
diff --git a/drivers/gpu/drm/amd/display/dc/link/link_detection.c b/drivers/gpu/drm/amd/display/dc/link/link_detection.c
index 863c24fe1117..344356e26f8b 100644
--- a/drivers/gpu/drm/amd/display/dc/link/link_detection.c
+++ b/drivers/gpu/drm/amd/display/dc/link/link_detection.c
@@ -48,6 +48,8 @@
 #include "dm_helpers.h"
 #include "clk_mgr.h"
 
+#include "dc_edid.h"
+
  // Offset DPCD 050Eh == 0x5A
 #define MST_HUB_ID_0x5A  0x5A
 
@@ -617,18 +619,6 @@ static bool detect_dp(struct dc_link *link,
 	return true;
 }
 
-static bool is_same_edid(struct dc_edid *old_edid, struct dc_edid *new_edid)
-{
-	if (old_edid->length != new_edid->length)
-		return false;
-
-	if (new_edid->length == 0)
-		return false;
-
-	return (memcmp(old_edid->raw_edid,
-		       new_edid->raw_edid, new_edid->length) == 0);
-}
-
 static bool wait_for_entering_dp_alt_mode(struct dc_link *link)
 {
 
@@ -1105,8 +1095,7 @@ static bool detect_link_and_local_sink(struct dc_link *link,
 		// Check if edid is the same
 		if ((prev_sink) &&
 		    (edid_status == EDID_THE_SAME || edid_status == EDID_OK))
-			same_edid = is_same_edid(&prev_sink->dc_edid,
-						 &sink->dc_edid);
+			same_edid = dc_edid_is_same_edid(prev_sink, sink);
 
 		if (sink->edid_caps.panel_patch.skip_scdc_overwrite)
 			link->ctx->dc->debug.hdmi20_disable = true;
-- 
2.47.2

[PATCH v4 11/14] drm/amd/display: create a function to fill dc_sink with edid data

[Melissa Wen]

From: Rodrigo Siqueira <siqueira@igalia.com>

As part of the effort of stopping using raw edid, this commit move the
copy of the edid in DC to a dedicated function that will allow the usage
of drm_edid in the next steps.

v3:
- remove superfulous cast (jani)

Signed-off-by: Rodrigo Siqueira <siqueira@igalia.com>
Co-developer--by: Melissa Wen <mwen@igalia.com>
Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c      | 8 ++++++++
 drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h      | 2 ++
 drivers/gpu/drm/amd/display/dc/link/link_detection.c | 3 +--
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
index fab873b091f5..a90545b176cc 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
@@ -17,3 +17,11 @@ bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
        return (memcmp(old_edid->raw_edid,
                       new_edid->raw_edid, new_edid->length) == 0);
 }
+
+void dc_edid_copy_edid_to_dc(struct dc_sink *dc_sink,
+			     const void *edid,
+			     int len)
+{
+	memmove(dc_sink->dc_edid.raw_edid, edid, len);
+	dc_sink->dc_edid.length = len;
+}
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
index 7e3b1177bc8a..f42cd5bbc730 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
@@ -7,5 +7,7 @@
 
 bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
 			  struct dc_sink *current_sink);
+void dc_edid_copy_edid_to_dc(struct dc_sink *dc_sink,
+			     const void *edid, int len);
 
 #endif /* __DC_EDID_H__ */
diff --git a/drivers/gpu/drm/amd/display/dc/link/link_detection.c b/drivers/gpu/drm/amd/display/dc/link/link_detection.c
index 344356e26f8b..c28072f980cc 100644
--- a/drivers/gpu/drm/amd/display/dc/link/link_detection.c
+++ b/drivers/gpu/drm/amd/display/dc/link/link_detection.c
@@ -1401,8 +1401,7 @@ struct dc_sink *link_add_remote_sink(
 	if (!dc_sink)
 		return NULL;
 
-	memmove(dc_sink->dc_edid.raw_edid, (const uint8_t *) edid, len);
-	dc_sink->dc_edid.length = len;
+	dc_edid_copy_edid_to_dc(dc_sink, edid, len);
 
 	if (!link_add_remote_sink_helper(
 			link,
-- 
2.47.2

[PATCH v4 12/14] drm/edid: introduce a helper that compares edid data from two drm_edid

[Melissa Wen]

AMD driver has a function used to compare if two edid are the same; this
is useful to some of the link detection algorithms implemented by
amdgpu. Since the amdgpu function can be helpful for other drivers, this
commit abstracts the AMD function to make it available at the DRM level
by wrapping existent drm_edid_eq().

v2:
- rename drm_edid_eq to drm_edid_eq_buf (jani)
- add NULL checks (jani)

v3:
- fix kernel-doc (jani)
- fix parameter names

Reviewed-by: Jani Nikula <jani.nikula@intel.com>
Co-developed-by: Rodrigo Siqueira <siqueira@igalia.com>
Signed-off-by: Rodrigo Siqueira <siqueira@igalia.com>
Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/drm_edid.c | 24 +++++++++++++++++++++---
 include/drm/drm_edid.h     |  2 ++
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index d5772a3d27f1..056e070b2f55 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1820,8 +1820,8 @@ static bool edid_block_is_zero(const void *edid)
 	return mem_is_zero(edid, EDID_LENGTH);
 }
 
-static bool drm_edid_eq(const struct drm_edid *drm_edid,
-			const void *raw_edid, size_t raw_edid_size)
+static bool drm_edid_eq_buf(const struct drm_edid *drm_edid,
+			    const void *raw_edid, size_t raw_edid_size)
 {
 	bool edid1_present = drm_edid && drm_edid->edid && drm_edid->size;
 	bool edid2_present = raw_edid && raw_edid_size;
@@ -6915,7 +6915,7 @@ static int _drm_edid_connector_property_update(struct drm_connector *connector,
 		const void *old_edid = connector->edid_blob_ptr->data;
 		size_t old_edid_size = connector->edid_blob_ptr->length;
 
-		if (old_edid && !drm_edid_eq(drm_edid, old_edid, old_edid_size)) {
+		if (old_edid && !drm_edid_eq_buf(drm_edid, old_edid, old_edid_size)) {
 			connector->epoch_counter++;
 			drm_dbg_kms(dev, "[CONNECTOR:%d:%s] EDID changed, epoch counter %llu\n",
 				    connector->base.id, connector->name,
@@ -7520,3 +7520,21 @@ bool drm_edid_is_digital(const struct drm_edid *drm_edid)
 		drm_edid->edid->input & DRM_EDID_INPUT_DIGITAL;
 }
 EXPORT_SYMBOL(drm_edid_is_digital);
+
+/**
+ * drm_edid_eq - Check if EDIDs are equal
+ *
+ * @drm_edid_1: first drm_edid to compare edid
+ * @drm_edid_2: second drm_edid to compare edid
+ *
+ * Return true if EDIDs are equal.
+ */
+bool drm_edid_eq(const struct drm_edid *drm_edid_1,
+		 const struct drm_edid *drm_edid_2)
+{
+	const void *edid_1 = drm_edid_1 ? drm_edid_1->edid : NULL;
+	size_t edid_1_size = drm_edid_1 ? drm_edid_1->size : 0;
+
+	return drm_edid_eq_buf(drm_edid_2, edid_1, edid_1_size);
+}
+EXPORT_SYMBOL(drm_edid_eq);
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 960592167486..e7a9a4928b97 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -469,6 +469,8 @@ int drm_edid_connector_update(struct drm_connector *connector,
 			      const struct drm_edid *edid);
 int drm_edid_connector_add_modes(struct drm_connector *connector);
 bool drm_edid_is_digital(const struct drm_edid *drm_edid);
+bool drm_edid_eq(const struct drm_edid *drm_edid_first,
+			 const struct drm_edid *drm_edid_second);
 void drm_edid_get_product_id(const struct drm_edid *drm_edid,
 			     struct drm_edid_product_id *id);
 void drm_edid_print_product_id(struct drm_printer *p,
-- 
2.47.2

[PATCH v4 13/14] drm/amd/display: add drm_edid to dc_sink

[Melissa Wen]

Add Linux opaque object to dc_sink for storing edid data cross driver,
drm_edid. Also include the Linux call to free this object, the
drm_edid_free()

v3:
- remove uneccessary include (jani)

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c | 6 ++++++
 drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h | 1 +
 drivers/gpu/drm/amd/display/dc/core/dc_sink.c   | 3 +++
 drivers/gpu/drm/amd/display/dc/dc.h             | 1 +
 include/drm/drm_edid.h                          | 4 ++--
 5 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
index a90545b176cc..9e86dc15557b 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: MIT
 #include "amdgpu_dm/dc_edid.h"
 #include "dc.h"
+#include <drm/drm_edid.h>
 
 bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
 			  struct dc_sink *current_sink)
@@ -25,3 +26,8 @@ void dc_edid_copy_edid_to_dc(struct dc_sink *dc_sink,
 	memmove(dc_sink->dc_edid.raw_edid, edid, len);
 	dc_sink->dc_edid.length = len;
 }
+
+void dc_edid_sink_edid_free(struct dc_sink *sink)
+{
+	drm_edid_free(sink->drm_edid);
+}
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
index f42cd5bbc730..2c76768be459 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
@@ -9,5 +9,6 @@ bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
 			  struct dc_sink *current_sink);
 void dc_edid_copy_edid_to_dc(struct dc_sink *dc_sink,
 			     const void *edid, int len);
+void dc_edid_sink_edid_free(struct dc_sink *sink);
 
 #endif /* __DC_EDID_H__ */
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_sink.c b/drivers/gpu/drm/amd/display/dc/core/dc_sink.c
index 455fa5dd1420..3774a3245506 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_sink.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_sink.c
@@ -26,6 +26,7 @@
 #include "dm_services.h"
 #include "dm_helpers.h"
 #include "core_types.h"
+#include "dc_edid.h"
 
 /*******************************************************************************
  * Private functions
@@ -65,6 +66,8 @@ void dc_sink_retain(struct dc_sink *sink)
 static void dc_sink_free(struct kref *kref)
 {
 	struct dc_sink *sink = container_of(kref, struct dc_sink, refcount);
+
+	dc_edid_sink_edid_free(sink);
 	kfree(sink->dc_container_id);
 	kfree(sink);
 }
diff --git a/drivers/gpu/drm/amd/display/dc/dc.h b/drivers/gpu/drm/amd/display/dc/dc.h
index 86feef038de6..cf56a0405a4f 100644
--- a/drivers/gpu/drm/amd/display/dc/dc.h
+++ b/drivers/gpu/drm/amd/display/dc/dc.h
@@ -2466,6 +2466,7 @@ struct scdc_caps {
 struct dc_sink {
 	enum signal_type sink_signal;
 	struct dc_edid dc_edid; /* raw edid */
+	const struct drm_edid *drm_edid; /* Linux DRM edid*/
 	struct dc_edid_caps edid_caps; /* parse display caps */
 	struct dc_container_id *dc_container_id;
 	uint32_t dongle_max_pix_clk;
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index e7a9a4928b97..8617d2285f38 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -469,8 +469,8 @@ int drm_edid_connector_update(struct drm_connector *connector,
 			      const struct drm_edid *edid);
 int drm_edid_connector_add_modes(struct drm_connector *connector);
 bool drm_edid_is_digital(const struct drm_edid *drm_edid);
-bool drm_edid_eq(const struct drm_edid *drm_edid_first,
-			 const struct drm_edid *drm_edid_second);
+bool drm_edid_eq(const struct drm_edid *drm_edid_1,
+		 const struct drm_edid *drm_edid_2);
 void drm_edid_get_product_id(const struct drm_edid *drm_edid,
 			     struct drm_edid_product_id *id);
 void drm_edid_print_product_id(struct drm_printer *p,
-- 
2.47.2

[PATCH v4 14/14] drm/amd/display: move dc_sink from dc_edid to drm_edid

[Melissa Wen]

Reduce direct handling of edid data by resorting to drm helpers that
deal with this info inside drm_edid infrastructure.

v3:
- use dc_edid_sink_edid_free in link_detection

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 26 +++++++------------
 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 24 +++++------------
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c   | 21 +++++----------
 .../gpu/drm/amd/display/amdgpu_dm/dc_edid.c   | 26 +++++++++----------
 .../gpu/drm/amd/display/amdgpu_dm/dc_edid.h   |  1 +
 .../drm/amd/display/dc/link/link_detection.c  |  3 ++-
 6 files changed, 40 insertions(+), 61 deletions(-)

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 c7efeb9f38b6..ec33a6236e37 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -68,6 +68,7 @@
 #endif
 #include "amdgpu_dm_psr.h"
 #include "amdgpu_dm_replay.h"
+#include "dc_edid.h"
 
 #include "ivsrcid/ivsrcid_vislands30.h"
 
@@ -3708,6 +3709,8 @@ void amdgpu_dm_update_connector_after_detect(
 	 * 2. Send an event and let userspace tell us what to do
 	 */
 	if (sink) {
+		const struct drm_edid *drm_edid = sink->drm_edid;
+
 		/*
 		 * TODO: check if we still need the S3 mode update workaround.
 		 * If yes, put it here.
@@ -3719,16 +3722,15 @@ void amdgpu_dm_update_connector_after_detect(
 
 		aconnector->dc_sink = sink;
 		dc_sink_retain(aconnector->dc_sink);
-		if (sink->dc_edid.length == 0) {
+
+		if (!drm_edid_valid(drm_edid)) {
 			aconnector->drm_edid = NULL;
 			hdmi_cec_unset_edid(aconnector);
 			if (aconnector->dc_link->aux_mode) {
 				drm_dp_cec_unset_edid(&aconnector->dm_dp_aux.aux);
 			}
 		} else {
-			const struct edid *edid = (const struct edid *)sink->dc_edid.raw_edid;
-
-			aconnector->drm_edid = drm_edid_alloc(edid, sink->dc_edid.length);
+			aconnector->drm_edid = drm_edid_dup(sink->drm_edid);
 			drm_edid_connector_update(connector, aconnector->drm_edid);
 
 			hdmi_cec_set_edid(aconnector);
@@ -7378,12 +7380,8 @@ static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector)
 	aconnector->drm_edid = drm_edid;
 	/* Update emulated (virtual) sink's EDID */
 	if (dc_em_sink && dc_link) {
-		// FIXME: Get rid of drm_edid_raw()
-		const struct edid *edid = drm_edid_raw(drm_edid);
-
 		memset(&dc_em_sink->edid_caps, 0, sizeof(struct dc_edid_caps));
-		memmove(dc_em_sink->dc_edid.raw_edid, edid,
-			(edid->extensions + 1) * EDID_LENGTH);
+		dc_edid_copy_edid_to_dc(dc_em_sink, drm_edid, 0);
 		dm_helpers_parse_edid_caps(dc_link, dc_em_sink);
 	}
 }
@@ -7416,7 +7414,6 @@ static void create_eml_sink(struct amdgpu_dm_connector *aconnector)
 			.sink_signal = SIGNAL_TYPE_VIRTUAL
 	};
 	const struct drm_edid *drm_edid;
-	const struct edid *edid;
 	struct i2c_adapter *ddc;
 
 	if (dc_link && dc_link->aux_mode)
@@ -7436,12 +7433,9 @@ static void create_eml_sink(struct amdgpu_dm_connector *aconnector)
 
 	aconnector->drm_edid = drm_edid;
 
-	edid = drm_edid_raw(drm_edid); // FIXME: Get rid of drm_edid_raw()
-	aconnector->dc_em_sink = dc_link_add_remote_sink(
-		aconnector->dc_link,
-		(uint8_t *)edid,
-		(edid->extensions + 1) * EDID_LENGTH,
-		&init_params);
+	aconnector->dc_em_sink = dc_link_add_remote_sink(aconnector->dc_link,
+							 drm_edid, 0,
+							 &init_params);
 
 	if (aconnector->base.force == DRM_FORCE_ON) {
 		aconnector->dc_sink = aconnector->dc_link->local_sink ?
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 abfce44dcee7..3e9d04760c21 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
@@ -48,6 +48,7 @@
 #include "dm_helpers.h"
 #include "ddc_service_types.h"
 #include "clk_mgr.h"
+#include "dc_edid.h"
 
 static void apply_edid_quirks(struct drm_device *dev,
 			      const struct drm_edid *drm_edid,
@@ -100,20 +101,16 @@ enum dc_edid_status dm_helpers_parse_edid_caps(struct dc_link *link,
 	struct amdgpu_dm_connector *aconnector = link->priv;
 	struct drm_connector *connector = &aconnector->base;
 	struct drm_device *dev = connector->dev;
-	struct edid *edid_buf;
-	const struct drm_edid *drm_edid;
+	const struct drm_edid *drm_edid = sink->drm_edid;
 	struct drm_edid_product_id product_id;
 	struct dc_edid_caps *edid_caps = &sink->edid_caps;
 	int sad_count;
 	int i = 0;
 	enum dc_edid_status result = EDID_OK;
 
-	edid_buf = (struct edid *) &sink->dc_edid.raw_edid;
-	if (!edid_caps || !edid_buf)
+	if (!edid_caps || !drm_edid)
 		return EDID_BAD_INPUT;
 
-	drm_edid = drm_edid_alloc(edid_buf, EDID_LENGTH * (edid_buf->extensions + 1));
-
 	if (!drm_edid_valid(drm_edid))
 		result = EDID_BAD_CHECKSUM;
 
@@ -135,10 +132,8 @@ enum dc_edid_status dm_helpers_parse_edid_caps(struct dc_link *link,
 	apply_edid_quirks(dev, drm_edid, edid_caps);
 
 	sad_count = drm_eld_sad_count(connector->eld);
-	if (sad_count <= 0) {
-		drm_edid_free(drm_edid);
+	if (sad_count <= 0)
 		return result;
-	}
 
 	edid_caps->audio_mode_count = min(sad_count, DC_MAX_AUDIO_DESC_COUNT);
 	for (i = 0; i < edid_caps->audio_mode_count; ++i) {
@@ -158,8 +153,6 @@ enum dc_edid_status dm_helpers_parse_edid_caps(struct dc_link *link,
 	else
 		edid_caps->speaker_flags = DEFAULT_SPEAKER_LOCATION;
 
-	drm_edid_free(drm_edid);
-
 	return result;
 }
 
@@ -991,7 +984,6 @@ enum dc_edid_status dm_helpers_read_local_edid(
 	int retry = 3;
 	enum dc_edid_status edid_status;
 	const struct drm_edid *drm_edid;
-	const struct edid *edid;
 
 	if (link->aux_mode)
 		ddc = &aconnector->dm_dp_aux.aux.ddc;
@@ -1021,11 +1013,7 @@ enum dc_edid_status dm_helpers_read_local_edid(
 		if (!drm_edid)
 			return EDID_NO_RESPONSE;
 
-		edid = drm_edid_raw(drm_edid); // FIXME: Get rid of drm_edid_raw()
-		sink->dc_edid.length = EDID_LENGTH * (edid->extensions + 1);
-		memmove(sink->dc_edid.raw_edid, (uint8_t *)edid, sink->dc_edid.length);
-
-		/* We don't need the original edid anymore */
+		sink->drm_edid = drm_edid_dup(drm_edid);
 		drm_edid_free(drm_edid);
 
 		edid_status = dm_helpers_parse_edid_caps(link, sink);
@@ -1051,6 +1039,8 @@ enum dc_edid_status dm_helpers_read_local_edid(
 
 		test_response.bits.EDID_CHECKSUM_WRITE = 1;
 
+		// TODO: drm_edid doesn't have a helper for dp_write_dpcd yet
+		dc_edid_copy_edid_to_sink(sink);
 		dm_helpers_dp_write_dpcd(ctx,
 					link,
 					DP_TEST_EDID_CHECKSUM,
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index 7187d5aedf0a..5ca3e668c8aa 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
@@ -359,12 +359,10 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
 					.link = aconnector->dc_link,
 					.sink_signal = SIGNAL_TYPE_DISPLAY_PORT_MST };
 
-				dc_sink = dc_link_add_remote_sink(
-					aconnector->dc_link,
-					NULL,
-					0,
-					&init_params);
-
+				dc_sink = dc_link_add_remote_sink(aconnector->dc_link,
+								  NULL,
+								  0,
+								  &init_params);
 				if (!dc_sink) {
 					DRM_ERROR("Unable to add a remote sink\n");
 					return 0;
@@ -397,15 +395,10 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
 		struct dc_sink_init_data init_params = {
 				.link = aconnector->dc_link,
 				.sink_signal = SIGNAL_TYPE_DISPLAY_PORT_MST };
-		const struct edid *edid;
-
-		edid = drm_edid_raw(aconnector->drm_edid); // FIXME: Get rid of drm_edid_raw()
-		dc_sink = dc_link_add_remote_sink(
-			aconnector->dc_link,
-			(uint8_t *)edid,
-			(edid->extensions + 1) * EDID_LENGTH,
-			&init_params);
 
+		dc_sink = dc_link_add_remote_sink(aconnector->dc_link,
+						  aconnector->drm_edid, 0,
+						  &init_params);
 		if (!dc_sink) {
 			DRM_ERROR("Unable to add a remote sink\n");
 			return 0;
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
index 9e86dc15557b..ce4a7f9e268a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
@@ -6,25 +6,25 @@
 bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
 			  struct dc_sink *current_sink)
 {
-	struct dc_edid *old_edid = &prev_sink->dc_edid;
-	struct dc_edid *new_edid = &current_sink->dc_edid;
-
-       if (old_edid->length != new_edid->length)
-               return false;
-
-       if (new_edid->length == 0)
-               return false;
-
-       return (memcmp(old_edid->raw_edid,
-                      new_edid->raw_edid, new_edid->length) == 0);
+	return drm_edid_eq(prev_sink->drm_edid, current_sink->drm_edid);
 }
 
 void dc_edid_copy_edid_to_dc(struct dc_sink *dc_sink,
 			     const void *edid,
 			     int len)
 {
-	memmove(dc_sink->dc_edid.raw_edid, edid, len);
-	dc_sink->dc_edid.length = len;
+	dc_sink->drm_edid = drm_edid_dup((const struct drm_edid *) edid);
+}
+
+void dc_edid_copy_edid_to_sink(struct dc_sink *sink)
+{
+	const struct edid *edid;
+	uint32_t edid_length;
+
+	edid = drm_edid_raw(sink->drm_edid); // FIXME: Get rid of drm_edid_raw()
+	edid_length = EDID_LENGTH * (edid->extensions + 1);
+	memcpy(sink->dc_edid.raw_edid, (uint8_t *) edid, edid_length);
+	sink->dc_edid.length = edid_length;
 }
 
 void dc_edid_sink_edid_free(struct dc_sink *sink)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
index 2c76768be459..a95cc6ccc743 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
@@ -9,6 +9,7 @@ bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
 			  struct dc_sink *current_sink);
 void dc_edid_copy_edid_to_dc(struct dc_sink *dc_sink,
 			     const void *edid, int len);
+void dc_edid_copy_edid_to_sink(struct dc_sink *sink);
 void dc_edid_sink_edid_free(struct dc_sink *sink);
 
 #endif /* __DC_EDID_H__ */
diff --git a/drivers/gpu/drm/amd/display/dc/link/link_detection.c b/drivers/gpu/drm/amd/display/dc/link/link_detection.c
index c28072f980cc..bddcfd8f02ba 100644
--- a/drivers/gpu/drm/amd/display/dc/link/link_detection.c
+++ b/drivers/gpu/drm/amd/display/dc/link/link_detection.c
@@ -1133,6 +1133,7 @@ static bool detect_link_and_local_sink(struct dc_link *link,
 			dp_trace_init(link);
 
 		/* Connectivity log: detection */
+		dc_edid_copy_edid_to_sink(sink);
 		for (i = 0; i < sink->dc_edid.length / DC_EDID_BLOCK_SIZE; i++) {
 			CONN_DATA_DETECT(link,
 					 &sink->dc_edid.raw_edid[i * DC_EDID_BLOCK_SIZE],
@@ -1415,7 +1416,7 @@ struct dc_sink *link_add_remote_sink(
 	 * parsing fails
 	 */
 	if (edid_status != EDID_OK && edid_status != EDID_PARTIAL_VALID) {
-		dc_sink->dc_edid.length = 0;
+		dc_edid_sink_edid_free(dc_sink);
 		dm_error("Bad EDID, status%d!\n", edid_status);
 	}
 
-- 
2.47.2

Re: [PATCH v4 13/14] drm/amd/display: add drm_edid to dc_sink

[Mario Limonciello]

On 6/13/2025 7:58 AM, Melissa Wen wrote:
> Add Linux opaque object to dc_sink for storing edid data cross driver,
> drm_edid. Also include the Linux call to free this object, the
> drm_edid_free()
> 
> v3:
> - remove uneccessary include (jani)
> 
> Signed-off-by: Melissa Wen <mwen@igalia.com>
> ---
>   drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c | 6 ++++++
>   drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h | 1 +
>   drivers/gpu/drm/amd/display/dc/core/dc_sink.c   | 3 +++
>   drivers/gpu/drm/amd/display/dc/dc.h             | 1 +
>   include/drm/drm_edid.h                          | 4 ++--
>   5 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
> index a90545b176cc..9e86dc15557b 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
> @@ -1,6 +1,7 @@
>   // SPDX-License-Identifier: MIT
>   #include "amdgpu_dm/dc_edid.h"
>   #include "dc.h"
> +#include <drm/drm_edid.h>
>   
>   bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
>   			  struct dc_sink *current_sink)
> @@ -25,3 +26,8 @@ void dc_edid_copy_edid_to_dc(struct dc_sink *dc_sink,
>   	memmove(dc_sink->dc_edid.raw_edid, edid, len);
>   	dc_sink->dc_edid.length = len;
>   }
> +
> +void dc_edid_sink_edid_free(struct dc_sink *sink)
> +{
> +	drm_edid_free(sink->drm_edid);
> +}
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
> index f42cd5bbc730..2c76768be459 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
> @@ -9,5 +9,6 @@ bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
>   			  struct dc_sink *current_sink);
>   void dc_edid_copy_edid_to_dc(struct dc_sink *dc_sink,
>   			     const void *edid, int len);
> +void dc_edid_sink_edid_free(struct dc_sink *sink);
>   
>   #endif /* __DC_EDID_H__ */
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_sink.c b/drivers/gpu/drm/amd/display/dc/core/dc_sink.c
> index 455fa5dd1420..3774a3245506 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc_sink.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_sink.c
> @@ -26,6 +26,7 @@
>   #include "dm_services.h"
>   #include "dm_helpers.h"
>   #include "core_types.h"
> +#include "dc_edid.h"
>   
>   /*******************************************************************************
>    * Private functions
> @@ -65,6 +66,8 @@ void dc_sink_retain(struct dc_sink *sink)
>   static void dc_sink_free(struct kref *kref)
>   {
>   	struct dc_sink *sink = container_of(kref, struct dc_sink, refcount);
> +
> +	dc_edid_sink_edid_free(sink);
>   	kfree(sink->dc_container_id);
>   	kfree(sink);
>   }
> diff --git a/drivers/gpu/drm/amd/display/dc/dc.h b/drivers/gpu/drm/amd/display/dc/dc.h
> index 86feef038de6..cf56a0405a4f 100644
> --- a/drivers/gpu/drm/amd/display/dc/dc.h
> +++ b/drivers/gpu/drm/amd/display/dc/dc.h
> @@ -2466,6 +2466,7 @@ struct scdc_caps {
>   struct dc_sink {
>   	enum signal_type sink_signal;
>   	struct dc_edid dc_edid; /* raw edid */
> +	const struct drm_edid *drm_edid; /* Linux DRM edid*/

Don't you need a forward declaration for 'struct drm_edid' in dc.h to be 
able to do this?

Also you're missing a space at the end of the comment before the '*/'.

>   	struct dc_edid_caps edid_caps; /* parse display caps */
>   	struct dc_container_id *dc_container_id;
>   	uint32_t dongle_max_pix_clk;
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index e7a9a4928b97..8617d2285f38 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -469,8 +469,8 @@ int drm_edid_connector_update(struct drm_connector *connector,
>   			      const struct drm_edid *edid);
>   int drm_edid_connector_add_modes(struct drm_connector *connector);
>   bool drm_edid_is_digital(const struct drm_edid *drm_edid);
> -bool drm_edid_eq(const struct drm_edid *drm_edid_first,
> -			 const struct drm_edid *drm_edid_second);
> +bool drm_edid_eq(const struct drm_edid *drm_edid_1,
> +		 const struct drm_edid *drm_edid_2);
>   void drm_edid_get_product_id(const struct drm_edid *drm_edid,
>   			     struct drm_edid_product_id *id);
>   void drm_edid_print_product_id(struct drm_printer *p,

Re: [PATCH v4 13/14] drm/amd/display: add drm_edid to dc_sink

[Melissa Wen]

On 06/13, Mario Limonciello wrote:
> On 6/13/2025 7:58 AM, Melissa Wen wrote:
> > Add Linux opaque object to dc_sink for storing edid data cross driver,
> > drm_edid. Also include the Linux call to free this object, the
> > drm_edid_free()
> > 
> > v3:
> > - remove uneccessary include (jani)
> > 
> > Signed-off-by: Melissa Wen <mwen@igalia.com>
> > ---
> >   drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c | 6 ++++++
> >   drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h | 1 +
> >   drivers/gpu/drm/amd/display/dc/core/dc_sink.c   | 3 +++
> >   drivers/gpu/drm/amd/display/dc/dc.h             | 1 +
> >   include/drm/drm_edid.h                          | 4 ++--
> >   5 files changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
> > index a90545b176cc..9e86dc15557b 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
> > @@ -1,6 +1,7 @@
> >   // SPDX-License-Identifier: MIT
> >   #include "amdgpu_dm/dc_edid.h"
> >   #include "dc.h"
> > +#include <drm/drm_edid.h>
> >   bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
> >   			  struct dc_sink *current_sink)
> > @@ -25,3 +26,8 @@ void dc_edid_copy_edid_to_dc(struct dc_sink *dc_sink,
> >   	memmove(dc_sink->dc_edid.raw_edid, edid, len);
> >   	dc_sink->dc_edid.length = len;
> >   }
> > +
> > +void dc_edid_sink_edid_free(struct dc_sink *sink)
> > +{
> > +	drm_edid_free(sink->drm_edid);
> > +}
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
> > index f42cd5bbc730..2c76768be459 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
> > @@ -9,5 +9,6 @@ bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
> >   			  struct dc_sink *current_sink);
> >   void dc_edid_copy_edid_to_dc(struct dc_sink *dc_sink,
> >   			     const void *edid, int len);
> > +void dc_edid_sink_edid_free(struct dc_sink *sink);
> >   #endif /* __DC_EDID_H__ */
> > diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_sink.c b/drivers/gpu/drm/amd/display/dc/core/dc_sink.c
> > index 455fa5dd1420..3774a3245506 100644
> > --- a/drivers/gpu/drm/amd/display/dc/core/dc_sink.c
> > +++ b/drivers/gpu/drm/amd/display/dc/core/dc_sink.c
> > @@ -26,6 +26,7 @@
> >   #include "dm_services.h"
> >   #include "dm_helpers.h"
> >   #include "core_types.h"
> > +#include "dc_edid.h"
> >   /*******************************************************************************
> >    * Private functions
> > @@ -65,6 +66,8 @@ void dc_sink_retain(struct dc_sink *sink)
> >   static void dc_sink_free(struct kref *kref)
> >   {
> >   	struct dc_sink *sink = container_of(kref, struct dc_sink, refcount);
> > +
> > +	dc_edid_sink_edid_free(sink);
> >   	kfree(sink->dc_container_id);
> >   	kfree(sink);
> >   }
> > diff --git a/drivers/gpu/drm/amd/display/dc/dc.h b/drivers/gpu/drm/amd/display/dc/dc.h
> > index 86feef038de6..cf56a0405a4f 100644
> > --- a/drivers/gpu/drm/amd/display/dc/dc.h
> > +++ b/drivers/gpu/drm/amd/display/dc/dc.h
> > @@ -2466,6 +2466,7 @@ struct scdc_caps {
> >   struct dc_sink {
> >   	enum signal_type sink_signal;
> >   	struct dc_edid dc_edid; /* raw edid */
> > +	const struct drm_edid *drm_edid; /* Linux DRM edid*/
> 
> Don't you need a forward declaration for 'struct drm_edid' in dc.h to be
> able to do this?

I understand that, as it's just a pointer (the compiler knows the size)
and there is no circular dependencies between dc_sink and drm_edid, we
don't need a forward declaration. So I think we are fine also because
dc_sink->drm_edid dereference only happens in dc_edid.h that already
needs to include drm_edid.h for drm_edid helpers... but let me know if
I'm missing something.

> 
> Also you're missing a space at the end of the comment before the '*/'.

ack. I'll wait for more comments to send it fixed.

Thanks for reviewing.

Melissa

> 
> >   	struct dc_edid_caps edid_caps; /* parse display caps */
> >   	struct dc_container_id *dc_container_id;
> >   	uint32_t dongle_max_pix_clk;
> > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> > index e7a9a4928b97..8617d2285f38 100644
> > --- a/include/drm/drm_edid.h
> > +++ b/include/drm/drm_edid.h
> > @@ -469,8 +469,8 @@ int drm_edid_connector_update(struct drm_connector *connector,
> >   			      const struct drm_edid *edid);
> >   int drm_edid_connector_add_modes(struct drm_connector *connector);
> >   bool drm_edid_is_digital(const struct drm_edid *drm_edid);
> > -bool drm_edid_eq(const struct drm_edid *drm_edid_first,
> > -			 const struct drm_edid *drm_edid_second);
> > +bool drm_edid_eq(const struct drm_edid *drm_edid_1,
> > +		 const struct drm_edid *drm_edid_2);
> >   void drm_edid_get_product_id(const struct drm_edid *drm_edid,
> >   			     struct drm_edid_product_id *id);
> >   void drm_edid_print_product_id(struct drm_printer *p,
> 

Re: [PATCH v4 13/14] drm/amd/display: add drm_edid to dc_sink

[Mario Limonciello]

On 6/13/2025 12:42 PM, Melissa Wen wrote:
> On 06/13, Mario Limonciello wrote:
>> On 6/13/2025 7:58 AM, Melissa Wen wrote:
>>> Add Linux opaque object to dc_sink for storing edid data cross driver,
>>> drm_edid. Also include the Linux call to free this object, the
>>> drm_edid_free()
>>>
>>> v3:
>>> - remove uneccessary include (jani)
>>>
>>> Signed-off-by: Melissa Wen <mwen@igalia.com>
>>> ---
>>>    drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c | 6 ++++++
>>>    drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h | 1 +
>>>    drivers/gpu/drm/amd/display/dc/core/dc_sink.c   | 3 +++
>>>    drivers/gpu/drm/amd/display/dc/dc.h             | 1 +
>>>    include/drm/drm_edid.h                          | 4 ++--
>>>    5 files changed, 13 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
>>> index a90545b176cc..9e86dc15557b 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
>>> @@ -1,6 +1,7 @@
>>>    // SPDX-License-Identifier: MIT
>>>    #include "amdgpu_dm/dc_edid.h"
>>>    #include "dc.h"
>>> +#include <drm/drm_edid.h>
>>>    bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
>>>    			  struct dc_sink *current_sink)
>>> @@ -25,3 +26,8 @@ void dc_edid_copy_edid_to_dc(struct dc_sink *dc_sink,
>>>    	memmove(dc_sink->dc_edid.raw_edid, edid, len);
>>>    	dc_sink->dc_edid.length = len;
>>>    }
>>> +
>>> +void dc_edid_sink_edid_free(struct dc_sink *sink)
>>> +{
>>> +	drm_edid_free(sink->drm_edid);
>>> +}
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
>>> index f42cd5bbc730..2c76768be459 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
>>> @@ -9,5 +9,6 @@ bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
>>>    			  struct dc_sink *current_sink);
>>>    void dc_edid_copy_edid_to_dc(struct dc_sink *dc_sink,
>>>    			     const void *edid, int len);
>>> +void dc_edid_sink_edid_free(struct dc_sink *sink);
>>>    #endif /* __DC_EDID_H__ */
>>> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_sink.c b/drivers/gpu/drm/amd/display/dc/core/dc_sink.c
>>> index 455fa5dd1420..3774a3245506 100644
>>> --- a/drivers/gpu/drm/amd/display/dc/core/dc_sink.c
>>> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_sink.c
>>> @@ -26,6 +26,7 @@
>>>    #include "dm_services.h"
>>>    #include "dm_helpers.h"
>>>    #include "core_types.h"
>>> +#include "dc_edid.h"
>>>    /*******************************************************************************
>>>     * Private functions
>>> @@ -65,6 +66,8 @@ void dc_sink_retain(struct dc_sink *sink)
>>>    static void dc_sink_free(struct kref *kref)
>>>    {
>>>    	struct dc_sink *sink = container_of(kref, struct dc_sink, refcount);
>>> +
>>> +	dc_edid_sink_edid_free(sink);
>>>    	kfree(sink->dc_container_id);
>>>    	kfree(sink);
>>>    }
>>> diff --git a/drivers/gpu/drm/amd/display/dc/dc.h b/drivers/gpu/drm/amd/display/dc/dc.h
>>> index 86feef038de6..cf56a0405a4f 100644
>>> --- a/drivers/gpu/drm/amd/display/dc/dc.h
>>> +++ b/drivers/gpu/drm/amd/display/dc/dc.h
>>> @@ -2466,6 +2466,7 @@ struct scdc_caps {
>>>    struct dc_sink {
>>>    	enum signal_type sink_signal;
>>>    	struct dc_edid dc_edid; /* raw edid */
>>> +	const struct drm_edid *drm_edid; /* Linux DRM edid*/
>>
>> Don't you need a forward declaration for 'struct drm_edid' in dc.h to be
>> able to do this?
> 
> I understand that, as it's just a pointer (the compiler knows the size)
> and there is no circular dependencies between dc_sink and drm_edid, we
> don't need a forward declaration. So I think we are fine also because
> dc_sink->drm_edid dereference only happens in dc_edid.h that already
> needs to include drm_edid.h for drm_edid helpers... but let me know if
> I'm missing something.

Did you compile with CONFIG_WERROR or at least W=1?  I feel like I've 
seen issues with this in the past that the compiler doesn't like having
unknown types, even if a pointer.

But if it works with W=1 / CONFIG_WERROR I must have been thinking about 
a dereference case and thus no concerns on my side.

> 
>>
>> Also you're missing a space at the end of the comment before the '*/'.
> 
> ack. I'll wait for more comments to send it fixed.
> 
> Thanks for reviewing.
> 
> Melissa
> 
>>
>>>    	struct dc_edid_caps edid_caps; /* parse display caps */
>>>    	struct dc_container_id *dc_container_id;
>>>    	uint32_t dongle_max_pix_clk;
>>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>>> index e7a9a4928b97..8617d2285f38 100644
>>> --- a/include/drm/drm_edid.h
>>> +++ b/include/drm/drm_edid.h
>>> @@ -469,8 +469,8 @@ int drm_edid_connector_update(struct drm_connector *connector,
>>>    			      const struct drm_edid *edid);
>>>    int drm_edid_connector_add_modes(struct drm_connector *connector);
>>>    bool drm_edid_is_digital(const struct drm_edid *drm_edid);
>>> -bool drm_edid_eq(const struct drm_edid *drm_edid_first,
>>> -			 const struct drm_edid *drm_edid_second);
>>> +bool drm_edid_eq(const struct drm_edid *drm_edid_1,
>>> +		 const struct drm_edid *drm_edid_2);
>>>    void drm_edid_get_product_id(const struct drm_edid *drm_edid,
>>>    			     struct drm_edid_product_id *id);
>>>    void drm_edid_print_product_id(struct drm_printer *p,
>>

Re: [PATCH v4 14/14] drm/amd/display: move dc_sink from dc_edid to drm_edid

[Mario Limonciello]

On 6/13/2025 9:58 AM, Melissa Wen wrote:
> Reduce direct handling of edid data by resorting to drm helpers that
> deal with this info inside drm_edid infrastructure.
> 
> v3:
> - use dc_edid_sink_edid_free in link_detection
> 
> Signed-off-by: Melissa Wen <mwen@igalia.com>
> ---
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 26 +++++++------------
>   .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 24 +++++------------
>   .../display/amdgpu_dm/amdgpu_dm_mst_types.c   | 21 +++++----------
>   .../gpu/drm/amd/display/amdgpu_dm/dc_edid.c   | 26 +++++++++----------
>   .../gpu/drm/amd/display/amdgpu_dm/dc_edid.h   |  1 +
>   .../drm/amd/display/dc/link/link_detection.c  |  3 ++-
>   6 files changed, 40 insertions(+), 61 deletions(-)
> 
> 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 c7efeb9f38b6..ec33a6236e37 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -68,6 +68,7 @@
>   #endif
>   #include "amdgpu_dm_psr.h"
>   #include "amdgpu_dm_replay.h"
> +#include "dc_edid.h"
>   
>   #include "ivsrcid/ivsrcid_vislands30.h"
>   
> @@ -3708,6 +3709,8 @@ void amdgpu_dm_update_connector_after_detect(
>   	 * 2. Send an event and let userspace tell us what to do
>   	 */
>   	if (sink) {
> +		const struct drm_edid *drm_edid = sink->drm_edid;
> +
>   		/*
>   		 * TODO: check if we still need the S3 mode update workaround.
>   		 * If yes, put it here.
> @@ -3719,16 +3722,15 @@ void amdgpu_dm_update_connector_after_detect(
>   
>   		aconnector->dc_sink = sink;
>   		dc_sink_retain(aconnector->dc_sink);
> -		if (sink->dc_edid.length == 0) {
> +
> +		if (!drm_edid_valid(drm_edid)) {
>   			aconnector->drm_edid = NULL;
>   			hdmi_cec_unset_edid(aconnector);
>   			if (aconnector->dc_link->aux_mode) {
>   				drm_dp_cec_unset_edid(&aconnector->dm_dp_aux.aux);
>   			}
>   		} else {
> -			const struct edid *edid = (const struct edid *)sink->dc_edid.raw_edid;
> -
> -			aconnector->drm_edid = drm_edid_alloc(edid, sink->dc_edid.length);
> +			aconnector->drm_edid = drm_edid_dup(sink->drm_edid);
>   			drm_edid_connector_update(connector, aconnector->drm_edid);
>   
>   			hdmi_cec_set_edid(aconnector);
> @@ -7378,12 +7380,8 @@ static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector)
>   	aconnector->drm_edid = drm_edid;
>   	/* Update emulated (virtual) sink's EDID */
>   	if (dc_em_sink && dc_link) {
> -		// FIXME: Get rid of drm_edid_raw()
> -		const struct edid *edid = drm_edid_raw(drm_edid);
> -
>   		memset(&dc_em_sink->edid_caps, 0, sizeof(struct dc_edid_caps));
> -		memmove(dc_em_sink->dc_edid.raw_edid, edid,
> -			(edid->extensions + 1) * EDID_LENGTH);
> +		dc_edid_copy_edid_to_dc(dc_em_sink, drm_edid, 0);
>   		dm_helpers_parse_edid_caps(dc_link, dc_em_sink);
>   	}
>   }
> @@ -7416,7 +7414,6 @@ static void create_eml_sink(struct amdgpu_dm_connector *aconnector)
>   			.sink_signal = SIGNAL_TYPE_VIRTUAL
>   	};
>   	const struct drm_edid *drm_edid;
> -	const struct edid *edid;
>   	struct i2c_adapter *ddc;
>   
>   	if (dc_link && dc_link->aux_mode)
> @@ -7436,12 +7433,9 @@ static void create_eml_sink(struct amdgpu_dm_connector *aconnector)
>   
>   	aconnector->drm_edid = drm_edid;
>   
> -	edid = drm_edid_raw(drm_edid); // FIXME: Get rid of drm_edid_raw()
> -	aconnector->dc_em_sink = dc_link_add_remote_sink(
> -		aconnector->dc_link,
> -		(uint8_t *)edid,
> -		(edid->extensions + 1) * EDID_LENGTH,
> -		&init_params);
> +	aconnector->dc_em_sink = dc_link_add_remote_sink(aconnector->dc_link,
> +							 drm_edid, 0,
> +							 &init_params);
>   
>   	if (aconnector->base.force == DRM_FORCE_ON) {
>   		aconnector->dc_sink = aconnector->dc_link->local_sink ?
> 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 abfce44dcee7..3e9d04760c21 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
> @@ -48,6 +48,7 @@
>   #include "dm_helpers.h"
>   #include "ddc_service_types.h"
>   #include "clk_mgr.h"
> +#include "dc_edid.h"
>   
>   static void apply_edid_quirks(struct drm_device *dev,
>   			      const struct drm_edid *drm_edid,
> @@ -100,20 +101,16 @@ enum dc_edid_status dm_helpers_parse_edid_caps(struct dc_link *link,
>   	struct amdgpu_dm_connector *aconnector = link->priv;
>   	struct drm_connector *connector = &aconnector->base;
>   	struct drm_device *dev = connector->dev;
> -	struct edid *edid_buf;
> -	const struct drm_edid *drm_edid;
> +	const struct drm_edid *drm_edid = sink->drm_edid;
>   	struct drm_edid_product_id product_id;
>   	struct dc_edid_caps *edid_caps = &sink->edid_caps;
>   	int sad_count;
>   	int i = 0;
>   	enum dc_edid_status result = EDID_OK;
>   
> -	edid_buf = (struct edid *) &sink->dc_edid.raw_edid;
> -	if (!edid_caps || !edid_buf)
> +	if (!edid_caps || !drm_edid)
>   		return EDID_BAD_INPUT;
>   
> -	drm_edid = drm_edid_alloc(edid_buf, EDID_LENGTH * (edid_buf->extensions + 1));
> -
>   	if (!drm_edid_valid(drm_edid))
>   		result = EDID_BAD_CHECKSUM;
>   
> @@ -135,10 +132,8 @@ enum dc_edid_status dm_helpers_parse_edid_caps(struct dc_link *link,
>   	apply_edid_quirks(dev, drm_edid, edid_caps);
>   
>   	sad_count = drm_eld_sad_count(connector->eld);
> -	if (sad_count <= 0) {
> -		drm_edid_free(drm_edid);
> +	if (sad_count <= 0)
>   		return result;
> -	}
>   
>   	edid_caps->audio_mode_count = min(sad_count, DC_MAX_AUDIO_DESC_COUNT);
>   	for (i = 0; i < edid_caps->audio_mode_count; ++i) {
> @@ -158,8 +153,6 @@ enum dc_edid_status dm_helpers_parse_edid_caps(struct dc_link *link,
>   	else
>   		edid_caps->speaker_flags = DEFAULT_SPEAKER_LOCATION;
>   
> -	drm_edid_free(drm_edid);
> -
>   	return result;
>   }
>   
> @@ -991,7 +984,6 @@ enum dc_edid_status dm_helpers_read_local_edid(
>   	int retry = 3;
>   	enum dc_edid_status edid_status;
>   	const struct drm_edid *drm_edid;
> -	const struct edid *edid;
>   
>   	if (link->aux_mode)
>   		ddc = &aconnector->dm_dp_aux.aux.ddc;
> @@ -1021,11 +1013,7 @@ enum dc_edid_status dm_helpers_read_local_edid(
>   		if (!drm_edid)
>   			return EDID_NO_RESPONSE;
>   
> -		edid = drm_edid_raw(drm_edid); // FIXME: Get rid of drm_edid_raw()
> -		sink->dc_edid.length = EDID_LENGTH * (edid->extensions + 1);
> -		memmove(sink->dc_edid.raw_edid, (uint8_t *)edid, sink->dc_edid.length);
> -
> -		/* We don't need the original edid anymore */
> +		sink->drm_edid = drm_edid_dup(drm_edid);
>   		drm_edid_free(drm_edid);

Is the duplication actually necessary?  Can you "steal" the pointer by 
just assigning directly?

>   
>   		edid_status = dm_helpers_parse_edid_caps(link, sink);
> @@ -1051,6 +1039,8 @@ enum dc_edid_status dm_helpers_read_local_edid(
>   
>   		test_response.bits.EDID_CHECKSUM_WRITE = 1;
>   
> +		// TODO: drm_edid doesn't have a helper for dp_write_dpcd yet
> +		dc_edid_copy_edid_to_sink(sink);
>   		dm_helpers_dp_write_dpcd(ctx,
>   					link,
>   					DP_TEST_EDID_CHECKSUM,
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> index 7187d5aedf0a..5ca3e668c8aa 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> @@ -359,12 +359,10 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
>   					.link = aconnector->dc_link,
>   					.sink_signal = SIGNAL_TYPE_DISPLAY_PORT_MST };
>   
> -				dc_sink = dc_link_add_remote_sink(
> -					aconnector->dc_link,
> -					NULL,
> -					0,
> -					&init_params);
> -
> +				dc_sink = dc_link_add_remote_sink(aconnector->dc_link,
> +								  NULL,
> +								  0,
> +								  &init_params);
>   				if (!dc_sink) {
>   					DRM_ERROR("Unable to add a remote sink\n");
>   					return 0;
> @@ -397,15 +395,10 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
>   		struct dc_sink_init_data init_params = {
>   				.link = aconnector->dc_link,
>   				.sink_signal = SIGNAL_TYPE_DISPLAY_PORT_MST };
> -		const struct edid *edid;
> -
> -		edid = drm_edid_raw(aconnector->drm_edid); // FIXME: Get rid of drm_edid_raw()
> -		dc_sink = dc_link_add_remote_sink(
> -			aconnector->dc_link,
> -			(uint8_t *)edid,
> -			(edid->extensions + 1) * EDID_LENGTH,
> -			&init_params);
>   
> +		dc_sink = dc_link_add_remote_sink(aconnector->dc_link,
> +						  aconnector->drm_edid, 0,
> +						  &init_params);
>   		if (!dc_sink) {
>   			DRM_ERROR("Unable to add a remote sink\n");
>   			return 0;
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
> index 9e86dc15557b..ce4a7f9e268a 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
> @@ -6,25 +6,25 @@
>   bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
>   			  struct dc_sink *current_sink)
>   {
> -	struct dc_edid *old_edid = &prev_sink->dc_edid;
> -	struct dc_edid *new_edid = &current_sink->dc_edid;
> -
> -       if (old_edid->length != new_edid->length)
> -               return false;
> -
> -       if (new_edid->length == 0)
> -               return false;
> -
> -       return (memcmp(old_edid->raw_edid,
> -                      new_edid->raw_edid, new_edid->length) == 0);
> +	return drm_edid_eq(prev_sink->drm_edid, current_sink->drm_edid);
>   }
>   
>   void dc_edid_copy_edid_to_dc(struct dc_sink *dc_sink,
>   			     const void *edid,
>   			     int len)
>   {
> -	memmove(dc_sink->dc_edid.raw_edid, edid, len);
> -	dc_sink->dc_edid.length = len;
> +	dc_sink->drm_edid = drm_edid_dup((const struct drm_edid *) edid);
> +}
> +
> +void dc_edid_copy_edid_to_sink(struct dc_sink *sink)
> +{
> +	const struct edid *edid;
> +	uint32_t edid_length;
> +
> +	edid = drm_edid_raw(sink->drm_edid); // FIXME: Get rid of drm_edid_raw()
> +	edid_length = EDID_LENGTH * (edid->extensions + 1);
> +	memcpy(sink->dc_edid.raw_edid, (uint8_t *) edid, edid_length);
> +	sink->dc_edid.length = edid_length;
>   }
>   
>   void dc_edid_sink_edid_free(struct dc_sink *sink)
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
> index 2c76768be459..a95cc6ccc743 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
> @@ -9,6 +9,7 @@ bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
>   			  struct dc_sink *current_sink);
>   void dc_edid_copy_edid_to_dc(struct dc_sink *dc_sink,
>   			     const void *edid, int len);
> +void dc_edid_copy_edid_to_sink(struct dc_sink *sink);
>   void dc_edid_sink_edid_free(struct dc_sink *sink);
>   
>   #endif /* __DC_EDID_H__ */
> diff --git a/drivers/gpu/drm/amd/display/dc/link/link_detection.c b/drivers/gpu/drm/amd/display/dc/link/link_detection.c
> index c28072f980cc..bddcfd8f02ba 100644
> --- a/drivers/gpu/drm/amd/display/dc/link/link_detection.c
> +++ b/drivers/gpu/drm/amd/display/dc/link/link_detection.c
> @@ -1133,6 +1133,7 @@ static bool detect_link_and_local_sink(struct dc_link *link,
>   			dp_trace_init(link);
>   
>   		/* Connectivity log: detection */
> +		dc_edid_copy_edid_to_sink(sink);
>   		for (i = 0; i < sink->dc_edid.length / DC_EDID_BLOCK_SIZE; i++) {
>   			CONN_DATA_DETECT(link,
>   					 &sink->dc_edid.raw_edid[i * DC_EDID_BLOCK_SIZE],
> @@ -1415,7 +1416,7 @@ struct dc_sink *link_add_remote_sink(
>   	 * parsing fails
>   	 */
>   	if (edid_status != EDID_OK && edid_status != EDID_PARTIAL_VALID) {
> -		dc_sink->dc_edid.length = 0;
> +		dc_edid_sink_edid_free(dc_sink);
>   		dm_error("Bad EDID, status%d!\n", edid_status);
>   	}
>   

Re: [PATCH v4 12/14] drm/edid: introduce a helper that compares edid data from two drm_edid

[Mario Limonciello]

On 6/13/2025 9:58 AM, Melissa Wen wrote:
> AMD driver has a function used to compare if two edid are the same; this
> is useful to some of the link detection algorithms implemented by
> amdgpu. Since the amdgpu function can be helpful for other drivers, this
> commit abstracts the AMD function to make it available at the DRM level
> by wrapping existent drm_edid_eq().
> 
> v2:
> - rename drm_edid_eq to drm_edid_eq_buf (jani)
> - add NULL checks (jani)
> 
> v3:
> - fix kernel-doc (jani)
> - fix parameter names
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> Co-developed-by: Rodrigo Siqueira <siqueira@igalia.com>
> Signed-off-by: Rodrigo Siqueira <siqueira@igalia.com>
> Signed-off-by: Melissa Wen <mwen@igalia.com>
> ---
>   drivers/gpu/drm/drm_edid.c | 24 +++++++++++++++++++++---
>   include/drm/drm_edid.h     |  2 ++
>   2 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index d5772a3d27f1..056e070b2f55 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1820,8 +1820,8 @@ static bool edid_block_is_zero(const void *edid)
>   	return mem_is_zero(edid, EDID_LENGTH);
>   }
>   
> -static bool drm_edid_eq(const struct drm_edid *drm_edid,
> -			const void *raw_edid, size_t raw_edid_size)
> +static bool drm_edid_eq_buf(const struct drm_edid *drm_edid,
> +			    const void *raw_edid, size_t raw_edid_size)
>   {
>   	bool edid1_present = drm_edid && drm_edid->edid && drm_edid->size;
>   	bool edid2_present = raw_edid && raw_edid_size;
> @@ -6915,7 +6915,7 @@ static int _drm_edid_connector_property_update(struct drm_connector *connector,
>   		const void *old_edid = connector->edid_blob_ptr->data;
>   		size_t old_edid_size = connector->edid_blob_ptr->length;
>   
> -		if (old_edid && !drm_edid_eq(drm_edid, old_edid, old_edid_size)) {
> +		if (old_edid && !drm_edid_eq_buf(drm_edid, old_edid, old_edid_size)) {
>   			connector->epoch_counter++;
>   			drm_dbg_kms(dev, "[CONNECTOR:%d:%s] EDID changed, epoch counter %llu\n",
>   				    connector->base.id, connector->name,
> @@ -7520,3 +7520,21 @@ bool drm_edid_is_digital(const struct drm_edid *drm_edid)
>   		drm_edid->edid->input & DRM_EDID_INPUT_DIGITAL;
>   }
>   EXPORT_SYMBOL(drm_edid_is_digital);
> +
> +/**
> + * drm_edid_eq - Check if EDIDs are equal
> + *
> + * @drm_edid_1: first drm_edid to compare edid
> + * @drm_edid_2: second drm_edid to compare edid
> + *
> + * Return true if EDIDs are equal.
> + */
> +bool drm_edid_eq(const struct drm_edid *drm_edid_1,
> +		 const struct drm_edid *drm_edid_2)
> +{
> +	const void *edid_1 = drm_edid_1 ? drm_edid_1->edid : NULL;
> +	size_t edid_1_size = drm_edid_1 ? drm_edid_1->size : 0;
> +
> +	return drm_edid_eq_buf(drm_edid_2, edid_1, edid_1_size);

What happens when the size for edid 2 is different than edid 1?  Does 
drm_edid_eq_buf() already handle this well (I didn't immediately look)?

If not; how about including an extra check directly in this function?

I was thinking this will handle it effectively:

if (!drm_edid_1 || !drm_edid_2)
	return false;
if (drm_edid_1->size != drm_edid_2->size)
	return false;
return drm_edid_eq_buf();

> +}
> +EXPORT_SYMBOL(drm_edid_eq);
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 960592167486..e7a9a4928b97 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -469,6 +469,8 @@ int drm_edid_connector_update(struct drm_connector *connector,
>   			      const struct drm_edid *edid);
>   int drm_edid_connector_add_modes(struct drm_connector *connector);
>   bool drm_edid_is_digital(const struct drm_edid *drm_edid);
> +bool drm_edid_eq(const struct drm_edid *drm_edid_first,
> +			 const struct drm_edid *drm_edid_second);
>   void drm_edid_get_product_id(const struct drm_edid *drm_edid,
>   			     struct drm_edid_product_id *id);
>   void drm_edid_print_product_id(struct drm_printer *p,

Re: [PATCH v4 11/14] drm/amd/display: create a function to fill dc_sink with edid data

[Mario Limonciello]

On 6/13/2025 9:58 AM, Melissa Wen wrote:
> From: Rodrigo Siqueira <siqueira@igalia.com>
> 
> As part of the effort of stopping using raw edid, this commit move the
> copy of the edid in DC to a dedicated function that will allow the usage
> of drm_edid in the next steps.
> 
> v3:
> - remove superfulous cast (jani)

I don't think that changelog needs to end up in the eventual commit 
history.  But it's useful while still under review.

Could you put the changelog below the cutlist (---)?

> 
> Signed-off-by: Rodrigo Siqueira <siqueira@igalia.com>
> Co-developer--by: Melissa Wen <mwen@igalia.com>

These tags are wrong.  It's "Co-developed-by".

> Signed-off-by: Melissa Wen <mwen@igalia.com>
> ---
>   drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c      | 8 ++++++++
>   drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h      | 2 ++
>   drivers/gpu/drm/amd/display/dc/link/link_detection.c | 3 +--
>   3 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
> index fab873b091f5..a90545b176cc 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
> @@ -17,3 +17,11 @@ bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
>          return (memcmp(old_edid->raw_edid,
>                         new_edid->raw_edid, new_edid->length) == 0);
>   }
> +
> +void dc_edid_copy_edid_to_dc(struct dc_sink *dc_sink,
> +			     const void *edid,
> +			     int len)
> +{
> +	memmove(dc_sink->dc_edid.raw_edid, edid, len);
> +	dc_sink->dc_edid.length = len;
> +}
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
> index 7e3b1177bc8a..f42cd5bbc730 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
> @@ -7,5 +7,7 @@
>   
>   bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
>   			  struct dc_sink *current_sink);
> +void dc_edid_copy_edid_to_dc(struct dc_sink *dc_sink,
> +			     const void *edid, int len);
>   
>   #endif /* __DC_EDID_H__ */
> diff --git a/drivers/gpu/drm/amd/display/dc/link/link_detection.c b/drivers/gpu/drm/amd/display/dc/link/link_detection.c
> index 344356e26f8b..c28072f980cc 100644
> --- a/drivers/gpu/drm/amd/display/dc/link/link_detection.c
> +++ b/drivers/gpu/drm/amd/display/dc/link/link_detection.c
> @@ -1401,8 +1401,7 @@ struct dc_sink *link_add_remote_sink(
>   	if (!dc_sink)
>   		return NULL;
>   
> -	memmove(dc_sink->dc_edid.raw_edid, (const uint8_t *) edid, len);
> -	dc_sink->dc_edid.length = len;
> +	dc_edid_copy_edid_to_dc(dc_sink, edid, len);
>   
>   	if (!link_add_remote_sink_helper(
>   			link,

Re: [PATCH v4 10/14] drm/amd/display: add a mid-layer file to handle EDID in DC

[Mario Limonciello]

On 6/13/2025 9:58 AM, Melissa Wen wrote:
> From: Rodrigo Siqueira <siqueira@igalia.com>
> 
> Since DC is a shared code, this commit introduces a new file to work as
> a mid-layer in DC for the edid manipulation.
> 
> v3:
> - rebase on top of asdn
Can you put changelog below cutlist (---)?
> 
> Signed-off-by: Rodrigo Siqueira <siqueira@igalia.com>
> Co-developed-by: Melissa Wen <mwen@igalia.com>
> Signed-off-by: Melissa Wen <mwen@igalia.com>
> ---
>   .../gpu/drm/amd/display/amdgpu_dm/Makefile    |  1 +
>   .../gpu/drm/amd/display/amdgpu_dm/dc_edid.c   | 19 +++++++++++++++++++
>   .../gpu/drm/amd/display/amdgpu_dm/dc_edid.h   | 11 +++++++++++
>   .../drm/amd/display/dc/link/link_detection.c  | 17 +++--------------
>   4 files changed, 34 insertions(+), 14 deletions(-)
>   create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
>   create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile b/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
> index 7329b8cc2576..09cb94d8e0e4 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
> @@ -39,6 +39,7 @@ AMDGPUDM = \
>   	amdgpu_dm_psr.o \
>   	amdgpu_dm_replay.o \
>   	amdgpu_dm_quirks.o \
> +	dc_edid.o \
>   	amdgpu_dm_wb.o
>   
>   ifdef CONFIG_DRM_AMD_DC_FP
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
> new file mode 100644
> index 000000000000..fab873b091f5
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
> @@ -0,0 +1,19 @@
> +// SPDX-License-Identifier: MIT
> +#include "amdgpu_dm/dc_edid.h"
> +#include "dc.h"
> +
> +bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
> +			  struct dc_sink *current_sink)
> +{
> +	struct dc_edid *old_edid = &prev_sink->dc_edid;
> +	struct dc_edid *new_edid = &current_sink->dc_edid;
> +
> +       if (old_edid->length != new_edid->length)
> +               return false;
> +
> +       if (new_edid->length == 0)
> +               return false;
> +
> +       return (memcmp(old_edid->raw_edid,
> +                      new_edid->raw_edid, new_edid->length) == 0);
> +}
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
> new file mode 100644
> index 000000000000..7e3b1177bc8a
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
> @@ -0,0 +1,11 @@
> +/* SPDX-License-Identifier: MIT */
> +
> +#ifndef __DC_EDID_H__
> +#define __DC_EDID_H__
> +
> +#include "dc.h"
> +
> +bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
> +			  struct dc_sink *current_sink);
> +
> +#endif /* __DC_EDID_H__ */
> diff --git a/drivers/gpu/drm/amd/display/dc/link/link_detection.c b/drivers/gpu/drm/amd/display/dc/link/link_detection.c
> index 863c24fe1117..344356e26f8b 100644
> --- a/drivers/gpu/drm/amd/display/dc/link/link_detection.c
> +++ b/drivers/gpu/drm/amd/display/dc/link/link_detection.c
> @@ -48,6 +48,8 @@
>   #include "dm_helpers.h"
>   #include "clk_mgr.h"
>   
> +#include "dc_edid.h"

There's a problem with the header location.  If you're naming it 
dc_edid.h but putting the header in amdgpu_dm/ directory then it's only 
going to compile for amdgpu.

I think dc_edid.h needs to go into dc/ directory.

> +
>    // Offset DPCD 050Eh == 0x5A
>   #define MST_HUB_ID_0x5A  0x5A
>   
> @@ -617,18 +619,6 @@ static bool detect_dp(struct dc_link *link,
>   	return true;
>   }
>   
> -static bool is_same_edid(struct dc_edid *old_edid, struct dc_edid *new_edid)
> -{
> -	if (old_edid->length != new_edid->length)
> -		return false;
> -
> -	if (new_edid->length == 0)
> -		return false;
> -
> -	return (memcmp(old_edid->raw_edid,
> -		       new_edid->raw_edid, new_edid->length) == 0);
> -}
> -
>   static bool wait_for_entering_dp_alt_mode(struct dc_link *link)
>   {
>   
> @@ -1105,8 +1095,7 @@ static bool detect_link_and_local_sink(struct dc_link *link,
>   		// Check if edid is the same
>   		if ((prev_sink) &&
>   		    (edid_status == EDID_THE_SAME || edid_status == EDID_OK))
> -			same_edid = is_same_edid(&prev_sink->dc_edid,
> -						 &sink->dc_edid);
> +			same_edid = dc_edid_is_same_edid(prev_sink, sink);
>   
>   		if (sink->edid_caps.panel_patch.skip_scdc_overwrite)
>   			link->ctx->dc->debug.hdmi20_disable = true;

Re: [PATCH v4 07/14] drm/amd/display: get SADB from drm_eld when parsing EDID caps

[Mario Limonciello]

On 6/13/2025 9:58 AM, Melissa Wen wrote:
> 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.
> 
> Signed-off-by: Melissa Wen <mwen@igalia.com>
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   .../drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 15 +++------------
>   1 file changed, 3 insertions(+), 12 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 10af9db47c39..e7cfbee6c67f 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
> @@ -106,9 +106,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;
>   
>   
> @@ -156,19 +155,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);
>   	drm_edid_free(drm_edid);
>   
>   	return result;

Re: [PATCH v4 06/14] drm/amd/display: get SAD from drm_eld when parsing EDID caps

[Mario Limonciello]

On 6/13/2025 9:58 AM, Melissa Wen wrote:
> 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.
> 
> Signed-off-by: Melissa Wen <mwen@igalia.com>
Reviewed-by: Mario Limonciello <mario.limonciello@amd.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 760da13612b9..10af9db47c39 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
> @@ -33,6 +33,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"
> @@ -105,9 +106,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;
> @@ -121,6 +120,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;
> @@ -137,7 +137,7 @@ 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) {
>   		drm_edid_free(drm_edid);
>   		return result;
> @@ -145,12 +145,15 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
>   
>   	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);
> @@ -165,7 +168,6 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
>   	else
>   		edid_caps->speaker_flags = DEFAULT_SPEAKER_LOCATION;
>   
> -	kfree(sads);
>   	kfree(sadb);
>   	drm_edid_free(drm_edid);
>   

Re: [PATCH v4 05/14] drm/amd/display: get panel id with drm_edid helper

[Mario Limonciello]

On 6/13/2025 9:58 AM, Melissa Wen wrote:
> Instead of using driver-specific code, use DRM helpers.
> 
> Signed-off-by: Melissa Wen <mwen@igalia.com>
Reviewed-by: Mario Limonciello <mario.limonciello@amd.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 514da4d5d300..760da13612b9 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
> @@ -48,16 +48,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 */
> @@ -140,7 +135,7 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
>   
>   	edid_caps->edid_hdmi = connector->display_info.is_hdmi;
>   
> -	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) {

Re: [PATCH v4 03/14] drm/amd/display: use drm_edid_product_id for parsing EDID product info

[Mario Limonciello]

On 6/13/2025 9:58 AM, Melissa Wen wrote:
> Since [1], we can use drm_edid_product_id to get debug info from
> drm_edid instead of directly parsing EDID.
> 
> Link: https://lore.kernel.org/dri-devel/cover.1712655867.git.jani.nikula@intel.com/ [1]

This is a pretty old commit.  It's landed now a while, right?
I'd say if you're going to reference it in the changelog it should be 
referenced by commit ABC123 ("Foo the bar").

> Signed-off-by: Melissa Wen <mwen@igalia.com>
> ---
>   .../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 5543780f1024..b1085f1195f7 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
> @@ -109,6 +109,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;
> @@ -125,13 +126,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;
>   
>   	drm_edid_get_monitor_name(edid_buf,
>   				  edid_caps->display_name,

Re: [PATCH v4 14/14] drm/amd/display: move dc_sink from dc_edid to drm_edid

[Melissa Wen]

On 13/06/2025 15:26, Mario Limonciello wrote:
> On 6/13/2025 9:58 AM, Melissa Wen wrote:
>> Reduce direct handling of edid data by resorting to drm helpers that
>> deal with this info inside drm_edid infrastructure.
>>
>> v3:
>> - use dc_edid_sink_edid_free in link_detection
>>
>> Signed-off-by: Melissa Wen <mwen@igalia.com>
>> ---
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 26 +++++++------------
>>   .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 24 +++++------------
>>   .../display/amdgpu_dm/amdgpu_dm_mst_types.c   | 21 +++++----------
>>   .../gpu/drm/amd/display/amdgpu_dm/dc_edid.c   | 26 +++++++++----------
>>   .../gpu/drm/amd/display/amdgpu_dm/dc_edid.h   |  1 +
>>   .../drm/amd/display/dc/link/link_detection.c  |  3 ++-
>>   6 files changed, 40 insertions(+), 61 deletions(-)
>>
>> 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 c7efeb9f38b6..ec33a6236e37 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -68,6 +68,7 @@
>>   #endif
>>   #include "amdgpu_dm_psr.h"
>>   #include "amdgpu_dm_replay.h"
>> +#include "dc_edid.h"
>>     #include "ivsrcid/ivsrcid_vislands30.h"
>>   @@ -3708,6 +3709,8 @@ void amdgpu_dm_update_connector_after_detect(
>>        * 2. Send an event and let userspace tell us what to do
>>        */
>>       if (sink) {
>> +        const struct drm_edid *drm_edid = sink->drm_edid;
>> +
>>           /*
>>            * TODO: check if we still need the S3 mode update workaround.
>>            * If yes, put it here.
>> @@ -3719,16 +3722,15 @@ void amdgpu_dm_update_connector_after_detect(
>>             aconnector->dc_sink = sink;
>>           dc_sink_retain(aconnector->dc_sink);
>> -        if (sink->dc_edid.length == 0) {
>> +
>> +        if (!drm_edid_valid(drm_edid)) {
>>               aconnector->drm_edid = NULL;
>>               hdmi_cec_unset_edid(aconnector);
>>               if (aconnector->dc_link->aux_mode) {
>> drm_dp_cec_unset_edid(&aconnector->dm_dp_aux.aux);
>>               }
>>           } else {
>> -            const struct edid *edid = (const struct edid 
>> *)sink->dc_edid.raw_edid;
>> -
>> -            aconnector->drm_edid = drm_edid_alloc(edid, 
>> sink->dc_edid.length);
>> +            aconnector->drm_edid = drm_edid_dup(sink->drm_edid);
>>               drm_edid_connector_update(connector, 
>> aconnector->drm_edid);
>>                 hdmi_cec_set_edid(aconnector);
>> @@ -7378,12 +7380,8 @@ static void 
>> amdgpu_dm_connector_funcs_force(struct drm_connector *connector)
>>       aconnector->drm_edid = drm_edid;
>>       /* Update emulated (virtual) sink's EDID */
>>       if (dc_em_sink && dc_link) {
>> -        // FIXME: Get rid of drm_edid_raw()
>> -        const struct edid *edid = drm_edid_raw(drm_edid);
>> -
>>           memset(&dc_em_sink->edid_caps, 0, sizeof(struct 
>> dc_edid_caps));
>> -        memmove(dc_em_sink->dc_edid.raw_edid, edid,
>> -            (edid->extensions + 1) * EDID_LENGTH);
>> +        dc_edid_copy_edid_to_dc(dc_em_sink, drm_edid, 0);
>>           dm_helpers_parse_edid_caps(dc_link, dc_em_sink);
>>       }
>>   }
>> @@ -7416,7 +7414,6 @@ static void create_eml_sink(struct 
>> amdgpu_dm_connector *aconnector)
>>               .sink_signal = SIGNAL_TYPE_VIRTUAL
>>       };
>>       const struct drm_edid *drm_edid;
>> -    const struct edid *edid;
>>       struct i2c_adapter *ddc;
>>         if (dc_link && dc_link->aux_mode)
>> @@ -7436,12 +7433,9 @@ static void create_eml_sink(struct 
>> amdgpu_dm_connector *aconnector)
>>         aconnector->drm_edid = drm_edid;
>>   -    edid = drm_edid_raw(drm_edid); // FIXME: Get rid of 
>> drm_edid_raw()
>> -    aconnector->dc_em_sink = dc_link_add_remote_sink(
>> -        aconnector->dc_link,
>> -        (uint8_t *)edid,
>> -        (edid->extensions + 1) * EDID_LENGTH,
>> -        &init_params);
>> +    aconnector->dc_em_sink = 
>> dc_link_add_remote_sink(aconnector->dc_link,
>> +                             drm_edid, 0,
>> +                             &init_params);
>>         if (aconnector->base.force == DRM_FORCE_ON) {
>>           aconnector->dc_sink = aconnector->dc_link->local_sink ?
>> 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 abfce44dcee7..3e9d04760c21 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
>> @@ -48,6 +48,7 @@
>>   #include "dm_helpers.h"
>>   #include "ddc_service_types.h"
>>   #include "clk_mgr.h"
>> +#include "dc_edid.h"
>>     static void apply_edid_quirks(struct drm_device *dev,
>>                     const struct drm_edid *drm_edid,
>> @@ -100,20 +101,16 @@ enum dc_edid_status 
>> dm_helpers_parse_edid_caps(struct dc_link *link,
>>       struct amdgpu_dm_connector *aconnector = link->priv;
>>       struct drm_connector *connector = &aconnector->base;
>>       struct drm_device *dev = connector->dev;
>> -    struct edid *edid_buf;
>> -    const struct drm_edid *drm_edid;
>> +    const struct drm_edid *drm_edid = sink->drm_edid;
>>       struct drm_edid_product_id product_id;
>>       struct dc_edid_caps *edid_caps = &sink->edid_caps;
>>       int sad_count;
>>       int i = 0;
>>       enum dc_edid_status result = EDID_OK;
>>   -    edid_buf = (struct edid *) &sink->dc_edid.raw_edid;
>> -    if (!edid_caps || !edid_buf)
>> +    if (!edid_caps || !drm_edid)
>>           return EDID_BAD_INPUT;
>>   -    drm_edid = drm_edid_alloc(edid_buf, EDID_LENGTH * 
>> (edid_buf->extensions + 1));
>> -
>>       if (!drm_edid_valid(drm_edid))
>>           result = EDID_BAD_CHECKSUM;
>>   @@ -135,10 +132,8 @@ enum dc_edid_status 
>> dm_helpers_parse_edid_caps(struct dc_link *link,
>>       apply_edid_quirks(dev, drm_edid, edid_caps);
>>         sad_count = drm_eld_sad_count(connector->eld);
>> -    if (sad_count <= 0) {
>> -        drm_edid_free(drm_edid);
>> +    if (sad_count <= 0)
>>           return result;
>> -    }
>>         edid_caps->audio_mode_count = min(sad_count, 
>> DC_MAX_AUDIO_DESC_COUNT);
>>       for (i = 0; i < edid_caps->audio_mode_count; ++i) {
>> @@ -158,8 +153,6 @@ enum dc_edid_status 
>> dm_helpers_parse_edid_caps(struct dc_link *link,
>>       else
>>           edid_caps->speaker_flags = DEFAULT_SPEAKER_LOCATION;
>>   -    drm_edid_free(drm_edid);
>> -
>>       return result;
>>   }
>>   @@ -991,7 +984,6 @@ enum dc_edid_status dm_helpers_read_local_edid(
>>       int retry = 3;
>>       enum dc_edid_status edid_status;
>>       const struct drm_edid *drm_edid;
>> -    const struct edid *edid;
>>         if (link->aux_mode)
>>           ddc = &aconnector->dm_dp_aux.aux.ddc;
>> @@ -1021,11 +1013,7 @@ enum dc_edid_status dm_helpers_read_local_edid(
>>           if (!drm_edid)
>>               return EDID_NO_RESPONSE;
>>   -        edid = drm_edid_raw(drm_edid); // FIXME: Get rid of 
>> drm_edid_raw()
>> -        sink->dc_edid.length = EDID_LENGTH * (edid->extensions + 1);
>> -        memmove(sink->dc_edid.raw_edid, (uint8_t *)edid, 
>> sink->dc_edid.length);
>> -
>> -        /* We don't need the original edid anymore */
>> +        sink->drm_edid = drm_edid_dup(drm_edid);
>>           drm_edid_free(drm_edid);
>
> Is the duplication actually necessary?  Can you "steal" the pointer by 
> just assigning directly?

Now you pointed it out and I realized there is a memory leak here if the 
loop continues.
I'll fix that.

Thanks for reviewing.

Melissa

>
>
>>             edid_status = dm_helpers_parse_edid_caps(link, sink);
>> @@ -1051,6 +1039,8 @@ enum dc_edid_status dm_helpers_read_local_edid(
>>             test_response.bits.EDID_CHECKSUM_WRITE = 1;
>>   +        // TODO: drm_edid doesn't have a helper for dp_write_dpcd yet
>> +        dc_edid_copy_edid_to_sink(sink);
>>           dm_helpers_dp_write_dpcd(ctx,
>>                       link,
>>                       DP_TEST_EDID_CHECKSUM,
>> diff --git 
>> a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c 
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>> index 7187d5aedf0a..5ca3e668c8aa 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>> @@ -359,12 +359,10 @@ static int dm_dp_mst_get_modes(struct 
>> drm_connector *connector)
>>                       .link = aconnector->dc_link,
>>                       .sink_signal = SIGNAL_TYPE_DISPLAY_PORT_MST };
>>   -                dc_sink = dc_link_add_remote_sink(
>> -                    aconnector->dc_link,
>> -                    NULL,
>> -                    0,
>> -                    &init_params);
>> -
>> +                dc_sink = dc_link_add_remote_sink(aconnector->dc_link,
>> +                                  NULL,
>> +                                  0,
>> +                                  &init_params);
>>                   if (!dc_sink) {
>>                       DRM_ERROR("Unable to add a remote sink\n");
>>                       return 0;
>> @@ -397,15 +395,10 @@ static int dm_dp_mst_get_modes(struct 
>> drm_connector *connector)
>>           struct dc_sink_init_data init_params = {
>>                   .link = aconnector->dc_link,
>>                   .sink_signal = SIGNAL_TYPE_DISPLAY_PORT_MST };
>> -        const struct edid *edid;
>> -
>> -        edid = drm_edid_raw(aconnector->drm_edid); // FIXME: Get rid 
>> of drm_edid_raw()
>> -        dc_sink = dc_link_add_remote_sink(
>> -            aconnector->dc_link,
>> -            (uint8_t *)edid,
>> -            (edid->extensions + 1) * EDID_LENGTH,
>> -            &init_params);
>>   +        dc_sink = dc_link_add_remote_sink(aconnector->dc_link,
>> +                          aconnector->drm_edid, 0,
>> +                          &init_params);
>>           if (!dc_sink) {
>>               DRM_ERROR("Unable to add a remote sink\n");
>>               return 0;
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c 
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
>> index 9e86dc15557b..ce4a7f9e268a 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
>> @@ -6,25 +6,25 @@
>>   bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
>>                 struct dc_sink *current_sink)
>>   {
>> -    struct dc_edid *old_edid = &prev_sink->dc_edid;
>> -    struct dc_edid *new_edid = &current_sink->dc_edid;
>> -
>> -       if (old_edid->length != new_edid->length)
>> -               return false;
>> -
>> -       if (new_edid->length == 0)
>> -               return false;
>> -
>> -       return (memcmp(old_edid->raw_edid,
>> -                      new_edid->raw_edid, new_edid->length) == 0);
>> +    return drm_edid_eq(prev_sink->drm_edid, current_sink->drm_edid);
>>   }
>>     void dc_edid_copy_edid_to_dc(struct dc_sink *dc_sink,
>>                    const void *edid,
>>                    int len)
>>   {
>> -    memmove(dc_sink->dc_edid.raw_edid, edid, len);
>> -    dc_sink->dc_edid.length = len;
>> +    dc_sink->drm_edid = drm_edid_dup((const struct drm_edid *) edid);
>> +}
>> +
>> +void dc_edid_copy_edid_to_sink(struct dc_sink *sink)
>> +{
>> +    const struct edid *edid;
>> +    uint32_t edid_length;
>> +
>> +    edid = drm_edid_raw(sink->drm_edid); // FIXME: Get rid of 
>> drm_edid_raw()
>> +    edid_length = EDID_LENGTH * (edid->extensions + 1);
>> +    memcpy(sink->dc_edid.raw_edid, (uint8_t *) edid, edid_length);
>> +    sink->dc_edid.length = edid_length;
>>   }
>>     void dc_edid_sink_edid_free(struct dc_sink *sink)
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h 
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
>> index 2c76768be459..a95cc6ccc743 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
>> @@ -9,6 +9,7 @@ bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
>>                 struct dc_sink *current_sink);
>>   void dc_edid_copy_edid_to_dc(struct dc_sink *dc_sink,
>>                    const void *edid, int len);
>> +void dc_edid_copy_edid_to_sink(struct dc_sink *sink);
>>   void dc_edid_sink_edid_free(struct dc_sink *sink);
>>     #endif /* __DC_EDID_H__ */
>> diff --git a/drivers/gpu/drm/amd/display/dc/link/link_detection.c 
>> b/drivers/gpu/drm/amd/display/dc/link/link_detection.c
>> index c28072f980cc..bddcfd8f02ba 100644
>> --- a/drivers/gpu/drm/amd/display/dc/link/link_detection.c
>> +++ b/drivers/gpu/drm/amd/display/dc/link/link_detection.c
>> @@ -1133,6 +1133,7 @@ static bool detect_link_and_local_sink(struct 
>> dc_link *link,
>>               dp_trace_init(link);
>>             /* Connectivity log: detection */
>> +        dc_edid_copy_edid_to_sink(sink);
>>           for (i = 0; i < sink->dc_edid.length / DC_EDID_BLOCK_SIZE; 
>> i++) {
>>               CONN_DATA_DETECT(link,
>>                        &sink->dc_edid.raw_edid[i * DC_EDID_BLOCK_SIZE],
>> @@ -1415,7 +1416,7 @@ struct dc_sink *link_add_remote_sink(
>>        * parsing fails
>>        */
>>       if (edid_status != EDID_OK && edid_status != EDID_PARTIAL_VALID) {
>> -        dc_sink->dc_edid.length = 0;
>> +        dc_edid_sink_edid_free(dc_sink);
>>           dm_error("Bad EDID, status%d!\n", edid_status);
>>       }
>

Re: [PATCH v4 13/14] drm/amd/display: add drm_edid to dc_sink

[Melissa Wen]

On 13/06/2025 15:23, Mario Limonciello wrote:
> On 6/13/2025 12:42 PM, Melissa Wen wrote:
>> On 06/13, Mario Limonciello wrote:
>>> On 6/13/2025 7:58 AM, Melissa Wen wrote:
>>>> Add Linux opaque object to dc_sink for storing edid data cross driver,
>>>> drm_edid. Also include the Linux call to free this object, the
>>>> drm_edid_free()
>>>>
>>>> v3:
>>>> - remove uneccessary include (jani)
>>>>
>>>> Signed-off-by: Melissa Wen <mwen@igalia.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c | 6 ++++++
>>>>    drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h | 1 +
>>>>    drivers/gpu/drm/amd/display/dc/core/dc_sink.c   | 3 +++
>>>>    drivers/gpu/drm/amd/display/dc/dc.h             | 1 +
>>>>    include/drm/drm_edid.h                          | 4 ++--
>>>>    5 files changed, 13 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c 
>>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
>>>> index a90545b176cc..9e86dc15557b 100644
>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
>>>> @@ -1,6 +1,7 @@
>>>>    // SPDX-License-Identifier: MIT
>>>>    #include "amdgpu_dm/dc_edid.h"
>>>>    #include "dc.h"
>>>> +#include <drm/drm_edid.h>
>>>>    bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
>>>>                  struct dc_sink *current_sink)
>>>> @@ -25,3 +26,8 @@ void dc_edid_copy_edid_to_dc(struct dc_sink 
>>>> *dc_sink,
>>>>        memmove(dc_sink->dc_edid.raw_edid, edid, len);
>>>>        dc_sink->dc_edid.length = len;
>>>>    }
>>>> +
>>>> +void dc_edid_sink_edid_free(struct dc_sink *sink)
>>>> +{
>>>> +    drm_edid_free(sink->drm_edid);
>>>> +}
>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h 
>>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
>>>> index f42cd5bbc730..2c76768be459 100644
>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
>>>> @@ -9,5 +9,6 @@ bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
>>>>                  struct dc_sink *current_sink);
>>>>    void dc_edid_copy_edid_to_dc(struct dc_sink *dc_sink,
>>>>                     const void *edid, int len);
>>>> +void dc_edid_sink_edid_free(struct dc_sink *sink);
>>>>    #endif /* __DC_EDID_H__ */
>>>> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_sink.c 
>>>> b/drivers/gpu/drm/amd/display/dc/core/dc_sink.c
>>>> index 455fa5dd1420..3774a3245506 100644
>>>> --- a/drivers/gpu/drm/amd/display/dc/core/dc_sink.c
>>>> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_sink.c
>>>> @@ -26,6 +26,7 @@
>>>>    #include "dm_services.h"
>>>>    #include "dm_helpers.h"
>>>>    #include "core_types.h"
>>>> +#include "dc_edid.h"
>>>> /*******************************************************************************
>>>>     * Private functions
>>>> @@ -65,6 +66,8 @@ void dc_sink_retain(struct dc_sink *sink)
>>>>    static void dc_sink_free(struct kref *kref)
>>>>    {
>>>>        struct dc_sink *sink = container_of(kref, struct dc_sink, 
>>>> refcount);
>>>> +
>>>> +    dc_edid_sink_edid_free(sink);
>>>>        kfree(sink->dc_container_id);
>>>>        kfree(sink);
>>>>    }
>>>> diff --git a/drivers/gpu/drm/amd/display/dc/dc.h 
>>>> b/drivers/gpu/drm/amd/display/dc/dc.h
>>>> index 86feef038de6..cf56a0405a4f 100644
>>>> --- a/drivers/gpu/drm/amd/display/dc/dc.h
>>>> +++ b/drivers/gpu/drm/amd/display/dc/dc.h
>>>> @@ -2466,6 +2466,7 @@ struct scdc_caps {
>>>>    struct dc_sink {
>>>>        enum signal_type sink_signal;
>>>>        struct dc_edid dc_edid; /* raw edid */
>>>> +    const struct drm_edid *drm_edid; /* Linux DRM edid*/
>>>
>>> Don't you need a forward declaration for 'struct drm_edid' in dc.h 
>>> to be
>>> able to do this?
>>
>> I understand that, as it's just a pointer (the compiler knows the size)
>> and there is no circular dependencies between dc_sink and drm_edid, we
>> don't need a forward declaration. So I think we are fine also because
>> dc_sink->drm_edid dereference only happens in dc_edid.h that already
>> needs to include drm_edid.h for drm_edid helpers... but let me know if
>> I'm missing something.
>
> Did you compile with CONFIG_WERROR or at least W=1?  I feel like I've 
> seen issues with this in the past that the compiler doesn't like having
> unknown types, even if a pointer.

Yeah, I don't see complaints here with the warning as error option enabled.
But let me know if you think forward declaration is safer or better for 
maintainance.
I just tried to reduce the number of linux-related stuff on DC as much 
as possible.

BR,

Melissa
>
> But if it works with W=1 / CONFIG_WERROR I must have been thinking 
> about a dereference case and thus no concerns on my side.
>
>>
>>>
>>> Also you're missing a space at the end of the comment before the '*/'.
>>
>> ack. I'll wait for more comments to send it fixed.
>>
>> Thanks for reviewing.
>>
>> Melissa
>>
>>>
>>>>        struct dc_edid_caps edid_caps; /* parse display caps */
>>>>        struct dc_container_id *dc_container_id;
>>>>        uint32_t dongle_max_pix_clk;
>>>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>>>> index e7a9a4928b97..8617d2285f38 100644
>>>> --- a/include/drm/drm_edid.h
>>>> +++ b/include/drm/drm_edid.h
>>>> @@ -469,8 +469,8 @@ int drm_edid_connector_update(struct 
>>>> drm_connector *connector,
>>>>                      const struct drm_edid *edid);
>>>>    int drm_edid_connector_add_modes(struct drm_connector *connector);
>>>>    bool drm_edid_is_digital(const struct drm_edid *drm_edid);
>>>> -bool drm_edid_eq(const struct drm_edid *drm_edid_first,
>>>> -             const struct drm_edid *drm_edid_second);
>>>> +bool drm_edid_eq(const struct drm_edid *drm_edid_1,
>>>> +         const struct drm_edid *drm_edid_2);
>>>>    void drm_edid_get_product_id(const struct drm_edid *drm_edid,
>>>>                     struct drm_edid_product_id *id);
>>>>    void drm_edid_print_product_id(struct drm_printer *p,
>>>
>

Re: [PATCH v4 12/14] drm/edid: introduce a helper that compares edid data from two drm_edid

[Melissa Wen]

On 13/06/2025 15:35, Mario Limonciello wrote:
> On 6/13/2025 9:58 AM, Melissa Wen wrote:
>> AMD driver has a function used to compare if two edid are the same; this
>> is useful to some of the link detection algorithms implemented by
>> amdgpu. Since the amdgpu function can be helpful for other drivers, this
>> commit abstracts the AMD function to make it available at the DRM level
>> by wrapping existent drm_edid_eq().
>>
>> v2:
>> - rename drm_edid_eq to drm_edid_eq_buf (jani)
>> - add NULL checks (jani)
>>
>> v3:
>> - fix kernel-doc (jani)
>> - fix parameter names
>>
>> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>> Co-developed-by: Rodrigo Siqueira <siqueira@igalia.com>
>> Signed-off-by: Rodrigo Siqueira <siqueira@igalia.com>
>> Signed-off-by: Melissa Wen <mwen@igalia.com>
>> ---
>>   drivers/gpu/drm/drm_edid.c | 24 +++++++++++++++++++++---
>>   include/drm/drm_edid.h     |  2 ++
>>   2 files changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index d5772a3d27f1..056e070b2f55 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -1820,8 +1820,8 @@ static bool edid_block_is_zero(const void *edid)
>>       return mem_is_zero(edid, EDID_LENGTH);
>>   }
>>   -static bool drm_edid_eq(const struct drm_edid *drm_edid,
>> -            const void *raw_edid, size_t raw_edid_size)
>> +static bool drm_edid_eq_buf(const struct drm_edid *drm_edid,
>> +                const void *raw_edid, size_t raw_edid_size)
>>   {
>>       bool edid1_present = drm_edid && drm_edid->edid && drm_edid->size;
>>       bool edid2_present = raw_edid && raw_edid_size;
>> @@ -6915,7 +6915,7 @@ static int 
>> _drm_edid_connector_property_update(struct drm_connector *connector,
>>           const void *old_edid = connector->edid_blob_ptr->data;
>>           size_t old_edid_size = connector->edid_blob_ptr->length;
>>   -        if (old_edid && !drm_edid_eq(drm_edid, old_edid, 
>> old_edid_size)) {
>> +        if (old_edid && !drm_edid_eq_buf(drm_edid, old_edid, 
>> old_edid_size)) {
>>               connector->epoch_counter++;
>>               drm_dbg_kms(dev, "[CONNECTOR:%d:%s] EDID changed, epoch 
>> counter %llu\n",
>>                       connector->base.id, connector->name,
>> @@ -7520,3 +7520,21 @@ bool drm_edid_is_digital(const struct drm_edid 
>> *drm_edid)
>>           drm_edid->edid->input & DRM_EDID_INPUT_DIGITAL;
>>   }
>>   EXPORT_SYMBOL(drm_edid_is_digital);
>> +
>> +/**
>> + * drm_edid_eq - Check if EDIDs are equal
>> + *
>> + * @drm_edid_1: first drm_edid to compare edid
>> + * @drm_edid_2: second drm_edid to compare edid
>> + *
>> + * Return true if EDIDs are equal.
>> + */
>> +bool drm_edid_eq(const struct drm_edid *drm_edid_1,
>> +         const struct drm_edid *drm_edid_2)
>> +{
>> +    const void *edid_1 = drm_edid_1 ? drm_edid_1->edid : NULL;
>> +    size_t edid_1_size = drm_edid_1 ? drm_edid_1->size : 0;
>> +
>> +    return drm_edid_eq_buf(drm_edid_2, edid_1, edid_1_size);
>
> What happens when the size for edid 2 is different than edid 1? Does 
> drm_edid_eq_buf() already handle this well (I didn't immediately look)?

drm_edid_eq_buf handles this case.
This is the code:

static bool drm_edid_eq_buf(const struct drm_edid *drm_edid,
                                              const void *raw_edid, 
size_t raw_edid_size)
{
     bool edid1_present = drm_edid && drm_edid->edid && drm_edid->size;
     bool edid2_present = raw_edid && raw_edid_size;

     if (edid1_present != edid2_present)
         return false;

     if (edid1_present) {
         if (drm_edid->size != raw_edid_size)
             return false;

         if (memcmp(drm_edid->edid, raw_edid, drm_edid->size))
             return false;
     }

     return true;
}

>
>
> If not; how about including an extra check directly in this function?
>
> I was thinking this will handle it effectively:
>
> if (!drm_edid_1 || !drm_edid_2)
>     return false;
If both EDIDs are NULL, they are equal (and we don't need to update 
anything).
So drm_edid_eq() should return true.


> if (drm_edid_1->size != drm_edid_2->size)
>     return false;
>
And the size comparison is handled by drm_edid_eq_buf() in this part:

     if (edid1_present) {
         if (drm_edid->size != raw_edid_size)
             return false;

> return drm_edid_eq_buf();
>
>
>
>> +}
>> +EXPORT_SYMBOL(drm_edid_eq);
>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>> index 960592167486..e7a9a4928b97 100644
>> --- a/include/drm/drm_edid.h
>> +++ b/include/drm/drm_edid.h
>> @@ -469,6 +469,8 @@ int drm_edid_connector_update(struct 
>> drm_connector *connector,
>>                     const struct drm_edid *edid);
>>   int drm_edid_connector_add_modes(struct drm_connector *connector);
>>   bool drm_edid_is_digital(const struct drm_edid *drm_edid);
>> +bool drm_edid_eq(const struct drm_edid *drm_edid_first,
>> +             const struct drm_edid *drm_edid_second);
>>   void drm_edid_get_product_id(const struct drm_edid *drm_edid,
>>                    struct drm_edid_product_id *id);
>>   void drm_edid_print_product_id(struct drm_printer *p,
>

Re: [PATCH v4 11/14] drm/amd/display: create a function to fill dc_sink with edid data

[Melissa Wen]

On 13/06/2025 15:38, Mario Limonciello wrote:
> On 6/13/2025 9:58 AM, Melissa Wen wrote:
>> From: Rodrigo Siqueira <siqueira@igalia.com>
>>
>> As part of the effort of stopping using raw edid, this commit move the
>> copy of the edid in DC to a dedicated function that will allow the usage
>> of drm_edid in the next steps.
>>
>> v3:
>> - remove superfulous cast (jani)
>
> I don't think that changelog needs to end up in the eventual commit 
> history.  But it's useful while still under review.
Ack
>
> Could you put the changelog below the cutlist (---)?
>
>>
>> Signed-off-by: Rodrigo Siqueira <siqueira@igalia.com>
>> Co-developer--by: Melissa Wen <mwen@igalia.com>
>
> These tags are wrong.  It's "Co-developed-by".
ugh. ack

Thanks

Melissa
>
>> Signed-off-by: Melissa Wen <mwen@igalia.com>
>> ---
>>   drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c      | 8 ++++++++
>>   drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h      | 2 ++
>>   drivers/gpu/drm/amd/display/dc/link/link_detection.c | 3 +--
>>   3 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c 
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
>> index fab873b091f5..a90545b176cc 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
>> @@ -17,3 +17,11 @@ bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
>>          return (memcmp(old_edid->raw_edid,
>>                         new_edid->raw_edid, new_edid->length) == 0);
>>   }
>> +
>> +void dc_edid_copy_edid_to_dc(struct dc_sink *dc_sink,
>> +                 const void *edid,
>> +                 int len)
>> +{
>> +    memmove(dc_sink->dc_edid.raw_edid, edid, len);
>> +    dc_sink->dc_edid.length = len;
>> +}
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h 
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
>> index 7e3b1177bc8a..f42cd5bbc730 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
>> @@ -7,5 +7,7 @@
>>     bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
>>                 struct dc_sink *current_sink);
>> +void dc_edid_copy_edid_to_dc(struct dc_sink *dc_sink,
>> +                 const void *edid, int len);
>>     #endif /* __DC_EDID_H__ */
>> diff --git a/drivers/gpu/drm/amd/display/dc/link/link_detection.c 
>> b/drivers/gpu/drm/amd/display/dc/link/link_detection.c
>> index 344356e26f8b..c28072f980cc 100644
>> --- a/drivers/gpu/drm/amd/display/dc/link/link_detection.c
>> +++ b/drivers/gpu/drm/amd/display/dc/link/link_detection.c
>> @@ -1401,8 +1401,7 @@ struct dc_sink *link_add_remote_sink(
>>       if (!dc_sink)
>>           return NULL;
>>   -    memmove(dc_sink->dc_edid.raw_edid, (const uint8_t *) edid, len);
>> -    dc_sink->dc_edid.length = len;
>> +    dc_edid_copy_edid_to_dc(dc_sink, edid, len);
>>         if (!link_add_remote_sink_helper(
>>               link,
>

Re: [PATCH v4 10/14] drm/amd/display: add a mid-layer file to handle EDID in DC

[Melissa Wen]

On 13/06/2025 15:48, Mario Limonciello wrote:
> On 6/13/2025 9:58 AM, Melissa Wen wrote:
>> From: Rodrigo Siqueira <siqueira@igalia.com>
>>
>> Since DC is a shared code, this commit introduces a new file to work as
>> a mid-layer in DC for the edid manipulation.
>>
>> v3:
>> - rebase on top of asdn
> Can you put changelog below cutlist (---)?
ack
>>
>> Signed-off-by: Rodrigo Siqueira <siqueira@igalia.com>
>> Co-developed-by: Melissa Wen <mwen@igalia.com>
>> Signed-off-by: Melissa Wen <mwen@igalia.com>
>> ---
>>   .../gpu/drm/amd/display/amdgpu_dm/Makefile    |  1 +
>>   .../gpu/drm/amd/display/amdgpu_dm/dc_edid.c   | 19 +++++++++++++++++++
>>   .../gpu/drm/amd/display/amdgpu_dm/dc_edid.h   | 11 +++++++++++
>>   .../drm/amd/display/dc/link/link_detection.c  | 17 +++--------------
>>   4 files changed, 34 insertions(+), 14 deletions(-)
>>   create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
>>   create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile 
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
>> index 7329b8cc2576..09cb94d8e0e4 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
>> @@ -39,6 +39,7 @@ AMDGPUDM = \
>>       amdgpu_dm_psr.o \
>>       amdgpu_dm_replay.o \
>>       amdgpu_dm_quirks.o \
>> +    dc_edid.o \
>>       amdgpu_dm_wb.o
>>     ifdef CONFIG_DRM_AMD_DC_FP
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c 
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
>> new file mode 100644
>> index 000000000000..fab873b091f5
>> --- /dev/null
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
>> @@ -0,0 +1,19 @@
>> +// SPDX-License-Identifier: MIT
>> +#include "amdgpu_dm/dc_edid.h"
>> +#include "dc.h"
>> +
>> +bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
>> +              struct dc_sink *current_sink)
>> +{
>> +    struct dc_edid *old_edid = &prev_sink->dc_edid;
>> +    struct dc_edid *new_edid = &current_sink->dc_edid;
>> +
>> +       if (old_edid->length != new_edid->length)
>> +               return false;
>> +
>> +       if (new_edid->length == 0)
>> +               return false;
>> +
>> +       return (memcmp(old_edid->raw_edid,
>> +                      new_edid->raw_edid, new_edid->length) == 0);
>> +}
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h 
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
>> new file mode 100644
>> index 000000000000..7e3b1177bc8a
>> --- /dev/null
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
>> @@ -0,0 +1,11 @@
>> +/* SPDX-License-Identifier: MIT */
>> +
>> +#ifndef __DC_EDID_H__
>> +#define __DC_EDID_H__
>> +
>> +#include "dc.h"
>> +
>> +bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
>> +              struct dc_sink *current_sink);
>> +
>> +#endif /* __DC_EDID_H__ */
>> diff --git a/drivers/gpu/drm/amd/display/dc/link/link_detection.c 
>> b/drivers/gpu/drm/amd/display/dc/link/link_detection.c
>> index 863c24fe1117..344356e26f8b 100644
>> --- a/drivers/gpu/drm/amd/display/dc/link/link_detection.c
>> +++ b/drivers/gpu/drm/amd/display/dc/link/link_detection.c
>> @@ -48,6 +48,8 @@
>>   #include "dm_helpers.h"
>>   #include "clk_mgr.h"
>>   +#include "dc_edid.h"
>
> There's a problem with the header location.  If you're naming it 
> dc_edid.h but putting the header in amdgpu_dm/ directory then it's 
> only going to compile for amdgpu.
>
> I think dc_edid.h needs to go into dc/ directory.
>
>> +
>>    // Offset DPCD 050Eh == 0x5A
>>   #define MST_HUB_ID_0x5A  0x5A
>>   @@ -617,18 +619,6 @@ static bool detect_dp(struct dc_link *link,
>>       return true;
>>   }
>>   -static bool is_same_edid(struct dc_edid *old_edid, struct dc_edid 
>> *new_edid)
>> -{
>> -    if (old_edid->length != new_edid->length)
>> -        return false;
>> -
>> -    if (new_edid->length == 0)
>> -        return false;
>> -
>> -    return (memcmp(old_edid->raw_edid,
>> -               new_edid->raw_edid, new_edid->length) == 0);
>> -}
>> -
>>   static bool wait_for_entering_dp_alt_mode(struct dc_link *link)
>>   {
>>   @@ -1105,8 +1095,7 @@ static bool detect_link_and_local_sink(struct 
>> dc_link *link,
>>           // Check if edid is the same
>>           if ((prev_sink) &&
>>               (edid_status == EDID_THE_SAME || edid_status == EDID_OK))
>> -            same_edid = is_same_edid(&prev_sink->dc_edid,
>> -                         &sink->dc_edid);
>> +            same_edid = dc_edid_is_same_edid(prev_sink, sink);
>>             if (sink->edid_caps.panel_patch.skip_scdc_overwrite)
>>               link->ctx->dc->debug.hdmi20_disable = true;
>

Re: [PATCH v4 03/14] drm/amd/display: use drm_edid_product_id for parsing EDID product info

[Melissa Wen]

On 13/06/2025 15:53, Mario Limonciello wrote:
> On 6/13/2025 9:58 AM, Melissa Wen wrote:
>> Since [1], we can use drm_edid_product_id to get debug info from
>> drm_edid instead of directly parsing EDID.
>>
>> Link: 
>> https://lore.kernel.org/dri-devel/cover.1712655867.git.jani.nikula@intel.com/ 
>> [1]
>
> This is a pretty old commit.  It's landed now a while, right?
> I'd say if you're going to reference it in the changelog it should be 
> referenced by commit ABC123 ("Foo the bar").

Makes sense.
As it was a series, I'll try to find the most relevant commit and 
replace the reference.

>
>> Signed-off-by: Melissa Wen <mwen@igalia.com>
>> ---
>>   .../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 5543780f1024..b1085f1195f7 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
>> @@ -109,6 +109,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;
>> @@ -125,13 +126,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;
>>         drm_edid_get_monitor_name(edid_buf,
>>                     edid_caps->display_name,
>

Re: [PATCH v4 10/14] drm/amd/display: add a mid-layer file to handle EDID in DC

[Rodrigo Siqueira]

On 06/13, Mario Limonciello wrote:
> On 6/13/2025 9:58 AM, Melissa Wen wrote:
> > From: Rodrigo Siqueira <siqueira@igalia.com>
> > 
> > Since DC is a shared code, this commit introduces a new file to work as
> > a mid-layer in DC for the edid manipulation.
> > 
> > v3:
> > - rebase on top of asdn
> Can you put changelog below cutlist (---)?
> > 
> > Signed-off-by: Rodrigo Siqueira <siqueira@igalia.com>
> > Co-developed-by: Melissa Wen <mwen@igalia.com>
> > Signed-off-by: Melissa Wen <mwen@igalia.com>
> > ---
> >   .../gpu/drm/amd/display/amdgpu_dm/Makefile    |  1 +
> >   .../gpu/drm/amd/display/amdgpu_dm/dc_edid.c   | 19 +++++++++++++++++++
> >   .../gpu/drm/amd/display/amdgpu_dm/dc_edid.h   | 11 +++++++++++
> >   .../drm/amd/display/dc/link/link_detection.c  | 17 +++--------------
> >   4 files changed, 34 insertions(+), 14 deletions(-)
> >   create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
> >   create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
> > 
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile b/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
> > index 7329b8cc2576..09cb94d8e0e4 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
> > @@ -39,6 +39,7 @@ AMDGPUDM = \
> >   	amdgpu_dm_psr.o \
> >   	amdgpu_dm_replay.o \
> >   	amdgpu_dm_quirks.o \
> > +	dc_edid.o \
> >   	amdgpu_dm_wb.o
> >   ifdef CONFIG_DRM_AMD_DC_FP
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
> > new file mode 100644
> > index 000000000000..fab873b091f5
> > --- /dev/null
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
> > @@ -0,0 +1,19 @@
> > +// SPDX-License-Identifier: MIT
> > +#include "amdgpu_dm/dc_edid.h"
> > +#include "dc.h"
> > +
> > +bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
> > +			  struct dc_sink *current_sink)
> > +{
> > +	struct dc_edid *old_edid = &prev_sink->dc_edid;
> > +	struct dc_edid *new_edid = &current_sink->dc_edid;
> > +
> > +       if (old_edid->length != new_edid->length)
> > +               return false;
> > +
> > +       if (new_edid->length == 0)
> > +               return false;
> > +
> > +       return (memcmp(old_edid->raw_edid,
> > +                      new_edid->raw_edid, new_edid->length) == 0);
> > +}
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
> > new file mode 100644
> > index 000000000000..7e3b1177bc8a
> > --- /dev/null
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
> > @@ -0,0 +1,11 @@
> > +/* SPDX-License-Identifier: MIT */
> > +
> > +#ifndef __DC_EDID_H__
> > +#define __DC_EDID_H__
> > +
> > +#include "dc.h"
> > +
> > +bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
> > +			  struct dc_sink *current_sink);
> > +
> > +#endif /* __DC_EDID_H__ */
> > diff --git a/drivers/gpu/drm/amd/display/dc/link/link_detection.c b/drivers/gpu/drm/amd/display/dc/link/link_detection.c
> > index 863c24fe1117..344356e26f8b 100644
> > --- a/drivers/gpu/drm/amd/display/dc/link/link_detection.c
> > +++ b/drivers/gpu/drm/amd/display/dc/link/link_detection.c
> > @@ -48,6 +48,8 @@
> >   #include "dm_helpers.h"
> >   #include "clk_mgr.h"
> > +#include "dc_edid.h"
> 
> There's a problem with the header location.  If you're naming it dc_edid.h
> but putting the header in amdgpu_dm/ directory then it's only going to
> compile for amdgpu.
> 
> I think dc_edid.h needs to go into dc/ directory.

Hey Mario,

DC is shared with other OSes, and sometimes, we need to find ways to
maintain this synergy with some tricks. One of them is to implement a
set of files with standard functions, but these files get different
implementations per OSes. One good example of this approach was the
introduction of the `dc_fpu.[c|h]` file:

 https://gitlab.freedesktop.org/agd5f/linux/-/commit/96ee63730fa30

The idea is that every other OS can implement the dc_fpu.c file as they
want, and we maintain the same interface. The same idea applies to this
patch. We want to keep all the drm-specific things isolated in the
amdgpu_dm, call the generic functions in DC, but other OSes will do the
same with their specific implementation. Additionally, I think that
keeping the dc_ name in the amdgpu folder is also a good indication of
this approach.

Thanks
Siqueira

> 
> > +
> >    // Offset DPCD 050Eh == 0x5A
> >   #define MST_HUB_ID_0x5A  0x5A
> > @@ -617,18 +619,6 @@ static bool detect_dp(struct dc_link *link,
> >   	return true;
> >   }
> > -static bool is_same_edid(struct dc_edid *old_edid, struct dc_edid *new_edid)
> > -{
> > -	if (old_edid->length != new_edid->length)
> > -		return false;
> > -
> > -	if (new_edid->length == 0)
> > -		return false;
> > -
> > -	return (memcmp(old_edid->raw_edid,
> > -		       new_edid->raw_edid, new_edid->length) == 0);
> > -}
> > -
> >   static bool wait_for_entering_dp_alt_mode(struct dc_link *link)
> >   {
> > @@ -1105,8 +1095,7 @@ static bool detect_link_and_local_sink(struct dc_link *link,
> >   		// Check if edid is the same
> >   		if ((prev_sink) &&
> >   		    (edid_status == EDID_THE_SAME || edid_status == EDID_OK))
> > -			same_edid = is_same_edid(&prev_sink->dc_edid,
> > -						 &sink->dc_edid);
> > +			same_edid = dc_edid_is_same_edid(prev_sink, sink);
> >   		if (sink->edid_caps.panel_patch.skip_scdc_overwrite)
> >   			link->ctx->dc->debug.hdmi20_disable = true;
> 

-- 
Rodrigo Siqueira

Re: [PATCH v4 10/14] drm/amd/display: add a mid-layer file to handle EDID in DC

[Limonciello, Mario]

On 6/17/25 1:56 PM, Rodrigo Siqueira wrote:
> On 06/13, Mario Limonciello wrote:
>> On 6/13/2025 9:58 AM, Melissa Wen wrote:
>>> From: Rodrigo Siqueira <siqueira@igalia.com>
>>>
>>> Since DC is a shared code, this commit introduces a new file to work as
>>> a mid-layer in DC for the edid manipulation.
>>>
>>> v3:
>>> - rebase on top of asdn
>> Can you put changelog below cutlist (---)?
>>>
>>> Signed-off-by: Rodrigo Siqueira <siqueira@igalia.com>
>>> Co-developed-by: Melissa Wen <mwen@igalia.com>
>>> Signed-off-by: Melissa Wen <mwen@igalia.com>
>>> ---
>>>    .../gpu/drm/amd/display/amdgpu_dm/Makefile    |  1 +
>>>    .../gpu/drm/amd/display/amdgpu_dm/dc_edid.c   | 19 +++++++++++++++++++
>>>    .../gpu/drm/amd/display/amdgpu_dm/dc_edid.h   | 11 +++++++++++
>>>    .../drm/amd/display/dc/link/link_detection.c  | 17 +++--------------
>>>    4 files changed, 34 insertions(+), 14 deletions(-)
>>>    create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
>>>    create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile b/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
>>> index 7329b8cc2576..09cb94d8e0e4 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
>>> @@ -39,6 +39,7 @@ AMDGPUDM = \
>>>    	amdgpu_dm_psr.o \
>>>    	amdgpu_dm_replay.o \
>>>    	amdgpu_dm_quirks.o \
>>> +	dc_edid.o \
>>>    	amdgpu_dm_wb.o
>>>    ifdef CONFIG_DRM_AMD_DC_FP
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
>>> new file mode 100644
>>> index 000000000000..fab873b091f5
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c
>>> @@ -0,0 +1,19 @@
>>> +// SPDX-License-Identifier: MIT
>>> +#include "amdgpu_dm/dc_edid.h"
>>> +#include "dc.h"
>>> +
>>> +bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
>>> +			  struct dc_sink *current_sink)
>>> +{
>>> +	struct dc_edid *old_edid = &prev_sink->dc_edid;
>>> +	struct dc_edid *new_edid = &current_sink->dc_edid;
>>> +
>>> +       if (old_edid->length != new_edid->length)
>>> +               return false;
>>> +
>>> +       if (new_edid->length == 0)
>>> +               return false;
>>> +
>>> +       return (memcmp(old_edid->raw_edid,
>>> +                      new_edid->raw_edid, new_edid->length) == 0);
>>> +}
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
>>> new file mode 100644
>>> index 000000000000..7e3b1177bc8a
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h
>>> @@ -0,0 +1,11 @@
>>> +/* SPDX-License-Identifier: MIT */
>>> +
>>> +#ifndef __DC_EDID_H__
>>> +#define __DC_EDID_H__
>>> +
>>> +#include "dc.h"
>>> +
>>> +bool dc_edid_is_same_edid(struct dc_sink *prev_sink,
>>> +			  struct dc_sink *current_sink);
>>> +
>>> +#endif /* __DC_EDID_H__ */
>>> diff --git a/drivers/gpu/drm/amd/display/dc/link/link_detection.c b/drivers/gpu/drm/amd/display/dc/link/link_detection.c
>>> index 863c24fe1117..344356e26f8b 100644
>>> --- a/drivers/gpu/drm/amd/display/dc/link/link_detection.c
>>> +++ b/drivers/gpu/drm/amd/display/dc/link/link_detection.c
>>> @@ -48,6 +48,8 @@
>>>    #include "dm_helpers.h"
>>>    #include "clk_mgr.h"
>>> +#include "dc_edid.h"
>>
>> There's a problem with the header location.  If you're naming it dc_edid.h
>> but putting the header in amdgpu_dm/ directory then it's only going to
>> compile for amdgpu.
>>
>> I think dc_edid.h needs to go into dc/ directory.
> 
> Hey Mario,
> 
> DC is shared with other OSes, and sometimes, we need to find ways to
> maintain this synergy with some tricks. One of them is to implement a
> set of files with standard functions, but these files get different
> implementations per OSes. One good example of this approach was the
> introduction of the `dc_fpu.[c|h]` file:
> 
>   https://gitlab.freedesktop.org/agd5f/linux/-/commit/96ee63730fa30
> 
> The idea is that every other OS can implement the dc_fpu.c file as they
> want, and we maintain the same interface. The same idea applies to this
> patch. We want to keep all the drm-specific things isolated in the
> amdgpu_dm, call the generic functions in DC, but other OSes will do the
> same with their specific implementation. Additionally, I think that
> keeping the dc_ name in the amdgpu folder is also a good indication of
> this approach.
> 
> Thanks
> Siqueira

Siqueira,

Thanks for the comment and clarifying the situation.  You know this area 
better than most people, and thanks for explaining it to me.

THanks,

> 
>>
>>> +
>>>     // Offset DPCD 050Eh == 0x5A
>>>    #define MST_HUB_ID_0x5A  0x5A
>>> @@ -617,18 +619,6 @@ static bool detect_dp(struct dc_link *link,
>>>    	return true;
>>>    }
>>> -static bool is_same_edid(struct dc_edid *old_edid, struct dc_edid *new_edid)
>>> -{
>>> -	if (old_edid->length != new_edid->length)
>>> -		return false;
>>> -
>>> -	if (new_edid->length == 0)
>>> -		return false;
>>> -
>>> -	return (memcmp(old_edid->raw_edid,
>>> -		       new_edid->raw_edid, new_edid->length) == 0);
>>> -}
>>> -
>>>    static bool wait_for_entering_dp_alt_mode(struct dc_link *link)
>>>    {
>>> @@ -1105,8 +1095,7 @@ static bool detect_link_and_local_sink(struct dc_link *link,
>>>    		// Check if edid is the same
>>>    		if ((prev_sink) &&
>>>    		    (edid_status == EDID_THE_SAME || edid_status == EDID_OK))
>>> -			same_edid = is_same_edid(&prev_sink->dc_edid,
>>> -						 &sink->dc_edid);
>>> +			same_edid = dc_edid_is_same_edid(prev_sink, sink);
>>>    		if (sink->edid_caps.panel_patch.skip_scdc_overwrite)
>>>    			link->ctx->dc->debug.hdmi20_disable = true;
>>
>