Re: [Intel-xe] [19/21] drm/xe/uapi: Drop OA_IOCTL_VERSION

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

On Fri, Oct 20, 2023 at 10:36:37AM +0300, Lionel Landwerlin wrote:
>On 19/09/2023 19:10, Ashutosh Dixit wrote:
>>OA version was previously used to track which OA properties were introduced
>>at which version. However OA version is an outlier in that a similar
>>version is not used anywhere else in the kernel.
>>
>>For XE, we will track addition of new properties by means of
>>xe_user_extension. Userland can either maintain a mapping of OA properties
>>against the kernel version, or rely on return codes (e.g. ENOTSUPP) to
>>"discover" OA properties.
>
>
>As a userspace driver this terrible.
>
>You're making us write more code because we have to discover every 
>single piece of implementation tweaks ever made to the kernel driver.
>
>Yes OA version was an outlier in that it was better than the rest of 
>kernel API.
>
>By reading a single value we could make decisions about how to 
>communicate with the kernel driver.
>
>
>Having xe_user_extension here is not helping, we still have to fuzz 
>our way around the kernel to figure out what feature level is 
>available.

Hi Lionel, Ashutosh,

I don't want to change interfaces to make it more difficult for the 
UMDs. Do you think capabilities could help achieve what you want?  We 
could have a bitmask of feature availability and the bitmask could be 
part of the xe oa query.

If that's seen as an unnecessary uApi impact, then I am ok with version 
being returned in the oa info query.

My primary concerns with the version in i915 were

(1) version bumps would need documentation even when version bump did 
not have explicit uApi change. I couldn't find a good place in 
i915_drm.h to put such documentation. I think capabilities would be self 
documenting and it would be explicitly added to the uApi header.  

(2) sometimes before upstreaming a feature, we maintained an internal 
version for the feature that was using a different range of versions.  
This would cause some grief when rebasing since the versions need to be 
increasing in the order of commits. If we had a bitmask, we can manage 
it better since there is no need to have an increasing value.

Suggestions welcome.

Thanks,
Umesh


>
>-Lionel
>
>
>>
>>Suggested-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>>Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>>---
>>  drivers/gpu/drm/xe/xe_oa.c    | 5 -----
>>  drivers/gpu/drm/xe/xe_oa.h    | 1 -
>>  drivers/gpu/drm/xe/xe_query.c | 1 -
>>  include/uapi/drm/xe_drm.h     | 3 +--
>>  4 files changed, 1 insertion(+), 9 deletions(-)
>>
>>diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
>>index 077698a0c5628..7cb900fc88f58 100644
>>--- a/drivers/gpu/drm/xe/xe_oa.c
>>+++ b/drivers/gpu/drm/xe/xe_oa.c
>>@@ -2331,11 +2331,6 @@ static struct ctl_table oa_ctl_table[] = {
>>  	{}
>>  };
>>-int xe_oa_ioctl_version(struct xe_device *xe)
>>-{
>>-	return 1;
>>-}
>>-
>>  int xe_oa_sysctl_register(void)
>>  {
>>  	sysctl_header = register_sysctl("dev/xe", oa_ctl_table);
>>diff --git a/drivers/gpu/drm/xe/xe_oa.h b/drivers/gpu/drm/xe/xe_oa.h
>>index cc6f64bc24ddf..c5a2745ab7cfe 100644
>>--- a/drivers/gpu/drm/xe/xe_oa.h
>>+++ b/drivers/gpu/drm/xe/xe_oa.h
>>@@ -12,7 +12,6 @@ int xe_oa_init(struct xe_device *xe);
>>  void xe_oa_fini(struct xe_device *xe);
>>  void xe_oa_register(struct xe_device *xe);
>>  void xe_oa_unregister(struct xe_device *xe);
>>-int xe_oa_ioctl_version(struct xe_device *xe);
>>  int xe_oa_sysctl_register(void);
>>  void xe_oa_sysctl_unregister(void);
>>diff --git a/drivers/gpu/drm/xe/xe_query.c b/drivers/gpu/drm/xe/xe_query.c
>>index ad280bac9eed4..8246ce4e24ce5 100644
>>--- a/drivers/gpu/drm/xe/xe_query.c
>>+++ b/drivers/gpu/drm/xe/xe_query.c
>>@@ -202,7 +202,6 @@ static int query_config(struct xe_device *xe, struct drm_xe_device_query *query)
>>  		hweight_long(xe->info.mem_region_mask);
>>  	config->info[XE_QUERY_CONFIG_MAX_ENGINE_PRIORITY] =
>>  		xe_exec_queue_device_get_max_priority(xe);
>>-	config->info[XE_QUERY_OA_IOCTL_VERSION] = xe_oa_ioctl_version(xe);
>>  	if (copy_to_user(query_ptr, config, size)) {
>>  		kfree(config);
>>diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
>>index 3b106bed42ea6..b0563cfc351ee 100644
>>--- a/include/uapi/drm/xe_drm.h
>>+++ b/include/uapi/drm/xe_drm.h
>>@@ -259,8 +259,7 @@ struct drm_xe_query_config {
>>  #define XE_QUERY_CONFIG_GT_COUNT		4
>>  #define XE_QUERY_CONFIG_MEM_REGION_COUNT	5
>>  #define XE_QUERY_CONFIG_MAX_ENGINE_PRIORITY	6
>>-#define XE_QUERY_OA_IOCTL_VERSION		7
>>-#define XE_QUERY_CONFIG_NUM_PARAM		(XE_QUERY_OA_IOCTL_VERSION + 1)
>>+#define XE_QUERY_CONFIG_NUM_PARAM		(XE_QUERY_CONFIG_MAX_ENGINE_PRIORITY + 1)
>>  	/** @info: array of elements containing the config info */
>>  	__u64 info[];
>>  };
>
>