Re: [PATCH v2 3/4] drm/amd/display: pass userspace brightness to power module

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [Medium] The brightness quirk mask is not applied to `backlight_millinits` for AMD AUX panels, causing firmware to write unmasked values during autonomous operations

Pre-existing issues:
- [High] Unprotected traversal of drm_mode_config.connector_list
- [High] Unprotected access to `dm->dc->current_state` leading to potential Use-After-Free
- [High] Potential division by zero in brightness conversion functions when firmware capabilities are anomalous
- [High] Incomplete bounds validation during power module creation allows multi-display setups to trigger a division by zero
--

commit 8608a59a12eb05fd14ffc0e7cb1af582e44dade9
Author: Andrei Rusu de Castro <arc@empyreal.works>
Date:   Wed Sep 2 22:31:59 2026 +0000

drm/amd/display: pass userspace brightness to power module

The patch adjusts how brightness is passed to the power module in amdgpu_dm,
keeping it as userspace percentage (millipercent) instead of converting to
PWM domain first. This fixes domain mismatch issues and handles panel-specific
brightness quirks for AMD AUX writes.

Closes: https://sashiko.dev/#/patchset/20260902-brightness-cover-abf809f2@empyreal.works?part=3

> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c
[ ... ]
> @@ -218,6 +218,39 @@ u32 convert_brightness_to_user(const struct amdgpu_dm_backlight_caps *caps,
>  }
>  EXPORT_IF_KUNIT(convert_brightness_to_user);
>  
> +static u32 convert_brightness_to_millipercent(const struct amdgpu_dm_backlight_caps *caps,
> +					      u32 brightness)
> +{
> +	unsigned int min, max;
> +
> +	if (!get_brightness_range(caps, &min, &max) || max <= min)
> +		return 0;
> +
> +	if (brightness >= max)
> +		return 100 * 1000;
> +
> +	return DIV_ROUND_CLOSEST_ULL((u64)brightness * 100 * 1000, max);
> +}
> +
> +STATIC_IFN_KUNIT
> +u32 convert_brightness_for_power_module(const struct amdgpu_dm_backlight_caps *caps,
> +					u32 user_brightness)
> +{
> +	u32 brightness;
> +
> +	if (!caps)
> +		return user_brightness;
> +
> +	if (!caps->aux_support)
> +		return convert_brightness_to_millipercent(caps, user_brightness);
> +
> +	brightness = convert_brightness_from_user(caps, user_brightness);

[Severity: High]
This is a pre-existing issue, but while convert_brightness_to_millipercent()
properly guards against max <= min to avoid division by zero, the
convert_brightness_from_user() function called here lacks these same guards.
Does convert_brightness_from_user() unconditionally divide by max, risking a
kernel panic if anomalous firmware sets max to zero? Similarly, does
convert_brightness_to_user() risk dividing by zero when evaluating max - min?

> +
> +	return brightness;
> +}
[ ... ]
> @@ -262,7 +295,6 @@ void amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
>  	bool rc = false, reallow_idle = false;
>  	struct drm_connector *connector;
>  	struct dc_stream_state *stream;
> -	unsigned int min, max;
>  
>  	list_for_each_entry(connector, &dm->ddev->mode_config.connector_list, head) {

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to traverse the DRM
connector_list using the raw list_for_each_entry macro without holding
mode_config.mutex or using the drm_connector_list_iter API? Can a concurrent
hotplug event removing a connector leave this loop traversing a freed pointer,
resulting in a use-after-free?

[ ... ]
> @@ -285,12 +317,9 @@ void amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
>  	/* update scratch register */
>  	if (bl_idx == 0)
>  		amdgpu_atombios_scratch_regs_set_backlight_level(dm->adev, dm->brightness[bl_idx]);
> -	brightness = convert_brightness_from_user(caps, dm->brightness[bl_idx]);
>  	link = (struct dc_link *)dm->backlight_link[bl_idx];
> -
> -	/* Apply brightness quirk */
> -	if (caps->brightness_mask)
> -		brightness |= caps->brightness_mask;
> +	brightness = convert_brightness_for_power_module(caps,
> +							 dm->brightness[bl_idx]);

[Severity: High]
This is a pre-existing issue, but further down in this function,
dm_find_stream_with_link() is called before acquiring dc_lock. Inside
dm_find_stream_with_link(), the dm->dc->current_state pointer is dereferenced
and iterated over. Because this sysfs callback runs concurrently with atomic
commits (which free the old state), is there a risk of a use-after-free crash
by reading a stale current_state pointer before the lock is held?

[ ... ]
> --- a/drivers/gpu/drm/amd/display/modules/power/power.c
> +++ b/drivers/gpu/drm/amd/display/modules/power/power.c
> @@ -157,6 +157,8 @@ struct mod_power *mod_power_create(struct dc *dc,
>  				init_params[inst].use_custom_backlight_caps;
>  		core_power->bl_prop[inst].custom_backlight_caps_config_no =
>  				init_params[inst].custom_backlight_caps_config_no;
> +		core_power->bl_prop[inst].brightness_mask =
> +				init_params[inst].brightness_mask;
>  
>  		// Do not allow less than 101 backlight levels
>  		if (init_params[inst].num_backlight_levels < 101)

[Severity: High]
This isn't a bug introduced by this patch, but further down in
mod_power_create(), a loop validates the brightness ranges for eDP instances:

for (inst = 0; inst < edp_num; inst++) {
    if (core_power->bl_prop[inst].nits_range != 0 && ...) {
        is_brightness_range_valid = true;
        break;
    }
}

Does this premature break cause the loop to exit without verifying remaining
instances? If panel 1 has a zero nits_range while panel 0 is valid, could
this lead to a division by zero later in backlight_millinit_to_pwm_legacy()
when it evaluates panel 1?

[ ... ]
> --- a/drivers/gpu/drm/amd/display/modules/power/power_abm.c
> +++ b/drivers/gpu/drm/amd/display/modules/power/power_abm.c
[ ... ]
> @@ -1020,6 +1050,11 @@ void fill_backlight_level_params(struct core_power *core_power,
>  
>  	if (backlight_control_type == BACKLIGHT_CONTROL_AMD_AUX && !is_hdr)
>  		backlight_level_params->control_type = BACKLIGHT_CONTROL_PWM;
> +
> +	if (backlight_level_params->control_type == BACKLIGHT_CONTROL_PWM)
> +		backlight_level_params->backlight_pwm_u16_16 =
> +			backlight_apply_source_mask(core_power, backlight_pwm,
> +						    panel_inst);
>  }

[Severity: Medium]
When backlight_level_params->control_type remains BACKLIGHT_CONTROL_AMD_AUX
(for instance, during HDR playback), the brightness_mask is not applied to
backlight_level_params->backlight_millinits here. Because these parameters
are subsequently passed to the DMCUB firmware for autonomous operations like
PSR/Replay exits, will this allow the firmware to write unmasked brightness
values directly to the panel, bypassing the quirk? Should
backlight_millinits also be masked here when the control type is AMD AUX?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788388049.git.arc@empyreal.works?part=3