public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH] drm/amd/display: Avoid using zero AC/DC brightness levels
@ 2026-05-26 21:00 Edson Juliano Drosdeck
  2026-05-28  2:44 ` Claude review: " Claude Code Review Bot
  2026-05-28  2:44 ` Claude Code Review Bot
  0 siblings, 2 replies; 3+ messages in thread
From: Edson Juliano Drosdeck @ 2026-05-26 21:00 UTC (permalink / raw)
  To: harry.wentland
  Cc: sunpeng.li, siqueira, alexander.deucher, christian.koenig,
	airlied, simona, mario.limonciello, alex.hung, Wayne.Lin,
	timur.kristof, superm1, aurabindo.pillai, ivan.lipski,
	chen-yu.chen, amd-gfx, dri-devel, linux-kernel, edson.drosdeck

Some systems report zero AC/DC brightness levels during
backlight initialization, causing the panel brightness to
start at the minimum level on boot.

Only use the firmware brightness levels when both AC and DC
values are non-zero

Signed-off-by: Edson Juliano Drosdeck <edson.drosdeck@gmail.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

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 5fc5d5608506..f947ce2a8625 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5380,7 +5380,9 @@ amdgpu_dm_register_backlight_device(struct amdgpu_dm_connector *aconnector)
 	}
 
 	caps = &dm->backlight_caps[aconnector->bl_idx];
-	if (get_brightness_range(caps, &min, &max)) {
+	if (get_brightness_range(caps, &min, &max) &&
+	    caps->ac_level > 0 &&
+	    caps->dc_level > 0) {
 		if (power_supply_is_system_supplied() > 0)
 			props.brightness = DIV_ROUND_CLOSEST((max - min) * caps->ac_level, 100);
 		else
-- 
2.47.3


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Claude review: drm/amd/display: Avoid using zero AC/DC brightness levels
  2026-05-26 21:00 [PATCH] drm/amd/display: Avoid using zero AC/DC brightness levels Edson Juliano Drosdeck
@ 2026-05-28  2:44 ` Claude Code Review Bot
  2026-05-28  2:44 ` Claude Code Review Bot
  1 sibling, 0 replies; 3+ messages in thread
From: Claude Code Review Bot @ 2026-05-28  2:44 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: drm/amd/display: Avoid using zero AC/DC brightness levels
Author: Edson Juliano Drosdeck <edson.drosdeck@gmail.com>
Patches: 1
Reviewed: 2026-05-28T12:44:12.591232

---

This is a single-patch fix for a real-world issue where some systems report zero `ac_level`/`dc_level` values from ACPI firmware during backlight initialization. When these are zero, `DIV_ROUND_CLOSEST((max - min) * 0, 100)` yields zero brightness, which would set the panel to minimum brightness on boot — a bad user experience.

The fix is straightforward and correct: add guards to reject zero values and fall through to the `MAX_BACKLIGHT_LEVEL` fallback. The code change is minimal and low-risk.

**Minor concerns exist** around the commit message and a potential edge case with `max_brightness` not being set when the fallback fires, but overall this is a reasonable fix.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Claude review: drm/amd/display: Avoid using zero AC/DC brightness levels
  2026-05-26 21:00 [PATCH] drm/amd/display: Avoid using zero AC/DC brightness levels Edson Juliano Drosdeck
  2026-05-28  2:44 ` Claude review: " Claude Code Review Bot
@ 2026-05-28  2:44 ` Claude Code Review Bot
  1 sibling, 0 replies; 3+ messages in thread
From: Claude Code Review Bot @ 2026-05-28  2:44 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Correctness: Generally good, with one behavioral concern.**

The added checks at `amdgpu_dm.c:5509-5511`:
```c
if (get_brightness_range(caps, &min, &max) &&
    caps->ac_level > 0 &&
    caps->dc_level > 0) {
```

This requires **both** `ac_level` and `dc_level` to be non-zero for the firmware values to be used. This is a conservative choice.

**Concern — should this be `||` instead of `&&`?** Consider a system where `ac_level` is valid (say 80) but `dc_level` is zero, or vice versa. With this patch, both firmware values are discarded even though the relevant one (based on `power_supply_is_system_supplied()`) might be perfectly valid. An alternative approach would be to only check the value actually being used:

```c
if (get_brightness_range(caps, &min, &max)) {
    if (power_supply_is_system_supplied() > 0 && caps->ac_level > 0)
        props.brightness = DIV_ROUND_CLOSEST(...ac_level...);
    else if (caps->dc_level > 0)
        props.brightness = DIV_ROUND_CLOSEST(...dc_level...);
    else
        /* fallback */
```

However, the current approach is simpler and more conservative — if the firmware is giving bogus zeros for one field, the other field may be suspect too. This is a reasonable engineering judgment.

**Fallback path sets `max_brightness` to `MAX_BACKLIGHT_LEVEL`:** When the new guard rejects zero levels, the fallback at line 5521 sets both `props.brightness` and `props.max_brightness` to `MAX_BACKLIGHT_LEVEL`. Previously this only happened when `get_brightness_range()` failed (i.e., `caps` was NULL). Now it also fires when caps exist but have zero AC/DC levels. In this case the firmware's min/max range is valid but unused — the backlight gets registered with `MAX_BACKLIGHT_LEVEL` as its range instead of the firmware-reported range. This seems acceptable as a safe default but is a behavioral change worth noting: user-space will see a different `max_brightness` on these affected systems than it would if only the initial brightness were corrected.

**`ac_level` and `dc_level` are `u8` (unsigned):** The `> 0` checks are correct; there's no sign issue. The values come from ACPI firmware (`amdgpu_acpi.c:400-401`) and represent percentages (0-100).

**Commit message nits:**
- Missing period at the end of "Only use the firmware brightness levels when both AC and DC values are non-zero" — minor style issue.
- The commit message could benefit from a `Fixes:` tag referencing the commit that introduced this brightness initialization logic, to help stable kernel backporting.
- No `Cc: stable@vger.kernel.org` tag — if this is intended as a bug fix for existing systems, it may be worth adding one.

**Overall:** The patch is simple, correct for the stated problem, and low-risk. Recommend adding a `Fixes:` tag and considering whether the fallback should preserve the firmware's `max_brightness` range even when it discards the initial brightness value.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-05-28  2:44 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-26 21:00 [PATCH] drm/amd/display: Avoid using zero AC/DC brightness levels Edson Juliano Drosdeck
2026-05-28  2:44 ` Claude review: " Claude Code Review Bot
2026-05-28  2:44 ` Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox