Instead of sharing pages with breadcrumbs, give each timeline a single page. This allows unrelated timelines not to share locks any more during command submission.

As an additional benefit, seqno wraparound no longer requires i915_vma_pin, which means we no longer need to worry about a potential -EDEADLK at a point where we are ready to submit.

Changes since v1: - Fix erroneous i915_vma_acquire that should be a i915_vma_release (ickle). - Extra check for completion in intel_read_hwsp(). Changes since v2: - Fix inconsistent indent in hwsp_alloc() (kbuild) - memset entire cacheline to 0. Changes since v3: - Do same in intel_timeline_reset_seqno(), and clflush for good measure. Changes since v4: - Use refcounting on timeline, instead of relying on i915_active. - Fix waiting on kernel requests. Changes since v5: - Bump amount of slots to maximum (256), for best wraparounds. - Add hwsp_offset to i915_request to fix potential wraparound hang. - Ensure timeline wrap test works with the changes. - Assign hwsp in intel_timeline_read_hwsp() within the rcu lock to fix a hang. Changes since v6: - Rename i915_request_active_offset to i915_request_active_seqno(), and elaborate the function. (tvrtko) Changes since v7: - Move hunk to where it belongs. (jekstrand) - Replace CACHELINE_BYTES with TIMELINE_SEQNO_BYTES. (jekstrand)

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Reviewed-by: Thomas Hellström thomas.hellstrom@intel.com #v1 Reported-by: kernel test robot lkp@intel.com --- drivers/gpu/drm/i915/gt/gen2_engine_cs.c | 2 +- drivers/gpu/drm/i915/gt/gen6_engine_cs.c | 8 +- drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 13 +- drivers/gpu/drm/i915/gt/intel_engine_cs.c | 1 + drivers/gpu/drm/i915/gt/intel_gt_types.h | 4 - drivers/gpu/drm/i915/gt/intel_timeline.c | 422 ++++-------------- .../gpu/drm/i915/gt/intel_timeline_types.h | 17 +- drivers/gpu/drm/i915/gt/selftest_engine_cs.c | 5 +- drivers/gpu/drm/i915/gt/selftest_timeline.c | 88 ++-- drivers/gpu/drm/i915/i915_request.c | 4 - drivers/gpu/drm/i915/i915_request.h | 31 +- 11 files changed, 178 insertions(+), 417 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/gen2_engine_cs.c b/drivers/gpu/drm/i915/gt/gen2_engine_cs.c index b491a64919c8..9646200d2792 100644 --- a/drivers/gpu/drm/i915/gt/gen2_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/gen2_engine_cs.c @@ -143,7 +143,7 @@ static u32 *__gen2_emit_breadcrumb(struct i915_request *rq, u32 *cs, int flush, int post) { GEM_BUG_ON(i915_request_active_timeline(rq)->hwsp_ggtt != rq->engine->status_page.vma); - GEM_BUG_ON(offset_in_page(i915_request_active_timeline(rq)->hwsp_offset) != I915_GEM_HWS_SEQNO_ADDR); + GEM_BUG_ON(offset_in_page(rq->hwsp_seqno) != I915_GEM_HWS_SEQNO_ADDR);

*cs++ = MI_FLUSH;

diff --git a/drivers/gpu/drm/i915/gt/gen6_engine_cs.c b/drivers/gpu/drm/i915/gt/gen6_engine_cs.c index ce38d1bcaba3..b388ceeeb1c9 100644 --- a/drivers/gpu/drm/i915/gt/gen6_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/gen6_engine_cs.c @@ -161,7 +161,7 @@ u32 *gen6_emit_breadcrumb_rcs(struct i915_request *rq, u32 *cs) PIPE_CONTROL_DC_FLUSH_ENABLE | PIPE_CONTROL_QW_WRITE | PIPE_CONTROL_CS_STALL); - *cs++ = i915_request_active_timeline(rq)->hwsp_offset | + *cs++ = i915_request_active_seqno(rq) | PIPE_CONTROL_GLOBAL_GTT; *cs++ = rq->fence.seqno;

@@ -359,7 +359,7 @@ u32 *gen7_emit_breadcrumb_rcs(struct i915_request *rq, u32 *cs) PIPE_CONTROL_QW_WRITE | PIPE_CONTROL_GLOBAL_GTT_IVB | PIPE_CONTROL_CS_STALL); - *cs++ = i915_request_active_timeline(rq)->hwsp_offset; + *cs++ = i915_request_active_seqno(rq); *cs++ = rq->fence.seqno;

*cs++ = MI_USER_INTERRUPT; @@ -374,7 +374,7 @@ u32 *gen7_emit_breadcrumb_rcs(struct i915_request *rq, u32 *cs) u32 *gen6_emit_breadcrumb_xcs(struct i915_request *rq, u32 *cs) { GEM_BUG_ON(i915_request_active_timeline(rq)->hwsp_ggtt != rq->engine->status_page.vma); - GEM_BUG_ON(offset_in_page(i915_request_active_timeline(rq)->hwsp_offset) != I915_GEM_HWS_SEQNO_ADDR); + GEM_BUG_ON(offset_in_page(rq->hwsp_seqno) != I915_GEM_HWS_SEQNO_ADDR);

*cs++ = MI_FLUSH_DW | MI_FLUSH_DW_OP_STOREDW | MI_FLUSH_DW_STORE_INDEX; *cs++ = I915_GEM_HWS_SEQNO_ADDR | MI_FLUSH_DW_USE_GTT; @@ -394,7 +394,7 @@ u32 *gen7_emit_breadcrumb_xcs(struct i915_request *rq, u32 *cs) int i;

GEM_BUG_ON(i915_request_active_timeline(rq)->hwsp_ggtt != rq->engine->status_page.vma); - GEM_BUG_ON(offset_in_page(i915_request_active_timeline(rq)->hwsp_offset) != I915_GEM_HWS_SEQNO_ADDR); + GEM_BUG_ON(offset_in_page(rq->hwsp_seqno) != I915_GEM_HWS_SEQNO_ADDR);

*cs++ = MI_FLUSH_DW | MI_INVALIDATE_TLB | MI_FLUSH_DW_OP_STOREDW | MI_FLUSH_DW_STORE_INDEX; diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c index cac80af7ad1c..6b9c34d3ac8d 100644 --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c @@ -338,15 +338,14 @@ static u32 preempt_address(struct intel_engine_cs *engine)

static u32 hwsp_offset(const struct i915_request *rq) { - const struct intel_timeline_cacheline *cl; + const struct intel_timeline *tl;

- /* Before the request is executed, the timeline/cachline is fixed */ + /* Before the request is executed, the timeline is fixed */ + tl = rcu_dereference_protected(rq->timeline, + !i915_request_signaled(rq));

- cl = rcu_dereference_protected(rq->hwsp_cacheline, 1); - if (cl) - return cl->ggtt_offset; - - return rcu_dereference_protected(rq->timeline, 1)->hwsp_offset; + /* See the comment in i915_request_active_seqno(). */ + return page_mask_bits(tl->hwsp_offset) + offset_in_page(rq->hwsp_seqno); }

int gen8_emit_init_breadcrumb(struct i915_request *rq) diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c index b4df275cba79..e6cefd00b4a1 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c @@ -752,6 +752,7 @@ static int measure_breadcrumb_dw(struct intel_context *ce) frame->rq.engine = engine; frame->rq.context = ce; rcu_assign_pointer(frame->rq.timeline, ce->timeline); + frame->rq.hwsp_seqno = ce->timeline->hwsp_seqno;

frame->ring.vaddr = frame->cs; frame->ring.size = sizeof(frame->cs); diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h index 626af37c7790..3f6db8357309 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h @@ -45,10 +45,6 @@ struct intel_gt { struct intel_gt_timelines { spinlock_t lock; /* protects active_list */ struct list_head active_list; - - /* Pack multiple timelines' seqnos into the same page */ - spinlock_t hwsp_lock; - struct list_head hwsp_free_list; } timelines;

struct intel_gt_requests { diff --git a/drivers/gpu/drm/i915/gt/intel_timeline.c b/drivers/gpu/drm/i915/gt/intel_timeline.c index 491b8df174c2..efe2030cfe5e 100644 --- a/drivers/gpu/drm/i915/gt/intel_timeline.c +++ b/drivers/gpu/drm/i915/gt/intel_timeline.c @@ -11,21 +11,9 @@ #include "intel_ring.h" #include "intel_timeline.h"

-#define ptr_set_bit(ptr, bit) ((typeof(ptr))((unsigned long)(ptr) | BIT(bit))) -#define ptr_test_bit(ptr, bit) ((unsigned long)(ptr) & BIT(bit)) +#define TIMELINE_SEQNO_BYTES 8

-#define CACHELINE_BITS 6 -#define CACHELINE_FREE CACHELINE_BITS - -struct intel_timeline_hwsp { - struct intel_gt *gt; - struct intel_gt_timelines *gt_timelines; - struct list_head free_link; - struct i915_vma *vma; - u64 free_bitmap; -}; - -static struct i915_vma *__hwsp_alloc(struct intel_gt *gt) +static struct i915_vma *hwsp_alloc(struct intel_gt *gt) { struct drm_i915_private *i915 = gt->i915; struct drm_i915_gem_object *obj; @@ -44,220 +32,59 @@ static struct i915_vma *__hwsp_alloc(struct intel_gt *gt) return vma; }

-static struct i915_vma * -hwsp_alloc(struct intel_timeline *timeline, unsigned int *cacheline) -{ - struct intel_gt_timelines *gt = &timeline->gt->timelines; - struct intel_timeline_hwsp *hwsp; - - BUILD_BUG_ON(BITS_PER_TYPE(u64) * CACHELINE_BYTES > PAGE_SIZE); - - spin_lock_irq(&gt->hwsp_lock); - - /* hwsp_free_list only contains HWSP that have available cachelines */ - hwsp = list_first_entry_or_null(&gt->hwsp_free_list, - typeof(*hwsp), free_link); - if (!hwsp) { - struct i915_vma *vma; - - spin_unlock_irq(&gt->hwsp_lock); - - hwsp = kmalloc(sizeof(*hwsp), GFP_KERNEL); - if (!hwsp) - return ERR_PTR(-ENOMEM); - - vma = __hwsp_alloc(timeline->gt); - if (IS_ERR(vma)) { - kfree(hwsp); - return vma; - } - - GT_TRACE(timeline->gt, "new HWSP allocated\n"); - - vma->private = hwsp; - hwsp->gt = timeline->gt; - hwsp->vma = vma; - hwsp->free_bitmap = ~0ull; - hwsp->gt_timelines = gt; - - spin_lock_irq(&gt->hwsp_lock); - list_add(&hwsp->free_link, &gt->hwsp_free_list); - } - - GEM_BUG_ON(!hwsp->free_bitmap); - *cacheline = __ffs64(hwsp->free_bitmap); - hwsp->free_bitmap &= ~BIT_ULL(*cacheline); - if (!hwsp->free_bitmap) - list_del(&hwsp->free_link); - - spin_unlock_irq(&gt->hwsp_lock); - - GEM_BUG_ON(hwsp->vma->private != hwsp); - return hwsp->vma; -} - -static void __idle_hwsp_free(struct intel_timeline_hwsp *hwsp, int cacheline) -{ - struct intel_gt_timelines *gt = hwsp->gt_timelines; - unsigned long flags; - - spin_lock_irqsave(&gt->hwsp_lock, flags); - - /* As a cacheline becomes available, publish the HWSP on the freelist */ - if (!hwsp->free_bitmap) - list_add_tail(&hwsp->free_link, &gt->hwsp_free_list); - - GEM_BUG_ON(cacheline >= BITS_PER_TYPE(hwsp->free_bitmap)); - hwsp->free_bitmap |= BIT_ULL(cacheline); - - /* And if no one is left using it, give the page back to the system */ - if (hwsp->free_bitmap == ~0ull) { - i915_vma_put(hwsp->vma); - list_del(&hwsp->free_link); - kfree(hwsp); - } - - spin_unlock_irqrestore(&gt->hwsp_lock, flags); -} - -static void __rcu_cacheline_free(struct rcu_head *rcu) -{ - struct intel_timeline_cacheline *cl = - container_of(rcu, typeof(*cl), rcu); - - /* Must wait until after all *rq->hwsp are complete before removing */ - i915_gem_object_unpin_map(cl->hwsp->vma->obj); - __idle_hwsp_free(cl->hwsp, ptr_unmask_bits(cl->vaddr, CACHELINE_BITS)); - - i915_active_fini(&cl->active); - kfree(cl); -} - -static void __idle_cacheline_free(struct intel_timeline_cacheline *cl) -{ - GEM_BUG_ON(!i915_active_is_idle(&cl->active)); - call_rcu(&cl->rcu, __rcu_cacheline_free); -} - __i915_active_call -static void __cacheline_retire(struct i915_active *active) +static void __timeline_retire(struct i915_active *active) { - struct intel_timeline_cacheline *cl = - container_of(active, typeof(*cl), active); + struct intel_timeline *tl = + container_of(active, typeof(*tl), active);

- i915_vma_unpin(cl->hwsp->vma); - if (ptr_test_bit(cl->vaddr, CACHELINE_FREE)) - __idle_cacheline_free(cl); + i915_vma_unpin(tl->hwsp_ggtt); + intel_timeline_put(tl); }

-static int __cacheline_active(struct i915_active *active) +static int __timeline_active(struct i915_active *active) { - struct intel_timeline_cacheline *cl = - container_of(active, typeof(*cl), active); + struct intel_timeline *tl = + container_of(active, typeof(*tl), active);

- __i915_vma_pin(cl->hwsp->vma); + __i915_vma_pin(tl->hwsp_ggtt); + intel_timeline_get(tl); return 0; }

-static struct intel_timeline_cacheline * -cacheline_alloc(struct intel_timeline_hwsp *hwsp, unsigned int cacheline) -{ - struct intel_timeline_cacheline *cl; - void *vaddr; - - GEM_BUG_ON(cacheline >= BIT(CACHELINE_BITS)); - - cl = kmalloc(sizeof(*cl), GFP_KERNEL); - if (!cl) - return ERR_PTR(-ENOMEM); - - vaddr = i915_gem_object_pin_map(hwsp->vma->obj, I915_MAP_WB); - if (IS_ERR(vaddr)) { - kfree(cl); - return ERR_CAST(vaddr); - } - - cl->hwsp = hwsp; - cl->vaddr = page_pack_bits(vaddr, cacheline); - - i915_active_init(&cl->active, __cacheline_active, __cacheline_retire); - - return cl; -} - -static void cacheline_acquire(struct intel_timeline_cacheline *cl, - u32 ggtt_offset) -{ - if (!cl) - return; - - cl->ggtt_offset = ggtt_offset; - i915_active_acquire(&cl->active); -} - -static void cacheline_release(struct intel_timeline_cacheline *cl) -{ - if (cl) - i915_active_release(&cl->active); -} - -static void cacheline_free(struct intel_timeline_cacheline *cl) -{ - if (!i915_active_acquire_if_busy(&cl->active)) { - __idle_cacheline_free(cl); - return; - } - - GEM_BUG_ON(ptr_test_bit(cl->vaddr, CACHELINE_FREE)); - cl->vaddr = ptr_set_bit(cl->vaddr, CACHELINE_FREE); - - i915_active_release(&cl->active); -} - static int intel_timeline_init(struct intel_timeline *timeline, struct intel_gt *gt, struct i915_vma *hwsp, unsigned int offset) { void *vaddr; + u32 *seqno;

kref_init(&timeline->kref); atomic_set(&timeline->pin_count, 0);

timeline->gt = gt;

- timeline->has_initial_breadcrumb = !hwsp; - timeline->hwsp_cacheline = NULL; - - if (!hwsp) { - struct intel_timeline_cacheline *cl; - unsigned int cacheline; - - hwsp = hwsp_alloc(timeline, &cacheline); + if (hwsp) { + timeline->hwsp_offset = offset; + timeline->hwsp_ggtt = i915_vma_get(hwsp); + } else { + timeline->has_initial_breadcrumb = true; + hwsp = hwsp_alloc(gt); if (IS_ERR(hwsp)) return PTR_ERR(hwsp); - - cl = cacheline_alloc(hwsp->private, cacheline); - if (IS_ERR(cl)) { - __idle_hwsp_free(hwsp->private, cacheline); - return PTR_ERR(cl); - } - - timeline->hwsp_cacheline = cl; - timeline->hwsp_offset = cacheline * CACHELINE_BYTES; - - vaddr = page_mask_bits(cl->vaddr); - } else { - timeline->hwsp_offset = offset; - vaddr = i915_gem_object_pin_map(hwsp->obj, I915_MAP_WB); - if (IS_ERR(vaddr)) - return PTR_ERR(vaddr); + timeline->hwsp_ggtt = hwsp; }

- timeline->hwsp_seqno = - memset(vaddr + timeline->hwsp_offset, 0, CACHELINE_BYTES); + vaddr = i915_gem_object_pin_map(hwsp->obj, I915_MAP_WB); + if (IS_ERR(vaddr)) + return PTR_ERR(vaddr); + + timeline->hwsp_map = vaddr; + seqno = vaddr + timeline->hwsp_offset; + WRITE_ONCE(*seqno, 0); + timeline->hwsp_seqno = seqno;

- timeline->hwsp_ggtt = i915_vma_get(hwsp); GEM_BUG_ON(timeline->hwsp_offset >= hwsp->size);

timeline->fence_context = dma_fence_context_alloc(1); @@ -268,6 +95,7 @@ static int intel_timeline_init(struct intel_timeline *timeline, INIT_LIST_HEAD(&timeline->requests);

i915_syncmap_init(&timeline->sync); + i915_active_init(&timeline->active, __timeline_active, __timeline_retire);

return 0; } @@ -278,23 +106,18 @@ void intel_gt_init_timelines(struct intel_gt *gt)

spin_lock_init(&timelines->lock); INIT_LIST_HEAD(&timelines->active_list); - - spin_lock_init(&timelines->hwsp_lock); - INIT_LIST_HEAD(&timelines->hwsp_free_list); }

-static void intel_timeline_fini(struct intel_timeline *timeline) +static void intel_timeline_fini(struct rcu_head *rcu) { - GEM_BUG_ON(atomic_read(&timeline->pin_count)); - GEM_BUG_ON(!list_empty(&timeline->requests)); - GEM_BUG_ON(timeline->retire); + struct intel_timeline *timeline = + container_of(rcu, struct intel_timeline, rcu);

- if (timeline->hwsp_cacheline) - cacheline_free(timeline->hwsp_cacheline); - else - i915_gem_object_unpin_map(timeline->hwsp_ggtt->obj); + i915_gem_object_unpin_map(timeline->hwsp_ggtt->obj);

i915_vma_put(timeline->hwsp_ggtt); + i915_active_fini(&timeline->active); + kfree(timeline); }

struct intel_timeline * @@ -360,9 +183,9 @@ int intel_timeline_pin(struct intel_timeline *tl, struct i915_gem_ww_ctx *ww) GT_TRACE(tl->gt, "timeline:%llx using HWSP offset:%x\n", tl->fence_context, tl->hwsp_offset);

- cacheline_acquire(tl->hwsp_cacheline, tl->hwsp_offset); + i915_active_acquire(&tl->active); if (atomic_fetch_inc(&tl->pin_count)) { - cacheline_release(tl->hwsp_cacheline); + i915_active_release(&tl->active); __i915_vma_unpin(tl->hwsp_ggtt); }

@@ -371,9 +194,13 @@ int intel_timeline_pin(struct intel_timeline *tl, struct i915_gem_ww_ctx *ww)

void intel_timeline_reset_seqno(const struct intel_timeline *tl) { + u32 *hwsp_seqno = (u32 *)tl->hwsp_seqno; /* Must be pinned to be writable, and no requests in flight. */ GEM_BUG_ON(!atomic_read(&tl->pin_count)); - WRITE_ONCE(*(u32 *)tl->hwsp_seqno, tl->seqno); + + memset(hwsp_seqno + 1, 0, TIMELINE_SEQNO_BYTES - sizeof(*hwsp_seqno)); + WRITE_ONCE(*hwsp_seqno, tl->seqno); + clflush(hwsp_seqno); }

void intel_timeline_enter(struct intel_timeline *tl) @@ -449,106 +276,23 @@ static u32 timeline_advance(struct intel_timeline *tl) return tl->seqno += 1 + tl->has_initial_breadcrumb; }

-static void timeline_rollback(struct intel_timeline *tl) -{ - tl->seqno -= 1 + tl->has_initial_breadcrumb; -} - static noinline int __intel_timeline_get_seqno(struct intel_timeline *tl, - struct i915_request *rq, u32 *seqno) { - struct intel_timeline_cacheline *cl; - unsigned int cacheline; - struct i915_vma *vma; - void *vaddr; - int err; - - might_lock(&tl->gt->ggtt->vm.mutex); - GT_TRACE(tl->gt, "timeline:%llx wrapped\n", tl->fence_context); - - /* - * If there is an outstanding GPU reference to this cacheline, - * such as it being sampled by a HW semaphore on another timeline, - * we cannot wraparound our seqno value (the HW semaphore does - * a strict greater-than-or-equals compare, not i915_seqno_passed). - * So if the cacheline is still busy, we must detach ourselves - * from it and leave it inflight alongside its users. - * - * However, if nobody is watching and we can guarantee that nobody - * will, we could simply reuse the same cacheline. - * - * if (i915_active_request_is_signaled(&tl->last_request) && - * i915_active_is_signaled(&tl->hwsp_cacheline->active)) - * return 0; - * - * That seems unlikely for a busy timeline that needed to wrap in - * the first place, so just replace the cacheline. - */ - - vma = hwsp_alloc(tl, &cacheline); - if (IS_ERR(vma)) { - err = PTR_ERR(vma); - goto err_rollback; - } - - err = i915_ggtt_pin(vma, NULL, 0, PIN_HIGH); - if (err) { - __idle_hwsp_free(vma->private, cacheline); - goto err_rollback; - } + u32 next_ofs = offset_in_page(tl->hwsp_offset + TIMELINE_SEQNO_BYTES);

- cl = cacheline_alloc(vma->private, cacheline); - if (IS_ERR(cl)) { - err = PTR_ERR(cl); - __idle_hwsp_free(vma->private, cacheline); - goto err_unpin; - } - GEM_BUG_ON(cl->hwsp->vma != vma); - - /* - * Attach the old cacheline to the current request, so that we only - * free it after the current request is retired, which ensures that - * all writes into the cacheline from previous requests are complete. - */ - err = i915_active_ref(&tl->hwsp_cacheline->active, - tl->fence_context, - &rq->fence); - if (err) - goto err_cacheline; + /* w/a: bit 5 needs to be zero for MI_FLUSH_DW address. */ + if (TIMELINE_SEQNO_BYTES <= BIT(5) && (next_ofs & BIT(5))) + next_ofs = offset_in_page(next_ofs + BIT(5));

- cacheline_release(tl->hwsp_cacheline); /* ownership now xfered to rq */ - cacheline_free(tl->hwsp_cacheline); - - i915_vma_unpin(tl->hwsp_ggtt); /* binding kept alive by old cacheline */ - i915_vma_put(tl->hwsp_ggtt); - - tl->hwsp_ggtt = i915_vma_get(vma); - - vaddr = page_mask_bits(cl->vaddr); - tl->hwsp_offset = cacheline * CACHELINE_BYTES; - tl->hwsp_seqno = - memset(vaddr + tl->hwsp_offset, 0, CACHELINE_BYTES); - - tl->hwsp_offset += i915_ggtt_offset(vma); - GT_TRACE(tl->gt, "timeline:%llx using HWSP offset:%x\n", - tl->fence_context, tl->hwsp_offset); - - cacheline_acquire(cl, tl->hwsp_offset); - tl->hwsp_cacheline = cl; + tl->hwsp_offset = i915_ggtt_offset(tl->hwsp_ggtt) + next_ofs; + tl->hwsp_seqno = tl->hwsp_map + next_ofs; + intel_timeline_reset_seqno(tl);

*seqno = timeline_advance(tl); GEM_BUG_ON(i915_seqno_passed(*tl->hwsp_seqno, *seqno)); return 0; - -err_cacheline: - cacheline_free(cl); -err_unpin: - i915_vma_unpin(vma); -err_rollback: - timeline_rollback(tl); - return err; }

int intel_timeline_get_seqno(struct intel_timeline *tl, @@ -558,51 +302,52 @@ int intel_timeline_get_seqno(struct intel_timeline *tl, *seqno = timeline_advance(tl);

/* Replace the HWSP on wraparound for HW semaphores */ - if (unlikely(!*seqno && tl->hwsp_cacheline)) - return __intel_timeline_get_seqno(tl, rq, seqno); + if (unlikely(!*seqno && tl->has_initial_breadcrumb)) + return __intel_timeline_get_seqno(tl, seqno);

return 0; }

-static int cacheline_ref(struct intel_timeline_cacheline *cl, - struct i915_request *rq) -{ - return i915_active_add_request(&cl->active, rq); -} - int intel_timeline_read_hwsp(struct i915_request *from, struct i915_request *to, u32 *hwsp) { - struct intel_timeline_cacheline *cl; + struct intel_timeline *tl; int err;

- GEM_BUG_ON(!rcu_access_pointer(from->hwsp_cacheline)); - rcu_read_lock(); - cl = rcu_dereference(from->hwsp_cacheline); - if (i915_request_signaled(from)) /* confirm cacheline is valid */ - goto unlock; - if (unlikely(!i915_active_acquire_if_busy(&cl->active))) - goto unlock; /* seqno wrapped and completed! */ - if (unlikely(__i915_request_is_complete(from))) - goto release; + tl = rcu_dereference(from->timeline); + if (i915_request_signaled(from) || + !i915_active_acquire_if_busy(&tl->active)) + tl = NULL; + + if (tl) { + /* hwsp_offset may wraparound, so use from->hwsp_seqno */ + *hwsp = i915_ggtt_offset(tl->hwsp_ggtt) + + offset_in_page(from->hwsp_seqno); + } + + /* ensure we wait on the right request, if not, we completed */ + if (tl && __i915_request_is_complete(from)) { + i915_active_release(&tl->active); + tl = NULL; + } rcu_read_unlock();

- err = cacheline_ref(cl, to); - if (err) + if (!tl) + return 1; + + /* Can't do semaphore waits on kernel context */ + if (!tl->has_initial_breadcrumb) { + err = -EINVAL; goto out; + } + + err = i915_active_add_request(&tl->active, to);

- *hwsp = cl->ggtt_offset; out: - i915_active_release(&cl->active); + i915_active_release(&tl->active); return err; - -release: - i915_active_release(&cl->active); -unlock: - rcu_read_unlock(); - return 1; }

void intel_timeline_unpin(struct intel_timeline *tl) @@ -611,8 +356,7 @@ void intel_timeline_unpin(struct intel_timeline *tl) if (!atomic_dec_and_test(&tl->pin_count)) return;

- cacheline_release(tl->hwsp_cacheline); - + i915_active_release(&tl->active); __i915_vma_unpin(tl->hwsp_ggtt); }

@@ -621,8 +365,11 @@ void __intel_timeline_free(struct kref *kref) struct intel_timeline *timeline = container_of(kref, typeof(*timeline), kref);

- intel_timeline_fini(timeline); - kfree_rcu(timeline, rcu); + GEM_BUG_ON(atomic_read(&timeline->pin_count)); + GEM_BUG_ON(!list_empty(&timeline->requests)); + GEM_BUG_ON(timeline->retire); + + call_rcu(&timeline->rcu, intel_timeline_fini); }

void intel_gt_fini_timelines(struct intel_gt *gt) @@ -630,7 +377,6 @@ void intel_gt_fini_timelines(struct intel_gt *gt) struct intel_gt_timelines *timelines = &gt->timelines;

GEM_BUG_ON(!list_empty(&timelines->active_list)); - GEM_BUG_ON(!list_empty(&timelines->hwsp_free_list)); }

void intel_gt_show_timelines(struct intel_gt *gt, diff --git a/drivers/gpu/drm/i915/gt/intel_timeline_types.h b/drivers/gpu/drm/i915/gt/intel_timeline_types.h index 9f677c9b7d06..74e67dbf89c5 100644 --- a/drivers/gpu/drm/i915/gt/intel_timeline_types.h +++ b/drivers/gpu/drm/i915/gt/intel_timeline_types.h @@ -17,7 +17,6 @@ struct i915_vma; struct i915_syncmap; struct intel_gt; -struct intel_timeline_hwsp;

struct intel_timeline { u64 fence_context; @@ -44,12 +43,11 @@ struct intel_timeline { atomic_t pin_count; atomic_t active_count;

+ void *hwsp_map; const u32 *hwsp_seqno; struct i915_vma *hwsp_ggtt; u32 hwsp_offset;

- struct intel_timeline_cacheline *hwsp_cacheline; - bool has_initial_breadcrumb;

/** @@ -66,6 +64,8 @@ struct intel_timeline { */ struct i915_active_fence last_request;

+ struct i915_active active; + /** A chain of completed timelines ready for early retirement. */ struct intel_timeline *retire;

@@ -89,15 +89,4 @@ struct intel_timeline { struct rcu_head rcu; };

-struct intel_timeline_cacheline { - struct i915_active active; - - struct intel_timeline_hwsp *hwsp; - void *vaddr; - - u32 ggtt_offset; - - struct rcu_head rcu; -}; - #endif /* __I915_TIMELINE_TYPES_H__ */ diff --git a/drivers/gpu/drm/i915/gt/selftest_engine_cs.c b/drivers/gpu/drm/i915/gt/selftest_engine_cs.c index 84d883de30ee..7e466ae114f8 100644 --- a/drivers/gpu/drm/i915/gt/selftest_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/selftest_engine_cs.c @@ -41,6 +41,9 @@ static int perf_end(struct intel_gt *gt)

static int write_timestamp(struct i915_request *rq, int slot) { + struct intel_timeline *tl = + rcu_dereference_protected(rq->timeline, + !i915_request_signaled(rq)); u32 cmd; u32 *cs;

@@ -53,7 +56,7 @@ static int write_timestamp(struct i915_request *rq, int slot) cmd++; *cs++ = cmd; *cs++ = i915_mmio_reg_offset(RING_TIMESTAMP(rq->engine->mmio_base)); - *cs++ = i915_request_timeline(rq)->hwsp_offset + slot * sizeof(u32); + *cs++ = tl->hwsp_offset + slot * sizeof(u32); *cs++ = 0;

intel_ring_advance(rq, cs); diff --git a/drivers/gpu/drm/i915/gt/selftest_timeline.c b/drivers/gpu/drm/i915/gt/selftest_timeline.c index d283dce5b4ac..be46465a57c9 100644 --- a/drivers/gpu/drm/i915/gt/selftest_timeline.c +++ b/drivers/gpu/drm/i915/gt/selftest_timeline.c @@ -34,10 +34,11 @@ static unsigned long hwsp_cacheline(struct intel_timeline *tl) { unsigned long address = (unsigned long)page_address(hwsp_page(tl));

- return (address + tl->hwsp_offset) / CACHELINE_BYTES; + return (address + offset_in_page(tl->hwsp_offset)) / TIMELINE_SEQNO_BYTES; }

-#define CACHELINES_PER_PAGE (PAGE_SIZE / CACHELINE_BYTES) +/* Only half of seqno's are usable, see __intel_timeline_get_seqno() */ +#define CACHELINES_PER_PAGE (PAGE_SIZE / TIMELINE_SEQNO_BYTES / 2)

struct mock_hwsp_freelist { struct intel_gt *gt; @@ -665,7 +666,7 @@ static int live_hwsp_wrap(void *arg) if (IS_ERR(tl)) return PTR_ERR(tl);

- if (!tl->has_initial_breadcrumb || !tl->hwsp_cacheline) + if (!tl->has_initial_breadcrumb) goto out_free;

err = intel_timeline_pin(tl, NULL); @@ -832,12 +833,26 @@ static int setup_watcher(struct hwsp_watcher *w, struct intel_gt *gt) return 0; }

+static void switch_tl_lock(struct i915_request *from, struct i915_request *to) +{ + /* some light mutex juggling required; think co-routines */ + + if (from) { + lockdep_unpin_lock(&from->context->timeline->mutex, from->cookie); + mutex_unlock(&from->context->timeline->mutex); + } + + if (to) { + mutex_lock(&to->context->timeline->mutex); + to->cookie = lockdep_pin_lock(&to->context->timeline->mutex); + } +} + static int create_watcher(struct hwsp_watcher *w, struct intel_engine_cs *engine, int ringsz) { struct intel_context *ce; - struct intel_timeline *tl;

ce = intel_context_create(engine); if (IS_ERR(ce)) @@ -850,11 +865,8 @@ static int create_watcher(struct hwsp_watcher *w, return PTR_ERR(w->rq);

w->addr = i915_ggtt_offset(w->vma); - tl = w->rq->context->timeline;

- /* some light mutex juggling required; think co-routines */ - lockdep_unpin_lock(&tl->mutex, w->rq->cookie); - mutex_unlock(&tl->mutex); + switch_tl_lock(w->rq, NULL);

return 0; } @@ -863,15 +875,13 @@ static int check_watcher(struct hwsp_watcher *w, const char *name, bool (*op)(u32 hwsp, u32 seqno)) { struct i915_request *rq = fetch_and_zero(&w->rq); - struct intel_timeline *tl = rq->context->timeline; u32 offset, end; int err;

GEM_BUG_ON(w->addr - i915_ggtt_offset(w->vma) > w->vma->size);

i915_request_get(rq); - mutex_lock(&tl->mutex); - rq->cookie = lockdep_pin_lock(&tl->mutex); + switch_tl_lock(NULL, rq); i915_request_add(rq);

if (i915_request_wait(rq, 0, HZ) < 0) { @@ -900,10 +910,7 @@ static int check_watcher(struct hwsp_watcher *w, const char *name, static void cleanup_watcher(struct hwsp_watcher *w) { if (w->rq) { - struct intel_timeline *tl = w->rq->context->timeline; - - mutex_lock(&tl->mutex); - w->rq->cookie = lockdep_pin_lock(&tl->mutex); + switch_tl_lock(NULL, w->rq);

i915_request_add(w->rq); } @@ -941,7 +948,7 @@ static struct i915_request *wrap_timeline(struct i915_request *rq) }

i915_request_put(rq); - rq = intel_context_create_request(ce); + rq = i915_request_create(ce); if (IS_ERR(rq)) return rq;

@@ -976,7 +983,7 @@ static int live_hwsp_read(void *arg) if (IS_ERR(tl)) return PTR_ERR(tl);

- if (!tl->hwsp_cacheline) + if (!tl->has_initial_breadcrumb) goto out_free;

for (i = 0; i < ARRAY_SIZE(watcher); i++) { @@ -998,7 +1005,7 @@ static int live_hwsp_read(void *arg) do { struct i915_sw_fence *submit; struct i915_request *rq; - u32 hwsp; + u32 hwsp, dummy;

submit = heap_fence_create(GFP_KERNEL); if (!submit) { @@ -1016,14 +1023,26 @@ static int live_hwsp_read(void *arg) goto out; }

- /* Skip to the end, saving 30 minutes of nops */ - tl->seqno = -10u + 2 * (count & 3); - WRITE_ONCE(*(u32 *)tl->hwsp_seqno, tl->seqno); ce->timeline = intel_timeline_get(tl);

- rq = intel_context_create_request(ce); + /* Ensure timeline is mapped, done during first pin */ + err = intel_context_pin(ce); + if (err) { + intel_context_put(ce); + goto out; + } + + /* + * Start at a new wrap, and set seqno right before another wrap, + * saving 30 minutes of nops + */ + tl->seqno = -12u + 2 * (count & 3); + __intel_timeline_get_seqno(tl, &dummy); + + rq = i915_request_create(ce); if (IS_ERR(rq)) { err = PTR_ERR(rq); + intel_context_unpin(ce); intel_context_put(ce); goto out; } @@ -1033,32 +1052,35 @@ static int live_hwsp_read(void *arg) GFP_KERNEL); if (err < 0) { i915_request_add(rq); + intel_context_unpin(ce); intel_context_put(ce); goto out; }

- mutex_lock(&watcher[0].rq->context->timeline->mutex); + switch_tl_lock(rq, watcher[0].rq); err = intel_timeline_read_hwsp(rq, watcher[0].rq, &hwsp); if (err == 0) err = emit_read_hwsp(watcher[0].rq, /* before */ rq->fence.seqno, hwsp, &watcher[0].addr); - mutex_unlock(&watcher[0].rq->context->timeline->mutex); + switch_tl_lock(watcher[0].rq, rq); if (err) { i915_request_add(rq); + intel_context_unpin(ce); intel_context_put(ce); goto out; }

- mutex_lock(&watcher[1].rq->context->timeline->mutex); + switch_tl_lock(rq, watcher[1].rq); err = intel_timeline_read_hwsp(rq, watcher[1].rq, &hwsp); if (err == 0) err = emit_read_hwsp(watcher[1].rq, /* after */ rq->fence.seqno, hwsp, &watcher[1].addr); - mutex_unlock(&watcher[1].rq->context->timeline->mutex); + switch_tl_lock(watcher[1].rq, rq); if (err) { i915_request_add(rq); + intel_context_unpin(ce); intel_context_put(ce); goto out; } @@ -1067,6 +1089,7 @@ static int live_hwsp_read(void *arg) i915_request_add(rq);

rq = wrap_timeline(rq); + intel_context_unpin(ce); intel_context_put(ce); if (IS_ERR(rq)) { err = PTR_ERR(rq); @@ -1106,8 +1129,8 @@ static int live_hwsp_read(void *arg) 3 * watcher[1].rq->ring->size) break;

- } while (!__igt_timeout(end_time, NULL)); - WRITE_ONCE(*(u32 *)tl->hwsp_seqno, 0xdeadbeef); + } while (!__igt_timeout(end_time, NULL) && + count < (PAGE_SIZE / TIMELINE_SEQNO_BYTES - 1) / 2);

pr_info("%s: simulated %lu wraps\n", engine->name, count); err = check_watcher(&watcher[1], "after", cmp_gte); @@ -1152,9 +1175,7 @@ static int live_hwsp_rollover_kernel(void *arg) }

GEM_BUG_ON(i915_active_fence_isset(&tl->last_request)); - tl->seqno = 0; - timeline_rollback(tl); - timeline_rollback(tl); + tl->seqno = -2u; WRITE_ONCE(*(u32 *)tl->hwsp_seqno, tl->seqno);

for (i = 0; i < ARRAY_SIZE(rq); i++) { @@ -1234,11 +1255,10 @@ static int live_hwsp_rollover_user(void *arg) goto out;

tl = ce->timeline; - if (!tl->has_initial_breadcrumb || !tl->hwsp_cacheline) + if (!tl->has_initial_breadcrumb) goto out;

- timeline_rollback(tl); - timeline_rollback(tl); + tl->seqno = -4u; WRITE_ONCE(*(u32 *)tl->hwsp_seqno, tl->seqno);

for (i = 0; i < ARRAY_SIZE(rq); i++) { diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c index e7b4c4bc41a6..59d942910558 100644 --- a/drivers/gpu/drm/i915/i915_request.c +++ b/drivers/gpu/drm/i915/i915_request.c @@ -794,7 +794,6 @@ __i915_request_create(struct intel_context *ce, gfp_t gfp) rq->fence.seqno = seqno;

RCU_INIT_POINTER(rq->timeline, tl); - RCU_INIT_POINTER(rq->hwsp_cacheline, tl->hwsp_cacheline); rq->hwsp_seqno = tl->hwsp_seqno; GEM_BUG_ON(__i915_request_is_complete(rq));

@@ -1039,9 +1038,6 @@ emit_semaphore_wait(struct i915_request *to, if (i915_request_has_initial_breadcrumb(to)) goto await_fence;

- if (!rcu_access_pointer(from->hwsp_cacheline)) - goto await_fence; - /* * If this or its dependents are waiting on an external fence * that may fail catastrophically, then we want to avoid using diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h index dd10a6db3d21..38062495b66f 100644 --- a/drivers/gpu/drm/i915/i915_request.h +++ b/drivers/gpu/drm/i915/i915_request.h @@ -239,16 +239,6 @@ struct i915_request { */ const u32 *hwsp_seqno;

- /* - * If we need to access the timeline's seqno for this request in - * another request, we need to keep a read reference to this associated - * cacheline, so that we do not free and recycle it before the foreign - * observers have completed. Hence, we keep a pointer to the cacheline - * inside the timeline's HWSP vma, but it is only valid while this - * request has not completed and guarded by the timeline mutex. - */ - struct intel_timeline_cacheline __rcu *hwsp_cacheline; - /** Position in the ring of the start of the request */ u32 head;

@@ -650,4 +640,25 @@ static inline bool i915_request_use_semaphores(const struct i915_request *rq) return intel_engine_has_semaphores(rq->engine); }

+static inline u32 +i915_request_active_seqno(const struct i915_request *rq) +{ + u32 hwsp_phys_base = + page_mask_bits(i915_request_active_timeline(rq)->hwsp_offset); + u32 hwsp_relative_offset = offset_in_page(rq->hwsp_seqno); + + /* + * Because of wraparound, we cannot simply take tl->hwsp_offset, + * but instead use the fact that the relative for vaddr is the + * offset as for hwsp_offset. Take the top bits from tl->hwsp_offset + * and combine them with the relative offset in rq->hwsp_seqno. + * + * As rw->hwsp_seqno is rewritten when signaled, this only works + * when the request isn't signaled yet, but at that point you + * no longer need the offset. + */ + + return hwsp_phys_base + hwsp_relative_offset; +} + #endif /* I915_REQUEST_H */

We need to get rid of allocations in the cmd parser, because it needs to be called from a signaling context, first move all pinning to execbuf, where we already hold all locks.

Allocate jump_whitelist in the execbuffer, and add annotations around intel_engine_cmd_parser(), to ensure we only call the command parser without allocating any memory, or taking any locks we're not supposed to.

Because i915_gem_object_get_page() may also allocate memory, add a path to i915_gem_object_get_sg() that prevents memory allocations, and walk the sg list manually. It should be similarly fast.

This has the added benefit of being able to catch all memory allocation errors before the point of no return, and return -ENOMEM safely to the execbuf submitter.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Acked-by: Thomas Hellström thomas.hellstrom@linux.intel.com --- .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 74 ++++++++++++- drivers/gpu/drm/i915/gem/i915_gem_object.h | 10 +- drivers/gpu/drm/i915/gem/i915_gem_pages.c | 21 +++- drivers/gpu/drm/i915/gt/intel_ggtt.c | 2 +- drivers/gpu/drm/i915/i915_cmd_parser.c | 104 ++++++++---------- drivers/gpu/drm/i915/i915_drv.h | 7 +- drivers/gpu/drm/i915/i915_memcpy.c | 2 +- drivers/gpu/drm/i915/i915_memcpy.h | 2 +- 8 files changed, 142 insertions(+), 80 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index c082fb0bef33..309d661c4c71 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -28,6 +28,7 @@ #include "i915_sw_fence_work.h" #include "i915_trace.h" #include "i915_user_extensions.h" +#include "i915_memcpy.h"

struct eb_vma { struct i915_vma *vma; @@ -2274,24 +2275,45 @@ struct eb_parse_work { struct i915_vma *trampoline; unsigned long batch_offset; unsigned long batch_length; + unsigned long *jump_whitelist; + const void *batch_map; + void *shadow_map; };

static int __eb_parse(struct dma_fence_work *work) { struct eb_parse_work *pw = container_of(work, typeof(*pw), base); + int ret; + bool cookie;

- return intel_engine_cmd_parser(pw->engine, - pw->batch, - pw->batch_offset, - pw->batch_length, - pw->shadow, - pw->trampoline); + cookie = dma_fence_begin_signalling(); + ret = intel_engine_cmd_parser(pw->engine, + pw->batch, + pw->batch_offset, + pw->batch_length, + pw->shadow, + pw->jump_whitelist, + pw->shadow_map, + pw->batch_map); + dma_fence_end_signalling(cookie); + + return ret; }

static void __eb_parse_release(struct dma_fence_work *work) { struct eb_parse_work *pw = container_of(work, typeof(*pw), base);

+ if (!IS_ERR_OR_NULL(pw->jump_whitelist)) + kfree(pw->jump_whitelist); + + if (pw->batch_map) + i915_gem_object_unpin_map(pw->batch->obj); + else + i915_gem_object_unpin_pages(pw->batch->obj); + + i915_gem_object_unpin_map(pw->shadow->obj); + if (pw->trampoline) i915_active_release(&pw->trampoline->active); i915_active_release(&pw->shadow->active); @@ -2341,6 +2363,8 @@ static int eb_parse_pipeline(struct i915_execbuffer *eb, struct i915_vma *trampoline) { struct eb_parse_work *pw; + struct drm_i915_gem_object *batch = eb->batch->vma->obj; + bool needs_clflush; int err;

GEM_BUG_ON(overflows_type(eb->batch_start_offset, pw->batch_offset)); @@ -2364,6 +2388,34 @@ static int eb_parse_pipeline(struct i915_execbuffer *eb, goto err_shadow; }

+ pw->shadow_map = i915_gem_object_pin_map(shadow->obj, I915_MAP_WB); + if (IS_ERR(pw->shadow_map)) { + err = PTR_ERR(pw->shadow_map); + goto err_trampoline; + } + + needs_clflush = + !(batch->cache_coherent & I915_BO_CACHE_COHERENT_FOR_READ); + + pw->batch_map = ERR_PTR(-ENODEV); + if (needs_clflush && i915_has_memcpy_from_wc()) + pw->batch_map = i915_gem_object_pin_map(batch, I915_MAP_WC); + + if (IS_ERR(pw->batch_map)) { + err = i915_gem_object_pin_pages(batch); + if (err) + goto err_unmap_shadow; + pw->batch_map = NULL; + } + + pw->jump_whitelist = + intel_engine_cmd_parser_alloc_jump_whitelist(eb->batch_len, + trampoline); + if (IS_ERR(pw->jump_whitelist)) { + err = PTR_ERR(pw->jump_whitelist); + goto err_unmap_batch; + } + dma_fence_work_init(&pw->base, &eb_parse_ops);

pw->engine = eb->engine; @@ -2403,6 +2455,16 @@ static int eb_parse_pipeline(struct i915_execbuffer *eb, dma_fence_work_commit_imm(&pw->base); return err;

+err_unmap_batch: + if (pw->batch_map) + i915_gem_object_unpin_map(batch); + else + i915_gem_object_unpin_pages(batch); +err_unmap_shadow: + i915_gem_object_unpin_map(shadow->obj); +err_trampoline: + if (trampoline) + i915_active_release(&trampoline->active); err_shadow: i915_active_release(&shadow->active); err_batch: diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h index 366d23afbb1a..dc949404843a 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h @@ -325,22 +325,22 @@ struct scatterlist * __i915_gem_object_get_sg(struct drm_i915_gem_object *obj, struct i915_gem_object_page_iter *iter, unsigned int n, - unsigned int *offset); + unsigned int *offset, bool allow_alloc);

static inline struct scatterlist * i915_gem_object_get_sg(struct drm_i915_gem_object *obj, unsigned int n, - unsigned int *offset) + unsigned int *offset, bool allow_alloc) { - return __i915_gem_object_get_sg(obj, &obj->mm.get_page, n, offset); + return __i915_gem_object_get_sg(obj, &obj->mm.get_page, n, offset, allow_alloc); }

static inline struct scatterlist * i915_gem_object_get_sg_dma(struct drm_i915_gem_object *obj, unsigned int n, - unsigned int *offset) + unsigned int *offset, bool allow_alloc) { - return __i915_gem_object_get_sg(obj, &obj->mm.get_dma_page, n, offset); + return __i915_gem_object_get_sg(obj, &obj->mm.get_dma_page, n, offset, allow_alloc); }

struct page * diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c index 43028f3539a6..d44b72dd13fe 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c @@ -448,7 +448,8 @@ struct scatterlist * __i915_gem_object_get_sg(struct drm_i915_gem_object *obj, struct i915_gem_object_page_iter *iter, unsigned int n, - unsigned int *offset) + unsigned int *offset, + bool allow_alloc) { const bool dma = iter == &obj->mm.get_dma_page; struct scatterlist *sg; @@ -470,6 +471,9 @@ __i915_gem_object_get_sg(struct drm_i915_gem_object *obj, if (n < READ_ONCE(iter->sg_idx)) goto lookup;

+ if (!allow_alloc) + goto manual_lookup; + mutex_lock(&iter->lock);

/* We prefer to reuse the last sg so that repeated lookup of this @@ -519,7 +523,16 @@ __i915_gem_object_get_sg(struct drm_i915_gem_object *obj, if (unlikely(n < idx)) /* insertion completed by another thread */ goto lookup;

- /* In case we failed to insert the entry into the radixtree, we need + goto manual_walk; + +manual_lookup: + idx = 0; + sg = obj->mm.pages->sgl; + count = __sg_page_count(sg); + +manual_walk: + /* + * In case we failed to insert the entry into the radixtree, we need * to look beyond the current sg. */ while (idx + count <= n) { @@ -566,7 +579,7 @@ i915_gem_object_get_page(struct drm_i915_gem_object *obj, unsigned int n)

GEM_BUG_ON(!i915_gem_object_has_struct_page(obj));

- sg = i915_gem_object_get_sg(obj, n, &offset); + sg = i915_gem_object_get_sg(obj, n, &offset, true); return nth_page(sg_page(sg), offset); }

@@ -592,7 +605,7 @@ i915_gem_object_get_dma_address_len(struct drm_i915_gem_object *obj, struct scatterlist *sg; unsigned int offset;

- sg = i915_gem_object_get_sg_dma(obj, n, &offset); + sg = i915_gem_object_get_sg_dma(obj, n, &offset, true);

if (len) *len = sg_dma_len(sg) - (offset << PAGE_SHIFT); diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c index b0b8ded834f0..c8eb034c806a 100644 --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c @@ -1434,7 +1434,7 @@ intel_partial_pages(const struct i915_ggtt_view *view, if (ret) goto err_sg_alloc;

- iter = i915_gem_object_get_sg_dma(obj, view->partial.offset, &offset); + iter = i915_gem_object_get_sg_dma(obj, view->partial.offset, &offset, true); GEM_BUG_ON(!iter);

sg = st->sgl; diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c index 5f86f5b2caf6..e6f1e93abbbb 100644 --- a/drivers/gpu/drm/i915/i915_cmd_parser.c +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c @@ -1144,38 +1144,20 @@ find_reg(const struct intel_engine_cs *engine, u32 addr) /* Returns a vmap'd pointer to dst_obj, which the caller must unmap */ static u32 *copy_batch(struct drm_i915_gem_object *dst_obj, struct drm_i915_gem_object *src_obj, - unsigned long offset, unsigned long length) + unsigned long offset, unsigned long length, + void *dst, const void *src) { - bool needs_clflush; - void *dst, *src; - int ret; - - dst = i915_gem_object_pin_map(dst_obj, I915_MAP_WB); - if (IS_ERR(dst)) - return dst; - - ret = i915_gem_object_pin_pages(src_obj); - if (ret) { - i915_gem_object_unpin_map(dst_obj); - return ERR_PTR(ret); - } - - needs_clflush = + bool needs_clflush = !(src_obj->cache_coherent & I915_BO_CACHE_COHERENT_FOR_READ);

- src = ERR_PTR(-ENODEV); - if (needs_clflush && i915_has_memcpy_from_wc()) { - src = i915_gem_object_pin_map(src_obj, I915_MAP_WC); - if (!IS_ERR(src)) { - i915_unaligned_memcpy_from_wc(dst, - src + offset, - length); - i915_gem_object_unpin_map(src_obj); - } - } - if (IS_ERR(src)) { - unsigned long x, n, remain; + if (src) { + GEM_BUG_ON(!needs_clflush); + i915_unaligned_memcpy_from_wc(dst, src + offset, length); + } else { + struct scatterlist *sg; void *ptr; + unsigned int x, sg_ofs; + unsigned long remain;

/* * We can avoid clflushing partial cachelines before the write @@ -1192,23 +1174,31 @@ static u32 *copy_batch(struct drm_i915_gem_object *dst_obj,

ptr = dst; x = offset_in_page(offset); - for (n = offset >> PAGE_SHIFT; remain; n++) { - int len = min(remain, PAGE_SIZE - x); - - src = kmap_atomic(i915_gem_object_get_page(src_obj, n)); - if (needs_clflush) - drm_clflush_virt_range(src + x, len); - memcpy(ptr, src + x, len); - kunmap_atomic(src); - - ptr += len; - remain -= len; - x = 0; + sg = i915_gem_object_get_sg(src_obj, offset >> PAGE_SHIFT, &sg_ofs, false); + + while (remain) { + unsigned long sg_max = sg->length >> PAGE_SHIFT; + + for (; remain && sg_ofs < sg_max; sg_ofs++) { + unsigned long len = min(remain, PAGE_SIZE - x); + void *map; + + map = kmap_atomic(nth_page(sg_page(sg), sg_ofs)); + if (needs_clflush) + drm_clflush_virt_range(map + x, len); + memcpy(ptr, map + x, len); + kunmap_atomic(map); + + ptr += len; + remain -= len; + x = 0; + } + + sg_ofs = 0; + sg = sg_next(sg); } }

- i915_gem_object_unpin_pages(src_obj); - memset32(dst + length, 0, (dst_obj->base.size - length) / sizeof(u32));

/* dst_obj is returned with vmap pinned */ @@ -1370,9 +1360,6 @@ static int check_bbstart(u32 *cmd, u32 offset, u32 length, if (target_cmd_index == offset) return 0;

- if (IS_ERR(jump_whitelist)) - return PTR_ERR(jump_whitelist); - if (!test_bit(target_cmd_index, jump_whitelist)) { DRM_DEBUG("CMD: BB_START to 0x%llx not a previously executed cmd\n", jump_target); @@ -1382,10 +1369,14 @@ static int check_bbstart(u32 *cmd, u32 offset, u32 length, return 0; }

-static unsigned long *alloc_whitelist(u32 batch_length) +unsigned long *intel_engine_cmd_parser_alloc_jump_whitelist(u32 batch_length, + bool trampoline) { unsigned long *jmp;

+ if (trampoline) + return NULL; + /* * We expect batch_length to be less than 256KiB for known users, * i.e. we need at most an 8KiB bitmap allocation which should be @@ -1423,14 +1414,16 @@ int intel_engine_cmd_parser(struct intel_engine_cs *engine, unsigned long batch_offset, unsigned long batch_length, struct i915_vma *shadow, - bool trampoline) + unsigned long *jump_whitelist, + void *shadow_map, + const void *batch_map) { u32 *cmd, *batch_end, offset = 0; struct drm_i915_cmd_descriptor default_desc = noop_desc; const struct drm_i915_cmd_descriptor *desc = &default_desc; - unsigned long *jump_whitelist; u64 batch_addr, shadow_addr; int ret = 0; + bool trampoline = !jump_whitelist;

GEM_BUG_ON(!IS_ALIGNED(batch_offset, sizeof(*cmd))); GEM_BUG_ON(!IS_ALIGNED(batch_length, sizeof(*cmd))); @@ -1438,16 +1431,8 @@ int intel_engine_cmd_parser(struct intel_engine_cs *engine, batch->size)); GEM_BUG_ON(!batch_length);

- cmd = copy_batch(shadow->obj, batch->obj, batch_offset, batch_length); - if (IS_ERR(cmd)) { - DRM_DEBUG("CMD: Failed to copy batch\n"); - return PTR_ERR(cmd); - } - - jump_whitelist = NULL; - if (!trampoline) - /* Defer failure until attempted use */ - jump_whitelist = alloc_whitelist(batch_length); + cmd = copy_batch(shadow->obj, batch->obj, batch_offset, batch_length, + shadow_map, batch_map);

shadow_addr = gen8_canonical_addr(shadow->node.start); batch_addr = gen8_canonical_addr(batch->node.start + batch_offset); @@ -1548,9 +1533,6 @@ int intel_engine_cmd_parser(struct intel_engine_cs *engine,

i915_gem_object_flush_map(shadow->obj);

- if (!IS_ERR_OR_NULL(jump_whitelist)) - kfree(jump_whitelist); - i915_gem_object_unpin_map(shadow->obj); return ret; }

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 214fe10eb092..58ae4b76533f 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1938,12 +1938,17 @@ const char *i915_cache_level_str(struct drm_i915_private *i915, int type); int i915_cmd_parser_get_version(struct drm_i915_private *dev_priv); int intel_engine_init_cmd_parser(struct intel_engine_cs *engine); void intel_engine_cleanup_cmd_parser(struct intel_engine_cs *engine); +unsigned long *intel_engine_cmd_parser_alloc_jump_whitelist(u32 batch_length, + bool trampoline); + int intel_engine_cmd_parser(struct intel_engine_cs *engine, struct i915_vma *batch, unsigned long batch_offset, unsigned long batch_length, struct i915_vma *shadow, - bool trampoline); + unsigned long *jump_whitelist, + void *shadow_map, + const void *batch_map); #define I915_CMD_PARSER_TRAMPOLINE_SIZE 8

/* intel_device_info.c */ diff --git a/drivers/gpu/drm/i915/i915_memcpy.c b/drivers/gpu/drm/i915/i915_memcpy.c index 7b3b83bd5ab8..1b021a4902de 100644 --- a/drivers/gpu/drm/i915/i915_memcpy.c +++ b/drivers/gpu/drm/i915/i915_memcpy.c @@ -135,7 +135,7 @@ bool i915_memcpy_from_wc(void *dst, const void *src, unsigned long len) * accepts that its arguments may not be aligned, but are valid for the * potential 16-byte read past the end. */ -void i915_unaligned_memcpy_from_wc(void *dst, void *src, unsigned long len) +void i915_unaligned_memcpy_from_wc(void *dst, const void *src, unsigned long len) { unsigned long addr;

diff --git a/drivers/gpu/drm/i915/i915_memcpy.h b/drivers/gpu/drm/i915/i915_memcpy.h index e36d30edd987..3df063a3293b 100644 --- a/drivers/gpu/drm/i915/i915_memcpy.h +++ b/drivers/gpu/drm/i915/i915_memcpy.h @@ -13,7 +13,7 @@ struct drm_i915_private; void i915_memcpy_init_early(struct drm_i915_private *i915);

bool i915_memcpy_from_wc(void *dst, const void *src, unsigned long len); -void i915_unaligned_memcpy_from_wc(void *dst, void *src, unsigned long len); +void i915_unaligned_memcpy_from_wc(void *dst, const void *src, unsigned long len);

/* The movntdqa instructions used for memcpy-from-wc require 16-byte alignment, * as well as SSE4.1 support. i915_memcpy_from_wc() will report if it cannot

Currently we have a lot of places where we hold the gem object lock, but haven't yet been converted to the ww dance. Complain loudly about those places.

i915_vma_pin shouldn't have the obj lock held, so we can do a ww dance, while i915_vma_pin_ww should.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Reviewed-by: Thomas Hellström thomas.hellstrom@linux.intel.com #irc --- drivers/gpu/drm/i915/gt/intel_renderstate.c | 2 +- drivers/gpu/drm/i915/i915_vma.c | 11 ++++++++++- drivers/gpu/drm/i915/i915_vma.h | 3 +++ 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_renderstate.c b/drivers/gpu/drm/i915/gt/intel_renderstate.c index 0f7c0a148b80..b03e197b1d99 100644 --- a/drivers/gpu/drm/i915/gt/intel_renderstate.c +++ b/drivers/gpu/drm/i915/gt/intel_renderstate.c @@ -176,7 +176,7 @@ int intel_renderstate_init(struct intel_renderstate *so, if (err) goto err_context;

- err = i915_vma_pin(so->vma, 0, 0, PIN_GLOBAL | PIN_HIGH); + err = i915_vma_pin_ww(so->vma, &so->ww, 0, 0, PIN_GLOBAL | PIN_HIGH); if (err) goto err_context;

diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c index caa9b041616b..7310893086f7 100644 --- a/drivers/gpu/drm/i915/i915_vma.c +++ b/drivers/gpu/drm/i915/i915_vma.c @@ -865,6 +865,8 @@ int i915_vma_pin_ww(struct i915_vma *vma, struct i915_gem_ww_ctx *ww, #ifdef CONFIG_PROVE_LOCKING if (debug_locks && lockdep_is_held(&vma->vm->i915->drm.struct_mutex)) WARN_ON(!ww); + if (debug_locks && ww && vma->resv) + assert_vma_held(vma); #endif

BUILD_BUG_ON(PIN_GLOBAL != I915_VMA_GLOBAL_BIND); @@ -1020,8 +1022,15 @@ int i915_ggtt_pin(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,

GEM_BUG_ON(!i915_vma_is_ggtt(vma));

+#ifdef CONFIG_LOCKDEP + WARN_ON(!ww && vma->resv && dma_resv_held(vma->resv)); +#endif + do { - err = i915_vma_pin_ww(vma, ww, 0, align, flags | PIN_GLOBAL); + if (ww) + err = i915_vma_pin_ww(vma, ww, 0, align, flags | PIN_GLOBAL); + else + err = i915_vma_pin(vma, 0, align, flags | PIN_GLOBAL); if (err != -ENOSPC) { if (!err) { err = i915_vma_wait_for_bind(vma); diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h index a64adc8c883b..3c914c9de9a9 100644 --- a/drivers/gpu/drm/i915/i915_vma.h +++ b/drivers/gpu/drm/i915/i915_vma.h @@ -243,6 +243,9 @@ i915_vma_pin_ww(struct i915_vma *vma, struct i915_gem_ww_ctx *ww, static inline int __must_check i915_vma_pin(struct i915_vma *vma, u64 size, u64 alignment, u64 flags) { +#ifdef CONFIG_LOCKDEP + WARN_ON_ONCE(vma->resv && dma_resv_held(vma->resv)); +#endif return i915_vma_pin_ww(vma, NULL, size, alignment, flags); }

We want to remove the changing of ops structure for attaching phys pages, so we need to kill off HAS_STRUCT_PAGE from ops->flags, and put it in the bo.

This will remove a potential race of dereferencing the wrong obj->ops without ww mutex held.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Reviewed-by: Thomas Hellström thomas.hellstrom@linux.intel.com --- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 2 +- drivers/gpu/drm/i915/gem/i915_gem_internal.c | 6 +++--- drivers/gpu/drm/i915/gem/i915_gem_lmem.c | 4 ++-- drivers/gpu/drm/i915/gem/i915_gem_mman.c | 7 +++---- drivers/gpu/drm/i915/gem/i915_gem_object.c | 4 +++- drivers/gpu/drm/i915/gem/i915_gem_object.h | 5 +++-- drivers/gpu/drm/i915/gem/i915_gem_object_types.h | 10 ++++++---- drivers/gpu/drm/i915/gem/i915_gem_pages.c | 5 ++--- drivers/gpu/drm/i915/gem/i915_gem_phys.c | 2 ++ drivers/gpu/drm/i915/gem/i915_gem_region.c | 4 +--- drivers/gpu/drm/i915/gem/i915_gem_region.h | 3 +-- drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 8 ++++---- drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 4 ++-- drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 6 +++--- drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c | 4 ++-- drivers/gpu/drm/i915/gem/selftests/huge_pages.c | 10 +++++----- drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c | 11 ++++------- drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c | 12 ++++++++++++ drivers/gpu/drm/i915/gvt/dmabuf.c | 2 +- drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 2 +- drivers/gpu/drm/i915/selftests/mock_region.c | 4 ++-- 21 files changed, 63 insertions(+), 52 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c index d804b0003e0d..c7100a83b8ea 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c @@ -261,7 +261,7 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev, }

drm_gem_private_object_init(dev, &obj->base, dma_buf->size); - i915_gem_object_init(obj, &i915_gem_object_dmabuf_ops, &lock_class); + i915_gem_object_init(obj, &i915_gem_object_dmabuf_ops, &lock_class, 0); obj->base.import_attach = attach; obj->base.resv = dma_buf->resv;

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_internal.c b/drivers/gpu/drm/i915/gem/i915_gem_internal.c index ad22f42541bd..21cc40897ca8 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_internal.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_internal.c @@ -138,8 +138,7 @@ static void i915_gem_object_put_pages_internal(struct drm_i915_gem_object *obj,

static const struct drm_i915_gem_object_ops i915_gem_object_internal_ops = { .name = "i915_gem_object_internal", - .flags = I915_GEM_OBJECT_HAS_STRUCT_PAGE | - I915_GEM_OBJECT_IS_SHRINKABLE, + .flags = I915_GEM_OBJECT_IS_SHRINKABLE, .get_pages = i915_gem_object_get_pages_internal, .put_pages = i915_gem_object_put_pages_internal, }; @@ -178,7 +177,8 @@ i915_gem_object_create_internal(struct drm_i915_private *i915, return ERR_PTR(-ENOMEM);

drm_gem_private_object_init(&i915->drm, &obj->base, size); - i915_gem_object_init(obj, &i915_gem_object_internal_ops, &lock_class); + i915_gem_object_init(obj, &i915_gem_object_internal_ops, &lock_class, + I915_BO_ALLOC_STRUCT_PAGE);

/* * Mark the object as volatile, such that the pages are marked as diff --git a/drivers/gpu/drm/i915/gem/i915_gem_lmem.c b/drivers/gpu/drm/i915/gem/i915_gem_lmem.c index 194f35342710..ce1c83c13d05 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_lmem.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_lmem.c @@ -40,13 +40,13 @@ int __i915_gem_lmem_object_init(struct intel_memory_region *mem, struct drm_i915_private *i915 = mem->i915;

drm_gem_private_object_init(&i915->drm, &obj->base, size); - i915_gem_object_init(obj, &i915_gem_lmem_obj_ops, &lock_class); + i915_gem_object_init(obj, &i915_gem_lmem_obj_ops, &lock_class, flags);

obj->read_domains = I915_GEM_DOMAIN_WC | I915_GEM_DOMAIN_GTT;

i915_gem_object_set_cache_coherency(obj, I915_CACHE_NONE);

- i915_gem_object_init_memory_region(obj, mem, flags); + i915_gem_object_init_memory_region(obj, mem);

return 0; } diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c index ec28a6cde49b..c0034d811e50 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c @@ -251,7 +251,7 @@ static vm_fault_t vm_fault_cpu(struct vm_fault *vmf) goto out;

iomap = -1; - if (!i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_STRUCT_PAGE)) { + if (!i915_gem_object_has_struct_page(obj)) { iomap = obj->mm.region->iomap.base; iomap -= obj->mm.region->region.start; } @@ -653,9 +653,8 @@ __assign_mmap_offset(struct drm_file *file, }

if (mmap_type != I915_MMAP_TYPE_GTT && - !i915_gem_object_type_has(obj, - I915_GEM_OBJECT_HAS_STRUCT_PAGE | - I915_GEM_OBJECT_HAS_IOMEM)) { + !i915_gem_object_has_struct_page(obj) && + !i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM)) { err = -ENODEV; goto out; } diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c index 6cdff5fc5882..6083b9c14be6 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c @@ -60,7 +60,7 @@ void i915_gem_object_free(struct drm_i915_gem_object *obj)

void i915_gem_object_init(struct drm_i915_gem_object *obj, const struct drm_i915_gem_object_ops *ops, - struct lock_class_key *key) + struct lock_class_key *key, unsigned flags) { __mutex_init(&obj->mm.lock, ops->name ?: "obj->mm.lock", key);

@@ -78,6 +78,8 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj, init_rcu_head(&obj->rcu);

obj->ops = ops; + GEM_BUG_ON(flags & ~I915_BO_ALLOC_FLAGS); + obj->flags = flags;

obj->mm.madv = I915_MADV_WILLNEED; INIT_RADIX_TREE(&obj->mm.get_page.radix, GFP_KERNEL | __GFP_NOWARN); diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h index dc949404843a..f706812280d6 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h @@ -49,7 +49,8 @@ void i915_gem_object_free(struct drm_i915_gem_object *obj);

void i915_gem_object_init(struct drm_i915_gem_object *obj, const struct drm_i915_gem_object_ops *ops, - struct lock_class_key *key); + struct lock_class_key *key, + unsigned alloc_flags); struct drm_i915_gem_object * i915_gem_object_create_shmem(struct drm_i915_private *i915, resource_size_t size); @@ -241,7 +242,7 @@ i915_gem_object_type_has(const struct drm_i915_gem_object *obj, static inline bool i915_gem_object_has_struct_page(const struct drm_i915_gem_object *obj) { - return i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_STRUCT_PAGE); + return obj->flags & I915_BO_ALLOC_STRUCT_PAGE; }

static inline bool diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h index 0a1fdbac882e..0320508b66b3 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h @@ -30,7 +30,6 @@ struct i915_lut_handle {

struct drm_i915_gem_object_ops { unsigned int flags; -#define I915_GEM_OBJECT_HAS_STRUCT_PAGE BIT(0) #define I915_GEM_OBJECT_HAS_IOMEM BIT(1) #define I915_GEM_OBJECT_IS_SHRINKABLE BIT(2) #define I915_GEM_OBJECT_IS_PROXY BIT(3) @@ -171,9 +170,12 @@ struct drm_i915_gem_object { unsigned long flags; #define I915_BO_ALLOC_CONTIGUOUS BIT(0) #define I915_BO_ALLOC_VOLATILE BIT(1) -#define I915_BO_ALLOC_FLAGS (I915_BO_ALLOC_CONTIGUOUS | I915_BO_ALLOC_VOLATILE) -#define I915_BO_READONLY BIT(2) -#define I915_TILING_QUIRK_BIT 3 /* unknown swizzling; do not release! */ +#define I915_BO_ALLOC_STRUCT_PAGE BIT(2) +#define I915_BO_ALLOC_FLAGS (I915_BO_ALLOC_CONTIGUOUS | \ + I915_BO_ALLOC_VOLATILE | \ + I915_BO_ALLOC_STRUCT_PAGE) +#define I915_BO_READONLY BIT(3) +#define I915_TILING_QUIRK_BIT 4 /* unknown swizzling; do not release! */

/* * Is the object to be mapped as read-only to the GPU diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c index d44b72dd13fe..bf61b88a2113 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c @@ -333,13 +333,12 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj, enum i915_map_type type) { enum i915_map_type has_type; - unsigned int flags; bool pinned; void *ptr; int err;

- flags = I915_GEM_OBJECT_HAS_STRUCT_PAGE | I915_GEM_OBJECT_HAS_IOMEM; - if (!i915_gem_object_type_has(obj, flags)) + if (!i915_gem_object_has_struct_page(obj) && + !i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM)) return ERR_PTR(-ENXIO);

err = mutex_lock_interruptible_nested(&obj->mm.lock, I915_MM_GET_PAGES); diff --git a/drivers/gpu/drm/i915/gem/i915_gem_phys.c b/drivers/gpu/drm/i915/gem/i915_gem_phys.c index 01fe89afe8c0..d1bf543d111a 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_phys.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_phys.c @@ -240,6 +240,7 @@ int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj, int align) pages = __i915_gem_object_unset_pages(obj);

obj->ops = &i915_gem_phys_ops; + obj->flags &= ~I915_BO_ALLOC_STRUCT_PAGE;

err = ____i915_gem_object_get_pages(obj); if (err) @@ -258,6 +259,7 @@ int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj, int align)

err_xfer: obj->ops = &i915_gem_shmem_ops; + obj->flags |= I915_BO_ALLOC_STRUCT_PAGE; if (!IS_ERR_OR_NULL(pages)) { unsigned int sg_page_sizes = i915_sg_page_sizes(pages->sgl);

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_region.c b/drivers/gpu/drm/i915/gem/i915_gem_region.c index 77dfa908f156..6a84fb6dde24 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_region.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_region.c @@ -106,13 +106,11 @@ i915_gem_object_get_pages_buddy(struct drm_i915_gem_object *obj) }

void i915_gem_object_init_memory_region(struct drm_i915_gem_object *obj, - struct intel_memory_region *mem, - unsigned long flags) + struct intel_memory_region *mem) { INIT_LIST_HEAD(&obj->mm.blocks); obj->mm.region = intel_memory_region_get(mem);

- obj->flags |= flags; if (obj->base.size <= mem->min_page_size) obj->flags |= I915_BO_ALLOC_CONTIGUOUS;

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_region.h b/drivers/gpu/drm/i915/gem/i915_gem_region.h index f2ff6f8bff74..ebddc86d78f7 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_region.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_region.h @@ -17,8 +17,7 @@ void i915_gem_object_put_pages_buddy(struct drm_i915_gem_object *obj, struct sg_table *pages);

void i915_gem_object_init_memory_region(struct drm_i915_gem_object *obj, - struct intel_memory_region *mem, - unsigned long flags); + struct intel_memory_region *mem); void i915_gem_object_release_memory_region(struct drm_i915_gem_object *obj);

struct drm_i915_gem_object * diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c index 680b370a8ef3..bb82b3bc8830 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c @@ -430,8 +430,7 @@ static void shmem_release(struct drm_i915_gem_object *obj)

const struct drm_i915_gem_object_ops i915_gem_shmem_ops = { .name = "i915_gem_object_shmem", - .flags = I915_GEM_OBJECT_HAS_STRUCT_PAGE | - I915_GEM_OBJECT_IS_SHRINKABLE, + .flags = I915_GEM_OBJECT_IS_SHRINKABLE,

.get_pages = shmem_get_pages, .put_pages = shmem_put_pages, @@ -491,7 +490,8 @@ static int shmem_object_init(struct intel_memory_region *mem, mapping_set_gfp_mask(mapping, mask); GEM_BUG_ON(!(mapping_gfp_mask(mapping) & __GFP_RECLAIM));

- i915_gem_object_init(obj, &i915_gem_shmem_ops, &lock_class); + i915_gem_object_init(obj, &i915_gem_shmem_ops, &lock_class, + I915_BO_ALLOC_STRUCT_PAGE);

obj->write_domain = I915_GEM_DOMAIN_CPU; obj->read_domains = I915_GEM_DOMAIN_CPU; @@ -515,7 +515,7 @@ static int shmem_object_init(struct intel_memory_region *mem,

i915_gem_object_set_cache_coherency(obj, cache_level);

- i915_gem_object_init_memory_region(obj, mem, 0); + i915_gem_object_init_memory_region(obj, mem);

return 0; } diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c index c5f85296a45f..7cdb32d881d9 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c @@ -630,7 +630,7 @@ static int __i915_gem_object_create_stolen(struct intel_memory_region *mem, int err;

drm_gem_private_object_init(&mem->i915->drm, &obj->base, stolen->size); - i915_gem_object_init(obj, &i915_gem_object_stolen_ops, &lock_class); + i915_gem_object_init(obj, &i915_gem_object_stolen_ops, &lock_class, 0);

obj->stolen = stolen; obj->read_domains = I915_GEM_DOMAIN_CPU | I915_GEM_DOMAIN_GTT; @@ -641,7 +641,7 @@ static int __i915_gem_object_create_stolen(struct intel_memory_region *mem, if (err) return err;

- i915_gem_object_init_memory_region(obj, mem, 0); + i915_gem_object_init_memory_region(obj, mem);

return 0; } diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c index 3e4785c2dfa2..0f9024c62c06 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c @@ -702,8 +702,7 @@ i915_gem_userptr_dmabuf_export(struct drm_i915_gem_object *obj)

static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = { .name = "i915_gem_object_userptr", - .flags = I915_GEM_OBJECT_HAS_STRUCT_PAGE | - I915_GEM_OBJECT_IS_SHRINKABLE | + .flags = I915_GEM_OBJECT_IS_SHRINKABLE | I915_GEM_OBJECT_NO_MMAP | I915_GEM_OBJECT_ASYNC_CANCEL, .get_pages = i915_gem_userptr_get_pages, @@ -796,7 +795,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev, return -ENOMEM;

drm_gem_private_object_init(dev, &obj->base, args->user_size); - i915_gem_object_init(obj, &i915_gem_userptr_ops, &lock_class); + i915_gem_object_init(obj, &i915_gem_userptr_ops, &lock_class, + I915_BO_ALLOC_STRUCT_PAGE); obj->read_domains = I915_GEM_DOMAIN_CPU; obj->write_domain = I915_GEM_DOMAIN_CPU; i915_gem_object_set_cache_coherency(obj, I915_CACHE_LLC); diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c b/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c index 2fb501a78a85..0c8ecfdf5405 100644 --- a/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c +++ b/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c @@ -89,7 +89,6 @@ static void huge_put_pages(struct drm_i915_gem_object *obj,

static const struct drm_i915_gem_object_ops huge_ops = { .name = "huge-gem", - .flags = I915_GEM_OBJECT_HAS_STRUCT_PAGE, .get_pages = huge_get_pages, .put_pages = huge_put_pages, }; @@ -115,7 +114,8 @@ huge_gem_object(struct drm_i915_private *i915, return ERR_PTR(-ENOMEM);

drm_gem_private_object_init(&i915->drm, &obj->base, dma_size); - i915_gem_object_init(obj, &huge_ops, &lock_class); + i915_gem_object_init(obj, &huge_ops, &lock_class, + I915_BO_ALLOC_STRUCT_PAGE);

obj->read_domains = I915_GEM_DOMAIN_CPU; obj->write_domain = I915_GEM_DOMAIN_CPU; diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c index 10ee24b252dd..515dbc468175 100644 --- a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c +++ b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c @@ -140,8 +140,7 @@ static void put_huge_pages(struct drm_i915_gem_object *obj,

static const struct drm_i915_gem_object_ops huge_page_ops = { .name = "huge-gem", - .flags = I915_GEM_OBJECT_HAS_STRUCT_PAGE | - I915_GEM_OBJECT_IS_SHRINKABLE, + .flags = I915_GEM_OBJECT_IS_SHRINKABLE, .get_pages = get_huge_pages, .put_pages = put_huge_pages, }; @@ -168,7 +167,8 @@ huge_pages_object(struct drm_i915_private *i915, return ERR_PTR(-ENOMEM);

drm_gem_private_object_init(&i915->drm, &obj->base, size); - i915_gem_object_init(obj, &huge_page_ops, &lock_class); + i915_gem_object_init(obj, &huge_page_ops, &lock_class, + I915_BO_ALLOC_STRUCT_PAGE);

i915_gem_object_set_volatile(obj); i915_gem_object_set_cache_coherency(obj, I915_CACHE_NONE); @@ -317,9 +317,9 @@ fake_huge_pages_object(struct drm_i915_private *i915, u64 size, bool single) drm_gem_private_object_init(&i915->drm, &obj->base, size);

if (single) - i915_gem_object_init(obj, &fake_ops_single, &lock_class); + i915_gem_object_init(obj, &fake_ops_single, &lock_class, 0); else - i915_gem_object_init(obj, &fake_ops, &lock_class); + i915_gem_object_init(obj, &fake_ops, &lock_class, 0);

i915_gem_object_set_volatile(obj); i915_gem_object_set_cache_coherency(obj, I915_CACHE_NONE); diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c index 39293d98f34d..49f17708c143 100644 --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c @@ -819,9 +819,8 @@ static bool can_mmap(struct drm_i915_gem_object *obj, enum i915_mmap_type type) return false;

if (type != I915_MMAP_TYPE_GTT && - !i915_gem_object_type_has(obj, - I915_GEM_OBJECT_HAS_STRUCT_PAGE | - I915_GEM_OBJECT_HAS_IOMEM)) + !i915_gem_object_has_struct_page(obj) && + !i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM)) return false;

return true; @@ -961,10 +960,8 @@ static const char *repr_mmap_type(enum i915_mmap_type type)

static bool can_access(const struct drm_i915_gem_object *obj) { - unsigned int flags = - I915_GEM_OBJECT_HAS_STRUCT_PAGE | I915_GEM_OBJECT_HAS_IOMEM; - - return i915_gem_object_type_has(obj, flags); + return i915_gem_object_has_struct_page(obj) || + i915_gem_object_type_has(obj, I915_GEM_OBJECT_HAS_IOMEM); }

static int __igt_mmap_access(struct drm_i915_private *i915, diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c index b62d02cb9579..338dada88359 100644 --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_phys.c @@ -25,12 +25,24 @@ static int mock_phys_object(void *arg) goto out; }

+ if (!i915_gem_object_has_struct_page(obj)) { + err = -EINVAL; + pr_err("shmem has no struct page\n"); + goto out_obj; + } + err = i915_gem_object_attach_phys(obj, PAGE_SIZE); if (err) { pr_err("i915_gem_object_attach_phys failed, err=%d\n", err); goto out_obj; }

+ if (i915_gem_object_has_struct_page(obj)) { + err = -EINVAL; + pr_err("shmem has a struct page\n"); + goto out_obj; + } + if (obj->ops != &i915_gem_phys_ops) { pr_err("i915_gem_object_attach_phys did not create a phys object\n"); err = -EINVAL; diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.c b/drivers/gpu/drm/i915/gvt/dmabuf.c index c3eb3838fe88..d4f883f35b95 100644 --- a/drivers/gpu/drm/i915/gvt/dmabuf.c +++ b/drivers/gpu/drm/i915/gvt/dmabuf.c @@ -218,7 +218,7 @@ static struct drm_i915_gem_object *vgpu_create_gem(struct drm_device *dev,

drm_gem_private_object_init(dev, &obj->base, roundup(info->size, PAGE_SIZE)); - i915_gem_object_init(obj, &intel_vgpu_gem_ops, &lock_class); + i915_gem_object_init(obj, &intel_vgpu_gem_ops, &lock_class, 0); i915_gem_object_set_readonly(obj);

obj->read_domains = I915_GEM_DOMAIN_GTT; diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c index c1adea8765a9..5be6dcf4357e 100644 --- a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c @@ -121,7 +121,7 @@ fake_dma_object(struct drm_i915_private *i915, u64 size) goto err;

drm_gem_private_object_init(&i915->drm, &obj->base, size); - i915_gem_object_init(obj, &fake_ops, &lock_class); + i915_gem_object_init(obj, &fake_ops, &lock_class, 0);

i915_gem_object_set_volatile(obj);

diff --git a/drivers/gpu/drm/i915/selftests/mock_region.c b/drivers/gpu/drm/i915/selftests/mock_region.c index 3c6021415274..5d2d010a1e22 100644 --- a/drivers/gpu/drm/i915/selftests/mock_region.c +++ b/drivers/gpu/drm/i915/selftests/mock_region.c @@ -27,13 +27,13 @@ static int mock_object_init(struct intel_memory_region *mem, return -E2BIG;

drm_gem_private_object_init(&i915->drm, &obj->base, size); - i915_gem_object_init(obj, &mock_region_obj_ops, &lock_class); + i915_gem_object_init(obj, &mock_region_obj_ops, &lock_class, flags);

obj->read_domains = I915_GEM_DOMAIN_CPU | I915_GEM_DOMAIN_GTT;

i915_gem_object_set_cache_coherency(obj, I915_CACHE_NONE);

- i915_gem_object_init_memory_region(obj, mem, flags); + i915_gem_object_init_memory_region(obj, mem);

return 0; }

Simple adding of i915_gem_object_lock, we may start to pass ww to get_pages() in the future, but that won't be the case here; We override shmem's get_pages() handling by calling i915_gem_object_get_pages_phys(), no ww is needed.

Changes since v1: - Call shmem put pages directly, the callback would go down the phys free path.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Reviewed-by: Thomas Hellström thomas.hellstrom@linux.intel.com --- drivers/gpu/drm/i915/gem/i915_gem_object.h | 3 ++- drivers/gpu/drm/i915/gem/i915_gem_phys.c | 12 ++++++++++-- drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 17 ++++++++++------- 3 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h index 75e8734f50d2..c8eb0df904f7 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h @@ -69,10 +69,11 @@ int i915_gem_object_pread_phys(struct drm_i915_gem_object *obj, const struct drm_i915_gem_pread *args);

int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj, int align); +void i915_gem_object_put_pages_shmem(struct drm_i915_gem_object *obj, + struct sg_table *pages); void i915_gem_object_put_pages_phys(struct drm_i915_gem_object *obj, struct sg_table *pages);

- void i915_gem_flush_free_objects(struct drm_i915_private *i915);

struct sg_table * diff --git a/drivers/gpu/drm/i915/gem/i915_gem_phys.c b/drivers/gpu/drm/i915/gem/i915_gem_phys.c index ed283e168f2f..06c481ff79d8 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_phys.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_phys.c @@ -201,7 +201,7 @@ static int i915_gem_object_shmem_to_phys(struct drm_i915_gem_object *obj) __i915_gem_object_pin_pages(obj);

if (!IS_ERR_OR_NULL(pages)) - i915_gem_shmem_ops.put_pages(obj, pages); + i915_gem_object_put_pages_shmem(obj, pages);

i915_gem_object_release_memory_region(obj); return 0; @@ -232,7 +232,13 @@ int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj, int align) if (err) return err;

- mutex_lock_nested(&obj->mm.lock, I915_MM_GET_PAGES); + err = i915_gem_object_lock_interruptible(obj, NULL); + if (err) + return err; + + err = mutex_lock_interruptible_nested(&obj->mm.lock, I915_MM_GET_PAGES); + if (err) + goto err_unlock;

if (unlikely(!i915_gem_object_has_struct_page(obj))) goto out; @@ -263,6 +269,8 @@ int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj, int align)

out: mutex_unlock(&obj->mm.lock); +err_unlock: + i915_gem_object_unlock(obj); return err; }

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c index c9820c19c5f2..59fb16a82270 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c @@ -296,18 +296,12 @@ __i915_gem_object_release_shmem(struct drm_i915_gem_object *obj, __start_cpu_write(obj); }

-static void -shmem_put_pages(struct drm_i915_gem_object *obj, struct sg_table *pages) +void i915_gem_object_put_pages_shmem(struct drm_i915_gem_object *obj, struct sg_table *pages) { struct sgt_iter sgt_iter; struct pagevec pvec; struct page *page;

- if (unlikely(!i915_gem_object_has_struct_page(obj))) { - i915_gem_object_put_pages_phys(obj, pages); - return; - } - __i915_gem_object_release_shmem(obj, pages, true);

i915_gem_gtt_finish_pages(obj, pages); @@ -336,6 +330,15 @@ shmem_put_pages(struct drm_i915_gem_object *obj, struct sg_table *pages) kfree(pages); }

+static void +shmem_put_pages(struct drm_i915_gem_object *obj, struct sg_table *pages) +{ + if (likely(i915_gem_object_has_struct_page(obj))) + i915_gem_object_put_pages_shmem(obj, pages); + else + i915_gem_object_put_pages_phys(obj, pages); +} + static int shmem_pwrite(struct drm_i915_gem_object *obj, const struct drm_i915_gem_pwrite *arg)

Userptr should not need the kernel for a userspace memcpy, userspace needs to call memcpy directly.

Specifically, disable i915_gem_pwrite_ioctl() and i915_gem_pread_ioctl().

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Reviewed-by: Thomas Hellström thomas.hellstrom@linux.intel.com

-- Still needs an ack from relevant userspace that it won't break, but should be good. --- drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 20 ++++++++++++++++++++ drivers/gpu/drm/i915/i915_gem.c | 5 +++++ 2 files changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c index 0f9024c62c06..5a19699c2d7e 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c @@ -700,6 +700,24 @@ i915_gem_userptr_dmabuf_export(struct drm_i915_gem_object *obj) return i915_gem_userptr_init__mmu_notifier(obj, 0); }

+static int +i915_gem_userptr_pwrite(struct drm_i915_gem_object *obj, + const struct drm_i915_gem_pwrite *args) +{ + drm_dbg(obj->base.dev, "pwrite to userptr no longer allowed\n"); + + return -EINVAL; +} + +static int +i915_gem_userptr_pread(struct drm_i915_gem_object *obj, + const struct drm_i915_gem_pread *args) +{ + drm_dbg(obj->base.dev, "pread from userptr no longer allowed\n"); + + return -EINVAL; +} + static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = { .name = "i915_gem_object_userptr", .flags = I915_GEM_OBJECT_IS_SHRINKABLE | @@ -708,6 +726,8 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = { .get_pages = i915_gem_userptr_get_pages, .put_pages = i915_gem_userptr_put_pages, .dmabuf_export = i915_gem_userptr_dmabuf_export, + .pwrite = i915_gem_userptr_pwrite, + .pread = i915_gem_userptr_pread, .release = i915_gem_userptr_release, };

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 25444d360f7f..dde12ce4f90b 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -403,6 +403,11 @@ i915_gem_pread_ioctl(struct drm_device *dev, void *data, }

trace_i915_gem_object_pread(obj, args->offset, args->size); + ret = -ENODEV; + if (obj->ops->pread) + ret = obj->ops->pread(obj, args); + if (ret != -ENODEV) + goto out;

ret = -ENODEV; if (obj->ops->pread)

It doesn't make sense to export a memory address, we will prevent allowing access this way to different address spaces when we rework userptr handling, so best to explicitly disable it.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Reviewed-by: Thomas Hellström thomas.hellstrom@linux.intel.com Acked-by: Jason Ekstrand jason@jlekstrand.net --- drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c index 5a19699c2d7e..0c30ca52dee3 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c @@ -694,10 +694,9 @@ i915_gem_userptr_release(struct drm_i915_gem_object *obj) static int i915_gem_userptr_dmabuf_export(struct drm_i915_gem_object *obj) { - if (obj->userptr.mmu_object) - return 0; + drm_dbg(obj->base.dev, "Exporting userptr no longer allowed\n");

- return i915_gem_userptr_init__mmu_notifier(obj, 0); + return -EINVAL; }

static int

Now that unsynchronized mappings are removed, the only time userptr works is when the MMU notifier is enabled. Put all of the userptr code behind a mmu notifier ifdef.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Reviewed-by: Thomas Hellström thomas.hellstrom@linux.intel.com --- .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 + drivers/gpu/drm/i915/gem/i915_gem_object.h | 4 ++ .../gpu/drm/i915/gem/i915_gem_object_types.h | 2 + drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 58 +++++++------------ drivers/gpu/drm/i915/i915_drv.h | 2 + 5 files changed, 31 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 3c37a85c0d9d..795c68fcc6ed 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -1971,8 +1971,10 @@ static noinline int eb_relocate_parse_slow(struct i915_execbuffer *eb, err = 0; }

+#ifdef CONFIG_MMU_NOTIFIER if (!err) flush_workqueue(eb->i915->mm.userptr_wq); +#endif

err_relock: i915_gem_ww_ctx_init(&eb->ww, true); diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h index 1008c6c47809..69509dbd01c7 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h @@ -576,7 +576,11 @@ void __i915_gem_object_invalidate_frontbuffer(struct drm_i915_gem_object *obj, static inline bool i915_gem_object_is_userptr(struct drm_i915_gem_object *obj) { +#ifdef CONFIG_MMU_NOTIFIER return obj->userptr.mm; +#else + return false; +#endif }

static inline void diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h index 0320508b66b3..414322619781 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h @@ -296,6 +296,7 @@ struct drm_i915_gem_object { unsigned long *bit_17;

union { +#ifdef CONFIG_MMU_NOTIFIER struct i915_gem_userptr { uintptr_t ptr;

@@ -303,6 +304,7 @@ struct drm_i915_gem_object { struct i915_mmu_object *mmu_object; struct work_struct *work; } userptr; +#endif

struct drm_mm_node *stolen;

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c index 80bc10b4ac74..b466ab2def4d 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c @@ -15,6 +15,8 @@ #include "i915_gem_object.h" #include "i915_scatterlist.h"

+#if defined(CONFIG_MMU_NOTIFIER) + struct i915_mm_struct { struct mm_struct *mm; struct drm_i915_private *i915; @@ -24,7 +26,6 @@ struct i915_mm_struct { struct rcu_work work; };

-#if defined(CONFIG_MMU_NOTIFIER) #include <linux/interval_tree.h>

struct i915_mmu_notifier { @@ -217,15 +218,11 @@ i915_mmu_notifier_find(struct i915_mm_struct *mm) }

static int -i915_gem_userptr_init__mmu_notifier(struct drm_i915_gem_object *obj, - unsigned flags) +i915_gem_userptr_init__mmu_notifier(struct drm_i915_gem_object *obj) { struct i915_mmu_notifier *mn; struct i915_mmu_object *mo;

- if (flags & I915_USERPTR_UNSYNCHRONIZED) - return -ENODEV; - if (GEM_WARN_ON(!obj->userptr.mm)) return -EINVAL;

@@ -258,32 +255,6 @@ i915_mmu_notifier_free(struct i915_mmu_notifier *mn, kfree(mn); }

-#else - -static void -__i915_gem_userptr_set_active(struct drm_i915_gem_object *obj, bool value) -{ -} - -static void -i915_gem_userptr_release__mmu_notifier(struct drm_i915_gem_object *obj) -{ -} - -static int -i915_gem_userptr_init__mmu_notifier(struct drm_i915_gem_object *obj, - unsigned flags) -{ - return -ENODEV; -} - -static void -i915_mmu_notifier_free(struct i915_mmu_notifier *mn, - struct mm_struct *mm) -{ -} - -#endif

static struct i915_mm_struct * __i915_mm_struct_find(struct drm_i915_private *i915, struct mm_struct *real) @@ -725,6 +696,8 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = { .release = i915_gem_userptr_release, };

+#endif + /* * Creates a new mm object that wraps some normal memory from the process * context - user memory. @@ -765,12 +738,12 @@ i915_gem_userptr_ioctl(struct drm_device *dev, void *data, struct drm_file *file) { - static struct lock_class_key lock_class; + static struct lock_class_key __maybe_unused lock_class; struct drm_i915_private *dev_priv = to_i915(dev); struct drm_i915_gem_userptr *args = data; - struct drm_i915_gem_object *obj; - int ret; - u32 handle; + struct drm_i915_gem_object __maybe_unused *obj; + int __maybe_unused ret; + u32 __maybe_unused handle;

if (!HAS_LLC(dev_priv) && !HAS_SNOOP(dev_priv)) { /* We cannot support coherent userptr objects on hw without @@ -795,6 +768,9 @@ i915_gem_userptr_ioctl(struct drm_device *dev, if (!access_ok((char __user *)(unsigned long)args->user_ptr, args->user_size)) return -EFAULT;

+ if (args->flags & I915_USERPTR_UNSYNCHRONIZED) + return -ENODEV; + if (args->flags & I915_USERPTR_READ_ONLY) { /* * On almost all of the older hw, we cannot tell the GPU that @@ -804,6 +780,7 @@ i915_gem_userptr_ioctl(struct drm_device *dev, return -ENODEV; }

+#ifdef CONFIG_MMU_NOTIFIER obj = i915_gem_object_alloc(); if (obj == NULL) return -ENOMEM; @@ -825,7 +802,7 @@ i915_gem_userptr_ioctl(struct drm_device *dev, */ ret = i915_gem_userptr_init__mm_struct(obj); if (ret == 0) - ret = i915_gem_userptr_init__mmu_notifier(obj, args->flags); + ret = i915_gem_userptr_init__mmu_notifier(obj); if (ret == 0) ret = drm_gem_handle_create(file, &obj->base, &handle);

@@ -836,10 +813,14 @@ i915_gem_userptr_ioctl(struct drm_device *dev,

args->handle = handle; return 0; +#else + return -ENODEV; +#endif }

int i915_gem_init_userptr(struct drm_i915_private *dev_priv) { +#ifdef CONFIG_MMU_NOTIFIER spin_lock_init(&dev_priv->mm_lock); hash_init(dev_priv->mm_structs);

@@ -849,11 +830,14 @@ int i915_gem_init_userptr(struct drm_i915_private *dev_priv) 0); if (!dev_priv->mm.userptr_wq) return -ENOMEM; +#endif

return 0; }

void i915_gem_cleanup_userptr(struct drm_i915_private *dev_priv) { +#ifdef CONFIG_MMU_NOTIFIER destroy_workqueue(dev_priv->mm.userptr_wq); +#endif } diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 58ae4b76533f..d738e2ba07c8 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -556,12 +556,14 @@ struct i915_gem_mm { struct notifier_block vmap_notifier; struct shrinker shrinker;

+#ifdef CONFIG_MMU_NOTIFIER /** * Workqueue to fault in userptr pages, flushed by the execbuf * when required but otherwise left to userspace to try again * on EAGAIN. */ struct workqueue_struct *userptr_wq; +#endif

/* shrinker accounting, also useful for userland debugging */ u64 shrink_memory;

We map the initial context during first pin.

This allows us to remove pin_map from state allocation, which saves us a few retry loops. We won't need this until first pin anyway.

intel_ring_submission_setup() is also reworked slightly to do all pinning in a single ww loop.

Changes since v1: - Handle -EDEADLK backoff in intel_ring_submission_setup() better. - Handle smatch errors reported by Dan and testbot.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Reported-by: kernel test robot lkp@intel.com Reported-by: Dan Carpenter dan.carpenter@oracle.com Reviewed-by: Thomas Hellström thomas.hellstrom@linux.intel.com --- .../gpu/drm/i915/gt/intel_ring_submission.c | 184 +++++++++++------- 1 file changed, 118 insertions(+), 66 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_ring_submission.c b/drivers/gpu/drm/i915/gt/intel_ring_submission.c index 282089d64789..f8ad891ad635 100644 --- a/drivers/gpu/drm/i915/gt/intel_ring_submission.c +++ b/drivers/gpu/drm/i915/gt/intel_ring_submission.c @@ -436,6 +436,26 @@ static void ring_context_destroy(struct kref *ref) intel_context_free(ce); }

+static int ring_context_init_default_state(struct intel_context *ce, + struct i915_gem_ww_ctx *ww) +{ + struct drm_i915_gem_object *obj = ce->state->obj; + void *vaddr; + + vaddr = i915_gem_object_pin_map(obj, I915_MAP_WB); + if (IS_ERR(vaddr)) + return PTR_ERR(vaddr); + + shmem_read(ce->engine->default_state, 0, + vaddr, ce->engine->context_size); + + i915_gem_object_flush_map(obj); + __i915_gem_object_release_map(obj); + + __set_bit(CONTEXT_VALID_BIT, &ce->flags); + return 0; +} + static int ring_context_pre_pin(struct intel_context *ce, struct i915_gem_ww_ctx *ww, void **unused) @@ -443,6 +463,13 @@ static int ring_context_pre_pin(struct intel_context *ce, struct i915_address_space *vm; int err = 0;

+ if (ce->engine->default_state && + !test_bit(CONTEXT_VALID_BIT, &ce->flags)) { + err = ring_context_init_default_state(ce, ww); + if (err) + return err; + } + vm = vm_alias(ce->vm); if (vm) err = gen6_ppgtt_pin(i915_vm_to_ppgtt((vm)), ww); @@ -498,22 +525,6 @@ alloc_context_vma(struct intel_engine_cs *engine) if (IS_IVYBRIDGE(i915)) i915_gem_object_set_cache_coherency(obj, I915_CACHE_L3_LLC);

- if (engine->default_state) { - void *vaddr; - - vaddr = i915_gem_object_pin_map(obj, I915_MAP_WB); - if (IS_ERR(vaddr)) { - err = PTR_ERR(vaddr); - goto err_obj; - } - - shmem_read(engine->default_state, 0, - vaddr, engine->context_size); - - i915_gem_object_flush_map(obj); - __i915_gem_object_release_map(obj); - } - vma = i915_vma_instance(obj, &engine->gt->ggtt->vm, NULL); if (IS_ERR(vma)) { err = PTR_ERR(vma); @@ -545,8 +556,6 @@ static int ring_context_alloc(struct intel_context *ce) return PTR_ERR(vma);

ce->state = vma; - if (engine->default_state) - __set_bit(CONTEXT_VALID_BIT, &ce->flags); }

return 0; @@ -1151,37 +1160,15 @@ static int gen7_ctx_switch_bb_setup(struct intel_engine_cs * const engine, return gen7_setup_clear_gpr_bb(engine, vma); }

-static int gen7_ctx_switch_bb_init(struct intel_engine_cs *engine) +static int gen7_ctx_switch_bb_init(struct intel_engine_cs *engine, + struct i915_gem_ww_ctx *ww, + struct i915_vma *vma) { - struct drm_i915_gem_object *obj; - struct i915_vma *vma; - int size; int err;

- size = gen7_ctx_switch_bb_setup(engine, NULL /* probe size */); - if (size <= 0) - return size; - - size = ALIGN(size, PAGE_SIZE); - obj = i915_gem_object_create_internal(engine->i915, size); - if (IS_ERR(obj)) - return PTR_ERR(obj); - - vma = i915_vma_instance(obj, engine->gt->vm, NULL); - if (IS_ERR(vma)) { - err = PTR_ERR(vma); - goto err_obj; - } - - vma->private = intel_context_create(engine); /* dummy residuals */ - if (IS_ERR(vma->private)) { - err = PTR_ERR(vma->private); - goto err_obj; - } - - err = i915_vma_pin(vma, 0, 0, PIN_USER | PIN_HIGH); + err = i915_vma_pin_ww(vma, ww, 0, 0, PIN_USER | PIN_HIGH); if (err) - goto err_private; + return err;

err = i915_vma_sync(vma); if (err) @@ -1196,17 +1183,53 @@ static int gen7_ctx_switch_bb_init(struct intel_engine_cs *engine)

err_unpin: i915_vma_unpin(vma); -err_private: - intel_context_put(vma->private); -err_obj: - i915_gem_object_put(obj); return err; }

+static struct i915_vma *gen7_ctx_vma(struct intel_engine_cs *engine) +{ + struct drm_i915_gem_object *obj; + struct i915_vma *vma; + int size, err; + + if (!IS_GEN(engine->i915, 7) || engine->class != RENDER_CLASS) + return 0; + + err = gen7_ctx_switch_bb_setup(engine, NULL /* probe size */); + if (err < 0) + return ERR_PTR(err); + if (!err) + return NULL; + + size = ALIGN(err, PAGE_SIZE); + + obj = i915_gem_object_create_internal(engine->i915, size); + if (IS_ERR(obj)) + return ERR_CAST(obj); + + vma = i915_vma_instance(obj, engine->gt->vm, NULL); + if (IS_ERR(vma)) { + i915_gem_object_put(obj); + return ERR_CAST(vma); + } + + vma->private = intel_context_create(engine); /* dummy residuals */ + if (IS_ERR(vma->private)) { + err = PTR_ERR(vma->private); + vma->private = NULL; + i915_gem_object_put(obj); + return ERR_PTR(err); + } + + return vma; +} + int intel_ring_submission_setup(struct intel_engine_cs *engine) { + struct i915_gem_ww_ctx ww; struct intel_timeline *timeline; struct intel_ring *ring; + struct i915_vma *gen7_wa_vma; int err;

setup_common(engine); @@ -1237,43 +1260,72 @@ int intel_ring_submission_setup(struct intel_engine_cs *engine) } GEM_BUG_ON(timeline->has_initial_breadcrumb);

- err = intel_timeline_pin(timeline, NULL); - if (err) - goto err_timeline; - ring = intel_engine_create_ring(engine, SZ_16K); if (IS_ERR(ring)) { err = PTR_ERR(ring); - goto err_timeline_unpin; + goto err_timeline; }

- err = intel_ring_pin(ring, NULL); - if (err) - goto err_ring; - GEM_BUG_ON(engine->legacy.ring); engine->legacy.ring = ring; engine->legacy.timeline = timeline;

- GEM_BUG_ON(timeline->hwsp_ggtt != engine->status_page.vma); + gen7_wa_vma = gen7_ctx_vma(engine); + if (IS_ERR(gen7_wa_vma)) { + err = PTR_ERR(gen7_wa_vma); + goto err_ring; + }

- if (IS_GEN(engine->i915, 7) && engine->class == RENDER_CLASS) { - err = gen7_ctx_switch_bb_init(engine); + i915_gem_ww_ctx_init(&ww, false); + +retry: + err = i915_gem_object_lock(timeline->hwsp_ggtt->obj, &ww); + if (!err && gen7_wa_vma) + err = i915_gem_object_lock(gen7_wa_vma->obj, &ww); + if (!err && engine->legacy.ring->vma->obj) + err = i915_gem_object_lock(engine->legacy.ring->vma->obj, &ww); + if (!err) + err = intel_timeline_pin(timeline, &ww); + if (!err) { + err = intel_ring_pin(ring, &ww); if (err) - goto err_ring_unpin; + intel_timeline_unpin(timeline); + } + if (err) + goto out; + + GEM_BUG_ON(timeline->hwsp_ggtt != engine->status_page.vma); + + if (gen7_wa_vma) { + err = gen7_ctx_switch_bb_init(engine, &ww, gen7_wa_vma); + if (err) { + intel_ring_unpin(ring); + intel_timeline_unpin(timeline); + } }

+out: + if (err == -EDEADLK) { + err = i915_gem_ww_ctx_backoff(&ww); + if (!err) + goto retry; + } + i915_gem_ww_ctx_fini(&ww); + if (err) + goto err_gen7_put; + /* Finally, take ownership and responsibility for cleanup! */ engine->release = ring_release;

return 0;

-err_ring_unpin: - intel_ring_unpin(ring); +err_gen7_put: + if (gen7_wa_vma) { + intel_context_put(gen7_wa_vma->private); + i915_gem_object_put(gen7_wa_vma->obj); + } err_ring: intel_ring_put(ring); -err_timeline_unpin: - intel_timeline_unpin(timeline); err_timeline: intel_timeline_put(timeline); err:

Pin in the caller, not in the work itself. This should also work better for dma-fence annotations.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Reviewed-by: Thomas Hellström thomas.hellstrom@linux.intel.com --- drivers/gpu/drm/i915/gem/i915_gem_clflush.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_clflush.c b/drivers/gpu/drm/i915/gem/i915_gem_clflush.c index a28f8c912a3e..e4c24558eaa8 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_clflush.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_clflush.c @@ -27,15 +27,8 @@ static void __do_clflush(struct drm_i915_gem_object *obj) static int clflush_work(struct dma_fence_work *base) { struct clflush *clflush = container_of(base, typeof(*clflush), base); - struct drm_i915_gem_object *obj = clflush->obj; - int err;

- err = i915_gem_object_pin_pages(obj); - if (err) - return err; - - __do_clflush(obj); - i915_gem_object_unpin_pages(obj); + __do_clflush(clflush->obj);

return 0; } @@ -44,6 +37,7 @@ static void clflush_release(struct dma_fence_work *base) { struct clflush *clflush = container_of(base, typeof(*clflush), base);

+ i915_gem_object_unpin_pages(clflush->obj); i915_gem_object_put(clflush->obj); }

@@ -61,6 +55,11 @@ static struct clflush *clflush_work_create(struct drm_i915_gem_object *obj) if (!clflush) return NULL;

+ if (__i915_gem_object_get_pages(obj) < 0) { + kfree(clflush); + return NULL; + } + dma_fence_work_init(&clflush->base, &clflush_ops); clflush->obj = i915_gem_object_get(obj); /* obj <-> clflush cycle */

Take a simple lock so we hold ww around (un)pin_pages as needed.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Reviewed-by: Thomas Hellström thomas.hellstrom@linux.intel.com --- drivers/gpu/drm/i915/gem/i915_gem_mman.c | 4 ++++ 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c index c0034d811e50..163208a6260d 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c @@ -246,6 +246,9 @@ static vm_fault_t vm_fault_cpu(struct vm_fault *vmf) area->vm_flags & VM_WRITE)) return VM_FAULT_SIGBUS;

+ if (i915_gem_object_lock_interruptible(obj, NULL)) + return VM_FAULT_NOPAGE; + err = i915_gem_object_pin_pages(obj); if (err) goto out; @@ -269,6 +272,7 @@ static vm_fault_t vm_fault_cpu(struct vm_fault *vmf) i915_gem_object_unpin_pages(obj);

out: + i915_gem_object_unlock(obj); return i915_error_to_vmf_fault(err); }

We previously complained when ww == NULL.

This function is now only used in selftests to pin an object, and ww locking is now fixed.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Reviewed-by: Thomas Hellström thomas.hellstrom@linux.intel.com --- .../i915/gem/selftests/i915_gem_coherency.c | 12 ++++------- drivers/gpu/drm/i915/i915_gem.c | 6 +++++- drivers/gpu/drm/i915/i915_vma.c | 4 +--- drivers/gpu/drm/i915/i915_vma.h | 20 +++++++++++++++---- 4 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c index b5dbf15570fc..3eec385d43bb 100644 --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c @@ -218,15 +218,13 @@ static int gpu_set(struct context *ctx, unsigned long offset, u32 v) u32 *cs; int err;

+ vma = i915_gem_object_ggtt_pin(ctx->obj, NULL, 0, 0, 0); + if (IS_ERR(vma)) + return PTR_ERR(vma); + i915_gem_object_lock(ctx->obj, NULL); i915_gem_object_set_to_gtt_domain(ctx->obj, false);

- vma = i915_gem_object_ggtt_pin(ctx->obj, NULL, 0, 0, 0); - if (IS_ERR(vma)) { - err = PTR_ERR(vma); - goto out_unlock; - } - rq = intel_engine_create_kernel_request(ctx->engine); if (IS_ERR(rq)) { err = PTR_ERR(rq); @@ -265,9 +263,7 @@ static int gpu_set(struct context *ctx, unsigned long offset, u32 v) i915_request_add(rq); out_unpin: i915_vma_unpin(vma); -out_unlock: i915_gem_object_unlock(ctx->obj); - return err; }

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 3dee4e31fb14..8373662e4b5f 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -920,7 +920,11 @@ i915_gem_object_ggtt_pin_ww(struct drm_i915_gem_object *obj, return ERR_PTR(ret); }

- ret = i915_vma_pin_ww(vma, ww, size, alignment, flags | PIN_GLOBAL); + if (ww) + ret = i915_vma_pin_ww(vma, ww, size, alignment, flags | PIN_GLOBAL); + else + ret = i915_vma_pin(vma, size, alignment, flags | PIN_GLOBAL); + if (ret) return ERR_PTR(ret);

diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c index 1ffda2aaa7a0..265e3a3079e2 100644 --- a/drivers/gpu/drm/i915/i915_vma.c +++ b/drivers/gpu/drm/i915/i915_vma.c @@ -863,9 +863,7 @@ int i915_vma_pin_ww(struct i915_vma *vma, struct i915_gem_ww_ctx *ww, int err;

#ifdef CONFIG_PROVE_LOCKING - if (debug_locks && lockdep_is_held(&vma->vm->i915->drm.struct_mutex)) - WARN_ON(!ww); - if (debug_locks && ww && vma->resv) + if (debug_locks && !WARN_ON(!ww) && vma->resv) assert_vma_held(vma); #endif

diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h index 6b48f5c42488..8df784a026d2 100644 --- a/drivers/gpu/drm/i915/i915_vma.h +++ b/drivers/gpu/drm/i915/i915_vma.h @@ -246,10 +246,22 @@ i915_vma_pin_ww(struct i915_vma *vma, struct i915_gem_ww_ctx *ww, static inline int __must_check i915_vma_pin(struct i915_vma *vma, u64 size, u64 alignment, u64 flags) { -#ifdef CONFIG_LOCKDEP - WARN_ON_ONCE(vma->resv && dma_resv_held(vma->resv)); -#endif - return i915_vma_pin_ww(vma, NULL, size, alignment, flags); + struct i915_gem_ww_ctx ww; + int err; + + i915_gem_ww_ctx_init(&ww, true); +retry: + err = i915_gem_object_lock(vma->obj, &ww); + if (!err) + err = i915_vma_pin_ww(vma, &ww, size, alignment, flags); + if (err == -EDEADLK) { + err = i915_gem_ww_ctx_backoff(&ww); + if (!err) + goto retry; + } + i915_gem_ww_ctx_fini(&ww); + + return err; }

int i915_ggtt_pin(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,

Make creation separate from pinning, in order to take the lock only once, and pin the mapping with the lock held.

Changes since v1: - Rebase on top of upstream changes. Changes since v2: - Fully clear wa_ctx on error.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Reviewed-by: Thomas Hellström thomas.hellstrom@linux.intel.com --- drivers/gpu/drm/i915/gt/intel_lrc.c | 49 ++++++++++++++++++++++------- 1 file changed, 38 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c index 8508b8d701c1..a2b916d27a39 100644 --- a/drivers/gpu/drm/i915/gt/intel_lrc.c +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c @@ -1421,7 +1421,7 @@ gen10_init_indirectctx_bb(struct intel_engine_cs *engine, u32 *batch)

#define CTX_WA_BB_SIZE (PAGE_SIZE)

-static int lrc_setup_wa_ctx(struct intel_engine_cs *engine) +static int lrc_create_wa_ctx(struct intel_engine_cs *engine) { struct drm_i915_gem_object *obj; struct i915_vma *vma; @@ -1437,10 +1437,6 @@ static int lrc_setup_wa_ctx(struct intel_engine_cs *engine) goto err; }

- err = i915_ggtt_pin(vma, NULL, 0, PIN_HIGH); - if (err) - goto err; - engine->wa_ctx.vma = vma; return 0;

@@ -1452,9 +1448,6 @@ static int lrc_setup_wa_ctx(struct intel_engine_cs *engine) void lrc_fini_wa_ctx(struct intel_engine_cs *engine) { i915_vma_unpin_and_release(&engine->wa_ctx.vma, 0); - - /* Called on error unwind, clear all flags to prevent further use */ - memset(&engine->wa_ctx, 0, sizeof(engine->wa_ctx)); }

typedef u32 *(*wa_bb_func_t)(struct intel_engine_cs *engine, u32 *batch); @@ -1466,6 +1459,7 @@ void lrc_init_wa_ctx(struct intel_engine_cs *engine) &wa_ctx->indirect_ctx, &wa_ctx->per_ctx }; wa_bb_func_t wa_bb_fn[ARRAY_SIZE(wa_bb)]; + struct i915_gem_ww_ctx ww; void *batch, *batch_ptr; unsigned int i; int err; @@ -1494,7 +1488,7 @@ void lrc_init_wa_ctx(struct intel_engine_cs *engine) return; }

- err = lrc_setup_wa_ctx(engine); + err = lrc_create_wa_ctx(engine); if (err) { /* * We continue even if we fail to initialize WA batch @@ -1507,7 +1501,22 @@ void lrc_init_wa_ctx(struct intel_engine_cs *engine) return; }

+ if (!engine->wa_ctx.vma) + return; + + i915_gem_ww_ctx_init(&ww, true); +retry: + err = i915_gem_object_lock(wa_ctx->vma->obj, &ww); + if (!err) + err = i915_ggtt_pin(wa_ctx->vma, &ww, 0, PIN_HIGH); + if (err) + goto err; + batch = i915_gem_object_pin_map(wa_ctx->vma->obj, I915_MAP_WB); + if (IS_ERR(batch)) { + err = PTR_ERR(batch); + goto err_unpin; + }

/* * Emit the two workaround batch buffers, recording the offset from the @@ -1532,8 +1541,26 @@ void lrc_init_wa_ctx(struct intel_engine_cs *engine) __i915_gem_object_release_map(wa_ctx->vma->obj);

/* Verify that we can handle failure to setup the wa_ctx */ - if (err || i915_inject_probe_error(engine->i915, -ENODEV)) - lrc_fini_wa_ctx(engine); + if (!err) + err = i915_inject_probe_error(engine->i915, -ENODEV); + +err_unpin: + if (err) + i915_vma_unpin(wa_ctx->vma); +err: + if (err == -EDEADLK) { + err = i915_gem_ww_ctx_backoff(&ww); + if (!err) + goto retry; + } + i915_gem_ww_ctx_fini(&ww); + + if (err) { + i915_vma_put(engine->wa_ctx.vma); + + /* Clear all flags to prevent further use */ + memset(wa_ctx, 0, sizeof(*wa_ctx)); + } }

static void st_runtime_underflow(struct intel_context_stats *stats, s32 dt)

We are removing obj->mm.lock, and need to take the reservation lock before we can pin pages. Move the pinning pages into the helper, and merge gtt pwrite/pread preparation and cleanup paths.

The fence lock is also removed; it will conflict with fence annotations, because of memory allocations done when pagefaulting inside copy_*_user.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Reviewed-by: Thomas Hellström thomas.hellstrom@linux.intel.com --- drivers/gpu/drm/i915/Makefile | 1 - drivers/gpu/drm/i915/gem/i915_gem_fence.c | 95 --------- drivers/gpu/drm/i915/gem/i915_gem_object.h | 5 - drivers/gpu/drm/i915/i915_gem.c | 215 +++++++++++---------- 4 files changed, 112 insertions(+), 204 deletions(-) delete mode 100644 drivers/gpu/drm/i915/gem/i915_gem_fence.c

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 33c2100414a0..70a535798ef5 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -140,7 +140,6 @@ gem-y += \ gem/i915_gem_dmabuf.o \ gem/i915_gem_domain.o \ gem/i915_gem_execbuffer.o \ - gem/i915_gem_fence.o \ gem/i915_gem_internal.o \ gem/i915_gem_object.o \ gem/i915_gem_object_blt.o \ diff --git a/drivers/gpu/drm/i915/gem/i915_gem_fence.c b/drivers/gpu/drm/i915/gem/i915_gem_fence.c deleted file mode 100644 index 8ab842c80f99..000000000000 --- a/drivers/gpu/drm/i915/gem/i915_gem_fence.c +++ /dev/null @@ -1,95 +0,0 @@ -/* - * SPDX-License-Identifier: MIT - * - * Copyright © 2019 Intel Corporation - */ - -#include "i915_drv.h" -#include "i915_gem_object.h" - -struct stub_fence { - struct dma_fence dma; - struct i915_sw_fence chain; -}; - -static int __i915_sw_fence_call -stub_notify(struct i915_sw_fence *fence, enum i915_sw_fence_notify state) -{ - struct stub_fence *stub = container_of(fence, typeof(*stub), chain); - - switch (state) { - case FENCE_COMPLETE: - dma_fence_signal(&stub->dma); - break; - - case FENCE_FREE: - dma_fence_put(&stub->dma); - break; - } - - return NOTIFY_DONE; -} - -static const char *stub_driver_name(struct dma_fence *fence) -{ - return DRIVER_NAME; -} - -static const char *stub_timeline_name(struct dma_fence *fence) -{ - return "object"; -} - -static void stub_release(struct dma_fence *fence) -{ - struct stub_fence *stub = container_of(fence, typeof(*stub), dma); - - i915_sw_fence_fini(&stub->chain); - - BUILD_BUG_ON(offsetof(typeof(*stub), dma)); - dma_fence_free(&stub->dma); -} - -static const struct dma_fence_ops stub_fence_ops = { - .get_driver_name = stub_driver_name, - .get_timeline_name = stub_timeline_name, - .release = stub_release, -}; - -struct dma_fence * -i915_gem_object_lock_fence(struct drm_i915_gem_object *obj) -{ - struct stub_fence *stub; - - assert_object_held(obj); - - stub = kmalloc(sizeof(*stub), GFP_KERNEL); - if (!stub) - return NULL; - - i915_sw_fence_init(&stub->chain, stub_notify); - dma_fence_init(&stub->dma, &stub_fence_ops, &stub->chain.wait.lock, - 0, 0); - - if (i915_sw_fence_await_reservation(&stub->chain, - obj->base.resv, NULL, true, - i915_fence_timeout(to_i915(obj->base.dev)), - I915_FENCE_GFP) < 0) - goto err; - - dma_resv_add_excl_fence(obj->base.resv, &stub->dma); - - return &stub->dma; - -err: - stub_release(&stub->dma); - return NULL; -} - -void i915_gem_object_unlock_fence(struct drm_i915_gem_object *obj, - struct dma_fence *fence) -{ - struct stub_fence *stub = container_of(fence, typeof(*stub), dma); - - i915_sw_fence_commit(&stub->chain); -} diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h index fef0d62f3eb7..6c3f75adb53c 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h @@ -189,11 +189,6 @@ static inline void i915_gem_object_unlock(struct drm_i915_gem_object *obj) dma_resv_unlock(obj->base.resv); }

-struct dma_fence * -i915_gem_object_lock_fence(struct drm_i915_gem_object *obj); -void i915_gem_object_unlock_fence(struct drm_i915_gem_object *obj, - struct dma_fence *fence); - static inline void i915_gem_object_set_readonly(struct drm_i915_gem_object *obj) { diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 8373662e4b5f..eeb952889e4a 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -204,7 +204,6 @@ i915_gem_shmem_pread(struct drm_i915_gem_object *obj, { unsigned int needs_clflush; unsigned int idx, offset; - struct dma_fence *fence; char __user *user_data; u64 remain; int ret; @@ -213,19 +212,17 @@ i915_gem_shmem_pread(struct drm_i915_gem_object *obj, if (ret) return ret;

+ ret = i915_gem_object_pin_pages(obj); + if (ret) + goto err_unlock; + ret = i915_gem_object_prepare_read(obj, &needs_clflush); - if (ret) { - i915_gem_object_unlock(obj); - return ret; - } + if (ret) + goto err_unpin;

- fence = i915_gem_object_lock_fence(obj); i915_gem_object_finish_access(obj); i915_gem_object_unlock(obj);

- if (!fence) - return -ENOMEM; - remain = args->size; user_data = u64_to_user_ptr(args->data_ptr); offset = offset_in_page(args->offset); @@ -243,7 +240,13 @@ i915_gem_shmem_pread(struct drm_i915_gem_object *obj, offset = 0; }

- i915_gem_object_unlock_fence(obj, fence); + i915_gem_object_unpin_pages(obj); + return ret; + +err_unpin: + i915_gem_object_unpin_pages(obj); +err_unlock: + i915_gem_object_unlock(obj); return ret; }

@@ -271,48 +274,99 @@ gtt_user_read(struct io_mapping *mapping, return unwritten; }

-static int -i915_gem_gtt_pread(struct drm_i915_gem_object *obj, - const struct drm_i915_gem_pread *args) +static struct i915_vma *i915_gem_gtt_prepare(struct drm_i915_gem_object *obj, + struct drm_mm_node *node, + bool write) { struct drm_i915_private *i915 = to_i915(obj->base.dev); struct i915_ggtt *ggtt = &i915->ggtt; - intel_wakeref_t wakeref; - struct drm_mm_node node; - struct dma_fence *fence; - void __user *user_data; struct i915_vma *vma; - u64 remain, offset; + struct i915_gem_ww_ctx ww; int ret;

- wakeref = intel_runtime_pm_get(&i915->runtime_pm); + i915_gem_ww_ctx_init(&ww, true); +retry: vma = ERR_PTR(-ENODEV); + ret = i915_gem_object_lock(obj, &ww); + if (ret) + goto err_ww; + + i915_gem_object_set_to_gtt_domain(obj, write); if (!i915_gem_object_is_tiled(obj)) - vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0, - PIN_MAPPABLE | - PIN_NONBLOCK /* NOWARN */ | - PIN_NOEVICT); - if (!IS_ERR(vma)) { - node.start = i915_ggtt_offset(vma); - node.flags = 0; + vma = i915_gem_object_ggtt_pin_ww(obj, &ww, NULL, 0, 0, + PIN_MAPPABLE | + PIN_NONBLOCK /* NOWARN */ | + PIN_NOEVICT); + if (vma == ERR_PTR(-EDEADLK)) { + ret = -EDEADLK; + goto err_ww; + } else if (!IS_ERR(vma)) { + node->start = i915_ggtt_offset(vma); + node->flags = 0; } else { - ret = insert_mappable_node(ggtt, &node, PAGE_SIZE); + ret = insert_mappable_node(ggtt, node, PAGE_SIZE); if (ret) - goto out_rpm; - GEM_BUG_ON(!drm_mm_node_allocated(&node)); + goto err_ww; + GEM_BUG_ON(!drm_mm_node_allocated(node)); + vma = NULL; }

- ret = i915_gem_object_lock_interruptible(obj, NULL); - if (ret) - goto out_unpin; + ret = i915_gem_object_pin_pages(obj); + if (ret) { + if (drm_mm_node_allocated(node)) { + ggtt->vm.clear_range(&ggtt->vm, node->start, node->size); + remove_mappable_node(ggtt, node); + } else { + i915_vma_unpin(vma); + } + } + +err_ww: + if (ret == -EDEADLK) { + ret = i915_gem_ww_ctx_backoff(&ww); + if (!ret) + goto retry; + } + i915_gem_ww_ctx_fini(&ww);

- i915_gem_object_set_to_gtt_domain(obj, false); + return ret ? ERR_PTR(ret) : vma; +}

- fence = i915_gem_object_lock_fence(obj); - i915_gem_object_unlock(obj); - if (!fence) { - ret = -ENOMEM; - goto out_unpin; +static void i915_gem_gtt_cleanup(struct drm_i915_gem_object *obj, + struct drm_mm_node *node, + struct i915_vma *vma) +{ + struct drm_i915_private *i915 = to_i915(obj->base.dev); + struct i915_ggtt *ggtt = &i915->ggtt; + + i915_gem_object_unpin_pages(obj); + if (drm_mm_node_allocated(node)) { + ggtt->vm.clear_range(&ggtt->vm, node->start, node->size); + remove_mappable_node(ggtt, node); + } else { + i915_vma_unpin(vma); + } +} + +static int +i915_gem_gtt_pread(struct drm_i915_gem_object *obj, + const struct drm_i915_gem_pread *args) +{ + struct drm_i915_private *i915 = to_i915(obj->base.dev); + struct i915_ggtt *ggtt = &i915->ggtt; + intel_wakeref_t wakeref; + struct drm_mm_node node; + void __user *user_data; + struct i915_vma *vma; + u64 remain, offset; + int ret = 0; + + wakeref = intel_runtime_pm_get(&i915->runtime_pm); + + vma = i915_gem_gtt_prepare(obj, &node, false); + if (IS_ERR(vma)) { + ret = PTR_ERR(vma); + goto out_rpm; }

user_data = u64_to_user_ptr(args->data_ptr); @@ -349,14 +403,7 @@ i915_gem_gtt_pread(struct drm_i915_gem_object *obj, offset += page_length; }

- i915_gem_object_unlock_fence(obj, fence); -out_unpin: - if (drm_mm_node_allocated(&node)) { - ggtt->vm.clear_range(&ggtt->vm, node.start, node.size); - remove_mappable_node(ggtt, &node); - } else { - i915_vma_unpin(vma); - } + i915_gem_gtt_cleanup(obj, &node, vma); out_rpm: intel_runtime_pm_put(&i915->runtime_pm, wakeref); return ret; @@ -421,15 +468,10 @@ i915_gem_pread_ioctl(struct drm_device *dev, void *data, if (ret) goto out;

- ret = i915_gem_object_pin_pages(obj); - if (ret) - goto out; - ret = i915_gem_shmem_pread(obj, args); if (ret == -EFAULT || ret == -ENODEV) ret = i915_gem_gtt_pread(obj, args);

- i915_gem_object_unpin_pages(obj); out: i915_gem_object_put(obj); return ret; @@ -477,11 +519,10 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj, struct intel_runtime_pm *rpm = &i915->runtime_pm; intel_wakeref_t wakeref; struct drm_mm_node node; - struct dma_fence *fence; struct i915_vma *vma; u64 remain, offset; void __user *user_data; - int ret; + int ret = 0;

if (i915_gem_object_has_struct_page(obj)) { /* @@ -499,33 +540,10 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj, wakeref = intel_runtime_pm_get(rpm); }

- vma = ERR_PTR(-ENODEV); - if (!i915_gem_object_is_tiled(obj)) - vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0, - PIN_MAPPABLE | - PIN_NONBLOCK /* NOWARN */ | - PIN_NOEVICT); - if (!IS_ERR(vma)) { - node.start = i915_ggtt_offset(vma); - node.flags = 0; - } else { - ret = insert_mappable_node(ggtt, &node, PAGE_SIZE); - if (ret) - goto out_rpm; - GEM_BUG_ON(!drm_mm_node_allocated(&node)); - } - - ret = i915_gem_object_lock_interruptible(obj, NULL); - if (ret) - goto out_unpin; - - i915_gem_object_set_to_gtt_domain(obj, true); - - fence = i915_gem_object_lock_fence(obj); - i915_gem_object_unlock(obj); - if (!fence) { - ret = -ENOMEM; - goto out_unpin; + vma = i915_gem_gtt_prepare(obj, &node, true); + if (IS_ERR(vma)) { + ret = PTR_ERR(vma); + goto out_rpm; }

i915_gem_object_invalidate_frontbuffer(obj, ORIGIN_CPU); @@ -574,14 +592,7 @@ i915_gem_gtt_pwrite_fast(struct drm_i915_gem_object *obj, intel_gt_flush_ggtt_writes(ggtt->vm.gt); i915_gem_object_flush_frontbuffer(obj, ORIGIN_CPU);

- i915_gem_object_unlock_fence(obj, fence); -out_unpin: - if (drm_mm_node_allocated(&node)) { - ggtt->vm.clear_range(&ggtt->vm, node.start, node.size); - remove_mappable_node(ggtt, &node); - } else { - i915_vma_unpin(vma); - } + i915_gem_gtt_cleanup(obj, &node, vma); out_rpm: intel_runtime_pm_put(rpm, wakeref); return ret; @@ -621,7 +632,6 @@ i915_gem_shmem_pwrite(struct drm_i915_gem_object *obj, unsigned int partial_cacheline_write; unsigned int needs_clflush; unsigned int offset, idx; - struct dma_fence *fence; void __user *user_data; u64 remain; int ret; @@ -630,19 +640,17 @@ i915_gem_shmem_pwrite(struct drm_i915_gem_object *obj, if (ret) return ret;

+ ret = i915_gem_object_pin_pages(obj); + if (ret) + goto err_unlock; + ret = i915_gem_object_prepare_write(obj, &needs_clflush); - if (ret) { - i915_gem_object_unlock(obj); - return ret; - } + if (ret) + goto err_unpin;

- fence = i915_gem_object_lock_fence(obj); i915_gem_object_finish_access(obj); i915_gem_object_unlock(obj);

- if (!fence) - return -ENOMEM; - /* If we don't overwrite a cacheline completely we need to be * careful to have up-to-date data by first clflushing. Don't * overcomplicate things and flush the entire patch. @@ -670,8 +678,14 @@ i915_gem_shmem_pwrite(struct drm_i915_gem_object *obj, }

i915_gem_object_flush_frontbuffer(obj, ORIGIN_CPU); - i915_gem_object_unlock_fence(obj, fence);

+ i915_gem_object_unpin_pages(obj); + return ret; + +err_unpin: + i915_gem_object_unpin_pages(obj); +err_unlock: + i915_gem_object_unlock(obj); return ret; }

@@ -735,10 +749,6 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data, if (ret) goto err;

- ret = i915_gem_object_pin_pages(obj); - if (ret) - goto err; - ret = -EFAULT; /* We can only do the GTT pwrite on untiled buffers, as otherwise * it would end up going through the fenced access, and we'll get @@ -759,7 +769,6 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data, ret = i915_gem_shmem_pwrite(obj, args); }

- i915_gem_object_unpin_pages(obj); err: i915_gem_object_put(obj); return ret;

pin_map needs the ww lock, so ensure we pin both before submission.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Reviewed-by: Thomas Hellström thomas.hellstrom@linux.intel.com --- drivers/gpu/drm/i915/gem/i915_gem_object.h | 3 + drivers/gpu/drm/i915/gem/i915_gem_pages.c | 12 +++ .../gpu/drm/i915/gt/selftest_workarounds.c | 95 +++++++++++++------ 3 files changed, 80 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h index 6c3f75adb53c..983f2d4b2a85 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h @@ -437,6 +437,9 @@ void i915_gem_object_writeback(struct drm_i915_gem_object *obj); void *__must_check i915_gem_object_pin_map(struct drm_i915_gem_object *obj, enum i915_map_type type);

+void *__must_check i915_gem_object_pin_map_unlocked(struct drm_i915_gem_object *obj, + enum i915_map_type type); + void __i915_gem_object_flush_map(struct drm_i915_gem_object *obj, unsigned long offset, unsigned long size); diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c index e947d4c0da1f..a24617af3c93 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c @@ -400,6 +400,18 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj, goto out_unlock; }

+void *i915_gem_object_pin_map_unlocked(struct drm_i915_gem_object *obj, + enum i915_map_type type) +{ + void *ret; + + i915_gem_object_lock(obj, NULL); + ret = i915_gem_object_pin_map(obj, type); + i915_gem_object_unlock(obj); + + return ret; +} + void __i915_gem_object_flush_map(struct drm_i915_gem_object *obj, unsigned long offset, unsigned long size) diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c b/drivers/gpu/drm/i915/gt/selftest_workarounds.c index de6136bd10ac..a508614b2fd5 100644 --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c @@ -103,15 +103,13 @@ read_nonprivs(struct intel_context *ce, struct i915_vma *result) int err; int i;

- rq = intel_context_create_request(ce); + rq = i915_request_create(ce); if (IS_ERR(rq)) return rq;

- i915_vma_lock(result); err = i915_request_await_object(rq, result->obj, true); if (err == 0) err = i915_vma_move_to_active(result, rq, EXEC_OBJECT_WRITE); - i915_vma_unlock(result); if (err) goto err_rq;

@@ -176,10 +174,11 @@ static int check_whitelist(struct intel_context *ce) u32 *vaddr; int i;

- result = __vm_create_scratch_for_read(&engine->gt->ggtt->vm, PAGE_SIZE); + result = __vm_create_scratch_for_read_pinned(&engine->gt->ggtt->vm, PAGE_SIZE); if (IS_ERR(result)) return PTR_ERR(result);

+ i915_gem_object_lock(result->obj, NULL); vaddr = i915_gem_object_pin_map(result->obj, I915_MAP_WB); if (IS_ERR(vaddr)) { err = PTR_ERR(vaddr); @@ -219,6 +218,8 @@ static int check_whitelist(struct intel_context *ce) out_map: i915_gem_object_unpin_map(result->obj); out_put: + i915_vma_unpin(result); + i915_gem_object_unlock(result->obj); i915_vma_put(result); return err; } @@ -279,10 +280,14 @@ static int check_whitelist_across_reset(struct intel_engine_cs *engine, if (IS_ERR(ce)) return PTR_ERR(ce);

- err = igt_spinner_init(&spin, engine->gt); + err = intel_context_pin(ce); if (err) goto out_ctx;

+ err = igt_spinner_init(&spin, engine->gt); + if (err) + goto out_unpin; + err = check_whitelist(ce); if (err) { pr_err("Invalid whitelist *before* %s reset!\n", name); @@ -315,6 +320,13 @@ static int check_whitelist_across_reset(struct intel_engine_cs *engine, err = PTR_ERR(tmp); goto out_spin; } + err = intel_context_pin(tmp); + if (err) { + intel_context_put(tmp); + goto out_spin; + } + + intel_context_unpin(ce); intel_context_put(ce); ce = tmp;

@@ -327,6 +339,8 @@ static int check_whitelist_across_reset(struct intel_engine_cs *engine,

out_spin: igt_spinner_fini(&spin); +out_unpin: + intel_context_unpin(ce); out_ctx: intel_context_put(ce); return err; @@ -475,6 +489,7 @@ static int check_dirty_whitelist(struct intel_context *ce)

for (i = 0; i < engine->whitelist.count; i++) { u32 reg = i915_mmio_reg_offset(engine->whitelist.list[i].reg); + struct i915_gem_ww_ctx ww; u64 addr = scratch->node.start; struct i915_request *rq; u32 srm, lrm, rsvd; @@ -490,6 +505,29 @@ static int check_dirty_whitelist(struct intel_context *ce)

ro_reg = ro_register(reg);

+ i915_gem_ww_ctx_init(&ww, false); +retry: + cs = NULL; + err = i915_gem_object_lock(scratch->obj, &ww); + if (!err) + err = i915_gem_object_lock(batch->obj, &ww); + if (!err) + err = intel_context_pin_ww(ce, &ww); + if (err) + goto out; + + cs = i915_gem_object_pin_map(batch->obj, I915_MAP_WC); + if (IS_ERR(cs)) { + err = PTR_ERR(cs); + goto out_ctx; + } + + results = i915_gem_object_pin_map(scratch->obj, I915_MAP_WB); + if (IS_ERR(results)) { + err = PTR_ERR(results); + goto out_unmap_batch; + } + /* Clear non priv flags */ reg &= RING_FORCE_TO_NONPRIV_ADDRESS_MASK;

@@ -501,12 +539,6 @@ static int check_dirty_whitelist(struct intel_context *ce) pr_debug("%s: Writing garbage to %x\n", engine->name, reg);

- cs = i915_gem_object_pin_map(batch->obj, I915_MAP_WC); - if (IS_ERR(cs)) { - err = PTR_ERR(cs); - goto out_batch; - } - /* SRM original */ *cs++ = srm; *cs++ = reg; @@ -553,11 +585,12 @@ static int check_dirty_whitelist(struct intel_context *ce) i915_gem_object_flush_map(batch->obj); i915_gem_object_unpin_map(batch->obj); intel_gt_chipset_flush(engine->gt); + cs = NULL;

- rq = intel_context_create_request(ce); + rq = i915_request_create(ce); if (IS_ERR(rq)) { err = PTR_ERR(rq); - goto out_batch; + goto out_unmap_scratch; }

if (engine->emit_init_breadcrumb) { /* Be nice if we hang */ @@ -566,20 +599,16 @@ static int check_dirty_whitelist(struct intel_context *ce) goto err_request; }

- i915_vma_lock(batch); err = i915_request_await_object(rq, batch->obj, false); if (err == 0) err = i915_vma_move_to_active(batch, rq, 0); - i915_vma_unlock(batch); if (err) goto err_request;

- i915_vma_lock(scratch); err = i915_request_await_object(rq, scratch->obj, true); if (err == 0) err = i915_vma_move_to_active(scratch, rq, EXEC_OBJECT_WRITE); - i915_vma_unlock(scratch); if (err) goto err_request;

@@ -595,13 +624,7 @@ static int check_dirty_whitelist(struct intel_context *ce) pr_err("%s: Futzing %x timedout; cancelling test\n", engine->name, reg); intel_gt_set_wedged(engine->gt); - goto out_batch; - } - - results = i915_gem_object_pin_map(scratch->obj, I915_MAP_WB); - if (IS_ERR(results)) { - err = PTR_ERR(results); - goto out_batch; + goto out_unmap_scratch; }

GEM_BUG_ON(values[ARRAY_SIZE(values) - 1] != 0xffffffff); @@ -612,7 +635,7 @@ static int check_dirty_whitelist(struct intel_context *ce) pr_err("%s: Unable to write to whitelisted register %x\n", engine->name, reg); err = -EINVAL; - goto out_unpin; + goto out_unmap_scratch; } } else { rsvd = 0; @@ -678,15 +701,27 @@ static int check_dirty_whitelist(struct intel_context *ce)

err = -EINVAL; } -out_unpin: +out_unmap_scratch: i915_gem_object_unpin_map(scratch->obj); +out_unmap_batch: + if (cs) + i915_gem_object_unpin_map(batch->obj); +out_ctx: + intel_context_unpin(ce); +out: + if (err == -EDEADLK) { + err = i915_gem_ww_ctx_backoff(&ww); + if (!err) + goto retry; + } + i915_gem_ww_ctx_fini(&ww); if (err) break; }

if (igt_flush_test(engine->i915)) err = -EIO; -out_batch: + i915_vma_unpin_and_release(&batch, 0); out_scratch: i915_vma_unpin_and_release(&scratch, 0); @@ -820,7 +855,7 @@ static int scrub_whitelisted_registers(struct intel_context *ce) if (IS_ERR(batch)) return PTR_ERR(batch);

- cs = i915_gem_object_pin_map(batch->obj, I915_MAP_WC); + cs = i915_gem_object_pin_map_unlocked(batch->obj, I915_MAP_WC); if (IS_ERR(cs)) { err = PTR_ERR(cs); goto err_batch; @@ -955,11 +990,11 @@ check_whitelisted_registers(struct intel_engine_cs *engine, u32 *a, *b; int i, err;

- a = i915_gem_object_pin_map(A->obj, I915_MAP_WB); + a = i915_gem_object_pin_map_unlocked(A->obj, I915_MAP_WB); if (IS_ERR(a)) return PTR_ERR(a);

- b = i915_gem_object_pin_map(B->obj, I915_MAP_WB); + b = i915_gem_object_pin_map_unlocked(B->obj, I915_MAP_WB); if (IS_ERR(b)) { err = PTR_ERR(b); goto err_a;

From: Thomas Hellström thomas.hellstrom@intel.com

Stolen objects need to lock, and we may call put_pages when refcount drops to 0, ensure all calls are handled correctly.

Changes since v1: - Rebase on top of upstream changes.

Idea-from: Thomas Hellström thomas.hellstrom@intel.com Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Signed-off-by: Thomas Hellström thomas.hellstrom@linux.intel.com --- drivers/gpu/drm/i915/gem/i915_gem_object.h | 14 ++++++++++++++ drivers/gpu/drm/i915/gem/i915_gem_pages.c | 14 ++++++++++++-- drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 12 +++++++----- 3 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h index 983f2d4b2a85..74de195b57de 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h @@ -144,6 +144,20 @@ i915_gem_object_put(struct drm_i915_gem_object *obj)

#define assert_object_held(obj) dma_resv_assert_held((obj)->base.resv)

+/* + * If more than one potential simultaneous locker, assert held. + */ +static inline void assert_object_held_shared(struct drm_i915_gem_object *obj) +{ + /* + * Note mm list lookup is protected by + * kref_get_unless_zero(). + */ + if (IS_ENABLED(CONFIG_LOCKDEP) && + kref_read(&obj->base.refcount) > 0) + lockdep_assert_held(&obj->mm.lock); +} + static inline int __i915_gem_object_lock(struct drm_i915_gem_object *obj, struct i915_gem_ww_ctx *ww, bool intr) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c index a24617af3c93..2d0065fa6e80 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c @@ -19,7 +19,7 @@ void __i915_gem_object_set_pages(struct drm_i915_gem_object *obj, bool shrinkable; int i;

- lockdep_assert_held(&obj->mm.lock); + assert_object_held_shared(obj);

if (i915_gem_object_is_volatile(obj)) obj->mm.madv = I915_MADV_DONTNEED; @@ -70,6 +70,7 @@ void __i915_gem_object_set_pages(struct drm_i915_gem_object *obj, struct list_head *list; unsigned long flags;

+ lockdep_assert_held(&obj->mm.lock); spin_lock_irqsave(&i915->mm.obj_lock, flags);

i915->mm.shrink_count++; @@ -91,6 +92,8 @@ int ____i915_gem_object_get_pages(struct drm_i915_gem_object *obj) struct drm_i915_private *i915 = to_i915(obj->base.dev); int err;

+ assert_object_held_shared(obj); + if (unlikely(obj->mm.madv != I915_MADV_WILLNEED)) { drm_dbg(&i915->drm, "Attempting to obtain a purgeable object\n"); @@ -118,6 +121,8 @@ int __i915_gem_object_get_pages(struct drm_i915_gem_object *obj) if (err) return err;

+ assert_object_held_shared(obj); + if (unlikely(!i915_gem_object_has_pages(obj))) { GEM_BUG_ON(i915_gem_object_has_pinned_pages(obj));

@@ -145,7 +150,7 @@ void i915_gem_object_truncate(struct drm_i915_gem_object *obj) /* Try to discard unwanted pages */ void i915_gem_object_writeback(struct drm_i915_gem_object *obj) { - lockdep_assert_held(&obj->mm.lock); + assert_object_held_shared(obj); GEM_BUG_ON(i915_gem_object_has_pages(obj));

if (obj->ops->writeback) @@ -176,6 +181,8 @@ __i915_gem_object_unset_pages(struct drm_i915_gem_object *obj) { struct sg_table *pages;

+ assert_object_held_shared(obj); + pages = fetch_and_zero(&obj->mm.pages); if (IS_ERR_OR_NULL(pages)) return pages; @@ -203,6 +210,9 @@ int __i915_gem_object_put_pages_locked(struct drm_i915_gem_object *obj) if (i915_gem_object_has_pinned_pages(obj)) return -EBUSY;

+ /* May be called by shrinker from within get_pages() (on another bo) */ + assert_object_held_shared(obj); + i915_gem_object_release_mmap_offset(obj);

/* diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c index 7cdb32d881d9..b0597de206de 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c @@ -637,13 +637,15 @@ static int __i915_gem_object_create_stolen(struct intel_memory_region *mem, cache_level = HAS_LLC(mem->i915) ? I915_CACHE_LLC : I915_CACHE_NONE; i915_gem_object_set_cache_coherency(obj, cache_level);

- err = i915_gem_object_pin_pages(obj); - if (err) - return err; + if (WARN_ON(!i915_gem_object_trylock(obj))) + return -EBUSY;

- i915_gem_object_init_memory_region(obj, mem); + err = i915_gem_object_pin_pages(obj); + if (!err) + i915_gem_object_init_memory_region(obj, mem); + i915_gem_object_unlock(obj);

- return 0; + return err; }

static int _i915_gem_object_stolen_init(struct intel_memory_region *mem,

i915_gem_object_pin_map potentially needs a ww context, so ensure we have one we can revoke.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Reviewed-by: Thomas Hellström thomas.hellstrom@linux.intel.com --- drivers/gpu/drm/i915/gem/i915_gem_mman.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c index 163208a6260d..2561a2f1e54f 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c @@ -421,7 +421,9 @@ vm_access(struct vm_area_struct *area, unsigned long addr, { struct i915_mmap_offset *mmo = area->vm_private_data; struct drm_i915_gem_object *obj = mmo->obj; + struct i915_gem_ww_ctx ww; void *vaddr; + int err = 0;

if (i915_gem_object_is_readonly(obj) && write) return -EACCES; @@ -430,10 +432,18 @@ vm_access(struct vm_area_struct *area, unsigned long addr, if (addr >= obj->base.size) return -EINVAL;

+ i915_gem_ww_ctx_init(&ww, true); +retry: + err = i915_gem_object_lock(obj, &ww); + if (err) + goto out; + /* As this is primarily for debugging, let's focus on simplicity */ vaddr = i915_gem_object_pin_map(obj, I915_MAP_FORCE_WC); - if (IS_ERR(vaddr)) - return PTR_ERR(vaddr); + if (IS_ERR(vaddr)) { + err = PTR_ERR(vaddr); + goto out; + }

if (write) { memcpy(vaddr + addr, buf, len); @@ -443,6 +453,16 @@ vm_access(struct vm_area_struct *area, unsigned long addr, }

i915_gem_object_unpin_map(obj); +out: + if (err == -EDEADLK) { + err = i915_gem_ww_ctx_backoff(&ww); + if (!err) + goto retry; + } + i915_gem_ww_ctx_fini(&ww); + + if (err) + return err;

return len; }

We need to lock a few more objects, some temporarily, add ww lock where needed.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com Reviewed-by: Thomas Hellström thomas.hellstrom@linux.intel.com --- drivers/gpu/drm/i915/i915_perf.c | 56 ++++++++++++++++++++++++-------- 1 file changed, 43 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c index 2fd2c13b76ac..66f1f25119b5 100644 --- a/drivers/gpu/drm/i915/i915_perf.c +++ b/drivers/gpu/drm/i915/i915_perf.c @@ -1573,7 +1573,7 @@ static int alloc_oa_buffer(struct i915_perf_stream *stream) stream->oa_buffer.vma = vma;

stream->oa_buffer.vaddr = - i915_gem_object_pin_map(bo, I915_MAP_WB); + i915_gem_object_pin_map_unlocked(bo, I915_MAP_WB); if (IS_ERR(stream->oa_buffer.vaddr)) { ret = PTR_ERR(stream->oa_buffer.vaddr); goto err_unpin; @@ -1627,6 +1627,7 @@ static int alloc_noa_wait(struct i915_perf_stream *stream) const u32 base = stream->engine->mmio_base; #define CS_GPR(x) GEN8_RING_CS_GPR(base, x) u32 *batch, *ts0, *cs, *jump; + struct i915_gem_ww_ctx ww; int ret, i; enum { START_TS, @@ -1644,15 +1645,21 @@ static int alloc_noa_wait(struct i915_perf_stream *stream) return PTR_ERR(bo); }

+ i915_gem_ww_ctx_init(&ww, true); +retry: + ret = i915_gem_object_lock(bo, &ww); + if (ret) + goto out_ww; + /* * We pin in GGTT because we jump into this buffer now because * multiple OA config BOs will have a jump to this address and it * needs to be fixed during the lifetime of the i915/perf stream. */ - vma = i915_gem_object_ggtt_pin(bo, NULL, 0, 0, PIN_HIGH); + vma = i915_gem_object_ggtt_pin_ww(bo, &ww, NULL, 0, 0, PIN_HIGH); if (IS_ERR(vma)) { ret = PTR_ERR(vma); - goto err_unref; + goto out_ww; }

batch = cs = i915_gem_object_pin_map(bo, I915_MAP_WB); @@ -1786,12 +1793,19 @@ static int alloc_noa_wait(struct i915_perf_stream *stream) __i915_gem_object_release_map(bo);

stream->noa_wait = vma; - return 0; + goto out_ww;

err_unpin: i915_vma_unpin_and_release(&vma, 0); -err_unref: - i915_gem_object_put(bo); +out_ww: + if (ret == -EDEADLK) { + ret = i915_gem_ww_ctx_backoff(&ww); + if (!ret) + goto retry; + } + i915_gem_ww_ctx_fini(&ww); + if (ret) + i915_gem_object_put(bo); return ret; }

@@ -1834,6 +1848,7 @@ alloc_oa_config_buffer(struct i915_perf_stream *stream, { struct drm_i915_gem_object *obj; struct i915_oa_config_bo *oa_bo; + struct i915_gem_ww_ctx ww; size_t config_length = 0; u32 *cs; int err; @@ -1854,10 +1869,16 @@ alloc_oa_config_buffer(struct i915_perf_stream *stream, goto err_free; }

+ i915_gem_ww_ctx_init(&ww, true); +retry: + err = i915_gem_object_lock(obj, &ww); + if (err) + goto out_ww; + cs = i915_gem_object_pin_map(obj, I915_MAP_WB); if (IS_ERR(cs)) { err = PTR_ERR(cs); - goto err_oa_bo; + goto out_ww; }

cs = write_cs_mi_lri(cs, @@ -1885,19 +1906,28 @@ alloc_oa_config_buffer(struct i915_perf_stream *stream, NULL); if (IS_ERR(oa_bo->vma)) { err = PTR_ERR(oa_bo->vma); - goto err_oa_bo; + goto out_ww; }

oa_bo->oa_config = i915_oa_config_get(oa_config); llist_add(&oa_bo->node, &stream->oa_config_bos);

- return oa_bo; +out_ww: + if (err == -EDEADLK) { + err = i915_gem_ww_ctx_backoff(&ww); + if (!err) + goto retry; + } + i915_gem_ww_ctx_fini(&ww);

-err_oa_bo: - i915_gem_object_put(obj); + if (err) + i915_gem_object_put(obj); err_free: - kfree(oa_bo); - return ERR_PTR(err); + if (err) { + kfree(oa_bo); + return ERR_PTR(err); + } + return oa_bo; }

static struct i915_vma *