Re: [PATCH v3 01/14] drm/panthor: Add uAPI
On Mon, 18 Dec 2023 13:20:07 + Chris Diamand wrote: > > +/** > > + * struct drm_panthor_group_submit - Arguments passed to > > DRM_IOCTL_PANTHOR_VM_BIND > > + */ > > +struct drm_panthor_group_submit { > > + /** @group_handle: Handle of the group to queue jobs to. */ > > + __u32 group_handle; > > + > > + /** @pad: MBZ. */ > > + __u32 pad; > > + > > + /** @queue_submits: Array of drm_panthor_queue_submit objects. */ > > + struct drm_panthor_obj_array queue_submits; > > +}; > > > Hi! Very minor nit - but shouldn't the comment above say > DRM_IOCTL_PANTHOR_GROUP_SUBMIT, not VM_BIND? Certainly, I'll fix it in v4.
Re: [PATCH v3 01/14] drm/panthor: Add uAPI
> diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h > new file mode 100644 > index ..6d815df5e829 > --- /dev/null > +++ b/include/uapi/drm/panthor_drm.h > @@ -0,0 +1,892 @@ > +/* SPDX-License-Identifier: MIT */ > +/* Copyright (C) 2023 Collabora ltd. */ > +#ifndef _PANTHOR_DRM_H_ > +#define _PANTHOR_DRM_H_ > + > +#include "drm.h" > + > +#if defined(__cplusplus) > +extern "C" { > +#endif > + > +/** > + * DOC: Introduction ... > +/** > + * struct drm_panthor_group_submit - Arguments passed to > DRM_IOCTL_PANTHOR_VM_BIND > + */ > +struct drm_panthor_group_submit { > + /** @group_handle: Handle of the group to queue jobs to. */ > + __u32 group_handle; > + > + /** @pad: MBZ. */ > + __u32 pad; > + > + /** @queue_submits: Array of drm_panthor_queue_submit objects. */ > + struct drm_panthor_obj_array queue_submits; > +}; Hi! Very minor nit - but shouldn't the comment above say DRM_IOCTL_PANTHOR_GROUP_SUBMIT, not VM_BIND? > + > +/** > + * enum drm_panthor_group_state_flags - Group state flags > + */ > +enum drm_panthor_group_state_flags { > + /** > + * @DRM_PANTHOR_GROUP_STATE_TIMEDOUT: Group had unfinished jobs. > + * > + * When a group ends up with this flag set, no jobs can be submitted to > its queues. > + */ > + DRM_PANTHOR_GROUP_STATE_TIMEDOUT = 1 << 0, > + > + /** > + * @DRM_PANTHOR_GROUP_STATE_FATAL_FAULT: Group had fatal faults. > + * > + * When a group ends up with this flag set, no jobs can be submitted to > its queues. > + */ > + DRM_PANTHOR_GROUP_STATE_FATAL_FAULT = 1 << 1, > +}; > + > +/** > + * struct drm_panthor_group_get_state - Arguments passed to > DRM_IOCTL_PANTHOR_GROUP_GET_STATE > + * > + * Used to query the state of a group and decide whether a new group should > be created to > + * replace it. > + */ > +struct drm_panthor_group_get_state { > + /** @group_handle: Handle of the group to query state on */ > + __u32 group_handle; > + > + /** > + * @state: Combination of DRM_PANTHOR_GROUP_STATE_* flags encoding the > + * group state. > + */ > + __u32 state; > + > + /** @fatal_queues: Bitmask of queues that faced fatal faults. */ > + __u32 fatal_queues; > + > + /** @pad: MBZ */ > + __u32 pad; > +}; > + > +/** > + * struct drm_panthor_tiler_heap_create - Arguments passed to > DRM_IOCTL_PANTHOR_TILER_HEAP_CREATE > + */ > +struct drm_panthor_tiler_heap_create { > + /** @vm_id: VM ID the tiler heap should be mapped to */ > + __u32 vm_id; > + > + /** @initial_chunk_count: Initial number of chunks to allocate. */ > + __u32 initial_chunk_count; > + > + /** @chunk_size: Chunk size. Must be a power of two at least 256KB > large. */ > + __u32 chunk_size; > + > + /** @max_chunks: Maximum number of chunks that can be allocated. */ > + __u32 max_chunks; > + > + /** > + * @target_in_flight: Maximum number of in-flight render passes. > + * > + * If the heap has more than tiler jobs in-flight, the FW will wait for > render > + * passes to finish before queuing new tiler jobs. > + */ > + __u32 target_in_flight; > + > + /** @handle: Returned heap handle. Passed back to DESTROY_TILER_HEAP. */ > + __u32 handle; > + > + /** @tiler_heap_ctx_gpu_va: Returned heap GPU virtual address returned > */ > + __u64 tiler_heap_ctx_gpu_va; > + > + /** > + * @first_heap_chunk_gpu_va: First heap chunk. > + * > + * The tiler heap is formed of heap chunks forming a single-link list. > This > + * is the first element in the list. > + */ > + __u64 first_heap_chunk_gpu_va; > +}; > + > +/** > + * struct drm_panthor_tiler_heap_destroy - Arguments passed to > DRM_IOCTL_PANTHOR_TILER_HEAP_DESTROY > + */ > +struct drm_panthor_tiler_heap_destroy { > + /** @handle: Handle of the tiler heap to destroy */ > + __u32 handle; > + > + /** @pad: Padding field, MBZ. */ > + __u32 pad; > +}; > + > +#if defined(__cplusplus) > +} > +#endif > + > +#endif /* _PANTHOR_DRM_H_ */ Cheers, Chris
Re: [PATCH v3 01/14] drm/panthor: Add uAPI
On 04/12/2023 17:32, Boris Brezillon wrote: > Panthor follows the lead of other recently submitted drivers with > ioctls allowing us to support modern Vulkan features, like sparse memory > binding: > > - Pretty standard GEM management ioctls (BO_CREATE and BO_MMAP_OFFSET), > with the 'exclusive-VM' bit to speed-up BO reservation on job submission > - VM management ioctls (VM_CREATE, VM_DESTROY and VM_BIND). The VM_BIND > ioctl is loosely based on the Xe model, and can handle both > asynchronous and synchronous requests > - GPU execution context creation/destruction, tiler heap context creation > and job submission. Those ioctls reflect how the hardware/scheduler > works and are thus driver specific. > > We also have a way to expose IO regions, such that the usermode driver > can directly access specific/well-isolate registers, like the > LATEST_FLUSH register used to implement cache-flush reduction. > > This uAPI intentionally keeps usermode queues out of the scope, which > explains why doorbell registers and command stream ring-buffers are not > directly exposed to userspace. > > v3: > - Add the concept of sync-only VM operation > - Fix support for 32-bit userspace > - Rework drm_panthor_vm_create to pass the user VA size instead of > the kernel VA size (suggested by Robin Murphy) > - Typo fixes > - Explicitly cast enums with top bit set to avoid compiler warnings in > -pedantic mode. > - Drop property core_group_count as it can be easily calculated by the > number of bits set in l2_present. > > Signed-off-by: Boris Brezillon > Signed-off-by: Steven Price NIT: I believe the signed-off lines should be like: Co-developed-by: Steven Signed-off-by: Steven Signed-off-by: Boris So yours at the end not mine (because you're the one posting it) and a "Co-developed-by" to explain why there's a sign-off from me. I'm guessing this is actually my fault since I sent the branch to you with my changes with the sign offs like this. Otherwise everything looks good: Reviewed-by: Steven Price Thanks, Steve > --- > Documentation/gpu/driver-uapi.rst | 5 + > include/uapi/drm/panthor_drm.h| 892 ++ > 2 files changed, 897 insertions(+) > create mode 100644 include/uapi/drm/panthor_drm.h > > diff --git a/Documentation/gpu/driver-uapi.rst > b/Documentation/gpu/driver-uapi.rst > index c08bcbb95fb3..7a667901830f 100644 > --- a/Documentation/gpu/driver-uapi.rst > +++ b/Documentation/gpu/driver-uapi.rst > @@ -17,3 +17,8 @@ VM_BIND / EXEC uAPI > :doc: Overview > > .. kernel-doc:: include/uapi/drm/nouveau_drm.h > + > +drm/panthor uAPI > + > + > +.. kernel-doc:: include/uapi/drm/panthor_drm.h > diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h > new file mode 100644 > index ..6d815df5e829 > --- /dev/null > +++ b/include/uapi/drm/panthor_drm.h > @@ -0,0 +1,892 @@ > +/* SPDX-License-Identifier: MIT */ > +/* Copyright (C) 2023 Collabora ltd. */ > +#ifndef _PANTHOR_DRM_H_ > +#define _PANTHOR_DRM_H_ > + > +#include "drm.h" > + > +#if defined(__cplusplus) > +extern "C" { > +#endif > + > +/** > + * DOC: Introduction > + * > + * This documentation describes the Panthor IOCTLs. > + * > + * Just a few generic rules about the data passed to the Panthor IOCTLs: > + * > + * - Structures must be aligned on 64-bit/8-byte. If the object is not > + * naturally aligned, a padding field must be added. > + * - Fields must be explicitly aligned to their natural type alignment with > + * pad[0..N] fields. > + * - All padding fields will be checked by the driver to make sure they are > + * zeroed. > + * - Flags can be added, but not removed/replaced. > + * - New fields can be added to the main structures (the structures > + * directly passed to the ioctl). Those fields can be added at the end of > + * the structure, or replace existing padding fields. Any new field being > + * added must preserve the behavior that existed before those fields were > + * added when a value of zero is passed. > + * - New fields can be added to indirect objects (objects pointed by the > + * main structure), iff those objects are passed a size to reflect the > + * size known by the userspace driver (see drm_panthor_obj_array::stride > + * or drm_panthor_dev_query::size). > + * - If the kernel driver is too old to know some fields, those will be > + * ignored if zero, and otherwise rejected (and so will be zero on output). > + * - If userspace is too old to know some fields, those will be zeroed > + * (input) before the structure is parsed by the kernel driver. > + * - Each new flag/field addition must come with a driver version update so > + * the userspace driver doesn't have to trial and error to know which > + * flags are supported. > + * - Structures should not contain unions, as this would defeat the > + * extensibility of such structures. > + * - IOCTLs can't be removed or replaced. New IOCTL IDs should be
[PATCH v3 01/14] drm/panthor: Add uAPI
Panthor follows the lead of other recently submitted drivers with ioctls allowing us to support modern Vulkan features, like sparse memory binding: - Pretty standard GEM management ioctls (BO_CREATE and BO_MMAP_OFFSET), with the 'exclusive-VM' bit to speed-up BO reservation on job submission - VM management ioctls (VM_CREATE, VM_DESTROY and VM_BIND). The VM_BIND ioctl is loosely based on the Xe model, and can handle both asynchronous and synchronous requests - GPU execution context creation/destruction, tiler heap context creation and job submission. Those ioctls reflect how the hardware/scheduler works and are thus driver specific. We also have a way to expose IO regions, such that the usermode driver can directly access specific/well-isolate registers, like the LATEST_FLUSH register used to implement cache-flush reduction. This uAPI intentionally keeps usermode queues out of the scope, which explains why doorbell registers and command stream ring-buffers are not directly exposed to userspace. v3: - Add the concept of sync-only VM operation - Fix support for 32-bit userspace - Rework drm_panthor_vm_create to pass the user VA size instead of the kernel VA size (suggested by Robin Murphy) - Typo fixes - Explicitly cast enums with top bit set to avoid compiler warnings in -pedantic mode. - Drop property core_group_count as it can be easily calculated by the number of bits set in l2_present. Signed-off-by: Boris Brezillon Signed-off-by: Steven Price --- Documentation/gpu/driver-uapi.rst | 5 + include/uapi/drm/panthor_drm.h| 892 ++ 2 files changed, 897 insertions(+) create mode 100644 include/uapi/drm/panthor_drm.h diff --git a/Documentation/gpu/driver-uapi.rst b/Documentation/gpu/driver-uapi.rst index c08bcbb95fb3..7a667901830f 100644 --- a/Documentation/gpu/driver-uapi.rst +++ b/Documentation/gpu/driver-uapi.rst @@ -17,3 +17,8 @@ VM_BIND / EXEC uAPI :doc: Overview .. kernel-doc:: include/uapi/drm/nouveau_drm.h + +drm/panthor uAPI + + +.. kernel-doc:: include/uapi/drm/panthor_drm.h diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h new file mode 100644 index ..6d815df5e829 --- /dev/null +++ b/include/uapi/drm/panthor_drm.h @@ -0,0 +1,892 @@ +/* SPDX-License-Identifier: MIT */ +/* Copyright (C) 2023 Collabora ltd. */ +#ifndef _PANTHOR_DRM_H_ +#define _PANTHOR_DRM_H_ + +#include "drm.h" + +#if defined(__cplusplus) +extern "C" { +#endif + +/** + * DOC: Introduction + * + * This documentation describes the Panthor IOCTLs. + * + * Just a few generic rules about the data passed to the Panthor IOCTLs: + * + * - Structures must be aligned on 64-bit/8-byte. If the object is not + * naturally aligned, a padding field must be added. + * - Fields must be explicitly aligned to their natural type alignment with + * pad[0..N] fields. + * - All padding fields will be checked by the driver to make sure they are + * zeroed. + * - Flags can be added, but not removed/replaced. + * - New fields can be added to the main structures (the structures + * directly passed to the ioctl). Those fields can be added at the end of + * the structure, or replace existing padding fields. Any new field being + * added must preserve the behavior that existed before those fields were + * added when a value of zero is passed. + * - New fields can be added to indirect objects (objects pointed by the + * main structure), iff those objects are passed a size to reflect the + * size known by the userspace driver (see drm_panthor_obj_array::stride + * or drm_panthor_dev_query::size). + * - If the kernel driver is too old to know some fields, those will be + * ignored if zero, and otherwise rejected (and so will be zero on output). + * - If userspace is too old to know some fields, those will be zeroed + * (input) before the structure is parsed by the kernel driver. + * - Each new flag/field addition must come with a driver version update so + * the userspace driver doesn't have to trial and error to know which + * flags are supported. + * - Structures should not contain unions, as this would defeat the + * extensibility of such structures. + * - IOCTLs can't be removed or replaced. New IOCTL IDs should be placed + * at the end of the drm_panthor_ioctl_id enum. + */ + +/** + * DOC: MMIO regions exposed to userspace. + * + * .. c:macro:: DRM_PANTHOR_USER_MMIO_OFFSET + * + * File offset for all MMIO regions being exposed to userspace. Don't use + * this value directly, use DRM_PANTHOR_USER__OFFSET values instead. + * pgoffset passed to mmap2() is an unsigned long, which forces us to use a + * different offset on 32-bit and 64-bit systems. + * + * .. c:macro:: DRM_PANTHOR_USER_FLUSH_ID_MMIO_OFFSET + * + * File offset for the LATEST_FLUSH_ID register. The Userspace driver controls + * GPU cache flushing through CS instructions, but the flush reduction + * mechanism requires a flush_id. This flush_id