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.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
