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

[Tvrtko Ursulin]

On 16/01/2018 17:40, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-01-16 16:22:52)
>>
>> On 16/01/2018 16:02, Lionel Landwerlin wrote:
>>> diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
>>> index 6468ca613d27..81367c8224ee 100644
>>> --- a/drivers/gpu/drm/i915/i915_query.c
>>> +++ b/drivers/gpu/drm/i915/i915_query.c
>>> @@ -25,8 +25,100 @@
>>>    #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)
>>> +             return total_length;
>>> +
>>> +     if (query_item->length != total_length)
> 
> Pretty please
> 	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 total_length;
>>> +}
>>>        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;
>>> @@ -45,11 +138,24 @@ int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>>>                        return -EINVAL;
>>>    
>>>                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;
> 
> I still can't believe Tvrtko hasn't asked this to become an function
> pointer array. Tvrtko, I presume you've looked at how engine-info may
> fit into this? And engine-stats? And have suggestions for making adding
> new queries easier. Or are you happy as is?

I was happy with the switch for now. Was expecting someone would 
complain on the whole idea anyway. Maybe I was too pessimistic. :)

As for the engine-info, I couldn't think of any reasons why this 
framework wouldn't work for it. I'd put input parameters into the query 
blobs.

Engine-stats I had no plans to expose here. You think it would be 
useful? Main issue would be that we'd need commands to enable/disable 
and not just queries. Unless some magic to auto-enable on first query 
and drop after some timeout.. thinking to far ahead.

Regards,

Tvrtko