On Sat, Dec 01, 2012 at 05:48:50PM +0000, Chris Wilson wrote:
> Before queuing the flip but crucially after attaching the unpin-work to
> the crtc, we continue to setup the unpin-work. However, should the
> hardware fire early, we see the connected unpin-work and queue the task.
> The task then promptly runs and unpins the fb before we finish taking
> the required references or even pinning it... Havoc.
>
> To close the race, we use the flip-pending atomic to indicate when the
> flip is finally setup and enqueued. So during the flip-done processing,
> we can check more accurately whether the flip was expected.
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
Hm, can't this logic race?
- emit the MI_FLIP
- flip irq happens because the gpu is idle and completes it right away
(or our thread is preempted), work->pending increments from 0 -> 1
- queue_flip sets work->pending to 1
So work->pending will be stuck at 1 forverer, the unpin never happens and
stalls all subsequent flips.
Then there's also the usual annoying maintainer questions:
- do we have a way to readily reproduce this race?
- do we have a chance to just shut out all these spurious pageflip
interrupts? If they all come from MMIO access, we should be able to lock
them out somehow ...
Cheers, Daniel
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 4 ++--
> drivers/gpu/drm/i915/i915_irq.c | 4 +++-
> drivers/gpu/drm/i915/intel_display.c | 23 ++++++++++++++++-------
> drivers/gpu/drm/i915/intel_drv.h | 5 ++++-
> 4 files changed, 25 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 8afc0dd..e6a11ca 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -317,7 +317,7 @@ static int i915_gem_pageflip_info(struct seq_file *m, void *data)
> seq_printf(m, "No flip due on pipe %c (plane %c)\n",
> pipe, plane);
> } else {
> - if (!work->pending) {
> + if (atomic_read(&work->pending) < INTEL_FLIP_COMPLETE) {
> seq_printf(m, "Flip queued on pipe %c (plane %c)\n",
> pipe, plane);
> } else {
> @@ -328,7 +328,7 @@ static int i915_gem_pageflip_info(struct seq_file *m, void *data)
> seq_printf(m, "Stall check enabled, ");
> else
> seq_printf(m, "Stall check waiting for page flip ioctl, ");
> - seq_printf(m, "%d prepares\n", work->pending);
> + seq_printf(m, "%d prepares\n", atomic_read(&work->pending));
>
> if (work->old_fb_obj) {
> struct drm_i915_gem_object *obj = work->old_fb_obj;
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 6cd3dc9..a4dc97f 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1466,7 +1466,9 @@ static void i915_pageflip_stall_check(struct drm_device *dev, int pipe)
> spin_lock_irqsave(&dev->event_lock, flags);
> work = intel_crtc->unpin_work;
>
> - if (work == NULL || work->pending || !work->enable_stall_check) {
> + if (work == NULL ||
> + atomic_read(&work->pending) >= INTEL_FLIP_COMPLETE ||
> + !work->enable_stall_check) {
> /* Either the pending flip IRQ arrived, or we're too early. Don't check */
> spin_unlock_irqrestore(&dev->event_lock, flags);
> return;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 78d12c4..2746b39 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6929,7 +6929,7 @@ static void do_intel_finish_page_flip(struct drm_device *dev,
>
> spin_lock_irqsave(&dev->event_lock, flags);
> work = intel_crtc->unpin_work;
> - if (work == NULL || !work->pending) {
> + if (work == NULL || atomic_read(&work->pending) < INTEL_FLIP_COMPLETE) {
> spin_unlock_irqrestore(&dev->event_lock, flags);
> return;
> }
> @@ -6977,13 +6977,13 @@ void intel_prepare_page_flip(struct drm_device *dev, int plane)
> to_intel_crtc(dev_priv->plane_to_crtc_mapping[plane]);
> unsigned long flags;
>
> + /* NB: An MMIO update of the plane base pointer will also
> + * generate a page-flip completion irq, i.e. every modeset
> + * is also accompanied by a spurious intel_prepare_page_flip().
> + */
> spin_lock_irqsave(&dev->event_lock, flags);
> - if (intel_crtc->unpin_work) {
> - if ((++intel_crtc->unpin_work->pending) > 1)
> - DRM_ERROR("Prepared flip multiple times\n");
> - } else {
> - DRM_DEBUG_DRIVER("preparing flip with no unpin work?\n");
> - }
> + if (intel_crtc->unpin_work)
> + atomic_inc_not_zero(&intel_crtc->unpin_work->pending);
> spin_unlock_irqrestore(&dev->event_lock, flags);
> }
>
> @@ -7020,6 +7020,8 @@ static int intel_gen2_queue_flip(struct drm_device *dev,
> intel_ring_emit(ring, fb->pitches[0]);
> intel_ring_emit(ring, obj->gtt_offset + intel_crtc->dspaddr_offset);
> intel_ring_emit(ring, 0); /* aux display base address, unused */
> +
> + atomic_set(&intel_crtc->unpin_work->pending, INTEL_FLIP_PENDING);
> intel_ring_advance(ring);
> return 0;
>
> @@ -7060,6 +7062,7 @@ static int intel_gen3_queue_flip(struct drm_device *dev,
> intel_ring_emit(ring, obj->gtt_offset + intel_crtc->dspaddr_offset);
> intel_ring_emit(ring, MI_NOOP);
>
> + atomic_set(&intel_crtc->unpin_work->pending, INTEL_FLIP_PENDING);
> intel_ring_advance(ring);
> return 0;
>
> @@ -7106,6 +7109,8 @@ static int intel_gen4_queue_flip(struct drm_device *dev,
> pf = 0;
> pipesrc = I915_READ(PIPESRC(intel_crtc->pipe)) & 0x0fff0fff;
> intel_ring_emit(ring, pf | pipesrc);
> +
> + atomic_set(&intel_crtc->unpin_work->pending, INTEL_FLIP_PENDING);
> intel_ring_advance(ring);
> return 0;
>
> @@ -7148,6 +7153,8 @@ static int intel_gen6_queue_flip(struct drm_device *dev,
> pf = 0;
> pipesrc = I915_READ(PIPESRC(intel_crtc->pipe)) & 0x0fff0fff;
> intel_ring_emit(ring, pf | pipesrc);
> +
> + atomic_set(&intel_crtc->unpin_work->pending, INTEL_FLIP_PENDING);
> intel_ring_advance(ring);
> return 0;
>
> @@ -7202,6 +7209,8 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
> intel_ring_emit(ring, (fb->pitches[0] | obj->tiling_mode));
> intel_ring_emit(ring, obj->gtt_offset + intel_crtc->dspaddr_offset);
> intel_ring_emit(ring, (MI_NOOP));
> +
> + atomic_set(&intel_crtc->unpin_work->pending, INTEL_FLIP_PENDING);
> intel_ring_advance(ring);
> return 0;
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 522061c..3915ca9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -401,7 +401,10 @@ struct intel_unpin_work {
> struct drm_i915_gem_object *old_fb_obj;
> struct drm_i915_gem_object *pending_flip_obj;
> struct drm_pending_vblank_event *event;
> - int pending;
> + atomic_t pending;
> +#define INTEL_FLIP_INACTIVE 0
> +#define INTEL_FLIP_PENDING 1
> +#define INTEL_FLIP_COMPLETE 2
> bool enable_stall_check;
> };
>
> --
> 1.7.10.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
