Re: [PATCH] drm/i915/selftests: Exercise potential false lite-restore

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 01/10/2019 13:22, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-10-01 13:16:19)
>>
>> On 01/10/2019 10:51, Chris Wilson wrote:
>>> +
>>> +                     /*
>>> +                      * Setup the pair of contexts such that if we
>>> +                      * lite-restore using the RING_TAIL from ce[1] it
>>> +                      * will execute garbage from ce[0]->ring.
>>> +                      */
>>> +                     memset(ce[n]->ring->vaddr,
>>> +                            POISON_INUSE,
>>> +                            ce[n]->ring->vma->size);
>>> +             }
>>> +             intel_ring_reset(ce[1]->ring, ce[1]->ring->vma->size / 2);
>>> +             __execlists_update_reg_state(ce[1], engine);
>>> +
>>> +             rq[0] = igt_spinner_create_request(&spin, ce[0], MI_ARB_CHECK);
>>> +             if (IS_ERR(rq[0])) {
>>> +                     err = PTR_ERR(rq[0]);
>>> +                     goto err_ce;
>>> +             }
>>> +
>>> +             GEM_BUG_ON(rq[0]->tail > ce[1]->ring->emit);
>>> +             i915_request_get(rq[0]);
>>> +             i915_request_add(rq[0]);
>>> +
>>> +             if (!igt_wait_for_spinner(&spin, rq[0])) {
>>> +                     i915_request_put(rq[0]);
>>> +                     goto err_ce;
>>> +             }
>>> +
>>> +             rq[1] = i915_request_create(ce[1]);
>>> +             if (IS_ERR(rq[1])) {
>>> +                     err = PTR_ERR(rq[1]);
>>> +                     i915_request_put(rq[0]);
>>> +                     goto err_ce;
>>> +             }
>>> +             GEM_BUG_ON(rq[1]->tail <= rq[0]->tail);
>>> +
>>> +             /* Ensure we do a completion switch from ce[0] to ce[1] */
>>> +             i915_request_await_dma_fence(rq[1], &rq[0]->fence);
>>
>> What do you mean by completion switch? You are setting up a dependency
>> so rq[1] (and rq[2]) won't be put into the elsp until spinner is ended
>> so it may not even be a context switch. Wouldn't you actually need the
>> opposite? I was expecting you would let the spinner run, make sure rq[1]
>> is in elsp and then count on time slicing to trigger a context switch.
> 
> The test I had in mind was to have
> 
> 	ELSP[0] = ce[0]
> 	ELSP[1] = ce[1]
> 
> and so chose to prevent any timeslicing reordering that. Same engine, so
> it will add a wait-on-submit-fence (i.e. a no-op) but would install the
> dependency link to prevent any reordering.

Ah my bad, did not think about the same engine optimisation. Expand the 
comment? :)

> A second test to have the spinner running then using priority to preempt
> it, seems like a good addition.

Priority it more controllable than timeslicing, true.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx