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 at 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(>t_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 at 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
