[PATCH v5 1/2] drm/atomic: attempt full modeset on page flip timeout

[PATCH v5 1/2] drm/atomic: attempt full modeset on page flip timeout

[Hamza Mahfooz]

We should try to recover from page flip timeouts. Forcing
a full modeset should be generic across all atomic KMS drivers,
so try that first.

Signed-off-by: Hamza Mahfooz <someguy@effective-light.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 51 +++++++++++++++++++++++++++--
 1 file changed, 48 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index a768398a1884..8ccc6e833c77 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1926,6 +1926,45 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
 
+static int force_full_modeset(struct drm_crtc *crtc)
+{
+	struct drm_modeset_acquire_ctx ctx;
+	struct drm_crtc_state *crtc_state;
+	struct drm_atomic_state *state;
+	int ret;
+	int err;
+
+	if (drm_atomic_crtc_needs_modeset(crtc->state))
+		return -EBUSY;
+
+	DRM_MODESET_LOCK_ALL_BEGIN(crtc->dev, ctx, 0, err);
+	state = drm_atomic_state_alloc(crtc->dev);
+	if (!state)
+		return -ENOMEM;
+
+	state->acquire_ctx = &ctx;
+
+	crtc_state = drm_atomic_get_crtc_state(state, crtc);
+	if (IS_ERR(crtc_state)) {
+		ret = PTR_ERR(crtc_state);
+		goto out;
+	}
+
+	crtc_state->connectors_changed = true;
+	crtc_state->mode_changed = true;
+	crtc_state->active_changed = true;
+
+	drm_info(crtc->dev,
+		 "[CRTC:%d:%s] Attempting force full modeset...\n",
+		 crtc->base.id, crtc->name);
+
+	ret = drm_atomic_commit(state);
+out:
+	drm_atomic_state_put(state);
+	DRM_MODESET_LOCK_ALL_END(crtc->dev, ctx, err);
+	return ret;
+}
+
 /**
  * drm_atomic_helper_wait_for_flip_done - wait for all page flips to be done
  * @dev: DRM device
@@ -1949,17 +1988,23 @@ void drm_atomic_helper_wait_for_flip_done(struct drm_device *dev,
 
 	for (i = 0; i < dev->mode_config.num_crtc; i++) {
 		struct drm_crtc_commit *commit = state->crtcs[i].commit;
-		int ret;
 
 		crtc = state->crtcs[i].ptr;
 
 		if (!crtc || !commit)
 			continue;
 
-		ret = wait_for_completion_timeout(&commit->flip_done, 10 * HZ);
-		if (ret == 0)
+		if (!wait_for_completion_timeout(&commit->flip_done, 10 * HZ)) {
+			int ret;
 			drm_err(dev, "[CRTC:%d:%s] flip_done timed out\n",
 				crtc->base.id, crtc->name);
+
+			ret = force_full_modeset(crtc);
+			if (ret)
+				drm_err(dev,
+					"[CRTC:%d:%s] force full modeset failed! ret=%d\n",
+					crtc->base.id, crtc->name, ret);
+		}
 	}
 
 	if (state->fake_commit)
-- 
2.54.0

[PATCH v5 2/2] drm/amd/display: add DMU timeout recovery support

[Hamza Mahfooz]

DMU already has robust hung state tracking, but timeout recovery
was never hooked up, so do so now.

Signed-off-by: Hamza Mahfooz <someguy@effective-light.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 23 ++++++++++++++-----
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  1 +
 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 12 ++++++++--
 3 files changed, 28 insertions(+), 8 deletions(-)

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 e96a12ff2d31..7be4ebee1cb7 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1246,7 +1246,7 @@ static  void amdgpu_dm_audio_eld_notify(struct amdgpu_device *adev, int pin)
 	}
 }
 
-static int dm_dmub_hw_init(struct amdgpu_device *adev)
+int amdgpu_dm_dmub_hw_init(struct amdgpu_device *adev)
 {
 	const struct dmcub_firmware_header_v1_0 *hdr;
 	struct dmub_srv *dmub_srv = adev->dm.dmub_srv;
@@ -1315,7 +1315,7 @@ static int dm_dmub_hw_init(struct amdgpu_device *adev)
 	/* if adev->firmware.load_type == AMDGPU_FW_LOAD_PSP,
 	 * amdgpu_ucode_init_single_fw will load dmub firmware
 	 * fw_inst_const part to cw0; otherwise, the firmware back door load
-	 * will be done by dm_dmub_hw_init
+	 * will be done by amdgpu_dm_dmub_hw_init().
 	 */
 	if (adev->firmware.load_type != AMDGPU_FW_LOAD_PSP) {
 		memcpy(fb_info->fb[DMUB_WINDOW_0_INST_CONST].cpu_addr, fw_inst_const,
@@ -1457,7 +1457,7 @@ static void dm_dmub_hw_resume(struct amdgpu_device *adev)
 			drm_warn(adev_to_drm(adev), "Wait for DMUB auto-load failed: %d\n", status);
 	} else {
 		/* Perform the full hardware initialization. */
-		r = dm_dmub_hw_init(adev);
+		r = amdgpu_dm_dmub_hw_init(adev);
 		if (r)
 			drm_err(adev_to_drm(adev), "DMUB interface failed to initialize: status=%d\n", r);
 	}
@@ -2041,6 +2041,9 @@ static int amdgpu_dm_init(struct amdgpu_device *adev)
 		goto error;
 	}
 
