Re: [PATCH v6 08/10] drm/amd/display: Drop brightness caching in amdgpu_dm

[sashiko-bot@kernel.org]

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

New issues:
- [High] Removal of brightness caching and restoration logic causes the display to remain at incorrect brightness levels after modesets, DPMS ON, and GPU resets, and permanently drops backlight changes made while the display is off.

Pre-existing issues:
- [High] Unsafe iteration of the global DRM `connector_list` without holding `mode_config.connector_list_lock` or using the `drm_connector_list_iter` API.
- [High] Unprotected access to `dm->dc->current_state` and its streams can lead to use-after-free during concurrent atomic commits.
- [High] Missing `dc_lock` and IPS (Idle Power Save) handling in `amdgpu_dm_backlight_get_level`.
--

commit 38d7d185db9bb8880779014318e409480bde8a6d
Author: Mario Limonciello (AMD) <superm1@kernel.org>

drm/amd/display: Drop brightness caching in amdgpu_dm

This commit drops brightness and actual_brightness caching from amdgpu_dm, 
relying on the DRM core to restore brightness when a connector is turned 
back on.

> 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 9653cfd99c6ad..43b22ab9180e8 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
[ ... ]
> @@ -5399,10 +5393,8 @@ static void amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
>  		if (aconnector->bl_idx != bl_idx)
>  			continue;
>  

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to iterate the global 
connector_list in this loop without holding mode_config.connector_list_lock or 
using the drm_connector_list_iter API? 

Could this lead to memory corruption if there is a concurrent DP MST hot-unplug 
while a client reads or writes the sysfs backlight property?

> -		/* if connector is off, save the brightness for next time it's on */
> +		/* if connector is off, DRM core will restore it next time it's on */
>  		if (!aconnector->base.encoder) {
> -			dm->brightness[bl_idx] = user_brightness;
> -			dm->actual_brightness[bl_idx] = 0;
>  			return;
>  		}
>  	}

[Severity: High]
Is relying on the DRM core to restore the backlight sufficient here? 

Since amdgpu implements a custom atomic commit tail that bypasses the standard 
DRM core restoration helpers, it seems changes made while DPMS is off are 
never pushed to the hardware.

> @@ -5410,11 +5402,10 @@ static void amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
>  	amdgpu_dm_update_backlight_caps(dm, bl_idx);
>  	caps = &dm->backlight_caps[bl_idx];
>  
> -	dm->brightness[bl_idx] = user_brightness;
>  	/* 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]);
> +		amdgpu_atombios_scratch_regs_set_backlight_level(dm->adev, user_brightness);
> +	brightness = convert_brightness_from_user(caps, user_brightness);
>  	link = (struct dc_link *)dm->backlight_link[bl_idx];
>  
>  	/* Apply brightness quirk */

[Severity: High]
This is a pre-existing issue, but does the unprotected access to the stream 
returned downstream from dm_find_stream_with_link() risk a use-after-free? 

If a concurrent atomic commit modifies current_state and releases the old 
dc_stream_state objects via dc_stream_release() before 
amdgpu_dm_backlight_set_level() passes the stream downstream, the memory 
might be freed.

[ ... ]
> @@ -5522,7 +5510,7 @@ static int amdgpu_dm_backlight_get_level(struct amdgpu_display_manager *dm,
>  static int amdgpu_dm_backlight_get_brightness(struct backlight_device *bd)
>  {
>  	struct amdgpu_display_manager *dm = bl_get_data(bd);
> -	int i, ret;
> +	int i;
>  
>  	for (i = 0; i < dm->num_of_edps; i++) {
>  		if (bd == dm->backlight_dev[i])
> @@ -5531,11 +5519,7 @@ static int amdgpu_dm_backlight_get_brightness(struct backlight_device *bd)
>  	if (i >= AMDGPU_DM_MAX_NUM_EDP)
>  		i = 0;
>  
> -	ret = amdgpu_dm_backlight_get_level(dm, i);
> -	if (ret < 0)
> -		return dm->brightness[i];
> -
> -	return ret;
> +	return amdgpu_dm_backlight_get_level(dm, i);
>  }

[Severity: High]
This is a pre-existing issue, but does amdgpu_dm_backlight_get_level() need 
to acquire dc_lock and call dc_allow_idle_optimizations() before reading 
hardware registers downstream? 

If the hardware is in a low-power state when a user reads the sysfs brightness 
attribute, could this missing lock lead to a hardware hang or MMIO timeout?

[ ... ]
> @@ -11085,18 +11050,6 @@ static void amdgpu_dm_commit_streams(struct drm_atomic_commit *state,
>  		}
>  	}
>  
> -	/* During boot up and resume the DC layer will reset the panel brightness
> -	 * to fix a flicker issue.
> -	 * It will cause the dm->actual_brightness is not the current panel brightness
> -	 * level. (the dm->brightness is the correct panel level)
> -	 * So we set the backlight level with dm->brightness value after set mode
> -	 */
> -	if (set_backlight_level) {
> -		for (i = 0; i < dm->num_of_edps; i++) {
> -			if (dm->backlight_dev[i])
> -				amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i]);
> -		}
> -	}
>  }

[Severity: High]
Without this restoration loop in amdgpu_dm_commit_streams(), does the driver 
fail to correct the hardware brightness after the DC layer internally resets 
it during modesets? 

It appears the backlight might remain at incorrect hardware default or reset 
levels upon resume or modeset.

[ ... ]
> @@ -11604,13 +11557,6 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_commit *state)
>  	/* Update audio instances for each connector. */
>  	amdgpu_dm_commit_audio(dev, state);
>  
> -	/* restore the backlight level */
> -	for (i = 0; i < dm->num_of_edps; i++) {
> -		if (dm->backlight_dev[i] &&
> -		    (dm->actual_brightness[i] != dm->brightness[i]))
> -			amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i]);
> -	}
> -
>  	/*
>  	 * send vblank event on all events not handled in flip and
>  	 * mark consumed event for drm_atomic_helper_commit_hw_done

[Severity: High]
Does removing the explicit manual backlight restoration from 
amdgpu_dm_atomic_commit_tail() cause the pending backlight level to never be 
pushed to hardware when the display is enabled? 

Because this custom atomic commit tail bypasses standard DRM core restoration 
helpers, it seems changes made while DPMS is off are permanently lost without 
this loop.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624165751.2014759-1-mario.limonciello@amd.com?part=8