[RFC PATCH v3 0/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid

[RFC PATCH v3 0/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid

[Melissa Wen]

Hi,

After finding a null-pointer dereference when running
igt@kms_connector_force_edid (fixed by [1]), I started migrating raw
edid handles in amdgpu connectors to use opaque drm_edid and its
helpers.

However, I'm struggling to remove all drm_edid_raw() occurrences for
different reasons and, with this RFC, I'm looking for suggestions to
overcome them:
1. `dc_link` ops need a copy of edid data and size via `dc_sink`
   (basically, dc_link_add_remote_sink()). I'm unsure what a possible
   approach to clean dc_link helpers should be since it seems part of the
   shared code. Any suggestions?

2. Most of the edid data handled by `dm_helpers_parse_edid_caps()` are
   in drm_edid halpers, but there are still a few that are not managed by
   them yet. For example:
   ```
	edid_caps->serial_number = edid_buf->serial;
	edid_caps->manufacture_week = edid_buf->mfg_week;
	edid_caps->manufacture_year = edid_buf->mfg_year;
   ```
   AFAIU I see the same pending issue in i915/pnpid_get_panel_type() -
   that still uses drm_edid_raw().

My suggestion is to keep the drm_edid_raw() around and verify if there
are no regressions or functional changes after this switch of
amdgpu_dm_connector to struct drm_edid. I added FIXME comments to all
drm_edid_raw occurrences to keep these pending issues on our radar. So
we can continue removing driver-specific code in favor of drm_edid
common code. What do you think?

This is the current status of drm_edid migration in this patchset:
- patch 1-2: remove unused variables found in this process.
- patch 3-4: migration of amdgpu_dm_connector from struct edid to struct
  drm_edid and its helpers.
- patch 5-6: remove duplicated code for parsing vrr caps. WIP: require
  more validation.

Additionally, it needs extensive testing and validation of a large
variety of display caps and I don't have the required setup for it
(perhaps you can test it in your CI to point issues?). I don't see how
to reduce the scope of changes to mitigate the noise/disruption, but any
suggestions are welcome.

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

Thanks,

Melissa

v1: https://lore.kernel.org/amd-gfx/20240126163429.56714-1-mwen@igalia.com/
- use const to fix compilation warnings (Alex Hung)
- remove unused variables
- remove driver-specific parser for connector info in favor of drm_edid
  common code

v2: https://lore.kernel.org/amd-gfx/20240327165828.288792-1-mwen@igalia.com/
- fix general protection fault on mst

Melissa Wen (6):
  drm/amd/display: remove unused pixel_clock_mhz from
    amdgpu_dm_connector
  drm/amd/display: clean unused variables for hdmi freesync parser
  drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid
  drm/amd/display: switch to setting physical address directly
  drm/amd/display: always call connector_update when parsing
    freesync_caps
  drm/amd/display: remove redundant freesync parser for DP

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 187 +++++-------------
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   5 +-
 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |   8 +-
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c   |  24 +--
 4 files changed, 69 insertions(+), 155 deletions(-)

-- 
2.43.0

[RFC PATCH v3 1/6] drm/amd/display: remove unused pixel_clock_mhz from amdgpu_dm_connector

[Melissa Wen]

We set pixel_clock_mhz when updating freesync caps, but it's not used
anywhere. Remove it.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 ---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 -
 2 files changed, 4 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 ef546932f6c9..05ffe844b1fc 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -11260,7 +11260,6 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
 
 		amdgpu_dm_connector->min_vfreq = 0;
 		amdgpu_dm_connector->max_vfreq = 0;
-		amdgpu_dm_connector->pixel_clock_mhz = 0;
 		connector->display_info.monitor_range.min_vfreq = 0;
 		connector->display_info.monitor_range.max_vfreq = 0;
 		freesync_capable = false;
@@ -11324,8 +11323,6 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
 					connector->display_info.monitor_range.min_vfreq;
 				amdgpu_dm_connector->max_vfreq =
 					connector->display_info.monitor_range.max_vfreq;
-				amdgpu_dm_connector->pixel_clock_mhz =
-					range->pixel_clock_mhz * 10;
 
 				break;
 			}
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index 09519b7abf67..67647bb5999b 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -678,7 +678,6 @@ struct amdgpu_dm_connector {
 	 * value is set to zero when there is no FreeSync support.
 	 */
 	int max_vfreq ;
-	int pixel_clock_mhz;
 
 	/* Audio instance - protected by audio_lock. */
 	int audio_inst;
-- 
2.43.0

[RFC PATCH v3 2/6] drm/amd/display: clean unused variables for hdmi freesync parser

[Melissa Wen]

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 ---
 1 file changed, 3 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 05ffe844b1fc..280562707cd0 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -11344,9 +11344,6 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
 	} else if (edid && sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A) {
 		i = parse_hdmi_amd_vsdb(amdgpu_dm_connector, edid, &vsdb_info);
 		if (i >= 0 && vsdb_info.freesync_supported) {
-			timing  = &edid->detailed_timings[i];
-			data    = &timing->data.other_data;
-
 			amdgpu_dm_connector->min_vfreq = vsdb_info.min_refresh_rate_hz;
 			amdgpu_dm_connector->max_vfreq = vsdb_info.max_refresh_rate_hz;
 			if (amdgpu_dm_connector->max_vfreq - amdgpu_dm_connector->min_vfreq > 10)
-- 
2.43.0

[RFC PATCH v3 3/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid

[Melissa Wen]

Replace raw edid handling (struct edid) with the opaque EDID type
(struct drm_edid) on amdgpu_dm_connector for consistency. It may also
prevent mismatch of approaches in different parts of the driver code.
Working in progress. It was only exercised with IGT tests.

v2: use const to fix warnings (Alex Hung)
v3: fix general protection fault on mst

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 99 +++++++++----------
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  4 +-
 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  8 +-
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c   | 24 ++---
 4 files changed, 67 insertions(+), 68 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 280562707cd0..bbbf9c9d40d5 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3269,18 +3269,19 @@ void amdgpu_dm_update_connector_after_detect(
 		aconnector->dc_sink = sink;
 		dc_sink_retain(aconnector->dc_sink);
 		if (sink->dc_edid.length == 0) {
-			aconnector->edid = NULL;
+			drm_edid_free(aconnector->edid);
 			if (aconnector->dc_link->aux_mode) {
 				drm_dp_cec_unset_edid(
 					&aconnector->dm_dp_aux.aux);
 			}
 		} else {
-			aconnector->edid =
-				(struct edid *)sink->dc_edid.raw_edid;
+			const struct edid *edid = (const struct edid *)sink->dc_edid.raw_edid;
+			aconnector->edid = drm_edid_alloc(edid, sink->dc_edid.length);
 
+			/* FIXME: Get rid of drm_edid_raw() */
 			if (aconnector->dc_link->aux_mode)
 				drm_dp_cec_set_edid(&aconnector->dm_dp_aux.aux,
-						    aconnector->edid);
+						    drm_edid_raw(aconnector->edid));
 		}
 
 		if (!aconnector->timing_requested) {
@@ -3291,17 +3292,17 @@ void amdgpu_dm_update_connector_after_detect(
 					"failed to create aconnector->requested_timing\n");
 		}
 
-		drm_connector_update_edid_property(connector, aconnector->edid);
+		drm_edid_connector_update(connector, aconnector->edid);
 		amdgpu_dm_update_freesync_caps(connector, aconnector->edid);
 		update_connector_ext_caps(aconnector);
 	} else {
 		drm_dp_cec_unset_edid(&aconnector->dm_dp_aux.aux);
 		amdgpu_dm_update_freesync_caps(connector, NULL);
-		drm_connector_update_edid_property(connector, NULL);
+		drm_edid_connector_update(connector, NULL);
 		aconnector->num_modes = 0;
 		dc_sink_release(aconnector->dc_sink);
 		aconnector->dc_sink = NULL;
-		aconnector->edid = NULL;
+		drm_edid_free(aconnector->edid);
 		kfree(aconnector->timing_requested);
 		aconnector->timing_requested = NULL;
 		/* Set CP to DESIRED if it was ENABLED, so we can re-enable it again on hotplug */
@@ -6661,13 +6662,7 @@ static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector)
 	struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
 	struct dc_link *dc_link = aconnector->dc_link;
 	struct dc_sink *dc_em_sink = aconnector->dc_em_sink;
-	struct edid *edid;
-	struct i2c_adapter *ddc;
-
-	if (dc_link && dc_link->aux_mode)
-		ddc = &aconnector->dm_dp_aux.aux.ddc;
-	else
-		ddc = &aconnector->i2c->base;
+	const struct drm_edid *drm_edid;
 
 	/*
 	 * Note: drm_get_edid gets edid in the following order:
@@ -6675,18 +6670,20 @@ static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector)
 	 * 2) firmware EDID if set via edid_firmware module parameter
 	 * 3) regular DDC read.
 	 */
-	edid = drm_get_edid(connector, ddc);
-	if (!edid) {
+	drm_edid = drm_edid_read(connector);
+
+	if (!drm_edid) {
 		DRM_ERROR("No EDID found on connector: %s.\n", connector->name);
 		return;
 	}
-
-	aconnector->edid = edid;
-
+	aconnector->edid = drm_edid;
+	drm_edid_connector_update(connector, drm_edid);
 	/* Update emulated (virtual) sink's EDID */
 	if (dc_em_sink && dc_link) {
+		const struct edid *edid = drm_edid_raw(drm_edid); // FIXME: Get rid of drm_edid_raw()
+
 		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);
+		memmove(dc_em_sink->dc_edid.raw_edid, (uint8_t *)edid, (edid->extensions + 1) * EDID_LENGTH);
 		dm_helpers_parse_edid_caps(
 			dc_link,
 			&dc_em_sink->dc_edid,
@@ -6716,18 +6713,12 @@ static int get_modes(struct drm_connector *connector)
 static void create_eml_sink(struct amdgpu_dm_connector *aconnector)
 {
 	struct drm_connector *connector = &aconnector->base;
-	struct dc_link *dc_link = aconnector->dc_link;
 	struct dc_sink_init_data init_params = {
 			.link = aconnector->dc_link,
 			.sink_signal = SIGNAL_TYPE_VIRTUAL
 	};
-	struct edid *edid;
-	struct i2c_adapter *ddc;
-
-	if (dc_link->aux_mode)
-		ddc = &aconnector->dm_dp_aux.aux.ddc;
-	else
-		ddc = &aconnector->i2c->base;
+	const struct drm_edid *drm_edid;
+	const struct edid *edid;
 
 	/*
 	 * Note: drm_get_edid gets edid in the following order:
@@ -6735,17 +6726,19 @@ static void create_eml_sink(struct amdgpu_dm_connector *aconnector)
 	 * 2) firmware EDID if set via edid_firmware module parameter
 	 * 3) regular DDC read.
 	 */
-	edid = drm_get_edid(connector, ddc);
-	if (!edid) {
+	drm_edid = drm_edid_read(connector);
+	if (!drm_edid) {
 		DRM_ERROR("No EDID found on connector: %s.\n", connector->name);
 		return;
 	}
 
-	if (drm_detect_hdmi_monitor(edid))
+	if (connector->display_info.is_hdmi)
 		init_params.sink_signal = SIGNAL_TYPE_HDMI_TYPE_A;
 
-	aconnector->edid = edid;
+	aconnector->edid = drm_edid;
+	drm_edid_connector_update(connector, 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,
@@ -7428,16 +7421,16 @@ static void amdgpu_set_panel_orientation(struct drm_connector *connector)
 }
 
 static void amdgpu_dm_connector_ddc_get_modes(struct drm_connector *connector,
-					      struct edid *edid)
+					      const struct drm_edid *drm_edid)
 {
 	struct amdgpu_dm_connector *amdgpu_dm_connector =
 			to_amdgpu_dm_connector(connector);
 
-	if (edid) {
+	if (drm_edid) {
 		/* empty probed_modes */
 		INIT_LIST_HEAD(&connector->probed_modes);
 		amdgpu_dm_connector->num_modes =
-				drm_add_edid_modes(connector, edid);
+				drm_edid_connector_add_modes(connector);
 
 		/* sorting the probed modes before calling function
 		 * amdgpu_dm_get_native_mode() since EDID can have
@@ -7451,10 +7444,10 @@ static void amdgpu_dm_connector_ddc_get_modes(struct drm_connector *connector,
 		amdgpu_dm_get_native_mode(connector);
 
 		/* Freesync capabilities are reset by calling
-		 * drm_add_edid_modes() and need to be
+		 * drm_edid_connector_add_modes() and need to be
 		 * restored here.
 		 */
-		amdgpu_dm_update_freesync_caps(connector, edid);
+		amdgpu_dm_update_freesync_caps(connector, drm_edid);
 	} else {
 		amdgpu_dm_connector->num_modes = 0;
 	}
@@ -7550,12 +7543,12 @@ static uint add_fs_modes(struct amdgpu_dm_connector *aconnector)
 }
 
 static void amdgpu_dm_connector_add_freesync_modes(struct drm_connector *connector,
-						   struct edid *edid)
+						   const struct drm_edid *drm_edid)
 {
 	struct amdgpu_dm_connector *amdgpu_dm_connector =
 		to_amdgpu_dm_connector(connector);
 
-	if (!(amdgpu_freesync_vid_mode && edid))
+	if (!(amdgpu_freesync_vid_mode && drm_edid))
 		return;
 
 	if (amdgpu_dm_connector->max_vfreq - amdgpu_dm_connector->min_vfreq > 10)
@@ -7568,23 +7561,23 @@ static int amdgpu_dm_connector_get_modes(struct drm_connector *connector)
 	struct amdgpu_dm_connector *amdgpu_dm_connector =
 			to_amdgpu_dm_connector(connector);
 	struct drm_encoder *encoder;
-	struct edid *edid = amdgpu_dm_connector->edid;
+	const struct drm_edid *drm_edid = amdgpu_dm_connector->edid;
 	struct dc_link_settings *verified_link_cap =
 			&amdgpu_dm_connector->dc_link->verified_link_cap;
 	const struct dc *dc = amdgpu_dm_connector->dc_link->dc;
 
 	encoder = amdgpu_dm_connector_to_encoder(connector);
 
-	if (!drm_edid_is_valid(edid)) {
+	if (!drm_edid_valid(drm_edid)) {
 		amdgpu_dm_connector->num_modes =
 				drm_add_modes_noedid(connector, 640, 480);
 		if (dc->link_srv->dp_get_encoding_format(verified_link_cap) == DP_128b_132b_ENCODING)
 			amdgpu_dm_connector->num_modes +=
 				drm_add_modes_noedid(connector, 1920, 1080);
 	} else {
-		amdgpu_dm_connector_ddc_get_modes(connector, edid);
+		amdgpu_dm_connector_ddc_get_modes(connector, drm_edid);
 		amdgpu_dm_connector_add_common_modes(encoder, connector);
-		amdgpu_dm_connector_add_freesync_modes(connector, edid);
+		amdgpu_dm_connector_add_freesync_modes(connector, drm_edid);
 	}
 	amdgpu_dm_fbc_init(connector);
 
@@ -11153,7 +11146,7 @@ static bool parse_edid_cea(struct amdgpu_dm_connector *aconnector,
 }
 
 static int parse_amd_vsdb(struct amdgpu_dm_connector *aconnector,
-			  struct edid *edid, struct amdgpu_hdmi_vsdb_info *vsdb_info)
+			  const struct edid *edid, struct amdgpu_hdmi_vsdb_info *vsdb_info)
 {
 	u8 *edid_ext = NULL;
 	int i;
@@ -11188,7 +11181,8 @@ static int parse_amd_vsdb(struct amdgpu_dm_connector *aconnector,
 }
 
 static int parse_hdmi_amd_vsdb(struct amdgpu_dm_connector *aconnector,
-		struct edid *edid, struct amdgpu_hdmi_vsdb_info *vsdb_info)
+			       const struct edid *edid,
+			       struct amdgpu_hdmi_vsdb_info *vsdb_info)
 {
 	u8 *edid_ext = NULL;
 	int i;
@@ -11230,19 +11224,19 @@ static int parse_hdmi_amd_vsdb(struct amdgpu_dm_connector *aconnector,
  * FreeSync parameters.
  */
 void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
-				    struct edid *edid)
+				    const struct drm_edid *drm_edid)
 {
 	int i = 0;
-	struct detailed_timing *timing;
-	struct detailed_non_pixel *data;
-	struct detailed_data_monitor_range *range;
+	const struct detailed_timing *timing;
+	const struct detailed_non_pixel *data;
+	const struct detailed_data_monitor_range *range;
 	struct amdgpu_dm_connector *amdgpu_dm_connector =
 			to_amdgpu_dm_connector(connector);
 	struct dm_connector_state *dm_con_state = NULL;
 	struct dc_sink *sink;
-
 	struct amdgpu_device *adev = drm_to_adev(connector->dev);
 	struct amdgpu_hdmi_vsdb_info vsdb_info = {0};
+	const struct edid *edid;
 	bool freesync_capable = false;
 	enum adaptive_sync_type as_type = ADAPTIVE_SYNC_TYPE_NONE;
 
@@ -11255,7 +11249,7 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
 		amdgpu_dm_connector->dc_sink :
 		amdgpu_dm_connector->dc_em_sink;
 
-	if (!edid || !sink) {
+	if (!drm_edid || !sink) {
 		dm_con_state = to_dm_connector_state(connector->state);
 
 		amdgpu_dm_connector->min_vfreq = 0;
@@ -11272,6 +11266,7 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
 	if (!adev->dm.freesync_module)
 		goto update;
 
+	edid = drm_edid_raw(drm_edid); // FIXME: Get rid of drm_edid_raw()
 	if (edid && (sink->sink_signal == SIGNAL_TYPE_DISPLAY_PORT ||
 		     sink->sink_signal == SIGNAL_TYPE_EDP)) {
 		bool edid_check_required = false;
@@ -11341,7 +11336,7 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
 			amdgpu_dm_connector->as_type = ADAPTIVE_SYNC_TYPE_EDP;
 		}
 
-	} else if (edid && sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A) {
+	} else if (drm_edid && sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A) {
 		i = parse_hdmi_amd_vsdb(amdgpu_dm_connector, edid, &vsdb_info);
 		if (i >= 0 && vsdb_info.freesync_supported) {
 			amdgpu_dm_connector->min_vfreq = vsdb_info.min_refresh_rate_hz;
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index 67647bb5999b..384f68203590 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -637,7 +637,7 @@ struct amdgpu_dm_connector {
 
 	/* we need to mind the EDID between detect
 	   and get modes due to analog/digital/tvencoder */
-	struct edid *edid;
+	const struct drm_edid *edid;
 
 	/* shared with amdgpu */
 	struct amdgpu_hpd hpd;
@@ -908,7 +908,7 @@ void dm_restore_drm_connector_state(struct drm_device *dev,
 				    struct drm_connector *connector);
 
 void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
-					struct edid *edid);
+				    const struct drm_edid *drm_edid);
 
 void amdgpu_dm_trigger_timing_sync(struct drm_device *dev);
 
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 c27063305a13..1a37dff9c2b6 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
@@ -901,7 +901,8 @@ enum dc_edid_status dm_helpers_read_local_edid(
 	struct i2c_adapter *ddc;
 	int retry = 3;
 	enum dc_edid_status edid_status;
-	struct edid *edid;
+	const struct drm_edid *drm_edid;
+	const struct edid *edid;
 
 	if (link->aux_mode)
 		ddc = &aconnector->dm_dp_aux.aux.ddc;
@@ -913,7 +914,8 @@ enum dc_edid_status dm_helpers_read_local_edid(
 	 */
 	do {
 
-		edid = drm_get_edid(&aconnector->base, ddc);
+		drm_edid = drm_edid_read_ddc(connector, ddc);
+		edid = drm_edid_raw(drm_edid); // FIXME: Get rid of drm_edid_raw()
 
 		/* DP Compliance Test 4.2.2.6 */
 		if (link->aux_mode && connector->edid_corrupt)
@@ -931,7 +933,7 @@ enum dc_edid_status dm_helpers_read_local_edid(
 		memmove(sink->dc_edid.raw_edid, (uint8_t *)edid, sink->dc_edid.length);
 
 		/* We don't need the original edid anymore */
-		kfree(edid);
+		drm_edid_free(drm_edid);
 
 		edid_status = dm_helpers_parse_edid_caps(
 						link,
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 941e96f100f4..a28004f9378a 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
@@ -128,7 +128,7 @@ dm_dp_mst_connector_destroy(struct drm_connector *connector)
 		dc_sink_release(aconnector->dc_sink);
 	}
 
-	kfree(aconnector->edid);
+	drm_edid_free(aconnector->edid);
 
 	drm_connector_cleanup(connector);
 	drm_dp_mst_put_port_malloc(aconnector->mst_output_port);
@@ -298,15 +298,15 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
 		return drm_add_edid_modes(connector, NULL);
 
 	if (!aconnector->edid) {
-		struct edid *edid;
+		const struct drm_edid *drm_edid;
 
-		edid = drm_dp_mst_get_edid(connector, &aconnector->mst_root->mst_mgr, aconnector->mst_output_port);
+		drm_edid = drm_dp_mst_edid_read(connector, &aconnector->mst_root->mst_mgr, aconnector->mst_output_port);
 
-		if (!edid) {
+		if (!drm_edid) {
 			amdgpu_dm_set_mst_status(&aconnector->mst_status,
 			MST_REMOTE_EDID, false);
 
-			drm_connector_update_edid_property(
+			drm_edid_connector_update(
 				&aconnector->base,
 				NULL);
 
@@ -340,7 +340,7 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
 			return ret;
 		}
 
-		aconnector->edid = edid;
+		aconnector->edid = drm_edid;
 		amdgpu_dm_set_mst_status(&aconnector->mst_status,
 			MST_REMOTE_EDID, true);
 	}
@@ -355,10 +355,13 @@ 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->edid); // FIXME: Get rid of drm_edid_raw()
 		dc_sink = dc_link_add_remote_sink(
 			aconnector->dc_link,
-			(uint8_t *)aconnector->edid,
-			(aconnector->edid->extensions + 1) * EDID_LENGTH,
+			(uint8_t *)edid,
+			(edid->extensions + 1) * EDID_LENGTH,
 			&init_params);
 
 		if (!dc_sink) {
@@ -412,10 +415,9 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
 		}
 	}
 
-	drm_connector_update_edid_property(
-					&aconnector->base, aconnector->edid);
+	drm_edid_connector_update(&aconnector->base, aconnector->edid);
 
-	ret = drm_add_edid_modes(connector, aconnector->edid);
+	ret = drm_edid_connector_add_modes(connector);
 
 	return ret;
 }
-- 
2.43.0

[RFC PATCH v3 4/6] drm/amd/display: switch to setting physical address directly

[Melissa Wen]

Connectors have source physical address available in display
info. Use drm_dp_cec_attach() to use it instead of parsing the EDID
again.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 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 bbbf9c9d40d5..edc5418db5be 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3277,11 +3277,11 @@ void amdgpu_dm_update_connector_after_detect(
 		} else {
 			const struct edid *edid = (const struct edid *)sink->dc_edid.raw_edid;
 			aconnector->edid = drm_edid_alloc(edid, sink->dc_edid.length);
+			drm_edid_connector_update(connector, aconnector->edid);
 
-			/* FIXME: Get rid of drm_edid_raw() */
 			if (aconnector->dc_link->aux_mode)
-				drm_dp_cec_set_edid(&aconnector->dm_dp_aux.aux,
-						    drm_edid_raw(aconnector->edid));
+				drm_dp_cec_attach(&aconnector->dm_dp_aux.aux,
+						  connector->display_info.source_physical_address);
 		}
 
 		if (!aconnector->timing_requested) {
-- 
2.43.0

[RFC PATCH v3 5/6] drm/amd/display: always call connector_update when parsing freesync_caps

[Melissa Wen]

Update connector caps with drm_edid data before parsing info for
freesync.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 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 edc5418db5be..b3c396d626e9 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3292,13 +3292,11 @@ void amdgpu_dm_update_connector_after_detect(
 					"failed to create aconnector->requested_timing\n");
 		}
 
-		drm_edid_connector_update(connector, aconnector->edid);
 		amdgpu_dm_update_freesync_caps(connector, aconnector->edid);
 		update_connector_ext_caps(aconnector);
 	} else {
 		drm_dp_cec_unset_edid(&aconnector->dm_dp_aux.aux);
 		amdgpu_dm_update_freesync_caps(connector, NULL);
-		drm_edid_connector_update(connector, NULL);
 		aconnector->num_modes = 0;
 		dc_sink_release(aconnector->dc_sink);
 		aconnector->dc_sink = NULL;
@@ -11249,6 +11247,8 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
 		amdgpu_dm_connector->dc_sink :
 		amdgpu_dm_connector->dc_em_sink;
 
+	drm_edid_connector_update(connector, drm_edid);
+
 	if (!drm_edid || !sink) {
 		dm_con_state = to_dm_connector_state(connector->state);
 
-- 
2.43.0

[RFC PATCH v3 6/6] drm/amd/display: remove redundant freesync parser for DP

[Melissa Wen]

When updating connector under drm_edid infrastructure, many calculations
and validations are already done and become redundant inside AMD driver.
Remove those driver-specific code in favor of the DRM common code.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 86 +------------------
 1 file changed, 4 insertions(+), 82 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 b3c396d626e9..7e0f93de27e6 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -10994,24 +10994,6 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
 	return ret;
 }
 
-static bool is_dp_capable_without_timing_msa(struct dc *dc,
-					     struct amdgpu_dm_connector *amdgpu_dm_connector)
-{
-	u8 dpcd_data;
-	bool capable = false;
-
-	if (amdgpu_dm_connector->dc_link &&
-		dm_helpers_dp_read_dpcd(
-				NULL,
-				amdgpu_dm_connector->dc_link,
-				DP_DOWN_STREAM_PORT_COUNT,
-				&dpcd_data,
-				sizeof(dpcd_data))) {
-		capable = (dpcd_data & DP_MSA_TIMING_PAR_IGNORED) ? true:false;
-	}
-
-	return capable;
-}
 
 static bool dm_edid_parser_send_cea(struct amdgpu_display_manager *dm,
 		unsigned int offset,
@@ -11225,9 +11207,6 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
 				    const struct drm_edid *drm_edid)
 {
 	int i = 0;
-	const struct detailed_timing *timing;
-	const struct detailed_non_pixel *data;
-	const struct detailed_data_monitor_range *range;
 	struct amdgpu_dm_connector *amdgpu_dm_connector =
 			to_amdgpu_dm_connector(connector);
 	struct dm_connector_state *dm_con_state = NULL;
@@ -11254,8 +11233,6 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
 
 		amdgpu_dm_connector->min_vfreq = 0;
 		amdgpu_dm_connector->max_vfreq = 0;
-		connector->display_info.monitor_range.min_vfreq = 0;
-		connector->display_info.monitor_range.max_vfreq = 0;
 		freesync_capable = false;
 
 		goto update;
@@ -11269,65 +11246,11 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
 	edid = drm_edid_raw(drm_edid); // FIXME: Get rid of drm_edid_raw()
 	if (edid && (sink->sink_signal == SIGNAL_TYPE_DISPLAY_PORT ||
 		     sink->sink_signal == SIGNAL_TYPE_EDP)) {
-		bool edid_check_required = false;
+		amdgpu_dm_connector->min_vfreq = connector->display_info.monitor_range.min_vfreq;
+		amdgpu_dm_connector->max_vfreq = connector->display_info.monitor_range.max_vfreq;
+		if (amdgpu_dm_connector->max_vfreq - amdgpu_dm_connector->min_vfreq > 10)
+			freesync_capable = true;
 
-		if (is_dp_capable_without_timing_msa(adev->dm.dc,
-						     amdgpu_dm_connector)) {
-			if (edid->features & DRM_EDID_FEATURE_CONTINUOUS_FREQ) {
-				freesync_capable = true;
-				amdgpu_dm_connector->min_vfreq = connector->display_info.monitor_range.min_vfreq;
-				amdgpu_dm_connector->max_vfreq = connector->display_info.monitor_range.max_vfreq;
-			} else {
-				edid_check_required = edid->version > 1 ||
-						      (edid->version == 1 &&
-						       edid->revision > 1);
-			}
-		}
-
-		if (edid_check_required) {
-			for (i = 0; i < 4; i++) {
-
-				timing	= &edid->detailed_timings[i];
-				data	= &timing->data.other_data;
-				range	= &data->data.range;
-				/*
-				 * Check if monitor has continuous frequency mode
-				 */
-				if (data->type != EDID_DETAIL_MONITOR_RANGE)
-					continue;
-				/*
-				 * Check for flag range limits only. If flag == 1 then
-				 * no additional timing information provided.
-				 * Default GTF, GTF Secondary curve and CVT are not
-				 * supported
-				 */
-				if (range->flags != 1)
-					continue;
-
-				connector->display_info.monitor_range.min_vfreq = range->min_vfreq;
-				connector->display_info.monitor_range.max_vfreq = range->max_vfreq;
-
-				if (edid->revision >= 4) {
-					if (data->pad2 & DRM_EDID_RANGE_OFFSET_MIN_VFREQ)
-						connector->display_info.monitor_range.min_vfreq += 255;
-					if (data->pad2 & DRM_EDID_RANGE_OFFSET_MAX_VFREQ)
-						connector->display_info.monitor_range.max_vfreq += 255;
-				}
-
-				amdgpu_dm_connector->min_vfreq =
-					connector->display_info.monitor_range.min_vfreq;
-				amdgpu_dm_connector->max_vfreq =
-					connector->display_info.monitor_range.max_vfreq;
-
-				break;
-			}
-
-			if (amdgpu_dm_connector->max_vfreq -
-			    amdgpu_dm_connector->min_vfreq > 10) {
-
-				freesync_capable = true;
-			}
-		}
 		parse_amd_vsdb(amdgpu_dm_connector, edid, &vsdb_info);
 
 		if (vsdb_info.replay_mode) {
@@ -11335,7 +11258,6 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
 			amdgpu_dm_connector->vsdb_info.amd_vsdb_version = vsdb_info.amd_vsdb_version;
 			amdgpu_dm_connector->as_type = ADAPTIVE_SYNC_TYPE_EDP;
 		}
-
 	} else if (drm_edid && sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A) {
 		i = parse_hdmi_amd_vsdb(amdgpu_dm_connector, edid, &vsdb_info);
 		if (i >= 0 && vsdb_info.freesync_supported) {
-- 
2.43.0

Re: [RFC PATCH v3 0/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid

[Jani Nikula]

On Wed, 27 Mar 2024, Melissa Wen <mwen@igalia.com> wrote:
> 2. Most of the edid data handled by `dm_helpers_parse_edid_caps()` are
>    in drm_edid halpers, but there are still a few that are not managed by
>    them yet. For example:
>    ```
> 	edid_caps->serial_number = edid_buf->serial;
> 	edid_caps->manufacture_week = edid_buf->mfg_week;
> 	edid_caps->manufacture_year = edid_buf->mfg_year;
>    ```
>    AFAIU I see the same pending issue in i915/pnpid_get_panel_type() -
>    that still uses drm_edid_raw().

See https://lore.kernel.org/r/cover.1711015462.git.jani.nikula@intel.com

BR,
Jani.


-- 
Jani Nikula, Intel

Re: [RFC PATCH v3 3/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid

[Jani Nikula]

On Wed, 27 Mar 2024, Melissa Wen <mwen@igalia.com> wrote:
> Replace raw edid handling (struct edid) with the opaque EDID type
> (struct drm_edid) on amdgpu_dm_connector for consistency. It may also
> prevent mismatch of approaches in different parts of the driver code.
> Working in progress. It was only exercised with IGT tests.
>
> v2: use const to fix warnings (Alex Hung)
> v3: fix general protection fault on mst
>
> Signed-off-by: Melissa Wen <mwen@igalia.com>
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 99 +++++++++----------
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  4 +-
>  .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  8 +-
>  .../display/amdgpu_dm/amdgpu_dm_mst_types.c   | 24 ++---
>  4 files changed, 67 insertions(+), 68 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 280562707cd0..bbbf9c9d40d5 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3269,18 +3269,19 @@ void amdgpu_dm_update_connector_after_detect(
>  		aconnector->dc_sink = sink;
>  		dc_sink_retain(aconnector->dc_sink);
>  		if (sink->dc_edid.length == 0) {
> -			aconnector->edid = NULL;
> +			drm_edid_free(aconnector->edid);

If aconnector->edid is used as some kind of cache, it might be prudent
to still set it to NULL.

It's up to the amd maintainers, but personally I've renamed any ->edid
fields to ->drm_edid when I've done these conversions to ensure every
single place is covered, and also later stable backports will give a
build failure if the change is not taken into account.

>  			if (aconnector->dc_link->aux_mode) {
>  				drm_dp_cec_unset_edid(
>  					&aconnector->dm_dp_aux.aux);
>  			}
>  		} else {
> -			aconnector->edid =
> -				(struct edid *)sink->dc_edid.raw_edid;
> +			const struct edid *edid = (const struct edid *)sink->dc_edid.raw_edid;
> +			aconnector->edid = drm_edid_alloc(edid, sink->dc_edid.length);
>  
> +			/* FIXME: Get rid of drm_edid_raw() */
>  			if (aconnector->dc_link->aux_mode)
>  				drm_dp_cec_set_edid(&aconnector->dm_dp_aux.aux,
> -						    aconnector->edid);
> +						    drm_edid_raw(aconnector->edid));

The goal should be to switch to use CEC functions that take the source
physical address directly instead of parsing the EDID.

>  		}
>  
>  		if (!aconnector->timing_requested) {
> @@ -3291,17 +3292,17 @@ void amdgpu_dm_update_connector_after_detect(
>  					"failed to create aconnector->requested_timing\n");
>  		}
>  
> -		drm_connector_update_edid_property(connector, aconnector->edid);
> +		drm_edid_connector_update(connector, aconnector->edid);
>  		amdgpu_dm_update_freesync_caps(connector, aconnector->edid);
>  		update_connector_ext_caps(aconnector);
>  	} else {
>  		drm_dp_cec_unset_edid(&aconnector->dm_dp_aux.aux);
>  		amdgpu_dm_update_freesync_caps(connector, NULL);
> -		drm_connector_update_edid_property(connector, NULL);
> +		drm_edid_connector_update(connector, NULL);
>  		aconnector->num_modes = 0;
>  		dc_sink_release(aconnector->dc_sink);
>  		aconnector->dc_sink = NULL;
> -		aconnector->edid = NULL;
> +		drm_edid_free(aconnector->edid);
>  		kfree(aconnector->timing_requested);
>  		aconnector->timing_requested = NULL;
>  		/* Set CP to DESIRED if it was ENABLED, so we can re-enable it again on hotplug */
> @@ -6661,13 +6662,7 @@ static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector)
>  	struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
>  	struct dc_link *dc_link = aconnector->dc_link;
>  	struct dc_sink *dc_em_sink = aconnector->dc_em_sink;
> -	struct edid *edid;
> -	struct i2c_adapter *ddc;
> -
> -	if (dc_link && dc_link->aux_mode)
> -		ddc = &aconnector->dm_dp_aux.aux.ddc;
> -	else
> -		ddc = &aconnector->i2c->base;
> +	const struct drm_edid *drm_edid;
>  
>  	/*
>  	 * Note: drm_get_edid gets edid in the following order:
> @@ -6675,18 +6670,20 @@ static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector)
>  	 * 2) firmware EDID if set via edid_firmware module parameter
>  	 * 3) regular DDC read.
>  	 */
> -	edid = drm_get_edid(connector, ddc);
> -	if (!edid) {
> +	drm_edid = drm_edid_read(connector);

You should call drm_edid_connector_update() here to clear the EDID
property etc. if it's NULL.

Ditto everywhere.

Due to a historical mistake, drm_get_edid() does that for you, but
shouldn't really.

> +
> +	if (!drm_edid) {
>  		DRM_ERROR("No EDID found on connector: %s.\n", connector->name);
>  		return;
>  	}
> -
> -	aconnector->edid = edid;
> -
> +	aconnector->edid = drm_edid;
> +	drm_edid_connector_update(connector, drm_edid);
>  	/* Update emulated (virtual) sink's EDID */
>  	if (dc_em_sink && dc_link) {
> +		const struct edid *edid = drm_edid_raw(drm_edid); // FIXME: Get rid of drm_edid_raw()
> +
>  		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);
> +		memmove(dc_em_sink->dc_edid.raw_edid, (uint8_t *)edid, (edid->extensions + 1) * EDID_LENGTH);
>  		dm_helpers_parse_edid_caps(
>  			dc_link,
>  			&dc_em_sink->dc_edid,
> @@ -6716,18 +6713,12 @@ static int get_modes(struct drm_connector *connector)
>  static void create_eml_sink(struct amdgpu_dm_connector *aconnector)
>  {
>  	struct drm_connector *connector = &aconnector->base;
> -	struct dc_link *dc_link = aconnector->dc_link;
>  	struct dc_sink_init_data init_params = {
>  			.link = aconnector->dc_link,
>  			.sink_signal = SIGNAL_TYPE_VIRTUAL
>  	};
> -	struct edid *edid;
> -	struct i2c_adapter *ddc;
> -
> -	if (dc_link->aux_mode)
> -		ddc = &aconnector->dm_dp_aux.aux.ddc;
> -	else
> -		ddc = &aconnector->i2c->base;
> +	const struct drm_edid *drm_edid;
> +	const struct edid *edid;
>  
>  	/*
>  	 * Note: drm_get_edid gets edid in the following order:
> @@ -6735,17 +6726,19 @@ static void create_eml_sink(struct amdgpu_dm_connector *aconnector)
>  	 * 2) firmware EDID if set via edid_firmware module parameter
>  	 * 3) regular DDC read.
>  	 */
> -	edid = drm_get_edid(connector, ddc);
> -	if (!edid) {
> +	drm_edid = drm_edid_read(connector);
> +	if (!drm_edid) {
>  		DRM_ERROR("No EDID found on connector: %s.\n", connector->name);
>  		return;
>  	}
>  
> -	if (drm_detect_hdmi_monitor(edid))
> +	if (connector->display_info.is_hdmi)
>  		init_params.sink_signal = SIGNAL_TYPE_HDMI_TYPE_A;
>  
> -	aconnector->edid = edid;
> +	aconnector->edid = drm_edid;
> +	drm_edid_connector_update(connector, 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,
> @@ -7428,16 +7421,16 @@ static void amdgpu_set_panel_orientation(struct drm_connector *connector)
>  }
>  
>  static void amdgpu_dm_connector_ddc_get_modes(struct drm_connector *connector,
> -					      struct edid *edid)
> +					      const struct drm_edid *drm_edid)
>  {
>  	struct amdgpu_dm_connector *amdgpu_dm_connector =
>  			to_amdgpu_dm_connector(connector);
>  
> -	if (edid) {
> +	if (drm_edid) {
>  		/* empty probed_modes */
>  		INIT_LIST_HEAD(&connector->probed_modes);
>  		amdgpu_dm_connector->num_modes =
> -				drm_add_edid_modes(connector, edid);
> +				drm_edid_connector_add_modes(connector);
>  
>  		/* sorting the probed modes before calling function
>  		 * amdgpu_dm_get_native_mode() since EDID can have
> @@ -7451,10 +7444,10 @@ static void amdgpu_dm_connector_ddc_get_modes(struct drm_connector *connector,
>  		amdgpu_dm_get_native_mode(connector);
>  
>  		/* Freesync capabilities are reset by calling
> -		 * drm_add_edid_modes() and need to be
> +		 * drm_edid_connector_add_modes() and need to be
>  		 * restored here.
>  		 */
> -		amdgpu_dm_update_freesync_caps(connector, edid);
> +		amdgpu_dm_update_freesync_caps(connector, drm_edid);
>  	} else {
>  		amdgpu_dm_connector->num_modes = 0;
>  	}
> @@ -7550,12 +7543,12 @@ static uint add_fs_modes(struct amdgpu_dm_connector *aconnector)
>  }
>  
>  static void amdgpu_dm_connector_add_freesync_modes(struct drm_connector *connector,
> -						   struct edid *edid)
> +						   const struct drm_edid *drm_edid)
>  {
>  	struct amdgpu_dm_connector *amdgpu_dm_connector =
>  		to_amdgpu_dm_connector(connector);
>  
> -	if (!(amdgpu_freesync_vid_mode && edid))
> +	if (!(amdgpu_freesync_vid_mode && drm_edid))
>  		return;
>  
>  	if (amdgpu_dm_connector->max_vfreq - amdgpu_dm_connector->min_vfreq > 10)
> @@ -7568,23 +7561,23 @@ static int amdgpu_dm_connector_get_modes(struct drm_connector *connector)
>  	struct amdgpu_dm_connector *amdgpu_dm_connector =
>  			to_amdgpu_dm_connector(connector);
>  	struct drm_encoder *encoder;
> -	struct edid *edid = amdgpu_dm_connector->edid;
> +	const struct drm_edid *drm_edid = amdgpu_dm_connector->edid;
>  	struct dc_link_settings *verified_link_cap =
>  			&amdgpu_dm_connector->dc_link->verified_link_cap;
>  	const struct dc *dc = amdgpu_dm_connector->dc_link->dc;
>  
>  	encoder = amdgpu_dm_connector_to_encoder(connector);
>  
> -	if (!drm_edid_is_valid(edid)) {
> +	if (!drm_edid_valid(drm_edid)) {

I don't think there should be reasons to validate the EDID at this
point. The EDID reading functions do that, and any amd driver specific
mechanisms should do that separately before setting ->edid. So it should
be just a NULL check here.

>  		amdgpu_dm_connector->num_modes =
>  				drm_add_modes_noedid(connector, 640, 480);
>  		if (dc->link_srv->dp_get_encoding_format(verified_link_cap) == DP_128b_132b_ENCODING)
>  			amdgpu_dm_connector->num_modes +=
>  				drm_add_modes_noedid(connector, 1920, 1080);
>  	} else {
> -		amdgpu_dm_connector_ddc_get_modes(connector, edid);
> +		amdgpu_dm_connector_ddc_get_modes(connector, drm_edid);
>  		amdgpu_dm_connector_add_common_modes(encoder, connector);
> -		amdgpu_dm_connector_add_freesync_modes(connector, edid);
> +		amdgpu_dm_connector_add_freesync_modes(connector, drm_edid);
>  	}
>  	amdgpu_dm_fbc_init(connector);
>  
> @@ -11153,7 +11146,7 @@ static bool parse_edid_cea(struct amdgpu_dm_connector *aconnector,
>  }
>  
>  static int parse_amd_vsdb(struct amdgpu_dm_connector *aconnector,
> -			  struct edid *edid, struct amdgpu_hdmi_vsdb_info *vsdb_info)
> +			  const struct edid *edid, struct amdgpu_hdmi_vsdb_info *vsdb_info)
>  {
>  	u8 *edid_ext = NULL;
>  	int i;
> @@ -11188,7 +11181,8 @@ static int parse_amd_vsdb(struct amdgpu_dm_connector *aconnector,
>  }
>  
>  static int parse_hdmi_amd_vsdb(struct amdgpu_dm_connector *aconnector,
> -		struct edid *edid, struct amdgpu_hdmi_vsdb_info *vsdb_info)
> +			       const struct edid *edid,
> +			       struct amdgpu_hdmi_vsdb_info *vsdb_info)
>  {
>  	u8 *edid_ext = NULL;
>  	int i;
> @@ -11230,19 +11224,19 @@ static int parse_hdmi_amd_vsdb(struct amdgpu_dm_connector *aconnector,
>   * FreeSync parameters.
>   */
>  void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
> -				    struct edid *edid)
> +				    const struct drm_edid *drm_edid)
>  {
>  	int i = 0;
> -	struct detailed_timing *timing;
> -	struct detailed_non_pixel *data;
> -	struct detailed_data_monitor_range *range;
> +	const struct detailed_timing *timing;
> +	const struct detailed_non_pixel *data;
> +	const struct detailed_data_monitor_range *range;
>  	struct amdgpu_dm_connector *amdgpu_dm_connector =
>  			to_amdgpu_dm_connector(connector);
>  	struct dm_connector_state *dm_con_state = NULL;
>  	struct dc_sink *sink;
> -
>  	struct amdgpu_device *adev = drm_to_adev(connector->dev);
>  	struct amdgpu_hdmi_vsdb_info vsdb_info = {0};
> +	const struct edid *edid;
>  	bool freesync_capable = false;
>  	enum adaptive_sync_type as_type = ADAPTIVE_SYNC_TYPE_NONE;
>  
> @@ -11255,7 +11249,7 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
>  		amdgpu_dm_connector->dc_sink :
>  		amdgpu_dm_connector->dc_em_sink;
>  
> -	if (!edid || !sink) {
> +	if (!drm_edid || !sink) {
>  		dm_con_state = to_dm_connector_state(connector->state);
>  
>  		amdgpu_dm_connector->min_vfreq = 0;
> @@ -11272,6 +11266,7 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
>  	if (!adev->dm.freesync_module)
>  		goto update;
>  
> +	edid = drm_edid_raw(drm_edid); // FIXME: Get rid of drm_edid_raw()
>  	if (edid && (sink->sink_signal == SIGNAL_TYPE_DISPLAY_PORT ||
>  		     sink->sink_signal == SIGNAL_TYPE_EDP)) {
>  		bool edid_check_required = false;
> @@ -11341,7 +11336,7 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
>  			amdgpu_dm_connector->as_type = ADAPTIVE_SYNC_TYPE_EDP;
>  		}
>  
> -	} else if (edid && sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A) {
> +	} else if (drm_edid && sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A) {
>  		i = parse_hdmi_amd_vsdb(amdgpu_dm_connector, edid, &vsdb_info);
>  		if (i >= 0 && vsdb_info.freesync_supported) {
>  			amdgpu_dm_connector->min_vfreq = vsdb_info.min_refresh_rate_hz;
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index 67647bb5999b..384f68203590 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -637,7 +637,7 @@ struct amdgpu_dm_connector {
>  
>  	/* we need to mind the EDID between detect
>  	   and get modes due to analog/digital/tvencoder */
> -	struct edid *edid;
> +	const struct drm_edid *edid;

As I said, I'd make this drm_edid. But not my call.

BR,
Jani.

>  
>  	/* shared with amdgpu */
>  	struct amdgpu_hpd hpd;
> @@ -908,7 +908,7 @@ void dm_restore_drm_connector_state(struct drm_device *dev,
>  				    struct drm_connector *connector);
>  
>  void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
> -					struct edid *edid);
> +				    const struct drm_edid *drm_edid);
>  
>  void amdgpu_dm_trigger_timing_sync(struct drm_device *dev);
>  
> 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 c27063305a13..1a37dff9c2b6 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
> @@ -901,7 +901,8 @@ enum dc_edid_status dm_helpers_read_local_edid(
>  	struct i2c_adapter *ddc;
>  	int retry = 3;
>  	enum dc_edid_status edid_status;
> -	struct edid *edid;
> +	const struct drm_edid *drm_edid;
> +	const struct edid *edid;
>  
>  	if (link->aux_mode)
>  		ddc = &aconnector->dm_dp_aux.aux.ddc;
> @@ -913,7 +914,8 @@ enum dc_edid_status dm_helpers_read_local_edid(
>  	 */
>  	do {
>  
> -		edid = drm_get_edid(&aconnector->base, ddc);
> +		drm_edid = drm_edid_read_ddc(connector, ddc);
> +		edid = drm_edid_raw(drm_edid); // FIXME: Get rid of drm_edid_raw()
>  
>  		/* DP Compliance Test 4.2.2.6 */
>  		if (link->aux_mode && connector->edid_corrupt)
> @@ -931,7 +933,7 @@ enum dc_edid_status dm_helpers_read_local_edid(
>  		memmove(sink->dc_edid.raw_edid, (uint8_t *)edid, sink->dc_edid.length);
>  
>  		/* We don't need the original edid anymore */
> -		kfree(edid);
> +		drm_edid_free(drm_edid);
>  
>  		edid_status = dm_helpers_parse_edid_caps(
>  						link,
> 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 941e96f100f4..a28004f9378a 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
> @@ -128,7 +128,7 @@ dm_dp_mst_connector_destroy(struct drm_connector *connector)
>  		dc_sink_release(aconnector->dc_sink);
>  	}
>  
> -	kfree(aconnector->edid);
> +	drm_edid_free(aconnector->edid);
>  
>  	drm_connector_cleanup(connector);
>  	drm_dp_mst_put_port_malloc(aconnector->mst_output_port);
> @@ -298,15 +298,15 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
>  		return drm_add_edid_modes(connector, NULL);
>  
>  	if (!aconnector->edid) {
> -		struct edid *edid;
> +		const struct drm_edid *drm_edid;
>  
> -		edid = drm_dp_mst_get_edid(connector, &aconnector->mst_root->mst_mgr, aconnector->mst_output_port);
> +		drm_edid = drm_dp_mst_edid_read(connector, &aconnector->mst_root->mst_mgr, aconnector->mst_output_port);
>  
> -		if (!edid) {
> +		if (!drm_edid) {
>  			amdgpu_dm_set_mst_status(&aconnector->mst_status,
>  			MST_REMOTE_EDID, false);
>  
> -			drm_connector_update_edid_property(
> +			drm_edid_connector_update(
>  				&aconnector->base,
>  				NULL);
>  
> @@ -340,7 +340,7 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
>  			return ret;
>  		}
>  
> -		aconnector->edid = edid;
> +		aconnector->edid = drm_edid;
>  		amdgpu_dm_set_mst_status(&aconnector->mst_status,
>  			MST_REMOTE_EDID, true);
>  	}
> @@ -355,10 +355,13 @@ 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->edid); // FIXME: Get rid of drm_edid_raw()
>  		dc_sink = dc_link_add_remote_sink(
>  			aconnector->dc_link,
> -			(uint8_t *)aconnector->edid,
> -			(aconnector->edid->extensions + 1) * EDID_LENGTH,
> +			(uint8_t *)edid,
> +			(edid->extensions + 1) * EDID_LENGTH,
>  			&init_params);
>  
>  		if (!dc_sink) {
> @@ -412,10 +415,9 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
>  		}
>  	}
>  
> -	drm_connector_update_edid_property(
> -					&aconnector->base, aconnector->edid);
> +	drm_edid_connector_update(&aconnector->base, aconnector->edid);
>  
> -	ret = drm_add_edid_modes(connector, aconnector->edid);
> +	ret = drm_edid_connector_add_modes(connector);
>  
>  	return ret;
>  }

-- 
Jani Nikula, Intel

Re: [RFC PATCH v3 3/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid

[Melissa Wen]

On 03/28, Jani Nikula wrote:
> On Wed, 27 Mar 2024, Melissa Wen <mwen@igalia.com> wrote:
> > Replace raw edid handling (struct edid) with the opaque EDID type
> > (struct drm_edid) on amdgpu_dm_connector for consistency. It may also
> > prevent mismatch of approaches in different parts of the driver code.
> > Working in progress. It was only exercised with IGT tests.
> >
> > v2: use const to fix warnings (Alex Hung)
> > v3: fix general protection fault on mst
> >
> > Signed-off-by: Melissa Wen <mwen@igalia.com>
> > ---
> >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 99 +++++++++----------
> >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  4 +-
> >  .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  8 +-
> >  .../display/amdgpu_dm/amdgpu_dm_mst_types.c   | 24 ++---
> >  4 files changed, 67 insertions(+), 68 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 280562707cd0..bbbf9c9d40d5 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -3269,18 +3269,19 @@ void amdgpu_dm_update_connector_after_detect(
> >  		aconnector->dc_sink = sink;
> >  		dc_sink_retain(aconnector->dc_sink);
> >  		if (sink->dc_edid.length == 0) {
> > -			aconnector->edid = NULL;
> > +			drm_edid_free(aconnector->edid);
> 
> If aconnector->edid is used as some kind of cache, it might be prudent
> to still set it to NULL.

Makes sense. Thanks for pointing it out.
> 
> It's up to the amd maintainers, but personally I've renamed any ->edid
> fields to ->drm_edid when I've done these conversions to ensure every
> single place is covered, and also later stable backports will give a
> build failure if the change is not taken into account.

I see. Good points. I'll follow your advice and do this name change in
the next version.
> 
> >  			if (aconnector->dc_link->aux_mode) {
> >  				drm_dp_cec_unset_edid(
> >  					&aconnector->dm_dp_aux.aux);
> >  			}
> >  		} else {
> > -			aconnector->edid =
> > -				(struct edid *)sink->dc_edid.raw_edid;
> > +			const struct edid *edid = (const struct edid *)sink->dc_edid.raw_edid;
> > +			aconnector->edid = drm_edid_alloc(edid, sink->dc_edid.length);
> >  
> > +			/* FIXME: Get rid of drm_edid_raw() */
> >  			if (aconnector->dc_link->aux_mode)
> >  				drm_dp_cec_set_edid(&aconnector->dm_dp_aux.aux,
> > -						    aconnector->edid);
> > +						    drm_edid_raw(aconnector->edid));
> 
> The goal should be to switch to use CEC functions that take the source
> physical address directly instead of parsing the EDID.

Yes, I understand I have the same goal as in the next patch:
- https://lore.kernel.org/dri-devel/20240327214953.367126-5-mwen@igalia.com/

Am I missing something?

> 
> >  		}
> >  
> >  		if (!aconnector->timing_requested) {
> > @@ -3291,17 +3292,17 @@ void amdgpu_dm_update_connector_after_detect(
> >  					"failed to create aconnector->requested_timing\n");
> >  		}
> >  
> > -		drm_connector_update_edid_property(connector, aconnector->edid);
> > +		drm_edid_connector_update(connector, aconnector->edid);
> >  		amdgpu_dm_update_freesync_caps(connector, aconnector->edid);
> >  		update_connector_ext_caps(aconnector);
> >  	} else {
> >  		drm_dp_cec_unset_edid(&aconnector->dm_dp_aux.aux);
> >  		amdgpu_dm_update_freesync_caps(connector, NULL);
> > -		drm_connector_update_edid_property(connector, NULL);
> > +		drm_edid_connector_update(connector, NULL);
> >  		aconnector->num_modes = 0;
> >  		dc_sink_release(aconnector->dc_sink);
> >  		aconnector->dc_sink = NULL;
> > -		aconnector->edid = NULL;
> > +		drm_edid_free(aconnector->edid);
> >  		kfree(aconnector->timing_requested);
> >  		aconnector->timing_requested = NULL;
> >  		/* Set CP to DESIRED if it was ENABLED, so we can re-enable it again on hotplug */
> > @@ -6661,13 +6662,7 @@ static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector)
> >  	struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
> >  	struct dc_link *dc_link = aconnector->dc_link;
> >  	struct dc_sink *dc_em_sink = aconnector->dc_em_sink;
> > -	struct edid *edid;
> > -	struct i2c_adapter *ddc;
> > -
> > -	if (dc_link && dc_link->aux_mode)
> > -		ddc = &aconnector->dm_dp_aux.aux.ddc;
> > -	else
> > -		ddc = &aconnector->i2c->base;
> > +	const struct drm_edid *drm_edid;
> >  
> >  	/*
> >  	 * Note: drm_get_edid gets edid in the following order:
> > @@ -6675,18 +6670,20 @@ static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector)
> >  	 * 2) firmware EDID if set via edid_firmware module parameter
> >  	 * 3) regular DDC read.
> >  	 */
> > -	edid = drm_get_edid(connector, ddc);
> > -	if (!edid) {
> > +	drm_edid = drm_edid_read(connector);
> 
> You should call drm_edid_connector_update() here to clear the EDID
> property etc. if it's NULL.
> 
> Ditto everywhere.
> 
> Due to a historical mistake, drm_get_edid() does that for you, but
> shouldn't really.

Oh, okay. I'll review the overall usage in this series. Thanks for
warning.
> 
> > +
> > +	if (!drm_edid) {
> >  		DRM_ERROR("No EDID found on connector: %s.\n", connector->name);
> >  		return;
> >  	}
> > -
> > -	aconnector->edid = edid;
> > -
> > +	aconnector->edid = drm_edid;
> > +	drm_edid_connector_update(connector, drm_edid);
> >  	/* Update emulated (virtual) sink's EDID */
> >  	if (dc_em_sink && dc_link) {
> > +		const struct edid *edid = drm_edid_raw(drm_edid); // FIXME: Get rid of drm_edid_raw()
> > +
> >  		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);
> > +		memmove(dc_em_sink->dc_edid.raw_edid, (uint8_t *)edid, (edid->extensions + 1) * EDID_LENGTH);
> >  		dm_helpers_parse_edid_caps(
> >  			dc_link,
> >  			&dc_em_sink->dc_edid,
> > @@ -6716,18 +6713,12 @@ static int get_modes(struct drm_connector *connector)
> >  static void create_eml_sink(struct amdgpu_dm_connector *aconnector)
> >  {
> >  	struct drm_connector *connector = &aconnector->base;
> > -	struct dc_link *dc_link = aconnector->dc_link;
> >  	struct dc_sink_init_data init_params = {
> >  			.link = aconnector->dc_link,
> >  			.sink_signal = SIGNAL_TYPE_VIRTUAL
> >  	};
> > -	struct edid *edid;
> > -	struct i2c_adapter *ddc;
> > -
> > -	if (dc_link->aux_mode)
> > -		ddc = &aconnector->dm_dp_aux.aux.ddc;
> > -	else
> > -		ddc = &aconnector->i2c->base;
> > +	const struct drm_edid *drm_edid;
> > +	const struct edid *edid;
> >  
> >  	/*
> >  	 * Note: drm_get_edid gets edid in the following order:
> > @@ -6735,17 +6726,19 @@ static void create_eml_sink(struct amdgpu_dm_connector *aconnector)
> >  	 * 2) firmware EDID if set via edid_firmware module parameter
> >  	 * 3) regular DDC read.
> >  	 */
> > -	edid = drm_get_edid(connector, ddc);
> > -	if (!edid) {
> > +	drm_edid = drm_edid_read(connector);
> > +	if (!drm_edid) {
> >  		DRM_ERROR("No EDID found on connector: %s.\n", connector->name);
> >  		return;
> >  	}
> >  
> > -	if (drm_detect_hdmi_monitor(edid))
> > +	if (connector->display_info.is_hdmi)
> >  		init_params.sink_signal = SIGNAL_TYPE_HDMI_TYPE_A;
> >  
> > -	aconnector->edid = edid;
> > +	aconnector->edid = drm_edid;
> > +	drm_edid_connector_update(connector, 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,
> > @@ -7428,16 +7421,16 @@ static void amdgpu_set_panel_orientation(struct drm_connector *connector)
> >  }
> >  
> >  static void amdgpu_dm_connector_ddc_get_modes(struct drm_connector *connector,
> > -					      struct edid *edid)
> > +					      const struct drm_edid *drm_edid)
> >  {
> >  	struct amdgpu_dm_connector *amdgpu_dm_connector =
> >  			to_amdgpu_dm_connector(connector);
> >  
> > -	if (edid) {
> > +	if (drm_edid) {
> >  		/* empty probed_modes */
> >  		INIT_LIST_HEAD(&connector->probed_modes);
> >  		amdgpu_dm_connector->num_modes =
> > -				drm_add_edid_modes(connector, edid);
> > +				drm_edid_connector_add_modes(connector);
> >  
> >  		/* sorting the probed modes before calling function
> >  		 * amdgpu_dm_get_native_mode() since EDID can have
> > @@ -7451,10 +7444,10 @@ static void amdgpu_dm_connector_ddc_get_modes(struct drm_connector *connector,
> >  		amdgpu_dm_get_native_mode(connector);
> >  
> >  		/* Freesync capabilities are reset by calling
> > -		 * drm_add_edid_modes() and need to be
> > +		 * drm_edid_connector_add_modes() and need to be
> >  		 * restored here.
> >  		 */
> > -		amdgpu_dm_update_freesync_caps(connector, edid);
> > +		amdgpu_dm_update_freesync_caps(connector, drm_edid);
> >  	} else {
> >  		amdgpu_dm_connector->num_modes = 0;
> >  	}
> > @@ -7550,12 +7543,12 @@ static uint add_fs_modes(struct amdgpu_dm_connector *aconnector)
> >  }
> >  
> >  static void amdgpu_dm_connector_add_freesync_modes(struct drm_connector *connector,
> > -						   struct edid *edid)
> > +						   const struct drm_edid *drm_edid)
> >  {
> >  	struct amdgpu_dm_connector *amdgpu_dm_connector =
> >  		to_amdgpu_dm_connector(connector);
> >  
> > -	if (!(amdgpu_freesync_vid_mode && edid))
> > +	if (!(amdgpu_freesync_vid_mode && drm_edid))
> >  		return;
> >  
> >  	if (amdgpu_dm_connector->max_vfreq - amdgpu_dm_connector->min_vfreq > 10)
> > @@ -7568,23 +7561,23 @@ static int amdgpu_dm_connector_get_modes(struct drm_connector *connector)
> >  	struct amdgpu_dm_connector *amdgpu_dm_connector =
> >  			to_amdgpu_dm_connector(connector);
> >  	struct drm_encoder *encoder;
> > -	struct edid *edid = amdgpu_dm_connector->edid;
> > +	const struct drm_edid *drm_edid = amdgpu_dm_connector->edid;
> >  	struct dc_link_settings *verified_link_cap =
> >  			&amdgpu_dm_connector->dc_link->verified_link_cap;
> >  	const struct dc *dc = amdgpu_dm_connector->dc_link->dc;
> >  
> >  	encoder = amdgpu_dm_connector_to_encoder(connector);
> >  
> > -	if (!drm_edid_is_valid(edid)) {
> > +	if (!drm_edid_valid(drm_edid)) {
> 
> I don't think there should be reasons to validate the EDID at this
> point. The EDID reading functions do that, and any amd driver specific
> mechanisms should do that separately before setting ->edid. So it should
> be just a NULL check here.
> 
> >  		amdgpu_dm_connector->num_modes =
> >  				drm_add_modes_noedid(connector, 640, 480);
> >  		if (dc->link_srv->dp_get_encoding_format(verified_link_cap) == DP_128b_132b_ENCODING)
> >  			amdgpu_dm_connector->num_modes +=
> >  				drm_add_modes_noedid(connector, 1920, 1080);
> >  	} else {
> > -		amdgpu_dm_connector_ddc_get_modes(connector, edid);
> > +		amdgpu_dm_connector_ddc_get_modes(connector, drm_edid);
> >  		amdgpu_dm_connector_add_common_modes(encoder, connector);
> > -		amdgpu_dm_connector_add_freesync_modes(connector, edid);
> > +		amdgpu_dm_connector_add_freesync_modes(connector, drm_edid);
> >  	}
> >  	amdgpu_dm_fbc_init(connector);
> >  
> > @@ -11153,7 +11146,7 @@ static bool parse_edid_cea(struct amdgpu_dm_connector *aconnector,
> >  }
> >  
> >  static int parse_amd_vsdb(struct amdgpu_dm_connector *aconnector,
> > -			  struct edid *edid, struct amdgpu_hdmi_vsdb_info *vsdb_info)
> > +			  const struct edid *edid, struct amdgpu_hdmi_vsdb_info *vsdb_info)
> >  {
> >  	u8 *edid_ext = NULL;
> >  	int i;
> > @@ -11188,7 +11181,8 @@ static int parse_amd_vsdb(struct amdgpu_dm_connector *aconnector,
> >  }
> >  
> >  static int parse_hdmi_amd_vsdb(struct amdgpu_dm_connector *aconnector,
> > -		struct edid *edid, struct amdgpu_hdmi_vsdb_info *vsdb_info)
> > +			       const struct edid *edid,
> > +			       struct amdgpu_hdmi_vsdb_info *vsdb_info)
> >  {
> >  	u8 *edid_ext = NULL;
> >  	int i;
> > @@ -11230,19 +11224,19 @@ static int parse_hdmi_amd_vsdb(struct amdgpu_dm_connector *aconnector,
> >   * FreeSync parameters.
> >   */
> >  void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
> > -				    struct edid *edid)
> > +				    const struct drm_edid *drm_edid)
> >  {
> >  	int i = 0;
> > -	struct detailed_timing *timing;
> > -	struct detailed_non_pixel *data;
> > -	struct detailed_data_monitor_range *range;
> > +	const struct detailed_timing *timing;
> > +	const struct detailed_non_pixel *data;
> > +	const struct detailed_data_monitor_range *range;
> >  	struct amdgpu_dm_connector *amdgpu_dm_connector =
> >  			to_amdgpu_dm_connector(connector);
> >  	struct dm_connector_state *dm_con_state = NULL;
> >  	struct dc_sink *sink;
> > -
> >  	struct amdgpu_device *adev = drm_to_adev(connector->dev);
> >  	struct amdgpu_hdmi_vsdb_info vsdb_info = {0};
> > +	const struct edid *edid;
> >  	bool freesync_capable = false;
> >  	enum adaptive_sync_type as_type = ADAPTIVE_SYNC_TYPE_NONE;
> >  
> > @@ -11255,7 +11249,7 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
> >  		amdgpu_dm_connector->dc_sink :
> >  		amdgpu_dm_connector->dc_em_sink;
> >  
> > -	if (!edid || !sink) {
> > +	if (!drm_edid || !sink) {
> >  		dm_con_state = to_dm_connector_state(connector->state);
> >  
> >  		amdgpu_dm_connector->min_vfreq = 0;
> > @@ -11272,6 +11266,7 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
> >  	if (!adev->dm.freesync_module)
> >  		goto update;
> >  
> > +	edid = drm_edid_raw(drm_edid); // FIXME: Get rid of drm_edid_raw()
> >  	if (edid && (sink->sink_signal == SIGNAL_TYPE_DISPLAY_PORT ||
> >  		     sink->sink_signal == SIGNAL_TYPE_EDP)) {
> >  		bool edid_check_required = false;
> > @@ -11341,7 +11336,7 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
> >  			amdgpu_dm_connector->as_type = ADAPTIVE_SYNC_TYPE_EDP;
> >  		}
> >  
> > -	} else if (edid && sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A) {
> > +	} else if (drm_edid && sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A) {
> >  		i = parse_hdmi_amd_vsdb(amdgpu_dm_connector, edid, &vsdb_info);
> >  		if (i >= 0 && vsdb_info.freesync_supported) {
> >  			amdgpu_dm_connector->min_vfreq = vsdb_info.min_refresh_rate_hz;
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> > index 67647bb5999b..384f68203590 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> > @@ -637,7 +637,7 @@ struct amdgpu_dm_connector {
> >  
> >  	/* we need to mind the EDID between detect
> >  	   and get modes due to analog/digital/tvencoder */
> > -	struct edid *edid;
> > +	const struct drm_edid *edid;
> 
> As I said, I'd make this drm_edid. But not my call.
> 
> BR,
> Jani.
> 
> >  
> >  	/* shared with amdgpu */
> >  	struct amdgpu_hpd hpd;
> > @@ -908,7 +908,7 @@ void dm_restore_drm_connector_state(struct drm_device *dev,
> >  				    struct drm_connector *connector);
> >  
> >  void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
> > -					struct edid *edid);
> > +				    const struct drm_edid *drm_edid);
> >  
> >  void amdgpu_dm_trigger_timing_sync(struct drm_device *dev);
> >  
> > 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 c27063305a13..1a37dff9c2b6 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
> > @@ -901,7 +901,8 @@ enum dc_edid_status dm_helpers_read_local_edid(
> >  	struct i2c_adapter *ddc;
> >  	int retry = 3;
> >  	enum dc_edid_status edid_status;
> > -	struct edid *edid;
> > +	const struct drm_edid *drm_edid;
> > +	const struct edid *edid;
> >  
> >  	if (link->aux_mode)
> >  		ddc = &aconnector->dm_dp_aux.aux.ddc;
> > @@ -913,7 +914,8 @@ enum dc_edid_status dm_helpers_read_local_edid(
> >  	 */
> >  	do {
> >  
> > -		edid = drm_get_edid(&aconnector->base, ddc);
> > +		drm_edid = drm_edid_read_ddc(connector, ddc);
> > +		edid = drm_edid_raw(drm_edid); // FIXME: Get rid of drm_edid_raw()
> >  
> >  		/* DP Compliance Test 4.2.2.6 */
> >  		if (link->aux_mode && connector->edid_corrupt)
> > @@ -931,7 +933,7 @@ enum dc_edid_status dm_helpers_read_local_edid(
> >  		memmove(sink->dc_edid.raw_edid, (uint8_t *)edid, sink->dc_edid.length);
> >  
> >  		/* We don't need the original edid anymore */
> > -		kfree(edid);
> > +		drm_edid_free(drm_edid);
> >  
> >  		edid_status = dm_helpers_parse_edid_caps(
> >  						link,
> > 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 941e96f100f4..a28004f9378a 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
> > @@ -128,7 +128,7 @@ dm_dp_mst_connector_destroy(struct drm_connector *connector)
> >  		dc_sink_release(aconnector->dc_sink);
> >  	}
> >  
> > -	kfree(aconnector->edid);
> > +	drm_edid_free(aconnector->edid);
> >  
> >  	drm_connector_cleanup(connector);
> >  	drm_dp_mst_put_port_malloc(aconnector->mst_output_port);
> > @@ -298,15 +298,15 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
> >  		return drm_add_edid_modes(connector, NULL);
> >  
> >  	if (!aconnector->edid) {
> > -		struct edid *edid;
> > +		const struct drm_edid *drm_edid;
> >  
> > -		edid = drm_dp_mst_get_edid(connector, &aconnector->mst_root->mst_mgr, aconnector->mst_output_port);
> > +		drm_edid = drm_dp_mst_edid_read(connector, &aconnector->mst_root->mst_mgr, aconnector->mst_output_port);
> >  
> > -		if (!edid) {
> > +		if (!drm_edid) {
> >  			amdgpu_dm_set_mst_status(&aconnector->mst_status,
> >  			MST_REMOTE_EDID, false);
> >  
> > -			drm_connector_update_edid_property(
> > +			drm_edid_connector_update(
> >  				&aconnector->base,
> >  				NULL);
> >  
> > @@ -340,7 +340,7 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
> >  			return ret;
> >  		}
> >  
> > -		aconnector->edid = edid;
> > +		aconnector->edid = drm_edid;
> >  		amdgpu_dm_set_mst_status(&aconnector->mst_status,
> >  			MST_REMOTE_EDID, true);
> >  	}
> > @@ -355,10 +355,13 @@ 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->edid); // FIXME: Get rid of drm_edid_raw()
> >  		dc_sink = dc_link_add_remote_sink(
> >  			aconnector->dc_link,
> > -			(uint8_t *)aconnector->edid,
> > -			(aconnector->edid->extensions + 1) * EDID_LENGTH,
> > +			(uint8_t *)edid,
> > +			(edid->extensions + 1) * EDID_LENGTH,
> >  			&init_params);
> >  
> >  		if (!dc_sink) {
> > @@ -412,10 +415,9 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
> >  		}
> >  	}
> >  
> > -	drm_connector_update_edid_property(
> > -					&aconnector->base, aconnector->edid);
> > +	drm_edid_connector_update(&aconnector->base, aconnector->edid);
> >  
> > -	ret = drm_add_edid_modes(connector, aconnector->edid);
> > +	ret = drm_edid_connector_add_modes(connector);
> >  
> >  	return ret;
> >  }
> 
> -- 
> Jani Nikula, Intel

Re: [RFC PATCH v3 0/6] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid

[Melissa Wen]

On 03/28, Jani Nikula wrote:
> On Wed, 27 Mar 2024, Melissa Wen <mwen@igalia.com> wrote:
> > 2. Most of the edid data handled by `dm_helpers_parse_edid_caps()` are
> >    in drm_edid halpers, but there are still a few that are not managed by
> >    them yet. For example:
> >    ```
> > 	edid_caps->serial_number = edid_buf->serial;
> > 	edid_caps->manufacture_week = edid_buf->mfg_week;
> > 	edid_caps->manufacture_year = edid_buf->mfg_year;
> >    ```
> >    AFAIU I see the same pending issue in i915/pnpid_get_panel_type() -
> >    that still uses drm_edid_raw().
> 
> See https://lore.kernel.org/r/cover.1711015462.git.jani.nikula@intel.com

Thanks!
I'll work on top of your series.

Melissa

> 
> BR,
> Jani.
> 
> 
> -- 
> Jani Nikula, Intel