Skip to contentSkip to conversation

FAILED: patch "[PATCH] drm/amd/display: Atomize IRQ register read/modify/write ops" failed to apply to 7.2-stable tree

PATCH Applied [email protected] 3 participants 3 messages Sep 22 – Sep 24

Subsystems (4)
Stable branch DRM drivers Radeon and amdgpu DRM drivers AMD display core
lore
Share & tools
lore

[email protected]

author maintainer
#0 lore raw

The patch below does not apply to the 7.2-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

To reproduce the conflict and resubmit, you may use the following commands:

git fetch https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ linux-7.2.y
git checkout FETCH_HEAD
git cherry-pick -x 63e19ef3ddab806c472748c825f4dc88dcd994e8
# <resolve conflicts, build, test, etc.>
git commit -s
git send-email --to '<[email protected]>' --in-reply-to '2026092220-morale-drizzle-4aa3@gregkh' --subject-prefix 'PATCH 7.2.y' 'HEAD^..'

Possible dependencies:

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From 63e19ef3ddab806c472748c825f4dc88dcd994e8 Mon Sep 17 00:00:00 2001
From: Leo Li <[email protected]>
Date: Tue, 25 Aug 2026 23:21:53 -0400
Subject: [PATCH] drm/amd/display: Atomize IRQ register read/modify/write ops

[Why]

The OTG_GLOBAL_SYNC_STATUS register controls various HW IRQ sources for the output timing generator (OTG). VUPDATE_NO_LOCK is one of them.

To enable the IRQ, driver sets the VUPDATE_NO_LOCK_EN bit in the GLOBAL_SYNC_STATUS register.

To ack the IRQ after it fires, the driver sets the VUPDATE_NO_LOCK_CLEAR bit in the same GLOBAL_SYNC_STATUS register.

The bit sets are done through read/modify/write operations, which are not atomic. Thus, the following race is possible:

Thread A: IRQ handler:
*HW IRQ fires*
# IRQ disable
val = read(GLOBAL_SYNC_STATUS)
unset(val, VUPDATE_NO_LOCK_EN)
write(val, GLOBAL_SYNC_STATUS)
# ACK reads VUPDATE_NO_LOCK_EN unset val1 = read(GLOBAL_SYNC_STATUS) set(val1, VUPDATE_NO_LOCK_CLEAR)
# IRQ enable
val = read(GLOBAL_SYNC_STATUS)
set(val, VUPDATE_NO_LOCK_EN)
write(val, GLOBAL_SYNC_STATUS)
# BAD! clears VUPDATE_NO_LOCK_EN write(val1, GLOBAL_SYNC_STATUS)

Regarding the tagged Fixes: change, it appears the change made this race more likely to occur. Since VUPDATE_NO_LOCK is now the sole IRQ source for vblank handling, a single race on high refresh panels can lead to a time out.

[How]

The GLOBAL_SYNC_STATUS register is only one example, other IRQ control registers also share the same scheme. On top of GLOBAL_SYNC_STATUS, let's clean up those as well.

To keep things simple, Let's atomize the IRQ rmw ops via a single driver-wide spinlock. Due to the small scope of this lock, it is unlikely to cause noticeable overhead on top of all the existing locking within the IRQ set/handle paths.

Since DM is responsible for locking, wrap dc_interrupt_set/ack with the spinlock in the new amdgpu_dm_irq_set/ack functions. Migrate/drop all references in DM to dc_interrupt_set/ack to use amdgpu_dm_irq_set/ack instead.

Closes https://gitlab.freedesktop.org/drm/amd/-/work_items/5616
Fixes c87e6635d2db ("drm/amd/display: consolidate DCN vblank/flip handling onto vupdate_no_lock")
Reviewed-by Mario Limonciello <[email protected]>
Signed-off-by Leo Li <[email protected]>
Signed-off-by Chenyu Chen <[email protected]>
Tested-by Daniel Wheeler <[email protected]>
Signed-off-by Alex Deucher <[email protected]>
(cherry picked from commit 70de0a0216583a53c946155f8c8adedfdca6b4e7)
Cc [email protected]
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +2 -2
index 08b8605029ab..36d2f86f000a 100644
@@ -1420,7 +1420,7 @@ static void dm_gpureset_toggle_interrupts(struct amdgpu_device *adev,
 		if (acrtc && state->stream_status[i].plane_count != 0 &&
 		    amdgpu_ip_version(adev, DCE_HWIP, 0) == 0) {
 			irq_source = IRQ_TYPE_PFLIP + acrtc->otg_inst;
-			rc = dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
+			rc = amdgpu_dm_irq_set(adev, irq_source, enable) ? 0 : -EBUSY;
 			if (rc)
 				drm_warn(adev_to_drm(adev), "Failed to %s pflip interrupts\n",
 					 enable ? "enable" : "disable");

@@ -1444,7 +1444,7 @@ static void dm_gpureset_toggle_interrupts(struct amdgpu_device *adev,
 			/* During gpu-reset we disable and then enable vblank irq, so
 			 * don't use amdgpu_irq_get/put() to avoid refcount change.
 			 */
-			if (!dc_interrupt_set(adev->dm.dc, irq_source, enable))
+			if (!amdgpu_dm_irq_set(adev, irq_source, enable))
 				drm_warn(adev_to_drm(adev), "Failed to %sable vblank interrupt\n", enable ? "en" : "dis");
 
 		} else if (acrtc && state->stream_status[i].plane_count != 0) {
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h +12 -0
index 3524931451c8..881c8d1c3cc0 100644
@@ -552,6 +552,18 @@ struct amdgpu_display_manager {
 	struct common_irq_params
 	vupdate_params[DC_IRQ_SOURCE_VUPDATE6 - DC_IRQ_SOURCE_VUPDATE1 + 1];
 
+	/**
+	 * @irq_reg_lock:
+	 *
+	 * Serializes the read-modify-writes of the HW interrupt control
+	 * registers. Several interrupt sources share one register - e.g. the
+	 * enable and clear bits of both VSTARTUP (vblank) and VUPDATE_NO_LOCK
+	 * live in OTG_GLOBAL_SYNC_STATUS. Therefore, enabling one source must
+	 * not race with acking another. Held only across amdgpu_dm_irq_set()
+	 * and amdgpu_dm_irq_ack().
+	 */
+	spinlock_t irq_reg_lock;
+
 	/**
 	 * @dmub_trace_params:
 	 *
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c +2 -1
index 62eac6e65334..1d941be73561 100644
@@ -31,6 +31,7 @@
 #include "amdgpu_dm_psr.h"
 #include "amdgpu_dm_replay.h"
 #include "amdgpu_dm_crtc.h"
+#include "amdgpu_dm_irq.h"
 #include "amdgpu_dm_plane.h"
 #include "amdgpu_dm_trace.h"
 #include "amdgpu_dm_debugfs.h"

@@ -91,7 +92,7 @@ int amdgpu_dm_crtc_set_vupdate_irq(struct drm_crtc *crtc, bool enable)
 
 	irq_source = IRQ_TYPE_VUPDATE + acrtc->otg_inst;
 
-	rc = dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
+	rc = amdgpu_dm_irq_set(adev, irq_source, enable) ? 0 : -EBUSY;
 
 	DRM_DEBUG_VBL("crtc %d - vupdate irq %sabling: r=%d\n",
 		      acrtc->crtc_id, enable ? "en" : "dis", rc);
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c +2 -1
index 298de7b75ca8..ced8b3d2d762 100644
@@ -1439,12 +1439,13 @@ void dm_helpers_free_gpu_mem(
 
 bool dm_helpers_dmub_outbox_interrupt_control(struct dc_context *ctx, bool enable)
 {
+	struct amdgpu_device *adev = ctx->driver_context;
 	enum dc_irq_source irq_source;
 	bool ret;
 
 	irq_source = DC_IRQ_SOURCE_DMCUB_OUTBOX;
 
-	ret = dc_interrupt_set(ctx->dc, irq_source, enable);
+	ret = amdgpu_dm_irq_set(adev, irq_source, enable);
 
 	DRM_DEBUG_DRIVER("Dmub trace irq %sabling: r=%d\n",
 			 enable ? "en" : "dis", ret);
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c +38 -26

168 lines not on this page · raw message

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.h +28 -0
index 4c200a9614a7..bc16ecc67329 100644
@@ -89,6 +89,34 @@ void amdgpu_dm_irq_unregister_interrupt(struct amdgpu_device *adev,
 					enum dc_irq_source irq_source,
 					void *ih_index);
 
+/**
+ * amdgpu_dm_irq_set - enable or disable a DC interrupt source.
+ *
+ * @adev: AMD DRM device
+ * @src: DC interrupt source to toggle
+ * @enable: true to enable the source, false to disable it
+ *
+ * DM-wide replacement for dc_interrupt_set(). As locking is DM's
+ * responsibility, this is a thin wrapper serializes the underlying
+ * read-modify-write against the other interrupt sources sharing HW control
+ * registers with @src, so DM must never call dc_interrupt_set() directly.
+ *
+ * Returns: true if the source was toggled.
+ */
+bool amdgpu_dm_irq_set(struct amdgpu_device *adev, enum dc_irq_source src,
+		       bool enable);
+
+/**
+ * amdgpu_dm_irq_ack - acknowledge a DC interrupt source.
+ *
+ * @adev: AMD DRM device
+ * @src: DC interrupt source to acknowledge
+ *
+ * DM-wide replacement for dc_interrupt_ack(), serialized the same way as
+ * amdgpu_dm_irq_set().
+ */
+void amdgpu_dm_irq_ack(struct amdgpu_device *adev, enum dc_irq_source src);
+
 void amdgpu_dm_set_irq_funcs(struct amdgpu_device *adev);
 
 void amdgpu_dm_outbox_init(struct amdgpu_device *adev);
drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_crtc_test.c +10 -6
index 0d998f204250..ae0f4da96252 100644
@@ -436,7 +436,7 @@ static void dm_test_crtc_set_vupdate_irq_no_otg(struct kunit *test)
  * dm_test_crtc_set_vupdate_irq_dc_busy - Test vupdate irq when DC rejects request
  * @test: The KUnit test context
  *
- * With an OTG instance assigned but no DC attached, dc_interrupt_set() returns
+ * With an OTG instance assigned but no DC attached, amdgpu_dm_irq_set() returns
  * false and the function must report the request as busy (-EBUSY).
  */
 static void dm_test_crtc_set_vupdate_irq_dc_busy(struct kunit *test)

@@ -453,12 +453,12 @@ static void dm_test_crtc_set_vupdate_irq_dc_busy(struct kunit *test)
 	acrtc->base.dev = &adev->ddev;
 	acrtc->otg_inst = 0;
 
-	/* adev->dm.dc is NULL, so dc_interrupt_set() returns false. */
+	/* adev->dm.dc is NULL, so amdgpu_dm_irq_set() returns false. */
 	KUNIT_EXPECT_EQ(test,
 			amdgpu_dm_crtc_set_vupdate_irq(&acrtc->base, true), -EBUSY);
 }
 
-/* Per-source funcs let dc_interrupt_set() succeed without register access. */
+/* Per-source funcs let amdgpu_dm_irq_set() succeed without register access. */
 static bool dm_test_vupdate_irq_src_set(struct irq_service *irq_service,
 					const struct irq_source_info *info,
 					bool enable)

@@ -477,7 +477,7 @@ static struct irq_source_info_funcs dm_test_vupdate_irq_src_funcs = {
 	.ack = dm_test_vupdate_irq_src_ack,
 };
 
-/* A .set that fails so dc_interrupt_set() reports the source as busy. */
+/* A .set that fails so amdgpu_dm_irq_set() reports the source as busy. */
 static bool dm_test_vupdate_irq_src_set_busy(struct irq_service *irq_service,
 					     const struct irq_source_info *info,
 					     bool enable)

@@ -519,7 +519,9 @@ static void dm_test_crtc_set_vupdate_irq_enable(struct kunit *test)
 	irqs = kunit_kzalloc(test, sizeof(*irqs), GFP_KERNEL);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, irqs);
 
-	/* Populate the per-source info table so dc_interrupt_set() succeeds. */
+	/*
+	 * Populate the per-source info table so amdgpu_dm_irq_set() succeeds.
+	 */
 	info = kunit_kzalloc(test, sizeof(*info) * DAL_IRQ_SOURCES_NUMBER,
 			     GFP_KERNEL);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, info);

@@ -1018,7 +1020,9 @@ static void dm_test_crtc_enable_vblank_vupdate_busy(struct kunit *test)
 	irqs = kunit_kzalloc(test, sizeof(*irqs), GFP_KERNEL);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, irqs);
 
-	/* Per-source .set fails so dc_interrupt_set() reports the source busy. */
+	/*
+	 * Per-source .set fails so amdgpu_dm_irq_set() reports the source busy.
+	 */
 	info = kunit_kzalloc(test, sizeof(*info) * DAL_IRQ_SOURCES_NUMBER,
 			     GFP_KERNEL);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, info);
drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_helpers_test.c +6 -2
index 82e0c984693c..639512bea275 100644
@@ -2442,17 +2442,21 @@ static void dm_test_is_dp_sink_present_null_priv(struct kunit *test)
  * dm_test_dmub_outbox_interrupt_control_null_dc - Test outbox irq control with NULL dc
  * @test: The KUnit test context
  *
- * dc_interrupt_set() is NULL-safe and returns false when dc is NULL, so the
+ * amdgpu_dm_irq_set() is NULL-safe and returns false when dc is NULL, so the
  * helper returns false without touching real interrupt hardware.
  */
 static void dm_test_dmub_outbox_interrupt_control_null_dc(struct kunit *test)
 {
+	struct amdgpu_device *adev;
 	struct dc_context *ctx;
 
+	adev = kunit_kzalloc(test, sizeof(*adev), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, adev);
 	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
 	KUNIT_ASSERT_NOT_NULL(test, ctx);
+	ctx->driver_context = adev;
 
-	/* ctx->dc is NULL → dc_interrupt_set returns false */
+	/* adev->dm.dc is NULL → amdgpu_dm_irq_set returns false */
 	KUNIT_EXPECT_FALSE(test, dm_helpers_dmub_outbox_interrupt_control(ctx, true));
 	KUNIT_EXPECT_FALSE(test, dm_helpers_dmub_outbox_interrupt_control(ctx, false));
 }
drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_irq_test.c +17 -17
index 861ee9eaa032..95322d8c8613 100644
@@ -268,7 +268,7 @@ static bool dm_test_irq_src_ack(struct irq_service *irq_service,
 	return true;
 }
 
-/* Per-source funcs let dc_interrupt_set() succeed without register access. */
+/* Per-source funcs let amdgpu_dm_irq_set() succeed without register access. */
 static struct irq_source_info_funcs dm_test_irq_src_funcs = {
 	.set = dm_test_irq_src_set,
 	.ack = dm_test_irq_src_ack,

@@ -290,7 +290,7 @@ static struct dc *dm_test_alloc_dc_with_irq_service(struct kunit *test,
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, irqs);
 
 	/*
-	 * Populate the per-source info table so dc_interrupt_set()/_ack()
+	 * Populate the per-source info table so amdgpu_dm_irq_set()/_ack()
 	 * succeed without touching hardware registers.
 	 */
 	info = kunit_kzalloc(test, sizeof(*info) * DAL_IRQ_SOURCES_NUMBER,

@@ -1227,7 +1227,7 @@ static void dm_test_irq_suspend_empty(struct kunit *test)
 	KUNIT_ASSERT_EQ(test, amdgpu_dm_irq_init(adev), 0);
 
 	/*
-	 * With no registered handlers the HW dc_interrupt_set() calls are
+	 * With no registered handlers the amdgpu_dm_irq_set() calls are
 	 * skipped, so suspend must complete without touching the (absent) DC.
 	 */
 	amdgpu_dm_irq_suspend(adev);

@@ -1275,11 +1275,11 @@ static void dm_test_irq_resume_late_empty(struct kunit *test)
 }
 
 /**
- * dm_test_irq_suspend_registered - Test suspend reaches the dc_interrupt_set path
+ * dm_test_irq_suspend_registered - Test suspend reaches the irq set path
  * @test: The KUnit test context
  *
  * Registers a low-context HPD handler so the handler list is non-empty,
- * forcing amdgpu_dm_irq_suspend() to call dc_interrupt_set() (NULL-safe with
+ * forcing amdgpu_dm_irq_suspend() to call amdgpu_dm_irq_set() (NULL-safe with
  * no DC) and flush_work() on the registered handler.
  */
 static void dm_test_irq_suspend_registered(struct kunit *test)

@@ -1330,11 +1330,11 @@ static void dm_test_irq_suspend_disables_polling(struct kunit *test)
 }
 
 /**
- * dm_test_irq_resume_early_registered - Test early resume reaches dc_interrupt_set
+ * dm_test_irq_resume_early_registered - Test early resume reaches irq set
  * @test: The KUnit test context
  *
  * Registers a low-context HPD RX handler so early resume calls
- * dc_interrupt_set() for the short-pulse interrupt source.
+ * amdgpu_dm_irq_set() for the short-pulse interrupt source.
  */
 static void dm_test_irq_resume_early_registered(struct kunit *test)
 {

@@ -1358,10 +1358,10 @@ static void dm_test_irq_resume_early_registered(struct kunit *test)
 }
 
 /**
- * dm_test_irq_resume_late_registered - Test late resume reaches dc_interrupt_set
+ * dm_test_irq_resume_late_registered - Test late resume reaches irq set
  * @test: The KUnit test context
  *
- * Registers a low-context HPD handler so late resume calls dc_interrupt_set()
+ * Registers a low-context HPD handler so late resume calls amdgpu_dm_irq_set()
  * for the HPD interrupt source.
  */
 static void dm_test_irq_resume_late_registered(struct kunit *test)

