[PATCH v3 0/5] Add support for fetching EDID from ACPI _DDC

[PATCH v3 0/5] Add support for fetching EDID from ACPI _DDC

[Mario Limonciello]

Some laptops ship an EDID in the BIOS encoded in the _DDC method that
differs than the EDID directly on the laptop panel for $REASONS.

This is the EDID that is used by the AMD Windows driver, and so sometimes
different results are found in different operating systems.

This series adds a new DRM helper that will use acpi_video to fetch the
EDID.

On amdgpu when an eDP panel is found the BIOS
is checked first for an EDID and that used as a preference if found.

On nouveau it replaces the previous local function doing a similar role.

This does *not* use struct drm_edid as this will require more involved
amdgpu display driver work that will come separately as part of follow-ups
to: https://lore.kernel.org/amd-gfx/20240126163429.56714-1-mwen@igalia.com/

v2-v3:
 * Clean up some of the 'select ACPI_VIDEO' kconfig spaghetti reported by LKP
 * Drop the 'select ACPI_VIDEO' from the DRM drivers

Mario Limonciello (5):
  ACPI: video: Handle fetching EDID that is longer than 256 bytes
  drm: Add drm_get_acpi_edid() helper
  drm/amd: Fetch the EDID from _DDC if available for eDP
  drm/nouveau: Use drm_get_acpi_edid() helper
  drm: Drop unneeded selects in DRM drivers

 drivers/acpi/acpi_video.c                     | 25 +++----
 drivers/gpu/drm/Kconfig                       |  5 ++
 drivers/gpu/drm/amd/amdgpu/Kconfig            |  7 --
 drivers/gpu/drm/amd/amdgpu/amdgpu.h           |  1 +
 .../gpu/drm/amd/amdgpu/amdgpu_connectors.c    |  4 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c       |  8 ++
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 ++-
 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  9 ++-
 drivers/gpu/drm/drm_edid.c                    | 73 +++++++++++++++++++
 drivers/gpu/drm/gma500/Kconfig                |  6 --
 drivers/gpu/drm/i915/Kconfig                  |  7 --
 drivers/gpu/drm/nouveau/nouveau_acpi.c        | 27 -------
 drivers/gpu/drm/nouveau/nouveau_acpi.h        |  2 -
 drivers/gpu/drm/nouveau/nouveau_connector.c   |  2 +-
 drivers/gpu/drm/radeon/Kconfig                |  7 --
 drivers/gpu/drm/xe/Kconfig                    |  6 --
 include/drm/drm_edid.h                        |  1 +
 17 files changed, 116 insertions(+), 84 deletions(-)

-- 
2.34.1

[PATCH v3 1/5] ACPI: video: Handle fetching EDID that is longer than 256 bytes

[Mario Limonciello]

The ACPI specification allows for an EDID to be up to 512 bytes but
the _DDC EDID fetching code will only try up to 256 bytes.

Modify the code to instead start at 512 bytes and work it's way
down instead.

As _DDC is now called up to 4 times on a machine debugging messages
are noisier than necessary.  Decrease from info to debug.

Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/Apx_B_Video_Extensions/output-device-specific-methods.html#ddc-return-the-edid-for-this-device
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v1->v2:
 * Use for loop for acpi_video_get_edid()
 * Use one of Rafael's suggestions for acpi_video_device_EDID()
 * Decrease message level too
---
 drivers/acpi/acpi_video.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index 4afdda9db019..3bfd013e09d2 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -625,12 +625,9 @@ acpi_video_device_EDID(struct acpi_video_device *device,
 
 	if (!device)
 		return -ENODEV;
-	if (length == 128)
-		arg0.integer.value = 1;
-	else if (length == 256)
-		arg0.integer.value = 2;
-	else
+	if (!length || (length % 128))
 		return -EINVAL;
+	arg0.integer.value = length / 128;
 
 	status = acpi_evaluate_object(device->dev->handle, "_DDC", &args, &buffer);
 	if (ACPI_FAILURE(status))
@@ -641,7 +638,8 @@ acpi_video_device_EDID(struct acpi_video_device *device,
 	if (obj && obj->type == ACPI_TYPE_BUFFER)
 		*edid = obj;
 	else {
-		acpi_handle_info(device->dev->handle, "Invalid _DDC data\n");
+		acpi_handle_debug(device->dev->handle,
+				 "Invalid _DDC data for length %ld\n", length);
 		status = -EFAULT;
 		kfree(obj);
 	}
@@ -1447,7 +1445,6 @@ int acpi_video_get_edid(struct acpi_device *device, int type, int device_id,
 
 	for (i = 0; i < video->attached_count; i++) {
 		video_device = video->attached_array[i].bind_info;
-		length = 256;
 
 		if (!video_device)
 			continue;
@@ -1478,18 +1475,14 @@ int acpi_video_get_edid(struct acpi_device *device, int type, int device_id,
 			continue;
 		}
 
-		status = acpi_video_device_EDID(video_device, &buffer, length);
-
-		if (ACPI_FAILURE(status) || !buffer ||
-		    buffer->type != ACPI_TYPE_BUFFER) {
-			length = 128;
+		for (length = 512; length > 0; length -= 128) {
 			status = acpi_video_device_EDID(video_device, &buffer,
 							length);
-			if (ACPI_FAILURE(status) || !buffer ||
-			    buffer->type != ACPI_TYPE_BUFFER) {
-				continue;
-			}
+			if (ACPI_SUCCESS(status))
+				break;
 		}
+		if (!length)
+			continue;
 
 		*edid = buffer->buffer.pointer;
 		return length;
-- 
2.34.1

[PATCH v3 2/5] drm: Add drm_get_acpi_edid() helper

[Mario Limonciello]

Some manufacturers have intentionally put an EDID that differs from
the EDID on the internal panel on laptops.  Drivers can call this
helper to attempt to fetch the EDID from the BIOS's ACPI _DDC method.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v1->v2:
 * Split code from previous amdgpu specific helper to generic drm helper.
v2->v3:
 * Add an extra select to fix a variety of randconfig errors found from
   LKP robot.
---
 drivers/gpu/drm/Kconfig    |  5 +++
 drivers/gpu/drm/drm_edid.c | 73 ++++++++++++++++++++++++++++++++++++++
 include/drm/drm_edid.h     |  1 +
 3 files changed, 79 insertions(+)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 2520db0b776e..14df907c96c8 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -21,6 +21,11 @@ menuconfig DRM
 	select KCMP
 	select VIDEO_CMDLINE
 	select VIDEO_NOMODESET
+	select ACPI_VIDEO if ACPI
+	select BACKLIGHT_CLASS_DEVICE if ACPI
+	select INPUT if ACPI
+	select X86_PLATFORM_DEVICES if ACPI && X86
+	select ACPI_WMI if ACPI && X86
 	help
 	  Kernel-level support for the Direct Rendering Infrastructure (DRI)
 	  introduced in XFree86 4.0. If you say Y here, you need to select
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 69c68804023f..1fbbeaa664b2 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -28,6 +28,7 @@
  * DEALINGS IN THE SOFTWARE.
  */
 
+#include <acpi/video.h>
 #include <linux/bitfield.h>
 #include <linux/cec.h>
 #include <linux/hdmi.h>
@@ -2188,6 +2189,47 @@ drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned int block, size_t len)
 	return ret == xfers ? 0 : -1;
 }
 
+/**
+ * drm_do_probe_acpi_edid() - get EDID information via ACPI _DDC
+ * @data: struct drm_device
+ * @buf: EDID data buffer to be filled
+ * @block: 128 byte EDID block to start fetching from
+ * @len: EDID data buffer length to fetch
+ *
+ * Try to fetch EDID information by calling acpi_video_get_edid() function.
+ *
+ * Return: 0 on success or error code on failure.
+ */
+static int
+drm_do_probe_acpi_edid(void *data, u8 *buf, unsigned int block, size_t len)
+{
+	struct drm_device *ddev = data;
+	struct acpi_device *acpidev = ACPI_COMPANION(ddev->dev);
+	unsigned char start = block * EDID_LENGTH;
+	void *edid;
+	int r;
+
+	if (!acpidev)
+		return -ENODEV;
+
+	/* fetch the entire edid from BIOS */
+	r = acpi_video_get_edid(acpidev, ACPI_VIDEO_DISPLAY_LCD, -1, &edid);
+	if (r < 0) {
+		DRM_DEBUG_KMS("Failed to get EDID from ACPI: %d\n", r);
+		return -EINVAL;
+	}
+	if (len > r || start > r || start + len > r) {
+		r = EINVAL;
+		goto cleanup;
+	}
+
+	memcpy(buf, edid + start, len);
+	r = 0;
+cleanup:
+	kfree(edid);
+	return r;
+}
+
 static void connector_bad_edid(struct drm_connector *connector,
 			       const struct edid *edid, int num_blocks)
 {
@@ -2643,6 +2685,37 @@ struct edid *drm_get_edid(struct drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_get_edid);
 
+/**
+ * drm_get_acpi_edid - get EDID data, if available
+ * @connector: connector we're probing
+ *
+ * Use the BIOS to attempt to grab EDID data if possible.  If found,
+ * attach it to the connector.
+ *
+ * Return: Pointer to valid EDID or NULL if we couldn't find any.
+ */
+struct edid *drm_get_acpi_edid(struct drm_connector *connector)
+{
+	struct edid *edid = NULL;
+
+	switch (connector->connector_type) {
+	case DRM_MODE_CONNECTOR_LVDS:
+	case DRM_MODE_CONNECTOR_eDP:
+		break;
+	default:
+		return NULL;
+	}
+
+	if (connector->force == DRM_FORCE_OFF)
+		return NULL;
+
+	edid = _drm_do_get_edid(connector, drm_do_probe_acpi_edid, connector->dev, NULL);
+
+	drm_connector_update_edid_property(connector, edid);
+	return edid;
+}
+EXPORT_SYMBOL(drm_get_acpi_edid);
+
 /**
  * drm_edid_read_custom - Read EDID data using given EDID block read function
  * @connector: Connector to use
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 518d1b8106c7..60fbdc06badc 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -412,6 +412,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 	void *data);
 struct edid *drm_get_edid(struct drm_connector *connector,
 			  struct i2c_adapter *adapter);
+struct edid *drm_get_acpi_edid(struct drm_connector *connector);
 u32 drm_edid_get_panel_id(struct i2c_adapter *adapter);
 struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
 				     struct i2c_adapter *adapter);
-- 
2.34.1

[PATCH v3 3/5] drm/amd: Fetch the EDID from _DDC if available for eDP

[Mario Limonciello]

Some manufacturers have intentionally put an EDID that differs from
the EDID on the internal panel on laptops.

Attempt to fetch this EDID if it exists and prefer it over the EDID
that is provided by the panel.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v2:
 * Use drm helper which will run more validation
 * Move eDP check to DRM helper
 * Add module parameter
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h                    |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c         |  4 ++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c                |  8 ++++++++
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c      | 10 ++++++++--
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c  |  9 ++++++---
 5 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 3d8a48f46b01..5d5be3e20687 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -217,6 +217,7 @@ extern int amdgpu_smartshift_bias;
 extern int amdgpu_use_xgmi_p2p;
 extern int amdgpu_mtype_local;
 extern bool enforce_isolation;
+extern bool acpi_edid;
 #ifdef CONFIG_HSA_AMD
 extern int sched_policy;
 extern bool debug_evictions;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
index 9caba10315a8..6aa8cc431abe 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
@@ -278,6 +278,10 @@ static void amdgpu_connector_get_edid(struct drm_connector *connector)
 	struct amdgpu_device *adev = drm_to_adev(dev);
 	struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(connector);
 
+	/* if the BIOS specifies the EDID via _DDC, prefer this */
+	if (acpi_edid && !amdgpu_connector->edid)
+		amdgpu_connector->edid = drm_get_acpi_edid(connector);
+
 	if (amdgpu_connector->edid)
 		return;
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index cc69005f5b46..be7a4da85a8b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -166,6 +166,7 @@ uint amdgpu_sdma_phase_quantum = 32;
 char *amdgpu_disable_cu;
 char *amdgpu_virtual_display;
 bool enforce_isolation;
+bool acpi_edid = true;
 /*
  * OverDrive(bit 14) disabled by default
  * GFX DCS(bit 19) disabled by default
@@ -990,6 +991,13 @@ MODULE_PARM_DESC(wbrf,
 	"Enable Wifi RFI interference mitigation (0 = disabled, 1 = enabled, -1 = auto(default)");
 module_param_named(wbrf, amdgpu_wbrf, int, 0444);
 
+/**
+ * DOC: acpi_edid (bool)
+ * Try to fetch EDID for eDP display from BIOS using ACPI _DDC method.
+ */
+module_param(acpi_edid, bool, 0444);
+MODULE_PARM_DESC(acpi_edid, "Fetch EDID for eDP display from BIOS");
+
 /* These devices are not supported by amdgpu.
  * They are supported by the mach64, r128, radeon drivers
  */
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 202c6ad443a3..688d615c6687 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6589,7 +6589,11 @@ static void amdgpu_dm_connector_funcs_force(struct drm_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;
+	struct edid *edid = NULL;
+
+	/* prefer ACPI over panel for eDP */
+	if (acpi_edid)
+		edid = drm_get_acpi_edid(connector);
 
 	/*
 	 * Note: drm_get_edid gets edid in the following order:
@@ -6597,7 +6601,9 @@ 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)
+		edid = drm_get_edid(connector, &amdgpu_connector->ddc_bus->aux.ddc);
+
 	if (!edid) {
 		DRM_ERROR("No EDID found on connector: %s.\n", connector->name);
 		return;
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 85b7f58a7f35..cc39b1c14aa8 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
@@ -899,7 +899,7 @@ 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;
+	struct edid *edid = NULL;
 
 	if (link->aux_mode)
 		ddc = &aconnector->dm_dp_aux.aux.ddc;
@@ -910,8 +910,11 @@ 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);
+		/* prefer ACPI over panel for eDP */
+		if (acpi_edid)
+			edid = drm_get_acpi_edid(connector);
+		if (!edid)
+			edid = drm_get_edid(&aconnector->base, ddc);
 
 		/* DP Compliance Test 4.2.2.6 */
 		if (link->aux_mode && connector->edid_corrupt)
-- 
2.34.1

[PATCH v3 4/5] drm/nouveau: Use drm_get_acpi_edid() helper

[Mario Limonciello]

Rather than inventing a wrapper to acpi_video_get_edid() use the
one provided by drm. This fixes two problems:
1. A memory leak that the memory provided by the ACPI call was
   never freed.
2. Validation of the BIOS provided blob.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v1->v2:
 * New patch
---
 drivers/gpu/drm/nouveau/nouveau_acpi.c      | 27 ---------------------
 drivers/gpu/drm/nouveau/nouveau_acpi.h      |  2 --
 drivers/gpu/drm/nouveau/nouveau_connector.c |  2 +-
 3 files changed, 1 insertion(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c
index 8f0c69aad248..de9daafb3fbb 100644
--- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
+++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
@@ -360,33 +360,6 @@ void nouveau_unregister_dsm_handler(void) {}
 void nouveau_switcheroo_optimus_dsm(void) {}
 #endif
 
-void *
-nouveau_acpi_edid(struct drm_device *dev, struct drm_connector *connector)
-{
-	struct acpi_device *acpidev;
-	int type, ret;
-	void *edid;
-
-	switch (connector->connector_type) {
-	case DRM_MODE_CONNECTOR_LVDS:
-	case DRM_MODE_CONNECTOR_eDP:
-		type = ACPI_VIDEO_DISPLAY_LCD;
-		break;
-	default:
-		return NULL;
-	}
-
-	acpidev = ACPI_COMPANION(dev->dev);
-	if (!acpidev)
-		return NULL;
-
-	ret = acpi_video_get_edid(acpidev, type, -1, &edid);
-	if (ret < 0)
-		return NULL;
-
-	return kmemdup(edid, EDID_LENGTH, GFP_KERNEL);
-}
-
 bool nouveau_acpi_video_backlight_use_native(void)
 {
 	return acpi_video_backlight_use_native();
diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.h b/drivers/gpu/drm/nouveau/nouveau_acpi.h
index e39dd8b94b8b..6a3def8e6cca 100644
--- a/drivers/gpu/drm/nouveau/nouveau_acpi.h
+++ b/drivers/gpu/drm/nouveau/nouveau_acpi.h
@@ -10,7 +10,6 @@ bool nouveau_is_v1_dsm(void);
 void nouveau_register_dsm_handler(void);
 void nouveau_unregister_dsm_handler(void);
 void nouveau_switcheroo_optimus_dsm(void);
-void *nouveau_acpi_edid(struct drm_device *, struct drm_connector *);
 bool nouveau_acpi_video_backlight_use_native(void);
 void nouveau_acpi_video_register_backlight(void);
 #else
@@ -19,7 +18,6 @@ static inline bool nouveau_is_v1_dsm(void) { return false; };
 static inline void nouveau_register_dsm_handler(void) {}
 static inline void nouveau_unregister_dsm_handler(void) {}
 static inline void nouveau_switcheroo_optimus_dsm(void) {}
-static inline void *nouveau_acpi_edid(struct drm_device *dev, struct drm_connector *connector) { return NULL; }
 static inline bool nouveau_acpi_video_backlight_use_native(void) { return true; }
 static inline void nouveau_acpi_video_register_backlight(void) {}
 #endif
diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
index 856b3ef5edb8..746571d4cac0 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -713,7 +713,7 @@ nouveau_connector_detect_lvds(struct drm_connector *connector, bool force)
 	 * valid - it's not (rh#613284)
 	 */
 	if (nv_encoder->dcb->lvdsconf.use_acpi_for_edid) {
-		edid = nouveau_acpi_edid(dev, connector);
+		edid = drm_get_acpi_edid(connector);
 		if (edid) {
 			status = connector_status_connected;
 			goto out;
-- 
2.34.1

[PATCH v3 5/5] drm: Drop unneeded selects in DRM drivers

[Mario Limonciello]

All of the selects on ACPI_VIDEO are unnecessary when DRM does the
select for ACPI_VIDEO as it provides a helper for acpi based EDID.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v2->v3:
 * new patch
---
 drivers/gpu/drm/amd/amdgpu/Kconfig | 7 -------
 drivers/gpu/drm/gma500/Kconfig     | 6 ------
 drivers/gpu/drm/i915/Kconfig       | 7 -------
 drivers/gpu/drm/radeon/Kconfig     | 7 -------
 drivers/gpu/drm/xe/Kconfig         | 6 ------
 5 files changed, 33 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig b/drivers/gpu/drm/amd/amdgpu/Kconfig
index 22d88f8ef527..b2178a5a947c 100644
--- a/drivers/gpu/drm/amd/amdgpu/Kconfig
+++ b/drivers/gpu/drm/amd/amdgpu/Kconfig
@@ -22,13 +22,6 @@ config DRM_AMDGPU
 	select DRM_BUDDY
 	select DRM_SUBALLOC_HELPER
 	select DRM_EXEC
-	# amdgpu depends on ACPI_VIDEO when ACPI is enabled, for select to work
-	# ACPI_VIDEO's dependencies must also be selected.
-	select INPUT if ACPI
-	select ACPI_VIDEO if ACPI
-	# On x86 ACPI_VIDEO also needs ACPI_WMI
-	select X86_PLATFORM_DEVICES if ACPI && X86
-	select ACPI_WMI if ACPI && X86
 	help
 	  Choose this option if you have a recent AMD Radeon graphics card.
 
diff --git a/drivers/gpu/drm/gma500/Kconfig b/drivers/gpu/drm/gma500/Kconfig
index efb4a2dd2f80..6921ef67b256 100644
--- a/drivers/gpu/drm/gma500/Kconfig
+++ b/drivers/gpu/drm/gma500/Kconfig
@@ -6,12 +6,6 @@ config DRM_GMA500
 	select FB_IOMEM_HELPERS if DRM_FBDEV_EMULATION
 	select I2C
 	select I2C_ALGOBIT
-	# GMA500 depends on ACPI_VIDEO when ACPI is enabled, just like i915
-	select ACPI_VIDEO if ACPI
-	select BACKLIGHT_CLASS_DEVICE if ACPI
-	select INPUT if ACPI
-	select X86_PLATFORM_DEVICES if ACPI
-	select ACPI_WMI if ACPI
 	help
 	  Say yes for an experimental 2D KMS framebuffer driver for the
 	  Intel GMA500 (Poulsbo), Intel GMA600 (Moorestown/Oak Trail) and
diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
index b5d6e3352071..476da09433bb 100644
--- a/drivers/gpu/drm/i915/Kconfig
+++ b/drivers/gpu/drm/i915/Kconfig
@@ -22,13 +22,6 @@ config DRM_I915
 	select I2C
 	select I2C_ALGOBIT
 	select IRQ_WORK
-	# i915 depends on ACPI_VIDEO when ACPI is enabled
-	# but for select to work, need to select ACPI_VIDEO's dependencies, ick
-	select BACKLIGHT_CLASS_DEVICE if ACPI
-	select INPUT if ACPI
-	select X86_PLATFORM_DEVICES if ACPI
-	select ACPI_WMI if ACPI
-	select ACPI_VIDEO if ACPI
 	select ACPI_BUTTON if ACPI
 	select SYNC_FILE
 	select IOSF_MBI if X86
diff --git a/drivers/gpu/drm/radeon/Kconfig b/drivers/gpu/drm/radeon/Kconfig
index f98356be0af2..12149d594100 100644
--- a/drivers/gpu/drm/radeon/Kconfig
+++ b/drivers/gpu/drm/radeon/Kconfig
@@ -19,13 +19,6 @@ config DRM_RADEON
 	select INTERVAL_TREE
 	select I2C
 	select I2C_ALGOBIT
-	# radeon depends on ACPI_VIDEO when ACPI is enabled, for select to work
-	# ACPI_VIDEO's dependencies must also be selected.
-	select INPUT if ACPI
-	select ACPI_VIDEO if ACPI
-	# On x86 ACPI_VIDEO also needs ACPI_WMI
-	select X86_PLATFORM_DEVICES if ACPI && X86
-	select ACPI_WMI if ACPI && X86
 	help
 	  Choose this option if you have an ATI Radeon graphics card.  There
 	  are both PCI and AGP versions.  You don't need to choose this to
diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig
index e36ae1f0d885..cf60bdcafb0c 100644
--- a/drivers/gpu/drm/xe/Kconfig
+++ b/drivers/gpu/drm/xe/Kconfig
@@ -19,13 +19,7 @@ config DRM_XE
 	select DRM_MIPI_DSI
 	select RELAY
 	select IRQ_WORK
-	# xe depends on ACPI_VIDEO when ACPI is enabled
-	# but for select to work, need to select ACPI_VIDEO's dependencies, ick
-	select BACKLIGHT_CLASS_DEVICE if ACPI
-	select INPUT if ACPI
-	select ACPI_VIDEO if X86 && ACPI
 	select ACPI_BUTTON if ACPI
-	select ACPI_WMI if X86 && ACPI
 	select SYNC_FILE
 	select IOSF_MBI
 	select CRC32
-- 
2.34.1

Re: [PATCH v3 5/5] drm: Drop unneeded selects in DRM drivers

[Pranjal Ramajor Asha Kanojiya]

On 2/2/2024 3:41 AM, Mario Limonciello wrote:
> All of the selects on ACPI_VIDEO are unnecessary when DRM does the
> select for ACPI_VIDEO as it provides a helper for acpi based EDID.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
Reviewed-by: Pranjal Ramajor Asha Kanojiya <quic_pkanojiy@quicinc.com>

Re: [PATCH v3 2/5] drm: Add drm_get_acpi_edid() helper

[Pranjal Ramajor Asha Kanojiya]

On 2/2/2024 3:41 AM, Mario Limonciello wrote:
> Some manufacturers have intentionally put an EDID that differs from
> the EDID on the internal panel on laptops.  Drivers can call this
> helper to attempt to fetch the EDID from the BIOS's ACPI _DDC method.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v1->v2:
>   * Split code from previous amdgpu specific helper to generic drm helper.
> v2->v3:
>   * Add an extra select to fix a variety of randconfig errors found from
>     LKP robot.
> ---
>   drivers/gpu/drm/Kconfig    |  5 +++
>   drivers/gpu/drm/drm_edid.c | 73 ++++++++++++++++++++++++++++++++++++++
>   include/drm/drm_edid.h     |  1 +
>   3 files changed, 79 insertions(+)
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 2520db0b776e..14df907c96c8 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -21,6 +21,11 @@ menuconfig DRM
>   	select KCMP
>   	select VIDEO_CMDLINE
>   	select VIDEO_NOMODESET
> +	select ACPI_VIDEO if ACPI
> +	select BACKLIGHT_CLASS_DEVICE if ACPI
> +	select INPUT if ACPI
> +	select X86_PLATFORM_DEVICES if ACPI && X86
> +	select ACPI_WMI if ACPI && X86
>   	help
>   	  Kernel-level support for the Direct Rendering Infrastructure (DRI)
>   	  introduced in XFree86 4.0. If you say Y here, you need to select
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 69c68804023f..1fbbeaa664b2 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -28,6 +28,7 @@
>    * DEALINGS IN THE SOFTWARE.
>    */
>   
> +#include <acpi/video.h>
>   #include <linux/bitfield.h>
>   #include <linux/cec.h>
>   #include <linux/hdmi.h>
> @@ -2188,6 +2189,47 @@ drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned int block, size_t len)
>   	return ret == xfers ? 0 : -1;
>   }
>   
> +/**
> + * drm_do_probe_acpi_edid() - get EDID information via ACPI _DDC
> + * @data: struct drm_device
> + * @buf: EDID data buffer to be filled
> + * @block: 128 byte EDID block to start fetching from
> + * @len: EDID data buffer length to fetch
> + *
> + * Try to fetch EDID information by calling acpi_video_get_edid() function.
> + *
> + * Return: 0 on success or error code on failure.
> + */
> +static int
> +drm_do_probe_acpi_edid(void *data, u8 *buf, unsigned int block, size_t len)
> +{
> +	struct drm_device *ddev = data;
> +	struct acpi_device *acpidev = ACPI_COMPANION(ddev->dev);
> +	unsigned char start = block * EDID_LENGTH;
> +	void *edid;
> +	int r;
> +
> +	if (!acpidev)
> +		return -ENODEV;
> +
> +	/* fetch the entire edid from BIOS */
> +	r = acpi_video_get_edid(acpidev, ACPI_VIDEO_DISPLAY_LCD, -1, &edid);
> +	if (r < 0) {
> +		DRM_DEBUG_KMS("Failed to get EDID from ACPI: %d\n", r);
> +		return -EINVAL;
> +	}
> +	if (len > r || start > r || start + len > r) {
> +		r = EINVAL;
-EINVAL ?
> +		goto cleanup;
> +	}
> +
> +	memcpy(buf, edid + start, len);
> +	r = 0;
New line before goto label?
> +cleanup:
> +	kfree(edid);
> +	return r;
> +}
> +
>   static void connector_bad_edid(struct drm_connector *connector,
>   			       const struct edid *edid, int num_blocks)
>   {
> @@ -2643,6 +2685,37 @@ struct edid *drm_get_edid(struct drm_connector *connector,
>   }
>   EXPORT_SYMBOL(drm_get_edid);
>   
> +/**
> + * drm_get_acpi_edid - get EDID data, if available
> + * @connector: connector we're probing
> + *
> + * Use the BIOS to attempt to grab EDID data if possible.  If found,
> + * attach it to the connector.
> + *
> + * Return: Pointer to valid EDID or NULL if we couldn't find any.
> + */
> +struct edid *drm_get_acpi_edid(struct drm_connector *connector)
> +{
> +	struct edid *edid = NULL;
> +
> +	switch (connector->connector_type) {
> +	case DRM_MODE_CONNECTOR_LVDS:
> +	case DRM_MODE_CONNECTOR_eDP:
> +		break;
> +	default:
> +		return NULL;
> +	}
> +
> +	if (connector->force == DRM_FORCE_OFF)
> +		return NULL;
> +
> +	edid = _drm_do_get_edid(connector, drm_do_probe_acpi_edid, connector->dev, NULL);
> +
> +	drm_connector_update_edid_property(connector, edid);
> +	return edid;
> +}
> +EXPORT_SYMBOL(drm_get_acpi_edid);
> +
>   /**
>    * drm_edid_read_custom - Read EDID data using given EDID block read function
>    * @connector: Connector to use
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 518d1b8106c7..60fbdc06badc 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -412,6 +412,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>   	void *data);
>   struct edid *drm_get_edid(struct drm_connector *connector,
>   			  struct i2c_adapter *adapter);
> +struct edid *drm_get_acpi_edid(struct drm_connector *connector);
>   u32 drm_edid_get_panel_id(struct i2c_adapter *adapter);
>   struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
>   				     struct i2c_adapter *adapter);

New line before return ?

Re: [PATCH v3 2/5] drm: Add drm_get_acpi_edid() helper

[Jani Nikula]

On Thu, 01 Feb 2024, Mario Limonciello <mario.limonciello@amd.com> wrote:
> Some manufacturers have intentionally put an EDID that differs from
> the EDID on the internal panel on laptops.  Drivers can call this
> helper to attempt to fetch the EDID from the BIOS's ACPI _DDC method.

I'm really not happy about adding new struct edid based APIs to
drm_edid.[ch]. Everything new should be struct drm_edid based. All
drivers should be converting towards struct drm_edid, instead of adding
more legacy to rip out later.

BR,
Jani.

>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v1->v2:
>  * Split code from previous amdgpu specific helper to generic drm helper.
> v2->v3:
>  * Add an extra select to fix a variety of randconfig errors found from
>    LKP robot.
> ---
>  drivers/gpu/drm/Kconfig    |  5 +++
>  drivers/gpu/drm/drm_edid.c | 73 ++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_edid.h     |  1 +
>  3 files changed, 79 insertions(+)
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 2520db0b776e..14df907c96c8 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -21,6 +21,11 @@ menuconfig DRM
>  	select KCMP
>  	select VIDEO_CMDLINE
>  	select VIDEO_NOMODESET
> +	select ACPI_VIDEO if ACPI
> +	select BACKLIGHT_CLASS_DEVICE if ACPI
> +	select INPUT if ACPI
> +	select X86_PLATFORM_DEVICES if ACPI && X86
> +	select ACPI_WMI if ACPI && X86
>  	help
>  	  Kernel-level support for the Direct Rendering Infrastructure (DRI)
>  	  introduced in XFree86 4.0. If you say Y here, you need to select
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 69c68804023f..1fbbeaa664b2 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -28,6 +28,7 @@
>   * DEALINGS IN THE SOFTWARE.
>   */
>  
> +#include <acpi/video.h>
>  #include <linux/bitfield.h>
>  #include <linux/cec.h>
>  #include <linux/hdmi.h>
> @@ -2188,6 +2189,47 @@ drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned int block, size_t len)
>  	return ret == xfers ? 0 : -1;
>  }
>  
> +/**
> + * drm_do_probe_acpi_edid() - get EDID information via ACPI _DDC
> + * @data: struct drm_device
> + * @buf: EDID data buffer to be filled
> + * @block: 128 byte EDID block to start fetching from
> + * @len: EDID data buffer length to fetch
> + *
> + * Try to fetch EDID information by calling acpi_video_get_edid() function.
> + *
> + * Return: 0 on success or error code on failure.
> + */
> +static int
> +drm_do_probe_acpi_edid(void *data, u8 *buf, unsigned int block, size_t len)
> +{
> +	struct drm_device *ddev = data;
> +	struct acpi_device *acpidev = ACPI_COMPANION(ddev->dev);
> +	unsigned char start = block * EDID_LENGTH;
> +	void *edid;
> +	int r;
> +
> +	if (!acpidev)
> +		return -ENODEV;
> +
> +	/* fetch the entire edid from BIOS */
> +	r = acpi_video_get_edid(acpidev, ACPI_VIDEO_DISPLAY_LCD, -1, &edid);
> +	if (r < 0) {
> +		DRM_DEBUG_KMS("Failed to get EDID from ACPI: %d\n", r);
> +		return -EINVAL;
> +	}
> +	if (len > r || start > r || start + len > r) {
> +		r = EINVAL;
> +		goto cleanup;
> +	}
> +
> +	memcpy(buf, edid + start, len);
> +	r = 0;
> +cleanup:
> +	kfree(edid);
> +	return r;
> +}
> +
>  static void connector_bad_edid(struct drm_connector *connector,
>  			       const struct edid *edid, int num_blocks)
>  {
> @@ -2643,6 +2685,37 @@ struct edid *drm_get_edid(struct drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_get_edid);
>  
> +/**
> + * drm_get_acpi_edid - get EDID data, if available
> + * @connector: connector we're probing
> + *
> + * Use the BIOS to attempt to grab EDID data if possible.  If found,
> + * attach it to the connector.
> + *
> + * Return: Pointer to valid EDID or NULL if we couldn't find any.
> + */
> +struct edid *drm_get_acpi_edid(struct drm_connector *connector)
> +{
> +	struct edid *edid = NULL;
> +
> +	switch (connector->connector_type) {
> +	case DRM_MODE_CONNECTOR_LVDS:
> +	case DRM_MODE_CONNECTOR_eDP:
> +		break;
> +	default:
> +		return NULL;
> +	}
> +
> +	if (connector->force == DRM_FORCE_OFF)
> +		return NULL;
> +
> +	edid = _drm_do_get_edid(connector, drm_do_probe_acpi_edid, connector->dev, NULL);
> +
> +	drm_connector_update_edid_property(connector, edid);
> +	return edid;
> +}
> +EXPORT_SYMBOL(drm_get_acpi_edid);
> +
>  /**
>   * drm_edid_read_custom - Read EDID data using given EDID block read function
>   * @connector: Connector to use
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 518d1b8106c7..60fbdc06badc 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -412,6 +412,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>  	void *data);
>  struct edid *drm_get_edid(struct drm_connector *connector,
>  			  struct i2c_adapter *adapter);
> +struct edid *drm_get_acpi_edid(struct drm_connector *connector);
>  u32 drm_edid_get_panel_id(struct i2c_adapter *adapter);
>  struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
>  				     struct i2c_adapter *adapter);

-- 
Jani Nikula, Intel

Re: [PATCH v3 2/5] drm: Add drm_get_acpi_edid() helper

[Mario Limonciello]

On 2/2/2024 04:29, Jani Nikula wrote:
> On Thu, 01 Feb 2024, Mario Limonciello <mario.limonciello@amd.com> wrote:
>> Some manufacturers have intentionally put an EDID that differs from
>> the EDID on the internal panel on laptops.  Drivers can call this
>> helper to attempt to fetch the EDID from the BIOS's ACPI _DDC method.
> 
> I'm really not happy about adding new struct edid based APIs to
> drm_edid.[ch]. Everything new should be struct drm_edid based. All
> drivers should be converting towards struct drm_edid, instead of adding
> more legacy to rip out later.

OK; I'll redo it with struct drm_edid.

The changeover for amdgpu to use drm_edid is going to be a pretty 
involved effort so I'm going to use a get_raw in amdgpu for now so we 
can unblock the issue this is fixing and let that part get removed when 
the rest of the overhaul gets done there.

> 
> BR,
> Jani.
> 
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>> v1->v2:
>>   * Split code from previous amdgpu specific helper to generic drm helper.
>> v2->v3:
>>   * Add an extra select to fix a variety of randconfig errors found from
>>     LKP robot.
>> ---
>>   drivers/gpu/drm/Kconfig    |  5 +++
>>   drivers/gpu/drm/drm_edid.c | 73 ++++++++++++++++++++++++++++++++++++++
>>   include/drm/drm_edid.h     |  1 +
>>   3 files changed, 79 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index 2520db0b776e..14df907c96c8 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -21,6 +21,11 @@ menuconfig DRM
>>   	select KCMP
>>   	select VIDEO_CMDLINE
>>   	select VIDEO_NOMODESET
>> +	select ACPI_VIDEO if ACPI
>> +	select BACKLIGHT_CLASS_DEVICE if ACPI
>> +	select INPUT if ACPI
>> +	select X86_PLATFORM_DEVICES if ACPI && X86
>> +	select ACPI_WMI if ACPI && X86
>>   	help
>>   	  Kernel-level support for the Direct Rendering Infrastructure (DRI)
>>   	  introduced in XFree86 4.0. If you say Y here, you need to select
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 69c68804023f..1fbbeaa664b2 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -28,6 +28,7 @@
>>    * DEALINGS IN THE SOFTWARE.
>>    */
>>   
>> +#include <acpi/video.h>
>>   #include <linux/bitfield.h>
>>   #include <linux/cec.h>
>>   #include <linux/hdmi.h>
>> @@ -2188,6 +2189,47 @@ drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned int block, size_t len)
>>   	return ret == xfers ? 0 : -1;
>>   }
>>   
>> +/**
>> + * drm_do_probe_acpi_edid() - get EDID information via ACPI _DDC
>> + * @data: struct drm_device
>> + * @buf: EDID data buffer to be filled
>> + * @block: 128 byte EDID block to start fetching from
>> + * @len: EDID data buffer length to fetch
>> + *
>> + * Try to fetch EDID information by calling acpi_video_get_edid() function.
>> + *
>> + * Return: 0 on success or error code on failure.
>> + */
>> +static int
>> +drm_do_probe_acpi_edid(void *data, u8 *buf, unsigned int block, size_t len)
>> +{
>> +	struct drm_device *ddev = data;
>> +	struct acpi_device *acpidev = ACPI_COMPANION(ddev->dev);
>> +	unsigned char start = block * EDID_LENGTH;
>> +	void *edid;
>> +	int r;
>> +
>> +	if (!acpidev)
>> +		return -ENODEV;
>> +
>> +	/* fetch the entire edid from BIOS */
>> +	r = acpi_video_get_edid(acpidev, ACPI_VIDEO_DISPLAY_LCD, -1, &edid);
>> +	if (r < 0) {
>> +		DRM_DEBUG_KMS("Failed to get EDID from ACPI: %d\n", r);
>> +		return -EINVAL;
>> +	}
>> +	if (len > r || start > r || start + len > r) {
>> +		r = EINVAL;
>> +		goto cleanup;
>> +	}
>> +
>> +	memcpy(buf, edid + start, len);
>> +	r = 0;
>> +cleanup:
>> +	kfree(edid);
>> +	return r;
>> +}
>> +
>>   static void connector_bad_edid(struct drm_connector *connector,
>>   			       const struct edid *edid, int num_blocks)
>>   {
>> @@ -2643,6 +2685,37 @@ struct edid *drm_get_edid(struct drm_connector *connector,
>>   }
>>   EXPORT_SYMBOL(drm_get_edid);
>>   
>> +/**
>> + * drm_get_acpi_edid - get EDID data, if available
>> + * @connector: connector we're probing
>> + *
>> + * Use the BIOS to attempt to grab EDID data if possible.  If found,
>> + * attach it to the connector.
>> + *
>> + * Return: Pointer to valid EDID or NULL if we couldn't find any.
>> + */
>> +struct edid *drm_get_acpi_edid(struct drm_connector *connector)
>> +{
>> +	struct edid *edid = NULL;
>> +
>> +	switch (connector->connector_type) {
>> +	case DRM_MODE_CONNECTOR_LVDS:
>> +	case DRM_MODE_CONNECTOR_eDP:
>> +		break;
>> +	default:
>> +		return NULL;
>> +	}
>> +
>> +	if (connector->force == DRM_FORCE_OFF)
>> +		return NULL;
>> +
>> +	edid = _drm_do_get_edid(connector, drm_do_probe_acpi_edid, connector->dev, NULL);
>> +
>> +	drm_connector_update_edid_property(connector, edid);
>> +	return edid;
>> +}
>> +EXPORT_SYMBOL(drm_get_acpi_edid);
>> +
>>   /**
>>    * drm_edid_read_custom - Read EDID data using given EDID block read function
>>    * @connector: Connector to use
>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>> index 518d1b8106c7..60fbdc06badc 100644
>> --- a/include/drm/drm_edid.h
>> +++ b/include/drm/drm_edid.h
>> @@ -412,6 +412,7 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
>>   	void *data);
>>   struct edid *drm_get_edid(struct drm_connector *connector,
>>   			  struct i2c_adapter *adapter);
>> +struct edid *drm_get_acpi_edid(struct drm_connector *connector);
>>   u32 drm_edid_get_panel_id(struct i2c_adapter *adapter);
>>   struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
>>   				     struct i2c_adapter *adapter);
> 

Re: [PATCH v3 1/5] ACPI: video: Handle fetching EDID that is longer than 256 bytes

[Rafael J. Wysocki]

On Thu, Feb 1, 2024 at 11:11 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> The ACPI specification allows for an EDID to be up to 512 bytes but
> the _DDC EDID fetching code will only try up to 256 bytes.
>
> Modify the code to instead start at 512 bytes and work it's way
> down instead.
>
> As _DDC is now called up to 4 times on a machine debugging messages
> are noisier than necessary.  Decrease from info to debug.
>
> Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/Apx_B_Video_Extensions/output-device-specific-methods.html#ddc-return-the-edid-for-this-device
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

or I can apply it if that's preferred.

Thanks!

> ---
> v1->v2:
>  * Use for loop for acpi_video_get_edid()
>  * Use one of Rafael's suggestions for acpi_video_device_EDID()
>  * Decrease message level too
> ---
>  drivers/acpi/acpi_video.c | 25 +++++++++----------------
>  1 file changed, 9 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
> index 4afdda9db019..3bfd013e09d2 100644
> --- a/drivers/acpi/acpi_video.c
> +++ b/drivers/acpi/acpi_video.c
> @@ -625,12 +625,9 @@ acpi_video_device_EDID(struct acpi_video_device *device,
>
>         if (!device)
>                 return -ENODEV;
> -       if (length == 128)
> -               arg0.integer.value = 1;
> -       else if (length == 256)
> -               arg0.integer.value = 2;
> -       else
> +       if (!length || (length % 128))
>                 return -EINVAL;
> +       arg0.integer.value = length / 128;
>
>         status = acpi_evaluate_object(device->dev->handle, "_DDC", &args, &buffer);
>         if (ACPI_FAILURE(status))
> @@ -641,7 +638,8 @@ acpi_video_device_EDID(struct acpi_video_device *device,
>         if (obj && obj->type == ACPI_TYPE_BUFFER)
>                 *edid = obj;
>         else {
> -               acpi_handle_info(device->dev->handle, "Invalid _DDC data\n");
> +               acpi_handle_debug(device->dev->handle,
> +                                "Invalid _DDC data for length %ld\n", length);
>                 status = -EFAULT;
>                 kfree(obj);
>         }
> @@ -1447,7 +1445,6 @@ int acpi_video_get_edid(struct acpi_device *device, int type, int device_id,
>
>         for (i = 0; i < video->attached_count; i++) {
>                 video_device = video->attached_array[i].bind_info;
> -               length = 256;
>
>                 if (!video_device)
>                         continue;
> @@ -1478,18 +1475,14 @@ int acpi_video_get_edid(struct acpi_device *device, int type, int device_id,
>                         continue;
>                 }
>
> -               status = acpi_video_device_EDID(video_device, &buffer, length);
> -
> -               if (ACPI_FAILURE(status) || !buffer ||
> -                   buffer->type != ACPI_TYPE_BUFFER) {
> -                       length = 128;
> +               for (length = 512; length > 0; length -= 128) {
>                         status = acpi_video_device_EDID(video_device, &buffer,
>                                                         length);
> -                       if (ACPI_FAILURE(status) || !buffer ||
> -                           buffer->type != ACPI_TYPE_BUFFER) {
> -                               continue;
> -                       }
> +                       if (ACPI_SUCCESS(status))
> +                               break;
>                 }
> +               if (!length)
> +                       continue;
>
>                 *edid = buffer->buffer.pointer;
>                 return length;
> --
> 2.34.1
>

Re: [PATCH v3 1/5] ACPI: video: Handle fetching EDID that is longer than 256 bytes

[Mario Limonciello]

On 2/2/2024 10:07, Rafael J. Wysocki wrote:
> On Thu, Feb 1, 2024 at 11:11 PM Mario Limonciello
> <mario.limonciello@amd.com> wrote:
>>
>> The ACPI specification allows for an EDID to be up to 512 bytes but
>> the _DDC EDID fetching code will only try up to 256 bytes.
>>
>> Modify the code to instead start at 512 bytes and work it's way
>> down instead.
>>
>> As _DDC is now called up to 4 times on a machine debugging messages
>> are noisier than necessary.  Decrease from info to debug.
>>
>> Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/Apx_B_Video_Extensions/output-device-specific-methods.html#ddc-return-the-edid-for-this-device
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> 
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> or I can apply it if that's preferred.

Thanks!

I think go ahead and apply this one to your -next tree.

The rest might take a few weeks to get right and no need to block on this.

> 
> Thanks!
> 
>> ---
>> v1->v2:
>>   * Use for loop for acpi_video_get_edid()
>>   * Use one of Rafael's suggestions for acpi_video_device_EDID()
>>   * Decrease message level too
>> ---
>>   drivers/acpi/acpi_video.c | 25 +++++++++----------------
>>   1 file changed, 9 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
>> index 4afdda9db019..3bfd013e09d2 100644
>> --- a/drivers/acpi/acpi_video.c
>> +++ b/drivers/acpi/acpi_video.c
>> @@ -625,12 +625,9 @@ acpi_video_device_EDID(struct acpi_video_device *device,
>>
>>          if (!device)
>>                  return -ENODEV;
>> -       if (length == 128)
>> -               arg0.integer.value = 1;
>> -       else if (length == 256)
>> -               arg0.integer.value = 2;
>> -       else
>> +       if (!length || (length % 128))
>>                  return -EINVAL;
>> +       arg0.integer.value = length / 128;
>>
>>          status = acpi_evaluate_object(device->dev->handle, "_DDC", &args, &buffer);
>>          if (ACPI_FAILURE(status))
>> @@ -641,7 +638,8 @@ acpi_video_device_EDID(struct acpi_video_device *device,
>>          if (obj && obj->type == ACPI_TYPE_BUFFER)
>>                  *edid = obj;
>>          else {
>> -               acpi_handle_info(device->dev->handle, "Invalid _DDC data\n");
>> +               acpi_handle_debug(device->dev->handle,
>> +                                "Invalid _DDC data for length %ld\n", length);
>>                  status = -EFAULT;
>>                  kfree(obj);
>>          }
>> @@ -1447,7 +1445,6 @@ int acpi_video_get_edid(struct acpi_device *device, int type, int device_id,
>>
>>          for (i = 0; i < video->attached_count; i++) {
>>                  video_device = video->attached_array[i].bind_info;
>> -               length = 256;
>>
>>                  if (!video_device)
>>                          continue;
>> @@ -1478,18 +1475,14 @@ int acpi_video_get_edid(struct acpi_device *device, int type, int device_id,
>>                          continue;
>>                  }
>>
>> -               status = acpi_video_device_EDID(video_device, &buffer, length);
>> -
>> -               if (ACPI_FAILURE(status) || !buffer ||
>> -                   buffer->type != ACPI_TYPE_BUFFER) {
>> -                       length = 128;
>> +               for (length = 512; length > 0; length -= 128) {
>>                          status = acpi_video_device_EDID(video_device, &buffer,
>>                                                          length);
>> -                       if (ACPI_FAILURE(status) || !buffer ||
>> -                           buffer->type != ACPI_TYPE_BUFFER) {
>> -                               continue;
>> -                       }
>> +                       if (ACPI_SUCCESS(status))
>> +                               break;
>>                  }
>> +               if (!length)
>> +                       continue;
>>
>>                  *edid = buffer->buffer.pointer;
>>                  return length;
>> --
>> 2.34.1
>>

Re: [PATCH v3 3/5] drm/amd: Fetch the EDID from _DDC if available for eDP

[kernel test robot]

Hi Mario,

kernel test robot noticed the following build warnings:

[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on drm-intel/for-linux-next-fixes drm-xe/drm-xe-next linus/master v6.8-rc2 next-20240202]
[cannot apply to drm-misc/drm-misc-next drm-intel/for-linux-next rafael-pm/acpi-bus rafael-pm/devprop]
[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/Mario-Limonciello/ACPI-video-Handle-fetching-EDID-that-is-longer-than-256-bytes/20240202-061301
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/20240201221119.42564-4-mario.limonciello%40amd.com
patch subject: [PATCH v3 3/5] drm/amd: Fetch the EDID from _DDC if available for eDP
config: i386-randconfig-004-20240203 (https://download.01.org/0day-ci/archive/20240203/202402032030.IBEf5Cme-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240203/202402032030.IBEf5Cme-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/202402032030.IBEf5Cme-lkp@intel.com/

All warnings (new ones prefixed by >>):

   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:6599:5: warning: 'edid' is used uninitialized in this function [-Wuninitialized]
    6599 |  if (!edid)
         |     ^


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

  6582	
  6583	static void create_eml_sink(struct amdgpu_dm_connector *aconnector)
  6584	{
  6585		struct drm_connector *connector = &aconnector->base;
  6586		struct amdgpu_connector *amdgpu_connector = to_amdgpu_connector(&aconnector->base);
  6587		struct dc_sink_init_data init_params = {
  6588				.link = aconnector->dc_link,
  6589				.sink_signal = SIGNAL_TYPE_VIRTUAL
  6590		};
  6591		struct edid *edid;
  6592	
  6593		/*
  6594		 * Note: drm_get_edid gets edid in the following order:
  6595		 * 1) override EDID if set via edid_override debugfs,
  6596		 * 2) firmware EDID if set via edid_firmware module parameter
  6597		 * 3) regular DDC read.
  6598		 */
> 6599		if (!edid)
  6600			edid = drm_get_edid(connector, &amdgpu_connector->ddc_bus->aux.ddc);
  6601	
  6602		if (!edid) {
  6603			DRM_ERROR("No EDID found on connector: %s.\n", connector->name);
  6604			return;
  6605		}
  6606	
  6607		if (drm_detect_hdmi_monitor(edid))
  6608			init_params.sink_signal = SIGNAL_TYPE_HDMI_TYPE_A;
  6609	
  6610		aconnector->edid = edid;
  6611	
  6612		aconnector->dc_em_sink = dc_link_add_remote_sink(
  6613			aconnector->dc_link,
  6614			(uint8_t *)edid,
  6615			(edid->extensions + 1) * EDID_LENGTH,
  6616			&init_params);
  6617	
  6618		if (aconnector->base.force == DRM_FORCE_ON) {
  6619			aconnector->dc_sink = aconnector->dc_link->local_sink ?
  6620			aconnector->dc_link->local_sink :
  6621			aconnector->dc_em_sink;
  6622			dc_sink_retain(aconnector->dc_sink);
  6623		}
  6624	}
  6625	

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

Re: [PATCH v3 1/5] ACPI: video: Handle fetching EDID that is longer than 256 bytes

[Rafael J. Wysocki]

On Fri, Feb 2, 2024 at 5:09 PM Mario Limonciello
<mario.limonciello@amd.com> wrote:
>
> On 2/2/2024 10:07, Rafael J. Wysocki wrote:
> > On Thu, Feb 1, 2024 at 11:11 PM Mario Limonciello
> > <mario.limonciello@amd.com> wrote:
> >>
> >> The ACPI specification allows for an EDID to be up to 512 bytes but
> >> the _DDC EDID fetching code will only try up to 256 bytes.
> >>
> >> Modify the code to instead start at 512 bytes and work it's way
> >> down instead.
> >>
> >> As _DDC is now called up to 4 times on a machine debugging messages
> >> are noisier than necessary.  Decrease from info to debug.
> >>
> >> Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/Apx_B_Video_Extensions/output-device-specific-methods.html#ddc-return-the-edid-for-this-device
> >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> >
> > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > or I can apply it if that's preferred.
>
> Thanks!
>
> I think go ahead and apply this one to your -next tree.

Applied now.

Barring any issues with it, It will get into linux-next in a couple of days.

Thanks!