[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 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.
-Chris