Allow ttm_buffer_object without resource

Allow ttm_buffer_object without resource

[Christian König]

Hi guys,

this patch set cleans up the handling of TTM buffer objects quite a bit
by allowing to create them without allocating a ttm_resource as well.

That's not only cleaner in general, but also a necessary prerequisite for
quite a number of related work.

Please review and comment,
Christian.

[PATCH 01/11] drm/radeon: switch over to ttm_bo_init_reserved

[Christian König]

Use the new interface instead.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/radeon/radeon_object.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c
index b827b87aefe2..7afafbbc4ea0 100644
--- a/drivers/gpu/drm/radeon/radeon_object.c
+++ b/drivers/gpu/drm/radeon/radeon_object.c
@@ -133,9 +133,12 @@ int radeon_bo_create(struct radeon_device *rdev,
 		     struct dma_resv *resv,
 		     struct radeon_bo **bo_ptr)
 {
-	struct radeon_bo *bo;
-	enum ttm_bo_type type;
 	unsigned long page_align = roundup(byte_align, PAGE_SIZE) >> PAGE_SHIFT;
+
+	/* Kernel allocation are uninterruptible */
+	struct ttm_operation_ctx ctx = { !kernel, false };
+	enum ttm_bo_type type;
+	struct radeon_bo *bo;
 	int r;
 
 	size = ALIGN(size, PAGE_SIZE);
@@ -200,11 +203,13 @@ int radeon_bo_create(struct radeon_device *rdev,
 #endif
 
 	radeon_ttm_placement_from_domain(bo, domain);
-	/* Kernel allocation are uninterruptible */
 	down_read(&rdev->pm.mclk_lock);
-	r = ttm_bo_init(&rdev->mman.bdev, &bo->tbo, size, type,
-			&bo->placement, page_align, !kernel, sg, resv,
-			&radeon_ttm_bo_destroy);
+	r = ttm_bo_init_reserved(&rdev->mman.bdev, &bo->tbo, size, type,
+				 &bo->placement, page_align, &ctx, sg, resv,
+				 &radeon_ttm_bo_destroy);
+        if (!r)
+		ttm_bo_unreserve(&bo->tbo);
+
 	up_read(&rdev->pm.mclk_lock);
 	if (unlikely(r != 0)) {
 		return r;
-- 
2.25.1

[PATCH 02/11] drm/nouveau: switch over to ttm_bo_init_reserved

[Christian König]

Use the new interface instead.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/nouveau/nouveau_bo.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
index fa73fe57f97b..ceac591a7c01 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.c
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
@@ -302,19 +302,23 @@ nouveau_bo_init(struct nouveau_bo *nvbo, u64 size, int align, u32 domain,
 		struct sg_table *sg, struct dma_resv *robj)
 {
 	int type = sg ? ttm_bo_type_sg : ttm_bo_type_device;
+	struct ttm_operation_ctx ctx = { false, false };
 	int ret;
 
 	nouveau_bo_placement_set(nvbo, domain, 0);
 	INIT_LIST_HEAD(&nvbo->io_reserve_lru);
 
-	ret = ttm_bo_init(nvbo->bo.bdev, &nvbo->bo, size, type,
-			  &nvbo->placement, align >> PAGE_SHIFT, false, sg,
-			  robj, nouveau_bo_del_ttm);
+	ret = ttm_bo_init_reserved(nvbo->bo.bdev, &nvbo->bo, size, type,
+				   &nvbo->placement, align >> PAGE_SHIFT, &ctx,
+				   sg, robj, nouveau_bo_del_ttm);
 	if (ret) {
 		/* ttm will call nouveau_bo_del_ttm if it fails.. */
 		return ret;
 	}
 
+	if (!robj)
+		ttm_bo_unreserve(&nvbo->bo);
+
 	return 0;
 }
 
-- 
2.25.1

[PATCH 03/11] drm/vram-helper: switch over to ttm_bo_init_reserved

[Christian König]

Use the new interface instead.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/drm_gem_vram_helper.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index 3f00192215d1..0bd46a138ded 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -186,6 +186,7 @@ struct drm_gem_vram_object *drm_gem_vram_create(struct drm_device *dev,
 						size_t size,
 						unsigned long pg_align)
 {
+	struct ttm_operation_ctx ctx = { false, false };
 	struct drm_gem_vram_object *gbo;
 	struct drm_gem_object *gem;
 	struct drm_vram_mm *vmm = dev->vram_mm;
@@ -225,12 +226,13 @@ struct drm_gem_vram_object *drm_gem_vram_create(struct drm_device *dev,
 	 * A failing ttm_bo_init will call ttm_buffer_object_destroy
 	 * to release gbo->bo.base and kfree gbo.
 	 */
-	ret = ttm_bo_init(bdev, &gbo->bo, size, ttm_bo_type_device,
-			  &gbo->placement, pg_align, false, NULL, NULL,
-			  ttm_buffer_object_destroy);
-	if (ret)
+	ret = ttm_bo_init_reserved(bdev, &gbo->bo, size, ttm_bo_type_device,
+				   &gbo->placement, pg_align, &ctx, NULL, NULL,
+				   ttm_buffer_object_destroy);
+        if (ret)
 		return ERR_PTR(ret);
 
+	ttm_bo_unreserve(&gbo->bo);
 	return gbo;
 }
 EXPORT_SYMBOL(drm_gem_vram_create);
-- 
2.25.1

[PATCH 04/11] drm/ttm: move default BO destructor into VMWGFX

[Christian König]

It's the only driver using this.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c       |  9 +--------
 drivers/gpu/drm/vmwgfx/vmwgfx_bo.c | 11 ++++++++++-
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index e5fd0f2c0299..7598d59423bf 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -44,12 +44,6 @@
 
 #include "ttm_module.h"
 
-/* default destructor */
-static void ttm_bo_default_destroy(struct ttm_buffer_object *bo)
-{
-	kfree(bo);
-}
-
 static void ttm_bo_mem_space_debug(struct ttm_buffer_object *bo,
 					struct ttm_placement *placement)
 {
@@ -938,8 +932,7 @@ int ttm_bo_init_reserved(struct ttm_device *bdev,
 	bool locked;
 	int ret;
 
-	bo->destroy = destroy ? destroy : ttm_bo_default_destroy;
-
+	bo->destroy = destroy;
 	kref_init(&bo->kref);
 	INIT_LIST_HEAD(&bo->ddestroy);
 	bo->bdev = bdev;
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
index 31aecc46624b..60dcc6a75248 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
@@ -378,6 +378,12 @@ void vmw_bo_bo_free(struct ttm_buffer_object *bo)
 	kfree(vmw_bo);
 }
 
+/* default destructor */
+static void vmw_bo_default_destroy(struct ttm_buffer_object *bo)
+{
+	kfree(bo);
+}
+
 /**
  * vmw_bo_create_kernel - Create a pinned BO for internal kernel use.
  *
@@ -410,7 +416,7 @@ int vmw_bo_create_kernel(struct vmw_private *dev_priv, unsigned long size,
 
 	ret = ttm_bo_init_reserved(&dev_priv->bdev, bo, size,
 				   ttm_bo_type_kernel, placement, 0,
-				   &ctx, NULL, NULL, NULL);
+				   &ctx, NULL, NULL, vmw_bo_default_destroy);
 	if (unlikely(ret))
 		goto error_free;
 
@@ -439,6 +445,9 @@ int vmw_bo_create(struct vmw_private *vmw,
 		return -ENOMEM;
 	}
 
+	if (!bo_free)
+		bo_free = vmw_bo_default_destroy;
+
 	ret = vmw_bo_init(vmw, *p_bo, size,
 			  placement, interruptible, pin,
 			  bo_free);
-- 
2.25.1

Re: [PATCH 04/11] drm/ttm: move default BO destructor into VMWGFX

[Zack Rusin]

On Tue, 2022-03-29 at 13:02 +0200, Christian König wrote:
> ⚠ External Email
> 
> It's the only driver using this.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>

Looks good. A small suggestion underneath.

Reviewed-by: Zack Rusin <zackr@vmware.com>

> ---
>  drivers/gpu/drm/ttm/ttm_bo.c       |  9 +--------
>  drivers/gpu/drm/vmwgfx/vmwgfx_bo.c | 11 ++++++++++-
>  2 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c
> b/drivers/gpu/drm/ttm/ttm_bo.c
> index e5fd0f2c0299..7598d59423bf 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -44,12 +44,6 @@
> 
>  #include "ttm_module.h"
> 
> -/* default destructor */
> -static void ttm_bo_default_destroy(struct ttm_buffer_object *bo)
> -{
> -       kfree(bo);
> -}
> -
>  static void ttm_bo_mem_space_debug(struct ttm_buffer_object *bo,
>                                         struct ttm_placement
> *placement)
>  {
> @@ -938,8 +932,7 @@ int ttm_bo_init_reserved(struct ttm_device *bdev,
>         bool locked;
>         int ret;
> 
> -       bo->destroy = destroy ? destroy : ttm_bo_default_destroy;
> -
> +       bo->destroy = destroy;
>         kref_init(&bo->kref);
>         INIT_LIST_HEAD(&bo->ddestroy);
>         bo->bdev = bdev;
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> index 31aecc46624b..60dcc6a75248 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> @@ -378,6 +378,12 @@ void vmw_bo_bo_free(struct ttm_buffer_object
> *bo)
>         kfree(vmw_bo);
>  }
> 
> +/* default destructor */
> +static void vmw_bo_default_destroy(struct ttm_buffer_object *bo)
> +{
> +       kfree(bo);
> +}
> +
>  /**
>   * vmw_bo_create_kernel - Create a pinned BO for internal kernel
> use.
>   *
> @@ -410,7 +416,7 @@ int vmw_bo_create_kernel(struct vmw_private
> *dev_priv, unsigned long size,
> 
>         ret = ttm_bo_init_reserved(&dev_priv->bdev, bo, size,
>                                    ttm_bo_type_kernel, placement, 0,
> -                                  &ctx, NULL, NULL, NULL);
> +                                  &ctx, NULL, NULL,
> vmw_bo_default_destroy);
>         if (unlikely(ret))
>                 goto error_free;
> 
> @@ -439,6 +445,9 @@ int vmw_bo_create(struct vmw_private *vmw,
>                 return -ENOMEM;
>         }
> 
> +       if (!bo_free)
> +               bo_free = vmw_bo_default_destroy;
> +

If you could change this to just  BUG_ON(!bo_free) that'd be great.
bo_free == NULL should never happen here.

z

[PATCH 05/11] drm/ttm: drop ttm_bo_init

[Christian König]

Not used any more.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 26 ---------------------
 include/drm/ttm/ttm_bo_api.h | 44 ------------------------------------
 2 files changed, 70 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 7598d59423bf..37f68e710247 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -988,32 +988,6 @@ int ttm_bo_init_reserved(struct ttm_device *bdev,
 }
 EXPORT_SYMBOL(ttm_bo_init_reserved);
 
-int ttm_bo_init(struct ttm_device *bdev,
-		struct ttm_buffer_object *bo,
-		size_t size,
-		enum ttm_bo_type type,
-		struct ttm_placement *placement,
-		uint32_t page_alignment,
-		bool interruptible,
-		struct sg_table *sg,
-		struct dma_resv *resv,
-		void (*destroy) (struct ttm_buffer_object *))
-{
-	struct ttm_operation_ctx ctx = { interruptible, false };
-	int ret;
-
-	ret = ttm_bo_init_reserved(bdev, bo, size, type, placement,
-				   page_alignment, &ctx, sg, resv, destroy);
-	if (ret)
-		return ret;
-
-	if (!resv)
-		ttm_bo_unreserve(bo);
-
-	return 0;
-}
-EXPORT_SYMBOL(ttm_bo_init);
-
 /*
  * buffer object vm functions.
  */
diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
index c61e1e5ceb83..51fbbd035c11 100644
--- a/include/drm/ttm/ttm_bo_api.h
+++ b/include/drm/ttm/ttm_bo_api.h
@@ -363,50 +363,6 @@ int ttm_bo_init_reserved(struct ttm_device *bdev,
 			 struct sg_table *sg, struct dma_resv *resv,
 			 void (*destroy) (struct ttm_buffer_object *));
 
-/**
- * ttm_bo_init
- *
- * @bdev: Pointer to a ttm_device struct.
- * @bo: Pointer to a ttm_buffer_object to be initialized.
- * @size: Requested size of buffer object.
- * @type: Requested type of buffer object.
- * @placement: Initial placement for buffer object.
- * @page_alignment: Data alignment in pages.
- * @interruptible: If needing to sleep to wait for GPU resources,
- * sleep interruptible.
- * pinned in physical memory. If this behaviour is not desired, this member
- * holds a pointer to a persistent shmem object. Typically, this would
- * point to the shmem object backing a GEM object if TTM is used to back a
- * GEM user interface.
- * @sg: Scatter-gather table.
- * @resv: Pointer to a dma_resv, or NULL to let ttm allocate one.
- * @destroy: Destroy function. Use NULL for kfree().
- *
- * This function initializes a pre-allocated struct ttm_buffer_object.
- * As this object may be part of a larger structure, this function,
- * together with the @destroy function,
- * enables driver-specific objects derived from a ttm_buffer_object.
- *
- * On successful return, the caller owns an object kref to @bo. The kref and
- * list_kref are usually set to 1, but note that in some situations, other
- * tasks may already be holding references to @bo as well.
- *
- * If a failure occurs, the function will call the @destroy function, or
- * kfree() if @destroy is NULL. Thus, after a failure, dereferencing @bo is
- * illegal and will likely cause memory corruption.
- *
- * Returns
- * -ENOMEM: Out of memory.
- * -EINVAL: Invalid placement flags.
- * -ERESTARTSYS: Interrupted by signal while sleeping waiting for resources.
- */
-int ttm_bo_init(struct ttm_device *bdev, struct ttm_buffer_object *bo,
-		size_t size, enum ttm_bo_type type,
-		struct ttm_placement *placement,
-		uint32_t page_alignment, bool interrubtible,
-		struct sg_table *sg, struct dma_resv *resv,
-		void (*destroy) (struct ttm_buffer_object *));
-
 /**
  * ttm_kmap_obj_virtual
  *
-- 
2.25.1

[PATCH 06/11] drm/ttm: rename and cleanup ttm_bo_init_reserved

[Christian König]

Rename ttm_bo_init_reserved to ttm_bo_init_validate since that better
matches what the function is actually doing.

Remove the unused size parameter, move the function's kerneldoc to the
implementation and cleanup the whole error handling.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |  2 +-
 drivers/gpu/drm/drm_gem_vram_helper.c      |  2 +-
 drivers/gpu/drm/i915/gem/i915_gem_ttm.c    |  5 +-
 drivers/gpu/drm/nouveau/nouveau_bo.c       |  2 +-
 drivers/gpu/drm/qxl/qxl_object.c           |  2 +-
 drivers/gpu/drm/radeon/radeon_object.c     |  2 +-
 drivers/gpu/drm/ttm/ttm_bo.c               | 92 ++++++++++++++--------
 drivers/gpu/drm/vmwgfx/vmwgfx_bo.c         | 12 ++-
 include/drm/ttm/ttm_bo_api.h               | 44 +----------
 9 files changed, 75 insertions(+), 88 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index ea0cde4904f0..4cb3afbdf6b9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -587,7 +587,7 @@ int amdgpu_bo_create(struct amdgpu_device *adev,
 	if (!bp->destroy)
 		bp->destroy = &amdgpu_bo_destroy;
 
-	r = ttm_bo_init_reserved(&adev->mman.bdev, &bo->tbo, size, bp->type,
+	r = ttm_bo_init_validate(&adev->mman.bdev, &bo->tbo, bp->type,
 				 &bo->placement, page_align, &ctx,  NULL,
 				 bp->resv, bp->destroy);
 	if (unlikely(r != 0))
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index 0bd46a138ded..3e678d49265c 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -226,7 +226,7 @@ struct drm_gem_vram_object *drm_gem_vram_create(struct drm_device *dev,
 	 * A failing ttm_bo_init will call ttm_buffer_object_destroy
 	 * to release gbo->bo.base and kfree gbo.
 	 */
-	ret = ttm_bo_init_reserved(bdev, &gbo->bo, size, ttm_bo_type_device,
+	ret = ttm_bo_init_validate(bdev, &gbo->bo, ttm_bo_type_device,
 				   &gbo->placement, pg_align, &ctx, NULL, NULL,
 				   ttm_buffer_object_destroy);
         if (ret)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index 6fc192082d8c..10d89bde7dd3 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -1075,9 +1075,8 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
 	 * Similarly, in delayed_destroy, we can't call ttm_bo_put()
 	 * until successful initialization.
 	 */
-	ret = ttm_bo_init_reserved(&i915->bdev, i915_gem_to_ttm(obj), size,
-				   bo_type, &i915_sys_placement,
-				   page_size >> PAGE_SHIFT,
+	ret = ttm_bo_init_validate(&i915->bdev, i915_gem_to_ttm(obj), bo_type,
+				   &i915_sys_placement, page_size >> PAGE_SHIFT,
 				   &ctx, NULL, NULL, i915_ttm_bo_destroy);
 	if (ret)
 		return i915_ttm_err_to_gem(ret);
diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
index ceac591a7c01..43c862bbbbfb 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.c
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
@@ -308,7 +308,7 @@ nouveau_bo_init(struct nouveau_bo *nvbo, u64 size, int align, u32 domain,
 	nouveau_bo_placement_set(nvbo, domain, 0);
 	INIT_LIST_HEAD(&nvbo->io_reserve_lru);
 
-	ret = ttm_bo_init_reserved(nvbo->bo.bdev, &nvbo->bo, size, type,
+	ret = ttm_bo_init_validate(nvbo->bo.bdev, &nvbo->bo, type,
 				   &nvbo->placement, align >> PAGE_SHIFT, &ctx,
 				   sg, robj, nouveau_bo_del_ttm);
 	if (ret) {
diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
index fbb36e3e8564..e4dbce4afdd0 100644
--- a/drivers/gpu/drm/qxl/qxl_object.c
+++ b/drivers/gpu/drm/qxl/qxl_object.c
@@ -141,7 +141,7 @@ int qxl_bo_create(struct qxl_device *qdev, unsigned long size,
 	qxl_ttm_placement_from_domain(bo, domain);
 
 	bo->tbo.priority = priority;
-	r = ttm_bo_init_reserved(&qdev->mman.bdev, &bo->tbo, size, type,
+	r = ttm_bo_init_validate(&qdev->mman.bdev, &bo->tbo, type,
 				 &bo->placement, 0, &ctx, NULL, NULL,
 				 &qxl_ttm_bo_destroy);
 	if (unlikely(r != 0)) {
diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c
index 7afafbbc4ea0..9d255170b771 100644
--- a/drivers/gpu/drm/radeon/radeon_object.c
+++ b/drivers/gpu/drm/radeon/radeon_object.c
@@ -204,7 +204,7 @@ int radeon_bo_create(struct radeon_device *rdev,
 
 	radeon_ttm_placement_from_domain(bo, domain);
 	down_read(&rdev->pm.mclk_lock);
-	r = ttm_bo_init_reserved(&rdev->mman.bdev, &bo->tbo, size, type,
+	r = ttm_bo_init_validate(&rdev->mman.bdev, &bo->tbo, type,
 				 &bo->placement, page_align, &ctx, sg, resv,
 				 &radeon_ttm_bo_destroy);
         if (!r)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 37f68e710247..b7e259245f82 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -917,37 +917,62 @@ int ttm_bo_validate(struct ttm_buffer_object *bo,
 }
 EXPORT_SYMBOL(ttm_bo_validate);
 
-int ttm_bo_init_reserved(struct ttm_device *bdev,
-			 struct ttm_buffer_object *bo,
-			 size_t size,
-			 enum ttm_bo_type type,
-			 struct ttm_placement *placement,
-			 uint32_t page_alignment,
-			 struct ttm_operation_ctx *ctx,
-			 struct sg_table *sg,
-			 struct dma_resv *resv,
+/**
+ * ttm_bo_init_validate
+ *
+ * @bdev: Pointer to a ttm_device struct.
+ * @bo: Pointer to a ttm_buffer_object to be initialized.
+ * @type: Requested type of buffer object.
+ * @placement: Initial placement for buffer object.
+ * @alignment: Data alignment in pages.
+ * @ctx: TTM operation context for memory allocation.
+ * @sg: Scatter-gather table.
+ * @resv: Pointer to a dma_resv, or NULL to let ttm allocate one.
+ * @destroy: Destroy function. Use NULL for kfree().
+ *
+ * This function initializes a pre-allocated struct ttm_buffer_object.
+ * As this object may be part of a larger structure, this function,
+ * together with the @destroy function, enables driver-specific objects
+ * derived from a ttm_buffer_object.
+ *
+ * On successful return, the caller owns an object kref to @bo. The kref and
+ * list_kref are usually set to 1, but note that in some situations, other
+ * tasks may already be holding references to @bo as well.
+ * Furthermore, if resv == NULL, the buffer's reservation lock will be held,
+ * and it is the caller's responsibility to call ttm_bo_unreserve.
+ *
+ * If a failure occurs, the function will call the @destroy function. Thus,
+ * after a failure, dereferencing @bo is illegal and will likely cause memory
+ * corruption.
+ *
+ * Returns
+ * -ENOMEM: Out of memory.
+ * -EINVAL: Invalid placement flags.
+ * -ERESTARTSYS: Interrupted by signal while sleeping waiting for resources.
+ */
+int ttm_bo_init_validate(struct ttm_device *bdev, struct ttm_buffer_object *bo,
+			 enum ttm_bo_type type, struct ttm_placement *placement,
+			 uint32_t alignment, struct ttm_operation_ctx *ctx,
+			 struct sg_table *sg, struct dma_resv *resv,
 			 void (*destroy) (struct ttm_buffer_object *))
 {
 	static const struct ttm_place sys_mem = { .mem_type = TTM_PL_SYSTEM };
-	bool locked;
 	int ret;
 
-	bo->destroy = destroy;
 	kref_init(&bo->kref);
 	INIT_LIST_HEAD(&bo->ddestroy);
 	bo->bdev = bdev;
 	bo->type = type;
-	bo->page_alignment = page_alignment;
+	bo->page_alignment = alignment;
+	bo->destroy = destroy;
 	bo->moving = NULL;
 	bo->pin_count = 0;
 	bo->sg = sg;
 	bo->bulk_move = NULL;
-	if (resv) {
+	if (resv)
 		bo->base.resv = resv;
-		dma_resv_assert_held(bo->base.resv);
-	} else {
+	else
 		bo->base.resv = &bo->base._resv;
-	}
 	atomic_inc(&ttm_glob.bo_count);
 
 	ret = ttm_resource_alloc(bo, &sys_mem, &bo->resource);
@@ -960,33 +985,36 @@ int ttm_bo_init_reserved(struct ttm_device *bdev,
 	 * For ttm_bo_type_device buffers, allocate
 	 * address space from the device.
 	 */
-	if (bo->type == ttm_bo_type_device ||
-	    bo->type == ttm_bo_type_sg)
+	if (bo->type == ttm_bo_type_device || bo->type == ttm_bo_type_sg) {
 		ret = drm_vma_offset_add(bdev->vma_manager, &bo->base.vma_node,
-					 bo->resource->num_pages);
+					 PFN_UP(bo->base.size));
+		if (ret)
+			goto err_put;
+	}
 
 	/* passed reservation objects should already be locked,
 	 * since otherwise lockdep will be angered in radeon.
 	 */
-	if (!resv) {
-		locked = dma_resv_trylock(bo->base.resv);
-		WARN_ON(!locked);
-	}
+	if (!resv)
+		WARN_ON(!dma_resv_trylock(bo->base.resv));
+	else
+		dma_resv_assert_held(resv);
 
-	if (likely(!ret))
-		ret = ttm_bo_validate(bo, placement, ctx);
+	ret = ttm_bo_validate(bo, placement, ctx);
+	if (unlikely(ret))
+		goto err_unlock;
 
-	if (unlikely(ret)) {
-		if (!resv)
-			ttm_bo_unreserve(bo);
+	return 0;
 
-		ttm_bo_put(bo);
-		return ret;
-	}
+err_unlock:
+	if (!resv)
+		dma_resv_unlock(bo->base.resv);
 
+err_put:
+	ttm_bo_put(bo);
 	return ret;
 }
-EXPORT_SYMBOL(ttm_bo_init_reserved);
+EXPORT_SYMBOL(ttm_bo_init_validate);
 
 /*
  * buffer object vm functions.
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
index 60dcc6a75248..052a68408b85 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
@@ -414,9 +414,9 @@ int vmw_bo_create_kernel(struct vmw_private *dev_priv, unsigned long size,
 
 	drm_gem_private_object_init(vdev, &bo->base, size);
 
-	ret = ttm_bo_init_reserved(&dev_priv->bdev, bo, size,
-				   ttm_bo_type_kernel, placement, 0,
-				   &ctx, NULL, NULL, vmw_bo_default_destroy);
+	ret = ttm_bo_init_validate(&dev_priv->bdev, bo, ttm_bo_type_kernel,
+				   placement, 0, &ctx, NULL, NULL,
+				   vmw_bo_default_destroy);
 	if (unlikely(ret))
 		goto error_free;
 
@@ -498,10 +498,8 @@ int vmw_bo_init(struct vmw_private *dev_priv,
 	size = ALIGN(size, PAGE_SIZE);
 	drm_gem_private_object_init(vdev, &vmw_bo->base.base, size);
 
-	ret = ttm_bo_init_reserved(bdev, &vmw_bo->base, size,
-				   ttm_bo_type_device,
-				   placement,
-				   0, &ctx, NULL, NULL, bo_free);
+	ret = ttm_bo_init_validate(bdev, &vmw_bo->base, ttm_bo_type_device,
+				   placement, 0, &ctx, NULL, NULL, bo_free);
 	if (unlikely(ret)) {
 		return ret;
 	}
diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
index 51fbbd035c11..4b3bb6cfca39 100644
--- a/include/drm/ttm/ttm_bo_api.h
+++ b/include/drm/ttm/ttm_bo_api.h
@@ -319,47 +319,9 @@ void ttm_bo_unlock_delayed_workqueue(struct ttm_device *bdev, int resched);
 bool ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
 			      const struct ttm_place *place);
 
-/**
- * ttm_bo_init_reserved
- *
- * @bdev: Pointer to a ttm_device struct.
- * @bo: Pointer to a ttm_buffer_object to be initialized.
- * @size: Requested size of buffer object.
- * @type: Requested type of buffer object.
- * @placement: Initial placement for buffer object.
- * @page_alignment: Data alignment in pages.
- * @ctx: TTM operation context for memory allocation.
- * @sg: Scatter-gather table.
- * @resv: Pointer to a dma_resv, or NULL to let ttm allocate one.
- * @destroy: Destroy function. Use NULL for kfree().
- *
- * This function initializes a pre-allocated struct ttm_buffer_object.
- * As this object may be part of a larger structure, this function,
- * together with the @destroy function,
- * enables driver-specific objects derived from a ttm_buffer_object.
- *
- * On successful return, the caller owns an object kref to @bo. The kref and
- * list_kref are usually set to 1, but note that in some situations, other
- * tasks may already be holding references to @bo as well.
- * Furthermore, if resv == NULL, the buffer's reservation lock will be held,
- * and it is the caller's responsibility to call ttm_bo_unreserve.
- *
- * If a failure occurs, the function will call the @destroy function, or
- * kfree() if @destroy is NULL. Thus, after a failure, dereferencing @bo is
- * illegal and will likely cause memory corruption.
- *
- * Returns
- * -ENOMEM: Out of memory.
- * -EINVAL: Invalid placement flags.
- * -ERESTARTSYS: Interrupted by signal while sleeping waiting for resources.
- */
-
-int ttm_bo_init_reserved(struct ttm_device *bdev,
-			 struct ttm_buffer_object *bo,
-			 size_t size, enum ttm_bo_type type,
-			 struct ttm_placement *placement,
-			 uint32_t page_alignment,
-			 struct ttm_operation_ctx *ctx,
+int ttm_bo_init_validate(struct ttm_device *bdev, struct ttm_buffer_object *bo,
+			 enum ttm_bo_type type, struct ttm_placement *placement,
+			 uint32_t alignment, struct ttm_operation_ctx *ctx,
 			 struct sg_table *sg, struct dma_resv *resv,
 			 void (*destroy) (struct ttm_buffer_object *));
 
-- 
2.25.1

[PATCH 07/11] drm/amdgpu: audit bo->resource usage

[Christian König]

Make sure we can at least move and release BOs without backing store.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c    | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 4cb3afbdf6b9..49e8ba118322 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -1299,7 +1299,7 @@ void amdgpu_bo_release_notify(struct ttm_buffer_object *bo)
 	if (bo->base.resv == &bo->base._resv)
 		amdgpu_amdkfd_remove_fence_on_pt_pd_bos(abo);
 
-	if (bo->resource->mem_type != TTM_PL_VRAM ||
+	if (!bo->resource || bo->resource->mem_type != TTM_PL_VRAM ||
 	    !(abo->flags & AMDGPU_GEM_CREATE_VRAM_WIPE_ON_RELEASE))
 		return;
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 57ac118fc266..22954c84df39 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -476,7 +476,8 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
 
 	adev = amdgpu_ttm_adev(bo->bdev);
 
-	if (old_mem->mem_type == TTM_PL_SYSTEM && bo->ttm == NULL) {
+	if (!old_mem || (old_mem->mem_type == TTM_PL_SYSTEM &&
+			 bo->ttm == NULL)) {
 		ttm_bo_move_null(bo, new_mem);
 		goto out;
 	}
-- 
2.25.1

[PATCH 08/11] drm/nouveau: audit bo->resource usage

[Christian König]

Make sure we can at least move and release BOs without backing store.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/nouveau/nouveau_bo.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
index 43c862bbbbfb..5b937d68a291 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.c
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
@@ -1003,7 +1003,8 @@ nouveau_bo_move(struct ttm_buffer_object *bo, bool evict,
 	}
 
 	/* Fake bo copy. */
-	if (old_reg->mem_type == TTM_PL_SYSTEM && !bo->ttm) {
+	if (!old_reg || (old_reg->mem_type == TTM_PL_SYSTEM &&
+			 !bo->ttm)) {
 		ttm_bo_move_null(bo, new_reg);
 		goto out;
 	}
-- 
2.25.1

[PATCH 09/11] drm/ttm: audit bo->resource usage

[Christian König]

Allow BOs to exist without backing store.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index b7e259245f82..bd001fdde9fb 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -117,12 +117,13 @@ static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo,
 				  struct ttm_operation_ctx *ctx,
 				  struct ttm_place *hop)
 {
-	struct ttm_resource_manager *old_man, *new_man;
 	struct ttm_device *bdev = bo->bdev;
+	bool old_use_tt, new_use_tt;
 	int ret;
 
-	old_man = ttm_manager_type(bdev, bo->resource->mem_type);
-	new_man = ttm_manager_type(bdev, mem->mem_type);
+	old_use_tt = bo->resource &&
+		ttm_manager_type(bdev, bo->resource->mem_type)->use_tt;
+	new_use_tt = ttm_manager_type(bdev, mem->mem_type)->use_tt;
 
 	ttm_bo_unmap_virtual(bo);
 
@@ -130,11 +131,11 @@ static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo,
 	 * Create and bind a ttm if required.
 	 */
 
-	if (new_man->use_tt) {
+	if (new_use_tt) {
 		/* Zero init the new TTM structure if the old location should
 		 * have used one as well.
 		 */
-		ret = ttm_tt_create(bo, old_man->use_tt);
+		ret = ttm_tt_create(bo, old_use_tt);
 		if (ret)
 			goto out_err;
 
@@ -156,8 +157,7 @@ static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo,
 	return 0;
 
 out_err:
-	new_man = ttm_manager_type(bdev, bo->resource->mem_type);
-	if (!new_man->use_tt)
+	if (!old_use_tt)
 		ttm_bo_tt_destroy(bo);
 
 	return ret;
@@ -900,7 +900,7 @@ int ttm_bo_validate(struct ttm_buffer_object *bo,
 	/*
 	 * Check whether we need to move buffer.
 	 */
-	if (!ttm_resource_compat(bo->resource, placement)) {
+	if (!bo->resource || !ttm_resource_compat(bo->resource, placement)) {
 		ret = ttm_bo_move_buffer(bo, placement, ctx);
 		if (ret)
 			return ret;
-- 
2.25.1

[PATCH 10/11] drm/ttm: stop allocating dummy resources during BO creation

[Christian König]

That should not be necessary any more when drivers should at least be
able to handle the move without a resource.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index bd001fdde9fb..9d40641dcf39 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -956,7 +956,6 @@ int ttm_bo_init_validate(struct ttm_device *bdev, struct ttm_buffer_object *bo,
 			 struct sg_table *sg, struct dma_resv *resv,
 			 void (*destroy) (struct ttm_buffer_object *))
 {
-	static const struct ttm_place sys_mem = { .mem_type = TTM_PL_SYSTEM };
 	int ret;
 
 	kref_init(&bo->kref);
@@ -975,12 +974,6 @@ int ttm_bo_init_validate(struct ttm_device *bdev, struct ttm_buffer_object *bo,
 		bo->base.resv = &bo->base._resv;
 	atomic_inc(&ttm_glob.bo_count);
 
-	ret = ttm_resource_alloc(bo, &sys_mem, &bo->resource);
-	if (unlikely(ret)) {
-		ttm_bo_put(bo);
-		return ret;
-	}
-
 	/*
 	 * For ttm_bo_type_device buffers, allocate
 	 * address space from the device.
-- 
2.25.1

[PATCH 11/11] drm/ttm: stop allocating a dummy resource for pipelined gutting

[Christian König]

That should not be necessary any more when drivers should at least be
able to handle a move without a resource.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo_util.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
index 0163e92b57af..979abe095f42 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -585,16 +585,10 @@ EXPORT_SYMBOL(ttm_bo_move_accel_cleanup);
  */
 int ttm_bo_pipeline_gutting(struct ttm_buffer_object *bo)
 {
-	static const struct ttm_place sys_mem = { .mem_type = TTM_PL_SYSTEM };
 	struct ttm_buffer_object *ghost;
-	struct ttm_resource *sys_res;
 	struct ttm_tt *ttm;
 	int ret;
 
-	ret = ttm_resource_alloc(bo, &sys_mem, &sys_res);
-	if (ret)
-		return ret;
-
 	/* If already idle, no need for ghost object dance. */
 	ret = ttm_bo_wait(bo, false, true);
 	if (ret != -EBUSY) {
@@ -602,14 +596,13 @@ int ttm_bo_pipeline_gutting(struct ttm_buffer_object *bo)
 			/* See comment below about clearing. */
 			ret = ttm_tt_create(bo, true);
 			if (ret)
-				goto error_free_sys_mem;
+				return ret;
 		} else {
 			ttm_tt_unpopulate(bo->bdev, bo->ttm);
 			if (bo->type == ttm_bo_type_device)
 				ttm_tt_mark_for_clear(bo->ttm);
 		}
 		ttm_resource_free(bo, &bo->resource);
-		ttm_bo_assign_mem(bo, sys_res);
 		return 0;
 	}
 
@@ -626,7 +619,7 @@ int ttm_bo_pipeline_gutting(struct ttm_buffer_object *bo)
 	ret = ttm_tt_create(bo, true);
 	swap(bo->ttm, ttm);
 	if (ret)
-		goto error_free_sys_mem;
+		return ret;
 
 	ret = ttm_buffer_object_transfer(bo, &ghost);
 	if (ret)
@@ -640,13 +633,9 @@ int ttm_bo_pipeline_gutting(struct ttm_buffer_object *bo)
 	dma_resv_unlock(&ghost->base._resv);
 	ttm_bo_put(ghost);
 	bo->ttm = ttm;
-	ttm_bo_assign_mem(bo, sys_res);
 	return 0;
 
 error_destroy_tt:
 	ttm_tt_destroy(bo->bdev, ttm);
-
-error_free_sys_mem:
-	ttm_resource_free(bo, &sys_res);
 	return ret;
 }
-- 
2.25.1

Re: Allow ttm_buffer_object without resource

[Daniel Vetter]

On Tue, Mar 29, 2022 at 01:02:32PM +0200, Christian König wrote:
> Hi guys,
> 
> this patch set cleans up the handling of TTM buffer objects quite a bit
> by allowing to create them without allocating a ttm_resource as well.
> 
> That's not only cleaner in general, but also a necessary prerequisite for
> quite a number of related work.

Maybe there's some threads I missed, but I can't really guess what this
could be useful for without even a hint.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: Allow ttm_buffer_object without resource

[Christian König]

Am 29.03.22 um 16:02 schrieb Daniel Vetter:
> On Tue, Mar 29, 2022 at 01:02:32PM +0200, Christian König wrote:
>> Hi guys,
>>
>> this patch set cleans up the handling of TTM buffer objects quite a bit
>> by allowing to create them without allocating a ttm_resource as well.
>>
>> That's not only cleaner in general, but also a necessary prerequisite for
>> quite a number of related work.
> Maybe there's some threads I missed, but I can't really guess what this
> could be useful for without even a hint.

Well about a week ago Bob send me a partial implementation of this and 
said he needs it for i915. What exactly i915 needs here I'm not sure 
about either.

I have this cleanup in the pipeline because amdgpu wants to improve his 
page table handling with this and independent of those driver use case 
patches #10 and #11 drop allocating dummy resources for two use cases in 
TTM.

Christian.

> -Daniel

Improve TTMs empty object handling

[Christian König]

Hi everyone,

re-sending this because Daniel was requesting a background why this is
useful.

When TTM creates a buffer this object initially should not have any
backing store and there no resource object associated with it. The same
can happen when a driver requests that the backing store of an object is
destroyed without allocating a new one.

This is really useful during initial buffer creation as well as temporary
buffers and page tables which content doesn't need to be preserved when
they are evicted.

Currently TTM allocates dummy system resources for that because drivers
couldn't handle a NULL pointer there. Audit the drivers and then clean
up TTM to stop making those dummy allocations.

Please review and comment,
Christian.

[PATCH 06/11] drm/ttm: rename and cleanup ttm_bo_init_reserved

[Christian König]

Rename ttm_bo_init_reserved to ttm_bo_init_validate since that better
matches what the function is actually doing.

Remove the unused size parameter, move the function's kerneldoc to the
implementation and cleanup the whole error handling.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |  2 +-
 drivers/gpu/drm/drm_gem_vram_helper.c      |  2 +-
 drivers/gpu/drm/i915/gem/i915_gem_ttm.c    |  5 +-
 drivers/gpu/drm/nouveau/nouveau_bo.c       |  2 +-
 drivers/gpu/drm/qxl/qxl_object.c           |  2 +-
 drivers/gpu/drm/radeon/radeon_object.c     |  2 +-
 drivers/gpu/drm/ttm/ttm_bo.c               | 92 ++++++++++++++--------
 drivers/gpu/drm/vmwgfx/vmwgfx_bo.c         | 12 ++-
 include/drm/ttm/ttm_bo_api.h               | 44 +----------
 9 files changed, 75 insertions(+), 88 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 5444515c1476..116c8d31e646 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -590,7 +590,7 @@ int amdgpu_bo_create(struct amdgpu_device *adev,
 	if (!bp->destroy)
 		bp->destroy = &amdgpu_bo_destroy;
 
-	r = ttm_bo_init_reserved(&adev->mman.bdev, &bo->tbo, size, bp->type,
+	r = ttm_bo_init_validate(&adev->mman.bdev, &bo->tbo, bp->type,
 				 &bo->placement, page_align, &ctx,  NULL,
 				 bp->resv, bp->destroy);
 	if (unlikely(r != 0))
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index 7449cbc2f925..73e91baccea0 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -226,7 +226,7 @@ struct drm_gem_vram_object *drm_gem_vram_create(struct drm_device *dev,
 	 * A failing ttm_bo_init will call ttm_buffer_object_destroy
 	 * to release gbo->bo.base and kfree gbo.
 	 */
-	ret = ttm_bo_init_reserved(bdev, &gbo->bo, size, ttm_bo_type_device,
+	ret = ttm_bo_init_validate(bdev, &gbo->bo, ttm_bo_type_device,
 				   &gbo->placement, pg_align, &ctx, NULL, NULL,
 				   ttm_buffer_object_destroy);
         if (ret)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index 4c25d9b2f138..253188da91eb 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -1229,9 +1229,8 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
 	 * Similarly, in delayed_destroy, we can't call ttm_bo_put()
 	 * until successful initialization.
 	 */
-	ret = ttm_bo_init_reserved(&i915->bdev, i915_gem_to_ttm(obj), size,
-				   bo_type, &i915_sys_placement,
-				   page_size >> PAGE_SHIFT,
+	ret = ttm_bo_init_validate(&i915->bdev, i915_gem_to_ttm(obj), bo_type,
+				   &i915_sys_placement, page_size >> PAGE_SHIFT,
 				   &ctx, NULL, NULL, i915_ttm_bo_destroy);
 	if (ret)
 		return i915_ttm_err_to_gem(ret);
diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
index 858b9382036c..666941804297 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.c
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
@@ -308,7 +308,7 @@ nouveau_bo_init(struct nouveau_bo *nvbo, u64 size, int align, u32 domain,
 	nouveau_bo_placement_set(nvbo, domain, 0);
 	INIT_LIST_HEAD(&nvbo->io_reserve_lru);
 
-	ret = ttm_bo_init_reserved(nvbo->bo.bdev, &nvbo->bo, size, type,
+	ret = ttm_bo_init_validate(nvbo->bo.bdev, &nvbo->bo, type,
 				   &nvbo->placement, align >> PAGE_SHIFT, &ctx,
 				   sg, robj, nouveau_bo_del_ttm);
 	if (ret) {
diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
index b42a657e4c2f..da3f76f508ea 100644
--- a/drivers/gpu/drm/qxl/qxl_object.c
+++ b/drivers/gpu/drm/qxl/qxl_object.c
@@ -141,7 +141,7 @@ int qxl_bo_create(struct qxl_device *qdev, unsigned long size,
 	qxl_ttm_placement_from_domain(bo, domain);
 
 	bo->tbo.priority = priority;
-	r = ttm_bo_init_reserved(&qdev->mman.bdev, &bo->tbo, size, type,
+	r = ttm_bo_init_validate(&qdev->mman.bdev, &bo->tbo, type,
 				 &bo->placement, 0, &ctx, NULL, NULL,
 				 &qxl_ttm_bo_destroy);
 	if (unlikely(r != 0)) {
diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c
index 1d414ff4ab0c..550ca056b3ac 100644
--- a/drivers/gpu/drm/radeon/radeon_object.c
+++ b/drivers/gpu/drm/radeon/radeon_object.c
@@ -204,7 +204,7 @@ int radeon_bo_create(struct radeon_device *rdev,
 
 	radeon_ttm_placement_from_domain(bo, domain);
 	down_read(&rdev->pm.mclk_lock);
-	r = ttm_bo_init_reserved(&rdev->mman.bdev, &bo->tbo, size, type,
+	r = ttm_bo_init_validate(&rdev->mman.bdev, &bo->tbo, type,
 				 &bo->placement, page_align, &ctx, sg, resv,
 				 &radeon_ttm_bo_destroy);
         if (!r)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index e652055b5175..2b01cb30694a 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -915,36 +915,61 @@ int ttm_bo_validate(struct ttm_buffer_object *bo,
 }
 EXPORT_SYMBOL(ttm_bo_validate);
 
-int ttm_bo_init_reserved(struct ttm_device *bdev,
-			 struct ttm_buffer_object *bo,
-			 size_t size,
-			 enum ttm_bo_type type,
-			 struct ttm_placement *placement,
-			 uint32_t page_alignment,
-			 struct ttm_operation_ctx *ctx,
-			 struct sg_table *sg,
-			 struct dma_resv *resv,
+/**
+ * ttm_bo_init_validate
+ *
+ * @bdev: Pointer to a ttm_device struct.
+ * @bo: Pointer to a ttm_buffer_object to be initialized.
+ * @type: Requested type of buffer object.
+ * @placement: Initial placement for buffer object.
+ * @alignment: Data alignment in pages.
+ * @ctx: TTM operation context for memory allocation.
+ * @sg: Scatter-gather table.
+ * @resv: Pointer to a dma_resv, or NULL to let ttm allocate one.
+ * @destroy: Destroy function. Use NULL for kfree().
+ *
+ * This function initializes a pre-allocated struct ttm_buffer_object.
+ * As this object may be part of a larger structure, this function,
+ * together with the @destroy function, enables driver-specific objects
+ * derived from a ttm_buffer_object.
+ *
+ * On successful return, the caller owns an object kref to @bo. The kref and
+ * list_kref are usually set to 1, but note that in some situations, other
+ * tasks may already be holding references to @bo as well.
+ * Furthermore, if resv == NULL, the buffer's reservation lock will be held,
+ * and it is the caller's responsibility to call ttm_bo_unreserve.
+ *
+ * If a failure occurs, the function will call the @destroy function. Thus,
+ * after a failure, dereferencing @bo is illegal and will likely cause memory
+ * corruption.
+ *
+ * Returns
+ * -ENOMEM: Out of memory.
+ * -EINVAL: Invalid placement flags.
+ * -ERESTARTSYS: Interrupted by signal while sleeping waiting for resources.
+ */
+int ttm_bo_init_validate(struct ttm_device *bdev, struct ttm_buffer_object *bo,
+			 enum ttm_bo_type type, struct ttm_placement *placement,
+			 uint32_t alignment, struct ttm_operation_ctx *ctx,
+			 struct sg_table *sg, struct dma_resv *resv,
 			 void (*destroy) (struct ttm_buffer_object *))
 {
 	static const struct ttm_place sys_mem = { .mem_type = TTM_PL_SYSTEM };
-	bool locked;
 	int ret;
 
-	bo->destroy = destroy;
 	kref_init(&bo->kref);
 	INIT_LIST_HEAD(&bo->ddestroy);
 	bo->bdev = bdev;
 	bo->type = type;
-	bo->page_alignment = page_alignment;
+	bo->page_alignment = alignment;
+	bo->destroy = destroy;
 	bo->pin_count = 0;
 	bo->sg = sg;
 	bo->bulk_move = NULL;
-	if (resv) {
+	if (resv)
 		bo->base.resv = resv;
-		dma_resv_assert_held(bo->base.resv);
-	} else {
+	else
 		bo->base.resv = &bo->base._resv;
-	}
 	atomic_inc(&ttm_glob.bo_count);
 
 	ret = ttm_resource_alloc(bo, &sys_mem, &bo->resource);
@@ -957,33 +982,36 @@ int ttm_bo_init_reserved(struct ttm_device *bdev,
 	 * For ttm_bo_type_device buffers, allocate
 	 * address space from the device.
 	 */
-	if (bo->type == ttm_bo_type_device ||
-	    bo->type == ttm_bo_type_sg)
+	if (bo->type == ttm_bo_type_device || bo->type == ttm_bo_type_sg) {
 		ret = drm_vma_offset_add(bdev->vma_manager, &bo->base.vma_node,
-					 bo->resource->num_pages);
+					 PFN_UP(bo->base.size));
+		if (ret)
+			goto err_put;
+	}
 
 	/* passed reservation objects should already be locked,
 	 * since otherwise lockdep will be angered in radeon.
 	 */
-	if (!resv) {
-		locked = dma_resv_trylock(bo->base.resv);
-		WARN_ON(!locked);
-	}
+	if (!resv)
+		WARN_ON(!dma_resv_trylock(bo->base.resv));
+	else
+		dma_resv_assert_held(resv);
 
-	if (likely(!ret))
-		ret = ttm_bo_validate(bo, placement, ctx);
+	ret = ttm_bo_validate(bo, placement, ctx);
+	if (unlikely(ret))
+		goto err_unlock;
 
-	if (unlikely(ret)) {
-		if (!resv)
-			ttm_bo_unreserve(bo);
+	return 0;
 
-		ttm_bo_put(bo);
-		return ret;
-	}
+err_unlock:
+	if (!resv)
+		dma_resv_unlock(bo->base.resv);
 
+err_put:
+	ttm_bo_put(bo);
 	return ret;
 }
-EXPORT_SYMBOL(ttm_bo_init_reserved);
+EXPORT_SYMBOL(ttm_bo_init_validate);
 
 /*
  * buffer object vm functions.
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
index c4f376d5e1d0..2bda298e51f0 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
@@ -429,9 +429,9 @@ int vmw_bo_create_kernel(struct vmw_private *dev_priv, unsigned long size,
 
 	drm_gem_private_object_init(vdev, &bo->base, size);
 
-	ret = ttm_bo_init_reserved(&dev_priv->bdev, bo, size,
-				   ttm_bo_type_kernel, placement, 0,
-				   &ctx, NULL, NULL, vmw_bo_default_destroy);
+	ret = ttm_bo_init_validate(&dev_priv->bdev, bo, ttm_bo_type_kernel,
+				   placement, 0, &ctx, NULL, NULL,
+				   vmw_bo_default_destroy);
 	if (unlikely(ret))
 		goto error_free;
 
@@ -515,10 +515,8 @@ int vmw_bo_init(struct vmw_private *dev_priv,
 	size = ALIGN(size, PAGE_SIZE);
 	drm_gem_private_object_init(vdev, &vmw_bo->base.base, size);
 
-	ret = ttm_bo_init_reserved(bdev, &vmw_bo->base, size,
-				   ttm_bo_type_device,
-				   placement,
-				   0, &ctx, NULL, NULL, bo_free);
+	ret = ttm_bo_init_validate(bdev, &vmw_bo->base, ttm_bo_type_device,
+				   placement, 0, &ctx, NULL, NULL, bo_free);
 	if (unlikely(ret)) {
 		return ret;
 	}
diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
index 29384e2cb704..d3821a130d74 100644
--- a/include/drm/ttm/ttm_bo_api.h
+++ b/include/drm/ttm/ttm_bo_api.h
@@ -317,47 +317,9 @@ void ttm_bo_unlock_delayed_workqueue(struct ttm_device *bdev, int resched);
 bool ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
 			      const struct ttm_place *place);
 
-/**
- * ttm_bo_init_reserved
- *
- * @bdev: Pointer to a ttm_device struct.
- * @bo: Pointer to a ttm_buffer_object to be initialized.
- * @size: Requested size of buffer object.
- * @type: Requested type of buffer object.
- * @placement: Initial placement for buffer object.
- * @page_alignment: Data alignment in pages.
- * @ctx: TTM operation context for memory allocation.
- * @sg: Scatter-gather table.
- * @resv: Pointer to a dma_resv, or NULL to let ttm allocate one.
- * @destroy: Destroy function. Use NULL for kfree().
- *
- * This function initializes a pre-allocated struct ttm_buffer_object.
- * As this object may be part of a larger structure, this function,
- * together with the @destroy function,
- * enables driver-specific objects derived from a ttm_buffer_object.
- *
- * On successful return, the caller owns an object kref to @bo. The kref and
- * list_kref are usually set to 1, but note that in some situations, other
- * tasks may already be holding references to @bo as well.
- * Furthermore, if resv == NULL, the buffer's reservation lock will be held,
- * and it is the caller's responsibility to call ttm_bo_unreserve.
- *
- * If a failure occurs, the function will call the @destroy function, or
- * kfree() if @destroy is NULL. Thus, after a failure, dereferencing @bo is
- * illegal and will likely cause memory corruption.
- *
- * Returns
- * -ENOMEM: Out of memory.
- * -EINVAL: Invalid placement flags.
- * -ERESTARTSYS: Interrupted by signal while sleeping waiting for resources.
- */
-
-int ttm_bo_init_reserved(struct ttm_device *bdev,
-			 struct ttm_buffer_object *bo,
-			 size_t size, enum ttm_bo_type type,
-			 struct ttm_placement *placement,
-			 uint32_t page_alignment,
-			 struct ttm_operation_ctx *ctx,
+int ttm_bo_init_validate(struct ttm_device *bdev, struct ttm_buffer_object *bo,
+			 enum ttm_bo_type type, struct ttm_placement *placement,
+			 uint32_t alignment, struct ttm_operation_ctx *ctx,
 			 struct sg_table *sg, struct dma_resv *resv,
 			 void (*destroy) (struct ttm_buffer_object *));
 
-- 
2.25.1

(no subject)

[Christian König]

Just sending that out once more to intel-gfx to let the CI systems take
a look.

No functional change compared to the last version.

Christian.

[PATCH 06/11] drm/ttm: rename and cleanup ttm_bo_init_reserved

[Christian König]

Rename ttm_bo_init_reserved to ttm_bo_init_validate since that better
matches what the function is actually doing.

Remove the unused size parameter, move the function's kerneldoc to the
implementation and cleanup the whole error handling.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |  2 +-
 drivers/gpu/drm/drm_gem_vram_helper.c      |  2 +-
 drivers/gpu/drm/i915/gem/i915_gem_ttm.c    |  5 +-
 drivers/gpu/drm/nouveau/nouveau_bo.c       |  2 +-
 drivers/gpu/drm/qxl/qxl_object.c           |  2 +-
 drivers/gpu/drm/radeon/radeon_object.c     |  2 +-
 drivers/gpu/drm/ttm/ttm_bo.c               | 92 ++++++++++++++--------
 drivers/gpu/drm/vmwgfx/vmwgfx_bo.c         | 12 ++-
 include/drm/ttm/ttm_bo_api.h               | 44 +----------
 9 files changed, 75 insertions(+), 88 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 5444515c1476..116c8d31e646 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -590,7 +590,7 @@ int amdgpu_bo_create(struct amdgpu_device *adev,
 	if (!bp->destroy)
 		bp->destroy = &amdgpu_bo_destroy;
 
-	r = ttm_bo_init_reserved(&adev->mman.bdev, &bo->tbo, size, bp->type,
+	r = ttm_bo_init_validate(&adev->mman.bdev, &bo->tbo, bp->type,
 				 &bo->placement, page_align, &ctx,  NULL,
 				 bp->resv, bp->destroy);
 	if (unlikely(r != 0))
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index 7449cbc2f925..73e91baccea0 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -226,7 +226,7 @@ struct drm_gem_vram_object *drm_gem_vram_create(struct drm_device *dev,
 	 * A failing ttm_bo_init will call ttm_buffer_object_destroy
 	 * to release gbo->bo.base and kfree gbo.
 	 */
-	ret = ttm_bo_init_reserved(bdev, &gbo->bo, size, ttm_bo_type_device,
+	ret = ttm_bo_init_validate(bdev, &gbo->bo, ttm_bo_type_device,
 				   &gbo->placement, pg_align, &ctx, NULL, NULL,
 				   ttm_buffer_object_destroy);
         if (ret)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index 4c25d9b2f138..253188da91eb 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -1229,9 +1229,8 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem,
 	 * Similarly, in delayed_destroy, we can't call ttm_bo_put()
 	 * until successful initialization.
 	 */
-	ret = ttm_bo_init_reserved(&i915->bdev, i915_gem_to_ttm(obj), size,
-				   bo_type, &i915_sys_placement,
-				   page_size >> PAGE_SHIFT,
+	ret = ttm_bo_init_validate(&i915->bdev, i915_gem_to_ttm(obj), bo_type,
+				   &i915_sys_placement, page_size >> PAGE_SHIFT,
 				   &ctx, NULL, NULL, i915_ttm_bo_destroy);
 	if (ret)
 		return i915_ttm_err_to_gem(ret);
diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
index 858b9382036c..666941804297 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.c
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
@@ -308,7 +308,7 @@ nouveau_bo_init(struct nouveau_bo *nvbo, u64 size, int align, u32 domain,
 	nouveau_bo_placement_set(nvbo, domain, 0);
 	INIT_LIST_HEAD(&nvbo->io_reserve_lru);
 
-	ret = ttm_bo_init_reserved(nvbo->bo.bdev, &nvbo->bo, size, type,
+	ret = ttm_bo_init_validate(nvbo->bo.bdev, &nvbo->bo, type,
 				   &nvbo->placement, align >> PAGE_SHIFT, &ctx,
 				   sg, robj, nouveau_bo_del_ttm);
 	if (ret) {
diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
index b42a657e4c2f..da3f76f508ea 100644
--- a/drivers/gpu/drm/qxl/qxl_object.c
+++ b/drivers/gpu/drm/qxl/qxl_object.c
@@ -141,7 +141,7 @@ int qxl_bo_create(struct qxl_device *qdev, unsigned long size,
 	qxl_ttm_placement_from_domain(bo, domain);
 
 	bo->tbo.priority = priority;
-	r = ttm_bo_init_reserved(&qdev->mman.bdev, &bo->tbo, size, type,
+	r = ttm_bo_init_validate(&qdev->mman.bdev, &bo->tbo, type,
 				 &bo->placement, 0, &ctx, NULL, NULL,
 				 &qxl_ttm_bo_destroy);
 	if (unlikely(r != 0)) {
diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c
index 1d414ff4ab0c..550ca056b3ac 100644
--- a/drivers/gpu/drm/radeon/radeon_object.c
+++ b/drivers/gpu/drm/radeon/radeon_object.c
@@ -204,7 +204,7 @@ int radeon_bo_create(struct radeon_device *rdev,
 
 	radeon_ttm_placement_from_domain(bo, domain);
 	down_read(&rdev->pm.mclk_lock);
-	r = ttm_bo_init_reserved(&rdev->mman.bdev, &bo->tbo, size, type,
+	r = ttm_bo_init_validate(&rdev->mman.bdev, &bo->tbo, type,
 				 &bo->placement, page_align, &ctx, sg, resv,
 				 &radeon_ttm_bo_destroy);
         if (!r)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index e652055b5175..2b01cb30694a 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -915,36 +915,61 @@ int ttm_bo_validate(struct ttm_buffer_object *bo,
 }
 EXPORT_SYMBOL(ttm_bo_validate);
 
-int ttm_bo_init_reserved(struct ttm_device *bdev,
-			 struct ttm_buffer_object *bo,
-			 size_t size,
-			 enum ttm_bo_type type,
-			 struct ttm_placement *placement,
-			 uint32_t page_alignment,
-			 struct ttm_operation_ctx *ctx,
-			 struct sg_table *sg,
-			 struct dma_resv *resv,
+/**
+ * ttm_bo_init_validate
+ *
+ * @bdev: Pointer to a ttm_device struct.
+ * @bo: Pointer to a ttm_buffer_object to be initialized.
+ * @type: Requested type of buffer object.
+ * @placement: Initial placement for buffer object.
+ * @alignment: Data alignment in pages.
+ * @ctx: TTM operation context for memory allocation.
+ * @sg: Scatter-gather table.
+ * @resv: Pointer to a dma_resv, or NULL to let ttm allocate one.
+ * @destroy: Destroy function. Use NULL for kfree().
+ *
+ * This function initializes a pre-allocated struct ttm_buffer_object.
+ * As this object may be part of a larger structure, this function,
+ * together with the @destroy function, enables driver-specific objects
+ * derived from a ttm_buffer_object.
+ *
+ * On successful return, the caller owns an object kref to @bo. The kref and
+ * list_kref are usually set to 1, but note that in some situations, other
+ * tasks may already be holding references to @bo as well.
+ * Furthermore, if resv == NULL, the buffer's reservation lock will be held,
+ * and it is the caller's responsibility to call ttm_bo_unreserve.
+ *
+ * If a failure occurs, the function will call the @destroy function. Thus,
+ * after a failure, dereferencing @bo is illegal and will likely cause memory
+ * corruption.
+ *
+ * Returns
+ * -ENOMEM: Out of memory.
+ * -EINVAL: Invalid placement flags.
+ * -ERESTARTSYS: Interrupted by signal while sleeping waiting for resources.
+ */
+int ttm_bo_init_validate(struct ttm_device *bdev, struct ttm_buffer_object *bo,
+			 enum ttm_bo_type type, struct ttm_placement *placement,
+			 uint32_t alignment, struct ttm_operation_ctx *ctx,
+			 struct sg_table *sg, struct dma_resv *resv,
 			 void (*destroy) (struct ttm_buffer_object *))
 {
 	static const struct ttm_place sys_mem = { .mem_type = TTM_PL_SYSTEM };
-	bool locked;
 	int ret;
 
-	bo->destroy = destroy;
 	kref_init(&bo->kref);
 	INIT_LIST_HEAD(&bo->ddestroy);
 	bo->bdev = bdev;
 	bo->type = type;
-	bo->page_alignment = page_alignment;
+	bo->page_alignment = alignment;
+	bo->destroy = destroy;
 	bo->pin_count = 0;
 	bo->sg = sg;
 	bo->bulk_move = NULL;
-	if (resv) {
+	if (resv)
 		bo->base.resv = resv;
-		dma_resv_assert_held(bo->base.resv);
-	} else {
+	else
 		bo->base.resv = &bo->base._resv;
-	}
 	atomic_inc(&ttm_glob.bo_count);
 
 	ret = ttm_resource_alloc(bo, &sys_mem, &bo->resource);
@@ -957,33 +982,36 @@ int ttm_bo_init_reserved(struct ttm_device *bdev,
 	 * For ttm_bo_type_device buffers, allocate
 	 * address space from the device.
 	 */
-	if (bo->type == ttm_bo_type_device ||
-	    bo->type == ttm_bo_type_sg)
+	if (bo->type == ttm_bo_type_device || bo->type == ttm_bo_type_sg) {
 		ret = drm_vma_offset_add(bdev->vma_manager, &bo->base.vma_node,
-					 bo->resource->num_pages);
+					 PFN_UP(bo->base.size));
+		if (ret)
+			goto err_put;
+	}
 
 	/* passed reservation objects should already be locked,
 	 * since otherwise lockdep will be angered in radeon.
 	 */
-	if (!resv) {
-		locked = dma_resv_trylock(bo->base.resv);
-		WARN_ON(!locked);
-	}
+	if (!resv)
+		WARN_ON(!dma_resv_trylock(bo->base.resv));
+	else
+		dma_resv_assert_held(resv);
 
-	if (likely(!ret))
-		ret = ttm_bo_validate(bo, placement, ctx);
+	ret = ttm_bo_validate(bo, placement, ctx);
+	if (unlikely(ret))
+		goto err_unlock;
 
-	if (unlikely(ret)) {
-		if (!resv)
-			ttm_bo_unreserve(bo);
+	return 0;
 
-		ttm_bo_put(bo);
-		return ret;
-	}
+err_unlock:
+	if (!resv)
+		dma_resv_unlock(bo->base.resv);
 
+err_put:
+	ttm_bo_put(bo);
 	return ret;
 }
-EXPORT_SYMBOL(ttm_bo_init_reserved);
+EXPORT_SYMBOL(ttm_bo_init_validate);
 
 /*
  * buffer object vm functions.
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
index c4f376d5e1d0..2bda298e51f0 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
@@ -429,9 +429,9 @@ int vmw_bo_create_kernel(struct vmw_private *dev_priv, unsigned long size,
 
 	drm_gem_private_object_init(vdev, &bo->base, size);
 
-	ret = ttm_bo_init_reserved(&dev_priv->bdev, bo, size,
-				   ttm_bo_type_kernel, placement, 0,
-				   &ctx, NULL, NULL, vmw_bo_default_destroy);
+	ret = ttm_bo_init_validate(&dev_priv->bdev, bo, ttm_bo_type_kernel,
+				   placement, 0, &ctx, NULL, NULL,
+				   vmw_bo_default_destroy);
 	if (unlikely(ret))
 		goto error_free;
 
@@ -515,10 +515,8 @@ int vmw_bo_init(struct vmw_private *dev_priv,
 	size = ALIGN(size, PAGE_SIZE);
 	drm_gem_private_object_init(vdev, &vmw_bo->base.base, size);
 
-	ret = ttm_bo_init_reserved(bdev, &vmw_bo->base, size,
-				   ttm_bo_type_device,
-				   placement,
-				   0, &ctx, NULL, NULL, bo_free);
+	ret = ttm_bo_init_validate(bdev, &vmw_bo->base, ttm_bo_type_device,
+				   placement, 0, &ctx, NULL, NULL, bo_free);
 	if (unlikely(ret)) {
 		return ret;
 	}
diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
index 29384e2cb704..d3821a130d74 100644
--- a/include/drm/ttm/ttm_bo_api.h
+++ b/include/drm/ttm/ttm_bo_api.h
@@ -317,47 +317,9 @@ void ttm_bo_unlock_delayed_workqueue(struct ttm_device *bdev, int resched);
 bool ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
 			      const struct ttm_place *place);
 
-/**
- * ttm_bo_init_reserved
- *
- * @bdev: Pointer to a ttm_device struct.
- * @bo: Pointer to a ttm_buffer_object to be initialized.
- * @size: Requested size of buffer object.
- * @type: Requested type of buffer object.
- * @placement: Initial placement for buffer object.
- * @page_alignment: Data alignment in pages.
- * @ctx: TTM operation context for memory allocation.
- * @sg: Scatter-gather table.
- * @resv: Pointer to a dma_resv, or NULL to let ttm allocate one.
- * @destroy: Destroy function. Use NULL for kfree().
- *
- * This function initializes a pre-allocated struct ttm_buffer_object.
- * As this object may be part of a larger structure, this function,
- * together with the @destroy function,
- * enables driver-specific objects derived from a ttm_buffer_object.
- *
- * On successful return, the caller owns an object kref to @bo. The kref and
- * list_kref are usually set to 1, but note that in some situations, other
- * tasks may already be holding references to @bo as well.
- * Furthermore, if resv == NULL, the buffer's reservation lock will be held,
- * and it is the caller's responsibility to call ttm_bo_unreserve.
- *
- * If a failure occurs, the function will call the @destroy function, or
- * kfree() if @destroy is NULL. Thus, after a failure, dereferencing @bo is
- * illegal and will likely cause memory corruption.
- *
- * Returns
- * -ENOMEM: Out of memory.
- * -EINVAL: Invalid placement flags.
- * -ERESTARTSYS: Interrupted by signal while sleeping waiting for resources.
- */
-
-int ttm_bo_init_reserved(struct ttm_device *bdev,
-			 struct ttm_buffer_object *bo,
-			 size_t size, enum ttm_bo_type type,
-			 struct ttm_placement *placement,
-			 uint32_t page_alignment,
-			 struct ttm_operation_ctx *ctx,
+int ttm_bo_init_validate(struct ttm_device *bdev, struct ttm_buffer_object *bo,
+			 enum ttm_bo_type type, struct ttm_placement *placement,
+			 uint32_t alignment, struct ttm_operation_ctx *ctx,
 			 struct sg_table *sg, struct dma_resv *resv,
 			 void (*destroy) (struct ttm_buffer_object *));
 
-- 
2.25.1

RE: [PATCH 06/11] drm/ttm: rename and cleanup ttm_bo_init_reserved

[Ruhl, Michael J]

>-----Original Message-----
>From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
>Christian König
>Sent: Thursday, May 19, 2022 5:55 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: matthew.william.auld@gmail.com; Christian König
><christian.koenig@amd.com>; dri-devel@lists.freedesktop.org
>Subject: [PATCH 06/11] drm/ttm: rename and cleanup ttm_bo_init_reserved
>
>Rename ttm_bo_init_reserved to ttm_bo_init_validate since that better
>matches what the function is actually doing.
>
>Remove the unused size parameter, move the function's kerneldoc to the
>implementation and cleanup the whole error handling.
>
>Signed-off-by: Christian König <christian.koenig@amd.com>
>---
> drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |  2 +-
> drivers/gpu/drm/drm_gem_vram_helper.c      |  2 +-
> drivers/gpu/drm/i915/gem/i915_gem_ttm.c    |  5 +-
> drivers/gpu/drm/nouveau/nouveau_bo.c       |  2 +-
> drivers/gpu/drm/qxl/qxl_object.c           |  2 +-
> drivers/gpu/drm/radeon/radeon_object.c     |  2 +-
> drivers/gpu/drm/ttm/ttm_bo.c               | 92 ++++++++++++++--------
> drivers/gpu/drm/vmwgfx/vmwgfx_bo.c         | 12 ++-
> include/drm/ttm/ttm_bo_api.h               | 44 +----------
> 9 files changed, 75 insertions(+), 88 deletions(-)
>
>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>index 5444515c1476..116c8d31e646 100644
>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>@@ -590,7 +590,7 @@ int amdgpu_bo_create(struct amdgpu_device *adev,
> 	if (!bp->destroy)
> 		bp->destroy = &amdgpu_bo_destroy;
>
>-	r = ttm_bo_init_reserved(&adev->mman.bdev, &bo->tbo, size, bp-
>>type,
>+	r = ttm_bo_init_validate(&adev->mman.bdev, &bo->tbo, bp->type,
> 				 &bo->placement, page_align, &ctx,  NULL,
> 				 bp->resv, bp->destroy);
> 	if (unlikely(r != 0))
>diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c
>b/drivers/gpu/drm/drm_gem_vram_helper.c
>index 7449cbc2f925..73e91baccea0 100644
>--- a/drivers/gpu/drm/drm_gem_vram_helper.c
>+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
>@@ -226,7 +226,7 @@ struct drm_gem_vram_object
>*drm_gem_vram_create(struct drm_device *dev,
> 	 * A failing ttm_bo_init will call ttm_buffer_object_destroy
> 	 * to release gbo->bo.base and kfree gbo.
> 	 */
>-	ret = ttm_bo_init_reserved(bdev, &gbo->bo, size,
>ttm_bo_type_device,
>+	ret = ttm_bo_init_validate(bdev, &gbo->bo, ttm_bo_type_device,
> 				   &gbo->placement, pg_align, &ctx, NULL,
>NULL,
> 				   ttm_buffer_object_destroy);
>         if (ret)
>diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>index 4c25d9b2f138..253188da91eb 100644
>--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
>@@ -1229,9 +1229,8 @@ int __i915_gem_ttm_object_init(struct
>intel_memory_region *mem,
> 	 * Similarly, in delayed_destroy, we can't call ttm_bo_put()
> 	 * until successful initialization.
> 	 */
>-	ret = ttm_bo_init_reserved(&i915->bdev, i915_gem_to_ttm(obj),
>size,
>-				   bo_type, &i915_sys_placement,
>-				   page_size >> PAGE_SHIFT,
>+	ret = ttm_bo_init_validate(&i915->bdev, i915_gem_to_ttm(obj),
>bo_type,
>+				   &i915_sys_placement, page_size >>
>PAGE_SHIFT,
> 				   &ctx, NULL, NULL, i915_ttm_bo_destroy);
> 	if (ret)
> 		return i915_ttm_err_to_gem(ret);
>diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c
>b/drivers/gpu/drm/nouveau/nouveau_bo.c
>index 858b9382036c..666941804297 100644
>--- a/drivers/gpu/drm/nouveau/nouveau_bo.c
>+++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
>@@ -308,7 +308,7 @@ nouveau_bo_init(struct nouveau_bo *nvbo, u64 size,
>int align, u32 domain,
> 	nouveau_bo_placement_set(nvbo, domain, 0);
> 	INIT_LIST_HEAD(&nvbo->io_reserve_lru);
>
>-	ret = ttm_bo_init_reserved(nvbo->bo.bdev, &nvbo->bo, size, type,
>+	ret = ttm_bo_init_validate(nvbo->bo.bdev, &nvbo->bo, type,
> 				   &nvbo->placement, align >> PAGE_SHIFT,
>&ctx,
> 				   sg, robj, nouveau_bo_del_ttm);
> 	if (ret) {
>diff --git a/drivers/gpu/drm/qxl/qxl_object.c
>b/drivers/gpu/drm/qxl/qxl_object.c
>index b42a657e4c2f..da3f76f508ea 100644
>--- a/drivers/gpu/drm/qxl/qxl_object.c
>+++ b/drivers/gpu/drm/qxl/qxl_object.c
>@@ -141,7 +141,7 @@ int qxl_bo_create(struct qxl_device *qdev, unsigned
>long size,
> 	qxl_ttm_placement_from_domain(bo, domain);
>
> 	bo->tbo.priority = priority;
>-	r = ttm_bo_init_reserved(&qdev->mman.bdev, &bo->tbo, size, type,
>+	r = ttm_bo_init_validate(&qdev->mman.bdev, &bo->tbo, type,
> 				 &bo->placement, 0, &ctx, NULL, NULL,
> 				 &qxl_ttm_bo_destroy);
> 	if (unlikely(r != 0)) {
>diff --git a/drivers/gpu/drm/radeon/radeon_object.c
>b/drivers/gpu/drm/radeon/radeon_object.c
>index 1d414ff4ab0c..550ca056b3ac 100644
>--- a/drivers/gpu/drm/radeon/radeon_object.c
>+++ b/drivers/gpu/drm/radeon/radeon_object.c
>@@ -204,7 +204,7 @@ int radeon_bo_create(struct radeon_device *rdev,
>
> 	radeon_ttm_placement_from_domain(bo, domain);
> 	down_read(&rdev->pm.mclk_lock);
>-	r = ttm_bo_init_reserved(&rdev->mman.bdev, &bo->tbo, size, type,
>+	r = ttm_bo_init_validate(&rdev->mman.bdev, &bo->tbo, type,
> 				 &bo->placement, page_align, &ctx, sg, resv,
> 				 &radeon_ttm_bo_destroy);
>         if (!r)
>diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
>index e652055b5175..2b01cb30694a 100644
>--- a/drivers/gpu/drm/ttm/ttm_bo.c
>+++ b/drivers/gpu/drm/ttm/ttm_bo.c
>@@ -915,36 +915,61 @@ int ttm_bo_validate(struct ttm_buffer_object *bo,
> }
> EXPORT_SYMBOL(ttm_bo_validate);
>
>-int ttm_bo_init_reserved(struct ttm_device *bdev,
>-			 struct ttm_buffer_object *bo,
>-			 size_t size,
>-			 enum ttm_bo_type type,
>-			 struct ttm_placement *placement,
>-			 uint32_t page_alignment,
>-			 struct ttm_operation_ctx *ctx,
>-			 struct sg_table *sg,
>-			 struct dma_resv *resv,
>+/**
>+ * ttm_bo_init_validate
>+ *
>+ * @bdev: Pointer to a ttm_device struct.
>+ * @bo: Pointer to a ttm_buffer_object to be initialized.
>+ * @type: Requested type of buffer object.
>+ * @placement: Initial placement for buffer object.
>+ * @alignment: Data alignment in pages.
>+ * @ctx: TTM operation context for memory allocation.
>+ * @sg: Scatter-gather table.
>+ * @resv: Pointer to a dma_resv, or NULL to let ttm allocate one.
>+ * @destroy: Destroy function. Use NULL for kfree().
>+ *
>+ * This function initializes a pre-allocated struct ttm_buffer_object.
>+ * As this object may be part of a larger structure, this function,
>+ * together with the @destroy function, enables driver-specific objects
>+ * derived from a ttm_buffer_object.
>+ *
>+ * On successful return, the caller owns an object kref to @bo. The kref and
>+ * list_kref are usually set to 1, but note that in some situations, other
>+ * tasks may already be holding references to @bo as well.
>+ * Furthermore, if resv == NULL, the buffer's reservation lock will be held,
>+ * and it is the caller's responsibility to call ttm_bo_unreserve.

Ahh, ok, I see why the first couple of patches are doing this unreserve.

Umm still not sure why the caller needs to be responsible for this.  Can you
explain the reason?

>+ *
>+ * If a failure occurs, the function will call the @destroy function. Thus,
>+ * after a failure, dereferencing @bo is illegal and will likely cause memory
>+ * corruption.
>+ *
>+ * Returns
>+ * -ENOMEM: Out of memory.
>+ * -EINVAL: Invalid placement flags.
>+ * -ERESTARTSYS: Interrupted by signal while sleeping waiting for resources.
>+ */
>+int ttm_bo_init_validate(struct ttm_device *bdev, struct ttm_buffer_object
>*bo,
>+			 enum ttm_bo_type type, struct ttm_placement
>*placement,
>+			 uint32_t alignment, struct ttm_operation_ctx *ctx,
>+			 struct sg_table *sg, struct dma_resv *resv,
> 			 void (*destroy) (struct ttm_buffer_object *))
> {
> 	static const struct ttm_place sys_mem = { .mem_type =
>TTM_PL_SYSTEM };
>-	bool locked;
> 	int ret;
>
>-	bo->destroy = destroy;
> 	kref_init(&bo->kref);
> 	INIT_LIST_HEAD(&bo->ddestroy);
> 	bo->bdev = bdev;
> 	bo->type = type;
>-	bo->page_alignment = page_alignment;
>+	bo->page_alignment = alignment;
>+	bo->destroy = destroy;
> 	bo->pin_count = 0;
> 	bo->sg = sg;
> 	bo->bulk_move = NULL;
>-	if (resv) {
>+	if (resv)
> 		bo->base.resv = resv;
>-		dma_resv_assert_held(bo->base.resv);
>-	} else {
>+	else
> 		bo->base.resv = &bo->base._resv;
>-	}
> 	atomic_inc(&ttm_glob.bo_count);
>
> 	ret = ttm_resource_alloc(bo, &sys_mem, &bo->resource);
>@@ -957,33 +982,36 @@ int ttm_bo_init_reserved(struct ttm_device *bdev,
> 	 * For ttm_bo_type_device buffers, allocate
> 	 * address space from the device.
> 	 */
>-	if (bo->type == ttm_bo_type_device ||
>-	    bo->type == ttm_bo_type_sg)
>+	if (bo->type == ttm_bo_type_device || bo->type ==
>ttm_bo_type_sg) {
> 		ret = drm_vma_offset_add(bdev->vma_manager, &bo-
>>base.vma_node,
>-					 bo->resource->num_pages);
>+					 PFN_UP(bo->base.size));
>+		if (ret)
>+			goto err_put;
>+	}
>
> 	/* passed reservation objects should already be locked,
> 	 * since otherwise lockdep will be angered in radeon.
> 	 */
>-	if (!resv) {
>-		locked = dma_resv_trylock(bo->base.resv);
>-		WARN_ON(!locked);
>-	}
>+	if (!resv)
>+		WARN_ON(!dma_resv_trylock(bo->base.resv));
>+	else
>+		dma_resv_assert_held(resv);
>
>-	if (likely(!ret))
>-		ret = ttm_bo_validate(bo, placement, ctx);
>+	ret = ttm_bo_validate(bo, placement, ctx);
>+	if (unlikely(ret))
>+		goto err_unlock;
>
>-	if (unlikely(ret)) {
>-		if (!resv)
>-			ttm_bo_unreserve(bo);
>+	return 0;
>
>-		ttm_bo_put(bo);
>-		return ret;
>-	}
>+err_unlock:
>+	if (!resv)
>+		dma_resv_unlock(bo->base.resv);

The original error path does a ttm_bo_unreserve, not an unlock.

Was that a bug in the original code?

Mike

>
>+err_put:
>+	ttm_bo_put(bo);
> 	return ret;
> }
>-EXPORT_SYMBOL(ttm_bo_init_reserved);
>+EXPORT_SYMBOL(ttm_bo_init_validate);
>
> /*
>  * buffer object vm functions.
>diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
>b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
>index c4f376d5e1d0..2bda298e51f0 100644
>--- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
>+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
>@@ -429,9 +429,9 @@ int vmw_bo_create_kernel(struct vmw_private
>*dev_priv, unsigned long size,
>
> 	drm_gem_private_object_init(vdev, &bo->base, size);
>
>-	ret = ttm_bo_init_reserved(&dev_priv->bdev, bo, size,
>-				   ttm_bo_type_kernel, placement, 0,
>-				   &ctx, NULL, NULL,
>vmw_bo_default_destroy);
>+	ret = ttm_bo_init_validate(&dev_priv->bdev, bo,
>ttm_bo_type_kernel,
>+				   placement, 0, &ctx, NULL, NULL,
>+				   vmw_bo_default_destroy);
> 	if (unlikely(ret))
> 		goto error_free;
>
>@@ -515,10 +515,8 @@ int vmw_bo_init(struct vmw_private *dev_priv,
> 	size = ALIGN(size, PAGE_SIZE);
> 	drm_gem_private_object_init(vdev, &vmw_bo->base.base, size);
>
>-	ret = ttm_bo_init_reserved(bdev, &vmw_bo->base, size,
>-				   ttm_bo_type_device,
>-				   placement,
>-				   0, &ctx, NULL, NULL, bo_free);
>+	ret = ttm_bo_init_validate(bdev, &vmw_bo->base,
>ttm_bo_type_device,
>+				   placement, 0, &ctx, NULL, NULL, bo_free);
> 	if (unlikely(ret)) {
> 		return ret;
> 	}
>diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
>index 29384e2cb704..d3821a130d74 100644
>--- a/include/drm/ttm/ttm_bo_api.h
>+++ b/include/drm/ttm/ttm_bo_api.h
>@@ -317,47 +317,9 @@ void ttm_bo_unlock_delayed_workqueue(struct
>ttm_device *bdev, int resched);
> bool ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
> 			      const struct ttm_place *place);
>
>-/**
>- * ttm_bo_init_reserved
>- *
>- * @bdev: Pointer to a ttm_device struct.
>- * @bo: Pointer to a ttm_buffer_object to be initialized.
>- * @size: Requested size of buffer object.
>- * @type: Requested type of buffer object.
>- * @placement: Initial placement for buffer object.
>- * @page_alignment: Data alignment in pages.
>- * @ctx: TTM operation context for memory allocation.
>- * @sg: Scatter-gather table.
>- * @resv: Pointer to a dma_resv, or NULL to let ttm allocate one.
>- * @destroy: Destroy function. Use NULL for kfree().
>- *
>- * This function initializes a pre-allocated struct ttm_buffer_object.
>- * As this object may be part of a larger structure, this function,
>- * together with the @destroy function,
>- * enables driver-specific objects derived from a ttm_buffer_object.
>- *
>- * On successful return, the caller owns an object kref to @bo. The kref and
>- * list_kref are usually set to 1, but note that in some situations, other
>- * tasks may already be holding references to @bo as well.
>- * Furthermore, if resv == NULL, the buffer's reservation lock will be held,
>- * and it is the caller's responsibility to call ttm_bo_unreserve.
>- *
>- * If a failure occurs, the function will call the @destroy function, or
>- * kfree() if @destroy is NULL. Thus, after a failure, dereferencing @bo is
>- * illegal and will likely cause memory corruption.
>- *
>- * Returns
>- * -ENOMEM: Out of memory.
>- * -EINVAL: Invalid placement flags.
>- * -ERESTARTSYS: Interrupted by signal while sleeping waiting for resources.
>- */
>-
>-int ttm_bo_init_reserved(struct ttm_device *bdev,
>-			 struct ttm_buffer_object *bo,
>-			 size_t size, enum ttm_bo_type type,
>-			 struct ttm_placement *placement,
>-			 uint32_t page_alignment,
>-			 struct ttm_operation_ctx *ctx,
>+int ttm_bo_init_validate(struct ttm_device *bdev, struct ttm_buffer_object
>*bo,
>+			 enum ttm_bo_type type, struct ttm_placement
>*placement,
>+			 uint32_t alignment, struct ttm_operation_ctx *ctx,
> 			 struct sg_table *sg, struct dma_resv *resv,
> 			 void (*destroy) (struct ttm_buffer_object *));
>
>--
>2.25.1