[PATCH v2 0/3] drm: backlight quirk infrastructure and lower minimum for Framework AMD 13

[PATCH v2 0/3] drm: backlight quirk infrastructure and lower minimum for Framework AMD 13

[Thomas Weißschuh]

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.

Add a generic quirk infrastructure for backlight configuration to
override the settings provided by the firmware.
Also add amdgpu as a user of that infrastructure and a quirk for the
Framework 13 matte panel.
Most likely this will also work for the glossy panel, but I can't test
that.

One solution would be a fixed firmware version, but given that the
problem exists since the release of the hardware, it has been known for
a month that the hardware can go lower and there was no acknowledgment
from Framework in any way, I'd like to explore this alternative
way forward.

Notes:

* Should the quirk infrastructure be part of drm_edid.c?
* The current allocation of struct drm_edid in amdgpu is bad.
  But it is done the same way in other parts of amdgpu.
  I do have patches migrating amdgpu to proper usage of struct drm_edid [0]

Mario:

I intentionally left out the consideration of the firmware version.
The quirk will stay correct even if the firmware starts reporting
correct values.
If there are strong opinions it would be easy to add, though.

Based on amdgpu/drm-next.

[0] https://lore.kernel.org/lkml/20240616-amdgpu-edid-bios-v1-1-2874f212b365@weissschuh.net/

---
Changes in v2:
- Introduce proper drm backlight quirk infrastructure
- Quirk by EDID and DMI instead of only DMI
- Limit quirk to only single Framework 13 matte panel
- Link to v1: https://lore.kernel.org/r/20240610-amdgpu-min-backlight-quirk-v1-1-8459895a5b2a@weissschuh.net

---
Thomas Weißschuh (3):
      drm: Add panel backlight quirks
      drm: panel-backlight-quirks: Add Framework 13 matte panel
      drm/amd/display: Add support backlight quirks

 Documentation/gpu/drm-kms-helpers.rst             |  3 +
 drivers/gpu/drm/Kconfig                           |  4 ++
 drivers/gpu/drm/Makefile                          |  1 +
 drivers/gpu/drm/amd/amdgpu/Kconfig                |  1 +
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 28 +++++++++
 drivers/gpu/drm/drm_panel_backlight_quirks.c      | 76 +++++++++++++++++++++++
 include/drm/drm_utils.h                           | 11 ++++
 7 files changed, 124 insertions(+)
---
base-commit: 1ecef5589320fd56af599b624d59c355d162ac7b
change-id: 20240610-amdgpu-min-backlight-quirk-8402fd8e736a

Best regards,
-- 
Thomas Weißschuh <linux@weissschuh.net>

[PATCH v2 1/3] drm: Add panel backlight quirks

[Thomas Weißschuh]

Panels using a PWM-controlled backlight source without an do not have a
standard way to communicate their valid PWM ranges.
On x86 the ranges are read from ACPI through driver-specific tables.
The built-in ranges are not necessarily correct, or may grow stale if an
older device can be retrofitted with newer panels.

Add a quirk infrastructure with which the valid backlight ranges can be
maintained as part of the kernel.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 Documentation/gpu/drm-kms-helpers.rst        |  3 ++
 drivers/gpu/drm/Kconfig                      |  4 ++
 drivers/gpu/drm/Makefile                     |  1 +
 drivers/gpu/drm/drm_panel_backlight_quirks.c | 67 ++++++++++++++++++++++++++++
 include/drm/drm_utils.h                      | 11 +++++
 5 files changed, 86 insertions(+)

diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
index 59cfe8a7a8ba..1998a2675210 100644
--- a/Documentation/gpu/drm-kms-helpers.rst
+++ b/Documentation/gpu/drm-kms-helpers.rst
@@ -224,6 +224,9 @@ Panel Helper Reference
 .. kernel-doc:: drivers/gpu/drm/drm_panel_orientation_quirks.c
    :export:
 
+.. kernel-doc:: drivers/gpu/drm/drm_panel_backlight_quirks.c
+   :export:
+
 Panel Self Refresh Helper Reference
 ===================================
 
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 959b19a04101..50ccb43315bf 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -443,6 +443,10 @@ config DRM_EXPORT_FOR_TESTS
 config DRM_PANEL_ORIENTATION_QUIRKS
 	tristate
 
+# Separate option as not all DRM drivers use it
+config DRM_PANEL_BACKLIGHT_QUIRKS
+	tristate
+
 config DRM_LIB_RANDOM
 	bool
 	default n
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index f9ca4f8fa6c5..6669913b907e 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -92,6 +92,7 @@ drm-$(CONFIG_DRM_PANIC) += drm_panic.o
 obj-$(CONFIG_DRM)	+= drm.o
 
 obj-$(CONFIG_DRM_PANEL_ORIENTATION_QUIRKS) += drm_panel_orientation_quirks.o
+obj-$(CONFIG_DRM_PANEL_BACKLIGHT_QUIRKS) += drm_panel_backlight_quirks.o
 
 #
 # Memory-management helpers
diff --git a/drivers/gpu/drm/drm_panel_backlight_quirks.c b/drivers/gpu/drm/drm_panel_backlight_quirks.c
new file mode 100644
index 000000000000..a89b5fd1940e
--- /dev/null
+++ b/drivers/gpu/drm/drm_panel_backlight_quirks.c
@@ -0,0 +1,67 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/array_size.h>
+#include <linux/dmi.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <drm/drm_connector.h>
+#include <drm/drm_utils.h>
+
+struct drm_panel_backlight_entry {
+	struct {
+		enum dmi_field field;
+		const char * const value;
+	} dmi_match;
+	struct drm_edid_ident ident;
+	struct drm_panel_backlight_quirk quirk;
+};
+
+static const struct drm_panel_backlight_entry drm_panel_backlight_entries[] = {
+};
+
+static bool drm_panel_backlight_entry_matches(const struct drm_panel_backlight_entry *entry,
+					      const struct drm_edid *edid)
+{
+	if (!dmi_match(entry->dmi_match.field, entry->dmi_match.value))
+		return false;
+
+	if (!drm_edid_match(edid, &entry->ident))
+		return false;
+
+	return true;
+}
+
+/**
+ * drm_get_panel_panel_quirk - Check for panel backlight quirks
+ * @edid: EDID of the panel to check
+ *
+ * This function checks for platform specific (e.g. DMI based) quirks
+ * providing info on backlight control for systems where this cannot be
+ * probed from the hard-/firm-ware.
+ *
+ * Returns:
+ * A struct drm_panel_backlight_quirk if a quirk is found or NULL otherwise.
+ */
+const struct drm_panel_backlight_quirk *drm_get_panel_backlight_quirk(const struct drm_edid *edid)
+{
+	const struct drm_panel_backlight_entry *entry;
+	size_t i;
+
+	if (!IS_ENABLED(CONFIG_DMI))
+		return NULL;
+
+	if (!edid)
+		return NULL;
+
+	for (i = 0; i < ARRAY_SIZE(drm_panel_backlight_entries); i++) {
+		entry = &drm_panel_backlight_entries[i];
+
+		if (drm_panel_backlight_entry_matches(entry, edid))
+			return &entry->quirk;
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL(drm_get_panel_backlight_quirk);
+
+MODULE_LICENSE("GPL");
diff --git a/include/drm/drm_utils.h b/include/drm/drm_utils.h
index 70775748d243..37cc6de1a01a 100644
--- a/include/drm/drm_utils.h
+++ b/include/drm/drm_utils.h
@@ -11,9 +11,20 @@
 #define __DRM_UTILS_H__
 
 #include <linux/types.h>
+#include <drm/drm_edid.h>
+
+struct drm_panel_backlight_quirk {
+	struct {
+		bool pwm_min_brightness:1;
+	} overrides;
+
+	u8 pwm_min_brightness; /* min_brightness/255 of max */
+};
 
 int drm_get_panel_orientation_quirk(int width, int height);
 
+const struct drm_panel_backlight_quirk *drm_get_panel_backlight_quirk(const struct drm_edid *edid);
+
 signed long drm_timeout_abs_to_jiffies(int64_t timeout_nsec);
 
 #endif

-- 
2.45.2

Re: [PATCH v2 1/3] drm: Add panel backlight quirks

[Mario Limonciello]

On 6/23/2024 03:51, Thomas Weißschuh wrote:
> Panels using a PWM-controlled backlight source without an do not have a
> standard way to communicate their valid PWM ranges.
> On x86 the ranges are read from ACPI through driver-specific tables.
> The built-in ranges are not necessarily correct, or may grow stale if an
> older device can be retrofitted with newer panels.
> 
> Add a quirk infrastructure with which the valid backlight ranges can be
> maintained as part of the kernel.
> 

So I was just talking to some folks in the Linux handheld gaming 
community (added to CC) about an issue they have where they need to know 
the correct panel orientation.  Due to reuse of panels across vendors 
the orientation on one might not be appropriate on another.  The trick 
is then to detect the combo of both the panel and the DMI data.

It's the same "kind" of problem where something advertised in the 
firmware should be ignored but only on a panel + SMBIOS combination.

So I am wondering if what you're proposing here could be more 
generalized.  IE "drm_panel_quirks.c" instead?

Thoughts?

> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>   Documentation/gpu/drm-kms-helpers.rst        |  3 ++
>   drivers/gpu/drm/Kconfig                      |  4 ++
>   drivers/gpu/drm/Makefile                     |  1 +
>   drivers/gpu/drm/drm_panel_backlight_quirks.c | 67 ++++++++++++++++++++++++++++
>   include/drm/drm_utils.h                      | 11 +++++
>   5 files changed, 86 insertions(+)
> 
> diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
> index 59cfe8a7a8ba..1998a2675210 100644
> --- a/Documentation/gpu/drm-kms-helpers.rst
> +++ b/Documentation/gpu/drm-kms-helpers.rst
> @@ -224,6 +224,9 @@ Panel Helper Reference
>   .. kernel-doc:: drivers/gpu/drm/drm_panel_orientation_quirks.c
>      :export:
>   
> +.. kernel-doc:: drivers/gpu/drm/drm_panel_backlight_quirks.c
> +   :export:
> +
>   Panel Self Refresh Helper Reference
>   ===================================
>   
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 959b19a04101..50ccb43315bf 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -443,6 +443,10 @@ config DRM_EXPORT_FOR_TESTS
>   config DRM_PANEL_ORIENTATION_QUIRKS
>   	tristate
>   
> +# Separate option as not all DRM drivers use it
> +config DRM_PANEL_BACKLIGHT_QUIRKS
> +	tristate
> +
>   config DRM_LIB_RANDOM
>   	bool
>   	default n
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index f9ca4f8fa6c5..6669913b907e 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -92,6 +92,7 @@ drm-$(CONFIG_DRM_PANIC) += drm_panic.o
>   obj-$(CONFIG_DRM)	+= drm.o
>   
>   obj-$(CONFIG_DRM_PANEL_ORIENTATION_QUIRKS) += drm_panel_orientation_quirks.o
> +obj-$(CONFIG_DRM_PANEL_BACKLIGHT_QUIRKS) += drm_panel_backlight_quirks.o
>   
>   #
>   # Memory-management helpers
> diff --git a/drivers/gpu/drm/drm_panel_backlight_quirks.c b/drivers/gpu/drm/drm_panel_backlight_quirks.c
> new file mode 100644
> index 000000000000..a89b5fd1940e
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_panel_backlight_quirks.c
> @@ -0,0 +1,67 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/array_size.h>
> +#include <linux/dmi.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <drm/drm_connector.h>
> +#include <drm/drm_utils.h>
> +
> +struct drm_panel_backlight_entry {
> +	struct {
> +		enum dmi_field field;
> +		const char * const value;
> +	} dmi_match;
> +	struct drm_edid_ident ident;
> +	struct drm_panel_backlight_quirk quirk;
> +};
> +
> +static const struct drm_panel_backlight_entry drm_panel_backlight_entries[] = {
> +};
> +
> +static bool drm_panel_backlight_entry_matches(const struct drm_panel_backlight_entry *entry,
> +					      const struct drm_edid *edid)
> +{
> +	if (!dmi_match(entry->dmi_match.field, entry->dmi_match.value))
> +		return false;
> +
> +	if (!drm_edid_match(edid, &entry->ident))
> +		return false;
> +
> +	return true;
> +}
> +
> +/**
> + * drm_get_panel_panel_quirk - Check for panel backlight quirks
> + * @edid: EDID of the panel to check
> + *
> + * This function checks for platform specific (e.g. DMI based) quirks
> + * providing info on backlight control for systems where this cannot be
> + * probed from the hard-/firm-ware.
> + *
> + * Returns:
> + * A struct drm_panel_backlight_quirk if a quirk is found or NULL otherwise.
> + */
> +const struct drm_panel_backlight_quirk *drm_get_panel_backlight_quirk(const struct drm_edid *edid)
> +{
> +	const struct drm_panel_backlight_entry *entry;
> +	size_t i;
> +
> +	if (!IS_ENABLED(CONFIG_DMI))
> +		return NULL;
> +
> +	if (!edid)
> +		return NULL;
> +
> +	for (i = 0; i < ARRAY_SIZE(drm_panel_backlight_entries); i++) {
> +		entry = &drm_panel_backlight_entries[i];
> +
> +		if (drm_panel_backlight_entry_matches(entry, edid))
> +			return &entry->quirk;
> +	}
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL(drm_get_panel_backlight_quirk);
> +
> +MODULE_LICENSE("GPL");
> diff --git a/include/drm/drm_utils.h b/include/drm/drm_utils.h
> index 70775748d243..37cc6de1a01a 100644
> --- a/include/drm/drm_utils.h
> +++ b/include/drm/drm_utils.h
> @@ -11,9 +11,20 @@
>   #define __DRM_UTILS_H__
>   
>   #include <linux/types.h>
> +#include <drm/drm_edid.h>
> +
> +struct drm_panel_backlight_quirk {
> +	struct {
> +		bool pwm_min_brightness:1;
> +	} overrides;
> +
> +	u8 pwm_min_brightness; /* min_brightness/255 of max */
> +};
>   
>   int drm_get_panel_orientation_quirk(int width, int height);
>   
> +const struct drm_panel_backlight_quirk *drm_get_panel_backlight_quirk(const struct drm_edid *edid);
> +
>   signed long drm_timeout_abs_to_jiffies(int64_t timeout_nsec);
>   
>   #endif
> 

Re: [PATCH v2 1/3] drm: Add panel backlight quirks

[Hans de Goede]

Hi,

On 6/23/24 10:20 PM, Mario Limonciello wrote:
> On 6/23/2024 03:51, Thomas Weißschuh wrote:
>> Panels using a PWM-controlled backlight source without an do not have a
>> standard way to communicate their valid PWM ranges.
>> On x86 the ranges are read from ACPI through driver-specific tables.
>> The built-in ranges are not necessarily correct, or may grow stale if an
>> older device can be retrofitted with newer panels.
>>
>> Add a quirk infrastructure with which the valid backlight ranges can be
>> maintained as part of the kernel.
>>
> 
> So I was just talking to some folks in the Linux handheld gaming community (added to CC) about an issue they have where they need to know the correct panel orientation.  Due to reuse of panels across vendors the orientation on one might not be appropriate on another.  The trick is then to detect the combo of both the panel and the DMI data.
> 
> It's the same "kind" of problem where something advertised in the firmware should be ignored but only on a panel + SMBIOS combination.
> 
> So I am wondering if what you're proposing here could be more generalized.  IE "drm_panel_quirks.c" instead?
> 
> Thoughts?

Note we already have a quirk mechanism for non upright mounted lcd-panels:

drivers/gpu/drm/drm_panel_orientation_quirks.c

note that the info here is shared with the simpledrm and
efifb drivers, so if the chose is made to extend this then
that needs to be taken into account.

Regards,

Hans





>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>> ---
>>   Documentation/gpu/drm-kms-helpers.rst        |  3 ++
>>   drivers/gpu/drm/Kconfig                      |  4 ++
>>   drivers/gpu/drm/Makefile                     |  1 +
>>   drivers/gpu/drm/drm_panel_backlight_quirks.c | 67 ++++++++++++++++++++++++++++
>>   include/drm/drm_utils.h                      | 11 +++++
>>   5 files changed, 86 insertions(+)
>>
>> diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
>> index 59cfe8a7a8ba..1998a2675210 100644
>> --- a/Documentation/gpu/drm-kms-helpers.rst
>> +++ b/Documentation/gpu/drm-kms-helpers.rst
>> @@ -224,6 +224,9 @@ Panel Helper Reference
>>   .. kernel-doc:: drivers/gpu/drm/drm_panel_orientation_quirks.c
>>      :export:
>>   +.. kernel-doc:: drivers/gpu/drm/drm_panel_backlight_quirks.c
>> +   :export:
>> +
>>   Panel Self Refresh Helper Reference
>>   ===================================
>>   diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index 959b19a04101..50ccb43315bf 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -443,6 +443,10 @@ config DRM_EXPORT_FOR_TESTS
>>   config DRM_PANEL_ORIENTATION_QUIRKS
>>       tristate
>>   +# Separate option as not all DRM drivers use it
>> +config DRM_PANEL_BACKLIGHT_QUIRKS
>> +    tristate
>> +
>>   config DRM_LIB_RANDOM
>>       bool
>>       default n
>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> index f9ca4f8fa6c5..6669913b907e 100644
>> --- a/drivers/gpu/drm/Makefile
>> +++ b/drivers/gpu/drm/Makefile
>> @@ -92,6 +92,7 @@ drm-$(CONFIG_DRM_PANIC) += drm_panic.o
>>   obj-$(CONFIG_DRM)    += drm.o
>>     obj-$(CONFIG_DRM_PANEL_ORIENTATION_QUIRKS) += drm_panel_orientation_quirks.o
>> +obj-$(CONFIG_DRM_PANEL_BACKLIGHT_QUIRKS) += drm_panel_backlight_quirks.o
>>     #
>>   # Memory-management helpers
>> diff --git a/drivers/gpu/drm/drm_panel_backlight_quirks.c b/drivers/gpu/drm/drm_panel_backlight_quirks.c
>> new file mode 100644
>> index 000000000000..a89b5fd1940e
>> --- /dev/null
>> +++ b/drivers/gpu/drm/drm_panel_backlight_quirks.c
>> @@ -0,0 +1,67 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include <linux/array_size.h>
>> +#include <linux/dmi.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/module.h>
>> +#include <drm/drm_connector.h>
>> +#include <drm/drm_utils.h>
>> +
>> +struct drm_panel_backlight_entry {
>> +    struct {
>> +        enum dmi_field field;
>> +        const char * const value;
>> +    } dmi_match;
>> +    struct drm_edid_ident ident;
>> +    struct drm_panel_backlight_quirk quirk;
>> +};
>> +
>> +static const struct drm_panel_backlight_entry drm_panel_backlight_entries[] = {
>> +};
>> +
>> +static bool drm_panel_backlight_entry_matches(const struct drm_panel_backlight_entry *entry,
>> +                          const struct drm_edid *edid)
>> +{
>> +    if (!dmi_match(entry->dmi_match.field, entry->dmi_match.value))
>> +        return false;
>> +
>> +    if (!drm_edid_match(edid, &entry->ident))
>> +        return false;
>> +
>> +    return true;
>> +}
>> +
>> +/**
>> + * drm_get_panel_panel_quirk - Check for panel backlight quirks
>> + * @edid: EDID of the panel to check
>> + *
>> + * This function checks for platform specific (e.g. DMI based) quirks
>> + * providing info on backlight control for systems where this cannot be
>> + * probed from the hard-/firm-ware.
>> + *
>> + * Returns:
>> + * A struct drm_panel_backlight_quirk if a quirk is found or NULL otherwise.
>> + */
>> +const struct drm_panel_backlight_quirk *drm_get_panel_backlight_quirk(const struct drm_edid *edid)
>> +{
>> +    const struct drm_panel_backlight_entry *entry;
>> +    size_t i;
>> +
>> +    if (!IS_ENABLED(CONFIG_DMI))
>> +        return NULL;
>> +
>> +    if (!edid)
>> +        return NULL;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(drm_panel_backlight_entries); i++) {
>> +        entry = &drm_panel_backlight_entries[i];
>> +
>> +        if (drm_panel_backlight_entry_matches(entry, edid))
>> +            return &entry->quirk;
>> +    }
>> +
>> +    return NULL;
>> +}
>> +EXPORT_SYMBOL(drm_get_panel_backlight_quirk);
>> +
>> +MODULE_LICENSE("GPL");
>> diff --git a/include/drm/drm_utils.h b/include/drm/drm_utils.h
>> index 70775748d243..37cc6de1a01a 100644
>> --- a/include/drm/drm_utils.h
>> +++ b/include/drm/drm_utils.h
>> @@ -11,9 +11,20 @@
>>   #define __DRM_UTILS_H__
>>     #include <linux/types.h>
>> +#include <drm/drm_edid.h>
>> +
>> +struct drm_panel_backlight_quirk {
>> +    struct {
>> +        bool pwm_min_brightness:1;
>> +    } overrides;
>> +
>> +    u8 pwm_min_brightness; /* min_brightness/255 of max */
>> +};
>>     int drm_get_panel_orientation_quirk(int width, int height);
>>   +const struct drm_panel_backlight_quirk *drm_get_panel_backlight_quirk(const struct drm_edid *edid);
>> +
>>   signed long drm_timeout_abs_to_jiffies(int64_t timeout_nsec);
>>     #endif
>>
> 

Re: [PATCH v2 1/3] drm: Add panel backlight quirks

[Mario Limonciello]

On 6/23/2024 15:55, Hans de Goede wrote:
> Hi,
> 
> On 6/23/24 10:20 PM, Mario Limonciello wrote:
>> On 6/23/2024 03:51, Thomas Weißschuh wrote:
>>> Panels using a PWM-controlled backlight source without an do not have a
>>> standard way to communicate their valid PWM ranges.
>>> On x86 the ranges are read from ACPI through driver-specific tables.
>>> The built-in ranges are not necessarily correct, or may grow stale if an
>>> older device can be retrofitted with newer panels.
>>>
>>> Add a quirk infrastructure with which the valid backlight ranges can be
>>> maintained as part of the kernel.
>>>
>>
>> So I was just talking to some folks in the Linux handheld gaming community (added to CC) about an issue they have where they need to know the correct panel orientation.  Due to reuse of panels across vendors the orientation on one might not be appropriate on another.  The trick is then to detect the combo of both the panel and the DMI data.
>>
>> It's the same "kind" of problem where something advertised in the firmware should be ignored but only on a panel + SMBIOS combination.
>>
>> So I am wondering if what you're proposing here could be more generalized.  IE "drm_panel_quirks.c" instead?
>>
>> Thoughts?
> 
> Note we already have a quirk mechanism for non upright mounted lcd-panels:
> 
> drivers/gpu/drm/drm_panel_orientation_quirks.c
> 
> note that the info here is shared with the simpledrm and
> efifb drivers, so if the chose is made to extend this then
> that needs to be taken into account.
> 
> Regards,
> 
> Hans

Thanks for sharing.  Totally agree this is this the better way to go for 
what I raised.

Re: [PATCH v2 1/3] drm: Add panel backlight quirks

[Hans de Goede]

Hi Thomas,

On 6/23/24 10:51 AM, Thomas Weißschuh wrote:
> Panels using a PWM-controlled backlight source without an do not have a
> standard way to communicate their valid PWM ranges.
> On x86 the ranges are read from ACPI through driver-specific tables.
> The built-in ranges are not necessarily correct, or may grow stale if an
> older device can be retrofitted with newer panels.
> 
> Add a quirk infrastructure with which the valid backlight ranges can be
> maintained as part of the kernel.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  Documentation/gpu/drm-kms-helpers.rst        |  3 ++
>  drivers/gpu/drm/Kconfig                      |  4 ++
>  drivers/gpu/drm/Makefile                     |  1 +
>  drivers/gpu/drm/drm_panel_backlight_quirks.c | 67 ++++++++++++++++++++++++++++
>  include/drm/drm_utils.h                      | 11 +++++
>  5 files changed, 86 insertions(+)
> 
> diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
> index 59cfe8a7a8ba..1998a2675210 100644
> --- a/Documentation/gpu/drm-kms-helpers.rst
> +++ b/Documentation/gpu/drm-kms-helpers.rst
> @@ -224,6 +224,9 @@ Panel Helper Reference
>  .. kernel-doc:: drivers/gpu/drm/drm_panel_orientation_quirks.c
>     :export:
>  
> +.. kernel-doc:: drivers/gpu/drm/drm_panel_backlight_quirks.c
> +   :export:
> +
>  Panel Self Refresh Helper Reference
>  ===================================
>  
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 959b19a04101..50ccb43315bf 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -443,6 +443,10 @@ config DRM_EXPORT_FOR_TESTS
>  config DRM_PANEL_ORIENTATION_QUIRKS
>  	tristate
>  
> +# Separate option as not all DRM drivers use it
> +config DRM_PANEL_BACKLIGHT_QUIRKS
> +	tristate
> +
>  config DRM_LIB_RANDOM
>  	bool
>  	default n
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index f9ca4f8fa6c5..6669913b907e 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -92,6 +92,7 @@ drm-$(CONFIG_DRM_PANIC) += drm_panic.o
>  obj-$(CONFIG_DRM)	+= drm.o
>  
>  obj-$(CONFIG_DRM_PANEL_ORIENTATION_QUIRKS) += drm_panel_orientation_quirks.o
> +obj-$(CONFIG_DRM_PANEL_BACKLIGHT_QUIRKS) += drm_panel_backlight_quirks.o
>  
>  #
>  # Memory-management helpers
> diff --git a/drivers/gpu/drm/drm_panel_backlight_quirks.c b/drivers/gpu/drm/drm_panel_backlight_quirks.c
> new file mode 100644
> index 000000000000..a89b5fd1940e
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_panel_backlight_quirks.c
> @@ -0,0 +1,67 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/array_size.h>
> +#include <linux/dmi.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <drm/drm_connector.h>
> +#include <drm/drm_utils.h>
> +
> +struct drm_panel_backlight_entry {
> +	struct {
> +		enum dmi_field field;
> +		const char * const value;
> +	} dmi_match;

Matching on a single DMI string is not always enough to uniquely identify
a machine. I would change this to a dmi_system_id struct and then add
an array with 2 dmi_system_id structs in drm_panel_backlight_entry_matches()
and copy the struct to the first array entry + zero out the second entry
(terminator) and then use dmi_check_system().

> +	struct drm_edid_ident ident;

Hmm, what about DSI panels? These do not (always) have EDID info AFAIK.

drivers/gpu/drm/drm_panel_orientation_quirks.c is using a resolution match
so as to hopefully not match external screens, but that is also so that it can
be used from efifb / simpledrm and here you really do want to differentiate
between different panels by panel model.

So I guess that the EDId match is fine and if we ever need to match DSI panels
without EDID we figure something out then.

Thinking more about this I have a question about the approach as a whole though,
I'll reply to the cover-letter with this.

Regards,

Hans



> +	struct drm_panel_backlight_quirk quirk;
> +};
> +
> +static const struct drm_panel_backlight_entry drm_panel_backlight_entries[] = {
> +};
> +
> +static bool drm_panel_backlight_entry_matches(const struct drm_panel_backlight_entry *entry,
> +					      const struct drm_edid *edid)
> +{
> +	if (!dmi_match(entry->dmi_match.field, entry->dmi_match.value))
> +		return false;
> +
> +	if (!drm_edid_match(edid, &entry->ident))
> +		return false;
> +
> +	return true;
> +}
> +
> +/**
> + * drm_get_panel_panel_quirk - Check for panel backlight quirks
> + * @edid: EDID of the panel to check
> + *
> + * This function checks for platform specific (e.g. DMI based) quirks
> + * providing info on backlight control for systems where this cannot be
> + * probed from the hard-/firm-ware.
> + *
> + * Returns:
> + * A struct drm_panel_backlight_quirk if a quirk is found or NULL otherwise.
> + */
> +const struct drm_panel_backlight_quirk *drm_get_panel_backlight_quirk(const struct drm_edid *edid)
> +{
> +	const struct drm_panel_backlight_entry *entry;
> +	size_t i;
> +
> +	if (!IS_ENABLED(CONFIG_DMI))
> +		return NULL;
> +
> +	if (!edid)
> +		return NULL;
> +
> +	for (i = 0; i < ARRAY_SIZE(drm_panel_backlight_entries); i++) {
> +		entry = &drm_panel_backlight_entries[i];
> +
> +		if (drm_panel_backlight_entry_matches(entry, edid))
> +			return &entry->quirk;
> +	}
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL(drm_get_panel_backlight_quirk);
> +
> +MODULE_LICENSE("GPL");
> diff --git a/include/drm/drm_utils.h b/include/drm/drm_utils.h
> index 70775748d243..37cc6de1a01a 100644
> --- a/include/drm/drm_utils.h
> +++ b/include/drm/drm_utils.h
> @@ -11,9 +11,20 @@
>  #define __DRM_UTILS_H__
>  
>  #include <linux/types.h>
> +#include <drm/drm_edid.h>
> +
> +struct drm_panel_backlight_quirk {
> +	struct {
> +		bool pwm_min_brightness:1;
> +	} overrides;
> +
> +	u8 pwm_min_brightness; /* min_brightness/255 of max */
> +};
>  
>  int drm_get_panel_orientation_quirk(int width, int height);
>  
> +const struct drm_panel_backlight_quirk *drm_get_panel_backlight_quirk(const struct drm_edid *edid);
> +
>  signed long drm_timeout_abs_to_jiffies(int64_t timeout_nsec);
>  
>  #endif
> 

Re: [PATCH v2 1/3] drm: Add panel backlight quirks

[Jeff Johnson]

On 6/23/24 01:51, Thomas Weißschuh wrote:
> Panels using a PWM-controlled backlight source without an do not have a
> standard way to communicate their valid PWM ranges.
> On x86 the ranges are read from ACPI through driver-specific tables.
> The built-in ranges are not necessarily correct, or may grow stale if an
> older device can be retrofitted with newer panels.
> 
> Add a quirk infrastructure with which the valid backlight ranges can be
> maintained as part of the kernel.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
...

> +EXPORT_SYMBOL(drm_get_panel_backlight_quirk);
> +
> +MODULE_LICENSE("GPL");

Missing a MODULE_DESCRIPTION()

This will result in a make W=1 warning

[PATCH v2 2/3] drm: panel-backlight-quirks: Add Framework 13 matte panel

[Thomas Weißschuh]

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.

Add a quirk to override that the minimum backlight PWM to "0" which
leads to a much lower minimum brightness, which is still visible.

Tested on a Framework AMD 13 BIOS 3.05 with the matte panel.

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/drm_panel_backlight_quirks.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/drm_panel_backlight_quirks.c b/drivers/gpu/drm/drm_panel_backlight_quirks.c
index a89b5fd1940e..e7671b1ba885 100644
--- a/drivers/gpu/drm/drm_panel_backlight_quirks.c
+++ b/drivers/gpu/drm/drm_panel_backlight_quirks.c
@@ -17,6 +17,15 @@ struct drm_panel_backlight_entry {
 };
 
 static const struct drm_panel_backlight_entry drm_panel_backlight_entries[] = {
+	/* 13 inch matte panel */
+	{
+		.dmi_match.field = DMI_BOARD_VENDOR,
+		.dmi_match.value = "Framework",
+		.ident.panel_id = drm_edid_encode_panel_id('B', 'O', 'E', 0x0bca),
+		.ident.name = "NE135FBM-N41",
+		.quirk.overrides.pwm_min_brightness = true,
+		.quirk.pwm_min_brightness = 0,
+	},
 };
 
 static bool drm_panel_backlight_entry_matches(const struct drm_panel_backlight_entry *entry,

-- 
2.45.2

[PATCH v2 3/3] drm/amd/display: Add support backlight quirks

[Thomas Weißschuh]

Not all platforms provide correct PWM backlight capabilities through ATIF.
Use the generic drm backlight quirk infrastructure to override the
capabilities where necessary.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/gpu/drm/amd/amdgpu/Kconfig                |  1 +
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 28 +++++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig b/drivers/gpu/drm/amd/amdgpu/Kconfig
index 692fa7cf8fd2..4fe0e8e74bb9 100644
--- a/drivers/gpu/drm/amd/amdgpu/Kconfig
+++ b/drivers/gpu/drm/amd/amdgpu/Kconfig
@@ -25,6 +25,7 @@ config DRM_AMDGPU
 	select DRM_BUDDY
 	select DRM_SUBALLOC_HELPER
 	select DRM_EXEC
+	select DRM_PANEL_BACKLIGHT_QUIRKS
 	# amdgpu depends on ACPI_VIDEO when ACPI is enabled, for select to work
 	# ACPI_VIDEO's dependencies must also be selected.
 	select INPUT if ACPI
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 60404385d4d0..2d8a6d875170 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -93,6 +93,7 @@
 #include <drm/drm_fourcc.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_eld.h>
+#include <drm/drm_utils.h>
 #include <drm/drm_vblank.h>
 #include <drm/drm_audio_component.h>
 #include <drm/drm_gem_atomic_helper.h>
@@ -3329,6 +3330,31 @@ static struct drm_mode_config_helper_funcs amdgpu_dm_mode_config_helperfuncs = {
 	.atomic_commit_setup = drm_dp_mst_atomic_setup_commit,
 };
 
+static void amdgpu_dm_apply_backlight_quirks(struct amdgpu_dm_connector *aconnector,
+					     struct amdgpu_dm_backlight_caps *caps)
+{
+	const struct drm_panel_backlight_quirk *quirk;
+	const struct drm_edid *edid;
+
+	edid = drm_edid_alloc(aconnector->edid, EDID_LENGTH * (aconnector->edid->extensions + 1));
+	if (!edid)
+		return;
+
+	quirk = drm_get_panel_backlight_quirk(edid);
+
+	drm_edid_free(edid);
+
+	if (!quirk)
+		return;
+
+	if (quirk->overrides.pwm_min_brightness &&
+	    caps->min_input_signal != quirk->pwm_min_brightness) {
+		drm_info(aconnector->base.dev,
+			 "Quirk: backlight min_input_signal=%d\n", quirk->pwm_min_brightness);
+		caps->min_input_signal = quirk->pwm_min_brightness;
+	}
+}
+
 static void update_connector_ext_caps(struct amdgpu_dm_connector *aconnector)
 {
 	struct amdgpu_dm_backlight_caps *caps;
@@ -3369,6 +3395,8 @@ static void update_connector_ext_caps(struct amdgpu_dm_connector *aconnector)
 		caps->aux_min_input_signal = 0;
 		caps->aux_max_input_signal = 512;
 	}
+
+	amdgpu_dm_apply_backlight_quirks(aconnector, caps);
 }
 
 void amdgpu_dm_update_connector_after_detect(

-- 
2.45.2

Re: [PATCH v2 0/3] drm: backlight quirk infrastructure and lower minimum for Framework AMD 13

[Hans de Goede]

Hi Thomas,

On 6/23/24 10:51 AM, 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.
> 
> Add a generic quirk infrastructure for backlight configuration to
> override the settings provided by the firmware.
> Also add amdgpu as a user of that infrastructure and a quirk for the
> Framework 13 matte panel.
> Most likely this will also work for the glossy panel, but I can't test
> that.
> 
> One solution would be a fixed firmware version, but given that the
> problem exists since the release of the hardware, it has been known for
> a month that the hardware can go lower and there was no acknowledgment
> from Framework in any way, I'd like to explore this alternative
> way forward.

There are many panels where the brightness can go lower then the advertised
minimum brightness by the firmware (e.g. VBT for i915). For most users
the minimum brightness is fine, especially since going lower often may lead
to an unreadable screen when indoors (not in the full sun) during daylight
hours. And some users get confused by the unreadable screen and find it
hard to recover things from this state.

So IMHO we should not be overriding the minimum brightness from the firmware
using quirks because:

a) This is going to be an endless game of whack-a-mole
b) The new value may be too low for certain users / use-cases

With that said I realize that there are also many users who want to have
a lower minimum brightness value for use in the evening, since they find
the available minimum value still too bright. I know some people want this
for e.g. various ThinkPad models too.

So rather then quirking this, with the above mentioned disadvantages I believe
that it would be better to extend the existing video=eDP-1:.... kernel
commandline parsing to allow overriding the minimum brightness in a driver
agnostic way.

The minimum brightness override set this way will still need hooking up
in each driver separately but by using the video=eDP-1:... mechanism
we can document how to do this in driver independent manner. since
I know there have been multiple requests for something like this in
the past I believe that having a single uniform way for users to do this
will be good.

Alternatively we could have each driver have a driver specific module-
parameter for this. Either way I think we need some way for users to
override this as a config/setting tweak rather then use quirks for this.

Regards,

Hans






> 
> Notes:
> 
> * Should the quirk infrastructure be part of drm_edid.c?
> * The current allocation of struct drm_edid in amdgpu is bad.
>   But it is done the same way in other parts of amdgpu.
>   I do have patches migrating amdgpu to proper usage of struct drm_edid [0]
> 
> Mario:
> 
> I intentionally left out the consideration of the firmware version.
> The quirk will stay correct even if the firmware starts reporting
> correct values.
> If there are strong opinions it would be easy to add, though.
> 
> Based on amdgpu/drm-next.
> 
> [0] https://lore.kernel.org/lkml/20240616-amdgpu-edid-bios-v1-1-2874f212b365@weissschuh.net/
> 
> ---
> Changes in v2:
> - Introduce proper drm backlight quirk infrastructure
> - Quirk by EDID and DMI instead of only DMI
> - Limit quirk to only single Framework 13 matte panel
> - Link to v1: https://lore.kernel.org/r/20240610-amdgpu-min-backlight-quirk-v1-1-8459895a5b2a@weissschuh.net
> 
> ---
> Thomas Weißschuh (3):
>       drm: Add panel backlight quirks
>       drm: panel-backlight-quirks: Add Framework 13 matte panel
>       drm/amd/display: Add support backlight quirks
> 
>  Documentation/gpu/drm-kms-helpers.rst             |  3 +
>  drivers/gpu/drm/Kconfig                           |  4 ++
>  drivers/gpu/drm/Makefile                          |  1 +
>  drivers/gpu/drm/amd/amdgpu/Kconfig                |  1 +
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 28 +++++++++
>  drivers/gpu/drm/drm_panel_backlight_quirks.c      | 76 +++++++++++++++++++++++
>  include/drm/drm_utils.h                           | 11 ++++
>  7 files changed, 124 insertions(+)
> ---
> base-commit: 1ecef5589320fd56af599b624d59c355d162ac7b
> change-id: 20240610-amdgpu-min-backlight-quirk-8402fd8e736a
> 
> Best regards,

Re: [PATCH v2 0/3] drm: backlight quirk infrastructure and lower minimum for Framework AMD 13

[Thomas Weißschuh]

Hi Hans!

thanks for your feedback!

On 2024-06-24 11:11:40+0000, Hans de Goede wrote:
> On 6/23/24 10:51 AM, 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.
> > 
> > Add a generic quirk infrastructure for backlight configuration to
> > override the settings provided by the firmware.
> > Also add amdgpu as a user of that infrastructure and a quirk for the
> > Framework 13 matte panel.
> > Most likely this will also work for the glossy panel, but I can't test
> > that.
> > 
> > One solution would be a fixed firmware version, but given that the
> > problem exists since the release of the hardware, it has been known for
> > a month that the hardware can go lower and there was no acknowledgment
> > from Framework in any way, I'd like to explore this alternative
> > way forward.
> 
> There are many panels where the brightness can go lower then the advertised
> minimum brightness by the firmware (e.g. VBT for i915). For most users
> the minimum brightness is fine, especially since going lower often may lead
> to an unreadable screen when indoors (not in the full sun) during daylight
> hours. And some users get confused by the unreadable screen and find it
> hard to recover things from this state.

There are a fair amount of complaints on the Framework forums about this.
And that specific panel is actually readable even at 0% PWM.

> So IMHO we should not be overriding the minimum brightness from the firmware
> using quirks because:
> 
> a) This is going to be an endless game of whack-a-mole

Indeed, but IMO it is better to maintain the list in the kernel than
forcing all users to resort to random forum advise and fiddle with
lowlevel system configuration.

> b) The new value may be too low for certain users / use-cases

The various userspace wrappers already are applying a safety
threshold to not go to "0".
At least gnome-settings-daemon and brightnessctl do not go below 1% of
brightness_max. They already have to deal with panels that can go
completely dark.

> With that said I realize that there are also many users who want to have
> a lower minimum brightness value for use in the evening, since they find
> the available minimum value still too bright. I know some people want this
> for e.g. various ThinkPad models too.

From my experience with ThinkPads, the default brightness range there
was fine for me. But on the Framework 13 AMD it is not.

> So rather then quirking this, with the above mentioned disadvantages I believe
> that it would be better to extend the existing video=eDP-1:.... kernel
> commandline parsing to allow overriding the minimum brightness in a driver
> agnostic way.

I'm not a fan. It seems much too complicated for most users.

Some more background to the Framework 13 AMD case:
The same panel on the Intel variant already goes darker.
The last responses we got from Framework didn't indicate that the high
minimum brightness was intentional [0], [1].
Coincidentally the "12" returned from ATIF matches
AMDGPU_DM_DEFAULT_MIN_BACKLIGHT, so maybe the firmware is just not set
up completely.

> The minimum brightness override set this way will still need hooking up
> in each driver separately but by using the video=eDP-1:... mechanism
> we can document how to do this in driver independent manner. since
> I know there have been multiple requests for something like this in
> the past I believe that having a single uniform way for users to do this
> will be good.
> 
> Alternatively we could have each driver have a driver specific module-
> parameter for this. Either way I think we need some way for users to
> override this as a config/setting tweak rather then use quirks for this.

This also seems much too complicated for normal users.

[0] https://community.frame.work/t/25711/26
[1] https://community.frame.work/t/47036/7

Re: [PATCH v2 0/3] drm: backlight quirk infrastructure and lower minimum for Framework AMD 13

[Hans de Goede]

Hi Thomas,

On 6/24/24 6:15 PM, Thomas Weißschuh wrote:
> Hi Hans!
> 
> thanks for your feedback!
> 
> On 2024-06-24 11:11:40+0000, Hans de Goede wrote:
>> On 6/23/24 10:51 AM, 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.
>>>
>>> Add a generic quirk infrastructure for backlight configuration to
>>> override the settings provided by the firmware.
>>> Also add amdgpu as a user of that infrastructure and a quirk for the
>>> Framework 13 matte panel.
>>> Most likely this will also work for the glossy panel, but I can't test
>>> that.
>>>
>>> One solution would be a fixed firmware version, but given that the
>>> problem exists since the release of the hardware, it has been known for
>>> a month that the hardware can go lower and there was no acknowledgment
>>> from Framework in any way, I'd like to explore this alternative
>>> way forward.
>>
>> There are many panels where the brightness can go lower then the advertised
>> minimum brightness by the firmware (e.g. VBT for i915). For most users
>> the minimum brightness is fine, especially since going lower often may lead
>> to an unreadable screen when indoors (not in the full sun) during daylight
>> hours. And some users get confused by the unreadable screen and find it
>> hard to recover things from this state.
> 
> There are a fair amount of complaints on the Framework forums about this.
> And that specific panel is actually readable even at 0% PWM.

If a lot of Framework users are complaining about this, then maybe Framework
should fix their VBT in a BIOS update ?  That seems like a better solution
then quirking this in the kernel.

> 
>> So IMHO we should not be overriding the minimum brightness from the firmware
>> using quirks because:
>>
>> a) This is going to be an endless game of whack-a-mole
> 
> Indeed, but IMO it is better to maintain the list in the kernel than
> forcing all users to resort to random forum advise and fiddle with
> lowlevel system configuration.

One of the problem is that what is an acceptable minimum brightness
value is subjective. One person's "still too bright" is another
person's "barely readable"

>> b) The new value may be too low for certain users / use-cases
> 
> The various userspace wrappers already are applying a safety
> threshold to not go to "0".
> At least gnome-settings-daemon and brightnessctl do not go below 1% of
> brightness_max. They already have to deal with panels that can go
> completely dark.

Right, something which was added because the minimum brightness value
on VBTs often is broken. Either it is missing or (subjectively) it is
too high.


>> With that said I realize that there are also many users who want to have
>> a lower minimum brightness value for use in the evening, since they find
>> the available minimum value still too bright. I know some people want this
>> for e.g. various ThinkPad models too.
> 
> From my experience with ThinkPads, the default brightness range there
> was fine for me. But on the Framework 13 AMD it is not.
> 
>> So rather then quirking this, with the above mentioned disadvantages I believe
>> that it would be better to extend the existing video=eDP-1:.... kernel
>> commandline parsing to allow overriding the minimum brightness in a driver
>> agnostic way.
> 
> I'm not a fan. It seems much too complicated for most users.

Wanting lower minimum brightness really is mostly a power-user thing
and what is the right value is somewhat subjective and this is an often
heard complained. I really believe that the kernel should NOT get in
the business of adding quirks for this. OTOH given that this is an often
heard complaint having some generic mechanism to override the VBT value
would be good to have.

As for this being too complicated, I fully agree that ideally things
should just work 100% OOTB, which is why I believe that a firmware fix
from Framework would be good. But when things do not work 100% adding
a kernel cmdline option is something which is regularly asked from users /
found in support questions on fora so I don't think this is overly
complicated. I agree it is not ideal but IMHO it is workable.

E.g. on Fedora it would simply be a question of users having to run:

sudo grubby --update-kernel=ALL --args="video=eDP-1:min-brightness=1"

will add the passed in argument to all currently installed (and
future) kernels.

> Some more background to the Framework 13 AMD case:
> The same panel on the Intel variant already goes darker.
> The last responses we got from Framework didn't indicate that the high
> minimum brightness was intentional [0], [1].
> Coincidentally the "12" returned from ATIF matches
> AMDGPU_DM_DEFAULT_MIN_BACKLIGHT, so maybe the firmware is just not set
> up completely.

Right, so I think this should be investigated closer and then get
framework to issue a BIOS fix, not add a quirk mechanism to the kernel.

IIRC the amdgpu driver will use AMDGPU_DM_DEFAULT_MIN_BACKLIGHT when
that setting is 0 in the VBT.

> 
>> The minimum brightness override set this way will still need hooking up
>> in each driver separately but by using the video=eDP-1:... mechanism
>> we can document how to do this in driver independent manner. since
>> I know there have been multiple requests for something like this in
>> the past I believe that having a single uniform way for users to do this
>> will be good.
>>
>> Alternatively we could have each driver have a driver specific module-
>> parameter for this. Either way I think we need some way for users to
>> override this as a config/setting tweak rather then use quirks for this.
> 
> This also seems much too complicated for normal users.

I agree that having a uniform way is better then having per driver
module options.

