Re: [PATCH] ACPI: video: Handle fetching EDID as ACPI_TYPE_PACKAGE

From: Mario Limonciello
Date: Fri Mar 28 2025 - 14:14:27 EST


On 3/28/2025 13:10, Gergo Koteles wrote:
Hi Mario,

Thanks for the suggestions!

On Fri, 2025-03-28 at 08:42 -0500, Mario Limonciello wrote:
On 3/28/2025 06:12, Rafael J. Wysocki wrote:
CC: Hans

On Fri, Mar 28, 2025 at 3:51 AM Gergo Koteles <soyer@xxxxxx> wrote:

Some Lenovo laptops incorrectly return EDID as
buffer in ACPI package (instead of just a buffer)
when calling _DDC.

Calling _DDC generates this ACPI Warning:
ACPI Warning: \_SB.PCI0.GP17.VGA.LCD._DDC: Return type mismatch - \
found Package, expected Integer/Buffer (20240827/nspredef-254)

Use the first element of the package to get the EDID buffer.

The DSDT:

Name (AUOP, Package (0x01)
{
Buffer (0x80)
{
...
}
})

...

Method (_DDC, 1, NotSerialized) // _DDC: Display Data Current
{
If ((PAID == AUID))
{
Return (AUOP) /* \_SB_.PCI0.GP17.VGA_.LCD_.AUOP */
}
ElseIf ((PAID == IVID))
{
Return (IVOP) /* \_SB_.PCI0.GP17.VGA_.LCD_.IVOP */
}
ElseIf ((PAID == BOID))
{
Return (BOEP) /* \_SB_.PCI0.GP17.VGA_.LCD_.BOEP */
}
ElseIf ((PAID == SAID))
{
Return (SUNG) /* \_SB_.PCI0.GP17.VGA_.LCD_.SUNG */
}

Return (Zero)
}

Cc: stable@xxxxxxxxxxxxxxx
Fixes: c6a837088bed ("drm/amd/display: Fetch the EDID from _DDC if available for eDP")
Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/4085
Signed-off-by: Gergo Koteles <soyer@xxxxxx>

FWIW the ACPI spec is clear that this /should/ be an ACPI buffer.

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

That being said this is production firmware and in the wild, I don't
personally see a problem with handling it this way.

Some other improvement suggestion though below.

---
drivers/acpi/acpi_video.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
index efdadc74e3f4..65cf36796506 100644
--- a/drivers/acpi/acpi_video.c
+++ b/drivers/acpi/acpi_video.c
@@ -649,6 +649,9 @@ acpi_video_device_EDID(struct acpi_video_device *device, void **edid, int length

obj = buffer.pointer;

+ if (obj && obj->type == ACPI_TYPE_PACKAGE && obj->package.count == 1)
+ obj = &obj->package.elements[0];
+

As the ACPI spec indicates this should be a buffer, I think it's a good
idea to emit a FW_BUG message here so that this can be detected by users
and tools like FWTS and the firmware can be improved in the future.

Something like this:

if (condition) {
pr_info(FW_BUG "EDID was found in ACPI package instead of ACPI buffer");
obj = &obj->package.elements[0];
}


An ACPI Warning is currently being generated:

ACPI Warning: \_SB.PCI0.GP17.VGA.LCD._DDC: Return type mismatch - found
Package, expected Integer/Buffer (20240827/nspredef-254)

This is also noticed by FWTS in the form of KlogAcpiReturnTypeMismatch
and may be noticed by users as well.

I think it is unnecessary to emit two warnings for the same problem.

However, some comments could make the code clearer. I will add some
comments to V2.

Ah yes; if this warning is already being emitted no extra FW_BUG is needed.

Sounds good on comments for v2.

You might also reference the ACPI spec in your commit message in v2.



if (obj && obj->type == ACPI_TYPE_BUFFER) {
*edid = kmemdup(obj->buffer.pointer, obj->buffer.length, GFP_KERNEL);
ret = *edid ? obj->buffer.length : -ENOMEM;
@@ -658,7 +661,7 @@ acpi_video_device_EDID(struct acpi_video_device *device, void **edid, int length
ret = -EFAULT;
}

- kfree(obj);
+ kfree(buffer.pointer);

Any reason for this change? obj is assigned to buffer.pointer already.



In the case of an ACPI package, obj points to the first element of the
package. The buffer.pointer still points to the original location.

Got it thanks.


Thanks,
Gergo