Op 03-05-16 om 15:48 schreef Patrik Jakobsson: > 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 <maarten.lankho...@linux.intel.com> >>>>>> --- >>>>>> 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? Needs smp_mb instead of *_after_atomic, thanks for catching!
The before_atomic ones are correct, afaict. >>>>>> 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. Ok. ~Maarten _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx