[PATCH v4 00/11] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid

[PATCH v4 00/11] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid

[Melissa Wen]

Hi,

This series is an updated version of the previous attempt to make AMD
display driver use opaque drm_edid instead of open struct edid[1]. Here
I address Jani's last review and remove the raw edid handling when
parsing edid caps by using drm_eld info and drm_edid_product_id from
Jani's series[2]. I also dropped the first patch from the previous
version since it was addressed by [3]

amd-staging-drm-next doesn't have the commits that support
drm_edid_product_id, so this version works on top of amd/drm-next
branch. Besides that, currently amd/drm-next has a bug that was already
fixed upstream by [4].

In short, this series applies to amd/drm-next + bug fix.

Patches 1-5 apply to amd-staging-drm-next.
- 1-2 basically change amd connector to store struct drm_edid instead of
  edid with some pending rework to get rid of raw edid.
- 3-5 update the driver code to use drm common-code, removing steps done
  during drm_edid updates.

Patches 6-11 depend on drm_edid_product_id, therefore, it doesn't apply
to amd-staging-drm-next (now it's on top of amd/drm-next). They parse
edid caps from drm_eld and drm_edid_product_id data, removing the need
of handling raw edid in the dm_helpers_parse_edid_caps(), since all
callers of this helper has updated display info from drm_edid at this
point.

To completely remove raw edid, I changed dc/link_detection in the last
commit because all calls of dm_helpers_parse_edid_caps in
link_add_remote_sink are preceded by the setup and update of drm_edid in
the connector, so we can always use the connector->drm_edid. If this
changed is not recommended, we can keep the dc_edid parameter, but
unused.

Finally, there are some pending drm_edid_raw to be addressed in next
iterations.

You can find a branch with amd-staging-drm-next and drm_edid missing
commits here[5].
You can also find a gitlab branch on top of amd/drm-next here[6].

Let me know your thoughts.

Melissa

Change log:
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

v3: https://lore.kernel.org/amd-gfx/20240327214953.367126-1-mwen@igalia.com/
- rename edid to drm_edid in amdgpu_connector (Jani)
- call drm_edid_connector_update to clear edid in case of NULL (Jani)
- keep setting NULL instead of free drm_edid (Jani)
- check drm_edid not NULL, instead of valid (Jani)
- use drm_edid_product_id to parse product info
- use drm_eld info to parse edid caps

[1] https://lore.kernel.org/amd-gfx/20240327214953.367126-1-mwen@igalia.com/
[2] https://lore.kernel.org/dri-devel/cover.1712655867.git.jani.nikula@intel.com
[3] https://lore.kernel.org/amd-gfx/20240509015527.754-1-mario.limonciello@amd.com/
[4] https://lore.kernel.org/amd-gfx/20240610180401.9540-1-Arunpravin.PaneerSelvam@amd.com/
[5] https://gitlab.freedesktop.org/mwen/linux-amd/-/commits/asdn-drm_edid-migration
[6] https://gitlab.freedesktop.org/mwen/linux-amd/-/commits/drm-next-drm_edid-migration

Melissa Wen (11):
  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
  drm/amd/display: use drm_edid_product_id for parsing EDID product info
  drm/amd/display: don't give initial values for sad/b_count
  drm/amd/display: parse display name from drm_eld
  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: remove dc_edid handler from
    dm_helpers_parse_edid_caps

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 198 +++++-------------
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   4 +-
 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 110 +++++-----
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c   |  32 +--
 drivers/gpu/drm/amd/display/dc/dm_helpers.h   |   1 -
 .../drm/amd/display/dc/link/link_detection.c  |   6 +-
 6 files changed, 128 insertions(+), 223 deletions(-)

-- 
2.43.0

[PATCH v4 01/11] 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 98cf523a629e..1dfa7ec9af35 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -12108,9 +12108,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

Re: [PATCH v4 01/11] drm/amd/display: clean unused variables for hdmi freesync parser

[Alex Hung]

Hi Melissa,

There are no commit messages in this patch.

Also, do you think this can be merged with Patch 5 "drm/amd/display: 
remove redundant freesync parser for  DP"?

On 2024-07-05 21:35, Melissa Wen wrote:
> 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 98cf523a629e..1dfa7ec9af35 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -12108,9 +12108,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)

Re: [PATCH v4 01/11] drm/amd/display: clean unused variables for hdmi freesync parser

[Melissa Wen]

On 07/25, Alex Hung wrote:
> Hi Melissa,
> 
> There are no commit messages in this patch.
> 
> Also, do you think this can be merged with Patch 5 "drm/amd/display: remove
> redundant freesync parser for  DP"?

Hi Alex,

Thanks for your feedback.
I'll add a brief description in the next version.
Regarding merging it into patch 5, I'd prefer to keep it detached
because here we have a non-functional change. I can send it as a
separate, single patch from this series to reduce noise and make
validation faster. WDYT?

Melissa

> 
> On 2024-07-05 21:35, Melissa Wen wrote:
> > 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 98cf523a629e..1dfa7ec9af35 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -12108,9 +12108,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)

Re: [PATCH v4 01/11] drm/amd/display: clean unused variables for hdmi freesync parser

[Alex Hung]

On 2024-07-28 19:32, Melissa Wen wrote:
> On 07/25, Alex Hung wrote:
>> Hi Melissa,
>>
>> There are no commit messages in this patch.
>>
>> Also, do you think this can be merged with Patch 5 "drm/amd/display: remove
>> redundant freesync parser for  DP"?
> 
> Hi Alex,
> 
> Thanks for your feedback.
> I'll add a brief description in the next version.
> Regarding merging it into patch 5, I'd prefer to keep it detached
> because here we have a non-functional change. I can send it as a
> separate, single patch from this series to reduce noise and make
> validation faster. WDYT?

Make thanks. Thanks for clarification.

> 
> Melissa
> 
>>
>> On 2024-07-05 21:35, Melissa Wen wrote:
>>> 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 98cf523a629e..1dfa7ec9af35 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -12108,9 +12108,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)

[PATCH v4 02/11] 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
v4: rename edid to drm_edid in amdgpu_connector (Jani)
    call drm_edid_connector_update to clear edid in case of NULL (Jani)
    keep setting NULL instead of free drm_edid (Jani)
    check drm_edid not NULL, instead of valid (Jani)

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 106 +++++++++---------
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   4 +-
 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  13 ++-
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c   |  32 +++---
 4 files changed, 79 insertions(+), 76 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 1dfa7ec9af35..49b8c5b00728 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3408,7 +3408,7 @@ void amdgpu_dm_update_connector_after_detect(
 			aconnector->dc_sink = sink;
 			dc_sink_retain(aconnector->dc_sink);
 			amdgpu_dm_update_freesync_caps(connector,
-					aconnector->edid);
+					aconnector->drm_edid);
 		} else {
 			amdgpu_dm_update_freesync_caps(connector, NULL);
 			if (!aconnector->dc_sink) {
@@ -3467,18 +3467,20 @@ 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;
+			aconnector->drm_edid = NULL;
 			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->drm_edid = drm_edid_alloc(edid, sink->dc_edid.length);
+			drm_edid_connector_update(connector, aconnector->drm_edid);
 
+			/* 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->drm_edid));
 		}
 
 		if (!aconnector->timing_requested) {
@@ -3489,17 +3491,18 @@ void amdgpu_dm_update_connector_after_detect(
 					"failed to create aconnector->requested_timing\n");
 		}
 
-		drm_connector_update_edid_property(connector, aconnector->edid);
-		amdgpu_dm_update_freesync_caps(connector, aconnector->edid);
+		drm_edid_connector_update(connector, aconnector->drm_edid);
+		amdgpu_dm_update_freesync_caps(connector, aconnector->drm_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->drm_edid);
+		aconnector->drm_edid = NULL;
 		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 */
@@ -7002,13 +7005,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:
@@ -7016,18 +7013,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);
+	drm_edid_connector_update(connector, drm_edid);
+	if (!drm_edid) {
 		DRM_ERROR("No EDID found on connector: %s.\n", connector->name);
 		return;
 	}
 
-	aconnector->edid = edid;
-
+	aconnector->drm_edid = 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,
@@ -7057,18 +7056,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:
@@ -7076,17 +7069,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);
+	drm_edid_connector_update(connector, drm_edid);
+	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->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,
@@ -7770,16 +7765,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
@@ -7793,10 +7788,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;
 	}
