Re: [Intel-gfx] [PATCH v9 30/70] drm/i915: Fix pread/pwrite to work with new locking rules.

On Tue, Mar 23, 2021 at 04:50:19PM +0100, Maarten Lankhorst wrote:

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

Same here, I picked the old version from there

https://lore.kernel.org/intel-gfx/20210128162612.927917-31-maarten.lankhorst...

because too much conflicts with this version here. -Daniel

---

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; -- 2.31.0

---

Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx