Re: [PATCH 1/5] drm/ttm: use a static ttm_mem_global instance

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Am 23.10.18 um 08:22 schrieb Thomas Zimmermann:
Hi Christian

Am 19.10.18 um 18:41 schrieb Christian König:
As the name says we only need one global instance of ttm_mem_global.

Drop all the driver initialization and just use a single exported
instance which is initialized during BO global initialization.

Signed-off-by: Christian König <christian.koenig@xxxxxxx>
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c         | 44 -------------------------
  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h         |  1 -
  drivers/gpu/drm/ast/ast_drv.h                   |  1 -
  drivers/gpu/drm/ast/ast_ttm.c                   | 32 ++----------------
  drivers/gpu/drm/bochs/bochs.h                   |  1 -
  drivers/gpu/drm/bochs/bochs_mm.c                | 30 ++---------------
  drivers/gpu/drm/cirrus/cirrus_drv.h             |  1 -
  drivers/gpu/drm/cirrus/cirrus_ttm.c             | 32 ++----------------
  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h |  1 -
  drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c     | 31 +++--------------
  drivers/gpu/drm/mgag200/mgag200_drv.h           |  1 -
  drivers/gpu/drm/mgag200/mgag200_ttm.c           | 32 ++----------------
  drivers/gpu/drm/nouveau/nouveau_drv.h           |  1 -
  drivers/gpu/drm/nouveau/nouveau_ttm.c           | 34 ++-----------------
  drivers/gpu/drm/qxl/qxl_drv.h                   |  1 -
  drivers/gpu/drm/qxl/qxl_ttm.c                   | 28 ----------------
  drivers/gpu/drm/radeon/radeon.h                 |  1 -
  drivers/gpu/drm/radeon/radeon_ttm.c             | 26 ---------------
  drivers/gpu/drm/ttm/ttm_bo.c                    | 10 ++++--
  drivers/gpu/drm/ttm/ttm_memory.c                |  5 +--
  drivers/gpu/drm/virtio/virtgpu_drv.h            |  1 -
  drivers/gpu/drm/virtio/virtgpu_ttm.c            | 27 ---------------
  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c             |  4 +--
  drivers/gpu/drm/vmwgfx/vmwgfx_drv.h             |  3 +-
  drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c        | 27 ---------------
  drivers/staging/vboxvideo/vbox_drv.h            |  1 -
  drivers/staging/vboxvideo/vbox_ttm.c            | 24 --------------
  include/drm/ttm/ttm_bo_driver.h                 |  8 ++---
  include/drm/ttm/ttm_memory.h                    |  4 +--
  29 files changed, 32 insertions(+), 380 deletions(-)
Great that you removed all the global TTM state from all the drivers.
This removes a lot of duplication and simplifies driver development a bit.


diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 9edece6510d3..3006050b1720 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -1526,18 +1526,22 @@ void ttm_bo_global_release(struct ttm_bo_global *glob)
  {
  	kobject_del(&glob->kobj);
  	kobject_put(&glob->kobj);
+	ttm_mem_global_release(&ttm_mem_glob);
  }
  EXPORT_SYMBOL(ttm_bo_global_release);
