[igt-dev] [Intel-gfx] [PATCH i-g-t 2/2] tests/i915_query: Engine discovery tests

[Tvrtko Ursulin]

On 01/05/2019 12:52, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-05-01 12:42:59)
>> +               switch (engine->engine.engine_class) {
>> +               case I915_ENGINE_CLASS_RENDER:
>> +                       /* Will be tested later. */
>> +                       break;
>> +               case I915_ENGINE_CLASS_COPY:
>> +                       igt_assert(gem_has_blt(fd));
>> +                       break;
>> +               case I915_ENGINE_CLASS_VIDEO:
>> +                       switch (engine->engine.engine_instance) {
>> +                       case 0:
>> +                               igt_assert(gem_has_bsd(fd));
>> +                               break;
>> +                       case 1:
>> +                               igt_assert(gem_has_bsd2(fd));
>> +                               break;
> 
> 
> Is that relationship a given?
> 
> One could argue that gem_has_blt() means that I915_EXEC_BLT works, but
> without !gem_has_blt() we could still access CLASS_COPY:0 via
> ctx->engines[].

"without !gem_has_blt()".. hmmm what do you mean? If you mean 
gem_has_blt() is false but bcs:0 is still accessible via ctx->engines[] 
then how?

>> +                       }
>> +                       break;
>> +               case I915_ENGINE_CLASS_VIDEO_ENHANCE:
>> +                       igt_assert(gem_has_vebox(fd));
>> +                       break;
>> +               default:
>> +                       igt_assert(0);
>> +               }
>> +       }
>> +
>> +       /* Reverse check to the above - all GET_PARAM engines are present. */
>> +       igt_assert(has_engine(engines, I915_ENGINE_CLASS_RENDER, 0));
>> +       if (gem_has_blt(fd))
>> +               igt_assert(has_engine(engines, I915_ENGINE_CLASS_COPY, 0));
>> +       if (gem_has_bsd(fd))
>> +               igt_assert(has_engine(engines, I915_ENGINE_CLASS_VIDEO, 0));
>> +       if (gem_has_bsd2(fd))
>> +               igt_assert(has_engine(engines, I915_ENGINE_CLASS_VIDEO, 1));
>> +       if (gem_has_vebox(fd))
>> +               igt_assert(has_engine(engines, I915_ENGINE_CLASS_VIDEO_ENHANCE,
>> +                                      0));
> 
> Whereas this should always make sense, given the legacy interface and
> the modern interface, the modern interface should be a superset of the
> legacy.
> 
> Just thinking aloud.

Sure, no harm in looking at it again. The test was written long time ago.

Regards,

Tvrtko