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.
>
>>>> 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.
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
