Re: [Intel-xe] [PATCH v3 15/16] drm/xe: Remove unused extension definition

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

On Thu, 30 Nov 2023 10:39:54 -0800, Francois Dugast wrote:
>
> From: Rodrigo Vivi <rodrigo.vivi@intel.com>
>
> The vm_create ioctl function doesn't accept any extension.
> Remove this left over.
> A backward compatible change.
>
> Cc: Francois Dugast <francois.dugast@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> ---
>  include/uapi/drm/xe_drm.h | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> index 84a477009e85..ffd9fc1172e8 100644
> --- a/include/uapi/drm/xe_drm.h
> +++ b/include/uapi/drm/xe_drm.h
> @@ -642,7 +642,6 @@ struct drm_xe_ext_set_property {
>  };
>
>  struct drm_xe_vm_create {
> -#define DRM_XE_VM_EXTENSION_SET_PROPERTY	0
>	/** @extensions: Pointer to the first extension struct, if any */
>	__u64 extensions;

I have another general comment on the use of these extensions (prompted by
John H's bringing up KLV's (key-length-value blobs) in an internal
chat). Here goes. Currently we have structs which are basically:

	struct xyz {
		/** @extensions: Pointer to the first extension struct, if any */
		__u64 extensions;

		/* Remaining struct fields */
	};

A slightly different way of doing this would be:

	struct root {
		/** @extensions: Pointer to the first extension struct */
		__u64 extensions;
	};

	struct xyz {
		/** @base: base user extension */
		struct xe_user_extension base;

		/* Remaining struct fields */
	};

So here the idea is the the first level struct ('struct root') does not
contain any real struct fields at all. It *always* points to a second level
struct ('struct xyz') which contain the real data to passed in.

Maybe what we have in Xe today is ok, but the second approach is closer to
KLV John brought up. If 'struct xyz' changes, we can just change
'extensions' in 'struct root' to point to say a new 'struct xyz_v2'.

What we have now in Xe is also ok, but we need a protocol for how to
interpret 'extensions', e.g. that if 'extensions' is 0, we will use the
first level struct, but if 'extensions' is not 0, we will follow the chain
and use the second level struct. As long as this protocol is clearly
defined we might be ok with what we have. But is the protocol defined
today? Is it the same behavior across all uapi's?

That's why the second approach seems a bit cleaner and unambiguous to me.

Thanks.
--
Ashutosh