Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Medium] drm/amd/display: stale brightness restored in amdgpu_dm_commit_streams() - [Medium] drm/amd/display: race and PSR exit in amdgpu_dm_backlight_restore() - [Medium] drm/amd/display: zero actual_brightness collision in amdgpu_dm_set_luminance() --
commit 1f2beac3090b1487d1ed83a29307a7bc88af9241 Author: Mario Limonciello <[email protected]> drm/amd: add luminance property support This commit exposes the DRM LUMINANCE connector property for internal panels when CONFIG_DRM_LUMINANCE is enabled and applies connector luminance updates through Display Core backlight helpers and atomic commit callbacks. > 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 56fca4e22df8d..2a9ddd1dc6f70 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c [ ... ] > @@ -4802,12 +4799,8 @@ static void amdgpu_dm_commit_streams(struct > drm_atomic_commit *state, > * 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]); > - } > - } > + if (set_backlight_level) > + amdgpu_dm_backlight_restore(dm); > } > > STATIC_IFN_KUNIT void > @@ -5413,6 +5406,9 @@ static void amdgpu_dm_atomic_commit_tail(struct > drm_atomic_commit *state) > /* Update audio instances for each connector. */ > amdgpu_dm_commit_audio(dev, state); > > + /* Apply connector luminance after the display pipeline is enabled. */ > + drm_atomic_helper_commit_luminance_enables(state); > + > /* restore the backlight level */ > for (i = 0; i < dm->num_of_edps; i++) { > if (dm->backlight_dev[i] && [Severity: Medium] When a connector is controlled via the LUMINANCE property, dm->brightness[] is only updated when drm_atomic_helper_commit_luminance_enables() calls amdgpu_dm_set_luminance() near the end of amdgpu_dm_atomic_commit_tail(). If userspace updates LUMINANCE while the connector is inactive (!new_conn_state->crtc or !new_crtc_state->active), or updates LUMINANCE in the same atomic commit as a modeset, dm->brightness[bl_idx] still holds the previous level when amdgpu_dm_commit_streams() runs earlier in amdgpu_dm_atomic_commit_tail(): amdgpu_dm_atomic_commit_tail() amdgpu_dm_commit_streams() amdgpu_dm_backlight_restore() amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i]) amdgpu_dm_mod_power_setup_streams() amdgpu_dm_commit_planes() ... drm_atomic_helper_commit_luminance_enables() amdgpu_dm_set_luminance() amdgpu_dm_backlight_set_level(dm, bl_idx, state->luminance) Because drm_atomic_helper_update_legacy_modeset_state() has already set aconnector->base.encoder and dc_commit_streams() has committed the stream, amdgpu_dm_backlight_restore() programs the panel hardware, dm->power_module->bl_state, and the ATOMBIOS scratch register with the stale dm->brightness[i] value before planes are committed, and then drm_atomic_helper_commit_luminance_enables() reprograms the hardware a second time with state->luminance. Furthermore, if a LUMINANCE change was committed while the connector was inactive and a later modeset exits self-refresh (old_crtc_state->self_refresh_active is true while old_conn_state->luminance == new_conn_state->luminance), drm_atomic_helper_commit_luminance_enables() skips calling set_luminance, leaving the panel permanently at the stale dm->brightness[i] level applied by amdgpu_dm_backlight_restore(). Should amdgpu_dm_commit_streams() avoid restoring stale dm->brightness[] for luminance-controlled connectors, or should dm->brightness[] be synced from the connector state before amdgpu_dm_backlight_restore() runs? > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c > index 0f664c658a127..f61e69bbdf69f 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c [ ... ] > @@ -675,6 +684,30 @@ void amdgpu_dm_setup_backlight_device(struct > amdgpu_display_manager *dm, > > amdgpu_dm_update_connector_ext_caps(aconnector); > > + /* > + * The property must exist before the device is registered. If ACPI > + * video should handle the backlight, leave it to > + * amdgpu_dm_register_backlight_device() to register the fallback. > + */ > + if (IS_ENABLED(CONFIG_DRM_LUMINANCE) && > acpi_video_backlight_use_native()) { > + caps = &dm->backlight_caps[bl_idx]; > + amdgpu_dm_backlight_fill_props(caps, > power_supply_is_system_supplied() > 0, > + !(amdgpu_dc_debug_mask & > + > DC_DISABLE_CUSTOM_BRIGHTNESS_CURVE), > + &props); > + /* Start from the level the hardware is actually at */ > + ret = amdgpu_dm_backlight_get_level(dm, bl_idx); > + if (ret >= 0 && ret <= props.max_brightness) > + props.brightness = ret; > + ret = drm_connector_attach_luminance_property(connector, 0, > + > props.max_brightness, > + props.brightness); > + if (!drm_WARN_ON(adev_to_drm(dm->adev), ret)) { > + dm->brightness[bl_idx] = props.brightness; > + dm->luminance[bl_idx] = true; > + } > + } > + > /* Offer ABM property when user didn't turn off by module parameter. > * OLED panels are included to support CACP (Content Adaptive > * Contrast and Power) feature via set_abm_level. > @@ -686,6 +719,45 @@ void amdgpu_dm_setup_backlight_device(struct > amdgpu_display_manager *dm, > } > EXPORT_IF_KUNIT(amdgpu_dm_setup_backlight_device); > > +void amdgpu_dm_set_luminance(const struct drm_connector_state *state) > +{ > + struct amdgpu_dm_connector *aconnector = > + to_amdgpu_dm_connector(state->connector); > + struct amdgpu_display_manager *dm = > + &drm_to_adev(state->connector->dev)->dm; > + > + int bl_idx = aconnector->bl_idx; > + > + if (bl_idx < 0) > + return; > + > + /* Skip if already applied, e.g. by amdgpu_dm_backlight_restore(). */ > + if (dm->brightness[bl_idx] == state->luminance && > + dm->actual_brightness[bl_idx] == state->luminance) > + return; > + > + amdgpu_dm_backlight_set_level(dm, bl_idx, state->luminance); > +} [Severity: Medium] Can amdgpu_dm_set_luminance() silently skip programming the hardware when state->luminance is 0? In amdgpu_dm_setup_backlight_device(), drm_connector_attach_luminance_property() registers 0 as the minimum valid luminance level, and dm->actual_brightness[] starts at 0 (unlike amdgpu_dm_register_backlight_device(), which seeds dm->actual_brightness[bl_idx] from the hardware level). In addition, amdgpu_dm_backlight_set_level() uses dm->actual_brightness[bl_idx] = 0 as the sentinel indicating that brightness was not applied while the connector had no encoder: /* if connector is off, save the brightness for next time it's on */ if (!aconnector->base.encoder) { dm->brightness[bl_idx] = user_brightness; dm->actual_brightness[bl_idx] = 0; return; } If dm->brightness[bl_idx] is 0 and dm->actual_brightness[bl_idx] is 0 (for instance, after saving brightness 0 while the connector was disabled, or if a prior amdgpu_dm_backlight_set_level(dm, bl_idx, 0) returned early or failed with rc == false), then calling amdgpu_dm_set_luminance() with state->luminance == 0 sees: if (dm->brightness[bl_idx] == state->luminance && dm->actual_brightness[bl_idx] == state->luminance) return; and returns early without applying minimum brightness to the panel. Also, in amdgpu_dm_atomic_commit_tail(), the fallback loop that retries unapplied brightness levels only checks dm->backlight_dev[i]: /* 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]); } Should dm->actual_brightness[] track whether a level has been applied without colliding with valid luminance 0, and should the retry loop in amdgpu_dm_atomic_commit_tail() also cover dm->luminance[i]? > + > +/** > + * amdgpu_dm_backlight_restore - reprogram the cached backlight levels > + * @dm: display manager > + * > + * Display Core may reset the panel brightness when committing streams, so > + * reapply the cached level of every panel whose backlight is controlled by > + * the driver, either through a backlight class device or through the > + * connector LUMINANCE property. > + */ > +void amdgpu_dm_backlight_restore(struct amdgpu_display_manager *dm) > +{ > + int i; > + > + for (i = 0; i < dm->num_of_edps; i++) { > + if (dm->backlight_dev[i] || dm->luminance[i]) > + amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i]); > + } > +} [Severity: Medium] Could calling amdgpu_dm_backlight_restore() from amdgpu_dm_commit_streams() on every stream commit cause a race with concurrent luminance updates and trigger unnecessary PSR/Replay/IPS exits on unaffected eDP panels? First, when a non-blocking modeset commit runs on an external display CRTC, do_aquire_global_lock() locks all modeset locks during atomic_check but does not add unmodified CRTCs (such as an active eDP CRTC) to the atomic state. Once drm_atomic_helper_swap_state() finishes and locks are dropped, a subsequent fast atomic commit updating LUMINANCE on the eDP connector drops dm->atomic_obj in amdgpu_dm_atomic_check() and runs amdgpu_dm_atomic_commit_tail() concurrently: CPU 0 (external CRTC modeset) CPU 1 (eDP fast luminance commit) amdgpu_dm_commit_streams() drm_atomic_helper_commit_luminance_enables() amdgpu_dm_backlight_restore() amdgpu_dm_set_luminance() reads old dm->brightness[0] amdgpu_dm_backlight_set_level(new) amdgpu_dm_backlight_set_level(old) dm->brightness[0] = new dm->brightness[0] = old dm->actual_brightness[0] = new overwrites hw & actual_brightness Because nothing serializes dm->brightness[] and dm->actual_brightness[] across these two commits, CPU 0 can overwrite the panel brightness back to the old level after CPU 1 has programmed the new luminance, leaving hardware brightness out of sync with connector->state->luminance. Second, whenever an external monitor undergoes a modeset while the eDP panel is idle in PSR, Replay, or IPS with unchanged luminance, amdgpu_dm_commit_streams() sets set_backlight_level = true and calls amdgpu_dm_backlight_restore(), which unconditionally calls amdgpu_dm_backlight_set_level() on all eDP panels. That exits IPS and latches psr_event_hw_programming and replay_event_hw_programming on the eDP stream, which is not cleared because the eDP CRTC has no plane commit in the state to run amdgpu_dm_enable_self_refresh(). Could amdgpu_dm_commit_streams() only restore backlight for eDP streams that actually underwent a modeset in that commit, or let drm_atomic_helper_commit_luminance_enables() handle luminance-controlled connectors? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20261103-superm1-backlight-property-v11-0-00cce7b2b...@kernel.org?part=4
