Re: [PATCH] drm/amd/display: Hold the VRR vblank reference only on active CRTCs

Re: [PATCH] drm/amd/display: Hold the VRR vblank reference only on active CRTCs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/amd/display: dc_exit_ips_for_hw_access() disables IPS on CRTC disable
--

commit 8e13aac5aafca2cc16730e98147592bfe9db266f
Author: Hugo Sales <hugo@hsal.es>

drm/amd/display: Hold the VRR vblank reference only on active CRTCs

This patch gates the VRR active state check in
amdgpu_dm_handle_vrr_transition() on the CRTC's base.active flag so that
disabling a CRTC drops the VRR vblank reference and enabling it takes the
reference again.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_freesync.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_freesync.c
> index 386ba88c57c6a..ee8a11f62a0d3 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_freesync.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_freesync.c
> @@ -414,8 +414,16 @@ void amdgpu_dm_handle_vrr_transition(struct amdgpu_display_manager *dm,
>  				     struct dm_crtc_state *new_state)
>  {
>  	struct amdgpu_device *adev = dm->adev;
> -	bool old_vrr_active = amdgpu_dm_crtc_vrr_active(old_state);
> -	bool new_vrr_active = amdgpu_dm_crtc_vrr_active(new_state);
> +	/*
> +	 * A disabled CRTC keeps its freesync state, but drm_crtc_vblank_get()
> +	 * fails there because vblank is off. Hold the reference only while
> +	 * the CRTC is active as well, which also covers resume, where the
> +	 * reset state lost track of the reference taken before suspend
> +	 */
> +	bool old_vrr_active = old_state->base.active &&
> +			      amdgpu_dm_crtc_vrr_active(old_state);
> +	bool new_vrr_active = new_state->base.active &&
> +			      amdgpu_dm_crtc_vrr_active(new_state);

[Severity: Medium]
Can gating new_vrr_active on new_state->base.active leave DC idle power
optimizations (IPS) disabled when disabling the last active CRTC in a
headless configuration while VRR was active?

In drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c,
amdgpu_dm_atomic_commit_tail() first calls amdgpu_dm_commit_streams(),
which disables vblank via manage_dm_interrupts(), flushes
dm->vblank_control_workqueue, commits the stream removal, and enables idle
optimizations for headless display-off:

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:amdgpu_dm_commit_streams() {
	...
	/* Allow idle optimization when vblank count is 0 for display off */
	if ((dm->active_vblank_irq_count == 0) && amdgpu_dm_is_headless(dm->adev))
		dc_allow_idle_optimizations(dm->dc, true);
}

Later in amdgpu_dm_atomic_commit_tail(), amdgpu_dm_handle_vrr_transition()
runs. When disabling a CRTC that had VRR_STATE_ACTIVE_VARIABLE,
old_vrr_active is true and new_vrr_active is now false, so execution enters
the old_vrr_active && !new_vrr_active branch:

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_freesync.c:amdgpu_dm_handle_vrr_transition() {
	...
	} else if (old_vrr_active && !new_vrr_active) {
		...
		drm_crtc_vblank_put(new_state->base.crtc);
		...
		scoped_guard(mutex, &dm->dc_lock) {
			dc_exit_ips_for_hw_access(dm->dc);
			amdgpu_dm_psr_set_event(dm, new_state->stream, false,
				psr_event_vrr_transition, false);
			amdgpu_dm_replay_set_event(dm, new_state->stream, false,
				replay_event_vrr, false);
		}
	}
}

Because drm_crtc_vblank_off() already set vblank->inmodeset and holds a
vblank reference, drm_crtc_vblank_put() decrements vblank->refcount from 2
to 1 without scheduling vblank_control_work.

Then dc_exit_ips_for_hw_access(dm->dc) unconditionally calls
dc_allow_idle_optimizations(dm->dc, false), even though new_state->stream
is NULL when the CRTC is being disabled and both amdgpu_dm_psr_set_event()
and amdgpu_dm_replay_set_event() immediately return false at their !stream
checks without touching hardware.

Since new_state->stream is NULL, amdgpu_dm_commit_planes() is skipped, and
amdgpu_dm_idle_worker() breaks out of its loop upon seeing
!dc->idle_optimizations_allowed. Should the dc_lock block and
dc_exit_ips_for_hw_access() in that branch be skipped when
new_state->stream is NULL?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031143351.3222033-1-hugo@hsal.es?part=1