Linaro-mm-sig August 2021

linaro-mm-sig@lists.linaro.org

15 participants
43 discussions

by Christian König

Hi guys, we are currently working an Freesync and direct scan out from system memory on AMD APUs in A+A laptops. On problem we stumbled over is that our display hardware needs to scan out from uncached system memory and we currently don't have a way to communicate that through DMA-buf. For our specific use case at hand we are going to implement something driver specific, but the question is should we have something more generic for this? After all the system memory access pattern is a PCIe extension and as such something generic. Regards, Christian.

2 years, 4 months

[PATCH] dma-buf/poll: Get a file reference for outstanding fence callbacks

by Michel Dänzer

From: Michel Dänzer <mdaenzer(a)redhat.com> This makes sure we don't hit the BUG_ON(dmabuf->cb_in.active || dmabuf->cb_out.active); in dma_buf_release, which could be triggered by user space closing the dma-buf file description while there are outstanding fence callbacks from dma_buf_poll. Cc: stable(a)vger.kernel.org Signed-off-by: Michel Dänzer <mdaenzer(a)redhat.com> --- drivers/dma-buf/dma-buf.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 6c520c9bd93c..ec25498a971f 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -65,12 +65,9 @@ static void dma_buf_release(struct dentry *dentry) BUG_ON(dmabuf->vmapping_counter); /* - * Any fences that a dma-buf poll can wait on should be signaled - * before releasing dma-buf. This is the responsibility of each - * driver that uses the reservation objects. - * - * If you hit this BUG() it means someone dropped their ref to the - * dma-buf while still having pending operation to the buffer. + * If you hit this BUG() it could mean: + * * There's a file reference imbalance in dma_buf_poll / dma_buf_poll_cb or somewhere else + * * dmabuf->cb_in/out.active are non-0 despite no pending fence callback */ BUG_ON(dmabuf->cb_in.active || dmabuf->cb_out.active); @@ -196,6 +193,7 @@ static loff_t dma_buf_llseek(struct file *file, loff_t offset, int whence) static void dma_buf_poll_cb(struct dma_fence *fence, struct dma_fence_cb *cb) { struct dma_buf_poll_cb_t *dcb = (struct dma_buf_poll_cb_t *)cb; + struct dma_buf *dmabuf = container_of(dcb->poll, struct dma_buf, poll); unsigned long flags; spin_lock_irqsave(&dcb->poll->lock, flags); @@ -203,6 +201,8 @@ static void dma_buf_poll_cb(struct dma_fence *fence, struct dma_fence_cb *cb) dcb->active = 0; spin_unlock_irqrestore(&dcb->poll->lock, flags); dma_fence_put(fence); + /* Paired with get_file in dma_buf_poll */ + fput(dmabuf->file); } static bool dma_buf_poll_shared(struct dma_resv *resv, @@ -278,6 +278,9 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll) spin_unlock_irq(&dmabuf->poll.lock); if (events & EPOLLOUT) { + /* Paired with fput in dma_buf_poll_cb */ + get_file(dmabuf->file); + if (!dma_buf_poll_shared(resv, dcb) && !dma_buf_poll_excl(resv, dcb)) /* No callback queued, wake up any other waiters */ @@ -299,6 +302,9 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll) spin_unlock_irq(&dmabuf->poll.lock); if (events & EPOLLIN) { + /* Paired with fput in dma_buf_poll_cb */ + get_file(dmabuf->file); + if (!dma_buf_poll_excl(resv, dcb)) /* No callback queued, wake up any other waiters */ dma_buf_poll_cb(NULL, &dcb->cb); -- 2.32.0

3 years, 1 month

Re: [Linaro-mm-sig] [PATCH] dma-buf: Add support for mapping buffers with DMA attributes

by Daniel Vetter

On Mon, Aug 30, 2021 at 10:39:11AM +0800, guangming.cao(a)mediatek.com wrote: > From: Guangming Cao <Guangming.Cao(a)mediatek.com> > > When mapping the memory represented by a dma-buf into a device's > address space, it might be desireable to map the memory with > certain DMA attributes. Thus, introduce the dma_mapping_attrs > field in the dma_buf_attachment structure so that when > the memory is mapped with dma_buf_map_attachment, it is mapped > with the desired DMA attributes. > > Signed-off-by: Isaac J. Manjarres <isaacm(a)codeaurora.org> > Signed-off-by: Sandeep Patil <sspatil(a)google.com> > Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com> Can you pls include the code that's going to use this here too? At a glance all the attributes you might want to set are supposed to be under the control of the exporter, not the importer. -Daniel > --- > drivers/dma-buf/heaps/cma_heap.c | 6 ++++-- > drivers/dma-buf/heaps/system_heap.c | 6 ++++-- > include/linux/dma-buf.h | 3 +++ > 3 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c > index 0c05b79870f9..2c9feb3bfc3e 100644 > --- a/drivers/dma-buf/heaps/cma_heap.c > +++ b/drivers/dma-buf/heaps/cma_heap.c > @@ -99,9 +99,10 @@ static struct sg_table *cma_heap_map_dma_buf(struct dma_buf_attachment *attachme > { > struct dma_heap_attachment *a = attachment->priv; > struct sg_table *table = &a->table; > + int attrs = attachment->dma_map_attrs; > int ret; > > - ret = dma_map_sgtable(attachment->dev, table, direction, 0); > + ret = dma_map_sgtable(attachment->dev, table, direction, attrs); > if (ret) > return ERR_PTR(-ENOMEM); > a->mapped = true; > @@ -113,9 +114,10 @@ static void cma_heap_unmap_dma_buf(struct dma_buf_attachment *attachment, > enum dma_data_direction direction) > { > struct dma_heap_attachment *a = attachment->priv; > + int attrs = attachment->dma_map_attrs; > > a->mapped = false; > - dma_unmap_sgtable(attachment->dev, table, direction, 0); > + dma_unmap_sgtable(attachment->dev, table, direction, attrs); > } > > static int cma_heap_dma_buf_begin_cpu_access(struct dma_buf *dmabuf, > diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c > index 23a7e74ef966..fc7b1e02988e 100644 > --- a/drivers/dma-buf/heaps/system_heap.c > +++ b/drivers/dma-buf/heaps/system_heap.c > @@ -130,9 +130,10 @@ static struct sg_table *system_heap_map_dma_buf(struct dma_buf_attachment *attac > { > struct dma_heap_attachment *a = attachment->priv; > struct sg_table *table = a->table; > + int attrs = attachment->dma_map_attrs; > int ret; > > - ret = dma_map_sgtable(attachment->dev, table, direction, 0); > + ret = dma_map_sgtable(attachment->dev, table, direction, attrs); > if (ret) > return ERR_PTR(ret); > > @@ -145,9 +146,10 @@ static void system_heap_unmap_dma_buf(struct dma_buf_attachment *attachment, > enum dma_data_direction direction) > { > struct dma_heap_attachment *a = attachment->priv; > + int attrs = attachment->dma_map_attrs; > > a->mapped = false; > - dma_unmap_sgtable(attachment->dev, table, direction, 0); > + dma_unmap_sgtable(attachment->dev, table, direction, attrs); > } > > static int system_heap_dma_buf_begin_cpu_access(struct dma_buf *dmabuf, > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > index efdc56b9d95f..4d650731766e 100644 > --- a/include/linux/dma-buf.h > +++ b/include/linux/dma-buf.h > @@ -379,6 +379,8 @@ struct dma_buf_attach_ops { > * @importer_ops: importer operations for this attachment, if provided > * dma_buf_map/unmap_attachment() must be called with the dma_resv lock held. > * @importer_priv: importer specific attachment data. > + * @dma_map_attrs: DMA attributes to be used when the exporter maps the buffer > + * through dma_buf_map_attachment. > * > * This structure holds the attachment information between the dma_buf buffer > * and its user device(s). The list contains one attachment struct per device > @@ -399,6 +401,7 @@ struct dma_buf_attachment { > const struct dma_buf_attach_ops *importer_ops; > void *importer_priv; > void *priv; > + unsigned long dma_map_attrs; > }; > > /** > -- > 2.17.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 3 months

[PATCH 1/2] dma-buf: nuke DMA_FENCE_TRACE macros v2

by Christian König

Only the DRM GPU scheduler, radeon and amdgpu where using them and they depend on a non existing config option to actually emit some code. v2: keep the signal path as is for now Signed-off-by: Christian König <christian.koenig(a)amd.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 10 +--------- drivers/gpu/drm/radeon/radeon_fence.c | 24 ++++------------------- drivers/gpu/drm/scheduler/sched_fence.c | 18 ++--------------- include/linux/dma-fence.h | 22 --------------------- 4 files changed, 7 insertions(+), 67 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c index 0b1c48590c43..c65994e382bd 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c @@ -246,7 +246,6 @@ bool amdgpu_fence_process(struct amdgpu_ring *ring) struct amdgpu_fence_driver *drv = &ring->fence_drv; struct amdgpu_device *adev = ring->adev; uint32_t seq, last_seq; - int r; do { last_seq = atomic_read(&ring->fence_drv.last_seq); @@ -278,12 +277,7 @@ bool amdgpu_fence_process(struct amdgpu_ring *ring) if (!fence) continue; - r = dma_fence_signal(fence); - if (!r) - DMA_FENCE_TRACE(fence, "signaled from irq context\n"); - else - BUG(); - + dma_fence_signal(fence); dma_fence_put(fence); pm_runtime_mark_last_busy(adev_to_drm(adev)->dev); pm_runtime_put_autosuspend(adev_to_drm(adev)->dev); @@ -639,8 +633,6 @@ static bool amdgpu_fence_enable_signaling(struct dma_fence *f) if (!timer_pending(&ring->fence_drv.fallback_timer)) amdgpu_fence_schedule_fallback(ring); - DMA_FENCE_TRACE(&fence->base, "armed on ring %i!\n", ring->idx); - return true; } diff --git a/drivers/gpu/drm/radeon/radeon_fence.c b/drivers/gpu/drm/radeon/radeon_fence.c index 18f2c2e0dfb3..3f351d222cbb 100644 --- a/drivers/gpu/drm/radeon/radeon_fence.c +++ b/drivers/gpu/drm/radeon/radeon_fence.c @@ -176,18 +176,11 @@ static int radeon_fence_check_signaled(wait_queue_entry_t *wait, unsigned mode, */ seq = atomic64_read(&fence->rdev->fence_drv[fence->ring].last_seq); if (seq >= fence->seq) { - int ret = dma_fence_signal_locked(&fence->base); - - if (!ret) - DMA_FENCE_TRACE(&fence->base, "signaled from irq context\n"); - else - DMA_FENCE_TRACE(&fence->base, "was already signaled\n"); - + dma_fence_signal_locked(&fence->base); radeon_irq_kms_sw_irq_put(fence->rdev, fence->ring); __remove_wait_queue(&fence->rdev->fence_queue, &fence->fence_wake); dma_fence_put(&fence->base); - } else - DMA_FENCE_TRACE(&fence->base, "pending\n"); + } return 0; } @@ -422,8 +415,6 @@ static bool radeon_fence_enable_signaling(struct dma_fence *f) fence->fence_wake.func = radeon_fence_check_signaled; __add_wait_queue(&rdev->fence_queue, &fence->fence_wake); dma_fence_get(f); - - DMA_FENCE_TRACE(&fence->base, "armed on ring %i!\n", fence->ring); return true; } @@ -441,11 +432,7 @@ bool radeon_fence_signaled(struct radeon_fence *fence) return true; if (radeon_fence_seq_signaled(fence->rdev, fence->seq, fence->ring)) { - int ret; - - ret = dma_fence_signal(&fence->base); - if (!ret) - DMA_FENCE_TRACE(&fence->base, "signaled from radeon_fence_signaled\n"); + dma_fence_signal(&fence->base); return true; } return false; @@ -550,7 +537,6 @@ long radeon_fence_wait_timeout(struct radeon_fence *fence, bool intr, long timeo { uint64_t seq[RADEON_NUM_RINGS] = {}; long r; - int r_sig; /* * This function should not be called on !radeon fences. @@ -567,9 +553,7 @@ long radeon_fence_wait_timeout(struct radeon_fence *fence, bool intr, long timeo return r; } - r_sig = dma_fence_signal(&fence->base); - if (!r_sig) - DMA_FENCE_TRACE(&fence->base, "signaled from fence_wait\n"); + dma_fence_signal(&fence->base); return r; } diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c index 69de2c76731f..3736746c47bd 100644 --- a/drivers/gpu/drm/scheduler/sched_fence.c +++ b/drivers/gpu/drm/scheduler/sched_fence.c @@ -50,26 +50,12 @@ static void __exit drm_sched_fence_slab_fini(void) void drm_sched_fence_scheduled(struct drm_sched_fence *fence) { - int ret = dma_fence_signal(&fence->scheduled); - - if (!ret) - DMA_FENCE_TRACE(&fence->scheduled, - "signaled from irq context\n"); - else - DMA_FENCE_TRACE(&fence->scheduled, - "was already signaled\n"); + dma_fence_signal(&fence->scheduled); } void drm_sched_fence_finished(struct drm_sched_fence *fence) { - int ret = dma_fence_signal(&fence->finished); - - if (!ret) - DMA_FENCE_TRACE(&fence->finished, - "signaled from irq context\n"); - else - DMA_FENCE_TRACE(&fence->finished, - "was already signaled\n"); + dma_fence_signal(&fence->finished); } static const char *drm_sched_fence_get_driver_name(struct dma_fence *fence) diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h index 6ffb4b2c6371..4cc119ab272f 100644 --- a/include/linux/dma-fence.h +++ b/include/linux/dma-fence.h @@ -590,26 +590,4 @@ struct dma_fence *dma_fence_get_stub(void); struct dma_fence *dma_fence_allocate_private_stub(void); u64 dma_fence_context_alloc(unsigned num); -#define DMA_FENCE_TRACE(f, fmt, args...) \ - do { \ - struct dma_fence *__ff = (f); \ - if (IS_ENABLED(CONFIG_DMA_FENCE_TRACE)) \ - pr_info("f %llu#%llu: " fmt, \ - __ff->context, __ff->seqno, ##args); \ - } while (0) - -#define DMA_FENCE_WARN(f, fmt, args...) \ - do { \ - struct dma_fence *__ff = (f); \ - pr_warn("f %llu#%llu: " fmt, __ff->context, __ff->seqno,\ - ##args); \ - } while (0) - -#define DMA_FENCE_ERR(f, fmt, args...) \ - do { \ - struct dma_fence *__ff = (f); \ - pr_err("f %llu#%llu: " fmt, __ff->context, __ff->seqno, \ - ##args); \ - } while (0) - #endif /* __LINUX_DMA_FENCE_H */ -- 2.25.1

3 years, 3 months

Re: [Linaro-mm-sig] [PATCH] dma-buf: heaps: remove duplicated cache sync

by Christian König

Am 31.08.21 um 05:44 schrieb guangming.cao(a)mediatek.com: > From: Guangming Cao <Guangming.Cao(a)mediatek.com> > >> Am 30.08.21 um 12:01 schrieb guangming.cao(a)mediatek.com: >>> From: Guangming Cao <Guangming.Cao(a)mediatek.com> >>> >>> Current flow, one dmabuf maybe call cache sync many times if >>> it has beed mapped more than one time. >> Well I'm not an expert on DMA heaps, but this will most likely not work >> correctly. >> > All attachments of one dmabuf will add into a list, I think it means dmabuf > supports map more than one time. Could you tell me more about it? Yes, that's correct and all of those needs to be synced as far as I know. See the dma_sync_sgtable_for_cpu() is intentionally for each SG table given out. >>> Is there any case that attachments of one dmabuf will points to >>> different memory? If not, seems do sync only one time is more better. >> I think that this can happen, yes. >> >> Christian. >> > Seems it's a very special case on Android, if you don't mind, could you > tell me more about it? That might be the case, nevertheless this change here is illegal from the DMA API point of view as far as I can see. Regards, Christian. > >>> Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com> >>> --- >>> drivers/dma-buf/heaps/system_heap.c | 14 ++++++++------ >>> 1 file changed, 8 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c >>> index 23a7e74ef966..909ef652a8c8 100644 >>> --- a/drivers/dma-buf/heaps/system_heap.c >>> +++ b/drivers/dma-buf/heaps/system_heap.c >>> @@ -162,9 +162,10 @@ static int system_heap_dma_buf_begin_cpu_access(struct dma_buf *dmabuf, >>> invalidate_kernel_vmap_range(buffer->vaddr, buffer->len); >>> >>> list_for_each_entry(a, &buffer->attachments, list) { >>> - if (!a->mapped) >>> - continue; >>> - dma_sync_sgtable_for_cpu(a->dev, a->table, direction); >>> + if (a->mapped) { >>> + dma_sync_sgtable_for_cpu(a->dev, a->table, direction); >>> + break; >>> + } >>> } >>> mutex_unlock(&buffer->lock); >>> >>> @@ -183,9 +184,10 @@ static int system_heap_dma_buf_end_cpu_access(struct dma_buf *dmabuf, >>> flush_kernel_vmap_range(buffer->vaddr, buffer->len); >>> >>> list_for_each_entry(a, &buffer->attachments, list) { >>> - if (!a->mapped) >>> - continue; >>> - dma_sync_sgtable_for_device(a->dev, a->table, direction); >>> + if (!a->mapped) { >>> + dma_sync_sgtable_for_device(a->dev, a->table, direction); >>> + break; >>> + } >>> } >>> mutex_unlock(&buffer->lock); >>>

3 years, 3 months

[PATCH v5 20/20] dma-resv: Give the docs a do-over

by Daniel Vetter

Specifically document the new/clarified rules around how the shared fences do not have any ordering requirements against the exclusive fence. But also document all the things a bit better, given how central struct dma_resv to dynamic buffer management the docs have been very inadequat. - Lots more links to other pieces of the puzzle. Unfortunately ttm_buffer_object has no docs, so no links :-( - Explain/complain a bit about dma_resv_locking_ctx(). I still don't like that one, but fixing the ttm call chains is going to be horrible. Plus we want to plug in real slowpath locking when we do that anyway. - Main part of the patch is some actual docs for struct dma_resv. Overall I think we still have a lot of bad naming in this area (e.g. dma_resv.fence is singular, but contains the multiple shared fences), but I think that's more indicative of how the semantics and rules are just not great. Another thing that's real awkard is how chaining exclusive fences right now means direct dma_resv.exclusive_fence pointer access with an rcu_assign_pointer. Not so great either. v2: - Fix a pile of typos (Matt, Jason) - Hammer it in that breaking the rules leads to use-after-free issues around dma-buf sharing (Christian) Reviewed-by: Christian König <christian.koenig(a)amd.com> Cc: Jason Ekstrand <jason(a)jlekstrand.net> Cc: Matthew Auld <matthew.auld(a)intel.com> Reviewed-by: Matthew Auld <matthew.auld(a)intel.com> Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org --- drivers/dma-buf/dma-resv.c | 24 ++++++--- include/linux/dma-buf.h | 7 +++ include/linux/dma-resv.h | 104 +++++++++++++++++++++++++++++++++++-- 3 files changed, 124 insertions(+), 11 deletions(-) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index e744fd87c63c..84fbe60629e3 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -48,6 +48,8 @@ * write operations) or N shared fences (read operations). The RCU * mechanism is used to protect read access to fences from locked * write-side updates. + * + * See struct dma_resv for more details. */ DEFINE_WD_CLASS(reservation_ww_class); @@ -137,7 +139,11 @@ EXPORT_SYMBOL(dma_resv_fini); * @num_fences: number of fences we want to add * * Should be called before dma_resv_add_shared_fence(). Must - * be called with obj->lock held. + * be called with @obj locked through dma_resv_lock(). + * + * Note that the preallocated slots need to be re-reserved if @obj is unlocked + * at any time before calling dma_resv_add_shared_fence(). This is validated + * when CONFIG_DEBUG_MUTEXES is enabled. * * RETURNS * Zero for success, or -errno @@ -234,8 +240,10 @@ EXPORT_SYMBOL(dma_resv_reset_shared_max); * @obj: the reservation object * @fence: the shared fence to add * - * Add a fence to a shared slot, obj->lock must be held, and + * Add a fence to a shared slot, @obj must be locked with dma_resv_lock(), and * dma_resv_reserve_shared() has been called. + * + * See also &dma_resv.fence for a discussion of the semantics. */ void dma_resv_add_shared_fence(struct dma_resv *obj, struct dma_fence *fence) { @@ -278,9 +286,11 @@ EXPORT_SYMBOL(dma_resv_add_shared_fence); /** * dma_resv_add_excl_fence - Add an exclusive fence. * @obj: the reservation object - * @fence: the shared fence to add + * @fence: the exclusive fence to add * - * Add a fence to the exclusive slot. The obj->lock must be held. + * Add a fence to the exclusive slot. @obj must be locked with dma_resv_lock(). + * Note that this function replaces all fences attached to @obj, see also + * &dma_resv.fence_excl for a discussion of the semantics. */ void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence) { @@ -609,9 +619,11 @@ static inline int dma_resv_test_signaled_single(struct dma_fence *passed_fence) * fence * * Callers are not required to hold specific locks, but maybe hold - * dma_resv_lock() already + * dma_resv_lock() already. + * * RETURNS - * true if all fences signaled, else false + * + * True if all fences signaled, else false. */ bool dma_resv_test_signaled(struct dma_resv *obj, bool test_all) { diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index 678b2006be78..fc62b5f9980c 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -420,6 +420,13 @@ struct dma_buf { * - Dynamic importers should set fences for any access that they can't * disable immediately from their &dma_buf_attach_ops.move_notify * callback. + * + * IMPORTANT: + * + * All drivers must obey the struct dma_resv rules, specifically the + * rules for updating fences, see &dma_resv.fence_excl and + * &dma_resv.fence. If these dependency rules are broken access tracking + * can be lost resulting in use after free issues. */ struct dma_resv *resv; diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h index e1ca2080a1ff..9100dd3dc21f 100644 --- a/include/linux/dma-resv.h +++ b/include/linux/dma-resv.h @@ -62,16 +62,90 @@ struct dma_resv_list { /** * struct dma_resv - a reservation object manages fences for a buffer - * @lock: update side lock - * @seq: sequence count for managing RCU read-side synchronization - * @fence_excl: the exclusive fence, if there is one currently - * @fence: list of current shared fences + * + * There are multiple uses for this, with sometimes slightly different rules in + * how the fence slots are used. + * + * One use is to synchronize cross-driver access to a struct dma_buf, either for + * dynamic buffer management or just to handle implicit synchronization between + * different users of the buffer in userspace. See &dma_buf.resv for a more + * in-depth discussion. + * + * The other major use is to manage access and locking within a driver in a + * buffer based memory manager. struct ttm_buffer_object is the canonical + * example here, since this is where reservation objects originated from. But + * use in drivers is spreading and some drivers also manage struct + * drm_gem_object with the same scheme. */ struct dma_resv { + /** + * @lock: + * + * Update side lock. Don't use directly, instead use the wrapper + * functions like dma_resv_lock() and dma_resv_unlock(). + * + * Drivers which use the reservation object to manage memory dynamically + * also use this lock to protect buffer object state like placement, + * allocation policies or throughout command submission. + */ struct ww_mutex lock; + + /** + * @seq: + * + * Sequence count for managing RCU read-side synchronization, allows + * read-only access to @fence_excl and @fence while ensuring we take a + * consistent snapshot. + */ seqcount_ww_mutex_t seq; + /** + * @fence_excl: + * + * The exclusive fence, if there is one currently. + * + * There are two ways to update this fence: + * + * - First by calling dma_resv_add_excl_fence(), which replaces all + * fences attached to the reservation object. To guarantee that no + * fences are lost, this new fence must signal only after all previous + * fences, both shared and exclusive, have signalled. In some cases it + * is convenient to achieve that by attaching a struct dma_fence_array + * with all the new and old fences. + * + * - Alternatively the fence can be set directly, which leaves the + * shared fences unchanged. To guarantee that no fences are lost, this + * new fence must signal only after the previous exclusive fence has + * signalled. Since the shared fences are staying intact, it is not + * necessary to maintain any ordering against those. If semantically + * only a new access is added without actually treating the previous + * one as a dependency the exclusive fences can be strung together + * using struct dma_fence_chain. + * + * Note that actual semantics of what an exclusive or shared fence mean + * is defined by the user, for reservation objects shared across drivers + * see &dma_buf.resv. + */ struct dma_fence __rcu *fence_excl; + + /** + * @fence: + * + * List of current shared fences. + * + * There are no ordering constraints of shared fences against the + * exclusive fence slot. If a waiter needs to wait for all access, it + * has to wait for both sets of fences to signal. + * + * A new fence is added by calling dma_resv_add_shared_fence(). Since + * this often needs to be done past the point of no return in command + * submission it cannot fail, and therefore sufficient slots need to be + * reserved by calling dma_resv_reserve_shared(). + * + * Note that actual semantics of what an exclusive or shared fence mean + * is defined by the user, for reservation objects shared across drivers + * see &dma_buf.resv. + */ struct dma_resv_list __rcu *fence; }; @@ -98,6 +172,13 @@ static inline void dma_resv_reset_shared_max(struct dma_resv *obj) {} * undefined order, a #ww_acquire_ctx is passed to unwind if a cycle * is detected. See ww_mutex_lock() and ww_acquire_init(). A reservation * object may be locked by itself by passing NULL as @ctx. + * + * When a die situation is indicated by returning -EDEADLK all locks held by + * @ctx must be unlocked and then dma_resv_lock_slow() called on @obj. + * + * Unlocked by calling dma_resv_unlock(). + * + * See also dma_resv_lock_interruptible() for the interruptible variant. */ static inline int dma_resv_lock(struct dma_resv *obj, struct ww_acquire_ctx *ctx) @@ -119,6 +200,12 @@ static inline int dma_resv_lock(struct dma_resv *obj, * undefined order, a #ww_acquire_ctx is passed to unwind if a cycle * is detected. See ww_mutex_lock() and ww_acquire_init(). A reservation * object may be locked by itself by passing NULL as @ctx. + * + * When a die situation is indicated by returning -EDEADLK all locks held by + * @ctx must be unlocked and then dma_resv_lock_slow_interruptible() called on + * @obj. + * + * Unlocked by calling dma_resv_unlock(). */ static inline int dma_resv_lock_interruptible(struct dma_resv *obj, struct ww_acquire_ctx *ctx) @@ -134,6 +221,8 @@ static inline int dma_resv_lock_interruptible(struct dma_resv *obj, * Acquires the reservation object after a die case. This function * will sleep until the lock becomes available. See dma_resv_lock() as * well. + * + * See also dma_resv_lock_slow_interruptible() for the interruptible variant. */ static inline void dma_resv_lock_slow(struct dma_resv *obj, struct ww_acquire_ctx *ctx) @@ -167,7 +256,7 @@ static inline int dma_resv_lock_slow_interruptible(struct dma_resv *obj, * if they overlap with a writer. * * Also note that since no context is provided, no deadlock protection is - * possible. + * possible, which is also not needed for a trylock. * * Returns true if the lock was acquired, false otherwise. */ @@ -193,6 +282,11 @@ static inline bool dma_resv_is_locked(struct dma_resv *obj) * * Returns the context used to lock a reservation object or NULL if no context * was used or the object is not locked at all. + * + * WARNING: This interface is pretty horrible, but TTM needs it because it + * doesn't pass the struct ww_acquire_ctx around in some very long callchains. + * Everyone else just uses it to check whether they're holding a reservation or + * not. */ static inline struct ww_acquire_ctx *dma_resv_locking_ctx(struct dma_resv *obj) { -- 2.32.0

3 years, 3 months

Re: [Linaro-mm-sig] [PATCH] dma-buf: heaps: remove duplicated cache sync

by Christian König

Am 30.08.21 um 12:01 schrieb guangming.cao(a)mediatek.com: > From: Guangming Cao <Guangming.Cao(a)mediatek.com> > > Current flow, one dmabuf maybe call cache sync many times if > it has beed mapped more than one time. Well I'm not an expert on DMA heaps, but this will most likely not work correctly. > Is there any case that attachments of one dmabuf will points to > different memory? If not, seems do sync only one time is more better. I think that this can happen, yes. Christian. > > Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com> > --- > drivers/dma-buf/heaps/system_heap.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c > index 23a7e74ef966..909ef652a8c8 100644 > --- a/drivers/dma-buf/heaps/system_heap.c > +++ b/drivers/dma-buf/heaps/system_heap.c > @@ -162,9 +162,10 @@ static int system_heap_dma_buf_begin_cpu_access(struct dma_buf *dmabuf, > invalidate_kernel_vmap_range(buffer->vaddr, buffer->len); > > list_for_each_entry(a, &buffer->attachments, list) { > - if (!a->mapped) > - continue; > - dma_sync_sgtable_for_cpu(a->dev, a->table, direction); > + if (a->mapped) { > + dma_sync_sgtable_for_cpu(a->dev, a->table, direction); > + break; > + } > } > mutex_unlock(&buffer->lock); > > @@ -183,9 +184,10 @@ static int system_heap_dma_buf_end_cpu_access(struct dma_buf *dmabuf, > flush_kernel_vmap_range(buffer->vaddr, buffer->len); > > list_for_each_entry(a, &buffer->attachments, list) { > - if (!a->mapped) > - continue; > - dma_sync_sgtable_for_device(a->dev, a->table, direction); > + if (!a->mapped) { > + dma_sync_sgtable_for_device(a->dev, a->table, direction); > + break; > + } > } > mutex_unlock(&buffer->lock); >

3 years, 3 months

[PATCH v5 12/20] drm/msm: Use scheduler dependency handling

by Daniel Vetter

drm_sched_job_init is already at the right place, so this boils down to deleting code. Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Cc: Rob Clark <robdclark(a)gmail.com> Cc: Sean Paul <sean(a)poorly.run> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: linux-arm-msm(a)vger.kernel.org Cc: freedreno(a)lists.freedesktop.org Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org --- drivers/gpu/drm/msm/msm_gem.h | 5 ----- drivers/gpu/drm/msm/msm_gem_submit.c | 19 +++++-------------- drivers/gpu/drm/msm/msm_ringbuffer.c | 12 ------------ 3 files changed, 5 insertions(+), 31 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h index f9e3ffb2309a..8bf0ac707fd7 100644 --- a/drivers/gpu/drm/msm/msm_gem.h +++ b/drivers/gpu/drm/msm/msm_gem.h @@ -312,11 +312,6 @@ struct msm_gem_submit { struct ww_acquire_ctx ticket; uint32_t seqno; /* Sequence number of the submit on the ring */ - /* Array of struct dma_fence * to block on before submitting this job. - */ - struct xarray deps; - unsigned long last_dep; - /* Hw fence, which is created when the scheduler executes the job, and * is signaled when the hw finishes (via seqno write from cmdstream) */ diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c index 96cea0ba4cfd..fb5a2eab27a2 100644 --- a/drivers/gpu/drm/msm/msm_gem_submit.c +++ b/drivers/gpu/drm/msm/msm_gem_submit.c @@ -52,8 +52,6 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev, return ERR_PTR(ret); } - xa_init_flags(&submit->deps, XA_FLAGS_ALLOC); - kref_init(&submit->ref); submit->dev = dev; submit->aspace = queue->ctx->aspace; @@ -72,8 +70,6 @@ void __msm_gem_submit_destroy(struct kref *kref) { struct msm_gem_submit *submit = container_of(kref, struct msm_gem_submit, ref); - unsigned long index; - struct dma_fence *fence; unsigned i; if (submit->fence_id) { @@ -82,12 +78,6 @@ void __msm_gem_submit_destroy(struct kref *kref) mutex_unlock(&submit->queue->lock); } - xa_for_each (&submit->deps, index, fence) { - dma_fence_put(fence); - } - - xa_destroy(&submit->deps); - dma_fence_put(submit->user_fence); dma_fence_put(submit->hw_fence); @@ -343,8 +333,9 @@ static int submit_fence_sync(struct msm_gem_submit *submit, bool no_implicit) if (no_implicit) continue; - ret = drm_gem_fence_array_add_implicit(&submit->deps, obj, - write); + ret = drm_sched_job_add_implicit_dependencies(&submit->base, + obj, + write); if (ret) break; } @@ -588,7 +579,7 @@ static struct drm_syncobj **msm_parse_deps(struct msm_gem_submit *submit, if (ret) break; - ret = drm_gem_fence_array_add(&submit->deps, fence); + ret = drm_sched_job_add_dependency(&submit->base, fence); if (ret) break; @@ -798,7 +789,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, goto out_unlock; } - ret = drm_gem_fence_array_add(&submit->deps, in_fence); + ret = drm_sched_job_add_dependency(&submit->base, in_fence); if (ret) goto out_unlock; } diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.c b/drivers/gpu/drm/msm/msm_ringbuffer.c index bd54c1412649..652b1dedd7c1 100644 --- a/drivers/gpu/drm/msm/msm_ringbuffer.c +++ b/drivers/gpu/drm/msm/msm_ringbuffer.c @@ -11,17 +11,6 @@ static uint num_hw_submissions = 8; MODULE_PARM_DESC(num_hw_submissions, "The max # of jobs to write into ringbuffer (default 8)"); module_param(num_hw_submissions, uint, 0600); -static struct dma_fence *msm_job_dependency(struct drm_sched_job *job, - struct drm_sched_entity *s_entity) -{ - struct msm_gem_submit *submit = to_msm_submit(job); - - if (!xa_empty(&submit->deps)) - return xa_erase(&submit->deps, submit->last_dep++); - - return NULL; -} - static struct dma_fence *msm_job_run(struct drm_sched_job *job) { struct msm_gem_submit *submit = to_msm_submit(job); @@ -52,7 +41,6 @@ static void msm_job_free(struct drm_sched_job *job) } const struct drm_sched_backend_ops msm_sched_ops = { - .dependency = msm_job_dependency, .run_job = msm_job_run, .free_job = msm_job_free }; -- 2.32.0

3 years, 3 months

[PATCH v5 02/20] drm/msm: Fix drm/sched point of no return rules

by Daniel Vetter

Originally drm_sched_job_init was the point of no return, after which drivers must submit a job. I've split that up, which allows us to fix this issue pretty easily. Only thing we have to take care of is to not skip to error paths after that. Other drivers do this the same for out-fence and similar things. Fixes: 1d8a5ca436ee ("drm/msm: Conversion to drm scheduler") Cc: Rob Clark <robdclark(a)chromium.org> Cc: Rob Clark <robdclark(a)gmail.com> Cc: Sean Paul <sean(a)poorly.run> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: linux-arm-msm(a)vger.kernel.org Cc: dri-devel(a)lists.freedesktop.org Cc: freedreno(a)lists.freedesktop.org Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> --- drivers/gpu/drm/msm/msm_gem_submit.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c index 6d6c44f0e1f3..d0ed4ddc509e 100644 --- a/drivers/gpu/drm/msm/msm_gem_submit.c +++ b/drivers/gpu/drm/msm/msm_gem_submit.c @@ -52,9 +52,6 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev, return ERR_PTR(ret); } - /* FIXME: this is way too early */ - drm_sched_job_arm(&job->base); - xa_init_flags(&submit->deps, XA_FLAGS_ALLOC); kref_init(&submit->ref); @@ -883,6 +880,9 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, submit->user_fence = dma_fence_get(&submit->base.s_fence->finished); + /* point of no return, we _have_ to submit no matter what */ + drm_sched_job_arm(&submit->base); + /* * Allocate an id which can be used by WAIT_FENCE ioctl to map back * to the underlying fence. @@ -892,17 +892,16 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, if (submit->fence_id < 0) { ret = submit->fence_id = 0; submit->fence_id = 0; - goto out; } - if (args->flags & MSM_SUBMIT_FENCE_FD_OUT) { + if (ret == 0 && args->flags & MSM_SUBMIT_FENCE_FD_OUT) { struct sync_file *sync_file = sync_file_create(submit->user_fence); if (!sync_file) { ret = -ENOMEM; - goto out; + } else { + fd_install(out_fence_fd, sync_file->file); + args->fence_fd = out_fence_fd; } - fd_install(out_fence_fd, sync_file->file); - args->fence_fd = out_fence_fd; } submit_attach_object_fences(submit); -- 2.32.0

3 years, 4 months

Re: [Linaro-mm-sig] [PATCH] parisc/parport_gsc: switch from 'pci_' to 'dma_' API

by Robin Murphy

On 2021-08-23 22:30, Christophe JAILLET wrote: > The wrappers in include/linux/pci-dma-compat.h should go away. > > The patch has been generated with the coccinelle script below. > > @@ > expression e1, e2, e3, e4; > @@ > - pci_free_consistent(e1, e2, e3, e4) > + dma_free_coherent(&e1->dev, e2, e3, e4) > > Signed-off-by: Christophe JAILLET <christophe.jaillet(a)wanadoo.fr> > --- > If needed, see post from Christoph Hellwig on the kernel-janitors ML: > https://marc.info/?l=kernel-janitors&m=158745678307186&w=4 > > This has *NOT* been compile tested because I don't have the needed > configuration. > ssdfs > --- > drivers/parport/parport_gsc.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/parport/parport_gsc.c b/drivers/parport/parport_gsc.c > index 1e43b3f399a8..db912fa6b6df 100644 > --- a/drivers/parport/parport_gsc.c > +++ b/drivers/parport/parport_gsc.c > @@ -390,9 +390,8 @@ static int __exit parport_remove_chip(struct parisc_device *dev) > if (p->irq != PARPORT_IRQ_NONE) > free_irq(p->irq, p); > if (priv->dma_buf) > - pci_free_consistent(priv->dev, PAGE_SIZE, > - priv->dma_buf, > - priv->dma_handle); > + dma_free_coherent(&priv->dev->dev, PAGE_SIZE, > + priv->dma_buf, priv->dma_handle); Hmm, seeing a free on its own made me wonder where the corresponding alloc was, but on closer inspection it seems there isn't one. AFAICS priv->dma_buf is only ever assigned with NULL (and priv->dev doesn't seem to be assigned at all), so this could likely just be removed. In fact it looks like all the references to DMA in this driver are just copy-paste from parport_pc and unused. Robin. > kfree (p->private_data); > parport_put_port(p); > kfree (ops); /* hope no-one cached it */ >

3 years, 4 months

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig August 2021