[PATCH 7/7] drm/xe/oa: Allow only certain property changes from config

[Cavitt, Jonathan]

-----Original Message-----
From: Dixit, Ashutosh <ashutosh.dixit at intel.com> 
Sent: Monday, August 19, 2024 5:58 PM
To: intel-xe at lists.freedesktop.org
Cc: Brost, Matthew <matthew.brost at intel.com>; Souza, Jose <jose.souza at intel.com>; Landwerlin, Lionel G <lionel.g.landwerlin at intel.com>; Nerlige Ramappa, Umesh <umesh.nerlige.ramappa at intel.com>; Cavitt, Jonathan <jonathan.cavitt at intel.com>
Subject: [PATCH 7/7] drm/xe/oa: Allow only certain property changes from config
> 
> Whereas all properties can be specified during OA stream open, when the OA
> stream is reconfigured only the config_id and syncs can be specified.
> 
> v2: Use separate function table for reconfig case (Jonathan)
>     Change bool function args to enum (Matt B)
> 
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit at intel.com>


Minor nits below, but otherwise LGTM.
Reviewed-by: Jonathan Cavitt <jonathan.cavitt at intel.com>


> ---
>  drivers/gpu/drm/xe/xe_oa.c | 51 ++++++++++++++++++++++++++++++--------
>  1 file changed, 40 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> index f43767e0a75ec..96420c3ea10fd 100644
> --- a/drivers/gpu/drm/xe/xe_oa.c
> +++ b/drivers/gpu/drm/xe/xe_oa.c
> @@ -47,6 +47,11 @@ enum xe_oa_submit_deps {
>  	XE_OA_SUBMIT_ADD_DEPS,
>  };
>  
> +enum xe_oa_user_extn_from {
> +	XE_OA_USER_EXTN_FROM_OPEN,
> +	XE_OA_USER_EXTN_FROM_CONFIG,
> +};
> +
>  struct xe_oa_reg {
>  	struct xe_reg addr;
>  	u32 value;
> @@ -1246,6 +1251,12 @@ static int xe_oa_set_prop_syncs_user(struct xe_oa *oa, u64 value,
>  	return 0;
>  }
>  
> +static int xe_oa_set_prop_ret_inval(struct xe_oa *oa, u64 value,
> +				    struct xe_oa_open_param *param)
> +{
> +	return -EINVAL;
> +}
> +
>  typedef int (*xe_oa_set_property_fn)(struct xe_oa *oa, u64 value,
>  				     struct xe_oa_open_param *param);
>  static const xe_oa_set_property_fn xe_oa_set_property_funcs[] = {


Perhaps we should consider renaming this to match the later funcs array.
"xe_oa_set_property_funcs_open" for example?
Just a suggestion, not blocking.


> @@ -1262,8 +1273,22 @@ static const xe_oa_set_property_fn xe_oa_set_property_funcs[] = {
>  	[DRM_XE_OA_PROPERTY_SYNCS] = xe_oa_set_prop_syncs_user,
>  };
>  
> -static int xe_oa_user_ext_set_property(struct xe_oa *oa, u64 extension,
> -				       struct xe_oa_open_param *param)
> +static const xe_oa_set_property_fn xe_oa_set_property_funcs_config[] = {
> +	[DRM_XE_OA_PROPERTY_OA_UNIT_ID] = xe_oa_set_prop_ret_inval,
> +	[DRM_XE_OA_PROPERTY_SAMPLE_OA] = xe_oa_set_prop_ret_inval,
> +	[DRM_XE_OA_PROPERTY_OA_METRIC_SET] = xe_oa_set_prop_metric_set,
> +	[DRM_XE_OA_PROPERTY_OA_FORMAT] = xe_oa_set_prop_ret_inval,
> +	[DRM_XE_OA_PROPERTY_OA_PERIOD_EXPONENT] = xe_oa_set_prop_ret_inval,
> +	[DRM_XE_OA_PROPERTY_OA_DISABLED] = xe_oa_set_prop_ret_inval,
> +	[DRM_XE_OA_PROPERTY_EXEC_QUEUE_ID] = xe_oa_set_prop_ret_inval,
> +	[DRM_XE_OA_PROPERTY_OA_ENGINE_INSTANCE] = xe_oa_set_prop_ret_inval,
> +	[DRM_XE_OA_PROPERTY_NO_PREEMPT] = xe_oa_set_prop_ret_inval,
> +	[DRM_XE_OA_PROPERTY_NUM_SYNCS] = xe_oa_set_prop_num_syncs,
> +	[DRM_XE_OA_PROPERTY_SYNCS] = xe_oa_set_prop_syncs_user,
> +};
> +
> +static int xe_oa_user_ext_set_property(struct xe_oa *oa, enum xe_oa_user_extn_from from,
> +				       u64 extension, struct xe_oa_open_param *param)
>  {
>  	u64 __user *address = u64_to_user_ptr(extension);
>  	struct drm_xe_ext_set_property ext;
> @@ -1279,18 +1304,22 @@ static int xe_oa_user_ext_set_property(struct xe_oa *oa, u64 extension,
>  		return -EINVAL;
>  
>  	idx = array_index_nospec(ext.property, ARRAY_SIZE(xe_oa_set_property_funcs));
> -	return xe_oa_set_property_funcs[idx](oa, ext.value, param);
> +
> +	if (from == XE_OA_USER_EXTN_FROM_CONFIG)
> +		return xe_oa_set_property_funcs_config[idx](oa, ext.value, param);
> +	else
> +		return xe_oa_set_property_funcs[idx](oa, ext.value, param);


If we want to extend this in the future, we might want a map between the
xe_oa_user_extn_from enum and the xe_oa_set_property_funcs_config.

Something like:

"""
static const xe_oa_set_property_fn xe_oa_set_property_funcs_list[][] {
	[XE_OA_USER_EXTN_FROM_OPEN] = xe_oa_set_property_funcs,
	[XE_OA_USER_EXTN_FROM_CONFIG] = xe_oa_set_property_funcs_config,
};
"""

Then, here, we'd just need to do something like:

"""
return xe_oa_set_property_funcs_list[from][idx](oa, ext.value, param);
"""

I don't think this is strictly necessary, though.
-Jonathan Cavitt


>  }
>  
> -typedef int (*xe_oa_user_extension_fn)(struct xe_oa *oa, u64 extension,
> -				       struct xe_oa_open_param *param);
> +typedef int (*xe_oa_user_extension_fn)(struct xe_oa *oa,  enum xe_oa_user_extn_from from,
> +				       u64 extension, struct xe_oa_open_param *param);
>  static const xe_oa_user_extension_fn xe_oa_user_extension_funcs[] = {
>  	[DRM_XE_OA_EXTENSION_SET_PROPERTY] = xe_oa_user_ext_set_property,
>  };
>  
>  #define MAX_USER_EXTENSIONS	16
> -static int xe_oa_user_extensions(struct xe_oa *oa, u64 extension, int ext_number,
> -				 struct xe_oa_open_param *param)
> +static int xe_oa_user_extensions(struct xe_oa *oa, enum xe_oa_user_extn_from from, u64 extension,
> +				 int ext_number, struct xe_oa_open_param *param)
>  {
>  	u64 __user *address = u64_to_user_ptr(extension);
>  	struct drm_xe_user_extension ext;
> @@ -1309,12 +1338,12 @@ static int xe_oa_user_extensions(struct xe_oa *oa, u64 extension, int ext_number
>  		return -EINVAL;
>  
>  	idx = array_index_nospec(ext.name, ARRAY_SIZE(xe_oa_user_extension_funcs));
> -	err = xe_oa_user_extension_funcs[idx](oa, extension, param);
> +	err = xe_oa_user_extension_funcs[idx](oa, from, extension, param);
>  	if (XE_IOCTL_DBG(oa->xe, err))
>  		return err;
>  
>  	if (ext.next_extension)
> -		return xe_oa_user_extensions(oa, ext.next_extension, ++ext_number, param);
> +		return xe_oa_user_extensions(oa, from, ext.next_extension, ++ext_number, param);
>  
>  	return 0;
>  }
> @@ -1460,7 +1489,7 @@ static long xe_oa_config_locked(struct xe_oa_stream *stream, u64 arg)
>  	struct xe_oa_config *config;
>  	int err;
>  
> -	err = xe_oa_user_extensions(stream->oa, arg, 0, &param);
> +	err = xe_oa_user_extensions(stream->oa, XE_OA_USER_EXTN_FROM_CONFIG, arg, 0, &param);
>  	if (err)
>  		return err;
>  
> @@ -2011,7 +2040,7 @@ int xe_oa_stream_open_ioctl(struct drm_device *dev, u64 data, struct drm_file *f
>  	}
>  
>  	param.xef = xef;
> -	ret = xe_oa_user_extensions(oa, data, 0, &param);
> +	ret = xe_oa_user_extensions(oa, XE_OA_USER_EXTN_FROM_OPEN, data, 0, &param);
>  	if (ret)
>  		return ret;
>  
> -- 
> 2.41.0
> 
>