Op 27-04-16 om 15:24 schreef Patrik Jakobsson: > 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 <[email protected]> >> --- >> 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? From Documentation/atomic_ops.txt:
*** WARNING: atomic_read() and atomic_set() DO NOT IMPLY BARRIERS! *** Plus a whole warning below how the atomic ops may be reordered. The memory barriers are definitely required. >> 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()? See above. ^ >> >> - 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. The commit message says stallcheck special handling is removed entirely. I thought it would imply that the special case, where a flip may be queued but stallcheck not yet active, is removed entirely. ~Maarten _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
