Re: [PATCH v7 01/15] gpu: host1x: Add DMA fence implementation

2021-06-15 Thread Dmitry Osipenko
..
> diff --git a/drivers/gpu/host1x/fence.c b/drivers/gpu/host1x/fence.c
> new file mode 100644
> index ..2b0bb97f053f
> --- /dev/null
> +++ b/drivers/gpu/host1x/fence.c
> @@ -0,0 +1,184 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Syncpoint dma_fence implementation
> + *
> + * Copyright (c) 2020, NVIDIA Corporation.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

Stale headers

> +#include "fence.h"
> +#include "intr.h"
> +#include "syncpt.h"
> +
> +DEFINE_SPINLOCK(lock);

static

...
> +static bool host1x_syncpt_fence_enable_signaling(struct dma_fence *f)
> +{
> + struct host1x_syncpt_fence *sf = to_host1x_fence(f);
> + int err;
> +
> + if (host1x_syncpt_is_expired(sf->sp, sf->threshold))
> + return false;
> +
> + dma_fence_get(f);
> +
> + /*
> +  * The dma_fence framework requires the fence driver to keep a
> +  * reference to any fences for which 'enable_signaling' has been
> +  * called (and that have not been signalled).
> +  * 
> +  * We provide a userspace API to create arbitrary syncpoint fences,
> +  * so we cannot normally guarantee that all fences get signalled.
> +  * As such, setup a timeout, so that long-lasting fences will get
> +  * reaped eventually.
> +  */
> + schedule_delayed_work(>timeout_work, msecs_to_jiffies(3));

I don't see this API. Please always remove all dead code, make patches
minimal and functional.

...> +int host1x_fence_extract(struct dma_fence *fence, u32 *id, u32
*threshold)
> +{
> + struct host1x_syncpt_fence *f;
> +
> + if (fence->ops != _syncpt_fence_ops)
> + return -EINVAL;
> +
> + f = container_of(fence, struct host1x_syncpt_fence, base);
> +
> + *id = f->sp->id;
> + *threshold = f->threshold;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(host1x_fence_extract);

dead code


[PATCH v7 01/15] gpu: host1x: Add DMA fence implementation

2021-06-10 Thread Mikko Perttunen
Add an implementation of dma_fences based on syncpoints. Syncpoint
interrupts are used to signal fences. Additionally, after
software signaling has been enabled, a 30 second timeout is started.
If the syncpoint threshold is not reached within this period,
the fence is signalled with an -ETIMEDOUT error code. This is to
allow fences that would never reach their syncpoint threshold to
be cleaned up. The timeout can potentially be removed in the future
after job tracking code has been refactored.

Signed-off-by: Mikko Perttunen 
---
v7:
* Remove unused fence_create_fd function
v6:
* Removed userspace interface.
* Add host1x_ prefixes in various places.
v5:
* Update for change in put_ref prototype.
v4:
* Fix _signal prototype and include it to avoid warning
* Remove use of unused local in error path
v3:
* Move declaration of host1x_fence_extract to public header
---
 drivers/gpu/host1x/Makefile |   1 +
 drivers/gpu/host1x/fence.c  | 184 
 drivers/gpu/host1x/fence.h  |  13 +++
 drivers/gpu/host1x/intr.c   |   9 ++
 drivers/gpu/host1x/intr.h   |   2 +
 include/linux/host1x.h  |   3 +
 6 files changed, 212 insertions(+)
 create mode 100644 drivers/gpu/host1x/fence.c
 create mode 100644 drivers/gpu/host1x/fence.h

diff --git a/drivers/gpu/host1x/Makefile b/drivers/gpu/host1x/Makefile
index 096017b8789d..d2b6f7de0498 100644
--- a/drivers/gpu/host1x/Makefile
+++ b/drivers/gpu/host1x/Makefile
@@ -9,6 +9,7 @@ host1x-y = \
job.o \
debug.o \
mipi.o \
+   fence.o \
hw/host1x01.o \
hw/host1x02.o \
hw/host1x04.o \
diff --git a/drivers/gpu/host1x/fence.c b/drivers/gpu/host1x/fence.c
new file mode 100644
index ..2b0bb97f053f
--- /dev/null
+++ b/drivers/gpu/host1x/fence.c
@@ -0,0 +1,184 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Syncpoint dma_fence implementation
+ *
+ * Copyright (c) 2020, NVIDIA Corporation.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "fence.h"
+#include "intr.h"
+#include "syncpt.h"
+
+DEFINE_SPINLOCK(lock);
+
+struct host1x_syncpt_fence {
+   struct dma_fence base;
+
+   atomic_t signaling;
+
+   struct host1x_syncpt *sp;
+   u32 threshold;
+
+   struct host1x_waitlist *waiter;
+   void *waiter_ref;
+
+   struct delayed_work timeout_work;
+};
+
+static const char *host1x_syncpt_fence_get_driver_name(struct dma_fence *f)
+{
+   return "host1x";
+}
+
+static const char *host1x_syncpt_fence_get_timeline_name(struct dma_fence *f)
+{
+   return "syncpoint";
+}
+
+static struct host1x_syncpt_fence *to_host1x_fence(struct dma_fence *f)
+{
+   return container_of(f, struct host1x_syncpt_fence, base);
+}
+
+static bool host1x_syncpt_fence_enable_signaling(struct dma_fence *f)
+{
+   struct host1x_syncpt_fence *sf = to_host1x_fence(f);
+   int err;
+
+   if (host1x_syncpt_is_expired(sf->sp, sf->threshold))
+   return false;
+
+   dma_fence_get(f);
+
+   /*
+* The dma_fence framework requires the fence driver to keep a
+* reference to any fences for which 'enable_signaling' has been
+* called (and that have not been signalled).
+* 
+* We provide a userspace API to create arbitrary syncpoint fences,
+* so we cannot normally guarantee that all fences get signalled.
+* As such, setup a timeout, so that long-lasting fences will get
+* reaped eventually.
+*/
+   schedule_delayed_work(>timeout_work, msecs_to_jiffies(3));
+
+   err = host1x_intr_add_action(sf->sp->host, sf->sp, sf->threshold,
+HOST1X_INTR_ACTION_SIGNAL_FENCE, f,
+sf->waiter, >waiter_ref);
+   if (err) {
+   cancel_delayed_work_sync(>timeout_work);
+   dma_fence_put(f);
+   return false;
+   }
+
+   /* intr framework takes ownership of waiter */
+   sf->waiter = NULL;
+
+   /*
+* The fence may get signalled at any time after the above call,
+* so we need to initialize all state used by signalling
+* before it.
+*/
+
+   return true;
+}
+
+static void host1x_syncpt_fence_release(struct dma_fence *f)
+{
+   struct host1x_syncpt_fence *sf = to_host1x_fence(f);
+
+   if (sf->waiter)
+   kfree(sf->waiter);
+
+   dma_fence_free(f);
+}
+
+const struct dma_fence_ops host1x_syncpt_fence_ops = {
+   .get_driver_name = host1x_syncpt_fence_get_driver_name,
+   .get_timeline_name = host1x_syncpt_fence_get_timeline_name,
+   .enable_signaling = host1x_syncpt_fence_enable_signaling,
+   .release = host1x_syncpt_fence_release,
+};
+
+void host1x_fence_signal(struct host1x_syncpt_fence *f)
+{
+   if (atomic_xchg(>signaling, 1))
+   return;
+
+   /*
+* Cancel pending timeout work - if it races, it will
+* not get 'f->signaling' and return.
+