@@ -1592,7 +1592,7 @@ static void dm_test_set_crtc_irq_state_enable(struct kunit *test)
 
 	/*
 	 * otg_inst >= 0 computes the irq source and reaches the NULL-safe
-	 * dc_interrupt_set(); the ips_support branch is skipped (dc == NULL).
+	 * amdgpu_dm_irq_set(); the ips_support branch is skipped (dc == NULL).
 	 */
 	acrtc->otg_inst = 3;
 	adev->mode_info.crtcs[0] = acrtc;

@@ -1671,8 +1671,8 @@ static void dm_test_set_vupdate_irq_state_enable(struct kunit *test)
  *
  * With a non-NULL DC that advertises IPS support and currently allows idle
  * optimizations, dm_irq_state() must call dc_allow_idle_optimizations() before
- * dc_interrupt_set(). disable_idle_power_optimizations makes that call a safe
- * early return, and per-source stub funcs let dc_interrupt_set() succeed.
+ * amdgpu_dm_irq_set(). disable_idle_power_optimizations makes that call a safe
+ * early return, and per-source stub funcs let amdgpu_dm_irq_set() succeed.
  */
 static void dm_test_set_crtc_irq_state_allows_idle(struct kunit *test)
 {

@@ -1891,7 +1891,7 @@ static void dm_test_set_hpd_irq_state_null_dc(struct kunit *test)
 	adev = kunit_kzalloc(test, sizeof(*adev), GFP_KERNEL);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, adev);
 
