Re: [Nouveau] [PATCH 1/4] drm: retrieve EDID via ACPI _DDC method
On Wed, Aug 12, 2020 at 10:31 PM Daniel Dadap 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
Re: [Nouveau] [PATCH 1/4] drm: retrieve EDID via ACPI _DDC method
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
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 > #include > #include > +#include 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
[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 --- 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 #include #include +#include #include #include @@ -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->hand