Re: [PATCH v5 09/21] gpu: host1x: DMA fences and userspace fence creation

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 7525 bytes --]

On Mon, Jan 11, 2021 at 03:00:07PM +0200, Mikko Perttunen wrote:
> 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.
> 
> Additionally, add a new /dev/host1x IOCTL for creating sync_file
> file descriptors backed by syncpoint fences.
> 
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
> 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  | 208 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/host1x/fence.h  |  13 +++
>  drivers/gpu/host1x/intr.c   |   9 ++
>  drivers/gpu/host1x/intr.h   |   2 +
>  drivers/gpu/host1x/uapi.c   | 103 ++++++++++++++++++
>  include/linux/host1x.h      |   4 +
>  7 files changed, 340 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 882f928d75e1..a48af2cefae1 100644
> --- a/drivers/gpu/host1x/Makefile
> +++ b/drivers/gpu/host1x/Makefile
> @@ -10,6 +10,7 @@ host1x-y = \
>  	debug.o \
>  	mipi.o \
>  	uapi.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 000000000000..e96ad93ff656
> --- /dev/null
> +++ b/drivers/gpu/host1x/fence.c
> @@ -0,0 +1,208 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Syncpoint dma_fence implementation
> + *
> + * Copyright (c) 2020, NVIDIA Corporation.
> + */
> +
> +#include <linux/dma-fence.h>
> +#include <linux/file.h>
> +#include <linux/fs.h>
> +#include <linux/slab.h>
> +#include <linux/sync_file.h>
> +
> +#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 *syncpt_fence_get_driver_name(struct dma_fence *f)
> +{
> +	return "host1x";
> +}
> +
> +static const char *syncpt_fence_get_timeline_name(struct dma_fence *f)
> +{
> +	return "syncpoint";
> +}
> +
> +static bool syncpt_fence_enable_signaling(struct dma_fence *f)
> +{
> +	struct host1x_syncpt_fence *sf =
> +		container_of(f, struct host1x_syncpt_fence, base);

Maybe add a casting helper to make this less annoying.

> +const struct dma_fence_ops syncpt_fence_ops = {

I'd prefer this to use the host1x_syncpt_ prefix for better scoping.

> +	.get_driver_name = syncpt_fence_get_driver_name,
> +	.get_timeline_name = syncpt_fence_get_timeline_name,
> +	.enable_signaling = syncpt_fence_enable_signaling,
> +	.release = syncpt_fence_release,

Maybe also do that for these, while at it.

> +static int dev_file_ioctl_create_fence(struct host1x *host1x, void __user *data)
> +{
> +	struct host1x_create_fence args;
> +	unsigned long copy_err;

Any reason why this needs to have that cumbersome copy_ prefix? There's
no other "err" variables, so why not just use the shorter "err" for
this?

> +	int fd;
> +
> +	copy_err = copy_from_user(&args, data, sizeof(args));
> +	if (copy_err)
> +		return -EFAULT;
> +
> +	if (args.reserved[0])
> +		return -EINVAL;
> +
> +	if (args.id >= host1x_syncpt_nb_pts(host1x))
> +		return -EINVAL;
> +
> +	args.id = array_index_nospec(args.id, host1x_syncpt_nb_pts(host1x));
> +
> +	fd = host1x_fence_create_fd(&host1x->syncpt[args.id], args.threshold);
> +	if (fd < 0)
> +		return fd;
> +
> +	args.fence_fd = fd;
> +
> +	copy_err = copy_to_user(data, &args, sizeof(args));
> +	if (copy_err)
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +static int dev_file_ioctl_fence_extract(struct host1x *host1x, void __user *data)
> +{
> +	struct host1x_fence_extract_fence __user *fences_user_ptr;
> +	struct dma_fence *fence, **fences;
> +	struct host1x_fence_extract args;
> +	struct dma_fence_array *array;
> +	unsigned int num_fences, i;
> +	unsigned long copy_err;

Can't do the same here, but perhaps just do what other copy_from_user()
callsites do and just use it directly in the conditional so you don't
even need to store the return value since you're not reusing it anyway.

In fact you could do the same thing above and just get rid of that
variable and render the code more idiomatic.

> +	int err;
> +
> +	copy_err = copy_from_user(&args, data, sizeof(args));
> +	if (copy_err)
> +		return -EFAULT;
> +
> +	fences_user_ptr = u64_to_user_ptr(args.fences_ptr);
> +
> +	if (args.reserved[0] || args.reserved[1])
> +		return -EINVAL;
> +
> +	fence = sync_file_get_fence(args.fence_fd);
> +	if (!fence)
> +		return -EINVAL;
> +
> +	array = to_dma_fence_array(fence);
> +	if (array) {
> +		fences = array->fences;
> +		num_fences = array->num_fences;
> +	} else {
> +		fences = &fence;
> +		num_fences = 1;
> +	}
> +
> +	for (i = 0; i < min(num_fences, args.num_fences); i++) {
> +		struct host1x_fence_extract_fence f;
> +
> +		err = host1x_fence_extract(fences[i], &f.id, &f.threshold);
> +		if (err)
> +			goto put_fence;
> +
> +		copy_err = copy_to_user(fences_user_ptr + i, &f, sizeof(f));
> +		if (copy_err) {
> +			err = -EFAULT;
> +			goto put_fence;
> +		}
> +	}
> +
> +	args.num_fences = i+1;

checkpatch will probably complain about this not having spaces around
that '+'.

> +
> +	copy_err = copy_to_user(data, &args, sizeof(args));
> +	if (copy_err) {
> +		err = -EFAULT;
> +		goto put_fence;
> +	}
> +
> +	return 0;
> +
> +put_fence:
> +	dma_fence_put(fence);
> +
> +	return err;
> +}
> +
>  static long dev_file_ioctl(struct file *file, unsigned int cmd,
>  			   unsigned long arg)
>  {
> @@ -210,6 +305,14 @@ static long dev_file_ioctl(struct file *file, unsigned int cmd,
>  		err = dev_file_ioctl_alloc_syncpoint(file->private_data, data);
>  		break;
>  
> +	case HOST1X_IOCTL_CREATE_FENCE:
> +		err = dev_file_ioctl_create_fence(file->private_data, data);
> +		break;
> +
> +	case HOST1X_IOCTL_FENCE_EXTRACT:
> +		err = dev_file_ioctl_fence_extract(file->private_data, data);
> +		break;
> +
>  	default:
>  		err = -ENOTTY;
>  	}
> diff --git a/include/linux/host1x.h b/include/linux/host1x.h
> index b3178ae51cae..080f9d3d29eb 100644
> --- a/include/linux/host1x.h
> +++ b/include/linux/host1x.h
> @@ -165,6 +165,10 @@ u32 host1x_syncpt_base_id(struct host1x_syncpt_base *base);
>  
>  struct host1x_syncpt *host1x_syncpt_fd_get(int fd);
>  
> +struct dma_fence *host1x_fence_create(struct host1x_syncpt *sp, u32 threshold);
> +int host1x_fence_create_fd(struct host1x_syncpt *sp, u32 threshold);
> +int host1x_fence_extract(struct dma_fence *fence, u32 *id, u32 *threshold);

Do we need these outside of the IOCTL implementations?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]