Re: [PATCH 13/16] drm/xe/oa/uapi: Query OA unit properties

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

On Wed, Feb 07, 2024 at 09:49:13PM -0800, Ashutosh Dixit wrote:
>Implement query for properties of OA units present on a device.
>
>v2: Clean up reserved/pad fields (Umesh)
>    Follow the same scheme as other query structs
>v3: Skip reporting reserved engines attached to OA units
>v4: Expose oa_buf_size via DRM_XE_PERF_IOCTL_INFO (Umesh)
>
>Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>---
> drivers/gpu/drm/xe/xe_oa.c    | 13 ++++++
> drivers/gpu/drm/xe/xe_query.c | 79 +++++++++++++++++++++++++++++++++++
> include/uapi/drm/xe_drm.h     | 75 +++++++++++++++++++++++++++++++++
> 3 files changed, 167 insertions(+)
>
>diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
>index dc138e5a10087..1178620cd4e76 100644
>--- a/drivers/gpu/drm/xe/xe_oa.c
>+++ b/drivers/gpu/drm/xe/xe_oa.c
>@@ -1018,6 +1018,17 @@ static long xe_oa_status_locked(struct xe_oa_stream *stream, unsigned long arg)
> 	return 0;
> }
>
>+static long xe_oa_info_locked(struct xe_oa_stream *stream, unsigned long arg)
>+{
>+	struct drm_xe_oa_stream_info info = { .oa_buf_size = XE_OA_BUFFER_SIZE, };
>+	void __user *uaddr = (void __user *)arg;
>+
>+	if (copy_to_user(uaddr, &info, sizeof(info)))
>+		return -EFAULT;
>+
>+	return 0;
>+}
>+
> static long xe_oa_ioctl_locked(struct xe_oa_stream *stream,
> 			       unsigned int cmd,
> 			       unsigned long arg)
>@@ -1033,6 +1044,8 @@ static long xe_oa_ioctl_locked(struct xe_oa_stream *stream,
> 		return xe_oa_config_locked(stream, arg);
> 	case DRM_XE_PERF_IOCTL_STATUS:
> 		return xe_oa_status_locked(stream, arg);
>+	case DRM_XE_PERF_IOCTL_INFO:
>+		return xe_oa_info_locked(stream, arg);
> 	}
>
> 	return -EINVAL;
>diff --git a/drivers/gpu/drm/xe/xe_query.c b/drivers/gpu/drm/xe/xe_query.c
>index 4f1ab91dbec58..828be65a076d6 100644
>--- a/drivers/gpu/drm/xe/xe_query.c
>+++ b/drivers/gpu/drm/xe/xe_query.c
>@@ -516,6 +516,84 @@ static int query_gt_topology(struct xe_device *xe,
> 	return 0;
> }
>
>+static size_t calc_oa_unit_query_size(struct xe_device *xe)
>+{
>+	size_t size = sizeof(struct drm_xe_query_oa_units);
>+	struct xe_gt *gt;
>+	int i, id;
>+
>+	for_each_gt(gt, xe, id) {
>+		for (i = 0; i < gt->oa.num_oa_units; i++) {
>+			size += sizeof(struct drm_xe_oa_unit);
>+			size += gt->oa.oa_unit[i].num_engines *
>+				sizeof(struct drm_xe_engine_class_instance);
>+		}
>+	}
>+
>+	return size;
>+}
>+
>+static int query_oa_units(struct xe_device *xe,
>+			  struct drm_xe_device_query *query)
>+{
>+	void __user *query_ptr = u64_to_user_ptr(query->data);
>+	size_t size = calc_oa_unit_query_size(xe);
>+	struct drm_xe_query_oa_units *qoa;
>+	enum xe_hw_engine_id hwe_id;
>+	struct drm_xe_oa_unit *du;
>+	struct xe_hw_engine *hwe;
>+	struct xe_oa_unit *u;
>+	int gt_id, i, j, ret;
>+	struct xe_gt *gt;
>+	u8 *pdu;
>+
>+	if (query->size == 0) {
>+		query->size = size;
>+		return 0;
>+	} else if (XE_IOCTL_DBG(xe, query->size != size)) {
>+		return -EINVAL;
>+	}
>+
>+	qoa = kzalloc(size, GFP_KERNEL);
>+	if (!qoa)
>+		return -ENOMEM;
>+
>+	pdu = (u8 *)&qoa->oa_units[0];
>+	for_each_gt(gt, xe, gt_id) {
>+		for (i = 0; i < gt->oa.num_oa_units; i++) {
>+			u = &gt->oa.oa_unit[i];
>+			du = (struct drm_xe_oa_unit *)pdu;
>+
>+			du->oa_unit_id = u->oa_unit_id;
>+			du->oa_unit_type = u->type;
>+			du->oa_timestamp_freq = xe_oa_timestamp_frequency(gt);
>+
>+			for (j = 1; j < DRM_XE_OA_PROPERTY_MAX; j++)
>+				du->capabilities |= BIT(j);
>+
>+			j = 0;
>+			for_each_hw_engine(hwe, gt, hwe_id) {
>+				if (!xe_hw_engine_is_reserved(hwe) &&
>+				    xe_oa_unit_id(hwe) == u->oa_unit_id) {
>+					du->eci[j].engine_class =
>+						xe_to_user_engine_class[hwe->class];
>+					du->eci[j].engine_instance = hwe->logical_instance;
>+					du->eci[j].gt_id = gt->info.id;
>+					j++;
>+				}
>+			}
>+			du->num_engines = j;
>+			pdu += sizeof(*du) + j * sizeof(du->eci[0]);
>+			qoa->num_oa_units++;
>+		}
>+	}
>+
>+	ret = copy_to_user(query_ptr, qoa, size);
>+	kfree(qoa);
>+
>+	return ret ? -EFAULT : 0;
>+}
>+
> static int (* const xe_query_funcs[])(struct xe_device *xe,
> 				      struct drm_xe_device_query *query) = {
> 	query_engines,
>@@ -525,6 +603,7 @@ static int (* const xe_query_funcs[])(struct xe_device *xe,
> 	query_hwconfig,
> 	query_gt_topology,
> 	query_engine_cycles,
>+	query_oa_units,
> };
>
> int xe_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
>index df0dfd60b52fa..bd535343a406c 100644
>--- a/include/uapi/drm/xe_drm.h
>+++ b/include/uapi/drm/xe_drm.h
>@@ -646,6 +646,7 @@ struct drm_xe_device_query {
> #define DRM_XE_DEVICE_QUERY_HWCONFIG		4
> #define DRM_XE_DEVICE_QUERY_GT_TOPOLOGY		5
> #define DRM_XE_DEVICE_QUERY_ENGINE_CYCLES	6
>+#define DRM_XE_DEVICE_QUERY_OA_UNITS		7
> 	/** @query: The type of data to query */
> 	__u32 query;
>
>@@ -1411,6 +1412,68 @@ enum drm_xe_oa_unit_type {
> 	DRM_XE_OA_UNIT_TYPE_OAM,
> };
>
>+/**
>+ * struct drm_xe_oa_unit - describe OA unit
>+ */
>+struct drm_xe_oa_unit {
>+	/** @oa_unit_id: OA unit ID */
>+	__u32 oa_unit_id;
>+
>+	/** @oa_unit_type: OA unit type of @drm_xe_oa_unit_type */
>+	__u32 oa_unit_type;
>+
>+	/**
>+	 * @capabilities: OA capabilities bit-mask: this is a bit-mask of
>+	 * property id's in enum @drm_xe_oa_property_id
>+	 */
>+	__u64 capabilities;

IMO, this should be a specific set of flags that are defined as and when 
we add new features so that it helps UMDs detect them. Some features or 
behaviors may be independent of the properties, so a 1:1 mapping to 
properties may not be the best solution. In the past we have had i915 
perf revision bumps even when we haven't added a new property (For ex: 
when we supported media engines in i915 - class:instance properties were 
already present and we just added support for OAM in that case. For such 
cases capabilities would define a new bit, but not properties).

The first set of properties defined with this series should not need a 
capabilities entry. If the uApi changes in future or adds new features, 
we need to use these bits.

Thanks,
Umesh

>+
>+	/** @oa_timestamp_freq: OA timestamp freq */
>+	__u64 oa_timestamp_freq;
>+
>+	/** @reserved: MBZ */
>+	__u64 reserved[4];
>+
>+	/** @num_engines: number of engines in @eci array */
>+	__u64 num_engines;
>+
>+	/** @eci: engines attached to this OA unit */
>+	struct drm_xe_engine_class_instance eci[];
>+};
>+
>+/**
>+ * struct drm_xe_query_oa_units - describe OA units
>+ *
>+ * If a query is made with a struct drm_xe_device_query where .query
>+ * is equal to DRM_XE_DEVICE_QUERY_OA_UNITS, then the reply uses struct
>+ * drm_xe_query_oa_units in .data.
>+ *
>+ * OA unit properties for all OA units can be accessed using a code block
>+ * such as the one below:
>+ *
>+ * .. code-block:: C
>+ *
>+ *	struct drm_xe_query_oa_units *qoa;
>+ *	struct drm_xe_oa_unit *oau;
>+ *	u8 *poau;
>+ *
>+ *	// malloc qoa and issue DRM_XE_DEVICE_QUERY_OA_UNITS. Then:
>+ *	poau = (u8 *)&qoa->oa_units[0];
>+ *	for (int i = 0; i < qoa->num_oa_units; i++) {
>+ *		oau = (struct drm_xe_oa_unit *)poau;
>+ *		// Access 'struct drm_xe_oa_unit' fields here
>+ *		poau += sizeof(*oau) + oau->num_engines * sizeof(oau->eci[0]);
>+ *	}
>+ */
>+struct drm_xe_query_oa_units {
>+	/** @num_oa_units: number of OA units returned in oau[] */
>+	__u32 num_oa_units;
>+	/** @pad: MBZ */
>+	__u32 pad;
>+	/** @oa_units: OA units returned for this device */
>+	struct drm_xe_oa_unit oa_units[];
>+};
>+
> /** enum drm_xe_oa_format_type - OA format types */
> enum drm_xe_oa_format_type {
> 	DRM_XE_OA_FMT_TYPE_OAG,
>@@ -1527,6 +1590,18 @@ struct drm_xe_oa_stream_status {
> 	__u64 reserved[3];
> };
>
>+/**
>+ * struct drm_xe_oa_stream_info - OA stream info returned from
>+ * @DRM_XE_PERF_IOCTL_INFO perf fd ioctl
>+ */
>+struct drm_xe_oa_stream_info {
>+	/** @oa_buf_size: OA buffer size */
>+	__u64 oa_buf_size;
>+
>+	/** @reserved */
>+	__u64 reserved[3];
>+};
>+
> #if defined(__cplusplus)
> }
> #endif
>-- 
>2.41.0
>