> -----Original Message-----
> From: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>
> Sent: Tuesday, September 13, 2022 7:00 PM
> To: Gupta, Anshuman <anshuman.gupta@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; chris@xxxxxxxxxxxxxxxxxx; Auld, Matthew
> <matthew.auld@xxxxxxxxx>; Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>
> Subject: Re: [PATCH v4 2/2] drm/i915/dgfx: Release mmap on rpm
> suspend
>
> Hi Anshuman,
>
> [...]
>
> > 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,18 +1000,24 @@ 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;
>
> we don't need for this change, wakeref is 0 here.
>
> > + }
> >
> > ret = ttm_bo_vm_reserve(bo, vmf);
> > if (ret)
> > - return ret;
> > + goto out_rpm;
>
> Same here.
>
> > if (obj->mm.madv != I915_MADV_WILLNEED) {
> > dma_resv_unlock(bo->base.resv);
> > - return VM_FAULT_SIGBUS;
> > + ret = VM_FAULT_SIGBUS;
> > + goto out_rpm;
>
> Same here.
>
> > }
> >
> > + if (i915_ttm_cpu_maps_iomem(bo->resource))
> > + wakeref =
> > +intel_runtime_pm_get(&to_i915(obj->base.dev)->runtime_pm);
> > +
> > if (!i915_ttm_resource_mappable(bo->resource)) {
> > int err = -ENODEV;
> > int i;
>
> [...]
>
> > diff --git a/drivers/gpu/drm/i915/i915_driver.c
> > b/drivers/gpu/drm/i915/i915_driver.c
> > index 8262bfb2a2d9..897148880acc 100644
> > --- a/drivers/gpu/drm/i915/i915_driver.c
> > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > @@ -1634,7 +1634,6 @@ static int intel_runtime_suspend(struct device
> *kdev)
> > return -ENODEV;
> >
> > drm_dbg(&dev_priv->drm, "Suspending device\n");
> > -
>
> Please remove this change.
>
> > disable_rpm_wakeref_asserts(rpm);
> >
> > /*
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c
> > b/drivers/gpu/drm/i915/i915_gem.c index 3463dd795950..c3a83b234b6e
> > 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_runtime_pm_object_release_mmap_offset(obj);
> > + }
>
> Don't need for brackets here.
>
> I see that all Matt's suggestions have been addressed. From v3 this latest release
> was the biggest concern. From my side looks all correct.
>
> would be nice if you add the cleanups above, besides that:
>
> Reviewed-by: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>
>
> Thanks and thanks Matt for the reviews.
Thanks matt and andi for review.
I will do the cleanup and send a new rev.
Thanks,
Anshuman Gupta
>
> Andi
>
> > +
> > /*
> > * 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
> > --
> > 2.26.2
