[Intel-gfx] [RFC] drm/i915/perf: Simplify perf query implementation

* [Intel-gfx] [RFC] drm/i915/perf: Simplify perf query implementation
@ 2023-09-28 10:06 Tvrtko Ursulin
  2023-09-28 12:27 ` [Intel-gfx] ✗ Fi.CI.BUILD: warning for " Patchwork
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Tvrtko Ursulin @ 2023-09-28 10:06 UTC (permalink / raw)
  To: Intel-gfx, dri-devel; +Cc: Rodrigo Vivi

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Simplify the implementation of perf/OA queries by re-ogranizing the way
querying of the OA config list is done, how temporary storage is used,
and also replace the multi-step manual retrieving of user data with the
existing copy_query_item helper.

Note that this could be further simplified by not having a temporary
array in query_perf_config_list() but directly doing put_user() as we walk
the list of configs.

Compile tested only!

If it works it does almost halve the number of lines needed to do these
queries. But let me know first please if there is interest to simplify or
just leave as is.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
 drivers/gpu/drm/i915/i915_query.c | 120 +++++++++++-------------------
 1 file changed, 44 insertions(+), 76 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
index ec658fc52834..23e2fe318c4d 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -227,17 +227,14 @@ static int query_perf_config_data(struct drm_i915_private *i915,
 				  struct drm_i915_query_item *query_item,
 				  bool use_uuid)
 {
-	struct drm_i915_query_perf_config __user *user_query_config_ptr =
-		u64_to_user_ptr(query_item->data_ptr);
 	struct drm_i915_perf_oa_config __user *user_config_ptr =
 		u64_to_user_ptr(query_item->data_ptr +
 				sizeof(struct drm_i915_query_perf_config));
+	struct drm_i915_query_perf_config query_config;
 	struct drm_i915_perf_oa_config user_config;
 	struct i915_perf *perf = &i915->perf;
 	struct i915_oa_config *oa_config;
-	char uuid[UUID_STRING_LEN + 1];
-	u64 config_id;
-	u32 flags, total_size;
+	u32 total_size;
 	int ret;
 
 	if (!perf->i915)
@@ -247,47 +244,29 @@ static int query_perf_config_data(struct drm_i915_private *i915,
 		sizeof(struct drm_i915_query_perf_config) +
 		sizeof(struct drm_i915_perf_oa_config);
 
-	if (query_item->length == 0)
-		return total_size;
+	ret = copy_query_item(&query_config, sizeof(query_config), total_size,
+			      query_item);
+	if (ret != 0)
+		return ret;
 
-	if (query_item->length < total_size) {
-		drm_dbg(&i915->drm,
-			"Invalid query config data item size=%u expected=%u\n",
-			query_item->length, total_size);
-		return -EINVAL;
-	}
-
-	if (get_user(flags, &user_query_config_ptr->flags))
-		return -EFAULT;
-
-	if (flags != 0)
+	if (query_config.flags != 0)
 		return -EINVAL;
 
 	if (use_uuid) {
 		struct i915_oa_config *tmp;
 		int id;
 
-		BUILD_BUG_ON(sizeof(user_query_config_ptr->uuid) >= sizeof(uuid));
-
-		memset(&uuid, 0, sizeof(uuid));
-		if (copy_from_user(uuid, user_query_config_ptr->uuid,
-				     sizeof(user_query_config_ptr->uuid)))
-			return -EFAULT;
-
 		oa_config = NULL;
 		rcu_read_lock();
 		idr_for_each_entry(&perf->metrics_idr, tmp, id) {
-			if (!strcmp(tmp->uuid, uuid)) {
+			if (!strcmp(tmp->uuid, query_config.uuid)) {
 				oa_config = i915_oa_config_get(tmp);
 				break;
 			}
 		}
 		rcu_read_unlock();
 	} else {
-		if (get_user(config_id, &user_query_config_ptr->config))
-			return -EFAULT;
-
-		oa_config = i915_perf_get_oa_config(perf, config_id);
+		oa_config = i915_perf_get_oa_config(perf, query_config.config);
 	}
 	if (!oa_config)
 		return -ENOENT;
@@ -355,7 +334,7 @@ static size_t sizeof_perf_config_list(size_t count)
 	return sizeof(struct drm_i915_query_perf_config) + sizeof(u64) * count;
 }
 
-static size_t sizeof_perf_metrics(struct i915_perf *perf)
+static size_t sizeof_perf_metrics(struct i915_perf *perf, u64 *n_configs)
 {
 	struct i915_oa_config *tmp;
 	size_t i;
@@ -367,6 +346,8 @@ static size_t sizeof_perf_metrics(struct i915_perf *perf)
 		i++;
 	rcu_read_unlock();
 
+	*n_configs = i;
+
 	return sizeof_perf_config_list(i);
 }
 
@@ -375,72 +356,59 @@ static int query_perf_config_list(struct drm_i915_private *i915,
 {
 	struct drm_i915_query_perf_config __user *user_query_config_ptr =
 		u64_to_user_ptr(query_item->data_ptr);
+	struct drm_i915_query_perf_config query_config;
 	struct i915_perf *perf = &i915->perf;
-	u64 *oa_config_ids = NULL;
-	int alloc, n_configs;
-	u32 flags;
-	int ret;
+	u64 *oa_config_ids, *ids, n_configs;
+	struct i915_oa_config *tmp;
+	u32 total_size;
+	int ret, id;
 
 	if (!perf->i915)
 		return -ENODEV;
 
-	if (query_item->length == 0)
-		return sizeof_perf_metrics(perf);
+	total_size = sizeof_perf_metrics(perf, &n_configs);
 
-	if (get_user(flags, &user_query_config_ptr->flags))
-		return -EFAULT;
+	ret = copy_query_item(&query_config, sizeof(query_config), total_size,
+			      query_item);
+	if (ret != 0)
+		return ret;
 
-	if (flags != 0)
+	if (query_config.flags != 0)
 		return -EINVAL;
 
-	n_configs = 1;
-	do {
-		struct i915_oa_config *tmp;
-		u64 *ids;
-		int id;
+	oa_config_ids = kcalloc(n_configs, sizeof(*oa_config_ids), GFP_KERNEL);
+	if (!oa_config_ids)
+		return -ENOMEM;
 
-		ids = krealloc(oa_config_ids,
-			       n_configs * sizeof(*oa_config_ids),
-			       GFP_KERNEL);
-		if (!ids)
-			return -ENOMEM;
-
-		alloc = fetch_and_zero(&n_configs);
-
-		ids[n_configs++] = 1ull; /* reserved for test_config */
-		rcu_read_lock();
-		idr_for_each_entry(&perf->metrics_idr, tmp, id) {
-			if (n_configs < alloc)
-				ids[n_configs] = id;
-			n_configs++;
+	ids = oa_config_ids;
+	*ids++ = 1ull; /* reserved for test_config */
+	ret = 1;
+	rcu_read_lock();
+	idr_for_each_entry(&perf->metrics_idr, tmp, id) {
+		if (ret++ >= n_configs) {
+			ret = -EINVAL; /* Try again, array grew since sizeof_perf_metrics() */
+			rcu_read_unlock();
+			goto out;
 		}
-		rcu_read_unlock();
-
-		oa_config_ids = ids;
-	} while (n_configs > alloc);
-
-	if (query_item->length < sizeof_perf_config_list(n_configs)) {
-		drm_dbg(&i915->drm,
-			"Invalid query config list item size=%u expected=%zu\n",
-			query_item->length,
-			sizeof_perf_config_list(n_configs));
-		kfree(oa_config_ids);
-		return -EINVAL;
+		*ids++ = id;
 	}
+	rcu_read_unlock();
 
 	if (put_user(n_configs, &user_query_config_ptr->config)) {
-		kfree(oa_config_ids);
-		return -EFAULT;
+		ret = -EFAULT;
+		goto out;
 	}
 
 	ret = copy_to_user(user_query_config_ptr + 1,
 			   oa_config_ids,
 			   n_configs * sizeof(*oa_config_ids));
+	if (ret)
+		ret = -EFAULT;
+
+out:
 	kfree(oa_config_ids);
-	if (ret)
-		return -EFAULT;
 
-	return sizeof_perf_config_list(n_configs);
+	return ret ?: total_size;
 }
 
 static int query_perf_config(struct drm_i915_private *i915,
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 4+ messages in thread