Regards,

Hans

Re: [PATCH v2 0/3] drm: backlight quirk infrastructure and lower minimum for Framework AMD 13

[Thomas Weißschuh]

Hi Hans,

On 2024-07-18 10:25:18+0000, Hans de Goede wrote:
> On 6/24/24 6:15 PM, Thomas Weißschuh wrote:
> > On 2024-06-24 11:11:40+0000, Hans de Goede wrote:
> >> On 6/23/24 10:51 AM, 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.
> >>>
> >>> Add a generic quirk infrastructure for backlight configuration to
> >>> override the settings provided by the firmware.
> >>> Also add amdgpu as a user of that infrastructure and a quirk for the
> >>> Framework 13 matte panel.
> >>> Most likely this will also work for the glossy panel, but I can't test
> >>> that.
> >>>
> >>> One solution would be a fixed firmware version, but given that the
> >>> problem exists since the release of the hardware, it has been known for
> >>> a month that the hardware can go lower and there was no acknowledgment
> >>> from Framework in any way, I'd like to explore this alternative
> >>> way forward.
> >>
> >> There are many panels where the brightness can go lower then the advertised
> >> minimum brightness by the firmware (e.g. VBT for i915). For most users
> >> the minimum brightness is fine, especially since going lower often may lead
> >> to an unreadable screen when indoors (not in the full sun) during daylight
> >> hours. And some users get confused by the unreadable screen and find it
> >> hard to recover things from this state.
> > 
> > There are a fair amount of complaints on the Framework forums about this.
> > And that specific panel is actually readable even at 0% PWM.
> 
> If a lot of Framework users are complaining about this, then maybe Framework
> should fix their VBT in a BIOS update ?  That seems like a better solution
> then quirking this in the kernel.

Framework has now stated that they will update the VBT for their 13' device. [0]
It won't happen for the 16' one as its out of spec there, although it
has been reported to work.

<snip>

> > From my experience with ThinkPads, the default brightness range there
> > was fine for me. But on the Framework 13 AMD it is not.
> > 
> >> So rather then quirking this, with the above mentioned disadvantages I believe
> >> that it would be better to extend the existing video=eDP-1:.... kernel
> >> commandline parsing to allow overriding the minimum brightness in a driver
> >> agnostic way.
> > 
> > I'm not a fan. It seems much too complicated for most users.
> 
> Wanting lower minimum brightness really is mostly a power-user thing
> and what is the right value is somewhat subjective and this is an often
> heard complained. I really believe that the kernel should NOT get in
> the business of adding quirks for this. OTOH given that this is an often
> heard complaint having some generic mechanism to override the VBT value
> would be good to have.
> 
> As for this being too complicated, I fully agree that ideally things
> should just work 100% OOTB, which is why I believe that a firmware fix
> from Framework would be good. But when things do not work 100% adding
> a kernel cmdline option is something which is regularly asked from users /
> found in support questions on fora so I don't think this is overly
> complicated. I agree it is not ideal but IMHO it is workable.
> 
> E.g. on Fedora it would simply be a question of users having to run:
> 
> sudo grubby --update-kernel=ALL --args="video=eDP-1:min-brightness=1"
> 
> will add the passed in argument to all currently installed (and
> future) kernels.

Thanks for taking the time for your explanations.
I came around to agree with your proposal for a cmdline override.

What to you think about:

void drm_connector_get_cmdline_backlight_overrides(struct drm_connector *connector,
						   struct drm_backlight_override *overrides);

struct drm_backlight_override would look like
struct drm_panel_backlight_quirk from this patch.

> > Some more background to the Framework 13 AMD case:
> > The same panel on the Intel variant already goes darker.
> > The last responses we got from Framework didn't indicate that the high
> > minimum brightness was intentional [0], [1].
> > Coincidentally the "12" returned from ATIF matches
> > AMDGPU_DM_DEFAULT_MIN_BACKLIGHT, so maybe the firmware is just not set
> > up completely.
> 
> Right, so I think this should be investigated closer and then get
> framework to issue a BIOS fix, not add a quirk mechanism to the kernel.
> 
> IIRC the amdgpu driver will use AMDGPU_DM_DEFAULT_MIN_BACKLIGHT when
> that setting is 0 in the VBT.

This is not my reading of the code.
To me it seems "0" will be accepted, which is also why the second "fix"
from [1] works.

> >> The minimum brightness override set this way will still need hooking up
> >> in each driver separately but by using the video=eDP-1:... mechanism
> >> we can document how to do this in driver independent manner. since
> >> I know there have been multiple requests for something like this in
> >> the past I believe that having a single uniform way for users to do this
> >> will be good.
> >>
> >> Alternatively we could have each driver have a driver specific module-
> >> parameter for this. Either way I think we need some way for users to
> >> override this as a config/setting tweak rather then use quirks for this.
> > 
> > This also seems much too complicated for normal users.
> 
> I agree that having a uniform way is better then having per driver
> module options.

Ack.

[0] https://community.frame.work/t/responded-amd-framework-minimum-brightness-too-high-now-with-measurements/47036/12
[1] https://community.frame.work/t/resolved-even-lower-screen-brightness/25711/42

Re: [PATCH v2 0/3] drm: backlight quirk infrastructure and lower minimum for Framework AMD 13

[Hans de Goede]

Hi Thomas,

On 7/20/24 9:31 AM, Thomas Weißschuh wrote:
> Hi Hans,
> 
> On 2024-07-18 10:25:18+0000, Hans de Goede wrote:
>> On 6/24/24 6:15 PM, Thomas Weißschuh wrote:
>>> On 2024-06-24 11:11:40+0000, Hans de Goede wrote:
>>>> On 6/23/24 10:51 AM, 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.
>>>>>
>>>>> Add a generic quirk infrastructure for backlight configuration to
>>>>> override the settings provided by the firmware.
>>>>> Also add amdgpu as a user of that infrastructure and a quirk for the
>>>>> Framework 13 matte panel.
>>>>> Most likely this will also work for the glossy panel, but I can't test
>>>>> that.
>>>>>
>>>>> One solution would be a fixed firmware version, but given that the
>>>>> problem exists since the release of the hardware, it has been known for
>>>>> a month that the hardware can go lower and there was no acknowledgment
>>>>> from Framework in any way, I'd like to explore this alternative
>>>>> way forward.
>>>>
>>>> There are many panels where the brightness can go lower then the advertised
>>>> minimum brightness by the firmware (e.g. VBT for i915). For most users
>>>> the minimum brightness is fine, especially since going lower often may lead
>>>> to an unreadable screen when indoors (not in the full sun) during daylight
>>>> hours. And some users get confused by the unreadable screen and find it
>>>> hard to recover things from this state.
>>>
>>> There are a fair amount of complaints on the Framework forums about this.
>>> And that specific panel is actually readable even at 0% PWM.
>>
>> If a lot of Framework users are complaining about this, then maybe Framework
>> should fix their VBT in a BIOS update ?  That seems like a better solution
>> then quirking this in the kernel.
> 
> Framework has now stated that they will update the VBT for their 13' device. [0]
> It won't happen for the 16' one as its out of spec there, although it
> has been reported to work.
> 
> <snip>
> 
>>> From my experience with ThinkPads, the default brightness range there
>>> was fine for me. But on the Framework 13 AMD it is not.
>>>
>>>> So rather then quirking this, with the above mentioned disadvantages I believe
>>>> that it would be better to extend the existing video=eDP-1:.... kernel
>>>> commandline parsing to allow overriding the minimum brightness in a driver
>>>> agnostic way.
>>>
>>> I'm not a fan. It seems much too complicated for most users.
>>
>> Wanting lower minimum brightness really is mostly a power-user thing
>> and what is the right value is somewhat subjective and this is an often
>> heard complained. I really believe that the kernel should NOT get in
>> the business of adding quirks for this. OTOH given that this is an often
>> heard complaint having some generic mechanism to override the VBT value
>> would be good to have.
>>
>> As for this being too complicated, I fully agree that ideally things
>> should just work 100% OOTB, which is why I believe that a firmware fix
>> from Framework would be good. But when things do not work 100% adding
>> a kernel cmdline option is something which is regularly asked from users /
>> found in support questions on fora so I don't think this is overly
>> complicated. I agree it is not ideal but IMHO it is workable.
>>
>> E.g. on Fedora it would simply be a question of users having to run:
>>
>> sudo grubby --update-kernel=ALL --args="video=eDP-1:min-brightness=1"
>>
>> will add the passed in argument to all currently installed (and
>> future) kernels.
> 
> Thanks for taking the time for your explanations.
> I came around to agree with your proposal for a cmdline override.
> 
> What to you think about:
> 
> void drm_connector_get_cmdline_backlight_overrides(struct drm_connector *connector,
> 						   struct drm_backlight_override *overrides);
> 
> struct drm_backlight_override would look like
> struct drm_panel_backlight_quirk from this patch.

I'm not entirely convinced that we need the struct drm_backlight_override
abstraction right away. Maybe we can start with just a
drm_connector_get_cmdline_min_brightness_override() which just returns an int?

If you prefer to keep the struct drm_backlight_override that is fine too,
we can see what others think when you submit a new version for review.


>>> Some more background to the Framework 13 AMD case:
>>> The same panel on the Intel variant already goes darker.
>>> The last responses we got from Framework didn't indicate that the high
>>> minimum brightness was intentional [0], [1].
>>> Coincidentally the "12" returned from ATIF matches
>>> AMDGPU_DM_DEFAULT_MIN_BACKLIGHT, so maybe the firmware is just not set
>>> up completely.
>>
>> Right, so I think this should be investigated closer and then get
>> framework to issue a BIOS fix, not add a quirk mechanism to the kernel.
>>
>> IIRC the amdgpu driver will use AMDGPU_DM_DEFAULT_MIN_BACKLIGHT when
>> that setting is 0 in the VBT.
> 
> This is not my reading of the code.
> To me it seems "0" will be accepted, which is also why the second "fix"
> from [1] works.

I have not looked at that code i quite a while, so you're probably right.

Regards,

Hans

Re: [PATCH v2 0/3] drm: backlight quirk infrastructure and lower minimum for Framework AMD 13

[Jani Nikula]

On Thu, 18 Jul 2024, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi Thomas,
>
> On 6/24/24 6:15 PM, Thomas Weißschuh wrote:
>> Hi Hans!
>> 
>> thanks for your feedback!
>> 
>> On 2024-06-24 11:11:40+0000, Hans de Goede wrote:
>>> On 6/23/24 10:51 AM, 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.
>>>>
>>>> Add a generic quirk infrastructure for backlight configuration to
>>>> override the settings provided by the firmware.
>>>> Also add amdgpu as a user of that infrastructure and a quirk for the
>>>> Framework 13 matte panel.
>>>> Most likely this will also work for the glossy panel, but I can't test
>>>> that.
>>>>
>>>> One solution would be a fixed firmware version, but given that the
>>>> problem exists since the release of the hardware, it has been known for
>>>> a month that the hardware can go lower and there was no acknowledgment
>>>> from Framework in any way, I'd like to explore this alternative
>>>> way forward.
>>>
>>> There are many panels where the brightness can go lower then the advertised
>>> minimum brightness by the firmware (e.g. VBT for i915). For most users
>>> the minimum brightness is fine, especially since going lower often may lead
>>> to an unreadable screen when indoors (not in the full sun) during daylight
>>> hours. And some users get confused by the unreadable screen and find it
>>> hard to recover things from this state.
>> 
>> There are a fair amount of complaints on the Framework forums about this.
>> And that specific panel is actually readable even at 0% PWM.
>
> If a lot of Framework users are complaining about this, then maybe Framework
> should fix their VBT in a BIOS update ?  That seems like a better solution
> then quirking this in the kernel.
>
>> 
>>> So IMHO we should not be overriding the minimum brightness from the firmware
>>> using quirks because:
>>>
>>> a) This is going to be an endless game of whack-a-mole
>> 
>> Indeed, but IMO it is better to maintain the list in the kernel than
>> forcing all users to resort to random forum advise and fiddle with
>> lowlevel system configuration.
>
> One of the problem is that what is an acceptable minimum brightness
> value is subjective. One person's "still too bright" is another
> person's "barely readable"

Side note, IIRC the minimum brightness in VBT was not originally about
subjective minimums, but rather to avoid electrical issues that 0% PWM
caused in some board designs.

BR,
Jani.


>
>>> b) The new value may be too low for certain users / use-cases
>> 
>> The various userspace wrappers already are applying a safety
>> threshold to not go to "0".
>> At least gnome-settings-daemon and brightnessctl do not go below 1% of
>> brightness_max. They already have to deal with panels that can go
>> completely dark.
>
> Right, something which was added because the minimum brightness value
> on VBTs often is broken. Either it is missing or (subjectively) it is
> too high.
>
>
>>> With that said I realize that there are also many users who want to have
>>> a lower minimum brightness value for use in the evening, since they find
>>> the available minimum value still too bright. I know some people want this
>>> for e.g. various ThinkPad models too.
>> 
>> From my experience with ThinkPads, the default brightness range there
>> was fine for me. But on the Framework 13 AMD it is not.
>> 
>>> So rather then quirking this, with the above mentioned disadvantages I believe
>>> that it would be better to extend the existing video=eDP-1:.... kernel
>>> commandline parsing to allow overriding the minimum brightness in a driver
>>> agnostic way.
>> 
>> I'm not a fan. It seems much too complicated for most users.
>
> Wanting lower minimum brightness really is mostly a power-user thing
> and what is the right value is somewhat subjective and this is an often
> heard complained. I really believe that the kernel should NOT get in
> the business of adding quirks for this. OTOH given that this is an often
> heard complaint having some generic mechanism to override the VBT value
> would be good to have.
>
> As for this being too complicated, I fully agree that ideally things
> should just work 100% OOTB, which is why I believe that a firmware fix
> from Framework would be good. But when things do not work 100% adding
> a kernel cmdline option is something which is regularly asked from users /
> found in support questions on fora so I don't think this is overly
> complicated. I agree it is not ideal but IMHO it is workable.
>
> E.g. on Fedora it would simply be a question of users having to run:
>
> sudo grubby --update-kernel=ALL --args="video=eDP-1:min-brightness=1"
>
> will add the passed in argument to all currently installed (and
> future) kernels.
>
>> Some more background to the Framework 13 AMD case:
>> The same panel on the Intel variant already goes darker.
>> The last responses we got from Framework didn't indicate that the high
>> minimum brightness was intentional [0], [1].
>> Coincidentally the "12" returned from ATIF matches
>> AMDGPU_DM_DEFAULT_MIN_BACKLIGHT, so maybe the firmware is just not set
>> up completely.
>
> Right, so I think this should be investigated closer and then get
> framework to issue a BIOS fix, not add a quirk mechanism to the kernel.
>
> IIRC the amdgpu driver will use AMDGPU_DM_DEFAULT_MIN_BACKLIGHT when
> that setting is 0 in the VBT.
>
>> 
>>> The minimum brightness override set this way will still need hooking up
>>> in each driver separately but by using the video=eDP-1:... mechanism
>>> we can document how to do this in driver independent manner. since
>>> I know there have been multiple requests for something like this in
>>> the past I believe that having a single uniform way for users to do this
>>> will be good.
>>>
>>> Alternatively we could have each driver have a driver specific module-
>>> parameter for this. Either way I think we need some way for users to
>>> override this as a config/setting tweak rather then use quirks for this.
>> 
>> This also seems much too complicated for normal users.
>
> I agree that having a uniform way is better then having per driver
> module options.
>
> Regards,
>
> Hans
>

-- 
Jani Nikula, Intel

Re: [PATCH v2 0/3] drm: backlight quirk infrastructure and lower minimum for Framework AMD 13

[Alex Deucher]

On Wed, Jul 24, 2024 at 4:58 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
> On Thu, 18 Jul 2024, Hans de Goede <hdegoede@redhat.com> wrote:
> > Hi Thomas,
> >
> > On 6/24/24 6:15 PM, Thomas Weißschuh wrote:
> >> Hi Hans!
> >>
> >> thanks for your feedback!
> >>
> >> On 2024-06-24 11:11:40+0000, Hans de Goede wrote:
> >>> On 6/23/24 10:51 AM, 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.
> >>>>
> >>>> Add a generic quirk infrastructure for backlight configuration to
> >>>> override the settings provided by the firmware.
> >>>> Also add amdgpu as a user of that infrastructure and a quirk for the
> >>>> Framework 13 matte panel.
> >>>> Most likely this will also work for the glossy panel, but I can't test
> >>>> that.
> >>>>
> >>>> One solution would be a fixed firmware version, but given that the
> >>>> problem exists since the release of the hardware, it has been known for
> >>>> a month that the hardware can go lower and there was no acknowledgment
> >>>> from Framework in any way, I'd like to explore this alternative
> >>>> way forward.
> >>>
> >>> There are many panels where the brightness can go lower then the advertised
> >>> minimum brightness by the firmware (e.g. VBT for i915). For most users
> >>> the minimum brightness is fine, especially since going lower often may lead
> >>> to an unreadable screen when indoors (not in the full sun) during daylight
> >>> hours. And some users get confused by the unreadable screen and find it
> >>> hard to recover things from this state.
> >>
> >> There are a fair amount of complaints on the Framework forums about this.
> >> And that specific panel is actually readable even at 0% PWM.
> >
> > If a lot of Framework users are complaining about this, then maybe Framework
> > should fix their VBT in a BIOS update ?  That seems like a better solution
> > then quirking this in the kernel.
> >
> >>
> >>> So IMHO we should not be overriding the minimum brightness from the firmware
> >>> using quirks because:
> >>>
> >>> a) This is going to be an endless game of whack-a-mole
> >>
> >> Indeed, but IMO it is better to maintain the list in the kernel than
> >> forcing all users to resort to random forum advise and fiddle with
> >> lowlevel system configuration.
> >
> > One of the problem is that what is an acceptable minimum brightness
> > value is subjective. One person's "still too bright" is another
> > person's "barely readable"
>
> Side note, IIRC the minimum brightness in VBT was not originally about
> subjective minimums, but rather to avoid electrical issues that 0% PWM
> caused in some board designs.

It's the same on AMD.  There was undesirable behavior on some panels
if the level dropped below a certain threshold.

Alex

>
> BR,
> Jani.
>
>
> >
> >>> b) The new value may be too low for certain users / use-cases
> >>
> >> The various userspace wrappers already are applying a safety
> >> threshold to not go to "0".
> >> At least gnome-settings-daemon and brightnessctl do not go below 1% of
> >> brightness_max. They already have to deal with panels that can go
> >> completely dark.
> >
> > Right, something which was added because the minimum brightness value
> > on VBTs often is broken. Either it is missing or (subjectively) it is
> > too high.
> >
> >
> >>> With that said I realize that there are also many users who want to have
> >>> a lower minimum brightness value for use in the evening, since they find
> >>> the available minimum value still too bright. I know some people want this
> >>> for e.g. various ThinkPad models too.
> >>
> >> From my experience with ThinkPads, the default brightness range there
> >> was fine for me. But on the Framework 13 AMD it is not.
> >>
> >>> So rather then quirking this, with the above mentioned disadvantages I believe
> >>> that it would be better to extend the existing video=eDP-1:.... kernel
> >>> commandline parsing to allow overriding the minimum brightness in a driver
> >>> agnostic way.
> >>
> >> I'm not a fan. It seems much too complicated for most users.
> >
> > Wanting lower minimum brightness really is mostly a power-user thing
> > and what is the right value is somewhat subjective and this is an often
> > heard complained. I really believe that the kernel should NOT get in
> > the business of adding quirks for this. OTOH given that this is an often
> > heard complaint having some generic mechanism to override the VBT value
> > would be good to have.
> >
> > As for this being too complicated, I fully agree that ideally things
> > should just work 100% OOTB, which is why I believe that a firmware fix
> > from Framework would be good. But when things do not work 100% adding
> > a kernel cmdline option is something which is regularly asked from users /
> > found in support questions on fora so I don't think this is overly
> > complicated. I agree it is not ideal but IMHO it is workable.
> >
> > E.g. on Fedora it would simply be a question of users having to run:
> >
> > sudo grubby --update-kernel=ALL --args="video=eDP-1:min-brightness=1"
> >
> > will add the passed in argument to all currently installed (and
> > future) kernels.
> >
> >> Some more background to the Framework 13 AMD case:
> >> The same panel on the Intel variant already goes darker.
> >> The last responses we got from Framework didn't indicate that the high
> >> minimum brightness was intentional [0], [1].
> >> Coincidentally the "12" returned from ATIF matches
> >> AMDGPU_DM_DEFAULT_MIN_BACKLIGHT, so maybe the firmware is just not set
> >> up completely.
> >
> > Right, so I think this should be investigated closer and then get
> > framework to issue a BIOS fix, not add a quirk mechanism to the kernel.
> >
> > IIRC the amdgpu driver will use AMDGPU_DM_DEFAULT_MIN_BACKLIGHT when
> > that setting is 0 in the VBT.
> >
> >>
> >>> The minimum brightness override set this way will still need hooking up
> >>> in each driver separately but by using the video=eDP-1:... mechanism
> >>> we can document how to do this in driver independent manner. since
> >>> I know there have been multiple requests for something like this in
> >>> the past I believe that having a single uniform way for users to do this
> >>> will be good.
> >>>
> >>> Alternatively we could have each driver have a driver specific module-
> >>> parameter for this. Either way I think we need some way for users to
> >>> override this as a config/setting tweak rather then use quirks for this.
> >>
> >> This also seems much too complicated for normal users.
> >
> > I agree that having a uniform way is better then having per driver
> > module options.
> >
> > Regards,
> >
> > Hans
> >
>
> --
> Jani Nikula, Intel

Re: [PATCH v2 0/3] drm: backlight quirk infrastructure and lower minimum for Framework AMD 13

[Mario Limonciello]

On 6/23/2024 3:51, 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.
> 
> Add a generic quirk infrastructure for backlight configuration to
> override the settings provided by the firmware.
> Also add amdgpu as a user of that infrastructure and a quirk for the
> Framework 13 matte panel.
> Most likely this will also work for the glossy panel, but I can't test
> that.
> 
> One solution would be a fixed firmware version, but given that the
> problem exists since the release of the hardware, it has been known for
> a month that the hardware can go lower and there was no acknowledgment
> from Framework in any way, I'd like to explore this alternative
> way forward.
> 
> Notes:
> 
> * Should the quirk infrastructure be part of drm_edid.c?
> * The current allocation of struct drm_edid in amdgpu is bad.
>    But it is done the same way in other parts of amdgpu.
>    I do have patches migrating amdgpu to proper usage of struct drm_edid [0]
> 
> Mario:
> 
> I intentionally left out the consideration of the firmware version.
> The quirk will stay correct even if the firmware starts reporting
> correct values.
> If there are strong opinions it would be easy to add, though.
> 
> Based on amdgpu/drm-next.
> 
> [0] https://lore.kernel.org/lkml/20240616-amdgpu-edid-bios-v1-1-2874f212b365@weissschuh.net/
> 

Thomas,

Are you doing any testing of this lower backlight value specifically 
with panel power savings enabled?  If not can you please check that?

Specifically write the maximum value of "4" to the sysfs file:

/sys/class/drm/card0-eDP-1/amdgpu/panel_power_savings

Thanks,

> ---
> Changes in v2:
> - Introduce proper drm backlight quirk infrastructure
> - Quirk by EDID and DMI instead of only DMI
> - Limit quirk to only single Framework 13 matte panel
> - Link to v1: https://lore.kernel.org/r/20240610-amdgpu-min-backlight-quirk-v1-1-8459895a5b2a@weissschuh.net
> 
> ---
> Thomas Weißschuh (3):
>        drm: Add panel backlight quirks
>        drm: panel-backlight-quirks: Add Framework 13 matte panel
>        drm/amd/display: Add support backlight quirks
> 
>   Documentation/gpu/drm-kms-helpers.rst             |  3 +
>   drivers/gpu/drm/Kconfig                           |  4 ++
>   drivers/gpu/drm/Makefile                          |  1 +
>   drivers/gpu/drm/amd/amdgpu/Kconfig                |  1 +
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 28 +++++++++
>   drivers/gpu/drm/drm_panel_backlight_quirks.c      | 76 +++++++++++++++++++++++
>   include/drm/drm_utils.h                           | 11 ++++
>   7 files changed, 124 insertions(+)
> ---
> base-commit: 1ecef5589320fd56af599b624d59c355d162ac7b
> change-id: 20240610-amdgpu-min-backlight-quirk-8402fd8e736a
> 
> Best regards,

Re: [PATCH v2 0/3] drm: backlight quirk infrastructure and lower minimum for Framework AMD 13

[Hans de Goede]

Hi Thomas,

On 6/23/24 10:51 AM, 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.
> 
> Add a generic quirk infrastructure for backlight configuration to
> override the settings provided by the firmware.
> Also add amdgpu as a user of that infrastructure and a quirk for the
> Framework 13 matte panel.
> Most likely this will also work for the glossy panel, but I can't test
> that.
> 
> One solution would be a fixed firmware version, but given that the
> problem exists since the release of the hardware, it has been known for
> a month that the hardware can go lower and there was no acknowledgment
> from Framework in any way, I'd like to explore this alternative
> way forward.
> 
> Notes:
> 
> * Should the quirk infrastructure be part of drm_edid.c?
> * The current allocation of struct drm_edid in amdgpu is bad.
>   But it is done the same way in other parts of amdgpu.
>   I do have patches migrating amdgpu to proper usage of struct drm_edid [0]

So now that we have agreed to move forward with this approach one
generic design issue / question which popped into my head is:

What happens if i915 also gets support for the minimum brightness quirk?

Specifically the panel used on the framework 13 will match the quirk
independent of which GPU driver is used. But does say a minimum value
of "5" have the same meaning / results in the same minimum duty-cycle
when used by the i915 code vs the amdgpu code ?

I know that the actual quirk sets the min pwm to 0, which presumably results
in a 0% duty cycle on both i915 and amdgpu, but I'm worried what happens
if we see the same panel used in designs which can have it attached to
different vendor GPUs, how should the GPU driver interpret the
pwm_min_brightness value so that it is interpretted consistently between
drivers ? 

I guess this is covered by the docbook saying that min-brightness is on
a 0-255 brightness scale (with 255 being 100%) though ?  Just making sure
that everyone has agreement on that being the scale and that drivers
should not directly take this value but scale it as necessary.
This should also (scale it as necessary) be explicitly mentioned in
the docs IMHO.

Regards,

Hans






> Mario:
> 
> I intentionally left out the consideration of the firmware version.
> The quirk will stay correct even if the firmware starts reporting
> correct values.
> If there are strong opinions it would be easy to add, though.
> 
> Based on amdgpu/drm-next.
> 
> [0] https://lore.kernel.org/lkml/20240616-amdgpu-edid-bios-v1-1-2874f212b365@weissschuh.net/
> 
> ---
> Changes in v2:
> - Introduce proper drm backlight quirk infrastructure
> - Quirk by EDID and DMI instead of only DMI
> - Limit quirk to only single Framework 13 matte panel
> - Link to v1: https://lore.kernel.org/r/20240610-amdgpu-min-backlight-quirk-v1-1-8459895a5b2a@weissschuh.net
> 
> ---
> Thomas Weißschuh (3):
>       drm: Add panel backlight quirks
>       drm: panel-backlight-quirks: Add Framework 13 matte panel
>       drm/amd/display: Add support backlight quirks
> 
>  Documentation/gpu/drm-kms-helpers.rst             |  3 +
>  drivers/gpu/drm/Kconfig                           |  4 ++
>  drivers/gpu/drm/Makefile                          |  1 +
>  drivers/gpu/drm/amd/amdgpu/Kconfig                |  1 +
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 28 +++++++++
>  drivers/gpu/drm/drm_panel_backlight_quirks.c      | 76 +++++++++++++++++++++++
>  include/drm/drm_utils.h                           | 11 ++++
>  7 files changed, 124 insertions(+)
> ---
> base-commit: 1ecef5589320fd56af599b624d59c355d162ac7b
> change-id: 20240610-amdgpu-min-backlight-quirk-8402fd8e736a
> 
> Best regards,