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

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

[Daniel Dadap]

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

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 1/4] drm: retrieve EDID via ACPI _DDC method

[Daniel Dadap]

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

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/4] i915: fall back to ACPI EDID retrieval

[Daniel Dadap]

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

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 3/4] nouveau: fall back to ACPI EDID retrieval

[Daniel Dadap]

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

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 4/4] radeon: fall back to ACPI EDID retrieval

[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@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

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 4/4] radeon: fall back to ACPI EDID retrieval

[Christian König]

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@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,

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 4/4] radeon: fall back to ACPI EDID retrieval

[Daniel Dadap]

On 7/28/20 1:50 AM, Christian König wrote:
>
> 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@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.


Ah, yes, that doesn't really need to be changed. I'll remove it from 
this patch. Would a separate patch to change the scope of that 
declaration (with a blank line after) be welcome, or should I just leave 
it alone?


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

I'm not personally aware of any AMD notebook designs that require the 
ACPI _DDC EDID retrieval. I've only seen it on NVIDIA+Intel hybrid 
systems and on a small number of NVIDIA discrete-only systems. I just 
figured I'd update the radeon DRM-KMS driver while updating i915 and 
Nouveau, for completeness, as it could be helpful should such a design 
exist. As for whether there should be some condition around this, I 
suppose that's reasonable, but I'm not really sure what would make sense 
as a condition. As it stands, drm_edid_acpi() only returns a value if at 
least one of the VGA or 3D controllers on the system provides an ACPI 
_DDC method, and if that ACPI method successfully returns an EDID.

On the caller's end, it's currently part of the path where the radeon 
driver is already trying to fall back to a hardcoded EDID provided by 
the system. Perhaps instead if we call it within the LVDS || eDP 
condition here, instead?


         if (rdev->is_atom_bios) {
             /* some laptops provide a hardcoded edid in rom for LCDs */
             if (((connector->connector_type == DRM_MODE_CONNECTOR_LVDS) ||
                  (connector->connector_type == DRM_MODE_CONNECTOR_eDP)))
                 radeon_connector->edid = 
radeon_bios_get_hardcoded_edid(rdev);
         } else {
             /* some servers provide a hardcoded edid in rom for KVMs */
             radeon_connector->edid = radeon_bios_get_hardcoded_edid(rdev);
}

That would be more in line with the changes in this patchset for i915 
and nouveau.


> Regards,
> Christian.
>
>>   }
>>
>>   static struct radeon_i2c_bus_rec combios_setup_i2c_bus(struct 
>> radeon_device *rdev,
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [Nouveau] [PATCH 1/4] drm: retrieve EDID via ACPI _DDC method

[Lukas Wunner]

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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [Nouveau] [PATCH 1/4] drm: retrieve EDID via ACPI _DDC method

[Daniel Dadap]

Thanks, Lukas. I've incorporated your feedback into my local tree, but 
will wait for additional feedback from the individual DRM driver 
maintainers before sending out a series v2.

On 8/8/20 5:11 PM, Lukas Wunner wrote:
> On Mon, Jul 27, 2020 at 03:53:54PM -0500, Daniel Dadap wrote:
>> +             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.


Yes, we'd definitely need a way to do this if this functionality ever 
needs to be extended to systems with more than one _DDC method. 
Unfortunately, this will be much easier said than done, since I'm not 
aware of any way to reliably do map _DOD entries to connectors in a GPU 
driver, especially when we're talking about possibly correlating 
connectors on multiple GPUs which mux to the same internal display or 
external connector. All systems which I am aware of that implement ACPI 
_DDC do so for a single internal panel. I don't believe there's any 
reason to ever retrieve an EDID via ACPI _DDC for an external panel, but 
a hypothetical design with multiple internal panels, more than one of 
which needs to retrieve an EDID via ACPI _DDC, would certainly be 
problematic.


On at least the system I'm working with for the various switcheroo and 
platform-x86 driver patches I've recently sent off, the dGPU has an ACPI 
_DOD table and one _DDC method corresponding to one of the _DOD entries, 
but the iGPU has neither a _DOD table nor a _DDC method. Either GPU can 
be connected to the internal panel via the dynamically switchable mux, 
and the internal panel's EDID is available via _DDC to allow a 
disconnected GPU to read the EDID. Since only the DGPU has _DOD and 
_DDC, and there's no obvious way to associate connectors on the iGPU 
with connectors on the dGPU, I've implemented the ACPI _DDC EDID 
retrieval with the "first available" implementation you see here. I'm 
open to other ideas if you have them, but didn't see a good way to 
search for the "right" _DDC implementation should there be more than one.


As for preventing the ACPI EDID retrieval from being used for external 
panels, I've done this in the individual DRM drivers that call into the 
new drm_edid_acpi() API since it seemed that each DRM driver had its own 
way of distinguishing display connector types. If there's a good way to 
filter for internal panels in DRM core, I'd be happy to do that instead.

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [Nouveau] [PATCH 1/4] drm: retrieve EDID via ACPI _DDC method

[Alex Deucher]

On Wed, Aug 12, 2020 at 10:31 PM Daniel Dadap <ddadap@nvidia.com> wrote:
>
> Thanks, Lukas. I've incorporated your feedback into my local tree, but
> will wait for additional feedback from the individual DRM driver
> maintainers before sending out a series v2.
>
> On 8/8/20 5:11 PM, Lukas Wunner wrote:
> > On Mon, Jul 27, 2020 at 03:53:54PM -0500, Daniel Dadap wrote:
> >> +             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.
>
>
> Yes, we'd definitely need a way to do this if this functionality ever
> needs to be extended to systems with more than one _DDC method.
> Unfortunately, this will be much easier said than done, since I'm not
> aware of any way to reliably do map _DOD entries to connectors in a GPU
> driver, especially when we're talking about possibly correlating
> connectors on multiple GPUs which mux to the same internal display or
> external connector. All systems which I am aware of that implement ACPI
> _DDC do so for a single internal panel. I don't believe there's any
> reason to ever retrieve an EDID via ACPI _DDC for an external panel, but
> a hypothetical design with multiple internal panels, more than one of
> which needs to retrieve an EDID via ACPI _DDC, would certainly be
> problematic.
>
>
> On at least the system I'm working with for the various switcheroo and
> platform-x86 driver patches I've recently sent off, the dGPU has an ACPI
> _DOD table and one _DDC method corresponding to one of the _DOD entries,
> but the iGPU has neither a _DOD table nor a _DDC method. Either GPU can
> be connected to the internal panel via the dynamically switchable mux,
> and the internal panel's EDID is available via _DDC to allow a
> disconnected GPU to read the EDID. Since only the DGPU has _DOD and
> _DDC, and there's no obvious way to associate connectors on the iGPU
> with connectors on the dGPU, I've implemented the ACPI _DDC EDID
> retrieval with the "first available" implementation you see here. I'm
> open to other ideas if you have them, but didn't see a good way to
> search for the "right" _DDC implementation should there be more than one.
>
>
> As for preventing the ACPI EDID retrieval from being used for external
> panels, I've done this in the individual DRM drivers that call into the
> new drm_edid_acpi() API since it seemed that each DRM driver had its own
> way of distinguishing display connector types. If there's a good way to
> filter for internal panels in DRM core, I'd be happy to do that instead.

I can double check with our ACPI and vbios teams, but I'm not sure
that we ever used the _DDC method on any AMD platforms.  Even if we
did, the driver is still able to get the integrated panel's mode info
via other means.  In general, external connectors would always get the
EDID via i2c or aux.  The only use case we ever had to hardcoding an
EDID was for some really old server chips so they would always think
something was connected if you wanted to attach a crash cart.  That
EDID was stored in the vbios, not ACPI.

As for enumerating which displays were muxed or not and the local
mappings to acpi ids, etc., we had a whole set of ATPX methods to do
that if anyone is interested:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/amd/include/amd_acpi.h

Alex
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx