Re: [PATCH v3 01/14] drm/panthor: Add uAPI

2024-01-15 Thread Boris Brezillon
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

2023-12-18 Thread Chris Diamand
> 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

2023-12-06 Thread Steven Price
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