[PATCH 8/8] drm/xe/oa: Allow only certain property changes from config ioctl
Dixit, Ashutosh
ashutosh.dixit at intel.com
Tue Aug 20 01:45:45 UTC 2024
On Thu, 08 Aug 2024 15:15:46 -0700, Cavitt, Jonathan wrote:
>
Hi Jonathan,
> -----Original Message-----
> From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of Ashutosh Dixit
> Sent: Thursday, August 8, 2024 10:42 AM
> To: intel-xe at lists.freedesktop.org
> Cc: Nerlige Ramappa, Umesh <umesh.nerlige.ramappa at intel.com>; Souza, Jose <jose.souza at intel.com>; Landwerlin, Lionel G <lionel.g.landwerlin at 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 at 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, ¶m->oa_format);
> > + int ret;
> >
> > + if (!all)
> > + return -EINVAL;
> > +
> > + ret = decode_oa_format(oa, value, ¶m->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 at 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, ¶m);
> > + err = xe_oa_user_extensions(stream->oa, arg, false, 0, ¶m);
> > 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, ¶m);
> > + ret = xe_oa_user_extensions(oa, data, true, 0, ¶m);
> > if (ret)
> > return ret;
> >
> > --
> > 2.41.0
> >
> >
More information about the Intel-xe
mailing list