| Message ID | 1353503044-20343-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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)
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(-)