Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Low] drm/amd/display: commit message claims non-modeset stream update path - [Low] drm/amd/display: duplicate connector state helpers in backlight KUnit test --
commit 60bc0c08268c70e96bfcf62cb19e21a33e151366 Author: Shivam Kalra <[email protected]> drm/amd/display: use atomic commit for panel power savings This commit updates panel_power_savings_store() to apply ABM level changes through an atomic commit instead of mutating connector->state directly and emitting a hotplug uevent, and extends the backlight KUnit test suite. > drm/amd/display: use atomic commit for panel power savings > > panel_power_savings_store() mutates connector->state directly and sends a > hotplug event. This bypasses atomic commit handling, so the new ABM level > is only applied when userspace happens to perform another modeset, and it > races with concurrent commits. > > Build an atomic state, set the level on the connector state and commit it. > If the connector is not attached to a CRTC, keep the level in the current > state so the next modeset picks it up, as before. > > A previous attempt to avoid the modeset by calling into DMUB directly was > reverted because it broke the abm_gradual IGT test (8d20a066fa9b, > 95aaa207e9ef). This version instead uses the normal atomic stream update > path. > > Expose the atomic state update helper for KUnit and extend the tests to > cover the atomic update and the inactive sysfs path. > > Fixes: 63d0b87213a0 ("drm/amd/display: add panel_power_savings sysfs > entry to eDP connectors") > Signed-off-by: Shivam Kalra <[email protected]> [Severity: Low] This isn't a bug, but is the commit message accurate when it says that this version avoids a modeset by using the normal atomic stream update path? When panel_power_savings_store() calls drm_atomic_commit(state) in drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_backlight.c, amdgpu_dm_atomic_check() in drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c marks connectors_changed when abm_level changes: if (dm_old_con_state->abm_level != dm_new_con_state->abm_level || dm_old_con_state->scaling != dm_new_con_state->scaling) new_crtc_state->connectors_changed = true; Because connectors_changed is set, drm_atomic_crtc_needs_modeset() returns true, causing amdgpu_dm_atomic_commit_tail() to perform a full modeset and skip the non-modeset connector stream update loop: /* Skip any modesets/resets */ if (!acrtc || drm_atomic_crtc_needs_modeset(new_crtc_state)) continue; The note below the "---" separator mentions that standalone operation still performs a modeset and depends on an unmerged companion series to use the non-modeset stream update path, but that note is stripped when the patch is applied. Should the commit message itself describe the standalone modeset behavior? > diff --git > a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c > index c6d15fb8ad989..a482261e85d25 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c > @@ -514,6 +514,37 @@ static void > dm_test_register_backlight_device_success(struct kunit *test) > adev->dm.backlight_dev[0]), 0); > } > > +static struct drm_connector_state * > +dm_test_panel_power_savings_duplicate_state(struct drm_connector *connector) > +{ > + struct dm_connector_state *state; > + > + state = kmemdup(to_dm_connector_state(connector->state), > + sizeof(*state), GFP_KERNEL); > + if (!state) > + return NULL; > + > + __drm_atomic_helper_connector_duplicate_state(connector, &state->base); > + > + return &state->base; > +} > + > +static void > +dm_test_panel_power_savings_destroy_state(struct drm_connector *connector, > + struct drm_connector_state *state) > +{ > + __drm_atomic_helper_connector_destroy_state(state); > + kfree(to_dm_connector_state(state)); > +} > + > +static const struct drm_connector_funcs > dm_test_panel_power_savings_connector_funcs = { > + .atomic_duplicate_state = dm_test_panel_power_savings_duplicate_state, > + .atomic_destroy_state = dm_test_panel_power_savings_destroy_state, > +}; [Severity: Low] This isn't a bug, but could dm_test_panel_power_savings_connector_funcs in drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_backlight_test.c reuse amdgpu_dm_connector_atomic_duplicate_state() and drm_atomic_helper_connector_destroy_state() instead of defining dm_test_panel_power_savings_duplicate_state() and dm_test_panel_power_savings_destroy_state()? In drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_connector.c, amdgpu_dm_connector_atomic_duplicate_state() is already exported with EXPORT_IF_KUNIT and performs the same kmemdup() and __drm_atomic_helper_connector_duplicate_state() sequence, and dm_test_connector_funcs in drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_connector_test.c uses both helpers directly: static const struct drm_connector_funcs dm_test_connector_funcs = { .reset = amdgpu_dm_connector_funcs_reset, .atomic_duplicate_state = amdgpu_dm_connector_atomic_duplicate_state, .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, }; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
