On Tue, Jan 15, 2013 at 6:33 AM, Maarten Lankhorst m.b.lankhorst@gmail.com wrote:

Hi Maarten,

This is a nice looking extension to avoid re-implementing a mutex in TTM/reservation code.. ofc, probably someone more familiar with mutex code should probably review, but probably a bit of explanation about what and why would be helpful.

mutex_reserve_lock, and mutex_reserve_lock_interruptible: Lock a buffer with a reservation_id set. reservation_id must not be set to 0, since this is a special value that means no reservation_id.

Normally if reservation_id is not set, or is older than the reservation_id that's currently set on the mutex, the behavior will be to wait normally.

However, if the reservation_id is newer than the current reservation_id, -EAGAIN will be returned, and this function must unreserve all other mutexes and then redo a blocking lock with normal mutex calls to prevent a deadlock, then call mutex_locked_set_reservation on successful locking to set the reservation_id inside the lock.

It might be a bit more clear to write up how this works from the perspective of the user of ticket_mutex, separately from the internal implementation first, and then how it works internally? Ie, the mutex_set_reservation_fastpath() call is internal to the implementation of ticket_mutex, but -EAGAIN is something the caller of ticket_mutex shall deal with. This might give a clearer picture of how TTM / reservation uses this to prevent deadlock, so those less familiar with TTM could better understand.

Well, here is an attempt to start a write-up, which should perhaps eventually be folded into Documentation/ticket-mutex-design.txt. But hopefully a better explanation of the problem and the solution will encourage some review of the ticket_mutex changes.

========================== Basic problem statement: ----- ------- --------- GPU's do operations that commonly involve many buffers. Those buffers can be shared across contexts/processes, exist in different memory domains (for example VRAM vs system memory), and so on. And with PRIME / dmabuf, they can even be shared across devices. So there are a handful of situations where the driver needs to wait for buffers to become ready. If you think about this in terms of waiting on a buffer mutex for it to become available, this presents a problem because there is no way to guarantee that buffers appear in a execbuf/batch in the same order in all contexts. That is directly under control of userspace, and a result of the sequence of GL calls that an application makes. Which results in the potential for deadlock. The problem gets more complex when you consider that the kernel may need to migrate the buffer(s) into VRAM before the GPU operates on the buffer(s), which main in turn require evicting some other buffers (and you don't want to evict other buffers which are already queued up to the GPU), but for a simplified understanding of the problem you can ignore this.

The algorithm that TTM came up with for dealing with this problem is quite simple. For each group of buffers (execbuf) that need to be locked, the caller would be assigned a unique reservation_id, from a global counter. In case of deadlock in the process of locking all the buffers associated with a execbuf, the one with the lowest reservation_id wins, and the one with the higher reservation_id unlocks all of the buffers that it has already locked, and then tries again.

Originally TTM implemented this algorithm on top of an event-queue and atomic-ops, but Maarten Lankhorst realized that by merging this with the mutex code we could take advantage of the existing mutex fast-path code and result in a simpler solution, and so ticket_mutex was born. (Well, there where also some additional complexities with the original implementation when you start adding in cross-device buffer sharing for PRIME.. Maarten could probably better explain.)

How it is used: --- -- -- -----

A very simplified version:

int submit_execbuf(execbuf) { /* acquiring locks, before queuing up to GPU: */ seqno = assign_global_seqno(); retry: for (buf in execbuf->buffers) { ret = mutex_reserve_lock(&buf->lock, seqno); switch (ret) { case 0: /* we got the lock */ break; case -EAGAIN: /* someone with a lower seqno, so unreserve and try again: */ for (buf2 in reverse order starting before buf in execbuf->buffers) mutex_unreserve_unlock(&buf2->lock); goto retry; default: goto err; } }

/* now everything is good to go, submit job to GPU: */ ... }

int finish_execbuf(execbuf) { /* when GPU is finished: */ for (buf in execbuf->buffers) mutex_unreserve_unlock(&buf->lock); } ==========================

anyways, for the rest of the patch, I'm still going through the mutex/ticket_mutex code in conjunction with the reservation/fence patches, so for now just a couple very superficial comments.

These functions will return -EDEADLK instead of -EAGAIN if reservation_id is the same as the reservation_id that's attempted to lock the mutex with, since in that case you presumably attempted to lock the same lock twice.

mutex_reserve_lock_slow and mutex_reserve_lock_intr_slow: Similar to mutex_reserve_lock, except it won't backoff with -EAGAIN. This is useful after mutex_reserve_lock failed with -EAGAIN, and you unreserved all buffers so no deadlock can occur.

mutex_unreserve_unlock: Unlock a buffer reserved with the previous calls.

Missing at the moment, maybe TODO?

• lockdep warnings when wrongly calling mutex_unreserve_unlock or mutex_unlock, depending on whether reservation_id was set previously or not.

  • Does lockdep have something for this or do I need to extend struct mutex?

• Check if lockdep warns if you unlock a lock that other locks were nested to.

  • spin_lock(m); spin_lock_nest_lock(a, m); spin_unlock(m); spin_unlock(a); would be nice if it gave a splat. Have to recheck if it does, though..

Design: I chose for ticket_mutex to encapsulate struct mutex, so the extra memory usage and atomic set on init will only happen when you deliberately create a ticket lock.

Since the mutexes are mostly meant to protect buffer object serialization in ttm, not much contention is expected. I could be slightly smarter with wakeups, but this would be at the expense at adding a field to struct mutex_waiter. Because this would add overhead to all cases where ticket_mutexes are not used, and ticket_mutexes are less performance sensitive anyway since they only protect buffer objects, I didn't want to do this. It's still better than ttm always calling wake_up_all, which does a unconditional spin_lock_irqsave/irqrestore.

I needed this in kernel/mutex.c because of the extensions to __lock_common, which are hopefully optimized away for all normal paths.

Changes since RFC patch v1:

• Updated to use atomic_long instead of atomic, since the reservation_id was a long.
• added mutex_reserve_lock_slow and mutex_reserve_lock_intr_slow
• removed mutex_locked_set_reservation_id (or w/e it was called)

Signed-off-by: Maarten Lankhorst maarten.lankhorst@canonical.com

include/linux/mutex.h | 86 +++++++++++++- kernel/mutex.c | 317 +++++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 387 insertions(+), 16 deletions(-)

