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, it's 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 existed ever since the introduction of pflip_status. Enabling replay or psr extended the duration of this window, and hence made corruption more likely to be observed.) [How] * Add a flip_programmed completion. Arm it (reinit_completion) under event_lock together with prepare_flip_isr(), and signal it (complete_all) right after update_planes_and_stream_adapter() programs the flip. It starts in the "completed" state at crtc init. * 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. * In the DCN OTG handler, only deliver flip completion once flip_programmed is signalled (try_wait_for_completion) and dc_get_flip_pending_on_otg() reports the flip is no longer pending. Otherwise leave the flip armed and retry on the next vupdate. 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 | 9 ++-- .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c | 5 +++ .../drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c | 34 +++++++++++--- .../display/amdgpu_dm/amdgpu_dm_irq_params.h | 8 ++++ drivers/gpu/drm/amd/display/dc/core/dc.c | 45 +++++++++++++++++++ drivers/gpu/drm/amd/display/dc/dc.h | 1 + 6 files changed, 92 insertions(+), 10 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 00f7a3b445ebf..571198c46c0c2 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -4384,17 +4384,17 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state, * from 0 -> n planes we have to skip a hardware generated event * and rely on sending it from software. */ + spin_lock_irqsave(&pcrtc->dev->event_lock, flags); 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); + /* Arm flip completion handling and event delivery */ + reinit_completion(&acrtc_attach->dm_irq_params.flip_programmed); prepare_flip_isr(acrtc_attach); - - spin_unlock_irqrestore(&pcrtc->dev->event_lock, flags); } + spin_unlock_irqrestore(&pcrtc->dev->event_lock, flags); if (acrtc_state->stream) { if (acrtc_state->freesync_vrr_info_changed) @@ -4468,6 +4468,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state, &bundle->stream_update, bundle->surface_updates); updated_planes_and_streams = true; + complete_all(&acrtc_attach->dm_irq_params.flip_programmed); /** * Enable or disable the interrupts on the backend. 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 8a6b732cf80c8..f8f4f8f350dbd 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 @@ -789,6 +789,11 @@ int amdgpu_dm_crtc_init(struct amdgpu_display_manager *dm, #ifdef AMD_PRIVATE_COLOR dm_crtc_additional_color_mgmt(&acrtc->base); #endif + + init_completion(&acrtc->dm_irq_params.flip_programmed); + /* No flip is in flight yet; start in the "programmed" state. */ + complete_all(&acrtc->dm_irq_params.flip_programmed); + return 0; fail: 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 7c6db2bfc553a..b4377444bf0bd 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 @@ -1990,14 +1990,36 @@ 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. + * On DCN, GRPH_PFLIP is not used; VUPDATE_NO_LOCK is the flip latch + * point, so deliver any pending pageflip completion event from here. + * But only once the armed flip has actually been programmed into HW + * (flip_programmed) and HW has consumed the new address (the OTG no + * longer reports a pending flip). This avoids reporting completion for + * a flip whose address has not yet been latched, which would let + * userspace render over the still-presented buffer and cause + * corruption. * - * 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 && + try_wait_for_completion(&acrtc->dm_irq_params.flip_programmed)) { + + 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/amdgpu_dm/amdgpu_dm_irq_params.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq_params.h index f0c1b0c1faa9f..52c3c45cfe680 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq_params.h +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq_params.h @@ -27,12 +27,20 @@ #ifndef __AMDGPU_DM_IRQ_PARAMS_H__ #define __AMDGPU_DM_IRQ_PARAMS_H__ +#include <linux/completion.h> + #include "amdgpu_dm_crc.h" struct dm_irq_params { u32 last_flip_vblank; struct mod_vrr_params vrr_params; struct dc_stream_state *stream; + + /* + * Completed once an armed flip has actually been programmed into HW. + */ + struct completion flip_programmed; + int active_planes; bool allow_sr_entry; struct mod_freesync_config freesync_config; diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c index 379d3c97a5211..c7b48d321c108 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 2de0f9cf8264f..f143b250cbf7d 100644 --- a/drivers/gpu/drm/amd/display/dc/dc.h +++ b/drivers/gpu/drm/amd/display/dc/dc.h @@ -2990,6 +2990,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
