[PATCH v5 0/4] drm: Minimum backlight overrides and implementation for amdgpu

[PATCH v5 0/4] drm: Minimum backlight overrides and implementation for amdgpu

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

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.

One solution would be a fixed firmware version, which was announced but
has no timeline.

---
Changes in v5:
- Forward-declare struct drm_edid
- Reorder patches, quirk entries are last
- Add patch from Dustin for additional quirk entries
- Link to v4: https://lore.kernel.org/r/20240812-amdgpu-min-backlight-quirk-v4-0-56a63ff897b7@weissschuh.net

Changes in v4:
- Switch back to v2 implementation
- Add MODULE_DESCRIPTION()
- Simplify quirk infrastructure to only handle min backlight quirks.
  It can be extended if necessary.
- Expand documentation.
- Link to v3: https://lore.kernel.org/r/20240731-amdgpu-min-backlight-quirk-v3-0-46d40bb21a62@weissschuh.net

Changes in v3:
- Switch to cmdline override parameter
- Link to v2: https://lore.kernel.org/r/20240623-amdgpu-min-backlight-quirk-v2-0-cecf7f49da9b@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

---
Dustin L. Howett (1):
      drm: panel-backlight-quirks: Add Framework 13 glossy and 2.8k panels

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

 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 | 10 +++
 drivers/gpu/drm/drm_panel_backlight_quirks.c      | 94 +++++++++++++++++++++++
 include/drm/drm_utils.h                           |  4 +
 7 files changed, 117 insertions(+)
---
base-commit: c3f2d783a459980eafd24c5af94ccd56a615961f
change-id: 20240610-amdgpu-min-backlight-quirk-8402fd8e736a

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

[PATCH v5 1/4] drm: Add panel backlight quirks

[Thomas Weißschuh]

Panels using a PWM-controlled backlight source 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 minimum valid backlight value
can be maintained as part of the kernel.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
Tested-by: Dustin L. Howett <dustin@howett.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 | 70 ++++++++++++++++++++++++++++
 include/drm/drm_utils.h                      |  4 ++
 5 files changed, 82 insertions(+)

diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
index 8435e8621cc0..a26989500129 100644
--- a/Documentation/gpu/drm-kms-helpers.rst
+++ b/Documentation/gpu/drm-kms-helpers.rst
@@ -230,6 +230,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 6b2c6b91f962..9ebb8cdb535e 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -454,6 +454,10 @@ config DRM_HYPERV
 config DRM_EXPORT_FOR_TESTS
 	bool
 
+# 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 68cc9258ffc4..adf85999aee2 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..6b8bbed77c7f
--- /dev/null
+++ b/drivers/gpu/drm/drm_panel_backlight_quirks.c
@@ -0,0 +1,70 @@
+// 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_edid.h>
+#include <drm/drm_utils.h>
+
+struct drm_panel_min_backlight_quirk {
+	struct {
+		enum dmi_field field;
+		const char * const value;
+	} dmi_match;
+	struct drm_edid_ident ident;
+	u8 min_brightness;
+};
+
+static const struct drm_panel_min_backlight_quirk drm_panel_min_backlight_quirks[] = {
+};
+
+static bool drm_panel_min_backlight_quirk_matches(const struct drm_panel_min_backlight_quirk *quirk,
+						  const struct drm_edid *edid)
+{
+	if (!dmi_match(quirk->dmi_match.field, quirk->dmi_match.value))
+		return false;
+
+	if (!drm_edid_match(edid, &quirk->ident))
+		return false;
+
+	return true;
+}
+
+/**
+ * drm_get_panel_min_brightness_quirk - Get minimum supported brightness level for a panel.
+ * @edid: EDID of the panel to check
+ *
+ * This function checks for platform specific (e.g. DMI based) quirks
+ * providing info on the minimum backlight brightness for systems where this
+ * cannot be probed correctly from the hard-/firm-ware.
+ *
+ * Returns:
+ * A negative error value or
+ * an override value in the range [0, 255] representing 0-100% to be scaled to
+ * the drivers target range.
+ */
+int drm_get_panel_min_brightness_quirk(const struct drm_edid *edid)
+{
+	const struct drm_panel_min_backlight_quirk *quirk;
+	size_t i;
+
+	if (!IS_ENABLED(CONFIG_DMI))
+		return -ENODATA;
+
+	if (!edid)
+		return -EINVAL;
+
+	for (i = 0; i < ARRAY_SIZE(drm_panel_min_backlight_quirks); i++) {
+		quirk = &drm_panel_min_backlight_quirks[i];
+
+		if (drm_panel_min_backlight_quirk_matches(quirk, edid))
+			return quirk->min_brightness;
+	}
+
+	return -ENODATA;
+}
+EXPORT_SYMBOL(drm_get_panel_min_brightness_quirk);
+
+MODULE_DESCRIPTION("Quirks for panel backlight overrides");
+MODULE_LICENSE("GPL");
diff --git a/include/drm/drm_utils.h b/include/drm/drm_utils.h
index 70775748d243..15fa9b6865f4 100644
--- a/include/drm/drm_utils.h
+++ b/include/drm/drm_utils.h
@@ -12,8 +12,12 @@
 
 #include <linux/types.h>
 
+struct drm_edid;
+
 int drm_get_panel_orientation_quirk(int width, int height);
 
+int drm_get_panel_min_brightness_quirk(const struct drm_edid *edid);
+
 signed long drm_timeout_abs_to_jiffies(int64_t timeout_nsec);
 
 #endif

-- 
2.46.0

[PATCH v5 2/4] drm/amd/display: Add support for minimum backlight quirk

[Thomas Weißschuh]

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

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
Tested-by: Dustin L. Howett <dustin@howett.net>
---
 drivers/gpu/drm/amd/amdgpu/Kconfig                |  1 +
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig b/drivers/gpu/drm/amd/amdgpu/Kconfig
index 0051fb1b437f..655c10aef2e3 100644
--- a/drivers/gpu/drm/amd/amdgpu/Kconfig
+++ b/drivers/gpu/drm/amd/amdgpu/Kconfig
@@ -23,6 +23,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 983a977632ff..056960ea335c 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>
@@ -3333,6 +3334,8 @@ static void update_connector_ext_caps(struct amdgpu_dm_connector *aconnector)
 	struct drm_connector *conn_base;
 	struct amdgpu_device *adev;
 	struct drm_luminance_range_info *luminance_range;
+	const struct drm_edid *drm_edid;
+	int min_input_signal_override;
 
 	if (aconnector->bl_idx == -1 ||
 	    aconnector->dc_link->connector_signal != SIGNAL_TYPE_EDP)
@@ -3367,6 +3370,13 @@ static void update_connector_ext_caps(struct amdgpu_dm_connector *aconnector)
 		caps->aux_min_input_signal = 0;
 		caps->aux_max_input_signal = 512;
 	}
