diff mbox series

drm/amd: force min_input_signal to 0 on Framework AMD 13/16

Message ID 20240610-amdgpu-min-backlight-quirk-v1-1-8459895a5b2a@weissschuh.net (mailing list archive)
State New, archived
Headers show
Series drm/amd: force min_input_signal to 0 on Framework AMD 13/16 | expand

Commit Message

Thomas Weißschuh June 10, 2024, 7:26 p.m. UTC 
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@weissschuh.net>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 35 ++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)


---
base-commit: 83a7eefedc9b56fe7bfeff13b6c7356688ffa670
change-id: 20240610-amdgpu-min-backlight-quirk-8402fd8e736a

Best regards,

Comments

Mario Limonciello June 10, 2024, 7:58 p.m. UTC | #1 
+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@weissschuh.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.

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.

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.

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.


> +		.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,
Thomas Weißschuh June 10, 2024, 8:12 p.m. UTC | #2 
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@weissschuh.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)

> 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.

> > +		.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,
>
Mario Limonciello June 10, 2024, 8:20 p.m. UTC | #3 
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@weissschuh.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.
diff mbox series

Patch

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"),
+		},
+		.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;
 	}