Daniel Dadap
2020-Jul-27 20:53 UTC
[Nouveau] [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
Daniel Dadap
2020-Jul-27 20:53 UTC
[Nouveau] [PATCH 1/4] drm: retrieve EDID via ACPI _DDC method
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
Daniel Dadap
2020-Jul-27 20:53 UTC
[Nouveau] [PATCH 2/4] i915: fall back to ACPI EDID retrieval
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
Daniel Dadap
2020-Jul-27 20:53 UTC
[Nouveau] [PATCH 3/4] nouveau: fall back to ACPI EDID retrieval
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
Daniel Dadap
2020-Jul-27 20:53 UTC
[Nouveau] [PATCH 4/4] radeon: fall back to ACPI EDID retrieval
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
Christian König
2020-Jul-28 06:50 UTC
[Nouveau] [PATCH 4/4] radeon: fall back to ACPI EDID retrieval
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,
Lukas Wunner
2020-Aug-08 22:11 UTC
[Nouveau] [PATCH 1/4] drm: retrieve EDID via ACPI _DDC method
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
Apparently Analagous Threads
- [PATCH 0/4] drm: add support for retrieving EDID via ACPI _DDC
- [PATCH 1/4] drm: retrieve EDID via ACPI _DDC method
- [PATCH 1/4] drm: retrieve EDID via ACPI _DDC method
- [PATCH 4/4] radeon: fall back to ACPI EDID retrieval
- [PATCH 4/4] radeon: fall back to ACPI EDID retrieval