drm/amd/display: hold a vblank ref while writeback is pending

Writeback completion is detected in dm_crtc_high_irq(), the CRTC vblank
IRQ handler. The arm path (dm_set_writeback) never took a vblank
reference, so the interrupt was only enabled incidentally (by a pageflip
on the same commit, fbcon, or a previous vblank's off-delay window).

A writeback-only commit right after a fresh drm_crtc_vblank_on() (e.g. a
writeback connector detached and re-attached) therefore has no vblank
reference: the IRQ never fires, wb_pending is never cleared and the out
fence times out. This is reproducible with IGT kms_writeback and was
seen via kms_colorop on writeback-capable hardware. The relevant IGT
branch is at
https://gitlab.freedesktop.org/hwentland/igt-gpu-tools/-/tree/yuv-fm-colorop

Take a vblank reference when arming the writeback and release it once
completion is signalled. The get is done before arming wb_pending so the
completion IRQ cannot drop the reference before it is taken. Factor the
shared completion bookkeeping into amdgpu_dm_crtc_complete_writeback()
and also call it from the teardown path, so a writeback torn down while
still pending signals its out fence and releases the reference instead of
leaking both.

Fixes: c81e13b929 ("drm/amd/display: Hande writeback request from userspace")
Assisted-by: Copilot:claude-opus-4.8
Reviewed-by: Alex Hung <alex.hung@amd.com>
Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Signed-off-by: George Zhang <george.zhang@amd.com>
Signed-off-by: Alex Deucher <alexander.deucher@amd.com>

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c

@@ -4498,10 +4498,55 @@ static void amdgpu_dm_crtc_copy_transient_flags(struct drm_crtc_state *crtc_stat
 	stream_state->mode_changed = drm_atomic_crtc_needs_modeset(crtc_state);
 }
 
+/**
+ * amdgpu_dm_crtc_complete_writeback - finish a pending writeback job
+ * @acrtc: the CRTC whose pending writeback should be completed
+ *
+ * Clears the pending state, signals the writeback out fence and releases the
+ * vblank reference taken in dm_set_writeback() while the writeback was armed.
+ * The pending flag is tested and cleared under the writeback job lock, so this
+ * is safe to call concurrently from the completion vblank IRQ
+ * (dm_crtc_high_irq()) and from the writeback teardown path
+ * (dm_clear_writeback()); only the caller that observes the pending job
+ * performs the completion.
+ *
+ * Return: true if a pending writeback job was completed by this call.
+ */
+bool amdgpu_dm_crtc_complete_writeback(struct amdgpu_crtc *acrtc)
+{
+	unsigned long flags;
+	bool pending;
+
+	if (!acrtc->wb_conn)
+		return false;
+
+	spin_lock_irqsave(&acrtc->wb_conn->job_lock, flags);
+	pending = acrtc->wb_pending;
+	acrtc->wb_pending = false;
+	spin_unlock_irqrestore(&acrtc->wb_conn->job_lock, flags);
+
+	if (!pending)
+		return false;
+
+	drm_writeback_signal_completion(acrtc->wb_conn, 0);
+	drm_crtc_vblank_put(&acrtc->base);
+
+	return true;
+}
+
 static void dm_clear_writeback(struct amdgpu_display_manager *dm,
+			      struct amdgpu_crtc *acrtc,
 			      struct dm_crtc_state *crtc_state)
 {
 	dc_stream_remove_writeback(dm->dc, crtc_state->stream, 0);
+
+	/*
+	 * If the writeback is still pending when it is torn down (its
+	 * completion vblank IRQ never fired), signal the out fence so a
+	 * waiting client does not stall and release the vblank reference
+	 * taken in dm_set_writeback().
+	 */
+	amdgpu_dm_crtc_complete_writeback(acrtc);
 }
 
 /**
@@ -4654,7 +4699,7 @@ static void amdgpu_dm_commit_streams(struct drm_atomic_commit *state,
 
 		dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);
 
-		dm_clear_writeback(dm, dm_old_crtc_state);
+		dm_clear_writeback(dm, acrtc, dm_old_crtc_state);
 		acrtc->wb_enabled = false;
 	}
 
@@ -4928,9 +4973,24 @@ static void dm_set_writeback(struct amdgpu_display_manager *dm,
 
 	dc_stream_add_writeback(dm->dc, crtc_state->stream, wb_info);
 
-	acrtc->wb_pending = true;
 	acrtc->wb_conn = wb_conn;
 	drm_writeback_queue_job(wb_conn, new_con_state);
+
+	/*
+	 * Writeback completion is detected in the CRTC vblank IRQ
+	 * (dm_crtc_high_irq()). Take a vblank reference so the vblank interrupt
+	 * stays enabled while the writeback is pending; otherwise a
+	 * writeback-only commit right after drm_crtc_vblank_on() (e.g.
+	 * re-enabling a CRTC that was disabled) has no other vblank reference,
+	 * the IRQ never fires and the out fence times out. The matching put
+	 * happens once completion is signalled in dm_crtc_high_irq(), or when
+	 * the writeback is torn down in dm_clear_writeback().
+	 *
+	 * Arm wb_pending only after the reference is held so the completion IRQ
+	 * cannot run its matching vblank_put before this get.
+	 */
+	WARN_ON(drm_crtc_vblank_get(&acrtc->base));
+	acrtc->wb_pending = true;
 }
 
 static void amdgpu_dm_update_hdcp(struct drm_atomic_commit *state)

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h

@@ -1120,6 +1120,8 @@ void dm_free_gpu_mem(struct amdgpu_device *adev,
 
 bool amdgpu_dm_is_headless(struct amdgpu_device *adev);
 
+bool amdgpu_dm_crtc_complete_writeback(struct amdgpu_crtc *acrtc);
+
 void retrieve_dmi_info(struct amdgpu_display_manager *dm);
 
 void amdgpu_dm_emulated_link_detect(struct dc_link *link);

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c

@@ -1965,7 +1965,6 @@ static void dm_crtc_high_irq(void *interrupt_params)
 {
 	struct common_irq_params *irq_params = interrupt_params;
 	struct amdgpu_device *adev = irq_params->adev;
-	struct drm_writeback_job *job;
 	struct amdgpu_crtc *acrtc;
 	unsigned long flags;
 	int vrr_active;
@@ -1974,32 +1973,24 @@ static void dm_crtc_high_irq(void *interrupt_params)
 	if (!acrtc)
 		return;
 
-	if (acrtc->wb_conn) {
-		spin_lock_irqsave(&acrtc->wb_conn->job_lock, flags);
+	if (acrtc->wb_conn && acrtc->wb_pending) {
+		struct dc_stream_state *stream = acrtc->dm_irq_params.stream;
+		unsigned int v_total, refresh_hz;
 
-		if (acrtc->wb_pending) {
-			job = list_first_entry_or_null(&acrtc->wb_conn->job_queue,
-						       struct drm_writeback_job,
-						       list_entry);
-			acrtc->wb_pending = false;
-			spin_unlock_irqrestore(&acrtc->wb_conn->job_lock, flags);
+		v_total = stream->adjust.v_total_max ?
+			  stream->adjust.v_total_max : stream->timing.v_total;
+		refresh_hz = div_u64((uint64_t) stream->timing.pix_clk_100hz *
+			     100LL, (v_total * stream->timing.h_total));
+		mdelay(1000 / refresh_hz);
 
-			if (job) {
-				unsigned int v_total, refresh_hz;
-				struct dc_stream_state *stream = acrtc->dm_irq_params.stream;
-
-				v_total = stream->adjust.v_total_max ?
-					  stream->adjust.v_total_max : stream->timing.v_total;
-				refresh_hz = div_u64((uint64_t) stream->timing.pix_clk_100hz *
-					     100LL, (v_total * stream->timing.h_total));
-				mdelay(1000 / refresh_hz);
-
-				drm_writeback_signal_completion(acrtc->wb_conn, 0);
-				dc_stream_fc_disable_writeback(adev->dm.dc,
-							       acrtc->dm_irq_params.stream, 0);
-			}
-		} else
-			spin_unlock_irqrestore(&acrtc->wb_conn->job_lock, flags);
+		/*
+		 * Completion (signalling the out fence and releasing the vblank
+		 * reference taken in dm_set_writeback()) is handled by the shared
+		 * helper, which is also used by the teardown path.
+		 */
+		if (amdgpu_dm_crtc_complete_writeback(acrtc))
+			dc_stream_fc_disable_writeback(adev->dm.dc,
+						       acrtc->dm_irq_params.stream, 0);
 	}
 
 	vrr_active = amdgpu_dm_crtc_vrr_active_irq(acrtc);