-	/* dc_interrupt_set() is a no-op when dc is NULL, so both states
+	/* amdgpu_dm_irq_set() is a no-op when dc is NULL, so both states
 	 * return 0 without dereferencing the (absent) DC.
 	 */
 	KUNIT_EXPECT_EQ(test, amdgpu_dm_set_hpd_irq_state(adev, NULL, AMDGPU_HPD_1,

@@ -1951,7 +1951,7 @@ static void dm_test_outbox_init_null_dc(struct kunit *test)
 	adev = kunit_kzalloc(test, sizeof(*adev), GFP_KERNEL);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, adev);
 
-	/* Single dc_interrupt_set() call must be skipped when dc is NULL. */
+	/* Single amdgpu_dm_irq_set() call must be skipped when dc is NULL. */
 	amdgpu_dm_outbox_init(adev);
 }
 

@@ -1969,7 +1969,7 @@ static void dm_test_hpd_init_empty_connectors(struct kunit *test)
 
 	/*
 	 * With an empty connector list the per-connector loop is skipped and
-	 * the initial clear loop relies on dc_interrupt_set() being a no-op
+	 * the initial clear loop relies on amdgpu_dm_irq_set() being a no-op
 	 * for a NULL dc, so init must complete without touching the DC.
 	 */
 	amdgpu_dm_hpd_init(adev);

@@ -2004,7 +2004,7 @@ static void dm_test_hpd_init_fini_with_connectors(struct kunit *test)
 
 	/*
 	 * num_hpd = 0 forces irq_type >= num_hpd so the loop takes the HW
-	 * fallback (dc_interrupt_set()) instead of amdgpu_irq_get(); with a
+	 * fallback (amdgpu_dm_irq_set()) instead of amdgpu_irq_get(); with a
 	 * NULL dc that fallback is a safe no-op.
 	 */
 	adev->mode_info.num_hpd = 0;

@@ -2090,7 +2090,7 @@ static void dm_test_hpd_init_fini_irq_ref(struct kunit *test)
 	/*
 	 * num_hpd >= 1 makes irq_type (0) < num_hpd, so the loop takes the
 	 * amdgpu_irq_get()/amdgpu_irq_put() branch instead of the
-	 * dc_interrupt_set() fallback. The mock device has irq.installed ==
+	 * amdgpu_dm_irq_set() fallback. The mock device has irq.installed ==
 	 * false, so both calls fail early with -ENOENT (logging an error)
 	 * without touching the base-driver irq state.
 	 */
#1 lore raw

PATCH 1/1 drm/amd/display: Atomize IRQ register read/modify/write ops

From: Leo Li <[email protected]>

[Why]

The OTG_GLOBAL_SYNC_STATUS register controls various HW IRQ sources for the output timing generator (OTG). VUPDATE_NO_LOCK is one of them.

To enable the IRQ, driver sets the VUPDATE_NO_LOCK_EN bit in the GLOBAL_SYNC_STATUS register.

To ack the IRQ after it fires, the driver sets the VUPDATE_NO_LOCK_CLEAR bit in the same GLOBAL_SYNC_STATUS register.

The bit sets are done through read/modify/write operations, which are not atomic. Thus, the following race is possible:

Thread A: IRQ handler:
*HW IRQ fires*
# IRQ disable
val = read(GLOBAL_SYNC_STATUS)
unset(val, VUPDATE_NO_LOCK_EN)
write(val, GLOBAL_SYNC_STATUS)
# ACK reads VUPDATE_NO_LOCK_EN unset val1 = read(GLOBAL_SYNC_STATUS) set(val1, VUPDATE_NO_LOCK_CLEAR)
# IRQ enable
val = read(GLOBAL_SYNC_STATUS)
set(val, VUPDATE_NO_LOCK_EN)
write(val, GLOBAL_SYNC_STATUS)
# BAD! clears VUPDATE_NO_LOCK_EN write(val1, GLOBAL_SYNC_STATUS)

Regarding the tagged Fixes: change, it appears the change made this race more likely to occur. Since VUPDATE_NO_LOCK is now the sole IRQ source for vblank handling, a single race on high refresh panels can lead to a time out.

[How]

The GLOBAL_SYNC_STATUS register is only one example, other IRQ control registers also share the same scheme. On top of GLOBAL_SYNC_STATUS, let's clean up those as well.

To keep things simple, Let's atomize the IRQ rmw ops via a single driver-wide spinlock. Due to the small scope of this lock, it is unlikely to cause noticeable overhead on top of all the existing locking within the IRQ set/handle paths.

Since DM is responsible for locking, wrap dc_interrupt_set/ack with the spinlock in the new amdgpu_dm_irq_set/ack functions. Migrate/drop all references in DM to dc_interrupt_set/ack to use amdgpu_dm_irq_set/ack instead.

Closes https://gitlab.freedesktop.org/drm/amd/-/work_items/5616
Fixes c87e6635d2db ("drm/amd/display: consolidate DCN vblank/flip handling onto vupdate_no_lock")
Reviewed-by Mario Limonciello <[email protected]>
Signed-off-by Leo Li <[email protected]>
Signed-off-by Chenyu Chen <[email protected]>
Tested-by Daniel Wheeler <[email protected]>
Signed-off-by Alex Deucher <[email protected]>
(cherry picked from commit 70de0a0216583a53c946155f8c8adedfdca6b4e7)
Cc [email protected]
(cherry picked from commit 63e19ef3ddab806c472748c825f4dc88dcd994e8)

Modified for unit tests not present in 7.2.y

