Re: [PATCH v5 07/21] gpu: host1x: Introduce UAPI header

[Mikko Perttunen <cyndis@kapsi.fi>]

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@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
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel