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

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

On Tue, 20 Aug 2024 07:15:51 -0700, Cavitt, Jonathan wrote:
>

Hi Jonathan,

> -----Original Message-----
> From: Dixit, Ashutosh <ashutosh.dixit@intel.com>
> Sent: Monday, August 19, 2024 5:58 PM
> To: intel-xe@lists.freedesktop.org
> Cc: Brost, Matthew <matthew.brost@intel.com>; Souza, Jose <jose.souza@intel.com>; Landwerlin, Lionel G <lionel.g.landwerlin@intel.com>; Nerlige Ramappa, Umesh <umesh.nerlige.ramappa@intel.com>; Cavitt, Jonathan <jonathan.cavitt@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@intel.com>
>
>
> Minor nits below, but otherwise LGTM.
> Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>

Thanks!

>
> > ---
> >  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.

OK I went ahead and made this change in v3. Increases the patch footprint a
bit but makes it clearer.

>
>
> > @@ -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.

This one I was tempted to make too, but then I let it be. I don't think we
are expecting more code paths where we expect this to happen. But thanks
for the suggestion, it is a good one.

Thanks.
--
Ashutosh

> >  }
> >
> > -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
> >
> >