Hopefully addressing most of these in the upcoming set, some comments below.
+/**
- drm_timeout_abs_to_jiffies - calculate jiffies timeout from absolute value
- @timeout_ns: timeout in ns
- Calculate the timeout in jiffies from an absolute timeout in ns.
- */
+unsigned long drm_timeout_abs_to_jiffies(uint64_t timeout_ns)
Not in any header or otherwise exported, so static?
Done.
/* clamp timeout to avoid unsigned-> signed overflow */if (timeout_jiffies > MAX_SCHEDULE_TIMEOUT )return MAX_SCHEDULE_TIMEOUT - 1;This about avoiding the conversion into an infinite timeout, right? I think the comment is misleading (certainly doesn't explain MAX_SCHEDULE_TIMEOUT-1) and the test should be >= MAX_SCHEDULE_TIMEOUT.
Taken from AMD code, but I see your logic, I've adjust the code and added the one as requested.
return timeout_jiffies;Timeouts tend to use timeout_jiffies + 1 to avoid the timer interrupt screwing up the calculation, or just generally returning a fraction too early.
+}
+static int drm_syncobj_wait_all_fences(struct drm_device *dev,
struct drm_file *file_private,struct drm_syncobj_wait *wait,uint32_t *handles)+{
uint32_t i;int ret;unsigned long timeout = drm_timeout_abs_to_jiffies(wait->timeout_ns);Also note that this doesn't handle timeout = 0 very gracefully with multiple fences. (dma_fence_wait_timeout will convert that on return to 1). Hmm, by using an absolute timeout we can't pass into timeout=0 to do a poll.
I've decdied to make timeout = 0 be poll, I can't see anyone having a realtime clock of 0 making sense anyways.
There is a fix in flight for dma_fence_wait_timeout doing that, it's in drm-next now.
&fence);if (ret)return ret;if (!fence)continue;I thought no fence for the syncobj was the *unsignaled* case, and to wait upon it was a user error. I second Jason's idea to make the lookup and error checking on the fences uniform between WAIT_ALL and WAIT_ANY.
ret = dma_fence_wait_timeout(fence, true, timeout);dma_fence_put(fence);if (ret < 0)return ret;if (ret == 0)break;timeout = ret;}wait->out_timeout_ns = jiffies_to_nsecs(ret);wait->out_status = (ret > 0);wait->first_signaled = 0;return 0;+}
+static int drm_syncobj_wait_any_fence(struct drm_device *dev,
struct drm_file *file_private,struct drm_syncobj_wait *wait,uint32_t *handles)+{
unsigned long timeout = drm_timeout_abs_to_jiffies(wait->timeout_ns);struct dma_fence **array;uint32_t i;int ret;uint32_t first = ~0;/* Prepare the fence array */array = kcalloc(wait->count_handles,sizeof(struct dma_fence *), GFP_KERNEL);if (array == NULL)return -ENOMEM;for (i = 0; i < wait->count_handles; i++) {struct dma_fence *fence;ret = drm_syncobj_fence_get(file_private, handles[i],&fence);if (ret)goto err_free_fence_array;else if (fence)array[i] = fence;else { /* NULL, the fence has been already signaled */ret = 1;goto out;}}ret = dma_fence_wait_any_timeout(array, wait->count_handles, true, timeout,&first);if (ret < 0)goto err_free_fence_array;+out:
wait->out_timeout_ns = jiffies_to_nsecs(ret);wait->out_status = (ret > 0);Didn't the amdgpu interface report which fence completed first? (I may be imagining that.)
Just below your comment down there is first_signaled getting assigned!
wait->first_signaled = first;/* set return value 0 to indicate success */ret = 0;+err_free_fence_array:
for (i = 0; i < wait->count_handles; i++)dma_fence_put(array[i]);kfree(array);return ret;+}
+int +drm_syncobj_wait_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_private)+{
struct drm_syncobj_wait *args = data;uint32_t *handles;int ret = 0;if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))return -ENODEV;if (args->flags != 0 && args->flags != DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL)return -EINVAL;if (args->count_handles == 0)return 0;Hmm, returning success without updating any of the status fields. -EINVAL? -ENOTHINGTODO.
Done, -EINVAL is its.
Otherwise I've pretty much done what Jason asked, gathered all the fences before waiting, and then waited,
Will resend new patch shortly.
Dave.