From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jani Nikula Subject: Re: [PATCH 2/3] drm/i915: Preallocate the drm_mm_node prior to manipulating the GTT drm_mm manager Date: Wed, 12 Dec 2012 12:18:35 +0200 Message-ID: <87vcc7a804.fsf@intel.com> References: <1354912628-7776-1-git-send-email-chris@chris-wilson.co.uk> <1354912628-7776-2-git-send-email-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 6D0B6E5BF4 for ; Wed, 12 Dec 2012 02:18:39 -0800 (PST) In-Reply-To: <1354912628-7776-2-git-send-email-chris@chris-wilson.co.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Chris Wilson , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Fri, 07 Dec 2012, Chris Wilson 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 > Signed-off-by: Chris Wilson > --- > 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 > + > 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