Re: [Intel-xe] [PATCH v5 10/20] drm/xe: Remove XE_EXEC_QUEUE_SET_PROPERTY_COMPUTE_MODE from uAPI

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Tue, 17 Oct 2023 09:02:42 -0700, Rodrigo Vivi wrote:
>
> On Tue, Oct 17, 2023 at 08:37:47AM -0700, Dixit, Ashutosh wrote:
> > On Fri, 06 Oct 2023 02:59:33 -0700, Francois Dugast wrote:
> > >
> >
> > Hi Francois,
> >
> > > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> > > index ad21ba1d6e0b..2a9e04024723 100644
> > > --- a/include/uapi/drm/xe_drm.h
> > > +++ b/include/uapi/drm/xe_drm.h
> > > @@ -781,21 +781,14 @@ struct drm_xe_exec_queue_set_property {
> > >	/** @exec_queue_id: Exec queue ID */
> > >	__u32 exec_queue_id;
> > >
> > > -#define XE_EXEC_QUEUE_SET_PROPERTY_PRIORITY			0
> > > +#define XE_EXEC_QUEUE_SET_PROPERTY_PRIORITY		0
> > >  #define XE_EXEC_QUEUE_SET_PROPERTY_TIMESLICE		1
> > >  #define XE_EXEC_QUEUE_SET_PROPERTY_PREEMPTION_TIMEOUT	2
> > > -	/*
> > > -	 * Long running or ULLS engine mode. DMA fences not allowed in this
> > > -	 * mode. Must match the value of DRM_XE_VM_CREATE_COMPUTE_MODE, serves
> > > -	 * as a sanity check the UMD knows what it is doing. Can only be set at
> > > -	 * engine create time.
> > > -	 */
> > > -#define XE_EXEC_QUEUE_SET_PROPERTY_COMPUTE_MODE		3
> > > -#define XE_EXEC_QUEUE_SET_PROPERTY_PERSISTENCE		4
> > > -#define XE_EXEC_QUEUE_SET_PROPERTY_JOB_TIMEOUT		5
> > > -#define XE_EXEC_QUEUE_SET_PROPERTY_ACC_TRIGGER		6
> > > -#define XE_EXEC_QUEUE_SET_PROPERTY_ACC_NOTIFY		7
> > > -#define XE_EXEC_QUEUE_SET_PROPERTY_ACC_GRANULARITY		8
> > > +#define XE_EXEC_QUEUE_SET_PROPERTY_PERSISTENCE		3
> > > +#define XE_EXEC_QUEUE_SET_PROPERTY_JOB_TIMEOUT		4
> > > +#define XE_EXEC_QUEUE_SET_PROPERTY_ACC_TRIGGER		5
> > > +#define XE_EXEC_QUEUE_SET_PROPERTY_ACC_NOTIFY		6
> > > +#define XE_EXEC_QUEUE_SET_PROPERTY_ACC_GRANULARITY	7
> > >	/** @property: property to set */
> > >	__u32 property;
> > >
> > >	/** @value: property value */
> > >	__u64 value;
> >
> > Mostly a nit, but I had a question about this because I am facing a similar
> > decision elsewhere. Why do we have one property/value pair in this struct
> > when all the rest of the property/value pairs are coming in via the
> > extensions? Basically, how about removing this property/value pair from
> > thus struct and let all the property/value pairs come in only via
> > extensions? I think that would both simplify the code a bit and be more
> > consistent. Thoughts?
>
> or maybe the other way around and do not have extensions on the first
> version of the api?

How would that work? Remove all these properties in v1 and replace by a
struct and only have any later changes via extenstions? That would be
non-uniform but of course doable.

I have an identical situation with some OA uapi too so wanted to get a
sense of what is preferable. So that we follow a common approach and don't
do things differently for different portions of the uapi. Thanks.

> just thinking out loud, but no hard feelings for either way...
>
> >
> > Thanks.
> > --
> > Ashutosh