Re: [Intel-xe] [PATCH 20/21] drm/xe/uapi: Use OA unit id to identify OA unit

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

On Tue, Sep 19, 2023 at 09:10:48AM -0700, Ashutosh Dixit wrote:
>Previous uapi uses an indirect way (the engine class/instance of an engine
>connected to an OA unit) to identify an OA unit. Replace this by directly
>using the OA unit ID to identify the OA unit.
>
>With this change DRM_XE_OA_PROP_OA_ENGINE_CLASS property is not needed any
>more and removed. DRM_XE_OA_PROP_OA_ENGINE_INSTANCE is still used with
>DRM_XE_OA_PROP_EXEC_QUEUE_ID.
>
>Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>---
> drivers/gpu/drm/xe/xe_oa.c | 267 +++++++++++++++++++++----------------
> include/uapi/drm/xe_drm.h  |  22 ++-
> 2 files changed, 160 insertions(+), 129 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
>index 7cb900fc88f58..ded52d5aabea6 100644
>--- a/drivers/gpu/drm/xe/xe_oa.c
>+++ b/drivers/gpu/drm/xe/xe_oa.c
>@@ -60,9 +60,13 @@ static const struct xe_oa_format oa_formats[] = {
> };
>
> struct xe_oa_open_properties {
>+	u16 oa_unit_id;
> 	bool sample;
>+
> 	bool single_exec_q;
> 	u64 exec_q_id;
>+	struct xe_exec_queue *exec_q;
>+	u16 instance;
>
> 	int metrics_set;
> 	enum xe_oa_format_name oa_format;
>@@ -1287,6 +1291,7 @@ static int xe_oa_stream_init(struct xe_oa_stream *stream,
> 	struct xe_oa *oa = stream->oa;
> 	int ret;
>
>+	stream->exec_q = props->exec_q;
> 	stream->poll_oa_period = props->poll_oa_period;
> 	stream->hwe = props->hwe;
> 	stream->gt = stream->hwe->gt;
>@@ -1377,61 +1382,27 @@ static int xe_oa_stream_init(struct xe_oa_stream *stream,
> static int
> xe_oa_stream_open_ioctl_locked(struct xe_oa *oa,
> 			       struct drm_xe_oa_open_param *param,
>-			       struct xe_oa_open_properties *props,
>-			       struct drm_file *file)
>+			       struct xe_oa_open_properties *props)
> {
>-	struct xe_file *xef = to_xe_file(file);
>-	struct xe_oa_stream *stream = NULL;
>-	struct xe_exec_queue *q = NULL;
>+	struct xe_oa_stream *stream;
> 	unsigned long f_flags = 0;
>-	bool privileged_op = true;
> 	int stream_fd;
> 	int ret;
>
>-	if (props->single_exec_q) {
>-		q = xe_exec_queue_lookup(xef, props->exec_q_id);
>-		if (XE_IOCTL_DBG(oa->xe, !q)) {
>-			ret = -ENOENT;
>-			goto err_exec_q;
>-		}
>-	}
>-
>-	/*
>-	 * The OAR unit only monitors the RCS on a per context basis. Relax
>-	 * requirements if the user doesn't request global stream access,
>-	 * i.e. query based sampling using MI_REPORT_PERF_COUNT
>-	 */
>-	if (q && !props->sample)
>-		privileged_op = false;
>-
>-	if (privileged_op && xe_oa_stream_paranoid && !perfmon_capable()) {
>-		drm_dbg(&oa->xe->drm, "Insufficient privileges to open xe perf stream\n");
>-		ret = -EACCES;
>-		goto err_exec_q;
>-	}
>-
>-	if (!props->sample && !q) {
>-		drm_dbg(&oa->xe->drm, "Only OA report sampling supported\n");
>-		ret = -EINVAL;
>-		goto err_exec_q;
>-	}
>-
> 	/* We currently only allow exclusive access */
> 	if (props->hwe->oa_group->exclusive_stream) {
> 		drm_dbg(&oa->xe->drm, "OA unit already in use\n");
> 		ret = -EBUSY;
>-		goto err_exec_q;
>+		goto exit;
> 	}
>
> 	stream = kzalloc(sizeof(*stream), GFP_KERNEL);
> 	if (!stream) {
> 		ret = -ENOMEM;
>-		goto err_exec_q;
>+		goto exit;
> 	}
>
> 	stream->oa = oa;
>-	stream->exec_q = q;
>-
> 	ret = xe_oa_stream_init(stream, props);
> 	if (ret)
> 		goto err_free;
>@@ -1458,9 +1429,7 @@ xe_oa_stream_open_ioctl_locked(struct xe_oa *oa,
> 	xe_oa_stream_destroy(stream);
> err_free:
> 	kfree(stream);
>-err_exec_q:
>-	if (q)
>-		xe_exec_queue_put(q);
>+exit:
> 	return ret;
> }
>
>@@ -1502,11 +1471,6 @@ static u64 oa_exponent_to_ns(struct xe_gt *gt, int exponent)
> 	return div_u64(nom + den - 1, den);
> }
>
>-static bool engine_supports_oa(const struct xe_hw_engine *hwe)
>-{
>-	return hwe->oa_group;
>-}
>-
> static bool engine_supports_oa_format(const struct xe_hw_engine *hwe, int type)
> {
> 	switch (hwe->oa_group->type) {
>@@ -1537,20 +1501,134 @@ static int decode_oa_format(struct xe_oa *oa, u64 prop, enum xe_oa_format_name *
> 	return -EINVAL;
> }
>
>+u16 xe_oa_unit_id(struct xe_hw_engine *hwe)
>+{
>+	return hwe->oa_group && hwe->oa_group->num_engines ?
>+		hwe->oa_group->oa_unit_id : U16_MAX;
>+}
>+
>+static int xe_oa_assign_hwe(struct xe_oa *oa, struct xe_oa_open_properties *props)
>+{
>+	struct xe_gt *gt;
>+	int i, ret = 0;
>+
>+	if (props->exec_q) {
>+		/* When we have an exec_q, get hwe from the exec_q */
>+		for_each_gt(gt, oa->xe, i) {
>+			props->hwe = xe_gt_hw_engine(gt, props->exec_q->class,
>+						     props->instance, false);
>+			if (props->hwe)
>+				break;
>+		}
>+		if (props->hwe && (xe_oa_unit_id(props->hwe) != props->oa_unit_id)) {
>+			drm_dbg(&oa->xe->drm, "OA unit ID mismatch for exec_q\n");
>+			ret = -EINVAL;
>+		}
>+	} else {
>+		struct xe_hw_engine *hwe;
>+		enum xe_hw_engine_id id;
>+
>+		/* Else just get the first hwe attached to the oa unit */
>+		for_each_gt(gt, oa->xe, i) {
>+			for_each_hw_engine(hwe, gt, id) {
>+				if (xe_oa_unit_id(hwe) == props->oa_unit_id) {
>+					props->hwe = hwe;
>+					goto out;
>+				}
>+			}
>+		}
>+	}

I think both the if and else blocks above will get you the same hwe 
object, so you can just pick the else code to assign hwe.

Are we allowing the user to pass either oa_unit_id OR exec_q/instance? I 
think based on the HW requirement we should enable OAR/OAG together, so 
maybe enforce that the user passes both params. Thoughts?

Thanks,
Umesh