[PATCH v4 1/2] drm: Introduce drm_crtc_vblank_prepare()

[PATCH v4 1/2] drm: Introduce drm_crtc_vblank_prepare()

[sunpeng.li]

From: Leo Li <sunpeng.li@amd.com>

Some drivers need to perform sleepable operations prior to enabling
vblank interrupts. A display hardware spin-up from a low-power state
that requires synchronization with the rest of the driver, for example.

To support this, introduce a DRM-internal drm_crtc_vblank_prepare()
helper that calls back into the driver -- if implemented -- for DRM to
do such preparation work before enabling vblank.

v3:
* Unexport drm_crtc_vblank_prepare() and make it DRM internal
* Drop warnings in drm core for vblank_prepare(), drivers can do so in
  their implementations
* Drop unnecessary crtc null checks
* Check for drm_dev_has_vblank()
* Rebase on latest drm-misc-next

v4:
* EXPORT function to fix build error on DRM_KMS_HELPER=m

Signed-off-by: Leo Li <sunpeng.li@amd.com>
---
 drivers/gpu/drm/drm_atomic_helper.c  |  9 +++++
 drivers/gpu/drm/drm_client_modeset.c |  4 +++
 drivers/gpu/drm/drm_internal.h       |  1 +
 drivers/gpu/drm/drm_plane.c          |  5 +++
 drivers/gpu/drm/drm_vblank.c         | 49 ++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_vblank_helper.c  |  5 ++-
 drivers/gpu/drm/drm_vblank_work.c    |  8 +++++
 include/drm/drm_crtc.h               | 21 ++++++++++++
 8 files changed, 101 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 5840e9cc6f666..2b9fa4aa48a1a 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -48,6 +48,7 @@
 
 #include "drm_crtc_helper_internal.h"
 #include "drm_crtc_internal.h"
+#include "drm_internal.h"
 
 /**
  * DOC: overview
@@ -1268,6 +1269,10 @@ crtc_disable(struct drm_device *dev, struct drm_atomic_state *state)
 		if (!drm_dev_has_vblank(dev))
 			continue;
 
+		ret = drm_crtc_vblank_prepare(crtc);
+		if (ret)
+			continue;
+
 		ret = drm_crtc_vblank_get(crtc);
 		/*
 		 * Self-refresh is not a true "disable"; ensure vblank remains
@@ -1823,6 +1828,10 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
 		if (!new_crtc_state->active)
 			continue;
 
+		ret = drm_crtc_vblank_prepare(crtc);
+		if (ret != 0)
+			continue;
+
 		ret = drm_crtc_vblank_get(crtc);
 		if (ret != 0)
 			continue;
diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
index fc4caf7da5fcd..6ccbde921dde4 100644
--- a/drivers/gpu/drm/drm_client_modeset.c
+++ b/drivers/gpu/drm/drm_client_modeset.c
@@ -1325,6 +1325,10 @@ int drm_client_modeset_wait_for_vblank(struct drm_client_dev *client, unsigned i
 	 * Only wait for a vblank event if the CRTC is enabled, otherwise
 	 * just don't do anything, not even report an error.
 	 */
+	ret = drm_crtc_vblank_prepare(crtc);
+	if (ret)
+		return ret;
+
 	ret = drm_crtc_vblank_get(crtc);
 	if (!ret) {
 		drm_crtc_wait_one_vblank(crtc);
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index f893b1e3a596e..8e3e21d734075 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -112,6 +112,7 @@ static inline bool drm_vblank_passed(u64 seq, u64 ref)
 }
 
 void drm_vblank_disable_and_save(struct drm_device *dev, unsigned int pipe);
+int drm_crtc_vblank_prepare(struct drm_crtc *crtc);
 int drm_vblank_get(struct drm_device *dev, unsigned int pipe);
 void drm_vblank_put(struct drm_device *dev, unsigned int pipe);
 u64 drm_vblank_count(struct drm_device *dev, unsigned int pipe);
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index bed2562bf911b..41681a3d96b15 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -35,6 +35,7 @@
 #include <drm/drm_vblank.h>
 
 #include "drm_crtc_internal.h"
+#include "drm_internal.h"
 
 /**
  * DOC: overview
@@ -1421,6 +1422,10 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 		u32 current_vblank;
 		int r;
 
+		r = drm_crtc_vblank_prepare(crtc);
+		if (r)
+			return r;
+
 		r = drm_crtc_vblank_get(crtc);
 		if (r)
 			return r;
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 42fe11cc139b9..f57de47438f0c 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -1208,6 +1208,33 @@ static int drm_vblank_enable(struct drm_device *dev, unsigned int pipe)
 	return ret;
 }
 
+/**
+ * drm_crtc_vblank_prepare - prepare to enable vblank interrupts
+ *
+ * @crtc: which CRTC to prepare
+ *
+ * Some drivers may need to spin-up hardware from a low power state before
+ * enabling vblank interrupts. This function calls the prepare_enable_vblank
+ * callback, if available, to allow drivers to do that.
+ *
+ * This is a DRM-internal function, and is a thin wrapper around a driver
+ * callback. Drivers are expected to sequence their own prepare work internally.
+ *
+ * The spin-up may call sleeping functions, such as mutex_lock(). Therefore,
+ * this must be called from process context, where sleeping is allowed.
+ */
+int drm_crtc_vblank_prepare(struct drm_crtc *crtc)
+{
+	if (!drm_dev_has_vblank(crtc->dev))
+		return -EINVAL;
+
+	if (crtc->funcs->prepare_enable_vblank)
+		return crtc->funcs->prepare_enable_vblank(crtc);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_crtc_vblank_prepare);
+
 int drm_vblank_get(struct drm_device *dev, unsigned int pipe)
 {
 	struct drm_vblank_crtc *vblank = drm_vblank_crtc(dev, pipe);
@@ -1306,6 +1333,10 @@ int drm_crtc_wait_one_vblank(struct drm_crtc *crtc)
 	int ret;
 	u64 last;
 
+	ret = drm_crtc_vblank_prepare(crtc);
+	if (ret)
+		return ret;
+
 	ret = drm_vblank_get(dev, pipe);
 	if (drm_WARN(dev, ret, "vblank not available on crtc %i, ret=%i\n",
 		     pipe, ret))
@@ -1489,6 +1520,9 @@ void drm_crtc_vblank_on_config(struct drm_crtc *crtc,
 	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
 		return;
 
+	if (drm_crtc_vblank_prepare(crtc))
+		return;
+
 	spin_lock_irq(&dev->vbl_lock);
 	drm_dbg_vbl(dev, "crtc %d, vblank enabled %d, inmodeset %d\n",
 		    pipe, vblank->enabled, vblank->inmodeset);
@@ -1796,6 +1830,13 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
 		return 0;
 	}
 
+	crtc = drm_crtc_from_index(dev, vblank->pipe);
+	if (crtc) {
+		ret = drm_crtc_vblank_prepare(crtc);
+		if (ret)
+			return ret;
+	}
+
 	ret = drm_vblank_get(dev, pipe);
 	if (ret) {
 		drm_dbg_core(dev,
@@ -2031,6 +2072,10 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
 		READ_ONCE(vblank->enabled);
 
 	if (!vblank_enabled) {
+		ret = drm_crtc_vblank_prepare(crtc);
+		if (ret)
+			return ret;
+
 		ret = drm_crtc_vblank_get(crtc);
 		if (ret) {
 			drm_dbg_core(dev,
@@ -2098,6 +2143,10 @@ int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data,
 	if (e == NULL)
 		return -ENOMEM;
 
+	ret = drm_crtc_vblank_prepare(crtc);
+	if (ret)
+		return ret;
+
 	ret = drm_crtc_vblank_get(crtc);
 	if (ret) {
 		drm_dbg_core(dev,
diff --git a/drivers/gpu/drm/drm_vblank_helper.c b/drivers/gpu/drm/drm_vblank_helper.c
index a04a6ba1b0ca0..fc5915acfa7f3 100644
--- a/drivers/gpu/drm/drm_vblank_helper.c
+++ b/drivers/gpu/drm/drm_vblank_helper.c
@@ -8,6 +8,8 @@
 #include <drm/drm_vblank.h>
 #include <drm/drm_vblank_helper.h>
 
+#include "drm_internal.h"
+
 /**
  * DOC: overview
  *
@@ -61,7 +63,8 @@ void drm_crtc_vblank_atomic_flush(struct drm_crtc *crtc,
 	crtc_state->event = NULL;
 
 	if (event) {
-		if (drm_crtc_vblank_get(crtc) == 0)
+		if (drm_crtc_vblank_prepare(crtc) == 0 &&
+		    drm_crtc_vblank_get(crtc) == 0)
 			drm_crtc_arm_vblank_event(crtc, event);
 		else
 			drm_crtc_send_vblank_event(crtc, event);
diff --git a/drivers/gpu/drm/drm_vblank_work.c b/drivers/gpu/drm/drm_vblank_work.c
index 70f0199251ea0..252f60007781b 100644
--- a/drivers/gpu/drm/drm_vblank_work.c
+++ b/drivers/gpu/drm/drm_vblank_work.c
@@ -113,11 +113,19 @@ int drm_vblank_work_schedule(struct drm_vblank_work *work,
 {
 	struct drm_vblank_crtc *vblank = work->vblank;
 	struct drm_device *dev = vblank->dev;
+	struct drm_crtc *crtc;
 	u64 cur_vbl;
 	unsigned long irqflags;
 	bool passed, inmodeset, rescheduling = false, wake = false;
 	int ret = 0;
 
+	crtc = drm_crtc_from_index(dev, vblank->pipe);
+	if (crtc) {
+		ret = drm_crtc_vblank_prepare(crtc);
+		if (ret)
+			return ret;
+	}
+
 	spin_lock_irqsave(&dev->event_lock, irqflags);
 	if (work->cancelling)
 		goto out;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 66278ffeebd68..e5cf232d604c9 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -871,6 +871,27 @@ struct drm_crtc_funcs {
 	 */
 	u32 (*get_vblank_counter)(struct drm_crtc *crtc);
 
+	/**
+	 * @prepare_enable_vblank:
+	 *
+	 * An optional callback to prepare driver for enabling of vblank
+	 * interrupts. It allows drivers to perform any blocking operations for
+	 * hardware setup that might be needed, and thus is called before any
+	 * vblank spinlocks are acquired. It is called unconditionally,
+	 * regardless of whether vblank interrupts are already enabled or not.
+	 *
+	 * Consequently, this callback is not synchronized with the rest of
+	 * vblank management. Drivers should not access spinlock protected
+	 * states here.
+	 *
+	 * This callback is optional. If not set, no preparation is performed.
+	 *
+	 * Returns:
+	 *
+	 * Zero on success, negative errno on failure.
+	 */
+	int (*prepare_enable_vblank)(struct drm_crtc *crtc);
+
 	/**
 	 * @enable_vblank:
 	 *
-- 
2.52.0

[PATCH v4 2/2] drm/amd/display: Implement prepare_vblank_enable callback

[sunpeng.li]

From: Leo Li <sunpeng.li@amd.com>

[Why]

APU DCN generations since DCN3.5 have the capability to power down
almost all of the DCN hw block during idle periods. This is referred to
as  IPS -- idle power states. In combination with a panel remote-buffer
feature (like PSR or Panel Replay), IPS can save additional power.

Once DCN is in an IPS, no register access can occur. This includes
control registers for vblank interrupts; IPS must first be exited.

Transitioning in or out of IPS requires synchronization with the rest of
DC, as it powers up or down DCN, and may communicate with other MCUs on
the SOC to do so. This is done via the dc_lock mutex.

While calling enable_vblank, the DRM vblank core holds spinlocks that
prevent blocking operations. Yet acquiring the dc_lock mutex is
blocking. Thus, IPS can not be exited piror to programming vblank
interrupt registers from within enable_vblank. At least not in a
race-free way.

Prior to this change, amdgpu_dm was exiting IPS(*) without holding the
dc_lock, opening the door for races:
https://gitlab.freedesktop.org/drm/amd/-/issues/5233

(*) From touching the interrupt registers. All register reads today have
an implicit IPS exit, see dm_read_reg_func()

To solve this, the prepare_vblank_enable callback can be implemented to
exit IPS, as it is called from process context.

[How]

Implement the prepare_vblank_enable callback for amdgpu_dm. In it,
the dc_lock mutex is acquired, and IPS is exited.

Note that the only place that should unconditionally IPS allow is the
vblank disable path. All other paths shall check whether IPS was
previously allowed. If so, they can re-allow after all programming is
complete. They also need to hold the dc_lock for the duration of the IPS
disallow to re-allow. (This is not the for all of amdgpu_dm today,
cleanup will come in future patches.)

v2: Add missing semicolon, add docstring for prepare_vbl_disallow_idle
v3: Do prepare work (IPS exit) directly, instead of routing through DRM
v4: Fix build error on CONFIG_DRM_AMD_SECURE_DISPLAY not set

Signed-off-by: Leo Li <sunpeng.li@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 37 ++++++++++++------
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  9 +++++
 .../drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c |  8 ++--
 .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c    | 38 +++++++++++++++++--
 4 files changed, 75 insertions(+), 17 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 740711ac1037c..d0c412260be0c 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -9681,7 +9681,8 @@ static void update_stream_irq_parameters(
 	spin_unlock_irqrestore(&adev_to_drm(adev)->event_lock, flags);
 }
 
-static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
+static void amdgpu_dm_handle_vrr_transition(struct amdgpu_display_manager *dm,
+					    struct dm_crtc_state *old_state,
 					    struct dm_crtc_state *new_state)
 {
 	bool old_vrr_active = amdgpu_dm_crtc_vrr_active(old_state);
@@ -9696,8 +9697,11 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
 		 * We also need vupdate irq for the actual core vblank handling
 		 * at end of vblank.
 		 */
-		WARN_ON(amdgpu_dm_crtc_set_vupdate_irq(new_state->base.crtc, true) != 0);
-		WARN_ON(drm_crtc_vblank_get(new_state->base.crtc) != 0);
+		scoped_guard(mutex, &dm->dc_lock) {
+			dc_exit_ips_for_hw_access(dm->dc);
+			WARN_ON(amdgpu_dm_crtc_set_vupdate_irq(new_state->base.crtc, true) != 0);
+			WARN_ON(drm_crtc_vblank_get(new_state->base.crtc) != 0);
+		}
 		drm_dbg_driver(new_state->base.crtc->dev, "%s: crtc=%u VRR off->on: Get vblank ref\n",
 				 __func__, new_state->base.crtc->base.id);
 	} else if (old_vrr_active && !new_vrr_active) {
@@ -10122,7 +10126,11 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 		 */
 		if (acrtc_attach->base.state->event &&
 		    acrtc_state->active_planes > 0) {
-			drm_crtc_vblank_get(pcrtc);
+
+			scoped_guard(mutex, &dm->dc_lock) {
+				dc_exit_ips_for_hw_access(dm->dc);
+				drm_crtc_vblank_get(pcrtc);
+			}
 
 			spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
 
@@ -10138,13 +10146,19 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 					&acrtc_state->stream->vrr_infopacket;
 		}
 	} else if (cursor_update && acrtc_state->active_planes > 0) {
-		spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
-		if (acrtc_attach->base.state->event) {
-			drm_crtc_vblank_get(pcrtc);
-			acrtc_attach->event = acrtc_attach->base.state->event;
-			acrtc_attach->base.state->event = NULL;
+
+		scoped_guard(mutex, &dm->dc_lock) {
+			dc_exit_ips_for_hw_access(dm->dc);
+
+			spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
+			if (acrtc_attach->base.state->event) {
+				drm_crtc_vblank_get(pcrtc);
+				acrtc_attach->event =
+					acrtc_attach->base.state->event;
+				acrtc_attach->base.state->event = NULL;
+			}
+			spin_unlock_irqrestore(&pcrtc->dev->event_lock, flags);
 		}
-		spin_unlock_irqrestore(&pcrtc->dev->event_lock, flags);
 	}
 
 	/* Update the planes if changed or disable if we don't have any. */
@@ -10976,7 +10990,8 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 			manage_dm_interrupts(adev, acrtc, dm_new_crtc_state);
 		}
 		/* Handle vrr on->off / off->on transitions */
-		amdgpu_dm_handle_vrr_transition(dm_old_crtc_state, dm_new_crtc_state);
+		amdgpu_dm_handle_vrr_transition(dm, dm_old_crtc_state,
+						dm_new_crtc_state);
 
 #ifdef CONFIG_DEBUG_FS
 		if (new_crtc_state->active &&
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 bd0403005f370..b2fbdaa7c5c9c 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -585,6 +585,15 @@ struct amdgpu_display_manager {
 	 */
 	uint32_t active_vblank_irq_count;
 
+	/**
+	 * @prepare_vbl_disallow_idle:
+	 *
+	 * Set to true when idle has been disallowed. Set to false when vblank
+	 * interrupts have been enabled. i.e. idle re-allow on vblank disable is
+	 * blocked if this is true.
+	 */
+	bool prepare_vbl_disallow_idle;
+
 #if defined(CONFIG_DRM_AMD_SECURE_DISPLAY)
 	/**
 	 * @secure_display_ctx:
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
index e20aa74380665..2d2eda804735a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
@@ -547,10 +547,8 @@ int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
 	struct drm_crtc_commit *commit;
 	struct dm_crtc_state *crtc_state;
 	struct drm_device *drm_dev = crtc->dev;
-#if defined(CONFIG_DRM_AMD_SECURE_DISPLAY)
 	struct amdgpu_device *adev = drm_to_adev(drm_dev);
 	struct amdgpu_display_manager *dm = &adev->dm;
-#endif
 	struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
 	struct drm_dp_aux *aux = NULL;
 	bool enable = false;
@@ -656,7 +654,11 @@ int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
 	 */
 	enabled = amdgpu_dm_is_valid_crc_source(cur_crc_src);
 	if (!enabled && enable) {
-		ret = drm_crtc_vblank_get(crtc);
+		scoped_guard(mutex, &dm->dc_lock) {
+			dc_exit_ips_for_hw_access(dm->dc);
+			ret = drm_crtc_vblank_get(crtc);
+		}
+
 		if (ret)
 			goto cleanup;
 	}
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 697e232acebfb..5edc035ec152a 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
@@ -258,8 +258,8 @@ static void amdgpu_dm_crtc_vblank_control_worker(struct work_struct *work)
 	else if (dm->active_vblank_irq_count)
 		dm->active_vblank_irq_count--;
 
-	if (dm->active_vblank_irq_count > 0)
-		dc_allow_idle_optimizations(dm->dc, false);
+	/* prepare_vblank_enable must disallow idle first */
+	ASSERT(dm->dc->idle_optimizations_allowed == false);
 
 	/*
 	 * Control PSR based on vblank requirements from OS
@@ -277,7 +277,13 @@ static void amdgpu_dm_crtc_vblank_control_worker(struct work_struct *work)
 			vblank_work->acrtc->dm_irq_params.allow_sr_entry);
 	}
 
-	if (dm->active_vblank_irq_count == 0) {
+	/*
+	 * If this worker runs disable between prepare_vblank and enable_vblank,
+	 * we need to block idle re-allow. Leave it to the next vblank disable
+	 * to re-allow idle.
+	 */
+	if (dm->active_vblank_irq_count == 0 &&
+	    !READ_ONCE(dm->prepare_vbl_disallow_idle)) {
 		dc_post_update_surfaces_to_stream(dm->dc);
 
 		r = amdgpu_dpm_pause_power_profile(adev, true);
@@ -308,6 +314,8 @@ static inline int amdgpu_dm_crtc_set_vblank(struct drm_crtc *crtc, bool enable)
 	int irq_type;
 	int rc = 0;
 
+	ASSERT(dm->dc->idle_optimizations_allowed == false);
+
 	if (enable && !acrtc->base.enabled) {
 		drm_dbg_vbl(crtc->dev,
 				"Reject vblank enable on unconfigured CRTC %d (enabled=%d)\n",
@@ -399,6 +407,9 @@ static inline int amdgpu_dm_crtc_set_vblank(struct drm_crtc *crtc, bool enable)
 	}
 #endif
 
+	/* Ensure compiler emits the write before worker is queued */
+	WRITE_ONCE(dm->prepare_vbl_disallow_idle, false);
+
 	if (amdgpu_in_reset(adev))
 		return 0;
 
@@ -423,6 +434,26 @@ static inline int amdgpu_dm_crtc_set_vblank(struct drm_crtc *crtc, bool enable)
 	return 0;
 }
 
+static int amdgpu_prepare_enable_vblank(struct drm_crtc *crtc)
+{
+	struct amdgpu_device *adev = drm_to_adev(crtc->dev);
+	struct amdgpu_display_manager *dm = &adev->dm;
+
+	guard(mutex)(&adev->dm.dc_lock);
+
+	if (dm->dc->idle_optimizations_allowed) {
+		/*
+		 * Prevent the disable worker from re-allowing idle until
+		 * interrupts are enabled. Ensure compiler emits the write
+		 * before disallowing idle.
+		 */
+		WRITE_ONCE(dm->prepare_vbl_disallow_idle, true);
+		dc_exit_ips_for_hw_access(dm->dc);
+	}
+
+	return 0;
+}
+
 int amdgpu_dm_crtc_enable_vblank(struct drm_crtc *crtc)
 {
 	return amdgpu_dm_crtc_set_vblank(crtc, true);
@@ -590,6 +621,7 @@ static const struct drm_crtc_funcs amdgpu_dm_crtc_funcs = {
 	.verify_crc_source = amdgpu_dm_crtc_verify_crc_source,
 	.get_crc_sources = amdgpu_dm_crtc_get_crc_sources,
 	.get_vblank_counter = amdgpu_get_vblank_counter_kms,
+	.prepare_enable_vblank = amdgpu_prepare_enable_vblank,
 	.enable_vblank = amdgpu_dm_crtc_enable_vblank,
 	.disable_vblank = amdgpu_dm_crtc_disable_vblank,
 	.get_vblank_timestamp = drm_crtc_vblank_helper_get_vblank_timestamp,
-- 
2.52.0

Re: [PATCH v4 2/2] drm/amd/display: Implement prepare_vblank_enable callback

[Mario Limonciello]

On 1/27/26 1:41 PM, sunpeng.li@amd.com wrote:
> From: Leo Li <sunpeng.li@amd.com>
> 
> [Why]
> 
> APU DCN generations since DCN3.5 have the capability to power down
> almost all of the DCN hw block during idle periods. This is referred to
> as  IPS -- idle power states. In combination with a panel remote-buffer
> feature (like PSR or Panel Replay), IPS can save additional power.
> 
> Once DCN is in an IPS, no register access can occur. This includes
> control registers for vblank interrupts; IPS must first be exited.
> 
> Transitioning in or out of IPS requires synchronization with the rest of
> DC, as it powers up or down DCN, and may communicate with other MCUs on
> the SOC to do so. This is done via the dc_lock mutex.
> 
> While calling enable_vblank, the DRM vblank core holds spinlocks that
> prevent blocking operations. Yet acquiring the dc_lock mutex is
> blocking. Thus, IPS can not be exited piror to programming vblank
> interrupt registers from within enable_vblank. At least not in a
> race-free way.
> 
> Prior to this change, amdgpu_dm was exiting IPS(*) without holding the
> dc_lock, opening the door for races:
> https://gitlab.freedesktop.org/drm/amd/-/issues/5233
> 
> (*) From touching the interrupt registers. All register reads today have
> an implicit IPS exit, see dm_read_reg_func()
> 
> To solve this, the prepare_vblank_enable callback can be implemented to
> exit IPS, as it is called from process context.
> 
> [How]
> 
> Implement the prepare_vblank_enable callback for amdgpu_dm. In it,
> the dc_lock mutex is acquired, and IPS is exited.
> 
> Note that the only place that should unconditionally IPS allow is the
> vblank disable path. All other paths shall check whether IPS was
> previously allowed. If so, they can re-allow after all programming is
> complete. They also need to hold the dc_lock for the duration of the IPS
> disallow to re-allow. (This is not the for all of amdgpu_dm today,
> cleanup will come in future patches.)
> 
> v2: Add missing semicolon, add docstring for prepare_vbl_disallow_idle
> v3: Do prepare work (IPS exit) directly, instead of routing through DRM
> v4: Fix build error on CONFIG_DRM_AMD_SECURE_DISPLAY not set
> 
> Signed-off-by: Leo Li <sunpeng.li@amd.com>
> ---
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 37 ++++++++++++------
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  9 +++++
>   .../drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c |  8 ++--
>   .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c    | 38 +++++++++++++++++--
>   4 files changed, 75 insertions(+), 17 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 740711ac1037c..d0c412260be0c 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -9681,7 +9681,8 @@ static void update_stream_irq_parameters(
>   	spin_unlock_irqrestore(&adev_to_drm(adev)->event_lock, flags);
>   }
>   
> -static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
> +static void amdgpu_dm_handle_vrr_transition(struct amdgpu_display_manager *dm,
> +					    struct dm_crtc_state *old_state,
>   					    struct dm_crtc_state *new_state)
>   {
>   	bool old_vrr_active = amdgpu_dm_crtc_vrr_active(old_state);
> @@ -9696,8 +9697,11 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
>   		 * We also need vupdate irq for the actual core vblank handling
>   		 * at end of vblank.
>   		 */
> -		WARN_ON(amdgpu_dm_crtc_set_vupdate_irq(new_state->base.crtc, true) != 0);
> -		WARN_ON(drm_crtc_vblank_get(new_state->base.crtc) != 0);
> +		scoped_guard(mutex, &dm->dc_lock) {
> +			dc_exit_ips_for_hw_access(dm->dc);
> +			WARN_ON(amdgpu_dm_crtc_set_vupdate_irq(new_state->base.crtc, true) != 0);
> +			WARN_ON(drm_crtc_vblank_get(new_state->base.crtc) != 0);
> +		}
>   		drm_dbg_driver(new_state->base.crtc->dev, "%s: crtc=%u VRR off->on: Get vblank ref\n",
>   				 __func__, new_state->base.crtc->base.id);
>   	} else if (old_vrr_active && !new_vrr_active) {
> @@ -10122,7 +10126,11 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>   		 */
>   		if (acrtc_attach->base.state->event &&
>   		    acrtc_state->active_planes > 0) {
> -			drm_crtc_vblank_get(pcrtc);
> +
> +			scoped_guard(mutex, &dm->dc_lock) {
> +				dc_exit_ips_for_hw_access(dm->dc);
> +				drm_crtc_vblank_get(pcrtc);

drm_crtc_vblank_get() can potentially fail, should you be checking that 
here and showing a warning?

> +			}
>   
>   			spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
>   
> @@ -10138,13 +10146,19 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>   					&acrtc_state->stream->vrr_infopacket;
>   		}
>   	} else if (cursor_update && acrtc_state->active_planes > 0) {
> -		spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
> -		if (acrtc_attach->base.state->event) {
> -			drm_crtc_vblank_get(pcrtc);
> -			acrtc_attach->event = acrtc_attach->base.state->event;
> -			acrtc_attach->base.state->event = NULL;
> +
> +		scoped_guard(mutex, &dm->dc_lock) {
> +			dc_exit_ips_for_hw_access(dm->dc);
> +
> +			spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
> +			if (acrtc_attach->base.state->event) {
> +				drm_crtc_vblank_get(pcrtc);
> +				acrtc_attach->event =
> +					acrtc_attach->base.state->event;
> +				acrtc_attach->base.state->event = NULL;
> +			}
> +			spin_unlock_irqrestore(&pcrtc->dev->event_lock, flags);
>   		}
> -		spin_unlock_irqrestore(&pcrtc->dev->event_lock, flags);
>   	}
>   
>   	/* Update the planes if changed or disable if we don't have any. */
> @@ -10976,7 +10990,8 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>   			manage_dm_interrupts(adev, acrtc, dm_new_crtc_state);
>   		}
>   		/* Handle vrr on->off / off->on transitions */
> -		amdgpu_dm_handle_vrr_transition(dm_old_crtc_state, dm_new_crtc_state);
> +		amdgpu_dm_handle_vrr_transition(dm, dm_old_crtc_state,
> +						dm_new_crtc_state);
>   
>   #ifdef CONFIG_DEBUG_FS
>   		if (new_crtc_state->active &&
> 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 bd0403005f370..b2fbdaa7c5c9c 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -585,6 +585,15 @@ struct amdgpu_display_manager {
>   	 */
>   	uint32_t active_vblank_irq_count;
>   
> +	/**
> +	 * @prepare_vbl_disallow_idle:
> +	 *
> +	 * Set to true when idle has been disallowed. Set to false when vblank
> +	 * interrupts have been enabled. i.e. idle re-allow on vblank disable is
> +	 * blocked if this is true.
> +	 */
> +	bool prepare_vbl_disallow_idle;
> +
>   #if defined(CONFIG_DRM_AMD_SECURE_DISPLAY)
>   	/**
>   	 * @secure_display_ctx:
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
> index e20aa74380665..2d2eda804735a 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
> @@ -547,10 +547,8 @@ int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
>   	struct drm_crtc_commit *commit;
>   	struct dm_crtc_state *crtc_state;
>   	struct drm_device *drm_dev = crtc->dev;
> -#if defined(CONFIG_DRM_AMD_SECURE_DISPLAY)
>   	struct amdgpu_device *adev = drm_to_adev(drm_dev);
>   	struct amdgpu_display_manager *dm = &adev->dm;
> -#endif
>   	struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>   	struct drm_dp_aux *aux = NULL;
>   	bool enable = false;
> @@ -656,7 +654,11 @@ int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
>   	 */
>   	enabled = amdgpu_dm_is_valid_crc_source(cur_crc_src);
>   	if (!enabled && enable) {
> -		ret = drm_crtc_vblank_get(crtc);
> +		scoped_guard(mutex, &dm->dc_lock) {
> +			dc_exit_ips_for_hw_access(dm->dc);
> +			ret = drm_crtc_vblank_get(crtc);
> +		}
> +
>   		if (ret)
>   			goto cleanup;
>   	}
> 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 697e232acebfb..5edc035ec152a 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
> @@ -258,8 +258,8 @@ static void amdgpu_dm_crtc_vblank_control_worker(struct work_struct *work)
>   	else if (dm->active_vblank_irq_count)
>   		dm->active_vblank_irq_count--;
>   
> -	if (dm->active_vblank_irq_count > 0)
> -		dc_allow_idle_optimizations(dm->dc, false);
> +	/* prepare_vblank_enable must disallow idle first */
> +	ASSERT(dm->dc->idle_optimizations_allowed == false);
>   
>   	/*
>   	 * Control PSR based on vblank requirements from OS
> @@ -277,7 +277,13 @@ static void amdgpu_dm_crtc_vblank_control_worker(struct work_struct *work)
>   			vblank_work->acrtc->dm_irq_params.allow_sr_entry);
>   	}
>   
> -	if (dm->active_vblank_irq_count == 0) {
> +	/*
> +	 * If this worker runs disable between prepare_vblank and enable_vblank,
> +	 * we need to block idle re-allow. Leave it to the next vblank disable
> +	 * to re-allow idle.
> +	 */
> +	if (dm->active_vblank_irq_count == 0 &&
> +	    !READ_ONCE(dm->prepare_vbl_disallow_idle)) {
>   		dc_post_update_surfaces_to_stream(dm->dc);
>   
>   		r = amdgpu_dpm_pause_power_profile(adev, true);
> @@ -308,6 +314,8 @@ static inline int amdgpu_dm_crtc_set_vblank(struct drm_crtc *crtc, bool enable)
>   	int irq_type;
>   	int rc = 0;
>   
> +	ASSERT(dm->dc->idle_optimizations_allowed == false);
> +
>   	if (enable && !acrtc->base.enabled) {
>   		drm_dbg_vbl(crtc->dev,
>   				"Reject vblank enable on unconfigured CRTC %d (enabled=%d)\n",
> @@ -399,6 +407,9 @@ static inline int amdgpu_dm_crtc_set_vblank(struct drm_crtc *crtc, bool enable)
>   	}
>   #endif
>   
> +	/* Ensure compiler emits the write before worker is queued */
> +	WRITE_ONCE(dm->prepare_vbl_disallow_idle, false);
> +
>   	if (amdgpu_in_reset(adev))
>   		return 0;
>   
> @@ -423,6 +434,26 @@ static inline int amdgpu_dm_crtc_set_vblank(struct drm_crtc *crtc, bool enable)
>   	return 0;
>   }
>   
> +static int amdgpu_prepare_enable_vblank(struct drm_crtc *crtc)
> +{
> +	struct amdgpu_device *adev = drm_to_adev(crtc->dev);
> +	struct amdgpu_display_manager *dm = &adev->dm;
> +
> +	guard(mutex)(&adev->dm.dc_lock);
> +
> +	if (dm->dc->idle_optimizations_allowed) {
> +		/*
> +		 * Prevent the disable worker from re-allowing idle until
> +		 * interrupts are enabled. Ensure compiler emits the write
> +		 * before disallowing idle.
> +		 */
> +		WRITE_ONCE(dm->prepare_vbl_disallow_idle, true);
> +		dc_exit_ips_for_hw_access(dm->dc);
> +	}
> +
> +	return 0;
> +}
> +
>   int amdgpu_dm_crtc_enable_vblank(struct drm_crtc *crtc)
>   {
>   	return amdgpu_dm_crtc_set_vblank(crtc, true);
> @@ -590,6 +621,7 @@ static const struct drm_crtc_funcs amdgpu_dm_crtc_funcs = {
>   	.verify_crc_source = amdgpu_dm_crtc_verify_crc_source,
>   	.get_crc_sources = amdgpu_dm_crtc_get_crc_sources,
>   	.get_vblank_counter = amdgpu_get_vblank_counter_kms,
> +	.prepare_enable_vblank = amdgpu_prepare_enable_vblank,
>   	.enable_vblank = amdgpu_dm_crtc_enable_vblank,
>   	.disable_vblank = amdgpu_dm_crtc_disable_vblank,
>   	.get_vblank_timestamp = drm_crtc_vblank_helper_get_vblank_timestamp,