On Thu, Apr 28, 2016 at 12:20:09PM +0200, Maarten Lankhorst wrote: > Op 28-04-16 om 11:54 schreef Patrik Jakobsson: > > On Thu, Apr 28, 2016 at 10:48:55AM +0200, Maarten Lankhorst wrote: > >> 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. > > Yes, the barriers are required. My point is that _after/before_atomic() > > should > > only be used with set/clear/inc etc atomic operations. For atomic operations > > that return a value you should use other macros. At least that is how I > > interpret the documentation. > > > > Here's the part from Documentation/atomic_ops.txt: > > > > -- > > > > If a caller requires memory barrier semantics around an atomic_t > > operation which does not return a value, a set of interfaces are > > defined which accomplish this: > > > > void smp_mb__before_atomic(void); > > void smp_mb__after_atomic(void); > > > > -- > > > > So I interpret this as, there's no guarantee that you'll get a full memory > > barrier from these macros. > >
Did you find an issue with this or is the current usage correct? > >>>> 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 > > The commit message tells what the patch does but not why. This might be > > obvious > > if you're familiar with the code. I stumbled a bit here so I guess I'm not > > :) > > > > "Both intel_unpin_work.pending and intel_unpin_work.enable_stall_check were > > used > > to see if work should be enabled" > > > > From this I imply that both checks are needed. > > > > "By only using pending some special cases are gone" > > > > This is what I don't find intuitive. Why can we suddently skip the > > enable_stall_check test? It would have been useful to know about the special > > case where a flip may be queued but stallcheck not yet active, and that > > it's no > > longer valid (and possibly why). > It looks like the pending member was added later to fix a race. It made > enable_stall_check obsolete, > but I'm not 100% sure that this was the reason. > > In any case for flips we set pending right before queueing, which eliminates > the race. Ok, can we add something like "A flip could previously be queued before stallcheck was active. With the addition of the pending member enable_stall_check became obsolete and can thus be removed. Pending is also set before queuing which eliminates the potential race"? Feel free to rephrase it. It's a bit verbose but I prefer that over misinterpreting the patch. -Patrik > > ~Maarten > _______________________________________________ > Intel-gfx mailing list > [email protected] > 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 [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