@@ -7892,12 +7887,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)
@@ -7910,24 +7905,24 @@ 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->drm_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) {
 		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);
 		if (encoder)
 			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);
 
@@ -11867,7 +11862,7 @@ static bool parse_edid_cea(struct amdgpu_dm_connector *aconnector,
 }
 
 static void parse_edid_displayid_vrr(struct drm_connector *connector,
-		struct edid *edid)
+				     const struct edid *edid)
 {
 	u8 *edid_ext = NULL;
 	int i;
@@ -11910,7 +11905,7 @@ static void parse_edid_displayid_vrr(struct drm_connector *connector,
 }
 
 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;
@@ -11945,7 +11940,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;
@@ -11987,19 +11983,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;
 
@@ -12012,7 +12008,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;
@@ -12029,6 +12025,8 @@ 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()
+
 	/* Some eDP panels only have the refresh rate range info in DisplayID */
 	if ((connector->display_info.monitor_range.min_vfreq == 0 ||
 	     connector->display_info.monitor_range.max_vfreq == 0))
@@ -12105,7 +12103,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 5fd1b6b44577..2aff4c4b76de 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -658,7 +658,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 *drm_edid;
 
 	/* shared with amdgpu */
 	struct amdgpu_hpd hpd;
@@ -936,7 +936,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 b490ae67b6be..be72f14f5429 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
@@ -897,7 +897,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;
@@ -909,25 +910,27 @@ 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);
+		drm_edid_connector_update(connector, drm_edid);
 
 		/* DP Compliance Test 4.2.2.6 */
 		if (link->aux_mode && connector->edid_corrupt)
 			drm_dp_send_real_edid_checksum(&aconnector->dm_dp_aux.aux, connector->real_edid_checksum);
 
-		if (!edid && connector->edid_corrupt) {
+		if (!drm_edid && connector->edid_corrupt) {
 			connector->edid_corrupt = false;
 			return EDID_BAD_CHECKSUM;
 		}
 
-		if (!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 */
-		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 5442da90f508..b0d307e5dd72 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
@@ -129,7 +129,7 @@ dm_dp_mst_connector_destroy(struct drm_connector *connector)
 		dc_sink_release(aconnector->dc_sink);
 	}
 
-	kfree(aconnector->edid);
+	drm_edid_free(aconnector->drm_edid);
 
 	drm_connector_cleanup(connector);
 	drm_dp_mst_put_port_malloc(aconnector->mst_output_port);
@@ -182,7 +182,7 @@ amdgpu_dm_mst_connector_early_unregister(struct drm_connector *connector)
 
 		dc_sink_release(dc_sink);
 		aconnector->dc_sink = NULL;
-		aconnector->edid = NULL;
+		aconnector->drm_edid = NULL;
 		aconnector->dsc_aux = NULL;
 		port->passthrough_aux = NULL;
 	}
@@ -302,16 +302,16 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
 	if (!aconnector)
 		return drm_add_edid_modes(connector, NULL);
 
-	if (!aconnector->edid) {
-		struct edid *edid;
+	if (!aconnector->drm_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);
 
@@ -345,7 +345,7 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
 			return ret;
 		}
 
-		aconnector->edid = edid;
+		aconnector->drm_edid = drm_edid;
 		amdgpu_dm_set_mst_status(&aconnector->mst_status,
 			MST_REMOTE_EDID, true);
 	}
@@ -360,10 +360,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->drm_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) {
@@ -405,7 +408,7 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
 
 		if (aconnector->dc_sink) {
 			amdgpu_dm_update_freesync_caps(
-					connector, aconnector->edid);
+					connector, aconnector->drm_edid);
 
 #if defined(CONFIG_DRM_AMD_DC_FP)
 			if (!validate_dsc_caps_on_connector(aconnector))
@@ -419,10 +422,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->drm_edid);
 
-	ret = drm_add_edid_modes(connector, aconnector->edid);
+	ret = drm_edid_connector_add_modes(connector);
 
 	return ret;
 }