Signed-off-by Mario Limonciello <[email protected]>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  4 +-
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 12 ++++
 .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c    |  3 +-
 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  3 +-
 .../drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c | 62 +++++++++++--------
 .../drm/amd/display/amdgpu_dm/amdgpu_dm_irq.h | 28 +++++++++
 6 files changed, 82 insertions(+), 30 deletions(-)
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +2 -2
index 9320e49e44289..0c57a419f07d1 100644
@@ -3332,7 +3332,7 @@ static void dm_gpureset_toggle_interrupts(struct amdgpu_device *adev,
 		if (acrtc && state->stream_status[i].plane_count != 0 &&
 		    amdgpu_ip_version(adev, DCE_HWIP, 0) == 0) {
 			irq_source = IRQ_TYPE_PFLIP + acrtc->otg_inst;
-			rc = dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
+			rc = amdgpu_dm_irq_set(adev, irq_source, enable) ? 0 : -EBUSY;
 			if (rc)
 				drm_warn(adev_to_drm(adev), "Failed to %s pflip interrupts\n",
 					 enable ? "enable" : "disable");

@@ -3356,7 +3356,7 @@ static void dm_gpureset_toggle_interrupts(struct amdgpu_device *adev,
 			/* During gpu-reset we disable and then enable vblank irq, so
 			 * don't use amdgpu_irq_get/put() to avoid refcount change.
 			 */
-			if (!dc_interrupt_set(adev->dm.dc, irq_source, enable))
+			if (!amdgpu_dm_irq_set(adev, irq_source, enable))
 				drm_warn(adev_to_drm(adev), "Failed to %sable vblank interrupt\n", enable ? "en" : "dis");
 
 		} else if (acrtc && state->stream_status[i].plane_count != 0) {
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h +12 -0
index dd199e0b79226..043ca49367132 100644
@@ -550,6 +550,18 @@ struct amdgpu_display_manager {
 	struct common_irq_params
 	vupdate_params[DC_IRQ_SOURCE_VUPDATE6 - DC_IRQ_SOURCE_VUPDATE1 + 1];
 
+	/**
+	 * @irq_reg_lock:
+	 *
+	 * Serializes the read-modify-writes of the HW interrupt control
+	 * registers. Several interrupt sources share one register - e.g. the
+	 * enable and clear bits of both VSTARTUP (vblank) and VUPDATE_NO_LOCK
+	 * live in OTG_GLOBAL_SYNC_STATUS. Therefore, enabling one source must
+	 * not race with acking another. Held only across amdgpu_dm_irq_set()
+	 * and amdgpu_dm_irq_ack().
+	 */
+	spinlock_t irq_reg_lock;
+
 	/**
 	 * @dmub_trace_params:
 	 *
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c +2 -1
index 56bf907f1f6cc..e236ea1dcd65a 100644
@@ -31,6 +31,7 @@
 #include "amdgpu_dm_psr.h"
 #include "amdgpu_dm_replay.h"
 #include "amdgpu_dm_crtc.h"
+#include "amdgpu_dm_irq.h"
 #include "amdgpu_dm_plane.h"
 #include "amdgpu_dm_trace.h"
 #include "amdgpu_dm_debugfs.h"

@@ -87,7 +88,7 @@ int amdgpu_dm_crtc_set_vupdate_irq(struct drm_crtc *crtc, bool enable)
 
 	irq_source = IRQ_TYPE_VUPDATE + acrtc->otg_inst;
 
-	rc = dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
+	rc = amdgpu_dm_irq_set(adev, irq_source, enable) ? 0 : -EBUSY;
 
 	DRM_DEBUG_VBL("crtc %d - vupdate irq %sabling: r=%d\n",
 		      acrtc->crtc_id, enable ? "en" : "dis", rc);
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c +2 -1
index 41aa8f9bb152b..903038ba4cf9f 100644
@@ -1377,12 +1377,13 @@ void dm_helpers_free_gpu_mem(
 
 bool dm_helpers_dmub_outbox_interrupt_control(struct dc_context *ctx, bool enable)
 {
+	struct amdgpu_device *adev = ctx->driver_context;
 	enum dc_irq_source irq_source;
 	bool ret;
 
 	irq_source = DC_IRQ_SOURCE_DMCUB_OUTBOX;
 
-	ret = dc_interrupt_set(ctx->dc, irq_source, enable);
+	ret = amdgpu_dm_irq_set(adev, irq_source, enable);
 
 	DRM_DEBUG_DRIVER("Dmub trace irq %sabling: r=%d\n",
 			 enable ? "en" : "dis", ret);
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c +36 -26

166 lines not on this page · raw message

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.h +28 -0
index 4f6b58f4f90d7..f0a4577848f35 100644
@@ -81,6 +81,34 @@ void amdgpu_dm_irq_unregister_interrupt(struct amdgpu_device *adev,
 					enum dc_irq_source irq_source,
 					void *ih_index);
 
+/**
+ * amdgpu_dm_irq_set - enable or disable a DC interrupt source.
+ *
+ * @adev: AMD DRM device
+ * @src: DC interrupt source to toggle
+ * @enable: true to enable the source, false to disable it
+ *
+ * DM-wide replacement for dc_interrupt_set(). As locking is DM's
+ * responsibility, this is a thin wrapper serializes the underlying
+ * read-modify-write against the other interrupt sources sharing HW control
+ * registers with @src, so DM must never call dc_interrupt_set() directly.
+ *
+ * Returns: true if the source was toggled.
+ */
+bool amdgpu_dm_irq_set(struct amdgpu_device *adev, enum dc_irq_source src,
+		       bool enable);
+
+/**
+ * amdgpu_dm_irq_ack - acknowledge a DC interrupt source.
+ *
+ * @adev: AMD DRM device
+ * @src: DC interrupt source to acknowledge
+ *
+ * DM-wide replacement for dc_interrupt_ack(), serialized the same way as
+ * amdgpu_dm_irq_set().
+ */
+void amdgpu_dm_irq_ack(struct amdgpu_device *adev, enum dc_irq_source src);
+
 void amdgpu_dm_set_irq_funcs(struct amdgpu_device *adev);
 
 void amdgpu_dm_outbox_init(struct amdgpu_device *adev);
Signature · 1 line
2.43.0
maintainer Applied
#2 lore raw

Re: [PATCH 7.2.y] drm/amd/display: Atomize IRQ register read/modify/write ops

Quoted 1 line
Fixes c87e6635d2db ("drm/amd/display: consolidate DCN vblank/flip handling onto vupdate_no_lock")

Queued for 7.2 and 6.18, thanks.

Signature · 2 lines
Thanks,
Sasha