+
+	drm_edid = drm_edid_alloc(aconnector->edid,
+				  EDID_LENGTH * (aconnector->edid->extensions + 1));
+	min_input_signal_override = drm_get_panel_min_brightness_quirk(drm_edid);
+	drm_edid_free(drm_edid);
+	if (min_input_signal_override >= 0)
+		caps->min_input_signal = min_input_signal_override;
 }
 
 void amdgpu_dm_update_connector_after_detect(

-- 
2.46.0

[PATCH v5 3/4] 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>
Tested-by: Dustin L. Howett <dustin@howett.net>
---
 drivers/gpu/drm/drm_panel_backlight_quirks.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/drm_panel_backlight_quirks.c b/drivers/gpu/drm/drm_panel_backlight_quirks.c
index 6b8bbed77c7f..f2aefff618dd 100644
--- a/drivers/gpu/drm/drm_panel_backlight_quirks.c
+++ b/drivers/gpu/drm/drm_panel_backlight_quirks.c
@@ -17,6 +17,14 @@ struct drm_panel_min_backlight_quirk {
 };
 
 static const struct drm_panel_min_backlight_quirk drm_panel_min_backlight_quirks[] = {
+	/* 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",
+		.min_brightness = 0,
+	},
 };
 
 static bool drm_panel_min_backlight_quirk_matches(const struct drm_panel_min_backlight_quirk *quirk,

-- 
2.46.0

[PATCH v5 4/4] drm: panel-backlight-quirks: Add Framework 13 glossy and 2.8k panels

[Thomas Weißschuh]

From: "Dustin L. Howett" <dustin@howett.net>

I have tested these panels on the Framework Laptop 13 AMD with firmware
revision 3.05 (latest at time of submission).

Signed-off-by: Dustin L. Howett <dustin@howett.net>
---
 drivers/gpu/drm/drm_panel_backlight_quirks.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/drm_panel_backlight_quirks.c b/drivers/gpu/drm/drm_panel_backlight_quirks.c
index f2aefff618dd..c477d98ade2b 100644
--- a/drivers/gpu/drm/drm_panel_backlight_quirks.c
+++ b/drivers/gpu/drm/drm_panel_backlight_quirks.c
@@ -25,6 +25,22 @@ static const struct drm_panel_min_backlight_quirk drm_panel_min_backlight_quirks
 		.ident.name = "NE135FBM-N41",
 		.min_brightness = 0,
 	},
+	/* 13 inch glossy panel */
+	{
+		.dmi_match.field = DMI_BOARD_VENDOR,
+		.dmi_match.value = "Framework",
+		.ident.panel_id = drm_edid_encode_panel_id('B', 'O', 'E', 0x095f),
+		.ident.name = "NE135FBM-N41",
+		.min_brightness = 0,
+	},
+	/* 13 inch 2.8k panel */
+	{
+		.dmi_match.field = DMI_BOARD_VENDOR,
+		.dmi_match.value = "Framework",
+		.ident.panel_id = drm_edid_encode_panel_id('B', 'O', 'E', 0x0cb4),
+		.ident.name = "NE135A1M-NY1",
+		.min_brightness = 0,
+	},
 };
 
 static bool drm_panel_min_backlight_quirk_matches(const struct drm_panel_min_backlight_quirk *quirk,

-- 
2.46.0

Re: [PATCH v5 0/4] drm: Minimum backlight overrides and implementation for amdgpu

[Mario Limonciello]

On 8/18/2024 01:56, 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.
> 

Apologies for my delayed response, I've been OOO a while.

An earlier version of this series I had asked if this could be tested 
specifically with panel_power_savings enabled to ensure we're not 
causing a regression with such a low minimum and panel power savings 
turned up to the maximum.  If such a dependency does exist, I believe 
panel power savings will need to get an extra check introduced to 
disable it when the brightness falls below the previous threshold that 
was programmed by the firmware.

Did that ever get checked?

> Tested on a Framework AMD 13 BIOS 3.05 and Framework AMD 16.
> 

As far as I can tell this series doesn't apply any quirks to Framework 
16.  So by testing, do you mean that you tested that it didn't cause 
regressions (which it shouldn't; the panels don't match!).

> One solution would be a fixed firmware version, which was announced but
> has no timeline. >
> ---
> Changes in v5:
> - Forward-declare struct drm_edid
> - Reorder patches, quirk entries are last
> - Add patch from Dustin for additional quirk entries
> - Link to v4: https://lore.kernel.org/r/20240812-amdgpu-min-backlight-quirk-v4-0-56a63ff897b7@weissschuh.net
> 
> Changes in v4:
> - Switch back to v2 implementation
> - Add MODULE_DESCRIPTION()
> - Simplify quirk infrastructure to only handle min backlight quirks.
>    It can be extended if necessary.
> - Expand documentation.
> - Link to v3: https://lore.kernel.org/r/20240731-amdgpu-min-backlight-quirk-v3-0-46d40bb21a62@weissschuh.net
> 
> Changes in v3:
> - Switch to cmdline override parameter
> - Link to v2: https://lore.kernel.org/r/20240623-amdgpu-min-backlight-quirk-v2-0-cecf7f49da9b@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
> 
> ---
> Dustin L. Howett (1):
>        drm: panel-backlight-quirks: Add Framework 13 glossy and 2.8k panels
> 
> Thomas Weißschuh (3):
>        drm: Add panel backlight quirks
>        drm/amd/display: Add support for minimum backlight quirk
>        drm: panel-backlight-quirks: Add Framework 13 matte panel
> 
>   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 | 10 +++
>   drivers/gpu/drm/drm_panel_backlight_quirks.c      | 94 +++++++++++++++++++++++
>   include/drm/drm_utils.h                           |  4 +
>   7 files changed, 117 insertions(+)
> ---
> base-commit: c3f2d783a459980eafd24c5af94ccd56a615961f
> change-id: 20240610-amdgpu-min-backlight-quirk-8402fd8e736a
> 
> Best regards,

Re: [PATCH v5 1/4] drm: Add panel backlight quirks

[Mario Limonciello]

On 8/18/2024 01:56, Thomas Weißschuh wrote:
> Panels using a PWM-controlled backlight source 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 minimum valid backlight value
> can be maintained as part of the kernel.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> Tested-by: Dustin L. Howett <dustin@howett.net>

One small nit below but otherwise this patch is fine to me.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

> ---
>   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 | 70 ++++++++++++++++++++++++++++
>   include/drm/drm_utils.h                      |  4 ++
>   5 files changed, 82 insertions(+)
> 
> diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
> index 8435e8621cc0..a26989500129 100644
> --- a/Documentation/gpu/drm-kms-helpers.rst
> +++ b/Documentation/gpu/drm-kms-helpers.rst
> @@ -230,6 +230,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 6b2c6b91f962..9ebb8cdb535e 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -454,6 +454,10 @@ config DRM_HYPERV
>   config DRM_EXPORT_FOR_TESTS
>   	bool
>   
> +# 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 68cc9258ffc4..adf85999aee2 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..6b8bbed77c7f
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_panel_backlight_quirks.c
> @@ -0,0 +1,70 @@
> +// 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_edid.h>
> +#include <drm/drm_utils.h>
> +
> +struct drm_panel_min_backlight_quirk {
> +	struct {
> +		enum dmi_field field;
> +		const char * const value;
> +	} dmi_match;
> +	struct drm_edid_ident ident;
> +	u8 min_brightness;
> +};
> +
> +static const struct drm_panel_min_backlight_quirk drm_panel_min_backlight_quirks[] = {
> +};
> +
> +static bool drm_panel_min_backlight_quirk_matches(const struct drm_panel_min_backlight_quirk *quirk,
> +						  const struct drm_edid *edid)
> +{
> +	if (!dmi_match(quirk->dmi_match.field, quirk->dmi_match.value))
> +		return false;
> +
> +	if (!drm_edid_match(edid, &quirk->ident))
> +		return false;
> +
> +	return true;
> +}
> +
> +/**
> + * drm_get_panel_min_brightness_quirk - Get minimum supported brightness level for a panel.
> + * @edid: EDID of the panel to check
> + *
> + * This function checks for platform specific (e.g. DMI based) quirks
> + * providing info on the minimum backlight brightness for systems where this
> + * cannot be probed correctly from the hard-/firm-ware.
> + *
> + * Returns:
> + * A negative error value or
> + * an override value in the range [0, 255] representing 0-100% to be scaled to
> + * the drivers target range.
> + */
> +int drm_get_panel_min_brightness_quirk(const struct drm_edid *edid)
> +{
> +	const struct drm_panel_min_backlight_quirk *quirk;
> +	size_t i;

Nit: this doesn't really seem like it needs to be size_t.  Shouldn't it 
just be an unsigned int?

> +
> +	if (!IS_ENABLED(CONFIG_DMI))
> +		return -ENODATA;
> +
> +	if (!edid)
> +		return -EINVAL;
> +
> +	for (i = 0; i < ARRAY_SIZE(drm_panel_min_backlight_quirks); i++) {
> +		quirk = &drm_panel_min_backlight_quirks[i];
> +
> +		if (drm_panel_min_backlight_quirk_matches(quirk, edid))
> +			return quirk->min_brightness;
> +	}
> +
> +	return -ENODATA;
> +}
> +EXPORT_SYMBOL(drm_get_panel_min_brightness_quirk);
> +
> +MODULE_DESCRIPTION("Quirks for panel backlight overrides");
> +MODULE_LICENSE("GPL");
> diff --git a/include/drm/drm_utils.h b/include/drm/drm_utils.h
> index 70775748d243..15fa9b6865f4 100644
> --- a/include/drm/drm_utils.h
> +++ b/include/drm/drm_utils.h
> @@ -12,8 +12,12 @@
>   
>   #include <linux/types.h>
>   
> +struct drm_edid;
> +
>   int drm_get_panel_orientation_quirk(int width, int height);
>   
> +int drm_get_panel_min_brightness_quirk(const struct drm_edid *edid);
> +
>   signed long drm_timeout_abs_to_jiffies(int64_t timeout_nsec);
>   
>   #endif
> 

Re: [PATCH v5 2/4] drm/amd/display: Add support for minimum backlight quirk

[Mario Limonciello]

On 8/18/2024 01:56, Thomas Weißschuh wrote:
> Not all platforms provide correct PWM backlight capabilities through ATIF.

I don't think correct is an accurate term here.  How about 'optimial'?

> Use the generic drm panel minimum backlight quirk infrastructure to
> override the capabilities where necessary.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> Tested-by: Dustin L. Howett <dustin@howett.net>

The code looks fine to me but please wait for an ack from someone on the 
AMD display team.

Also, I would like to see comments about the testing with panel power 
savings enabled to avoid a conflict.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/Kconfig                |  1 +
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 ++++++++++
>   2 files changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig b/drivers/gpu/drm/amd/amdgpu/Kconfig
> index 0051fb1b437f..655c10aef2e3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/Kconfig
> +++ b/drivers/gpu/drm/amd/amdgpu/Kconfig
> @@ -23,6 +23,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 983a977632ff..056960ea335c 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>
> @@ -3333,6 +3334,8 @@ static void update_connector_ext_caps(struct amdgpu_dm_connector *aconnector)
>   	struct drm_connector *conn_base;
>   	struct amdgpu_device *adev;
>   	struct drm_luminance_range_info *luminance_range;
> +	const struct drm_edid *drm_edid;
> +	int min_input_signal_override;
>   
>   	if (aconnector->bl_idx == -1 ||
>   	    aconnector->dc_link->connector_signal != SIGNAL_TYPE_EDP)
> @@ -3367,6 +3370,13 @@ static void update_connector_ext_caps(struct amdgpu_dm_connector *aconnector)
>   		caps->aux_min_input_signal = 0;
>   		caps->aux_max_input_signal = 512;
>   	}
> +
> +	drm_edid = drm_edid_alloc(aconnector->edid,
> +				  EDID_LENGTH * (aconnector->edid->extensions + 1));
> +	min_input_signal_override = drm_get_panel_min_brightness_quirk(drm_edid);
> +	drm_edid_free(drm_edid);
> +	if (min_input_signal_override >= 0)
> +		caps->min_input_signal = min_input_signal_override;
>   }
>   
>   void amdgpu_dm_update_connector_after_detect(
> 

Re: [PATCH v5 3/4] drm: panel-backlight-quirks: Add Framework 13 matte panel

[Mario Limonciello]

On 8/18/2024 01:56, 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 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>
> Tested-by: Dustin L. Howett <dustin@howett.net>
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   drivers/gpu/drm/drm_panel_backlight_quirks.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_panel_backlight_quirks.c b/drivers/gpu/drm/drm_panel_backlight_quirks.c
> index 6b8bbed77c7f..f2aefff618dd 100644
> --- a/drivers/gpu/drm/drm_panel_backlight_quirks.c
> +++ b/drivers/gpu/drm/drm_panel_backlight_quirks.c
> @@ -17,6 +17,14 @@ struct drm_panel_min_backlight_quirk {
>   };
>   
>   static const struct drm_panel_min_backlight_quirk drm_panel_min_backlight_quirks[] = {
> +	/* 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",
> +		.min_brightness = 0,
> +	},
>   };
>   
>   static bool drm_panel_min_backlight_quirk_matches(const struct drm_panel_min_backlight_quirk *quirk,
> 

Re: [PATCH v5 4/4] drm: panel-backlight-quirks: Add Framework 13 glossy and 2.8k panels

[Mario Limonciello]

On 8/18/2024 01:56, Thomas Weißschuh wrote:
> From: "Dustin L. Howett" <dustin@howett.net>
> 
> I have tested these panels on the Framework Laptop 13 AMD with firmware
> revision 3.05 (latest at time of submission).
> 
> Signed-off-by: Dustin L. Howett <dustin@howett.net>

When you send someone else's patch you need to add your own S-o-b as 
well.  Please add that for v6.  Otherwise:

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

> ---
>   drivers/gpu/drm/drm_panel_backlight_quirks.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_panel_backlight_quirks.c b/drivers/gpu/drm/drm_panel_backlight_quirks.c
> index f2aefff618dd..c477d98ade2b 100644
> --- a/drivers/gpu/drm/drm_panel_backlight_quirks.c
> +++ b/drivers/gpu/drm/drm_panel_backlight_quirks.c
> @@ -25,6 +25,22 @@ static const struct drm_panel_min_backlight_quirk drm_panel_min_backlight_quirks
>   		.ident.name = "NE135FBM-N41",
>   		.min_brightness = 0,
>   	},
> +	/* 13 inch glossy panel */
> +	{
> +		.dmi_match.field = DMI_BOARD_VENDOR,
> +		.dmi_match.value = "Framework",
> +		.ident.panel_id = drm_edid_encode_panel_id('B', 'O', 'E', 0x095f),
> +		.ident.name = "NE135FBM-N41",
> +		.min_brightness = 0,
> +	},
> +	/* 13 inch 2.8k panel */
> +	{
> +		.dmi_match.field = DMI_BOARD_VENDOR,
> +		.dmi_match.value = "Framework",
> +		.ident.panel_id = drm_edid_encode_panel_id('B', 'O', 'E', 0x0cb4),
> +		.ident.name = "NE135A1M-NY1",
> +		.min_brightness = 0,
> +	},
>   };
>   
>   static bool drm_panel_min_backlight_quirk_matches(const struct drm_panel_min_backlight_quirk *quirk,
> 

Re: [PATCH v5 0/4] drm: Minimum backlight overrides and implementation for amdgpu

[Thomas Weißschuh]

Hi Mario,

(+ a question for Dustin inside)

On 2024-08-21 15:45:46+0000, Mario Limonciello wrote:
> On 8/18/2024 01:56, 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.
> > 
> 
> Apologies for my delayed response, I've been OOO a while.

Welcome back!

> An earlier version of this series I had asked if this could be tested
> specifically with panel_power_savings enabled to ensure we're not causing a
> regression with such a low minimum and panel power savings turned up to the
> maximum.  If such a dependency does exist, I believe panel power savings
> will need to get an extra check introduced to disable it when the brightness
> falls below the previous threshold that was programmed by the firmware.
> 
> Did that ever get checked?

No, I forgot to check that. Sorry about that.
I'll check it for v6.
Dustin, maybe you could also test this on the other panels?

> > Tested on a Framework AMD 13 BIOS 3.05 and Framework AMD 16.
> > 
> 
> As far as I can tell this series doesn't apply any quirks to Framework 16.
> So by testing, do you mean that you tested that it didn't cause regressions
> (which it shouldn't; the panels don't match!).

This is a leftover from an earlier revision, which applied to all
Framework devices. I'll drop it.

> 
> > One solution would be a fixed firmware version, which was announced but
> > has no timeline. >
> > ---
> > Changes in v5:
> > - Forward-declare struct drm_edid
> > - Reorder patches, quirk entries are last
> > - Add patch from Dustin for additional quirk entries
> > - Link to v4: https://lore.kernel.org/r/20240812-amdgpu-min-backlight-quirk-v4-0-56a63ff897b7@weissschuh.net
> > 
> > Changes in v4:
> > - Switch back to v2 implementation
> > - Add MODULE_DESCRIPTION()
> > - Simplify quirk infrastructure to only handle min backlight quirks.
> >    It can be extended if necessary.
> > - Expand documentation.
> > - Link to v3: https://lore.kernel.org/r/20240731-amdgpu-min-backlight-quirk-v3-0-46d40bb21a62@weissschuh.net
> > 
> > Changes in v3:
> > - Switch to cmdline override parameter
> > - Link to v2: https://lore.kernel.org/r/20240623-amdgpu-min-backlight-quirk-v2-0-cecf7f49da9b@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
> > 
> > ---
> > Dustin L. Howett (1):
> >        drm: panel-backlight-quirks: Add Framework 13 glossy and 2.8k panels
> > 
> > Thomas Weißschuh (3):
> >        drm: Add panel backlight quirks
> >        drm/amd/display: Add support for minimum backlight quirk
> >        drm: panel-backlight-quirks: Add Framework 13 matte panel
> > 
> >   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 | 10 +++
> >   drivers/gpu/drm/drm_panel_backlight_quirks.c      | 94 +++++++++++++++++++++++
> >   include/drm/drm_utils.h                           |  4 +
> >   7 files changed, 117 insertions(+)
> > ---
> > base-commit: c3f2d783a459980eafd24c5af94ccd56a615961f
> > change-id: 20240610-amdgpu-min-backlight-quirk-8402fd8e736a
> > 
> > Best regards,
> 

Re: [PATCH v5 1/4] drm: Add panel backlight quirks

[Thomas Weißschuh]

On 2024-08-21 15:51:17+0000, Mario Limonciello wrote:
> On 8/18/2024 01:56, Thomas Weißschuh wrote:
> > Panels using a PWM-controlled backlight source 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 minimum valid backlight value
> > can be maintained as part of the kernel.
> > 
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > Tested-by: Dustin L. Howett <dustin@howett.net>
> 
> One small nit below but otherwise this patch is fine to me.
> 
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> 
> > ---
> >   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 | 70 ++++++++++++++++++++++++++++
> >   include/drm/drm_utils.h                      |  4 ++
> >   5 files changed, 82 insertions(+)
> > 
> > diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
> > index 8435e8621cc0..a26989500129 100644
> > --- a/Documentation/gpu/drm-kms-helpers.rst
> > +++ b/Documentation/gpu/drm-kms-helpers.rst
> > @@ -230,6 +230,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 6b2c6b91f962..9ebb8cdb535e 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -454,6 +454,10 @@ config DRM_HYPERV
> >   config DRM_EXPORT_FOR_TESTS
> >   	bool
> > +# 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 68cc9258ffc4..adf85999aee2 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..6b8bbed77c7f
> > --- /dev/null
> > +++ b/drivers/gpu/drm/drm_panel_backlight_quirks.c
> > @@ -0,0 +1,70 @@
> > +// 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_edid.h>
> > +#include <drm/drm_utils.h>
> > +
> > +struct drm_panel_min_backlight_quirk {
> > +	struct {
> > +		enum dmi_field field;
> > +		const char * const value;
> > +	} dmi_match;
> > +	struct drm_edid_ident ident;
> > +	u8 min_brightness;
> > +};
> > +
> > +static const struct drm_panel_min_backlight_quirk drm_panel_min_backlight_quirks[] = {
> > +};
> > +
> > +static bool drm_panel_min_backlight_quirk_matches(const struct drm_panel_min_backlight_quirk *quirk,
> > +						  const struct drm_edid *edid)
> > +{
> > +	if (!dmi_match(quirk->dmi_match.field, quirk->dmi_match.value))
> > +		return false;
> > +
> > +	if (!drm_edid_match(edid, &quirk->ident))
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> > +/**
> > + * drm_get_panel_min_brightness_quirk - Get minimum supported brightness level for a panel.
> > + * @edid: EDID of the panel to check
> > + *
> > + * This function checks for platform specific (e.g. DMI based) quirks
> > + * providing info on the minimum backlight brightness for systems where this
> > + * cannot be probed correctly from the hard-/firm-ware.
> > + *
> > + * Returns:
> > + * A negative error value or
> > + * an override value in the range [0, 255] representing 0-100% to be scaled to
> > + * the drivers target range.
> > + */
> > +int drm_get_panel_min_brightness_quirk(const struct drm_edid *edid)
> > +{
> > +	const struct drm_panel_min_backlight_quirk *quirk;
> > +	size_t i;
> 
> Nit: this doesn't really seem like it needs to be size_t.  Shouldn't it just
> be an unsigned int?

ARRAY_SIZE() works with sizeof() which returns size_t.
It doesn't really matter, but I slightly prefer to keep the size_t.

> > +
> > +	if (!IS_ENABLED(CONFIG_DMI))
> > +		return -ENODATA;
> > +
> > +	if (!edid)
> > +		return -EINVAL;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(drm_panel_min_backlight_quirks); i++) {
> > +		quirk = &drm_panel_min_backlight_quirks[i];
> > +
> > +		if (drm_panel_min_backlight_quirk_matches(quirk, edid))
> > +			return quirk->min_brightness;
> > +	}
> > +
> > +	return -ENODATA;
> > +}
> > +EXPORT_SYMBOL(drm_get_panel_min_brightness_quirk);
> > +
> > +MODULE_DESCRIPTION("Quirks for panel backlight overrides");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/drm/drm_utils.h b/include/drm/drm_utils.h
> > index 70775748d243..15fa9b6865f4 100644
> > --- a/include/drm/drm_utils.h
> > +++ b/include/drm/drm_utils.h
> > @@ -12,8 +12,12 @@
> >   #include <linux/types.h>
> > +struct drm_edid;
> > +
> >   int drm_get_panel_orientation_quirk(int width, int height);
> > +int drm_get_panel_min_brightness_quirk(const struct drm_edid *edid);
> > +
> >   signed long drm_timeout_abs_to_jiffies(int64_t timeout_nsec);
> >   #endif
> > 
> 

Re: [PATCH v5 2/4] drm/amd/display: Add support for minimum backlight quirk

[Thomas Weißschuh]

On 2024-08-21 15:54:14+0000, Mario Limonciello wrote:
> On 8/18/2024 01:56, Thomas Weißschuh wrote:
> > Not all platforms provide correct PWM backlight capabilities through ATIF.
> 
> I don't think correct is an accurate term here.  How about 'optimial'?

Looks typoed to me :-)

What about this?

Not all platforms provide the full range of PWM backlight capabilities
supported by the hardware through ATIF.


> > Use the generic drm panel minimum backlight quirk infrastructure to
> > override the capabilities where necessary.
> > 
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > Tested-by: Dustin L. Howett <dustin@howett.net>
> 
> The code looks fine to me but please wait for an ack from someone on the AMD
> display team.
> 
> Also, I would like to see comments about the testing with panel power
> savings enabled to avoid a conflict.

Ack.

> 
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/Kconfig                |  1 +
> >   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 ++++++++++
> >   2 files changed, 11 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig b/drivers/gpu/drm/amd/amdgpu/Kconfig
> > index 0051fb1b437f..655c10aef2e3 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/Kconfig
> > +++ b/drivers/gpu/drm/amd/amdgpu/Kconfig
> > @@ -23,6 +23,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 983a977632ff..056960ea335c 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>
> > @@ -3333,6 +3334,8 @@ static void update_connector_ext_caps(struct amdgpu_dm_connector *aconnector)
> >   	struct drm_connector *conn_base;
> >   	struct amdgpu_device *adev;
> >   	struct drm_luminance_range_info *luminance_range;
> > +	const struct drm_edid *drm_edid;
> > +	int min_input_signal_override;
> >   	if (aconnector->bl_idx == -1 ||
> >   	    aconnector->dc_link->connector_signal != SIGNAL_TYPE_EDP)
> > @@ -3367,6 +3370,13 @@ static void update_connector_ext_caps(struct amdgpu_dm_connector *aconnector)
> >   		caps->aux_min_input_signal = 0;
> >   		caps->aux_max_input_signal = 512;
> >   	}
> > +
> > +	drm_edid = drm_edid_alloc(aconnector->edid,
> > +				  EDID_LENGTH * (aconnector->edid->extensions + 1));
> > +	min_input_signal_override = drm_get_panel_min_brightness_quirk(drm_edid);
> > +	drm_edid_free(drm_edid);
> > +	if (min_input_signal_override >= 0)
> > +		caps->min_input_signal = min_input_signal_override;
> >   }
> >   void amdgpu_dm_update_connector_after_detect(
> > 
> 

Re: [PATCH v5 1/4] drm: Add panel backlight quirks

[Mario Limonciello]

On 8/22/2024 01:12, Thomas Weißschuh wrote:
> On 2024-08-21 15:51:17+0000, Mario Limonciello wrote:
>> On 8/18/2024 01:56, Thomas Weißschuh wrote:
>>> Panels using a PWM-controlled backlight source 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 minimum valid backlight value
>>> can be maintained as part of the kernel.
>>>
>>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>>> Tested-by: Dustin L. Howett <dustin@howett.net>
>>
>> One small nit below but otherwise this patch is fine to me.
>>
>> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
>>
>>> ---
>>>    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 | 70 ++++++++++++++++++++++++++++
>>>    include/drm/drm_utils.h                      |  4 ++
>>>    5 files changed, 82 insertions(+)
>>>
>>> diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
>>> index 8435e8621cc0..a26989500129 100644
>>> --- a/Documentation/gpu/drm-kms-helpers.rst
>>> +++ b/Documentation/gpu/drm-kms-helpers.rst
>>> @@ -230,6 +230,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 6b2c6b91f962..9ebb8cdb535e 100644
>>> --- a/drivers/gpu/drm/Kconfig
>>> +++ b/drivers/gpu/drm/Kconfig
>>> @@ -454,6 +454,10 @@ config DRM_HYPERV
>>>    config DRM_EXPORT_FOR_TESTS
>>>    	bool
>>> +# 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 68cc9258ffc4..adf85999aee2 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..6b8bbed77c7f
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/drm_panel_backlight_quirks.c
>>> @@ -0,0 +1,70 @@
>>> +// 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_edid.h>
>>> +#include <drm/drm_utils.h>
>>> +
>>> +struct drm_panel_min_backlight_quirk {
>>> +	struct {
>>> +		enum dmi_field field;
>>> +		const char * const value;
>>> +	} dmi_match;
>>> +	struct drm_edid_ident ident;
>>> +	u8 min_brightness;
>>> +};
>>> +
>>> +static const struct drm_panel_min_backlight_quirk drm_panel_min_backlight_quirks[] = {
>>> +};
>>> +
>>> +static bool drm_panel_min_backlight_quirk_matches(const struct drm_panel_min_backlight_quirk *quirk,
>>> +						  const struct drm_edid *edid)
>>> +{
>>> +	if (!dmi_match(quirk->dmi_match.field, quirk->dmi_match.value))
>>> +		return false;
>>> +
>>> +	if (!drm_edid_match(edid, &quirk->ident))
>>> +		return false;
>>> +
>>> +	return true;
>>> +}
>>> +
>>> +/**
>>> + * drm_get_panel_min_brightness_quirk - Get minimum supported brightness level for a panel.
>>> + * @edid: EDID of the panel to check
>>> + *
>>> + * This function checks for platform specific (e.g. DMI based) quirks
>>> + * providing info on the minimum backlight brightness for systems where this
>>> + * cannot be probed correctly from the hard-/firm-ware.
>>> + *
>>> + * Returns:
>>> + * A negative error value or
>>> + * an override value in the range [0, 255] representing 0-100% to be scaled to
>>> + * the drivers target range.
>>> + */
>>> +int drm_get_panel_min_brightness_quirk(const struct drm_edid *edid)
>>> +{
>>> +	const struct drm_panel_min_backlight_quirk *quirk;
>>> +	size_t i;
>>
>> Nit: this doesn't really seem like it needs to be size_t.  Shouldn't it just
>> be an unsigned int?
> 
> ARRAY_SIZE() works with sizeof() which returns size_t.
> It doesn't really matter, but I slightly prefer to keep the size_t.

Ah thanks, I didn't realize that.  Feel free to leave as is then.

> 
>>> +
>>> +	if (!IS_ENABLED(CONFIG_DMI))
>>> +		return -ENODATA;
>>> +
>>> +	if (!edid)
>>> +		return -EINVAL;
>>> +
>>> +	for (i = 0; i < ARRAY_SIZE(drm_panel_min_backlight_quirks); i++) {
>>> +		quirk = &drm_panel_min_backlight_quirks[i];
>>> +
>>> +		if (drm_panel_min_backlight_quirk_matches(quirk, edid))
>>> +			return quirk->min_brightness;
>>> +	}
>>> +
>>> +	return -ENODATA;
>>> +}
>>> +EXPORT_SYMBOL(drm_get_panel_min_brightness_quirk);
>>> +
>>> +MODULE_DESCRIPTION("Quirks for panel backlight overrides");
>>> +MODULE_LICENSE("GPL");
>>> diff --git a/include/drm/drm_utils.h b/include/drm/drm_utils.h
>>> index 70775748d243..15fa9b6865f4 100644
>>> --- a/include/drm/drm_utils.h
>>> +++ b/include/drm/drm_utils.h
>>> @@ -12,8 +12,12 @@
>>>    #include <linux/types.h>
>>> +struct drm_edid;
>>> +
>>>    int drm_get_panel_orientation_quirk(int width, int height);
>>> +int drm_get_panel_min_brightness_quirk(const struct drm_edid *edid);
>>> +
>>>    signed long drm_timeout_abs_to_jiffies(int64_t timeout_nsec);
>>>    #endif
>>>
>>

Re: [PATCH v5 2/4] drm/amd/display: Add support for minimum backlight quirk

[Mario Limonciello]

On 8/22/2024 01:14, Thomas Weißschuh wrote:
> On 2024-08-21 15:54:14+0000, Mario Limonciello wrote:
>> On 8/18/2024 01:56, Thomas Weißschuh wrote:
>>> Not all platforms provide correct PWM backlight capabilities through ATIF.
>>
>> I don't think correct is an accurate term here.  How about 'optimial'?
> 
> Looks typoed to me :-)

heh.

> 
> What about this?
> 
> Not all platforms provide the full range of PWM backlight capabilities
> supported by the hardware through ATIF.

Sounds great, thanks!
> 
> 
>>> Use the generic drm panel minimum backlight quirk infrastructure to
>>> override the capabilities where necessary.
>>>
>>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>>> Tested-by: Dustin L. Howett <dustin@howett.net>
>>
>> The code looks fine to me but please wait for an ack from someone on the AMD
>> display team.
>>
>> Also, I would like to see comments about the testing with panel power
>> savings enabled to avoid a conflict.
> 
> Ack.
> 
>>
>> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/Kconfig                |  1 +
>>>    drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 ++++++++++
>>>    2 files changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig b/drivers/gpu/drm/amd/amdgpu/Kconfig
>>> index 0051fb1b437f..655c10aef2e3 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/Kconfig
>>> +++ b/drivers/gpu/drm/amd/amdgpu/Kconfig
>>> @@ -23,6 +23,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 983a977632ff..056960ea335c 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>
>>> @@ -3333,6 +3334,8 @@ static void update_connector_ext_caps(struct amdgpu_dm_connector *aconnector)
>>>    	struct drm_connector *conn_base;
>>>    	struct amdgpu_device *adev;
>>>    	struct drm_luminance_range_info *luminance_range;
>>> +	const struct drm_edid *drm_edid;
>>> +	int min_input_signal_override;
>>>    	if (aconnector->bl_idx == -1 ||
>>>    	    aconnector->dc_link->connector_signal != SIGNAL_TYPE_EDP)
>>> @@ -3367,6 +3370,13 @@ static void update_connector_ext_caps(struct amdgpu_dm_connector *aconnector)
>>>    		caps->aux_min_input_signal = 0;
>>>    		caps->aux_max_input_signal = 512;
>>>    	}
>>> +
>>> +	drm_edid = drm_edid_alloc(aconnector->edid,
>>> +				  EDID_LENGTH * (aconnector->edid->extensions + 1));
>>> +	min_input_signal_override = drm_get_panel_min_brightness_quirk(drm_edid);
>>> +	drm_edid_free(drm_edid);
>>> +	if (min_input_signal_override >= 0)
>>> +		caps->min_input_signal = min_input_signal_override;
>>>    }
>>>    void amdgpu_dm_update_connector_after_detect(
>>>
>>