[Intel-gfx] [PATCH v4 6/6] drm/i915: expose rcs topology through query uAPI
Lionel Landwerlin
lionel.g.landwerlin at intel.com
Tue Jan 16 10:42:53 UTC 2018
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 at 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
>
>
More information about the Intel-gfx
mailing list