Re: [Intel-xe] [PATCH] drm/xe/uapi: Use common drm_xe_ext_set_property extension

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Sat, Sep 09, 2023 at 11:15:20AM -0400, Rodrigo Vivi wrote:
> On Fri, Sep 08, 2023 at 10:21:53PM -0700, Ashutosh Dixit wrote:
> > There really is no difference between 'struct drm_xe_ext_vm_set_property'
> > and 'struct drm_xe_ext_exec_queue_set_property', they are extensions which
> > specify a <property, value> pair. Replace the two extensions with a single
> > common 'struct drm_xe_ext_set_property' extension. The rationale is that
> > rather than have each XE module (including future modules) invent their own
> > property/value extensions, all XE modules use a common set_property
> > extension when possible.
> 
> What about just killing this entirely?
> https://lore.kernel.org/all/20230908203302.449041-2-rodrigo.vivi@intel.com/

hmm... or maybe what we want is a mix of your patch and mine.
Let's use yours to kill the drm_xe_ext_exec_queue_set_property
in favor of a generic drm_xe_ext_set_property
and then mine goes on top killing just the
XE_VM_PROPERTY_BIND_OP_ERROR_CAPTURE_ADDRESS operation.

thoughts? I can integrate and carry your patch in my series if you are
okay with that.

> 
> 
> > 
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_exec_queue.c |  2 +-
> >  drivers/gpu/drm/xe/xe_vm.c         |  2 +-
> >  include/uapi/drm/xe_drm.h          | 20 +++-----------------
> >  3 files changed, 5 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
> > index e44d71c679cc3..fc44249f13372 100644
> > --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> > +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
> > @@ -459,7 +459,7 @@ static int exec_queue_user_ext_set_property(struct xe_device *xe,
> >  					    bool create)
> >  {
> >  	u64 __user *address = u64_to_user_ptr(extension);
> > -	struct drm_xe_ext_exec_queue_set_property ext;
> > +	struct drm_xe_ext_set_property ext;
> >  	int err;
> >  	u32 idx;
> >  
> > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > index 1d9aa5c40659c..36c39589b0036 100644
> > --- a/drivers/gpu/drm/xe/xe_vm.c
> > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > @@ -1915,7 +1915,7 @@ static int vm_user_ext_set_property(struct xe_device *xe, struct xe_vm *vm,
> >  				    u64 extension)
> >  {
> >  	u64 __user *address = u64_to_user_ptr(extension);
> > -	struct drm_xe_ext_vm_set_property ext;
> > +	struct drm_xe_ext_set_property ext;
> >  	int err;
> >  
> >  	err = __copy_from_user(&ext, address, sizeof(ext));
> > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> > index 00d5cb4ef85e7..1338a64d42854 100644
> > --- a/include/uapi/drm/xe_drm.h
> > +++ b/include/uapi/drm/xe_drm.h
> > @@ -497,12 +497,11 @@ struct drm_xe_vm_bind_op_error_capture {
> >  	__u64 size;
> >  };
> >  
> > -/** struct drm_xe_ext_vm_set_property - VM set property extension */
> > -struct drm_xe_ext_vm_set_property {
> > +/** struct drm_xe_ext_set_property - XE set property extension */
> > +struct drm_xe_ext_set_property {
> >  	/** @base: base user extension */
> >  	struct xe_user_extension base;
> >  
> > -#define XE_VM_PROPERTY_BIND_OP_ERROR_CAPTURE_ADDRESS		0
> >  	/** @property: property to set */
> >  	__u32 property;
> >  
> > @@ -518,6 +517,7 @@ struct drm_xe_ext_vm_set_property {
> >  
> >  struct drm_xe_vm_create {
> >  #define XE_VM_EXTENSION_SET_PROPERTY	0
> > +#define XE_VM_PROPERTY_BIND_OP_ERROR_CAPTURE_ADDRESS		0
> >  	/** @extensions: Pointer to the first extension struct, if any */
> >  	__u64 extensions;
> >  
> > @@ -681,20 +681,6 @@ struct drm_xe_vm_bind {
> >  };
> >  
> >  /** struct drm_xe_ext_exec_queue_set_property - exec queue set property extension */
> > -struct drm_xe_ext_exec_queue_set_property {
> > -	/** @base: base user extension */
> > -	struct xe_user_extension base;
> > -
> > -	/** @property: property to set */
> > -	__u32 property;
> > -
> > -	/** @pad: MBZ */
> > -	__u32 pad;
> > -
> > -	/** @value: property value */
> > -	__u64 value;
> > -};
> > -
> >  /**
> >   * struct drm_xe_exec_queue_set_property - exec queue set property
> >   *
> > -- 
> > 2.41.0
> >