[PATCH v5 07/21] gpu: host1x: Introduce UAPI header
Mikko Perttunen
cyndis at kapsi.fi
Tue Mar 23 11:12:36 UTC 2021
On 3/23/21 12:52 PM, Thierry Reding wrote:
> 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 at nvidia.com>
>> ---
>> 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.
>
Correct.
>> +
>> +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...
Yep.
>
>> +
>> +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.
Yeah. The model is that reading any syncpoint is OK but writing is not.
I think these things were mentioned in the original proposal text but I
did not carry them over to the comments. Will fix (however see below)
>
>> +
>> +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?
This is needed to process incoming prefences for userspace-programmed
engines -- mainly, the GPU with usermode submit enabled.
To align with other upstream code, I've been thinking of removing this
whole UAPI; moving the syncpoint allocation part to the DRM UAPI, and
dropping the sync_file stuff altogether (if we have support for job
submission outputting syncobjs, those could still be converted into
sync_files). This doesn't support usecases like GPU usermode submit, so
for downstream we'll have to add it back in, though. Would like to hear
your opinion on it as well.
Mikko
>
> 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
>>
More information about the dri-devel
mailing list