Re: [PATCH v5 09/14] drm/syncobj: Add deadline support for syncobj waits
On Wed, Feb 22, 2023 at 6:06 AM Rodrigo Vivi wrote:
>
> On Wed, Feb 22, 2023 at 12:09:04PM +0200, Pekka Paalanen wrote:
> > On Tue, 21 Feb 2023 09:25:18 -0800
> > Rob Clark wrote:
> >
> > > On Tue, Feb 21, 2023 at 12:53 AM Pekka Paalanen
> > > wrote:
> > > >
> > > > On Mon, 20 Feb 2023 12:18:56 -0800
> > > > Rob Clark wrote:
> > > >
> > > > > From: Rob Clark
> > > > >
> > > > > 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
> > > > >
> > > > > Signed-off-by: Rob Clark
> > > > > ---
> > > > > drivers/gpu/drm/drm_syncobj.c | 59
> > > > > +++
> > > > > include/uapi/drm/drm.h| 5 +++
> > > > > 2 files changed, 51 insertions(+), 13 deletions(-)
> > > >
> > > > ...
> > > >
> > > > > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > > > > index 642808520d92..aefc8cc743e0 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_nsec/sec */
> > > >
> > > > Hi,
> > > >
> > > > where is the UAPI documentation explaining what is a "fence deadline"
> > > > and what setting it does here?
> > >
> > > It's with the rest of the drm_syncobj UAPI docs ;-)
> >
> > Is that
> > https://www.kernel.org/doc/html/latest/driver-api/dma-buf.html#dma-fence-uabi-sync-file
> > ?
> >
> > That whole page never mentions e.g. WAIT_AVAILABLE, so at least the
> > flags are not there. Does not mention syncobj_wait either.
Sorry, that was a snarky reference to thin UABI docs about syncobj.
The kernel side of it is better documented.
> probably this:
> https://docs.kernel.org/gpu/drm-mm.html
>
> the new one needs to be added there as well.
>
> >
> > I could ask where the real non-IGT userspace is or the plan for it,
> > too, since this is new DRM UAPI.
>
> yeap, it looks like we need to close on this...
> https://gitlab.freedesktop.org/drm/intel/-/issues/8014
>
> I confess I got lost on the many discussions and on how this will
> be used. Is mesa going to set the deadline based on the vk priority?
>
> Will this continue to be internal? I didn't get the broader picture
> I'm afraid...
Yes, the plan is to use it from mesa vk-common helpers (and elsewhere
in mesa if needed). There is a separate discussion[1] about
limiting/allowing boost (perhaps based on ctx flag or cgroups) but
that is more about how drivers react to the deadline hint. The
immediate goal of this patch is just to fix the regression mentioned
in that gitlab issue when using syncobj waits instead of
DRM_IOCTL_I915_GEM_WAIT
BR,
-R
[1] https://www.spinics.net/lists/dri-devel/msg383075.html
> >
> >
> > Thanks,
> > pq
> >
> > >
> > > BR,
> > > -R
> > >
> > > > btw. no nsec/sec anymore.
> > > >
> > > >
> > > > Thanks,
> > > > pq
> > > >
> > > >
> > > > > struct drm_syncobj_wait {
> > > > > __u64 handles;
> > > > > /* absolute timeout */
> > > > > @@ -895,6 +896,8 @@ struct drm_syncobj_wait {
> > > > > __u32 flags;
> > > > > __u32 first_signaled; /* only valid when not waiting all */
> > > > > __u32 pad;
> > > > > + /* Deadline hint to set on backing fence(s) in CLOCK_MONOTONIC:
> > > > > */
> > > > > + __u64 deadline_ns;
> > > > > };
> > > > >
> > > > > struct drm_syncobj_timeline_wait {
> > > > > @@ -907,6 +910,8 @@ struct drm_syncobj_timeline_wait {
> > > > > __u32 flags;
> > > > > __u32 first_signaled; /* only valid when not waiting all */
> > > > > __u32 pad;
> > > > > + /* Deadline hint to set on backing fence(s) in CLOCK_MONOTONIC:
> > > > > */
> > > > > + __u64 deadline_ns;
> > > > > };
> > > > >
> > > > >
> > > >
> >
>
>
Re: [PATCH v5 09/14] drm/syncobj: Add deadline support for syncobj waits
On Wed, Feb 22, 2023 at 12:09:04PM +0200, Pekka Paalanen wrote:
> On Tue, 21 Feb 2023 09:25:18 -0800
> Rob Clark wrote:
>
> > On Tue, Feb 21, 2023 at 12:53 AM Pekka Paalanen wrote:
> > >
> > > On Mon, 20 Feb 2023 12:18:56 -0800
> > > Rob Clark wrote:
> > >
> > > > From: Rob Clark
> > > >
> > > > 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
> > > >
> > > > Signed-off-by: Rob Clark
> > > > ---
> > > > drivers/gpu/drm/drm_syncobj.c | 59 +++
> > > > include/uapi/drm/drm.h| 5 +++
> > > > 2 files changed, 51 insertions(+), 13 deletions(-)
> > >
> > > ...
> > >
> > > > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > > > index 642808520d92..aefc8cc743e0 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_nsec/sec */
> > >
> > > Hi,
> > >
> > > where is the UAPI documentation explaining what is a "fence deadline"
> > > and what setting it does here?
> >
> > It's with the rest of the drm_syncobj UAPI docs ;-)
>
> Is that
> https://www.kernel.org/doc/html/latest/driver-api/dma-buf.html#dma-fence-uabi-sync-file
> ?
>
> That whole page never mentions e.g. WAIT_AVAILABLE, so at least the
> flags are not there. Does not mention syncobj_wait either.
probably this:
https://docs.kernel.org/gpu/drm-mm.html
the new one needs to be added there as well.
>
> I could ask where the real non-IGT userspace is or the plan for it,
> too, since this is new DRM UAPI.
yeap, it looks like we need to close on this...
https://gitlab.freedesktop.org/drm/intel/-/issues/8014
I confess I got lost on the many discussions and on how this will
be used. Is mesa going to set the deadline based on the vk priority?
Will this continue to be internal? I didn't get the broader picture
I'm afraid...
>
>
> Thanks,
> pq
>
> >
> > BR,
> > -R
> >
> > > btw. no nsec/sec anymore.
> > >
> > >
> > > Thanks,
> > > pq
> > >
> > >
> > > > struct drm_syncobj_wait {
> > > > __u64 handles;
> > > > /* absolute timeout */
> > > > @@ -895,6 +896,8 @@ struct drm_syncobj_wait {
> > > > __u32 flags;
> > > > __u32 first_signaled; /* only valid when not waiting all */
> > > > __u32 pad;
> > > > + /* Deadline hint to set on backing fence(s) in CLOCK_MONOTONIC: */
> > > > + __u64 deadline_ns;
> > > > };
> > > >
> > > > struct drm_syncobj_timeline_wait {
> > > > @@ -907,6 +910,8 @@ struct drm_syncobj_timeline_wait {
> > > > __u32 flags;
> > > > __u32 first_signaled; /* only valid when not waiting all */
> > > > __u32 pad;
> > > > + /* Deadline hint to set on backing fence(s) in CLOCK_MONOTONIC: */
> > > > + __u64 deadline_ns;
> > > > };
> > > >
> > > >
> > >
>
Re: [PATCH v5 09/14] drm/syncobj: Add deadline support for syncobj waits
On Tue, 21 Feb 2023 09:25:18 -0800
Rob Clark wrote:
> On Tue, Feb 21, 2023 at 12:53 AM Pekka Paalanen wrote:
> >
> > On Mon, 20 Feb 2023 12:18:56 -0800
> > Rob Clark wrote:
> >
> > > From: Rob Clark
> > >
> > > 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
> > >
> > > Signed-off-by: Rob Clark
> > > ---
> > > drivers/gpu/drm/drm_syncobj.c | 59 +++
> > > include/uapi/drm/drm.h| 5 +++
> > > 2 files changed, 51 insertions(+), 13 deletions(-)
> >
> > ...
> >
> > > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > > index 642808520d92..aefc8cc743e0 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_nsec/sec */
> >
> > Hi,
> >
> > where is the UAPI documentation explaining what is a "fence deadline"
> > and what setting it does here?
>
> It's with the rest of the drm_syncobj UAPI docs ;-)
Is that
https://www.kernel.org/doc/html/latest/driver-api/dma-buf.html#dma-fence-uabi-sync-file
?
That whole page never mentions e.g. WAIT_AVAILABLE, so at least the
flags are not there. Does not mention syncobj_wait either.
I could ask where the real non-IGT userspace is or the plan for it,
too, since this is new DRM UAPI.
Thanks,
pq
>
> BR,
> -R
>
> > btw. no nsec/sec anymore.
> >
> >
> > Thanks,
> > pq
> >
> >
> > > struct drm_syncobj_wait {
> > > __u64 handles;
> > > /* absolute timeout */
> > > @@ -895,6 +896,8 @@ struct drm_syncobj_wait {
> > > __u32 flags;
> > > __u32 first_signaled; /* only valid when not waiting all */
> > > __u32 pad;
> > > + /* Deadline hint to set on backing fence(s) in CLOCK_MONOTONIC: */
> > > + __u64 deadline_ns;
> > > };
> > >
> > > struct drm_syncobj_timeline_wait {
> > > @@ -907,6 +910,8 @@ struct drm_syncobj_timeline_wait {
> > > __u32 flags;
> > > __u32 first_signaled; /* only valid when not waiting all */
> > > __u32 pad;
> > > + /* Deadline hint to set on backing fence(s) in CLOCK_MONOTONIC: */
> > > + __u64 deadline_ns;
> > > };
> > >
> > >
> >
pgpSS8tG5fe1y.pgp
Description: OpenPGP digital signature
Re: [PATCH v5 09/14] drm/syncobj: Add deadline support for syncobj waits
On Tue, Feb 21, 2023 at 12:53 AM Pekka Paalanen wrote:
>
> On Mon, 20 Feb 2023 12:18:56 -0800
> Rob Clark wrote:
>
> > From: Rob Clark
> >
> > 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
> >
> > Signed-off-by: Rob Clark
> > ---
> > drivers/gpu/drm/drm_syncobj.c | 59 +++
> > include/uapi/drm/drm.h| 5 +++
> > 2 files changed, 51 insertions(+), 13 deletions(-)
>
> ...
>
> > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > index 642808520d92..aefc8cc743e0 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_nsec/sec */
>
> Hi,
>
> where is the UAPI documentation explaining what is a "fence deadline"
> and what setting it does here?
It's with the rest of the drm_syncobj UAPI docs ;-)
BR,
-R
> btw. no nsec/sec anymore.
>
>
> Thanks,
> pq
>
>
> > struct drm_syncobj_wait {
> > __u64 handles;
> > /* absolute timeout */
> > @@ -895,6 +896,8 @@ struct drm_syncobj_wait {
> > __u32 flags;
> > __u32 first_signaled; /* only valid when not waiting all */
> > __u32 pad;
> > + /* Deadline hint to set on backing fence(s) in CLOCK_MONOTONIC: */
> > + __u64 deadline_ns;
> > };
> >
> > struct drm_syncobj_timeline_wait {
> > @@ -907,6 +910,8 @@ struct drm_syncobj_timeline_wait {
> > __u32 flags;
> > __u32 first_signaled; /* only valid when not waiting all */
> > __u32 pad;
> > + /* Deadline hint to set on backing fence(s) in CLOCK_MONOTONIC: */
> > + __u64 deadline_ns;
> > };
> >
> >
>
Re: [PATCH v5 09/14] drm/syncobj: Add deadline support for syncobj waits
On Mon, 20 Feb 2023 12:18:56 -0800
Rob Clark wrote:
> From: Rob Clark
>
> 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
>
> Signed-off-by: Rob Clark
> ---
> drivers/gpu/drm/drm_syncobj.c | 59 +++
> include/uapi/drm/drm.h| 5 +++
> 2 files changed, 51 insertions(+), 13 deletions(-)
...
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 642808520d92..aefc8cc743e0 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_nsec/sec */
Hi,
where is the UAPI documentation explaining what is a "fence deadline"
and what setting it does here?
btw. no nsec/sec anymore.
Thanks,
pq
> struct drm_syncobj_wait {
> __u64 handles;
> /* absolute timeout */
> @@ -895,6 +896,8 @@ struct drm_syncobj_wait {
> __u32 flags;
> __u32 first_signaled; /* only valid when not waiting all */
> __u32 pad;
> + /* Deadline hint to set on backing fence(s) in CLOCK_MONOTONIC: */
> + __u64 deadline_ns;
> };
>
> struct drm_syncobj_timeline_wait {
> @@ -907,6 +910,8 @@ struct drm_syncobj_timeline_wait {
> __u32 flags;
> __u32 first_signaled; /* only valid when not waiting all */
> __u32 pad;
> + /* Deadline hint to set on backing fence(s) in CLOCK_MONOTONIC: */
> + __u64 deadline_ns;
> };
>
>
pgpmPoYLmoGnf.pgp
Description: OpenPGP digital signature
[PATCH v5 09/14] drm/syncobj: Add deadline support for syncobj waits
From: Rob Clark
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
Signed-off-by: Rob Clark
---
drivers/gpu/drm/drm_syncobj.c | 59 +++
include/uapi/drm/drm.h| 5 +++
2 files changed, 51 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 0c2be8360525..4f9c3b3906f1 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -973,7 +973,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 +1054,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 +1161,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 +1173,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 +1184,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 +1256,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;
ret = drm_syncobj_array_find(file_private,
u64_to_user_ptr(args->handles),
@@ -1262,8 +1280,13 @@ drm_syncobj_wait_ioctl(struct drm_devic
