[igt-dev] [RFC PATCH v8 2/5] lib/i915: add gem_query library

[Chris Wilson]

Quoting Andi Shyti (2019-02-13 09:55:26)
> Hi Chris,
> 
> > > > > +static int __gem_query(int fd, struct drm_i915_query *q)
> > > > > +{
> > > > > +       return igt_ioctl(fd, DRM_IOCTL_I915_QUERY, q) ? -errno : 0;
> > > > > +}
> > > > > +
> > > > > +void gem_query(int fd, struct drm_i915_query *q)
> > > > > +{
> > > > > +       igt_assert(!__gem_query(fd, q));
> > > > 
> > > > For extra tidy asserts:
> > > > 
> > > > static int __gem_query(int fd, struct drm_i915_query *q)
> > > > {
> > > >       int err;
> > > > 
> > > >       err = 0;
> > > >       if (igt_ioctl(fd, DRM_IOCTL_I915_QUERY, q))
> > > >               err = -errno;
> > > > 
> > > >       errno = 0;
> > > >       return er;
> > > > }
> > > 
> > > Yes, I've seen this around, although it looks a bit redundant to
> > > me, I'll keep the style.
> > 
> > Wait until you read the igt_assert output.
> 
> OK, didn't think about it, I'm definitely missing some history
> here.
> 
> > > > > +       item.query_id = DRM_I915_QUERY_ENGINE_INFO;
> > > > > +       query.items_ptr = to_user_pointer(&item);
> > > > > +       query.num_items = 1;
> > > > > +       item.length = sizeof(*query_engines) +
> > > > > +                     64 * sizeof(struct drm_i915_engine_info);
> > > > 
> > > > You are betting we are not going to exceed 64 engines? A common bet for
> > > > sure...
> > > 
> > > We've been discussing about this in v4 and we agreed that 64 is
> > > big enough[*]. Am I missing anything?
> > > Besides, I thought that we won't have more engines than
> > > I915_EXEC_RING_MASK.
> > 
> > Do you think that execbuf2 is our final form? Besides the argument about
> > heap vs stack is more appropriate here; using the stack is more
> > appropriate later.
> 
> I can do that in case of 'drm_i915_query_engine_info' because the
> flexible structure has a name (struct drm_i915_engine_info engines[];).
> 
> But here the choices are less pleasant. Because I either do in
> the kernel something like:
> 
> +       struct whatever_name {
> -       struct {
>                 __u16 engine_class; /* see enum drm_i915_gem_engine_class */
>                 __u16 engine_instance;
>         } class_instance[0];
> 
> Or I define some ugly unflexible array in __set_ctx_engine_map():
> 
>         struct {
>                 __u16 ngine_class;
>                 __u16 engine_instance;
>         } class_instance[I915_EXEC_RING_MASK];
> 
> which looks to me quite strict (if we are considering that we
> might subvert everyting).

You can have an on-stack and off-stack buffer. But in this case since
you are returning a heap pointer, just create the heap pointer the right
size.
 
> Or, even worse, some "__u32 engine_stuff[I915_EXEC_RING_MASK]"
> and cast it.
> 
> Allocating it everywhere it looked cleaner.
> 
> Whatever you like :)

Around here, I'd rather see what people come up with to see how robust
the interface is, and if it's horrible tidy up the iface to prevent
that. But for the moment, returning a fixed size heap block when you're
being told the correct size, seems silly.
-Chris