@@ -500,7 +502,7 @@ dm_dp_mst_detect(struct drm_connector *connector,
 
 		dc_sink_release(aconnector->dc_sink);
 		aconnector->dc_sink = NULL;
-		aconnector->edid = NULL;
+		aconnector->drm_edid = NULL;
 		aconnector->dsc_aux = NULL;
 		port->passthrough_aux = NULL;
 
-- 
2.43.0

Re: [PATCH v4 02/11] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid

[Alex Hung]

Please see inline comments.

On 2024-07-05 21:35, Melissa Wen 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
> v4: rename edid to drm_edid in amdgpu_connector (Jani)
>      call drm_edid_connector_update to clear edid in case of NULL (Jani)
>      keep setting NULL instead of free drm_edid (Jani)
>      check drm_edid not NULL, instead of valid (Jani)
> 
> Signed-off-by: Melissa Wen <mwen@igalia.com>
> ---
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 106 +++++++++---------
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   4 +-
>   .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  13 ++-
>   .../display/amdgpu_dm/amdgpu_dm_mst_types.c   |  32 +++---
>   4 files changed, 79 insertions(+), 76 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 1dfa7ec9af35..49b8c5b00728 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3408,7 +3408,7 @@ void amdgpu_dm_update_connector_after_detect(
>   			aconnector->dc_sink = sink;
>   			dc_sink_retain(aconnector->dc_sink);
>   			amdgpu_dm_update_freesync_caps(connector,
> -					aconnector->edid);
> +					aconnector->drm_edid);
>   		} else {
>   			amdgpu_dm_update_freesync_caps(connector, NULL);
>   			if (!aconnector->dc_sink) {
> @@ -3467,18 +3467,20 @@ 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;
> +			aconnector->drm_edid = NULL;
>   			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->drm_edid = drm_edid_alloc(edid, sink->dc_edid.length);
> +			drm_edid_connector_update(connector, aconnector->drm_edid);
>   
> +			/* 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);
Why not pass edid but drm_edid_raw(aconnector->drm_edid)?

> +						    drm_edid_raw(aconnector->drm_edid));
>   		}
>   
>   		if (!aconnector->timing_requested) {
> @@ -3489,17 +3491,18 @@ void amdgpu_dm_update_connector_after_detect(
>   					"failed to create aconnector->requested_timing\n");
>   		}
>   
> -		drm_connector_update_edid_property(connector, aconnector->edid);
> -		amdgpu_dm_update_freesync_caps(connector, aconnector->edid);
> +		drm_edid_connector_update(connector, aconnector->drm_edid);
> +		amdgpu_dm_update_freesync_caps(connector, aconnector->drm_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->drm_edid);
> +		aconnector->drm_edid = NULL;
>   		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 */
> @@ -7002,13 +7005,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:
> @@ -7016,18 +7013,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);
drm_get_edid() is removed here, and thhe above comments should be 
removed as well.

> -	if (!edid) {
> +	drm_edid = drm_edid_read(connector);
> +	drm_edid_connector_update(connector, drm_edid);
> +	if (!drm_edid) {
>   		DRM_ERROR("No EDID found on connector: %s.\n", connector->name);
>   		return;
>   	}
>   
> -	aconnector->edid = edid;
> -
> +	aconnector->drm_edid = 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);

is casting to (uint8 *) necessary?

>   		dm_helpers_parse_edid_caps(
>   			dc_link,
>   			&dc_em_sink->dc_edid,
> @@ -7057,18 +7056,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:
> @@ -7076,17 +7069,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);

drm_get_edid() is removed here, and thhe above comments should be 
removed as well.

> -	if (!edid) {
> +	drm_edid = drm_edid_read(connector);
> +	drm_edid_connector_update(connector, drm_edid);
> +	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->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,
> @@ -7770,16 +7765,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
> @@ -7793,10 +7788,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;
>   	}
> @@ -7892,12 +7887,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)
> @@ -7910,24 +7905,24 @@ 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->drm_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) {
>   		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);
>   		if (encoder)
>   			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);
>   
> @@ -11867,7 +11862,7 @@ static bool parse_edid_cea(struct amdgpu_dm_connector *aconnector,
>   }
>   
>   static void parse_edid_displayid_vrr(struct drm_connector *connector,
> -		struct edid *edid)
> +				     const struct edid *edid)
>   {
>   	u8 *edid_ext = NULL;
>   	int i;
> @@ -11910,7 +11905,7 @@ static void parse_edid_displayid_vrr(struct drm_connector *connector,
>   }
>   
>   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;
> @@ -11945,7 +11940,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;
> @@ -11987,19 +11983,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;
>   
> @@ -12012,7 +12008,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;
> @@ -12029,6 +12025,8 @@ 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()
> +
>   	/* Some eDP panels only have the refresh rate range info in DisplayID */
>   	if ((connector->display_info.monitor_range.min_vfreq == 0 ||
>   	     connector->display_info.monitor_range.max_vfreq == 0))
> @@ -12105,7 +12103,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 5fd1b6b44577..2aff4c4b76de 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -658,7 +658,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 *drm_edid;
>   
>   	/* shared with amdgpu */
>   	struct amdgpu_hpd hpd;
> @@ -936,7 +936,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 b490ae67b6be..be72f14f5429 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
> @@ -897,7 +897,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;
> @@ -909,25 +910,27 @@ 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);
> +		drm_edid_connector_update(connector, drm_edid);
>   
>   		/* DP Compliance Test 4.2.2.6 */
>   		if (link->aux_mode && connector->edid_corrupt)
>   			drm_dp_send_real_edid_checksum(&aconnector->dm_dp_aux.aux, connector->real_edid_checksum);
>   
> -		if (!edid && connector->edid_corrupt) {
> +		if (!drm_edid && connector->edid_corrupt) {
>   			connector->edid_corrupt = false;
>   			return EDID_BAD_CHECKSUM;
>   		}
>   
> -		if (!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 */
> -		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 5442da90f508..b0d307e5dd72 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
> @@ -129,7 +129,7 @@ dm_dp_mst_connector_destroy(struct drm_connector *connector)
>   		dc_sink_release(aconnector->dc_sink);
>   	}
>   
> -	kfree(aconnector->edid);
> +	drm_edid_free(aconnector->drm_edid);
>   
>   	drm_connector_cleanup(connector);
>   	drm_dp_mst_put_port_malloc(aconnector->mst_output_port);
> @@ -182,7 +182,7 @@ amdgpu_dm_mst_connector_early_unregister(struct drm_connector *connector)
>   
>   		dc_sink_release(dc_sink);
>   		aconnector->dc_sink = NULL;
> -		aconnector->edid = NULL;
> +		aconnector->drm_edid = NULL;
>   		aconnector->dsc_aux = NULL;
>   		port->passthrough_aux = NULL;
>   	}
> @@ -302,16 +302,16 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
>   	if (!aconnector)
>   		return drm_add_edid_modes(connector, NULL);
>   
> -	if (!aconnector->edid) {
> -		struct edid *edid;
> +	if (!aconnector->drm_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);
>   
> @@ -345,7 +345,7 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
>   			return ret;
>   		}
>   
> -		aconnector->edid = edid;
> +		aconnector->drm_edid = drm_edid;
>   		amdgpu_dm_set_mst_status(&aconnector->mst_status,
>   			MST_REMOTE_EDID, true);
>   	}
> @@ -360,10 +360,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->drm_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) {
> @@ -405,7 +408,7 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
>   
>   		if (aconnector->dc_sink) {
>   			amdgpu_dm_update_freesync_caps(
> -					connector, aconnector->edid);
> +					connector, aconnector->drm_edid);
>   
>   #if defined(CONFIG_DRM_AMD_DC_FP)
>   			if (!validate_dsc_caps_on_connector(aconnector))
> @@ -419,10 +422,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->drm_edid);
>   
> -	ret = drm_add_edid_modes(connector, aconnector->edid);
> +	ret = drm_edid_connector_add_modes(connector);
>   
>   	return ret;
>   }
> @@ -500,7 +502,7 @@ dm_dp_mst_detect(struct drm_connector *connector,
>   
>   		dc_sink_release(aconnector->dc_sink);
>   		aconnector->dc_sink = NULL;
> -		aconnector->edid = NULL;
> +		aconnector->drm_edid = NULL;
>   		aconnector->dsc_aux = NULL;
>   		port->passthrough_aux = NULL;
>   

Re: [PATCH v4 02/11] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid

[Melissa Wen]

On 07/25, Alex Hung wrote:
> Please see inline comments.
> 
> On 2024-07-05 21:35, Melissa Wen 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
> > v4: rename edid to drm_edid in amdgpu_connector (Jani)
> >      call drm_edid_connector_update to clear edid in case of NULL (Jani)
> >      keep setting NULL instead of free drm_edid (Jani)
> >      check drm_edid not NULL, instead of valid (Jani)
> > 
> > Signed-off-by: Melissa Wen <mwen@igalia.com>
> > ---
> >   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 106 +++++++++---------
> >   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   4 +-
> >   .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  13 ++-
> >   .../display/amdgpu_dm/amdgpu_dm_mst_types.c   |  32 +++---
> >   4 files changed, 79 insertions(+), 76 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 1dfa7ec9af35..49b8c5b00728 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -3408,7 +3408,7 @@ void amdgpu_dm_update_connector_after_detect(
> >   			aconnector->dc_sink = sink;
> >   			dc_sink_retain(aconnector->dc_sink);
> >   			amdgpu_dm_update_freesync_caps(connector,
> > -					aconnector->edid);
> > +					aconnector->drm_edid);
> >   		} else {
> >   			amdgpu_dm_update_freesync_caps(connector, NULL);
> >   			if (!aconnector->dc_sink) {
> > @@ -3467,18 +3467,20 @@ 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;
> > +			aconnector->drm_edid = NULL;
> >   			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->drm_edid = drm_edid_alloc(edid, sink->dc_edid.length);
> > +			drm_edid_connector_update(connector, aconnector->drm_edid);
> > +			/* 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);
> Why not pass edid but drm_edid_raw(aconnector->drm_edid)?

ack. Indeed, I can use edid directly.

My intention was to keep the code with drm_edid closer to the original
with edid as an element of aconnector, but this function call will be
removed in the next patch and then the comment will be wrong placed.
> 
> > +						    drm_edid_raw(aconnector->drm_edid));
> >   		}
> >   		if (!aconnector->timing_requested) {
> > @@ -3489,17 +3491,18 @@ void amdgpu_dm_update_connector_after_detect(
> >   					"failed to create aconnector->requested_timing\n");
> >   		}
> > -		drm_connector_update_edid_property(connector, aconnector->edid);
> > -		amdgpu_dm_update_freesync_caps(connector, aconnector->edid);
> > +		drm_edid_connector_update(connector, aconnector->drm_edid);
> > +		amdgpu_dm_update_freesync_caps(connector, aconnector->drm_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->drm_edid);
> > +		aconnector->drm_edid = NULL;
> >   		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 */
> > @@ -7002,13 +7005,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:
> > @@ -7016,18 +7013,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);
> drm_get_edid() is removed here, and thhe above comments should be removed as
> well.

ack.

> 
> > -	if (!edid) {
> > +	drm_edid = drm_edid_read(connector);
> > +	drm_edid_connector_update(connector, drm_edid);
> > +	if (!drm_edid) {
> >   		DRM_ERROR("No EDID found on connector: %s.\n", connector->name);
> >   		return;
> >   	}
> > -	aconnector->edid = edid;
> > -
> > +	aconnector->drm_edid = 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);
> 
> is casting to (uint8 *) necessary?

hmm.. probably not. I'll double check.

Thanks

> 
> >   		dm_helpers_parse_edid_caps(
> >   			dc_link,
> >   			&dc_em_sink->dc_edid,
> > @@ -7057,18 +7056,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:
> > @@ -7076,17 +7069,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);
> 
> drm_get_edid() is removed here, and thhe above comments should be removed as
> well.

ack.

> 
> > -	if (!edid) {
> > +	drm_edid = drm_edid_read(connector);
> > +	drm_edid_connector_update(connector, drm_edid);
> > +	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->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,
> > @@ -7770,16 +7765,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
> > @@ -7793,10 +7788,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;
> >   	}
> > @@ -7892,12 +7887,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)
> > @@ -7910,24 +7905,24 @@ 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->drm_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) {
> >   		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);
> >   		if (encoder)
> >   			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);
> > @@ -11867,7 +11862,7 @@ static bool parse_edid_cea(struct amdgpu_dm_connector *aconnector,
> >   }
> >   static void parse_edid_displayid_vrr(struct drm_connector *connector,
> > -		struct edid *edid)
> > +				     const struct edid *edid)
> >   {
> >   	u8 *edid_ext = NULL;
> >   	int i;
> > @@ -11910,7 +11905,7 @@ static void parse_edid_displayid_vrr(struct drm_connector *connector,
> >   }
> >   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;
> > @@ -11945,7 +11940,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;
> > @@ -11987,19 +11983,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;
> > @@ -12012,7 +12008,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;
> > @@ -12029,6 +12025,8 @@ 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()
> > +
> >   	/* Some eDP panels only have the refresh rate range info in DisplayID */
> >   	if ((connector->display_info.monitor_range.min_vfreq == 0 ||
> >   	     connector->display_info.monitor_range.max_vfreq == 0))
> > @@ -12105,7 +12103,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 5fd1b6b44577..2aff4c4b76de 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> > @@ -658,7 +658,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 *drm_edid;
> >   	/* shared with amdgpu */
> >   	struct amdgpu_hpd hpd;
> > @@ -936,7 +936,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 b490ae67b6be..be72f14f5429 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
> > @@ -897,7 +897,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;
> > @@ -909,25 +910,27 @@ 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);
> > +		drm_edid_connector_update(connector, drm_edid);
> >   		/* DP Compliance Test 4.2.2.6 */
> >   		if (link->aux_mode && connector->edid_corrupt)
> >   			drm_dp_send_real_edid_checksum(&aconnector->dm_dp_aux.aux, connector->real_edid_checksum);
> > -		if (!edid && connector->edid_corrupt) {
> > +		if (!drm_edid && connector->edid_corrupt) {
> >   			connector->edid_corrupt = false;
> >   			return EDID_BAD_CHECKSUM;
> >   		}
> > -		if (!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 */
> > -		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 5442da90f508..b0d307e5dd72 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
> > @@ -129,7 +129,7 @@ dm_dp_mst_connector_destroy(struct drm_connector *connector)
> >   		dc_sink_release(aconnector->dc_sink);
> >   	}
> > -	kfree(aconnector->edid);
> > +	drm_edid_free(aconnector->drm_edid);
> >   	drm_connector_cleanup(connector);
> >   	drm_dp_mst_put_port_malloc(aconnector->mst_output_port);
> > @@ -182,7 +182,7 @@ amdgpu_dm_mst_connector_early_unregister(struct drm_connector *connector)
> >   		dc_sink_release(dc_sink);
> >   		aconnector->dc_sink = NULL;
> > -		aconnector->edid = NULL;
> > +		aconnector->drm_edid = NULL;
> >   		aconnector->dsc_aux = NULL;
> >   		port->passthrough_aux = NULL;
> >   	}
> > @@ -302,16 +302,16 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
> >   	if (!aconnector)
> >   		return drm_add_edid_modes(connector, NULL);
> > -	if (!aconnector->edid) {
> > -		struct edid *edid;
> > +	if (!aconnector->drm_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);
> > @@ -345,7 +345,7 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
> >   			return ret;
> >   		}
> > -		aconnector->edid = edid;
> > +		aconnector->drm_edid = drm_edid;
> >   		amdgpu_dm_set_mst_status(&aconnector->mst_status,
> >   			MST_REMOTE_EDID, true);
> >   	}
> > @@ -360,10 +360,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->drm_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) {
> > @@ -405,7 +408,7 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
> >   		if (aconnector->dc_sink) {
> >   			amdgpu_dm_update_freesync_caps(
> > -					connector, aconnector->edid);
> > +					connector, aconnector->drm_edid);
> >   #if defined(CONFIG_DRM_AMD_DC_FP)
> >   			if (!validate_dsc_caps_on_connector(aconnector))
> > @@ -419,10 +422,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->drm_edid);
> > -	ret = drm_add_edid_modes(connector, aconnector->edid);
> > +	ret = drm_edid_connector_add_modes(connector);
> >   	return ret;
> >   }
> > @@ -500,7 +502,7 @@ dm_dp_mst_detect(struct drm_connector *connector,
> >   		dc_sink_release(aconnector->dc_sink);
> >   		aconnector->dc_sink = NULL;
> > -		aconnector->edid = NULL;
> > +		aconnector->drm_edid = NULL;
> >   		aconnector->dsc_aux = NULL;
> >   		port->passthrough_aux = NULL;

[PATCH v4 03/11] 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 | 5 ++---
 1 file changed, 2 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 49b8c5b00728..163850aeac4a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3477,10 +3477,9 @@ void amdgpu_dm_update_connector_after_detect(
 			aconnector->drm_edid = drm_edid_alloc(edid, sink->dc_edid.length);
 			drm_edid_connector_update(connector, aconnector->drm_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->drm_edid));
+				drm_dp_cec_attach(&aconnector->dm_dp_aux.aux,
+						  connector->display_info.source_physical_address);
 		}
 
 		if (!aconnector->timing_requested) {
-- 
2.43.0

Re: [PATCH v4 03/11] drm/amd/display: switch to setting physical address directly

[Alex Hung]

On 2024-07-05 21:35, Melissa Wen wrote:
> 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 | 5 ++---
>   1 file changed, 2 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 49b8c5b00728..163850aeac4a 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3477,10 +3477,9 @@ void amdgpu_dm_update_connector_after_detect(
>   			aconnector->drm_edid = drm_edid_alloc(edid, sink->dc_edid.length);
>   			drm_edid_connector_update(connector, aconnector->drm_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->drm_edid));
drm_dp_cec_set_edid is removed here. I guess it doesn't matter if edid 
or drm_edid_raw(aconnector->drm_edid) is past here.

> +				drm_dp_cec_attach(&aconnector->dm_dp_aux.aux,
> +						  connector->display_info.source_physical_address);
>   		}
>   
>   		if (!aconnector->timing_requested) {

[PATCH v4 04/11] 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 163850aeac4a..d3f6823fe60b 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3490,13 +3490,11 @@ void amdgpu_dm_update_connector_after_detect(
 					"failed to create aconnector->requested_timing\n");
 		}
 
