Re: [Intel-gfx] [PATCH] drm/i915/uapi: Add DRM_I915_QUERY_GEOMETRY_SUBSLICES

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




+ Joonas

On 25/03/2022 23:03, Francisco Jerez wrote:
Matt Atwood <matthew.s.atwood@xxxxxxxxx> writes:

Newer platforms have DSS that aren't necessarily available for both
geometry and compute, two queries will need to exist. This introduces
the first, when passing a valid engine class and engine instance in the
flags returns a topology describing geometry.

v2: fix white space errors
v3: change flags from hosting 2 8 bit numbers to holding a
i915_engine_class_instance struct

Cc: Ashutosh Dixit <ashutosh.dixit@xxxxxxxxx>
Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
UMD (mesa): https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/14143
Signed-off-by: Matt Atwood <matthew.s.atwood@xxxxxxxxx>
---
  drivers/gpu/drm/i915/i915_query.c | 68 ++++++++++++++++++++++---------
  include/uapi/drm/i915_drm.h       | 24 +++++++----
  2 files changed, 65 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
index 2dfbc22857a3..fcb374201edb 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -9,6 +9,7 @@
  #include "i915_drv.h"
  #include "i915_perf.h"
  #include "i915_query.h"
+#include "gt/intel_engine_user.h"
  #include <uapi/drm/i915_drm.h>
static int copy_query_item(void *query_hdr, size_t query_sz,
@@ -28,36 +29,30 @@ static int copy_query_item(void *query_hdr, size_t query_sz,
  	return 0;
  }
-static int query_topology_info(struct drm_i915_private *dev_priv,
-			       struct drm_i915_query_item *query_item)
+static int fill_topology_info(const struct sseu_dev_info *sseu,
+			      struct drm_i915_query_item *query_item,
+			      const u8 *subslice_mask)
  {
-	const struct sseu_dev_info *sseu = &to_gt(dev_priv)->info.sseu;
  	struct drm_i915_query_topology_info topo;
  	u32 slice_length, subslice_length, eu_length, total_length;
  	int ret;
- if (query_item->flags != 0)
-		return -EINVAL;
+	BUILD_BUG_ON(sizeof(u8) != sizeof(sseu->slice_mask));
if (sseu->max_slices == 0)
  		return -ENODEV;
- BUILD_BUG_ON(sizeof(u8) != sizeof(sseu->slice_mask));
-
  	slice_length = sizeof(sseu->slice_mask);
  	subslice_length = sseu->max_slices * sseu->ss_stride;
  	eu_length = sseu->max_slices * sseu->max_subslices * sseu->eu_stride;
  	total_length = sizeof(topo) + slice_length + subslice_length +
  		       eu_length;
- ret = copy_query_item(&topo, sizeof(topo), total_length,
-			      query_item);
+	ret = copy_query_item(&topo, sizeof(topo), total_length, query_item);
+
  	if (ret != 0)
  		return ret;
- if (topo.flags != 0)
-		return -EINVAL;
-
  	memset(&topo, 0, sizeof(topo));
  	topo.max_slices = sseu->max_slices;
  	topo.max_subslices = sseu->max_subslices;
@@ -69,27 +64,61 @@ static int query_topology_info(struct drm_i915_private *dev_priv,
  	topo.eu_stride = sseu->eu_stride;
if (copy_to_user(u64_to_user_ptr(query_item->data_ptr),
-			   &topo, sizeof(topo)))
+			 &topo, sizeof(topo)))
  		return -EFAULT;
if (copy_to_user(u64_to_user_ptr(query_item->data_ptr + sizeof(topo)),
-			   &sseu->slice_mask, slice_length))
+			 &sseu->slice_mask, slice_length))
  		return -EFAULT;
if (copy_to_user(u64_to_user_ptr(query_item->data_ptr +
-					   sizeof(topo) + slice_length),
-			   sseu->subslice_mask, subslice_length))
+					 sizeof(topo) + slice_length),
+			 subslice_mask, subslice_length))
  		return -EFAULT;
