Re: [PATCH i-g-t 22/24] i915: Add gem_ctx_engines

[Chris Wilson <chris@chris-wilson.co.uk>]

Quoting Andi Shyti (2019-03-22 16:40:07)
> Hi Chris,
> 
> sorry for the late reply, I got 5 version of this same patch and
> I couldn't figure out what was what :)
> 
> Could you please add some versioning or note if version is
> the same?
> 
> Some nits and questions....
> 
> > +static bool has_context_engines(int i915)
> > +{
> > +     struct drm_i915_gem_context_param param = {
> > +             .ctx_id = 0,
> > +             .param = I915_CONTEXT_PARAM_ENGINES,
> > +     };
> > +     return __gem_context_set_param(i915, &param) == 0;
> > +}
> 
> I had it and removed it so many times in gem_engine_topology,
> shall I put it back and we take it from there? (maybe in the
> future).
> 
> [...]
> > +     igt_assert_eq(__gem_context_set_param(i915, &param), -ENOENT);
> > +
> > +     mprotect(engines, 4096, PROT_READ);
> 
> (from the last review) mprotect can fail, do we care?

Debatable, yes we care as we won't get the expected faults in the next
tests, but do we want to call this the test failure? I want something
other than igt_require/igt_assert!

> > +     idx = 0;
> > +     memset(&engines, 0, sizeof(engines));
> > +     for_each_engine_class_instance(i915, e) {
> > +             engines.class_instance[idx].engine_class = e->class;
> > +             engines.class_instance[idx].engine_instance = e->instance;
> > +             idx++;
> > +     }
> > +     idx *= sizeof(*engines.class_instance);
> > +     p.size = base + idx;
> 
> (I normally review from bottom to top) You used at least three
> different ways to calculate param's size (some unclear to who
> is new to igt some more clear).
> 
> Does it make sense to have a global define and we keep it
> consistent?
> 
>  p.size = SIZEOF_CTX_PARAM(idx);

Definitely not shouting about it. I honestly believe that a plethora
of styles within tests is a good thing, and everything using the same
code pattern reduces the amount of test serendipity.

While this is a bit of trivial math and should not affect the outcome in
anyway, I quite like having bits and pieces fall naturally out of the
code because the code should also be an example of different ways it
might be used.

> it's a piece of code that I think it will be ussed a lot.
> 
> > +     /* Unadulterated I915_EXEC_DEFAULT should work */
> > +     execbuf.flags = 0;
> > +     igt_assert_eq(__gem_execbuf(i915, &execbuf), 0);
> 
> why aren't you using simply gem_execbuf()?

So the style matched the open calls to __gem_execbuf() later.

> > +                             execbuf.flags = j;
> > +                             err =__gem_execbuf(i915, &execbuf);
> > +                             if (j == i) {
> > +                                     igt_assert_f(err == 0,
> > +                                                  "Failed to report the valid engine for slot %d\n",
> > +                                                  i);
> > +                             } else {
> > +                                     igt_assert_f(err == -EINVAL,
> > +                                                  "Failed to report an invalid engine for slot %d (valid at %d)\n",
> > +                                                  j, i);
> > +                             }
> > +                     }
> > +
> > +                     do_ioctl(i915, DRM_IOCTL_I915_GEM_BUSY, &busy);
> > +                     if (i != -1) {
> > +                             igt_assert_eq(busy.busy, 1 << (e->class + 16));
> > +                     } else {
> > +                             igt_assert_eq(busy.busy, 0);
> > +                     }
> > +
> 
> (from the last review) this is not kernel style, not that I care
> much, but I thought you did.

Indeed, _we_ do care ;)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx