Re: [PATCH] drm/amd/display: Fix flip-done timeouts on mode1 reset

[Mario Limonciello <superm1@kernel.org>]

On 7/23/26 13:01, sunpeng.li@amd.com wrote:
> From: Leo Li <sunpeng.li@amd.com>
> 
> The vblank on/off callbacks mixed use of amdgpu_irq_get/put() and
> amdgpu_dm_crtc_set_vupdate_irq() to enable and disable IRQs.
> 
> With get/put, base driver will callback into DC to disable IRQs when
> refcount == 0. With set_vupdate_irq(), DC is called directly to disable
> IRQs, bypassing base driver's refcount tracking.
> 
> During gpu reset, base driver can restore IRQs via
> amdgpu_irq_gpu_reset_resume_helper() > amdgpu_irq_update(). So if
> get/put() is not used (i.e. refcount == 0), then vupdate_irq will be
> disabled.
> 
> This is problematic if DRM requests vblank on before amdgpu_irq_update()
> is called: drm_vblank_on() > set_vupdate_irq() enables vupdate_irq, but
> the refcount is still 0. gpu_reset_resume_helper() > irq_update() then
> immediately disables it, thus leading to flip done timeouts.
> 
> This is made worse on DCN since VUPDATE_NO_LOCK is the only IRQ enabled.
> Prior to 8382cd234981, a combination of GRPH_FLIP and VSTARTUP IRQs were
> used, and they used get/put(). This explains why 8382cd234981 exposed
> this issue.
> 
> Fix by using get/put() instead of set_vupdate_irq(). DCE is unchanged,
> since it relies on unbalanced enable/disable calls based on VRR status,
> and hence requires direct set_vupdate_irq(). Plus, it also uses
> GRPH_FLIP and VLINE IRQs, which are properly tracked by get/put().
> 
> Cc: stable@vger.kernel.org
> Fixes: 8382cd234981 ("drm/amd/display: consolidate DCN vblank/flip handling onto vupdate_no_lock")
> Signed-off-by: Leo Li <sunpeng.li@amd.com>
> ---
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c  | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
> index 05d6915f9a6b0..079d4ccc88da6 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
> @@ -287,10 +287,19 @@ static inline int amdgpu_dm_crtc_set_vblank(struct drm_crtc *crtc, bool enable)
>   	 * is enabled. On DCE, vupdate is only needed in VRR mode.
>   	 */
>   	if (amdgpu_ip_version(adev, DCE_HWIP, 0) != 0) {
> -		rc = amdgpu_dm_crtc_set_vupdate_irq(crtc, enable);
> +		if (enable) {
> +			rc = amdgpu_irq_get(adev, &adev->vupdate_irq, irq_type);
> +			drm_dbg_vbl(crtc->dev, "Get vupdate_irq ret=%d\n", rc);
> +		} else {
> +			rc = amdgpu_irq_put(adev, &adev->vupdate_irq, irq_type);
> +			drm_dbg_vbl(crtc->dev, "Put vupdate_irq ret=%d\n", rc);
> +		}

For completeness in error handling, shouldn't you pass up the return 
code on non-zero?  It looks like it could pass up to DRM core then.

>   	} else if (dc_supports_vrr(dm->dc->ctx->dce_version)) {
>   		if (enable) {
> -			/* vblank irq on -> Only need vupdate irq in vrr mode */
> +			/* vblank irq on -> Only need vupdate irq in vrr mode
> +			 * Not ref-counted since we need explicit enable/disable
> +			 * for DCE VRR handling
> +			 */
>   			if (amdgpu_dm_crtc_vrr_active(acrtc_state))
>   				rc = amdgpu_dm_crtc_set_vupdate_irq(crtc, true);
>   		} else {