Hi all,
Following Jason's suggestion on another thread adding timeline documentation [1], here is a small series adding a creation flag to syncobjs so that users are prevented to drop the existing timeline fences in the syncobj, effectivelly ensuring a user always adds to the dma_fence_chain instead of replacing it.
We still allow explicit reset.
Apart from the fact we need to enforce this policy in each driver's submission path, I haven't run into odds things yet.
Cheers,
[1] : https://lists.freedesktop.org/archives/dri-devel/2019-August/232700.html
Lionel Landwerlin (3): drm/syncobj: protect timeline syncobjs drm/amd/amdgpu: disallow replacing fences in timeline syncobjs drm/i915: disallow replacing fences of timeline syncobjs
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 ++ drivers/gpu/drm/drm_syncobj.c | 30 ++++++++++++++++++- .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 8 +++++ include/drm/drm_syncobj.h | 8 +++++ include/uapi/drm/drm.h | 1 + 5 files changed, 48 insertions(+), 1 deletion(-)
-- 2.23.0
Binary/legacy signal operations on a syncobj work by replacing the dma_fence held within the syncobj. Whe dealing with timeline semaphores we would like to avoid this as this would effectivelly lead to looser synchronization (by discarding the dma_fence_chain mechanism waiting on all previous dma_fence to signal before signal itself).
This change adds a flags that can be used at creation of the syncobj to mean that the syncobj will hold a timeline of dma_fence (using dma_fence_chain). When flagged as such, the dma_fence held by the syncobj should not be replaced but instead we should always adding to the timeline.
Signed-off-by: Lionel Landwerlin lionel.g.landwerlin@intel.com --- drivers/gpu/drm/drm_syncobj.c | 30 +++++++++++++++++++++++++++++- include/drm/drm_syncobj.h | 8 ++++++++ include/uapi/drm/drm.h | 1 + 3 files changed, 38 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index 72d083acd388..69d43c791a42 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -476,6 +476,8 @@ int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags,
if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) drm_syncobj_assign_null_handle(syncobj); + if (flags & DRM_SYNCOBJ_CREATE_TIMELINE) + syncobj->is_timeline = true;
if (fence) drm_syncobj_replace_fence(syncobj, fence); @@ -661,6 +663,10 @@ static int drm_syncobj_import_sync_file_fence(struct drm_file *file_private, dma_fence_put(fence); return -ENOENT; } + if (syncobj->is_timeline) { + dma_fence_put(fence); + return -EINVAL; + }
drm_syncobj_replace_fence(syncobj, fence); dma_fence_put(fence); @@ -749,7 +755,13 @@ drm_syncobj_create_ioctl(struct drm_device *dev, void *data, return -EOPNOTSUPP;
/* no valid flags yet */ - if (args->flags & ~DRM_SYNCOBJ_CREATE_SIGNALED) + if (args->flags & ~(DRM_SYNCOBJ_CREATE_SIGNALED | + DRM_SYNCOBJ_CREATE_TIMELINE)) + return -EINVAL; + + /* Creating a signaled timeline makes no sense. */ + if ((args->flags & DRM_SYNCOBJ_CREATE_SIGNALED) && + (args->flags & DRM_SYNCOBJ_CREATE_TIMELINE)) return -EINVAL;
return drm_syncobj_create_as_handle(file_private, @@ -862,6 +874,10 @@ drm_syncobj_transfer_to_binary(struct drm_file *file_private, binary_syncobj = drm_syncobj_find(file_private, args->dst_handle); if (!binary_syncobj) return -ENOENT; + if (binary_syncobj->is_timeline) { + ret = -EINVAL; + goto err; + } ret = drm_syncobj_find_fence(file_private, args->src_handle, args->src_point, args->flags, &fence); if (ret) @@ -1137,6 +1153,7 @@ static int drm_syncobj_array_wait(struct drm_device *dev, static int drm_syncobj_array_find(struct drm_file *file_private, void __user *user_handles, uint32_t count_handles, + bool no_timeline, struct drm_syncobj ***syncobjs_out) { uint32_t i, *handles; @@ -1165,6 +1182,10 @@ static int drm_syncobj_array_find(struct drm_file *file_private, ret = -ENOENT; goto err_put_syncobjs; } + if (no_timeline && syncobjs[i]->is_timeline) { + ret = -EINVAL; + goto err_put_syncobjs; + } }
kfree(handles); @@ -1211,6 +1232,7 @@ drm_syncobj_wait_ioctl(struct drm_device *dev, void *data, ret = drm_syncobj_array_find(file_private, u64_to_user_ptr(args->handles), args->count_handles, + false, &syncobjs); if (ret < 0) return ret; @@ -1245,6 +1267,7 @@ drm_syncobj_timeline_wait_ioctl(struct drm_device *dev, void *data, ret = drm_syncobj_array_find(file_private, u64_to_user_ptr(args->handles), args->count_handles, + false, &syncobjs); if (ret < 0) return ret; @@ -1279,6 +1302,7 @@ drm_syncobj_reset_ioctl(struct drm_device *dev, void *data, ret = drm_syncobj_array_find(file_private, u64_to_user_ptr(args->handles), args->count_handles, + false, &syncobjs); if (ret < 0) return ret; @@ -1314,6 +1338,7 @@ drm_syncobj_signal_ioctl(struct drm_device *dev, void *data, ret = drm_syncobj_array_find(file_private, u64_to_user_ptr(args->handles), args->count_handles, + true, &syncobjs); if (ret < 0) return ret; @@ -1349,6 +1374,7 @@ drm_syncobj_timeline_signal_ioctl(struct drm_device *dev, void *data, ret = drm_syncobj_array_find(file_private, u64_to_user_ptr(args->handles), args->count_handles, + false, &syncobjs); if (ret < 0) return ret; @@ -1420,6 +1446,7 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data, ret = drm_syncobj_array_find(file_private, u64_to_user_ptr(args->handles), args->count_handles, + false, &syncobjs); if (ret < 0) return ret; @@ -1484,6 +1511,7 @@ int drm_syncobj_binary_ioctl(struct drm_device *dev, void *data, ret = drm_syncobj_array_find(file_private, u64_to_user_ptr(args->handles), args->count_handles, + false, &syncobjs); if (ret < 0) return ret; diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h index aa76cb3f9107..f96a95fbe025 100644 --- a/include/drm/drm_syncobj.h +++ b/include/drm/drm_syncobj.h @@ -70,6 +70,14 @@ struct drm_syncobj { * seqno will be put into the syncobj. */ atomic64_t binary_payload; + /** + * @is_timeline: Whether the syncobj holds a timeline. + * + * Holding a timeline adds some restriction to prevent userspace from + * accidentally "losing" the timeline. This could happen for example + * by replacing &fence. + */ + bool is_timeline; };
void drm_syncobj_free(struct kref *kref); diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 78a0a413b788..b3c46b2993be 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -719,6 +719,7 @@ struct drm_prime_handle { struct drm_syncobj_create { __u32 handle; #define DRM_SYNCOBJ_CREATE_SIGNALED (1 << 0) +#define DRM_SYNCOBJ_CREATE_TIMELINE (1 << 1) __u32 flags; };
Similarly to the host path from drm_syncobj.c we would like to disallow those operations to help applications figure where they using the wrong kind of ioctl.
Signed-off-by: Lionel Landwerlin lionel.g.landwerlin@intel.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c index 2e53feed40e2..d9bbc31e97d0 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c @@ -1159,6 +1159,8 @@ static int amdgpu_cs_process_syncobj_out_dep(struct amdgpu_cs_parser *p, drm_syncobj_find(p->filp, deps[i].handle); if (!p->post_deps[i].syncobj) return -EINVAL; + if (p->post_deps[i].syncobj->is_timeline) + return -EINVAL; p->post_deps[i].chain = NULL; p->post_deps[i].point = 0; p->num_post_deps++;
Signed-off-by: Lionel Landwerlin lionel.g.landwerlin@intel.com --- drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 09248398fa7b..f1af3490f96b 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -2494,6 +2494,14 @@ get_legacy_fence_array(struct i915_execbuffer *eb, goto err; }
+ if ((user_fence.flags & I915_EXEC_FENCE_SIGNAL) && + syncobj->is_timeline) { + DRM_DEBUG("Cannot replace fence in timeline syncobj\n"); + drm_syncobj_put(syncobj); + err = -EINVAL; + goto err; + } + if (user_fence.flags & I915_EXEC_FENCE_WAIT) { fence = drm_syncobj_fence_get(syncobj); if (!fence) {
dri-devel@lists.freedesktop.org
-
Lionel Landwerlin