[RFC PATCH 0/2] drm/amd/display: switch amdgpu_dm

All of lore.kernel.org
 help / color / mirror / Atom feed

* [RFC PATCH 0/2] drm/amd/display: switch amdgpu_dm_connector to
@ 2024-01-26 16:28 Melissa Wen
  2024-01-26 16:28 ` [RFC PATCH 1/2] drm/amd/display: fix null-pointer dereference on edid reading Melissa Wen
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Melissa Wen @ 2024-01-26 16:28 UTC (permalink / raw)
  To: airlied, alexander.deucher, alex.hung, christian.koenig, daniel,
	harry.wentland, jani.nikula, Rodrigo.Siqueira, sunpeng.li,
	Xinhui.Pan
  Cc: kernel-dev, dri-devel, amd-gfx

Hi,

I'm debugging a null-pointer dereference when running
igt@kms_connector_force_edid and the way I found to solve the bug is to
stop using raw edid handler in amdgpu_connector_funcs_force and
create_eml_sink in favor of managing resouces via sruct drm_edid helpers
(Patch 1). The proper solution seems to be switch amdgpu_dm_connector
from struct edid to struct drm_edid and avoid the usage of different
approaches in the driver (Patch 2). However, doing it implies a good
amount of work and validation, therefore I decided to send this RFC
first to collect opinions and check if there is any parallel work on
this side. It's a working in progress.

The null-pointer error trigger by the igt@kms_connector_force_edid test
was introduced by:
- e54ed41620f ("drm/amd/display: Remove unwanted drm edid references")

You can check the error trace in the first patch.

This series was tested with kms_hdmi_inject and kms_force_connector. No
null-pointer error, kms_hdmi_inject is successul and kms_force_connector
is sucessful after the second execution - the force-edid subtest
still fails in the first run (I'm still investigating).

There is also a couple of cast warnings to be addressed - I'm looking
for the best replacement. 

I appreciate any feedback and testing.

Melissa

Melissa Wen (2):
  drm/amd/display: fix null-pointer dereference on edid reading
  drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 78 ++++++++++---------
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  4 +-
 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  9 ++-
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c   | 23 +++---
 4 files changed, 60 insertions(+), 54 deletions(-)

-- 
2.43.0

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [RFC PATCH 1/2] drm/amd/display: fix null-pointer dereference on edid reading
  2024-01-26 16:28 [RFC PATCH 0/2] drm/amd/display: switch amdgpu_dm_connector to Melissa Wen
@ 2024-01-26 16:28 ` Melissa Wen
  2024-01-29 14:18   ` kernel test robot
  2024-01-26 16:28 ` [RFC PATCH 2/2] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid Melissa Wen
  2024-01-26 18:22 ` [RFC PATCH 0/2] drm/amd/display: switch amdgpu_dm_connector to Mario Limonciello
  2 siblings, 1 reply; 11+ messages in thread
From: Melissa Wen @ 2024-01-26 16:28 UTC (permalink / raw)
  To: harry.wentland, sunpeng.li, Rodrigo.Siqueira, alexander.deucher,
	christian.koenig, Xinhui.Pan, airlied, daniel, jani.nikula,
	alex.hung
  Cc: kernel-dev, dri-devel, amd-gfx

Use drm_edid helpers to fix a null-pointer derefence that happens when
running igt@kms_force_connector_basic in a system with DCN2.1 and HDMI
connector detected as below:

[  +0.178146] BUG: kernel NULL pointer dereference, address: 00000000000004c0
[  +0.000010] #PF: supervisor read access in kernel mode
[  +0.000005] #PF: error_code(0x0000) - not-present page
[  +0.000004] PGD 0 P4D 0
[  +0.000006] Oops: 0000 [#1] PREEMPT SMP NOPTI
[  +0.000006] CPU: 15 PID: 2368 Comm: kms_force_conne Not tainted 6.5.0-asdn+ #152
[  +0.000005] Hardware name: HP HP ENVY x360 Convertible 13-ay1xxx/8929, BIOS F.01 07/14/2021
[  +0.000004] RIP: 0010:i2c_transfer+0xd/0x100
[  +0.000011] Code: ea fc ff ff 66 0f 1f 84 00 00 00 00 00 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 f3 0f 1e fa 0f 1f 44 00 00 41 54 55 53 <48> 8b 47 10 48 89 fb 48 83 38 00 0f 84 b3 00 00 00 83 3d 2f 80 16
[  +0.000004] RSP: 0018:ffff9c4f89c0fad0 EFLAGS: 00010246
[  +0.000005] RAX: 0000000000000000 RBX: 0000000000000005 RCX: 0000000000000080
[  +0.000003] RDX: 0000000000000002 RSI: ffff9c4f89c0fb20 RDI: 00000000000004b0
[  +0.000003] RBP: ffff9c4f89c0fb80 R08: 0000000000000080 R09: ffff8d8e0b15b980
[  +0.000003] R10: 00000000000380e0 R11: 0000000000000000 R12: 0000000000000080
[  +0.000002] R13: 0000000000000002 R14: ffff9c4f89c0fb0e R15: ffff9c4f89c0fb0f
[  +0.000004] FS:  00007f9ad2176c40(0000) GS:ffff8d90fe9c0000(0000) knlGS:0000000000000000
[  +0.000003] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  +0.000004] CR2: 00000000000004c0 CR3: 0000000121bc4000 CR4: 0000000000750ee0
[  +0.000003] PKRU: 55555554
[  +0.000003] Call Trace:
[  +0.000006]  <TASK>
[  +0.000006]  ? __die+0x23/0x70
[  +0.000011]  ? page_fault_oops+0x17d/0x4c0
[  +0.000008]  ? preempt_count_add+0x6e/0xa0
[  +0.000008]  ? srso_alias_return_thunk+0x5/0x7f
[  +0.000011]  ? exc_page_fault+0x7f/0x180
[  +0.000009]  ? asm_exc_page_fault+0x26/0x30
[  +0.000013]  ? i2c_transfer+0xd/0x100
[  +0.000010]  drm_do_probe_ddc_edid+0xc2/0x140 [drm]
[  +0.000067]  ? srso_alias_return_thunk+0x5/0x7f
[  +0.000006]  ? _drm_do_get_edid+0x97/0x3c0 [drm]
[  +0.000043]  ? __pfx_drm_do_probe_ddc_edid+0x10/0x10 [drm]
[  +0.000042]  edid_block_read+0x3b/0xd0 [drm]
[  +0.000043]  _drm_do_get_edid+0xb6/0x3c0 [drm]
[  +0.000041]  ? __pfx_drm_do_probe_ddc_edid+0x10/0x10 [drm]
[  +0.000043]  drm_edid_read_custom+0x37/0xd0 [drm]
[  +0.000044]  amdgpu_dm_connector_mode_valid+0x129/0x1d0 [amdgpu]
[  +0.000153]  drm_connector_mode_valid+0x3b/0x60 [drm_kms_helper]
[  +0.000000]  __drm_helper_update_and_validate+0xfe/0x3c0 [drm_kms_helper]
[  +0.000000]  ? amdgpu_dm_connector_get_modes+0xb6/0x520 [amdgpu]
[  +0.000000]  ? srso_alias_return_thunk+0x5/0x7f
[  +0.000000]  drm_helper_probe_single_connector_modes+0x2ab/0x540 [drm_kms_helper]
[  +0.000000]  status_store+0xb2/0x1f0 [drm]
[  +0.000000]  kernfs_fop_write_iter+0x136/0x1d0
[  +0.000000]  vfs_write+0x24d/0x440
[  +0.000000]  ksys_write+0x6f/0xf0
[  +0.000000]  do_syscall_64+0x60/0xc0
[  +0.000000]  ? srso_alias_return_thunk+0x5/0x7f
[  +0.000000]  ? syscall_exit_to_user_mode+0x2b/0x40
[  +0.000000]  ? srso_alias_return_thunk+0x5/0x7f
[  +0.000000]  ? do_syscall_64+0x6c/0xc0
[  +0.000000]  ? do_syscall_64+0x6c/0xc0
[  +0.000000]  entry_SYSCALL_64_after_hwframe+0x6e/0xd8
[  +0.000000] RIP: 0033:0x7f9ad46b4b00
[  +0.000000] Code: 40 00 48 8b 15 19 b3 0d 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 00 80 3d e1 3a 0e 00 00 74 17 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 58 c3 0f 1f 80 00 00 00 00 48 83 ec 28 48 89
[  +0.000000] RSP: 002b:00007ffcbd3bd6d8 EFLAGS: 00000202 ORIG_RAX: 0000000000000001
[  +0.000000] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f9ad46b4b00
[  +0.000000] RDX: 0000000000000002 RSI: 00007f9ad48a7417 RDI: 0000000000000009
[  +0.000000] RBP: 0000000000000002 R08: 0000000000000064 R09: 0000000000000000
[  +0.000000] R10: 0000000000000000 R11: 0000000000000202 R12: 00007f9ad48a7417
[  +0.000000] R13: 0000000000000009 R14: 00007ffcbd3bd760 R15: 0000000000000001
[  +0.000000]  </TASK>
[  +0.000000] Modules linked in: ctr ccm rfcomm snd_seq_dummy snd_hrtimer snd_seq snd_seq_device cmac algif_hash algif_skcipher af_alg bnep btusb btrtl btbcm btintel btmtk bluetooth uvcvideo videobuf2_vmalloc sha3_generic videobuf2_memops uvc jitterentropy_rng videobuf2_v4l2 videodev drbg videobuf2_common ansi_cprng mc ecdh_generic ecc qrtr binfmt_misc hid_sensor_accel_3d hid_sensor_magn_3d hid_sensor_gyro_3d hid_sensor_trigger industrialio_triggered_buffer kfifo_buf industrialio snd_ctl_led joydev hid_sensor_iio_common rtw89_8852ae rtw89_8852a rtw89_pci snd_hda_codec_realtek rtw89_core snd_hda_codec_generic intel_rapl_msr ledtrig_audio intel_rapl_common snd_hda_codec_hdmi mac80211 snd_hda_intel snd_intel_dspcfg kvm_amd snd_hda_codec snd_soc_dmic snd_acp3x_rn snd_acp3x_pdm_dma libarc4 snd_hwdep snd_soc_core kvm snd_hda_core cfg80211 snd_pci_acp6x snd_pcm nls_ascii snd_timer hp_wmi snd_pci_acp5x nls_cp437 snd_rn_pci_acp3x ucsi_acpi sparse_keymap ccp snd platform_profile snd_acp_config typec_ucsi irqbypass vfat sp5100_tco
[  +0.000000]  snd_soc_acpi fat rapl pcspkr wmi_bmof roles rfkill rng_core snd_pci_acp3x soundcore k10temp watchdog typec battery ac amd_pmc acpi_tad button hid_sensor_hub hid_multitouch evdev serio_raw msr parport_pc ppdev lp parport fuse loop efi_pstore configfs ip_tables x_tables autofs4 ext4 crc16 mbcache jbd2 btrfs blake2b_generic dm_crypt dm_mod efivarfs raid10 raid456 async_raid6_recov async_memcpy async_pq async_xor async_tx libcrc32c crc32c_generic xor raid6_pq raid1 raid0 multipath linear md_mod amdgpu amdxcp i2c_algo_bit drm_ttm_helper ttm crc32_pclmul crc32c_intel drm_exec gpu_sched drm_suballoc_helper nvme ghash_clmulni_intel drm_buddy drm_display_helper sha512_ssse3 nvme_core ahci xhci_pci sha512_generic hid_generic xhci_hcd libahci rtsx_pci_sdmmc t10_pi i2c_hid_acpi drm_kms_helper i2c_hid mmc_core libata aesni_intel crc64_rocksoft_generic crypto_simd amd_sfh crc64_rocksoft scsi_mod usbcore cryptd crc_t10dif cec drm crct10dif_generic hid rtsx_pci crct10dif_pclmul scsi_common rc_core crc64 i2c_piix4
[  +0.000000]  usb_common crct10dif_common video wmi
[  +0.000000] CR2: 00000000000004c0
[  +0.000000] ---[ end trace 0000000000000000 ]---

Fixes: e54ed41620f ("drm/amd/display: Remove unwanted drm edid references")
Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 23 +++++++++++--------
 1 file changed, 13 insertions(+), 10 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 cd98b3565178..68e71e2ea472 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6561,10 +6561,10 @@ amdgpu_dm_connector_late_register(struct drm_connector *connector)
 static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector)
 {
 	struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
-	struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);
 	struct dc_link *dc_link = aconnector->dc_link;
 	struct dc_sink *dc_em_sink = aconnector->dc_em_sink;
-	struct edid *edid;
+	const struct drm_edid *drm_edid;
+	const struct edid *edid;
 
 	/*
 	 * Note: drm_get_edid gets edid in the following order:
@@ -6572,18 +6572,19 @@ 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, &amdgpu_connector->ddc_bus->aux.ddc);
-	if (!edid) {
+	drm_edid = drm_edid_read(connector);
+
+	if (!drm_edid) {
 		DRM_ERROR("No EDID found on connector: %s.\n", connector->name);
 		return;
 	}
-
+	edid = drm_edid_raw(drm_edid);
 	aconnector->edid = edid;
 
 	/* Update emulated (virtual) sink's EDID */
 	if (dc_em_sink && dc_link) {
 		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,
@@ -6613,12 +6614,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 amdgpu_connector *amdgpu_connector = to_amdgpu_connector(&aconnector->base);
 	struct dc_sink_init_data init_params = {
 			.link = aconnector->dc_link,
 			.sink_signal = SIGNAL_TYPE_VIRTUAL
 	};
-	struct edid *edid;
+	const struct drm_edid *drm_edid;
+	const struct edid *edid;
 
 	/*
 	 * Note: drm_get_edid gets edid in the following order:
@@ -6626,12 +6627,14 @@ 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, &amdgpu_connector->ddc_bus->aux.ddc);
-	if (!edid) {
+	drm_edid = drm_edid_read(connector);
+	if (!drm_edid) {
 		DRM_ERROR("No EDID found on connector: %s.\n", connector->name);
 		return;
 	}
 
+	edid = drm_edid_raw(drm_edid);
+
 	if (drm_detect_hdmi_monitor(edid))
 		init_params.sink_signal = SIGNAL_TYPE_HDMI_TYPE_A;
 
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [RFC PATCH 2/2] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid
  2024-01-26 16:28 [RFC PATCH 0/2] drm/amd/display: switch amdgpu_dm_connector to Melissa Wen
  2024-01-26 16:28 ` [RFC PATCH 1/2] drm/amd/display: fix null-pointer dereference on edid reading Melissa Wen
@ 2024-01-26 16:28 ` Melissa Wen
  2024-01-26 19:33   ` Alex Hung
                     ` (2 more replies)
  2024-01-26 18:22 ` [RFC PATCH 0/2] drm/amd/display: switch amdgpu_dm_connector to Mario Limonciello
  2 siblings, 3 replies; 11+ messages in thread
From: Melissa Wen @ 2024-01-26 16:28 UTC (permalink / raw)
  To: harry.wentland, sunpeng.li, Rodrigo.Siqueira, alexander.deucher,
	christian.koenig, Xinhui.Pan, airlied, daniel
  Cc: Alex Hung, kernel-dev, dri-devel, amd-gfx

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. There are a couple of cast warnings and it was only
tested with IGT.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 63 ++++++++++---------
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  4 +-
 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  9 +--
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c   | 23 +++----
 4 files changed, 51 insertions(+), 48 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 68e71e2ea472..741081d73bb3 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3277,12 +3277,12 @@ void amdgpu_dm_update_connector_after_detect(
 					&aconnector->dm_dp_aux.aux);
 			}
 		} else {
-			aconnector->edid =
-				(struct edid *)sink->dc_edid.raw_edid;
+			const struct edid *edid = (const struct edid *)sink->dc_edid.raw_edid;
+			aconnector->edid = drm_edid_alloc(edid, (edid->extensions + 1) * EDID_LENGTH);
 
 			if (aconnector->dc_link->aux_mode)
 				drm_dp_cec_set_edid(&aconnector->dm_dp_aux.aux,
-						    aconnector->edid);
+						    drm_edid_raw(aconnector->edid));
 		}
 
 		if (!aconnector->timing_requested) {
@@ -3293,13 +3293,13 @@ void amdgpu_dm_update_connector_after_detect(
 					"failed to create aconnector->requested_timing\n");
 		}
 
-		drm_connector_update_edid_property(connector, aconnector->edid);
+		drm_edid_connector_update(connector, aconnector->edid);
 		amdgpu_dm_update_freesync_caps(connector, aconnector->edid);
 		update_connector_ext_caps(aconnector);
 	} else {
 		drm_dp_cec_unset_edid(&aconnector->dm_dp_aux.aux);
 		amdgpu_dm_update_freesync_caps(connector, NULL);
-		drm_connector_update_edid_property(connector, NULL);
+		drm_edid_connector_update(connector, NULL);
 		aconnector->num_modes = 0;
 		dc_sink_release(aconnector->dc_sink);
 		aconnector->dc_sink = NULL;
@@ -6564,7 +6564,6 @@ static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector)
 	struct dc_link *dc_link = aconnector->dc_link;
 	struct dc_sink *dc_em_sink = aconnector->dc_em_sink;
 	const struct drm_edid *drm_edid;