-		drm_edid_connector_update(connector, aconnector->drm_edid);
 		amdgpu_dm_update_freesync_caps(connector, aconnector->drm_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;
@@ -12007,6 +12005,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

[PATCH v4 05/11] 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 | 88 +------------------
 1 file changed, 4 insertions(+), 84 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 d3f6823fe60b..34e380b4408e 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -11709,24 +11709,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,
@@ -11983,9 +11965,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;
@@ -12012,8 +11991,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;
@@ -12033,67 +12010,11 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
 
 	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) {
-				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;
-			} 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) {
@@ -12101,7 +12022,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

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

[1] https://lore.kernel.org/dri-devel/cover.1712655867.git.jani.nikula@intel.com/

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 37 ++++++++++---------
 1 file changed, 19 insertions(+), 18 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 be72f14f5429..85704fd75ee4 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
@@ -45,16 +45,15 @@
 #include "dm_helpers.h"
 #include "ddc_service_types.h"
 
-static u32 edid_extract_panel_id(struct edid *edid)
+static u32 edid_extract_panel_id(struct drm_edid_product_id *product_id)
 {
-	return (u32)edid->mfg_id[0] << 24   |
-	       (u32)edid->mfg_id[1] << 16   |
-	       (u32)EDID_PRODUCT_ID(edid);
+	return (u32)be16_to_cpu(product_id->manufacturer_name) << 16 |
+	       (u32)le16_to_cpu(product_id->product_code);
 }
 
-static void apply_edid_quirks(struct edid *edid, struct dc_edid_caps *edid_caps)
+static void apply_edid_quirks(struct drm_edid_product_id *product_id, struct dc_edid_caps *edid_caps)
 {
-	uint32_t panel_id = edid_extract_panel_id(edid);
+	uint32_t panel_id = edid_extract_panel_id(product_id);
 
 	switch (panel_id) {
 	/* Workaround for some monitors which does not work well with FAMS */
@@ -94,6 +93,8 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
 {
 	struct amdgpu_dm_connector *aconnector = link->priv;
 	struct drm_connector *connector = &aconnector->base;
+	const struct drm_edid *drm_edid = aconnector->drm_edid;
+	struct drm_edid_product_id product_id;
 	struct edid *edid_buf = edid ? (struct edid *) edid->raw_edid : NULL;
 	struct cea_sad *sads;
 	int sad_count = -1;
@@ -109,13 +110,13 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
 	if (!drm_edid_is_valid(edid_buf))
 		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 = le16_to_cpu(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,
@@ -123,7 +124,7 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
 
 	edid_caps->edid_hdmi = connector->display_info.is_hdmi;
 
-	apply_edid_quirks(edid_buf, edid_caps);
+	apply_edid_quirks(&product_id, edid_caps);
 
 	sad_count = drm_edid_to_sad((struct edid *) edid->raw_edid, &sads);
 	if (sad_count <= 0)
@@ -909,9 +910,9 @@ enum dc_edid_status dm_helpers_read_local_edid(
 	 * do check sum and retry to make sure read correct edid.
 	 */
 	do {
-
 		drm_edid = drm_edid_read_ddc(connector, ddc);
 		drm_edid_connector_update(connector, drm_edid);
+		aconnector->drm_edid = drm_edid;
 
 		/* DP Compliance Test 4.2.2.6 */
 		if (link->aux_mode && connector->edid_corrupt)
@@ -929,14 +930,14 @@ enum dc_edid_status dm_helpers_read_local_edid(
 		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 */
-		drm_edid_free(drm_edid);
-
 		edid_status = dm_helpers_parse_edid_caps(
 						link,
 						&sink->dc_edid,
 						&sink->edid_caps);
 
+		/* We don't need the original edid anymore */
+		drm_edid_free(drm_edid);
+
 	} while (edid_status == EDID_BAD_CHECKSUM && --retry > 0);
 
 	if (edid_status != EDID_OK)
-- 
2.43.0

[PATCH v4 07/11] drm/amd/display: don't give initial values for sad/b_count

[Melissa Wen]

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 3 +--
 1 file changed, 1 insertion(+), 2 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 85704fd75ee4..6df55161d6df 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
@@ -97,8 +97,7 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
 	struct drm_edid_product_id product_id;
 	struct edid *edid_buf = edid ? (struct edid *) edid->raw_edid : NULL;
 	struct cea_sad *sads;
-	int sad_count = -1;
-	int sadb_count = -1;
+	int sad_count, sadb_count;
 	int i = 0;
 	uint8_t *sadb = NULL;
 
-- 
2.43.0

Re: [PATCH v4 07/11] drm/amd/display: don't give initial values for sad/b_count

[Alex Hung]

Can this be merged with [PATCH 10/11] drm/amd/display: get SADB from 
drm_eld when parsing EDID caps

On 2024-07-05 21:35, Melissa Wen wrote:
> Signed-off-by: Melissa Wen <mwen@igalia.com>
> ---
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 3 +--
>   1 file changed, 1 insertion(+), 2 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 85704fd75ee4..6df55161d6df 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
> @@ -97,8 +97,7 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
>   	struct drm_edid_product_id product_id;
>   	struct edid *edid_buf = edid ? (struct edid *) edid->raw_edid : NULL;
>   	struct cea_sad *sads;
> -	int sad_count = -1;
> -	int sadb_count = -1;
> +	int sad_count, sadb_count;
>   	int i = 0;
>   	uint8_t *sadb = NULL;
>   

Re: [PATCH v4 07/11] drm/amd/display: don't give initial values for sad/b_count

[Melissa Wen]

On 07/25, Alex Hung wrote:
> Can this be merged with [PATCH 10/11] drm/amd/display: get SADB from drm_eld
> when parsing EDID caps

sure.

> 
> On 2024-07-05 21:35, Melissa Wen wrote:
> > Signed-off-by: Melissa Wen <mwen@igalia.com>
> > ---
> >   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 3 +--
> >   1 file changed, 1 insertion(+), 2 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 85704fd75ee4..6df55161d6df 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
> > @@ -97,8 +97,7 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
> >   	struct drm_edid_product_id product_id;
> >   	struct edid *edid_buf = edid ? (struct edid *) edid->raw_edid : NULL;
> >   	struct cea_sad *sads;
> > -	int sad_count = -1;
> > -	int sadb_count = -1;
> > +	int sad_count, sadb_count;
> >   	int i = 0;
> >   	uint8_t *sadb = NULL;

[PATCH v4 08/11] drm/amd/display: parse display name from drm_eld

[Melissa Wen]

We don't need to parse dc_edid to get the display name since it's
already set in drm_eld which in turn had it values updated when updating
connector with the opaque drm_edid.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c  | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index 6df55161d6df..7657b1051c54 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
@@ -32,7 +32,7 @@
 #include <drm/amdgpu_drm.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_fixed.h>
-
+#include <drm/drm_eld.h>
 #include "dm_services.h"
 #include "amdgpu.h"
 #include "dc.h"
@@ -77,6 +77,7 @@ static void apply_edid_quirks(struct drm_edid_product_id *product_id, struct dc_
 	}
 }
 
+#define AMDGPU_ELD_DISPLAY_NAME_SIZE_IN_CHARS 13
 /**
  * dm_helpers_parse_edid_caps() - Parse edid caps
  *
@@ -117,9 +118,10 @@ 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,
-				  edid_caps->display_name,
-				  AUDIO_INFO_DISPLAY_NAME_SIZE_IN_CHARS);
+	memset(edid_caps->display_name, 0, AUDIO_INFO_DISPLAY_NAME_SIZE_IN_CHARS);
+	memcpy(edid_caps->display_name,
+	       &connector->eld[DRM_ELD_MONITOR_NAME_STRING],
+	       AMDGPU_ELD_DISPLAY_NAME_SIZE_IN_CHARS);
 
 	edid_caps->edid_hdmi = connector->display_info.is_hdmi;
 
-- 
2.43.0

[PATCH v4 09/11] drm/amd/display: get SAD from drm_eld when parsing EDID caps

[Melissa Wen]

instead of parsing struct edid.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c   | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 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 7657b1051c54..45c04de08c65 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
@@ -97,7 +97,6 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
 	const struct drm_edid *drm_edid = aconnector->drm_edid;
 	struct drm_edid_product_id product_id;
 	struct edid *edid_buf = edid ? (struct edid *) edid->raw_edid : NULL;
-	struct cea_sad *sads;
 	int sad_count, sadb_count;
 	int i = 0;
 	uint8_t *sadb = NULL;
@@ -127,18 +126,21 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
 
 	apply_edid_quirks(&product_id, 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)
 		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) {
-		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);
@@ -153,7 +155,6 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
 	else
 		edid_caps->speaker_flags = DEFAULT_SPEAKER_LOCATION;
 
-	kfree(sads);
 	kfree(sadb);
 
 	return result;
-- 
2.43.0

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

[Alex Hung]

On 2024-07-05 21:35, Melissa Wen wrote:
> instead of parsing struct edid.

A more informative commit message will be helpful.

> 
> Signed-off-by: Melissa Wen <mwen@igalia.com>
> ---
>   .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c   | 17 +++++++++--------
>   1 file changed, 9 insertions(+), 8 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 7657b1051c54..45c04de08c65 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
> @@ -97,7 +97,6 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
>   	const struct drm_edid *drm_edid = aconnector->drm_edid;
>   	struct drm_edid_product_id product_id;
>   	struct edid *edid_buf = edid ? (struct edid *) edid->raw_edid : NULL;
> -	struct cea_sad *sads;
>   	int sad_count, sadb_count;
>   	int i = 0;
>   	uint8_t *sadb = NULL;
> @@ -127,18 +126,21 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
>   
>   	apply_edid_quirks(&product_id, 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)
>   		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) {
> -		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);
> @@ -153,7 +155,6 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
>   	else
>   		edid_caps->speaker_flags = DEFAULT_SPEAKER_LOCATION;
>   
> -	kfree(sads);
>   	kfree(sadb);
>   
>   	return result;

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

[Melissa Wen]

On 07/25, Alex Hung wrote:
> 
> 
> On 2024-07-05 21:35, Melissa Wen wrote:
> > instead of parsing struct edid.
> 
> A more informative commit message will be helpful.

sure. I'll improve it in the next version.
> 
> > 
> > Signed-off-by: Melissa Wen <mwen@igalia.com>
> > ---
> >   .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c   | 17 +++++++++--------
> >   1 file changed, 9 insertions(+), 8 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 7657b1051c54..45c04de08c65 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
> > @@ -97,7 +97,6 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
> >   	const struct drm_edid *drm_edid = aconnector->drm_edid;
> >   	struct drm_edid_product_id product_id;
> >   	struct edid *edid_buf = edid ? (struct edid *) edid->raw_edid : NULL;
> > -	struct cea_sad *sads;
> >   	int sad_count, sadb_count;
> >   	int i = 0;
> >   	uint8_t *sadb = NULL;
> > @@ -127,18 +126,21 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
> >   	apply_edid_quirks(&product_id, 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)
> >   		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) {
> > -		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);
> > @@ -153,7 +155,6 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
> >   	else
> >   		edid_caps->speaker_flags = DEFAULT_SPEAKER_LOCATION;
> > -	kfree(sads);
> >   	kfree(sadb);
> >   	return result;

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

[Alex Hung]

On 2024-07-28 20:02, Melissa Wen wrote:
> On 07/25, Alex Hung wrote:
>>
>>
>> On 2024-07-05 21:35, Melissa Wen wrote:
>>> instead of parsing struct edid.
>>
>> A more informative commit message will be helpful.
> 
> sure. I'll improve it in the next version.

A soft reminder - a few other patches need improved commit messages too.

>>
>>>
>>> Signed-off-by: Melissa Wen <mwen@igalia.com>
>>> ---
>>>    .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c   | 17 +++++++++--------
>>>    1 file changed, 9 insertions(+), 8 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 7657b1051c54..45c04de08c65 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
>>> @@ -97,7 +97,6 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
>>>    	const struct drm_edid *drm_edid = aconnector->drm_edid;
>>>    	struct drm_edid_product_id product_id;
>>>    	struct edid *edid_buf = edid ? (struct edid *) edid->raw_edid : NULL;
>>> -	struct cea_sad *sads;
>>>    	int sad_count, sadb_count;
>>>    	int i = 0;
>>>    	uint8_t *sadb = NULL;
>>> @@ -127,18 +126,21 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
>>>    	apply_edid_quirks(&product_id, 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)
>>>    		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) {
>>> -		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);
>>> @@ -153,7 +155,6 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
>>>    	else
>>>    		edid_caps->speaker_flags = DEFAULT_SPEAKER_LOCATION;
>>> -	kfree(sads);
>>>    	kfree(sadb);
>>>    	return result;

[PATCH v4 10/11] drm/amd/display: get SADB from drm_eld when parsing EDID caps

[Melissa Wen]

instead of parsing struct edid.

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 45c04de08c65..3fb07f437793 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
@@ -97,9 +97,8 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
 	const struct drm_edid *drm_edid = aconnector->drm_edid;
 	struct drm_edid_product_id product_id;
 	struct edid *edid_buf = edid ? (struct edid *) edid->raw_edid : NULL;
-	int sad_count, sadb_count;
+	int sad_count;
 	int i = 0;
-	uint8_t *sadb = NULL;
 
 	enum dc_edid_status result = EDID_OK;
 
@@ -143,20 +142,12 @@ 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);
-
 	return result;
 }
 
-- 
2.43.0

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

[Alex Hung]

On 2024-07-05 21:35, Melissa Wen wrote:
> instead of parsing struct edid.

A more informative commit message will be helpful.

> 
> 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 45c04de08c65..3fb07f437793 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
> @@ -97,9 +97,8 @@ enum dc_edid_status dm_helpers_parse_edid_caps(
>   	const struct drm_edid *drm_edid = aconnector->drm_edid;
>   	struct drm_edid_product_id product_id;
>   	struct edid *edid_buf = edid ? (struct edid *) edid->raw_edid : NULL;
> -	int sad_count, sadb_count;
> +	int sad_count;
>   	int i = 0;
> -	uint8_t *sadb = NULL;
>   
>   	enum dc_edid_status result = EDID_OK;
>   
> @@ -143,20 +142,12 @@ 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);
> -
>   	return result;
>   }
>   

[PATCH v4 11/11] drm/amd/display: remove dc_edid handler from dm_helpers_parse_edid_caps

[Melissa Wen]

We can parse edid caps from drm_edid and drm_eld and any calls of
dm_helpers_parse_edid_caps is made in a state that we have drm_edid set
to amdgpu connector.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  6 ++--
 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 29 +++++++++----------
 drivers/gpu/drm/amd/display/dc/dm_helpers.h   |  1 -
 .../drm/amd/display/dc/link/link_detection.c  |  6 ++--
 4 files changed, 17 insertions(+), 25 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 34e380b4408e..fd3580bf1fb2 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -7024,10 +7024,8 @@ 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, (uint8_t *)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->edid_caps);
 	}
 }
 
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 3fb07f437793..59c12cb1db5a 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
@@ -87,27 +87,20 @@ static void apply_edid_quirks(struct drm_edid_product_id *product_id, struct dc_
  *
  * 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_edid_caps *edid_caps)
 {
 	struct amdgpu_dm_connector *aconnector = link->priv;
 	struct drm_connector *connector = &aconnector->base;
 	const struct drm_edid *drm_edid = aconnector->drm_edid;
 	struct drm_edid_product_id product_id;
-	struct edid *edid_buf = edid ? (struct edid *) edid->raw_edid : NULL;
 	int sad_count;
 	int i = 0;
-
 	enum dc_edid_status result = EDID_OK;
 
-	if (!edid_caps || !edid)
+	if (!edid_caps || !drm_edid)
 		return EDID_BAD_INPUT;
 
-	if (!drm_edid_is_valid(edid_buf))
-		result = EDID_BAD_CHECKSUM;
-
 	drm_edid_get_product_id(drm_edid, &product_id);
 
 	edid_caps->manufacturer_id = le16_to_cpu(product_id.manufacturer_name);
@@ -919,19 +912,23 @@ 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()
+		/* FIXME: Get rid of drm_edid_raw()
+		 * Raw edid is still needed for dm_helpers_dp_write_dpcd()
+		 */
+		edid = drm_edid_raw(drm_edid);
 		sink->dc_edid.length = EDID_LENGTH * (edid->extensions + 1);
 		memmove(sink->dc_edid.raw_edid, (uint8_t *)edid, sink->dc_edid.length);
 
 		edid_status = dm_helpers_parse_edid_caps(
 						link,
-						&sink->dc_edid,
 						&sink->edid_caps);
 
-		/* We don't need the original edid anymore */
-		drm_edid_free(drm_edid);
-
-	} while (edid_status == EDID_BAD_CHECKSUM && --retry > 0);
+		if (edid_status != EDID_OK) {
+			/* We can discard the drm_edid and retry */
+			drm_edid_free(drm_edid);
+			drm_edid_connector_update(connector, drm_edid);
+		}
+	} while (edid_status != EDID_OK && --retry > 0);
 
 	if (edid_status != EDID_OK)
 		DRM_ERROR("EDID err: %d, on connector: %s",
diff --git a/drivers/gpu/drm/amd/display/dc/dm_helpers.h b/drivers/gpu/drm/amd/display/dc/dm_helpers.h
index 34adae7ab6e8..bcdfc46c844e 100644
--- a/drivers/gpu/drm/amd/display/dc/dm_helpers.h
+++ b/drivers/gpu/drm/amd/display/dc/dm_helpers.h
@@ -61,7 +61,6 @@ void dm_helpers_free_gpu_mem(
 
 enum dc_edid_status dm_helpers_parse_edid_caps(
 	struct dc_link *link,
-	const struct dc_edid *edid,
 	struct dc_edid_caps *edid_caps);
 
 
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 bba644024780..c7bb146636b2 100644
--- a/drivers/gpu/drm/amd/display/dc/link/link_detection.c
+++ b/drivers/gpu/drm/amd/display/dc/link/link_detection.c
@@ -1417,10 +1417,8 @@ 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->edid_caps);
 
 	/*
 	 * Treat device as no EDID device if EDID
-- 
2.43.0

Re: [PATCH v4 11/11] drm/amd/display: remove dc_edid handler from dm_helpers_parse_edid_caps

[kernel test robot]

Hi Melissa,

kernel test robot noticed the following build warnings:

[auto build test WARNING on next-20240703]
[cannot apply to amd-pstate/linux-next amd-pstate/bleeding-edge linus/master v6.10-rc6 v6.10-rc5 v6.10-rc4 v6.10-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Melissa-Wen/drm-amd-display-clean-unused-variables-for-hdmi-freesync-parser/20240706-121621
base:   next-20240703
patch link:    https://lore.kernel.org/r/20240706034004.801329-12-mwen%40igalia.com
patch subject: [PATCH v4 11/11] drm/amd/display: remove dc_edid handler from dm_helpers_parse_edid_caps
config: loongarch-allmodconfig (https://download.01.org/0day-ci/archive/20240707/202407071047.0LkXU4JN-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240707/202407071047.0LkXU4JN-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407071047.0LkXU4JN-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm_helpers.c:92: warning: Excess function parameter 'edid' description in 'dm_helpers_parse_edid_caps'


vim +92 drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm_helpers.c

613a7956deb3b1 Aurabindo Pillai     2023-06-12   79  
4855fdb5fc5488 Melissa Wen          2024-07-06   80  #define AMDGPU_ELD_DISPLAY_NAME_SIZE_IN_CHARS 13
f0b60e6e9b2ba3 Srinivasan Shanmugam 2023-07-13   81  /**
f0b60e6e9b2ba3 Srinivasan Shanmugam 2023-07-13   82   * dm_helpers_parse_edid_caps() - Parse edid caps
4562236b3bc0a2 Harry Wentland       2017-09-12   83   *
f0b60e6e9b2ba3 Srinivasan Shanmugam 2023-07-13   84   * @link: current detected link
4562236b3bc0a2 Harry Wentland       2017-09-12   85   * @edid:	[in] pointer to edid
f0b60e6e9b2ba3 Srinivasan Shanmugam 2023-07-13   86   * @edid_caps:	[in] pointer to edid caps
f0b60e6e9b2ba3 Srinivasan Shanmugam 2023-07-13   87   *
f0b60e6e9b2ba3 Srinivasan Shanmugam 2023-07-13   88   * Return: void
f0b60e6e9b2ba3 Srinivasan Shanmugam 2023-07-13   89   */
4dbd1b96ef4bca Melissa Wen          2024-07-06   90  enum dc_edid_status dm_helpers_parse_edid_caps(struct dc_link *link,
4562236b3bc0a2 Harry Wentland       2017-09-12   91  					       struct dc_edid_caps *edid_caps)
4562236b3bc0a2 Harry Wentland       2017-09-12  @92  {
3c021931023a30 Claudio Suarez       2021-10-17   93  	struct amdgpu_dm_connector *aconnector = link->priv;
3c021931023a30 Claudio Suarez       2021-10-17   94  	struct drm_connector *connector = &aconnector->base;
766cded62d4ba6 Melissa Wen          2024-07-06   95  	const struct drm_edid *drm_edid = aconnector->drm_edid;
766cded62d4ba6 Melissa Wen          2024-07-06   96  	struct drm_edid_product_id product_id;
c3e817e49503e3 Melissa Wen          2024-07-06   97  	int sad_count;
4562236b3bc0a2 Harry Wentland       2017-09-12   98  	int i = 0;
4562236b3bc0a2 Harry Wentland       2017-09-12   99  	enum dc_edid_status result = EDID_OK;
4562236b3bc0a2 Harry Wentland       2017-09-12  100  
4dbd1b96ef4bca Melissa Wen          2024-07-06  101  	if (!edid_caps || !drm_edid)
4562236b3bc0a2 Harry Wentland       2017-09-12  102  		return EDID_BAD_INPUT;
4562236b3bc0a2 Harry Wentland       2017-09-12  103  
766cded62d4ba6 Melissa Wen          2024-07-06  104  	drm_edid_get_product_id(drm_edid, &product_id);
766cded62d4ba6 Melissa Wen          2024-07-06  105  
766cded62d4ba6 Melissa Wen          2024-07-06  106  	edid_caps->manufacturer_id = le16_to_cpu(product_id.manufacturer_name);
766cded62d4ba6 Melissa Wen          2024-07-06  107  	edid_caps->product_id = le16_to_cpu(product_id.product_code);
766cded62d4ba6 Melissa Wen          2024-07-06  108  	edid_caps->serial_number = le32_to_cpu(product_id.serial_number);
766cded62d4ba6 Melissa Wen          2024-07-06  109  	edid_caps->manufacture_week = product_id.week_of_manufacture;
766cded62d4ba6 Melissa Wen          2024-07-06  110  	edid_caps->manufacture_year = product_id.year_of_manufacture;
4562236b3bc0a2 Harry Wentland       2017-09-12  111  
4855fdb5fc5488 Melissa Wen          2024-07-06  112  	memset(edid_caps->display_name, 0, AUDIO_INFO_DISPLAY_NAME_SIZE_IN_CHARS);
4855fdb5fc5488 Melissa Wen          2024-07-06  113  	memcpy(edid_caps->display_name,
4855fdb5fc5488 Melissa Wen          2024-07-06  114  	       &connector->eld[DRM_ELD_MONITOR_NAME_STRING],
4855fdb5fc5488 Melissa Wen          2024-07-06  115  	       AMDGPU_ELD_DISPLAY_NAME_SIZE_IN_CHARS);
4562236b3bc0a2 Harry Wentland       2017-09-12  116  
3c021931023a30 Claudio Suarez       2021-10-17  117  	edid_caps->edid_hdmi = connector->display_info.is_hdmi;
4562236b3bc0a2 Harry Wentland       2017-09-12  118  
766cded62d4ba6 Melissa Wen          2024-07-06  119  	apply_edid_quirks(&product_id, edid_caps);
b7cdccc6a84956 Ryan Lin             2024-02-28  120  
b13a74b7280b1a Melissa Wen          2024-07-06  121  	sad_count = drm_eld_sad_count(connector->eld);
ae2a3495973ef0 Jean Delvare         2019-09-04  122  	if (sad_count <= 0)
4562236b3bc0a2 Harry Wentland       2017-09-12  123  		return result;
4562236b3bc0a2 Harry Wentland       2017-09-12  124  
1347b15d5e8e16 Srinivasan Shanmugam 2023-08-13  125  	edid_caps->audio_mode_count = min(sad_count, DC_MAX_AUDIO_DESC_COUNT);
4562236b3bc0a2 Harry Wentland       2017-09-12  126  	for (i = 0; i < edid_caps->audio_mode_count; ++i) {
b13a74b7280b1a Melissa Wen          2024-07-06  127  		struct cea_sad sad;
4562236b3bc0a2 Harry Wentland       2017-09-12  128  
b13a74b7280b1a Melissa Wen          2024-07-06  129  		if (drm_eld_sad_get(connector->eld, i, &sad) < 0)
b13a74b7280b1a Melissa Wen          2024-07-06  130  			continue;
b13a74b7280b1a Melissa Wen          2024-07-06  131  
b13a74b7280b1a Melissa Wen          2024-07-06  132  		edid_caps->audio_modes[i].format_code = sad.format;
b13a74b7280b1a Melissa Wen          2024-07-06  133  		edid_caps->audio_modes[i].channel_count = sad.channels + 1;
b13a74b7280b1a Melissa Wen          2024-07-06  134  		edid_caps->audio_modes[i].sample_rate = sad.freq;
b13a74b7280b1a Melissa Wen          2024-07-06  135  		edid_caps->audio_modes[i].sample_size = sad.byte2;
4562236b3bc0a2 Harry Wentland       2017-09-12  136  	}
4562236b3bc0a2 Harry Wentland       2017-09-12  137  
4562236b3bc0a2 Harry Wentland       2017-09-12  138  
c3e817e49503e3 Melissa Wen          2024-07-06  139  	if (connector->eld[DRM_ELD_SPEAKER])
c3e817e49503e3 Melissa Wen          2024-07-06  140  		edid_caps->speaker_flags = connector->eld[DRM_ELD_SPEAKER];
4562236b3bc0a2 Harry Wentland       2017-09-12  141  	else
4562236b3bc0a2 Harry Wentland       2017-09-12  142  		edid_caps->speaker_flags = DEFAULT_SPEAKER_LOCATION;
4562236b3bc0a2 Harry Wentland       2017-09-12  143  
4562236b3bc0a2 Harry Wentland       2017-09-12  144  	return result;
4562236b3bc0a2 Harry Wentland       2017-09-12  145  }
4562236b3bc0a2 Harry Wentland       2017-09-12  146  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki