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.landwer...@intel.com>
> ---
> 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;
> +
> + 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;
> +#define DRM_I915_BIT(bit) (1 << (bit))
((__u32)1 << (bit))
Might as well prepare for that 32nd bit.
> +
> +/* 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
