On Tue, Apr 19, 2016 at 09:52:22AM +0200, Maarten Lankhorst wrote: > Both intel_unpin_work.pending and intel_unpin_work.enable_stall_check > were used to see if work should be enabled. By only using pending > some special cases are gone, and access to unpin_work can be simplified. > > Use this to only access work members untilintel_mark_page_flip_active > is called, or intel_queue_mmio_flip is used. This will prevent > use-after-free, and makes it easier to verify accesses. > > Changes since v1: > - Reword commit message. > - Do not access unpin_work after intel_mark_page_flip_active. > - Add the right memory barriers. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 11 +++--- > drivers/gpu/drm/i915/intel_display.c | 71 ++++++++++++++---------------------- > drivers/gpu/drm/i915/intel_drv.h | 1 - > 3 files changed, 34 insertions(+), 49 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 931dc6086f3b..0092aaf47c43 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -612,9 +612,14 @@ 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 { > + u32 pending; > u32 addr; > > - if (atomic_read(&work->pending) < INTEL_FLIP_COMPLETE) { > + 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 if (pending >= INTEL_FLIP_COMPLETE) { > seq_printf(m, "Flip queued on pipe %c (plane %c)\n", > pipe, plane); > } else { > @@ -636,10 +641,6 @@ static int i915_gem_pageflip_info(struct seq_file *m, void *data) > work->flip_queued_vblank, > work->flip_ready_vblank, > drm_crtc_vblank_count(&crtc->base)); > - if (work->enable_stall_check) > - seq_puts(m, "Stall check enabled, "); > - else > - seq_puts(m, "Stall check waiting for page flip ioctl, "); > seq_printf(m, "%d prepares\n", atomic_read(&work->pending)); > > if (INTEL_INFO(dev)->gen >= 4) > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 4cb830e2a62e..97a8418f6539 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3896,8 +3896,6 @@ static void page_flip_completed(struct intel_crtc *intel_crtc) > struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev); > struct intel_unpin_work *work = intel_crtc->unpin_work; > > - /* ensure that the unpin work is consistent wrt ->pending. */ > - smp_rmb(); > intel_crtc->unpin_work = NULL; > > if (work->event) > @@ -10980,16 +10978,13 @@ static void do_intel_finish_page_flip(struct drm_device *dev, > spin_lock_irqsave(&dev->event_lock, flags); > work = intel_crtc->unpin_work; > > - /* Ensure we don't miss a work->pending update ... */ > - smp_rmb(); > + if (work && atomic_read(&work->pending) >= INTEL_FLIP_COMPLETE) { > + /* ensure that the unpin work is consistent wrt ->pending. */ > + smp_mb__after_atomic(); The docs on smp_mb__after/before_atomic() states that they are used with atomic functions that do not return a value. Why are we using it together with atomic_read() here? > > - if (work == NULL || atomic_read(&work->pending) < INTEL_FLIP_COMPLETE) { > - spin_unlock_irqrestore(&dev->event_lock, flags); > - return; > + page_flip_completed(intel_crtc); > } > > - page_flip_completed(intel_crtc); > - > spin_unlock_irqrestore(&dev->event_lock, flags); > } > > @@ -11087,10 +11082,8 @@ void intel_prepare_page_flip(struct drm_device *dev, int plane) > static inline void intel_mark_page_flip_active(struct intel_unpin_work *work) > { > /* Ensure that the work item is consistent when activating it ... */ > - smp_wmb(); > + smp_mb__before_atomic(); > atomic_set(&work->pending, INTEL_FLIP_PENDING); > - /* and that it is marked active as soon as the irq could fire. */ > - smp_wmb(); > } > > static int intel_gen2_queue_flip(struct drm_device *dev, > @@ -11124,7 +11117,6 @@ static int intel_gen2_queue_flip(struct drm_device *dev, > intel_ring_emit(engine, intel_crtc->unpin_work->gtt_offset); > intel_ring_emit(engine, 0); /* aux display base address, unused */ > > - intel_mark_page_flip_active(intel_crtc->unpin_work); > return 0; > } > > @@ -11156,7 +11148,6 @@ static int intel_gen3_queue_flip(struct drm_device *dev, > intel_ring_emit(engine, intel_crtc->unpin_work->gtt_offset); > intel_ring_emit(engine, MI_NOOP); > > - intel_mark_page_flip_active(intel_crtc->unpin_work); > return 0; > } > > @@ -11195,7 +11186,6 @@ static int intel_gen4_queue_flip(struct drm_device *dev, > pipesrc = I915_READ(PIPESRC(intel_crtc->pipe)) & 0x0fff0fff; > intel_ring_emit(engine, pf | pipesrc); > > - intel_mark_page_flip_active(intel_crtc->unpin_work); > return 0; > } > > @@ -11231,7 +11221,6 @@ static int intel_gen6_queue_flip(struct drm_device *dev, > pipesrc = I915_READ(PIPESRC(intel_crtc->pipe)) & 0x0fff0fff; > intel_ring_emit(engine, pf | pipesrc); > > - intel_mark_page_flip_active(intel_crtc->unpin_work); > return 0; > } > > @@ -11326,7 +11315,6 @@ static int intel_gen7_queue_flip(struct drm_device *dev, > intel_ring_emit(engine, intel_crtc->unpin_work->gtt_offset); > intel_ring_emit(engine, (MI_NOOP)); > > - intel_mark_page_flip_active(intel_crtc->unpin_work); > return 0; > } > > @@ -11453,8 +11441,6 @@ static void intel_do_mmio_flip(struct intel_mmio_flip *mmio_flip) > if (work == NULL) > return; > > - intel_mark_page_flip_active(work); > - > intel_pipe_update_start(crtc); > > if (INTEL_INFO(mmio_flip->i915)->gen >= 9) > @@ -11464,6 +11450,8 @@ static void intel_do_mmio_flip(struct intel_mmio_flip *mmio_flip) > ilk_do_mmio_flip(crtc, work); > > intel_pipe_update_end(crtc); > + > + intel_mark_page_flip_active(work); > } > > static void intel_mmio_flip_work_func(struct work_struct *work) > @@ -11529,15 +11517,14 @@ static bool __intel_pageflip_stall_check(struct drm_device *dev, > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > struct intel_unpin_work *work = intel_crtc->unpin_work; > u32 addr; > + u32 pending; > > - if (atomic_read(&work->pending) >= INTEL_FLIP_COMPLETE) > - return true; > - > - if (atomic_read(&work->pending) < INTEL_FLIP_PENDING) > - return false; > + pending = atomic_read(&work->pending); > + /* ensure that the unpin work is consistent wrt ->pending. */ > + smp_mb__after_atomic(); Why paired with atomic_read()? > > - if (!work->enable_stall_check) > - return false; > + if (pending != INTEL_FLIP_PENDING) > + return pending == INTEL_FLIP_COMPLETE; Am I correct in assuming that we can remove the enable_stall_check test here since it's always enabled? If so, that would be useful to explain in the commit message. > > if (work->flip_ready_vblank == 0) { > if (work->flip_queued_req && > @@ -11718,6 +11705,11 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, > */ > if (!mmio_flip) { > ret = i915_gem_object_sync(obj, engine, &request); > + if (!ret && !request) { > + request = i915_gem_request_alloc(engine, NULL); > + ret = PTR_ERR_OR_ZERO(request); > + } > + > if (ret) > goto cleanup_pending; > } > @@ -11731,36 +11723,29 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, > work->gtt_offset += intel_crtc->dspaddr_offset; > > if (mmio_flip) { > - ret = intel_queue_mmio_flip(dev, crtc, obj); > - if (ret) > - goto cleanup_unpin; > + work->flip_queued_vblank = drm_crtc_vblank_count(crtc); > > i915_gem_request_assign(&work->flip_queued_req, > obj->last_write_req); > - } else { > - if (!request) { > - request = i915_gem_request_alloc(engine, NULL); > - if (IS_ERR(request)) { > - ret = PTR_ERR(request); > - goto cleanup_unpin; > - } > - } > > + ret = intel_queue_mmio_flip(dev, crtc, obj); > + if (ret) > + goto cleanup_unpin; > + } else { > ret = dev_priv->display.queue_flip(dev, crtc, fb, obj, request, > page_flip_flags); > if (ret) > goto cleanup_unpin; > > i915_gem_request_assign(&work->flip_queued_req, request); > - } > > - if (request) > - i915_add_request_no_flush(request); > + work->flip_queued_vblank = drm_crtc_vblank_count(crtc); > + intel_mark_page_flip_active(work); > > - work->flip_queued_vblank = drm_crtc_vblank_count(crtc); > - work->enable_stall_check = true; > + i915_add_request_no_flush(request); > + } > > - i915_gem_track_fb(intel_fb_obj(work->old_fb), obj, > + i915_gem_track_fb(intel_fb_obj(old_fb), obj, > to_intel_plane(primary)->frontbuffer_bit); > mutex_unlock(&dev->struct_mutex); > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index e13ce2290de7..517baebb2399 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -957,7 +957,6 @@ struct intel_unpin_work { > struct drm_i915_gem_request *flip_queued_req; > u32 flip_queued_vblank; > u32 flip_ready_vblank; > - bool enable_stall_check; > }; > > struct intel_load_detect_pipe { > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Intel Sweden AB Registered Office: Knarrarnasgatan 15, 164 40 Kista, Stockholm, Sweden Registration Number: 556189-6027 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx