[Intel-gfx] [PATCH v5 6/6] drm/i915: expose rcs topology through query uAPI

[Lionel Landwerlin]

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 at 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;

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.