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@xxxxxxxxxxxxxxx>
>> ---
>> 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
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
