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

[Francois Dugast <francois.dugast@intel.com>]

On Thu, Dec 14, 2023 at 10:45:59AM -0600, Lucas De Marchi wrote:
> On Thu, Dec 07, 2023 at 01:50:04PM +0000, Francois Dugast wrote:
> > From: Rodrigo Vivi <rodrigo.vivi@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@intel.com>
> > Signed-off-by: Francois Dugast <francois.dugast@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)?

uAPI seems used this way in DRM (though not everywhere), see Documentation/gpu/driver-uapi.rst.

> 
> > + *
> > + * 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@intel.com>

I will fix them, thanks.

Francois

> 
> 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
> >