Linaro-mm-sig July 2018

linaro-mm-sig@lists.linaro.org

9 participants
14 discussions

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.

5 years, 10 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, >>

5 years, 10 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, >>

5 years, 10 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

5 years, 10 months

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig July 2018