Nouveau - Jul 2020 - [PATCH 0/4] drm: add support for retrieving EDID via ACPI _DDC

Some notebook systems provide the EDID for the internal panel via the
_DDC method in ACPI, instead of or in addition to providing the EDID via
DDC on LVDS/eDP. Add a DRM helper to search for an ACP _DDC method under
the ACPI namespace for each VGA/3D controller, and return the first EDID
successfully retrieved via _DDC. Update the i915, nouveau, and radeon
DRM-KMS drivers to fall back to retrieving the EDID via ACPI _DDC on
notebook internal display panels after failing to retrieve an EDID via
other means.

This is useful for retrieving an internal panel's EDID both on hybrid
graphics systems with muxed display output, when the display is muxed
away, as well as on a small number of non-muxed and/or non-hybrid
systems where ACPI _DDC is the only means of accessing the EDID for
the internal panel.

Daniel Dadap (4):
  drm: retrieve EDID via ACPI _DDC method
  i915: fall back to ACPI EDID retrieval
  nouveau: fall back to ACPI EDID retrieval
  radeon: fall back to ACPI EDID retrieval

 drivers/gpu/drm/drm_edid.c                  | 161 ++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_dp.c     |   8 +-
 drivers/gpu/drm/i915/display/intel_lvds.c   |   4 +
 drivers/gpu/drm/nouveau/nouveau_connector.c |   6 +
 drivers/gpu/drm/radeon/radeon_combios.c     |   6 +-
 include/drm/drm_edid.h                      |   1 +
 6 files changed, 182 insertions(+), 4 deletions(-)

-- 
2.18.4

Some notebook computer systems expose the EDID for the internal
panel via the ACPI _DDC method. On some systems this is because
the panel does not populate the hardware DDC lines, and on some
systems with dynamic display muxes, _DDC is implemented to allow
the internal panel's EDID to be read at any time, regardless of
how the mux is switched.

The _DDC method can be implemented for each individual display
output, so there could be an arbitrary number of outputs exposing
their EDIDs via _DDC; however, in practice, this has only been
implemented so far on systems with a single panel, so the current
implementation of drm_get_edid_acpi() walks the outputs listed by
each GPU's ACPI _DOD method and returns the first EDID successfully
retrieved by any attached _DDC method. It may be necessary in the
future to allow for the retrieval of distinct EDIDs for different
output devices, but in order to do so, it will first be necessary
to develop a way to correlate individual DRM outputs with their
corresponding entities in ACPI.

Signed-off-by: Daniel Dadap <ddadap at nvidia.com>
---
 drivers/gpu/drm/drm_edid.c | 161 +++++++++++++++++++++++++++++++++++++
 include/drm/drm_edid.h     |   1 +
 2 files changed, 162 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 116451101426..f66d6bf048c6 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -34,6 +34,7 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/vga_switcheroo.h>
+#include <linux/pci.h>
 
 #include <drm/drm_displayid.h>
 #include <drm/drm_drv.h>
@@ -2058,6 +2059,166 @@ struct edid *drm_get_edid_switcheroo(struct
drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_get_edid_switcheroo);
 
+#if defined(CONFIG_ACPI) && defined(CONFIG_PCI)
+static u64 *get_dod_entries(acpi_handle handle, int *count)
+{
+	acpi_status status;
+	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *dod;
+	int i;
+	u64 *ret = NULL;
+
+	*count = 0;
+
+	status = acpi_evaluate_object(handle, "_DOD", NULL, &buf);
+
+	if (ACPI_FAILURE(status))
+		return NULL;
+
+	dod = buf.pointer;
+
+	if (dod == NULL || dod->type != ACPI_TYPE_PACKAGE)
+		goto done;
+
+	ret = kmalloc_array(dod->package.count, sizeof(*ret), GFP_KERNEL);
+	if (ret == NULL)
+		goto done;
+
+	for (i = 0; i < dod->package.count; i++) {
+		if (dod->package.elements[i].type != ACPI_TYPE_INTEGER)
+			continue;
+		ret[*count] = dod->package.elements[i].integer.value;
+		(*count)++;
+	}
+
+done:
+	kfree(buf.pointer);
+	return ret;
+}
+
+static void *do_acpi_ddc(acpi_handle handle)
+{
+	int i;
+	void *ret = NULL;
+
+	/*
+	 * The _DDC spec defines an integer argument for specifying the size of
+	 * the EDID to be retrieved. A value of 1 requests a 128-byte EDID, and
+	 * a value of 2 requests a 256-byte EDID. Attempt the larger read first.
+	 */
+	for (i = 2; i >= 1; i--) {
+		struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL };
+		union acpi_object arg = { ACPI_TYPE_INTEGER };
+		struct acpi_object_list in = { 1, &arg };
+		union acpi_object *edid;
+		acpi_status status;
+
+		arg.integer.value = i;
+		status = acpi_evaluate_object(handle, "_DDC", &in, &out);
+		edid = out.pointer;
+
+		if (ACPI_SUCCESS(status))
+			ret = edid->buffer.pointer;
+
+		kfree(edid);
+
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+
+static struct edid *first_edid_from_acpi_ddc(struct pci_dev *pdev)
+{
+	acpi_handle handle;
+	acpi_status status;
+	struct acpi_device *device = NULL;
+	struct edid *ret = NULL;
+	int num_dod_entries;
+	u64 *dod_entries = NULL;
+	struct list_head *node, *next;
+
+	handle = ACPI_HANDLE(&pdev->dev);
+	if (handle == NULL)
+		return NULL;
+
+	dod_entries = get_dod_entries(handle, &num_dod_entries);
+	if (dod_entries == NULL || num_dod_entries == 0)
+		goto done;
+
+	status = acpi_bus_get_device(handle, &device);
+	if (ACPI_FAILURE(status) || device == NULL)
+		goto done;
+
+	list_for_each_safe(node, next, &device->children) {
+		struct acpi_device *child;
+		u64 adr;
+		int i;
+
+		child = list_entry(node, struct acpi_device, node);
+		if (child == NULL)
+			continue;
+
+		status = acpi_evaluate_integer(child->handle, "_ADR", NULL,
+			&adr);
+		if (ACPI_FAILURE(status))
+			continue;
+
+		for (i = 0; i < num_dod_entries; i++) {
+			if (adr == dod_entries[i]) {
+				ret = do_acpi_ddc(child->handle);
+
+				if (ret != NULL)
+					goto done;
+			}
+		}
+	}
+done:
+	kfree(dod_entries);
+	return ret;
+}
+
+static struct edid *search_pci_class_for_acpi_ddc(unsigned int class)
+{
+	struct pci_dev *dev = NULL;
+
+	while ((dev = pci_get_class(class << 8, dev))) {
+		struct edid *edid = first_edid_from_acpi_ddc(dev);
+
+		if (edid)
+			return edid;
+	}
+
+	return NULL;
+}
+#endif
+
+/**
+ * drm_get_edid_acpi() - retrieve an EDID via the ACPI _DDC method
+ *
+ * Iterate over the ACPI namespace objects for all PCI VGA/3D controllers
+ * and attempt to evaluate any present _DDC method handles, returning the
+ * first successfully found EDID, or %NULL if none was found.
+ */
+struct edid *drm_get_edid_acpi(void)
+{
+#if defined(CONFIG_ACPI) && defined(CONFIG_PCI)
+	struct edid *edid;
+
+	edid = search_pci_class_for_acpi_ddc(PCI_CLASS_DISPLAY_VGA);
+	if (edid)
+		return edid;
+
+	edid = search_pci_class_for_acpi_ddc(PCI_CLASS_DISPLAY_3D);
+	if (edid)
+		return edid;
+#endif
+
+	return NULL;
+}
+EXPORT_SYMBOL(drm_get_edid_acpi);
+
 /**
  * drm_edid_duplicate - duplicate an EDID and the extensions
  * @edid: EDID to duplicate
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 34b15e3d070c..ec2fe6d98560 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -485,6 +485,7 @@ struct edid *drm_get_edid(struct drm_connector *connector,
 			  struct i2c_adapter *adapter);
 struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
 				     struct i2c_adapter *adapter);
+struct edid *drm_get_edid_acpi(void);
 struct edid *drm_edid_duplicate(const struct edid *edid);
 int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid);
 int drm_add_override_edid_modes(struct drm_connector *connector);
-- 
2.18.4

Fall back to retrieving the EDID via the ACPI _DDC method, when present
for notebook internal panels, when EDID retrieval via the standard EDID
paths is unsuccessful.

Signed-off-by: Daniel Dadap <ddadap at nvidia.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c   | 8 +++++++-
 drivers/gpu/drm/i915/display/intel_lvds.c | 4 ++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
b/drivers/gpu/drm/i915/display/intel_dp.c
index 804b1d966f66..ff402cef8183 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -5657,6 +5657,7 @@ static struct edid *
 intel_dp_get_edid(struct intel_dp *intel_dp)
 {
 	struct intel_connector *intel_connector = intel_dp->attached_connector;
+	struct edid *edid;
 
 	/* use cached edid if we have one */
 	if (intel_connector->edid) {
@@ -5666,8 +5667,13 @@ intel_dp_get_edid(struct intel_dp *intel_dp)
 
 		return drm_edid_duplicate(intel_connector->edid);
 	} else
-		return drm_get_edid(&intel_connector->base,
+		edid = drm_get_edid(&intel_connector->base,
 				    &intel_dp->aux.ddc);
+
+	if (!edid && intel_dp_is_edp(intel_dp))
+		edid = drm_get_edid_acpi();
+
+	return edid;
 }
 
 static void
diff --git a/drivers/gpu/drm/i915/display/intel_lvds.c
b/drivers/gpu/drm/i915/display/intel_lvds.c
index 9a067effcfa0..811eea3f5d9f 100644
--- a/drivers/gpu/drm/i915/display/intel_lvds.c
+++ b/drivers/gpu/drm/i915/display/intel_lvds.c
@@ -946,6 +946,10 @@ void intel_lvds_init(struct drm_i915_private *dev_priv)
 	else
 		edid = drm_get_edid(connector,
 				    intel_gmbus_get_adapter(dev_priv, pin));
+
+	if (!edid)
+		edid = drm_get_edid_acpi();
+
 	if (edid) {
 		if (drm_add_edid_modes(connector, edid)) {
 			drm_connector_update_edid_property(connector,
-- 
2.18.4

Fall back to retrieving the EDID via the ACPI _DDC method, when present
for notebook internal panels, when EDID retrieval via the standard EDID
paths is unsuccessful.

Signed-off-by: Daniel Dadap <ddadap at nvidia.com>
---
 drivers/gpu/drm/nouveau/nouveau_connector.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c
b/drivers/gpu/drm/nouveau/nouveau_connector.c
index 9a9a7f5003d3..95836a02a06b 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -581,6 +581,12 @@ nouveau_connector_detect(struct drm_connector *connector,
bool force)
 		else
 			nv_connector->edid = drm_get_edid(connector, i2c);
 
+		if (!nv_connector->edid &&
+			(nv_connector->type == DCB_CONNECTOR_LVDS ||
+			nv_connector->type == DCB_CONNECTOR_eDP)) {
+			nv_connector->edid = drm_get_edid_acpi();
+		}
+
 		drm_connector_update_edid_property(connector,
 							nv_connector->edid);
 		if (!nv_connector->edid) {
-- 
2.18.4

Fall back to retrieving the EDID via the ACPI _DDC method, when present
for notebook internal panels, when retrieving BIOS-embedded EDIDs.

Signed-off-by: Daniel Dadap <ddadap at nvidia.com>
---
 drivers/gpu/drm/radeon/radeon_combios.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_combios.c
b/drivers/gpu/drm/radeon/radeon_combios.c
index c3e49c973812..de801d9fca54 100644
--- a/drivers/gpu/drm/radeon/radeon_combios.c
+++ b/drivers/gpu/drm/radeon/radeon_combios.c
@@ -401,9 +401,8 @@ bool radeon_combios_check_hardcoded_edid(struct
radeon_device *rdev)
 struct edid *
 radeon_bios_get_hardcoded_edid(struct radeon_device *rdev)
 {
-	struct edid *edid;
-
 	if (rdev->mode_info.bios_hardcoded_edid) {
+		struct edid *edid;
 		edid = kmalloc(rdev->mode_info.bios_hardcoded_edid_size, GFP_KERNEL);
 		if (edid) {
 			memcpy((unsigned char *)edid,
@@ -412,7 +411,8 @@ radeon_bios_get_hardcoded_edid(struct radeon_device *rdev)
 			return edid;
 		}
 	}
-	return NULL;
+
+	return drm_get_edid_acpi();
 }
 
 static struct radeon_i2c_bus_rec combios_setup_i2c_bus(struct radeon_device
*rdev,
-- 
2.18.4

Am 27.07.20 um 22:53 schrieb Daniel Dadap:
> Fall back to retrieving the EDID via the ACPI _DDC method, when present
> for notebook internal panels, when retrieving BIOS-embedded EDIDs.
>
> Signed-off-by: Daniel Dadap <ddadap at nvidia.com>
> ---
>   drivers/gpu/drm/radeon/radeon_combios.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_combios.c
b/drivers/gpu/drm/radeon/radeon_combios.c
> index c3e49c973812..de801d9fca54 100644
> --- a/drivers/gpu/drm/radeon/radeon_combios.c
> +++ b/drivers/gpu/drm/radeon/radeon_combios.c
> @@ -401,9 +401,8 @@ bool radeon_combios_check_hardcoded_edid(struct
radeon_device *rdev)
>   struct edid *
>   radeon_bios_get_hardcoded_edid(struct radeon_device *rdev)
>   {
> -	struct edid *edid;
> -
>   	if (rdev->mode_info.bios_hardcoded_edid) {
> +		struct edid *edid;
That's an unrelated an incorrect style change. You need a blank line 
after declaration.
>   		edid = kmalloc(rdev->mode_info.bios_hardcoded_edid_size,
GFP_KERNEL);
>   		if (edid) {
>   			memcpy((unsigned char *)edid,
> @@ -412,7 +411,8 @@ radeon_bios_get_hardcoded_edid(struct radeon_device
*rdev)
>   			return edid;
>   		}
>   	}
> -	return NULL;
> +
> +	return drm_get_edid_acpi();
In general a good idea, but I'm wondering if we should really do this so 
unconditionally here.

Regards,
Christian.
>   }
>   
>   static struct radeon_i2c_bus_rec combios_setup_i2c_bus(struct
radeon_device *rdev,

On Mon, Jul 27, 2020 at 03:53:54PM -0500, Daniel Dadap
wrote:
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -34,6 +34,7 @@
>  #include <linux/module.h>
>  #include <linux/slab.h>
>  #include <linux/vga_switcheroo.h>
> +#include <linux/pci.h>
Nit: Alphabetic ordering.
> +static u64 *get_dod_entries(acpi_handle handle, int *count)
> +{
> +	acpi_status status;
> +	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
> +	union acpi_object *dod;
> +	int i;
> +	u64 *ret = NULL;
Nits: Reverse christmas tree.
"ret" is a poor name, I'd suggest "entries" or something
like that.
The spec says that _DOD is a list of 32-bit values, not 64-bit.
> +	status = acpi_evaluate_object(handle, "_DOD", NULL, &buf);
> +
> +	if (ACPI_FAILURE(status))
> +		return NULL;
Nit: No blank line between function invocation and error check.
> +	dod = buf.pointer;
> +
> +	if (dod == NULL || dod->type != ACPI_TYPE_PACKAGE)
> +		goto done;
Same.
> +	ret = kmalloc_array(dod->package.count, sizeof(*ret), GFP_KERNEL);
> +	if (ret == NULL)
> +		goto done;
Nit: Usually we use "if (!ret)" or "if (ret)".
> +	list_for_each_safe(node, next, &device->children) {
No, that's not safe because the ACPI namespace may change concurrently,
e.g. because a DSDT patch is applied by the user via sysfs.
Use acpi_walk_namespace() with a depth of 1 instead.
> +		for (i = 0; i < num_dod_entries; i++) {
> +			if (adr == dod_entries[i]) {
> +				ret = do_acpi_ddc(child->handle);
> +
> +				if (ret != NULL)
> +					goto done;
I guess ideally we'd want to correlate the display objects with
drm_connectors or at least constrain the search to Display Type
"Internal/Integrated Digital Flat Panel" instead of picking the
first EDID found.  Otherwise we might erroneously use the DDC
for an externally attached display.
> +struct edid *drm_get_edid_acpi(void)
> +{
> +#if defined(CONFIG_ACPI) && defined(CONFIG_PCI)
No, put an empty inline stub in the header file instead of using #ifdef, see:

https://www.kernel.org/doc/html/latest/process/coding-style.html#conditional-compilation

Patches 2, 3 and 4 need a "drm/" prefix in the Subject, e.g.
"drm/i915: ".

Please cc all ACPI-related patches to linux-acpi.

Thanks,

Lukas