Re: [PATCH 1/3] drm/ttm: split BO structure initialization into a separate function

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

 



On 14.02.2017 13:51, Christian König wrote:
Am 14.02.2017 um 13:00 schrieb Nicolai Hähnle:
On 14.02.2017 11:49, Christian König wrote:
Am 14.02.2017 um 11:37 schrieb Nicolai Hähnle:
From: Nicolai Hähnle <nicolai.haehnle@xxxxxxx>

Allow callers to opt out of calling ttm_bo_validate immediately. This
allows more flexibility in how locking of the reservation object is
done, which is needed to fix a locking bug (destroy locked mutex)
in amdgpu.

Signed-off-by: Nicolai Hähnle <nicolai.haehnle@xxxxxxx>

Please squash that into your other patch. It fixes another bug, but I
don't think fixing one bug just to run into another is really a good
idea.

I don't understand. I'm not aware that this patch fixes anything, it
just enables the subsequent fix in amdgpu in patch #2. I don't think
squashing those together is a good idea (one is in ttm, the other in
amdgpu).

Ok, forget it I've messed up the different reference count.

With at least initializing bo->kref and bo->destroy before returning the
first error the patch is Reviewed-by: Christian König
<christian.koenig@xxxxxxx>.

Thanks. Does this apply to patches #2 and #3 as well?

Cheers,
Nicolai



Regards,
Christian.



Additional to that one comment below.

---
  drivers/gpu/drm/ttm/ttm_bo.c | 62
+++++++++++++++++++++++++++++---------------
  include/drm/ttm/ttm_bo_api.h | 45 ++++++++++++++++++++++++++++++++
  2 files changed, 86 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c
b/drivers/gpu/drm/ttm/ttm_bo.c
index 76bee42..ce4c0f5 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -1120,41 +1120,30 @@ int ttm_bo_validate(struct ttm_buffer_object
*bo,
  }
  EXPORT_SYMBOL(ttm_bo_validate);
  -int ttm_bo_init(struct ttm_bo_device *bdev,
-        struct ttm_buffer_object *bo,
-        unsigned long size,
-        enum ttm_bo_type type,
-        struct ttm_placement *placement,
-        uint32_t page_alignment,
-        bool interruptible,
-        struct file *persistent_swap_storage,
-        size_t acc_size,
-        struct sg_table *sg,
-        struct reservation_object *resv,
-        void (*destroy) (struct ttm_buffer_object *))
+int ttm_bo_init_top(struct ttm_bo_device *bdev,
+            struct ttm_buffer_object *bo,
+            unsigned long size,
+            enum ttm_bo_type type,
+            uint32_t page_alignment,
+            struct file *persistent_swap_storage,
+            size_t acc_size,
+            struct sg_table *sg,
+            struct reservation_object *resv,
+            void (*destroy) (struct ttm_buffer_object *))
  {
      int ret = 0;
      unsigned long num_pages;
      struct ttm_mem_global *mem_glob = bdev->glob->mem_glob;
-    bool locked;
        ret = ttm_mem_global_alloc(mem_glob, acc_size, false, false);
      if (ret) {
          pr_err("Out of kernel memory\n");
-        if (destroy)
-            (*destroy)(bo);
-        else
-            kfree(bo);
          return -ENOMEM;
      }
        num_pages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
      if (num_pages == 0) {
          pr_err("Illegal buffer object size\n");
-        if (destroy)
-            (*destroy)(bo);
-        else
-            kfree(bo);
          ttm_mem_global_free(mem_glob, acc_size);
          return -EINVAL;
      }

I would move those checks after all the field initializations. This way
the structure has at least a valid content and we can safely use
ttm_bo_unref on it.

That feels odd to me, since the return value indicates that the buffer
wasn't properly initialized, but I don't feel strongly about it.

Cheers,
Nicolai



Regards,
Christian.

@@ -1204,6 +1193,37 @@ int ttm_bo_init(struct ttm_bo_device *bdev,
          ret = drm_vma_offset_add(&bdev->vma_manager, &bo->vma_node,
                       bo->mem.num_pages);
  +    return ret;
+}
+EXPORT_SYMBOL(ttm_bo_init_top);
+
+int ttm_bo_init(struct ttm_bo_device *bdev,
+        struct ttm_buffer_object *bo,
+        unsigned long size,
+        enum ttm_bo_type type,
+        struct ttm_placement *placement,
+        uint32_t page_alignment,
+        bool interruptible,
+        struct file *persistent_swap_storage,
+        size_t acc_size,
+        struct sg_table *sg,
+        struct reservation_object *resv,
+        void (*destroy) (struct ttm_buffer_object *))
+{
+    bool locked;
+    int ret;
+
+    ret = ttm_bo_init_top(bdev, bo, size, type, page_alignment,
+                  persistent_swap_storage, acc_size, sg, resv,
+                  destroy);
+    if (ret) {
+        if (destroy)
+            (*destroy)(bo);
+        else
+            kfree(bo);
+        return ret;
+    }
+
      /* passed reservation objects should already be locked,
       * since otherwise lockdep will be angered in radeon.
       */
diff --git a/include/drm/ttm/ttm_bo_api.h
b/include/drm/ttm/ttm_bo_api.h
index f195899..d44b8e4 100644
--- a/include/drm/ttm/ttm_bo_api.h
+++ b/include/drm/ttm/ttm_bo_api.h
@@ -453,6 +453,51 @@ size_t ttm_bo_dma_acc_size(struct ttm_bo_device
*bdev,
                 unsigned struct_size);
    /**
+ * ttm_bo_init_top
+ *
+ * @bdev: Pointer to a ttm_bo_device struct.
+ * @bo: Pointer to a ttm_buffer_object to be initialized.
+ * @size: Requested size of buffer object.
+ * @type: Requested type of buffer object.
+ * @flags: Initial placement flags.
+ * @page_alignment: Data alignment in pages.
+ * @persistent_swap_storage: Usually the swap storage is deleted for
buffers
+ * 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.
+ * @acc_size: Accounted size for this object.
+ * @resv: Pointer to a reservation_object, 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.
+ *
+ * Unlike ttm_bo_init, @bo is not validated, and when an error is
returned,
+ * the caller is responsible for freeing @bo (but the setup
performed by
+ * ttm_bo_init_top itself is cleaned up).
+ *
+ * On successful return, the object kref and list_kref are set to 1.
+ *
+ * Returns
+ * -ENOMEM: Out of memory.
+ * -EINVAL: Invalid buffer size.
+ */
+
+extern int ttm_bo_init_top(struct ttm_bo_device *bdev,
+               struct ttm_buffer_object *bo,
+               unsigned long size,
+               enum ttm_bo_type type,
+               uint32_t page_alignment,
+               struct file *persistent_swap_storage,
+               size_t acc_size,
+               struct sg_table *sg,
+               struct reservation_object *resv,
+               void (*destroy) (struct ttm_buffer_object *));
+
+/**
   * ttm_bo_init
   *
   * @bdev: Pointer to a ttm_bo_device struct.





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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux