[PATCH] drm/xe/oa/uapi: Allow OA config's to have arbitrary names/id's

[Cavitt, Jonathan]

-----Original Message-----
From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of Ashutosh Dixit
Sent: Tuesday, July 9, 2024 6:18 PM
To: intel-xe at lists.freedesktop.org
Cc: Landwerlin, Lionel G <lionel.g.landwerlin at intel.com>; Nerlige Ramappa, Umesh <umesh.nerlige.ramappa at intel.com>; Krzemien, Robert <robert.krzemien at intel.com>; Souza, Jose <jose.souza at intel.com>
Subject: [PATCH] drm/xe/oa/uapi: Allow OA config's to have arbitrary names/id's
> 
> Added OA configs were previously identified by means of a uuid. Rather than
> forcing userspace to use uuid, here we generalize this scheme to allow
> userspace the flexibility to use whatever naming scheme (including uuid)
> they may wish to use to identify and track OA config's. Names must still be
> unique or will be rejected.
> 
> Fixes: cdf02fe1a94a ("drm/xe/oa/uapi: Add/remove OA config perf ops")
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit at intel.com>

I read the extended explanation in reply and agree this is a good
change.  It might be best to try and move parts of that explanation
into the commit message for added clarity, but otherwise:
Reviewed-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
-Jonathan Cavitt

> ---
>  drivers/gpu/drm/xe/xe_oa.c | 30 ++++++++++++------------------
>  include/uapi/drm/xe_drm.h  |  4 ++--
>  2 files changed, 14 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> index 6d69f751bf78..4f00577e92b4 100644
> --- a/drivers/gpu/drm/xe/xe_oa.c
> +++ b/drivers/gpu/drm/xe/xe_oa.c
> @@ -49,7 +49,7 @@ struct xe_oa_reg {
>  struct xe_oa_config {
>  	struct xe_oa *oa;
>  
> -	char uuid[UUID_STRING_LEN + 1];
> +	char name[UUID_STRING_LEN + 1];
>  	int id;
>  
>  	const struct xe_oa_reg *regs;
> @@ -899,8 +899,8 @@ xe_oa_alloc_config_buffer(struct xe_oa_stream *stream, struct xe_oa_config *oa_c
>  	/* Look for the buffer in the already allocated BOs attached to the stream */
>  	llist_for_each_entry(oa_bo, stream->oa_config_bos.first, node) {
>  		if (oa_bo->oa_config == oa_config &&
> -		    memcmp(oa_bo->oa_config->uuid, oa_config->uuid,
> -			   sizeof(oa_config->uuid)) == 0)
> +		    memcmp(oa_bo->oa_config->name, oa_config->name,
> +			   sizeof(oa_config->name)) == 0)
>  			goto out;
>  	}
>  
> @@ -1430,8 +1430,8 @@ static int xe_oa_stream_init(struct xe_oa_stream *stream,
>  		goto err_put_k_exec_q;
>  	}
>  
> -	drm_dbg(&stream->oa->xe->drm, "opening stream oa config uuid=%s\n",
> -		stream->oa_config->uuid);
> +	drm_dbg(&stream->oa->xe->drm, "opening stream oa config name=%s\n",
> +		stream->oa_config->name);
>  
>  	WRITE_ONCE(u->exclusive_stream, stream);
>  
> @@ -2066,7 +2066,7 @@ static int create_dynamic_oa_sysfs_entry(struct xe_oa *oa,
>  	oa_config->attrs[0] = &oa_config->sysfs_metric_id.attr;
>  	oa_config->attrs[1] = NULL;
>  
> -	oa_config->sysfs_metric.name = oa_config->uuid;
> +	oa_config->sysfs_metric.name = oa_config->name;
>  	oa_config->sysfs_metric.attrs = oa_config->attrs;
>  
>  	return sysfs_create_group(oa->metrics_kobj, &oa_config->sysfs_metric);
> @@ -2118,14 +2118,8 @@ int xe_oa_add_config_ioctl(struct drm_device *dev, u64 data, struct drm_file *fi
>  	oa_config->oa = oa;
>  	kref_init(&oa_config->ref);
>  
> -	if (!uuid_is_valid(arg->uuid)) {
> -		drm_dbg(&oa->xe->drm, "Invalid uuid format for OA config\n");
> -		err = -EINVAL;
> -		goto reg_err;
> -	}
> -
> -	/* Last character in oa_config->uuid will be 0 because oa_config is kzalloc */
> -	memcpy(oa_config->uuid, arg->uuid, sizeof(arg->uuid));
> +	/* Last character in oa_config->name will be 0 because oa_config is kzalloc */
> +	memcpy(oa_config->name, arg->name, sizeof(arg->name));
>  
>  	oa_config->regs_len = arg->n_regs;
>  	regs = xe_oa_alloc_regs(oa, xe_oa_is_valid_config_reg_addr,
> @@ -2144,8 +2138,8 @@ int xe_oa_add_config_ioctl(struct drm_device *dev, u64 data, struct drm_file *fi
>  
>  	/* We shouldn't have too many configs, so this iteration shouldn't be too costly */
>  	idr_for_each_entry(&oa->metrics_idr, tmp, id) {
> -		if (!strcmp(tmp->uuid, oa_config->uuid)) {
> -			drm_dbg(&oa->xe->drm, "OA config already exists with this uuid\n");
> +		if (!strcmp(tmp->name, oa_config->name)) {
> +			drm_dbg(&oa->xe->drm, "OA config already exists with this name\n");
>  			err = -EADDRINUSE;
>  			goto sysfs_err;
>  		}
> @@ -2166,7 +2160,7 @@ int xe_oa_add_config_ioctl(struct drm_device *dev, u64 data, struct drm_file *fi
>  
>  	mutex_unlock(&oa->metrics_lock);
>  
> -	drm_dbg(&oa->xe->drm, "Added config %s id=%i\n", oa_config->uuid, oa_config->id);
> +	drm_dbg(&oa->xe->drm, "Added config %s id=%i\n", oa_config->name, oa_config->id);
>  
>  	return oa_config->id;
>  
> @@ -2224,7 +2218,7 @@ int xe_oa_remove_config_ioctl(struct drm_device *dev, u64 data, struct drm_file
>  
>  	mutex_unlock(&oa->metrics_lock);
>  
> -	drm_dbg(&oa->xe->drm, "Removed config %s id=%i\n", oa_config->uuid, oa_config->id);
> +	drm_dbg(&oa->xe->drm, "Removed config %s id=%i\n", oa_config->name, oa_config->id);
>  
>  	xe_oa_config_put(oa_config);
>  
> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> index 19619d4952a8..43bcae64a7b0 100644
> --- a/include/uapi/drm/xe_drm.h
> +++ b/include/uapi/drm/xe_drm.h
> @@ -1637,8 +1637,8 @@ struct drm_xe_oa_config {
>  	/** @extensions: Pointer to the first extension struct, if any */
>  	__u64 extensions;
>  
> -	/** @uuid: String formatted like "%\08x-%\04x-%\04x-%\04x-%\012x" */
> -	char uuid[36];
> +	/** @name: Unique config name/id */
> +	char name[36];
>  
>  	/** @n_regs: Number of regs in @regs_ptr */
>  	__u32 n_regs;
> -- 
> 2.41.0
> 
>