On Tue, 21 Feb 2023 09:25:18 -0800
Rob Clark <[email protected]> wrote:

> On Tue, Feb 21, 2023 at 12:53 AM Pekka Paalanen <[email protected]> wrote:
> >
> > On Mon, 20 Feb 2023 12:18:56 -0800
> > Rob Clark <[email protected]> wrote:
> >
> > > From: Rob Clark <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > > 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;
> > > };
> > >
> > >
> >

Attachments:

(No filename) (833.00 B)
OpenPGP digital signature

2023-02-22 14:06:34

by Rodrigo Vivi

[permalink] [raw]

Subject: 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 <[email protected]> wrote:
>
> > On Tue, Feb 21, 2023 at 12:53 AM Pekka Paalanen <[email protected]> wrote:
> > >
> > > On Mon, 20 Feb 2023 12:18:56 -0800
> > > Rob Clark <[email protected]> wrote:
> > >
> > > > From: Rob Clark <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > > > ---
> > > > 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;
> > > > };
> > > >
> > > >
> > >
>

2023-02-22 15:34:29

by Rob Clark

[permalink] [raw]

Subject: Re: [PATCH v5 09/14] drm/syncobj: Add deadline support for syncobj waits

On Wed, Feb 22, 2023 at 6:06 AM Rodrigo Vivi <[email protected]> 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 <[email protected]> wrote:
> >
> > > On Tue, Feb 21, 2023 at 12:53 AM Pekka Paalanen <[email protected]> wrote:
> > > >
> > > > On Mon, 20 Feb 2023 12:18:56 -0800
> > > > Rob Clark <[email protected]> wrote:
> > > >
> > > > > From: Rob Clark <[email protected]>
> > > > >
> > > > > 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 <[email protected]>
> > > > > ---
> > > > > 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;
> > > > > };
> > > > >
> > > > >
> > > >
> >
>
>