Re: [PATCH 3/4] 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 15, 2017 at 09:25:08AM +0100, Chris Wilson wrote:
> Actually transferring from shmemfs to the physically contiguous set of
> pages should be wholly guarded by its obj->mm.lock!
> 
> v2: Remember to free the old pages.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 44 +++++++++++++++++++++++++++--------------
>  1 file changed, 29 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 31cbe78171a9..7f3be5deeb5e 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,45 @@ 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);
> -	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);
> +
> +	i915_gem_object_ops.put_pages(obj, pages);

Leak sorted, so this lgtm.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +	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

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