[PATCH 8/8] drm/xe/oa: Allow only certain property changes from config ioctl
Cavitt, Jonathan
jonathan.cavitt at intel.com
Thu Aug 8 22:15:46 UTC 2024
-----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.
-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