| Message ID | 20211021103605.735002-2-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/28] drm/i915: Fix i915_request fence wait semantics | expand |
Am 21.10.21 um 12:35 schrieb Maarten Lankhorst: > From: Christian König <christian.koenig@amd.com> > > Simplifying the code a bit. > > Signed-off-by: Christian König <christian.koenig@amd.com> > [mlankhorst: Handle timeout = 0 correctly, use new i915_request_wait_timeout.] > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> LGTM, do you want to push it or should I pick it up into drm-misc-next? Christian. > --- > drivers/gpu/drm/i915/gem/i915_gem_wait.c | 65 ++++++++---------------- > 1 file changed, 20 insertions(+), 45 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c > index f909aaa09d9c..840c13706999 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c > @@ -25,7 +25,7 @@ i915_gem_object_wait_fence(struct dma_fence *fence, > return timeout; > > if (dma_fence_is_i915(fence)) > - return i915_request_wait(to_request(fence), flags, timeout); > + return i915_request_wait_timeout(to_request(fence), flags, timeout); > > return dma_fence_wait_timeout(fence, > flags & I915_WAIT_INTERRUPTIBLE, > @@ -37,58 +37,29 @@ i915_gem_object_wait_reservation(struct dma_resv *resv, > unsigned int flags, > long timeout) > { > - struct dma_fence *excl; > - bool prune_fences = false; > - > - if (flags & I915_WAIT_ALL) { > - struct dma_fence **shared; > - unsigned int count, i; > - int ret; > - > - ret = dma_resv_get_fences(resv, &excl, &count, &shared); > - if (ret) > - return ret; > - > - for (i = 0; i < count; i++) { > - timeout = i915_gem_object_wait_fence(shared[i], > - flags, timeout); > - if (timeout < 0) > - break; > - > - dma_fence_put(shared[i]); > - } > - > - for (; i < count; i++) > - dma_fence_put(shared[i]); > - kfree(shared); > + struct dma_resv_iter cursor; > + struct dma_fence *fence; > + long ret = timeout ?: 1; > + > + dma_resv_iter_begin(&cursor, resv, flags & I915_WAIT_ALL); > + dma_resv_for_each_fence_unlocked(&cursor, fence) { > + ret = i915_gem_object_wait_fence(fence, flags, timeout); > + if (ret <= 0) > + break; > > - /* > - * If both shared fences and an exclusive fence exist, > - * then by construction the shared fences must be later > - * than the exclusive fence. If we successfully wait for > - * all the shared fences, we know that the exclusive fence > - * must all be signaled. If all the shared fences are > - * signaled, we can prune the array and recover the > - * floating references on the fences/requests. > - */ > - prune_fences = count && timeout >= 0; > - } else { > - excl = dma_resv_get_excl_unlocked(resv); > + if (timeout) > + timeout = ret; > } > - > - if (excl && timeout >= 0) > - timeout = i915_gem_object_wait_fence(excl, flags, timeout); > - > - dma_fence_put(excl); > + dma_resv_iter_end(&cursor); > > /* > * Opportunistically prune the fences iff we know they have *all* been > * signaled. > */ > - if (prune_fences) > + if (timeout > 0) > dma_resv_prune(resv); > > - return timeout; > + return ret; > } > > static void fence_set_priority(struct dma_fence *fence, > @@ -196,7 +167,11 @@ i915_gem_object_wait(struct drm_i915_gem_object *obj, > > timeout = i915_gem_object_wait_reservation(obj->base.resv, > flags, timeout); > - return timeout < 0 ? timeout : 0; > + > + if (timeout < 0) > + return timeout; > + > + return !timeout ? -ETIME : 0; > } > > static inline unsigned long nsecs_to_jiffies_timeout(const u64 n)
Op 21-10-2021 om 12:38 schreef Christian König: > Am 21.10.21 um 12:35 schrieb Maarten Lankhorst: >> From: Christian König <christian.koenig@amd.com> >> >> Simplifying the code a bit. >> >> Signed-off-by: Christian König <christian.koenig@amd.com> >> [mlankhorst: Handle timeout = 0 correctly, use new i915_request_wait_timeout.] >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > LGTM, do you want to push it or should I pick it up into drm-misc-next? I think it can be applied to drm-intel-gt-next, after a backmerge. It needs patch 1 too, which fixes i915_request_wait semantics when used in dma-fence. It exports a dma-fence compatible i915_request_wait_timeout function, used in this patch.
On 21/10/2021 12:06, Maarten Lankhorst wrote: > Op 21-10-2021 om 12:38 schreef Christian König: >> Am 21.10.21 um 12:35 schrieb Maarten Lankhorst: >>> From: Christian König <christian.koenig@amd.com> >>> >>> Simplifying the code a bit. >>> >>> Signed-off-by: Christian König <christian.koenig@amd.com> >>> [mlankhorst: Handle timeout = 0 correctly, use new i915_request_wait_timeout.] >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> >> LGTM, do you want to push it or should I pick it up into drm-misc-next? > > I think it can be applied to drm-intel-gt-next, after a backmerge. It needs patch 1 too, which fixes > > i915_request_wait semantics when used in dma-fence. It exports a dma-fence compatible i915_request_wait_timeout function, used in this patch. I don't think my open has been resolved, at least I haven't seen a reply from Daniel on the topic of potential for infinite waits with untrusted clients after this change. +Daniel Regards, Tvrtko
Am 21.10.21 um 13:13 schrieb Tvrtko Ursulin: > > On 21/10/2021 12:06, Maarten Lankhorst wrote: >> Op 21-10-2021 om 12:38 schreef Christian König: >>> Am 21.10.21 um 12:35 schrieb Maarten Lankhorst: >>>> From: Christian König <christian.koenig@amd.com> >>>> >>>> Simplifying the code a bit. >>>> >>>> Signed-off-by: Christian König <christian.koenig@amd.com> >>>> [mlankhorst: Handle timeout = 0 correctly, use new >>>> i915_request_wait_timeout.] >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> >>> LGTM, do you want to push it or should I pick it up into drm-misc-next? >> >> I think it can be applied to drm-intel-gt-next, after a backmerge. It >> needs patch 1 too, which fixes >> >> i915_request_wait semantics when used in dma-fence. It exports a >> dma-fence compatible i915_request_wait_timeout function, used in this >> patch. What about the other i915 patches? I guess you then want to merge them through drm-intel-gt-next as well. > I don't think my open has been resolved, at least I haven't seen a > reply from Daniel on the topic of potential for infinite waits with > untrusted clients after this change. +Daniel Please resolve that internally and let me know the result. I'm fine to use any of the possible approaches, I just need to know which one. Regards, Christian. > > Regards, > > Tvrtko
On Thu, Oct 28, 2021 at 10:41:38AM +0200, Christian König wrote: > Am 21.10.21 um 13:13 schrieb Tvrtko Ursulin: > > > > On 21/10/2021 12:06, Maarten Lankhorst wrote: > > > Op 21-10-2021 om 12:38 schreef Christian König: > > > > Am 21.10.21 um 12:35 schrieb Maarten Lankhorst: > > > > > From: Christian König <christian.koenig@amd.com> > > > > > > > > > > Simplifying the code a bit. > > > > > > > > > > Signed-off-by: Christian König <christian.koenig@amd.com> > > > > > [mlankhorst: Handle timeout = 0 correctly, use new > > > > > i915_request_wait_timeout.] > > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > > > > > LGTM, do you want to push it or should I pick it up into drm-misc-next? > > > > > > I think it can be applied to drm-intel-gt-next, after a backmerge. > > > It needs patch 1 too, which fixes > > > > > > i915_request_wait semantics when used in dma-fence. It exports a > > > dma-fence compatible i915_request_wait_timeout function, used in > > > this patch. > > What about the other i915 patches? I guess you then want to merge them > through drm-intel-gt-next as well. > > > I don't think my open has been resolved, at least I haven't seen a reply > > from Daniel on the topic of potential for infinite waits with untrusted > > clients after this change. +Daniel > > Please resolve that internally and let me know the result. I'm fine to use > any of the possible approaches, I just need to know which one. I thought I explained this in the patch set from Maarten. This isn't an issue, since the exact same thing can happen if you get interrupts and stuff. The only proper fix for bounding the waits is a) compositor grabs a stable set of dma_fence from the dma-buf through the proposed fence export ioctl b) compositor waits on that fence (or drm_syncobj). Everything else is cargo-culted nonsense, and very much includes that igt patch that's floating around internally. I can also whack this into drm-next if this is stuck in this silly bikeshed. -Daniel
On 28/10/2021 16:30, Daniel Vetter wrote: > On Thu, Oct 28, 2021 at 10:41:38AM +0200, Christian König wrote: >> Am 21.10.21 um 13:13 schrieb Tvrtko Ursulin: >>> >>> On 21/10/2021 12:06, Maarten Lankhorst wrote: >>>> Op 21-10-2021 om 12:38 schreef Christian König: >>>>> Am 21.10.21 um 12:35 schrieb Maarten Lankhorst: >>>>>> From: Christian König <christian.koenig@amd.com> >>>>>> >>>>>> Simplifying the code a bit. >>>>>> >>>>>> Signed-off-by: Christian König <christian.koenig@amd.com> >>>>>> [mlankhorst: Handle timeout = 0 correctly, use new >>>>>> i915_request_wait_timeout.] >>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>>> >>>>> LGTM, do you want to push it or should I pick it up into drm-misc-next? >>>> >>>> I think it can be applied to drm-intel-gt-next, after a backmerge. >>>> It needs patch 1 too, which fixes >>>> >>>> i915_request_wait semantics when used in dma-fence. It exports a >>>> dma-fence compatible i915_request_wait_timeout function, used in >>>> this patch. >> >> What about the other i915 patches? I guess you then want to merge them >> through drm-intel-gt-next as well. >> >>> I don't think my open has been resolved, at least I haven't seen a reply >>> from Daniel on the topic of potential for infinite waits with untrusted >>> clients after this change. +Daniel >> >> Please resolve that internally and let me know the result. I'm fine to use >> any of the possible approaches, I just need to know which one. > > I thought I explained this in the patch set from Maarten. This isn't an > issue, since the exact same thing can happen if you get interrupts and > stuff. Ah were you trying to point out all this time the infinite wait just got moved from inside the "old" dma_resv_get_fences to the new iterator caller? Regards, Tvrtko > > The only proper fix for bounding the waits is a) compositor grabs a stable > set of dma_fence from the dma-buf through the proposed fence export ioctl > b) compositor waits on that fence (or drm_syncobj). > > Everything else is cargo-culted nonsense, and very much includes that igt > patch that's floating around internally. > > I can also whack this into drm-next if this is stuck in this silly > bikeshed. > -Daniel >
Am 01.11.21 um 10:41 schrieb Tvrtko Ursulin: > > On 28/10/2021 16:30, Daniel Vetter wrote: >> On Thu, Oct 28, 2021 at 10:41:38AM +0200, Christian König wrote: >>> Am 21.10.21 um 13:13 schrieb Tvrtko Ursulin: >>>> >>>> On 21/10/2021 12:06, Maarten Lankhorst wrote: >>>>> Op 21-10-2021 om 12:38 schreef Christian König: >>>>>> Am 21.10.21 um 12:35 schrieb Maarten Lankhorst: >>>>>>> From: Christian König <christian.koenig@amd.com> >>>>>>> >>>>>>> Simplifying the code a bit. >>>>>>> >>>>>>> Signed-off-by: Christian König <christian.koenig@amd.com> >>>>>>> [mlankhorst: Handle timeout = 0 correctly, use new >>>>>>> i915_request_wait_timeout.] >>>>>>> Signed-off-by: Maarten Lankhorst >>>>>>> <maarten.lankhorst@linux.intel.com> >>>>>> >>>>>> LGTM, do you want to push it or should I pick it up into >>>>>> drm-misc-next? >>>>> >>>>> I think it can be applied to drm-intel-gt-next, after a backmerge. >>>>> It needs patch 1 too, which fixes >>>>> >>>>> i915_request_wait semantics when used in dma-fence. It exports a >>>>> dma-fence compatible i915_request_wait_timeout function, used in >>>>> this patch. >>> >>> What about the other i915 patches? I guess you then want to merge them >>> through drm-intel-gt-next as well. >>> >>>> I don't think my open has been resolved, at least I haven't seen a >>>> reply >>>> from Daniel on the topic of potential for infinite waits with >>>> untrusted >>>> clients after this change. +Daniel >>> >>> Please resolve that internally and let me know the result. I'm fine >>> to use >>> any of the possible approaches, I just need to know which one. >> >> I thought I explained this in the patch set from Maarten. This isn't an >> issue, since the exact same thing can happen if you get interrupts and >> stuff. > > Ah were you trying to point out all this time the infinite wait just > got moved from inside the "old" dma_resv_get_fences to the new > iterator caller? At least I think so, yes. But Daniel needs to answer that. > > Regards, > > Tvrtko > >> >> The only proper fix for bounding the waits is a) compositor grabs a >> stable >> set of dma_fence from the dma-buf through the proposed fence export >> ioctl >> b) compositor waits on that fence (or drm_syncobj). >> >> Everything else is cargo-culted nonsense, and very much includes that >> igt >> patch that's floating around internally. >> >> I can also whack this into drm-next if this is stuck in this silly >> bikeshed. Can we move forward with those patches? I still don't see them in drm-misc-next. I you want I can also pick Maartens modified version here up and send it out with the reset of the i915 patches once more. Everything else is already pushed. Thanks, Christian. >> -Daniel >>
On Thu, Nov 11, 2021 at 12:36:47PM +0100, Christian König wrote: > Am 01.11.21 um 10:41 schrieb Tvrtko Ursulin: > > > > On 28/10/2021 16:30, Daniel Vetter wrote: > > > On Thu, Oct 28, 2021 at 10:41:38AM +0200, Christian König wrote: > > > > Am 21.10.21 um 13:13 schrieb Tvrtko Ursulin: > > > > > > > > > > On 21/10/2021 12:06, Maarten Lankhorst wrote: > > > > > > Op 21-10-2021 om 12:38 schreef Christian König: > > > > > > > Am 21.10.21 um 12:35 schrieb Maarten Lankhorst: > > > > > > > > From: Christian König <christian.koenig@amd.com> > > > > > > > > > > > > > > > > Simplifying the code a bit. > > > > > > > > > > > > > > > > Signed-off-by: Christian König <christian.koenig@amd.com> > > > > > > > > [mlankhorst: Handle timeout = 0 correctly, use new > > > > > > > > i915_request_wait_timeout.] > > > > > > > > Signed-off-by: Maarten Lankhorst > > > > > > > > <maarten.lankhorst@linux.intel.com> > > > > > > > > > > > > > > LGTM, do you want to push it or should I pick it up > > > > > > > into drm-misc-next? > > > > > > > > > > > > I think it can be applied to drm-intel-gt-next, after a backmerge. > > > > > > It needs patch 1 too, which fixes > > > > > > > > > > > > i915_request_wait semantics when used in dma-fence. It exports a > > > > > > dma-fence compatible i915_request_wait_timeout function, used in > > > > > > this patch. > > > > > > > > What about the other i915 patches? I guess you then want to merge them > > > > through drm-intel-gt-next as well. > > > > > > > > > I don't think my open has been resolved, at least I haven't > > > > > seen a reply > > > > > from Daniel on the topic of potential for infinite waits > > > > > with untrusted > > > > > clients after this change. +Daniel > > > > > > > > Please resolve that internally and let me know the result. I'm > > > > fine to use > > > > any of the possible approaches, I just need to know which one. > > > > > > I thought I explained this in the patch set from Maarten. This isn't an > > > issue, since the exact same thing can happen if you get interrupts and > > > stuff. > > > > Ah were you trying to point out all this time the infinite wait just got > > moved from inside the "old" dma_resv_get_fences to the new iterator > > caller? > > At least I think so, yes. But Daniel needs to answer that. Well maybe there's also an infinite wait inside the old dma_resv_get_fences, what I mean is that when you have some signals interrupting you, then the infinite wait is already there due to a retry loop outside of the syscall even. Anyway _any_ userspace which wants to use this wait on a shared bo and waits to be safe against the other end adding more rendering has to use something else (like the sync_file export ioctl on the dma-buf that Jason typed). Trying to make this ioctl here against fence addition is just bs. > > Regards, > > > > Tvrtko > > > > > > > > The only proper fix for bounding the waits is a) compositor grabs a > > > stable > > > set of dma_fence from the dma-buf through the proposed fence export > > > ioctl > > > b) compositor waits on that fence (or drm_syncobj). > > > > > > Everything else is cargo-culted nonsense, and very much includes > > > that igt > > > patch that's floating around internally. > > > > > > I can also whack this into drm-next if this is stuck in this silly > > > bikeshed. > > Can we move forward with those patches? I still don't see them in > drm-misc-next. > > I you want I can also pick Maartens modified version here up and send it out > with the reset of the i915 patches once more. > > Everything else is already pushed. Please push to drm-misc-next or wherever (assuming CI is happy) and feel free to add my ack. -Daniel
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c index f909aaa09d9c..840c13706999 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c @@ -25,7 +25,7 @@ i915_gem_object_wait_fence(struct dma_fence *fence, return timeout; if (dma_fence_is_i915(fence)) - return i915_request_wait(to_request(fence), flags, timeout); + return i915_request_wait_timeout(to_request(fence), flags, timeout); return dma_fence_wait_timeout(fence, flags & I915_WAIT_INTERRUPTIBLE, @@ -37,58 +37,29 @@ i915_gem_object_wait_reservation(struct dma_resv *resv, unsigned int flags, long timeout) { - struct dma_fence *excl; - bool prune_fences = false; - - if (flags & I915_WAIT_ALL) { - struct dma_fence **shared; - unsigned int count, i; - int ret; - - ret = dma_resv_get_fences(resv, &excl, &count, &shared); - if (ret) - return ret; - - for (i = 0; i < count; i++) { - timeout = i915_gem_object_wait_fence(shared[i], - flags, timeout); - if (timeout < 0) - break; - - dma_fence_put(shared[i]); - } - - for (; i < count; i++) - dma_fence_put(shared[i]); - kfree(shared); + struct dma_resv_iter cursor; + struct dma_fence *fence; + long ret = timeout ?: 1; + + dma_resv_iter_begin(&cursor, resv, flags & I915_WAIT_ALL); + dma_resv_for_each_fence_unlocked(&cursor, fence) { + ret = i915_gem_object_wait_fence(fence, flags, timeout); + if (ret <= 0) + break; - /* - * If both shared fences and an exclusive fence exist, - * then by construction the shared fences must be later - * than the exclusive fence. If we successfully wait for - * all the shared fences, we know that the exclusive fence - * must all be signaled. If all the shared fences are - * signaled, we can prune the array and recover the - * floating references on the fences/requests. - */ - prune_fences = count && timeout >= 0; - } else { - excl = dma_resv_get_excl_unlocked(resv); + if (timeout) + timeout = ret; } - - if (excl && timeout >= 0) - timeout = i915_gem_object_wait_fence(excl, flags, timeout); - - dma_fence_put(excl); + dma_resv_iter_end(&cursor); /* * Opportunistically prune the fences iff we know they have *all* been * signaled. */ - if (prune_fences) + if (timeout > 0) dma_resv_prune(resv); - return timeout; + return ret; } static void fence_set_priority(struct dma_fence *fence, @@ -196,7 +167,11 @@ i915_gem_object_wait(struct drm_i915_gem_object *obj, timeout = i915_gem_object_wait_reservation(obj->base.resv, flags, timeout); - return timeout < 0 ? timeout : 0; + + if (timeout < 0) + return timeout; + + return !timeout ? -ETIME : 0; } static inline unsigned long nsecs_to_jiffies_timeout(const u64 n)