[PATCH 1/3] drm/etnaviv: submit support for in-fences

[PATCH 1/3] drm/etnaviv: submit support for in-fences

[Philipp Zabel]

Loosely based on commit f0a42bb5423a ("drm/msm: submit support for
in-fences"). Unfortunately, struct drm_etnaviv_gem_submit doesn't have
a flags field yet, so we have to extend the structure and trust that
drm_ioctl will clear the flags for us if an older userspace only submits
part of the struct.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/Kconfig              |  1 +
 drivers/gpu/drm/etnaviv/etnaviv_gem.h        |  1 +
 drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 34 +++++++++++++++++++++++++++-
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c        |  5 +++-
 drivers/gpu/drm/etnaviv/etnaviv_gpu.h        |  2 +-
 include/uapi/drm/etnaviv_drm.h               |  6 +++++
 6 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/Kconfig b/drivers/gpu/drm/etnaviv/Kconfig
index cc1731c5289c2..71cee4e9fefbb 100644
--- a/drivers/gpu/drm/etnaviv/Kconfig
+++ b/drivers/gpu/drm/etnaviv/Kconfig
@@ -5,6 +5,7 @@ config DRM_ETNAVIV
 	depends on ARCH_MXC || ARCH_DOVE || (ARM && COMPILE_TEST)
 	depends on MMU
 	select SHMEM
+	select SYNC_FILE
 	select TMPFS
 	select IOMMU_API
 	select IOMMU_SUPPORT
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
index e63ff116a3b3d..120410d67eb5b 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
@@ -107,6 +107,7 @@ struct etnaviv_gem_submit {
 	u32 fence;
 	unsigned int nr_bos;
 	struct etnaviv_gem_submit_bo bos[0];
+	u32 flags;
 };
 
 int etnaviv_gem_wait_bo(struct etnaviv_gpu *gpu, struct drm_gem_object *obj,
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
index 726090d7a6ace..e67d83eac22dc 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
@@ -15,6 +15,7 @@
  */
 
 #include <linux/reservation.h>
+#include <linux/sync_file.h>
 #include "etnaviv_cmdbuf.h"
 #include "etnaviv_drv.h"
 #include "etnaviv_gpu.h"
@@ -169,8 +170,10 @@ static int submit_fence_sync(const struct etnaviv_gem_submit *submit)
 	for (i = 0; i < submit->nr_bos; i++) {
 		struct etnaviv_gem_object *etnaviv_obj = submit->bos[i].obj;
 		bool write = submit->bos[i].flags & ETNA_SUBMIT_BO_WRITE;
+		bool explicit = !(submit->flags & ETNA_SUBMIT_NO_IMPLICIT);
 
-		ret = etnaviv_gpu_fence_sync_obj(etnaviv_obj, context, write);
+		ret = etnaviv_gpu_fence_sync_obj(etnaviv_obj, context, write,
+						 explicit);
 		if (ret)
 			break;
 	}
@@ -303,6 +306,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 	struct etnaviv_gem_submit *submit;
 	struct etnaviv_cmdbuf *cmdbuf;
 	struct etnaviv_gpu *gpu;
+	struct dma_fence *in_fence = NULL;
 	void *stream;
 	int ret;
 
@@ -326,6 +330,11 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 		return -EINVAL;
 	}
 
+	if (args->flags & ~ETNA_SUBMIT_FLAGS) {
+		DRM_ERROR("invalid flags: 0x%x\n", args->flags);
+		return -EINVAL;
+	}
+
 	/*
 	 * Copy the command submission and bo array to kernel space in
 	 * one go, and do this outside of any locks.
@@ -371,6 +380,8 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 		goto err_submit_cmds;
 	}
 
+	submit->flags = args->flags;
+
 	ret = submit_lookup_objects(submit, file, bos, args->nr_bos);
 	if (ret)
 		goto err_submit_objects;
@@ -385,6 +396,25 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 		goto err_submit_objects;
 	}
 
+	if (args->flags & ETNA_SUBMIT_FENCE_FD_IN) {
+		in_fence = sync_file_get_fence(args->fence_fd);
+		if (!in_fence) {
+			ret = -EINVAL;
+			goto err_submit_objects;
+		}
+
+		/* TODO if we get an array-fence due to userspace merging
+		 * multiple fences, we need a way to determine if all the
+		 * backing fences are from our own context..
+		 */
+
+		if (in_fence->context != gpu->fence_context) {
+			ret = dma_fence_wait(in_fence, true);
+			if (ret)
+				goto err_submit_objects;
+		}
+	}
+
 	ret = submit_fence_sync(submit);
 	if (ret)
 		goto err_submit_objects;
@@ -419,6 +449,8 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 		flush_workqueue(priv->wq);
 
 err_submit_objects:
+	if (in_fence)
+		dma_fence_put(in_fence);
 	submit_cleanup(submit);
 
 err_submit_cmds:
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index 130d7d517a19a..51d52c72aef17 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -1064,7 +1064,7 @@ static struct dma_fence *etnaviv_gpu_fence_alloc(struct etnaviv_gpu *gpu)
 }
 
 int etnaviv_gpu_fence_sync_obj(struct etnaviv_gem_object *etnaviv_obj,
-	unsigned int context, bool exclusive)
+	unsigned int context, bool exclusive, bool explicit)
 {
 	struct reservation_object *robj = etnaviv_obj->resv;
 	struct reservation_object_list *fobj;
@@ -1077,6 +1077,9 @@ int etnaviv_gpu_fence_sync_obj(struct etnaviv_gem_object *etnaviv_obj,
 			return ret;
 	}
 
+	if (explicit)
+		return 0;
+
 	/*
 	 * If we have any shared fences, then the exclusive fence
 	 * should be ignored as it will already have been signalled.
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
index 1c0606ea7d5e8..dc27c7a039060 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
@@ -181,7 +181,7 @@ int etnaviv_gpu_debugfs(struct etnaviv_gpu *gpu, struct seq_file *m);
 #endif
 
 int etnaviv_gpu_fence_sync_obj(struct etnaviv_gem_object *etnaviv_obj,
-	unsigned int context, bool exclusive);
+	unsigned int context, bool exclusive, bool implicit);
 
 void etnaviv_gpu_retire(struct etnaviv_gpu *gpu);
 int etnaviv_gpu_wait_fence_interruptible(struct etnaviv_gpu *gpu,
diff --git a/include/uapi/drm/etnaviv_drm.h b/include/uapi/drm/etnaviv_drm.h
index 2584c1cca42f6..e9c388a1d8ebe 100644
--- a/include/uapi/drm/etnaviv_drm.h
+++ b/include/uapi/drm/etnaviv_drm.h
@@ -154,6 +154,10 @@ struct drm_etnaviv_gem_submit_bo {
  * one or more cmdstream buffers.  This allows for conditional execution
  * (context-restore), and IB buffers needed for per tile/bin draw cmds.
  */
+#define ETNA_SUBMIT_NO_IMPLICIT         0x0001
+#define ETNA_SUBMIT_FENCE_FD_IN         0x0002
+#define ETNA_SUBMIT_FLAGS		(ETNA_SUBMIT_NO_IMPLICIT | \
+					 ETNA_SUBMIT_FENCE_FD_IN)
 #define ETNA_PIPE_3D      0x00
 #define ETNA_PIPE_2D      0x01
 #define ETNA_PIPE_VG      0x02
@@ -167,6 +171,8 @@ struct drm_etnaviv_gem_submit {
 	__u64 bos;            /* in, ptr to array of submit_bo's */
 	__u64 relocs;         /* in, ptr to array of submit_reloc's */
 	__u64 stream;         /* in, ptr to cmdstream */
+	__u32 flags;          /* in, mask of ETNA_SUBMIT_x */
+	__s32 fence_fd;       /* in, fence fd (see ETNA_SUBMIT_FENCE_FD_IN) */
 };
 
 /* The normal way to synchronize with the GPU is just to CPU_PREP on
-- 
2.11.0

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

[PATCH 2/3] drm/etnaviv: move fence allocation out of etnaviv_gpu_submit()

[Philipp Zabel]

The next patch will need the dma_fence to create the sync_file in
etnaviv_ioctl_gem_submit, in case an out_fence_fd is requested.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem.h        |  3 ++-
 drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c |  8 +++++++-
 drivers/gpu/drm/etnaviv/etnaviv_gpu.c        | 21 ++++++---------------
 drivers/gpu/drm/etnaviv/etnaviv_gpu.h        |  1 +
 4 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
index 120410d67eb5b..c4a091e874269 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
@@ -20,6 +20,7 @@
 #include <linux/reservation.h>
 #include "etnaviv_drv.h"
 
+struct dma_fence;
 struct etnaviv_gem_ops;
 struct etnaviv_gem_object;
 
@@ -104,7 +105,7 @@ struct etnaviv_gem_submit {
 	struct drm_device *dev;
 	struct etnaviv_gpu *gpu;
 	struct ww_acquire_ctx ticket;
-	u32 fence;
+	struct dma_fence *fence;
 	unsigned int nr_bos;
 	struct etnaviv_gem_submit_bo bos[0];
 	u32 flags;
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
index e67d83eac22dc..022fcc7d57f48 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
@@ -431,11 +431,17 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 	memcpy(cmdbuf->vaddr, stream, args->stream_size);
 	cmdbuf->user_size = ALIGN(args->stream_size, 8);
 
+	submit->fence = etnaviv_gpu_fence_alloc(gpu);
+	if (!submit->fence) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
 	ret = etnaviv_gpu_submit(gpu, submit, cmdbuf);
 	if (ret == 0)
 		cmdbuf = NULL;
 
-	args->fence = submit->fence;
+	args->fence = submit->fence->seqno;
 
 out:
 	submit_unpin_objects(submit);
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index 51d52c72aef17..a439700cc577f 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -1047,7 +1047,7 @@ static const struct dma_fence_ops etnaviv_fence_ops = {
 	.release = etnaviv_fence_release,
 };
 
-static struct dma_fence *etnaviv_gpu_fence_alloc(struct etnaviv_gpu *gpu)
+struct dma_fence *etnaviv_gpu_fence_alloc(struct etnaviv_gpu *gpu)
 {
 	struct etnaviv_fence *f;
 
@@ -1290,7 +1290,6 @@ void etnaviv_gpu_pm_put(struct etnaviv_gpu *gpu)
 int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
 	struct etnaviv_gem_submit *submit, struct etnaviv_cmdbuf *cmdbuf)
 {
-	struct dma_fence *fence;
 	unsigned int event, i;
 	int ret;
 
@@ -1314,18 +1313,10 @@ int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
 		goto out_pm_put;
 	}
 
-	fence = etnaviv_gpu_fence_alloc(gpu);
-	if (!fence) {
-		event_free(gpu, event);
-		ret = -ENOMEM;
-		goto out_pm_put;
-	}
-
 	mutex_lock(&gpu->lock);
 
-	gpu->event[event].fence = fence;
-	submit->fence = fence->seqno;
-	gpu->active_fence = submit->fence;
+	gpu->event[event].fence = submit->fence;
+	gpu->active_fence = submit->fence->seqno;
 
 	if (gpu->lastctx != cmdbuf->ctx) {
 		gpu->mmu->need_flush = true;
@@ -1335,7 +1326,7 @@ int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
 
 	etnaviv_buffer_queue(gpu, event, cmdbuf);
 
-	cmdbuf->fence = fence;
+	cmdbuf->fence = submit->fence;
 	list_add_tail(&cmdbuf->node, &gpu->active_cmd_list);
 
 	/* We're committed to adding this command buffer, hold a PM reference */
@@ -1351,10 +1342,10 @@ int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
 
 		if (submit->bos[i].flags & ETNA_SUBMIT_BO_WRITE)
 			reservation_object_add_excl_fence(etnaviv_obj->resv,
-							  fence);
+							  submit->fence);
 		else
 			reservation_object_add_shared_fence(etnaviv_obj->resv,
-							    fence);
+							    submit->fence);
 	}
 	cmdbuf->nr_bos = submit->nr_bos;
 	hangcheck_timer_reset(gpu);
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
index dc27c7a039060..20f7191018daf 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
@@ -180,6 +180,7 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu);
 int etnaviv_gpu_debugfs(struct etnaviv_gpu *gpu, struct seq_file *m);
 #endif
 
+struct dma_fence *etnaviv_gpu_fence_alloc(struct etnaviv_gpu *gpu);
 int etnaviv_gpu_fence_sync_obj(struct etnaviv_gem_object *etnaviv_obj,
 	unsigned int context, bool exclusive, bool implicit);
 
-- 
2.11.0

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

[PATCH 3/3] drm/etnaviv: submit support for out-fences

[Philipp Zabel]

Based on commit 4cd0945901a6 ("drm/msm: submit support for out-fences").
We increment the minor driver version so userspace can detect explicit
fence support.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_drv.c        |  2 +-
 drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 27 +++++++++++++++++++++++++++
 include/uapi/drm/etnaviv_drm.h               |  6 ++++--
 3 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index 587e45043542b..00f7e9acf68ad 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -512,7 +512,7 @@ static struct drm_driver etnaviv_drm_driver = {
 	.desc               = "etnaviv DRM",
 	.date               = "20151214",
 	.major              = 1,
-	.minor              = 0,
+	.minor              = 1,
 };
 
 /*
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
index 022fcc7d57f48..7d4dc946104b9 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
@@ -307,6 +307,8 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 	struct etnaviv_cmdbuf *cmdbuf;
 	struct etnaviv_gpu *gpu;
 	struct dma_fence *in_fence = NULL;
+	struct sync_file *sync_file = NULL;
+	int out_fence_fd = -1;
 	void *stream;
 	int ret;
 
@@ -374,6 +376,14 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 		goto err_submit_cmds;
 	}
 
+	if (args->flags & ETNA_SUBMIT_FENCE_FD_OUT) {
+		out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
+		if (out_fence_fd < 0) {
+			ret = out_fence_fd;
+			goto err_submit_cmds;
+		}
+	}
+
 	submit = submit_create(dev, gpu, args->nr_bos);
 	if (!submit) {
 		ret = -ENOMEM;
@@ -437,10 +447,25 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 		goto out;
 	}
 
+	if (args->flags & ETNA_SUBMIT_FENCE_FD_OUT) {
+		sync_file = sync_file_create(submit->fence);
+		if (!sync_file) {
+			dma_fence_put(submit->fence);
+			submit->fence = NULL;
+			ret = -ENOMEM;
+			goto out;
+		}
+	}
+
 	ret = etnaviv_gpu_submit(gpu, submit, cmdbuf);
 	if (ret == 0)
 		cmdbuf = NULL;
 
+	if (args->flags & ETNA_SUBMIT_FENCE_FD_OUT) {
+		fd_install(out_fence_fd, sync_file->file);
+	}
+
+	args->fence_fd = out_fence_fd;
 	args->fence = submit->fence->seqno;
 
 out:
@@ -460,6 +485,8 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 	submit_cleanup(submit);
 
 err_submit_cmds:
+	if (ret && (out_fence_fd >= 0))
+		put_unused_fd(out_fence_fd);
 	/* if we still own the cmdbuf */
 	if (cmdbuf)
 		etnaviv_cmdbuf_free(cmdbuf);
diff --git a/include/uapi/drm/etnaviv_drm.h b/include/uapi/drm/etnaviv_drm.h
index e9c388a1d8ebe..76f6f78a352ba 100644
--- a/include/uapi/drm/etnaviv_drm.h
+++ b/include/uapi/drm/etnaviv_drm.h
@@ -156,8 +156,10 @@ struct drm_etnaviv_gem_submit_bo {
  */
 #define ETNA_SUBMIT_NO_IMPLICIT         0x0001
 #define ETNA_SUBMIT_FENCE_FD_IN         0x0002
+#define ETNA_SUBMIT_FENCE_FD_OUT        0x0004
 #define ETNA_SUBMIT_FLAGS		(ETNA_SUBMIT_NO_IMPLICIT | \
-					 ETNA_SUBMIT_FENCE_FD_IN)
+					 ETNA_SUBMIT_FENCE_FD_IN | \
+					 ETNA_SUBMIT_FENCE_FD_OUT)
 #define ETNA_PIPE_3D      0x00
 #define ETNA_PIPE_2D      0x01
 #define ETNA_PIPE_VG      0x02
@@ -172,7 +174,7 @@ struct drm_etnaviv_gem_submit {
 	__u64 relocs;         /* in, ptr to array of submit_reloc's */
 	__u64 stream;         /* in, ptr to cmdstream */
 	__u32 flags;          /* in, mask of ETNA_SUBMIT_x */
-	__s32 fence_fd;       /* in, fence fd (see ETNA_SUBMIT_FENCE_FD_IN) */
+	__s32 fence_fd;       /* in/out, fence fd (see ETNA_SUBMIT_FENCE_FD_x) */
 };
 
 /* The normal way to synchronize with the GPU is just to CPU_PREP on
-- 
2.11.0

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

Re: [PATCH 1/3] drm/etnaviv: submit support for in-fences

[Gustavo Padovan]

Hi Philipp,

2017-03-08 Philipp Zabel <p.zabel@pengutronix.de>:

> Loosely based on commit f0a42bb5423a ("drm/msm: submit support for
> in-fences"). Unfortunately, struct drm_etnaviv_gem_submit doesn't have
> a flags field yet, so we have to extend the structure and trust that
> drm_ioctl will clear the flags for us if an older userspace only submits
> part of the struct.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/gpu/drm/etnaviv/Kconfig              |  1 +
>  drivers/gpu/drm/etnaviv/etnaviv_gem.h        |  1 +
>  drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 34 +++++++++++++++++++++++++++-
>  drivers/gpu/drm/etnaviv/etnaviv_gpu.c        |  5 +++-
>  drivers/gpu/drm/etnaviv/etnaviv_gpu.h        |  2 +-
>  include/uapi/drm/etnaviv_drm.h               |  6 +++++
>  6 files changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/Kconfig b/drivers/gpu/drm/etnaviv/Kconfig
> index cc1731c5289c2..71cee4e9fefbb 100644
> --- a/drivers/gpu/drm/etnaviv/Kconfig
> +++ b/drivers/gpu/drm/etnaviv/Kconfig
> @@ -5,6 +5,7 @@ config DRM_ETNAVIV
>  	depends on ARCH_MXC || ARCH_DOVE || (ARM && COMPILE_TEST)
>  	depends on MMU
>  	select SHMEM
> +	select SYNC_FILE
>  	select TMPFS
>  	select IOMMU_API
>  	select IOMMU_SUPPORT
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> index e63ff116a3b3d..120410d67eb5b 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> @@ -107,6 +107,7 @@ struct etnaviv_gem_submit {
>  	u32 fence;
>  	unsigned int nr_bos;
>  	struct etnaviv_gem_submit_bo bos[0];
> +	u32 flags;
>  };
>  
>  int etnaviv_gem_wait_bo(struct etnaviv_gpu *gpu, struct drm_gem_object *obj,
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> index 726090d7a6ace..e67d83eac22dc 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> @@ -15,6 +15,7 @@
>   */
>  
>  #include <linux/reservation.h>
> +#include <linux/sync_file.h>
>  #include "etnaviv_cmdbuf.h"
>  #include "etnaviv_drv.h"
>  #include "etnaviv_gpu.h"
> @@ -169,8 +170,10 @@ static int submit_fence_sync(const struct etnaviv_gem_submit *submit)
>  	for (i = 0; i < submit->nr_bos; i++) {
>  		struct etnaviv_gem_object *etnaviv_obj = submit->bos[i].obj;
>  		bool write = submit->bos[i].flags & ETNA_SUBMIT_BO_WRITE;
> +		bool explicit = !(submit->flags & ETNA_SUBMIT_NO_IMPLICIT);
>  
> -		ret = etnaviv_gpu_fence_sync_obj(etnaviv_obj, context, write);
> +		ret = etnaviv_gpu_fence_sync_obj(etnaviv_obj, context, write,
> +						 explicit);
>  		if (ret)
>  			break;
>  	}
> @@ -303,6 +306,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
>  	struct etnaviv_gem_submit *submit;
>  	struct etnaviv_cmdbuf *cmdbuf;
>  	struct etnaviv_gpu *gpu;
> +	struct dma_fence *in_fence = NULL;
>  	void *stream;
>  	int ret;
>  
> @@ -326,6 +330,11 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
>  		return -EINVAL;
>  	}
>  
> +	if (args->flags & ~ETNA_SUBMIT_FLAGS) {
> +		DRM_ERROR("invalid flags: 0x%x\n", args->flags);
> +		return -EINVAL;
> +	}
> +
>  	/*
>  	 * Copy the command submission and bo array to kernel space in
>  	 * one go, and do this outside of any locks.
> @@ -371,6 +380,8 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
>  		goto err_submit_cmds;
>  	}
>  
> +	submit->flags = args->flags;
> +
>  	ret = submit_lookup_objects(submit, file, bos, args->nr_bos);
>  	if (ret)
>  		goto err_submit_objects;
> @@ -385,6 +396,25 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
>  		goto err_submit_objects;
>  	}
>  
> +	if (args->flags & ETNA_SUBMIT_FENCE_FD_IN) {
> +		in_fence = sync_file_get_fence(args->fence_fd);
> +		if (!in_fence) {
> +			ret = -EINVAL;
> +			goto err_submit_objects;
> +		}
> +
> +		/* TODO if we get an array-fence due to userspace merging
> +		 * multiple fences, we need a way to determine if all the
> +		 * backing fences are from our own context..
> +		 */

All GPU drivers seem to have the same need on fence_array, so I think we
can create a fence array helper to inspect if it has a specific context
or not.

> +
> +		if (in_fence->context != gpu->fence_context) {
> +			ret = dma_fence_wait(in_fence, true);
> +			if (ret)
> +				goto err_submit_objects;

sync_file_get_fence() hold a fence ref, we need to release it here
always and not only in case of error.

> +		}
> +	}
> +
>  	ret = submit_fence_sync(submit);
>  	if (ret)
>  		goto err_submit_objects;
> @@ -419,6 +449,8 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
>  		flush_workqueue(priv->wq);
>  
>  err_submit_objects:
> +	if (in_fence)
> +		dma_fence_put(in_fence);
>  	submit_cleanup(submit);
>  
>  err_submit_cmds:
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> index 130d7d517a19a..51d52c72aef17 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> @@ -1064,7 +1064,7 @@ static struct dma_fence *etnaviv_gpu_fence_alloc(struct etnaviv_gpu *gpu)
>  }
>  
>  int etnaviv_gpu_fence_sync_obj(struct etnaviv_gem_object *etnaviv_obj,
> -	unsigned int context, bool exclusive)
> +	unsigned int context, bool exclusive, bool explicit)
>  {
>  	struct reservation_object *robj = etnaviv_obj->resv;
>  	struct reservation_object_list *fobj;
> @@ -1077,6 +1077,9 @@ int etnaviv_gpu_fence_sync_obj(struct etnaviv_gem_object *etnaviv_obj,
>  			return ret;
>  	}
>  
> +	if (explicit)
> +		return 0;
> +
>  	/*
>  	 * If we have any shared fences, then the exclusive fence
>  	 * should be ignored as it will already have been signalled.
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> index 1c0606ea7d5e8..dc27c7a039060 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> @@ -181,7 +181,7 @@ int etnaviv_gpu_debugfs(struct etnaviv_gpu *gpu, struct seq_file *m);
>  #endif
>  
>  int etnaviv_gpu_fence_sync_obj(struct etnaviv_gem_object *etnaviv_obj,
> -	unsigned int context, bool exclusive);
> +	unsigned int context, bool exclusive, bool implicit);
>  
>  void etnaviv_gpu_retire(struct etnaviv_gpu *gpu);
>  int etnaviv_gpu_wait_fence_interruptible(struct etnaviv_gpu *gpu,
> diff --git a/include/uapi/drm/etnaviv_drm.h b/include/uapi/drm/etnaviv_drm.h
> index 2584c1cca42f6..e9c388a1d8ebe 100644
> --- a/include/uapi/drm/etnaviv_drm.h
> +++ b/include/uapi/drm/etnaviv_drm.h
> @@ -154,6 +154,10 @@ struct drm_etnaviv_gem_submit_bo {
>   * one or more cmdstream buffers.  This allows for conditional execution
>   * (context-restore), and IB buffers needed for per tile/bin draw cmds.
>   */
> +#define ETNA_SUBMIT_NO_IMPLICIT         0x0001
> +#define ETNA_SUBMIT_FENCE_FD_IN         0x0002

ETNA_SUBMIT_NO_IMPLICIT and ETNA_SUBMIT_FENCE_FD_IN are the same, when
you send and fence_fd to the kernel you are requesting on explicit sync
thus I think the ETNA_SUBMIT_NO_IMPLICIT can be dropped and
ETNA_SUBMIT_FENCE_FD_IN would be the one to look at.

> +#define ETNA_SUBMIT_FLAGS		(ETNA_SUBMIT_NO_IMPLICIT | \
> +					 ETNA_SUBMIT_FENCE_FD_IN)
>  #define ETNA_PIPE_3D      0x00
>  #define ETNA_PIPE_2D      0x01
>  #define ETNA_PIPE_VG      0x02
> @@ -167,6 +171,8 @@ struct drm_etnaviv_gem_submit {
>  	__u64 bos;            /* in, ptr to array of submit_bo's */
>  	__u64 relocs;         /* in, ptr to array of submit_reloc's */
>  	__u64 stream;         /* in, ptr to cmdstream */
> +	__u32 flags;          /* in, mask of ETNA_SUBMIT_x */
> +	__s32 fence_fd;       /* in, fence fd (see ETNA_SUBMIT_FENCE_FD_IN) */
>  };
>  
>  /* The normal way to synchronize with the GPU is just to CPU_PREP on
> -- 
> 2.11.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/etnaviv: move fence allocation out of etnaviv_gpu_submit()

[Gustavo Padovan]

Hi Philipp,

2017-03-08 Philipp Zabel <p.zabel@pengutronix.de>:

> The next patch will need the dma_fence to create the sync_file in
> etnaviv_ioctl_gem_submit, in case an out_fence_fd is requested.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_gem.h        |  3 ++-
>  drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c |  8 +++++++-
>  drivers/gpu/drm/etnaviv/etnaviv_gpu.c        | 21 ++++++---------------
>  drivers/gpu/drm/etnaviv/etnaviv_gpu.h        |  1 +
>  4 files changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> index 120410d67eb5b..c4a091e874269 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> @@ -20,6 +20,7 @@
>  #include <linux/reservation.h>
>  #include "etnaviv_drv.h"
>  
> +struct dma_fence;

Why not #include <linux/dma_fence.h> ?

Other than that looks good to me:

Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.com>

Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/etnaviv: submit support for out-fences

[Gustavo Padovan]

Hi Philipp,

2017-03-08 Philipp Zabel <p.zabel@pengutronix.de>:

> Based on commit 4cd0945901a6 ("drm/msm: submit support for out-fences").
> We increment the minor driver version so userspace can detect explicit
> fence support.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_drv.c        |  2 +-
>  drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 27 +++++++++++++++++++++++++++
>  include/uapi/drm/etnaviv_drm.h               |  6 ++++--
>  3 files changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> index 587e45043542b..00f7e9acf68ad 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> @@ -512,7 +512,7 @@ static struct drm_driver etnaviv_drm_driver = {
>  	.desc               = "etnaviv DRM",
>  	.date               = "20151214",
>  	.major              = 1,
> -	.minor              = 0,
> +	.minor              = 1,
>  };
>  
>  /*
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> index 022fcc7d57f48..7d4dc946104b9 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> @@ -307,6 +307,8 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
>  	struct etnaviv_cmdbuf *cmdbuf;
>  	struct etnaviv_gpu *gpu;
>  	struct dma_fence *in_fence = NULL;
> +	struct sync_file *sync_file = NULL;
> +	int out_fence_fd = -1;
>  	void *stream;
>  	int ret;
>  
> @@ -374,6 +376,14 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
>  		goto err_submit_cmds;
>  	}
>  
> +	if (args->flags & ETNA_SUBMIT_FENCE_FD_OUT) {
> +		out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
> +		if (out_fence_fd < 0) {
> +			ret = out_fence_fd;
> +			goto err_submit_cmds;
> +		}
> +	}
> +
>  	submit = submit_create(dev, gpu, args->nr_bos);
>  	if (!submit) {
>  		ret = -ENOMEM;
> @@ -437,10 +447,25 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
>  		goto out;
>  	}
>  
> +	if (args->flags & ETNA_SUBMIT_FENCE_FD_OUT) {
> +		sync_file = sync_file_create(submit->fence);
> +		if (!sync_file) {
> +			dma_fence_put(submit->fence);
> +			submit->fence = NULL;
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +	}
> +
>  	ret = etnaviv_gpu_submit(gpu, submit, cmdbuf);
>  	if (ret == 0)
>  		cmdbuf = NULL;
>  
> +	if (args->flags & ETNA_SUBMIT_FENCE_FD_OUT) {
> +		fd_install(out_fence_fd, sync_file->file);
> +	}

Extra braces here.

Otherwise looks good to me.

Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.com>

Gustavo

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

Re: [PATCH 2/3] drm/etnaviv: move fence allocation out of etnaviv_gpu_submit()

[Russell King - ARM Linux]

On Wed, Mar 08, 2017 at 11:42:17AM -0300, Gustavo Padovan wrote:
> Hi Philipp,
> 
> 2017-03-08 Philipp Zabel <p.zabel@pengutronix.de>:
> 
> > The next patch will need the dma_fence to create the sync_file in
> > etnaviv_ioctl_gem_submit, in case an out_fence_fd is requested.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >  drivers/gpu/drm/etnaviv/etnaviv_gem.h        |  3 ++-
> >  drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c |  8 +++++++-
> >  drivers/gpu/drm/etnaviv/etnaviv_gpu.c        | 21 ++++++---------------
> >  drivers/gpu/drm/etnaviv/etnaviv_gpu.h        |  1 +
> >  4 files changed, 16 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> > index 120410d67eb5b..c4a091e874269 100644
> > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> > @@ -20,6 +20,7 @@
> >  #include <linux/reservation.h>
> >  #include "etnaviv_drv.h"
> >  
> > +struct dma_fence;
> 
> Why not #include <linux/dma_fence.h> ?

Adding needless includes when a struct prototype will do eventually
creates headaches with circular dependencies and the like.  Low
probability in this case, but the principle is a good one to adhere
to.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/etnaviv: submit support for in-fences

[Philipp Zabel]

Hi Gustavo,

thank you for the review.

On Wed, 2017-03-08 at 11:37 -0300, Gustavo Padovan wrote:
[...]
> > @@ -385,6 +396,25 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
> >  		goto err_submit_objects;
> >  	}
> >  
> > +	if (args->flags & ETNA_SUBMIT_FENCE_FD_IN) {
> > +		in_fence = sync_file_get_fence(args->fence_fd);
> > +		if (!in_fence) {
> > +			ret = -EINVAL;
> > +			goto err_submit_objects;
> > +		}
> > +
> > +		/* TODO if we get an array-fence due to userspace merging
> > +		 * multiple fences, we need a way to determine if all the
> > +		 * backing fences are from our own context..
> > +		 */
> 
> All GPU drivers seem to have the same need on fence_array, so I think we
> can create a fence array helper to inspect if it has a specific context
> or not.

Do you have a pointer where I could read up on how this should be done?

> > +
> > +		if (in_fence->context != gpu->fence_context) {
> > +			ret = dma_fence_wait(in_fence, true);
> > +			if (ret)
> > +				goto err_submit_objects;
> 
> sync_file_get_fence() hold a fence ref, we need to release it here
> always and not only in case of error.

We do that already. err_submit_objects is an unfortunate name for patch
review, but the out label at the end of the function falls right through
to it.

> > +		}
> > +	}
> > +
> >  	ret = submit_fence_sync(submit);
> >  	if (ret)
> >  		goto err_submit_objects;
> > @@ -419,6 +449,8 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
> >  		flush_workqueue(priv->wq);
> >  
> >  err_submit_objects:
> > +	if (in_fence)
> > +		dma_fence_put(in_fence);

We pass through here in the non-error case, too.

[...]
> > @@ -154,6 +154,10 @@ struct drm_etnaviv_gem_submit_bo {
> >   * one or more cmdstream buffers.  This allows for conditional execution
> >   * (context-restore), and IB buffers needed for per tile/bin draw cmds.
> >   */
> > +#define ETNA_SUBMIT_NO_IMPLICIT         0x0001
> > +#define ETNA_SUBMIT_FENCE_FD_IN         0x0002
> 
> ETNA_SUBMIT_NO_IMPLICIT and ETNA_SUBMIT_FENCE_FD_IN are the same, when
> you send and fence_fd to the kernel you are requesting on explicit sync
> thus I think the ETNA_SUBMIT_NO_IMPLICIT can be dropped and
> ETNA_SUBMIT_FENCE_FD_IN would be the one to look at.

I just followed Rob's lead. I'm not sure if there may be a reason to
submit an in fence still keep implicit fencing enabled at the same time,
or to allow a submit without any fencing whatsoever. Maybe for testing
purposes?
I'm happy to drop the NO_IMPLICIT flag if there is no need for it.

regards
Philipp

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

Re: [PATCH 3/3] drm/etnaviv: submit support for out-fences

[Philipp Zabel]

On Wed, 2017-03-08 at 11:48 -0300, Gustavo Padovan wrote:
[...]
> > @@ -437,10 +447,25 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
> >  		goto out;
> >  	}
> >  
> > +	if (args->flags & ETNA_SUBMIT_FENCE_FD_OUT) {
> > +		sync_file = sync_file_create(submit->fence);
> > +		if (!sync_file) {
> > +			dma_fence_put(submit->fence);
> > +			submit->fence = NULL;
> > +			ret = -ENOMEM;
> > +			goto out;
> > +		}
> > +	}
> > +
> >  	ret = etnaviv_gpu_submit(gpu, submit, cmdbuf);
> >  	if (ret == 0)
> >  		cmdbuf = NULL;
> >  
> > +	if (args->flags & ETNA_SUBMIT_FENCE_FD_OUT) {
> > +		fd_install(out_fence_fd, sync_file->file);
> > +	}
> 
> Extra braces here.

I'll drop those next time.

> Otherwise looks good to me.
> 
> Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.com>

thanks
Philipp

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

Re: [PATCH 2/3] drm/etnaviv: move fence allocation out of etnaviv_gpu_submit()

[Philipp Zabel]

On Wed, 2017-03-08 at 11:42 -0300, Gustavo Padovan wrote:
> Hi Philipp,
> 
> 2017-03-08 Philipp Zabel <p.zabel@pengutronix.de>:
> 
> > The next patch will need the dma_fence to create the sync_file in
> > etnaviv_ioctl_gem_submit, in case an out_fence_fd is requested.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >  drivers/gpu/drm/etnaviv/etnaviv_gem.h        |  3 ++-
> >  drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c |  8 +++++++-
> >  drivers/gpu/drm/etnaviv/etnaviv_gpu.c        | 21 ++++++---------------
> >  drivers/gpu/drm/etnaviv/etnaviv_gpu.h        |  1 +
> >  4 files changed, 16 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> > index 120410d67eb5b..c4a091e874269 100644
> > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> > @@ -20,6 +20,7 @@
> >  #include <linux/reservation.h>
> >  #include "etnaviv_drv.h"
> >  
> > +struct dma_fence;
> 
> Why not #include <linux/dma_fence.h> ?

I don't see the need to include the header as long as the memory layout
of struct dma_fence doesn't have to be known. Here we just need to pass
pointers to the structure type as function arguments.

> Other than that looks good to me:
> 
> Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.com>

May I keep the review tag without adding the #include?

regards
Philipp

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

Re: [PATCH 2/3] drm/etnaviv: move fence allocation out of etnaviv_gpu_submit()

[Gustavo Padovan]

2017-03-13 Philipp Zabel <p.zabel@pengutronix.de>:

> On Wed, 2017-03-08 at 11:42 -0300, Gustavo Padovan wrote:
> > Hi Philipp,
> > 
> > 2017-03-08 Philipp Zabel <p.zabel@pengutronix.de>:
> > 
> > > The next patch will need the dma_fence to create the sync_file in
> > > etnaviv_ioctl_gem_submit, in case an out_fence_fd is requested.
> > > 
> > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > ---
> > >  drivers/gpu/drm/etnaviv/etnaviv_gem.h        |  3 ++-
> > >  drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c |  8 +++++++-
> > >  drivers/gpu/drm/etnaviv/etnaviv_gpu.c        | 21 ++++++---------------
> > >  drivers/gpu/drm/etnaviv/etnaviv_gpu.h        |  1 +
> > >  4 files changed, 16 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> > > index 120410d67eb5b..c4a091e874269 100644
> > > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
> > > @@ -20,6 +20,7 @@
> > >  #include <linux/reservation.h>
> > >  #include "etnaviv_drv.h"
> > >  
> > > +struct dma_fence;
> > 
> > Why not #include <linux/dma_fence.h> ?
> 
> I don't see the need to include the header as long as the memory layout
> of struct dma_fence doesn't have to be known. Here we just need to pass
> pointers to the structure type as function arguments.
> 
> > Other than that looks good to me:
> > 
> > Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> 
> May I keep the review tag without adding the #include?

Sure. Please keep it.

Gustavo

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

Re: [PATCH 1/3] drm/etnaviv: submit support for in-fences

[Gustavo Padovan]

Hi Philipp,

2017-03-13 Philipp Zabel <p.zabel@pengutronix.de>:

> Hi Gustavo,
> 
> thank you for the review.
> 
> On Wed, 2017-03-08 at 11:37 -0300, Gustavo Padovan wrote:
> [...]
> > > @@ -385,6 +396,25 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
> > >  		goto err_submit_objects;
> > >  	}
> > >  
> > > +	if (args->flags & ETNA_SUBMIT_FENCE_FD_IN) {
> > > +		in_fence = sync_file_get_fence(args->fence_fd);
> > > +		if (!in_fence) {
> > > +			ret = -EINVAL;
> > > +			goto err_submit_objects;
> > > +		}
> > > +
> > > +		/* TODO if we get an array-fence due to userspace merging
> > > +		 * multiple fences, we need a way to determine if all the
> > > +		 * backing fences are from our own context..
> > > +		 */
> > 
> > All GPU drivers seem to have the same need on fence_array, so I think we
> > can create a fence array helper to inspect if it has a specific context
> > or not.
> 
> Do you have a pointer where I could read up on how this should be done?

I was thinking on some function that would iterate over all fences in
the fence_array and check their context. The if we find our own gpu
context in there we fail the submit.

> 
> > > +
> > > +		if (in_fence->context != gpu->fence_context) {
> > > +			ret = dma_fence_wait(in_fence, true);
> > > +			if (ret)
> > > +				goto err_submit_objects;
> > 
> > sync_file_get_fence() hold a fence ref, we need to release it here
> > always and not only in case of error.
> 
> We do that already. err_submit_objects is an unfortunate name for patch
> review, but the out label at the end of the function falls right through
> to it.
> 
> > > +		}
> > > +	}
> > > +
> > >  	ret = submit_fence_sync(submit);
> > >  	if (ret)
> > >  		goto err_submit_objects;
> > > @@ -419,6 +449,8 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
> > >  		flush_workqueue(priv->wq);
> > >  
> > >  err_submit_objects:
> > > +	if (in_fence)
> > > +		dma_fence_put(in_fence);
> 
> We pass through here in the non-error case, too.

Cool.

> 
> [...]
> > > @@ -154,6 +154,10 @@ struct drm_etnaviv_gem_submit_bo {
> > >   * one or more cmdstream buffers.  This allows for conditional execution
> > >   * (context-restore), and IB buffers needed for per tile/bin draw cmds.
> > >   */
> > > +#define ETNA_SUBMIT_NO_IMPLICIT         0x0001
> > > +#define ETNA_SUBMIT_FENCE_FD_IN         0x0002
> > 
> > ETNA_SUBMIT_NO_IMPLICIT and ETNA_SUBMIT_FENCE_FD_IN are the same, when
> > you send and fence_fd to the kernel you are requesting on explicit sync
> > thus I think the ETNA_SUBMIT_NO_IMPLICIT can be dropped and
> > ETNA_SUBMIT_FENCE_FD_IN would be the one to look at.
> 
> I just followed Rob's lead. I'm not sure if there may be a reason to
> submit an in fence still keep implicit fencing enabled at the same time,
> or to allow a submit without any fencing whatsoever. Maybe for testing
> purposes?
> I'm happy to drop the NO_IMPLICIT flag if there is no need for it.

Right. My understanding is that the flags would mean the same thing, but
I'm no expert on the GPU side of things. Maybe Rob can provide us some
insight on why he added NO_IMPLICIT.

Gustavo

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

Re: [PATCH 1/3] drm/etnaviv: submit support for in-fences

[Philipp Zabel]

Hi Gustavo,

On Mon, 2017-03-13 at 14:37 -0300, Gustavo Padovan wrote:
[...]
> I was thinking on some function that would iterate over all fences in
> the fence_array and check their context. The if we find our own gpu
> context in there we fail the submit.

Why would we have to fail if somebody feeds us our own fences? Wouldn't
it be enough to just wait if there are foreign fences in the array?

How about something like this:

----------8<----------
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
index 364fe50d020de..0b0bdaf4406d4 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
@@ -296,6 +296,22 @@ static void submit_cleanup(struct etnaviv_gem_submit *submit)
 	kfree(submit);
 }
 
+static bool dma_fence_all_in_context(struct dma_fence *fence, u64 context)
+{
+	if (dma_fence_is_array(fence)) {
+		struct dma_fence_array *array = to_dma_fence_array(fence);
+		int i;
+
+		for (i = 0; i < array->num_fences; i++) {
+			if (array->fences[i]->context != context)
+				return false;
+		}
+		return true;
+	}
+
+	return fence->context == context;
+}
+
 int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 		struct drm_file *file)
 {
@@ -413,12 +429,11 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
 			goto err_submit_objects;
 		}
 
-		/* TODO if we get an array-fence due to userspace merging
-		 * multiple fences, we need a way to determine if all the
-		 * backing fences are from our own context..
+		/*
+		 * Wait if the fence is from a foreign context, or if the fence
+		 * array contains any fence from a foreign context.
 		 */
-
-		if (in_fence->context != gpu->fence_context) {
+		if (!dma_fence_all_in_context(in_fence, gpu->fence_context)) {
 			ret = dma_fence_wait(in_fence, true);
 			if (ret)
 				goto err_submit_objects;
---------->8----------

[...]
> > > > @@ -154,6 +154,10 @@ struct drm_etnaviv_gem_submit_bo {
> > > >   * one or more cmdstream buffers.  This allows for conditional execution
> > > >   * (context-restore), and IB buffers needed for per tile/bin draw cmds.
> > > >   */
> > > > +#define ETNA_SUBMIT_NO_IMPLICIT         0x0001
> > > > +#define ETNA_SUBMIT_FENCE_FD_IN         0x0002
> > > 
> > > ETNA_SUBMIT_NO_IMPLICIT and ETNA_SUBMIT_FENCE_FD_IN are the same, when
> > > you send and fence_fd to the kernel you are requesting on explicit sync
> > > thus I think the ETNA_SUBMIT_NO_IMPLICIT can be dropped and
> > > ETNA_SUBMIT_FENCE_FD_IN would be the one to look at.
> > 
> > I just followed Rob's lead. I'm not sure if there may be a reason to
> > submit an in fence still keep implicit fencing enabled at the same time,
> > or to allow a submit without any fencing whatsoever. Maybe for testing
> > purposes?
> > I'm happy to drop the NO_IMPLICIT flag if there is no need for it.
> 
> Right. My understanding is that the flags would mean the same thing, but
> I'm no expert on the GPU side of things. Maybe Rob can provide us some
> insight on why he added NO_IMPLICIT.

Yes, that would be welcome.

regards
Philipp

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

Re: [PATCH 1/3] drm/etnaviv: submit support for in-fences

[Rob Clark]

On Wed, Mar 8, 2017 at 9:37 AM, Gustavo Padovan <gustavo@padovan.org> wrote:
>> diff --git a/include/uapi/drm/etnaviv_drm.h b/include/uapi/drm/etnaviv_drm.h
>> index 2584c1cca42f6..e9c388a1d8ebe 100644
>> --- a/include/uapi/drm/etnaviv_drm.h
>> +++ b/include/uapi/drm/etnaviv_drm.h
>> @@ -154,6 +154,10 @@ struct drm_etnaviv_gem_submit_bo {
>>   * one or more cmdstream buffers.  This allows for conditional execution
>>   * (context-restore), and IB buffers needed for per tile/bin draw cmds.
>>   */
>> +#define ETNA_SUBMIT_NO_IMPLICIT         0x0001
>> +#define ETNA_SUBMIT_FENCE_FD_IN         0x0002
>
> ETNA_SUBMIT_NO_IMPLICIT and ETNA_SUBMIT_FENCE_FD_IN are the same, when
> you send and fence_fd to the kernel you are requesting on explicit sync
> thus I think the ETNA_SUBMIT_NO_IMPLICIT can be dropped and
> ETNA_SUBMIT_FENCE_FD_IN would be the one to look at.

jfwiw, I kept separate no-implicit and fence-fd-in flags in msm mostly
because I couldn't think of a good backwards-compatible way to add it
later if needed.  Currently userspace sets both flags together, and
possibly always will.  But keeping separate flags seemed like a good
idea for future-proofing..

BR,
-R


>> +#define ETNA_SUBMIT_FLAGS            (ETNA_SUBMIT_NO_IMPLICIT | \
>> +                                      ETNA_SUBMIT_FENCE_FD_IN)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/etnaviv: submit support for in-fences

[Gustavo Padovan]

2017-03-16 Rob Clark <robdclark@gmail.com>:

> On Wed, Mar 8, 2017 at 9:37 AM, Gustavo Padovan <gustavo@padovan.org> wrote:
> >> diff --git a/include/uapi/drm/etnaviv_drm.h b/include/uapi/drm/etnaviv_drm.h
> >> index 2584c1cca42f6..e9c388a1d8ebe 100644
> >> --- a/include/uapi/drm/etnaviv_drm.h
> >> +++ b/include/uapi/drm/etnaviv_drm.h
> >> @@ -154,6 +154,10 @@ struct drm_etnaviv_gem_submit_bo {
> >>   * one or more cmdstream buffers.  This allows for conditional execution
> >>   * (context-restore), and IB buffers needed for per tile/bin draw cmds.
> >>   */
> >> +#define ETNA_SUBMIT_NO_IMPLICIT         0x0001
> >> +#define ETNA_SUBMIT_FENCE_FD_IN         0x0002
> >
> > ETNA_SUBMIT_NO_IMPLICIT and ETNA_SUBMIT_FENCE_FD_IN are the same, when
> > you send and fence_fd to the kernel you are requesting on explicit sync
> > thus I think the ETNA_SUBMIT_NO_IMPLICIT can be dropped and
> > ETNA_SUBMIT_FENCE_FD_IN would be the one to look at.
> 
> jfwiw, I kept separate no-implicit and fence-fd-in flags in msm mostly
> because I couldn't think of a good backwards-compatible way to add it
> later if needed.  Currently userspace sets both flags together, and
> possibly always will.  But keeping separate flags seemed like a good
> idea for future-proofing..

Fair enough. Let's do the same for etnaviv then.

Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/etnaviv: submit support for in-fences

[Gustavo Padovan]

2017-03-16 Philipp Zabel <p.zabel@pengutronix.de>:

> Hi Gustavo,
> 
> On Mon, 2017-03-13 at 14:37 -0300, Gustavo Padovan wrote:
> [...]
> > I was thinking on some function that would iterate over all fences in
> > the fence_array and check their context. The if we find our own gpu
> > context in there we fail the submit.
> 
> Why would we have to fail if somebody feeds us our own fences? Wouldn't
> it be enough to just wait if there are foreign fences in the array?

You don't need to fail necessarily. In my mind I had the use case that
maybe some driver could deadlock waiting for his own fence. What you
do with the information that the array has it a fence with the driver's
context is entirely up to the driver to decide.

> 
> How about something like this:
> 
> ----------8<----------
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> index 364fe50d020de..0b0bdaf4406d4 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> @@ -296,6 +296,22 @@ static void submit_cleanup(struct etnaviv_gem_submit *submit)
>  	kfree(submit);
>  }
>  
> +static bool dma_fence_all_in_context(struct dma_fence *fence, u64 context)
> +{
> +	if (dma_fence_is_array(fence)) {
> +		struct dma_fence_array *array = to_dma_fence_array(fence);
> +		int i;
> +
> +		for (i = 0; i < array->num_fences; i++) {
> +			if (array->fences[i]->context != context)
> +				return false;
> +		}
> +		return true;
> +	}
> +
> +	return fence->context == context;
> +}

If we don't mind having fences with our own context, why should we check
them?

Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/etnaviv: submit support for in-fences

[Philipp Zabel]

On Fri, 2017-03-17 at 11:00 -0300, Gustavo Padovan wrote:
> 2017-03-16 Philipp Zabel <p.zabel@pengutronix.de>:
> 
> > Hi Gustavo,
> > 
> > On Mon, 2017-03-13 at 14:37 -0300, Gustavo Padovan wrote:
> > [...]
> > > I was thinking on some function that would iterate over all fences in
> > > the fence_array and check their context. The if we find our own gpu
> > > context in there we fail the submit.
> > 
> > Why would we have to fail if somebody feeds us our own fences? Wouldn't
> > it be enough to just wait if there are foreign fences in the array?
> 
> You don't need to fail necessarily. In my mind I had the use case that
> maybe some driver could deadlock waiting for his own fence. What you
> do with the information that the array has it a fence with the driver's
> context is entirely up to the driver to decide.
[...]
> If we don't mind having fences with our own context, why should we check
> them?

My understanding was that for foreign fences we have to wait, for
example before resolving into a framebuffer that is still being scanned
out. But if we are fed fences from our own context, we can just ignore
them because etnaviv just has a single command queue, so anything
waiting for a fence from our own context will be queued after that fence
is signaled anyway.

regards
Philipp

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

Re: [PATCH 1/3] drm/etnaviv: submit support for in-fences

[Philipp Zabel]

On Fri, 2017-03-17 at 10:55 -0300, Gustavo Padovan wrote:
> 2017-03-16 Rob Clark <robdclark@gmail.com>:
> 
> > On Wed, Mar 8, 2017 at 9:37 AM, Gustavo Padovan <gustavo@padovan.org> wrote:
> > >> diff --git a/include/uapi/drm/etnaviv_drm.h b/include/uapi/drm/etnaviv_drm.h
> > >> index 2584c1cca42f6..e9c388a1d8ebe 100644
> > >> --- a/include/uapi/drm/etnaviv_drm.h
> > >> +++ b/include/uapi/drm/etnaviv_drm.h
> > >> @@ -154,6 +154,10 @@ struct drm_etnaviv_gem_submit_bo {
> > >>   * one or more cmdstream buffers.  This allows for conditional execution
> > >>   * (context-restore), and IB buffers needed for per tile/bin draw cmds.
> > >>   */
> > >> +#define ETNA_SUBMIT_NO_IMPLICIT         0x0001
> > >> +#define ETNA_SUBMIT_FENCE_FD_IN         0x0002
> > >
> > > ETNA_SUBMIT_NO_IMPLICIT and ETNA_SUBMIT_FENCE_FD_IN are the same, when
> > > you send and fence_fd to the kernel you are requesting on explicit sync
> > > thus I think the ETNA_SUBMIT_NO_IMPLICIT can be dropped and
> > > ETNA_SUBMIT_FENCE_FD_IN would be the one to look at.
> > 
> > jfwiw, I kept separate no-implicit and fence-fd-in flags in msm mostly
> > because I couldn't think of a good backwards-compatible way to add it
> > later if needed.  Currently userspace sets both flags together, and
> > possibly always will.  But keeping separate flags seemed like a good
> > idea for future-proofing..
> 
> Fair enough. Let's do the same for etnaviv then.

Alright. Unless Lucas disagrees, I'll keep it as is for consistency.

regards
Philipp

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

Re: [PATCH 1/3] drm/etnaviv: submit support for in-fences

[Lucas Stach]

Am Donnerstag, den 16.03.2017, 12:05 +0100 schrieb Philipp Zabel:
> Hi Gustavo,
> 
> On Mon, 2017-03-13 at 14:37 -0300, Gustavo Padovan wrote:
> [...]
> > I was thinking on some function that would iterate over all fences in
> > the fence_array and check their context. The if we find our own gpu
> > context in there we fail the submit.
> 
> Why would we have to fail if somebody feeds us our own fences? Wouldn't
> it be enough to just wait if there are foreign fences in the array?

Yes, skipping the wait if all fences are from our own context is an
optimization and it's certainly not an issue if someone feeds us our own
fences.

> 
> How about something like this:
> 
> ----------8<----------
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> index 364fe50d020de..0b0bdaf4406d4 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> @@ -296,6 +296,22 @@ static void submit_cleanup(struct etnaviv_gem_submit *submit)
>  	kfree(submit);
>  }
>  
> +static bool dma_fence_all_in_context(struct dma_fence *fence, u64 context)

dma_fence_match_context?

> +{
> +	if (dma_fence_is_array(fence)) {
> +		struct dma_fence_array *array = to_dma_fence_array(fence);
> +		int i;
> +
> +		for (i = 0; i < array->num_fences; i++) {
> +			if (array->fences[i]->context != context)
> +				return false;
> +		}
> +		return true;
> +	}
> +
> +	return fence->context == context;
> +}
> +

This looks good to me. Please add this to a common location in the next
submission.

>  int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
>  		struct drm_file *file)
>  {
> @@ -413,12 +429,11 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
>  			goto err_submit_objects;
>  		}
>  
> -		/* TODO if we get an array-fence due to userspace merging
> -		 * multiple fences, we need a way to determine if all the
> -		 * backing fences are from our own context..
> +		/*
> +		 * Wait if the fence is from a foreign context, or if the fence
> +		 * array contains any fence from a foreign context.
>  		 */
> -
> -		if (in_fence->context != gpu->fence_context) {
> +		if (!dma_fence_all_in_context(in_fence, gpu->fence_context)) {
>  			ret = dma_fence_wait(in_fence, true);
>  			if (ret)
>  				goto err_submit_objects;
> ---------->8----------
> 
> [...]
> > > > > @@ -154,6 +154,10 @@ struct drm_etnaviv_gem_submit_bo {
> > > > >   * one or more cmdstream buffers.  This allows for conditional execution
> > > > >   * (context-restore), and IB buffers needed for per tile/bin draw cmds.
> > > > >   */
> > > > > +#define ETNA_SUBMIT_NO_IMPLICIT         0x0001
> > > > > +#define ETNA_SUBMIT_FENCE_FD_IN         0x0002
> > > > 
> > > > ETNA_SUBMIT_NO_IMPLICIT and ETNA_SUBMIT_FENCE_FD_IN are the same, when
> > > > you send and fence_fd to the kernel you are requesting on explicit sync
> > > > thus I think the ETNA_SUBMIT_NO_IMPLICIT can be dropped and
> > > > ETNA_SUBMIT_FENCE_FD_IN would be the one to look at.
> > > 
> > > I just followed Rob's lead. I'm not sure if there may be a reason to
> > > submit an in fence still keep implicit fencing enabled at the same time,
> > > or to allow a submit without any fencing whatsoever. Maybe for testing
> > > purposes?
> > > I'm happy to drop the NO_IMPLICIT flag if there is no need for it.
> > 
> > Right. My understanding is that the flags would mean the same thing, but
> > I'm no expert on the GPU side of things. Maybe Rob can provide us some
> > insight on why he added NO_IMPLICIT.
> 
> Yes, that would be welcome.


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

Re: [PATCH 1/3] drm/etnaviv: submit support for in-fences

[Lucas Stach]

Am Freitag, den 17.03.2017, 15:09 +0100 schrieb Philipp Zabel:
> On Fri, 2017-03-17 at 10:55 -0300, Gustavo Padovan wrote:
> > 2017-03-16 Rob Clark <robdclark@gmail.com>:
> > 
> > > On Wed, Mar 8, 2017 at 9:37 AM, Gustavo Padovan <gustavo@padovan.org> wrote:
> > > >> diff --git a/include/uapi/drm/etnaviv_drm.h b/include/uapi/drm/etnaviv_drm.h
> > > >> index 2584c1cca42f6..e9c388a1d8ebe 100644
> > > >> --- a/include/uapi/drm/etnaviv_drm.h
> > > >> +++ b/include/uapi/drm/etnaviv_drm.h
> > > >> @@ -154,6 +154,10 @@ struct drm_etnaviv_gem_submit_bo {
> > > >>   * one or more cmdstream buffers.  This allows for conditional execution
> > > >>   * (context-restore), and IB buffers needed for per tile/bin draw cmds.
> > > >>   */
> > > >> +#define ETNA_SUBMIT_NO_IMPLICIT         0x0001
> > > >> +#define ETNA_SUBMIT_FENCE_FD_IN         0x0002
> > > >
> > > > ETNA_SUBMIT_NO_IMPLICIT and ETNA_SUBMIT_FENCE_FD_IN are the same, when
> > > > you send and fence_fd to the kernel you are requesting on explicit sync
> > > > thus I think the ETNA_SUBMIT_NO_IMPLICIT can be dropped and
> > > > ETNA_SUBMIT_FENCE_FD_IN would be the one to look at.
> > > 
> > > jfwiw, I kept separate no-implicit and fence-fd-in flags in msm mostly
> > > because I couldn't think of a good backwards-compatible way to add it
> > > later if needed.  Currently userspace sets both flags together, and
> > > possibly always will.  But keeping separate flags seemed like a good
> > > idea for future-proofing..
> > 
> > Fair enough. Let's do the same for etnaviv then.
> 
> Alright. Unless Lucas disagrees, I'll keep it as is for consistency.

This flag might make things a bit more "fun" later, as we might need to
merge fences (or even fence arrays of different types) if we are going
to use both implicit and explicit fences to infer scheduling decisions
from.

But to avoid introducing any unnecessary differences between freedreno
and etnaviv, I would suggest to keep the flag around.

Regards,
Lucas

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

Re: [PATCH 1/3] drm/etnaviv: submit support for in-fences

[Russell King - ARM Linux]

On Fri, Mar 17, 2017 at 03:10:21PM +0100, Lucas Stach wrote:
> Am Donnerstag, den 16.03.2017, 12:05 +0100 schrieb Philipp Zabel:
> > Hi Gustavo,
> > 
> > On Mon, 2017-03-13 at 14:37 -0300, Gustavo Padovan wrote:
> > [...]
> > > I was thinking on some function that would iterate over all fences in
> > > the fence_array and check their context. The if we find our own gpu
> > > context in there we fail the submit.
> > 
> > Why would we have to fail if somebody feeds us our own fences? Wouldn't
> > it be enough to just wait if there are foreign fences in the array?
> 
> Yes, skipping the wait if all fences are from our own context is an
> optimization and it's certainly not an issue if someone feeds us our own
> fences.

Are you sure about that - what if we have two GPUs, a 2D and 3D GPU,
and we're fed an etnaviv fence for one GPU when submitting to the
other GPU.

So we do end up being fed our own fences, and we have to respect them
otherwise we lose inter-GPU synchronisation, and that will break
existing userspace.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/etnaviv: submit support for in-fences

[Lucas Stach]

Am Freitag, den 17.03.2017, 14:42 +0000 schrieb Russell King - ARM
Linux:
> On Fri, Mar 17, 2017 at 03:10:21PM +0100, Lucas Stach wrote:
> > Am Donnerstag, den 16.03.2017, 12:05 +0100 schrieb Philipp Zabel:
> > > Hi Gustavo,
> > > 
> > > On Mon, 2017-03-13 at 14:37 -0300, Gustavo Padovan wrote:
> > > [...]
> > > > I was thinking on some function that would iterate over all fences in
> > > > the fence_array and check their context. The if we find our own gpu
> > > > context in there we fail the submit.
> > > 
> > > Why would we have to fail if somebody feeds us our own fences? Wouldn't
> > > it be enough to just wait if there are foreign fences in the array?
> > 
> > Yes, skipping the wait if all fences are from our own context is an
> > optimization and it's certainly not an issue if someone feeds us our own
> > fences.
> 
> Are you sure about that - what if we have two GPUs, a 2D and 3D GPU,
> and we're fed an etnaviv fence for one GPU when submitting to the
> other GPU.
> 
> So we do end up being fed our own fences, and we have to respect them
> otherwise we lose inter-GPU synchronisation, and that will break
> existing userspace.
> 
The etnaviv GPUs, while being on the same DRM device, have distinct
fence contexts. So the 3D GPU will consider a fence from the 2D GPU as
foreign and properly wait on it.

It's only when we get an in fence that has been generated as an out
fence by one (or multiple) submits to the same GPU, that we are able to
skip the wait and enqueue the command without waiting for the fence to
signal.

Regards,
Lucas

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

Re: [PATCH 1/3] drm/etnaviv: submit support for in-fences

[Russell King - ARM Linux]

On Fri, Mar 17, 2017 at 03:58:27PM +0100, Lucas Stach wrote:
> Am Freitag, den 17.03.2017, 14:42 +0000 schrieb Russell King - ARM
> Linux:
> > On Fri, Mar 17, 2017 at 03:10:21PM +0100, Lucas Stach wrote:
> > > Am Donnerstag, den 16.03.2017, 12:05 +0100 schrieb Philipp Zabel:
> > > > Hi Gustavo,
> > > > 
> > > > On Mon, 2017-03-13 at 14:37 -0300, Gustavo Padovan wrote:
> > > > [...]
> > > > > I was thinking on some function that would iterate over all fences in
> > > > > the fence_array and check their context. The if we find our own gpu
> > > > > context in there we fail the submit.
> > > > 
> > > > Why would we have to fail if somebody feeds us our own fences? Wouldn't
> > > > it be enough to just wait if there are foreign fences in the array?
> > > 
> > > Yes, skipping the wait if all fences are from our own context is an
> > > optimization and it's certainly not an issue if someone feeds us our own
> > > fences.
> > 
> > Are you sure about that - what if we have two GPUs, a 2D and 3D GPU,
> > and we're fed an etnaviv fence for one GPU when submitting to the
> > other GPU.
> > 
> > So we do end up being fed our own fences, and we have to respect them
> > otherwise we lose inter-GPU synchronisation, and that will break
> > existing userspace.
> > 
> The etnaviv GPUs, while being on the same DRM device, have distinct
> fence contexts. So the 3D GPU will consider a fence from the 2D GPU as
> foreign and properly wait on it.
> 
> It's only when we get an in fence that has been generated as an out
> fence by one (or multiple) submits to the same GPU, that we are able to
> skip the wait and enqueue the command without waiting for the fence to
> signal.

Sounds fine then.  Thanks.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/etnaviv: submit support for in-fences

[Chris Healy]

On Fri, Mar 17, 2017 at 7:58 AM, Lucas Stach <l.stach@pengutronix.de> wrote:
> Am Freitag, den 17.03.2017, 14:42 +0000 schrieb Russell King - ARM
> Linux:
>> On Fri, Mar 17, 2017 at 03:10:21PM +0100, Lucas Stach wrote:
>> > Am Donnerstag, den 16.03.2017, 12:05 +0100 schrieb Philipp Zabel:
>> > > Hi Gustavo,
>> > >
>> > > On Mon, 2017-03-13 at 14:37 -0300, Gustavo Padovan wrote:
>> > > [...]
>> > > > I was thinking on some function that would iterate over all fences in
>> > > > the fence_array and check their context. The if we find our own gpu
>> > > > context in there we fail the submit.
>> > >
>> > > Why would we have to fail if somebody feeds us our own fences? Wouldn't
>> > > it be enough to just wait if there are foreign fences in the array?
>> >
>> > Yes, skipping the wait if all fences are from our own context is an
>> > optimization and it's certainly not an issue if someone feeds us our own
>> > fences.
>>
>> Are you sure about that - what if we have two GPUs, a 2D and 3D GPU,
>> and we're fed an etnaviv fence for one GPU when submitting to the
>> other GPU.
>>
>> So we do end up being fed our own fences, and we have to respect them
>> otherwise we lose inter-GPU synchronisation, and that will break
>> existing userspace.
>>
> The etnaviv GPUs, while being on the same DRM device, have distinct
> fence contexts. So the 3D GPU will consider a fence from the 2D GPU as
> foreign and properly wait on it.
>
> It's only when we get an in fence that has been generated as an out
> fence by one (or multiple) submits to the same GPU, that we are able to
> skip the wait and enqueue the command without waiting for the fence to
> signal.

With regard to the 2D and 3D GPU case, it seems to me that a good
example use case would be where the 3D GPU is used in Android for all
the surface generation using OpenGL and then the 2D GPU would get used
to composite all those surfaces together, leaving the 3D open to work
on other stuff.  As I understand it, the 2D GPU is much faster at 2D
compositing than the 3D GPU would be, (not to mention less power
hungry.)

>
> Regards,
> Lucas
>
> _______________________________________________
> etnaviv mailing list
> etnaviv@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/etnaviv
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/etnaviv: submit support for in-fences

[Christian König]

Am 17.03.2017 um 15:58 schrieb Lucas Stach:
> Am Freitag, den 17.03.2017, 14:42 +0000 schrieb Russell King - ARM
> Linux:
>> On Fri, Mar 17, 2017 at 03:10:21PM +0100, Lucas Stach wrote:
>>> Am Donnerstag, den 16.03.2017, 12:05 +0100 schrieb Philipp Zabel:
>>>> Hi Gustavo,
>>>>
>>>> On Mon, 2017-03-13 at 14:37 -0300, Gustavo Padovan wrote:
>>>> [...]
>>>>> I was thinking on some function that would iterate over all fences in
>>>>> the fence_array and check their context. The if we find our own gpu
>>>>> context in there we fail the submit.
>>>> Why would we have to fail if somebody feeds us our own fences? Wouldn't
>>>> it be enough to just wait if there are foreign fences in the array?
>>> Yes, skipping the wait if all fences are from our own context is an
>>> optimization and it's certainly not an issue if someone feeds us our own
>>> fences.
>> Are you sure about that - what if we have two GPUs, a 2D and 3D GPU,
>> and we're fed an etnaviv fence for one GPU when submitting to the
>> other GPU.
>>
>> So we do end up being fed our own fences, and we have to respect them
>> otherwise we lose inter-GPU synchronisation, and that will break
>> existing userspace.
>>
> The etnaviv GPUs, while being on the same DRM device, have distinct
> fence contexts. So the 3D GPU will consider a fence from the 2D GPU as
> foreign and properly wait on it.
>
> It's only when we get an in fence that has been generated as an out
> fence by one (or multiple) submits to the same GPU, that we are able to
> skip the wait and enqueue the command without waiting for the fence to
> signal.

BTW: Do you still have the needs for a GPU scheduler?

The scheduler amdgpu uses is hopefully still hardware agnostic and has 
all that handling already included.

Using it can avoid blocking for foreign fences during your command 
submission and I won't mind seeing that moved into common drm code.

Regards,
Christian.

>
> Regards,
> Lucas
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


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

Re: [PATCH 1/3] drm/etnaviv: submit support for in-fences

[Lucas Stach]

Am Samstag, den 18.03.2017, 15:19 +0100 schrieb Christian König:
> Am 17.03.2017 um 15:58 schrieb Lucas Stach:
> > Am Freitag, den 17.03.2017, 14:42 +0000 schrieb Russell King - ARM
> > Linux:
> > > On Fri, Mar 17, 2017 at 03:10:21PM +0100, Lucas Stach wrote:
> > > > Am Donnerstag, den 16.03.2017, 12:05 +0100 schrieb Philipp
> > > > Zabel:
> > > > > Hi Gustavo,
> > > > > 
> > > > > On Mon, 2017-03-13 at 14:37 -0300, Gustavo Padovan wrote:
> > > > > [...]
> > > > > > I was thinking on some function that would iterate over all
> > > > > > fences in
> > > > > > the fence_array and check their context. The if we find our
> > > > > > own gpu
> > > > > > context in there we fail the submit.
> > > > > 
> > > > > Why would we have to fail if somebody feeds us our own
> > > > > fences? Wouldn't
> > > > > it be enough to just wait if there are foreign fences in the
> > > > > array?
> > > > 
> > > > Yes, skipping the wait if all fences are from our own context
> > > > is an
> > > > optimization and it's certainly not an issue if someone feeds
> > > > us our own
> > > > fences.
> > > 
> > > Are you sure about that - what if we have two GPUs, a 2D and 3D
> > > GPU,
> > > and we're fed an etnaviv fence for one GPU when submitting to the
> > > other GPU.
> > > 
> > > So we do end up being fed our own fences, and we have to respect
> > > them
> > > otherwise we lose inter-GPU synchronisation, and that will break
> > > existing userspace.
> > > 
> > 
> > The etnaviv GPUs, while being on the same DRM device, have distinct
> > fence contexts. So the 3D GPU will consider a fence from the 2D GPU
> > as
> > foreign and properly wait on it.
> > 
> > It's only when we get an in fence that has been generated as an out
> > fence by one (or multiple) submits to the same GPU, that we are
> > able to
> > skip the wait and enqueue the command without waiting for the fence
> > to
> > signal.
> 
> BTW: Do you still have the needs for a GPU scheduler?
> 
> The scheduler amdgpu uses is hopefully still hardware agnostic and
> has 
> all that handling already included.
> 
> Using it can avoid blocking for foreign fences during your command 
> submission and I won't mind seeing that moved into common drm code.

Yes, it's still on my list of features to enable for etnaviv. It's just
 that other things like enabling more hardware and getting performance
up had priority over this.

I've looked at the amdgpu scheduler and I agree that it's probably the
right thing to move this out into common code and make use of it in
etnaviv.

Regards,
Lucas
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/etnaviv: submit support for in-fences

[Daniel Vetter]

On Fri, Mar 17, 2017 at 11:00:44AM -0300, Gustavo Padovan wrote:
> 2017-03-16 Philipp Zabel <p.zabel@pengutronix.de>:
> 
> > Hi Gustavo,
> > 
> > On Mon, 2017-03-13 at 14:37 -0300, Gustavo Padovan wrote:
> > [...]
> > > I was thinking on some function that would iterate over all fences in
> > > the fence_array and check their context. The if we find our own gpu
> > > context in there we fail the submit.
> > 
> > Why would we have to fail if somebody feeds us our own fences? Wouldn't
> > it be enough to just wait if there are foreign fences in the array?
> 
> You don't need to fail necessarily. In my mind I had the use case that
> maybe some driver could deadlock waiting for his own fence. What you
> do with the information that the array has it a fence with the driver's
> context is entirely up to the driver to decide.

With the current infrastructure you can't have future fences (i.e. a fence
for something that might happen somewhen in the future, but for which no
driver is yet committed to execute - i.e. it's not yet queued). And
without future fences you can't ever have deadlocks.

The "are these all my own fences" check is more useful just to import the
fences into your own internal objects and use your driver-internal depency
handling (which usually means, more hw, less cpu waiting). But that's
entirely orthogonal to "can we deadlock", which atm is "no"[1].
-Daniel

1: You can deadlock when e.g. one driver holds a lock while waiting for a
fence, and another driver needs that lock before it is willing to signal
said fence. But that's not any different from waiting for anything else in
the kernel, and will be checked by the cross-release lockdep stuff rsn.

> 
> > 
> > How about something like this:
> > 
> > ----------8<----------
> > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> > index 364fe50d020de..0b0bdaf4406d4 100644
> > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> > @@ -296,6 +296,22 @@ static void submit_cleanup(struct etnaviv_gem_submit *submit)
> >  	kfree(submit);
> >  }
> >  
> > +static bool dma_fence_all_in_context(struct dma_fence *fence, u64 context)
> > +{
> > +	if (dma_fence_is_array(fence)) {
> > +		struct dma_fence_array *array = to_dma_fence_array(fence);
> > +		int i;
> > +
> > +		for (i = 0; i < array->num_fences; i++) {
> > +			if (array->fences[i]->context != context)
> > +				return false;
> > +		}
> > +		return true;
> > +	}
> > +
> > +	return fence->context == context;
> > +}
> 
> If we don't mind having fences with our own context, why should we check
> them?
> 
> Gustavo
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel