Re: [PATCH 5/6] drm/i915: Embed drm_mm_node in i915 gem obj

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jul 03, 2013 at 02:45:25PM -0700, Ben Widawsky wrote:
> Embedding the node in the obj is more natural in the transition to VMAs
> which will also have embedded nodes. This change also helps transition
> away from put_block to remove node.
> 
> Though it's quite an uncommon occurrence, it's somewhat convenient to not
> fail at bind time because we cannot allocate the node. Though in
> practice there are other allocations (like the request structure) which
> would probably make this point not terribly useful.
> 
> Quoting Daniel:
> Note that the only difference between put_block and remove_node is
> that the former fills up the preallocation cache. Which we don't need
> anyway and hence is just wasted space.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

This one is blocked for now on getting the create_block drm_mm changes in.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h        | 10 +++++-----
>  drivers/gpu/drm/i915/i915_gem.c        | 31 +++++++++++--------------------
>  drivers/gpu/drm/i915/i915_gem_evict.c  |  6 +++---
>  drivers/gpu/drm/i915/i915_gem_gtt.c    | 13 +++----------
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 10 ++--------
>  5 files changed, 24 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b6864ffb..67bdcf1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1227,7 +1227,7 @@ struct drm_i915_gem_object {
>  	const struct drm_i915_gem_object_ops *ops;
>  
>  	/** Current space allocated to this object in the GTT, if any. */
> -	struct drm_mm_node *ggtt_space;
> +	struct drm_mm_node ggtt_space;
>  	/** Stolen memory for this object, instead of being backed by shmem. */
>  	struct drm_mm_node *stolen;
>  	struct list_head global_list;
> @@ -1357,14 +1357,14 @@ struct drm_i915_gem_object {
>  static inline unsigned long
>  i915_gem_obj_ggtt_offset(struct drm_i915_gem_object *o)
>  {
> -	return o->ggtt_space->start;
> +	return o->ggtt_space.start;
>  }
>  
>  /* Whether or not this object is currently mapped by the translation tables */
>  static inline bool
>  i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *o)
>  {
> -	return o->ggtt_space != NULL;
> +	return drm_mm_node_allocated(&o->ggtt_space);
>  }
>  
>  /* The size used in the translation tables may be larger than the actual size of
> @@ -1374,14 +1374,14 @@ i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *o)
>  static inline unsigned long
>  i915_gem_obj_ggtt_size(struct drm_i915_gem_object *o)
>  {
> -	return o->ggtt_space->size;
> +	return o->ggtt_space.size;
>  }
>  
>  static inline void
>  i915_gem_obj_ggtt_set_color(struct drm_i915_gem_object *o,
>  			    enum i915_cache_level color)
>  {
> -	o->ggtt_space->color = color;
> +	o->ggtt_space.color = color;
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index e0568e3..6cb9210 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2621,8 +2621,7 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj)
>  	/* Avoid an unnecessary call to unbind on rebind. */
>  	obj->map_and_fenceable = true;
>  
> -	drm_mm_put_block(obj->ggtt_space);
> -	obj->ggtt_space = NULL;
> +	drm_mm_remove_node(&obj->ggtt_space);
>  
>  	return 0;
>  }
> @@ -3000,7 +2999,7 @@ static bool i915_gem_valid_gtt_space(struct drm_device *dev,
>  	if (HAS_LLC(dev))
>  		return true;
>  
> -	if (gtt_space == NULL)
> +	if (!drm_mm_node_allocated(gtt_space))
>  		return true;
>  
>  	if (list_empty(&gtt_space->node_list))
> @@ -3068,7 +3067,6 @@ 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 *node;
>  	u32 size, fence_size, fence_alignment, unfenced_alignment;
>  	bool mappable, fenceable;
>  	size_t gtt_max = map_and_fenceable ?
> @@ -3113,14 +3111,9 @@ 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:
> -	ret = drm_mm_insert_node_in_range_generic(&dev_priv->mm.gtt_space, node,
> +	ret = drm_mm_insert_node_in_range_generic(&dev_priv->mm.gtt_space,
> +						  &obj->ggtt_space,
>  						  size, alignment,
>  						  obj->cache_level, 0, gtt_max);
>  	if (ret) {
> @@ -3132,30 +3125,28 @@ search_free:
>  			goto search_free;
>  
>  		i915_gem_object_unpin_pages(obj);
> -		kfree(node);
>  		return ret;
>  	}
> -	if (WARN_ON(!i915_gem_valid_gtt_space(dev, node, obj->cache_level))) {
> +	if (WARN_ON(!i915_gem_valid_gtt_space(dev, &obj->ggtt_space,
> +					      obj->cache_level))) {
>  		i915_gem_object_unpin_pages(obj);
> -		drm_mm_put_block(node);
> +		drm_mm_remove_node(&obj->ggtt_space);
>  		return -EINVAL;
>  	}
>  
>  	ret = i915_gem_gtt_prepare_object(obj);
>  	if (ret) {
>  		i915_gem_object_unpin_pages(obj);
> -		drm_mm_put_block(node);
> +		drm_mm_remove_node(&obj->ggtt_space);
>  		return ret;
>  	}
>  
>  	list_move_tail(&obj->global_list, &dev_priv->mm.bound_list);
>  	list_add_tail(&obj->mm_list, &dev_priv->mm.inactive_list);
>  
> -	obj->ggtt_space = node;
> -
>  	fenceable =
> -		node->size == fence_size &&
> -		(node->start & (fence_alignment - 1)) == 0;
> +		i915_gem_obj_ggtt_size(obj) == fence_size &&
> +		(i915_gem_obj_ggtt_offset(obj) & (fence_alignment - 1)) == 0;
>  
>  	mappable = i915_gem_obj_ggtt_offset(obj) + obj->base.size <=
>  		dev_priv->gtt.mappable_end;
> @@ -3319,7 +3310,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
>  		return -EBUSY;
>  	}
>  
> -	if (!i915_gem_valid_gtt_space(dev, obj->ggtt_space, cache_level)) {
> +	if (!i915_gem_valid_gtt_space(dev, &obj->ggtt_space, cache_level)) {
>  		ret = i915_gem_object_unbind(obj);
>  		if (ret)
>  			return ret;
> diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
> index 5bbdea4..a1e7ec8 100644
> --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> @@ -38,7 +38,7 @@ mark_free(struct drm_i915_gem_object *obj, struct list_head *unwind)
>  		return false;
>  
>  	list_add(&obj->exec_list, unwind);
> -	return drm_mm_scan_add_block(obj->ggtt_space);
> +	return drm_mm_scan_add_block(&obj->ggtt_space);
>  }
>  
>  int
> @@ -107,7 +107,7 @@ none:
>  				       struct drm_i915_gem_object,
>  				       exec_list);
>  
> -		ret = drm_mm_scan_remove_block(obj->ggtt_space);
> +		ret = drm_mm_scan_remove_block(&obj->ggtt_space);
>  		BUG_ON(ret);
>  
>  		list_del_init(&obj->exec_list);
> @@ -127,7 +127,7 @@ found:
>  		obj = list_first_entry(&unwind_list,
>  				       struct drm_i915_gem_object,
>  				       exec_list);
> -		if (drm_mm_scan_remove_block(obj->ggtt_space)) {
> +		if (drm_mm_scan_remove_block(&obj->ggtt_space)) {
>  			list_move(&obj->exec_list, &eviction_list);
>  			drm_gem_object_reference(&obj->base);
>  			continue;
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index fbe2e72..4df6159 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -633,20 +633,13 @@ void i915_gem_setup_global_gtt(struct drm_device *dev,
>  		DRM_DEBUG_KMS("reserving preallocated space: %lx + %zx\n",
>  			      i915_gem_obj_ggtt_offset(obj), obj->base.size);
>  
> -		BUG_ON(obj->ggtt_space->start != I915_GTT_RESERVED);
> -		obj->ggtt_space = kzalloc(sizeof(*obj->ggtt_space), GFP_KERNEL);
> -		if (!obj->ggtt_space) {
> -			DRM_ERROR("Failed to preserve all objects\n");
> -			break;
> -		}
> +		BUG_ON(i915_gem_obj_ggtt_offset(obj) != I915_GTT_RESERVED);
>  		ret = drm_mm_create_block(&dev_priv->mm.gtt_space,
> -					  obj->ggtt_space,
> +					  &obj->ggtt_space,
>  					  i915_gem_obj_ggtt_offset(obj),
>  					  obj->base.size);
> -		if (ret) {
> +		if (ret)
>  			DRM_DEBUG_KMS("Reservation failed\n");
> -			kfree(obj->ggtt_space);
> -		}
>  		obj->has_global_gtt_mapping = 1;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index c9d7016..dc68b30 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -374,21 +374,15 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
>  	 * later.
>  	 */
>  	if (drm_mm_initialized(&dev_priv->mm.gtt_space)) {
> -		obj->ggtt_space = kzalloc(sizeof(*obj->ggtt_space), GFP_KERNEL);
> -		if (!obj->ggtt_space) {
> -			DRM_DEBUG_KMS("-ENOMEM stolen GTT space\n");
> -			goto unref_out;
> -		}
> -
>  		ret = drm_mm_create_block(&dev_priv->mm.gtt_space,
> -					  obj->ggtt_space,
> +					  &obj->ggtt_space,
>  					  gtt_offset, size);
>  		if (ret) {
>  			DRM_DEBUG_KMS("failed to allocate stolen GTT space\n");
>  			goto unref_out;
>  		}
>  	} else
> -		obj->ggtt_space->start = I915_GTT_RESERVED;
> +		obj->ggtt_space.start = I915_GTT_RESERVED;
>  
>  	obj->has_global_gtt_mapping = 1;
>  
> -- 
> 1.8.3.2
> 
> _______________________________________________
> 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