| Message ID | 20170728212951.7818-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Chris, 2017-07-28 Chris Wilson <chris@chris-wilson.co.uk>: > Up until recently sync_file were create to export a single dma-fence to > userspace, and so we could canabalise a bit insie dma-fence to mark > whether or not we had enable polling for the sync_file itself. However, > with the advent of syncobj, we do allow userspace to create multiple > sync_files for a single dma-fence. (Similarly, that the sw-sync > validation framework also started returning multiple sync-files wrapping > a single dma-fence for a syncpt also triggering the problem.) > > This patch reverts my suggestion in commit e24165537312 > ("dma-buf/sync_file: only enable fence signalling on poll()") to use a > single bit in the shared dma-fence and restores the sync_file->flags for > tracking the bits individually. > > Reported-by: Gustavo Padovan <gustavo@padovan.org> > Fixes: f1e8c67123cf ("dma-buf/sw-sync: Use an rbtree to sort fences in the timeline") > Fixes: e9083420bbac ("drm: introduce sync objects (v4)") > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Sumit Semwal <sumit.semwal@linaro.org> > Cc: Sean Paul <seanpaul@chromium.org> > Cc: Gustavo Padovan <gustavo@padovan.org> > Cc: dri-devel@lists.freedesktop.org > Cc: <drm-intel-fixes@lists.freedesktop.org> # v4.13-rc1+ > --- > drivers/dma-buf/sync_file.c | 5 +++-- > include/linux/sync_file.h | 3 ++- > 2 files changed, 5 insertions(+), 3 deletions(-) I confirm the patch fixes the sync kselftests for me. Pushed to drm-misc-next. Gustavo
On Sat, Jul 29, 2017 at 12:18:32PM -0300, Gustavo Padovan wrote: > Hi Chris, > > 2017-07-28 Chris Wilson <chris@chris-wilson.co.uk>: > > > Up until recently sync_file were create to export a single dma-fence to > > userspace, and so we could canabalise a bit insie dma-fence to mark > > whether or not we had enable polling for the sync_file itself. However, > > with the advent of syncobj, we do allow userspace to create multiple > > sync_files for a single dma-fence. (Similarly, that the sw-sync > > validation framework also started returning multiple sync-files wrapping > > a single dma-fence for a syncpt also triggering the problem.) > > > > This patch reverts my suggestion in commit e24165537312 > > ("dma-buf/sync_file: only enable fence signalling on poll()") to use a > > single bit in the shared dma-fence and restores the sync_file->flags for > > tracking the bits individually. > > > > Reported-by: Gustavo Padovan <gustavo@padovan.org> > > Fixes: f1e8c67123cf ("dma-buf/sw-sync: Use an rbtree to sort fences in the timeline") > > Fixes: e9083420bbac ("drm: introduce sync objects (v4)") > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Sumit Semwal <sumit.semwal@linaro.org> > > Cc: Sean Paul <seanpaul@chromium.org> > > Cc: Gustavo Padovan <gustavo@padovan.org> > > Cc: dri-devel@lists.freedesktop.org > > Cc: <drm-intel-fixes@lists.freedesktop.org> # v4.13-rc1+ > > --- > > drivers/dma-buf/sync_file.c | 5 +++-- > > include/linux/sync_file.h | 3 ++- > > 2 files changed, 5 insertions(+), 3 deletions(-) > > I confirm the patch fixes the sync kselftests for me. Pushed to > drm-misc-next. You need to cherry-pick this to drm-misc-fixes (using cherry-pick -x to make it clear we applied this twice), since the bug is in 4.13. drm-misc-next is for stuff that's only needed in 4.14. -Daniel
2017-07-31 Daniel Vetter <daniel@ffwll.ch>: > On Sat, Jul 29, 2017 at 12:18:32PM -0300, Gustavo Padovan wrote: > > Hi Chris, > > > > 2017-07-28 Chris Wilson <chris@chris-wilson.co.uk>: > > > > > Up until recently sync_file were create to export a single dma-fence to > > > userspace, and so we could canabalise a bit insie dma-fence to mark > > > whether or not we had enable polling for the sync_file itself. However, > > > with the advent of syncobj, we do allow userspace to create multiple > > > sync_files for a single dma-fence. (Similarly, that the sw-sync > > > validation framework also started returning multiple sync-files wrapping > > > a single dma-fence for a syncpt also triggering the problem.) > > > > > > This patch reverts my suggestion in commit e24165537312 > > > ("dma-buf/sync_file: only enable fence signalling on poll()") to use a > > > single bit in the shared dma-fence and restores the sync_file->flags for > > > tracking the bits individually. > > > > > > Reported-by: Gustavo Padovan <gustavo@padovan.org> > > > Fixes: f1e8c67123cf ("dma-buf/sw-sync: Use an rbtree to sort fences in the timeline") > > > Fixes: e9083420bbac ("drm: introduce sync objects (v4)") > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: Sumit Semwal <sumit.semwal@linaro.org> > > > Cc: Sean Paul <seanpaul@chromium.org> > > > Cc: Gustavo Padovan <gustavo@padovan.org> > > > Cc: dri-devel@lists.freedesktop.org > > > Cc: <drm-intel-fixes@lists.freedesktop.org> # v4.13-rc1+ > > > --- > > > drivers/dma-buf/sync_file.c | 5 +++-- > > > include/linux/sync_file.h | 3 ++- > > > 2 files changed, 5 insertions(+), 3 deletions(-) > > > > I confirm the patch fixes the sync kselftests for me. Pushed to > > drm-misc-next. > > You need to cherry-pick this to drm-misc-fixes (using cherry-pick -x to > make it clear we applied this twice), since the bug is in 4.13. > drm-misc-next is for stuff that's only needed in 4.14. > -Daniel Pushed to drm-misc-fixes now. Gustavo
diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c index d7e219d2669d..66fb40d0ebdb 100644 --- a/drivers/dma-buf/sync_file.c +++ b/drivers/dma-buf/sync_file.c @@ -304,7 +304,7 @@ static int sync_file_release(struct inode *inode, struct file *file) { struct sync_file *sync_file = file->private_data; - if (test_bit(POLL_ENABLED, &sync_file->fence->flags)) + if (test_bit(POLL_ENABLED, &sync_file->flags)) dma_fence_remove_callback(sync_file->fence, &sync_file->cb); dma_fence_put(sync_file->fence); kfree(sync_file); @@ -318,7 +318,8 @@ static unsigned int sync_file_poll(struct file *file, poll_table *wait) poll_wait(file, &sync_file->wq, wait); - if (!test_and_set_bit(POLL_ENABLED, &sync_file->fence->flags)) { + if (list_empty(&sync_file->cb.node) && + !test_and_set_bit(POLL_ENABLED, &sync_file->flags)) { if (dma_fence_add_callback(sync_file->fence, &sync_file->cb, fence_check_cb_func) < 0) wake_up_all(&sync_file->wq); diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h index 5726107963b2..0ad87c434ae6 100644 --- a/include/linux/sync_file.h +++ b/include/linux/sync_file.h @@ -43,12 +43,13 @@ struct sync_file { #endif wait_queue_head_t wq; + unsigned long flags; struct dma_fence *fence; struct dma_fence_cb cb; }; -#define POLL_ENABLED DMA_FENCE_FLAG_USER_BITS +#define POLL_ENABLED 0 struct sync_file *sync_file_create(struct dma_fence *fence); struct dma_fence *sync_file_get_fence(int fd);
Up until recently sync_file were create to export a single dma-fence to userspace, and so we could canabalise a bit insie dma-fence to mark whether or not we had enable polling for the sync_file itself. However, with the advent of syncobj, we do allow userspace to create multiple sync_files for a single dma-fence. (Similarly, that the sw-sync validation framework also started returning multiple sync-files wrapping a single dma-fence for a syncpt also triggering the problem.) This patch reverts my suggestion in commit e24165537312 ("dma-buf/sync_file: only enable fence signalling on poll()") to use a single bit in the shared dma-fence and restores the sync_file->flags for tracking the bits individually. Reported-by: Gustavo Padovan <gustavo@padovan.org> Fixes: f1e8c67123cf ("dma-buf/sw-sync: Use an rbtree to sort fences in the timeline") Fixes: e9083420bbac ("drm: introduce sync objects (v4)") Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Sumit Semwal <sumit.semwal@linaro.org> Cc: Sean Paul <seanpaul@chromium.org> Cc: Gustavo Padovan <gustavo@padovan.org> Cc: dri-devel@lists.freedesktop.org Cc: <drm-intel-fixes@lists.freedesktop.org> # v4.13-rc1+ --- drivers/dma-buf/sync_file.c | 5 +++-- include/linux/sync_file.h | 3 ++- 2 files changed, 5 insertions(+), 3 deletions(-)