On 04/10/2018 17:33, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2018-10-04 11:51:18)
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)
v3:
* Describe engine flags and mark mbz fields. (Lionel)
* HEVC only applies to VCS.
v4:
* Squash SFC flag into main patch.
* Tidy some comments.
v5:
* Add uabi_ prefix to engine capabilities. (Chris Wilson)
* Report exact size of engine info array. (Chris Wilson)
* Drop the engine flags. (Joonas Lahtinen)
* Added some more reserved fields.
* Move flags after class/instance.
v6:
* Do not check engine info array was zeroed by userspace but zero the
unused fields for them instead.
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>
Cc: Tony Ye <tony.ye@xxxxxxxxx>
---
drivers/gpu/drm/i915/i915_query.c | 56 +++++++++++++++++++++++++
drivers/gpu/drm/i915/intel_engine_cs.c | 12 ++++++
drivers/gpu/drm/i915/intel_ringbuffer.h | 3 ++
include/uapi/drm/i915_drm.h | 47 +++++++++++++++++++++
4 files changed, 118 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c
index 5821002cad42..5ac8ef9f5de4 100644
--- a/drivers/gpu/drm/i915/i915_query.c
+++ b/drivers/gpu/drm/i915/i915_query.c
@@ -84,9 +84,65 @@ 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 drm_i915_engine_info info = { };
+ struct intel_engine_cs *engine;
+ enum intel_engine_id id;
+ int len;
+
+ if (query_item->flags)
+ return -EINVAL;
+
+ len = 0;
+ for_each_engine(engine, i915, id)
+ len++;
(Isn't this INTEL_INFO()->num_rings?)
/o\
+ len *= sizeof(struct drm_i915_engine_info);
+ len += sizeof(struct drm_i915_query_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))
+ return -EFAULT;
Hmm, so length is the sizeof of the whole struct and not the space in
the pointed at block? Ok. I was just a little confused by the lack of
checking for info_ptr, as by this point the connection with query_ptr is
off the page.
May I beg
info_ptr = &query_ptr->engines[0];
here. That should make it more obvious for feeble readers like myself to
see that info_ptr is contained within the access_ok check.
Ok.
+ for_each_engine(engine, i915, id) {
+ info.class = engine->uabi_class;
+ info.instance = engine->instance;
+ info.capabilities = engine->uabi_capabilities;
+
GEM_BUG_ON((void *)info_ptr > (void *)query_ptr + query_item->length - sizeof(info));
There is a check above that len fits in query_item->length. So unless
the code distrusts itself in a space of a few lines, or we distrust
INTEL_INFO->num_engines vs for_each_engine I don't see it is needed?
+ if (__copy_to_user(info_ptr, &info, sizeof(info)))
+ return -EFAULT;
+
+ query.num_engines++;
+ info_ptr++;
+ }
+
+ if (__copy_to_user(query_ptr, &query, sizeof(query)))
+ return -EFAULT;
+
+ return len;
+}
+
static int (* const i915_query_funcs[])(struct drm_i915_private *dev_priv,
struct drm_i915_query_item *query_item) = {
query_topology_info,
+ query_engine_info,
};
int i915_query_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 1c6143bdf5a4..134f0cec724c 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -298,6 +298,18 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
engine->uabi_id = info->uabi_id;
engine->uabi_class = intel_engine_classes[info->class].uabi_class;
+ if (engine->class == VIDEO_DECODE_CLASS) {
+ /* HEVC support is present only on vcs0. */
+ if (INTEL_GEN(dev_priv) >= 8 && info->instance == 0)
+ engine->uabi_capabilities =
+ I915_VCS_CLASS_CAPABILITY_HEVC;
+
+ /* SFC support is wired only to even VCS instances. */
+ if (INTEL_GEN(dev_priv) >= 9 && !(info->instance & 1))
+ engine->uabi_capabilities |=
+ I915_VCS_CLASS_CAPABILITY_SFC;
I trust your judgement here.
And I trust pending feedback from media team. :)
+/**
+ * struct drm_i915_query_engine_info
+ *
+ * Engine info query enumerates all engines known to the driver by filling in
+ * an array of struct drm_i915_engine_info structures.
+ */
+struct drm_i915_query_engine_info {
+ /** Number of struct drm_i915_engine_info structs following. */
+ __u32 num_engines;
+
+ /** MBZ */
+ __u32 rsvd[3];
+
+ /** Marker for drm_i915_engine_info structures. */
+ struct drm_i915_engine_info engines[];
Do we need [0] for old-c/c++ compatibility?
Hm.. trying to remember. It is GNU C vs C99 for zero sized array vs
flexible array member. We went for the latter in the topology query
after some discussion but I don't remember the details now. Grepping the
uapi headers there are both so don't know.. GCC documentation says
flexible members are preferred.
Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
I'd value an ack from Lionel in case I've forgotten a quirk about the
query api, and an ack from a user that this meets their needs. It will
certainly simplify some of our igt code!
Have r-b from Lionel already.
Question for overall design, do we want a conjoint capability flag
space, or one per class? One per class gives us more room, so I think
should be preferred, but I wonder if a set of common flags would make
it easier for userspace to filter. (Though not hard to match on both
class and caps!)
Could split common and per class, 32-bits each with separate fields?
__u32 capabilities;
__u32 class_capabilities;
Or keep __u64 capabilities and say common start from bit0 and per class
start at bit32? this option could be added/defined post factum as well,
with no required changes now - once there is a first common cap. As long
as there is a nice block of free bits at that point.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
