[PATCH v1 09/14] drm/xe/uapi: More uAPI documentation additions and cosmetic updates

[Lucas De Marchi]

On Thu, Dec 07, 2023 at 01:50:04PM +0000, Francois Dugast wrote:
>From: Rodrigo Vivi <rodrigo.vivi at intel.com>
>
>No functional change in this patch.
>
>Let's ensure all of our structs are documented and with a certain
>standard. Also, let's have an overview and list of IOCTLs as the
>very beginning of the generated HTML doc.
>
>Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>Signed-off-by: Francois Dugast <francois.dugast at intel.com>
>---
> include/uapi/drm/xe_drm.h | 48 +++++++++++++++++++++++++++++++++------
> 1 file changed, 41 insertions(+), 7 deletions(-)
>
>diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
>index b36f3bd53611..55cb22b2c7e2 100644
>--- a/include/uapi/drm/xe_drm.h
>+++ b/include/uapi/drm/xe_drm.h
>@@ -25,6 +25,28 @@ extern "C" {
>  *   6. PMU
>  */
>
>+/**
>+ * DOC: Xe uAPI Overview

not sure about the lowercase u. Isn't that used when it refers to 
µ, like in uC (microcontroller)?

>+ *
>+ * This section aims to describe the Xe's IOCTL entries, its structs, and other
>+ * Xe related uAPI such as uevents and PMU (Platform Monitoring Unit) related
>+ * entries and usage.
>+ *
>+ * List of supported IOCTLs:
>+ *  - &DRM_IOCTL_XE_DEVICE_QUERY
>+ *  - &DRM_IOCTL_XE_GEM_CREATE
>+ *  - &DRM_IOCTL_XE_GEM_MMAP_OFFSET
>+ *  - &DRM_IOCTL_XE_VM_CREATE
>+ *  - &DRM_IOCTL_XE_VM_DESTROY
>+ *  - &DRM_IOCTL_XE_VM_BIND
>+ *  - &DRM_IOCTL_XE_EXEC
>+ *  - &DRM_IOCTL_XE_EXEC_QUEUE_CREATE
>+ *  - &DRM_IOCTL_XE_EXEC_QUEUE_DESTROY
>+ *  - &DRM_IOCTL_XE_EXEC_QUEUE_GET_PROPERTY
>+ *  - &DRM_IOCTL_XE_WAIT_USER_FENCE
>+ *

stray newline

>+ */
>+
> /*
>  * xe specific ioctls.
>  *
>@@ -59,7 +81,10 @@ extern "C" {
> #define DRM_IOCTL_XE_WAIT_USER_FENCE		DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_WAIT_USER_FENCE, struct drm_xe_wait_user_fence)
>
> /**
>- * struct xe_user_extension - Base class for defining a chain of extensions
>+ * DOC: Xe IOCT Extensions

IOCTL

>+ *
>+ * Before detailing the IOCTLs and its structs, it is important to highlight
>+ * that every IOCTL in Xe is extensible.
>  *
>  * Many interfaces need to grow over time. In most cases we can simply
>  * extend the struct and have userspace pass in more data. Another option,
>@@ -93,7 +118,10 @@ extern "C" {
>  * Typically the struct xe_user_extension would be embedded in some uAPI

			  ^ to be renamed to drm_xe_...

>  * struct, and in this case we would feed it the head of the chain(i.e ext1),
>  * which would then apply all of the above extensions.
>- *
>+*/
>+
>+/**
>+ * struct xe_user_extension - Base class for defining a chain of extensions
>  */
> struct xe_user_extension {
> 	/**
>@@ -123,7 +151,10 @@ struct xe_user_extension {
> };
>
> /**
>- * struct drm_xe_ext_set_property - XE set property extension
>+ * struct drm_xe_ext_set_property - Generic set property extension
>+ *
>+ * A generic struct that could allow any of the Xe's IOCTL to be extended

IMO looks better in the present tense: s/that could allow/that allows/


other than these nits,


	Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>

thanks
Lucas De Marchi

>+ * with a set_property operation.
>  */
> struct drm_xe_ext_set_property {
> 	/** @base: base user extension */
>@@ -294,7 +325,7 @@ struct drm_xe_mem_region {
> 	 * here will always be zero).
> 	 */
> 	__u64 cpu_visible_used;
>-	/** @reserved: MBZ */
>+	/** @reserved: Reserved */
> 	__u64 reserved[6];
> };
>
>@@ -1051,8 +1082,8 @@ struct drm_xe_sync {
> 		__u32 handle;
>
> 		/**
>-		 * @addr: Address of user fence. When sync passed in via exec
>-		 * IOCTL this a GPU address in the VM. When sync passed in via
>+		 * @addr: Address of user fence. When sync is passed in via exec
>+		 * IOCTL this is a GPU address in the VM. When sync passed in via
> 		 * VM bind IOCTL this is a user pointer. In either case, it is
> 		 * the users responsibility that this address is present and
> 		 * mapped when the user fence is signalled. Must be qword
>@@ -1061,7 +1092,10 @@ struct drm_xe_sync {
> 		__u64 addr;
> 	};
>
>-	/** @timeline_value: Timeline point of the sync object */
>+	/**
>+	 * @timeline_value: Input for the timeline sync object. Needs to be
>+	 * different than 0 when used with %DRM_XE_SYNC_FLAG_TIMELINE_SYNCOBJ.
>+	 */
> 	__u64 timeline_value;
>
> 	/** @reserved: Reserved */
>-- 
>2.34.1
>