[PATCH 0/7] DRM: Remove gem_init_object() and friends

[PATCH 0/7] DRM: Remove gem_init_object() and friends

[David Herrmann]

Hi

This series removes any access to gem_obj->driver_private from all drivers and
then drops drm_gem_object_alloc() with its ->gem_init_object() driver callback.

All this is only needed if a gem object is not embedded into the driver's bo.
However, all drivers (except for nouveau, see patch #6) embed the objects. No
need to allow dynamic allocations, anymore.

Patches #1 to #5 are trivial and only remove dead code. Patch #6 converts
nouveau to embed gem objects into nouveau_bo. I tested this on my nv50 and it
runs fine. Patch #7 drops gem_init_object() from all drivers as it is unused and
the callbacks are all empty (or BUG()).

And the diff-stat looks pretty nice, too.

Cheers
David

David Herrmann (7):
  drm/ast: remove unused driver_private access
  drm/mgag200: remove unused driver_private access
  drm/cirrus: remove unused driver_private access
  drm/qxl: remove unused object_pin/unpin() helpers
  drm/radeon: remove stale gem->driver_private access
  drm/nouveau: embed gem object in nouveau_bo
  drm: kill ->gem_init_object() and friends

 drivers/gpu/drm/ast/ast_drv.c             |  1 -
 drivers/gpu/drm/ast/ast_drv.h             |  1 -
 drivers/gpu/drm/ast/ast_main.c            |  6 -----
 drivers/gpu/drm/ast/ast_ttm.c             |  1 -
 drivers/gpu/drm/cirrus/cirrus_drv.c       |  1 -
 drivers/gpu/drm/cirrus/cirrus_drv.h       |  1 -
 drivers/gpu/drm/cirrus/cirrus_main.c      |  6 -----
 drivers/gpu/drm/cirrus/cirrus_ttm.c       |  1 -
 drivers/gpu/drm/drm_gem.c                 | 29 ---------------------
 drivers/gpu/drm/exynos/exynos_drm_drv.c   |  1 -
 drivers/gpu/drm/exynos/exynos_drm_gem.c   |  5 ----
 drivers/gpu/drm/exynos/exynos_drm_gem.h   |  3 ---
 drivers/gpu/drm/gma500/gem.c              |  5 ----
 drivers/gpu/drm/gma500/psb_drv.c          |  1 -
 drivers/gpu/drm/gma500/psb_drv.h          |  1 -
 drivers/gpu/drm/i915/i915_drv.c           |  1 -
 drivers/gpu/drm/i915/i915_drv.h           |  1 -
 drivers/gpu/drm/i915/i915_gem.c           |  7 ------
 drivers/gpu/drm/mgag200/mgag200_drv.c     |  1 -
 drivers/gpu/drm/mgag200/mgag200_drv.h     |  1 -
 drivers/gpu/drm/mgag200/mgag200_main.c    |  6 -----
 drivers/gpu/drm/mgag200/mgag200_ttm.c     |  1 -
 drivers/gpu/drm/nouveau/nouveau_abi16.c   |  4 +--
 drivers/gpu/drm/nouveau/nouveau_bo.c      |  2 +-
 drivers/gpu/drm/nouveau/nouveau_bo.h      |  5 +++-
 drivers/gpu/drm/nouveau/nouveau_display.c | 10 ++++----
 drivers/gpu/drm/nouveau/nouveau_drm.c     |  1 -
 drivers/gpu/drm/nouveau/nouveau_fbcon.c   |  2 +-
 drivers/gpu/drm/nouveau/nouveau_gem.c     | 42 +++++++++++++------------------
 drivers/gpu/drm/nouveau/nouveau_gem.h     |  3 +--
 drivers/gpu/drm/nouveau/nouveau_prime.c   | 10 +++++---
 drivers/gpu/drm/omapdrm/omap_drv.c        |  1 -
 drivers/gpu/drm/omapdrm/omap_drv.h        |  1 -
 drivers/gpu/drm/omapdrm/omap_gem.c        |  5 ----
 drivers/gpu/drm/qxl/qxl_drv.c             |  1 -
 drivers/gpu/drm/qxl/qxl_drv.h             |  4 ---
 drivers/gpu/drm/qxl/qxl_gem.c             | 32 -----------------------
 drivers/gpu/drm/qxl/qxl_object.c          |  1 -
 drivers/gpu/drm/radeon/radeon_drv.c       |  2 --
 drivers/gpu/drm/radeon/radeon_gem.c       |  7 ------
 drivers/gpu/drm/radeon/radeon_object.c    |  1 -
 drivers/gpu/drm/radeon/radeon_prime.c     |  1 -
 drivers/gpu/drm/udl/udl_drv.c             |  1 -
 drivers/gpu/drm/udl/udl_drv.h             |  1 -
 drivers/gpu/drm/udl/udl_gem.c             |  7 ------
 include/drm/drmP.h                        |  5 ----
 46 files changed, 37 insertions(+), 193 deletions(-)

-- 
1.8.3.4

[PATCH 1/7] drm/ast: remove unused driver_private access

[David Herrmann]

gem_bo->driver_private is never read by ast nor DRM core. No need to set
it. Besides, drm core clears it during setup, anyway.

Cc: Dave Airlie <airlied@gmail.com>
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/ast/ast_ttm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/ast/ast_ttm.c b/drivers/gpu/drm/ast/ast_ttm.c
index 98d6708..cf1c833 100644
--- a/drivers/gpu/drm/ast/ast_ttm.c
+++ b/drivers/gpu/drm/ast/ast_ttm.c
@@ -321,7 +321,6 @@ int ast_bo_create(struct drm_device *dev, int size, int align,
 		return ret;
 	}
 
-	astbo->gem.driver_private = NULL;
 	astbo->bo.bdev = &ast->ttm.bdev;
 
 	ast_ttm_placement(astbo, TTM_PL_FLAG_VRAM | TTM_PL_FLAG_SYSTEM);
-- 
1.8.3.4

[PATCH 2/7] drm/mgag200: remove unused driver_private access

[David Herrmann]

gem_bo->driver_private is never read by mgag200 nor DRM core. No need to
set it. Besides, drm core clears it during setup, anyway.

Cc: Dave Airlie <airlied@gmail.com>
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/mgag200/mgag200_ttm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_ttm.c b/drivers/gpu/drm/mgag200/mgag200_ttm.c
index 3acb2b0..6cf3fa0 100644
--- a/drivers/gpu/drm/mgag200/mgag200_ttm.c
+++ b/drivers/gpu/drm/mgag200/mgag200_ttm.c
@@ -321,7 +321,6 @@ int mgag200_bo_create(struct drm_device *dev, int size, int align,
 		return ret;
 	}
 
-	mgabo->gem.driver_private = NULL;
 	mgabo->bo.bdev = &mdev->ttm.bdev;
 
 	mgag200_ttm_placement(mgabo, TTM_PL_FLAG_VRAM | TTM_PL_FLAG_SYSTEM);
-- 
1.8.3.4

[PATCH 3/7] drm/cirrus: remove unused driver_private access

[David Herrmann]

gem_bo->driver_private is never read by cirrus nor DRM core. No need to
set it. Besides, drm core clears it during setup, anyway.

Cc: Dave Airlie <airlied@gmail.com>
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/cirrus/cirrus_ttm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/cirrus/cirrus_ttm.c b/drivers/gpu/drm/cirrus/cirrus_ttm.c
index 0047012..bf8a506 100644
--- a/drivers/gpu/drm/cirrus/cirrus_ttm.c
+++ b/drivers/gpu/drm/cirrus/cirrus_ttm.c
@@ -326,7 +326,6 @@ int cirrus_bo_create(struct drm_device *dev, int size, int align,
 		return ret;
 	}
 
-	cirrusbo->gem.driver_private = NULL;
 	cirrusbo->bo.bdev = &cirrus->ttm.bdev;
 
 	cirrus_ttm_placement(cirrusbo, TTM_PL_FLAG_VRAM | TTM_PL_FLAG_SYSTEM);
-- 
1.8.3.4

[PATCH 4/7] drm/qxl: remove unused object_pin/unpin() helpers

[David Herrmann]

These two helpers are unused. Remove them. They rely on
gem_obj->driver_private, which is set to NULL during setup. As this field
isn't used by the driver, anymore, we can remove this assignment as well.

Cc: Dave Airlie <airlied@gmail.com>
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/qxl/qxl_drv.h    |  3 ---
 drivers/gpu/drm/qxl/qxl_gem.c    | 26 --------------------------
 drivers/gpu/drm/qxl/qxl_object.c |  1 -
 3 files changed, 30 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index afd09d4..77bfed4 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -396,9 +396,6 @@ int qxl_gem_object_create(struct qxl_device *qdev, int size,
 			  bool discardable, bool kernel,
 			  struct qxl_surface *surf,
 			  struct drm_gem_object **obj);
-int qxl_gem_object_pin(struct drm_gem_object *obj, uint32_t pin_domain,
-			  uint64_t *gpu_addr);
-void qxl_gem_object_unpin(struct drm_gem_object *obj);
 int qxl_gem_object_create_with_handle(struct qxl_device *qdev,
 				      struct drm_file *file_priv,
 				      u32 domain,
diff --git a/drivers/gpu/drm/qxl/qxl_gem.c b/drivers/gpu/drm/qxl/qxl_gem.c
index a235693..31fd4c3 100644
--- a/drivers/gpu/drm/qxl/qxl_gem.c
+++ b/drivers/gpu/drm/qxl/qxl_gem.c
@@ -101,32 +101,6 @@ int qxl_gem_object_create_with_handle(struct qxl_device *qdev,
 	return 0;
 }
 
-int qxl_gem_object_pin(struct drm_gem_object *obj, uint32_t pin_domain,
-			  uint64_t *gpu_addr)
-{
-	struct qxl_bo *qobj = obj->driver_private;
-	int r;
-
-	r = qxl_bo_reserve(qobj, false);
-	if (unlikely(r != 0))
-		return r;
-	r = qxl_bo_pin(qobj, pin_domain, gpu_addr);
-	qxl_bo_unreserve(qobj);
-	return r;
-}
-
-void qxl_gem_object_unpin(struct drm_gem_object *obj)
-{
-	struct qxl_bo *qobj = obj->driver_private;
-	int r;
-
-	r = qxl_bo_reserve(qobj, false);
-	if (likely(r == 0)) {
-		qxl_bo_unpin(qobj);
-		qxl_bo_unreserve(qobj);
-	}
-}
-
 int qxl_gem_object_open(struct drm_gem_object *obj, struct drm_file *file_priv)
 {
 	return 0;
diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
index 1191fe7..7ff7cbf 100644
--- a/drivers/gpu/drm/qxl/qxl_object.c
+++ b/drivers/gpu/drm/qxl/qxl_object.c
@@ -97,7 +97,6 @@ int qxl_bo_create(struct qxl_device *qdev,
 		kfree(bo);
 		return r;
 	}
-	bo->gem_base.driver_private = NULL;
 	bo->type = domain;
 	bo->pin_count = 0;
 	bo->surface_id = 0;
-- 
1.8.3.4

[PATCH 5/7] drm/radeon: remove stale gem->driver_private access

[David Herrmann]

This field is never read. No need to set it in radeon. Besides, DRM gem
core clears it during setup, anyway.

Cc: Dave Airlie <airlied@gmail.com>
Cc: Alex Deucher <alexdeucher@gmail.com>
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/radeon/radeon_object.c | 1 -
 drivers/gpu/drm/radeon/radeon_prime.c  | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c
index 2020bf4..c0fa4aa 100644
--- a/drivers/gpu/drm/radeon/radeon_object.c
+++ b/drivers/gpu/drm/radeon/radeon_object.c
@@ -142,7 +142,6 @@ int radeon_bo_create(struct radeon_device *rdev,
 		return r;
 	}
 	bo->rdev = rdev;
-	bo->gem_base.driver_private = NULL;
 	bo->surface_reg = -1;
 	INIT_LIST_HEAD(&bo->list);
 	INIT_LIST_HEAD(&bo->va);
diff --git a/drivers/gpu/drm/radeon/radeon_prime.c b/drivers/gpu/drm/radeon/radeon_prime.c
index 65b9eab..2007456 100644
--- a/drivers/gpu/drm/radeon/radeon_prime.c
+++ b/drivers/gpu/drm/radeon/radeon_prime.c
@@ -68,7 +68,6 @@ struct drm_gem_object *radeon_gem_prime_import_sg_table(struct drm_device *dev,
 			       RADEON_GEM_DOMAIN_GTT, sg, &bo);
 	if (ret)
 		return ERR_PTR(ret);
-	bo->gem_base.driver_private = bo;
 
 	mutex_lock(&rdev->gem.mutex);
 	list_add_tail(&bo->list, &rdev->gem.objects);
-- 
1.8.3.4

[PATCH 6/7] drm/nouveau: embed gem object in nouveau_bo

[David Herrmann]

There is no reason to keep the gem object separately allocated. nouveau is
the last user of gem_obj->driver_private, so if we embed it, we can get
rid of 8bytes per gem-object.

The implementation follows the radeon driver. bo->gem is only valid, iff
the bo was created via the gem helpers _and_ iff the user holds a valid
gem reference. That is, as the gem object holds a reference to the
nouveau_bo. If you use nouveau_ref() to gain a bo reference, you are not
guaranteed to also hold a gem reference. The gem object might get
destroyed after the last user drops the gem-ref via
drm_gem_object_unreference(). Use drm_gem_object_reference() to gain a
gem-reference.

For debugging, we can use bo->gem.filp != NULL to test whether a gem-bo is
valid. However, this shouldn't be used for real functionality to avoid
gem-internal dependencies.

Note that the implementation follows the previous style. However, we no
longer can check for bo->gem != NULL to test for a valid gem object. This
wasn't done before, so we should be safe now.

Cc: Ben Skeggs <skeggsb@gmail.com>
Cc: Maarten Lankhorst <maarten.lankhorst@canonical.com>
Cc: Martin Peres <martin.peres@labri.fr>
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/nouveau/nouveau_abi16.c   |  4 ++--
 drivers/gpu/drm/nouveau/nouveau_bo.c      |  2 +-
 drivers/gpu/drm/nouveau/nouveau_bo.h      |  5 ++++-
 drivers/gpu/drm/nouveau/nouveau_display.c | 10 ++++-----
 drivers/gpu/drm/nouveau/nouveau_fbcon.c   |  2 +-
 drivers/gpu/drm/nouveau/nouveau_gem.c     | 36 +++++++++++++++----------------
 drivers/gpu/drm/nouveau/nouveau_gem.h     |  2 +-
 drivers/gpu/drm/nouveau/nouveau_prime.c   | 10 +++++----
 8 files changed, 37 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_abi16.c b/drivers/gpu/drm/nouveau/nouveau_abi16.c
index 8f467e7..3897549 100644
--- a/drivers/gpu/drm/nouveau/nouveau_abi16.c
+++ b/drivers/gpu/drm/nouveau/nouveau_abi16.c
@@ -130,7 +130,7 @@ nouveau_abi16_chan_fini(struct nouveau_abi16 *abi16,
 	if (chan->ntfy) {
 		nouveau_bo_vma_del(chan->ntfy, &chan->ntfy_vma);
 		nouveau_bo_unpin(chan->ntfy);
-		drm_gem_object_unreference_unlocked(chan->ntfy->gem);
+		drm_gem_object_unreference_unlocked(&chan->ntfy->gem);
 	}
 
 	if (chan->heap.block_size)
@@ -320,7 +320,7 @@ nouveau_abi16_ioctl_channel_alloc(ABI16_IOCTL_ARGS)
 			goto done;
 	}
 
-	ret = drm_gem_handle_create(file_priv, chan->ntfy->gem,
+	ret = drm_gem_handle_create(file_priv, &chan->ntfy->gem,
 				    &init->notifier_handle);
 	if (ret)
 		goto done;
diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
index 4e7ee5f..7767a89 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.c
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
@@ -146,7 +146,7 @@ nouveau_bo_del_ttm(struct ttm_buffer_object *bo)
 	struct drm_device *dev = drm->dev;
 	struct nouveau_bo *nvbo = nouveau_bo(bo);
 
-	if (unlikely(nvbo->gem))
+	if (unlikely(nvbo->gem.filp))
 		DRM_ERROR("bo %p still attached to GEM object\n", bo);
 	WARN_ON(nvbo->pin_refcnt > 0);
 	nv10_bo_put_tile_region(dev, nvbo->tile, NULL);
diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.h b/drivers/gpu/drm/nouveau/nouveau_bo.h
index 653dbbb..ff17c1f 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.h
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.h
@@ -27,7 +27,10 @@ struct nouveau_bo {
 	u32 tile_flags;
 	struct nouveau_drm_tile *tile;
 
-	struct drm_gem_object *gem;
+	/* Only valid if allocated via nouveau_gem_new() and iff you hold a
+	 * gem reference to it! For debugging, use gem.filp != NULL to test
+	 * whether it is valid. */
+	struct drm_gem_object gem;
 
 	/* protect by the ttm reservation lock */
 	int pin_refcnt;
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
index 78637af..85571ed 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -50,7 +50,7 @@ nouveau_user_framebuffer_destroy(struct drm_framebuffer *drm_fb)
 	struct nouveau_framebuffer *fb = nouveau_framebuffer(drm_fb);
 
 	if (fb->nvbo)
-		drm_gem_object_unreference_unlocked(fb->nvbo->gem);
+		drm_gem_object_unreference_unlocked(&fb->nvbo->gem);
 
 	drm_framebuffer_cleanup(drm_fb);
 	kfree(fb);
@@ -63,7 +63,7 @@ nouveau_user_framebuffer_create_handle(struct drm_framebuffer *drm_fb,
 {
 	struct nouveau_framebuffer *fb = nouveau_framebuffer(drm_fb);
 
-	return drm_gem_handle_create(file_priv, fb->nvbo->gem, handle);
+	return drm_gem_handle_create(file_priv, &fb->nvbo->gem, handle);
 }
 
 static const struct drm_framebuffer_funcs nouveau_framebuffer_funcs = {
@@ -668,8 +668,8 @@ nouveau_display_dumb_create(struct drm_file *file_priv, struct drm_device *dev,
 	if (ret)
 		return ret;
 
-	ret = drm_gem_handle_create(file_priv, bo->gem, &args->handle);
-	drm_gem_object_unreference_unlocked(bo->gem);
+	ret = drm_gem_handle_create(file_priv, &bo->gem, &args->handle);
+	drm_gem_object_unreference_unlocked(&bo->gem);
 	return ret;
 }
 
@@ -682,7 +682,7 @@ nouveau_display_dumb_map_offset(struct drm_file *file_priv,
 
 	gem = drm_gem_object_lookup(dev, file_priv, handle);
 	if (gem) {
-		struct nouveau_bo *bo = gem->driver_private;
+		struct nouveau_bo *bo = nouveau_gem_object(gem);
 		*poffset = drm_vma_node_offset_addr(&bo->bo.vma_node);
 		drm_gem_object_unreference_unlocked(gem);
 		return 0;
diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
index 4c1bc06..e20b695 100644
--- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
+++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
@@ -419,7 +419,7 @@ nouveau_fbcon_destroy(struct drm_device *dev, struct nouveau_fbdev *fbcon)
 		nouveau_bo_unmap(nouveau_fb->nvbo);
 		nouveau_bo_vma_del(nouveau_fb->nvbo, &nouveau_fb->vma);
 		nouveau_bo_unpin(nouveau_fb->nvbo);
-		drm_gem_object_unreference_unlocked(nouveau_fb->nvbo->gem);
+		drm_gem_object_unreference_unlocked(&nouveau_fb->nvbo->gem);
 		nouveau_fb->nvbo = NULL;
 	}
 	drm_fb_helper_fini(&fbcon->helper);
diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
index 487242f..39278e9 100644
--- a/drivers/gpu/drm/nouveau/nouveau_gem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
@@ -43,20 +43,17 @@ nouveau_gem_object_new(struct drm_gem_object *gem)
 void
 nouveau_gem_object_del(struct drm_gem_object *gem)
 {
-	struct nouveau_bo *nvbo = gem->driver_private;
+	struct nouveau_bo *nvbo = nouveau_gem_object(gem);
 	struct ttm_buffer_object *bo = &nvbo->bo;
 
-	if (!nvbo)
-		return;
-	nvbo->gem = NULL;
-
 	if (gem->import_attach)
 		drm_prime_gem_destroy(gem, nvbo->bo.sg);
 
-	ttm_bo_unref(&bo);
-
 	drm_gem_object_release(gem);
-	kfree(gem);
+
+	/* reset filp so nouveau_bo_del_ttm() can test for it */
+	gem->filp = NULL;
+	ttm_bo_unref(&bo);
 }
 
 int
@@ -186,14 +183,15 @@ nouveau_gem_new(struct drm_device *dev, int size, int align, uint32_t domain,
 	if (nv_device(drm->device)->card_type >= NV_50)
 		nvbo->valid_domains &= domain;
 
-	nvbo->gem = drm_gem_object_alloc(dev, nvbo->bo.mem.size);
-	if (!nvbo->gem) {
+	/* Initialize the embedded gem-object. We return a single gem-reference
+	 * to the caller, instead of a normal nouveau_bo ttm reference. */
+	ret = drm_gem_object_init(dev, &nvbo->gem, nvbo->bo.mem.size);
+	if (ret) {
 		nouveau_bo_ref(NULL, pnvbo);
 		return -ENOMEM;
 	}
 
-	nvbo->bo.persistent_swap_storage = nvbo->gem->filp;
-	nvbo->gem->driver_private = nvbo;
+	nvbo->bo.persistent_swap_storage = nvbo->gem.filp;
 	return 0;
 }
 
@@ -250,15 +248,15 @@ nouveau_gem_ioctl_new(struct drm_device *dev, void *data,
 	if (ret)
 		return ret;
 
-	ret = drm_gem_handle_create(file_priv, nvbo->gem, &req->info.handle);
+	ret = drm_gem_handle_create(file_priv, &nvbo->gem, &req->info.handle);
 	if (ret == 0) {
-		ret = nouveau_gem_info(file_priv, nvbo->gem, &req->info);
+		ret = nouveau_gem_info(file_priv, &nvbo->gem, &req->info);
 		if (ret)
 			drm_gem_handle_delete(file_priv, req->info.handle);
 	}
 
 	/* drop reference from allocate - handle holds it now */
-	drm_gem_object_unreference_unlocked(nvbo->gem);
+	drm_gem_object_unreference_unlocked(&nvbo->gem);
 	return ret;
 }
 
@@ -266,7 +264,7 @@ static int
 nouveau_gem_set_domain(struct drm_gem_object *gem, uint32_t read_domains,
 		       uint32_t write_domains, uint32_t valid_domains)
 {
-	struct nouveau_bo *nvbo = gem->driver_private;
+	struct nouveau_bo *nvbo = nouveau_gem_object(gem);
 	struct ttm_buffer_object *bo = &nvbo->bo;
 	uint32_t domains = valid_domains & nvbo->valid_domains &
 		(write_domains ? write_domains : read_domains);
@@ -327,7 +325,7 @@ validate_fini_list(struct list_head *list, struct nouveau_fence *fence,
 		list_del(&nvbo->entry);
 		nvbo->reserved_by = NULL;
 		ttm_bo_unreserve_ticket(&nvbo->bo, ticket);
-		drm_gem_object_unreference_unlocked(nvbo->gem);
+		drm_gem_object_unreference_unlocked(&nvbo->gem);
 	}
 }
 
@@ -376,7 +374,7 @@ retry:
 			validate_fini(op, NULL);
 			return -ENOENT;
 		}
-		nvbo = gem->driver_private;
+		nvbo = nouveau_gem_object(gem);
 		if (nvbo == res_bo) {
 			res_bo = NULL;
 			drm_gem_object_unreference_unlocked(gem);
@@ -478,7 +476,7 @@ validate_list(struct nouveau_channel *chan, struct nouveau_cli *cli,
 			return ret;
 		}
 
-		ret = nouveau_gem_set_domain(nvbo->gem, b->read_domains,
+		ret = nouveau_gem_set_domain(&nvbo->gem, b->read_domains,
 					     b->write_domains,
 					     b->valid_domains);
 		if (unlikely(ret)) {
diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.h b/drivers/gpu/drm/nouveau/nouveau_gem.h
index 502e429..b535895 100644
--- a/drivers/gpu/drm/nouveau/nouveau_gem.h
+++ b/drivers/gpu/drm/nouveau/nouveau_gem.h
@@ -12,7 +12,7 @@
 static inline struct nouveau_bo *
 nouveau_gem_object(struct drm_gem_object *gem)
 {
-	return gem ? gem->driver_private : NULL;
+	return gem ? container_of(gem, struct nouveau_bo, gem) : NULL;
 }
 
 /* nouveau_gem.c */
diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c
index e90468d..51a2cb1 100644
--- a/drivers/gpu/drm/nouveau/nouveau_prime.c
+++ b/drivers/gpu/drm/nouveau/nouveau_prime.c
@@ -71,14 +71,16 @@ struct drm_gem_object *nouveau_gem_prime_import_sg_table(struct drm_device *dev,
 		return ERR_PTR(ret);
 
 	nvbo->valid_domains = NOUVEAU_GEM_DOMAIN_GART;
-	nvbo->gem = drm_gem_object_alloc(dev, nvbo->bo.mem.size);
-	if (!nvbo->gem) {
+
+	/* Initialize the embedded gem-object. We return a single gem-reference
+	 * to the caller, instead of a normal nouveau_bo ttm reference. */
+	ret = drm_gem_object_init(dev, &nvbo->gem, nvbo->bo.mem.size);
+	if (ret) {
 		nouveau_bo_ref(NULL, &nvbo);
 		return ERR_PTR(-ENOMEM);
 	}
 
-	nvbo->gem->driver_private = nvbo;
-	return nvbo->gem;
+	return &nvbo->gem;
 }
 
 int nouveau_gem_prime_pin(struct drm_gem_object *obj)
-- 
1.8.3.4

[PATCH 7/7] drm: kill ->gem_init_object() and friends

[David Herrmann]

All drivers embed gem-objects into their own buffer objects. There is no
reason to keep drm_gem_object_alloc(), gem->driver_private and
->gem_init_object() anymore.

New drivers are highly encouraged to do the same. There is no benefit in
allocating gem-objects separately.

Cc: Dave Airlie <airlied@gmail.com>
Cc: Alex Deucher <alexdeucher@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Jerome Glisse <jglisse@redhat.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Inki Dae <inki.dae@samsung.com>
Cc: Ben Skeggs <skeggsb@gmail.com>
Cc: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/ast/ast_drv.c           |  1 -
 drivers/gpu/drm/ast/ast_drv.h           |  1 -
 drivers/gpu/drm/ast/ast_main.c          |  6 ------
 drivers/gpu/drm/cirrus/cirrus_drv.c     |  1 -
 drivers/gpu/drm/cirrus/cirrus_drv.h     |  1 -
 drivers/gpu/drm/cirrus/cirrus_main.c    |  6 ------
 drivers/gpu/drm/drm_gem.c               | 29 -----------------------------
 drivers/gpu/drm/exynos/exynos_drm_drv.c |  1 -
 drivers/gpu/drm/exynos/exynos_drm_gem.c |  5 -----
 drivers/gpu/drm/exynos/exynos_drm_gem.h |  3 ---
 drivers/gpu/drm/gma500/gem.c            |  5 -----
 drivers/gpu/drm/gma500/psb_drv.c        |  1 -
 drivers/gpu/drm/gma500/psb_drv.h        |  1 -
 drivers/gpu/drm/i915/i915_drv.c         |  1 -
 drivers/gpu/drm/i915/i915_drv.h         |  1 -
 drivers/gpu/drm/i915/i915_gem.c         |  7 -------
 drivers/gpu/drm/mgag200/mgag200_drv.c   |  1 -
 drivers/gpu/drm/mgag200/mgag200_drv.h   |  1 -
 drivers/gpu/drm/mgag200/mgag200_main.c  |  6 ------
 drivers/gpu/drm/nouveau/nouveau_drm.c   |  1 -
 drivers/gpu/drm/nouveau/nouveau_gem.c   |  6 ------
 drivers/gpu/drm/nouveau/nouveau_gem.h   |  1 -
 drivers/gpu/drm/omapdrm/omap_drv.c      |  1 -
 drivers/gpu/drm/omapdrm/omap_drv.h      |  1 -
 drivers/gpu/drm/omapdrm/omap_gem.c      |  5 -----
 drivers/gpu/drm/qxl/qxl_drv.c           |  1 -
 drivers/gpu/drm/qxl/qxl_drv.h           |  1 -
 drivers/gpu/drm/qxl/qxl_gem.c           |  6 ------
 drivers/gpu/drm/radeon/radeon_drv.c     |  2 --
 drivers/gpu/drm/radeon/radeon_gem.c     |  7 -------
 drivers/gpu/drm/udl/udl_drv.c           |  1 -
 drivers/gpu/drm/udl/udl_drv.h           |  1 -
 drivers/gpu/drm/udl/udl_gem.c           |  7 -------
 include/drm/drmP.h                      |  5 -----
 34 files changed, 124 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index a144fb0..7b1b47a 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -212,7 +212,6 @@ static struct drm_driver driver = {
 	.minor = DRIVER_MINOR,
 	.patchlevel = DRIVER_PATCHLEVEL,
 
-	.gem_init_object = ast_gem_init_object,
 	.gem_free_object = ast_gem_free_object,
 	.dumb_create = ast_dumb_create,
 	.dumb_map_offset = ast_dumb_mmap_offset,
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 796dbb2..a6b7b29 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -323,7 +323,6 @@ extern int ast_dumb_create(struct drm_file *file,
 			   struct drm_device *dev,
 			   struct drm_mode_create_dumb *args);
 
-extern int ast_gem_init_object(struct drm_gem_object *obj);
 extern void ast_gem_free_object(struct drm_gem_object *obj);
 extern int ast_dumb_mmap_offset(struct drm_file *file,
 				struct drm_device *dev,
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 7f6152d..af0b868 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -449,12 +449,6 @@ int ast_dumb_create(struct drm_file *file,
 	return 0;
 }
 
-int ast_gem_init_object(struct drm_gem_object *obj)
-{
-	BUG();
-	return 0;
-}
-
 void ast_bo_unref(struct ast_bo **bo)
 {
 	struct ttm_buffer_object *tbo;
diff --git a/drivers/gpu/drm/cirrus/cirrus_drv.c b/drivers/gpu/drm/cirrus/cirrus_drv.c
index d35d99c..76fa5cd 100644
--- a/drivers/gpu/drm/cirrus/cirrus_drv.c
+++ b/drivers/gpu/drm/cirrus/cirrus_drv.c
@@ -98,7 +98,6 @@ static struct drm_driver driver = {
 	.major = DRIVER_MAJOR,
 	.minor = DRIVER_MINOR,
 	.patchlevel = DRIVER_PATCHLEVEL,
-	.gem_init_object = cirrus_gem_init_object,
 	.gem_free_object = cirrus_gem_free_object,
 	.dumb_create = cirrus_dumb_create,
 	.dumb_map_offset = cirrus_dumb_mmap_offset,
diff --git a/drivers/gpu/drm/cirrus/cirrus_drv.h b/drivers/gpu/drm/cirrus/cirrus_drv.h
index 9b0bb91..b6aded7 100644
--- a/drivers/gpu/drm/cirrus/cirrus_drv.h
+++ b/drivers/gpu/drm/cirrus/cirrus_drv.h
@@ -191,7 +191,6 @@ int cirrus_device_init(struct cirrus_device *cdev,
 		      struct pci_dev *pdev,
 		      uint32_t flags);
 void cirrus_device_fini(struct cirrus_device *cdev);
-int cirrus_gem_init_object(struct drm_gem_object *obj);
 void cirrus_gem_free_object(struct drm_gem_object *obj);
 int cirrus_dumb_mmap_offset(struct drm_file *file,
 			    struct drm_device *dev,
diff --git a/drivers/gpu/drm/cirrus/cirrus_main.c b/drivers/gpu/drm/cirrus/cirrus_main.c
index f130a53..78e76f2 100644
--- a/drivers/gpu/drm/cirrus/cirrus_main.c
+++ b/drivers/gpu/drm/cirrus/cirrus_main.c
@@ -255,12 +255,6 @@ int cirrus_dumb_create(struct drm_file *file,
 	return 0;
 }
 
-int cirrus_gem_init_object(struct drm_gem_object *obj)
-{
-	BUG();
-	return 0;
-}
-
 void cirrus_bo_unref(struct cirrus_bo **bo)
 {
 	struct ttm_buffer_object *tbo;
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 9ab038c..ca5521e 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -159,35 +159,6 @@ void drm_gem_private_object_init(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_gem_private_object_init);
 
-/**
- * Allocate a GEM object of the specified size with shmfs backing store
- */
-struct drm_gem_object *
-drm_gem_object_alloc(struct drm_device *dev, size_t size)
-{
-	struct drm_gem_object *obj;
-
-	obj = kzalloc(sizeof(*obj), GFP_KERNEL);
-	if (!obj)
-		goto free;
-
-	if (drm_gem_object_init(dev, obj, size) != 0)
-		goto free;
-
-	if (dev->driver->gem_init_object != NULL &&
-	    dev->driver->gem_init_object(obj) != 0) {
-		goto fput;
-	}
-	return obj;
-fput:
-	/* Object_init mangles the global counters - readjust them. */
-	fput(obj->filp);
-free:
-	kfree(obj);
-	return NULL;
-}
-EXPORT_SYMBOL(drm_gem_object_alloc);
-
 static void
 drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp)
 {
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index df81d3c..c110138 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -266,7 +266,6 @@ static struct drm_driver exynos_drm_driver = {
 	.get_vblank_counter	= drm_vblank_count,
 	.enable_vblank		= exynos_drm_crtc_enable_vblank,
 	.disable_vblank		= exynos_drm_crtc_disable_vblank,
-	.gem_init_object	= exynos_drm_gem_init_object,
 	.gem_free_object	= exynos_drm_gem_free_object,
 	.gem_vm_ops		= &exynos_drm_gem_vm_ops,
 	.dumb_create		= exynos_drm_gem_dumb_create,
diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
index b904633..50b89ad 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
@@ -631,11 +631,6 @@ void exynos_gem_unmap_sgt_from_dma(struct drm_device *drm_dev,
 	dma_unmap_sg(drm_dev->dev, sgt->sgl, sgt->nents, dir);
 }
 
-int exynos_drm_gem_init_object(struct drm_gem_object *obj)
-{
-	return 0;
-}
-
 void exynos_drm_gem_free_object(struct drm_gem_object *obj)
 {
 	struct exynos_drm_gem_obj *exynos_gem_obj;
diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.h b/drivers/gpu/drm/exynos/exynos_drm_gem.h
index 09555af..702ec3a 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gem.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_gem.h
@@ -135,9 +135,6 @@ unsigned long exynos_drm_gem_get_size(struct drm_device *dev,
 						unsigned int gem_handle,
 						struct drm_file *file_priv);
 
-/* initialize gem object. */
-int exynos_drm_gem_init_object(struct drm_gem_object *obj);
-
 /* free gem object. */
 void exynos_drm_gem_free_object(struct drm_gem_object *gem_obj);
 
diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
index 10ae8c5..e2db48a 100644
--- a/drivers/gpu/drm/gma500/gem.c
+++ b/drivers/gpu/drm/gma500/gem.c
@@ -29,11 +29,6 @@
 #include <drm/drm_vma_manager.h>
 #include "psb_drv.h"
 
-int psb_gem_init_object(struct drm_gem_object *obj)
-{
-	return -EINVAL;
-}
-
 void psb_gem_free_object(struct drm_gem_object *obj)
 {
 	struct gtt_range *gtt = container_of(obj, struct gtt_range, gem);
diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
index d13c2fc..50a4c86 100644
--- a/drivers/gpu/drm/gma500/psb_drv.c
+++ b/drivers/gpu/drm/gma500/psb_drv.c
@@ -647,7 +647,6 @@ static struct drm_driver driver = {
 	.preclose = psb_driver_preclose,
 	.postclose = psb_driver_close,
 
-	.gem_init_object = psb_gem_init_object,
 	.gem_free_object = psb_gem_free_object,
 	.gem_vm_ops = &psb_gem_vm_ops,
 	.dumb_create = psb_gem_dumb_create,
diff --git a/drivers/gpu/drm/gma500/psb_drv.h b/drivers/gpu/drm/gma500/psb_drv.h
index 984cacf..2fa2407 100644
--- a/drivers/gpu/drm/gma500/psb_drv.h
+++ b/drivers/gpu/drm/gma500/psb_drv.h
@@ -832,7 +832,6 @@ extern const struct drm_connector_helper_funcs
 extern const struct drm_connector_funcs psb_intel_lvds_connector_funcs;
 
 /* gem.c */
-extern int psb_gem_init_object(struct drm_gem_object *obj);
 extern void psb_gem_free_object(struct drm_gem_object *obj);
 extern int psb_gem_get_aperture(struct drm_device *dev, void *data,
 			struct drm_file *file);
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 13457e3e..e443883 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1027,7 +1027,6 @@ static struct drm_driver driver = {
 	.debugfs_init = i915_debugfs_init,
 	.debugfs_cleanup = i915_debugfs_cleanup,
 #endif
-	.gem_init_object = i915_gem_init_object,
 	.gem_free_object = i915_gem_free_object,
 	.gem_vm_ops = &i915_gem_vm_ops,
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ab56820..261a684 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1738,7 +1738,6 @@ int i915_gem_wait_ioctl(struct drm_device *dev, void *data,
 void i915_gem_load(struct drm_device *dev);
 void *i915_gem_object_alloc(struct drm_device *dev);
 void i915_gem_object_free(struct drm_i915_gem_object *obj);
-int i915_gem_init_object(struct drm_gem_object *obj);
 void i915_gem_object_init(struct drm_i915_gem_object *obj,
 			 const struct drm_i915_gem_object_ops *ops);
 struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 26c5f80..46d8346 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3933,13 +3933,6 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
 	return obj;
 }
 
-int i915_gem_init_object(struct drm_gem_object *obj)
-{
-	BUG();
-
-	return 0;
-}
-
 void i915_gem_free_object(struct drm_gem_object *gem_obj)
 {
 	struct drm_i915_gem_object *obj = to_intel_bo(gem_obj);
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
index bd91964..1fcfd3e 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -100,7 +100,6 @@ static struct drm_driver driver = {
 	.minor = DRIVER_MINOR,
 	.patchlevel = DRIVER_PATCHLEVEL,
 
-	.gem_init_object = mgag200_gem_init_object,
 	.gem_free_object = mgag200_gem_free_object,
 	.dumb_create = mgag200_dumb_create,
 	.dumb_map_offset = mgag200_dumb_mmap_offset,
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index baaae19..cf11ee6 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -260,7 +260,6 @@ int mgag200_driver_unload(struct drm_device *dev);
 int mgag200_gem_create(struct drm_device *dev,
 		   u32 size, bool iskernel,
 		       struct drm_gem_object **obj);
-int mgag200_gem_init_object(struct drm_gem_object *obj);
 int mgag200_dumb_create(struct drm_file *file,
 			struct drm_device *dev,
 			struct drm_mode_create_dumb *args);
diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
index 0f8b861..b1120cb 100644
--- a/drivers/gpu/drm/mgag200/mgag200_main.c
+++ b/drivers/gpu/drm/mgag200/mgag200_main.c
@@ -310,12 +310,6 @@ int mgag200_dumb_create(struct drm_file *file,
 	return 0;
 }
 
-int mgag200_gem_init_object(struct drm_gem_object *obj)
-{
-	BUG();
-	return 0;
-}
-
 void mgag200_bo_unref(struct mgag200_bo **bo)
 {
 	struct ttm_buffer_object *tbo;
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 1faa75f..d6fa983 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -718,7 +718,6 @@ driver = {
 	.gem_prime_vmap = nouveau_gem_prime_vmap,
 	.gem_prime_vunmap = nouveau_gem_prime_vunmap,
 
-	.gem_init_object = nouveau_gem_object_new,
 	.gem_free_object = nouveau_gem_object_del,
 	.gem_open_object = nouveau_gem_object_open,
 	.gem_close_object = nouveau_gem_object_close,
diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
index 39278e9..cc43230 100644
--- a/drivers/gpu/drm/nouveau/nouveau_gem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
@@ -34,12 +34,6 @@
 #include "nouveau_ttm.h"
 #include "nouveau_gem.h"
 
-int
-nouveau_gem_object_new(struct drm_gem_object *gem)
-{
-	return 0;
-}
-
 void
 nouveau_gem_object_del(struct drm_gem_object *gem)
 {
diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.h b/drivers/gpu/drm/nouveau/nouveau_gem.h
index b535895..7caca05 100644
--- a/drivers/gpu/drm/nouveau/nouveau_gem.h
+++ b/drivers/gpu/drm/nouveau/nouveau_gem.h
@@ -19,7 +19,6 @@ nouveau_gem_object(struct drm_gem_object *gem)
 extern int nouveau_gem_new(struct drm_device *, int size, int align,
 			   uint32_t domain, uint32_t tile_mode,
 			   uint32_t tile_flags, struct nouveau_bo **);
-extern int nouveau_gem_object_new(struct drm_gem_object *);
 extern void nouveau_gem_object_del(struct drm_gem_object *);
 extern int nouveau_gem_object_open(struct drm_gem_object *, struct drm_file *);
 extern void nouveau_gem_object_close(struct drm_gem_object *,
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 2f9e22e..00e0e27 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -628,7 +628,6 @@ static struct drm_driver omap_drm_driver = {
 		.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
 		.gem_prime_export = omap_gem_prime_export,
 		.gem_prime_import = omap_gem_prime_import,
-		.gem_init_object = omap_gem_init_object,
 		.gem_free_object = omap_gem_free_object,
 		.gem_vm_ops = &omap_gem_vm_ops,
 		.dumb_create = omap_gem_dumb_create,
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index f2ba425..77b4fc2 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -221,7 +221,6 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev,
 int omap_gem_new_handle(struct drm_device *dev, struct drm_file *file,
 		union omap_gem_size gsize, uint32_t flags, uint32_t *handle);
 void omap_gem_free_object(struct drm_gem_object *obj);
-int omap_gem_init_object(struct drm_gem_object *obj);
 void *omap_gem_vaddr(struct drm_gem_object *obj);
 int omap_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
 		uint32_t handle, uint64_t *offset);
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index b1f1970..e640af6 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -1274,11 +1274,6 @@ unlock:
 	return ret;
 }
 
-int omap_gem_init_object(struct drm_gem_object *obj)
-{
-	return -EINVAL;          /* unused */
-}
-
 /* don't call directly.. called from GEM core when it is time to actually
  * free the object..
  */
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
index 48f2dfd..4759f23 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.c
+++ b/drivers/gpu/drm/qxl/qxl_drv.c
@@ -226,7 +226,6 @@ static struct drm_driver qxl_driver = {
 	.debugfs_init = qxl_debugfs_init,
 	.debugfs_cleanup = qxl_debugfs_takedown,
 #endif
-	.gem_init_object = qxl_gem_object_init,
 	.gem_free_object = qxl_gem_object_free,
 	.gem_open_object = qxl_gem_object_open,
 	.gem_close_object = qxl_gem_object_close,
diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 77bfed4..7591400 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -403,7 +403,6 @@ int qxl_gem_object_create_with_handle(struct qxl_device *qdev,
 				      struct qxl_surface *surf,
 				      struct qxl_bo **qobj,
 				      uint32_t *handle);
-int qxl_gem_object_init(struct drm_gem_object *obj);
 void qxl_gem_object_free(struct drm_gem_object *gobj);
 int qxl_gem_object_open(struct drm_gem_object *obj, struct drm_file *file_priv);
 void qxl_gem_object_close(struct drm_gem_object *obj,
diff --git a/drivers/gpu/drm/qxl/qxl_gem.c b/drivers/gpu/drm/qxl/qxl_gem.c
index 31fd4c3..e6bb9af 100644
--- a/drivers/gpu/drm/qxl/qxl_gem.c
+++ b/drivers/gpu/drm/qxl/qxl_gem.c
@@ -28,12 +28,6 @@
 #include "qxl_drv.h"
 #include "qxl_object.h"
 
-int qxl_gem_object_init(struct drm_gem_object *obj)
-{
-	/* we do nothings here */
-	return 0;
-}
-
 void qxl_gem_object_free(struct drm_gem_object *gobj)
 {
 	struct qxl_bo *qobj = gem_to_qxl_bo(gobj);
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
index fa7a7e1..76d6749 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -101,7 +101,6 @@ void radeon_driver_irq_preinstall_kms(struct drm_device *dev);
 int radeon_driver_irq_postinstall_kms(struct drm_device *dev);
 void radeon_driver_irq_uninstall_kms(struct drm_device *dev);
 irqreturn_t radeon_driver_irq_handler_kms(DRM_IRQ_ARGS);
-int radeon_gem_object_init(struct drm_gem_object *obj);
 void radeon_gem_object_free(struct drm_gem_object *obj);
 int radeon_gem_object_open(struct drm_gem_object *obj,
 				struct drm_file *file_priv);
@@ -412,7 +411,6 @@ static struct drm_driver kms_driver = {
 	.irq_uninstall = radeon_driver_irq_uninstall_kms,
 	.irq_handler = radeon_driver_irq_handler_kms,
 	.ioctls = radeon_ioctls_kms,
-	.gem_init_object = radeon_gem_object_init,
 	.gem_free_object = radeon_gem_object_free,
 	.gem_open_object = radeon_gem_object_open,
 	.gem_close_object = radeon_gem_object_close,
diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c
index dce99c8..805c5e5 100644
--- a/drivers/gpu/drm/radeon/radeon_gem.c
+++ b/drivers/gpu/drm/radeon/radeon_gem.c
@@ -29,13 +29,6 @@
 #include <drm/radeon_drm.h>
 #include "radeon.h"
 
-int radeon_gem_object_init(struct drm_gem_object *obj)
-{
-	BUG();
-
-	return 0;
-}
-
 void radeon_gem_object_free(struct drm_gem_object *gobj)
 {
 	struct radeon_bo *robj = gem_to_radeon_bo(gobj);
diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index bb0af58..5529c71 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -78,7 +78,6 @@ static struct drm_driver driver = {
 	.unload = udl_driver_unload,
 
 	/* gem hooks */
-	.gem_init_object = udl_gem_init_object,
 	.gem_free_object = udl_gem_free_object,
 	.gem_vm_ops = &udl_gem_vm_ops,
 
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 56aec94..1fbf7b3 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -115,7 +115,6 @@ int udl_dumb_create(struct drm_file *file_priv,
 int udl_gem_mmap(struct drm_file *file_priv, struct drm_device *dev,
 		 uint32_t handle, uint64_t *offset);
 
-int udl_gem_init_object(struct drm_gem_object *obj);
 void udl_gem_free_object(struct drm_gem_object *gem_obj);
 struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
 					    size_t size);
diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
index b5e3b80..d0293de 100644
--- a/drivers/gpu/drm/udl/udl_gem.c
+++ b/drivers/gpu/drm/udl/udl_gem.c
@@ -108,13 +108,6 @@ int udl_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 	}
 }
 
-int udl_gem_init_object(struct drm_gem_object *obj)
-{
-	BUG();
-
-	return 0;
-}
-
 static int udl_gem_get_pages(struct udl_gem_object *obj, gfp_t gfpmask)
 {
 	int page_count, i;
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 3ecdde6..3a6b3bd 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -673,8 +673,6 @@ struct drm_gem_object {
 	uint32_t pending_read_domains;
 	uint32_t pending_write_domain;
 
-	void *driver_private;
-
 	/* dma buf exported from this GEM object */
 	struct dma_buf *export_dma_buf;
 
@@ -908,7 +906,6 @@ struct drm_driver {
 	 *
 	 * Returns 0 on success.
 	 */
-	int (*gem_init_object) (struct drm_gem_object *obj);
 	void (*gem_free_object) (struct drm_gem_object *obj);
 	int (*gem_open_object) (struct drm_gem_object *, struct drm_file *);
 	void (*gem_close_object) (struct drm_gem_object *, struct drm_file *);
@@ -1556,8 +1553,6 @@ int drm_gem_init(struct drm_device *dev);
 void drm_gem_destroy(struct drm_device *dev);
 void drm_gem_object_release(struct drm_gem_object *obj);
 void drm_gem_object_free(struct kref *kref);
-struct drm_gem_object *drm_gem_object_alloc(struct drm_device *dev,
-					    size_t size);
 int drm_gem_object_init(struct drm_device *dev,
 			struct drm_gem_object *obj, size_t size);
 void drm_gem_private_object_init(struct drm_device *dev,
-- 
1.8.3.4

Re: [PATCH 5/7] drm/radeon: remove stale gem->driver_private access

[Alex Deucher]

On Wed, Aug 14, 2013 at 9:07 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> This field is never read. No need to set it in radeon. Besides, DRM gem
> core clears it during setup, anyway.
>
> Cc: Dave Airlie <airlied@gmail.com>
> Cc: Alex Deucher <alexdeucher@gmail.com>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/radeon/radeon_object.c | 1 -
>  drivers/gpu/drm/radeon/radeon_prime.c  | 1 -
>  2 files changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c
> index 2020bf4..c0fa4aa 100644
> --- a/drivers/gpu/drm/radeon/radeon_object.c
> +++ b/drivers/gpu/drm/radeon/radeon_object.c
> @@ -142,7 +142,6 @@ int radeon_bo_create(struct radeon_device *rdev,
>                 return r;
>         }
>         bo->rdev = rdev;
> -       bo->gem_base.driver_private = NULL;
>         bo->surface_reg = -1;
>         INIT_LIST_HEAD(&bo->list);
>         INIT_LIST_HEAD(&bo->va);
> diff --git a/drivers/gpu/drm/radeon/radeon_prime.c b/drivers/gpu/drm/radeon/radeon_prime.c
> index 65b9eab..2007456 100644
> --- a/drivers/gpu/drm/radeon/radeon_prime.c
> +++ b/drivers/gpu/drm/radeon/radeon_prime.c
> @@ -68,7 +68,6 @@ struct drm_gem_object *radeon_gem_prime_import_sg_table(struct drm_device *dev,
>                                RADEON_GEM_DOMAIN_GTT, sg, &bo);
>         if (ret)
>                 return ERR_PTR(ret);
> -       bo->gem_base.driver_private = bo;
>
>         mutex_lock(&rdev->gem.mutex);
>         list_add_tail(&bo->list, &rdev->gem.objects);
> --
> 1.8.3.4
>

Re: [PATCH 0/7] DRM: Remove gem_init_object() and friends

[Daniel Vetter]

On Wed, Aug 14, 2013 at 03:07:13PM +0200, David Herrmann wrote:
> Hi
> 
> This series removes any access to gem_obj->driver_private from all drivers and
> then drops drm_gem_object_alloc() with its ->gem_init_object() driver callback.
> 
> All this is only needed if a gem object is not embedded into the driver's bo.
> However, all drivers (except for nouveau, see patch #6) embed the objects. No
> need to allow dynamic allocations, anymore.
> 
> Patches #1 to #5 are trivial and only remove dead code. Patch #6 converts
> nouveau to embed gem objects into nouveau_bo. I tested this on my nv50 and it
> runs fine. Patch #7 drops gem_init_object() from all drivers as it is unused and
> the callbacks are all empty (or BUG()).
> 
> And the diff-stat looks pretty nice, too.
> 
> Cheers
> David
> 
> David Herrmann (7):
>   drm/ast: remove unused driver_private access
>   drm/mgag200: remove unused driver_private access
>   drm/cirrus: remove unused driver_private access
>   drm/qxl: remove unused object_pin/unpin() helpers
>   drm/radeon: remove stale gem->driver_private access
>   drm/nouveau: embed gem object in nouveau_bo
>   drm: kill ->gem_init_object() and friends

On the series:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> 
>  drivers/gpu/drm/ast/ast_drv.c             |  1 -
>  drivers/gpu/drm/ast/ast_drv.h             |  1 -
>  drivers/gpu/drm/ast/ast_main.c            |  6 -----
>  drivers/gpu/drm/ast/ast_ttm.c             |  1 -
>  drivers/gpu/drm/cirrus/cirrus_drv.c       |  1 -
>  drivers/gpu/drm/cirrus/cirrus_drv.h       |  1 -
>  drivers/gpu/drm/cirrus/cirrus_main.c      |  6 -----
>  drivers/gpu/drm/cirrus/cirrus_ttm.c       |  1 -
>  drivers/gpu/drm/drm_gem.c                 | 29 ---------------------
>  drivers/gpu/drm/exynos/exynos_drm_drv.c   |  1 -
>  drivers/gpu/drm/exynos/exynos_drm_gem.c   |  5 ----
>  drivers/gpu/drm/exynos/exynos_drm_gem.h   |  3 ---
>  drivers/gpu/drm/gma500/gem.c              |  5 ----
>  drivers/gpu/drm/gma500/psb_drv.c          |  1 -
>  drivers/gpu/drm/gma500/psb_drv.h          |  1 -
>  drivers/gpu/drm/i915/i915_drv.c           |  1 -
>  drivers/gpu/drm/i915/i915_drv.h           |  1 -
>  drivers/gpu/drm/i915/i915_gem.c           |  7 ------
>  drivers/gpu/drm/mgag200/mgag200_drv.c     |  1 -
>  drivers/gpu/drm/mgag200/mgag200_drv.h     |  1 -
>  drivers/gpu/drm/mgag200/mgag200_main.c    |  6 -----
>  drivers/gpu/drm/mgag200/mgag200_ttm.c     |  1 -
>  drivers/gpu/drm/nouveau/nouveau_abi16.c   |  4 +--
>  drivers/gpu/drm/nouveau/nouveau_bo.c      |  2 +-
>  drivers/gpu/drm/nouveau/nouveau_bo.h      |  5 +++-
>  drivers/gpu/drm/nouveau/nouveau_display.c | 10 ++++----
>  drivers/gpu/drm/nouveau/nouveau_drm.c     |  1 -
>  drivers/gpu/drm/nouveau/nouveau_fbcon.c   |  2 +-
>  drivers/gpu/drm/nouveau/nouveau_gem.c     | 42 +++++++++++++------------------
>  drivers/gpu/drm/nouveau/nouveau_gem.h     |  3 +--
>  drivers/gpu/drm/nouveau/nouveau_prime.c   | 10 +++++---
>  drivers/gpu/drm/omapdrm/omap_drv.c        |  1 -
>  drivers/gpu/drm/omapdrm/omap_drv.h        |  1 -
>  drivers/gpu/drm/omapdrm/omap_gem.c        |  5 ----
>  drivers/gpu/drm/qxl/qxl_drv.c             |  1 -
>  drivers/gpu/drm/qxl/qxl_drv.h             |  4 ---
>  drivers/gpu/drm/qxl/qxl_gem.c             | 32 -----------------------
>  drivers/gpu/drm/qxl/qxl_object.c          |  1 -
>  drivers/gpu/drm/radeon/radeon_drv.c       |  2 --
>  drivers/gpu/drm/radeon/radeon_gem.c       |  7 ------
>  drivers/gpu/drm/radeon/radeon_object.c    |  1 -
>  drivers/gpu/drm/radeon/radeon_prime.c     |  1 -
>  drivers/gpu/drm/udl/udl_drv.c             |  1 -
>  drivers/gpu/drm/udl/udl_drv.h             |  1 -
>  drivers/gpu/drm/udl/udl_gem.c             |  7 ------
>  include/drm/drmP.h                        |  5 ----
>  46 files changed, 37 insertions(+), 193 deletions(-)
> 
> -- 
> 1.8.3.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 6/7] drm/nouveau: embed gem object in nouveau_bo

[Maarten Lankhorst]

Op 14-08-13 15:07, David Herrmann schreef:
> There is no reason to keep the gem object separately allocated. nouveau is
> the last user of gem_obj->driver_private, so if we embed it, we can get
> rid of 8bytes per gem-object.
>
> The implementation follows the radeon driver. bo->gem is only valid, iff
> the bo was created via the gem helpers _and_ iff the user holds a valid
> gem reference. That is, as the gem object holds a reference to the
> nouveau_bo. If you use nouveau_ref() to gain a bo reference, you are not
> guaranteed to also hold a gem reference. The gem object might get
> destroyed after the last user drops the gem-ref via
> drm_gem_object_unreference(). Use drm_gem_object_reference() to gain a
> gem-reference.
>
> For debugging, we can use bo->gem.filp != NULL to test whether a gem-bo is
> valid. However, this shouldn't be used for real functionality to avoid
> gem-internal dependencies.
>
> Note that the implementation follows the previous style. However, we no
> longer can check for bo->gem != NULL to test for a valid gem object. This
> wasn't done before, so we should be safe now.
I'm all for getting rid of it, but I think it's not thorough enough. :P

There's still a separate refcount in ttm for the bo, and I think it doesn't make sense to keep it like that.
Instead of having that refcount in ttm, could it be put entirely in gem?

ttm_buffer_object_transfer is the only time ttm creates a bo, and it's immediately destroyed, so instead
of calling ttm_bo_unref it could call ttm_bo_release directly, in which case it doesn't matter that refcount
is managed by gem.

Oh except for vmwgfx of course, I always forget that 'special' case.

Maybe something like this in memory?

struct {
  struct ttm_buffer_object bo;
  struct drm_gem_object gem; // Can be anything, as long as first member is a refcount, and no hole between ttm and this..
};

This would allow ttm to do kref_put((kref_t*)(bo +1), driver->releasefn), where driver->releasefn has to call ttm_bo_release again.
Unfortunately unless drm is fixed dma-buf is still going to be as buggy as before, not much I can do about that. :/
But this is something for a future where dma-buf in drm doesn't suck..

~Maarten

Re: [PATCH 6/7] drm/nouveau: embed gem object in nouveau_bo

[David Herrmann]

Hi

On Wed, Aug 14, 2013 at 4:31 PM, Maarten Lankhorst
<maarten.lankhorst@canonical.com> wrote:
> Op 14-08-13 15:07, David Herrmann schreef:
>> There is no reason to keep the gem object separately allocated. nouveau is
>> the last user of gem_obj->driver_private, so if we embed it, we can get
>> rid of 8bytes per gem-object.
>>
>> The implementation follows the radeon driver. bo->gem is only valid, iff
>> the bo was created via the gem helpers _and_ iff the user holds a valid
>> gem reference. That is, as the gem object holds a reference to the
>> nouveau_bo. If you use nouveau_ref() to gain a bo reference, you are not
>> guaranteed to also hold a gem reference. The gem object might get
>> destroyed after the last user drops the gem-ref via
>> drm_gem_object_unreference(). Use drm_gem_object_reference() to gain a
>> gem-reference.
>>
>> For debugging, we can use bo->gem.filp != NULL to test whether a gem-bo is
>> valid. However, this shouldn't be used for real functionality to avoid
>> gem-internal dependencies.
>>
>> Note that the implementation follows the previous style. However, we no
>> longer can check for bo->gem != NULL to test for a valid gem object. This
>> wasn't done before, so we should be safe now.
> I'm all for getting rid of it, but I think it's not thorough enough. :P

Does that mean you are in favor of this patch? Or do you want me to
rework it right now for v2? Because I'd like to see this cleanup
applied first so all TTM drivers work the same way. On top of that we
can then do whatever we want regarding TTM refcounts. I am willing to
spend some time on this.

> There's still a separate refcount in ttm for the bo, and I think it doesn't make sense to keep it like that.
> Instead of having that refcount in ttm, could it be put entirely in gem?
>
> ttm_buffer_object_transfer is the only time ttm creates a bo, and it's immediately destroyed, so instead
> of calling ttm_bo_unref it could call ttm_bo_release directly, in which case it doesn't matter that refcount
> is managed by gem.

The bo_vm code also takes bo references.

> Oh except for vmwgfx of course, I always forget that 'special' case.
>
> Maybe something like this in memory?
>
> struct {
>   struct ttm_buffer_object bo;
>   struct drm_gem_object gem; // Can be anything, as long as first member is a refcount, and no hole between ttm and this..
> };

Ugh.. This won't particularly win any beauty contest..

> This would allow ttm to do kref_put((kref_t*)(bo +1), driver->releasefn), where driver->releasefn has to call ttm_bo_release again.
> Unfortunately unless drm is fixed dma-buf is still going to be as buggy as before, not much I can do about that. :/
> But this is something for a future where dma-buf in drm doesn't suck..

This same issue came up with the vma-access-management patches. I
dislike adding hacks to TTM to allow accessing the surrounding GEM
object only to keep TTM gem-agnostic. We could fix all this with a
simple gem_object* pointer in the ttm-bo (or a flag indicating
container_of is valid).

Or we make gem TTM-aware by optionally embedding either a ttm_bo or a
kref object. Hm. Ideas welcome!

Regards
David

Re: [PATCH 6/7] drm/nouveau: embed gem object in nouveau_bo

[Daniel Vetter]

On Thu, Aug 15, 2013 at 2:32 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
>> This would allow ttm to do kref_put((kref_t*)(bo +1), driver->releasefn), where driver->releasefn has to call ttm_bo_release again.
>> Unfortunately unless drm is fixed dma-buf is still going to be as buggy as before, not much I can do about that. :/
>> But this is something for a future where dma-buf in drm doesn't suck..
>
> This same issue came up with the vma-access-management patches. I
> dislike adding hacks to TTM to allow accessing the surrounding GEM
> object only to keep TTM gem-agnostic. We could fix all this with a
> simple gem_object* pointer in the ttm-bo (or a flag indicating
> container_of is valid).
>
> Or we make gem TTM-aware by optionally embedding either a ttm_bo or a
> kref object. Hm. Ideas welcome!

Imo embedding a gem bo into a ttm bo should be too harmful. I think we
already have funny lifetime issues around
ttm_bo->presistent_swap_storage, so this would imo be a win. Four
downsides afaik:
- gem shows up in drm/ttm/ ;-)
- i915 gem crap like the read/write domains get splattered even more
around. We could just move them to the i915 gem object struct and call
it a day I think.
- vmwgfx doesn't expose a gem interface. Imo this doesn't apply any
more since we have support for driver private gem objects since a
while. So shouldn't be too hard to do the final untangling and allow
non-gem drivers to init gem objects, but not link them up anywhere.
- gem_object_unref uses dev->struct_mutex. The last reason for that is
the vma manager (which can be fixed to use the same
kref_get_unless_zero trick ttm uses) and then the locking pushed down
into drivers (probably needs a deferred free list to get going).

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 6/7] drm/nouveau: embed gem object in nouveau_bo

[Maarten Lankhorst]

Op 15-08-13 14:44, Daniel Vetter schreef:
> On Thu, Aug 15, 2013 at 2:32 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
>>> This would allow ttm to do kref_put((kref_t*)(bo +1), driver->releasefn), where driver->releasefn has to call ttm_bo_release again.
>>> Unfortunately unless drm is fixed dma-buf is still going to be as buggy as before, not much I can do about that. :/
>>> But this is something for a future where dma-buf in drm doesn't suck..
>> This same issue came up with the vma-access-management patches. I
>> dislike adding hacks to TTM to allow accessing the surrounding GEM
>> object only to keep TTM gem-agnostic. We could fix all this with a
>> simple gem_object* pointer in the ttm-bo (or a flag indicating
>> container_of is valid).
>>
>> Or we make gem TTM-aware by optionally embedding either a ttm_bo or a
>> kref object. Hm. Ideas welcome!
Oh original patch:

Acked-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>

I think with this series the normal ttm refcount could be removed entirely. The only place in ttm that
uses it is ttm_bo_mmap and ttm_bo_init. If struct vm_operations_struct is set by the driver the refcount
isn't required in ttm any more at all. The other places in core ttm that use refcount now can be cleaned up.

Radeon was already overriding the mmap ops for ttm, so this could now be done in a cleaner way. :-)

That leaves vmwgfx of course, I'll probably kill ttm_bo_create because that will no longer work without refcount,
and fixup their code.

ttm_bo_io and ttm_bo_fbdev_io can be removed entirely, nothing uses it (I'll send a patch for it).
> Imo embedding a gem bo into a ttm bo should be too harmful. I think we
> already have funny lifetime issues around
> ttm_bo->presistent_swap_storage, so this would imo be a win. Four
> downsides afaik:
> - gem shows up in drm/ttm/ ;-)
> - i915 gem crap like the read/write domains get splattered even more
> around. We could just move them to the i915 gem object struct and call
> it a day I think.
> - vmwgfx doesn't expose a gem interface. Imo this doesn't apply any
> more since we have support for driver private gem objects since a
> while. So shouldn't be too hard to do the final untangling and allow
> non-gem drivers to init gem objects, but not link them up anywhere.
> - gem_object_unref uses dev->struct_mutex. The last reason for that is
> the vma manager (which can be fixed to use the same
> kref_get_unless_zero trick ttm uses) and then the locking pushed down
> into drivers (probably needs a deferred free list to get going).
It's not required to move ttm to gem to kill the refcount. :-)

~Maarten

Re: [PATCH 6/7] drm/nouveau: embed gem object in nouveau_bo

[Ben Skeggs]

On Wed, Aug 14, 2013 at 11:07 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> There is no reason to keep the gem object separately allocated. nouveau is
> the last user of gem_obj->driver_private, so if we embed it, we can get
> rid of 8bytes per gem-object.
>
> The implementation follows the radeon driver. bo->gem is only valid, iff
> the bo was created via the gem helpers _and_ iff the user holds a valid
> gem reference. That is, as the gem object holds a reference to the
> nouveau_bo. If you use nouveau_ref() to gain a bo reference, you are not
> guaranteed to also hold a gem reference. The gem object might get
> destroyed after the last user drops the gem-ref via
> drm_gem_object_unreference(). Use drm_gem_object_reference() to gain a
> gem-reference.
>
> For debugging, we can use bo->gem.filp != NULL to test whether a gem-bo is
> valid. However, this shouldn't be used for real functionality to avoid
> gem-internal dependencies.
>
> Note that the implementation follows the previous style. However, we no
> longer can check for bo->gem != NULL to test for a valid gem object. This
> wasn't done before, so we should be safe now.
>
> Cc: Ben Skeggs <skeggsb@gmail.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@canonical.com>
> Cc: Martin Peres <martin.peres@labri.fr>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Reviewed-by: Ben Skeggs <bskeggs@redhat.com>

Dave, are you taking this through your tree?

Thanks David,
Ben.

> ---
>  drivers/gpu/drm/nouveau/nouveau_abi16.c   |  4 ++--
>  drivers/gpu/drm/nouveau/nouveau_bo.c      |  2 +-
>  drivers/gpu/drm/nouveau/nouveau_bo.h      |  5 ++++-
>  drivers/gpu/drm/nouveau/nouveau_display.c | 10 ++++-----
>  drivers/gpu/drm/nouveau/nouveau_fbcon.c   |  2 +-
>  drivers/gpu/drm/nouveau/nouveau_gem.c     | 36 +++++++++++++++----------------
>  drivers/gpu/drm/nouveau/nouveau_gem.h     |  2 +-
>  drivers/gpu/drm/nouveau/nouveau_prime.c   | 10 +++++----
>  8 files changed, 37 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_abi16.c b/drivers/gpu/drm/nouveau/nouveau_abi16.c
> index 8f467e7..3897549 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_abi16.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_abi16.c
> @@ -130,7 +130,7 @@ nouveau_abi16_chan_fini(struct nouveau_abi16 *abi16,
>         if (chan->ntfy) {
>                 nouveau_bo_vma_del(chan->ntfy, &chan->ntfy_vma);
>                 nouveau_bo_unpin(chan->ntfy);
> -               drm_gem_object_unreference_unlocked(chan->ntfy->gem);
> +               drm_gem_object_unreference_unlocked(&chan->ntfy->gem);
>         }
>
>         if (chan->heap.block_size)
> @@ -320,7 +320,7 @@ nouveau_abi16_ioctl_channel_alloc(ABI16_IOCTL_ARGS)
>                         goto done;
>         }
>
> -       ret = drm_gem_handle_create(file_priv, chan->ntfy->gem,
> +       ret = drm_gem_handle_create(file_priv, &chan->ntfy->gem,
>                                     &init->notifier_handle);
>         if (ret)
>                 goto done;
> diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
> index 4e7ee5f..7767a89 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_bo.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
> @@ -146,7 +146,7 @@ nouveau_bo_del_ttm(struct ttm_buffer_object *bo)
>         struct drm_device *dev = drm->dev;
>         struct nouveau_bo *nvbo = nouveau_bo(bo);
>
> -       if (unlikely(nvbo->gem))
> +       if (unlikely(nvbo->gem.filp))
>                 DRM_ERROR("bo %p still attached to GEM object\n", bo);
>         WARN_ON(nvbo->pin_refcnt > 0);
>         nv10_bo_put_tile_region(dev, nvbo->tile, NULL);
> diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.h b/drivers/gpu/drm/nouveau/nouveau_bo.h
> index 653dbbb..ff17c1f 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_bo.h
> +++ b/drivers/gpu/drm/nouveau/nouveau_bo.h
> @@ -27,7 +27,10 @@ struct nouveau_bo {
>         u32 tile_flags;
>         struct nouveau_drm_tile *tile;
>
> -       struct drm_gem_object *gem;
> +       /* Only valid if allocated via nouveau_gem_new() and iff you hold a
> +        * gem reference to it! For debugging, use gem.filp != NULL to test
> +        * whether it is valid. */
> +       struct drm_gem_object gem;
>
>         /* protect by the ttm reservation lock */
>         int pin_refcnt;
> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
> index 78637af..85571ed 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_display.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
> @@ -50,7 +50,7 @@ nouveau_user_framebuffer_destroy(struct drm_framebuffer *drm_fb)
>         struct nouveau_framebuffer *fb = nouveau_framebuffer(drm_fb);
>
>         if (fb->nvbo)
> -               drm_gem_object_unreference_unlocked(fb->nvbo->gem);
> +               drm_gem_object_unreference_unlocked(&fb->nvbo->gem);
>
>         drm_framebuffer_cleanup(drm_fb);
>         kfree(fb);
> @@ -63,7 +63,7 @@ nouveau_user_framebuffer_create_handle(struct drm_framebuffer *drm_fb,
>  {
>         struct nouveau_framebuffer *fb = nouveau_framebuffer(drm_fb);
>
> -       return drm_gem_handle_create(file_priv, fb->nvbo->gem, handle);
> +       return drm_gem_handle_create(file_priv, &fb->nvbo->gem, handle);
>  }
>
>  static const struct drm_framebuffer_funcs nouveau_framebuffer_funcs = {
> @@ -668,8 +668,8 @@ nouveau_display_dumb_create(struct drm_file *file_priv, struct drm_device *dev,
>         if (ret)
>                 return ret;
>
> -       ret = drm_gem_handle_create(file_priv, bo->gem, &args->handle);
> -       drm_gem_object_unreference_unlocked(bo->gem);
> +       ret = drm_gem_handle_create(file_priv, &bo->gem, &args->handle);
> +       drm_gem_object_unreference_unlocked(&bo->gem);
>         return ret;
>  }
>
> @@ -682,7 +682,7 @@ nouveau_display_dumb_map_offset(struct drm_file *file_priv,
>
>         gem = drm_gem_object_lookup(dev, file_priv, handle);
>         if (gem) {
> -               struct nouveau_bo *bo = gem->driver_private;
> +               struct nouveau_bo *bo = nouveau_gem_object(gem);
>                 *poffset = drm_vma_node_offset_addr(&bo->bo.vma_node);
>                 drm_gem_object_unreference_unlocked(gem);
>                 return 0;
> diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
> index 4c1bc06..e20b695 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
> @@ -419,7 +419,7 @@ nouveau_fbcon_destroy(struct drm_device *dev, struct nouveau_fbdev *fbcon)
>                 nouveau_bo_unmap(nouveau_fb->nvbo);
>                 nouveau_bo_vma_del(nouveau_fb->nvbo, &nouveau_fb->vma);
>                 nouveau_bo_unpin(nouveau_fb->nvbo);
> -               drm_gem_object_unreference_unlocked(nouveau_fb->nvbo->gem);
> +               drm_gem_object_unreference_unlocked(&nouveau_fb->nvbo->gem);
>                 nouveau_fb->nvbo = NULL;
>         }
>         drm_fb_helper_fini(&fbcon->helper);
> diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
> index 487242f..39278e9 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> @@ -43,20 +43,17 @@ nouveau_gem_object_new(struct drm_gem_object *gem)
>  void
>  nouveau_gem_object_del(struct drm_gem_object *gem)
>  {
> -       struct nouveau_bo *nvbo = gem->driver_private;
> +       struct nouveau_bo *nvbo = nouveau_gem_object(gem);
>         struct ttm_buffer_object *bo = &nvbo->bo;
>
> -       if (!nvbo)
> -               return;
> -       nvbo->gem = NULL;
> -
>         if (gem->import_attach)
>                 drm_prime_gem_destroy(gem, nvbo->bo.sg);
>
> -       ttm_bo_unref(&bo);
> -
>         drm_gem_object_release(gem);
> -       kfree(gem);
> +
> +       /* reset filp so nouveau_bo_del_ttm() can test for it */
> +       gem->filp = NULL;
> +       ttm_bo_unref(&bo);
>  }
>
>  int
> @@ -186,14 +183,15 @@ nouveau_gem_new(struct drm_device *dev, int size, int align, uint32_t domain,
>         if (nv_device(drm->device)->card_type >= NV_50)
>                 nvbo->valid_domains &= domain;
>
> -       nvbo->gem = drm_gem_object_alloc(dev, nvbo->bo.mem.size);
> -       if (!nvbo->gem) {
> +       /* Initialize the embedded gem-object. We return a single gem-reference
> +        * to the caller, instead of a normal nouveau_bo ttm reference. */
> +       ret = drm_gem_object_init(dev, &nvbo->gem, nvbo->bo.mem.size);
> +       if (ret) {
>                 nouveau_bo_ref(NULL, pnvbo);
>                 return -ENOMEM;
>         }
>
> -       nvbo->bo.persistent_swap_storage = nvbo->gem->filp;
> -       nvbo->gem->driver_private = nvbo;
> +       nvbo->bo.persistent_swap_storage = nvbo->gem.filp;
>         return 0;
>  }
>
> @@ -250,15 +248,15 @@ nouveau_gem_ioctl_new(struct drm_device *dev, void *data,
>         if (ret)
>                 return ret;
>
> -       ret = drm_gem_handle_create(file_priv, nvbo->gem, &req->info.handle);
> +       ret = drm_gem_handle_create(file_priv, &nvbo->gem, &req->info.handle);
>         if (ret == 0) {
> -               ret = nouveau_gem_info(file_priv, nvbo->gem, &req->info);
> +               ret = nouveau_gem_info(file_priv, &nvbo->gem, &req->info);
>                 if (ret)
>                         drm_gem_handle_delete(file_priv, req->info.handle);
>         }
>
>         /* drop reference from allocate - handle holds it now */
> -       drm_gem_object_unreference_unlocked(nvbo->gem);
> +       drm_gem_object_unreference_unlocked(&nvbo->gem);
>         return ret;
>  }
>
> @@ -266,7 +264,7 @@ static int
>  nouveau_gem_set_domain(struct drm_gem_object *gem, uint32_t read_domains,
>                        uint32_t write_domains, uint32_t valid_domains)
>  {
> -       struct nouveau_bo *nvbo = gem->driver_private;
> +       struct nouveau_bo *nvbo = nouveau_gem_object(gem);
>         struct ttm_buffer_object *bo = &nvbo->bo;
>         uint32_t domains = valid_domains & nvbo->valid_domains &
>                 (write_domains ? write_domains : read_domains);
> @@ -327,7 +325,7 @@ validate_fini_list(struct list_head *list, struct nouveau_fence *fence,
>                 list_del(&nvbo->entry);
>                 nvbo->reserved_by = NULL;
>                 ttm_bo_unreserve_ticket(&nvbo->bo, ticket);
> -               drm_gem_object_unreference_unlocked(nvbo->gem);
> +               drm_gem_object_unreference_unlocked(&nvbo->gem);
>         }
>  }
>
> @@ -376,7 +374,7 @@ retry:
>                         validate_fini(op, NULL);
>                         return -ENOENT;
>                 }
> -               nvbo = gem->driver_private;
> +               nvbo = nouveau_gem_object(gem);
>                 if (nvbo == res_bo) {
>                         res_bo = NULL;
>                         drm_gem_object_unreference_unlocked(gem);
> @@ -478,7 +476,7 @@ validate_list(struct nouveau_channel *chan, struct nouveau_cli *cli,
>                         return ret;
>                 }
>
> -               ret = nouveau_gem_set_domain(nvbo->gem, b->read_domains,
> +               ret = nouveau_gem_set_domain(&nvbo->gem, b->read_domains,
>                                              b->write_domains,
>                                              b->valid_domains);
>                 if (unlikely(ret)) {
> diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.h b/drivers/gpu/drm/nouveau/nouveau_gem.h
> index 502e429..b535895 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_gem.h
> +++ b/drivers/gpu/drm/nouveau/nouveau_gem.h
> @@ -12,7 +12,7 @@
>  static inline struct nouveau_bo *
>  nouveau_gem_object(struct drm_gem_object *gem)
>  {
> -       return gem ? gem->driver_private : NULL;
> +       return gem ? container_of(gem, struct nouveau_bo, gem) : NULL;
>  }
>
>  /* nouveau_gem.c */
> diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c
> index e90468d..51a2cb1 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_prime.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c
> @@ -71,14 +71,16 @@ struct drm_gem_object *nouveau_gem_prime_import_sg_table(struct drm_device *dev,
>                 return ERR_PTR(ret);
>
>         nvbo->valid_domains = NOUVEAU_GEM_DOMAIN_GART;
> -       nvbo->gem = drm_gem_object_alloc(dev, nvbo->bo.mem.size);
> -       if (!nvbo->gem) {
> +
> +       /* Initialize the embedded gem-object. We return a single gem-reference
> +        * to the caller, instead of a normal nouveau_bo ttm reference. */
> +       ret = drm_gem_object_init(dev, &nvbo->gem, nvbo->bo.mem.size);
> +       if (ret) {
>                 nouveau_bo_ref(NULL, &nvbo);
>                 return ERR_PTR(-ENOMEM);
>         }
>
> -       nvbo->gem->driver_private = nvbo;
> -       return nvbo->gem;
> +       return &nvbo->gem;
>  }
>
>  int nouveau_gem_prime_pin(struct drm_gem_object *obj)
> --
> 1.8.3.4
>