| Message ID | 20170214103744.4133-1-nhaehnle@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 14.02.2017 um 11:37 schrieb Nicolai Hähnle: > From: Nicolai Hähnle <nicolai.haehnle@amd.com> > > 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@amd.com> 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. 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. 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.
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@amd.com> >> >> 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@amd.com> > > 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(&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. > >
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@amd.com> >>> >>> 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@amd.com> >> >> 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@amd.com>. 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. >> >> >
On 2017年02月14日 18:37, Nicolai Hähnle wrote: > From: Nicolai Hähnle <nicolai.haehnle@amd.com> > > 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@amd.com> > --- > 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(&bdev->vma_manager, &bo->vma_node, > bo->mem.num_pages); if (ret && !resv), we should call reservation_object_fini(&bo->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(&bo->tbo.ttm_resv); locked = ww_mutex_trylock(&bo->tbo.ttm_resv.lock); WARN_ON(!locked); } r = ttm_bo_init_top(&adev->mman.bdev, &bo->tbo, size, type, page_align, NULL, acc_size, sg, resv ? resv : &bo->tbo.ttm_resv, &amdgpu_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); > > /** > + * 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.
On 15.02.2017 04:16, zhoucm1 wrote: > On 2017年02月14日 18:37, Nicolai Hähnle wrote: >> From: Nicolai Hähnle <nicolai.haehnle@amd.com> >> >> 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@amd.com> >> --- >> 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(&bdev->vma_manager, &bo->vma_node, >> bo->mem.num_pages); > if (ret && !resv), we should call > reservation_object_fini(&bo->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(&bo->tbo.ttm_resv); > locked = ww_mutex_trylock(&bo->tbo.ttm_resv.lock); > WARN_ON(!locked); > } > r = ttm_bo_init_top(&adev->mman.bdev, &bo->tbo, size, type, > page_align, NULL, > acc_size, sg, resv ? resv : &bo->tbo.ttm_resv, > &amdgpu_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 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. >
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 <nicolai.haehnle@amd.com> >>> >>> 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@amd.com> >>> --- >>> 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(&bdev->vma_manager, &bo->vma_node, >>> bo->mem.num_pages); >> if (ret && !resv), we should call >> reservation_object_fini(&bo->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(&bo->tbo.ttm_resv); >> locked = ww_mutex_trylock(&bo->tbo.ttm_resv.lock); >> WARN_ON(!locked); >> } >> r = ttm_bo_init_top(&adev->mman.bdev, &bo->tbo, size, type, >> page_align, NULL, >> acc_size, sg, resv ? resv : >> &bo->tbo.ttm_resv, >> &amdgpu_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 (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. >> >
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@amd.com> >>>> >>>> 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@amd.com> >>> >>> 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@amd.com>. 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. >>> >>> >> >
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 <nicolai.haehnle@amd.com> >>>>> >>>>> 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@amd.com> >>>> >>>> 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@amd.com>. > > 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
On 15.02.2017 04:16, zhoucm1 wrote: > > > On 2017年02月14日 18:37, Nicolai Hähnle wrote: >> From: Nicolai Hähnle <nicolai.haehnle@amd.com> >> >> 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@amd.com> >> --- >> 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(&bdev->vma_manager, &bo->vma_node, >> bo->mem.num_pages); > if (ret && !resv), we should call > reservation_object_fini(&bo->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
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(&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.