[PATCH v3 0/2] drm: minimum backlight overrides and implementation for amdgpu

[PATCH v3 0/2] 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.

Add a generic override helper for the user to override the settings
provided by the firmware through the kernel cmdline.
Also add amdgpu as a user of that helper.

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

This helper does conflict with the mode override via the cmdline.
Only one can be specified.
IMO the mode override can be extended to also handle "min-brightness"
when that becomes necessary.

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

---
Thomas Weißschuh (2):
      drm/connector: add drm_connector_get_cmdline_min_brightness_override()
      drm/amd/display: implement minimum brightness override

 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  6 ++++
 drivers/gpu/drm/drm_connector.c                   | 34 +++++++++++++++++++++++
 include/drm/drm_connector.h                       |  2 ++
 3 files changed, 42 insertions(+)
---
base-commit: 36821612eb3091a21f7f4a907b497064725080c3
change-id: 20240610-amdgpu-min-backlight-quirk-8402fd8e736a

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

[PATCH v3 1/2] drm/connector: add drm_connector_get_cmdline_min_brightness_override()

[Thomas Weißschuh]

Add a helper which can read a overridden minimum backlight value from the
kernel cmdline.
This is useful if the minimum backlight as reported by machines VBT
does not match the user expectations.

As an example, the minimum backlight brightness on the Framework 13
matte panel is too high.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/gpu/drm/drm_connector.c | 34 ++++++++++++++++++++++++++++++++++
 include/drm/drm_connector.h     |  2 ++
 2 files changed, 36 insertions(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index ab6ab7ff7ea8..d0b3d5d6c7c2 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -34,6 +34,7 @@
 #include <drm/drm_utils.h>
 
 #include <linux/property.h>
+#include <linux/string.h>
 #include <linux/uaccess.h>
 
 #include <video/cmdline.h>
@@ -3399,3 +3400,36 @@ struct drm_tile_group *drm_mode_create_tile_group(struct drm_device *dev,
 	return tg;
 }
 EXPORT_SYMBOL(drm_mode_create_tile_group);
+
+/**
+ * drm_connector_get_cmdline_min_brightness_override - read an overridden
+ * minimum brightness value from the cmdline
+ * @connector: connector to query
+ *
+ * Read an minimum brightness override from the kernel cmdline if present.
+ * The parameter takes the form "video=CONNECTOR_NAME:min-brightness=VALUE".
+ *
+ * RETURNS:
+ * negative error or override value in the range [0, 255]
+ */
+int drm_connector_get_cmdline_min_brightness_override(struct drm_connector *connector)
+{
+	const char *option, *value_str;
+	int err;
+	u8 val;
+
+	option = video_get_options(connector->name);
+	if (!option)
+		return -ENOENT;
+
+	value_str = option + str_has_prefix(option, "min-brightness=");
+	if (value_str == option)
+		return -EINVAL;
+
+	err = kstrtou8(value_str, 10, &val);
+	if (err)
+		return err;
+
+	return val;
+}
+EXPORT_SYMBOL_GPL(drm_connector_get_cmdline_min_brightness_override);
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index c754651044d4..64d86604cc6e 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -2357,6 +2357,8 @@ bool drm_connector_has_possible_encoder(struct drm_connector *connector,
 					struct drm_encoder *encoder);
 const char *drm_get_colorspace_name(enum drm_colorspace colorspace);
 
+int drm_connector_get_cmdline_min_brightness_override(struct drm_connector *connector);
+
 /**
  * drm_for_each_connector_iter - connector_list iterator macro
  * @connector: &struct drm_connector pointer used as cursor

-- 
2.46.0

[PATCH v3 2/2] drm/amd/display: implement minimum brightness override

[Thomas Weißschuh]

This is useful if the minimum backlight as reported by machines VBT
does not match the user expectations.

As an example, the minimum backlight brightness on the Framework 13
matte panel is too high.

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

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 7e7929f24ae4..ae15465cacfc 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3330,6 +3330,7 @@ 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;
+	int min_input_signal_override;
 
 	if (aconnector->bl_idx == -1 ||
 	    aconnector->dc_link->connector_signal != SIGNAL_TYPE_EDP)
@@ -3364,6 +3365,11 @@ static void update_connector_ext_caps(struct amdgpu_dm_connector *aconnector)
 		caps->aux_min_input_signal = 0;
 		caps->aux_max_input_signal = 512;
 	}
+
+	min_input_signal_override = drm_connector_get_cmdline_min_brightness_override(conn_base);
+	if (min_input_signal_override >= 0)
+		caps->min_input_signal = min_input_signal_override;
+
 }
 
 void amdgpu_dm_update_connector_after_detect(

-- 
2.46.0

Re: [PATCH v3 0/2] drm: minimum backlight overrides and implementation for amdgpu

[Jani Nikula]

On Wed, 31 Jul 2024, Thomas Weißschuh <linux@weissschuh.net> 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 override helper for the user to override the settings
> provided by the firmware through the kernel cmdline.
> Also add amdgpu as a user of that helper.
>
> One solution would be a fixed firmware version, which was announced but
> has no timeline.

The flip side is that if we add this now, it pretty much has a timeline:
We'll have to carry and support it forever.

It's not a great prospect for something so specific. Not to mention that
the limits are generally there for electrical minimums that should not
be overridden. And before you know it, we'll have bug reports about
flickering screens...

BR,
Jani.


>
> This helper does conflict with the mode override via the cmdline.
> Only one can be specified.
> IMO the mode override can be extended to also handle "min-brightness"
> when that becomes necessary.
>
> ---
> 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
>
> ---
> Thomas Weißschuh (2):
>       drm/connector: add drm_connector_get_cmdline_min_brightness_override()
>       drm/amd/display: implement minimum brightness override
>
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  6 ++++
>  drivers/gpu/drm/drm_connector.c                   | 34 +++++++++++++++++++++++
>  include/drm/drm_connector.h                       |  2 ++
>  3 files changed, 42 insertions(+)
> ---
> base-commit: 36821612eb3091a21f7f4a907b497064725080c3
> change-id: 20240610-amdgpu-min-backlight-quirk-8402fd8e736a
>
> Best regards,

-- 
Jani Nikula, Intel

Re: [PATCH v3 0/2] drm: minimum backlight overrides and implementation for amdgpu

[Daniel Vetter]

On Wed, Jul 31, 2024 at 08:40:12PM +0300, Jani Nikula wrote:
> On Wed, 31 Jul 2024, Thomas Weißschuh <linux@weissschuh.net> 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 override helper for the user to override the settings
> > provided by the firmware through the kernel cmdline.
> > Also add amdgpu as a user of that helper.
> >
> > One solution would be a fixed firmware version, which was announced but
> > has no timeline.
> 
> The flip side is that if we add this now, it pretty much has a timeline:
> We'll have to carry and support it forever.
> 
> It's not a great prospect for something so specific. Not to mention that
> the limits are generally there for electrical minimums that should not
> be overridden. And before you know it, we'll have bug reports about
> flickering screens...

Yeah I think for this specific case where a fixed firmware is already
kinda promised, a quirk is the right fix. Otherwise we open up a can of
worms here ... so personally I like v2 a lot more.
-Sima

> 
> BR,
> Jani.
> 
> 
> >
> > This helper does conflict with the mode override via the cmdline.
> > Only one can be specified.
> > IMO the mode override can be extended to also handle "min-brightness"
> > when that becomes necessary.
> >
> > ---
> > 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
> >
> > ---
> > Thomas Weißschuh (2):
> >       drm/connector: add drm_connector_get_cmdline_min_brightness_override()
> >       drm/amd/display: implement minimum brightness override
> >
> >  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  6 ++++
> >  drivers/gpu/drm/drm_connector.c                   | 34 +++++++++++++++++++++++
> >  include/drm/drm_connector.h                       |  2 ++
> >  3 files changed, 42 insertions(+)
> > ---
> > base-commit: 36821612eb3091a21f7f4a907b497064725080c3
> > change-id: 20240610-amdgpu-min-backlight-quirk-8402fd8e736a
> >
> > Best regards,
> 
> -- 
> Jani Nikula, Intel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v3 0/2] drm: minimum backlight overrides and implementation for amdgpu

[Hans de Goede]

Hi,

On 7/31/24 10:55 PM, Daniel Vetter wrote:
> On Wed, Jul 31, 2024 at 08:40:12PM +0300, Jani Nikula wrote:
>> On Wed, 31 Jul 2024, Thomas Weißschuh <linux@weissschuh.net> 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 override helper for the user to override the settings
>>> provided by the firmware through the kernel cmdline.
>>> Also add amdgpu as a user of that helper.
>>>
>>> One solution would be a fixed firmware version, which was announced but
>>> has no timeline.
>>
>> The flip side is that if we add this now, it pretty much has a timeline:
>> We'll have to carry and support it forever.
>>
>> It's not a great prospect for something so specific. Not to mention that
>> the limits are generally there for electrical minimums that should not
>> be overridden. And before you know it, we'll have bug reports about
>> flickering screens...
> 
> Yeah I think for this specific case where a fixed firmware is already
> kinda promised, a quirk is the right fix. Otherwise we open up a can of
> worms here ... so personally I like v2 a lot more.
> -Sima

Ok, with both Jani and Sima preferring the quirk approach from v2,
I withdraw my objection against v2. So lets go with that version.

Thomas, sorry about this.

I see that other then a remark from Jeff Johnson about a missing
MODULE_DESCRIPTION() v2 does not have any reviews yet though.

So we will need to review that version now. Might be best for
visibility of the patches in people's review queue to repost
v2 as v4 with the MODULE_DESCRIPTION() fixed ?

Regards,

Hans



>>> This helper does conflict with the mode override via the cmdline.
>>> Only one can be specified.
>>> IMO the mode override can be extended to also handle "min-brightness"
>>> when that becomes necessary.
>>>
>>> ---
>>> 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
>>>
>>> ---
>>> Thomas Weißschuh (2):
>>>       drm/connector: add drm_connector_get_cmdline_min_brightness_override()
>>>       drm/amd/display: implement minimum brightness override
>>>
>>>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  6 ++++
>>>  drivers/gpu/drm/drm_connector.c                   | 34 +++++++++++++++++++++++
>>>  include/drm/drm_connector.h                       |  2 ++
>>>  3 files changed, 42 insertions(+)
>>> ---
>>> base-commit: 36821612eb3091a21f7f4a907b497064725080c3
>>> change-id: 20240610-amdgpu-min-backlight-quirk-8402fd8e736a
>>>
>>> Best regards,
>>
>> -- 
>> Jani Nikula, Intel
> 

Re: [PATCH v3 0/2] drm: minimum backlight overrides and implementation for amdgpu

[Thomas Weißschuh]

Hi,

On 2024-08-01 10:52:55+0000, Hans de Goede wrote:
> On 7/31/24 10:55 PM, Daniel Vetter wrote:
> > On Wed, Jul 31, 2024 at 08:40:12PM +0300, Jani Nikula wrote:
> >> On Wed, 31 Jul 2024, Thomas Weißschuh <linux@weissschuh.net> 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 override helper for the user to override the settings
> >>> provided by the firmware through the kernel cmdline.
> >>> Also add amdgpu as a user of that helper.
> >>>
> >>> One solution would be a fixed firmware version, which was announced but
> >>> has no timeline.
> >>
> >> The flip side is that if we add this now, it pretty much has a timeline:
> >> We'll have to carry and support it forever.
> >>
> >> It's not a great prospect for something so specific. Not to mention that
> >> the limits are generally there for electrical minimums that should not
> >> be overridden. And before you know it, we'll have bug reports about
> >> flickering screens...
> > 
> > Yeah I think for this specific case where a fixed firmware is already
> > kinda promised, a quirk is the right fix. Otherwise we open up a can of
> > worms here ... so personally I like v2 a lot more.
> > -Sima
> 
> Ok, with both Jani and Sima preferring the quirk approach from v2,
> I withdraw my objection against v2. So lets go with that version.

Ack.

I want to note though, that there are enough other commandline
options that can mess things up.
An invalid video=MODELINE, custom ACPI tables, etc, so the fallout from the
new commandline variable doesn't seem too bad to me.

> Thomas, sorry about this.

No worries!

> I see that other then a remark from Jeff Johnson about a missing
> MODULE_DESCRIPTION() v2 does not have any reviews yet though.
> 
> So we will need to review that version now. Might be best for
> visibility of the patches in people's review queue to repost
> v2 as v4 with the MODULE_DESCRIPTION() fixed ?

I can do that.

> 
> >>> This helper does conflict with the mode override via the cmdline.
> >>> Only one can be specified.
> >>> IMO the mode override can be extended to also handle "min-brightness"
> >>> when that becomes necessary.
> >>>
> >>> ---
> >>> 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
> >>>
> >>> ---
> >>> Thomas Weißschuh (2):
> >>>       drm/connector: add drm_connector_get_cmdline_min_brightness_override()
> >>>       drm/amd/display: implement minimum brightness override
> >>>
> >>>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  6 ++++
> >>>  drivers/gpu/drm/drm_connector.c                   | 34 +++++++++++++++++++++++
> >>>  include/drm/drm_connector.h                       |  2 ++
> >>>  3 files changed, 42 insertions(+)
> >>> ---
> >>> base-commit: 36821612eb3091a21f7f4a907b497064725080c3
> >>> change-id: 20240610-amdgpu-min-backlight-quirk-8402fd8e736a
> >>>
> >>> Best regards,
> >>
> >> -- 
> >> Jani Nikula, Intel
> > 
>