diff mbox series

[RFC,v3,2/3] drm/i915: Update i915 uapi documentation

Message ID 20220517183212.20274-3-niranjana.vishwanathapura@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/doc/rfc: i915 VM_BIND feature design + uapi | expand

Commit Message

Niranjana Vishwanathapura May 17, 2022, 6:32 p.m. UTC
Add some missing i915 upai documentation which the new
i915 VM_BIND feature documentation will be refer to.

Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
---
 include/uapi/drm/i915_drm.h | 153 +++++++++++++++++++++++++++---------
 1 file changed, 116 insertions(+), 37 deletions(-)

Comments

Matthew Auld June 8, 2022, 11:24 a.m. UTC | #1
On Tue, 17 May 2022 at 19:32, Niranjana Vishwanathapura
<niranjana.vishwanathapura@intel.com> wrote:
>
> Add some missing i915 upai documentation which the new
> i915 VM_BIND feature documentation will be refer to.
>
> Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
> ---
>  include/uapi/drm/i915_drm.h | 153 +++++++++++++++++++++++++++---------
>  1 file changed, 116 insertions(+), 37 deletions(-)
>
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index a2def7b27009..8c834a31b56f 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -751,9 +751,16 @@ typedef struct drm_i915_irq_wait {
>
>  /* Must be kept compact -- no holes and well documented */
>
> +/**
> + * typedef drm_i915_getparam_t - Driver parameter query structure.

This one looks funny in the rendered html for some reason, since it
doesn't seem to emit the @param and @value, I guess it doesn't really
understand typedef <struct> ?

Maybe make this "struct drm_i915_getparam - Driver parameter query structure." ?

> + */
>  typedef struct drm_i915_getparam {
> +       /** @param: Driver parameter to query. */
>         __s32 param;
> -       /*
> +
> +       /**
> +        * @value: Address of memory where queried value should be put.
> +        *
>          * WARNING: Using pointers instead of fixed-size u64 means we need to write
>          * compat32 code. Don't repeat this mistake.
>          */
> @@ -1239,76 +1246,114 @@ struct drm_i915_gem_exec_object2 {
>         __u64 rsvd2;
>  };
>
> +/**
> + * struct drm_i915_gem_exec_fence - An input or output fence for the execbuff

s/execbuff/execbuf/, at least that seems to be what we use elsewhere, AFAICT.

> + * ioctl.
> + *
> + * The request will wait for input fence to signal before submission.
> + *
> + * The returned output fence will be signaled after the completion of the
> + * request.
> + */
>  struct drm_i915_gem_exec_fence {
> -       /**
> -        * User's handle for a drm_syncobj to wait on or signal.
> -        */
> +       /** @handle: User's handle for a drm_syncobj to wait on or signal. */
>         __u32 handle;
>
> +       /**
> +        * @flags: Supported flags are,

are:

> +        *
> +        * I915_EXEC_FENCE_WAIT:
> +        * Wait for the input fence before request submission.
> +        *
> +        * I915_EXEC_FENCE_SIGNAL:
> +        * Return request completion fence as output
> +        */
> +       __u32 flags;
>  #define I915_EXEC_FENCE_WAIT            (1<<0)
>  #define I915_EXEC_FENCE_SIGNAL          (1<<1)
>  #define __I915_EXEC_FENCE_UNKNOWN_FLAGS (-(I915_EXEC_FENCE_SIGNAL << 1))
> -       __u32 flags;
>  };
>
> -/*
> - * See drm_i915_gem_execbuffer_ext_timeline_fences.
> - */
> -#define DRM_I915_GEM_EXECBUFFER_EXT_TIMELINE_FENCES 0
> -
> -/*
> +/**
> + * struct drm_i915_gem_execbuffer_ext_timeline_fences - Timeline fences
> + * for execbuff.
> + *
>   * This structure describes an array of drm_syncobj and associated points for
>   * timeline variants of drm_syncobj. It is invalid to append this structure to
>   * the execbuf if I915_EXEC_FENCE_ARRAY is set.
>   */
>  struct drm_i915_gem_execbuffer_ext_timeline_fences {
> +#define DRM_I915_GEM_EXECBUFFER_EXT_TIMELINE_FENCES 0
> +       /** @base: Extension link. See struct i915_user_extension. */
>         struct i915_user_extension base;
>
>         /**
> -        * Number of element in the handles_ptr & value_ptr arrays.
> +        * @fence_count: Number of element in the @handles_ptr & @value_ptr

s/element/elements/

> +        * arrays.
>          */
>         __u64 fence_count;
>
>         /**
> -        * Pointer to an array of struct drm_i915_gem_exec_fence of length
> -        * fence_count.
> +        * @handles_ptr: Pointer to an array of struct drm_i915_gem_exec_fence
> +        * of length @fence_count.
>          */
>         __u64 handles_ptr;
>
>         /**
> -        * Pointer to an array of u64 values of length fence_count. Values
> -        * must be 0 for a binary drm_syncobj. A Value of 0 for a timeline
> -        * drm_syncobj is invalid as it turns a drm_syncobj into a binary one.
> +        * @values_ptr: Pointer to an array of u64 values of length
> +        * @fence_count.
> +        * Values must be 0 for a binary drm_syncobj. A Value of 0 for a
> +        * timeline drm_syncobj is invalid as it turns a drm_syncobj into a
> +        * binary one.
>          */
>         __u64 values_ptr;
>  };
>
> +/**
> + * struct drm_i915_gem_execbuffer2 - Structure for execbuff submission
> + */
>  struct drm_i915_gem_execbuffer2 {
> -       /**
> -        * List of gem_exec_object2 structs
> -        */
> +       /** @buffers_ptr: Pointer to a list of gem_exec_object2 structs */
>         __u64 buffers_ptr;
> +
> +       /** @buffer_count: Number of elements in @buffers_ptr array */
>         __u32 buffer_count;
>
> -       /** Offset in the batchbuffer to start execution from. */
> +       /**
> +        * @batch_start_offset: Offset in the batchbuffer to start execution
> +        * from.
> +        */
>         __u32 batch_start_offset;
> -       /** Bytes used in batchbuffer from batch_start_offset */
> +
> +       /** @batch_len: Bytes used in batchbuffer from batch_start_offset */

"Length in bytes of the batchbuffer, otherwise assumed to be the
object size if zero, starting from the @batch_start_offset."

>         __u32 batch_len;
> +
> +       /** @DR1: deprecated */
>         __u32 DR1;
> +
> +       /** @DR4: deprecated */
>         __u32 DR4;
> +
> +       /** @num_cliprects: See @cliprects_ptr */
>         __u32 num_cliprects;
> +
>         /**
> -        * This is a struct drm_clip_rect *cliprects if I915_EXEC_FENCE_ARRAY
> -        * & I915_EXEC_USE_EXTENSIONS are not set.
> +        * @cliprects_ptr: Kernel clipping was a DRI1 misfeature.
> +        *
> +        * It is invalid to use this field if I915_EXEC_FENCE_ARRAY or
> +        * I915_EXEC_USE_EXTENSIONS flags are not set.
>          *
>          * If I915_EXEC_FENCE_ARRAY is set, then this is a pointer to an array
> -        * of struct drm_i915_gem_exec_fence and num_cliprects is the length
> -        * of the array.
> +        * of &drm_i915_gem_exec_fence and @num_cliprects is the length of the
> +        * array.
>          *
>          * If I915_EXEC_USE_EXTENSIONS is set, then this is a pointer to a
> -        * single struct i915_user_extension and num_cliprects is 0.
> +        * single &i915_user_extension and num_cliprects is 0.
>          */
>         __u64 cliprects_ptr;
> +
> +       /** @flags: Execbuff flags */

s/Execbuff/Execbuf/

Could maybe document the I915_EXEC_* also, or maybe not ;)

> +       __u64 flags;
>  #define I915_EXEC_RING_MASK              (0x3f)
>  #define I915_EXEC_DEFAULT                (0<<0)
>  #define I915_EXEC_RENDER                 (1<<0)
> @@ -1326,10 +1371,6 @@ struct drm_i915_gem_execbuffer2 {
>  #define I915_EXEC_CONSTANTS_REL_GENERAL (0<<6) /* default */
>  #define I915_EXEC_CONSTANTS_ABSOLUTE   (1<<6)
>  #define I915_EXEC_CONSTANTS_REL_SURFACE (2<<6) /* gen4/5 only */
> -       __u64 flags;
> -       __u64 rsvd1; /* now used for context info */
> -       __u64 rsvd2;
> -};
>
>  /** Resets the SO write offset registers for transform feedback on gen7. */
>  #define I915_EXEC_GEN7_SOL_RESET       (1<<8)
> @@ -1432,9 +1473,23 @@ struct drm_i915_gem_execbuffer2 {
>   * drm_i915_gem_execbuffer_ext enum.
>   */
>  #define I915_EXEC_USE_EXTENSIONS       (1 << 21)
> -
>  #define __I915_EXEC_UNKNOWN_FLAGS (-(I915_EXEC_USE_EXTENSIONS << 1))
>
> +       /** @rsvd1: Context id */
> +       __u64 rsvd1;
> +
> +       /**
> +        * @rsvd2: in and out sync_file file descriptors.
> +        *
> +        * When I915_EXEC_FENCE_IN or I915_EXEC_FENCE_SUBMIT flag is set, the
> +        * lower 32 bits of this field will have the in sync_file fd (input).
> +        *
> +        * When I915_EXEC_FENCE_OUT flag is set, the upper 32 bits of this
> +        * field will have the out sync_file fd (output).
> +        */
> +       __u64 rsvd2;
> +};
> +
>  #define I915_EXEC_CONTEXT_ID_MASK      (0xffffffff)
>  #define i915_execbuffer2_set_context_id(eb2, context) \
>         (eb2).rsvd1 = context & I915_EXEC_CONTEXT_ID_MASK
> @@ -1814,13 +1869,32 @@ struct drm_i915_gem_context_create {
>         __u32 pad;
>  };
>
> +/**
> + * struct drm_i915_gem_context_create_ext - Structure for creating contexts.
> + */
>  struct drm_i915_gem_context_create_ext {
> -       __u32 ctx_id; /* output: id of new context*/
> +       /** @ctx_id: Id of the created context (output) */
> +       __u32 ctx_id;
> +
> +       /**
> +        * @flags: Supported flags are,

are:

> +        *
> +        * I915_CONTEXT_CREATE_FLAGS_USE_EXTENSIONS:
> +        *
> +        * Extensions may be appended to this structure and driver must check
> +        * for those.

Maybe add "See @extensions.", and then....

> +        *
> +        * I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE
> +        *
> +        * Created context will have single timeline.
> +        */
>         __u32 flags;
>  #define I915_CONTEXT_CREATE_FLAGS_USE_EXTENSIONS       (1u << 0)
>  #define I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE      (1u << 1)
>  #define I915_CONTEXT_CREATE_FLAGS_UNKNOWN \
>         (-(I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE << 1))
> +
> +       /** @extensions: Zero-terminated chain of extensions. */

...here perhaps list the extensions, and maybe also move the #define
for each here? See for example @extensions in drm_i915_gem_create_ext.

Reviewed-by: Matthew Auld <matthew.auld@intel.com>

>         __u64 extensions;
>  };
>
> @@ -2387,7 +2461,9 @@ struct drm_i915_gem_context_destroy {
>         __u32 pad;
>  };
>
> -/*
> +/**
> + * struct drm_i915_gem_vm_control - Structure to create or destroy VM.
> + *
>   * DRM_I915_GEM_VM_CREATE -
>   *
>   * Create a new virtual memory address space (ppGTT) for use within a context
> @@ -2397,20 +2473,23 @@ struct drm_i915_gem_context_destroy {
>   * The id of new VM (bound to the fd) for use with I915_CONTEXT_PARAM_VM is
>   * returned in the outparam @id.
>   *
> - * No flags are defined, with all bits reserved and must be zero.
> - *
>   * An extension chain maybe provided, starting with @extensions, and terminated
>   * by the @next_extension being 0. Currently, no extensions are defined.
>   *
>   * DRM_I915_GEM_VM_DESTROY -
>   *
> - * Destroys a previously created VM id, specified in @id.
> + * Destroys a previously created VM id, specified in @vm_id.
>   *
>   * No extensions or flags are allowed currently, and so must be zero.
>   */
>  struct drm_i915_gem_vm_control {
> +       /** @extensions: Zero-terminated chain of extensions. */
>         __u64 extensions;
> +
> +       /** @flags: reserved for future usage, currently MBZ */
>         __u32 flags;
> +
> +       /** @vm_id: Id of the VM created or to be destroyed */
>         __u32 vm_id;
>  };
>
> --
> 2.21.0.rc0.32.g243a4c7e27
>
Niranjana Vishwanathapura June 10, 2022, 1:43 a.m. UTC | #2
On Wed, Jun 08, 2022 at 12:24:04PM +0100, Matthew Auld wrote:
>On Tue, 17 May 2022 at 19:32, Niranjana Vishwanathapura
><niranjana.vishwanathapura@intel.com> wrote:
>>
>> Add some missing i915 upai documentation which the new
>> i915 VM_BIND feature documentation will be refer to.
>>
>> Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
>> ---
>>  include/uapi/drm/i915_drm.h | 153 +++++++++++++++++++++++++++---------
>>  1 file changed, 116 insertions(+), 37 deletions(-)
>>
>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>> index a2def7b27009..8c834a31b56f 100644
>> --- a/include/uapi/drm/i915_drm.h
>> +++ b/include/uapi/drm/i915_drm.h
>> @@ -751,9 +751,16 @@ typedef struct drm_i915_irq_wait {
>>
>>  /* Must be kept compact -- no holes and well documented */
>>
>> +/**
>> + * typedef drm_i915_getparam_t - Driver parameter query structure.
>
>This one looks funny in the rendered html for some reason, since it
>doesn't seem to emit the @param and @value, I guess it doesn't really
>understand typedef <struct> ?
>
>Maybe make this "struct drm_i915_getparam - Driver parameter query structure." ?

Thanks Matt.
Yah, there doesn't seems to be a good way to add kernel doc for this
kind of declaration. 'struct drm_i915_getparam' also didn't help.
I was able to fix it by first defining the structure and then adding
a typedef for it. Not sure if that has any value, but at least we can
get kernel doc for that.

>
>> + */
>>  typedef struct drm_i915_getparam {
>> +       /** @param: Driver parameter to query. */
>>         __s32 param;
>> -       /*
>> +
>> +       /**
>> +        * @value: Address of memory where queried value should be put.
>> +        *
>>          * WARNING: Using pointers instead of fixed-size u64 means we need to write
>>          * compat32 code. Don't repeat this mistake.
>>          */
>> @@ -1239,76 +1246,114 @@ struct drm_i915_gem_exec_object2 {
>>         __u64 rsvd2;
>>  };
>>
>> +/**
>> + * struct drm_i915_gem_exec_fence - An input or output fence for the execbuff
>
>s/execbuff/execbuf/, at least that seems to be what we use elsewhere, AFAICT.
>
>> + * ioctl.
>> + *
>> + * The request will wait for input fence to signal before submission.
>> + *
>> + * The returned output fence will be signaled after the completion of the
>> + * request.
>> + */
>>  struct drm_i915_gem_exec_fence {
>> -       /**
>> -        * User's handle for a drm_syncobj to wait on or signal.
>> -        */
>> +       /** @handle: User's handle for a drm_syncobj to wait on or signal. */
>>         __u32 handle;
>>
>> +       /**
>> +        * @flags: Supported flags are,
>
>are:
>
>> +        *
>> +        * I915_EXEC_FENCE_WAIT:
>> +        * Wait for the input fence before request submission.
>> +        *
>> +        * I915_EXEC_FENCE_SIGNAL:
>> +        * Return request completion fence as output
>> +        */
>> +       __u32 flags;
>>  #define I915_EXEC_FENCE_WAIT            (1<<0)
>>  #define I915_EXEC_FENCE_SIGNAL          (1<<1)
>>  #define __I915_EXEC_FENCE_UNKNOWN_FLAGS (-(I915_EXEC_FENCE_SIGNAL << 1))
>> -       __u32 flags;
>>  };
>>
>> -/*
>> - * See drm_i915_gem_execbuffer_ext_timeline_fences.
>> - */
>> -#define DRM_I915_GEM_EXECBUFFER_EXT_TIMELINE_FENCES 0
>> -
>> -/*
>> +/**
>> + * struct drm_i915_gem_execbuffer_ext_timeline_fences - Timeline fences
>> + * for execbuff.
>> + *
>>   * This structure describes an array of drm_syncobj and associated points for
>>   * timeline variants of drm_syncobj. It is invalid to append this structure to
>>   * the execbuf if I915_EXEC_FENCE_ARRAY is set.
>>   */
>>  struct drm_i915_gem_execbuffer_ext_timeline_fences {
>> +#define DRM_I915_GEM_EXECBUFFER_EXT_TIMELINE_FENCES 0
>> +       /** @base: Extension link. See struct i915_user_extension. */
>>         struct i915_user_extension base;
>>
>>         /**
>> -        * Number of element in the handles_ptr & value_ptr arrays.
>> +        * @fence_count: Number of element in the @handles_ptr & @value_ptr
>
>s/element/elements/
>
>> +        * arrays.
>>          */
>>         __u64 fence_count;
>>
>>         /**
>> -        * Pointer to an array of struct drm_i915_gem_exec_fence of length
>> -        * fence_count.
>> +        * @handles_ptr: Pointer to an array of struct drm_i915_gem_exec_fence
>> +        * of length @fence_count.
>>          */
>>         __u64 handles_ptr;
>>
>>         /**
>> -        * Pointer to an array of u64 values of length fence_count. Values
>> -        * must be 0 for a binary drm_syncobj. A Value of 0 for a timeline
>> -        * drm_syncobj is invalid as it turns a drm_syncobj into a binary one.
>> +        * @values_ptr: Pointer to an array of u64 values of length
>> +        * @fence_count.
>> +        * Values must be 0 for a binary drm_syncobj. A Value of 0 for a
>> +        * timeline drm_syncobj is invalid as it turns a drm_syncobj into a
>> +        * binary one.
>>          */
>>         __u64 values_ptr;
>>  };
>>
>> +/**
>> + * struct drm_i915_gem_execbuffer2 - Structure for execbuff submission
>> + */
>>  struct drm_i915_gem_execbuffer2 {
>> -       /**
>> -        * List of gem_exec_object2 structs
>> -        */
>> +       /** @buffers_ptr: Pointer to a list of gem_exec_object2 structs */
>>         __u64 buffers_ptr;
>> +
>> +       /** @buffer_count: Number of elements in @buffers_ptr array */
>>         __u32 buffer_count;
>>
>> -       /** Offset in the batchbuffer to start execution from. */
>> +       /**
>> +        * @batch_start_offset: Offset in the batchbuffer to start execution
>> +        * from.
>> +        */
>>         __u32 batch_start_offset;
>> -       /** Bytes used in batchbuffer from batch_start_offset */
>> +
>> +       /** @batch_len: Bytes used in batchbuffer from batch_start_offset */
>
>"Length in bytes of the batchbuffer, otherwise assumed to be the
>object size if zero, starting from the @batch_start_offset."
>
>>         __u32 batch_len;
>> +
>> +       /** @DR1: deprecated */
>>         __u32 DR1;
>> +
>> +       /** @DR4: deprecated */
>>         __u32 DR4;
>> +
>> +       /** @num_cliprects: See @cliprects_ptr */
>>         __u32 num_cliprects;
>> +
>>         /**
>> -        * This is a struct drm_clip_rect *cliprects if I915_EXEC_FENCE_ARRAY
>> -        * & I915_EXEC_USE_EXTENSIONS are not set.
>> +        * @cliprects_ptr: Kernel clipping was a DRI1 misfeature.
>> +        *
>> +        * It is invalid to use this field if I915_EXEC_FENCE_ARRAY or
>> +        * I915_EXEC_USE_EXTENSIONS flags are not set.
>>          *
>>          * If I915_EXEC_FENCE_ARRAY is set, then this is a pointer to an array
>> -        * of struct drm_i915_gem_exec_fence and num_cliprects is the length
>> -        * of the array.
>> +        * of &drm_i915_gem_exec_fence and @num_cliprects is the length of the
>> +        * array.
>>          *
>>          * If I915_EXEC_USE_EXTENSIONS is set, then this is a pointer to a
>> -        * single struct i915_user_extension and num_cliprects is 0.
>> +        * single &i915_user_extension and num_cliprects is 0.
>>          */
>>         __u64 cliprects_ptr;
>> +
>> +       /** @flags: Execbuff flags */
>
>s/Execbuff/Execbuf/
>
>Could maybe document the I915_EXEC_* also, or maybe not ;)
>

We no longer need to refer to execbuf2 as vm_bind will have its own
new execbuf3. But will keep the already added execbuf2 documentation.

>> +       __u64 flags;
>>  #define I915_EXEC_RING_MASK              (0x3f)
>>  #define I915_EXEC_DEFAULT                (0<<0)
>>  #define I915_EXEC_RENDER                 (1<<0)
>> @@ -1326,10 +1371,6 @@ struct drm_i915_gem_execbuffer2 {
>>  #define I915_EXEC_CONSTANTS_REL_GENERAL (0<<6) /* default */
>>  #define I915_EXEC_CONSTANTS_ABSOLUTE   (1<<6)
>>  #define I915_EXEC_CONSTANTS_REL_SURFACE (2<<6) /* gen4/5 only */
>> -       __u64 flags;
>> -       __u64 rsvd1; /* now used for context info */
>> -       __u64 rsvd2;
>> -};
>>
>>  /** Resets the SO write offset registers for transform feedback on gen7. */
>>  #define I915_EXEC_GEN7_SOL_RESET       (1<<8)
>> @@ -1432,9 +1473,23 @@ struct drm_i915_gem_execbuffer2 {
>>   * drm_i915_gem_execbuffer_ext enum.
>>   */
>>  #define I915_EXEC_USE_EXTENSIONS       (1 << 21)
>> -
>>  #define __I915_EXEC_UNKNOWN_FLAGS (-(I915_EXEC_USE_EXTENSIONS << 1))
>>
>> +       /** @rsvd1: Context id */
>> +       __u64 rsvd1;
>> +
>> +       /**
>> +        * @rsvd2: in and out sync_file file descriptors.
>> +        *
>> +        * When I915_EXEC_FENCE_IN or I915_EXEC_FENCE_SUBMIT flag is set, the
>> +        * lower 32 bits of this field will have the in sync_file fd (input).
>> +        *
>> +        * When I915_EXEC_FENCE_OUT flag is set, the upper 32 bits of this
>> +        * field will have the out sync_file fd (output).
>> +        */
>> +       __u64 rsvd2;
>> +};
>> +
>>  #define I915_EXEC_CONTEXT_ID_MASK      (0xffffffff)
>>  #define i915_execbuffer2_set_context_id(eb2, context) \
>>         (eb2).rsvd1 = context & I915_EXEC_CONTEXT_ID_MASK
>> @@ -1814,13 +1869,32 @@ struct drm_i915_gem_context_create {
>>         __u32 pad;
>>  };
>>
>> +/**
>> + * struct drm_i915_gem_context_create_ext - Structure for creating contexts.
>> + */
>>  struct drm_i915_gem_context_create_ext {
>> -       __u32 ctx_id; /* output: id of new context*/
>> +       /** @ctx_id: Id of the created context (output) */
>> +       __u32 ctx_id;
>> +
>> +       /**
>> +        * @flags: Supported flags are,
>
>are:
>
>> +        *
>> +        * I915_CONTEXT_CREATE_FLAGS_USE_EXTENSIONS:
>> +        *
>> +        * Extensions may be appended to this structure and driver must check
>> +        * for those.
>
>Maybe add "See @extensions.", and then....
>
>> +        *
>> +        * I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE
>> +        *
>> +        * Created context will have single timeline.
>> +        */
>>         __u32 flags;
>>  #define I915_CONTEXT_CREATE_FLAGS_USE_EXTENSIONS       (1u << 0)
>>  #define I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE      (1u << 1)
>>  #define I915_CONTEXT_CREATE_FLAGS_UNKNOWN \
>>         (-(I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE << 1))
>> +
>> +       /** @extensions: Zero-terminated chain of extensions. */
>
>...here perhaps list the extensions, and maybe also move the #define
>for each here? See for example @extensions in drm_i915_gem_create_ext.
>

Ok, will address all your comments above.

Niranjana

>Reviewed-by: Matthew Auld <matthew.auld@intel.com>
>
>>         __u64 extensions;
>>  };
>>
>> @@ -2387,7 +2461,9 @@ struct drm_i915_gem_context_destroy {
>>         __u32 pad;
>>  };
>>
>> -/*
>> +/**
>> + * struct drm_i915_gem_vm_control - Structure to create or destroy VM.
>> + *
>>   * DRM_I915_GEM_VM_CREATE -
>>   *
>>   * Create a new virtual memory address space (ppGTT) for use within a context
>> @@ -2397,20 +2473,23 @@ struct drm_i915_gem_context_destroy {
>>   * The id of new VM (bound to the fd) for use with I915_CONTEXT_PARAM_VM is
>>   * returned in the outparam @id.
>>   *
>> - * No flags are defined, with all bits reserved and must be zero.
>> - *
>>   * An extension chain maybe provided, starting with @extensions, and terminated
>>   * by the @next_extension being 0. Currently, no extensions are defined.
>>   *
>>   * DRM_I915_GEM_VM_DESTROY -
>>   *
>> - * Destroys a previously created VM id, specified in @id.
>> + * Destroys a previously created VM id, specified in @vm_id.
>>   *
>>   * No extensions or flags are allowed currently, and so must be zero.
>>   */
>>  struct drm_i915_gem_vm_control {
>> +       /** @extensions: Zero-terminated chain of extensions. */
>>         __u64 extensions;
>> +
>> +       /** @flags: reserved for future usage, currently MBZ */
>>         __u32 flags;
>> +
>> +       /** @vm_id: Id of the VM created or to be destroyed */
>>         __u32 vm_id;
>>  };
>>
>> --
>> 2.21.0.rc0.32.g243a4c7e27
>>
diff mbox series

Patch

diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index a2def7b27009..8c834a31b56f 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -751,9 +751,16 @@  typedef struct drm_i915_irq_wait {
 
 /* Must be kept compact -- no holes and well documented */
 
+/**
+ * typedef drm_i915_getparam_t - Driver parameter query structure.
+ */
 typedef struct drm_i915_getparam {
+	/** @param: Driver parameter to query. */
 	__s32 param;
-	/*
+
+	/**
+	 * @value: Address of memory where queried value should be put.
+	 *
 	 * WARNING: Using pointers instead of fixed-size u64 means we need to write
 	 * compat32 code. Don't repeat this mistake.
 	 */
@@ -1239,76 +1246,114 @@  struct drm_i915_gem_exec_object2 {
 	__u64 rsvd2;
 };
 
+/**
+ * struct drm_i915_gem_exec_fence - An input or output fence for the execbuff
+ * ioctl.
+ *
+ * The request will wait for input fence to signal before submission.
+ *
+ * The returned output fence will be signaled after the completion of the
+ * request.
+ */
 struct drm_i915_gem_exec_fence {
-	/**
-	 * User's handle for a drm_syncobj to wait on or signal.
-	 */
+	/** @handle: User's handle for a drm_syncobj to wait on or signal. */
 	__u32 handle;
 
+	/**
+	 * @flags: Supported flags are,
+	 *
+	 * I915_EXEC_FENCE_WAIT:
+	 * Wait for the input fence before request submission.
+	 *
+	 * I915_EXEC_FENCE_SIGNAL:
+	 * Return request completion fence as output
+	 */
+	__u32 flags;
 #define I915_EXEC_FENCE_WAIT            (1<<0)
 #define I915_EXEC_FENCE_SIGNAL          (1<<1)
 #define __I915_EXEC_FENCE_UNKNOWN_FLAGS (-(I915_EXEC_FENCE_SIGNAL << 1))
-	__u32 flags;
 };
 
-/*
- * See drm_i915_gem_execbuffer_ext_timeline_fences.
- */
-#define DRM_I915_GEM_EXECBUFFER_EXT_TIMELINE_FENCES 0
-
-/*
+/**
+ * struct drm_i915_gem_execbuffer_ext_timeline_fences - Timeline fences
+ * for execbuff.
+ *
  * This structure describes an array of drm_syncobj and associated points for
  * timeline variants of drm_syncobj. It is invalid to append this structure to
  * the execbuf if I915_EXEC_FENCE_ARRAY is set.
  */
 struct drm_i915_gem_execbuffer_ext_timeline_fences {
+#define DRM_I915_GEM_EXECBUFFER_EXT_TIMELINE_FENCES 0
+	/** @base: Extension link. See struct i915_user_extension. */
 	struct i915_user_extension base;
 
 	/**
-	 * Number of element in the handles_ptr & value_ptr arrays.
+	 * @fence_count: Number of element in the @handles_ptr & @value_ptr
+	 * arrays.
 	 */
 	__u64 fence_count;
 
 	/**
-	 * Pointer to an array of struct drm_i915_gem_exec_fence of length
-	 * fence_count.
+	 * @handles_ptr: Pointer to an array of struct drm_i915_gem_exec_fence
+	 * of length @fence_count.
 	 */
 	__u64 handles_ptr;
 
 	/**
-	 * Pointer to an array of u64 values of length fence_count. Values
-	 * must be 0 for a binary drm_syncobj. A Value of 0 for a timeline
-	 * drm_syncobj is invalid as it turns a drm_syncobj into a binary one.
+	 * @values_ptr: Pointer to an array of u64 values of length
+	 * @fence_count.
+	 * Values must be 0 for a binary drm_syncobj. A Value of 0 for a
+	 * timeline drm_syncobj is invalid as it turns a drm_syncobj into a
+	 * binary one.
 	 */
 	__u64 values_ptr;
 };
 
+/**
+ * struct drm_i915_gem_execbuffer2 - Structure for execbuff submission
+ */
 struct drm_i915_gem_execbuffer2 {
-	/**
-	 * List of gem_exec_object2 structs
-	 */
+	/** @buffers_ptr: Pointer to a list of gem_exec_object2 structs */
 	__u64 buffers_ptr;
+
+	/** @buffer_count: Number of elements in @buffers_ptr array */
 	__u32 buffer_count;
 
-	/** Offset in the batchbuffer to start execution from. */
+	/**
+	 * @batch_start_offset: Offset in the batchbuffer to start execution
+	 * from.
+	 */
 	__u32 batch_start_offset;
-	/** Bytes used in batchbuffer from batch_start_offset */
+
+	/** @batch_len: Bytes used in batchbuffer from batch_start_offset */
 	__u32 batch_len;
+
+	/** @DR1: deprecated */
 	__u32 DR1;
+
+	/** @DR4: deprecated */
 	__u32 DR4;
+
+	/** @num_cliprects: See @cliprects_ptr */
 	__u32 num_cliprects;
+
 	/**
-	 * This is a struct drm_clip_rect *cliprects if I915_EXEC_FENCE_ARRAY
-	 * & I915_EXEC_USE_EXTENSIONS are not set.
+	 * @cliprects_ptr: Kernel clipping was a DRI1 misfeature.
+	 *
+	 * It is invalid to use this field if I915_EXEC_FENCE_ARRAY or
+	 * I915_EXEC_USE_EXTENSIONS flags are not set.
 	 *
 	 * If I915_EXEC_FENCE_ARRAY is set, then this is a pointer to an array
-	 * of struct drm_i915_gem_exec_fence and num_cliprects is the length
-	 * of the array.
+	 * of &drm_i915_gem_exec_fence and @num_cliprects is the length of the
+	 * array.
 	 *
 	 * If I915_EXEC_USE_EXTENSIONS is set, then this is a pointer to a
-	 * single struct i915_user_extension and num_cliprects is 0.
+	 * single &i915_user_extension and num_cliprects is 0.
 	 */
 	__u64 cliprects_ptr;
+
+	/** @flags: Execbuff flags */
+	__u64 flags;
 #define I915_EXEC_RING_MASK              (0x3f)
 #define I915_EXEC_DEFAULT                (0<<0)
 #define I915_EXEC_RENDER                 (1<<0)
@@ -1326,10 +1371,6 @@  struct drm_i915_gem_execbuffer2 {
 #define I915_EXEC_CONSTANTS_REL_GENERAL (0<<6) /* default */
 #define I915_EXEC_CONSTANTS_ABSOLUTE 	(1<<6)
 #define I915_EXEC_CONSTANTS_REL_SURFACE (2<<6) /* gen4/5 only */
-	__u64 flags;
-	__u64 rsvd1; /* now used for context info */
-	__u64 rsvd2;
-};
 
 /** Resets the SO write offset registers for transform feedback on gen7. */
 #define I915_EXEC_GEN7_SOL_RESET	(1<<8)
@@ -1432,9 +1473,23 @@  struct drm_i915_gem_execbuffer2 {
  * drm_i915_gem_execbuffer_ext enum.
  */
 #define I915_EXEC_USE_EXTENSIONS	(1 << 21)
-
 #define __I915_EXEC_UNKNOWN_FLAGS (-(I915_EXEC_USE_EXTENSIONS << 1))
 
+	/** @rsvd1: Context id */
+	__u64 rsvd1;
+
+	/**
+	 * @rsvd2: in and out sync_file file descriptors.
+	 *
+	 * When I915_EXEC_FENCE_IN or I915_EXEC_FENCE_SUBMIT flag is set, the
+	 * lower 32 bits of this field will have the in sync_file fd (input).
+	 *
+	 * When I915_EXEC_FENCE_OUT flag is set, the upper 32 bits of this
+	 * field will have the out sync_file fd (output).
+	 */
+	__u64 rsvd2;
+};
+
 #define I915_EXEC_CONTEXT_ID_MASK	(0xffffffff)
 #define i915_execbuffer2_set_context_id(eb2, context) \
 	(eb2).rsvd1 = context & I915_EXEC_CONTEXT_ID_MASK
@@ -1814,13 +1869,32 @@  struct drm_i915_gem_context_create {
 	__u32 pad;
 };
 
+/**
+ * struct drm_i915_gem_context_create_ext - Structure for creating contexts.
+ */
 struct drm_i915_gem_context_create_ext {
-	__u32 ctx_id; /* output: id of new context*/
+	/** @ctx_id: Id of the created context (output) */
+	__u32 ctx_id;
+
+	/**
+	 * @flags: Supported flags are,
+	 *
+	 * I915_CONTEXT_CREATE_FLAGS_USE_EXTENSIONS:
+	 *
+	 * Extensions may be appended to this structure and driver must check
+	 * for those.
+	 *
+	 * I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE
+	 *
+	 * Created context will have single timeline.
+	 */
 	__u32 flags;
 #define I915_CONTEXT_CREATE_FLAGS_USE_EXTENSIONS	(1u << 0)
 #define I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE	(1u << 1)
 #define I915_CONTEXT_CREATE_FLAGS_UNKNOWN \
 	(-(I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE << 1))
+
+	/** @extensions: Zero-terminated chain of extensions. */
 	__u64 extensions;
 };
 
@@ -2387,7 +2461,9 @@  struct drm_i915_gem_context_destroy {
 	__u32 pad;
 };
 
-/*
+/**
+ * struct drm_i915_gem_vm_control - Structure to create or destroy VM.
+ *
  * DRM_I915_GEM_VM_CREATE -
  *
  * Create a new virtual memory address space (ppGTT) for use within a context
@@ -2397,20 +2473,23 @@  struct drm_i915_gem_context_destroy {
  * The id of new VM (bound to the fd) for use with I915_CONTEXT_PARAM_VM is
  * returned in the outparam @id.
  *
- * No flags are defined, with all bits reserved and must be zero.
- *
  * An extension chain maybe provided, starting with @extensions, and terminated
  * by the @next_extension being 0. Currently, no extensions are defined.
  *
  * DRM_I915_GEM_VM_DESTROY -
  *
- * Destroys a previously created VM id, specified in @id.
+ * Destroys a previously created VM id, specified in @vm_id.
  *
  * No extensions or flags are allowed currently, and so must be zero.
  */
 struct drm_i915_gem_vm_control {
+	/** @extensions: Zero-terminated chain of extensions. */
 	__u64 extensions;
+
+	/** @flags: reserved for future usage, currently MBZ */
 	__u32 flags;
+
+	/** @vm_id: Id of the VM created or to be destroyed */
 	__u32 vm_id;
 };