Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

18 participants
2944 discussions

Re: [Linaro-mm-sig] [PATCH 00/15] iio: buffer-dma: write() and new DMABUF based API

by Daniel Vetter

On Mon, Nov 15, 2021 at 02:19:10PM +0000, Paul Cercueil wrote: > Hi Jonathan, > > This patchset introduces a new userspace interface based on DMABUF > objects, to complement the existing fileio based API. > > The advantage of this DMABUF based interface vs. the fileio > interface, is that it avoids an extra copy of the data between the > kernel and userspace. This is particularly userful for high-speed > devices which produce several megabytes or even gigabytes of data per > second. > > The first few patches [01/15] to [03/15] are not really related, but > allow to reduce the size of the patches that introduce the new API. > > Patch [04/15] to [06/15] enables write() support to the buffer-dma > implementation of the buffer API, to continue the work done by > Mihail Chindris. > > Patches [07/15] to [12/15] introduce the new DMABUF based API. > > Patches [13/15] and [14/15] add support for cyclic buffers, only through > the new API. A cyclic buffer will be repeated on the output until the > buffer is disabled. > > Patch [15/15] adds documentation about the new API. > > For now, the API allows you to alloc DMABUF objects and mmap() them to > read or write the samples. It does not yet allow to import DMABUFs > parented to other subsystems, but that should eventually be possible > once it's wired. > > This patchset is inspired by the "mmap interface" that was previously > submitted by Alexandru Ardelean and Lars-Peter Clausen, so it would be > great if I could get a review from you guys. Alexandru's commit was > signed with his @analog.com address but he doesn't work at ADI anymore, > so I believe I'll need him to sign with a new email. Why dma-buf? dma-buf looks like something super generic and useful, until you realize that there's a metric ton of gpu/accelerator bagage piled in. So unless buffer sharing with a gpu/video/accel/whatever device is the goal here, and it's just for a convenient way to get at buffer handles, this doesn't sound like a good idea. Also if the idea is to this with gpus/accelerators then I'd really like to see the full thing, since most likely at that point you also want dma_fence. And once we talk dma_fence things get truly horrible from a locking pov :-( Or well, just highly constrained and I get to review what iio is doing with these buffers to make sure it all fits. Cheers, Daniel > > Cheers, > -Paul > > Alexandru Ardelean (1): > iio: buffer-dma: split iio_dma_buffer_fileio_free() function > > Paul Cercueil (14): > iio: buffer-dma: Get rid of incoming/outgoing queues > iio: buffer-dma: Remove unused iio_buffer_block struct > iio: buffer-dma: Use round_down() instead of rounddown() > iio: buffer-dma: Enable buffer write support > iio: buffer-dmaengine: Support specifying buffer direction > iio: buffer-dmaengine: Enable write support > iio: core: Add new DMABUF interface infrastructure > iio: buffer-dma: Use DMABUFs instead of custom solution > iio: buffer-dma: Implement new DMABUF based userspace API > iio: buffer-dma: Boost performance using write-combine cache setting > iio: buffer-dmaengine: Support new DMABUF based userspace API > iio: core: Add support for cyclic buffers > iio: buffer-dmaengine: Add support for cyclic buffers > Documentation: iio: Document high-speed DMABUF based API > > Documentation/driver-api/dma-buf.rst | 2 + > Documentation/iio/dmabuf_api.rst | 94 +++ > Documentation/iio/index.rst | 2 + > drivers/iio/adc/adi-axi-adc.c | 3 +- > drivers/iio/buffer/industrialio-buffer-dma.c | 670 ++++++++++++++---- > .../buffer/industrialio-buffer-dmaengine.c | 42 +- > drivers/iio/industrialio-buffer.c | 49 ++ > include/linux/iio/buffer-dma.h | 43 +- > include/linux/iio/buffer-dmaengine.h | 5 +- > include/linux/iio/buffer_impl.h | 8 + > include/uapi/linux/iio/buffer.h | 30 + > 11 files changed, 783 insertions(+), 165 deletions(-) > create mode 100644 Documentation/iio/dmabuf_api.rst > > -- > 2.33.0 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 7 months

Re: [Linaro-mm-sig] [PATCH] dma-buf: add DMA_BUF_IOCTL_SYNC_PARTIAL support

by Christian König

Am 13.11.21 um 07:22 schrieb Jianqun Xu: > Add DMA_BUF_IOCTL_SYNC_PARTIAL support for user to sync dma-buf with > offset and len. You have not given an use case for this so it is a bit hard to review. And from the existing use cases I don't see why this should be necessary. Even worse from the existing backend implementation I don't even see how drivers should be able to fulfill this semantics. Please explain further, Christian. > > Change-Id: I03d2d2e10e48d32aa83c31abade57e0931e1be49 > Signed-off-by: Jianqun Xu <jay.xu(a)rock-chips.com> > --- > drivers/dma-buf/dma-buf.c | 42 ++++++++++++++++++++++++++++++++++++ > include/uapi/linux/dma-buf.h | 8 +++++++ > 2 files changed, 50 insertions(+) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index d9948d58b3f4..78f37f7c3462 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -392,6 +392,7 @@ static long dma_buf_ioctl(struct file *file, > { > struct dma_buf *dmabuf; > struct dma_buf_sync sync; > + struct dma_buf_sync_partial sync_p; > enum dma_data_direction direction; > int ret; > > @@ -430,6 +431,47 @@ static long dma_buf_ioctl(struct file *file, > case DMA_BUF_SET_NAME_B: > return dma_buf_set_name(dmabuf, (const char __user *)arg); > > + case DMA_BUF_IOCTL_SYNC_PARTIAL: > + if (copy_from_user(&sync_p, (void __user *) arg, sizeof(sync_p))) > + return -EFAULT; > + > + if (sync_p.len == 0) > + return 0; > + > + if ((sync_p.offset % cache_line_size()) || (sync_p.len % cache_line_size())) > + return -EINVAL; > + > + if (sync_p.len > dmabuf->size || sync_p.offset > dmabuf->size - sync_p.len) > + return -EINVAL; > + > + if (sync_p.flags & ~DMA_BUF_SYNC_VALID_FLAGS_MASK) > + return -EINVAL; > + > + switch (sync_p.flags & DMA_BUF_SYNC_RW) { > + case DMA_BUF_SYNC_READ: > + direction = DMA_FROM_DEVICE; > + break; > + case DMA_BUF_SYNC_WRITE: > + direction = DMA_TO_DEVICE; > + break; > + case DMA_BUF_SYNC_RW: > + direction = DMA_BIDIRECTIONAL; > + break; > + default: > + return -EINVAL; > + } > + > + if (sync_p.flags & DMA_BUF_SYNC_END) > + ret = dma_buf_end_cpu_access_partial(dmabuf, direction, > + sync_p.offset, > + sync_p.len); > + else > + ret = dma_buf_begin_cpu_access_partial(dmabuf, direction, > + sync_p.offset, > + sync_p.len); > + > + return ret; > + > default: > return -ENOTTY; > } > diff --git a/include/uapi/linux/dma-buf.h b/include/uapi/linux/dma-buf.h > index 7f30393b92c3..6236c644624d 100644 > --- a/include/uapi/linux/dma-buf.h > +++ b/include/uapi/linux/dma-buf.h > @@ -47,4 +47,12 @@ struct dma_buf_sync { > #define DMA_BUF_SET_NAME_A _IOW(DMA_BUF_BASE, 1, u32) > #define DMA_BUF_SET_NAME_B _IOW(DMA_BUF_BASE, 1, u64) > > +struct dma_buf_sync_partial { > + __u64 flags; > + __u32 offset; > + __u32 len; > +}; > + > +#define DMA_BUF_IOCTL_SYNC_PARTIAL _IOW(DMA_BUF_BASE, 2, struct dma_buf_sync_partial) > + > #endif

3 years, 7 months

Re: [Linaro-mm-sig] AUTOSEL series truncated was -- Re: [PATCH AUTOSEL 5.15 001/146] dma-buf: WARN on dmabuf release with pending attachments

by Randy Dunlap

On 11/8/21 11:54 PM, Pavel Machek wrote: > Hi! > > This series is truncated .. I only got first patches. Similary, 5.10 > series is truncated, [PATCH AUTOSEL 5.10 035/101] media: s5p-mfc: Add > checking to s5p_mfc_probe... is last one I got. > > I got all the patches before that, so I believe it is not problem on > my side, but I'd not mind someone confirming they are seeing the same > problem... Yes, several of the patch series were incomplete for me also... -- ~Randy

3 years, 7 months

[PATCH 0/2] drm/msm: wait_fence fixes

by Rob Clark

From: Rob Clark <robdclark(a)chromium.org> A couple of wait_fence related fixes. Rob Clark (2): drm/msm: Fix wait_fence submitqueue leak drm/msm: Restore error return on invalid fence drivers/gpu/drm/msm/msm_drv.c | 49 ++++++++++++++++++---------- drivers/gpu/drm/msm/msm_gem_submit.c | 1 + drivers/gpu/drm/msm/msm_gpu.h | 3 ++ 3 files changed, 36 insertions(+), 17 deletions(-) -- 2.31.1

3 years, 8 months

Re: [Linaro-mm-sig] [Freedreno] [PATCH v4 07/13] drm/msm: Track "seqno" fences by idr

by Rob Clark

On Thu, Nov 11, 2021 at 7:54 AM Akhil P Oommen <akhilpo(a)codeaurora.org> wrote: > > On 11/10/2021 10:25 PM, Rob Clark wrote: > > On Wed, Nov 10, 2021 at 7:28 AM Akhil P Oommen <akhilpo(a)codeaurora.org> wrote: > >> > >> On 7/28/2021 6:36 AM, Rob Clark wrote: > >>> From: Rob Clark <robdclark(a)chromium.org> > >>> > >>> Previously the (non-fd) fence returned from submit ioctl was a raw > >>> seqno, which is scoped to the ring. But from UABI standpoint, the > >>> ioctls related to seqno fences all specify a submitqueue. We can > >>> take advantage of that to replace the seqno fences with a cyclic idr > >>> handle. > >>> > >>> This is in preperation for moving to drm scheduler, at which point > >>> the submit ioctl will return after queuing the submit job to the > >>> scheduler, but before the submit is written into the ring (and > >>> therefore before a ring seqno has been assigned). Which means we > >>> need to replace the dma_fence that userspace may need to wait on > >>> with a scheduler fence. > >>> > >>> Signed-off-by: Rob Clark <robdclark(a)chromium.org> > >>> Acked-by: Christian König <christian.koenig(a)amd.com> > >>> --- > >>> drivers/gpu/drm/msm/msm_drv.c | 30 +++++++++++++++++-- > >>> drivers/gpu/drm/msm/msm_fence.c | 42 --------------------------- > >>> drivers/gpu/drm/msm/msm_fence.h | 3 -- > >>> drivers/gpu/drm/msm/msm_gem.h | 1 + > >>> drivers/gpu/drm/msm/msm_gem_submit.c | 23 ++++++++++++++- > >>> drivers/gpu/drm/msm/msm_gpu.h | 5 ++++ > >>> drivers/gpu/drm/msm/msm_submitqueue.c | 5 ++++ > >>> 7 files changed, 61 insertions(+), 48 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > >>> index 9b8fa2ad0d84..1594ae39d54f 100644 > >>> --- a/drivers/gpu/drm/msm/msm_drv.c > >>> +++ b/drivers/gpu/drm/msm/msm_drv.c > >>> @@ -911,6 +911,7 @@ static int msm_ioctl_wait_fence(struct drm_device *dev, void *data, > >>> ktime_t timeout = to_ktime(args->timeout); > >>> struct msm_gpu_submitqueue *queue; > >>> struct msm_gpu *gpu = priv->gpu; > >>> + struct dma_fence *fence; > >>> int ret; > >>> > >>> if (args->pad) { > >>> @@ -925,10 +926,35 @@ static int msm_ioctl_wait_fence(struct drm_device *dev, void *data, > >>> if (!queue) > >>> return -ENOENT; > >>> > >>> - ret = msm_wait_fence(gpu->rb[queue->prio]->fctx, args->fence, &timeout, > >>> - true); > >>> + /* > >>> + * Map submitqueue scoped "seqno" (which is actually an idr key) > >>> + * back to underlying dma-fence > >>> + * > >>> + * The fence is removed from the fence_idr when the submit is > >>> + * retired, so if the fence is not found it means there is nothing > >>> + * to wait for > >>> + */ > >>> + ret = mutex_lock_interruptible(&queue->lock); > >>> + if (ret) > >>> + return ret; > >>> + fence = idr_find(&queue->fence_idr, args->fence); > >>> + if (fence) > >>> + fence = dma_fence_get_rcu(fence); > >>> + mutex_unlock(&queue->lock); > >>> + > >>> + if (!fence) > >>> + return 0; > >>> > >>> + ret = dma_fence_wait_timeout(fence, true, timeout_to_jiffies(&timeout)); > >>> + if (ret == 0) { > >>> + ret = -ETIMEDOUT; > >>> + } else if (ret != -ERESTARTSYS) { > >>> + ret = 0; > >>> + } > >>> + > >>> + dma_fence_put(fence); > >>> msm_submitqueue_put(queue); > >>> + > >>> return ret; > >>> } > >>> > >>> diff --git a/drivers/gpu/drm/msm/msm_fence.c b/drivers/gpu/drm/msm/msm_fence.c > >>> index b92a9091a1e2..f2cece542c3f 100644 > >>> --- a/drivers/gpu/drm/msm/msm_fence.c > >>> +++ b/drivers/gpu/drm/msm/msm_fence.c > >>> @@ -24,7 +24,6 @@ msm_fence_context_alloc(struct drm_device *dev, volatile uint32_t *fenceptr, > >>> strncpy(fctx->name, name, sizeof(fctx->name)); > >>> fctx->context = dma_fence_context_alloc(1); > >>> fctx->fenceptr = fenceptr; > >>> - init_waitqueue_head(&fctx->event); > >>> spin_lock_init(&fctx->spinlock); > >>> > >>> return fctx; > >>> @@ -45,53 +44,12 @@ static inline bool fence_completed(struct msm_fence_context *fctx, uint32_t fenc > >>> (int32_t)(*fctx->fenceptr - fence) >= 0; > >>> } > >>> > >>> -/* legacy path for WAIT_FENCE ioctl: */ > >>> -int msm_wait_fence(struct msm_fence_context *fctx, uint32_t fence, > >>> - ktime_t *timeout, bool interruptible) > >>> -{ > >>> - int ret; > >>> - > >>> - if (fence > fctx->last_fence) { > >>> - DRM_ERROR_RATELIMITED("%s: waiting on invalid fence: %u (of %u)\n", > >>> - fctx->name, fence, fctx->last_fence); > >>> - return -EINVAL; > >> > >> Rob, we changed this pre-existing behaviour in this patch. Now, when > >> userspace tries to wait on a future fence, we don't return an error. > >> > >> I just want to check if this was accidental or not? > > > > Hmm, perhaps we should do this to restore the previous behavior: > > > > ------------- > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > > index 73e827641024..3dd6da56eae6 100644 > > --- a/drivers/gpu/drm/msm/msm_drv.c > > +++ b/drivers/gpu/drm/msm/msm_drv.c > > @@ -1000,8 +1000,12 @@ static int msm_ioctl_wait_fence(struct > > drm_device *dev, void *data, > > fence = dma_fence_get_rcu(fence); > > mutex_unlock(&queue->lock); > > > > - if (!fence) > > - return 0; > > + if (!fence) { > > + struct msm_fence_context *fctx = gpu->rb[queue->ring_nr]->fctx; > > + DRM_ERROR_RATELIMITED("%s: waiting on invalid fence: > > %u (of %u)\n", > > + fctx->name, fence, fctx->last_fence); > > + return -EINVAL; > > + } > > With this, when userspace tries to wait on a fence which is already > retired, it gets -EINVAL instead of success. Will this break userspace? Oh, right, we definitely don't want that.. I guess that was the reason for the original logic. I have a different idea.. will send a patch in a bit. BR, -R > -Akhil. > > > > > ret = dma_fence_wait_timeout(fence, true, timeout_to_jiffies(&timeout)); > > if (ret == 0) { > > ------------- > > > > BR, > > -R > > > >> -Akhil. > >> > >>> - } > >>> - > >>> - if (!timeout) { > >>> - /* no-wait: */ > >>> - ret = fence_completed(fctx, fence) ? 0 : -EBUSY; > >>> - } else { > >>> - unsigned long remaining_jiffies = timeout_to_jiffies(timeout); > >>> - > >>> - if (interruptible) > >>> - ret = wait_event_interruptible_timeout(fctx->event, > >>> - fence_completed(fctx, fence), > >>> - remaining_jiffies); > >>> - else > >>> - ret = wait_event_timeout(fctx->event, > >>> - fence_completed(fctx, fence), > >>> - remaining_jiffies); > >>> - > >>> - if (ret == 0) { > >>> - DBG("timeout waiting for fence: %u (completed: %u)", > >>> - fence, fctx->completed_fence); > >>> - ret = -ETIMEDOUT; > >>> - } else if (ret != -ERESTARTSYS) { > >>> - ret = 0; > >>> - } > >>> - } > >>> - > >>> - return ret; > >>> -} > >>> - > >>> /* called from workqueue */ > >>> void msm_update_fence(struct msm_fence_context *fctx, uint32_t fence) > >>> { > >>> spin_lock(&fctx->spinlock); > >>> fctx->completed_fence = max(fence, fctx->completed_fence); > >>> spin_unlock(&fctx->spinlock); > >>> - > >>> - wake_up_all(&fctx->event); > >>> } > >>> > >>> struct msm_fence { > >>> diff --git a/drivers/gpu/drm/msm/msm_fence.h b/drivers/gpu/drm/msm/msm_fence.h > >>> index 6ab97062ff1a..4783db528bcc 100644 > >>> --- a/drivers/gpu/drm/msm/msm_fence.h > >>> +++ b/drivers/gpu/drm/msm/msm_fence.h > >>> @@ -49,7 +49,6 @@ struct msm_fence_context { > >>> */ > >>> volatile uint32_t *fenceptr; > >>> > >>> - wait_queue_head_t event; > >>> spinlock_t spinlock; > >>> }; > >>> > >>> @@ -57,8 +56,6 @@ struct msm_fence_context * msm_fence_context_alloc(struct drm_device *dev, > >>> volatile uint32_t *fenceptr, const char *name); > >>> void msm_fence_context_free(struct msm_fence_context *fctx); > >>> > >>> -int msm_wait_fence(struct msm_fence_context *fctx, uint32_t fence, > >>> - ktime_t *timeout, bool interruptible); > >>> void msm_update_fence(struct msm_fence_context *fctx, uint32_t fence); > >>> > >>> struct dma_fence * msm_fence_alloc(struct msm_fence_context *fctx); > >>> diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h > >>> index da3af702a6c8..e0579abda5b9 100644 > >>> --- a/drivers/gpu/drm/msm/msm_gem.h > >>> +++ b/drivers/gpu/drm/msm/msm_gem.h > >>> @@ -320,6 +320,7 @@ struct msm_gem_submit { > >>> struct ww_acquire_ctx ticket; > >>> uint32_t seqno; /* Sequence number of the submit on the ring */ > >>> struct dma_fence *fence; > >>> + int fence_id; /* key into queue->fence_idr */ > >>> struct msm_gpu_submitqueue *queue; > >>> struct pid *pid; /* submitting process */ > >>> bool fault_dumped; /* Limit devcoredump dumping to one per submit */ > >>> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c > >>> index 4f02fa3c78f9..f6f595aae2c5 100644 > >>> --- a/drivers/gpu/drm/msm/msm_gem_submit.c > >>> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c > >>> @@ -68,7 +68,14 @@ void __msm_gem_submit_destroy(struct kref *kref) > >>> container_of(kref, struct msm_gem_submit, ref); > >>> unsigned i; > >>> > >>> + if (submit->fence_id) { > >>> + mutex_lock(&submit->queue->lock); > >>> + idr_remove(&submit->queue->fence_idr, submit->fence_id); > >>> + mutex_unlock(&submit->queue->lock); > >>> + } > >>> + > >>> dma_fence_put(submit->fence); > >>> + > >>> put_pid(submit->pid); > >>> msm_submitqueue_put(submit->queue); > >>> > >>> @@ -872,6 +879,20 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, > >>> goto out; > >>> } > >>> > >>> + /* > >>> + * Allocate an id which can be used by WAIT_FENCE ioctl to map back > >>> + * to the underlying fence. > >>> + */ > >>> + mutex_lock(&queue->lock); > >>> + submit->fence_id = idr_alloc_cyclic(&queue->fence_idr, > >>> + submit->fence, 0, INT_MAX, GFP_KERNEL); > >>> + mutex_unlock(&queue->lock); > >>> + if (submit->fence_id < 0) { > >>> + ret = submit->fence_id = 0; > >>> + submit->fence_id = 0; > >>> + goto out; > >>> + } > >>> + > >>> if (args->flags & MSM_SUBMIT_FENCE_FD_OUT) { > >>> struct sync_file *sync_file = sync_file_create(submit->fence); > >>> if (!sync_file) { > >>> @@ -886,7 +907,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, > >>> > >>> msm_gpu_submit(gpu, submit); > >>> > >>> - args->fence = submit->fence->seqno; > >>> + args->fence = submit->fence_id; > >>> > >>> msm_reset_syncobjs(syncobjs_to_reset, args->nr_in_syncobjs); > >>> msm_process_post_deps(post_deps, args->nr_out_syncobjs, > >>> diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h > >>> index 96efcb31e502..579627252540 100644 > >>> --- a/drivers/gpu/drm/msm/msm_gpu.h > >>> +++ b/drivers/gpu/drm/msm/msm_gpu.h > >>> @@ -263,6 +263,9 @@ struct msm_gpu_perfcntr { > >>> * which set of pgtables do submits jobs associated with the > >>> * submitqueue use) > >>> * @node: node in the context's list of submitqueues > >>> + * @fence_idr: maps fence-id to dma_fence for userspace visible fence > >>> + * seqno, protected by submitqueue lock > >>> + * @lock: submitqueue lock > >>> * @ref: reference count > >>> */ > >>> struct msm_gpu_submitqueue { > >>> @@ -272,6 +275,8 @@ struct msm_gpu_submitqueue { > >>> int faults; > >>> struct msm_file_private *ctx; > >>> struct list_head node; > >>> + struct idr fence_idr; > >>> + struct mutex lock; > >>> struct kref ref; > >>> }; > >>> > >>> diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c > >>> index 9e9fec61d629..66f8d0fb38b0 100644 > >>> --- a/drivers/gpu/drm/msm/msm_submitqueue.c > >>> +++ b/drivers/gpu/drm/msm/msm_submitqueue.c > >>> @@ -12,6 +12,8 @@ void msm_submitqueue_destroy(struct kref *kref) > >>> struct msm_gpu_submitqueue *queue = container_of(kref, > >>> struct msm_gpu_submitqueue, ref); > >>> > >>> + idr_destroy(&queue->fence_idr); > >>> + > >>> msm_file_private_put(queue->ctx); > >>> > >>> kfree(queue); > >>> @@ -89,6 +91,9 @@ int msm_submitqueue_create(struct drm_device *drm, struct msm_file_private *ctx, > >>> if (id) > >>> *id = queue->id; > >>> > >>> + idr_init(&queue->fence_idr); > >>> + mutex_init(&queue->lock); > >>> + > >>> list_add_tail(&queue->node, &ctx->submitqueues); > >>> > >>> write_unlock(&ctx->queuelock); > >>> > >> >

3 years, 8 months

Re: [Linaro-mm-sig] [PATCH v4 07/13] drm/msm: Track "seqno" fences by idr

by Rob Clark

On Wed, Nov 10, 2021 at 7:28 AM Akhil P Oommen <akhilpo(a)codeaurora.org> wrote: > > On 7/28/2021 6:36 AM, Rob Clark wrote: > > From: Rob Clark <robdclark(a)chromium.org> > > > > Previously the (non-fd) fence returned from submit ioctl was a raw > > seqno, which is scoped to the ring. But from UABI standpoint, the > > ioctls related to seqno fences all specify a submitqueue. We can > > take advantage of that to replace the seqno fences with a cyclic idr > > handle. > > > > This is in preperation for moving to drm scheduler, at which point > > the submit ioctl will return after queuing the submit job to the > > scheduler, but before the submit is written into the ring (and > > therefore before a ring seqno has been assigned). Which means we > > need to replace the dma_fence that userspace may need to wait on > > with a scheduler fence. > > > > Signed-off-by: Rob Clark <robdclark(a)chromium.org> > > Acked-by: Christian König <christian.koenig(a)amd.com> > > --- > > drivers/gpu/drm/msm/msm_drv.c | 30 +++++++++++++++++-- > > drivers/gpu/drm/msm/msm_fence.c | 42 --------------------------- > > drivers/gpu/drm/msm/msm_fence.h | 3 -- > > drivers/gpu/drm/msm/msm_gem.h | 1 + > > drivers/gpu/drm/msm/msm_gem_submit.c | 23 ++++++++++++++- > > drivers/gpu/drm/msm/msm_gpu.h | 5 ++++ > > drivers/gpu/drm/msm/msm_submitqueue.c | 5 ++++ > > 7 files changed, 61 insertions(+), 48 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > > index 9b8fa2ad0d84..1594ae39d54f 100644 > > --- a/drivers/gpu/drm/msm/msm_drv.c > > +++ b/drivers/gpu/drm/msm/msm_drv.c > > @@ -911,6 +911,7 @@ static int msm_ioctl_wait_fence(struct drm_device *dev, void *data, > > ktime_t timeout = to_ktime(args->timeout); > > struct msm_gpu_submitqueue *queue; > > struct msm_gpu *gpu = priv->gpu; > > + struct dma_fence *fence; > > int ret; > > > > if (args->pad) { > > @@ -925,10 +926,35 @@ static int msm_ioctl_wait_fence(struct drm_device *dev, void *data, > > if (!queue) > > return -ENOENT; > > > > - ret = msm_wait_fence(gpu->rb[queue->prio]->fctx, args->fence, &timeout, > > - true); > > + /* > > + * Map submitqueue scoped "seqno" (which is actually an idr key) > > + * back to underlying dma-fence > > + * > > + * The fence is removed from the fence_idr when the submit is > > + * retired, so if the fence is not found it means there is nothing > > + * to wait for > > + */ > > + ret = mutex_lock_interruptible(&queue->lock); > > + if (ret) > > + return ret; > > + fence = idr_find(&queue->fence_idr, args->fence); > > + if (fence) > > + fence = dma_fence_get_rcu(fence); > > + mutex_unlock(&queue->lock); > > + > > + if (!fence) > > + return 0; > > > > + ret = dma_fence_wait_timeout(fence, true, timeout_to_jiffies(&timeout)); > > + if (ret == 0) { > > + ret = -ETIMEDOUT; > > + } else if (ret != -ERESTARTSYS) { > > + ret = 0; > > + } > > + > > + dma_fence_put(fence); > > msm_submitqueue_put(queue); > > + > > return ret; > > } > > > > diff --git a/drivers/gpu/drm/msm/msm_fence.c b/drivers/gpu/drm/msm/msm_fence.c > > index b92a9091a1e2..f2cece542c3f 100644 > > --- a/drivers/gpu/drm/msm/msm_fence.c > > +++ b/drivers/gpu/drm/msm/msm_fence.c > > @@ -24,7 +24,6 @@ msm_fence_context_alloc(struct drm_device *dev, volatile uint32_t *fenceptr, > > strncpy(fctx->name, name, sizeof(fctx->name)); > > fctx->context = dma_fence_context_alloc(1); > > fctx->fenceptr = fenceptr; > > - init_waitqueue_head(&fctx->event); > > spin_lock_init(&fctx->spinlock); > > > > return fctx; > > @@ -45,53 +44,12 @@ static inline bool fence_completed(struct msm_fence_context *fctx, uint32_t fenc > > (int32_t)(*fctx->fenceptr - fence) >= 0; > > } > > > > -/* legacy path for WAIT_FENCE ioctl: */ > > -int msm_wait_fence(struct msm_fence_context *fctx, uint32_t fence, > > - ktime_t *timeout, bool interruptible) > > -{ > > - int ret; > > - > > - if (fence > fctx->last_fence) { > > - DRM_ERROR_RATELIMITED("%s: waiting on invalid fence: %u (of %u)\n", > > - fctx->name, fence, fctx->last_fence); > > - return -EINVAL; > > Rob, we changed this pre-existing behaviour in this patch. Now, when > userspace tries to wait on a future fence, we don't return an error. > > I just want to check if this was accidental or not? Hmm, perhaps we should do this to restore the previous behavior: ------------- diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index 73e827641024..3dd6da56eae6 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -1000,8 +1000,12 @@ static int msm_ioctl_wait_fence(struct drm_device *dev, void *data, fence = dma_fence_get_rcu(fence); mutex_unlock(&queue->lock); - if (!fence) - return 0; + if (!fence) { + struct msm_fence_context *fctx = gpu->rb[queue->ring_nr]->fctx; + DRM_ERROR_RATELIMITED("%s: waiting on invalid fence: %u (of %u)\n", + fctx->name, fence, fctx->last_fence); + return -EINVAL; + } ret = dma_fence_wait_timeout(fence, true, timeout_to_jiffies(&timeout)); if (ret == 0) { ------------- BR, -R > -Akhil. > > > - } > > - > > - if (!timeout) { > > - /* no-wait: */ > > - ret = fence_completed(fctx, fence) ? 0 : -EBUSY; > > - } else { > > - unsigned long remaining_jiffies = timeout_to_jiffies(timeout); > > - > > - if (interruptible) > > - ret = wait_event_interruptible_timeout(fctx->event, > > - fence_completed(fctx, fence), > > - remaining_jiffies); > > - else > > - ret = wait_event_timeout(fctx->event, > > - fence_completed(fctx, fence), > > - remaining_jiffies); > > - > > - if (ret == 0) { > > - DBG("timeout waiting for fence: %u (completed: %u)", > > - fence, fctx->completed_fence); > > - ret = -ETIMEDOUT; > > - } else if (ret != -ERESTARTSYS) { > > - ret = 0; > > - } > > - } > > - > > - return ret; > > -} > > - > > /* called from workqueue */ > > void msm_update_fence(struct msm_fence_context *fctx, uint32_t fence) > > { > > spin_lock(&fctx->spinlock); > > fctx->completed_fence = max(fence, fctx->completed_fence); > > spin_unlock(&fctx->spinlock); > > - > > - wake_up_all(&fctx->event); > > } > > > > struct msm_fence { > > diff --git a/drivers/gpu/drm/msm/msm_fence.h b/drivers/gpu/drm/msm/msm_fence.h > > index 6ab97062ff1a..4783db528bcc 100644 > > --- a/drivers/gpu/drm/msm/msm_fence.h > > +++ b/drivers/gpu/drm/msm/msm_fence.h > > @@ -49,7 +49,6 @@ struct msm_fence_context { > > */ > > volatile uint32_t *fenceptr; > > > > - wait_queue_head_t event; > > spinlock_t spinlock; > > }; > > > > @@ -57,8 +56,6 @@ struct msm_fence_context * msm_fence_context_alloc(struct drm_device *dev, > > volatile uint32_t *fenceptr, const char *name); > > void msm_fence_context_free(struct msm_fence_context *fctx); > > > > -int msm_wait_fence(struct msm_fence_context *fctx, uint32_t fence, > > - ktime_t *timeout, bool interruptible); > > void msm_update_fence(struct msm_fence_context *fctx, uint32_t fence); > > > > struct dma_fence * msm_fence_alloc(struct msm_fence_context *fctx); > > diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h > > index da3af702a6c8..e0579abda5b9 100644 > > --- a/drivers/gpu/drm/msm/msm_gem.h > > +++ b/drivers/gpu/drm/msm/msm_gem.h > > @@ -320,6 +320,7 @@ struct msm_gem_submit { > > struct ww_acquire_ctx ticket; > > uint32_t seqno; /* Sequence number of the submit on the ring */ > > struct dma_fence *fence; > > + int fence_id; /* key into queue->fence_idr */ > > struct msm_gpu_submitqueue *queue; > > struct pid *pid; /* submitting process */ > > bool fault_dumped; /* Limit devcoredump dumping to one per submit */ > > diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c > > index 4f02fa3c78f9..f6f595aae2c5 100644 > > --- a/drivers/gpu/drm/msm/msm_gem_submit.c > > +++ b/drivers/gpu/drm/msm/msm_gem_submit.c > > @@ -68,7 +68,14 @@ void __msm_gem_submit_destroy(struct kref *kref) > > container_of(kref, struct msm_gem_submit, ref); > > unsigned i; > > > > + if (submit->fence_id) { > > + mutex_lock(&submit->queue->lock); > > + idr_remove(&submit->queue->fence_idr, submit->fence_id); > > + mutex_unlock(&submit->queue->lock); > > + } > > + > > dma_fence_put(submit->fence); > > + > > put_pid(submit->pid); > > msm_submitqueue_put(submit->queue); > > > > @@ -872,6 +879,20 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, > > goto out; > > } > > > > + /* > > + * Allocate an id which can be used by WAIT_FENCE ioctl to map back > > + * to the underlying fence. > > + */ > > + mutex_lock(&queue->lock); > > + submit->fence_id = idr_alloc_cyclic(&queue->fence_idr, > > + submit->fence, 0, INT_MAX, GFP_KERNEL); > > + mutex_unlock(&queue->lock); > > + if (submit->fence_id < 0) { > > + ret = submit->fence_id = 0; > > + submit->fence_id = 0; > > + goto out; > > + } > > + > > if (args->flags & MSM_SUBMIT_FENCE_FD_OUT) { > > struct sync_file *sync_file = sync_file_create(submit->fence); > > if (!sync_file) { > > @@ -886,7 +907,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, > > > > msm_gpu_submit(gpu, submit); > > > > - args->fence = submit->fence->seqno; > > + args->fence = submit->fence_id; > > > > msm_reset_syncobjs(syncobjs_to_reset, args->nr_in_syncobjs); > > msm_process_post_deps(post_deps, args->nr_out_syncobjs, > > diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h > > index 96efcb31e502..579627252540 100644 > > --- a/drivers/gpu/drm/msm/msm_gpu.h > > +++ b/drivers/gpu/drm/msm/msm_gpu.h > > @@ -263,6 +263,9 @@ struct msm_gpu_perfcntr { > > * which set of pgtables do submits jobs associated with the > > * submitqueue use) > > * @node: node in the context's list of submitqueues > > + * @fence_idr: maps fence-id to dma_fence for userspace visible fence > > + * seqno, protected by submitqueue lock > > + * @lock: submitqueue lock > > * @ref: reference count > > */ > > struct msm_gpu_submitqueue { > > @@ -272,6 +275,8 @@ struct msm_gpu_submitqueue { > > int faults; > > struct msm_file_private *ctx; > > struct list_head node; > > + struct idr fence_idr; > > + struct mutex lock; > > struct kref ref; > > }; > > > > diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c > > index 9e9fec61d629..66f8d0fb38b0 100644 > > --- a/drivers/gpu/drm/msm/msm_submitqueue.c > > +++ b/drivers/gpu/drm/msm/msm_submitqueue.c > > @@ -12,6 +12,8 @@ void msm_submitqueue_destroy(struct kref *kref) > > struct msm_gpu_submitqueue *queue = container_of(kref, > > struct msm_gpu_submitqueue, ref); > > > > + idr_destroy(&queue->fence_idr); > > + > > msm_file_private_put(queue->ctx); > > > > kfree(queue); > > @@ -89,6 +91,9 @@ int msm_submitqueue_create(struct drm_device *drm, struct msm_file_private *ctx, > > if (id) > > *id = queue->id; > > > > + idr_init(&queue->fence_idr); > > + mutex_init(&queue->lock); > > + > > list_add_tail(&queue->node, &ctx->submitqueues); > > > > write_unlock(&ctx->queuelock); > > >

3 years, 8 months

Re: [Linaro-mm-sig] AUTOSEL series truncated was -- Re: [PATCH AUTOSEL 5.15 001/146] dma-buf: WARN on dmabuf release with pending attachments

by Christian König

Hi Pavel, Am 09.11.21 um 08:54 schrieb Pavel Machek: > Hi! > > This series is truncated .. I only got first patches. Similary, 5.10 > series is truncated, [PATCH AUTOSEL 5.10 035/101] media: s5p-mfc: Add > checking to s5p_mfc_probe... is last one I got. > > I got all the patches before that, so I believe it is not problem on > my side, but I'd not mind someone confirming they are seeing the same > problem... It could of course be a different issue, but I've been experiencing similar problems since a couple of weeks now, especially with mailing lists hosted on the freedesktop.org servers and long series of mails. The symptons are that individual mails are missing from a series. I'm usually registered with two completely separated mail accounts (private and work) on those lists and if a mail is missing it is always missing on both accounts. The interesting thing is that if it is a patch set then patchwork (https://patchwork.freedesktop.org/) always seems to get all mails. No idea what's going on here and so far it was to rarely to complain, but with this series it is totally obvious that something is wrong. Regards, Christian. > > Best regards, > Pavel >

3 years, 8 months

[PATCH AUTOSEL 5.4 01/74] dma-buf: WARN on dmabuf release with pending attachments

by Sasha Levin

From: Charan Teja Reddy <charante(a)codeaurora.org> [ Upstream commit f492283b157053e9555787262f058ae33096f568 ] It is expected from the clients to follow the below steps on an imported dmabuf fd: a) dmabuf = dma_buf_get(fd) // Get the dmabuf from fd b) dma_buf_attach(dmabuf); // Clients attach to the dmabuf o Here the kernel does some slab allocations, say for dma_buf_attachment and may be some other slab allocation in the dmabuf->ops->attach(). c) Client may need to do dma_buf_map_attachment(). d) Accordingly dma_buf_unmap_attachment() should be called. e) dma_buf_detach () // Clients detach to the dmabuf. o Here the slab allocations made in b) are freed. f) dma_buf_put(dmabuf) // Can free the dmabuf if it is the last reference. Now say an erroneous client failed at step c) above thus it directly called dma_buf_put(), step f) above. Considering that it may be the last reference to the dmabuf, buffer will be freed with pending attachments left to the dmabuf which can show up as the 'memory leak'. This should at least be reported as the WARN(). Signed-off-by: Charan Teja Reddy <charante(a)codeaurora.org> Reviewed-by: Christian König <christian.koenig(a)amd.com> Link: https://patchwork.freedesktop.org/patch/msgid/1627043468-16381-1-git-send-e… Signed-off-by: Christian König <christian.koenig(a)amd.com> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- drivers/dma-buf/dma-buf.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 758de0e9b2ddc..16bbc9bc9e6d1 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -79,6 +79,7 @@ static void dma_buf_release(struct dentry *dentry) if (dmabuf->resv == (struct dma_resv *)&dmabuf[1]) dma_resv_fini(dmabuf->resv); + WARN_ON(!list_empty(&dmabuf->attachments)); module_put(dmabuf->owner); kfree(dmabuf->name); kfree(dmabuf); -- 2.33.0

3 years, 8 months

[PATCH AUTOSEL 5.10 001/101] dma-buf: WARN on dmabuf release with pending attachments

by Sasha Levin

From: Charan Teja Reddy <charante(a)codeaurora.org> [ Upstream commit f492283b157053e9555787262f058ae33096f568 ] It is expected from the clients to follow the below steps on an imported dmabuf fd: a) dmabuf = dma_buf_get(fd) // Get the dmabuf from fd b) dma_buf_attach(dmabuf); // Clients attach to the dmabuf o Here the kernel does some slab allocations, say for dma_buf_attachment and may be some other slab allocation in the dmabuf->ops->attach(). c) Client may need to do dma_buf_map_attachment(). d) Accordingly dma_buf_unmap_attachment() should be called. e) dma_buf_detach () // Clients detach to the dmabuf. o Here the slab allocations made in b) are freed. f) dma_buf_put(dmabuf) // Can free the dmabuf if it is the last reference. Now say an erroneous client failed at step c) above thus it directly called dma_buf_put(), step f) above. Considering that it may be the last reference to the dmabuf, buffer will be freed with pending attachments left to the dmabuf which can show up as the 'memory leak'. This should at least be reported as the WARN(). Signed-off-by: Charan Teja Reddy <charante(a)codeaurora.org> Reviewed-by: Christian König <christian.koenig(a)amd.com> Link: https://patchwork.freedesktop.org/patch/msgid/1627043468-16381-1-git-send-e… Signed-off-by: Christian König <christian.koenig(a)amd.com> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- drivers/dma-buf/dma-buf.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 922416b3aaceb..93e9bf7382595 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -79,6 +79,7 @@ static void dma_buf_release(struct dentry *dentry) if (dmabuf->resv == (struct dma_resv *)&dmabuf[1]) dma_resv_fini(dmabuf->resv); + WARN_ON(!list_empty(&dmabuf->attachments)); module_put(dmabuf->owner); kfree(dmabuf->name); kfree(dmabuf); -- 2.33.0

3 years, 8 months

[PATCH AUTOSEL 5.14 001/138] dma-buf: WARN on dmabuf release with pending attachments

by Sasha Levin

From: Charan Teja Reddy <charante(a)codeaurora.org> [ Upstream commit f492283b157053e9555787262f058ae33096f568 ] It is expected from the clients to follow the below steps on an imported dmabuf fd: a) dmabuf = dma_buf_get(fd) // Get the dmabuf from fd b) dma_buf_attach(dmabuf); // Clients attach to the dmabuf o Here the kernel does some slab allocations, say for dma_buf_attachment and may be some other slab allocation in the dmabuf->ops->attach(). c) Client may need to do dma_buf_map_attachment(). d) Accordingly dma_buf_unmap_attachment() should be called. e) dma_buf_detach () // Clients detach to the dmabuf. o Here the slab allocations made in b) are freed. f) dma_buf_put(dmabuf) // Can free the dmabuf if it is the last reference. Now say an erroneous client failed at step c) above thus it directly called dma_buf_put(), step f) above. Considering that it may be the last reference to the dmabuf, buffer will be freed with pending attachments left to the dmabuf which can show up as the 'memory leak'. This should at least be reported as the WARN(). Signed-off-by: Charan Teja Reddy <charante(a)codeaurora.org> Reviewed-by: Christian König <christian.koenig(a)amd.com> Link: https://patchwork.freedesktop.org/patch/msgid/1627043468-16381-1-git-send-e… Signed-off-by: Christian König <christian.koenig(a)amd.com> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- drivers/dma-buf/dma-buf.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 511fe0d217a08..733c8b1c8467c 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -79,6 +79,7 @@ static void dma_buf_release(struct dentry *dentry) if (dmabuf->resv == (struct dma_resv *)&dmabuf[1]) dma_resv_fini(dmabuf->resv); + WARN_ON(!list_empty(&dmabuf->attachments)); module_put(dmabuf->owner); kfree(dmabuf->name); kfree(dmabuf); -- 2.33.0

3 years, 8 months

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig