Re: [RFC v2] drm/i915: Engine discovery query

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>

Thanks!

On 11/04/18 02:46, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>

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@xxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Jon Bloomfield <jon.bloomfield@xxxxxxxxx>
Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin@xxxxxxxxx>
Cc: Lionel Landwerlin <lionel.g.landwerlin@xxxxxxxxx>
Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
---
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 :)

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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux