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

[Gustavo Padovan <gustavo@padovan.org>]

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