On 16/04/18 03:11, Tvrtko Ursulin wrote:
>
> On 11/04/2018 17:32, Lionel Landwerlin wrote:
>> Looks good to me, a few nits below.
>> I'll try to find some time to display this information in gputop.
>>
>> Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>
> Thanks!
>
>> On 11/04/18 02:46, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>
>>> Engine discovery query allows userspace to enumerate engines, probe
>>> their
>>> configuration features, all without needing to maintain the internal
>>> PCI
>>> ID based database.
>>>
>>> A new query for the generic i915 query ioctl is added named
>>> DRM_I915_QUERY_ENGINE_INFO, together with accompanying structure
>>> drm_i915_query_engine_info. The address of latter should be passed
>>> to the
>>> kernel in the query.data_ptr field, and should be large enough for the
>>> kernel to fill out all known engines as struct drm_i915_engine_info
>>> elements trailing the query.
>>>
>>> As with other queries, setting the item query length to zero allows
>>> userspace to query minimum required buffer size.
>>>
>>> Enumerated engines have common type mask which can be used to query all
>>> hardware engines, versus engines userspace can submit to using the
>>> execbuf
>>> uAPI.
>>>
>>> Engines also have capabilities which are per engine class namespace of
>>> bits describing features not present on all engine instances.
>>>
>>> v2:
>>> * Fixed HEVC assignment.
>>> * Reorder some fields, rename type to flags, increase width. (Lionel)
>>> * No need to allocate temporary storage if we do it engine by engine.
>>> (Lionel)
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>>> Cc: Jon Bloomfield <jon.bloomfield at intel.com>
>>> Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin at intel.com>
>>> Cc: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>>> ---
>>> This is RFC for now since it is not very usable before the new
>>> execbuf API
>>> or virtual engine queue submission gets closer.
>>>
>>> In this version I have added capability of distinguishing between
>>> hardware
>>> engines and ABI engines. This is to account for probable future use
>>> cases
>>> like virtualization, where guest might only see one engine instance,
>>> but
>>> will want to know overall hardware capabilities in order to tune its
>>> behaviour.
>>>
>>> In general I think we will have to wait a bit before defining the final
>>> look of the uAPI, but in the meantime I thought it useful to share
>>> as RFC.
>>> ---
>>> drivers/gpu/drm/i915/i915_query.c | 63
>>> +++++++++++++++++++++++++++++++++
>>> drivers/gpu/drm/i915/intel_engine_cs.c | 2 ++
>>> drivers/gpu/drm/i915/intel_ringbuffer.h | 3 ++
>>> include/uapi/drm/i915_drm.h | 46 ++++++++++++++++++++++++
>>> 4 files changed, 114 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_query.c
>>> b/drivers/gpu/drm/i915/i915_query.c
>>> index 3ace929dd90f..a1c0e3acc882 100644
>>> --- a/drivers/gpu/drm/i915/i915_query.c
>>> +++ b/drivers/gpu/drm/i915/i915_query.c
>>> @@ -82,9 +82,72 @@ static int query_topology_info(struct
>>> drm_i915_private *dev_priv,
>>> return total_length;
>>> }
>>> +static int
>>> +query_engine_info(struct drm_i915_private *i915,
>>> + struct drm_i915_query_item *query_item)
>>> +{
>>> + struct drm_i915_query_engine_info __user *query_ptr =
>>> + u64_to_user_ptr(query_item->data_ptr);
>>> + struct drm_i915_engine_info __user *info_ptr =
>>> &query_ptr->engines[0];
>>> + struct drm_i915_query_engine_info query;
>>> + struct intel_engine_cs *engine;
>>> + enum intel_engine_id id;
>>> + int len;
>>> +
>>> + if (query_item->flags)
>>> + return -EINVAL;
>>> +
>>> + len = sizeof(struct drm_i915_query_engine_info) +
>>> + I915_NUM_ENGINES * sizeof(struct drm_i915_engine_info);
>>> +
>>> + if (!query_item->length)
>>> + return len;
>>> + else if (query_item->length < len)
>>> + return -EINVAL;
>>> +
>>> + if (copy_from_user(&query, query_ptr, sizeof(query)))
>>> + return -EFAULT;
>>> +
>>> + if (query.num_engines || query.rsvd[0] || query.rsvd[1] ||
>>> + query.rsvd[2])
>>> + return -EINVAL;
>>> +
>>> + if (!access_ok(VERIFY_WRITE, query_ptr, query_item->length))
>>
>> You could limit this to len ?
>
> I could, I am just wondering if you are leading somewhere with this
> suggestion like something I missed?
>
> Check against query_item->length has the semantics of "you told me you
> are giving me this big of a buffer, and I'll check if you were lying
> even if I won't use it all", versus check against len would be "I'll
> check only the part I'll write into, even if you told me buffer is
> bigger".
>
> The current check is a protecting userspace against more mistakes, but
> perhaps you are thinking about some tricks which would be possible by
> just checking len?
Cool, looks like you thought about everything :)