diff --git a/include/linux/mutex.h b/include/linux/mutex.h index 9121595..602c247 100644 --- a/include/linux/mutex.h +++ b/include/linux/mutex.h @@ -62,6 +62,11 @@ struct mutex { #endif };

+struct ticket_mutex {

•   struct mutex base;
  

•   atomic_long_t reservation_id;
  

+};

/*

• This is the control structure for tasks blocked on mutex,
• which resides on the blocked task's kernel stack:

@@ -109,12 +114,24 @@ static inline void mutex_destroy(struct mutex *lock) {} __DEBUG_MUTEX_INITIALIZER(lockname) \ __DEP_MAP_MUTEX_INITIALIZER(lockname) }

+#define __TICKET_MUTEX_INITIALIZER(lockname) \

•           { .base = __MUTEX_INITIALIZER(lockname) \
  

•           , .reservation_id = ATOMIC_LONG_INIT(0) }
  

#define DEFINE_MUTEX(mutexname) \ struct mutex mutexname = __MUTEX_INITIALIZER(mutexname)

extern void __mutex_init(struct mutex *lock, const char *name, struct lock_class_key *key);

+static inline void __ticket_mutex_init(struct ticket_mutex *lock,

•                                  const char *name,
  

•                                  struct lock_class_key *key)
  

+{

•   __mutex_init(&lock->base, name, key);
  

•   atomic_long_set(&lock->reservation_id, 0);
  

+}

/**

• mutex_is_locked - is the mutex locked
• @lock: the mutex to be queried

@@ -133,26 +150,91 @@ static inline int mutex_is_locked(struct mutex *lock) #ifdef CONFIG_DEBUG_LOCK_ALLOC extern void mutex_lock_nested(struct mutex *lock, unsigned int subclass); extern void _mutex_lock_nest_lock(struct mutex *lock, struct lockdep_map *nest_lock);

extern int __must_check mutex_lock_interruptible_nested(struct mutex *lock, unsigned int subclass); extern int __must_check mutex_lock_killable_nested(struct mutex *lock, unsigned int subclass);

+extern int __must_check _mutex_reserve_lock(struct ticket_mutex *lock,

•                                   struct lockdep_map *nest_lock,
  

•                                   unsigned long reservation_id);
  

+extern int __must_check _mutex_reserve_lock_interruptible(struct ticket_mutex *,

•                                   struct lockdep_map *nest_lock,
  

•                                   unsigned long reservation_id);
  

+extern void _mutex_reserve_lock_slow(struct ticket_mutex *lock,

•                                struct lockdep_map *nest_lock,
  

•                                unsigned long reservation_id);
  

+extern int __must_check _mutex_reserve_lock_intr_slow(struct ticket_mutex *,

•                                   struct lockdep_map *nest_lock,
  

•                                   unsigned long reservation_id);
  

#define mutex_lock(lock) mutex_lock_nested(lock, 0) #define mutex_lock_interruptible(lock) mutex_lock_interruptible_nested(lock, 0) #define mutex_lock_killable(lock) mutex_lock_killable_nested(lock, 0)

#define mutex_lock_nest_lock(lock, nest_lock) \ do { \

•   typecheck(struct lockdep_map *, &(nest_lock)->dep_map);         \
  

•   typecheck(struct lockdep_map *, &(nest_lock)->dep_map); \
  

looks like that was unintended whitespace change..`

    _mutex_lock_nest_lock(lock, &(nest_lock)->dep_map);             \

} while (0)

+#define mutex_reserve_lock(lock, nest_lock, reservation_id) \ +({ \

•   typecheck(struct lockdep_map *, &(nest_lock)->dep_map); \
  

•   _mutex_reserve_lock(lock, &(nest_lock)->dep_map, reservation_id);       \
  

+})

+#define mutex_reserve_lock_interruptible(lock, nest_lock, reservation_id) \ +({ \

•   typecheck(struct lockdep_map *, &(nest_lock)->dep_map); \
  

•   _mutex_reserve_lock_interruptible(lock, &(nest_lock)->dep_map,  \
  

•                                      reservation_id);             \
  

+})

+#define mutex_reserve_lock_slow(lock, nest_lock, reservation_id) \ +do { \

•   typecheck(struct lockdep_map *, &(nest_lock)->dep_map); \
  

•   _mutex_reserve_lock_slow(lock, &(nest_lock)->dep_map, reservation_id);  \
  

+} while (0)

+#define mutex_reserve_lock_intr_slow(lock, nest_lock, reservation_id) \ +({ \

•   typecheck(struct lockdep_map *, &(nest_lock)->dep_map); \
  

•   _mutex_reserve_lock_intr_slow(lock, &(nest_lock)->dep_map,      \
  

•                                 reservation_id);                  \
  

+})

#else extern void mutex_lock(struct mutex *lock); extern int __must_check mutex_lock_interruptible(struct mutex *lock); extern int __must_check mutex_lock_killable(struct mutex *lock);

+extern int __must_check _mutex_reserve_lock(struct ticket_mutex *lock,

•                                       unsigned long reservation_id);
  

+extern int __must_check _mutex_reserve_lock_interruptible(struct ticket_mutex *,

•                                           unsigned long reservation_id);
  

+extern void _mutex_reserve_lock_slow(struct ticket_mutex *lock,

•                                unsigned long reservation_id);
  

+extern int __must_check _mutex_reserve_lock_intr_slow(struct ticket_mutex *,

•                                           unsigned long reservation_id);
  

+#define mutex_reserve_lock(lock, nest_lock, reservation_id) \

•   _mutex_reserve_lock(lock, reservation_id)
  

+#define mutex_reserve_lock_interruptible(lock, nest_lock, reservation_id) \

•   _mutex_reserve_lock_interruptible(lock, reservation_id)
  

+#define mutex_reserve_lock_slow(lock, nest_lock, reservation_id) \

•   _mutex_reserve_lock_slow(lock, reservation_id)
  

+#define mutex_reserve_lock_intr_slow(lock, nest_lock, reservation_id) \

•   _mutex_reserve_lock_intr_slow(lock, reservation_id)
  

# define mutex_lock_nested(lock, subclass) mutex_lock(lock) # define mutex_lock_interruptible_nested(lock, subclass) mutex_lock_interruptible(lock) # define mutex_lock_killable_nested(lock, subclass) mutex_lock_killable(lock) @@ -167,6 +249,8 @@ extern int __must_check mutex_lock_killable(struct mutex *lock); */ extern int mutex_trylock(struct mutex *lock); extern void mutex_unlock(struct mutex *lock); +extern void mutex_unreserve_unlock(struct ticket_mutex *lock);

extern int atomic_dec_and_mutex_lock(atomic_t *cnt, struct mutex *lock);

#ifndef CONFIG_HAVE_ARCH_MUTEX_CPU_RELAX diff --git a/kernel/mutex.c b/kernel/mutex.c index a307cc9..8282729 100644 --- a/kernel/mutex.c +++ b/kernel/mutex.c @@ -126,16 +126,119 @@ void __sched mutex_unlock(struct mutex *lock)

EXPORT_SYMBOL(mutex_unlock);

+/**

• • mutex_unreserve_unlock - release the mutex
• • @lock: the mutex to be released
• • Unlock a mutex that has been locked by this task previously
• • with _mutex_reserve_lock*.
• • This function must not be used in interrupt context. Unlocking
• • of a not locked mutex is not allowed.
• */

+void __sched mutex_unreserve_unlock(struct ticket_mutex *lock) +{

•   /*
  

•    * mark mutex as no longer part of a reservation, next
  

•    * locker can set this again
  

•    */
  

•   atomic_long_set(&lock->reservation_id, 0);
  

•   /*
  

•    * The unlocking fastpath is the 0->1 transition from 'locked'
  

•    * into 'unlocked' state:
  

•    */
  

+#ifndef CONFIG_DEBUG_MUTEXES

•   /*
  

•    * When debugging is enabled we must not clear the owner before time,
  

•    * the slow path will always be taken, and that clears the owner field
  

•    * after verifying that it was indeed current.
  

•    */
  

•   mutex_clear_owner(&lock->base);
  

+#endif

•   __mutex_fastpath_unlock(&lock->base.count, __mutex_unlock_slowpath);
  

+} +EXPORT_SYMBOL(mutex_unreserve_unlock);

+static inline int __sched +__mutex_lock_check_reserve(struct mutex *lock, unsigned long reservation_id) +{

•   struct ticket_mutex *m = container_of(lock, struct ticket_mutex, base);
  

•   unsigned long cur_id;
  

•   cur_id = atomic_long_read(&m->reservation_id);
  

•   if (!cur_id)
  

•           return 0;
  

•   if (unlikely(reservation_id == cur_id))
  

•           return -EDEADLK;
  

•   if (unlikely(reservation_id - cur_id <= LONG_MAX))
  

•           return -EAGAIN;
  

•   return 0;
  

+}

+/*

• • after acquiring lock with fastpath or when we lost out in contested
• • slowpath, set reservation_id and wake up any waiters so they can recheck.
• */

I think that is a bit misleading, if I'm understanding correctly this is called once you get the lock (but in either fast or slow path)

+static __always_inline void +mutex_set_reservation_fastpath(struct ticket_mutex *lock,

•                          unsigned long reservation_id, bool check_res)
  

+{

•   unsigned long flags;
  

•   struct mutex_waiter *cur;
  

•   if (check_res || config_enabled(CONFIG_DEBUG_LOCK_ALLOC)) {
  

•           unsigned long cur_id;
  

•           cur_id = atomic_long_xchg(&lock->reservation_id,
  

•                                     reservation_id);
  

+#ifdef CONFIG_DEBUG_LOCK_ALLOC

•           if (check_res)
  

•                   DEBUG_LOCKS_WARN_ON(cur_id &&
  

•                                      cur_id != reservation_id);
  

•           else
  

•                   DEBUG_LOCKS_WARN_ON(cur_id);
  

•           lockdep_assert_held(&lock->base);
  

+#endif

•           if (unlikely(cur_id == reservation_id))
  

•                   return;
  

•   } else
  

•           atomic_long_set(&lock->reservation_id, reservation_id);
  

•   /*
  

•    * Check if lock is contended, if not there is nobody to wake up
  

•    */
  

•   if (likely(atomic_read(&lock->base.count) == 0))
  

•           return;
  

•   /*
  

•    * Uh oh, we raced in fastpath, wake up everyone in this case,
  

•    * so they can see the new reservation_id
  

•    */
  

•   spin_lock_mutex(&lock->base.wait_lock, flags);
  

•   list_for_each_entry(cur, &lock->base.wait_list, list) {
  

•           debug_mutex_wake_waiter(&lock->base, cur);
  

•           wake_up_process(cur->task);
  

•   }
  

•   spin_unlock_mutex(&lock->base.wait_lock, flags);
  

+}

/*

• Lock a mutex (possibly interruptible), slowpath:

*/ static inline int __sched __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,

•               struct lockdep_map *nest_lock, unsigned long ip)
  

•               struct lockdep_map *nest_lock, unsigned long ip,
  

•               unsigned long reservation_id, bool res_slow)
  

{ struct task_struct *task = current; struct mutex_waiter waiter; unsigned long flags;

•   int ret;
  
    preempt_disable();
    mutex_acquire_nest(&lock->dep_map, subclass, 0, nest_lock, ip);
  

@@ -162,6 +265,12 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass, for (;;) { struct task_struct *owner;

•           if (!__builtin_constant_p(reservation_id) && !res_slow) {
  

•                   ret = __mutex_lock_check_reserve(lock, reservation_id);
  

•                   if (ret)
  

•                           goto err_nowait;
  

•           }
  

•           /*
             * If there's an owner, wait for it to either
             * release the lock or go to sleep.
  

@@ -172,6 +281,13 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,

            if (atomic_cmpxchg(&lock->count, 1, 0) == 1) {
                    lock_acquired(&lock->dep_map, ip);

•                   if (res_slow) {
  

•                           struct ticket_mutex *m;
  

•                           m = container_of(lock, struct ticket_mutex, base);
  

•                           mutex_set_reservation_fastpath(m, reservation_id, false);
  

•                   }
  

•                   mutex_set_owner(lock);
                    preempt_enable();
                    return 0;
  

@@ -227,15 +343,16 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass, * TASK_UNINTERRUPTIBLE case.) */ if (unlikely(signal_pending_state(state, task))) {

•                   mutex_remove_waiter(lock, &waiter,
  

•                                       task_thread_info(task));
  

•                   mutex_release(&lock->dep_map, 1, ip);
  

•                   spin_unlock_mutex(&lock->wait_lock, flags);
  

•                   ret = -EINTR;
  

•                   goto err;
  

•           }
  
  

•                   debug_mutex_free_waiter(&waiter);
  

•                   preempt_enable();
  

•                   return -EINTR;
  

•           if (!__builtin_constant_p(reservation_id) && !res_slow) {
  

•                   ret = __mutex_lock_check_reserve(lock, reservation_id);
  

•                   if (ret)
  

•                           goto err;
            }
  

•           __set_task_state(task, state);
  
            /* didn't get the lock, go to sleep: */
  

@@ -250,6 +367,28 @@ done: mutex_remove_waiter(lock, &waiter, current_thread_info()); mutex_set_owner(lock);

•   if (!__builtin_constant_p(reservation_id)) {
  

•           struct ticket_mutex *m;
  

•           struct mutex_waiter *cur;
  

•           /*
  

•            * this should get optimized out for the common case,
  

•            * and is only important for _mutex_reserve_lock
  

•            */
  

•           m = container_of(lock, struct ticket_mutex, base);
  

•           atomic_long_set(&m->reservation_id, reservation_id);
  

•           /*
  

•            * give any possible sleeping processes the chance to wake up,
  

•            * so they can recheck if they have to back off from
  

•            * reservations
  

•            */
  

•           list_for_each_entry(cur, &lock->wait_list, list) {
  

•                   debug_mutex_wake_waiter(lock, cur);
  

•                   wake_up_process(cur->task);
  

•           }
  

•   }
  

•   /* set it to 0 if there are no waiters left: */
    if (likely(list_empty(&lock->wait_list)))
            atomic_set(&lock->count, 0);
  

@@ -260,6 +399,19 @@ done: preempt_enable();

    return 0;

+err:

•   mutex_remove_waiter(lock, &waiter, task_thread_info(task));
  

•   spin_unlock_mutex(&lock->wait_lock, flags);
  

•   debug_mutex_free_waiter(&waiter);
  

+#ifdef CONFIG_MUTEX_SPIN_ON_OWNER +err_nowait: +#endif

•   mutex_release(&lock->dep_map, 1, ip);
  

•   preempt_enable();
  

•   return ret;
  

}

#ifdef CONFIG_DEBUG_LOCK_ALLOC @@ -267,7 +419,8 @@ void __sched mutex_lock_nested(struct mutex *lock, unsigned int subclass) { might_sleep();

•   __mutex_lock_common(lock, TASK_UNINTERRUPTIBLE, subclass, NULL, _RET_IP_);
  

•   __mutex_lock_common(lock, TASK_UNINTERRUPTIBLE,
  

•                       subclass, NULL, _RET_IP_, 0, 0);
  

}

EXPORT_SYMBOL_GPL(mutex_lock_nested); @@ -276,7 +429,8 @@ void __sched _mutex_lock_nest_lock(struct mutex *lock, struct lockdep_map *nest) { might_sleep();

•   __mutex_lock_common(lock, TASK_UNINTERRUPTIBLE, 0, nest, _RET_IP_);
  

•   __mutex_lock_common(lock, TASK_UNINTERRUPTIBLE,
  

•                       0, nest, _RET_IP_, 0, 0);
  

}

EXPORT_SYMBOL_GPL(_mutex_lock_nest_lock); @@ -285,7 +439,8 @@ int __sched mutex_lock_killable_nested(struct mutex *lock, unsigned int subclass) { might_sleep();

•   return __mutex_lock_common(lock, TASK_KILLABLE, subclass, NULL, _RET_IP_);
  

•   return __mutex_lock_common(lock, TASK_KILLABLE,
  

•                              subclass, NULL, _RET_IP_, 0, 0);
  

} EXPORT_SYMBOL_GPL(mutex_lock_killable_nested);

@@ -294,10 +449,63 @@ mutex_lock_interruptible_nested(struct mutex *lock, unsigned int subclass) { might_sleep(); return __mutex_lock_common(lock, TASK_INTERRUPTIBLE,

•                              subclass, NULL, _RET_IP_);
  

•                              subclass, NULL, _RET_IP_, 0, 0);
  

}

EXPORT_SYMBOL_GPL(mutex_lock_interruptible_nested);

+int __sched +_mutex_reserve_lock(struct ticket_mutex *lock, struct lockdep_map *nest,

•               unsigned long reservation_id)
  

+{

•   DEBUG_LOCKS_WARN_ON(!reservation_id);
  

•   might_sleep();
  

•   return __mutex_lock_common(&lock->base, TASK_UNINTERRUPTIBLE,
  

•                              0, nest, _RET_IP_, reservation_id, 0);
  

+} +EXPORT_SYMBOL_GPL(_mutex_reserve_lock);

+int __sched +_mutex_reserve_lock_interruptible(struct ticket_mutex *lock,

•                             struct lockdep_map *nest,
  

•                             unsigned long reservation_id)
  

+{

•   DEBUG_LOCKS_WARN_ON(!reservation_id);
  

•   might_sleep();
  

•   return __mutex_lock_common(&lock->base, TASK_INTERRUPTIBLE,
  

•                              0, nest, _RET_IP_, reservation_id, 0);
  

+} +EXPORT_SYMBOL_GPL(_mutex_reserve_lock_interruptible);

+void __sched +_mutex_reserve_lock_slow(struct ticket_mutex *lock, struct lockdep_map *nest,

•                    unsigned long reservation_id)
  

+{

•   DEBUG_LOCKS_WARN_ON(!reservation_id);
  

•   might_sleep();
  

•   __mutex_lock_common(&lock->base, TASK_UNINTERRUPTIBLE, 0,
  

•                       nest, _RET_IP_, reservation_id, 1);
  

+} +EXPORT_SYMBOL_GPL(_mutex_reserve_lock_slow);

+int __sched +_mutex_reserve_lock_intr_slow(struct ticket_mutex *lock,

•                         struct lockdep_map *nest,
  

•                         unsigned long reservation_id)
  

+{

•   DEBUG_LOCKS_WARN_ON(!reservation_id);
  

•   might_sleep();
  

•   return __mutex_lock_common(&lock->base, TASK_INTERRUPTIBLE, 0,
  

•                              nest, _RET_IP_, reservation_id, 1);
  

+} +EXPORT_SYMBOL_GPL(_mutex_reserve_lock_intr_slow);

#endif

/* @@ -400,7 +608,8 @@ __mutex_lock_slowpath(atomic_t *lock_count) { struct mutex *lock = container_of(lock_count, struct mutex, count);

•   __mutex_lock_common(lock, TASK_UNINTERRUPTIBLE, 0, NULL, _RET_IP_);
  

•   __mutex_lock_common(lock, TASK_UNINTERRUPTIBLE, 0,
  

•                       NULL, _RET_IP_, 0, 0);
  

}

static noinline int __sched @@ -408,7 +617,8 @@ __mutex_lock_killable_slowpath(atomic_t *lock_count) { struct mutex *lock = container_of(lock_count, struct mutex, count);

•   return __mutex_lock_common(lock, TASK_KILLABLE, 0, NULL, _RET_IP_);
  

•   return __mutex_lock_common(lock, TASK_KILLABLE, 0,
  

•                              NULL, _RET_IP_, 0, 0);
  

}

static noinline int __sched @@ -416,8 +626,28 @@ __mutex_lock_interruptible_slowpath(atomic_t *lock_count) { struct mutex *lock = container_of(lock_count, struct mutex, count);

•   return __mutex_lock_common(lock, TASK_INTERRUPTIBLE, 0, NULL, _RET_IP_);
  

•   return __mutex_lock_common(lock, TASK_INTERRUPTIBLE, 0,
  

•                              NULL, _RET_IP_, 0, 0);
  

+}

+static noinline int __sched +__mutex_lock_reserve_slowpath(atomic_t *lock_count, void *rid) +{

•   struct mutex *lock = container_of(lock_count, struct mutex, count);
  

•   return __mutex_lock_common(lock, TASK_UNINTERRUPTIBLE, 0,
  

•                              NULL, _RET_IP_, (unsigned long)rid, 0);
  

+}

+static noinline int __sched +__mutex_lock_interruptible_reserve_slowpath(atomic_t *lock_count, void *rid) +{

•   struct mutex *lock = container_of(lock_count, struct mutex, count);
  

•   return __mutex_lock_common(lock, TASK_INTERRUPTIBLE, 0,
  

•                              NULL, _RET_IP_, (unsigned long)rid, 0);
  

}

#endif

/* @@ -473,6 +703,63 @@ int __sched mutex_trylock(struct mutex *lock) } EXPORT_SYMBOL(mutex_trylock);

+#ifndef CONFIG_DEBUG_LOCK_ALLOC +int __sched +_mutex_reserve_lock(struct ticket_mutex *lock, unsigned long rid) +{

•   int ret;
  

•   might_sleep();
  

•   ret = __mutex_fastpath_lock_retval_arg(&lock->base.count, (void *)rid,
  

•                                   __mutex_lock_reserve_slowpath);
  

•   if (!ret) {
  

•           mutex_set_reservation_fastpath(lock, rid, true);
  

•           mutex_set_owner(&lock->base);
  

•   }
  

•   return ret;
  

+} +EXPORT_SYMBOL(_mutex_reserve_lock);

+int __sched +_mutex_reserve_lock_interruptible(struct ticket_mutex *lock, unsigned long rid) +{

•   int ret;
  

•   might_sleep();
  

•   ret = __mutex_fastpath_lock_retval_arg(&lock->base.count, (void *)rid,
  

•                           __mutex_lock_interruptible_reserve_slowpath);
  

•   if (!ret) {
  

•           mutex_set_reservation_fastpath(lock, rid, true);
  

•           mutex_set_owner(&lock->base);
  

•   }
  

•   return ret;
  

+} +EXPORT_SYMBOL(_mutex_reserve_lock_interruptible);

+void __sched +_mutex_reserve_lock_slow(struct ticket_mutex *lock, unsigned long rid) +{

•   might_sleep();
  

•   __mutex_lock_common(&lock->base, TASK_UNINTERRUPTIBLE,
  

•                       0, NULL, _RET_IP_, rid, 1);
  

+} +EXPORT_SYMBOL(_mutex_reserve_lock_slow);

+int __sched +_mutex_reserve_lock_intr_slow(struct ticket_mutex *lock, unsigned long rid) +{

•   might_sleep();
  

•   return __mutex_lock_common(&lock->base, TASK_INTERRUPTIBLE,
  

•                              0, NULL, _RET_IP_, rid, 1);
  

+} +EXPORT_SYMBOL(_mutex_reserve_lock_intr_slow);

+#endif

/**

• atomic_dec_and_mutex_lock - return holding mutex if we dec to 0
• @cnt: the atomic which we are to dec

-- 1.8.0.3

---

dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel

Op 30-01-13 02:07, Rob Clark schreef:

On Tue, Jan 15, 2013 at 6:33 AM, Maarten Lankhorst m.b.lankhorst@gmail.com wrote: Hi Maarten,

This is a nice looking extension to avoid re-implementing a mutex in TTM/reservation code.. ofc, probably someone more familiar with mutex code should probably review, but probably a bit of explanation about what and why would be helpful.

...

mutex_reserve_lock, and mutex_reserve_lock_interruptible: Lock a buffer with a reservation_id set. reservation_id must not be set to 0, since this is a special value that means no reservation_id.

Normally if reservation_id is not set, or is older than the reservation_id that's currently set on the mutex, the behavior will be to wait normally.

However, if the reservation_id is newer than the current reservation_id, -EAGAIN will be returned, and this function must unreserve all other mutexes and then redo a blocking lock with normal mutex calls to prevent a deadlock, then call mutex_locked_set_reservation on successful locking to set the reservation_id inside the lock.

It might be a bit more clear to write up how this works from the perspective of the user of ticket_mutex, separately from the internal implementation first, and then how it works internally? Ie, the mutex_set_reservation_fastpath() call is internal to the implementation of ticket_mutex, but -EAGAIN is something the caller of ticket_mutex shall deal with. This might give a clearer picture of how TTM / reservation uses this to prevent deadlock, so those less familiar with TTM could better understand.

Well, here is an attempt to start a write-up, which should perhaps eventually be folded into Documentation/ticket-mutex-design.txt. But hopefully a better explanation of the problem and the solution will encourage some review of the ticket_mutex changes.

========================== Basic problem statement:

---

GPU's do operations that commonly involve many buffers. Those buffers can be shared across contexts/processes, exist in different memory domains (for example VRAM vs system memory), and so on. And with PRIME / dmabuf, they can even be shared across devices. So there are a handful of situations where the driver needs to wait for buffers to become ready. If you think about this in terms of waiting on a buffer mutex for it to become available, this presents a problem because there is no way to guarantee that buffers appear in a execbuf/batch in the same order in all contexts. That is directly under control of userspace, and a result of the sequence of GL calls that an application makes. Which results in the potential for deadlock. The problem gets more complex when you consider that the kernel may need to migrate the buffer(s) into VRAM before the GPU operates on the buffer(s), which main in turn require evicting some other buffers (and you don't want to evict other buffers which are already queued up to the GPU), but for a simplified understanding of the problem you can ignore this.

The algorithm that TTM came up with for dealing with this problem is quite simple. For each group of buffers (execbuf) that need to be locked, the caller would be assigned a unique reservation_id, from a global counter. In case of deadlock in the process of locking all the buffers associated with a execbuf, the one with the lowest reservation_id wins, and the one with the higher reservation_id unlocks all of the buffers that it has already locked, and then tries again.

Originally TTM implemented this algorithm on top of an event-queue and atomic-ops, but Maarten Lankhorst realized that by merging this with the mutex code we could take advantage of the existing mutex fast-path code and result in a simpler solution, and so ticket_mutex was born. (Well, there where also some additional complexities with the original implementation when you start adding in cross-device buffer sharing for PRIME.. Maarten could probably better explain.)

How it is used:

---

A very simplified version:

int submit_execbuf(execbuf) { /* acquiring locks, before queuing up to GPU: */ seqno = assign_global_seqno();

