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

[Andi Shyti <andi.shyti@linux.intel.com>]

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@linux.intel.com>

Thanks and thanks Matt for the reviews.

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