Re: [PATCH v6] drm/i915: Engine discovery query

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

 



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

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

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux