[Intel-gfx] [PATCH v6] drm/i915: Engine discovery query
Chris Wilson
chris at chris-wilson.co.uk
Fri Oct 5 10:35:42 UTC 2018
Quoting Tvrtko Ursulin (2018-10-05 09:26:37)
>
> On 04/10/2018 17:33, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2018-10-04 11:51:18)
> >> 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)
> >>
> >> 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 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>
> >> Cc: Tony Ye <tony.ye at intel.com>
> >> ---
> >> 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?
My only point was to keep tying info_ptr back to the early checks on
query_ptr. From the standpoint of looking at this loop copying into
userspace, I wanted to reassure myself that everything was checked and
cross-checked.
> >> +/**
> >> + * 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 at chris-wilson.co.uk>
> >
> > 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.
I like the suggestion to carve out some bits ahead of time for common
caps. Then I just worry if 32 is enough :)
Hmm, how about a challenge of describing which engines are compatible
for v.eng? Trial-and-error strikes me as being the simplest discovery
method still.
-Chris
More information about the Intel-gfx
mailing list