[Intel-gfx] [PATCH v9 30/70] drm/i915: Fix pread/pwrite to work with new locking rules.
Daniel Vetter
daniel at ffwll.ch
Wed Mar 24 14:45:06 UTC 2021
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 at linux.intel.com>
> Reviewed-by: Thomas Hellström <thomas.hellstrom at linux.intel.com>
Same here, I picked the old version from there
https://lore.kernel.org/intel-gfx/20210128162612.927917-31-maarten.lankhorst@linux.intel.com/
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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list