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

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

[sunpeng.li]

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

Some drivers need to perform blocking 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 via a mutex,
for example.

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

In DRM core, call this helper before drm_vblank_get(). Drivers can
choose to call this if they implement the callback in the future.

Signed-off-by: Leo Li <sunpeng.li@amd.com>
---
 drivers/gpu/drm/drm_atomic_helper.c |  8 ++++
 drivers/gpu/drm/drm_fb_helper.c     |  4 ++
 drivers/gpu/drm/drm_plane.c         |  4 ++
 drivers/gpu/drm/drm_vblank.c        | 69 +++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_vblank_work.c   |  8 ++++
 include/drm/drm_crtc.h              | 27 +++++++++++
 include/drm/drm_vblank.h            |  1 +
 7 files changed, 121 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index ef56b474acf59..e52dd41f83117 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1264,6 +1264,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
@@ -1815,6 +1819,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_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 11a5b60cb9ce4..7400942fd7d1d 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1103,6 +1103,10 @@ int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd,
 		 * enabled, otherwise just don't do anythintg,
 		 * not even report an error.
 		 */
+		ret = drm_crtc_vblank_prepare(crtc);
+		if (ret)
+			break;
+
 		ret = drm_crtc_vblank_get(crtc);
 		if (!ret) {
 			drm_crtc_wait_one_vblank(crtc);
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 38f82391bfda5..f2e40eaa385e6 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -1421,6 +1421,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 46f59883183d9..4dac3228c021f 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -1194,6 +1194,30 @@ 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 run blocking operations to prepare for enabling
+ * vblank interrupts. This function calls the prepare_enable_vblank callback, if
+ * available, to allow drivers to do that.
+ *
+ * The spin-up may call blocking functions, such as mutex_lock(). Therefore,
+ * this must be called from process context, where sleeping is allowed.
+ *
+ * Also see &drm_crtc_funcs.prepare_enable_vblank.
+ *
+ * Returns: Zero on success or a negative error code on failure.
+ */
+int drm_crtc_vblank_prepare(struct drm_crtc *crtc)
+{
+	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);
@@ -1288,12 +1312,22 @@ EXPORT_SYMBOL(drm_crtc_vblank_put);
 void drm_wait_one_vblank(struct drm_device *dev, unsigned int pipe)
 {
 	struct drm_vblank_crtc *vblank = drm_vblank_crtc(dev, pipe);
+	struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
 	int ret;
 	u64 last;
 
 	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
 		return;
 
+	crtc = drm_crtc_from_index(dev, pipe);
+	if (crtc) {
+		ret = drm_crtc_vblank_prepare(crtc);
+		if (drm_WARN(dev, ret,
+			     "prepare vblank failed on crtc %i, ret=%i\n",
+			     pipe, ret))
+			return;
+	}
+
 	ret = drm_vblank_get(dev, pipe);
 	if (drm_WARN(dev, ret, "vblank not available on crtc %i, ret=%i\n",
 		     pipe, ret))
@@ -1485,10 +1519,18 @@ void drm_crtc_vblank_on_config(struct drm_crtc *crtc,
 	struct drm_device *dev = crtc->dev;
 	unsigned int pipe = drm_crtc_index(crtc);
 	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
+	int ret;
 
 	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
 		return;
 
+	if (crtc) {
+		ret = drm_crtc_vblank_prepare(crtc);
+		drm_WARN_ON(dev, ret);
+		if (ret)
+			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 +1838,17 @@ 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) {
+			drm_dbg_core(dev,
+				     "prepare vblank failed on crtc %i, ret=%i\n",
+				     pipe, ret);
+			return ret;
+		}
+	}
+
 	ret = drm_vblank_get(dev, pipe);
 	if (ret) {
 		drm_dbg_core(dev,
@@ -2031,6 +2084,14 @@ 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) {
+			drm_dbg_core(dev,
+				     "prepare vblank failed on crtc %i, ret=%i\n",
+				     pipe, ret);
+			return ret;
+		}
+
 		ret = drm_crtc_vblank_get(crtc);
 		if (ret) {
 			drm_dbg_core(dev,
@@ -2098,6 +2159,14 @@ 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) {
+		drm_dbg_core(dev,
+			     "prepare vblank failed on crtc %i, ret=%i\n",
+			     pipe, ret);
+		return ret;
+	}
+
 	ret = drm_crtc_vblank_get(crtc);
 	if (ret) {
 		drm_dbg_core(dev,
diff --git a/drivers/gpu/drm/drm_vblank_work.c b/drivers/gpu/drm/drm_vblank_work.c
index e4e1873f0e1e1..582ee7fd94adf 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 caa56e039da2a..456cf9ba0143a 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -860,6 +860,33 @@ struct drm_crtc_funcs {
 	 */
 	u32 (*get_vblank_counter)(struct drm_crtc *crtc);
 
+	/**
+	 * @prepare_enable_vblank:
+	 *
+	 * An optional callback for preparing to enable vblank interrupts. It
+	 * allows drivers to perform blocking operations, and thus is called
+	 * without any vblank spinlocks. Consequently, this callback is not
+	 * synchronized with the rest of drm_vblank management; drivers are
+	 * responsible for ensuring it won't race with drm_vblank and it's other
+	 * driver callbacks.
+	 *
+	 * For example, drivers may use this to spin-up hardware from a low
+	 * power state -- which may require blocking operations -- such that
+	 * hardware registers are available to read/write. However, the driver
+	 * must be careful as to when to reenter low-power state, such that it
+	 * won't race with enable_vblank.
+	 *
+	 * It is called unconditionally, regardless of whether vblank interrupts
+	 * are already enabled or not.
+	 *
+	 * 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:
 	 *
diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
index 151ab1e85b1b7..5abc367aa4376 100644
--- a/include/drm/drm_vblank.h
+++ b/include/drm/drm_vblank.h
@@ -272,6 +272,7 @@ void drm_vblank_set_event(struct drm_pending_vblank_event *e,
 			  ktime_t *now);
 bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe);
 bool drm_crtc_handle_vblank(struct drm_crtc *crtc);
+int drm_crtc_vblank_prepare(struct drm_crtc *crtc);
 int drm_crtc_vblank_get(struct drm_crtc *crtc);
 void drm_crtc_vblank_put(struct drm_crtc *crtc);
 void drm_wait_one_vblank(struct drm_device *dev, unsigned int pipe);
-- 
2.51.0

[PATCH v2 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.

v2: Add missing semicolon, add docstring for prepare_vbl_disallow_idle

Signed-off-by: Leo Li <sunpeng.li@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  3 ++
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  9 +++++
 .../drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c |  4 +++
 .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c    | 36 +++++++++++++++++--
 4 files changed, 49 insertions(+), 3 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 0346052f2e574..842a93e2d6ce0 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -9682,6 +9682,7 @@ 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(drm_crtc_vblank_prepare(new_state->base.crtc) != 0);
 		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",
@@ -10108,6 +10109,7 @@ 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_prepare(pcrtc);
 			drm_crtc_vblank_get(pcrtc);
 
 			spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
@@ -10124,6 +10126,7 @@ 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) {
+		drm_crtc_vblank_prepare(pcrtc);
 		spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
 		if (acrtc_attach->base.state->event) {
 			drm_crtc_vblank_get(pcrtc);
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 7065b20aa2e6b..a99612fb3467a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -587,6 +587,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..7839b56859391 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
@@ -656,6 +656,10 @@ 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_prepare(crtc);
+		if (ret)
+			goto cleanup;
+
 		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 38f9ea313dcbb..dd693419111db 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,24 @@ 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_allow_idle_optimizations(dm->dc, false);
+	}
+
+	return 0;
+}
+
 int amdgpu_dm_crtc_enable_vblank(struct drm_crtc *crtc)
 {
 	return amdgpu_dm_crtc_set_vblank(crtc, true);
@@ -590,6 +619,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.51.0

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

[kernel test robot]

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20251201231807.287414-1-sunpeng.li@amd.com>
References: <20251201231807.287414-1-sunpeng.li@amd.com>
TO: sunpeng.li@amd.com
TO: amd-gfx@lists.freedesktop.org
TO: dri-devel@lists.freedesktop.org
CC: Harry.Wentland@amd.com
CC: Nicholas.Kazlauskas@amd.com
CC: simona@ffwll.ch
CC: airlied@gmail.com
CC: Leo Li <sunpeng.li@amd.com>

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-i915/for-linux-next-fixes]
[also build test WARNING on daeinki-drm-exynos/exynos-drm-next linus/master v6.18]
[cannot apply to drm-misc/drm-misc-next drm/drm-next drm-i915/for-linux-next drm-tip/drm-tip next-20251203]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/sunpeng-li-amd-com/drm-amd-display-Implement-prepare_vblank_enable-callback/20251202-072501
base:   https://gitlab.freedesktop.org/drm/i915/kernel.git for-linux-next-fixes
patch link:    https://lore.kernel.org/r/20251201231807.287414-1-sunpeng.li%40amd.com
patch subject: [PATCH v2 1/2] drm: Introduce drm_crtc_vblank_prepare()
:::::: branch date: 2 days ago
:::::: commit date: 2 days ago
config: nios2-randconfig-r071-20251204 (https://download.01.org/0day-ci/archive/20251204/202512041153.nYks4oYu-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 8.5.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202512041153.nYks4oYu-lkp@intel.com/

smatch warnings:
drivers/gpu/drm/drm_vblank.c:1527 drm_crtc_vblank_on_config() warn: variable dereferenced before check 'crtc' (see line 1519)

vim +/crtc +1527 drivers/gpu/drm/drm_vblank.c

ed20151a7699bb2 Ville Syrjälä 2018-11-27  1502  
3ed4351a83ca05d Simona Vetter 2017-05-31  1503  /**
0d5040e406d2c44 Hamza Mahfooz 2024-07-25  1504   * drm_crtc_vblank_on_config - enable vblank events on a CRTC with custom
0d5040e406d2c44 Hamza Mahfooz 2024-07-25  1505   *     configuration options
3ed4351a83ca05d Simona Vetter 2017-05-31  1506   * @crtc: CRTC in question
0d5040e406d2c44 Hamza Mahfooz 2024-07-25  1507   * @config: Vblank configuration value
3ed4351a83ca05d Simona Vetter 2017-05-31  1508   *
0d5040e406d2c44 Hamza Mahfooz 2024-07-25  1509   * See drm_crtc_vblank_on(). In addition, this function allows you to provide a
0d5040e406d2c44 Hamza Mahfooz 2024-07-25  1510   * custom vblank configuration for a given CRTC.
0d5040e406d2c44 Hamza Mahfooz 2024-07-25  1511   *
0d5040e406d2c44 Hamza Mahfooz 2024-07-25  1512   * Note that @config is copied, the pointer does not need to stay valid beyond
0d5040e406d2c44 Hamza Mahfooz 2024-07-25  1513   * this function call. For details of the parameters see
0d5040e406d2c44 Hamza Mahfooz 2024-07-25  1514   * struct drm_vblank_crtc_config.
3ed4351a83ca05d Simona Vetter 2017-05-31  1515   */
0d5040e406d2c44 Hamza Mahfooz 2024-07-25  1516  void drm_crtc_vblank_on_config(struct drm_crtc *crtc,
0d5040e406d2c44 Hamza Mahfooz 2024-07-25  1517  			       const struct drm_vblank_crtc_config *config)
3ed4351a83ca05d Simona Vetter 2017-05-31  1518  {
3ed4351a83ca05d Simona Vetter 2017-05-31 @1519  	struct drm_device *dev = crtc->dev;
3ed4351a83ca05d Simona Vetter 2017-05-31  1520  	unsigned int pipe = drm_crtc_index(crtc);
d12e36494dc2bf2 Ville Syrjälä 2024-04-08  1521  	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
38bd1e412d0aa4b Leo Li        2025-12-01  1522  	int ret;
3ed4351a83ca05d Simona Vetter 2017-05-31  1523  
5a4784f49b2dcff Sam Ravnborg  2020-05-23  1524  	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
3ed4351a83ca05d Simona Vetter 2017-05-31  1525  		return;
3ed4351a83ca05d Simona Vetter 2017-05-31  1526  
38bd1e412d0aa4b Leo Li        2025-12-01 @1527  	if (crtc) {
38bd1e412d0aa4b Leo Li        2025-12-01  1528  		ret = drm_crtc_vblank_prepare(crtc);
38bd1e412d0aa4b Leo Li        2025-12-01  1529  		drm_WARN_ON(dev, ret);
38bd1e412d0aa4b Leo Li        2025-12-01  1530  		if (ret)
38bd1e412d0aa4b Leo Li        2025-12-01  1531  			return;
38bd1e412d0aa4b Leo Li        2025-12-01  1532  	}
38bd1e412d0aa4b Leo Li        2025-12-01  1533  
92cc68e35863c1c Lyude Paul    2020-07-20  1534  	spin_lock_irq(&dev->vbl_lock);
02149a76d32bd8f Sam Ravnborg  2020-05-23  1535  	drm_dbg_vbl(dev, "crtc %d, vblank enabled %d, inmodeset %d\n",
3ed4351a83ca05d Simona Vetter 2017-05-31  1536  		    pipe, vblank->enabled, vblank->inmodeset);
3ed4351a83ca05d Simona Vetter 2017-05-31  1537  
0d5040e406d2c44 Hamza Mahfooz 2024-07-25  1538  	vblank->config = *config;
0d5040e406d2c44 Hamza Mahfooz 2024-07-25  1539  
3ed4351a83ca05d Simona Vetter 2017-05-31  1540  	/* Drop our private "prevent drm_vblank_get" refcount */
3ed4351a83ca05d Simona Vetter 2017-05-31  1541  	if (vblank->inmodeset) {
3ed4351a83ca05d Simona Vetter 2017-05-31  1542  		atomic_dec(&vblank->refcount);
3ed4351a83ca05d Simona Vetter 2017-05-31  1543  		vblank->inmodeset = 0;
3ed4351a83ca05d Simona Vetter 2017-05-31  1544  	}
3ed4351a83ca05d Simona Vetter 2017-05-31  1545  
3ed4351a83ca05d Simona Vetter 2017-05-31  1546  	drm_reset_vblank_timestamp(dev, pipe);
3ed4351a83ca05d Simona Vetter 2017-05-31  1547  
3ed4351a83ca05d Simona Vetter 2017-05-31  1548  	/*
3ed4351a83ca05d Simona Vetter 2017-05-31  1549  	 * re-enable interrupts if there are users left, or the
3ed4351a83ca05d Simona Vetter 2017-05-31  1550  	 * user wishes vblank interrupts to be enabled all the time.
3ed4351a83ca05d Simona Vetter 2017-05-31  1551  	 */
0d5040e406d2c44 Hamza Mahfooz 2024-07-25  1552  	if (atomic_read(&vblank->refcount) != 0 || !vblank->config.offdelay_ms)
5a4784f49b2dcff Sam Ravnborg  2020-05-23  1553  		drm_WARN_ON(dev, drm_vblank_enable(dev, pipe));
92cc68e35863c1c Lyude Paul    2020-07-20  1554  	spin_unlock_irq(&dev->vbl_lock);
3ed4351a83ca05d Simona Vetter 2017-05-31  1555  }
0d5040e406d2c44 Hamza Mahfooz 2024-07-25  1556  EXPORT_SYMBOL(drm_crtc_vblank_on_config);
0d5040e406d2c44 Hamza Mahfooz 2024-07-25  1557  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Dan Carpenter]

Hi,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/sunpeng-li-amd-com/drm-amd-display-Implement-prepare_vblank_enable-callback/20251202-072501
base:   https://gitlab.freedesktop.org/drm/i915/kernel.git for-linux-next-fixes
patch link:    https://lore.kernel.org/r/20251201231807.287414-1-sunpeng.li%40amd.com
patch subject: [PATCH v2 1/2] drm: Introduce drm_crtc_vblank_prepare()
config: nios2-randconfig-r071-20251204 (https://download.01.org/0day-ci/archive/20251204/202512041153.nYks4oYu-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 8.5.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202512041153.nYks4oYu-lkp@intel.com/

smatch warnings:
drivers/gpu/drm/drm_vblank.c:1527 drm_crtc_vblank_on_config() warn: variable dereferenced before check 'crtc' (see line 1519)

vim +/crtc +1527 drivers/gpu/drm/drm_vblank.c

0d5040e406d2c44 Hamza Mahfooz 2024-07-25  1516  void drm_crtc_vblank_on_config(struct drm_crtc *crtc,
0d5040e406d2c44 Hamza Mahfooz 2024-07-25  1517  			       const struct drm_vblank_crtc_config *config)
3ed4351a83ca05d Simona Vetter 2017-05-31  1518  {
3ed4351a83ca05d Simona Vetter 2017-05-31 @1519  	struct drm_device *dev = crtc->dev;
3ed4351a83ca05d Simona Vetter 2017-05-31  1520  	unsigned int pipe = drm_crtc_index(crtc);
                                                                                           ^^^^
Unchecked dereference.  I'm pretty certain crtc can't be NULL.

d12e36494dc2bf2 Ville Syrjälä 2024-04-08  1521  	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
38bd1e412d0aa4b Leo Li        2025-12-01  1522  	int ret;
3ed4351a83ca05d Simona Vetter 2017-05-31  1523  
5a4784f49b2dcff Sam Ravnborg  2020-05-23  1524  	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
3ed4351a83ca05d Simona Vetter 2017-05-31  1525  		return;
3ed4351a83ca05d Simona Vetter 2017-05-31  1526  
38bd1e412d0aa4b Leo Li        2025-12-01 @1527  	if (crtc) {
                                                        ^^^^^^^^^^^
So this NULL check is too late, and can be removed.

38bd1e412d0aa4b Leo Li        2025-12-01  1528  		ret = drm_crtc_vblank_prepare(crtc);
38bd1e412d0aa4b Leo Li        2025-12-01  1529  		drm_WARN_ON(dev, ret);
38bd1e412d0aa4b Leo Li        2025-12-01  1530  		if (ret)
38bd1e412d0aa4b Leo Li        2025-12-01  1531  			return;
38bd1e412d0aa4b Leo Li        2025-12-01  1532  	}
38bd1e412d0aa4b Leo Li        2025-12-01  1533  
92cc68e35863c1c Lyude Paul    2020-07-20  1534  	spin_lock_irq(&dev->vbl_lock);
02149a76d32bd8f Sam Ravnborg  2020-05-23  1535  	drm_dbg_vbl(dev, "crtc %d, vblank enabled %d, inmodeset %d\n",
3ed4351a83ca05d Simona Vetter 2017-05-31  1536  		    pipe, vblank->enabled, vblank->inmodeset);
3ed4351a83ca05d Simona Vetter 2017-05-31  1537  
0d5040e406d2c44 Hamza Mahfooz 2024-07-25  1538  	vblank->config = *config;
0d5040e406d2c44 Hamza Mahfooz 2024-07-25  1539  
3ed4351a83ca05d Simona Vetter 2017-05-31  1540  	/* Drop our private "prevent drm_vblank_get" refcount */
3ed4351a83ca05d Simona Vetter 2017-05-31  1541  	if (vblank->inmodeset) {
3ed4351a83ca05d Simona Vetter 2017-05-31  1542  		atomic_dec(&vblank->refcount);
3ed4351a83ca05d Simona Vetter 2017-05-31  1543  		vblank->inmodeset = 0;
3ed4351a83ca05d Simona Vetter 2017-05-31  1544  	}
3ed4351a83ca05d Simona Vetter 2017-05-31  1545  
3ed4351a83ca05d Simona Vetter 2017-05-31  1546  	drm_reset_vblank_timestamp(dev, pipe);
3ed4351a83ca05d Simona Vetter 2017-05-31  1547  
3ed4351a83ca05d Simona Vetter 2017-05-31  1548  	/*
3ed4351a83ca05d Simona Vetter 2017-05-31  1549  	 * re-enable interrupts if there are users left, or the
3ed4351a83ca05d Simona Vetter 2017-05-31  1550  	 * user wishes vblank interrupts to be enabled all the time.
3ed4351a83ca05d Simona Vetter 2017-05-31  1551  	 */
0d5040e406d2c44 Hamza Mahfooz 2024-07-25  1552  	if (atomic_read(&vblank->refcount) != 0 || !vblank->config.offdelay_ms)
5a4784f49b2dcff Sam Ravnborg  2020-05-23  1553  		drm_WARN_ON(dev, drm_vblank_enable(dev, pipe));
92cc68e35863c1c Lyude Paul    2020-07-20  1554  	spin_unlock_irq(&dev->vbl_lock);
3ed4351a83ca05d Simona Vetter 2017-05-31  1555  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20251201231807.287414-1-sunpeng.li@amd.com>
References: <20251201231807.287414-1-sunpeng.li@amd.com>
TO: sunpeng.li@amd.com
TO: amd-gfx@lists.freedesktop.org
TO: dri-devel@lists.freedesktop.org
CC: Harry.Wentland@amd.com
CC: Nicholas.Kazlauskas@amd.com
CC: simona@ffwll.ch
CC: airlied@gmail.com
CC: Leo Li <sunpeng.li@amd.com>

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-i915/for-linux-next-fixes]
[also build test WARNING on daeinki-drm-exynos/exynos-drm-next v6.18]
[cannot apply to drm-misc/drm-misc-next drm/drm-next drm-i915/for-linux-next drm-tip/drm-tip linus/master next-20251204]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/sunpeng-li-amd-com/drm-amd-display-Implement-prepare_vblank_enable-callback/20251202-072501
base:   https://gitlab.freedesktop.org/drm/i915/kernel.git for-linux-next-fixes
patch link:    https://lore.kernel.org/r/20251201231807.287414-1-sunpeng.li%40amd.com
patch subject: [PATCH v2 1/2] drm: Introduce drm_crtc_vblank_prepare()
:::::: branch date: 3 days ago
:::::: commit date: 3 days ago
config: nios2-randconfig-r071-20251204 (https://download.01.org/0day-ci/archive/20251205/202512051120.3HWHmkDI-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 8.5.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202512051120.3HWHmkDI-lkp@intel.com/

smatch warnings:
drivers/gpu/drm/drm_vblank.c:1527 drm_crtc_vblank_on_config() warn: variable dereferenced before check 'crtc' (see line 1519)

vim +/crtc +1527 drivers/gpu/drm/drm_vblank.c

ed20151a7699bb Ville Syrjälä 2018-11-27  1502  
3ed4351a83ca05 Simona Vetter 2017-05-31  1503  /**
0d5040e406d2c4 Hamza Mahfooz 2024-07-25  1504   * drm_crtc_vblank_on_config - enable vblank events on a CRTC with custom
0d5040e406d2c4 Hamza Mahfooz 2024-07-25  1505   *     configuration options
3ed4351a83ca05 Simona Vetter 2017-05-31  1506   * @crtc: CRTC in question
0d5040e406d2c4 Hamza Mahfooz 2024-07-25  1507   * @config: Vblank configuration value
3ed4351a83ca05 Simona Vetter 2017-05-31  1508   *
0d5040e406d2c4 Hamza Mahfooz 2024-07-25  1509   * See drm_crtc_vblank_on(). In addition, this function allows you to provide a
0d5040e406d2c4 Hamza Mahfooz 2024-07-25  1510   * custom vblank configuration for a given CRTC.
0d5040e406d2c4 Hamza Mahfooz 2024-07-25  1511   *
0d5040e406d2c4 Hamza Mahfooz 2024-07-25  1512   * Note that @config is copied, the pointer does not need to stay valid beyond
0d5040e406d2c4 Hamza Mahfooz 2024-07-25  1513   * this function call. For details of the parameters see
0d5040e406d2c4 Hamza Mahfooz 2024-07-25  1514   * struct drm_vblank_crtc_config.
3ed4351a83ca05 Simona Vetter 2017-05-31  1515   */
0d5040e406d2c4 Hamza Mahfooz 2024-07-25  1516  void drm_crtc_vblank_on_config(struct drm_crtc *crtc,
0d5040e406d2c4 Hamza Mahfooz 2024-07-25  1517  			       const struct drm_vblank_crtc_config *config)
3ed4351a83ca05 Simona Vetter 2017-05-31  1518  {
3ed4351a83ca05 Simona Vetter 2017-05-31 @1519  	struct drm_device *dev = crtc->dev;
3ed4351a83ca05 Simona Vetter 2017-05-31  1520  	unsigned int pipe = drm_crtc_index(crtc);
d12e36494dc2bf Ville Syrjälä 2024-04-08  1521  	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
38bd1e412d0aa4 Leo Li        2025-12-01  1522  	int ret;
3ed4351a83ca05 Simona Vetter 2017-05-31  1523  
5a4784f49b2dcf Sam Ravnborg  2020-05-23  1524  	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
3ed4351a83ca05 Simona Vetter 2017-05-31  1525  		return;
3ed4351a83ca05 Simona Vetter 2017-05-31  1526  
38bd1e412d0aa4 Leo Li        2025-12-01 @1527  	if (crtc) {
38bd1e412d0aa4 Leo Li        2025-12-01  1528  		ret = drm_crtc_vblank_prepare(crtc);
38bd1e412d0aa4 Leo Li        2025-12-01  1529  		drm_WARN_ON(dev, ret);
38bd1e412d0aa4 Leo Li        2025-12-01  1530  		if (ret)
38bd1e412d0aa4 Leo Li        2025-12-01  1531  			return;
38bd1e412d0aa4 Leo Li        2025-12-01  1532  	}
38bd1e412d0aa4 Leo Li        2025-12-01  1533  
92cc68e35863c1 Lyude Paul    2020-07-20  1534  	spin_lock_irq(&dev->vbl_lock);
02149a76d32bd8 Sam Ravnborg  2020-05-23  1535  	drm_dbg_vbl(dev, "crtc %d, vblank enabled %d, inmodeset %d\n",
3ed4351a83ca05 Simona Vetter 2017-05-31  1536  		    pipe, vblank->enabled, vblank->inmodeset);
3ed4351a83ca05 Simona Vetter 2017-05-31  1537  
0d5040e406d2c4 Hamza Mahfooz 2024-07-25  1538  	vblank->config = *config;
0d5040e406d2c4 Hamza Mahfooz 2024-07-25  1539  
3ed4351a83ca05 Simona Vetter 2017-05-31  1540  	/* Drop our private "prevent drm_vblank_get" refcount */
3ed4351a83ca05 Simona Vetter 2017-05-31  1541  	if (vblank->inmodeset) {
3ed4351a83ca05 Simona Vetter 2017-05-31  1542  		atomic_dec(&vblank->refcount);
3ed4351a83ca05 Simona Vetter 2017-05-31  1543  		vblank->inmodeset = 0;
3ed4351a83ca05 Simona Vetter 2017-05-31  1544  	}
3ed4351a83ca05 Simona Vetter 2017-05-31  1545  
3ed4351a83ca05 Simona Vetter 2017-05-31  1546  	drm_reset_vblank_timestamp(dev, pipe);
3ed4351a83ca05 Simona Vetter 2017-05-31  1547  
3ed4351a83ca05 Simona Vetter 2017-05-31  1548  	/*
3ed4351a83ca05 Simona Vetter 2017-05-31  1549  	 * re-enable interrupts if there are users left, or the
3ed4351a83ca05 Simona Vetter 2017-05-31  1550  	 * user wishes vblank interrupts to be enabled all the time.
3ed4351a83ca05 Simona Vetter 2017-05-31  1551  	 */
0d5040e406d2c4 Hamza Mahfooz 2024-07-25  1552  	if (atomic_read(&vblank->refcount) != 0 || !vblank->config.offdelay_ms)
5a4784f49b2dcf Sam Ravnborg  2020-05-23  1553  		drm_WARN_ON(dev, drm_vblank_enable(dev, pipe));
92cc68e35863c1 Lyude Paul    2020-07-20  1554  	spin_unlock_irq(&dev->vbl_lock);
3ed4351a83ca05 Simona Vetter 2017-05-31  1555  }
0d5040e406d2c4 Hamza Mahfooz 2024-07-25  1556  EXPORT_SYMBOL(drm_crtc_vblank_on_config);
0d5040e406d2c4 Hamza Mahfooz 2024-07-25  1557  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Harry Wentland]

On 2025-12-01 18:18, sunpeng.li@amd.com wrote:
> From: Leo Li <sunpeng.li@amd.com>
> 
> Some drivers need to perform blocking 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 via a mutex,
> for example.
> 
> To support this, introduce a new drm_crtc_vblank_prepare() helper that
> calls back into the driver -- if implemented -- for the driver to do
> such preparation work.
> 
> In DRM core, call this helper before drm_vblank_get(). Drivers can
> choose to call this if they implement the callback in the future.
> 
> Signed-off-by: Leo Li <sunpeng.li@amd.com>

Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Harry

> ---
>  drivers/gpu/drm/drm_atomic_helper.c |  8 ++++
>  drivers/gpu/drm/drm_fb_helper.c     |  4 ++
>  drivers/gpu/drm/drm_plane.c         |  4 ++
>  drivers/gpu/drm/drm_vblank.c        | 69 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_vblank_work.c   |  8 ++++
>  include/drm/drm_crtc.h              | 27 +++++++++++
>  include/drm/drm_vblank.h            |  1 +
>  7 files changed, 121 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index ef56b474acf59..e52dd41f83117 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1264,6 +1264,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
> @@ -1815,6 +1819,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_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 11a5b60cb9ce4..7400942fd7d1d 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1103,6 +1103,10 @@ int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd,
>  		 * enabled, otherwise just don't do anythintg,
>  		 * not even report an error.
>  		 */
> +		ret = drm_crtc_vblank_prepare(crtc);
> +		if (ret)
> +			break;
> +
>  		ret = drm_crtc_vblank_get(crtc);
>  		if (!ret) {
>  			drm_crtc_wait_one_vblank(crtc);
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 38f82391bfda5..f2e40eaa385e6 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1421,6 +1421,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 46f59883183d9..4dac3228c021f 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -1194,6 +1194,30 @@ 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 run blocking operations to prepare for enabling
> + * vblank interrupts. This function calls the prepare_enable_vblank callback, if
> + * available, to allow drivers to do that.
> + *
> + * The spin-up may call blocking functions, such as mutex_lock(). Therefore,
> + * this must be called from process context, where sleeping is allowed.
> + *
> + * Also see &drm_crtc_funcs.prepare_enable_vblank.
> + *
> + * Returns: Zero on success or a negative error code on failure.
> + */
> +int drm_crtc_vblank_prepare(struct drm_crtc *crtc)
> +{
> +	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);
> @@ -1288,12 +1312,22 @@ EXPORT_SYMBOL(drm_crtc_vblank_put);
>  void drm_wait_one_vblank(struct drm_device *dev, unsigned int pipe)
>  {
>  	struct drm_vblank_crtc *vblank = drm_vblank_crtc(dev, pipe);
> +	struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
>  	int ret;
>  	u64 last;
>  
>  	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
>  		return;
>  
> +	crtc = drm_crtc_from_index(dev, pipe);
> +	if (crtc) {
> +		ret = drm_crtc_vblank_prepare(crtc);
> +		if (drm_WARN(dev, ret,
> +			     "prepare vblank failed on crtc %i, ret=%i\n",
> +			     pipe, ret))
> +			return;
> +	}
> +
>  	ret = drm_vblank_get(dev, pipe);
>  	if (drm_WARN(dev, ret, "vblank not available on crtc %i, ret=%i\n",
>  		     pipe, ret))
> @@ -1485,10 +1519,18 @@ void drm_crtc_vblank_on_config(struct drm_crtc *crtc,
>  	struct drm_device *dev = crtc->dev;
>  	unsigned int pipe = drm_crtc_index(crtc);
>  	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
> +	int ret;
>  
>  	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
>  		return;
>  
> +	if (crtc) {
> +		ret = drm_crtc_vblank_prepare(crtc);
> +		drm_WARN_ON(dev, ret);
> +		if (ret)
> +			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 +1838,17 @@ 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) {
> +			drm_dbg_core(dev,
> +				     "prepare vblank failed on crtc %i, ret=%i\n",
> +				     pipe, ret);
> +			return ret;
> +		}
> +	}
> +
>  	ret = drm_vblank_get(dev, pipe);
>  	if (ret) {
>  		drm_dbg_core(dev,
> @@ -2031,6 +2084,14 @@ 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) {
> +			drm_dbg_core(dev,
> +				     "prepare vblank failed on crtc %i, ret=%i\n",
> +				     pipe, ret);
> +			return ret;
> +		}
> +
>  		ret = drm_crtc_vblank_get(crtc);
>  		if (ret) {
>  			drm_dbg_core(dev,
> @@ -2098,6 +2159,14 @@ 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) {
> +		drm_dbg_core(dev,
> +			     "prepare vblank failed on crtc %i, ret=%i\n",
> +			     pipe, ret);
> +		return ret;
> +	}
> +
>  	ret = drm_crtc_vblank_get(crtc);
>  	if (ret) {
>  		drm_dbg_core(dev,
> diff --git a/drivers/gpu/drm/drm_vblank_work.c b/drivers/gpu/drm/drm_vblank_work.c
> index e4e1873f0e1e1..582ee7fd94adf 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 caa56e039da2a..456cf9ba0143a 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -860,6 +860,33 @@ struct drm_crtc_funcs {
>  	 */
>  	u32 (*get_vblank_counter)(struct drm_crtc *crtc);
>  
> +	/**
> +	 * @prepare_enable_vblank:
> +	 *
> +	 * An optional callback for preparing to enable vblank interrupts. It
> +	 * allows drivers to perform blocking operations, and thus is called
> +	 * without any vblank spinlocks. Consequently, this callback is not
> +	 * synchronized with the rest of drm_vblank management; drivers are
> +	 * responsible for ensuring it won't race with drm_vblank and it's other
> +	 * driver callbacks.
> +	 *
> +	 * For example, drivers may use this to spin-up hardware from a low
> +	 * power state -- which may require blocking operations -- such that
> +	 * hardware registers are available to read/write. However, the driver
> +	 * must be careful as to when to reenter low-power state, such that it
> +	 * won't race with enable_vblank.
> +	 *
> +	 * It is called unconditionally, regardless of whether vblank interrupts
> +	 * are already enabled or not.
> +	 *
> +	 * 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:
>  	 *
> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> index 151ab1e85b1b7..5abc367aa4376 100644
> --- a/include/drm/drm_vblank.h
> +++ b/include/drm/drm_vblank.h
> @@ -272,6 +272,7 @@ void drm_vblank_set_event(struct drm_pending_vblank_event *e,
>  			  ktime_t *now);
>  bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe);
>  bool drm_crtc_handle_vblank(struct drm_crtc *crtc);
> +int drm_crtc_vblank_prepare(struct drm_crtc *crtc);
>  int drm_crtc_vblank_get(struct drm_crtc *crtc);
>  void drm_crtc_vblank_put(struct drm_crtc *crtc);
>  void drm_wait_one_vblank(struct drm_device *dev, unsigned int pipe);

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

[Harry Wentland]

On 2025-12-01 18:18, 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.
> 
> v2: Add missing semicolon, add docstring for prepare_vbl_disallow_idle
> 
> Signed-off-by: Leo Li <sunpeng.li@amd.com>

Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Harry

> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  3 ++
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  9 +++++
>  .../drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c |  4 +++
>  .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c    | 36 +++++++++++++++++--
>  4 files changed, 49 insertions(+), 3 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 0346052f2e574..842a93e2d6ce0 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -9682,6 +9682,7 @@ 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(drm_crtc_vblank_prepare(new_state->base.crtc) != 0);
>  		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",
> @@ -10108,6 +10109,7 @@ 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_prepare(pcrtc);
>  			drm_crtc_vblank_get(pcrtc);
>  
>  			spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
> @@ -10124,6 +10126,7 @@ 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) {
> +		drm_crtc_vblank_prepare(pcrtc);
>  		spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
>  		if (acrtc_attach->base.state->event) {
>  			drm_crtc_vblank_get(pcrtc);
> 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 7065b20aa2e6b..a99612fb3467a 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -587,6 +587,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..7839b56859391 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
> @@ -656,6 +656,10 @@ 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_prepare(crtc);
> +		if (ret)
> +			goto cleanup;
> +
>  		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 38f9ea313dcbb..dd693419111db 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,24 @@ 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_allow_idle_optimizations(dm->dc, false);
> +	}
> +
> +	return 0;
> +}
> +
>  int amdgpu_dm_crtc_enable_vblank(struct drm_crtc *crtc)
>  {
>  	return amdgpu_dm_crtc_set_vblank(crtc, true);
> @@ -590,6 +619,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,

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

[Jani Nikula]

On Mon, 01 Dec 2025, <sunpeng.li@amd.com> wrote:
> From: Leo Li <sunpeng.li@amd.com>
>
> Some drivers need to perform blocking 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 via a mutex,
> for example.
>
> To support this, introduce a new drm_crtc_vblank_prepare() helper that
> calls back into the driver -- if implemented -- for the driver to do
> such preparation work.
>
> In DRM core, call this helper before drm_vblank_get(). Drivers can
> choose to call this if they implement the callback in the future.

Have you considered hiding all of this inside drm_vblank.c? Call prepare
in drm_crtc_vblank_get() and a couple of other places? And actually
don't call it on !drm_dev_has_vblank(dev)?

There's just so much littering all over the place with the prepare, and
it seems brittle. Especially when you expect not only the drm core but
also the relevant drivers to call drm_crtc_vblank_prepare() when needed.

There does seem to be a few places in amdgpu that wrap the
drm_crtc_vblank_get() inside dev->event_lock, but is there really any
need to do so? Do the get first, and grab the event_lock after?

Some random comments inline.

Cc: Ville


BR,
Jani.


>
> Signed-off-by: Leo Li <sunpeng.li@amd.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c |  8 ++++
>  drivers/gpu/drm/drm_fb_helper.c     |  4 ++
>  drivers/gpu/drm/drm_plane.c         |  4 ++
>  drivers/gpu/drm/drm_vblank.c        | 69 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_vblank_work.c   |  8 ++++
>  include/drm/drm_crtc.h              | 27 +++++++++++
>  include/drm/drm_vblank.h            |  1 +
>  7 files changed, 121 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index ef56b474acf59..e52dd41f83117 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1264,6 +1264,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
> @@ -1815,6 +1819,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_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 11a5b60cb9ce4..7400942fd7d1d 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1103,6 +1103,10 @@ int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd,
>  		 * enabled, otherwise just don't do anythintg,
>  		 * not even report an error.
>  		 */
> +		ret = drm_crtc_vblank_prepare(crtc);
> +		if (ret)
> +			break;
> +
>  		ret = drm_crtc_vblank_get(crtc);
>  		if (!ret) {
>  			drm_crtc_wait_one_vblank(crtc);
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 38f82391bfda5..f2e40eaa385e6 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1421,6 +1421,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 46f59883183d9..4dac3228c021f 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -1194,6 +1194,30 @@ 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 run blocking operations to prepare for enabling
> + * vblank interrupts. This function calls the prepare_enable_vblank callback, if
> + * available, to allow drivers to do that.
> + *
> + * The spin-up may call blocking functions, such as mutex_lock(). Therefore,
> + * this must be called from process context, where sleeping is allowed.
> + *
> + * Also see &drm_crtc_funcs.prepare_enable_vblank.
> + *
> + * Returns: Zero on success or a negative error code on failure.
> + */
> +int drm_crtc_vblank_prepare(struct drm_crtc *crtc)
> +{
> +	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);
> @@ -1288,12 +1312,22 @@ EXPORT_SYMBOL(drm_crtc_vblank_put);
>  void drm_wait_one_vblank(struct drm_device *dev, unsigned int pipe)
>  {
>  	struct drm_vblank_crtc *vblank = drm_vblank_crtc(dev, pipe);
> +	struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);

Initialization...

>  	int ret;
>  	u64 last;
>  
>  	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
>  		return;
>  
> +	crtc = drm_crtc_from_index(dev, pipe);

...and another initialization.

But really, this function needs to die, and you'll have the crtc without
looking it up [1]. I'd really love to land that first to not make that
*and* this series harder for absolutely no reason.

[1] https://lore.kernel.org/r/2a17538a24f1d12c3c82d9cde03363195b64d0cf.1764933891.git.jani.nikula@intel.com

> +	if (crtc) {
> +		ret = drm_crtc_vblank_prepare(crtc);
> +		if (drm_WARN(dev, ret,
> +			     "prepare vblank failed on crtc %i, ret=%i\n",
> +			     pipe, ret))

Do we really need the warning backtraces or debug logs for every call?
There's one driver that needs the call, and it always returns 0. And
there's like a hundred lines of debug logging in this patch.

If you insist, please at least use the [CRTC:%d:%s] style logging, and
make it all somehow consistent.

> +			return;
> +	}
> +
>  	ret = drm_vblank_get(dev, pipe);
>  	if (drm_WARN(dev, ret, "vblank not available on crtc %i, ret=%i\n",
>  		     pipe, ret))
> @@ -1485,10 +1519,18 @@ void drm_crtc_vblank_on_config(struct drm_crtc *crtc,
>  	struct drm_device *dev = crtc->dev;
>  	unsigned int pipe = drm_crtc_index(crtc);
>  	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
> +	int ret;
>  
>  	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
>  		return;
>  
> +	if (crtc) {
> +		ret = drm_crtc_vblank_prepare(crtc);
> +		drm_WARN_ON(dev, ret);
> +		if (ret)
> +			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 +1838,17 @@ 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) {
> +			drm_dbg_core(dev,
> +				     "prepare vblank failed on crtc %i, ret=%i\n",
> +				     pipe, ret);
> +			return ret;
> +		}
> +	}
> +
>  	ret = drm_vblank_get(dev, pipe);
>  	if (ret) {
>  		drm_dbg_core(dev,
> @@ -2031,6 +2084,14 @@ 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) {
> +			drm_dbg_core(dev,
> +				     "prepare vblank failed on crtc %i, ret=%i\n",
> +				     pipe, ret);
> +			return ret;
> +		}
> +
>  		ret = drm_crtc_vblank_get(crtc);
>  		if (ret) {
>  			drm_dbg_core(dev,
> @@ -2098,6 +2159,14 @@ 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) {
> +		drm_dbg_core(dev,
> +			     "prepare vblank failed on crtc %i, ret=%i\n",
> +			     pipe, ret);
> +		return ret;
> +	}
> +
>  	ret = drm_crtc_vblank_get(crtc);
>  	if (ret) {
>  		drm_dbg_core(dev,
> diff --git a/drivers/gpu/drm/drm_vblank_work.c b/drivers/gpu/drm/drm_vblank_work.c
> index e4e1873f0e1e1..582ee7fd94adf 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 caa56e039da2a..456cf9ba0143a 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -860,6 +860,33 @@ struct drm_crtc_funcs {
>  	 */
>  	u32 (*get_vblank_counter)(struct drm_crtc *crtc);
>  
> +	/**
> +	 * @prepare_enable_vblank:
> +	 *
> +	 * An optional callback for preparing to enable vblank interrupts. It
> +	 * allows drivers to perform blocking operations, and thus is called
> +	 * without any vblank spinlocks. Consequently, this callback is not
> +	 * synchronized with the rest of drm_vblank management; drivers are
> +	 * responsible for ensuring it won't race with drm_vblank and it's other
> +	 * driver callbacks.
> +	 *
> +	 * For example, drivers may use this to spin-up hardware from a low
> +	 * power state -- which may require blocking operations -- such that
> +	 * hardware registers are available to read/write. However, the driver
> +	 * must be careful as to when to reenter low-power state, such that it
> +	 * won't race with enable_vblank.
> +	 *
> +	 * It is called unconditionally, regardless of whether vblank interrupts
> +	 * are already enabled or not.
> +	 *
> +	 * 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:
>  	 *
> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> index 151ab1e85b1b7..5abc367aa4376 100644
> --- a/include/drm/drm_vblank.h
> +++ b/include/drm/drm_vblank.h
> @@ -272,6 +272,7 @@ void drm_vblank_set_event(struct drm_pending_vblank_event *e,
>  			  ktime_t *now);
>  bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe);
>  bool drm_crtc_handle_vblank(struct drm_crtc *crtc);
> +int drm_crtc_vblank_prepare(struct drm_crtc *crtc);
>  int drm_crtc_vblank_get(struct drm_crtc *crtc);
>  void drm_crtc_vblank_put(struct drm_crtc *crtc);
>  void drm_wait_one_vblank(struct drm_device *dev, unsigned int pipe);

-- 
Jani Nikula, Intel

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

[Ville Syrjälä]

On Tue, Dec 09, 2025 at 12:05:31PM +0200, Jani Nikula wrote:
> On Mon, 01 Dec 2025, <sunpeng.li@amd.com> wrote:
> > From: Leo Li <sunpeng.li@amd.com>
> >
> > Some drivers need to perform blocking 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 via a mutex,
> > for example.
> >
> > To support this, introduce a new drm_crtc_vblank_prepare() helper that
> > calls back into the driver -- if implemented -- for the driver to do
> > such preparation work.
> >
> > In DRM core, call this helper before drm_vblank_get(). Drivers can
> > choose to call this if they implement the callback in the future.
> 
> Have you considered hiding all of this inside drm_vblank.c? Call prepare
> in drm_crtc_vblank_get() and a couple of other places? And actually
> don't call it on !drm_dev_has_vblank(dev)?
> 
> There's just so much littering all over the place with the prepare, and
> it seems brittle. Especially when you expect not only the drm core but
> also the relevant drivers to call drm_crtc_vblank_prepare() when needed.
> 
> There does seem to be a few places in amdgpu that wrap the
> drm_crtc_vblank_get() inside dev->event_lock, but is there really any
> need to do so? Do the get first, and grab the event_lock after?
> 
> Some random comments inline.
> 
> Cc: Ville

drm_vblank_get() can get called from any kind of context.
The only workable solution might be the schedule a work from
.vblank_enable() and do whatever is needed from there.

-- 
Ville Syrjälä
Intel

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

[Leo Li]

On 2025-12-09 05:05, Jani Nikula wrote:
> On Mon, 01 Dec 2025, <sunpeng.li@amd.com> wrote:
>> From: Leo Li <sunpeng.li@amd.com>
>>
>> Some drivers need to perform blocking 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 via a mutex,
>> for example.
>>
>> To support this, introduce a new drm_crtc_vblank_prepare() helper that
>> calls back into the driver -- if implemented -- for the driver to do
>> such preparation work.
>>
>> In DRM core, call this helper before drm_vblank_get(). Drivers can
>> choose to call this if they implement the callback in the future.
> 
> Have you considered hiding all of this inside drm_vblank.c? Call prepare
> in drm_crtc_vblank_get() and a couple of other places? And actually
> don't call it on !drm_dev_has_vblank(dev)?
> 
> There's just so much littering all over the place with the prepare, and
> it seems brittle. Especially when you expect not only the drm core but
> also the relevant drivers to call drm_crtc_vblank_prepare() when needed.
> 
> There does seem to be a few places in amdgpu that wrap the
> drm_crtc_vblank_get() inside dev->event_lock, but is there really any
> need to do so? Do the get first, and grab the event_lock after?
> 
> Some random comments inline.
> 
> Cc: Ville
> 
> 
> BR,
> Jani.

Hi Jani,
Thanks for the feedback. I'll reply to the above in the other thread with
Ville, but addressing the in-lines below.
- Leo
> 
> 
>>
>> Signed-off-by: Leo Li <sunpeng.li@amd.com>
>> ---
>>  drivers/gpu/drm/drm_atomic_helper.c |  8 ++++
>>  drivers/gpu/drm/drm_fb_helper.c     |  4 ++
>>  drivers/gpu/drm/drm_plane.c         |  4 ++
>>  drivers/gpu/drm/drm_vblank.c        | 69 +++++++++++++++++++++++++++++
>>  drivers/gpu/drm/drm_vblank_work.c   |  8 ++++
>>  include/drm/drm_crtc.h              | 27 +++++++++++
>>  include/drm/drm_vblank.h            |  1 +
>>  7 files changed, 121 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index ef56b474acf59..e52dd41f83117 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -1264,6 +1264,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
>> @@ -1815,6 +1819,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_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index 11a5b60cb9ce4..7400942fd7d1d 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -1103,6 +1103,10 @@ int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd,
>>  		 * enabled, otherwise just don't do anythintg,
>>  		 * not even report an error.
>>  		 */
>> +		ret = drm_crtc_vblank_prepare(crtc);
>> +		if (ret)
>> +			break;
>> +
>>  		ret = drm_crtc_vblank_get(crtc);
>>  		if (!ret) {
>>  			drm_crtc_wait_one_vblank(crtc);
>> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
>> index 38f82391bfda5..f2e40eaa385e6 100644
>> --- a/drivers/gpu/drm/drm_plane.c
>> +++ b/drivers/gpu/drm/drm_plane.c
>> @@ -1421,6 +1421,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 46f59883183d9..4dac3228c021f 100644
>> --- a/drivers/gpu/drm/drm_vblank.c
>> +++ b/drivers/gpu/drm/drm_vblank.c
>> @@ -1194,6 +1194,30 @@ 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 run blocking operations to prepare for enabling
>> + * vblank interrupts. This function calls the prepare_enable_vblank callback, if
>> + * available, to allow drivers to do that.
>> + *
>> + * The spin-up may call blocking functions, such as mutex_lock(). Therefore,
>> + * this must be called from process context, where sleeping is allowed.
>> + *
>> + * Also see &drm_crtc_funcs.prepare_enable_vblank.
>> + *
>> + * Returns: Zero on success or a negative error code on failure.
>> + */
>> +int drm_crtc_vblank_prepare(struct drm_crtc *crtc)
>> +{
>> +	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);
>> @@ -1288,12 +1312,22 @@ EXPORT_SYMBOL(drm_crtc_vblank_put);
>>  void drm_wait_one_vblank(struct drm_device *dev, unsigned int pipe)
>>  {
>>  	struct drm_vblank_crtc *vblank = drm_vblank_crtc(dev, pipe);
>> +	struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
> 
> Initialization...
> 
>>  	int ret;
>>  	u64 last;
>>  
>>  	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
>>  		return;
>>  
>> +	crtc = drm_crtc_from_index(dev, pipe);
> 
> ...and another initialization.
> 
> But really, this function needs to die, and you'll have the crtc without
> looking it up [1]. I'd really love to land that first to not make that
> *and* this series harder for absolutely no reason.
> 
> [1] https://lore.kernel.org/r/2a17538a24f1d12c3c82d9cde03363195b64d0cf.1764933891.git.jani.nikula@intel.com
>

The series looks sensible. I can definitely rebase on them. 
>> +	if (crtc) {
>> +		ret = drm_crtc_vblank_prepare(crtc);
>> +		if (drm_WARN(dev, ret,
>> +			     "prepare vblank failed on crtc %i, ret=%i\n",
>> +			     pipe, ret))
> 
> Do we really need the warning backtraces or debug logs for every call?
> There's one driver that needs the call, and it always returns 0. And
> there's like a hundred lines of debug logging in this patch.
> 
> If you insist, please at least use the [CRTC:%d:%s] style logging, and
> make it all somehow consistent.
> 

I'm OK with dropping these. It may be better for drivers to print warnings
themselves since preparation work is vendor specific.
>> +			return;
>> +	}
>> +
>>  	ret = drm_vblank_get(dev, pipe);
>>  	if (drm_WARN(dev, ret, "vblank not available on crtc %i, ret=%i\n",
>>  		     pipe, ret))
>> @@ -1485,10 +1519,18 @@ void drm_crtc_vblank_on_config(struct drm_crtc *crtc,
>>  	struct drm_device *dev = crtc->dev;
>>  	unsigned int pipe = drm_crtc_index(crtc);
>>  	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
>> +	int ret;
>>  
>>  	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
>>  		return;
>>  
>> +	if (crtc) {
>> +		ret = drm_crtc_vblank_prepare(crtc);
>> +		drm_WARN_ON(dev, ret);
>> +		if (ret)
>> +			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 +1838,17 @@ 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) {
>> +			drm_dbg_core(dev,
>> +				     "prepare vblank failed on crtc %i, ret=%i\n",
>> +				     pipe, ret);
>> +			return ret;
>> +		}
>> +	}
>> +
>>  	ret = drm_vblank_get(dev, pipe);
>>  	if (ret) {
>>  		drm_dbg_core(dev,
>> @@ -2031,6 +2084,14 @@ 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) {
>> +			drm_dbg_core(dev,
>> +				     "prepare vblank failed on crtc %i, ret=%i\n",
>> +				     pipe, ret);
>> +			return ret;
>> +		}
>> +
>>  		ret = drm_crtc_vblank_get(crtc);
>>  		if (ret) {
>>  			drm_dbg_core(dev,
>> @@ -2098,6 +2159,14 @@ 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) {
>> +		drm_dbg_core(dev,
>> +			     "prepare vblank failed on crtc %i, ret=%i\n",
>> +			     pipe, ret);
>> +		return ret;
>> +	}
>> +
>>  	ret = drm_crtc_vblank_get(crtc);
>>  	if (ret) {
>>  		drm_dbg_core(dev,
>> diff --git a/drivers/gpu/drm/drm_vblank_work.c b/drivers/gpu/drm/drm_vblank_work.c
>> index e4e1873f0e1e1..582ee7fd94adf 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 caa56e039da2a..456cf9ba0143a 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -860,6 +860,33 @@ struct drm_crtc_funcs {
>>  	 */
>>  	u32 (*get_vblank_counter)(struct drm_crtc *crtc);
>>  
>> +	/**
>> +	 * @prepare_enable_vblank:
>> +	 *
>> +	 * An optional callback for preparing to enable vblank interrupts. It
>> +	 * allows drivers to perform blocking operations, and thus is called
>> +	 * without any vblank spinlocks. Consequently, this callback is not
>> +	 * synchronized with the rest of drm_vblank management; drivers are
>> +	 * responsible for ensuring it won't race with drm_vblank and it's other
>> +	 * driver callbacks.
>> +	 *
>> +	 * For example, drivers may use this to spin-up hardware from a low
>> +	 * power state -- which may require blocking operations -- such that
>> +	 * hardware registers are available to read/write. However, the driver
>> +	 * must be careful as to when to reenter low-power state, such that it
>> +	 * won't race with enable_vblank.
>> +	 *
>> +	 * It is called unconditionally, regardless of whether vblank interrupts
>> +	 * are already enabled or not.
>> +	 *
>> +	 * 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:
>>  	 *
>> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
>> index 151ab1e85b1b7..5abc367aa4376 100644
>> --- a/include/drm/drm_vblank.h
>> +++ b/include/drm/drm_vblank.h
>> @@ -272,6 +272,7 @@ void drm_vblank_set_event(struct drm_pending_vblank_event *e,
>>  			  ktime_t *now);
>>  bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe);
>>  bool drm_crtc_handle_vblank(struct drm_crtc *crtc);
>> +int drm_crtc_vblank_prepare(struct drm_crtc *crtc);
>>  int drm_crtc_vblank_get(struct drm_crtc *crtc);
>>  void drm_crtc_vblank_put(struct drm_crtc *crtc);
>>  void drm_wait_one_vblank(struct drm_device *dev, unsigned int pipe);
> 

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

[Leo Li]

On 2025-12-09 05:47, Ville Syrjälä wrote:
> On Tue, Dec 09, 2025 at 12:05:31PM +0200, Jani Nikula wrote:
>> On Mon, 01 Dec 2025, <sunpeng.li@amd.com> wrote:
>>> From: Leo Li <sunpeng.li@amd.com>
>>>
>>> Some drivers need to perform blocking 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 via a mutex,
>>> for example.
>>>
>>> To support this, introduce a new drm_crtc_vblank_prepare() helper that
>>> calls back into the driver -- if implemented -- for the driver to do
>>> such preparation work.
>>>
>>> In DRM core, call this helper before drm_vblank_get(). Drivers can
>>> choose to call this if they implement the callback in the future.
>>
>> Have you considered hiding all of this inside drm_vblank.c? Call prepare
>> in drm_crtc_vblank_get() and a couple of other places? And actually
>> don't call it on !drm_dev_has_vblank(dev)?
>>
>> There's just so much littering all over the place with the prepare, and
>> it seems brittle. Especially when you expect not only the drm core but
>> also the relevant drivers to call drm_crtc_vblank_prepare() when needed.
>>
>> There does seem to be a few places in amdgpu that wrap the
>> drm_crtc_vblank_get() inside dev->event_lock, but is there really any
>> need to do so? Do the get first, and grab the event_lock after?
>>
>> Some random comments inline.
>>
>> Cc: Ville
> 
> drm_vblank_get() can get called from any kind of context.
> The only workable solution might be the schedule a work from
> .vblank_enable() and do whatever is needed from there.
> 
Yeah, drm_vblank_get() can be called from interrupt context, so the sleeping
work has to happen outside of it.

The issue with deferring work from .enable_vblank() is drm_vblank_enable()
follows up immediately with a drm_update_vblank_count(); it expects HW to be
online for reading the vblank counter. Meanwhile, the prepare work can be
pending, and waiting for HW to be online from drm_update_vblank_count() wouldn't
be ideal.

I've thought about implementing a sw vblank counter while HW is waking up, but
that sounds overly complex. It would be simpler to somehow signal prepare work
before we enter non-sleeping context.

Would a function like drm_crtc_get_vblank_with_prepare() help? vblank_prepare()
can be wrapped in it before calling get_vblank(). drm_crtc_get_vblank()
call-sites outside of drm_vblank.c -- called from process context -- can be
replaced with it.

The only case it's not called from process context (within drm core) is in
drm_crtc_vblank_on_config(), which is in drm_vblank.c itself. I think
vblank_prepare() can be open coded there. Drivers can choose to call
get_vblank_with_prepare() if they actually have blocking prepare work.

Thanks,
Leo

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

[Leo Li]

On 2025-12-10 12:55, Leo Li wrote:
> 
> 
> On 2025-12-09 05:47, Ville Syrjälä wrote:
>> On Tue, Dec 09, 2025 at 12:05:31PM +0200, Jani Nikula wrote:
>>> On Mon, 01 Dec 2025, <sunpeng.li@amd.com> wrote:
>>>> From: Leo Li <sunpeng.li@amd.com>
>>>>
>>>> Some drivers need to perform blocking 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 via a mutex,
>>>> for example.
>>>>
>>>> To support this, introduce a new drm_crtc_vblank_prepare() helper that
>>>> calls back into the driver -- if implemented -- for the driver to do
>>>> such preparation work.
>>>>
>>>> In DRM core, call this helper before drm_vblank_get(). Drivers can
>>>> choose to call this if they implement the callback in the future.
>>>
>>> Have you considered hiding all of this inside drm_vblank.c? Call prepare
>>> in drm_crtc_vblank_get() and a couple of other places? And actually
>>> don't call it on !drm_dev_has_vblank(dev)?
>>>
>>> There's just so much littering all over the place with the prepare, and
>>> it seems brittle. Especially when you expect not only the drm core but
>>> also the relevant drivers to call drm_crtc_vblank_prepare() when needed.
>>>
>>> There does seem to be a few places in amdgpu that wrap the
>>> drm_crtc_vblank_get() inside dev->event_lock, but is there really any
>>> need to do so? Do the get first, and grab the event_lock after?
>>>
>>> Some random comments inline.
>>>
>>> Cc: Ville
>>
>> drm_vblank_get() can get called from any kind of context.
>> The only workable solution might be the schedule a work from
>> .vblank_enable() and do whatever is needed from there.
>>
> Yeah, drm_vblank_get() can be called from interrupt context, so the sleeping
> work has to happen outside of it.
> 
> The issue with deferring work from .enable_vblank() is drm_vblank_enable()
> follows up immediately with a drm_update_vblank_count(); it expects HW to be
> online for reading the vblank counter. Meanwhile, the prepare work can be
> pending, and waiting for HW to be online from drm_update_vblank_count() wouldn't
> be ideal.
> 
> I've thought about implementing a sw vblank counter while HW is waking up, but
> that sounds overly complex. It would be simpler to somehow signal prepare work
> before we enter non-sleeping context.
> 
> Would a function like drm_crtc_get_vblank_with_prepare() help? vblank_prepare()
> can be wrapped in it before calling get_vblank(). drm_crtc_get_vblank()
> call-sites outside of drm_vblank.c -- called from process context -- can be
> replaced with it.
> 
> The only case it's not called from process context (within drm core) is in
> drm_crtc_vblank_on_config(), which is in drm_vblank.c itself. I think
> vblank_prepare() can be open coded there. Drivers can choose to call
> get_vblank_with_prepare() if they actually have blocking prepare work.

Sorry for the delay, just had some time to look at this again.

It seems I missed a place in my last comment about combining prepare() and get()
into one function. drm_vblank_work_schedule() in drm_vblank_work.c calls
vblank_get() with the event_lock, so an option to vblank_prepare() separately is
still needed.

I don't think we actually need to export drm_crtc_vblank_prepare() to drivers.
It's purpose is for DRM to run blocking driver work to prepare for vblank
programming, not the other way around. Drivers can sequence prepare work
themselves, if they need to, before doing any programming.

I think vblank_prepare() can be hidden in drm_internal.h, to limit it's use to
DRM core. I'll spin up a v2 rebased on Jani's series with this and some other
comments addressed.

Thanks,
Leo

> 
> Thanks,
> Leo
>