On Mon, Jan 11, 2021 at 03:00:05PM +0200, Mikko Perttunen wrote:
> Add the userspace interface header, specifying interfaces
> for allocating and accessing syncpoints from userspace,
> and for creating sync_file based fences based on syncpoint
> thresholds.
>
> Signed-off-by: Mikko Perttunen <mperttunen@xxxxxxxxxx>
> ---
> include/uapi/linux/host1x.h | 134 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 134 insertions(+)
> create mode 100644 include/uapi/linux/host1x.h
What's the number of these syncpoints that we expect userspace to
create? There's a limited amount of open file descriptors available by
default, so this needs to be kept reasonably low.
> diff --git a/include/uapi/linux/host1x.h b/include/uapi/linux/host1x.h
> new file mode 100644
> index 000000000000..9c8fb9425cb2
> --- /dev/null
> +++ b/include/uapi/linux/host1x.h
> @@ -0,0 +1,134 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/* Copyright (c) 2020 NVIDIA Corporation */
> +
> +#ifndef _UAPI__LINUX_HOST1X_H
> +#define _UAPI__LINUX_HOST1X_H
> +
> +#include <linux/ioctl.h>
> +#include <linux/types.h>
> +
> +#if defined(__cplusplus)
> +extern "C" {
> +#endif
> +
> +struct host1x_allocate_syncpoint {
> + /**
> + * @fd: [out]
> + *
> + * New file descriptor representing the allocated syncpoint.
> + */
> + __s32 fd;
> +
> + __u32 reserved[3];
> +};
> +
> +struct host1x_syncpoint_info {
> + /**
> + * @id: [out]
> + *
> + * System-global ID of the syncpoint.
> + */
> + __u32 id;
> +
> + __u32 reserved[3];
> +};
Given that this has only out parameters, I expect this will be called on
the FD returned by HOST1X_IOCTL_ALLOCATE_SYNCPOINT? It might be worth
pointing that out explicitly in a comment.
> +
> +struct host1x_syncpoint_increment {
> + /**
> + * @count: [in]
> + *
> + * Number of times to increment the syncpoint. The syncpoint can
> + * be observed at in-between values, but each increment is atomic.
> + */
> + __u32 count;
> +};
This seems like it would have to be called on the FD as well...
> +
> +struct host1x_read_syncpoint {
> + /**
> + * @id: [in]
> + *
> + * ID of the syncpoint to read.
> + */
> + __u32 id;
> +
> + /**
> + * @value: [out]
> + *
> + * Current value of the syncpoint.
> + */
> + __u32 value;
> +};
... but then, all of a sudden you seem to switch things around and allow
reading the value of an arbitrary syncpoint specified by ID.
Now, I suspect that's because reading the syncpoint is harmless and does
not allow abuse, whereas incrementing could be abused if allowed on an
arbitrary syncpoint ID. But I think it's worth spelling all that out in
some documentation to make this clear from a security point of view and
from a usability point of view for people trying to figure out how to
use these interfaces.
> +
> +struct host1x_create_fence {
> + /**
> + * @id: [in]
> + *
> + * ID of the syncpoint to create a fence for.
> + */
> + __u32 id;
> +
> + /**
> + * @threshold: [in]
> + *
> + * When the syncpoint reaches this value, the fence will be signaled.
> + * The syncpoint is considered to have reached the threshold when the
> + * following condition is true:
> + *
> + * ((value - threshold) & 0x80000000U) == 0U
> + *
> + */
> + __u32 threshold;
> +
> + /**
> + * @fence_fd: [out]
> + *
> + * New sync_file file descriptor containing the created fence.
> + */
> + __s32 fence_fd;
> +
> + __u32 reserved[1];
> +};
Again this takes an arbitrary syncpoint ID as input, so I expect that
the corresponding IOCTL will have to be called on the host1x device
node? Again, I think it would be good to either point that out for each
structure or IOCTL, or alternatively maybe reorder these such that this
becomes clearer.
> +
> +struct host1x_fence_extract_fence {
> + __u32 id;
> + __u32 threshold;
> +};
> +
> +struct host1x_fence_extract {
> + /**
> + * @fence_fd: [in]
> + *
> + * sync_file file descriptor
> + */
> + __s32 fence_fd;
> +
> + /**
> + * @num_fences: [in,out]
> + *
> + * In: size of the `fences_ptr` array counted in elements.
> + * Out: required size of the `fences_ptr` array counted in elements.
> + */
> + __u32 num_fences;
> +
> + /**
> + * @fences_ptr: [in]
> + *
> + * Pointer to array of `struct host1x_fence_extract_fence`.
> + */
> + __u64 fences_ptr;
> +
> + __u32 reserved[2];
> +};
For the others it's pretty clear to me what the purpose is, but I'm at a
complete loss with this one. What's the use-case for this?
In general I think it'd make sense to add a bit more documentation about
how all these IOCTLs are meant to be used to give people a better
understanding of why these are needed.
Thierry
> +
> +#define HOST1X_IOCTL_ALLOCATE_SYNCPOINT _IOWR('X', 0x00, struct host1x_allocate_syncpoint)
> +#define HOST1X_IOCTL_READ_SYNCPOINT _IOR ('X', 0x01, struct host1x_read_syncpoint)
> +#define HOST1X_IOCTL_CREATE_FENCE _IOWR('X', 0x02, struct host1x_create_fence)
> +#define HOST1X_IOCTL_SYNCPOINT_INFO _IOWR('X', 0x03, struct host1x_syncpoint_info)
> +#define HOST1X_IOCTL_SYNCPOINT_INCREMENT _IOWR('X', 0x04, struct host1x_syncpoint_increment)
> +#define HOST1X_IOCTL_FENCE_EXTRACT _IOWR('X', 0x05, struct host1x_fence_extract)
> +
> +#if defined(__cplusplus)
> +}
> +#endif
> +
> +#endif
> --
> 2.30.0
>
Attachment:
signature.asc
Description: PGP signature
