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) <[email protected]> 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/[email protected]?part=8
