Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

15 participants
2904 discussions

Re: [Linaro-mm-sig] [PATCH] dma-buf: Move BUG_ON from _add_shared_fence to _add_shared_inplace

by Christian König

Am 04.07.2018 um 11:09 schrieb Michel Dänzer: > On 2018-07-04 10:31 AM, Christian König wrote: >> Am 26.06.2018 um 16:31 schrieb Michel Dänzer: >>> From: Michel Dänzer <michel.daenzer(a)amd.com> >>> >>> Fixes the BUG_ON spuriously triggering under the following >>> circumstances: >>> >>> * ttm_eu_reserve_buffers processes a list containing multiple BOs using >>> the same reservation object, so it calls >>> reservation_object_reserve_shared with that reservation object once >>> for each such BO. >>> * In reservation_object_reserve_shared, old->shared_count == >>> old->shared_max - 1, so obj->staged is freed in preparation of an >>> in-place update. >>> * ttm_eu_fence_buffer_objects calls reservation_object_add_shared_fence >>> once for each of the BOs above, always with the same fence. >>> * The first call adds the fence in the remaining free slot, after which >>> old->shared_count == old->shared_max. >> Well, the explanation here is not correct. For multiple BOs using the >> same reservation object we won't call >> reservation_object_add_shared_fence() multiple times because we move >> those to the duplicates list in ttm_eu_reserve_buffers(). >> >> But this bug can still happen because we call >> reservation_object_add_shared_fence() manually with fences for the same >> context in a couple of places. >> >> One prominent case which comes to my mind are for the VM BOs during >> updates. Another possibility are VRAM BOs which need to be cleared. > Thanks. How about the following: > > * ttm_eu_reserve_buffers calls reservation_object_reserve_shared. > * In reservation_object_reserve_shared, shared_count == shared_max - 1, > so obj->staged is freed in preparation of an in-place update. > * ttm_eu_fence_buffer_objects calls reservation_object_add_shared_fence, > after which shared_count == shared_max. > * The amdgpu driver also calls reservation_object_add_shared_fence for > the same reservation object, and the BUG_ON triggers. I would rather completely drop the reference to the ttm_eu_* functions, cause those wrappers are completely unrelated to the problem. Instead let's just note something like the following: * When reservation_object_reserve_shared is called with shared_count == shared_max - 1, so obj->staged is freed in preparation of an in-place update. * Now reservation_object_add_shared_fence is called with the first fence and after that shared_count == shared_max. * After that reservation_object_add_shared_fence can be called with follow up fences from the same context, but since shared_count == shared_max we would run into this BUG_ON. > However, nothing bad would happen in > reservation_object_add_shared_inplace, since all fences use the same > context, so they can only occupy a single slot. > > Prevent this by moving the BUG_ON to where an overflow would actually > happen (e.g. if a buggy caller didn't call > reservation_object_reserve_shared before). > > > Also, I'll add a reference to https://bugs.freedesktop.org/106418 in v2, > as I suspect this fix is necessary under the circumstances described > there as well. The rest sounds good to me. Regards, Christian.

6 years, 12 months

Re: [Linaro-mm-sig] [PATCH] dma-buf: Move BUG_ON from _add_shared_fence to _add_shared_inplace

by Christian König

Am 26.06.2018 um 16:31 schrieb Michel Dänzer: > From: Michel Dänzer <michel.daenzer(a)amd.com> > > Fixes the BUG_ON spuriously triggering under the following > circumstances: > > * ttm_eu_reserve_buffers processes a list containing multiple BOs using > the same reservation object, so it calls > reservation_object_reserve_shared with that reservation object once > for each such BO. > * In reservation_object_reserve_shared, old->shared_count == > old->shared_max - 1, so obj->staged is freed in preparation of an > in-place update. > * ttm_eu_fence_buffer_objects calls reservation_object_add_shared_fence > once for each of the BOs above, always with the same fence. > * The first call adds the fence in the remaining free slot, after which > old->shared_count == old->shared_max. Well, the explanation here is not correct. For multiple BOs using the same reservation object we won't call reservation_object_add_shared_fence() multiple times because we move those to the duplicates list in ttm_eu_reserve_buffers(). But this bug can still happen because we call reservation_object_add_shared_fence() manually with fences for the same context in a couple of places. One prominent case which comes to my mind are for the VM BOs during updates. Another possibility are VRAM BOs which need to be cleared. > > In the next call to reservation_object_add_shared_fence, the BUG_ON > triggers. However, nothing bad would happen in > reservation_object_add_shared_inplace, since the fence is already in the > reservation object. > > Prevent this by moving the BUG_ON to where an overflow would actually > happen (e.g. if a buggy caller didn't call > reservation_object_reserve_shared before). > > Cc: stable(a)vger.kernel.org > Signed-off-by: Michel Dänzer <michel.daenzer(a)amd.com> Reviewed-by: Christian König <christian.koenig(a)amd.com>. Regards, Christian. > --- > drivers/dma-buf/reservation.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c > index 314eb1071cce..532545b9488e 100644 > --- a/drivers/dma-buf/reservation.c > +++ b/drivers/dma-buf/reservation.c > @@ -141,6 +141,7 @@ reservation_object_add_shared_inplace(struct reservation_object *obj, > if (signaled) { > RCU_INIT_POINTER(fobj->shared[signaled_idx], fence); > } else { > + BUG_ON(fobj->shared_count >= fobj->shared_max); > RCU_INIT_POINTER(fobj->shared[fobj->shared_count], fence); > fobj->shared_count++; > } > @@ -230,10 +231,9 @@ void reservation_object_add_shared_fence(struct reservation_object *obj, > old = reservation_object_get_list(obj); > obj->staged = NULL; > > - if (!fobj) { > - BUG_ON(old->shared_count >= old->shared_max); > + if (!fobj) > reservation_object_add_shared_inplace(obj, old, fence); > - } else > + else > reservation_object_add_shared_replace(obj, old, fobj, fence); > } > EXPORT_SYMBOL(reservation_object_add_shared_fence);

6 years, 12 months

First step towards unpinned DMA-buf operation.

by Christian König

[As requested by Daniel cross posting to intel-gfx as well]. This set is the first step towards allowing to use a DMA-buf without actually pinning the underlying resources. This in turn the the ground work for PCIe P2P operations as well as quite a bunch of other use cases. The idea is that we build the support for unpinned operation around the already present reservation lock in the DMA-buf object. For this we now grab the reservation object lock while mapping and unmapping DMA-bufs. The down side is that all implementations as well as users of DMA-buf needs to be audited to make sure that we don't run into double locking or lock inversions. So please test and/or comment and report back how badly lockdep complains :) Thanks, Christian.

6 years, 12 months

Re: [Linaro-mm-sig] [PATCH 1/4] dma-buf: add dma_buf_(un)map_attachment_locked variants v2

by Christian König

Am 28.06.2018 um 11:53 schrieb Zhang, Jerry (Junwei): > On 06/22/2018 10:11 PM, Christian König wrote: >> Add function variants which can be called with the reservation lock >> already held. >> >> v2: reordered, add lockdep asserts, fix kerneldoc >> >> Signed-off-by: Christian König <christian.koenig(a)amd.com> >> --- >> drivers/dma-buf/dma-buf.c | 57 >> +++++++++++++++++++++++++++++++++++++++++++++++ >> include/linux/dma-buf.h | 5 +++++ >> 2 files changed, 62 insertions(+) >> >> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c >> index 852a3928ee71..dc94e76e2e2a 100644 >> --- a/drivers/dma-buf/dma-buf.c >> +++ b/drivers/dma-buf/dma-buf.c >> @@ -606,6 +606,40 @@ void dma_buf_detach(struct dma_buf *dmabuf, >> struct dma_buf_attachment *attach) >> } >> EXPORT_SYMBOL_GPL(dma_buf_detach); >> >> +/** >> + * dma_buf_map_attachment_locked - Maps the buffer into _device_ >> address space >> + * with the reservation lock held. Is a wrapper for map_dma_buf() of >> the >> + * >> + * Returns the scatterlist table of the attachment; >> + * dma_buf_ops. >> + * @attach: [in] attachment whose scatterlist is to be returned >> + * @direction: [in] direction of DMA transfer >> + * >> + * Returns sg_table containing the scatterlist to be returned; >> returns ERR_PTR >> + * on error. May return -EINTR if it is interrupted by a signal. >> + * >> + * A mapping must be unmapped by using >> dma_buf_unmap_attachment_locked(). Note >> + * that the underlying backing storage is pinned for as long as a >> mapping >> + * exists, therefore users/importers should not hold onto a mapping >> for undue >> + * amounts of time. >> + */ >> +struct sg_table * >> +dma_buf_map_attachment_locked(struct dma_buf_attachment *attach, >> + enum dma_data_direction direction) >> +{ >> + struct sg_table *sg_table; >> + > > Perhaps better to add some error check, like dma_buf_map_attachment() > > WARN_ON(!attach || !attach->dmabuf) Actually I wanted to remove those from the other functions as well. WARN_ON and BUG_ON checks for NULL pointers before using them are totally pointless because they have the same effect as a crash. Regards, Christian. > > Apart from that, it's > Reviewed-by: Junwei Zhang <Jerry.Zhang(a)amd.com> > > Jerry > >> + might_sleep(); >> + reservation_object_assert_held(attach->dmabuf->resv); >> + >> + sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction); >> + if (!sg_table) >> + sg_table = ERR_PTR(-ENOMEM); >> + >> + return sg_table; >> +} >> +EXPORT_SYMBOL_GPL(dma_buf_map_attachment_locked); >> + >> /** >> * dma_buf_map_attachment - Returns the scatterlist table of the >> attachment; >> * mapped into _device_ address space. Is a wrapper for >> map_dma_buf() of the >> @@ -639,6 +673,29 @@ struct sg_table *dma_buf_map_attachment(struct >> dma_buf_attachment *attach, >> } >> EXPORT_SYMBOL_GPL(dma_buf_map_attachment); >> >> +/** >> + * dma_buf_unmap_attachment_locked - unmaps the buffer with >> reservation lock >> + * held, should deallocate the associated scatterlist. Is a wrapper for >> + * unmap_dma_buf() of dma_buf_ops. >> + * @attach: [in] attachment to unmap buffer from >> + * @sg_table: [in] scatterlist info of the buffer to unmap >> + * @direction: [in] direction of DMA transfer >> + * >> + * This unmaps a DMA mapping for @attached obtained by >> + * dma_buf_map_attachment_locked(). >> + */ >> +void dma_buf_unmap_attachment_locked(struct dma_buf_attachment *attach, >> + struct sg_table *sg_table, >> + enum dma_data_direction direction) >> +{ >> + might_sleep(); >> + reservation_object_assert_held(attach->dmabuf->resv); >> + >> + attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, >> + direction); >> +} >> +EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment_locked); >> + >> /** >> * dma_buf_unmap_attachment - unmaps and decreases usecount of the >> buffer;might >> * deallocate the scatterlist associated. Is a wrapper for >> unmap_dma_buf() of >> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h >> index 991787a03199..a25e754ae2f7 100644 >> --- a/include/linux/dma-buf.h >> +++ b/include/linux/dma-buf.h >> @@ -384,8 +384,13 @@ int dma_buf_fd(struct dma_buf *dmabuf, int flags); >> struct dma_buf *dma_buf_get(int fd); >> void dma_buf_put(struct dma_buf *dmabuf); >> >> +struct sg_table *dma_buf_map_attachment_locked(struct >> dma_buf_attachment *, >> + enum dma_data_direction); >> struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *, >> enum dma_data_direction); >> +void dma_buf_unmap_attachment_locked(struct dma_buf_attachment *, >> + struct sg_table *, >> + enum dma_data_direction); >> void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct >> sg_table *, >> enum dma_data_direction); >> int dma_buf_begin_cpu_access(struct dma_buf *dma_buf, >>

6 years, 12 months

Re: [Linaro-mm-sig] [PATCH 3/4] drm/amdgpu: add independent DMA-buf export v3

by Christian König

Am 28.06.2018 um 11:58 schrieb Zhang, Jerry (Junwei): > On 06/22/2018 10:11 PM, Christian König wrote: >> The caching of SGT's done by the DRM code is actually quite harmful and >> should probably removed altogether in the long term. >> >> Start by providing a separate DMA-buf export implementation in >> amdgpu. This is >> also a prerequisite of unpinned DMA-buf handling. >> >> v2: fix unintended recursion, remove debugging leftovers >> v3: split out from unpinned DMA-buf work >> >> Signed-off-by: Christian König <christian.koenig(a)amd.com> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 - >> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 1 - >> drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 94 >> +++++++++++++------------------ >> 3 files changed, 39 insertions(+), 57 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h >> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h >> index d8e0cc08f9db..5e71af8dd3a7 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h >> @@ -373,7 +373,6 @@ int amdgpu_gem_object_open(struct drm_gem_object >> *obj, >> void amdgpu_gem_object_close(struct drm_gem_object *obj, >> struct drm_file *file_priv); >> unsigned long amdgpu_gem_timeout(uint64_t timeout_ns); >> -struct sg_table *amdgpu_gem_prime_get_sg_table(struct drm_gem_object >> *obj); >> struct drm_gem_object * >> amdgpu_gem_prime_import_sg_table(struct drm_device *dev, >> struct dma_buf_attachment *attach, >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c >> index a549483032b0..cdf0be85d361 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c >> @@ -919,7 +919,6 @@ static struct drm_driver kms_driver = { >> .gem_prime_export = amdgpu_gem_prime_export, >> .gem_prime_import = amdgpu_gem_prime_import, >> .gem_prime_res_obj = amdgpu_gem_prime_res_obj, >> - .gem_prime_get_sg_table = amdgpu_gem_prime_get_sg_table, > > I may miss some patches or important info. > > Even applied the series of patch, I still could find below code in > drm_gem_map_dma_buf(). > In this case, it will cause NULL pointer access. Please help me out of > here. > {{{ > sgt = obj->dev->driver->gem_prime_get_sg_table(obj); > }}} Take a look at the change a bit further down. > > Jerry > >> .gem_prime_import_sg_table = amdgpu_gem_prime_import_sg_table, >> .gem_prime_vmap = amdgpu_gem_prime_vmap, >> .gem_prime_vunmap = amdgpu_gem_prime_vunmap, >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c >> index b2286bc41aec..038a8c8488b7 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c >> @@ -40,22 +40,6 @@ >> >> static const struct dma_buf_ops amdgpu_dmabuf_ops; >> >> -/** >> - * amdgpu_gem_prime_get_sg_table - &drm_driver.gem_prime_get_sg_table >> - * implementation >> - * @obj: GEM buffer object >> - * >> - * Returns: >> - * A scatter/gather table for the pinned pages of the buffer >> object's memory. >> - */ >> -struct sg_table *amdgpu_gem_prime_get_sg_table(struct drm_gem_object >> *obj) >> -{ >> - struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); >> - int npages = bo->tbo.num_pages; >> - >> - return drm_prime_pages_to_sg(bo->tbo.ttm->pages, npages); >> -} >> - >> /** >> * amdgpu_gem_prime_vmap - &dma_buf_ops.vmap implementation >> * @obj: GEM buffer object >> @@ -189,35 +173,29 @@ amdgpu_gem_prime_import_sg_table(struct >> drm_device *dev, >> } >> >> /** >> - * amdgpu_gem_map_attach - &dma_buf_ops.attach implementation >> - * @dma_buf: shared DMA buffer >> - * @target_dev: target device >> + * amdgpu_gem_map_dma_buf - &dma_buf_ops.map_dma_buf implementation >> * @attach: DMA-buf attachment >> + * @dir: DMA direction >> * >> * Makes sure that the shared DMA buffer can be accessed by the >> target device. >> * For now, simply pins it to the GTT domain, where it should be >> accessible by >> * all DMA devices. >> * >> * Returns: >> - * 0 on success or negative error code. >> + * sg_table filled with the DMA addresses to use or ERR_PRT with >> negative error >> + * code. >> */ >> -static int amdgpu_gem_map_attach(struct dma_buf *dma_buf, >> - struct dma_buf_attachment *attach) >> +static struct sg_table * >> +amdgpu_gem_map_dma_buf(struct dma_buf_attachment *attach, >> + enum dma_data_direction dir) >> { >> + struct dma_buf *dma_buf = attach->dmabuf; >> struct drm_gem_object *obj = dma_buf->priv; >> struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); >> struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev); >> + struct sg_table *sgt; >> long r; >> >> - r = drm_gem_map_attach(dma_buf, attach); >> - if (r) >> - return r; >> - >> - r = amdgpu_bo_reserve(bo, false); >> - if (unlikely(r != 0)) >> - goto error_detach; >> - >> - >> if (attach->dev->driver != adev->dev->driver) { >> /* >> * Wait for all shared fences to complete before we switch >> to future >> @@ -228,54 +206,62 @@ static int amdgpu_gem_map_attach(struct dma_buf >> *dma_buf, >> MAX_SCHEDULE_TIMEOUT); >> if (unlikely(r < 0)) { >> DRM_DEBUG_PRIME("Fence wait failed: %li\n", r); >> - goto error_unreserve; >> + return ERR_PTR(r); >> } >> } >> >> /* pin buffer into GTT */ >> r = amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT, NULL); >> if (r) >> - goto error_unreserve; >> + return ERR_PTR(r); >> + >> + sgt = drm_prime_pages_to_sg(bo->tbo.ttm->pages, bo->tbo.num_pages); >> + if (IS_ERR(sgt)) >> + return sgt; >> + >> + if (!dma_map_sg_attrs(attach->dev, sgt->sgl, sgt->nents, dir, >> + DMA_ATTR_SKIP_CPU_SYNC)) >> + goto error_free; >> >> if (attach->dev->driver != adev->dev->driver) >> bo->prime_shared_count++; >> >> -error_unreserve: >> - amdgpu_bo_unreserve(bo); >> + return sgt; >> >> -error_detach: >> - if (r) >> - drm_gem_map_detach(dma_buf, attach); >> - return r; >> +error_free: >> + sg_free_table(sgt); >> + kfree(sgt); >> + return ERR_PTR(-ENOMEM); >> } >> >> /** >> - * amdgpu_gem_map_detach - &dma_buf_ops.detach implementation >> - * @dma_buf: shared DMA buffer >> + * amdgpu_gem_unmap_dma_buf - &dma_buf_ops.unmap_dma_buf implementation >> * @attach: DMA-buf attachment >> + * @sgt: sg_table to unmap >> + * @dir: DMA direction >> * >> * This is called when a shared DMA buffer no longer needs to be >> accessible by >> * the other device. For now, simply unpins the buffer from GTT. >> */ >> -static void amdgpu_gem_map_detach(struct dma_buf *dma_buf, >> - struct dma_buf_attachment *attach) >> +static void amdgpu_gem_unmap_dma_buf(struct dma_buf_attachment *attach, >> + struct sg_table *sgt, >> + enum dma_data_direction dir) >> { >> + struct dma_buf *dma_buf = attach->dmabuf; >> struct drm_gem_object *obj = dma_buf->priv; >> struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); >> struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev); >> - int ret = 0; >> - >> - ret = amdgpu_bo_reserve(bo, true); >> - if (unlikely(ret != 0)) >> - goto error; >> >> amdgpu_bo_unpin(bo); >> + >> if (attach->dev->driver != adev->dev->driver && >> bo->prime_shared_count) >> bo->prime_shared_count--; >> - amdgpu_bo_unreserve(bo); >> >> -error: >> - drm_gem_map_detach(dma_buf, attach); >> + if (sgt) { >> + dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir); >> + sg_free_table(sgt); >> + kfree(sgt); >> + } >> } >> >> /** >> @@ -333,10 +319,8 @@ static int amdgpu_gem_begin_cpu_access(struct >> dma_buf *dma_buf, >> } >> >> static const struct dma_buf_ops amdgpu_dmabuf_ops = { >> - .attach = amdgpu_gem_map_attach, >> - .detach = amdgpu_gem_map_detach, >> - .map_dma_buf = drm_gem_map_dma_buf, >> - .unmap_dma_buf = drm_gem_unmap_dma_buf, Here we are stopping to use drm_gem_map_dma_buf(), so we don't need amdgpu_gem_prime_get_sg_table() any more either. Christian. >> + .map_dma_buf = amdgpu_gem_map_dma_buf, >> + .unmap_dma_buf = amdgpu_gem_unmap_dma_buf, >> .release = drm_gem_dmabuf_release, >> .begin_cpu_access = amdgpu_gem_begin_cpu_access, >> .map = drm_gem_dmabuf_kmap, >>

6 years, 12 months

[PATCH 04/15] dma-fence: Make ->wait callback optional

by Daniel Vetter

Almost everyone uses dma_fence_default_wait. v2: Also remove the BUG_ON(!ops->wait) (Chris). Reviewed-by: Christian König <christian.koenig(a)amd.com> (v1) Signed-off-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> Cc: Chris Wilson <chris(a)chris-wilson.co.uk> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: Gustavo Padovan <gustavo(a)padovan.org> Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org --- drivers/dma-buf/dma-fence-array.c | 1 - drivers/dma-buf/dma-fence.c | 8 +++++--- drivers/dma-buf/sw_sync.c | 1 - include/linux/dma-fence.h | 13 ++++++++----- 4 files changed, 13 insertions(+), 10 deletions(-) diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c index dd1edfb27b61..a8c254497251 100644 --- a/drivers/dma-buf/dma-fence-array.c +++ b/drivers/dma-buf/dma-fence-array.c @@ -104,7 +104,6 @@ const struct dma_fence_ops dma_fence_array_ops = { .get_timeline_name = dma_fence_array_get_timeline_name, .enable_signaling = dma_fence_array_enable_signaling, .signaled = dma_fence_array_signaled, - .wait = dma_fence_default_wait, .release = dma_fence_array_release, }; EXPORT_SYMBOL(dma_fence_array_ops); diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 59049375bd19..41ec19c9efc7 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -158,7 +158,10 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout) return -EINVAL; trace_dma_fence_wait_start(fence); - ret = fence->ops->wait(fence, intr, timeout); + if (fence->ops->wait) + ret = fence->ops->wait(fence, intr, timeout); + else + ret = dma_fence_default_wait(fence, intr, timeout); trace_dma_fence_wait_end(fence); return ret; } @@ -562,8 +565,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, spinlock_t *lock, u64 context, unsigned seqno) { BUG_ON(!lock); - BUG_ON(!ops || !ops->wait || - !ops->get_driver_name || !ops->get_timeline_name); + BUG_ON(!ops || !ops->get_driver_name || !ops->get_timeline_name); kref_init(&fence->refcount); fence->ops = ops; diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c index 3d78ca89a605..53c1d6d36a64 100644 --- a/drivers/dma-buf/sw_sync.c +++ b/drivers/dma-buf/sw_sync.c @@ -188,7 +188,6 @@ static const struct dma_fence_ops timeline_fence_ops = { .get_timeline_name = timeline_fence_get_timeline_name, .enable_signaling = timeline_fence_enable_signaling, .signaled = timeline_fence_signaled, - .wait = dma_fence_default_wait, .release = timeline_fence_release, .fence_value_str = timeline_fence_value_str, .timeline_value_str = timeline_fence_timeline_value_str, diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h index c053d19e1e24..02dba8cd033d 100644 --- a/include/linux/dma-fence.h +++ b/include/linux/dma-fence.h @@ -191,11 +191,14 @@ struct dma_fence_ops { /** * @wait: * - * Custom wait implementation, or dma_fence_default_wait. + * Custom wait implementation, defaults to dma_fence_default_wait() if + * not set. * - * Must not be NULL, set to dma_fence_default_wait for default implementation. - * the dma_fence_default_wait implementation should work for any fence, as long - * as enable_signaling works correctly. + * The dma_fence_default_wait implementation should work for any fence, as long + * as @enable_signaling works correctly. This hook allows drivers to + * have an optimized version for the case where a process context is + * already available, e.g. if @enable_signaling for the general case + * needs to set up a worker thread. * * Must return -ERESTARTSYS if the wait is intr = true and the wait was * interrupted, and remaining jiffies if fence has signaled, or 0 if wait @@ -203,7 +206,7 @@ struct dma_fence_ops { * which should be treated as if the fence is signaled. For example a hardware * lockup could be reported like that. * - * This callback is mandatory. + * This callback is optional. */ signed long (*wait)(struct dma_fence *fence, bool intr, signed long timeout); -- 2.17.0

6 years, 12 months

Re: [Linaro-mm-sig] [PATCH] dma-buf: Move BUG_ON from _add_shared_fence to _add_shared_inplace

by Chris Wilson

Quoting Michel Dänzer (2018-06-26 15:31:47) > From: Michel Dänzer <michel.daenzer(a)amd.com> > > Fixes the BUG_ON spuriously triggering under the following > circumstances: > > * ttm_eu_reserve_buffers processes a list containing multiple BOs using > the same reservation object, so it calls > reservation_object_reserve_shared with that reservation object once > for each such BO. > * In reservation_object_reserve_shared, old->shared_count == > old->shared_max - 1, so obj->staged is freed in preparation of an > in-place update. > * ttm_eu_fence_buffer_objects calls reservation_object_add_shared_fence > once for each of the BOs above, always with the same fence. > * The first call adds the fence in the remaining free slot, after which > old->shared_count == old->shared_max. > > In the next call to reservation_object_add_shared_fence, the BUG_ON > triggers. However, nothing bad would happen in > reservation_object_add_shared_inplace, since the fence is already in the > reservation object. > > Prevent this by moving the BUG_ON to where an overflow would actually > happen (e.g. if a buggy caller didn't call > reservation_object_reserve_shared before). > > Cc: stable(a)vger.kernel.org > Signed-off-by: Michel Dänzer <michel.daenzer(a)amd.com> I've convinced myself (or rather have not found a valid argument against) that being able to call reserve_shared + add_shared multiple times for the same fence is an intended part of reservation_object API I'd double check with Christian though. Reviewed-by: Chris Wilson <chris(a)chris-wilson.co.uk> > drivers/dma-buf/reservation.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c > index 314eb1071cce..532545b9488e 100644 > --- a/drivers/dma-buf/reservation.c > +++ b/drivers/dma-buf/reservation.c > @@ -141,6 +141,7 @@ reservation_object_add_shared_inplace(struct reservation_object *obj, > if (signaled) { > RCU_INIT_POINTER(fobj->shared[signaled_idx], fence); > } else { > + BUG_ON(fobj->shared_count >= fobj->shared_max); Personally I would just let kasan detect this and throw away the BUG_ON or at least move it behind some DMABUF_BUG_ON(). -Chris

7 years

Re: [Linaro-mm-sig] [PATCH v2] dma-buf/fence: Take refcount on the module that owns the fence

by Daniel Vetter

On Mon, Jun 25, 2018 at 09:21:15PM +0530, Akhil P Oommen wrote: > > > On 6/25/2018 1:20 PM, Daniel Vetter wrote: > > On Fri, Jun 22, 2018 at 11:08:48AM +0100, Chris Wilson wrote: > > > Quoting Gustavo Padovan (2018-06-22 11:04:16) > > > > Hi Akhil, > > > > > > > > On Fri, 2018-06-22 at 15:10 +0530, Akhil P Oommen wrote: > > > > > Each fence object holds function pointers of the module that > > > > > initialized > > > > > it. Allowing the module to unload before this fence's release is > > > > > catastrophic. So, keep a refcount on the module until the fence is > > > > > released. > > > > > > > > > > Signed-off-by: Akhil P Oommen <akhilpo(a)codeaurora.org> > > > > > --- > > > > > Changes in v2: > > > > > - added description for the new function parameter. > > > > > > > > > > drivers/dma-buf/dma-fence.c | 16 +++++++++++++--- > > > > > include/linux/dma-fence.h | 10 ++++++++-- > > > > > 2 files changed, 21 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma- > > > > > fence.c > > > > > index 4edb9fd..2aaa44e 100644 > > > > > --- a/drivers/dma-buf/dma-fence.c > > > > > +++ b/drivers/dma-buf/dma-fence.c > > > > > @@ -18,6 +18,7 @@ > > > > > * more details. > > > > > */ > > > > > +#include <linux/module.h> > > > > > #include <linux/slab.h> > > > > > #include <linux/export.h> > > > > > #include <linux/atomic.h> > > > > > @@ -168,6 +169,7 @@ void dma_fence_release(struct kref *kref) > > > > > { > > > > > struct dma_fence *fence = > > > > > container_of(kref, struct dma_fence, refcount); > > > > > + struct module *module = fence->owner; > > > > > trace_dma_fence_destroy(fence); > > > > > @@ -178,6 +180,8 @@ void dma_fence_release(struct kref *kref) > > > > > fence->ops->release(fence); > > > > > else > > > > > dma_fence_free(fence); > > > > > + > > > > > + module_put(module); > > > > > } > > > > > EXPORT_SYMBOL(dma_fence_release); > > > > > @@ -541,6 +545,7 @@ struct default_wait_cb { > > > > > /** > > > > > * dma_fence_init - Initialize a custom fence. > > > > > + * @module: [in] the module that calls this API > > > > > * @fence: [in] the fence to initialize > > > > > * @ops: [in] the dma_fence_ops for operations on this > > > > > fence > > > > > * @lock: [in] the irqsafe spinlock to use for locking > > > > > this fence > > > > > @@ -556,8 +561,9 @@ struct default_wait_cb { > > > > > * to check which fence is later by simply using dma_fence_later. > > > > > */ > > > > > void > > > > > -dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops > > > > > *ops, > > > > > - spinlock_t *lock, u64 context, unsigned seqno) > > > > > +_dma_fence_init(struct module *module, struct dma_fence *fence, > > > > > + const struct dma_fence_ops *ops, spinlock_t *lock, > > > > > + u64 context, unsigned seqno) > > > > > { > > > > > BUG_ON(!lock); > > > > > BUG_ON(!ops || !ops->wait || !ops->enable_signaling || > > > > > @@ -571,7 +577,11 @@ struct default_wait_cb { > > > > > fence->seqno = seqno; > > > > > fence->flags = 0UL; > > > > > fence->error = 0; > > > > > + fence->owner = module; > > > > > + > > > > > + if (!try_module_get(module)) > > > > > + fence->owner = NULL; > > > > > trace_dma_fence_init(fence); > > > > > } > > > > > -EXPORT_SYMBOL(dma_fence_init); > > > > > +EXPORT_SYMBOL(_dma_fence_init); > > > > Do we still need to export the symbol, it won't be called from outside > > > > anymore? Other than that looks good to me: > > > There's a big drawback in that a module reference is often insufficient, > > > and that a reference on the driver (or whatever is required for the > > > lifetime of the fence) will already hold the module reference. > > > > > > Considering that we want a few 100k fences in flight per second, is > > > there no other way to only export a fence with a module reference? > > We'd need to make the timeline a full-blown object (Maarten owes me one > > for that design screw-up), and then we could stuff all these things in > > there. > > > > And I think that's the right fix, since try_module_get for every > > dma_fence_init just ain't cool really :-) > > -Daniel > Thanks for the feedback, Daniel. > I see your point, but I am not sure how much impact an extra refcounting > would create considering the whole effort of setting up a new fence. Also, > this refcounting is not required for built-in modules. > > As of now, unloading a kernel module that uses fence_init() is an easy way > to bring down the system. This patch simply fixes that. What you have > suggested sounds like a non-trivial effort which someone who is more > familiar with this code base can do a better job than me. Perhaps we can > take this patch now to fix the issue at hand and later somebody else can > share a more optimal solution. :) Module unload is a developer-only feature for a reason. Given that I don't think fixing this with a hack is the right approach. And dma_fence_init() is supposed to be really fast. Also note that you can fix this already for your own driver by simply waiting for all pending dma_fences to get released, so I don't think it's super-important to land this asap. Yes the real fix is a bit more involved, but shouldn't be too hard to pull off really. -Daniel > > @Gustavo & @Sumit, I would like the maintainers to take a decision here. > > -Akhil. > _______________________________________________ > dri-devel mailing list > dri-devel(a)lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

7 years

Re: [Linaro-mm-sig] [PATCH v2] dma-buf/fence: Take refcount on the module that owns the fence

by Gustavo Padovan

Hi Akhil, On Fri, 2018-06-22 at 15:10 +0530, Akhil P Oommen wrote: > Each fence object holds function pointers of the module that > initialized > it. Allowing the module to unload before this fence's release is > catastrophic. So, keep a refcount on the module until the fence is > released. > > Signed-off-by: Akhil P Oommen <akhilpo(a)codeaurora.org> > --- > Changes in v2: > - added description for the new function parameter. > > drivers/dma-buf/dma-fence.c | 16 +++++++++++++--- > include/linux/dma-fence.h | 10 ++++++++-- > 2 files changed, 21 insertions(+), 5 deletions(-) > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma- > fence.c > index 4edb9fd..2aaa44e 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -18,6 +18,7 @@ > * more details. > */ > > +#include <linux/module.h> > #include <linux/slab.h> > #include <linux/export.h> > #include <linux/atomic.h> > @@ -168,6 +169,7 @@ void dma_fence_release(struct kref *kref) > { > struct dma_fence *fence = > container_of(kref, struct dma_fence, refcount); > + struct module *module = fence->owner; > > trace_dma_fence_destroy(fence); > > @@ -178,6 +180,8 @@ void dma_fence_release(struct kref *kref) > fence->ops->release(fence); > else > dma_fence_free(fence); > + > + module_put(module); > } > EXPORT_SYMBOL(dma_fence_release); > > @@ -541,6 +545,7 @@ struct default_wait_cb { > > /** > * dma_fence_init - Initialize a custom fence. > + * @module: [in] the module that calls this API > * @fence: [in] the fence to initialize > * @ops: [in] the dma_fence_ops for operations on this > fence > * @lock: [in] the irqsafe spinlock to use for locking > this fence > @@ -556,8 +561,9 @@ struct default_wait_cb { > * to check which fence is later by simply using dma_fence_later. > */ > void > -dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops > *ops, > - spinlock_t *lock, u64 context, unsigned seqno) > +_dma_fence_init(struct module *module, struct dma_fence *fence, > + const struct dma_fence_ops *ops, spinlock_t *lock, > + u64 context, unsigned seqno) > { > BUG_ON(!lock); > BUG_ON(!ops || !ops->wait || !ops->enable_signaling || > @@ -571,7 +577,11 @@ struct default_wait_cb { > fence->seqno = seqno; > fence->flags = 0UL; > fence->error = 0; > + fence->owner = module; > + > + if (!try_module_get(module)) > + fence->owner = NULL; > > trace_dma_fence_init(fence); > } > -EXPORT_SYMBOL(dma_fence_init); > +EXPORT_SYMBOL(_dma_fence_init); Do we still need to export the symbol, it won't be called from outside anymore? Other than that looks good to me: Reviewed-by: Gustavo Padovan <gustavo.padovan(a)collabora.com> Gustavo

7 years

[PATCH 1/5] dma_buf: remove device parameter from attach callback

by Christian König

The device parameter is completely unused because it is available in the attachment structure as well. Signed-off-by: Christian König <christian.koenig(a)amd.com> --- drivers/dma-buf/dma-buf.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 3 +-- drivers/gpu/drm/drm_prime.c | 3 +-- drivers/gpu/drm/udl/udl_dmabuf.c | 1 - drivers/gpu/drm/vmwgfx/vmwgfx_prime.c | 1 - drivers/media/common/videobuf2/videobuf2-dma-contig.c | 2 +- drivers/media/common/videobuf2/videobuf2-dma-sg.c | 2 +- drivers/media/common/videobuf2/videobuf2-vmalloc.c | 2 +- include/drm/drm_prime.h | 2 +- include/linux/dma-buf.h | 3 +-- 10 files changed, 8 insertions(+), 13 deletions(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index d78d5fc173dc..e99a8d19991b 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -568,7 +568,7 @@ struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, mutex_lock(&dmabuf->lock); if (dmabuf->ops->attach) { - ret = dmabuf->ops->attach(dmabuf, dev, attach); + ret = dmabuf->ops->attach(dmabuf, attach); if (ret) goto err_attach; } diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c index 4683626b065f..f1500f1ec0f5 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c @@ -133,7 +133,6 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev, } static int amdgpu_gem_map_attach(struct dma_buf *dma_buf, - struct device *target_dev, struct dma_buf_attachment *attach) { struct drm_gem_object *obj = dma_buf->priv; @@ -141,7 +140,7 @@ static int amdgpu_gem_map_attach(struct dma_buf *dma_buf, struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev); long r; - r = drm_gem_map_attach(dma_buf, target_dev, attach); + r = drm_gem_map_attach(dma_buf, attach); if (r) return r; diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 7856a9b3f8a8..4a3a232fea67 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -186,7 +186,6 @@ static int drm_prime_lookup_buf_handle(struct drm_prime_file_private *prime_fpri /** * drm_gem_map_attach - dma_buf attach implementation for GEM * @dma_buf: buffer to attach device to - * @target_dev: not used * @attach: buffer attachment data * * Allocates &drm_prime_attachment and calls &drm_driver.gem_prime_pin for @@ -195,7 +194,7 @@ static int drm_prime_lookup_buf_handle(struct drm_prime_file_private *prime_fpri * * Returns 0 on success, negative error code on failure. */ -int drm_gem_map_attach(struct dma_buf *dma_buf, struct device *target_dev, +int drm_gem_map_attach(struct dma_buf *dma_buf, struct dma_buf_attachment *attach) { struct drm_prime_attachment *prime_attach; diff --git a/drivers/gpu/drm/udl/udl_dmabuf.c b/drivers/gpu/drm/udl/udl_dmabuf.c index 2867ed155ff6..5fdc8bdc2026 100644 --- a/drivers/gpu/drm/udl/udl_dmabuf.c +++ b/drivers/gpu/drm/udl/udl_dmabuf.c @@ -29,7 +29,6 @@ struct udl_drm_dmabuf_attachment { }; static int udl_attach_dma_buf(struct dma_buf *dmabuf, - struct device *dev, struct dma_buf_attachment *attach) { struct udl_drm_dmabuf_attachment *udl_attach; diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c b/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c index 0d42a46521fc..fbffb37ccf42 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c @@ -40,7 +40,6 @@ */ static int vmw_prime_map_attach(struct dma_buf *dma_buf, - struct device *target_dev, struct dma_buf_attachment *attach) { return -ENOSYS; diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c index f1178f6f434d..12d0072c52c2 100644 --- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c +++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c @@ -222,7 +222,7 @@ struct vb2_dc_attachment { enum dma_data_direction dma_dir; }; -static int vb2_dc_dmabuf_ops_attach(struct dma_buf *dbuf, struct device *dev, +static int vb2_dc_dmabuf_ops_attach(struct dma_buf *dbuf, struct dma_buf_attachment *dbuf_attach) { struct vb2_dc_attachment *attach; diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c index 753ed3138dcc..cf94765e593f 100644 --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c @@ -371,7 +371,7 @@ struct vb2_dma_sg_attachment { enum dma_data_direction dma_dir; }; -static int vb2_dma_sg_dmabuf_ops_attach(struct dma_buf *dbuf, struct device *dev, +static int vb2_dma_sg_dmabuf_ops_attach(struct dma_buf *dbuf, struct dma_buf_attachment *dbuf_attach) { struct vb2_dma_sg_attachment *attach; diff --git a/drivers/media/common/videobuf2/videobuf2-vmalloc.c b/drivers/media/common/videobuf2/videobuf2-vmalloc.c index 3a7c80cd1a17..298ffb9ecdae 100644 --- a/drivers/media/common/videobuf2/videobuf2-vmalloc.c +++ b/drivers/media/common/videobuf2/videobuf2-vmalloc.c @@ -209,7 +209,7 @@ struct vb2_vmalloc_attachment { enum dma_data_direction dma_dir; }; -static int vb2_vmalloc_dmabuf_ops_attach(struct dma_buf *dbuf, struct device *dev, +static int vb2_vmalloc_dmabuf_ops_attach(struct dma_buf *dbuf, struct dma_buf_attachment *dbuf_attach) { struct vb2_vmalloc_attachment *attach; diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h index 4d5f5d6cf6a6..ef338151cea8 100644 --- a/include/drm/drm_prime.h +++ b/include/drm/drm_prime.h @@ -82,7 +82,7 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev, struct dma_buf *drm_gem_dmabuf_export(struct drm_device *dev, struct dma_buf_export_info *exp_info); void drm_gem_dmabuf_release(struct dma_buf *dma_buf); -int drm_gem_map_attach(struct dma_buf *dma_buf, struct device *target_dev, +int drm_gem_map_attach(struct dma_buf *dma_buf, struct dma_buf_attachment *attach); void drm_gem_map_detach(struct dma_buf *dma_buf, struct dma_buf_attachment *attach); diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index 085db2fee2d7..346caf77937f 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -77,8 +77,7 @@ struct dma_buf_ops { * to signal that backing storage is already allocated and incompatible * with the requirements of requesting device. */ - int (*attach)(struct dma_buf *, struct device *, - struct dma_buf_attachment *); + int (*attach)(struct dma_buf *, struct dma_buf_attachment *); /** * @detach: -- 2.14.1

7 years

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig