Re: [PATCH] drm/i915: Guard pages being reaped by OOM whilst binding-to-GTT

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Nov 09, 2012 at 08:58:50AM +0000, Chris Wilson wrote:
> In the circumstances that the shrinker is allowed to steal the mutex
> in order to reap pages, we need to be careful to prevent it operating on
> the current object and shooting ourselves in the foot.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Ok, I this is definitely the major one that I've thought of, but I think
I've found another place that need a similar treatment:

i915_gem_fault: In between bind_to_gtt and vm_insert_pfn we have a call to
get_fence, which might potentially call kmalloc due to a pending request.
I know, it's highly unlikely that this will ever matter, but I think if we
braket this with a pin/unpin, it will nicely serve as a reminder where to
reserve/unreserve and object, in case we ever switch to a per-object
locking scheme.

Otherwise I can't poke holes into this any more ...

Cheers, Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem.c |    9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 159dca5..f225583 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3091,6 +3091,8 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
>  	if (ret)
>  		return ret;
>  
> +	i915_gem_object_pin_pages(obj);
> +
>   search_free:
>  	if (map_and_fenceable)
>  		free_space =
> @@ -3121,14 +3123,17 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
>  					       obj->cache_level,
>  					       map_and_fenceable,
>  					       nonblocking);
> -		if (ret)
> +		if (ret) {
> +			i915_gem_object_unpin_pages(obj);
>  			return ret;
> +		}
>  
>  		goto search_free;
>  	}
>  	if (WARN_ON(!i915_gem_valid_gtt_space(dev,
>  					      obj->gtt_space,
>  					      obj->cache_level))) {
> +		i915_gem_object_unpin_pages(obj);
>  		drm_mm_put_block(obj->gtt_space);
>  		obj->gtt_space = NULL;
>  		return -EINVAL;
> @@ -3137,6 +3142,7 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
>  
>  	ret = i915_gem_gtt_prepare_object(obj);
>  	if (ret) {
> +		i915_gem_object_unpin_pages(obj);
>  		drm_mm_put_block(obj->gtt_space);
>  		obj->gtt_space = NULL;
>  		return ret;
> @@ -3159,6 +3165,7 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
>  
>  	obj->map_and_fenceable = mappable && fenceable;
>  
> +	i915_gem_object_unpin_pages(obj);
>  	trace_i915_gem_object_bind(obj, map_and_fenceable);
>  	i915_gem_verify_gtt(dev);
>  	return 0;
> -- 
> 1.7.10.4
> 
> _______________________________________________
> 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