[Intel-xe] [RFC PATCH 1/1] drm/xe/uapi: Add query engines uAPI

[Matthew Brost]

On Wed, Mar 22, 2023 at 02:55:22PM -0600, Souza, Jose wrote:
> On Wed, 2023-03-22 at 19:37 +0000, Matthew Brost wrote:
> > On Wed, Mar 22, 2023 at 10:18:33AM -0600, Souza, Jose wrote:
> > > On Tue, 2023-03-21 at 19:34 -0700, Matthew Brost wrote:
> > > > Not all hardware engines are created equally, certain instance have
> > > > unique capabilities or properties. Introduce a uAPI to query for both
> > > > capabilities and properties. The capabilities are represented by a bit
> > > > mask which is unique to each class. The properties are represented by a
> > > > name and value return in an array to the user.
> > > 
> > > When re-sending the same patch series with fixes, please include "vX" in the subject we know what is the latest version to review.
> > > 
> > 
> > Will do.
> > 
> > > > 
> > > > Signed-off-by: Matthew Brost <matthew.brost at intel.com>
> > > > ---
> > > >  include/uapi/drm/xe_drm.h | 44 +++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 44 insertions(+)
> > > > 
> > > > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> > > > index 661d7929210c..557c9ae6f364 100644
> > > > --- a/include/uapi/drm/xe_drm.h
> > > > +++ b/include/uapi/drm/xe_drm.h
> > > > @@ -119,6 +119,7 @@ struct xe_user_extension {
> > > >  #define DRM_XE_WAIT_USER_FENCE		0x0b
> > > >  #define DRM_XE_VM_MADVISE		0x0c
> > > >  #define DRM_XE_ENGINE_GET_PROPERTY	0x0d
> > > > +#define DRM_XE_ENGINE_QUERY		0x0f
> > > 
> > > s/0x0f/0x0e
> > > 
> > 
> > Yep.
> > 
> > > To avoid confusion with DRM_XE_ENGINE_GET_PROPERTY, would be better to name this and related stuff as DRM_XE_ENGINE_CLASS_QUERY.
> > > 
> > 
> > I don't love DRM_XE_ENGINE_CLASS_QUERY as the query is for the class,
> > id, gt tuple.
> > 
> > We definitely need to do work in general on the naming to avoid
> > confusion between a hardware engine and a user engine. Perhaps just do a
> > large find replace across Xe where a user engine -> context?
> > 
> > Another option would be the uAPI with HW engine naming as needed. e.g.
> > 
> > s/DRM_XE_ENGINE_CLASS_QUERY/DRM_XE_HW_ENGINE_CLASS_QUERY
> > s/drm_xe_engine_class_instance/drm_xe_hw_engine_class_instance
> > s/DRM_XE_DEVICE_QUERY_ENGINES/DRM_XE_DEVICE_QUERY_HW_ENGINES
> 
> I liked option 2 too.
>

Cool, in next rev I will also include these changes.

> About the new UAPI, what usage do you have in mind?
> I can't think any information that Mesa would need from hw_engine at this moment.
> Maybe add some comments about this in xe_drm.h.
> 

This a ask from media, I guess not all Video decode hardware engines
have the same features thus we need a way communicate this to the media
UMD. Who knows what else we will need to report about hardware engines
in the future so tried to make this interface as flexable as possible.

Matt

> > 
> > I'd lean towards option #2 but open to either.
> > 
> > Matt
> > 
> > > >  
> > > >  /* Must be kept compact -- no holes */
> > > >  #define DRM_IOCTL_XE_DEVICE_QUERY		DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_DEVICE_QUERY, struct drm_xe_device_query)
> > > > @@ -135,6 +136,7 @@ struct xe_user_extension {
> > > >  #define DRM_IOCTL_XE_ENGINE_SET_PROPERTY	DRM_IOW( DRM_COMMAND_BASE + DRM_XE_ENGINE_SET_PROPERTY, struct drm_xe_engine_set_property)
> > > >  #define DRM_IOCTL_XE_WAIT_USER_FENCE		DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_WAIT_USER_FENCE, struct drm_xe_wait_user_fence)
> > > >  #define DRM_IOCTL_XE_VM_MADVISE			DRM_IOW( DRM_COMMAND_BASE + DRM_XE_VM_MADVISE, struct drm_xe_vm_madvise)
> > > > +#define DRM_IOCTL_XE_ENGINE_QUERY		DRM_IOWR(DRM_COMMAND_BASE + DRM_XE_ENGINE_QUERY, struct drm_xe_engine_query)
> > > 
> > > 
> > > 
> > > >  
> > > >  struct drm_xe_engine_class_instance {
> > > >  	__u16 engine_class;
> > > > @@ -253,6 +255,48 @@ struct drm_xe_device_query {
> > > >  	__u64 reserved[2];
> > > >  };
> > > >  
> > > > +struct drm_xe_engine_query_property {
> > > > +	/** @property: pointer to the first property struct, if any */
> > > > +	__u64 property;
> > > > +
> > > > +	/** @name: name of the property */
> > > > +	__u64 name;
> > > > +
> > > > +	/** @value: value property */
> > > > +	__u64 value;
> > > > +
> > > > +	/** @reserved: reserved */
> > > > +	__u64 reserved[2];
> > > > +};
> > > > +
> > > > +struct drm_xe_engine_query {
> > > > +	/** @extensions: pointer to the first extension struct, if any */
> > > > +	__u64 extensions;
> > > > +
> > > > +	/** @eci: engine class instance to query */
> > > > +	struct drm_xe_engine_class_instance eci;
> > > > +
> > > > +	/** @capabilities: bit mask of capabilities, specific to each class */
> > > > +	__u64 capabilities;
> > > > +
> > > > +	/**
> > > > +	 * @size: Size of the queried properties, if user passes in zero the KMD
> > > > +	 * returns the size of the properties array, if user passes in the size
> > > > +	 * of the properties array the KMD copies the properties to the user
> > > > +	 * pointer.
> > > > +	 */
> > > > +	__u32 size;
> > > > +
> > > > +	/**
> > > > +	 * @properties: user pointer to which an array of  struct
> > > > +	 * drm_xe_engine_query_property are copied to if size is non-zero
> > > > +	 */
> > > > +	__u64 properties;
> > > > +
> > > > +	/** @reserved: reserved */
> > > > +	__u64 reserved[2];
> > > > +};
> > > > +
> > > >  struct drm_xe_gem_create {
> > > >  	/** @extensions: Pointer to the first extension struct, if any */
> > > >  	__u64 extensions;
> > > 
>