[PATCH 2/5] drm/syncobj: add sync obj wait interface. (v3)

[Dave Airlie]

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.