You also need to make a 'lock' type for seqno, and lock it for lockdep purposes. This will be a virtual lock that will only exist in lockdep, but it's needed for proper lockdep annotation.

See reservation_ticket_init/fini. It's also important that seqno must not be 0, ever.

retry: for (buf in execbuf->buffers) { ret = mutex_reserve_lock(&buf->lock, seqno);

The lockdep class for this lock must be the same for all reservations, and for maximum lockdep usability you want all the buf->lock lockdep class for all objects across all devices to be the same too.

The __ticket_mutex_init in reservation_object_init does just that for you. :-)

      switch (ret) {
      case 0:
          /* we got the lock */
          break;
      case -EAGAIN:
          /* someone with a lower seqno, so unreserve and try again: */
          for (buf2 in reverse order starting before buf in

execbuf->buffers) mutex_unreserve_unlock(&buf2->lock); goto retry;

Almost correct, you need to re-regrab buf->lock after unreserving all other buffers with mutex_reserve_lock_slow, then goto retry, and skip over this bo when doing the normal locking.

The difference between mutex_reserve_lock and mutex_reserve_lock_slow is that mutex_reserve_lock_slow will block indefinitely where mutex_reserve_lock would return -EAGAIN. mutex_reserve_lock_slow does not return an error code. mutex_reserve_lock_intr_slow can return -EINTR if interrupted.

      default:
          goto err;
      }
  }

  /* now everything is good to go, submit job to GPU: */
  ...

}

int finish_execbuf(execbuf) { /* when GPU is finished: */ for (buf in execbuf->buffers) mutex_unreserve_unlock(&buf->lock); } ==========================

Thanks for taking the effort into writing this.

anyways, for the rest of the patch, I'm still going through the mutex/ticket_mutex code in conjunction with the reservation/fence patches, so for now just a couple very superficial comments.

...

These functions will return -EDEADLK instead of -EAGAIN if reservation_id is the same as the reservation_id that's attempted to lock the mutex with, since in that case you presumably attempted to lock the same lock twice.

mutex_reserve_lock_slow and mutex_reserve_lock_intr_slow: Similar to mutex_reserve_lock, except it won't backoff with -EAGAIN. This is useful after mutex_reserve_lock failed with -EAGAIN, and you unreserved all buffers so no deadlock can occur.

mutex_unreserve_unlock: Unlock a buffer reserved with the previous calls.

Missing at the moment, maybe TODO?

• lockdep warnings when wrongly calling mutex_unreserve_unlock or mutex_unlock, depending on whether reservation_id was set previously or not.

  • Does lockdep have something for this or do I need to extend struct mutex?

• Check if lockdep warns if you unlock a lock that other locks were nested to.

  • spin_lock(m); spin_lock_nest_lock(a, m); spin_unlock(m); spin_unlock(a); would be nice if it gave a splat. Have to recheck if it does, though..

Design: I chose for ticket_mutex to encapsulate struct mutex, so the extra memory usage and atomic set on init will only happen when you deliberately create a ticket lock.

Since the mutexes are mostly meant to protect buffer object serialization in ttm, not much contention is expected. I could be slightly smarter with wakeups, but this would be at the expense at adding a field to struct mutex_waiter. Because this would add overhead to all cases where ticket_mutexes are not used, and ticket_mutexes are less performance sensitive anyway since they only protect buffer objects, I didn't want to do this. It's still better than ttm always calling wake_up_all, which does a unconditional spin_lock_irqsave/irqrestore.

I needed this in kernel/mutex.c because of the extensions to __lock_common, which are hopefully optimized away for all normal paths.

Changes since RFC patch v1:

• Updated to use atomic_long instead of atomic, since the reservation_id was a long.
• added mutex_reserve_lock_slow and mutex_reserve_lock_intr_slow
• removed mutex_locked_set_reservation_id (or w/e it was called)

Signed-off-by: Maarten Lankhorst maarten.lankhorst@canonical.com

include/linux/mutex.h | 86 +++++++++++++- kernel/mutex.c | 317 +++++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 387 insertions(+), 16 deletions(-)

diff --git a/include/linux/mutex.h b/include/linux/mutex.h index 9121595..602c247 100644 --- a/include/linux/mutex.h +++ b/include/linux/mutex.h @@ -62,6 +62,11 @@ struct mutex { #endif };

+struct ticket_mutex {

•   struct mutex base;
  

•   atomic_long_t reservation_id;
  

+};

/*

• This is the control structure for tasks blocked on mutex,
• which resides on the blocked task's kernel stack:

@@ -109,12 +114,24 @@ static inline void mutex_destroy(struct mutex *lock) {} __DEBUG_MUTEX_INITIALIZER(lockname) \ __DEP_MAP_MUTEX_INITIALIZER(lockname) }

+#define __TICKET_MUTEX_INITIALIZER(lockname) \

•           { .base = __MUTEX_INITIALIZER(lockname) \
  

•           , .reservation_id = ATOMIC_LONG_INIT(0) }
  

#define DEFINE_MUTEX(mutexname) \ struct mutex mutexname = __MUTEX_INITIALIZER(mutexname)

extern void __mutex_init(struct mutex *lock, const char *name, struct lock_class_key *key);

+static inline void __ticket_mutex_init(struct ticket_mutex *lock,

•                                  const char *name,
  

•                                  struct lock_class_key *key)
  

+{

•   __mutex_init(&lock->base, name, key);
  

•   atomic_long_set(&lock->reservation_id, 0);
  

+}

/**

• mutex_is_locked - is the mutex locked
• @lock: the mutex to be queried

@@ -133,26 +150,91 @@ static inline int mutex_is_locked(struct mutex *lock) #ifdef CONFIG_DEBUG_LOCK_ALLOC extern void mutex_lock_nested(struct mutex *lock, unsigned int subclass); extern void _mutex_lock_nest_lock(struct mutex *lock, struct lockdep_map *nest_lock);

extern int __must_check mutex_lock_interruptible_nested(struct mutex *lock, unsigned int subclass); extern int __must_check mutex_lock_killable_nested(struct mutex *lock, unsigned int subclass);

+extern int __must_check _mutex_reserve_lock(struct ticket_mutex *lock,

•                                   struct lockdep_map *nest_lock,
  

•                                   unsigned long reservation_id);
  

+extern int __must_check _mutex_reserve_lock_interruptible(struct ticket_mutex *,

•                                   struct lockdep_map *nest_lock,
  

•                                   unsigned long reservation_id);
  

+extern void _mutex_reserve_lock_slow(struct ticket_mutex *lock,

•                                struct lockdep_map *nest_lock,
  

•                                unsigned long reservation_id);
  

+extern int __must_check _mutex_reserve_lock_intr_slow(struct ticket_mutex *,

•                                   struct lockdep_map *nest_lock,
  

•                                   unsigned long reservation_id);
  

#define mutex_lock(lock) mutex_lock_nested(lock, 0) #define mutex_lock_interruptible(lock) mutex_lock_interruptible_nested(lock, 0) #define mutex_lock_killable(lock) mutex_lock_killable_nested(lock, 0)

#define mutex_lock_nest_lock(lock, nest_lock) \ do { \

•   typecheck(struct lockdep_map *, &(nest_lock)->dep_map);         \
  

•   typecheck(struct lockdep_map *, &(nest_lock)->dep_map); \
  

looks like that was unintended whitespace change..`

I think it was intentional, as it would be just above 80 lines otherwise.

...

    _mutex_lock_nest_lock(lock, &(nest_lock)->dep_map);             \

} while (0)

+#define mutex_reserve_lock(lock, nest_lock, reservation_id) \ +({ \

•   typecheck(struct lockdep_map *, &(nest_lock)->dep_map); \
  

•   _mutex_reserve_lock(lock, &(nest_lock)->dep_map, reservation_id);       \
  

+})

+#define mutex_reserve_lock_interruptible(lock, nest_lock, reservation_id) \ +({ \

•   typecheck(struct lockdep_map *, &(nest_lock)->dep_map); \
  

•   _mutex_reserve_lock_interruptible(lock, &(nest_lock)->dep_map,  \
  

•                                      reservation_id);             \
  

+})

+#define mutex_reserve_lock_slow(lock, nest_lock, reservation_id) \ +do { \

•   typecheck(struct lockdep_map *, &(nest_lock)->dep_map); \
  

•   _mutex_reserve_lock_slow(lock, &(nest_lock)->dep_map, reservation_id);  \
  

+} while (0)

+#define mutex_reserve_lock_intr_slow(lock, nest_lock, reservation_id) \ +({ \

•   typecheck(struct lockdep_map *, &(nest_lock)->dep_map); \
  

•   _mutex_reserve_lock_intr_slow(lock, &(nest_lock)->dep_map,      \
  

•                                 reservation_id);                  \
  

+})

#else extern void mutex_lock(struct mutex *lock); extern int __must_check mutex_lock_interruptible(struct mutex *lock); extern int __must_check mutex_lock_killable(struct mutex *lock);

+extern int __must_check _mutex_reserve_lock(struct ticket_mutex *lock,

•                                       unsigned long reservation_id);
  

+extern int __must_check _mutex_reserve_lock_interruptible(struct ticket_mutex *,

•                                           unsigned long reservation_id);
  

+extern void _mutex_reserve_lock_slow(struct ticket_mutex *lock,

•                                unsigned long reservation_id);
  

+extern int __must_check _mutex_reserve_lock_intr_slow(struct ticket_mutex *,

•                                           unsigned long reservation_id);
  

+#define mutex_reserve_lock(lock, nest_lock, reservation_id) \

•   _mutex_reserve_lock(lock, reservation_id)
  

+#define mutex_reserve_lock_interruptible(lock, nest_lock, reservation_id) \

•   _mutex_reserve_lock_interruptible(lock, reservation_id)
  

+#define mutex_reserve_lock_slow(lock, nest_lock, reservation_id) \

•   _mutex_reserve_lock_slow(lock, reservation_id)
  

+#define mutex_reserve_lock_intr_slow(lock, nest_lock, reservation_id) \

•   _mutex_reserve_lock_intr_slow(lock, reservation_id)
  

# define mutex_lock_nested(lock, subclass) mutex_lock(lock) # define mutex_lock_interruptible_nested(lock, subclass) mutex_lock_interruptible(lock) # define mutex_lock_killable_nested(lock, subclass) mutex_lock_killable(lock) @@ -167,6 +249,8 @@ extern int __must_check mutex_lock_killable(struct mutex *lock); */ extern int mutex_trylock(struct mutex *lock); extern void mutex_unlock(struct mutex *lock); +extern void mutex_unreserve_unlock(struct ticket_mutex *lock);

extern int atomic_dec_and_mutex_lock(atomic_t *cnt, struct mutex *lock);

#ifndef CONFIG_HAVE_ARCH_MUTEX_CPU_RELAX diff --git a/kernel/mutex.c b/kernel/mutex.c index a307cc9..8282729 100644 --- a/kernel/mutex.c +++ b/kernel/mutex.c @@ -126,16 +126,119 @@ void __sched mutex_unlock(struct mutex *lock)

EXPORT_SYMBOL(mutex_unlock);

+/**

• • mutex_unreserve_unlock - release the mutex
• • @lock: the mutex to be released
• • Unlock a mutex that has been locked by this task previously
• • with _mutex_reserve_lock*.
• • This function must not be used in interrupt context. Unlocking
• • of a not locked mutex is not allowed.
• */

+void __sched mutex_unreserve_unlock(struct ticket_mutex *lock) +{

•   /*
  

•    * mark mutex as no longer part of a reservation, next
  

•    * locker can set this again
  

•    */
  

•   atomic_long_set(&lock->reservation_id, 0);
  

•   /*
  

•    * The unlocking fastpath is the 0->1 transition from 'locked'
  

•    * into 'unlocked' state:
  

•    */
  

+#ifndef CONFIG_DEBUG_MUTEXES

•   /*
  

•    * When debugging is enabled we must not clear the owner before time,
  

•    * the slow path will always be taken, and that clears the owner field
  

•    * after verifying that it was indeed current.
  

•    */
  

•   mutex_clear_owner(&lock->base);
  

+#endif

•   __mutex_fastpath_unlock(&lock->base.count, __mutex_unlock_slowpath);
  

+} +EXPORT_SYMBOL(mutex_unreserve_unlock);

+static inline int __sched +__mutex_lock_check_reserve(struct mutex *lock, unsigned long reservation_id) +{

•   struct ticket_mutex *m = container_of(lock, struct ticket_mutex, base);
  

•   unsigned long cur_id;
  

•   cur_id = atomic_long_read(&m->reservation_id);
  

•   if (!cur_id)
  

•           return 0;
  

•   if (unlikely(reservation_id == cur_id))
  

•           return -EDEADLK;
  

•   if (unlikely(reservation_id - cur_id <= LONG_MAX))
  

•           return -EAGAIN;
  

•   return 0;
  

+}

+/*

• • after acquiring lock with fastpath or when we lost out in contested
• • slowpath, set reservation_id and wake up any waiters so they can recheck.
• */

I think that is a bit misleading, if I'm understanding correctly this is called once you get the lock (but in either fast or slow path)

Yes, but strictly speaking it does not need to be called on slow path, it will do an atomic_xchg to set reservation_id, see that it had already set reservation_id, and skip the rest. :-)

