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