| Message ID | 1354912628-7776-2-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 07 Dec 2012, Chris Wilson <chris@chris-wilson.co.uk> wrote: > As we may reap neighbouring objects in order to free up pages for > allocations, we need to be careful not to allocate in the middle of the > drm_mm manager. To accomplish this, we can simply allocate the > drm_mm_node up front and then use the combined search & insert > drm_mm routines, reducing our code footprint in the process. > > Fixes (partially) i-g-t/gem_tiled_swapping > > Reported-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_gem.c | 64 +++++++++++++++++---------------------- > 1 file changed, 27 insertions(+), 37 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index c1f6919..d17f52d 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2890,7 +2890,7 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj, > { > struct drm_device *dev = obj->base.dev; > drm_i915_private_t *dev_priv = dev->dev_private; > - struct drm_mm_node *free_space; > + struct drm_mm_node *node; > u32 size, fence_size, fence_alignment, unfenced_alignment; > bool mappable, fenceable; > int ret; > @@ -2936,66 +2936,56 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj, > > i915_gem_object_pin_pages(obj); > > + node = kzalloc(sizeof(*node), GFP_KERNEL); > + if (node == NULL) { > + i915_gem_object_unpin_pages(obj); > + return -ENOMEM; > + } Any reason not to do the kzalloc before i915_gem_object_pin_pages, with a slight simplification of the error path there? Otherwise, with the disclaimer that I'm a newbie in drm mm, Reviewed-by: Jani Nikula <jani.nikula@intel.com> > + > 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); > + ret = drm_mm_insert_node_in_range_generic(&dev_priv->mm.gtt_space, node, > + 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, > - false); > - > - if (free_space != NULL) { > - if (map_and_fenceable) > - free_space = > - drm_mm_get_block_range_generic(free_space, > - size, alignment, obj->cache_level, > - 0, dev_priv->mm.gtt_mappable_end, > - false); > - else > - free_space = > - drm_mm_get_block_generic(free_space, > - size, alignment, obj->cache_level, > - false); > - } > - if (free_space == NULL) { > + ret = drm_mm_insert_node_generic(&dev_priv->mm.gtt_space, node, > + size, alignment, obj->cache_level, > + false); > + if (ret) { > ret = i915_gem_evict_something(dev, size, alignment, > obj->cache_level, > map_and_fenceable, > nonblocking); > - if (ret) { > - i915_gem_object_unpin_pages(obj); > - return ret; > - } > + if (ret == 0) > + goto search_free; > > - goto search_free; > + i915_gem_object_unpin_pages(obj); > + kfree(node); > + return ret; > } > - if (WARN_ON(!i915_gem_valid_gtt_space(dev, > - free_space, > - obj->cache_level))) { > + if (WARN_ON(!i915_gem_valid_gtt_space(dev, node, obj->cache_level))) { > i915_gem_object_unpin_pages(obj); > - drm_mm_put_block(free_space); > + drm_mm_put_block(node); > return -EINVAL; > } > > ret = i915_gem_gtt_prepare_object(obj); > if (ret) { > i915_gem_object_unpin_pages(obj); > - drm_mm_put_block(free_space); > + drm_mm_put_block(node); > return ret; > } > > list_move_tail(&obj->gtt_list, &dev_priv->mm.bound_list); > list_add_tail(&obj->mm_list, &dev_priv->mm.inactive_list); > > - obj->gtt_space = free_space; > - obj->gtt_offset = free_space->start; > + obj->gtt_space = node; > + obj->gtt_offset = node->start; > > fenceable = > - free_space->size == fence_size && > - (free_space->start & (fence_alignment - 1)) == 0; > + node->size == fence_size && > + (node->start & (fence_alignment - 1)) == 0; > > mappable = > obj->gtt_offset + obj->base.size <= dev_priv->mm.gtt_mappable_end; > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 12 Dec 2012 12:18:35 +0200, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Fri, 07 Dec 2012, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > As we may reap neighbouring objects in order to free up pages for > > allocations, we need to be careful not to allocate in the middle of the > > drm_mm manager. To accomplish this, we can simply allocate the > > drm_mm_node up front and then use the combined search & insert > > drm_mm routines, reducing our code footprint in the process. > > > > Fixes (partially) i-g-t/gem_tiled_swapping > > > > Reported-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/i915_gem.c | 64 +++++++++++++++++---------------------- > > 1 file changed, 27 insertions(+), 37 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index c1f6919..d17f52d 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -2890,7 +2890,7 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj, > > { > > struct drm_device *dev = obj->base.dev; > > drm_i915_private_t *dev_priv = dev->dev_private; > > - struct drm_mm_node *free_space; > > + struct drm_mm_node *node; > > u32 size, fence_size, fence_alignment, unfenced_alignment; > > bool mappable, fenceable; > > int ret; > > @@ -2936,66 +2936,56 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj, > > > > i915_gem_object_pin_pages(obj); > > > > + node = kzalloc(sizeof(*node), GFP_KERNEL); > > + if (node == NULL) { > > + i915_gem_object_unpin_pages(obj); > > + return -ENOMEM; > > + } > > Any reason not to do the kzalloc before i915_gem_object_pin_pages, with > a slight simplification of the error path there? No reason at all. In my defense, I was trying to make the code as similar as possible... -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index c1f6919..d17f52d 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2890,7 +2890,7 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj, { struct drm_device *dev = obj->base.dev; drm_i915_private_t *dev_priv = dev->dev_private; - struct drm_mm_node *free_space; + struct drm_mm_node *node; u32 size, fence_size, fence_alignment, unfenced_alignment; bool mappable, fenceable; int ret; @@ -2936,66 +2936,56 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj, i915_gem_object_pin_pages(obj); + node = kzalloc(sizeof(*node), GFP_KERNEL); + if (node == NULL) { + i915_gem_object_unpin_pages(obj); + return -ENOMEM; + } + 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); + ret = drm_mm_insert_node_in_range_generic(&dev_priv->mm.gtt_space, node, + 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, - false); - - if (free_space != NULL) { - if (map_and_fenceable) - free_space = - drm_mm_get_block_range_generic(free_space, - size, alignment, obj->cache_level, - 0, dev_priv->mm.gtt_mappable_end, - false); - else - free_space = - drm_mm_get_block_generic(free_space, - size, alignment, obj->cache_level, - false); - } - if (free_space == NULL) { + ret = drm_mm_insert_node_generic(&dev_priv->mm.gtt_space, node, + size, alignment, obj->cache_level, + false); + if (ret) { ret = i915_gem_evict_something(dev, size, alignment, obj->cache_level, map_and_fenceable, nonblocking); - if (ret) { - i915_gem_object_unpin_pages(obj); - return ret; - } + if (ret == 0) + goto search_free; - goto search_free; + i915_gem_object_unpin_pages(obj); + kfree(node); + return ret; } - if (WARN_ON(!i915_gem_valid_gtt_space(dev, - free_space, - obj->cache_level))) { + if (WARN_ON(!i915_gem_valid_gtt_space(dev, node, obj->cache_level))) { i915_gem_object_unpin_pages(obj); - drm_mm_put_block(free_space); + drm_mm_put_block(node); return -EINVAL; } ret = i915_gem_gtt_prepare_object(obj); if (ret) { i915_gem_object_unpin_pages(obj); - drm_mm_put_block(free_space); + drm_mm_put_block(node); return ret; } list_move_tail(&obj->gtt_list, &dev_priv->mm.bound_list); list_add_tail(&obj->mm_list, &dev_priv->mm.inactive_list); - obj->gtt_space = free_space; - obj->gtt_offset = free_space->start; + obj->gtt_space = node; + obj->gtt_offset = node->start; fenceable = - free_space->size == fence_size && - (free_space->start & (fence_alignment - 1)) == 0; + node->size == fence_size && + (node->start & (fence_alignment - 1)) == 0; mappable = obj->gtt_offset + obj->base.size <= dev_priv->mm.gtt_mappable_end;
As we may reap neighbouring objects in order to free up pages for allocations, we need to be careful not to allocate in the middle of the drm_mm manager. To accomplish this, we can simply allocate the drm_mm_node up front and then use the combined search & insert drm_mm routines, reducing our code footprint in the process. Fixes (partially) i-g-t/gem_tiled_swapping Reported-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_gem.c | 64 +++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 37 deletions(-)