Re: [PATCH] drm/i915: Make i915_gem_object_phys_attach() use obj->mm.lock more appropriately

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Jun 08, 2017 at 11:53:44AM +0100, Chris Wilson wrote:
> Actually transferring from shmemfs to the physically contiguous set of
> pages should be wholly guarded by its obj->mm.lock!
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 43 +++++++++++++++++++++++++++--------------
>  1 file changed, 28 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index aff449807399..db168e395e8e 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -574,7 +574,8 @@ int
>  i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
>  			    int align)
>  {
> -	int ret;
> +	struct sg_table *pages;
> +	int err;
>  
>  	if (align > obj->base.size)
>  		return -EINVAL;
> @@ -582,32 +583,44 @@ i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
>  	if (obj->ops == &i915_gem_phys_ops)
>  		return 0;
>  
> -	if (obj->mm.madv != I915_MADV_WILLNEED)
> -		return -EFAULT;
> -
> -	if (obj->base.filp == NULL)
> +	if (obj->ops != &i915_gem_object_ops)
>  		return -EINVAL;
>  
> -	ret = i915_gem_object_unbind(obj);
> -	if (ret)
> -		return ret;
> +	err = i915_gem_object_unbind(obj);
> +	if (err)
> +		return err;
> +
> +	mutex_lock(&obj->mm.lock);
> +
> +	if (obj->mm.quirked) {
> +		err = -EFAULT;
> +		goto err_unlock;
> +	}
>  
> -	__i915_gem_object_put_pages(obj, I915_MM_NORMAL);

Are we now leaking the old mm.pages?

> -	if (obj->mm.pages)
> -		return -EBUSY;
> +	if (obj->mm.mapping) {
> +		err = -EBUSY;
> +		goto err_unlock;
> +	}
>  
> -	GEM_BUG_ON(obj->ops != &i915_gem_object_ops);
> +	pages = obj->mm.pages;
>  	obj->ops = &i915_gem_phys_ops;
>  
> -	ret = i915_gem_object_pin_pages(obj);
> -	if (ret)
> +	err = __i915_gem_object_get_pages(obj);
> +	if (err)
>  		goto err_xfer;
>  
> +	/* Perma-pin (until release) the physical set of pages */
> +	__i915_gem_object_pin_pages(obj);
> +
> +	mutex_unlock(&obj->mm.lock);
>  	return 0;
>  
>  err_xfer:
>  	obj->ops = &i915_gem_object_ops;
> -	return ret;
> +	obj->mm.pages = pages;
> +err_unlock:
> +	mutex_unlock(&obj->mm.lock);
> +	return err;
>  }
>  
>  static int
> -- 
> 2.11.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx