Re: [Intel-gfx] [PATCH v6 07/11] drm/i915: add syncobj timeline support
On 15/07/2019 14:30, Koenig, Christian wrote:
Hi Lionel,
sorry for the delayed response, I'm just back from vacation.
Am 03.07.19 um 11:17 schrieb Lionel Landwerlin:
On 03/07/2019 11:56, Chris Wilson wrote:
Quoting Lionel Landwerlin (2019-07-01 12:34:33)
+ syncobj = drm_syncobj_find(eb->file,
user_fence.handle);
+ if (!syncobj) {
+ DRM_DEBUG("Invalid syncobj handle provided\n");
+ err = -EINVAL;
+ goto err;
+ }
+
+ if (user_fence.flags & I915_EXEC_FENCE_WAIT) {
+ fence = drm_syncobj_fence_get(syncobj);
+ if (!fence) {
+ DRM_DEBUG("Syncobj handle has no
fence\n");
+ drm_syncobj_put(syncobj);
+ err = -EINVAL;
+ goto err;
+ }
+
+ err = dma_fence_chain_find_seqno(&fence,
point);
I'm very dubious about chain_find_seqno().
It returns -EINVAL if the point is older than the first in the chain --
it is in an unknown state, but may be signaled since we remove signaled
links from the chain. If we are waiting for an already signaled syncpt,
we should not be erring out!
You're right, I got this wrong.
We can get fence = NULL if the point is already signaled.
The easiest would be to skip it from the list, or add the stub fence.
I guess the CTS got lucky that it always got the point needed before
it was garbage collected...
The topmost point is never garbage collected. So IIRC the check is
actually correct and you should never get NULL here.
Do we allow later requests to insert earlier syncpt into the chain? If
so, then the request we wait on here may be woefully inaccurate and
quite easily lead to cycles in the fence tree. We have no way of
resolving such deadlocks -- we would have to treat this fence as a
foreign fence and install a backup timer. Alternatively, we only allow
this to return the exact fence for a syncpt, and proxies for the rest.
Adding points < latest added point is forbidden.
I wish we enforced it a bit more than what's currently done in
drm_syncobj_add_point().
In my view we should :
- lock the syncobj in get_timeline_fence_array() do the sanity
check there.
- keep the lock until we add the point to the timeline
- unlock once added
That way we would ensure that the application cannot generate invalid
timelines and error out if it does.
We could do the same for host signaling in
drm_syncobj_timeline_signal_ioctl/drm_syncobj_transfer_to_timeline
(there the locking a lot shorter).
That requires holding the lock for longer than maybe other driver
would prefer.
Ccing Christian who can tell whether that's out of question for AMD.
Yeah, adding the lock was the only other option I could see as well, but
we intentionally decided against that.
Since we have multiple out sync objects we would need to use a ww_mutex
as lock here.
That in turn would result in a another rather complicated dance for
deadlock avoidance. Something which each driver would have to implement
correctly.
That doesn't sounds like a good idea to me just to improve error checking.
As long as it is only in the same process userspace could check that as
well before doing the submission.
Thanks Christian,
Would you be opposed to exposing an _locked() version of
drm_syncobj_add_point() and have a static inline do the locking?
I don't think it would be a difference for your driver and we could add
checking with a proxy fence Chris suggested on our side.
We could also allow do checks in drm_syncobj_timeline_signal_ioctl().
-Lionel
Regards,
Christian.
Cheers,
-Lionel
+ if (err || !fence) {
+ DRM_DEBUG("Syncobj handle missing
requested point\n");
+ drm_syncobj_put(syncobj);
+ err = err != 0 ? err : -EINVAL;
+ goto err;
+ }
+ }
+
+ /*
+ * For timeline syncobjs we need to preallocate
chains for
+ * later signaling.
+ */
+ if (point != 0 && user_fence.flags &
I915_EXEC_FENCE_SIGNAL) {
+ fences[n].chain_fence =
+ kmalloc(sizeof(*fences[n].chain_fence),
+ GFP_KERNEL);
+ if (!fences[n].chain_fence) {
+ dma_fence_put(fence);
+ drm_syncobj_put(syncobj);
+ err = -ENOMEM;
+ DRM_DEBUG("Unable to alloc
chain_fence\n");
+ goto err;
+ }
What happens if we later try to insert two fences for the same syncpt?
Should we not reserve the slot in the chain to reject duplica
Re: [Intel-gfx] [PATCH v6 07/11] drm/i915: add syncobj timeline support
Hi Lionel,
sorry for the delayed response, I'm just back from vacation.
Am 03.07.19 um 11:17 schrieb Lionel Landwerlin:
> On 03/07/2019 11:56, Chris Wilson wrote:
>> Quoting Lionel Landwerlin (2019-07-01 12:34:33)
>>> + syncobj = drm_syncobj_find(eb->file,
>>> user_fence.handle);
>>> + if (!syncobj) {
>>> + DRM_DEBUG("Invalid syncobj handle provided\n");
>>> + err = -EINVAL;
>>> + goto err;
>>> + }
>>> +
>>> + if (user_fence.flags & I915_EXEC_FENCE_WAIT) {
>>> + fence = drm_syncobj_fence_get(syncobj);
>>> + if (!fence) {
>>> + DRM_DEBUG("Syncobj handle has no
>>> fence\n");
>>> + drm_syncobj_put(syncobj);
>>> + err = -EINVAL;
>>> + goto err;
>>> + }
>>> +
>>> + err = dma_fence_chain_find_seqno(&fence,
>>> point);
>> I'm very dubious about chain_find_seqno().
>>
>> It returns -EINVAL if the point is older than the first in the chain --
>> it is in an unknown state, but may be signaled since we remove signaled
>> links from the chain. If we are waiting for an already signaled syncpt,
>> we should not be erring out!
>
>
> You're right, I got this wrong.
>
> We can get fence = NULL if the point is already signaled.
>
> The easiest would be to skip it from the list, or add the stub fence.
>
>
> I guess the CTS got lucky that it always got the point needed before
> it was garbage collected...
The topmost point is never garbage collected. So IIRC the check is
actually correct and you should never get NULL here.
>>
>> Do we allow later requests to insert earlier syncpt into the chain? If
>> so, then the request we wait on here may be woefully inaccurate and
>> quite easily lead to cycles in the fence tree. We have no way of
>> resolving such deadlocks -- we would have to treat this fence as a
>> foreign fence and install a backup timer. Alternatively, we only allow
>> this to return the exact fence for a syncpt, and proxies for the rest.
>
>
> Adding points < latest added point is forbidden.
>
> I wish we enforced it a bit more than what's currently done in
> drm_syncobj_add_point().
>
> In my view we should :
>
> - lock the syncobj in get_timeline_fence_array() do the sanity
> check there.
>
> - keep the lock until we add the point to the timeline
>
> - unlock once added
>
>
> That way we would ensure that the application cannot generate invalid
> timelines and error out if it does.
>
> We could do the same for host signaling in
> drm_syncobj_timeline_signal_ioctl/drm_syncobj_transfer_to_timeline
> (there the locking a lot shorter).
>
> That requires holding the lock for longer than maybe other driver
> would prefer.
>
>
> Ccing Christian who can tell whether that's out of question for AMD.
Yeah, adding the lock was the only other option I could see as well, but
we intentionally decided against that.
Since we have multiple out sync objects we would need to use a ww_mutex
as lock here.
That in turn would result in a another rather complicated dance for
deadlock avoidance. Something which each driver would have to implement
correctly.
That doesn't sounds like a good idea to me just to improve error checking.
As long as it is only in the same process userspace could check that as
well before doing the submission.
Regards,
Christian.
>
>
> Cheers,
>
>
> -Lionel
>
>
>>> + if (err || !fence) {
>>> + DRM_DEBUG("Syncobj handle missing
>>> requested point\n");
>>> + drm_syncobj_put(syncobj);
>>> + err = err != 0 ? err : -EINVAL;
>>> + goto err;
>>> + }
>>> + }
>>> +
>>> + /*
>>> + * For timeline syncobjs we need to preallocate
>>> chains for
>>> + * later signaling.
>>> + */
>>> + if (point != 0 && user_fence.flags &
>>> I915_EXEC_FENCE_SIGNAL) {
>>> + fences[n].chain_fence =
>>> + kmalloc(sizeof(*fences[n].chain_fence),
>>> + GFP_KERNEL);
>>> + if (!fences[n].chain_fence) {
>>> + dma_fence_put(fence);
>>> + drm_syncobj_put(syncobj);
>>> + err = -ENOMEM;
>>> + DRM_DEBUG("Unable to alloc
>>> chain_fence\n");
>>> + goto err;
>>> + }
>> What happens if we later try to insert two fences for the same syncpt?
>> Should we not reserve the slot in the chain to reject duplicates?
>> -Chris
>>
>
___
Intel-gfx mailing list
In
Re: [Intel-gfx] [PATCH v6 07/11] drm/i915: add syncobj timeline support
On 03/07/2019 11:56, Chris Wilson wrote:
Quoting Lionel Landwerlin (2019-07-01 12:34:33)
+ syncobj = drm_syncobj_find(eb->file, user_fence.handle);
+ if (!syncobj) {
+ DRM_DEBUG("Invalid syncobj handle provided\n");
+ err = -EINVAL;
+ goto err;
+ }
+
+ if (user_fence.flags & I915_EXEC_FENCE_WAIT) {
+ fence = drm_syncobj_fence_get(syncobj);
+ if (!fence) {
+ DRM_DEBUG("Syncobj handle has no fence\n");
+ drm_syncobj_put(syncobj);
+ err = -EINVAL;
+ goto err;
+ }
+
+ err = dma_fence_chain_find_seqno(&fence, point);
I'm very dubious about chain_find_seqno().
It returns -EINVAL if the point is older than the first in the chain --
it is in an unknown state, but may be signaled since we remove signaled
links from the chain. If we are waiting for an already signaled syncpt,
we should not be erring out!
You're right, I got this wrong.
We can get fence = NULL if the point is already signaled.
The easiest would be to skip it from the list, or add the stub fence.
I guess the CTS got lucky that it always got the point needed before it
was garbage collected...
Do we allow later requests to insert earlier syncpt into the chain? If
so, then the request we wait on here may be woefully inaccurate and
quite easily lead to cycles in the fence tree. We have no way of
resolving such deadlocks -- we would have to treat this fence as a
foreign fence and install a backup timer. Alternatively, we only allow
this to return the exact fence for a syncpt, and proxies for the rest.
Adding points < latest added point is forbidden.
I wish we enforced it a bit more than what's currently done in
drm_syncobj_add_point().
In my view we should :
- lock the syncobj in get_timeline_fence_array() do the sanity
check there.
- keep the lock until we add the point to the timeline
- unlock once added
That way we would ensure that the application cannot generate invalid
timelines and error out if it does.
We could do the same for host signaling in
drm_syncobj_timeline_signal_ioctl/drm_syncobj_transfer_to_timeline
(there the locking a lot shorter).
That requires holding the lock for longer than maybe other driver would
prefer.
Ccing Christian who can tell whether that's out of question for AMD.
Cheers,
-Lionel
+ if (err || !fence) {
+ DRM_DEBUG("Syncobj handle missing requested
point\n");
+ drm_syncobj_put(syncobj);
+ err = err != 0 ? err : -EINVAL;
+ goto err;
+ }
+ }
+
+ /*
+* For timeline syncobjs we need to preallocate chains for
+* later signaling.
+*/
+ if (point != 0 && user_fence.flags & I915_EXEC_FENCE_SIGNAL) {
+ fences[n].chain_fence =
+ kmalloc(sizeof(*fences[n].chain_fence),
+ GFP_KERNEL);
+ if (!fences[n].chain_fence) {
+ dma_fence_put(fence);
+ drm_syncobj_put(syncobj);
+ err = -ENOMEM;
+ DRM_DEBUG("Unable to alloc chain_fence\n");
+ goto err;
+ }
What happens if we later try to insert two fences for the same syncpt?
Should we not reserve the slot in the chain to reject duplicates?
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v6 07/11] drm/i915: add syncobj timeline support
Quoting Lionel Landwerlin (2019-07-01 12:34:33)
> + syncobj = drm_syncobj_find(eb->file, user_fence.handle);
> + if (!syncobj) {
> + DRM_DEBUG("Invalid syncobj handle provided\n");
> + err = -EINVAL;
> + goto err;
> + }
> +
> + if (user_fence.flags & I915_EXEC_FENCE_WAIT) {
> + fence = drm_syncobj_fence_get(syncobj);
> + if (!fence) {
> + DRM_DEBUG("Syncobj handle has no fence\n");
> + drm_syncobj_put(syncobj);
> + err = -EINVAL;
> + goto err;
> + }
> +
> + err = dma_fence_chain_find_seqno(&fence, point);
I'm very dubious about chain_find_seqno().
It returns -EINVAL if the point is older than the first in the chain --
it is in an unknown state, but may be signaled since we remove signaled
links from the chain. If we are waiting for an already signaled syncpt,
we should not be erring out!
Do we allow later requests to insert earlier syncpt into the chain? If
so, then the request we wait on here may be woefully inaccurate and
quite easily lead to cycles in the fence tree. We have no way of
resolving such deadlocks -- we would have to treat this fence as a
foreign fence and install a backup timer. Alternatively, we only allow
this to return the exact fence for a syncpt, and proxies for the rest.
> + if (err || !fence) {
> + DRM_DEBUG("Syncobj handle missing requested
> point\n");
> + drm_syncobj_put(syncobj);
> + err = err != 0 ? err : -EINVAL;
> + goto err;
> + }
> + }
> +
> + /*
> +* For timeline syncobjs we need to preallocate chains for
> +* later signaling.
> +*/
> + if (point != 0 && user_fence.flags & I915_EXEC_FENCE_SIGNAL) {
> + fences[n].chain_fence =
> + kmalloc(sizeof(*fences[n].chain_fence),
> + GFP_KERNEL);
> + if (!fences[n].chain_fence) {
> + dma_fence_put(fence);
> + drm_syncobj_put(syncobj);
> + err = -ENOMEM;
> + DRM_DEBUG("Unable to alloc chain_fence\n");
> + goto err;
> + }
What happens if we later try to insert two fences for the same syncpt?
Should we not reserve the slot in the chain to reject duplicates?
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v6 07/11] drm/i915: add syncobj timeline support
On 01/07/2019 16:18, Chris Wilson wrote:
Quoting Lionel Landwerlin (2019-07-01 12:34:33)
+ /*
+* For timeline syncobjs we need to preallocate chains for
+* later signaling.
+*/
+ if (point != 0 && user_fence.flags & I915_EXEC_FENCE_SIGNAL) {
+ fences[n].chain_fence =
+ kmalloc(sizeof(*fences[n].chain_fence),
+ GFP_KERNEL);
+ if (!fences[n].chain_fence) {
+ dma_fence_put(fence);
+ drm_syncobj_put(syncobj);
+ err = -ENOMEM;
+ DRM_DEBUG("Unable to alloc chain_fence\n");
This is like throwing a grenade, waiting for the explosion, and then
saying "bang" under your breath. :)
-Chris
I don't get your point :)
-Lionel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v6 07/11] drm/i915: add syncobj timeline support
Quoting Lionel Landwerlin (2019-07-01 12:34:33)
> + /*
> +* For timeline syncobjs we need to preallocate chains for
> +* later signaling.
> +*/
> + if (point != 0 && user_fence.flags & I915_EXEC_FENCE_SIGNAL) {
> + fences[n].chain_fence =
> + kmalloc(sizeof(*fences[n].chain_fence),
> + GFP_KERNEL);
> + if (!fences[n].chain_fence) {
> + dma_fence_put(fence);
> + drm_syncobj_put(syncobj);
> + err = -ENOMEM;
> + DRM_DEBUG("Unable to alloc chain_fence\n");
This is like throwing a grenade, waiting for the explosion, and then
saying "bang" under your breath. :)
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v6 07/11] drm/i915: add syncobj timeline support
On 01/07/2019 16:13, Chris Wilson wrote:
Quoting Lionel Landwerlin (2019-07-01 12:34:33)
struct i915_execbuffer {
struct drm_i915_private *i915; /** i915 backpointer */
struct drm_file *file; /** per-file lookup tables and limits */
@@ -275,6 +282,7 @@ struct i915_execbuffer {
struct {
u64 flags; /** Available extensions parameters */
+ struct drm_i915_gem_execbuffer_ext_timeline_fences
timeline_fences;
} extensions;
};
+static int parse_timeline_fences(struct i915_user_extension __user *ext, void
*data)
+{
+ struct i915_execbuffer *eb = data;
+
+ /* Timeline fences are incompatible with the fence array flag. */
+ if (eb->args->flags & I915_EXEC_FENCE_ARRAY)
+ return -EINVAL;
+
+ if (eb->extensions.flags &
BIT(DRM_I915_GEM_EXECBUFFER_EXT_TIMELINE_FENCES))
+ return -EINVAL;
flags is 64b, so wiser if we use BIT_ULL() from the start. You don't
want to copy my bugs ;)
-Chris
Dammit! Why aren't all bit macros 64bits? :)
-Lionel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v6 07/11] drm/i915: add syncobj timeline support
Quoting Lionel Landwerlin (2019-07-01 12:34:33)
> struct i915_execbuffer {
> struct drm_i915_private *i915; /** i915 backpointer */
> struct drm_file *file; /** per-file lookup tables and limits */
> @@ -275,6 +282,7 @@ struct i915_execbuffer {
>
> struct {
> u64 flags; /** Available extensions parameters */
> + struct drm_i915_gem_execbuffer_ext_timeline_fences
> timeline_fences;
> } extensions;
> };
> +static int parse_timeline_fences(struct i915_user_extension __user *ext,
> void *data)
> +{
> + struct i915_execbuffer *eb = data;
> +
> + /* Timeline fences are incompatible with the fence array flag. */
> + if (eb->args->flags & I915_EXEC_FENCE_ARRAY)
> + return -EINVAL;
> +
> + if (eb->extensions.flags &
> BIT(DRM_I915_GEM_EXECBUFFER_EXT_TIMELINE_FENCES))
> + return -EINVAL;
flags is 64b, so wiser if we use BIT_ULL() from the start. You don't
want to copy my bugs ;)
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v6 07/11] 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)
Signed-off-by: Lionel Landwerlin
---
.../gpu/drm/i915/gem/i915_gem_execbuffer.c| 288 ++
drivers/gpu/drm/i915/i915_drv.c | 4 +-
include/uapi/drm/i915_drm.h | 38 +++
3 files changed, 273 insertions(+), 57 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 9887fa9e3ac8..d3004fc1f995 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -213,6 +213,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 */
@@ -275,6 +282,7 @@ struct i915_execbuffer {
struct {
u64 flags; /** Available extensions parameters */
+ struct drm_i915_gem_execbuffer_ext_timeline_fences
timeline_fences;
} extensions;
};
@@ -2224,67 +2232,198 @@ eb_select_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;
+ const u64 num_fences = timeline_fences->fence_count;
+ unsigned long n;
+ int err;
+
+ *out_n_fences = num_fences;
+
+ /* Check multiplication overflow for access_ok() and kvmalloc_array() */
+ BUILD_BUG_ON(sizeof(size_t) > sizeof(unsigned long));
+ if (num_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_fences * sizeof(*user_fences)))
+ return ERR_PTR(-EFAULT);
+
+ user_values = u64_to_user_ptr(timeline_fences->values_ptr);
+ if (!access_ok(user_values, num_fences * sizeof(*user_values)))
+ return ERR_PTR(-EFAULT);
+
+ fences = kvmalloc_array(num_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; n < num_fences; 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++)) {
+ err = -EFAULT;
+ goto err;
+ }
+
+ syncobj = drm_syncobj_find(eb->file, user_fence.handle);
+ if (!syncobj) {
+ DRM_DEBUG("Invalid syncobj handle provided\n");
+ err = -EINVAL;
+ goto err;
+ }
+
+ if (user_fence.flags & I915_EXEC_FENCE_WAIT) {
+ fence = drm_syncobj_fence_get(syncobj);
+ if (!fence) {
+ DRM_DEBUG("Syncobj handle has no fence\n");
+ drm_syncobj_put(syn
