i915/gem_ctx_engine: Prevent preemption

Submitted by Chris Wilson on June 18, 2019, 8:21 p.m.

Details

Message ID 20190618202139.1481-1-chris@chris-wilson.co.uk
State New
Headers show
Series "i915/gem_ctx_engine: Prevent preemption" ( rev: 1 ) in IGT (deprecated)

Not browsing as part of any series.

Commit Message

Chris Wilson June 18, 2019, 8:21 p.m.
In order to pin the engine as busy, we have to prevent the kernel from
executing other independent work ahead of our plug, so tell the spinner
to not allow preemption.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/i915/gem_ctx_engines.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/tests/i915/gem_ctx_engines.c b/tests/i915/gem_ctx_engines.c
index 3ecade417..d47cbdd7c 100644
--- a/tests/i915/gem_ctx_engines.c
+++ b/tests/i915/gem_ctx_engines.c
@@ -283,7 +283,8 @@  static void execute_one(int i915)
 
 			spin = igt_spin_new(i915,
 					    .ctx = param.ctx_id,
-					    .engine = 0);
+					    .engine = 0,
+					    .flags = IGT_SPIN_NO_PREEMPTION);
 
 			igt_debug("Testing with map of %d engines\n", i + 1);
 			memset(&engines.engines, -1, sizeof(engines.engines));

Comments

On 18/06/2019 21:21, Chris Wilson wrote:
> In order to pin the engine as busy, we have to prevent the kernel from
> executing other independent work ahead of our plug, so tell the spinner
> to not allow preemption.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   tests/i915/gem_ctx_engines.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/i915/gem_ctx_engines.c b/tests/i915/gem_ctx_engines.c
> index 3ecade417..d47cbdd7c 100644
> --- a/tests/i915/gem_ctx_engines.c
> +++ b/tests/i915/gem_ctx_engines.c
> @@ -283,7 +283,8 @@ static void execute_one(int i915)
>   
>   			spin = igt_spin_new(i915,
>   					    .ctx = param.ctx_id,
> -					    .engine = 0);
> +					    .engine = 0,
> +					    .flags = IGT_SPIN_NO_PREEMPTION);
>   
>   			igt_debug("Testing with map of %d engines\n", i + 1);
>   			memset(&engines.engines, -1, sizeof(engines.engines));
> 

The no-op batch preempts the spinner? How does that affect the busy 
check on the no-op batch?

Regards,

Tvrtko
Quoting Tvrtko Ursulin (2019-06-19 07:32:05)
> 
> On 18/06/2019 21:21, Chris Wilson wrote:
> > In order to pin the engine as busy, we have to prevent the kernel from
> > executing other independent work ahead of our plug, so tell the spinner
> > to not allow preemption.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >   tests/i915/gem_ctx_engines.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tests/i915/gem_ctx_engines.c b/tests/i915/gem_ctx_engines.c
> > index 3ecade417..d47cbdd7c 100644
> > --- a/tests/i915/gem_ctx_engines.c
> > +++ b/tests/i915/gem_ctx_engines.c
> > @@ -283,7 +283,8 @@ static void execute_one(int i915)
> >   
> >                       spin = igt_spin_new(i915,
> >                                           .ctx = param.ctx_id,
> > -                                         .engine = 0);
> > +                                         .engine = 0,
> > +                                         .flags = IGT_SPIN_NO_PREEMPTION);
> >   
> >                       igt_debug("Testing with map of %d engines\n", i + 1);
> >                       memset(&engines.engines, -1, sizeof(engines.engines));
> > 
> 
> The no-op batch preempts the spinner? How does that affect the busy 
> check on the no-op batch?

We are checking the second batch in the pipeline, expecting it to be
blocked by the already executing spinner on the engine of interest.
There is no dependency between the two batches, just order of
submission, hence the kernel ends up executing the second no-op batch
after the first batch's timeslice expired.
-Chris
On 19/06/2019 09:34, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-06-19 07:32:05)
>>
>> On 18/06/2019 21:21, Chris Wilson wrote:
>>> In order to pin the engine as busy, we have to prevent the kernel from
>>> executing other independent work ahead of our plug, so tell the spinner
>>> to not allow preemption.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> ---
>>>    tests/i915/gem_ctx_engines.c | 3 ++-
>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tests/i915/gem_ctx_engines.c b/tests/i915/gem_ctx_engines.c
>>> index 3ecade417..d47cbdd7c 100644
>>> --- a/tests/i915/gem_ctx_engines.c
>>> +++ b/tests/i915/gem_ctx_engines.c
>>> @@ -283,7 +283,8 @@ static void execute_one(int i915)
>>>    
>>>                        spin = igt_spin_new(i915,
>>>                                            .ctx = param.ctx_id,
>>> -                                         .engine = 0);
>>> +                                         .engine = 0,
>>> +                                         .flags = IGT_SPIN_NO_PREEMPTION);
>>>    
>>>                        igt_debug("Testing with map of %d engines\n", i + 1);
>>>                        memset(&engines.engines, -1, sizeof(engines.engines));
>>>
>>
>> The no-op batch preempts the spinner? How does that affect the busy
>> check on the no-op batch?
> 
> We are checking the second batch in the pipeline, expecting it to be
> blocked by the already executing spinner on the engine of interest.
> There is no dependency between the two batches, just order of
> submission, hence the kernel ends up executing the second no-op batch
> after the first batch's timeslice expired.

Yes of course.. but even without timeslicing I think we could have this 
assumption spinner plugs in a lot of places.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
Quoting Tvrtko Ursulin (2019-06-19 09:44:18)
> 
> On 19/06/2019 09:34, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-06-19 07:32:05)
> >>
> >> On 18/06/2019 21:21, Chris Wilson wrote:
> >>> In order to pin the engine as busy, we have to prevent the kernel from
> >>> executing other independent work ahead of our plug, so tell the spinner
> >>> to not allow preemption.
> >>>
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>> ---
> >>>    tests/i915/gem_ctx_engines.c | 3 ++-
> >>>    1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/tests/i915/gem_ctx_engines.c b/tests/i915/gem_ctx_engines.c
> >>> index 3ecade417..d47cbdd7c 100644
> >>> --- a/tests/i915/gem_ctx_engines.c
> >>> +++ b/tests/i915/gem_ctx_engines.c
> >>> @@ -283,7 +283,8 @@ static void execute_one(int i915)
> >>>    
> >>>                        spin = igt_spin_new(i915,
> >>>                                            .ctx = param.ctx_id,
> >>> -                                         .engine = 0);
> >>> +                                         .engine = 0,
> >>> +                                         .flags = IGT_SPIN_NO_PREEMPTION);
> >>>    
> >>>                        igt_debug("Testing with map of %d engines\n", i + 1);
> >>>                        memset(&engines.engines, -1, sizeof(engines.engines));
> >>>
> >>
> >> The no-op batch preempts the spinner? How does that affect the busy
> >> check on the no-op batch?
> > 
> > We are checking the second batch in the pipeline, expecting it to be
> > blocked by the already executing spinner on the engine of interest.
> > There is no dependency between the two batches, just order of
> > submission, hence the kernel ends up executing the second no-op batch
> > after the first batch's timeslice expired.
> 
> Yes of course.. but even without timeslicing I think we could have this 
> assumption spinner plugs in a lot of places.

Yes. In the places where I've thought about this plugging issue, we
issue BIGNUM high priority requests. However, this is the only
consistent failure atm, no doubt more will turn up.
-Chris