Maybe I should just return !__builtin_constant_p(reservation_id) in __mutex_lock_common to distinguish between fastpath and slowpath. That would cause __mutex_lock_common to return 0 for normal mutexes, 1 when reservation_id is set and slowpath is used. This would allow me to check whether mutex_set_reservation_fastpath needs to be called or not, and tighten up lockdep detection of mismatched mutex_reserve_lock / mutex_lock with mutex_unlock / mutex_reserve_unlock.

~Maarte

Op 22-01-13 16:13, Francesco Lavra schreef:

Hi,

On 01/15/2013 01:34 PM, Maarten Lankhorst wrote: [...]

...

diff --git a/include/linux/fence.h b/include/linux/fence.h new file mode 100644 index 0000000..d9f091d --- /dev/null +++ b/include/linux/fence.h @@ -0,0 +1,347 @@ +/*

• • Fence mechanism for dma-buf to allow for asynchronous dma access
• • Copyright (C) 2012 Canonical Ltd
• • Copyright (C) 2012 Texas Instruments
• • Authors:
• • Rob Clark rob.clark@linaro.org
• • Maarten Lankhorst maarten.lankhorst@canonical.com
• • 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.
• • You should have received a copy of the GNU General Public License along with
• • this program. If not, see http://www.gnu.org/licenses/.
• */

+#ifndef __LINUX_FENCE_H +#define __LINUX_FENCE_H

+#include <linux/err.h> +#include <linux/wait.h> +#include <linux/list.h> +#include <linux/bitops.h> +#include <linux/kref.h> +#include <linux/sched.h> +#include <linux/printk.h>

+struct fence; +struct fence_ops; +struct fence_cb;

+/**

• • struct fence - software synchronization primitive
• • @refcount: refcount for this fence
• • @ops: fence_ops associated with this fence
• • @cb_list: list of all callbacks to call
• • @lock: spin_lock_irqsave used for locking
• • @priv: fence specific private data
• • @flags: A mask of FENCE_FLAG_* defined below
• • the flags member must be manipulated and read using the appropriate
• • atomic ops (bit_*), so taking the spinlock will not be needed most
• • of the time.
• • FENCE_FLAG_SIGNALED_BIT - fence is already signaled
• • FENCE_FLAG_ENABLE_SIGNAL_BIT - enable_signaling might have been called*
• • FENCE_FLAG_USER_BITS - start of the unused bits, can be used by the
• • implementer of the fence for its own purposes. Can be used in different
• • ways by different fence implementers, so do not rely on this.
• • *) Since atomic bitops are used, this is not guaranteed to be the case.
• • Particularly, if the bit was set, but fence_signal was called right
• • before this bit was set, it would have been able to set the
• • FENCE_FLAG_SIGNALED_BIT, before enable_signaling was called.
• • Adding a check for FENCE_FLAG_SIGNALED_BIT after setting
• • FENCE_FLAG_ENABLE_SIGNAL_BIT closes this race, and makes sure that
• • after fence_signal was called, any enable_signaling call will have either
• • been completed, or never called at all.
• */

+struct fence {

• struct kref refcount;
• const struct fence_ops *ops;
• struct list_head cb_list;
• spinlock_t *lock;
• unsigned context, seqno;
• unsigned long flags;

+};

The documentation above should be updated with the new structure members context and seqno.

...

+enum fence_flag_bits {

• FENCE_FLAG_SIGNALED_BIT,
• FENCE_FLAG_ENABLE_SIGNAL_BIT,
• FENCE_FLAG_USER_BITS, /* must always be last member */

+};

+typedef void (*fence_func_t)(struct fence *fence, struct fence_cb *cb, void *priv);

+/**

• • struct fence_cb - callback for fence_add_callback
• • @func: fence_func_t to call
• • @priv: value of priv to pass to function
• • This struct will be initialized by fence_add_callback, additional
• • data can be passed along by embedding fence_cb in another struct.
• */

+struct fence_cb {

• struct list_head node;
• fence_func_t func;
• void *priv;

+};

Documentation should be updated here too.

...

+/**

• • struct fence_ops - operations implemented for fence
• • @enable_signaling: enable software signaling of fence
• • @signaled: [optional] peek whether the fence is signaled
• • @release: [optional] called on destruction of fence
• • Notes on enable_signaling:
• • For fence implementations that have the capability for hw->hw
• • signaling, they can implement this op to enable the necessary
• • irqs, or insert commands into cmdstream, etc. This is called
• • in the first wait() or add_callback() path to let the fence
• • implementation know that there is another driver waiting on
• • the signal (ie. hw->sw case).
• • This function can be called called from atomic context, but not
• • from irq context, so normal spinlocks can be used.
• • A return value of false indicates the fence already passed,
• • or some failure occured that made it impossible to enable
• • signaling. True indicates succesful enabling.
• • Calling fence_signal before enable_signaling is called allows
• • for a tiny race window in which enable_signaling is called during,
• • before, or after fence_signal. To fight this, it is recommended
• • that before enable_signaling returns true an extra reference is
• • taken on the fence, to be released when the fence is signaled.
• • This will mean fence_signal will still be called twice, but
• • the second time will be a noop since it was already signaled.
• • Notes on release:
• • Can be NULL, this function allows additional commands to run on
• • destruction of the fence. Can be called from irq context.
• • If pointer is set to NULL, kfree will get called instead.
• */

+struct fence_ops {

• bool (*enable_signaling)(struct fence *fence);
• bool (*signaled)(struct fence *fence);
• long (*wait)(struct fence *fence, bool intr, signed long);
• void (*release)(struct fence *fence);

+};

Ditto.

...

+/**

• • __fence_init - Initialize a custom fence.
• • @fence: [in] the fence to initialize
• • @ops: [in] the fence_ops for operations on this fence
• • @lock: [in] the irqsafe spinlock to use for locking this fence
• • @context: [in] the execution context this fence is run on
• • @seqno: [in] a linear increasing sequence number for this context
• • Initializes an allocated fence, the caller doesn't have to keep its
• • refcount after committing with this fence, but it will need to hold a
• • refcount again if fence_ops.enable_signaling gets called. This can
• • be used for other implementing other types of fence.
• • context and seqno are used for easy comparison between fences, allowing
• • to check which fence is later by simply using fence_later.
• */

+static inline void +__fence_init(struct fence *fence, const struct fence_ops *ops,

•     spinlock_t *lock, unsigned context, unsigned seqno)
  

+{

• BUG_ON(!ops || !lock || !ops->enable_signaling || !ops->wait);
• kref_init(&fence->refcount);
• fence->ops = ops;
• INIT_LIST_HEAD(&fence->cb_list);
• fence->lock = lock;
• fence->context = context;
• fence->seqno = seqno;
• fence->flags = 0UL;

+}

+/**

• • fence_get - increases refcount of the fence
• • @fence: [in] fence to increase refcount of
• */

+static inline void fence_get(struct fence *fence) +{

• if (WARN_ON(!fence))

• return;
  

• kref_get(&fence->refcount);

+}

+extern void release_fence(struct kref *kref);

+/**

• • fence_put - decreases refcount of the fence
• • @fence: [in] fence to reduce refcount of
• */

+static inline void fence_put(struct fence *fence) +{

• if (WARN_ON(!fence))

• return;
  

• kref_put(&fence->refcount, release_fence);

+}

+int fence_signal(struct fence *fence); +int __fence_signal(struct fence *fence); +long fence_default_wait(struct fence *fence, bool intr, signed long);

In the parameter list the first two parameters are named, and the last one isn't. Feels a bit odd...

...

+int fence_add_callback(struct fence *fence, struct fence_cb *cb,

•        fence_func_t func, void *priv);
  

+bool fence_remove_callback(struct fence *fence, struct fence_cb *cb); +void fence_enable_sw_signaling(struct fence *fence);

+/**

• • fence_is_signaled - Return an indication if the fence is signaled yet.
• • @fence: [in] the fence to check
• • Returns true if the fence was already signaled, false if not. Since this
• • function doesn't enable signaling, it is not guaranteed to ever return true
• • If fence_add_callback, fence_wait or fence_enable_sw_signaling
• • haven't been called before.
• • It's recommended for seqno fences to call fence_signal when the
• • operation is complete, it makes it possible to prevent issues from
• • wraparound between time of issue and time of use by checking the return
• • value of this function before calling hardware-specific wait instructions.
• */

+static inline bool +fence_is_signaled(struct fence *fence) +{

• if (test_bit(FENCE_FLAG_SIGNALED_BIT, &fence->flags))

• return true;
  

• if (fence->ops->signaled && fence->ops->signaled(fence)) {

• fence_signal(fence);
  

• return true;
  

• }
• return false;

+}

+/**

• • fence_later - return the chronologically later fence
• • @f1: [in] the first fence from the same context
• • @f2: [in] the second fence from the same context
• • Returns NULL if both fences are signaled, otherwise the fence that would be
• • signaled last. Both fences must be from the same context, since a seqno is
• • not re-used across contexts.
• */

+static inline struct fence *fence_later(struct fence *f1, struct fence *f2) +{

• bool sig1, sig2;
• /*

• * can't check just FENCE_FLAG_SIGNALED_BIT here, it may never have been
  

• * set called if enable_signaling wasn't, and enabling that here is
  

• * overkill.
  

• */
  

• sig1 = fence_is_signaled(f1);
• sig2 = fence_is_signaled(f2);
• if (sig1 && sig2)

• return NULL;
  

• BUG_ON(f1->context != f2->context);
• if (sig1 || f2->seqno - f2->seqno <= INT_MAX)

I guess you meant (f2->seqno - f1->seqno).

...

• return f2;
  

• else

• return f1;
  

+}

Regards, Francesco

Thanks for the review, how does this delta look?

diff --git a/include/linux/fence.h b/include/linux/fence.h index d9f091d..831ed0a 100644 --- a/include/linux/fence.h +++ b/include/linux/fence.h @@ -42,7 +42,10 @@ struct fence_cb; * @ops: fence_ops associated with this fence * @cb_list: list of all callbacks to call * @lock: spin_lock_irqsave used for locking - * @priv: fence specific private data + * @context: execution context this fence belongs to, returned by + * fence_context_alloc() + * @seqno: the sequence number of this fence inside the executation context, + * can be compared to decide which fence would be signaled later. * @flags: A mask of FENCE_FLAG_* defined below * * the flags member must be manipulated and read using the appropriate @@ -83,6 +86,7 @@ typedef void (*fence_func_t)(struct fence *fence, struct fence_cb *cb, void *pri

/** * struct fence_cb - callback for fence_add_callback + * @node: used by fence_add_callback to append this struct to fence::cb_list * @func: fence_func_t to call * @priv: value of priv to pass to function * @@ -98,8 +102,9 @@ struct fence_cb { /** * struct fence_ops - operations implemented for fence * @enable_signaling: enable software signaling of fence - * @signaled: [optional] peek whether the fence is signaled - * @release: [optional] called on destruction of fence + * @signaled: [optional] peek whether the fence is signaled, can be null + * @wait: custom wait implementation + * @release: [optional] called on destruction of fence, can be null * * Notes on enable_signaling: * For fence implementations that have the capability for hw->hw @@ -124,6 +129,16 @@ struct fence_cb { * This will mean fence_signal will still be called twice, but * the second time will be a noop since it was already signaled. * + * Notes on wait: + * Must not be NULL, set to fence_default_wait for default implementation. + * the fence_default_wait implementation should work for any fence, as long + * as enable_signaling works correctly. + * + * Must return -ERESTARTSYS if the wait is intr = true and the wait was interrupted, + * and remaining jiffies if fence has signaled. Can also return other error + * values on custom implementations, which should be treated as if the fence + * is signaled. For example a hardware lockup could be reported like that. + * * Notes on release: * Can be NULL, this function allows additional commands to run on * destruction of the fence. Can be called from irq context. @@ -133,7 +148,7 @@ struct fence_cb { struct fence_ops { bool (*enable_signaling)(struct fence *fence); bool (*signaled)(struct fence *fence); - long (*wait)(struct fence *fence, bool intr, signed long); + long (*wait)(struct fence *fence, bool intr, signed long timeout); void (*release)(struct fence *fence); };

@@ -194,7 +209,7 @@ static inline void fence_put(struct fence *fence)

int fence_signal(struct fence *fence); int __fence_signal(struct fence *fence); -long fence_default_wait(struct fence *fence, bool intr, signed long); +long fence_default_wait(struct fence *fence, bool intr, signed long timeout); int fence_add_callback(struct fence *fence, struct fence_cb *cb, fence_func_t func, void *priv); bool fence_remove_callback(struct fence *fence, struct fence_cb *cb); @@ -254,7 +269,7 @@ static inline struct fence *fence_later(struct fence *f1, struct fence *f2)

BUG_ON(f1->context != f2->context);

- if (sig1 || f2->seqno - f2->seqno <= INT_MAX) + if (sig1 || f2->seqno - f1->seqno <= INT_MAX) return f2; else return f1;

2013/1/15 Maarten Lankhorst m.b.lankhorst@gmail.com:

This type of fence can be used with hardware synchronization for simple hardware that can block execution until the condition (dma_buf[offset] - value) >= 0 has been met.

A software fallback still has to be provided in case the fence is used with a device that doesn't support this mechanism. It is useful to expose this for graphics cards that have an op to support this.

Some cards like i915 can export those, but don't have an option to wait, so they need the software fallback.

I extended the original patch by Rob Clark.

v1: Original v2: Renamed from bikeshed to seqno, moved into dma-fence.c since not much was left of the file. Lots of documentation added. v3: Use fence_ops instead of custom callbacks. Moved to own file to avoid circular dependency between dma-buf.h and fence.h v4: Add spinlock pointer to seqno_fence_init

Signed-off-by: Maarten Lankhorst maarten.lankhorst@canonical.com

Documentation/DocBook/device-drivers.tmpl | 1 + drivers/base/fence.c | 38 +++++++++++ include/linux/seqno-fence.h | 105 ++++++++++++++++++++++++++++++ 3 files changed, 144 insertions(+) create mode 100644 include/linux/seqno-fence.h

diff --git a/Documentation/DocBook/device-drivers.tmpl b/Documentation/DocBook/device-drivers.tmpl index 6f53fc0..ad14396 100644 --- a/Documentation/DocBook/device-drivers.tmpl +++ b/Documentation/DocBook/device-drivers.tmpl @@ -128,6 +128,7 @@ X!Edrivers/base/interface.c !Edrivers/base/dma-buf.c !Edrivers/base/fence.c !Iinclude/linux/fence.h +!Iinclude/linux/seqno-fence.h !Edrivers/base/dma-coherent.c !Edrivers/base/dma-mapping.c </sect1> diff --git a/drivers/base/fence.c b/drivers/base/fence.c index 28e5ffd..1d3f29c 100644 --- a/drivers/base/fence.c +++ b/drivers/base/fence.c @@ -24,6 +24,7 @@ #include <linux/slab.h> #include <linux/export.h> #include <linux/fence.h> +#include <linux/seqno-fence.h>

atomic_t fence_context_counter = ATOMIC_INIT(0); EXPORT_SYMBOL(fence_context_counter); @@ -284,3 +285,40 @@ out: return ret; } EXPORT_SYMBOL(fence_default_wait);

+static bool seqno_enable_signaling(struct fence *fence) +{

•   struct seqno_fence *seqno_fence = to_seqno_fence(fence);
  

•   return seqno_fence->ops->enable_signaling(fence);
  

+}

+static bool seqno_signaled(struct fence *fence) +{

•   struct seqno_fence *seqno_fence = to_seqno_fence(fence);
  

•   return seqno_fence->ops->signaled && seqno_fence->ops->signaled(fence);
  

+}

+static void seqno_release(struct fence *fence) +{

•   struct seqno_fence *f = to_seqno_fence(fence);
  

•   dma_buf_put(f->sync_buf);
  

•   if (f->ops->release)
  

•           f->ops->release(fence);
  

•   else
  

•           kfree(f);
  

+}

+static long seqno_wait(struct fence *fence, bool intr, signed long timeout) +{

•   struct seqno_fence *f = to_seqno_fence(fence);
  

•   return f->ops->wait(fence, intr, timeout);
  

+}

+const struct fence_ops seqno_fence_ops = {

•   .enable_signaling = seqno_enable_signaling,
  

•   .signaled = seqno_signaled,
  

•   .wait = seqno_wait,
  

•   .release = seqno_release
  

+}; +EXPORT_SYMBOL_GPL(seqno_fence_ops); diff --git a/include/linux/seqno-fence.h b/include/linux/seqno-fence.h new file mode 100644 index 0000000..603adc0 --- /dev/null +++ b/include/linux/seqno-fence.h @@ -0,0 +1,105 @@ +/*

• • seqno-fence, using a dma-buf to synchronize fencing
• • Copyright (C) 2012 Texas Instruments
• • Copyright (C) 2012 Canonical Ltd
• • Authors:
• • Rob Clark rob.clark@linaro.org
• • Maarten Lankhorst maarten.lankhorst@canonical.com
• • 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.
• • You should have received a copy of the GNU General Public License along with
• • this program. If not, see http://www.gnu.org/licenses/.
• */

+#ifndef __LINUX_SEQNO_FENCE_H +#define __LINUX_SEQNO_FENCE_H

+#include <linux/fence.h> +#include <linux/dma-buf.h>

+struct seqno_fence {

•   struct fence base;
  

•   const struct fence_ops *ops;
  

•   struct dma_buf *sync_buf;
  

•   uint32_t seqno_ofs;
  

+};

Hi maarten,

I'm applying dma-fence v11 and seqno-fence v4 to exynos drm and have some proposals.

The above seqno_fence structure has only one dmabuf. Shouldn't it have mutiple dmabufs? For example, in case of drm driver, when pageflip is requested, one framebuffer could have one more gem buffer for NV12M format. And this means that one more exported dmabufs should be sychronized with other devices. Below is simple structure for it,

struct seqno_fence_dmabuf { struct list_head list; int id; struct dmabuf *sync_buf; uint32_t seqno_ops; uint32_t seqno; };

The member, id, could be used to identify which device sync_buf is going to be accessed by. In case of drm driver, one framebuffer could be accessed by one more devices, one is Display controller and another is HDMI controller. So id would have crtc number.

And seqno_fence structure could be defined like below,

struct seqno_fence { struct list_head sync_buf_list; struct fence base; const struct fence_ops *ops; };

In addition, I have implemented fence-helper framework for sw sync as WIP and below is intefaces for it,

struct fence_helper { struct list_head entries; struct reservation_ticket ticket; struct seqno_fence *sf; spinlock_t lock; void *priv; };

int fence_helper_init(struct fence_helper *fh, void *priv, void (*remease)(struct fence *fence)); - This function is called at driver open so process unique context would have a new seqno_fence instance. This function does just seqno_fence_init call, initialize entries list and set device specific fence release callback.

bool fence_helper_check_sync_buf(struct fence_helper *fh, struct dma_buf *sync_buf, int id); - This function is called before dma is started and checks if same sync_bufs had already be committed to reservation_object, bo->fence_shared[n]. And id could be used to identy which device sync_buf is going to be accessed by.

int fence_helper_add_sync_buf(struct fence_helper *fh, struct dma_buf *sync_buf, int id); - This function is called if fence_helper_check_sync_buf() is true and adds it seqno_fence's sync_buf_list wrapping sync_buf as seqno_fence_dma_buf structure. With this function call, one seqno_fence instance would have more sync_bufs. At this time, the reference count to this sync_buf is taken.

void fence_helper_del_sync_buf(struct fence_helper *fh, int id); - This function is called if some operation is failed after fence_helper_add_sync_buf call to release relevant resources.

int fence_helper_init_reservation_entry(struct fence_helper *fh, struct dma_buf *dmabuf, bool shared, int id); - This function is called after fence_helper_add_sync_buf call and calls reservation_entry_init function to set a reservation object of sync_buf to a new reservation_entry object. And then the new reservation_entry is added to fh->entries to track all sync_bufs this device is going to access.

void fence_helper_fini_reservation_entry(struct fence_helper *fh, int id); - This function is called if some operation is failed after fence_helper_init_reservation_entry call to releae relevant resources.

int fence_helper_ticket_reserve(struct fence_helper *fh, int id); - This function is called after fence_helper_init_reservation_entry call and calls ticket_reserve function to reserve a ticket(locked for each reservation entry in fh->entires)

void fence_helper_ticket_commit(struct fence_helper *fh, int id); - This function is called after fence_helper_ticket_reserve() is called to commit this device's fence to all reservation_objects of each sync_buf. After that, once other devices try to access these buffers, they would be blocked and unlock each reservation entry in fh->entires.

int fence_helper_wait(struct fence_helper *fh, struct dma_buf *dmabuf, bool intr); - This function is called before fence_helper_add_sync_buf() is called to wait for a signal from other devices.

int fence_helper_signal(struct fence_helper *fh, int id); - This function is called by device's interrupt handler or somewhere when dma access to this buffer has been completed and calls fence_signal() with each fence registed to each reservation object in fh->entries to notify dma access completion to other deivces. At this time, other devices blocked would be waked up and forward their next step.

For more detail, in addition, this function does the following, - delete each reservation entry in fh->entries. - release each seqno_fence_dmabuf object in seqno_fence's sync_buf_list and call dma_buf_put() to put the reference count to dmabuf.

Now the fence-helper framework is just WIP yet so there may be my missing points. If you are ok, I'd like to post it as RFC.

Thanks, Inki Dae

+extern const struct fence_ops seqno_fence_ops;

+/**

• • to_seqno_fence - cast a fence to a seqno_fence
• • @fence: fence to cast to a seqno_fence
• • Returns NULL if the fence is not a seqno_fence,
• • or the seqno_fence otherwise.
• */

+static inline struct seqno_fence * +to_seqno_fence(struct fence *fence) +{

•   if (fence->ops != &seqno_fence_ops)
  

•           return NULL;
  

•   return container_of(fence, struct seqno_fence, base);
  

+}

+/**

• • seqno_fence_init - initialize a seqno fence
• • @fence: seqno_fence to initialize
• • @lock: pointer to spinlock to use for fence
• • @sync_buf: buffer containing the memory location to signal on
• • @context: the execution context this fence is a part of
• • @seqno_ofs: the offset within @sync_buf
• • @seqno: the sequence # to signal on
• • @ops: the fence_ops for operations on this seqno fence
• • This function initializes a struct seqno_fence with passed parameters,
• • and takes a reference on sync_buf which is released on fence destruction.
• • A seqno_fence is a dma_fence which can complete in software when
• • enable_signaling is called, but it also completes when
• • (s32)((sync_buf)[seqno_ofs] - seqno) >= 0 is true
• • The seqno_fence will take a refcount on the sync_buf until it's
• • destroyed, but actual lifetime of sync_buf may be longer if one of the
• • callers take a reference to it.
• • Certain hardware have instructions to insert this type of wait condition
• • in the command stream, so no intervention from software would be needed.
• • This type of fence can be destroyed before completed, however a reference
• • on the sync_buf dma-buf can be taken. It is encouraged to re-use the same
• • dma-buf for sync_buf, since mapping or unmapping the sync_buf to the
• • device's vm can be expensive.
• • It is recommended for creators of seqno_fence to call fence_signal
• • before destruction. This will prevent possible issues from wraparound at
• • time of issue vs time of check, since users can check fence_is_signaled
• • before submitting instructions for the hardware to wait on the fence.
• • However, when ops.enable_signaling is not called, it doesn't have to be
• • done as soon as possible, just before there's any real danger of seqno
• • wraparound.
• */

+static inline void +seqno_fence_init(struct seqno_fence *fence, spinlock_t *lock,

•            struct dma_buf *sync_buf,  uint32_t context, uint32_t seqno_ofs,
  

•            uint32_t seqno, const struct fence_ops *ops)
  

+{

•   BUG_ON(!fence || !sync_buf || !ops->enable_signaling || !ops->wait);
  

•   __fence_init(&fence->base, &seqno_fence_ops, lock, context, seqno);
  

•   get_dma_buf(sync_buf);
  

•   fence->ops = ops;
  

•   fence->sync_buf = sync_buf;
  

•   fence->seqno_ofs = seqno_ofs;
  

+}

+#endif /* __LINUX_SEQNO_FENCE_H */

1.8.0.3

---

Linaro-mm-sig mailing list Linaro-mm-sig@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-mm-sig

2013/1/16 Maarten Lankhorst maarten.lankhorst@canonical.com:

Op 16-01-13 07:28, Inki Dae schreef:

...

2013/1/15 Maarten Lankhorst m.b.lankhorst@gmail.com:

...

This type of fence can be used with hardware synchronization for simple hardware that can block execution until the condition (dma_buf[offset] - value) >= 0 has been met.

A software fallback still has to be provided in case the fence is used with a device that doesn't support this mechanism. It is useful to expose this for graphics cards that have an op to support this.

Some cards like i915 can export those, but don't have an option to wait, so they need the software fallback.

I extended the original patch by Rob Clark.

v1: Original v2: Renamed from bikeshed to seqno, moved into dma-fence.c since not much was left of the file. Lots of documentation added. v3: Use fence_ops instead of custom callbacks. Moved to own file to avoid circular dependency between dma-buf.h and fence.h v4: Add spinlock pointer to seqno_fence_init

Signed-off-by: Maarten Lankhorst maarten.lankhorst@canonical.com

Documentation/DocBook/device-drivers.tmpl | 1 + drivers/base/fence.c | 38 +++++++++++ include/linux/seqno-fence.h | 105 ++++++++++++++++++++++++++++++ 3 files changed, 144 insertions(+) create mode 100644 include/linux/seqno-fence.h

diff --git a/Documentation/DocBook/device-drivers.tmpl b/Documentation/DocBook/device-drivers.tmpl index 6f53fc0..ad14396 100644 --- a/Documentation/DocBook/device-drivers.tmpl +++ b/Documentation/DocBook/device-drivers.tmpl @@ -128,6 +128,7 @@ X!Edrivers/base/interface.c !Edrivers/base/dma-buf.c !Edrivers/base/fence.c !Iinclude/linux/fence.h +!Iinclude/linux/seqno-fence.h !Edrivers/base/dma-coherent.c !Edrivers/base/dma-mapping.c </sect1> diff --git a/drivers/base/fence.c b/drivers/base/fence.c index 28e5ffd..1d3f29c 100644 --- a/drivers/base/fence.c +++ b/drivers/base/fence.c @@ -24,6 +24,7 @@ #include <linux/slab.h> #include <linux/export.h> #include <linux/fence.h> +#include <linux/seqno-fence.h>

atomic_t fence_context_counter = ATOMIC_INIT(0); EXPORT_SYMBOL(fence_context_counter); @@ -284,3 +285,40 @@ out: return ret; } EXPORT_SYMBOL(fence_default_wait);

+static bool seqno_enable_signaling(struct fence *fence) +{

•   struct seqno_fence *seqno_fence = to_seqno_fence(fence);
  

•   return seqno_fence->ops->enable_signaling(fence);
  

+}

+static bool seqno_signaled(struct fence *fence) +{

•   struct seqno_fence *seqno_fence = to_seqno_fence(fence);
  

•   return seqno_fence->ops->signaled && seqno_fence->ops->signaled(fence);
  

+}

+static void seqno_release(struct fence *fence) +{

•   struct seqno_fence *f = to_seqno_fence(fence);
  

•   dma_buf_put(f->sync_buf);
  

•   if (f->ops->release)
  

•           f->ops->release(fence);
  

•   else
  

•           kfree(f);
  

+}

+static long seqno_wait(struct fence *fence, bool intr, signed long timeout) +{

•   struct seqno_fence *f = to_seqno_fence(fence);
  

•   return f->ops->wait(fence, intr, timeout);
  

+}

+const struct fence_ops seqno_fence_ops = {

•   .enable_signaling = seqno_enable_signaling,
  

•   .signaled = seqno_signaled,
  

•   .wait = seqno_wait,
  

•   .release = seqno_release
  

+}; +EXPORT_SYMBOL_GPL(seqno_fence_ops); diff --git a/include/linux/seqno-fence.h b/include/linux/seqno-fence.h new file mode 100644 index 0000000..603adc0 --- /dev/null +++ b/include/linux/seqno-fence.h @@ -0,0 +1,105 @@ +/*

• • seqno-fence, using a dma-buf to synchronize fencing
• • Copyright (C) 2012 Texas Instruments
• • Copyright (C) 2012 Canonical Ltd
• • Authors:
• • Rob Clark rob.clark@linaro.org
• • Maarten Lankhorst maarten.lankhorst@canonical.com
• • 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.
• • You should have received a copy of the GNU General Public License along with
• • this program. If not, see http://www.gnu.org/licenses/.
• */

+#ifndef __LINUX_SEQNO_FENCE_H +#define __LINUX_SEQNO_FENCE_H

+#include <linux/fence.h> +#include <linux/dma-buf.h>

+struct seqno_fence {

•   struct fence base;
  

•   const struct fence_ops *ops;
  

•   struct dma_buf *sync_buf;
  

•   uint32_t seqno_ofs;
  

+};

Hi maarten,

I'm applying dma-fence v11 and seqno-fence v4 to exynos drm and have some proposals.

The above seqno_fence structure has only one dmabuf. Shouldn't it have mutiple dmabufs? For example, in case of drm driver, when pageflip is requested, one framebuffer could have one more gem buffer for NV12M format. And this means that one more exported dmabufs should be sychronized with other devices. Below is simple structure for it,

The fence guards a single operation, as such I didn't feel like more than one dma-buf was needed to guard it.

Have you considered simply attaching multiple fences instead? Each with their own dma-buf.

I thought each context per device should have one fence. If not so, I think we could use multiple fences instead.

There has been some muttering about allowing multiple exclusive fences to be attached, for arm soc's.

