[RFC] Synchronizing access to buffers shared with dma-buf between drivers/devices

[RFC] Synchronizing access to buffers shared with dma-buf between drivers/devices

[Tom Cooksey]

Hi All,

I realise it's been a while since this was last discussed, however I'd like
to bring up kernel-side synchronization again. By kernel-side
synchronization, I mean allowing multiple drivers/devices wanting to access
the same buffer to do so without bouncing up to userspace to resolve
dependencies such as "the display controller can't start scanning out a
buffer until the GPU has finished rendering into it". As such, this is
really just an optimization which reduces latency between E.g. The GPU
finishing a rendering job and that buffer being scanned out. I appreciate
this particular example is already solved on desktop graphics cards as the
display controller and 3D core are both controlled by the same driver, so no
"generic" mechanism is needed. However on ARM SoCs, the 3D core (like an ARM
Mali) and display controller tend to be driven by separate drivers, so some
mechanism is needed to allow both drivers to synchronize their access to
buffers.

There are multiple ways synchronization can be achieved, fences/sync objects
is one common approach, however we're presenting a different approach.
Personally, I quite like fence sync objects, however we believe it requires
a lot of userspace interfaces to be changed to pass around sync object
handles. Our hope is that the kds approach will require less effort to make
use of as no existing userspace interfaces need to be changed. E.g. To use
explicit fences, the struct drm_mode_crtc_page_flip would need a new members
to pass in the handle(s) of sync object(s) which the flip depends on (I.e.
don't flip until these fences fire). The additional benefit of our approach
is that it prevents userspace specifying dependency loops which can cause a
deadlock (see kds.txt for an explanation of what I mean here).

I have waited until now to bring this up again because I am now able to
share the code I was trying (and failing I think) to explain previously. The
code has now been released under the GPLv2 from ARM Mali's developer portal,
however I've attempted to turn that into a patch to allow it to be discussed
on this list. Please find the patch inline below.

While KDS defines a very generic mechanism, I am proposing that this code or
at least the concepts be merged with the existing dma_buf code, so a the
struct kds_resource members get moved to struct dma_buf, kds_* functions get
renamed to dma_buf_* functions, etc. So I guess what I'm saying is please
don't review the actual code just yet, only the concepts the code describes,
where kds_resource == dma_duf.


Cheers,

Tom



Author: Tom Cooksey <tom.cooksey@arm.com>
Date:   Fri May 25 10:45:27 2012 +0100

    Add new system to allow synchronizing access to resources
    
    See Documentation/kds.txt for details, however the general
    idea is that this kds framework synchronizes multiple drivers
    ("clients") wanting to access the same resources, where a
    resource is typically a 2D image buffer being shared around
    using dma-buf.
    
    Note: This patch is created by extracting the sources from the
    tarball on <http://www.malideveloper.com/open-source-mali-gpus-lin
    ux-kernel-device-drivers---dev-releases.php> and putting them in
    roughly the right places.

diff --git a/Documentation/kds.txt b/Documentation/kds.txt
new file mode 100644
index 0000000..a96db21
--- /dev/null
+++ b/Documentation/kds.txt
@@ -0,0 +1,113 @@
+#
+# (C) COPYRIGHT 2012 ARM Limited. All rights reserved.
+#
+# This program is free software and is provided to you under the terms of
the GNU General Public License version 2
+# as published by the Free Software Foundation, and any use by you of this
program is subject to the terms of such GNU licence.
+#
+# A copy of the licence is included with the program, and can also be
obtained from Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
02110-1301, USA. 
+#
+#
+
+
+==============================
+kds - Kernel Dependency System
+==============================
+
+Introduction
+------------
+kds provides a mechanism for clients to atomically lock down multiple
abstract resources.
+This can be done either synchronously or asynchronously.
+Abstract resources is used to allow a set of clients to use kds to control
access to any
+resource, an example is structured memory buffers.
+
+kds supports that buffer is locked for exclusive access and sharing of
buffers.
+
+kds can be built as either a integrated feature of the kernel or as a
module.
+It supports being compiled as a module both in-tree and out-of-tree.
+
+
+Concepts
+--------
+A core concept in kds is abstract resources.
+A kds resource is just an abstraction for some client object, kds doesn't
care what it is.
+Typically EGL will consider UMP buffers as being a resource, thus each UMP
buffer has 
+a kds resource for synchronization to the buffer.
+
+kds allows a client to create and destroy the abstract resource objects.
+A new resource object is made available asap (it is just a simple malloc
with some initializations),
+while destroy it requires some external synchronization.
+
+The other core concept in kds is consumer of resources.
+kds is requested to allow a client to consume a set of resources and the
client will be notified when it can consume the resources.
+
+Exclusive access allows only one client to consume a resource.
+Shared access permits multiple consumers to acceess a resource
concurrently.
+
+
+APIs
+----
+kds provides simple resource allocate and destroy functions.
+Clients use this to instantiate and control the lifetime of the resources
kds manages.
+
+kds provides two ways to wait for resources:
+- Asynchronous wait: the client specifies a function pointer to be called
when wait is over
+- Synchronous wait: Function blocks until access is gained.
+
+The synchronous API has a timeout for the wait.
+The call can early out if a signal is delivered.
+
+After a client is done consuming the resource kds must be notified to
release the resources and let some other client take ownership.
+This is done via resource set release call.
+
+A Windows comparison:
+kds implements WaitForMultipleObjectsEx(..., bWaitAll = TRUE, ...) but also
has an asynchronous version in addition.
+kds resources can be seen as being the same as NT object manager resources.
+
+Internals
+---------
+kds guarantees atomicity when a set of resources is operated on.
+This is implemented via a global resource lock which is taken by kds when
it updates resource objects.
+
+Internally a resource in kds is a linked list head with some flags.
+
+When a consumer requests access to a set of resources it is queued on each
of the resources.
+The link from the consumer to the resources can be triggered. Once all
links are triggered 
+the registered callback is called or the blocking function returns.
+A link is considered triggered if it is the first on the list of consumers
of a resource, 
+or if all the links ahead of it is marked as shared and itself is of the
type shared.
+
+When the client is done consuming the consumer object is removed from the
linked lists of 
+the resources and a potential new consumer becomes the head of the
resources.
+As we add and remove consumers atomically across all resources we can
guarantee that 
+we never introduces a A->B + B->A type of loops/deadlocks.
+
+
+kbase/base implementation
+-------------------------
+A HW job needs access to a set of shared resources.
+EGL tracks this and encodes the set along with the atom in the ringbuffer.
+EGL allocates a (k)base dep object to represent the dependency to the set
of resources and encodes that along with the list of resources.
+This dep object is use to create a dependency from a job chain(atom) to the
resources it needs to run.
+When kbase decodes the atom in the ringbuffer it finds the set of resources
and calls kds to request all the needed resources.
+As EGL needs to know when the kds request is delivered a new base event
object is needed: atom enqueued. This event is only delivered for atoms
which uses kds.
+The callback kbase registers trigger the dependency object described which
would trigger the existing JD system to release the job chain.
+When the atom is done kds resource set release is call to release the
resources.
+
+EGL will typically use exclusive access to the render target, while all
buffers used as input can be marked as shared.
+
+
+Buffer publish/vsync
+--------------------
+EGL will use a separate ioctl or DRM flip to request the flip.
+If the LCD driver is integrated with kds EGL can do these operations early.
+The LCD driver must then implement the ioctl or DRM flip to be asynchronous
with kds async call.
+The LCD driver binds a kds resource to each virtual buffer (2 buffers in
case of double-buffering).
+EGL will make a dependency to the target kds resource in the kbase atom.
+After EGL receives a atom enqueued event it can ask the LCD driver to pan
to the target kds resource.
+When the atom is completed it'll release the resource and the LCD driver
will get its callback.
+In the callback it'll load the target buffer into the DMA unit of the LCD
hardware.
+The LCD driver will be the consumer of both buffers for a short period.
+The LCD driver will call kds resource set release on the previous on-screen
buffer when the next vsync/dma read end is handled.
+
+
diff --git a/drivers/misc/kds.c b/drivers/misc/kds.c
new file mode 100644
index 0000000..8d7d55e
--- /dev/null
+++ b/drivers/misc/kds.c
@@ -0,0 +1,461 @@
+/*
+ *
+ * (C) COPYRIGHT 2012 ARM Limited. All rights reserved.
+ *
+ * This program is free software and is provided to you under the terms of
the GNU General Public License version 2
+ * as published by the Free Software Foundation, and any use by you of this
program is subject to the terms of such GNU licence.
+ *
+ * A copy of the licence is included with the program, and can also be
obtained from Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
02110-1301, USA.
+ *
+ */
+
+
+
+#include <linux/slab.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/wait.h>
+#include <linux/sched.h>
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/workqueue.h>
+#include <linux/kds.h>
+
+
+#define KDS_LINK_TRIGGERED (1u << 0)
+#define KDS_LINK_EXCLUSIVE (1u << 1)
+
+#define KDS_IGNORED NULL
+#define KDS_INVALID (void*)-2
+#define KDS_RESOURCE (void*)-1
+
+struct kds_resource_set
+{
+	unsigned long         num_resources;
+	unsigned long         pending;
+	unsigned long         locked_resources;
+	struct kds_callback * cb;
+	void *                callback_parameter;
+	void *                callback_extra_parameter;
+	struct list_head      callback_link;
+	struct work_struct    callback_work;
+	struct kds_link       resources[0];
+};
+
+static DEFINE_MUTEX(kds_lock);
+
+int kds_callback_init(struct kds_callback * cb, int direct, kds_callback_fn
user_cb)
+{
+	int ret = 0;
+
+	cb->direct = direct;
+	cb->user_cb = user_cb;
+
+	if (!direct)
+	{
+		cb->wq = alloc_workqueue("kds", WQ_UNBOUND | WQ_HIGHPRI,
WQ_UNBOUND_MAX_ACTIVE);
+		if (!cb->wq)
+			ret = -ENOMEM;
+	}
+	else
+	{
+		cb->wq = NULL;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(kds_callback_init);
+
+void kds_callback_term(struct kds_callback * cb)
+{
+	if (!cb->direct)
+	{
+		BUG_ON(!cb->wq);
+		destroy_workqueue(cb->wq);
+	}
+	else
+	{
+		BUG_ON(cb->wq);
+	}
+}
+
+EXPORT_SYMBOL(kds_callback_term);
+
+static void kds_do_user_callback(struct kds_resource_set * rset)
+{
+	rset->cb->user_cb(rset->callback_parameter,
rset->callback_extra_parameter);
+}
+
+static void kds_queued_callback(struct work_struct * work)
+{
+	struct kds_resource_set * rset;
+	rset = container_of( work, struct kds_resource_set, callback_work);
+	
+	kds_do_user_callback(rset);
+}
+
+static void kds_callback_perform(struct kds_resource_set * rset)
+{
+	if (rset->cb->direct)
+		kds_do_user_callback(rset);
+	else
+	{
+		int result;
+		result = queue_work(rset->cb->wq, &rset->callback_work);
+		/* if we got a 0 return it means we've triggered the same
rset twice! */
+		BUG_ON(!result);
+	}
+}
+
+void kds_resource_init(struct kds_resource * res)
+{
+	BUG_ON(!res);
+	INIT_LIST_HEAD(&res->waiters.link);
+	res->waiters.parent = KDS_RESOURCE;
+}
+EXPORT_SYMBOL(kds_resource_init);
+
+void kds_resource_term(struct kds_resource * res)
+{
+	BUG_ON(!res);
+	BUG_ON(!list_empty(&res->waiters.link));
+	res->waiters.parent = KDS_INVALID;
+}
+EXPORT_SYMBOL(kds_resource_term);
+
+int kds_async_waitall(
+               struct kds_resource_set ** pprset,
+               unsigned long              flags,
+               struct kds_callback *      cb,
+               void *                     callback_parameter,
+               void *                     callback_extra_parameter,
+               int                        number_resources,
+               unsigned long *            exclusive_access_bitmap,
+               struct kds_resource **     resource_list)
+{
+	struct kds_resource_set * rset = NULL;
+	int i;
+	int triggered;
+	int err = -EFAULT;
+
+	BUG_ON(!pprset);
+	BUG_ON(!resource_list);
+	BUG_ON(!cb);
+
+	mutex_lock(&kds_lock);
+
+	if ((flags & KDS_FLAG_LOCKED_ACTION) == KDS_FLAG_LOCKED_FAIL)
+	{
+		for (i = 0; i < number_resources; i++)
+		{
+			if (resource_list[i]->lock_count)
+			{
+				err = -EBUSY;
+				goto errout;
+			}
+		}
+	}
+
+	rset = kmalloc(sizeof(*rset) + number_resources * sizeof(struct
kds_link), GFP_KERNEL);
+	if (!rset)
+	{
+		err = -ENOMEM;
+		goto errout;
+	}
+
+	rset->num_resources = number_resources;
+	rset->pending = number_resources;
+	rset->locked_resources = 0;
+	rset->cb = cb;
+	rset->callback_parameter = callback_parameter;
+	rset->callback_extra_parameter = callback_extra_parameter;
+	INIT_LIST_HEAD(&rset->callback_link);
+	INIT_WORK(&rset->callback_work, kds_queued_callback);
+
+	for (i = 0; i < number_resources; i++)
+	{
+		unsigned long link_state = 0;
+
+		INIT_LIST_HEAD(&rset->resources[i].link);
+		rset->resources[i].parent = rset;
+
+		if (test_bit(i, exclusive_access_bitmap))
+		{
+			link_state |= KDS_LINK_EXCLUSIVE;
+		}
+
+		/* no-one else waiting? */
+		if (list_empty(&resource_list[i]->waiters.link))
+		{
+			link_state |= KDS_LINK_TRIGGERED;
+			rset->pending--;
+		}
+		/* Adding a non-exclusive and the current tail is a
triggered non-exclusive? */
+		else if (((link_state & KDS_LINK_EXCLUSIVE) == 0) &&
+		        (((list_entry(resource_list[i]->waiters.link.prev,
struct kds_link, link)->state & (KDS_LINK_EXCLUSIVE | KDS_LINK_TRIGGERED))
== KDS_LINK_TRIGGERED)))
+		{
+			link_state |= KDS_LINK_TRIGGERED;
+			rset->pending--;
+		}
+		/* locked & ignore locked? */
+		else if ((resource_list[i]->lock_count) && ((flags &
KDS_FLAG_LOCKED_ACTION) == KDS_FLAG_LOCKED_IGNORE) )
+		{
+			link_state |= KDS_LINK_TRIGGERED;
+			rset->pending--;
+			rset->resources[i].parent = KDS_IGNORED; /* to
disable decrementing the pending count when we get the ignored resource */
+		}
+		rset->resources[i].state = link_state;
+		list_add_tail(&rset->resources[i].link,
&resource_list[i]->waiters.link);
+	}
+
+	triggered = (rset->pending == 0);
+
+	mutex_unlock(&kds_lock);
+
+	/* set the pointer before the callback is called so it sees it */
+	*pprset = rset;
+
+	if (triggered)
+	{
+		/* all resources obtained, trigger callback */
+		kds_callback_perform(rset);
+	}
+
+	return 0;
+
+errout:
+	mutex_unlock(&kds_lock);
+	return err;
+}
+EXPORT_SYMBOL(kds_async_waitall);
+
+static void wake_up_sync_call(void * callback_parameter, void *
callback_extra_parameter)
+{
+	wait_queue_head_t * wait = (wait_queue_head_t*)callback_parameter;
+	wake_up(wait);
+}
+
+static struct kds_callback sync_cb =
+{
+	wake_up_sync_call,
+	1,
+	NULL,
+};
+
+struct kds_resource_set * kds_waitall(
+               int                    number_resources,
+               unsigned long *        exclusive_access_bitmap,
+               struct kds_resource ** resource_list,
+               unsigned long          jiffies_timeout)
+{
+	struct kds_resource_set * rset;
+	int i;
+	int triggered = 0;
+	DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wake);
+
+	rset = kmalloc(sizeof(*rset) + number_resources * sizeof(struct
kds_link), GFP_KERNEL);
+	if (!rset)
+		return rset;
+
+	rset->num_resources = number_resources;
+	rset->pending = number_resources;
+	rset->locked_resources = 1;
+	INIT_LIST_HEAD(&rset->callback_link);
+	INIT_WORK(&rset->callback_work, kds_queued_callback);
+
+	mutex_lock(&kds_lock);
+
+	for (i = 0; i < number_resources; i++)
+	{
+		unsigned long link_state = 0;
+
+		if (likely(resource_list[i]->lock_count < ULONG_MAX))
+			resource_list[i]->lock_count++;
+		else
+			break;
+
+		if (test_bit(i, exclusive_access_bitmap))
+		{
+			link_state |= KDS_LINK_EXCLUSIVE;
+		}
+		
+		if (list_empty(&resource_list[i]->waiters.link))
+		{
+			link_state |= KDS_LINK_TRIGGERED;
+			rset->pending--;
+		}
+		/* Adding a non-exclusive and the current tail is a
triggered non-exclusive? */
+		else if (((link_state & KDS_LINK_EXCLUSIVE) == 0) &&
+		        (((list_entry(resource_list[i]->waiters.link.prev,
struct kds_link, link)->state & (KDS_LINK_EXCLUSIVE | KDS_LINK_TRIGGERED))
== KDS_LINK_TRIGGERED)))
+		{
+			link_state |= KDS_LINK_TRIGGERED;
+			rset->pending--;
+		}
+
+		INIT_LIST_HEAD(&rset->resources[i].link);
+		rset->resources[i].parent = rset;
+		rset->resources[i].state = link_state;
+		list_add_tail(&rset->resources[i].link,
&resource_list[i]->waiters.link);
+	}
+
+	if (i < number_resources)
+	{
+		/* an overflow was detected, roll back */
+		while (i--)
+		{
+			list_del(&rset->resources[i].link);
+			resource_list[i]->lock_count--;
+		}
+		mutex_unlock(&kds_lock);
+		kfree(rset);
+		return ERR_PTR(-EFAULT);
+	}
+
+	if (rset->pending == 0)
+		triggered = 1;
+	else
+	{
+		rset->cb = &sync_cb;
+		rset->callback_parameter = &wake;
+		rset->callback_extra_parameter = NULL;
+	}
+
+	mutex_unlock(&kds_lock);
+
+	if (!triggered)
+	{
+		long wait_res;
+		if ( KDS_WAIT_BLOCKING == jiffies_timeout )
+		{
+			wait_res = wait_event_interruptible(wake,
rset->pending == 0);
+		}
+		else
+		{
+			wait_res = wait_event_interruptible_timeout(wake,
rset->pending == 0, jiffies_timeout);
+		}
+		if ((wait_res == -ERESTARTSYS) || (wait_res == 0))
+		{
+			/* use \a kds_resource_set_release to roll back */
+			kds_resource_set_release(&rset);
+			return ERR_PTR(wait_res);
+		}
+	}
+	return rset;
+}
+EXPORT_SYMBOL(kds_waitall);
+
+void kds_resource_set_release(struct kds_resource_set ** pprset)
+{
+	struct list_head triggered = LIST_HEAD_INIT(triggered);
+	struct kds_resource_set * rset;
+	struct kds_resource_set * it;
+	int i;
+
+	BUG_ON(!pprset);
+	
+	mutex_lock(&kds_lock);
+
+	rset = *pprset;
+	if (!rset)
+	{
+		/* caught a race between a cancelation
+		 * and a completion, nothing to do */
+		mutex_unlock(&kds_lock);
+		return;
+	}
+
+	/* clear user pointer so we'll be the only
+	 * thread handling the release */
+	*pprset = NULL;
+
+	for (i = 0; i < rset->num_resources; i++)
+	{
+		struct kds_resource * resource;
+		struct kds_link * it = NULL;
+
+		/* fetch the previous entry on the linked list */
+		it = list_entry(rset->resources[i].link.prev, struct
kds_link, link);
+		/* unlink ourself */
+		list_del(&rset->resources[i].link);
+
+		/* any waiters? */
+		if (list_empty(&it->link))
+			continue;
+
+		/* were we the head of the list? (head if prev is a
resource) */
+		if (it->parent != KDS_RESOURCE)
+			continue;
+
+		/* we were the head, find the kds_resource */
+		resource = container_of(it, struct kds_resource, waiters);
+
+		if (rset->locked_resources)
+		{
+			resource->lock_count--;
+		}
+
+		/* we know there is someone waiting from the any-waiters
test above */
+
+		/* find the head of the waiting list */
+		it = list_first_entry(&resource->waiters.link, struct
kds_link, link);
+
+		/* new exclusive owner? */
+		if (it->state & KDS_LINK_EXCLUSIVE)
+		{
+			/* link now triggered */
+			it->state |= KDS_LINK_TRIGGERED;
+			/* a parent to update? */
+			if (it->parent != KDS_IGNORED)
+			{
+				if (0 == --it->parent->pending)
+				{
+					/* new owner now triggered, track
for callback later */
+					list_add(&it->parent->callback_link,
&triggered);
+				}
+			}
+		}
+		/* exclusive releasing ? */
+		else if (rset->resources[i].state & KDS_LINK_EXCLUSIVE)
+		{
+			/* trigger non-exclusive until end-of-list or first
exclusive */
+			list_for_each_entry(it, &resource->waiters.link,
link)
+			{
+				/* exclusive found, stop triggering */
+				if (it->state & KDS_LINK_EXCLUSIVE)
+					break;
+
+				it->state |= KDS_LINK_TRIGGERED;
+				/* a parent to update? */
+				if (it->parent != KDS_IGNORED)
+				{
+					if (0 == --it->parent->pending)
+					{
+						/* new owner now triggered,
track for callback later */
+
list_add(&it->parent->callback_link, &triggered);
+					}
+				}
+			}
+		}
+
+	}
+
+	mutex_unlock(&kds_lock);
+
+	while (!list_empty(&triggered))
+	{
+		it = list_first_entry(&triggered, struct kds_resource_set,
callback_link);
+		list_del(&it->callback_link);
+		kds_callback_perform(it);
+	}
+
+	cancel_work_sync(&rset->callback_work);
+
+	/* free the resource set */
+	kfree(rset);
+}
+EXPORT_SYMBOL(kds_resource_set_release);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("ARM Ltd.");
+MODULE_VERSION("1.0");
diff --git a/include/linux/kds.h b/include/linux/kds.h
new file mode 100644
index 0000000..65e5706
--- /dev/null
+++ b/include/linux/kds.h
@@ -0,0 +1,154 @@
+/*
+ *
+ * (C) COPYRIGHT 2012 ARM Limited. All rights reserved.
+ *
+ * This program is free software and is provided to you under the terms of
the GNU General Public License version 2
+ * as published by the Free Software Foundation, and any use by you of this
program is subject to the terms of such GNU licence.
+ * 
+ * A copy of the licence is included with the program, and can also be
obtained from Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
02110-1301, USA.
+ * 
+ */
+
+
+
+#ifndef _KDS_H_
+#define _KDS_H_
+
+#include <linux/list.h>
+#include <linux/workqueue.h>
+
+#define KDS_WAIT_BLOCKING (ULONG_MAX)
+
+/* what to do when waitall must wait for a synchronous locked resource: */
+#define KDS_FLAG_LOCKED_FAIL    (0u <<  0) /* fail waitall */
+#define KDS_FLAG_LOCKED_IGNORE  (1u <<  0) /* don't wait, but block other
that waits */
+#define KDS_FLAG_LOCKED_WAIT    (2u <<  0) /* wait (normal */
+#define KDS_FLAG_LOCKED_ACTION  (3u <<  0) /* mask to extract the action to
do on locked resources */
+
+struct kds_resource_set;
+
+typedef void (*kds_callback_fn) (void * callback_parameter, void *
callback_extra_parameter);
+
+struct kds_callback
+{
+	kds_callback_fn  user_cb; /* real cb */
+	int direct;               /* do direct or queued call? */
+	struct workqueue_struct * wq;
+};
+
+struct kds_link
+{
+	struct kds_resource_set * parent;
+	struct list_head          link;
+	unsigned long             state;
+};
+
+struct kds_resource
+{
+	struct kds_link waiters;
+	unsigned long lock_count;
+};
+
+/* callback API */
+
+/* Initialize a callback object.
+ *
+ * Typically created per context or per hw resource.
+ *
+ * Callbacks can be performed directly if no nested locking can
+ * happen in the client.
+ *
+ * Nested locking can occur when a lock is held during the
kds_async_waitall or
+ * kds_resource_set_release call. If the callback needs to take the same
lock
+ * nested locking will happen.
+ *
+ * If nested locking could happen non-direct callbacks can be requested.
+ * Callbacks will then be called asynchronous to the triggering call.
+ */
+int kds_callback_init(struct kds_callback * cb, int direct, kds_callback_fn
user_cb);
+
+/* Terminate the use of a callback object.
+ *
+ * If the callback object was set up as non-direct
+ * any pending callbacks will be flushed first.
+ * Note that to avoid a deadlock the lock callbacks needs
+ * can't be held when a callback object is terminated.
+ */
+void kds_callback_term(struct kds_callback * cb);
+
+
+/* resource object API */
+
+/* initialize a resource handle for a shared resource */
+void kds_resource_init(struct kds_resource * resource);
+
+/*
+ * Will assert if the resource is being used or waited on.
+ * The caller should NOT try to terminate a resource that could still have
clients.
+ * After the function returns the resource is no longer known by kds.
+ */
+void kds_resource_term(struct kds_resource * resource);
+
+/* Asynchronous wait for a set of resources.
+ * Callback will be called when all resources are available.
+ * If all the resources was available the callback will be called before
kds_async_waitall returns.
+ * So one must not hold any locks the callback code-flow can take when
calling kds_async_waitall.
+ * Caller considered to own/use the resources until \a kds_rset_release is
called.
+ * flags is one or more of the KDS_FLAG_* set.
+ * exclusive_access_bitmap is a bitmap where a high bit means exclusive
access while a low bit means shared access.
+ * Use the Linux __set_bit API, where the index of the buffer to control is
used as the bit index.
+ *
+ * Standard Linux error return value.
+ */
+int kds_async_waitall(
+               struct kds_resource_set ** pprset,
+               unsigned long              flags,
+               struct kds_callback *      cb,
+               void *                     callback_parameter,
+               void *                     callback_extra_parameter,
+               int                        number_resources,
+               unsigned long *            exclusive_access_bitmap,
+               struct kds_resource **     resource_list);
+
+/* Synchronous wait for a set of resources.
+ * Function will return when one of these have happened:
+ * - all resources have been obtained
+ * - timeout lapsed while waiting
+ * - a signal was received while waiting
+ *
+ * Caller considered to own/use the resources when the function returns.
+ * Caller must release the resources using \a kds_rset_release.
+ *
+ * Calling this function while holding already locked resources or other
locking primitives is dangerous.
+ * One must if this is needed decide on a lock order of the resources
and/or the other locking primitives
+ * and always take the resources/locking primitives in the specific order.
+ *
+ * Use the ERR_PTR framework to decode the return value.
+ * NULL = time out
+ * If IS_ERR then PTR_ERR gives:
+ *  ERESTARTSYS = signal received, retry call after signal
+ *  all other values = internal error, lock failed
+ * Other values  = successful wait, now the owner, must call
kds_resource_set_release
+ */
+struct kds_resource_set * kds_waitall(
+               int                    number_resources,
+               unsigned long *        exclusive_access_bitmap,
+               struct kds_resource ** resource_list,
+               unsigned long          jifies_timeout);
+
+/* Release resources after use.
+ * Caller must handle that other async callbacks will trigger,
+ * so must avoid holding any locks a callback will take.
+ *
+ * The function takes a pointer to your poiner to handle a race
+ * between a cancelation and a completion.
+ *
+ * If the caller can't guarantee that a race can't occur then
+ * the passed in pointer must be the same in both call paths
+ * to allow kds to manage the potential race.
+ */
+void kds_resource_set_release(struct kds_resource_set ** pprset);
+
+#endif /* _KDS_H_ */
+

Re: [RFC] Synchronizing access to buffers shared with dma-buf between drivers/devices

[Pauli]

On 25/05/12 14:08, Tom Cooksey wrote:
> Hi All,
>
> I realise it's been a while since this was last discussed, however I'd like
> to bring up kernel-side synchronization again. By kernel-side
> synchronization, I mean allowing multiple drivers/devices wanting to access
> the same buffer to do so without bouncing up to userspace to resolve
> dependencies such as "the display controller can't start scanning out a
> buffer until the GPU has finished rendering into it". As such, this is
> really just an optimization which reduces latency between E.g. The GPU
> finishing a rendering job and that buffer being scanned out. I appreciate
> this particular example is already solved on desktop graphics cards as the
> display controller and 3D core are both controlled by the same driver, so no
> "generic" mechanism is needed. However on ARM SoCs, the 3D core (like an ARM
> Mali) and display controller tend to be driven by separate drivers, so some
> mechanism is needed to allow both drivers to synchronize their access to
> buffers.
>
> There are multiple ways synchronization can be achieved, fences/sync objects
> is one common approach, however we're presenting a different approach.
> Personally, I quite like fence sync objects, however we believe it requires
> a lot of userspace interfaces to be changed to pass around sync object
> handles. Our hope is that the kds approach will require less effort to make
> use of as no existing userspace interfaces need to be changed. E.g. To use
> explicit fences, the struct drm_mode_crtc_page_flip would need a new members
> to pass in the handle(s) of sync object(s) which the flip depends on (I.e.
> don't flip until these fences fire). The additional benefit of our approach
> is that it prevents userspace specifying dependency loops which can cause a
> deadlock (see kds.txt for an explanation of what I mean here).

It is easy to cause cyclic dependencies with implicit fences unless you 
are very sure that client can only cause linear implicit dependencies. 
But clients already have synchronization dependencies with userspace. 
That makes implicit synchronization possibly cause unexpected deadlocks.

Explicit synchronization is easier to debug because developer using 
explicit synchronization can track the dependencies in userspace. But of 
course that makes userspace API harder to use than API using implicitly 
synchronization.

But implicit synchronization can avoid client deadlock issues. Providing 
if client may never block "fence" from triggering in finite time when it 
is granted access. The page flip can be synchronized in that manner if 
client can't block HW from processing queued rendering.

You were talking about adding new parameter to page flip ioctl. I fail 
to see need for it because page flip already has fb object as parameter 
that should map to the implicit synchronization fence through dma_buf.

> I have waited until now to bring this up again because I am now able to
> share the code I was trying (and failing I think) to explain previously. The
> code has now been released under the GPLv2 from ARM Mali's developer portal,
> however I've attempted to turn that into a patch to allow it to be discussed
> on this list. Please find the patch inline below.
>
> While KDS defines a very generic mechanism, I am proposing that this code or
> at least the concepts be merged with the existing dma_buf code, so a the
> struct kds_resource members get moved to struct dma_buf, kds_* functions get
> renamed to dma_buf_* functions, etc. So I guess what I'm saying is please
> don't review the actual code just yet, only the concepts the code describes,
> where kds_resource == dma_duf.

But the documented functionality sounds very much deadlock prone. If 
userspace gets exclusive access and needs to wait for implicit access 
synchronization.

app A has access to buffer X
app B requests exclusive access to buffer X and blocks waiting for access
app A makes synchronous IPC call to app B

I didn't read the actual code at all to figure out if that is possible 
scenario. But it sounds like possible scenario based on documentation 
talking EGL depending on exclusive access.

>
> Cheers,
>
> Tom
>

Pauli

RE: [RFC] Synchronizing access to buffers shared with dma-buf between drivers/devices

[Tom Cooksey]

> > There are multiple ways synchronization can be achieved, 
> > fences/sync objects is one common approach, however we're
> > presenting a different approach. Personally, I quite like 
> > fence sync objects, however we believe it requires a lot of 
> > userspace interfaces to be changed to pass around sync object
> > handles. Our hope is that the kds approach will require less 
> > effort to make use of as no existing userspace interfaces need
> > to be changed. E.g. To use explicit fences, the struct
> > drm_mode_crtc_page_flip would need a new members to pass in the
> > handle(s) of sync object(s) which the flip depends on (I.e.
> > don't flip until these fences fire). The additional benefit of
> > our approach is that it prevents userspace specifying dependency
> > loops which can cause a deadlock (see kds.txt for an explanation
> > of what I mean here).
> 
> It is easy to cause cyclic dependencies with implicit fences unless you
> are very sure that client can only cause linear implicit dependencies.

I'm not sure I know what you mean by linear implicit dependencies?


> But clients already have synchronization dependencies with userspace.
> That makes implicit synchronization possibly cause unexpected
> deadlocks.

Again, not sure what you mean here? Do you mean that userspace can
Submit a piece of work to a driver which depends on something
else happening in userpsace?


> Explicit synchronization is easier to debug because developer using
> explicit synchronization can track the dependencies in userspace. But
> of course that makes userspace API harder to use than API using
> implicitly synchronization.
> 
> But implicit synchronization can avoid client deadlock issues.
> Providing if client may never block "fence" from triggering in finite
> time when it is granted access. The page flip can be synchronized in
> that manner if client can't block HW from processing queued rendering.

Yes, I guess this is the critical point - this approach assumes that
when a client starts using a resource, it will only do so for a finite
amount of time. If userspace wanted to participate in the scheme, we
would probably need some kind of timeout, otherwise userspace could
prevent other devices from accessing a resource.


> You were talking about adding new parameter to page flip ioctl. I fail
> to see need for it because page flip already has fb object as parameter
> that should map to the implicit synchronization fence through dma_buf.

This is the point I was trying to make. With explicit fence objects
you do have to add a new parameter, whereas with this kds implicit
approach you do not - the buffer itself becomes the sync object.



> > While KDS defines a very generic mechanism, I am proposing that 
> > this code or at least the concepts be merged with the existing 
> 
> > dma_buf code, so a the struct kds_resource members get moved to
> > struct dma_buf, kds_* functions get renamed to dma_buf_*
> > functions, etc. So I guess what I'm saying is please don't review
> > the actual code just yet, only the concepts the code describes,
> > where kds_resource == dma_duf.
> 
> But the documented functionality sounds very much deadlock prone. If
> userspace gets exclusive access and needs to wait for implicit access
> synchronization.
> 
> app A has access to buffer X
> app B requests exclusive access to buffer X and blocks waiting for access
> app A makes synchronous IPC call to app B
> 
> I didn't read the actual code at all to figure out if that is possible
> scenario. But it sounds like possible scenario based on documentation
> talking EGL depending on exclusive access.

The intention was to use this mechanism for synchronizing between
drivers rather than between userspace processes, I think the userspace
access is somewhat an afterthought which will probably need some more
thought. In the example you give, app A making a synchronous IPC call
to app B breaks the clients must guarantee they complete in a finite
time, which in the case of userspace access could be enforced by a
timeout. Though I would have thought there's a better way to handle
this than just a timeout.


Cheers,

Tom

Re: [RFC] Synchronizing access to buffers shared with dma-buf between drivers/devices

[Rob Clark]

Some comments inline.. at this stage mostly superficial issues about
how the API works, etc..  not had a chance to dig too much into the
implementation yet (although some of my comments about the API would
change those anyways).

Anyways, thanks for getting the ball rolling on this, and I think I
can volunteer linaro to pick up and run w/ this if needed.

On Fri, May 25, 2012 at 7:08 PM, Tom Cooksey <tom.cooksey@arm.com> wrote:
> Hi All,
>
> I realise it's been a while since this was last discussed, however I'd like
> to bring up kernel-side synchronization again. By kernel-side
> synchronization, I mean allowing multiple drivers/devices wanting to access
> the same buffer to do so without bouncing up to userspace to resolve
> dependencies such as "the display controller can't start scanning out a
> buffer until the GPU has finished rendering into it". As such, this is
> really just an optimization which reduces latency between E.g. The GPU
> finishing a rendering job and that buffer being scanned out. I appreciate
> this particular example is already solved on desktop graphics cards as the
> display controller and 3D core are both controlled by the same driver, so no
> "generic" mechanism is needed. However on ARM SoCs, the 3D core (like an ARM
> Mali) and display controller tend to be driven by separate drivers, so some
> mechanism is needed to allow both drivers to synchronize their access to
> buffers.
>
> There are multiple ways synchronization can be achieved, fences/sync objects
> is one common approach, however we're presenting a different approach.
> Personally, I quite like fence sync objects, however we believe it requires
> a lot of userspace interfaces to be changed to pass around sync object
> handles. Our hope is that the kds approach will require less effort to make
> use of as no existing userspace interfaces need to be changed. E.g. To use
> explicit fences, the struct drm_mode_crtc_page_flip would need a new members
> to pass in the handle(s) of sync object(s) which the flip depends on (I.e.
> don't flip until these fences fire). The additional benefit of our approach
> is that it prevents userspace specifying dependency loops which can cause a
> deadlock (see kds.txt for an explanation of what I mean here).
>
> I have waited until now to bring this up again because I am now able to
> share the code I was trying (and failing I think) to explain previously. The
> code has now been released under the GPLv2 from ARM Mali's developer portal,
> however I've attempted to turn that into a patch to allow it to be discussed
> on this list. Please find the patch inline below.
>
> While KDS defines a very generic mechanism, I am proposing that this code or
> at least the concepts be merged with the existing dma_buf code, so a the
> struct kds_resource members get moved to struct dma_buf, kds_* functions get
> renamed to dma_buf_* functions, etc. So I guess what I'm saying is please
> don't review the actual code just yet, only the concepts the code describes,
> where kds_resource == dma_duf.
>
>
> Cheers,
>
> Tom
>
>
>
> Author: Tom Cooksey <tom.cooksey@arm.com>
> Date:   Fri May 25 10:45:27 2012 +0100
>
>    Add new system to allow synchronizing access to resources
>
>    See Documentation/kds.txt for details, however the general
>    idea is that this kds framework synchronizes multiple drivers
>    ("clients") wanting to access the same resources, where a
>    resource is typically a 2D image buffer being shared around
>    using dma-buf.
>
>    Note: This patch is created by extracting the sources from the
>    tarball on <http://www.malideveloper.com/open-source-mali-gpus-lin
>    ux-kernel-device-drivers---dev-releases.php> and putting them in
>    roughly the right places.
>
> diff --git a/Documentation/kds.txt b/Documentation/kds.txt

fwiw, I think the documentation could be made a bit more generic, but
this and code style, etc shouldn't be too hard to fix

> new file mode 100644
> index 0000000..a96db21
> --- /dev/null
> +++ b/Documentation/kds.txt
> @@ -0,0 +1,113 @@
> +#
> +# (C) COPYRIGHT 2012 ARM Limited. All rights reserved.
> +#
> +# This program is free software and is provided to you under the terms of
> the GNU General Public License version 2
> +# as published by the Free Software Foundation, and any use by you of this
> program is subject to the terms of such GNU licence.
> +#
> +# A copy of the licence is included with the program, and can also be
> obtained from Free Software
> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> 02110-1301, USA.
> +#
> +#
> +
> +
> +==============================
> +kds - Kernel Dependency System
> +==============================
> +
> +Introduction
> +------------
> +kds provides a mechanism for clients to atomically lock down multiple
> abstract resources.
> +This can be done either synchronously or asynchronously.
> +Abstract resources is used to allow a set of clients to use kds to control
> access to any
> +resource, an example is structured memory buffers.
> +
> +kds supports that buffer is locked for exclusive access and sharing of
> buffers.
> +
> +kds can be built as either a integrated feature of the kernel or as a
> module.
> +It supports being compiled as a module both in-tree and out-of-tree.
> +
> +
> +Concepts
> +--------
> +A core concept in kds is abstract resources.
> +A kds resource is just an abstraction for some client object, kds doesn't
> care what it is.
> +Typically EGL will consider UMP buffers as being a resource, thus each UMP
> buffer has
> +a kds resource for synchronization to the buffer.
> +
> +kds allows a client to create and destroy the abstract resource objects.
> +A new resource object is made available asap (it is just a simple malloc
> with some initializations),
> +while destroy it requires some external synchronization.
> +
> +The other core concept in kds is consumer of resources.
> +kds is requested to allow a client to consume a set of resources and the
> client will be notified when it can consume the resources.
> +
> +Exclusive access allows only one client to consume a resource.
> +Shared access permits multiple consumers to acceess a resource
> concurrently.
> +
> +
> +APIs
> +----
> +kds provides simple resource allocate and destroy functions.
> +Clients use this to instantiate and control the lifetime of the resources
> kds manages.
> +
> +kds provides two ways to wait for resources:
> +- Asynchronous wait: the client specifies a function pointer to be called
> when wait is over
> +- Synchronous wait: Function blocks until access is gained.
> +
> +The synchronous API has a timeout for the wait.
> +The call can early out if a signal is delivered.
> +
> +After a client is done consuming the resource kds must be notified to
> release the resources and let some other client take ownership.
> +This is done via resource set release call.
> +
> +A Windows comparison:
> +kds implements WaitForMultipleObjectsEx(..., bWaitAll = TRUE, ...) but also
> has an asynchronous version in addition.
> +kds resources can be seen as being the same as NT object manager resources.
> +
> +Internals
> +---------
> +kds guarantees atomicity when a set of resources is operated on.
> +This is implemented via a global resource lock which is taken by kds when
> it updates resource objects.
> +
> +Internally a resource in kds is a linked list head with some flags.
> +
> +When a consumer requests access to a set of resources it is queued on each
> of the resources.
> +The link from the consumer to the resources can be triggered. Once all
> links are triggered
> +the registered callback is called or the blocking function returns.
> +A link is considered triggered if it is the first on the list of consumers
> of a resource,
> +or if all the links ahead of it is marked as shared and itself is of the
> type shared.
> +
> +When the client is done consuming the consumer object is removed from the
> linked lists of
> +the resources and a potential new consumer becomes the head of the
> resources.
> +As we add and remove consumers atomically across all resources we can
> guarantee that
> +we never introduces a A->B + B->A type of loops/deadlocks.
> +
> +
> +kbase/base implementation
> +-------------------------
> +A HW job needs access to a set of shared resources.
> +EGL tracks this and encodes the set along with the atom in the ringbuffer.
> +EGL allocates a (k)base dep object to represent the dependency to the set
> of resources and encodes that along with the list of resources.
> +This dep object is use to create a dependency from a job chain(atom) to the
> resources it needs to run.
> +When kbase decodes the atom in the ringbuffer it finds the set of resources
> and calls kds to request all the needed resources.
> +As EGL needs to know when the kds request is delivered a new base event
> object is needed: atom enqueued. This event is only delivered for atoms
> which uses kds.
> +The callback kbase registers trigger the dependency object described which
> would trigger the existing JD system to release the job chain.
> +When the atom is done kds resource set release is call to release the
> resources.
> +
> +EGL will typically use exclusive access to the render target, while all
> buffers used as input can be marked as shared.
> +
> +
> +Buffer publish/vsync
> +--------------------
> +EGL will use a separate ioctl or DRM flip to request the flip.
> +If the LCD driver is integrated with kds EGL can do these operations early.
> +The LCD driver must then implement the ioctl or DRM flip to be asynchronous
> with kds async call.
> +The LCD driver binds a kds resource to each virtual buffer (2 buffers in
> case of double-buffering).
> +EGL will make a dependency to the target kds resource in the kbase atom.
> +After EGL receives a atom enqueued event it can ask the LCD driver to pan
> to the target kds resource.
> +When the atom is completed it'll release the resource and the LCD driver
> will get its callback.
> +In the callback it'll load the target buffer into the DMA unit of the LCD
> hardware.
> +The LCD driver will be the consumer of both buffers for a short period.
> +The LCD driver will call kds resource set release on the previous on-screen
> buffer when the next vsync/dma read end is handled.
> +
> +
> diff --git a/drivers/misc/kds.c b/drivers/misc/kds.c
> new file mode 100644
> index 0000000..8d7d55e
> --- /dev/null
> +++ b/drivers/misc/kds.c
> @@ -0,0 +1,461 @@
> +/*
> + *
> + * (C) COPYRIGHT 2012 ARM Limited. All rights reserved.
> + *
> + * This program is free software and is provided to you under the terms of
> the GNU General Public License version 2
> + * as published by the Free Software Foundation, and any use by you of this
> program is subject to the terms of such GNU licence.
> + *
> + * A copy of the licence is included with the program, and can also be
> obtained from Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> 02110-1301, USA.
> + *
> + */
> +
> +
> +
> +#include <linux/slab.h>
> +#include <linux/list.h>
> +#include <linux/mutex.h>
> +#include <linux/wait.h>
> +#include <linux/sched.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/workqueue.h>
> +#include <linux/kds.h>
> +
> +
> +#define KDS_LINK_TRIGGERED (1u << 0)
> +#define KDS_LINK_EXCLUSIVE (1u << 1)
> +
> +#define KDS_IGNORED NULL
> +#define KDS_INVALID (void*)-2
> +#define KDS_RESOURCE (void*)-1
> +
> +struct kds_resource_set
> +{
> +       unsigned long         num_resources;
> +       unsigned long         pending;
> +       unsigned long         locked_resources;
> +       struct kds_callback * cb;
> +       void *                callback_parameter;
> +       void *                callback_extra_parameter;
> +       struct list_head      callback_link;
> +       struct work_struct    callback_work;
> +       struct kds_link       resources[0];
> +};
> +
> +static DEFINE_MUTEX(kds_lock);
> +
> +int kds_callback_init(struct kds_callback * cb, int direct, kds_callback_fn
> user_cb)
> +{
> +       int ret = 0;
> +
> +       cb->direct = direct;
> +       cb->user_cb = user_cb;
> +
> +       if (!direct)
> +       {
> +               cb->wq = alloc_workqueue("kds", WQ_UNBOUND | WQ_HIGHPRI,
> WQ_UNBOUND_MAX_ACTIVE);
> +               if (!cb->wq)
> +                       ret = -ENOMEM;
> +       }
> +       else
> +       {
> +               cb->wq = NULL;
> +       }
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL(kds_callback_init);
> +
> +void kds_callback_term(struct kds_callback * cb)
> +{
> +       if (!cb->direct)
> +       {
> +               BUG_ON(!cb->wq);
> +               destroy_workqueue(cb->wq);
> +       }
> +       else
> +       {
> +               BUG_ON(cb->wq);
> +       }
> +}
> +
> +EXPORT_SYMBOL(kds_callback_term);
> +
> +static void kds_do_user_callback(struct kds_resource_set * rset)
> +{
> +       rset->cb->user_cb(rset->callback_parameter,
> rset->callback_extra_parameter);
> +}
> +
> +static void kds_queued_callback(struct work_struct * work)
> +{
> +       struct kds_resource_set * rset;
> +       rset = container_of( work, struct kds_resource_set, callback_work);
> +
> +       kds_do_user_callback(rset);
> +}
> +
> +static void kds_callback_perform(struct kds_resource_set * rset)
> +{
> +       if (rset->cb->direct)
> +               kds_do_user_callback(rset);
> +       else
> +       {
> +               int result;
> +               result = queue_work(rset->cb->wq, &rset->callback_work);
> +               /* if we got a 0 return it means we've triggered the same
> rset twice! */
> +               BUG_ON(!result);
> +       }
> +}
> +
> +void kds_resource_init(struct kds_resource * res)
> +{
> +       BUG_ON(!res);
> +       INIT_LIST_HEAD(&res->waiters.link);
> +       res->waiters.parent = KDS_RESOURCE;
> +}
> +EXPORT_SYMBOL(kds_resource_init);
> +
> +void kds_resource_term(struct kds_resource * res)
> +{
> +       BUG_ON(!res);
> +       BUG_ON(!list_empty(&res->waiters.link));
> +       res->waiters.parent = KDS_INVALID;
> +}
> +EXPORT_SYMBOL(kds_resource_term);
> +
> +int kds_async_waitall(
> +               struct kds_resource_set ** pprset,
> +               unsigned long              flags,
> +               struct kds_callback *      cb,
> +               void *                     callback_parameter,
> +               void *                     callback_extra_parameter,
> +               int                        number_resources,
> +               unsigned long *            exclusive_access_bitmap,
> +               struct kds_resource **     resource_list)
> +{
> +       struct kds_resource_set * rset = NULL;
> +       int i;
> +       int triggered;
> +       int err = -EFAULT;
> +
> +       BUG_ON(!pprset);
> +       BUG_ON(!resource_list);
> +       BUG_ON(!cb);
> +
> +       mutex_lock(&kds_lock);
> +
> +       if ((flags & KDS_FLAG_LOCKED_ACTION) == KDS_FLAG_LOCKED_FAIL)
> +       {
> +               for (i = 0; i < number_resources; i++)
> +               {
> +                       if (resource_list[i]->lock_count)
> +                       {
> +                               err = -EBUSY;
> +                               goto errout;
> +                       }
> +               }
> +       }
> +
> +       rset = kmalloc(sizeof(*rset) + number_resources * sizeof(struct
> kds_link), GFP_KERNEL);
> +       if (!rset)
> +       {
> +               err = -ENOMEM;
> +               goto errout;
> +       }
> +
> +       rset->num_resources = number_resources;
> +       rset->pending = number_resources;
> +       rset->locked_resources = 0;
> +       rset->cb = cb;
> +       rset->callback_parameter = callback_parameter;
> +       rset->callback_extra_parameter = callback_extra_parameter;
> +       INIT_LIST_HEAD(&rset->callback_link);
> +       INIT_WORK(&rset->callback_work, kds_queued_callback);
> +
> +       for (i = 0; i < number_resources; i++)
> +       {
> +               unsigned long link_state = 0;
> +
> +               INIT_LIST_HEAD(&rset->resources[i].link);
> +               rset->resources[i].parent = rset;
> +
> +               if (test_bit(i, exclusive_access_bitmap))
> +               {
> +                       link_state |= KDS_LINK_EXCLUSIVE;
> +               }
> +
> +               /* no-one else waiting? */
> +               if (list_empty(&resource_list[i]->waiters.link))
> +               {
> +                       link_state |= KDS_LINK_TRIGGERED;
> +                       rset->pending--;
> +               }
> +               /* Adding a non-exclusive and the current tail is a
> triggered non-exclusive? */
> +               else if (((link_state & KDS_LINK_EXCLUSIVE) == 0) &&
> +                       (((list_entry(resource_list[i]->waiters.link.prev,
> struct kds_link, link)->state & (KDS_LINK_EXCLUSIVE | KDS_LINK_TRIGGERED))
> == KDS_LINK_TRIGGERED)))
> +               {
> +                       link_state |= KDS_LINK_TRIGGERED;
> +                       rset->pending--;
> +               }
> +               /* locked & ignore locked? */
> +               else if ((resource_list[i]->lock_count) && ((flags &
> KDS_FLAG_LOCKED_ACTION) == KDS_FLAG_LOCKED_IGNORE) )
> +               {
> +                       link_state |= KDS_LINK_TRIGGERED;
> +                       rset->pending--;
> +                       rset->resources[i].parent = KDS_IGNORED; /* to
> disable decrementing the pending count when we get the ignored resource */
> +               }
> +               rset->resources[i].state = link_state;
> +               list_add_tail(&rset->resources[i].link,
> &resource_list[i]->waiters.link);
> +       }
> +
> +       triggered = (rset->pending == 0);
> +
> +       mutex_unlock(&kds_lock);
> +
> +       /* set the pointer before the callback is called so it sees it */
> +       *pprset = rset;
> +
> +       if (triggered)
> +       {
> +               /* all resources obtained, trigger callback */
> +               kds_callback_perform(rset);
> +       }
> +
> +       return 0;
> +
> +errout:
> +       mutex_unlock(&kds_lock);
> +       return err;
> +}
> +EXPORT_SYMBOL(kds_async_waitall);
> +
> +static void wake_up_sync_call(void * callback_parameter, void *
> callback_extra_parameter)
> +{
> +       wait_queue_head_t * wait = (wait_queue_head_t*)callback_parameter;
> +       wake_up(wait);
> +}
> +
> +static struct kds_callback sync_cb =
> +{
> +       wake_up_sync_call,
> +       1,
> +       NULL,
> +};
> +
> +struct kds_resource_set * kds_waitall(
> +               int                    number_resources,
> +               unsigned long *        exclusive_access_bitmap,
> +               struct kds_resource ** resource_list,
> +               unsigned long          jiffies_timeout)
> +{
> +       struct kds_resource_set * rset;
> +       int i;
> +       int triggered = 0;
> +       DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wake);
> +
> +       rset = kmalloc(sizeof(*rset) + number_resources * sizeof(struct
> kds_link), GFP_KERNEL);
> +       if (!rset)
> +               return rset;
> +
> +       rset->num_resources = number_resources;
> +       rset->pending = number_resources;
> +       rset->locked_resources = 1;
> +       INIT_LIST_HEAD(&rset->callback_link);
> +       INIT_WORK(&rset->callback_work, kds_queued_callback);
> +
> +       mutex_lock(&kds_lock);
> +
> +       for (i = 0; i < number_resources; i++)
> +       {
> +               unsigned long link_state = 0;
> +
> +               if (likely(resource_list[i]->lock_count < ULONG_MAX))
> +                       resource_list[i]->lock_count++;
> +               else
> +                       break;
> +
> +               if (test_bit(i, exclusive_access_bitmap))
> +               {
> +                       link_state |= KDS_LINK_EXCLUSIVE;
> +               }
> +
> +               if (list_empty(&resource_list[i]->waiters.link))
> +               {
> +                       link_state |= KDS_LINK_TRIGGERED;
> +                       rset->pending--;
> +               }
> +               /* Adding a non-exclusive and the current tail is a
> triggered non-exclusive? */
> +               else if (((link_state & KDS_LINK_EXCLUSIVE) == 0) &&
> +                       (((list_entry(resource_list[i]->waiters.link.prev,
> struct kds_link, link)->state & (KDS_LINK_EXCLUSIVE | KDS_LINK_TRIGGERED))
> == KDS_LINK_TRIGGERED)))
> +               {
> +                       link_state |= KDS_LINK_TRIGGERED;
> +                       rset->pending--;
> +               }
> +
> +               INIT_LIST_HEAD(&rset->resources[i].link);
> +               rset->resources[i].parent = rset;
> +               rset->resources[i].state = link_state;
> +               list_add_tail(&rset->resources[i].link,
> &resource_list[i]->waiters.link);
> +       }
> +
> +       if (i < number_resources)
> +       {
> +               /* an overflow was detected, roll back */
> +               while (i--)
> +               {
> +                       list_del(&rset->resources[i].link);
> +                       resource_list[i]->lock_count--;
> +               }
> +               mutex_unlock(&kds_lock);
> +               kfree(rset);
> +               return ERR_PTR(-EFAULT);
> +       }
> +
> +       if (rset->pending == 0)
> +               triggered = 1;
> +       else
> +       {
> +               rset->cb = &sync_cb;
> +               rset->callback_parameter = &wake;
> +               rset->callback_extra_parameter = NULL;
> +       }
> +
> +       mutex_unlock(&kds_lock);
> +
> +       if (!triggered)
> +       {
> +               long wait_res;
> +               if ( KDS_WAIT_BLOCKING == jiffies_timeout )
> +               {
> +                       wait_res = wait_event_interruptible(wake,
> rset->pending == 0);
> +               }
> +               else
> +               {
> +                       wait_res = wait_event_interruptible_timeout(wake,
> rset->pending == 0, jiffies_timeout);
> +               }
> +               if ((wait_res == -ERESTARTSYS) || (wait_res == 0))
> +               {
> +                       /* use \a kds_resource_set_release to roll back */
> +                       kds_resource_set_release(&rset);
> +                       return ERR_PTR(wait_res);
> +               }
> +       }
> +       return rset;
> +}
> +EXPORT_SYMBOL(kds_waitall);
> +
> +void kds_resource_set_release(struct kds_resource_set ** pprset)
> +{
> +       struct list_head triggered = LIST_HEAD_INIT(triggered);
> +       struct kds_resource_set * rset;
> +       struct kds_resource_set * it;
> +       int i;
> +
> +       BUG_ON(!pprset);
> +
> +       mutex_lock(&kds_lock);
> +
> +       rset = *pprset;
> +       if (!rset)
> +       {
> +               /* caught a race between a cancelation
> +                * and a completion, nothing to do */
> +               mutex_unlock(&kds_lock);
> +               return;
> +       }
> +
> +       /* clear user pointer so we'll be the only
> +        * thread handling the release */
> +       *pprset = NULL;
> +
> +       for (i = 0; i < rset->num_resources; i++)
> +       {
> +               struct kds_resource * resource;
> +               struct kds_link * it = NULL;
> +
> +               /* fetch the previous entry on the linked list */
> +               it = list_entry(rset->resources[i].link.prev, struct
> kds_link, link);
> +               /* unlink ourself */
> +               list_del(&rset->resources[i].link);
> +
> +               /* any waiters? */
> +               if (list_empty(&it->link))
> +                       continue;
> +
> +               /* were we the head of the list? (head if prev is a
> resource) */
> +               if (it->parent != KDS_RESOURCE)
> +                       continue;
> +
> +               /* we were the head, find the kds_resource */
> +               resource = container_of(it, struct kds_resource, waiters);
> +
> +               if (rset->locked_resources)
> +               {
> +                       resource->lock_count--;
> +               }
> +
> +               /* we know there is someone waiting from the any-waiters
> test above */
> +
> +               /* find the head of the waiting list */
> +               it = list_first_entry(&resource->waiters.link, struct
> kds_link, link);
> +
> +               /* new exclusive owner? */
> +               if (it->state & KDS_LINK_EXCLUSIVE)
> +               {
> +                       /* link now triggered */
> +                       it->state |= KDS_LINK_TRIGGERED;
> +                       /* a parent to update? */
> +                       if (it->parent != KDS_IGNORED)
> +                       {
> +                               if (0 == --it->parent->pending)
> +                               {
> +                                       /* new owner now triggered, track
> for callback later */
> +                                       list_add(&it->parent->callback_link,
> &triggered);
> +                               }
> +                       }
> +               }
> +               /* exclusive releasing ? */
> +               else if (rset->resources[i].state & KDS_LINK_EXCLUSIVE)
> +               {
> +                       /* trigger non-exclusive until end-of-list or first
> exclusive */
> +                       list_for_each_entry(it, &resource->waiters.link,
> link)
> +                       {
> +                               /* exclusive found, stop triggering */
> +                               if (it->state & KDS_LINK_EXCLUSIVE)
> +                                       break;
> +
> +                               it->state |= KDS_LINK_TRIGGERED;
> +                               /* a parent to update? */
> +                               if (it->parent != KDS_IGNORED)
> +                               {
> +                                       if (0 == --it->parent->pending)
> +                                       {
> +                                               /* new owner now triggered,
> track for callback later */
> +
> list_add(&it->parent->callback_link, &triggered);
> +                                       }
> +                               }
> +                       }
> +               }
> +
> +       }
> +
> +       mutex_unlock(&kds_lock);
> +
> +       while (!list_empty(&triggered))
> +       {
> +               it = list_first_entry(&triggered, struct kds_resource_set,
> callback_link);
> +               list_del(&it->callback_link);
> +               kds_callback_perform(it);
> +       }
> +
> +       cancel_work_sync(&rset->callback_work);
> +
> +       /* free the resource set */
> +       kfree(rset);
> +}
> +EXPORT_SYMBOL(kds_resource_set_release);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("ARM Ltd.");
> +MODULE_VERSION("1.0");
> diff --git a/include/linux/kds.h b/include/linux/kds.h
> new file mode 100644
> index 0000000..65e5706
> --- /dev/null
> +++ b/include/linux/kds.h
> @@ -0,0 +1,154 @@
> +/*
> + *
> + * (C) COPYRIGHT 2012 ARM Limited. All rights reserved.
> + *
> + * This program is free software and is provided to you under the terms of
> the GNU General Public License version 2
> + * as published by the Free Software Foundation, and any use by you of this
> program is subject to the terms of such GNU licence.
> + *
> + * A copy of the licence is included with the program, and can also be
> obtained from Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> 02110-1301, USA.
> + *
> + */
> +
> +
> +
> +#ifndef _KDS_H_
> +#define _KDS_H_
> +
> +#include <linux/list.h>
> +#include <linux/workqueue.h>
> +
> +#define KDS_WAIT_BLOCKING (ULONG_MAX)
> +
> +/* what to do when waitall must wait for a synchronous locked resource: */
> +#define KDS_FLAG_LOCKED_FAIL    (0u <<  0) /* fail waitall */
> +#define KDS_FLAG_LOCKED_IGNORE  (1u <<  0) /* don't wait, but block other
> that waits */
> +#define KDS_FLAG_LOCKED_WAIT    (2u <<  0) /* wait (normal */
> +#define KDS_FLAG_LOCKED_ACTION  (3u <<  0) /* mask to extract the action to
> do on locked resources */
> +
> +struct kds_resource_set;
> +
> +typedef void (*kds_callback_fn) (void * callback_parameter, void *
> callback_extra_parameter);
> +
> +struct kds_callback
> +{
> +       kds_callback_fn  user_cb; /* real cb */
> +       int direct;               /* do direct or queued call? */
> +       struct workqueue_struct * wq;
> +};
> +
> +struct kds_link
> +{
> +       struct kds_resource_set * parent;
> +       struct list_head          link;
> +       unsigned long             state;
> +};
> +
> +struct kds_resource
> +{
> +       struct kds_link waiters;
> +       unsigned long lock_count;
> +};
> +
> +/* callback API */
> +
> +/* Initialize a callback object.
> + *
> + * Typically created per context or per hw resource.
> + *
> + * Callbacks can be performed directly if no nested locking can
> + * happen in the client.
> + *
> + * Nested locking can occur when a lock is held during the
> kds_async_waitall or
> + * kds_resource_set_release call. If the callback needs to take the same
> lock
> + * nested locking will happen.
> + *
> + * If nested locking could happen non-direct callbacks can be requested.
> + * Callbacks will then be called asynchronous to the triggering call.
> + */
> +int kds_callback_init(struct kds_callback * cb, int direct, kds_callback_fn
> user_cb);
> +
> +/* Terminate the use of a callback object.
> + *
> + * If the callback object was set up as non-direct
> + * any pending callbacks will be flushed first.
> + * Note that to avoid a deadlock the lock callbacks needs
> + * can't be held when a callback object is terminated.
> + */
> +void kds_callback_term(struct kds_callback * cb);

hmm, not hugely a fan of this.. having callbacks that might need to
aquire locks be potentially called synchronously in special cases.

It seems like it can get simpler if pending callbacks could hold a
reference to the underlying object until the callback completes.
Although not quite sure offhand how that can work without coupling kds
to dmabuf or GEM..

> +
> +/* resource object API */
> +
> +/* initialize a resource handle for a shared resource */
> +void kds_resource_init(struct kds_resource * resource);
> +
> +/*
> + * Will assert if the resource is being used or waited on.
> + * The caller should NOT try to terminate a resource that could still have
> clients.
> + * After the function returns the resource is no longer known by kds.
> + */
> +void kds_resource_term(struct kds_resource * resource);
> +
> +/* Asynchronous wait for a set of resources.
> + * Callback will be called when all resources are available.
> + * If all the resources was available the callback will be called before
> kds_async_waitall returns.
> + * So one must not hold any locks the callback code-flow can take when
> calling kds_async_waitall.
> + * Caller considered to own/use the resources until \a kds_rset_release is
> called.
> + * flags is one or more of the KDS_FLAG_* set.
> + * exclusive_access_bitmap is a bitmap where a high bit means exclusive
> access while a low bit means shared access.
> + * Use the Linux __set_bit API, where the index of the buffer to control is
> used as the bit index.
> + *
> + * Standard Linux error return value.
> + */
> +int kds_async_waitall(
> +               struct kds_resource_set ** pprset,
> +               unsigned long              flags,
> +               struct kds_callback *      cb,
> +               void *                     callback_parameter,
> +               void *                     callback_extra_parameter,
> +               int                        number_resources,
> +               unsigned long *            exclusive_access_bitmap,

hmm, is there advantage of passing the requested resources this way,
vs. just having two arrays (one for exclusive access, one for shared
access)?

> +               struct kds_resource **     resource_list);

callback_parameter + callback_extra_parameter seems a bit, well, odd.
I'm guessing that is some implementation detail about mali driver
peaking through?

But maybe instead of inventing something new, we can just use 'struct
kthread_work' instead of 'struct kds_callback' plus the two 'void *'s?
 If the user needs some extra args they can embed 'struct
kthread_work' in their own struct and use container_of() magic in the
cb.

Plus this is a natural fit if you want to dispatch callbacks instead
on a kthread_worker, which seems like it would simplify a few things
when it comes to deadlock avoidance.. ie., not resource deadlock
avoidance, but dispatching callbacks when some lock is held.

/me wonders what sort of fun would otherwise happen if the cb ever
indirectly called kds_resource_set_release()?


> +/* Synchronous wait for a set of resources.
> + * Function will return when one of these have happened:
> + * - all resources have been obtained
> + * - timeout lapsed while waiting
> + * - a signal was received while waiting
> + *
> + * Caller considered to own/use the resources when the function returns.
> + * Caller must release the resources using \a kds_rset_release.
> + *
> + * Calling this function while holding already locked resources or other
> locking primitives is dangerous.
> + * One must if this is needed decide on a lock order of the resources
> and/or the other locking primitives
> + * and always take the resources/locking primitives in the specific order.
> + *
> + * Use the ERR_PTR framework to decode the return value.
> + * NULL = time out
> + * If IS_ERR then PTR_ERR gives:
> + *  ERESTARTSYS = signal received, retry call after signal
> + *  all other values = internal error, lock failed
> + * Other values  = successful wait, now the owner, must call
> kds_resource_set_release
> + */
> +struct kds_resource_set * kds_waitall(
> +               int                    number_resources,
> +               unsigned long *        exclusive_access_bitmap,
> +               struct kds_resource ** resource_list,
> +               unsigned long          jifies_timeout);
> +
> +/* Release resources after use.
> + * Caller must handle that other async callbacks will trigger,
> + * so must avoid holding any locks a callback will take.
> + *
> + * The function takes a pointer to your poiner to handle a race
> + * between a cancelation and a completion.
> + *
> + * If the caller can't guarantee that a race can't occur then
> + * the passed in pointer must be the same in both call paths
> + * to allow kds to manage the potential race.
> + */
> +void kds_resource_set_release(struct kds_resource_set ** pprset);

maybe using a worker as mentioned above for dealing w/ async cb's
simplify's things?  Maybe I'm a bit paranoid about locking around all
of this, but basically the synchronization framework ends up having to
deal w/ all the potential deadlock and recursive lock issues that we
tried to hide from w/ dmabuf ;-)


BR,
-R


> +
> +#endif /* _KDS_H_ */
> +
>
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [Linaro-mm-sig] [RFC] Synchronizing access to buffers shared with dma-buf between drivers/devices

[John Reitan]

Hi All,

I'm the original designer of the KDS system that Tom posted 
while I was on paternity leave. Find my responses inline...

> -----Original Message-----
> From: linaro-mm-sig-bounces@lists.linaro.org [mailto:linaro-mm-sig-
> bounces@lists.linaro.org] On Behalf Of Rob Clark
> Sent: Monday, June 04, 2012 10:31 PM
> To: Tom Cooksey
> Cc: linaro-mm-sig@lists.linaro.org; Pauli; dri-
> devel@lists.freedesktop.org
> Subject: Re: [Linaro-mm-sig] [RFC] Synchronizing access to buffers
> shared with dma-buf between drivers/devices
> 
> Some comments inline.. at this stage mostly superficial issues about
> how the API works, etc..  not had a chance to dig too much into the
> implementation yet (although some of my comments about the API would
> change those anyways).

It was the API we really wanted input on.
The implementation still leaves a bit to be desired. 

> Anyways, thanks for getting the ball rolling on this, and I think I
> can volunteer linaro to pick up and run w/ this if needed.
> 
> On Fri, May 25, 2012 at 7:08 PM, Tom Cooksey <tom.cooksey@arm.com>
> wrote:
> > Hi All,
> >
> > I realise it's been a while since this was last discussed, however
> I'd like
> > to bring up kernel-side synchronization again. By kernel-side
> > synchronization, I mean allowing multiple drivers/devices wanting to
> access
> > the same buffer to do so without bouncing up to userspace to resolve
> > dependencies such as "the display controller can't start scanning out
> a
> > buffer until the GPU has finished rendering into it". As such, this
> is
> > really just an optimization which reduces latency between E.g. The
> GPU
> > finishing a rendering job and that buffer being scanned out. I
> appreciate
> > this particular example is already solved on desktop graphics cards
> as the
> > display controller and 3D core are both controlled by the same
> driver, so no
> > "generic" mechanism is needed. However on ARM SoCs, the 3D core (like
> an ARM
> > Mali) and display controller tend to be driven by separate drivers,
> so some
> > mechanism is needed to allow both drivers to synchronize their access
> to
> > buffers.
> >
> > There are multiple ways synchronization can be achieved, fences/sync
> objects
> > is one common approach, however we're presenting a different
> approach.
> > Personally, I quite like fence sync objects, however we believe it
> requires
> > a lot of userspace interfaces to be changed to pass around sync
> object
> > handles. Our hope is that the kds approach will require less effort
> to make
> > use of as no existing userspace interfaces need to be changed. E.g.
> To use
> > explicit fences, the struct drm_mode_crtc_page_flip would need a new
> members
> > to pass in the handle(s) of sync object(s) which the flip depends on
> (I.e.
> > don't flip until these fences fire). The additional benefit of our
> approach
> > is that it prevents userspace specifying dependency loops which can
> cause a
> > deadlock (see kds.txt for an explanation of what I mean here).
> >
> > I have waited until now to bring this up again because I am now able
> to
> > share the code I was trying (and failing I think) to explain
> previously. The
> > code has now been released under the GPLv2 from ARM Mali's developer
> portal,
> > however I've attempted to turn that into a patch to allow it to be
> discussed
> > on this list. Please find the patch inline below.
> >
> > While KDS defines a very generic mechanism, I am proposing that this
> code or
> > at least the concepts be merged with the existing dma_buf code, so a
> the
> > struct kds_resource members get moved to struct dma_buf, kds_*
> functions get
> > renamed to dma_buf_* functions, etc. So I guess what I'm saying is
> please
> > don't review the actual code just yet, only the concepts the code
> describes,
> > where kds_resource == dma_duf.
> >
> >
> > Cheers,
> >
> > Tom
> >
> >
> >
> > Author: Tom Cooksey <tom.cooksey@arm.com>
> > Date:   Fri May 25 10:45:27 2012 +0100
> >
> >    Add new system to allow synchronizing access to resources
> >
> >    See Documentation/kds.txt for details, however the general
> >    idea is that this kds framework synchronizes multiple drivers
> >    ("clients") wanting to access the same resources, where a
> >    resource is typically a 2D image buffer being shared around
> >    using dma-buf.
> >
> >    Note: This patch is created by extracting the sources from the
> >    tarball on <http://www.malideveloper.com/open-source-mali-gpus-lin
> >    ux-kernel-device-drivers---dev-releases.php> and putting them in
> >    roughly the right places.
> >
> > diff --git a/Documentation/kds.txt b/Documentation/kds.txt
> 
> fwiw, I think the documentation could be made a bit more generic, but
> this and code style, etc shouldn't be too hard to fix
> 
> > new file mode 100644
> > index 0000000..a96db21
> > --- /dev/null
> > +++ b/Documentation/kds.txt
> > @@ -0,0 +1,113 @@
> > +#
> > +# (C) COPYRIGHT 2012 ARM Limited. All rights reserved.
> > +#
> > +# This program is free software and is provided to you under the
> terms of
> > the GNU General Public License version 2
> > +# as published by the Free Software Foundation, and any use by you
> of this
> > program is subject to the terms of such GNU licence.
> > +#
> > +# A copy of the licence is included with the program, and can also
> be
> > obtained from Free Software
> > +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> > 02110-1301, USA.
> > +#
> > +#
> > +
> > +
> > +==============================
> > +kds - Kernel Dependency System
> > +==============================
> > +
> > +Introduction
> > +------------
> > +kds provides a mechanism for clients to atomically lock down
> multiple
> > abstract resources.
> > +This can be done either synchronously or asynchronously.
> > +Abstract resources is used to allow a set of clients to use kds to
> control
> > access to any
> > +resource, an example is structured memory buffers.
> > +
> > +kds supports that buffer is locked for exclusive access and sharing
> of
> > buffers.
> > +
> > +kds can be built as either a integrated feature of the kernel or as
> a
> > module.
> > +It supports being compiled as a module both in-tree and out-of-tree.
> > +
> > +
> > +Concepts
> > +--------
> > +A core concept in kds is abstract resources.
> > +A kds resource is just an abstraction for some client object, kds
> doesn't
> > care what it is.
> > +Typically EGL will consider UMP buffers as being a resource, thus
> each UMP
> > buffer has
> > +a kds resource for synchronization to the buffer.
> > +
> > +kds allows a client to create and destroy the abstract resource
> objects.
> > +A new resource object is made available asap (it is just a simple
> malloc
> > with some initializations),
> > +while destroy it requires some external synchronization.
> > +
> > +The other core concept in kds is consumer of resources.
> > +kds is requested to allow a client to consume a set of resources and
> the
> > client will be notified when it can consume the resources.
> > +
> > +Exclusive access allows only one client to consume a resource.
> > +Shared access permits multiple consumers to acceess a resource
> > concurrently.
> > +
> > +
> > +APIs
> > +----
> > +kds provides simple resource allocate and destroy functions.
> > +Clients use this to instantiate and control the lifetime of the
> resources
> > kds manages.
> > +
> > +kds provides two ways to wait for resources:
> > +- Asynchronous wait: the client specifies a function pointer to be
> called
> > when wait is over
> > +- Synchronous wait: Function blocks until access is gained.
> > +
> > +The synchronous API has a timeout for the wait.
> > +The call can early out if a signal is delivered.
> > +
> > +After a client is done consuming the resource kds must be notified
> to
> > release the resources and let some other client take ownership.
> > +This is done via resource set release call.
> > +
> > +A Windows comparison:
> > +kds implements WaitForMultipleObjectsEx(..., bWaitAll = TRUE, ...)
> but also
> > has an asynchronous version in addition.
> > +kds resources can be seen as being the same as NT object manager
> resources.
> > +
> > +Internals
> > +---------
> > +kds guarantees atomicity when a set of resources is operated on.
> > +This is implemented via a global resource lock which is taken by kds
> when
> > it updates resource objects.
> > +
> > +Internally a resource in kds is a linked list head with some flags.
> > +
> > +When a consumer requests access to a set of resources it is queued
> on each
> > of the resources.
> > +The link from the consumer to the resources can be triggered. Once
> all
> > links are triggered
> > +the registered callback is called or the blocking function returns.
> > +A link is considered triggered if it is the first on the list of
> consumers
> > of a resource,
> > +or if all the links ahead of it is marked as shared and itself is of
> the
> > type shared.
> > +
> > +When the client is done consuming the consumer object is removed
> from the
> > linked lists of
> > +the resources and a potential new consumer becomes the head of the
> > resources.
> > +As we add and remove consumers atomically across all resources we
> can
> > guarantee that
> > +we never introduces a A->B + B->A type of loops/deadlocks.
> > +
> > +
> > +kbase/base implementation
> > +-------------------------
> > +A HW job needs access to a set of shared resources.
> > +EGL tracks this and encodes the set along with the atom in the
> ringbuffer.
> > +EGL allocates a (k)base dep object to represent the dependency to
> the set
> > of resources and encodes that along with the list of resources.
> > +This dep object is use to create a dependency from a job chain(atom)
> to the
> > resources it needs to run.
> > +When kbase decodes the atom in the ringbuffer it finds the set of
> resources
> > and calls kds to request all the needed resources.
> > +As EGL needs to know when the kds request is delivered a new base
> event
> > object is needed: atom enqueued. This event is only delivered for
> atoms
> > which uses kds.
> > +The callback kbase registers trigger the dependency object described
> which
> > would trigger the existing JD system to release the job chain.
> > +When the atom is done kds resource set release is call to release
> the
> > resources.
> > +
> > +EGL will typically use exclusive access to the render target, while
> all
> > buffers used as input can be marked as shared.
> > +
> > +
> > +Buffer publish/vsync
> > +--------------------
> > +EGL will use a separate ioctl or DRM flip to request the flip.
> > +If the LCD driver is integrated with kds EGL can do these operations
> early.
> > +The LCD driver must then implement the ioctl or DRM flip to be
> asynchronous
> > with kds async call.
> > +The LCD driver binds a kds resource to each virtual buffer (2
> buffers in
> > case of double-buffering).
> > +EGL will make a dependency to the target kds resource in the kbase
> atom.
> > +After EGL receives a atom enqueued event it can ask the LCD driver
> to pan
> > to the target kds resource.
> > +When the atom is completed it'll release the resource and the LCD
> driver
> > will get its callback.
> > +In the callback it'll load the target buffer into the DMA unit of
> the LCD
> > hardware.
> > +The LCD driver will be the consumer of both buffers for a short
> period.
> > +The LCD driver will call kds resource set release on the previous
> on-screen
> > buffer when the next vsync/dma read end is handled.
> > +
> > +
> > diff --git a/drivers/misc/kds.c b/drivers/misc/kds.c
> > new file mode 100644
> > index 0000000..8d7d55e
> > --- /dev/null
> > +++ b/drivers/misc/kds.c
> > @@ -0,0 +1,461 @@
> > +/*
> > + *
> > + * (C) COPYRIGHT 2012 ARM Limited. All rights reserved.
> > + *
> > + * This program is free software and is provided to you under the
> terms of
> > the GNU General Public License version 2
> > + * as published by the Free Software Foundation, and any use by you
> of this
> > program is subject to the terms of such GNU licence.
> > + *
> > + * A copy of the licence is included with the program, and can also
> be
> > obtained from Free Software
> > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> > 02110-1301, USA.
> > + *
> > + */
> > +
> > +
> > +
> > +#include <linux/slab.h>
> > +#include <linux/list.h>
> > +#include <linux/mutex.h>
> > +#include <linux/wait.h>
> > +#include <linux/sched.h>
> > +#include <linux/err.h>
> > +#include <linux/module.h>
> > +#include <linux/workqueue.h>
> > +#include <linux/kds.h>
> > +
> > +
> > +#define KDS_LINK_TRIGGERED (1u << 0)
> > +#define KDS_LINK_EXCLUSIVE (1u << 1)
> > +
> > +#define KDS_IGNORED NULL
> > +#define KDS_INVALID (void*)-2
> > +#define KDS_RESOURCE (void*)-1
> > +
> > +struct kds_resource_set
> > +{
> > +       unsigned long         num_resources;
> > +       unsigned long         pending;
> > +       unsigned long         locked_resources;
> > +       struct kds_callback * cb;
> > +       void *                callback_parameter;
> > +       void *                callback_extra_parameter;
> > +       struct list_head      callback_link;
> > +       struct work_struct    callback_work;
> > +       struct kds_link       resources[0];
> > +};
> > +
> > +static DEFINE_MUTEX(kds_lock);
> > +
> > +int kds_callback_init(struct kds_callback * cb, int direct,
> kds_callback_fn
> > user_cb)
> > +{
> > +       int ret = 0;
> > +
> > +       cb->direct = direct;
> > +       cb->user_cb = user_cb;
> > +
> > +       if (!direct)
> > +       {
> > +               cb->wq = alloc_workqueue("kds", WQ_UNBOUND |
> WQ_HIGHPRI,
> > WQ_UNBOUND_MAX_ACTIVE);
> > +               if (!cb->wq)
> > +                       ret = -ENOMEM;
> > +       }
> > +       else
> > +       {
> > +               cb->wq = NULL;
> > +       }
> > +
> > +       return ret;
> > +}
> > +EXPORT_SYMBOL(kds_callback_init);
> > +
> > +void kds_callback_term(struct kds_callback * cb)
> > +{
> > +       if (!cb->direct)
> > +       {
> > +               BUG_ON(!cb->wq);
> > +               destroy_workqueue(cb->wq);
> > +       }
> > +       else
> > +       {
> > +               BUG_ON(cb->wq);
> > +       }
> > +}
> > +
> > +EXPORT_SYMBOL(kds_callback_term);
> > +
> > +static void kds_do_user_callback(struct kds_resource_set * rset)
> > +{
> > +       rset->cb->user_cb(rset->callback_parameter,
> > rset->callback_extra_parameter);
> > +}
> > +
> > +static void kds_queued_callback(struct work_struct * work)
> > +{
> > +       struct kds_resource_set * rset;
> > +       rset = container_of( work, struct kds_resource_set,
> callback_work);
> > +
> > +       kds_do_user_callback(rset);
> > +}
> > +
> > +static void kds_callback_perform(struct kds_resource_set * rset)
> > +{
> > +       if (rset->cb->direct)
> > +               kds_do_user_callback(rset);
> > +       else
> > +       {
> > +               int result;
> > +               result = queue_work(rset->cb->wq, &rset-
> >callback_work);
> > +               /* if we got a 0 return it means we've triggered the
> same
> > rset twice! */
> > +               BUG_ON(!result);
> > +       }
> > +}
> > +
> > +void kds_resource_init(struct kds_resource * res)
> > +{
> > +       BUG_ON(!res);
> > +       INIT_LIST_HEAD(&res->waiters.link);
> > +       res->waiters.parent = KDS_RESOURCE;
> > +}
> > +EXPORT_SYMBOL(kds_resource_init);
> > +
> > +void kds_resource_term(struct kds_resource * res)
> > +{
> > +       BUG_ON(!res);
> > +       BUG_ON(!list_empty(&res->waiters.link));
> > +       res->waiters.parent = KDS_INVALID;
> > +}
> > +EXPORT_SYMBOL(kds_resource_term);
> > +
> > +int kds_async_waitall(
> > +               struct kds_resource_set ** pprset,
> > +               unsigned long              flags,
> > +               struct kds_callback *      cb,
> > +               void *                     callback_parameter,
> > +               void *                     callback_extra_parameter,
> > +               int                        number_resources,
> > +               unsigned long *            exclusive_access_bitmap,
> > +               struct kds_resource **     resource_list)
> > +{
> > +       struct kds_resource_set * rset = NULL;
> > +       int i;
> > +       int triggered;
> > +       int err = -EFAULT;
> > +
> > +       BUG_ON(!pprset);
> > +       BUG_ON(!resource_list);
> > +       BUG_ON(!cb);
> > +
> > +       mutex_lock(&kds_lock);
> > +
> > +       if ((flags & KDS_FLAG_LOCKED_ACTION) == KDS_FLAG_LOCKED_FAIL)
> > +       {
> > +               for (i = 0; i < number_resources; i++)
> > +               {
> > +                       if (resource_list[i]->lock_count)
> > +                       {
> > +                               err = -EBUSY;
> > +                               goto errout;
> > +                       }
> > +               }
> > +       }
> > +
> > +       rset = kmalloc(sizeof(*rset) + number_resources *
> sizeof(struct
> > kds_link), GFP_KERNEL);
> > +       if (!rset)
> > +       {
> > +               err = -ENOMEM;
> > +               goto errout;
> > +       }
> > +
> > +       rset->num_resources = number_resources;
> > +       rset->pending = number_resources;
> > +       rset->locked_resources = 0;
> > +       rset->cb = cb;
> > +       rset->callback_parameter = callback_parameter;
> > +       rset->callback_extra_parameter = callback_extra_parameter;
> > +       INIT_LIST_HEAD(&rset->callback_link);
> > +       INIT_WORK(&rset->callback_work, kds_queued_callback);
> > +
> > +       for (i = 0; i < number_resources; i++)
> > +       {
> > +               unsigned long link_state = 0;
> > +
> > +               INIT_LIST_HEAD(&rset->resources[i].link);
> > +               rset->resources[i].parent = rset;
> > +
> > +               if (test_bit(i, exclusive_access_bitmap))
> > +               {
> > +                       link_state |= KDS_LINK_EXCLUSIVE;
> > +               }
> > +
> > +               /* no-one else waiting? */
> > +               if (list_empty(&resource_list[i]->waiters.link))
> > +               {
> > +                       link_state |= KDS_LINK_TRIGGERED;
> > +                       rset->pending--;
> > +               }
> > +               /* Adding a non-exclusive and the current tail is a
> > triggered non-exclusive? */
> > +               else if (((link_state & KDS_LINK_EXCLUSIVE) == 0) &&
> > +                       (((list_entry(resource_list[i]-
> >waiters.link.prev,
> > struct kds_link, link)->state & (KDS_LINK_EXCLUSIVE |
> KDS_LINK_TRIGGERED))
> > == KDS_LINK_TRIGGERED)))
> > +               {
> > +                       link_state |= KDS_LINK_TRIGGERED;
> > +                       rset->pending--;
> > +               }
> > +               /* locked & ignore locked? */
> > +               else if ((resource_list[i]->lock_count) && ((flags &
> > KDS_FLAG_LOCKED_ACTION) == KDS_FLAG_LOCKED_IGNORE) )
> > +               {
> > +                       link_state |= KDS_LINK_TRIGGERED;
> > +                       rset->pending--;
> > +                       rset->resources[i].parent = KDS_IGNORED; /*
> to
> > disable decrementing the pending count when we get the ignored
> resource */
> > +               }
> > +               rset->resources[i].state = link_state;
> > +               list_add_tail(&rset->resources[i].link,
> > &resource_list[i]->waiters.link);
> > +       }
> > +
> > +       triggered = (rset->pending == 0);
> > +
> > +       mutex_unlock(&kds_lock);
> > +
> > +       /* set the pointer before the callback is called so it sees
> it */
> > +       *pprset = rset;
> > +
> > +       if (triggered)
> > +       {
> > +               /* all resources obtained, trigger callback */
> > +               kds_callback_perform(rset);
> > +       }
> > +
> > +       return 0;
> > +
> > +errout:
> > +       mutex_unlock(&kds_lock);
> > +       return err;
> > +}
> > +EXPORT_SYMBOL(kds_async_waitall);
> > +
> > +static void wake_up_sync_call(void * callback_parameter, void *
> > callback_extra_parameter)
> > +{
> > +       wait_queue_head_t * wait =
> (wait_queue_head_t*)callback_parameter;
> > +       wake_up(wait);
> > +}
> > +
> > +static struct kds_callback sync_cb =
> > +{
> > +       wake_up_sync_call,
> > +       1,
> > +       NULL,
> > +};
> > +
> > +struct kds_resource_set * kds_waitall(
> > +               int                    number_resources,
> > +               unsigned long *        exclusive_access_bitmap,
> > +               struct kds_resource ** resource_list,
> > +               unsigned long          jiffies_timeout)
> > +{
> > +       struct kds_resource_set * rset;
> > +       int i;
> > +       int triggered = 0;
> > +       DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wake);
> > +
> > +       rset = kmalloc(sizeof(*rset) + number_resources *
> sizeof(struct
> > kds_link), GFP_KERNEL);
> > +       if (!rset)
> > +               return rset;
> > +
> > +       rset->num_resources = number_resources;
> > +       rset->pending = number_resources;
> > +       rset->locked_resources = 1;
> > +       INIT_LIST_HEAD(&rset->callback_link);
> > +       INIT_WORK(&rset->callback_work, kds_queued_callback);
> > +
> > +       mutex_lock(&kds_lock);
> > +
> > +       for (i = 0; i < number_resources; i++)
> > +       {
> > +               unsigned long link_state = 0;
> > +
> > +               if (likely(resource_list[i]->lock_count < ULONG_MAX))
> > +                       resource_list[i]->lock_count++;
> > +               else
> > +                       break;
> > +
> > +               if (test_bit(i, exclusive_access_bitmap))
> > +               {
> > +                       link_state |= KDS_LINK_EXCLUSIVE;
> > +               }
> > +
> > +               if (list_empty(&resource_list[i]->waiters.link))
> > +               {
> > +                       link_state |= KDS_LINK_TRIGGERED;
> > +                       rset->pending--;
> > +               }
> > +               /* Adding a non-exclusive and the current tail is a
> > triggered non-exclusive? */
> > +               else if (((link_state & KDS_LINK_EXCLUSIVE) == 0) &&
> > +                       (((list_entry(resource_list[i]-
> >waiters.link.prev,
> > struct kds_link, link)->state & (KDS_LINK_EXCLUSIVE |
> KDS_LINK_TRIGGERED))
> > == KDS_LINK_TRIGGERED)))
> > +               {
> > +                       link_state |= KDS_LINK_TRIGGERED;
> > +                       rset->pending--;
> > +               }
> > +
> > +               INIT_LIST_HEAD(&rset->resources[i].link);
> > +               rset->resources[i].parent = rset;
> > +               rset->resources[i].state = link_state;
> > +               list_add_tail(&rset->resources[i].link,
> > &resource_list[i]->waiters.link);
> > +       }
> > +
> > +       if (i < number_resources)
> > +       {
> > +               /* an overflow was detected, roll back */
> > +               while (i--)
> > +               {
> > +                       list_del(&rset->resources[i].link);
> > +                       resource_list[i]->lock_count--;
> > +               }
> > +               mutex_unlock(&kds_lock);
> > +               kfree(rset);
> > +               return ERR_PTR(-EFAULT);
> > +       }
> > +
> > +       if (rset->pending == 0)
> > +               triggered = 1;
> > +       else
> > +       {
> > +               rset->cb = &sync_cb;
> > +               rset->callback_parameter = &wake;
> > +               rset->callback_extra_parameter = NULL;
> > +       }
> > +
> > +       mutex_unlock(&kds_lock);
> > +
> > +       if (!triggered)
> > +       {
> > +               long wait_res;
> > +               if ( KDS_WAIT_BLOCKING == jiffies_timeout )
> > +               {
> > +                       wait_res = wait_event_interruptible(wake,
> > rset->pending == 0);
> > +               }
> > +               else
> > +               {
> > +                       wait_res =
> wait_event_interruptible_timeout(wake,
> > rset->pending == 0, jiffies_timeout);
> > +               }
> > +               if ((wait_res == -ERESTARTSYS) || (wait_res == 0))
> > +               {
> > +                       /* use \a kds_resource_set_release to roll
> back */
> > +                       kds_resource_set_release(&rset);
> > +                       return ERR_PTR(wait_res);
> > +               }
> > +       }
> > +       return rset;
> > +}
> > +EXPORT_SYMBOL(kds_waitall);
> > +
> > +void kds_resource_set_release(struct kds_resource_set ** pprset)
> > +{
> > +       struct list_head triggered = LIST_HEAD_INIT(triggered);
> > +       struct kds_resource_set * rset;
> > +       struct kds_resource_set * it;
> > +       int i;
> > +
> > +       BUG_ON(!pprset);
> > +
> > +       mutex_lock(&kds_lock);
> > +
> > +       rset = *pprset;
> > +       if (!rset)
> > +       {
> > +               /* caught a race between a cancelation
> > +                * and a completion, nothing to do */
> > +               mutex_unlock(&kds_lock);
> > +               return;
> > +       }
> > +
> > +       /* clear user pointer so we'll be the only
> > +        * thread handling the release */
> > +       *pprset = NULL;
> > +
> > +       for (i = 0; i < rset->num_resources; i++)
> > +       {
> > +               struct kds_resource * resource;
> > +               struct kds_link * it = NULL;
> > +
> > +               /* fetch the previous entry on the linked list */
> > +               it = list_entry(rset->resources[i].link.prev, struct
> > kds_link, link);
> > +               /* unlink ourself */
> > +               list_del(&rset->resources[i].link);
> > +
> > +               /* any waiters? */
> > +               if (list_empty(&it->link))
> > +                       continue;
> > +
> > +               /* were we the head of the list? (head if prev is a
> > resource) */
> > +               if (it->parent != KDS_RESOURCE)
> > +                       continue;
> > +
> > +               /* we were the head, find the kds_resource */
> > +               resource = container_of(it, struct kds_resource,
> waiters);
> > +
> > +               if (rset->locked_resources)
> > +               {
> > +                       resource->lock_count--;
> > +               }
> > +
> > +               /* we know there is someone waiting from the any-
> waiters
> > test above */
> > +
> > +               /* find the head of the waiting list */
> > +               it = list_first_entry(&resource->waiters.link, struct
> > kds_link, link);
> > +
> > +               /* new exclusive owner? */
> > +               if (it->state & KDS_LINK_EXCLUSIVE)
> > +               {
> > +                       /* link now triggered */
> > +                       it->state |= KDS_LINK_TRIGGERED;
> > +                       /* a parent to update? */
> > +                       if (it->parent != KDS_IGNORED)
> > +                       {
> > +                               if (0 == --it->parent->pending)
> > +                               {
> > +                                       /* new owner now triggered,
> track
> > for callback later */
> > +                                       list_add(&it->parent-
> >callback_link,
> > &triggered);
> > +                               }
> > +                       }
> > +               }
> > +               /* exclusive releasing ? */
> > +               else if (rset->resources[i].state &
> KDS_LINK_EXCLUSIVE)
> > +               {
> > +                       /* trigger non-exclusive until end-of-list or
> first
> > exclusive */
> > +                       list_for_each_entry(it, &resource-
> >waiters.link,
> > link)
> > +                       {
> > +                               /* exclusive found, stop triggering
> */
> > +                               if (it->state & KDS_LINK_EXCLUSIVE)
> > +                                       break;
> > +
> > +                               it->state |= KDS_LINK_TRIGGERED;
> > +                               /* a parent to update? */
> > +                               if (it->parent != KDS_IGNORED)
> > +                               {
> > +                                       if (0 == --it->parent-
> >pending)
> > +                                       {
> > +                                               /* new owner now
> triggered,
> > track for callback later */
> > +
> > list_add(&it->parent->callback_link, &triggered);
> > +                                       }
> > +                               }
> > +                       }
> > +               }
> > +
> > +       }
> > +
> > +       mutex_unlock(&kds_lock);
> > +
> > +       while (!list_empty(&triggered))
> > +       {
> > +               it = list_first_entry(&triggered, struct
> kds_resource_set,
> > callback_link);
> > +               list_del(&it->callback_link);
> > +               kds_callback_perform(it);
> > +       }
> > +
> > +       cancel_work_sync(&rset->callback_work);
> > +
> > +       /* free the resource set */
> > +       kfree(rset);
> > +}
> > +EXPORT_SYMBOL(kds_resource_set_release);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("ARM Ltd.");
> > +MODULE_VERSION("1.0");
> > diff --git a/include/linux/kds.h b/include/linux/kds.h
> > new file mode 100644
> > index 0000000..65e5706
> > --- /dev/null
> > +++ b/include/linux/kds.h
> > @@ -0,0 +1,154 @@
> > +/*
> > + *
> > + * (C) COPYRIGHT 2012 ARM Limited. All rights reserved.
> > + *
> > + * This program is free software and is provided to you under the
> terms of
> > the GNU General Public License version 2
> > + * as published by the Free Software Foundation, and any use by you
> of this
> > program is subject to the terms of such GNU licence.
> > + *
> > + * A copy of the licence is included with the program, and can also
> be
> > obtained from Free Software
> > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> > 02110-1301, USA.
> > + *
> > + */
> > +
> > +
> > +
> > +#ifndef _KDS_H_
> > +#define _KDS_H_
> > +
> > +#include <linux/list.h>
> > +#include <linux/workqueue.h>
> > +
> > +#define KDS_WAIT_BLOCKING (ULONG_MAX)
> > +
> > +/* what to do when waitall must wait for a synchronous locked
> resource: */
> > +#define KDS_FLAG_LOCKED_FAIL    (0u <<  0) /* fail waitall */
> > +#define KDS_FLAG_LOCKED_IGNORE  (1u <<  0) /* don't wait, but block
> other
> > that waits */
> > +#define KDS_FLAG_LOCKED_WAIT    (2u <<  0) /* wait (normal */
> > +#define KDS_FLAG_LOCKED_ACTION  (3u <<  0) /* mask to extract the
> action to
> > do on locked resources */
> > +
> > +struct kds_resource_set;
> > +
> > +typedef void (*kds_callback_fn) (void * callback_parameter, void *
> > callback_extra_parameter);
> > +
> > +struct kds_callback
> > +{
> > +       kds_callback_fn  user_cb; /* real cb */
> > +       int direct;               /* do direct or queued call? */
> > +       struct workqueue_struct * wq;
> > +};
> > +
> > +struct kds_link
> > +{
> > +       struct kds_resource_set * parent;
> > +       struct list_head          link;
> > +       unsigned long             state;
> > +};
> > +
> > +struct kds_resource
> > +{
> > +       struct kds_link waiters;
> > +       unsigned long lock_count;
> > +};
> > +
> > +/* callback API */
> > +
> > +/* Initialize a callback object.
> > + *
> > + * Typically created per context or per hw resource.
> > + *
> > + * Callbacks can be performed directly if no nested locking can
> > + * happen in the client.
> > + *
> > + * Nested locking can occur when a lock is held during the
> > kds_async_waitall or
> > + * kds_resource_set_release call. If the callback needs to take the
> same
> > lock
> > + * nested locking will happen.
> > + *
> > + * If nested locking could happen non-direct callbacks can be
> requested.
> > + * Callbacks will then be called asynchronous to the triggering
> call.
> > + */
> > +int kds_callback_init(struct kds_callback * cb, int direct,
> kds_callback_fn
> > user_cb);
> > +
> > +/* Terminate the use of a callback object.
> > + *
> > + * If the callback object was set up as non-direct
> > + * any pending callbacks will be flushed first.
> > + * Note that to avoid a deadlock the lock callbacks needs
> > + * can't be held when a callback object is terminated.
> > + */
> > +void kds_callback_term(struct kds_callback * cb);
> 
> hmm, not hugely a fan of this.. having callbacks that might need to
> aquire locks be potentially called synchronously in special cases.
> 
> It seems like it can get simpler if pending callbacks could hold a
> reference to the underlying object until the callback completes.
> Although not quite sure offhand how that can work without coupling kds
> to dmabuf or GEM..

Better to use proper ref-counting, agree.
Then the term is just a simple deref.
Then we might want to add a deref_and_wait version to 
know that the callback has completed?

> > +
> > +/* resource object API */
> > +
> > +/* initialize a resource handle for a shared resource */
> > +void kds_resource_init(struct kds_resource * resource);
> > +
> > +/*
> > + * Will assert if the resource is being used or waited on.
> > + * The caller should NOT try to terminate a resource that could
> still have
> > clients.
> > + * After the function returns the resource is no longer known by
> kds.
> > + */
> > +void kds_resource_term(struct kds_resource * resource);
> > +
> > +/* Asynchronous wait for a set of resources.
> > + * Callback will be called when all resources are available.
> > + * If all the resources was available the callback will be called
> before
> > kds_async_waitall returns.
> > + * So one must not hold any locks the callback code-flow can take
> when
> > calling kds_async_waitall.
> > + * Caller considered to own/use the resources until \a
> kds_rset_release is
> > called.
> > + * flags is one or more of the KDS_FLAG_* set.
> > + * exclusive_access_bitmap is a bitmap where a high bit means
> exclusive
> > access while a low bit means shared access.
> > + * Use the Linux __set_bit API, where the index of the buffer to
> control is
> > used as the bit index.
> > + *
> > + * Standard Linux error return value.
> > + */
> > +int kds_async_waitall(
> > +               struct kds_resource_set ** pprset,
> > +               unsigned long              flags,
> > +               struct kds_callback *      cb,
> > +               void *                     callback_parameter,
> > +               void *                     callback_extra_parameter,
> > +               int                        number_resources,
> > +               unsigned long *            exclusive_access_bitmap,
> 
> hmm, is there advantage of passing the requested resources this way,
> vs. just having two arrays (one for exclusive access, one for shared
> access)?

Well, you must have one extra arg with your solution:
Two pointers plus an int for the current version
Two pointers plus two ints for your suggestion.

Other than that there is no real reason to not change the API really...

> 
> > +               struct kds_resource **     resource_list);
> 
> callback_parameter + callback_extra_parameter seems a bit, well, odd.
> I'm guessing that is some implementation detail about mali driver
> peaking through?

Guess so :)
This was done as callbacks needed two different things to operate on, 
and instead of mallocing a struct to track the two objects we used the 
existing alloc holding the original ptr.
But continuing with your next comment:

> But maybe instead of inventing something new, we can just use 'struct
> kthread_work' instead of 'struct kds_callback' plus the two 'void *'s?
>  If the user needs some extra args they can embed 'struct
> kthread_work' in their own struct and use container_of() magic in the
> cb.
> 
> Plus this is a natural fit if you want to dispatch callbacks instead
> on a kthread_worker, which seems like it would simplify a few things
> when it comes to deadlock avoidance.. ie., not resource deadlock
> avoidance, but dispatching callbacks when some lock is held.

That sounds like a better approach.
Will make a cleaner API, will look into it.
 
> /me wonders what sort of fun would otherwise happen if the cb ever
> indirectly called kds_resource_set_release()?

It should work.
The callback is performed without any kds side locks held.
When a callback returns the resource set object should not be 
accessed again, so it's safe for it to have been freed by 
kds_resource_set_release.

> 
>
> > +/* Synchronous wait for a set of resources.
> > + * Function will return when one of these have happened:
> > + * - all resources have been obtained
> > + * - timeout lapsed while waiting
> > + * - a signal was received while waiting
> > + *
> > + * Caller considered to own/use the resources when the function
> returns.
> > + * Caller must release the resources using \a kds_rset_release.
> > + *
> > + * Calling this function while holding already locked resources or
> other
> > locking primitives is dangerous.
> > + * One must if this is needed decide on a lock order of the
> resources
> > and/or the other locking primitives
> > + * and always take the resources/locking primitives in the specific
> order.
> > + *
> > + * Use the ERR_PTR framework to decode the return value.
> > + * NULL = time out
> > + * If IS_ERR then PTR_ERR gives:
> > + *  ERESTARTSYS = signal received, retry call after signal
> > + *  all other values = internal error, lock failed
> > + * Other values  = successful wait, now the owner, must call
> > kds_resource_set_release
> > + */
> > +struct kds_resource_set * kds_waitall(
> > +               int                    number_resources,
> > +               unsigned long *        exclusive_access_bitmap,
> > +               struct kds_resource ** resource_list,
> > +               unsigned long          jifies_timeout);
> > +
> > +/* Release resources after use.
> > + * Caller must handle that other async callbacks will trigger,
> > + * so must avoid holding any locks a callback will take.
> > + *
> > + * The function takes a pointer to your poiner to handle a race
> > + * between a cancelation and a completion.
> > + *
> > + * If the caller can't guarantee that a race can't occur then
> > + * the passed in pointer must be the same in both call paths
> > + * to allow kds to manage the potential race.
> > + */
> > +void kds_resource_set_release(struct kds_resource_set ** pprset);
> 
> maybe using a worker as mentioned above for dealing w/ async cb's
> simplify's things?  Maybe I'm a bit paranoid about locking around all
> of this, but basically the synchronization framework ends up having to
> deal w/ all the potential deadlock and recursive lock issues that we
> tried to hide from w/ dmabuf ;-)

With the kthread change the lock problems will be reduced.
But the real point of this framework is to remove lock order problems
when you need to lock down two or more more resources, but you're
not in control of all the code operating on the buffers, which is
typical for many SoC projects.
But yes, this system must be vetted for all potential deadlocks
that it could introduce.

Cheers,
John


> 
> 
> BR,
> -R
> 
> 
> > +
> > +#endif /* _KDS_H_ */
> > +
> >
> >
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> _______________________________________________
> Linaro-mm-sig mailing list
> Linaro-mm-sig@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-mm-sig

Re: [Linaro-mm-sig] [RFC] Synchronizing access to buffers shared with dma-buf between drivers/devices

[Erik Gilling]

On Wed, Jun 6, 2012 at 6:33 AM, John Reitan <john.reitan@arm.com> wrote:
>> But maybe instead of inventing something new, we can just use 'struct
>> kthread_work' instead of 'struct kds_callback' plus the two 'void *'s?
>>  If the user needs some extra args they can embed 'struct
>> kthread_work' in their own struct and use container_of() magic in the
>> cb.
>>
>> Plus this is a natural fit if you want to dispatch callbacks instead
>> on a kthread_worker, which seems like it would simplify a few things
>> when it comes to deadlock avoidance.. ie., not resource deadlock
>> avoidance, but dispatching callbacks when some lock is held.
>
> That sounds like a better approach.
> Will make a cleaner API, will look into it.

When Tom visited us for android graphics camp in the fall he argued
that there were cases where we would want to avoid an extra schedule.
Consider the case where the GPU is waiting for a render buffer that
the display controller is using.  If that render can be kicked off w/o
acquiring locks, the display's vsync IRQ handler can call release,
which in turn calls the GPU callback, which in turn kicks off the
render very quickly w/o having to leave IRQ context.

One way around the locking issue with callbacks/async wait is to have
async wait return a value to indicate that the resource has been
acquired instead of calling the callback.  This is the approach I
chose in our sync framework.

-Erik