On 16/01/18 14:22, Chris Wilson wrote:
Quoting Lionel Landwerlin (2018-01-16 13:40:10)
With the introduction of asymmetric slices in CNL, we cannot rely on
the previous SUBSLICE_MASK getparam to tell userspace what subslices
are available. Here we introduce a more detailed way of querying the
Gen's GPU topology that doesn't aggregate numbers.
This is essential for monitoring parts of the GPU with the OA unit,
because counters need to be normalized to the number of
EUs/subslices/slices. The current aggregated numbers like EU_TOTAL do
not gives us sufficient information.
As a bonus we can draw representations of the GPU :
https://imgur.com/a/vuqpa
v2: Rename uapi struct s/_mask/_info/ (Tvrtko)
Report max_slice/subslice/eus_per_subslice rather than strides (Tvrtko)
Add uapi macros to read data from *_info structs (Tvrtko)
v3: Use !!(v & DRM_I915_BIT()) for uapi macros instead of custom shifts (Tvrtko)
v4: factorize query item writting (Tvrtko)
tweak uapi struct/define names (Tvrtko)
Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@xxxxxxxxx>
---
drivers/gpu/drm/i915/i915_query.c | 107 ++++++++++++++++++++++++++++++++++++++
include/uapi/drm/i915_drm.h | 53 +++++++++++++++++++
2 files changed, 160 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
index 5694cfea4553..4d18fbd07cbd 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -25,8 +25,102 @@
#include "i915_drv.h"
#include <uapi/drm/i915_drm.h>
+static int copy_query_data(struct drm_i915_query_item *query_item,
+ const void *item_ptr, u32 item_length,
+ const void *data_ptr, u32 data_length)
+{
+ u32 total_length = item_length + data_length;
+
+ if (query_item->length == 0) {
+ query_item->length = total_length;
+ return 0;
+ }
+
+ if (query_item->length != total_length)
+ return -EINVAL;
Let the user pass in a preallocated buffer of a certain size, and only
have to resort to reallocating if too small. i.e.
if (query_item->length < total_length)
return -EINVAL;
Done.
+
+ if (copy_to_user(u64_to_user_ptr(query_item->data_ptr),
+ item_ptr, item_length))
+ return -EFAULT;
+
+ if (copy_to_user(u64_to_user_ptr(query_item->data_ptr + item_length),
+ data_ptr, data_length))
+ return -EFAULT;
+
+ return 0;
+}
+
+static int query_slice_info(struct drm_i915_private *dev_priv,
+ struct drm_i915_query_item *query_item)
+static int query_subslice_info(struct drm_i915_private *dev_priv,
+ struct drm_i915_query_item *query_item)
+static int query_eu_info(struct drm_i915_private *dev_priv,
+ struct drm_i915_query_item *query_item)
Couldn't spot any stray leaks.
int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
{
+ struct drm_i915_private *dev_priv = to_i915(dev);
struct drm_i915_query *args = data;
struct drm_i915_query_item __user *user_item_ptr =
u64_to_user_ptr(args->items_ptr);
@@ -34,15 +128,28 @@ int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
for (i = 0; i < args->num_items; i++, user_item_ptr++) {
struct drm_i915_query_item item;
+ int ret;
if (copy_from_user(&item, user_item_ptr, sizeof(item)))
return -EFAULT;
switch (item.query_id) {
+ case DRM_I915_QUERY_SLICE_INFO:
+ ret = query_slice_info(dev_priv, &item);
+ break;
+ case DRM_I915_QUERY_SUBSLICE_INFO:
+ ret = query_subslice_info(dev_priv, &item);
+ break;
+ case DRM_I915_QUERY_EU_INFO:
+ ret = query_eu_info(dev_priv, &item);
+ break;
default:
return -EINVAL;
}
+ if (ret)
+ return ret;
I would make item const and return the copied length:
if (ret < 0)
return 0;
if (ret != item.length && put_user(ret, &user_item_ptr->length))
return -EFAULT;
Okay.
+#define DRM_I915_BIT(bit) (1 << (bit))
((__u32)1 << (bit))
Might as well prepare for that 32nd bit.
Done.
+
+/* Data written by the kernel with query DRM_I915_QUERY_ID_SLICES_INFO :
+ *
+ * data: each bit indicates whether a slice is available (1) or fused off (0).
+ * Use DRM_I915_QUERY_SLICE_AVAILABLE() to query a given slice's
+ * availability.
+ */
+struct drm_i915_query_slice_info {
+ __u32 max_slices;
+
+#define DRM_I915_QUERY_SLICE_AVAILABLE(info, slice) \
+ !!((info)->data[(slice) / 8] & DRM_I915_BIT((slice) % 8))
+ __u8 data[];
+};
+
+/* Data written by the kernel with query DRM_I915_QUERY_ID_SUBSLICES_INFO :
+ *
+ * data: each bit indicates whether a subslice is available (1) or fused off
+ * (0). Use DRM_I915_QUERY_SUBSLICE_AVAILABLE() to query a given
+ * subslice's availability.
+ */
+struct drm_i915_query_subslice_info {
+ __u32 max_slices;
+ __u32 max_subslices;
+
+#define DRM_I915_QUERY_SUBSLICE_AVAILABLE(info, slice, subslice) \
+ !!((info)->data[(slice) * ALIGN((info)->max_subslices, 8) / 8 + \
Where did we pull ALIGN() in from? If it's from the kernel headers,
conflicts will ensue. If not, we have no definition for it.
-Chris
I've noticed other kernel uapi headers where using it. So I assumed it
was fine.
I should replace it too.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
