Re: [PATCH v4 6/6] drm/i915: expose rcs topology through query uAPI

[Lionel Landwerlin <lionel.g.landwerlin@intel.com>]

On 16/01/18 08:42, Tvrtko Ursulin wrote:
>
> On 15/01/2018 18:23, Lionel Landwerlin wrote:
>> On 15/01/18 17:54, Tvrtko Ursulin wrote:
>>>
>>> On 15/01/2018 14:41, Lionel Landwerlin wrote:
>>>> 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)
>>>>
>>>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/i915_query.c | 134 
>>>> ++++++++++++++++++++++++++++++++++++++
>>>>   include/uapi/drm/i915_drm.h       |  53 +++++++++++++++
>>>>   2 files changed, 187 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_query.c 
>>>> b/drivers/gpu/drm/i915/i915_query.c
>>>> index 5694cfea4553..465ec18a472f 100644
>>>> --- a/drivers/gpu/drm/i915/i915_query.c
>>>> +++ b/drivers/gpu/drm/i915/i915_query.c
>>>> @@ -25,8 +25,129 @@
>>>>   #include "i915_drv.h"
>>>>   #include <uapi/drm/i915_drm.h>
>>>>   +static int query_slices_info(struct drm_i915_private *dev_priv,
>>>> +                 struct drm_i915_query_item *query_item)
>>>> +{
>>>> +    const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu;
>>>> +    struct drm_i915_query_slices_info slices_info;
>>>> +    u32 data_length, length;
>>>> +
>>>> +    if (sseu->max_slices == 0)
>>>> +        return -ENODEV;
>>>> +
>>>> +    data_length = sizeof(sseu->slice_mask);
>>>> +    length = sizeof(slices_info) + data_length;
>>>> +
>>>> +    /*
>>>> +     * If we ever change the internal slice mask data type, we'll 
>>>> need to
>>>> +     * update this function.
>>>> +     */
>>>> +    BUILD_BUG_ON(sizeof(u8) != sizeof(sseu->slice_mask));
>>>> +
>>>> +    if (query_item->length == 0) {
>>>> +        query_item->length = length;
>>>> +        return 0;
>>>> +    }
>>>> +
>>>> +    if (query_item->length != length)
>>>> +        return -EINVAL;
>>>> +
>>>> +    memset(&slices_info, 0, sizeof(slices_info));
>>>> +    slices_info.max_slices = sseu->max_slices;
>>>> +
>>>> +    if (copy_to_user(u64_to_user_ptr(query_item->data_ptr), 
>>>> &slices_info,
>>>> +             sizeof(slices_info)))
>>>> +        return -EFAULT;
>>>> +
>>>> +    if (copy_to_user(u64_to_user_ptr(query_item->data_ptr +
>>>> +                     offsetof(struct drm_i915_query_slices_info, 
>>>> data)),
>>>> +             &sseu->slice_mask, data_length))
>>>> +        return -EFAULT;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int query_subslices_info(struct drm_i915_private *dev_priv,
>>>> +                struct drm_i915_query_item *query_item)
>>>> +{
>>>> +    const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu;
>>>> +    struct drm_i915_query_subslices_info subslices_info;
>>>> +    u32 data_length, length;
>>>> +
>>>> +    if (sseu->max_slices == 0)
>>>> +        return -ENODEV;
>>>> +
>>>> +    memset(&subslices_info, 0, sizeof(subslices_info));
>>>> +    subslices_info.max_slices = sseu->max_slices;
>>>> +    subslices_info.max_subslices = sseu->max_subslices;
>>>> +
>>>> +    data_length = subslices_info.max_slices *
>>>> +        DIV_ROUND_UP(subslices_info.max_subslices,
>>>> +                 sizeof(sseu->subslice_mask[0]) * BITS_PER_BYTE);
>>>> +    length = sizeof(subslices_info) + data_length;
>>>> +
>>>> +    if (query_item->length == 0) {
>>>> +        query_item->length = length;
>>>> +        return 0;
>>>> +    }
>>>> +
>>>> +    if (query_item->length != length)
>>>> +        return -EINVAL;
>>>> +
>>>> +    if (copy_to_user(u64_to_user_ptr(query_item->data_ptr), 
>>>> &subslices_info,
>>>> +             sizeof(subslices_info)))
>>>> +        return -EFAULT;
>>>> +
>>>> +    if (copy_to_user(u64_to_user_ptr(query_item->data_ptr +
>>>> +                     offsetof(struct 
>>>> drm_i915_query_subslices_info, data)),
>>>> +             sseu->subslice_mask, data_length))
>>>> +        return -EFAULT;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int query_eus_info(struct drm_i915_private *dev_priv,
>>>> +              struct drm_i915_query_item *query_item)
>>>> +{
>>>> +    const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu;
>>>> +    struct drm_i915_query_eus_info eus_info;
>>>> +    u32 data_length, length;
>>>> +
>>>> +    if (sseu->max_slices == 0)
>>>> +        return -ENODEV;
>>>> +
>>>> +    memset(&eus_info, 0, sizeof(eus_info));
>>>> +    eus_info.max_slices = sseu->max_slices;
>>>> +    eus_info.max_subslices = sseu->max_subslices;
>>>> +    eus_info.max_eus_per_subslice = sseu->max_eus_per_subslice;
>>>> +
>>>> +    data_length = eus_info.max_slices * eus_info.max_subslices *
>>>> +        DIV_ROUND_UP(eus_info.max_eus_per_subslice, BITS_PER_BYTE);
>>>> +    length = sizeof(eus_info) + data_length;
>>>> +
>>>> +    if (query_item->length == 0) {
>>>> +        query_item->length = length;
>>>> +        return 0;
>>>> +    }
>>>> +
>>>> +    if (query_item->length != length)
>>>> +        return -EINVAL;
>>>> +
>>>> +    if (copy_to_user(u64_to_user_ptr(query_item->data_ptr), 
>>>> &eus_info,
>>>> +             sizeof(eus_info)))
>>>> +        return -EFAULT;
>>>> +
>>>> +    if (copy_to_user(u64_to_user_ptr(query_item->data_ptr +
>>>> +                     offsetof(struct drm_i915_query_eus_info, data)),
>>>> +             sseu->eu_mask, data_length))
>>>> +        return -EFAULT;
>>>
>>> I think in all three queries, once you have the length, you could 
>>> extract the four if statement above into a common helper? Arguments 
>>> are query_item, length, data_length, ptr_query_item, ptr_data, and 
>>> offset_data, unless I am missing something. Up to you if you would 
>>> find that a worthy compaction.
>>
>> I just tried that but the result doesn't really look good.
>> You need to maintain the control flow via an indirect return value...
>
> I was thinking something like this:
>
> int insert_query_helper_name(query_item, query_item_sz, length, 
> data_length, query_item_ptr, query_data_ptr)
> {
>     if (query_item->length == 0) {
>         query_item->length = length;
>         return 0;
>     }
>
>     if (query_item->length != length)
>         return -EINVAL;
>
>     if (copy_to_user(u64_to_user_ptr(query_item->data_ptr),
>             query_item_ptr, query_item_sz))
>         return -EFAULT;
>
>     if (copy_to_user(u64_to_user_ptr(query_item->data_ptr +
>                      query_item_sz,
>              query_data_ptr, data_length))
>         return -EFAULT;
>
>     return 0;
> }
>
> Then in the handlers just end with:
>
> return insert_query_helper_hame(...);

Ah thanks, I stopped at copying the query item but didn't do the data...
That looks better indeed.

>
> ?
>
>>>
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>>   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 +155,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_ID_SLICES_INFO:
>>>> +            ret = query_slices_info(dev_priv, &item);
>>>> +            break;
>>>> +        case DRM_I915_QUERY_ID_SUBSLICES_INFO:
>>>> +            ret = query_subslices_info(dev_priv, &item);
>>>> +            break;
>>>> +        case DRM_I915_QUERY_ID_EUS_INFO:
>>>> +            ret = query_eus_info(dev_priv, &item);
>>>> +            break;
>>>>           default:
>>>>               return -EINVAL;
>>>>           }
>>>>   +        if (ret)
>>>> +            return ret;
>>>> +
>>>>           if (copy_to_user(user_item_ptr, &item, sizeof(item)))
>>>>               return -EFAULT;
>>>>       }
>>>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>>>> index 39e93f10f2cd..d25f21e3c107 100644
>>>> --- a/include/uapi/drm/i915_drm.h
>>>> +++ b/include/uapi/drm/i915_drm.h
>>>> @@ -1618,6 +1618,9 @@ struct drm_i915_perf_oa_config {
>>>>     struct drm_i915_query_item {
>>>>       __u64 query_id;
>>>> +#define DRM_I915_QUERY_ID_SLICES_INFO    0x01
>>>> +#define DRM_I915_QUERY_ID_SUBSLICES_INFO 0x02
>>>> +#define DRM_I915_QUERY_ID_EUS_INFO       0x03
>>>
>>> Would DRM_I915_QUERY_ID_"singular"_INFO read better? In struct names 
>>> as well if so?
>>
>> Hmm... Sorry, I don't understand your comment here.
>> What would be "singular" ?
>
> Opposite of plural, like:
>
> _QUERY_ID_SLICE_INFO, _QUERY_ID_SUBSLICE_INFO, _QUERY_ID_EU_INFO.
>
> Actually now I'm thinking if _ID_ part could also be removed for no loss?

Yeah, sure.

>
> Regards,
>
> Tvrtko
>
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx