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

[Tvrtko Ursulin]

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.

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.

Regards,

Tvrtko