[Intel-gfx] [PATCH 2/3] drm/i915: add syncobj timeline support
Introduces a new parameters to execbuf so that we can specify syncobj
handles as well as timeline points.
v2: Reuse i915_user_extension_fn
v3: Check that the chained extension is only present once (Chris)
v4: Check that dma_fence_chain_find_seqno returns a non NULL fence (Lionel)
v5: Use BIT_ULL (Chris)
v6: Fix issue with already signaled timeline points,
dma_fence_chain_find_seqno() setting fence to NULL (Chris)
v7: Report ENOENT with invalid syncobj handle (Lionel)
v8: Check for out of order timeline point insertion (Chris)
v9: After explanations on
https://lists.freedesktop.org/archives/dri-devel/2019-August/229287.html
drop the ordering check from v8 (Lionel)
v10: Set first extension enum item to 1 (Jason)
v11: Rebase
Signed-off-by: Lionel Landwerlin
Reviewed-by: Daniel Vetter
---
.../gpu/drm/i915/gem/i915_gem_execbuffer.c| 167 +-
drivers/gpu/drm/i915/i915_drv.c | 3 +-
drivers/gpu/drm/i915/i915_getparam.c | 1 +
include/uapi/drm/i915_drm.h | 39
4 files changed, 203 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index ed8d1c2517f6..1f766431f3a3 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -285,6 +285,8 @@ struct i915_execbuffer {
struct i915_eb_fence {
struct drm_syncobj *syncobj; /* Use with ptr_mask_bits() */
+ u64 value;
+ struct dma_fence_chain *chain_fence;
} *fences;
u32 n_fences;
@@ -2200,14 +2202,121 @@ eb_pin_engine(struct i915_execbuffer *eb,
static void
__free_fence_array(struct i915_eb_fence *fences, unsigned int n)
{
- while (n--)
+ while (n--) {
drm_syncobj_put(ptr_mask_bits(fences[n].syncobj, 2));
+ kfree(fences[n].chain_fence);
+ }
kvfree(fences);
}
static int
-get_fence_array(struct drm_i915_gem_execbuffer2 *args,
- struct i915_execbuffer *eb)
+get_timeline_fence_array(const struct
drm_i915_gem_execbuffer_ext_timeline_fences *timeline_fences,
+struct i915_execbuffer *eb)
+{
+ struct drm_i915_gem_exec_fence __user *user_fences;
+ struct i915_eb_fence *fences;
+ u64 __user *user_values;
+ u64 num_fences, num_user_fences = timeline_fences->fence_count;
+ unsigned long n;
+ int err;
+
+ /* Check multiplication overflow for access_ok() and kvmalloc_array() */
+ BUILD_BUG_ON(sizeof(size_t) > sizeof(unsigned long));
+ if (num_user_fences > min_t(unsigned long,
+ ULONG_MAX / sizeof(*user_fences),
+ SIZE_MAX / sizeof(*fences)))
+ return -EINVAL;
+
+ user_fences = u64_to_user_ptr(timeline_fences->handles_ptr);
+ if (!access_ok(user_fences, num_user_fences * sizeof(*user_fences)))
+ return -EFAULT;
+
+ user_values = u64_to_user_ptr(timeline_fences->values_ptr);
+ if (!access_ok(user_values, num_user_fences * sizeof(*user_values)))
+ return -EFAULT;
+
+ fences = kvmalloc_array(num_user_fences, sizeof(*fences),
+ __GFP_NOWARN | GFP_KERNEL);
+ if (!fences)
+ return -ENOMEM;
+
+ BUILD_BUG_ON(~(ARCH_KMALLOC_MINALIGN - 1) &
+~__I915_EXEC_FENCE_UNKNOWN_FLAGS);
+
+ for (n = 0, num_fences = 0; n < timeline_fences->fence_count; n++) {
+ struct drm_i915_gem_exec_fence fence;
+ struct drm_syncobj *syncobj;
+ u64 point;
+
+ if (__copy_from_user(&fence, user_fences++, sizeof(fence))) {
+ err = -EFAULT;
+ goto err;
+ }
+
+ if (fence.flags & __I915_EXEC_FENCE_UNKNOWN_FLAGS) {
+ err = -EINVAL;
+ goto err;
+ }
+
+ if (__get_user(point, user_values++)) {
+ err = -EFAULT;
+ goto err;
+ }
+
+ syncobj = drm_syncobj_find(eb->file, fence.handle);
+ if (!syncobj) {
+ DRM_DEBUG("Invalid syncobj handle provided\n");
+ err = -ENOENT;
+ goto err;
+ }
+
+ /*
+* For timeline syncobjs we need to preallocate chains for
+* later signaling.
+*/
+ if (point != 0 && fence.flags & I915_EXEC_FENCE_SIGNAL) {
+ /*
+* Waiting and signaling the same point (when point !=
+* 0) would break the timeline.
+*/
+ if (fence.flags & I915_EXEC_FENCE_WAIT) {
+ DRM_DEBUG("
[Intel-gfx] [PATCH 2/3] drm/i915: add syncobj timeline support
Introduces a new parameters to execbuf so that we can specify syncobj
handles as well as timeline points.
v2: Reuse i915_user_extension_fn
v3: Check that the chained extension is only present once (Chris)
v4: Check that dma_fence_chain_find_seqno returns a non NULL fence (Lionel)
v5: Use BIT_ULL (Chris)
v6: Fix issue with already signaled timeline points,
dma_fence_chain_find_seqno() setting fence to NULL (Chris)
v7: Report ENOENT with invalid syncobj handle (Lionel)
v8: Check for out of order timeline point insertion (Chris)
v9: After explanations on
https://lists.freedesktop.org/archives/dri-devel/2019-August/229287.html
drop the ordering check from v8 (Lionel)
v10: Set first extension enum item to 1 (Jason)
v11: Rebase
Signed-off-by: Lionel Landwerlin
Reviewed-by: Daniel Vetter
---
.../gpu/drm/i915/gem/i915_gem_execbuffer.c| 167 +-
drivers/gpu/drm/i915/i915_drv.c | 3 +-
drivers/gpu/drm/i915/i915_getparam.c | 1 +
include/uapi/drm/i915_drm.h | 39
4 files changed, 203 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index ed8d1c2517f6..1f766431f3a3 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -285,6 +285,8 @@ struct i915_execbuffer {
struct i915_eb_fence {
struct drm_syncobj *syncobj; /* Use with ptr_mask_bits() */
+ u64 value;
+ struct dma_fence_chain *chain_fence;
} *fences;
u32 n_fences;
@@ -2200,14 +2202,121 @@ eb_pin_engine(struct i915_execbuffer *eb,
static void
__free_fence_array(struct i915_eb_fence *fences, unsigned int n)
{
- while (n--)
+ while (n--) {
drm_syncobj_put(ptr_mask_bits(fences[n].syncobj, 2));
+ kfree(fences[n].chain_fence);
+ }
kvfree(fences);
}
static int
-get_fence_array(struct drm_i915_gem_execbuffer2 *args,
- struct i915_execbuffer *eb)
+get_timeline_fence_array(const struct
drm_i915_gem_execbuffer_ext_timeline_fences *timeline_fences,
+struct i915_execbuffer *eb)
+{
+ struct drm_i915_gem_exec_fence __user *user_fences;
+ struct i915_eb_fence *fences;
+ u64 __user *user_values;
+ u64 num_fences, num_user_fences = timeline_fences->fence_count;
+ unsigned long n;
+ int err;
+
+ /* Check multiplication overflow for access_ok() and kvmalloc_array() */
+ BUILD_BUG_ON(sizeof(size_t) > sizeof(unsigned long));
+ if (num_user_fences > min_t(unsigned long,
+ ULONG_MAX / sizeof(*user_fences),
+ SIZE_MAX / sizeof(*fences)))
+ return -EINVAL;
+
+ user_fences = u64_to_user_ptr(timeline_fences->handles_ptr);
+ if (!access_ok(user_fences, num_user_fences * sizeof(*user_fences)))
+ return -EFAULT;
+
+ user_values = u64_to_user_ptr(timeline_fences->values_ptr);
+ if (!access_ok(user_values, num_user_fences * sizeof(*user_values)))
+ return -EFAULT;
+
+ fences = kvmalloc_array(num_user_fences, sizeof(*fences),
+ __GFP_NOWARN | GFP_KERNEL);
+ if (!fences)
+ return -ENOMEM;
+
+ BUILD_BUG_ON(~(ARCH_KMALLOC_MINALIGN - 1) &
+~__I915_EXEC_FENCE_UNKNOWN_FLAGS);
+
+ for (n = 0, num_fences = 0; n < timeline_fences->fence_count; n++) {
+ struct drm_i915_gem_exec_fence fence;
+ struct drm_syncobj *syncobj;
+ u64 point;
+
+ if (__copy_from_user(&fence, user_fences++, sizeof(fence))) {
+ err = -EFAULT;
+ goto err;
+ }
+
+ if (fence.flags & __I915_EXEC_FENCE_UNKNOWN_FLAGS) {
+ err = -EINVAL;
+ goto err;
+ }
+
+ if (__get_user(point, user_values++)) {
+ err = -EFAULT;
+ goto err;
+ }
+
+ syncobj = drm_syncobj_find(eb->file, fence.handle);
+ if (!syncobj) {
+ DRM_DEBUG("Invalid syncobj handle provided\n");
+ err = -ENOENT;
+ goto err;
+ }
+
+ /*
+* For timeline syncobjs we need to preallocate chains for
+* later signaling.
+*/
+ if (point != 0 && fence.flags & I915_EXEC_FENCE_SIGNAL) {
+ /*
+* Waiting and signaling the same point (when point !=
+* 0) would break the timeline.
+*/
+ if (fence.flags & I915_EXEC_FENCE_WAIT) {
+ DRM_DEBUG("
Re: [Intel-gfx] [PATCH 2/3] drm/i915: add syncobj timeline support
On Fri, Jul 31, 2020 at 9:07 PM Lionel Landwerlin
wrote:
>
> On 31/07/2020 17:30, Chris Wilson wrote:
> > Quoting Lionel Landwerlin (2020-07-31 14:45:52)
> >> - drm_syncobj_replace_fence(syncobj, fence);
> >> + if (eb->fences[n].chain_fence) {
> >> + drm_syncobj_add_point(syncobj,
> >> eb->fences[n].chain_fence,
> >> + fence, eb->fences[n].value);
> > This function remains not acceptable. It is trivial to write an igt test
> > that causes the DRM_ERROR. A user controllable error message is still
> > not allowed. If you do not have such a test in your igt series, then that
> > too is lacking.
> > -Chris
>
> As far as I remember there are IGT tests for this (*unordered* subtests).
>
> So CI should report a fairlure. The IGT test itself won't fail though.
CI did catch it.
> I thought we removed that DRM_ERROR a while ago.
>
> I'll send a patch to remove it. Thanks.
Typed it already (since I didn't see yours), as soon as it's pushed
you can just hit the retest button with your current series and then
we see whether it's all gone.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] drm/i915: add syncobj timeline support
On 31/07/2020 17:32, Chris Wilson wrote: Quoting Lionel Landwerlin (2020-07-31 14:45:52) Introduces a new parameters to execbuf so that we can specify syncobj handles as well as timeline points. v2: Reuse i915_user_extension_fn v3: Check that the chained extension is only present once (Chris) v4: Check that dma_fence_chain_find_seqno returns a non NULL fence (Lionel) v5: Use BIT_ULL (Chris) v6: Fix issue with already signaled timeline points, dma_fence_chain_find_seqno() setting fence to NULL (Chris) v7: Report ENOENT with invalid syncobj handle (Lionel) v8: Check for out of order timeline point insertion (Chris) v9: After explanations on https://lists.freedesktop.org/archives/dri-devel/2019-August/229287.html drop the ordering check from v8 (Lionel) v10: Set first extension enum item to 1 (Jason) The other unaddressed issue here is that we do not need to arbitrarily limit the caller to only a single extension. The code to handle multiple invocations is actually smaller... -Chris You mean an application could send multiple DRM_I915_GEM_EXECBUFFER_EXT_TIMELINE_FENCES items in the chain of extensions? That's somewhat different than how the current fences are handled. If other extension want to support that, that's up to them. We don't have any use for multiple arrays of timeline fences for a given execbuf in our userspace driver. -Lionel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] drm/i915: add syncobj timeline support
On 31/07/2020 17:30, Chris Wilson wrote:
Quoting Lionel Landwerlin (2020-07-31 14:45:52)
- drm_syncobj_replace_fence(syncobj, fence);
+ if (eb->fences[n].chain_fence) {
+ drm_syncobj_add_point(syncobj,
eb->fences[n].chain_fence,
+ fence, eb->fences[n].value);
This function remains not acceptable. It is trivial to write an igt test
that causes the DRM_ERROR. A user controllable error message is still
not allowed. If you do not have such a test in your igt series, then that
too is lacking.
-Chris
As far as I remember there are IGT tests for this (*unordered* subtests).
So CI should report a fairlure. The IGT test itself won't fail though.
I thought we removed that DRM_ERROR a while ago.
I'll send a patch to remove it. Thanks.
-Lionel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] drm/i915: add syncobj timeline support
Quoting Lionel Landwerlin (2020-07-31 14:45:52) > Introduces a new parameters to execbuf so that we can specify syncobj > handles as well as timeline points. > > v2: Reuse i915_user_extension_fn > > v3: Check that the chained extension is only present once (Chris) > > v4: Check that dma_fence_chain_find_seqno returns a non NULL fence (Lionel) > > v5: Use BIT_ULL (Chris) > > v6: Fix issue with already signaled timeline points, > dma_fence_chain_find_seqno() setting fence to NULL (Chris) > > v7: Report ENOENT with invalid syncobj handle (Lionel) > > v8: Check for out of order timeline point insertion (Chris) > > v9: After explanations on > https://lists.freedesktop.org/archives/dri-devel/2019-August/229287.html > drop the ordering check from v8 (Lionel) > > v10: Set first extension enum item to 1 (Jason) The other unaddressed issue here is that we do not need to arbitrarily limit the caller to only a single extension. The code to handle multiple invocations is actually smaller... -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] drm/i915: add syncobj timeline support
Quoting Lionel Landwerlin (2020-07-31 14:45:52)
> - drm_syncobj_replace_fence(syncobj, fence);
> + if (eb->fences[n].chain_fence) {
> + drm_syncobj_add_point(syncobj,
> eb->fences[n].chain_fence,
> + fence, eb->fences[n].value);
This function remains not acceptable. It is trivial to write an igt test
that causes the DRM_ERROR. A user controllable error message is still
not allowed. If you do not have such a test in your igt series, then that
too is lacking.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/3] drm/i915: add syncobj timeline support
Introduces a new parameters to execbuf so that we can specify syncobj
handles as well as timeline points.
v2: Reuse i915_user_extension_fn
v3: Check that the chained extension is only present once (Chris)
v4: Check that dma_fence_chain_find_seqno returns a non NULL fence (Lionel)
v5: Use BIT_ULL (Chris)
v6: Fix issue with already signaled timeline points,
dma_fence_chain_find_seqno() setting fence to NULL (Chris)
v7: Report ENOENT with invalid syncobj handle (Lionel)
v8: Check for out of order timeline point insertion (Chris)
v9: After explanations on
https://lists.freedesktop.org/archives/dri-devel/2019-August/229287.html
drop the ordering check from v8 (Lionel)
v10: Set first extension enum item to 1 (Jason)
v11: Rebase
Signed-off-by: Lionel Landwerlin
Reviewed-by: Daniel Vetter
---
.../gpu/drm/i915/gem/i915_gem_execbuffer.c| 167 +-
drivers/gpu/drm/i915/i915_drv.c | 3 +-
drivers/gpu/drm/i915/i915_getparam.c | 1 +
include/uapi/drm/i915_drm.h | 39
4 files changed, 203 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 5aac474c058f..652f3b30a374 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -285,6 +285,8 @@ struct i915_execbuffer {
struct i915_eb_fence {
struct drm_syncobj *syncobj; /* Use with ptr_mask_bits() */
+ u64 value;
+ struct dma_fence_chain *chain_fence;
} *fences;
u32 n_fences;
@@ -2200,14 +2202,121 @@ eb_pin_engine(struct i915_execbuffer *eb,
static void
__free_fence_array(struct i915_eb_fence *fences, unsigned int n)
{
- while (n--)
+ while (n--) {
drm_syncobj_put(ptr_mask_bits(fences[n].syncobj, 2));
+ kfree(fences[n].chain_fence);
+ }
kvfree(fences);
}
static int
-get_fence_array(struct drm_i915_gem_execbuffer2 *args,
- struct i915_execbuffer *eb)
+get_timeline_fence_array(const struct
drm_i915_gem_execbuffer_ext_timeline_fences *timeline_fences,
+struct i915_execbuffer *eb)
+{
+ struct drm_i915_gem_exec_fence __user *user_fences;
+ struct i915_eb_fence *fences;
+ u64 __user *user_values;
+ u64 num_fences, num_user_fences = timeline_fences->fence_count;
+ unsigned long n;
+ int err;
+
+ /* Check multiplication overflow for access_ok() and kvmalloc_array() */
+ BUILD_BUG_ON(sizeof(size_t) > sizeof(unsigned long));
+ if (num_user_fences > min_t(unsigned long,
+ ULONG_MAX / sizeof(*user_fences),
+ SIZE_MAX / sizeof(*fences)))
+ return -EINVAL;
+
+ user_fences = u64_to_user_ptr(timeline_fences->handles_ptr);
+ if (!access_ok(user_fences, num_user_fences * sizeof(*user_fences)))
+ return -EFAULT;
+
+ user_values = u64_to_user_ptr(timeline_fences->values_ptr);
+ if (!access_ok(user_values, num_user_fences * sizeof(*user_values)))
+ return -EFAULT;
+
+ fences = kvmalloc_array(num_user_fences, sizeof(*fences),
+ __GFP_NOWARN | GFP_KERNEL);
+ if (!fences)
+ return -ENOMEM;
+
+ BUILD_BUG_ON(~(ARCH_KMALLOC_MINALIGN - 1) &
+~__I915_EXEC_FENCE_UNKNOWN_FLAGS);
+
+ for (n = 0, num_fences = 0; n < timeline_fences->fence_count; n++) {
+ struct drm_i915_gem_exec_fence fence;
+ struct drm_syncobj *syncobj;
+ u64 point;
+
+ if (__copy_from_user(&fence, user_fences++, sizeof(fence))) {
+ err = -EFAULT;
+ goto err;
+ }
+
+ if (fence.flags & __I915_EXEC_FENCE_UNKNOWN_FLAGS) {
+ err = -EINVAL;
+ goto err;
+ }
+
+ if (__get_user(point, user_values++)) {
+ err = -EFAULT;
+ goto err;
+ }
+
+ syncobj = drm_syncobj_find(eb->file, fence.handle);
+ if (!syncobj) {
+ DRM_DEBUG("Invalid syncobj handle provided\n");
+ err = -ENOENT;
+ goto err;
+ }
+
+ /*
+* For timeline syncobjs we need to preallocate chains for
+* later signaling.
+*/
+ if (point != 0 && fence.flags & I915_EXEC_FENCE_SIGNAL) {
+ /*
+* Waiting and signaling the same point (when point !=
+* 0) would break the timeline.
+*/
+ if (fence.flags & I915_EXEC_FENCE_WAIT) {
+ DRM_DEBUG("
Re: [Intel-gfx] [PATCH 2/3] drm/i915: add syncobj timeline support
On 06/04/2020 23:07, Venkata Sandeep Dhanalakota wrote:
Introduces a new parameters to execbuf so that we can specify syncobj
handles as well as timeline points.
v2: Reuse i915_user_extension_fn
v3: Check that the chained extension is only present once (Chris)
v4: Check that dma_fence_chain_find_seqno returns a non NULL fence
(Lionel)
v5: Use BIT_ULL (Chris)
v6: Fix issue with already signaled timeline points,
dma_fence_chain_find_seqno() setting fence to NULL (Chris)
v7: Report ENOENT with invalid syncobj handle (Lionel)
v8: Check for out of order timeline point insertion (Chris)
v9: After explanations on
https://lists.freedesktop.org/archives/dri-devel/2019-August/229287.html
drop the ordering check from v8 (Lionel)
v10: Set first extension enum item to 1 (Jason)
v11: Add wait on previous sync points in timelines (Sandeep)
Signed-off-by: Lionel Landwerlin
Signed-off-by: Venkata Sandeep Dhanalakota
---
.../gpu/drm/i915/gem/i915_gem_execbuffer.c| 312 ++
drivers/gpu/drm/i915/i915_drv.c | 3 +-
drivers/gpu/drm/i915/i915_getparam.c | 1 +
include/uapi/drm/i915_drm.h | 38 +++
4 files changed, 296 insertions(+), 58 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 16831f715daa..4cb4cd035daa 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -230,6 +230,13 @@ enum {
* the batchbuffer in trusted mode, otherwise the ioctl is rejected.
*/
+struct i915_eb_fences {
+ struct drm_syncobj *syncobj; /* Use with ptr_mask_bits() */
+ struct dma_fence *dma_fence;
+ u64 value;
+ struct dma_fence_chain *chain_fence;
+};
+
struct i915_execbuffer {
struct drm_i915_private *i915; /** i915 backpointer */
struct drm_file *file; /** per-file lookup tables and limits */
@@ -292,6 +299,7 @@ struct i915_execbuffer {
struct {
u64 flags; /** Available extensions parameters */
+ struct drm_i915_gem_execbuffer_ext_timeline_fences
timeline_fences;
} extensions;
};
@@ -2244,67 +2252,219 @@ eb_pin_engine(struct i915_execbuffer *eb,
}
static void
-__free_fence_array(struct drm_syncobj **fences, unsigned int n)
+__free_fence_array(struct i915_eb_fences *fences, unsigned int n)
{
- while (n--)
- drm_syncobj_put(ptr_mask_bits(fences[n], 2));
+ while (n--) {
+ drm_syncobj_put(ptr_mask_bits(fences[n].syncobj, 2));
+ dma_fence_put(fences[n].dma_fence);
+ kfree(fences[n].chain_fence);
+ }
kvfree(fences);
}
-static struct drm_syncobj **
-get_fence_array(struct drm_i915_gem_execbuffer2 *args,
- struct drm_file *file)
+static struct i915_eb_fences *
+get_timeline_fence_array(struct i915_execbuffer *eb, int *out_n_fences)
+{
+ struct drm_i915_gem_execbuffer_ext_timeline_fences *timeline_fences =
+ &eb->extensions.timeline_fences;
+ struct drm_i915_gem_exec_fence __user *user_fences;
+ struct i915_eb_fences *fences;
+ u64 __user *user_values;
+ u64 num_fences, num_user_fences = timeline_fences->fence_count;
+ unsigned long n;
+ int err = 0;
+
+ /* Check multiplication overflow for access_ok() and kvmalloc_array() */
+ BUILD_BUG_ON(sizeof(size_t) > sizeof(unsigned long));
+ if (num_user_fences > min_t(unsigned long,
+ ULONG_MAX / sizeof(*user_fences),
+ SIZE_MAX / sizeof(*fences)))
+ return ERR_PTR(-EINVAL);
+
+ user_fences = u64_to_user_ptr(timeline_fences->handles_ptr);
+ if (!access_ok(user_fences, num_user_fences * sizeof(*user_fences)))
+ return ERR_PTR(-EFAULT);
+
+ user_values = u64_to_user_ptr(timeline_fences->values_ptr);
+ if (!access_ok(user_values, num_user_fences * sizeof(*user_values)))
+ return ERR_PTR(-EFAULT);
+
+ fences = kvmalloc_array(num_user_fences, sizeof(*fences),
+ __GFP_NOWARN | GFP_KERNEL);
+ if (!fences)
+ return ERR_PTR(-ENOMEM);
+
+ BUILD_BUG_ON(~(ARCH_KMALLOC_MINALIGN - 1) &
+~__I915_EXEC_FENCE_UNKNOWN_FLAGS);
+
+ for (n = 0, num_fences = 0; n < timeline_fences->fence_count; n++) {
+ struct drm_i915_gem_exec_fence user_fence;
+ struct drm_syncobj *syncobj;
+ struct dma_fence *fence = NULL;
+ u64 point;
+
+ if (__copy_from_user(&user_fence, user_fences++,
sizeof(user_fence))) {
+ err = -EFAULT;
+ goto err;
+ }
+
+ if (user_fence.flags & __I915_EXEC_FENCE_UNKNOWN_FLAGS) {
+ err = -EINVAL;
+ goto e
Re: [Intel-gfx] [PATCH 2/3] drm/i915: add syncobj timeline support
On 20/04/08 07:29, Lionel Landwerlin wrote:
> On 06/04/2020 23:07, Venkata Sandeep Dhanalakota wrote:
> > Introduces a new parameters to execbuf so that we can specify syncobj
> > handles as well as timeline points.
> >
> > v2: Reuse i915_user_extension_fn
> >
> > v3: Check that the chained extension is only present once (Chris)
> >
> > v4: Check that dma_fence_chain_find_seqno returns a non NULL fence
> > (Lionel)
> >
> > v5: Use BIT_ULL (Chris)
> >
> > v6: Fix issue with already signaled timeline points,
> > dma_fence_chain_find_seqno() setting fence to NULL (Chris)
> >
> > v7: Report ENOENT with invalid syncobj handle (Lionel)
> >
> > v8: Check for out of order timeline point insertion (Chris)
> >
> > v9: After explanations on
> >
> > https://lists.freedesktop.org/archives/dri-devel/2019-August/229287.html
> > drop the ordering check from v8 (Lionel)
> >
> > v10: Set first extension enum item to 1 (Jason)
> >
> > v11: Add wait on previous sync points in timelines (Sandeep)
>
>
> Thanks for picking this series up!
>
>
> Could you point to the changes in v11?
>
> I haven't look at it in a while and I can't remember what you would have
> changed.
>
Hi,
Mainly the changes are in get_timeline_fence_array(), to enforce the
implicit dependencies in signal fence-array. we want have efficient waits
on the last point on timelines so that we signal at a correct point in time
along the timeline
the order is controlled so that we always wait on the previous request/sync
point in the timeline
and signal after the completion of the current request.
Thank you,
~sandeep
>
> Thanks a lot,
>
>
> -Lionel
>
>
> >
> > Signed-off-by: Lionel Landwerlin
> > Signed-off-by: Venkata Sandeep Dhanalakota > intel.com>
> > ---
> > .../gpu/drm/i915/gem/i915_gem_execbuffer.c| 312 ++
> > drivers/gpu/drm/i915/i915_drv.c | 3 +-
> > drivers/gpu/drm/i915/i915_getparam.c | 1 +
> > include/uapi/drm/i915_drm.h | 38 +++
> > 4 files changed, 296 insertions(+), 58 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > index 16831f715daa..4cb4cd035daa 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > @@ -230,6 +230,13 @@ enum {
> >* the batchbuffer in trusted mode, otherwise the ioctl is rejected.
> >*/
> > +struct i915_eb_fences {
> > + struct drm_syncobj *syncobj; /* Use with ptr_mask_bits() */
> > + struct dma_fence *dma_fence;
> > + u64 value;
> > + struct dma_fence_chain *chain_fence;
> > +};
> > +
> > struct i915_execbuffer {
> > struct drm_i915_private *i915; /** i915 backpointer */
> > struct drm_file *file; /** per-file lookup tables and limits */
> > @@ -292,6 +299,7 @@ struct i915_execbuffer {
> > struct {
> > u64 flags; /** Available extensions parameters */
> > + struct drm_i915_gem_execbuffer_ext_timeline_fences
> > timeline_fences;
> > } extensions;
> > };
> > @@ -2244,67 +2252,219 @@ eb_pin_engine(struct i915_execbuffer *eb,
> > }
> > static void
> > -__free_fence_array(struct drm_syncobj **fences, unsigned int n)
> > +__free_fence_array(struct i915_eb_fences *fences, unsigned int n)
> > {
> > - while (n--)
> > - drm_syncobj_put(ptr_mask_bits(fences[n], 2));
> > + while (n--) {
> > + drm_syncobj_put(ptr_mask_bits(fences[n].syncobj, 2));
> > + dma_fence_put(fences[n].dma_fence);
> > + kfree(fences[n].chain_fence);
> > + }
> > kvfree(fences);
> > }
> > -static struct drm_syncobj **
> > -get_fence_array(struct drm_i915_gem_execbuffer2 *args,
> > - struct drm_file *file)
> > +static struct i915_eb_fences *
> > +get_timeline_fence_array(struct i915_execbuffer *eb, int *out_n_fences)
> > +{
> > + struct drm_i915_gem_execbuffer_ext_timeline_fences *timeline_fences =
> > + &eb->extensions.timeline_fences;
> > + struct drm_i915_gem_exec_fence __user *user_fences;
> > + struct i915_eb_fences *fences;
> > + u64 __user *user_values;
> > + u64 num_fences, num_user_fences = timeline_fences->fence_count;
> > + unsigned long n;
> > + int err = 0;
> > +
> > + /* Check multiplication overflow for access_ok() and kvmalloc_array() */
> > + BUILD_BUG_ON(sizeof(size_t) > sizeof(unsigned long));
> > + if (num_user_fences > min_t(unsigned long,
> > + ULONG_MAX / sizeof(*user_fences),
> > + SIZE_MAX / sizeof(*fences)))
> > + return ERR_PTR(-EINVAL);
> > +
> > + user_fences = u64_to_user_ptr(timeline_fences->handles_ptr);
> > + if (!access_ok(user_fences, num_user_fences * sizeof(*user_fences)))
> > + return ERR_PTR(-EFAULT);
> > +
> > + user_values = u64_to_user_ptr(timeline_fences->values_ptr);
> > + if (!access_ok(user_values, num_user_fenc
Re: [Intel-gfx] [PATCH 2/3] drm/i915: add syncobj timeline support
On 06/04/2020 23:07, Venkata Sandeep Dhanalakota wrote:
Introduces a new parameters to execbuf so that we can specify syncobj
handles as well as timeline points.
v2: Reuse i915_user_extension_fn
v3: Check that the chained extension is only present once (Chris)
v4: Check that dma_fence_chain_find_seqno returns a non NULL fence
(Lionel)
v5: Use BIT_ULL (Chris)
v6: Fix issue with already signaled timeline points,
dma_fence_chain_find_seqno() setting fence to NULL (Chris)
v7: Report ENOENT with invalid syncobj handle (Lionel)
v8: Check for out of order timeline point insertion (Chris)
v9: After explanations on
https://lists.freedesktop.org/archives/dri-devel/2019-August/229287.html
drop the ordering check from v8 (Lionel)
v10: Set first extension enum item to 1 (Jason)
v11: Add wait on previous sync points in timelines (Sandeep)
Thanks for picking this series up!
Could you point to the changes in v11?
I haven't look at it in a while and I can't remember what you would have
changed.
Thanks a lot,
-Lionel
Signed-off-by: Lionel Landwerlin
Signed-off-by: Venkata Sandeep Dhanalakota
---
.../gpu/drm/i915/gem/i915_gem_execbuffer.c| 312 ++
drivers/gpu/drm/i915/i915_drv.c | 3 +-
drivers/gpu/drm/i915/i915_getparam.c | 1 +
include/uapi/drm/i915_drm.h | 38 +++
4 files changed, 296 insertions(+), 58 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 16831f715daa..4cb4cd035daa 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -230,6 +230,13 @@ enum {
* the batchbuffer in trusted mode, otherwise the ioctl is rejected.
*/
+struct i915_eb_fences {
+ struct drm_syncobj *syncobj; /* Use with ptr_mask_bits() */
+ struct dma_fence *dma_fence;
+ u64 value;
+ struct dma_fence_chain *chain_fence;
+};
+
struct i915_execbuffer {
struct drm_i915_private *i915; /** i915 backpointer */
struct drm_file *file; /** per-file lookup tables and limits */
@@ -292,6 +299,7 @@ struct i915_execbuffer {
struct {
u64 flags; /** Available extensions parameters */
+ struct drm_i915_gem_execbuffer_ext_timeline_fences
timeline_fences;
} extensions;
};
@@ -2244,67 +2252,219 @@ eb_pin_engine(struct i915_execbuffer *eb,
}
static void
-__free_fence_array(struct drm_syncobj **fences, unsigned int n)
+__free_fence_array(struct i915_eb_fences *fences, unsigned int n)
{
- while (n--)
- drm_syncobj_put(ptr_mask_bits(fences[n], 2));
+ while (n--) {
+ drm_syncobj_put(ptr_mask_bits(fences[n].syncobj, 2));
+ dma_fence_put(fences[n].dma_fence);
+ kfree(fences[n].chain_fence);
+ }
kvfree(fences);
}
-static struct drm_syncobj **
-get_fence_array(struct drm_i915_gem_execbuffer2 *args,
- struct drm_file *file)
+static struct i915_eb_fences *
+get_timeline_fence_array(struct i915_execbuffer *eb, int *out_n_fences)
+{
+ struct drm_i915_gem_execbuffer_ext_timeline_fences *timeline_fences =
+ &eb->extensions.timeline_fences;
+ struct drm_i915_gem_exec_fence __user *user_fences;
+ struct i915_eb_fences *fences;
+ u64 __user *user_values;
+ u64 num_fences, num_user_fences = timeline_fences->fence_count;
+ unsigned long n;
+ int err = 0;
+
+ /* Check multiplication overflow for access_ok() and kvmalloc_array() */
+ BUILD_BUG_ON(sizeof(size_t) > sizeof(unsigned long));
+ if (num_user_fences > min_t(unsigned long,
+ ULONG_MAX / sizeof(*user_fences),
+ SIZE_MAX / sizeof(*fences)))
+ return ERR_PTR(-EINVAL);
+
+ user_fences = u64_to_user_ptr(timeline_fences->handles_ptr);
+ if (!access_ok(user_fences, num_user_fences * sizeof(*user_fences)))
+ return ERR_PTR(-EFAULT);
+
+ user_values = u64_to_user_ptr(timeline_fences->values_ptr);
+ if (!access_ok(user_values, num_user_fences * sizeof(*user_values)))
+ return ERR_PTR(-EFAULT);
+
+ fences = kvmalloc_array(num_user_fences, sizeof(*fences),
+ __GFP_NOWARN | GFP_KERNEL);
+ if (!fences)
+ return ERR_PTR(-ENOMEM);
+
+ BUILD_BUG_ON(~(ARCH_KMALLOC_MINALIGN - 1) &
+~__I915_EXEC_FENCE_UNKNOWN_FLAGS);
+
+ for (n = 0, num_fences = 0; n < timeline_fences->fence_count; n++) {
+ struct drm_i915_gem_exec_fence user_fence;
+ struct drm_syncobj *syncobj;
+ struct dma_fence *fence = NULL;
+ u64 point;
+
+ if (__copy_from_user(&user_fence, user_fences++,
sizeof(user_fence))) {
+ err = -EFAULT;
+
[Intel-gfx] [PATCH 2/3] drm/i915: add syncobj timeline support
Introduces a new parameters to execbuf so that we can specify syncobj
handles as well as timeline points.
v2: Reuse i915_user_extension_fn
v3: Check that the chained extension is only present once (Chris)
v4: Check that dma_fence_chain_find_seqno returns a non NULL fence
(Lionel)
v5: Use BIT_ULL (Chris)
v6: Fix issue with already signaled timeline points,
dma_fence_chain_find_seqno() setting fence to NULL (Chris)
v7: Report ENOENT with invalid syncobj handle (Lionel)
v8: Check for out of order timeline point insertion (Chris)
v9: After explanations on
https://lists.freedesktop.org/archives/dri-devel/2019-August/229287.html
drop the ordering check from v8 (Lionel)
v10: Set first extension enum item to 1 (Jason)
v11: Add wait on previous sync points in timelines (Sandeep)
Signed-off-by: Lionel Landwerlin
Signed-off-by: Venkata Sandeep Dhanalakota
---
.../gpu/drm/i915/gem/i915_gem_execbuffer.c| 312 ++
drivers/gpu/drm/i915/i915_drv.c | 3 +-
drivers/gpu/drm/i915/i915_getparam.c | 1 +
include/uapi/drm/i915_drm.h | 38 +++
4 files changed, 296 insertions(+), 58 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 16831f715daa..4cb4cd035daa 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -230,6 +230,13 @@ enum {
* the batchbuffer in trusted mode, otherwise the ioctl is rejected.
*/
+struct i915_eb_fences {
+ struct drm_syncobj *syncobj; /* Use with ptr_mask_bits() */
+ struct dma_fence *dma_fence;
+ u64 value;
+ struct dma_fence_chain *chain_fence;
+};
+
struct i915_execbuffer {
struct drm_i915_private *i915; /** i915 backpointer */
struct drm_file *file; /** per-file lookup tables and limits */
@@ -292,6 +299,7 @@ struct i915_execbuffer {
struct {
u64 flags; /** Available extensions parameters */
+ struct drm_i915_gem_execbuffer_ext_timeline_fences
timeline_fences;
} extensions;
};
@@ -2244,67 +2252,219 @@ eb_pin_engine(struct i915_execbuffer *eb,
}
static void
-__free_fence_array(struct drm_syncobj **fences, unsigned int n)
+__free_fence_array(struct i915_eb_fences *fences, unsigned int n)
{
- while (n--)
- drm_syncobj_put(ptr_mask_bits(fences[n], 2));
+ while (n--) {
+ drm_syncobj_put(ptr_mask_bits(fences[n].syncobj, 2));
+ dma_fence_put(fences[n].dma_fence);
+ kfree(fences[n].chain_fence);
+ }
kvfree(fences);
}
-static struct drm_syncobj **
-get_fence_array(struct drm_i915_gem_execbuffer2 *args,
- struct drm_file *file)
+static struct i915_eb_fences *
+get_timeline_fence_array(struct i915_execbuffer *eb, int *out_n_fences)
+{
+ struct drm_i915_gem_execbuffer_ext_timeline_fences *timeline_fences =
+ &eb->extensions.timeline_fences;
+ struct drm_i915_gem_exec_fence __user *user_fences;
+ struct i915_eb_fences *fences;
+ u64 __user *user_values;
+ u64 num_fences, num_user_fences = timeline_fences->fence_count;
+ unsigned long n;
+ int err = 0;
+
+ /* Check multiplication overflow for access_ok() and kvmalloc_array() */
+ BUILD_BUG_ON(sizeof(size_t) > sizeof(unsigned long));
+ if (num_user_fences > min_t(unsigned long,
+ ULONG_MAX / sizeof(*user_fences),
+ SIZE_MAX / sizeof(*fences)))
+ return ERR_PTR(-EINVAL);
+
+ user_fences = u64_to_user_ptr(timeline_fences->handles_ptr);
+ if (!access_ok(user_fences, num_user_fences * sizeof(*user_fences)))
+ return ERR_PTR(-EFAULT);
+
+ user_values = u64_to_user_ptr(timeline_fences->values_ptr);
+ if (!access_ok(user_values, num_user_fences * sizeof(*user_values)))
+ return ERR_PTR(-EFAULT);
+
+ fences = kvmalloc_array(num_user_fences, sizeof(*fences),
+ __GFP_NOWARN | GFP_KERNEL);
+ if (!fences)
+ return ERR_PTR(-ENOMEM);
+
+ BUILD_BUG_ON(~(ARCH_KMALLOC_MINALIGN - 1) &
+~__I915_EXEC_FENCE_UNKNOWN_FLAGS);
+
+ for (n = 0, num_fences = 0; n < timeline_fences->fence_count; n++) {
+ struct drm_i915_gem_exec_fence user_fence;
+ struct drm_syncobj *syncobj;
+ struct dma_fence *fence = NULL;
+ u64 point;
+
+ if (__copy_from_user(&user_fence, user_fences++,
sizeof(user_fence))) {
+ err = -EFAULT;
+ goto err;
+ }
+
+ if (user_fence.flags & __I915_EXEC_FENCE_UNKNOWN_FLAGS) {
+ err = -EINVAL;
+ goto err;
+ }
+
+ if (__get_user(point, user_values++)) {