-int ttm_bo_global_init(struct ttm_bo_global *glob,
-		       struct ttm_mem_global *mem_glob)
+int ttm_bo_global_init(struct ttm_bo_global *glob)
  {
  	int ret;
  	unsigned i;
+ ret = ttm_mem_global_init(&ttm_mem_glob);
+	if (ret)
+		return ret;
+
What I really dislike about this patch set is that it mixes state and
implementation into that same functions. The original code had a fairly
good separation of both. Now the mechanisms and policies are located in
the same places.[^1]

This looks like a simplification, but from my experience, such code is a
setup for long-term maintenance problems. For example, I can imagine
that someone at some point wants multiple global buffers (e.g., on a
NUMA-like architecture).

That is exactly the thinking which messed up things in the first place. Keep in mind that this is the kernel and not a userspace C++ project.

Handling this as global state in a module is perfectly and we already support NUMA use cases with this. E.g. if we would have this state multiple times I would actually consider this a bug.

So pushing this into a structure makes it much more error prone and doesn't buy us anything.

In the long term I'm actually thinking about getting completely rid of the ttm_mem_global and ttm_bo_global and just use static variables for this.

Regards,
Christian.


I understand that I'm new here, have no say, and probably don't get the
big picture, but from my point of view, this is not a forward-thinking
change.

Best regards
Thomas

[^1] More philosophically speaking, program state can be global, but
data structures can only be share-able. ttm_mem_global and ttm_bo_global
should be renamed to something like ttm_shared_mem, rsp. ttm_shared_bo.


  	mutex_init(&glob->device_list_mutex);
  	spin_lock_init(&glob->lru_lock);
-	glob->mem_glob = mem_glob;
+	glob->mem_glob = &ttm_mem_glob;
  	glob->mem_glob->bo_glob = glob;
  	glob->dummy_read_page = alloc_page(__GFP_ZERO | GFP_DMA32);
diff --git a/drivers/gpu/drm/ttm/ttm_memory.c b/drivers/gpu/drm/ttm/ttm_memory.c
index 450387c92b63..7704e17c402f 100644
--- a/drivers/gpu/drm/ttm/ttm_memory.c
+++ b/drivers/gpu/drm/ttm/ttm_memory.c
@@ -41,6 +41,9 @@
#define TTM_MEMORY_ALLOC_RETRIES 4 +struct ttm_mem_global ttm_mem_glob;
+EXPORT_SYMBOL(ttm_mem_glob);
+
  struct ttm_mem_zone {
  	struct kobject kobj;
  	struct ttm_mem_global *glob;
@@ -464,7 +467,6 @@ int ttm_mem_global_init(struct ttm_mem_global *glob)
  	ttm_mem_global_release(glob);
  	return ret;
  }
-EXPORT_SYMBOL(ttm_mem_global_init);
void ttm_mem_global_release(struct ttm_mem_global *glob)
  {
@@ -486,7 +488,6 @@ void ttm_mem_global_release(struct ttm_mem_global *glob)
  	kobject_del(&glob->kobj);
  	kobject_put(&glob->kobj);
  }
-EXPORT_SYMBOL(ttm_mem_global_release);
static void ttm_check_swapping(struct ttm_mem_global *glob)
  {
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 65605e207bbe..dec42d421e00 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -133,7 +133,6 @@ struct virtio_gpu_framebuffer {
struct virtio_gpu_mman {
  	struct ttm_bo_global_ref        bo_global_ref;
-	struct drm_global_reference	mem_global_ref;
  	bool				mem_global_referenced;
  	struct ttm_bo_device		bdev;
  };
diff --git a/drivers/gpu/drm/virtio/virtgpu_ttm.c b/drivers/gpu/drm/virtio/virtgpu_ttm.c
index 0ec46b47b423..b99ecc6d97d3 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ttm.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ttm.c
@@ -50,37 +50,12 @@ virtio_gpu_device *virtio_gpu_get_vgdev(struct ttm_bo_device *bdev)
  	return vgdev;
  }
-static int virtio_gpu_ttm_mem_global_init(struct drm_global_reference *ref)
-{
-	return ttm_mem_global_init(ref->object);
-}
-
-static void virtio_gpu_ttm_mem_global_release(struct drm_global_reference *ref)
-{
-	ttm_mem_global_release(ref->object);
-}
-
  static int virtio_gpu_ttm_global_init(struct virtio_gpu_device *vgdev)
  {
  	struct drm_global_reference *global_ref;
  	int r;
vgdev->mman.mem_global_referenced = false;
-	global_ref = &vgdev->mman.mem_global_ref;
-	global_ref->global_type = DRM_GLOBAL_TTM_MEM;
-	global_ref->size = sizeof(struct ttm_mem_global);
-	global_ref->init = &virtio_gpu_ttm_mem_global_init;
-	global_ref->release = &virtio_gpu_ttm_mem_global_release;
-
-	r = drm_global_item_ref(global_ref);
-	if (r != 0) {
-		DRM_ERROR("Failed setting up TTM memory accounting "
-			  "subsystem.\n");
-		return r;
-	}
-
-	vgdev->mman.bo_global_ref.mem_glob =
-		vgdev->mman.mem_global_ref.object;
  	global_ref = &vgdev->mman.bo_global_ref.ref;
  	global_ref->global_type = DRM_GLOBAL_TTM_BO;
  	global_ref->size = sizeof(struct ttm_bo_global);
@@ -89,7 +64,6 @@ static int virtio_gpu_ttm_global_init(struct virtio_gpu_device *vgdev)
  	r = drm_global_item_ref(global_ref);
  	if (r != 0) {
  		DRM_ERROR("Failed setting up TTM BO subsystem.\n");
-		drm_global_item_unref(&vgdev->mman.mem_global_ref);
  		return r;
  	}
@@ -101,7 +75,6 @@ static void virtio_gpu_ttm_global_fini(struct virtio_gpu_device *vgdev)
  {
  	if (vgdev->mman.mem_global_referenced) {
  		drm_global_item_unref(&vgdev->mman.bo_global_ref.ref);
-		drm_global_item_unref(&vgdev->mman.mem_global_ref);
  		vgdev->mman.mem_global_referenced = false;
  	}
  }
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index bb6dbbe18835..57df776d987c 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -828,8 +828,8 @@ static int vmw_driver_load(struct drm_device *dev, unsigned long chipset)
  		goto out_err4;
  	}
- dev_priv->tdev = ttm_object_device_init
-		(dev_priv->mem_global_ref.object, 12, &vmw_prime_dmabuf_ops);
+	dev_priv->tdev = ttm_object_device_init(&ttm_mem_glob, 12,
+						&vmw_prime_dmabuf_ops);
if (unlikely(dev_priv->tdev == NULL)) {
  		DRM_ERROR("Unable to initialize TTM object management.\n");
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
index 1abe21758b0d..df15a745efc3 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
@@ -366,7 +366,6 @@ enum {
  struct vmw_private {
  	struct ttm_bo_device bdev;
  	struct ttm_bo_global_ref bo_global_ref;
-	struct drm_global_reference mem_global_ref;
struct vmw_fifo_state fifo; @@ -1288,7 +1287,7 @@ vmw_bo_reference(struct vmw_buffer_object *buf) static inline struct ttm_mem_global *vmw_mem_glob(struct vmw_private *dev_priv)
  {
-	return (struct ttm_mem_global *) dev_priv->mem_global_ref.object;
+	return &ttm_mem_glob;
  }
static inline void vmw_fifo_resource_inc(struct vmw_private *dev_priv)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
index f3ce43c41978..0ac473cd5136 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
@@ -43,36 +43,11 @@ int vmw_mmap(struct file *filp, struct vm_area_struct *vma)
  	return ttm_bo_mmap(filp, vma, &dev_priv->bdev);
  }
-static int vmw_ttm_mem_global_init(struct drm_global_reference *ref)
-{
-	DRM_INFO("global init.\n");
-	return ttm_mem_global_init(ref->object);
-}
-
-static void vmw_ttm_mem_global_release(struct drm_global_reference *ref)
-{
-	ttm_mem_global_release(ref->object);
-}
-
  int vmw_ttm_global_init(struct vmw_private *dev_priv)
  {
  	struct drm_global_reference *global_ref;
  	int ret;
- global_ref = &dev_priv->mem_global_ref;
-	global_ref->global_type = DRM_GLOBAL_TTM_MEM;
-	global_ref->size = sizeof(struct ttm_mem_global);
-	global_ref->init = &vmw_ttm_mem_global_init;
-	global_ref->release = &vmw_ttm_mem_global_release;
-
-	ret = drm_global_item_ref(global_ref);
-	if (unlikely(ret != 0)) {
-		DRM_ERROR("Failed setting up TTM memory accounting.\n");
-		return ret;
-	}
-
-	dev_priv->bo_global_ref.mem_glob =
-		dev_priv->mem_global_ref.object;
  	global_ref = &dev_priv->bo_global_ref.ref;
  	global_ref->global_type = DRM_GLOBAL_TTM_BO;
  	global_ref->size = sizeof(struct ttm_bo_global);
@@ -87,12 +62,10 @@ int vmw_ttm_global_init(struct vmw_private *dev_priv)
return 0;
  out_no_bo:
-	drm_global_item_unref(&dev_priv->mem_global_ref);
  	return ret;
  }
void vmw_ttm_global_release(struct vmw_private *dev_priv)
  {
  	drm_global_item_unref(&dev_priv->bo_global_ref.ref);
-	drm_global_item_unref(&dev_priv->mem_global_ref);
  }
diff --git a/drivers/staging/vboxvideo/vbox_drv.h b/drivers/staging/vboxvideo/vbox_drv.h
index 594f84272957..41f760b77704 100644
--- a/drivers/staging/vboxvideo/vbox_drv.h
+++ b/drivers/staging/vboxvideo/vbox_drv.h
@@ -95,7 +95,6 @@ struct vbox_private {
  	int fb_mtrr;
struct {
-		struct drm_global_reference mem_global_ref;
  		struct ttm_bo_global_ref bo_global_ref;
  		struct ttm_bo_device bdev;
  	} ttm;
diff --git a/drivers/staging/vboxvideo/vbox_ttm.c b/drivers/staging/vboxvideo/vbox_ttm.c
index 2329a55d4636..88cdacf2b0f0 100644
--- a/drivers/staging/vboxvideo/vbox_ttm.c
+++ b/drivers/staging/vboxvideo/vbox_ttm.c
@@ -35,16 +35,6 @@ static inline struct vbox_private *vbox_bdev(struct ttm_bo_device *bd)
  	return container_of(bd, struct vbox_private, ttm.bdev);
  }
-static int vbox_ttm_mem_global_init(struct drm_global_reference *ref)
-{
-	return ttm_mem_global_init(ref->object);
-}
-
-static void vbox_ttm_mem_global_release(struct drm_global_reference *ref)
-{
-	ttm_mem_global_release(ref->object);
-}
-
  /**
   * Adds the vbox memory manager object/structures to the global memory manager.
   */
@@ -53,18 +43,6 @@ static int vbox_ttm_global_init(struct vbox_private *vbox)
  	struct drm_global_reference *global_ref;
  	int ret;
- global_ref = &vbox->ttm.mem_global_ref;
-	global_ref->global_type = DRM_GLOBAL_TTM_MEM;
-	global_ref->size = sizeof(struct ttm_mem_global);
-	global_ref->init = &vbox_ttm_mem_global_init;
-	global_ref->release = &vbox_ttm_mem_global_release;
-	ret = drm_global_item_ref(global_ref);
-	if (ret) {
-		DRM_ERROR("Failed setting up TTM memory subsystem.\n");
-		return ret;
-	}
-
-	vbox->ttm.bo_global_ref.mem_glob = vbox->ttm.mem_global_ref.object;
  	global_ref = &vbox->ttm.bo_global_ref.ref;
  	global_ref->global_type = DRM_GLOBAL_TTM_BO;
  	global_ref->size = sizeof(struct ttm_bo_global);
@@ -74,7 +52,6 @@ static int vbox_ttm_global_init(struct vbox_private *vbox)
  	ret = drm_global_item_ref(global_ref);
  	if (ret) {
  		DRM_ERROR("Failed setting up TTM BO subsystem.\n");
-		drm_global_item_unref(&vbox->ttm.mem_global_ref);
  		return ret;
  	}
@@ -87,7 +64,6 @@ static int vbox_ttm_global_init(struct vbox_private *vbox)
  static void vbox_ttm_global_release(struct vbox_private *vbox)
  {
  	drm_global_item_unref(&vbox->ttm.bo_global_ref.ref);
-	drm_global_item_unref(&vbox->ttm.mem_global_ref);
  }
static void vbox_bo_ttm_destroy(struct ttm_buffer_object *tbo)
diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
index c6ee07d10281..4ae6fc33f761 100644
--- a/include/drm/ttm/ttm_bo_driver.h
+++ b/include/drm/ttm/ttm_bo_driver.h
@@ -570,8 +570,7 @@ void ttm_bo_mem_put_locked(struct ttm_buffer_object *bo,
  			   struct ttm_mem_reg *mem);
void ttm_bo_global_release(struct ttm_bo_global *glob);
-int ttm_bo_global_init(struct ttm_bo_global *glob,
-		       struct ttm_mem_global *mem_glob);
+int ttm_bo_global_init(struct ttm_bo_global *glob);
int ttm_bo_device_release(struct ttm_bo_device *bdev); @@ -895,7 +894,6 @@ extern const struct ttm_mem_type_manager_func ttm_bo_manager_func; struct ttm_bo_global_ref {
  	struct drm_global_reference ref;
-	struct ttm_mem_global *mem_glob;
  };
/**
@@ -909,9 +907,7 @@ struct ttm_bo_global_ref {
   */
  static inline int ttm_bo_global_ref_init(struct drm_global_reference *ref)
  {
-	struct ttm_bo_global_ref *bo_ref =
-		container_of(ref, struct ttm_bo_global_ref, ref);
-	return ttm_bo_global_init(ref->object, bo_ref->mem_glob);
+	return ttm_bo_global_init(ref->object);
  }
/**
diff --git a/include/drm/ttm/ttm_memory.h b/include/drm/ttm/ttm_memory.h
index 737b5fed8003..3ff48a0a2d7b 100644
--- a/include/drm/ttm/ttm_memory.h
+++ b/include/drm/ttm/ttm_memory.h
@@ -63,7 +63,7 @@
#define TTM_MEM_MAX_ZONES 2
  struct ttm_mem_zone;
-struct ttm_mem_global {
+extern struct ttm_mem_global {
  	struct kobject kobj;
  	struct ttm_bo_global *bo_glob;
  	struct workqueue_struct *swap_queue;
@@ -78,7 +78,7 @@ struct ttm_mem_global {
  #else
  	struct ttm_mem_zone *zone_dma32;
  #endif
-};
+} ttm_mem_glob;
extern int ttm_mem_global_init(struct ttm_mem_global *glob);
  extern void ttm_mem_global_release(struct ttm_mem_global *glob);


_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux