From: Leo Li <[email protected]>
[Why]
After unifying DCN interrupt sources under VUPDATE_NO_LOCK, we have two
remaining issues to clean up:
1. On DCN, flip completion is now delivered from VUPDATE_NO_LOCK
(dm_crtc_high_irq_handler) instead of GRPH_PFLIP. But VUPDATE_NO_LOCK
fires every frame, regardless of whether a flip has latched.
2. There is a window during commit where a flip is armed (pflip_status =
SUBMITTED) but not yet programmed into HW. If the VUPDATE_NO_LOCK
fires in that window, its handler would deliver a flip event to
userspace before HW has latched to it. If userspace then renders to
what it believes is now the back buffer (but HW is still latched to
it!), it will cause display corruption. This issue seemed to have
been introduced by:
commit 1159898a88db ("drm/amd/display: Handle commit plane with no FB.")
Enabling replay or psr extended the duration of this window, and
hence made corruption more likely to be observed.
[How]
* Move acrtc->event/pflip_status arming to after
update_planes_and_stream_adapter() has programmed the flip into HW.
This closes the window where pflip_status is SUBMITTED but the flip is
not yet programmed.
* Add dc_get_flip_pending_on_otg(), which reads the HUBP flip-pending
status straight from HW for the pipe(s) bound to an OTG instance. It
is keyed only by otg_inst and does not take or mutate a
dc_plane_state, so it is safe to call from the OTG interrupt handler
without racing a concurrent commit that may be modifying plane state.
* Optimistically query for flip-pending after programming, in the event
that HW latched to the new fb between programming start and arming
event. If it latched, send the vblank event immediately, rather than
wait for the next vblank IRQ.
* In the VUPDATE_NO_LOCK handler, only deliver flip completion once
dc_get_flip_pending_on_otg() reports the flip is no longer pending.
Otherwise leave the flip armed and retry on the next vupdate.
* For DCE, maintain the existing behavior of arming flips before
programming, and relying on GRPH_FLIP to fire at HW latch.
v2:
* Drop flip_programmed completion object, instead move
event/pflip_status arming after programming.
* For DCN, optimistically query for flip pending immediately after
programming, and if it latched, send event right away.
Cc: [email protected]
Fixes: 9b47278cec98 ("drm/amd/display: temp w/a for dGPU to enter idle
optimizations")
Link: https://gitlab.freedesktop.org/drm/amd/-/work_items/3787
Link: https://gitlab.freedesktop.org/drm/amd/-/work_items/4141
Assisted-by: Copilot:claude-opus-4.8
Signed-off-by: Leo Li <[email protected]>
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 125 ++++++++++++------
.../drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c | 27 +++-
drivers/gpu/drm/amd/display/dc/core/dc.c | 45 +++++++
drivers/gpu/drm/amd/display/dc/dc.h | 1 +
4 files changed, 156 insertions(+), 42 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 da118377b73a8..732ddafb5cfea 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4135,6 +4135,28 @@ static void amdgpu_dm_enable_self_refresh(struct
amdgpu_display_manager *dm,
}
}
+static void dm_arm_vblank_event(struct amdgpu_crtc *acrtc,
+ struct dm_crtc_state *acrtc_state,
+ bool pflip_update,
+ bool cursor_update)
+{
+ assert_spin_locked(&acrtc->base.dev->event_lock);
+
+ if (pflip_update && acrtc->base.state->event &&
+ acrtc_state->active_planes > 0) {
+ drm_crtc_vblank_get(&acrtc->base);
+ WARN_ON(acrtc->pflip_status != AMDGPU_FLIP_NONE);
+ /* Arm flip completion handling and event delivery after
programming. */
+ prepare_flip_isr(acrtc);
+ } else if (cursor_update && acrtc_state->active_planes > 0) {
+ if (acrtc->base.state->event) {
+ drm_crtc_vblank_get(&acrtc->base);
+ acrtc->event = acrtc->base.state->event;
+ acrtc->base.state->event = NULL;
+ }
+ }
+}
+
static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
struct drm_device *dev,
struct amdgpu_display_manager *dm,
@@ -4158,6 +4180,7 @@ static void amdgpu_dm_commit_planes(struct
drm_atomic_state *state,
bool cursor_update = false;
bool pflip_present = false;
bool immediate_flip = false;
+ bool flip_latched_during_prog = false;
bool dirty_rects_changed = false;
bool updated_planes_and_streams = false;
struct {
@@ -4390,39 +4413,23 @@ static void amdgpu_dm_commit_planes(struct
drm_atomic_state *state,
usleep_range(1000, 1100);
}
- /**
- * Prepare the flip event for the pageflip interrupt to handle.
- *
- * This only works in the case where we've already turned on the
- * appropriate hardware blocks (eg. HUBP) so in the transition
case
- * from 0 -> n planes we have to skip a hardware generated event
- * and rely on sending it from software.
- */
- if (acrtc_attach->base.state->event &&
- acrtc_state->active_planes > 0) {
- drm_crtc_vblank_get(pcrtc);
-
- spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
-
- WARN_ON(acrtc_attach->pflip_status != AMDGPU_FLIP_NONE);
- prepare_flip_isr(acrtc_attach);
-
- spin_unlock_irqrestore(&pcrtc->dev->event_lock, flags);
- }
-
if (acrtc_state->stream) {
if (acrtc_state->freesync_vrr_info_changed)
bundle->stream_update.vrr_infopacket =
&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;
- }
- spin_unlock_irqrestore(&pcrtc->dev->event_lock, flags);
+ }
+
+ /*
+ * DCE depends on a combination of GRPH_FLIP, VLINE0, and VUPDATE for
+ * event delivery. Only GRPH_FLIP handler can send pflip events, and it
+ * only fires if HW latched to the flip. Maintain legacy behavior by
+ * arming event before programming.
+ */
+ if (amdgpu_ip_version(dm->adev, DCE_HWIP, 0) == 0) {
+ scoped_guard(spinlock_irqsave, &pcrtc->dev->event_lock)
+ dm_arm_vblank_event(acrtc_attach, acrtc_state,
+ pflip_present, cursor_update);
}
/* Update the planes if changed or disable if we don't have any. */
@@ -4515,19 +4522,63 @@ static void amdgpu_dm_commit_planes(struct
drm_atomic_state *state,
amdgpu_dm_commit_cursors(state);
/*
- * On DCN, flip completion is normally delivered from VUPDATE_NO_LOCK.
- * However, an immediate (tearing / async) flip is latched by HW right
- * away and does not wait for the next vupdate, so deliver its
- * completion event here after programming.
+ * DCN specific vblank handling:
+ *
+ * With the event_lock held, arm the vblank event, and determine whether
+ * deliver it immediately, or in VUPDATE_NO_LOCK IRQ handler. Do this
+ * *after* programming so that vblank IRQ(s) will not deliver the event
+ * before HW consumes it.
+ *
+ * There's a consequence of arming after: it's possible for the IRQ to
+ * fire between HW programming and acrtc->event/pflip_status arming
+ * below. When this happens, the handler will skip event sending and
+ * flip_done signaling, even though HW has latched onto the programmed
+ * values, and send it on the next vblank.
+ *
+ * The more correct fix is to wrap programming and arming with the
+ * event_lock and thus serializing it with the IRQ handler. However,
+ * there are various sleep-waits on within
+ * update_planes_and_stream_adapter() that makes spin locking illegal.
+ * And on full updates, it can take 1-2 frame-times to return (see
+ * commit_planes_for_stream).
*
- * On DCE, GRPH_PFLIP already fires immediately for immediate flips, so
- * this is DCN-only.
+ * Details of how this is handled are in comments below.
*/
- if (immediate_flip && amdgpu_ip_version(dm->adev, DCE_HWIP, 0) != 0) {
+ if (amdgpu_ip_version(dm->adev, DCE_HWIP, 0) != 0) {
spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
- if (acrtc_attach->pflip_status == AMDGPU_FLIP_SUBMITTED &&
+ /*
+ * By the time programming returns, it's possible that HW has
+ * already latched. So, optimistically query whether HW latching
+ * has occurred, and if so, send out the event immediately
+ * rather than wait for the next IRQ.
+ */
+ if (updated_planes_and_streams)
+ flip_latched_during_prog =
+ !dc_get_flip_pending_on_otg(dm->dc,
acrtc_attach->otg_inst);
+
+ dm_arm_vblank_event(acrtc_attach, acrtc_state,
+ pflip_present, cursor_update);
+
+ /*
+ * Deliver the event immediately on immediate flip, or on a
+ * update that has already latched.
+ *
+ * Since drm_send_vblank_event() uses cooked values from the
+ * last drm_update_vblank_count(), which the VUPDATE_NO_LOCK irq
+ * handler hits via drm_crtc_handle_vblank(), the count and
+ * timestamp correctly reflect the most recent HW latch point.
+ *
+ * However, for immediate flips, the HW latch point is
+ * immediately after programming; the vblank count and timestamp
+ * need to be cooked again before sending.
+ *
+ * On DCE, GRPH_PFLIP used and takes care of this.
+ */
+ if ((immediate_flip || flip_latched_during_prog) &&
+ acrtc_attach->pflip_status == AMDGPU_FLIP_SUBMITTED &&
acrtc_attach->event) {
- drm_crtc_accurate_vblank_count(&acrtc_attach->base);
+ if (immediate_flip)
+
drm_crtc_accurate_vblank_count(&acrtc_attach->base);
drm_crtc_send_vblank_event(&acrtc_attach->base,
acrtc_attach->event);
acrtc_attach->event = NULL;
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
index d0181c797b71e..6018bccba7e5b 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
@@ -1991,13 +1991,30 @@ static void dm_crtc_high_irq_handler(struct
amdgpu_device *adev,
* Deliver pageflip completion events (DCN only).
*
* Since GRPH_PFLIP is not used, VUPDATE_NO_LOCK is the flip latch
- * point. Deliver any pending pageflip completion event from here.
+ * point. Deliver any pending pageflip completion event from here,
+ * once HW has consumed the new address (the OTG no longer reports a
+ * pending flip).
*
- * NOTE: This can deliver an event for a flip that was armed but not yet
- * programmed into HW; that race is closed in a follow-up change by
- * checking the programmed flip status.
+ * Also handle the case here where there aren't any active planes and
+ * DCN HUBP may be clock-gated, so the flip-pending status may be
+ * undefined.
*/
- if (is_dcn && acrtc->pflip_status == AMDGPU_FLIP_SUBMITTED) {
+ if (is_dcn && acrtc->pflip_status == AMDGPU_FLIP_SUBMITTED &&
+ acrtc->event) {
+
+ if (!dc_get_flip_pending_on_otg(adev->dm.dc, acrtc->otg_inst)) {
+ drm_crtc_send_vblank_event(&acrtc->base, acrtc->event);
+ acrtc->event = NULL;
+ drm_crtc_vblank_put(&acrtc->base);
+ acrtc->pflip_status = AMDGPU_FLIP_NONE;
+ }
+ /*
+ * If the flip is still pending, leave it armed and
+ * retry on the next vupdate.
+ */
+ } else if (is_dcn && acrtc->pflip_status == AMDGPU_FLIP_SUBMITTED &&
+ acrtc->dm_irq_params.active_planes == 0) {
+
if (acrtc->event) {
drm_crtc_send_vblank_event(&acrtc->base, acrtc->event);
acrtc->event = NULL;
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c
b/drivers/gpu/drm/amd/display/dc/core/dc.c
index ae776519e6ea6..330f31578cb03 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
@@ -6219,6 +6219,51 @@ void dc_interrupt_ack(struct dc *dc, enum dc_irq_source
src)
dal_irq_service_ack(dc->res_pool->irqs, src);
}
+/*
+ * dc_get_flip_pending_on_otg() - Check if a GRPH_FLIP is still pending on OTG
+ *
+ * @dc: display core context @otg_inst: OTG instance to query
+ *
+ * Reads the HUBP flip-pending status for the pipe(s) bound to @otg_inst,
+ * returning true if any of them has not yet latched its programmed surface
+ * address.
+ *
+ * Unlike dc_plane_get_status(), this does not take or mutate a dc_plane_state,
+ * so it is safe to call from interrupt context without racing a concurrent
+ * commit that may be updating plane state.
+ *
+ * Return: true if a flip is still pending on the OTG, false otherwise.
+ */
+bool dc_get_flip_pending_on_otg(struct dc *dc, int otg_inst)
+{
+ bool flip_pending = false;
+ int i;
+
+ if (!dc || !dc->current_state)
+ return false;
+
+ dc_exit_ips_for_hw_access(dc);
+
+ for (i = 0; i < dc->res_pool->pipe_count; i++) {
+ struct pipe_ctx *pipe_ctx =
&dc->current_state->res_ctx.pipe_ctx[i];
+ struct hubp *hubp = pipe_ctx->plane_res.hubp;
+
+ if (!pipe_ctx->plane_state || !pipe_ctx->stream_res.tg)
+ continue;
+
+ if (pipe_ctx->stream_res.tg->inst != otg_inst)
+ continue;
+
+ if (hubp && hubp->funcs->hubp_is_flip_pending &&
+ hubp->funcs->hubp_is_flip_pending(hubp)) {
+ flip_pending = true;
+ break;
+ }
+ }
+
+ return flip_pending;
+}
+
void dc_power_down_on_boot(struct dc *dc)
{
if (dc->ctx->dce_environment != DCE_ENV_VIRTUAL_HW &&
diff --git a/drivers/gpu/drm/amd/display/dc/dc.h
b/drivers/gpu/drm/amd/display/dc/dc.h
index 0e115b1aac5f7..81076d6287922 100644
--- a/drivers/gpu/drm/amd/display/dc/dc.h
+++ b/drivers/gpu/drm/amd/display/dc/dc.h
@@ -2992,6 +2992,7 @@ enum dc_irq_source dc_interrupt_to_irq_source(
uint32_t ext_id);
bool dc_interrupt_set(struct dc *dc, enum dc_irq_source src, bool enable);
void dc_interrupt_ack(struct dc *dc, enum dc_irq_source src);
+bool dc_get_flip_pending_on_otg(struct dc *dc, int otg_inst);
enum dc_irq_source dc_get_hpd_irq_source_at_index(
struct dc *dc, uint32_t link_index);
--
2.54.0
