[PATCH 0/3] Provide struct ttm_global for TTM global state

[PATCH 0/3] Provide struct ttm_global for TTM global state

[Thomas Zimmermann]

TTM provides global memory and a global BO that is shared by all
TTM-based drivers. The data structures are provided by struct drm_global
and its helpers. All TTM-based DRM drivers copy the initialization and
clean-up code for the global TTM state from each other; leading to code
duplication.

The new structure struct ttm_global and its helpers provide a unified
implementation. Drivers only have to initialize it and forward the
contained BO global object to ttm_bo_device_init().

The amdgpu and radeon drivers are converted to struct ttm_global as
part of this patch set. All other TTM-based drivers share exactly the
same code patterns and can be converted in the same way.

Future directions: I already have patches for converting the remaining
TTM drivers to struct ttm_global. These patches can be merged after
the structure has become available in upstream. struct ttm_global is
implemented on top of struct drm_global. The latter actually maintains
TTM state instead of DRM state. Once all drivers have been converted,
    the code for struct drm_global can be merged into struct ttm_global.

(Resending this patch set, as I forgot to CC the mailing lists as first.)

Thomas Zimmermann (3):
  drm/ttm: Provide struct ttm_global for referencing TTM global state
  drm/amdgpu: Replace TTM initialization/release with ttm_global
  drm/radeon: Replace TTM initialization/release with ttm_global

 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 63 ++--------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h |  4 +-
 drivers/gpu/drm/radeon/radeon.h         |  4 +-
 drivers/gpu/drm/radeon/radeon_ttm.c     | 40 ++--------
 drivers/gpu/drm/ttm/Makefile            |  2 +-
 drivers/gpu/drm/ttm/ttm_global.c        | 98 +++++++++++++++++++++++++
 include/drm/drm_global.h                |  8 ++
 include/drm/ttm/ttm_global.h            | 79 ++++++++++++++++++++
 8 files changed, 200 insertions(+), 98 deletions(-)
 create mode 100644 drivers/gpu/drm/ttm/ttm_global.c
 create mode 100644 include/drm/ttm/ttm_global.h

--
2.19.1

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

[PATCH 1/3] drm/ttm: Provide struct ttm_global for referencing TTM global state

[Thomas Zimmermann]

The new struct ttm_global provides drivers with TTM's global memory and
BO in a unified way. Initialization and release is handled internally.

The functionality provided by struct ttm_global is currently re-implemented
by a dozen individual DRM drivers using struct drm_global. The implementation
of struct ttm_global is also built on top of drm_global, so it can co-exists
with the existing drivers. Once all TTM-based drivers have been converted to
struct ttm_global, the implementation of struct drm_global can be made
private to TTM.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ttm/Makefile     |  2 +-
 drivers/gpu/drm/ttm/ttm_global.c | 98 ++++++++++++++++++++++++++++++++
 include/drm/drm_global.h         |  8 +++
 include/drm/ttm/ttm_global.h     | 79 +++++++++++++++++++++++++
 4 files changed, 186 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/ttm/ttm_global.c
 create mode 100644 include/drm/ttm/ttm_global.h

diff --git a/drivers/gpu/drm/ttm/Makefile b/drivers/gpu/drm/ttm/Makefile
index 01fc670ce7a2..b7272b26e9f3 100644
--- a/drivers/gpu/drm/ttm/Makefile
+++ b/drivers/gpu/drm/ttm/Makefile
@@ -5,7 +5,7 @@
 ttm-y := ttm_memory.o ttm_tt.o ttm_bo.o \
 	ttm_bo_util.o ttm_bo_vm.o ttm_module.o \
 	ttm_execbuf_util.o ttm_page_alloc.o ttm_bo_manager.o \
-	ttm_page_alloc_dma.o
+	ttm_page_alloc_dma.o ttm_global.o
 ttm-$(CONFIG_AGP) += ttm_agp_backend.o
 
 obj-$(CONFIG_DRM_TTM) += ttm.o
diff --git a/drivers/gpu/drm/ttm/ttm_global.c b/drivers/gpu/drm/ttm/ttm_global.c
new file mode 100644
index 000000000000..ca9da0a46147
--- /dev/null
+++ b/drivers/gpu/drm/ttm/ttm_global.c
@@ -0,0 +1,98 @@
+/* SPDX-License-Identifier: GPL-2.0 OR MIT */
+/**************************************************************************
+ *
+ * Copyright 2008-2009 VMware, Inc., Palo Alto, CA., USA
+ * All Rights Reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the
+ * "Software"), to deal in the Software without restriction, including
+ * without limitation the rights to use, copy, modify, merge, publish,
+ * distribute, sub license, and/or sell copies of the Software, and to
+ * permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial portions
+ * of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM,
+ * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+ * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+ * USE OR OTHER DEALINGS IN THE SOFTWARE.
+ *
+ **************************************************************************/
+
+#include <drm/ttm/ttm_global.h>
+#include <drm/ttm/ttm_bo_driver.h>
+#include <drm/ttm/ttm_memory.h>
+#include <linux/kernel.h>
+
+static int ttm_global_init_mem(struct drm_global_reference *ref)
+{
+	BUG_ON(!ref->object);
+	return ttm_mem_global_init(ref->object);
+}
+
+static void ttm_global_release_mem(struct drm_global_reference *ref)
+{
+	BUG_ON(!ref->object);
+	ttm_mem_global_release(ref->object);
+}
+
+static int ttm_global_init_bo(struct drm_global_reference *ref)
+{
+	struct ttm_global *glob =
+		container_of(ref, struct ttm_global, bo_ref);
+	BUG_ON(!ref->object);
+	BUG_ON(!glob->mem_ref.object);
+	return ttm_bo_global_init(ref->object, glob->mem_ref.object);
+}
+
+static void ttm_global_release_bo(struct drm_global_reference *ref)
+{
+	BUG_ON(!ref->object);
+	ttm_bo_global_release(ref->object);
+}
+
+int ttm_global_init(struct ttm_global *glob)
+{
+	int ret;
+
+	glob->mem_ref.global_type = DRM_GLOBAL_TTM_MEM;
+	glob->mem_ref.size = sizeof(struct ttm_mem_global);
+	glob->bo_ref.object = NULL;
+	glob->mem_ref.init = &ttm_global_init_mem;
+	glob->mem_ref.release = &ttm_global_release_mem;
+	ret = drm_global_item_ref(&glob->mem_ref);
+	if (ret)
+		return ret;
+
+	glob->bo_ref.global_type = DRM_GLOBAL_TTM_BO;
+	glob->bo_ref.size = sizeof(struct ttm_bo_global);
+	glob->bo_ref.object = NULL;
+	glob->bo_ref.init = &ttm_global_init_bo;
+	glob->bo_ref.release = &ttm_global_release_bo;
+	ret = drm_global_item_ref(&glob->bo_ref);
+	if (ret)
+		goto err_drm_global_item_unref_mem;
+
+	return 0;
+
+err_drm_global_item_unref_mem:
+	drm_global_item_unref(&glob->mem_ref);
+	return ret;
+}
+
+EXPORT_SYMBOL(ttm_global_init);
+
+void ttm_global_release(struct ttm_global *glob)
+{
+	drm_global_item_unref(&glob->bo_ref);
+	drm_global_item_unref(&glob->mem_ref);
+}
+
+EXPORT_SYMBOL(ttm_global_release);
diff --git a/include/drm/drm_global.h b/include/drm/drm_global.h
index 3a830602a2e4..4482a9bbd6e9 100644
--- a/include/drm/drm_global.h
+++ b/include/drm/drm_global.h
@@ -28,8 +28,16 @@
  * Authors: Thomas Hellstrom <thellstrom-at-vmware-dot-com>
  */
 
+/*
+ *  The interfaces in this file are deprecated. Please use ttm_global
+ *  from <drm/ttm/ttm_global.h> instead.
+ */
+
 #ifndef _DRM_GLOBAL_H_
 #define _DRM_GLOBAL_H_
+
+#include <linux/types.h>
+
 enum drm_global_types {
 	DRM_GLOBAL_TTM_MEM = 0,
 	DRM_GLOBAL_TTM_BO,
diff --git a/include/drm/ttm/ttm_global.h b/include/drm/ttm/ttm_global.h
new file mode 100644
index 000000000000..06e791499f87
--- /dev/null
+++ b/include/drm/ttm/ttm_global.h
@@ -0,0 +1,79 @@
+/**************************************************************************
+ *
+ * Copyright 2008-2009 VMware, Inc., Palo Alto, CA., USA
+ * All Rights Reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the
+ * "Software"), to deal in the Software without restriction, including
+ * without limitation the rights to use, copy, modify, merge, publish,
+ * distribute, sub license, and/or sell copies of the Software, and to
+ * permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial portions
+ * of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM,
+ * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+ * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+ * USE OR OTHER DEALINGS IN THE SOFTWARE.
+ *
+ **************************************************************************/
+
+#ifndef _TTM_GLOBAL_H_
+#define _TTM_GLOBAL_H_
+
+#include <drm/drm_global.h>
+
+struct ttm_bo_global;
+
+/**
+ * struct ttm_global - Stores references to global TTM state
+ */
+struct ttm_global {
+	struct drm_global_reference mem_ref;
+	struct drm_global_reference bo_ref;
+};
+
+/**
+ * ttm_global_init
+ *
+ * @glob: A pointer to a struct ttm_global that is about to be initialized
+ * @return Zero on success, or a negative error code otherwise.
+ *
+ * Initializes an instance of struct ttm_global with TTM's global state
+ */
+int ttm_global_init(struct ttm_global *glob);
+
+/**
+ * ttm_global_release
+ *
+ * @glob: A pointer to an instance of struct ttm_global
+ *
+ * Releases an initialized instance of struct ttm_global. If the instance
+ * constains the final references to the global memory and BO, the global
+ * structures are released as well.
+ */
+void ttm_global_release(struct ttm_global *glob);
+
+/**
+ * ttm_global_get_bo_global
+ *
+ * @glob A pointer to an instance of struct ttm_global
+ * @return A refernce to the global BO.
+ *
+ * Returns the global BO. The BO should be forwarded to the initialization
+ * of a driver's TTM BO device.
+ */
+static inline struct ttm_bo_global*
+ttm_global_get_bo_global(struct ttm_global *glob)
+{
+	return glob->bo_ref.object;
+}
+
+#endif /* _TTM_GLOBAL_H_ */
-- 
2.19.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] drm/amdgpu: Replace TTM initialization/release with ttm_global

[Thomas Zimmermann]

Unified initialization and relesae of the global TTM state is provided
by struct ttm_global and its interfaces.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 63 ++-----------------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h |  4 +-
 2 files changed, 7 insertions(+), 60 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 3a6802846698..70b0e8c77bb4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -65,33 +65,6 @@ static void amdgpu_ttm_debugfs_fini(struct amdgpu_device *adev);
  * Global memory.
  */
 
-/**
- * amdgpu_ttm_mem_global_init - Initialize and acquire reference to
- * memory object
- *
- * @ref: Object for initialization.
- *
- * This is called by drm_global_item_ref() when an object is being
- * initialized.
- */
-static int amdgpu_ttm_mem_global_init(struct drm_global_reference *ref)
-{
-	return ttm_mem_global_init(ref->object);
-}
-
-/**
- * amdgpu_ttm_mem_global_release - Drop reference to a memory object
- *
- * @ref: Object being removed
- *
- * This is called by drm_global_item_unref() when an object is being
- * released.
- */
-static void amdgpu_ttm_mem_global_release(struct drm_global_reference *ref)
-{
-	ttm_mem_global_release(ref->object);
-}
-
 /**
  * amdgpu_ttm_global_init - Initialize global TTM memory reference structures.
  *
@@ -102,35 +75,15 @@ static void amdgpu_ttm_mem_global_release(struct drm_global_reference *ref)
  */
 static int amdgpu_ttm_global_init(struct amdgpu_device *adev)
 {
-	struct drm_global_reference *global_ref;
 	int r;
 
 	/* ensure reference is false in case init fails */
 	adev->mman.mem_global_referenced = false;
 
-	global_ref = &adev->mman.mem_global_ref;
-	global_ref->global_type = DRM_GLOBAL_TTM_MEM;
-	global_ref->size = sizeof(struct ttm_mem_global);
-	global_ref->init = &amdgpu_ttm_mem_global_init;
-	global_ref->release = &amdgpu_ttm_mem_global_release;
-	r = drm_global_item_ref(global_ref);
+	r = ttm_global_init(&adev->mman.glob);
 	if (r) {
-		DRM_ERROR("Failed setting up TTM memory accounting "
-			  "subsystem.\n");
-		goto error_mem;
-	}
-
-	adev->mman.bo_global_ref.mem_glob =
-		adev->mman.mem_global_ref.object;
-	global_ref = &adev->mman.bo_global_ref.ref;
-	global_ref->global_type = DRM_GLOBAL_TTM_BO;
-	global_ref->size = sizeof(struct ttm_bo_global);
-	global_ref->init = &ttm_bo_global_ref_init;
-	global_ref->release = &ttm_bo_global_ref_release;
-	r = drm_global_item_ref(global_ref);
-	if (r) {
-		DRM_ERROR("Failed setting up TTM BO subsystem.\n");
-		goto error_bo;
+		DRM_ERROR("Failed setting up TTM subsystem.\n");
+		return r;
 	}
 
 	mutex_init(&adev->mman.gtt_window_lock);
@@ -138,19 +91,13 @@ static int amdgpu_ttm_global_init(struct amdgpu_device *adev)
 	adev->mman.mem_global_referenced = true;
 
 	return 0;
-
-error_bo:
-	drm_global_item_unref(&adev->mman.mem_global_ref);
-error_mem:
-	return r;
 }
 
 static void amdgpu_ttm_global_fini(struct amdgpu_device *adev)
 {
 	if (adev->mman.mem_global_referenced) {
 		mutex_destroy(&adev->mman.gtt_window_lock);
-		drm_global_item_unref(&adev->mman.bo_global_ref.ref);
-		drm_global_item_unref(&adev->mman.mem_global_ref);
+		ttm_global_release(&adev->mman.glob);
 		adev->mman.mem_global_referenced = false;
 	}
 }
@@ -1765,7 +1712,7 @@ int amdgpu_ttm_init(struct amdgpu_device *adev)
 	}
 	/* No others user of address space so set it to 0 */
 	r = ttm_bo_device_init(&adev->mman.bdev,
-			       adev->mman.bo_global_ref.ref.object,
+			       ttm_global_get_bo_global(&adev->mman.glob),
 			       &amdgpu_bo_driver,
 			       adev->ddev->anon_inode->i_mapping,
 			       DRM_FILE_PAGE_OFFSET,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
index fe8f276e9811..c3a7fe3ead3a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
@@ -26,6 +26,7 @@
 
 #include "amdgpu.h"
 #include <drm/gpu_scheduler.h>
+#include <drm/ttm/ttm_global.h>
 
 #define AMDGPU_PL_GDS		(TTM_PL_PRIV + 0)
 #define AMDGPU_PL_GWS		(TTM_PL_PRIV + 1)
@@ -39,8 +40,7 @@
 #define AMDGPU_GTT_NUM_TRANSFER_WINDOWS	2
 
 struct amdgpu_mman {
-	struct ttm_bo_global_ref        bo_global_ref;
-	struct drm_global_reference	mem_global_ref;
+	struct ttm_global		glob;
 	struct ttm_bo_device		bdev;
 	bool				mem_global_referenced;
 	bool				initialized;
-- 
2.19.1

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

[PATCH 3/3] drm/radeon: Replace TTM initialization/release with ttm_global

[Thomas Zimmermann]

Unified initialization and release of the global TTM state is provided
by struct ttm_global and its interfaces.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/radeon/radeon.h     |  4 +--
 drivers/gpu/drm/radeon/radeon_ttm.c | 40 ++++-------------------------
 2 files changed, 7 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index 1a6f6edb3515..554c0421b779 100644
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -73,6 +73,7 @@
 #include <drm/ttm/ttm_placement.h>
 #include <drm/ttm/ttm_module.h>
 #include <drm/ttm/ttm_execbuf_util.h>
+#include <drm/ttm/ttm_global.h>
 
 #include <drm/drm_gem.h>
 
@@ -448,8 +449,7 @@ struct radeon_surface_reg {
  * TTM.
  */
 struct radeon_mman {
-	struct ttm_bo_global_ref        bo_global_ref;
-	struct drm_global_reference	mem_global_ref;
+	struct ttm_global		glob;
 	struct ttm_bo_device		bdev;
 	bool				mem_global_referenced;
 	bool				initialized;
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
index dac4ec5a120b..363860e82892 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -64,45 +64,16 @@ static struct radeon_device *radeon_get_rdev(struct ttm_bo_device *bdev)
 /*
  * Global memory.
  */
-static int radeon_ttm_mem_global_init(struct drm_global_reference *ref)
-{
-	return ttm_mem_global_init(ref->object);
-}
-
-static void radeon_ttm_mem_global_release(struct drm_global_reference *ref)
-{
-	ttm_mem_global_release(ref->object);
-}
 
 static int radeon_ttm_global_init(struct radeon_device *rdev)
 {
-	struct drm_global_reference *global_ref;
 	int r;
 
 	rdev->mman.mem_global_referenced = false;
-	global_ref = &rdev->mman.mem_global_ref;
-	global_ref->global_type = DRM_GLOBAL_TTM_MEM;
-	global_ref->size = sizeof(struct ttm_mem_global);
-	global_ref->init = &radeon_ttm_mem_global_init;
-	global_ref->release = &radeon_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;
-	}
 
-	rdev->mman.bo_global_ref.mem_glob =
-		rdev->mman.mem_global_ref.object;
-	global_ref = &rdev->mman.bo_global_ref.ref;
-	global_ref->global_type = DRM_GLOBAL_TTM_BO;
-	global_ref->size = sizeof(struct ttm_bo_global);
-	global_ref->init = &ttm_bo_global_ref_init;
-	global_ref->release = &ttm_bo_global_ref_release;
-	r = drm_global_item_ref(global_ref);
-	if (r != 0) {
-		DRM_ERROR("Failed setting up TTM BO subsystem.\n");
-		drm_global_item_unref(&rdev->mman.mem_global_ref);
+	r = ttm_global_init(&rdev->mman.glob);
+	if (r) {
+		DRM_ERROR("Failed setting up TTM subsystem.\n");
 		return r;
 	}
 
@@ -113,8 +84,7 @@ static int radeon_ttm_global_init(struct radeon_device *rdev)
 static void radeon_ttm_global_fini(struct radeon_device *rdev)
 {
 	if (rdev->mman.mem_global_referenced) {
-		drm_global_item_unref(&rdev->mman.bo_global_ref.ref);
-		drm_global_item_unref(&rdev->mman.mem_global_ref);
+		ttm_global_release(&rdev->mman.glob);
 		rdev->mman.mem_global_referenced = false;
 	}
 }
@@ -853,7 +823,7 @@ int radeon_ttm_init(struct radeon_device *rdev)
 	}
 	/* No others user of address space so set it to 0 */
 	r = ttm_bo_device_init(&rdev->mman.bdev,
-			       rdev->mman.bo_global_ref.ref.object,
+			       ttm_global_get_bo_global(&rdev->mman.glob),
 			       &radeon_bo_driver,
 			       rdev->ddev->anon_inode->i_mapping,
 			       DRM_FILE_PAGE_OFFSET,
-- 
2.19.1

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

Re: [PATCH 1/3] drm/ttm: Provide struct ttm_global for referencing TTM global state

[Huang Rui]

On Fri, Oct 19, 2018 at 12:27:50AM +0800, Thomas Zimmermann wrote:
> The new struct ttm_global provides drivers with TTM's global memory and
> BO in a unified way. Initialization and release is handled internally.
> 
> The functionality provided by struct ttm_global is currently re-implemented
> by a dozen individual DRM drivers using struct drm_global. The implementation
> of struct ttm_global is also built on top of drm_global, so it can co-exists
> with the existing drivers. Once all TTM-based drivers have been converted to
> struct ttm_global, the implementation of struct drm_global can be made
> private to TTM.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/ttm/Makefile     |  2 +-
>  drivers/gpu/drm/ttm/ttm_global.c | 98 ++++++++++++++++++++++++++++++++
>  include/drm/drm_global.h         |  8 +++
>  include/drm/ttm/ttm_global.h     | 79 +++++++++++++++++++++++++
>  4 files changed, 186 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/ttm/ttm_global.c
>  create mode 100644 include/drm/ttm/ttm_global.h
> 
> diff --git a/drivers/gpu/drm/ttm/Makefile b/drivers/gpu/drm/ttm/Makefile
> index 01fc670ce7a2..b7272b26e9f3 100644
> --- a/drivers/gpu/drm/ttm/Makefile
> +++ b/drivers/gpu/drm/ttm/Makefile
> @@ -5,7 +5,7 @@
>  ttm-y := ttm_memory.o ttm_tt.o ttm_bo.o \
>  	ttm_bo_util.o ttm_bo_vm.o ttm_module.o \
>  	ttm_execbuf_util.o ttm_page_alloc.o ttm_bo_manager.o \
> -	ttm_page_alloc_dma.o
> +	ttm_page_alloc_dma.o ttm_global.o
>  ttm-$(CONFIG_AGP) += ttm_agp_backend.o
>  
>  obj-$(CONFIG_DRM_TTM) += ttm.o
> diff --git a/drivers/gpu/drm/ttm/ttm_global.c b/drivers/gpu/drm/ttm/ttm_global.c
> new file mode 100644
> index 000000000000..ca9da0a46147
> --- /dev/null
> +++ b/drivers/gpu/drm/ttm/ttm_global.c
> @@ -0,0 +1,98 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> +/**************************************************************************
> + *
> + * Copyright 2008-2009 VMware, Inc., Palo Alto, CA., USA
> + * All Rights Reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the
> + * "Software"), to deal in the Software without restriction, including
> + * without limitation the rights to use, copy, modify, merge, publish,
> + * distribute, sub license, and/or sell copies of the Software, and to
> + * permit persons to whom the Software is furnished to do so, subject to
> + * the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> + * next paragraph) shall be included in all copies or substantial portions
> + * of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM,
> + * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> + * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
> + * USE OR OTHER DEALINGS IN THE SOFTWARE.
> + *
> + **************************************************************************/
> +
> +#include <drm/ttm/ttm_global.h>
> +#include <drm/ttm/ttm_bo_driver.h>
> +#include <drm/ttm/ttm_memory.h>
> +#include <linux/kernel.h>
> +
> +static int ttm_global_init_mem(struct drm_global_reference *ref)
> +{
> +	BUG_ON(!ref->object);
> +	return ttm_mem_global_init(ref->object);
> +}
> +
> +static void ttm_global_release_mem(struct drm_global_reference *ref)
> +{
> +	BUG_ON(!ref->object);
> +	ttm_mem_global_release(ref->object);
> +}
> +
> +static int ttm_global_init_bo(struct drm_global_reference *ref)
> +{
> +	struct ttm_global *glob =
> +		container_of(ref, struct ttm_global, bo_ref);
> +	BUG_ON(!ref->object);
> +	BUG_ON(!glob->mem_ref.object);
> +	return ttm_bo_global_init(ref->object, glob->mem_ref.object);
> +}
> +
> +static void ttm_global_release_bo(struct drm_global_reference *ref)
> +{
> +	BUG_ON(!ref->object);
> +	ttm_bo_global_release(ref->object);
> +}
> +
> +int ttm_global_init(struct ttm_global *glob)
> +{
> +	int ret;
> +

We would better add a protection here to make sure the glob is not NULL.

if (!glob)
        return -EINVAL;

Others, look good for me.

Thanks,
Ray

> +	glob->mem_ref.global_type = DRM_GLOBAL_TTM_MEM;
> +	glob->mem_ref.size = sizeof(struct ttm_mem_global);
> +	glob->bo_ref.object = NULL;
> +	glob->mem_ref.init = &ttm_global_init_mem;
> +	glob->mem_ref.release = &ttm_global_release_mem;
> +	ret = drm_global_item_ref(&glob->mem_ref);
> +	if (ret)
> +		return ret;
> +
> +	glob->bo_ref.global_type = DRM_GLOBAL_TTM_BO;
> +	glob->bo_ref.size = sizeof(struct ttm_bo_global);
> +	glob->bo_ref.object = NULL;
> +	glob->bo_ref.init = &ttm_global_init_bo;
> +	glob->bo_ref.release = &ttm_global_release_bo;
> +	ret = drm_global_item_ref(&glob->bo_ref);
> +	if (ret)
> +		goto err_drm_global_item_unref_mem;
> +
> +	return 0;
> +
> +err_drm_global_item_unref_mem:
> +	drm_global_item_unref(&glob->mem_ref);
> +	return ret;
> +}
> +
> +EXPORT_SYMBOL(ttm_global_init);
> +
> +void ttm_global_release(struct ttm_global *glob)
> +{
> +	drm_global_item_unref(&glob->bo_ref);
> +	drm_global_item_unref(&glob->mem_ref);
> +}
> +
> +EXPORT_SYMBOL(ttm_global_release);
> diff --git a/include/drm/drm_global.h b/include/drm/drm_global.h
> index 3a830602a2e4..4482a9bbd6e9 100644
> --- a/include/drm/drm_global.h
> +++ b/include/drm/drm_global.h
> @@ -28,8 +28,16 @@
>   * Authors: Thomas Hellstrom <thellstrom-at-vmware-dot-com>
>   */
>  
> +/*
> + *  The interfaces in this file are deprecated. Please use ttm_global
> + *  from <drm/ttm/ttm_global.h> instead.
> + */
> +
>  #ifndef _DRM_GLOBAL_H_
>  #define _DRM_GLOBAL_H_
> +
> +#include <linux/types.h>
> +
>  enum drm_global_types {
>  	DRM_GLOBAL_TTM_MEM = 0,
>  	DRM_GLOBAL_TTM_BO,
> diff --git a/include/drm/ttm/ttm_global.h b/include/drm/ttm/ttm_global.h
> new file mode 100644
> index 000000000000..06e791499f87
> --- /dev/null
> +++ b/include/drm/ttm/ttm_global.h
> @@ -0,0 +1,79 @@
> +/**************************************************************************
> + *
> + * Copyright 2008-2009 VMware, Inc., Palo Alto, CA., USA
> + * All Rights Reserved.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the
> + * "Software"), to deal in the Software without restriction, including
> + * without limitation the rights to use, copy, modify, merge, publish,
> + * distribute, sub license, and/or sell copies of the Software, and to
> + * permit persons to whom the Software is furnished to do so, subject to
> + * the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> + * next paragraph) shall be included in all copies or substantial portions
> + * of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM,
> + * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> + * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
> + * USE OR OTHER DEALINGS IN THE SOFTWARE.
> + *
> + **************************************************************************/
> +
> +#ifndef _TTM_GLOBAL_H_
> +#define _TTM_GLOBAL_H_
> +
> +#include <drm/drm_global.h>
> +
> +struct ttm_bo_global;
> +
> +/**
> + * struct ttm_global - Stores references to global TTM state
> + */
> +struct ttm_global {
> +	struct drm_global_reference mem_ref;
> +	struct drm_global_reference bo_ref;
> +};
> +
> +/**
> + * ttm_global_init
> + *
> + * @glob: A pointer to a struct ttm_global that is about to be initialized
> + * @return Zero on success, or a negative error code otherwise.
> + *
> + * Initializes an instance of struct ttm_global with TTM's global state
> + */
> +int ttm_global_init(struct ttm_global *glob);
> +
> +/**
> + * ttm_global_release
> + *
> + * @glob: A pointer to an instance of struct ttm_global
> + *
> + * Releases an initialized instance of struct ttm_global. If the instance
> + * constains the final references to the global memory and BO, the global
> + * structures are released as well.
> + */
> +void ttm_global_release(struct ttm_global *glob);
> +
> +/**
> + * ttm_global_get_bo_global
> + *
> + * @glob A pointer to an instance of struct ttm_global
> + * @return A refernce to the global BO.
> + *
> + * Returns the global BO. The BO should be forwarded to the initialization
> + * of a driver's TTM BO device.
> + */
> +static inline struct ttm_bo_global*
> +ttm_global_get_bo_global(struct ttm_global *glob)
> +{
> +	return glob->bo_ref.object;
> +}
> +
> +#endif /* _TTM_GLOBAL_H_ */
> -- 
> 2.19.1
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/amdgpu: Replace TTM initialization/release with ttm_global

[Huang Rui]

On Fri, Oct 19, 2018 at 12:27:51AM +0800, Thomas Zimmermann wrote:
> Unified initialization and relesae of the global TTM state is provided
> by struct ttm_global and its interfaces.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Reviewed-by: Huang Rui <ray.huang@amd.com>

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 63 ++-----------------------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h |  4 +-
>  2 files changed, 7 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 3a6802846698..70b0e8c77bb4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -65,33 +65,6 @@ static void amdgpu_ttm_debugfs_fini(struct amdgpu_device *adev);
>   * Global memory.
>   */
>  
> -/**
> - * amdgpu_ttm_mem_global_init - Initialize and acquire reference to
> - * memory object
> - *
> - * @ref: Object for initialization.
> - *
> - * This is called by drm_global_item_ref() when an object is being
> - * initialized.
> - */
> -static int amdgpu_ttm_mem_global_init(struct drm_global_reference *ref)
> -{
> -	return ttm_mem_global_init(ref->object);
> -}
> -
> -/**
> - * amdgpu_ttm_mem_global_release - Drop reference to a memory object
> - *
> - * @ref: Object being removed
> - *
> - * This is called by drm_global_item_unref() when an object is being
> - * released.
> - */
> -static void amdgpu_ttm_mem_global_release(struct drm_global_reference *ref)
> -{
> -	ttm_mem_global_release(ref->object);
> -}
> -
>  /**
>   * amdgpu_ttm_global_init - Initialize global TTM memory reference structures.
>   *
> @@ -102,35 +75,15 @@ static void amdgpu_ttm_mem_global_release(struct drm_global_reference *ref)
>   */
>  static int amdgpu_ttm_global_init(struct amdgpu_device *adev)
>  {
> -	struct drm_global_reference *global_ref;
>  	int r;
>  
>  	/* ensure reference is false in case init fails */
>  	adev->mman.mem_global_referenced = false;
>  
> -	global_ref = &adev->mman.mem_global_ref;
> -	global_ref->global_type = DRM_GLOBAL_TTM_MEM;
> -	global_ref->size = sizeof(struct ttm_mem_global);
> -	global_ref->init = &amdgpu_ttm_mem_global_init;
> -	global_ref->release = &amdgpu_ttm_mem_global_release;
> -	r = drm_global_item_ref(global_ref);
> +	r = ttm_global_init(&adev->mman.glob);
>  	if (r) {
> -		DRM_ERROR("Failed setting up TTM memory accounting "
> -			  "subsystem.\n");
> -		goto error_mem;
> -	}
> -
> -	adev->mman.bo_global_ref.mem_glob =
> -		adev->mman.mem_global_ref.object;
> -	global_ref = &adev->mman.bo_global_ref.ref;
> -	global_ref->global_type = DRM_GLOBAL_TTM_BO;
> -	global_ref->size = sizeof(struct ttm_bo_global);
> -	global_ref->init = &ttm_bo_global_ref_init;
> -	global_ref->release = &ttm_bo_global_ref_release;
> -	r = drm_global_item_ref(global_ref);
> -	if (r) {
> -		DRM_ERROR("Failed setting up TTM BO subsystem.\n");
> -		goto error_bo;
> +		DRM_ERROR("Failed setting up TTM subsystem.\n");
> +		return r;
>  	}
>  
>  	mutex_init(&adev->mman.gtt_window_lock);
> @@ -138,19 +91,13 @@ static int amdgpu_ttm_global_init(struct amdgpu_device *adev)
>  	adev->mman.mem_global_referenced = true;
>  
>  	return 0;
> -
> -error_bo:
> -	drm_global_item_unref(&adev->mman.mem_global_ref);
> -error_mem:
> -	return r;
>  }
>  
>  static void amdgpu_ttm_global_fini(struct amdgpu_device *adev)
>  {
>  	if (adev->mman.mem_global_referenced) {
>  		mutex_destroy(&adev->mman.gtt_window_lock);
> -		drm_global_item_unref(&adev->mman.bo_global_ref.ref);
> -		drm_global_item_unref(&adev->mman.mem_global_ref);
> +		ttm_global_release(&adev->mman.glob);
>  		adev->mman.mem_global_referenced = false;
>  	}
>  }
> @@ -1765,7 +1712,7 @@ int amdgpu_ttm_init(struct amdgpu_device *adev)
>  	}
>  	/* No others user of address space so set it to 0 */
>  	r = ttm_bo_device_init(&adev->mman.bdev,
> -			       adev->mman.bo_global_ref.ref.object,
> +			       ttm_global_get_bo_global(&adev->mman.glob),
>  			       &amdgpu_bo_driver,
>  			       adev->ddev->anon_inode->i_mapping,
>  			       DRM_FILE_PAGE_OFFSET,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> index fe8f276e9811..c3a7fe3ead3a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> @@ -26,6 +26,7 @@
>  
>  #include "amdgpu.h"
>  #include <drm/gpu_scheduler.h>
> +#include <drm/ttm/ttm_global.h>
>  
>  #define AMDGPU_PL_GDS		(TTM_PL_PRIV + 0)
>  #define AMDGPU_PL_GWS		(TTM_PL_PRIV + 1)
> @@ -39,8 +40,7 @@
>  #define AMDGPU_GTT_NUM_TRANSFER_WINDOWS	2
>  
>  struct amdgpu_mman {
> -	struct ttm_bo_global_ref        bo_global_ref;
> -	struct drm_global_reference	mem_global_ref;
> +	struct ttm_global		glob;
>  	struct ttm_bo_device		bdev;
>  	bool				mem_global_referenced;
>  	bool				initialized;
> -- 
> 2.19.1
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 3/3] drm/radeon: Replace TTM initialization/release with ttm_global

[Huang Rui]

On Fri, Oct 19, 2018 at 12:27:52AM +0800, Thomas Zimmermann wrote:
> Unified initialization and release of the global TTM state is provided
> by struct ttm_global and its interfaces.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Reviewed-by: Huang Rui <ray.huang@amd.com>

> ---
>  drivers/gpu/drm/radeon/radeon.h     |  4 +--
>  drivers/gpu/drm/radeon/radeon_ttm.c | 40 ++++-------------------------
>  2 files changed, 7 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
> index 1a6f6edb3515..554c0421b779 100644
> --- a/drivers/gpu/drm/radeon/radeon.h
> +++ b/drivers/gpu/drm/radeon/radeon.h
> @@ -73,6 +73,7 @@
>  #include <drm/ttm/ttm_placement.h>
>  #include <drm/ttm/ttm_module.h>
>  #include <drm/ttm/ttm_execbuf_util.h>
> +#include <drm/ttm/ttm_global.h>
>  
>  #include <drm/drm_gem.h>
>  
> @@ -448,8 +449,7 @@ struct radeon_surface_reg {
>   * TTM.
>   */
>  struct radeon_mman {
> -	struct ttm_bo_global_ref        bo_global_ref;
> -	struct drm_global_reference	mem_global_ref;
> +	struct ttm_global		glob;
>  	struct ttm_bo_device		bdev;
>  	bool				mem_global_referenced;
>  	bool				initialized;
> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
> index dac4ec5a120b..363860e82892 100644
> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> @@ -64,45 +64,16 @@ static struct radeon_device *radeon_get_rdev(struct ttm_bo_device *bdev)
>  /*
>   * Global memory.
>   */
> -static int radeon_ttm_mem_global_init(struct drm_global_reference *ref)
> -{
> -	return ttm_mem_global_init(ref->object);
> -}
> -
> -static void radeon_ttm_mem_global_release(struct drm_global_reference *ref)
> -{
> -	ttm_mem_global_release(ref->object);
> -}
>  
>  static int radeon_ttm_global_init(struct radeon_device *rdev)
>  {
> -	struct drm_global_reference *global_ref;
>  	int r;
>  
>  	rdev->mman.mem_global_referenced = false;
> -	global_ref = &rdev->mman.mem_global_ref;
> -	global_ref->global_type = DRM_GLOBAL_TTM_MEM;
> -	global_ref->size = sizeof(struct ttm_mem_global);
> -	global_ref->init = &radeon_ttm_mem_global_init;
> -	global_ref->release = &radeon_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;
> -	}
>  
> -	rdev->mman.bo_global_ref.mem_glob =
> -		rdev->mman.mem_global_ref.object;
> -	global_ref = &rdev->mman.bo_global_ref.ref;
> -	global_ref->global_type = DRM_GLOBAL_TTM_BO;
> -	global_ref->size = sizeof(struct ttm_bo_global);
> -	global_ref->init = &ttm_bo_global_ref_init;
> -	global_ref->release = &ttm_bo_global_ref_release;
> -	r = drm_global_item_ref(global_ref);
> -	if (r != 0) {
> -		DRM_ERROR("Failed setting up TTM BO subsystem.\n");
> -		drm_global_item_unref(&rdev->mman.mem_global_ref);
> +	r = ttm_global_init(&rdev->mman.glob);
> +	if (r) {
> +		DRM_ERROR("Failed setting up TTM subsystem.\n");
>  		return r;
>  	}
>  
> @@ -113,8 +84,7 @@ static int radeon_ttm_global_init(struct radeon_device *rdev)
>  static void radeon_ttm_global_fini(struct radeon_device *rdev)
>  {
>  	if (rdev->mman.mem_global_referenced) {
> -		drm_global_item_unref(&rdev->mman.bo_global_ref.ref);
> -		drm_global_item_unref(&rdev->mman.mem_global_ref);
> +		ttm_global_release(&rdev->mman.glob);
>  		rdev->mman.mem_global_referenced = false;
>  	}
>  }
> @@ -853,7 +823,7 @@ int radeon_ttm_init(struct radeon_device *rdev)
>  	}
>  	/* No others user of address space so set it to 0 */
>  	r = ttm_bo_device_init(&rdev->mman.bdev,
> -			       rdev->mman.bo_global_ref.ref.object,
> +			       ttm_global_get_bo_global(&rdev->mman.glob),
>  			       &radeon_bo_driver,
>  			       rdev->ddev->anon_inode->i_mapping,
>  			       DRM_FILE_PAGE_OFFSET,
> -- 
> 2.19.1
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/amdgpu: Replace TTM initialization/release with ttm_global

[Zhang, Jerry(Junwei)]

On 10/19/2018 12:27 AM, Thomas Zimmermann wrote:
> Unified initialization and relesae of the global TTM state is provided
> by struct ttm_global and its interfaces.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 63 ++-----------------------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h |  4 +-
>   2 files changed, 7 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 3a6802846698..70b0e8c77bb4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -65,33 +65,6 @@ static void amdgpu_ttm_debugfs_fini(struct amdgpu_device *adev);
>    * Global memory.
>    */
>   
> -/**
> - * amdgpu_ttm_mem_global_init - Initialize and acquire reference to
> - * memory object
> - *
> - * @ref: Object for initialization.
> - *
> - * This is called by drm_global_item_ref() when an object is being
> - * initialized.
> - */
> -static int amdgpu_ttm_mem_global_init(struct drm_global_reference *ref)
> -{
> -	return ttm_mem_global_init(ref->object);
> -}
> -
> -/**
> - * amdgpu_ttm_mem_global_release - Drop reference to a memory object
> - *
> - * @ref: Object being removed
> - *
> - * This is called by drm_global_item_unref() when an object is being
> - * released.
> - */
> -static void amdgpu_ttm_mem_global_release(struct drm_global_reference *ref)
> -{
> -	ttm_mem_global_release(ref->object);
> -}
> -
>   /**
>    * amdgpu_ttm_global_init - Initialize global TTM memory reference structures.
>    *
> @@ -102,35 +75,15 @@ static void amdgpu_ttm_mem_global_release(struct drm_global_reference *ref)
>    */
>   static int amdgpu_ttm_global_init(struct amdgpu_device *adev)
>   {
> -	struct drm_global_reference *global_ref;
>   	int r;
>   
>   	/* ensure reference is false in case init fails */
>   	adev->mman.mem_global_referenced = false;
>   
> -	global_ref = &adev->mman.mem_global_ref;
> -	global_ref->global_type = DRM_GLOBAL_TTM_MEM;
> -	global_ref->size = sizeof(struct ttm_mem_global);
> -	global_ref->init = &amdgpu_ttm_mem_global_init;
> -	global_ref->release = &amdgpu_ttm_mem_global_release;
> -	r = drm_global_item_ref(global_ref);
> +	r = ttm_global_init(&adev->mman.glob);
>   	if (r) {
> -		DRM_ERROR("Failed setting up TTM memory accounting "
> -			  "subsystem.\n");
> -		goto error_mem;
> -	}
> -
> -	adev->mman.bo_global_ref.mem_glob =
> -		adev->mman.mem_global_ref.object;

Seems to miss this action.

Or are you going to replace
       struct ttm_bo_global_ref bo_global_ref
with
       struct ttm_global glob ->  struct drm_global_reference bo_ref

if so, may need to remove ttm_bo_global_ref_init() and struct 
ttm_bo_global_ref at the same time.


Regards,
Jerry

> -	global_ref = &adev->mman.bo_global_ref.ref;
> -	global_ref->global_type = DRM_GLOBAL_TTM_BO;
> -	global_ref->size = sizeof(struct ttm_bo_global);
> -	global_ref->init = &ttm_bo_global_ref_init;
> -	global_ref->release = &ttm_bo_global_ref_release;
> -	r = drm_global_item_ref(global_ref);
> -	if (r) {
> -		DRM_ERROR("Failed setting up TTM BO subsystem.\n");
> -		goto error_bo;
> +		DRM_ERROR("Failed setting up TTM subsystem.\n");
> +		return r;
>   	}
>   
>   	mutex_init(&adev->mman.gtt_window_lock);
> @@ -138,19 +91,13 @@ static int amdgpu_ttm_global_init(struct amdgpu_device *adev)
>   	adev->mman.mem_global_referenced = true;
>   
>   	return 0;
> -
> -error_bo:
> -	drm_global_item_unref(&adev->mman.mem_global_ref);
> -error_mem:
> -	return r;
>   }
>   
>   static void amdgpu_ttm_global_fini(struct amdgpu_device *adev)
>   {
>   	if (adev->mman.mem_global_referenced) {
>   		mutex_destroy(&adev->mman.gtt_window_lock);
> -		drm_global_item_unref(&adev->mman.bo_global_ref.ref);
> -		drm_global_item_unref(&adev->mman.mem_global_ref);
> +		ttm_global_release(&adev->mman.glob);
>   		adev->mman.mem_global_referenced = false;
>   	}
>   }
> @@ -1765,7 +1712,7 @@ int amdgpu_ttm_init(struct amdgpu_device *adev)
>   	}
>   	/* No others user of address space so set it to 0 */
>   	r = ttm_bo_device_init(&adev->mman.bdev,
> -			       adev->mman.bo_global_ref.ref.object,
> +			       ttm_global_get_bo_global(&adev->mman.glob),
>   			       &amdgpu_bo_driver,
>   			       adev->ddev->anon_inode->i_mapping,
>   			       DRM_FILE_PAGE_OFFSET,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> index fe8f276e9811..c3a7fe3ead3a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> @@ -26,6 +26,7 @@
>   
>   #include "amdgpu.h"
>   #include <drm/gpu_scheduler.h>
> +#include <drm/ttm/ttm_global.h>
>   
>   #define AMDGPU_PL_GDS		(TTM_PL_PRIV + 0)
>   #define AMDGPU_PL_GWS		(TTM_PL_PRIV + 1)
> @@ -39,8 +40,7 @@
>   #define AMDGPU_GTT_NUM_TRANSFER_WINDOWS	2
>   
>   struct amdgpu_mman {
> -	struct ttm_bo_global_ref        bo_global_ref;
> -	struct drm_global_reference	mem_global_ref;
> +	struct ttm_global		glob;
>   	struct ttm_bo_device		bdev;
>   	bool				mem_global_referenced;
>   	bool				initialized;

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

Re: [PATCH 0/3] Provide struct ttm_global for TTM global state

[Koenig, Christian]

Hi Thomas,

I honestly still find that way to complicated compared to what it 
actually does.

Both ttm_mem_global and ttm_bo_global can just be some static variables. 
E.g. the whole handling with drm_global_reference is just superfluous.

Can I just write a patch to clean up the mess we created?

Regards,
Christian.

Am 18.10.18 um 18:27 schrieb Thomas Zimmermann:
> TTM provides global memory and a global BO that is shared by all
> TTM-based drivers. The data structures are provided by struct drm_global
> and its helpers. All TTM-based DRM drivers copy the initialization and
> clean-up code for the global TTM state from each other; leading to code
> duplication.
>
> The new structure struct ttm_global and its helpers provide a unified
> implementation. Drivers only have to initialize it and forward the
> contained BO global object to ttm_bo_device_init().
>
> The amdgpu and radeon drivers are converted to struct ttm_global as
> part of this patch set. All other TTM-based drivers share exactly the
> same code patterns and can be converted in the same way.
>
> Future directions: I already have patches for converting the remaining
> TTM drivers to struct ttm_global. These patches can be merged after
> the structure has become available in upstream. struct ttm_global is
> implemented on top of struct drm_global. The latter actually maintains
> TTM state instead of DRM state. Once all drivers have been converted,
>      the code for struct drm_global can be merged into struct ttm_global.
>
> (Resending this patch set, as I forgot to CC the mailing lists as first.)
>
> Thomas Zimmermann (3):
>    drm/ttm: Provide struct ttm_global for referencing TTM global state
>    drm/amdgpu: Replace TTM initialization/release with ttm_global
>    drm/radeon: Replace TTM initialization/release with ttm_global
>
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 63 ++--------------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h |  4 +-
>   drivers/gpu/drm/radeon/radeon.h         |  4 +-
>   drivers/gpu/drm/radeon/radeon_ttm.c     | 40 ++--------
>   drivers/gpu/drm/ttm/Makefile            |  2 +-
>   drivers/gpu/drm/ttm/ttm_global.c        | 98 +++++++++++++++++++++++++
>   include/drm/drm_global.h                |  8 ++
>   include/drm/ttm/ttm_global.h            | 79 ++++++++++++++++++++
>   8 files changed, 200 insertions(+), 98 deletions(-)
>   create mode 100644 drivers/gpu/drm/ttm/ttm_global.c
>   create mode 100644 include/drm/ttm/ttm_global.h
>
> --
> 2.19.1
>

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

Re: [PATCH 2/3] drm/amdgpu: Replace TTM initialization/release with ttm_global

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 6041 bytes --]

Hi,

thanks for reviewing the patch.

Am 19.10.18 um 08:18 schrieb Zhang, Jerry(Junwei):
> On 10/19/2018 12:27 AM, Thomas Zimmermann wrote:
>>   /**
>>    * amdgpu_ttm_global_init - Initialize global TTM memory reference
>> structures.
>>    *
>> @@ -102,35 +75,15 @@ static void amdgpu_ttm_mem_global_release(struct
>> drm_global_reference *ref)
>>    */
>>   static int amdgpu_ttm_global_init(struct amdgpu_device *adev)
>>   {
>> -    struct drm_global_reference *global_ref;
>>       int r;
>>         /* ensure reference is false in case init fails */
>>       adev->mman.mem_global_referenced = false;
>>   -    global_ref = &adev->mman.mem_global_ref;
>> -    global_ref->global_type = DRM_GLOBAL_TTM_MEM;
>> -    global_ref->size = sizeof(struct ttm_mem_global);
>> -    global_ref->init = &amdgpu_ttm_mem_global_init;
>> -    global_ref->release = &amdgpu_ttm_mem_global_release;
>> -    r = drm_global_item_ref(global_ref);
>> +    r = ttm_global_init(&adev->mman.glob);
>>       if (r) {
>> -        DRM_ERROR("Failed setting up TTM memory accounting "
>> -              "subsystem.\n");
>> -        goto error_mem;
>> -    }
>> -
>> -    adev->mman.bo_global_ref.mem_glob =
>> -        adev->mman.mem_global_ref.object;
> 
> Seems to miss this action.

Should be fine. This was a workaround for setting up the global BO's
memory. It received the value that is here stored in
adev->mman.bo_global_ref.mem_glob.

Earlier this week, a patch set was merged into the TTM tree that cleans
up the init interfaces of ttm_bo_global. [1] There's now

  int ttm_bo_global_init(struct ttm_bo_global *glob,
			 struct ttm_mem_global *mem_glob)

which takes the the global memory as second argument. In patch [1/3] we
call ttm_bo_global_init() from ttm_global_init_bo() with the correct
global memory.

So this workaround is not needed any longer.

> Or are you going to replace
>       struct ttm_bo_global_ref bo_global_ref
> with
>       struct ttm_global glob ->  struct drm_global_reference bo_ref
> 
> if so, may need to remove ttm_bo_global_ref_init() and struct
> ttm_bo_global_ref at the same time.
> 
> 

I have a patch set for removing ttm_bo_global_ref and its interfaces,
but it has to wait until all drivers have been converted to struct
ttm_global. struct drm_global_reference will then become an
implementation detail of struct ttm_global and can be removed from DRM's
public interfaces.

Best regards
Thomas

[1]
https://cgit.freedesktop.org/~agd5f/linux/commit/?h=drm-next-4.21-wip&id=1fbb364ae523177bd0fac81d32befb3c9eb1b37e

> Regards,
> Jerry
> 
>> -    global_ref = &adev->mman.bo_global_ref.ref;
>> -    global_ref->global_type = DRM_GLOBAL_TTM_BO;
>> -    global_ref->size = sizeof(struct ttm_bo_global);
>> -    global_ref->init = &ttm_bo_global_ref_init;
>> -    global_ref->release = &ttm_bo_global_ref_release;
>> -    r = drm_global_item_ref(global_ref);
>> -    if (r) {
>> -        DRM_ERROR("Failed setting up TTM BO subsystem.\n");
>> -        goto error_bo;
>> +        DRM_ERROR("Failed setting up TTM subsystem.\n");
>> +        return r;
>>       }
>>         mutex_init(&adev->mman.gtt_window_lock);
>> @@ -138,19 +91,13 @@ static int amdgpu_ttm_global_init(struct
>> amdgpu_device *adev)
>>       adev->mman.mem_global_referenced = true;
>>         return 0;
>> -
>> -error_bo:
>> -    drm_global_item_unref(&adev->mman.mem_global_ref);
>> -error_mem:
>> -    return r;
>>   }
>>     static void amdgpu_ttm_global_fini(struct amdgpu_device *adev)
>>   {
>>       if (adev->mman.mem_global_referenced) {
>>           mutex_destroy(&adev->mman.gtt_window_lock);
>> -        drm_global_item_unref(&adev->mman.bo_global_ref.ref);
>> -        drm_global_item_unref(&adev->mman.mem_global_ref);
>> +        ttm_global_release(&adev->mman.glob);
>>           adev->mman.mem_global_referenced = false;
>>       }
>>   }
>> @@ -1765,7 +1712,7 @@ int amdgpu_ttm_init(struct amdgpu_device *adev)
>>       }
>>       /* No others user of address space so set it to 0 */
>>       r = ttm_bo_device_init(&adev->mman.bdev,
>> -                   adev->mman.bo_global_ref.ref.object,
>> +                   ttm_global_get_bo_global(&adev->mman.glob),
>>                      &amdgpu_bo_driver,
>>                      adev->ddev->anon_inode->i_mapping,
>>                      DRM_FILE_PAGE_OFFSET,
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>> index fe8f276e9811..c3a7fe3ead3a 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
>> @@ -26,6 +26,7 @@
>>     #include "amdgpu.h"
>>   #include <drm/gpu_scheduler.h>
>> +#include <drm/ttm/ttm_global.h>
>>     #define AMDGPU_PL_GDS        (TTM_PL_PRIV + 0)
>>   #define AMDGPU_PL_GWS        (TTM_PL_PRIV + 1)
>> @@ -39,8 +40,7 @@
>>   #define AMDGPU_GTT_NUM_TRANSFER_WINDOWS    2
>>     struct amdgpu_mman {
>> -    struct ttm_bo_global_ref        bo_global_ref;
>> -    struct drm_global_reference    mem_global_ref;
>> +    struct ttm_global        glob;
>>       struct ttm_bo_device        bdev;
>>       bool                mem_global_referenced;
>>       bool                initialized;
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstr. 5, D-90409 Nürnberg
Tel: +49-911-74053-0; Fax: +49-911-7417755;  https://www.suse.com/
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard,
Graham Norton, HRB 21284 (AG Nürnberg)


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

Re: [PATCH 1/3] drm/ttm: Provide struct ttm_global for referencing TTM global state

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 5994 bytes --]

Hi,

thanks for the review.

Am 19.10.18 um 05:30 schrieb Huang Rui:
> On Fri, Oct 19, 2018 at 12:27:50AM +0800, Thomas Zimmermann wrote:
>> +int ttm_global_init(struct ttm_global *glob)
>> +{
>> +	int ret;
>> +
> 
> We would better add a protection here to make sure the glob is not NULL.
> 
> if (!glob)
>         return -EINVAL;

Passing a NULL pointer is a programming error, not a runtime error. I'd
rather use BUG_ON() for this test. Ok?

Best regards
Thomas

> 
> Others, look good for me.
> 
> Thanks,
> Ray
> 
>> +	glob->mem_ref.global_type = DRM_GLOBAL_TTM_MEM;
>> +	glob->mem_ref.size = sizeof(struct ttm_mem_global);
>> +	glob->bo_ref.object = NULL;
>> +	glob->mem_ref.init = &ttm_global_init_mem;
>> +	glob->mem_ref.release = &ttm_global_release_mem;
>> +	ret = drm_global_item_ref(&glob->mem_ref);
>> +	if (ret)
>> +		return ret;
>> +
>> +	glob->bo_ref.global_type = DRM_GLOBAL_TTM_BO;
>> +	glob->bo_ref.size = sizeof(struct ttm_bo_global);
>> +	glob->bo_ref.object = NULL;
>> +	glob->bo_ref.init = &ttm_global_init_bo;
>> +	glob->bo_ref.release = &ttm_global_release_bo;
>> +	ret = drm_global_item_ref(&glob->bo_ref);
>> +	if (ret)
>> +		goto err_drm_global_item_unref_mem;
>> +
>> +	return 0;
>> +
>> +err_drm_global_item_unref_mem:
>> +	drm_global_item_unref(&glob->mem_ref);
>> +	return ret;
>> +}
>> +
>> +EXPORT_SYMBOL(ttm_global_init);
>> +
>> +void ttm_global_release(struct ttm_global *glob)
>> +{
>> +	drm_global_item_unref(&glob->bo_ref);
>> +	drm_global_item_unref(&glob->mem_ref);
>> +}
>> +
>> +EXPORT_SYMBOL(ttm_global_release);
>> diff --git a/include/drm/drm_global.h b/include/drm/drm_global.h
>> index 3a830602a2e4..4482a9bbd6e9 100644
>> --- a/include/drm/drm_global.h
>> +++ b/include/drm/drm_global.h
>> @@ -28,8 +28,16 @@
>>   * Authors: Thomas Hellstrom <thellstrom-at-vmware-dot-com>
>>   */
>>  
>> +/*
>> + *  The interfaces in this file are deprecated. Please use ttm_global
>> + *  from <drm/ttm/ttm_global.h> instead.
>> + */
>> +
>>  #ifndef _DRM_GLOBAL_H_
>>  #define _DRM_GLOBAL_H_
>> +
>> +#include <linux/types.h>
>> +
>>  enum drm_global_types {
>>  	DRM_GLOBAL_TTM_MEM = 0,
>>  	DRM_GLOBAL_TTM_BO,
>> diff --git a/include/drm/ttm/ttm_global.h b/include/drm/ttm/ttm_global.h
>> new file mode 100644
>> index 000000000000..06e791499f87
>> --- /dev/null
>> +++ b/include/drm/ttm/ttm_global.h
>> @@ -0,0 +1,79 @@
>> +/**************************************************************************
>> + *
>> + * Copyright 2008-2009 VMware, Inc., Palo Alto, CA., USA
>> + * All Rights Reserved.
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the
>> + * "Software"), to deal in the Software without restriction, including
>> + * without limitation the rights to use, copy, modify, merge, publish,
>> + * distribute, sub license, and/or sell copies of the Software, and to
>> + * permit persons to whom the Software is furnished to do so, subject to
>> + * the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including the
>> + * next paragraph) shall be included in all copies or substantial portions
>> + * of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
>> + * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM,
>> + * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
>> + * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
>> + * USE OR OTHER DEALINGS IN THE SOFTWARE.
>> + *
>> + **************************************************************************/
>> +
>> +#ifndef _TTM_GLOBAL_H_
>> +#define _TTM_GLOBAL_H_
>> +
>> +#include <drm/drm_global.h>
>> +
>> +struct ttm_bo_global;
>> +
>> +/**
>> + * struct ttm_global - Stores references to global TTM state
>> + */
>> +struct ttm_global {
>> +	struct drm_global_reference mem_ref;
>> +	struct drm_global_reference bo_ref;
>> +};
>> +
>> +/**
>> + * ttm_global_init
>> + *
>> + * @glob: A pointer to a struct ttm_global that is about to be initialized
>> + * @return Zero on success, or a negative error code otherwise.
>> + *
>> + * Initializes an instance of struct ttm_global with TTM's global state
>> + */
>> +int ttm_global_init(struct ttm_global *glob);
>> +
>> +/**
>> + * ttm_global_release
>> + *
>> + * @glob: A pointer to an instance of struct ttm_global
>> + *
>> + * Releases an initialized instance of struct ttm_global. If the instance
>> + * constains the final references to the global memory and BO, the global
>> + * structures are released as well.
>> + */
>> +void ttm_global_release(struct ttm_global *glob);
>> +
>> +/**
>> + * ttm_global_get_bo_global
>> + *
>> + * @glob A pointer to an instance of struct ttm_global
>> + * @return A refernce to the global BO.
>> + *
>> + * Returns the global BO. The BO should be forwarded to the initialization
>> + * of a driver's TTM BO device.
>> + */
>> +static inline struct ttm_bo_global*
>> +ttm_global_get_bo_global(struct ttm_global *glob)
>> +{
>> +	return glob->bo_ref.object;
>> +}
>> +
>> +#endif /* _TTM_GLOBAL_H_ */
>> -- 
>> 2.19.1
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstr. 5, D-90409 Nürnberg
Tel: +49-911-74053-0; Fax: +49-911-7417755;  https://www.suse.com/
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard,
Graham Norton, HRB 21284 (AG Nürnberg)


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

Re: [PATCH 0/3] Provide struct ttm_global for TTM global state

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 3337 bytes --]

Hi

Am 19.10.18 um 09:24 schrieb Koenig, Christian:
> Hi Thomas,
> 
> I honestly still find that way to complicated compared to what it 
> actually does.
> 
> Both ttm_mem_global and ttm_bo_global can just be some static variables. 
> E.g. the whole handling with drm_global_reference is just superfluous.
> 
> Can I just write a patch to clean up the mess we created?

'Can I?' I don't know :)

Sure, it's a mess. For example, if a driver inits drm_global_reference()
and is later unloaded, there are dangling function pointers to the
driver's code.

I do a slow clean-up of the code because TTM-based drivers depend on the
current way of doing things.

Maybe let me post my full patch set for RFC and discuss this instead.

Best regards
Thomas


> Regards,
> Christian.
> 
> Am 18.10.18 um 18:27 schrieb Thomas Zimmermann:
>> TTM provides global memory and a global BO that is shared by all
>> TTM-based drivers. The data structures are provided by struct drm_global
>> and its helpers. All TTM-based DRM drivers copy the initialization and
>> clean-up code for the global TTM state from each other; leading to code
>> duplication.
>>
>> The new structure struct ttm_global and its helpers provide a unified
>> implementation. Drivers only have to initialize it and forward the
>> contained BO global object to ttm_bo_device_init().
>>
>> The amdgpu and radeon drivers are converted to struct ttm_global as
>> part of this patch set. All other TTM-based drivers share exactly the
>> same code patterns and can be converted in the same way.
>>
>> Future directions: I already have patches for converting the remaining
>> TTM drivers to struct ttm_global. These patches can be merged after
>> the structure has become available in upstream. struct ttm_global is
>> implemented on top of struct drm_global. The latter actually maintains
>> TTM state instead of DRM state. Once all drivers have been converted,
>>      the code for struct drm_global can be merged into struct ttm_global.
>>
>> (Resending this patch set, as I forgot to CC the mailing lists as first.)
>>
>> Thomas Zimmermann (3):
>>    drm/ttm: Provide struct ttm_global for referencing TTM global state
>>    drm/amdgpu: Replace TTM initialization/release with ttm_global
>>    drm/radeon: Replace TTM initialization/release with ttm_global
>>
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 63 ++--------------
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h |  4 +-
>>   drivers/gpu/drm/radeon/radeon.h         |  4 +-
>>   drivers/gpu/drm/radeon/radeon_ttm.c     | 40 ++--------
>>   drivers/gpu/drm/ttm/Makefile            |  2 +-
>>   drivers/gpu/drm/ttm/ttm_global.c        | 98 +++++++++++++++++++++++++
>>   include/drm/drm_global.h                |  8 ++
>>   include/drm/ttm/ttm_global.h            | 79 ++++++++++++++++++++
>>   8 files changed, 200 insertions(+), 98 deletions(-)
>>   create mode 100644 drivers/gpu/drm/ttm/ttm_global.c
>>   create mode 100644 include/drm/ttm/ttm_global.h
>>
>> --
>> 2.19.1
>>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstr. 5, D-90409 Nürnberg
Tel: +49-911-74053-0; Fax: +49-911-7417755;  https://www.suse.com/
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard,
Graham Norton, HRB 21284 (AG Nürnberg)


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

Re: [PATCH 1/3] drm/ttm: Provide struct ttm_global for referencing TTM global state

[Daniel Vetter]

On Fri, Oct 19, 2018 at 9:38 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi,
>
> thanks for the review.
>
> Am 19.10.18 um 05:30 schrieb Huang Rui:
> > On Fri, Oct 19, 2018 at 12:27:50AM +0800, Thomas Zimmermann wrote:
> >> +int ttm_global_init(struct ttm_global *glob)
> >> +{
> >> +    int ret;
> >> +
> >
> > We would better add a protection here to make sure the glob is not NULL.
> >
> > if (!glob)
> >         return -EINVAL;
>
> Passing a NULL pointer is a programming error, not a runtime error. I'd
> rather use BUG_ON() for this test. Ok?

if (WARN_ON(!glob)
    return -EINVAL;

instead of BUG_ON is much friendly for debugging. Especially in gfx
drivers, where the entire init runs while holding a few critical
kernel locks (console_lock, among others), and so will result in a
very silent death of the entire machine.
-Daniel

>
> Best regards
> Thomas
>
> >
> > Others, look good for me.
> >
> > Thanks,
> > Ray
> >
> >> +    glob->mem_ref.global_type = DRM_GLOBAL_TTM_MEM;
> >> +    glob->mem_ref.size = sizeof(struct ttm_mem_global);
> >> +    glob->bo_ref.object = NULL;
> >> +    glob->mem_ref.init = &ttm_global_init_mem;
> >> +    glob->mem_ref.release = &ttm_global_release_mem;
> >> +    ret = drm_global_item_ref(&glob->mem_ref);
> >> +    if (ret)
> >> +            return ret;
> >> +
> >> +    glob->bo_ref.global_type = DRM_GLOBAL_TTM_BO;
> >> +    glob->bo_ref.size = sizeof(struct ttm_bo_global);
> >> +    glob->bo_ref.object = NULL;
> >> +    glob->bo_ref.init = &ttm_global_init_bo;
> >> +    glob->bo_ref.release = &ttm_global_release_bo;
> >> +    ret = drm_global_item_ref(&glob->bo_ref);
> >> +    if (ret)
> >> +            goto err_drm_global_item_unref_mem;
> >> +
> >> +    return 0;
> >> +
> >> +err_drm_global_item_unref_mem:
> >> +    drm_global_item_unref(&glob->mem_ref);
> >> +    return ret;
> >> +}
> >> +
> >> +EXPORT_SYMBOL(ttm_global_init);
> >> +
> >> +void ttm_global_release(struct ttm_global *glob)
> >> +{
> >> +    drm_global_item_unref(&glob->bo_ref);
> >> +    drm_global_item_unref(&glob->mem_ref);
> >> +}
> >> +
> >> +EXPORT_SYMBOL(ttm_global_release);
> >> diff --git a/include/drm/drm_global.h b/include/drm/drm_global.h
> >> index 3a830602a2e4..4482a9bbd6e9 100644
> >> --- a/include/drm/drm_global.h
> >> +++ b/include/drm/drm_global.h
> >> @@ -28,8 +28,16 @@
> >>   * Authors: Thomas Hellstrom <thellstrom-at-vmware-dot-com>
> >>   */
> >>
> >> +/*
> >> + *  The interfaces in this file are deprecated. Please use ttm_global
> >> + *  from <drm/ttm/ttm_global.h> instead.
> >> + */
> >> +
> >>  #ifndef _DRM_GLOBAL_H_
> >>  #define _DRM_GLOBAL_H_
> >> +
> >> +#include <linux/types.h>
> >> +
> >>  enum drm_global_types {
> >>      DRM_GLOBAL_TTM_MEM = 0,
> >>      DRM_GLOBAL_TTM_BO,
> >> diff --git a/include/drm/ttm/ttm_global.h b/include/drm/ttm/ttm_global.h
> >> new file mode 100644
> >> index 000000000000..06e791499f87
> >> --- /dev/null
> >> +++ b/include/drm/ttm/ttm_global.h
> >> @@ -0,0 +1,79 @@
> >> +/**************************************************************************
> >> + *
> >> + * Copyright 2008-2009 VMware, Inc., Palo Alto, CA., USA
> >> + * All Rights Reserved.
> >> + *
> >> + * Permission is hereby granted, free of charge, to any person obtaining a
> >> + * copy of this software and associated documentation files (the
> >> + * "Software"), to deal in the Software without restriction, including
> >> + * without limitation the rights to use, copy, modify, merge, publish,
> >> + * distribute, sub license, and/or sell copies of the Software, and to
> >> + * permit persons to whom the Software is furnished to do so, subject to
> >> + * the following conditions:
> >> + *
> >> + * The above copyright notice and this permission notice (including the
> >> + * next paragraph) shall be included in all copies or substantial portions
> >> + * of the Software.
> >> + *
> >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> >> + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
> >> + * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM,
> >> + * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> >> + * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
> >> + * USE OR OTHER DEALINGS IN THE SOFTWARE.
> >> + *
> >> + **************************************************************************/
> >> +
> >> +#ifndef _TTM_GLOBAL_H_
> >> +#define _TTM_GLOBAL_H_
> >> +
> >> +#include <drm/drm_global.h>
> >> +
> >> +struct ttm_bo_global;
> >> +
> >> +/**
> >> + * struct ttm_global - Stores references to global TTM state
> >> + */
> >> +struct ttm_global {
> >> +    struct drm_global_reference mem_ref;
> >> +    struct drm_global_reference bo_ref;
> >> +};
> >> +
> >> +/**
> >> + * ttm_global_init
> >> + *
> >> + * @glob: A pointer to a struct ttm_global that is about to be initialized
> >> + * @return Zero on success, or a negative error code otherwise.
> >> + *
> >> + * Initializes an instance of struct ttm_global with TTM's global state
> >> + */
> >> +int ttm_global_init(struct ttm_global *glob);
> >> +
> >> +/**
> >> + * ttm_global_release
> >> + *
> >> + * @glob: A pointer to an instance of struct ttm_global
> >> + *
> >> + * Releases an initialized instance of struct ttm_global. If the instance
> >> + * constains the final references to the global memory and BO, the global
> >> + * structures are released as well.
> >> + */
> >> +void ttm_global_release(struct ttm_global *glob);
> >> +
> >> +/**
> >> + * ttm_global_get_bo_global
> >> + *
> >> + * @glob A pointer to an instance of struct ttm_global
> >> + * @return A refernce to the global BO.
> >> + *
> >> + * Returns the global BO. The BO should be forwarded to the initialization
> >> + * of a driver's TTM BO device.
> >> + */
> >> +static inline struct ttm_bo_global*
> >> +ttm_global_get_bo_global(struct ttm_global *glob)
> >> +{
> >> +    return glob->bo_ref.object;
> >> +}
> >> +
> >> +#endif /* _TTM_GLOBAL_H_ */
> >> --
> >> 2.19.1
> >>
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Linux GmbH, Maxfeldstr. 5, D-90409 Nürnberg
> Tel: +49-911-74053-0; Fax: +49-911-7417755;  https://www.suse.com/
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard,
> Graham Norton, HRB 21284 (AG Nürnberg)
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/3] drm/ttm: Provide struct ttm_global for referencing TTM global state

[Koenig, Christian]

Am 19.10.18 um 09:49 schrieb Daniel Vetter:
> On Fri, Oct 19, 2018 at 9:38 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Hi,
>>
>> thanks for the review.
>>
>> Am 19.10.18 um 05:30 schrieb Huang Rui:
>>> On Fri, Oct 19, 2018 at 12:27:50AM +0800, Thomas Zimmermann wrote:
>>>> +int ttm_global_init(struct ttm_global *glob)
>>>> +{
>>>> +    int ret;
>>>> +
>>> We would better add a protection here to make sure the glob is not NULL.
>>>
>>> if (!glob)
>>>          return -EINVAL;
>> Passing a NULL pointer is a programming error, not a runtime error. I'd
>> rather use BUG_ON() for this test. Ok?
> if (WARN_ON(!glob)
>      return -EINVAL;
>
> instead of BUG_ON is much friendly for debugging. Especially in gfx
> drivers, where the entire init runs while holding a few critical
> kernel locks (console_lock, among others), and so will result in a
> very silent death of the entire machine.

Yeah, additional to that a BUG_ON a NULL pointer is rather pointless.

Dereferencing the NULL pointer has pretty much the same effect as a 
BUG_ON :)

Christian.

> -Daniel
>
>> Best regards
>> Thomas
>>
>>> Others, look good for me.
>>>
>>> Thanks,
>>> Ray
>>>
>>>> +    glob->mem_ref.global_type = DRM_GLOBAL_TTM_MEM;
>>>> +    glob->mem_ref.size = sizeof(struct ttm_mem_global);
>>>> +    glob->bo_ref.object = NULL;
>>>> +    glob->mem_ref.init = &ttm_global_init_mem;
>>>> +    glob->mem_ref.release = &ttm_global_release_mem;
>>>> +    ret = drm_global_item_ref(&glob->mem_ref);
>>>> +    if (ret)
>>>> +            return ret;
>>>> +
>>>> +    glob->bo_ref.global_type = DRM_GLOBAL_TTM_BO;
>>>> +    glob->bo_ref.size = sizeof(struct ttm_bo_global);
>>>> +    glob->bo_ref.object = NULL;
>>>> +    glob->bo_ref.init = &ttm_global_init_bo;
>>>> +    glob->bo_ref.release = &ttm_global_release_bo;
>>>> +    ret = drm_global_item_ref(&glob->bo_ref);
>>>> +    if (ret)
>>>> +            goto err_drm_global_item_unref_mem;
>>>> +
>>>> +    return 0;
>>>> +
>>>> +err_drm_global_item_unref_mem:
>>>> +    drm_global_item_unref(&glob->mem_ref);
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +EXPORT_SYMBOL(ttm_global_init);
>>>> +
>>>> +void ttm_global_release(struct ttm_global *glob)
>>>> +{
>>>> +    drm_global_item_unref(&glob->bo_ref);
>>>> +    drm_global_item_unref(&glob->mem_ref);
>>>> +}
>>>> +
>>>> +EXPORT_SYMBOL(ttm_global_release);
>>>> diff --git a/include/drm/drm_global.h b/include/drm/drm_global.h
>>>> index 3a830602a2e4..4482a9bbd6e9 100644
>>>> --- a/include/drm/drm_global.h
>>>> +++ b/include/drm/drm_global.h
>>>> @@ -28,8 +28,16 @@
>>>>    * Authors: Thomas Hellstrom <thellstrom-at-vmware-dot-com>
>>>>    */
>>>>
>>>> +/*
>>>> + *  The interfaces in this file are deprecated. Please use ttm_global
>>>> + *  from <drm/ttm/ttm_global.h> instead.
>>>> + */
>>>> +
>>>>   #ifndef _DRM_GLOBAL_H_
>>>>   #define _DRM_GLOBAL_H_
>>>> +
>>>> +#include <linux/types.h>
>>>> +
>>>>   enum drm_global_types {
>>>>       DRM_GLOBAL_TTM_MEM = 0,
>>>>       DRM_GLOBAL_TTM_BO,
>>>> diff --git a/include/drm/ttm/ttm_global.h b/include/drm/ttm/ttm_global.h
>>>> new file mode 100644
>>>> index 000000000000..06e791499f87
>>>> --- /dev/null
>>>> +++ b/include/drm/ttm/ttm_global.h
>>>> @@ -0,0 +1,79 @@
>>>> +/**************************************************************************
>>>> + *
>>>> + * Copyright 2008-2009 VMware, Inc., Palo Alto, CA., USA
>>>> + * All Rights Reserved.
>>>> + *
>>>> + * Permission is hereby granted, free of charge, to any person obtaining a
>>>> + * copy of this software and associated documentation files (the
>>>> + * "Software"), to deal in the Software without restriction, including
>>>> + * without limitation the rights to use, copy, modify, merge, publish,
>>>> + * distribute, sub license, and/or sell copies of the Software, and to
>>>> + * permit persons to whom the Software is furnished to do so, subject to
>>>> + * the following conditions:
>>>> + *
>>>> + * The above copyright notice and this permission notice (including the
>>>> + * next paragraph) shall be included in all copies or substantial portions
>>>> + * of the Software.
>>>> + *
>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
>>>> + * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM,
>>>> + * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
>>>> + * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
>>>> + * USE OR OTHER DEALINGS IN THE SOFTWARE.
>>>> + *
>>>> + **************************************************************************/
>>>> +
>>>> +#ifndef _TTM_GLOBAL_H_
>>>> +#define _TTM_GLOBAL_H_
>>>> +
>>>> +#include <drm/drm_global.h>
>>>> +
>>>> +struct ttm_bo_global;
>>>> +
>>>> +/**
>>>> + * struct ttm_global - Stores references to global TTM state
>>>> + */
>>>> +struct ttm_global {
>>>> +    struct drm_global_reference mem_ref;
>>>> +    struct drm_global_reference bo_ref;
>>>> +};
>>>> +
>>>> +/**
>>>> + * ttm_global_init
>>>> + *
>>>> + * @glob: A pointer to a struct ttm_global that is about to be initialized
>>>> + * @return Zero on success, or a negative error code otherwise.
>>>> + *
>>>> + * Initializes an instance of struct ttm_global with TTM's global state
>>>> + */
>>>> +int ttm_global_init(struct ttm_global *glob);
>>>> +
>>>> +/**
>>>> + * ttm_global_release
>>>> + *
>>>> + * @glob: A pointer to an instance of struct ttm_global
>>>> + *
>>>> + * Releases an initialized instance of struct ttm_global. If the instance
>>>> + * constains the final references to the global memory and BO, the global
>>>> + * structures are released as well.
>>>> + */
>>>> +void ttm_global_release(struct ttm_global *glob);
>>>> +
>>>> +/**
>>>> + * ttm_global_get_bo_global
>>>> + *
>>>> + * @glob A pointer to an instance of struct ttm_global
>>>> + * @return A refernce to the global BO.
>>>> + *
>>>> + * Returns the global BO. The BO should be forwarded to the initialization
>>>> + * of a driver's TTM BO device.
>>>> + */
>>>> +static inline struct ttm_bo_global*
>>>> +ttm_global_get_bo_global(struct ttm_global *glob)
>>>> +{
>>>> +    return glob->bo_ref.object;
>>>> +}
>>>> +
>>>> +#endif /* _TTM_GLOBAL_H_ */
>>>> --
>>>> 2.19.1
>>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Linux GmbH, Maxfeldstr. 5, D-90409 Nürnberg
>> Tel: +49-911-74053-0; Fax: +49-911-7417755;  https://www.suse.com/
>> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard,
>> Graham Norton, HRB 21284 (AG Nürnberg)
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>

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

Re: [PATCH 0/3] Provide struct ttm_global for TTM global state

[Koenig, Christian]

Am 19.10.18 um 09:44 schrieb Thomas Zimmermann:
> Hi
>
> Am 19.10.18 um 09:24 schrieb Koenig, Christian:
>> Hi Thomas,
>>
>> I honestly still find that way to complicated compared to what it
>> actually does.
>>
>> Both ttm_mem_global and ttm_bo_global can just be some static variables.
>> E.g. the whole handling with drm_global_reference is just superfluous.
>>
>> Can I just write a patch to clean up the mess we created?
> 'Can I?' I don't know :)
>
> Sure, it's a mess. For example, if a driver inits drm_global_reference()
> and is later unloaded, there are dangling function pointers to the
> driver's code.
>
> I do a slow clean-up of the code because TTM-based drivers depend on the
> current way of doing things.
>
> Maybe let me post my full patch set for RFC and discuss this instead.

Yeah, the key point is that this is a lot of stuff to review while the 
end result is we just trow away most of it.

I would rather just go ahead and do a few simple patches instead:
1. Use only a single instance of ttm_mem_global in ttm_memory.c and 
remove all "global" parameters from the functions.
2. Use only a single instance of ttm_bo_global in ttm_bo.c.
3. Remove bdev->global.

The *_global_init()/*_global_release() should just increase/decrease a 
static reference count protected by a static lock.

Regards,
Christian.

>
> Best regards
> Thomas
>
>
>> Regards,
>> Christian.
>>
>> Am 18.10.18 um 18:27 schrieb Thomas Zimmermann:
>>> TTM provides global memory and a global BO that is shared by all
>>> TTM-based drivers. The data structures are provided by struct drm_global
>>> and its helpers. All TTM-based DRM drivers copy the initialization and
>>> clean-up code for the global TTM state from each other; leading to code
>>> duplication.
>>>
>>> The new structure struct ttm_global and its helpers provide a unified
>>> implementation. Drivers only have to initialize it and forward the
>>> contained BO global object to ttm_bo_device_init().
>>>
>>> The amdgpu and radeon drivers are converted to struct ttm_global as
>>> part of this patch set. All other TTM-based drivers share exactly the
>>> same code patterns and can be converted in the same way.
>>>
>>> Future directions: I already have patches for converting the remaining
>>> TTM drivers to struct ttm_global. These patches can be merged after
>>> the structure has become available in upstream. struct ttm_global is
>>> implemented on top of struct drm_global. The latter actually maintains
>>> TTM state instead of DRM state. Once all drivers have been converted,
>>>       the code for struct drm_global can be merged into struct ttm_global.
>>>
>>> (Resending this patch set, as I forgot to CC the mailing lists as first.)
>>>
>>> Thomas Zimmermann (3):
>>>     drm/ttm: Provide struct ttm_global for referencing TTM global state
>>>     drm/amdgpu: Replace TTM initialization/release with ttm_global
>>>     drm/radeon: Replace TTM initialization/release with ttm_global
>>>
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 63 ++--------------
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h |  4 +-
>>>    drivers/gpu/drm/radeon/radeon.h         |  4 +-
>>>    drivers/gpu/drm/radeon/radeon_ttm.c     | 40 ++--------
>>>    drivers/gpu/drm/ttm/Makefile            |  2 +-
>>>    drivers/gpu/drm/ttm/ttm_global.c        | 98 +++++++++++++++++++++++++
>>>    include/drm/drm_global.h                |  8 ++
>>>    include/drm/ttm/ttm_global.h            | 79 ++++++++++++++++++++
>>>    8 files changed, 200 insertions(+), 98 deletions(-)
>>>    create mode 100644 drivers/gpu/drm/ttm/ttm_global.c
>>>    create mode 100644 include/drm/ttm/ttm_global.h
>>>
>>> --
>>> 2.19.1
>>>

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