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

[Souza, Jose]

On Wed, 2023-03-22 at 22:35 +0000, Matthew Brost wrote:
> 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.

Oh media, makes sense.

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