On Thu, Apr 14, 2016 at 06:29:34PM -0700, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
>
> struct fence_collection inherits from struct fence and carries a
> collection of fences that needs to be waited together.
>
> It is useful to translate a sync_file to a fence to remove the complexity
> of dealing with sync_files from DRM drivers. So even if there are many
> fences in the sync_file that needs to waited for a commit to happen
> drivers would only worry about a standard struct fence.That means that no
> changes needed to any driver besides supporting fences.
>
> fence_collection's fence doesn't belong to any timeline context.
>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
> ---
> drivers/dma-buf/Makefile | 2 +-
> drivers/dma-buf/fence-collection.c | 138 +++++++++++++++++++++++++++++++++++++
> drivers/dma-buf/fence.c | 2 +-
> include/linux/fence-collection.h | 56 +++++++++++++++
> include/linux/fence.h | 2 +
> 5 files changed, 198 insertions(+), 2 deletions(-)
> create mode 100644 drivers/dma-buf/fence-collection.c
> create mode 100644 include/linux/fence-collection.h
>
> diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile
> index 43325a1..30b8464 100644
> --- a/drivers/dma-buf/Makefile
> +++ b/drivers/dma-buf/Makefile
> @@ -1,3 +1,3 @@
> -obj-y := dma-buf.o fence.o reservation.o seqno-fence.o
> +obj-y := dma-buf.o fence.o reservation.o seqno-fence.o fence-collection.o
> obj-$(CONFIG_SYNC_FILE) += sync_file.o sync_timeline.o sync_debug.o
> obj-$(CONFIG_SW_SYNC) += sw_sync.o
> diff --git a/drivers/dma-buf/fence-collection.c b/drivers/dma-buf/fence-collection.c
> new file mode 100644
> index 0000000..8a4ecb0
> --- /dev/null
> +++ b/drivers/dma-buf/fence-collection.c
> @@ -0,0 +1,138 @@
> +/*
> + * fence-collection: aggregate fences to be waited together
> + *
> + * Copyright (C) 2016 Collabora Ltd
> + * Authors:
> + * Gustavo Padovan <gustavo@xxxxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + */
> +
> +#include <linux/export.h>
> +#include <linux/slab.h>
> +#include <linux/fence-collection.h>
> +
> +static const char *fence_collection_get_driver_name(struct fence *fence)
> +{
> + struct fence_collection *collection = to_fence_collection(fence);
> + struct fence *f = collection->fences[0].fence;
> +
> + return f->ops->get_driver_name(fence);
> +}
> +
> +static const char *fence_collection_get_timeline_name(struct fence *fence)
> +{
> + return "no context";
> +}
> +
> +static bool fence_collection_enable_signaling(struct fence *fence)
> +{
> + struct fence_collection *collection = to_fence_collection(fence);
> +
> + return atomic_read(&collection->num_pending_fences);
> +}
> +
> +static bool fence_collection_signaled(struct fence *fence)
> +{
> + struct fence_collection *collection = to_fence_collection(fence);
> +
> + return (atomic_read(&collection->num_pending_fences) == 0);
> +}
> +
> +static void fence_collection_release(struct fence *fence)
> +{
> + struct fence_collection *collection = to_fence_collection(fence);
> + int i;
> +
> + for (i = 0 ; i < collection->num_fences ; i++) {
> + fence_remove_callback(collection->fences[i].fence,
> + &collection->fences[i].cb);
> + fence_put(collection->fences[i].fence);
> + }
> +
> + fence_free(fence);
> +}
> +
> +static signed long fence_collection_wait(struct fence *fence, bool intr,
> + signed long timeout)
> +{
> + struct fence_collection *collection = to_fence_collection(fence);
> + int i;
> +
> + for (i = 0 ; i < collection->num_fences ; i++) {
> + timeout = fence_wait(collection->fences[i].fence, intr);
> + if (timeout < 0)
> + return timeout;
> + }
> +
> + return timeout;
> +}
> +
> +static const struct fence_ops fence_collection_ops = {
> + .get_driver_name = fence_collection_get_driver_name,
> + .get_timeline_name = fence_collection_get_timeline_name,
> + .enable_signaling = fence_collection_enable_signaling,
> + .signaled = fence_collection_signaled,
> + .wait = fence_collection_wait,
> + .release = fence_collection_release,
> +};
> +
> +static void collection_check_cb_func(struct fence *fence, struct fence_cb *cb)
> +{
> + struct fence_collection_cb *f_cb;
> + struct fence_collection *collection;
> +
> + f_cb = container_of(cb, struct fence_collection_cb, cb);
> + collection = f_cb->collection;
> +
> + if (atomic_dec_and_test(&collection->num_pending_fences))
> + fence_signal(&collection->base);
> +}
> +
> +void fence_collection_add(struct fence_collection *collection,
> + struct fence *fence)
> +{
> + int n = collection->num_fences;
> +
> + collection->fences[n].collection = collection;
> + collection->fences[n].fence = fence;
> +
> + if (fence_add_callback(fence, &collection->fences[n].cb,
> + collection_check_cb_func))
> + return;
> +
> + fence_get(fence);
> +
> + collection->num_fences++;
> + atomic_inc(&collection->num_pending_fences);
> +}
For the interface I think we should not split it into _init and _add - it
shouldn't be allowed to change a collection once it's created. So probably
cleaner if we add an array of fence pointers to _init.
Other nitpick: Adding the callback should (I think) only be done in
->enable_signalling.
Finally: Have you looked into stitching together a few unit tests for
fence_collection?
Fence collections also break the assumption that every fence is on a
timeline. fence_later and fence_is_later need to be adjusted. We also need
a special collection context to filter these out. This means
fence_collection isn't perfectly opaque abstraction.
> +
> +struct fence_collection *fence_collection_init(int num_fences)
> +{
> + struct fence_collection *collection;
> +
> + collection = kzalloc(offsetof(struct fence_collection,
> + fences[num_fences]), GFP_KERNEL);
> + if (!collection)
> + return NULL;
> +
> + spin_lock_init(&collection->lock);
> + fence_init(&collection->base, &fence_collection_ops, &collection->lock,
> + FENCE_NO_CONTEXT, 0);
> +
> + return collection;
> +}
> +EXPORT_SYMBOL(fence_collection_init);
> +
> +void fence_collection_put(struct fence_collection *collection)
> +{
> + fence_put(&collection->base);
Not sure a specialized _put function is useful, I'd leave it out.
> +}
> +EXPORT_SYMBOL(fence_collection_put);
> diff --git a/drivers/dma-buf/fence.c b/drivers/dma-buf/fence.c
> index 7b05dbe..486e95c 100644
> --- a/drivers/dma-buf/fence.c
> +++ b/drivers/dma-buf/fence.c
> @@ -35,7 +35,7 @@ EXPORT_TRACEPOINT_SYMBOL(fence_emit);
> * context or not. One device can have multiple separate contexts,
> * and they're used if some engine can run independently of another.
> */
> -static atomic_t fence_context_counter = ATOMIC_INIT(0);
> +static atomic_t fence_context_counter = ATOMIC_INIT(1);
>
> /**
> * fence_context_alloc - allocate an array of fence contexts
> diff --git a/include/linux/fence-collection.h b/include/linux/fence-collection.h
> new file mode 100644
> index 0000000..a798925
> --- /dev/null
> +++ b/include/linux/fence-collection.h
> @@ -0,0 +1,56 @@
> +/*
> + * fence-collection: aggregates fence to be waited together
> + *
> + * Copyright (C) 2016 Collabora Ltd
> + * Authors:
> + * Gustavo Padovan <gustavo@xxxxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + */
> +
> +#ifndef __LINUX_FENCE_COLLECTION_H
> +#define __LINUX_FENCE_COLLECTION_H
> +
> +#include <linux/fence.h>
> +
> +struct fence_collection_cb {
> + struct fence_cb cb;
> + struct fence *fence;
> + struct fence_collection *collection;
> +};
> +
> +struct fence_collection {
> + struct fence base;
> +
> + spinlock_t lock;
> + struct fence_cb fence_cb;
> + atomic_t num_pending_fences;
> + int num_fences;
> + struct fence_collection_cb fences[];
> +};
> +
> +/**
> + * to_fence_collection - cast a fence to a fence_collection
> + * @fence: fence to cast to a fence_collection
> + *
> + * Returns NULL if the fence is not a fence_collection,
> + * or the fence_collection otherwise.
> + */
> +static inline struct fence_collection * to_fence_collection(struct fence *fence)
> +{
Kerneldoc claims it, but you don't check that the fence is indeed a
fence_collection. That's usually done by comparing the ops pointer.
> + return container_of(fence, struct fence_collection, base);
> +}
> +
> +struct fence_collection *fence_collection_init(int num_fences);
> +void fence_collection_add(struct fence_collection *collection,
> + struct fence *fence);
> +void fence_collection_put(struct fence_collection *collection);
> +
> +#endif /* __LINUX_FENCE_COLLECTION_H */
> diff --git a/include/linux/fence.h b/include/linux/fence.h
> index 2b17698..02170dd 100644
> --- a/include/linux/fence.h
> +++ b/include/linux/fence.h
> @@ -30,6 +30,8 @@
> #include <linux/printk.h>
> #include <linux/rcupdate.h>
>
> +#define FENCE_NO_CONTEXT 0
> +
> struct fence;
> struct fence_ops;
> struct fence_cb;
> --
> 2.5.5
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