But I'm also considering getting rid of the dma-buf member and add a function call to retrieve it, since the sync dma-buf member should not be changing often, and it would zap 2 atomic ops on every fence, but I want it replaced by something that's not 10x more complicated.

Maybe "int get_sync_dma_buf(fence, old_dma_buf, &new_dma_buf)" that will set new_dma_buf = NULL if the old_dma_buf is unchanged, and return true + return a new reference to the sync dma_buf if it's not identical to old_dma_buf. old_dma_buf can also be NULL or a dma_buf that belongs to a different fence->context entirely. It might be capable of returning an error, in which case the fence would count as being signaled. This could reduce the need for separately checking fence_is_signaled first.

I think this would allow caching the synchronization dma_buf in a similar way without each fence needing to hold a reference to the dma_buf all the time, even for fences that are only used internally.

...

struct seqno_fence_dmabuf { struct list_head list; int id; struct dmabuf *sync_buf; uint32_t seqno_ops; uint32_t seqno; };

The member, id, could be used to identify which device sync_buf is going to be accessed by. In case of drm driver, one framebuffer could be accessed by one more devices, one is Display controller and another is HDMI controller. So id would have crtc number.

Why do you need this? the base fence already has a context member.

In fact I don't see why you need a linked list, at worst you'd need a static array since the amount of dma-bufs should already be known during allocation time.

I would prefer to simply make reservation_object->fence_exclusive an array, since it would be easier to implement, and there have been some calls that arm might need such a thing.

Right, the array could be used instead. I just had quick implemention so it's not perfect.

...

And seqno_fence structure could be defined like below,

struct seqno_fence { struct list_head sync_buf_list; struct fence base; const struct fence_ops *ops; };

In addition, I have implemented fence-helper framework for sw sync as WIP and below is intefaces for it,

struct fence_helper { struct list_head entries; struct reservation_ticket ticket; struct seqno_fence *sf; spinlock_t lock; void *priv; };

int fence_helper_init(struct fence_helper *fh, void *priv, void (*remease)(struct fence *fence));

• This function is called at driver open so process unique context

would have a new seqno_fence instance. This function does just seqno_fence_init call, initialize entries list and set device specific fence release callback.

bool fence_helper_check_sync_buf(struct fence_helper *fh, struct dma_buf *sync_buf, int id);

• This function is called before dma is started and checks if same

sync_bufs had already be committed to reservation_object, bo->fence_shared[n]. And id could be used to identy which device sync_buf is going to be accessed by.

int fence_helper_add_sync_buf(struct fence_helper *fh, struct dma_buf *sync_buf, int id);

• This function is called if fence_helper_check_sync_buf() is true and

adds it seqno_fence's sync_buf_list wrapping sync_buf as seqno_fence_dma_buf structure. With this function call, one seqno_fence instance would have more sync_bufs. At this time, the reference count to this sync_buf is taken.

void fence_helper_del_sync_buf(struct fence_helper *fh, int id);

• This function is called if some operation is failed after

fence_helper_add_sync_buf call to release relevant resources.

int fence_helper_init_reservation_entry(struct fence_helper *fh, struct dma_buf *dmabuf, bool shared, int id);

• This function is called after fence_helper_add_sync_buf call and

calls reservation_entry_init function to set a reservation object of sync_buf to a new reservation_entry object. And then the new reservation_entry is added to fh->entries to track all sync_bufs this device is going to access.

void fence_helper_fini_reservation_entry(struct fence_helper *fh, int id);

• This function is called if some operation is failed after

fence_helper_init_reservation_entry call to releae relevant resources.

int fence_helper_ticket_reserve(struct fence_helper *fh, int id);

• This function is called after fence_helper_init_reservation_entry

call and calls ticket_reserve function to reserve a ticket(locked for each reservation entry in fh->entires)

void fence_helper_ticket_commit(struct fence_helper *fh, int id);

• This function is called after fence_helper_ticket_reserve() is

called to commit this device's fence to all reservation_objects of each sync_buf. After that, once other devices try to access these buffers, they would be blocked and unlock each reservation entry in fh->entires.

int fence_helper_wait(struct fence_helper *fh, struct dma_buf *dmabuf, bool intr);

• This function is called before fence_helper_add_sync_buf() is called

to wait for a signal from other devices.

int fence_helper_signal(struct fence_helper *fh, int id);

• This function is called by device's interrupt handler or somewhere

when dma access to this buffer has been completed and calls fence_signal() with each fence registed to each reservation object in fh->entries to notify dma access completion to other deivces. At this time, other devices blocked would be waked up and forward their next step.

For more detail, in addition, this function does the following,

• delete each reservation entry in fh->entries.
• release each seqno_fence_dmabuf object in seqno_fence's

sync_buf_list and call dma_buf_put() to put the reference count to dmabuf.

Now the fence-helper framework is just WIP yet so there may be my missing points. If you are ok, I'd like to post it as RFC.

Way too complicated..

The purpose to the fence-helper is to use the dma-fence more simply. With the fence-helper, we doesn't need to consider fence and reservation interfaces for it. All we have to do is to call only the fence-helper interfaces without considering two things(fence and reservation).

For example(In consumer case),

driver_open() { struct fence-helper *fh; ... ctx->fh = kzalloc(); ... fence_helper_init(fh, ...); }

driver_addfb() { ... fence_helper_add_sync_buf(fh, sync_buf, ...); ... }

driver_pageflip() { ... fence_helper_wait(fh, sync_buf, ...); fence_helper_init_reservation_entry(fh, sync_buf, ...); fence_helper_ticket_reserve(fh, ...); fence_helper_ticket_commit(fh, ...); ... }

driver_pageflip_handler() { ... fence_helper_signal(fh, ...); }

The above functions are called in the following order, 1. driver_open() -> 2. driver_addfb() -> 3. driver_pageflip() -> 4.driver_pageflip_handler()

Step 3 and 4 would be called repeatedly. And also producer could use similar way.

I'm not sure that I understand the dma-fence framework fully so there might be something wrong and better way.

Thanks, Inki Dae

---

dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel

Op 22-01-13 17:47, Francesco Lavra schreef:

On 01/15/2013 01:34 PM, Maarten Lankhorst wrote:

...

This adds support for a generic reservations framework that can be hooked up to ttm and dma-buf and allows easy sharing of reservations across devices.

The idea is that a dma-buf and ttm object both will get a pointer to a struct reservation_object, which has to be reserved before anything is done with the contents of the dma-buf.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@canonical.com

Documentation/DocBook/device-drivers.tmpl | 2 + drivers/base/Makefile | 2 +- drivers/base/reservation.c | 251 ++++++++++++++++++++++++++++++ include/linux/reservation.h | 182 ++++++++++++++++++++++ 4 files changed, 436 insertions(+), 1 deletion(-) create mode 100644 drivers/base/reservation.c create mode 100644 include/linux/reservation.h

[...]

...

diff --git a/include/linux/reservation.h b/include/linux/reservation.h new file mode 100644 index 0000000..fc2349d --- /dev/null +++ b/include/linux/reservation.h @@ -0,0 +1,182 @@ +/*

• • Header file for reservations for dma-buf and ttm
• • Copyright(C) 2011 Linaro Limited. All rights reserved.
• • Copyright (C) 2012 Canonical Ltd
• • Copyright (C) 2012 Texas Instruments
• • Authors:
• • Rob Clark rob.clark@linaro.org
• • Maarten Lankhorst maarten.lankhorst@canonical.com
• • Thomas Hellstrom <thellstrom-at-vmware-dot-com>
• • Based on bo.c which bears the following copyright notice,
• • but is dual licensed:
• • Copyright (c) 2006-2009 VMware, Inc., Palo Alto, CA., USA
• • All Rights Reserved.
• • Permission is hereby granted, free of charge, to any person obtaining a
• • copy of this software and associated documentation files (the
• • "Software"), to deal in the Software without restriction, including
• • without limitation the rights to use, copy, modify, merge, publish,
• • distribute, sub license, and/or sell copies of the Software, and to
• • permit persons to whom the Software is furnished to do so, subject to
• • the following conditions:
• • The above copyright notice and this permission notice (including the
• • next paragraph) shall be included in all copies or substantial portions
• • of the Software.
• • THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
• • IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
• • FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
• • THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM,
• • DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
• • OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
• • USE OR OTHER DEALINGS IN THE SOFTWARE.
• */

+#ifndef _LINUX_RESERVATION_H +#define _LINUX_RESERVATION_H

+#include <linux/spinlock.h> +#include <linux/mutex.h> +#include <linux/fence.h>

+#define BUF_MAX_SHARED_FENCE 8

+struct fence;

+extern atomic_long_t reservation_counter; +extern const char reservation_object_name[]; +extern struct lock_class_key reservation_object_class; +extern const char reservation_ticket_name[]; +extern struct lock_class_key reservation_ticket_class;

+struct reservation_object {

• struct ticket_mutex lock;
• u32 fence_shared_count;
• struct fence *fence_excl;
• struct fence *fence_shared[BUF_MAX_SHARED_FENCE];

+};

+struct reservation_ticket {

• unsigned long seqno;

+#ifdef CONFIG_DEBUG_LOCK_ALLOC

• struct lockdep_map dep_map;

+#endif +};

+/**

• • struct reservation_entry - reservation structure for a
• • reservation_object
• • @head: list entry
• • @obj_shared: pointer to a reservation_object to reserve
• • Bit 0 of obj_shared is set to bool shared, as such pointer has to be
• • converted back, which can be done with reservation_entry_get.
• */

+struct reservation_entry {

• struct list_head head;
• unsigned long obj_shared;

+};

+static inline void +reservation_object_init(struct reservation_object *obj) +{

• obj->fence_shared_count = 0;
• obj->fence_excl = NULL;
• __ticket_mutex_init(&obj->lock, reservation_object_name,

• 	    &reservation_object_class);
  

+}

+static inline void +reservation_object_fini(struct reservation_object *obj) +{

• int i;
• if (obj->fence_excl)

• fence_put(obj->fence_excl);
  

• for (i = 0; i < obj->fence_shared_count; ++i)

• fence_put(obj->fence_shared[i]);
  

• mutex_destroy(&obj->lock.base);

+}

+static inline void +reservation_ticket_init(struct reservation_ticket *t) +{ +#ifdef CONFIG_DEBUG_LOCK_ALLOC

• /*

• * Make sure we are not reinitializing a held ticket:
  

• */
  

• debug_check_no_locks_freed((void *)t, sizeof(*t));
• lockdep_init_map(&t->dep_map, reservation_ticket_name,

• 	 &reservation_ticket_class, 0);
  

+#endif

• mutex_acquire(&t->dep_map, 0, 0, _THIS_IP_);

If CONFIG_DEBUG_LOCK_ALLOC is not defined, t->dep_map is not there.

And mutex_acquire will not expand either, so it's harmless. :-)

...

• do {

• t->seqno = atomic_long_inc_return(&reservation_counter);
  

• } while (unlikely(!t->seqno));

+}

-- Francesco