Re: [RFC 02/12] staging/android/sync: add sync_fence_create_dma
On 23/11/2015 13:27, Maarten Lankhorst wrote:
Op 23-11-15 om 12:34 schreef john.c.harri...@intel.com:
From: Maarten Lankhorst
This allows users of dma fences to create a android fence.
v2: Added kerneldoc. (Tvrtko Ursulin).
Signed-off-by: Maarten Lankhorst
Signed-off-by: Tvrtko Ursulin
Cc: Maarten Lankhorst
Cc: Daniel Vetter
Cc: Jesse Barnes
Cc: de...@driverdev.osuosl.org
Cc: Riley Andrews
Cc: Greg Kroah-Hartman
Cc: Arve Hjønnevåg
---
drivers/staging/android/sync.c | 13 +
drivers/staging/android/sync.h | 12 +++-
2 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c
index f83e00c..7f0e919 100644
--- a/drivers/staging/android/sync.c
+++ b/drivers/staging/android/sync.c
@@ -188,7 +188,7 @@ static void fence_check_cb_func(struct fence *f, struct
fence_cb *cb)
}
/* TODO: implement a create which takes more that one sync_pt */
-struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt)
+struct sync_fence *sync_fence_create_dma(const char *name, struct fence *pt)
{
struct sync_fence *fence;
@@ -199,16 +199,21 @@ struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt)
fence->num_fences = 1;
atomic_set(&fence->status, 1);
- fence->cbs[0].sync_pt = &pt->base;
+ fence->cbs[0].sync_pt = pt;
fence->cbs[0].fence = fence;
- if (fence_add_callback(&pt->base, &fence->cbs[0].cb,
- fence_check_cb_func))
+ if (fence_add_callback(pt, &fence->cbs[0].cb, fence_check_cb_func))
atomic_dec(&fence->status);
sync_fence_debug_add(fence);
return fence;
}
+EXPORT_SYMBOL(sync_fence_create_dma);
+
+struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt)
+{
+ return sync_fence_create_dma(name, &pt->base);
+}
EXPORT_SYMBOL(sync_fence_create);
struct sync_fence *sync_fence_fdget(int fd)
diff --git a/drivers/staging/android/sync.h b/drivers/staging/android/sync.h
index 61f8a3a..798cd56 100644
--- a/drivers/staging/android/sync.h
+++ b/drivers/staging/android/sync.h
@@ -250,10 +250,20 @@ void sync_pt_free(struct sync_pt *pt);
* @pt: sync_pt to add to the fence
*
* Creates a fence containg @pt. Once this is called, the fence takes
- * ownership of @pt.
+ * a reference on @pt.
*/
struct sync_fence *sync_fence_create(const char *name, struct sync_pt *pt);
No it doesn't.
+/**
+ * sync_fence_create_dma() - creates a sync fence from dma-fence
+ * @name: name of fence to create
+ * @pt:dma-fence to add to the fence
+ *
+ * Creates a fence containg @pt. Once this is called, the fence takes
+ * a reference on @pt.
+ */
No it doesn't.
This is your patch isn't it? Or is this something Tvrtko added on the
way past? Either way, what should the correct description be? It takes a
copy of the pointer to 'pt'. Is the comment meaning a reference in the
sense of 'pass by reference', i.e. a pointer? But you are meaning a
reference in the sense of incrementing a usage count?
+struct sync_fence *sync_fence_create_dma(const char *name, struct fence *pt);
+
/*
* API for sync_fence consumers
*/
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC 26/29] dma-buf/fence: remove pointless fence_timeline_signal at destroy phase
On 15/01/2016 14:55, Gustavo Padovan wrote:
From: Gustavo Padovan
All changes to timeline value come through the user via
fence_timeline_signal() calls. When fence_timeline_destroy() is called no
changes on timeline->value happens hence call fence_timeline_signal() with
no increment is pointless.
Signed-off-by: Gustavo Padovan
---
drivers/dma-buf/fence.c | 6 +-
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/dma-buf/fence.c b/drivers/dma-buf/fence.c
index 7a5fc9b..26f5f0f 100644
--- a/drivers/dma-buf/fence.c
+++ b/drivers/dma-buf/fence.c
@@ -136,7 +136,7 @@ EXPORT_SYMBOL(fence_timeline_put);
* fence_timeline_destroy - destroy a fence_timeline
* @timeline [in]the fence_timeline to destroy
*
- * This function destroys a timeline. It signals any active fence first.
+ * This function destroys a timeline.
The implementation for this was certainly broken but I would say it
should be fixed to match the comment rather than just abandoned
completely. That is, what happens if a timeline owner destroys their
timeline while there are outstanding fences which other drivers are
waiting on? That is presumably a bug in the code that called destroy
prematurely, but bugs happen.
The old implementation simply leaked the fences. Doing a debugfs dump
would show the timeline with all its outstanding fences still floating
around forever after. Worse, anything waiting on them would never be
signalled and is therefore potentially deadlocked.
Note that I haven't had chance to look through the entire patch series
yet so maybe this has been fixed up elsewhere. If not, then I think it
definitely needs looking into.
*/
void fence_timeline_destroy(struct fence_timeline *timeline)
{
@@ -147,10 +147,6 @@ void fence_timeline_destroy(struct fence_timeline
*timeline)
*/
smp_wmb();
- /*
-* signal any children that their parent is going away.
-*/
- fence_timeline_signal(timeline, 0);
fence_timeline_put(timeline);
}
EXPORT_SYMBOL(fence_timeline_destroy);
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC 00/29] De-stage android's sync framework
On 19/01/2016 15:23, Gustavo Padovan wrote:
Hi Daniel,
2016-01-19 Daniel Vetter :
On Fri, Jan 15, 2016 at 12:55:10PM -0200, Gustavo Padovan wrote:
From: Gustavo Padovan
This patch series de-stage the sync framework, and in order to accomplish that
a bunch of cleanups/improvements on the sync and fence were made.
The sync framework contained some abstractions around struct fence and those
were removed in the de-staging process among other changes:
Userspace visible changes
-
* The sw_sync file was moved from /dev/sw_sync to /sync/sw_sync. No
other change.
Kernel API changes
--
* struct sync_timeline is now struct fence_timeline
* sync_timeline_ops is now fence_timeline_ops and they now carry struct
fence as parameter instead of struct sync_pt
* a .cleanup() fence op was added to allow sync_fence to run a cleanup when
the fence_timeline is destroyed
* added fence_add_used_data() to pass a private point to struct fence. This
pointer is sent back on the .cleanup op.
* The sync timeline function were moved to be fence_timeline functions:
- sync_timeline_create() -> fence_timeline_create()
- sync_timeline_get() -> fence_timeline_get()
- sync_timeline_put() -> fence_timeline_put()
- sync_timeline_destroy() -> fence_timeline_destroy()
- sync_timeline_signal() -> fence_timeline_signal()
* sync_pt_create() was replaced be fence_create_on_timeline()
Internal changes
* fence_timeline_ops was removed in favor of direct use fence_ops
* fence default functions were created for fence_ops
* removed structs sync_pt, sw_sync_timeline and sw_sync_pt
Bunch of fairly random comments all over:
- include/uapi/linux/sw_sync.h imo should be dropped, it's just a private
debugfs interface between fence fds and the testsuite. Since the plan is
to have the testcases integrated into the kernel tree too we don't need
a public header.
- similar for include/linux/sw_sync.h Imo that should all be moved into
sync_debug.c. Same for sw_sync.c, that should all land in sync_debug
imo, and made optional with a Kconfig option. At least we should reuse
CONFIG_DEBUGFS.
These two items sounds reasonable to me.
I have just posted our in-progress IGT for testing i915 syncs (with a CC
of Gustavo). It uses the sw_sync mechanisms. Can you take a quick look
and see if it is the kind of thing you would expect us to be doing? Or
is it using interfaces that you are planning to remove and/or make
kernel only?
I'm not sure having a kernel only test is the best way to go. Having
user land tests like IGT would be much more versatile.
- fence_context and fence_timeline are really the same. timeline has some
super-basic support for doing sw-only fence timelines, but imo that's
not really worth keeping (and if so better to keep seperate in a
sw-fence.c or similar, like seqno-fence.c). The other main thing
timeline provides is support to clean up fences on a timeline. And imo
that cleanup should be done by the core fence support, not by the add-on
stuff.
Yes, they are. But I currently doesn't know how to merge them best, so I
decided to go for a RFC instead of trying some crazy solution touching
all fence_context users.
Interlude about fence cleanup on driver unload:
Working drivers imo should never call timeline_destroy when there's still
an unsignalled fence around for that timeline/context. That just means
they're broken and failed to clean up all the pending work. So the problem
really is only what to do with fences where the driver disappeared, and
for that we essentially need a fence_revoke() function (which could be
called internally from timeline_free). So here's what I think
timeline_free should do:
for_each_fence_on_timel() {
WARN_ON(!fence_is_signalled());
fence_revoke(fence);
}
Implementing fence_revoke is a bit tricky since we need to make sure the
memory contained ->ops and similar stuff doesn't disappear. Simplest
option might be to grab a temporary reference (using
kref_get_unless_zero), and then exchange ->ops with one that has only a
release function. We don't need anything else as long as all fence_*
functions the kernel might call check for signalling correctly first
(fence_wait is broken at least).
Or we just give up (for now) and declare module unload as slightly racy.
dma-buf is similar. An intermediate option might be to at least add a
THIS_MODULE reference to each fence (but that's a bit expensive ...).
I'd say we just give up for now as we don't have any driver using
timeline_destroy for now. So we could go for other improvements first.
- back to timeline vs. context: I have no idea how to best clean up this
mess, but least painful option long-term is probably to switch over all
current users of fence_context_alloc to timelines and remove the plain
context interface.
Agreed.