-	const struct edid *edid;
 
 	/*
 	 * Note: drm_get_edid gets edid in the following order:
@@ -6578,11 +6577,12 @@ static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector)
 		DRM_ERROR("No EDID found on connector: %s.\n", connector->name);
 		return;
 	}
-	edid = drm_edid_raw(drm_edid);
-	aconnector->edid = edid;
-
+	aconnector->edid = drm_edid;
+	drm_edid_connector_update(connector, drm_edid);
 	/* Update emulated (virtual) sink's EDID */
 	if (dc_em_sink && dc_link) {
+		const struct edid *edid = drm_edid_raw(drm_edid);
+
 		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(
@@ -6633,13 +6633,13 @@ static void create_eml_sink(struct amdgpu_dm_connector *aconnector)
 		return;
 	}
 
-	edid = drm_edid_raw(drm_edid);
-
-	if (drm_detect_hdmi_monitor(edid))
+	if (connector->display_info.is_hdmi)
 		init_params.sink_signal = SIGNAL_TYPE_HDMI_TYPE_A;
 
-	aconnector->edid = edid;
+	aconnector->edid = drm_edid;
+	drm_edid_connector_update(connector, drm_edid);
 
+	edid = drm_edid_raw(drm_edid);
 	aconnector->dc_em_sink = dc_link_add_remote_sink(
 		aconnector->dc_link,
 		(uint8_t *)edid,
@@ -7322,16 +7322,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
@@ -7345,10 +7345,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;
 	}
@@ -7444,12 +7444,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 (!edid)
+	if (!drm_edid)
 		return;
 
 	if (amdgpu_dm_connector->max_vfreq - amdgpu_dm_connector->min_vfreq > 10)
@@ -7462,23 +7462,23 @@ static int amdgpu_dm_connector_get_modes(struct drm_connector *connector)
 	struct amdgpu_dm_connector *amdgpu_dm_connector =
 			to_amdgpu_dm_connector(connector);
 	struct drm_encoder *encoder;
-	struct edid *edid = amdgpu_dm_connector->edid;
+	const struct drm_edid *drm_edid = amdgpu_dm_connector->edid;
 	struct dc_link_settings *verified_link_cap =
 			&amdgpu_dm_connector->dc_link->verified_link_cap;
 	const struct dc *dc = amdgpu_dm_connector->dc_link->dc;
 
 	encoder = amdgpu_dm_connector_to_encoder(connector);
 
-	if (!drm_edid_is_valid(edid)) {
+	if (!drm_edid_valid(drm_edid)) {
 		amdgpu_dm_connector->num_modes =
 				drm_add_modes_noedid(connector, 640, 480);
 		if (dc->link_srv->dp_get_encoding_format(verified_link_cap) == DP_128b_132b_ENCODING)
 			amdgpu_dm_connector->num_modes +=
 				drm_add_modes_noedid(connector, 1920, 1080);
 	} else {
-		amdgpu_dm_connector_ddc_get_modes(connector, edid);
+		amdgpu_dm_connector_ddc_get_modes(connector, drm_edid);
 		amdgpu_dm_connector_add_common_modes(encoder, connector);
-		amdgpu_dm_connector_add_freesync_modes(connector, edid);
+		amdgpu_dm_connector_add_freesync_modes(connector, drm_edid);
 	}
 	amdgpu_dm_fbc_init(connector);
 
@@ -11038,7 +11038,7 @@ static bool parse_edid_cea(struct amdgpu_dm_connector *aconnector,
 }
 
 static int parse_amd_vsdb(struct amdgpu_dm_connector *aconnector,
-			  struct edid *edid, struct amdgpu_hdmi_vsdb_info *vsdb_info)
+			  const struct edid *edid, struct amdgpu_hdmi_vsdb_info *vsdb_info)
 {
 	u8 *edid_ext = NULL;
 	int i;
@@ -11073,7 +11073,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;
@@ -11115,7 +11116,7 @@ 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;
@@ -11125,9 +11126,9 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *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 = drm_edid_raw(drm_edid);
 	bool freesync_capable = false;
 	enum adaptive_sync_type as_type = ADAPTIVE_SYNC_TYPE_NONE;
 
@@ -11140,7 +11141,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;
@@ -11162,7 +11163,7 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
 		|| sink->sink_signal == SIGNAL_TYPE_EDP) {
 		bool edid_check_required = false;
 
-		if (edid) {
+		if (drm_edid) {
 			edid_check_required = is_dp_capable_without_timing_msa(
 						adev->dm.dc,
 						amdgpu_dm_connector);
@@ -11214,7 +11215,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) {
 			timing  = &edid->detailed_timings[i];
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 9c1871b866cc..b81cf6f3713b 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -637,7 +637,7 @@ struct amdgpu_dm_connector {
 
 	/* we need to mind the EDID between detect
 	   and get modes due to analog/digital/tvencoder */
-	struct edid *edid;
+	const struct drm_edid *edid;
 
 	/* shared with amdgpu */
 	struct amdgpu_hpd hpd;
@@ -908,7 +908,7 @@ void dm_restore_drm_connector_state(struct drm_device *dev,
 				    struct drm_connector *connector);
 
 void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
-					struct edid *edid);
+				    const struct drm_edid *drm_edid);
 
 void amdgpu_dm_trigger_timing_sync(struct drm_device *dev);
 
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index e3915c4f8566..91006326ce6d 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
@@ -898,7 +898,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,8 +910,8 @@ enum dc_edid_status dm_helpers_read_local_edid(
 	 * do check sum and retry to make sure read correct edid.
 	 */
 	do {
-
-		edid = drm_get_edid(&aconnector->base, ddc);
+		drm_edid = drm_edid_read_ddc(connector, ddc);
+		edid = drm_edid_raw(drm_edid);
 
 		/* DP Compliance Test 4.2.2.6 */
 		if (link->aux_mode && connector->edid_corrupt)
@@ -928,7 +929,7 @@ enum dc_edid_status dm_helpers_read_local_edid(
 		memmove(sink->dc_edid.raw_edid, (uint8_t *)edid, sink->dc_edid.length);
 
 		/* We don't need the original edid anymore */
-		kfree(edid);
+		drm_edid_free(drm_edid);
 
 		edid_status = dm_helpers_parse_edid_caps(
 						link,
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index 7075a85dd16e..cdebd0e74b26 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
@@ -127,7 +127,7 @@ dm_dp_mst_connector_destroy(struct drm_connector *connector)
 		dc_sink_release(aconnector->dc_sink);
 	}
 
-	kfree(aconnector->edid);
+	drm_edid_free(aconnector->edid);
 
 	drm_connector_cleanup(connector);
 	drm_dp_mst_put_port_malloc(aconnector->mst_output_port);
@@ -297,15 +297,15 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
 		return drm_add_edid_modes(connector, NULL);
 
 	if (!aconnector->edid) {
-		struct edid *edid;
+		const struct drm_edid *drm_edid;
 
-		edid = drm_dp_mst_get_edid(connector, &aconnector->mst_root->mst_mgr, aconnector->mst_output_port);
+		drm_edid = drm_dp_mst_edid_read(connector, &aconnector->mst_root->mst_mgr, aconnector->mst_output_port);
 
-		if (!edid) {
+		if (!drm_edid) {
 			amdgpu_dm_set_mst_status(&aconnector->mst_status,
 			MST_REMOTE_EDID, false);
 
-			drm_connector_update_edid_property(
+			drm_edid_connector_update(
 				&aconnector->base,
 				NULL);
 
@@ -339,7 +339,7 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
 			return ret;
 		}
 
-		aconnector->edid = edid;
+		aconnector->edid = drm_edid;
 		amdgpu_dm_set_mst_status(&aconnector->mst_status,
 			MST_REMOTE_EDID, true);
 	}
@@ -354,10 +354,12 @@ 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 = drm_edid_raw(aconnector->edid);
+
 		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) {
@@ -411,10 +413,9 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
 		}
 	}
 
-	drm_connector_update_edid_property(
-					&aconnector->base, aconnector->edid);
+	drm_edid_connector_update(&aconnector->base, aconnector->edid);
 
-	ret = drm_add_edid_modes(connector, aconnector->edid);
+	ret = drm_edid_connector_add_modes(connector);
 
 	return ret;
 }
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [RFC PATCH 0/2] drm/amd/display: switch amdgpu_dm_connector to
  2024-01-26 16:28 [RFC PATCH 0/2] drm/amd/display: switch amdgpu_dm_connector to Melissa Wen
  2024-01-26 16:28 ` [RFC PATCH 1/2] drm/amd/display: fix null-pointer dereference on edid reading Melissa Wen
  2024-01-26 16:28 ` [RFC PATCH 2/2] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid Melissa Wen
@ 2024-01-26 18:22 ` Mario Limonciello
  2024-01-29  9:30   ` Jani Nikula
  2 siblings, 1 reply; 11+ messages in thread
From: Mario Limonciello @ 2024-01-26 18:22 UTC (permalink / raw)
  To: Melissa Wen, airlied, alexander.deucher, alex.hung,
	christian.koenig, daniel, harry.wentland, jani.nikula,
	Rodrigo.Siqueira, sunpeng.li, Xinhui.Pan
  Cc: amd-gfx, dri-devel, kernel-dev

On 1/26/2024 10:28, Melissa Wen wrote:
> Hi,
> 
> I'm debugging a null-pointer dereference when running
> igt@kms_connector_force_edid and the way I found to solve the bug is to
> stop using raw edid handler in amdgpu_connector_funcs_force and
> create_eml_sink in favor of managing resouces via sruct drm_edid helpers
> (Patch 1). The proper solution seems to be switch amdgpu_dm_connector
> from struct edid to struct drm_edid and avoid the usage of different
> approaches in the driver (Patch 2). However, doing it implies a good
> amount of work and validation, therefore I decided to send this RFC
> first to collect opinions and check if there is any parallel work on
> this side. It's a working in progress.
> 
> The null-pointer error trigger by the igt@kms_connector_force_edid test
> was introduced by:
> - e54ed41620f ("drm/amd/display: Remove unwanted drm edid references")
> 
> You can check the error trace in the first patch.
> 
> This series was tested with kms_hdmi_inject and kms_force_connector. No
> null-pointer error, kms_hdmi_inject is successul and kms_force_connector
> is sucessful after the second execution - the force-edid subtest
> still fails in the first run (I'm still investigating).
> 
> There is also a couple of cast warnings to be addressed - I'm looking
> for the best replacement.
> 
> I appreciate any feedback and testing.

So I'm actually a little bit worried by hardcoding EDID_LENGTH in this 
series.

I have some other patches that I'm posting later on that let you get the 
EDID from _DDC BIOS method too.  My observation was that the EDID can be 
anywhere up to 512 bytes according to the ACPI spec.

An earlier version of my patch was using EDID_LENGTH when fetching it 
and the EDID checksum failed.

I'll CC you on the post, we probably want to get your changes and mine 
merged together.

> 
> Melissa
> 
> Melissa Wen (2):
>    drm/amd/display: fix null-pointer dereference on edid reading
>    drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid
> 
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 78 ++++++++++---------
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  4 +-
>   .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  9 ++-
>   .../display/amdgpu_dm/amdgpu_dm_mst_types.c   | 23 +++---
>   4 files changed, 60 insertions(+), 54 deletions(-)
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC PATCH 2/2] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid
  2024-01-26 16:28 ` [RFC PATCH 2/2] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid Melissa Wen
@ 2024-01-26 19:33   ` Alex Hung
  2024-02-05 14:33     ` Melissa Wen
  2024-01-29  9:54   ` kernel test robot
  2024-01-29 23:29   ` kernel test robot
  2 siblings, 1 reply; 11+ messages in thread
From: Alex Hung @ 2024-01-26 19:33 UTC (permalink / raw)
  To: Melissa Wen, harry.wentland, sunpeng.li, Rodrigo.Siqueira,
	alexander.deucher, christian.koenig, Xinhui.Pan, airlied, daniel
  Cc: kernel-dev, dri-devel, amd-gfx



On 2024-01-26 09:28, 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. There are a couple of cast warnings and it was only
> tested with IGT.
> 
> Signed-off-by: Melissa Wen <mwen@igalia.com>
> ---
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 63 ++++++++++---------
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  4 +-
>   .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  9 +--
>   .../display/amdgpu_dm/amdgpu_dm_mst_types.c   | 23 +++----
>   4 files changed, 51 insertions(+), 48 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 68e71e2ea472..741081d73bb3 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3277,12 +3277,12 @@ void amdgpu_dm_update_connector_after_detect(
>   					&aconnector->dm_dp_aux.aux);
>   			}
>   		} else {
> -			aconnector->edid =
> -				(struct edid *)sink->dc_edid.raw_edid;
> +			const struct edid *edid = (const struct edid *)sink->dc_edid.raw_edid;
> +			aconnector->edid = drm_edid_alloc(edid, (edid->extensions + 1) * EDID_LENGTH);
>   
>   			if (aconnector->dc_link->aux_mode)
>   				drm_dp_cec_set_edid(&aconnector->dm_dp_aux.aux,
> -						    aconnector->edid);
> +						    drm_edid_raw(aconnector->edid));
>   		}
>   
>   		if (!aconnector->timing_requested) {
> @@ -3293,13 +3293,13 @@ void amdgpu_dm_update_connector_after_detect(
>   					"failed to create aconnector->requested_timing\n");
>   		}
>   
> -		drm_connector_update_edid_property(connector, aconnector->edid);
> +		drm_edid_connector_update(connector, aconnector->edid);
>   		amdgpu_dm_update_freesync_caps(connector, aconnector->edid);
>   		update_connector_ext_caps(aconnector);
>   	} else {
>   		drm_dp_cec_unset_edid(&aconnector->dm_dp_aux.aux);
>   		amdgpu_dm_update_freesync_caps(connector, NULL);
> -		drm_connector_update_edid_property(connector, NULL);
> +		drm_edid_connector_update(connector, NULL);
>   		aconnector->num_modes = 0;
>   		dc_sink_release(aconnector->dc_sink);
>   		aconnector->dc_sink = NULL;
> @@ -6564,7 +6564,6 @@ static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector)
>   	struct dc_link *dc_link = aconnector->dc_link;
>   	struct dc_sink *dc_em_sink = aconnector->dc_em_sink;
>   	const struct drm_edid *drm_edid;
> -	const struct edid *edid;
>   
>   	/*
>   	 * Note: drm_get_edid gets edid in the following order:
> @@ -6578,11 +6577,12 @@ static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector)
>   		DRM_ERROR("No EDID found on connector: %s.\n", connector->name);
>   		return;
>   	}
> -	edid = drm_edid_raw(drm_edid);
> -	aconnector->edid = edid;
> -
> +	aconnector->edid = drm_edid;
> +	drm_edid_connector_update(connector, drm_edid);
>   	/* Update emulated (virtual) sink's EDID */
>   	if (dc_em_sink && dc_link) {
> +		const struct edid *edid = drm_edid_raw(drm_edid);
> +
>   		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(
> @@ -6633,13 +6633,13 @@ static void create_eml_sink(struct amdgpu_dm_connector *aconnector)
>   		return;
>   	}
>   
> -	edid = drm_edid_raw(drm_edid);
> -
> -	if (drm_detect_hdmi_monitor(edid))
> +	if (connector->display_info.is_hdmi)
>   		init_params.sink_signal = SIGNAL_TYPE_HDMI_TYPE_A;
>   
> -	aconnector->edid = edid;
> +	aconnector->edid = drm_edid;
> +	drm_edid_connector_update(connector, drm_edid);
>   
> +	edid = drm_edid_raw(drm_edid);
>   	aconnector->dc_em_sink = dc_link_add_remote_sink(
>   		aconnector->dc_link,
>   		(uint8_t *)edid,
> @@ -7322,16 +7322,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
> @@ -7345,10 +7345,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;
>   	}
> @@ -7444,12 +7444,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 (!edid)
> +	if (!drm_edid)
>   		return;
>   
>   	if (amdgpu_dm_connector->max_vfreq - amdgpu_dm_connector->min_vfreq > 10)
> @@ -7462,23 +7462,23 @@ static int amdgpu_dm_connector_get_modes(struct drm_connector *connector)
>   	struct amdgpu_dm_connector *amdgpu_dm_connector =
>   			to_amdgpu_dm_connector(connector);
>   	struct drm_encoder *encoder;
> -	struct edid *edid = amdgpu_dm_connector->edid;
> +	const struct drm_edid *drm_edid = amdgpu_dm_connector->edid;
>   	struct dc_link_settings *verified_link_cap =
>   			&amdgpu_dm_connector->dc_link->verified_link_cap;
>   	const struct dc *dc = amdgpu_dm_connector->dc_link->dc;
>   
>   	encoder = amdgpu_dm_connector_to_encoder(connector);
>   
> -	if (!drm_edid_is_valid(edid)) {
> +	if (!drm_edid_valid(drm_edid)) {
>   		amdgpu_dm_connector->num_modes =
>   				drm_add_modes_noedid(connector, 640, 480);
>   		if (dc->link_srv->dp_get_encoding_format(verified_link_cap) == DP_128b_132b_ENCODING)
>   			amdgpu_dm_connector->num_modes +=
>   				drm_add_modes_noedid(connector, 1920, 1080);
>   	} else {
> -		amdgpu_dm_connector_ddc_get_modes(connector, edid);
> +		amdgpu_dm_connector_ddc_get_modes(connector, drm_edid);
>   		amdgpu_dm_connector_add_common_modes(encoder, connector);
> -		amdgpu_dm_connector_add_freesync_modes(connector, edid);
> +		amdgpu_dm_connector_add_freesync_modes(connector, drm_edid);
>   	}
>   	amdgpu_dm_fbc_init(connector);
>   
> @@ -11038,7 +11038,7 @@ static bool parse_edid_cea(struct amdgpu_dm_connector *aconnector,
>   }
>   
>   static int parse_amd_vsdb(struct amdgpu_dm_connector *aconnector,
> -			  struct edid *edid, struct amdgpu_hdmi_vsdb_info *vsdb_info)
> +			  const struct edid *edid, struct amdgpu_hdmi_vsdb_info *vsdb_info)
>   {
>   	u8 *edid_ext = NULL;
>   	int i;
> @@ -11073,7 +11073,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;
> @@ -11115,7 +11116,7 @@ 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;
> @@ -11125,9 +11126,9 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *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 = drm_edid_raw(drm_edid);


I got below compile errors:

drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c: In function 
‘amdgpu_dm_update_freesync_caps’:
drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:11176:41: 
error: assignment discards ‘const’ qualifier from pointer target type 
[-Werror=discarded-qualifiers]
11176 |                                 timing  = 
&edid->detailed_timings[i];
       |                                         ^
drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:11221:33: 
error: assignment discards ‘const’ qualifier from pointer target type 
[-Werror=discarded-qualifiers]
11221 |                         timing  = &edid->detailed_timings[i];


and the following changes fixes it:

-       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;




>   	bool freesync_capable = false;
>   	enum adaptive_sync_type as_type = ADAPTIVE_SYNC_TYPE_NONE;
>   
> @@ -11140,7 +11141,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;
> @@ -11162,7 +11163,7 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
>   		|| sink->sink_signal == SIGNAL_TYPE_EDP) {
>   		bool edid_check_required = false;
>   
> -		if (edid) {
> +		if (drm_edid) {
>   			edid_check_required = is_dp_capable_without_timing_msa(
>   						adev->dm.dc,
>   						amdgpu_dm_connector);
> @@ -11214,7 +11215,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) {
>   			timing  = &edid->detailed_timings[i];
> 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 9c1871b866cc..b81cf6f3713b 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -637,7 +637,7 @@ struct amdgpu_dm_connector {
>   
>   	/* we need to mind the EDID between detect
>   	   and get modes due to analog/digital/tvencoder */
> -	struct edid *edid;
> +	const struct drm_edid *edid;
>   
>   	/* shared with amdgpu */
>   	struct amdgpu_hpd hpd;
> @@ -908,7 +908,7 @@ void dm_restore_drm_connector_state(struct drm_device *dev,
>   				    struct drm_connector *connector);
>   
>   void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
> -					struct edid *edid);
> +				    const struct drm_edid *drm_edid);
>   
>   void amdgpu_dm_trigger_timing_sync(struct drm_device *dev);
>   
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> index e3915c4f8566..91006326ce6d 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
> @@ -898,7 +898,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,8 +910,8 @@ enum dc_edid_status dm_helpers_read_local_edid(
>   	 * do check sum and retry to make sure read correct edid.
>   	 */
>   	do {
> -
> -		edid = drm_get_edid(&aconnector->base, ddc);
> +		drm_edid = drm_edid_read_ddc(connector, ddc);
> +		edid = drm_edid_raw(drm_edid);
>   
>   		/* DP Compliance Test 4.2.2.6 */
>   		if (link->aux_mode && connector->edid_corrupt)
> @@ -928,7 +929,7 @@ enum dc_edid_status dm_helpers_read_local_edid(
>   		memmove(sink->dc_edid.raw_edid, (uint8_t *)edid, sink->dc_edid.length);
>   
>   		/* We don't need the original edid anymore */
> -		kfree(edid);
> +		drm_edid_free(drm_edid);
>   
>   		edid_status = dm_helpers_parse_edid_caps(
>   						link,
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> index 7075a85dd16e..cdebd0e74b26 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
> @@ -127,7 +127,7 @@ dm_dp_mst_connector_destroy(struct drm_connector *connector)
>   		dc_sink_release(aconnector->dc_sink);
>   	}
>   
> -	kfree(aconnector->edid);
> +	drm_edid_free(aconnector->edid);
>   
>   	drm_connector_cleanup(connector);
>   	drm_dp_mst_put_port_malloc(aconnector->mst_output_port);
> @@ -297,15 +297,15 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
>   		return drm_add_edid_modes(connector, NULL);
>   
>   	if (!aconnector->edid) {
> -		struct edid *edid;
> +		const struct drm_edid *drm_edid;
>   
> -		edid = drm_dp_mst_get_edid(connector, &aconnector->mst_root->mst_mgr, aconnector->mst_output_port);
> +		drm_edid = drm_dp_mst_edid_read(connector, &aconnector->mst_root->mst_mgr, aconnector->mst_output_port);
>   
> -		if (!edid) {
> +		if (!drm_edid) {
>   			amdgpu_dm_set_mst_status(&aconnector->mst_status,
>   			MST_REMOTE_EDID, false);
>   
> -			drm_connector_update_edid_property(
> +			drm_edid_connector_update(
>   				&aconnector->base,
>   				NULL);
>   
> @@ -339,7 +339,7 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
>   			return ret;
>   		}
>   
> -		aconnector->edid = edid;
> +		aconnector->edid = drm_edid;
>   		amdgpu_dm_set_mst_status(&aconnector->mst_status,
>   			MST_REMOTE_EDID, true);
>   	}
> @@ -354,10 +354,12 @@ 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 = drm_edid_raw(aconnector->edid);
> +
>   		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) {
> @@ -411,10 +413,9 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
>   		}
>   	}
>   
> -	drm_connector_update_edid_property(
> -					&aconnector->base, aconnector->edid);
> +	drm_edid_connector_update(&aconnector->base, aconnector->edid);
>   
> -	ret = drm_add_edid_modes(connector, aconnector->edid);
> +	ret = drm_edid_connector_add_modes(connector);
>   
>   	return ret;
>   }

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC PATCH 0/2] drm/amd/display: switch amdgpu_dm_connector to
  2024-01-26 18:22 ` [RFC PATCH 0/2] drm/amd/display: switch amdgpu_dm_connector to Mario Limonciello
@ 2024-01-29  9:30   ` Jani Nikula
  2024-02-05 14:27     ` Melissa Wen
  0 siblings, 1 reply; 11+ messages in thread
From: Jani Nikula @ 2024-01-29  9:30 UTC (permalink / raw)
  To: Mario Limonciello, Melissa Wen, airlied, alexander.deucher,
	alex.hung, christian.koenig, daniel, harry.wentland,
	Rodrigo.Siqueira, sunpeng.li, Xinhui.Pan
  Cc: amd-gfx, dri-devel, kernel-dev

On Fri, 26 Jan 2024, Mario Limonciello <mario.limonciello@amd.com> wrote:
> On 1/26/2024 10:28, Melissa Wen wrote:
>> Hi,
>> 
>> I'm debugging a null-pointer dereference when running
>> igt@kms_connector_force_edid and the way I found to solve the bug is to
>> stop using raw edid handler in amdgpu_connector_funcs_force and
>> create_eml_sink in favor of managing resouces via sruct drm_edid helpers
>> (Patch 1). The proper solution seems to be switch amdgpu_dm_connector
>> from struct edid to struct drm_edid and avoid the usage of different
>> approaches in the driver (Patch 2). However, doing it implies a good
>> amount of work and validation, therefore I decided to send this RFC
>> first to collect opinions and check if there is any parallel work on
>> this side. It's a working in progress.
>> 
>> The null-pointer error trigger by the igt@kms_connector_force_edid test
>> was introduced by:
>> - e54ed41620f ("drm/amd/display: Remove unwanted drm edid references")
>> 
>> You can check the error trace in the first patch.
>> 
>> This series was tested with kms_hdmi_inject and kms_force_connector. No
>> null-pointer error, kms_hdmi_inject is successul and kms_force_connector
>> is sucessful after the second execution - the force-edid subtest
>> still fails in the first run (I'm still investigating).
>> 
>> There is also a couple of cast warnings to be addressed - I'm looking
>> for the best replacement.
>> 
>> I appreciate any feedback and testing.
>
> So I'm actually a little bit worried by hardcoding EDID_LENGTH in this 
> series.
>
> I have some other patches that I'm posting later on that let you get the 
> EDID from _DDC BIOS method too.  My observation was that the EDID can be 
> anywhere up to 512 bytes according to the ACPI spec.
>
> An earlier version of my patch was using EDID_LENGTH when fetching it 
> and the EDID checksum failed.
>
> I'll CC you on the post, we probably want to get your changes and mine 
> merged together.

One of the main points of struct drm_edid is that it tracks the
allocation size separately.

We should simply not trust edid->extensions, because most of the time it
originates from outside the kernel.

Using drm_edid and immediately drm_edid_raw() falls short. That function
should only be used during migration to help. And yeah, it also means
EDID parsing should be done in drm_edid.c, and not spread out all over
the subsystem.


BR,
Jani.


>
>> 
>> Melissa
>> 
>> Melissa Wen (2):
>>    drm/amd/display: fix null-pointer dereference on edid reading
>>    drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid
>> 
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 78 ++++++++++---------
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  4 +-
>>   .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  9 ++-
>>   .../display/amdgpu_dm/amdgpu_dm_mst_types.c   | 23 +++---
>>   4 files changed, 60 insertions(+), 54 deletions(-)
>> 
>

-- 
Jani Nikula, Intel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC PATCH 2/2] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid
  2024-01-26 16:28 ` [RFC PATCH 2/2] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid Melissa Wen
  2024-01-26 19:33   ` Alex Hung
@ 2024-01-29  9:54   ` kernel test robot
  2024-01-29 23:29   ` kernel test robot
  2 siblings, 0 replies; 11+ messages in thread
From: kernel test robot @ 2024-01-29  9:54 UTC (permalink / raw)
  To: Melissa Wen; +Cc: llvm, oe-kbuild-all

Hi Melissa,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:

[auto build test ERROR on v6.8-rc1]
[also build test ERROR on linus/master next-20240129]
[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-fix-null-pointer-dereference-on-edid-reading/20240127-010342
base:   v6.8-rc1
patch link:    https://lore.kernel.org/r/20240126163429.56714-3-mwen%40igalia.com
patch subject: [RFC PATCH 2/2] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20240129/202401291732.o3PWNUUx-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240129/202401291732.o3PWNUUx-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/202401291732.o3PWNUUx-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:11134:12: error: assigning to 'struct detailed_timing *' from 'const struct detailed_timing *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
    11134 |                                 timing  = &edid->detailed_timings[i];
          |                                         ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:11179:12: error: assigning to 'struct detailed_timing *' from 'const struct detailed_timing *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
    11179 |                         timing  = &edid->detailed_timings[i];
          |                                 ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~
   2 errors generated.


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

f9b4f20c4777bd Stylon Wang          2020-12-04  11064  
c620e79bb695b8 Rodrigo Siqueira     2022-02-21  11065  /**
c620e79bb695b8 Rodrigo Siqueira     2022-02-21  11066   * amdgpu_dm_update_freesync_caps - Update Freesync capabilities
c620e79bb695b8 Rodrigo Siqueira     2022-02-21  11067   *
41ee1f18ef5239 Alex Deucher         2022-08-30  11068   * @connector: Connector to query.
41ee1f18ef5239 Alex Deucher         2022-08-30  11069   * @edid: EDID from monitor
c620e79bb695b8 Rodrigo Siqueira     2022-02-21  11070   *
c620e79bb695b8 Rodrigo Siqueira     2022-02-21  11071   * Amdgpu supports Freesync in DP and HDMI displays, and it is required to keep
c620e79bb695b8 Rodrigo Siqueira     2022-02-21  11072   * track of some of the display information in the internal data struct used by
c620e79bb695b8 Rodrigo Siqueira     2022-02-21  11073   * amdgpu_dm. This function checks which type of connector we need to set the
c620e79bb695b8 Rodrigo Siqueira     2022-02-21  11074   * FreeSync parameters.
c620e79bb695b8 Rodrigo Siqueira     2022-02-21  11075   */
98e6436d3af5fe Anthony Koo          2018-08-21  11076  void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
e5d89dbba370d6 Melissa Wen          2024-01-26  11077  				    const struct drm_edid *drm_edid)
e7b07ceef2a650 Harry Wentland       2017-08-10  11078  {
eb0709ba077a21 Souptick Joarder     2021-02-23  11079  	int i = 0;
e7b07ceef2a650 Harry Wentland       2017-08-10  11080  	struct detailed_timing *timing;
e7b07ceef2a650 Harry Wentland       2017-08-10  11081  	struct detailed_non_pixel *data;
e7b07ceef2a650 Harry Wentland       2017-08-10  11082  	struct detailed_data_monitor_range *range;
c84dec2fe8837f Harry Wentland       2017-09-05  11083  	struct amdgpu_dm_connector *amdgpu_dm_connector =
c84dec2fe8837f Harry Wentland       2017-09-05  11084  			to_amdgpu_dm_connector(connector);
bb47de73666188 Nicholas Kazlauskas  2018-10-04  11085  	struct dm_connector_state *dm_con_state = NULL;
9ad544670514e2 Colin Ian King       2021-08-29  11086  	struct dc_sink *sink;
534eee82356c22 Srinivasan Shanmugam 2023-11-12  11087  	struct amdgpu_device *adev = drm_to_adev(connector->dev);
f9b4f20c4777bd Stylon Wang          2020-12-04  11088  	struct amdgpu_hdmi_vsdb_info vsdb_info = {0};
e5d89dbba370d6 Melissa Wen          2024-01-26  11089  	const struct edid *edid = drm_edid_raw(drm_edid);
c620e79bb695b8 Rodrigo Siqueira     2022-02-21  11090  	bool freesync_capable = false;
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11091  	enum adaptive_sync_type as_type = ADAPTIVE_SYNC_TYPE_NONE;
b830ebc910f641 Harry Wentland       2017-07-26  11092  
8218d7f1f70179 Harry Wentland       2017-10-17  11093  	if (!connector->state) {
8218d7f1f70179 Harry Wentland       2017-10-17  11094  		DRM_ERROR("%s - Connector has no state", __func__);
bb47de73666188 Nicholas Kazlauskas  2018-10-04  11095  		goto update;
8218d7f1f70179 Harry Wentland       2017-10-17  11096  	}
8218d7f1f70179 Harry Wentland       2017-10-17  11097  
9b2fdc33218933 Aurabindo Pillai     2021-08-11  11098  	sink = amdgpu_dm_connector->dc_sink ?
9b2fdc33218933 Aurabindo Pillai     2021-08-11  11099  		amdgpu_dm_connector->dc_sink :
9b2fdc33218933 Aurabindo Pillai     2021-08-11  11100  		amdgpu_dm_connector->dc_em_sink;
9b2fdc33218933 Aurabindo Pillai     2021-08-11  11101  
e5d89dbba370d6 Melissa Wen          2024-01-26  11102  	if (!drm_edid || !sink) {
98e6436d3af5fe Anthony Koo          2018-08-21  11103  		dm_con_state = to_dm_connector_state(connector->state);
98e6436d3af5fe Anthony Koo          2018-08-21  11104  
98e6436d3af5fe Anthony Koo          2018-08-21  11105  		amdgpu_dm_connector->min_vfreq = 0;
98e6436d3af5fe Anthony Koo          2018-08-21  11106  		amdgpu_dm_connector->max_vfreq = 0;
98e6436d3af5fe Anthony Koo          2018-08-21  11107  		amdgpu_dm_connector->pixel_clock_mhz = 0;
9b2fdc33218933 Aurabindo Pillai     2021-08-11  11108  		connector->display_info.monitor_range.min_vfreq = 0;
9b2fdc33218933 Aurabindo Pillai     2021-08-11  11109  		connector->display_info.monitor_range.max_vfreq = 0;
9b2fdc33218933 Aurabindo Pillai     2021-08-11  11110  		freesync_capable = false;
98e6436d3af5fe Anthony Koo          2018-08-21  11111  
bb47de73666188 Nicholas Kazlauskas  2018-10-04  11112  		goto update;
98e6436d3af5fe Anthony Koo          2018-08-21  11113  	}
98e6436d3af5fe Anthony Koo          2018-08-21  11114  
8218d7f1f70179 Harry Wentland       2017-10-17  11115  	dm_con_state = to_dm_connector_state(connector->state);
8218d7f1f70179 Harry Wentland       2017-10-17  11116  
e7b07ceef2a650 Harry Wentland       2017-08-10  11117  	if (!adev->dm.freesync_module)
bb47de73666188 Nicholas Kazlauskas  2018-10-04  11118  		goto update;
f9b4f20c4777bd Stylon Wang          2020-12-04  11119  
9b2fdc33218933 Aurabindo Pillai     2021-08-11  11120  	if (sink->sink_signal == SIGNAL_TYPE_DISPLAY_PORT
9b2fdc33218933 Aurabindo Pillai     2021-08-11  11121  		|| sink->sink_signal == SIGNAL_TYPE_EDP) {
f9b4f20c4777bd Stylon Wang          2020-12-04  11122  		bool edid_check_required = false;
f9b4f20c4777bd Stylon Wang          2020-12-04  11123  
e5d89dbba370d6 Melissa Wen          2024-01-26  11124  		if (drm_edid) {
e7b07ceef2a650 Harry Wentland       2017-08-10  11125  			edid_check_required = is_dp_capable_without_timing_msa(
e7b07ceef2a650 Harry Wentland       2017-08-10  11126  						adev->dm.dc,
c84dec2fe8837f Harry Wentland       2017-09-05  11127  						amdgpu_dm_connector);
e7b07ceef2a650 Harry Wentland       2017-08-10  11128  		}
f9b4f20c4777bd Stylon Wang          2020-12-04  11129  
e7b07ceef2a650 Harry Wentland       2017-08-10  11130  		if (edid_check_required == true && (edid->version > 1 ||
e7b07ceef2a650 Harry Wentland       2017-08-10  11131  		   (edid->version == 1 && edid->revision > 1))) {
e7b07ceef2a650 Harry Wentland       2017-08-10  11132  			for (i = 0; i < 4; i++) {
e7b07ceef2a650 Harry Wentland       2017-08-10  11133  
e7b07ceef2a650 Harry Wentland       2017-08-10 @11134  				timing	= &edid->detailed_timings[i];
e7b07ceef2a650 Harry Wentland       2017-08-10  11135  				data	= &timing->data.other_data;
e7b07ceef2a650 Harry Wentland       2017-08-10  11136  				range	= &data->data.range;
e7b07ceef2a650 Harry Wentland       2017-08-10  11137  				/*
e7b07ceef2a650 Harry Wentland       2017-08-10  11138  				 * Check if monitor has continuous frequency mode
e7b07ceef2a650 Harry Wentland       2017-08-10  11139  				 */
e7b07ceef2a650 Harry Wentland       2017-08-10  11140  				if (data->type != EDID_DETAIL_MONITOR_RANGE)
e7b07ceef2a650 Harry Wentland       2017-08-10  11141  					continue;
e7b07ceef2a650 Harry Wentland       2017-08-10  11142  				/*
e7b07ceef2a650 Harry Wentland       2017-08-10  11143  				 * Check for flag range limits only. If flag == 1 then
e7b07ceef2a650 Harry Wentland       2017-08-10  11144  				 * no additional timing information provided.
e7b07ceef2a650 Harry Wentland       2017-08-10  11145  				 * Default GTF, GTF Secondary curve and CVT are not
e7b07ceef2a650 Harry Wentland       2017-08-10  11146  				 * supported
e7b07ceef2a650 Harry Wentland       2017-08-10  11147  				 */
e7b07ceef2a650 Harry Wentland       2017-08-10  11148  				if (range->flags != 1)
e7b07ceef2a650 Harry Wentland       2017-08-10  11149  					continue;
e7b07ceef2a650 Harry Wentland       2017-08-10  11150  
c84dec2fe8837f Harry Wentland       2017-09-05  11151  				amdgpu_dm_connector->min_vfreq = range->min_vfreq;
c84dec2fe8837f Harry Wentland       2017-09-05  11152  				amdgpu_dm_connector->max_vfreq = range->max_vfreq;
c84dec2fe8837f Harry Wentland       2017-09-05  11153  				amdgpu_dm_connector->pixel_clock_mhz =
e7b07ceef2a650 Harry Wentland       2017-08-10  11154  					range->pixel_clock_mhz * 10;
a0ffc3fd67e72b Stylon Wang          2021-01-05  11155  
a0ffc3fd67e72b Stylon Wang          2021-01-05  11156  				connector->display_info.monitor_range.min_vfreq = range->min_vfreq;
a0ffc3fd67e72b Stylon Wang          2021-01-05  11157  				connector->display_info.monitor_range.max_vfreq = range->max_vfreq;
a0ffc3fd67e72b Stylon Wang          2021-01-05  11158  
e7b07ceef2a650 Harry Wentland       2017-08-10  11159  				break;
e7b07ceef2a650 Harry Wentland       2017-08-10  11160  			}
e7b07ceef2a650 Harry Wentland       2017-08-10  11161  
c84dec2fe8837f Harry Wentland       2017-09-05  11162  			if (amdgpu_dm_connector->max_vfreq -
c84dec2fe8837f Harry Wentland       2017-09-05  11163  			    amdgpu_dm_connector->min_vfreq > 10) {
98e6436d3af5fe Anthony Koo          2018-08-21  11164  
bb47de73666188 Nicholas Kazlauskas  2018-10-04  11165  				freesync_capable = true;
e7b07ceef2a650 Harry Wentland       2017-08-10  11166  			}
e7b07ceef2a650 Harry Wentland       2017-08-10  11167  		}
ec8e59cb4e0c1a Bhawanpreet Lakha    2023-06-12  11168  		parse_amd_vsdb(amdgpu_dm_connector, edid, &vsdb_info);
ec8e59cb4e0c1a Bhawanpreet Lakha    2023-06-12  11169  
ec8e59cb4e0c1a Bhawanpreet Lakha    2023-06-12  11170  		if (vsdb_info.replay_mode) {
ec8e59cb4e0c1a Bhawanpreet Lakha    2023-06-12  11171  			amdgpu_dm_connector->vsdb_info.replay_mode = vsdb_info.replay_mode;
ec8e59cb4e0c1a Bhawanpreet Lakha    2023-06-12  11172  			amdgpu_dm_connector->vsdb_info.amd_vsdb_version = vsdb_info.amd_vsdb_version;
ec8e59cb4e0c1a Bhawanpreet Lakha    2023-06-12  11173  			amdgpu_dm_connector->as_type = ADAPTIVE_SYNC_TYPE_EDP;
ec8e59cb4e0c1a Bhawanpreet Lakha    2023-06-12  11174  		}
ec8e59cb4e0c1a Bhawanpreet Lakha    2023-06-12  11175  
e5d89dbba370d6 Melissa Wen          2024-01-26  11176  	} else if (drm_edid && sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A) {
7c7dd77489540d Arnd Bergmann        2021-02-25  11177  		i = parse_hdmi_amd_vsdb(amdgpu_dm_connector, edid, &vsdb_info);
7c7dd77489540d Arnd Bergmann        2021-02-25  11178  		if (i >= 0 && vsdb_info.freesync_supported) {
f9b4f20c4777bd Stylon Wang          2020-12-04  11179  			timing  = &edid->detailed_timings[i];
f9b4f20c4777bd Stylon Wang          2020-12-04  11180  			data    = &timing->data.other_data;
f9b4f20c4777bd Stylon Wang          2020-12-04  11181  
f9b4f20c4777bd Stylon Wang          2020-12-04  11182  			amdgpu_dm_connector->min_vfreq = vsdb_info.min_refresh_rate_hz;
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11183  			amdgpu_dm_connector->max_vfreq = vsdb_info.max_refresh_rate_hz;
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11184  			if (amdgpu_dm_connector->max_vfreq - amdgpu_dm_connector->min_vfreq > 10)
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11185  				freesync_capable = true;
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11186  
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11187  			connector->display_info.monitor_range.min_vfreq = vsdb_info.min_refresh_rate_hz;
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11188  			connector->display_info.monitor_range.max_vfreq = vsdb_info.max_refresh_rate_hz;
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11189  		}
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11190  	}
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11191  
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11192  	as_type = dm_get_adaptive_sync_support_type(amdgpu_dm_connector->dc_link);
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11193  
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11194  	if (as_type == FREESYNC_TYPE_PCON_IN_WHITELIST) {
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11195  		i = parse_hdmi_amd_vsdb(amdgpu_dm_connector, edid, &vsdb_info);
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11196  		if (i >= 0 && vsdb_info.freesync_supported && vsdb_info.amd_vsdb_version > 0) {
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11197  
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11198  			amdgpu_dm_connector->pack_sdp_v1_3 = true;
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11199  			amdgpu_dm_connector->as_type = as_type;
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11200  			amdgpu_dm_connector->vsdb_info = vsdb_info;
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11201  
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11202  			amdgpu_dm_connector->min_vfreq = vsdb_info.min_refresh_rate_hz;
f9b4f20c4777bd Stylon Wang          2020-12-04  11203  			amdgpu_dm_connector->max_vfreq = vsdb_info.max_refresh_rate_hz;
f9b4f20c4777bd Stylon Wang          2020-12-04  11204  			if (amdgpu_dm_connector->max_vfreq - amdgpu_dm_connector->min_vfreq > 10)
f9b4f20c4777bd Stylon Wang          2020-12-04  11205  				freesync_capable = true;
f9b4f20c4777bd Stylon Wang          2020-12-04  11206  
f9b4f20c4777bd Stylon Wang          2020-12-04  11207  			connector->display_info.monitor_range.min_vfreq = vsdb_info.min_refresh_rate_hz;
f9b4f20c4777bd Stylon Wang          2020-12-04  11208  			connector->display_info.monitor_range.max_vfreq = vsdb_info.max_refresh_rate_hz;
f9b4f20c4777bd Stylon Wang          2020-12-04  11209  		}
f9b4f20c4777bd Stylon Wang          2020-12-04  11210  	}
bb47de73666188 Nicholas Kazlauskas  2018-10-04  11211  
bb47de73666188 Nicholas Kazlauskas  2018-10-04  11212  update:
bb47de73666188 Nicholas Kazlauskas  2018-10-04  11213  	if (dm_con_state)
bb47de73666188 Nicholas Kazlauskas  2018-10-04  11214  		dm_con_state->freesync_capable = freesync_capable;
bb47de73666188 Nicholas Kazlauskas  2018-10-04  11215  
bb47de73666188 Nicholas Kazlauskas  2018-10-04  11216  	if (connector->vrr_capable_property)
bb47de73666188 Nicholas Kazlauskas  2018-10-04  11217  		drm_connector_set_vrr_capable_property(connector,
bb47de73666188 Nicholas Kazlauskas  2018-10-04  11218  						       freesync_capable);
e7b07ceef2a650 Harry Wentland       2017-08-10  11219  }
e7b07ceef2a650 Harry Wentland       2017-08-10  11220  

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC PATCH 1/2] drm/amd/display: fix null-pointer dereference on edid reading
  2024-01-26 16:28 ` [RFC PATCH 1/2] drm/amd/display: fix null-pointer dereference on edid reading Melissa Wen
@ 2024-01-29 14:18   ` kernel test robot
  0 siblings, 0 replies; 11+ messages in thread
From: kernel test robot @ 2024-01-29 14:18 UTC (permalink / raw)
  To: Melissa Wen; +Cc: oe-kbuild-all

Hi Melissa,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:

[auto build test WARNING on v6.8-rc1]
[also build test WARNING on linus/master next-20240129]
[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-fix-null-pointer-dereference-on-edid-reading/20240127-010342
base:   v6.8-rc1
patch link:    https://lore.kernel.org/r/20240126163429.56714-2-mwen%40igalia.com
patch subject: [RFC PATCH 1/2] drm/amd/display: fix null-pointer dereference on edid reading
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20240129/202401292240.ZOQBYHFf-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240129/202401292240.ZOQBYHFf-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/202401292240.ZOQBYHFf-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c: In function 'amdgpu_dm_connector_funcs_force':
>> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:6541:26: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
    6541 |         aconnector->edid = edid;
         |                          ^
   drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c: In function 'create_eml_sink':
   drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:6600:26: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
    6600 |         aconnector->edid = edid;
         |                          ^


vim +/const +6541 drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c

14f04fa4834a51 Alex Deucher  2020-02-04  6519  
dae343b343ff74 Arnd Bergmann 2023-05-01  6520  static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector)
0ba4a784a14592 Alex Hung     2023-04-05  6521  {
0ba4a784a14592 Alex Hung     2023-04-05  6522  	struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
0ba4a784a14592 Alex Hung     2023-04-05  6523  	struct dc_link *dc_link = aconnector->dc_link;
0ba4a784a14592 Alex Hung     2023-04-05  6524  	struct dc_sink *dc_em_sink = aconnector->dc_em_sink;
e8e134507fe4c2 Melissa Wen   2024-01-26  6525  	const struct drm_edid *drm_edid;
e8e134507fe4c2 Melissa Wen   2024-01-26  6526  	const struct edid *edid;
0ba4a784a14592 Alex Hung     2023-04-05  6527  
0e859faf8670a7 Alex Hung     2023-08-25  6528  	/*
0e859faf8670a7 Alex Hung     2023-08-25  6529  	 * Note: drm_get_edid gets edid in the following order:
0e859faf8670a7 Alex Hung     2023-08-25  6530  	 * 1) override EDID if set via edid_override debugfs,
0e859faf8670a7 Alex Hung     2023-08-25  6531  	 * 2) firmware EDID if set via edid_firmware module parameter
0e859faf8670a7 Alex Hung     2023-08-25  6532  	 * 3) regular DDC read.
0e859faf8670a7 Alex Hung     2023-08-25  6533  	 */
e8e134507fe4c2 Melissa Wen   2024-01-26  6534  	drm_edid = drm_edid_read(connector);
e8e134507fe4c2 Melissa Wen   2024-01-26  6535  
e8e134507fe4c2 Melissa Wen   2024-01-26  6536  	if (!drm_edid) {
0e859faf8670a7 Alex Hung     2023-08-25  6537  		DRM_ERROR("No EDID found on connector: %s.\n", connector->name);
0ba4a784a14592 Alex Hung     2023-04-05  6538  		return;
0e859faf8670a7 Alex Hung     2023-08-25  6539  	}
e8e134507fe4c2 Melissa Wen   2024-01-26  6540  	edid = drm_edid_raw(drm_edid);
0ba4a784a14592 Alex Hung     2023-04-05 @6541  	aconnector->edid = edid;
0ba4a784a14592 Alex Hung     2023-04-05  6542  
0ba4a784a14592 Alex Hung     2023-04-05  6543  	/* Update emulated (virtual) sink's EDID */
0ba4a784a14592 Alex Hung     2023-04-05  6544  	if (dc_em_sink && dc_link) {
0ba4a784a14592 Alex Hung     2023-04-05  6545  		memset(&dc_em_sink->edid_caps, 0, sizeof(struct dc_edid_caps));
e8e134507fe4c2 Melissa Wen   2024-01-26  6546  		memmove(dc_em_sink->dc_edid.raw_edid, (uint8_t *)edid, (edid->extensions + 1) * EDID_LENGTH);
0ba4a784a14592 Alex Hung     2023-04-05  6547  		dm_helpers_parse_edid_caps(
0ba4a784a14592 Alex Hung     2023-04-05  6548  			dc_link,
0ba4a784a14592 Alex Hung     2023-04-05  6549  			&dc_em_sink->dc_edid,
0ba4a784a14592 Alex Hung     2023-04-05  6550  			&dc_em_sink->edid_caps);
0ba4a784a14592 Alex Hung     2023-04-05  6551  	}
0ba4a784a14592 Alex Hung     2023-04-05  6552  }
0ba4a784a14592 Alex Hung     2023-04-05  6553  

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC PATCH 2/2] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid
  2024-01-26 16:28 ` [RFC PATCH 2/2] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid Melissa Wen
  2024-01-26 19:33   ` Alex Hung
  2024-01-29  9:54   ` kernel test robot
@ 2024-01-29 23:29   ` kernel test robot
  2 siblings, 0 replies; 11+ messages in thread
From: kernel test robot @ 2024-01-29 23:29 UTC (permalink / raw)
  To: Melissa Wen; +Cc: oe-kbuild-all

Hi Melissa,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build warnings:

[auto build test WARNING on v6.8-rc1]
[also build test WARNING on linus/master next-20240129]
[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-fix-null-pointer-dereference-on-edid-reading/20240127-010342
base:   v6.8-rc1
patch link:    https://lore.kernel.org/r/20240126163429.56714-3-mwen%40igalia.com
patch subject: [RFC PATCH 2/2] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20240130/202401300733.nUTAQZcr-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240130/202401300733.nUTAQZcr-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/202401300733.nUTAQZcr-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:11078: warning: Function parameter or struct member 'drm_edid' not described in 'amdgpu_dm_update_freesync_caps'
>> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:11078: warning: Excess function parameter 'edid' description in 'amdgpu_dm_update_freesync_caps'


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

f9b4f20c4777bd Stylon Wang          2020-12-04  11064  
c620e79bb695b8 Rodrigo Siqueira     2022-02-21  11065  /**
c620e79bb695b8 Rodrigo Siqueira     2022-02-21  11066   * amdgpu_dm_update_freesync_caps - Update Freesync capabilities
c620e79bb695b8 Rodrigo Siqueira     2022-02-21  11067   *
41ee1f18ef5239 Alex Deucher         2022-08-30  11068   * @connector: Connector to query.
41ee1f18ef5239 Alex Deucher         2022-08-30  11069   * @edid: EDID from monitor
c620e79bb695b8 Rodrigo Siqueira     2022-02-21  11070   *
c620e79bb695b8 Rodrigo Siqueira     2022-02-21  11071   * Amdgpu supports Freesync in DP and HDMI displays, and it is required to keep
c620e79bb695b8 Rodrigo Siqueira     2022-02-21  11072   * track of some of the display information in the internal data struct used by
c620e79bb695b8 Rodrigo Siqueira     2022-02-21  11073   * amdgpu_dm. This function checks which type of connector we need to set the
c620e79bb695b8 Rodrigo Siqueira     2022-02-21  11074   * FreeSync parameters.
c620e79bb695b8 Rodrigo Siqueira     2022-02-21  11075   */
98e6436d3af5fe Anthony Koo          2018-08-21  11076  void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
e5d89dbba370d6 Melissa Wen          2024-01-26  11077  				    const struct drm_edid *drm_edid)
e7b07ceef2a650 Harry Wentland       2017-08-10 @11078  {
eb0709ba077a21 Souptick Joarder     2021-02-23  11079  	int i = 0;
e7b07ceef2a650 Harry Wentland       2017-08-10  11080  	struct detailed_timing *timing;
e7b07ceef2a650 Harry Wentland       2017-08-10  11081  	struct detailed_non_pixel *data;
e7b07ceef2a650 Harry Wentland       2017-08-10  11082  	struct detailed_data_monitor_range *range;
c84dec2fe8837f Harry Wentland       2017-09-05  11083  	struct amdgpu_dm_connector *amdgpu_dm_connector =
c84dec2fe8837f Harry Wentland       2017-09-05  11084  			to_amdgpu_dm_connector(connector);
bb47de73666188 Nicholas Kazlauskas  2018-10-04  11085  	struct dm_connector_state *dm_con_state = NULL;
9ad544670514e2 Colin Ian King       2021-08-29  11086  	struct dc_sink *sink;
534eee82356c22 Srinivasan Shanmugam 2023-11-12  11087  	struct amdgpu_device *adev = drm_to_adev(connector->dev);
f9b4f20c4777bd Stylon Wang          2020-12-04  11088  	struct amdgpu_hdmi_vsdb_info vsdb_info = {0};
e5d89dbba370d6 Melissa Wen          2024-01-26  11089  	const struct edid *edid = drm_edid_raw(drm_edid);
c620e79bb695b8 Rodrigo Siqueira     2022-02-21  11090  	bool freesync_capable = false;
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11091  	enum adaptive_sync_type as_type = ADAPTIVE_SYNC_TYPE_NONE;
b830ebc910f641 Harry Wentland       2017-07-26  11092  
8218d7f1f70179 Harry Wentland       2017-10-17  11093  	if (!connector->state) {
8218d7f1f70179 Harry Wentland       2017-10-17  11094  		DRM_ERROR("%s - Connector has no state", __func__);
bb47de73666188 Nicholas Kazlauskas  2018-10-04  11095  		goto update;
8218d7f1f70179 Harry Wentland       2017-10-17  11096  	}
8218d7f1f70179 Harry Wentland       2017-10-17  11097  
9b2fdc33218933 Aurabindo Pillai     2021-08-11  11098  	sink = amdgpu_dm_connector->dc_sink ?
9b2fdc33218933 Aurabindo Pillai     2021-08-11  11099  		amdgpu_dm_connector->dc_sink :
9b2fdc33218933 Aurabindo Pillai     2021-08-11  11100  		amdgpu_dm_connector->dc_em_sink;
9b2fdc33218933 Aurabindo Pillai     2021-08-11  11101  
e5d89dbba370d6 Melissa Wen          2024-01-26  11102  	if (!drm_edid || !sink) {
98e6436d3af5fe Anthony Koo          2018-08-21  11103  		dm_con_state = to_dm_connector_state(connector->state);
98e6436d3af5fe Anthony Koo          2018-08-21  11104  
98e6436d3af5fe Anthony Koo          2018-08-21  11105  		amdgpu_dm_connector->min_vfreq = 0;
98e6436d3af5fe Anthony Koo          2018-08-21  11106  		amdgpu_dm_connector->max_vfreq = 0;
98e6436d3af5fe Anthony Koo          2018-08-21  11107  		amdgpu_dm_connector->pixel_clock_mhz = 0;
9b2fdc33218933 Aurabindo Pillai     2021-08-11  11108  		connector->display_info.monitor_range.min_vfreq = 0;
9b2fdc33218933 Aurabindo Pillai     2021-08-11  11109  		connector->display_info.monitor_range.max_vfreq = 0;
9b2fdc33218933 Aurabindo Pillai     2021-08-11  11110  		freesync_capable = false;
98e6436d3af5fe Anthony Koo          2018-08-21  11111  
bb47de73666188 Nicholas Kazlauskas  2018-10-04  11112  		goto update;
98e6436d3af5fe Anthony Koo          2018-08-21  11113  	}
98e6436d3af5fe Anthony Koo          2018-08-21  11114  
8218d7f1f70179 Harry Wentland       2017-10-17  11115  	dm_con_state = to_dm_connector_state(connector->state);
8218d7f1f70179 Harry Wentland       2017-10-17  11116  
e7b07ceef2a650 Harry Wentland       2017-08-10  11117  	if (!adev->dm.freesync_module)
bb47de73666188 Nicholas Kazlauskas  2018-10-04  11118  		goto update;
f9b4f20c4777bd Stylon Wang          2020-12-04  11119  
9b2fdc33218933 Aurabindo Pillai     2021-08-11  11120  	if (sink->sink_signal == SIGNAL_TYPE_DISPLAY_PORT
9b2fdc33218933 Aurabindo Pillai     2021-08-11  11121  		|| sink->sink_signal == SIGNAL_TYPE_EDP) {
f9b4f20c4777bd Stylon Wang          2020-12-04  11122  		bool edid_check_required = false;
f9b4f20c4777bd Stylon Wang          2020-12-04  11123  
e5d89dbba370d6 Melissa Wen          2024-01-26  11124  		if (drm_edid) {
e7b07ceef2a650 Harry Wentland       2017-08-10  11125  			edid_check_required = is_dp_capable_without_timing_msa(
e7b07ceef2a650 Harry Wentland       2017-08-10  11126  						adev->dm.dc,
c84dec2fe8837f Harry Wentland       2017-09-05  11127  						amdgpu_dm_connector);
e7b07ceef2a650 Harry Wentland       2017-08-10  11128  		}
f9b4f20c4777bd Stylon Wang          2020-12-04  11129  
e7b07ceef2a650 Harry Wentland       2017-08-10  11130  		if (edid_check_required == true && (edid->version > 1 ||
e7b07ceef2a650 Harry Wentland       2017-08-10  11131  		   (edid->version == 1 && edid->revision > 1))) {
e7b07ceef2a650 Harry Wentland       2017-08-10  11132  			for (i = 0; i < 4; i++) {
e7b07ceef2a650 Harry Wentland       2017-08-10  11133  
e7b07ceef2a650 Harry Wentland       2017-08-10  11134  				timing	= &edid->detailed_timings[i];
e7b07ceef2a650 Harry Wentland       2017-08-10  11135  				data	= &timing->data.other_data;
e7b07ceef2a650 Harry Wentland       2017-08-10  11136  				range	= &data->data.range;
e7b07ceef2a650 Harry Wentland       2017-08-10  11137  				/*
e7b07ceef2a650 Harry Wentland       2017-08-10  11138  				 * Check if monitor has continuous frequency mode
e7b07ceef2a650 Harry Wentland       2017-08-10  11139  				 */
e7b07ceef2a650 Harry Wentland       2017-08-10  11140  				if (data->type != EDID_DETAIL_MONITOR_RANGE)
e7b07ceef2a650 Harry Wentland       2017-08-10  11141  					continue;
e7b07ceef2a650 Harry Wentland       2017-08-10  11142  				/*
e7b07ceef2a650 Harry Wentland       2017-08-10  11143  				 * Check for flag range limits only. If flag == 1 then
e7b07ceef2a650 Harry Wentland       2017-08-10  11144  				 * no additional timing information provided.
e7b07ceef2a650 Harry Wentland       2017-08-10  11145  				 * Default GTF, GTF Secondary curve and CVT are not
e7b07ceef2a650 Harry Wentland       2017-08-10  11146  				 * supported
e7b07ceef2a650 Harry Wentland       2017-08-10  11147  				 */
e7b07ceef2a650 Harry Wentland       2017-08-10  11148  				if (range->flags != 1)
e7b07ceef2a650 Harry Wentland       2017-08-10  11149  					continue;
e7b07ceef2a650 Harry Wentland       2017-08-10  11150  
c84dec2fe8837f Harry Wentland       2017-09-05  11151  				amdgpu_dm_connector->min_vfreq = range->min_vfreq;
c84dec2fe8837f Harry Wentland       2017-09-05  11152  				amdgpu_dm_connector->max_vfreq = range->max_vfreq;
c84dec2fe8837f Harry Wentland       2017-09-05  11153  				amdgpu_dm_connector->pixel_clock_mhz =
e7b07ceef2a650 Harry Wentland       2017-08-10  11154  					range->pixel_clock_mhz * 10;
a0ffc3fd67e72b Stylon Wang          2021-01-05  11155  
a0ffc3fd67e72b Stylon Wang          2021-01-05  11156  				connector->display_info.monitor_range.min_vfreq = range->min_vfreq;
a0ffc3fd67e72b Stylon Wang          2021-01-05  11157  				connector->display_info.monitor_range.max_vfreq = range->max_vfreq;
a0ffc3fd67e72b Stylon Wang          2021-01-05  11158  
e7b07ceef2a650 Harry Wentland       2017-08-10  11159  				break;
e7b07ceef2a650 Harry Wentland       2017-08-10  11160  			}
e7b07ceef2a650 Harry Wentland       2017-08-10  11161  
c84dec2fe8837f Harry Wentland       2017-09-05  11162  			if (amdgpu_dm_connector->max_vfreq -
c84dec2fe8837f Harry Wentland       2017-09-05  11163  			    amdgpu_dm_connector->min_vfreq > 10) {
98e6436d3af5fe Anthony Koo          2018-08-21  11164  
bb47de73666188 Nicholas Kazlauskas  2018-10-04  11165  				freesync_capable = true;
e7b07ceef2a650 Harry Wentland       2017-08-10  11166  			}
e7b07ceef2a650 Harry Wentland       2017-08-10  11167  		}
ec8e59cb4e0c1a Bhawanpreet Lakha    2023-06-12  11168  		parse_amd_vsdb(amdgpu_dm_connector, edid, &vsdb_info);
ec8e59cb4e0c1a Bhawanpreet Lakha    2023-06-12  11169  
ec8e59cb4e0c1a Bhawanpreet Lakha    2023-06-12  11170  		if (vsdb_info.replay_mode) {
ec8e59cb4e0c1a Bhawanpreet Lakha    2023-06-12  11171  			amdgpu_dm_connector->vsdb_info.replay_mode = vsdb_info.replay_mode;
ec8e59cb4e0c1a Bhawanpreet Lakha    2023-06-12  11172  			amdgpu_dm_connector->vsdb_info.amd_vsdb_version = vsdb_info.amd_vsdb_version;
ec8e59cb4e0c1a Bhawanpreet Lakha    2023-06-12  11173  			amdgpu_dm_connector->as_type = ADAPTIVE_SYNC_TYPE_EDP;
ec8e59cb4e0c1a Bhawanpreet Lakha    2023-06-12  11174  		}
ec8e59cb4e0c1a Bhawanpreet Lakha    2023-06-12  11175  
e5d89dbba370d6 Melissa Wen          2024-01-26  11176  	} else if (drm_edid && sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A) {
7c7dd77489540d Arnd Bergmann        2021-02-25  11177  		i = parse_hdmi_amd_vsdb(amdgpu_dm_connector, edid, &vsdb_info);
7c7dd77489540d Arnd Bergmann        2021-02-25  11178  		if (i >= 0 && vsdb_info.freesync_supported) {
f9b4f20c4777bd Stylon Wang          2020-12-04  11179  			timing  = &edid->detailed_timings[i];
f9b4f20c4777bd Stylon Wang          2020-12-04  11180  			data    = &timing->data.other_data;
f9b4f20c4777bd Stylon Wang          2020-12-04  11181  
f9b4f20c4777bd Stylon Wang          2020-12-04  11182  			amdgpu_dm_connector->min_vfreq = vsdb_info.min_refresh_rate_hz;
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11183  			amdgpu_dm_connector->max_vfreq = vsdb_info.max_refresh_rate_hz;
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11184  			if (amdgpu_dm_connector->max_vfreq - amdgpu_dm_connector->min_vfreq > 10)
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11185  				freesync_capable = true;
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11186  
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11187  			connector->display_info.monitor_range.min_vfreq = vsdb_info.min_refresh_rate_hz;
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11188  			connector->display_info.monitor_range.max_vfreq = vsdb_info.max_refresh_rate_hz;
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11189  		}
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11190  	}
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11191  
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11192  	as_type = dm_get_adaptive_sync_support_type(amdgpu_dm_connector->dc_link);
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11193  
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11194  	if (as_type == FREESYNC_TYPE_PCON_IN_WHITELIST) {
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11195  		i = parse_hdmi_amd_vsdb(amdgpu_dm_connector, edid, &vsdb_info);
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11196  		if (i >= 0 && vsdb_info.freesync_supported && vsdb_info.amd_vsdb_version > 0) {
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11197  
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11198  			amdgpu_dm_connector->pack_sdp_v1_3 = true;
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11199  			amdgpu_dm_connector->as_type = as_type;
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11200  			amdgpu_dm_connector->vsdb_info = vsdb_info;
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11201  
5b49da02ddbe1b Sung Joon Kim        2023-01-12  11202  			amdgpu_dm_connector->min_vfreq = vsdb_info.min_refresh_rate_hz;
f9b4f20c4777bd Stylon Wang          2020-12-04  11203  			amdgpu_dm_connector->max_vfreq = vsdb_info.max_refresh_rate_hz;
f9b4f20c4777bd Stylon Wang          2020-12-04  11204  			if (amdgpu_dm_connector->max_vfreq - amdgpu_dm_connector->min_vfreq > 10)
f9b4f20c4777bd Stylon Wang          2020-12-04  11205  				freesync_capable = true;
f9b4f20c4777bd Stylon Wang          2020-12-04  11206  
f9b4f20c4777bd Stylon Wang          2020-12-04  11207  			connector->display_info.monitor_range.min_vfreq = vsdb_info.min_refresh_rate_hz;
f9b4f20c4777bd Stylon Wang          2020-12-04  11208  			connector->display_info.monitor_range.max_vfreq = vsdb_info.max_refresh_rate_hz;
f9b4f20c4777bd Stylon Wang          2020-12-04  11209  		}
f9b4f20c4777bd Stylon Wang          2020-12-04  11210  	}
bb47de73666188 Nicholas Kazlauskas  2018-10-04  11211  
bb47de73666188 Nicholas Kazlauskas  2018-10-04  11212  update:
bb47de73666188 Nicholas Kazlauskas  2018-10-04  11213  	if (dm_con_state)
bb47de73666188 Nicholas Kazlauskas  2018-10-04  11214  		dm_con_state->freesync_capable = freesync_capable;
bb47de73666188 Nicholas Kazlauskas  2018-10-04  11215  
bb47de73666188 Nicholas Kazlauskas  2018-10-04  11216  	if (connector->vrr_capable_property)
bb47de73666188 Nicholas Kazlauskas  2018-10-04  11217  		drm_connector_set_vrr_capable_property(connector,
bb47de73666188 Nicholas Kazlauskas  2018-10-04  11218  						       freesync_capable);
e7b07ceef2a650 Harry Wentland       2017-08-10  11219  }
e7b07ceef2a650 Harry Wentland       2017-08-10  11220  

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC PATCH 0/2] drm/amd/display: switch amdgpu_dm_connector to
  2024-01-29  9:30   ` Jani Nikula
@ 2024-02-05 14:27     ` Melissa Wen
  0 siblings, 0 replies; 11+ messages in thread
From: Melissa Wen @ 2024-02-05 14:27 UTC (permalink / raw)
  To: Jani Nikula
  Cc: Mario Limonciello, airlied, alexander.deucher, alex.hung,
	christian.koenig, daniel, harry.wentland, Rodrigo.Siqueira,
	sunpeng.li, Xinhui.Pan, kernel-dev, dri-devel, amd-gfx

On 01/29, Jani Nikula wrote:
> On Fri, 26 Jan 2024, Mario Limonciello <mario.limonciello@amd.com> wrote:
> > On 1/26/2024 10:28, Melissa Wen wrote:
> >> Hi,
> >> 
> >> I'm debugging a null-pointer dereference when running
> >> igt@kms_connector_force_edid and the way I found to solve the bug is to
> >> stop using raw edid handler in amdgpu_connector_funcs_force and
> >> create_eml_sink in favor of managing resouces via sruct drm_edid helpers
> >> (Patch 1). The proper solution seems to be switch amdgpu_dm_connector
> >> from struct edid to struct drm_edid and avoid the usage of different
> >> approaches in the driver (Patch 2). However, doing it implies a good
> >> amount of work and validation, therefore I decided to send this RFC
> >> first to collect opinions and check if there is any parallel work on
> >> this side. It's a working in progress.
> >> 
> >> The null-pointer error trigger by the igt@kms_connector_force_edid test
> >> was introduced by:
> >> - e54ed41620f ("drm/amd/display: Remove unwanted drm edid references")
> >> 
> >> You can check the error trace in the first patch.
> >> 
> >> This series was tested with kms_hdmi_inject and kms_force_connector. No
> >> null-pointer error, kms_hdmi_inject is successul and kms_force_connector
> >> is sucessful after the second execution - the force-edid subtest
> >> still fails in the first run (I'm still investigating).
> >> 
> >> There is also a couple of cast warnings to be addressed - I'm looking
> >> for the best replacement.
> >> 
> >> I appreciate any feedback and testing.
> >
> > So I'm actually a little bit worried by hardcoding EDID_LENGTH in this 
> > series.
> >
> > I have some other patches that I'm posting later on that let you get the 
> > EDID from _DDC BIOS method too.  My observation was that the EDID can be 
> > anywhere up to 512 bytes according to the ACPI spec.
> >
> > An earlier version of my patch was using EDID_LENGTH when fetching it 
> > and the EDID checksum failed.
> >
> > I'll CC you on the post, we probably want to get your changes and mine 
> > merged together.
> 
> One of the main points of struct drm_edid is that it tracks the
> allocation size separately.
> 
> We should simply not trust edid->extensions, because most of the time it
> originates from outside the kernel.
> 
> Using drm_edid and immediately drm_edid_raw() falls short. That function
> should only be used during migration to help. And yeah, it also means
> EDID parsing should be done in drm_edid.c, and not spread out all over
> the subsystem.

Hi Mario and Jani,

Thanks for the feedback.

I agree with you. I used the drm_edid_raw() as an intermediate step to
assess/validate this migration, but I'll work on removing this hack.

So, I understand that the transition from edid to drm_edid is the right
path, so I'll improve this work (keeping the points you raised in mind)
and send a version.

BR,

Melissa
> 
> 
> BR,
> Jani.
> 
> 
> >
> >> 
> >> Melissa
> >> 
> >> Melissa Wen (2):
> >>    drm/amd/display: fix null-pointer dereference on edid reading
> >>    drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid
> >> 
> >>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 78 ++++++++++---------
> >>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  4 +-
> >>   .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  9 ++-
> >>   .../display/amdgpu_dm/amdgpu_dm_mst_types.c   | 23 +++---
> >>   4 files changed, 60 insertions(+), 54 deletions(-)
> >> 
> >
> 
> -- 
> Jani Nikula, Intel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC PATCH 2/2] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid
  2024-01-26 19:33   ` Alex Hung
@ 2024-02-05 14:33     ` Melissa Wen
  0 siblings, 0 replies; 11+ messages in thread
From: Melissa Wen @ 2024-02-05 14:33 UTC (permalink / raw)
  To: Alex Hung
  Cc: harry.wentland, sunpeng.li, Rodrigo.Siqueira, alexander.deucher,
	christian.koenig, Xinhui.Pan, airlied, daniel, amd-gfx, dri-devel,
	kernel-dev

On 01/26, Alex Hung wrote:
> 
> 
> On 2024-01-26 09:28, 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. There are a couple of cast warnings and it was only
> > tested with IGT.
> > 
> > Signed-off-by: Melissa Wen <mwen@igalia.com>
> > ---
> >   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 63 ++++++++++---------
> >   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  4 +-
> >   .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  9 +--
> >   .../display/amdgpu_dm/amdgpu_dm_mst_types.c   | 23 +++----
> >   4 files changed, 51 insertions(+), 48 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 68e71e2ea472..741081d73bb3 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > @@ -3277,12 +3277,12 @@ void amdgpu_dm_update_connector_after_detect(
> >   					&aconnector->dm_dp_aux.aux);
> >   			}
> >   		} else {
> > -			aconnector->edid =
> > -				(struct edid *)sink->dc_edid.raw_edid;
> > +			const struct edid *edid = (const struct edid *)sink->dc_edid.raw_edid;
> > +			aconnector->edid = drm_edid_alloc(edid, (edid->extensions + 1) * EDID_LENGTH);
> >   			if (aconnector->dc_link->aux_mode)
> >   				drm_dp_cec_set_edid(&aconnector->dm_dp_aux.aux,
> > -						    aconnector->edid);
> > +						    drm_edid_raw(aconnector->edid));
> >   		}
> >   		if (!aconnector->timing_requested) {
> > @@ -3293,13 +3293,13 @@ void amdgpu_dm_update_connector_after_detect(
> >   					"failed to create aconnector->requested_timing\n");
> >   		}
> > -		drm_connector_update_edid_property(connector, aconnector->edid);
> > +		drm_edid_connector_update(connector, aconnector->edid);
> >   		amdgpu_dm_update_freesync_caps(connector, aconnector->edid);
> >   		update_connector_ext_caps(aconnector);
> >   	} else {
> >   		drm_dp_cec_unset_edid(&aconnector->dm_dp_aux.aux);
> >   		amdgpu_dm_update_freesync_caps(connector, NULL);
> > -		drm_connector_update_edid_property(connector, NULL);
> > +		drm_edid_connector_update(connector, NULL);
> >   		aconnector->num_modes = 0;
> >   		dc_sink_release(aconnector->dc_sink);
> >   		aconnector->dc_sink = NULL;
> > @@ -6564,7 +6564,6 @@ static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector)
> >   	struct dc_link *dc_link = aconnector->dc_link;
> >   	struct dc_sink *dc_em_sink = aconnector->dc_em_sink;
> >   	const struct drm_edid *drm_edid;
> > -	const struct edid *edid;
> >   	/*
> >   	 * Note: drm_get_edid gets edid in the following order:
> > @@ -6578,11 +6577,12 @@ static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector)
> >   		DRM_ERROR("No EDID found on connector: %s.\n", connector->name);
> >   		return;
> >   	}
> > -	edid = drm_edid_raw(drm_edid);
> > -	aconnector->edid = edid;
> > -
> > +	aconnector->edid = drm_edid;
> > +	drm_edid_connector_update(connector, drm_edid);
> >   	/* Update emulated (virtual) sink's EDID */
> >   	if (dc_em_sink && dc_link) {
> > +		const struct edid *edid = drm_edid_raw(drm_edid);
> > +
> >   		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(
> > @@ -6633,13 +6633,13 @@ static void create_eml_sink(struct amdgpu_dm_connector *aconnector)
> >   		return;
> >   	}
> > -	edid = drm_edid_raw(drm_edid);
> > -
> > -	if (drm_detect_hdmi_monitor(edid))
> > +	if (connector->display_info.is_hdmi)
> >   		init_params.sink_signal = SIGNAL_TYPE_HDMI_TYPE_A;
> > -	aconnector->edid = edid;
> > +	aconnector->edid = drm_edid;
> > +	drm_edid_connector_update(connector, drm_edid);
> > +	edid = drm_edid_raw(drm_edid);
> >   	aconnector->dc_em_sink = dc_link_add_remote_sink(
> >   		aconnector->dc_link,
> >   		(uint8_t *)edid,
> > @@ -7322,16 +7322,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
> > @@ -7345,10 +7345,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;
> >   	}
> > @@ -7444,12 +7444,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 (!edid)
> > +	if (!drm_edid)
> >   		return;
> >   	if (amdgpu_dm_connector->max_vfreq - amdgpu_dm_connector->min_vfreq > 10)
> > @@ -7462,23 +7462,23 @@ static int amdgpu_dm_connector_get_modes(struct drm_connector *connector)
> >   	struct amdgpu_dm_connector *amdgpu_dm_connector =
> >   			to_amdgpu_dm_connector(connector);
> >   	struct drm_encoder *encoder;
> > -	struct edid *edid = amdgpu_dm_connector->edid;
> > +	const struct drm_edid *drm_edid = amdgpu_dm_connector->edid;
> >   	struct dc_link_settings *verified_link_cap =
> >   			&amdgpu_dm_connector->dc_link->verified_link_cap;
> >   	const struct dc *dc = amdgpu_dm_connector->dc_link->dc;
> >   	encoder = amdgpu_dm_connector_to_encoder(connector);
> > -	if (!drm_edid_is_valid(edid)) {
> > +	if (!drm_edid_valid(drm_edid)) {
> >   		amdgpu_dm_connector->num_modes =
> >   				drm_add_modes_noedid(connector, 640, 480);
> >   		if (dc->link_srv->dp_get_encoding_format(verified_link_cap) == DP_128b_132b_ENCODING)
> >   			amdgpu_dm_connector->num_modes +=
> >   				drm_add_modes_noedid(connector, 1920, 1080);
> >   	} else {
> > -		amdgpu_dm_connector_ddc_get_modes(connector, edid);
> > +		amdgpu_dm_connector_ddc_get_modes(connector, drm_edid);
> >   		amdgpu_dm_connector_add_common_modes(encoder, connector);
> > -		amdgpu_dm_connector_add_freesync_modes(connector, edid);
> > +		amdgpu_dm_connector_add_freesync_modes(connector, drm_edid);
> >   	}
> >   	amdgpu_dm_fbc_init(connector);
> > @@ -11038,7 +11038,7 @@ static bool parse_edid_cea(struct amdgpu_dm_connector *aconnector,
> >   }
> >   static int parse_amd_vsdb(struct amdgpu_dm_connector *aconnector,
> > -			  struct edid *edid, struct amdgpu_hdmi_vsdb_info *vsdb_info)
> > +			  const struct edid *edid, struct amdgpu_hdmi_vsdb_info *vsdb_info)
> >   {
> >   	u8 *edid_ext = NULL;
> >   	int i;
> > @@ -11073,7 +11073,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;
> > @@ -11115,7 +11116,7 @@ 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;
> > @@ -11125,9 +11126,9 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *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 = drm_edid_raw(drm_edid);
> 
> 
> I got below compile errors:
> 
> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c: In function
> ‘amdgpu_dm_update_freesync_caps’:
> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:11176:41: error:
> assignment discards ‘const’ qualifier from pointer target type
> [-Werror=discarded-qualifiers]
> 11176 |                                 timing  =
> &edid->detailed_timings[i];
>       |                                         ^
> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:11221:33: error:
> assignment discards ‘const’ qualifier from pointer target type
> [-Werror=discarded-qualifiers]
> 11221 |                         timing  = &edid->detailed_timings[i];
> 
> 
> and the following changes fixes it:
> 
> -       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;

Oh, thanks! I didn't realize this fast path to fix these warnings.

I'm considering to replace it with a drm_edid helper in the next version
and avoid handling `struct edid` in the driver.

I'll work on it for the next version.

BR,

Melissa

> 
> 
> 
> 
> >   	bool freesync_capable = false;
> >   	enum adaptive_sync_type as_type = ADAPTIVE_SYNC_TYPE_NONE;
> > @@ -11140,7 +11141,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;
> > @@ -11162,7 +11163,7 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
> >   		|| sink->sink_signal == SIGNAL_TYPE_EDP) {
> >   		bool edid_check_required = false;
> > -		if (edid) {
> > +		if (drm_edid) {
> >   			edid_check_required = is_dp_capable_without_timing_msa(
> >   						adev->dm.dc,
> >   						amdgpu_dm_connector);
> > @@ -11214,7 +11215,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) {
> >   			timing  = &edid->detailed_timings[i];
> > 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 9c1871b866cc..b81cf6f3713b 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> > @@ -637,7 +637,7 @@ struct amdgpu_dm_connector {
> >   	/* we need to mind the EDID between detect
> >   	   and get modes due to analog/digital/tvencoder */
> > -	struct edid *edid;
> > +	const struct drm_edid *edid;
> >   	/* shared with amdgpu */
> >   	struct amdgpu_hpd hpd;
> > @@ -908,7 +908,7 @@ void dm_restore_drm_connector_state(struct drm_device *dev,
> >   				    struct drm_connector *connector);
> >   void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
> > -					struct edid *edid);
> > +				    const struct drm_edid *drm_edid);
> >   void amdgpu_dm_trigger_timing_sync(struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> > index e3915c4f8566..91006326ce6d 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
> > @@ -898,7 +898,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,8 +910,8 @@ enum dc_edid_status dm_helpers_read_local_edid(
> >   	 * do check sum and retry to make sure read correct edid.
> >   	 */
> >   	do {
> > -
> > -		edid = drm_get_edid(&aconnector->base, ddc);
> > +		drm_edid = drm_edid_read_ddc(connector, ddc);
> > +		edid = drm_edid_raw(drm_edid);
> >   		/* DP Compliance Test 4.2.2.6 */
> >   		if (link->aux_mode && connector->edid_corrupt)
> > @@ -928,7 +929,7 @@ enum dc_edid_status dm_helpers_read_local_edid(
> >   		memmove(sink->dc_edid.raw_edid, (uint8_t *)edid, sink->dc_edid.length);
> >   		/* We don't need the original edid anymore */
> > -		kfree(edid);
> > +		drm_edid_free(drm_edid);
> >   		edid_status = dm_helpers_parse_edid_caps(
> >   						link,
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> > index 7075a85dd16e..cdebd0e74b26 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
> > @@ -127,7 +127,7 @@ dm_dp_mst_connector_destroy(struct drm_connector *connector)
> >   		dc_sink_release(aconnector->dc_sink);
> >   	}
> > -	kfree(aconnector->edid);
> > +	drm_edid_free(aconnector->edid);
> >   	drm_connector_cleanup(connector);
> >   	drm_dp_mst_put_port_malloc(aconnector->mst_output_port);
> > @@ -297,15 +297,15 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
> >   		return drm_add_edid_modes(connector, NULL);
> >   	if (!aconnector->edid) {
> > -		struct edid *edid;
> > +		const struct drm_edid *drm_edid;
> > -		edid = drm_dp_mst_get_edid(connector, &aconnector->mst_root->mst_mgr, aconnector->mst_output_port);
> > +		drm_edid = drm_dp_mst_edid_read(connector, &aconnector->mst_root->mst_mgr, aconnector->mst_output_port);
> > -		if (!edid) {
> > +		if (!drm_edid) {
> >   			amdgpu_dm_set_mst_status(&aconnector->mst_status,
> >   			MST_REMOTE_EDID, false);
> > -			drm_connector_update_edid_property(
> > +			drm_edid_connector_update(
> >   				&aconnector->base,
> >   				NULL);
> > @@ -339,7 +339,7 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
> >   			return ret;
> >   		}
> > -		aconnector->edid = edid;
> > +		aconnector->edid = drm_edid;
> >   		amdgpu_dm_set_mst_status(&aconnector->mst_status,
> >   			MST_REMOTE_EDID, true);
> >   	}
> > @@ -354,10 +354,12 @@ 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 = drm_edid_raw(aconnector->edid);
> > +
> >   		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) {
> > @@ -411,10 +413,9 @@ static int dm_dp_mst_get_modes(struct drm_connector *connector)
> >   		}
> >   	}
> > -	drm_connector_update_edid_property(
> > -					&aconnector->base, aconnector->edid);
> > +	drm_edid_connector_update(&aconnector->base, aconnector->edid);
> > -	ret = drm_add_edid_modes(connector, aconnector->edid);
> > +	ret = drm_edid_connector_add_modes(connector);
> >   	return ret;
> >   }

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2024-02-05 14:34 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-26 16:28 [RFC PATCH 0/2] drm/amd/display: switch amdgpu_dm_connector to Melissa Wen
2024-01-26 16:28 ` [RFC PATCH 1/2] drm/amd/display: fix null-pointer dereference on edid reading Melissa Wen
2024-01-29 14:18   ` kernel test robot
2024-01-26 16:28 ` [RFC PATCH 2/2] drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid Melissa Wen
2024-01-26 19:33   ` Alex Hung
2024-02-05 14:33     ` Melissa Wen
2024-01-29  9:54   ` kernel test robot
2024-01-29 23:29   ` kernel test robot
2024-01-26 18:22 ` [RFC PATCH 0/2] drm/amd/display: switch amdgpu_dm_connector to Mario Limonciello
2024-01-29  9:30   ` Jani Nikula
2024-02-05 14:27     ` Melissa Wen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.