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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Russell,

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux@xxxxxxxxxxxxxxxx]
> Sent: Friday, June 14, 2013 2:26 AM
> To: Inki Dae
> Cc: maarten.lankhorst@xxxxxxxxxxxxx; daniel@xxxxxxxx; robdclark@xxxxxxxxx;
> linux-fbdev@xxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx;
> kyungmin.park@xxxxxxxxxxx; myungjoo.ham@xxxxxxxxxxx; yj44.cho@xxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-media@xxxxxxxxxxxxxxx
> Subject: Re: [RFC PATCH] dmabuf-sync: Introduce buffer synchronization
> framework
> 
> 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=4030bdee9bab5841ad32faade528d04cc0c5fc94
> >
> > 		https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
> exynos.git/
> > 		commit/?h=dmabuf-
> sync&id=6ca548e9ea9e865592719ef6b1cde58366af9f5c
> >
> > 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@xxxxxxxxxxx>
> > ---
> >  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@xxxxxxxxxxx>
> > + *
> > + * 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.

It seems that you say dmabuf_sync_lock_objs function. Right, this function
doesn't use sync->lock. However, ww_mutex_lock would be used instead of it
there. So I think there also is in a protected region.

> 
> 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.

Thanks for your advice. Anyway, it's likely to better to use
atomic_add_unless even though protected.

Thanks,
Inki Dae

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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux