Re: [RFC PATCH] dmabuf-sync: Introduce buffer synchronization framework

[Russell King - ARM Linux <linux@arm.linux.org.uk>]

On Thu, Jun 13, 2013 at 05:28:08PM +0900, Inki Dae wrote:
> This patch adds a buffer synchronization framework based on DMA BUF[1]
> and reservation[2] to use dma-buf resource, and based on ww-mutexes[3]
> for lock mechanism.
> 
> The purpose of this framework is not only to couple cache operations,
> and buffer access control to CPU and DMA but also to provide easy-to-use
> interfaces for device drivers and potentially user application
> (not implemented for user applications, yet). And this framework can be
> used for all dma devices using system memory as dma buffer, especially
> for most ARM based SoCs.
> 
> The mechanism of this framework has the following steps,
>     1. Register dmabufs to a sync object - A task gets a new sync object and
>     can add one or more dmabufs that the task wants to access.
>     This registering should be performed when a device context or an event
>     context such as a page flip event is created or before CPU accesses a shared
>     buffer.
> 
> 	dma_buf_sync_get(a sync object, a dmabuf);
> 
>     2. Lock a sync object - A task tries to lock all dmabufs added in its own
>     sync object. Basically, the lock mechanism uses ww-mutex[1] to avoid dead
>     lock issue and for race condition between CPU and CPU, CPU and DMA, and DMA
>     and DMA. Taking a lock means that others cannot access all locked dmabufs
>     until the task that locked the corresponding dmabufs, unlocks all the locked
>     dmabufs.
>     This locking should be performed before DMA or CPU accesses these dmabufs.
> 
> 	dma_buf_sync_lock(a sync object);
> 
>     3. Unlock a sync object - The task unlocks all dmabufs added in its own sync
>     object. The unlock means that the DMA or CPU accesses to the dmabufs have
>     been completed so that others may access them.
>     This unlocking should be performed after DMA or CPU has completed accesses
>     to the dmabufs.
> 
> 	dma_buf_sync_unlock(a sync object);
> 
>     4. Unregister one or all dmabufs from a sync object - A task unregisters
>     the given dmabufs from the sync object. This means that the task dosen't
>     want to lock the dmabufs.
>     The unregistering should be performed after DMA or CPU has completed
>     accesses to the dmabufs or when dma_buf_sync_lock() is failed.
> 
> 	dma_buf_sync_put(a sync object, a dmabuf);
> 	dma_buf_sync_put_all(a sync object);
> 
>     The described steps may be summarized as:
> 	get -> lock -> CPU or DMA access to a buffer/s -> unlock -> put
> 
> This framework includes the following two features.
>     1. read (shared) and write (exclusive) locks - A task is required to declare
>     the access type when the task tries to register a dmabuf;
>     READ, WRITE, READ DMA, or WRITE DMA.
> 
>     The below is example codes,
> 	struct dmabuf_sync *sync;
> 
> 	sync = dmabuf_sync_init(NULL, "test sync");
> 
> 	dmabuf_sync_get(sync, dmabuf, DMA_BUF_ACCESS_READ);
> 	...
> 
> 	And the below can be used as access types:
> 		DMA_BUF_ACCESS_READ,
> 		- CPU will access a buffer for read.
> 		DMA_BUF_ACCESS_WRITE,
> 		- CPU will access a buffer for read or write.
> 		DMA_BUF_ACCESS_READ | DMA_BUF_ACCESS_DMA,
> 		- DMA will access a buffer for read
> 		DMA_BUF_ACCESS_WRITE | DMA_BUF_ACCESS_DMA,
> 		- DMA will access a buffer for read or write.
> 
>     2. Mandatory resource releasing - a task cannot hold a lock indefinitely.
>     A task may never try to unlock a buffer after taking a lock to the buffer.
>     In this case, a timer handler to the corresponding sync object is called
>     in five (default) seconds and then the timed-out buffer is unlocked by work
>     queue handler to avoid lockups and to enforce resources of the buffer.
> 
> The below is how to use:
> 	1. Allocate and Initialize a sync object:
> 		struct dmabuf_sync *sync;
> 
> 		sync = dmabuf_sync_init(NULL, "test sync");
> 		...
> 
> 	2. Add a dmabuf to the sync object when setting up dma buffer relevant
> 	   registers:
> 		dmabuf_sync_get(sync, dmabuf, DMA_BUF_ACCESS_READ);
> 		...
> 
> 	3. Lock all dmabufs of the sync object before DMA or CPU accesses
> 	   the dmabufs:
> 		dmabuf_sync_lock(sync);
> 		...
> 
> 	4. Now CPU or DMA can access all dmabufs locked in step 3.
> 
> 	5. Unlock all dmabufs added in a sync object after DMA or CPU access
> 	   to these dmabufs is completed:
> 		dmabuf_sync_unlock(sync);
> 
> 	   And call the following functions to release all resources,
> 		dmabuf_sync_put_all(sync);
> 		dmabuf_sync_fini(sync);
> 
> 	You can refer to actual example codes:
> 		https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-exynos.git/
> 		commit/?h=dmabuf-sync&id@30bdee9bab5841ad32faade528d04cc0c5fc94
> 
> 		https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-exynos.git/
> 		commit/?h=dmabuf-sync&idla548e9ea9e865592719ef6b1cde58366af9f5c
> 
> The framework performs cache operation based on the previous and current access
> types to the dmabufs after the locks to all dmabufs are taken:
> 	Call dma_buf_begin_cpu_access() to invalidate cache if,
> 		previous access type is DMA_BUF_ACCESS_WRITE | DMA and
> 		current access type is DMA_BUF_ACCESS_READ
> 
> 	Call dma_buf_end_cpu_access() to clean cache if,
> 		previous access type is DMA_BUF_ACCESS_WRITE and
> 		current access type is DMA_BUF_ACCESS_READ | DMA
> 
> Such cache operations are invoked via dma-buf interfaces so the dma buf exporter
> should implement dmabuf->ops->begin_cpu_access/end_cpu_access callbacks.
> 
> [1] http://lwn.net/Articles/470339/
> [2] http://lwn.net/Articles/532616/
> [3] https://patchwork-mail1.kernel.org/patch/2625321/
> 
> Signed-off-by: Inki Dae <inki.dae@samsung.com>
> ---
>  Documentation/dma-buf-sync.txt |  246 ++++++++++++++++++
>  drivers/base/Kconfig           |    7 +
>  drivers/base/Makefile          |    1 +
>  drivers/base/dmabuf-sync.c     |  555 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/dma-buf.h        |    5 +
>  include/linux/dmabuf-sync.h    |  115 +++++++++
>  include/linux/reservation.h    |    7 +
>  7 files changed, 936 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/dma-buf-sync.txt
>  create mode 100644 drivers/base/dmabuf-sync.c
>  create mode 100644 include/linux/dmabuf-sync.h
> 
> diff --git a/Documentation/dma-buf-sync.txt b/Documentation/dma-buf-sync.txt
> new file mode 100644
> index 0000000..e71b6f4
> --- /dev/null
> +++ b/Documentation/dma-buf-sync.txt
> @@ -0,0 +1,246 @@
> +                    DMA Buffer Synchronization Framework
> +                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +                                  Inki Dae
> +                      <inki dot dae at samsung dot com>
> +                          <daeinki at gmail dot com>
> +
> +This document is a guide for device-driver writers describing the DMA buffer
> +synchronization API. This document also describes how to use the API to
> +use buffer synchronization between CPU and CPU, CPU and DMA, and DMA and DMA.
> +
> +The DMA Buffer synchronization API provides buffer synchronization mechanism;
> +i.e., buffer access control to CPU and DMA, cache operations, and easy-to-use
> +interfaces for device drivers and potentially user application
> +(not implemented for user applications, yet). And this API can be used for all
> +dma devices using system memory as dma buffer, especially for most ARM based
> +SoCs.
> +
> +
> +Motivation
> +----------
> +
> +Sharing a buffer, a device cannot be aware of when the other device will access
> +the shared buffer: a device may access a buffer containing wrong data if
> +the device accesses the shared buffer while another device is still accessing
> +the shared buffer. Therefore, a user process should have waited for
> +the completion of DMA access by another device before a device tries to access
> +the shared buffer.
> +
> +Besides, there is the same issue when CPU and DMA are sharing a buffer; i.e.,
> +a user process should consider that when the user process have to send a buffer
> +to a device driver for the device driver to access the buffer as input.
> +This means that a user process needs to understand how the device driver is
> +worked. Hence, the conventional mechanism not only makes user application
> +complicated but also incurs performance overhead because the conventional
> +mechanism cannot control devices precisely without additional and complex
> +implemantations.
> +
> +In addition, in case of ARM based SoCs, most devices have no hardware cache
> +consistency mechanisms between CPU and DMA devices because they do not use ACP
> +(Accelerator Coherency Port). ACP can be connected to DMA engine or similar
> +devices in order to keep cache coherency between CPU cache and DMA device.
> +Thus, we need additional cache operations to have the devices operate properly;
> +i.e., user applications should request cache operations to kernel before DMA
> +accesses the buffer and after the completion of buffer access by CPU, or vise
> +versa.
> +
> +	buffer access by CPU -> cache clean -> buffer access by DMA
> +
> +Or,
> +	buffer access by DMA -> cache invalidate -> buffer access by CPU
> +
> +The below shows why cache operations should be requested by user
> +process,
> +    (Presume that CPU and DMA share a buffer and the buffer is mapped
> +     with user space as cachable)
> +
> +	handle = drm_gem_alloc(size);
> +	...
> +	va1 = drm_gem_mmap(handle1);
> +	va2 = malloc(size);
> +	...
> +
> +	while(conditions) {
> +		memcpy(va1, some data, size);
> +		...
> +		drm_xxx_set_dma_buffer(handle, ...);
> +		...
> +
> +		/* user need to request cache clean at here. */
> +
> +		/* blocked until dma operation is completed. */
> +		drm_xxx_start_dma(...);
> +		...
> +
> +		/* user need to request cache invalidate at here. */
> +
> +		memcpy(va2, va1, size);
> +	}
> +
> +The issue arises: user processes may incur cache operations: user processes may
> +request unnecessary cache operations to kernel. Besides, kernel cannot prevent
> +user processes from requesting such cache operations. Therefore, we need to
> +prevent such excessive and unnecessary cache operations from user processes.
> +
> +
> +Basic concept
> +-------------
> +
> +The mechanism of this framework has the following steps,
> +    1. Register dmabufs to a sync object - A task gets a new sync object and
> +    can add one or more dmabufs that the task wants to access.
> +    This registering should be performed when a device context or an event
> +    context such as a page flip event is created or before CPU accesses a shared
> +    buffer.
> +
> +	dma_buf_sync_get(a sync object, a dmabuf);
> +
> +    2. Lock a sync object - A task tries to lock all dmabufs added in its own
> +    sync object. Basically, the lock mechanism uses ww-mutex[1] to avoid dead
> +    lock issue and for race condition between CPU and CPU, CPU and DMA, and DMA
> +    and DMA. Taking a lock means that others cannot access all locked dmabufs
> +    until the task that locked the corresponding dmabufs, unlocks all the locked
> +    dmabufs.
> +    This locking should be performed before DMA or CPU accesses these dmabufs.
> +
> +	dma_buf_sync_lock(a sync object);
> +
> +    3. Unlock a sync object - The task unlocks all dmabufs added in its own sync
> +    object. The unlock means that the DMA or CPU accesses to the dmabufs have
> +    been completed so that others may access them.
> +    This unlocking should be performed after DMA or CPU has completed accesses
> +    to the dmabufs.
> +
> +	dma_buf_sync_unlock(a sync object);
> +
> +    4. Unregister one or all dmabufs from a sync object - A task unregisters
> +    the given dmabufs from the sync object. This means that the task dosen't
> +    want to lock the dmabufs.
> +    The unregistering should be performed after DMA or CPU has completed
> +    accesses to the dmabufs or when dma_buf_sync_lock() is failed.
> +
> +	dma_buf_sync_put(a sync object, a dmabuf);
> +	dma_buf_sync_put_all(a sync object);
> +
> +    The described steps may be summarized as:
> +	get -> lock -> CPU or DMA access to a buffer/s -> unlock -> put
> +
> +This framework includes the following two features.
> +    1. read (shared) and write (exclusive) locks - A task is required to declare
> +    the access type when the task tries to register a dmabuf;
> +    READ, WRITE, READ DMA, or WRITE DMA.
> +
> +    The below is example codes,
> +	struct dmabuf_sync *sync;
> +
> +	sync = dmabuf_sync_init(NULL, "test sync");
> +
> +	dmabuf_sync_get(sync, dmabuf, DMA_BUF_ACCESS_READ);
> +	...
> +
> +    2. Mandatory resource releasing - a task cannot hold a lock indefinitely.
> +    A task may never try to unlock a buffer after taking a lock to the buffer.
> +    In this case, a timer handler to the corresponding sync object is called
> +    in five (default) seconds and then the timed-out buffer is unlocked by work
> +    queue handler to avoid lockups and to enforce resources of the buffer.
> +
> +
> +Access types
> +------------
> +
> +DMA_BUF_ACCESS_READ - CPU will access a buffer for read.
> +DMA_BUF_ACCESS_WRITE - CPU will access a buffer for read or write.
> +DMA_BUF_ACCESS_READ | DMA_BUF_ACCESS_DMA - DMA will access a buffer for read
> +DMA_BUF_ACCESS_WRITE | DMA_BUF_ACCESS_DMA - DMA will access a buffer for read
> +					    or write.
> +
> +
> +API set
> +-------
> +
> +bool is_dmabuf_sync_supported(void)
> +	- Check if dmabuf sync is supported or not.
> +
> +struct dmabuf_sync *dmabuf_sync_init(void *priv, const char *name)
> +	- Allocate and initialize a new sync object. The caller can get a new
> +	sync object for buffer synchronization. priv is used to set caller's
> +	private data and name is the name of sync object.
> +
> +void dmabuf_sync_fini(struct dmabuf_sync *sync)
> +	- Release all resources to the sync object.
> +
> +int dmabuf_sync_get(struct dmabuf_sync *sync, void *sync_buf,
> +			unsigned int type)
> +	- Add a dmabuf to a sync object. The caller can group multiple dmabufs
> +	by calling this function several times. Internally, this function also
> +	takes a reference to a dmabuf.
> +
> +void dmabuf_sync_put(struct dmabuf_sync *sync, struct dma_buf *dmabuf)
> +	- Remove a given dmabuf from a sync object. Internally, this function
> +	also release every reference to the given dmabuf.
> +
> +void dmabuf_sync_put_all(struct dmabuf_sync *sync)
> +	- Remove all dmabufs added in a sync object. Internally, this function
> +	also release every reference to the dmabufs of the sync object.
> +
> +int dmabuf_sync_lock(struct dmabuf_sync *sync)
> +	- Lock all dmabufs added in a sync object. The caller should call this
> +	function prior to CPU or DMA access to the dmabufs so that others can
> +	not access the dmabufs. Internally, this function avoids dead lock
> +	issue with ww-mutex.
> +
> +int dmabuf_sync_unlock(struct dmabuf_sync *sync)
> +	- Unlock all dmabufs added in a sync object. The caller should call
> +	this function after CPU or DMA access to the dmabufs is completed so
> +	that others can access the dmabufs.
> +
> +
> +Tutorial
> +--------
> +
> +1. Allocate and Initialize a sync object:
> +	struct dmabuf_sync *sync;
> +
> +	sync = dmabuf_sync_init(NULL, "test sync");
> +	...
> +
> +2. Add a dmabuf to the sync object when setting up dma buffer relevant registers:
> +	dmabuf_sync_get(sync, dmabuf, DMA_BUF_ACCESS_READ);
> +	...
> +
> +3. Lock all dmabufs of the sync object before DMA or CPU accesses the dmabufs:
> +	dmabuf_sync_lock(sync);
> +	...
> +
> +4. Now CPU or DMA can access all dmabufs locked in step 3.
> +
> +5. Unlock all dmabufs added in a sync object after DMA or CPU access to these
> +   dmabufs is completed:
> +	dmabuf_sync_unlock(sync);
> +
> +   And call the following functions to release all resources,
> +	dmabuf_sync_put_all(sync);
> +	dmabuf_sync_fini(sync);
> +
> +
> +Cache operation
> +---------------
> +
> +The framework performs cache operation based on the previous and current access
> +types to the dmabufs after the locks to all dmabufs are taken:
> +	Call dma_buf_begin_cpu_access() to invalidate cache if,
> +		previous access type is DMA_BUF_ACCESS_WRITE | DMA and
> +		current access type is DMA_BUF_ACCESS_READ
> +
> +	Call dma_buf_end_cpu_access() to clean cache if,
> +		previous access type is DMA_BUF_ACCESS_WRITE and
> +		current access type is DMA_BUF_ACCESS_READ | DMA
> +
> +Such cache operations are invoked via dma-buf interfaces. Thus, the dma buf
> +exporter should implement dmabuf->ops->begin_cpu_access and end_cpu_access
> +callbacks.
> +
> +
> +References:
> +[1] https://patchwork-mail1.kernel.org/patch/2625321/
> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index 5ccf182..54a1d5a 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -212,6 +212,13 @@ config FENCE_TRACE
>  	  lockup related problems for dma-buffers shared across multiple
>  	  devices.
>  
> +config DMABUF_SYNC
> +	bool "DMABUF Synchronization Framework"
> +	depends on DMA_SHARED_BUFFER
> +	help
> +	  This option enables dmabuf sync framework for buffer synchronization between
> +	  DMA and DMA, CPU and DMA, and CPU and CPU.
> +
>  config CMA
>  	bool "Contiguous Memory Allocator"
>  	depends on HAVE_DMA_CONTIGUOUS && HAVE_MEMBLOCK
> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
> index 8a55cb9..599f6c90 100644
> --- a/drivers/base/Makefile
> +++ b/drivers/base/Makefile
> @@ -11,6 +11,7 @@ obj-y			+= power/
>  obj-$(CONFIG_HAS_DMA)	+= dma-mapping.o
>  obj-$(CONFIG_HAVE_GENERIC_DMA_COHERENT) += dma-coherent.o
>  obj-$(CONFIG_DMA_SHARED_BUFFER) += dma-buf.o fence.o reservation.o
> +obj-$(CONFIG_DMABUF_SYNC) += dmabuf-sync.o
>  obj-$(CONFIG_ISA)	+= isa.o
>  obj-$(CONFIG_FW_LOADER)	+= firmware_class.o
>  obj-$(CONFIG_NUMA)	+= node.o
> diff --git a/drivers/base/dmabuf-sync.c b/drivers/base/dmabuf-sync.c
> new file mode 100644
> index 0000000..c8723a5
> --- /dev/null
> +++ b/drivers/base/dmabuf-sync.c
> @@ -0,0 +1,555 @@
> +/*
> + * Copyright (C) 2013 Samsung Electronics Co.Ltd
> + * Authors:
> + *	Inki Dae <inki.dae@samsung.com>
> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/debugfs.h>
> +#include <linux/uaccess.h>
> +
> +#include <linux/dmabuf-sync.h>
> +
> +#define MAX_SYNC_TIMEOUT	5 /* Second. */
> +
> +#define NEED_BEGIN_CPU_ACCESS(old, new)	\
> +		(old->accessed_type = (DMA_BUF_ACCESS_WRITE | \
> +					DMA_BUF_ACCESS_DMA) && \
> +		 (new->access_type = DMA_BUF_ACCESS_READ))
> +
> +#define NEED_END_CPU_ACCESS(old, new)	\
> +		(old->accessed_type = DMA_BUF_ACCESS_WRITE && \
> +		 ((new->access_type = (DMA_BUF_ACCESS_READ | \
> +					DMA_BUF_ACCESS_DMA)) || \
> +		  (new->access_type = (DMA_BUF_ACCESS_WRITE | \
> +					DMA_BUF_ACCESS_DMA))))
> +
> +int dmabuf_sync_enabled = 1;
> +
> +MODULE_PARM_DESC(enabled, "Check if dmabuf sync is supported or not");
> +module_param_named(enabled, dmabuf_sync_enabled, int, 0444);
> +
> +static void dmabuf_sync_timeout_worker(struct work_struct *work)
> +{
> +	struct dmabuf_sync *sync = container_of(work, struct dmabuf_sync, work);
> +	struct dmabuf_sync_object *sobj;
> +
> +	mutex_lock(&sync->lock);
> +
> +	list_for_each_entry(sobj, &sync->syncs, head) {
> +		if (WARN_ON(!sobj->robj))
> +			continue;
> +
> +		printk(KERN_WARNING "%s: timeout = 0x%x [type = %d, " \
> +					"refcnt = %d, locked = %d]\n",
> +					sync->name, (u32)sobj->dmabuf,
> +					sobj->access_type,
> +					atomic_read(&sobj->robj->shared_cnt),
> +					sobj->robj->locked);
> +
> +		/* unlock only valid sync object. */
> +		if (!sobj->robj->locked)
> +			continue;
> +
> +		if (sobj->robj->shared &&
> +				atomic_read(&sobj->robj->shared_cnt) > 1) {
> +			atomic_dec(&sobj->robj->shared_cnt);

So, in my long standing complaints about atomic_t, this one really takes
the biscuit.  What makes you think that this is somehow safe?

What happens if:

shared_cnt = 2
	CPU0				CPU1
	atomic_read(&shared_cnt)
					atomic_read(&shared_cnt)
	atomic_dec(&shared_cnt)
					atomic_dec(&shared_cnt)

Now, it's zero.  That's not what the above code intends.  You probably
think that because it's called "atomic_*" it has some magical properties
which saves you from stuff like this.  I'm afraid it doesn't.

sync->lock may save you from that, but if that's the case, why use
atomic_t's anyway here because you're already in a protected region.
But maybe not, because I see other uses of shared_cnt without this
lock below.

I think you need to revisit this and think more carefully about how
to deal with this counting.  If you wish to continue using the
atomic_* API, please take time to get familiar with it, and most
importantly realise that virtually any sequence of:

	if some-condition-based-on atomic_read(&something)
		do something with atomic_*(&something)

is a bug.  Maybe take a look at atomic_add_unless() which can be used
with negative values to decrement.