Re: [PATCH] drm/i915: Only discard backing storage on releasing the last ref

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, May 22, 2014 at 09:16:52AM +0100, Chris Wilson wrote:
> Before purging our pages (as opposed to copying back the contents from
> the GPU), make sure that there is not an exposed CPU mmapping through
> which the user can inspect the results.
> 
> Regression from
> 
> commit 5537252b6b6d71fb1a8ed7395a8e5babf91953fd
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Tue Mar 25 13:23:06 2014 +0000
> 
>     drm/i915: Invalidate our pages under memory pressure
> 
> Testcase: igt/gem_mmap/new-object
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=79005
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Tested-by: Guo Jinxian <jinxianx.guo@intel.com>

Queued for -next, thanks for the patch.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 9cb7c1908279..31b9a04bd543 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4668,6 +4668,30 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
>  	return obj;
>  }
>  
> +static bool discard_backing_storage(struct drm_i915_gem_object *obj)
> +{
> +	/* If we are the last user of the backing storage (be it shmemfs
> +	 * pages or stolen etc), we know that the pages are going to be
> +	 * immediately released. In this case, we can then skip copying
> +	 * back the contents from the GPU.
> +	 */
> +
> +	if (obj->madv != I915_MADV_WILLNEED)
> +		return false;
> +
> +	if (obj->base.filp == NULL)
> +		return true;
> +
> +	/* At first glance, this looks racy, but then again so would be
> +	 * userspace racing mmap against close. However, the first external
> +	 * reference to the filp can only be obtained through the
> +	 * i915_gem_mmap_ioctl() which safeguards us against the user
> +	 * acquiring such a reference whilst we are in the middle of
> +	 * freeing the object.
> +	 */
> +	return atomic_long_read(&obj->base.filp->f_count) == 1;
> +}
> +
>  void i915_gem_free_object(struct drm_gem_object *gem_obj)
>  {
>  	struct drm_i915_gem_object *obj = to_intel_bo(gem_obj);
> @@ -4705,7 +4729,7 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
>  
>  	if (WARN_ON(obj->pages_pin_count))
>  		obj->pages_pin_count = 0;
> -	if (obj->madv != __I915_MADV_PURGED)
> +	if (discard_backing_storage(obj))
>  		obj->madv = I915_MADV_DONTNEED;
>  	i915_gem_object_put_pages(obj);
>  	i915_gem_object_free_mmap_offset(obj);
> -- 
> 2.0.0.rc2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch