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

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

2017-02-15 Thread Nicolai Hähnle


On 15.02.2017 14:35, Nicolai Hähnle wrote:

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 

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 


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
.


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


Well, there's some minor necessary rebase fixes, so I'll probably just 
send out a new version once I got to test it.


Nicolai

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

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

2017-02-15 Thread Nicolai Hähnle


On 15.02.2017 04:16, zhoucm1 wrote:



On 2017年02月14日 18:37, Nicolai Hähnle wrote:

From: Nicolai Hähnle 

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 
---
  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;
  }
@@ -1204,6 +1193,37 @@ int ttm_bo_init(struct ttm_bo_device *bdev,
  ret = drm_vma_offset_add(>vma_manager, >vma_node,
   bo->mem.num_pages);

if (ret && !resv), we should call
reservation_object_fini(>ttm_resv), right?


FWIW, you were right about this (and also mutex_destroy needs to be 
called for wu_mutex, etc.). But I'm following Christian's suggestion of 
having the caller use ttm_bo_unref for error cleanup, so all this error 
cleanup needn't be duplicated.


Cheers,
Nicola
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

2017-02-15 Thread Nicolai Hähnle


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 

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 


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
.


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(>vma_manager, >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

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

2017-02-15 Thread zhoucm1




On 2017年02月15日 18:43, Nicolai Hähnle wrote:

On 15.02.2017 04:16, zhoucm1 wrote:

On 2017年02月14日 18:37, Nicolai Hähnle wrote:

From: Nicolai Hähnle 

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 
---
  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;
  }
@@ -1204,6 +1193,37 @@ int ttm_bo_init(struct ttm_bo_device *bdev,
  ret = drm_vma_offset_add(>vma_manager, >vma_node,
   bo->mem.num_pages);

if (ret && !resv), we should call
reservation_object_fini(>ttm_resv), right?


Good point, though actually, ret can never be != 0 here, so this can 
be simplified a bit.




  +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;
+

Can we lock resv anyway before ttm_bo_init_top like what you did in
patch #3? if yes, seems we don't need patch#3 any more, right?


if (!resv) {
bool locked;

reservation_object_init(>tbo.ttm_resv);
locked = ww_mutex_trylock(>tbo.ttm_resv.lock);
WARN_ON(!locked);
}
r = ttm_bo_init_top(>mman.bdev, >tbo, size, type,
page_align, NULL,
acc_size, sg, resv ? resv : 
>tbo.ttm_resv,

_ttm_bo_destroy);


No, because there's still different behavior when it comes to 
unlocking. There are three different behaviors that are needed:


1. resv == NULL, and leaving ttm_bo_init with the BO unreserved.

2. resv != NULL, and not changing the reserved status during 
initialization.


3. resv != NULL, and leaving initialization with the BO reserved, but 
unlocking when the BO is destroyed.


1+2 can be expressed well with the ttm_bo_init interface, but 3 cannot.

I think a possible alternative would be to change ttm_bo_init so that 
it always returns on success with the BO reserved, but that would 
require changing all the drivers, and I don't really see the advantage 
over just being more explicit in each driver.

OK, make sense, then wait Alex to submit to staging branch and verify it.

Thanks,
David Zhou


Cheers,
Nicolai



Regards,
David Zhou

+ret = ttm_bo_init_top(bdev, bo, size, type, page_alignment,
+  persistent_swap_storage, acc_size, sg, resv,
+  destroy);
+if (ret) {
+if

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

2017-02-15 Thread Nicolai Hähnle


On 15.02.2017 04:16, zhoucm1 wrote:

On 2017年02月14日 18:37, Nicolai Hähnle wrote:

From: Nicolai Hähnle 

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 
---
  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;
  }
@@ -1204,6 +1193,37 @@ int ttm_bo_init(struct ttm_bo_device *bdev,
  ret = drm_vma_offset_add(>vma_manager, >vma_node,
   bo->mem.num_pages);

if (ret && !resv), we should call
reservation_object_fini(>ttm_resv), right?


Good point, though actually, ret can never be != 0 here, so this can be 
simplified a bit.




  +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;
+

Can we lock resv anyway before ttm_bo_init_top like what you did in
patch #3? if yes, seems we don't need patch#3 any more, right?


if (!resv) {
bool locked;

reservation_object_init(>tbo.ttm_resv);
locked = ww_mutex_trylock(>tbo.ttm_resv.lock);
WARN_ON(!locked);
}
r = ttm_bo_init_top(>mman.bdev, >tbo, size, type,
page_align, NULL,
acc_size, sg, resv ? resv : >tbo.ttm_resv,
_ttm_bo_destroy);


No, because there's still different behavior when it comes to unlocking. 
There are three different behaviors that are needed:


1. resv == NULL, and leaving ttm_bo_init with the BO unreserved.

2. resv != NULL, and not changing the reserved status during initialization.

3. resv != NULL, and leaving initialization with the BO reserved, but 
unlocking when the BO is destroyed.


1+2 can be expressed well with the ttm_bo_init interface, but 3 cannot.

I think a possible alternative would be to change ttm_bo_init so that it 
always returns on success with the BO reserved, but that would require 
changing all the drivers, and I don't really see the advantage over just 
being more explicit in each driver.


Cheers,
Nicolai



Regards,
David Zhou

+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

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

2017-02-14 Thread zhoucm1




On 2017年02月14日 18:37, Nicolai Hähnle wrote:

From: Nicolai Hähnle 

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 
---
  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;
}
@@ -1204,6 +1193,37 @@ int ttm_bo_init(struct ttm_bo_device *bdev,
ret = drm_vma_offset_add(>vma_manager, >vma_node,
 bo->mem.num_pages);
if (ret && !resv), we should call 
reservation_object_fini(>ttm_resv), right?


  
+	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;
+
Can we lock resv anyway before ttm_bo_init_top like what you did in 
patch #3? if yes, seems we don't need patch#3 any more, right?



if (!resv) {
bool locked;

reservation_object_init(>tbo.ttm_resv);
locked = ww_mutex_trylock(>tbo.ttm_resv.lock);
WARN_ON(!locked);
}
r = ttm_bo_init_top(>mman.bdev, >tbo, size, type,
page_align, NULL,
acc_size, sg, resv ? resv : >tbo.ttm_resv,
_ttm_bo_destroy);


Regards,
David Zhou

+   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);
  
  /**

+

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

2017-02-14 Thread 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 

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 


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).




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(>vma_manager, >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:

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

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

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

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

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

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

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

7 matches

Site Navigation

Mail list logo

Footer information