- Linaro-mm-sig - lists.linaro.org

Re: [Linaro-mm-sig] [PATCH 01/15] iio: buffer-dma: Get rid of incoming/outgoing queues

by Lars-Peter Clausen

On 11/15/21 3:19 PM, Paul Cercueil wrote: > The buffer-dma code was using two queues, incoming and outgoing, to > manage the state of the blocks in use. > > While this totally works, it adds some complexity to the code, > especially since the code only manages 2 blocks. It is much easier to > just check each block's state manually, and keep a counter for the next > block to dequeue. > > Since the new DMABUF based API wouldn't use these incoming and outgoing > queues anyway, getting rid of them now makes the upcoming changes > simpler. > > Signed-off-by: Paul Cercueil <paul(a)crapouillou.net> The outgoing queue is going to be replaced by fences, but I think we need to keep the incoming queue. > [...] > @@ -442,28 +435,33 @@ EXPORT_SYMBOL_GPL(iio_dma_buffer_disable); > static void iio_dma_buffer_enqueue(struct iio_dma_buffer_queue *queue, > struct iio_dma_buffer_block *block) > { > - if (block->state == IIO_BLOCK_STATE_DEAD) { > + if (block->state == IIO_BLOCK_STATE_DEAD) > iio_buffer_block_put(block); > - } else if (queue->active) { > + else if (queue->active) > iio_dma_buffer_submit_block(queue, block); > - } else { > + else > block->state = IIO_BLOCK_STATE_QUEUED; > - list_add_tail(&block->head, &queue->incoming); If iio_dma_buffer_enqueue() is called with a dmabuf and the buffer is not active, it will be marked as queued, but we don't actually keep a reference to it anywhere. It will never be submitted to the DMA, and it will never be signaled as completed.

3 years, 6 months

1
0
0 0

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

by Christian König

Am 16.11.21 um 17:31 schrieb Laurent Pinchart: > On Tue, Nov 16, 2021 at 05:02:25PM +0100, Daniel Vetter wrote: >> On Mon, Nov 15, 2021 at 02:57:37PM +0000, Paul Cercueil wrote: >>> Le lun., nov. 15 2021 at 15:37:16 +0100, Daniel Vetter a écrit : >>>> 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 > And cameras (maybe they're included in "whatever" ?). > >>>> goal here, and it's just for a convenient way to get at buffer handles, >>>> this doesn't sound like a good idea. >>> Good question. The first reason is that a somewhat similar API was intented >>> before[1], but refused upstream as it was kind of re-inventing the wheel. >>> >>> The second reason, is that we want to be able to share buffers too, not with >>> gpu/video but with the network (zctap) and in the future with USB >>> (functionFS) too. >>> >>> [1]: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kern… >> Hm is that code merged already in upstream already? >> >> I know that dma-buf looks really generic, but like I said if there's no >> need ever to interface with any of the gpu buffer sharing it might be >> better to use something else (like get_user_pages maybe, would that work?). > Not GUP please. That brings another set of issues, especially when > dealing with DMA, we've suffered enough from the USERPTR support in V4L2 > to avoid repeating this. Let's make dma-buf better instead. Yeah, when comparing GUP and DMA-buf the later is clearly the lesser evil. DMA-buf indeed has some design issues we need to work on, especially around the async operations and synchronization. But I still think those are solvable. GUP on the other hand has some hard fundamental problems which you can only solved completely if the hardware is capable of fast and reliable recoverable page faults. Regards, Christian.

3 years, 6 months

1
0
0 0

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:57:37PM +0000, Paul Cercueil wrote: > Hi Daniel, > > Le lun., nov. 15 2021 at 15:37:16 +0100, Daniel Vetter <daniel(a)ffwll.ch> a > écrit : > > 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. > > Good question. The first reason is that a somewhat similar API was intented > before[1], but refused upstream as it was kind of re-inventing the wheel. > > The second reason, is that we want to be able to share buffers too, not with > gpu/video but with the network (zctap) and in the future with USB > (functionFS) too. > > [1]: https://lore.kernel.org/linux-iio/20210217073638.21681-1-alexandru.ardelean… Hm is that code merged already in upstream already? I know that dma-buf looks really generic, but like I said if there's no need ever to interface with any of the gpu buffer sharing it might be better to use something else (like get_user_pages maybe, would that work?). > > 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. > > There is some dma_fence action in patch #10, which is enough for the > userspace apps to use the API. > > What "horribleness" are we talking about here? It doesn't look that scary to > me, but I certainly don't have the complete picture. You need to annotate all the code involved in signalling that dma_fence using dma_fence_begin/end_signalling, and then enable full lockdep and everything. You can safely assume you'll find bugs, because we even have bugs about this in gpu drivers (where that annotation isn't fully rolled out yet). The tldr is that you can allocate memory in there. And a pile of other restrictions, but not being able to allocate memory (well GFP_ATOMIC is ok, but that can fail) is a very serious restriction. -Daniel > > Cheers, > -Paul > > > 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 > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 6 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH 2/2] drm/msm: Restore error return on invalid fence

by Rob Clark

On Mon, Nov 15, 2021 at 6:43 AM Akhil P Oommen <akhilpo(a)codeaurora.org> wrote: > > On 11/12/2021 12:54 AM, Rob Clark wrote: > > From: Rob Clark <robdclark(a)chromium.org> > > > > When converting to use an idr to map userspace fence seqno values back > > to a dma_fence, we lost the error return when userspace passes seqno > > that is larger than the last submitted fence. Restore this check. > > > > Reported-by: Akhil P Oommen <akhilpo(a)codeaurora.org> > > Fixes: a61acbbe9cf8 ("drm/msm: Track "seqno" fences by idr") > > Signed-off-by: Rob Clark <robdclark(a)chromium.org> > > --- > > Note: I will rebase "drm/msm: Handle fence rollover" on top of this, > > to simplify backporting this patch to stable kernels > > > > drivers/gpu/drm/msm/msm_drv.c | 6 ++++++ > > drivers/gpu/drm/msm/msm_gem_submit.c | 1 + > > drivers/gpu/drm/msm/msm_gpu.h | 3 +++ > > 3 files changed, 10 insertions(+) > > > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > > index cb14d997c174..56500eb5219e 100644 > > --- a/drivers/gpu/drm/msm/msm_drv.c > > +++ b/drivers/gpu/drm/msm/msm_drv.c > > @@ -967,6 +967,12 @@ static int wait_fence(struct msm_gpu_submitqueue *queue, uint32_t fence_id, > > struct dma_fence *fence; > > int ret; > > > > + if (fence_id > queue->last_fence) { > > But fence_id can wrap around and then this check won't be valid. that is correct, but see my note about rebasing "drm/msm: Handle fence rollover" on top of this patch, so this patch could be more easily cherry-picked to stable/lts branches BR, -R > -Akhil. > > > + DRM_ERROR_RATELIMITED("waiting on invalid fence: %u (of %u)\n", > > + fence_id, queue->last_fence); > > + return -EINVAL; > > + } > > + > > /* > > * Map submitqueue scoped "seqno" (which is actually an idr key) > > * back to underlying dma-fence > > diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c > > index 151d19e4453c..a38f23be497d 100644 > > --- a/drivers/gpu/drm/msm/msm_gem_submit.c > > +++ b/drivers/gpu/drm/msm/msm_gem_submit.c > > @@ -911,6 +911,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, > > drm_sched_entity_push_job(&submit->base, queue->entity); > > > > args->fence = submit->fence_id; > > + queue->last_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 bd4e0024033e..e73a5bb03544 100644 > > --- a/drivers/gpu/drm/msm/msm_gpu.h > > +++ b/drivers/gpu/drm/msm/msm_gpu.h > > @@ -376,6 +376,8 @@ static inline int msm_gpu_convert_priority(struct msm_gpu *gpu, int prio, > > * @ring_nr: the ringbuffer used by this submitqueue, which is determined > > * by the submitqueue's priority > > * @faults: the number of GPU hangs associated with this submitqueue > > + * @last_fence: the sequence number of the last allocated fence (for error > > + * checking) > > * @ctx: the per-drm_file context associated with the submitqueue (ie. > > * which set of pgtables do submits jobs associated with the > > * submitqueue use) > > @@ -391,6 +393,7 @@ struct msm_gpu_submitqueue { > > u32 flags; > > u32 ring_nr; > > int faults; > > + uint32_t last_fence; > > struct msm_file_private *ctx; > > struct list_head node; > > struct idr fence_idr; > > >

3 years, 6 months

1
0
0 0

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, 6 months

1
0
0 0

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, 6 months

1
0
0 0

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, 6 months

2
1
0 0

[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, 6 months

1
1
0 0

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, 6 months

1
0
0 0

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, 6 months

1
0
0 0

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, 6 months

1
0
0 0

[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, 6 months

1
0
0 0

[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, 6 months

1
0
0 0

[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, 6 months

1
1
0 0

[PATCH AUTOSEL 5.15 001/146] 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 63d32261b63ff..474de2d988ca7 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -82,6 +82,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, 6 months

1
1
0 0

[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, 7 months

4
7
0 0

Re: [Linaro-mm-sig] [PATCH v2] dma-buf: acquire name lock before read/write dma_buf.name

by Christian König

Am 29.10.21 um 04:15 schrieb guangming.cao(a)mediatek.com: > From: Guangming Cao <Guangming.Cao(a)mediatek.com> > > On Fri, 2021-10-08 at 12:24 +0200, Christian König wrote: >> Am 08.10.21 um 09:54 schrieb guangming.cao(a)mediatek.com: >>> From: Guangming Cao <Guangming.Cao(a)mediatek.com> >>> >>> Because dma-buf.name can be freed in func: "dma_buf_set_name", >>> so, we need to acquire lock first before we read/write dma_buf.name >>> to prevent Use After Free(UAF) issue. >>> >>> Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com> >> Reviewed-by: Christian König <christian.koenig(a)amd.com> >> >> Going to push that upstream if nobody else objects. >> >> Thanks, >> Christian. > Just a gentle ping for this patch, please kindly let me know how is it going. Ah, yes. Thanks for the reminder. I've just pushed this to drm-misc-fixes. Christian. > >>> --- >>> drivers/dma-buf/dma-buf.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c >>> index 511fe0d217a0..a7f6fd13a635 100644 >>> --- a/drivers/dma-buf/dma-buf.c >>> +++ b/drivers/dma-buf/dma-buf.c >>> @@ -1372,6 +1372,8 @@ static int dma_buf_debug_show(struct seq_file >>> *s, void *unused) >>> if (ret) >>> goto error_unlock; >>> >>> + >>> + spin_lock(&buf_obj->name_lock); >>> seq_printf(s, >>> "%08zu\t%08x\t%08x\t%08ld\t%s\t%08lu\t%s\n", >>> buf_obj->size, >>> buf_obj->file->f_flags, buf_obj->file- >>>> f_mode, >>> @@ -1379,6 +1381,7 @@ static int dma_buf_debug_show(struct seq_file >>> *s, void *unused) >>> buf_obj->exp_name, >>> file_inode(buf_obj->file)->i_ino, >>> buf_obj->name ?: ""); >>> + spin_unlock(&buf_obj->name_lock); >>> >>> robj = buf_obj->resv; >>> fence = dma_resv_excl_fence(robj); >> >> _______________________________________________ >> Linux-mediatek mailing list >> Linux-mediatek(a)lists.infradead.org >> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.infr…

3 years, 7 months

1
0
0 0

Deploying new iterator interface for dma-buf

by Christian König

Hi guys, a few more bug fixes, looks like the more selftests I add the more odies I find. Assuming the CI tests now pass I will start pushing patches I've already got an rb for to drm-misc-next. Please review and/or comment, Christian.

3 years, 7 months

5
61
0 0

Re: [Linaro-mm-sig] [PATCH v3] dma-buf: remove restriction of IOCTL:DMA_BUF_SET_NAME

by Christian König

Am 26.10.21 um 13:52 schrieb guangming.cao(a)mediatek.com: > From: Guangming Cao <Guangming.Cao(a)mediatek.com> > > On Tue, 2021-10-26 at 13:18 +0200, Christian König wrote: >> Am 14.10.21 um 12:25 schrieb guangming.cao(a)mediatek.com: >>> From: Guangming Cao <Guangming.Cao(a)mediatek.com> >>> >>> In this patch(https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpat…;reserved=0), >>> it add a new IOCTL to support dma-buf user to set debug name. >>> >>> But it also added a limitation of this IOCTL, it needs the >>> attachments of dmabuf should be empty, otherwise it will fail. >>> >>> For the original series, the idea was that allowing name change >>> mid-use could confuse the users about the dma-buf. >>> However, the rest of the series also makes sure each dma-buf have a >>> unique >>> inode(https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpat…;reserved=0), and any >>> accounting >>> should probably use that, without relying on the name as much. >>> >>> So, removing this restriction will let dma-buf userspace users to >>> use it >>> more comfortably and without any side effect. >>> >>> Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com> >> We could now cleanup the return value from dma_buf_set_name() into a >> void since that function can't fail any more as far as I can see. >> >> But that isn't mandatory I think, patch is Reviewed-by: Christian >> König >> <christian.koenig(a)amd.com> >> > So, here is no need to check return value of 'strndup_user', > just return without error code if the almost impossible error occurs? Good point, totally missed that one. In that case I'm going to push the patch to drm-misc-next as is. Regards, Christian. > > Guangming. > >> Regards, >> Christian. >> >>> --- >>> drivers/dma-buf/dma-buf.c | 17 +++-------------- >>> 1 file changed, 3 insertions(+), 14 deletions(-) >>> >>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c >>> index 511fe0d217a0..5fbb3a2068a3 100644 >>> --- a/drivers/dma-buf/dma-buf.c >>> +++ b/drivers/dma-buf/dma-buf.c >>> @@ -325,10 +325,8 @@ static __poll_t dma_buf_poll(struct file >>> *file, poll_table *poll) >>> >>> /** >>> * dma_buf_set_name - Set a name to a specific dma_buf to track >>> the usage. >>> - * The name of the dma-buf buffer can only be set when the dma-buf >>> is not >>> - * attached to any devices. It could theoritically support >>> changing the >>> - * name of the dma-buf if the same piece of memory is used for >>> multiple >>> - * purpose between different devices. >>> + * It could support changing the name of the dma-buf if the same >>> + * piece of memory is used for multiple purpose between different >>> devices. >>> * >>> * @dmabuf: [in] dmabuf buffer that will be renamed. >>> * @buf: [in] A piece of userspace memory that contains >>> the name of >>> @@ -341,25 +339,16 @@ static __poll_t dma_buf_poll(struct file >>> *file, poll_table *poll) >>> static long dma_buf_set_name(struct dma_buf *dmabuf, const char >>> __user *buf) >>> { >>> char *name = strndup_user(buf, DMA_BUF_NAME_LEN); >>> - long ret = 0; >>> >>> if (IS_ERR(name)) >>> return PTR_ERR(name); >>> >>> - dma_resv_lock(dmabuf->resv, NULL); >>> - if (!list_empty(&dmabuf->attachments)) { >>> - ret = -EBUSY; >>> - kfree(name); >>> - goto out_unlock; >>> - } >>> spin_lock(&dmabuf->name_lock); >>> kfree(dmabuf->name); >>> dmabuf->name = name; >>> spin_unlock(&dmabuf->name_lock); >>> >>> -out_unlock: >>> - dma_resv_unlock(dmabuf->resv); >>> - return ret; >>> + return 0; >>> } >>> >>> static long dma_buf_ioctl(struct file *file, >>

3 years, 7 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v3] dma-buf: remove restriction of IOCTL:DMA_BUF_SET_NAME

by Christian König

Am 14.10.21 um 12:25 schrieb guangming.cao(a)mediatek.com: > From: Guangming Cao <Guangming.Cao(a)mediatek.com> > > In this patch(https://patchwork.freedesktop.org/patch/310349), > it add a new IOCTL to support dma-buf user to set debug name. > > But it also added a limitation of this IOCTL, it needs the > attachments of dmabuf should be empty, otherwise it will fail. > > For the original series, the idea was that allowing name change > mid-use could confuse the users about the dma-buf. > However, the rest of the series also makes sure each dma-buf have a unique > inode(https://patchwork.freedesktop.org/patch/310387/), and any accounting > should probably use that, without relying on the name as much. > > So, removing this restriction will let dma-buf userspace users to use it > more comfortably and without any side effect. > > Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com> We could now cleanup the return value from dma_buf_set_name() into a void since that function can't fail any more as far as I can see. But that isn't mandatory I think, patch is Reviewed-by: Christian König <christian.koenig(a)amd.com> Regards, Christian. > --- > drivers/dma-buf/dma-buf.c | 17 +++-------------- > 1 file changed, 3 insertions(+), 14 deletions(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 511fe0d217a0..5fbb3a2068a3 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -325,10 +325,8 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll) > > /** > * dma_buf_set_name - Set a name to a specific dma_buf to track the usage. > - * The name of the dma-buf buffer can only be set when the dma-buf is not > - * attached to any devices. It could theoritically support changing the > - * name of the dma-buf if the same piece of memory is used for multiple > - * purpose between different devices. > + * It could support changing the name of the dma-buf if the same > + * piece of memory is used for multiple purpose between different devices. > * > * @dmabuf: [in] dmabuf buffer that will be renamed. > * @buf: [in] A piece of userspace memory that contains the name of > @@ -341,25 +339,16 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll) > static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf) > { > char *name = strndup_user(buf, DMA_BUF_NAME_LEN); > - long ret = 0; > > if (IS_ERR(name)) > return PTR_ERR(name); > > - dma_resv_lock(dmabuf->resv, NULL); > - if (!list_empty(&dmabuf->attachments)) { > - ret = -EBUSY; > - kfree(name); > - goto out_unlock; > - } > spin_lock(&dmabuf->name_lock); > kfree(dmabuf->name); > dmabuf->name = name; > spin_unlock(&dmabuf->name_lock); > > -out_unlock: > - dma_resv_unlock(dmabuf->resv); > - return ret; > + return 0; > } > > static long dma_buf_ioctl(struct file *file,

3 years, 7 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] dma-buf: st: fix error handling in test_get_fences()

by Christian König

Am 26.10.21 um 10:34 schrieb Arnd Bergmann: > From: Arnd Bergmann <arnd(a)arndb.de> > > The new driver incorrectly unwinds after errors, as clang points out: > > drivers/dma-buf/st-dma-resv.c:295:7: error: variable 'i' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized] > if (r) { > ^ > drivers/dma-buf/st-dma-resv.c:336:9: note: uninitialized use occurs here > while (i--) > ^ > drivers/dma-buf/st-dma-resv.c:295:3: note: remove the 'if' if its condition is always false > if (r) { > ^~~~~~~~ > drivers/dma-buf/st-dma-resv.c:288:6: error: variable 'i' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized] > if (r) { > ^ > drivers/dma-buf/st-dma-resv.c:336:9: note: uninitialized use occurs here > while (i--) > ^ > drivers/dma-buf/st-dma-resv.c:288:2: note: remove the 'if' if its condition is always false > if (r) { > ^~~~~~~~ > drivers/dma-buf/st-dma-resv.c:280:10: note: initialize the variable 'i' to silence this warning > int r, i; > ^ > = 0 > > Skip cleaning up the bits that have not been allocated at this point. > > Fixes: 1d51775cd3f5 ("dma-buf: add dma_resv selftest v4") > Signed-off-by: Arnd Bergmann <arnd(a)arndb.de> I already send out a patch to fix this up, but forgot to fix both gotos. Going to add my rb and using that one here instead. Thanks, Christian. > --- > I'm not familiar with these interfaces, so I'm just guessing where > we should jump after an error, please double-check and fix if necessary. > --- > drivers/dma-buf/st-dma-resv.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma-buf/st-dma-resv.c b/drivers/dma-buf/st-dma-resv.c > index 6f3ba756da3e..bc32b3eedcb6 100644 > --- a/drivers/dma-buf/st-dma-resv.c > +++ b/drivers/dma-buf/st-dma-resv.c > @@ -287,7 +287,7 @@ static int test_get_fences(void *arg, bool shared) > r = dma_resv_lock(&resv, NULL); > if (r) { > pr_err("Resv locking failed\n"); > - goto err_free; > + goto err_resv; > } > > if (shared) { > @@ -295,7 +295,7 @@ static int test_get_fences(void *arg, bool shared) > if (r) { > pr_err("Resv shared slot allocation failed\n"); > dma_resv_unlock(&resv); > - goto err_free; > + goto err_resv; > } > > dma_resv_add_shared_fence(&resv, f); > @@ -336,6 +336,7 @@ static int test_get_fences(void *arg, bool shared) > while (i--) > dma_fence_put(fences[i]); > kfree(fences); > +err_resv: > dma_resv_fini(&resv); > dma_fence_put(f); > return r;

3 years, 7 months

1
0
0 0

Re: [Linaro-mm-sig] [RFC 06/13] soc: mediatek: apu: Add apu core driver

by Randy Dunlap

Hi, On 10/23/21 4:14 AM, Flora Fu wrote: > diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig > index d9bac2710494..074b0cf24c44 100644 > --- a/drivers/soc/mediatek/Kconfig > +++ b/drivers/soc/mediatek/Kconfig > @@ -24,6 +24,24 @@ config MTK_APU_PM > APU power domain shall be enabled before accessing the > internal sub modules. > > +config MTK_APU > + tristate "MediaTek APUSYS Support" > + select REGMAP > + select MTK_APU_PM > + select MTK_SCP > + help > + Say yes here to add support for the APU tinysys. The tinsys is tinysys runs on > + running on a micro processor in APU. a microprocessor in the APU. > + Its firmware is load and boot from Kernel side. Kernel and tinysys use is loaded and booted > + IPI to tx/rx messages. to send/receive messages. > + > +config MTK_APU_DEBUG > + tristate "MediaTek APUSYS debug functions" > + depends on MTK_APU > + help > + Say yes here to enalbe debug on APUSYS. enable > + Disable it if you don't need them. -- ~Randy

3 years, 7 months

1
0
0 0

[PATCH 0/2] dma-buf: Fix breakages from dma_resv_iter conversion.

by Maarten Lankhorst

Urgent fixes! dma_resv_test_signaled is completely broken, dma_resv_wait_timeout kind of broken. Maarten Lankhorst (2): dma-buf: Fix dma_resv_wait_timeout handling of timeout = 0. dma-buf: Fix dma_resv_test_signaled. drivers/dma-buf/dma-resv.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) -- 2.33.0

3 years, 7 months

4
7
0 0

Re: [Linaro-mm-sig] [PATCH] dma-buf: add attachments empty check for dma_buf_release

by Daniel Vetter

On Tue, Oct 19, 2021 at 08:23:45PM +0800, guangming.cao(a)mediatek.com wrote: > From: Guangming Cao <Guangming.Cao(a)mediatek.com> > > Since there is no mandatory inspection for attachments in dma_buf_release. > There will be a case that dma_buf already released but attachment is still > in use, which can points to the dmabuf, and it maybe cause > some unexpected issues. > > With IOMMU, when this cases occurs, there will have IOMMU address > translation fault(s) followed by this warning, > I think it's useful for dma devices to debug issue. > > Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com> This feels a lot like hand-rolling kobject debugging. If you want to do this then I think adding kobject debug support to dma_buf/dma_buf_attachment would be better than hand-rolling something bespoke here. Also on the patch itself: You don't need the trylock. For correctly working code non one else can get at the dma-buf, so no locking needed to iterate through the attachment list. For incorrect code the kernel will be on fire pretty soon anyway, trying to do locking won't help :-) And without the trylock we can catch more bugs (e.g. if you also forgot to unlock and not just forgot to detach). -Daniel > --- > drivers/dma-buf/dma-buf.c | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 511fe0d217a0..672404857d6a 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -74,6 +74,29 @@ static void dma_buf_release(struct dentry *dentry) > */ > BUG_ON(dmabuf->cb_shared.active || dmabuf->cb_excl.active); > > + /* attachment check */ > + if (dma_resv_trylock(dmabuf->resv) && WARN(!list_empty(&dmabuf->attachments), > + "%s err, inode:%08lu size:%08zu name:%s exp_name:%s flags:0x%08x mode:0x%08x, %s\n", > + __func__, file_inode(dmabuf->file)->i_ino, dmabuf->size, > + dmabuf->name, dmabuf->exp_name, > + dmabuf->file->f_flags, dmabuf->file->f_mode, > + "Release dmabuf before detach all attachments, dump attach:\n")) { > + int attach_cnt = 0; > + dma_addr_t dma_addr; > + struct dma_buf_attachment *attach_obj; > + /* dump all attachment info */ > + list_for_each_entry(attach_obj, &dmabuf->attachments, node) { > + dma_addr = (dma_addr_t)0; > + if (attach_obj->sgt) > + dma_addr = sg_dma_address(attach_obj->sgt->sgl); > + pr_err("attach[%d]: dev:%s dma_addr:0x%-12lx\n", > + attach_cnt, dev_name(attach_obj->dev), dma_addr); > + attach_cnt++; > + } > + pr_err("Total %d devices attached\n\n", attach_cnt); > + dma_resv_unlock(dmabuf->resv); > + } > + > dmabuf->ops->release(dmabuf); > > if (dmabuf->resv == (struct dma_resv *)&dmabuf[1]) > -- > 2.17.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 7 months

2
2
0 0

Re: [Linaro-mm-sig] [PATCH 16/28] drm/scheduler: use new iterator in drm_sched_job_add_implicit_dependencies v2

by Christian König

Thanks for the notice. Going to take a deeper look into this tomorrow. Basically looks like we messed up the fence ref count somehow. Thanks, Christian. Am 17.10.21 um 16:40 schrieb Nicolas Frattaroli: > On Dienstag, 5. Oktober 2021 13:37:30 CEST Christian König wrote: >> Simplifying the code a bit. >> >> v2: use dma_resv_for_each_fence >> >> Signed-off-by: Christian König <christian.koenig(a)amd.com> >> Reviewed-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> >> --- >> drivers/gpu/drm/scheduler/sched_main.c | 26 ++++++-------------------- >> 1 file changed, 6 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/gpu/drm/scheduler/sched_main.c >> b/drivers/gpu/drm/scheduler/sched_main.c index 042c16b5d54a..5bc5f775abe1 >> 100644 >> --- a/drivers/gpu/drm/scheduler/sched_main.c >> +++ b/drivers/gpu/drm/scheduler/sched_main.c >> @@ -699,30 +699,16 @@ int drm_sched_job_add_implicit_dependencies(struct >> drm_sched_job *job, struct drm_gem_object *obj, >> bool write) >> { >> + struct dma_resv_iter cursor; >> + struct dma_fence *fence; >> int ret; >> - struct dma_fence **fences; >> - unsigned int i, fence_count; >> - >> - if (!write) { >> - struct dma_fence *fence = dma_resv_get_excl_unlocked(obj- >> resv); >> - >> - return drm_sched_job_add_dependency(job, fence); >> - } >> - >> - ret = dma_resv_get_fences(obj->resv, NULL, &fence_count, &fences); >> - if (ret || !fence_count) >> - return ret; >> >> - for (i = 0; i < fence_count; i++) { >> - ret = drm_sched_job_add_dependency(job, fences[i]); >> + dma_resv_for_each_fence(&cursor, obj->resv, write, fence) { >> + ret = drm_sched_job_add_dependency(job, fence); >> if (ret) >> - break; >> + return ret; >> } >> - >> - for (; i < fence_count; i++) >> - dma_fence_put(fences[i]); >> - kfree(fences); >> - return ret; >> + return 0; >> } >> EXPORT_SYMBOL(drm_sched_job_add_implicit_dependencies); > Hi Christian, > > unfortunately, this breaks lima on the rk3328 quite badly. Running glmark2- > es2-drm just locks up the device with the following traces: > > [ 39.624100] ------------[ cut here ]------------ > [ 39.624555] refcount_t: addition on 0; use-after-free. > [ 39.625058] WARNING: CPU: 0 PID: 123 at lib/refcount.c:25 > refcount_warn_saturate+0xa4/0x150 > [ 39.625825] Modules linked in: 8021q garp stp mrp llc crct10dif_ce > hantro_vpu(C) fuse ip_tables x_tables ipv6 > [ 39.626753] CPU: 0 PID: 123 Comm: pp Tainted: G C 5.15.0- > rc1fratti-00251-g9c2ba265352a #158 > [ 39.627614] Hardware name: Pine64 Rock64 (DT) > [ 39.628004] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > [ 39.628628] pc : refcount_warn_saturate+0xa4/0x150 > [ 39.629062] lr : refcount_warn_saturate+0xa4/0x150 > [ 39.629495] sp : ffffffc0124d3d90 > [ 39.629794] x29: ffffffc0124d3d90 x28: 0000000000000000 x27: > 0000000000000000 > [ 39.630441] x26: 0000000000000000 x25: ffffffc0117fe000 x24: > ffffff8001ad73f8 > [ 39.631087] x23: ffffffc0107fc3e0 x22: ffffffc0117fe000 x21: > ffffff8010660000 > [ 39.631731] x20: ffffff8001ad73c0 x19: ffffff807db094c8 x18: > ffffffffffffffff > [ 39.632377] x17: 0000000000000001 x16: 0000000000000001 x15: > 0765076507720766 > [ 39.633022] x14: 072d077207650774 x13: 0765076507720766 x12: > 072d077207650774 > [ 39.633668] x11: 0720072007200720 x10: ffffffc011c4b1b0 x9 : > ffffffc01010ac54 > [ 39.634314] x8 : 00000000ffffdfff x7 : ffffffc011cfb1b0 x6 : > 0000000000000001 > [ 39.634960] x5 : ffffff807fb4d980 x4 : 0000000000000000 x3 : > 0000000000000027 > [ 39.635605] x2 : 0000000000000000 x1 : 0000000000000000 x0 : > ffffff8000e1f000 > [ 39.636250] Call trace: > [ 39.636475] refcount_warn_saturate+0xa4/0x150 > [ 39.636879] drm_sched_entity_pop_job+0x414/0x4a0 > [ 39.637307] drm_sched_main+0xe4/0x450 > [ 39.637651] kthread+0x12c/0x140 > [ 39.637949] ret_from_fork+0x10/0x20 > [ 39.638279] ---[ end trace 47528e09b2512330 ]--- > [ 39.638783] ------------[ cut here ]------------ > [ 39.639214] refcount_t: underflow; use-after-free. > [ 39.639687] WARNING: CPU: 0 PID: 123 at lib/refcount.c:28 > refcount_warn_saturate+0xf8/0x150 > [ 39.640447] Modules linked in: 8021q garp stp mrp llc crct10dif_ce > hantro_vpu(C) fuse ip_tables x_tables ipv6 > [ 39.641373] CPU: 0 PID: 123 Comm: pp Tainted: G WC 5.15.0- > rc1fratti-00251-g9c2ba265352a #158 > [ 39.642237] Hardware name: Pine64 Rock64 (DT) > [ 39.642632] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > [ 39.643257] pc : refcount_warn_saturate+0xf8/0x150 > [ 39.643693] lr : refcount_warn_saturate+0xf8/0x150 > [ 39.644128] sp : ffffffc0124d3d90 > [ 39.644430] x29: ffffffc0124d3d90 x28: 0000000000000000 x27: > 0000000000000000 > [ 39.645077] x26: 0000000000000000 x25: ffffffc0117fe000 x24: > ffffff8001ad73f8 > [ 39.645724] x23: ffffffc0107fc3e0 x22: ffffffc0117fe000 x21: > ffffff8010660000 > [ 39.646372] x20: ffffff8001ad73c0 x19: ffffff807db094c8 x18: > ffffffffffffffff > [ 39.647020] x17: 0000000000000001 x16: 0000000000000001 x15: > 072007200720072e > [ 39.647666] x14: 0765076507720766 x13: 072007200720072e x12: > 0765076507720766 > [ 39.648312] x11: 0720072007200720 x10: ffffffc011c4b1b0 x9 : > ffffffc01010ac54 > [ 39.648957] x8 : 00000000ffffdfff x7 : ffffffc011cfb1b0 x6 : > 0000000000000001 > [ 39.649602] x5 : ffffff807fb4d980 x4 : 0000000000000000 x3 : > 0000000000000027 > [ 39.650247] x2 : 0000000000000000 x1 : 0000000000000000 x0 : > ffffff8000e1f000 > [ 39.650894] Call trace: > [ 39.651119] refcount_warn_saturate+0xf8/0x150 > [ 39.651526] drm_sched_entity_pop_job+0x420/0x4a0 > [ 39.651953] drm_sched_main+0xe4/0x450 > [ 39.652296] kthread+0x12c/0x140 > [ 39.652595] ret_from_fork+0x10/0x20 > [ 39.652924] ---[ end trace 47528e09b2512331 ]--- > [ 39.717053] ------------[ cut here ]------------ > [ 39.717543] refcount_t: saturated; leaking memory. > [ 39.718030] WARNING: CPU: 1 PID: 375 at lib/refcount.c:22 > refcount_warn_saturate+0x78/0x150 > [ 39.718800] Modules linked in: 8021q garp stp mrp llc crct10dif_ce > hantro_vpu(C) fuse ip_tables x_tables ipv6 > [ 39.719744] CPU: 1 PID: 375 Comm: glmark2-es2-drm Tainted: G WC > 5.15.0-rc1fratti-00251-g9c2ba265352a #158 > [ 39.720712] Hardware name: Pine64 Rock64 (DT) > [ 39.721109] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > [ 39.721739] pc : refcount_warn_saturate+0x78/0x150 > [ 39.722178] lr : refcount_warn_saturate+0x78/0x150 > [ 39.722617] sp : ffffffc012913a90 > [ 39.722921] x29: ffffffc012913a90 x28: ffffff8010630000 x27: > ffffff8005219e00 > [ 39.723576] x26: ffffff80103da500 x25: 0000000000000000 x24: > ffffff8000cb24c0 > [ 39.724230] x23: ffffff800ac045b0 x22: ffffff8005212100 x21: > 0000000000000000 > [ 39.724884] x20: ffffff8000cb24c0 x19: 0000000000000000 x18: > ffffffffffffffff > [ 39.725538] x17: 0000000000000000 x16: 0000000000000000 x15: > 072007200720072e > [ 39.726192] x14: 07790772076f076d x13: 072007200720072e x12: > 07790772076f076d > [ 39.726846] x11: 0720072007200720 x10: ffffffc011c4b1b0 x9 : > ffffffc01010ac54 > [ 39.727501] x8 : 00000000ffffdfff x7 : ffffffc011cfb1b0 x6 : > 0000000000000001 > [ 39.728155] x5 : ffffff807fb68980 x4 : 0000000000000000 x3 : > 0000000000000027 > [ 39.728808] x2 : 0000000000000000 x1 : 0000000000000000 x0 : > ffffff8004d7b800 > [ 39.729464] Call trace: > [ 39.729691] refcount_warn_saturate+0x78/0x150 > [ 39.730101] dma_resv_add_shared_fence+0x1ac/0x1cc > [ 39.730543] lima_gem_submit+0x300/0x580 > [ 39.730909] lima_ioctl_gem_submit+0x284/0x340 > [ 39.731318] drm_ioctl_kernel+0xd0/0x180 > [ 39.731685] drm_ioctl+0x220/0x450 > [ 39.732005] __arm64_sys_ioctl+0x568/0xe9c > [ 39.732386] invoke_syscall.constprop.0+0x58/0xf0 > [ 39.732824] do_el0_svc+0x138/0x170 > [ 39.733152] el0_svc+0x28/0xc0 > [ 39.733441] el0t_64_sync_handler+0xa8/0x130 > [ 39.733837] el0t_64_sync+0x1a0/0x1a4 > [ 39.734178] ---[ end trace 47528e09b2512332 ]--- > [ 39.734926] Unable to handle kernel write to read-only memory at virtual > address ffffffc0107fbc70 > [ 39.735763] Mem abort info: > [ 39.736029] ESR = 0x9600004e > [ 39.736313] EC = 0x25: DABT (current EL), IL = 32 bits > [ 39.736796] SET = 0, FnV = 0 > [ 39.737080] EA = 0, S1PTW = 0 > [ 39.737368] FSC = 0x0e: level 2 permission fault > [ 39.737804] Data abort info: > [ 39.738068] ISV = 0, ISS = 0x0000004e > [ 39.738419] CM = 0, WnR = 1 > [ 39.738693] swapper pgtable: 4k pages, 39-bit VAs, pgdp=0000000003893000 > [ 39.739297] [ffffffc0107fbc70] pgd=100000007ffff003, p4d=100000007ffff003, > pud=100000007ffff003, pmd=0040000002800781 > [ 39.740270] Internal error: Oops: 9600004e [#1] SMP > [ 39.740719] Modules linked in: 8021q garp stp mrp llc crct10dif_ce > hantro_vpu(C) fuse ip_tables x_tables ipv6 > [ 39.741665] CPU: 0 PID: 123 Comm: pp Tainted: G WC 5.15.0- > rc1fratti-00251-g9c2ba265352a #158 > [ 39.742537] Hardware name: Pine64 Rock64 (DT) > [ 39.742934] pstate: 000000c5 (nzcv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > [ 39.743570] pc : dma_fence_add_callback+0xb0/0xf0 > [ 39.744017] lr : dma_fence_add_callback+0x5c/0xf0 > [ 39.744457] sp : ffffffc0124d3d60 > [ 39.744764] x29: ffffffc0124d3d60 x28: 0000000000000000 x27: > 0000000000000000 > [ 39.745423] x26: 0000000000000000 x25: ffffffc0117fe000 x24: > ffffff800536b6e0 > [ 39.746080] x23: 0000000000000000 x22: 0000000000000000 x21: > ffffffc0107fc3e0 > [ 39.746736] x20: ffffff807db09528 x19: ffffff8000cb24c0 x18: > 0000000000000001 > [ 39.747390] x17: 000000040044ffff x16: 000000000000000c x15: > 000000000000000d > [ 39.748044] x14: 0000000000000000 x13: 000000000000072b x12: > 071c71c71c71c71c > [ 39.748697] x11: 000000000000072b x10: 0000000000000002 x9 : > ffffffc01087d5ac > [ 39.749350] x8 : 0000000000000238 x7 : 0000000000000000 x6 : > 0000000000000000 > [ 39.750002] x5 : 0000000000000000 x4 : 0000000000000000 x3 : > ffffff8000cb24f0 > [ 39.750654] x2 : 0000000000000000 x1 : ffffffc0107fbc70 x0 : > ffffff8000cb24d0 > [ 39.751309] Call trace: > [ 39.751539] dma_fence_add_callback+0xb0/0xf0 > [ 39.751944] drm_sched_entity_pop_job+0xac/0x4a0 > [ 39.752371] drm_sched_main+0xe4/0x450 > [ 39.752720] kthread+0x12c/0x140 > [ 39.753024] ret_from_fork+0x10/0x20 > [ 39.753367] Code: 91004260 f9400e61 f9000e74 a9000680 (f9000034) > [ 39.753920] ---[ end trace 47528e09b2512333 ]--- > [ 40.253374] [drm:lima_sched_timedout_job] *ERROR* lima job timeout > > I've bisected the problem to this commit, and confirmed that reverting it gets > glmark2's 3d horse back to spinning. > > It's possible this patch just uncovers a bug in lima, so I've added the lima > list as a recipient to this reply as well. > > Since I doubt AMD has many Rockchip SoCs laying about, I'll gladly test any > prospective fixes for this. > > Regards, > Nicolas Frattaroli > >

3 years, 7 months

1
0
0 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig