<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> <div class="moz-cite-prefix">Am 10.08.2017 um 01:53 schrieb Jason Ekstrand: </div> <blockquote type="cite" cite="mid:CAOFGe974Zp6V9gKMmD0Buw9oR1guQ1Kms9JBy5b2Aj9XU9idWg@mail.gmail.com"> <div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Wed, Aug 9, 2017 at 3:41 PM, Chris Wilson <<a href="mailto:chris@chris-wilson.co.uk" target="_blank" moz-do-not-send="true">chris@chris-wilson.co.uk</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Quoting Chris Wilson (2017-08-09 18:57:44) > So we are taking a snapshot here. It looks like this could have been > done using a dma_fence_array + dma_fence_proxy for capturing the future > fence. A quick sketch of this idea looks like: void drm_syncobj_replace_fence(struct drm_syncobj *syncobj, struct dma_fence *fence) { - struct dma_fence *old_fence; + unsigned long flags; - if (fence) - dma_fence_get(fence); - old_fence = xchg(&syncobj->fence, fence); - - dma_fence_put(old_fence); + spin_lock_irqsave(&syncobj->lock, flags); + dma_fence_replace_proxy(&syncobj->fence, fence); + spin_unlock_irqrestore(&syncobj->lock, flags); } +int +drm_syncobj_wait_ioctl(struct drm_device *dev, void *data, + struct drm_file *file_private) +{ + struct drm_syncobj_wait *args = data; + struct dma_fence **fences; + struct dma_fence_array *array; + unsigned long timeout; + unsigned int count; + u32 *handles; + int ret = 0; + u32 i; + + BUILD_BUG_ON(sizeof(*fences) < sizeof(*handles)); + + if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ)) + return -ENODEV; + + if (args->flags != 0 && args->flags != DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL) + return -EINVAL; + + count = args->count_handles; + if (!count) + return -EINVAL; + + /* Get the handles from userspace */ + fences = kmalloc_array(count, + sizeof(struct dma_fence *), + __GFP_NOWARN | GFP_KERNEL); + if (!fences) + return -ENOMEM; + + handles = (void *)fences + count * (sizeof(*fences) - sizeof(*handles)); + if (copy_from_user(handles, + u64_to_user_ptr(args->handles), + sizeof(u32) * count)) { + ret = -EFAULT; + goto err; + } + + for (i = 0; i < count; i++) { + struct drm_syncobj *s; + + ret = -ENOENT; + s = drm_syncobj_find(file_private, handles[i]); + if (s) { + ret = 0; + spin_lock_irq(&s->lock); + if (!s->fence) { + if (args->flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) + s->fence = dma_fence_create_proxy(); + else + ret = -EINVAL; + } + if (s->fence) + fences[i] = dma_fence_get(s->fence); + spin_unlock_irq(&s->lock); + } + if (ret) { + count = i; + goto err_fences; + } + } + + array = dma_fence_array_create(count, fences, 0, 0, + !(args->flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL)); + if (!array) { + ret = -ENOMEM; + goto err_fences; + } + + timeout = drm_timeout_abs_to_jiffies(args->timeout_nsec); + timeout = dma_fence_wait_timeout(&array->base, true, timeout); + args->first_signaled = array->first_signaled; + dma_fence_put(&array->base); + + return timeout < 0 ? timeout : 0; + +err_fences: + for (i = 0; i < count; i++) + dma_fence_put(fences[i]); +err: + kfree(fences); + return ret; +} The key advantage is that this translate the ioctl into a dma-fence-array which already has to deal with the mess, sharing the burden. (But it does require a trivial patch to dma-fence-array to record the first signaled fence.) However, this installs the proxy into syncobj->fence with the result that any concurrent wait also become a WAIT_FOR_SUBMIT. The behaviour of drm_syncobj is then quite inconsistent, sometimes it will wait for a future fence, sometimes it will report an error. </blockquote> </div> </div> <div class="gmail_extra">Yeah, that's not good. I thought about a variety of solutions to try and re-use more core dma_fence code. Ultimately I chose the current one because it takes a snapshot of the syncobjs and then, from that point forward, it's consistent with its snapshot. Nothing I was able to come up with based on core dma_fence wait routines does that. </div> </div> </blockquote> As Chris pointed out, that's really not a good idea. Most of the time we need the behavior of reporting an error and only when the flag is given wait until some fence shows up. In general I suggest that we only support this use case in the form of a wait_event_interruptible() on setting the first fence on an object. Waiting on the first one of multiple objects wouldn't be possible (you would always wait till all objects have fences), but I think that this is acceptable. The big advantage of the wait_event_interruptible() interface is that your condition checking and process handling is bullet prove, as far as I have seen so far your code is a bit buggy in that direction. Christian. <blockquote type="cite" cite="mid:CAOFGe974Zp6V9gKMmD0Buw9oR1guQ1Kms9JBy5b2Aj9XU9idWg@mail.gmail.com"> <div dir="ltr"> <div class="gmail_extra"> </div> <div class="gmail_extra">--Jason </div> </div> <fieldset class="mimeAttachmentHeader"></fieldset> <pre wrap="">_______________________________________________ dri-devel mailing list <a class="moz-txt-link-abbreviated" href="mailto:dri-devel@lists.freedesktop.org">dri-devel@lists.freedesktop.org</a> <a class="moz-txt-link-freetext" href="https://lists.freedesktop.org/mailman/listinfo/dri-devel">https://lists.freedesktop.org/mailman/listinfo/dri-devel</a> </pre> </blockquote> </body> </html>