Re: [PATCH 8/8] drm/xe/oa: Allow only certain property changes from config ioctl

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

On Thu, 08 Aug 2024 15:15:46 -0700, Cavitt, Jonathan wrote:
>

Hi Jonathan,

> -----Original Message-----
> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Ashutosh Dixit
> Sent: Thursday, August 8, 2024 10:42 AM
> To: intel-xe@lists.freedesktop.org
> Cc: Nerlige Ramappa, Umesh <umesh.nerlige.ramappa@intel.com>; Souza, Jose <jose.souza@intel.com>; Landwerlin, Lionel G <lionel.g.landwerlin@intel.com>
> Subject: [PATCH 8/8] drm/xe/oa: Allow only certain property changes from config ioctl
> >
> > 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.
> >
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_oa.c | 69 ++++++++++++++++++++++++++------------
> >  1 file changed, 47 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> > index c9f77f0342b60..27654a0d4358f 100644
> > --- a/drivers/gpu/drm/xe/xe_oa.c
> > +++ b/drivers/gpu/drm/xe/xe_oa.c
> > @@ -1133,9 +1133,12 @@ static int decode_oa_format(struct xe_oa *oa, u64 fmt, enum xe_oa_format_name *n
> >	return -EINVAL;
> >  }
> >
> > -static int xe_oa_set_prop_oa_unit_id(struct xe_oa *oa, u64 value,
> > +static int xe_oa_set_prop_oa_unit_id(struct xe_oa *oa, u64 value, bool all,
> >				     struct xe_oa_open_param *param)
> >  {
> > +	if (!all)
> > +		return -EINVAL;
> > +
> >	if (value >= oa->oa_unit_ids) {
> >		drm_dbg(&oa->xe->drm, "OA unit ID out of range %lld\n", value);
> >		return -EINVAL;
> > @@ -1144,25 +1147,32 @@ static int xe_oa_set_prop_oa_unit_id(struct xe_oa *oa, u64 value,
> >	return 0;
> >  }
> >
> > -static int xe_oa_set_prop_sample_oa(struct xe_oa *oa, u64 value,
> > +static int xe_oa_set_prop_sample_oa(struct xe_oa *oa, u64 value, bool all,
> >				    struct xe_oa_open_param *param)
> >  {
> > +	if (!all)
> > +		return -EINVAL;
> > +
> >	param->sample = value;
> >	return 0;
> >  }
> >
> > -static int xe_oa_set_prop_metric_set(struct xe_oa *oa, u64 value,
> > +static int xe_oa_set_prop_metric_set(struct xe_oa *oa, u64 value, bool all,
> >				     struct xe_oa_open_param *param)
> >  {
> >	param->metric_set = value;
> >	return 0;
> >  }
> >
> > -static int xe_oa_set_prop_oa_format(struct xe_oa *oa, u64 value,
> > +static int xe_oa_set_prop_oa_format(struct xe_oa *oa, u64 value, bool all,
> >				    struct xe_oa_open_param *param)
> >  {
> > -	int ret = decode_oa_format(oa, value, &param->oa_format);
> > +	int ret;
> >
> > +	if (!all)
> > +		return -EINVAL;
> > +
> > +	ret = decode_oa_format(oa, value, &param->oa_format);
> >	if (ret) {
> >		drm_dbg(&oa->xe->drm, "Unsupported OA report format %#llx\n", value);
> >		return ret;
> > @@ -1170,11 +1180,14 @@ static int xe_oa_set_prop_oa_format(struct xe_oa *oa, u64 value,
> >	return 0;
> >  }
> >
> > -static int xe_oa_set_prop_oa_exponent(struct xe_oa *oa, u64 value,
> > +static int xe_oa_set_prop_oa_exponent(struct xe_oa *oa, u64 value, bool all,
> >				      struct xe_oa_open_param *param)
> >  {
> >  #define OA_EXPONENT_MAX 31
> >
> > +	if (!all)
> > +		return -EINVAL;
> > +
> >	if (value > OA_EXPONENT_MAX) {
> >		drm_dbg(&oa->xe->drm, "OA timer exponent too high (> %u)\n", OA_EXPONENT_MAX);
> >		return -EINVAL;
> > @@ -1183,49 +1196,61 @@ static int xe_oa_set_prop_oa_exponent(struct xe_oa *oa, u64 value,
> >	return 0;
> >  }
> >
> > -static int xe_oa_set_prop_disabled(struct xe_oa *oa, u64 value,
> > +static int xe_oa_set_prop_disabled(struct xe_oa *oa, u64 value, bool all,
> >				   struct xe_oa_open_param *param)
> >  {
> > +	if (!all)
> > +		return -EINVAL;
> > +
> >	param->disabled = value;
> >	return 0;
> >  }
> >
> > -static int xe_oa_set_prop_exec_queue_id(struct xe_oa *oa, u64 value,
> > +static int xe_oa_set_prop_exec_queue_id(struct xe_oa *oa, u64 value, bool all,
> >					struct xe_oa_open_param *param)
> >  {
> > +	if (!all)
> > +		return -EINVAL;
> > +
> >	param->exec_queue_id = value;
> >	return 0;
> >  }
> >
> > -static int xe_oa_set_prop_engine_instance(struct xe_oa *oa, u64 value,
> > +static int xe_oa_set_prop_engine_instance(struct xe_oa *oa, u64 value, bool all,
> >					  struct xe_oa_open_param *param)
> >  {
> > +	if (!all)
> > +		return -EINVAL;
> > +
> >	param->engine_instance = value;
> >	return 0;
> >  }
> >
> > -static int xe_oa_set_no_preempt(struct xe_oa *oa, u64 value,
> > +static int xe_oa_set_no_preempt(struct xe_oa *oa, u64 value, bool all,
> >				struct xe_oa_open_param *param)
> >  {
> > +	if (!all)
> > +		return -EINVAL;
> > +
> >	param->no_preempt = value;
> >	return 0;
> >  }
> >
> > -static int xe_oa_set_num_syncs(struct xe_oa *oa, u64 value,
> > +static int xe_oa_set_num_syncs(struct xe_oa *oa, u64 value, bool all,
> >			       struct xe_oa_open_param *param)
> >  {
> >	param->num_syncs = value;
> >	return 0;
> >  }
> >
> > -static int xe_oa_set_syncs_user(struct xe_oa *oa, u64 value,
> > +static int xe_oa_set_syncs_user(struct xe_oa *oa, u64 value, bool all,
> >				struct xe_oa_open_param *param)
> >  {
> >	param->syncs_user = u64_to_user_ptr(value);
> >	return 0;
> >  }
> >
> > -typedef int (*xe_oa_set_property_fn)(struct xe_oa *oa, u64 value,
> > +typedef int (*xe_oa_set_property_fn)(struct xe_oa *oa, u64 value, bool all,
> >				     struct xe_oa_open_param *param);
> >  static const xe_oa_set_property_fn xe_oa_set_property_funcs[] = {
> >	[DRM_XE_OA_PROPERTY_OA_UNIT_ID] = xe_oa_set_prop_oa_unit_id,
> > @@ -1241,7 +1266,7 @@ static const xe_oa_set_property_fn xe_oa_set_property_funcs[] = {
> >	[DRM_XE_OA_PROPERTY_SYNCS] = xe_oa_set_syncs_user,
> >  };
> >
> > -static int xe_oa_user_ext_set_property(struct xe_oa *oa, u64 extension,
> > +static int xe_oa_user_ext_set_property(struct xe_oa *oa, u64 extension, bool all,
> >				       struct xe_oa_open_param *param)
> >  {
> >	u64 __user *address = u64_to_user_ptr(extension);
> > @@ -1258,18 +1283,18 @@ 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);
> > +	return xe_oa_set_property_funcs[idx](oa, ext.value, all, param);
>
> We might be able to reduce the amount of times the "all" Boolean param needs to get
> propagated if we had a separate xe_oa_set_property_fn array for the reconfiguration
> case.  Say:
>
> """
> static int xe_oa_set_prop_inval(struct xe_oa *oa, u64 value,
>				  struct xe_oa_open_param *param)
> {
>	return -EINVAL;
> }
>
> static const xe_oa_set_property_fn xe_oa_set_property_funcs_recon[] = {
>	[DRM_XE_OA_PROPERTY_OA_UNIT_ID] = xe_oa_set_prop_inval,
>	[DRM_XE_OA_PROPERTY_SAMPLE_OA] = xe_oa_set_prop_inval,
>	[DRM_XE_OA_PROPERTY_OA_METRIC_SET] = xe_oa_set_prop_metric_set,
>	[DRM_XE_OA_PROPERTY_OA_FORMAT] = xe_oa_set_prop_inval,
>	[DRM_XE_OA_PROPERTY_OA_PERIOD_EXPONENT] = xe_oa_set_prop_inval,
>	[DRM_XE_OA_PROPERTY_OA_DISABLED] = xe_oa_set_prop_inval,
>	[DRM_XE_OA_PROPERTY_EXEC_QUEUE_ID] = xe_oa_set_prop_inval,
>	[DRM_XE_OA_PROPERTY_OA_ENGINE_INSTANCE] = xe_oa_set_prop_inval,
>	[DRM_XE_OA_PROPERTY_NO_PREEMPT] = xe_oa_set_prop_inval,
>	[DRM_XE_OA_PROPERTY_NUM_SYNCS] = xe_oa_set_num_syncs,
>	[DRM_XE_OA_PROPERTY_SYNCS] = xe_oa_set_syncs_user,
> };
> """
>
> Then from there, say:
>
> -	return xe_oa_set_property_funcs[idx](oa, ext.value, param);
> +	if (all)
> +		return xe_oa_set_property_funcs[idx](oa, ext.value, param);
> +	else
> +		return xe_oa_set_property_funcs_recon[idx](oa, ext.value, param);
>
> I guess at that point, we'd want to rename oa_set_property_funcs to something else
> (such as "xe_oa_set_property_funcs_all") for clarity.
>
> The alternative would be to rename "all" to "open" so that later down the execution
> stack, it remains clear that we're blocking the execution of the various functions in the
> reconfiguration case (or the "not open" case, if you will).
>
> Either change by itself would be sufficient, I think.

I liked your suggestion so have now implemented almost exactly this in
v2. Forgot to add:

Suggested-by: Jonathan Cavitt <jonathan.cavitt@intel.com>

Will add. The only other minor change was it seems bool's are not welcome
so I had to introduce an enum. Please take a look again.

Thanks.
--
Ashutosh



>
> -Jonathan Cavitt
>
> >  }
> >
> > -typedef int (*xe_oa_user_extension_fn)(struct xe_oa *oa, u64 extension,
> > +typedef int (*xe_oa_user_extension_fn)(struct xe_oa *oa, u64 extension, bool all,
> >				       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, u64 extension, bool all,
> > +				 int ext_number, struct xe_oa_open_param *param)
> >  {
> >	u64 __user *address = u64_to_user_ptr(extension);
> >	struct drm_xe_user_extension ext;
> > @@ -1288,12 +1313,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, extension, all, 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, ext.next_extension, all, ++ext_number, param);
> >
> >	return 0;
> >  }
> > @@ -1439,7 +1464,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, arg, false, 0, &param);
> >	if (err)
> >		return err;
> >
> > @@ -1989,7 +2014,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, data, true, 0, &param);
> >	if (ret)
> >		return ret;
> >
> > --
> > 2.41.0
> >
> >