[Intel-gfx] [PATCH v4 2/2] drm/i915/dgfx: Release mmap on rpm suspend

[Gupta, Anshuman]

> -----Original Message-----
> From: Andi Shyti <andi.shyti at linux.intel.com>
> Sent: Tuesday, September 13, 2022 7:00 PM
> To: Gupta, Anshuman <anshuman.gupta at intel.com>
> Cc: intel-gfx at lists.freedesktop.org; chris at chris-wilson.co.uk; Auld, Matthew
> <matthew.auld at intel.com>; Vivi, Rodrigo <rodrigo.vivi at intel.com>
> Subject: Re: [Intel-gfx] [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 at linux.intel.com>
> 
> 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