From fd3a40242e8766ae342fa9fe77d44ae7c8139b45 Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Thu, 20 Jul 2017 19:57:51 +0200 Subject: [PATCH] drm/i915: Rip out legacy page_flip completion/irq handling Mime-version: 1.0 Content-type: text/plain; charset=UTF-8 Content-transfer-encoding: 8bit Git-commit: fd3a40242e8766ae342fa9fe77d44ae7c8139b45 Patch-mainline: v4.14-rc1 References: FATE#322643 bsc#1055900 All these races and things are now solved through the vblank evasion trick, plus event handling is done using normal vblank even processing and drm_crtc_arm_vblank_event. We can get rid of all this complexity. Cc: Maarten Lankhorst Cc: Ville Syrjälä Reviewed-by: Maarten Lankhorst Signed-off-by: Daniel Vetter Link: https://patchwork.freedesktop.org/patch/msgid/20170720175754.30751-5-daniel.vetter@ffwll.ch Acked-by: Takashi Iwai --- drivers/gpu/drm/i915/i915_irq.c | 151 +++--------------------- drivers/gpu/drm/i915/intel_display.c | 215 ----------------------------------- drivers/gpu/drm/i915/intel_drv.h | 3 3 files changed, 22 insertions(+), 347 deletions(-) --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -1708,18 +1708,6 @@ static void gen9_guc_irq_handler(struct } } -static bool intel_pipe_handle_vblank(struct drm_i915_private *dev_priv, - enum pipe pipe) -{ - bool ret; - - ret = drm_handle_vblank(&dev_priv->drm, pipe); - if (ret) - intel_finish_page_flip_mmio(dev_priv, pipe); - - return ret; -} - static void valleyview_pipestat_irq_ack(struct drm_i915_private *dev_priv, u32 iir, u32 pipe_stats[I915_MAX_PIPES]) { @@ -1784,12 +1772,8 @@ static void valleyview_pipestat_irq_hand enum pipe pipe; for_each_pipe(dev_priv, pipe) { - if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS && - intel_pipe_handle_vblank(dev_priv, pipe)) - intel_check_page_flip(dev_priv, pipe); - - if (pipe_stats[pipe] & PLANE_FLIP_DONE_INT_STATUS_VLV) - intel_finish_page_flip_cs(dev_priv, pipe); + if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS) + drm_handle_vblank(&dev_priv->drm, pipe); if (pipe_stats[pipe] & PIPE_CRC_DONE_INTERRUPT_STATUS) i9xx_pipe_crc_irq_handler(dev_priv, pipe); @@ -2241,19 +2225,14 @@ static void ilk_display_irq_handler(stru DRM_ERROR("Poison interrupt\n"); for_each_pipe(dev_priv, pipe) { - if (de_iir & DE_PIPE_VBLANK(pipe) && - intel_pipe_handle_vblank(dev_priv, pipe)) - intel_check_page_flip(dev_priv, pipe); + if (de_iir & DE_PIPE_VBLANK(pipe)) + drm_handle_vblank(&dev_priv->drm, pipe); if (de_iir & DE_PIPE_FIFO_UNDERRUN(pipe)) intel_cpu_fifo_underrun_irq_handler(dev_priv, pipe); if (de_iir & DE_PIPE_CRC_DONE(pipe)) i9xx_pipe_crc_irq_handler(dev_priv, pipe); - - /* plane/pipes map 1:1 on ilk+ */ - if (de_iir & DE_PLANE_FLIP_DONE(pipe)) - intel_finish_page_flip_cs(dev_priv, pipe); } /* check event from PCH */ @@ -2292,13 +2271,8 @@ static void ivb_display_irq_handler(stru intel_opregion_asle_intr(dev_priv); for_each_pipe(dev_priv, pipe) { - if (de_iir & (DE_PIPE_VBLANK_IVB(pipe)) && - intel_pipe_handle_vblank(dev_priv, pipe)) - intel_check_page_flip(dev_priv, pipe); - - /* plane/pipes map 1:1 on ilk+ */ - if (de_iir & DE_PLANE_FLIP_DONE_IVB(pipe)) - intel_finish_page_flip_cs(dev_priv, pipe); + if (de_iir & (DE_PIPE_VBLANK_IVB(pipe))) + drm_handle_vblank(&dev_priv->drm, pipe); } /* check event from PCH */ @@ -2479,7 +2453,7 @@ gen8_de_irq_handler(struct drm_i915_priv } for_each_pipe(dev_priv, pipe) { - u32 flip_done, fault_errors; + u32 fault_errors; if (!(master_ctl & GEN8_DE_PIPE_IRQ(pipe))) continue; @@ -2493,18 +2467,8 @@ gen8_de_irq_handler(struct drm_i915_priv ret = IRQ_HANDLED; I915_WRITE(GEN8_DE_PIPE_IIR(pipe), iir); - if (iir & GEN8_PIPE_VBLANK && - intel_pipe_handle_vblank(dev_priv, pipe)) - intel_check_page_flip(dev_priv, pipe); - - flip_done = iir; - if (INTEL_GEN(dev_priv) >= 9) - flip_done &= GEN9_PIPE_PLANE1_FLIP_DONE; - else - flip_done &= GEN8_PIPE_PRIMARY_FLIP_DONE; - - if (flip_done) - intel_finish_page_flip_cs(dev_priv, pipe); + if (iir & GEN8_PIPE_VBLANK) + drm_handle_vblank(&dev_priv->drm, pipe); if (iir & GEN8_PIPE_CDCLK_CRC_DONE) hsw_pipe_crc_irq_handler(dev_priv, pipe); @@ -3675,34 +3639,6 @@ static int i8xx_irq_postinstall(struct d /* * Returns true when a page flip has completed. */ -static bool i8xx_handle_vblank(struct drm_i915_private *dev_priv, - int plane, int pipe, u32 iir) -{ - u16 flip_pending = DISPLAY_PLANE_FLIP_PENDING(plane); - - if (!intel_pipe_handle_vblank(dev_priv, pipe)) - return false; - - if ((iir & flip_pending) == 0) - goto check_page_flip; - - /* We detect FlipDone by looking for the change in PendingFlip from '1' - * to '0' on the following vblank, i.e. IIR has the Pendingflip - * asserted following the MI_DISPLAY_FLIP, but ISR is deasserted, hence - * the flip is completed (no longer pending). Since this doesn't raise - * an interrupt per se, we watch for the change at vblank. - */ - if (I915_READ16(ISR) & flip_pending) - goto check_page_flip; - - intel_finish_page_flip_cs(dev_priv, pipe); - return true; - -check_page_flip: - intel_check_page_flip(dev_priv, pipe); - return false; -} - static irqreturn_t i8xx_irq_handler(int irq, void *arg) { struct drm_device *dev = arg; @@ -3710,9 +3646,6 @@ static irqreturn_t i8xx_irq_handler(int u16 iir, new_iir; u32 pipe_stats[2]; int pipe; - u16 flip_mask = - I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT | - I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT; irqreturn_t ret; if (!intel_irqs_enabled(dev_priv)) @@ -3726,7 +3659,7 @@ static irqreturn_t i8xx_irq_handler(int if (iir == 0) goto out; - while (iir & ~flip_mask) { + while (iir) { /* Can't rely on pipestat interrupt bit in iir as it might * have been cleared after the pipestat interrupt was received. * It doesn't set the bit in iir again, but it still produces @@ -3748,7 +3681,7 @@ static irqreturn_t i8xx_irq_handler(int } spin_unlock(&dev_priv->irq_lock); - I915_WRITE16(IIR, iir & ~flip_mask); + I915_WRITE16(IIR, iir); new_iir = I915_READ16(IIR); /* Flush posted writes */ if (iir & I915_USER_INTERRUPT) @@ -3759,9 +3692,8 @@ static irqreturn_t i8xx_irq_handler(int if (HAS_FBC(dev_priv)) plane = !plane; - if (pipe_stats[pipe] & PIPE_VBLANK_INTERRUPT_STATUS && - i8xx_handle_vblank(dev_priv, plane, pipe, iir)) - flip_mask &= ~DISPLAY_PLANE_FLIP_PENDING(plane); + if (pipe_stats[pipe] & PIPE_VBLANK_INTERRUPT_STATUS) + drm_handle_vblank(&dev_priv->drm, pipe); if (pipe_stats[pipe] & PIPE_CRC_DONE_INTERRUPT_STATUS) i9xx_pipe_crc_irq_handler(dev_priv, pipe); @@ -3861,45 +3793,11 @@ static int i915_irq_postinstall(struct d return 0; } -/* - * Returns true when a page flip has completed. - */ -static bool i915_handle_vblank(struct drm_i915_private *dev_priv, - int plane, int pipe, u32 iir) -{ - u32 flip_pending = DISPLAY_PLANE_FLIP_PENDING(plane); - - if (!intel_pipe_handle_vblank(dev_priv, pipe)) - return false; - - if ((iir & flip_pending) == 0) - goto check_page_flip; - - /* We detect FlipDone by looking for the change in PendingFlip from '1' - * to '0' on the following vblank, i.e. IIR has the Pendingflip - * asserted following the MI_DISPLAY_FLIP, but ISR is deasserted, hence - * the flip is completed (no longer pending). Since this doesn't raise - * an interrupt per se, we watch for the change at vblank. - */ - if (I915_READ(ISR) & flip_pending) - goto check_page_flip; - - intel_finish_page_flip_cs(dev_priv, pipe); - return true; - -check_page_flip: - intel_check_page_flip(dev_priv, pipe); - return false; -} - static irqreturn_t i915_irq_handler(int irq, void *arg) { struct drm_device *dev = arg; struct drm_i915_private *dev_priv = to_i915(dev); u32 iir, new_iir, pipe_stats[I915_MAX_PIPES]; - u32 flip_mask = - I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT | - I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT; int pipe, ret = IRQ_NONE; if (!intel_irqs_enabled(dev_priv)) @@ -3910,7 +3808,7 @@ static irqreturn_t i915_irq_handler(int iir = I915_READ(IIR); do { - bool irq_received = (iir & ~flip_mask) != 0; + bool irq_received = (iir) != 0; bool blc_event = false; /* Can't rely on pipestat interrupt bit in iir as it might @@ -3945,7 +3843,7 @@ static irqreturn_t i915_irq_handler(int i9xx_hpd_irq_handler(dev_priv, hotplug_status); } - I915_WRITE(IIR, iir & ~flip_mask); + I915_WRITE(IIR, iir); new_iir = I915_READ(IIR); /* Flush posted writes */ if (iir & I915_USER_INTERRUPT) @@ -3956,9 +3854,8 @@ static irqreturn_t i915_irq_handler(int if (HAS_FBC(dev_priv)) plane = !plane; - if (pipe_stats[pipe] & PIPE_VBLANK_INTERRUPT_STATUS && - i915_handle_vblank(dev_priv, plane, pipe, iir)) - flip_mask &= ~DISPLAY_PLANE_FLIP_PENDING(plane); + if (pipe_stats[pipe] & PIPE_VBLANK_INTERRUPT_STATUS) + drm_handle_vblank(&dev_priv->drm, pipe); if (pipe_stats[pipe] & PIPE_LEGACY_BLC_EVENT_STATUS) blc_event = true; @@ -3991,7 +3888,7 @@ static irqreturn_t i915_irq_handler(int */ ret = IRQ_HANDLED; iir = new_iir; - } while (iir & ~flip_mask); + } while (iir); enable_rpm_wakeref_asserts(dev_priv); @@ -4126,9 +4023,6 @@ static irqreturn_t i965_irq_handler(int u32 iir, new_iir; u32 pipe_stats[I915_MAX_PIPES]; int ret = IRQ_NONE, pipe; - u32 flip_mask = - I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT | - I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT; if (!intel_irqs_enabled(dev_priv)) return IRQ_NONE; @@ -4139,7 +4033,7 @@ static irqreturn_t i965_irq_handler(int iir = I915_READ(IIR); for (;;) { - bool irq_received = (iir & ~flip_mask) != 0; + bool irq_received = (iir) != 0; bool blc_event = false; /* Can't rely on pipestat interrupt bit in iir as it might @@ -4177,7 +4071,7 @@ static irqreturn_t i965_irq_handler(int i9xx_hpd_irq_handler(dev_priv, hotplug_status); } - I915_WRITE(IIR, iir & ~flip_mask); + I915_WRITE(IIR, iir); new_iir = I915_READ(IIR); /* Flush posted writes */ if (iir & I915_USER_INTERRUPT) @@ -4186,9 +4080,8 @@ static irqreturn_t i965_irq_handler(int notify_ring(dev_priv->engine[VCS]); for_each_pipe(dev_priv, pipe) { - if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS && - i915_handle_vblank(dev_priv, pipe, pipe, iir)) - flip_mask &= ~DISPLAY_PLANE_FLIP_PENDING(pipe); + if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS) + drm_handle_vblank(&dev_priv->drm, pipe); if (pipe_stats[pipe] & PIPE_LEGACY_BLC_EVENT_STATUS) blc_event = true; --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3405,14 +3405,6 @@ static void skylake_disable_primary_plan spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); } -static void intel_complete_page_flips(struct drm_i915_private *dev_priv) -{ - struct intel_crtc *crtc; - - for_each_intel_crtc(&dev_priv->drm, crtc) - intel_finish_page_flip_cs(dev_priv, crtc->pipe); -} - static int __intel_display_resume(struct drm_device *dev, struct drm_atomic_state *state, @@ -3528,13 +3520,6 @@ void intel_finish_reset(struct drm_i915_ if (!state) goto unlock; - /* - * Flips in the rings will be nuked by the reset, - * so complete all pending flips so that user space - * will get its events and not get stuck. - */ - intel_complete_page_flips(dev_priv); - dev_priv->modeset_restore_state = NULL; /* reset doesn't touch the display */ @@ -10128,140 +10113,6 @@ static void intel_crtc_destroy(struct dr kfree(intel_crtc); } -/* Is 'a' after or equal to 'b'? */ -static bool g4x_flip_count_after_eq(u32 a, u32 b) -{ - return !((a - b) & 0x80000000); -} - -static bool __pageflip_finished_cs(struct intel_crtc *crtc, - struct intel_flip_work *work) -{ - struct drm_device *dev = crtc->base.dev; - struct drm_i915_private *dev_priv = to_i915(dev); - - if (abort_flip_on_reset(crtc)) - return true; - - /* - * The relevant registers doen't exist on pre-ctg. - * As the flip done interrupt doesn't trigger for mmio - * flips on gmch platforms, a flip count check isn't - * really needed there. But since ctg has the registers, - * include it in the check anyway. - */ - if (INTEL_GEN(dev_priv) < 5 && !IS_G4X(dev_priv)) - return true; - - /* - * BDW signals flip done immediately if the plane - * is disabled, even if the plane enable is already - * armed to occur at the next vblank :( - */ - - /* - * A DSPSURFLIVE check isn't enough in case the mmio and CS flips - * used the same base address. In that case the mmio flip might - * have completed, but the CS hasn't even executed the flip yet. - * - * A flip count check isn't enough as the CS might have updated - * the base address just after start of vblank, but before we - * managed to process the interrupt. This means we'd complete the - * CS flip too soon. - * - * Combining both checks should get us a good enough result. It may - * still happen that the CS flip has been executed, but has not - * yet actually completed. But in case the base address is the same - * anyway, we don't really care. - */ - return (I915_READ(DSPSURFLIVE(crtc->plane)) & ~0xfff) == - crtc->flip_work->gtt_offset && - g4x_flip_count_after_eq(I915_READ(PIPE_FLIPCOUNT_G4X(crtc->pipe)), - crtc->flip_work->flip_count); -} - -static bool -__pageflip_finished_mmio(struct intel_crtc *crtc, - struct intel_flip_work *work) -{ - /* - * MMIO work completes when vblank is different from - * flip_queued_vblank. - * - * Reset counter value doesn't matter, this is handled by - * i915_wait_request finishing early, so no need to handle - * reset here. - */ - return intel_crtc_get_vblank_counter(crtc) != work->flip_queued_vblank; -} - - -static bool pageflip_finished(struct intel_crtc *crtc, - struct intel_flip_work *work) -{ - if (!atomic_read(&work->pending)) - return false; - - smp_rmb(); - - if (is_mmio_work(work)) - return __pageflip_finished_mmio(crtc, work); - else - return __pageflip_finished_cs(crtc, work); -} - -void intel_finish_page_flip_cs(struct drm_i915_private *dev_priv, int pipe) -{ - struct drm_device *dev = &dev_priv->drm; - struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, pipe); - struct intel_flip_work *work; - unsigned long flags; - - /* Ignore early vblank irqs */ - if (!crtc) - return; - - /* - * This is called both by irq handlers and the reset code (to complete - * lost pageflips) so needs the full irqsave spinlocks. - */ - spin_lock_irqsave(&dev->event_lock, flags); - work = crtc->flip_work; - - if (work != NULL && - !is_mmio_work(work) && - pageflip_finished(crtc, work)) - page_flip_completed(crtc); - - spin_unlock_irqrestore(&dev->event_lock, flags); -} - -void intel_finish_page_flip_mmio(struct drm_i915_private *dev_priv, int pipe) -{ - struct drm_device *dev = &dev_priv->drm; - struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, pipe); - struct intel_flip_work *work; - unsigned long flags; - - /* Ignore early vblank irqs */ - if (!crtc) - return; - - /* - * This is called both by irq handlers and the reset code (to complete - * lost pageflips) so needs the full irqsave spinlocks. - */ - spin_lock_irqsave(&dev->event_lock, flags); - work = crtc->flip_work; - - if (work != NULL && - is_mmio_work(work) && - pageflip_finished(crtc, work)) - page_flip_completed(crtc); - - spin_unlock_irqrestore(&dev->event_lock, flags); -} - static inline void intel_mark_page_flip_active(struct intel_crtc *crtc, struct intel_flip_work *work) { @@ -10272,72 +10123,6 @@ static inline void intel_mark_page_flip_ atomic_set(&work->pending, 1); } -static bool __pageflip_stall_check_cs(struct drm_i915_private *dev_priv, - struct intel_crtc *intel_crtc, - struct intel_flip_work *work) -{ - u32 addr, vblank; - - if (!atomic_read(&work->pending)) - return false; - - smp_rmb(); - - vblank = intel_crtc_get_vblank_counter(intel_crtc); - if (work->flip_ready_vblank == 0) { - if (work->flip_queued_req && - !i915_gem_request_completed(work->flip_queued_req)) - return false; - - work->flip_ready_vblank = vblank; - } - - if (vblank - work->flip_ready_vblank < 3) - return false; - - /* Potential stall - if we see that the flip has happened, - * assume a missed interrupt. */ - if (INTEL_GEN(dev_priv) >= 4) - addr = I915_HI_DISPBASE(I915_READ(DSPSURF(intel_crtc->plane))); - else - addr = I915_READ(DSPADDR(intel_crtc->plane)); - - /* There is a potential issue here with a false positive after a flip - * to the same address. We could address this by checking for a - * non-incrementing frame counter. - */ - return addr == work->gtt_offset; -} - -void intel_check_page_flip(struct drm_i915_private *dev_priv, int pipe) -{ - struct drm_device *dev = &dev_priv->drm; - struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, pipe); - struct intel_flip_work *work; - - WARN_ON(!in_interrupt()); - - if (crtc == NULL) - return; - - spin_lock(&dev->event_lock); - work = crtc->flip_work; - - if (work != NULL && !is_mmio_work(work) && - __pageflip_stall_check_cs(dev_priv, crtc, work)) { - WARN_ONCE(1, - "Kicking stuck page flip: queued at %d, now %d\n", - work->flip_queued_vblank, intel_crtc_get_vblank_counter(crtc)); - page_flip_completed(crtc); - work = NULL; - } - - if (work != NULL && !is_mmio_work(work) && - intel_crtc_get_vblank_counter(crtc) - work->flip_queued_vblank > 1) - intel_queue_rps_boost_for_request(work->flip_queued_req); - spin_unlock(&dev->event_lock); -} - /** * intel_wm_need_update - Check whether watermarks need updating * @plane: drm plane --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1407,9 +1407,6 @@ void intel_unpin_fb_vma(struct i915_vma struct drm_framebuffer * intel_framebuffer_create(struct drm_i915_gem_object *obj, struct drm_mode_fb_cmd2 *mode_cmd); -void intel_finish_page_flip_cs(struct drm_i915_private *dev_priv, int pipe); -void intel_finish_page_flip_mmio(struct drm_i915_private *dev_priv, int pipe); -void intel_check_page_flip(struct drm_i915_private *dev_priv, int pipe); int intel_prepare_plane_fb(struct drm_plane *plane, struct drm_plane_state *new_state); void intel_cleanup_plane_fb(struct drm_plane *plane,