[PATCH 1/2] drm/i915: Defer assignment of obj->gtt_space until after all possible mallocs

[PATCH 1/2] drm/i915: Defer assignment of obj->gtt_space until after all possible mallocs

[Chris Wilson]

As we may invoke the shrinker whilst trying to allocate memory to hold
the gtt_space for this object, we need to be careful not to mark the
drm_mm_node as activated (by assigning it to this object) before we
have finished our sequence of allocations.

Reported-by: Imre Deak <imre.deak@gmail.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c |   39 +++++++++++++++++++--------------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a7067e0..fd68ed4 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2915,11 +2915,10 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
 
  search_free:
 	if (map_and_fenceable)
-		free_space =
-			drm_mm_search_free_in_range_color(&dev_priv->mm.gtt_space,
-							  size, alignment, obj->cache_level,
-							  0, dev_priv->mm.gtt_mappable_end,
-							  false);
+		free_space = drm_mm_search_free_in_range_color(&dev_priv->mm.gtt_space,
+							       size, alignment, obj->cache_level,
+							       0, dev_priv->mm.gtt_mappable_end,
+							       false);
 	else
 		free_space = drm_mm_search_free_color(&dev_priv->mm.gtt_space,
 						      size, alignment, obj->cache_level,
@@ -2927,18 +2926,18 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
 
 	if (free_space != NULL) {
 		if (map_and_fenceable)
-			obj->gtt_space =
+			free_space =
 				drm_mm_get_block_range_generic(free_space,
 							       size, alignment, obj->cache_level,
 							       0, dev_priv->mm.gtt_mappable_end,
 							       false);
 		else
-			obj->gtt_space =
+			free_space =
 				drm_mm_get_block_generic(free_space,
 							 size, alignment, obj->cache_level,
 							 false);
 	}
-	if (obj->gtt_space == NULL) {
+	if (free_space == NULL) {
 		ret = i915_gem_evict_something(dev, size, alignment,
 					       obj->cache_level,
 					       map_and_fenceable,
@@ -2951,34 +2950,29 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
 		goto search_free;
 	}
 	if (WARN_ON(!i915_gem_valid_gtt_space(dev,
-					      obj->gtt_space,
+					      free_space,
 					      obj->cache_level))) {
 		i915_gem_object_unpin_pages(obj);
-		drm_mm_put_block(obj->gtt_space);
-		obj->gtt_space = NULL;
+		drm_mm_put_block(free_space);
 		return -EINVAL;
 	}
 
-
 	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;
+		drm_mm_put_block(free_space);
 		return ret;
 	}
 
-	if (!dev_priv->mm.aliasing_ppgtt)
-		i915_gem_gtt_bind_object(obj, obj->cache_level);
-
 	list_move_tail(&obj->gtt_list, &dev_priv->mm.bound_list);
 	list_add_tail(&obj->mm_list, &dev_priv->mm.inactive_list);
 
-	obj->gtt_offset = obj->gtt_space->start;
+	obj->gtt_space = free_space;
+	obj->gtt_offset = free_space->start;
 
 	fenceable =
-		obj->gtt_space->size == fence_size &&
-		(obj->gtt_space->start & (fence_alignment - 1)) == 0;
+		free_space->size == fence_size &&
+		(free_space->start & (fence_alignment - 1)) == 0;
 
 	mappable =
 		obj->gtt_offset + obj->base.size <= dev_priv->mm.gtt_mappable_end;
@@ -3449,11 +3443,16 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
 	}
 
 	if (obj->gtt_space == NULL) {
+		struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
+
 		ret = i915_gem_object_bind_to_gtt(obj, alignment,
 						  map_and_fenceable,
 						  nonblocking);
 		if (ret)
 			return ret;
+
+		if (!dev_priv->mm.aliasing_ppgtt)
+			i915_gem_gtt_bind_object(obj, obj->cache_level);
 	}
 
 	if (!obj->has_global_gtt_mapping && map_and_fenceable)
-- 
1.7.10.4

[PATCH 2/2] drm/i915: Borrow our struct_mutex for the direct reclaim

[Chris Wilson]

If we have hit oom whilst holding our struct_mutex, then currently we
cannot reap our own GPU buffers which likely pin most of memory, making
an outright OOM more likely. So if we are running in direct reclaim and
already hold the mutex, attempt to free buffers knowing that the
original function can not continue until we return.

v2: Add a note explaining that the mutex may be stolen due to
pre-emption, and that is bad.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c |   25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index fd68ed4..d439f30 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4339,6 +4339,19 @@ void i915_gem_release(struct drm_device *dev, struct drm_file *file)
 	spin_unlock(&file_priv->mm.lock);
 }
 
+static bool mutex_is_locked_by(struct mutex *mutex, struct task_struct *task)
+{
+	if (!mutex_is_locked(mutex))
+		return false;
+
+#if defined(CONFIG_SMP) || defined(CONFIG_DEBUG_MUTEXES)
+	return mutex->owner == task;
+#else
+	/* Since UP may be pre-empted, we cannot assume that we own the lock */
+	return false;
+#endif
+}
+
 static int
 i915_gem_inactive_shrink(struct shrinker *shrinker, struct shrink_control *sc)
 {
@@ -4349,10 +4362,15 @@ i915_gem_inactive_shrink(struct shrinker *shrinker, struct shrink_control *sc)
 	struct drm_device *dev = dev_priv->dev;
 	struct drm_i915_gem_object *obj;
 	int nr_to_scan = sc->nr_to_scan;
+	bool unlock = true;
 	int cnt;
 
-	if (!mutex_trylock(&dev->struct_mutex))
-		return 0;
+	if (!mutex_trylock(&dev->struct_mutex)) {
+		if (!mutex_is_locked_by(&dev->struct_mutex, current))
+			return 0;
+
+		unlock = false;
+	}
 
 	if (nr_to_scan) {
 		nr_to_scan -= i915_gem_purge(dev_priv, nr_to_scan);
@@ -4368,6 +4386,7 @@ i915_gem_inactive_shrink(struct shrinker *shrinker, struct shrink_control *sc)
 		if (obj->pin_count == 0 && obj->pages_pin_count == 0)
 			cnt += obj->base.size >> PAGE_SHIFT;
 
-	mutex_unlock(&dev->struct_mutex);
+	if (unlock)
+		mutex_unlock(&dev->struct_mutex);
 	return cnt;
 }
-- 
1.7.10.4

Re: [PATCH 1/2] drm/i915: Defer assignment of obj->gtt_space until after all possible mallocs

[Daniel Vetter]

On Wed, Nov 21, 2012 at 01:04:03PM +0000, Chris Wilson wrote:
> As we may invoke the shrinker whilst trying to allocate memory to hold
> the gtt_space for this object, we need to be careful not to mark the
> drm_mm_node as activated (by assigning it to this object) before we
> have finished our sequence of allocations.
> 
> Reported-by: Imre Deak <imre.deak@gmail.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---

> @@ -3449,11 +3443,16 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
>  	}
>  
>  	if (obj->gtt_space == NULL) {
> +		struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
> +
>  		ret = i915_gem_object_bind_to_gtt(obj, alignment,
>  						  map_and_fenceable,
>  						  nonblocking);
>  		if (ret)
>  			return ret;
> +
> +		if (!dev_priv->mm.aliasing_ppgtt)
> +			i915_gem_gtt_bind_object(obj, obj->cache_level);

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

Re: [PATCH 1/2] drm/i915: Defer assignment of obj->gtt_space until after all possible mallocs

[Chris Wilson]

On Wed, 21 Nov 2012 14:11:34 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Nov 21, 2012 at 01:04:03PM +0000, Chris Wilson wrote:
> > As we may invoke the shrinker whilst trying to allocate memory to hold
> > the gtt_space for this object, we need to be careful not to mark the
> > drm_mm_node as activated (by assigning it to this object) before we
> > have finished our sequence of allocations.
> > 
> > Reported-by: Imre Deak <imre.deak@gmail.com>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> 
> > @@ -3449,11 +3443,16 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
> >  	}
> >  
> >  	if (obj->gtt_space == NULL) {
> > +		struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
> > +
> >  		ret = i915_gem_object_bind_to_gtt(obj, alignment,
> >  						  map_and_fenceable,
> >  						  nonblocking);
> >  		if (ret)
> >  			return ret;
> > +
> > +		if (!dev_priv->mm.aliasing_ppgtt)
> > +			i915_gem_gtt_bind_object(obj, obj->cache_level);
> 
> Spurious hunk?

Not really, I need to reorder the bind_object until after the assignment
of obj->gtt_space and upon reflection it looked better if I did the bind
there next to its compadre then amongst the assignments in the tail of
bind_to_gtt(). Of course, this means that bind_to_gtt is now a grand
misnomer.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: Defer assignment of obj->gtt_space until after all possible mallocs

[Daniel Vetter]

On Wed, Nov 21, 2012 at 2:15 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Wed, 21 Nov 2012 14:11:34 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Wed, Nov 21, 2012 at 01:04:03PM +0000, Chris Wilson wrote:
>> > As we may invoke the shrinker whilst trying to allocate memory to hold
>> > the gtt_space for this object, we need to be careful not to mark the
>> > drm_mm_node as activated (by assigning it to this object) before we
>> > have finished our sequence of allocations.
>> >
>> > Reported-by: Imre Deak <imre.deak@gmail.com>
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > ---
>>
>> > @@ -3449,11 +3443,16 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
>> >     }
>> >
>> >     if (obj->gtt_space == NULL) {
>> > +           struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
>> > +
>> >             ret = i915_gem_object_bind_to_gtt(obj, alignment,
>> >                                               map_and_fenceable,
>> >                                               nonblocking);
>> >             if (ret)
>> >                     return ret;
>> > +
>> > +           if (!dev_priv->mm.aliasing_ppgtt)
>> > +                   i915_gem_gtt_bind_object(obj, obj->cache_level);
>>
>> Spurious hunk?
>
> Not really, I need to reorder the bind_object until after the assignment
> of obj->gtt_space and upon reflection it looked better if I did the bind
> there next to its compadre then amongst the assignments in the tail of
> bind_to_gtt(). Of course, this means that bind_to_gtt is now a grand
> misnomer.

Ok, I now see what's going on. Can't we do the bind together with the
mappable bind, i.e.

if (!has_global_mapping && (map_and_fenceable || !aliasing_ppgtt))
        i915_gem_gtt_bind_object()

Maybe also mention that the gtt binding needs to be moved around, for
dummies like me. Wrt renaming i915_gem_object_bind_to_gtt, I don't
have a good idea to denote the "reserve some space in the abstract
address space and update respective lrus" action it's doing. I guess
we can ponder that again with real ppgtt support, where binding into
the address space and binding into pagetables are different steps
indeed.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/2] drm/i915: Defer assignment of obj->gtt_space until after all possible mallocs

[Chris Wilson]

On Wed, 21 Nov 2012 14:27:06 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> Ok, I now see what's going on. Can't we do the bind together with the
> mappable bind, i.e.
> 
> if (!has_global_mapping && (map_and_fenceable || !aliasing_ppgtt))
>         i915_gem_gtt_bind_object()

You can indeed do it that way, it just took up too many lines... Imo, I
think we want to eventually push the i915_gem_gtt_bind_object() to the
callers. (You'll see why later if we ever get the current 3.7 regression
resolved.)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: Defer assignment of obj->gtt_space until after all possible mallocs

[Daniel Vetter]

On Wed, Nov 21, 2012 at 01:46:29PM +0000, Chris Wilson wrote:
> On Wed, 21 Nov 2012 14:27:06 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> > Ok, I now see what's going on. Can't we do the bind together with the
> > mappable bind, i.e.
> > 
> > if (!has_global_mapping && (map_and_fenceable || !aliasing_ppgtt))
> >         i915_gem_gtt_bind_object()
> 
> You can indeed do it that way, it just took up too many lines... Imo, I
> think we want to eventually push the i915_gem_gtt_bind_object() to the
> callers. (You'll see why later if we ever get the current 3.7 regression
> resolved.)

Ok, count me convinced, both patches merged.

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