| Message ID | 4cef341fdf7a0e877c50b502fc95ee8be28aa811.1743129387.git.soyer@irl.hu (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | ACPI: video: Handle fetching EDID as ACPI_TYPE_PACKAGE | expand |
CC: Hans On Fri, Mar 28, 2025 at 3:51 AM Gergo Koteles <soyer@irl.hu> 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@vger.kernel.org > 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@irl.hu> > --- > 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]; > + > 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); > return ret; > } > > -- > 2.49.0 >
On 3/28/2025 06:12, Rafael J. Wysocki wrote: > CC: Hans > > On Fri, Mar 28, 2025 at 3:51 AM Gergo Koteles <soyer@irl.hu> 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@vger.kernel.org >> 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@irl.hu> 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]; } >> 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. >> return ret; >> } >> >> -- >> 2.49.0 >>
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@irl.hu> 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@vger.kernel.org > > > 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@irl.hu> > > 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. > > > 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. Thanks, Gergo
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@irl.hu> 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@vger.kernel.org >>>> 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@irl.hu> >> >> 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 >
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]; + 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); return ret; }
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@vger.kernel.org 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@irl.hu> --- drivers/acpi/acpi_video.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)