[PATCH libdrm 4/5] wrap syncobj timeline query/wait APIs for amdgpu v3

[Christian König]

Am 30.11.18 um 08:35 schrieb zhoucm1:
>
>
> On 2018年11月28日 22:50, Christian König wrote:
>> From: Chunming Zhou <david1.zhou at amd.com>
>>
>> v2: symbos are stored in lexical order.
>> v3: drop export/import and extra query indirection
>>
>> Signed-off-by: Chunming Zhou <david1.zhou at amd.com>
>> Signed-off-by: Christian König <christian.koenig at amd.com>
>> ---
>>   amdgpu/amdgpu-symbol-check |  2 ++
>>   amdgpu/amdgpu.h            | 39 
>> +++++++++++++++++++++++++++++++++++++++
>>   amdgpu/amdgpu_cs.c         | 23 +++++++++++++++++++++++
>>   3 files changed, 64 insertions(+)
>>
>> diff --git a/amdgpu/amdgpu-symbol-check b/amdgpu/amdgpu-symbol-check
>> index 6f5e0f95..4553736f 100755
>> --- a/amdgpu/amdgpu-symbol-check
>> +++ b/amdgpu/amdgpu-symbol-check
>> @@ -49,8 +49,10 @@ amdgpu_cs_submit
>>   amdgpu_cs_submit_raw
>>   amdgpu_cs_syncobj_export_sync_file
>>   amdgpu_cs_syncobj_import_sync_file
>> +amdgpu_cs_syncobj_query
>>   amdgpu_cs_syncobj_reset
>>   amdgpu_cs_syncobj_signal
>> +amdgpu_cs_syncobj_timeline_wait
>>   amdgpu_cs_syncobj_wait
>>   amdgpu_cs_wait_fences
>>   amdgpu_cs_wait_semaphore
>> diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h
>> index dc51659a..330658a0 100644
>> --- a/amdgpu/amdgpu.h
>> +++ b/amdgpu/amdgpu.h
>> @@ -1489,6 +1489,45 @@ int 
>> amdgpu_cs_syncobj_wait(amdgpu_device_handle dev,
>>                  int64_t timeout_nsec, unsigned flags,
>>                  uint32_t *first_signaled);
>>   +/**
>> + *  Wait for one or all sync objects on their points to signal.
>> + *
>> + * \param   dev        - \c [in] self-explanatory
>> + * \param   handles - \c [in] array of sync object handles
>> + * \param   points - \c [in] array of sync points to wait
>> + * \param   num_handles - \c [in] self-explanatory
>> + * \param   timeout_nsec - \c [in] self-explanatory
>> + * \param   flags   - \c [in] a bitmask of DRM_SYNCOBJ_WAIT_FLAGS_*
>> + * \param   first_signaled - \c [in] self-explanatory
>> + *
>> + * \return   0 on success\n
>> + *          -ETIME - Timeout
>> + *          <0 - Negative POSIX Error code
>> + *
>> + */
>> +int amdgpu_cs_syncobj_timeline_wait(amdgpu_device_handle dev,
>> +                    uint32_t *handles, uint64_t *points,
>> +                    unsigned num_handles,
>> +                    int64_t timeout_nsec, unsigned flags,
>> +                    uint32_t *first_signaled);
>> +/**
>> + *  Query sync objects payloads.
>> + *
>> + * \param   dev        - \c [in] self-explanatory
>> + * \param   handles - \c [in] array of sync object handles
>> + * \param   points - \c [out] array of sync points returned, which 
>> presents
>> + * syncobj payload.
>> + * \param   num_handles - \c [in] self-explanatory
>> + *
>> + * \return   0 on success\n
>> + *          -ETIME - Timeout
>> + *          <0 - Negative POSIX Error code
>> + *
>> + */
>> +int amdgpu_cs_syncobj_query(amdgpu_device_handle dev,
>> +                uint32_t *handles, uint64_t *points,
>> +                unsigned num_handles);
>> +
>>   /**
>>    *  Export kernel sync object to shareable fd.
>>    *
>> diff --git a/amdgpu/amdgpu_cs.c b/amdgpu/amdgpu_cs.c
>> index 3b8231aa..e4a547c6 100644
>> --- a/amdgpu/amdgpu_cs.c
>> +++ b/amdgpu/amdgpu_cs.c
>> @@ -661,6 +661,29 @@ drm_public int 
>> amdgpu_cs_syncobj_wait(amdgpu_device_handle dev,
>>                     flags, first_signaled);
>>   }
>>   +drm_public int 
>> amdgpu_cs_syncobj_timeline_wait(amdgpu_device_handle dev,
>> +                           uint32_t *handles, uint64_t *points,
>> +                           unsigned num_handles,
>> +                           int64_t timeout_nsec, unsigned flags,
>> +                           uint32_t *first_signaled)
>> +{
>> +    if (NULL == dev)
>> +        return -EINVAL;
>> +
>> +    return drmSyncobjTimelineWait(dev->fd, handles, points, 
>> num_handles,
>> +                      timeout_nsec, flags, first_signaled);
>> +}
>> +
>> +drm_public int amdgpu_cs_syncobj_query(amdgpu_device_handle dev,
>> +                       uint32_t *handles, uint64_t *points,
> This interfaces is public to umd, I think they like "uint64_t 
> **points" for batch query, I've verified before, it works well and 
> more convenience.
> If removing num_handles, that means only one syncobj to query, I agree 
> with "uint64_t *point".

"handles" as well as "points" are an array of objects. If the UMD wants 
to write the points to separate locations it can do so manually after 
calling the function.

It doesn't make any sense that libdrm or the kernel does the extra 
indirection, the transferred pointers are 64bit as well (even on a 32bit 
system) so the overhead is identical.

Adding another indirection just makes the implementation unnecessary 
complex.

Christian.

>
> -David
>> +                       unsigned num_handles)
>> +{
>> +    if (NULL == dev)
>> +        return -EINVAL;
>> +
>> +    return drmSyncobjQuery(dev->fd, handles, points, num_handles);
>> +}
>> +
>>   drm_public int amdgpu_cs_export_syncobj(amdgpu_device_handle dev,
>>                       uint32_t handle,
>>                       int *shared_fd)
>