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@xxxxxxxxxxxxxxx> > Reviewed-by: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx> Same here, I picked the old version from there https://lore.kernel.org/intel-gfx/20210128162612.927917-31-maarten.lankhorst@xxxxxxxxxxxxxxx/ 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@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