if (copy_to_user(u64_to_user_ptr(query_item->data_ptr +
-					   sizeof(topo) +
-					   slice_length + subslice_length),
-			   sseu->eu_mask, eu_length))
+					 sizeof(topo) +
+					 slice_length + subslice_length),
+			 sseu->eu_mask, eu_length))
  		return -EFAULT;
return total_length;
  }
+static int query_topology_info(struct drm_i915_private *dev_priv,
+			       struct drm_i915_query_item *query_item)
+{
+	const struct sseu_dev_info *sseu = &to_gt(dev_priv)->info.sseu;
+
+	if (query_item->flags != 0)
+		return -EINVAL;
+
+	return fill_topology_info(sseu, query_item, sseu->subslice_mask);
+}
+
+static int query_geometry_subslices(struct drm_i915_private *i915,
+				    struct drm_i915_query_item *query_item)
+{
+	const struct sseu_dev_info *sseu;
+	struct intel_engine_cs *engine;
+	struct i915_engine_class_instance classinstance;
+
+	if (GRAPHICS_VER_FULL(i915) < IP_VER(12, 50))
+		return -ENODEV;
+
+	classinstance = *((struct i915_engine_class_instance *)&query_item->flags);
+
+	engine = intel_engine_lookup_user(i915, (u8) classinstance.engine_class,
+					  (u8) classinstance.engine_instance);
+
+	if (!engine)
+		return -EINVAL;
+
+	sseu = &engine->gt->info.sseu;
+
+	return fill_topology_info(sseu, query_item, sseu->geometry_subslice_mask);
+}
+
  static int
  query_engine_info(struct drm_i915_private *i915,
  		  struct drm_i915_query_item *query_item)
@@ -485,6 +514,7 @@ static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv,
  	query_engine_info,
  	query_perf_config,
  	query_memregion_info,
+	query_geometry_subslices,
  };
int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 05c3642aaece..b539c83a4034 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -2687,10 +2687,11 @@ struct drm_i915_perf_oa_config {
  struct drm_i915_query_item {
  	/** @query_id: The id for this query */
  	__u64 query_id;
-#define DRM_I915_QUERY_TOPOLOGY_INFO    1
-#define DRM_I915_QUERY_ENGINE_INFO	2
-#define DRM_I915_QUERY_PERF_CONFIG      3
-#define DRM_I915_QUERY_MEMORY_REGIONS   4
+#define DRM_I915_QUERY_TOPOLOGY_INFO		1
+#define DRM_I915_QUERY_ENGINE_INFO		2
+#define DRM_I915_QUERY_PERF_CONFIG		3
+#define DRM_I915_QUERY_MEMORY_REGIONS		4
+#define DRM_I915_QUERY_GEOMETRY_SUBSLICES	5
  /* Must be kept compact -- no holes and well documented */
/**
@@ -2714,6 +2715,9 @@ struct drm_i915_query_item {
  	 *	- DRM_I915_QUERY_PERF_CONFIG_LIST
  	 *      - DRM_I915_QUERY_PERF_CONFIG_DATA_FOR_UUID
  	 *      - DRM_I915_QUERY_PERF_CONFIG_FOR_UUID
+	 *
+	 * When query_id == DRM_I915_QUERY_GEOMETRY_SUBSLICES must have a valid
+	 * i915_engine_class_instance struct.

To get back to our previous discussion off-list, I find this interface
kind of confusing, since it's expecting an engine ID as argument, but it
returns the set of subslices available to the *render* engine regardless
of the engine class specified.  I think it would make sense to rename
this to DRM_I915_QUERY_ENGINE_SUBSLICES or similar and have the mask
returned be the set of subslices actually available to the engine that
was specified (e.g. the compute subslice mask if a compute engine is
specified, or an error if the engine specified doesn't have any
connection to subslices).  Alternatively, if this is really only meant
to work for the render engine, maybe the engine class should be dropped
from "flags", only the engine instance is necessary -- I think that
would prevent programming errors and would leave additional room in
"flags" for future expansion.

I have asked a similar question and AFAIR Matt explained it was an arch level direction to have it like it is. Not sure of the reasoning.

I wouldn't take the option of implying render and only having instance in flags, but returning error for engines not applicable sounds good to me. If there isn't a good reason not to do it.

Regards,

Tvrtko



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux