| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <556979e9-c3d1-45d9-aaa5-b5a17422db5f@amd.com>
Date: Mon, 10 Jun 2024 15:20:20 -0500
From: Mario Limonciello <mario.limonciello@....com>
To: Thomas Weißschuh <linux@...ssschuh.net>
Cc: Alex Deucher <alexander.deucher@....com>,
Christian König <christian.koenig@....com>,
David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
Kieran Levin <ktl@...mework.net>, amd-gfx@...ts.freedesktop.org,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
Dustin Howett <dustin@...ett.net>, Matt Hartley <matt.hartley@...il.com>
Subject: Re: [PATCH] drm/amd: force min_input_signal to 0 on Framework AMD
13/16
On 6/10/2024 15:12, Thomas Weißschuh wrote:
> On 2024-06-10 14:58:02+0000, Mario Limonciello wrote:
>> +Kieran
>>
>> On 6/10/2024 14:26, Thomas Weißschuh wrote:
>>> The value of "min_input_signal" returned from ATIF on a Framework AMD 13
>>> is "12". This leads to a fairly bright minimum display backlight.
>>>
>>> Introduce a quirk to override "min_input_signal" to "0" which leads to a
>>> much lower minimum brightness, which is still readable even in daylight.
>>>
>>> Tested on a Framework AMD 13 BIOS 3.05 and Framework AMD 16.
>>>
>>> Link: https://community.frame.work/t/25711/9
>>> Link: https://community.frame.work/t/47036
>>> Signed-off-by: Thomas Weißschuh <linux@...ssschuh.net>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 35 ++++++++++++++++++++++++++++++++
>>> 1 file changed, 35 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
>>> index 7099ff9cf8c5..b481889f7491 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
>>> @@ -25,6 +25,7 @@
>>> #include <linux/pci.h>
>>> #include <linux/acpi.h>
>>> #include <linux/backlight.h>
>>> +#include <linux/dmi.h>
>>> #include <linux/slab.h>
>>> #include <linux/xarray.h>
>>> #include <linux/power_supply.h>
>>> @@ -130,6 +131,35 @@ static struct amdgpu_acpi_priv {
>>> struct amdgpu_atcs atcs;
>>> } amdgpu_acpi_priv;
>>> +struct amdgpu_acpi_quirks {
>>> + bool ignore_min_input_signal;
>>> +};
>>> +
>>> +static const struct dmi_system_id amdgpu_acpi_quirk_table[] = {
>>> + {
>>> + /* the Framework Laptop 13 (AMD Ryzen) and 16 (AMD Ryzen) */
>>> + .matches = {
>>> + DMI_MATCH(DMI_SYS_VENDOR, "Framework"),
>>> + DMI_MATCH(DMI_PRODUCT_NAME, "AMD Ryzen"),
>>> + DMI_MATCH(DMI_PRODUCT_FAMILY, "Laptop"),
>>> + },
>>
>> Two problems I see:
>>
>> 1) This really "should" be fixed in the BIOS. I added Kieran to the thread
>> for comments if that's viable.
>
> Agreed!
>
>> 2) IMO this is going to match too liberally across all potential Framework
>> models. If they introduce a refreshed motherboard for either product then
>> the quirk would apply to both products when we don't know that such a
>> deficiency would exist.
>
> Also agreed.
> In addition to be really specific this should also match by display type
> (via EDID?).
>
> So far this was only tested with the matte panel.
> (I forgot to mention that, sorry)
Yeah; I would expect this also matters for the new high res panel that
they announced whether this value can work.
>
>> You can reference drivers/platform/x86/amd/pmc/pmc-quirks.c for what we used
>> for a quirk that was matching against a single product and single BIOS.
>
> Will do for the next revision, but let's gather some feedback first.
👍
>
>> But FWIW if that issue isn't fixed in the next BIOS I think we'll end up
>> needing to tear out the BIOS string match and match just the platform.
>
> I'm wondering what the longterm strategy will have to be.
> Given that there are different kinds of displays, and new ones will be
> released, each new display type will require an update to the firmware.
>
> When there are no firmware updates for a device anymore, but new,
> compatible displays are released, then the kernel will need the quirks
> again.
Yeah I think all this points to the 'best' home for this is BIOS.
Framework can test whether the 0 value works on all the displays they
want to support and look for negative impacts for all OSes they support.
>
>>> + .driver_data = &(struct amdgpu_acpi_quirks) {
>>> + .ignore_min_input_signal = true,
>>> + },
>>> + },
>>> + {}
>>> +};
>>> +
>>> +static const struct amdgpu_acpi_quirks *amdgpu_acpi_get_quirks(void)
>>> +{
>>> + const struct dmi_system_id *dmi_id;
>>> +
>>> + dmi_id = dmi_first_match(amdgpu_acpi_quirk_table);
>>> + if (!dmi_id)
>>> + return NULL;
>>> + return dmi_id->driver_data;
>>> +}
>>> +
>>> /* Call the ATIF method
>>> */
>>> /**
>>> @@ -1388,6 +1418,7 @@ bool amdgpu_acpi_should_gpu_reset(struct amdgpu_device *adev)
>>> */
>>> void amdgpu_acpi_detect(void)
>>> {
>>> + const struct amdgpu_acpi_quirks *quirks = amdgpu_acpi_get_quirks();
>>> struct amdgpu_atif *atif = &amdgpu_acpi_priv.atif;
>>> struct amdgpu_atcs *atcs = &amdgpu_acpi_priv.atcs;
>>> struct pci_dev *pdev = NULL;
>>> @@ -1429,6 +1460,10 @@ void amdgpu_acpi_detect(void)
>>> ret);
>>> atif->backlight_caps.caps_valid = false;
>>> }
>>> + if (quirks && quirks->ignore_min_input_signal) {
>>> + DRM_INFO("amdgpu_acpi quirk: min_input_signal=0\n");
>>> + atif->backlight_caps.min_input_signal = 0;
>>> + }
>>> } else {
>>> atif->backlight_caps.caps_valid = false;
>>> }
>>>
>>> ---
>>> base-commit: 83a7eefedc9b56fe7bfeff13b6c7356688ffa670
>>> change-id: 20240610-amdgpu-min-backlight-quirk-8402fd8e736a
>>>
>>> Best regards,
>>
Powered by blists - more mailing lists