[Intel-gfx] [PATCH v3 2/2] drm/i915/dgfx: Release mmap on rpm suspend
Gupta, Anshuman
anshuman.gupta at intel.com
Mon Sep 12 06:05:28 UTC 2022
> -----Original Message-----
> From: Auld, Matthew <matthew.auld at intel.com>
> Sent: Friday, September 9, 2022 9:06 PM
> To: Gupta, Anshuman <anshuman.gupta at intel.com>; intel-
> gfx at lists.freedesktop.org
> Cc: joonas.lahtinen at linux.intel.com; Vivi, Rodrigo <rodrigo.vivi at intel.com>;
> Nilawar, Badal <badal.nilawar at intel.com>; chris at chris-wilson.co.uk
> Subject: Re: [PATCH v3 2/2] drm/i915/dgfx: Release mmap on rpm suspend
>
> On 09/09/2022 12:24, Anshuman Gupta wrote:
> > Release all mmap mapping for all lmem objects which are associated
> > with userfault such that, while pcie function in D3hot, any access to
> > memory mappings will raise a userfault.
> >
> > Runtime resume the dgpu(when gem object lies in lmem).
> > This will transition the dgpu graphics function to D0 state if it was
> > in D3 in order to access the mmap memory mappings.
> >
> > v2:
> > - Squashes the patches. [Matt Auld]
> > - Add adequate locking for lmem_userfault_list addition. [Matt Auld]
> > - Reused obj->userfault_count to avoid double addition. [Matt Auld]
> > - Added i915_gem_object_lock to check
> > i915_gem_object_is_lmem. [Matt Auld]
> >
> > v3:
> > - Use i915_ttm_cpu_maps_iomem. [Matt Auld]
> > - Fix 'ret == 0 to ret == VM_FAULT_NOPAGE'. [Matt Auld]
> > - Reuse obj->userfault_count as a bool 0 or 1. [Matt Auld]
> > - Delete the mmaped obj from lmem_userfault_list in obj
> > destruction path. [Matt Auld]
> > - Get a wakeref for object destruction patch. [Matt Auld]
> > - Use intel_wakeref_auto to delay runtime PM. [Matt Auld]
> >
> > PCIe Specs 5.3.1.4.1
> >
> > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/6331
> > Cc: Matthew Auld <matthew.auld at intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > Signed-off-by: Anshuman Gupta <anshuman.gupta at intel.com>
> > ---
> > drivers/gpu/drm/i915/gem/i915_gem_mman.c | 18 ++++++--
> > drivers/gpu/drm/i915/gem/i915_gem_mman.h | 1 +
> > drivers/gpu/drm/i915/gem/i915_gem_object.c | 2 +-
> > .../gpu/drm/i915/gem/i915_gem_object_types.h | 3 +-
> > drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 46 ++++++++++++++++---
> > drivers/gpu/drm/i915/gt/intel_gt.c | 2 +
> > drivers/gpu/drm/i915/gt/intel_gt_types.h | 3 ++
> > drivers/gpu/drm/i915/i915_driver.c | 1 -
> > drivers/gpu/drm/i915/i915_gem.c | 5 ++
> > 9 files changed, 68 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > index 2be222c03c82..55a4e9fba5ba 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > @@ -550,13 +550,10 @@ void i915_gem_object_release_mmap_gtt(struct
> drm_i915_gem_object *obj)
> > intel_runtime_pm_put(&i915->runtime_pm, wakeref);
> > }
> >
> > -void i915_gem_object_release_mmap_offset(struct drm_i915_gem_object
> > *obj)
> > +void __i915_gem_object_release_mmap_offset(struct drm_i915_gem_object
> > +*obj)
> > {
> > struct i915_mmap_offset *mmo, *mn;
> >
> > - if (obj->ops->unmap_virtual)
> > - obj->ops->unmap_virtual(obj);
> > -
> > spin_lock(&obj->mmo.lock);
> > rbtree_postorder_for_each_entry_safe(mmo, mn,
> > &obj->mmo.offsets, offset) { @@ -
> 573,6 +570,19 @@ void
> > i915_gem_object_release_mmap_offset(struct drm_i915_gem_object *obj)
> > spin_lock(&obj->mmo.lock);
> > }
> > spin_unlock(&obj->mmo.lock);
> > +
> > + if (obj->userfault_count) {
> > + list_del(&obj->userfault_link);
> > + obj->userfault_count = 0;
> > + }
> > +}
> > +
> > +void i915_gem_object_release_mmap_offset(struct drm_i915_gem_object
> > +*obj) {
> > + if (obj->ops->unmap_virtual)
> > + obj->ops->unmap_virtual(obj);
> > +
> > + __i915_gem_object_release_mmap_offset(obj);
> > }
> >
> > static struct i915_mmap_offset *
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.h
> > b/drivers/gpu/drm/i915/gem/i915_gem_mman.h
> > index efee9e0d2508..271039fdf875 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.h
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.h
> > @@ -27,6 +27,7 @@ int i915_gem_dumb_mmap_offset(struct drm_file
> *file_priv,
> > void __i915_gem_object_release_mmap_gtt(struct drm_i915_gem_object
> *obj);
> > void i915_gem_object_release_mmap_gtt(struct drm_i915_gem_object
> > *obj);
> >
> > +void __i915_gem_object_release_mmap_offset(struct drm_i915_gem_object
> > +*obj);
> > void i915_gem_object_release_mmap_offset(struct drm_i915_gem_object
> > *obj);
> >
> > #endif
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > index 389e9f157ca5..f6e60cc86b9e 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > @@ -238,7 +238,7 @@ static void __i915_gem_object_free_mmaps(struct
> drm_i915_gem_object *obj)
> > {
> > /* Skip serialisation and waking the device if known to be not
> > used. */
> >
> > - if (obj->userfault_count)
> > + if (obj->userfault_count && !IS_DGFX(to_i915(obj->base.dev)))
> > i915_gem_object_release_mmap_gtt(obj);
> >
> > if (!RB_EMPTY_ROOT(&obj->mmo.offsets)) { 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 9f6b14ec189a..40305e2bcd49 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> > @@ -298,7 +298,8 @@ struct drm_i915_gem_object {
> > };
> >
> > /**
> > - * Whether the object is currently in the GGTT mmap.
> > + * Whether the object is currently in the GGTT or any other supported
> > + * fake offset mmap backed by lmem.
> > */
> > unsigned int userfault_count;
> > struct list_head userfault_link;
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> > index bc9c432edffe..bfb2074d65ae 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> > @@ -509,9 +509,17 @@ static int i915_ttm_shrink(struct
> drm_i915_gem_object *obj, unsigned int flags)
> > static void i915_ttm_delete_mem_notify(struct ttm_buffer_object *bo)
> > {
> > struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
> > + intel_wakeref_t wakeref = 0;
> >
> > if (likely(obj)) {
> > + if (i915_ttm_cpu_maps_iomem(bo->resource))
> > + wakeref =
> > +intel_runtime_pm_get(&to_i915(obj->base.dev)->runtime_pm);
> > +
> > __i915_gem_object_pages_fini(obj);
> > +
> > + if (wakeref)
> > + intel_runtime_pm_put(&to_i915(obj->base.dev)-
> >runtime_pm,
> > +wakeref);
> > +
> > i915_ttm_free_cached_io_rsgt(obj);
> > }
> > }
> > @@ -981,6 +989,7 @@ static vm_fault_t vm_fault_ttm(struct vm_fault *vmf)
> > struct ttm_buffer_object *bo = area->vm_private_data;
> > struct drm_device *dev = bo->base.dev;
> > struct drm_i915_gem_object *obj;
> > + intel_wakeref_t wakeref = 0;
> > vm_fault_t ret;
> > int idx;
> >
> > @@ -990,16 +999,22 @@ static vm_fault_t vm_fault_ttm(struct vm_fault
> > *vmf)
> >
> > /* Sanity check that we allow writing into this object */
> > if (unlikely(i915_gem_object_is_readonly(obj) &&
> > - area->vm_flags & VM_WRITE))
> > - return VM_FAULT_SIGBUS;
> > + area->vm_flags & VM_WRITE)) {
> > + ret = VM_FAULT_SIGBUS;
> > + goto out_rpm;
> > + }
> >
> > ret = ttm_bo_vm_reserve(bo, vmf);
> > if (ret)
> > - return ret;
> > + goto out_rpm;
> > +
> > + if (i915_ttm_cpu_maps_iomem(bo->resource))
> > + wakeref =
> > +intel_runtime_pm_get(&to_i915(obj->base.dev)->runtime_pm);
>
> Could maybe move this down a bit. No need to grab the wakeref if the object is
> marked as DONTNEED.
>
> >
> > if (obj->mm.madv != I915_MADV_WILLNEED) {
> > dma_resv_unlock(bo->base.resv);
> > - return VM_FAULT_SIGBUS;
> > + ret = VM_FAULT_SIGBUS;
> > + goto out_rpm;
> > }
> >
> > if (!i915_ttm_resource_mappable(bo->resource)) { @@ -1023,7 +1038,8
> > @@ static vm_fault_t vm_fault_ttm(struct vm_fault *vmf)
> > if (err) {
> > drm_dbg(dev, "Unable to make resource CPU
> accessible\n");
> > dma_resv_unlock(bo->base.resv);
> > - return VM_FAULT_SIGBUS;
> > + ret = VM_FAULT_SIGBUS;
> > + goto out_rpm;
> > }
> > }
> >
> > @@ -1034,12 +1050,30 @@ static vm_fault_t vm_fault_ttm(struct vm_fault
> *vmf)
> > } else {
> > ret = ttm_bo_vm_dummy_page(vmf, vmf->vma-
> >vm_page_prot);
> > }
> > +
> > if (ret == VM_FAULT_RETRY && !(vmf->flags &
> FAULT_FLAG_RETRY_NOWAIT))
> > - return ret;
> > + goto out_rpm;
> > +
> > + /* ttm_bo_vm_reserve() already has dma_resv_lock */
> > + if (ret == VM_FAULT_NOPAGE && wakeref && !obj->userfault_count) {
> > + obj->userfault_count = 1;
> > + mutex_lock(&to_gt(to_i915(obj->base.dev))-
> >lmem_userfault_lock);
> > + list_add(&obj->userfault_link, &to_gt(to_i915(obj->base.dev))-
> >lmem_userfault_list);
> > + mutex_unlock(&to_gt(to_i915(obj->base.dev))-
> >lmem_userfault_lock);
> > + }
> > +
> > + if (wakeref && CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND)
> > + intel_wakeref_auto(&to_gt(to_i915(obj->base.dev))-
> >userfault_wakeref,
> > +
> > +msecs_to_jiffies_timeout(CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND));
> >
> > i915_ttm_adjust_lru(obj);
> >
> > dma_resv_unlock(bo->base.resv);
> > +
> > +out_rpm:
> > + if (wakeref)
> > + intel_runtime_pm_put(&to_i915(obj->base.dev)->runtime_pm,
> wakeref);
> > +
> > return ret;
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c
> > b/drivers/gpu/drm/i915/gt/intel_gt.c
> > index 1ce344cfa827..ee9ee815f505 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> > @@ -39,6 +39,8 @@ static void __intel_gt_init_early(struct intel_gt *gt)
> > {
> > spin_lock_init(>->irq_lock);
> >
> > + INIT_LIST_HEAD(>->lmem_userfault_list);
> > + mutex_init(>->lmem_userfault_lock);
> > INIT_LIST_HEAD(>->closed_vma);
> > spin_lock_init(>->closed_lock);
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > index e6a662f9d7c0..a2d87e742161 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> > @@ -132,6 +132,9 @@ struct intel_gt {
> > struct intel_wakeref wakeref;
> > atomic_t user_wakeref;
> >
> > + struct mutex lmem_userfault_lock; /* Protects access to usefault list */
> > + struct list_head lmem_userfault_list;
>
> Probably needs a big comment explaining how this all works, since it seems quite
> tricky.
>
> "If we are outside of the runtime suspend path, access to @lmem_userfault_list
> requires always first grabbing the runtime pm, to ensure we can't race against
> runtime suspend removing items. Once we have that we also need to grab
> @lmem_userfault_lock, at which point we have exclusive access. The runtime
> suspend path is special since it doens't really hold any locks, but instead has
> exlusive access by virtue of all other accesses requiring holding the runtime pm."
>
> Also according to that we are then missing holding lmem_userfault_lock in
> i915_gem_object_release_mmap_offset(), for the object destruction case. We
> are only holding the runtime pm, which only saves us from runtime suspend, and
> not some other concurrent user, like a different object destroy or fault touching
> the list.
Sure I will update this comment.
>
> > +
> > struct list_head closed_vma;
> > spinlock_t closed_lock; /* guards the list of closed_vma */
> >
> > diff --git a/drivers/gpu/drm/i915/i915_driver.c
> b/drivers/gpu/drm/i915/i915_driver.c
> > index 1332c70370a6..81699aa505e2 100644
> > --- a/drivers/gpu/drm/i915/i915_driver.c
> > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > @@ -1591,7 +1591,6 @@ static int intel_runtime_suspend(struct device
> *kdev)
> > return -ENODEV;
> >
> > drm_dbg(&dev_priv->drm, "Suspending device\n");
> > -
> > disable_rpm_wakeref_asserts(rpm);
> >
> > /*
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c
> > index 70f082f7911a..01f23c0e7fec 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -842,6 +842,11 @@ void i915_gem_runtime_suspend(struct
> drm_i915_private *i915)
> > &to_gt(i915)->ggtt->userfault_list,
> userfault_link)
> > __i915_gem_object_release_mmap_gtt(obj);
> >
> > + list_for_each_entry_safe(obj, on,
> > + &to_gt(i915)->lmem_userfault_list,
> userfault_link) {
> > + __i915_gem_object_release_mmap_offset(obj);
>
> drm_vma_node_unmap(&bo->base.vma_node, bdev->dev_mapping);
>
> I don't think mm.offsets is tracking the ttm node...
Thanks for pointing it, I am surprised in this case how existing implementation of
i915_gem_object_release_mmap_offset (object destruction path)
is releasing the mmap mapping with mmo.offset, do we need to change there as well ?
Thanks,
Anshuman Gupta.
>
> > + }
> > +
> > /*
> > * The fence will be lost when the device powers down. If any were
> > * in use by hardware (i.e. they are pinned), we should not be powering
More information about the Intel-gfx
mailing list