[igt-dev] [PATCH i-g-t v2 1/1] lib/i915/gem_ring : set the engine to default context

[Chris Wilson]

Quoting Tvrtko Ursulin (2020-02-28 14:23:46)
> 
> On 28/02/2020 13:41, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2020-02-28 11:15:14)
> >>
> >> On 18/02/2020 08:22, Bommu Krishnaiah wrote:
> >>> Copy the existing engine map from default context to
> >>> newly created  default context
> >>>
> >>> v2: addressed review comments
> >>>
> >>> Signed-off-by: Bommu Krishnaiah <krishnaiah.bommu at intel.com>
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> >>> ---
> >>>    lib/i915/gem_ring.c              | 8 ++++++--
> >>>    lib/i915/gem_ring.h              | 2 +-
> >>>    tests/i915/gem_busy.c            | 2 +-
> >>>    tests/i915/gem_ctx_persistence.c | 2 +-
> >>>    tests/i915/gem_eio.c             | 6 +++---
> >>>    tests/i915/gem_exec_await.c      | 2 +-
> >>>    tests/i915/gem_exec_fence.c      | 2 +-
> >>>    tests/i915/gem_exec_latency.c    | 2 +-
> >>>    tests/i915/gem_exec_schedule.c   | 6 +++---
> >>>    tests/i915/gem_ringfill.c        | 2 +-
> >>>    tests/perf_pmu.c                 | 2 +-
> >>>    11 files changed, 20 insertions(+), 16 deletions(-)
> >>>
> >>> diff --git a/lib/i915/gem_ring.c b/lib/i915/gem_ring.c
> >>> index 99f4741c..14abfb16 100644
> >>> --- a/lib/i915/gem_ring.c
> >>> +++ b/lib/i915/gem_ring.c
> >>> @@ -143,11 +143,15 @@ __gem_measure_ring_inflight(int fd, unsigned int engine, enum measure_ring_flags
> >>>     * Number of batches that fit in the ring
> >>>     */
> >>>    unsigned int
> >>> -gem_measure_ring_inflight(int fd, unsigned int engine, enum measure_ring_flags flags)
> >>> +gem_measure_ring_inflight(int sfd, unsigned int engine, enum measure_ring_flags flags, bool copy_engine_map)
> >>>    {
> >>>        unsigned int min = ~0u;
> >>> +     int fd;
> >>>    
> >>> -     fd = gem_reopen_driver(fd);
> >>> +     fd = gem_reopen_driver(sfd);
> >>> +
> >>> +     if (copy_engine_map)
> >>> +             gem_context_copy_engines(sfd, 0, fd, 0);
> >>
> >> Would querying the context for presence of engine map be too hacky? It
> >> would avoid having to change the prototype etc. I guess it would be
> >> hacky since it could incorrectly imply from which kind of context
> >> 'engine' comes.
> >>
> >> Perhaps proper fix it to pass in fd/ctx tuple in here and then decide
> >> locally. That sounds better to me than the bool. Chris, your opinion?
> > 
> > I was just planning on adding USE_ENGINE to flags.
> > 
> > The reason we had to add gem_reopen_driver() here was someone converted
> > some tests to __for_each_physical_engine but later on in the same test
> > we used gem_measure_ring_inflight with a legacy ring. So I was wary of
> > that breaking again, and thought the exercise of making the USE_ENGINE
> > explicit would be useful review of the callers.
> 
> What would be the meaning of USE_ENGINE, I don't follow.

Currently engine means ring so USE_ENGINE would make engine mean
engine. :)

> I agree there is some attractiveness in marking the callers and so 
> making the "kind" of 'engine' explicit (and audited), however that 
> process in general is also very tedious and error prone.
> 
> Ctx+engine tuplet sounds more robust. If ctx has engine map we copy it 
> after re-opening the driver.
> 
> Or you want to drop re-opening? Then I don't see what a new flag could do.

I did think we could drop the re-open, but reopen is nice to get around
any prior state on the old fd affecting the measurement. So keep it.

What I think would be useful for future API is to use
intel_execution_engine2 (someone come up with a better name please!) as
our primary designator for physical engines. Then this would be

i915 = gem_reopen_driver(i915);
gem_context_set_engine(i915, 0, e->class, e->instance);

And we would always use eb.flags=0 for the measurement.

That way there's less ambiguity and we always have enough to create a
new ctx->engines[] map if desired.
-Chris