[igt-dev] [Intel-gfx] [PATCH i-g-t] i915: Improve static engine map for legacy

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-05-23 10:35:01)
> 
> On 23/05/2019 10:14, Chris Wilson wrote:
> > Quoting Chris Wilson (2019-05-23 10:09:48)
> >> Quoting Tvrtko Ursulin (2019-05-23 09:46:08)
> >>>
> >>> On 23/05/2019 09:06, Chris Wilson wrote:
> >>>>    const struct intel_execution_engine2 intel_execution_engines2[] = {
> >>>> -     { "rcs0", I915_ENGINE_CLASS_RENDER, 0 },
> >>>> -     { "bcs0", I915_ENGINE_CLASS_COPY, 0 },
> >>>> -     { "vcs0", I915_ENGINE_CLASS_VIDEO, 0 },
> >>>> -     { "vcs1", I915_ENGINE_CLASS_VIDEO, 1 },
> >>>> -     { "vcs2", I915_ENGINE_CLASS_VIDEO, 2 },
> >>>> -     { "vecs0", I915_ENGINE_CLASS_VIDEO_ENHANCE, 0 },
> >>>> +     { "rcs0", I915_ENGINE_CLASS_RENDER, 0, I915_EXEC_RENDER },
> >>>> +     { "bcs0", I915_ENGINE_CLASS_COPY, 0, I915_EXEC_BLT },
> >>>> +     { "vcs0", I915_ENGINE_CLASS_VIDEO, 0, I915_EXEC_BSD | I915_EXEC_BSD_RING1 },
> >>>
> >>> execbuf will reject this on single vcs parts. :( Am I not seeing some
> >>> place where you fudge it into compliance?
> >>
> >>
> >>          if (user_ring_id == I915_EXEC_BSD && HAS_ENGINE(dev_priv, VCS1)) {
> >>                  unsigned int bsd_idx = args->flags & I915_EXEC_BSD_MASK;
> >>
> >>                  if (bsd_idx == I915_EXEC_BSD_DEFAULT) {
> >>                          bsd_idx = gen8_dispatch_bsd_engine(dev_priv, file);
> >>                  } else if (bsd_idx >= I915_EXEC_BSD_RING1 &&
> >>                             bsd_idx <= I915_EXEC_BSD_RING2) {
> >>                          bsd_idx >>= I915_EXEC_BSD_SHIFT;
> >>                          bsd_idx--;
> >>                  } else {
> >>                          DRM_DEBUG("execbuf with unknown bsd ring: %u\n",
> >>                                    bsd_idx);
> >>                          return NULL;
> >>                  }
> >>
> >>                  engine = dev_priv->engine[_VCS(bsd_idx)];
> >>          } else {
> >>                  engine = dev_priv->engine[user_ring_map[user_ring_id]];
> >>          }
> >>
> >> Looks ok to me... I was trying to double check but dif didn't boot on
> >> bsw. Sigh.
> > 
> > So instead I removed vcs1 from bdw, and vcs0 continues to work.
> > 
> > What is still incorrect though:
> > $ ./build/tests/gem_exec_parallel --list-subtests
> > ...
> > vcs0
> > vcs0-contexts
> > vcs0-fds
> > vcs1
> > vcs1-contexts
> > vcs1-fds
> > vcs2
> > vcs2-contexts
> > vcs2-fds
> > ...
> 
> As you know this part is correct since CI mandates we list subtests not 
> applicable to running platform.
> 
> > $ sudo ./build/tests/gem_exec_parallel --run vcs1
> > IGT-Version: 1.23-g82137ba4 (x86_64) (Linux: 5.2.0-rc1+ x86_64)
> > (gem_exec_parallel:16219) igt_core-WARNING: Unknown subtest: vcs1
> 
> I guess we need to make the library function igt_skip instead of not 
> enumerating the impossible and missing engines.
> 
> But it doesn't work.. skips need to be in either subtests or fixtures.
> 
> Perhaps the idea of using a single context aware iterator has therefore 
> fail and we do need two separate ones. :( One just for subtest 
> enumeration which *only* uses the static list, and another for use in 
> tests, which skips missing/impossible engines depending on context.

I'm willing to sweep this bit under the carpet so long as it isn't
flagged as a failure to Joonas.
-Chris