dma-buf: add new dma_fence_chain container v4

From: Christian König <[email protected]>

Implement finding the right timeline point in drm_syncobj_find_fence.

v2: return -EINVAL when the point is not submitted yet.
v3: fix reference counting bug, add flags handling as well

Signed-off-by Christian König <[email protected]>

---
 drivers/gpu/drm/drm_syncobj.c | 43 ++++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)

drivers/gpu/drm/drm_syncobj.c +23 -3

index 76ce13dafc4d..d964b348ecba 100644
@@ -231,16 +231,53 @@ int drm_syncobj_find_fence(struct drm_file *file_private,
                           struct dma_fence **fence)
 {
        struct drm_syncobj *syncobj = drm_syncobj_find(file_private, handle);
-       int ret = 0;
+       struct syncobj_wait_entry wait;
+       int ret;
 
        if (!syncobj)
                return -ENOENT;
 
        *fence = drm_syncobj_fence_get(syncobj);
-       if (!*fence) {
+       drm_syncobj_put(syncobj);
+
+       if (*fence) {
+               ret = dma_fence_chain_find_seqno(fence, point);
+               if (!ret)
+                       return 0;
+               dma_fence_put(*fence);
+       } else {
                ret = -EINVAL;
        }
-       drm_syncobj_put(syncobj);
+
+       if (!(flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT))
+               return ret;
+
+       memset(&wait, 0, sizeof(wait));
+       wait.task = current;
+       wait.point = point;
+       drm_syncobj_fence_add_wait(syncobj, &wait);
 
+
+       do {
+               set_current_state(TASK_INTERRUPTIBLE);
+               if (wait.fence) {
+                       ret = 0;

Quoting Chunming Zhou (2018-12-11 10:34:45)

Quoted 58 lines

From: Christian König <[email protected]>

Implement finding the right timeline point in drm_syncobj_find_fence.

v2: return -EINVAL when the point is not submitted yet.
v3: fix reference counting bug, add flags handling as well

Signed-off-by Christian König <[email protected]>

---
  drivers/gpu/drm/drm_syncobj.c | 43 ++++++++++++++++++++++++++++++++---
  1 file changed, 40 insertions(+), 3 deletions(-)

drivers/gpu/drm/drm_syncobj.c +28 -3

index 76ce13dafc4d..d964b348ecba 100644
@@ -231,16 +231,53 @@ int drm_syncobj_find_fence(struct drm_file *file_private,
                            struct dma_fence **fence)
  {
         struct drm_syncobj *syncobj = drm_syncobj_find(file_private, handle);
-       int ret = 0;
+       struct syncobj_wait_entry wait;
+       int ret;
 
         if (!syncobj)
                 return -ENOENT;
 
         *fence = drm_syncobj_fence_get(syncobj);
-       if (!*fence) {
+       drm_syncobj_put(syncobj);
+
+       if (*fence) {
+               ret = dma_fence_chain_find_seqno(fence, point);
+               if (!ret)
+                       return 0;
+               dma_fence_put(*fence);
+       } else {
                 ret = -EINVAL;
         }
-       drm_syncobj_put(syncobj);
+
+       if (!(flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT))
+               return ret;
+
+       memset(&wait, 0, sizeof(wait));
+       wait.task = current;
+       wait.point = point;
+       drm_syncobj_fence_add_wait(syncobj, &wait);
+
+       do {
+               set_current_state(TASK_INTERRUPTIBLE);
+               if (wait.fence) {
+                       ret = 0;
+                       break;
+               }
+
+               if (signal_pending(current)) {
+                       ret = -ERESTARTSYS;

drivers/gpu/drm/drm_syncobj.c int drm_syncobj_find_fence(struct drm_file *file_private, · line 273

Quoted 5 lines

drivers/gpu/drm/drm_syncobj.c +5 -0

+                       break;
+               }
+
+               schedule();
+       } while (1);

I've previously used a dma_fence_proxy so that we could do nonblocking waits on future submits. That would be preferrable (a requirement for our stupid BKL-driven code).

-Chris

Am 13.12.18 um 12:37 schrieb Chris Wilson:

Quoted 59 lines

Quoting Chunming Zhou (2018-12-11 10:34:45)

Quoted 58 lines

From: Christian König <[email protected]>

Implement finding the right timeline point in drm_syncobj_find_fence.

v2: return -EINVAL when the point is not submitted yet.
v3: fix reference counting bug, add flags handling as well

Signed-off-by Christian König <[email protected]>

---
  drivers/gpu/drm/drm_syncobj.c | 43 ++++++++++++++++++++++++++++++++---
  1 file changed, 40 insertions(+), 3 deletions(-)

drivers/gpu/drm/drm_syncobj.c +28 -3

index 76ce13dafc4d..d964b348ecba 100644
@@ -231,16 +231,53 @@ int drm_syncobj_find_fence(struct drm_file *file_private,
                            struct dma_fence **fence)
  {
         struct drm_syncobj *syncobj = drm_syncobj_find(file_private, handle);
-       int ret = 0;
+       struct syncobj_wait_entry wait;
+       int ret;
 
         if (!syncobj)
                 return -ENOENT;
 
         *fence = drm_syncobj_fence_get(syncobj);
-       if (!*fence) {
+       drm_syncobj_put(syncobj);
+
+       if (*fence) {
+               ret = dma_fence_chain_find_seqno(fence, point);
+               if (!ret)
+                       return 0;
+               dma_fence_put(*fence);
+       } else {
                 ret = -EINVAL;
         }
-       drm_syncobj_put(syncobj);
+
+       if (!(flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT))
+               return ret;
+
+       memset(&wait, 0, sizeof(wait));
+       wait.task = current;
+       wait.point = point;
+       drm_syncobj_fence_add_wait(syncobj, &wait);
+
+       do {
+               set_current_state(TASK_INTERRUPTIBLE);
+               if (wait.fence) {
+                       ret = 0;
+                       break;
+               }
+
+               if (signal_pending(current)) {
+                       ret = -ERESTARTSYS;

Quoted 8 lines

drivers/gpu/drm/drm_syncobj.c int drm_syncobj_find_fence(struct drm_file *file_private, · line 273

Quoted 5 lines

drivers/gpu/drm/drm_syncobj.c +5 -0

+                       break;
+               }
+
+               schedule();
+       } while (1);

I've previously used a dma_fence_proxy so that we could do nonblocking waits on future submits. That would be preferrable (a requirement for our stupid BKL-driven code).

That is exactly what I would definitely NAK.

I would rather say we should come up with a wait_multiple_events() macro and completely nuke the custom implementation of this in:
1. dma_fence_default_wait and dma_fence_wait_any_timeout
2. the radeon fence implementation
3. the nouveau fence implementation
4. the syncobj code

Cause all of them do exactly the same. The dma_fence implementation unfortunately came up with a custom event handling mechanism instead of extending the core Linux wait_event() system.

Quoting Koenig, Christian (2018-12-13 12:11:10)

Quoted 77 lines

Am 13.12.18 um 12:37 schrieb Chris Wilson:

Quoted 59 lines

Quoting Chunming Zhou (2018-12-11 10:34:45)

Quoted 58 lines

From: Christian König <[email protected]>

Implement finding the right timeline point in drm_syncobj_find_fence.

v2: return -EINVAL when the point is not submitted yet.
v3: fix reference counting bug, add flags handling as well

Signed-off-by Christian König <[email protected]>

---
   drivers/gpu/drm/drm_syncobj.c | 43 ++++++++++++++++++++++++++++++++---
   1 file changed, 40 insertions(+), 3 deletions(-)

drivers/gpu/drm/drm_syncobj.c +28 -3

index 76ce13dafc4d..d964b348ecba 100644
@@ -231,16 +231,53 @@ int drm_syncobj_find_fence(struct drm_file *file_private,
                             struct dma_fence **fence)
   {
          struct drm_syncobj *syncobj = drm_syncobj_find(file_private, handle);
-       int ret = 0;
+       struct syncobj_wait_entry wait;
+       int ret;
 
          if (!syncobj)
                  return -ENOENT;
 
          *fence = drm_syncobj_fence_get(syncobj);
-       if (!*fence) {
+       drm_syncobj_put(syncobj);
+
+       if (*fence) {
+               ret = dma_fence_chain_find_seqno(fence, point);
+               if (!ret)
+                       return 0;
+               dma_fence_put(*fence);
+       } else {
                  ret = -EINVAL;
          }
-       drm_syncobj_put(syncobj);
+
+       if (!(flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT))
+               return ret;
+
+       memset(&wait, 0, sizeof(wait));
+       wait.task = current;
+       wait.point = point;
+       drm_syncobj_fence_add_wait(syncobj, &wait);
+
+       do {
+               set_current_state(TASK_INTERRUPTIBLE);
+               if (wait.fence) {
+                       ret = 0;
+                       break;
+               }
+
+               if (signal_pending(current)) {
+                       ret = -ERESTARTSYS;

Quoted 8 lines

drivers/gpu/drm/drm_syncobj.c int drm_syncobj_find_fence(struct drm_file *file_private, · line 273

Quoted 5 lines

drivers/gpu/drm/drm_syncobj.c +5 -0

+                       break;
+               }
+
+               schedule();
+       } while (1);

I've previously used a dma_fence_proxy so that we could do nonblocking waits on future submits. That would be preferrable (a requirement for our stupid BKL-driven code).

That is exactly what I would definitely NAK.

I would rather say we should come up with a wait_multiple_events() macro and completely nuke the custom implementation of this in:
1. dma_fence_default_wait and dma_fence_wait_any_timeout
2. the radeon fence implementation
3. the nouveau fence implementation
4. the syncobj code

Quoted 3 lines

Cause all of them do exactly the same. The dma_fence implementation unfortunately came up with a custom event handling mechanism instead of extending the core Linux wait_event() system.

I don't want a blocking wait at all.

-Chris

Am 13.12.18 um 13:21 schrieb Chris Wilson:

Quoted 78 lines

Quoting Koenig, Christian (2018-12-13 12:11:10)

Quoted 77 lines

Am 13.12.18 um 12:37 schrieb Chris Wilson:

Quoted 59 lines

Quoting Chunming Zhou (2018-12-11 10:34:45)

Quoted 58 lines

From: Christian König <[email protected]>

Implement finding the right timeline point in drm_syncobj_find_fence.

v2: return -EINVAL when the point is not submitted yet.
v3: fix reference counting bug, add flags handling as well

Signed-off-by Christian König <[email protected]>

---
   drivers/gpu/drm/drm_syncobj.c | 43 ++++++++++++++++++++++++++++++++---
   1 file changed, 40 insertions(+), 3 deletions(-)

drivers/gpu/drm/drm_syncobj.c +28 -3

index 76ce13dafc4d..d964b348ecba 100644
@@ -231,16 +231,53 @@ int drm_syncobj_find_fence(struct drm_file *file_private,
                             struct dma_fence **fence)
   {
          struct drm_syncobj *syncobj = drm_syncobj_find(file_private, handle);
-       int ret = 0;
+       struct syncobj_wait_entry wait;
+       int ret;
 
          if (!syncobj)
                  return -ENOENT;
 
          *fence = drm_syncobj_fence_get(syncobj);
-       if (!*fence) {
+       drm_syncobj_put(syncobj);
+
+       if (*fence) {
+               ret = dma_fence_chain_find_seqno(fence, point);
+               if (!ret)
+                       return 0;
+               dma_fence_put(*fence);
+       } else {
                  ret = -EINVAL;
          }
-       drm_syncobj_put(syncobj);
+
+       if (!(flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT))
+               return ret;
+
+       memset(&wait, 0, sizeof(wait));
+       wait.task = current;
+       wait.point = point;
+       drm_syncobj_fence_add_wait(syncobj, &wait);
+
+       do {
+               set_current_state(TASK_INTERRUPTIBLE);
+               if (wait.fence) {
+                       ret = 0;
+                       break;
+               }
+
+               if (signal_pending(current)) {
+                       ret = -ERESTARTSYS;

Quoted 8 lines

drivers/gpu/drm/drm_syncobj.c int drm_syncobj_find_fence(struct drm_file *file_private, · line 273

Quoted 5 lines

drivers/gpu/drm/drm_syncobj.c +5 -0

+                       break;
+               }
+
+               schedule();
+       } while (1);

I've previously used a dma_fence_proxy so that we could do nonblocking waits on future submits. That would be preferrable (a requirement for our stupid BKL-driven code).

That is exactly what I would definitely NAK.

I would rather say we should come up with a wait_multiple_events() macro and completely nuke the custom implementation of this in:
1. dma_fence_default_wait and dma_fence_wait_any_timeout
2. the radeon fence implementation
3. the nouveau fence implementation
4. the syncobj code

Quoted 4 lines

Quoted 3 lines

Cause all of them do exactly the same. The dma_fence implementation unfortunately came up with a custom event handling mechanism instead of extending the core Linux wait_event() system.

I don't want a blocking wait at all.

Ok I wasn't clear enough :) That is exactly what I would NAK!

The wait must be blocking or otherwise you would allow wait-before-signal.

Christian.

Quoted 1 line

-Chris

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 79 lines

Am 13.12.18 um 13:21 schrieb Chris Wilson:

Quoted 78 lines

Quoting Koenig, Christian (2018-12-13 12:11:10)

Quoted 77 lines

Am 13.12.18 um 12:37 schrieb Chris Wilson:

Quoted 59 lines

Quoting Chunming Zhou (2018-12-11 10:34:45)

Quoted 58 lines

From: Christian König <[email protected]>

Implement finding the right timeline point in drm_syncobj_find_fence.

v2: return -EINVAL when the point is not submitted yet.
v3: fix reference counting bug, add flags handling as well

Signed-off-by Christian König <[email protected]>

---
    drivers/gpu/drm/drm_syncobj.c | 43 ++++++++++++++++++++++++++++++++---
    1 file changed, 40 insertions(+), 3 deletions(-)

drivers/gpu/drm/drm_syncobj.c +28 -3

index 76ce13dafc4d..d964b348ecba 100644
@@ -231,16 +231,53 @@ int drm_syncobj_find_fence(struct drm_file *file_private,
                              struct dma_fence **fence)
    {
           struct drm_syncobj *syncobj = drm_syncobj_find(file_private, handle);
-       int ret = 0;
+       struct syncobj_wait_entry wait;
+       int ret;
 
           if (!syncobj)
                   return -ENOENT;
 
           *fence = drm_syncobj_fence_get(syncobj);
-       if (!*fence) {
+       drm_syncobj_put(syncobj);
+
+       if (*fence) {
+               ret = dma_fence_chain_find_seqno(fence, point);
+               if (!ret)
+                       return 0;
+               dma_fence_put(*fence);
+       } else {
                   ret = -EINVAL;
           }
-       drm_syncobj_put(syncobj);
+
+       if (!(flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT))
+               return ret;
+
+       memset(&wait, 0, sizeof(wait));
+       wait.task = current;
+       wait.point = point;
+       drm_syncobj_fence_add_wait(syncobj, &wait);
+
+       do {
+               set_current_state(TASK_INTERRUPTIBLE);
+               if (wait.fence) {
+                       ret = 0;
+                       break;
+               }
+
+               if (signal_pending(current)) {
+                       ret = -ERESTARTSYS;

Quoted 8 lines

drivers/gpu/drm/drm_syncobj.c int drm_syncobj_find_fence(struct drm_file *file_private, · line 273

Quoted 5 lines

drivers/gpu/drm/drm_syncobj.c +5 -0

+                       break;
+               }
+
+               schedule();
+       } while (1);

I've previously used a dma_fence_proxy so that we could do nonblocking waits on future submits. That would be preferrable (a requirement for our stupid BKL-driven code).

That is exactly what I would definitely NAK.

I would rather say we should come up with a wait_multiple_events() macro and completely nuke the custom implementation of this in:
1. dma_fence_default_wait and dma_fence_wait_any_timeout
2. the radeon fence implementation
3. the nouveau fence implementation
4. the syncobj code

Quoted 7 lines

Quoted 4 lines

Quoted 3 lines

Cause all of them do exactly the same. The dma_fence implementation unfortunately came up with a custom event handling mechanism instead of extending the core Linux wait_event() system.

I don't want a blocking wait at all.

Ok I wasn't clear enough :) That is exactly what I would NAK!

The wait must be blocking or otherwise you would allow wait-before-signal.

Well the current implementation is pulling a rather big trick on readers in this regard: It looks like a dma_fence, it's implemented as one even, heck you even open-code a dma_fence_wait here.

Except the semantics are completely different.

So aside from the discussion whether we really want to fully chain them I think it just doesn't make sense to implement the "wait for fence submit" as a dma_fence wait. And I'd outright remove that part from the uapi, and force the wait. The current radv/anv plans I discussed with Jason was that we'd have a separate submit thread, and hence unconditionally blocking until the fence has materialized is the right thing to do. Even allowing that option, either through a flag, or making these things look like dma_fences (they are _not_) just tricks folks into misunderstanding what's going on.

Code sharing just because the code looks similar is imo a really bad idea, when the semantics are entirely different (that was also the reason behind not reusing all the cpu event stuff for dma_fence, they're not normal cpu events).

-Daniel

Quoted 7 lines

Christian.

Quoted 1 line

-Chris

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Am 13.12.18 um 17:01 schrieb Daniel Vetter:

Quoted 93 lines · Christian

Quoted 79 lines

Am 13.12.18 um 13:21 schrieb Chris Wilson:

Quoted 78 lines

Quoting Koenig, Christian (2018-12-13 12:11:10)

Quoted 77 lines

Am 13.12.18 um 12:37 schrieb Chris Wilson:

Quoted 59 lines

Quoting Chunming Zhou (2018-12-11 10:34:45)

Quoted 58 lines

From: Christian König <[email protected]>

Implement finding the right timeline point in drm_syncobj_find_fence.

v2: return -EINVAL when the point is not submitted yet.
v3: fix reference counting bug, add flags handling as well

Signed-off-by Christian König <[email protected]>

---
    drivers/gpu/drm/drm_syncobj.c | 43 ++++++++++++++++++++++++++++++++---
    1 file changed, 40 insertions(+), 3 deletions(-)

drivers/gpu/drm/drm_syncobj.c +28 -3

index 76ce13dafc4d..d964b348ecba 100644
@@ -231,16 +231,53 @@ int drm_syncobj_find_fence(struct drm_file *file_private,
                              struct dma_fence **fence)
    {
           struct drm_syncobj *syncobj = drm_syncobj_find(file_private, handle);
-       int ret = 0;
+       struct syncobj_wait_entry wait;
+       int ret;
 
           if (!syncobj)
                   return -ENOENT;
 
           *fence = drm_syncobj_fence_get(syncobj);
-       if (!*fence) {
+       drm_syncobj_put(syncobj);
+
+       if (*fence) {
+               ret = dma_fence_chain_find_seqno(fence, point);
+               if (!ret)
+                       return 0;
+               dma_fence_put(*fence);
+       } else {
                   ret = -EINVAL;
           }
-       drm_syncobj_put(syncobj);
+
+       if (!(flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT))
+               return ret;
+
+       memset(&wait, 0, sizeof(wait));
+       wait.task = current;
+       wait.point = point;
+       drm_syncobj_fence_add_wait(syncobj, &wait);
+
+       do {
+               set_current_state(TASK_INTERRUPTIBLE);
+               if (wait.fence) {
+                       ret = 0;
+                       break;
+               }
+
+               if (signal_pending(current)) {
+                       ret = -ERESTARTSYS;

Quoted 8 lines

drivers/gpu/drm/drm_syncobj.c int drm_syncobj_find_fence(struct drm_file *file_private, · line 273

Quoted 5 lines

drivers/gpu/drm/drm_syncobj.c +5 -0

+                       break;
+               }
+
+               schedule();
+       } while (1);

I've previously used a dma_fence_proxy so that we could do nonblocking waits on future submits. That would be preferrable (a requirement for our stupid BKL-driven code).

That is exactly what I would definitely NAK.

I would rather say we should come up with a wait_multiple_events() macro and completely nuke the custom implementation of this in:
1. dma_fence_default_wait and dma_fence_wait_any_timeout
2. the radeon fence implementation
3. the nouveau fence implementation
4. the syncobj code

Quoted 7 lines

Quoted 4 lines

Quoted 3 lines

Cause all of them do exactly the same. The dma_fence implementation unfortunately came up with a custom event handling mechanism instead of extending the core Linux wait_event() system.

I don't want a blocking wait at all.

Ok I wasn't clear enough :) That is exactly what I would NAK!

The wait must be blocking or otherwise you would allow wait-before-signal.

Well the current implementation is pulling a rather big trick on readers in this regard: It looks like a dma_fence, it's implemented as one even, heck you even open-code a dma_fence_wait here.

Except the semantics are completely different.

Quoted 9 lines

So aside from the discussion whether we really want to fully chain them I think it just doesn't make sense to implement the "wait for fence submit" as a dma_fence wait. And I'd outright remove that part from the uapi, and force the wait. The current radv/anv plans I discussed with Jason was that we'd have a separate submit thread, and hence unconditionally blocking until the fence has materialized is the right thing to do. Even allowing that option, either through a flag, or making these things look like dma_fences (they are _not_) just tricks folks into misunderstanding what's going on.

Good, that sounds strongly like something I can agree on as well.

Quoted 4 lines

Code sharing just because the code looks similar is imo a really bad idea, when the semantics are entirely different (that was also the reason behind not reusing all the cpu event stuff for dma_fence, they're not normal cpu events).

Ok, the last sentence is what I don't understand.

What exactly is the semantic difference between the dma_fence_wait and the wait_event interface?

I mean the wait_event interface was introduced to prevent drivers from openly coding an event interface and getting it wrong all the time.

So a good part of the bugs we have seen around waiting for dma-fences are exactly why wait_event was invented in the first place.

The only big thing I can see missing in the wait_event interface is waiting for many events at the same time, but that should be a rather easy addition.

Quoted 12 lines

Quoted 4 lines

Code sharing just because the code looks similar is imo a really bad idea, when the semantics are entirely different (that was also the reason behind not reusing all the cpu event stuff for dma_fence, they're not normal cpu events).

Ok, the last sentence is what I don't understand.

What exactly is the semantic difference between the dma_fence_wait and the wait_event interface?

I mean the wait_event interface was introduced to prevent drivers from openly coding an event interface and getting it wrong all the time.

Quoted 6 lines

So a good part of the bugs we have seen around waiting for dma-fences are exactly why wait_event was invented in the first place.

The only big thing I can see missing in the wait_event interface is waiting for many events at the same time, but that should be a rather easy addition.

So this bikeshed was years ago, maybe I should type a patch to document it, but as far as I remember the big difference is:

- wait_event and friends generally Just Work. It can go wrong of course, but the usual pattern is that the waker-side does and uncoditional wake_up_all, and hence all the waiter needs to do is add themselves to the waiter list.

- dma_buf otoh is entirely different: We wanted to support all kinds fo signalling modes, including having interrupts disabled by default (not sure whether we actually achieve this still with all the cpu-side scheduling the big drivers do). Which means the waker does not unconditionally call wake_up_all, at least not timeline, and waiters need to call dma_fence_enable_signalling before they can add themselves to the waiter list and call schedule().

The other bit difference is how you check for the classic wakeup races where the event happens between when you checked for it and when you go to sleep. Because hw is involved, the rules are again a bit different, and their different between drivers because hw is incoherent/broken in all kinds of ways. So there's also really tricky things going on between adding the waiter to the waiter list and dma_fence_enable_signalling. For pure cpu events you can ignore this and bake the few necessary barriers into the various macros, dma_fence needs more.

Adding Maarten, maybe there was more. I definitely remember huge&very long discussions about all this.
-Daniel

From: Christian König <[email protected]>

Lockless container implementation similar to a dma_fence_array, but with only two elements per node and automatic garbage collection.

v2: properly document dma_fence_chain_for_each, add dma_fence_chain_find_seqno,
drop prev reference during garbage collection if it's not a chain fence.
v3: use head and iterator for dma_fence_chain_for_each
v4: fix reference count in dma_fence_chain_enable_signaling

Signed-off-by Christian König <[email protected]>

---
 drivers/dma-buf/Makefile          |   3 +-
 drivers/dma-buf/dma-fence-chain.c | 241 ++++++++++++++++++++++++++++++
 include/linux/dma-fence-chain.h   |  81 ++++++++++
 3 files changed, 324 insertions(+), 1 deletion(-)
 create mode 100644 drivers/dma-buf/dma-fence-chain.c
 create mode 100644 include/linux/dma-fence-chain.h

drivers/dma-buf/Makefile +2 -1

index 0913a6ccab5a..1f006e083eb9 100644
@@ -1,4 +1,5 @@
-obj-y := dma-buf.o dma-fence.o dma-fence-array.o reservation.o seqno-fence.o
+obj-y := dma-buf.o dma-fence.o dma-fence-array.o dma-fence-chain.o \
+        reservation.o seqno-fence.o
 obj-$(CONFIG_SYNC_FILE)                += sync_file.o
 obj-$(CONFIG_SW_SYNC)          += sw_sync.o sync_debug.o
 obj-$(CONFIG_UDMABUF)          += udmabuf.o

drivers/dma-buf/dma-fence-chain.c new file +0 -0

new file mode 100644
index 000000000000..0c5e3c902fa0

drivers/dma-buf/dma-fence-chain.c new file +66 -0

+ * fence-chain: chain fences together in a timeline
+ *
+ * Copyright (C) 2018 Advanced Micro Devices, Inc.
+ * Authors:
+ *     Christian König <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#include <linux/dma-fence-chain.h>
+
+static bool dma_fence_chain_enable_signaling(struct dma_fence *fence);
+
+/**
+ * dma_fence_chain_get_prev - use RCU to get a reference to the previous fence
+ * @chain: chain node to get the previous node from
+ *
+ * Use dma_fence_get_rcu_safe to get a reference to the previous fence of the
+ * chain node.
+ */
+static struct dma_fence *dma_fence_chain_get_prev(struct dma_fence_chain *chain)
+{
+       struct dma_fence *prev;
+
+       rcu_read_lock();
+       prev = dma_fence_get_rcu_safe(&chain->prev);
+       rcu_read_unlock();
+       return prev;
+}
+
+/**
+ * dma_fence_chain_walk - chain walking function
+ * @fence: current chain node
+ *
+ * Walk the chain to the next node. Returns the next fence or NULL if we are at
+ * the end of the chain. Garbage collects chain nodes which are already
+ * signaled.
+ */
+struct dma_fence *dma_fence_chain_walk(struct dma_fence *fence)
+{
+       struct dma_fence_chain *chain, *prev_chain;
+       struct dma_fence *prev, *replacement, *tmp;
+
+       chain = to_dma_fence_chain(fence);
+       if (!chain) {
+               dma_fence_put(fence);
+               return NULL;
+       }
+
+       while ((prev = dma_fence_chain_get_prev(chain))) {
+
+               prev_chain = to_dma_fence_chain(prev);
+               if (prev_chain) {
+                       if (!dma_fence_is_signaled(prev_chain->fence))
+                               break;
+
+                       replacement = dma_fence_chain_get_prev(prev_chain);
+               } else {
+                       if (!dma_fence_is_signaled(prev))

drivers/dma-buf/dma-fence-chain.c new file +6 -0

+       return prev;
+}
+EXPORT_SYMBOL(dma_fence_chain_walk);
 
+
+/**
+ * dma_fence_chain_find_seqno - find fence chain node by seqno

-----Original Message-----
From: dri-devel <[email protected]> On Behalf Of Chunming Zhou
Sent: Tuesday, December 11, 2018 6:35 PM
To: Koenig, Christian <[email protected]>; dri- [email protected]; [email protected]; intel- [email protected]
Cc: Christian König <[email protected]>; Koenig, Christian <[email protected]>
Subject: [PATCH 01/10] dma-buf: add new dma_fence_chain container v4

From: Christian König <[email protected]>

Lockless container implementation similar to a dma_fence_array, but with only two elements per node and automatic garbage collection.

v2: properly document dma_fence_chain_for_each, add dma_fence_chain_find_seqno,
drop prev reference during garbage collection if it's not a chain fence.
v3: use head and iterator for dma_fence_chain_for_each
v4: fix reference count in dma_fence_chain_enable_signaling

Signed-off-by Christian König <[email protected]>

---
 drivers/dma-buf/Makefile          |   3 +-
 drivers/dma-buf/dma-fence-chain.c | 241
++++++++++++++++++++++++++++++
 include/linux/dma-fence-chain.h   |  81 ++++++++++
 3 files changed, 324 insertions(+), 1 deletion(-)  create mode 100644

drivers/dma-buf/dma-fence-chain.c create mode 100644 include/linux/dma- fence-chain.h

drivers/dma-buf/Makefile index renamed from drivers/dma-buf/Makefile +0 -0

0913a6ccab5a..1f006e083eb9 100644

drivers/dma-buf/Makefile +0 -1

@@ -1,4 +1,5 @@
-obj-y := dma-buf.o dma-fence.o dma-fence-array.o reservation.o seqno-

fence.o

drivers/dma-buf/Makefile +2 -0

+obj-y := dma-buf.o dma-fence.o dma-fence-array.o dma-fence-chain.o \
+	 reservation.o seqno-fence.o
 obj-$(CONFIG_SYNC_FILE)		+= sync_file.o
 obj-$(CONFIG_SW_SYNC)		+= sw_sync.o sync_debug.o
 obj-$(CONFIG_UDMABUF)		+= udmabuf.o

drivers/dma-buf/dma- renamed from drivers/dma-buf/dma-fence-chain.c +0 -0

fence-chain.c
new file mode 100644
index 000000000000..0c5e3c902fa0

drivers/dma-buf/dma-fence-chain.c new file +16 -0

@@ -0,0 +1,241 @@
+/*
+ * fence-chain: chain fences together in a timeline
+ *
+ * Copyright (C) 2018 Advanced Micro Devices, Inc.
+ * Authors:
+ *	Christian König <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+it
+ * under the terms of the GNU General Public License version 2 as
+published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+WITHOUT
+ * ANY WARRANTY; without even the implied warranty of

MERCHANTABILITY

drivers/dma-buf/dma-fence-chain.c new file +56 -0

+or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
+License for
+ * more details.
+ */
+
+#include <linux/dma-fence-chain.h>
+
+static bool dma_fence_chain_enable_signaling(struct dma_fence *fence);
+
+/**
+ * dma_fence_chain_get_prev - use RCU to get a reference to the
+previous fence
+ * @chain: chain node to get the previous node from
+ *
+ * Use dma_fence_get_rcu_safe to get a reference to the previous fence
+of the
+ * chain node.
+ */
+static struct dma_fence *dma_fence_chain_get_prev(struct
+dma_fence_chain *chain) {
+	struct dma_fence *prev;
+
+	rcu_read_lock();
+	prev = dma_fence_get_rcu_safe(&chain->prev);
+	rcu_read_unlock();
+	return prev;
+}
+
+/**
+ * dma_fence_chain_walk - chain walking function
+ * @fence: current chain node
+ *
+ * Walk the chain to the next node. Returns the next fence or NULL if
+we are at
+ * the end of the chain. Garbage collects chain nodes which are already
+ * signaled.
+ */
+struct dma_fence *dma_fence_chain_walk(struct dma_fence *fence) {
+	struct dma_fence_chain *chain, *prev_chain;
+	struct dma_fence *prev, *replacement, *tmp;
+
+	chain = to_dma_fence_chain(fence);
+	if (!chain) {
+		dma_fence_put(fence);
+		return NULL;
+	}
+
+	while ((prev = dma_fence_chain_get_prev(chain))) {
+
+		prev_chain = to_dma_fence_chain(prev);
+		if (prev_chain) {
+			if (!dma_fence_is_signaled(prev_chain->fence))
+				break;
+
+			replacement =

dma_fence_chain_get_prev(prev_chain);

drivers/dma-buf/dma-fence-chain.c new file +92 -0

+		} else {
+			if (!dma_fence_is_signaled(prev))
+				break;
+
+			replacement = NULL;
+		}
+
+		tmp = cmpxchg(&chain->prev, prev, replacement);
+		if (tmp == prev)
+			dma_fence_put(tmp);
+		else
+			dma_fence_put(replacement);
+		dma_fence_put(prev);
+	}
+
+	dma_fence_put(fence);
+	return prev;
+}
+EXPORT_SYMBOL(dma_fence_chain_walk);
+
+/**
+ * dma_fence_chain_find_seqno - find fence chain node by seqno
+ * @pfence: pointer to the chain node where to start
+ * @seqno: the sequence number to search for
+ *
+ * Advance the fence pointer to the chain node which will signal this
+sequence
+ * number. If no sequence number is provided then this is a no-op.
+ *
+ * Returns EINVAL if the fence is not a chain node or the sequence
+number has
+ * not yet advanced far enough.
+ */
+int dma_fence_chain_find_seqno(struct dma_fence **pfence, uint64_t
+seqno) {
+	struct dma_fence_chain *chain;
+
+	if (!seqno)
+		return 0;
+
+	chain = to_dma_fence_chain(*pfence);
+	if (!chain || chain->base.seqno < seqno)
+		return -EINVAL;
+
+	dma_fence_chain_for_each(*pfence, &chain->base) {
+		if ((*pfence)->context != chain->base.context ||
+		    to_dma_fence_chain(*pfence)->prev_seqno < seqno)
+			break;
+	}
+	dma_fence_put(&chain->base);
+
+	return 0;
+}
+EXPORT_SYMBOL(dma_fence_chain_find_seqno);
+
+static const char *dma_fence_chain_get_driver_name(struct dma_fence
+*fence) {
+        return "dma_fence_chain";
+}
+
+static const char *dma_fence_chain_get_timeline_name(struct dma_fence
+*fence) {
+        return "unbound";
+}
+
+static void dma_fence_chain_irq_work(struct irq_work *work) {
+	struct dma_fence_chain *chain;
+
+	chain = container_of(work, typeof(*chain), work);
+
+	/* Try to rearm the callback */
+	if (!dma_fence_chain_enable_signaling(&chain->base))
+		/* Ok, we are done. No more unsignaled fences left */
+		dma_fence_signal(&chain->base);
+	dma_fence_put(&chain->base);
+}
+
+static void dma_fence_chain_cb(struct dma_fence *f, struct dma_fence_cb
+*cb) {
+	struct dma_fence_chain *chain;
+
+	chain = container_of(cb, typeof(*chain), cb);
+	irq_work_queue(&chain->work);
+	dma_fence_put(f);
+}
+
+static bool dma_fence_chain_enable_signaling(struct dma_fence *fence) {
+	struct dma_fence_chain *head = to_dma_fence_chain(fence);
+
+	dma_fence_get(&head->base);
+	dma_fence_chain_for_each(fence, &head->base) {
+		struct dma_fence_chain *chain =

to_dma_fence_chain(fence);

drivers/dma-buf/dma-fence-chain.c new file +4 -0

+		struct dma_fence *f = chain ? chain->fence : fence;
+
+		dma_fence_get(f);
+		if (!dma_fence_add_callback(f, &head->cb,

dma_fence_chain_cb)) {

drivers/dma-buf/dma-fence-chain.c new file +12 -0

+			dma_fence_put(fence);
+			return true;
+		}
+		dma_fence_put(f);
+	}
+	dma_fence_put(&head->base);
+	return false;
+}
+
+static bool dma_fence_chain_signaled(struct dma_fence *fence) {
+	dma_fence_chain_for_each(fence, fence) {
+		struct dma_fence_chain *chain =

to_dma_fence_chain(fence);

drivers/dma-buf/dma-fence-chain.c new file +59 -0

+		struct dma_fence *f = chain ? chain->fence : fence;
+
+		if (!dma_fence_is_signaled(f)) {
+			dma_fence_put(fence);
+			return false;
+		}
+	}
+
+	return true;
+}
+
+static void dma_fence_chain_release(struct dma_fence *fence) {
+	struct dma_fence_chain *chain = to_dma_fence_chain(fence);
+
+	dma_fence_put(chain->prev);
+	dma_fence_put(chain->fence);
+	dma_fence_free(fence);
+}
+
+const struct dma_fence_ops dma_fence_chain_ops = {
+	.get_driver_name = dma_fence_chain_get_driver_name,
+	.get_timeline_name = dma_fence_chain_get_timeline_name,
+	.enable_signaling = dma_fence_chain_enable_signaling,
+	.signaled = dma_fence_chain_signaled,
+	.release = dma_fence_chain_release,
+};
+EXPORT_SYMBOL(dma_fence_chain_ops);
+
+/**
+ * dma_fence_chain_init - initialize a fence chain
+ * @chain: the chain node to initialize
+ * @prev: the previous fence
+ * @fence: the current fence
+ *
+ * Initialize a new chain node and either start a new chain or add the
+node to
+ * the existing chain of the previous fence.
+ */
+void dma_fence_chain_init(struct dma_fence_chain *chain,
+			  struct dma_fence *prev,
+			  struct dma_fence *fence,
+			  uint64_t seqno)
+{
+	struct dma_fence_chain *prev_chain = to_dma_fence_chain(prev);
+	uint64_t context;
+
+	spin_lock_init(&chain->lock);
+	chain->prev = prev;
+	chain->fence = fence;
+	chain->prev_seqno = 0;
+	init_irq_work(&chain->work, dma_fence_chain_irq_work);
+
+	/* Try to reuse the context of the previous chain node. */
+	if (prev_chain && __dma_fence_is_later(seqno, prev->seqno)) {
+		context = prev->context;
+		chain->prev_seqno = prev->seqno;
+	} else {
+		context = dma_fence_context_alloc(1);
+		/* Make sure that we always have a valid sequence number.

*/

drivers/dma-buf/dma-fence-chain.c new file +7 -0

+		if (prev_chain)
+			seqno = max(prev->seqno, seqno);
+	}
+
+	dma_fence_init(&chain->base, &dma_fence_chain_ops,
+		       &chain->lock, context, seqno); }
+EXPORT_SYMBOL(dma_fence_chain_init);

include/linux/dma-fence- renamed from include/linux/dma-fence-chain.h +0 -0

chain.h new file mode 100644 index 000000000000..a5c2e8c6915c

include/linux/dma-fence-chain.h new file +16 -0

@@ -0,0 +1,81 @@
+/*
+ * fence-chain: chain fences together in a timeline
+ *
+ * Copyright (C) 2018 Advanced Micro Devices, Inc.
+ * Authors:
+ *	Christian König <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+it
+ * under the terms of the GNU General Public License version 2 as
+published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+WITHOUT
+ * ANY WARRANTY; without even the implied warranty of

MERCHANTABILITY

include/linux/dma-fence-chain.h new file +63 -0

+or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
+License for
+ * more details.
+ */
+
+#ifndef __LINUX_DMA_FENCE_CHAIN_H
+#define __LINUX_DMA_FENCE_CHAIN_H
+
+#include <linux/dma-fence.h>
+#include <linux/irq_work.h>
+
+/**
+ * struct dma_fence_chain - fence to represent an node of a fence chain
+ * @base: fence base class
+ * @lock: spinlock for fence handling
+ * @prev: previous fence of the chain
+ * @prev_seqno: original previous seqno before garbage collection
+ * @fence: encapsulated fence
+ * @cb: callback structure for signaling
+ * @work: irq work item for signaling
+ */
+struct dma_fence_chain {
+	struct dma_fence base;
+	spinlock_t lock;
+	struct dma_fence *prev;
+	u64 prev_seqno;
+	struct dma_fence *fence;
+	struct dma_fence_cb cb;
+	struct irq_work work;
+};
+
+extern const struct dma_fence_ops dma_fence_chain_ops;
+
+/**
+ * to_dma_fence_chain - cast a fence to a dma_fence_chain
+ * @fence: fence to cast to a dma_fence_array
+ *
+ * Returns NULL if the fence is not a dma_fence_chain,
+ * or the dma_fence_chain otherwise.
+ */
+static inline struct dma_fence_chain *
+to_dma_fence_chain(struct dma_fence *fence) {
+	if (!fence || fence->ops != &dma_fence_chain_ops)
+		return NULL;
+
+	return container_of(fence, struct dma_fence_chain, base); }
+
+/**
+ * dma_fence_chain_for_each - iterate over all fences in chain
+ * @iter: current fence
+ * @head: starting point
+ *
+ * Iterate over all fences in the chain. We keep a reference to the
+current
+ * fence while inside the loop which must be dropped when breaking out.
+ */
+#define dma_fence_chain_for_each(iter, head)	\
+	for (iter = dma_fence_get(head); iter; \
+	     iter = dma_fence_chain_walk(head))
+
+struct dma_fence *dma_fence_chain_walk(struct dma_fence *fence); int
+dma_fence_chain_find_seqno(struct dma_fence **pfence, uint64_t

seqno);

include/linux/dma-fence-chain.h new file +6 -1

+void dma_fence_chain_init(struct dma_fence_chain *chain,
+			  struct dma_fence *prev,
+			  struct dma_fence *fence,
+			  uint64_t seqno);
+
+#endif /* __LINUX_DMA_FENCE_CHAIN_H */
--

2.17.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel