Re: [PATCH] drm/amd/display: Only allow idle optimizations when all CRTCs are idle

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] drm/amd/display: idle optimization stall in
  dm_ism_all_crtcs_idle()
- [High] drm/amd/display: unlocked crtc->state access in
  dm_ism_all_crtcs_idle()
- [Medium] drm/amd/display: NULL dereference in dm_ism_all_crtcs_idle()
  in KUnit tests
--

commit 41942a97fda7dd8d4e331b05631c7ae4c3d2cd19
Author: Claudio Forain <claudioforain@gmail.com>

drm/amd/display: Only allow idle optimizations when all CRTCs are idle

This commit adds dm_ism_all_crtcs_idle() so that hardware-wide idle
optimizations are only enabled when every active CRTC's Idle State Manager
is in an optimized idle state.

Link: https://gitlab.freedesktop.org/drm/amd/-/issues/4906

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_ism.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_ism.c
> index 127eaba4de60b..79546ad592874 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_ism.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_ism.c
> @@ -276,6 +276,32 @@ bool dm_ism_trigger_event(struct amdgpu_dm_ism *ism,
>  EXPORT_IF_KUNIT(dm_ism_trigger_event);
>  
>  
> +/*
> + * Idle optimizations are HW-wide, but each CRTC runs its own ISM. Only allow
> + * them once every active CRTC's ISM has reached an optimized idle state, so a
> + * static CRTC cannot enable idle optimizations while another CRTC is still busy
> + * (e.g. receiving cursor updates without a vblank reference). Otherwise every
> + * cursor update on the busy CRTC must exit and re-enter idle through DMUB.
> + */
> +static bool dm_ism_all_crtcs_idle(struct amdgpu_display_manager *dm)
> +{
> +	struct drm_crtc *crtc;
> +
> +	drm_for_each_crtc(crtc, adev_to_drm(dm->adev)) {

[Severity: Medium]
Does walking adev_to_drm(dm->adev)->mode_config.crtc_list here cause a NULL
pointer dereference in the amdgpu_dm_ism KUnit test suite?

Unlike amdgpu_dm_ism_disable(), amdgpu_dm_ism_force_full_power() and
amdgpu_dm_ism_enable(), which pass dm->ddev to drm_for_each_crtc(), this
uses adev_to_drm(dm->adev).  In amdgpu_dm_ism_test.c, alloc_test_acrtc() and
setup_commit_test() set adev->dm.ddev = &adev->ddev, but leave adev->dm.adev
NULL and do not initialize mode_config.crtc_list or register the test CRTC on
it, crashing the six behavioral commit and worker KUnit tests when they reach
dm_ism_commit_idle_optimization_state().

> +		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
> +		struct dm_crtc_state *state = to_dm_crtc_state(crtc->state);
> +
> +		if (!state || !state->stream)
> +			continue;

[Severity: High]
Can reading crtc->state and dereferencing state->stream across all CRTCs
under only dm->dc_lock race with an atomic commit on another CRTC and cause a
use-after-free?

dm_ism_all_crtcs_idle() runs from ISM delayed work or
amdgpu_dm_crtc_vblank_control_worker() holding dm->dc_lock, while crtc->state
is protected by crtc->mutex.  An atomic commit on another CRTC that does not
take dm->dc_lock in amdgpu_dm_atomic_commit_tail() can swap and free the old
CRTC state concurrently:

CPU0 (ISM worker on CRTC 0)           CPU1 (atomic commit on CRTC 1)
dm_ism_delayed_work_func()
  mutex_lock(&dm->dc_lock)
  ...
  dm_ism_all_crtcs_idle(dm)
    state = to_dm_crtc_state(
              crtc1->state)
                                      drm_atomic_helper_swap_state()
                                        crtc1->state = new_crtc_state
                                      ...
                                      drm_atomic_commit_put()
                                        amdgpu_dm_crtc_destroy_state()
                                          kfree(old_crtc_state)
    if (!state || !state->stream)  <-- use-after-free

> +
> +		if (acrtc->ism.current_state != DM_ISM_STATE_OPTIMIZED_IDLE &&
> +		    acrtc->ism.current_state != DM_ISM_STATE_OPTIMIZED_IDLE_SSO)
> +			return false;

[Severity: High]
Can requiring every active CRTC to be in DM_ISM_STATE_OPTIMIZED_IDLE or
DM_ISM_STATE_OPTIMIZED_IDLE_SSO permanently block hardware-wide idle
optimizations?

Both amdgpu_dm_ism_init() and amdgpu_dm_ism_force_full_power() (called on
suspend and GPU reset) leave each CRTC in DM_ISM_STATE_FULL_POWER_RUNNING.
The only transition toward DM_ISM_STATE_OPTIMIZED_IDLE is
DM_ISM_EVENT_ENTER_IDLE_REQUESTED, which is only sent from
amdgpu_dm_crtc_vblank_control_worker() when vblank is disabled.

When a CRTC is enabled from off via a modeset without a pageflip event, or
restored on resume or GPU reset, manage_dm_interrupts() calls
drm_crtc_vblank_on_config() without enabling vblank.  If that display stays
static and never takes and drops a vblank reference, its ISM remains in
DM_ISM_STATE_FULL_POWER_RUNNING and dm_ism_all_crtcs_idle() returns false
forever.

Similarly, in amdgpu_dm_commit_streams(), a modeset or disable on a CRTC in
DM_ISM_STATE_HYSTERESIS_WAITING calls amdgpu_dm_ism_flush(), which cancels
delayed_work without updating current_state or re-evaluating idle state for
the remaining active CRTCs.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109230147.16302-1-claudioforain@gmail.com?part=1