Re: [PATCH] drm/i915: Use generic_access_phys

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, May 26, 2021 at 05:11:06PM +0200, Daniel Vetter wrote:
> Since
> 
> commit 96667f8a4382db9ed042332ca6ee165ae9b91307
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Fri Nov 27 17:41:21 2020 +0100
> 
>     mm: Close race in generic_access_phys
> 
> it is race-free and can therefore be safely used for dynamic mappings
> like we have too.
> 
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: "Christian König" <christian.koenig@amd.com>
> Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
> Cc: Michel Lespinasse <walken@google.com>

This turned out to be a bad idea, because we stuff normal memory into our
io mappings (how well does that work on other archs, no idea). That would
be fixable, but then again we have our issue with using multiple
inconsistent mmaps, and at that point it's probably a lost cause :-(
-Daniel

> ---
>  drivers/gpu/drm/i915/gem/i915_gem_mman.c | 60 +++---------------------
>  1 file changed, 6 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> index f6fe5cb01438..717798293044 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> @@ -414,58 +414,6 @@ static vm_fault_t vm_fault_gtt(struct vm_fault *vmf)
>  	return i915_error_to_vmf_fault(ret);
>  }
>  
> -static int
> -vm_access(struct vm_area_struct *area, unsigned long addr,
> -	  void *buf, int len, int write)
> -{
> -	struct i915_mmap_offset *mmo = area->vm_private_data;
> -	struct drm_i915_gem_object *obj = mmo->obj;
> -	struct i915_gem_ww_ctx ww;
> -	void *vaddr;
> -	int err = 0;
> -
> -	if (i915_gem_object_is_readonly(obj) && write)
> -		return -EACCES;
> -
> -	addr -= area->vm_start;
> -	if (addr >= obj->base.size)
> -		return -EINVAL;
> -
> -	i915_gem_ww_ctx_init(&ww, true);
> -retry:
> -	err = i915_gem_object_lock(obj, &ww);
> -	if (err)
> -		goto out;
> -
> -	/* As this is primarily for debugging, let's focus on simplicity */
> -	vaddr = i915_gem_object_pin_map(obj, I915_MAP_FORCE_WC);
> -	if (IS_ERR(vaddr)) {
> -		err = PTR_ERR(vaddr);
> -		goto out;
> -	}
> -
> -	if (write) {
> -		memcpy(vaddr + addr, buf, len);
> -		__i915_gem_object_flush_map(obj, addr, len);
> -	} else {
> -		memcpy(buf, vaddr + addr, len);
> -	}
> -
> -	i915_gem_object_unpin_map(obj);
> -out:
> -	if (err == -EDEADLK) {
> -		err = i915_gem_ww_ctx_backoff(&ww);
> -		if (!err)
> -			goto retry;
> -	}
> -	i915_gem_ww_ctx_fini(&ww);
> -
> -	if (err)
> -		return err;
> -
> -	return len;
> -}
> -
>  void __i915_gem_object_release_mmap_gtt(struct drm_i915_gem_object *obj)
>  {
>  	struct i915_vma *vma;
> @@ -801,14 +749,18 @@ static void vm_close(struct vm_area_struct *vma)
>  
>  static const struct vm_operations_struct vm_ops_gtt = {
>  	.fault = vm_fault_gtt,
> -	.access = vm_access,
> +#ifdef CONFIG_HAVE_IOREMAP_PROT
> +	.access = generic_access_phys
> +#endif
>  	.open = vm_open,
>  	.close = vm_close,
>  };
>  
>  static const struct vm_operations_struct vm_ops_cpu = {
>  	.fault = vm_fault_cpu,
> -	.access = vm_access,
> +#ifdef CONFIG_HAVE_IOREMAP_PROT
> +	.access = generic_access_phys
> +#endif
>  	.open = vm_open,
>  	.close = vm_close,
>  };
> -- 
> 2.31.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch