Re: [PATCH] drm: Add crtc_queue_syncobj and crtc_get_syncobj ioctls

[Keith Packard]

Jason Ekstrand  writes:

> Yeah, I suppose an application could ask for 10k frames in the future or
> something ridiculous like that.  For sync_file, people strongly want a
> finite time guarantee for security/deadlock reasons.  I don't know how
> happy they would be with a finite time of 10 minutes...

Sure, we've put arbitrary finite limits on vblank counters in other
places, so adding some kind of arbitrary limit like a couple of seconds
would be entirely reasonable here. The Vulkan API doesn't need any of
this complexity at all; the one I'm doing only lets you create a fence
for the next vblank.

> Ok, that's not expected... Part of the point of sync objects is that
> they're re-usable.  The client is free to not re-use them or to be very
> careful about the recycling process.

Heh. I thought the opposite -- lightweight objects that you'd use once
and delete. I can certainly change the API to pass in an existing
syncobj rather than creating a new one. That would be easier in some
ways as it reduces the failure paths a bit.

> Is the event the important part or the moderately accurate timestamp?

I need the timestamp and sequence number, getting them in an event would
mean not having to make another syscall.

> We could probably modify drm_syncobj to have a "last signaled"
> timestamp that's queryable through an IOCTL.

That's exactly what I did, but it only works for these new syncobjs. The
fields are global and could be filled in by other bits of the code.

> Is the sequence number returned necessary?  Will it ever be different from
> the one requested?

Yes, when the application queues it just slightly too late.

The application doesn't actually need either of these values directly,
but the system needs them to respond to requests to queue presentation
at a specific time, so the vulkan driver wants 'recent' vblank
time/sequence data to estimate when a vblank will occur.

-- 
-keith


signature.asc
Description: PGP signature

Re: [PATCH] drm: Add crtc_queue_syncobj and crtc_get_syncobj ioctls

[Keith Packard]

Jason Ekstrand  writes:

> Yeah, I suppose an application could ask for 10k frames in the future or
> something ridiculous like that.  For sync_file, people strongly want a
> finite time guarantee for security/deadlock reasons.  I don't know how
> happy they would be with a finite time of 10 minutes...

Sure, we've put arbitrary finite limits on vblank counters in other
places, so adding some kind of arbitrary limit like a couple of seconds
would be entirely reasonable here. The Vulkan API doesn't need any of
this complexity at all; the one I'm doing only lets you create a fence
for the next vblank.

> Ok, that's not expected... Part of the point of sync objects is that
> they're re-usable.  The client is free to not re-use them or to be very
> careful about the recycling process.

Heh. I thought the opposite -- lightweight objects that you'd use once
and delete. I can certainly change the API to pass in an existing
syncobj rather than creating a new one. That would be easier in some
ways as it reduces the failure paths a bit.

> Is the event the important part or the moderately accurate timestamp?

I need the timestamp and sequence number, getting them in an event would
mean not having to make another syscall.

> We could probably modify drm_syncobj to have a "last signaled"
> timestamp that's queryable through an IOCTL.

That's exactly what I did, but it only works for these new syncobjs. The
fields are global and could be filled in by other bits of the code.

> Is the sequence number returned necessary?  Will it ever be different from
> the one requested?

Yes, when the application queues it just slightly too late.

The application doesn't actually need either of these values directly,
but the system needs them to respond to requests to queue presentation
at a specific time, so the vulkan driver wants 'recent' vblank
time/sequence data to estimate when a vblank will occur.

-- 
-keith


signature.asc
Description: PGP signature

Re: [PATCH] drm: Add crtc_queue_syncobj and crtc_get_syncobj ioctls

[Keith Packard]

Jason Ekstrand  writes:

> Is the given sequence number guaranteed to be hit in finite time?

Certainly, it's a finite value...

However, realistically, it's just like all of the other vblank
interfaces where you can specify a crazy sequence and block for a very
long time. So, no different from the current situation.

Of course, the only vulkan API available today only lets you wait for
the next vblank, so we'll be asking for a sequence which is one more
than the current sequence.

> Just to make sure I've got this straight, the client queues a syncobj with
> queue_syncobj to fire at a given sequence number.  Once the syncobj has
> fired (which it can find out by waiting on it), it then calls get_syncobj
> to figure out when it was fired?

If it cares, it can ask. It might not care at all, in which case it
wouldn't have to ask. The syncobj API doesn't provide any direct
information about the state of the object in the wait call.

> If so, what happens if a syncobj is re-used?  Do you just loose the
> information?

You can't reuse one of these -- the 'queue_syncobj' API creates a new
one each time.

> If we have a finite time guarantee, I'm kind-of thinking a sync_file might
> be better as it's a one-shot and doesn't have the information loss
> problem.  I'm not actually sure though.

It's a one-shot, once signaled, you're done with it.

> This whole "wait for a syncobj and then ask when it fired" thing is a bit
> odd.  I'll have to think on it.

Yeah, the event mechanism has the nice feature that you get data with
each event. I guess the alternative would be to have a way to get an
event when a sync object was ready; perhaps a call which provided a set
of syncobjs and delivered a DRM event when any of them was ready?

That has a lot of appeal; it turns the poll-like nature of the current
API into an epoll-like system. Hrm.

-- 
-keith


signature.asc
Description: PGP signature

Re: [PATCH] drm: Add crtc_queue_syncobj and crtc_get_syncobj ioctls

[Keith Packard]

Jason Ekstrand  writes:

> Is the given sequence number guaranteed to be hit in finite time?

Certainly, it's a finite value...

However, realistically, it's just like all of the other vblank
interfaces where you can specify a crazy sequence and block for a very
long time. So, no different from the current situation.

Of course, the only vulkan API available today only lets you wait for
the next vblank, so we'll be asking for a sequence which is one more
than the current sequence.

> Just to make sure I've got this straight, the client queues a syncobj with
> queue_syncobj to fire at a given sequence number.  Once the syncobj has
> fired (which it can find out by waiting on it), it then calls get_syncobj
> to figure out when it was fired?

If it cares, it can ask. It might not care at all, in which case it
wouldn't have to ask. The syncobj API doesn't provide any direct
information about the state of the object in the wait call.

> If so, what happens if a syncobj is re-used?  Do you just loose the
> information?

You can't reuse one of these -- the 'queue_syncobj' API creates a new
one each time.

> If we have a finite time guarantee, I'm kind-of thinking a sync_file might
> be better as it's a one-shot and doesn't have the information loss
> problem.  I'm not actually sure though.

It's a one-shot, once signaled, you're done with it.

> This whole "wait for a syncobj and then ask when it fired" thing is a bit
> odd.  I'll have to think on it.

Yeah, the event mechanism has the nice feature that you get data with
each event. I guess the alternative would be to have a way to get an
event when a sync object was ready; perhaps a call which provided a set
of syncobjs and delivered a DRM event when any of them was ready?

That has a lot of appeal; it turns the poll-like nature of the current
API into an epoll-like system. Hrm.

-- 
-keith


signature.asc
Description: PGP signature

[PATCH] drm: Add crtc_queue_syncobj and crtc_get_syncobj ioctls

[Keith Packard]

crtc_queue_syncobj creates a new syncobj that will get signaled at a
specified vblank sequence count.

crtc_get_syncobj returns the time and vblank sequence count when the
syncobj was signaled.

The pair of these allows use of syncobjs instead of events for
monitoring vblank activity.

Signed-off-by: Keith Packard 
---
 drivers/gpu/drm/drm_file.c |   5 +
 drivers/gpu/drm/drm_internal.h |   4 +
 drivers/gpu/drm/drm_ioctl.c|   2 +
 drivers/gpu/drm/drm_syncobj.c  |  13 ++-
 drivers/gpu/drm/drm_vblank.c   | 212 +
 include/drm/drm_file.h |  11 ++-
 include/drm/drm_syncobj.h  |  13 +++
 include/uapi/drm/drm.h |  17 
 8 files changed, 253 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index b3c6e997ccdb..c1ada3fe70b0 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -37,6 +37,7 @@
 
 #include 
 #include 
+#include 
 
 #include "drm_legacy.h"
 #include "drm_internal.h"
@@ -711,6 +712,10 @@ void drm_send_event_locked(struct drm_device *dev, struct 
drm_pending_event *e)
dma_fence_put(e->fence);
}
 
+   if (e->syncobj) {
+   drm_syncobj_put(e->syncobj);
+   }
+
if (!e->file_priv) {
kfree(e);
return;
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index c9d5a6cd4d41..71b9435b5b37 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -75,6 +75,10 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void 
*data,
 
 int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data,
  struct drm_file *filp);
+int drm_crtc_queue_syncobj_ioctl(struct drm_device *dev, void *data,
+struct drm_file *filp);
+int drm_crtc_get_syncobj_ioctl(struct drm_device *dev, void *data,
+struct drm_file *filp);
 
 /* drm_auth.c */
 int drm_getmagic(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 4aafe4802099..309611fb5d0d 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -665,6 +665,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
  DRM_UNLOCKED|DRM_RENDER_ALLOW),
DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE, drm_crtc_get_sequence_ioctl, 
DRM_UNLOCKED),
DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE, 
drm_crtc_queue_sequence_ioctl, DRM_UNLOCKED),
+   DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SYNCOBJ, 
drm_crtc_queue_syncobj_ioctl, DRM_UNLOCKED),
+   DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SYNCOBJ, drm_crtc_get_syncobj_ioctl, 
DRM_UNLOCKED),
DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl, 
DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
DRM_IOCTL_DEF(DRM_IOCTL_MODE_LIST_LESSEES, drm_mode_list_lessees_ioctl, 
DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
DRM_IOCTL_DEF(DRM_IOCTL_MODE_GET_LEASE, drm_mode_get_lease_ioctl, 
DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index cb4d09c70fd4..e197b007079d 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -208,7 +208,7 @@ static const struct dma_fence_ops 
drm_syncobj_null_fence_ops = {
.release = NULL,
 };
 
-static int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj)
+int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj, bool signal)
 {
struct drm_syncobj_null_fence *fence;
fence = kzalloc(sizeof(*fence), GFP_KERNEL);
@@ -218,7 +218,8 @@ static int drm_syncobj_assign_null_handle(struct 
drm_syncobj *syncobj)
spin_lock_init(>lock);
dma_fence_init(>base, _syncobj_null_fence_ops,
   >lock, 0, 0);
-   dma_fence_signal(>base);
+   if (signal)
+   dma_fence_signal(>base);
 
drm_syncobj_replace_fence(syncobj, >base);
 
@@ -226,6 +227,7 @@ static int drm_syncobj_assign_null_handle(struct 
drm_syncobj *syncobj)
 
return 0;
 }
+EXPORT_SYMBOL(drm_syncobj_assign_null_handle);
 
 int drm_syncobj_find_fence(struct drm_file *file_private,
   u32 handle,
@@ -283,7 +285,7 @@ int drm_syncobj_create(struct drm_syncobj **out_syncobj, 
uint32_t flags,
spin_lock_init(>lock);
 
if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) {
-   ret = drm_syncobj_assign_null_handle(syncobj);
+   ret = drm_syncobj_assign_null_handle(syncobj, true);
if (ret < 0) {
drm_syncobj_put(syncobj);
return ret;
@@ -341,7 +343,7 @@ static int drm_syncobj_create_as_handle(struct drm_file 
*file_private,
return ret;
 }
 
-static int drm_syncobj_destroy(struct drm_file *file_private,
+int drm_syncobj_destroy(struct drm_file 

[PATCH] drm: Add crtc_queue_syncobj and crtc_get_syncobj ioctls

[Keith Packard]

crtc_queue_syncobj creates a new syncobj that will get signaled at a
specified vblank sequence count.

crtc_get_syncobj returns the time and vblank sequence count when the
syncobj was signaled.

The pair of these allows use of syncobjs instead of events for
monitoring vblank activity.

Signed-off-by: Keith Packard 
---
 drivers/gpu/drm/drm_file.c |   5 +
 drivers/gpu/drm/drm_internal.h |   4 +
 drivers/gpu/drm/drm_ioctl.c|   2 +
 drivers/gpu/drm/drm_syncobj.c  |  13 ++-
 drivers/gpu/drm/drm_vblank.c   | 212 +
 include/drm/drm_file.h |  11 ++-
 include/drm/drm_syncobj.h  |  13 +++
 include/uapi/drm/drm.h |  17 
 8 files changed, 253 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index b3c6e997ccdb..c1ada3fe70b0 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -37,6 +37,7 @@
 
 #include 
 #include 
+#include 
 
 #include "drm_legacy.h"
 #include "drm_internal.h"
@@ -711,6 +712,10 @@ void drm_send_event_locked(struct drm_device *dev, struct 
drm_pending_event *e)
dma_fence_put(e->fence);
}
 
+   if (e->syncobj) {
+   drm_syncobj_put(e->syncobj);
+   }
+
if (!e->file_priv) {
kfree(e);
return;
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index c9d5a6cd4d41..71b9435b5b37 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -75,6 +75,10 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void 
*data,
 
 int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data,
  struct drm_file *filp);
+int drm_crtc_queue_syncobj_ioctl(struct drm_device *dev, void *data,
+struct drm_file *filp);
+int drm_crtc_get_syncobj_ioctl(struct drm_device *dev, void *data,
+struct drm_file *filp);
 
 /* drm_auth.c */
 int drm_getmagic(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 4aafe4802099..309611fb5d0d 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -665,6 +665,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
  DRM_UNLOCKED|DRM_RENDER_ALLOW),
DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE, drm_crtc_get_sequence_ioctl, 
DRM_UNLOCKED),
DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE, 
drm_crtc_queue_sequence_ioctl, DRM_UNLOCKED),
+   DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SYNCOBJ, 
drm_crtc_queue_syncobj_ioctl, DRM_UNLOCKED),
+   DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SYNCOBJ, drm_crtc_get_syncobj_ioctl, 
DRM_UNLOCKED),
DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl, 
DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
DRM_IOCTL_DEF(DRM_IOCTL_MODE_LIST_LESSEES, drm_mode_list_lessees_ioctl, 
DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
DRM_IOCTL_DEF(DRM_IOCTL_MODE_GET_LEASE, drm_mode_get_lease_ioctl, 
DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index cb4d09c70fd4..e197b007079d 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -208,7 +208,7 @@ static const struct dma_fence_ops 
drm_syncobj_null_fence_ops = {
.release = NULL,
 };
 
-static int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj)
+int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj, bool signal)
 {
struct drm_syncobj_null_fence *fence;
fence = kzalloc(sizeof(*fence), GFP_KERNEL);
@@ -218,7 +218,8 @@ static int drm_syncobj_assign_null_handle(struct 
drm_syncobj *syncobj)
spin_lock_init(>lock);
dma_fence_init(>base, _syncobj_null_fence_ops,
   >lock, 0, 0);
-   dma_fence_signal(>base);
+   if (signal)
+   dma_fence_signal(>base);
 
drm_syncobj_replace_fence(syncobj, >base);
 
@@ -226,6 +227,7 @@ static int drm_syncobj_assign_null_handle(struct 
drm_syncobj *syncobj)
 
return 0;
 }
+EXPORT_SYMBOL(drm_syncobj_assign_null_handle);
 
 int drm_syncobj_find_fence(struct drm_file *file_private,
   u32 handle,
@@ -283,7 +285,7 @@ int drm_syncobj_create(struct drm_syncobj **out_syncobj, 
uint32_t flags,
spin_lock_init(>lock);
 
if (flags & DRM_SYNCOBJ_CREATE_SIGNALED) {
-   ret = drm_syncobj_assign_null_handle(syncobj);
+   ret = drm_syncobj_assign_null_handle(syncobj, true);
if (ret < 0) {
drm_syncobj_put(syncobj);
return ret;
@@ -341,7 +343,7 @@ static int drm_syncobj_create_as_handle(struct drm_file 
*file_private,
return ret;
 }
 
-static int drm_syncobj_destroy(struct drm_file *file_private,
+int drm_syncobj_destroy(struct drm_file *file_private,