From: Daniel Vetter <daniel@ffwll.ch> To: David Stevens <stevensd@chromium.org> Cc: "Christian König" <christian.koenig@amd.com>, "Maarten Lankhorst" <maarten.lankhorst@linux.intel.com>, "Maxime Ripard" <mripard@kernel.org>, "Thomas Zimmermann" <tzimmermann@suse.de>, "David Airlie" <airlied@linux.ie>, "Daniel Vetter" <daniel@ffwll.ch>, "Sumit Semwal" <sumit.semwal@linaro.org>, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, linux-media@vger.kernel.org, linaro-mm-sig@lists.linaro.org Subject: Re: [PATCH] drm/syncobj: use newly allocated stub fences Date: Thu, 8 Apr 2021 13:40:26 +0200 [thread overview] Message-ID: <YG7rqkzLRBubwtk4@phenom.ffwll.local> (raw) In-Reply-To: <20210408093448.3897988-1-stevensd@google.com> On Thu, Apr 08, 2021 at 06:34:48PM +0900, David Stevens wrote: > From: David Stevens <stevensd@chromium.org> > > Allocate a new private stub fence in drm_syncobj_assign_null_handle, > instead of using a static stub fence. > > When userspace creates a fence with DRM_SYNCOBJ_CREATE_SIGNALED or when > userspace signals a fence via DRM_IOCTL_SYNCOBJ_SIGNAL, the timestamp > obtained when the fence is exported and queried with SYNC_IOC_FILE_INFO > should match when the fence's status was changed from the perspective of > userspace, which is during the respective ioctl. > > When a static stub fence started being used in by these ioctls, this > behavior changed. Instead, the timestamp returned by SYNC_IOC_FILE_INFO > became the first time anything used the static stub fence, which has no > meaning to userspace. > > Signed-off-by: David Stevens <stevensd@chromium.org> Is there also an igt patch to enforce this in the drm_syncobj testcases? Would be really good to have that too. -Daniel > --- > drivers/dma-buf/dma-fence.c | 33 ++++++++++++++++++++++++++++++++- > drivers/gpu/drm/drm_syncobj.c | 28 ++++++++++++++++++++-------- > include/linux/dma-fence.h | 1 + > 3 files changed, 53 insertions(+), 9 deletions(-) > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > index d64fc03929be..6081eb962490 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -26,6 +26,11 @@ EXPORT_TRACEPOINT_SYMBOL(dma_fence_signaled); > static DEFINE_SPINLOCK(dma_fence_stub_lock); > static struct dma_fence dma_fence_stub; > > +struct drm_fence_private_stub { > + struct dma_fence base; > + spinlock_t lock; > +}; > + > /* > * fence context counter: each execution context should have its own > * fence context, this allows checking if fences belong to the same > @@ -123,7 +128,9 @@ static const struct dma_fence_ops dma_fence_stub_ops = { > /** > * dma_fence_get_stub - return a signaled fence > * > - * Return a stub fence which is already signaled. > + * Return a stub fence which is already signaled. The fence's > + * timestamp corresponds to the first time after boot this > + * function is called. > */ > struct dma_fence *dma_fence_get_stub(void) > { > @@ -141,6 +148,30 @@ struct dma_fence *dma_fence_get_stub(void) > } > EXPORT_SYMBOL(dma_fence_get_stub); > > +/** > + * dma_fence_allocate_private_stub - return a private, signaled fence > + * > + * Return a newly allocated and signaled stub fence. > + */ > +struct dma_fence *dma_fence_allocate_private_stub(void) > +{ > + struct drm_fence_private_stub *fence; > + > + fence = kzalloc(sizeof(*fence), GFP_KERNEL); > + if (fence == NULL) > + return ERR_PTR(-ENOMEM); > + > + spin_lock_init(&fence->lock); > + dma_fence_init(&fence->base, > + &dma_fence_stub_ops, > + &fence->lock, > + 0, 0); > + dma_fence_signal(&fence->base); > + > + return &fence->base; > +} > +EXPORT_SYMBOL(dma_fence_allocate_private_stub); > + > /** > * dma_fence_context_alloc - allocate an array of fence contexts > * @num: amount of contexts to allocate > diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c > index 349146049849..c6125e57ae37 100644 > --- a/drivers/gpu/drm/drm_syncobj.c > +++ b/drivers/gpu/drm/drm_syncobj.c > @@ -350,12 +350,15 @@ EXPORT_SYMBOL(drm_syncobj_replace_fence); > * > * Assign a already signaled stub fence to the sync object. > */ > -static void drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj) > +static int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj) > { > - struct dma_fence *fence = dma_fence_get_stub(); > + struct dma_fence *fence = dma_fence_allocate_private_stub(); > + if (IS_ERR(fence)) > + return PTR_ERR(fence); > > - drm_syncobj_replace_fence(syncobj, fence); > - dma_fence_put(fence); > + drm_syncobj_replace_fence(syncobj, fence); > + dma_fence_put(fence); > + return 0; > } > > /* 5s default for wait submission */ > @@ -469,6 +472,7 @@ EXPORT_SYMBOL(drm_syncobj_free); > int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags, > struct dma_fence *fence) > { > + int ret; > struct drm_syncobj *syncobj; > > syncobj = kzalloc(sizeof(struct drm_syncobj), GFP_KERNEL); > @@ -479,8 +483,13 @@ int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags, > INIT_LIST_HEAD(&syncobj->cb_list); > spin_lock_init(&syncobj->lock); > > - if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) > - drm_syncobj_assign_null_handle(syncobj); > + if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) { > + ret = drm_syncobj_assign_null_handle(syncobj); > + if (ret < 0) { > + drm_syncobj_put(syncobj); > + return ret; > + } > + } > > if (fence) > drm_syncobj_replace_fence(syncobj, fence); > @@ -1322,8 +1331,11 @@ drm_syncobj_signal_ioctl(struct drm_device *dev, void *data, > if (ret < 0) > return ret; > > - for (i = 0; i < args->count_handles; i++) > - drm_syncobj_assign_null_handle(syncobjs[i]); > + for (i = 0; i < args->count_handles; i++) { > + ret = drm_syncobj_assign_null_handle(syncobjs[i]); > + if (ret < 0) > + break; > + } > > drm_syncobj_array_free(syncobjs, args->count_handles); > > diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h > index 9f12efaaa93a..6ffb4b2c6371 100644 > --- a/include/linux/dma-fence.h > +++ b/include/linux/dma-fence.h > @@ -587,6 +587,7 @@ static inline signed long dma_fence_wait(struct dma_fence *fence, bool intr) > } > > struct dma_fence *dma_fence_get_stub(void); > +struct dma_fence *dma_fence_allocate_private_stub(void); > u64 dma_fence_context_alloc(unsigned num); > > #define DMA_FENCE_TRACE(f, fmt, args...) \ > -- > 2.31.0.208.g409f899ff0-goog > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
WARNING: multiple messages have this Message-ID (diff)
From: Daniel Vetter <daniel@ffwll.ch> To: David Stevens <stevensd@chromium.org> Cc: "David Airlie" <airlied@linux.ie>, linux-kernel@vger.kernel.org, linaro-mm-sig@lists.linaro.org, dri-devel@lists.freedesktop.org, "Thomas Zimmermann" <tzimmermann@suse.de>, "Christian König" <christian.koenig@amd.com>, linux-media@vger.kernel.org Subject: Re: [PATCH] drm/syncobj: use newly allocated stub fences Date: Thu, 8 Apr 2021 13:40:26 +0200 [thread overview] Message-ID: <YG7rqkzLRBubwtk4@phenom.ffwll.local> (raw) In-Reply-To: <20210408093448.3897988-1-stevensd@google.com> On Thu, Apr 08, 2021 at 06:34:48PM +0900, David Stevens wrote: > From: David Stevens <stevensd@chromium.org> > > Allocate a new private stub fence in drm_syncobj_assign_null_handle, > instead of using a static stub fence. > > When userspace creates a fence with DRM_SYNCOBJ_CREATE_SIGNALED or when > userspace signals a fence via DRM_IOCTL_SYNCOBJ_SIGNAL, the timestamp > obtained when the fence is exported and queried with SYNC_IOC_FILE_INFO > should match when the fence's status was changed from the perspective of > userspace, which is during the respective ioctl. > > When a static stub fence started being used in by these ioctls, this > behavior changed. Instead, the timestamp returned by SYNC_IOC_FILE_INFO > became the first time anything used the static stub fence, which has no > meaning to userspace. > > Signed-off-by: David Stevens <stevensd@chromium.org> Is there also an igt patch to enforce this in the drm_syncobj testcases? Would be really good to have that too. -Daniel > --- > drivers/dma-buf/dma-fence.c | 33 ++++++++++++++++++++++++++++++++- > drivers/gpu/drm/drm_syncobj.c | 28 ++++++++++++++++++++-------- > include/linux/dma-fence.h | 1 + > 3 files changed, 53 insertions(+), 9 deletions(-) > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > index d64fc03929be..6081eb962490 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -26,6 +26,11 @@ EXPORT_TRACEPOINT_SYMBOL(dma_fence_signaled); > static DEFINE_SPINLOCK(dma_fence_stub_lock); > static struct dma_fence dma_fence_stub; > > +struct drm_fence_private_stub { > + struct dma_fence base; > + spinlock_t lock; > +}; > + > /* > * fence context counter: each execution context should have its own > * fence context, this allows checking if fences belong to the same > @@ -123,7 +128,9 @@ static const struct dma_fence_ops dma_fence_stub_ops = { > /** > * dma_fence_get_stub - return a signaled fence > * > - * Return a stub fence which is already signaled. > + * Return a stub fence which is already signaled. The fence's > + * timestamp corresponds to the first time after boot this > + * function is called. > */ > struct dma_fence *dma_fence_get_stub(void) > { > @@ -141,6 +148,30 @@ struct dma_fence *dma_fence_get_stub(void) > } > EXPORT_SYMBOL(dma_fence_get_stub); > > +/** > + * dma_fence_allocate_private_stub - return a private, signaled fence > + * > + * Return a newly allocated and signaled stub fence. > + */ > +struct dma_fence *dma_fence_allocate_private_stub(void) > +{ > + struct drm_fence_private_stub *fence; > + > + fence = kzalloc(sizeof(*fence), GFP_KERNEL); > + if (fence == NULL) > + return ERR_PTR(-ENOMEM); > + > + spin_lock_init(&fence->lock); > + dma_fence_init(&fence->base, > + &dma_fence_stub_ops, > + &fence->lock, > + 0, 0); > + dma_fence_signal(&fence->base); > + > + return &fence->base; > +} > +EXPORT_SYMBOL(dma_fence_allocate_private_stub); > + > /** > * dma_fence_context_alloc - allocate an array of fence contexts > * @num: amount of contexts to allocate > diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c > index 349146049849..c6125e57ae37 100644 > --- a/drivers/gpu/drm/drm_syncobj.c > +++ b/drivers/gpu/drm/drm_syncobj.c > @@ -350,12 +350,15 @@ EXPORT_SYMBOL(drm_syncobj_replace_fence); > * > * Assign a already signaled stub fence to the sync object. > */ > -static void drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj) > +static int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj) > { > - struct dma_fence *fence = dma_fence_get_stub(); > + struct dma_fence *fence = dma_fence_allocate_private_stub(); > + if (IS_ERR(fence)) > + return PTR_ERR(fence); > > - drm_syncobj_replace_fence(syncobj, fence); > - dma_fence_put(fence); > + drm_syncobj_replace_fence(syncobj, fence); > + dma_fence_put(fence); > + return 0; > } > > /* 5s default for wait submission */ > @@ -469,6 +472,7 @@ EXPORT_SYMBOL(drm_syncobj_free); > int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags, > struct dma_fence *fence) > { > + int ret; > struct drm_syncobj *syncobj; > > syncobj = kzalloc(sizeof(struct drm_syncobj), GFP_KERNEL); > @@ -479,8 +483,13 @@ int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags, > INIT_LIST_HEAD(&syncobj->cb_list); > spin_lock_init(&syncobj->lock); > > - if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) > - drm_syncobj_assign_null_handle(syncobj); > + if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) { > + ret = drm_syncobj_assign_null_handle(syncobj); > + if (ret < 0) { > + drm_syncobj_put(syncobj); > + return ret; > + } > + } > > if (fence) > drm_syncobj_replace_fence(syncobj, fence); > @@ -1322,8 +1331,11 @@ drm_syncobj_signal_ioctl(struct drm_device *dev, void *data, > if (ret < 0) > return ret; > > - for (i = 0; i < args->count_handles; i++) > - drm_syncobj_assign_null_handle(syncobjs[i]); > + for (i = 0; i < args->count_handles; i++) { > + ret = drm_syncobj_assign_null_handle(syncobjs[i]); > + if (ret < 0) > + break; > + } > > drm_syncobj_array_free(syncobjs, args->count_handles); > > diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h > index 9f12efaaa93a..6ffb4b2c6371 100644 > --- a/include/linux/dma-fence.h > +++ b/include/linux/dma-fence.h > @@ -587,6 +587,7 @@ static inline signed long dma_fence_wait(struct dma_fence *fence, bool intr) > } > > struct dma_fence *dma_fence_get_stub(void); > +struct dma_fence *dma_fence_allocate_private_stub(void); > u64 dma_fence_context_alloc(unsigned num); > > #define DMA_FENCE_TRACE(f, fmt, args...) \ > -- > 2.31.0.208.g409f899ff0-goog > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2021-04-08 11:40 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-04-08 9:34 [PATCH] drm/syncobj: use newly allocated stub fences David Stevens 2021-04-08 9:34 ` David Stevens 2021-04-08 9:36 ` David Stevens 2021-04-08 9:36 ` David Stevens 2021-04-08 9:41 ` Christian König 2021-04-08 9:41 ` Christian König 2021-04-08 11:40 ` Daniel Vetter [this message] 2021-04-08 11:40 ` Daniel Vetter
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=YG7rqkzLRBubwtk4@phenom.ffwll.local \ --to=daniel@ffwll.ch \ --cc=airlied@linux.ie \ --cc=christian.koenig@amd.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=linaro-mm-sig@lists.linaro.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-media@vger.kernel.org \ --cc=maarten.lankhorst@linux.intel.com \ --cc=mripard@kernel.org \ --cc=stevensd@chromium.org \ --cc=sumit.semwal@linaro.org \ --cc=tzimmermann@suse.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.