[Intel-gfx] [PATCH 02/19] drm/i915: Remove stallcheck special handling, v2.
Patrik Jakobsson
patrik.jakobsson at linux.intel.com
Tue May 3 13:48:43 UTC 2016
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.lankhorst at 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?
> >>>> 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
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Intel Sweden AB Registered Office: Knarrarnasgatan 15, 164 40 Kista, Stockholm, Sweden Registration Number: 556189-6027
More information about the Intel-gfx
mailing list