+	adev->dm.dc->debug.enable_dmu_recovery =
+		amdgpu_device_should_recover_gpu(adev);
+
 	if (amdgpu_dc_debug_mask & DC_DISABLE_PIPE_SPLIT) {
 		adev->dm.dc->debug.force_single_disp_pipe_split = false;
 		adev->dm.dc->debug.pipe_split_policy = MPC_SPLIT_AVOID;
@@ -2090,7 +2093,7 @@ static int amdgpu_dm_init(struct amdgpu_device *adev)
 	if (adev->dm.dc->caps.dp_hdmi21_pcon_support)
 		drm_info(adev_to_drm(adev), "DP-HDMI FRL PCON supported\n");
 
-	r = dm_dmub_hw_init(adev);
+	r = amdgpu_dm_dmub_hw_init(adev);
 	if (r) {
 		drm_err(adev_to_drm(adev), "DMUB interface failed to initialize: status=%d\n", r);
 		goto error;
@@ -3604,7 +3607,7 @@ static int dm_resume(struct amdgpu_ip_block *ip_block)
 		 */
 		link_enc_cfg_copy(adev->dm.dc->current_state, dc_state);
 
-		r = dm_dmub_hw_init(adev);
+		r = amdgpu_dm_dmub_hw_init(adev);
 		if (r) {
 			drm_err(adev_to_drm(adev), "DMUB interface failed to initialize: status=%d\n", r);
 			return r;
@@ -9623,7 +9626,15 @@ static void prepare_flip_isr(struct amdgpu_crtc *acrtc)
 {
 
 	assert_spin_locked(&acrtc->base.dev->event_lock);
-	WARN_ON(acrtc->event);
+
+	/*
+	 * Compositors will refuse to make forward progress unless we send
+	 * the previous flip's completion event.
+	 */
+	if (WARN_ON(acrtc->event)) {
+		drm_crtc_send_vblank_event(&acrtc->base, acrtc->event);
+		drm_crtc_vblank_put(&acrtc->base);
+	}
 
 	acrtc->event = acrtc->base.state->event;
 
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index 74a8fe1a1999..dc808ee83c2a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -1086,6 +1086,7 @@ int amdgpu_dm_verify_lut3d_size(struct amdgpu_device *adev,
 #define MAX_COLOR_LEGACY_LUT_ENTRIES 256
 
 void amdgpu_dm_init_color_mod(void);
+int amdgpu_dm_dmub_hw_init(struct amdgpu_device *adev);
 int amdgpu_dm_create_color_properties(struct amdgpu_device *adev);
 int amdgpu_dm_verify_lut_sizes(const struct drm_crtc_state *crtc_state);
 int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc);
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index 3b8ae7798a93..8f10117483e2 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -33,6 +33,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/amdgpu_drm.h>
+#include <drm/drm_drv.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_fixed.h>
 
@@ -1165,8 +1166,15 @@ void dm_set_dcn_clocks(struct dc_context *ctx, struct dc_clocks *clks)
 
 void dm_helpers_dmu_timeout(struct dc_context *ctx)
 {
-	// TODO:
-	//amdgpu_device_gpu_recover(dc_context->driver-context, NULL);
+	struct amdgpu_device *adev = ctx->driver_context;
+
+	lockdep_assert_held(&adev->dm.dc_lock);
+
+	drm_info(adev_to_drm(adev), "attempting firmware reset\n");
+	if (amdgpu_dm_dmub_hw_init(adev))
+		drm_dev_wedged_event(adev_to_drm(adev),
+				     DRM_WEDGE_RECOVERY_REBIND |
+				     DRM_WEDGE_RECOVERY_BUS_RESET, NULL);
 }
 
 void dm_helpers_smu_timeout(struct dc_context *ctx, unsigned int msg_id, unsigned int param, unsigned int timeout_us)
-- 
2.54.0

Re: [PATCH v5 1/2] drm/atomic: attempt full modeset on page flip timeout

[Mario Limonciello]

On 5/1/26 15:35, Hamza Mahfooz wrote:
> We should try to recover from page flip timeouts. Forcing
> a full modeset should be generic across all atomic KMS drivers,
> so try that first.
> 
> Signed-off-by: Hamza Mahfooz <someguy@effective-light.com>
> ---
>   drivers/gpu/drm/drm_atomic_helper.c | 51 +++++++++++++++++++++++++++--
>   1 file changed, 48 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index a768398a1884..8ccc6e833c77 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1926,6 +1926,45 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
>   }
>   EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
>   
> +static int force_full_modeset(struct drm_crtc *crtc)
> +{
> +	struct drm_modeset_acquire_ctx ctx;
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_atomic_state *state;
> +	int ret;
> +	int err;
> +
> +	if (drm_atomic_crtc_needs_modeset(crtc->state))
> +		return -EBUSY;
> +
> +	DRM_MODESET_LOCK_ALL_BEGIN(crtc->dev, ctx, 0, err);
> +	state = drm_atomic_state_alloc(crtc->dev);
> +	if (!state)
> +		return -ENOMEM;
> +
> +	state->acquire_ctx = &ctx;
> +
> +	crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +	if (IS_ERR(crtc_state)) {
> +		ret = PTR_ERR(crtc_state);
> +		goto out;
> +	}
> +
> +	crtc_state->connectors_changed = true;
> +	crtc_state->mode_changed = true;
> +	crtc_state->active_changed = true;

Do you actually need to set all 3 of these to true?

I would think you only need:

crtc_state->mode_changed = true;

> +
> +	drm_info(crtc->dev,
> +		 "[CRTC:%d:%s] Attempting force full modeset...\n",
> +		 crtc->base.id, crtc->name);
> +
> +	ret = drm_atomic_commit(state);
> +out:
> +	drm_atomic_state_put(state);
> +	DRM_MODESET_LOCK_ALL_END(crtc->dev, ctx, err);
> +	return ret;
> +}
> +
>   /**
>    * drm_atomic_helper_wait_for_flip_done - wait for all page flips to be done
>    * @dev: DRM device
> @@ -1949,17 +1988,23 @@ void drm_atomic_helper_wait_for_flip_done(struct drm_device *dev,
>   
>   	for (i = 0; i < dev->mode_config.num_crtc; i++) {
>   		struct drm_crtc_commit *commit = state->crtcs[i].commit;
> -		int ret;
>   
>   		crtc = state->crtcs[i].ptr;
>   
>   		if (!crtc || !commit)
>   			continue;
>   
> -		ret = wait_for_completion_timeout(&commit->flip_done, 10 * HZ);
> -		if (ret == 0)
> +		if (!wait_for_completion_timeout(&commit->flip_done, 10 * HZ)) {
> +			int ret;
>   			drm_err(dev, "[CRTC:%d:%s] flip_done timed out\n",
>   				crtc->base.id, crtc->name);
> +
> +			ret = force_full_modeset(crtc);
> +			if (ret)
> +				drm_err(dev,
> +					"[CRTC:%d:%s] force full modeset failed! ret=%d\n",
> +					crtc->base.id, crtc->name, ret);
> +		}
>   	}
>   
>   	if (state->fake_commit)

Re: [PATCH v5 2/2] drm/amd/display: add DMU timeout recovery support

[Mario Limonciello]

On 5/1/26 15:35, Hamza Mahfooz wrote:
> DMU already has robust hung state tracking, but timeout recovery
> was never hooked up, so do so now.
> 
> Signed-off-by: Hamza Mahfooz <someguy@effective-light.com>

Reviewed-by: Mario Limonciello (AMD) <superm1@kernel.org>

I think this is a reasonable approach.  But Leo should sign off on it to 
go forward.

> ---
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 23 ++++++++++++++-----
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  1 +
>   .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 12 ++++++++--
>   3 files changed, 28 insertions(+), 8 deletions(-)
> 
> 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 e96a12ff2d31..7be4ebee1cb7 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -1246,7 +1246,7 @@ static  void amdgpu_dm_audio_eld_notify(struct amdgpu_device *adev, int pin)
>   	}
>   }
>   
> -static int dm_dmub_hw_init(struct amdgpu_device *adev)
> +int amdgpu_dm_dmub_hw_init(struct amdgpu_device *adev)
>   {
>   	const struct dmcub_firmware_header_v1_0 *hdr;
>   	struct dmub_srv *dmub_srv = adev->dm.dmub_srv;
> @@ -1315,7 +1315,7 @@ static int dm_dmub_hw_init(struct amdgpu_device *adev)
>   	/* if adev->firmware.load_type == AMDGPU_FW_LOAD_PSP,
>   	 * amdgpu_ucode_init_single_fw will load dmub firmware
>   	 * fw_inst_const part to cw0; otherwise, the firmware back door load
> -	 * will be done by dm_dmub_hw_init
> +	 * will be done by amdgpu_dm_dmub_hw_init().
>   	 */
>   	if (adev->firmware.load_type != AMDGPU_FW_LOAD_PSP) {
>   		memcpy(fb_info->fb[DMUB_WINDOW_0_INST_CONST].cpu_addr, fw_inst_const,
> @@ -1457,7 +1457,7 @@ static void dm_dmub_hw_resume(struct amdgpu_device *adev)
>   			drm_warn(adev_to_drm(adev), "Wait for DMUB auto-load failed: %d\n", status);
>   	} else {
>   		/* Perform the full hardware initialization. */
> -		r = dm_dmub_hw_init(adev);
> +		r = amdgpu_dm_dmub_hw_init(adev);
>   		if (r)
>   			drm_err(adev_to_drm(adev), "DMUB interface failed to initialize: status=%d\n", r);
>   	}
> @@ -2041,6 +2041,9 @@ static int amdgpu_dm_init(struct amdgpu_device *adev)
>   		goto error;
>   	}
>   
> +	adev->dm.dc->debug.enable_dmu_recovery =
> +		amdgpu_device_should_recover_gpu(adev);
> +
>   	if (amdgpu_dc_debug_mask & DC_DISABLE_PIPE_SPLIT) {
>   		adev->dm.dc->debug.force_single_disp_pipe_split = false;
>   		adev->dm.dc->debug.pipe_split_policy = MPC_SPLIT_AVOID;
> @@ -2090,7 +2093,7 @@ static int amdgpu_dm_init(struct amdgpu_device *adev)
>   	if (adev->dm.dc->caps.dp_hdmi21_pcon_support)
>   		drm_info(adev_to_drm(adev), "DP-HDMI FRL PCON supported\n");
>   
> -	r = dm_dmub_hw_init(adev);
> +	r = amdgpu_dm_dmub_hw_init(adev);
>   	if (r) {
>   		drm_err(adev_to_drm(adev), "DMUB interface failed to initialize: status=%d\n", r);
>   		goto error;
> @@ -3604,7 +3607,7 @@ static int dm_resume(struct amdgpu_ip_block *ip_block)
>   		 */
>   		link_enc_cfg_copy(adev->dm.dc->current_state, dc_state);
>   
> -		r = dm_dmub_hw_init(adev);
> +		r = amdgpu_dm_dmub_hw_init(adev);
>   		if (r) {
>   			drm_err(adev_to_drm(adev), "DMUB interface failed to initialize: status=%d\n", r);
>   			return r;
> @@ -9623,7 +9626,15 @@ static void prepare_flip_isr(struct amdgpu_crtc *acrtc)
>   {
>   
>   	assert_spin_locked(&acrtc->base.dev->event_lock);
> -	WARN_ON(acrtc->event);
> +
> +	/*
> +	 * Compositors will refuse to make forward progress unless we send
> +	 * the previous flip's completion event.
> +	 */
> +	if (WARN_ON(acrtc->event)) {
> +		drm_crtc_send_vblank_event(&acrtc->base, acrtc->event);
> +		drm_crtc_vblank_put(&acrtc->base);
> +	}
>   
>   	acrtc->event = acrtc->base.state->event;
>   
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index 74a8fe1a1999..dc808ee83c2a 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -1086,6 +1086,7 @@ int amdgpu_dm_verify_lut3d_size(struct amdgpu_device *adev,
>   #define MAX_COLOR_LEGACY_LUT_ENTRIES 256
>   
>   void amdgpu_dm_init_color_mod(void);
> +int amdgpu_dm_dmub_hw_init(struct amdgpu_device *adev);
>   int amdgpu_dm_create_color_properties(struct amdgpu_device *adev);
>   int amdgpu_dm_verify_lut_sizes(const struct drm_crtc_state *crtc_state);
>   int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc);
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> index 3b8ae7798a93..8f10117483e2 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> @@ -33,6 +33,7 @@
>   #include <drm/drm_atomic.h>
>   #include <drm/drm_probe_helper.h>
>   #include <drm/amdgpu_drm.h>
> +#include <drm/drm_drv.h>
>   #include <drm/drm_edid.h>
>   #include <drm/drm_fixed.h>
>   
> @@ -1165,8 +1166,15 @@ void dm_set_dcn_clocks(struct dc_context *ctx, struct dc_clocks *clks)
>   
>   void dm_helpers_dmu_timeout(struct dc_context *ctx)
>   {
> -	// TODO:
> -	//amdgpu_device_gpu_recover(dc_context->driver-context, NULL);
> +	struct amdgpu_device *adev = ctx->driver_context;
> +
> +	lockdep_assert_held(&adev->dm.dc_lock);
> +
> +	drm_info(adev_to_drm(adev), "attempting firmware reset\n");
> +	if (amdgpu_dm_dmub_hw_init(adev))
> +		drm_dev_wedged_event(adev_to_drm(adev),
> +				     DRM_WEDGE_RECOVERY_REBIND |
> +				     DRM_WEDGE_RECOVERY_BUS_RESET, NULL);
>   }
>   
>   void dm_helpers_smu_timeout(struct dc_context *ctx, unsigned int msg_id, unsigned int param, unsigned int timeout_us)

Re: [PATCH v5 1/2] drm/atomic: attempt full modeset on page flip timeout

[Hamza Mahfooz]

On Mon, May 04, 2026 at 04:50:21PM -0500, Mario Limonciello wrote:
> Do you actually need to set all 3 of these to true?
> 
> I would think you only need:
> 
> crtc_state->mode_changed = true;
> 

According to my testing `mode_changed` on it's own is sufficient for
amdgpu and the documentation [1] seems to suggest that it should be fine
more broadly. Though, it doesn't seem harmful to set all of them just
for safe measure.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/drm/drm_crtc.h?h=v7.1-rc2#n118

Re: [PATCH v5 1/2] drm/atomic: attempt full modeset on page flip timeout

[Mario Limonciello]

On 5/5/26 06:41, Hamza Mahfooz wrote:
> On Mon, May 04, 2026 at 04:50:21PM -0500, Mario Limonciello wrote:
>> Do you actually need to set all 3 of these to true?
>>
>> I would think you only need:
>>
>> crtc_state->mode_changed = true;
>>
> 
> According to my testing `mode_changed` on it's own is sufficient for
> amdgpu and the documentation [1] seems to suggest that it should be fine
> more broadly. Though, it doesn't seem harmful to set all of them just
> for safe measure.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/drm/drm_crtc.h?h=v7.1-rc2#n118

My main concern was it's safe right now, but what if future changes to 
atomic control flow mean it executes more code paths than needed; or 
worse unexpected code paths.

Re: [PATCH v5 2/2] drm/amd/display: add DMU timeout recovery support

[Leo Li]

On 2026-05-01 16:35, Hamza Mahfooz wrote:
> DMU already has robust hung state tracking, but timeout recovery
> was never hooked up, so do so now.
> 
> Signed-off-by: Hamza Mahfooz <someguy@effective-light.com>

Thanks for the patch Hamza, implementing a DMUB reset in dm_helpers_dmu_timeout() sounds like the right approach.
One comment inline.

> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 23 ++++++++++++++-----
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  1 +
>  .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 12 ++++++++--
>  3 files changed, 28 insertions(+), 8 deletions(-)
> 
> 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 e96a12ff2d31..7be4ebee1cb7 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -1246,7 +1246,7 @@ static  void amdgpu_dm_audio_eld_notify(struct amdgpu_device *adev, int pin)
>  	}
>  }
>  
> -static int dm_dmub_hw_init(struct amdgpu_device *adev)
> +int amdgpu_dm_dmub_hw_init(struct amdgpu_device *adev)
>  {
>  	const struct dmcub_firmware_header_v1_0 *hdr;
>  	struct dmub_srv *dmub_srv = adev->dm.dmub_srv;
> @@ -1315,7 +1315,7 @@ static int dm_dmub_hw_init(struct amdgpu_device *adev)
>  	/* if adev->firmware.load_type == AMDGPU_FW_LOAD_PSP,
>  	 * amdgpu_ucode_init_single_fw will load dmub firmware
>  	 * fw_inst_const part to cw0; otherwise, the firmware back door load
> -	 * will be done by dm_dmub_hw_init
> +	 * will be done by amdgpu_dm_dmub_hw_init().
>  	 */
>  	if (adev->firmware.load_type != AMDGPU_FW_LOAD_PSP) {
>  		memcpy(fb_info->fb[DMUB_WINDOW_0_INST_CONST].cpu_addr, fw_inst_const,
> @@ -1457,7 +1457,7 @@ static void dm_dmub_hw_resume(struct amdgpu_device *adev)
>  			drm_warn(adev_to_drm(adev), "Wait for DMUB auto-load failed: %d\n", status);
>  	} else {
>  		/* Perform the full hardware initialization. */
> -		r = dm_dmub_hw_init(adev);
> +		r = amdgpu_dm_dmub_hw_init(adev);
>  		if (r)
>  			drm_err(adev_to_drm(adev), "DMUB interface failed to initialize: status=%d\n", r);
>  	}
> @@ -2041,6 +2041,9 @@ static int amdgpu_dm_init(struct amdgpu_device *adev)
>  		goto error;
>  	}
>  
> +	adev->dm.dc->debug.enable_dmu_recovery =
> +		amdgpu_device_should_recover_gpu(adev);
> +
>  	if (amdgpu_dc_debug_mask & DC_DISABLE_PIPE_SPLIT) {
>  		adev->dm.dc->debug.force_single_disp_pipe_split = false;
>  		adev->dm.dc->debug.pipe_split_policy = MPC_SPLIT_AVOID;
> @@ -2090,7 +2093,7 @@ static int amdgpu_dm_init(struct amdgpu_device *adev)
>  	if (adev->dm.dc->caps.dp_hdmi21_pcon_support)
>  		drm_info(adev_to_drm(adev), "DP-HDMI FRL PCON supported\n");
>  
> -	r = dm_dmub_hw_init(adev);
> +	r = amdgpu_dm_dmub_hw_init(adev);
>  	if (r) {
>  		drm_err(adev_to_drm(adev), "DMUB interface failed to initialize: status=%d\n", r);
>  		goto error;
> @@ -3604,7 +3607,7 @@ static int dm_resume(struct amdgpu_ip_block *ip_block)
>  		 */
>  		link_enc_cfg_copy(adev->dm.dc->current_state, dc_state);
>  
> -		r = dm_dmub_hw_init(adev);
> +		r = amdgpu_dm_dmub_hw_init(adev);
>  		if (r) {
>  			drm_err(adev_to_drm(adev), "DMUB interface failed to initialize: status=%d\n", r);
>  			return r;
> @@ -9623,7 +9626,15 @@ static void prepare_flip_isr(struct amdgpu_crtc *acrtc)
>  {
>  
>  	assert_spin_locked(&acrtc->base.dev->event_lock);
> -	WARN_ON(acrtc->event);
> +
> +	/*
> +	 * Compositors will refuse to make forward progress unless we send
> +	 * the previous flip's completion event.
> +	 */
> +	if (WARN_ON(acrtc->event)) {
> +		drm_crtc_send_vblank_event(&acrtc->base, acrtc->event);
> +		drm_crtc_vblank_put(&acrtc->base);
> +	}

I would expect this WARN_ON to occur only after the 10s flip_done timeout expires, allowing 'this' commit to progress with the previously armed acrtc->event and ->pflip_status from the previous commit ('this' commit would be gated by drm_atomic_helper_wait_for_dependencies).

In which case, we probably want to apply the same above hunk for the cursor path here and also raise a warning: https://elixir.bootlin.com/linux/v6.19.3/source/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#L10170

- Leo

>  
>  	acrtc->event = acrtc->base.state->event;
>  
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index 74a8fe1a1999..dc808ee83c2a 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -1086,6 +1086,7 @@ int amdgpu_dm_verify_lut3d_size(struct amdgpu_device *adev,
>  #define MAX_COLOR_LEGACY_LUT_ENTRIES 256
>  
>  void amdgpu_dm_init_color_mod(void);
> +int amdgpu_dm_dmub_hw_init(struct amdgpu_device *adev);
>  int amdgpu_dm_create_color_properties(struct amdgpu_device *adev);
>  int amdgpu_dm_verify_lut_sizes(const struct drm_crtc_state *crtc_state);
>  int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc);
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> index 3b8ae7798a93..8f10117483e2 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> @@ -33,6 +33,7 @@
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_probe_helper.h>
>  #include <drm/amdgpu_drm.h>
> +#include <drm/drm_drv.h>
>  #include <drm/drm_edid.h>
>  #include <drm/drm_fixed.h>
>  
> @@ -1165,8 +1166,15 @@ void dm_set_dcn_clocks(struct dc_context *ctx, struct dc_clocks *clks)
>  
>  void dm_helpers_dmu_timeout(struct dc_context *ctx)
>  {
> -	// TODO:
> -	//amdgpu_device_gpu_recover(dc_context->driver-context, NULL);
> +	struct amdgpu_device *adev = ctx->driver_context;
> +
> +	lockdep_assert_held(&adev->dm.dc_lock);
> +
> +	drm_info(adev_to_drm(adev), "attempting firmware reset\n");
> +	if (amdgpu_dm_dmub_hw_init(adev))
> +		drm_dev_wedged_event(adev_to_drm(adev),
> +				     DRM_WEDGE_RECOVERY_REBIND |
> +				     DRM_WEDGE_RECOVERY_BUS_RESET, NULL);
>  }
>  
>  void dm_helpers_smu_timeout(struct dc_context *ctx, unsigned int msg_id, unsigned int param, unsigned int timeout_us)

Re: [PATCH v5 2/2] drm/amd/display: add DMU timeout recovery support

[Leo Li]

On 2026-05-05 12:02, Leo Li wrote:
>> +	/*
>> +	 * Compositors will refuse to make forward progress unless we send
>> +	 * the previous flip's completion event.
>> +	 */
>> +	if (WARN_ON(acrtc->event)) {
>> +		drm_crtc_send_vblank_event(&acrtc->base, acrtc->event);
>> +		drm_crtc_vblank_put(&acrtc->base);
>> +	}
> I would expect this WARN_ON to occur only after the 10s flip_done timeout expires, allowing 'this' commit to progress with the previously armed acrtc->event and ->pflip_status from the previous commit ('this' commit would be gated by drm_atomic_helper_wait_for_dependencies).
> 
> In which case, we probably want to apply the same above hunk for the cursor path here and also raise a warning: https://elixir.bootlin.com/linux/v6.19.3/source/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#L10170

Hmm, scratch that, looks like I didn't finish my own thought:

I think this would also mean the compositor sent 'this' commit without waiting for the previous vblank event. IOW it's not that compositors refuse to make forward progress, but wait_for_dependencies() in kernel will wait for a flip_done completion that will never come. And that can be a long time, since the drm_crtc_commit_wait()s stack.

Indeed, it seems to be the case in the dmesg log attached to this issue: https://gitlab.freedesktop.org/drm/amd/-/work_items/4809

The back-to-back WARN_ONs are likely from the hunk above, and one level up at https://elixir.bootlin.com/linux/v7.0.1/source/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#L10186. The compositor attempts another commit soon after, and hits the series of timeouts within wait_for_dependencies(), before we hit the next series of WARN_ONs from the same locations.

So ultimately, I don't think sending the event here will do anything. Since by the time we hit that WARN_ON, we've already waited through wait_for_dependencies(). At which point, there aren't anymore waiters on it.

I'm thinking an alternative would be to issue a 1-2s delayed worker whenever prepare_flip_isr() is called. The worker is canceled whenever the event is sent from the irq handlers. If it runs, then the expected pflip interrupt never fired, so we deliver the event in the worker. It's effectively a SW fallback for event delivery.

Of course, it is only a fallback, ideally we figure out why interrupts were missed in the first place.

- Leo 

Re: [PATCH v5 2/2] drm/amd/display: add DMU timeout recovery support

[Leo Li]

On 2026-05-05 12:36, Leo Li wrote:
> 
> 
> On 2026-05-05 12:02, Leo Li wrote:
>>> +	/*
>>> +	 * Compositors will refuse to make forward progress unless we send
>>> +	 * the previous flip's completion event.
>>> +	 */
>>> +	if (WARN_ON(acrtc->event)) {
>>> +		drm_crtc_send_vblank_event(&acrtc->base, acrtc->event);
>>> +		drm_crtc_vblank_put(&acrtc->base);
>>> +	}
>> I would expect this WARN_ON to occur only after the 10s flip_done timeout expires, allowing 'this' commit to progress with the previously armed acrtc->event and ->pflip_status from the previous commit ('this' commit would be gated by drm_atomic_helper_wait_for_dependencies).
>>
>> In which case, we probably want to apply the same above hunk for the cursor path here and also raise a warning: https://elixir.bootlin.com/linux/v6.19.3/source/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#L10170
> 
> Hmm, scratch that, looks like I didn't finish my own thought:
> 
> I think this would also mean the compositor sent 'this' commit without waiting for the previous vblank event. IOW it's not that compositors refuse to make forward progress, but wait_for_dependencies() in kernel will wait for a flip_done completion that will never come. And that can be a long time, since the drm_crtc_commit_wait()s stack.
> 
> Indeed, it seems to be the case in the dmesg log attached to this issue: https://gitlab.freedesktop.org/drm/amd/-/work_items/4809
> 
> The back-to-back WARN_ONs are likely from the hunk above, and one level up at https://elixir.bootlin.com/linux/v7.0.1/source/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#L10186. The compositor attempts another commit soon after, and hits the series of timeouts within wait_for_dependencies(), before we hit the next series of WARN_ONs from the same locations.
> 
> So ultimately, I don't think sending the event here will do anything. Since by the time we hit that WARN_ON, we've already waited through wait_for_dependencies(). At which point, there aren't anymore waiters on it.
> 
> I'm thinking an alternative would be to issue a 1-2s delayed worker whenever prepare_flip_isr() is called. The worker is canceled whenever the event is sent from the irq handlers. If it runs, then the expected pflip interrupt never fired, so we deliver the event in the worker. It's effectively a SW fallback for event delivery.
> 
> Of course, it is only a fallback, ideally we figure out why interrupts were missed in the first place.
> 
> - Leo 

(Apologies for the spam)
But regarding this patch, I'm OK with dropping the above hunk but leaving the dm_helpers_dmu_timeout() implementation.
With that change, this is

Reviewed-by: Leo Li <sunpeng.li@amd.com>

Re: [PATCH v5 1/2] drm/atomic: attempt full modeset on page flip timeout

[Hamza Mahfooz]

On Tue, May 05, 2026 at 10:32:57AM -0500, Mario Limonciello wrote:
> 
> 
> On 5/5/26 06:41, Hamza Mahfooz wrote:
> > On Mon, May 04, 2026 at 04:50:21PM -0500, Mario Limonciello wrote:
> > > Do you actually need to set all 3 of these to true?
> > > 
> > > I would think you only need:
> > > 
> > > crtc_state->mode_changed = true;
> > > 
> > 
> > According to my testing `mode_changed` on it's own is sufficient for
> > amdgpu and the documentation [1] seems to suggest that it should be fine
> > more broadly. Though, it doesn't seem harmful to set all of them just
> > for safe measure.
> > 
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/drm/drm_crtc.h?h=v7.1-rc2#n118
> 
> My main concern was it's safe right now, but what if future changes to
> atomic control flow mean it executes more code paths than needed; or worse
> unexpected code paths.

Point taken, I'll respin this with them removed.