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

Dave Airlie airlied at gmail.com
Mon May 29 06:52:35 UTC 2017


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.


More information about the amd-gfx mailing list