Re: [igt-dev] [PATCH i-g-t 2/3] i915/gem_ctx_shared: Make gem_ctx_shared understand static priority mapping

[Matthew Brost <matthew.brost@intel.com>]

On Tue, Oct 05, 2021 at 11:44:02AM +0100, Tvrtko Ursulin wrote:
> 
> On 05/10/2021 00:26, Daniele Ceraolo Spurio wrote:
> > 
> > 
> > On 9/16/2021 11:03 AM, Matthew Brost wrote:
> > > The i915 currently has 2k visible priority levels which are currently
> > > unique. This is changing to statically map these 2k levels into 3
> > > buckets:
> > > 
> > > low: < 0
> > > mid: 0
> > > high: > 0
> > > 
> > > Update gem_ctx_shared to understand this. This entails updating
> > > promotion test to use 3 levels that will map into different buckets and
> > > also add bit of delay after releasing a cork beforing completing the
> > > spinners to give time to the i915 schedule to process the fence and
> > > release and queue the requests.
> > > 
> > > v2: Add a delay between starting releasing spinner and cork in
> > > promotion
> > > v3:
> > >   (Daniele)
> > >    - Always add delay, update commit message
> > > 
> > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > > ---
> > >   tests/i915/gem_ctx_shared.c | 5 +++--
> > >   1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
> > > index ea1b5dd1b..7f88871b8 100644
> > > --- a/tests/i915/gem_ctx_shared.c
> > > +++ b/tests/i915/gem_ctx_shared.c
> > > @@ -622,6 +622,7 @@ static void unplug_show_queue(int i915, struct
> > > igt_cork *c, uint64_t ahnd,
> > >       igt_cork_unplug(c); /* batches will now be queued on the engine */
> > >       igt_debugfs_dump(i915, "i915_engine_info");
> > > +    usleep(250000);
> > 
> > Same as previous patch, with a comment added:
> > 
> > Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> 
> Come on guys, it is a bit bad and lazy for good taste no? 250ms more test

Yea, this is way too long of a sleep. 25ms seems just fine.

On TGL /w the updated sleep:
gem_ctx_shared --r *promotion*
IGT-Version: 1.26-g7e489b053 (x86_64) (Linux: 5.15.0-rc3-guc+ x86_64)
Starting subtest: Q-promotion
Starting dynamic subtest: rcs0
Dynamic subtest rcs0: SUCCESS (0.059s)
Starting dynamic subtest: bcs0
Dynamic subtest bcs0: SUCCESS (0.059s)
Starting dynamic subtest: vcs0
Dynamic subtest vcs0: SUCCESS (0.060s)
Starting dynamic subtest: vecs0
Dynamic subtest vecs0: SUCCESS (0.061s)
Subtest Q-promotion: SUCCESS (0.239s)

> runtime, multiplied by number of tests and subtests (engines) will add up
> and over shadows the current test time. For instance current state is:
> 
> # tests/gem_ctx_shared --r *promotion*
> IGT-Version: 1.26-NO-GIT (x86_64) (Linux: 5.15.0-rc4 x86_64)
> Starting subtest: Q-promotion
> Starting dynamic subtest: rcs0
> Dynamic subtest rcs0: SUCCESS (0.174s)
> Starting dynamic subtest: bcs0
> Dynamic subtest bcs0: SUCCESS (0.224s)
> Starting dynamic subtest: vcs0
> Dynamic subtest vcs0: SUCCESS (0.153s)
> Starting dynamic subtest: vecs0
> Dynamic subtest vecs0: SUCCESS (0.153s)
> Subtest Q-promotion: SUCCESS (0.708s)
> 
> This patch instantly makes this 1.7 seconds instead. Add the in/out order
> subtests, other tests, platforms with more engines, in a world where CI time
> budget is scarce - can't we do better than this and not settle on sprinkling
> of usleep all over the place?
> 
> Like maybe add out fences to the two requests, merge them, and wait on that
> (since there is no implicit write declared so set domain does not suffice)?
>

Not sure I follow. Anyways reposting now with a smaller timeout value.
Unless we hear something we plan on merging this tomorrow.

Matt 

> Unless I am missing something it would be strictly correct for execlists
> backend as well since it now works just because it is fast enough.
> 
> Regards,
> 
> Tvrtko
> 
> > 
> > Daniele
> > 
> > >       for (int n = 0; n < ARRAY_SIZE(spin); n++) {
> > >           ahnd = spin[n]->ahnd;
> > > @@ -831,10 +832,10 @@ static void promotion(int i915, const
> > > intel_ctx_cfg_t *cfg, unsigned ring)
> > >       gem_context_set_priority(i915, ctx[LO]->id, MIN_PRIO);
> > >       ctx[HI] = intel_ctx_create(i915, &q_cfg);
> > > -    gem_context_set_priority(i915, ctx[HI]->id, 0);
> > > +    gem_context_set_priority(i915, ctx[HI]->id, MAX_PRIO);
> > >       ctx[NOISE] = intel_ctx_create(i915, &q_cfg);
> > > -    gem_context_set_priority(i915, ctx[NOISE]->id, MIN_PRIO/2);
> > > +    gem_context_set_priority(i915, ctx[NOISE]->id, 0);
> > >       result = gem_create(i915, 4096);
> > >       result_offset = get_offset(ahnd, result, 4096, 0);
> >