On Fri, Mar 17, 2023 at 12:08 PM Faith Ekstrand <fa...@gfxstrand.net> wrote:
>
>
> On Wed, Mar 8, 2023 at 9:54 AM Rob Clark <robdcl...@gmail.com> wrote:
>>
>> From: Rob Clark <robdcl...@chromium.org>
>>
>> Add a new flag to let userspace provide a deadline as a hint for syncobj
>> and timeline waits.  This gives a hint to the driver signaling the
>> backing fences about how soon userspace needs it to compete work, so it
>> can addjust GPU frequency accordingly.  An immediate deadline can be
>> given to provide something equivalent to i915 "wait boost".
>>
>> v2: Use absolute u64 ns value for deadline hint, drop cap and driver
>>     feature flag in favor of allowing count_handles==0 as a way for
>>     userspace to probe kernel for support of new flag
>> v3: More verbose comments about UAPI
>>
>> Signed-off-by: Rob Clark <robdcl...@chromium.org>
>> ---
>>  drivers/gpu/drm/drm_syncobj.c | 64 ++++++++++++++++++++++++++++-------
>>  include/uapi/drm/drm.h        | 17 ++++++++++
>>  2 files changed, 68 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
>> index 0c2be8360525..a85e9464f07b 100644
>> --- a/drivers/gpu/drm/drm_syncobj.c
>> +++ b/drivers/gpu/drm/drm_syncobj.c
>> @@ -126,6 +126,11 @@
>>   * synchronize between the two.
>>   * This requirement is inherited from the Vulkan fence API.
>>   *
>> + * If &DRM_SYNCOBJ_WAIT_FLAGS_WAIT_DEADLINE is set, the ioctl will also set
>> + * a fence deadline hint on the backing fences before waiting, to provide 
>> the
>> + * fence signaler with an appropriate sense of urgency.  The deadline is
>> + * specified as an absolute &CLOCK_MONOTONIC value in units of ns.
>> + *
>>   * Similarly, &DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT takes an array of syncobj
>>   * handles as well as an array of u64 points and does a host-side wait on 
>> all
>>   * of syncobj fences at the given points simultaneously.
>> @@ -973,7 +978,8 @@ static signed long drm_syncobj_array_wait_timeout(struct 
>> drm_syncobj **syncobjs,
>>                                                   uint32_t count,
>>                                                   uint32_t flags,
>>                                                   signed long timeout,
>> -                                                 uint32_t *idx)
>> +                                                 uint32_t *idx,
>> +                                                 ktime_t *deadline)
>>  {
>>         struct syncobj_wait_entry *entries;
>>         struct dma_fence *fence;
>> @@ -1053,6 +1059,15 @@ static signed long 
>> drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
>>                         drm_syncobj_fence_add_wait(syncobjs[i], &entries[i]);
>>         }
>>
>> +       if (deadline) {
>> +               for (i = 0; i < count; ++i) {
>> +                       fence = entries[i].fence;
>> +                       if (!fence)
>> +                               continue;
>> +                       dma_fence_set_deadline(fence, *deadline);
>> +               }
>> +       }
>> +
>>         do {
>>                 set_current_state(TASK_INTERRUPTIBLE);
>>
>> @@ -1151,7 +1166,8 @@ static int drm_syncobj_array_wait(struct drm_device 
>> *dev,
>>                                   struct drm_file *file_private,
>>                                   struct drm_syncobj_wait *wait,
>>                                   struct drm_syncobj_timeline_wait 
>> *timeline_wait,
>> -                                 struct drm_syncobj **syncobjs, bool 
>> timeline)
>> +                                 struct drm_syncobj **syncobjs, bool 
>> timeline,
>> +                                 ktime_t *deadline)
>>  {
>>         signed long timeout = 0;
>>         uint32_t first = ~0;
>> @@ -1162,7 +1178,8 @@ static int drm_syncobj_array_wait(struct drm_device 
>> *dev,
>>                                                          NULL,
>>                                                          wait->count_handles,
>>                                                          wait->flags,
>> -                                                        timeout, &first);
>> +                                                        timeout, &first,
>> +                                                        deadline);
>>                 if (timeout < 0)
>>                         return timeout;
>>                 wait->first_signaled = first;
>> @@ -1172,7 +1189,8 @@ static int drm_syncobj_array_wait(struct drm_device 
>> *dev,
>>                                                          
>> u64_to_user_ptr(timeline_wait->points),
>>                                                          
>> timeline_wait->count_handles,
>>                                                          
>> timeline_wait->flags,
>> -                                                        timeout, &first);
>> +                                                        timeout, &first,
>> +                                                        deadline);
>>                 if (timeout < 0)
>>                         return timeout;
>>                 timeline_wait->first_signaled = first;
>> @@ -1243,17 +1261,22 @@ drm_syncobj_wait_ioctl(struct drm_device *dev, void 
>> *data,
>>  {
>>         struct drm_syncobj_wait *args = data;
>>         struct drm_syncobj **syncobjs;
>> +       unsigned possible_flags;
>> +       ktime_t t, *tp = NULL;
>>         int ret = 0;
>>
>>         if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
>>                 return -EOPNOTSUPP;
>>
>> -       if (args->flags & ~(DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL |
>> -                           DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT))
>> +       possible_flags = DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL |
>> +                        DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT |
>> +                        DRM_SYNCOBJ_WAIT_FLAGS_WAIT_DEADLINE;
>> +
>> +       if (args->flags & ~possible_flags)
>>                 return -EINVAL;
>>
>>         if (args->count_handles == 0)
>> -               return -EINVAL;
>> +               return 0;
>
>
> Did you intend this change? If so, why? What does waiting with no handles 
> gain us? I mean, it's probably fine but it seems unrelated to this change.

Yes, it was intentional.. it gives userspace a way to probe whether
the kernel supports the new flag..

BR,
-R

>>         ret = drm_syncobj_array_find(file_private,
>>                                      u64_to_user_ptr(args->handles),
>> @@ -1262,8 +1285,13 @@ drm_syncobj_wait_ioctl(struct drm_device *dev, void 
>> *data,
>>         if (ret < 0)
>>                 return ret;
>>
>> +       if (args->flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_DEADLINE) {
>> +               t = ns_to_ktime(args->deadline_ns);
>> +               tp = &t;
>> +       }
>> +
>>         ret = drm_syncobj_array_wait(dev, file_private,
>> -                                    args, NULL, syncobjs, false);
>> +                                    args, NULL, syncobjs, false, tp);
>>
>>         drm_syncobj_array_free(syncobjs, args->count_handles);
>>
>> @@ -1276,18 +1304,23 @@ drm_syncobj_timeline_wait_ioctl(struct drm_device 
>> *dev, void *data,
>>  {
>>         struct drm_syncobj_timeline_wait *args = data;
>>         struct drm_syncobj **syncobjs;
>> +       unsigned possible_flags;
>> +       ktime_t t, *tp = NULL;
>>         int ret = 0;
>>
>>         if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE))
>>                 return -EOPNOTSUPP;
>>
>> -       if (args->flags & ~(DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL |
>> -                           DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT |
>> -                           DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE))
>> +       possible_flags = DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL |
>> +                        DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT |
>> +                        DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE |
>> +                        DRM_SYNCOBJ_WAIT_FLAGS_WAIT_DEADLINE;
>> +
>> +       if (args->flags & ~possible_flags)
>>                 return -EINVAL;
>>
>>         if (args->count_handles == 0)
>> -               return -EINVAL;
>> +               return -0;
>
>
> Did you intend this change? -0 is a pretty weird integer.
>
>>
>>         ret = drm_syncobj_array_find(file_private,
>>                                      u64_to_user_ptr(args->handles),
>> @@ -1296,8 +1329,13 @@ drm_syncobj_timeline_wait_ioctl(struct drm_device 
>> *dev, void *data,
>>         if (ret < 0)
>>                 return ret;
>>
>> +       if (args->flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_DEADLINE) {
>> +               t = ns_to_ktime(args->deadline_ns);
>> +               tp = &t;
>> +       }
>> +
>>         ret = drm_syncobj_array_wait(dev, file_private,
>> -                                    NULL, args, syncobjs, true);
>> +                                    NULL, args, syncobjs, true, tp);
>>
>>         drm_syncobj_array_free(syncobjs, args->count_handles);
>>
>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>> index 642808520d92..bff0509ac8b6 100644
>> --- a/include/uapi/drm/drm.h
>> +++ b/include/uapi/drm/drm.h
>> @@ -887,6 +887,7 @@ struct drm_syncobj_transfer {
>>  #define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL (1 << 0)
>>  #define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT (1 << 1)
>>  #define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE (1 << 2) /* wait for time 
>> point to become available */
>> +#define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_DEADLINE (1 << 3) /* set fence deadline 
>> based to deadline_ns */
>>  struct drm_syncobj_wait {
>>         __u64 handles;
>>         /* absolute timeout */
>> @@ -895,6 +896,14 @@ struct drm_syncobj_wait {
>>         __u32 flags;
>>         __u32 first_signaled; /* only valid when not waiting all */
>>         __u32 pad;
>> +       /**
>> +        * @deadline_ns - fence deadline hint
>> +        *
>> +        * Deadline hint, in absolute CLOCK_MONOTONIC, to set on backing
>> +        * fence(s) if the DRM_SYNCOBJ_WAIT_FLAGS_WAIT_DEADLINE flag is
>> +        * set.
>> +        */
>> +       __u64 deadline_ns;
>>  };
>>
>>  struct drm_syncobj_timeline_wait {
>> @@ -907,6 +916,14 @@ struct drm_syncobj_timeline_wait {
>>         __u32 flags;
>>         __u32 first_signaled; /* only valid when not waiting all */
>>         __u32 pad;
>> +       /**
>> +        * @deadline_ns - fence deadline hint
>> +        *
>> +        * Deadline hint, in absolute CLOCK_MONOTONIC, to set on backing
>> +        * fence(s) if the DRM_SYNCOBJ_WAIT_FLAGS_WAIT_DEADLINE flag is
>> +        * set.
>> +        */
>> +       __u64 deadline_ns;
>>  };
>>
>>
>> --
>> 2.39.2
>>

Reply via email to