[PATCH v4 2/4] drm/gem: convert to new unified vma manager

David Herrmann dh.herrmann at gmail.com
Wed Jul 24 09:27:33 PDT 2013


Hi

On Wed, Jul 24, 2013 at 5:52 PM, Daniel Vetter <daniel at ffwll.ch> wrote:
> On Tue, Jul 23, 2013 at 02:47:14PM +0200, David Herrmann wrote:
>> Use the new vma manager instead of the old hashtable. Also convert all
>> drivers to use the new convenience helpers. This drops all the
>> (map_list.hash.key << PAGE_SHIFT) non-sense.
>>
>> Locking and access-management is exactly the same as before with an
>> additional lock inside of the vma-manager, which strictly wouldn't be
>> needed for gem.
>>
>> v2:
>>  - rebase on drm-next
>>  - init nodes via drm_vma_node_reset() in drm_gem.c
>> v3:
>>  - fix tegra
>> v4:
>>  - remove duplicate if (drm_vma_node_has_offset()) checks
>>  - inline now trivial drm_vma_node_offset_addr() calls
>>
>> Cc: Inki Dae <inki.dae at samsung.com>
>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>> Cc: Rob Clark <robdclark at gmail.com>
>> Cc: Dave Airlie <airlied at redhat.com>
>> Cc: Thierry Reding <thierry.reding at gmail.com>
>> Signed-off-by: David Herrmann <dh.herrmann at gmail.com>
>> Acked-by: Patrik Jakobsson <patrik.r.jakobsson at gmail.com>
>> ---
>>  drivers/gpu/drm/drm_gem.c                  | 89 +++++-------------------------
>>  drivers/gpu/drm/drm_gem_cma_helper.c       | 16 ++----
>>  drivers/gpu/drm/exynos/exynos_drm_gem.c    | 14 ++---
>>  drivers/gpu/drm/gma500/gem.c               | 15 ++---
>>  drivers/gpu/drm/i915/i915_gem.c            | 10 ++--
>>  drivers/gpu/drm/omapdrm/omap_gem.c         | 28 +++++-----
>>  drivers/gpu/drm/omapdrm/omap_gem_helpers.c | 49 +---------------
>>  drivers/gpu/drm/udl/udl_gem.c              | 13 ++---
>>  drivers/gpu/host1x/drm/gem.c               |  5 +-
>>  include/drm/drmP.h                         |  7 +--
>>  include/uapi/drm/drm.h                     |  2 +-
>>  11 files changed, 62 insertions(+), 186 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>> index 1ad9e7e..8d3cdf3 100644
>> --- a/drivers/gpu/drm/drm_gem.c
>> +++ b/drivers/gpu/drm/drm_gem.c
>> @@ -37,6 +37,7 @@
>>  #include <linux/shmem_fs.h>
>>  #include <linux/dma-buf.h>
>>  #include <drm/drmP.h>
>> +#include <drm/drm_vma_manager.h>
>>
>>  /** @file drm_gem.c
>>   *
>> @@ -102,14 +103,9 @@ drm_gem_init(struct drm_device *dev)
>>       }
>>
>>       dev->mm_private = mm;
>> -
>> -     if (drm_ht_create(&mm->offset_hash, 12)) {
>> -             kfree(mm);
>> -             return -ENOMEM;
>> -     }
>> -
>> -     drm_mm_init(&mm->offset_manager, DRM_FILE_PAGE_OFFSET_START,
>> -                 DRM_FILE_PAGE_OFFSET_SIZE);
>> +     drm_vma_offset_manager_init(&mm->vma_manager,
>> +                                 DRM_FILE_PAGE_OFFSET_START,
>> +                                 DRM_FILE_PAGE_OFFSET_SIZE);
>>
>>       return 0;
>>  }
>> @@ -119,8 +115,7 @@ drm_gem_destroy(struct drm_device *dev)
>>  {
>>       struct drm_gem_mm *mm = dev->mm_private;
>>
>> -     drm_mm_takedown(&mm->offset_manager);
>> -     drm_ht_remove(&mm->offset_hash);
>> +     drm_vma_offset_manager_destroy(&mm->vma_manager);
>>       kfree(mm);
>>       dev->mm_private = NULL;
>>  }
>> @@ -160,6 +155,7 @@ void drm_gem_private_object_init(struct drm_device *dev,
>>
>>       kref_init(&obj->refcount);
>>       atomic_set(&obj->handle_count, 0);
>> +     drm_vma_node_reset(&obj->vma_node);
>
> We already presume (but it's not really documented) that a gem object is
> cleared already (or allocated with kzalloc) so feels a bit like overhead.

I am fine with dropping it, if that's what gem already requires from drivers.

>>       obj->size = size;
>>  }
>>  EXPORT_SYMBOL(drm_gem_private_object_init);
>> @@ -302,12 +298,8 @@ drm_gem_free_mmap_offset(struct drm_gem_object *obj)
>>  {
>>       struct drm_device *dev = obj->dev;
>>       struct drm_gem_mm *mm = dev->mm_private;
>> -     struct drm_map_list *list = &obj->map_list;
>>
>> -     drm_ht_remove_item(&mm->offset_hash, &list->hash);
>> -     drm_mm_put_block(list->file_offset_node);
>> -     kfree(list->map);
>> -     list->map = NULL;
>> +     drm_vma_offset_remove(&mm->vma_manager, &obj->vma_node);
>>  }
>>  EXPORT_SYMBOL(drm_gem_free_mmap_offset);
>>
>> @@ -327,54 +319,9 @@ drm_gem_create_mmap_offset(struct drm_gem_object *obj)
>>  {
>>       struct drm_device *dev = obj->dev;
>>       struct drm_gem_mm *mm = dev->mm_private;
>> -     struct drm_map_list *list;
>> -     struct drm_local_map *map;
>> -     int ret;
>> -
>> -     /* Set the object up for mmap'ing */
>> -     list = &obj->map_list;
>> -     list->map = kzalloc(sizeof(struct drm_map_list), GFP_KERNEL);
>> -     if (!list->map)
>> -             return -ENOMEM;
>> -
>> -     map = list->map;
>> -     map->type = _DRM_GEM;
>> -     map->size = obj->size;
>> -     map->handle = obj;
>> -
>> -     /* Get a DRM GEM mmap offset allocated... */
>> -     list->file_offset_node = drm_mm_search_free(&mm->offset_manager,
>> -                     obj->size / PAGE_SIZE, 0, false);
>> -
>> -     if (!list->file_offset_node) {
>> -             DRM_ERROR("failed to allocate offset for bo %d\n", obj->name);
>> -             ret = -ENOSPC;
>> -             goto out_free_list;
>> -     }
>>
>> -     list->file_offset_node = drm_mm_get_block(list->file_offset_node,
>> -                     obj->size / PAGE_SIZE, 0);
>> -     if (!list->file_offset_node) {
>> -             ret = -ENOMEM;
>> -             goto out_free_list;
>> -     }
>> -
>> -     list->hash.key = list->file_offset_node->start;
>> -     ret = drm_ht_insert_item(&mm->offset_hash, &list->hash);
>> -     if (ret) {
>> -             DRM_ERROR("failed to add to map hash\n");
>> -             goto out_free_mm;
>> -     }
>> -
>> -     return 0;
>> -
>> -out_free_mm:
>> -     drm_mm_put_block(list->file_offset_node);
>> -out_free_list:
>> -     kfree(list->map);
>> -     list->map = NULL;
>> -
>> -     return ret;
>> +     return drm_vma_offset_add(&mm->vma_manager, &obj->vma_node,
>> +                               obj->size / PAGE_SIZE);
>>  }
>>  EXPORT_SYMBOL(drm_gem_create_mmap_offset);
>>
>> @@ -703,8 +650,8 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>>       struct drm_file *priv = filp->private_data;
>>       struct drm_device *dev = priv->minor->dev;
>>       struct drm_gem_mm *mm = dev->mm_private;
>> -     struct drm_local_map *map = NULL;
>> -     struct drm_hash_item *hash;
>> +     struct drm_gem_object *obj;
>> +     struct drm_vma_offset_node *node;
>>       int ret = 0;
>>
>>       if (drm_device_is_unplugged(dev))
>> @@ -712,21 +659,15 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>>
>>       mutex_lock(&dev->struct_mutex);
>>
>> -     if (drm_ht_find_item(&mm->offset_hash, vma->vm_pgoff, &hash)) {
>> +     node = drm_vma_offset_exact_lookup(&mm->vma_manager, vma->vm_pgoff);
>> +     if (!node) {
>>               mutex_unlock(&dev->struct_mutex);
>>               return drm_mmap(filp, vma);
>>       }
>>
>> -     map = drm_hash_entry(hash, struct drm_map_list, hash)->map;
>> -     if (!map ||
>> -         ((map->flags & _DRM_RESTRICTED) && !capable(CAP_SYS_ADMIN))) {
>> -             ret =  -EPERM;
>> -             goto out_unlock;
>> -     }
>> -
>> -     ret = drm_gem_mmap_obj(map->handle, map->size, vma);
>> +     obj = container_of(node, struct drm_gem_object, vma_node);
>> +     ret = drm_gem_mmap_obj(obj, node->vm_pages, vma);
>
> Iirc the old code only resulted in a ht hit if the start of the mmap
> regioni exactly matched the offset. The new code (I think at least, maybe
> I've misread it a bit) will return an object even if the start isn't
> aligned. drm_gem_mmap_obj checks already that the object is big enough but
> we don't check the start any more. I think this should be readded.
>
> It's a bit funny that gem wants an exact match but is ok with just mapping
> the beginning of the object, but I guess that abi is now enshrined ;-)

drm_vma_offset_exact_lookup() was meant for exactly that but it fails
to actually verify it. I can add a "drm_vma_node_start() == start"
check and return NULL if it doesn't match. The name is currently a bit
misleading.

I will fix that.

>>
>> -out_unlock:
>>       mutex_unlock(&dev->struct_mutex);
>>
>>       return ret;
>> diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
>> index ece72a8..bf09a6dc 100644
>> --- a/drivers/gpu/drm/drm_gem_cma_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
>> @@ -27,11 +27,7 @@
>>  #include <drm/drmP.h>
>>  #include <drm/drm.h>
>>  #include <drm/drm_gem_cma_helper.h>
>> -
>> -static unsigned int get_gem_mmap_offset(struct drm_gem_object *obj)
>> -{
>> -     return (unsigned int)obj->map_list.hash.key << PAGE_SHIFT;
>> -}
>> +#include <drm/drm_vma_manager.h>
>>
>>  /*
>>   * __drm_gem_cma_create - Create a GEM CMA object without allocating memory
>> @@ -172,8 +168,7 @@ void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
>>  {
>>       struct drm_gem_cma_object *cma_obj;
>>
>> -     if (gem_obj->map_list.map)
>> -             drm_gem_free_mmap_offset(gem_obj);
>> +     drm_gem_free_mmap_offset(gem_obj);
>>
>>       cma_obj = to_drm_gem_cma_obj(gem_obj);
>>
>> @@ -237,7 +232,7 @@ int drm_gem_cma_dumb_map_offset(struct drm_file *file_priv,
>>               return -EINVAL;
>>       }
>>
>> -     *offset = get_gem_mmap_offset(gem_obj);
>> +     *offset = (unsigned int)drm_vma_node_offset_addr(&gem_obj->vma_node);
>
> *offset is an u64, same as drm_vma_node_offset_addr. Why do we need a cast
> here?

Copied from the helper above. I can fix it.

>>
>>       drm_gem_object_unreference(gem_obj);
>>
>> @@ -301,12 +296,11 @@ void drm_gem_cma_describe(struct drm_gem_cma_object *cma_obj, struct seq_file *m
>>  {
>>       struct drm_gem_object *obj = &cma_obj->base;
>>       struct drm_device *dev = obj->dev;
>> -     uint64_t off = 0;
>> +     uint64_t off;
>>
>>       WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>>
>> -     if (obj->map_list.map)
>> -             off = (uint64_t)obj->map_list.hash.key;
>> +     off = drm_vma_node_start(&obj->vma_node);
>>
>>       seq_printf(m, "%2d (%2d) %08llx %08Zx %p %d",
>>                       obj->name, obj->refcount.refcount.counter,
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
>> index 24c22a8..be32db1 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
>> @@ -10,6 +10,7 @@
>>   */
>>
>>  #include <drm/drmP.h>
>> +#include <drm/drm_vma_manager.h>
>>
>>  #include <linux/shmem_fs.h>
>>  #include <drm/exynos_drm.h>
>> @@ -152,8 +153,7 @@ out:
>>       exynos_drm_fini_buf(obj->dev, buf);
>>       exynos_gem_obj->buffer = NULL;
>>
>> -     if (obj->map_list.map)
>> -             drm_gem_free_mmap_offset(obj);
>> +     drm_gem_free_mmap_offset(obj);
>>
>>       /* release file pointer to gem object. */
>>       drm_gem_object_release(obj);
>> @@ -703,13 +703,11 @@ int exynos_drm_gem_dumb_map_offset(struct drm_file *file_priv,
>>               goto unlock;
>>       }
>>
>> -     if (!obj->map_list.map) {
>> -             ret = drm_gem_create_mmap_offset(obj);
>> -             if (ret)
>> -                     goto out;
>> -     }
>> +     ret = drm_gem_create_mmap_offset(obj);
>> +     if (ret)
>> +             goto out;
>>
>> -     *offset = (u64)obj->map_list.hash.key << PAGE_SHIFT;
>> +     *offset = drm_vma_node_offset_addr(&obj->vma_node);
>>       DRM_DEBUG_KMS("offset = 0x%lx\n", (unsigned long)*offset);
>>
>>  out:
>> diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
>> index fe1d332..2f77bea 100644
>> --- a/drivers/gpu/drm/gma500/gem.c
>> +++ b/drivers/gpu/drm/gma500/gem.c
>> @@ -26,6 +26,7 @@
>>  #include <drm/drmP.h>
>>  #include <drm/drm.h>
>>  #include <drm/gma_drm.h>
>> +#include <drm/drm_vma_manager.h>
>>  #include "psb_drv.h"
>>
>>  int psb_gem_init_object(struct drm_gem_object *obj)
>> @@ -38,8 +39,7 @@ void psb_gem_free_object(struct drm_gem_object *obj)
>>       struct gtt_range *gtt = container_of(obj, struct gtt_range, gem);
>>
>>       /* Remove the list map if one is present */
>> -     if (obj->map_list.map)
>> -             drm_gem_free_mmap_offset(obj);
>> +     drm_gem_free_mmap_offset(obj);
>>       drm_gem_object_release(obj);
>>
>>       /* This must occur last as it frees up the memory of the GEM object */
>> @@ -81,13 +81,10 @@ int psb_gem_dumb_map_gtt(struct drm_file *file, struct drm_device *dev,
>>       /* What validation is needed here ? */
>>
>>       /* Make it mmapable */
>> -     if (!obj->map_list.map) {
>> -             ret = drm_gem_create_mmap_offset(obj);
>> -             if (ret)
>> -                     goto out;
>> -     }
>> -     /* GEM should really work out the hash offsets for us */
>> -     *offset = (u64)obj->map_list.hash.key << PAGE_SHIFT;
>> +     ret = drm_gem_create_mmap_offset(obj);
>> +     if (ret)
>> +             goto out;
>> +     *offset = drm_vma_node_offset_addr(&obj->vma_node);
>>  out:
>>       drm_gem_object_unreference(obj);
>>  unlock:
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index 46bf7e3..53f81b3 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -26,6 +26,7 @@
>>   */
>>
>>  #include <drm/drmP.h>
>> +#include <drm/drm_vma_manager.h>
>>  #include <drm/i915_drm.h>
>>  #include "i915_drv.h"
>>  #include "i915_trace.h"
>> @@ -1428,7 +1429,7 @@ i915_gem_release_mmap(struct drm_i915_gem_object *obj)
>>
>>       if (obj->base.dev->dev_mapping)
>>               unmap_mapping_range(obj->base.dev->dev_mapping,
>> -                                 (loff_t)obj->base.map_list.hash.key<<PAGE_SHIFT,
>> +                                 (loff_t)drm_vma_node_offset_addr(&obj->base.vma_node),
>>                                   obj->base.size, 1);
>>
>>       obj->fault_mappable = false;
>> @@ -1486,7 +1487,7 @@ static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj)
>>       struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
>>       int ret;
>>
>> -     if (obj->base.map_list.map)
>> +     if (drm_vma_node_has_offset(&obj->base.vma_node))
>>               return 0;
>>
>>       dev_priv->mm.shrinker_no_lock_stealing = true;
>> @@ -1517,9 +1518,6 @@ out:
>>
>>  static void i915_gem_object_free_mmap_offset(struct drm_i915_gem_object *obj)
>>  {
>> -     if (!obj->base.map_list.map)
>> -             return;
>> -
>>       drm_gem_free_mmap_offset(&obj->base);
>>  }
>>
>> @@ -1558,7 +1556,7 @@ i915_gem_mmap_gtt(struct drm_file *file,
>>       if (ret)
>>               goto out;
>>
>> -     *offset = (u64)obj->base.map_list.hash.key << PAGE_SHIFT;
>> +     *offset = drm_vma_node_offset_addr(&obj->base.vma_node);
>>
>>  out:
>>       drm_gem_object_unreference(&obj->base);
>> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
>> index cbcd71e..f90531fc00 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
>> @@ -20,6 +20,7 @@
>>
>>  #include <linux/spinlock.h>
>>  #include <linux/shmem_fs.h>
>> +#include <drm/drm_vma_manager.h>
>>
>>  #include "omap_drv.h"
>>  #include "omap_dmm_tiler.h"
>> @@ -308,21 +309,20 @@ uint32_t omap_gem_flags(struct drm_gem_object *obj)
>>  static uint64_t mmap_offset(struct drm_gem_object *obj)
>>  {
>>       struct drm_device *dev = obj->dev;
>> +     int ret;
>> +     size_t size;
>>
>>       WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>>
>> -     if (!obj->map_list.map) {
>> -             /* Make it mmapable */
>> -             size_t size = omap_gem_mmap_size(obj);
>> -             int ret = _drm_gem_create_mmap_offset_size(obj, size);
>> -
>> -             if (ret) {
>> -                     dev_err(dev->dev, "could not allocate mmap offset\n");
>> -                     return 0;
>> -             }
>> +     /* Make it mmapable */
>> +     size = omap_gem_mmap_size(obj);
>> +     ret = _drm_gem_create_mmap_offset_size(obj, size);
>> +     if (ret) {
>> +             dev_err(dev->dev, "could not allocate mmap offset\n");
>> +             return 0;
>>       }
>>
>> -     return (uint64_t)obj->map_list.hash.key << PAGE_SHIFT;
>> +     return drm_vma_node_offset_addr(&obj->vma_node);
>>  }
>>
>>  uint64_t omap_gem_mmap_offset(struct drm_gem_object *obj)
>> @@ -997,12 +997,11 @@ void omap_gem_describe(struct drm_gem_object *obj, struct seq_file *m)
>>  {
>>       struct drm_device *dev = obj->dev;
>>       struct omap_gem_object *omap_obj = to_omap_bo(obj);
>> -     uint64_t off = 0;
>> +     uint64_t off;
>>
>>       WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>>
>> -     if (obj->map_list.map)
>> -             off = (uint64_t)obj->map_list.hash.key;
>> +     off = drm_vma_node_start(&obj->vma_node);
>>
>>       seq_printf(m, "%08x: %2d (%2d) %08llx %08Zx (%2d) %p %4d",
>>                       omap_obj->flags, obj->name, obj->refcount.refcount.counter,
>> @@ -1309,8 +1308,7 @@ void omap_gem_free_object(struct drm_gem_object *obj)
>>
>>       list_del(&omap_obj->mm_list);
>>
>> -     if (obj->map_list.map)
>> -             drm_gem_free_mmap_offset(obj);
>> +     drm_gem_free_mmap_offset(obj);
>>
>>       /* this means the object is still pinned.. which really should
>>        * not happen.  I think..
>> diff --git a/drivers/gpu/drm/omapdrm/omap_gem_helpers.c b/drivers/gpu/drm/omapdrm/omap_gem_helpers.c
>> index f9eb679..dbb1575 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_gem_helpers.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_gem_helpers.c
>> @@ -118,52 +118,7 @@ _drm_gem_create_mmap_offset_size(struct drm_gem_object *obj, size_t size)
>>  {
>>       struct drm_device *dev = obj->dev;
>>       struct drm_gem_mm *mm = dev->mm_private;
>> -     struct drm_map_list *list;
>> -     struct drm_local_map *map;
>> -     int ret = 0;
>> -
>> -     /* Set the object up for mmap'ing */
>> -     list = &obj->map_list;
>> -     list->map = kzalloc(sizeof(struct drm_map_list), GFP_KERNEL);
>> -     if (!list->map)
>> -             return -ENOMEM;
>> -
>> -     map = list->map;
>> -     map->type = _DRM_GEM;
>> -     map->size = size;
>> -     map->handle = obj;
>> -
>> -     /* Get a DRM GEM mmap offset allocated... */
>> -     list->file_offset_node = drm_mm_search_free(&mm->offset_manager,
>> -                     size / PAGE_SIZE, 0, 0);
>> -
>> -     if (!list->file_offset_node) {
>> -             DRM_ERROR("failed to allocate offset for bo %d\n", obj->name);
>> -             ret = -ENOSPC;
>> -             goto out_free_list;
>> -     }
>> -
>> -     list->file_offset_node = drm_mm_get_block(list->file_offset_node,
>> -                     size / PAGE_SIZE, 0);
>> -     if (!list->file_offset_node) {
>> -             ret = -ENOMEM;
>> -             goto out_free_list;
>> -     }
>> -
>> -     list->hash.key = list->file_offset_node->start;
>> -     ret = drm_ht_insert_item(&mm->offset_hash, &list->hash);
>> -     if (ret) {
>> -             DRM_ERROR("failed to add to map hash\n");
>> -             goto out_free_mm;
>> -     }
>> -
>> -     return 0;
>> -
>> -out_free_mm:
>> -     drm_mm_put_block(list->file_offset_node);
>> -out_free_list:
>> -     kfree(list->map);
>> -     list->map = NULL;
>>
>> -     return ret;
>> +     return drm_vma_offset_add(&mm->vma_manager, &obj->vma_node,
>> +                               size / PAGE_SIZE);
>>  }
>> diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
>> index ef034fa..2a4cb2f 100644
>> --- a/drivers/gpu/drm/udl/udl_gem.c
>> +++ b/drivers/gpu/drm/udl/udl_gem.c
>> @@ -223,8 +223,7 @@ void udl_gem_free_object(struct drm_gem_object *gem_obj)
>>       if (obj->pages)
>>               udl_gem_put_pages(obj);
>>
>> -     if (gem_obj->map_list.map)
>> -             drm_gem_free_mmap_offset(gem_obj);
>> +     drm_gem_free_mmap_offset(gem_obj);
>>  }
>>
>>  /* the dumb interface doesn't work with the GEM straight MMAP
>> @@ -247,13 +246,11 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev,
>>       ret = udl_gem_get_pages(gobj, GFP_KERNEL);
>>       if (ret)
>>               goto out;
>> -     if (!gobj->base.map_list.map) {
>> -             ret = drm_gem_create_mmap_offset(obj);
>> -             if (ret)
>> -                     goto out;
>> -     }
>> +     ret = drm_gem_create_mmap_offset(obj);
>> +     if (ret)
>> +             goto out;
>>
>> -     *offset = (u64)gobj->base.map_list.hash.key << PAGE_SHIFT;
>> +     *offset = drm_vma_node_offset_addr(&gobj->base.vma_node);
>>
>>  out:
>>       drm_gem_object_unreference(&gobj->base);
>> diff --git a/drivers/gpu/host1x/drm/gem.c b/drivers/gpu/host1x/drm/gem.c
>> index c5e9a9b..bc323b3 100644
>> --- a/drivers/gpu/host1x/drm/gem.c
>> +++ b/drivers/gpu/host1x/drm/gem.c
>> @@ -108,7 +108,7 @@ static void tegra_bo_destroy(struct drm_device *drm, struct tegra_bo *bo)
>>
>>  unsigned int tegra_bo_get_mmap_offset(struct tegra_bo *bo)
>>  {
>> -     return (unsigned int)bo->gem.map_list.hash.key << PAGE_SHIFT;
>> +     return (unsigned int)drm_vma_node_offset_addr(&bo->gem.vma_node);
>
> Ok, tegra is pretty funny here about mmap offsets, using an int.
> Whatever, pre-existing fanciness so not our problem.

All TTM helpers use "unsigned int", too. We can fix that in a followup patch.

>>  }
>>
>>  struct tegra_bo *tegra_bo_create(struct drm_device *drm, unsigned int size)
>> @@ -182,8 +182,7 @@ void tegra_bo_free_object(struct drm_gem_object *gem)
>>  {
>>       struct tegra_bo *bo = to_tegra_bo(gem);
>>
>> -     if (gem->map_list.map)
>> -             drm_gem_free_mmap_offset(gem);
>> +     drm_gem_free_mmap_offset(gem);
>>
>>       drm_gem_object_release(gem);
>>       tegra_bo_destroy(gem->dev, bo);
>> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
>> index 0ab6a09..4b518e0 100644
>> --- a/include/drm/drmP.h
>> +++ b/include/drm/drmP.h
>> @@ -71,6 +71,7 @@
>>  #include <asm/pgalloc.h>
>>  #include <drm/drm.h>
>>  #include <drm/drm_sarea.h>
>> +#include <drm/drm_vma_manager.h>
>>
>>  #include <linux/idr.h>
>>
>> @@ -587,7 +588,6 @@ struct drm_map_list {
>>       struct drm_local_map *map;      /**< mapping */
>>       uint64_t user_token;
>>       struct drm_master *master;
>> -     struct drm_mm_node *file_offset_node;   /**< fake offset */
>>  };
>>
>>  /**
>> @@ -622,8 +622,7 @@ struct drm_ati_pcigart_info {
>>   * GEM specific mm private for tracking GEM objects
>>   */
>>  struct drm_gem_mm {
>> -     struct drm_mm offset_manager;   /**< Offset mgmt for buffer objects */
>> -     struct drm_open_hash offset_hash; /**< User token hash table for maps */
>> +     struct drm_vma_offset_manager vma_manager;
>>  };
>>
>>  /**
>> @@ -644,7 +643,7 @@ struct drm_gem_object {
>>       struct file *filp;
>>
>>       /* Mapping info for this object */
>> -     struct drm_map_list map_list;
>> +     struct drm_vma_offset_node vma_node;
>>
>>       /**
>>        * Size of the object, in bytes.  Immutable over the object's
>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>> index 238a166..272580c 100644
>> --- a/include/uapi/drm/drm.h
>> +++ b/include/uapi/drm/drm.h
>> @@ -181,7 +181,7 @@ enum drm_map_type {
>>       _DRM_AGP = 3,             /**< AGP/GART */
>>       _DRM_SCATTER_GATHER = 4,  /**< Scatter/gather memory for PCI DMA */
>>       _DRM_CONSISTENT = 5,      /**< Consistent memory for PCI DMA */
>> -     _DRM_GEM = 6,             /**< GEM object */
>> +     _DRM_GEM = 6,             /**< GEM object (obsolete) */
>>  };
>>
>>  /**
>> --
>> 1.8.3.3
>
> A few small things, with those addressed this is:
>
> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>
> I guess with the common infrastructure we'll have a bit of room for
> follow-up cleanups, but that can be done on especially rainy autumn days
> ;-)

Thanks!
David


More information about the dri-devel mailing list