| Message ID | 20190731140733.23373-3-lionel.g.landwerlin@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: timeline semaphore support | expand |
Quoting Lionel Landwerlin (2019-07-31 15:07:33) > -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; > + > + /* 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++)) { > + err = -EFAULT; > + goto err; > + } > + > + syncobj = drm_syncobj_find(eb->file, user_fence.handle); > + if (!syncobj) { > + DRM_DEBUG("Invalid syncobj handle provided\n"); > + err = -ENOENT; > + 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); > + if (err) { > + DRM_DEBUG("Syncobj handle missing requested point %llu\n", point); > + drm_syncobj_put(syncobj); > + goto err; > + } > + > + /* A point might have been signaled already and > + * garbage collected from the timeline. In this case > + * just ignore the point and carry on. > + */ > + if (!fence) { > + drm_syncobj_put(syncobj); > + continue; > + } > + } > + > + /* > + * For timeline syncobjs we need to preallocate chains for > + * later signaling. > + */ > + if (point != 0 && user_fence.flags & I915_EXEC_FENCE_SIGNAL) { if (dma_fence_chain_find_seqno() == 0) return -EINVAL; as an early sanity check? > + fences[num_fences].chain_fence = > + kmalloc(sizeof(*fences[num_fences].chain_fence), > + GFP_KERNEL); > + if (!fences[num_fences].chain_fence) { > + dma_fence_put(fence); > + drm_syncobj_put(syncobj); > + err = -ENOMEM; > + DRM_DEBUG("Unable to alloc chain_fence\n"); > + goto err; > + } > + } else { > + fences[num_fences].chain_fence = NULL; > + } > + > + fences[num_fences].syncobj = ptr_pack_bits(syncobj, user_fence.flags, 2); > + fences[num_fences].dma_fence = fence; > + fences[num_fences].value = point; > + num_fences++; > + } > + > + *out_n_fences = num_fences; > + > + return fences; > + > +err: > + __free_fence_array(fences, num_fences); > + return ERR_PTR(err); > +} > @@ -2329,9 +2484,9 @@ await_fence_array(struct i915_execbuffer *eb, > > static void > signal_fence_array(struct i915_execbuffer *eb, > - struct drm_syncobj **fences) > + struct i915_eb_fences *fences, > + int nfences) > { > - const unsigned int nfences = eb->args->num_cliprects; > struct dma_fence * const fence = &eb->request->fence; > unsigned int n; > > @@ -2339,15 +2494,46 @@ signal_fence_array(struct i915_execbuffer *eb, > struct drm_syncobj *syncobj; > unsigned int flags; > > - syncobj = ptr_unpack_bits(fences[n], &flags, 2); > + syncobj = ptr_unpack_bits(fences[n].syncobj, &flags, 2); > if (!(flags & I915_EXEC_FENCE_SIGNAL)) > continue; > > - drm_syncobj_replace_fence(syncobj, fence); > + if (fences[n].chain_fence) { > + drm_syncobj_add_point(syncobj, fences[n].chain_fence, > + fence, fences[n].value); > + /* > + * The chain's ownership is transfered to the > + * timeline. > + */ > + fences[n].chain_fence = NULL; > + } else { > + drm_syncobj_replace_fence(syncobj, fence); > + } > } > } I think I have convinced myself that with the split between wait before, signal after combined with the rule that seqno point along the syncobj are monotonic, you should not be able to generate an AB-BA deadlock between concurrent clients. Except that we need to fixup drm_syncobj_add_point() to actually apply that rule. Throwing an error and leaving the client stuck is less than ideal, we can't even recover with a GPU reset, and as they are native fences we don't employ a failsafe timer. Doesn't WAIT_FOR_SUBMIT throw a spanner in the works and allow for deadlocks? On the other hand, if they used MI_SEMA rather scheduler fences, give or take some coarseness in our timeslicing granularity (I need to enable fast context switches on user semaphores -- first attempt was just an interrupt storm!) it will work as well as a round-robin scheduler can work. (In other words, we only need coarse fences for the scheduler and userspace can implement its own semaphores -- with the open discussion on how to share the iova user semaphores between processes, cf ugpu.) -Chris
On 31/07/2019 23:03, Chris Wilson wrote: > Quoting Lionel Landwerlin (2019-07-31 15:07:33) >> -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; >> + >> + /* 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++)) { >> + err = -EFAULT; >> + goto err; >> + } >> + >> + syncobj = drm_syncobj_find(eb->file, user_fence.handle); >> + if (!syncobj) { >> + DRM_DEBUG("Invalid syncobj handle provided\n"); >> + err = -ENOENT; >> + 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); >> + if (err) { >> + DRM_DEBUG("Syncobj handle missing requested point %llu\n", point); >> + drm_syncobj_put(syncobj); >> + goto err; >> + } >> + >> + /* A point might have been signaled already and >> + * garbage collected from the timeline. In this case >> + * just ignore the point and carry on. >> + */ >> + if (!fence) { >> + drm_syncobj_put(syncobj); >> + continue; >> + } >> + } >> + >> + /* >> + * For timeline syncobjs we need to preallocate chains for >> + * later signaling. >> + */ >> + if (point != 0 && user_fence.flags & I915_EXEC_FENCE_SIGNAL) { > if (dma_fence_chain_find_seqno() == 0) > return -EINVAL; > > as an early sanity check? Makes sense, let me see if that breaks anything. Also waiting and signaling the same point doesn't work anymore for timelines. Will check that too. > >> + fences[num_fences].chain_fence = >> + kmalloc(sizeof(*fences[num_fences].chain_fence), >> + GFP_KERNEL); >> + if (!fences[num_fences].chain_fence) { >> + dma_fence_put(fence); >> + drm_syncobj_put(syncobj); >> + err = -ENOMEM; >> + DRM_DEBUG("Unable to alloc chain_fence\n"); >> + goto err; >> + } >> + } else { >> + fences[num_fences].chain_fence = NULL; >> + } >> + >> + fences[num_fences].syncobj = ptr_pack_bits(syncobj, user_fence.flags, 2); >> + fences[num_fences].dma_fence = fence; >> + fences[num_fences].value = point; >> + num_fences++; >> + } >> + >> + *out_n_fences = num_fences; >> + >> + return fences; >> + >> +err: >> + __free_fence_array(fences, num_fences); >> + return ERR_PTR(err); >> +} >> @@ -2329,9 +2484,9 @@ await_fence_array(struct i915_execbuffer *eb, >> >> static void >> signal_fence_array(struct i915_execbuffer *eb, >> - struct drm_syncobj **fences) >> + struct i915_eb_fences *fences, >> + int nfences) >> { >> - const unsigned int nfences = eb->args->num_cliprects; >> struct dma_fence * const fence = &eb->request->fence; >> unsigned int n; >> >> @@ -2339,15 +2494,46 @@ signal_fence_array(struct i915_execbuffer *eb, >> struct drm_syncobj *syncobj; >> unsigned int flags; >> >> - syncobj = ptr_unpack_bits(fences[n], &flags, 2); >> + syncobj = ptr_unpack_bits(fences[n].syncobj, &flags, 2); >> if (!(flags & I915_EXEC_FENCE_SIGNAL)) >> continue; >> >> - drm_syncobj_replace_fence(syncobj, fence); >> + if (fences[n].chain_fence) { >> + drm_syncobj_add_point(syncobj, fences[n].chain_fence, >> + fence, fences[n].value); >> + /* >> + * The chain's ownership is transfered to the >> + * timeline. >> + */ >> + fences[n].chain_fence = NULL; >> + } else { >> + drm_syncobj_replace_fence(syncobj, fence); >> + } >> } >> } > I think I have convinced myself that with the split between wait before, > signal after combined with the rule that seqno point along the syncobj > are monotonic, you should not be able to generate an AB-BA deadlock > between concurrent clients. > > Except that we need to fixup drm_syncobj_add_point() to actually apply > that rule. Throwing an error and leaving the client stuck is less than > ideal, we can't even recover with a GPU reset, and as they are native > fences we don't employ a failsafe timer. Unfortunately we're not the only user of add_point() and amdgpu doesn't want it to fail. Best I can come up with is take the syncobj lock when signaling in get_timeline_fence_array() do the check there, unlock in __free_fence_array. > > Doesn't WAIT_FOR_SUBMIT throw a spanner in the works and allow for > deadlocks? Hm... I can't see how. Unless you mean clients could deadlock them themselves the same way it would using 2 pthread_mutex and having 2 threads trying to lock both mutexes in opposite order. Is it that the kernel's problem? > On the other hand, if they used MI_SEMA rather scheduler > fences, give or take some coarseness in our timeslicing granularity (I > need to enable fast context switches on user semaphores -- first attempt > was just an interrupt storm!) it will work as well as a round-robin > scheduler can work. (In other words, we only need coarse fences for the > scheduler and userspace can implement its own semaphores -- with the > open discussion on how to share the iova user semaphores between > processes, cf ugpu.) > -Chris >
Quoting Lionel Landwerlin (2019-08-01 08:43:24) > On 31/07/2019 23:03, Chris Wilson wrote: > > Quoting Lionel Landwerlin (2019-07-31 15:07:33) > > I think I have convinced myself that with the split between wait before, > > signal after combined with the rule that seqno point along the syncobj > > are monotonic, you should not be able to generate an AB-BA deadlock > > between concurrent clients. > > > > Except that we need to fixup drm_syncobj_add_point() to actually apply > > that rule. Throwing an error and leaving the client stuck is less than > > ideal, we can't even recover with a GPU reset, and as they are native > > fences we don't employ a failsafe timer. > > > Unfortunately we're not the only user of add_point() and amdgpu doesn't > want it to fail. It's dangerously exposing the deadlock from incorrect seqno ordering to userspace. We should be able to hit that DRM_ERROR() in testing, and be able to detect the out-of-sequence timeline. > Best I can come up with is take the syncobj lock when signaling in > get_timeline_fence_array() do the check there, unlock in __free_fence_array. I think the intermediate step is a "safe" version of add_point that doesn't leave the timeline broken. That still leaves a glitch visible to userspace, but it should not deadlock. The way I was looking at it was to reserve a placeholder syncpt before building the request and allow replacing the placeholder afterwards. If we abort the submission part way, we leave the placeholder in the timeline to be replaced later, or subsumed by a later syncpt. > > Doesn't WAIT_FOR_SUBMIT throw a spanner in the works and allow for > > deadlocks? > > > Hm... I can't see how. > > Unless you mean clients could deadlock them themselves the same way it > would using 2 pthread_mutex and having 2 threads trying to lock both > mutexes in opposite order. Yes. > Is it that the kernel's problem? It becomes a problem for us as it ties up kernel resources that we can not reap currently. Userspace is not meant to be able to break the kernel on a whim. Even futexes are robust. ;) -Chris
On 01/08/2019 11:08, Chris Wilson wrote: > Quoting Lionel Landwerlin (2019-08-01 08:43:24) >> On 31/07/2019 23:03, Chris Wilson wrote: >>> Quoting Lionel Landwerlin (2019-07-31 15:07:33) >>> I think I have convinced myself that with the split between wait before, >>> signal after combined with the rule that seqno point along the syncobj >>> are monotonic, you should not be able to generate an AB-BA deadlock >>> between concurrent clients. >>> >>> Except that we need to fixup drm_syncobj_add_point() to actually apply >>> that rule. Throwing an error and leaving the client stuck is less than >>> ideal, we can't even recover with a GPU reset, and as they are native >>> fences we don't employ a failsafe timer. >> >> Unfortunately we're not the only user of add_point() and amdgpu doesn't >> want it to fail. > It's dangerously exposing the deadlock from incorrect seqno ordering to > userspace. We should be able to hit that DRM_ERROR() in testing, and be > able to detect the out-of-sequence timeline. > >> Best I can come up with is take the syncobj lock when signaling in >> get_timeline_fence_array() do the check there, unlock in __free_fence_array. > I think the intermediate step is a "safe" version of add_point that > doesn't leave the timeline broken. That still leaves a glitch visible to > userspace, but it should not deadlock. Right, sounds doable. What happens in execbuf after add_point() fails? We've already handed the request to the scheduler and potentially it might be running already. > > The way I was looking at it was to reserve a placeholder syncpt before > building the request and allow replacing the placeholder afterwards. That sounds fairly tricky to get right :( The fence stuff is already full of magic. > > If we abort the submission part way, we leave the placeholder in the > timeline to be replaced later, or subsumed by a later syncpt. > >>> Doesn't WAIT_FOR_SUBMIT throw a spanner in the works and allow for >>> deadlocks? >> >> Hm... I can't see how. >> >> Unless you mean clients could deadlock them themselves the same way it >> would using 2 pthread_mutex and having 2 threads trying to lock both >> mutexes in opposite order. > Yes. > >> Is it that the kernel's problem? > It becomes a problem for us as it ties up kernel resources that we can > not reap currently. Userspace is not meant to be able to break the > kernel on a whim. Even futexes are robust. ;) > -Chris > Thanks, -Lionel
Quoting Lionel Landwerlin (2019-08-01 10:01:44) > On 01/08/2019 11:08, Chris Wilson wrote: > > Quoting Lionel Landwerlin (2019-08-01 08:43:24) > >> On 31/07/2019 23:03, Chris Wilson wrote: > >>> Quoting Lionel Landwerlin (2019-07-31 15:07:33) > >>> I think I have convinced myself that with the split between wait before, > >>> signal after combined with the rule that seqno point along the syncobj > >>> are monotonic, you should not be able to generate an AB-BA deadlock > >>> between concurrent clients. > >>> > >>> Except that we need to fixup drm_syncobj_add_point() to actually apply > >>> that rule. Throwing an error and leaving the client stuck is less than > >>> ideal, we can't even recover with a GPU reset, and as they are native > >>> fences we don't employ a failsafe timer. > >> > >> Unfortunately we're not the only user of add_point() and amdgpu doesn't > >> want it to fail. > > It's dangerously exposing the deadlock from incorrect seqno ordering to > > userspace. We should be able to hit that DRM_ERROR() in testing, and be > > able to detect the out-of-sequence timeline. > > > >> Best I can come up with is take the syncobj lock when signaling in > >> get_timeline_fence_array() do the check there, unlock in __free_fence_array. > > I think the intermediate step is a "safe" version of add_point that > > doesn't leave the timeline broken. That still leaves a glitch visible to > > userspace, but it should not deadlock. > > > Right, sounds doable. What happens in execbuf after add_point() fails? > > We've already handed the request to the scheduler and potentially it > might be running already. Right, at present we've already submitted the request, so the batch will be run and fence will be signaled. The failure to add to it the timeline means that someone else already submitted a later fence and some third party is using that syncpt instead of ours for their dependency. So that third party will be delayed, but so long as they kept their dependencies in order it should just be a delay. The problem comes into play if we go ahead and insert our fence into the dma_fence_chain out of order, breaking all the monotonic guarantees and flipping the order of the fences. (The equivalent to taking mutexes out of order.) > > The way I was looking at it was to reserve a placeholder syncpt before > > building the request and allow replacing the placeholder afterwards. > > > That sounds fairly tricky to get right :( Invasive, yeah, turns out one needs to start walking the fence chain more carefully. The actual placeholder fence is pretty run of the mill as far as dma-fence*.c goes, which is say... > The fence stuff is already full of magic. It's full of magic. -Chris
On 01/08/2019 12:22, Chris Wilson wrote: > Quoting Lionel Landwerlin (2019-08-01 10:01:44) >> On 01/08/2019 11:08, Chris Wilson wrote: >>> Quoting Lionel Landwerlin (2019-08-01 08:43:24) >>>> On 31/07/2019 23:03, Chris Wilson wrote: >>>>> Quoting Lionel Landwerlin (2019-07-31 15:07:33) >>>>> I think I have convinced myself that with the split between wait before, >>>>> signal after combined with the rule that seqno point along the syncobj >>>>> are monotonic, you should not be able to generate an AB-BA deadlock >>>>> between concurrent clients. >>>>> >>>>> Except that we need to fixup drm_syncobj_add_point() to actually apply >>>>> that rule. Throwing an error and leaving the client stuck is less than >>>>> ideal, we can't even recover with a GPU reset, and as they are native >>>>> fences we don't employ a failsafe timer. >>>> Unfortunately we're not the only user of add_point() and amdgpu doesn't >>>> want it to fail. >>> It's dangerously exposing the deadlock from incorrect seqno ordering to >>> userspace. We should be able to hit that DRM_ERROR() in testing, and be >>> able to detect the out-of-sequence timeline. >>> >>>> Best I can come up with is take the syncobj lock when signaling in >>>> get_timeline_fence_array() do the check there, unlock in __free_fence_array. >>> I think the intermediate step is a "safe" version of add_point that >>> doesn't leave the timeline broken. That still leaves a glitch visible to >>> userspace, but it should not deadlock. >> >> Right, sounds doable. What happens in execbuf after add_point() fails? >> >> We've already handed the request to the scheduler and potentially it >> might be running already. > Right, at present we've already submitted the request, so the batch will > be run and fence will be signaled. The failure to add to it the > timeline means that someone else already submitted a later fence and > some third party is using that syncpt instead of ours for their > dependency. So that third party will be delayed, but so long as they kept > their dependencies in order it should just be a delay. But should we return an error to userspace? -Lionel > > The problem comes into play if we go ahead and insert our fence into the > dma_fence_chain out of order, breaking all the monotonic guarantees and > flipping the order of the fences. (The equivalent to taking mutexes out > of order.) > >>> The way I was looking at it was to reserve a placeholder syncpt before >>> building the request and allow replacing the placeholder afterwards. >> >> That sounds fairly tricky to get right :( > Invasive, yeah, turns out one needs to start walking the fence chain > more carefully. The actual placeholder fence is pretty run of the mill > as far as dma-fence*.c goes, which is say... > >> The fence stuff is already full of magic. > It's full of magic. > -Chris >
Quoting Lionel Landwerlin (2019-08-01 10:37:23) > On 01/08/2019 12:22, Chris Wilson wrote: > > Quoting Lionel Landwerlin (2019-08-01 10:01:44) > >> On 01/08/2019 11:08, Chris Wilson wrote: > >>> Quoting Lionel Landwerlin (2019-08-01 08:43:24) > >>>> On 31/07/2019 23:03, Chris Wilson wrote: > >>>>> Quoting Lionel Landwerlin (2019-07-31 15:07:33) > >>>>> I think I have convinced myself that with the split between wait before, > >>>>> signal after combined with the rule that seqno point along the syncobj > >>>>> are monotonic, you should not be able to generate an AB-BA deadlock > >>>>> between concurrent clients. > >>>>> > >>>>> Except that we need to fixup drm_syncobj_add_point() to actually apply > >>>>> that rule. Throwing an error and leaving the client stuck is less than > >>>>> ideal, we can't even recover with a GPU reset, and as they are native > >>>>> fences we don't employ a failsafe timer. > >>>> Unfortunately we're not the only user of add_point() and amdgpu doesn't > >>>> want it to fail. > >>> It's dangerously exposing the deadlock from incorrect seqno ordering to > >>> userspace. We should be able to hit that DRM_ERROR() in testing, and be > >>> able to detect the out-of-sequence timeline. > >>> > >>>> Best I can come up with is take the syncobj lock when signaling in > >>>> get_timeline_fence_array() do the check there, unlock in __free_fence_array. > >>> I think the intermediate step is a "safe" version of add_point that > >>> doesn't leave the timeline broken. That still leaves a glitch visible to > >>> userspace, but it should not deadlock. > >> > >> Right, sounds doable. What happens in execbuf after add_point() fails? > >> > >> We've already handed the request to the scheduler and potentially it > >> might be running already. > > Right, at present we've already submitted the request, so the batch will > > be run and fence will be signaled. The failure to add to it the > > timeline means that someone else already submitted a later fence and > > some third party is using that syncpt instead of ours for their > > dependency. So that third party will be delayed, but so long as they kept > > their dependencies in order it should just be a delay. > > > But should we return an error to userspace? No, the submission itself hasn't failed and the result of the batch and its fence will be true. It's a case where userspace lost a race with itself -- if there was a non-error warning flag, maybe. If you keep the language loose that a timeline semaphore wait will be after its syncpt has been signaled, but not necessarily before the next syncpt is signaled, unless explicitly ordered by the client. -Chris
On 31/07/2019 23:03, Chris Wilson wrote: > Quoting Lionel Landwerlin (2019-07-31 15:07:33) >> -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; >> + >> + /* 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++)) { >> + err = -EFAULT; >> + goto err; >> + } >> + >> + syncobj = drm_syncobj_find(eb->file, user_fence.handle); >> + if (!syncobj) { >> + DRM_DEBUG("Invalid syncobj handle provided\n"); >> + err = -ENOENT; >> + 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); >> + if (err) { >> + DRM_DEBUG("Syncobj handle missing requested point %llu\n", point); >> + drm_syncobj_put(syncobj); >> + goto err; >> + } >> + >> + /* A point might have been signaled already and >> + * garbage collected from the timeline. In this case >> + * just ignore the point and carry on. >> + */ >> + if (!fence) { >> + drm_syncobj_put(syncobj); >> + continue; >> + } >> + } >> + >> + /* >> + * For timeline syncobjs we need to preallocate chains for >> + * later signaling. >> + */ >> + if (point != 0 && user_fence.flags & I915_EXEC_FENCE_SIGNAL) { > if (dma_fence_chain_find_seqno() == 0) > return -EINVAL; > > as an early sanity check? > >> + fences[num_fences].chain_fence = >> + kmalloc(sizeof(*fences[num_fences].chain_fence), >> + GFP_KERNEL); >> + if (!fences[num_fences].chain_fence) { >> + dma_fence_put(fence); >> + drm_syncobj_put(syncobj); >> + err = -ENOMEM; >> + DRM_DEBUG("Unable to alloc chain_fence\n"); >> + goto err; >> + } >> + } else { >> + fences[num_fences].chain_fence = NULL; >> + } >> + >> + fences[num_fences].syncobj = ptr_pack_bits(syncobj, user_fence.flags, 2); >> + fences[num_fences].dma_fence = fence; >> + fences[num_fences].value = point; >> + num_fences++; >> + } >> + >> + *out_n_fences = num_fences; >> + >> + return fences; >> + >> +err: >> + __free_fence_array(fences, num_fences); >> + return ERR_PTR(err); >> +} >> @@ -2329,9 +2484,9 @@ await_fence_array(struct i915_execbuffer *eb, >> >> static void >> signal_fence_array(struct i915_execbuffer *eb, >> - struct drm_syncobj **fences) >> + struct i915_eb_fences *fences, >> + int nfences) >> { >> - const unsigned int nfences = eb->args->num_cliprects; >> struct dma_fence * const fence = &eb->request->fence; >> unsigned int n; >> >> @@ -2339,15 +2494,46 @@ signal_fence_array(struct i915_execbuffer *eb, >> struct drm_syncobj *syncobj; >> unsigned int flags; >> >> - syncobj = ptr_unpack_bits(fences[n], &flags, 2); >> + syncobj = ptr_unpack_bits(fences[n].syncobj, &flags, 2); >> if (!(flags & I915_EXEC_FENCE_SIGNAL)) >> continue; >> >> - drm_syncobj_replace_fence(syncobj, fence); >> + if (fences[n].chain_fence) { >> + drm_syncobj_add_point(syncobj, fences[n].chain_fence, >> + fence, fences[n].value); >> + /* >> + * The chain's ownership is transfered to the >> + * timeline. >> + */ >> + fences[n].chain_fence = NULL; >> + } else { >> + drm_syncobj_replace_fence(syncobj, fence); >> + } >> } >> } > I think I have convinced myself that with the split between wait before, > signal after combined with the rule that seqno point along the syncobj > are monotonic, you should not be able to generate an AB-BA deadlock > between concurrent clients. Can you come up with an example that would deadlock? Because as far as I can think, we're always submitting fences that are already gone through submission. The only think I could think is an MI_WAIT_SEMAPHORE but that's already problematic today. -Lionel > > Except that we need to fixup drm_syncobj_add_point() to actually apply > that rule. Throwing an error and leaving the client stuck is less than > ideal, we can't even recover with a GPU reset, and as they are native > fences we don't employ a failsafe timer. > > Doesn't WAIT_FOR_SUBMIT throw a spanner in the works and allow for > deadlocks? On the other hand, if they used MI_SEMA rather scheduler > fences, give or take some coarseness in our timeslicing granularity (I > need to enable fast context switches on user semaphores -- first attempt > was just an interrupt storm!) it will work as well as a round-robin > scheduler can work. (In other words, we only need coarse fences for the > scheduler and userspace can implement its own semaphores -- with the > open discussion on how to share the iova user semaphores between > processes, cf ugpu.) > -Chris >
Quoting Lionel Landwerlin (2019-08-01 15:29:34) > On 31/07/2019 23:03, Chris Wilson wrote: > > Quoting Lionel Landwerlin (2019-07-31 15:07:33) > >> -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; > >> + > >> + /* 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++)) { > >> + err = -EFAULT; > >> + goto err; > >> + } > >> + > >> + syncobj = drm_syncobj_find(eb->file, user_fence.handle); > >> + if (!syncobj) { > >> + DRM_DEBUG("Invalid syncobj handle provided\n"); > >> + err = -ENOENT; > >> + 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); > >> + if (err) { > >> + DRM_DEBUG("Syncobj handle missing requested point %llu\n", point); > >> + drm_syncobj_put(syncobj); > >> + goto err; > >> + } > >> + > >> + /* A point might have been signaled already and > >> + * garbage collected from the timeline. In this case > >> + * just ignore the point and carry on. > >> + */ > >> + if (!fence) { > >> + drm_syncobj_put(syncobj); > >> + continue; > >> + } > >> + } > >> + > >> + /* > >> + * For timeline syncobjs we need to preallocate chains for > >> + * later signaling. > >> + */ > >> + if (point != 0 && user_fence.flags & I915_EXEC_FENCE_SIGNAL) { > > if (dma_fence_chain_find_seqno() == 0) > > return -EINVAL; > > > > as an early sanity check? > > > >> + fences[num_fences].chain_fence = > >> + kmalloc(sizeof(*fences[num_fences].chain_fence), > >> + GFP_KERNEL); > >> + if (!fences[num_fences].chain_fence) { > >> + dma_fence_put(fence); > >> + drm_syncobj_put(syncobj); > >> + err = -ENOMEM; > >> + DRM_DEBUG("Unable to alloc chain_fence\n"); > >> + goto err; > >> + } > >> + } else { > >> + fences[num_fences].chain_fence = NULL; > >> + } > >> + > >> + fences[num_fences].syncobj = ptr_pack_bits(syncobj, user_fence.flags, 2); > >> + fences[num_fences].dma_fence = fence; > >> + fences[num_fences].value = point; > >> + num_fences++; > >> + } > >> + > >> + *out_n_fences = num_fences; > >> + > >> + return fences; > >> + > >> +err: > >> + __free_fence_array(fences, num_fences); > >> + return ERR_PTR(err); > >> +} > >> @@ -2329,9 +2484,9 @@ await_fence_array(struct i915_execbuffer *eb, > >> > >> static void > >> signal_fence_array(struct i915_execbuffer *eb, > >> - struct drm_syncobj **fences) > >> + struct i915_eb_fences *fences, > >> + int nfences) > >> { > >> - const unsigned int nfences = eb->args->num_cliprects; > >> struct dma_fence * const fence = &eb->request->fence; > >> unsigned int n; > >> > >> @@ -2339,15 +2494,46 @@ signal_fence_array(struct i915_execbuffer *eb, > >> struct drm_syncobj *syncobj; > >> unsigned int flags; > >> > >> - syncobj = ptr_unpack_bits(fences[n], &flags, 2); > >> + syncobj = ptr_unpack_bits(fences[n].syncobj, &flags, 2); > >> if (!(flags & I915_EXEC_FENCE_SIGNAL)) > >> continue; > >> > >> - drm_syncobj_replace_fence(syncobj, fence); > >> + if (fences[n].chain_fence) { > >> + drm_syncobj_add_point(syncobj, fences[n].chain_fence, > >> + fence, fences[n].value); > >> + /* > >> + * The chain's ownership is transfered to the > >> + * timeline. > >> + */ > >> + fences[n].chain_fence = NULL; > >> + } else { > >> + drm_syncobj_replace_fence(syncobj, fence); > >> + } > >> } > >> } > > I think I have convinced myself that with the split between wait before, > > signal after combined with the rule that seqno point along the syncobj > > are monotonic, you should not be able to generate an AB-BA deadlock > > between concurrent clients. > > > Can you come up with an example that would deadlock? Timeline holds 2,1; a wait on 2 will fail with -EINVAL to userspace. (Though possibly perfectly valid behaviour on the part of the user.) Timeline holds 2, with 1 being submitted. A wait on 1 waits on 2 instead. If 1 gains a dependency on A (e.g. bad userspace or an implicit fence, it's a concurrent wait/submit so expect the worst, i.e. userspace has to be racing with itself to get into this mess), you now have a deadlock. (The assumption being that the syncpt along the timeline are themselves not strictly ordered, and considering they are external syncobj, that seems like a reasonable generalisation.) -Chris
On 01/08/2019 18:16, Chris Wilson wrote: > Quoting Lionel Landwerlin (2019-08-01 15:29:34) >> On 31/07/2019 23:03, Chris Wilson wrote: >>> Quoting Lionel Landwerlin (2019-07-31 15:07:33) ... >>> I think I have convinced myself that with the split between wait before, >>> signal after combined with the rule that seqno point along the syncobj >>> are monotonic, you should not be able to generate an AB-BA deadlock >>> between concurrent clients. >> >> Can you come up with an example that would deadlock? > Timeline holds 2,1; a wait on 2 will fail with -EINVAL to userspace. > (Though possibly perfectly valid behaviour on the part of the user.) > > Timeline holds 2, with 1 being submitted. A wait on 1 waits on 2 > instead. If 1 gains a dependency on A (e.g. bad userspace or an You mean : 1 gains a dependency on 2? > implicit fence, it's a concurrent wait/submit so expect the worst, i.e. > userspace has to be racing with itself to get into this mess), you > now have a deadlock. Not quite sure I see why... 2 doesn't have any dependency on 1, it just happens to have a number higher. -Lionel > (The assumption being that the syncpt along the > timeline are themselves not strictly ordered, and considering they are > external syncobj, that seems like a reasonable generalisation.) > -Chris >
On 01/08/2019 17:16, Chris Wilson wrote: > Quoting Lionel Landwerlin (2019-08-01 15:29:34) >> On 31/07/2019 23:03, Chris Wilson wrote: >>> Quoting Lionel Landwerlin (2019-07-31 15:07:33) >>>> -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; >>>> + >>>> + /* 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++)) { >>>> + err = -EFAULT; >>>> + goto err; >>>> + } >>>> + >>>> + syncobj = drm_syncobj_find(eb->file, user_fence.handle); >>>> + if (!syncobj) { >>>> + DRM_DEBUG("Invalid syncobj handle provided\n"); >>>> + err = -ENOENT; >>>> + 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); >>>> + if (err) { >>>> + DRM_DEBUG("Syncobj handle missing requested point %llu\n", point); >>>> + drm_syncobj_put(syncobj); >>>> + goto err; >>>> + } >>>> + >>>> + /* A point might have been signaled already and >>>> + * garbage collected from the timeline. In this case >>>> + * just ignore the point and carry on. >>>> + */ >>>> + if (!fence) { >>>> + drm_syncobj_put(syncobj); >>>> + continue; >>>> + } >>>> + } >>>> + >>>> + /* >>>> + * For timeline syncobjs we need to preallocate chains for >>>> + * later signaling. >>>> + */ >>>> + if (point != 0 && user_fence.flags & I915_EXEC_FENCE_SIGNAL) { >>> if (dma_fence_chain_find_seqno() == 0) >>> return -EINVAL; >>> >>> as an early sanity check? >>> >>>> + fences[num_fences].chain_fence = >>>> + kmalloc(sizeof(*fences[num_fences].chain_fence), >>>> + GFP_KERNEL); >>>> + if (!fences[num_fences].chain_fence) { >>>> + dma_fence_put(fence); >>>> + drm_syncobj_put(syncobj); >>>> + err = -ENOMEM; >>>> + DRM_DEBUG("Unable to alloc chain_fence\n"); >>>> + goto err; >>>> + } >>>> + } else { >>>> + fences[num_fences].chain_fence = NULL; >>>> + } >>>> + >>>> + fences[num_fences].syncobj = ptr_pack_bits(syncobj, user_fence.flags, 2); >>>> + fences[num_fences].dma_fence = fence; >>>> + fences[num_fences].value = point; >>>> + num_fences++; >>>> + } >>>> + >>>> + *out_n_fences = num_fences; >>>> + >>>> + return fences; >>>> + >>>> +err: >>>> + __free_fence_array(fences, num_fences); >>>> + return ERR_PTR(err); >>>> +} >>>> @@ -2329,9 +2484,9 @@ await_fence_array(struct i915_execbuffer *eb, >>>> >>>> static void >>>> signal_fence_array(struct i915_execbuffer *eb, >>>> - struct drm_syncobj **fences) >>>> + struct i915_eb_fences *fences, >>>> + int nfences) >>>> { >>>> - const unsigned int nfences = eb->args->num_cliprects; >>>> struct dma_fence * const fence = &eb->request->fence; >>>> unsigned int n; >>>> >>>> @@ -2339,15 +2494,46 @@ signal_fence_array(struct i915_execbuffer *eb, >>>> struct drm_syncobj *syncobj; >>>> unsigned int flags; >>>> >>>> - syncobj = ptr_unpack_bits(fences[n], &flags, 2); >>>> + syncobj = ptr_unpack_bits(fences[n].syncobj, &flags, 2); >>>> if (!(flags & I915_EXEC_FENCE_SIGNAL)) >>>> continue; >>>> >>>> - drm_syncobj_replace_fence(syncobj, fence); >>>> + if (fences[n].chain_fence) { >>>> + drm_syncobj_add_point(syncobj, fences[n].chain_fence, >>>> + fence, fences[n].value); >>>> + /* >>>> + * The chain's ownership is transfered to the >>>> + * timeline. >>>> + */ >>>> + fences[n].chain_fence = NULL; >>>> + } else { >>>> + drm_syncobj_replace_fence(syncobj, fence); >>>> + } >>>> } >>>> } >>> I think I have convinced myself that with the split between wait before, >>> signal after combined with the rule that seqno point along the syncobj >>> are monotonic, you should not be able to generate an AB-BA deadlock >>> between concurrent clients. >> >> Can you come up with an example that would deadlock? > Timeline holds 2,1; a wait on 2 will fail with -EINVAL to userspace. > (Though possibly perfectly valid behaviour on the part of the user.) > > Timeline holds 2, with 1 being submitted. A wait on 1 waits on 2 > instead. If 1 gains a dependency on A (e.g. bad userspace or an > implicit fence, it's a concurrent wait/submit so expect the worst, i.e. > userspace has to be racing with itself to get into this mess), you > now have a deadlock. (The assumption being that the syncpt along the > timeline are themselves not strictly ordered, and considering they are > external syncobj, that seems like a reasonable generalisation.) > -Chris > Hey Chris, Does this discussion around how the dma-fence-chain works prevent those AB-BA deadlocks? : https://lists.freedesktop.org/archives/dri-devel/2019-August/229289.html Essentially any point added to the timeline will not be signaled until all previous points are. This is ensured by the dma-fence-chain node which waits for all previous dma-fence-chains nodes to be signal and its own wrapped fence (in our case an i915 one). Thanks, -Lionel
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 58eb3bd57656..d0e39986f0f4 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 */ @@ -274,6 +281,7 @@ struct i915_execbuffer { struct { u64 flags; /** Available extensions parameters */ + struct drm_i915_gem_execbuffer_ext_timeline_fences timeline_fences; } extensions; }; @@ -2219,67 +2227,207 @@ 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; + 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 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++)) { + err = -EFAULT; + goto err; + } + + syncobj = drm_syncobj_find(eb->file, user_fence.handle); + if (!syncobj) { + DRM_DEBUG("Invalid syncobj handle provided\n"); + err = -ENOENT; + 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); + if (err) { + DRM_DEBUG("Syncobj handle missing requested point %llu\n", point); + drm_syncobj_put(syncobj); + goto err; + } + + /* A point might have been signaled already and + * garbage collected from the timeline. In this case + * just ignore the point and carry on. + */ + if (!fence) { + drm_syncobj_put(syncobj); + continue; + } + } + + /* + * For timeline syncobjs we need to preallocate chains for + * later signaling. + */ + if (point != 0 && user_fence.flags & I915_EXEC_FENCE_SIGNAL) { + fences[num_fences].chain_fence = + kmalloc(sizeof(*fences[num_fences].chain_fence), + GFP_KERNEL); + if (!fences[num_fences].chain_fence) { + dma_fence_put(fence); + drm_syncobj_put(syncobj); + err = -ENOMEM; + DRM_DEBUG("Unable to alloc chain_fence\n"); + goto err; + } + } else { + fences[num_fences].chain_fence = NULL; + } + + fences[num_fences].syncobj = ptr_pack_bits(syncobj, user_fence.flags, 2); + fences[num_fences].dma_fence = fence; + fences[num_fences].value = point; + num_fences++; + } + + *out_n_fences = num_fences; + + return fences; + +err: + __free_fence_array(fences, num_fences); + return ERR_PTR(err); +} + +static struct i915_eb_fences * +get_legacy_fence_array(struct i915_execbuffer *eb, + int *out_n_fences) { - const unsigned long nfences = args->num_cliprects; + struct drm_i915_gem_execbuffer2 *args = eb->args; struct drm_i915_gem_exec_fence __user *user; - struct drm_syncobj **fences; + struct i915_eb_fences *fences; + const u32 num_fences = args->num_cliprects; unsigned long n; int err; - if (!(args->flags & I915_EXEC_FENCE_ARRAY)) - return NULL; + *out_n_fences = num_fences; /* Check multiplication overflow for access_ok() and kvmalloc_array() */ BUILD_BUG_ON(sizeof(size_t) > sizeof(unsigned long)); - if (nfences > min_t(unsigned long, - ULONG_MAX / sizeof(*user), - SIZE_MAX / sizeof(*fences))) + if (*out_n_fences > min_t(unsigned long, + ULONG_MAX / sizeof(*user), + SIZE_MAX / sizeof(*fences))) return ERR_PTR(-EINVAL); user = u64_to_user_ptr(args->cliprects_ptr); - if (!access_ok(user, nfences * sizeof(*user))) + if (!access_ok(user, *out_n_fences * sizeof(*user))) return ERR_PTR(-EFAULT); - fences = kvmalloc_array(nfences, sizeof(*fences), + fences = kvmalloc_array(*out_n_fences, sizeof(*fences), __GFP_NOWARN | GFP_KERNEL); if (!fences) return ERR_PTR(-ENOMEM); - for (n = 0; n < nfences; n++) { - struct drm_i915_gem_exec_fence fence; + for (n = 0; n < *out_n_fences; n++) { + struct drm_i915_gem_exec_fence user_fence; struct drm_syncobj *syncobj; + struct dma_fence *fence = NULL; - if (__copy_from_user(&fence, user++, sizeof(fence))) { + if (__copy_from_user(&user_fence, user++, sizeof(user_fence))) { err = -EFAULT; goto err; } - if (fence.flags & __I915_EXEC_FENCE_UNKNOWN_FLAGS) { + if (user_fence.flags & __I915_EXEC_FENCE_UNKNOWN_FLAGS) { err = -EINVAL; goto err; } - syncobj = drm_syncobj_find(file, fence.handle); + syncobj = drm_syncobj_find(eb->file, user_fence.handle); if (!syncobj) { DRM_DEBUG("Invalid syncobj handle provided\n"); err = -ENOENT; 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; + } + } + BUILD_BUG_ON(~(ARCH_KMALLOC_MINALIGN - 1) & ~__I915_EXEC_FENCE_UNKNOWN_FLAGS); - fences[n] = ptr_pack_bits(syncobj, fence.flags, 2); + fences[n].syncobj = ptr_pack_bits(syncobj, user_fence.flags, 2); + fences[n].dma_fence = fence; + fences[n].value = 0; + fences[n].chain_fence = NULL; } return fences; @@ -2289,37 +2437,44 @@ get_fence_array(struct drm_i915_gem_execbuffer2 *args, return ERR_PTR(err); } +static struct i915_eb_fences * +get_fence_array(struct i915_execbuffer *eb, int *out_n_fences) +{ + if (eb->args->flags & I915_EXEC_FENCE_ARRAY) + return get_legacy_fence_array(eb, out_n_fences); + + if (eb->extensions.flags & BIT_ULL(DRM_I915_GEM_EXECBUFFER_EXT_TIMELINE_FENCES)) + return get_timeline_fence_array(eb, out_n_fences); + + *out_n_fences = 0; + return NULL; +} + static void -put_fence_array(struct drm_i915_gem_execbuffer2 *args, - struct drm_syncobj **fences) +put_fence_array(struct i915_eb_fences *fences, int nfences) { if (fences) - __free_fence_array(fences, args->num_cliprects); + __free_fence_array(fences, nfences); } static int await_fence_array(struct i915_execbuffer *eb, - struct drm_syncobj **fences) + struct i915_eb_fences *fences, + int nfences) { - const unsigned int nfences = eb->args->num_cliprects; unsigned int n; int err; for (n = 0; n < nfences; n++) { struct drm_syncobj *syncobj; - struct dma_fence *fence; unsigned int flags; - syncobj = ptr_unpack_bits(fences[n], &flags, 2); + syncobj = ptr_unpack_bits(fences[n].syncobj, &flags, 2); if (!(flags & I915_EXEC_FENCE_WAIT)) continue; - fence = drm_syncobj_fence_get(syncobj); - if (!fence) - return -EINVAL; - - err = i915_request_await_dma_fence(eb->request, fence); - dma_fence_put(fence); + err = i915_request_await_dma_fence(eb->request, + fences[n].dma_fence); if (err < 0) return err; } @@ -2329,9 +2484,9 @@ await_fence_array(struct i915_execbuffer *eb, static void signal_fence_array(struct i915_execbuffer *eb, - struct drm_syncobj **fences) + struct i915_eb_fences *fences, + int nfences) { - const unsigned int nfences = eb->args->num_cliprects; struct dma_fence * const fence = &eb->request->fence; unsigned int n; @@ -2339,15 +2494,46 @@ signal_fence_array(struct i915_execbuffer *eb, struct drm_syncobj *syncobj; unsigned int flags; - syncobj = ptr_unpack_bits(fences[n], &flags, 2); + syncobj = ptr_unpack_bits(fences[n].syncobj, &flags, 2); if (!(flags & I915_EXEC_FENCE_SIGNAL)) continue; - drm_syncobj_replace_fence(syncobj, fence); + if (fences[n].chain_fence) { + drm_syncobj_add_point(syncobj, fences[n].chain_fence, + fence, fences[n].value); + /* + * The chain's ownership is transfered to the + * timeline. + */ + fences[n].chain_fence = NULL; + } else { + drm_syncobj_replace_fence(syncobj, fence); + } } } +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_ULL(DRM_I915_GEM_EXECBUFFER_EXT_TIMELINE_FENCES)) + return -EINVAL; + + if (copy_from_user(&eb->extensions.timeline_fences, ext, + sizeof(eb->extensions.timeline_fences))) + return -EFAULT; + + eb->extensions.flags |= BIT_ULL(DRM_I915_GEM_EXECBUFFER_EXT_TIMELINE_FENCES); + + return 0; +} + static const i915_user_extension_fn execbuf_extensions[] = { + [DRM_I915_GEM_EXECBUFFER_EXT_TIMELINE_FENCES] = parse_timeline_fences, }; static int @@ -2378,14 +2564,15 @@ static int i915_gem_do_execbuffer(struct drm_device *dev, struct drm_file *file, struct drm_i915_gem_execbuffer2 *args, - struct drm_i915_gem_exec_object2 *exec, - struct drm_syncobj **fences) + struct drm_i915_gem_exec_object2 *exec) { struct i915_execbuffer eb; struct dma_fence *in_fence = NULL; struct dma_fence *exec_fence = NULL; struct sync_file *out_fence = NULL; + struct i915_eb_fences *fences = NULL; int out_fence_fd = -1; + int nfences = 0; int err; BUILD_BUG_ON(__EXEC_INTERNAL_FLAGS & ~__I915_EXEC_ILLEGAL_FLAGS); @@ -2424,10 +2611,16 @@ i915_gem_do_execbuffer(struct drm_device *dev, if (err) return err; + fences = get_fence_array(&eb, &nfences); + if (IS_ERR(fences)) + return PTR_ERR(fences); + if (args->flags & I915_EXEC_FENCE_IN) { in_fence = sync_file_get_fence(lower_32_bits(args->rsvd2)); - if (!in_fence) - return -EINVAL; + if (!in_fence) { + err = -EINVAL; + goto err_fences; + } } if (args->flags & I915_EXEC_FENCE_SUBMIT) { @@ -2585,7 +2778,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, } if (fences) { - err = await_fence_array(&eb, fences); + err = await_fence_array(&eb, fences, nfences); if (err) goto err_request; } @@ -2614,7 +2807,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, i915_request_add(eb.request); if (fences) - signal_fence_array(&eb, fences); + signal_fence_array(&eb, fences, nfences); if (out_fence) { if (err == 0) { @@ -2649,6 +2842,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, dma_fence_put(exec_fence); err_in_fence: dma_fence_put(in_fence); +err_fences: + put_fence_array(fences, nfences); return err; } @@ -2742,7 +2937,7 @@ i915_gem_execbuffer_ioctl(struct drm_device *dev, void *data, exec2_list[i].flags = 0; } - err = i915_gem_do_execbuffer(dev, file, &exec2, exec2_list, NULL); + err = i915_gem_do_execbuffer(dev, file, &exec2, exec2_list); if (exec2.flags & __EXEC_HAS_RELOC) { struct drm_i915_gem_exec_object __user *user_exec_list = u64_to_user_ptr(args->buffers_ptr); @@ -2773,7 +2968,6 @@ i915_gem_execbuffer2_ioctl(struct drm_device *dev, void *data, { struct drm_i915_gem_execbuffer2 *args = data; struct drm_i915_gem_exec_object2 *exec2_list; - struct drm_syncobj **fences = NULL; const size_t count = args->buffer_count; int err; @@ -2801,15 +2995,7 @@ i915_gem_execbuffer2_ioctl(struct drm_device *dev, void *data, return -EFAULT; } - if (args->flags & I915_EXEC_FENCE_ARRAY) { - fences = get_fence_array(args, file); - if (IS_ERR(fences)) { - kvfree(exec2_list); - return PTR_ERR(fences); - } - } - - err = i915_gem_do_execbuffer(dev, file, args, exec2_list, fences); + err = i915_gem_do_execbuffer(dev, file, args, exec2_list); /* * Now that we have begun execution of the batchbuffer, we ignore @@ -2849,7 +3035,6 @@ end:; } args->flags &= ~__I915_EXEC_UNKNOWN_FLAGS; - put_fence_array(args, fences); kvfree(exec2_list); return err; } diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 761726818a22..a6acece99abf 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -463,6 +463,7 @@ static int i915_getparam_ioctl(struct drm_device *dev, void *data, case I915_PARAM_HAS_EXEC_BATCH_FIRST: case I915_PARAM_HAS_EXEC_FENCE_ARRAY: case I915_PARAM_HAS_EXEC_SUBMIT_FENCE: + case I915_PARAM_HAS_EXEC_TIMELINE_FENCES: /* For the time being all of these are always true; * if some supported hardware does not have one of these * features this value needs to be provided from @@ -3208,7 +3209,8 @@ static struct drm_driver driver = { */ .driver_features = DRIVER_GEM | - DRIVER_RENDER | DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_SYNCOBJ, + DRIVER_RENDER | DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_SYNCOBJ | + DRIVER_SYNCOBJ_TIMELINE, .release = i915_driver_release, .open = i915_driver_open, .lastclose = i915_driver_lastclose, diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 7ea35b048a82..92c3323f3167 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -611,6 +611,13 @@ typedef struct drm_i915_irq_wait { * See I915_EXEC_FENCE_OUT and I915_EXEC_FENCE_SUBMIT. */ #define I915_PARAM_HAS_EXEC_SUBMIT_FENCE 53 + +/* Query whether DRM_I915_GEM_EXECBUFFER2 supports supplying an array of + * timeline syncobj through drm_i915_gem_execbuf_ext_timeline_fences. See + * I915_EXEC_EXT. + */ +#define I915_PARAM_HAS_EXEC_TIMELINE_FENCES 54 + /* Must be kept compact -- no holes and well documented */ typedef struct drm_i915_getparam { @@ -1008,9 +1015,41 @@ struct drm_i915_gem_exec_fence { }; enum drm_i915_gem_execbuffer_ext { + /** + * See drm_i915_gem_execbuf_ext_timeline_fences. + */ + DRM_I915_GEM_EXECBUFFER_EXT_TIMELINE_FENCES = 0, + DRM_I915_GEM_EXECBUFFER_EXT_MAX /* non-ABI */ }; +/** + * This structure describes an array of drm_syncobj and associated points for + * timeline variants of drm_syncobj. It is invalid to append this structure to + * the execbuf if I915_EXEC_FENCE_ARRAY is set. + */ +struct drm_i915_gem_execbuffer_ext_timeline_fences { + struct i915_user_extension base; + + /** + * Number of element in the handles_ptr & value_ptr arrays. + */ + __u64 fence_count; + + /** + * Pointer to an array of struct drm_i915_gem_exec_fence of length + * fence_count. + */ + __u64 handles_ptr; + + /** + * Pointer to an array of u64 values of length fence_count. Values + * must be 0 for a binary drm_syncobj. A Value of 0 for a timeline + * drm_syncobj is invalid as it turns a drm_syncobj into a binary one. + */ + __u64 values_ptr; +}; + struct drm_i915_gem_execbuffer2 { /** * List of gem_exec_object2 structs
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) Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> --- .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 297 ++++++++++++++---- drivers/gpu/drm/i915/i915_drv.c | 4 +- include/uapi/drm/i915_drm.h | 39 +++ 3 files changed, 283 insertions(+), 57 deletions(-)