Re: [PATCH 29/66] drm: pre allocate node for create_block

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Jun 27, 2013 at 04:30:30PM -0700, Ben Widawsky wrote:
> For an upcoming patch where we introduce the i915 VMA, it's ideal to
> have the drm_mm_node as part of the VMA struct (ie. it's pre-allocated).
> Part of the conversion to VMAs is to kill off obj->gtt_space. Doing this
> will break a bunch of code, but amongst them are 2 callers of
> drm_mm_create_block(), both related to stolen memory.
> 
> As a side note, this patch has is able to leverage all the existing
> drm_mm_put_block because the node is still kzalloc'd. When the
> aforementioned VMA code comes into play, that too has to change.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Same here about cc'ing dri-devel. Furthermore I think it'd be nice to kill
the interfaces from drm_mm.c which allocate the drm_mm_node themselves.
The last user outside of drm/i915 is ttm, killing that one would also
allow us to remove the (racy) preallocation madness.

So if you convert over all of drm/i915 to the preallcoate functions which
pass in the drm_mm_node, I'll volunteer myself to fix up ttm.
-Daniel

> ---
>  drivers/gpu/drm/drm_mm.c               | 16 +++++-----------
>  drivers/gpu/drm/i915/i915_drv.h        |  2 +-
>  drivers/gpu/drm/i915/i915_gem_gtt.c    | 20 ++++++++++++++-----
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 35 +++++++++++++++++++++++-----------
>  include/drm/drm_mm.h                   |  9 ++++-----
>  5 files changed, 49 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
> index 7095328..a2dcfdb 100644
> --- a/drivers/gpu/drm/drm_mm.c
> +++ b/drivers/gpu/drm/drm_mm.c
> @@ -158,12 +158,10 @@ static void drm_mm_insert_helper(struct drm_mm_node *hole_node,
>  	}
>  }
>  
> -struct drm_mm_node *drm_mm_create_block(struct drm_mm *mm,
> -					unsigned long start,
> -					unsigned long size,
> -					enum drm_mm_allocator_flags flags)
> +int drm_mm_create_block(struct drm_mm *mm, struct drm_mm_node *node,
> +			unsigned long start, unsigned long size)
>  {
> -	struct drm_mm_node *hole, *node;
> +	struct drm_mm_node *hole;
>  	unsigned long end = start + size;
>  	unsigned long hole_start;
>  	unsigned long hole_end;
> @@ -172,10 +170,6 @@ struct drm_mm_node *drm_mm_create_block(struct drm_mm *mm,
>  		if (hole_start > start || hole_end < end)
>  			continue;
>  
> -		node = drm_mm_kmalloc(mm, flags & DRM_MM_CREATE_ATOMIC);
> -		if (unlikely(node == NULL))
> -			return NULL;
> -
>  		node->start = start;
>  		node->size = size;
>  		node->mm = mm;
> @@ -195,11 +189,11 @@ struct drm_mm_node *drm_mm_create_block(struct drm_mm *mm,
>  			node->hole_follows = 1;
>  		}
>  
> -		return node;
> +		return 0;
>  	}
>  
>  	WARN(1, "no hole found for block 0x%lx + 0x%lx\n", start, size);
> -	return NULL;
> +	return -ENOSPC;
>  }
>  EXPORT_SYMBOL(drm_mm_create_block);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f6704d3..bc80ce0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1197,7 +1197,7 @@ enum hdmi_force_audio {
>  	HDMI_AUDIO_ON,			/* force turn on HDMI audio */
>  };
>  
> -#define I915_GTT_RESERVED ((struct drm_mm_node *)0x1)
> +#define I915_GTT_RESERVED 0x1
>  
>  struct drm_i915_gem_object_ops {
>  	/* Interface between the GEM object and its backing storage.
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index a45c00d..17e334f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -679,14 +679,24 @@ void i915_gem_setup_global_gtt(struct drm_device *dev,
>  
>  	/* Mark any preallocated objects as occupied */
>  	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
> +		uintptr_t gtt_offset = (uintptr_t)obj->gtt_space;
> +		int ret;
>  		DRM_DEBUG_KMS("reserving preallocated space: %lx + %zx\n",
>  			      obj->gtt_space->start, obj->base.size);
>  
> -		BUG_ON(obj->gtt_space != I915_GTT_RESERVED);
> -		obj->gtt_space = drm_mm_create_block(&i915_gtt_vm->mm,
> -						     obj->gtt_space->start,
> -						     obj->base.size,
> -						     false);
> +		BUG_ON((gtt_offset & I915_GTT_RESERVED) == 0);
> +		gtt_offset = gtt_offset & ~I915_GTT_RESERVED;
> +		obj->gtt_space = kzalloc(sizeof(*obj->gtt_space), GFP_KERNEL);
> +		if (!obj->gtt_space) {
> +			DRM_ERROR("Failed to preserve all objects\n");
> +			break;
> +		}
> +		ret = drm_mm_create_block(&i915_gtt_vm->mm,
> +					  obj->gtt_space,
> +					  gtt_offset,
> +					  obj->base.size);
> +		if (ret)
> +			DRM_DEBUG_KMS("Reservation failed\n");
>  		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 7fba6f5..925f3b1 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -330,6 +330,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_gem_object *obj;
>  	struct drm_mm_node *stolen;
> +	int ret;
>  
>  	if (dev_priv->gtt.stolen_base == 0)
>  		return NULL;
> @@ -344,11 +345,15 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
>  	if (WARN_ON(size == 0))
>  		return NULL;
>  
> -	stolen = drm_mm_create_block(&dev_priv->gtt.stolen,
> -				     stolen_offset, size,
> -				     false);
> -	if (stolen == NULL) {
> +	stolen = kzalloc(sizeof(*stolen), GFP_KERNEL);
> +	if (!stolen)
> +		return NULL;
> +
> +	ret = drm_mm_create_block(&dev_priv->gtt.stolen, stolen, stolen_offset,
> +				  size);
> +	if (ret) {
>  		DRM_DEBUG_KMS("failed to allocate stolen space\n");
> +		kfree(stolen);
>  		return NULL;
>  	}
>  
> @@ -369,18 +374,26 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
>  	 * later.
>  	 */
>  	if (drm_mm_initialized(&i915_gtt_vm->mm)) {
> -		obj->gtt_space = drm_mm_create_block(&i915_gtt_vm->mm,
> -						     gtt_offset, size,
> -						     false);
> -		if (obj->gtt_space == NULL) {
> +		obj->gtt_space = kzalloc(sizeof(*obj->gtt_space), GFP_KERNEL);
> +		if (!obj->gtt_space) {
> +			drm_gem_object_unreference(&obj->base);
> +			return NULL;
> +		}
> +		ret = drm_mm_create_block(&i915_gtt_vm->mm, obj->gtt_space,
> +					  gtt_offset, size);
> +		if (ret) {
>  			DRM_DEBUG_KMS("failed to allocate stolen GTT space\n");
>  			drm_gem_object_unreference(&obj->base);
> +			kfree(obj->gtt_space);
>  			return NULL;
>  		}
> -	} else
> -		obj->gtt_space = I915_GTT_RESERVED;
> +		obj->gtt_space->start = gtt_offset;
> +	} else {
> +		/* NB: Safe because we assert page alignment */
> +		obj->gtt_space = (struct drm_mm_node *)
> +			((uintptr_t)gtt_offset | I915_GTT_RESERVED);
> +	}
>  
> -	obj->gtt_space->start = gtt_offset;
>  	obj->has_global_gtt_mapping = 1;
>  
>  	list_add_tail(&obj->global_list, &dev_priv->mm.bound_list);
> diff --git a/include/drm/drm_mm.h b/include/drm/drm_mm.h
> index 8935710..0cfb06c 100644
> --- a/include/drm/drm_mm.h
> +++ b/include/drm/drm_mm.h
> @@ -161,11 +161,10 @@ static inline unsigned long drm_mm_hole_node_end(struct drm_mm_node *hole_node)
>  /*
>   * Basic range manager support (drm_mm.c)
>   */
> -extern struct drm_mm_node *
> -drm_mm_create_block(struct drm_mm *mm,
> -		    unsigned long start,
> -		    unsigned long size,
> -		    enum drm_mm_allocator_flags flags);
> +extern int drm_mm_create_block(struct drm_mm *mm,
> +			       struct drm_mm_node *node,
> +			       unsigned long start,
> +			       unsigned long size);
>  extern struct drm_mm_node *
>  drm_mm_get_block_generic(struct drm_mm_node *node,
>  			 unsigned long size,
> -- 
> 1.8.3.1
> 
> _______________________________________________
> 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