Op 02-05-16 om 11:22 schreef Patrik Jakobsson: > On Tue, Apr 19, 2016 at 09:52:28AM +0200, Maarten Lankhorst wrote: >> This will be required to allow more than 1 update in the future. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankho...@linux.intel.com> >> --- >> drivers/gpu/drm/i915/i915_debugfs.c | 90 +++++++++++++++++------------- >> drivers/gpu/drm/i915/i915_drv.h | 2 +- >> drivers/gpu/drm/i915/intel_display.c | 105 >> ++++++++++++++++++++--------------- >> drivers/gpu/drm/i915/intel_drv.h | 4 +- >> 4 files changed, 114 insertions(+), 87 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c >> b/drivers/gpu/drm/i915/i915_debugfs.c >> index aabd7a13cec7..513c7da24c3d 100644 >> --- a/drivers/gpu/drm/i915/i915_debugfs.c >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >> @@ -589,6 +589,53 @@ static int i915_gem_gtt_info(struct seq_file *m, void >> *data) >> return 0; >> } >> >> +static void i915_dump_pageflip(struct seq_file *m, >> + struct drm_i915_private *dev_priv, >> + struct intel_crtc *crtc, >> + struct intel_flip_work *work) >> +{ >> + const char pipe = pipe_name(crtc->pipe); >> + const char plane = plane_name(crtc->plane); >> + u32 pending; >> + u32 addr; >> + >> + pending = atomic_read(&work->pending); >> + if (pending == INTEL_FLIP_INACTIVE) { >> + seq_printf(m, "Flip ioctl preparing on pipe %c (plane %c)\n", >> + pipe, plane); >> + } else { >> + seq_printf(m, "Flip pending (waiting for vsync) on pipe %c >> (plane %c)\n", >> + pipe, plane); >> + } >> + if (work->flip_queued_req) { >> + struct intel_engine_cs *engine = >> i915_gem_request_get_engine(work->flip_queued_req); >> + >> + seq_printf(m, "Flip queued on %s at seqno %x, next seqno %x >> [current breadcrumb %x], completed? %d\n", >> + engine->name, >> + i915_gem_request_get_seqno(work->flip_queued_req), >> + dev_priv->next_seqno, >> + engine->get_seqno(engine), >> + i915_gem_request_completed(work->flip_queued_req, >> true)); >> + } else >> + seq_printf(m, "Flip not associated with any ring\n"); >> + seq_printf(m, "Flip queued on frame %d, (was ready on frame %d), now >> %d\n", >> + work->flip_queued_vblank, >> + work->flip_ready_vblank, >> + intel_crtc_get_vblank_counter(crtc)); >> + seq_printf(m, "%d prepares\n", atomic_read(&work->pending)); >> + >> + if (INTEL_INFO(dev_priv)->gen >= 4) >> + addr = I915_HI_DISPBASE(I915_READ(DSPSURF(crtc->plane))); >> + else >> + addr = I915_READ(DSPADDR(crtc->plane)); >> + seq_printf(m, "Current scanout address 0x%08x\n", addr); >> + >> + if (work->pending_flip_obj) { >> + seq_printf(m, "New framebuffer address 0x%08lx\n", >> (long)work->gtt_offset); >> + seq_printf(m, "MMIO update completed? %d\n", addr == >> work->gtt_offset); >> + } >> +} >> + >> static int i915_gem_pageflip_info(struct seq_file *m, void *data) >> { >> struct drm_info_node *node = m->private; >> @@ -607,48 +654,13 @@ static int i915_gem_pageflip_info(struct seq_file *m, >> void *data) >> struct intel_flip_work *work; >> >> spin_lock_irq(&dev->event_lock); >> - work = crtc->flip_work; >> - if (work == NULL) { >> + if (list_empty(&crtc->flip_work)) { >> seq_printf(m, "No flip due on pipe %c (plane %c)\n", >> pipe, plane); >> } else { >> - u32 pending; >> - u32 addr; >> - >> - pending = atomic_read(&work->pending); >> - if (pending == INTEL_FLIP_INACTIVE) { >> - seq_printf(m, "Flip ioctl preparing on pipe %c >> (plane %c)\n", >> - pipe, plane); >> - } else { >> - seq_printf(m, "Flip pending (waiting for vsync) >> on pipe %c (plane %c)\n", >> - pipe, plane); >> - } >> - if (work->flip_queued_req) { >> - struct intel_engine_cs *engine = >> i915_gem_request_get_engine(work->flip_queued_req); >> - >> - seq_printf(m, "Flip queued on %s at seqno %x, >> next seqno %x [current breadcrumb %x], completed? %d\n", >> - engine->name, >> - >> i915_gem_request_get_seqno(work->flip_queued_req), >> - dev_priv->next_seqno, >> - engine->get_seqno(engine), >> - >> i915_gem_request_completed(work->flip_queued_req, true)); >> - } else >> - seq_printf(m, "Flip not associated with any >> ring\n"); >> - seq_printf(m, "Flip queued on frame %d, (was ready on >> frame %d), now %d\n", >> - work->flip_queued_vblank, >> - work->flip_ready_vblank, >> - intel_crtc_get_vblank_counter(crtc)); >> - seq_printf(m, "%d prepares\n", >> atomic_read(&work->pending)); >> - >> - if (INTEL_INFO(dev)->gen >= 4) >> - addr = >> I915_HI_DISPBASE(I915_READ(DSPSURF(crtc->plane))); >> - else >> - addr = I915_READ(DSPADDR(crtc->plane)); >> - seq_printf(m, "Current scanout address 0x%08x\n", addr); >> - >> - if (work->pending_flip_obj) { >> - seq_printf(m, "New framebuffer address >> 0x%08lx\n", (long)work->gtt_offset); >> - seq_printf(m, "MMIO update completed? %d\n", >> addr == work->gtt_offset); >> + list_for_each_entry(work, &crtc->flip_work, head); { > Accidental semi-colon after list_for_each_entry() Oops indeed! Thanks for catching. >> + i915_dump_pageflip(m, dev_priv, crtc, work); >> + seq_puts(m, "\n"); >> } >> } >> spin_unlock_irq(&dev->event_lock); >> diff --git a/drivers/gpu/drm/i915/i915_drv.h >> b/drivers/gpu/drm/i915/i915_drv.h >> index 85102ad75962..fb19fee24584 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -611,7 +611,7 @@ struct drm_i915_display_funcs { >> struct drm_framebuffer *fb, >> struct drm_i915_gem_object *obj, >> struct drm_i915_gem_request *req, >> - uint32_t flags); >> + uint64_t gtt_offset); >> void (*hpd_irq_setup)(struct drm_device *dev); >> /* clock updates for mode set */ >> /* cursor updates */ >> diff --git a/drivers/gpu/drm/i915/intel_display.c >> b/drivers/gpu/drm/i915/intel_display.c >> index d1181089512a..a2b4be06eb84 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -3300,17 +3300,12 @@ static bool intel_crtc_has_pending_flip(struct >> drm_crtc *crtc) >> struct drm_device *dev = crtc->dev; >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> unsigned reset_counter; >> - bool pending; >> >> reset_counter = i915_reset_counter(&to_i915(dev)->gpu_error); >> if (intel_crtc->reset_counter != reset_counter) >> return false; >> >> - spin_lock_irq(&dev->event_lock); >> - pending = to_intel_crtc(crtc)->flip_work != NULL; >> - spin_unlock_irq(&dev->event_lock); >> - >> - return pending; >> + return !list_empty_careful(&to_intel_crtc(crtc)->flip_work); >> } >> >> static void intel_update_pipe_config(struct intel_crtc *crtc, >> @@ -3886,7 +3881,7 @@ bool intel_has_pending_fb_unpin(struct drm_device *dev) >> if (atomic_read(&crtc->unpin_work_count) == 0) >> continue; >> >> - if (crtc->flip_work) >> + if (!list_empty_careful(&crtc->flip_work)) >> intel_wait_for_vblank(dev, crtc->pipe); >> >> return true; >> @@ -3895,12 +3890,11 @@ bool intel_has_pending_fb_unpin(struct drm_device >> *dev) >> return false; >> } >> >> -static void page_flip_completed(struct intel_crtc *intel_crtc) >> +static void page_flip_completed(struct intel_crtc *intel_crtc, struct >> intel_flip_work *work) >> { >> struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev); >> - struct intel_flip_work *work = intel_crtc->flip_work; >> >> - intel_crtc->flip_work = NULL; >> + list_del_init(&work->head); >> >> if (work->event) >> drm_crtc_send_vblank_event(&intel_crtc->base, work->event); >> @@ -3935,10 +3929,11 @@ static int intel_crtc_wait_for_pending_flips(struct >> drm_crtc *crtc) >> struct intel_flip_work *work; >> >> spin_lock_irq(&dev->event_lock); >> - work = intel_crtc->flip_work; >> + work = list_first_entry_or_null(&intel_crtc->flip_work, >> + struct intel_flip_work, head); > I'm not quite following this. Why is it enough for us to only look at the > first > entry. Is it hardcoded atm because we only have a list with a single entry. If > so we should write a comment about this. Yes, plus if a page flip gets stuck it would be the first entry in the list. >> if (work && !is_mmio_work(work)) { >> WARN_ONCE(1, "Removing stuck page flip\n"); >> - page_flip_completed(intel_crtc); >> + page_flip_completed(intel_crtc, work); >> } >> spin_unlock_irq(&dev->event_lock); >> } >> @@ -6335,7 +6330,7 @@ static void intel_crtc_disable_noatomic(struct >> drm_crtc *crtc) >> return; >> >> if (to_intel_plane_state(crtc->primary->state)->visible) { >> - WARN_ON(intel_crtc->flip_work); >> + WARN_ON(list_empty(&intel_crtc->flip_work)); >> >> intel_pre_disable_primary_noatomic(crtc); >> >> @@ -10924,17 +10919,24 @@ static void intel_crtc_destroy(struct drm_crtc >> *crtc) >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> struct drm_device *dev = crtc->dev; >> struct intel_flip_work *work; >> + struct list_head head; >> + >> + INIT_LIST_HEAD(&head); > What's this? Bogus leftover. >> >> spin_lock_irq(&dev->event_lock); >> - work = intel_crtc->flip_work; >> - intel_crtc->flip_work = NULL; >> - spin_unlock_irq(&dev->event_lock); >> + while (!list_empty(&intel_crtc->flip_work)) { >> + work = list_first_entry(&intel_crtc->flip_work, >> + struct intel_flip_work, head); > Hardcoded assumption here again? Nope, this removes all because of the while !list_empty. >> + list_del_init(&work->head); >> + spin_unlock_irq(&dev->event_lock); >> >> - if (work) { >> cancel_work_sync(&work->mmio_work); >> cancel_work_sync(&work->unpin_work); >> kfree(work); >> + >> + spin_lock_irq(&dev->event_lock); >> } >> + spin_unlock_irq(&dev->event_lock); >> >> drm_crtc_cleanup(crtc); >> >> @@ -10976,7 +10978,8 @@ static bool g4x_flip_count_after_eq(u32 a, u32 b) >> return !((a - b) & 0x80000000); >> } >> >> -static bool page_flip_finished(struct intel_crtc *crtc) >> +static bool page_flip_finished(struct intel_crtc *crtc, >> + struct intel_flip_work *work) >> { >> struct drm_device *dev = crtc->base.dev; >> struct drm_i915_private *dev_priv = dev->dev_private; >> @@ -11021,9 +11024,9 @@ static bool page_flip_finished(struct intel_crtc >> *crtc) >> * anyway, we don't really care. >> */ >> return (I915_READ(DSPSURFLIVE(crtc->plane)) & ~0xfff) == >> - crtc->flip_work->gtt_offset && >> + work->gtt_offset && >> >> g4x_flip_count_after_eq(I915_READ(PIPE_FLIPCOUNT_G4X(crtc->pipe)), >> - crtc->flip_work->flip_count); >> + work->flip_count); >> } >> >> static void do_intel_finish_page_flip(struct drm_device *dev, >> @@ -11042,12 +11045,14 @@ static void do_intel_finish_page_flip(struct >> drm_device *dev, >> * lost pageflips) so needs the full irqsave spinlocks. >> */ >> spin_lock_irqsave(&dev->event_lock, flags); >> - work = intel_crtc->flip_work; >> + work = list_first_entry_or_null(&intel_crtc->flip_work, >> + struct intel_flip_work, >> + head); > Hardcoded...? I'm only assuming a single flip will complete per call, so it makes sense here.
This might change in the future. >> >> if (work != NULL && >> atomic_read(&work->pending) == INTEL_FLIP_PENDING && >> - page_flip_finished(intel_crtc)) >> - page_flip_completed(intel_crtc); >> + page_flip_finished(intel_crtc, work)) >> + page_flip_completed(intel_crtc, work); >> >> spin_unlock_irqrestore(&dev->event_lock, flags); >> } >> @@ -11083,7 +11088,7 @@ static int intel_gen2_queue_flip(struct drm_device >> *dev, >> struct drm_framebuffer *fb, >> struct drm_i915_gem_object *obj, >> struct drm_i915_gem_request *req, >> - uint32_t flags) >> + uint64_t gtt_offset) >> { >> struct intel_engine_cs *engine = req->engine; >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> @@ -11106,7 +11111,7 @@ static int intel_gen2_queue_flip(struct drm_device >> *dev, >> intel_ring_emit(engine, MI_DISPLAY_FLIP | >> MI_DISPLAY_FLIP_PLANE(intel_crtc->plane)); >> intel_ring_emit(engine, fb->pitches[0]); >> - intel_ring_emit(engine, intel_crtc->flip_work->gtt_offset); >> + intel_ring_emit(engine, gtt_offset); >> intel_ring_emit(engine, 0); /* aux display base address, unused */ >> >> return 0; >> @@ -11117,7 +11122,7 @@ static int intel_gen3_queue_flip(struct drm_device >> *dev, >> struct drm_framebuffer *fb, >> struct drm_i915_gem_object *obj, >> struct drm_i915_gem_request *req, >> - uint32_t flags) >> + uint64_t gtt_offset) >> { >> struct intel_engine_cs *engine = req->engine; >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> @@ -11137,7 +11142,7 @@ static int intel_gen3_queue_flip(struct drm_device >> *dev, >> intel_ring_emit(engine, MI_DISPLAY_FLIP_I915 | >> MI_DISPLAY_FLIP_PLANE(intel_crtc->plane)); >> intel_ring_emit(engine, fb->pitches[0]); >> - intel_ring_emit(engine, intel_crtc->flip_work->gtt_offset); >> + intel_ring_emit(engine, gtt_offset); >> intel_ring_emit(engine, MI_NOOP); >> >> return 0; >> @@ -11148,7 +11153,7 @@ static int intel_gen4_queue_flip(struct drm_device >> *dev, >> struct drm_framebuffer *fb, >> struct drm_i915_gem_object *obj, >> struct drm_i915_gem_request *req, >> - uint32_t flags) >> + uint64_t gtt_offset) >> { >> struct intel_engine_cs *engine = req->engine; >> struct drm_i915_private *dev_priv = dev->dev_private; >> @@ -11167,8 +11172,7 @@ static int intel_gen4_queue_flip(struct drm_device >> *dev, >> intel_ring_emit(engine, MI_DISPLAY_FLIP | >> MI_DISPLAY_FLIP_PLANE(intel_crtc->plane)); >> intel_ring_emit(engine, fb->pitches[0]); >> - intel_ring_emit(engine, intel_crtc->flip_work->gtt_offset | >> - obj->tiling_mode); >> + intel_ring_emit(engine, gtt_offset | obj->tiling_mode); >> >> /* XXX Enabling the panel-fitter across page-flip is so far >> * untested on non-native modes, so ignore it for now. >> @@ -11186,7 +11190,7 @@ static int intel_gen6_queue_flip(struct drm_device >> *dev, >> struct drm_framebuffer *fb, >> struct drm_i915_gem_object *obj, >> struct drm_i915_gem_request *req, >> - uint32_t flags) >> + uint64_t gtt_offset) >> { >> struct intel_engine_cs *engine = req->engine; >> struct drm_i915_private *dev_priv = dev->dev_private; >> @@ -11201,7 +11205,7 @@ static int intel_gen6_queue_flip(struct drm_device >> *dev, >> intel_ring_emit(engine, MI_DISPLAY_FLIP | >> MI_DISPLAY_FLIP_PLANE(intel_crtc->plane)); >> intel_ring_emit(engine, fb->pitches[0] | obj->tiling_mode); >> - intel_ring_emit(engine, intel_crtc->flip_work->gtt_offset); >> + intel_ring_emit(engine, gtt_offset); >> >> /* Contrary to the suggestions in the documentation, >> * "Enable Panel Fitter" does not seem to be required when page >> @@ -11221,7 +11225,7 @@ static int intel_gen7_queue_flip(struct drm_device >> *dev, >> struct drm_framebuffer *fb, >> struct drm_i915_gem_object *obj, >> struct drm_i915_gem_request *req, >> - uint32_t flags) >> + uint64_t gtt_offset) >> { >> struct intel_engine_cs *engine = req->engine; >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> @@ -11304,7 +11308,7 @@ static int intel_gen7_queue_flip(struct drm_device >> *dev, >> >> intel_ring_emit(engine, MI_DISPLAY_FLIP_I915 | plane_bit); >> intel_ring_emit(engine, (fb->pitches[0] | obj->tiling_mode)); >> - intel_ring_emit(engine, intel_crtc->flip_work->gtt_offset); >> + intel_ring_emit(engine, gtt_offset); >> intel_ring_emit(engine, (MI_NOOP)); >> >> return 0; >> @@ -11371,17 +11375,17 @@ static int intel_default_queue_flip(struct >> drm_device *dev, >> struct drm_framebuffer *fb, >> struct drm_i915_gem_object *obj, >> struct drm_i915_gem_request *req, >> - uint32_t flags) >> + uint64_t gtt_offset) >> { >> return -ENODEV; >> } >> >> static bool __intel_pageflip_stall_check(struct drm_device *dev, >> - struct drm_crtc *crtc) >> + struct drm_crtc *crtc, >> + struct intel_flip_work *work) >> { >> struct drm_i915_private *dev_priv = dev->dev_private; >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> - struct intel_flip_work *work = intel_crtc->flip_work; >> u32 addr, vblank; >> u32 pending; >> >> @@ -11437,12 +11441,14 @@ void intel_check_page_flip(struct drm_device *dev, >> int pipe) >> return; >> >> spin_lock(&dev->event_lock); >> - work = intel_crtc->flip_work; >> - if (work != NULL && __intel_pageflip_stall_check(dev, crtc)) { >> + work = list_first_entry_or_null(&intel_crtc->flip_work, >> + struct intel_flip_work, head); > ... Yeah, should probably retry instead here if page flip is stalled. >> + >> + if (work != NULL && __intel_pageflip_stall_check(dev, crtc, work)) { >> WARN_ONCE(!is_mmio_work(work), >> "Kicking stuck page flip: queued at %d, now %d\n", >> work->flip_queued_vblank, >> intel_crtc_get_vblank_counter(intel_crtc)); >> - page_flip_completed(intel_crtc); >> + page_flip_completed(intel_crtc, work); >> work = NULL; >> } >> if (work != NULL && !is_mmio_work(work) && >> @@ -11508,13 +11514,18 @@ static int intel_crtc_page_flip(struct drm_crtc >> *crtc, >> >> /* We borrow the event spin lock for protecting flip_work */ >> spin_lock_irq(&dev->event_lock); >> - if (intel_crtc->flip_work) { >> + if (!list_empty(&intel_crtc->flip_work)) { >> + struct intel_flip_work *old_work; >> + >> + old_work = list_first_entry(&intel_crtc->flip_work, >> + struct intel_flip_work, head); >> + > ... Should probably be last entry here. _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx